bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

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


Hi,

So here is a patch that I think fixes the calculation of the menu bar
entry to activate from the mouse click in menu bar with the no toolkit
build.

Issue: Before this patch, if you change the font of the menu face (for
instance with (set-face-font 'menu "SomeOtherFontOfDifferentSize")), a
click in the menu bar won't activate the correct entry.

What is not in this patch: The height of the menu bar is not
recalculated correctly.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-menu-bar-height-for-with-x-toolkit-no.patch --]
[-- Type: text/x-patch, Size: 3058 bytes --]

>From 100ed84198cd97664a94e89d1730e97b1fabbbb9 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 14 Oct 2022 16:24:10 +0200
Subject: [PATCH] Fix menu bar height for --with-x-toolkit=no

* src/xterm.c (x_menu_bar_height): New function to compute menu
bar height with menu face and boxed information.
(x_new_font): Use it.

* src/xfns.c (x_set_menu_bar_lines): Use it.
---
 src/xfns.c  |  2 +-
 src/xterm.c | 25 ++++++++++++++++++++++++-
 src/xterm.h |  1 +
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/src/xfns.c b/src/xfns.c
index 9112448899..3d93a6a207 100644
--- a/src/xfns.c
+++ b/src/xfns.c
@@ -1674,7 +1674,7 @@ x_set_menu_bar_lines (struct frame *f, Lisp_Object value, Lisp_Object oldval)
     }
 #else /* not USE_X_TOOLKIT && not USE_GTK */
   FRAME_MENU_BAR_LINES (f) = nlines;
-  FRAME_MENU_BAR_HEIGHT (f) = nlines * FRAME_LINE_HEIGHT (f);
+  FRAME_MENU_BAR_HEIGHT (f) = nlines * x_menu_bar_height (f);
   if (FRAME_X_WINDOW (f))
     x_clear_under_internal_border (f);
 
diff --git a/src/xterm.c b/src/xterm.c
index 9059ad7136..c3bc494667 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -25464,6 +25464,29 @@ x_io_error_quitter (Display *display)
 \f
 /* Changing the font of the frame.  */
 
+int
+x_menu_bar_height (struct frame *f)
+{
+  int font_ascent = 0;
+  int font_descent = 0;
+  int height = 0;
+
+  recompute_basic_faces (f);
+  if (FRAME_FACE_CACHE (f))
+    {
+      struct face *face = FACE_FROM_ID_OR_NULL (f, MENU_FACE_ID);
+      if (face && face->font)
+	{
+	  height += (face->box_horizontal_line_width > 0)
+	    ? (face->box_horizontal_line_width * 2)
+	    : 0;
+	  get_font_ascent_descent (face->font, &font_ascent, &font_descent);
+	}
+    }
+
+  return (height + font_ascent + font_descent);
+}
+
 /* Give frame F the font FONT-OBJECT as its default font.  The return
    value is FONT-OBJECT.  FONTSET is an ID of the fontset for the
    frame.  If it is negative, generate a new fontset from
@@ -25490,7 +25513,7 @@ x_new_font (struct frame *f, Lisp_Object font_object, int fontset)
   FRAME_LINE_HEIGHT (f) = font_ascent + font_descent;
 
 #ifndef USE_X_TOOLKIT
-  FRAME_MENU_BAR_HEIGHT (f) = FRAME_MENU_BAR_LINES (f) * FRAME_LINE_HEIGHT (f);
+  FRAME_MENU_BAR_HEIGHT (f) = FRAME_MENU_BAR_LINES (f) * x_menu_bar_height (f);
 #endif
   /* We could use a more elaborate calculation here.  */
   FRAME_TAB_BAR_HEIGHT (f) = FRAME_TAB_BAR_LINES (f) * FRAME_LINE_HEIGHT (f);
diff --git a/src/xterm.h b/src/xterm.h
index b68a234faa..5b914d48da 100644
--- a/src/xterm.h
+++ b/src/xterm.h
@@ -1595,6 +1595,7 @@ #define SELECTION_EVENT_TIME(eventp)	\
 extern void x_ignore_errors_for_next_request (struct x_display_info *);
 extern void x_stop_ignoring_errors (struct x_display_info *);
 extern void x_clear_errors (Display *);
+extern int x_menu_bar_height (struct frame *);
 extern void x_set_window_size (struct frame *, bool, int, int);
 extern void x_set_last_user_time_from_lisp (struct x_display_info *, Time);
 extern void x_make_frame_visible (struct frame *);
-- 
2.38.0


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


Thanks,


In GNU Emacs 29.0.50 (build 1, x86_64-unknown-openbsd7.2, cairo version
 1.17.6) of 2022-11-18 built on elite.giraud
Repository revision: 61b9f2c3179aa454963493e1923307e875e2322a
Repository branch: mgi/update-menu-bar-height
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: OpenBSD elite.giraud 7.2 GENERIC.MP#835 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:
  gnus-dired-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  display-time-mode: t
  display-battery-mode: t
  server-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
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-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/.el/exwm/exwm hides /home/manuel/.emacs.d/elpa/exwm-0.27/exwm
/home/manuel/.el/exwm/exwm-xim hides /home/manuel/.emacs.d/elpa/exwm-0.27/exwm-xim
/home/manuel/.el/exwm/exwm-workspace hides /home/manuel/.emacs.d/elpa/exwm-0.27/exwm-workspace
/home/manuel/.el/exwm/exwm-systemtray hides /home/manuel/.emacs.d/elpa/exwm-0.27/exwm-systemtray
/home/manuel/.el/exwm/exwm-randr hides /home/manuel/.emacs.d/elpa/exwm-0.27/exwm-randr
/home/manuel/.el/exwm/exwm-manage hides /home/manuel/.emacs.d/elpa/exwm-0.27/exwm-manage
/home/manuel/.el/exwm/exwm-layout hides /home/manuel/.emacs.d/elpa/exwm-0.27/exwm-layout
/home/manuel/.el/exwm/exwm-input hides /home/manuel/.emacs.d/elpa/exwm-0.27/exwm-input
/home/manuel/.el/exwm/exwm-floating hides /home/manuel/.emacs.d/elpa/exwm-0.27/exwm-floating
/home/manuel/.el/exwm/exwm-core hides /home/manuel/.emacs.d/elpa/exwm-0.27/exwm-core
/home/manuel/.el/exwm/exwm-config hides /home/manuel/.emacs.d/elpa/exwm-0.27/exwm-config
/home/manuel/.el/exwm/exwm-cm hides /home/manuel/.emacs.d/elpa/exwm-0.27/exwm-cm
/home/manuel/.emacs.d/elpa/transient-20221028.1430/transient hides /home/manuel/emacs/share/emacs/29.0.50/lisp/transient

Features:
(shadow sort mail-extr emacsbug whitespace gnus-dired magit-patch
dabbrev executable vc-git vc-dispatcher vc-svn bug-reference
magit-extras 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 diff-mode 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 tmm cus-start
pulse tutorial paredit edmacro 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 nnselect
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 inline mailabbrev eieio-opt
cl-extra help-mode 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 derived 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 advice
slime-scratch slime-presentations 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 slime-parse slime 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 rust-mode-autoloads stimmung-themes-autoloads
debbugs-autoloads paredit-autoloads ebdb-autoloads ef-themes-autoloads
ytdious-autoloads auctex-autoloads tex-site exwm-autoloads
hyperbole-autoloads magit-autoloads magit-section-autoloads
git-commit-autoloads with-editor-autoloads transient-autoloads
dash-autoloads compat-autoloads info 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 url-vars cl-loaddefs cl-lib rmc
iso-transl tooltip cconv 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 theme-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 756398 76643)
 (symbols 48 57143 2)
 (strings 32 169527 9002)
 (string-bytes 1 5556598)
 (vectors 16 101653)
 (vector-slots 8 1331942 88236)
 (floats 8 528 478)
 (intervals 56 10690 906)
 (buffers 984 26))

-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

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

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

> Hi,
>
> So here is a patch that I think fixes the calculation of the menu bar
> entry to activate from the mouse click in menu bar with the no toolkit
> build.
>
> Issue: Before this patch, if you change the font of the menu face (for
> instance with (set-face-font 'menu "SomeOtherFontOfDifferentSize")), a
> click in the menu bar won't activate the correct entry.
>
> What is not in this patch: The height of the menu bar is not
> recalculated correctly.

Judging by the contents of the patch, you sent the wrong one.  :(

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

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

Po Lu <luangruo@yahoo.com> writes:

[...]

> Judging by the contents of the patch, you sent the wrong one.  :(

😅 sorry for this.  Here is the good one.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-click-position-to-menu-bar-entry-with-no-toolkit.patch --]
[-- Type: text/x-patch, Size: 2614 bytes --]

From 697cbac26e81457840f187399530b0f91adb1768 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 18 Nov 2022 09:24:55 +0100
Subject: [PATCH] Fix click position to menu bar entry with no-toolkit

* src/keyboard.c (make_lispy_event): Use x_y_to_hpos_vpos to
compute correct menu bar position should the menu face change.
* src/xdisp.c (x_y_to_hpos_vpos): Not static anymore.
* src/dispextern.h: Export x_y_to_hpos_vpos.
---
 src/dispextern.h |  2 ++
 src/keyboard.c   | 13 +++++++++++--
 src/xdisp.c      |  2 +-
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 2f5f4335fe..c59f594f9e 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3376,6 +3376,8 @@ #define TTY_CAP_STRIKE_THROUGH	0x20
 struct glyph_row *row_containing_pos (struct window *, ptrdiff_t,
                                       struct glyph_row *,
                                       struct glyph_row *, int);
+struct glyph *x_y_to_hpos_vpos (struct window *, int, int, int *, int *,
+				int *, int *, int *);
 int line_bottom_y (struct it *);
 int default_line_pixel_height (struct window *);
 bool display_prop_intangible_p (Lisp_Object, Lisp_Object, ptrdiff_t, ptrdiff_t);
diff --git a/src/keyboard.c b/src/keyboard.c
index 069cf0627b..5cc7209846 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -5974,8 +5974,17 @@ make_lispy_event (struct input_event *event)
 	       in a menu (non-toolkit version).  */
 	    if (!toolkit_menubar_in_use (f))
 	      {
-		pixel_to_glyph_coords (f, XFIXNUM (event->x), XFIXNUM (event->y),
-				       &column, &row, NULL, 1);
+		int dummy;
+
+		/* I guess this works because the menu bar is always
+		   at position (0, 0) in a frame.  Should this changed
+		   and we need a way to calculate the correct position
+		   into the menu bar from the position of the event in
+		   the frame.  */
+		x_y_to_hpos_vpos (XWINDOW (f->menu_bar_window),
+				  XFIXNUM (event->x), XFIXNUM (event->y),
+				  &column, &row,
+				  NULL, NULL, &dummy);
 
 		/* In the non-toolkit version, clicks on the menu bar
 		   are ordinary button events in the event buffer.
diff --git a/src/xdisp.c b/src/xdisp.c
index f6a279636a..0c073c0fb5 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -2330,7 +2330,7 @@ pixel_to_glyph_coords (struct frame *f, int pix_x, int pix_y, int *x, int *y,
    text, or we can't tell because W's current matrix is not up to
    date.  */
 
-static struct glyph *
+struct glyph *
 x_y_to_hpos_vpos (struct window *w, int x, int y, int *hpos, int *vpos,
 		  int *dx, int *dy, int *area)
 {
-- 
2.38.1


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

-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

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

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

> +struct glyph *x_y_to_hpos_vpos (struct window *, int, int, int *, int *,
> +				int *, int *, int *);

Shouldn't this be "extern struct glyph *x_y_to_hpos_vpos ...?"

> +		/* I guess this works because the menu bar is always
> +		   at position (0, 0) in a frame.  Should this changed
> +		   and we need a way to calculate the correct position
> +		   into the menu bar from the position of the event in
> +		   the frame.  */

I think this comment is unnecessary.  Where else would a menu bar be?

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Fri, 18 Nov 2022 09:37:20 +0100
> 
> What is not in this patch: The height of the menu bar is not
> recalculated correctly.

Why not?  And why did you not include that in the patch?

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> Cc: 59351@debbugs.gnu.org
> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Fri, 18 Nov 2022 12:01:15 +0100
> 
> diff --git a/src/keyboard.c b/src/keyboard.c
> index 069cf0627b..5cc7209846 100644
> --- a/src/keyboard.c
> +++ b/src/keyboard.c
> @@ -5974,8 +5974,17 @@ make_lispy_event (struct input_event *event)
>  	       in a menu (non-toolkit version).  */
>  	    if (!toolkit_menubar_in_use (f))
>  	      {
> -		pixel_to_glyph_coords (f, XFIXNUM (event->x), XFIXNUM (event->y),
> -				       &column, &row, NULL, 1);
> +		int dummy;
> +
> +		/* I guess this works because the menu bar is always
> +		   at position (0, 0) in a frame.  Should this changed
> +		   and we need a way to calculate the correct position
> +		   into the menu bar from the position of the event in
> +		   the frame.  */
> +		x_y_to_hpos_vpos (XWINDOW (f->menu_bar_window),
> +				  XFIXNUM (event->x), XFIXNUM (event->y),
> +				  &column, &row,
> +				  NULL, NULL, &dummy);

Did you test the results on a TTY frame that has a mouse?

Also, does this work after the user clicks the menu bar and Emacs
drops down a menu for the menu item on which the user clicked?  I
don't remember what happens to the pseudo-window geometry when that
happens.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Po Lu <luangruo@yahoo.com> writes:

> Manuel Giraud <manuel@ledu-giraud.fr> writes:
>
>> +struct glyph *x_y_to_hpos_vpos (struct window *, int, int, int *, int *,
>> +				int *, int *, int *);
>
> Shouldn't this be "extern struct glyph *x_y_to_hpos_vpos ...?"

Ok.  I'll fix that.

>> +		/* I guess this works because the menu bar is always
>> +		   at position (0, 0) in a frame.  Should this changed
>> +		   and we need a way to calculate the correct position
>> +		   into the menu bar from the position of the event in
>> +		   the frame.  */
>
> I think this comment is unnecessary.  Where else would a menu bar be?

I don't know.  Maybe in some distant future, menu bar will be anywhere
:-)  It also helps with the previous comment about event position being
relative to frame because here we are converting in f->menu_bar_window.
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Date: Fri, 18 Nov 2022 09:37:20 +0100
>> 
>> What is not in this patch: The height of the menu bar is not
>> recalculated correctly.
>
> Why not?  And why did you not include that in the patch?

It is not that I don't want to… but I have not yet found how to do it.
As you know, Emacs display code is hard to understand.
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> Did you test the results on a TTY frame that has a mouse?

Yes, I should check that.

> Also, does this work after the user clicks the menu bar and Emacs
> drops down a menu for the menu item on which the user clicked?  I
> don't remember what happens to the pseudo-window geometry when that
> happens.

I do not understand your question.
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 59351@debbugs.gnu.org
> Date: Fri, 18 Nov 2022 13:12:57 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Manuel Giraud <manuel@ledu-giraud.fr>
> >> Date: Fri, 18 Nov 2022 09:37:20 +0100
> >> 
> >> What is not in this patch: The height of the menu bar is not
> >> recalculated correctly.
> >
> > Why not?  And why did you not include that in the patch?
> 
> It is not that I don't want to… but I have not yet found how to do it.
> As you know, Emacs display code is hard to understand.

display_menu_bar is supposed to update the metrics of the menu-bar
line, see the call to compute_line_metrics at its end.  Why doesn't it
happen?

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: luangruo@yahoo.com,  59351@debbugs.gnu.org
> Date: Fri, 18 Nov 2022 13:15:57 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Also, does this work after the user clicks the menu bar and Emacs
> > drops down a menu for the menu item on which the user clicked?  I
> > don't remember what happens to the pseudo-window geometry when that
> > happens.
> 
> I do not understand your question.

When you click on the menu bar, Emacs drops down a menu, right?  For
example, if you click on "File", you will see a menu with items like
"Visit New File...", "Open File..." etc.  After that, if you click in
the dropped-down menu, is the code you suggest to modify involved, and
if so, does the conversion from X/Y to COL/ROW still work?

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> When you click on the menu bar, Emacs drops down a menu, right?  For
> example, if you click on "File", you will see a menu with items like
> "Visit New File...", "Open File..." etc.  After that, if you click in
> the dropped-down menu, is the code you suggest to modify involved, and
> if so, does the conversion from X/Y to COL/ROW still work?

Po could correct me but I do not think that this code is involved here.
I think that when *into* the menu most things are done by the oldXmenu
library.
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: luangruo@yahoo.com,  59351@debbugs.gnu.org
> Date: Fri, 18 Nov 2022 13:41:25 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [...]
> 
> > When you click on the menu bar, Emacs drops down a menu, right?  For
> > example, if you click on "File", you will see a menu with items like
> > "Visit New File...", "Open File..." etc.  After that, if you click in
> > the dropped-down menu, is the code you suggest to modify involved, and
> > if so, does the conversion from X/Y to COL/ROW still work?
> 
> Po could correct me but I do not think that this code is involved here.
> I think that when *into* the menu most things are done by the oldXmenu
> library.

Possibly.  It's been a while since I looked at that code and debugged
it.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

> Possibly.  It's been a while since I looked at that code and debugged
> it.

Yes, I might be reviving old stuff.  But I find it useful anyway.
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> display_menu_bar is supposed to update the metrics of the menu-bar
> line, see the call to compute_line_metrics at its end.  Why doesn't it
> happen?

Yes but it seems that this updating does not take place when the menu
face is changed (not the whole frame face, though: this does work).  And
the menu face, when changed, is displayed correctly, but the height of
the window is not modified accordingly.
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> Cc: 59351@debbugs.gnu.org
> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Fri, 18 Nov 2022 13:10:07 +0100
> 
> Po Lu <luangruo@yahoo.com> writes:
> 
> >> +		/* I guess this works because the menu bar is always
> >> +		   at position (0, 0) in a frame.  Should this changed
> >> +		   and we need a way to calculate the correct position
> >> +		   into the menu bar from the position of the event in
> >> +		   the frame.  */
> >
> > I think this comment is unnecessary.  Where else would a menu bar be?
> 
> I don't know.  Maybe in some distant future, menu bar will be anywhere
> :-)  It also helps with the previous comment about event position being
> relative to frame because here we are converting in f->menu_bar_window.

Why not use FRAME_TO_WINDOW_PIXEL_X and FRAME_TO_WINDOW_PIXEL_Y, and
make sure this will _always_ work?

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

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

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> Did you test the results on a TTY frame that has a mouse?

It doesn't even compile --without-x.  So here is a new version of this
patch.  I even remove the comment as suggested by Po.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-click-position-to-menu-bar-entry-with-no-toolkit.patch --]
[-- Type: text/x-patch, Size: 2390 bytes --]

From 225d6eb5cf17587619850ee2143e1e59a1c9fa4e Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 18 Nov 2022 09:24:55 +0100
Subject: [PATCH] Fix click position to menu bar entry with no-toolkit

* src/keyboard.c (make_lispy_event): Use x_y_to_hpos_vpos to
compute correct menu bar position should the menu face change.
* src/xdisp.c (x_y_to_hpos_vpos): Not static anymore.
* src/dispextern.h: Export x_y_to_hpos_vpos.
---
 src/dispextern.h | 3 ++-
 src/keyboard.c   | 9 +++++++++
 src/xdisp.c      | 2 +-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 2f5f4335fe..2afbdeabaa 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3495,7 +3495,8 @@ #define TTY_CAP_STRIKE_THROUGH	0x20
 extern void tty_draw_row_with_mouse_face (struct window *, struct glyph_row *,
 					  int, int, enum draw_glyphs_face);
 extern void display_tty_menu_item (const char *, int, int, int, int, bool);
-
+extern struct glyph *x_y_to_hpos_vpos (struct window *, int, int, int *, int *,
+				       int *, int *, int *);
 /* Flags passed to try_window.  */
 #define TRY_WINDOW_CHECK_MARGINS	(1 << 0)
 #define TRY_WINDOW_IGNORE_FONTS_CHANGE	(1 << 1)
diff --git a/src/keyboard.c b/src/keyboard.c
index 069cf0627b..716ee973a2 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -5974,8 +5974,17 @@ make_lispy_event (struct input_event *event)
 	       in a menu (non-toolkit version).  */
 	    if (!toolkit_menubar_in_use (f))
 	      {
+#if defined HAVE_WINDOW_SYSTEM
+		int dummy;
+
+		x_y_to_hpos_vpos (XWINDOW (f->menu_bar_window),
+				  XFIXNUM (event->x), XFIXNUM (event->y),
+				  &column, &row,
+				  NULL, NULL, &dummy);
+#else
 		pixel_to_glyph_coords (f, XFIXNUM (event->x), XFIXNUM (event->y),
 				       &column, &row, NULL, 1);
+#endif
 
 		/* In the non-toolkit version, clicks on the menu bar
 		   are ordinary button events in the event buffer.
diff --git a/src/xdisp.c b/src/xdisp.c
index f6a279636a..0c073c0fb5 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -2330,7 +2330,7 @@ pixel_to_glyph_coords (struct frame *f, int pix_x, int pix_y, int *x, int *y,
    text, or we can't tell because W's current matrix is not up to
    date.  */
 
-static struct glyph *
+struct glyph *
 x_y_to_hpos_vpos (struct window *w, int x, int y, int *hpos, int *vpos,
 		  int *dx, int *dy, int *area)
 {
-- 
2.38.1


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

-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

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

Eli Zaretskii <eliz@gnu.org> writes:

> When you click on the menu bar, Emacs drops down a menu, right?  For
> example, if you click on "File", you will see a menu with items like
> "Visit New File...", "Open File..." etc.  After that, if you click in
> the dropped-down menu, is the code you suggest to modify involved, and
> if so, does the conversion from X/Y to COL/ROW still work?

Right, that is a problem.  I tried and saw the menu selection stop
working at all.

My suggestion is to use the new code only under X, which is the only
place where that piece of keyboard.c is used and variable-width fonts
are supported.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: luangruo@yahoo.com,  59351@debbugs.gnu.org
>> Date: Fri, 18 Nov 2022 13:41:25 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> [...]
>> 
>> > When you click on the menu bar, Emacs drops down a menu, right?  For
>> > example, if you click on "File", you will see a menu with items like
>> > "Visit New File...", "Open File..." etc.  After that, if you click in
>> > the dropped-down menu, is the code you suggest to modify involved, and
>> > if so, does the conversion from X/Y to COL/ROW still work?
>> 
>> Po could correct me but I do not think that this code is involved here.
>> I think that when *into* the menu most things are done by the oldXmenu
>> library.
>
> Possibly.  It's been a while since I looked at that code and debugged
> it.

Please see my other reply.  The pseudo-window is changed when a menu bar
is activated on a TTY with a mouse, but not under X, where the XMenu
library is used to display the menu.  Since the proposed code only makes
a difference on terminals with variable-width fonts, my suggestion is to
use it only on X terminals, whilst letting the TTY menu bar operate as
before.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

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

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

> I don't know.  Maybe in some distant future, menu bar will be anywhere
> :-)  It also helps with the previous comment about event position being
> relative to frame because here we are converting in f->menu_bar_window.

If so, then why not stay safe and use the FRAME_TO_WINDOW_PIXEL_ macros
to convert the coordinates to that of the menu bar window?

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Po Lu <luangruo@yahoo.com> writes:

[...]

> Please see my other reply.  The pseudo-window is changed when a menu bar
> is activated on a TTY with a mouse, but not under X, where the XMenu
> library is used to display the menu.  Since the proposed code only makes
> a difference on terminals with variable-width fonts, my suggestion is to
> use it only on X terminals, whilst letting the TTY menu bar operate as
> before.

Ok. I think that the #ifdef of the new version of the patch did just
that (i.e. use old cold if not on X)…  but wait!  I'm testing a new
version using FRAME_TO_WINDOW_PIXEL_* macros you two proposed.
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

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

Here is a new version of the fix.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-click-position-to-menu-bar-entry-with-no-toolkit.patch --]
[-- Type: text/x-patch, Size: 2534 bytes --]

From 4d7774ca34d53bd8cf147ade7bf058a3b8876f7e Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 18 Nov 2022 09:24:55 +0100
Subject: [PATCH] Fix click position to menu bar entry with no-toolkit

* src/keyboard.c (make_lispy_event): Use x_y_to_hpos_vpos to
compute correct menu bar position should the menu face change.
* src/xdisp.c (x_y_to_hpos_vpos): Not static anymore.
* src/dispextern.h: Export x_y_to_hpos_vpos.
---
 src/dispextern.h |  3 ++-
 src/keyboard.c   | 11 +++++++++++
 src/xdisp.c      |  2 +-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 2f5f4335fe..2afbdeabaa 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3495,7 +3495,8 @@ #define TTY_CAP_STRIKE_THROUGH	0x20
 extern void tty_draw_row_with_mouse_face (struct window *, struct glyph_row *,
 					  int, int, enum draw_glyphs_face);
 extern void display_tty_menu_item (const char *, int, int, int, int, bool);
-
+extern struct glyph *x_y_to_hpos_vpos (struct window *, int, int, int *, int *,
+				       int *, int *, int *);
 /* Flags passed to try_window.  */
 #define TRY_WINDOW_CHECK_MARGINS	(1 << 0)
 #define TRY_WINDOW_IGNORE_FONTS_CHANGE	(1 << 1)
diff --git a/src/keyboard.c b/src/keyboard.c
index 069cf0627b..5a204c8ebd 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -5974,8 +5974,19 @@ make_lispy_event (struct input_event *event)
 	       in a menu (non-toolkit version).  */
 	    if (!toolkit_menubar_in_use (f))
 	      {
+#if defined HAVE_WINDOW_SYSTEM
+		struct window *menu_w = XWINDOW (f->menu_bar_window);
+		int x, y, dummy;
+
+		x = FRAME_TO_WINDOW_PIXEL_X (menu_w, XFIXNUM (event->x));
+		y = FRAME_TO_WINDOW_PIXEL_Y (menu_w, XFIXNUM (event->y));
+
+		x_y_to_hpos_vpos (XWINDOW (f->menu_bar_window), x, y, &column, &row,
+				  NULL, NULL, &dummy);
+#else
 		pixel_to_glyph_coords (f, XFIXNUM (event->x), XFIXNUM (event->y),
 				       &column, &row, NULL, 1);
+#endif
 
 		/* In the non-toolkit version, clicks on the menu bar
 		   are ordinary button events in the event buffer.
diff --git a/src/xdisp.c b/src/xdisp.c
index f6a279636a..0c073c0fb5 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -2330,7 +2330,7 @@ pixel_to_glyph_coords (struct frame *f, int pix_x, int pix_y, int *x, int *y,
    text, or we can't tell because W's current matrix is not up to
    date.  */
 
-static struct glyph *
+struct glyph *
 x_y_to_hpos_vpos (struct window *w, int x, int y, int *hpos, int *vpos,
 		  int *dx, int *dy, int *area)
 {
-- 
2.38.1


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

-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

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

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

> +#if defined HAVE_WINDOW_SYSTEM
> +		struct window *menu_w = XWINDOW (f->menu_bar_window);
> +		int x, y, dummy;
> +
> +		x = FRAME_TO_WINDOW_PIXEL_X (menu_w, XFIXNUM (event->x));
> +		y = FRAME_TO_WINDOW_PIXEL_Y (menu_w, XFIXNUM (event->y));
> +
> +		x_y_to_hpos_vpos (XWINDOW (f->menu_bar_window), x, y, &column, &row,
> +				  NULL, NULL, &dummy);
> +#else

Builds with X can still create TTY frames!

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 59351@debbugs.gnu.org
> Date: Fri, 18 Nov 2022 14:16:34 +0100
> 
> > display_menu_bar is supposed to update the metrics of the menu-bar
> > line, see the call to compute_line_metrics at its end.  Why doesn't it
> > happen?
> 
> Yes but it seems that this updating does not take place when the menu
> face is changed (not the whole frame face, though: this does work).  And
> the menu face, when changed, is displayed correctly, but the height of
> the window is not modified accordingly.

You mean, display_menu_bar is not called in that case?  If that is the
problem, we should look into the reason.  Perhaps some condition is
missing for when display_menu_bar is called from redisplay_window.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: luangruo@yahoo.com,  59351@debbugs.gnu.org
> Date: Fri, 18 Nov 2022 14:20:15 +0100
> 
> > Did you test the results on a TTY frame that has a mouse?
> 
> It doesn't even compile --without-x.  So here is a new version of this
> patch.  I even remove the comment as suggested by Po.
> 
> --- a/src/keyboard.c
> +++ b/src/keyboard.c
> @@ -5974,8 +5974,17 @@ make_lispy_event (struct input_event *event)
>  	       in a menu (non-toolkit version).  */
>  	    if (!toolkit_menubar_in_use (f))
>  	      {
> +#if defined HAVE_WINDOW_SYSTEM
> +		int dummy;
> +
> +		x_y_to_hpos_vpos (XWINDOW (f->menu_bar_window),
> +				  XFIXNUM (event->x), XFIXNUM (event->y),
> +				  &column, &row,
> +				  NULL, NULL, &dummy);
> +#else
>  		pixel_to_glyph_coords (f, XFIXNUM (event->x), XFIXNUM (event->y),
>  				       &column, &row, NULL, 1);
> +#endif

This cannot be a compile-time condition, because Emacs compiled with X
support can still have TTY frames.  So you need to test for
FRAME_WINDOW_P, and only call x_y_to_hpos_vpos if FRAME_WINDOW_P
returns non-zero; otherwise call pixel_to_glyph_coords even in a build
where HAVE_WINDOW_SYSTEM is defined.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,  59351@debbugs.gnu.org
> Date: Fri, 18 Nov 2022 21:23:09 +0800
> 
> Please see my other reply.  The pseudo-window is changed when a menu bar
> is activated on a TTY with a mouse

On TTY frames the menu bar is not a window, it's just a special line.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

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

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> This cannot be a compile-time condition, because Emacs compiled with X
> support can still have TTY frames.  So you need to test for
> FRAME_WINDOW_P, and only call x_y_to_hpos_vpos if FRAME_WINDOW_P
> returns non-zero; otherwise call pixel_to_glyph_coords even in a build
> where HAVE_WINDOW_SYSTEM is defined.

Ok.  Sorry about that.  Here is a new one.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-click-position-to-menu-bar-entry-with-no-toolkit.patch --]
[-- Type: text/x-patch, Size: 2668 bytes --]

From 13cd5a971b6ff7e7b809b9cc4073039c695b9dcd Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 18 Nov 2022 09:24:55 +0100
Subject: [PATCH] Fix click position to menu bar entry with no-toolkit

* src/keyboard.c (make_lispy_event): Use x_y_to_hpos_vpos to
compute correct menu bar position should the menu face change.
* src/xdisp.c (x_y_to_hpos_vpos): Not static anymore.
* src/dispextern.h: Export x_y_to_hpos_vpos.
---
 src/dispextern.h |  3 ++-
 src/keyboard.c   | 16 ++++++++++++++--
 src/xdisp.c      |  2 +-
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 2f5f4335fe..2afbdeabaa 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3495,7 +3495,8 @@ #define TTY_CAP_STRIKE_THROUGH	0x20
 extern void tty_draw_row_with_mouse_face (struct window *, struct glyph_row *,
 					  int, int, enum draw_glyphs_face);
 extern void display_tty_menu_item (const char *, int, int, int, int, bool);
-
+extern struct glyph *x_y_to_hpos_vpos (struct window *, int, int, int *, int *,
+				       int *, int *, int *);
 /* Flags passed to try_window.  */
 #define TRY_WINDOW_CHECK_MARGINS	(1 << 0)
 #define TRY_WINDOW_IGNORE_FONTS_CHANGE	(1 << 1)
diff --git a/src/keyboard.c b/src/keyboard.c
index 069cf0627b..c16f8808c4 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -5974,8 +5974,20 @@ make_lispy_event (struct input_event *event)
 	       in a menu (non-toolkit version).  */
 	    if (!toolkit_menubar_in_use (f))
 	      {
-		pixel_to_glyph_coords (f, XFIXNUM (event->x), XFIXNUM (event->y),
-				       &column, &row, NULL, 1);
+		if (FRAME_WINDOW_P (f))
+		  {
+		    struct window *menu_w = XWINDOW (f->menu_bar_window);
+		    int x, y, dummy;
+
+		    x = FRAME_TO_WINDOW_PIXEL_X (menu_w, XFIXNUM (event->x));
+		    y = FRAME_TO_WINDOW_PIXEL_Y (menu_w, XFIXNUM (event->y));
+
+		    x_y_to_hpos_vpos (XWINDOW (f->menu_bar_window), x, y, &column, &row,
+				      NULL, NULL, &dummy);
+		  }
+		else
+		  pixel_to_glyph_coords (f, XFIXNUM (event->x), XFIXNUM (event->y),
+					 &column, &row, NULL, 1);
 
 		/* In the non-toolkit version, clicks on the menu bar
 		   are ordinary button events in the event buffer.
diff --git a/src/xdisp.c b/src/xdisp.c
index f6a279636a..0c073c0fb5 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -2330,7 +2330,7 @@ pixel_to_glyph_coords (struct frame *f, int pix_x, int pix_y, int *x, int *y,
    text, or we can't tell because W's current matrix is not up to
    date.  */
 
-static struct glyph *
+struct glyph *
 x_y_to_hpos_vpos (struct window *w, int x, int y, int *hpos, int *vpos,
 		  int *dx, int *dy, int *area)
 {
-- 
2.38.1


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

-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: luangruo@yahoo.com,  59351@debbugs.gnu.org
> Date: Fri, 18 Nov 2022 16:20:16 +0100
> 
> > This cannot be a compile-time condition, because Emacs compiled with X
> > support can still have TTY frames.  So you need to test for
> > FRAME_WINDOW_P, and only call x_y_to_hpos_vpos if FRAME_WINDOW_P
> > returns non-zero; otherwise call pixel_to_glyph_coords even in a build
> > where HAVE_WINDOW_SYSTEM is defined.
> 
> Ok.  Sorry about that.  Here is a new one.

No need to be sorry, no one can possibly remember all this stuff all
the time.

> --- a/src/keyboard.c
> +++ b/src/keyboard.c
> @@ -5974,8 +5974,20 @@ make_lispy_event (struct input_event *event)
>  	       in a menu (non-toolkit version).  */
>  	    if (!toolkit_menubar_in_use (f))
>  	      {
> -		pixel_to_glyph_coords (f, XFIXNUM (event->x), XFIXNUM (event->y),
> -				       &column, &row, NULL, 1);
> +		if (FRAME_WINDOW_P (f))
> +		  {
> +		    struct window *menu_w = XWINDOW (f->menu_bar_window);
> +		    int x, y, dummy;
> +
> +		    x = FRAME_TO_WINDOW_PIXEL_X (menu_w, XFIXNUM (event->x));
> +		    y = FRAME_TO_WINDOW_PIXEL_Y (menu_w, XFIXNUM (event->y));
> +
> +		    x_y_to_hpos_vpos (XWINDOW (f->menu_bar_window), x, y, &column, &row,
> +				      NULL, NULL, &dummy);
> +		  }
> +		else
> +		  pixel_to_glyph_coords (f, XFIXNUM (event->x), XFIXNUM (event->y),
> +					 &column, &row, NULL, 1);

I thought you said XWINDOW (f->menu_bar_window) doesn't compile in the
"--without-x" build?  I think you need the HAVE_WINDOW_SYSTEM
condition as well.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

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

Eli Zaretskii <eliz@gnu.org> writes:

> I thought you said XWINDOW (f->menu_bar_window) doesn't compile in the
> "--without-x" build?  I think you need the HAVE_WINDOW_SYSTEM
> condition as well.

This new one does compile "--without-x".

About the height of the menu bar, you were right.  I've tested and
display_menu_bar is called quite often and is called after a
(set-face-font 'menu "Iosevka-18").  So maybe it is the last call to
compute_line_metrics that does not do the required job.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-click-position-to-menu-bar-entry-with-no-toolkit.patch --]
[-- Type: text/x-patch, Size: 2710 bytes --]

From 6fc191dcfa85ac98a482a7e373a4221499f86825 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 18 Nov 2022 09:24:55 +0100
Subject: [PATCH] Fix click position to menu bar entry with no-toolkit

* src/keyboard.c (make_lispy_event): Use x_y_to_hpos_vpos to
compute correct menu bar position should the menu face change.
* src/xdisp.c (x_y_to_hpos_vpos): Not static anymore.
* src/dispextern.h: Export x_y_to_hpos_vpos.
---
 src/dispextern.h |  3 ++-
 src/keyboard.c   | 18 ++++++++++++++++--
 src/xdisp.c      |  2 +-
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 2f5f4335fe..2afbdeabaa 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3495,7 +3495,8 @@ #define TTY_CAP_STRIKE_THROUGH	0x20
 extern void tty_draw_row_with_mouse_face (struct window *, struct glyph_row *,
 					  int, int, enum draw_glyphs_face);
 extern void display_tty_menu_item (const char *, int, int, int, int, bool);
-
+extern struct glyph *x_y_to_hpos_vpos (struct window *, int, int, int *, int *,
+				       int *, int *, int *);
 /* Flags passed to try_window.  */
 #define TRY_WINDOW_CHECK_MARGINS	(1 << 0)
 #define TRY_WINDOW_IGNORE_FONTS_CHANGE	(1 << 1)
diff --git a/src/keyboard.c b/src/keyboard.c
index 069cf0627b..6ce6ce17f2 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -5974,8 +5974,22 @@ make_lispy_event (struct input_event *event)
 	       in a menu (non-toolkit version).  */
 	    if (!toolkit_menubar_in_use (f))
 	      {
-		pixel_to_glyph_coords (f, XFIXNUM (event->x), XFIXNUM (event->y),
-				       &column, &row, NULL, 1);
+#if defined HAVE_WINDOW_SYSTEM
+		if (FRAME_WINDOW_P (f))
+		  {
+		    struct window *menu_w = XWINDOW (f->menu_bar_window);
+		    int x, y, dummy;
+
+		    x = FRAME_TO_WINDOW_PIXEL_X (menu_w, XFIXNUM (event->x));
+		    y = FRAME_TO_WINDOW_PIXEL_Y (menu_w, XFIXNUM (event->y));
+
+		    x_y_to_hpos_vpos (XWINDOW (f->menu_bar_window), x, y, &column, &row,
+				      NULL, NULL, &dummy);
+		  }
+		else
+#endif
+		  pixel_to_glyph_coords (f, XFIXNUM (event->x), XFIXNUM (event->y),
+					 &column, &row, NULL, 1);
 
 		/* In the non-toolkit version, clicks on the menu bar
 		   are ordinary button events in the event buffer.
diff --git a/src/xdisp.c b/src/xdisp.c
index f6a279636a..0c073c0fb5 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -2330,7 +2330,7 @@ pixel_to_glyph_coords (struct frame *f, int pix_x, int pix_y, int *x, int *y,
    text, or we can't tell because W's current matrix is not up to
    date.  */
 
-static struct glyph *
+struct glyph *
 x_y_to_hpos_vpos (struct window *w, int x, int y, int *hpos, int *vpos,
 		  int *dx, int *dy, int *area)
 {
-- 
2.38.1


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

-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: luangruo@yahoo.com,  59351@debbugs.gnu.org
> Date: Fri, 18 Nov 2022 18:23:18 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I thought you said XWINDOW (f->menu_bar_window) doesn't compile in the
> > "--without-x" build?  I think you need the HAVE_WINDOW_SYSTEM
> > condition as well.
> 
> This new one does compile "--without-x".

LGTM, thanks.

> About the height of the menu bar, you were right.  I've tested and
> display_menu_bar is called quite often and is called after a
> (set-face-font 'menu "Iosevka-18").  So maybe it is the last call to
> compute_line_metrics that does not do the required job.

I suggest to step through the code there and see why that happens.
Feel free to describe what you see if the reason is not obvious.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

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

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

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> I thought you said XWINDOW (f->menu_bar_window) doesn't compile in the
>> "--without-x" build?  I think you need the HAVE_WINDOW_SYSTEM
>> condition as well.
>
> This new one does compile "--without-x".
>
> About the height of the menu bar, you were right.  I've tested and
> display_menu_bar is called quite often and is called after a
> (set-face-font 'menu "Iosevka-18").  So maybe it is the last call to
> compute_line_metrics that does not do the required job.
>
> From 6fc191dcfa85ac98a482a7e373a4221499f86825 Mon Sep 17 00:00:00 2001
> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Fri, 18 Nov 2022 09:24:55 +0100
> Subject: [PATCH] Fix click position to menu bar entry with no-toolkit
>
> * src/keyboard.c (make_lispy_event): Use x_y_to_hpos_vpos to
> compute correct menu bar position should the menu face change.
> * src/xdisp.c (x_y_to_hpos_vpos): Not static anymore.
> * src/dispextern.h: Export x_y_to_hpos_vpos.
> ---
>  src/dispextern.h |  3 ++-
>  src/keyboard.c   | 18 ++++++++++++++++--
>  src/xdisp.c      |  2 +-
>  3 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/src/dispextern.h b/src/dispextern.h
> index 2f5f4335fe..2afbdeabaa 100644
> --- a/src/dispextern.h
> +++ b/src/dispextern.h
> @@ -3495,7 +3495,8 @@ #define TTY_CAP_STRIKE_THROUGH	0x20
>  extern void tty_draw_row_with_mouse_face (struct window *, struct glyph_row *,
>  					  int, int, enum draw_glyphs_face);
>  extern void display_tty_menu_item (const char *, int, int, int, int, bool);
> -
> +extern struct glyph *x_y_to_hpos_vpos (struct window *, int, int, int *, int *,
> +				       int *, int *, int *);
>  /* Flags passed to try_window.  */
>  #define TRY_WINDOW_CHECK_MARGINS	(1 << 0)
>  #define TRY_WINDOW_IGNORE_FONTS_CHANGE	(1 << 1)
> diff --git a/src/keyboard.c b/src/keyboard.c
> index 069cf0627b..6ce6ce17f2 100644
> --- a/src/keyboard.c
> +++ b/src/keyboard.c
> @@ -5974,8 +5974,22 @@ make_lispy_event (struct input_event *event)
>  	       in a menu (non-toolkit version).  */
>  	    if (!toolkit_menubar_in_use (f))
>  	      {
> -		pixel_to_glyph_coords (f, XFIXNUM (event->x), XFIXNUM (event->y),
> -				       &column, &row, NULL, 1);
> +#if defined HAVE_WINDOW_SYSTEM
> +		if (FRAME_WINDOW_P (f))
> +		  {
> +		    struct window *menu_w = XWINDOW (f->menu_bar_window);
> +		    int x, y, dummy;
> +
> +		    x = FRAME_TO_WINDOW_PIXEL_X (menu_w, XFIXNUM (event->x));
> +		    y = FRAME_TO_WINDOW_PIXEL_Y (menu_w, XFIXNUM (event->y));
> +
> +		    x_y_to_hpos_vpos (XWINDOW (f->menu_bar_window), x, y, &column, &row,
> +				      NULL, NULL, &dummy);
> +		  }
> +		else
> +#endif
> +		  pixel_to_glyph_coords (f, XFIXNUM (event->x), XFIXNUM (event->y),
> +					 &column, &row, NULL, 1);
>  
>  		/* In the non-toolkit version, clicks on the menu bar
>  		   are ordinary button events in the event buffer.
> diff --git a/src/xdisp.c b/src/xdisp.c
> index f6a279636a..0c073c0fb5 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -2330,7 +2330,7 @@ pixel_to_glyph_coords (struct frame *f, int pix_x, int pix_y, int *x, int *y,
>     text, or we can't tell because W's current matrix is not up to
>     date.  */
>  
> -static struct glyph *
> +struct glyph *
>  x_y_to_hpos_vpos (struct window *w, int x, int y, int *hpos, int *vpos,
>  		  int *dx, int *dy, int *area)
>  {
> -- 
> 2.38.1

LGTM.  I installed this and am closing this bug.  Thanks for working on
Emacs.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Po Lu <luangruo@yahoo.com> writes:

> LGTM.  I installed this and am closing this bug.  Thanks for working on
> Emacs.

Thanks!  And I see that you also improve it!
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> I suggest to step through the code there and see why that happens.
> Feel free to describe what you see if the reason is not obvious.

Hi Eli,

I'm trying to debug this from "M-x gdb".  I've put a breakpoint at
display_menu_bar but whenever I'm doing a 'next' at the init_iterator
call I get the following message:

--8<---------------cut here---------------start------------->8---
Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
_thread_sys_poll () at /tmp/-:3
3	/tmp/-: No such file or directory.
--8<---------------cut here---------------end--------------->8---

I guess that this is an issue with thread but maybe there is some tricks
to debug a threaded emacs.  I cannot find hindsights in "etc/DEBUG".  I
also tried to compile "--without-threads" but it seems to be for elisp
support.  How should I proceed?  (I understand that this starts to be a
tad off topic for a bug report so tell if should be moved elsewhere).
Thanks.
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,  luangruo@yahoo.com,
>   59351@debbugs.gnu.org
> Date: Mon, 21 Nov 2022 14:40:16 +0100
> 
> I'm trying to debug this from "M-x gdb".  I've put a breakpoint at
> display_menu_bar but whenever I'm doing a 'next' at the init_iterator
> call I get the following message:
> 
> --8<---------------cut here---------------start------------->8---
> Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
> _thread_sys_poll () at /tmp/-:3
> 3	/tmp/-: No such file or directory.
> --8<---------------cut here---------------end--------------->8---

What does "bt" show in this case?

> I guess that this is an issue with thread but maybe there is some tricks
> to debug a threaded emacs.  I cannot find hindsights in "etc/DEBUG".  I
> also tried to compile "--without-threads" but it seems to be for elisp
> support.

This has nothing to do with --without-threads, so no need to rebuild Emacs.
The only thing you need to make sure is that Emacs is build without
optimizations (-O0 compiler switch) and with -g3 (to include detailed debug
info including macros).

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,  luangruo@yahoo.com,
>>   59351@debbugs.gnu.org
>> Date: Mon, 21 Nov 2022 14:40:16 +0100
>> 
>> I'm trying to debug this from "M-x gdb".  I've put a breakpoint at
>> display_menu_bar but whenever I'm doing a 'next' at the init_iterator
>> call I get the following message:
>> 
>> --8<---------------cut here---------------start------------->8---
>> Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
>> _thread_sys_poll () at /tmp/-:3
>> 3	/tmp/-: No such file or directory.
>> --8<---------------cut here---------------end--------------->8---
>
> What does "bt" show in this case?

Here it is:
--8<---------------cut here---------------start------------->8---
#0  _thread_sys_poll () at /tmp/-:3
#1  0x00000a4800b10c4e in _libc_poll_cancel (fds=0x7f7ffffc52a8, nfds=1, timeout=-1) at /usr/src/lib/libc/sys/w_poll.c:27
#2  0x00000a4798da5532 in _xcb_conn_wait (c=0xa47977e9000, cond=<optimized out>, vector=0x0, count=0x0) at /usr/xenocara/lib/libxcb/libxcb/../../../dist/libxcb/src/xcb_conn.c:508
#3  0x00000a4798db7ad4 in wait_for_reply (c=0xa47977e9000, request=815, e=0x7f7ffffc53b8) at /usr/xenocara/lib/libxcb/libxcb/../../../dist/libxcb/src/xcb_in.c:522
#4  0x00000a4798db7bf5 in xcb_wait_for_reply64 (c=0xa47977e9000, request=815, e=0x7f7ffffc53b8) at /usr/xenocara/lib/libxcb/libxcb/../../../dist/libxcb/src/xcb_in.c:566
#5  0x00000a47866f6ac2 in _XReply () from /usr/X11R6/lib/libX11.so.18.0
#6  0x00000a47866d6814 in XGetWindowProperty () from /usr/X11R6/lib/libX11.so.18.0
#7  0x00000a4504dfe121 in x_handle_wm_state (f=0xa47cbcaf7b0, ie=0x7f7ffffc6c28) at xterm.c:18198
#8  0x00000a4504dd9801 in handle_one_xevent (dpyinfo=0xa47dab78080, event=0x7f7ffffc6cb8, finish=0x7f7ffffc6d7c, hold_quit=0x7f7ffffc6e08) at xterm.c:19096
#9  0x00000a4504e07b2f in XTread_socket (terminal=0xa47dab8c950, hold_quit=0x7f7ffffc6e08) at xterm.c:24751
#10 0x00000a4504e7f73c in gobble_input () at keyboard.c:7413
#11 0x00000a4504e80037 in handle_async_input () at keyboard.c:7644
#12 0x00000a4504e7fffe in process_pending_signals () at keyboard.c:7658
#13 0x00000a4504e800bb in unblock_input_to (level=0) at keyboard.c:7673
#14 0x00000a4504e7c650 in unblock_input () at keyboard.c:7692
#15 0x00000a450511f632 in ftcrfont_text_extents (font=0xa4767ecc8f8, code=0x7f7ffffc72e8, nglyphs=1, metrics=0xa45056fea78 <get_per_char_metric.metrics>) at ftcrfont.c:430
#16 0x00000a4504c7ffe7 in get_per_char_metric (font=0xa4767ecc8f8, char2b=0x7f7ffffc72e8) at xdisp.c:29566
#17 0x00000a4504c841d9 in gui_produce_glyphs (it=0x7f7ffffc7370) at xdisp.c:31736
#18 0x00000a4504c569eb in produce_special_glyphs (it=0x7f7ffffc8960, what=IT_TRUNCATION) at xdisp.c:31346
#19 0x00000a4504c54dac in init_iterator (it=0x7f7ffffc8960, w=0xa4767edf000, charpos=-1, bytepos=-1, row=0xa4767ec8000, base_face_id=MENU_FACE_ID) at xdisp.c:3314
#20 0x00000a4504cd4e8b in display_menu_bar (w=0xa47cbcafa20) at xdisp.c:26281
#21 0x00000a4504cc80ae in redisplay_window (window=XIL(0xa47cbcafa25), just_this_one_p=false) at xdisp.c:20374
#22 0x00000a4504cc2d7a in redisplay_window_0 (window=XIL(0xa47cbcafa25)) at xdisp.c:17397
#23 0x00000a4504fd67b6 in internal_condition_case_1 (bfun=0xa4504cc2d30 <redisplay_window_0>, arg=XIL(0xa47cbcafa25), handlers=XIL(0xa4736467f5b), hfun=0xa4504cc1160 <redisplay_window_error>) at eval.c:1498
#24 0x00000a4504cc0fe1 in redisplay_windows (window=XIL(0xa47cbcafa25)) at xdisp.c:17367
#25 0x00000a4504c6500b in redisplay_internal () at xdisp.c:16816
#26 0x00000a4504c70567 in redisplay () at xdisp.c:16006
#27 0x00000a4504e72d3f in read_char (commandflag=1, map=XIL(0xa47b29a34b3), prev_event=XIL(0), used_mouse_menu=0x7f7ffffd059f, end_time=0x0) at keyboard.c:2623
#28 0x00000a4504e6d0ea in read_key_sequence (keybuf=0x7f7ffffd0a90, prompt=XIL(0), dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=false) at keyboard.c:10070
#29 0x00000a4504e6aad6 in command_loop_1 () at keyboard.c:1376
#30 0x00000a4504fd667e in internal_condition_case (bfun=0xa4504e6a4a0 <command_loop_1>, handlers=XIL(0x90), hfun=0xa4504e6bcd0 <cmd_error>) at eval.c:1474
#31 0x00000a4504e6a430 in command_loop_2 (handlers=XIL(0x90)) at keyboard.c:1125
#32 0x00000a4504fd5413 in internal_catch (tag=XIL(0xfb10), func=0xa4504e6a400 <command_loop_2>, arg=XIL(0x90)) at eval.c:1197
#33 0x00000a4504e690e6 in command_loop () at keyboard.c:1103
#34 0x00000a4504e68e2d in recursive_edit_1 () at keyboard.c:712
#35 0x00000a4504e695d1 in Frecursive_edit () at keyboard.c:795
#36 0x00000a4504e6517b in main (argc=2, argv=0x7f7ffffd1238) at emacs.c:2516
--8<---------------cut here---------------end--------------->8---

>> I guess that this is an issue with thread but maybe there is some tricks
>> to debug a threaded emacs.  I cannot find hindsights in "etc/DEBUG".  I
>> also tried to compile "--without-threads" but it seems to be for elisp
>> support.
>
> This has nothing to do with --without-threads, so no need to rebuild Emacs.
> The only thing you need to make sure is that Emacs is build without
> optimizations (-O0 compiler switch) and with -g3 (to include detailed debug
> info including macros).

Yes, it is compiled with both -O0 and -g3 (and even
--enable-checking="yes,glyphs" and --enable-check-lisp-object-type: I
have followed etc/DEBUG on this).
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,  luangruo@yahoo.com,
>   59351@debbugs.gnu.org
> Date: Mon, 21 Nov 2022 15:46:51 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Manuel Giraud <manuel@ledu-giraud.fr>
> >> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,  luangruo@yahoo.com,
> >>   59351@debbugs.gnu.org
> >> Date: Mon, 21 Nov 2022 14:40:16 +0100
> >> 
> >> I'm trying to debug this from "M-x gdb".  I've put a breakpoint at
> >> display_menu_bar but whenever I'm doing a 'next' at the init_iterator
> >> call I get the following message:
> >> 
> >> --8<---------------cut here---------------start------------->8---
> >> Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
> >> _thread_sys_poll () at /tmp/-:3
> >> 3	/tmp/-: No such file or directory.
> >> --8<---------------cut here---------------end--------------->8---
> >
> > What does "bt" show in this case?
> 
> Here it is:
> --8<---------------cut here---------------start------------->8---
> #0  _thread_sys_poll () at /tmp/-:3
> #1  0x00000a4800b10c4e in _libc_poll_cancel (fds=0x7f7ffffc52a8, nfds=1, timeout=-1) at /usr/src/lib/libc/sys/w_poll.c:27
> #2  0x00000a4798da5532 in _xcb_conn_wait (c=0xa47977e9000, cond=<optimized out>, vector=0x0, count=0x0) at /usr/xenocara/lib/libxcb/libxcb/../../../dist/libxcb/src/xcb_conn.c:508
> #3  0x00000a4798db7ad4 in wait_for_reply (c=0xa47977e9000, request=815, e=0x7f7ffffc53b8) at /usr/xenocara/lib/libxcb/libxcb/../../../dist/libxcb/src/xcb_in.c:522

Hmm... that's strange.  Po Lu, any idea why we get SIGTRAP there?

Anyway, instead of stepping with "next", try this, after the breakpoint in
display_menu_bar breaks:

 (gdb) tbreak 26300
 (gdb) continue

If this works, and Emacs is stopped at line 26300, I'd suggest stepping
directly into compute_line_metrics, which is called at the end of
display_menu_bar:

 (gdb) tbreak compute_line_metrics
 (gdb) continue

This should stop inside compute_line_metrics, and then step through it and
take note of the various metrics of the glyph row that it uses to compute
the height.  You should see the metrics that correspond to the new font.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

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

Eli Zaretskii <eliz@gnu.org> writes:

> Hmm... that's strange.  Po Lu, any idea why we get SIGTRAP there?

No, sorry.  I'm not sure why SIGTRAP is sent there: Xlib is simply
waiting for a reply from a protocol request that was made.

I think it could be some left over debugging code somewhere outside
Emacs.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> About the height of the menu bar, you were right.  I've tested and
>> display_menu_bar is called quite often and is called after a
>> (set-face-font 'menu "Iosevka-18").  So maybe it is the last call to
>> compute_line_metrics that does not do the required job.
>
> I suggest to step through the code there and see why that happens.
> Feel free to describe what you see if the reason is not obvious.

Hi,

I think I might be onto something for the misbehaving update of the menu
bar height when changing font face (in --with-x-toolkit=no build).

The main difficulty was to set correct breakpoint because
display_menu_bar is called quite often.  So I'm starting my GDB session
with:

--8<---------------cut here---------------start------------->8---
set args -Q
break xdisp.c:3412 if ((base_face_id == MENU_FACE_ID) && (face->font->height > 14))
run
--8<---------------cut here---------------end--------------->8---

I think the previous 14 in the breakpoint should be adapt to not trigger
at the initial face and to do trigger after face font changed.  Then
from the scracth buffer of this emacs, I type: (set-face-font 'menu
"Iosevka-18") and evaluate it.  The breakpoint triggers as this font is
quite big.

Then from GDB, I do:

--8<---------------cut here---------------start------------->8---
tbreak 26337
continue
step
--8<---------------cut here---------------end--------------->8---

This last step goes into this compute_line_metrics call.  Everything
seems fine up to xdisp.c:22886.  Before this conditional, I have the
following values:

--8<---------------cut here---------------start------------->8---
row->y = 0
row->height = 31
row->visible_height = 31
max_y = 13
--8<---------------cut here---------------end--------------->8---

And so after it, row->visible_height becomes 13.  So maybe that is why
the menu bar is not update to have more height here.  WDYT?
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: luangruo@yahoo.com,  59351@debbugs.gnu.org
> Date: Wed, 23 Nov 2022 17:43:39 +0100
> 
> This last step goes into this compute_line_metrics call.  Everything
> seems fine up to xdisp.c:22886.  Before this conditional, I have the
> following values:
> 
> --8<---------------cut here---------------start------------->8---
> row->y = 0
> row->height = 31
> row->visible_height = 31
> max_y = 13
> --8<---------------cut here---------------end--------------->8---
> 
> And so after it, row->visible_height becomes 13.  So maybe that is why
> the menu bar is not update to have more height here.  WDYT?

Why do you think row->visible_height plays any role in this?  Maybe I'm
missing something, but it looks like it has nothing to do with the
non-resizing of the menu bar.  It looks like we never actually supported
this situation.  Think about it: in the no-toolkit build, the menu bar is
just a window.  A special kind of window, but a window nonetheless.  And
when a font changes, the dimensions of the window only change if we force
that, it isn't automatic.  If we don't force resizing, we keep the window at
its dimensions and redisplay the text inside, so fewer or more lines of text
will be visible in the window.

And that is what happens here: we just take note that the menu bar will be
partially visible, but don't do anything to resize the menu-bar window.

What I think we need to do is something like this:

  /* Compute the total height of the lines.  */
  compute_line_metrics (&it);
  if (FRAME_WINDOW_P (it.f))
    {
      struct glyph_row *row = it.glyph_row;
      if (row->y + row->height > WINDOW_BOX_HEIGHT_NO_MODE_LINE (w))
        {
	  FRAME_MENU_BAR_HEIGHT (it.f) = it.row->height;
          it.f->fonts_changed = true;
	}
    }

I hope that setting the frame's fonts_changed flag will cause Emacs resize
the menu-bar window.  You should see a call to adjust_frame_glyphs from
redisplay_internal; if that doesn't happen, perhaps we should force such a
call right after display_menu_bar returns.  Can you try this?

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> Why do you think row->visible_height plays any role in this?  Maybe I'm
> missing something, but it looks like it has nothing to do with the
> non-resizing of the menu bar.

Yes.  I think that I was misled by the name.

[...]

> What I think we need to do is something like this:
>
>   /* Compute the total height of the lines.  */
>   compute_line_metrics (&it);
>   if (FRAME_WINDOW_P (it.f))
>     {
>       struct glyph_row *row = it.glyph_row;
>       if (row->y + row->height > WINDOW_BOX_HEIGHT_NO_MODE_LINE (w))
>         {
> 	  FRAME_MENU_BAR_HEIGHT (it.f) = it.row->height;
>           it.f->fonts_changed = true;
> 	}
>     }
>
> I hope that setting the frame's fonts_changed flag will cause Emacs resize
> the menu-bar window.  You should see a call to adjust_frame_glyphs from
> redisplay_internal; if that doesn't happen, perhaps we should force such a
> call right after display_menu_bar returns.  Can you try this?

Ok.  I'll try this and report.  Thanks.
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

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

Hi,

I've come up with this solution that works for me.  Upon menu face
change, the menu bar is correctly updated.  WDYT?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-src-xdisp.c-display_menu_bar-Update-menu_bar_window-.patch --]
[-- Type: text/x-patch, Size: 1692 bytes --]

From 0ca358c52ed9641fdd16eed8db20475b619433dd Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 25 Nov 2022 15:50:41 +0100
Subject: [PATCH] ; * src/xdisp.c (display_menu_bar): Update menu_bar_window
 height.

---
 src/xdisp.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 5dcf21dc4c..14003ed5f6 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -26272,11 +26272,11 @@ display_menu_bar (struct window *w)
   it.first_visible_x = 0;
   it.last_visible_x = FRAME_PIXEL_WIDTH (f);
 #elif defined (HAVE_X_WINDOWS) /* X without toolkit.  */
+  struct window *menu_w;
   if (FRAME_WINDOW_P (f))
     {
       /* Menu bar lines are displayed in the desired matrix of the
 	 dummy window menu_bar_window.  */
-      struct window *menu_w;
       menu_w = XWINDOW (f->menu_bar_window);
       init_iterator (&it, menu_w, -1, -1, menu_w->desired_matrix->rows,
 		     MENU_FACE_ID);
@@ -26335,6 +26335,22 @@ display_menu_bar (struct window *w)
 
   /* Compute the total height of the lines.  */
   compute_line_metrics (&it);
+
+#if defined (HAVE_X_WINDOWS) && !defined (USE_X_TOOLKIT) && !defined (USE_GTK)
+  if (FRAME_WINDOW_P (it.f))
+    {
+      struct glyph_row *row = it.glyph_row;
+      int delta_height = ((row->y + row->height)
+			  - WINDOW_BOX_HEIGHT_NO_MODE_LINE (menu_w));
+      if (delta_height != 0)
+        {
+	  FRAME_MENU_BAR_HEIGHT (it.f) += delta_height;
+	  WINDOW_PIXEL_HEIGHT (w) -= delta_height;
+	  WINDOW_TOP_PIXEL_EDGE (w) += delta_height;
+          it.f->fonts_changed = true;
+	}
+    }
+#endif
 }
 
 /* Deep copy of a glyph row, including the glyphs.  */
-- 
2.38.1


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

-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

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

> I've come up with this solution that works for me.  Upon menu face
> change, the menu bar is correctly updated.  WDYT?

I spoke to fast.  It does not work correctly with more than one window
in the same frame.
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: luangruo@yahoo.com,  59351@debbugs.gnu.org
> Date: Fri, 25 Nov 2022 16:00:20 +0100
> 
> Manuel Giraud <manuel@ledu-giraud.fr> writes:
> 
> > I've come up with this solution that works for me.  Upon menu face
> > change, the menu bar is correctly updated.  WDYT?
> 
> I spoke to fast.  It does not work correctly with more than one window
> in the same frame.

The idea looks correct, thanks.  What exactly prevents this from working
with multiple windows on the same frame?

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> The idea looks correct, thanks.  What exactly prevents this from working
> with multiple windows on the same frame?

For instance, when vertically split (by C-x 3), the other window is not
correctly redrawn (text area is above menu bar).  Everything is then
fixed by the next unsplit/split.  Maybe, I should have use
adjust_frame_size here.
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: luangruo@yahoo.com,  59351@debbugs.gnu.org
> Date: Sat, 26 Nov 2022 18:26:34 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [...]
> 
> > The idea looks correct, thanks.  What exactly prevents this from working
> > with multiple windows on the same frame?
> 
> For instance, when vertically split (by C-x 3), the other window is not
> correctly redrawn (text area is above menu bar).  Everything is then
> fixed by the next unsplit/split.  Maybe, I should have use
> adjust_frame_size here.

In redisplay_internal, we have:

	      if (FRAME_VISIBLE_P (f) && !FRAME_OBSCURED_P (f))
		{
		  /* If fonts changed on visible frame, display again.  */
		  if (f->fonts_changed)
		    {
		      adjust_frame_glyphs (f);
		      /* Disable all redisplay optimizations for this
			 frame.  For the reasons, see the comment near
			 the previous call to adjust_frame_glyphs above.  */
		      SET_FRAME_GARBAGED (f);
		      f->fonts_changed = false;
		      goto retry_frame;
		    }

It sounds like this part is not being run in your case, so please try to
figure out why.  I think if the above code runs, what you want to do will
happen automatically.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: luangruo@yahoo.com,  59351@debbugs.gnu.org
>> Date: Sat, 26 Nov 2022 18:26:34 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> [...]
>> 
>> > The idea looks correct, thanks.  What exactly prevents this from working
>> > with multiple windows on the same frame?
>> 
>> For instance, when vertically split (by C-x 3), the other window is not
>> correctly redrawn (text area is above menu bar).  Everything is then
>> fixed by the next unsplit/split.  Maybe, I should have use
>> adjust_frame_size here.
>
> In redisplay_internal, we have:
>
> 	      if (FRAME_VISIBLE_P (f) && !FRAME_OBSCURED_P (f))
> 		{
> 		  /* If fonts changed on visible frame, display again.  */
> 		  if (f->fonts_changed)
> 		    {
> 		      adjust_frame_glyphs (f);
> 		      /* Disable all redisplay optimizations for this
> 			 frame.  For the reasons, see the comment near
> 			 the previous call to adjust_frame_glyphs above.  */
> 		      SET_FRAME_GARBAGED (f);
> 		      f->fonts_changed = false;
> 		      goto retry_frame;
> 		    }
>
> It sounds like this part is not being run in your case, so please try to
> figure out why.  I think if the above code runs, what you want to do will
> happen automatically.

Now, before adding more calls to adjust_frame_size, please wait for
Emacs 29 to be cut first.  That function tends to interact with X window
managers in different ways on builds with different toolkits, so adding
or removing calls to it is very likely to break stuff.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,  59351@debbugs.gnu.org
> Date: Sun, 27 Nov 2022 08:51:09 +0800
> 
> Now, before adding more calls to adjust_frame_size, please wait for
> Emacs 29 to be cut first.  That function tends to interact with X window
> managers in different ways on builds with different toolkits, so adding
> or removing calls to it is very likely to break stuff.

We are talking about fixing a bug, albeit probably a very old one.  We could
condition the added code so that only non-toolkit X builds run it, at least
on the release branch, if you are uneasy with doing that with other
toolkits.  Would that be good enough?

I do want to try to solve this for Emacs 29, if possible.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

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

Hi,

Here is a new version of this patch that tries to adapt the menu bar
height in the non-toolkit build.

FWIW, I've also attached a quick video of how it works with the fvwm
window manager with an emacs evaluating the following code:
--8<---------------cut here---------------start------------->8---
(split-window-right)
(info-emacs-manual)
(dolist (font '("Iosevka-18" "Ttyp0-12" "Inconsolata-14" "Inconsolata-32"
		"Noto Sans-6"))
  (sit-for 2)
  (set-face-font 'menu font))
--8<---------------cut here---------------end--------------->8---

It also works as intended with EXWM.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-src-xdisp.c-display_menu_bar-Update-menu_bar_window-.patch --]
[-- Type: text/x-patch, Size: 1719 bytes --]

From 075e88eb94474a863e928db388b7af8adbeba038 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 25 Nov 2022 15:50:41 +0100
Subject: [PATCH] ; * src/xdisp.c (display_menu_bar): Update menu_bar_window
 height.

---
 src/xdisp.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 5dcf21dc4c..3d14914600 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -26272,11 +26272,11 @@ display_menu_bar (struct window *w)
   it.first_visible_x = 0;
   it.last_visible_x = FRAME_PIXEL_WIDTH (f);
 #elif defined (HAVE_X_WINDOWS) /* X without toolkit.  */
+  struct window *menu_w;
   if (FRAME_WINDOW_P (f))
     {
       /* Menu bar lines are displayed in the desired matrix of the
 	 dummy window menu_bar_window.  */
-      struct window *menu_w;
       menu_w = XWINDOW (f->menu_bar_window);
       init_iterator (&it, menu_w, -1, -1, menu_w->desired_matrix->rows,
 		     MENU_FACE_ID);
@@ -26335,6 +26335,22 @@ display_menu_bar (struct window *w)
 
   /* Compute the total height of the lines.  */
   compute_line_metrics (&it);
+
+#if defined (HAVE_X_WINDOWS) && !defined (USE_X_TOOLKIT) && !defined (USE_GTK)
+  /* With the non-toolkit version, modify the menu bar window height
+     accordingly.  */
+  if (FRAME_WINDOW_P (it.f))
+    {
+      struct glyph_row *row = it.glyph_row;
+      int delta_height = ((row->y + row->height)
+			  - WINDOW_BOX_HEIGHT_NO_MODE_LINE (menu_w));
+      if (delta_height != 0)
+        {
+	  FRAME_MENU_BAR_HEIGHT (it.f) += delta_height;
+	  adjust_frame_size (it.f, -1, -1, 3, false, Qmenu_bar_lines);
+	}
+    }
+#endif
 }
 
 /* Deep copy of a glyph row, including the glyphs.  */
-- 
2.38.1


[-- Attachment #3: video.mp4 --]
[-- Type: video/mp4, Size: 492081 bytes --]

[-- Attachment #4: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

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

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

> Hi,
>
> Here is a new version of this patch that tries to adapt the menu bar
> height in the non-toolkit build.
>
> FWIW, I've also attached a quick video of how it works with the fvwm
> window manager with an emacs evaluating the following code:
>
> (split-window-right)
> (info-emacs-manual)
> (dolist (font '("Iosevka-18" "Ttyp0-12" "Inconsolata-14" "Inconsolata-32"
> 		"Noto Sans-6"))
>   (sit-for 2)
>   (set-face-font 'menu font))
>
> It also works as intended with EXWM.

If it works for you, please install this on the `master' branch (NOT
emacs-29!)

Does the menu bar wrap? If it does, then the code in keyboard.c has to
be adjusted as well.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Po Lu <luangruo@yahoo.com> writes:

[...]

> If it works for you, please install this on the `master' branch (NOT
> emacs-29!)

I do not think I have rights to install on master (or emacs-29 BTW).
But master is ok for me: it is not really important if it does not make
it into emacs-29.

> Does the menu bar wrap? If it does, then the code in keyboard.c has to
> be adjusted as well.

You mean does it wrap on multiple lines?  Then, no it does not.
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

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

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

> I do not think I have rights to install on master (or emacs-29 BTW).
> But master is ok for me: it is not really important if it does not make
> it into emacs-29.

OK, sure.  I will install it tomorrow, but please remind me if I forget.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  59351@debbugs.gnu.org
> Date: Tue, 29 Nov 2022 08:47:23 +0800
> 
> Manuel Giraud <manuel@ledu-giraud.fr> writes:
> 
> > Hi,
> >
> > Here is a new version of this patch that tries to adapt the menu bar
> > height in the non-toolkit build.
> >
> > FWIW, I've also attached a quick video of how it works with the fvwm
> > window manager with an emacs evaluating the following code:
> >
> > (split-window-right)
> > (info-emacs-manual)
> > (dolist (font '("Iosevka-18" "Ttyp0-12" "Inconsolata-14" "Inconsolata-32"
> > 		"Noto Sans-6"))
> >   (sit-for 2)
> >   (set-face-font 'menu font))
> >
> > It also works as intended with EXWM.
> 
> If it works for you, please install this on the `master' branch (NOT
> emacs-29!)

Why not emacs-29?  This fixes a bug, albeit an old one, and the conditions
are (or supposed to be) such that the new code only affects X builds with no
toolkits.  So what is the danger you see in installing this on emacs-29?

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

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

Eli Zaretskii <eliz@gnu.org> writes:

> Why not emacs-29?  This fixes a bug, albeit an old one, and the conditions
> are (or supposed to be) such that the new code only affects X builds with no
> toolkits.  So what is the danger you see in installing this on emacs-29?

Fiddling with the code behind the various bars (or any other code that
can cause the size of a frame to change) tends to introduce regressions
in interactions between Emacs and window managers, and there are too
many window managers and build configurations to test in a period of 6
to 8 months.  As such, I'd be really uncomfortable with installing that
change on the release branch, especially since most people are not using
the no toolkit build.

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Po Lu <luangruo@yahoo.com> writes:

> OK, sure.  I will install it tomorrow, but please remind me if I
> forget.

Ping (i guess :-)
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Po Lu <luangruo@yahoo.com>,  59351@debbugs.gnu.org
> Date: Mon, 28 Nov 2022 18:07:17 +0100
> 
> Here is a new version of this patch that tries to adapt the menu bar
> height in the non-toolkit build.

Thanks, I installed this.  But please look at the modified log message I
committed and try to follow it in the future.  In particular:

  . fixes of real bugs (as opposed to typos and such) should appear in
    ChangeLog, so starting with ";" in inappropriate in such cases
  . always mention the bug number in the log message

Should this bug be closed now?

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> Should this bug be closed now?

Ok for me.
-- 
Manuel Giraud

bug#59351: 29.0.50; [PATCH] Fix mouse click position to menu bar entry

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,  luangruo@yahoo.com,
>   59351@debbugs.gnu.org
> Date: Thu, 01 Dec 2022 17:23:54 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [...]
> 
> > Should this bug be closed now?
> 
> Ok for me.

Thanks, closing.