bug#64440: 30.0.50; [PATCH] Highlight on non toolkit menu bar items

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 64440@debbugs.gnu.org, Manuel Giraud <manuel@ledu-giraud.fr>
>> Date: Sat, 02 Sep 2023 08:44:16 +0800
>> From:  Po Lu via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> Stefan Kangas <stefankangas@gmail.com> writes:
>> 
>> >> Here is a new set of patches with two more on top of the previous ones
>> >> (ie. Number 1 and 2 should be the same as before).
>> >>
>> >> Number 3 sets the default mouse cursor to be an arrow on the default
>> >> menu bar area.  Number 4 fixes a flickering I had while moving the mouse
>> >> pointer *into* a menu bar entry.
>> >
>> > Po Lu, do you have any comments on this patch series?
>> >
>> > Thanks in advance.
>> 
>> Thanks.  I don't understand why adjustments to note_tab_bar_highlight or
>> note_tool_bar_highlight are warranted, and I think this ought to be
>> optional.
>> 
>> ChangeLog entries are also absent from the commit messages.
>
> I'd appreciate a walkthrough of the patches with explanations for the
> significant hunks.  It's a non-trivial change, so I think the
> rationale and the main ideas of the implementation should be described
> and discussed.

Hi,

For this walkthrough, I have inserted the 4 patches, made some elisions
and comment inline.

>From ceb818090de83fe216af3e9b6bcc198eba9188e3 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Sat, 1 Jul 2023 21:19:06 +0200
Subject: [PATCH 1/4] Possibility to get enter event from menu_bar window

[...]

This first patch is really about being able to get the menu-bar window
from 'window_from_coordinates'.  Without this I would not be able to get
a mouse event that happen on this window.

diff --git a/src/window.c b/src/window.c
index 2a0c62f5d53..af7dcfdd423 100644
--- a/src/window.c
+++ b/src/window.c
@@ -1680,7 +1680,8 @@ check_window_containing (struct window *w, void 
*user_data)
 
 Lisp_Object
 window_from_coordinates (struct frame *f, int x, int y,
-                        enum window_part *part, bool tab_bar_p, bool 
tool_bar_p)
+                        enum window_part *part, bool menu_bar_p,
+                        bool tab_bar_p, bool tool_bar_p)
 {
   Lisp_Object window;
   struct check_window_data cw;
@@ -1693,6 +1694,21 @@ window_from_coordinates (struct frame *f, int x, int y,
   cw.window = &window, cw.x = x, cw.y = y; cw.part = part;
   foreach_window (f, check_window_containing, &cw);
 
+#if defined (HAVE_WINDOW_SYSTEM) && ! defined (HAVE_EXT_MENU_BAR)
+  /* If not found above, see if it's in the menu bar window, if a menu
+     bar exists.  */
+  if (NILP (window)
+      && menu_bar_p
+      && WINDOWP (f->menu_bar_window)
+      && WINDOW_TOTAL_LINES (XWINDOW (f->menu_bar_window)) > 0
+      && (coordinates_in_window (XWINDOW (f->menu_bar_window), x, y)
+         != ON_NOTHING))
+    {
+      *part = ON_TEXT;
+      window = f->menu_bar_window;
+    }
+#endif
+
 #if defined (HAVE_WINDOW_SYSTEM)
   /* If not found above, see if it's in the tab bar window, if a tab
      bar exists.  */

In window_from_coordinates, I tried to mimic what is done for the
tab_bar and the tool_bar.  So I add a menu_bar_p boolean as argument
(before the toolbar one to respect a top to bottom order) and use it to
return the menu_bar window when it is asked and we are in this window.

The rest is this patch is really just adding this new argument in all of
the calls to window_from_coordinates.  I've just copy tool_bar_p and
tab_bar_p values in those call: true when true and false when false.

[...]

>From d5ddf8e04d06730917f94cb7d2fcb026c4437788 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Mon, 3 Jul 2023 17:35:06 +0200
Subject: [PATCH 2/4] Highlight on non toolkit menu bar items

This patch introduce two new functions: get_menu_bar_item and
note_menu_bar_highlight.  I tried to have the same behaviour as
get_tool_bar_item and note_tool_bar_highlight.

AFAIU, get_menu_bar_item job is to determine if we are on a menu entry
and what are its spatial limits.  note_menu_bar_highlight job is to do
something with those limits: for instance, highlight this area.

[...]

+/* Get information about the menu-bar item at position X/Y on frame F.
+   Return menu-bar's item char position in H_START/H_END and pixel
+   position in X_START/X_END.  Value is
+
+   -1  if X/Y is not on a menu-bar item
+   0   if X/Y is on the same item that was highlighted before.
+   1   otherwise.  */
+
+static int
+get_menu_bar_item (struct frame *f, int x, int y, int *h_start, int *h_end,
+                  int *x_start, int *x_end, int *vpos)
+{
+  Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
+  struct window *w = XWINDOW (f->menu_bar_window);
+  struct glyph_row *row;
+  int dummy;
+  Lisp_Object items;
+  int i;
+
+  /* Find glyph's hpos and vpos under X/Y.  */
+  if (x_y_to_hpos_vpos (w, x, y, h_start, vpos, NULL, NULL, &dummy) == NULL)
+    return -1;
+
+  /* Compute h_start and h_end for this menu bar item.  */
+  items = FRAME_MENU_BAR_ITEMS (f);
+  for (i = 0; i < ASIZE (items); i += 4)
+    {
+      Lisp_Object pos, string;
+      string = AREF (items, i + 1);
+      pos = AREF (items, i + 3);
+      if (NILP (string))
+       return -1;
+      if (*h_start >= XFIXNUM (pos)
+         && *h_start < XFIXNUM (pos) + SCHARS (string))
+       {
+         *h_start = XFIXNUM (pos);
+         *h_end = *h_start + SCHARS (string);
+         break;
+       }
+    }

So in this loop above, I'm trying to find what menu bar item I'm on and
get horizontal start and end (in char numbers).

+  /* Convert to pixels bounds.  */
+  row = MATRIX_ROW (w->current_matrix, *vpos);
+  *x_start = 0;
+  for (i = 0; i < *h_start; ++i)
+    *x_start += row->glyphs[TEXT_AREA][i].pixel_width;
+
+  *x_end = *x_start;
+  for (i = *h_start; i < *h_end; ++i)
+    *x_end += row->glyphs[TEXT_AREA][i].pixel_width;

Here, I convert those limits from chars to pixels.

+
+  /* Is mouse on the highlighted item?  */
+  if (EQ (f->menu_bar_window, hlinfo->mouse_face_window)
+      && *vpos >= hlinfo->mouse_face_beg_row
+      && *vpos <= hlinfo->mouse_face_end_row
+      && (*vpos > hlinfo->mouse_face_beg_row
+         || *h_start >= hlinfo->mouse_face_beg_col)
+      && (*vpos < hlinfo->mouse_face_end_row
+         || *h_end < hlinfo->mouse_face_end_col
+         || hlinfo->mouse_face_past_end))
+    return 0;
+
+  return 1;
+}

Finally, I compute the correct return value for get_*_item functions:
              . 0 means we already were on this item
              . 1 means we changed position to a new item
                  
+/* Possibly highlight a menu-bar item on frame F when mouse moves to
+   menu-bar window-relative coordinates X/Y.  Called from
+   note_mouse_highlight.  */
+
+static void
+note_menu_bar_highlight (struct frame *f, int x, int y)
+{
+  Lisp_Object window = f->menu_bar_window;
+  Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
+  int h_start, h_end, vpos, x_start, x_end;
+  int rc;
+
+  /* Function note_mouse_highlight is called with negative X/Y
+     values when mouse moves outside of the frame.  */
+  if (x <= 0 || y <= 0)
+    {
+      clear_mouse_face (hlinfo);
+      return;
+    }
+
+  h_start = h_end = 0;
+  rc = get_menu_bar_item (f, x, y, &h_start, &h_end, &x_start, &x_end, &vpos);
+  if (rc < 0)
+    {
+      /* Not on menu-bar item.  */
+      clear_mouse_face (hlinfo);
+      return;
+    }
+  else if (rc == 0)
+    /* On same menu-bar item as before.  */
+    return;

In note_menu_bar_highlight here, I'm testing (from get_menu_bar_item
result): 
      . if we are not a menu bar item, I clear the mouse face (as done
        on other note_*_highlight functions)
      . if we are on the same item: do nothing
           
+  if (!NILP (Vmouse_highlight))
+    {
+      /* Record this as the current active region.  */
+      hlinfo->mouse_face_beg_col = h_start;
+      hlinfo->mouse_face_beg_row = vpos;
+      hlinfo->mouse_face_beg_x = x_start;
+      hlinfo->mouse_face_past_end = false;
+
+      hlinfo->mouse_face_end_col = h_end;
+      hlinfo->mouse_face_end_row = vpos;
+      hlinfo->mouse_face_end_x = x_end;
+      hlinfo->mouse_face_window = window;
+      hlinfo->mouse_face_face_id = MENU_FACE_ID;
+
+      /* Display it as active.  */
+      show_mouse_face (hlinfo, DRAW_MOUSE_FACE);
+    }
+}

Here, if the user want mouse highlight, I set the highlight info on the
given area and activate it.  I have used MENU_FACE_ID so there should be
no user visible changes here but later we might introduce a new face for
this (MENU_HIGHLIGHT_FACE_ID for instance).  I also used DRAW_MOUSE_FACE
in the show_mouse_face call to have the mouse cursor changed to the
« little hand »: so far it is the only visible feedback that I have.

 
 /***********************************************************************
@@ -35223,6 +35339,16 @@ note_mouse_highlight (struct frame *f, int x, int y)
   w = XWINDOW (window);
   frame_to_window_pixel_xy (w, &x, &y);
 
+#if defined (HAVE_WINDOW_SYSTEM) && ! defined (HAVE_EXT_MENU_BAR)
+  /* Handle menu-bar window differently since it doesn't display a
+     buffer.  */
+  if (EQ (window, f->menu_bar_window))
+    {
+      note_menu_bar_highlight (f, x, y);
+      return;
+    }
+#endif
+

Here, I'm just using note_menu_bar_highlight in the toplevel
note_mouse_highlight.

>From 08279c0754c49b567fda0adedc3ecfd4e11a7ec5 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Tue, 4 Jul 2023 17:48:42 +0200
Subject: [PATCH 3/4] nontext cursor on the menu bar by default

* src/xdisp.c (show_mouse_face): Arrow for DRAW_NORMAL_TEXT on the
menu bar
---
 src/xdisp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/xdisp.c b/src/xdisp.c
index 25b33e3a8c4..e1c4c9ee7b9 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -33878,6 +33878,9 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum 
draw_glyphs_face draw)
   if (FRAME_WINDOW_P (f) && NILP (track_mouse))
     {
       if (draw == DRAW_NORMAL_TEXT
+#ifndef HAVE_EXT_MENU_BAR
+         && !EQ (hlinfo->mouse_face_window, f->menu_bar_window)
+#endif
 #ifndef HAVE_EXT_TOOL_BAR
          && !EQ (hlinfo->mouse_face_window, f->tool_bar_window)
 #endif

Here, I thought I was toggling the mouse cursor to an arrow when
entering the menu bar but it does not seem to work.  It does not work
for tool bar either BTW.

>From 159ae74ed2ec452902913690f6d462767aa3a96d Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Tue, 4 Jul 2023 17:57:39 +0200
Subject: [PATCH 4/4] Avoid mouse cursor flicker

This last patch works for me but is a kludge and, as Po said, I
shouldn't be touching note_tool_bar_highlight and note_tab_bar_highlight
here.  I should rework this.

It fixes the following issue: when I'm over the same menu item and
moving, the mouse cursor flickers.

* src/dispextern.h (Mouse_HLInfo): Introduce a mouse_cursor_update
set to true by default.
* src/xdisp.c (show_mouse_face): Take it into account.
(note_menu_bar_highlight, note_tab_bar_highlight)
(note_tool_bar_highlight): Don't update the mouse cursor from
here.
---
 src/dispextern.h |  5 +++++
 src/xdisp.c      | 13 ++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index ece128949f5..ed03a7c244e 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -2876,6 +2876,10 @@ #define PRODUCE_GLYPHS(IT)                              \
 
   /* True means that the mouse highlight should not be shown.  */
   bool_bf mouse_face_hidden : 1;
+
+  /* True means that the mouse highlight should update the mouse
+     cursor.  */
+  bool_bf mouse_cursor_update : 1;
 } Mouse_HLInfo;
 
 INLINE void
@@ -2892,6 +2896,7 @@ reset_mouse_highlight (Mouse_HLInfo *hlinfo)
   hlinfo->mouse_face_past_end = false;
   hlinfo->mouse_face_hidden = false;
   hlinfo->mouse_face_defer = false;
+  hlinfo->mouse_cursor_update = true;
 }
 
 /***********************************************************************
diff --git a/src/xdisp.c b/src/xdisp.c
index e1c4c9ee7b9..86bb1be9240 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -13956,6 +13956,11 @@ note_menu_bar_highlight (struct frame *f, int x, int y)
     /* On same menu-bar item as before.  */
     return;
 
+  /* Clear mouse face but the mouse cursor.  */
+  hlinfo->mouse_cursor_update = false;
+  clear_mouse_face (hlinfo);
+  hlinfo->mouse_cursor_update = true;
+
   if (!NILP (Vmouse_highlight))
     {
       /* Record this as the current active region.  */
@@ -14838,7 +14843,10 @@ note_tab_bar_highlight (struct frame *f, int x, int y)
     /* On same tab-bar item as before.  */
     goto set_help_echo;
 
+  /* Clear mouse face but the mouse cursor.  */
+  hlinfo->mouse_cursor_update = false;
   clear_mouse_face (hlinfo);
+  hlinfo->mouse_cursor_update = true;
 
   bool mouse_down_p = false;
   /* Mouse is down, but on different tab-bar item?  Or alternatively,
@@ -15793,7 +15801,10 @@ note_tool_bar_highlight (struct frame *f, int x, int y)
     /* On same tool-bar item as before.  */
     goto set_help_echo;
 
+  /* Clear mouse face but the mouse cursor.  */
+  hlinfo->mouse_cursor_update = false;
   clear_mouse_face (hlinfo);
+  hlinfo->mouse_cursor_update = true;
 
   /* Mouse is down, but on different tool-bar item?  */
   mouse_down_p = (gui_mouse_grabbed (dpyinfo)
@@ -33875,7 +33886,7 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum 
draw_glyphs_face draw)
 
 #ifdef HAVE_WINDOW_SYSTEM
   /* Change the mouse cursor.  */
-  if (FRAME_WINDOW_P (f) && NILP (track_mouse))
+  if (FRAME_WINDOW_P (f) && NILP (track_mouse) && hlinfo->mouse_cursor_update)
     {
       if (draw == DRAW_NORMAL_TEXT
 #ifndef HAVE_EXT_MENU_BAR
-- 
2.40.0

-- 
Manuel Giraud