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

[Vladimir Kazanov <vekazanov@gmail.com>]

Hi Eli,

Thank you for looking into this and sorry for the delay - life
happened. Anyways, I have some time now to finish the patch.

I've integrated all of the suggestions but the last one, related to
many forms of display specs.

The code in the (note_fringe_highlight) function works outside of the
main (handle_single_display_spec) call. So to get to the :help-echo
string it has to parse into the spec, or a list of specs, or a vector
of specs, effectively duplicating parsing logic in
handle_single_display_spec. This is not nice but doable.

But every display spec can be wrapped in a (when CONDITION . SPEC)
form. And because (handle_single_display_spec) happens at a time
different from the (note_fringe_highlight) call time we can never be
sure that the condition still holds even if the code reevaluates
CONDITION.

One approach would be to just go into SPEC of the condition without
any reevaluations and get the :help-echo string anyway, hoping that
there are no discrepancies. This would work, hopefully, most of the
time but not all the time.

What do you think?

Thank you

On Sun, 24 Dec 2023 at 16:54, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > 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.



-- 
Regards,

Vladimir Kazanov