Re: Allowing point to be outside the window?

[Eli Zaretskii <eliz@gnu.org>]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 06 Feb 2022 15:22:57 +0800
> 
> Po Lu <luangruo@yahoo.com> writes:
> 
> > To be completely clear, the "recenter around point" fallback you allude
> > to is the code under the `recenter' label in `redisplay_window',
> > correct?
> >
> > Please forgive me if this has been answered before, but I haven't been
> > working on this in a while, and my memory is already getting rusty.
> >
> > Thanks.
> 
> How about this: we recenter around the position of the first modified
> character?  Point isn't moved at all during that process.

It's very hard to keep a discussion with such long pauses.  I don't
even remember what we were discussing the last time and what issues
remained unresolved.

> +@vindex keep-point-visible
> +  If @code{keep-point-visible} is nil, redisplay will not move recenter
> +the display when the window start is changed.
> +
> +@vindex scroll-move-point
> +  If @code{scroll-move-point} is nil, scrolling commands will not move
> +point to keep it inside the visible part of the window.

Why do we need 2 flags?  Are they indeed orthogonal, or can we have a
single variable (perhaps with more than 2 states)?

> +** New variable 'keep-point-visible'.
> +This variable controls if redisplay will try to keep point visible
> +inside the window.
> +
> ++++
> +** New variable 'scroll-move-point'.
> +This variable controls if scrolling moves point to stay inside the
> +window.

This is waaaay too terse for such a significant change...

> --- a/lisp/pixel-scroll.el
> +++ b/lisp/pixel-scroll.el

Why do we need to have changes in pixel-scroll.el be part of this
changeset?  It makes the changes harder to review, and is not really
related to the changes in the display code.

> --- a/src/window.c
> +++ b/src/window.c
> @@ -5578,7 +5578,8 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, bool noerror)
>       something like (scroll-down 1) with PT in the line before
>       the partially visible one would recenter.  */
>  
> -  if (!pos_visible_p (w, PT, &x, &y, &rtop, &rbot, &rowh, &vpos))
> +  if (!pos_visible_p (w, PT, &x, &y, &rtop, &rbot, &rowh, &vpos)
> +      && scroll_move_point)

Don't you need to test keep_point_visible as well here?  If not, why
not?

> @@ -5659,8 +5660,9 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, bool noerror)
>  		  w->start_at_line_beg = true;
>  		  wset_update_mode_line (w);
>  		  /* Set force_start so that redisplay_window will run the
> -		     window-scroll-functions.  */
> -		  w->force_start = true;
> +		     window-scroll-functions, unless scroll_move_point is false,
> +		     in which case forcing the start will cause recentering.  */
> +		  w->force_start = scroll_move_point;

Should the logic of whether and how to obey the force_start flag be
confined to the display code, instead of having part of it here?  What
does it mean when you set the w->start point, but do NOT set the
w->force_start flag?

> @@ -5844,8 +5846,9 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, bool noerror)
>        w->start_at_line_beg = (pos == BEGV || FETCH_BYTE (bytepos - 1) == '\n');
>        wset_update_mode_line (w);
>        /* Set force_start so that redisplay_window will run the
> -	 window-scroll-functions.  */
> -      w->force_start = true;
> +	 window-scroll-functions, unless scroll_move_point is false,
> +	 in which case forcing the start will cause recentering.  */
> +      w->force_start = scroll_move_point;

Same here.

> @@ -5857,7 +5860,7 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, bool noerror)
>       even if there is a header line.  */
>    this_scroll_margin = window_scroll_margin (w, MARGIN_IN_PIXELS);
>  
> -  if (n > 0)
> +  if (scroll_move_point)

This and the rest of changes in window_scroll_pixel_based are
impossible to review: you replaced the "n > 0" condition with a
different one, and now the rest of the diffs are completely
unreadable.

I also don't understand how come the exact same code which was
previously run only for n > 0 is now run for any value of 'n', and
_by_default_ (since scroll_move_point is non-zero by default)?  How
can that be TRT??

> +  if (!keep_point_visible && window_outdated (w))
> +    {
> +      /* If some text changed between window start, then recenter the
> +	 display around the first character that changed, to avoid
> +	 confusing the user by not updating the display to reflect the
> +	 changes.  */
> +      ptrdiff_t last_changed_charpos, first_changed_charpos;
> +
> +      /* Make sure beg_unchanged and end_unchanged are up to date.  Do it
> +	 only if buffer has really changed.  The reason is that the gap is
> +	 initially at Z for freshly visited files.  The code below would
> +	 set end_unchanged to 0 in that case.  */
> +      if (GPT - BEG < BEG_UNCHANGED)
> +	BEG_UNCHANGED = GPT - BEG;
> +      if (Z - GPT < END_UNCHANGED)
> +	END_UNCHANGED = Z - GPT;

I'm not sure I understand this part.  Why do you need to change the
values of BEG_UNCHANGED and END_UNCHANGED -- those are supposed to be
changed only by code that modifies the buffer text.

> -	    /* -1 means we need to scroll.
> -	       0 means we need new matrices, but fonts_changed
> -	       is set in that case, so we will detect it below.  */
> -	    goto try_to_scroll;
> +	    {
> +	      /* -1 means we need to scroll.
> +		 0 means we need new matrices, but fonts_changed
> +		 is set in that case, so we will detect it below.  */
> +	      goto try_to_scroll;
> +	    }

These braces are redundant.

> +  /* Determine the window start relative to where we want to recenter
> +     to.  */
> +
> +  if (need_recenter_even_if_point_can_be_invisible)
> +    init_iterator (&it, w, that_recentering_position,
> +		   that_recentering_byte, NULL, DEFAULT_FACE_ID);
> +  else
> +    init_iterator (&it, w, PT, PT_BYTE, NULL, DEFAULT_FACE_ID);
>    it.current_y = it.last_visible_y;
> +

You want to display window around the change, but not bring point
there?  Is that a good idea?

> + maybe_try_window:

Why do we need this maybe_try_window stuff?  It seems to repeat
existing code, doesn't it?

Thanks.