Re: move_it_vertically_backward question

[Eli Zaretskii <eliz@gnu.org>]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 19 Dec 2021 08:54:29 +0800
> 
> >> +  else if (CONSP (from))
> >> +    {
> >> +      start = clip_to_bounds (BEGV, fix_position (XCAR (from)), ZV);
> >> +      bpos = CHAR_TO_BYTE (start);
> >> +      CHECK_FIXNAT (XCDR (from));
> >> +      movement = XFIXNAT (XCDR (from));
> >> +    }
> >>    else
> >>      {
> >>        start = clip_to_bounds (BEGV, fix_position (from), ZV);
> 
> > There's code duplication here, which it would be good to avoid: the
> > calculation of bpos and the call to clip_to_bounds in calculation of
> > start.
> 
> I don't think the duplication here is a particularly big problem.

Not a big problem, it just makes the code a tad harder to read and
comprehend.

> > Also, the assumption that this offset is always positive, and that it
> > means the position _before_ FROM, is also something that makes the
> > function less general than it could be.  It would be better to have
> > positive values mean _after_ FROM and negative values to mean before
> > it.  Then you could use move_it_vertically instead of
> > move_it_vertically_backward.
> 
> Does `move_it_vertically' have the same limitation of
> `move_it_vertically_backwards' where the iterator might end up either
> before or after the target position?

Look at the code: when DY is non-positive, move_it_vertically
delegates to move_it_vertically_backwards.

> >> +  if (movement > 0)
> >> +    {
> >> +      int last_y;
> >> +      it.current_y = 0;
> >> +
> >> +      move_it_by_lines (&it, 0);
> 
> > Why is this needed?
> 
> That will move the iterator to the beginning of the visual line.  I
> don't think it makes any sense to move it elsewhere, as IIUC the
> behaviour of moving an iterator upwards from a non-beginning position is
> not very well defined.

I think you assume that the new argument to window-text-pixel-size is
non-nil; then indeed we should move to the beginning of the visual
line.  But in general, I don't think that's true.

Anyway, I don't necessarily object that we should do this always, but
then we should document that when FROM is a cons cell, that specifies
the beginning of the screen line at that pixel offset.

> > The code that follows this, which you leave intact, will reseat the
> > iterator to the beginning of the visual line where you ended up after
> > the loop that goes backward.  Is that really TRT? what if there's a
> > multi-line display or overlay string at that place?  Did you test the
> > code in that case?  AFAIU, you have already assured that you are at
> > the beginning of a visual line, so the reseat, and the following
> > return to the START position, might not be needed.  When you return to
> > START, you may end up very far from where the loop going backward
> > ended, if START is "covered" by a display property or overlay string.
> 
> Yes, that's the correct behaviour here, at least for the pixel scrolling
> code (which I tested with both overlays and multi-line display
> properties).

When window-start is inside a display property or overlay (i.e., the
first thing shown in the window is the result of that display
property/overlay), starting from the underlying buffer position will
almost definitely affect the results, because that buffer position
could be at a very different position on the screen.  For example,
what happens if window-start position has a before-string, and that
string has a 'display' property specifying an image?  This should
display the image as the first display element at the window
beginning, and the buffer position of window-start will then be to the
right and possibly also at a different Y coordinate.