bug#52286: 28.0.90; [PATCH] Be consistent in naming of separators in context menu

[Jim Porter <jporterbugs@gmail.com>]

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

On 12/6/2021 4:55 AM, Eli Zaretskii wrote:
> I think the most natural place is where we describe how to construct
> menus.

Thanks.

>> We can detect whether an item is a separator pretty easily without this
>> change. However, context menu functions use specific separators to
>> determine where exactly it should put new menu items. For example,
>> `elisp-context-menu' adds menu items after `middle-separator':
>>
>>       (define-key-after menu [elisp-separator] menu-bar-separator
>>         'middle-separator)
>>
>> (Later in the function, it adds the actual menu items after the
>> newly-added `elisp-separator'.)
> 
> If this is a private convention of elisp-mode, we don't have to codify
> it.

It's not just for `elisp-mode'; any mode (or other third-party code) may 
want to insert context menu items in a certain spot. `middle-separator' 
follows the naming convention I recommend, but a hypothetical mode might 
want to insert a new item just after the separator for the Undo section. 
However, that's currently named `separator-undo' on master, so it can be 
confusing to remember the difference in naming between these two cases:

   (define-key-after menu [my-separator] menu-bar-separator
     'middle-separator)

   (define-key-after menu [my-separator] menu-bar-separator
     'separator-undo)

> So maybe instead of conventions we should have recommendations, with
> explanation why following that could be useful in some situation.
> Then let Lisp programmers decide what they want.

Ok, I've tried to provide a brief explanation of the recommendation and 
the reasoning for it without *over*-explaining it. I also added a small 
explanation about how to use `menu-bar-separator', since I didn't see 
documentation on that in the manual, and it helped segue into an 
explanation about the naming recommendation.

[-- Attachment #2: 0001-Rename-context-menu-separators-to-FOO-separator-for-.patch --]
[-- Type: text/plain, Size: 5135 bytes --]

From 7b646daeb0a8c9838d92eb9c093ac203bd9e1f03 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 11 Dec 2021 12:44:12 -0800
Subject: [PATCH] Rename context menu separators to 'FOO-separator' for
 consistency

* lisp/mouse.el (context-menu-toolbar, context-menu-global)
(context-menu-local, context-menu-minor, context-menu-buffers)
(context-menu-vc, context-menu-undo, context-menu-region): Rename
separators to 'FOO-separator'.
* doc/lispref/keymaps.texi (Menu Separators): Add documentation
explaining 'menu-bar-separator' and naming convention of separators.
---
 doc/lispref/keymaps.texi | 15 ++++++++++++++-
 lisp/mouse.el            | 16 ++++++++--------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/doc/lispref/keymaps.texi b/doc/lispref/keymaps.texi
index edf1d6e83f..4fb66cecde 100644
--- a/doc/lispref/keymaps.texi
+++ b/doc/lispref/keymaps.texi
@@ -2460,7 +2460,20 @@ Menu Separators
 
   A menu separator is a kind of menu item that doesn't display any
 text---instead, it divides the menu into subparts with a horizontal line.
-A separator looks like this in the menu keymap:
+To create a basic separator, you can use @code{menu-bar-separator} when
+defining the item:
+
+@example
+(define-key map [@var{foo-separator}] menu-bar-separator)
+@end example
+
+@noindent
+When possible, it's recommended to name the key for the separator with
+the form @var{foo-separator} so that it's clear the underlying object
+is a separator.  For example, @code{region-separator} is preferred
+over @code{separator-region}.
+
+  A separator looks like this in the menu keymap:
 
 @example
 (menu-item @var{separator-type})
diff --git a/lisp/mouse.el b/lisp/mouse.el
index 11fdd3f639..96205fa346 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -358,7 +358,7 @@ context-menu-middle-separator
 (defun context-menu-toolbar (menu _click)
   "Populate MENU with submenus from the tool bar."
   (run-hooks 'activate-menubar-hook 'menu-bar-update-hook)
-  (define-key-after menu [separator-toolbar] menu-bar-separator)
+  (define-key-after menu [toolbar-separator] menu-bar-separator)
   (map-keymap (lambda (key binding)
                 (when (consp binding)
                   (define-key-after menu (vector key)
@@ -369,7 +369,7 @@ context-menu-toolbar
 (defun context-menu-global (menu _click)
   "Populate MENU with submenus from the global menu."
   (run-hooks 'activate-menubar-hook 'menu-bar-update-hook)
-  (define-key-after menu [separator-global] menu-bar-separator)
+  (define-key-after menu [global-separator] menu-bar-separator)
   (map-keymap (lambda (key binding)
                 (when (consp binding)
                   (define-key-after menu (vector key)
@@ -380,7 +380,7 @@ 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)
+  (define-key-after menu [local-separator] menu-bar-separator)
   (let ((keymap (local-key-binding [menu-bar])))
     (when keymap
       (map-keymap (lambda (key binding)
@@ -393,7 +393,7 @@ context-menu-local
 (defun context-menu-minor (menu _click)
   "Populate MENU with submenus provided by minor modes."
   (run-hooks 'activate-menubar-hook 'menu-bar-update-hook)
-  (define-key-after menu [separator-minor] menu-bar-separator)
+  (define-key-after menu [minor-separator] menu-bar-separator)
   (dolist (mode (reverse (minor-mode-key-binding [menu-bar])))
     (when (and (consp mode) (symbol-value (car mode)))
       (map-keymap (lambda (key binding)
@@ -406,7 +406,7 @@ context-menu-minor
 (defun context-menu-buffers (menu _click)
   "Populate MENU with the buffer submenus to buffer switching."
   (run-hooks 'activate-menubar-hook 'menu-bar-update-hook)
-  (define-key-after menu [separator-buffers] menu-bar-separator)
+  (define-key-after menu [buffers-separator] menu-bar-separator)
   (map-keymap (lambda (key binding)
                 (when (consp binding)
                   (define-key-after menu (vector key)
@@ -416,13 +416,13 @@ context-menu-buffers
 
 (defun context-menu-vc (menu _click)
   "Populate MENU with Version Control commands."
-  (define-key-after menu [separator-vc] menu-bar-separator)
+  (define-key-after menu [vc-separator] menu-bar-separator)
   (define-key-after menu [vc-menu] vc-menu-entry)
   menu)
 
 (defun context-menu-undo (menu _click)
   "Populate MENU with undo commands."
-  (define-key-after menu [separator-undo] menu-bar-separator)
+  (define-key-after menu [undo-separator] menu-bar-separator)
   (when (and (not buffer-read-only)
              (not (eq t buffer-undo-list))
              (if (eq last-command 'undo)
@@ -440,7 +440,7 @@ context-menu-undo
 
 (defun context-menu-region (menu click)
   "Populate MENU with region commands."
-  (define-key-after menu [separator-region] menu-bar-separator)
+  (define-key-after menu [region-separator] menu-bar-separator)
   (when (and mark-active (not buffer-read-only))
     (define-key-after menu [cut]
       '(menu-item "Cut" kill-region
-- 
2.25.1