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

Display/specify which-key for general-predicate-dispatch #79

Open
terlar opened this issue Nov 2, 2017 · 9 comments
Open

Display/specify which-key for general-predicate-dispatch #79

terlar opened this issue Nov 2, 2017 · 9 comments

Comments

@terlar
Copy link

terlar commented Nov 2, 2017

Not sure if there is any way around this, but when I define a keybinding with general-predicate-dispatch I don't see that key appearing at all with which-key. Otherwise I really like how this one is working, my usage currently:

(general-define-key
 :prefix "C-c"
 "SPC" (general-predicate-dispatch #'projectile-switch-project
         :docstring "Find file in project or switch project"
         (projectile-project-p) #'projectile-find-file))
@noctuid
Copy link
Owner

noctuid commented Nov 2, 2017

Unlike general-key-dispatch, for example, general-predicate-dispatch does not return a function but a menu item. If you eval (key-binding (kbd "C-c SPC")), you will actually get either projectile-switch-project or projectile-find-file depending on (projectile-project-p). I'm guessing that which-key doesn't support displaying menu items, so I'd suggest making an issue there.

@terlar
Copy link
Author

terlar commented Nov 2, 2017

Thanks for the info, I really like this feature especially since some plugins rely on the actual binding being to projectile-find-file and replaces that binding with it's own, which works with general-predicate-dispatch but doesn't seem to work with the other approaches.

@terlar
Copy link
Author

terlar commented Nov 2, 2017

I am not familiar with the menu items, but found this related closed issue inside the which-key project: justbur/emacs-which-key#147

Perhaps that tells you something about the state? Or is it not really related?

@noctuid
Copy link
Owner

noctuid commented Nov 2, 2017

It's not really related. #39 spawned that issue. The idea of that pull request was to use the menu-item ITEM-NAME to clue which-key in about what to display as a replacement (and was rejected). In that case the only reason menu-item was used was to give which-key a replacement. In this case, menu-item is used for a predicate. I'd make a new which-key issue. It seems you can't even manually add a replacement for keys bound to a menu-item.

@justbur
Copy link
Contributor

justbur commented Nov 6, 2017

@terlar and @noctuid. which-key can easily support this if (describe-buffer-bindings (current-buffer) KEY t) showed menu-items but it doesn't (at least how they are used here). That last argument suggests that menu-items should be reported but they are not for me.

It seems to me that this is something that could be fixed in describe-buffer-bindings.

For reasons that I outlined in the other attempts to change the way which-key found bindings, I'm not interested in writing a custom describe-buffer-bindings function. It's too error prone. And this usage is too narrow in my opinion.

I provided another way to conditionally display a certain string for a bindings here, which is an alternative route.

@justbur
Copy link
Contributor

justbur commented Dec 13, 2017

@noctuid This commit (justbur/emacs-which-key@f516b84) takes a partial step in that direction. If it works well, I can extend it. The idea is to pre-process arguments to define-key instead of taking on the more complicated task of parsing keymaps on the fly without built-in functions.

@noctuid
Copy link
Owner

noctuid commented Jan 20, 2018

It would be nice if it was possible to dynamically retrieve the description from a (STRING . DEFN) keybinding, but this seems like the best alternative. I do think it would be nice to extend this to work for extended menu items (e.g. use the ITEM-NAME as the description).

@akirak
Copy link

akirak commented Dec 28, 2019

I was able to display keybindings to general-predicate-dispatch actions with the following workaround:

  1. Specify :docstring in general-predicate-dispatch forms.
  2. Override the definition of which-key--get-current-bindings with the following implementation (diff):
(defun which-key--get-current-bindings (&optional prefix)
  "Generate a list of current active bindings."
  (let* ((ignore-bindings '(self-insert-command
                            ignore
                            ignore-event
                            company-ignore))
         (buffer (current-buffer))
         (active-maps (with-current-buffer buffer
                        (current-active-maps))))
    (cl-loop for entry in (cl-remove-duplicates
                           (thread-last
                               (if prefix
                                   (mapcar (lambda (map) (lookup-key map prefix))
                                           active-maps)
                                 active-maps)
                             (cl-remove-if-not #'keymapp)
                             (mapcar (lambda (map)
                                       (when (eq map 'mode-specific-command-prefix)
                                         (setq map (buffer-local-value 'mode-specific-map
                                                                       buffer)))
                                       (cl-etypecase map
                                         (list (cdr map))
                                         (symbol (symbol-value map)))))
                             (apply #'append))
                           :from-end t
                           ;; Prevent an error (wrong-type-argument listp keymap)
                           :key #'car-safe
                           :test #'eql)
             with bindings = nil
             do (pcase entry
                  (`(,key menu-item . ,details)
                   (push (cons (key-description (cl-etypecase key
                                                  (number (vector key))
                                                  (vector key)))
                               (or (car-safe details)
                                   "menu-item"))
                         bindings))
                  ((and `(,key . ,def)
                        (guard (not (memq def ignore-bindings))))
                   (push (cons (key-description (cl-etypecase key
                                                  (vector key)
                                                  (otherwise (vector key))))
                               (cond
                                ((keymapp def) "Prefix Command")
                                ((symbolp def) (copy-sequence (symbol-name def)))
                                ((eq 'lambda (car-safe def)) "lambda")
                                ((eq 'menu-item (car-safe def)) "menu-item")
                                ((stringp def) def)
                                ((vectorp def) (key-description def))
                                (t "unknown")))
                         bindings)))
             finally return bindings)))

The problem was that which-key (in which-key--get-current-bindings) uses describe-buffer-bindings to obtain keybindings, and the function doesn't generate keybindings to menu items. It accepts a third argument MENUS, but even if the option is set to non-nil, menu items generated by general-predicate-dispatch aren't contained in the result. I seem to have resolved the issue by reimplementing which-key--get-current-bindings by looking up the given prefix key in all active maps.

I am not sure if this is the right solution. There may be issues I am unaware of, so I want you to test if it produces expected results.

You don't have to turn on which-key-enable-extended-define-key. general-predicate-dispatch doesn't seem to work when the option is turned on. This must be an issue with which-key itself, and not with your package.

@akirak
Copy link

akirak commented Jan 1, 2020

Sorry, it seems to need a lot of workarounds to make it work, so my solution is unrealistic. @terlar was right. Please ignore my comment above.

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

4 participants