Re: move_it_vertically_backward question

[Eli Zaretskii <eliz@gnu.org>]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Thu, 23 Dec 2021 09:30:34 +0800
> 
> > Also, here's a patch with some of the changes made.  I tried with the
> > documentation as well, but I'm pretty bad with that stuff.
> 
> I tried updating the documentation again to better fit what's actually
> done (including documenting that measurements always begin at the
> beginning of the visual line).
> 
> Please take a look and see if it's OK to install this now, thanks.

Does the code DTRT in your use cases, and is it OK performance-wise to
consider it stable enough for master?

> * src/xdisp.c (window_text_pixel_size): Understand a special
> format of `from' that specifies the amount of pixels above a
> position.                                            ^^^^^

"above or below".  Or maybe just "at a given pixel offset from".

> diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
> index 98a15404f9..7e6ee59824 100644
> --- a/doc/lispref/display.texi
> +++ b/doc/lispref/display.texi
> @@ -2090,19 +2090,23 @@ Size of Displayed Text
>  This function returns the size of the text of @var{window}'s buffer in
>  pixels.  @var{window} must be a live window and defaults to the
>  selected one.  The return value is a cons of the maximum pixel-width
> -of any text line and the maximum pixel-height of all text lines.  This
> -function exists to allow Lisp programs to adjust the dimensions of
> -@var{window} to the buffer text it needs to display.
> +of any text line and the maximum pixel-height of all text lines, or if
> +@var{from} is a cons, a list of the pixel-width, pixel-height, and the
> +buffer position of the first line that was measured.

This sentence now becomes too long and thus confusing.  I suggest to
leave the original sentence alone, and add a new sentence, something
like

  The return value can also optionally (see below) include the buffer
  position of the first line whose dimensions were measured.

>                                                          This function
> +exists to allow Lisp programs to adjust the dimensions of @var{window}
> +to the buffer text it needs to display.

I would add "and for other similar situations" to this sentence.

> +character.  If @var{from} is a cons, the cdr specifies a position, and
> +the car specifies the minimum amount of pixels above or below the
> +beginning of the visual line at that position to start measuring from.

First, I think you swapped the car and cdr.

Second, I would suggest to simplify/clarify:

  ... its @code{car} specifies a buffer position, and its @code{car}
  specifies the vertical offset in pixels from that position to the
  first screen line whose text is to be measured.  (The measurement
  will start from the visual beginning of that screen line.)

And finally, please add here the detailed description of the return
value, both when FROM is a position and when it's a cons cell.  Since
the forms of FROM are already described, such a description of the
return value here will be less confusing.

> ++++
> +** 'window-text-pixel-size' can now measure from a set amount of pixels above a position.

This is too long for a NEWS heading.  Can you make it shorter, leaving
the details for the body?  I would also add more explanations, because
this sounds very vague.

> +  else if (CONSP (from))
> +    {
> +      start = clip_to_bounds (BEGV, fix_position (XCAR (from)), ZV);
> +      bpos = CHAR_TO_BYTE (start);
> +      CHECK_FIXNUM (XCDR (from));
> +      vertical_offset = XFIXNUM (XCDR (from));

In principle, the offset could be a bignum or even a float, so the
restriction here seems to be too stringent, no?  A float in particular
could be useful, I think, in case the caller calculated the pixels as
a float.

> +  if (vertical_offset != 0)
> +    {
> +      int last_y;
> +      it.current_y = 0;
> +
> +      move_it_by_lines (&it, 0);
> +
> +      if (vertical_offset < 0)
> +	{
> +	  while (it.current_y > vertical_offset)
> +	    {
> +	      last_y = it.current_y;
> +	      move_it_vertically_backward (&it,
> +					   (abs (vertical_offset)
> +					    + it.current_y));
> +
> +	      if (it.current_y == last_y)
> +		break;
> +	    }
> +	}
> +      else
> +	{
> +	  move_it_vertically (&it, vertical_offset);
> +	}
> +

Can you add a comment explaining why in the negative case you call
move_it_vertically_backward in a loop, while in the positive case you
call move_it_vertically just once?

>  DEFUN ("window-text-pixel-size", Fwindow_text_pixel_size, Swindow_text_pixel_size, 0, 7, 0,
> -       doc: /* Return the size of the text of WINDOW's buffer in pixels.
> -WINDOW must be a live window and defaults to the selected one.  The
> -return value is a cons of the maximum pixel-width of any text line
> -and the pixel-height of all the text lines in the accessible portion
> -of buffer text.
> +       doc: /* Return the size of the text of WINDOW's buffer in
> +pixels.  WINDOW must be a live window and defaults to the selected

The first line must be a complete sentence, so your reformatting of it
is a step backward.

> +one.  The return value is a cons of the maximum pixel-width of any
> +text line and the pixel-height of all the text lines in the accessible
> +portion of buffer text or a list of the maximum pixel-width,
> +pixel-height, and the buffer position of the line at FROM.

Here, too, I would suggest to first say what is the form of the return
value in the "usual" case, and then add a separate sentence like

  If FROM is a cons cell, the return value includes, in addition to
  the dimensions, also a third element that provides the buffer
  position from which measuring of the text dimensions was actually
  started.

> +of the buffer.  If FROM is a cons, the cdr specifies the amount of
> +pixels above or below a buffer position to begin measuring text, and
> +the car specifies the buffer position.

Consider rewording like I suggested for the manual.

>                                         In that case, a list is
> +returned.

This was already said above, so it's redundant.

Thanks.