Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot seem to get enable-extended-define-key nor add-keymap-based-replacements to work #263

Closed
TRSx80 opened this issue Sep 2, 2020 · 7 comments

Comments

@TRSx80
Copy link

TRSx80 commented Sep 2, 2020

Hallo,

Thanks for sharing which-key, it's really helpful!

I was trying last night to do replacements on some long command names, without success.

First I tried setting which-key-enable-extended-define-key to t (via setq) but it did not seem to have any effect.

The README talks about "advising define-key" and the docstring for above setting states that "This variable must be set before loading which-key" however I apparently failed to figure out exactly how to do that. I tried toggling which-key on and off via M-x which-key and even restarting Emacs, all to no avail.

So then I tried the approach using the function which-key-add-keymap-based-replacements. I realized this is quite new, so at this point I did package-delete on the MELPA installed version, and git clone the latest (2b10b8e) and install via package-install-file. I define the following keymap:

  (which-key-add-keymap-based-replacements global-map
    (kbd "C-c z c")   '("Create new" .  ostrta-zettel-node-create)
    (kbd "C-c z e")   '("Edit" . nil)
    (kbd "C-c z e p") '("edit Properties" . ostrta-zettel-node-edit-properties)
    (kbd "C-c z e t") '("edit Type" . ostrta-zettel-node-edit-type)
    (kbd "C-c z o")   '("Open" . ostrta-zettel-node-open)
    (kbd "C-c z t")   '("Toggle" . nil)
    (kbd "C-c z t l") '("toggle Logbook" . ostrta-zettel-node-toggle-fold-logbook)
    (kbd "C-c z t p") '("toggle Properties" . ostrta-zettel-node-toggle-fold-properties)
    (kbd "C-c z u")   '("Update, save & close" . ostrta-zettel-node-save-update-close))

"Edit" and "Toggle" are strictly for menu purposes. It is unclear to me whether I also need to define them somewhere else or not. Above is my latest attempt, however previously I tried to also define those like:

'("Edit")

or like:

"Edit"

... all to no avail.

But those entries are not actually the problem anyway. It seems like the actual keys (to commands) are not getting bound. If I do C-c z for instance, which-key pops up, but all I see are "Edit" and "Toggle" but not any of the actual keymaps (to commands). Also, executing the full key sequence does not invoke the command, either.

I am not sure what else to do at this point, so I come here and make this Issue. Any help would be gratefully appreciated.

@TRSx80
Copy link
Author

TRSx80 commented Sep 2, 2020

I took a closer look at docstring for the function which-key-add-keymap-based-replacements and then I removed my kbd wrapping around key commands and also changed the key prefix to a plain string, like so:

  (which-key-add-keymap-based-replacements global-map
    "C-c z c"   '("Create new" .  ostrta-zettel-node-create)
    "C-c z e"   "Edit"
    "C-c z e p" '("edit Properties" . ostrta-zettel-node-edit-properties)
    "C-c z e t" '("edit Type" . ostrta-zettel-node-edit-type)
    "C-c z o"   '("Open" . ostrta-zettel-node-open)
    "C-c z t"   "Toggle"
    "C-c z t l" '("toggle Logbook" . ostrta-zettel-node-toggle-fold-logbook)
    "C-c z t p" '("toggle Properties" . ostrta-zettel-node-toggle-fold-properties)
    "C-c z u"   '("Update, save & close" . ostrta-zettel-node-save-update-close))

But unfortunately, still doesn't work. Even aver restarting Emacs.

I then tried with only a single entry (only "Create new" line above) and that didn't work either.

I studied the source of that function, but I think I do not understand it fully. Looks to me like we are sticking a function into the definition for the key with define-key but I do not see any plain which-key function defined in the source, and so I am guessing it must be used in some way I am not understanding yet. Maybe which-key (the package) does something when it discovers that value in a keymap, I dunno. Or perhaps simply a typo has been made? I am too unfamiliar with the code and perhaps also too low level Elisp wizard to tell which it is. 😄

(defun which-key-add-keymap-based-replacements (keymap key replacement &rest more)
  "[...docstring...]"
  (while key
    (let ((string (if (stringp replacement)
                      replacement
                    (car-safe replacement)))
          (command (cdr-safe replacement)))
      (define-key keymap (which-key--pseudo-key (kbd key))
        `(which-key ,(cons string command))))  ; <-------------------------------------- ?
    (setq key (pop more)
          replacement (pop more))))
(put 'which-key-add-keymap-based-replacements 'lisp-indent-function 'defun)

@justbur
Copy link
Owner

justbur commented Sep 4, 2020

Your usage looks correct. The only thing you said that is odd is

But those entries are not actually the problem anyway. It seems like the actual keys (to commands) are not getting bound. If I do C-c z for instance, which-key pops up, but all I see are "Edit" and "Toggle" but not any of the actual keymaps (to commands). Also, executing the full key sequence does not invoke the command, either.

which-key-add-keymap-based-replacements doesn't actually bind the commands. It adds a separate entry into the keymap so that which-key knows to replace the name of the command.

Alternatively, the extended define-key will do both (bind and enter the replacement) at the same time. For example,

(setq which-key-enable-extended-define-key t)
(require 'which-key)
(define-key global-map (kbd "C-c z c")   '("Create new" .  ostrta-zettel-node-create))
(which-key-mode)

should bind the command and enter the replacement info at the same time. This method is more elegant, but I understand if people don't feel comfortable advising key functions.

@TRSx80
Copy link
Author

TRSx80 commented Sep 4, 2020

the extended define-key will do both (bind and enter the replacement) at the same time.

Yes, I'm pretty sure this is what I'm looking for. Thanks so much!

This method is more elegant, but I understand if people don't feel comfortable advising key functions.

(emphasis mine)
You also allude to this in the docs/README. I don't understand why though?

@justbur
Copy link
Owner

justbur commented Sep 4, 2020

You also allude to this in the docs/README. I don't understand why though?

I think out of fear that the advice breaks a critical function (not that mine does)

@TRSx80
Copy link
Author

TRSx80 commented Sep 4, 2020

Oh yes, now I recall vaguely reading at some point some people saying not to use advice.

I think everything is clear to me now. However it was not at first.

As someone coming into the project new and unfamiliar, the following sentence (from README):

If you do not want to enable the advise on define-key, you may also use which-key-add-keymap-based-replacements. The above examples can be alternatively written as

makes it sound like you can avoid the (perceived?) problems of advice, while still gaining the performance benefits of “keymap-based” replacement. It was not clear to me (initially) that I would also need to bind the keys separately.

Although now that I think about it, it makes perfect sense, as which-key really is just popping up info about already existing keybindings. Well, "20/20 hindsight" as they say...

However, in order to make things more clear, perhaps some slight adjustments to the wording of the README are in order?

@justbur
Copy link
Owner

justbur commented Sep 4, 2020 via email

@TRSx80
Copy link
Author

TRSx80 commented Sep 4, 2020

Thanks a lot for the help, justbur! And thanks for picking up the torch on which-key! It's a super helpful package.

Enjoy your holiday weekend, mate! Cheers! 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants