bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

[Jim Porter]

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

Sometimes, menu separators are doubled up in context-menu-mode. To see 
this in action:

   emacs -Q --eval '(context-menu-mode)'
   ;; Right-click somewhere in the scratch buffer, like the empty area.

You should see a double separator just before "Lisp-Interaction". 
`context-menu-map' tries to eliminate doubled separators, but it doesn't 
account for *tripled* separators. If we have a menu like so ("->" is the 
current list item when iterating):

-> (separator-foo "--")
    (separator-bar "--")
    (separator-baz "--")
    (regular-item "Item")

The duplicate remover sees that both the current item and the next are 
separators, so removes it:

-> (separator-foo "--")
    (separator-baz "--")
    (regular-item "Item")

But then the very next operation is to move to the next list item:

    (separator-foo "--")
-> (separator-baz "--")
    (regular-item "Item")

Now, on the next iteration of the while loop, it won't detect the 
duplicate because it's too far ahead. Attached is a patch to fix this; 
it only advances to the next list item when it *didn't* just delete a 
duplicate separator. That way, it can keep deleting subsequent dupes 
until it sees a non-separator item.

I've only tested this on Emacs 29 so far, but it may occur on Emacs 28 
as well. Also, I noticed that separators can appear at the beginning 
and/or end of the context menu. Should they be removed too?

[-- Attachment #2: 0001-Ensure-there-are-no-duplicate-separators-when-creati.patch --]
[-- Type: text/plain, Size: 1255 bytes --]

From b5983b920eba32fa944758f510cb5fee6c281d4b Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 1 Dec 2021 21:55:31 -0800
Subject: [PATCH] Ensure there are no duplicate separators when creating a
 context menu

Previously, if there were three or more consecutive menu separators,
not all of them would be removed.

* lisp/mouse.el (context-menu-map): Ensure no duplicate separators.
---
 lisp/mouse.el | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 3ab9fbcdfe..ec43aecdd0 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -330,10 +330,10 @@ context-menu-map
     ;; Remove duplicate separators
     (let ((l menu))
       (while (consp 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))))
+        (if (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 click)))
-- 
2.25.1

bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

> I've only tested this on Emacs 29 so far, but it may occur on Emacs 28
> as well.

Thanks; applied to Emacs 29.

> Also, I noticed that separators can appear at the beginning
> and/or end of the context menu. Should they be removed too?

Hm...  I think that might make sense, but I'm not sure.  Anybody got an
opinion?

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

bug#52237: [External] : bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

[Drew Adams]

Why should any repetition of separators be removed
or ignored?

If someone codes that then they presumably want that.
Vanilla Emacs need not code such repetition.


Runtime "fixes" of such repetition should be a no-no.
If you don't want consecutive separators then don't
use them.  End of story.

If Emacs automatically ignores or removes separators
then that behavior is too clever by half, IMHO.

(Apologies, if I've misunderstood what this feature,
"`context-menu-map' tries to eliminate doubled
separators", is really about.  But if I've understood
correctly then I suggest this control is misguided.)

bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

[Juri Linkov]

>> I've only tested this on Emacs 29 so far, but it may occur on Emacs 28
>> as well.
>
> Thanks; applied to Emacs 29.

Why not to Emacs 28?

>> Also, I noticed that separators can appear at the beginning
>> and/or end of the context menu. Should they be removed too?
>
> Hm...  I think that might make sense, but I'm not sure.  Anybody got an
> opinion?

Removing leading/trailing separators would be nice too.

bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

[Jim Porter]

On 12/2/2021 9:31 AM, Drew Adams wrote:
> Why should any repetition of separators be removed
> or ignored?
> 
> If someone codes that then they presumably want that.
> Vanilla Emacs need not code such repetition.

As far as I understand (which isn't very far; I've only just started 
tinkering with context-menu-mode), the general idea is that the context 
menu is generated dynamically by a list of functions stored in 
`context-menu-functions'. Each of these can add items to the menu. Some 
of these, like `context-menu-minor', first add a separator and then 
iterate over a list of things (minor modes in this case) to add more 
items. If that list is empty, you just get a separator, but then that 
separator might get doubled up with the separator from the *next* 
context menu function.

In some cases, these separators are used as anchors to determine where 
to put the results of *later* context menu functions too. For example, 
`context-menu-middle-separator' is one of the default entries in 
`context-menu-functions', and as the name implies, it *only* adds a 
separator. Some other context menu functions (e.g. `elisp-context-menu') 
look for that separator to know where to put new menu items, so we want 
that separator to be there during construction, even if it might result 
in duplicated separators by the end (which `context-menu-map' would then 
strip out before display). This logic could apply to separators 
generated by other functions too, such as `context-menu-minor' described 
above.

bug#52237: [External] : Re: bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

[Drew Adams]

I see.  In that case, maybe there should be
an enhancement request, to be able to produce
consecutive separators in a context menu.

(No, I'm not requesting that, myself.)

bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

[Jim Porter]

On 12/1/2021 10:06 PM, Jim Porter wrote:
> Sometimes, menu separators are doubled up in context-menu-mode.

I found another odd case, but I'm not 100% sure the best way to fix it:

   emacs -Q --eval '(context-menu-mode)'
   C-h o identity RET
   ;; Right-click somewhere in the Help buffer

There's a doubled separator after "Next Topic". Looking at the code, 
this is because `help-mode-context-menu' inserts new items using 
`define-key', which has the effect of putting the new items *before* the 
(hidden) menu title. The resulting keymap ends up looking like this:

   (keymap
    (Previous\ Topic ...)
    (Next\ Topic ...)
    (help-mode-separator "--")
    #("Context Menu" 0 12 (hide t))
    (separator-undo "--")
    ...)

Since there's a hidden item (the keymap title) between the 
`help-mode-separator' and `separator-undo'[1], the de-duplication 
doesn't handle that. Is there a standard/common way of defining a key 
such that it goes immediately *after* the keymap's title? I guess we 
could add `context-menu-top-separator' as an anchor (by analogy to 
`context-menu-middle-separator'), but maybe there's a simpler way...

[1] As an aside, is there a standard naming convention to use here? 
Should "separator" go at the beginning of the name or the end?

bug#52237: 29.0.50; [PATCH 2] Doubled separators in context-menu-mode

[Jim Porter]

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

On 12/2/2021 9:44 AM, Juri Linkov wrote:
>>> I've only tested this on Emacs 29 so far, but it may occur on Emacs 28
>>> as well.
>>
>> Thanks; applied to Emacs 29.
> 
> Why not to Emacs 28?

I got a chance to check this under Emacs 28, and the bug exists there 
too, so it'd be nice to backport.

>>> Also, I noticed that separators can appear at the beginning
>>> and/or end of the context menu. Should they be removed too?
>>
>> Hm...  I think that might make sense, but I'm not sure.  Anybody got an
>> opinion?
> 
> Removing leading/trailing separators would be nice too.

Ok, here's a patch to do it. There might be a better way to do this, but 
it should work reliably.

Note: this patch doesn't fix the duplicated separators in the help-mode 
context menu. I haven't had time to look into the right way to fix that 
yet (there may be other mode-specific context menu functions that have 
this bug too).

[-- Attachment #2: 0001-Remove-separators-at-the-beginning-and-end-of-the-co.patch --]
[-- Type: text/plain, Size: 1745 bytes --]

From c76d753b46199da04863f19f75faf100d8e9e773 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 2 Dec 2021 20:39:33 -0800
Subject: [PATCH] Remove separators at the beginning and end of the context
 menu

* lisp/mouse.el (context-menu-map): Remove beginning/end seperators.
---
 lisp/mouse.el | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index ec43aecdd0..b5ca80a446 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -327,12 +327,21 @@ context-menu-map
                           (setq menu (funcall fun menu click))
                           nil)))
 
-    ;; Remove duplicate separators
-    (let ((l menu))
-      (while (consp l)
-        (if (and (equal (cdr-safe (car l)) menu-bar-separator)
-                 (equal (cdr-safe (cadr l)) menu-bar-separator))
+    ;; Remove duplicate separators as well as ones at the beginning or
+    ;; end of the menu.
+    (let ((l menu) saw-first-item)
+      (while (consp (cdr l))
+        ;; If the next item is a separator, remove it if 1) we haven't
+        ;; seen any other items yet, or 2) it's followed by either
+        ;; another separator or the end of the list.
+        (if (and (equal (cdr-safe (cadr l)) menu-bar-separator)
+                 (or (not saw-first-item)
+                     (null (caddr l))
+                     (equal (cdr-safe (caddr l)) menu-bar-separator)))
             (setcdr l (cddr l))
+          ;; The "first item" is any cons cell; this excludes the
+          ;; `keymap' symbol and the menu name.
+          (when (consp (cadr l)) (setq saw-first-item t))
           (setq l (cdr l)))))
 
     (when (functionp context-menu-filter-function)
-- 
2.25.1

bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

>>> I've only tested this on Emacs 29 so far, but it may occur on Emacs 28
>>> as well.
>>
>> Thanks; applied to Emacs 29.
>
> Why not to Emacs 28?

We're only fixing regressions on the release branch now, so I don't
think this qualifies?  (And even if it did, since it's a visual thing,
I'm not sure it's important enough anyway.)

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

bug#52237: 29.0.50; [PATCH 2] Doubled separators in context-menu-mode

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

>> Removing leading/trailing separators would be nice too.
>
> Ok, here's a patch to do it. There might be a better way to do this,
> but it should work reliably.

Thanks; applied to Emacs 29.

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

bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

[Jim Porter]

On 12/3/2021 8:08 AM, Lars Ingebrigtsen wrote:
> We're only fixing regressions on the release branch now, so I don't
> think this qualifies?  (And even if it did, since it's a visual thing,
> I'm not sure it's important enough anyway.)

Just so I know, for stuff like this, is there a chance it could be 
backported at some point for 28.2 (assuming that version gets published 
in the future), or would it have to wait until 29.1?

If the former, I'll try to remember to remind folks post-28.1 about this 
bug so it can be backported.

bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

[Eli Zaretskii]

> Resent-From: Jim Porter <jporterbugs@gmail.com>
> Original-Sender: "Debbugs-submit" <debbugs-submit-bounces@debbugs.gnu.org>
> Resent-CC: bug-gnu-emacs@gnu.org
> Resent-Sender: help-debbugs@gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Fri, 3 Dec 2021 22:44:43 -0800
> Cc: 52237@debbugs.gnu.org
> 
> On 12/3/2021 8:08 AM, Lars Ingebrigtsen wrote:
> > We're only fixing regressions on the release branch now, so I don't
> > think this qualifies?  (And even if it did, since it's a visual thing,
> > I'm not sure it's important enough anyway.)
> 
> Just so I know, for stuff like this, is there a chance it could be 
> backported at some point for 28.2 (assuming that version gets published 
> in the future), or would it have to wait until 29.1?

We could consider it for Emacs 28.2, yes, assuming that it wouldn't
cause any regressions or adverse side effects.

bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

[Juri Linkov]

> I found another odd case, but I'm not 100% sure the best way to fix it:
>
>   emacs -Q --eval '(context-menu-mode)'
>   C-h o identity RET
>   ;; Right-click somewhere in the Help buffer
>
> There's a doubled separator after "Next Topic". Looking at the code, this
> is because `help-mode-context-menu' inserts new items using `define-key',
> which has the effect of putting the new items *before* the (hidden) menu
> title. The resulting keymap ends up looking like this:
>
>   (keymap
>    (Previous\ Topic ...)
>    (Next\ Topic ...)
>    (help-mode-separator "--")
>    #("Context Menu" 0 12 (hide t))
>    (separator-undo "--")
>    ...)

The core function that displays the menu can handle
the menu title in the middle of the menu, so it wasn't
a problem until the recent need to remove duplicate items.

> Since there's a hidden item (the keymap title) between the
> `help-mode-separator' and `separator-undo'[1], the de-duplication doesn't
> handle that. Is there a standard/common way of defining a key such that it
> goes immediately *after* the keymap's title? I guess we could add
> `context-menu-top-separator' as an anchor (by analogy to
> `context-menu-middle-separator'), but maybe there's a simpler way...

I see no simpler way, so perhaps we need to add `context-menu-top-separator'.

> [1] As an aside, is there a standard naming convention to use here? Should
> "separator" go at the beginning of the name or the end?

`context-menu-middle-separator' is a function, so it requires the
`context-menu-' prefix.  It adds the menu item [middle-separator].
Since "middle" is an adjective, the word order can't be "separator-middle".
But "buffers-separator" doesn't look nicer than `separator-buffers'.

If you could propose a more consistent naming convention,
maybe it's not too late to rename these symbols on the release branch,
because after the release it will be impossible to rename them.

bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

[Jim Porter]

On 12/4/2021 11:50 AM, Juri Linkov wrote:
>> I found another odd case, but I'm not 100% sure the best way to fix it:
>>
>>    emacs -Q --eval '(context-menu-mode)'
>>    C-h o identity RET
>>    ;; Right-click somewhere in the Help buffer
>>
>> There's a doubled separator after "Next Topic".
[snip]
> I see no simpler way, so perhaps we need to add `context-menu-top-separator'.

Sounds good. I'll work on a patch for this and file a new bug for it 
once it's ready.

>> [1] As an aside, is there a standard naming convention to use here? Should
>> "separator" go at the beginning of the name or the end?
> 
> `context-menu-middle-separator' is a function, so it requires the
> `context-menu-' prefix.  It adds the menu item [middle-separator].
> Since "middle" is an adjective, the word order can't be "separator-middle".
> But "buffers-separator" doesn't look nicer than `separator-buffers'.

I think `FOO-separator' is the best choice here for the separators 
themselves. `FOO' acts as a noun adjunct[1], modifying the underlying 
object: a separator. For functions that just make a separator, they'd be 
named `MODULE-[FOO-]-separator', with `FOO-' being optional if it would 
be redundant with the module name. So then 
`context-menu-middle-separator' is the right function name, and it adds 
a separator named `middle-separator'.

`buffers-separator' is a bit of an odd phrasing since, as the Wikipedia 
article mentions, "Noun adjuncts were traditionally mostly singular". 
However, I think it's still a bit easier to read that correctly as "the 
separator used to mark the buffers", whereas `separator-buffers' reads 
more like "buffers used to separate things" to me.

Since this should just be a trivial renaming, hopefully we can merge 
that into Emacs 28 so we're not locked into the current names.

[1] https://en.wikipedia.org/wiki/Noun_adjunct

bug#52237: 29.0.50; [PATCH] Doubled separators in context-menu-mode

[Jim Porter]

On 12/4/2021 12:56 PM, Jim Porter wrote:
> On 12/4/2021 11:50 AM, Juri Linkov wrote:
>> `context-menu-middle-separator' is a function, so it requires the
>> `context-menu-' prefix.  It adds the menu item [middle-separator].
>> Since "middle" is an adjective, the word order can't be 
>> "separator-middle".
>> But "buffers-separator" doesn't look nicer than `separator-buffers'.
> 
> I think `FOO-separator' is the best choice here for the separators 
> themselves. `FOO' acts as a noun adjunct[1], modifying the underlying 
> object: a separator. For functions that just make a separator, they'd be 
> named `MODULE-[FOO-]-separator', with `FOO-' being optional if it would 
> be redundant with the module name. So then 
> `context-menu-middle-separator' is the right function name, and it adds 
> a separator named `middle-separator'.
> 
> `buffers-separator' is a bit of an odd phrasing since, as the Wikipedia 
> article mentions, "Noun adjuncts were traditionally mostly singular". 
> However, I think it's still a bit easier to read that correctly as "the 
> separator used to mark the buffers", whereas `separator-buffers' reads 
> more like "buffers used to separate things" to me.
> 
> Since this should just be a trivial renaming, hopefully we can merge 
> that into Emacs 28 so we're not locked into the current names.
> 
> [1] https://en.wikipedia.org/wiki/Noun_adjunct

Ok, filed bug#52286 for this: 
https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-12/msg00327.html

bug#52237: 29.0.50; [PATCH 2] Doubled separators in context-menu-mode

[Juri Linkov]

> Ok, here's a patch to do it. There might be a better way to do this, but it
> should work reliably.
>
> -      (while (consp l)
> +      (while (consp (cdr l))

This change broke flyspell-correct-word.  When a context menu is invoked
on a misspelled word in flyspell-mode, context-menu-map uses the property
context-menu-function to get a command symbol 'flyspell-correct-word'
instead of a list of menu items.

bug#50851 fixed this error by checking for a list with `(consp l)' like above:

  (consp 'flyspell-correct-word) => nil

but now 'cdr' fails on (consp (cdr 'flyspell-correct-word)).

bug#52237: 29.0.50; [PATCH 2] Doubled separators in context-menu-mode

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

>> -      (while (consp l)
>> +      (while (consp (cdr l))
>
> This change broke flyspell-correct-word.  When a context menu is invoked
> on a misspelled word in flyspell-mode, context-menu-map uses the property
> context-menu-function to get a command symbol 'flyspell-correct-word'
> instead of a list of menu items.
>
> bug#50851 fixed this error by checking for a list with `(consp l)' like above:
>
>   (consp 'flyspell-correct-word) => nil
>
> but now 'cdr' fails on (consp (cdr 'flyspell-correct-word)).

OK; I've added another (and (consp there now.

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