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

[Eli Zaretskii <eliz@gnu.org>]

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