Menu items

[loafofpiecrust]

I have been hacking on keymaps for the past week. I was happy to see the addition of which-key-add-keymap-based-replacements for command descriptions, but I was bothered by the fact that the implementation needed to make extra bindings when we can use the built-in menu-item syntax for define-key. You support this with which-key-enable-extended-define-key, but I don't think advice is necessary, as noted in #261

This PR adds support for built-in menu item entries in keymaps. Instead of using describe-buffer-bindings for the active bindings, we can grab them using current-active-maps and filtering by current prefix. Then, the full binding is given, letting us support the full extent of menu items. That means we no longer need pseudo-bindings because we can use the command replacement embedded in the original keymap. In addition, we can fully support both single command menu items and sub-menus.

These changes also reduce the total code footprint by making use of which-key--get-keymap-bindings for all which-key views, not just ones specific to one keymap. To add descriptions, we no longer need which-key-enable-extended-define-key so I removed it and the associated advice.

Please let me know what you think. I think the design is sound, and I hope my implementation is too. I have used it for the past day or so and have not run into many issues. It also doesn't seem to incur any more overhead; when I profile, the slowest part of which-key init by far is the regex replacements.

Logistical note: I haven't done the FSF copyright assignment yet so I'll get on that if this PR is appealing to merge.

[justbur]

Thank you. I looked quickly. My two main concerns with parsing keymaps directly are

• Do you handle the precedence rules correctly when multiple active maps define the same binding?

• What are the performance implications?

Also I think the motivation is not quite clear to me. Pseudo keys are an alternative method for defining strings for which-key to use. People have been using menu-items as a hack to make dynamic bindings, which is not the same.

[loafofpiecrust]

• Precedence: I think that I am handling that right. Keymap precedence is defined in the Emacs docs like so Usually, the active keymaps are: (i) the keymap specified by the keymap property, (ii) the keymaps of enabled minor modes, (iii) the current buffer’s local keymap, and (iv) the global keymap, in that order. Emacs searches for each input key sequence in all these keymaps.. Testing (current-active-maps t) seems to return in that order as well and we're pulling the first instance of each key binding from there. I will have to do more testing to make sure that it covers multiple definitions of a prefix key and edge cases, but I give a provisional yes.
• Performance: Here's a CPU trace for my leader key with regex replacements turned on

  388  77% - timer-event-handler
  381  76%  - apply
  326  65%   - which-key--update
  326  65%    - which-key--create-buffer-and-show
  295  58%     - which-key--get-bindings
  280  55%      - which-key--format-and-replace
  150  29%       + which-key--maybe-replace
  95  18%       + which-key--propertize-description
  26   5%       + which-key--maybe-add-docstring
  7   1%       + kbd
  1   0%         which-key--extract-key
  10   1%      - which-key--get-current-bindings
  8   1%       - mapcan
  8   1%        - #<compiled 0x19f099e583673454>
  8   1%         - which-key--get-keymap-bindings
  8   1%          - let
  8   1%           + map-keymap
  1   0%       + cl-remove-if-not
  1   0%       + cl-remove-duplicates
  4   0%      + sort
  30   5%     + which-key--create-pages

And a CPU trace for my leader key with regex replacements turned off (since they're the biggest performance hit):

717  85% - timer-event-handler
714  85%  - apply
551  66%   - which-key--update
550  65%    - which-key--create-buffer-and-show
388  46%     - which-key--get-bindings
359  43%      + which-key--format-and-replace
 21   2%      - which-key--get-current-bindings
 16   1%       - mapcan
 16   1%        - #<compiled 0x19f099e583673454>
 16   1%         + which-key--get-keymap-bindings
  5   0%       + cl-remove-duplicates
  8   0%      + sort
162  19%     + which-key--create-pages
  1   0%      which-key--this-command-keys

I don't notice extra delays and from these CPU traces, it looks like which-key--get-current-bindings likely isn't causing slowdowns.

• Motivation: using the built-in menu items mechanism means that the which-key buffer could theoretically/eventually align with the menu-bar for a particular mode. This is appealing to me for better UX and because it means that when I look at my keymaps in help buffers they aren't littered with <which-key> bindings. Based on just reading the docs for menu items putting a string name in there is not a hack at all. The conversation here even demonstrates a way in which you can get a super basic binding hint with stock Emacs by giving the keymap a string name and simply doing (define-key map "f" '("Find file!" . find-file)). With this PR, which-key would be compatible with that methodology, though not yet as snappy!

[minad]

@loafofpiecrust @justbur I would very much love to see menu-item support in which-key! See also the feature request https://github.com/justbur/emacs-which-key/issues/177. This would be immediately useful for Consult and Embark which use keymap based menus in the minibuffer. If there is menu-item support these nice minibuffer action menus would just work out of the box. And I suspect there are many more use cases for this, as @loafofpiecrust also demonstrates in his prototype, see https://www.reddit.com/r/emacs/comments/kxd9z4/grouped_bindings_in_whichkey/.

[loafofpiecrust]

Be aware that I have not implemented support for any menu item properties here, like the conditional items in the issue @minad references, just the description field from simple and expanded menu items. I figured that's a good place to start and supporting any other properties can be evaluated later on.

[minad]

@loafofpiecrust For the use cases I have in mind, the simple menu items (string . def) would be totally sufficient!

[justbur]

@loafofpiecrust ok, I've come around to the idea, and I'm open to it. If you're willing to support issues that might arise, I'm willing to merge it after going through the code.

The idea behind the extended key definition stuff was to create a temporary workaround for emacs not having a simple way to expose the (string . def) bindings through something akin to describe-buffer-bindings. I actually wrote a patch at one point for emacs to do this, and the plan was to remove the advice when that patch became available. The process of getting that patch accepted stalled though, and I got busy and lost track of it.

I do think having which-key just read the keymaps and know about (string . def) as well as menu-items is attractive. However, I was hesitant about the two issues I mentioned to you and didn't have the time to really investigate myself.

On the performance side, my understanding is that the profiler is not very good at measuring performance (it's also hard to interpret what you're showing, since you're only showing the calls that come out of timer-event-handler). If which-key starts to feel sluggish I think that would ruin the experience. That doesn't mean, I know there will be performance issues. It's just that as you're aware this code is run very often when which-key is enabled.

@loafofpiecrust For the use cases I have in mind, the simple menu items (string . def) would be totally sufficient!

@minad That's exactly the point of the keymap-based replacement, which is already available (see here)

[minad]

@justbur

That's exactly the point of the keymap-based replacement, which is already available (see here)

Yes, but I would just like to manipulate keymaps directly instead of bothering with the which-key pseudo keys. I would very much prefer to use the Emacs standard (string . def) menu items instead!

[loafofpiecrust]

@minad Thanks for the further explanation! A patch to describe-buffer-bindings might have been the way to go, but at least this method doesn't involve parsing the raw string. If there's a performance or other issue that can't be solved outside of core, then I might be inclined to look at current-active-maps for a patch or exposing the logic behind describe-buffer-bindings as another function.

I have been testing this branch with which-key-idle-delay down to 0.1 and don't notice an added delay, nor do I with secondary maps (pressing SPC, waiting a second, pressing p). I just pushed a commit optimizing it a bit, but the larger slowdowns that I have noticed in which-key are regex replacements (in DOOM there's a big fat list of them) and (documentation xxx) with key substitutes which I disabled to get an idea of this branch's performance impact alone. Also, in those traces I showed the only parts that included which-key functions, the other stuff was irrelevant here.

Lastly, I am definitely willing to support with issues that come up! The only glaring one that I've noticed so far is <override-state> -> all from evil shows up. I need to check for that and make it ignore following keymaps if one has <override-state> (I think that's how that is supposed to work!).

[loafofpiecrust]

I now have the FSF copyright assignment! I will add one or two more commits to address some single keymaps not displaying right, but this PR is generally ready for code review.

[minad]

@justbur @loafofpiecrust Is anything missing here? From my perspective this sounds like a great addition. And after looking at the code, it also seems like a solid approach, in particular since it gets rid of the describe-buffer-bindings parsing. Removing the advice on define-key is also a good idea!

[a13]

any news here?

[bdarcus]

any news here?

@loafofpiecrust - any update? Would be great to get this merged.

[justbur]

See #308. I decided to do it myself. If you are interested in this feature. Please test that PR and report there whether you run into bugs.

[justbur]

I merged support for menu-items

[loafofpiecrust]

Thank you for carrying this change home @justbur, stuff with my job and living situation got really hectic this past few months.

[justbur]

@loafofpiecrust no problem. Thanks for your work on this