justbur / emacs-which-key

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

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

Closed TRSx80 closed 4 years ago

TRSx80 commented 4 years ago

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 commented 4 years ago

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. :smile:

(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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Sure I can do that. I just write those things quickly so I’m not surprised they’re not totally clear.

On Fri, Sep 4, 2020 at 2:35 PM TRS-80 notifications@github.com wrote:

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?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/justbur/emacs-which-key/issues/263#issuecomment-687314250, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQ3E7RZUYK3NGBSC2IUKB3SEEXNVANCNFSM4QTM73FA .

TRSx80 commented 4 years ago

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! :beers: