search

justbur / emacs-which-key

Emacs package that displays available keybindings in popup
GNU General Public License v3.0
1.74k stars 87 forks source link

Speed up replacements #261

Open justbur opened 4 years ago

justbur commented 4 years ago

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

bitclick commented 4 years ago

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

justbur commented 4 years ago

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.

bitclick commented 4 years ago

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

rgrinberg commented 4 years ago

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.

justbur commented 4 years ago

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