Re: move_it_vertically_backward question

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Fri, 17 Dec 2021 09:45:53 +0800
> 
> > The difference is that window-text-pixel-size already exists, we just
> > extend it for another use case.
> 
> > Well, then this is equivalent to telling window-text-pixel-size to
> > stop at the visual left edge of a screen line, right?  That is,
> > interpret TO not as a literal buffer position, but as the visual
> > beginning of the line which shows position TO.
> 
> Yes, I think the two meanings are equivalent.  I documented it as the
> former, but it wouldn't hurt to document it like this instead.

I think it would be a lot easier to understand if we use the
description I proposed.  Lisp programmers do not necessarily know what
are ascent and descent, and what they have to do with this function's
operation.

However, I don't think I see where that additional argument causes the
function to stop at the visual left edge of a screen line containing
TO.  What did I miss?

> Anyway, here are two patches that implement the extensions to
> `window-text-pixel-size', along with the code in
> pixel-scroll-precision-mode that makes use of them.

Please for the next iteration show them as a single patch.  One of
them seems to depend on the other, and it's hard to review the changes
piecemeal.

> I tried to update the documentation, but to be frank I'm not good at
> writing documentation and couldn't word it very well

Don't give up trying!  No one is born with these abilities, they are
acquired by writing documentation and learning from that.

For now, in my comments below I focused on the code, and left the
documentation for later, when we arrive at the final version of the
code.

> @@ -10969,8 +10969,19 @@ window_text_pixel_size (Lisp_Object window, 
> Lisp_Object from, Lisp_Object to, Li
>        if (IT_CHARPOS (it) == end)
>       {
>         x += it.pixel_width;
> -       it.max_ascent = max (it.max_ascent, it.ascent);
> -       it.max_descent = max (it.max_descent, it.descent);
> +
> +       /* DTRT if no_ascents_descents is t.  */
> +       if (no_ascents_descents)
> +         {
> +           saw_display_prop_at_end_p = true;

This bool value seems to be a misnomer: we don't really have evidence
that we found a display property at end of text.

Also, you should use NILP in the if-clause, as no_ascents_descents is
a Lisp object.

> @@ -10991,8 +11002,14 @@ window_text_pixel_size (Lisp_Object window, 
> Lisp_Object from, Lisp_Object to, Li
>  
>    /* Subtract height of header-line and tab-line which was counted
>       automatically by start_display.  */
> -  y = it.current_y + it.max_ascent + it.max_descent
> -    - WINDOW_TAB_LINE_HEIGHT (w) - WINDOW_HEADER_LINE_HEIGHT (w);
> +  y = (it.current_y + (NILP (no_ascents_descents)
> +                    ? it.max_ascent + it.max_descent
> +                    : 0)
> +       - WINDOW_TAB_LINE_HEIGHT (w) - WINDOW_HEADER_LINE_HEIGHT (w));
> +
> +  if (saw_display_prop_at_end_p)
> +    y += doff;

This is quite confusing: you actually add the same value, but in two
different ways and with 2 different (but subtly equivalent)
conditions.  It would be good to make this less confusing.

> @@ -10866,6 +10867,13 @@ window_text_pixel_size (Lisp_Object window, 
> Lisp_Object from, Lisp_Object to, Li
>           break;
>       }
>      }
> +  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.

The name 'movement' is not the best name.  I would suggest 'offset' or
maybe even 'y_offset' instead.

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.

> +  if (movement > 0)
> +    {
> +      int last_y;
> +      it.current_y = 0;
> +
> +      move_it_by_lines (&it, 0);
         ^^^^^^^^^^^^^^^^
Why is this needed?

> +      while (-it.current_y < movement)
> +     {
> +       last_y = it.current_y;
> +       move_it_vertically_backward (&it, movement + it.current_y);
> +
> +       if (it.current_y == last_y)
> +         break;
> +     }
> +
> +      it.current_y = 0;
> +      start = clip_to_bounds (BEGV, IT_CHARPOS (it), ZV);
> +    }
> +
>    int start_y = it.current_y;
>    /* It makes no sense to measure dimensions of region of text that
>       crosses the point where bidi reordering changes scan direction.

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.

> @@ -11053,26 +11085,32 @@ window_text_pixel_size (Lisp_Object window, 
> Lisp_Object from, Lisp_Object to, Li
>  
>    bidi_unshelve_cache (itdata, false);
>  
> -  return Fcons (make_fixnum (x - start_x), make_fixnum (y));
> +  return (!movement
> +       ? Fcons (make_fixnum (x - start_x), make_fixnum (y))
> +       : list3i (x - start_x, y, start));

Why not use list2i instead of Fcons there?

Do these changes solve the performance problem in
pixel-scroll-precision-mode?

Thanks for working.