Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

Feature/auto describe more binding types #147

Conversation

pdcawley
Copy link

Okay, here's another attempt at rejigging which-key--get-current-bindings and avoiding crawling over the describe-bindings buffer. This replaces #144, and I think I've got most of the edge cases it was missing (key translations, so C-x 8 gets a proper set of prompts, and M- bindings, which end up in the keymap under (27 keymap (?x . cmd)) style nested key maps.)

There's probably stuff I'm missing, but here's the key things it enables and why I think this is worth pursuing:

(global-set-key "C-c T P"
  '(menu-item (if paredit-mode "[x] paredit-mode" "[ ] paredit-mode") paredit-mode))

Now when you enter C-c T and wait, your prompt will include a checkbox showing the current state of paredit-mode. Or:

(define-key help-map "A d" '(menu-item "documentation" apropos-documentation))
(define-key help-map "A f" '(menu-item "function" apropos-function))

And now, whether you get to those keys via C-h A, <f1> A or any other binding you may have dropped help-map into, you'll get your rewritten description.

Getting the bindings for a a prefix by parsing the buffer produced by
`describe-buffer-bindings` seems a little convoluted. Especially when
`map-keymap` seems to be just the job and has the added advantage of
giving us the actual values of the bindings in the current keymap. This
doesn't matter too much for a bound command, but when the binding is to
a keymap it lets us attempt to get `(keymap-prompt binding)` as a
potential name.

I'd not be surprised if it's faster too since we're not mucking about
with regular expressions.
Allow things like

  (define-key global-map (kbd "C-c .")
              (lambda ()
                "open .init.el"
                (find-file-existing user-init-file)))

to Just Work without having to tell which which-key anything else
I was trying to be too clever when dealing with the the 'key' part of
map-keymap. If we just do `(vector key)`, we get something compatible
with key-description
Binding something like:

  `(menu-map "" nil :filter
     ,(lambda (&optional _)
       (if (some-predicate) 'command-name)))

is a cunning trick for setting up bindings that are only active if some
condition holds. This makes which-key show the correct 'final' binding
by evaluating the filter.
Yay for free software! After taking a look at the implementation of
`map-keymap` it's apparent that it respects the ordering of a keymap so
the first entry we get for a given key will be the binding that would be
triggered if we were to actually type that key sequence. So
`-get-raw-current-bindings` now does an assoc look up on the key
description and skips any that have already been seen.

Fixes the issue with C-x u showing up twice while `global-undo-tree` is
turned on (and other, similar cases).
Introduces a new `which-key--canonicalize-bindings` function and
rewrites `which-key--get-raw-current-bindings` in terms of it. This will
allow us to test our keymap munging by passing artificial keymaps into
-canonicalize-bindings, which is a good deal easier than trying to set
up a temporary buffer with the bindings we need (I tried. I couldn't do it).
Minor modes can end up creating keymaps analogous to:

  '(keymap (?a . 'active-binding)
           (keymap (?a . 'overriden-binding)))

We should ensure that the output from `which-key--get-current-bindings`
has eliminated any overridden bindings. This tests that.
Here are the thoughts of an unthinking hacker.

  "Right... the tests are running fine, but I've just realised that that
  parameter is poorly named. I'll just fix that and commit, no need to
  re-run the tests.

  "Hmm... the Travis build is failing... why's that?"

  *tapitty tappity*

  "Ah... yeah, that'd do it, I need to change that parameter name in the
  body of the function don't I?"

We now return you to your regular programming.
If we have:

  (define-key map key (lambda () "description" (interactive) ...)

Then the first line of the function's DOCSTR is probably a better bet
for our key description than "??"
Silly mistakes with quoting corrected
Eliminates an unnecessary intermediate function. Follow master re
ignoring some keys and bindings.
It actually only describes the top level of its keymap arg, so rename to
-describe-immediate-bindings. Not sure that's a good name, but it's a
better name and I can live with that.
Use which-key--describe-binding instead
When you bind a key like `M-n`, it can end up in the key map as a
binding to `ESC n`. So when we hit a form like `(27 keymap ...)` while
describing a keymap, we should map the contents of its keymap onto 'M-'
style keys.
For some reason, (lookup-key key-translation-map key) returns 1 rather
than nil if there's no appropriate entry in the map. This breaks an
assumption in which-key--get-current-bindings. So we fix that.
@justbur
Copy link
Owner

justbur commented Nov 3, 2016

Here are my thoughts on this.

The example of evaluating a function for the binding description is interesting and potentially useful, so I think it's worth pursuing. I am not a fan of abusing menu-items to do this however. First, of all this assumes that people would bind their keys that way, but even then I think it's wrong to encourage this behavior because it's non-standard.

In my opinion, the simpler (and better) way to do this is to manipulate the existing system to allow one to bind a function producing a string to a key sequence. That can be handled easily.

I am also opposed to moving the logic for describing keys into which-key when there's a perfectly fine (and maintained by emacs developers) way of producing the currently accessible key bindings in a buffer. Ideally, I would not have to read in text from a temporary buffer to get it, but that's not really a big deal and it has proven very reliable over the past two years or so. It is further supported by the fact that other popular libraries do the same thing more or less.

It would be more valuable for you to contribute code to emacs itself that produces a list of current key bindings, so that it could be kept up to date by those who know what they're doing.

@justbur
Copy link
Owner

justbur commented Nov 30, 2016

Thank you for the work on this and the ideas. I'm assuming since I haven't heard from you that you are not interested in working on this more.

I recently reworked the way replacements worked in such a way that allows for one of your ideas to work. Try

(push (cons '(nil . "paredit-mode") 
            (lambda (kb)
              (cons (car kb)
                    (if paredit-mode
                        "[x] paredit-mode"
                      "[ ] paredit-mode"))))
      which-key-replacement-alist)

Please see the docstring of which-key-replacement-alist for more information.

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

Successfully merging this pull request may close these issues.

2 participants