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

[Jim Porter <jporterbugs@gmail.com>]

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.