emacs-which-key icon indicating copy to clipboard operation
emacs-which-key copied to clipboard

Speed up replacements

Open justbur opened this issue 5 years ago • 5 comments

cc @rgrinberg ref #226

After thinking about this problem and different approaches, the simplest one seems to be to push towards defining replacements using the extended define-key mechanism, which puts the replacement information directly in the relevant keymap. See which-key--process-define-key-args (source) and which-key--get-pseudo-binding (source) for a sense of how this works.

We could also add a function that would allow the easy addition of "keymap-based bindings" if someone doesn't want to use the define-key advice.

I would then discourage the heavy use of which-key-replacement-alist and friends.

There are, of course, other approaches (adding specialized hash tables for instance), but they add complication and I'm skeptical of a performance improvement over using emacs internal keymap lookup functions (could be wrong here).

Happy to hear other thoughts

justbur avatar Aug 22 '20 12:08 justbur

I am not sure, i understand the implications of the extended define-key mechanism (or its current implementation), but using emacs' internal mechanisms as much as possible seems reasonable to me.

I think, a specialized function (or functions) to use the mechanism without the advice would help to "lower the bar" on using the feature, because it would be more transparent, to the user (and "spacemacs developer" ;-) Also support for the evil-way of binding keys would be good.

Here is an example of how bindings are done in spacemacs (i dont know if this is exhaustive): https://github.com/syl20bnr/spacemacs/blob/master/layers/%2Bdistributions/spacemacs-base/keybindings.el

they seem to use their own wrapper-functions for binding keys, so maybe the advice-solution would be better after all?! https://github.com/syl20bnr/spacemacs/blob/master/core/core-keybindings.el

bitclick avatar Aug 24 '20 21:08 bitclick

Added a new function called which-key-add-keymap-based-replacements which like the define-key mechanism stores the information in the keymap itself, and allows one to avoid making which-key-replacement-alist very long.

FWIW, I think this is the "right way" to specify most replacements, and should be the primary way to do it going forward. It doesn't allow for the fancy regexp stuff that which-key-replacement-alist does, but hopefully the need for that is not great.

justbur avatar Aug 28 '20 14:08 justbur

Thanks! Lets hope this will be adopted by spacemacs and doom etc..

bitclick avatar Sep 07 '20 10:09 bitclick

Sorry for the late turn around. Just had a look and it looks quite good.

There are, of course, other approaches (adding specialized hash tables for instance), but they add complication and I'm skeptical of a performance improvement over using emacs internal keymap lookup functions (could be wrong here).

Seems like the performance of this is going to be quite good. I agree more complicated mechanisms aren't necessary.

Added a new function called which-key-add-keymap-based-replacements which like the define-key mechanism stores the information in the keymap itself, and allows one to avoid making which-key-replacement-alist very long.

This sounds much more reasonable advising define-key. I would suggest to scrub any mentions of advising define-key for the sake of the users :)

It would also be nice to expand the test suite to cover this new functionality. I more or less understand how things work from reading the code and experimenting, but some test cases would help have helped me out.

rgrinberg avatar Oct 16 '20 02:10 rgrinberg

Thanks @rgrinberg. I modified the README and added a couple of tests

justbur avatar Oct 19 '20 15:10 justbur