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

[Jim Porter <jporterbugs@gmail.com>]

[-- 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]