GNU bug report logs - #12170
save-excursion fails boundary case with recenter

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 12170 in the body.
You can then email your comments to 12170 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Bill Brodie" <wbrodie <at> panix.com>
To: <bug-gnu-emacs <at> gnu.org>
Subject: save-excursion fails boundary case with recenter
Date: Thu, 9 Aug 2012 21:23:04 -0400

[Message part 1 (text/plain, inline)]

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.

 

 

[Message part 2 (text/html, inline)]

Message #8 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Bill Brodie <wbrodie <at> panix.com>
Cc: 12170 <at> debbugs.gnu.org
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 11:34:11 +0200

>                 (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

Message #11 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: Bastien <bzg <at> gnu.org>
To: martin rudalics <rudalics <at> gmx.at>
Cc: 12170 <at> debbugs.gnu.org, Bill Brodie <wbrodie <at> panix.com>
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 11:50:50 +0200

martin rudalics <rudalics <at> 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

Message #14 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Bastien <bzg <at> gnu.org>
Cc: 12170 <at> debbugs.gnu.org, Bill Brodie <wbrodie <at> panix.com>
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 15:03:39 +0200

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

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

martin

Message #17 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: "Bill Brodie" <wbrodie <at> panix.com>
To: "'martin rudalics'" <rudalics <at> gmx.at>
Cc: 12170 <at> debbugs.gnu.org
Subject: RE: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 09:40:24 -0400

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 <at> gmx.at] 
Sent: Friday, August 10, 2012 5:34 AM
To: Bill Brodie
Cc: 12170 <at> 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

Message #20 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Bill Brodie <wbrodie <at> panix.com>
Cc: 12170 <at> debbugs.gnu.org
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 16:47:09 +0200

> (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

Message #23 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: "Bill Brodie" <wbrodie <at> panix.com>
Cc: 'martin rudalics' <rudalics <at> gmx.at>, 12170 <at> debbugs.gnu.org
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 11:04:47 -0400

> (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

Message #26 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: "Bill Brodie" <wbrodie <at> panix.com>
To: "'Stefan Monnier'" <monnier <at> IRO.UMontreal.CA>
Cc: 'martin rudalics' <rudalics <at> gmx.at>, 12170 <at> debbugs.gnu.org
Subject: RE: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 11:46:25 -0400

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

Message #29 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: martin rudalics <rudalics <at> gmx.at>
Cc: 12170 <at> debbugs.gnu.org, Bill Brodie <wbrodie <at> panix.com>
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 12:18:08 -0400

> 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

Message #32 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 12170 <at> debbugs.gnu.org, Bill Brodie <wbrodie <at> panix.com>
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 18:46:39 +0200

> 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

Message #35 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Bill Brodie <wbrodie <at> panix.com>
Cc: 12170 <at> debbugs.gnu.org, 'Stefan Monnier' <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 18:46:53 +0200

> 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

Message #38 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 12170 <at> debbugs.gnu.org, Bill Brodie <wbrodie <at> panix.com>
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 18:47:14 +0200

> 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

Message #41 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: martin rudalics <rudalics <at> gmx.at>
Cc: 12170 <at> debbugs.gnu.org, Bill Brodie <wbrodie <at> panix.com>
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 13:07:12 -0400

>> 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

Message #44 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Bill Brodie <wbrodie <at> panix.com>
Cc: rudalics <at> gmx.at, 12170 <at> debbugs.gnu.org
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 21:58:02 +0300

> From: "Bill Brodie" <wbrodie <at> panix.com>
> Date: Fri, 10 Aug 2012 09:40:24 -0400
> Cc: 12170 <at> 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.

Message #47 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: martin rudalics <rudalics <at> gmx.at>
Cc: 12170 <at> debbugs.gnu.org, wbrodie <at> panix.com
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Fri, 10 Aug 2012 22:01:51 +0300

> Date: Fri, 10 Aug 2012 16:47:09 +0200
> From: martin rudalics <rudalics <at> gmx.at>
> Cc: 12170 <at> 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?

Message #50 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 12170 <at> debbugs.gnu.org, Bill Brodie <wbrodie <at> panix.com>
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Sat, 11 Aug 2012 11:31:32 +0200

>> (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

Message #53 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 12170 <at> debbugs.gnu.org, wbrodie <at> panix.com
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Sat, 11 Aug 2012 11:32:29 +0200

>>    (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

Message #56 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: martin rudalics <rudalics <at> gmx.at>
Cc: 12170 <at> debbugs.gnu.org, wbrodie <at> panix.com
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Sat, 11 Aug 2012 14:11:23 +0300

> Date: Sat, 11 Aug 2012 11:32:29 +0200
> From: martin rudalics <rudalics <at> gmx.at>
> CC: wbrodie <at> panix.com, 12170 <at> 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.

Message #59 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 12170 <at> debbugs.gnu.org, wbrodie <at> panix.com
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Sat, 11 Aug 2012 16:22:39 +0200

> 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

Message #62 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: martin rudalics <rudalics <at> gmx.at>
Cc: 12170 <at> debbugs.gnu.org, wbrodie <at> panix.com
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Sat, 11 Aug 2012 18:10:26 +0300

> Date: Sat, 11 Aug 2012 16:22:39 +0200
> From: martin rudalics <rudalics <at> gmx.at>
> CC: wbrodie <at> panix.com, 12170 <at> 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.

Message #65 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 12170 <at> debbugs.gnu.org, wbrodie <at> panix.com
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Sat, 11 Aug 2012 18:05:14 +0200

> 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

Message #68 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: "Bill Brodie" <wbrodie <at> panix.com>
To: "'Eli Zaretskii'" <eliz <at> gnu.org>,
	"'martin rudalics'" <rudalics <at> gmx.at>
Cc: 12170 <at> debbugs.gnu.org
Subject: RE: bug#12170: save-excursion fails boundary case with recenter
Date: Sat, 11 Aug 2012 12:27:42 -0400

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 <at> 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.

Message #71 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: martin rudalics <rudalics <at> gmx.at>
Cc: 12170 <at> debbugs.gnu.org, wbrodie <at> panix.com
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Sat, 11 Aug 2012 19:31:53 +0300

> Date: Sat, 11 Aug 2012 18:05:14 +0200
> From: martin rudalics <rudalics <at> gmx.at>
> CC: wbrodie <at> panix.com, 12170 <at> 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.

Message #74 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: rudalics <at> gmx.at
Cc: 12170 <at> debbugs.gnu.org
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Sat, 11 Aug 2012 19:49:41 +0300

> Date: Sat, 11 Aug 2012 19:31:53 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: 12170 <at> debbugs.gnu.org, wbrodie <at> panix.com
> 
> > Date: Sat, 11 Aug 2012 18:05:14 +0200
> > From: martin rudalics <rudalics <at> gmx.at>
> > CC: wbrodie <at> panix.com, 12170 <at> 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.

Message #77 received at 12170 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 12170 <at> debbugs.gnu.org
Subject: Re: bug#12170: save-excursion fails boundary case with recenter
Date: Sun, 12 Aug 2012 12:31:05 +0200

> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.