bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[lompik]

Hi,

The command `forward-line` fails (no error message) inside a `with-selected-window` statement with 

Here's some steps to reproduce this bug. It is not reproducible with 24.3.

1. emacs -Q
2. In scratch copy these line and C-M-x

(global-set-key (kbd "C-`") '(lambda ()
(interactive)
(with-selected-window (get-buffer-window "*Completions*")
(forward-line 1))))

3. Use these commands:

C-x C-f (select one with lots of file in directory)
C-x C-+ (increase font size)
C-+ 
tab 
tab (open *Completion* window)
C-x o (other-window)
C-x o 
C-x C-+ (increase font size in *Completion* window, you might need more C-+)
C-+ 
C-+
C-+
C-+ 
C-x o (switch back to mnibuffer)
C-h k a (as help while in minibuffer, Important!!)
C-` (forward-line in *Completion* window)
C-` 
C-` 
C-`
C-`
.....(at some point fails to scroll screen

Normal expected behavior is to be able to type C-` until the end of the list in the *Completions* window. With this bug, you will be unable to go past a line that is 
truncated or half displayed. (for example https://cloud.githubusercontent.com/assets/3791334/4373312/be814774-432b-11e4-8add-47167bc1ef9c.png)

Regards !


___________________________________________________________
Mode, hifi, maison,… J'achète malin. Je compare les prix avec Voila.fr http://shopping.voila.fr/

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Stefan Monnier]

> The command `forward-line` fails (no error message) inside
> a `with-selected-window` statement with

Could you try it in Emacs-24.3.93 (which is the main focus for
bug-fixing right now)?


        Stefan

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[lompik]

I confirm the issue on Emacs 24.3.93 (build on 2014-08-15).


> Message du 24/09/14 à 17h14
> De : "Stefan Monnier" 
> A : lompik@voila.fr
> Copie à : 18545@debbugs.gnu.org
> Objet : Re: bug#18545: 24.4.50: Bug - forward-line inside with-selected-window
> 
> > The command `forward-line` fails (no error message) inside
> > a `with-selected-window` statement with
> 
> Could you try it in Emacs-24.3.93 (which is the main focus for
> bug-fixing right now)?
> 
> 
> Stefan
> 

___________________________________________________________
Mode, hifi, maison,… J'achète malin. Je compare les prix avec Voila.fr http://shopping.voila.fr/

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Wed, 24 Sep 2014 17:50:43 +0200 (CEST)
> From: lompik@voila.fr
> Cc: 18545@debbugs.gnu.org
> 
> I confirm the issue on Emacs 24.3.93 (build on 2014-08-15).
> 
> 
> > Message du 24/09/14 à 17h14
> > De : "Stefan Monnier" 
> > A : lompik@voila.fr
> > Copie à : 18545@debbugs.gnu.org
> > Objet : Re: bug#18545: 24.4.50: Bug - forward-line inside with-selected-window
> > 
> > > The command `forward-line` fails (no error message) inside
> > > a `with-selected-window` statement with

If you add

  (message "%s" (point))

to the form inside with-selected-window, you will see that
forward-line does its job perfectly.  Also, the line number in the
mode line gets incremented, even if you don't make the above addition.
The problem is that the window showing the *Completions* buffer is not
scrolled to bring point into view.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

This works OK in v24.3, so it's a regression.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 18545@debbugs.gnu.org
> 
> This works OK in v24.3, so it's a regression.

The patch below seems to fix the problem.  It catches the situation
where try_window failed to redisplay the window, in which case we
shouldn't "goto done" as if we succeeded.


=== modified file 'src/xdisp.c'
--- src/xdisp.c	2014-09-18 15:10:33 +0000
+++ src/xdisp.c	2014-09-25 15:15:42 +0000
@@ -16293,6 +16293,11 @@ redisplay_window (Lisp_Object window, bo
 	    }
 	  */
 	}
+      else if (w->cursor.vpos < 0)
+	{
+	  clear_glyph_matrix (w->desired_matrix);
+	  goto try_to_scroll;
+	}
 
 #ifdef GLYPH_DEBUG
       debug_method_add (w, "forced window start");

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[lompik]

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

It does if the point goes completely off the window. However it still does not scroll in the case where the pointer is partly off window and partly inside. In the 
screenshot attached, the pointer is just above the modeline and I am unable to calling forward-line has no effect. The cursor got in this current position by a 
series of forward-line.
Hopefully this is clear.

> Message du 25/09/14 à 17h20
> De : "Eli Zaretskii" 
> A : lompik@voila.fr
> Copie à : 18545@debbugs.gnu.org
> Objet : Re: bug#18545: 24.4.50: Bug - forward-line inside with-selected-window
> 
> > From: Eli Zaretskii 
> > Cc: 18545@debbugs.gnu.org
> > 
> > This works OK in v24.3, so it's a regression.
> 
> The patch below seems to fix the problem. It catches the situation
> where try_window failed to redisplay the window, in which case we
> shouldn't "goto done" as if we succeeded.
> 
> 
> === modified file 'src/xdisp.c'
> --- src/xdisp.c 2014-09-18 15:10:33 +0000
> +++ src/xdisp.c 2014-09-25 15:15:42 +0000
> @@ -16293,6 +16293,11 @@ redisplay_window (Lisp_Object window, bo
> }
> */
> }
> + else if (w->cursor.vpos < 0)
> + {
> + clear_glyph_matrix (w->desired_matrix);
> + goto try_to_scroll;
> + }
> 
> #ifdef GLYPH_DEBUG
> debug_method_add (w, "forced window start");
> 
> 

___________________________________________________________
Mode, hifi, maison,… J'achète malin. Je compare les prix avec Voila.fr http://shopping.voila.fr/

[-- Attachment #2: cursoroff.png --]
[-- Type: image/png, Size: 10509 bytes --]

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Thu, 25 Sep 2014 19:41:46 +0200 (CEST)
> From: lompik@voila.fr
> Cc: 18545@debbugs.gnu.org
> 
> It does if the point goes completely off the window. However it still does not scroll in the case where the pointer is partly off window and partly inside.

And that is _after_ applying the patch?

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[lompik]

yes _after_ the patch.

> Message du 25/09/14 à 19h51
> De : "Eli Zaretskii" 
> A : lompik@voila.fr
> Copie à : 18545@debbugs.gnu.org
> Objet : Re: bug#18545: 24.4.50: Bug - forward-line inside with-selected-window
> 
> > Date: Thu, 25 Sep 2014 19:41:46 +0200 (CEST)
> > From: lompik@voila.fr
> > Cc: 18545@debbugs.gnu.org
> > 
> > It does if the point goes completely off the window. However it still does not scroll in the case where the pointer is partly off window and partly inside.
> 
> And that is _after_ applying the patch?
> 

___________________________________________________________
Mode, hifi, maison,… J'achète malin. Je compare les prix avec Voila.fr http://shopping.voila.fr/

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Thu, 25 Sep 2014 20:14:36 +0200 (CEST)
> From: lompik@voila.fr
> Cc: 18545@debbugs.gnu.org
> 
> 
> yes _after_ the patch.

Can you tell me how to reproduce this?  I don't see this in the recipe
you described in your bug report.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[lompik]

I will have to take some time to do some more research provide you with a reproducible minimal example.. The one I have now involves the *helm* library.


> Message du 25/09/14 à 20h31
> De : "Eli Zaretskii" 
> A : lompik@voila.fr
> Copie à : 18545@debbugs.gnu.org
> Objet : Re: bug#18545: 24.4.50: Bug - forward-line inside with-selected-window
> 
> > Date: Thu, 25 Sep 2014 20:14:36 +0200 (CEST)
> > From: lompik@voila.fr
> > Cc: 18545@debbugs.gnu.org
> > 
> > 
> > yes _after_ the patch.
> 
> Can you tell me how to reproduce this? I don't see this in the recipe
> you described in your bug report.
> 

___________________________________________________________
Mode, hifi, maison,… J'achète malin. Je compare les prix avec Voila.fr http://shopping.voila.fr/

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Thu, 25 Sep 2014 21:06:00 +0200 (CEST)
> From: lompik@voila.fr
> Cc: 18545@debbugs.gnu.org
> 
> I will have to take some time to do some more research provide you with a reproducible minimal example.

Thanks in advance.  Having a minimal reproducer means we are half-way
to a solution.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[lompik]

here's a related scrolling involving the lisp function recenter:

1. emacs -Q
2. define those binding:


(global-set-key (kbd "C-`") '(lambda ()
(interactive)
(with-selected-window (get-buffer-window "*Completions*")
(forward-line 1))))

(global-set-key (kbd "C-~") '(lambda ()
(interactive)
(with-selected-window (get-buffer-window "*Completions*")
(forward-line -1))))

(global-set-key (kbd "C-;") '(lambda ()
(interactive)
(with-selected-window (get-buffer-window "*Completions*")
(recenter 5))))

3. do:

C-x C-f -> in a directory with lots of file.
tab tab -> open completion buffer
C-` -> to until scrolling down the *completion* buffer

hit "C-;" -> nothing happens. it should recenter. 
hit "C-'`" -> it recenter then move one line down

Regards


> Message du 25/09/14 à 21h18
> De : "Eli Zaretskii" 
> A : lompik@voila.fr
> Copie à : 18545@debbugs.gnu.org
> Objet : Re: bug#18545: 24.4.50: Bug - forward-line inside with-selected-window
> 
> > Date: Thu, 25 Sep 2014 21:06:00 +0200 (CEST)
> > From: lompik@voila.fr
> > Cc: 18545@debbugs.gnu.org
> > 
> > I will have to take some time to do some more research provide you with a reproducible minimal example.
> 
> Thanks in advance. Having a minimal reproducer means we are half-way
> to a solution.
> 

___________________________________________________________
Mode, hifi, maison,… J'achète malin. Je compare les prix avec Voila.fr http://shopping.voila.fr/

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Thu, 25 Sep 2014 22:05:36 +0200 (CEST)
> From: lompik@voila.fr
> Cc: 18545@debbugs.gnu.org
> 
> here's a related scrolling involving the lisp function recenter:
> 
> 1. emacs -Q
> 2. define those binding:
> 
> 
> (global-set-key (kbd "C-`") '(lambda ()
> (interactive)
> (with-selected-window (get-buffer-window "*Completions*")
> (forward-line 1))))
> 
> (global-set-key (kbd "C-~") '(lambda ()
> (interactive)
> (with-selected-window (get-buffer-window "*Completions*")
> (forward-line -1))))
> 
> (global-set-key (kbd "C-;") '(lambda ()
> (interactive)
> (with-selected-window (get-buffer-window "*Completions*")
> (recenter 5))))
> 
> 3. do:
> 
> C-x C-f -> in a directory with lots of file.
> tab tab -> open completion buffer
> C-` -> to until scrolling down the *completion* buffer
> 
> hit "C-;" -> nothing happens. it should recenter. 
> hit "C-'`" -> it recenter then move one line down

I'm not sure this is related to this bug.  The problem here was that
the display engine was not considering for redisplay the window
showing the *Completions* buffer.  The patch below fixes that.

=== modified file 'src/window.c'
--- src/window.c	2014-09-11 08:47:34 +0000
+++ src/window.c	2014-09-26 07:28:02 +0000
@@ -5897,6 +5897,8 @@ and redisplay normally--don't erase and 
   w->start_at_line_beg = (bytepos == BEGV_BYTE ||
 			  FETCH_BYTE (bytepos - 1) == '\n');
 
+  wset_redisplay (w);
+
   set_buffer_internal (obuf);
   return Qnil;
 }

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Stefan Monnier]

> I'm not sure this is related to this bug.  The problem here was that
> the display engine was not considering for redisplay the window
> showing the *Completions* buffer.  The patch below fixes that.

> === modified file 'src/window.c'
> --- src/window.c	2014-09-11 08:47:34 +0000
> +++ src/window.c	2014-09-26 07:28:02 +0000
> @@ -5897,6 +5897,8 @@ and redisplay normally--don't erase and 
w-> start_at_line_beg = (bytepos == BEGV_BYTE ||
>  			  FETCH_BYTE (bytepos - 1) == '\n');
 
> +  wset_redisplay (w);
> +
>    set_buffer_internal (obuf);
>    return Qnil;
>  }

Hmm... Now that make me wonder:
Why does

 (with-selected-window (get-buffer-window "*Completions*")
   (recenter 5))

require an explicit call to wset_redisplay from recenter, whereas

 (with-selected-window (get-buffer-window "*Completions*")
   (forward-line 1))

doesn't need an explicit call to wset_redisplay (or bset_redisplay) from
forward-line?


        Stefan

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: lompik@voila.fr,  18545@debbugs.gnu.org
> Date: Fri, 26 Sep 2014 08:48:27 -0400
> 
> Why does
> 
>  (with-selected-window (get-buffer-window "*Completions*")
>    (recenter 5))
> 
> require an explicit call to wset_redisplay from recenter, whereas
> 
>  (with-selected-window (get-buffer-window "*Completions*")
>    (forward-line 1))
> 
> doesn't need an explicit call to wset_redisplay (or bset_redisplay) from
> forward-line?

I think that's because forward-line moves point, while recenter
doesn't.

But if you'd like to add wset_redisplay to forward-line, I won't
object.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Stefan Monnier]

>> Why does
>> (with-selected-window (get-buffer-window "*Completions*")
>> (recenter 5))
>> require an explicit call to wset_redisplay from recenter, whereas
>> (with-selected-window (get-buffer-window "*Completions*")
>> (forward-line 1))
>> doesn't need an explicit call to wset_redisplay (or bset_redisplay) from
>> forward-line?
> I think that's because forward-line moves point, while recenter
> doesn't.

But I don't see why moving point would help: calling wset_redisplay
should only change the fact that this window is considered for
redisplay, so if it's needed for the recenter case, that means that
without it, the window would not be considered at all, which in turn
should imply that in the forward-line case we wouldn't notice that point
has changed, unless set_point_both ends up calling bset_redisplay
somehow (but I fail to see how).

> But if you'd like to add wset_redisplay to forward-line, I won't
> object.

If it's not needed, I'd rather not add it.


        Stefan

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: lompik@voila.fr,  18545@debbugs.gnu.org
> Date: Fri, 26 Sep 2014 10:13:45 -0400
> 
> >> Why does
> >> (with-selected-window (get-buffer-window "*Completions*")
> >> (recenter 5))
> >> require an explicit call to wset_redisplay from recenter, whereas
> >> (with-selected-window (get-buffer-window "*Completions*")
> >> (forward-line 1))
> >> doesn't need an explicit call to wset_redisplay (or bset_redisplay) from
> >> forward-line?
> > I think that's because forward-line moves point, while recenter
> > doesn't.
> 
> But I don't see why moving point would help: calling wset_redisplay
> should only change the fact that this window is considered for
> redisplay

There are redisplay optimizations that don't depend on whether we
consider a window for redisplay; see around line 13700 in xdisp.c from
emacs-24.  You will see a little ways below that place that we test
point against its recorded value in w->last_point, for example.

> so if it's needed for the recenter case, that means that
> without it, the window would not be considered at all

'with-selected-window makes' the offending window selected, and the
selected window is always considered for redisplay, right?

> which in turn should imply that in the forward-line case we wouldn't
> notice that point has changed, unless set_point_both ends up calling
> bset_redisplay somehow (but I fail to see how).

If you really want me to come up with a detailed explanation of why
the forward-line case doesn't need wset_redisplay, I can do that, but
it will require some digging.

My fixes where based on turning on trace-redisplay and observing
thereafter that, in the forward-line case the offending window was
shown in the trace, but always with an indication that the forced
window-start was OK, whereas in the recenter case the offending window
was not shown at all in the trace.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Stefan Monnier]

>> >> Why does
>> >> (with-selected-window (get-buffer-window "*Completions*")
>> >> (recenter 5))
>> >> require an explicit call to wset_redisplay from recenter, whereas
>> >> (with-selected-window (get-buffer-window "*Completions*")
>> >> (forward-line 1))
>> >> doesn't need an explicit call to wset_redisplay (or bset_redisplay) from
>> >> forward-line?
>> > I think that's because forward-line moves point, while recenter
>> > doesn't.
>> But I don't see why moving point would help: calling wset_redisplay
>> should only change the fact that this window is considered for
>> redisplay
> There are redisplay optimizations that don't depend on whether we
> consider a window for redisplay; see around line 13700 in xdisp.c from
> emacs-24.  You will see a little ways below that place that we test
> point against its recorded value in w->last_point, for example.

Right, but these are only for the current-buffer/selected-window,
whereas the example modifies another window (and another buffer), so
they first need to be considered (via [bwf]set_redisplay) before
anything else will look at them.

>> so if it's needed for the recenter case, that means that
>> without it, the window would not be considered at all
> 'with-selected-window makes' the offending window selected, and the
> selected window is always considered for redisplay, right?

No.  If it were the wset_redisplay you added would have been a no-op.

Oh, wait I see it now.  We do test if point changed, in
redisplay_window:15965:

  if (!just_this_one_p
      && REDISPLAY_SOME_P ()
      && !w->redisplay
      && !f->redisplay
      && !buffer->text->redisplay
      && BUF_PT (buffer) == w->last_point)
    return;

So indeed changing point ends up doing the moral equivalent of bset_redisplay.


        Stefan

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Fri, 26 Sep 2014 10:31:18 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 18545@debbugs.gnu.org
> 
> > Date: Thu, 25 Sep 2014 22:05:36 +0200 (CEST)
> > From: lompik@voila.fr
> > Cc: 18545@debbugs.gnu.org
> > 
> > here's a related scrolling involving the lisp function recenter:
> > 
> > 1. emacs -Q
> > 2. define those binding:
> > 
> > 
> > (global-set-key (kbd "C-`") '(lambda ()
> > (interactive)
> > (with-selected-window (get-buffer-window "*Completions*")
> > (forward-line 1))))
> > 
> > (global-set-key (kbd "C-~") '(lambda ()
> > (interactive)
> > (with-selected-window (get-buffer-window "*Completions*")
> > (forward-line -1))))
> > 
> > (global-set-key (kbd "C-;") '(lambda ()
> > (interactive)
> > (with-selected-window (get-buffer-window "*Completions*")
> > (recenter 5))))
> > 
> > 3. do:
> > 
> > C-x C-f -> in a directory with lots of file.
> > tab tab -> open completion buffer
> > C-` -> to until scrolling down the *completion* buffer
> > 
> > hit "C-;" -> nothing happens. it should recenter. 
> > hit "C-'`" -> it recenter then move one line down
> 
> I'm not sure this is related to this bug.  The problem here was that
> the display engine was not considering for redisplay the window
> showing the *Completions* buffer.  The patch below fixes that.
> 
> === modified file 'src/window.c'
> --- src/window.c	2014-09-11 08:47:34 +0000
> +++ src/window.c	2014-09-26 07:28:02 +0000
> @@ -5897,6 +5897,8 @@ and redisplay normally--don't erase and 
>    w->start_at_line_beg = (bytepos == BEGV_BYTE ||
>  			  FETCH_BYTE (bytepos - 1) == '\n');
>  
> +  wset_redisplay (w);
> +
>    set_buffer_internal (obuf);
>    return Qnil;
>  }

Does this fix the other problem which you saw with scrolling?  (I
doubt it does.)  If so, could you please tell me how to reproduce
that?

Thanks.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[martin rudalics]

 > Can you tell me how to reproduce this?  I don't see this in the recipe
 > you described in your bug report.

I can reproduce something similar here but it hardly makes sense to
share my recipe.  The bug manifests itself such that after an implicit
`forward-line' the cursor appears stuck in a partially visible line at
the bottom of a window.  That window has a sibling on the right.  On
Windows these siblings must be in the lower half of a split frame.  On
Gtk no vertical splitting is needed.  The cursor continues to move by
one line when keeping the down key (which implicitly runs `forward-line'
here) pressed for some three seconds and gets stuck again immediately.

Some additional particularities:

(1) The bug is not reproducible with Emacs 24-4, only with current
     trunk.

(2) My settings are needed and must be in .emacs.  Starting emacs -Q,
     putting my .emacs contents in *scratch* and doing an `eval-buffer'
     there does not reproduce it.

(3) My frame must be fully maximized.  Any other form of maximization
     doesn't trigger it.

(4) Your patch doesn't fix it.  A breakpoint set there is never hit.

The bug might be related to your changes from 2014-07-09.  For example
the following excerpt from a gdb session seems suspicious (the build is
with your patch but it doesn't matter):


(gdb) break xdisp.c:16261
Breakpoint 3 at 0x1050f51: file xdisp.c, line 16261.
[...]
(gdb) bt
#0  redisplay_window (window=..., just_this_one_p=false) at xdisp.c:16261
#1  0x0104a340 in redisplay_window_0 (window=...) at xdisp.c:14322
#2  0x01191406 in internal_condition_case_1 (bfun=0x104a30a <redisplay_window_0>, arg=..., handlers=..., hfun=0x104a2e6 <redisplay_window_error>) at eval.c:1368
#3  0x0104a2cb in redisplay_windows (window=...) at xdisp.c:14302
#4  0x0104a280 in redisplay_windows (window=...) at xdisp.c:14296
#5  0x0104a280 in redisplay_windows (window=...) at xdisp.c:14296
#6  0x01049291 in redisplay_internal () at xdisp.c:13901
#7  0x01047276 in redisplay () at xdisp.c:13181
#8  0x01104bad in read_char (commandflag=1, map=..., prev_event=..., used_mouse_menu=0x82f7ef, end_time=0x0) at keyboard.c:2594
#9  0x0111297b in read_key_sequence (keybuf=0x82f8e4, bufsize=30, prompt=..., dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=false) at keyboard.c:9178
#10 0x01102665 in command_loop_1 () at keyboard.c:1467
#11 0x011912f3 in internal_condition_case (bfun=0x11022e0 <command_loop_1>, handlers=..., hfun=0x1101b4b <cmd_error>) at eval.c:1344
#12 0x01101f96 in command_loop_2 (ignore=...) at keyboard.c:1198
#13 0x01190892 in internal_catch (tag=..., func=0x1101f72 <command_loop_2>, arg=...) at eval.c:1105
#14 0x01101f50 in command_loop () at keyboard.c:1177
#15 0x011016e7 in recursive_edit_1 () at keyboard.c:787
#16 0x011018a4 in Frecursive_edit () at keyboard.c:858
#17 0x010ff600 in main (argc=1, argv=0xa32808) at emacs.c:1643

Lisp Backtrace:
"redisplay_internal (C function)" (0x206c3b4)
(gdb) p new_vpos
$1 = 426
(gdb) p w->cursor.y
$2 = 432
(gdb)

In this particular case the display should be scrolled since otherwise
point ends up on the partially visible line.  But the test

	  if (new_vpos >= w->cursor.y)

fails to trigger that.

martin

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Sat, 27 Sep 2014 09:35:22 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: 18545@debbugs.gnu.org
> 
>  > Can you tell me how to reproduce this?  I don't see this in the recipe
>  > you described in your bug report.
> 
> I can reproduce something similar here but it hardly makes sense to
> share my recipe.  The bug manifests itself such that after an implicit
> `forward-line' the cursor appears stuck in a partially visible line at
> the bottom of a window.  That window has a sibling on the right.  On
> Windows these siblings must be in the lower half of a split frame.  On
> Gtk no vertical splitting is needed.  The cursor continues to move by
> one line when keeping the down key (which implicitly runs `forward-line'
> here) pressed for some three seconds and gets stuck again immediately.

Do you see the line number in the mode line of that window increasing,
after the cursor gets stuck, each time forward-line is run in that
window?

> Some additional particularities:
> 
> (1) The bug is not reproducible with Emacs 24-4, only with current
>      trunk.

That's strange, since the code to which you pointed is present in both
branches.

I'd like to hear from the OP (lompik@voila.fr) whether the bug
observed with Helm is reproducible in the latest pretest 24.3.93 or in
the emacs-24 branch.

> The bug might be related to your changes from 2014-07-09.

I guess you meant 2014-07-04?  There are no changes in all of xdisp.c
from 2014-07-09 or from a day before or after that (to account for
possible local time differences).  A bzr revno would be useful.

> (gdb) p new_vpos
> $1 = 426
> (gdb) p w->cursor.y
> $2 = 432
> (gdb)
> 
> In this particular case the display should be scrolled since otherwise
> point ends up on the partially visible line.  But the test
> 
> 	  if (new_vpos >= w->cursor.y)
> 
> fails to trigger that.

I cannot test a fix unless I have a way to reproduce the problem.
Since you can reproduce it, could you propose a solution?  One simple
solution would be to add this:

      if (!cursor_row_fully_visible_p (w, 0, 0))
        {
	   w->cursor.vpos = -1;
	   clear_glyph_matrix (w->desired_matrix);
	   goto try_to_scroll;
	}

after the call to set_cursor_from_row on line 16322.

If this doesn't work, please see why.

Thanks.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[martin rudalics]

 > Do you see the line number in the mode line of that window increasing,
 > after the cursor gets stuck, each time forward-line is run in that
 > window?

It gets updated normally until and including when the cursor is at the
partially visible line.  After that it gets updated with every
stuttering step, that is, when after three seconds the display actually
scrolls.  So FWIW the line number is congruent wrt to what I see on the
screen.

 >> (1) The bug is not reproducible with Emacs 24-4, only with current
 >>       trunk.
 >
 > That's strange, since the code to which you pointed is present in both
 > branches.

The remaining aspects like the need for a maximized frame and the fact
that the changes must be in my .emacs are much more strange.

 >> The bug might be related to your changes from 2014-07-09.
 >
 > I guess you meant 2014-07-04?  There are no changes in all of xdisp.c
 > from 2014-07-09 or from a day before or after that (to account for
 > possible local time differences).  A bzr revno would be useful.

I took the dates from trunk's ChangeLog.  On the release they appear as
of 2014-07-04.  More precisely I meant this change:

	* xdisp.c (redisplay_window): If redisplay of a window ends up
	with point in a partially visible line at end of the window, make
	sure the amended position of point actually has smaller Y
	coordinate; if not, give up and scroll the display.  (Bug#17905)

 >> (gdb) p new_vpos
 >> $1 = 426
 >> (gdb) p w->cursor.y
 >> $2 = 432
 >> (gdb)
 >>
 >> In this particular case the display should be scrolled since otherwise
 >> point ends up on the partially visible line.  But the test
 >>
 >> 	  if (new_vpos >= w->cursor.y)
 >>
 >> fails to trigger that.
 >
 > I cannot test a fix unless I have a way to reproduce the problem.

I can trigger the problem instantaneously here, so I can do anything you
propose.  OTOH sharing my recipe could be a very tedious endeavour.

 > Since you can reproduce it, could you propose a solution?  One simple
 > solution would be to add this:
 >
 >        if (!cursor_row_fully_visible_p (w, 0, 0))
 >          {
 > 	   w->cursor.vpos = -1;
 > 	   clear_glyph_matrix (w->desired_matrix);
 > 	   goto try_to_scroll;
 > 	}
 >
 > after the call to set_cursor_from_row on line 16322.
 >
 > If this doesn't work, please see why.

It fails to do anything because running cursor_row_fully_visible_p
returns at

   if (!MATRIX_ROW_PARTIALLY_VISIBLE_P (w, row))
     return 1;

and this one apparently fails to detect that the row is partially visible
because of

(gdb) p row->height
$5 = 16
(gdb) p row->visible_height
$6 = 16

Any ideas?  BTW is there a way to print the value returned by a macro in
gdb?

martin

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Sat, 27 Sep 2014 12:01:47 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: lompik@voila.fr, 18545@debbugs.gnu.org
> 
>  > Do you see the line number in the mode line of that window increasing,
>  > after the cursor gets stuck, each time forward-line is run in that
>  > window?
> 
> It gets updated normally until and including when the cursor is at the
> partially visible line.  After that it gets updated with every
> stuttering step, that is, when after three seconds the display actually
> scrolls.

AFAIU, this means the window is not being redrawn on each
forward-line; not even its mode line is updated.  You should be able
to confirm this if you turn on trace-redisplay (assuming you've built
with GLYPH_DEBUG, a.k.a. "--enable-checking=glyphs") -- you will not
see the window announced in the trace.  (Btw, turning off
blink-cursor-mode removes a lot of clutter from the redisplay trace,
so I suggest to do that in these experiments.)

If you add to the function that calls forward-line a message showing
point, do you see the value of point move on each forward-line even
though the window is not redrawn?

>  >> (1) The bug is not reproducible with Emacs 24-4, only with current
>  >>       trunk.
>  >
>  > That's strange, since the code to which you pointed is present in both
>  > branches.
> 
> The remaining aspects like the need for a maximized frame and the fact
> that the changes must be in my .emacs are much more strange.

Actually, I'm not so sure those are strange: rare situations with
pixel dimensions might be only exposed by such techniques.

>  >        if (!cursor_row_fully_visible_p (w, 0, 0))
>  >          {
>  > 	   w->cursor.vpos = -1;
>  > 	   clear_glyph_matrix (w->desired_matrix);
>  > 	   goto try_to_scroll;
>  > 	}
>  >
>  > after the call to set_cursor_from_row on line 16322.
>  >
>  > If this doesn't work, please see why.
> 
> It fails to do anything because running cursor_row_fully_visible_p
> returns at
> 
>    if (!MATRIX_ROW_PARTIALLY_VISIBLE_P (w, row))
>      return 1;
> 
> and this one apparently fails to detect that the row is partially visible
> because of
> 
> (gdb) p row->height
> $5 = 16
> (gdb) p row->visible_height
> $6 = 16
> 
> Any ideas?

What is last_visible_y in that window?  To see that, step into
try_window called on line 16235, wait until it calls start_display,
and look at it.last_visible_y.

> BTW is there a way to print the value returned by a macro in gdb?

Yes, just print it:

 (gdb) p MATRIX_ROW_PARTIALLY_VISIBLE_P (w, row)

If this doesn't work, perhaps you didn't build with -g3, or your
compiler is buggy.

Thanks.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[martin rudalics]

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

 > AFAIU, this means the window is not being redrawn on each
 > forward-line; not even its mode line is updated.  You should be able
 > to confirm this if you turn on trace-redisplay (assuming you've built
 > with GLYPH_DEBUG, a.k.a. "--enable-checking=glyphs") -- you will not
 > see the window announced in the trace.  (Btw, turning off
 > blink-cursor-mode removes a lot of clutter from the redisplay trace,
 > so I suggest to do that in these experiments.)

I attach an output, can't make much head or tail of it.  The *sidebar*
window is the one with the problem, the .emacs window the one on the
right of it.

 > If you add to the function that calls forward-line a message showing
 > point, do you see the value of point move on each forward-line even
 > though the window is not redrawn?

Simply copied from the *Messages* buffer the positions appear as:

17
22
27
32
37
42
47
52
57
62
67
72
77
82
87
92
97
102
107
112
117
122 [28 times]
127
132 [21 times]
137 [23 times]
142 [23 times]
147 [7 times]
Auto-saving...
147 [3 times]
152
157 [22 times]
162 [23 times]
167 [23 times]
172 [9 times]

Each line contains four characters and apparently stuttering begins
at position 122.

 >>   >        if (!cursor_row_fully_visible_p (w, 0, 0))
 >>   >          {
 >>   > 	   w->cursor.vpos = -1;
 >>   > 	   clear_glyph_matrix (w->desired_matrix);
 >>   > 	   goto try_to_scroll;
 >>   > 	}
 >>   >
 >>   > after the call to set_cursor_from_row on line 16322.
 >>   >
 >>   > If this doesn't work, please see why.
 >>
 >> It fails to do anything because running cursor_row_fully_visible_p
 >> returns at
 >>
 >>     if (!MATRIX_ROW_PARTIALLY_VISIBLE_P (w, row))
 >>       return 1;
 >>
 >> and this one apparently fails to detect that the row is partially visible
 >> because of
 >>
 >> (gdb) p row->height
 >> $5 = 16
 >> (gdb) p row->visible_height
 >> $6 = 16
 >>
 >> Any ideas?
 >
 > What is last_visible_y in that window?  To see that, step into
 > try_window called on line 16235, wait until it calls start_display,
 > and look at it.last_visible_y.

At the xdisp.c line reading:

   while (it.current_y < it.last_visible_y)

I have

(gdb) p it.last_visible_y
$7 = 442

and (just to confirm the earlier posted) at the xdisp.c line reading

	  if (new_vpos >= w->cursor.y)

I have:

(gdb) p new_vpos
$10 = 426
(gdb) p w->cursor.y
$11 = 432

 >> BTW is there a way to print the value returned by a macro in gdb?
 >
 > Yes, just print it:
 >
 >   (gdb) p MATRIX_ROW_PARTIALLY_VISIBLE_P (w, row)

Here I get

(gdb) p MATRIX_ROW_PARTIALLY_VISIBLE_P (w, row)
No symbol "__FILE__" in current context.

 > If this doesn't work, perhaps you didn't build with -g3,

I used CPPFLAGS='-DGLYPH_DEBUG=1' CFLAGS='-O0 -g3'

 > or your
 > compiler is buggy.

Hmm...

martin


As an aside, in my function calling `forward-line' I found this

;;   (unless (pos-visible-in-window-p (point) (selected-window))
;;     (recenter (- (window-height) 2)))) ; what for did I use that ?

commented out at least since 2008-06-24.  Apparently something here did
behave differently many years ago.  BTW, the bug does not disappear when
I comment in those lines.

[-- Attachment #2: trace-redisplay.txt --]
[-- Type: text/plain, Size: 9460 bytes --]

redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_preserve_echo_area (8)
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_preserve_echo_area (8)
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_preserve_echo_area (8)
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_preserve_echo_area (8)
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_preserve_echo_area (8)
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_preserve_echo_area (8)
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_preserve_echo_area (8)
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_preserve_echo_area (8)
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_preserve_echo_area (8)
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_preserve_echo_area (8)
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_preserve_echo_area (8)
redisplay_internal 0
trying display optimization 1
017fce48 ( *Minibuf-1*): optimization 1
redisplay_internal 0
01963f50 (*scratch*): same window start
01963f50 (*scratch*): 1
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (50, 500, 16, 426)
expose_window (50, 500, 16, 426)
expose_frame (0, 926, 50, 16)
expose_window (0, 926, 50, 16)
expose_frame (0, 926, 1664, 16)
expose_window (0, 926, 72, 16)
expose_window (72, 926, 1592, 16)
expose_frame (72, 926, 1592, 16)
expose_window (72, 926, 0, 16)
expose_window (72, 926, 1592, 16)
redisplay_preserve_echo_area (8)
redisplay_internal 0
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
redisplay_preserve_echo_area (9)
redisplay_internal 0
04fa38f0 ( *sidebar*): cursor movement
redisplay_preserve_echo_area (8)
redisplay_internal 0
redisplay_preserve_echo_area (8)
redisplay_internal 0
04fa38f0 ( *sidebar*): same window start
04fa38f0 ( *sidebar*): 1
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (0, 564, 50, 16)
expose_window (0, 564, 50, 16)
expose_frame (72, 868, 1592, 48)
expose_window (72, 868, 0, 48)
expose_window (72, 868, 1592, 48)
redisplay_preserve_echo_area (8)
redisplay_internal 0
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
redisplay_preserve_echo_area (9)
redisplay_internal 0
04fa38f0 ( *sidebar*): cursor movement
redisplay_preserve_echo_area (8)
redisplay_internal 0
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 836, 1592, 80)
expose_window (72, 836, 0, 80)
expose_window (72, 836, 1592, 80)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 836, 1592, 80)
expose_window (72, 836, 0, 80)
expose_window (72, 836, 1592, 80)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 644, 1592, 272)
expose_window (72, 644, 0, 272)
expose_window (72, 644, 1592, 272)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 564, 1592, 352)
expose_window (72, 564, 0, 352)
expose_window (72, 564, 1592, 352)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 580, 1592, 336)
expose_window (72, 580, 0, 336)
expose_window (72, 580, 1592, 336)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 580, 1592, 320)
expose_window (72, 580, 0, 320)
expose_window (72, 580, 1592, 320)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 612, 1592, 160)
expose_window (72, 612, 0, 160)
expose_window (72, 612, 1592, 160)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame  garbaged
redisplay_preserve_echo_area (11)
redisplay_internal 0
01963f50 (*scratch*): same window start
01963f50 (*scratch*): 1
04fa38f0 ( *sidebar*): same window start
04fa38f0 ( *sidebar*): 1
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 500, 1592, 416)
expose_window (72, 500, 0, 416)
expose_window (72, 500, 1592, 416)
expose_frame (72, 804, 1592, 16)
expose_window (72, 804, 0, 16)
expose_window (72, 804, 1592, 16)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 740, 1592, 176)
expose_window (72, 740, 0, 176)
expose_window (72, 740, 1592, 176)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 756, 1592, 48)
expose_window (72, 756, 0, 48)
expose_window (72, 756, 1592, 48)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 772, 1592, 48)
expose_window (72, 772, 0, 48)
expose_window (72, 772, 1592, 48)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 788, 1592, 112)
expose_window (72, 788, 0, 112)
expose_window (72, 788, 1592, 112)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 804, 1592, 48)
expose_window (72, 804, 0, 48)
expose_window (72, 804, 1592, 48)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 820, 1592, 48)
expose_window (72, 820, 0, 48)
expose_window (72, 820, 1592, 48)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): forced window start
expose_frame (72, 804, 1592, 48)
expose_window (72, 804, 0, 48)
expose_window (72, 804, 1592, 48)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
04f98ad0 (.emacs): try_scrolling
expose_frame (72, 836, 1592, 64)
expose_window (72, 836, 0, 64)
expose_window (72, 836, 1592, 64)
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
04f98ad0 (.emacs): try_scrolling
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
04f98ad0 (.emacs): try_scrolling
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
04f98ad0 (.emacs): try_scrolling
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
04f98ad0 (.emacs): try_scrolling
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
04f98ad0 (.emacs): try_scrolling
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
04f98ad0 (.emacs): try_scrolling
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
04f98ad0 (.emacs): try_scrolling
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
04f98ad0 (.emacs): try_scrolling
redisplay_internal 0
04fa38f0 ( *sidebar*): forced window start
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
04f98ad0 (.emacs): try_scrolling
redisplay_preserve_echo_area (8)
redisplay_internal 0
04f98ad0 (.emacs): same window start
04f98ad0 (.emacs): 1
redisplay_preserve_echo_area (9)
redisplay_internal 0
04fa38f0 ( *sidebar*): cursor movement
redisplay_preserve_echo_area (8)
redisplay_internal 0
redisplay_preserve_echo_area (8)
redisplay_internal 0
04fa38f0 ( *sidebar*): same window start
04fa38f0 ( *sidebar*): 1
redisplay_preserve_echo_area (8)
redisplay_internal 0
redisplay_internal 0
redisplay_preserve_echo_area (8)
redisplay_internal 0
redisplay_preserve_echo_area (8)
redisplay_internal 0
redisplay_preserve_echo_area (8)
redisplay_internal 0
redisplay_preserve_echo_area (8)
redisplay_internal 0
redisplay_preserve_echo_area (8)
redisplay_internal 0
redisplay_preserve_echo_area (8)
redisplay_internal 0

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[lompik]

I can only do some tests from Sunday.


Thanks


> 
> Does this fix the other problem which you saw with scrolling? (I
> doubt it does.) If so, could you please tell me how to reproduce
> that?
> 
> Thanks.
> 

___________________________________________________________
Mode, hifi, maison,… J'achète malin. Je compare les prix avec Voila.fr http://shopping.voila.fr/

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Sat, 27 Sep 2014 16:45:16 +0200 (CEST)
> From: lompik@voila.fr
> Cc: 18545@debbugs.gnu.org
> 
> 
> I can only do some tests from Sunday.

OK, thanks.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Sat, 27 Sep 2014 15:36:49 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: lompik@voila.fr, 18545@debbugs.gnu.org
> 
>  > AFAIU, this means the window is not being redrawn on each
>  > forward-line; not even its mode line is updated.  You should be able
>  > to confirm this if you turn on trace-redisplay (assuming you've built
>  > with GLYPH_DEBUG, a.k.a. "--enable-checking=glyphs") -- you will not
>  > see the window announced in the trace.  (Btw, turning off
>  > blink-cursor-mode removes a lot of clutter from the redisplay trace,
>  > so I suggest to do that in these experiments.)
> 
> I attach an output, can't make much head or tail of it.  The *sidebar*
> window is the one with the problem, the .emacs window the one on the
> right of it.

AFAIU, it's indeed similar to the problem I solved with my previous
patch (the one you have installed).

> 117
> 122 [28 times]

This says point doesn't move, which I don't understand how can
happen.  forward-line doesn't care about anything except moving point
to the next line.

>  >> (gdb) p row->height
>  >> $5 = 16
>  >> (gdb) p row->visible_height
>  >> $6 = 16
>  >>
>  >> Any ideas?
>  >
>  > What is last_visible_y in that window?  To see that, step into
>  > try_window called on line 16235, wait until it calls start_display,
>  > and look at it.last_visible_y.
> 
> At the xdisp.c line reading:
> 
>    while (it.current_y < it.last_visible_y)
> 
> I have
> 
> (gdb) p it.last_visible_y
> $7 = 442
> 
> and (just to confirm the earlier posted) at the xdisp.c line reading
> 
> 	  if (new_vpos >= w->cursor.y)
> 
> I have:
> 
> (gdb) p new_vpos
> $10 = 426
> (gdb) p w->cursor.y
> $11 = 432

But if new_vpos is 426 and row->height is 16, then the last row, which
starts at y = 426, will end at y = 442, i.e. it's fully visible.  This
contradicts what you said earlier, that the last line is only
partially visible.

>  >> BTW is there a way to print the value returned by a macro in gdb?
>  >
>  > Yes, just print it:
>  >
>  >   (gdb) p MATRIX_ROW_PARTIALLY_VISIBLE_P (w, row)
> 
> Here I get
> 
> (gdb) p MATRIX_ROW_PARTIALLY_VISIBLE_P (w, row)
> No symbol "__FILE__" in current context.

I get this:

  (gdb) p MATRIX_ROW_PARTIALLY_VISIBLE_P(w,row)
  $1 = 0

>  > If this doesn't work, perhaps you didn't build with -g3,
> 
> I used CPPFLAGS='-DGLYPH_DEBUG=1' CFLAGS='-O0 -g3'
> 
>  > or your
>  > compiler is buggy.
> 
> Hmm...

Which GCC version is that?  I have 4.8.1 here.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Stefan Monnier]

> This says point doesn't move, which I don't understand how can
> happen.  forward-line doesn't care about anything except moving point
> to the next line.

That's odd, indeed.  It can be explained if the redisplay ends up moving
point to keep it in the window (instead of scrolling the window to
follow point).


        Stefan

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: martin rudalics <rudalics@gmx.at>,  lompik@voila.fr,  18545@debbugs.gnu.org
> Date: Sat, 27 Sep 2014 13:25:04 -0400
> 
> > This says point doesn't move, which I don't understand how can
> > happen.  forward-line doesn't care about anything except moving point
> > to the next line.
> 
> That's odd, indeed.  It can be explained if the redisplay ends up moving
> point to keep it in the window (instead of scrolling the window to
> follow point).

I don't think this can explain it: forward-line works on the buffer,
and the location of point is (or should be, see below) printed
_before_ Emacs enters redisplay, as part of running the command that
called forward-line.

This, of course, assumes that what Martin did was equivalent to the
below:

  (defun my-fwd-line ()
    (interactive)
    (forward-line 1)
    (message "%s" (point)))

  (define-key global-map [down] 'my-fwd-line)

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Stefan Monnier]

>> That's odd, indeed.  It can be explained if the redisplay ends up moving
>> point to keep it in the window (instead of scrolling the window to
>> follow point).

> I don't think this can explain it: forward-line works on the buffer,
> and the location of point is (or should be, see below) printed
> _before_ Emacs enters redisplay, as part of running the command that
> called forward-line.

You assume that the value of point displayed is consistent with the one
you see on the screen.  But maybe the value of point printed is the one
of the line *after* the last line.  So after printing that value,
redisplay re-sets point and at the end of the next command, the
displayed point will be the same as the last displayed one.


        Stefan

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[martin rudalics]

 >> 117
 >> 122 [28 times]
 >
 > This says point doesn't move, which I don't understand how can
 > happen.  forward-line doesn't care about anything except moving point
 > to the next line.

But after 28 times point moves which is even less understandable.

 > But if new_vpos is 426 and row->height is 16, then the last row, which
 > starts at y = 426, will end at y = 442, i.e. it's fully visible.  This
 > contradicts what you said earlier, that the last line is only
 > partially visible.

Then why does cursor_row_fully_visible_p return 0 here?

#0  cursor_row_fully_visible_p (w=0x5a25d78, force_p=0, current_matrix_p=0) at xdisp.c:15039
#1  0x01050f3f in redisplay_window (window=..., just_this_one_p=false) at xdisp.c:16250
#2  0x0104a340 in redisplay_window_0 (window=...) at xdisp.c:14322
#3  0x011913e2 in internal_condition_case_1 (bfun=0x104a30a <redisplay_window_0>, arg=..., handlers=..., hfun=0x104a2e6 <redisplay_window_error>) at eval.c:1368
#4  0x0104a2cb in redisplay_windows (window=...) at xdisp.c:14302
#5  0x0104a280 in redisplay_windows (window=...) at xdisp.c:14296
#6  0x0104a280 in redisplay_windows (window=...) at xdisp.c:14296
#7  0x01049291 in redisplay_internal () at xdisp.c:13901
#8  0x01047276 in redisplay () at xdisp.c:13181
#9  0x01104b89 in read_char (commandflag=1, map=..., prev_event=..., used_mouse_menu=0x82f7ef, end_time=0x0) at keyboard.c:2594
#10 0x01112957 in read_key_sequence (keybuf=0x82f8e4, bufsize=30, prompt=..., dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=false) at keyboard.c:9178
#11 0x01102641 in command_loop_1 () at keyboard.c:1467
#12 0x011912cf in internal_condition_case (bfun=0x11022bc <command_loop_1>, handlers=..., hfun=0x1101b27 <cmd_error>) at eval.c:1344
#13 0x01101f72 in command_loop_2 (ignore=...) at keyboard.c:1198
#14 0x0119086e in internal_catch (tag=..., func=0x1101f4e <command_loop_2>, arg=...) at eval.c:1105
#15 0x01101f2c in command_loop () at keyboard.c:1177
#16 0x011016c3 in recursive_edit_1 () at keyboard.c:787
#17 0x01101880 in Frecursive_edit () at keyboard.c:858
#18 0x010ff5dc in main (argc=1, argv=0xa327e8) at emacs.c:1643

Lisp Backtrace:
"redisplay_internal (C function)" (0x206c3b4)

You probably mean I should answer that myself but honestly I don't
understand cursor_row_fully_visible_p much.  In any case, at least one
pixel is missing visually.

 > Which GCC version is that?  I have 4.8.1 here.

4.6.2.  I'll try with Debian the next time.

martin

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[martin rudalics]

 > I don't think this can explain it: forward-line works on the buffer,
 > and the location of point is (or should be, see below) printed
 > _before_ Emacs enters redisplay, as part of running the command that
 > called forward-line.

This is a red herring.  The values I print can't be attributed to a
specific "failing" instance of `forward-line'.  What you see is the
readjusted value after scrolling was wrongly dismissed in the last
round.  With `before' and `after' around the `forward-line' call I get:

before 97
after 102
before 102
after 107
before 107
after 112
before 112
after 117
before 117
after 122
before 117
after 122
before 117
after 122
before 117
after 122
before 117
after 122
before 117
after 122
before 117
after 122
before 117
after 122
before 117
after 122
before 117
after 122
before 117
after 122
before 117
after 122

So `forward-line' always skips five characters but point gets reset
later on.

 > This, of course, assumes that what Martin did was equivalent to the
 > below:
 >
 >    (defun my-fwd-line ()
 >      (interactive)
 >      (forward-line 1)
 >      (message "%s" (point)))

That's precisely what I did.

martin

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[martin rudalics]

 > You assume that the value of point displayed is consistent with the one
 > you see on the screen.  But maybe the value of point printed is the one
 > of the line *after* the last line.  So after printing that value,
 > redisplay re-sets point and at the end of the next command, the
 > displayed point will be the same as the last displayed one.

Indeed.  Redisplay moves point back exactly to where it was before.

martin

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Sat, 27 Sep 2014 21:01:51 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: lompik@voila.fr, 18545@debbugs.gnu.org
> 
>  >> 117
>  >> 122 [28 times]
>  >
>  > This says point doesn't move, which I don't understand how can
>  > happen.  forward-line doesn't care about anything except moving point
>  > to the next line.
> 
> But after 28 times point moves which is even less understandable.

I don't think this is relevant.  It could be related to
blink-cursor-mode or some other feature that triggers a more thorough
redisplay.  (Try typing "M-x", for example.)  The code part that seems
to be involved in this is an optimization, so when it is not taken,
you don't see the problem.

>  > But if new_vpos is 426 and row->height is 16, then the last row, which
>  > starts at y = 426, will end at y = 442, i.e. it's fully visible.  This
>  > contradicts what you said earlier, that the last line is only
>  > partially visible.
> 
> Then why does cursor_row_fully_visible_p return 0 here?

Because w->cursor.y is 432, only 10 pixels from the window bottom,
which is less than 16 pixels we need for the row.

Hmm, I think I see the problem in the logic behind this part:

      if (!cursor_row_fully_visible_p (w, 0, 0))
	{
	  /* Point does appear, but on a line partly visible at end of window.
	     Move it back to a fully-visible line.  */
	  new_vpos = window_box_height (w);
	  /* But if window_box_height suggests a Y coordinate that is
	     not less than we already have, that line will clearly not
	     be fully visible, so give up and scroll the display.
	     This can happen when the default face uses a font whose
	     dimensions are different from the frame's default
	     font.  */
	  if (new_vpos >= w->cursor.y)
	    {
	      w->cursor.vpos = -1;
	      clear_glyph_matrix (w->desired_matrix);
	      goto try_to_scroll;
	    }

What I think happens is that window_box_height tells us to put the
cursor at y = 426, but the first row that fits that is too far down,
due to the window size being not an integral multiple of the font
height.  So the condition to goto try_to_scroll needs to become
smarter.

Can you show all the values of MATRIX_ROW_BOTTOM_Y that are examined
in this loop, after we determine that new_vpos should be 426:

	  row = MATRIX_FIRST_TEXT_ROW (w->desired_matrix);
	  while (MATRIX_ROW_BOTTOM_Y (row) < new_vpos)
	    ++row;

> You probably mean I should answer that myself but honestly I don't
> understand cursor_row_fully_visible_p much.

Which part of it do you not understand?

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Sat, 27 Sep 2014 21:01:55 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: lompik@voila.fr, 18545@debbugs.gnu.org
> 
>  > I don't think this can explain it: forward-line works on the buffer,
>  > and the location of point is (or should be, see below) printed
>  > _before_ Emacs enters redisplay, as part of running the command that
>  > called forward-line.
> 
> This is a red herring.  The values I print can't be attributed to a
> specific "failing" instance of `forward-line'.  What you see is the
> readjusted value after scrolling was wrongly dismissed in the last
> round.  With `before' and `after' around the `forward-line' call I get:
> [...]
> before 117
> after 122
> before 117
> after 122
> [...]
> So `forward-line' always skips five characters but point gets reset
> later on.

But that means the code is doing exactly what it was intended to do:
move point back so that it is in view.  IOW, "notabug".  Right?

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Sat, 27 Sep 2014 21:03:30 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: lompik@voila.fr, 18545@debbugs.gnu.org
> 
>  > You assume that the value of point displayed is consistent with the one
>  > you see on the screen.  But maybe the value of point printed is the one
>  > of the line *after* the last line.  So after printing that value,
>  > redisplay re-sets point and at the end of the next command, the
>  > displayed point will be the same as the last displayed one.
> 
> Indeed.  Redisplay moves point back exactly to where it was before.

Which is exactly what that code was written to do.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Sat, 27 Sep 2014 22:38:08 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: lompik@voila.fr, 18545@debbugs.gnu.org
> 
> > Date: Sat, 27 Sep 2014 21:03:30 +0200
> > From: martin rudalics <rudalics@gmx.at>
> > CC: lompik@voila.fr, 18545@debbugs.gnu.org
> > 
> >  > You assume that the value of point displayed is consistent with the one
> >  > you see on the screen.  But maybe the value of point printed is the one
> >  > of the line *after* the last line.  So after printing that value,
> >  > redisplay re-sets point and at the end of the next command, the
> >  > displayed point will be the same as the last displayed one.
> > 
> > Indeed.  Redisplay moves point back exactly to where it was before.
> 
> Which is exactly what that code was written to do.

Hmm... I wonder why did we enter this area of the code, i.e. why did
this condition fire:

  /* Handle case where place to start displaying has been specified,
     unless the specified location is outside the accessible range.  */
  if (w->force_start || window_frozen_p (w))

Was w->force_start non-zero, or did window_frozen_p return non-zero?
If the former, can you see where was the force_start flag set?  (I'd
be surprised if it's the latter, since windows are only frozen when we
grow the minibuffer, which shouldn't be happening here, I think.)

In general, if the force_start flag of a window is set, that means we
shouldn't scroll, so bringing point back into view makes sense.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[martin rudalics]

 >> But after 28 times point moves which is even less understandable.
 >
 > I don't think this is relevant.  It could be related to
 > blink-cursor-mode

... always turned off here ...

 > or some other feature that triggers a more thorough
 > redisplay.  (Try typing "M-x", for example.)  The code part that seems
 > to be involved in this is an optimization, so when it is not taken,
 > you don't see the problem.

OK.  But the only thing I do here is leaning on the <down> button.

 > Can you show all the values of MATRIX_ROW_BOTTOM_Y that are examined
 > in this loop, after we determine that new_vpos should be 426:
 >
 > 	  row = MATRIX_FIRST_TEXT_ROW (w->desired_matrix);
 > 	  while (MATRIX_ROW_BOTTOM_Y (row) < new_vpos)
 > 	    ++row;

Using

	  while (MATRIX_ROW_BOTTOM_Y (row) < new_vpos)
	    {
	      ++row;
	      Vdebug_on_message = Fcons (Fcons (make_number (row->y), make_number (new_vpos)),
					 Vdebug_on_message);
	    }

and nreverse on the value of Vdebug_on_message gets me a repeated
pattern of

(32 . 426) (48 . 426) (64 . 426) (80 . 426) (96 . 426) (112 . 426) (128
. 426) (144 . 426) (160 . 426) (176 . 426) (192 . 426) (208 . 426) (224
. 426) (240 . 426) (256 . 426) (272 . 426) (288 . 426) (304 . 426) (320
. 426) (336 . 426) (352 . 426) (368 . 426) (384 . 426) (400 . 426) (416
. 426)

 >> I don't
 >> understand cursor_row_fully_visible_p much.
 >
 > Which part of it do you not understand?

Why it should return 1 in these two cases

   if (!MATRIX_ROW_PARTIALLY_VISIBLE_P (w, row))
     return 1;

and

   if (row->height >= window_height)
     {
       if (!force_p || MINI_WINDOW_P (w)
	  || w->vscroll || w->cursor.vpos == 0)
	return 1;
     }

martin

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[martin rudalics]

 > Hmm... I wonder why did we enter this area of the code, i.e. why did
 > this condition fire:
 >
 >    /* Handle case where place to start displaying has been specified,
 >       unless the specified location is outside the accessible range.  */
 >    if (w->force_start || window_frozen_p (w))
 >
 > Was w->force_start non-zero, or did window_frozen_p return non-zero?
 > If the former, can you see where was the force_start flag set?  (I'd
 > be surprised if it's the latter, since windows are only frozen when we
 > grow the minibuffer, which shouldn't be happening here, I think.)
 >
 > In general, if the force_start flag of a window is set, that means we
 > shouldn't scroll, so bringing point back into view makes sense.

That's likely me.  Under certain conditions I do `recenter' with a
negative argument in `post-command-hook' which basically looks like

              (recenter (max 0 (- (window-height) 7)))

which triggers w->force_start being set here in xdisp.c:

       if (IT_CHARPOS (it) == PT)
	w->force_start = 1;

So my case _is_ very likely special.  Still "bringing point back into
view" shouldn't make point appear on a partially visible line.  Or am I
missing something?

martin

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Sun, 28 Sep 2014 11:34:44 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: lompik@voila.fr, 18545@debbugs.gnu.org
> 
>  > Hmm... I wonder why did we enter this area of the code, i.e. why did
>  > this condition fire:
>  >
>  >    /* Handle case where place to start displaying has been specified,
>  >       unless the specified location is outside the accessible range.  */
>  >    if (w->force_start || window_frozen_p (w))
>  >
>  > Was w->force_start non-zero, or did window_frozen_p return non-zero?
>  > If the former, can you see where was the force_start flag set?  (I'd
>  > be surprised if it's the latter, since windows are only frozen when we
>  > grow the minibuffer, which shouldn't be happening here, I think.)
>  >
>  > In general, if the force_start flag of a window is set, that means we
>  > shouldn't scroll, so bringing point back into view makes sense.
> 
> That's likely me.  Under certain conditions I do `recenter' with a
> negative argument in `post-command-hook' which basically looks like
> 
>               (recenter (max 0 (- (window-height) 7)))
> 
> which triggers w->force_start being set here in xdisp.c:
> 
>        if (IT_CHARPOS (it) == PT)
> 	w->force_start = 1;

That's what I thought.

> So my case _is_ very likely special.  Still "bringing point back into
> view" shouldn't make point appear on a partially visible line.

No, that's likely the bug here.  I will look into that.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Sun, 28 Sep 2014 11:33:36 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: lompik@voila.fr, 18545@debbugs.gnu.org
> 
>  >> I don't
>  >> understand cursor_row_fully_visible_p much.
>  >
>  > Which part of it do you not understand?
> 
> Why it should return 1 in these two cases
> 
>    if (!MATRIX_ROW_PARTIALLY_VISIBLE_P (w, row))
>      return 1;

If a row is _not_ partially visible, it is _fully_ visible, right?

> and
> 
>    if (row->height >= window_height)
>      {
>        if (!force_p || MINI_WINDOW_P (w)
> 	  || w->vscroll || w->cursor.vpos == 0)
> 	return 1;
>      }

There's a comment that that's supposed to explain this.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Sun, 28 Sep 2014 19:29:47 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: lompik@voila.fr, 18545@debbugs.gnu.org
> 
> > Date: Sun, 28 Sep 2014 11:34:44 +0200
> > From: martin rudalics <rudalics@gmx.at>
> > CC: lompik@voila.fr, 18545@debbugs.gnu.org
> > 
> >  > Hmm... I wonder why did we enter this area of the code, i.e. why did
> >  > this condition fire:
> >  >
> >  >    /* Handle case where place to start displaying has been specified,
> >  >       unless the specified location is outside the accessible range.  */
> >  >    if (w->force_start || window_frozen_p (w))
> >  >
> >  > Was w->force_start non-zero, or did window_frozen_p return non-zero?
> >  > If the former, can you see where was the force_start flag set?  (I'd
> >  > be surprised if it's the latter, since windows are only frozen when we
> >  > grow the minibuffer, which shouldn't be happening here, I think.)
> >  >
> >  > In general, if the force_start flag of a window is set, that means we
> >  > shouldn't scroll, so bringing point back into view makes sense.
> > 
> > That's likely me.  Under certain conditions I do `recenter' with a
> > negative argument in `post-command-hook' which basically looks like
> > 
> >               (recenter (max 0 (- (window-height) 7)))
> > 
> > which triggers w->force_start being set here in xdisp.c:
> > 
> >        if (IT_CHARPOS (it) == PT)
> > 	w->force_start = 1;
> 
> That's what I thought.
> 
> > So my case _is_ very likely special.  Still "bringing point back into
> > view" shouldn't make point appear on a partially visible line.
> 
> No, that's likely the bug here.  I will look into that.

Does the patch below help?  If not, can you tell where I goofed?

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2014-09-25 07:01:35 +0000
+++ src/xdisp.c	2014-09-28 17:45:22 +0000
@@ -16179,15 +16179,21 @@ redisplay_window (Lisp_Object window, bo
       && CHARPOS (startp) >= BEGV
       && CHARPOS (startp) <= ZV)
     {
+      ptrdiff_t it_charpos;
+
       w->optional_new_start = 0;
       start_display (&it, w, startp);
       move_it_to (&it, PT, 0, it.last_visible_y, -1,
 		  MOVE_TO_POS | MOVE_TO_X | MOVE_TO_Y);
-      if (IT_CHARPOS (it) == PT)
-	w->force_start = 1;
-      /* IT may overshoot PT if text at PT is invisible.  */
-      else if (IT_CHARPOS (it) > PT && CHARPOS (startp) <= PT)
-	w->force_start = 1;
+      it_charpos = IT_CHARPOS (it);
+      if (line_bottom_y (&it) < it.last_visible_y)
+	{
+	  if (it_charpos == PT)
+	    w->force_start = 1;
+	  /* IT may overshoot PT if text at PT is invisible.  */
+	  else if (it_charpos > PT && CHARPOS (startp) <= PT)
+	    w->force_start = 1;
+	}
     }
 
  force_start:

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[martin rudalics]

 > Does the patch below help?

Works like a charm (I was already about to rewrite my 10 years old code
and get rid of that recentering rigmarole).

Let's see whether it's of any help for lompik.

Many thanks, martin

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[martin rudalics]

 >>   >> I don't
 >>   >> understand cursor_row_fully_visible_p much.
 >>   >
 >>   > Which part of it do you not understand?
 >>
 >> Why it should return 1 in these two cases
 >>
 >>     if (!MATRIX_ROW_PARTIALLY_VISIBLE_P (w, row))
 >>       return 1;
 >
 > If a row is _not_ partially visible, it is _fully_ visible, right?

Hmmm ... yes.  Unless it's completely invisible.  I was probably fooled
by the inverted uses of fully and partially visible.

 >> and
 >>
 >>     if (row->height >= window_height)
 >>       {
 >>         if (!force_p || MINI_WINDOW_P (w)
 >> 	  || w->vscroll || w->cursor.vpos == 0)
 >> 	return 1;
 >>       }
 >
 > There's a comment that that's supposed to explain this.

I don't understand the

	  || w->vscroll || w->cursor.vpos == 0

disjuncts in the second conditional.  But I presume that this case is
too obscure anyway.

martin

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Sun, 28 Sep 2014 21:04:00 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: lompik@voila.fr, 18545@debbugs.gnu.org
> 
> I don't understand the
> 
> 	  || w->vscroll || w->cursor.vpos == 0
> 
> disjuncts in the second conditional.

I think w->vscroll means the window is vscrolled, and so the fact that
a row is not fully visible doesn't count (since that's what vscroll
does), and w->cursor.vpos == 0 means you have a very tall row that is
taller than the window, in which case it again gains us nothing to
consider it not fully visible.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Sun, 28 Sep 2014 21:03:33 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: lompik@voila.fr, 18545@debbugs.gnu.org
> 
>  > Does the patch below help?
> 
> Works like a charm

Thanks.

And you say the problem it fixes is not visible on the emacs-24
branch?  Or did I misunderstand?  (The offending code is identical on
the branch.)

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[martin rudalics]

 > And you say the problem it fixes is not visible on the emacs-24
 > branch?  Or did I misunderstand?  (The offending code is identical on
 > the branch.)

No.  My problem is not visible on the emacs-24 branch.

And now that you asked: When I turn off horizontal scroll bars, the
problem is not visible on an unpatched trunk either.  So the problem was
introduced by the presence of horizontal scroll bars.

martin

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[lompik]

It still wrong scroll with all the 3 patches.I guess it is configuration specific + a call to "C-h k a".Fortunately, I had some some time to spend on it and here's a 
minimal example:

1. emacs -Q
2. define following settings:

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; scale minibuffer font up on entering
(defun my-minibuffer-setup ()
(set (make-local-variable 'face-remapping-alist)
'((default :height 1.8))))

(add-hook 'minibuffer-setup-hook 'my-minibuffer-setup)


;; make *Completions* buffer editable and add header line + insert "hello world" as overlay
(global-set-key (kbd "C-!") '(lambda ()
(interactive)
(with-selected-window (get-buffer-window "*Completions*")
(blink-cursor-mode -1)
(read-only-mode 0)
(goto-char 0)
(let ((start (point)))
(setq header-line-format
(propertize "Header line appears here" 'face 'header-line))
(insert "test:")
(overlay-put (make-overlay (point-at-bol) (point-at-eol))
'display "Hello World")
(insert "\n")
(put-text-property start
(point)
'face '(:family "Sans Serif" :foreground "black" :height 1.3 :weight bold) )))))

; some key-bindings to navigate *Completion* buffer from minibuffer 
(global-set-key (kbd "C-`") '(lambda ()
(interactive)
(with-selected-window (get-buffer-window "*Completions*")
(forward-line 1))))
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

3. hit the following key sequence:

C-x C-f -> in a folder with lots of files
[tab]x2 -> open completion buffer
C-! -> modify completion buffer
C-h k a -> Show help for letter "a" (Important ! skipping this results in no bug ) 
C-` until bottom of *Completions* buffer -> no scrolling,


I have applied the 3 patches that you supplied on emacs bzr revno 117517. I have tested this on 24.3.93 as well.

Regards,
Lompik.


> Message du 28/09/14 à 19h51
> De : "Eli Zaretskii" 
> A : rudalics@gmx.at
> Copie à : lompik@voila.fr, 18545@debbugs.gnu.org
> Objet : Re: bug#18545: 24.4.50: Bug - forward-line inside with-selected-window
> 
> > Date: Sun, 28 Sep 2014 19:29:47 +0300
> > From: Eli Zaretskii 
> > Cc: lompik@voila.fr, 18545@debbugs.gnu.org
> > 
> > > Date: Sun, 28 Sep 2014 11:34:44 +0200
> > > From: martin rudalics 
> > > CC: lompik@voila.fr, 18545@debbugs.gnu.org
> > > 
> > > > Hmm... I wonder why did we enter this area of the code, i.e. why did
> > > > this condition fire:
> > > >
> > > > /* Handle case where place to start displaying has been specified,
> > > > unless the specified location is outside the accessible range. */
> > > > if (w->force_start || window_frozen_p (w))
> > > >
> > > > Was w->force_start non-zero, or did window_frozen_p return non-zero?
> > > > If the former, can you see where was the force_start flag set? (I'd
> > > > be surprised if it's the latter, since windows are only frozen when we
> > > > grow the minibuffer, which shouldn't be happening here, I think.)
> > > >
> > > > In general, if the force_start flag of a window is set, that means we
> > > > shouldn't scroll, so bringing point back into view makes sense.
> > > 
> > > That's likely me. Under certain conditions I do `recenter' with a
> > > negative argument in `post-command-hook' which basically looks like
> > > 
> > > (recenter (max 0 (- (window-height) 7)))
> > > 
> > > which triggers w->force_start being set here in xdisp.c:
> > > 
> > > if (IT_CHARPOS (it) == PT)
> > > w->force_start = 1;
> > 
> > That's what I thought.
> > 
> > > So my case _is_ very likely special. Still "bringing point back into
> > > view" shouldn't make point appear on a partially visible line.
> > 
> > No, that's likely the bug here. I will look into that.
> 
> Does the patch below help? If not, can you tell where I goofed?
> 
> === modified file 'src/xdisp.c'
> --- src/xdisp.c 2014-09-25 07:01:35 +0000
> +++ src/xdisp.c 2014-09-28 17:45:22 +0000
> @@ -16179,15 +16179,21 @@ redisplay_window (Lisp_Object window, bo
> && CHARPOS (startp) >= BEGV
> && CHARPOS (startp) <= ZV)
> {
> + ptrdiff_t it_charpos;
> +
> w->optional_new_start = 0;
> start_display (&it, w, startp);
> move_it_to (&it, PT, 0, it.last_visible_y, -1,
> MOVE_TO_POS | MOVE_TO_X | MOVE_TO_Y);
> - if (IT_CHARPOS (it) == PT)
> - w->force_start = 1;
> - /* IT may overshoot PT if text at PT is invisible. */
> - else if (IT_CHARPOS (it) > PT && CHARPOS (startp) <= PT)
> - w->force_start = 1;
> + it_charpos = IT_CHARPOS (it);
> + if (line_bottom_y (&it) < it.last_visible_y)
> + {
> + if (it_charpos == PT)
> + w->force_start = 1;
> + /* IT may overshoot PT if text at PT is invisible. */
> + else if (it_charpos > PT && CHARPOS (startp) <= PT)
> + w->force_start = 1;
> + }
> }
> 
> force_start:
> 
> 

___________________________________________________________
Mode, hifi, maison,… J'achète malin. Je compare les prix avec Voila.fr http://shopping.voila.fr/

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Mon, 29 Sep 2014 02:31:04 +0200 (CEST)
> From: lompik@voila.fr
> Cc: 18545@debbugs.gnu.org
> 
> C-x C-f -> in a folder with lots of files
> [tab]x2 -> open completion buffer
> C-! -> modify completion buffer
> C-h k a -> Show help for letter "a" (Important ! skipping this results in no bug ) 
> C-` until bottom of *Completions* buffer -> no scrolling,
> 
> 
> I have applied the 3 patches that you supplied on emacs bzr revno 117517. I have tested this on 24.3.93 as well.

Thanks.  This is indeed very similar to what Martin presented,
i.e. forward-line moves point outside of the window, and then
redisplay returns point back into the view.  But since the change that
fixed Martin's case doesn't fix this one, I guess some other place in
the code is setting the force_start flag of the window.

I will look into this.

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Mon, 29 Sep 2014 09:16:22 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 18545@debbugs.gnu.org
> 
> > Date: Mon, 29 Sep 2014 02:31:04 +0200 (CEST)
> > From: lompik@voila.fr
> > Cc: 18545@debbugs.gnu.org
> > 
> > C-x C-f -> in a folder with lots of files
> > [tab]x2 -> open completion buffer
> > C-! -> modify completion buffer
> > C-h k a -> Show help for letter "a" (Important ! skipping this results in no bug ) 
> > C-` until bottom of *Completions* buffer -> no scrolling,
> > 
> > 
> > I have applied the 3 patches that you supplied on emacs bzr revno 117517. I have tested this on 24.3.93 as well.
> 
> Thanks.  This is indeed very similar to what Martin presented,
> i.e. forward-line moves point outside of the window, and then
> redisplay returns point back into the view.  But since the change that
> fixed Martin's case doesn't fix this one, I guess some other place in
> the code is setting the force_start flag of the window.
> 
> I will look into this.

It's a god-awful mess.  In this use case, we enter that code not
because of the force_start flag, but because of the frame's
frozen_window_starts flag.  That flag is set when we resize the
minibuffer window, and it currently acts almost like the force_start
flag.  (There's a function window_frozen_p, which attempts to exempt
some special windows from this "frozen-start" state, but it cannot
handle this complicated case, because by the time redisplay kicks in,
the fact that *Completions* was at some point displayed by the
selected window is long forgotten, and its window is now just like any
other one.)

The best solution I can propose is in the patch below, which is a
cumulative patch against the emacs-24 branch, and is supposed to fix
all of these problems.  It fixes this last problem by treating the
frozen_window_starts flag as advisory, i.e. like we treat the
optional_new_start flag of a window.

Please test it.  I will commit it later today (to get it into the next
pretest).

=== modified file 'src/window.c'
--- src/window.c	2014-09-11 08:47:34 +0000
+++ src/window.c	2014-09-29 06:03:36 +0000
@@ -5897,6 +5897,8 @@ and redisplay normally--don't erase and 
   w->start_at_line_beg = (bytepos == BEGV_BYTE ||
 			  FETCH_BYTE (bytepos - 1) == '\n');
 
+  wset_redisplay (w);
+
   set_buffer_internal (obuf);
   return Qnil;
 }

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2014-09-18 15:10:33 +0000
+++ src/xdisp.c	2014-09-29 13:13:42 +0000
@@ -15027,6 +15027,10 @@ run_window_scroll_functions (Lisp_Object
    If FORCE_P is non-zero, return 0 even if partial visible cursor row
    is higher than window.
 
+   If CURRENT_MATRIX_P is non-zero, use the information from the
+   window's current glyph matrix; otherwise uze the desired glyph
+   matrix.
+
    A value of 0 means the caller should do scrolling
    as if point had gone off the screen.  */
 
@@ -16136,26 +16140,47 @@ redisplay_window (Lisp_Object window, bo
 
   /* If someone specified a new starting point but did not insist,
      check whether it can be used.  */
-  if (w->optional_new_start
+  if ((w->optional_new_start || window_frozen_p (w))
       && CHARPOS (startp) >= BEGV
       && CHARPOS (startp) <= ZV)
     {
+      ptrdiff_t it_charpos;
+
       w->optional_new_start = 0;
       start_display (&it, w, startp);
       move_it_to (&it, PT, 0, it.last_visible_y, -1,
 		  MOVE_TO_POS | MOVE_TO_X | MOVE_TO_Y);
-      if (IT_CHARPOS (it) == PT)
-	w->force_start = 1;
-      /* IT may overshoot PT if text at PT is invisible.  */
-      else if (IT_CHARPOS (it) > PT && CHARPOS (startp) <= PT)
-	w->force_start = 1;
+      /* Record IT's position now, since line_bottom_y might change
+	 that.  */
+      it_charpos = IT_CHARPOS (it);
+      /* Make sure we set the force_start flag only if the cursor row
+	 will be fully visible.  Otherwise, the code under force_start
+	 label below will try to move point back into view, which is
+	 not what the code which sets optional_new_start wants.  */
+      if (line_bottom_y (&it) < it.last_visible_y && !w->force_start)
+	{
+	  if (it_charpos == PT)
+	    w->force_start = 1;
+	  /* IT may overshoot PT if text at PT is invisible.  */
+	  else if (it_charpos > PT && CHARPOS (startp) <= PT)
+	    w->force_start = 1;
+#ifdef GLYPH_DEBUG
+	  if (w->force_start)
+	    {
+	      if (window_frozen_p (w))
+		debug_method_add (w, "set force_start from frozen window start");
+	      else
+		debug_method_add (w, "set force_start from optional_new_start");
+	    }
+#endif
+	}
     }
 
  force_start:
 
   /* Handle case where place to start displaying has been specified,
      unless the specified location is outside the accessible range.  */
-  if (w->force_start || window_frozen_p (w))
+  if (w->force_start)
     {
       /* We set this later on if we have to adjust point.  */
       int new_vpos = -1;
@@ -16200,7 +16225,7 @@ redisplay_window (Lisp_Object window, bo
 	  goto need_larger_matrices;
 	}
 
-      if (w->cursor.vpos < 0 && !window_frozen_p (w))
+      if (w->cursor.vpos < 0)
 	{
 	  /* If point does not appear, try to move point so it does
 	     appear. The desired matrix has been built above, so we
@@ -16293,6 +16318,11 @@ redisplay_window (Lisp_Object window, bo
 	    }
 	  */
 	}
+      if (w->cursor.vpos < 0 || !cursor_row_fully_visible_p (w, 0, 0))
+	{
+	  clear_glyph_matrix (w->desired_matrix);
+	  goto try_to_scroll;
+	}
 
 #ifdef GLYPH_DEBUG
       debug_method_add (w, "forced window start");
@@ -16357,7 +16387,8 @@ redisplay_window (Lisp_Object window, bo
 	       || CHARPOS (startp) == BEGV
 	       || !window_outdated (w)))
     {
-      int d1, d2, d3, d4, d5, d6;
+      int d1, d2, d5, d6;
+      int rtop, rbot;
 
       /* If first window line is a continuation line, and window start
 	 is inside the modified region, but the first change is before
@@ -16386,10 +16417,14 @@ redisplay_window (Lisp_Object window, bo
 	     doing so will move point from its correct position
 	     instead of scrolling the window to bring point into view.
 	     See bug#9324.  */
-	  && pos_visible_p (w, PT, &d1, &d2, &d3, &d4, &d5, &d6))
+	  && pos_visible_p (w, PT, &d1, &d2, &rtop, &rbot, &d5, &d6)
+	  && rtop == 0 && rbot == 0)
 	{
 	  w->force_start = 1;
 	  SET_TEXT_POS_FROM_MARKER (startp, w->start);
+#ifdef GLYPH_DEBUG
+	  debug_method_add (w, "recomputed window start in continuation line");
+#endif
 	  goto force_start;
       	}
 

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[lompik]

Works great ! Thank you !

> 
> It's a god-awful mess. In this use case, we enter that code not
> because of the force_start flag, but because of the frame's
> frozen_window_starts flag. That flag is set when we resize the
> minibuffer window, and it currently acts almost like the force_start
> flag. (There's a function window_frozen_p, which attempts to exempt
> some special windows from this "frozen-start" state, but it cannot
> handle this complicated case, because by the time redisplay kicks in,
> the fact that *Completions* was at some point displayed by the
> selected window is long forgotten, and its window is now just like any
> other one.)
> 
> The best solution I can propose is in the patch below, which is a
> cumulative patch against the emacs-24 branch, and is supposed to fix
> all of these problems. It fixes this last problem by treating the
> frozen_window_starts flag as advisory, i.e. like we treat the
> optional_new_start flag of a window.
> 
> Please test it. I will commit it later today (to get it into the next
> pretest).
> 
> === modified file 'src/window.c'
> --- src/window.c 2014-09-11 08:47:34 +0000
> +++ src/window.c 2014-09-29 06:03:36 +0000
> @@ -5897,6 +5897,8 @@ and redisplay normally--don't erase and 
> w->start_at_line_beg = (bytepos == BEGV_BYTE ||
> FETCH_BYTE (bytepos - 1) == '\n');
> 
> + wset_redisplay (w);
> +
> set_buffer_internal (obuf);
> return Qnil;
> }
> 
> === modified file 'src/xdisp.c'
> --- src/xdisp.c 2014-09-18 15:10:33 +0000
> +++ src/xdisp.c 2014-09-29 13:13:42 +0000
> @@ -15027,6 +15027,10 @@ run_window_scroll_functions (Lisp_Object
> If FORCE_P is non-zero, return 0 even if partial visible cursor row
> is higher than window.
> 
> + If CURRENT_MATRIX_P is non-zero, use the information from the
> + window's current glyph matrix; otherwise uze the desired glyph
> + matrix.
> +
> A value of 0 means the caller should do scrolling
> as if point had gone off the screen. */
> 
> @@ -16136,26 +16140,47 @@ redisplay_window (Lisp_Object window, bo
> 
> /* If someone specified a new starting point but did not insist,
> check whether it can be used. */
> - if (w->optional_new_start
> + if ((w->optional_new_start || window_frozen_p (w))
> && CHARPOS (startp) >= BEGV
> && CHARPOS (startp) <= ZV)
> {
> + ptrdiff_t it_charpos;
> +
> w->optional_new_start = 0;
> start_display (&it, w, startp);
> move_it_to (&it, PT, 0, it.last_visible_y, -1,
> MOVE_TO_POS | MOVE_TO_X | MOVE_TO_Y);
> - if (IT_CHARPOS (it) == PT)
> - w->force_start = 1;
> - /* IT may overshoot PT if text at PT is invisible. */
> - else if (IT_CHARPOS (it) > PT && CHARPOS (startp) <= PT)
> - w->force_start = 1;
> + /* Record IT's position now, since line_bottom_y might change
> + that. */
> + it_charpos = IT_CHARPOS (it);
> + /* Make sure we set the force_start flag only if the cursor row
> + will be fully visible. Otherwise, the code under force_start
> + label below will try to move point back into view, which is
> + not what the code which sets optional_new_start wants. */
> + if (line_bottom_y (&it) < it.last_visible_y && !w->force_start)
> + {
> + if (it_charpos == PT)
> + w->force_start = 1;
> + /* IT may overshoot PT if text at PT is invisible. */
> + else if (it_charpos > PT && CHARPOS (startp) <= PT)
> + w->force_start = 1;
> +#ifdef GLYPH_DEBUG
> + if (w->force_start)
> + {
> + if (window_frozen_p (w))
> + debug_method_add (w, "set force_start from frozen window start");
> + else
> + debug_method_add (w, "set force_start from optional_new_start");
> + }
> +#endif
> + }
> }
> 
> force_start:
> 
> /* Handle case where place to start displaying has been specified,
> unless the specified location is outside the accessible range. */
> - if (w->force_start || window_frozen_p (w))
> + if (w->force_start)
> {
> /* We set this later on if we have to adjust point. */
> int new_vpos = -1;
> @@ -16200,7 +16225,7 @@ redisplay_window (Lisp_Object window, bo
> goto need_larger_matrices;
> }
> 
> - if (w->cursor.vpos < 0 && !window_frozen_p (w))
> + if (w->cursor.vpos < 0)
> {
> /* If point does not appear, try to move point so it does
> appear. The desired matrix has been built above, so we
> @@ -16293,6 +16318,11 @@ redisplay_window (Lisp_Object window, bo
> }
> */
> }
> + if (w->cursor.vpos < 0 || !cursor_row_fully_visible_p (w, 0, 0))
> + {
> + clear_glyph_matrix (w->desired_matrix);
> + goto try_to_scroll;
> + }
> 
> #ifdef GLYPH_DEBUG
> debug_method_add (w, "forced window start");
> @@ -16357,7 +16387,8 @@ redisplay_window (Lisp_Object window, bo
> || CHARPOS (startp) == BEGV
> || !window_outdated (w)))
> {
> - int d1, d2, d3, d4, d5, d6;
> + int d1, d2, d5, d6;
> + int rtop, rbot;
> 
> /* If first window line is a continuation line, and window start
> is inside the modified region, but the first change is before
> @@ -16386,10 +16417,14 @@ redisplay_window (Lisp_Object window, bo
> doing so will move point from its correct position
> instead of scrolling the window to bring point into view.
> See bug#9324. */
> - && pos_visible_p (w, PT, &d1, &d2, &d3, &d4, &d5, &d6))
> + && pos_visible_p (w, PT, &d1, &d2, &rtop, &rbot, &d5, &d6)
> + && rtop == 0 && rbot == 0)
> {
> w->force_start = 1;
> SET_TEXT_POS_FROM_MARKER (startp, w->start);
> +#ifdef GLYPH_DEBUG
> + debug_method_add (w, "recomputed window start in continuation line");
> +#endif
> goto force_start;
> }
> 
> 
> 

___________________________________________________________
Mode, hifi, maison,… J'achète malin. Je compare les prix avec Voila.fr http://shopping.voila.fr/

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Mon, 29 Sep 2014 16:21:40 +0200 (CEST)
> From: lompik@voila.fr
> Cc: 18545@debbugs.gnu.org
> 
> Works great ! Thank you !

Thanks for testing.  I would actually like to commit a slightly
different patch below, which should be safer, as it won't fail with
very tall lines which are taller than the window in which you scroll.

So if you have time, please try the patch below.  I already verified
that it works with all 3 of your recipes, but I understand that your
real use case is with Helm, which I don't have and couldn't try.

Thanks.

=== modified file 'src/window.c'
--- src/window.c	2014-09-11 08:47:34 +0000
+++ src/window.c	2014-09-29 06:03:36 +0000
@@ -5897,6 +5897,8 @@ and redisplay normally--don't erase and 
   w->start_at_line_beg = (bytepos == BEGV_BYTE ||
 			  FETCH_BYTE (bytepos - 1) == '\n');
 
+  wset_redisplay (w);
+
   set_buffer_internal (obuf);
   return Qnil;
 }

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2014-09-18 15:10:33 +0000
+++ src/xdisp.c	2014-09-29 16:35:45 +0000
@@ -15027,6 +15027,10 @@ run_window_scroll_functions (Lisp_Object
    If FORCE_P is non-zero, return 0 even if partial visible cursor row
    is higher than window.
 
+   If CURRENT_MATRIX_P is non-zero, use the information from the
+   window's current glyph matrix; otherwise uze the desired glyph
+   matrix.
+
    A value of 0 means the caller should do scrolling
    as if point had gone off the screen.  */
 
@@ -16136,26 +16140,48 @@ redisplay_window (Lisp_Object window, bo
 
   /* If someone specified a new starting point but did not insist,
      check whether it can be used.  */
-  if (w->optional_new_start
+  if ((w->optional_new_start || window_frozen_p (w))
       && CHARPOS (startp) >= BEGV
       && CHARPOS (startp) <= ZV)
     {
+      ptrdiff_t it_charpos;
+
       w->optional_new_start = 0;
       start_display (&it, w, startp);
       move_it_to (&it, PT, 0, it.last_visible_y, -1,
 		  MOVE_TO_POS | MOVE_TO_X | MOVE_TO_Y);
-      if (IT_CHARPOS (it) == PT)
-	w->force_start = 1;
-      /* IT may overshoot PT if text at PT is invisible.  */
-      else if (IT_CHARPOS (it) > PT && CHARPOS (startp) <= PT)
-	w->force_start = 1;
+      /* Record IT's position now, since line_bottom_y might change
+	 that.  */
+      it_charpos = IT_CHARPOS (it);
+      /* Make sure we set the force_start flag only if the cursor row
+	 will be fully visible.  Otherwise, the code under force_start
+	 label below will try to move point back into view, which is
+	 not what the code which sets optional_new_start wants.  */
+      if ((it.current_y == 0 || line_bottom_y (&it) < it.last_visible_y)
+	  && !w->force_start)
+	{
+	  if (it_charpos == PT)
+	    w->force_start = 1;
+	  /* IT may overshoot PT if text at PT is invisible.  */
+	  else if (it_charpos > PT && CHARPOS (startp) <= PT)
+	    w->force_start = 1;
+#ifdef GLYPH_DEBUG
+	  if (w->force_start)
+	    {
+	      if (window_frozen_p (w))
+		debug_method_add (w, "set force_start from frozen window start");
+	      else
+		debug_method_add (w, "set force_start from optional_new_start");
+	    }
+#endif
+	}
     }
 
  force_start:
 
   /* Handle case where place to start displaying has been specified,
      unless the specified location is outside the accessible range.  */
-  if (w->force_start || window_frozen_p (w))
+  if (w->force_start)
     {
       /* We set this later on if we have to adjust point.  */
       int new_vpos = -1;
@@ -16200,7 +16226,7 @@ redisplay_window (Lisp_Object window, bo
 	  goto need_larger_matrices;
 	}
 
-      if (w->cursor.vpos < 0 && !window_frozen_p (w))
+      if (w->cursor.vpos < 0)
 	{
 	  /* If point does not appear, try to move point so it does
 	     appear. The desired matrix has been built above, so we
@@ -16293,6 +16319,11 @@ redisplay_window (Lisp_Object window, bo
 	    }
 	  */
 	}
+      if (w->cursor.vpos < 0 || !cursor_row_fully_visible_p (w, 0, 0))
+	{
+	  clear_glyph_matrix (w->desired_matrix);
+	  goto try_to_scroll;
+	}
 
 #ifdef GLYPH_DEBUG
       debug_method_add (w, "forced window start");
@@ -16357,7 +16388,8 @@ redisplay_window (Lisp_Object window, bo
 	       || CHARPOS (startp) == BEGV
 	       || !window_outdated (w)))
     {
-      int d1, d2, d3, d4, d5, d6;
+      int d1, d2, d5, d6;
+      int rtop, rbot;
 
       /* If first window line is a continuation line, and window start
 	 is inside the modified region, but the first change is before
@@ -16382,14 +16414,20 @@ redisplay_window (Lisp_Object window, bo
 	  && compute_window_start_on_continuation_line (w)
 	  /* It doesn't make sense to force the window start like we
 	     do at label force_start if it is already known that point
-	     will not be visible in the resulting window, because
+	     will not be fully visible in the resulting window, because
 	     doing so will move point from its correct position
 	     instead of scrolling the window to bring point into view.
 	     See bug#9324.  */
-	  && pos_visible_p (w, PT, &d1, &d2, &d3, &d4, &d5, &d6))
+	  && pos_visible_p (w, PT, &d1, &d2, &rtop, &rbot, &d5, &d6)
+	  /* A very tall row could need more than the window height,
+	     in which case we accept that it is partially visible.  */
+	  && (rtop != 0) == (rbot != 0))
 	{
 	  w->force_start = 1;
 	  SET_TEXT_POS_FROM_MARKER (startp, w->start);
+#ifdef GLYPH_DEBUG
+	  debug_method_add (w, "recomputed window start in continuation line");
+#endif
 	  goto force_start;
       	}
 

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[lompik]

I tested revno 117521. No more issues. 

I also experimented with increased font size + tall lines (which were truncated) in buffer and was unable to reproduce the issue. 

Thanks again !

> Thanks for testing. I would actually like to commit a slightly
> different patch below, which should be safer, as it won't fail with
> very tall lines which are taller than the window in which you scroll.
> 
> So if you have time, please try the patch below. I already verified
> that it works with all 3 of your recipes, but I understand that your
> real use case is with Helm, which I don't have and couldn't try.
> 
> Thanks.
> 
> === modified file 'src/window.c'
> --- src/window.c 2014-09-11 08:47:34 +0000
> +++ src/window.c 2014-09-29 06:03:36 +0000
> @@ -5897,6 +5897,8 @@ and redisplay normally--don't erase and 
> w->start_at_line_beg = (bytepos == BEGV_BYTE ||
> FETCH_BYTE (bytepos - 1) == '\n');
> 
> + wset_redisplay (w);
> +
> set_buffer_internal (obuf);
> return Qnil;
> }
> 
> === modified file 'src/xdisp.c'
> --- src/xdisp.c 2014-09-18 15:10:33 +0000
> +++ src/xdisp.c 2014-09-29 16:35:45 +0000
> @@ -15027,6 +15027,10 @@ run_window_scroll_functions (Lisp_Object
> If FORCE_P is non-zero, return 0 even if partial visible cursor row
> is higher than window.
> 
> + If CURRENT_MATRIX_P is non-zero, use the information from the
> + window's current glyph matrix; otherwise uze the desired glyph
> + matrix.
> +
> A value of 0 means the caller should do scrolling
> as if point had gone off the screen. */
> 
> @@ -16136,26 +16140,48 @@ redisplay_window (Lisp_Object window, bo
> 
> /* If someone specified a new starting point but did not insist,
> check whether it can be used. */
> - if (w->optional_new_start
> + if ((w->optional_new_start || window_frozen_p (w))
> && CHARPOS (startp) >= BEGV
> && CHARPOS (startp) <= ZV)
> {
> + ptrdiff_t it_charpos;
> +
> w->optional_new_start = 0;
> start_display (&it, w, startp);
> move_it_to (&it, PT, 0, it.last_visible_y, -1,
> MOVE_TO_POS | MOVE_TO_X | MOVE_TO_Y);
> - if (IT_CHARPOS (it) == PT)
> - w->force_start = 1;
> - /* IT may overshoot PT if text at PT is invisible. */
> - else if (IT_CHARPOS (it) > PT && CHARPOS (startp) <= PT)
> - w->force_start = 1;
> + /* Record IT's position now, since line_bottom_y might change
> + that. */
> + it_charpos = IT_CHARPOS (it);
> + /* Make sure we set the force_start flag only if the cursor row
> + will be fully visible. Otherwise, the code under force_start
> + label below will try to move point back into view, which is
> + not what the code which sets optional_new_start wants. */
> + if ((it.current_y == 0 || line_bottom_y (&it) < it.last_visible_y)
> + && !w->force_start)
> + {
> + if (it_charpos == PT)
> + w->force_start = 1;
> + /* IT may overshoot PT if text at PT is invisible. */
> + else if (it_charpos > PT && CHARPOS (startp) <= PT)
> + w->force_start = 1;
> +#ifdef GLYPH_DEBUG
> + if (w->force_start)
> + {
> + if (window_frozen_p (w))
> + debug_method_add (w, "set force_start from frozen window start");
> + else
> + debug_method_add (w, "set force_start from optional_new_start");
> + }
> +#endif
> + }
> }
> 
> force_start:
> 
> /* Handle case where place to start displaying has been specified,
> unless the specified location is outside the accessible range. */
> - if (w->force_start || window_frozen_p (w))
> + if (w->force_start)
> {
> /* We set this later on if we have to adjust point. */
> int new_vpos = -1;
> @@ -16200,7 +16226,7 @@ redisplay_window (Lisp_Object window, bo
> goto need_larger_matrices;
> }
> 
> - if (w->cursor.vpos < 0 && !window_frozen_p (w))
> + if (w->cursor.vpos < 0)
> {
> /* If point does not appear, try to move point so it does
> appear. The desired matrix has been built above, so we
> @@ -16293,6 +16319,11 @@ redisplay_window (Lisp_Object window, bo
> }
> */
> }
> + if (w->cursor.vpos < 0 || !cursor_row_fully_visible_p (w, 0, 0))
> + {
> + clear_glyph_matrix (w->desired_matrix);
> + goto try_to_scroll;
> + }
> 
> #ifdef GLYPH_DEBUG
> debug_method_add (w, "forced window start");
> @@ -16357,7 +16388,8 @@ redisplay_window (Lisp_Object window, bo
> || CHARPOS (startp) == BEGV
> || !window_outdated (w)))
> {
> - int d1, d2, d3, d4, d5, d6;
> + int d1, d2, d5, d6;
> + int rtop, rbot;
> 
> /* If first window line is a continuation line, and window start
> is inside the modified region, but the first change is before
> @@ -16382,14 +16414,20 @@ redisplay_window (Lisp_Object window, bo
> && compute_window_start_on_continuation_line (w)
> /* It doesn't make sense to force the window start like we
> do at label force_start if it is already known that point
> - will not be visible in the resulting window, because
> + will not be fully visible in the resulting window, because
> doing so will move point from its correct position
> instead of scrolling the window to bring point into view.
> See bug#9324. */
> - && pos_visible_p (w, PT, &d1, &d2, &d3, &d4, &d5, &d6))
> + && pos_visible_p (w, PT, &d1, &d2, &rtop, &rbot, &d5, &d6)
> + /* A very tall row could need more than the window height,
> + in which case we accept that it is partially visible. */
> + && (rtop != 0) == (rbot != 0))
> {
> w->force_start = 1;
> SET_TEXT_POS_FROM_MARKER (startp, w->start);
> +#ifdef GLYPH_DEBUG
> + debug_method_add (w, "recomputed window start in continuation line");
> +#endif
> goto force_start;
> }
> 
> 
> 

___________________________________________________________
Mode, hifi, maison,… J'achète malin. Je compare les prix avec Voila.fr http://shopping.voila.fr/

bug#18545: 24.4.50: Bug - forward-line inside with-selected-window

[Eli Zaretskii]

> Date: Tue, 30 Sep 2014 00:56:38 +0200 (CEST)
> From: lompik@voila.fr
> Cc: 18545@debbugs.gnu.org
> 
> I tested revno 117521. No more issues. 
> 
> I also experimented with increased font size + tall lines (which were truncated) in buffer and was unable to reproduce the issue. 
> 
> Thanks again !

OK, thanks.  I'm closing this bug.