bug#52293: 29.0.50; [PATCH] Prevent further cases of duplicated separators in context menus

bug#52293: 29.0.50; [PATCH] Prevent further cases of duplicated separators in context menus

[Jim Porter]

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

This is a followup to bug#52237. I'll just quote my message describing 
the issue from there[1]:

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

Attached is a patch to fix this based on the discussion in bug#52237. 
One slightly odd thing is that for context menu functions that put their 
items at the top, they place their separator *below* the items. Other 
functions place the separator *above* the items. It might be too late to 
fix this though, given that Emacs 28 is only open for fixing 
regressions, and changing it in 29 would be a (small) compatibility 
break. However, I can update the patch to put the separators in these 
functions at the top if people think that's best.

[1] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-12/msg00143.html

[-- Attachment #2: 0001-Add-a-top-separator-to-the-context-menu.patch --]
[-- Type: text/plain, Size: 9036 bytes --]

From 2b395a2cb1efca85cdca992804b614cdf2641413 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 4 Dec 2021 21:42:57 -0800
Subject: [PATCH] Add a 'top-separator' to the context menu

This provides an anchor for context menu functions to use to insert
their menu items after it. Using 'define-key' doesn't work properly in
this case, since it inserts the items before the menu title, confusing
the separator de-duplication in 'context-menu-map'.

* lisp/mouse.el (context-menu-top-separator): New function.
(context-menu-functions): Use it.
* lisp/dired.el (dired-context-menu):
* lisp/help-mode.el (help-mode-context-menu):
* lisp/info.el (Info-context-menu):
* lisp/net/eww.el (eww-context-menu):
* lisp/net/goto-addr.el (goto-address-context-menu):
Use 'top-separator'.
---
 lisp/dired.el         |  5 +++--
 lisp/help-mode.el     | 10 ++++++----
 lisp/info.el          |  9 +++++----
 lisp/mouse.el         | 12 ++++++++++--
 lisp/net/eww.el       | 14 ++++++++------
 lisp/net/goto-addr.el |  8 +++++---
 6 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/lisp/dired.el b/lisp/dired.el
index d0e547ba0b..b004d81495 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2282,7 +2282,7 @@ dired-mode-operate-menu
 (defun dired-context-menu (menu click)
   "Populate MENU with Dired mode commands at CLICK."
   (when (mouse-posn-property (event-start click) 'dired-filename)
-    (define-key menu [dired-separator] menu-bar-separator)
+    (define-key-after menu [dired-separator] menu-bar-separator 'top-separator)
     (let ((easy-menu (make-sparse-keymap "Immediate")))
       (easy-menu-define nil easy-menu nil
         '("Immediate"
@@ -2292,7 +2292,8 @@ dired-context-menu
            :help "Edit file at mouse click in other window"]))
       (dolist (item (reverse (lookup-key easy-menu [menu-bar immediate])))
         (when (consp item)
-          (define-key menu (vector (car item)) (cdr item))))))
+          (define-key-after menu (vector (car item)) (cdr item)
+            'top-separator)))))
   menu)
 
 \f
diff --git a/lisp/help-mode.el b/lisp/help-mode.el
index 792f2e5af3..2ed20577f5 100644
--- a/lisp/help-mode.el
+++ b/lisp/help-mode.el
@@ -74,7 +74,8 @@ help-mode-menu
 
 (defun help-mode-context-menu (menu click)
   "Populate MENU with Help mode commands at CLICK."
-  (define-key menu [help-mode-separator] menu-bar-separator)
+  (define-key-after menu [help-mode-separator] menu-bar-separator
+    'top-separator)
   (let ((easy-menu (make-sparse-keymap "Help-Mode")))
     (easy-menu-define nil easy-menu nil
       '("Help-Mode"
@@ -86,14 +87,15 @@ help-mode-context-menu
          :active help-xref-forward-stack]))
     (dolist (item (reverse (lookup-key easy-menu [menu-bar help-mode])))
       (when (consp item)
-        (define-key menu (vector (car item)) (cdr item)))))
+        (define-key-after menu (vector (car item)) (cdr item) 'top-separator))))
 
   (when (mouse-posn-property (event-start click) 'mouse-face)
-    (define-key menu [help-mode-push-button]
+    (define-key-after menu [help-mode-push-button]
       '(menu-item "Follow Link" (lambda (event)
                                   (interactive "e")
                                   (push-button event))
-                  :help "Follow the link at click")))
+                  :help "Follow the link at click")
+      'top-separator))
 
   menu)
 
diff --git a/lisp/info.el b/lisp/info.el
index 94537c2417..76963af41c 100644
--- a/lisp/info.el
+++ b/lisp/info.el
@@ -4191,7 +4191,7 @@ Info-check-pointer
 
 (defun Info-context-menu (menu click)
   "Populate MENU with Info commands at CLICK."
-  (define-key menu [Info-separator] menu-bar-separator)
+  (define-key-after menu [Info-separator] menu-bar-separator 'top-separator)
   (let ((easy-menu (make-sparse-keymap "Info")))
     (easy-menu-define nil easy-menu nil
       '("Info"
@@ -4201,12 +4201,13 @@ Info-context-menu
          :help "Go forward in history"]))
     (dolist (item (reverse (lookup-key easy-menu [menu-bar info])))
       (when (consp item)
-        (define-key menu (vector (car item)) (cdr item)))))
+        (define-key-after menu (vector (car item)) (cdr item) 'top-separator))))
 
   (when (mouse-posn-property (event-start click) 'mouse-face)
-    (define-key menu [Info-mouse-follow-nearest-node]
+    (define-key-after menu [Info-mouse-follow-nearest-node]
       '(menu-item "Follow Link" Info-mouse-follow-nearest-node
-                  :help "Follow a link where you click")))
+                  :help "Follow a link where you click")
+      'top-separator))
 
   menu)
 
diff --git a/lisp/mouse.el b/lisp/mouse.el
index b5ca80a446..a7a7bb6eae 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -279,7 +279,8 @@ mouse-menu-bar-map
 \f
 ;; Context menus.
 
-(defcustom context-menu-functions '(context-menu-undo
+(defcustom context-menu-functions '(context-menu-top-separator
+                                    context-menu-undo
                                     context-menu-region
                                     context-menu-middle-separator
                                     context-menu-local
@@ -288,7 +289,8 @@ context-menu-functions
 Each function receives the menu and the mouse click event as its arguments
 and should return the same menu with changes such as added new menu items."
   :type '(repeat
-          (choice (function-item context-menu-undo)
+          (choice (function-item context-menu-top-separator)
+                  (function-item context-menu-undo)
                   (function-item context-menu-region)
                   (function-item context-menu-middle-separator)
                   (function-item context-menu-toolbar)
@@ -348,6 +350,12 @@ context-menu-map
       (setq menu (funcall context-menu-filter-function menu click)))
     menu))
 
+(defun context-menu-top-separator (menu _click)
+  "Add separator to the top of the context menu.
+Some context functions add menu items below the separator."
+  (define-key-after menu [top-separator] menu-bar-separator)
+  menu)
+
 (defun context-menu-middle-separator (menu _click)
   "Add separator to the middle of the context menu.
 Some context functions add menu items below the separator."
diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index e86d21f889..7303e01a37 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -1106,7 +1106,7 @@ eww-mode-map
 
 (defun eww-context-menu (menu click)
   "Populate MENU with eww commands at CLICK."
-  (define-key menu [eww-separator] menu-bar-separator)
+  (define-key-after menu [eww-separator] menu-bar-separator 'top-separator)
   (let ((easy-menu (make-sparse-keymap "Eww")))
     (easy-menu-define nil easy-menu nil
       '("Eww"
@@ -1117,20 +1117,22 @@ eww-context-menu
 	["Reload" eww-reload t]))
     (dolist (item (reverse (lookup-key easy-menu [menu-bar eww])))
       (when (consp item)
-        (define-key menu (vector (car item)) (cdr item)))))
+        (define-key-after menu (vector (car item)) (cdr item) 'top-separator))))
 
   (when (or (mouse-posn-property (event-start click) 'shr-url)
             (mouse-posn-property (event-start click) 'image-url))
-    (define-key menu [shr-mouse-browse-url-new-window]
+    (define-key-after menu [shr-mouse-browse-url-new-window]
       `(menu-item "Follow URL in new window" ,(if browse-url-new-window-flag
                                                   'shr-mouse-browse-url
                                                 'shr-mouse-browse-url-new-window)
-                  :help "Browse the URL under the mouse cursor in a new window"))
-    (define-key menu [shr-mouse-browse-url]
+                  :help "Browse the URL under the mouse cursor in a new window")
+      'top-separator)
+    (define-key-after menu [shr-mouse-browse-url]
       `(menu-item "Follow URL" ,(if browse-url-new-window-flag
                                     'shr-mouse-browse-url-new-window
                                   'shr-mouse-browse-url)
-                  :help "Browse the URL under the mouse cursor")))
+                  :help "Browse the URL under the mouse cursor")
+      'top-separator))
 
   menu)
 
diff --git a/lisp/net/goto-addr.el b/lisp/net/goto-addr.el
index 848bad3b0d..bb0f0b00ee 100644
--- a/lisp/net/goto-addr.el
+++ b/lisp/net/goto-addr.el
@@ -127,10 +127,12 @@ goto-address-highlight-keymap
 (defun goto-address-context-menu (menu click)
   "Populate MENU with `goto-address' commands at CLICK."
   (when (mouse-posn-property (event-start click) 'goto-address)
-    (define-key menu [goto-address-separator] menu-bar-separator)
-    (define-key menu [goto-address-at-mouse]
+    (define-key-after menu [goto-address-separator] menu-bar-separator
+      'top-separator)
+    (define-key-after menu [goto-address-at-mouse]
       '(menu-item "Follow Link" goto-address-at-mouse
-                  :help "Follow a link where you click")))
+                  :help "Follow a link where you click")
+      'top-separator))
   menu)
 
 (defcustom goto-address-url-face 'link
-- 
2.25.1

bug#52293: 29.0.50; [PATCH] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

> Attached is a patch to fix this based on the discussion in bug#52237.

Thanks for the patch.

> One slightly odd thing is that for context menu functions that put their items
> at the top, they place their separator *below* the items. Other functions
> place the separator *above* the items.

There is some logic in this: when the menu doesn't contain a separator
at the top, then it is necessary to add a separator below the added items,
e.g. by modifying such a menu:

  - existing menu item 1
  - existing menu item 2

to

  - new item 1
  - new item 2
  - new separator
  - existing menu item 1
  - existing menu item 2

bug#52293: 29.0.50; [PATCH] Prevent further cases of duplicated separators in context menus

[Jim Porter]

On 12/5/2021 1:39 AM, Juri Linkov wrote:
>> Attached is a patch to fix this based on the discussion in bug#52237.
> 
> Thanks for the patch.
> 
>> One slightly odd thing is that for context menu functions that put their items
>> at the top, they place their separator *below* the items. Other functions
>> place the separator *above* the items.
> 
> There is some logic in this: when the menu doesn't contain a separator
> at the top, then it is necessary to add a separator below the added items,
> e.g. by modifying such a menu:
> 
>    - existing menu item 1
>    - existing menu item 2
> 
> to
> 
>    - new item 1
>    - new item 2
>    - new separator
>    - existing menu item 1
>    - existing menu item 2

Right, makes sense.

(I was originally thinking that we could put items above 
`top-separator', and that then there'd be a separator below the items. 
That's not right though, since we'd use `define-key-after', putting the 
items *below* `top-separator'. There's no `define-key-before', so I just 
had the logic backwards in my head.)

bug#52293: 29.0.50; [PATCH] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

>  (defun help-mode-context-menu (menu click)
>    "Populate MENU with Help mode commands at CLICK."
> -  (define-key menu [help-mode-separator] menu-bar-separator)
> +  (define-key-after menu [help-mode-separator] menu-bar-separator
> +    'top-separator)

Now I realized that it's possible to do the same without 'top-separator':

  (define-key-after menu [help-mode-separator] menu-bar-separator
    "Context Menu")

Or when the title string is defined as a variable:

  (defvar context-menu-title "Context Menu")

  (define-key-after menu [help-mode-separator] menu-bar-separator
    context-menu-title)

But maybe 'top-separator' still could be used for clarity?
Or it increases complexity?

bug#52293: 29.0.50; [PATCH] Prevent further cases of duplicated separators in context menus

[Jim Porter]

On 12/5/2021 9:59 AM, Juri Linkov wrote:
>>   (defun help-mode-context-menu (menu click)
>>     "Populate MENU with Help mode commands at CLICK."
>> -  (define-key menu [help-mode-separator] menu-bar-separator)
>> +  (define-key-after menu [help-mode-separator] menu-bar-separator
>> +    'top-separator)
> 
> Now I realized that it's possible to do the same without 'top-separator':
> 
>    (define-key-after menu [help-mode-separator] menu-bar-separator
>      "Context Menu")
> 
> Or when the title string is defined as a variable:
> 
>    (defvar context-menu-title "Context Menu")
> 
>    (define-key-after menu [help-mode-separator] menu-bar-separator
>      context-menu-title)
> 
> But maybe 'top-separator' still could be used for clarity?
> Or it increases complexity?

Hmm, that might work. One downside is that I think it makes it harder 
for context menu functions to change the menu's title/prompt to 
something else. Of course, if we used `context-menu-title' as the anchor 
like your example above, it should still be possible to update the menu 
title via `context-menu-filter-function'. That would be trickier to use 
though, at least in the situations I have in mind.

For example, I added a very limited context menu that uses the menu 
title in my config under Emacs 27 for org-mode links: I can right-click 
and it shows a context menu with the URL as the title and menu items for 
different ways to open the URL (in Firefox, Firefox Private Browsing, or 
EWW). It's nice to be able to see the URL since that can influence which 
item I choose.

Updating this part of my config for Emacs 28 was actually what prompted 
me to start looking into `context-menu-mode' in more detail. It would be 
easier to implement this if context menu functions didn't rely on the 
context menu title having a particular value.

bug#52293: 29.0.50; [PATCH] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

> On 12/5/2021 9:59 AM, Juri Linkov wrote:
>>>   (defun help-mode-context-menu (menu click)
>>>     "Populate MENU with Help mode commands at CLICK."
>>> -  (define-key menu [help-mode-separator] menu-bar-separator)
>>> +  (define-key-after menu [help-mode-separator] menu-bar-separator
>>> +    'top-separator)
>> Now I realized that it's possible to do the same without 'top-separator':
>>    (define-key-after menu [help-mode-separator] menu-bar-separator
>>      "Context Menu")
>> Or when the title string is defined as a variable:
>>    (defvar context-menu-title "Context Menu")
>>    (define-key-after menu [help-mode-separator] menu-bar-separator
>>      context-menu-title)
>> But maybe 'top-separator' still could be used for clarity?
>> Or it increases complexity?
>
> Hmm, that might work. One downside is that I think it makes it harder for
> context menu functions to change the menu's title/prompt to something
> else. Of course, if we used `context-menu-title' as the anchor like your
> example above, it should still be possible to update the menu title via
> `context-menu-filter-function'. That would be trickier to use though, at
> least in the situations I have in mind.

I agree that relying on the constant value of the menu title
would be too unreliable.

> For example, I added a very limited context menu that uses the menu title
> in my config under Emacs 27 for org-mode links: I can right-click and it
> shows a context menu with the URL as the title and menu items for different
> ways to open the URL (in Firefox, Firefox Private Browsing, or EWW). It's
> nice to be able to see the URL since that can influence which item
> I choose.

Good example.  Another example is flyspell that shows a misspelled word
in the menu title.

> Updating this part of my config for Emacs 28 was actually what prompted me
> to start looking into `context-menu-mode' in more detail. It would be
> easier to implement this if context menu functions didn't rely on the
> context menu title having a particular value.

I think the remaining problem we have to solve is to try to
prevent such a situation when the user accidentally removes
`context-menu-top-separator' from `context-menu-functions'.
To not break the menu in this case, maybe we need to ensure
that the menu always contains `top-separator' as well as
`middle-separator' even when `context-menu-functions'
doesn't contain a function that adds them?  I.e. just to check
if these separators are missing, and add them at the top.

bug#52293: 29.0.50; [PATCH v2] Prevent further cases of duplicated separators in context menus

[Jim Porter]

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

On 12/6/2021 1:23 AM, Juri Linkov wrote:
>> Updating this part of my config for Emacs 28 was actually what prompted me
>> to start looking into `context-menu-mode' in more detail. It would be
>> easier to implement this if context menu functions didn't rely on the
>> context menu title having a particular value.
> 
> I think the remaining problem we have to solve is to try to
> prevent such a situation when the user accidentally removes
> `context-menu-top-separator' from `context-menu-functions'.
> To not break the menu in this case, maybe we need to ensure
> that the menu always contains `top-separator' as well as
> `middle-separator' even when `context-menu-functions'
> doesn't contain a function that adds them?  I.e. just to check
> if these separators are missing, and add them at the top.

For the top separator, maybe instead of providing a function for users 
to put wherever they like in `context-menu-functions', we could just add 
it directly in `context-menu-map' immediately before calling the 
functions in `context-menu-functions'? Since it's supposed to be at the 
top, it doesn't make a lot of sense to be able to customize where it goes.

It does makes sense to customize where `middle-separator' goes though; 
what the user considers the "middle" depends on what other menu items 
are normally present. However, if the user didn't include 
`context-menu-middle-separator', then

   (define-key-after menu [foo] '(menu-item ...) 'middle-separator)

just adds `foo' to the end of the menu. That seems ok to me. Then, 
adding after `top-separator' puts your item at the beginning of the 
menu, and adding after `middle-separator' puts your item at the middle 
or the end, depending on the user's configuration. How does that sound 
to you?

[-- Attachment #2: 0001-Add-a-top-separator-to-the-context-menu.patch --]
[-- Type: text/plain, Size: 7955 bytes --]

From 831ba55dd209fe5551733bbea1d33fc0e720227e Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 6 Dec 2021 19:40:40 -0800
Subject: [PATCH] Add a 'top-separator' to the context menu

This provides an anchor for context menu functions to use to insert
their menu items after it. Using 'define-key' doesn't work properly in
this case, since it inserts the items before the menu title, confusing
the separator de-duplication in 'context-menu-map'.

* lisp/mouse.el (context-menu-map): Add 'top-separator' to the menu.
* lisp/dired.el (dired-context-menu):
* lisp/help-mode.el (help-mode-context-menu):
* lisp/info.el (Info-context-menu):
* lisp/net/eww.el (eww-context-menu):
* lisp/net/goto-addr.el (goto-address-context-menu):
Use 'top-separator'.
---
 lisp/dired.el         |  5 +++--
 lisp/help-mode.el     | 10 ++++++----
 lisp/info.el          |  9 +++++----
 lisp/mouse.el         |  3 +++
 lisp/net/eww.el       | 14 ++++++++------
 lisp/net/goto-addr.el |  8 +++++---
 6 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/lisp/dired.el b/lisp/dired.el
index d0e547ba0b..b004d81495 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2282,7 +2282,7 @@ dired-mode-operate-menu
 (defun dired-context-menu (menu click)
   "Populate MENU with Dired mode commands at CLICK."
   (when (mouse-posn-property (event-start click) 'dired-filename)
-    (define-key menu [dired-separator] menu-bar-separator)
+    (define-key-after menu [dired-separator] menu-bar-separator 'top-separator)
     (let ((easy-menu (make-sparse-keymap "Immediate")))
       (easy-menu-define nil easy-menu nil
         '("Immediate"
@@ -2292,7 +2292,8 @@ dired-context-menu
            :help "Edit file at mouse click in other window"]))
       (dolist (item (reverse (lookup-key easy-menu [menu-bar immediate])))
         (when (consp item)
-          (define-key menu (vector (car item)) (cdr item))))))
+          (define-key-after menu (vector (car item)) (cdr item)
+            'top-separator)))))
   menu)
 
 \f
diff --git a/lisp/help-mode.el b/lisp/help-mode.el
index 792f2e5af3..2ed20577f5 100644
--- a/lisp/help-mode.el
+++ b/lisp/help-mode.el
@@ -74,7 +74,8 @@ help-mode-menu
 
 (defun help-mode-context-menu (menu click)
   "Populate MENU with Help mode commands at CLICK."
-  (define-key menu [help-mode-separator] menu-bar-separator)
+  (define-key-after menu [help-mode-separator] menu-bar-separator
+    'top-separator)
   (let ((easy-menu (make-sparse-keymap "Help-Mode")))
     (easy-menu-define nil easy-menu nil
       '("Help-Mode"
@@ -86,14 +87,15 @@ help-mode-context-menu
          :active help-xref-forward-stack]))
     (dolist (item (reverse (lookup-key easy-menu [menu-bar help-mode])))
       (when (consp item)
-        (define-key menu (vector (car item)) (cdr item)))))
+        (define-key-after menu (vector (car item)) (cdr item) 'top-separator))))
 
   (when (mouse-posn-property (event-start click) 'mouse-face)
-    (define-key menu [help-mode-push-button]
+    (define-key-after menu [help-mode-push-button]
       '(menu-item "Follow Link" (lambda (event)
                                   (interactive "e")
                                   (push-button event))
-                  :help "Follow the link at click")))
+                  :help "Follow the link at click")
+      'top-separator))
 
   menu)
 
diff --git a/lisp/info.el b/lisp/info.el
index 559460e8d2..05e9277698 100644
--- a/lisp/info.el
+++ b/lisp/info.el
@@ -4193,7 +4193,7 @@ Info-check-pointer
 
 (defun Info-context-menu (menu click)
   "Populate MENU with Info commands at CLICK."
-  (define-key menu [Info-separator] menu-bar-separator)
+  (define-key-after menu [Info-separator] menu-bar-separator 'top-separator)
   (let ((easy-menu (make-sparse-keymap "Info")))
     (easy-menu-define nil easy-menu nil
       '("Info"
@@ -4203,12 +4203,13 @@ Info-context-menu
          :help "Go forward in history"]))
     (dolist (item (reverse (lookup-key easy-menu [menu-bar info])))
       (when (consp item)
-        (define-key menu (vector (car item)) (cdr item)))))
+        (define-key-after menu (vector (car item)) (cdr item) 'top-separator))))
 
   (when (mouse-posn-property (event-start click) 'mouse-face)
-    (define-key menu [Info-mouse-follow-nearest-node]
+    (define-key-after menu [Info-mouse-follow-nearest-node]
       '(menu-item "Follow Link" Info-mouse-follow-nearest-node
-                  :help "Follow a link where you click")))
+                  :help "Follow a link where you click")
+      'top-separator))
 
   menu)
 
diff --git a/lisp/mouse.el b/lisp/mouse.el
index af1eca12f4..36003a1db8 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -319,6 +319,9 @@ context-menu-map
          (click (or click last-input-event))
          (fun (mouse-posn-property (event-start click)
                                    'context-menu-function)))
+    ;; Add a separator to the top of the menu to provide an anchor for
+    ;; context menu functions to use.
+    (define-key-after menu [top-separator] menu-bar-separator)
 
     (if (functionp fun)
         (setq menu (funcall fun menu click))
diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index e86d21f889..7303e01a37 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -1106,7 +1106,7 @@ eww-mode-map
 
 (defun eww-context-menu (menu click)
   "Populate MENU with eww commands at CLICK."
-  (define-key menu [eww-separator] menu-bar-separator)
+  (define-key-after menu [eww-separator] menu-bar-separator 'top-separator)
   (let ((easy-menu (make-sparse-keymap "Eww")))
     (easy-menu-define nil easy-menu nil
       '("Eww"
@@ -1117,20 +1117,22 @@ eww-context-menu
 	["Reload" eww-reload t]))
     (dolist (item (reverse (lookup-key easy-menu [menu-bar eww])))
       (when (consp item)
-        (define-key menu (vector (car item)) (cdr item)))))
+        (define-key-after menu (vector (car item)) (cdr item) 'top-separator))))
 
   (when (or (mouse-posn-property (event-start click) 'shr-url)
             (mouse-posn-property (event-start click) 'image-url))
-    (define-key menu [shr-mouse-browse-url-new-window]
+    (define-key-after menu [shr-mouse-browse-url-new-window]
       `(menu-item "Follow URL in new window" ,(if browse-url-new-window-flag
                                                   'shr-mouse-browse-url
                                                 'shr-mouse-browse-url-new-window)
-                  :help "Browse the URL under the mouse cursor in a new window"))
-    (define-key menu [shr-mouse-browse-url]
+                  :help "Browse the URL under the mouse cursor in a new window")
+      'top-separator)
+    (define-key-after menu [shr-mouse-browse-url]
       `(menu-item "Follow URL" ,(if browse-url-new-window-flag
                                     'shr-mouse-browse-url-new-window
                                   'shr-mouse-browse-url)
-                  :help "Browse the URL under the mouse cursor")))
+                  :help "Browse the URL under the mouse cursor")
+      'top-separator))
 
   menu)
 
diff --git a/lisp/net/goto-addr.el b/lisp/net/goto-addr.el
index 848bad3b0d..bb0f0b00ee 100644
--- a/lisp/net/goto-addr.el
+++ b/lisp/net/goto-addr.el
@@ -127,10 +127,12 @@ goto-address-highlight-keymap
 (defun goto-address-context-menu (menu click)
   "Populate MENU with `goto-address' commands at CLICK."
   (when (mouse-posn-property (event-start click) 'goto-address)
-    (define-key menu [goto-address-separator] menu-bar-separator)
-    (define-key menu [goto-address-at-mouse]
+    (define-key-after menu [goto-address-separator] menu-bar-separator
+      'top-separator)
+    (define-key-after menu [goto-address-at-mouse]
       '(menu-item "Follow Link" goto-address-at-mouse
-                  :help "Follow a link where you click")))
+                  :help "Follow a link where you click")
+      'top-separator))
   menu)
 
 (defcustom goto-address-url-face 'link
-- 
2.25.1

bug#52293: 29.0.50; [PATCH v2] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

> For the top separator, maybe instead of providing a function for users to
> put wherever they like in `context-menu-functions', we could just add it
> directly in `context-menu-map' immediately before calling the functions in
> `context-menu-functions'? Since it's supposed to be at the top, it doesn't
> make a lot of sense to be able to customize where it goes.
>
> It does makes sense to customize where `middle-separator' goes though; what
> the user considers the "middle" depends on what other menu items are
> normally present. However, if the user didn't include
> `context-menu-middle-separator', then
>
>   (define-key-after menu [foo] '(menu-item ...) 'middle-separator)
>
> just adds `foo' to the end of the menu. That seems ok to me. Then, adding
> after `top-separator' puts your item at the beginning of the menu, and
> adding after `middle-separator' puts your item at the middle or the end,
> depending on the user's configuration. How does that sound to you?

Thanks, this makes perfect sense.  I vote for pushing this to emacs-28,
so the authors of packages could rely on this scheme.

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Jim Porter]

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

On 12/7/2021 12:19 AM, Juri Linkov wrote:
> Thanks, this makes perfect sense.  I vote for pushing this to emacs-28,
> so the authors of packages could rely on this scheme.

Sounds good. Attached is an updated patch with an expanded docstring for 
`context-menu-functions' that mentions the existence of `top-separator' 
and `middle-separator' and how to use them. Everything else is the same 
as v2 though.

[-- Attachment #2: 0001-Add-a-top-separator-to-the-context-menu.patch --]
[-- Type: text/plain, Size: 8891 bytes --]

From 89fef1244ebd55041eb415ef7b9d59e3b1014adc Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 7 Dec 2021 20:34:08 -0800
Subject: [PATCH] Add a 'top-separator' to the context menu

This provides an anchor for context menu functions to use to insert
their menu items after it. Using 'define-key' doesn't work properly in
this case, since it inserts the items before the menu title, confusing
the separator de-duplication in 'context-menu-map'.

* lisp/mouse.el (context-menu-functions): Mention 'top-separator',
'middle-separator', and how to use them.
(context-menu-map): Add 'top-separator' to the menu.
* lisp/dired.el (dired-context-menu):
* lisp/help-mode.el (help-mode-context-menu):
* lisp/info.el (Info-context-menu):
* lisp/net/eww.el (eww-context-menu):
* lisp/net/goto-addr.el (goto-address-context-menu):
Use 'top-separator'.
---
 lisp/dired.el         |  5 +++--
 lisp/help-mode.el     | 10 ++++++----
 lisp/info.el          |  9 +++++----
 lisp/mouse.el         | 11 ++++++++++-
 lisp/net/eww.el       | 14 ++++++++------
 lisp/net/goto-addr.el |  8 +++++---
 6 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/lisp/dired.el b/lisp/dired.el
index d0e547ba0b..b004d81495 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2282,7 +2282,7 @@ dired-mode-operate-menu
 (defun dired-context-menu (menu click)
   "Populate MENU with Dired mode commands at CLICK."
   (when (mouse-posn-property (event-start click) 'dired-filename)
-    (define-key menu [dired-separator] menu-bar-separator)
+    (define-key-after menu [dired-separator] menu-bar-separator 'top-separator)
     (let ((easy-menu (make-sparse-keymap "Immediate")))
       (easy-menu-define nil easy-menu nil
         '("Immediate"
@@ -2292,7 +2292,8 @@ dired-context-menu
            :help "Edit file at mouse click in other window"]))
       (dolist (item (reverse (lookup-key easy-menu [menu-bar immediate])))
         (when (consp item)
-          (define-key menu (vector (car item)) (cdr item))))))
+          (define-key-after menu (vector (car item)) (cdr item)
+            'top-separator)))))
   menu)
 
 \f
diff --git a/lisp/help-mode.el b/lisp/help-mode.el
index 792f2e5af3..2ed20577f5 100644
--- a/lisp/help-mode.el
+++ b/lisp/help-mode.el
@@ -74,7 +74,8 @@ help-mode-menu
 
 (defun help-mode-context-menu (menu click)
   "Populate MENU with Help mode commands at CLICK."
-  (define-key menu [help-mode-separator] menu-bar-separator)
+  (define-key-after menu [help-mode-separator] menu-bar-separator
+    'top-separator)
   (let ((easy-menu (make-sparse-keymap "Help-Mode")))
     (easy-menu-define nil easy-menu nil
       '("Help-Mode"
@@ -86,14 +87,15 @@ help-mode-context-menu
          :active help-xref-forward-stack]))
     (dolist (item (reverse (lookup-key easy-menu [menu-bar help-mode])))
       (when (consp item)
-        (define-key menu (vector (car item)) (cdr item)))))
+        (define-key-after menu (vector (car item)) (cdr item) 'top-separator))))
 
   (when (mouse-posn-property (event-start click) 'mouse-face)
-    (define-key menu [help-mode-push-button]
+    (define-key-after menu [help-mode-push-button]
       '(menu-item "Follow Link" (lambda (event)
                                   (interactive "e")
                                   (push-button event))
-                  :help "Follow the link at click")))
+                  :help "Follow the link at click")
+      'top-separator))
 
   menu)
 
diff --git a/lisp/info.el b/lisp/info.el
index 559460e8d2..05e9277698 100644
--- a/lisp/info.el
+++ b/lisp/info.el
@@ -4193,7 +4193,7 @@ Info-check-pointer
 
 (defun Info-context-menu (menu click)
   "Populate MENU with Info commands at CLICK."
-  (define-key menu [Info-separator] menu-bar-separator)
+  (define-key-after menu [Info-separator] menu-bar-separator 'top-separator)
   (let ((easy-menu (make-sparse-keymap "Info")))
     (easy-menu-define nil easy-menu nil
       '("Info"
@@ -4203,12 +4203,13 @@ Info-context-menu
          :help "Go forward in history"]))
     (dolist (item (reverse (lookup-key easy-menu [menu-bar info])))
       (when (consp item)
-        (define-key menu (vector (car item)) (cdr item)))))
+        (define-key-after menu (vector (car item)) (cdr item) 'top-separator))))
 
   (when (mouse-posn-property (event-start click) 'mouse-face)
-    (define-key menu [Info-mouse-follow-nearest-node]
+    (define-key-after menu [Info-mouse-follow-nearest-node]
       '(menu-item "Follow Link" Info-mouse-follow-nearest-node
-                  :help "Follow a link where you click")))
+                  :help "Follow a link where you click")
+      'top-separator))
 
   menu)
 
diff --git a/lisp/mouse.el b/lisp/mouse.el
index af1eca12f4..744df5f918 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -286,7 +286,13 @@ context-menu-functions
                                     context-menu-minor)
   "List of functions that produce the contents of the context menu.
 Each function receives the menu and the mouse click event as its arguments
-and should return the same menu with changes such as added new menu items."
+and should return the same menu with changes such as added new menu items.
+
+Functions can insert new menu items in whatever order makes sense to them.
+To help simplify the placement of new items, the menu provides the
+separators `top-separator' and `middle-separator', which can be passed as
+the last argument to `define-key-after' in order to position the new item
+accordingly."
   :type '(repeat
           (choice (function-item context-menu-undo)
                   (function-item context-menu-region)
@@ -319,6 +325,9 @@ context-menu-map
          (click (or click last-input-event))
          (fun (mouse-posn-property (event-start click)
                                    'context-menu-function)))
+    ;; Add a separator to the top of the menu to provide an anchor for
+    ;; context menu functions to use.
+    (define-key-after menu [top-separator] menu-bar-separator)
 
     (if (functionp fun)
         (setq menu (funcall fun menu click))
diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index e86d21f889..7303e01a37 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -1106,7 +1106,7 @@ eww-mode-map
 
 (defun eww-context-menu (menu click)
   "Populate MENU with eww commands at CLICK."
-  (define-key menu [eww-separator] menu-bar-separator)
+  (define-key-after menu [eww-separator] menu-bar-separator 'top-separator)
   (let ((easy-menu (make-sparse-keymap "Eww")))
     (easy-menu-define nil easy-menu nil
       '("Eww"
@@ -1117,20 +1117,22 @@ eww-context-menu
 	["Reload" eww-reload t]))
     (dolist (item (reverse (lookup-key easy-menu [menu-bar eww])))
       (when (consp item)
-        (define-key menu (vector (car item)) (cdr item)))))
+        (define-key-after menu (vector (car item)) (cdr item) 'top-separator))))
 
   (when (or (mouse-posn-property (event-start click) 'shr-url)
             (mouse-posn-property (event-start click) 'image-url))
-    (define-key menu [shr-mouse-browse-url-new-window]
+    (define-key-after menu [shr-mouse-browse-url-new-window]
       `(menu-item "Follow URL in new window" ,(if browse-url-new-window-flag
                                                   'shr-mouse-browse-url
                                                 'shr-mouse-browse-url-new-window)
-                  :help "Browse the URL under the mouse cursor in a new window"))
-    (define-key menu [shr-mouse-browse-url]
+                  :help "Browse the URL under the mouse cursor in a new window")
+      'top-separator)
+    (define-key-after menu [shr-mouse-browse-url]
       `(menu-item "Follow URL" ,(if browse-url-new-window-flag
                                     'shr-mouse-browse-url-new-window
                                   'shr-mouse-browse-url)
-                  :help "Browse the URL under the mouse cursor")))
+                  :help "Browse the URL under the mouse cursor")
+      'top-separator))
 
   menu)
 
diff --git a/lisp/net/goto-addr.el b/lisp/net/goto-addr.el
index 848bad3b0d..bb0f0b00ee 100644
--- a/lisp/net/goto-addr.el
+++ b/lisp/net/goto-addr.el
@@ -127,10 +127,12 @@ goto-address-highlight-keymap
 (defun goto-address-context-menu (menu click)
   "Populate MENU with `goto-address' commands at CLICK."
   (when (mouse-posn-property (event-start click) 'goto-address)
-    (define-key menu [goto-address-separator] menu-bar-separator)
-    (define-key menu [goto-address-at-mouse]
+    (define-key-after menu [goto-address-separator] menu-bar-separator
+      'top-separator)
+    (define-key-after menu [goto-address-at-mouse]
       '(menu-item "Follow Link" goto-address-at-mouse
-                  :help "Follow a link where you click")))
+                  :help "Follow a link where you click")
+      'top-separator))
   menu)
 
 (defcustom goto-address-url-face 'link
-- 
2.25.1

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Eli Zaretskii]

> From: Jim Porter <jporterbugs@gmail.com>
> Date: Tue, 7 Dec 2021 20:37:28 -0800
> Cc: 52293@debbugs.gnu.org
> 
> On 12/7/2021 12:19 AM, Juri Linkov wrote:
> > Thanks, this makes perfect sense.  I vote for pushing this to emacs-28,
> > so the authors of packages could rely on this scheme.

Perhaps we should decide that Emacs 28.1 will be release some time
after 2031, then.  Because we are evidently unable to avoid
temptations to add "one more feature".

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Jim Porter]

On 12/8/2021 4:59 AM, Eli Zaretskii wrote:
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Tue, 7 Dec 2021 20:37:28 -0800
>> Cc: 52293@debbugs.gnu.org
>>
>> On 12/7/2021 12:19 AM, Juri Linkov wrote:
>>> Thanks, this makes perfect sense.  I vote for pushing this to emacs-28,
>>> so the authors of packages could rely on this scheme.
> 
> Perhaps we should decide that Emacs 28.1 will be release some time
> after 2031, then.  Because we are evidently unable to avoid
> temptations to add "one more feature".

I agree that since this isn't fixing a regression from Emacs 27, and 
since the real problem is just some visual ugliness, it's probably too 
late to put into 28.1. If possible, it'd be nice to get this for 28.2 
though, since it's a fix for a new feature added in 28.1.

We can worry about that post-28.1 though. I'll try to keep this on my 
radar until 28.1 is out the door, and then send a reminder message to 
try and get this merged for 28.2.

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

>> > Thanks, this makes perfect sense.  I vote for pushing this to emacs-28,
>> > so the authors of packages could rely on this scheme.
>
> Perhaps we should decide that Emacs 28.1 will be release some time
> after 2031, then.  Because we are evidently unable to avoid
> temptations to add "one more feature".

With the naming scheme of using the year as the version number as in
Windows 95/98, Emacs 28 should be released in 2028 :-)

But before the release we need to decide whether the authors of packages
should add new menu items at the top of the menu before its title,
or help them by providing an anchor in the menu.

Also please decide what to do bug#52286: should we unify such
menu symbols as [separator-global] to [global-separator]
before the release?

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Jim Porter <jporterbugs@gmail.com>,  52293@debbugs.gnu.org
> Date: Wed, 08 Dec 2021 21:07:28 +0200
> 
> But before the release we need to decide whether the authors of packages
> should add new menu items at the top of the menu before its title,
> or help them by providing an anchor in the menu.
> 
> Also please decide what to do bug#52286: should we unify such
> menu symbols as [separator-global] to [global-separator]
> before the release?

I'd prefer all of those to go to master.  I'm quite sure this is not
the last time we hear that something in those quarters needs to be
fixed (I think the feature wasn't mature enough to go to Emacs 28 in
the first place, but that's water under the bridge now).  No
catastrophe will happen if this will be fixed after 28.1, or even in
29.1.

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

>> But before the release we need to decide whether the authors of packages
>> should add new menu items at the top of the menu before its title,
>> or help them by providing an anchor in the menu.
>>
>> Also please decide what to do bug#52286: should we unify such
>> menu symbols as [separator-global] to [global-separator]
>> before the release?
>
> I'd prefer all of those to go to master.  I'm quite sure this is not
> the last time we hear that something in those quarters needs to be
> fixed (I think the feature wasn't mature enough to go to Emacs 28 in
> the first place, but that's water under the bridge now).  No
> catastrophe will happen if this will be fixed after 28.1, or even in
> 29.1.

Do you think that [separator-global] should be renamed to [global-separator]
only in master?

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: jporterbugs@gmail.com,  52293@debbugs.gnu.org
> Date: Thu, 09 Dec 2021 11:06:36 +0200
> 
> > I'd prefer all of those to go to master.  I'm quite sure this is not
> > the last time we hear that something in those quarters needs to be
> > fixed (I think the feature wasn't mature enough to go to Emacs 28 in
> > the first place, but that's water under the bridge now).  No
> > catastrophe will happen if this will be fixed after 28.1, or even in
> > 29.1.
> 
> Do you think that [separator-global] should be renamed to [global-separator]
> only in master?

Yes.  This way, we have quite some time before us to let people bump
into any problems this could cause and report back to us.

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Jim Porter]

On 12/9/2021 1:39 AM, Eli Zaretskii wrote:
>> From: Juri Linkov <juri@linkov.net>
>> Cc: jporterbugs@gmail.com,  52293@debbugs.gnu.org
>> Date: Thu, 09 Dec 2021 11:06:36 +0200
>>
>>> I'd prefer all of those to go to master.  I'm quite sure this is not
>>> the last time we hear that something in those quarters needs to be
>>> fixed (I think the feature wasn't mature enough to go to Emacs 28 in
>>> the first place, but that's water under the bridge now).  No
>>> catastrophe will happen if this will be fixed after 28.1, or even in
>>> 29.1.
>>
>> Do you think that [separator-global] should be renamed to [global-separator]
>> only in master?
> 
> Yes.  This way, we have quite some time before us to let people bump
> into any problems this could cause and report back to us.

That's ok by me. Note that this[1] is a very mildly incompatible change, 
so as long as people are aware of that, I think it should be ok to merge 
into 29, and hopefully into 28.2. The only incompatibility would be 
people wanting to add context menu items after certain specific 
separators like `global-separator' or `undo-separator'. I think that's 
fairly unlikely though, since:

a) `context-menu-mode' is new so there aren't many (any?) third-party 
packages that use it yet.

b) `context-menu-mode' doesn't guarantee that any particular items are 
actually present in the menu. Users can customize the context menu, so 
it could have anything at all in it. Relying on the presence of 
`global-separator' (however it's named) would be somewhat risky. 
`middle-separator' and `top-separator' (the latter is from the patch in 
this bug) are/will be at least *likely* to be present though, so are 
more likely to be used in third-party code.

If there are any other issues with the latest patch in this bug (or 
bug#52286), just let me know.

[1] bug#52286, that is.

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Eli Zaretskii]

> Cc: 52293@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Sat, 11 Dec 2021 20:02:41 -0800
> 
> > Yes.  This way, we have quite some time before us to let people bump
> > into any problems this could cause and report back to us.
> 
> That's ok by me. Note that this[1] is a very mildly incompatible change, 
> so as long as people are aware of that, I think it should be ok to merge 
> into 29, and hopefully into 28.2. The only incompatibility would be 
> people wanting to add context menu items after certain specific 
> separators like `global-separator' or `undo-separator'. I think that's 
> fairly unlikely though, since:
> 
> a) `context-menu-mode' is new so there aren't many (any?) third-party 
> packages that use it yet.
> 
> b) `context-menu-mode' doesn't guarantee that any particular items are 
> actually present in the menu. Users can customize the context menu, so 
> it could have anything at all in it. Relying on the presence of 
> `global-separator' (however it's named) would be somewhat risky. 
> `middle-separator' and `top-separator' (the latter is from the patch in 
> this bug) are/will be at least *likely* to be present though, so are 
> more likely to be used in third-party code.

Are you saying that we will be recommending a convention for separator
names only for menus popped up in context-menu-mode?  Does it make
sense to have such a specialized convention?  Isn't it possible to
show context menus outside of the context-menu-mode?

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Jim Porter]

On 12/11/2021 11:02 PM, Eli Zaretskii wrote:
> Are you saying that we will be recommending a convention for separator
> names only for menus popped up in context-menu-mode?  Does it make
> sense to have such a specialized convention?  Isn't it possible to
> show context menus outside of the context-menu-mode?

No, I think this convention should be recommended for separators used 
anywhere in Emacs going forward. I'm focusing mostly on 
context-menu-mode simply because it's pretty new, so there won't be much 
third-party code that relies on the old (`separator-foo') naming 
convention yet; it shouldn't be too risky to change this now. On the 
other hand, separators in the menu-bar (for example) have been around 
for longer, so I think it's probably too late to change those.

Overall, my guideline would be: use `foo-separator' when adding a new 
separator to Emacs, unless that part of the code consistently uses the 
`separator-foo' naming convention. In that case, it's probably best to 
stick with the existing scheme in those parts in order to be more 
internally-consistent.

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Eli Zaretskii]

> Cc: 52293@debbugs.gnu.org, juri@linkov.net
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Sun, 12 Dec 2021 12:27:34 -0800
> 
> On 12/11/2021 11:02 PM, Eli Zaretskii wrote:
> > Are you saying that we will be recommending a convention for separator
> > names only for menus popped up in context-menu-mode?  Does it make
> > sense to have such a specialized convention?  Isn't it possible to
> > show context menus outside of the context-menu-mode?
> 
> No, I think this convention should be recommended for separators used 
> anywhere in Emacs going forward.

Ok, so then the effect of this change is wide, as I envisioned, and
compatibility considerations still apply.

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Jim Porter]

On 12/12/2021 12:43 PM, Eli Zaretskii wrote:
>> Cc: 52293@debbugs.gnu.org, juri@linkov.net
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Sun, 12 Dec 2021 12:27:34 -0800
>>
>> On 12/11/2021 11:02 PM, Eli Zaretskii wrote:
>>> Are you saying that we will be recommending a convention for separator
>>> names only for menus popped up in context-menu-mode?  Does it make
>>> sense to have such a specialized convention?  Isn't it possible to
>>> show context menus outside of the context-menu-mode?
>>
>> No, I think this convention should be recommended for separators used
>> anywhere in Emacs going forward.
> 
> Ok, so then the effect of this change is wide, as I envisioned, and
> compatibility considerations still apply.

I'm also ok with applying this naming convention *only* to context menus 
and not treating this as a general guideline; that would still make it 
easier to remember the names of each context menu separator without 
having to double-check the source code.

I don't even have a problem with closing that bug (bug#52286) as 
wontfix, since it's just a minor issue anyway. It's really not a big 
deal if the naming is inconsistent, so long as the names are still unique.

I think this patch, which adds `top-separator' to help the 
de-duplication logic work better, would be useful to apply regardless of 
the decision on how the separators should be named.

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Eli Zaretskii]

> Cc: 52293@debbugs.gnu.org, juri@linkov.net
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Sun, 12 Dec 2021 13:59:16 -0800
> 
> On 12/12/2021 12:43 PM, Eli Zaretskii wrote:
> >> Cc: 52293@debbugs.gnu.org, juri@linkov.net
> >> From: Jim Porter <jporterbugs@gmail.com>
> >> Date: Sun, 12 Dec 2021 12:27:34 -0800
> >>
> >> On 12/11/2021 11:02 PM, Eli Zaretskii wrote:
> >>> Are you saying that we will be recommending a convention for separator
> >>> names only for menus popped up in context-menu-mode?  Does it make
> >>> sense to have such a specialized convention?  Isn't it possible to
> >>> show context menus outside of the context-menu-mode?
> >>
> >> No, I think this convention should be recommended for separators used
> >> anywhere in Emacs going forward.
> > 
> > Ok, so then the effect of this change is wide, as I envisioned, and
> > compatibility considerations still apply.
> 
> I'm also ok with applying this naming convention *only* to context menus 
> and not treating this as a general guideline; that would still make it 
> easier to remember the names of each context menu separator without 
> having to double-check the source code.

I don't think this would make sense, and I said that above.

bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Jim Porter]

On 12/13/2021 4:23 AM, Eli Zaretskii wrote:
>> Cc: 52293@debbugs.gnu.org, juri@linkov.net
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Sun, 12 Dec 2021 13:59:16 -0800
>>
>> On 12/12/2021 12:43 PM, Eli Zaretskii wrote:
>>>> Cc: 52293@debbugs.gnu.org, juri@linkov.net
>>>> From: Jim Porter <jporterbugs@gmail.com>
>>>> Date: Sun, 12 Dec 2021 12:27:34 -0800
>>>>
>>>> On 12/11/2021 11:02 PM, Eli Zaretskii wrote:
>>>>> Are you saying that we will be recommending a convention for separator
>>>>> names only for menus popped up in context-menu-mode?  Does it make
>>>>> sense to have such a specialized convention?  Isn't it possible to
>>>>> show context menus outside of the context-menu-mode?
>>>>
>>>> No, I think this convention should be recommended for separators used
>>>> anywhere in Emacs going forward.
>>>
>>> Ok, so then the effect of this change is wide, as I envisioned, and
>>> compatibility considerations still apply.
>>
>> I'm also ok with applying this naming convention *only* to context menus
>> and not treating this as a general guideline; that would still make it
>> easier to remember the names of each context menu separator without
>> having to double-check the source code.
> 
> I don't think this would make sense, and I said that above.

I don't really have a preference about what we recommend in the 
documentation. We don't need to recommend anything at all. Above, I was 
just referring to changing the separator names within context-menu-mode 
functions to use a single style (`foo-separator'), not necessarily 
documenting/recommending it.

Even the code change I only have a very slight preference for. I was 
just reading through the context-menu code to understand it, saw that 
some separators were named `foo-separator' and others named 
`separator-foo', and thought that was a bit odd. The only "problem" (and 
it's barely a problem) is that programmers writing code referring to 
these separators later (e.g. with `define-key-after') might get the 
names mixed up if they're not paying attention. Since I came across it, 
I thought I'd submit a patch in case the maintainers wanted it.

However, if it's controversial (or just too late in the release cycle to 
make a small compatibility break), I don't have a problem with closing 
bug#52286 as wontfix. It's such a minor issue that I really don't see 
any major downside to wontfixing the bug if there's any hesitation about it.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Drew Adams]

> No, I think this convention should be recommended for separators used
> anywhere in Emacs going forward. I'm focusing mostly on
> context-menu-mode simply because it's pretty new, so there won't be much
> third-party code that relies on the old (`separator-foo') naming
> convention yet; it shouldn't be too risky to change this now. On the
> other hand, separators in the menu-bar (for example) have been around
> for longer, so I think it's probably too late to change those.
> 
> Overall, my guideline would be: use `foo-separator' when adding a new
> separator to Emacs, unless that part of the code consistently uses the
> `separator-foo' naming convention. In that case, it's probably best to
> stick with the existing scheme in those parts in order to be more
> internally-consistent.

Caveat: I'm not following this thread.  FWIW:

1. In my code I have _lots_ of menu separators that
are apparently in what you call the old naming
convention.

2. I believe there was no stated convention.

3. I think there's zero need for any naming
   convention for menu-item names, whether
   separators or other.

4. As I stated early on in this thread, I think
   it's misguided to prevent the use of duplicate
   separators.  If someone wants such duplicates
   for some reason (and there can be any number
   of reasons), let them be.  And if someone, for
   some reason, wants to prevent such duplicates
   they can do so easily enough, manually or by
   code.

   Don't install useless roadblocks in the way of
   users.

Just one opinion.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Jim Porter]

On 12/12/2021 1:00 PM, Drew Adams wrote:
> Caveat: I'm not following this thread.  FWIW:
> 
> 1. In my code I have _lots_ of menu separators that
> are apparently in what you call the old naming
> convention.

That's fine. I'd prefer we *not* update old menu separators, since 
that's a compatibility break. The only exception would be for existing 
context menu separators, since they're pretty new anyway, so the chance 
of any code relying on the old context menu names is low.

> 3. I think there's zero need for any naming
>     convention for menu-item names, whether
>     separators or other.

The goal was to make it easier to remember the names of the separators 
if you wanted to add a new item after a particular separator that had 
already been added. It's easier to remember if they're all 
`foo-separator' instead of a mix of styles.

> 4. As I stated early on in this thread, I think
>     it's misguided to prevent the use of duplicate
>     separators.  If someone wants such duplicates
>     for some reason (and there can be any number
>     of reasons), let them be.  And if someone, for
>     some reason, wants to prevent such duplicates
>     they can do so easily enough, manually or by
>     code.

Technically, this is already possible by using extended-format menu 
items. Only simple separators are de-duplicated. So this would be 
de-duplicated:

   (define-key menu [foo-separator] '("--"))

But this wouldn't:

   (define-key menu [bar-separator] '(menu-item "--"))

That's not documented though, and I'm not sure what promises we should 
make here. It might be better to have a more-explicit way of opting into 
de-duplication, but I'm not sure what that would be off-hand.

It may be possible for context menu functions to be more careful about 
the insertion of separators so that duplicates never crop up in the 
first place. However, that would take a bit of experimentation, and I'm 
not sure of all the pros and cons of a solution like that. Maybe Juri 
has some thoughts on this though.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Jim Porter]

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

On 12/12/2021 2:12 PM, Jim Porter wrote:
> It may be possible for context menu functions to be more careful about 
> the insertion of separators so that duplicates never crop up in the 
> first place. However, that would take a bit of experimentation, and I'm 
> not sure of all the pros and cons of a solution like that. Maybe Juri 
> has some thoughts on this though.

Here's a *very* experimental patch that handles separators in a totally 
different way. Instead of removing duplicates, this *inserts* separators 
between different sections of the context menu. This works by giving 
menu items a `:section' property, and if that changes (and both the old 
and new section names are non-nil), the code inserts a separator between 
the two items.

This patch only really works for elisp-mode using the default context 
menu functions, since I didn't want to spend too much time updating 
everything for a small experiment. It shouldn't be terribly hard to 
update all the other context menu functions if we decide to go with 
something like this though.

This strategy seems less brittle from my experiments so far, since we no 
longer have to be so careful about getting the order of the keys exactly 
right in order to be able to detect the duplicated separators. It also 
makes it easier to insert duplicated separators if that's really what 
you want.

[-- Attachment #2: context-menu-sections.patch --]
[-- Type: text/plain, Size: 10486 bytes --]

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 11fdd3f639..d34991104e 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -327,32 +327,29 @@ context-menu-map
                           (setq menu (funcall fun menu click))
                           nil)))
 
-    ;; Remove duplicate separators as well as ones at the beginning or
-    ;; end of the menu.
-    (let ((l menu) saw-first-item)
+    ;; Insert separators between different sections of the context menu.
+    (let ((l menu) section)
       (while (and (consp l)
                   (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 (consp (cadr l))
+          (when-let ((next-section (plist-get (nthcdr 4 (cadr l)) :section)))
+            (when (and section (not (eq section next-section)))
+              ;; The section name has changed. Insert a new separator.
+              (setcdr l (cons `(,(make-symbol
+                                  (concat "separator-" next-section))
+                                "--")
+                              (cdr l))))
+            (setq section next-section)))
+        (setq l (cdr l))))
 
     (when (functionp context-menu-filter-function)
       (setq menu (funcall context-menu-filter-function menu click)))
     menu))
 
 (defun context-menu-middle-separator (menu _click)
-  "Add separator to the middle of the context menu.
+  "Add invisible separator to the middle of the context menu.
 Some context functions add menu items below the separator."
-  (define-key-after menu [middle-separator] menu-bar-separator)
+  (define-key-after menu [middle-separator] '(menu-item "--" nil :visible nil))
   menu)
 
 (defun context-menu-toolbar (menu _click)
@@ -380,13 +377,12 @@ context-menu-global
 (defun context-menu-local (menu _click)
   "Populate MENU with submenus provided by major mode."
   (run-hooks 'activate-menubar-hook 'menu-bar-update-hook)
-  (define-key-after menu [separator-local] menu-bar-separator)
   (let ((keymap (local-key-binding [menu-bar])))
     (when keymap
       (map-keymap (lambda (key binding)
                     (when (consp binding)
                       (define-key-after menu (vector key)
-                        (copy-sequence binding))))
+                        (append binding '(:section "local")))))
                   (menu-bar-keymap keymap))))
   menu)
 
@@ -422,7 +418,6 @@ context-menu-vc
 
 (defun context-menu-undo (menu _click)
   "Populate MENU with undo commands."
-  (define-key-after menu [separator-undo] menu-bar-separator)
   (when (and (not buffer-read-only)
              (not (eq t buffer-undo-list))
              (if (eq last-command 'undo)
@@ -430,22 +425,25 @@ context-menu-undo
                (consp buffer-undo-list)))
     (define-key-after menu [undo]
       `(menu-item ,(if (region-active-p) "Undo in Region" "Undo") undo
-                  :help "Undo last edits")))
+                  :help "Undo last edits"
+                  :section "undo")))
   (when (and (not buffer-read-only)
              (undo--last-change-was-undo-p buffer-undo-list))
     (define-key-after menu [undo-redo]
       `(menu-item (if undo-in-region "Redo in Region" "Redo") undo-redo
-                  :help "Redo last undone edits")))
+                  :help "Redo last undone edits"
+                  :section "undo")))
   menu)
 
 (defun context-menu-region (menu click)
   "Populate MENU with region commands."
-  (define-key-after menu [separator-region] menu-bar-separator)
+  ;;(define-key-after menu [separator-region] menu-bar-separator)
   (when (and mark-active (not buffer-read-only))
     (define-key-after menu [cut]
       '(menu-item "Cut" kill-region
                   :help
-                  "Cut (kill) text in region between mark and current position")))
+                  "Cut (kill) text in region between mark and current position"
+                  :section "region")))
   (when mark-active
     (define-key-after menu [copy]
       ;; ns-win.el said: Substitute a Copy function that works better
@@ -456,7 +454,8 @@ context-menu-region
                   :help "Copy text in region between mark and current position"
                   :keys ,(if (featurep 'ns)
                              "\\[ns-copy-including-secondary]"
-                           "\\[kill-ring-save]"))))
+                           "\\[kill-ring-save]")
+                  :section "region")))
   (when (and (or (gui-backend-selection-exists-p 'CLIPBOARD)
                  (if (featurep 'ns) ; like paste-from-menu
                      (cdr yank-menu)
@@ -464,7 +463,8 @@ context-menu-region
              (not buffer-read-only))
     (define-key-after menu [paste]
       `(menu-item "Paste" mouse-yank-at-click
-                  :help "Paste (yank) text most recently cut/copied")))
+                  :help "Paste (yank) text most recently cut/copied"
+                  :section "region")))
   (when (and (cdr yank-menu) (not buffer-read-only))
     (let ((submenu (make-sparse-keymap (propertize "Paste from Kill Menu")))
           (i 0))
@@ -477,12 +477,14 @@ context-menu-region
       (define-key-after menu (if (featurep 'ns) [select-paste] [paste-from-menu])
         `(menu-item ,(if (featurep 'ns) "Select and Paste" "Paste from Kill Menu")
                     ,submenu
-                    :help "Choose a string from the kill ring and paste it"))))
+                    :help "Choose a string from the kill ring and paste it"
+                    :section "region"))))
   (when (and mark-active (not buffer-read-only))
     (define-key-after menu [clear]
       '(menu-item "Clear" delete-active-region
                   :help
-                  "Delete text in region between mark and current position")))
+                  "Delete text in region between mark and current position"
+                  :section "region")))
 
   (let ((submenu (make-sparse-keymap (propertize "Select"))))
     (define-key-after submenu [mark-whole-buffer]
@@ -509,7 +511,7 @@ context-menu-region
                     :help "Deactivate the region")))
 
     (define-key-after menu [select-region]
-      `(menu-item "Select" ,submenu)))
+      `(menu-item "Select" ,submenu :section "region")))
   menu)
 
 (defun context-menu-ffap (menu click)
diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 7da93a351a..79960f3839 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -156,9 +156,6 @@ emacs-lisp-mode-menu
 (defun elisp-context-menu (menu click)
   "Populate MENU with symbol help commands at CLICK."
   (when (thing-at-mouse click 'symbol)
-    (define-key-after menu [elisp-separator] menu-bar-separator
-      'middle-separator)
-
     (let* ((string (thing-at-mouse click 'symbol t))
            (symbol (when (stringp string) (intern string)))
            (title (cond
@@ -178,14 +175,16 @@ elisp-context-menu
           `(menu-item "Look up in Manual"
                       (lambda (_click) (interactive "e")
                         (info-lookup-symbol ',symbol))
-                      :help ,(format "Find `%s' in relevant manual" symbol))
-          'elisp-separator)
+                      :help ,(format "Find `%s' in relevant manual" symbol)
+                      :section "elisp")
+          'middle-separator)
         (define-key-after menu [describe-symbol]
           `(menu-item (format "Describe %s" ,title)
                       (lambda (_click) (interactive "e")
                         (describe-symbol ',symbol))
-                      :help ,(format "Display the documentation of `%s'" symbol))
-          'elisp-separator))))
+                      :help ,(format "Display the documentation of `%s'" symbol)
+                      :section "elisp")
+          'middle-separator))))
   menu)
 
 (defun emacs-lisp-byte-compile ()
diff --git a/lisp/progmodes/prog-mode.el b/lisp/progmodes/prog-mode.el
index 496b081018..aa25080ab5 100644
--- a/lisp/progmodes/prog-mode.el
+++ b/lisp/progmodes/prog-mode.el
@@ -46,20 +46,19 @@ prog-mode-hook
 (defun prog-context-menu (menu click)
   "Populate MENU with xref commands at CLICK."
   (require 'xref)
-  (define-key-after menu [prog-separator] menu-bar-separator
-    'middle-separator)
-
   (unless (xref-forward-history-empty-p)
     (define-key-after menu [xref-forward]
       '(menu-item "Go Forward" xref-go-forward
-                  :help "Forward to the position gone Back from")
-      'prog-separator))
+                  :help "Forward to the position gone Back from"
+                  :section "prog")
+      'middle-separator))
 
   (unless (xref-marker-stack-empty-p)
     (define-key-after menu [xref-pop]
       '(menu-item "Go Back" xref-go-back
-                  :help "Back to the position of the last search")
-      'prog-separator))
+                  :help "Back to the position of the last search"
+                  :section "prog")
+      'middle-separator))
 
   (let ((identifier (save-excursion
                       (mouse-set-point click)
@@ -68,12 +67,14 @@ prog-context-menu
     (when identifier
       (define-key-after menu [xref-find-ref]
         `(menu-item "Find References" xref-find-references-at-mouse
-                    :help ,(format "Find references to `%s'" identifier))
-        'prog-separator)
+                    :help ,(format "Find references to `%s'" identifier)
+                    :section "prog")
+        'middle-separator)
       (define-key-after menu [xref-find-def]
         `(menu-item "Find Definition" xref-find-definitions-at-mouse
-                    :help ,(format "Find definition of `%s'" identifier))
-        'prog-separator)))
+                    :help ,(format "Find definition of `%s'" identifier)
+                    :section "prog")
+        'middle-separator)))
 
   (when (thing-at-mouse click 'symbol)
     (define-key-after menu [select-region mark-symbol]

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Drew Adams]

> > 3. I think there's zero need for any naming
> >     convention for menu-item names, whether
> >     separators or other.
> 
> The goal was to make it easier to remember the names of the separators
> if you wanted to add a new item after a particular separator that had
> already been added. It's easier to remember if they're all
> `foo-separator' instead of a mix of styles.

Why does anyone need to _remember_ such names?
What's the use case for remembering?

> > 4. As I stated early on in this thread, I think
> >     it's misguided to prevent the use of duplicate
> >     separators.  If someone wants such duplicates
> >     for some reason (and there can be any number
> >     of reasons), let them be.  And if someone, for
> >     some reason, wants to prevent such duplicates
> >     they can do so easily enough, manually or by
> >     code.
> 
> Technically, this is already possible by using extended-format menu
> items. Only simple separators are de-duplicated. So this would be
> de-duplicated:
> 
>    (define-key menu [foo-separator] '("--"))
> 
> But this wouldn't:
> 
>    (define-key menu [bar-separator] '(menu-item "--"))

Deduplication?  We're not talking about removing
exact duplicates are we?  I thought this was about
removing consecutive separators, leaving only one.
This involves no duplicate menu items:

  (define-key menu [separator-1] '("--"))
  (define-key menu [separator-2] '("--"))

More importantly, I didn't know this was about
removing any ordinary `define-key' bindings.
I really hope it's not.  I thought this was only
for Juri's new context menus.

If we're now automatically removing consecutive
separators coded with `define-key' then count me
as one user who's definitely against that.

What can possibly be the reason for imposing that
kind of meddling with someone's code, preventing
Emacs from showing consecutive separators (without
having to add superfluous `menu-item's)? 

> That's not documented though, and I'm not sure 
> what promises we should make here. 

The only promise I, as one user, am interested
in hearing is not to neuter code that creates
consecutive menu separators.

> It might be better to have a more-explicit way of opting into
> de-duplication, but I'm not sure what that would be off-hand.

Why should we even provide such removal?  If
someone doesn't want it they won't code it.
That's all.

And if it appears because some menu items that
might otherwise be present between 2 separators
aren't displayed (e.g. because of :visible),
then it's up to the coder to deal with that.
Maybe she wants to show that there are missing
menu items, by using consecutive separators.

I don't claim to know what you're really doing,
but IIUC, this is overoverengineering.

If you instead just provided a coding way for
someone to indicate, at some particular part of
a menu, that a separator shouldn't be shown if
it directly follows another separator, then just
do that.

(And that should already be possible, using
:invisible for the separator itself.)

> It may be possible for context menu functions

Is this all only about _context_ menus or not?

If it is, I don't care a lot.  But if it's about
menus generally, then what I think I'm hearing
isn't something I'm in favor of.  But again,
just one opinion.

> to be more careful about
> the insertion of separators so that duplicates never crop up in the
> first place.

Why do you care if they "crop up"?

> However, that would take a bit of experimentation, and I'm
> not sure of all the pros and cons of a solution like that. Maybe Juri
> has some thoughts on this though.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Jim Porter]

On 12/12/2021 5:16 PM, Drew Adams wrote:
>>> 3. I think there's zero need for any naming
>>>      convention for menu-item names, whether
>>>      separators or other.
>>
>> The goal was to make it easier to remember the names of the separators
>> if you wanted to add a new item after a particular separator that had
>> already been added. It's easier to remember if they're all
>> `foo-separator' instead of a mix of styles.
> 
> Why does anyone need to _remember_ such names?
> What's the use case for remembering?

A third-party package author may want to insert context menu items in a 
particular spot in the menu (i.e. use `define-key-after'). Because the 
naming convention of separators is inconsistent, the author can more 
easily slip up and use the wrong name.

As you might be able to tell, this is a very small issue. But the fix 
was easy, so I posted a patch for it. I really don't have a problem with 
closing that bug as wontfix if it's controversial.

> Deduplication?  We're not talking about removing
> exact duplicates are we?  I thought this was about
> removing consecutive separators, leaving only one.
> This involves no duplicate menu items:
> 
>    (define-key menu [separator-1] '("--"))
>    (define-key menu [separator-2] '("--"))

That's correct. The above menu items would be de-duplicated since 
they're both separators. However, that logic only applies to "simple" 
separators, so the de-duplication code doesn't apply to this:

   (define-key menu [separator-1] '(menu-item "--"))
   (define-key menu [separator-2] '(menu-item "--"))

(It's possible that's just an accidental omission in the original code 
though.)

> More importantly, I didn't know this was about
> removing any ordinary `define-key' bindings.
> I really hope it's not.  I thought this was only
> for Juri's new context menus.

This is *only* for context menus. The de-duplication code is in 
`context-menu-map', which is a function that gets called when opening 
the context menu. It calls all the elements of `context-menu-functions' 
in order, and then does the de-duplication step. No other menus will be 
affected by this.

In any case, I think I prefer the solution I proposed in my followup: 
instead of removing consecutive separators so things look right, we can 
*add* separators between different "sections" of the context menu. 
Sections can be marked by the `:section' keyword and a name for the 
section (note: this only applies to the context menu, not other menus). 
That should completely prevent any unwanted manipulation of menus, since 
the programmer needs to opt into this behavior explicitly.

In my limited tests, that method is also less brittle than the current 
method (this bug is an example of the brittleness). See my other 
message[1] for more details.

[1] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-12/msg01079.html

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Drew Adams]

> This is *only* for context menus. 

OK, thanks.  Very glad to hear that.
In that case, you can forget I said anything,
and sorry for the noise.

> In any case, I think I prefer the solution I proposed in my followup:
> instead of removing consecutive separators so things look right, we can
> *add* separators between different "sections" of the context menu.
> Sections can be marked by the `:section' keyword and a name for the
> section (note: this only applies to the context menu, not other menus).
> That should completely prevent any unwanted manipulation of menus, since
> the programmer needs to opt into this behavior explicitly.

Sounds better to me too, FWIW.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

>> This involves no duplicate menu items:
>>    (define-key menu [separator-1] '("--"))
>>    (define-key menu [separator-2] '("--"))
>
> That's correct. The above menu items would be de-duplicated since they're
> both separators. However, that logic only applies to "simple" separators,
> so the de-duplication code doesn't apply to this:
>
>   (define-key menu [separator-1] '(menu-item "--"))
>   (define-key menu [separator-2] '(menu-item "--"))
>
> (It's possible that's just an accidental omission in the original code
> though.)

Actually, this wasn't an omission.  Now that I'm thinking more about this,
maybe separators that are subject to possible removal could be marked by e.g.
using text properties, thus opting into this behavior explicitly:

  (defconst context-menu-separator (list (propertize "--" 'remove t)))
  (define-key menu [separator-1] context-menu-separator)
  (define-key menu [separator-2] context-menu-separator)

Then code that de-duplicates separators could check for this property.

> In any case, I think I prefer the solution I proposed in my followup:
> instead of removing consecutive separators so things look right, we can
> *add* separators between different "sections" of the context menu. Sections
> can be marked by the `:section' keyword and a name for the section (note:
> this only applies to the context menu, not other menus). That should
> completely prevent any unwanted manipulation of menus, since the programmer
> needs to opt into this behavior explicitly.
>
> In my limited tests, that method is also less brittle than the current
> method (this bug is an example of the brittleness). See my other message[1]
> for more details.

Adding a new keyword to every menu item requires more work from authors
of context menus, and actually makes menus more brittle -
when an author forgets to add the new keyword `:section' to some menu item,
then two unexpected separators will be inserted: before and after
such an item.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Jim Porter]

On 12/13/2021 12:47 AM, Juri Linkov wrote:
> Actually, this wasn't an omission.  Now that I'm thinking more about this,
> maybe separators that are subject to possible removal could be marked by e.g.
> using text properties, thus opting into this behavior explicitly:
> 
>    (defconst context-menu-separator (list (propertize "--" 'remove t)))
>    (define-key menu [separator-1] context-menu-separator)
>    (define-key menu [separator-2] context-menu-separator)
> 
> Then code that de-duplicates separators could check for this property.

If we did that, how about using an extend-format separator, since it 
already supports properties? We could just add a new property for the 
de-duplicator to check:

   (define-key menu [separator-1] '(menu-item "--" nil :deduplicate t))

Maybe there's a relatively simple way to reuse `:visible' for this?

One benefit to this being opt-in is that if people wanted this behavior 
for other menus, it would be possible to add it without breaking any 
existing code.

> Adding a new keyword to every menu item requires more work from authors
> of context menus, and actually makes menus more brittle -
> when an author forgets to add the new keyword `:section' to some menu item,
> then two unexpected separators will be inserted: before and after
> such an item.

The way I've implemented this now shouldn't have this problem: if a menu 
item doesn't have a `:section', it's treated as being in the same 
section as the previous item (unless there were no sections before this 
item; in that case, it's treated as being in the same section as the 
next item). It's only actually *required* to specify the section for the 
first item in the section.

One of the main benefits here is that we don't have to be as careful 
with the order of menu items. For example, my previous patch[1] adds 
`top-separator' so that we can ensure the context menu title is always 
the first item in the keymap in order to let us find consecutive 
separators. With `:section', the `top-separator' patch can be thrown 
out, and programmers can use `define-key' to add menu items to the top 
as they normally would.

However, using `:section' makes it harder to insert new items at the 
beginning of a previously-defined section. With separators, you can just 
call `define-key-after' and pass in the separator's name, but it's 
pretty tricky when using `:section'. One way to handle this would be to 
add `define-key-before', but then the programmer still has to remember 
to add `:section'.

In the end, there's a tradeoff with each implementation. When using 
separators, programmers have to be careful to use `define-key-after' 
(and if we add a property to opt into de-duplication, to use the 
property). When using `:section', programmers have to remember to set 
the section, and we probably want to add something like 
`define-key-before' to make things easier[2]. I think the implementation 
of `context-menu-map' is slightly easier to follow for `:section' too, 
but the difference isn't major (that said, my current implementation is 
just a sketch and could use some cleanup).

[1] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-12/msg00709.html
[2] This may be useful in general though. It's not *strictly* necessary, 
but it'd be helpful in any case where you want to insert a menu item 
before another, but you don't know the name of the item already 
preceding it in the menu.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

>> Actually, this wasn't an omission.  Now that I'm thinking more about this,
>> maybe separators that are subject to possible removal could be marked by e.g.
>> using text properties, thus opting into this behavior explicitly:
>>    (defconst context-menu-separator (list (propertize "--" 'remove t)))
>>    (define-key menu [separator-1] context-menu-separator)
>>    (define-key menu [separator-2] context-menu-separator)
>> Then code that de-duplicates separators could check for this property.
>
> If we did that, how about using an extend-format separator, since it
> already supports properties? We could just add a new property for the
> de-duplicator to check:
>
>   (define-key menu [separator-1] '(menu-item "--" nil :deduplicate t))
>
> Maybe there's a relatively simple way to reuse `:visible' for this?
>
> One benefit to this being opt-in is that if people wanted this behavior for
> other menus, it would be possible to add it without breaking any existing
> code.

I agree that using an extend-format separator is a good idea,
especially if we'll provide a special variable that will contain it.
But I don't understand why opt-in.  I think opt-out would be better
since most of the time the users want to have de-duplicated menus.

>> Adding a new keyword to every menu item requires more work from authors
>> of context menus, and actually makes menus more brittle -
>> when an author forgets to add the new keyword `:section' to some menu item,
>> then two unexpected separators will be inserted: before and after
>> such an item.
>
> The way I've implemented this now shouldn't have this problem: if a menu
> item doesn't have a `:section', it's treated as being in the same section
> as the previous item (unless there were no sections before this item; in
> that case, it's treated as being in the same section as the next
> item). It's only actually *required* to specify the section for the first
> item in the section.

So the authors will have the requirement to be careful to mark the first
menu item.  When the menus are added conditionally, then authors will need
to mark several menu items that can become the first menu item.

> One of the main benefits here is that we don't have to be as careful with
> the order of menu items. For example, my previous patch[1] adds
> `top-separator' so that we can ensure the context menu title is always the
> first item in the keymap in order to let us find consecutive
> separators. With `:section', the `top-separator' patch can be thrown out,
> and programmers can use `define-key' to add menu items to the top as they
> normally would.

The same way the `top-separator' can be avoided when post-processing code
will search the menu title and move it to the beginning before starting
to remove duplicates, or to use some more complex logic that takes into account
the location of the menu title in the middle of the menu.

> However, using `:section' makes it harder to insert new items at the
> beginning of a previously-defined section. With separators, you can just
> call `define-key-after' and pass in the separator's name, but it's pretty
> tricky when using `:section'. One way to handle this would be to add
> `define-key-before', but then the programmer still has to remember to add
> `:section'.

Please also consider additional costs for authors to learn about this
subset of the standard menu functionality with more features.
Currently authors have no problems with constructing the standard menus
with separators, but with `:section' they need to learn that this feature
is not available in all menus, but only in context menus, this would be
too confusing.

> In the end, there's a tradeoff with each implementation. When using
> separators, programmers have to be careful to use `define-key-after' (and
> if we add a property to opt into de-duplication, to use the property). When
> using `:section', programmers have to remember to set the section, and we
> probably want to add something like `define-key-before' to make things
> easier[2]. I think the implementation of `context-menu-map' is slightly
> easier to follow for `:section' too, but the difference isn't major (that
> said, my current implementation is just a sketch and could use some
> cleanup).
>
> [1] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-12/msg00709.html
> [2] This may be useful in general though. It's not *strictly* necessary,
> but it'd be helpful in any case where you want to insert a menu item before
> another, but you don't know the name of the item already preceding it in
> the menu.

Maybe better first to try fixing the problem using the separators?
When this doesn't help, only then we could consider alternatives.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Jim Porter]

On 12/13/2021 10:58 AM, Juri Linkov wrote:
> I agree that using an extend-format separator is a good idea,
> especially if we'll provide a special variable that will contain it.
> But I don't understand why opt-in.  I think opt-out would be better
> since most of the time the users want to have de-duplicated menus.

One benefit to this being opt-in would be that it would be possible to 
(eventually) support the de-duplication behavior for *all* menus without 
us having to worry about breaking compatibility for other menus[1]. If 
we provide a constant for people to use like in your earlier example[2], 
it would be still be reasonably simple to use.

I think it would be nice to (again, eventually) support this for all 
menus, since it's probably useful in some other places, and it would 
keep the differences between specific menus' behaviors to a minimum.

> Maybe better first to try fixing the problem using the separators?
> When this doesn't help, only then we could consider alternatives.

Ok, let's stick with deduplication then. I'll try to write a patch that 
doesn't require `top-separator', since that's easy for programmers to 
forget about (no other menu requires doing that to put items at the 
beginning).

[1] It's not *likely* that people want consecutive separators in other 
menus, but I think it's good to be on the safe side and not break 
compatibility for anyone who does want that.

[2] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-12/msg01100.html

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

>> I agree that using an extend-format separator is a good idea,
>> especially if we'll provide a special variable that will contain it.
>> But I don't understand why opt-in.  I think opt-out would be better
>> since most of the time the users want to have de-duplicated menus.
>
> One benefit to this being opt-in would be that it would be possible to
> (eventually) support the de-duplication behavior for *all* menus without us
> having to worry about breaking compatibility for other menus[1]. If we
> provide a constant for people to use like in your earlier example[2], it
> would be still be reasonably simple to use.
>
> I think it would be nice to (again, eventually) support this for all menus,
> since it's probably useful in some other places, and it would keep the
> differences between specific menus' behaviors to a minimum.

As Drew already noted, other menus have no such problem because
they are static, and the authors can easily ensure that a single
separator is used between submenus.  But context menus are
dynamically constructed, and it's easier for authors just to
add a separator even for empty submenus, relying on post-processing
that will de-duplicate them.

So better to make this opt-out for context menus, and opt-in
for other menus, although I think this feature is not needed
for other menus.

>> Maybe better first to try fixing the problem using the separators?
>> When this doesn't help, only then we could consider alternatives.
>
> Ok, let's stick with deduplication then. I'll try to write a patch that
> doesn't require `top-separator', since that's easy for programmers to
> forget about (no other menu requires doing that to put items at the
> beginning).

Thanks.  The current question was raised by Drew who worried that
sometimes authors might want to leave two adjacent separators
for an empty submenu.  But you answered this question that it can be
addressed by using a special form of a separator with a menu-item.
So I see no more problems here.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Tue, 14 Dec 2021 10:30:10 +0200
> Cc: "52293@debbugs.gnu.org" <52293@debbugs.gnu.org>
> 
> As Drew already noted, other menus have no such problem because
> they are static, and the authors can easily ensure that a single
> separator is used between submenus.

No, other menus aren't static, because Emacs adds and removes items to
some menus as we see fit.  So I think this distinction is artificial
and incorrect.  It is true that context menus are "more dynamic", so
to say, but that's not a fundamental distinction.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Drew Adams]

> > As Drew already noted, other menus have no such problem because
> > they are static, and the authors can easily ensure that a single
> > separator is used between submenus.
> 
> No, other menus aren't static, because Emacs adds and removes items to
> some menus as we see fit.  So I think this distinction is artificial
> and incorrect.  It is true that context menus are "more dynamic", so
> to say, but that's not a fundamental distinction.

To be clear, I never said that other menus are static.

I said only that someone (for whatever reasons) might
want to provide or allow consecutive separators, and
that that should be possible.  That's all.  And I
said that programmers can anyway make separators,
like other menu items, conditional (e.g. invisible).

And wrt Juri's context menus, I said I really don't
care much what you do wrt separators.

I've elsewhere expressed my displeasure in seeing
context menus implemented in the way Emacs is doing
that, but that was ignored.  (I use my own approach
to providing mouse-3 context menus, which allows the
standard, longstanding Emacs mouse-3 behavior at the
same time.)

My posts in this thread were only a concern that
automatic separator deletion be limited to context
menus.  It was confirmed that they are, which is good.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

> I said only that someone (for whatever reasons) might
> want to provide or allow consecutive separators, and
> that that should be possible.  That's all.  And I
> said that programmers can anyway make separators,
> like other menu items, conditional (e.g. invisible).

I was referring to your following words:

  Why should we even provide such removal?  If
  someone doesn't want it they won't code it.
  That's all.

But the problem is that it's not easy not to add separators that
become duplicated when no menu items are added to submenus.

> I've elsewhere expressed my displeasure in seeing
> context menus implemented in the way Emacs is doing
> that, but that was ignored.  (I use my own approach
> to providing mouse-3 context menus, which allows the
> standard, longstanding Emacs mouse-3 behavior at the
> same time.)

Interesting.  Could you please describe your approach.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Drew Adams]

> > I said only that someone (for whatever reasons) might
> > want to provide or allow consecutive separators, and
> > that that should be possible.  That's all.  And I
> > said that programmers can anyway make separators,
> > like other menu items, conditional (e.g. invisible).
> 
> I was referring to your following words:
> 
>   Why should we even provide such removal?  If
>   someone doesn't want it they won't code it.
>   That's all.
> 
> But the problem is that it's not easy not to add separators that
> become duplicated when no menu items are added to submenus.

Please read the rest of what I wrote.

I specifically pointed out that some conditional menu
items might not appear, resulting in consecutive
separators. And that such a possibility might, or
might not, be what someone wants.

And I mentioned the possibility of making separators
themselves conditional.  You can programmatically
control what happens dynamically.

Of course, if you're only trying to insert some item
into an existing menu, then do more than just insert
the item.  At the limit, you might even need to
reprogram the menu, or at least some part of it
beyond that new item.

And I specifically mentioned the problem of having
two separators end up being consecutive because of a
dynamic situation - even just conditional insertion
or removal of some items.  I was the first one (maybe
the only one?) to have mentioned that possibility.

There are multiple ways in which a menu, including its
separators, can be "dynamic".  I'm well aware of that,
as I think you know.  

> > I've elsewhere expressed my displeasure in seeing
> > context menus implemented in the way Emacs is doing
> > that, but that was ignored.  (I use my own approach
> > to providing mouse-3 context menus, which allows the
> > standard, longstanding Emacs mouse-3 behavior at the
> > same time.)
> 
> Interesting.  Could you please describe your approach.

I already did, including in the thread where your
context-menu was proposed.  I've described it more
than once.  If you're truly interested and haven't
paid any mind to it before, you can find a description
here:

https://www.emacswiki.org/emacs/Mouse3

And you can find the code here:

https://www.emacswiki.org/emacs/download/mouse3.el

In general, I'm in favor of:

1. Combining these two advantages:

   . Allowing for a `mouse-3' context menu
   . Emacs's longstanding `mouse-3' actions

   Users shouldn't have to sacrifice one to have
   the other.

2. Letting users easily configure such menus, for
   whatever contexts they want.

3. Letting user code do the same (e.g., dynamic
   control).

`mouse3.el' is a simple start, but it already goes
a long way toward all of that.  My impression, from
the discussion about your context-menu approach, is
that it's much less open to user and programmatic
control, and it doesn't enable the traditional
`mouse-3' behavior at the same time.

The traditional `mouse-3' behavior (including
deleting) is a strong plus, IMHO.  Many Emacs users,
even those who use a mouse heavily, might not even
be aware of it, which is too bad, IMO.  I wonder
how many are even aware of multiclicking `mouse-1'.
Again, too bad, if they're not.

Instead of throwing the traditional Emacs `mouse-3'
under the bus, we should be running it up the flag
pole and shining a light on it.

That your new context-menu feature has the effect
of throwing Emacs's traditional `mouse-3' under the
bus is just a guess of mine.  Mille excuses, if you
do in fact allow both the traditional behavior and
a context menu at the same time.

Don't ask me to prove that your approach hard-codes
things in such a way.  That was my impression when
reading your descriptions and the arguments for the
approach.  I don't claim to be an expert on what
resulted.  As an eternal optimist, I can hope that
it isn't as closed, narrow, and predefined as what
my impression of it suggests.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

> Instead of throwing the traditional Emacs `mouse-3'
> under the bus, we should be running it up the flag
> pole and shining a light on it.

I wonder how do you think it's possible to combine
the traditional `mouse-3' that operates on the region,
and `mouse-3' that pops up the context menu?

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Drew Adams]

> > Instead of throwing the traditional Emacs `mouse-3'
> > under the bus, we should be running it up the flag
> > pole and shining a light on it.
> 
> I wonder how do you think it's possible to combine
> the traditional `mouse-3' that operates on the region,
> and `mouse-3' that pops up the context menu?

Why do you wonder?  You asked that same question
yesterday, and I answered it, in the same message
you're quoting from.

Is there some part of either the description or
the code of `mouse3.el' that isn't clear in this
regard?  Did you follow those links?  Do you have
a specific question about how it works or behaves?

Here are the links again.
Description:
https://www.emacswiki.org/emacs/Mouse3
Code:
https://www.emacswiki.org/emacs/download/mouse3.el

Anyway - from the description:

 Library `mouse3.el' lets you pop up such a menu by
 using only 'mouse-3' - no need to use the keyboard
 (hitting the 'Control' key). Yet you can still use
 'mouse-3' to extend and delete the selection.

 How does it work? [read that section]

That section tells you that "`mouse3.el' redefines
standard command 'mouse-save-then-kill' in a trivial
way to give you custom behavior for a second 'mouse-3'
click at the same spot."

Instead of a second single click always deleting, you
can use it to access a context menu, to delete or
perform any other actions.

Doesn't that remove the standard second-click-deletes 
behavior?  No, because it distinguishes a `mouse-3'
double-click (which performs the usual delete action)
from a second single click (which shows a context
menu - or in fact to do anything else you like).

Vanilla Emacs doesn't distinguish these two ways to
click `mouse-3' a second time, so it misses an
opportunity to provide both a menu and the standard
extend-or-delete-region behavior.

 The redefined `mouse-save-then-kill' command just uses 
 function `mouse3-second-click-command' to handle a
 second click at the same spot. That function returns
 the command that `mouse-save-then-kill' invokes:
 either the command that is the value of variable
 `mouse3-save-then-kill-command' or, if that is `nil'
 the command that is the value of user option
 `mouse3-second-click-default-command'.

The default value of that user option is command
`mouse3-popup-menu', which pops up a `Region' menu,
which generally has items that act on the region.
___

To obtain the vanilla Emacs behavior, customize that
option value to command `mouse3-kill/delete-region'.

To _always_ have `mouse-3' pop up a context menu,
set option `mouse3-menu-always-flag' to non-`nil', or
bind `mouse-3' to `mouse3-action-wo-save-then-kill'.

IOW, both vanilla Emacs's hard-coded behavior and
an always-use-context-menu behavior are trivial
subsets of what `mouse3.el' offer.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

> Is there some part of either the description or
> the code of `mouse3.el' that isn't clear in this
> regard?  Did you follow those links?  Do you have
> a specific question about how it works or behaves?

It's too overwhelming to wade through the massive wall of text.
Thanks for the short overview below.

> Instead of a second single click always deleting, you
> can use it to access a context menu, to delete or
> perform any other actions.
>
> Doesn't that remove the standard second-click-deletes 
> behavior?  No, because it distinguishes a `mouse-3'
> double-click (which performs the usual delete action)
> from a second single click (which shows a context
> menu - or in fact to do anything else you like).

So double-click mouse-3 pops up the context menu?
But no other app uses such a gesture. So no user
would expect that the context menu should be invoked
by double-clicking the right mouse button.
Moreover, double-clicking mouse-3 is not convenient to use.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Drew Adams]

> > Instead of a second single click always deleting, you
> > can use it to access a context menu, to delete or
> > perform any other actions.
> >
> > Doesn't that remove the standard second-click-deletes
> > behavior?  No, because it distinguishes a `mouse-3'
> > double-click (which performs the usual delete action)
> > from a second single click (which shows a context
> > menu - or in fact to do anything else you like).
> 
> So double-click mouse-3 pops up the context menu?

No - reread that, or read what I repeated, below.

> But no other app uses such a gesture.

No other app lets you do what Emacs lets you do
with mouse-3.  No other app lets you do lots of
things that Emacs lets you do.

> So no user would expect that the context menu should
> be invoked by double-clicking the right mouse button.

Not without learning about it or stumbling upon
it, perhaps.  The same is true for a great deal
of Emacs - wonderful features.

The first thing to learn is how to find out about
things.  And yes, even that takes some learning.

> Moreover, double-clicking mouse-3 is not convenient to use.

I disagree.  I'll bet it's what you're already
doing when (if) you right-click `mouse-3' to
kill selected text.  I'll bet you do NOT use a
second single-click.

And anyway, it's not a double-click that brings
up a menu - that continues to do what it's always
done in Emacs: delete the active region.  It's a
_second single-click_ (at the same spot) that
brings up the menu.

(That's a design choice, so as to fit best with
traditional Emacs behavior.  Those two could be
reversed: double-click to bring up menu, two
single-clicks to delete region.)
___

And again, there are simple user actions to get
ONLY what you apparently prefer or ONLY what
Emacs users are used to:

. It's trivial to choose to always get a context
  menu instead (and yes, with a single click):
  just turn on option `mouse3-menu-always-flag'.

. It's trivial to get only the longstanding Emacs
  mouse-3 behavior (never a context menu): just
  set option `mouse3-second-click-default-command'
  to `mouse3-kill/delete-region'.  (In Customize,
  choose "Kill/delete, per `mouse-drag-copy-region'"
  from the Value Menu.)

Better such a choice than no choice.

Better such a choice AND a 3rd choice - for the
behavior of offering both: context menu and
region-kill.
___

And the doc I pointed you to isn't a "wall of text".
It's short and to the point.  The library offers
lots of possibilities.  And that "wall" also
presents examples of how to create specialized
context-sensitive menus.
___

And this isn't the first time I've described the
behavior in emails to the mailing lists.  Either
you haven't paid attention, even in your own
thread introducing what you wanted to do for
context menus.  In this current thread I'm
repeating myself.  I don't mind, if you actually
listen or are interested.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

>> Moreover, double-clicking mouse-3 is not convenient to use.
>
> I disagree.  I'll bet it's what you're already
> doing when (if) you right-click `mouse-3' to
> kill selected text.  I'll bet you do NOT use a
> second single-click.

No, I don't use `mouse-3' to kill selected text,
because it's easy to kill selected text
by choosing "Cut" from the context menu.

> And anyway, it's not a double-click that brings
> up a menu - that continues to do what it's always
> done in Emacs: delete the active region.  It's a
> _second single-click_ (at the same spot) that
> brings up the menu.

So there should be a delay longer than for double-click
before the second click pops up the context menu?

> (That's a design choice, so as to fit best with
> traditional Emacs behavior.  Those two could be
> reversed: double-click to bring up menu, two
> single-clicks to delete region.)

Still inconvenient when double-click brings up menu.

> And again, there are simple user actions to get
> ONLY what you apparently prefer or ONLY what
> Emacs users are used to:
>
> . It's trivial to choose to always get a context
>   menu instead (and yes, with a single click):
>   just turn on option `mouse3-menu-always-flag'.

Maybe context menus should be enabled by default?

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Drew Adams]

> >> Moreover, double-clicking mouse-3 is not convenient to use.
> >
> > I disagree.  I'll bet it's what you're already
> > doing when (if) you right-click `mouse-3' to
> > kill selected text.  I'll bet you do NOT use a
> > second single-click.
> 
> No, I don't use `mouse-3' to kill selected text,
> because it's easy to kill selected text
> by choosing "Cut" from the context menu.

Well, you were claiming, in general, that
double-clicking is not convenient.  Perhaps I should
have said that I'll be that it's what most users who
do use `mouse-3' to kill text do.

It seems you're maybe not the best placed to propose
that the traditional `mouse-3' behavior be shoved
under a bus, and just replaced by a context menu,
since you don't even use that traditional behavior.

I've argued that Emacs has great mouse behavior,
in particular wrt selection and deletion.  That's
comparing Emacs's mouse behavior with other mouse
behavior.  It's not comparing mouse behavior with
no mouse.

If someone does want to use the mouse for selecting
and deleting text, Emacs's mouse-3 behavior is great.

> > And anyway, it's not a double-click that brings
> > up a menu - that continues to do what it's always
> > done in Emacs: delete the active region.  It's a
> > _second single-click_ (at the same spot) that
> > brings up the menu.
> 
> So there should be a delay longer than for double-click
> before the second click pops up the context menu?

Users define the time period for a double-click.
They distinguish double-click from two clicks at the
same place.  Not a problem.  You can set that period
as brief as you like.

> > (That's a design choice, so as to fit best with
> > traditional Emacs behavior.  Those two could be
> > reversed: double-click to bring up menu, two
> > single-clicks to delete region.)
> 
> Still inconvenient when double-click brings up menu.

Your opinion's noted.  And your preference is easily
satisfied, as I explained.

Perhaps you're arguing for your preference to be the
_default_?  (That's what you're apparently doing
anyway, if your context-menu feature will, by default,
_replace_ the traditional mouse-3 behavior, instead
of being added and play well with that traditional
behavior.)

> > And again, there are simple user actions to get
> > ONLY what you apparently prefer or ONLY what
> > Emacs users are used to:
> >
> > . It's trivial to choose to always get a context
> >   menu instead (and yes, with a single click):
> >   just turn on option `mouse3-menu-always-flag'.
> 
> Maybe context menus should be enabled by default?

Clearly that's what you think.  I don't.  Not now.

Normally, we keep the default behavior when we add
a new, alternative behavior.  We add the new behavior
as an option (opt-in).

Then, after some experience and time and some user
feedback, we might consider changing the default
behavior.

I say "normally".  But we seem to no longer be in
normal times.  Dear Prudence ... won't you come out,
to play?"

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Drew Adams]

> have said that I'll be that it's what most users who
                        ^
                        t

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

> If someone does want to use the mouse for selecting
> and deleting text, Emacs's mouse-3 behavior is great.

Not great, but bizarre.  No other app uses such method
because of its limitations: while the currently default `mouse-3'
only kills selected text, using the context menu from `mouse-3'
allows to select more operations on the region -
kill the region without adding it to the kill ring,
kill the region with adding it to the kill ring,
add the region to the kill ring without killing it,
replace the region by pasting other text, etc.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Drew Adams]

> > If someone does want to use the mouse for selecting
> > and deleting text, Emacs's mouse-3 behavior is great.
> 
> Not great, but bizarre.  No other app uses such method
> because of its limitations: 

What you call a limitation is a strength.

> while the currently default `mouse-3'
> only kills selected text, using the context menu from
> `mouse-3' allows to select more operations on the region -

That argument applies to almost any key.  We could
replace any number of "limited" key bindings by a
menu popup that makes you choose an action.

That argument doesn't hold water.  At best, you
can reduce it to the argument that if Emacs
imposed a popup menu then it would be like other
apps, which users might be used to.  That abstract
argument has never been enough, _on its own_, to
redefine Emacs behavior.

Better to give Emacs users themselves that choice.
Even better to give them that choice on the fly,
not just with a user option, by combining Emacs's
great `mouse-3' behavior with the possibility of
popping up a context menu.

> kill the region without adding it to the kill ring,
> kill the region with adding it to the kill ring,
> add the region to the kill ring without killing it,
> replace the region by pasting other text, etc.

You can put anything on a mouse-3 menu, of course.
And with the region active it makes sense for such a
menu to be region-specific or at least include actions
specific to the region.  (Which is what `mouse3.el'
does, of course.) 

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Jim Porter]

On 12/14/2021 12:30 AM, Juri Linkov wrote:
> Thanks.  The current question was raised by Drew who worried that
> sometimes authors might want to leave two adjacent separators
> for an empty submenu.  But you answered this question that it can be
> addressed by using a special form of a separator with a menu-item.
> So I see no more problems here.

For simplicity, I'll focus on *just* the issue described in the original 
post to this bug (i.e. improving how we detect consecutive separators so 
we can de-duplicate them more reliably). Then I'll send a message to 
emacs-devel to gauge interest in supporting de-duplication in other menus.

If there's interest, it would probably make sense to have the 
de-duplication be opt-in so that we avoid any surprising behavior in 
other menus. However, if people don't think de-duplication of separators 
would be useful in other menus, then we can just keep the logic we have 
now (along with the bug fix mentioned above).

Does that sound reasonable?

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

>> Thanks.  The current question was raised by Drew who worried that
>> sometimes authors might want to leave two adjacent separators
>> for an empty submenu.  But you answered this question that it can be
>> addressed by using a special form of a separator with a menu-item.
>> So I see no more problems here.
>
> For simplicity, I'll focus on *just* the issue described in the original
> post to this bug (i.e. improving how we detect consecutive separators so we
> can de-duplicate them more reliably). Then I'll send a message to
> emacs-devel to gauge interest in supporting de-duplication in other menus.
>
> If there's interest, it would probably make sense to have the
> de-duplication be opt-in so that we avoid any surprising behavior in other
> menus. However, if people don't think de-duplication of separators would be
> useful in other menus, then we can just keep the logic we have now (along
> with the bug fix mentioned above).
>
> Does that sound reasonable?

I'm not sure if de-duplication is needed for other menus.  Currently
de-duplication is used only to simplify creation of context menus.
But there is no indication that someone needed this for other menus.
Otherwise, they would report duplicate separators in other menus as a bug.

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Jim Porter]

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

On 12/15/2021 12:57 AM, Juri Linkov wrote:
> I'm not sure if de-duplication is needed for other menus.  Currently
> de-duplication is used only to simplify creation of context menus.
> But there is no indication that someone needed this for other menus.
> Otherwise, they would report duplicate separators in other menus as a bug.

Ok, we can worry about that another time.

I've attached an updated patch that lets context-menu-functions add 
items to the beginning of the keymap as they currently do, while still 
removing consecutive separators correctly. Since this logic is a bit 
tricky, it could probably use an automated test or two, but before I 
write some, I wanted to check that the strategy I'm using seems 
reasonable. It's probably easiest to explain the logic by just pointing 
to the patch; I added several comments describing the behavior so that 
reviewers (and future readers) should be able to make sense of it.

[-- Attachment #2: 0001-Prevent-further-cases-of-duplicated-separators-in-co.patch --]
[-- Type: text/plain, Size: 2676 bytes --]

From 0310f926b62fd74932e3766c03bd7a39974c457b Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 31 Dec 2021 23:04:57 -0800
Subject: [PATCH] Prevent further cases of duplicated separators in context
 menus

In some cases, context menu items are added before the overall prompt
string.  This could cause multiple consecutive separators to appear if
they "surround" the prompt string.

* lisp/mouse.el (context-menu-map): Improve the de-duplication logic
to ignore non-menu-items when checking for consecutive separators.
---
 lisp/mouse.el | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 11fdd3f639..41cfff8d82 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -329,20 +329,27 @@ context-menu-map
 
     ;; Remove duplicate separators as well as ones at the beginning or
     ;; end of the menu.
-    (let ((l menu) saw-first-item)
+    (let ((l menu) (remove-next-separator t))
       (while (and (consp l)
                   (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))
+        (if (equal (cdr-safe (cadr l)) menu-bar-separator)
+            (progn
+              ;; The next item is a separator.  Remove it if requested
+              ;; by remove-next-separator or if it's the last item in
+              ;; the list.
+              (if (or remove-next-separator
+                      (null (caddr l)))
+                  (setcdr l (cddr l))
+                (setq l (cdr l)))
+              ;; We just saw a separator.  Remove any immediately
+              ;; following this.
+              (setq remove-next-separator t))
+          ;; If the next item is a cons cell, we found a non-separator
+          ;; item.  Don't remove the next separator we see.  We
+          ;; specifically check for cons cells to avoid treating the
+          ;; overall prompt string as a menu item.
+          (when (consp (cadr l))
+            (setq remove-next-separator nil))
           (setq l (cdr l)))))
 
     (when (functionp context-menu-filter-function)
-- 
2.25.1

bug#52293: [External] : bug#52293: 29.0.50; [PATCH v3] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

> I've attached an updated patch that lets context-menu-functions add items
> to the beginning of the keymap as they currently do, while still removing
> consecutive separators correctly. Since this logic is a bit tricky, it
> could probably use an automated test or two, but before I write some,
> I wanted to check that the strategy I'm using seems reasonable. It's
> probably easiest to explain the logic by just pointing to the patch;
> I added several comments describing the behavior so that reviewers (and
> future readers) should be able to make sense of it.

Thanks, I suppose it's for master, not for the release branch?

bug#52293: 29.0.50; [PATCH v4] Prevent further cases of duplicated separators in context menus

[Jim Porter]

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

On 1/2/2022 11:27 AM, Juri Linkov wrote:
>> I've attached an updated patch that lets context-menu-functions add items
>> to the beginning of the keymap as they currently do, while still removing
>> consecutive separators correctly. Since this logic is a bit tricky, it
>> could probably use an automated test or two, but before I write some,
>> I wanted to check that the strategy I'm using seems reasonable. It's
>> probably easiest to explain the logic by just pointing to the patch;
>> I added several comments describing the behavior so that reviewers (and
>> future readers) should be able to make sense of it.
> 
> Thanks, I suppose it's for master, not for the release branch?

Yeah, it's based on top of my previous patches that only landed on 
master, so the same applies for this one. I don't personally have an 
issue with if it merged to the release branch, but I also understand 
that we can't keep adding things to Emacs 28 forever.

Attached is an updated patch with unit tests as well as a fix to the 
behavior from the previous version; in my last patch, it didn't delete 
the last separator in the menu if it was *before* the "Context Menu" 
overall prompt string. (This could happen if all the 
context-menu-functions *only* used `define-key'.)

I've fixed that, though it did make the function a bit more complex. 
I've compensated for that with some more comments and what I hope are 
pretty thorough tests to make sure everything works as expected.

[-- Attachment #2: 0001-Prevent-further-cases-of-duplicated-separators-in-co.patch --]
[-- Type: text/plain, Size: 11505 bytes --]

From dc2b04d5f20ef861e13b0820a4377951cb528b53 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 2 Jan 2022 22:08:52 -0800
Subject: [PATCH] Prevent further cases of duplicated separators in context
 menus

In some cases, context menu items are added before the overall prompt
string.  This could cause multiple consecutive separators to appear if
they "surround" the prompt string.

* lisp/mouse.el (context-menu-map): Improve the de-duplication logic
to ignore non-menu-items when checking for consecutive separators.

* test/lisp/mouse-tests.el
(context-menu-map-remove-consecutive-separators)
(context-menu-map-remove-consecutive-separators): New tests.
---
 lisp/mouse.el            |  34 ++++----
 test/lisp/mouse-tests.el | 162 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 183 insertions(+), 13 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 11fdd3f639..50fb1137ec 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -329,21 +329,29 @@ context-menu-map
 
     ;; Remove duplicate separators as well as ones at the beginning or
     ;; end of the menu.
-    (let ((l menu) saw-first-item)
+    (let ((l menu) (last-saw-separator t))
       (while (and (consp l)
                   (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)))))
+        (if (equal (cdr-safe (cadr l)) menu-bar-separator)
+            (progn
+              ;; The next item is a separator.  Remove it if the last
+              ;; item we saw was a separator too.
+              (if last-saw-separator
+                  (setcdr l (cddr l))
+                ;; If we didn't delete this separator, update the last
+                ;; separator we saw to this one.
+                (setq last-saw-separator l
+                      l (cdr l))))
+          ;; If the next item is a cons cell, we found a non-separator
+          ;; item.  Don't remove the next separator we see.  We
+          ;; specifically check for cons cells to avoid treating the
+          ;; overall prompt string as a menu item.
+          (when (consp (cadr l))
+            (setq last-saw-separator nil))
+          (setq l (cdr l))))
+      ;; If the last item we saw was a separator, remove it.
+      (when (consp last-saw-separator)
+        (setcdr last-saw-separator (cddr last-saw-separator))))
 
     (when (functionp context-menu-filter-function)
       (setq menu (funcall context-menu-filter-function menu click)))
diff --git a/test/lisp/mouse-tests.el b/test/lisp/mouse-tests.el
index 56411d0365..96206cd9f8 100644
--- a/test/lisp/mouse-tests.el
+++ b/test/lisp/mouse-tests.el
@@ -52,5 +52,167 @@ bug26816-mouse-frame-movement
     (should (equal (mouse-position)
                    (cons frame (cons 0 0))))))
 
+(ert-deftest context-menu-map-remove-consecutive-separators ()
+  "Check that `context-menu-map' removes consecutive separators."
+  ;; Both separators after the overall prompt string.
+  (let ((context-menu-functions
+         '((lambda (menu _click)
+             (define-key-after menu [foo-item] '(menu-item "Foo" identity))
+             (define-key-after menu [separator-1] menu-bar-separator)
+             (define-key-after menu [separator-2] menu-bar-separator)
+             (define-key-after menu [bar-item] '(menu-item "Bar" identity))
+             menu))))
+    (should (equal `(keymap
+                     "Context Menu"
+                     (foo-item menu-item "Foo" identity)
+                     (separator-1 . ,menu-bar-separator)
+                     (bar-item menu-item "Bar" identity))
+                   (context-menu-map))))
+  ;; Both separators before the overall prompt string.
+  (let ((context-menu-functions
+         '((lambda (menu _click)
+             (define-key menu [bar-item] '(menu-item "Bar" identity))
+             (define-key menu [separator-2] menu-bar-separator)
+             (define-key menu [separator-1] menu-bar-separator)
+             (define-key menu [foo-item] '(menu-item "Foo" identity))
+             menu))))
+    (should (equal `(keymap
+                     (foo-item menu-item "Foo" identity)
+                     (separator-1 . ,menu-bar-separator)
+                     (bar-item menu-item "Bar" identity)
+                     "Context Menu")
+                   (context-menu-map))))
+  ;; First separator before and second separator after the overall
+  ;; prompt string.
+  (let ((context-menu-functions
+         '((lambda (menu _click)
+             (define-key-after menu [separator-2] menu-bar-separator)
+             (define-key-after menu [bar-item] '(menu-item "Bar" identity))
+             (define-key menu [separator-1] menu-bar-separator)
+             (define-key menu [foo-item] '(menu-item "Foo" identity))
+             menu))))
+    (should (equal `(keymap
+                     (foo-item menu-item "Foo" identity)
+                     (separator-1 . ,menu-bar-separator)
+                     "Context Menu"
+                     (bar-item menu-item "Bar" identity))
+                   (context-menu-map))))
+  ;; Three consecutive separators.
+  (let ((context-menu-functions
+         '((lambda (menu _click)
+             (define-key-after menu [foo-item] '(menu-item "Foo" identity))
+             (define-key-after menu [separator-1] menu-bar-separator)
+             (define-key-after menu [separator-2] menu-bar-separator)
+             (define-key-after menu [separator-3] menu-bar-separator)
+             (define-key-after menu [bar-item] '(menu-item "Bar" identity))
+             menu))))
+    (should (equal `(keymap
+                     "Context Menu"
+                     (foo-item menu-item "Foo" identity)
+                     (separator-1 . ,menu-bar-separator)
+                     (bar-item menu-item "Bar" identity))
+                   (context-menu-map)))))
+
+(ert-deftest context-menu-map-remove-separators-at-beginning-or-end ()
+  "Check that `context-menu-map' removes separators at the
+beginning or end of the menu."
+  ;; Menus with only separators.
+  (let ((test-functions
+         '(;; Separator before the overall prompt string.
+           (lambda (menu _click)
+             (define-key menu [separator] menu-bar-separator)
+             menu)
+           ;; Separator after the overall prompt string.
+           (lambda (menu _click)
+             (define-key-after menu [separator] menu-bar-separator)
+             menu)
+           ;; Begin and end separators before the overall prompt string.
+           (lambda (menu _click)
+             (define-key menu [end-separator] menu-bar-separator)
+             (define-key menu [begin-separator] menu-bar-separator)
+             menu)
+           ;; Begin and end separators after the overall prompt string.
+           (lambda (menu _click)
+             (define-key-after menu [begin-separator] menu-bar-separator)
+             (define-key-after menu [end-separator] menu-bar-separator)
+             menu)
+           ;; Begin separator before and end separator after the
+           ;; overall prompt string.
+           (lambda (menu _click)
+             (define-key menu [begin-separator] menu-bar-separator)
+             (define-key-after menu [end-separator] menu-bar-separator)
+             menu))))
+    (dolist (fun test-functions)
+      (let ((context-menu-functions (list fun)))
+        (should (equal '(keymap "Context Menu")
+                       (context-menu-map))))))
+  ;; Menus with separators at beginning and/or end with a menu-item
+  ;; before the prompt string.
+  (let ((test-functions
+         '(;; Separator before the overall prompt string and the menu-item.
+           (lambda (menu _click)
+             (define-key menu [foo-item] '(menu-item "Foo" identity))
+             (define-key menu [separator] menu-bar-separator)
+             menu)
+           ;; Separator before the overall prompt string, but after
+           ;; the menu-item.
+           (lambda (menu _click)
+             (define-key menu [separator] menu-bar-separator)
+             (define-key menu [foo-item] '(menu-item "Foo" identity))
+             menu)
+           ;; Separator at the end.
+           (lambda (menu _click)
+             (define-key menu [foo-item] '(menu-item "Foo" identity))
+             (define-key-after menu [separator] menu-bar-separator)
+             menu)
+           ;; Begin separator before and end separator after the
+           ;; overall prompt string.
+           (lambda (menu _click)
+             (define-key menu [foo-item] '(menu-item "Foo" identity))
+             (define-key menu [begin-separator] menu-bar-separator)
+             (define-key-after menu [end-separator] menu-bar-separator)
+             menu))))
+    (dolist (fun test-functions)
+      (let ((context-menu-functions (list fun)))
+        (should (equal '(keymap (foo-item menu-item "Foo" identity)
+                                "Context Menu")
+                       (context-menu-map))))))
+  ;; Menus with separators at beginning and/or end with a menu-item
+  ;; after the prompt string.
+  (let ((test-functions
+         '(;; Separator before the overall prompt string.
+           (lambda (menu _click)
+             (define-key menu [separator] menu-bar-separator)
+             (define-key-after menu [foo-item] '(menu-item "Foo" identity))
+             menu)
+           ;; Separator after the overall prompt string, but before
+           ;; the menu-item.
+           (lambda (menu _click)
+             (define-key-after menu [separator] menu-bar-separator)
+             (define-key-after menu [foo-item] '(menu-item "Foo" identity))
+             menu)
+           ;; Separator at the end.
+           (lambda (menu _click)
+             (define-key-after menu [foo-item] '(menu-item "Foo" identity))
+             (define-key-after menu [separator] menu-bar-separator)
+             menu)
+           ;; Begin and end separators after the overall prompt string.
+           (lambda (menu _click)
+             (define-key-after menu [begin-separator] menu-bar-separator)
+             (define-key-after menu [foo-item] '(menu-item "Foo" identity))
+             (define-key-after menu [end-separator] menu-bar-separator)
+             menu)
+           ;; Begin separator before and end separator after the
+           ;; overall prompt string.
+           (lambda (menu _click)
+             (define-key menu [begin-separator] menu-bar-separator)
+             (define-key-after menu [foo-item] '(menu-item "Foo" identity))
+             (define-key-after menu [end-separator] menu-bar-separator)
+             menu))))
+    (dolist (fun test-functions)
+      (let ((context-menu-functions (list fun)))
+        (should (equal '(keymap "Context Menu"
+                                (foo-item menu-item "Foo" identity))
+                       (context-menu-map)))))))
 
 ;;; mouse-tests.el ends here
-- 
2.25.1

bug#52293: 29.0.50; [PATCH v4] Prevent further cases of duplicated separators in context menus

[Juri Linkov]

close 52293 29.0.50
thanks

>>> I've attached an updated patch that lets context-menu-functions add items
>>> to the beginning of the keymap as they currently do, while still removing
>>> consecutive separators correctly. Since this logic is a bit tricky, it
>>> could probably use an automated test or two, but before I write some,
>>> I wanted to check that the strategy I'm using seems reasonable. It's
>>> probably easiest to explain the logic by just pointing to the patch;
>>> I added several comments describing the behavior so that reviewers (and
>>> future readers) should be able to make sense of it.
>> Thanks, I suppose it's for master, not for the release branch?
>
> Yeah, it's based on top of my previous patches that only landed on master,
> so the same applies for this one. I don't personally have an issue with if
> it merged to the release branch, but I also understand that we can't keep
> adding things to Emacs 28 forever.
>
> Attached is an updated patch with unit tests as well as a fix to the
> behavior from the previous version; in my last patch, it didn't delete the
> last separator in the menu if it was *before* the "Context Menu" overall
> prompt string. (This could happen if all the context-menu-functions *only*
> used `define-key'.)
>
> I've fixed that, though it did make the function a bit more complex. I've
> compensated for that with some more comments and what I hope are pretty
> thorough tests to make sure everything works as expected.

Thank you for the fix and for the unit tests.  Now pushed to master.
It seems this bug report can be closed now.  If you have more patches,
it can be reopened.

bug#52293: 29.0.50; [PATCH v4] Prevent further cases of duplicated separators in context menus

[Jim Porter]

On 1/4/2022 12:19 AM, Juri Linkov wrote:
> Thank you for the fix and for the unit tests.  Now pushed to master.
> It seems this bug report can be closed now.  If you have more patches,
> it can be reopened.

Thanks for pushing the patch. Assuming I didn't introduce any new bugs, 
I don't think I'll have any more patches for this issue, so I agree that 
this report can be closed.