bug#12170: save-excursion fails boundary case with recenter

bug#12170: save-excursion fails boundary case with recenter

[Bill Brodie]

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

Running GNU Emacs 24.1.1 (i386-mingw-nt6.1.7601) of 2012-06-10 on MARVIN
under Windows 7.

 

Define:

 

                (defun f (n) (save-excursion (forward-line (- n)) (recenter
0)))

 

Let H be the window height in lines, and suppose that point is positioned at
the beginning of line L > H.

 

Despite 'save-excursion', (f K) moves point if K = H.  It leaves point
unchanged for any other value of K, 0 <= K < L.

 

 


[-- Attachment #2: Type: text/html, Size: 2450 bytes --]

bug#12170: save-excursion fails boundary case with recenter

[martin rudalics]

 >                 (defun f (n) (save-excursion (forward-line (- n)) (recenter
 > 0)))
 >
 >
 >
 > Let H be the window height in lines, and suppose that point is positioned at
 > the beginning of line L > H.
 >
 >
 >
 > Despite 'save-excursion', (f K) moves point if K = H.  It leaves point
 > unchanged for any other value of K, 0 <= K < L.

I'm not sure whether I clearly understand your scenario.  Suppose with
emacs -Q I evaluate

(progn
   (defun f (n)
     (save-excursion (forward-line (- n)) (recenter 0)))
   (let ((buffer (switch-to-buffer "foo"))
	(height (window-height (get-buffer-window "foo"))))
     (insert-char 10 (* height 2))
     (let ((pt (point)))
       (f height)
       (message "old %s new %s" pt (point)))))

Then the values printed by the message are equal.  Please elaborate.

Thanks, martin

bug#12170: save-excursion fails boundary case with recenter

[Bastien]

martin rudalics <rudalics@gmx.at> writes:

> Then the values printed by the message are equal.  

Indeed.

> Please elaborate.

I think the OP lacks something like `save-visual-excursion'.

-- 
 Bastien

bug#12170: save-excursion fails boundary case with recenter

[martin rudalics]

 > I think the OP lacks something like `save-visual-excursion'.

Then `save-window-excursion' should handle it.

martin

bug#12170: save-excursion fails boundary case with recenter

[Bill Brodie]

Martin,

Thanks for the test code and clarification request.

(1) By "window height", I meant the number of lines displayed for the actual
buffer, not counting the mode line or minibuffer.  It turns out this is one
less than the value returned by `window-height'.

(2) The value of `point' changes (the cursor "hops", in other words) when
the window is redisplayed, or when control returns to the user.

Here is code that will produce the bug (with emacs -Q):

(progn
   (defun f (n)
     (save-excursion (forward-line (- n)) (recenter 0)))
   (let ((buffer (switch-to-buffer "foo"))
         (height (1- (window-height (get-buffer-window "foo")))))
     (insert-char 10 (* height 2))
     (let ((pt (point)))
       (f height)
       (redisplay)
       (message "height %s old %s new %s" height pt (point)))))

If you leave out the `redisplay' call, on the other hand, old = new in the
message -- but after control returns to the user, point has still been moved
in the buffer.

Bill

-----Original Message-----
From: martin rudalics [mailto:rudalics@gmx.at] 
Sent: Friday, August 10, 2012 5:34 AM
To: Bill Brodie
Cc: 12170@debbugs.gnu.org
Subject: Re: bug#12170: save-excursion fails boundary case with recenter

 >                 (defun f (n) (save-excursion (forward-line (- n))
(recenter
 > 0)))
 >
 >
 >
 > Let H be the window height in lines, and suppose that point is positioned
at  > the beginning of line L > H.
 >
 >
 >
 > Despite 'save-excursion', (f K) moves point if K = H.  It leaves point  >
unchanged for any other value of K, 0 <= K < L.

I'm not sure whether I clearly understand your scenario.  Suppose with emacs
-Q I evaluate

(progn
   (defun f (n)
     (save-excursion (forward-line (- n)) (recenter 0)))
   (let ((buffer (switch-to-buffer "foo"))
	(height (window-height (get-buffer-window "foo"))))
     (insert-char 10 (* height 2))
     (let ((pt (point)))
       (f height)
       (message "old %s new %s" pt (point)))))

Then the values printed by the message are equal.  Please elaborate.

Thanks, martin

bug#12170: save-excursion fails boundary case with recenter

[martin rudalics]

 > (1) By "window height", I meant the number of lines displayed for the actual
 > buffer, not counting the mode line or minibuffer.  It turns out this is one
 > less than the value returned by `window-height'.
 >
 > (2) The value of `point' changes (the cursor "hops", in other words) when
 > the window is redisplayed, or when control returns to the user.
 >
 > Here is code that will produce the bug (with emacs -Q):
 >
 > (progn
 >    (defun f (n)
 >      (save-excursion (forward-line (- n)) (recenter 0)))
 >    (let ((buffer (switch-to-buffer "foo"))
 >          (height (1- (window-height (get-buffer-window "foo")))))
 >      (insert-char 10 (* height 2))
 >      (let ((pt (point)))
 >        (f height)
 >        (redisplay)
 >        (message "height %s old %s new %s" height pt (point)))))
 >
 > If you leave out the `redisplay' call, on the other hand, old = new in the
 > message -- but after control returns to the user, point has still been moved
 > in the buffer.

I see it now, thanks.  Surprisingly it doesn't show up with code like

(progn
   (defmacro save-this-window-excursion (&rest body)
     "..."
     (let ((start (make-symbol "start"))
	  (point (make-symbol "point")))
       `(let ((,start (copy-marker (window-start)))
	     (,point (copy-marker (window-point))))
	 (save-selected-window
	   (progn ,@body))
	 (set-window-start (selected-window) ,start t)
	 (set-window-point (selected-window) ,point))))

   (defun f (n)
      (save-this-window-excursion (forward-line (- n)) (recenter 0)))

    (let ((buffer (switch-to-buffer "foo"))
          (height (1- (window-height (get-buffer-window "foo")))))
      (insert-char 10 (* height 2))
      (let ((pt (point)))
        (f height)
        (redisplay)
        (message "height %s old %s new %s" height pt (point)))))

so I'd suspect the culprit somewhere in redisplay_window's code

   if (w->optional_new_start

w->optional_new_start is 1 from `recenter'

       && CHARPOS (startp) >= BEGV
       && CHARPOS (startp) <= ZV)
     {
       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;

w->force_start 1 will cause redisplay to honor the start position set up
by `recenter' despite of save_excursion_restore's Fgoto_char.

     }

But I don't have the slightest idea how calling

	 (set-window-start (selected-window) ,start t)

would remedy this.  Eli will soon teach us a lesson here.

martin

bug#12170: save-excursion fails boundary case with recenter

[Stefan Monnier]

> (progn
>    (defun f (n)
>      (save-excursion (forward-line (- n)) (recenter 0)))
>    (let ((buffer (switch-to-buffer "foo"))
>          (height (1- (window-height (get-buffer-window "foo")))))
>      (insert-char 10 (* height 2))
>      (let ((pt (point)))
>        (f height)
>        (redisplay)
>        (message "height %s old %s new %s" height pt (point)))))

As mentioned by Martin, this is a misunderstanding about what
save-excursion does and what `point' is.

Every buffer can have many different `point's (it basically has one per
window, accessible via `window-point' and changeable via
`set-window-point', plus one for itself, called `point').
`save-excursion' preserves only `point'.
`recenter' changes `window-point'.

But `point' and `window-point' are linked (point is set to window-point
and vice-versa in various occasions), so they're often confused.

It seems your real problem is not that `point' changes but that the
cursor ends up in a different position than the one you wanted (the
cursor position, is represented by `window-point' rather than by
`point'), right?

If so, you want to preserve window-point.  And there's nothing quite
like save-excursion to preserve window-point.  You can try
save-window-excursion, tho it will do a lot more than you asked for.
Or otherwise manually read window-point at the beginning and
set-window-point at the end.


        Stefan

bug#12170: save-excursion fails boundary case with recenter

[Bill Brodie]

Thanks, Stefan.

The purpose of the original code was to scroll the window so that a previous
section marker is at the top of the window only if it will fit without
moving point:

(defun f ()
   (interactive)
   (save-excursion
     (re-search-backward "^__")
     (beginning-of-line)
     (recenter 0)))

I was surprised to discover that this fails if the span of lines from the
previous section marker to the current position, inclusive, just barely
won't fit in the window.

Perhaps the moral is that `save-excursion' should not enclose any code,
including `recenter', that affects how buffers are displayed within windows.

Bill

-----Original Message-----

> (progn
>    (defun f (n)
>      (save-excursion (forward-line (- n)) (recenter 0)))
>    (let ((buffer (switch-to-buffer "foo"))
>          (height (1- (window-height (get-buffer-window "foo")))))
>      (insert-char 10 (* height 2))
>      (let ((pt (point)))
>        (f height)
>        (redisplay)
>        (message "height %s old %s new %s" height pt (point)))))

As mentioned by Martin, this is a misunderstanding about what save-excursion
does and what `point' is.

Every buffer can have many different `point's (it basically has one per
window, accessible via `window-point' and changeable via `set-window-point',
plus one for itself, called `point').
`save-excursion' preserves only `point'.
`recenter' changes `window-point'.

But `point' and `window-point' are linked (point is set to window-point and
vice-versa in various occasions), so they're often confused.

It seems your real problem is not that `point' changes but that the cursor
ends up in a different position than the one you wanted (the cursor
position, is represented by `window-point' rather than by `point'), right?

If so, you want to preserve window-point.  And there's nothing quite like
save-excursion to preserve window-point.  You can try save-window-excursion,
tho it will do a lot more than you asked for.
Or otherwise manually read window-point at the beginning and
set-window-point at the end.


        Stefan

bug#12170: save-excursion fails boundary case with recenter

[Stefan Monnier]

> But I don't have the slightest idea how calling
> 	 (set-window-start (selected-window) ,start t)
> would remedy this.  Eli will soon teach us a lesson here.

The redisplay tries to make sure (window-)point is visible, which can
either be done by changing (window-)point or by changing window-start.

The first happens when you use a scrolling operation (which sets
window-start), the other happens in response to normal navigation in
the buffer.

Knowing which to do when is not obvious, but IIUC if window-start has
not been changed, then the redisplay assumes that it is free to change
(window-)point, whereas otherwise it will assume that it should not
change (window-)point but can instead change window-start.


        Stefan

bug#12170: save-excursion fails boundary case with recenter

[martin rudalics]

 > As mentioned by Martin, this is a misunderstanding about what
 > save-excursion does and what `point' is.

As a matter of fact, I thought so initially but did not mention it.

 > Every buffer can have many different `point's (it basically has one per
 > window, accessible via `window-point' and changeable via
 > `set-window-point', plus one for itself, called `point').
 > `save-excursion' preserves only `point'.
 > `recenter' changes `window-point'.

The scenario shows up with one single window showing that buffer.

 > But `point' and `window-point' are linked (point is set to window-point
 > and vice-versa in various occasions), so they're often confused.
 >
 > It seems your real problem is not that `point' changes but that the
 > cursor ends up in a different position than the one you wanted (the
 > cursor position, is represented by `window-point' rather than by
 > `point'), right?
 >
 > If so, you want to preserve window-point.  And there's nothing quite
 > like save-excursion to preserve window-point.  You can try
 > save-window-excursion, tho it will do a lot more than you asked for.
 > Or otherwise manually read window-point at the beginning and
 > set-window-point at the end.

Neither of these cut it.  Maybe the purpose of `recenter' is to
overrule, if necessary, any further point movement until the next
redisplay.  But I still don't understand how putting in a call of
`set-window-start' with NOFORCE t can calm it.

martin

bug#12170: save-excursion fails boundary case with recenter

[martin rudalics]

 > Perhaps the moral is that `save-excursion' should not enclose any code,
 > including `recenter', that affects how buffers are displayed within windows.

Ideally, code should not call `recenter' unless it's sure that no
point-changing code follows it before the next redisplay.  Or better,
code should call `recenter' iff it definitely wants to overrule any
point-changing code that follows it.

martin

bug#12170: save-excursion fails boundary case with recenter

[martin rudalics]

 > Knowing which to do when is not obvious, but IIUC if window-start has
 > not been changed, then the redisplay assumes that it is free to change
 > (window-)point, whereas otherwise it will assume that it should not
 > change (window-)point but can instead change window-start.

IIUC redisplay_window respects the window start position specified in
w->start provided w->force_start or w->frozen_window_start_p hold.
Scrolling, recentering, and things like `move-to-window-line' set the
former (frozen_window_start_p seems purely internal).  The question is
whether these should prevail subsequent point movement or, subsequent
point movement should prevail and reset these flags.

martin

bug#12170: save-excursion fails boundary case with recenter

[Stefan Monnier]

>> Knowing which to do when is not obvious, but IIUC if window-start has
>> not been changed, then the redisplay assumes that it is free to change
>> (window-)point, whereas otherwise it will assume that it should not
>> change (window-)point but can instead change window-start.

> IIUC redisplay_window respects the window start position specified in
> w->start provided w->force_start or w->frozen_window_start_p hold.
> Scrolling, recentering, and things like `move-to-window-line' set the
> former (frozen_window_start_p seems purely internal).  The question is
> whether these should prevail subsequent point movement or, subsequent
> point movement should prevail and reset these flags.

Point movement can't convenient reset those flags because it's very
frequent (so speed matters) and because those flags are about windows,
whereas point movement is only indirectly related to windows (and only
if the selected window happens to display the current buffer).


        Stefan

bug#12170: save-excursion fails boundary case with recenter

[Eli Zaretskii]

> From: "Bill Brodie" <wbrodie@panix.com>
> Date: Fri, 10 Aug 2012 09:40:24 -0400
> Cc: 12170@debbugs.gnu.org
> 
> (1) By "window height", I meant the number of lines displayed for the actual
> buffer, not counting the mode line or minibuffer.  It turns out this is one
> less than the value returned by `window-height'.

As documented, window-height returns the height of the window
including the mode line (and the header line, if there is any).  So
the above is expected.

bug#12170: save-excursion fails boundary case with recenter

[Eli Zaretskii]

> Date: Fri, 10 Aug 2012 16:47:09 +0200
> From: martin rudalics <rudalics@gmx.at>
> Cc: 12170@debbugs.gnu.org
> 
>    (defun f (n)
>       (save-this-window-excursion (forward-line (- n)) (recenter 0)))
> 
>     (let ((buffer (switch-to-buffer "foo"))
>           (height (1- (window-height (get-buffer-window "foo")))))
>       (insert-char 10 (* height 2))
>       (let ((pt (point)))
>         (f height)
>         (redisplay)
>         (message "height %s old %s new %s" height pt (point)))))
> 
> so I'd suspect the culprit somewhere in redisplay_window's code

By saying "culprit" you mean you think this is a bug?  I think Stefan
explained why it isn't.

>    if (w->optional_new_start
> 
> w->optional_new_start is 1 from `recenter'
> 
>        && CHARPOS (startp) >= BEGV
>        && CHARPOS (startp) <= ZV)
>      {
>        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;
> 
> w->force_start 1 will cause redisplay to honor the start position set up
> by `recenter'

Only if point will be visible when window is displayed starting at
startp.

> But I don't have the slightest idea how calling
> 
> 	 (set-window-start (selected-window) ,start t)
> 
> would remedy this.  Eli will soon teach us a lesson here.

Not sure there's a lesson here to learn, but set-window-start sets the
w->force_start flag unconditionally, so it will hold even if point is
not in the displayed portion of the window (i.e., point will move).
Does that explain things?

bug#12170: save-excursion fails boundary case with recenter

[martin rudalics]

 >> (1) By "window height", I meant the number of lines displayed for the actual
 >> buffer, not counting the mode line or minibuffer.  It turns out this is one
 >> less than the value returned by `window-height'.
 >
 > As documented, window-height returns the height of the window
 > including the mode line (and the header line, if there is any).  So
 > the above is expected.

To avoid this confusion `window-total-height' should be used instead of
`window-height'.  The "number of lines displayed for the actual buffer"
is returned by the function `window-body-height'.

martin

bug#12170: save-excursion fails boundary case with recenter

[martin rudalics]

 >>    (defun f (n)
 >>       (save-this-window-excursion (forward-line (- n)) (recenter 0)))
 >>
 >>     (let ((buffer (switch-to-buffer "foo"))
 >>           (height (1- (window-height (get-buffer-window "foo")))))
 >>       (insert-char 10 (* height 2))
 >>       (let ((pt (point)))
 >>         (f height)
 >>         (redisplay)
 >>         (message "height %s old %s new %s" height pt (point)))))
 >>
 >> so I'd suspect the culprit somewhere in redisplay_window's code
 >
 > By saying "culprit" you mean you think this is a bug?

I suppose so, yes.

 > I think Stefan
 > explained why it isn't.

With the OP's last recipe, `point' moves to the position implied by the
`recenter' call.  With the `save-this-window-excursion' macro I gave,
`point' moves to the position implied by that macro instead and the
position implied by the `recenter' call is ignored.  So here something
unexplained happens.

If we wanted to document the current behavior, we'd have to tell users
that if they call any of the window-related movement functions like
`recenter', `move-to-window-line', or `set-window-start' with a nil
NOFORCE argument, then `point' may move after the next redisplay in
order to honor the window start position implied by the last of these
functions executed.  Do you agree so far?

If you agree, then we'd have to explain why a subsequent invocation of
`set-window-start' with NOFORCE t can override the setting of the window
start position implied by the last invocation of one of the functions
mentioned above.

 >> w->force_start 1 will cause redisplay to honor the start position set up
 >> by `recenter'
 >
 > Only if point will be visible when window is displayed starting at
 > startp.

I completely miss you here.

 >> But I don't have the slightest idea how calling
 >>
 >> 	 (set-window-start (selected-window) ,start t)
 >>
 >> would remedy this.  Eli will soon teach us a lesson here.
 >
 > Not sure there's a lesson here to learn, but set-window-start sets the
 > w->force_start flag unconditionally,

IMHO

   if (NILP (noforce))
     w->force_start = 1;

means "conditionally".

 > so it will hold even if point is
 > not in the displayed portion of the window (i.e., point will move).
 > Does that explain things?

Not yet ;-)

martin

bug#12170: save-excursion fails boundary case with recenter

[Eli Zaretskii]

> Date: Sat, 11 Aug 2012 11:32:29 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: wbrodie@panix.com, 12170@debbugs.gnu.org
> 
>  >>    (defun f (n)
>  >>       (save-this-window-excursion (forward-line (- n)) (recenter 0)))
>  >>
>  >>     (let ((buffer (switch-to-buffer "foo"))
>  >>           (height (1- (window-height (get-buffer-window "foo")))))
>  >>       (insert-char 10 (* height 2))
>  >>       (let ((pt (point)))
>  >>         (f height)
>  >>         (redisplay)
>  >>         (message "height %s old %s new %s" height pt (point)))))
>  >>
>  >> so I'd suspect the culprit somewhere in redisplay_window's code
>  >
>  > By saying "culprit" you mean you think this is a bug?
> 
> I suppose so, yes.
> 
>  > I think Stefan
>  > explained why it isn't.
> 
> With the OP's last recipe, `point' moves to the position implied by the
> `recenter' call.  With the `save-this-window-excursion' macro I gave,
> `point' moves to the position implied by that macro instead and the
> position implied by the `recenter' call is ignored.  So here something
> unexplained happens.

Your recipe also calls set-window-point, which moves point on its own,
and also does this:

  /* We have to make sure that redisplay updates the window to show
     the new value of point.  */
  if (!EQ (window, selected_window))
    ++windows_or_buffers_changed;

The windows_or_buffers_changed flag will force a thorough redisplay.

Given this, do we still have something unexplained?

> If we wanted to document the current behavior, we'd have to tell users
> that if they call any of the window-related movement functions like
> `recenter', `move-to-window-line', or `set-window-start' with a nil
> NOFORCE argument, then `point' may move after the next redisplay in
> order to honor the window start position implied by the last of these
> functions executed.  Do you agree so far?

I guess.

> If you agree, then we'd have to explain why a subsequent invocation of
> `set-window-start' with NOFORCE t can override the setting of the window
> start position implied by the last invocation of one of the functions
> mentioned above.

You didn't just call set-window-start, you also called
set-window-point.  If I remove that second call, the result of your
code is very different, and the window start position as the macro set
it is still in effect when control is returned.

>  >> w->force_start 1 will cause redisplay to honor the start position set up
>  >> by `recenter'
>  >
>  > Only if point will be visible when window is displayed starting at
>  > startp.
> 
> I completely miss you here.

I meant this code:

       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 only sets the force_start flag if displaying the window starting at
startp will show point visible inside the window.  The call to
move_it_to moves in a simulated display lines, and stops either at
point or at the last pixel position visible in the window, whichever
happens first.  The subsequent test that IT_CHARPOS (it) == PT
verifies that it stopped at point and not because it reached the end
of the text displayed in the window.

Ergo, sometimes setting the window start position will not be
honored.  That's what the comment above all this means when it says:

  /* If someone specified a new starting point but did not insist,
     check whether it can be used.  */

"Did not insist" means that whoever set w->optional_new_start did not
also set w->force_start.  The "check whether it can be used" part
describes what I just explained.

>  > Not sure there's a lesson here to learn, but set-window-start sets the
>  > w->force_start flag unconditionally,
> 
> IMHO
> 
>    if (NILP (noforce))
>      w->force_start = 1;
> 
> means "conditionally".

Right.  So my current theory is that set-window-point is what makes
the difference.

bug#12170: save-excursion fails boundary case with recenter

[martin rudalics]

 > Your recipe also calls set-window-point, which moves point on its own,
 > and also does this:
 >
 >   /* We have to make sure that redisplay updates the window to show
 >      the new value of point.  */
 >   if (!EQ (window, selected_window))
 >     ++windows_or_buffers_changed;
 >
 > The windows_or_buffers_changed flag will force a thorough redisplay.
 >
 > Given this, do we still have something unexplained?

Yes, because `set-window-point' doesn't set windows_or_buffers_changed
in the case at hand since the window is the selected window.  Or am I
missing something?

 >> If you agree, then we'd have to explain why a subsequent invocation of
 >> `set-window-start' with NOFORCE t can override the setting of the window
 >> start position implied by the last invocation of one of the functions
 >> mentioned above.
 >
 > You didn't just call set-window-start, you also called
 > set-window-point.  If I remove that second call, the result of your
 > code is very different, and the window start position as the macro set
 > it is still in effect when control is returned.

But `set-window-point' should be equivalent to `goto-char' here because
the window is the selected window.

 >>  >> w->force_start 1 will cause redisplay to honor the start position set up
 >>  >> by `recenter'
 >>  >
 >>  > Only if point will be visible when window is displayed starting at
 >>  > startp.
 >>
 >> I completely miss you here.
 >
 > I meant this code:
 >
 >        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 only sets the force_start flag if displaying the window starting at
 > startp will show point visible inside the window.  The call to
 > move_it_to moves in a simulated display lines, and stops either at
 > point or at the last pixel position visible in the window, whichever
 > happens first.  The subsequent test that IT_CHARPOS (it) == PT
 > verifies that it stopped at point and not because it reached the end
 > of the text displayed in the window.
 >
 > Ergo, sometimes setting the window start position will not be
 > honored.  That's what the comment above all this means when it says:
 >
 >   /* If someone specified a new starting point but did not insist,
 >      check whether it can be used.  */
 >
 > "Did not insist" means that whoever set w->optional_new_start did not
 > also set w->force_start.  The "check whether it can be used" part
 > describes what I just explained.

I believe you.  But it remains a mystery to me why `set-window-point'
should make a difference here.  As a matter of fact, if I do

(progn
   (defmacro save-this-window-excursion (&rest body)
     "..."
     (let ((start (make-symbol "start"))
       (point (make-symbol "point")))
       `(let ((,start (copy-marker (window-start)))
          (,point (copy-marker (window-point))))
      (save-selected-window
        (progn ,@body))
      (set-window-start (selected-window) ,start t)
      (with-current-buffer (window-buffer (selected-window))
        (goto-char ,point)))))

   (defun f (n)
      (save-this-window-excursion (forward-line (- n)) (recenter 0)))

    (let ((buffer (switch-to-buffer "foo"))
          (height (1- (window-height (get-buffer-window "foo")))))
      (insert-char 10 (* height 2))
      (let ((pt (point)))
        (f height)
        (redisplay)
        (message "height %s old %s new %s" height pt (point)))))

I get the same results as with `set-window-point'.  IMHO the
`set-window-start' call makes the difference but I don't understand why.

martin

bug#12170: save-excursion fails boundary case with recenter

[Eli Zaretskii]

> Date: Sat, 11 Aug 2012 16:22:39 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: wbrodie@panix.com, 12170@debbugs.gnu.org
> 
> (progn
>    (defmacro save-this-window-excursion (&rest body)
>      "..."
>      (let ((start (make-symbol "start"))
>        (point (make-symbol "point")))
>        `(let ((,start (copy-marker (window-start)))
>           (,point (copy-marker (window-point))))
>       (save-selected-window
>         (progn ,@body))
>       (set-window-start (selected-window) ,start t)
>       (with-current-buffer (window-buffer (selected-window))
>         (goto-char ,point)))))
> 
>    (defun f (n)
>       (save-this-window-excursion (forward-line (- n)) (recenter 0)))
> 
>     (let ((buffer (switch-to-buffer "foo"))
>           (height (1- (window-height (get-buffer-window "foo")))))
>       (insert-char 10 (* height 2))
>       (let ((pt (point)))
>         (f height)
>         (redisplay)
>         (message "height %s old %s new %s" height pt (point)))))
> 
> I get the same results as with `set-window-point'.  IMHO the
> `set-window-start' call makes the difference but I don't understand why.

Maybe.  If it's really important, I could try looking at this in the
debugger, although doing so to investigate such situations is
notoriously hard, because redisplay is entered several times.

IMO, save-excursion is simply not designed to make sure display isn't
changed in such cases.  It's for excursions into other portions of the
buffer for processing those other parts, without affecting display.
If the processing also scrolls the screen or calls recenter or
otherwise affects the display, all bets are off, because scrolling can
legitimately move point, and when that happens, it is no longer clear
that restoring point should take precedence over window-start etc.

bug#12170: save-excursion fails boundary case with recenter

[martin rudalics]

 > Maybe.  If it's really important,

It's not.

 > I could try looking at this in the
 > debugger, although doing so to investigate such situations is
 > notoriously hard, because redisplay is entered several times.
 >
 > IMO, save-excursion is simply not designed to make sure display isn't
 > changed in such cases.

Agreed.  But `save-window-excursion' doesn't handle this case either.
So the difference should be in one of the four assignments

   w->start_at_line_beg = 0;
   w->update_mode_line = 1;
   w->last_modified = 0;
   w->last_overlay_modified = 0;

and I'd obviously vote for the first one.  Interesting side-effect.

martin

bug#12170: save-excursion fails boundary case with recenter

[Bill Brodie]

From my point of view (the OP), I've generally used save-excursion to
assemble or manipulate information elsewhere in the buffer or in other
buffers, without explicit reference to window displays.  This seems like a
natural restriction.

I've modified my original code to perform the window operations outside the
form.  Thanks for the illuminating discussion.

-----Original Message-----
From: Eli Zaretskii [mailto:eliz@gnu.org] 

IMO, save-excursion is simply not designed to make sure display isn't
changed in such cases.  It's for excursions into other portions of the
buffer for processing those other parts, without affecting display.
If the processing also scrolls the screen or calls recenter or
otherwise affects the display, all bets are off, because scrolling can
legitimately move point, and when that happens, it is no longer clear
that restoring point should take precedence over window-start etc.

bug#12170: save-excursion fails boundary case with recenter

[Eli Zaretskii]

> Date: Sat, 11 Aug 2012 18:05:14 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: wbrodie@panix.com, 12170@debbugs.gnu.org
> 
> So the difference should be in one of the four assignments
> 
>    w->start_at_line_beg = 0;
>    w->update_mode_line = 1;
>    w->last_modified = 0;
>    w->last_overlay_modified = 0;
> 
> and I'd obviously vote for the first one.  Interesting side-effect.

FWIW, there are only 2 places in redisplay that look at
w->start_at_line_beg, but neither your nor Bill's recipe don't
exercise the code of either of these two places.

bug#12170: save-excursion fails boundary case with recenter

[Eli Zaretskii]

> Date: Sat, 11 Aug 2012 19:31:53 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 12170@debbugs.gnu.org, wbrodie@panix.com
> 
> > Date: Sat, 11 Aug 2012 18:05:14 +0200
> > From: martin rudalics <rudalics@gmx.at>
> > CC: wbrodie@panix.com, 12170@debbugs.gnu.org
> > 
> > So the difference should be in one of the four assignments
> > 
> >    w->start_at_line_beg = 0;
> >    w->update_mode_line = 1;
> >    w->last_modified = 0;
> >    w->last_overlay_modified = 0;
> > 
> > and I'd obviously vote for the first one.  Interesting side-effect.
> 
> FWIW, there are only 2 places in redisplay that look at
> w->start_at_line_beg, but neither your nor Bill's recipe don't
> exercise the code of either of these two places.

As another FWIW, running the recipe with save-window-excursion shows
that set-window-configuration called after the body tries to set the
window start at position 1 and the window point at position 67.  But
the last position in a 33-line window that starts at position 1 is 33,
so these two values cannot be satisfied at once.

bug#12170: save-excursion fails boundary case with recenter

[martin rudalics]

 > As another FWIW, running the recipe with save-window-excursion shows
 > that set-window-configuration called after the body tries to set the
 > window start at position 1 and the window point at position 67.  But
 > the last position in a 33-line window that starts at position 1 is 33,
 > so these two values cannot be satisfied at once.

Due to the fact that we use markers when storing start and point
positions.  Usually the point position prevails.  But apparently
`recenter' is stronger.

martin