bug#58406: 29.0.50; Bars refactoring?

bug#58406: 29.0.50; Bars refactoring?

[Manuel Giraud]

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]


Hi,

I'm trying to have some kind of highlight feature on the no-toolkit menu
bar.  As this feature is already present in tool/tab bar, I'd like to
mimic those.

While trying this, I think that I found that some "bar" functions could
be factorized (up to a certain point I guess).  Here is a patch that
shows my direction.

Do you think that this approach could make it into master or would it be
frown upon?  And if so, what would be a better way?

Best regards,

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-start-bars-refactoring.patch --]
[-- Type: text/x-patch, Size: 13127 bytes --]

From cc5a4e5702c4eeb427d91776204b844ceda61a32 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Sun, 9 Oct 2022 13:27:31 +0200
Subject: [PATCH] start bars refactoring

---
 src/frame.h |   7 ++
 src/xdisp.c | 280 ++++++++++++++++++++++++----------------------------
 2 files changed, 137 insertions(+), 150 deletions(-)

diff --git a/src/frame.h b/src/frame.h
index 458b6257e4..4c78f0a555 100644
--- a/src/frame.h
+++ b/src/frame.h
@@ -76,6 +76,13 @@ #define EMACS_FRAME_H
 #endif
 #endif /* HAVE_WINDOW_SYSTEM */
 
+enum bar_type
+  {
+    MENU_BAR,
+    TAB_BAR,
+    TOOL_BAR
+  };
+
 /* The structure representing a frame.  */
 
 struct frame
diff --git a/src/xdisp.c b/src/xdisp.c
index 9534e27843..a822b0fe1c 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -13524,6 +13524,110 @@ gui_consider_frame_title (Lisp_Object frame)
 
 #endif /* not HAVE_WINDOW_SYSTEM */
 
+\f
+/***********************************************************************
+			      Generics for Bars
+ ***********************************************************************/
+
+/* Get information about the bar item which is displayed in GLYPH on
+   frame F.  Return in *PROP_IDX the index where bar item properties
+   start in the bar items.  For TAB_BAR, return in CLOSE_P an
+   indication whether the click was on the close-tab icon of the tab.
+   Value is false if GLYPH doesn't display a bar item.  */
+
+static bool
+bar_item_info (ENUM_BF(bar_type) bar, struct frame *f, struct glyph *glyph,
+	       int *prop_idx, bool *close_p)
+{
+  Lisp_Object prop, string;
+  ptrdiff_t charpos;
+
+  if (bar == TOOL_BAR)
+    string = f->current_tool_bar_string;
+  else if (bar == TAB_BAR)
+    string = f->current_tab_bar_string;
+  else
+    return false;
+
+  /* This function can be called asynchronously, which means we must
+     exclude any possibility that Fget_text_property signals an
+     error.  */
+  charpos = min (SCHARS (string), glyph->charpos);
+  charpos = max (0, charpos);
+
+  /* Get the text property `menu-item' at pos. The value of that
+     property is the start index of this item's properties in
+     F->tool_bar_items.  */
+  prop = Fget_text_property (make_fixnum (charpos), Qmenu_item, string);
+  if (! FIXNUMP (prop))
+    return false;
+  *prop_idx = XFIXNUM (prop);
+
+  if (bar == TAB_BAR)
+    *close_p = !NILP (Fget_text_property (make_fixnum (charpos),
+					  Qclose_tab,
+					  string));
+
+  return true;
+}
+
+/* Get information about the bar item at position X/Y on frame F.
+   Return in *GLYPH a pointer to the glyph of the bar item in the
+   current matrix of the bar window of F, or NULL if not on a bar
+   item.  Return in *PROP_IDX the index of the bar item in bar items.
+   Value is
+
+   -1	if X/Y is not on a bar item
+   0	if X/Y is on the same item that was highlighted before.
+   1	otherwise.  */
+
+static int
+get_bar_item (ENUM_BF(bar_type) bar, struct frame *f, int x, int y,
+	      struct glyph **glyph, int *hpos, int *vpos, int *prop_idx,
+	      bool *close_p)
+{
+  Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
+  Lisp_Object window;
+  struct window *w;
+  int area;
+
+  if (bar == TOOL_BAR)
+    window = f->tool_bar_window;
+  else if (bar == MENU_BAR)
+    window = f->menu_bar_window;
+  else if (bar == TAB_BAR)
+    window = f->tab_bar_window;
+  else
+    return -1;
+
+  w = XWINDOW (window);
+
+  /* Find the glyph under X/Y.  */
+  *glyph = x_y_to_hpos_vpos (w, x, y, hpos, vpos, 0, 0, &area);
+  if (*glyph == NULL)
+    return -1;
+
+  /* Get the start of this tool-bar item's properties in
+     f->tool_bar_items.  */
+  if (!bar_item_info (bar, f, *glyph, prop_idx, close_p))
+    return -1;
+
+  /* Is mouse on the highlighted item?  */
+  if (bar == TAB_BAR)
+    return *prop_idx == f->last_tab_bar_item ? 0 : 1;
+  else if (EQ (window, hlinfo->mouse_face_window)
+      && *vpos >= hlinfo->mouse_face_beg_row
+      && *vpos <= hlinfo->mouse_face_end_row
+      && (*vpos > hlinfo->mouse_face_beg_row
+	  || *hpos >= hlinfo->mouse_face_beg_col)
+      && (*vpos < hlinfo->mouse_face_end_row
+	  || *hpos < hlinfo->mouse_face_end_col
+	  || hlinfo->mouse_face_past_end))
+    return 0;
+
+  return 1;
+}
+
 \f
 /***********************************************************************
 			      Menu Bars
@@ -14471,73 +14575,6 @@ redisplay_tab_bar (struct frame *f)
   return false;
 }
 
-/* Get information about the tab-bar item which is displayed in GLYPH
-   on frame F.  Return in *PROP_IDX the index where tab-bar item
-   properties start in F->tab_bar_items.  Return in CLOSE_P an
-   indication whether the click was on the close-tab icon of the tab.
-   Value is false if GLYPH doesn't display a tab-bar item.  */
-
-static bool
-tab_bar_item_info (struct frame *f, struct glyph *glyph,
-		   int *prop_idx, bool *close_p)
-{
-  Lisp_Object prop;
-  ptrdiff_t charpos;
-
-  /* This function can be called asynchronously, which means we must
-     exclude any possibility that Fget_text_property signals an
-     error.  */
-  charpos = min (SCHARS (f->current_tab_bar_string), glyph->charpos);
-  charpos = max (0, charpos);
-
-  /* Get the text property `menu-item' at pos. The value of that
-     property is the start index of this item's properties in
-     F->tab_bar_items.  */
-  prop = Fget_text_property (make_fixnum (charpos),
-			     Qmenu_item, f->current_tab_bar_string);
-  if (! FIXNUMP (prop))
-    return false;
-  *prop_idx = XFIXNUM (prop);
-
-  *close_p = !NILP (Fget_text_property (make_fixnum (charpos),
-                                        Qclose_tab,
-                                        f->current_tab_bar_string));
-
-  return true;
-}
-
-\f
-/* Get information about the tab-bar item at position X/Y on frame F.
-   Return in *GLYPH a pointer to the glyph of the tab-bar item in
-   the current matrix of the tab-bar window of F, or NULL if not
-   on a tab-bar item.  Return in *PROP_IDX the index of the tab-bar
-   item in F->tab_bar_items.  Value is
-
-   -1	if X/Y is not on a tab-bar item
-   0	if X/Y is on the same item that was highlighted before.
-   1	otherwise.  */
-
-static int
-get_tab_bar_item (struct frame *f, int x, int y, struct glyph **glyph,
-		   int *hpos, int *vpos, int *prop_idx, bool *close_p)
-{
-  struct window *w = XWINDOW (f->tab_bar_window);
-  int area;
-
-  /* Find the glyph under X/Y.  */
-  *glyph = x_y_to_hpos_vpos (w, x, y, hpos, vpos, 0, 0, &area);
-  if (*glyph == NULL)
-    return -1;
-
-  /* Get the start of this tab-bar item's properties in
-     f->tab_bar_items.  */
-  if (!tab_bar_item_info (f, *glyph, prop_idx, close_p))
-    return -1;
-
-  return *prop_idx == f->last_tab_bar_item ? 0 : 1;
-}
-
-
 /* EXPORT:
    Handle mouse button event on the tab-bar of frame F, at
    frame-relative coordinates X/Y.  DOWN_P is true for a button press,
@@ -14557,7 +14594,7 @@ handle_tab_bar_click (struct frame *f, int x, int y, bool down_p,
   int ts;
 
   frame_to_window_pixel_xy (w, &x, &y);
-  ts = get_tab_bar_item (f, x, y, &glyph, &hpos, &vpos, &prop_idx, &close_p);
+  ts = get_bar_item (TAB_BAR, f, x, y, &glyph, &hpos, &vpos, &prop_idx, &close_p);
   if (ts == -1)
     return Fcons (Qtab_bar, Qnil);
 
@@ -14624,7 +14661,7 @@ note_tab_bar_highlight (struct frame *f, int x, int y)
       return;
     }
 
-  rc = get_tab_bar_item (f, x, y, &glyph, &hpos, &vpos, &prop_idx, &close_p);
+  rc = get_bar_item (TAB_BAR, f, x, y, &glyph, &hpos, &vpos, &prop_idx, &close_p);
   if (rc < 0)
     {
       /* Not on tab-bar item.  */
@@ -15400,77 +15437,6 @@ redisplay_tool_bar (struct frame *f)
   return false;
 }
 
-/* Get information about the tool-bar item which is displayed in GLYPH
-   on frame F.  Return in *PROP_IDX the index where tool-bar item
-   properties start in F->tool_bar_items.  Value is false if
-   GLYPH doesn't display a tool-bar item.  */
-
-static bool
-tool_bar_item_info (struct frame *f, struct glyph *glyph, int *prop_idx)
-{
-  Lisp_Object prop;
-  ptrdiff_t charpos;
-
-  /* This function can be called asynchronously, which means we must
-     exclude any possibility that Fget_text_property signals an
-     error.  */
-  charpos = min (SCHARS (f->current_tool_bar_string), glyph->charpos);
-  charpos = max (0, charpos);
-
-  /* Get the text property `menu-item' at pos. The value of that
-     property is the start index of this item's properties in
-     F->tool_bar_items.  */
-  prop = Fget_text_property (make_fixnum (charpos),
-			     Qmenu_item, f->current_tool_bar_string);
-  if (! FIXNUMP (prop))
-    return false;
-  *prop_idx = XFIXNUM (prop);
-  return true;
-}
-
-\f
-/* Get information about the tool-bar item at position X/Y on frame F.
-   Return in *GLYPH a pointer to the glyph of the tool-bar item in
-   the current matrix of the tool-bar window of F, or NULL if not
-   on a tool-bar item.  Return in *PROP_IDX the index of the tool-bar
-   item in F->tool_bar_items.  Value is
-
-   -1	if X/Y is not on a tool-bar item
-   0	if X/Y is on the same item that was highlighted before.
-   1	otherwise.  */
-
-static int
-get_tool_bar_item (struct frame *f, int x, int y, struct glyph **glyph,
-		   int *hpos, int *vpos, int *prop_idx)
-{
-  Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
-  struct window *w = XWINDOW (f->tool_bar_window);
-  int area;
-
-  /* Find the glyph under X/Y.  */
-  *glyph = x_y_to_hpos_vpos (w, x, y, hpos, vpos, 0, 0, &area);
-  if (*glyph == NULL)
-    return -1;
-
-  /* Get the start of this tool-bar item's properties in
-     f->tool_bar_items.  */
-  if (!tool_bar_item_info (f, *glyph, prop_idx))
-    return -1;
-
-  /* Is mouse on the highlighted item?  */
-  if (EQ (f->tool_bar_window, hlinfo->mouse_face_window)
-      && *vpos >= hlinfo->mouse_face_beg_row
-      && *vpos <= hlinfo->mouse_face_end_row
-      && (*vpos > hlinfo->mouse_face_beg_row
-	  || *hpos >= hlinfo->mouse_face_beg_col)
-      && (*vpos < hlinfo->mouse_face_end_row
-	  || *hpos < hlinfo->mouse_face_end_col
-	  || hlinfo->mouse_face_past_end))
-    return 0;
-
-  return 1;
-}
-
 
 /* EXPORT:
    Handle mouse button event on the tool-bar of frame F, at
@@ -15485,6 +15451,7 @@ handle_tool_bar_click_with_device (struct frame *f, int x, int y, bool down_p,
   Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
   struct window *w = XWINDOW (f->tool_bar_window);
   int hpos, vpos, prop_idx;
+  bool close_p;
   struct glyph *glyph;
   Lisp_Object enabled_p;
   int ts;
@@ -15497,7 +15464,7 @@ handle_tool_bar_click_with_device (struct frame *f, int x, int y, bool down_p,
      highlight, since tool-bar items are not highlighted in that
      case.  */
   frame_to_window_pixel_xy (w, &x, &y);
-  ts = get_tool_bar_item (f, x, y, &glyph, &hpos, &vpos, &prop_idx);
+  ts = get_bar_item (TOOL_BAR, f, x, y, &glyph, &hpos, &vpos, &prop_idx, &close_p);
   if (ts == -1
       || (ts != 0 && !NILP (Vmouse_highlight)))
     return;
@@ -15555,11 +15522,10 @@ handle_tool_bar_click (struct frame *f, int x, int y, bool down_p,
    note_mouse_highlight.  */
 
 static void
-note_tool_bar_highlight (struct frame *f, int x, int y)
+note_bar_highlight (ENUM_BF(bar_type) bar, struct frame *f, int x, int y)
 {
-  Lisp_Object window = f->tool_bar_window;
-  struct window *w = XWINDOW (window);
-  Display_Info *dpyinfo = FRAME_DISPLAY_INFO (f);
+  Lisp_Object window;
+  struct window *w;
   Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
   int hpos, vpos;
   struct glyph *glyph;
@@ -15567,10 +15533,22 @@ note_tool_bar_highlight (struct frame *f, int x, int y)
   int i;
   Lisp_Object enabled_p;
   int prop_idx;
+  bool close_p;
   enum draw_glyphs_face draw = DRAW_IMAGE_RAISED;
-  bool mouse_down_p;
   int rc;
 
+  /* Select bar window.  */
+  if (bar == TOOL_BAR)
+    window = f->tool_bar_window;
+  else if (bar == MENU_BAR)
+    window = f->menu_bar_window;
+  else if (bar == TAB_BAR)
+    window = f->tab_bar_window;
+  else
+    return;
+
+  w = XWINDOW (window);
+
   /* Function note_mouse_highlight is called with negative X/Y
      values when mouse moves outside of the frame.  */
   if (x <= 0 || y <= 0)
@@ -15579,20 +15557,22 @@ note_tool_bar_highlight (struct frame *f, int x, int y)
       return;
     }
 
-  rc = get_tool_bar_item (f, x, y, &glyph, &hpos, &vpos, &prop_idx);
+  rc = get_bar_item (bar, f, x, y, &glyph, &hpos, &vpos, &prop_idx, &close_p);
   if (rc < 0)
     {
-      /* Not on tool-bar item.  */
+      /* Not on bar item.  */
       clear_mouse_face (hlinfo);
       return;
     }
   else if (rc == 0)
-    /* On same tool-bar item as before.  */
+    /* On same bar item as before.  */
     goto set_help_echo;
 
   clear_mouse_face (hlinfo);
 
+  bool mouse_down_p = false;
   /* Mouse is down, but on different tool-bar item?  */
+  Display_Info *dpyinfo = FRAME_DISPLAY_INFO (f);
   mouse_down_p = (gui_mouse_grabbed (dpyinfo)
 		  && f == dpyinfo->last_mouse_frame);
 
@@ -35049,7 +35029,7 @@ note_mouse_highlight (struct frame *f, int x, int y)
      buffer.  */
   if (EQ (window, f->tool_bar_window))
     {
-      note_tool_bar_highlight (f, x, y);
+      note_bar_highlight (TOOL_BAR, f, x, y);
       return;
     }
 #endif
-- 
2.37.3


[-- Attachment #3: Type: text/plain, Size: 7235 bytes --]



In GNU Emacs 29.0.50 (build 1, x86_64-unknown-openbsd7.2, cairo version
 1.17.6) of 2022-10-09 built on elite.giraud
Repository revision: 300a6ab95ca970899b3fef8852af06d4548d0395
Repository branch: mgi/menubar
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: OpenBSD elite.giraud 7.2 GENERIC.MP#739 amd64

Configured using:
 'configure --prefix=/home/manuel/emacs --bindir=/home/manuel/bin
 --with-x-toolkit=no --without-sound --without-compress-install
 CPPFLAGS=-I/usr/local/include LDFLAGS=-L/usr/local/lib'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LCMS2 LIBOTF LIBXML2 MODULES NOTIFY KQUEUE OLDXMENU PDUMPER PNG RSVG
SQLITE3 THREADS TIFF WEBP X11 XDBE XIM XINPUT2 XPM ZLIB

Important settings:
  value of $LC_ALL: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Dired by name

Minor modes in effect:
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  gnus-dired-mode: t
  icomplete-mode: t
  display-time-mode: t
  display-battery-mode: t
  shell-dirtrack-mode: t
  global-so-long-mode: t
  repeat-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  buffer-read-only: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/manuel/.emacs.d/elpa/transient-20220918.2101/transient hides /home/manuel/emacs/share/emacs/29.0.50/lisp/transient

Features:
(shadow sort mail-extr emacsbug whitespace magit-patch magit-extras
misearch multi-isearch bug-reference face-remap magit-bookmark
magit-submodule magit-obsolete magit-blame magit-stash magit-reflog
magit-bisect magit-push magit-pull magit-fetch magit-clone magit-remote
magit-commit magit-sequence magit-notes magit-worktree magit-tag
magit-merge magit-branch magit-reset magit-files magit-refs magit-status
magit magit-repos magit-apply magit-wip magit-log which-func imenu
magit-diff smerge-mode diff git-commit log-edit pcvs-util add-log
magit-core magit-autorevert autorevert filenotify magit-margin
magit-transient magit-process with-editor magit-mode transient magit-git
magit-base magit-section dash compat-27 compat-26 compat compat-macs
gnus-dired sh-script smie executable pulse vc-git diff-mode
vc-dispatcher vc-svn shortdoc help-fns radix-tree cl-print paredit
edmacro icomplete time battery exwm-randr xcb-randr exwm-config exwm
exwm-input xcb-keysyms xcb-xkb exwm-manage exwm-floating xcb-cursor
xcb-render exwm-layout exwm-workspace exwm-core xcb-ewmh xcb-icccm xcb
xcb-xproto xcb-types xcb-debug kmacro server stimmung-themes
modus-operandi-theme modus-themes ytdious osm mingus libmpdee reporter
edebug debug backtrace transmission diary-lib diary-loaddefs color
calc-bin calc-ext calc calc-loaddefs rect calc-macs w3m-load mu4e
mu4e-org mu4e-main mu4e-view mu4e-headers mu4e-compose mu4e-draft
mu4e-actions smtpmail mu4e-search mu4e-lists mu4e-bookmarks mu4e-mark
mu4e-message flow-fill mule-util hl-line mu4e-contacts mu4e-update
mu4e-folders mu4e-server mu4e-context mu4e-vars mu4e-helpers mu4e-config
bookmark ido supercite regi ebdb-message ebdb-gnus gnus-msg gnus-art
mm-uu mml2015 mm-view mml-smime smime gnutls dig gnus-sum shr pixel-fill
kinsoku url-file svg dom gnus-group gnus-undo gnus-start gnus-dbus
gnus-cloud nnimap nnmail mail-source utf7 nnoo gnus-spec gnus-int
gnus-range message sendmail yank-media puny rfc822 mml mml-sec epa epg
rfc6068 epg-config mm-decode mm-bodies mm-encode mail-parse rfc2231
rfc2047 rfc2045 ietf-drums gmm-utils mailheader gnus-win gnus nnheader
gnus-util mail-utils range mm-util mail-prsvr ebdb-mua ebdb-com crm
ebdb-format ebdb mailabbrev eieio-opt speedbar ezimage dframe eieio-base
pcase timezone org ob ob-tangle ob-ref ob-lob ob-table ob-exp org-macro
org-footnote org-src ob-comint org-pcomplete org-list org-faces
org-entities org-version ob-emacs-lisp ob-core ob-eval org-table
oc-basic bibtex ol org-keys oc org-compat org-macs org-loaddefs
find-func cal-menu calendar cal-loaddefs visual-basic-mode cl web-mode
disp-table erlang-start smart-tabs-mode skeleton cc-mode cc-fonts
cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs
slime-asdf grep slime-tramp tramp tramp-loaddefs trampver
tramp-integration cus-edit cus-load wid-edit files-x tramp-compat rx
shell pcomplete parse-time iso8601 time-date ls-lisp format-spec
slime-fancy slime-indentation slime-cl-indent cl-indent
slime-trace-dialog slime-fontifying-fu slime-package-fu slime-references
slime-compiler-notes-tree slime-scratch slime-presentations advice
bridge slime-macrostep macrostep slime-mdot-fu slime-enclosing-context
slime-fuzzy slime-fancy-trace slime-fancy-inspector slime-c-p-c
slime-editing-commands slime-autodoc slime-repl elp slime-parse slime
derived cl-extra help-mode lisp-mnt gud apropos compile
text-property-search etags fileloop generator xref project arc-mode
archive-mode noutline outline icons pp comint ansi-osc ansi-color ring
hyperspec thingatpt slime-autoloads dired-aux dired-x dired
dired-loaddefs so-long notifications dbus xml repeat easy-mmode
auctex-autoloads tex-site boxquote-autoloads debbugs-autoloads
hyperbole-autoloads magit-autoloads git-commit-autoloads
magit-section-autoloads dash-autoloads paredit-autoloads
rust-mode-autoloads stimmung-themes-autoloads transient-autoloads
with-editor-autoloads info compat-autoloads ytdious-autoloads package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie generate-lisp-file url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs password-cache
json subr-x map byte-opt gv bytecomp byte-compile cconv url-vars
cl-loaddefs cl-lib rmc iso-transl tooltip eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs faces cus-face macroexp files window
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget keymap hashtable-print-readable backquote threads dbusbind
kqueue lcms2 dynamic-setting system-font-setting font-render-setting
cairo xinput2 x multi-tty make-network-process emacs)

Memory information:
((conses 16 783040 73048)
 (symbols 48 57310 3)
 (strings 32 175122 8336)
 (string-bytes 1 5700851)
 (vectors 16 96473)
 (vector-slots 8 1272766 67536)
 (floats 8 572 583)
 (intervals 56 15839 533)
 (buffers 1000 25))

-- 
Manuel Giraud

bug#58406: 29.0.50; Bars refactoring?

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> I'm trying to have some kind of highlight feature on the no-toolkit menu
> bar.  As this feature is already present in tool/tab bar, I'd like to
> mimic those.

What kind of highlight?

> +enum bar_type
> +  {
> +    MENU_BAR,
> +    TAB_BAR,
> +    TOOL_BAR
> +  };

These enums are definitely named too generally.  They could conflict
with other libraries down the road.

> +/* Get information about the bar item which is displayed in GLYPH on
> +   frame F.  Return in *PROP_IDX the index where bar item properties
> +   start in the bar items.  For TAB_BAR, return in CLOSE_P an
> +   indication whether the click was on the close-tab icon of the tab.
> +   Value is false if GLYPH doesn't display a bar item.  */
> +
> +static bool
> +bar_item_info (ENUM_BF(bar_type) bar, struct frame *f, struct glyph *glyph,
> +	       int *prop_idx, bool *close_p)

Why ENUM_BF?

BTW, I really don't recommend doing this kind of refactoring so close to
cutting the Emacs 29 branch.  Last year I and Alan Third found and fixed
several obscure bugs related to the tab bar code being a poor cargo cult
of the tool bar code, introducing subtle differences between both pieces
of code.  Any refactoring there is likely to introduce more bugs, or to
reintroduce old ones, and those bugs are much too subtle to find before
November (or even February.)

bug#58406: 29.0.50; Bars refactoring?

[Manuel Giraud]

Po Lu <luangruo@yahoo.com> writes:

> Manuel Giraud <manuel@ledu-giraud.fr> writes:
>
>> I'm trying to have some kind of highlight feature on the no-toolkit menu
>> bar.  As this feature is already present in tool/tab bar, I'd like to
>> mimic those.
>
> What kind of highlight?

mouse over highlight: like the relief on tool bar button or, even
better, with highlight face like on the modeline.

>> +enum bar_type
>> +  {
>> +    MENU_BAR,
>> +    TAB_BAR,
>> +    TOOL_BAR
>> +  };
>
> These enums are definitely named too generally.  They could conflict
> with other libraries down the road.

Ok.

[...]

> Why ENUM_BF?

A mistake.  I thought it was the way to use enum types in emacs.

> BTW, I really don't recommend doing this kind of refactoring so close to
> cutting the Emacs 29 branch.  Last year I and Alan Third found and fixed
> several obscure bugs related to the tab bar code being a poor cargo cult
> of the tool bar code, introducing subtle differences between both pieces
> of code.  Any refactoring there is likely to introduce more bugs, or to
> reintroduce old ones, and those bugs are much too subtle to find before
> November (or even February.)

I understand that it is too close to Emacs 29.  Do you think the menu
bar could be made to mimic some feature of the tool bar (like tab bar
did)?  Or do you think it would add to the cargo cult?
-- 
Manuel Giraud

bug#58406: 29.0.50; Bars refactoring?

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Mon, 10 Oct 2022 09:37:43 +0200
> 
> I'm trying to have some kind of highlight feature on the no-toolkit menu
> bar.  As this feature is already present in tool/tab bar, I'd like to
> mimic those.
> 
> While trying this, I think that I found that some "bar" functions could
> be factorized (up to a certain point I guess).  Here is a patch that
> shows my direction.
> 
> Do you think that this approach could make it into master or would it be
> frown upon?  And if so, what would be a better way?

The approach is OK, and welcome.  But let's please wait with this
refactoring until after the emacs-29 branch is cut.  OK?

Some minor comments below:

> +  /* Get the text property `menu-item' at pos. The value of that
> +     property is the start index of this item's properties in
> +     F->tool_bar_items.  */

The "F->tool_bar_items" part of the comment is outdated.

Also, our style is to leave two spaces between sentences in comments
and documentation.

> +  /* Get the start of this tool-bar item's properties in
> +     f->tool_bar_items.  */

Likewise here: outdated reference to tool_bar_items.

> +  /* Is mouse on the highlighted item?  */
> +  if (bar == TAB_BAR)
> +    return *prop_idx == f->last_tab_bar_item ? 0 : 1;
> +  else if (EQ (window, hlinfo->mouse_face_window)
> +      && *vpos >= hlinfo->mouse_face_beg_row
> +      && *vpos <= hlinfo->mouse_face_end_row
> +      && (*vpos > hlinfo->mouse_face_beg_row
> +	  || *hpos >= hlinfo->mouse_face_beg_col)
> +      && (*vpos < hlinfo->mouse_face_end_row
> +	  || *hpos < hlinfo->mouse_face_end_col
> +	  || hlinfo->mouse_face_past_end))
> +    return 0;

The "else if" clause should only be used for the tool bar, not for the
menu bar, AFAIU.

> @@ -15485,6 +15451,7 @@ handle_tool_bar_click_with_device (struct frame *f, int x, int y, bool down_p,
>    Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
>    struct window *w = XWINDOW (f->tool_bar_window);
>    int hpos, vpos, prop_idx;
> +  bool close_p;

The value of this is ignored here, so it is better to call this
variable 'ignored' or 'dummy' or something to that effect.

bug#58406: 29.0.50; Bars refactoring?

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> mouse over highlight: like the relief on tool bar button or, even
> better, with highlight face like on the modeline.

I'm afraid I don't see the utility of that in a menu bar, since it
doesn't consist of "buttons" in the sense that the tool bar or tab bar
do, and no program does that anymore.

> I understand that it is too close to Emacs 29.  Do you think the menu
> bar could be made to mimic some feature of the tool bar (like tab bar
> did)?  Or do you think it would add to the cargo cult?

I'd rather not touch any of the *bar code before Emacs 29 is cut.  Most
people do not use the bars, leading to many latent bugs.  Examples:
mouse face not being cleared when tooltips are enabled and the mouse
moves outside the frame, last_tool_bar_item not being cleared in the
same case, and last_tab_bar_item not being cleared, leading to mouse
highlight not working after the mouse is released outside a frame.

These bugs are particularly nasty because they only happen in rare
situations, but can completely ruin the visual appearance of a frame
when they do.

bug#58406: 29.0.50; Bars refactoring?

[Eli Zaretskii]

> Cc: 58406@debbugs.gnu.org, Manuel Giraud <manuel@ledu-giraud.fr>
> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Mon, 10 Oct 2022 10:37:18 +0200
> 
> > Why ENUM_BF?
> 
> A mistake.  I thought it was the way to use enum types in emacs.

Only for bitfields in a struct.

bug#58406: 29.0.50; Bars refactoring?

[Eli Zaretskii]

> Cc: 58406@debbugs.gnu.org
> Date: Mon, 10 Oct 2022 16:51:40 +0800
> From:  Po Lu via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Most people do not use the bars, leading to many latent bugs.
> Examples: mouse face not being cleared when tooltips are enabled and
> the mouse moves outside the frame, last_tool_bar_item not being
> cleared in the same case, and last_tab_bar_item not being cleared,
> leading to mouse highlight not working after the mouse is released
> outside a frame.

I do use the bars, and I don't think I see any of the above problems.
How would one notice them?

bug#58406: 29.0.50; Bars refactoring?

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 58406@debbugs.gnu.org
>> Date: Mon, 10 Oct 2022 16:51:40 +0800
>> From:  Po Lu via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> Most people do not use the bars, leading to many latent bugs.
>> Examples: mouse face not being cleared when tooltips are enabled and
>> the mouse moves outside the frame, last_tool_bar_item not being
>> cleared in the same case, and last_tab_bar_item not being cleared,
>> leading to mouse highlight not working after the mouse is released
>> outside a frame.
>
> I do use the bars, and I don't think I see any of the above problems.
> How would one notice them?

One of the problems (which was fixed earlier this summer) could be
reproduced by holding Alt-TAB (or waiting for a focus to steal the
focus) with the mouse held down on a tab or tool bar button.

bug#58406: 29.0.50; Bars refactoring?

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: manuel@ledu-giraud.fr,  58406@debbugs.gnu.org
> Date: Mon, 10 Oct 2022 18:05:08 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Cc: 58406@debbugs.gnu.org
> >> Date: Mon, 10 Oct 2022 16:51:40 +0800
> >> From:  Po Lu via "Bug reports for GNU Emacs,
> >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >> 
> >> Most people do not use the bars, leading to many latent bugs.
> >> Examples: mouse face not being cleared when tooltips are enabled and
> >> the mouse moves outside the frame, last_tool_bar_item not being
> >> cleared in the same case, and last_tab_bar_item not being cleared,
> >> leading to mouse highlight not working after the mouse is released
> >> outside a frame.
> >
> > I do use the bars, and I don't think I see any of the above problems.
> > How would one notice them?
> 
> One of the problems (which was fixed earlier this summer) could be
> reproduced by holding Alt-TAB (or waiting for a focus to steal the
> focus) with the mouse held down on a tab or tool bar button.

I just tried that, and saw nothing suspicious.  What should I watch
for? what do you see in that case?

bug#58406: 29.0.50; Bars refactoring?

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> The approach is OK, and welcome.  But let's please wait with this
> refactoring until after the emacs-29 branch is cut.  OK?

Yes.  Po Lu made that point clear too.

[...]

> The "F->tool_bar_items" part of the comment is outdated.
>
> Also, our style is to leave two spaces between sentences in comments
> and documentation.

[...]

> Likewise here: outdated reference to tool_bar_items.


Yes.  Sorry to have wasted your time: this patch is far from clean (and
it shows in comment and the like).  It was really just to show my
direction.

>> +  /* Is mouse on the highlighted item?  */
>> +  if (bar == TAB_BAR)
>> +    return *prop_idx == f->last_tab_bar_item ? 0 : 1;
>> +  else if (EQ (window, hlinfo->mouse_face_window)
>> +      && *vpos >= hlinfo->mouse_face_beg_row
>> +      && *vpos <= hlinfo->mouse_face_end_row
>> +      && (*vpos > hlinfo->mouse_face_beg_row
>> +	  || *hpos >= hlinfo->mouse_face_beg_col)
>> +      && (*vpos < hlinfo->mouse_face_end_row
>> +	  || *hpos < hlinfo->mouse_face_end_col
>> +	  || hlinfo->mouse_face_past_end))
>> +    return 0;
>
> The "else if" clause should only be used for the tool bar, not for the
> menu bar, AFAIU.

I don't know yet 😅

>> @@ -15485,6 +15451,7 @@ handle_tool_bar_click_with_device (struct frame *f, int x, int y, bool down_p,
>>    Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
>>    struct window *w = XWINDOW (f->tool_bar_window);
>>    int hpos, vpos, prop_idx;
>> +  bool close_p;
>
> The value of this is ignored here, so it is better to call this
> variable 'ignored' or 'dummy' or something to that effect.

Yes it is a good idea.  I'll try to remember that next time.
-- 
Manuel Giraud

bug#58406: 29.0.50; Bars refactoring?

[Manuel Giraud]

Po Lu <luangruo@yahoo.com> writes:

> Manuel Giraud <manuel@ledu-giraud.fr> writes:
>
>> mouse over highlight: like the relief on tool bar button or, even
>> better, with highlight face like on the modeline.
>
> I'm afraid I don't see the utility of that in a menu bar, since it
> doesn't consist of "buttons" in the sense that the tool bar or tab bar
> do, and no program does that anymore.

I find it useful to have some visual feed back that some text is
clickable (the mode-line is a good example of this IMO).  What do you
mean by "no program does that anymore"?  Do you that this is done by the
widget library?

>> I understand that it is too close to Emacs 29.  Do you think the menu
>> bar could be made to mimic some feature of the tool bar (like tab bar
>> did)?  Or do you think it would add to the cargo cult?
>
> I'd rather not touch any of the *bar code before Emacs 29 is cut.  Most
> people do not use the bars, leading to many latent bugs.  Examples:
> mouse face not being cleared when tooltips are enabled and the mouse
> moves outside the frame, last_tool_bar_item not being cleared in the
> same case, and last_tab_bar_item not being cleared, leading to mouse
> highlight not working after the mouse is released outside a frame.
>
> These bugs are particularly nasty because they only happen in rare
> situations, but can completely ruin the visual appearance of a frame
> when they do.

Ok, maybe I could look into them as an exercise on xdisp.c
-- 
Manuel Giraud

bug#58406: 29.0.50; Bars refactoring?

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> I find it useful to have some visual feed back that some text is
> clickable (the mode-line is a good example of this IMO).  What do you
> mean by "no program does that anymore"?  Do you that this is done by the
> widget library?

No, I meant that no other program has highlightable menu items in their
menu bars.

>>> I understand that it is too close to Emacs 29.  Do you think the menu
>>> bar could be made to mimic some feature of the tool bar (like tab bar
>>> did)?  Or do you think it would add to the cargo cult?
>>
>> I'd rather not touch any of the *bar code before Emacs 29 is cut.  Most
>> people do not use the bars, leading to many latent bugs.  Examples:
>> mouse face not being cleared when tooltips are enabled and the mouse
>> moves outside the frame, last_tool_bar_item not being cleared in the
>> same case, and last_tab_bar_item not being cleared, leading to mouse
>> highlight not working after the mouse is released outside a frame.
>>
>> These bugs are particularly nasty because they only happen in rare
>> situations, but can completely ruin the visual appearance of a frame
>> when they do.
>
> Ok, maybe I could look into them as an exercise on xdisp.c

The bugs previously mentioned were fixed, I'm just afraid that trying to
merge the tool bar and tab bar code will introduce more similar, nasty,
bugs.

bug#58406: 29.0.50; Bars refactoring?

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> I just tried that, and saw nothing suspicious.  What should I watch
> for? what do you see in that case?

Moving the mouse back onto the tab bar button will lead to it not being
highlighted as expected, until you click on the tab bar.

But as I said, that was fixed.  Messing with the *bar code has a very
real potential to introduce similar nasty bugs again.

bug#58406: 29.0.50; Bars refactoring?

[Manuel Giraud]

Po Lu <luangruo@yahoo.com> writes:

[...]

> No, I meant that no other program has highlightable menu items in their
> menu bars.

Ok, I see what you mean.  I've two remarks:

      - maybe, at a minimum, we could have a visual clue that we are on
        the menu bar by the mean of mouse cursor change.  Mine stays
        what it was: an arrow or a vertical insert bar.

      - what most other programs do is to highlight the menu bar entry
        when clicked (and then stay that way while the menu is opened).
        So even with this case, we'd have to have a way of highlighting
        a menu bar entry.

I understand that all of this takes place in the "--with-x-toolkit=no"
land and that it might not be the most used setup but maybe it could be
a place to test some moderately "wild" ideas 😊
-- 
Manuel Giraud

bug#58406: 29.0.50; Bars refactoring?

[Eli Zaretskii]

> Cc: 58406@debbugs.gnu.org
> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Mon, 10 Oct 2022 16:30:00 +0200
> 
> I understand that all of this takes place in the "--with-x-toolkit=no"
> land and that it might not be the most used setup but maybe it could be
> a place to test some moderately "wild" ideas 😊

The same code is used by menus on TTY frames and by the MSDOS build,
FWIW.

bug#58406: 29.0.50; Bars refactoring?

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

>       - what most other programs do is to highlight the menu bar entry
>         when clicked (and then stay that way while the menu is opened).
>         So even with this case, we'd have to have a way of highlighting
>         a menu bar entry.

That would not be mouse highlight anymore, and relying on mouse
highlight to implement that means you'd have to fix the expose
processing to respect it, in which case I'm not even sure expose
processing works at all in the oldXMenu.

Besides, the menu usually covers the bar item in the no toolkit build,
so I'm fairly certain people won't get to see the highlight.