Re: Allowing point to be outside the window?

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

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

Sorry for that, and I don't quite remember the problems either.  IIRC,
the one problem that remained unsolved was to find a better solution for
the case where some text changed behind window start than to recenter
the window around point, which I think I've found.

>> +@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)?

The first flag controls the behaviour of redisplay, while the second
controls that of the scrolling commands, so I think they are orthogonal.

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

Agreed, though I can't think of anything better.

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

It allows pixel-scroll to take advantage of the new feature.  You can
just ignore it for now, I simply forgot to remove it.

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

Thanks for catching that.

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

> What does it mean when you set the w->start point, but do NOT set the
> w->force_start flag?

w->force_start will result in point being constrained inside window
start by the code under force_start: in redisplay_window.

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

??? That seems to be a problem which came up after I rebased the changes
onto master from some version of master around late December, not
before.  Thanks for catching it.

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

I saw the code do that in try_window_id, so even though I didn't really
understand it I thought it would be safer to do that here as well.

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

Thanks.

>> +  /* 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?

Yes, IMO.  If the change is actually at point, then the window will be
recentered around point by the `PT != w->last_point' conditional.

Otherwise, such surprising changes to point would be unwanted.

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

It does.  There was a reason I put it there last December, but I can't
remember it right now.

Thanks.