bug#43756: Fix for TTY menus mouse interaction

bug#43756: Fix for TTY menus mouse interaction

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

[-- Attachment #1.1: Type: text/plain, Size: 485 bytes --]

Attached is a simple bugfix for tmm.el in Emacs. Currently when the top
level menu is displayed (like when pressing F10), the menu entries are
not in the same order as they are displayed in the menu bar.
Additionally, clicking on any menu entry other than "Help" jumps
straight into the menu clicked on, but clicking on "Help" displays the
root menu. 

Let me know if you need more details to reproduce the issues. In this
case, I think the issues fixed are straightforward. 

  -- MJF

[-- Attachment #1.2: Type: text/html, Size: 692 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fixing-small-bugs-in-tmm.el.patch --]
[-- Type: text/x-diff; name=0001-Fixing-small-bugs-in-tmm.el.patch, Size: 1354 bytes --]

From 836ae1b4e2529cba5af52e270be7dff764abbc9e Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Thu, 17 Sep 2020 22:37:00 -0700
Subject: [PATCH] Fixing small bugs in tmm.el.

* `tmm-menubar-keymap' now properly sorts final items.
* `tmm-menubar' allows clicks on the final menu item.
---
 lisp/tmm.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/tmm.el b/lisp/tmm.el
index e9f3f5b038..1e18c8b4ae 100644
--- a/lisp/tmm.el
+++ b/lisp/tmm.el
@@ -57,7 +57,7 @@ tmm-menubar-keymap
                  menu-end
                menu-bar)))
      (tmm-get-keybind [menu-bar]))
-    `(keymap ,@(nreverse menu-bar) ,@(nreverse menu-end))))
+    `(keymap ,@(nreverse menu-bar) ,@menu-end)))
 
 ;;;###autoload (define-key global-map "\M-`" 'tmm-menubar)
 ;;;###autoload (define-key global-map [menu-bar mouse-1] 'tmm-menubar-mouse)
@@ -96,7 +96,10 @@ tmm-menubar
                                          (or (null visible)
                                              (eval visible)))))))
                   (setq column (+ column (length name) 1)))))
-             menu-bar))))
+             menu-bar)
+            ;; Check the last menu item.
+            (when (> column x-position)
+              (setq menu-bar-item prev-key)))))
     (tmm-prompt menu-bar nil menu-bar-item)))
 
 ;;;###autoload
-- 
2.20.1

bug#43756: Fix for TTY menus mouse interaction

[Lars Ingebrigtsen]

Jared Finder <jared@finder.org> writes:

> Let me know if you need more details to reproduce the issues. In this
> case, I think the issues fixed are straightforward.

Yes, a recipe for reproducing this problem would be nice (including what
you're seeing and what you're expecting to see), starting from "emacs -Q".

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43756: Fix for TTY menus mouse interaction

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

On 2020-10-02 7:37 am, Lars Ingebrigtsen wrote:
> Jared Finder <jared@finder.org> writes:
> 
>> Let me know if you need more details to reproduce the issues. In this
>> case, I think the issues fixed are straightforward.
> 
> Yes, a recipe for reproducing this problem would be nice (including 
> what
> you're seeing and what you're expecting to see), starting from "emacs 
> -Q".

Environment:

* Emacs repo from git://git.sv.gnu.org/emacs.git, commit 
f6277911eb2c520aec8f0efd80c91999226e3322
* Run on Debian under Windows Subsystem for Linux 2 (the VM version). 
I've also seen this on MacOS and native Win32 builds of Emacs.

Steps to reproduce:

emacs -Q
M-x ielm
M-x xterm-mouse-mode
Click on File in the menu bar.
--> observe that you get the File menu displayed via tmm.el
Click on Edit, Options, Buffers, Tools, IELM in the menu bar.
--> observe that you get the menu clicked on displayed via tmm.el.

Now the buggy parts:

Click on the "C" in Complete in the menu bar.
--> The Help menu displays. I expect the Complete menu to display.
Click on the "I" in In/Out in the menu bar.
--> The Signals menu displays. I expect the In/Out menu to display.
Click on the "H" in Help in the menu bar.
--> The root menu displays. I expect the Help menu to display.
Run M-x tmm-menubar
--> The menu listed is in the order File, Edit, Options, Buffers, Tools, 
IELM, Help, Signals, In/Out, Complete. I expect the menu to be displayed 
in the same order as the menu bar display.

   -- MJF

bug#43756: Fix for TTY menus mouse interaction

[Eli Zaretskii]

> Cc: 43756@debbugs.gnu.org
> Date: Fri, 02 Oct 2020 09:45:33 -0700
> From: Jared Finder via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> * Emacs repo from git://git.sv.gnu.org/emacs.git, commit 
> f6277911eb2c520aec8f0efd80c91999226e3322
> * Run on Debian under Windows Subsystem for Linux 2 (the VM version). 
> I've also seen this on MacOS and native Win32 builds of Emacs.

Native Windows build doesn't support xterm-mouse, and the native mouse
clicks work correctly there.  If you have a recipe for the native
Windows build that shows incorrect menus popping down, please show
that.

> emacs -Q
> M-x ielm
> M-x xterm-mouse-mode

Ah, xterm-mouse-mode.  AFAIR, no one has made TTY menus work with
xterm-mouse-mode.  The first thing to do is to disable tmm-menubar,
and then you need to cause a mouse click call menu-bar-open with the
2nd argument set to the X coordinate of the click.

bug#43756: Fix for TTY menus mouse interaction

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

On 2020-10-02 12:35 pm, Eli Zaretskii wrote:
>> Cc: 43756@debbugs.gnu.org
>> Date: Fri, 02 Oct 2020 09:45:33 -0700
>> From: Jared Finder via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> * Emacs repo from git://git.sv.gnu.org/emacs.git, commit
>> f6277911eb2c520aec8f0efd80c91999226e3322
>> * Run on Debian under Windows Subsystem for Linux 2 (the VM version).
>> I've also seen this on MacOS and native Win32 builds of Emacs.
> 
> Native Windows build doesn't support xterm-mouse, and the native mouse
> clicks work correctly there.  If you have a recipe for the native
> Windows build that shows incorrect menus popping down, please show
> that.

On Windows builds, only the M-x tmm-menubar step is broken, with the 
order in the displayed menu bar being different then in the tmm-menubar 
popup. This is *only* about text based menus, not the native platform 
ones.

>> emacs -Q
>> M-x ielm
>> M-x xterm-mouse-mode
> 
> Ah, xterm-mouse-mode.  AFAIR, no one has made TTY menus work with
> xterm-mouse-mode.  The first thing to do is to disable tmm-menubar,
> and then you need to cause a mouse click call menu-bar-open with the
> 2nd argument set to the X coordinate of the click.

I think this may be getting mixed up with my feature proposal on 
emacs-devel? I did find this bug when working on making the TTY menus 
work with xterm-mouse-mode and they both affect menus. The patches I 
attached in that thread follow the pattern you described.

This bug report is separable from the rest of that feature. It's also 
much smaller, just two lines. :) This fixes the current behavior of the 
command tmm-menubar-mouse, which is bound to <menu-bar> <mouse-1>.

   -- MJF

bug#43756: Fix for TTY menus mouse interaction

[Eli Zaretskii]

> Date: Fri, 02 Oct 2020 12:54:54 -0700
> From: Jared Finder <jared@finder.org>
> Cc: larsi@gnus.org, 43756@debbugs.gnu.org
> 
> > Ah, xterm-mouse-mode.  AFAIR, no one has made TTY menus work with
> > xterm-mouse-mode.  The first thing to do is to disable tmm-menubar,
> > and then you need to cause a mouse click call menu-bar-open with the
> > 2nd argument set to the X coordinate of the click.
> 
> I think this may be getting mixed up with my feature proposal on 
> emacs-devel? I did find this bug when working on making the TTY menus 
> work with xterm-mouse-mode and they both affect menus. The patches I 
> attached in that thread follow the pattern you described.
> 
> This bug report is separable from the rest of that feature. It's also 
> much smaller, just two lines. :) This fixes the current behavior of the 
> command tmm-menubar-mouse, which is bound to <menu-bar> <mouse-1>.

So you are saying that when tmm-menubar-mouse is invoked by mouse
clicks, it shows incorrect menus after "M-x ielm", but only if you
click on the menu items specific to IELM?  It sounds like tmm-menubar
has a bug in its translation of the X coordinate of the click to the
menu-bar item, perhaps because it considers only the global menu
keymap.

bug#43756: Fix for TTY menus mouse interaction

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

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

On 2020-10-03 12:23 am, Eli Zaretskii wrote:
>> Date: Fri, 02 Oct 2020 12:54:54 -0700
>> From: Jared Finder <jared@finder.org>
>> Cc: larsi@gnus.org, 43756@debbugs.gnu.org
>> 
>> > Ah, xterm-mouse-mode.  AFAIR, no one has made TTY menus work with
>> > xterm-mouse-mode.  The first thing to do is to disable tmm-menubar,
>> > and then you need to cause a mouse click call menu-bar-open with the
>> > 2nd argument set to the X coordinate of the click.
>> 
>> I think this may be getting mixed up with my feature proposal on
>> emacs-devel? I did find this bug when working on making the TTY menus
>> work with xterm-mouse-mode and they both affect menus. The patches I
>> attached in that thread follow the pattern you described.
>> 
>> This bug report is separable from the rest of that feature. It's also
>> much smaller, just two lines. :) This fixes the current behavior of 
>> the
>> command tmm-menubar-mouse, which is bound to <menu-bar> <mouse-1>.
> 
> So you are saying that when tmm-menubar-mouse is invoked by mouse
> clicks, it shows incorrect menus after "M-x ielm", but only if you
> click on the menu items specific to IELM?  It sounds like tmm-menubar
> has a bug in its translation of the X coordinate of the click to the
> menu-bar item, perhaps because it considers only the global menu
> keymap.

I think the issue is that the ordering of elements in 
menu-bar-final-items controls the order of the display menu bar, but 
tmm-menubar-keymap does not currently take that into account. It was 
pure luck that my first patch fixed anything. :)

Attached is a new patch and more detailed repro steps.

First, the ordering bug (the changes inside tmm-menubar-keymap):

Environment:

* Emacs repo from git://git.sv.gnu.org/emacs.git, commit 
f6277911eb2c520aec8f0efd80c91999226e3322
* Run on Debian under Windows Subsystem for Linux 2 (the VM version). 
I've also seen this on MacOS and native Win32 builds of Emacs.

Steps to reproduce:

emacs -Q
M-x load-library <RET> tmm <RET>

Defining the following stripped down version of tmm-menubar makes it 
easier to visually compare the displayed menu bar with what tmm-menubar 
would show:

(defun menubar-items ()
   "Simplified version of tmm-menubar."
   (let ((column 0)
         (menu-bar (tmm-menubar-keymap))
         list)
     (catch 'done
       (map-keymap
        (lambda (key binding)
          (pcase binding
            ((or `(,(and (pred stringp) name) . ,_) ;Simple menu item.
                 `(menu-item ,name ,_cmd            ;Extended menu item.
                             . ,(and props
                                     (guard (let ((visible
                                                   (plist-get props 
:visible)))
                                              (or (null visible)
                                                  (eval visible)))))))
             (push (cons name column) list)
             (setq column (+ column (length name) 1)))))
        menu-bar)
     (nreverse list))))

M-x ielm

Eval (menubar-items)
--> observe that the ordering of menu items returned is not the same as 
the displayed menu bar.
--> With my patch this is correct.

Eval (setf menu-bar-final-items '(help-menu completion signals inout))
Force redisplay via C-l
--> observe that the ordering of menu items in the displayed menu bar 
has changed.

Eval (menubar-items)
--> observe that the ordering of menu items returned has not changed.
--> With my patch this is correct.

Repeat above steps for other values of menu-bar-final-items. You can add 
'file to the list, set it to nil, etc.


The click bug is a simple fix in tmm-menubar since the lambda passed in 
to map-keymap always checks against the previous menu item (e.g. 
prev-key), therefore missing the last item. Repro steps are same as 
before.

   -- MJF

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fixing-small-bugs-in-tmm.el.patch --]
[-- Type: text/x-diff; name=0001-Fixing-small-bugs-in-tmm.el.patch, Size: 2812 bytes --]

From 12d3bfc01d6632fb8a903dd757ee9574da96e420 Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Thu, 17 Sep 2020 22:37:00 -0700
Subject: [PATCH 1/3] Fixing small bugs in tmm.el.

* `tmm-menubar-keymap' now properly sorts final items.
* `tmm-menubar' allows clicks on the final menu item.
---
 lisp/tmm.el    | 23 +++++++++++++++--------
 src/keyboard.c |  3 ++-
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/lisp/tmm.el b/lisp/tmm.el
index e9f3f5b038..074ee7593f 100644
--- a/lisp/tmm.el
+++ b/lisp/tmm.el
@@ -50,14 +50,18 @@ tmm-menubar-keymap
         (menu-end '()))
     (map-keymap
      (lambda (key binding)
-       (push (cons key binding)
-             ;; If KEY is the name of an item that we want to put last,
-             ;; move it to the end.
-             (if (memq key menu-bar-final-items)
-                 menu-end
-               menu-bar)))
+       (let ((pos (seq-position menu-bar-final-items key))
+             (menu-item (cons key binding)))
+         (if pos
+             ;; If KEY is the name of an item that we want to put
+             ;; last, store it separately with explicit ordering for
+             ;; sorting.
+             (push (cons pos menu-item) menu-end)
+           (push menu-item menu-bar))))
      (tmm-get-keybind [menu-bar]))
-    `(keymap ,@(nreverse menu-bar) ,@(nreverse menu-end))))
+    `(keymap ,@(nreverse menu-bar)
+             ,@(mapcar 'cdr (sort menu-end
+                                  (lambda (a b) (< (car a) (car b))))))))
 
 ;;;###autoload (define-key global-map "\M-`" 'tmm-menubar)
 ;;;###autoload (define-key global-map [menu-bar mouse-1] 'tmm-menubar-mouse)
@@ -96,7 +100,10 @@ tmm-menubar
                                          (or (null visible)
                                              (eval visible)))))))
                   (setq column (+ column (length name) 1)))))
-             menu-bar))))
+             menu-bar)
+            ;; Check the last menu item.
+            (when (> column x-position)
+              (setq menu-bar-item prev-key)))))
     (tmm-prompt menu-bar nil menu-bar-item)))
 
 ;;;###autoload
diff --git a/src/keyboard.c b/src/keyboard.c
index af075a42c7..ea5f961304 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -12106,7 +12106,8 @@ syms_of_keyboard (void)
 
   DEFVAR_LISP ("menu-bar-final-items", Vmenu_bar_final_items,
 	       doc: /* List of menu bar items to move to the end of the menu bar.
-The elements of the list are event types that may have menu bar bindings.  */);
+The order of this list controls the order of the items. The elements of the list
+are event types that may have menu bar bindings.  */);
   Vmenu_bar_final_items = Qnil;
 
   DEFVAR_LISP ("tab-bar-separator-image-expression", Vtab_bar_separator_image_expression,
-- 
2.20.1

bug#43756: Fix for TTY menus mouse interaction

[Lars Ingebrigtsen]

Jared Finder <jared@finder.org> writes:

> Attached is a new patch and more detailed repro steps.

Thanks; with that recipe I could reproduce the bugs here with emacs -Q
-nw, and your patch fixes the problem, so I've applied it to Emacs 28.

It was small enough to apply without a copyright assignment (but I
forgot to put in a copyright exempt marker again *sigh*).  If you think
it's possible that you'll submit further patches in the future, it might
be a good idea to do the copyright assignment process now -- would you
be willing to do so?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43756: Fix for TTY menus mouse interaction

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

On 2020-10-05 1:24 am, Lars Ingebrigtsen wrote:
> Jared Finder <jared@finder.org> writes:
> 
>> Attached is a new patch and more detailed repro steps.
> 
> Thanks; with that recipe I could reproduce the bugs here with emacs -Q
> -nw, and your patch fixes the problem, so I've applied it to Emacs 28.
> 
> It was small enough to apply without a copyright assignment (but I
> forgot to put in a copyright exempt marker again *sigh*).  If you think
> it's possible that you'll submit further patches in the future, it 
> might
> be a good idea to do the copyright assignment process now -- would you
> be willing to do so?

Yay! Happy to sign a copyright assignment. Let's start that process.

   -- MJF

bug#43756: Fix for TTY menus mouse interaction

[Eli Zaretskii]

> Date: Mon, 05 Oct 2020 07:53:17 -0700
> From: Jared Finder <jared@finder.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, 43756@debbugs.gnu.org
> 
> Yay! Happy to sign a copyright assignment. Let's start that process.

Form sent off-list.