bug#26445: 26.0.50; Scroll margin and cursor movement working incorrectly when scrolling over different height lines

[Eli Zaretskii <eliz@gnu.org>]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Thu, 13 Apr 2017 15:09:14 -0400
> Cc: 26445@debbugs.gnu.org
> 
> This code in `try_cursor_movement' is what's different for scrolling
> vs non-scrolling lines.
> 
>           /* If within the scroll margin, scroll.  Note that
>          MATRIX_ROW_BOTTOM_Y gives the pixel position at which
>          the next line would be drawn, and that
>          this_scroll_margin can be zero.  */
>           if (MATRIX_ROW_BOTTOM_Y (row) > last_y
>           || PT > MATRIX_ROW_END_CHARPOS (row)
>           /* Line is completely visible last line in window
>              and PT is to be set in the next line.  */
>           || (MATRIX_ROW_BOTTOM_Y (row) == last_y
>               && PT == MATRIX_ROW_END_CHARPOS (row)
>               && !row->ends_at_zv_p
>               && !MATRIX_ROW_ENDS_IN_MIDDLE_OF_CHAR_P (row)))
>         scroll_p = true;

Not sure what you are saying: do you see some problem in the above
code?  If so, where do you see a potential problem.

> I think the root issue might be that scroll-margin is given in lines,
> and then it's translated to pixels under the assumption that lines are
> all using the default height.

If that's the problem, I see no good solutions here, because
scroll-conservatively > 100 explicitly requests to position point as
close to the window edge as possible, and the alternating lines of
different height in the recipe of this bug report force us to stop one
line earlier every other scroll.  Am I missing something?

> Although my initial attempt to make
> window_scroll_margin take different line heights into account doesn't
> seem to have any effect. AFAICT, the next test, PT >
> MATRIX_ROW_END_CHARPOS (row), just triggers instead.

Not sure how to interpret what you say here.  If point is beyond
MATRIX_ROW_END_CHARPOS of a row, it means point is not in this row,
because MATRIX_ROW_END_CHARPOS gives the buffer position of the first
character in the next row.  There are no coordinates, pixel or
otherwise, involved here.

> modified   src/window.c
> @@ -4820,10 +4820,17 @@ window_scroll_margin (struct window *window,
> enum margin_unit unit)
>          }
>        int max_margin = min ((window_lines - 1)/2,
>                              (int) (window_lines * ratio));
> -      int margin = clip_to_bounds (0, scroll_margin, max_margin);
> -      return (unit == MARGIN_IN_PIXELS)
> -        ? margin * frame_line_height
> -        : margin;
> +      int margin_lines = clip_to_bounds (0, scroll_margin, max_margin);
> +      if (unit == MARGIN_IN_LINES)
> +          return margin_lines;
> +      else
> +        {
> +          struct it it;
> +          init_iterator (&it, window, BEGV, BEGV_BYTE, NULL, DEFAULT_FACE_ID);
> +          move_it_to (&it, -1, -1, it.last_visible_y, -1, MOVE_TO_Y);
> +          move_it_by_lines (&it, -margin_lines);
> +          return it.last_visible_y - it.current_y;
> +        }

Is this a suggested patch?  If so, how can it fix the issue?  When a
line is taller than the canonical height, positioning point on it
might enter the scroll-margin, and we still need to back up, don't we?
IOW, the above code still move "by lines", and will "stutter" if lines
are intermittently of different height.  What possible solution could
we come up with in such situations?  It seems to me that the user gets
what they asked for.