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

[Juri Linkov]

> 1. Mac users expect C-mouse-1 to be equivalent to mouse-3 because Macs
> traditionally only have one mouse button; this is especially true of laptop
> users (probably the majority today). Simply speaking, control-mouse-1 is
> expected to invoke the context menu; this is the platform convention and we
> should try to find a way to make it so.
>
> We could add a mac-only setting that remaps control-mouse-1 to mouse-3
> (preserving all other modifiers like shift, alt and super). (I don't think
> there already is such a translation but could be mistaken). I'm attaching
> a tentative patch as a proof of concept.
>
> It is also possible to just do it in Lisp, but then we'd probably need
> to do it specially for the context menu. (I tried using event
> translation but that didn't work right.)

The raison d'être for the special mode context-menu-mode is to
rebind the default keys optionally.  So it would make sense to bind
context-menu-entry conditionally:

1. to [C-mouse-1] on macOS depending on ‘(featurep 'ns)’;
2. to [down-mouse-3] everywhere else.

in the function body of context-menu-mode.

> Currently, C-mouse-1 pops up the buffer menu but it's unclear if
> that's used by a sizeable part of the population, and in any case Macs
> have a Buffer menu easily accessible in the menu bar.

Additionally, a Buffer menu is accessible from the context menu
when context-menu-functions is customized to contain ‘context-menu-global’.

> 2. The context menu contains the disabled entry "Context Menu" as some kind
> of title – that is very alien on macOS where context menus never have
> titles. I believe the same is true at least on Windows, and frankly, there
> should be no need to explicitly tell the user that what he or she is
> looking at is a context menu. I suggest we just drop the title on
> all platforms.
>
> Replacing (purecopy "Context Menu") by "" in `context-menu-entry` makes it
> go away, but then we get the new title "Select" from heavens knows where
> (menu.c, from the look of it). Apparently the Emacs menu system just wants
> a title; we should find a way to disable it in popup menus.

After trying to remove it altogether, there is no title at all,
but maybe it's platform-dependent (I tested on GTK):

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 9b7d4c240f..5193994231 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -304,7 +304,7 @@ context-menu-filter-function
 (defun context-menu-map ()
   "Return composite menu map."
-  (let ((menu (make-sparse-keymap "Context Menu")))
+  (let ((menu (make-sparse-keymap)))
     (run-hook-wrapped 'context-menu-functions
                       (lambda (fun)
                         (setq menu (funcall fun menu))

> 3. Not Mac-specific (really about xref): in some modes, Find Definition
> applies to point instead of where the click occurred. Apparently the xref
> backend ignores the symbol discovered by xref-find-definition-at-mouse
> because that tokenisation isn't appropriate for the language and it does
> a more thorough job that is based on point instead. What we really should
> do is to set point temporarily for the whole duration of the
> xref-find-definitions call.

Could you please provide a test case?  Because I've thoroughly tested
“Find Definition” to apply where the click occurred, and it works as expected.

bug#50067: Context menus

[Mattias Engdegård]

18 aug. 2021 kl. 18.42 skrev Juri Linkov <juri@linkov.net>:

> The raison d'être for the special mode context-menu-mode is to
> rebind the default keys optionally.  So it would make sense to bind
> context-menu-entry conditionally:
> 
> 1. to [C-mouse-1] on macOS depending on ‘(featurep 'ns)’;
> 2. to [down-mouse-3] everywhere else.
> 
> in the function body of context-menu-mode.

We'd like both bindings to work on macOS (the user could have a mouse with a right button) but that should be doable.

> After trying to remove it altogether, there is no title at all,
> but maybe it's platform-dependent (I tested on GTK):

After your suggested change I still get the "Select" title; needs to be investigated further.

> Could you please provide a test case?  Because I've thoroughly tested
> “Find Definition” to apply where the click occurred, and it works as expected.

Yes it works for elisp, because its xref backend is simplistic enough to just accept the symbol given as argument. An example of a package where it doesn't work is merlin which is rather more sophisticated and wants to find the exact context so that it can find the right definition, so it ignores the argument.

It would be useful to do this for elisp as well, so that it would work for local variables etc.

bug#50067: Context menus

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Wed, 18 Aug 2021 19:42:12 +0300
> Cc: 50067@debbugs.gnu.org, Tak Kunihiro <homeros.misasa@gmail.com>,
>  tkk@misasa.okayama-u.ac.jp, Lars Ingebrigtsen <larsi@gnus.org>,
>  Alan Third <alan@idiocy.org>
> 
> > 2. The context menu contains the disabled entry "Context Menu" as some kind
> > of title – that is very alien on macOS where context menus never have
> > titles. I believe the same is true at least on Windows, and frankly, there
> > should be no need to explicitly tell the user that what he or she is
> > looking at is a context menu. I suggest we just drop the title on
> > all platforms.
> >
> > Replacing (purecopy "Context Menu") by "" in `context-menu-entry` makes it
> > go away, but then we get the new title "Select" from heavens knows where
> > (menu.c, from the look of it). Apparently the Emacs menu system just wants
> > a title; we should find a way to disable it in popup menus.
> 
> After trying to remove it altogether, there is no title at all,
> but maybe it's platform-dependent (I tested on GTK):
> 
> diff --git a/lisp/mouse.el b/lisp/mouse.el
> index 9b7d4c240f..5193994231 100644
> --- a/lisp/mouse.el
> +++ b/lisp/mouse.el
> @@ -304,7 +304,7 @@ context-menu-filter-function
>  (defun context-menu-map ()
>    "Return composite menu map."
> -  (let ((menu (make-sparse-keymap "Context Menu")))
> +  (let ((menu (make-sparse-keymap)))
>      (run-hook-wrapped 'context-menu-functions
>                        (lambda (fun)
>                          (setq menu (funcall fun menu))

I see you already pushed this, but it's a bad idea: it makes ugly
context menus on TTYs (and AFAIU also on non-toolkit X builds): these
_require_ the menu name because they display a caption which looks bad
with an empty name.

I don't really understand the original complaint: we have similar
captions on the menu shown by C-mouse-3 in the default configuration:
do macOS users want those to be removed as well?  If not, what is the
difference?

bug#50067: Context menus

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 18 Aug 2021 19:46:45 +0200
> Cc: 50067@debbugs.gnu.org, Tak Kunihiro <homeros.misasa@gmail.com>,
>  tkk@misasa.okayama-u.ac.jp, Lars Ingebrigtsen <larsi@gnus.org>,
>  Alan Third <alan@idiocy.org>
> 
> > After trying to remove it altogether, there is no title at all,
> > but maybe it's platform-dependent (I tested on GTK):
> 
> After your suggested change I still get the "Select" title; needs to be investigated further.

That "Select" is probably macOS specific, I don't see it here.

bug#50067: Context menus

[Mattias Engdegård]

18 aug. 2021 kl. 19.46 skrev Eli Zaretskii <eliz@gnu.org>:

> I don't really understand the original complaint: we have similar
> captions on the menu shown by C-mouse-3 in the default configuration:
> do macOS users want those to be removed as well?

We definitely do but few people use that (rather useless) menu so its appearance doesn't matter much.
If the goal here is to make a genuinely useful context menu that people actually want to use, then it should look like one.

I'm probably wrong, but Windows context menus haven't titles either as I remember it and they would look quite out of place on that platform as well.

bug#50067: Context menus

[Juri Linkov]

>> diff --git a/lisp/mouse.el b/lisp/mouse.el
>> index 9b7d4c240f..5193994231 100644
>> --- a/lisp/mouse.el
>> +++ b/lisp/mouse.el
>> @@ -304,7 +304,7 @@ context-menu-filter-function
>>  (defun context-menu-map ()
>>    "Return composite menu map."
>> -  (let ((menu (make-sparse-keymap "Context Menu")))
>> +  (let ((menu (make-sparse-keymap)))
>>      (run-hook-wrapped 'context-menu-functions
>>                        (lambda (fun)
>>                          (setq menu (funcall fun menu))
>
> I see you already pushed this, but it's a bad idea: it makes ugly
> context menus on TTYs (and AFAIU also on non-toolkit X builds): these
> _require_ the menu name because they display a caption which looks bad
> with an empty name.

I guess the presence of the title should be platform-dependent.
For example, without a title it looks great on the GTK build.

bug#50067: Context menus

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 18 Aug 2021 20:01:17 +0200
> Cc: Juri Linkov <juri@linkov.net>, 50067@debbugs.gnu.org,
>         homeros.misasa@gmail.com, tkk@misasa.okayama-u.ac.jp, larsi@gnus.org,
>         alan@idiocy.org
> 
> 18 aug. 2021 kl. 19.46 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > I don't really understand the original complaint: we have similar
> > captions on the menu shown by C-mouse-3 in the default configuration:
> > do macOS users want those to be removed as well?
> 
> We definitely do but few people use that (rather useless) menu so its appearance doesn't matter much.

OK, so I guess this change should be done only on macOS (but not on
TTY frames displayed on macOS).

> I'm probably wrong, but Windows context menus haven't titles either as I remember it and they would look quite out of place on that platform as well.

Well, C-mouse-3 shows the caption of a menu on MS-Windows, so I see no
problem with showing "Context menu" there (or any other string, if we
find a better one, actually).  That serves as a small description of
what this menu is about.

bug#50067: Context menus

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: mattiase@acm.org,  50067@debbugs.gnu.org,  homeros.misasa@gmail.com,
>   tkk@misasa.okayama-u.ac.jp,  larsi@gnus.org,  alan@idiocy.org
> Date: Wed, 18 Aug 2021 21:06:11 +0300
> 
> > I see you already pushed this, but it's a bad idea: it makes ugly
> > context menus on TTYs (and AFAIU also on non-toolkit X builds): these
> > _require_ the menu name because they display a caption which looks bad
> > with an empty name.
> 
> I guess the presence of the title should be platform-dependent.
> For example, without a title it looks great on the GTK build.

What happens if the GTK build displays a TTY frame?  Isn't the menu
definition global, and thus doesn't distinguish between frame types?

bug#50067: Context menus

[Eli Zaretskii]

> Date: Wed, 18 Aug 2021 21:12:58 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: alan@idiocy.org, mattiase@acm.org, homeros.misasa@gmail.com,
>  tkk@misasa.okayama-u.ac.jp, larsi@gnus.org, 50067@debbugs.gnu.org
> 
> > > I see you already pushed this, but it's a bad idea: it makes ugly
> > > context menus on TTYs (and AFAIU also on non-toolkit X builds): these
> > > _require_ the menu name because they display a caption which looks bad
> > > with an empty name.
> > 
> > I guess the presence of the title should be platform-dependent.
> > For example, without a title it looks great on the GTK build.
> 
> What happens if the GTK build displays a TTY frame?  Isn't the menu
> definition global, and thus doesn't distinguish between frame types?

Come to think about it: aren't menu keymaps _required_ to have this
string?  The ELisp manual says:

   -- Function: make-sparse-keymap &optional prompt
       This function creates and returns a new sparse keymap with no
       entries.  (A sparse keymap is the kind of keymap you usually want.)
       The new keymap does not contain a char-table, unlike ‘make-keymap’,
       and does not bind any events.

	    (make-sparse-keymap)
		⇒ (keymap)

       If you specify PROMPT, that becomes the overall prompt string for
       the keymap.  You should specify this only for menu keymaps (*note
       Defining Menus::).  A keymap with an overall prompt string will
       always present a mouse menu or a keyboard menu if it is active for
       looking up the next input event.

Stefan, did we remove this requirement at some point, or is it still a
requirement?

If this is still needed (and it seems to be, at least on some frame
types), I guess it's the code which produces native menus that wants
not to have this caption (e.g., on macOS and GTK) -- that code should
ignore this string and not stuff it into the native menu widget.  But
internally the menu name should still be present in the Lisp data
structure.

bug#50067: Context menus

[Juri Linkov]

>> I guess the presence of the title should be platform-dependent.
>> For example, without a title it looks great on the GTK build.
>
> What happens if the GTK build displays a TTY frame?  Isn't the menu
> definition global, and thus doesn't distinguish between frame types?

The menu definition is constructed dynamically, so it possible
to set the title when (framep (selected-frame)) returns t on tty.

bug#50067: Context menus

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: mattiase@acm.org,  50067@debbugs.gnu.org,  homeros.misasa@gmail.com,
>   tkk@misasa.okayama-u.ac.jp,  larsi@gnus.org,  alan@idiocy.org
> Date: Wed, 18 Aug 2021 21:40:19 +0300
> 
> >> I guess the presence of the title should be platform-dependent.
> >> For example, without a title it looks great on the GTK build.
> >
> > What happens if the GTK build displays a TTY frame?  Isn't the menu
> > definition global, and thus doesn't distinguish between frame types?
> 
> The menu definition is constructed dynamically, so it possible
> to set the title when (framep (selected-frame)) returns t on tty.

OK, but as I wrote elsewhere, I think the string should always be
present, and if some GUI toolkit wants to ignore it, it should avoid
putting it into the native menu structure when it creates the menu
widget(s).  The Lisp data should remain the same, IMO.

bug#50067: Context menus

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

> Stefan, did we remove this requirement at some point, or is it still a
> requirement?

If memory serves it's used (as the title) for the non-toolkit menus and
it's not used for the toolkit menus, but the reality is probably less
clear cut than that.


        Stefan

bug#50067: Context menus

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: juri@linkov.net,  alan@idiocy.org,  mattiase@acm.org,
>   homeros.misasa@gmail.com,  tkk@misasa.okayama-u.ac.jp,  larsi@gnus.org,
>   50067@debbugs.gnu.org
> Date: Wed, 18 Aug 2021 21:31:37 -0400
> 
> > Stefan, did we remove this requirement at some point, or is it still a
> > requirement?
> 
> If memory serves it's used (as the title) for the non-toolkit menus and
> it's not used for the toolkit menus, but the reality is probably less
> clear cut than that.

That's also my recollection.  In particular TTY menus actually
_expect_ to find a meaningful string there.

So I think we need to reinstate the name in the context menus (though
perhaps we could come up with a better name than "Context menu"), and
if some toolkit wants to ignore it, they should do it when they create
the menu widgets to show the menu on the screen.

bug#50067: Context menus

[Juri Linkov]

>> >> I guess the presence of the title should be platform-dependent.
>> >> For example, without a title it looks great on the GTK build.
>> >
>> > What happens if the GTK build displays a TTY frame?  Isn't the menu
>> > definition global, and thus doesn't distinguish between frame types?
>> 
>> The menu definition is constructed dynamically, so it possible
>> to set the title when (framep (selected-frame)) returns t on tty.
>
> OK, but as I wrote elsewhere, I think the string should always be
> present, and if some GUI toolkit wants to ignore it, it should avoid
> putting it into the native menu structure when it creates the menu
> widget(s).  The Lisp data should remain the same, IMO.

GUI toolkits can't ignore titles for all menus.  Some menus should be
displayed with a title for all toolkits.  The context menu is special.
Nowadays everyone is accustomed to down-mouse-3 popping up a context menu
without title.  But other Emacs-specific menus that are not familiar to users
such as mouse-buffer-menu bound C-<down-mouse-1> should display a title
for all toolkits to explain to the user what choice the menu presents.

bug#50067: Context menus

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: mattiase@acm.org,  50067@debbugs.gnu.org,  homeros.misasa@gmail.com,
>   tkk@misasa.okayama-u.ac.jp,  larsi@gnus.org,  alan@idiocy.org
> Date: Thu, 19 Aug 2021 10:12:54 +0300
> 
> > OK, but as I wrote elsewhere, I think the string should always be
> > present, and if some GUI toolkit wants to ignore it, it should avoid
> > putting it into the native menu structure when it creates the menu
> > widget(s).  The Lisp data should remain the same, IMO.
> 
> GUI toolkits can't ignore titles for all menus.  Some menus should be
> displayed with a title for all toolkits.

Which popup menus have titles that cannot be ignored, and why?

> The context menu is special.  Nowadays everyone is accustomed to
> down-mouse-3 popping up a context menu without title.  But other
> Emacs-specific menus that are not familiar to users such as
> mouse-buffer-menu bound C-<down-mouse-1> should display a title for
> all toolkits to explain to the user what choice the menu presents.

If we want some titles to behave in special ways, we could put a text
property on the title string to mark those titles that should get
special treatment.  That is better than removing the title, and
certainly better than removing it based on the frame type, because
then Lisp code which handles the menu keymaps will need to be prepared
to handle both kinds of menu keymaps, the ones with a title and ones
without it.  It is also against the documented practice.

So let's please reinstate the title, and use properties or some other
mechanism to mark the title strings that need special handling in some
circumstances.

Thanks.

bug#50067: Context menus

[Mattias Engdegård]

18 aug. 2021 kl. 19.46 skrev Mattias Engdegård <mattiase@acm.org>:

> Yes it works for elisp, because its xref backend is simplistic enough to just accept the symbol given as argument. An example of a package where it doesn't work is merlin which is rather more sophisticated and wants to find the exact context so that it can find the right definition, so it ignores the argument.

Having actually read the doc strings I see that xref backends are supposed to define `xref-backend-identifier-at-point` which can stuff any information it needs in a property of the string it returns, as sort of a semi-covert channel to `xref-backend-definitions` etc. Sorry about the unfounded complaint.

Would adding `xref-find-references-at-mouse` be handy for use in the context menu?

bug#50067: Context menus

[Juri Linkov]

>> GUI toolkits can't ignore titles for all menus.  Some menus should be
>> displayed with a title for all toolkits.
>
> Which popup menus have titles that cannot be ignored, and why?

An example of such menu is mouse-buffer-menu bound to C-<down-mouse-1>
where the user might wonder what do these menu items with mode names mean?

>> The context menu is special.  Nowadays everyone is accustomed to
>> down-mouse-3 popping up a context menu without title.  But other
>> Emacs-specific menus that are not familiar to users such as
>> mouse-buffer-menu bound C-<down-mouse-1> should display a title for
>> all toolkits to explain to the user what choice the menu presents.
>
> If we want some titles to behave in special ways, we could put a text
> property on the title string to mark those titles that should get
> special treatment.  That is better than removing the title, and
> certainly better than removing it based on the frame type, because
> then Lisp code which handles the menu keymaps will need to be prepared
> to handle both kinds of menu keymaps, the ones with a title and ones
> without it.  It is also against the documented practice.

Using a text property would be a good solution.  Should then
the NS-specific "Select" title be removed in menu.c
when the title has a special text property?

bug#50067: Context menus

[Juri Linkov]

>> Yes it works for elisp, because its xref backend is simplistic enough to
>> just accept the symbol given as argument. An example of a package where
>> it doesn't work is merlin which is rather more sophisticated and wants to
>> find the exact context so that it can find the right definition, so it
>> ignores the argument.
>
> Having actually read the doc strings I see that xref backends are supposed
> to define `xref-backend-identifier-at-point` which can stuff any
> information it needs in a property of the string it returns, as sort of
> a semi-covert channel to `xref-backend-definitions` etc. Sorry about the
> unfounded complaint.
>
> Would adding `xref-find-references-at-mouse` be handy for use in the context menu?

If the existing `xref-backend-identifier-at-point` can't be reused
by adding a new optional arg `click`, then a cleaner solution
would be to add `xref-backend-identifier-at-mouse` indeed.

bug#50067: Context menus

[Mattias Engdegård]

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

20 aug. 2021 kl. 09.29 skrev Juri Linkov <juri@linkov.net>:

> An example of such menu is mouse-buffer-menu bound to C-<down-mouse-1>
> where the user might wonder what do these menu items with mode names mean?

That's moot for macOS since C-mouse-1 will be used for the context menu. (Proposed patch attached.)

In macOS/NS, "titles" as disabled menu entries simply do not exist in the GUI vocabulary and look alien, amateurish, confusing or wrong. The menu is expected to be understood in its context of activation.

Where menus have titles, it's the corresponding entry in the parent menu (or menu bar). They are never named "something Menu" because that would be silly; typically it's a verb, or a noun setting a context for the entries in the sub-menu. For a menu of buffers to switch to, the title (if any) might be "Switch to buffer", "Buffer", "Switch to" or similar.


[-- Attachment #2: 0001-Use-C-mouse-1-for-context-menu-on-NS.patch --]
[-- Type: application/octet-stream, Size: 3303 bytes --]

From e505aa17a1ac679fee55220c594f0cfd53342739 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 20 Aug 2021 12:03:20 +0200
Subject: [PATCH] Use C-mouse-1 for context menu on NS

The Mac platform convention is to use control-left-click for context
menus (as a synonym to right-click).

* lisp/mouse.el (context-menu--old-bindings): Remove.
(context-menu--saved-bindings)
(context-menu--bind-mouse, context-menu--reset-bindings): New.
(context-menu-mode): Use new functions, with C-mouse-1 as extra
binding on NS.
---
 lisp/mouse.el | 45 +++++++++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 4c4a7d35a8..8af5509afc 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -440,8 +440,28 @@ context-menu-entry
   `(menu-item ,(purecopy "Context Menu") ignore
               :filter (lambda (_) (context-menu-map))))
 
-(defvar context-menu--old-down-mouse-3 nil)
-(defvar context-menu--old-mouse-3 nil)
+(defvar context-menu--saved-bindings nil
+  "Alist of bindings to restore when `context-menu-mode' is disabled.")
+
+(defun context-menu--bind-mouse (click-sym down-sym)
+  "Enable `context-menu-mode' mouse bindings.
+CLICK-SYM and DOWN-SYM are the mouse click and down key symbols to use."
+  (let ((click (vector click-sym))
+        (down (vector down-sym)))
+    (push (cons click-sym (global-key-binding click))
+          context-menu--saved-bindings)
+    (global-unset-key click)
+    (push (cons down-sym (global-key-binding down))
+          context-menu--saved-bindings)
+    (global-set-key down context-menu-entry)))
+
+(defun context-menu--reset-bindings ()
+  "Restore saved `context-menu-mode' bindings."
+  (pcase-dolist (`(sym . binding) context-menu--saved-bindings)
+    (let ((key (vector sym)))
+      (if binding
+          (global-set-key key binding)
+        (global-unset-key key)))))
 
 (define-minor-mode context-menu-mode
   "Toggle Context Menu mode.
@@ -449,20 +469,13 @@ context-menu-mode
 When Context Menu mode is enabled, clicking the mouse button down-mouse-3
 activates the menu whose contents depends on its surrounding context."
   :global t :group 'mouse
-  (cond
-   (context-menu-mode
-    (setq context-menu--old-mouse-3 (global-key-binding [mouse-3]))
-    (global-unset-key [mouse-3])
-    (setq context-menu--old-down-mouse-3 (global-key-binding [down-mouse-3]))
-    (global-set-key [down-mouse-3] context-menu-entry))
-   (t
-    (if (not context-menu--old-down-mouse-3)
-        (global-unset-key [down-mouse-3])
-      (global-set-key [down-mouse-3] context-menu--old-down-mouse-3)
-      (setq context-menu--old-down-mouse-3 nil))
-    (when context-menu--old-mouse-3
-      (global-set-key [mouse-3] context-menu--old-mouse-3)
-      (setq context-menu--old-mouse-3 nil)))))
+  (if context-menu-mode
+      (progn
+        (setq context-menu--saved-bindings nil)
+        (context-menu--bind-mouse 'mouse-3 'down-mouse-3)
+        (when (featurep 'ns)
+          (context-menu--bind-mouse 'C-mouse-1 'C-down-mouse-1)))
+    (context-menu--restore-bindings)))
 
 \f
 ;; Commands that operate on windows.
-- 
2.21.1 (Apple Git-122.3)


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

bug#50067: Context menus

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 20 Aug 2021 12:29:46 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, 50067@debbugs.gnu.org,
>         homeros.misasa@gmail.com, tkk@misasa.okayama-u.ac.jp, larsi@gnus.org,
>         alan@idiocy.org
> 
> In macOS/NS, "titles" as disabled menu entries simply do not exist in the GUI vocabulary and look alien, amateurish, confusing or wrong. The menu is expected to be understood in its context of activation.

Then please propose macOS-specific changes that ignore the menu names
instead of displaying them.  Platform-specific conventions should not
leak into general Lisp data structures.

bug#50067: Context menus

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: mattiase@acm.org,  50067@debbugs.gnu.org,  homeros.misasa@gmail.com,
>   tkk@misasa.okayama-u.ac.jp,  larsi@gnus.org,  alan@idiocy.org
> Date: Fri, 20 Aug 2021 10:29:47 +0300
> 
> >> GUI toolkits can't ignore titles for all menus.  Some menus should be
> >> displayed with a title for all toolkits.
> >
> > Which popup menus have titles that cannot be ignored, and why?
> 
> An example of such menu is mouse-buffer-menu bound to C-<down-mouse-1>
> where the user might wonder what do these menu items with mode names mean?

OK, but I still don't think I fully understand how context menus are
different.  Are we sure the user will immediately understand the
purpose of the context menus, but not of a buffer-menu?

> > If we want some titles to behave in special ways, we could put a text
> > property on the title string to mark those titles that should get
> > special treatment.  That is better than removing the title, and
> > certainly better than removing it based on the frame type, because
> > then Lisp code which handles the menu keymaps will need to be prepared
> > to handle both kinds of menu keymaps, the ones with a title and ones
> > without it.  It is also against the documented practice.
> 
> Using a text property would be a good solution.  Should then
> the NS-specific "Select" title be removed in menu.c
> when the title has a special text property?

I'm not sure I understand what is the NS-specific "Select" title.  Can
you point me to the relevant code?

bug#50067: Context menus

[Mattias Engdegård]

20 aug. 2021 kl. 12.53 skrev Eli Zaretskii <eliz@gnu.org>:

> Then please propose macOS-specific changes that ignore the menu names
> instead of displaying them.  Platform-specific conventions should not
> leak into general Lisp data structures.

I fully agree. I'll see what I can do.

Meanwhile I'm pushing the C-mouse-1 change since it seems to be in line with the intent of context-menu-mode.

bug#50067: Context menus

[Juri Linkov]

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

>> An example of such menu is mouse-buffer-menu bound to C-<down-mouse-1>
>> where the user might wonder what do these menu items with mode names mean?
>
> OK, but I still don't think I fully understand how context menus are
> different.  Are we sure the user will immediately understand the
> purpose of the context menus, but not of a buffer-menu?

Context menus displayed up by down-mouse-3 are ubiquitous nowadays,
they are everywhere, and the users expect them without a title,
unlike Emacs-specific menus unfamiliar to most users.

>> Using a text property would be a good solution.  Should then
>> the NS-specific "Select" title be removed in menu.c
>> when the title has a special text property?
>
> I'm not sure I understand what is the NS-specific "Select" title.  Can
> you point me to the relevant code?

I meant such patch:


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

diff --git a/lisp/mouse.el b/lisp/mouse.el
index d2a5200d8d..f9355b4f73 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -304,7 +304,7 @@ context-menu-filter-function
 
 (defun context-menu-map ()
   "Return composite menu map."
-  (let ((menu (make-sparse-keymap)))
+  (let ((menu (make-sparse-keymap (propertize "Context Menu" 'hide t))))
     (run-hook-wrapped 'context-menu-functions
                       (lambda (fun)
                         (setq menu (funcall fun menu))
diff --git a/src/menu.c b/src/menu.c
index 3b1d740257..6654c47c93 100644
--- a/src/menu.c
+++ b/src/menu.c
@@ -1284,6 +1284,14 @@ x_popup_menu_1 (Lisp_Object position, Lisp_Object menu)
       /* 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 defined (USE_GTK) || defined (HAVE_NS)
+      if (STRINGP (prompt)
+	  && SCHARS (prompt) > 0
+	  && !NILP (Fget_text_property (make_fixnum (0), Qhide, prompt)))
+	title = Qnil;
+      else
+#endif
       if (!NILP (prompt))
 	title = prompt;
 #ifdef HAVE_NS		/* Is that needed and NS-specific?  --Stef  */

bug#50067: Context menus

[Juri Linkov]

> Meanwhile I'm pushing the C-mouse-1 change since it seems to be
> in line with the intent of context-menu-mode.

On emacs-devel Ergus proposed to add such bindings to a new
context-menu-mode-map.  Do you think this is feasible?

bug#50067: Context menus

[Mattias Engdegård]

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

20 aug. 2021 kl. 09.31 skrev Juri Linkov <juri@linkov.net>:

> If the existing `xref-backend-identifier-at-point` can't be reused
> by adding a new optional arg `click`, then a cleaner solution
> would be to add `xref-backend-identifier-at-mouse` indeed.

`xref-backend-identifier-at-point` is a generic function implemented by each backend; a new function appears cleaner. Suggested patch attached!

More discoveries and questions:

* If I start emacs -Q and enable context-menu-mode, right-clicking on an identifier in an elisp buffer still doesn't produce the Find Definition entry, presumably because xref hasn't been loaded. Shouldn't it be arranged to be autoloaded somehow, which is how xref works when invoked by keystrokes?

* `xref-make-match` requires (contrary to its doc string) its LOCATION argument to be of type `xref-file-location`, but some backends may only be able to make an `xref-buffer-location`. Would anyone object to changing the :location slot of `xref-match-item` to have type `xref-location`? I don't see how it could hurt.


[-- Attachment #2: 0001-Add-Find-References-to-context-menu.patch --]
[-- Type: application/octet-stream, Size: 3167 bytes --]

From 8001c94088d1ee8418001e7fc1875f9cb2ec84f9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 20 Aug 2021 18:19:43 +0200
Subject: [PATCH] Add "Find References" to context menu

The new entry appears next to "Find Definition" and like it only
appears when the context menu was invoked on an identifier.

* lisp/progmodes/prog-mode.el (prog-context-menu): New menu entry.
* lisp/progmodes/xref.el (xref-find-references-at-mouse): New
function, analogous to `xref-find-definitions-at-mouse`.
---
 lisp/progmodes/prog-mode.el | 15 ++++++++++++---
 lisp/progmodes/xref.el      | 14 ++++++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/lisp/progmodes/prog-mode.el b/lisp/progmodes/prog-mode.el
index a8b608b018..89e2eb54d6 100644
--- a/lisp/progmodes/prog-mode.el
+++ b/lisp/progmodes/prog-mode.el
@@ -51,14 +51,23 @@ prog-context-menu
       '(menu-item "Find Definition" xref-find-definitions-at-mouse
                   :visible (save-excursion
                              (mouse-set-point last-input-event)
-                             (xref-backend-identifier-at-point (xref-find-backend)))
-                  :help "Find definition of function or variable")
+                             (xref-backend-identifier-at-point
+                              (xref-find-backend)))
+                  :help "Find definition of identifier")
       'prog-separator)
+    (define-key-after menu [xref-find-ref]
+      '(menu-item "Find References" xref-find-references-at-mouse
+                  :visible (save-excursion
+                             (mouse-set-point last-input-event)
+                             (xref-backend-identifier-at-point
+                              (xref-find-backend)))
+                  :help "Find references to identifier")
+      'xref-find-def)
     (define-key-after menu [xref-pop]
       '(menu-item "Back Definition" xref-pop-marker-stack
                   :visible (not (xref-marker-stack-empty-p))
                   :help "Back to the position of the last search")
-      'xref-find-def))
+      'xref-find-ref))
   menu)
 
 (defvar prog-mode-map
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index b6ad485407..254d00e722 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1349,6 +1349,20 @@ xref-find-definitions-at-mouse
         (xref-find-definitions identifier)
       (user-error "No identifier here"))))
 
+;;;###autoload
+(defun xref-find-references-at-mouse (event)
+  "Find references to the identifier at or around mouse click.
+This command is intended to be bound to a mouse event."
+  (interactive "e")
+  (let ((identifier
+         (save-excursion
+           (mouse-set-point event)
+           (xref-backend-identifier-at-point (xref-find-backend)))))
+    (if identifier
+        (let ((xref-prompt-for-identifier nil))
+          (xref-find-references identifier))
+      (user-error "No identifier here"))))
+
 (declare-function apropos-parse-pattern "apropos" (pattern))
 
 ;;;###autoload
-- 
2.21.1 (Apple Git-122.3)

bug#50067: Context menus

[Mattias Engdegård]

20 aug. 2021 kl. 18.50 skrev Juri Linkov <juri@linkov.net>:

> On emacs-devel Ergus proposed to add such bindings to a new
> context-menu-mode-map.  Do you think this is feasible?

Quite possibly, but the proof is in the pudding.

bug#50067: Context menus

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: mattiase@acm.org,  50067@debbugs.gnu.org,  homeros.misasa@gmail.com,
>   tkk@misasa.okayama-u.ac.jp,  larsi@gnus.org,  alan@idiocy.org
> Date: Fri, 20 Aug 2021 19:36:58 +0300
> 
> > OK, but I still don't think I fully understand how context menus are
> > different.  Are we sure the user will immediately understand the
> > purpose of the context menus, but not of a buffer-menu?
> 
> Context menus displayed up by down-mouse-3 are ubiquitous nowadays,
> they are everywhere, and the users expect them without a title,
> unlike Emacs-specific menus unfamiliar to most users.

So if some menu is popped up by mouse-3, it is automatically
considered ubiquitous and expected?  I really doubt that, but I won't
argue any further.

> >> Using a text property would be a good solution.  Should then
> >> the NS-specific "Select" title be removed in menu.c
> >> when the title has a special text property?
> >
> > I'm not sure I understand what is the NS-specific "Select" title.  Can
> > you point me to the relevant code?
> 
> I meant such patch:

OK, but where in that patch is the NS-specific "Select" title that
should be removed?

bug#50067: Context menus

[Mattias Engdegård]

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

20 aug. 2021 kl. 19.59 skrev Eli Zaretskii <eliz@gnu.org>:

> OK, but where in that patch is the NS-specific "Select" title that
> should be removed?

I'm not Juri, but the following patch removes the default "Select" title used by the NS port. Not sure why it was ever added -- perhaps something Gnustep-specific? Works well with Cocoa in any case.


[-- Attachment #2: 0001-Remove-default-Select-title-from-NS-popup-menus.patch --]
[-- Type: application/octet-stream, Size: 1580 bytes --]

From d08bba26704cbe2cd8c655279d95cfed88ff29ba Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 20 Aug 2021 21:21:05 +0200
Subject: [PATCH] Remove default "Select" title from NS popup menus

* src/menu.c (x_popup_menu_1): Remove default "Select" title.
* src/nsmenu.m (ns_menu_show): Allow title to be absent.
---
 src/menu.c   | 4 ----
 src/nsmenu.m | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/menu.c b/src/menu.c
index 3b1d740257..e441d22ea0 100644
--- a/src/menu.c
+++ b/src/menu.c
@@ -1286,10 +1286,6 @@ x_popup_menu_1 (Lisp_Object position, Lisp_Object menu)
       prompt = Fkeymap_prompt (keymap);
       if (!NILP (prompt))
 	title = prompt;
-#ifdef HAVE_NS		/* Is that needed and NS-specific?  --Stef  */
-      else
-	title = build_string ("Select");
-#endif
 
       /* Make that be the pane title of the first pane.  */
       if (!NILP (prompt) && menu_items_n_panes >= 0)
diff --git a/src/nsmenu.m b/src/nsmenu.m
index bb0dd2634d..fe4f825832 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -959,7 +959,7 @@ - (void)menu:(NSMenu *)menu willHighlightItem:(NSMenuItem *)item
     }
 
   pmenu = [[EmacsMenu alloc] initWithTitle:
-                               [NSString stringWithLispString: title]];
+                   NILP (title) ? @"" : [NSString stringWithLispString: title]];
   [pmenu fillWithWidgetValue: first_wv->contents];
   free_menubar_widget_value_tree (first_wv);
   unbind_to (specpdl_count, Qnil);
-- 
2.21.1 (Apple Git-122.3)


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

bug#50067: Context menus

[Dmitry Gutov]

Hi Mattias!

On 20.08.2021 20:06, Mattias Engdegård wrote:
> * If I start emacs -Q and enable context-menu-mode, right-clicking on an identifier in an elisp buffer still doesn't produce the Find Definition entry, presumably because xref hasn't been loaded. Shouldn't it be arranged to be autoloaded somehow, which is how xref works when invoked by keystrokes?

I wonder what could be the reason for that. It would seem the menu 
should handle autoloaded commands fine. Even the visibility predicate 
should work: xref-find-backend is autoloaded as well.

Try rewriting it with a let, to ensure that xref-find-backend is called 
first:

(let ((backend (xref-find-backend)))
   (xref-backend-identifier-at-point backend))

> * `xref-make-match` requires (contrary to its doc string) its LOCATION argument to be of type `xref-file-location`, but some backends may only be able to make an `xref-buffer-location`. Would anyone object to changing the :location slot of `xref-match-item` to have type `xref-location`? I don't see how it could hurt.

Makes sense to me, seems like an accident. I've done this change 
locally, no obvious bugs fell out.

bug#50067: Context menus

[Tak Kunihiro]

I’m new to 28 and it will take a while to adopt to it.
I’m sending comments so far.

* Binding

I suggest to assign [drag-mouse-3] as well, as shown below.

(define-key map [mouse-3] context-menu-entry)
(define-key map [drag-mouse-3] context-menu-entry)
 
On Mac, I suggest to assign [C-double-mouse-1] as well as shown below.

(define-key map [C-down-mouse-1] #'ignore)
(define-key map [C-mouse-1] context-menu-entry)
(define-key map [C-double-mouse-1] context-menu-entry)
(define-key map [C-triple-mouse-1] context-menu-entry)
(define-key map [C-drag-mouse-1] context-menu-entry)

* Error detection system

It is good if the context-menu system is ready for
an error that occurs on one of context-menu-functions.
For now, when there is error, Emacs only tells
`<mouse-3> is undefined'.

* Long click system

It’s time to discuss long-left-click also as trigger to
show context menu.

https://lists.gnu.org/archive/html/emacs-devel/2017-11/msg00267.html
https://lists.gnu.org/archive/html/emacs-devel/2020-09/msg01277.html

* Open by other frame

File would be opened by this window. Sometimes I want to open it by
other frame.  It is good if which window to open, is selectable after
showing context-menu. I do not know how to do so.

* Details

It is handy to have buffer menu when right click mode bar.

(define-key map [remap buffer-menu-open] context-menu-entry)

Also it is handy to have frame menu when right click title bar.
I do not know how to do so.

Also it is nice to show word candidates by ispell when click a word.

bug#50067: Context menus

[Tak Kunihiro]

** context-menu-region

All commands belong to `Edit' in menu bar.
I think `context-menu-edit' sounds better.

Isn't it better to use `menu-bar-edit-menu' as a source rather than
create a menu from scratch?

On paste when there is a region and delete-selection-mode is t, the
region should be replaced by the text.

** no multiple horizontal lines

Sometimes I see double lines on the context menu.  I think that there is
no useful case to have double lines.  To allow only one horizontal line
would look cool.

** reuse existing menu

I cannot figure out how to include pre-existing menu such for
(mouse-buffer-menu-map).  To lean how to manipulate menu is not easy.
It's nice to have an example something like below (does not work).

  (defun context-menu-buffer (menu)
  "Add a buffer menu entry to MENU."
    (let ((map (mouse-buffer-menu-map)))
      (define-key-after menu [buffer] map))
    menu)

bug#50067: Context menus

[Alan Third]

On Fri, Aug 20, 2021 at 09:29:47PM +0200, Mattias Engdegård wrote:
> 20 aug. 2021 kl. 19.59 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > OK, but where in that patch is the NS-specific "Select" title that
> > should be removed?
> 
> I'm not Juri, but the following patch removes the default "Select"
> title used by the NS port. Not sure why it was ever added -- perhaps
> something Gnustep-specific? Works well with Cocoa in any case.

GNUstep, and I believe NEXTstep and old school macOS, allows you to
"tear off" menus and leave them on screen as sort of custom toolbars.
Hence the title on each menu.

Emacs doesn't support this with the main menus (it was the source of a
crash, so I removed it), but I don't know if it's something we should
support. I suspect not because once Emacs updates the menus it
probably can't handle clicks on old ones.
-- 
Alan Third

bug#50067: Context menus

[Mattias Engdegård]

(Reply to multiple messages)

21 aug. 2021 kl. 11.42 skrev Alan Third <alan@idiocy.org>:

> GNUstep, and I believe NEXTstep and old school macOS, allows you to
> "tear off" menus and leave them on screen as sort of custom toolbars.
> Hence the title on each menu.
> 
> Emacs doesn't support this with the main menus (it was the source of a
> crash, so I removed it), but I don't know if it's something we should
> support. I suspect not because once Emacs updates the menus it
> probably can't handle clicks on old ones.

Thank you, I pushed the removal of the default "Select" title: titles will still be there if given explicitly. If this causes trouble for Gnustep, then we'll reinsert the default title for that platform only.

21 aug. 2021 kl. 01.31 skrev Dmitry Gutov <dgutov@yandex.ru>:

>> * If I start emacs -Q and enable context-menu-mode, right-clicking on an identifier in an elisp buffer still doesn't produce the Find Definition entry, presumably because xref hasn't been loaded. Shouldn't it be arranged to be autoloaded somehow, which is how xref works when invoked by keystrokes?
> 
> I wonder what could be the reason for that. It would seem the menu should handle autoloaded commands fine. Even the visibility predicate should work: xref-find-backend is autoloaded as well.

It was just a (featurep 'xref) test which didn't seem to be needed; as you say, all the functions involved are autoloaded and I have verified that xref will indeed be loaded when the menu is used the first time. Pushed to master.

>> * `xref-make-match` requires (contrary to its doc string) its LOCATION argument to be of type `xref-file-location`, but some backends may only be able to make an `xref-buffer-location`. Would anyone object to changing the :location slot of `xref-match-item` to have type `xref-location`? I don't see how it could hurt.
> 
> Makes sense to me, seems like an accident. I've done this change locally, no obvious bugs fell out.

Thank you, fixed on master.

Also pushed:

* Previous patch that adds "Find References" to the menu; it seemed to be the right thing to do.
* A fringe arrow is now used to indicate the current position in the *xref* buffer
* Messages added to assuage the boredom of users while searching for references. Could be moved, rephrased, made optional or removed altogether, but it did look a lot better than freezing for a long time.

bug#50067: Context menus

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 21 Aug 2021 12:57:23 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, Juri Linkov <juri@linkov.net>,
>         50067@debbugs.gnu.org, Tak Kunihiro <homeros.misasa@gmail.com>,
>         tkk@misasa.okayama-u.ac.jp, Lars Ingebrigtsen <larsi@gnus.org>
> 
> (Reply to multiple messages)
> 
> * A fringe arrow is now used to indicate the current position in the *xref* buffer

I think it may be confusing that the arrow doesn't appear immediately
after M-., only when you switch from the XREF buffer.

Thanks.

bug#50067: Context menus

[Mattias Engdegård]

21 aug. 2021 kl. 13.17 skrev Eli Zaretskii <eliz@gnu.org>:

> I think it may be confusing that the arrow doesn't appear immediately
> after M-., only when you switch from the XREF buffer.

I don't quite follow -- M-. typically doesn't create an *xref* buffer at all; when it does, it does not follow any of the matches found therein.

The arrow indicates the latest match reference that has been followed to its target, in the same way as in compile, grep and occur buffers. In neither of these buffers the arrow appears immediately.

bug#50067: Context menus

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 21 Aug 2021 13:45:08 +0200
> Cc: alan@idiocy.org, juri@linkov.net, 50067@debbugs.gnu.org,
>         homeros.misasa@gmail.com, tkk@misasa.okayama-u.ac.jp, larsi@gnus.org
> 
> 21 aug. 2021 kl. 13.17 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > I think it may be confusing that the arrow doesn't appear immediately
> > after M-., only when you switch from the XREF buffer.
> 
> I don't quite follow -- M-. typically doesn't create an *xref* buffer at all; when it does, it does not follow any of the matches found therein.

Oh, so it's only appearing when the tag is shown?  Confusing... but I
guess we will have to live with that.

> The arrow indicates the latest match reference that has been followed to its target, in the same way as in compile, grep and occur buffers. In neither of these buffers the arrow appears immediately.

Got it.

bug#50067: Context menus

[Juri Linkov]

> I’m new to 28 and it will take a while to adopt to it.
> I’m sending comments so far.

Thanks for the comments.

> * Binding
>
> I suggest to assign [drag-mouse-3] as well, as shown below.
>
> (define-key map [mouse-3] context-menu-entry)
> (define-key map [drag-mouse-3] context-menu-entry)

There is no need to bind [drag-mouse-3] because [down-mouse-3] is bound.

> On Mac, I suggest to assign [C-double-mouse-1] as well as shown below.
>
> (define-key map [C-down-mouse-1] #'ignore)
> (define-key map [C-mouse-1] context-menu-entry)
> (define-key map [C-double-mouse-1] context-menu-entry)
> (define-key map [C-triple-mouse-1] context-menu-entry)
> (define-key map [C-drag-mouse-1] context-menu-entry)

I leave this for the users of Mac to decide what would be better on Mac.

> * Error detection system
>
> It is good if the context-menu system is ready for
> an error that occurs on one of context-menu-functions.
> For now, when there is error, Emacs only tells
> `<mouse-3> is undefined'.

I agree, a better error reporting would be nice.  Patches welcome.

> * Long click system
>
> It’s time to discuss long-left-click also as trigger to
> show context menu.
>
> https://lists.gnu.org/archive/html/emacs-devel/2017-11/msg00267.html
> https://lists.gnu.org/archive/html/emacs-devel/2020-09/msg01277.html

I tried to implement this, but soon found that long-left-click is unusable,
because such artificial delay is a hassle - no one would have
patience to wait even half a second until the menu finally appears.
But this could be implemented anyway when users will demand this.

> * Open by other frame
>
> File would be opened by this window. Sometimes I want to open it by
> other frame.  It is good if which window to open, is selectable after
> showing context-menu. I do not know how to do so.

Sorry, I don't understand what menu do you mean, maybe in Dired mode?

> * Details
>
> It is handy to have buffer menu when right click mode bar.

I don't see where buffer-menu-open currently is used on the mode-line.

> (define-key map [remap buffer-menu-open] context-menu-entry)

This gives the error "void-variable map".

> Also it is handy to have frame menu when right click title bar.

I think this is a good idea.

> I do not know how to do so.

I do not know how to do this for the frame title bar.
But currently I'm doing this for the tab bar.

> Also it is nice to show word candidates by ispell when click a word.

Maybe like flyspell-mode?

> ** context-menu-region
>
> All commands belong to `Edit' in menu bar.
> I think `context-menu-edit' sounds better.

I already tried to use your idea to name it `context-menu-edit'.
But the problem is that other functions like context-menu-undo are also
related to editing.  OTOH, some items in context-menu-region don't do
editing such as "Select All" is not editing, "Copy" is not editing, etc.

> Isn't it better to use `menu-bar-edit-menu' as a source rather than
> create a menu from scratch?

I already tried this idea but this menu is already available
in the global menu that already can be enabled by adding
context-menu-global to context-menu-functions.
But maybe `context-menu-edit' could be added as well
from `menu-bar-edit-menu'.

> On paste when there is a region and delete-selection-mode is t, the
> region should be replaced by the text.

Please explain how the region should be replaced by the text,
when mouse-yank-at-click or mouse-yank-primary is used
to paste where the mouse is clicked.  Should it delete the region
and paste where mouse is clicked on another part of the buffer?
What if the mouse is clicked in another window?

> ** no multiple horizontal lines
>
> Sometimes I see double lines on the context menu.  I think that there is
> no useful case to have double lines.  To allow only one horizontal line
> would look cool.

Right, double separators should be removed.

> ** reuse existing menu
>
> I cannot figure out how to include pre-existing menu such for
> (mouse-buffer-menu-map).  To lean how to manipulate menu is not easy.
> It's nice to have an example something like below (does not work).
>
>   (defun context-menu-buffer (menu)
>   "Add a buffer menu entry to MENU."
>     (let ((map (mouse-buffer-menu-map)))
>       (define-key-after menu [buffer] map))
>     menu)

Adding the existing menu is not easy.  There are some examples in
context-menu-global, context-menu-local, context-menu-minor.

bug#50067: Context menus

[Juri Linkov]

>> >> Using a text property would be a good solution.  Should then
>> >> the NS-specific "Select" title be removed in menu.c
>> >> when the title has a special text property?
>> >
>> > I'm not sure I understand what is the NS-specific "Select" title.  Can
>> > you point me to the relevant code?
>> 
>> I meant such patch:
>
> OK, but where in that patch is the NS-specific "Select" title that
> should be removed?

Mattias pushed the NS-specific patch, and I pushed the text property handling.

bug#50067: Context menus

[Dmitry Gutov]

On 21.08.2021 15:16, Eli Zaretskii wrote:
>>> I think it may be confusing that the arrow doesn't appear immediately
>>> after M-., only when you switch from the XREF buffer.
>> I don't quite follow -- M-. typically doesn't create an*xref*  buffer at all; when it does, it does not follow any of the matches found therein.
> Oh, so it's only appearing when the tag is shown?  Confusing... but I
> guess we will have to live with that.
> 

What is confusing? We do the same in Grep and Compilation.

bug#50067: Context menus

[Eli Zaretskii]

> Cc: alan@idiocy.org, juri@linkov.net, homeros.misasa@gmail.com,
>  tkk@misasa.okayama-u.ac.jp, larsi@gnus.org, 50067@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 22 Aug 2021 22:11:03 +0300
> 
> On 21.08.2021 15:16, Eli Zaretskii wrote:
> >>> I think it may be confusing that the arrow doesn't appear immediately
> >>> after M-., only when you switch from the XREF buffer.
> >> I don't quite follow -- M-. typically doesn't create an*xref*  buffer at all; when it does, it does not follow any of the matches found therein.
> > Oh, so it's only appearing when the tag is shown?  Confusing... but I
> > guess we will have to live with that.
> > 
> 
> What is confusing?

What I described.

> We do the same in Grep and Compilation.

That's not exactly the same, because with M-. I already asked to show
me a definition.  Anyway, not worth an argument.

bug#50067: Context menus

[Dmitry Gutov]

On 22.08.2021 22:22, Eli Zaretskii wrote:
> That's not exactly the same, because with M-. I already asked to show
> me a definition.  Anyway, not worth an argument.

Perhaps your complaint is instead about no definition being shown, even 
though you feel like you already asked Emacs to show one (which the 
fileloop UI does).

bug#50067: Context menus

[Eli Zaretskii]

> Cc: alan@idiocy.org, mattiase@acm.org, juri@linkov.net,
>  homeros.misasa@gmail.com, tkk@misasa.okayama-u.ac.jp, larsi@gnus.org,
>  50067@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 22 Aug 2021 22:54:57 +0300
> 
> On 22.08.2021 22:22, Eli Zaretskii wrote:
> > That's not exactly the same, because with M-. I already asked to show
> > me a definition.  Anyway, not worth an argument.
> 
> Perhaps your complaint is instead about no definition being shown, even 
> though you feel like you already asked Emacs to show one (which the 
> fileloop UI does).

Yes.  But that ship has sailed long ago.

bug#50067: Context menus

[Tak Kunihiro]

>> I suggest to assign [drag-mouse-3] as well, as shown below.
>> 
>> (define-key map [mouse-3] context-menu-entry)
>> (define-key map [drag-mouse-3] context-menu-entry)
> 
> There is no need to bind [drag-mouse-3] because [down-mouse-3] is bound.

I confirmed. I took the suggestion back.

>> On Mac, I suggest to assign [C-double-mouse-1] as well as shown below.
>> 
>> (define-key map [C-down-mouse-1] #'ignore)
>> (define-key map [C-mouse-1] context-menu-entry)
>> (define-key map [C-double-mouse-1] context-menu-entry)
>> (define-key map [C-triple-mouse-1] context-menu-entry)
>> (define-key map [C-drag-mouse-1] context-menu-entry)
> 
> I leave this for the users of Mac to decide what would be better on Mac.

I agree.  User can bind it later too.

>> * Error detection system
>> 
>> It is good if the context-menu system is ready for
>> an error that occurs on one of context-menu-functions.
>> For now, when there is error, Emacs only tells
>> `<mouse-3> is undefined'.
> 
> I agree, a better error reporting would be nice.  Patches welcome.

This is something I wanted to have for a long time.  I do not have
idea where to start. I hope someone works on this using this opportunity.

>> * Long click system
>> 
>> It’s time to discuss long-left-click also as trigger to
>> show context menu.
>> 
>> https://lists.gnu.org/archive/html/emacs-devel/2017-11/msg00267.html
>> https://lists.gnu.org/archive/html/emacs-devel/2020-09/msg01277.html
> 
> I tried to implement this, but soon found that long-left-click is unusable,
> because such artificial delay is a hassle - no one would have
> patience to wait even half a second until the menu finally appears.
> But this could be implemented anyway when users will demand this.

I think this is useful especially on laptop computer with Mac.
As inferred on other posts laptop computer with Mac does not have
mouse-3.  However, it is possible that I’m the only one.

>> * Open by other frame
>> 
>> File would be opened by this window. Sometimes I want to open it by
>> other frame.  It is good if which window to open, is selectable after
>> showing context-menu. I do not know how to do so.
> 
> Sorry, I don't understand what menu do you mean, maybe in Dired mode?

`ffap-at-mouse’ will open a file under pointer on `this window’.
Randomly I want to open it by `other frame’.
It’s good if I can choose open the file on `this window’ or `other frame’.
Is there a way to detect meta key pressed when select one of menu items?

>> * Details
>> 
>> It is handy to have buffer menu when right click mode bar.
> 
> I don't see where buffer-menu-open currently is used on the mode-line.
>> (define-key map [remap buffer-menu-open] context-menu-entry)
> 
> This gives the error "void-variable map".


This suggestion may be unrelated to context-menu.
On mode bar, both mouse-1 and mouse-3 switches buffer. 
I think mouse-3 should show something like (mouse-buffer-menu-map).

>> Also it is handy to have frame menu when right click title bar.
> 
> I think this is a good idea.
> 
>> I do not know how to do so.
> 
> I do not know how to do this for the frame title bar.
> But currently I'm doing this for the tab bar.

OK.

>> Also it is nice to show word candidates by ispell when click a word.
> 
> Maybe like flyspell-mode?

I have an idea. Possibly, send patch in the future.

>> ** context-menu-region
>> 
>> All commands belong to `Edit' in menu bar.
>> I think `context-menu-edit' sounds better.
> 
> I already tried to use your idea to name it `context-menu-edit'.
> But the problem is that other functions like context-menu-undo are also
> related to editing.  OTOH, some items in context-menu-region don't do
> editing such as "Select All" is not editing, "Copy" is not editing, etc.
> 

>> Isn't it better to use `menu-bar-edit-menu' as a source rather than
>> create a menu from scratch?
> 
> I already tried this idea but this menu is already available
> in the global menu that already can be enabled by adding
> context-menu-global to context-menu-functions.
> But maybe `context-menu-edit' could be added as well
> from `menu-bar-edit-menu'.


OK.  I only wanted to suggest reducing maintenance cost.

> 
>> On paste when there is a region and delete-selection-mode is t, the
>> region should be replaced by the text.
> 
> Please explain how the region should be replaced by the text,
> when mouse-yank-at-click or mouse-yank-primary is used
> to paste where the mouse is clicked.  Should it delete the region
> and paste where mouse is clicked on another part of the buffer?
> What if the mouse is clicked in another window?

When there is a region and yank text by `C-y’, the text would
be replaced.

I think when there is a region and point in on region, region
should be replaced by text.  No?

>> ** no multiple horizontal lines
>> 
>> Sometimes I see double lines on the context menu.  I think that there is
>> no useful case to have double lines.  To allow only one horizontal line
>> would look cool.
> 
> Right, double separators should be removed.

OK.

>> ** reuse existing menu
>> 
>> I cannot figure out how to include pre-existing menu such for
>> (mouse-buffer-menu-map).  To lean how to manipulate menu is not easy.
>> It's nice to have an example something like below (does not work).
>> 
>>  (defun context-menu-buffer (menu)
>>  "Add a buffer menu entry to MENU."
>>    (let ((map (mouse-buffer-menu-map)))
>>      (define-key-after menu [buffer] map))
>>    menu)
> 
> Adding the existing menu is not easy.  There are some examples in
> context-menu-global, context-menu-local, context-menu-minor.

OK.  I did not know it is not easy.
I cannot find guid line how the menu should be created.
Do you know where to look?

bug#50067: Context menus

[Juri Linkov]

>> I tried to implement this, but soon found that long-left-click is unusable,
>> because such artificial delay is a hassle - no one would have
>> patience to wait even half a second until the menu finally appears.
>> But this could be implemented anyway when users will demand this.
>
> I think this is useful especially on laptop computer with Mac.
> As inferred on other posts laptop computer with Mac does not have
> mouse-3.  However, it is possible that I’m the only one.

Recently Mattias bound C-mouse-1 to use instead of mouse-3 on Mac.

> `ffap-at-mouse’ will open a file under pointer on `this window’.
> Randomly I want to open it by `other frame’.
> It’s good if I can choose open the file on `this window’ or `other frame’.
> Is there a way to detect meta key pressed when select one of menu items?

There is no way to detect meta key.  But you can create a new command
to open in a new frame, and add this command to the menu.  Or use
the existing `ffap-other-frame'.

>>> It is handy to have buffer menu when right click mode bar.
>
> This suggestion may be unrelated to context-menu.
> On mode bar, both mouse-1 and mouse-3 switches buffer. 
> I think mouse-3 should show something like (mouse-buffer-menu-map).

Yes, this would be useful.

>>> On paste when there is a region and delete-selection-mode is t, the
>>> region should be replaced by the text.
>> 
>> Please explain how the region should be replaced by the text,
>> when mouse-yank-at-click or mouse-yank-primary is used
>> to paste where the mouse is clicked.  Should it delete the region
>> and paste where mouse is clicked on another part of the buffer?
>> What if the mouse is clicked in another window?
>
> When there is a region and yank text by `C-y’, the text would
> be replaced.
>
> I think when there is a region and point in on region, region
> should be replaced by text.  No?

mouse-yank-at-click is intended to paste where you click.
So when there is a region, and you click mouse-3 at some other position
where you want to paste, and select "Paste" from the context-menu,
it's unclear what to do with the region.  It makes no sense
to delete the region, when you paste at the clicked position
outside of the region, but not on the region.

Please see more in mouse-yank-at-click and mouse-yank-primary
that contain such comment about the need to deactivate the region:

  ;; Without this, confusing things happen upon e.g. inserting into
  ;; the middle of an active region.
  (when select-active-regions
    (let (select-active-regions)
      (deactivate-mark)))

>>> I cannot figure out how to include pre-existing menu such for
>>> (mouse-buffer-menu-map).

Actually, the right function to use is (mouse-buffer-menu-keymap)
instead of (mouse-buffer-menu-map).

> I cannot find guid line how the menu should be created.
> Do you know where to look?

Good documentation is in (info "(elisp) Defining Menus").

bug#50067: Context menus

[Dmitry Gutov]

On 23.08.2021 05:21, Eli Zaretskii wrote:
> Yes.  But that ship has sailed long ago.

Only if the chief maintainer thinks that the UI must be frozen in time.

We could add an option, at the very least.

bug#50067: Context menus

[Eli Zaretskii]

> Cc: alan@idiocy.org, mattiase@acm.org, juri@linkov.net,
>  homeros.misasa@gmail.com, tkk@misasa.okayama-u.ac.jp, larsi@gnus.org,
>  50067@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 23 Aug 2021 14:18:56 +0300
> 
> On 23.08.2021 05:21, Eli Zaretskii wrote:
> > Yes.  But that ship has sailed long ago.
> 
> Only if the chief maintainer thinks that the UI must be frozen in time.
> 
> We could add an option, at the very least.

An option to display the first match right away will be most
appreciated, thanks.

bug#50067: Context menus

[Juri Linkov]

>> We could add an option, at the very least.
>
> An option to display the first match right away will be most
> appreciated, thanks.

Like compilation-auto-jump-to-first-error.

bug#50067: Context menus

[Tak Kunihiro]

I started to understand the system.
A plug-in to list frames would be something like this.
I think it is good to have interface to basic utilities such for
recentf and bookmarks.

(defun context-menu-frame (menu)
  "Add MENU a list of frames."
  (let (frame-map)
    (dolist (frame (visible-frame-list))
      (let ((nickname (cdr (assoc 'name (frame-parameters frame))))
            (cmd `(lambda nil (interactive) (funcall 'menu-bar-select-frame ,frame))))
        (push (vector nickname cmd :active (not (equal frame (selected-frame)))) frame-map)))
    (push ["--" ignore] frame-map)
    (push ["New" make-frame-command] frame-map)
    (setq frame-map (reverse frame-map))
    (push "Frames" frame-map)
    (easy-menu-add-item menu nil frame-map)
    menu))

bug#50067: Context menus

[Juri Linkov]

> I started to understand the system.
> A plug-in to list frames would be something like this.
> I think it is good to have interface to basic utilities such for
> recentf and bookmarks.
>
> (defun context-menu-frame (menu)

There are endless possibilities in creating various submenus.
So the default set of submenus is limited only to already existing menus:

context-menu-toolbar copied from the tool-bar
context-menu-global copied from the global menu-bar
context-menu-local copied from the local menu-bar
context-menu-minor copied from the minor-modes menu-bar
context-menu-vc copied from the vc menu
context-menu-undo copied from the Edit menu
context-menu-region copied from the Edit menu

So if you can find an existing menu, it could be added.
But I can't find the existing menu with a list of frames.
There are only the existing menu with a list of buffers
like you proposed (mouse-buffer-menu-map) from [C-down-mouse-1],
but better to use (mouse-buffer-menu-keymap),
so now context-menu-buffers is added to the default list.

The remaining existing menu is 'mouse-appearance-menu' bound to
[S-down-mouse-1].  I don't know if it's significant enough
to be added to the default list?

Also you proposed a good idea to have a frame menu when
right clicking on the title bar.

bug#50067: Context menus

[Dmitry Gutov]

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

On 23.08.2021 19:02, Juri Linkov wrote:
>>> We could add an option, at the very least.
>> An option to display the first match right away will be most
>> appreciated, thanks.
> Like compilation-auto-jump-to-first-error.

So we even have a precedent, very good.

Could you both check out the attached patch?

Together with (setq xref-auto-jump-to-first-definition t)

Questions for feedback:

1. Does the new behavior work okay window management-wise (it does 
occupy +1 window, after all)?

2. Should this setting also extend to other commands like 
xref-find-references? Asking for personal preferences here.

[-- Attachment #2: xref-auto-jump-to-first-definition.diff --]
[-- Type: text/x-patch, Size: 3183 bytes --]

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index d004a0c32c..ca055a36af 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -416,6 +416,12 @@ xref-after-update-hook
   :version "28.1"
   :package-version '(xref . "1.0.4"))
 
+(defcustom xref-auto-jump-to-first-definition nil
+  "If non-nil, `xref-find-definitions' always jumps to the first result."
+  :type 'boolean
+  :version "28.1"
+  :package-version '(xref . "1.2.0"))
+
 (defvar xref--marker-ring (make-ring xref-marker-ring-length)
   "Ring of markers to implement the marker stack.")
 
@@ -1060,19 +1066,30 @@ xref-revert-buffer
            (error-message-string err)
            'face 'error)))))))
 
+(defun xref--auto-jump-first (buf)
+  (select-window (get-buffer-window buf))
+  (goto-char (point-min))
+  (xref-next-line-no-show)
+  (xref-goto-xref))
+
 (defun xref-show-definitions-buffer (fetcher alist)
   "Show the definitions list in a regular window.
 
 When only one definition found, jump to it right away instead."
-  (let ((xrefs (funcall fetcher)))
+  (let ((xrefs (funcall fetcher))
+        buf)
     (cond
      ((not (cdr xrefs))
       (xref-pop-to-location (car xrefs)
                             (assoc-default 'display-action alist)))
      (t
-      (xref--show-xref-buffer fetcher
-                              (cons (cons 'fetched-xrefs xrefs)
-                                    alist))))))
+      (setq buf
+            (xref--show-xref-buffer fetcher
+                                    (cons (cons 'fetched-xrefs xrefs)
+                                          alist)))
+      (when (assoc-default 'auto-jump alist)
+        (xref--auto-jump-first buf))
+      buf))))
 
 (define-obsolete-function-alias
   'xref--show-defs-buffer #'xref-show-definitions-buffer "28.1")
@@ -1088,7 +1105,8 @@ xref-show-definitions-buffer-at-bottom
          ;; XXX: Make percentage customizable maybe?
          (max-height (/ (window-height) 2))
          (size-fun (lambda (window)
-                     (fit-window-to-buffer window max-height))))
+                     (fit-window-to-buffer window max-height)))
+         buf)
     (cond
      ((not (cdr xrefs))
       (xref-pop-to-location (car xrefs)
@@ -1101,7 +1119,10 @@ xref-show-definitions-buffer-at-bottom
         (pop-to-buffer (current-buffer)
                        `(display-buffer-in-direction . ((direction . below)
                                                         (window-height . ,size-fun))))
-        (current-buffer))))))
+        (setq buf (current-buffer)))
+      (when (assoc-default 'auto-jump alist)
+        (xref--auto-jump-first buf))
+      buf))))
 
 (define-obsolete-function-alias 'xref--show-defs-buffer-at-bottom
   #'xref-show-definitions-buffer-at-bottom "28.1")
@@ -1236,7 +1257,8 @@ xref--show-defs
   (xref--push-markers)
   (funcall xref-show-definitions-function xrefs
            `((window . ,(selected-window))
-             (display-action . ,display-action))))
+             (display-action . ,display-action)
+             (auto-jump . ,xref-auto-jump-to-first-definition))))
 
 (defun xref--push-markers ()
   (unless (region-active-p) (push-mark nil t))

bug#50067: Context menus

[Tak Kunihiro]

* flyspell menu

It is handy to show 'flyspell-correct-word when click on typo word.
However, menu is embedded inside of pre existing
'flyspell-correct-word and it is hard to collect menu.  Since
popup-menu interface can accept both menu and function with `e', it's
good to utilize 'flyspell-correct-word.

How about something like below to interrupt evaluation of
context-menu-functions in the middle when one of
context-menu-functions returns symbol instead of menu?

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 6332d9fcec..23f0dda3e8 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -308,7 +308,7 @@ context-menu-map
     (run-hook-wrapped 'context-menu-functions
                       (lambda (fun)
                         (setq menu (funcall fun menu))
-                        nil))
+                        (not (keymapp menu))))
     (when (functionp context-menu-filter-function)
       (setq menu (funcall context-menu-filter-function menu)))
     menu))

Then function something below will pop word choices.

(defun context-menu-spell (menu)
  "Return 'flyspell-correct-word when word under mouse click is incorrect."
  (let ((faces-at-point (mapcar (lambda (xxx) (overlay-get xxx 'face))
                                (overlays-at (posn-point (event-start last-input-event))))))
    (if (or (member 'flyspell-incorrect faces-at-point)
            (member 'flyspell-duplicate faces-at-point))
        #'flyspell-correct-word
      menu)))

(setq context-menu-functions '(context-menu-spell ;; flyspell
                               context-menu-undo
                               context-menu-region
                               context-menu-local
                               context-menu-minor))

bug#50067: Context menus

[Dmitry Gutov]

On 24.08.2021 20:59, Dmitry Gutov wrote:
> 
> Together with (setq xref-auto-jump-to-first-definition t)
> 
> Questions for feedback:
> 
> 1. Does the new behavior work okay window management-wise (it does 
> occupy +1 window, after all)?
> 
> 2. Should this setting also extend to other commands like 
> xref-find-references? Asking for personal preferences here.

Eli?

bug#50067: Context menus

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Cc: alan@idiocy.org, mattiase@acm.org, homeros.misasa@gmail.com,
>  tkk@misasa.okayama-u.ac.jp, larsi@gnus.org, 50067@debbugs.gnu.org
> Date: Wed, 25 Aug 2021 17:15:28 +0300
> 
> On 24.08.2021 20:59, Dmitry Gutov wrote:
> > 
> > Together with (setq xref-auto-jump-to-first-definition t)
> > 
> > Questions for feedback:
> > 
> > 1. Does the new behavior work okay window management-wise (it does 
> > occupy +1 window, after all)?
> > 
> > 2. Should this setting also extend to other commands like 
> > xref-find-references? Asking for personal preferences here.
> 
> Eli?

Sorry, I didn't yet have time to try this; will do tomorrow.

bug#50067: Context menus

[Juri Linkov]

> * flyspell menu
>
> It is handy to show 'flyspell-correct-word when click on typo word.
> However, menu is embedded inside of pre existing
> 'flyspell-correct-word and it is hard to collect menu.  Since
> popup-menu interface can accept both menu and function with `e', it's
> good to utilize 'flyspell-correct-word.
>
> How about something like below to interrupt evaluation of
> context-menu-functions in the middle when one of
> context-menu-functions returns symbol instead of menu?

Thanks, adapting flyspell to use the context menu is our next priority.
But it seems interrupting evaluation is too hackish solution, and it still
uses x-popup-menu.  Would it be possible for flyspell to put its context
function to the end of context-menu-functions, and then replace all
previously added menus with own menu that contains word corrections?

bug#50067: Context menus

[Juri Linkov]

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

>> * flyspell menu
>>
>> It is handy to show 'flyspell-correct-word when click on typo word.
>> However, menu is embedded inside of pre existing
>> 'flyspell-correct-word and it is hard to collect menu.  Since
>> popup-menu interface can accept both menu and function with `e', it's
>> good to utilize 'flyspell-correct-word.
>>
>> How about something like below to interrupt evaluation of
>> context-menu-functions in the middle when one of
>> context-menu-functions returns symbol instead of menu?
>
> Thanks, adapting flyspell to use the context menu is our next priority.
> But it seems interrupting evaluation is too hackish solution, and it still
> uses x-popup-menu.  Would it be possible for flyspell to put its context
> function to the end of context-menu-functions, and then replace all
> previously added menus with own menu that contains word corrections?

Here is the first step that adds support for overlay-local context-menu,
and removes the recently added 'flyspell-use-mouse-3-for-menu'.
But 'flyspell-context-menu' still returns 'flyspell-correct-word'.

I invite you or anyone else to refactor 'flyspell-correct-word' and
related functions to return a keymap menu where every menu item
is bound to a function that calls flyspell-do-correct with an argument
that is a correct word.


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

diff --git a/doc/emacs/fixit.texi b/doc/emacs/fixit.texi
index b558ebc3fd..85cdbff5fa 100644
--- a/doc/emacs/fixit.texi
+++ b/doc/emacs/fixit.texi
@@ -462,10 +462,9 @@ Spelling
   When Flyspell mode highlights a word as misspelled, you can click on
 it with @kbd{mouse-2} (@code{flyspell-correct-word}) to display a menu
 of possible corrections and actions.  If you want this menu on
-@kbd{mouse-3} instead, customize the variable
-@code{flyspell-use-mouse-3-for-menu}.  In addition, @kbd{C-.} or
-@kbd{@key{ESC}-@key{TAB}} (@code{flyspell-auto-correct-word}) will
-propose various successive corrections for the word at point, and
+@kbd{mouse-3} instead, enable @code{context-menu-mode}.  In addition,
+@kbd{C-.} or @kbd{@key{ESC}-@key{TAB}} (@code{flyspell-auto-correct-word})
+will propose various successive corrections for the word at point, and
 @w{@kbd{C-c $}} (@code{flyspell-correct-word-before-point}) will pop
 up a menu of possible corrections.  Of course, you can always correct
 the misspelled word by editing it manually in any way you like.
diff --git a/etc/NEWS b/etc/NEWS
index 04e482364a..a6d9b3a8b8 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2889,8 +2893,7 @@ like 'flymake-mode-line-error-counter',
 When Flyspell mode highlights a word as misspelled, you can click on
 it to display a menu of possible corrections and actions.  You can now
 easily bind this menu to 'down-mouse-3' (usually the right mouse button)
-instead of 'mouse-2' (the default) by customizing the new user option
-'flyspell-use-mouse-3-for-menu'.
+instead of 'mouse-2' (the default) by enabling 'context-menu-mode'.
 
 ---
 *** The current dictionary is now displayed in the minor mode lighter.
diff --git a/lisp/mouse.el b/lisp/mouse.el
index d137419e02..f52fb3f6ba 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -307,10 +307,15 @@ context-menu-filter-function
 (defun context-menu-map ()
   "Return composite menu map."
   (let ((menu (make-sparse-keymap (propertize "Context Menu" 'hide t))))
-    (run-hook-wrapped 'context-menu-functions
-                      (lambda (fun)
-                        (setq menu (funcall fun menu))
-                        nil))
+    (let ((fun (mouse-posn-property (event-start last-input-event)
+                                    'context-menu-function)))
+      (if (functionp fun)
+          (setq menu (funcall fun menu))
+        (run-hook-wrapped 'context-menu-functions
+                          (lambda (fun)
+                            (setq menu (funcall fun menu))
+                            nil))))
+    ;; TODO: remove double separators
     (when (functionp context-menu-filter-function)
       (setq menu (funcall context-menu-filter-function menu)))
     menu))
diff --git a/lisp/textmodes/flyspell.el b/lisp/textmodes/flyspell.el
index 836d889a1c..c05be9655f 100644
--- a/lisp/textmodes/flyspell.el
+++ b/lisp/textmodes/flyspell.el
@@ -442,22 +442,6 @@ flyspell-mode-map
     map)
   "Minor mode keymap for Flyspell mode--for the whole buffer.")
 
-;; correct on mouse 3
-(defun flyspell--set-use-mouse-3-for-menu (var value)
-  (set-default var value)
-  (if value
-      (progn (define-key flyspell-mouse-map [mouse-2] nil)
-             (define-key flyspell-mouse-map [down-mouse-3] 'flyspell-correct-word))
-    (define-key flyspell-mouse-map [mouse-2] 'flyspell-correct-word)
-    (define-key flyspell-mouse-map [down-mouse-3] nil)))
-
-(defcustom flyspell-use-mouse-3-for-menu nil
-  "Non-nil means to bind `mouse-3' to `flyspell-correct-word'.
-If this is set, also unbind `mouse-2'."
-  :type 'boolean
-  :set 'flyspell--set-use-mouse-3-for-menu
-  :version "28.1")
-
 ;; dash character machinery
 (defvar-local flyspell-consider-dash-as-word-delimiter-flag nil
   "Non-nil means that the `-' char is considered as a word delimiter.")
@@ -486,6 +470,10 @@ flyspell-duplicate
 
 (defvar flyspell-overlay nil)
 
+(defun flyspell-context-menu (_menu)
+  "Context menu for `context-menu-mode'."
+  'flyspell-correct-word)
+
 ;;*---------------------------------------------------------------------*/
 ;;*    flyspell-mode ...                                                */
 ;;*---------------------------------------------------------------------*/
@@ -537,10 +525,7 @@ flyspell-mode
   :group 'flyspell
   (if flyspell-mode
       (condition-case err
-          (progn
-            (when flyspell-use-mouse-3-for-menu
-              (flyspell--set-use-mouse-3-for-menu 'flyspell-use-mouse-3-for-menu t))
-            (flyspell-mode-on (called-interactively-p 'interactive)))
+	  (flyspell-mode-on (called-interactively-p 'interactive))
 	(error (message "Error enabling Flyspell mode:\n%s" (cdr err))
 	       (flyspell-mode -1)))
     (flyspell-mode-off)))
@@ -656,8 +641,7 @@ flyspell-mode-on
            show-msg)
       (let* ((binding (where-is-internal 'flyspell-auto-correct-word
                                          nil 'non-ascii))
-             (mouse-button (if flyspell-use-mouse-3-for-menu
-                               "Mouse-3" "Mouse-2")))
+             (mouse-button (if context-menu-mode "Mouse-3" "Mouse-2")))
         (message (format-message
                   "Welcome to Flyspell. Use %s to correct words."
                   (if binding
@@ -1820,13 +1804,15 @@ make-flyspell-overlay
     (overlay-put overlay 'mouse-face mouse-face)
     (overlay-put overlay 'flyspell-overlay t)
     (overlay-put overlay 'evaporate t)
-    (overlay-put overlay 'help-echo (concat (if flyspell-use-mouse-3-for-menu
-                                                "mouse-3"
-                                              "mouse-2") ": correct word at point"))
-    ;; If misspelled text has a 'keymap' property, let that remain in
-    ;; effect for the bindings that flyspell-mouse-map doesn't override.
-    (set-keymap-parent flyspell-mouse-map (get-char-property beg 'keymap))
-    (overlay-put overlay 'keymap flyspell-mouse-map)
+    (overlay-put overlay 'help-echo
+                 (concat (if context-menu-mode "mouse-3" "mouse-2")
+                         ": correct word at point"))
+    (if context-menu-mode
+        (overlay-put overlay 'context-menu-function 'flyspell-context-menu)
+      ;; If misspelled text has a 'keymap' property, let that remain in
+      ;; effect for the bindings that flyspell-mouse-map doesn't override.
+      (set-keymap-parent flyspell-mouse-map (get-char-property beg 'keymap))
+      (overlay-put overlay 'keymap flyspell-mouse-map))
     (when (eq face 'flyspell-incorrect)
       (and (stringp flyspell-before-incorrect-word-string)
            (overlay-put overlay 'before-string

bug#50067: Context menus

[Eli Zaretskii]

> Cc: alan@idiocy.org, mattiase@acm.org, homeros.misasa@gmail.com,
>  tkk@misasa.okayama-u.ac.jp, larsi@gnus.org, 50067@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 24 Aug 2021 20:59:40 +0300
> 
> >> An option to display the first match right away will be most
> >> appreciated, thanks.
> > Like compilation-auto-jump-to-first-error.
> 
> So we even have a precedent, very good.
> 
> Could you both check out the attached patch?
> 
> Together with (setq xref-auto-jump-to-first-definition t)

Thanks, this looks very handy, I will definitely use it.

> Questions for feedback:
> 
> 1. Does the new behavior work okay window management-wise (it does 
> occupy +1 window, after all)?

Not sure I understand the question: we pop up an additional window
when there are more than one candidate even without this option, so
why do you say "+1 window"?  Maybe you had some recipe in mind that I
didn't try?

> 2. Should this setting also extend to other commands like 
> xref-find-references?

Not necessarily.  Perhaps xref-auto-jump-to-first-definition should be
tri-state, to allow users to request the same with
xref-find-references as well?

bug#50067: Context menus

[Dmitry Gutov]

On 26.08.2021 16:01, Eli Zaretskii wrote:
>> Cc: alan@idiocy.org, mattiase@acm.org, homeros.misasa@gmail.com,
>>   tkk@misasa.okayama-u.ac.jp, larsi@gnus.org, 50067@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Tue, 24 Aug 2021 20:59:40 +0300
>>
>>>> An option to display the first match right away will be most
>>>> appreciated, thanks.
>>> Like compilation-auto-jump-to-first-error.
>>
>> So we even have a precedent, very good.
>>
>> Could you both check out the attached patch?
>>
>> Together with (setq xref-auto-jump-to-first-definition t)
> 
> Thanks, this looks very handy, I will definitely use it.

Very good. Let's now discuss a couple of minor alterations. We can 
always go back to this patch if we don't decide on anything better.

I think I remember now why it didn't make sense to me to have this 
behavior OOTB: I think the main goal of the user who calls 
xref-find-definitions is, usually, to pick one definition they wanted to 
visit. Which also means having the xref buffer dismissed at the end.

With the patch under discussion we automatically jump to the first 
location. We can even iterate through locations with 
next-error/previous-error (M-g M-n/M-g M-p). But to close 
(quit/kill/etc) the list of locations, you have to switch back to its 
window and press 'q'. Didn't that look like a bother to you?

Here's how it could look instead:

1. When you press M-., the first location is "shown", but not jumped to. 
The focus remains on the Xref window, with point on its first item (the 
arrow beside it is visible, like you wanted). Location is visible in the 
other window, and we can either visit it and dismiss the Xref buffer 
(with 'C-u RET'), simply visit it with 'RET', or look at the other 
locations with 'n'/'p'.

For this to work, the patch will need to change xref--auto-jump-first, 
swapping

+  (xref-next-line-no-show)
+  (xref-goto-xref))

for

+  (xref-next-line)

The new option's name would probably be different too.

And you could also use a "transient" show-definitions-function like:

(setq xref-show-definitions-function
       #'xref-show-definitions-buffer-at-bottom)

Then you'd only need to press RET in the results buffer to jump and 
dismiss the results buffer.

2. Simply have point move to the first location in the list (rather than 
remain on the group name). From there, the user can press 'C-o' to show 
the location without visiting, or 'RET', or 'C-u RET' like described 
above. I understand this does not fit your prior workflows, but it does 
require the least number of button presses in the scenario "go to the 
first location and dismiss the Xref buffer", especially in combination 
with the (setq xref-show-definitions-function ...) form above.

>> Questions for feedback:
>>
>> 1. Does the new behavior work okay window management-wise (it does
>> occupy +1 window, after all)?
> 
> Not sure I understand the question: we pop up an additional window
> when there are more than one candidate even without this option, so
> why do you say "+1 window"?  Maybe you had some recipe in mind that I
> didn't try?

It's "+1 window" compared to how 'find-tag' worked/works, which I assume 
is the target.

So it's still not the same behavior.

>> 2. Should this setting also extend to other commands like
>> xref-find-references?
> 
> Not necessarily.  Perhaps xref-auto-jump-to-first-definition should be
> tri-state, to allow users to request the same with
> xref-find-references as well?

Sure. Or we can have two variables, especially if we end up cramming 
different variations of behavior into them.

We can do a lot of things. What would help, is better knowledge about 
what people *want* to do.

bug#50067: Context menus

[Dmitry Gutov]

On 27.08.2021 00:05, Dmitry Gutov wrote:
> +  (xref-next-line-no-show)
> +  (xref-goto-xref))
> 
> for
> 
> +  (xref-next-line)

Sorry, missed extra paren:

+  (xref-next-line))

bug#50067: Context menus

[Juri Linkov]

I clicked mouse-3 on the Flymake mode-line indicator expecting a context menu,
but mouse-3 abruptly closed the current buffer.  This can be fixed by such patch:

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index 77a807f21a..cc12fce04a 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -1256,6 +1256,8 @@ flymake--mode-line-title
     ,(let ((map (make-sparse-keymap)))
        (define-key map [mode-line down-mouse-1]
          flymake-menu)
+       (define-key map [mode-line down-mouse-3]
+         flymake-menu)
        (define-key map [mode-line mouse-2]
          (lambda ()
            (interactive)

bug#50067: Context menus

[Juri Linkov]

> Here is the first step that adds support for overlay-local context-menu,
> and removes the recently added 'flyspell-use-mouse-3-for-menu'.
> But 'flyspell-context-menu' still returns 'flyspell-correct-word'.

Now pushed.

> I invite you or anyone else to refactor 'flyspell-correct-word' and
> related functions to return a keymap menu where every menu item
> is bound to a function that calls flyspell-do-correct with an argument
> that is a correct word.

Any help is appreciated to create a menu keymap from misspelled words.

bug#50067: Context menus

[Eli Zaretskii]

> Cc: juri@linkov.net, alan@idiocy.org, mattiase@acm.org,
>  homeros.misasa@gmail.com, tkk@misasa.okayama-u.ac.jp, larsi@gnus.org,
>  50067@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 27 Aug 2021 00:05:39 +0300
> 
> I think I remember now why it didn't make sense to me to have this 
> behavior OOTB: I think the main goal of the user who calls 
> xref-find-definitions is, usually, to pick one definition they wanted to 
> visit. Which also means having the xref buffer dismissed at the end.

That's one use case.  Another use case is when the candidates are all
related to some issue the user is working on, and therefore leaving
the xref buffer displayed for a long time is what they want.

> With the patch under discussion we automatically jump to the first 
> location. We can even iterate through locations with 
> next-error/previous-error (M-g M-n/M-g M-p). But to close 
> (quit/kill/etc) the list of locations, you have to switch back to its 
> window and press 'q'. Didn't that look like a bother to you?

No.  In my case, I just never bother to dismiss the xref buffer.  The
window showing it is a small one, and sooner or later the xref buffer
gets replaced by *Help* or ChangeLog or one of the other buffers I
display at the bottom of the frame.

> Here's how it could look instead:
> 
> 1. When you press M-., the first location is "shown", but not jumped to. 
> The focus remains on the Xref window, with point on its first item (the 
> arrow beside it is visible, like you wanted). Location is visible in the 
> other window, and we can either visit it and dismiss the Xref buffer 
> (with 'C-u RET'), simply visit it with 'RET', or look at the other 
> locations with 'n'/'p'.

This AFAIU corresponds to the situation where the user is not certain
which of the candidates is the one he/she wants.  I don't see how it
fundamentally differs from the original patch, since "M-g M-n" (or
"C-x `", which is what I use) isn't less convenient than 'n' followed
by "C-x o".  It might be more convenient to those who like to dismiss
the xref buffer, but (a) I'm not one of them, and (b) one can dismiss
it without going into it with "C-x 4 C-o".

> And you could also use a "transient" show-definitions-function like:
> 
> (setq xref-show-definitions-function
>        #'xref-show-definitions-buffer-at-bottom)
> 
> Then you'd only need to press RET in the results buffer to jump and 
> dismiss the results buffer.

Do a lot of people really like to dismiss the xref buffer?

In any case, I think questions about this aspect are better answered
by someone who does like to dismiss the buffer, because the issue
simply doesn't bother me enough to give you any useful input.

> 2. Simply have point move to the first location in the list (rather than 
> remain on the group name). From there, the user can press 'C-o' to show 
> the location without visiting, or 'RET', or 'C-u RET' like described 
> above. I understand this does not fit your prior workflows, but it does 
> require the least number of button presses in the scenario "go to the 
> first location and dismiss the Xref buffer", especially in combination 
> with the (setq xref-show-definitions-function ...) form above.

That's just a minor change in what we have now.  I don't object to
such a change, not even by default, but it isn't what we were
discussing until now.

> >> 1. Does the new behavior work okay window management-wise (it does
> >> occupy +1 window, after all)?
> > 
> > Not sure I understand the question: we pop up an additional window
> > when there are more than one candidate even without this option, so
> > why do you say "+1 window"?  Maybe you had some recipe in mind that I
> > didn't try?
> 
> It's "+1 window" compared to how 'find-tag' worked/works, which I assume 
> is the target.

No, I think xref is actually an improvement in this department,
because it shows the list of candidates instead of letting the user
guess how many are there.

> >> 2. Should this setting also extend to other commands like
> >> xref-find-references?
> > 
> > Not necessarily.  Perhaps xref-auto-jump-to-first-definition should be
> > tri-state, to allow users to request the same with
> > xref-find-references as well?
> 
> Sure. Or we can have two variables, especially if we end up cramming 
> different variations of behavior into them.
> 
> We can do a lot of things. What would help, is better knowledge about 
> what people *want* to do.

If we don't want to take a guess, I'd suggest leaving the option as it
is, affecting only xref-find-definitions, and extend it to other
commands as user requests arrive.

bug#50067: Context menus

[Tak Kunihiro]

>> I invite you or anyone else to refactor 'flyspell-correct-word' and
>> related functions to return a keymap menu where every menu item
>> is bound to a function that calls flyspell-do-correct with an argument
>> that is a correct word.
>
> Any help is appreciated to create a menu keymap from misspelled words.

OK.  I understand the task.  I will try.

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

[Dmitry Gutov]

On 27.08.2021 09:26, Eli Zaretskii wrote:
>> Cc: juri@linkov.net, alan@idiocy.org, mattiase@acm.org,
>>   homeros.misasa@gmail.com, tkk@misasa.okayama-u.ac.jp, larsi@gnus.org,
>>   50067@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Fri, 27 Aug 2021 00:05:39 +0300
>>
>> I think I remember now why it didn't make sense to me to have this
>> behavior OOTB: I think the main goal of the user who calls
>> xref-find-definitions is, usually, to pick one definition they wanted to
>> visit. Which also means having the xref buffer dismissed at the end.
> 
> That's one use case.  Another use case is when the candidates are all
> related to some issue the user is working on, and therefore leaving
> the xref buffer displayed for a long time is what they want.

Fair enough.

>> With the patch under discussion we automatically jump to the first
>> location. We can even iterate through locations with
>> next-error/previous-error (M-g M-n/M-g M-p). But to close
>> (quit/kill/etc) the list of locations, you have to switch back to its
>> window and press 'q'. Didn't that look like a bother to you?
> 
> No.  In my case, I just never bother to dismiss the xref buffer.  The
> window showing it is a small one, and sooner or later the xref buffer
> gets replaced by *Help* or ChangeLog or one of the other buffers I
> display at the bottom of the frame.

I see.

This does not correspond to my usage and expectations, but, fingers 
crossed, this addition will satisfy the needs of other former users of 
'find-tag' as well.

>> Here's how it could look instead:
>>
>> 1. When you press M-., the first location is "shown", but not jumped to.
>> The focus remains on the Xref window, with point on its first item (the
>> arrow beside it is visible, like you wanted). Location is visible in the
>> other window, and we can either visit it and dismiss the Xref buffer
>> (with 'C-u RET'), simply visit it with 'RET', or look at the other
>> locations with 'n'/'p'.
> 
> This AFAIU corresponds to the situation where the user is not certain
> which of the candidates is the one he/she wants.  I don't see how it
> fundamentally differs from the original patch, since "M-g M-n" (or
> "C-x `", which is what I use) isn't less convenient than 'n' followed
> by "C-x o".  It might be more convenient to those who like to dismiss
> the xref buffer, but (a) I'm not one of them, and (b) one can dismiss
> it without going into it with "C-x 4 C-o".

All right. You still prefer the original patch, then?

>>>> 1. Does the new behavior work okay window management-wise (it does
>>>> occupy +1 window, after all)?
>>>
>>> Not sure I understand the question: we pop up an additional window
>>> when there are more than one candidate even without this option, so
>>> why do you say "+1 window"?  Maybe you had some recipe in mind that I
>>> didn't try?
>>
>> It's "+1 window" compared to how 'find-tag' worked/works, which I assume
>> is the target.
> 
> No, I think xref is actually an improvement in this department,
> because it shows the list of candidates instead of letting the user
> guess how many are there.

Cool.

>>>> 2. Should this setting also extend to other commands like
>>>> xref-find-references?
>>>
>>> Not necessarily.  Perhaps xref-auto-jump-to-first-definition should be
>>> tri-state, to allow users to request the same with
>>> xref-find-references as well?
>>
>> Sure. Or we can have two variables, especially if we end up cramming
>> different variations of behavior into them.
>>
>> We can do a lot of things. What would help, is better knowledge about
>> what people *want* to do.
> 
> If we don't want to take a guess, I'd suggest leaving the option as it
> is, affecting only xref-find-definitions, and extend it to other
> commands as user requests arrive.

All right.

bug#50067: Context menus

[Eli Zaretskii]

> Cc: juri@linkov.net, alan@idiocy.org, mattiase@acm.org,
>  homeros.misasa@gmail.com, tkk@misasa.okayama-u.ac.jp, larsi@gnus.org,
>  50067@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 30 Aug 2021 05:45:07 +0300
> 
> All right. You still prefer the original patch, then?

Yes, the original patch satisfies my needs, AFAICT.  It would be good
to hear from Juri as well, though.

Thanks.

bug#50067: Context menus

[Juri Linkov]

>> All right. You still prefer the original patch, then?
>
> Yes, the original patch satisfies my needs, AFAICT.  It would be good
> to hear from Juri as well, though.

Sorry, I can't find the original patch.  This bug#50067 collected
so many different things, so it's now hard to find anything.

bug#50067: Context menus

[Dmitry Gutov]

On 31.08.2021 10:05, Juri Linkov wrote:
>>> All right. You still prefer the original patch, then?
>> Yes, the original patch satisfies my needs, AFAICT.  It would be good
>> to hear from Juri as well, though.
> Sorry, I can't find the original patch.  This bug#50067 collected
> so many different things, so it's now hard to find anything.

Here it is: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=50067#183

bug#50067: Context menus

[Juri Linkov]

>>>> All right. You still prefer the original patch, then?
>>> Yes, the original patch satisfies my needs, AFAICT.  It would be good
>>> to hear from Juri as well, though.
>> Sorry, I can't find the original patch.  This bug#50067 collected
>> so many different things, so it's now hard to find anything.
>
> Here it is: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=50067#183

Oh, I thought there were two patches: the patch above and also
the original patch.  But it the above is the original patch,
then I'd recommend to add more already discussed additions,
so the new option could provide at least these choices:

1. 'jump': Jump to the first location by selecting its window;
2. 'show': Show the first item, but keep focus in the Xref window;
3. 'move': Move point to the first item in the Xref window
   without showing it (maybe this should be the default behavior).

To be able to add later a similar variable for xref-find-references,
the new variable for xref-find-definitions could be named accordingly
with a name that refers to xref-find-definitions, for example,
xref-find-definitions-auto-jump.  Then later xref-find-references-auto-jump.

bug#50067: Context menus

[Juri Linkov]

>>>> On paste when there is a region and delete-selection-mode is t, the
>>>> region should be replaced by the text.
>>>
>>> Please explain how the region should be replaced by the text,
>>> when mouse-yank-at-click or mouse-yank-primary is used
>>> to paste where the mouse is clicked.  Should it delete the region
>>> and paste where mouse is clicked on another part of the buffer?
>>> What if the mouse is clicked in another window?
>>
>> When there is a region and yank text by `C-y’, the text would
>> be replaced.
>>
>> I think when there is a region and point in on region, region
>> should be replaced by text.  No?
>
> mouse-yank-at-click is intended to paste where you click.
> So when there is a region, and you click mouse-3 at some other position
> where you want to paste, and select "Paste" from the context-menu,
> it's unclear what to do with the region.  It makes no sense
> to delete the region, when you paste at the clicked position
> outside of the region, but not on the region.
>
> Please see more in mouse-yank-at-click and mouse-yank-primary
> that contain such comment about the need to deactivate the region:
>
>   ;; Without this, confusing things happen upon e.g. inserting into
>   ;; the middle of an active region.
>   (when select-active-regions
>     (let (select-active-regions)
>       (deactivate-mark)))

But maybe still it should delete the region before yanking?
I suggest to try these settings:

  (put 'mouse-yank-primary 'delete-selection 'yank)
  (put 'mouse-yank-at-click 'delete-selection 'yank)
  (put 'menu-bar-select-yank 'delete-selection 'yank)

Then maybe they should be added to delsel.el.

bug#50067: Context menus

[Juri Linkov]

>>> ** no multiple horizontal lines
>>>
>>> Sometimes I see double lines on the context menu.  I think that there is
>>> no useful case to have double lines.  To allow only one horizontal line
>>> would look cool.
>>
>> Right, double separators should be removed.
>
> OK.

This is not easy to do.  Because there are menu-items
that use the filter :visible, e.g.:

  (defun prog-context-menu (menu)
    (require 'xref)
    (define-key-after menu [prog-separator] menu-bar-separator
      'mark-whole-buffer)
    (define-key-after menu [xref-find-def]
      '(menu-item "Find Definition" xref-find-definitions-at-mouse
                  :visible (save-excursion
                             (mouse-set-point last-input-event)
                             (xref-backend-identifier-at-point
                              (xref-find-backend)))
                  :help "Find definition of identifier")
      'prog-separator)

So it's not known whether the menu-item will be displayed
until the menu is displayed.  So there is no way to remove
a stray separator in the context-menu function that creates
the context menu.

Maybe such items should be rewritten to avoid the filter :visible,
e.g.:

  (defun prog-context-menu (menu)
    (require 'xref)
    (define-key-after menu [prog-separator] menu-bar-separator
      'mark-whole-buffer)
    (when (save-excursion
            (mouse-set-point last-input-event)
            (xref-backend-identifier-at-point
             (xref-find-backend)))
      (define-key-after menu [xref-find-def]
        '(menu-item "Find Definition" xref-find-definitions-at-mouse
                    :help "Find definition of identifier")
        'prog-separator))

bug#50067: Context menus

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Alan Third <alan@idiocy.org>,  Mattias Engdegård
>  <mattiase@acm.org>,
>   Lars Ingebrigtsen <larsi@gnus.org>,  50067@debbugs.gnu.org,  Eli
>  Zaretskii <eliz@gnu.org>
> Date: Tue, 31 Aug 2021 20:43:36 +0300
> 
> >>> ** no multiple horizontal lines
> >>>
> >>> Sometimes I see double lines on the context menu.  I think that there is
> >>> no useful case to have double lines.  To allow only one horizontal line
> >>> would look cool.
> >>
> >> Right, double separators should be removed.
> >
> > OK.
> 
> This is not easy to do.  Because there are menu-items
> that use the filter :visible, e.g.:

Can I ask why do we need the separators in the context menus?  Why not
remove them all, unconditionally?

bug#50067: Context menus

[Dmitry Gutov]

On 31.08.2021 19:56, Juri Linkov wrote:
>>>>> All right. You still prefer the original patch, then?
>>>> Yes, the original patch satisfies my needs, AFAICT.  It would be good
>>>> to hear from Juri as well, though.
>>> Sorry, I can't find the original patch.  This bug#50067 collected
>>> so many different things, so it's now hard to find anything.
>>
>> Here it is: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=50067#183
> 
> Oh, I thought there were two patches: the patch above and also
> the original patch.  But it the above is the original patch,
> then I'd recommend to add more already discussed additions,
> so the new option could provide at least these choices:
> 
> 1. 'jump': Jump to the first location by selecting its window;
> 2. 'show': Show the first item, but keep focus in the Xref window;
> 3. 'move': Move point to the first item in the Xref window
>     without showing it (maybe this should be the default behavior).

We can do that. Do you expect to be using more than 1 of these values 
yourself, or is it just for completeness?

Regarding 3 as default, it makes a certain sense, but then you won't be 
able to iterate through all locations with just 'n'. You'd have to press 
'C-o' and then 'n', 'n', 'n'...

> To be able to add later a similar variable for xref-find-references,
> the new variable for xref-find-definitions could be named accordingly
> with a name that refers to xref-find-definitions, for example,
> xref-find-definitions-auto-jump.  Then later xref-find-references-auto-jump.

I think the "other" variable would be called something with the word 
"xrefs", not "references", and apply to other commands as well, such as 
xref-find-apropos and project-find-regexp.

So the proposed scheme would not quite work. Maybe like this instead?

  - xref-auto-jump-to-first-definition
  - xref-auto-jump-to-first-<xref/item/match/result>. Or just 
xref-auto-jump-to-first.

bug#50067: Context menus

[Juri Linkov]

>> so the new option could provide at least these choices:
>> 1. 'jump': Jump to the first location by selecting its window;
>> 2. 'show': Show the first item, but keep focus in the Xref window;
>> 3. 'move': Move point to the first item in the Xref window
>>     without showing it (maybe this should be the default behavior).
>
> We can do that. Do you expect to be using more than 1 of these values
> yourself, or is it just for completeness?

A good indication that all values are needed is that I still can't decide
which to use, so it would be possible to switch to another value when
one of them does too much or too little.

> Regarding 3 as default, it makes a certain sense, but then you won't be
> able to iterate through all locations with just 'n'. You'd have to press
> 'C-o' and then 'n', 'n', 'n'...

I agree, it should not be the default.

>> To be able to add later a similar variable for xref-find-references,
>> the new variable for xref-find-definitions could be named accordingly
>> with a name that refers to xref-find-definitions, for example,
>> xref-find-definitions-auto-jump.  Then later xref-find-references-auto-jump.
>
> I think the "other" variable would be called something with the word
> "xrefs", not "references", and apply to other commands as well, such as
> xref-find-apropos and project-find-regexp.
>
> So the proposed scheme would not quite work. Maybe like this instead?
>
>  - xref-auto-jump-to-first-definition
>  - xref-auto-jump-to-first-<xref/item/match/result>. Or just
>    xref-auto-jump-to-first.

grep/compilation already supports the value 'jump' by non-nil
'compilation-auto-jump-to-first-error', and the value 'move' when
'compilation-scroll-output' is customized to 'first-error'.

But I think for xref still two separate options are needed
xref-auto-jump-to-first-definition for xref-find-definitions, and
xref-auto-jump-to-first-xref for other more grep-like xref commands.

BTW, I'm testing compilation errors/warnings on xref context menu,
and it reposts this warning:

  prog-mode.el:60:12: Warning: the function `xref-backend-identifier-at-point'
      is not known to be defined.

on this code:

  (defun prog-context-menu (menu)
    (require 'xref)
    (define-key-after menu [prog-separator] menu-bar-separator
      'mark-whole-buffer)
    (when (save-excursion
            (mouse-set-point last-input-event)
            (xref-backend-identifier-at-point
             (xref-find-backend)))
      (define-key-after menu [xref-find-def]
        '(menu-item "Find Definition" xref-find-definitions-at-mouse
                    :help "Find definition of identifier")
        'prog-separator))

Maybe `xref-backend-identifier-at-point' should be autoloaded?

bug#50067: Context menus

[Juri Linkov]

>> >>> ** no multiple horizontal lines
>> >>>
>> >>> Sometimes I see double lines on the context menu.  I think that there is
>> >>> no useful case to have double lines.  To allow only one horizontal line
>> >>> would look cool.
>> >>
>> >> Right, double separators should be removed.
>> >
>> > OK.
>>
>> This is not easy to do.  Because there are menu-items
>> that use the filter :visible, e.g.:
>
> Can I ask why do we need the separators in the context menus?  Why not
> remove them all, unconditionally?

The separators are of great help to provide visual cues for groups
of objects.  All menus on the menu-bar in Emacs use separators.
Anyway, this is implemented now:

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 7d3ed9a0e4..8ac22a07c9 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -315,7 +315,14 @@ context-menu-map
                           (lambda (fun)
                             (setq menu (funcall fun menu))
                             nil))))
-    ;; TODO: remove double separators
+    ;; Remove double separators
+    (let ((l menu))
+      (while l
+        (when (and (equal (cdr-safe (car l)) menu-bar-separator)
+                   (equal (cdr-safe (cadr l)) menu-bar-separator))
+          (setcdr l (cddr l)))
+        (setq l (cdr l))))
+
     (when (functionp context-menu-filter-function)
       (setq menu (funcall context-menu-filter-function menu)))
     menu))

bug#50067: Context menus

[Dmitry Gutov]

On 01.09.2021 10:08, Juri Linkov wrote:
> BTW, I'm testing compilation errors/warnings on xref context menu,
> and it reposts this warning:
> 
>    prog-mode.el:60:12: Warning: the function `xref-backend-identifier-at-point'
>        is not known to be defined.
> 
> on this code:
> 
>    (defun prog-context-menu (menu)
>      (require 'xref)
>      (define-key-after menu [prog-separator] menu-bar-separator
>        'mark-whole-buffer)
>      (when (save-excursion
>              (mouse-set-point last-input-event)
>              (xref-backend-identifier-at-point
>               (xref-find-backend)))
>        (define-key-after menu [xref-find-def]
>          '(menu-item "Find Definition" xref-find-definitions-at-mouse
>                      :help "Find definition of identifier")
>          'prog-separator))
> 
> Maybe `xref-backend-identifier-at-point' should be autoloaded?

I don't know. What are the rules?

There is no risk to call it before xref.el is loaded because 
xref-find-backend itself is autoloaded.

But the lack of autoloading can cause byte-compilation warnings. They 
can be dealt with by other means too, though.

bug#50067: Context menus

[Dmitry Gutov]

On 01.09.2021 10:08, Juri Linkov wrote:
>>> so the new option could provide at least these choices:
>>> 1. 'jump': Jump to the first location by selecting its window;
>>> 2. 'show': Show the first item, but keep focus in the Xref window;
>>> 3. 'move': Move point to the first item in the Xref window
>>>      without showing it (maybe this should be the default behavior).
>>
>> We can do that. Do you expect to be using more than 1 of these values
>> yourself, or is it just for completeness?
> 
> A good indication that all values are needed is that I still can't decide
> which to use, so it would be possible to switch to another value when
> one of them does too much or too little.
> 
>> Regarding 3 as default, it makes a certain sense, but then you won't be
>> able to iterate through all locations with just 'n'. You'd have to press
>> 'C-o' and then 'n', 'n', 'n'...
> 
> I agree, it should not be the default.
> 
>>> To be able to add later a similar variable for xref-find-references,
>>> the new variable for xref-find-definitions could be named accordingly
>>> with a name that refers to xref-find-definitions, for example,
>>> xref-find-definitions-auto-jump.  Then later xref-find-references-auto-jump.
>>
>> I think the "other" variable would be called something with the word
>> "xrefs", not "references", and apply to other commands as well, such as
>> xref-find-apropos and project-find-regexp.
>>
>> So the proposed scheme would not quite work. Maybe like this instead?
>>
>>   - xref-auto-jump-to-first-definition
>>   - xref-auto-jump-to-first-<xref/item/match/result>. Or just
>>     xref-auto-jump-to-first.
> 
> grep/compilation already supports the value 'jump' by non-nil
> 'compilation-auto-jump-to-first-error', and the value 'move' when
> 'compilation-scroll-output' is customized to 'first-error'.
> 
> But I think for xref still two separate options are needed
> xref-auto-jump-to-first-definition for xref-find-definitions, and
> xref-auto-jump-to-first-xref for other more grep-like xref commands.

It's now in master: two variables, three methods of behavior for each.

Let me know if you see any problems.

bug#50067: Context menus

[Juri Linkov]

>>>   - xref-auto-jump-to-first-definition
>>>   - xref-auto-jump-to-first-<xref/item/match/result>. Or just
>>>     xref-auto-jump-to-first.
>> grep/compilation already supports the value 'jump' by non-nil
>> 'compilation-auto-jump-to-first-error', and the value 'move' when
>> 'compilation-scroll-output' is customized to 'first-error'.
>> But I think for xref still two separate options are needed
>> xref-auto-jump-to-first-definition for xref-find-definitions, and
>> xref-auto-jump-to-first-xref for other more grep-like xref commands.
>
> It's now in master: two variables, three methods of behavior for each.
>
> Let me know if you see any problems.

Thanks, one problem is that defcustoms have no '(const nil :tag'
for the default value 'nil'.  I don't know what tag to use,
maybe "No auto-jump".  There were other problems so that it was
easier to just push the fix :)

bug#50067: Context menus

[Dmitry Gutov]

On 05.09.2021 11:37, Juri Linkov wrote:
> Thanks, one problem is that defcustoms have no '(const nil :tag'
> for the default value 'nil'.  I don't know what tag to use,
> maybe "No auto-jump".  There were other problems so that it was
> easier to just push the fix:)

Thanks! I really skipped on testing the Customize behavior.

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: 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: 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.