bug#52561: 28.0.50; [PATCH] Tall characters create scrolling stasis

[Eli Zaretskii]

> From: dick.r.chiang@gmail.com
> Date: Thu, 16 Dec 2021 18:01:08 -0500
> 
> After this prints "all bad", try M-v (scroll-down).

It never prints "all bad" in my testing, neither in Emacs 27.2 nor in
Emacs 29.  But the first line is not taller here, so I also tried to
copy the Hindu line from etc/HELLO (which does display taller here) to
try reproducing the issue, and I still get "all good".

> diff --git a/src/xdisp.c b/src/xdisp.c
> index 5e549c9c63..65632ff167 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -18921,7 +18921,7 @@ redisplay_window (Lisp_Object window, bool 
> just_this_one_p)
>    if (w->force_start)
>      {
>        /* We set this later on if we have to adjust point.  */
> -      int new_vpos = -1;
> +      int new_y = -1;
>  
>        w->force_start = false;
>        w->vscroll = 0;
> @@ -18991,28 +18991,16 @@ redisplay_window (Lisp_Object window, bool 
> just_this_one_p)
>                                     NULL, 0);
>           }
>         if (r)
> -         new_vpos = MATRIX_ROW_BOTTOM_Y (r);
> +         new_y = MATRIX_ROW_BOTTOM_Y (r);
>         else  /* Give up and just move to the middle of the window.  */
> -         new_vpos = window_box_height (w) / 2;
> +         new_y = window_box_height (w) / 2;
>       }
>  
>        if (!cursor_row_fully_visible_p (w, false, false, false))
>       {
>         /* Point does appear, but on a line partly visible at end of window.
>            Move it back to a fully-visible line.  */
> -       new_vpos = window_box_height (w);
> -       /* But if window_box_height suggests a Y coordinate that is
> -          not less than we already have, that line will clearly not
> -          be fully visible, so give up and scroll the display.
> -          This can happen when the default face uses a font whose
> -          dimensions are different from the frame's default
> -          font.  */
> -       if (new_vpos >= w->cursor.y)
> -         {
> -           w->cursor.vpos = -1;
> -           clear_glyph_matrix (w->desired_matrix);
> -           goto try_to_scroll;
> -         }
> +       new_y =  WINDOW_BOX_HEIGHT_NO_MODE_LINE (w);
>       }
>        else if (w->cursor.vpos >= 0)
>       {
> @@ -19052,13 +19040,18 @@ redisplay_window (Lisp_Object window, bool 
> just_this_one_p)
>  
>        /* If we need to move point for either of the above reasons,
>        now actually do it.  */
> -      if (new_vpos >= 0)
> +      if (new_y >= 0)
>       {
>         struct glyph_row *row;
>  
> -       row = MATRIX_FIRST_TEXT_ROW (w->desired_matrix);
> -       while (MATRIX_ROW_BOTTOM_Y (row) < new_vpos)
> -         ++row;
> +       for (row = MATRIX_FIRST_TEXT_ROW (w->desired_matrix);
> +            row < MATRIX_BOTTOM_TEXT_ROW (w->desired_matrix, w);
> +            ++row)
> +         if (MATRIX_ROW_BOTTOM_Y (row) - row->extra_line_spacing > new_y)
> +           {
> +             row--;
> +             break;
> +           }
>  
>         TEMP_SET_PT_BOTH (MATRIX_ROW_START_CHARPOS (row),
>                           MATRIX_ROW_START_BYTEPOS (row));

If this is supposed to fix the problem shown by the recipe, then I
don't understand the rationale.

For starters, a breakpoint set in the while/for loop of the last hunk
never breaks for me when I run the recipe, so it seems unrelated.
row->extra_line_spacing is supposed to be zero (and your recipe
doesn't change that), so the original inequality and the one you
propose are equivalent, I think.

As for replacing window_box_height with
WINDOW_BOX_HEIGHT_NO_MODE_LINE: are you saying that this could move
point too much down the window, when we have a header-line and/or a
tab-line?  That could be true (though it's mostly harmless in the use
cases this code is supposed to support), but again, it's unrelated to
the recipe, which specifies neither header-line nor tab-line.

And the part you propose to remove, viz.:

-         /* But if window_box_height suggests a Y coordinate that is
-            not less than we already have, that line will clearly not
-            be fully visible, so give up and scroll the display.
-            This can happen when the default face uses a font whose
-            dimensions are different from the frame's default
-            font.  */
-         if (new_vpos >= w->cursor.y)
-           {
-             w->cursor.vpos = -1;
-             clear_glyph_matrix (w->desired_matrix);
-             goto try_to_scroll;
-           }

was added to fix a bug, bug#17095.  Did you try the recipes provided
there to make sure this doesn't reintroduce that bug back?

Finally, the patch seems to contain unrelated changes to
multisession.el and its test suite?

In general, please make a point of better explaining the rationale for
the changes you propose, and, if you remove code added to fix specific
bugs, please describe the testing you have done to verify those bugs
are not re-introduced.  Without that, how can we possibly accept
changes that potentially make Emacs worse?

Thanks.