Re: [PATCH] User-defined fringe tooltips (a request for review)

[Eli Zaretskii <eliz@gnu.org>]

> From: Vladimir Kazanov <vekazanov@gmail.com>
> Date: Sun, 24 Dec 2023 11:31:28 +0000
> Cc: emacs-devel@gnu.org
> 
> After figuring out a couple of nasty crashes I came up with a patch
> which works for all display specs coming from buffer text itself.
> 
> But I've just realized that some fringe sources are still unhandled
> here. For example, when the fringe indicator is defined in non-buffer
> strings, like overlays with 'before-string/'after-string properties.
> Just iterating through text positions doesn't scan any of that. This
> means that we'd have to imitate more of the iterator logic when
> looking for display specs containing help-echo.
> 
> Do we want to manage fringe help-echo text coming from various strings
> as well? Certainly doable. I wonder if this asks for a separate patch.

There's no catastrophe if we only support some sources of this display
property; we already do that in other similar cases.  We can later add
more sources if there's demand.

I have some comments on the code and the documentation:

> --- a/doc/lispref/display.texi
> +++ b/doc/lispref/display.texi
> @@ -5492,14 +5492,15 @@ Other Display Specs
>  but it is done as a special case of marginal display (@pxref{Display
>  Margins}).
> 
> -@item (left-fringe @var{bitmap} @r{[}@var{face}@r{]})
> -@itemx (right-fringe @var{bitmap} @r{[}@var{face}@r{]})
> +@item (left-fringe @var{bitmap} @r{[}@var{face}@r{]} @r{[}@var{help-echo}@r{]})
> +@itemx (right-fringe @var{bitmap} @r{[}@var{face}@r{]} @r{[}@var{help-echo}@r{]})
>  This display specification on any character of a line of text causes
>  the specified @var{bitmap} be displayed in the left or right fringes
>  for that line, instead of the characters that have the display
>  specification.  The optional @var{face} specifies the face whose
> -colors are to be used for the bitmap display.  @xref{Fringe Bitmaps},
> -for the details.
> +colors are to be used for the bitmap display.  The optional
> +@var{help-echo} string can be used to display tooltips or help text in
> +the echo area.  @xref{Fringe Bitmaps}, for the details.

The @xref sentence should be after the sentence which ends with
"bitmap display", since it describes more details about that, and has
nothing to do with the new help-echo string feature.

> ++++
> +** Tooltips for user fringe indicators.
> +User fringe indicators defined in text display specifications now
> +support user-defined tooltips. See the "Other Display Specifications"
                                ^^
Our conventions are to leave two spaces between sentences.

> +  /* Translate windows coordinates into a vertical window position. */
> +  int hpos, vpos, area;
> +  struct window *w = XWINDOW (window);
> +  x_y_to_hpos_vpos (w, x, y, &hpos, &vpos, 0, 0, &area);

This was already done by the caller, so no need to do it again;
instead, pass the info as arguments to this function.

> +  /* Get to the current glyph row. */
> +  struct glyph_row *row = MATRIX_ROW (w->current_matrix, vpos);
> +
> +  /* No fringe indicators - no need to look things up  */

Our style for comments in C is to have them as complete sentences
ending in a period, and leave two spaces between the last sentence and
the closing "*/" comment delimiter.

> +  /* Check display specs for both visual and invisible text in the
> +     row. */
> +  ptrdiff_t charpos;
> +  ptrdiff_t charpos_start = row->minpos.charpos;
> +  ptrdiff_t charpos_end = row->maxpos.charpos;
> +  for (charpos = charpos_start; charpos <= charpos_end; charpos++)
> +    {
> +      /* Properties in question can be either in text props or
> +         overlays, so check both. */
> +      Lisp_Object spec = get_char_property_and_overlay (make_fixnum (charpos),
> +							Qdisplay, Qnil, NULL);

To avoid punishing display of text that has no such properties, I
would first see if any Qdisplay properties are within this glyph row,
using Fnext_char_property_change, and only enter the loop if that call
returns a position in-range.

> +      /* Hovering over the right fringe - check the right-fringe
> +         spec  */
> +      /* Hovering over the left fringe - check the left-fringe
> +         spec  */
> +      if (!(EQ (XCAR (spec), Qleft_fringe) && part == ON_LEFT_FRINGE) &&
> +	  !(EQ (XCAR (spec), Qright_fringe) && part == ON_RIGHT_FRINGE))

Please place each comment before the line of code on which it
comments.

> +	continue;

We prefer not to over-use 'continue' in loops; instead, reverse the
condition in 'if' and do everything inside the 'if' block.

> +      /* Get the caption while ingoring all non-strings */
> +      Lisp_Object fringe_help_echo = CAR_SAFE (CDR_SAFE (CDR_SAFE (CDR_SAFE (spec))));
> +      if (!STRINGP (fringe_help_echo))
> +	continue;
> +
> +      help_echo_string = fringe_help_echo;

Here, you only support the simplest form of the display property's
value, i.e.

  (left-fringe BITMAP [FACE] HELP-STRING)

But the property's value can have a more complex form:

  . it could be a vector or a list of several display specifications
  . a single specification could have the form (when CONDITION . SPEC)

So I think you need something more complex here; see the loop in
handle_display_prop for the details.

> +  else if (part == ON_LEFT_FRINGE || part == ON_RIGHT_FRINGE) {

This is not our style of placing braces.  We use this style:

   else if (CONDITION)
     {
       ...
     }

Thanks.