bug#50067: Context menus

bug#50067: Context menus

[Juri Linkov]

The branch 'feature/context-menu' is ready for merging to master.

It was created after the discussion in
https://lists.gnu.org/archive/html/emacs-devel/2021-07/msg00300.html
as a proof-of-concept.

And after testing with different modes, it proved to be flexible enough
to support various needs.

After merging it could be improved further with more development in master.

The branch contains a NEWS entry and changes in the documentation.

bug#50067: Context menus

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> The branch 'feature/context-menu' is ready for merging to master.
>
> It was created after the discussion in
> https://lists.gnu.org/archive/html/emacs-devel/2021-07/msg00300.html
> as a proof-of-concept.
>
> And after testing with different modes, it proved to be flexible enough
> to support various needs.
>
> After merging it could be improved further with more development in master.

I haven't tested the branch, but reading the diff, it looks like an
excellent feature to me.  Looking at the implementation of the stuff in
various modes, I'm wondering whether the interface should perhaps be
tweaked a bit:

+(defun eww-context-menu (menu)
[...]
+  (when (or (mouse-posn-property (event-start last-input-event) 'shr-url)
+            (mouse-posn-property (event-start last-input-event) 'image-url))
+    (define-key menu [shr-mouse-browse-url-new-window]

Perhaps the signature of the context menu functions should be:

+(defun eww-context-menu (menu event)

?

I'm also wondering whether we should add a `context-menu' text property.

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

bug#50067: Context menus

[Juri Linkov]

> I haven't tested the branch, but reading the diff, it looks like an
> excellent feature to me.  Looking at the implementation of the stuff in
> various modes, I'm wondering whether the interface should perhaps be
> tweaked a bit:
>
> +(defun eww-context-menu (menu)
> [...]
> +  (when (or (mouse-posn-property (event-start last-input-event) 'shr-url)
> +            (mouse-posn-property (event-start last-input-event) 'image-url))
> +    (define-key menu [shr-mouse-browse-url-new-window]
>
> Perhaps the signature of the context menu functions should be:
>
> +(defun eww-context-menu (menu event)
>
> ?

To get an event would be nice, but I see no way to do this.
The top function 'context-menu-map' is called by:

  `(menu-item ,(purecopy "Context Menu") ignore
              :filter (lambda (_) (context-menu-map))))

that has no access to the event - an unused argument of lambda above
is just the binding that is 'ignore' in this case.

> I'm also wondering whether we should add a `context-menu' text property.

As soon as such a need arises, a text property could be added as well.
But it seems currently much cleaner is to use a single context-menu
function for every mode.

bug#50067: Context menus

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

>> +(defun eww-context-menu (menu)
>> [...]
>> +  (when (or (mouse-posn-property (event-start last-input-event) 'shr-url)
>> +            (mouse-posn-property (event-start last-input-event) 'image-url))
>> +    (define-key menu [shr-mouse-browse-url-new-window]

[...]

> To get an event would be nice, but I see no way to do this.
> The top function 'context-menu-map' is called by:
>
>   `(menu-item ,(purecopy "Context Menu") ignore
>               :filter (lambda (_) (context-menu-map))))
>
> that has no access to the event - an unused argument of lambda above
> is just the binding that is 'ignore' in this case.

Could just use `last-input-event', I guess?  But that doesn't really
give us anything better than what we have, so there's probably no point.

>> I'm also wondering whether we should add a `context-menu' text property.
>
> As soon as such a need arises, a text property could be added as well.
> But it seems currently much cleaner is to use a single context-menu
> function for every mode.

Right.  I was thinking that it's pretty likely that all button-like
things are going to grow a context menu, but we can add `context-menu'
later if that turns out to be the case.

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

bug#50067: Context menus

[Juri Linkov]

tags 50067 fixed
close 50067 28.0.50
quit

>>> I'm also wondering whether we should add a `context-menu' text property.
>>
>> As soon as such a need arises, a text property could be added as well.
>> But it seems currently much cleaner is to use a single context-menu
>> function for every mode.
>
> Right.  I was thinking that it's pretty likely that all button-like
> things are going to grow a context menu, but we can add `context-menu'
> later if that turns out to be the case.

It would be easier to make more experiments with code in master,
so now merged the branch to master.

bug#50067: Context menus

[Tak Kunihiro]

> It would be easier to make more experiments with code in master,
> so now merged the branch to master.

I see the context menu after M-x context-menu-mode.  Thank you for
working on this.

I will see how it works and try to give feedback soon.

bug#50067: Context menus

[Juri Linkov]

>> It would be easier to make more experiments with code in master,
>> so now merged the branch to master.
>
> I see the context menu after M-x context-menu-mode.  Thank you for
> working on this.

Thank you for the great idea of using a list of functions.

> I will see how it works and try to give feedback soon.

Waiting for your feedback.

bug#50067: Context menus

[Naoya Yamashita]

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

Hi.  I'm one of the users of Emacs-jp.  Tak introduced us to this
thread and it got me interested, I'm sending this Email.  This
mail thread is huge and I haven't read all of it, so I'm sorry if
I misread the context.

I've created a context menu for ispell (referencing `context-menu-ffap`).
You may find some inspiration from this.

    (defun context-menu-ispell (menu)
      "Ispell at point menu."
      (when t ;; (ffap-guess-file-name-at-point)
        (define-key menu [ispell-separator] menu-bar-separator)
        (define-key menu [ispell-at-mouse]
          '(menu-item "Check spelling of word" ispell-word
                      :help "Check spelling of word under or before the
cursor.")))
      menu)

Regards,
Naoya.

[-- Attachment #2: Type: text/html, Size: 943 bytes --]

bug#50067: Context menus

[Juri Linkov]

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

> Hi.  I'm one of the users of Emacs-jp.  Tak introduced us to this
> thread and it got me interested, I'm sending this Email.  This
> mail thread is huge and I haven't read all of it, so I'm sorry if
> I misread the context.
>
> I've created a context menu for ispell (referencing `context-menu-ffap`).
> You may find some inspiration from this.
>
>     (defun context-menu-ispell (menu)
>       "Ispell at point menu."
>       (when t ;; (ffap-guess-file-name-at-point)
>         (define-key menu [ispell-separator] menu-bar-separator)
>         (define-key menu [ispell-at-mouse]
>           '(menu-item "Check spelling of word" ispell-word
>                       :help "Check spelling of word under or before the cursor.")))
>       menu)

Thanks for the suggestion.  I think such menus should be added to their
respective packages.  context-menu-ispell could be added to ispell.el
(when flyspell is unavailable).

Then dictionary.el could provide own context menu too:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: context-menu-dictionary.patch --]
[-- Type: text/x-diff, Size: 1102 bytes --]

diff --git a/lisp/net/dictionary.el b/lisp/net/dictionary.el
index f33cbaf112..7a84f9978f 100644
--- a/lisp/net/dictionary.el
+++ b/lisp/net/dictionary.el
@@ -1368,5 +1368,27 @@ global-dictionary-tooltip-mode
                     (if on #'dictionary-tooltip-track-mouse #'ignore))
     on))
 
+(defun dictionary-search-word-at-mouse (event)
+  (interactive "e")
+  (let ((word (save-window-excursion
+		(save-excursion
+		  (mouse-set-point event)
+		  (current-word)))))
+    (selected-window)
+    (dictionary-search word)))
+
+(defun context-menu-dictionary (menu)
+  "Dictionary word at point menu."
+  (save-excursion
+    (mouse-set-point last-input-event)
+    (when (thing-at-point 'word)
+      (define-key menu [dictionary-separator] menu-bar-separator)
+      (define-key menu [dictionary-search-word-at-mouse]
+        '(menu-item "Dictionary Search" dictionary-search-word-at-mouse
+                    :help "Search the word at mouse click in dictionary"))))
+  menu)
+
+(add-hook 'context-menu-functions 'context-menu-dictionary 15)
+
 (provide 'dictionary)
 ;;; dictionary.el ends here

bug#50067: Context menus

[Juri Linkov]

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

Other programs don't show the keys in context menus.
The Human Interface Guidelines say:

  Show keyboard shortcuts in menu bar menus, not contextual menus.
  Contextual menus are already shortcuts to task-specific commands;
  it's redundant to display keyboard shortcuts too.

This patch hides all keys from the context menus:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: inhibit-menu-keys.patch --]
[-- Type: text/x-diff, Size: 3680 bytes --]

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 5f3db46516..2d9b1c8f0b 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -314,7 +314,9 @@ context-menu-map
 it overrides all functions from `context-menu-functions'.
 At the end, it's possible to modify the final menu by specifying
 the function `context-menu-filter-function'."
-  (let* ((menu (make-sparse-keymap (propertize "Context Menu" 'hide t)))
+  (let* ((menu (make-sparse-keymap (propertize "Context Menu"
+                                               'hide t
+                                               'no-keys t)))
          (click (or click last-input-event))
          (fun (mouse-posn-property (event-start click)
                                    'context-menu-function)))
diff --git a/src/keyboard.c b/src/keyboard.c
index 462b415c1d..8c90292137 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -7832,16 +7832,20 @@ parse_menu_item (Lisp_Object item, int inmenubar)
 		filter = item;
 	      else if (EQ (tem, QCkey_sequence))
 		{
-		  tem = XCAR (item);
-		  if (SYMBOLP (tem) || STRINGP (tem) || VECTORP (tem))
-		    /* Be GC protected. Set keyhint to item instead of tem.  */
-		    keyhint = item;
-		}
+		  if (!inhibit_menu_keys)
+		    {
+		      tem = XCAR (item);
+		      if (SYMBOLP (tem) || STRINGP (tem) || VECTORP (tem))
+			/* Be GC protected. Set keyhint to item instead of tem.  */
+			keyhint = item;
+		    }		}
 	      else if (EQ (tem, QCkeys))
 		{
-		  tem = XCAR (item);
-		  if (CONSP (tem) || STRINGP (tem))
-		    ASET (item_properties, ITEM_PROPERTY_KEYEQ, tem);
+		  if (!inhibit_menu_keys){
+		    tem = XCAR (item);
+		    if (CONSP (tem) || STRINGP (tem))
+		      ASET (item_properties, ITEM_PROPERTY_KEYEQ, tem);
+		  }
 		}
 	      else if (EQ (tem, QCbutton) && CONSP (XCAR (item)))
 		{
@@ -7916,6 +7920,7 @@ parse_menu_item (Lisp_Object item, int inmenubar)
   if (inmenubar > 0)
     return 1;
 
+  if (!inhibit_menu_keys)
   { /* This is a command.  See if there is an equivalent key binding.  */
     Lisp_Object keyeq = AREF (item_properties, ITEM_PROPERTY_KEYEQ);
     AUTO_STRING (space_space, "  ");
@@ -12495,6 +12500,11 @@ syms_of_keyboard (void)
                Vwhile_no_input_ignore_events,
                doc: /* Ignored events from while-no-input.  */);
 
+  DEFVAR_BOOL ("inhibit-menu-keys",
+               inhibit_menu_keys,
+               doc: /* If non-nil, inhibit menu keys.  */);
+  inhibit_menu_keys = false;
+
   pdumper_do_now_and_after_load (syms_of_keyboard_for_pdumper);
 }
 
diff --git a/src/menu.c b/src/menu.c
index 1aafa78c3c..e7e7ecca6a 100644
--- a/src/menu.c
+++ b/src/menu.c
@@ -1281,13 +1281,18 @@ x_popup_menu_1 (Lisp_Object position, Lisp_Object menu)
       /* We were given a keymap.  Extract menu info from the keymap.  */
       Lisp_Object prompt;
 
-      /* Extract the detailed info to make one pane.  */
-      keymap_panes (&menu, 1);
-
       /* Search for a string appearing directly as an element of the keymap.
 	 That string is the title of the menu.  */
       prompt = Fkeymap_prompt (keymap);
 
+      if (STRINGP (prompt)
+	  && SCHARS (prompt) > 0
+	  && !NILP (Fget_text_property (make_fixnum (0), Qno_keys, prompt)))
+      specbind (Qinhibit_menu_keys, Qt);
+
+      /* Extract the detailed info to make one pane.  */
+      keymap_panes (&menu, 1);
+
 #if defined (USE_GTK) || defined (HAVE_NS)
       if (STRINGP (prompt)
 	  && SCHARS (prompt) > 0
@@ -1583,6 +1588,8 @@ syms_of_menu (void)
   staticpro (&menu_items);
 
   DEFSYM (Qhide, "hide");
+  DEFSYM (Qno_keys, "no-keys");
+  DEFSYM (Qinhibit_menu_keys, "inhibit-menu-keys");
 
   defsubr (&Sx_popup_menu);
   defsubr (&Sx_popup_dialog);

bug#50067: Context menus

[Juri Linkov]

After typing 'C-h k', clicking mouse-3 on a function name,
then selecting e.g. "Describe Function", the Help buffer says

  <down-mouse-3> <describe-symbol> at that spot is undefined

This can be fixed by this patch:

diff --git a/lisp/help.el b/lisp/help.el
index 8f77167040..b794751eca 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -695,7 +695,7 @@ help--analyze-key
 	 (mouse-msg (if (or (memq 'click modifiers) (memq 'down modifiers)
 			    (memq 'drag modifiers))
                         " at that spot" ""))
-	 (defn (key-binding key t)))
+	 (defn (save-excursion (mouse-set-point event) (key-binding key t))))
     ;; Handle the case where we faked an entry in "Select and Paste" menu.
     (when (and (eq defn nil)
 	       (stringp (aref key (1- (length key))))
-- 

bug#50067: Context menus

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> Other programs don't show the keys in context menus.
> The Human Interface Guidelines say:
>
>   Show keyboard shortcuts in menu bar menus, not contextual menus.
>   Contextual menus are already shortcuts to task-specific commands;
>   it's redundant to display keyboard shortcuts too.

I'm not sure I agree with those guidelines -- displaying the key
bindings increases the discoverability of those key bindings.  (Not
using the mouse is more productive in the long term for most people.)

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

bug#50067: [External] : bug#50067: Context menus

[Drew Adams]

> > Other programs don't show the keys in context menus.
> > The Human Interface Guidelines say:
> >
> >   Show keyboard shortcuts in menu bar menus, not contextual menus.
> >   Contextual menus are already shortcuts to task-specific commands;
> >   it's redundant to display keyboard shortcuts too.
> 
> I'm not sure I agree with those guidelines -- displaying the key
> bindings increases the discoverability of those key bindings.  (Not
> using the mouse is more productive in the long term for most people.)

+1.

Provided, that is, that the (context) menu-item
is bound to exactly the same command as the key.
But if the key does something different from what
the item does (e.g. dependent on the context),
then omitting the key name can make sense.

bug#50067: Context menus

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Mon, 27 Sep 2021 18:30:23 +0300
> 
> Other programs don't show the keys in context menus.
> The Human Interface Guidelines say:
> 
>   Show keyboard shortcuts in menu bar menus, not contextual menus.
>   Contextual menus are already shortcuts to task-specific commands;
>   it's redundant to display keyboard shortcuts too.
> 
> This patch hides all keys from the context menus:

Unconditionally? why??  Emacs always shows the keyboard bindings in
its menus, so why should we care what other apps do?  And why force
this on everyone?

bug#50067: Context menus

[Juri Linkov]

>> Other programs don't show the keys in context menus.
>> The Human Interface Guidelines say:
>>
>>   Show keyboard shortcuts in menu bar menus, not contextual menus.
>>   Contextual menus are already shortcuts to task-specific commands;
>>   it's redundant to display keyboard shortcuts too.
>
> I'm not sure I agree with those guidelines -- displaying the key
> bindings increases the discoverability of those key bindings.

For example, how to explain to the users why a keybinding
is displayed for "Undo", but not for "Redo".

Another question is why currently `C-x u' is displayed
instead of the shorter `C-/' for "Undo".

So maybe simpler just to disable keys only for "Undo"
with ‘:keys ""’.  Or the other way around: to manually
add some suggested keybinding to "Redo".

bug#50067: Context menus

[Juri Linkov]

>> Other programs don't show the keys in context menus.
>> The Human Interface Guidelines say:
>>
>>   Show keyboard shortcuts in menu bar menus, not contextual menus.
>>   Contextual menus are already shortcuts to task-specific commands;
>>   it's redundant to display keyboard shortcuts too.
>>
>> This patch hides all keys from the context menus:
>
> Unconditionally? why??  Emacs always shows the keyboard bindings in
> its menus, so why should we care what other apps do?  And why force
> this on everyone?

The patch disables the keys conditionally by adding a new
text property to the menu title.

bug#50067: Context menus

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 50067@debbugs.gnu.org
> Date: Tue, 28 Sep 2021 21:54:40 +0300
> 
> >> Other programs don't show the keys in context menus.
> >> The Human Interface Guidelines say:
> >>
> >>   Show keyboard shortcuts in menu bar menus, not contextual menus.
> >>   Contextual menus are already shortcuts to task-specific commands;
> >>   it's redundant to display keyboard shortcuts too.
> >>
> >> This patch hides all keys from the context menus:
> >
> > Unconditionally? why??  Emacs always shows the keyboard bindings in
> > its menus, so why should we care what other apps do?  And why force
> > this on everyone?
> 
> The patch disables the keys conditionally by adding a new
> text property to the menu title.

I meant user's ability to request the display of key bindings in the
menu.  I hope you don't expect users to change text properties on menu
titles as the means to have that control.  There should be an option,
and frankly I don't understand why these menus should behave
differently from any other menu in Emacs that shows commands.

bug#50067: [External] : bug#50067: Context menus

[Drew Adams]

> For example, how to explain to the users why a keybinding
> is displayed for "Undo", but not for "Redo".

When & why does that need to be explained?
Is there a missing binding, perhaps?

> Another question is why currently `C-x u' is displayed
> instead of the shorter `C-/' for "Undo".

That's an orthogonal problem, which is not
specific to context menus, right?  That's
a general, longstanding problem, IIUC.

> So maybe simpler just to disable keys only for "Undo"
> with ‘:keys ""’.  Or the other way around: to manually
> add some suggested keybinding to "Redo".

Is there a reason why one has and the other
doesn't have a binding?  Does it matter?

How is this related to the general question
of whether _context menu_ items should be
able to have keys listed?  Doesn't all that
you wrote apply to menus generally?

bug#50067: Context menus

[Juri Linkov]

>> I'm not sure I agree with those guidelines -- displaying the key
>> bindings increases the discoverability of those key bindings.
>
> For example, how to explain to the users why a keybinding
> is displayed for "Undo", but not for "Redo".
>
> Another question is why currently `C-x u' is displayed
> instead of the shorter `C-/' for "Undo".
>
> So maybe simpler just to disable keys only for "Undo"
> with ‘:keys ""’.  Or the other way around: to manually
> add some suggested keybinding to "Redo".

I think it would be better to rebind "Undo" from 'undo'
to 'undo-only'.  Since 'undo-only' has no keybinding,
it will not be displayed in the menu.

bug#50067: Context menus

[Juri Linkov]

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

Currently the option context-menu-global of context-menu-functions
doesn't take into account the variable menu-bar-final-items
to properly order the menu items.  This patch makes it possible
to order the items of global-map instead of menu-bar-current-active-maps
used in menu-bar-keymap by default:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: menu-bar-keymap.patch --]
[-- Type: text/x-diff, Size: 1573 bytes --]

diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index 980ba2fcd1..b6cc29720d 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -2713,9 +2713,11 @@ menu-bar-open-mouse
                        (cdr menu-bar-item-cons)
                      0))))
 
-(defun menu-bar-keymap ()
+(defun menu-bar-keymap (&optional keymap)
   "Return the current menu-bar keymap.
-
+It's possible to use the KEYMAP argument to override the default keymap
+that is the currently active maps.  For example, the argument KEYMAP
+could provide `global-map' where items are limited to the global map only.
 The ordering of the return value respects `menu-bar-final-items'."
   (let ((menu-bar '())
         (menu-end '()))
@@ -2729,7 +2731,7 @@ menu-bar-keymap
              ;; sorting.
              (push (cons pos menu-item) menu-end)
            (push menu-item menu-bar))))
-     (lookup-key (menu-bar-current-active-maps) [menu-bar]))
+     (lookup-key (or keymap (menu-bar-current-active-maps)) [menu-bar]))
     `(keymap ,@(nreverse menu-bar)
              ,@(mapcar #'cdr (sort menu-end
                                    (lambda (a b)
diff --git a/lisp/mouse.el b/lisp/mouse.el
index 3e00ca7ce1..6c97cc365f 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -364,7 +364,7 @@ context-menu-global
                 (when (consp binding)
                   (define-key-after menu (vector key)
                     (copy-sequence binding))))
-              (lookup-key global-map [menu-bar]))
+              (menu-bar-keymap global-map))
   menu)
 
 (defun context-menu-local (menu _click)

bug#50067: Context menus

[Juri Linkov]

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

> Currently the option context-menu-global of context-menu-functions
> doesn't take into account the variable menu-bar-final-items
> to properly order the menu items.  This patch makes it possible
> to order the items of global-map instead of menu-bar-current-active-maps
> used in menu-bar-keymap by default:

Actually, here is a better patch that will also allow sorting items on
context-menu-local as well.  The same KEYMAP arg of menu-bar-keymap
could be used also in mouse-menu-bar-map to sort items of [C-down-mouse-3]
in the right order (they were unsorted for a long time):


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: menu-bar-keymap-2.patch --]
[-- Type: text/x-diff, Size: 1907 bytes --]

diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index 1a81f1a3d0..94e75efeeb 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -2738,7 +2738,7 @@ menu-bar-keymap
              ;; sorting.
              (push (cons pos menu-item) menu-end)
            (push menu-item menu-bar))))
-     (lookup-key (or keymap (menu-bar-current-active-maps)) [menu-bar]))
+     (or keymap (lookup-key (menu-bar-current-active-maps) [menu-bar])))
     `(keymap ,@(nreverse menu-bar)
              ,@(mapcar #'cdr (sort menu-end
                                    (lambda (a b)
diff --git a/lisp/mouse.el b/lisp/mouse.el
index d6912892ef..5b9ae121d7 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -271,10 +271,10 @@ mouse-menu-bar-map
     ;; FIXME: We have a problem here: we have to use the global/local/minor
     ;; so they're displayed in the expected order, but later on in the command
     ;; loop, they're actually looked up in the opposite order.
-    (apply 'append
-           global-menu
-           local-menu
-           minor-mode-menus)))
+    (menu-bar-keymap (apply 'append
+                            global-menu
+                            local-menu
+                            minor-mode-menus))))
 
 \f
 ;; Context menus.
@@ -364,7 +364,7 @@ context-menu-global
                 (when (consp binding)
                   (define-key-after menu (vector key)
                     (copy-sequence binding))))
-              (menu-bar-keymap global-map))
+              (menu-bar-keymap (lookup-key global-map [menu-bar])))
   menu)
 
 (defun context-menu-local (menu _click)
@@ -377,7 +377,7 @@ context-menu-local
                     (when (consp binding)
                       (define-key-after menu (vector key)
                         (copy-sequence binding))))
-                  keymap)))
+                  (menu-bar-keymap keymap))))
   menu)
 
 (defun context-menu-minor (menu _click)

bug#50067: Context menus

[Juri Linkov]

>> Currently the option context-menu-global of context-menu-functions
>> doesn't take into account the variable menu-bar-final-items
>> to properly order the menu items.  This patch makes it possible
>> to order the items of global-map instead of menu-bar-current-active-maps
>> used in menu-bar-keymap by default:
>
> Actually, here is a better patch that will also allow sorting items on
> context-menu-local as well.  The same KEYMAP arg of menu-bar-keymap
> could be used also in mouse-menu-bar-map to sort items of [C-down-mouse-3]
> in the right order (they were unsorted for a long time):

Only part of the patch that fixes the new feature was pushed to emacs-28.
But the following part that fixes the old function could pushed to master later:

> diff --git a/lisp/mouse.el b/lisp/mouse.el
> index d6912892ef..5b9ae121d7 100644
> --- a/lisp/mouse.el
> +++ b/lisp/mouse.el
> @@ -271,10 +271,10 @@ mouse-menu-bar-map
>      ;; FIXME: We have a problem here: we have to use the global/local/minor
>      ;; so they're displayed in the expected order, but later on in the command
>      ;; loop, they're actually looked up in the opposite order.
> -    (apply 'append
> -           global-menu
> -           local-menu
> -           minor-mode-menus)))
> +    (menu-bar-keymap (apply 'append
> +                            global-menu
> +                            local-menu
> +                            minor-mode-menus))))

bug#50067: Context menus

[Juri Linkov]

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

After clicking the right mouse button e.g. on the mode-line's minor-modes,
typing S-F10 on a misspelled word pops up the context menu over the mode-line
instead of the position over the misspelled word.  Because it tries to find
the last event that is a mouse event.  In case of keyboard-based 'context-menu-open'
that let-binds 'inhibit-mouse-event-check' to t, this is not needed:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: inhibit_mouse_event_check.patch --]
[-- Type: text/x-diff, Size: 829 bytes --]

diff --git a/src/callint.c b/src/callint.c
index 44dae361c1..525a18683d 100644
--- a/src/callint.c
+++ b/src/callint.c
@@ -367,7 +367,7 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
      event with parameters.  */
   ptrdiff_t next_event;
   for (next_event = 0; next_event < key_count; next_event++)
-    if (EVENT_HAS_PARAMETERS (AREF (keys, next_event)))
+    if (inhibit_mouse_event_check || EVENT_HAS_PARAMETERS (AREF (keys, next_event)))
       break;
 
   /* Handle special starting chars `*' and `@'.  Also `-'.  */
@@ -618,6 +618,7 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
 	  do
 	    next_event++;
 	  while (next_event < key_count
+		 && ! inhibit_mouse_event_check
 		 && ! EVENT_HAS_PARAMETERS (AREF (keys, next_event)));
 
 	  break;

bug#50067: Context menus

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 25 Nov 2021 09:50:40 +0200
> 
> --- a/src/callint.c
> +++ b/src/callint.c
> @@ -367,7 +367,7 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
>       event with parameters.  */
>    ptrdiff_t next_event;
>    for (next_event = 0; next_event < key_count; next_event++)
> -    if (EVENT_HAS_PARAMETERS (AREF (keys, next_event)))
> +    if (inhibit_mouse_event_check || EVENT_HAS_PARAMETERS (AREF (keys, next_event)))
>        break;
>  
>    /* Handle special starting chars `*' and `@'.  Also `-'.  */
> @@ -618,6 +618,7 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
>  	  do
>  	    next_event++;
>  	  while (next_event < key_count
> +		 && ! inhibit_mouse_event_check
>  		 && ! EVENT_HAS_PARAMETERS (AREF (keys, next_event)));
>  
>  	  break;

Why check this condition inside the loops, rather than avoid entering
the loops when the condition is right, in the first place?

And please add comments there explaining the meaning of the
inhibit_mouse_event_check test.

Thanks.

bug#50067: Context menus

[Juri Linkov]

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

>> @@ -367,7 +367,7 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
>>       event with parameters.  */
>>    ptrdiff_t next_event;
>>    for (next_event = 0; next_event < key_count; next_event++)
>> -    if (EVENT_HAS_PARAMETERS (AREF (keys, next_event)))
>> +    if (inhibit_mouse_event_check || EVENT_HAS_PARAMETERS (AREF (keys, next_event)))
>>        break;
>>  
>>    /* Handle special starting chars `*' and `@'.  Also `-'.  */
>> @@ -618,6 +618,7 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
>>  	  do
>>  	    next_event++;
>>  	  while (next_event < key_count
>> +		 && ! inhibit_mouse_event_check
>>  		 && ! EVENT_HAS_PARAMETERS (AREF (keys, next_event)));
>>  
>>  	  break;
>
> Why check this condition inside the loops, rather than avoid entering
> the loops when the condition is right, in the first place?

I thought that checking the condition inside the loops
would be less risky for the emacs-28 release branch.

> And please add comments there explaining the meaning of the
> inhibit_mouse_event_check test.

This patch also adds comments, and removes one condition
that is not unnecessary when event with parameters
is not searched when inhibit_mouse_event_check is non-nil:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: inhibit_mouse_event_check_2.patch --]
[-- Type: text/x-diff, Size: 2074 bytes --]

diff --git a/src/callint.c b/src/callint.c
index 44dae361c1..68f103759a 100644
--- a/src/callint.c
+++ b/src/callint.c
@@ -364,11 +364,14 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
 
   /* The index of the next element of this_command_keys to examine for
      the 'e' interactive code.  Initialize it to point to the first
-     event with parameters.  */
-  ptrdiff_t next_event;
-  for (next_event = 0; next_event < key_count; next_event++)
-    if (EVENT_HAS_PARAMETERS (AREF (keys, next_event)))
-      break;
+     event with parameters.  When `inhibit_mouse_event_check' is non-nil,
+     the command can accept an event without parameters,
+     so don't search for the event with parameters in this case.  */
+  ptrdiff_t next_event = 0;
+  if (!inhibit_mouse_event_check)
+    for (; next_event < key_count; next_event++)
+      if (EVENT_HAS_PARAMETERS (AREF (keys, next_event)))
+	break;
 
   /* Handle special starting chars `*' and `@'.  Also `-'.  */
   /* Note that `+' is reserved for user extensions.  */
@@ -606,7 +609,7 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
 	  break;
 
 	case 'e':		/* The invoking event.  */
-	  if (!inhibit_mouse_event_check && next_event >= key_count)
+	  if (next_event >= key_count)
 	    error ("%s must be bound to an event with parameters",
 		   (SYMBOLP (function)
 		    ? SSDATA (SYMBOL_NAME (function))
@@ -614,11 +617,15 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
 	  args[i] = AREF (keys, next_event);
 	  varies[i] = -1;
 
-	  /* Find the next parameterized event.  */
-	  do
+	  /* `inhibit_mouse_event_check' allows non-parameterized events.  */
+	  if (inhibit_mouse_event_check)
 	    next_event++;
-	  while (next_event < key_count
-		 && ! EVENT_HAS_PARAMETERS (AREF (keys, next_event)));
+	  else
+	    /* Find the next parameterized event.  */
+	    do
+	      next_event++;
+	    while (next_event < key_count
+		   && ! EVENT_HAS_PARAMETERS (AREF (keys, next_event)));
 
 	  break;
 

bug#50067: Context menus

[Juri Linkov]

>> Why check this condition inside the loops, rather than avoid entering
>> the loops when the condition is right, in the first place?
>
> I thought that checking the condition inside the loops
> would be less risky for the emacs-28 release branch.
>
>> And please add comments there explaining the meaning of the
>> inhibit_mouse_event_check test.
>
> This patch also adds comments, and removes one condition
> that is not unnecessary when event with parameters
> is not searched when inhibit_mouse_event_check is non-nil:

Patch pushed now.