On Thu, Feb 27, 2025 at 3:15 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > Subject: [PATCH] Handle mouse-face properties on tab-bar tabs (bug#76394)
>
> Thanks, I have a few comments:
>
> > * src/xdisp.c (note_tab_bar_highlight):
> > Handle mouse-face properties, if found.  Realize the face if necessary.
>
> Please reformat this log entry according to our conventions (see
> examples in Git).
>

I tried.  I hope the revised log is acceptable.

Modeled after Juri's 24c4bc1:

* src/xdisp.c (note_tab_bar_highlight): Remove fallback for
help_echo_string.
Don't use TAB_BAR_ITEM_CAPTION as the default value
when TAB_BAR_ITEM_HELP is not specified (bug#73050).

And Eli's 1136963:

; * src/xdisp.c (maybe_produce_line_number): Fix last change (bug#76362).

> +      if (STRINGP (string))
> > +        {
> > +          /* Compute starting column of the tab-bar-item to adjust col
> > +             of the mouse face relative to row_start_glyph.
> > +
> > +             tab_bar_item_info does not contain the absolute starting
> > +             offset of the item.  We compute it by looking backwards
> > +             until we find a glyph that belongs to a previous tab bar
> > +             item, or if this is the first item. */
>
> Please indent with TABs and SPCs, not just with SPCs, per our
> conventions for C source files.  (This is supposed to happen
> automatically due to our .dir-locals.el, unless you disable that for
> some reason.)
>

I had a misconfiguration of tree sitter for C/C++ that borked the mode to
not match .dir-locals.  I fixed that.

> +                break; /* Just before the beginning of this item. */
>                                                                    ^^
> Please leave two spaces at the end of a comment (here and elsewhere in
> the patch).
>

Done.


> > +          mouse_face = Fget_text_property (make_fixnum (hpos_caption),
> > +                                           Qmouse_face, string);
> > +          if (!NILP (mouse_face))
> > +            {
> > +              mouse_face_id = lookup_named_face (NULL, f, mouse_face,
> false);
>                                                     ^^^^
> Why NULL and not 'w'?
>

I wasn't sure remaps were valid in tab_bar.  I put w in now, just in case
they are.

> +       if ( EQ (window, hlinfo->mouse_face_window)
> > +            && (!row->reversed_p
>
> Can a glyph row that corresponds to a tab bar be reversed?  IOW, can
> the tab bar be ever displayed right-to-left?  I don't think so, but if
> I'm wrong, can you describe a scenario where it can happen?
>

There's a comment in display_tab_bar:

  /* FIXME: This should be controlled by a user option.  See the
     comments in redisplay_tool_bar and display_mode_line about
     this.  */
  it.paragraph_embedding = L2R;

I figured let's be defensive for when this is implemented.

Also added tab-bar-tab-highlight face in lisp/tab-bar.el.

-Stephane