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

Some keymap names no longer show up #314

Closed peniblec closed 3 years ago

peniblec commented 3 years ago

Hello!

which-key recently lost the ability to display a keymap's name, when defined like so:

;; repro.el

(defvar my/buffer-map
  (let ((map (define-prefix-command 'my/buffer-map)))
    (define-key map "b" 'bury-buffer)
    (define-key map "g" 'revert-buffer)
    (define-key map "r" 'rename-buffer)
    map)
  "Keymap for buffer manipulation commands.")

(global-set-key (kbd "C-c b") 'my/buffer-map)

With Emacs 27.2 and 28.0.50, and…

(Tested with git checkout $commit ; $EMACS -Q -l which-key.el -eval '(which-key-mode)' -l repro.el)

I haven't been able to pinpoint where exactly on the alt-get-bindings branch the problem appeared.

FWIW, it is entirely possible that the way my keymap is defined is "wrong" or convoluted somehow. It is the DRYest way I found to define a prefix command which (1) has a docstring that shows up in M-x describe-keymap (2) has a name that shows up in C-h b (3) has a name that shows up in which-key (up to e236920); I admit that my understanding of keymaps is far from complete, however, so the problem might not lie in which-key.

At any rate, thanks for this great package.

peniblec commented 3 years ago

FWIW, this trivial patch fixes the issue AFAICT:

diff --git a/which-key.el b/which-key.el
index 0fdcb7a..9a334cc 100644
--- a/which-key.el
+++ b/which-key.el
@@ -1743,8 +1743,8 @@ Requires `which-key-compute-remaps' to be non-nil"
                     (binding
                      (cons key-desc
                            (cond
-                            ((keymapp def) "prefix")
                             ((symbolp def) (which-key--compute-binding def))
+                            ((keymapp def) "prefix")
                             ((eq 'lambda (car-safe def)) "lambda")
                             ((eq 'closure (car-safe def)) "closure")
                             ((stringp def) def)

I'll see if I can add a test case, and put all this together in a PR. Hopefully that's the Right Thing To Do?