bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[Eli Zaretskii]

> From: Kelly Dean <kelly@prtime.org>
> Date: Sat, 15 Nov 2014 06:57:45 +0000
> 
> With 24.4, emacs -Q, then M-x split-window-horizontally, then open any file with a lot of long logical lines (each a few visual lines long), so that as you page down through the file, every screenful of text has some wrapped lines on it. Adjust the vertical size of your Emacs frame so that the vertical size of your Emacs windows is not an integer multiple of the visual line height, so that the bottom visual line of text is partially covered by the top of the mode line.
> Scroll forward a couple pages using pgdn (scroll-up-command), then notice exactly which visual line is at the top, and which one is at the bottom. Press pgdn, then pgup, and notice that the text doesn't return to the previous position; instead, it's scrolled backward by one visual line. This bug wasn't in 24.3.
> 
> The bug is in window.c of the 24.4 release, line 5042. The fix is to change ⌜window_box_height (w)⌝ on that line to ⌜window_box_height (w) / dy * dy⌝ in order to align the result to an integer multiple of the visual line height, since as of 24.4, Emacs no longer ensures that window_box_height is such a multiple. Patch excluded, per request.
> 
> There might be a related bug (with the same fix) on line 4959, but I haven't tested that.


Martin, I propose the following patch:

diff --git a/src/window.c b/src/window.c
index b002423..7462fdc 100644
--- a/src/window.c
+++ b/src/window.c
@@ -4956,8 +4956,8 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, int noerror)
 	  int px;
 	  int dy = frame_line_height;
 	  if (whole)
-	    dy = max ((window_box_height (w)
-		       - next_screen_context_lines * dy),
+	    dy = max ((window_box_height (w) / dy
+		       - next_screen_context_lines) * dy,
 		      dy);
 	  dy *= n;
 
@@ -5039,8 +5039,7 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, int noerror)
     {
       ptrdiff_t start_pos = IT_CHARPOS (it);
       int dy = frame_line_height;
-      dy = max ((window_box_height (w)
-		 - next_screen_context_lines * dy),
+      dy = max ((window_box_height (w) / dy - next_screen_context_lines) * dy,
 		dy) * n;
 
       /* Note that move_it_vertically always moves the iterator to the


> BTW on line 471 (copied from line 466) of window.h, ‟width” should be ‟height”.

Indeed, and "column" should be "line".  Thanks, fixed.

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[martin rudalics]

First of all many thanks to Kelly Dean for finding the problem and
proposing a solution.

 > Martin, I propose the following patch:
 >
 > diff --git a/src/window.c b/src/window.c
 > index b002423..7462fdc 100644
 > --- a/src/window.c
 > +++ b/src/window.c
 > @@ -4956,8 +4956,8 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, int noerror)
 >   	  int px;
 >   	  int dy = frame_line_height;
 >   	  if (whole)
 > -	    dy = max ((window_box_height (w)
 > -		       - next_screen_context_lines * dy),
 > +	    dy = max ((window_box_height (w) / dy
 > +		       - next_screen_context_lines) * dy,
 >   		      dy);
 >   	  dy *= n;
 >
 > @@ -5039,8 +5039,7 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, int noerror)
 >       {
 >         ptrdiff_t start_pos = IT_CHARPOS (it);
 >         int dy = frame_line_height;
 > -      dy = max ((window_box_height (w)
 > -		 - next_screen_context_lines * dy),
 > +      dy = max ((window_box_height (w) / dy - next_screen_context_lines) * dy,
 >   		dy) * n;
 >
 >         /* Note that move_it_vertically always moves the iterator to the

If it works for you, please apply it.  I'm too silly to understand it,
though.  IIUC we have a rounding issue which makes us go by one line too
far or too few when scrolling up and your patch compensates that
rounding issue by putting it back into that window_box_height (w) / dy
calculation.  Could you add comments which tell more or less how you
corrected the issue?

Thanks, martin

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[Eli Zaretskii]

> Date: Sat, 15 Nov 2014 12:13:17 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: 19060@debbugs.gnu.org
> 
>  > diff --git a/src/window.c b/src/window.c
>  > index b002423..7462fdc 100644
>  > --- a/src/window.c
>  > +++ b/src/window.c
>  > @@ -4956,8 +4956,8 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, int noerror)
>  >   	  int px;
>  >   	  int dy = frame_line_height;
>  >   	  if (whole)
>  > -	    dy = max ((window_box_height (w)
>  > -		       - next_screen_context_lines * dy),
>  > +	    dy = max ((window_box_height (w) / dy
>  > +		       - next_screen_context_lines) * dy,
>  >   		      dy);
>  >   	  dy *= n;
>  >
>  > @@ -5039,8 +5039,7 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, int noerror)
>  >       {
>  >         ptrdiff_t start_pos = IT_CHARPOS (it);
>  >         int dy = frame_line_height;
>  > -      dy = max ((window_box_height (w)
>  > -		 - next_screen_context_lines * dy),
>  > +      dy = max ((window_box_height (w) / dy - next_screen_context_lines) * dy,
>  >   		dy) * n;
>  >
>  >         /* Note that move_it_vertically always moves the iterator to the
> 
> If it works for you, please apply it.

It evidently works for Kelly.

> I'm too silly to understand it, though.  IIUC we have a rounding
> issue which makes us go by one line too far or too few when
> scrolling up and your patch compensates that rounding issue by
> putting it back into that window_box_height (w) / dy calculation.

Yes.  Why is it hard to understand?

> Could you add comments which tell more or less how you corrected the
> issue?

Will do, once we agree that this is the right fix.

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[martin rudalics]

 > Yes.  Why is it hard to understand?

When I try to correct an off-by-one error I usually never understand
what I'm doing.  I "fix" it by rounding in one direction first and if
this doesn't seem to work as intended I try the other direction.

 >> Could you add comments which tell more or less how you corrected the
 >> issue?
 >
 > Will do, once we agree that this is the right fix.

First of all I'd need to understand the bug itself.  Do we assume that
scrolling down is "correct" in some sense or does the bug manifest
itself there already?  Does scrolling up always move to the line before
the "previously established as correct" one or to the line after it?
(I'm asking because I cannot reproduce the problem with Kelly Dean's
pgup/pgdn recipe.)

martin

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[Eli Zaretskii]

> Date: Sat, 15 Nov 2014 13:12:47 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: kelly@prtime.org, 19060@debbugs.gnu.org
> 
> First of all I'd need to understand the bug itself.  Do we assume that
> scrolling down is "correct" in some sense or does the bug manifest
> itself there already?  Does scrolling up always move to the line before
> the "previously established as correct" one or to the line after it?
> (I'm asking because I cannot reproduce the problem with Kelly Dean's
> pgup/pgdn recipe.)

Kelly, please help Martin reproduce the problem.  Perhaps you need to
provide more details, like maybe the file you used and the geometry of
the frame needed to see the issue.

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[martin rudalics]

 > Kelly, please help Martin reproduce the problem.  Perhaps you need to
 > provide more details, like maybe the file you used and the geometry of
 > the frame needed to see the issue.

Just a few informations.  I tried on Windows XP with the current trunk
(the release doesn't allow to resize frames pixelwise easily).  I did
emacs -Q, displayed xdisp.c, did C-x 3 and resized the frame with the
mouse to make sure that the last lines of the windows showing xdisp.c do
get displayed only partially.  Now pgup/pgdn in the right window always
gets me to the same line it had before when scrolling in the other
direction.  Am I missing something?

martin

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[Eli Zaretskii]

> Date: Sat, 15 Nov 2014 14:29:12 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: kelly@prtime.org, 19060@debbugs.gnu.org
> 
> Just a few informations.  I tried on Windows XP with the current trunk
> (the release doesn't allow to resize frames pixelwise easily).  I did
> emacs -Q, displayed xdisp.c, did C-x 3 and resized the frame with the
> mouse to make sure that the last lines of the windows showing xdisp.c do
> get displayed only partially.  Now pgup/pgdn in the right window always
> gets me to the same line it had before when scrolling in the other
> direction.  Am I missing something?

Since the issue is rounding vs truncating, perhaps try playing with
the portion of the last displayed line, trying both less and greater
than half the canonical line height.

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[martin rudalics]

 > Since the issue is rounding vs truncating, perhaps try playing with
 > the portion of the last displayed line, trying both less and greater
 > than half the canonical line height.

Got it.  The last line must show more than a half of the frame's
character height.  And the patch fixes it AFAICT.  So please apply it
with as much comment as you can provide.

Thanks, martin

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[Eli Zaretskii]

> Date: Sat, 15 Nov 2014 15:32:41 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: kelly@prtime.org, 19060@debbugs.gnu.org
> 
>  > Since the issue is rounding vs truncating, perhaps try playing with
>  > the portion of the last displayed line, trying both less and greater
>  > than half the canonical line height.
> 
> Got it.  The last line must show more than a half of the frame's
> character height.  And the patch fixes it AFAICT.  So please apply it
> with as much comment as you can provide.

Done on the emacs-24 branch.

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[Angelo Graziosi]

Martin Rudalics wrote:
> IIUC we have a rounding issue which makes us go by one line too
> far or too few when scrolling up and your patch compensates that
> rounding issue by putting it back into that window_box_height (w) / dy
> calculation.

Apparently, this seem related, in some way, to what I flagged here:

  http://lists.gnu.org/archive/html/bug-gnu-emacs/2014-11/msg00621.html

..or not?

Ciao,
  Angelo.

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[martin rudalics]

 > Apparently, this seem related, in some way, to what I flagged here:
 >
 >   http://lists.gnu.org/archive/html/bug-gnu-emacs/2014-11/msg00621.html
 >
 > ..or not?

Maybe.  But I have no idea what your report describes.  IIUC you are
using `desktop-save' to save the window configuration across sessions
and when you restart Emacs its frame's height is one line less.  Is that
correct?  Then please tell us how much the height decreases in pixels
(you can use the function `window--dump-frame' for that) and what the
height stored by `desktop-save' is.  I suppose this is somewhere in a
cons whose car is something like `height' or `frame-height' or the like.

martin

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[Angelo Graziosi]

[-- Attachment #1: Type: text/plain, Size: 1687 bytes --]



Il 15/11/2014 15:33, martin rudalics ha scritto:
>  > Apparently, this seem related, in some way, to what I flagged here:
>  >
>  >   http://lists.gnu.org/archive/html/bug-gnu-emacs/2014-11/msg00621.html
>  >
>  > ..or not?
>
> Maybe.  But I have no idea what your report describes.  IIUC you are
> using `desktop-save' to save the window configuration across sessions
> and when you restart Emacs its frame's height is one line less.  Is that
> correct?  Then please tell us how much the height decreases in pixels

Yes, even if each time not always the same size: sometimes more, 
sometimes less. I have quantified this in about one line (or, if you 
prefer, about the height of the minibuffer). I would say 10-20 pixel.

> (you can use the function `window--dump-frame' for that) and what the
> height stored by `desktop-save' is.  I suppose this is somewhere in a
> cons whose car is something like `height' or `frame-height' or the like.

I am afraid, I wasn't able to use that function.. How to evaluate it? I 
tried a few command but they didn't work.

I have attached a tar-ball with a few examples of desktop file. They 
have been obtained in this way.

First I removed the current desktop and history files. So I started 
Emacs and moved it so that it was centered in the Windows desktop (I 
tried this with the Cygwin build..). Then I closed Emacs and it asked to 
save the desktop. Yes - and this generated the "desktop-00" file. Then I 
restarted Emacs and closed it obtaining the "desktop-01" file. I 
repeated this 2 times obtaining "desktop-03". After repeating this a few 
times I obtained the "desktop" file: now Emacs frame is conspicuously short.


Ciao,
  Angelo.

[-- Attachment #2: desktop_files.tar.gz --]
[-- Type: application/x-gzip, Size: 1324 bytes --]

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[martin rudalics]

 >> (you can use the function `window--dump-frame' for that) and what the
 >> height stored by `desktop-save' is.  I suppose this is somewhere in a
 >> cons whose car is something like `height' or `frame-height' or the like.
 >
 > I am afraid, I wasn't able to use that function.. How to evaluate it? I tried a few command but they didn't work.

M-: (window--dump-frame)

The results appear in a buffer called *window-frame-dump*.  Do this
please once before exiting one session and once after entering the
subsequent session and post the respective buffer contents here.  And
please do that in the appropriate thread, namely for bug#19048.

 > First I removed the current desktop and history files. So I started
 > Emacs and moved it so that it was centered in the Windows desktop (I
 > tried this with the Cygwin build..). Then I closed Emacs and it asked
 > to save the desktop. Yes - and this generated the "desktop-00"
 > file. Then I restarted Emacs and closed it obtaining the "desktop-01"
 > file. I repeated this 2 times obtaining "desktop-03". After repeating
 > this a few times I obtained the "desktop" file: now Emacs frame is
 > conspicuously short.

I suppose the frame height is stored in the (height . xx) conses.  In
desktop-00 you have (height . 34), in desktop-01 (height . 33) and in
desktop-03 (height . 31).  So your frame height seems to decrease by one
line every time you restore it.  Using `window--dump-frame' we should be
able to see what the frame height was before exiting one session and
after restoring it in another one and hopefully why the frame decreases
by that line.

Thanks, martin

bug#19060: [FIX INCLUDED] Off-by-one-line scrolling bug in window_scroll_pixel_based

[Kelly Dean]

With 24.4, emacs -Q, then M-x split-window-horizontally, then open any file with a lot of long logical lines (each a few visual lines long), so that as you page down through the file, every screenful of text has some wrapped lines on it. Adjust the vertical size of your Emacs frame so that the vertical size of your Emacs windows is not an integer multiple of the visual line height, so that the bottom visual line of text is partially covered by the top of the mode line.
Scroll forward a couple pages using pgdn (scroll-up-command), then notice exactly which visual line is at the top, and which one is at the bottom. Press pgdn, then pgup, and notice that the text doesn't return to the previous position; instead, it's scrolled backward by one visual line. This bug wasn't in 24.3.

The bug is in window.c of the 24.4 release, line 5042. The fix is to change ⌜window_box_height (w)⌝ on that line to ⌜window_box_height (w) / dy * dy⌝ in order to align the result to an integer multiple of the visual line height, since as of 24.4, Emacs no longer ensures that window_box_height is such a multiple. Patch excluded, per request.

There might be a related bug (with the same fix) on line 4959, but I haven't tested that.

BTW on line 471 (copied from line 466) of window.h, ‟width” should be ‟height”.