bug#43709: minibuffer keybinding cheatsheet and launcher [CODE SUBMISSION]

[Stefan Monnier]

> If you've ever used packages such as `ivy' or `magit', you've probably
> benefited from each's custom combination keybinding cheatsheet and
> launcher: `hydra' in the case of `ivy', and `transient' for `magit'. The
> attached package `key-assist' attempts to offer a generic and very
> simple alternative requiring only the `completing-read' function
> commonly used in core vanilla emacs. `key-assist' is trivial to
> implement "on-the-fly" interactively for any buffer, and
> programmatically much simpler to customize that either `hydra' or
> `transient'. And did I mention that it only requires `completing-read'?

I'm not completely sure what is the intended use scenario.

I mean, I understand the package is not 100% ready, so I'm interested to
know how you imagine it working *ideally* once the various kinks have
been sorted out.

[ To see why I say it's not quite ready, here's my experience with it:
  I just tried it in a fresh new Emacs session, and after `M-x
  key-assist` (which itself is a problem because most users won't
  know/bother to run this command way, so we'd need some more "obvious"
  way to run it) I get a prompt for something and without reading more
  the description I wouldn't know what the prompt wants (I think this
  prompt should only appear if requested explicitly) and then it said
  "no choices found".  So I tried again in a dired buffer, where it
  instead showed me a minibuffer prompt "Select: " with no indication of
  what it is I should be selecting, after that I hit TAB (because I read
  that it uses `completing-read`, but for the un-initiated it might not
  be obvious) at which point it did show me a list of bindings and short
  descriptions and when I selected the first entry (i.e. `!`) it
  signal'd an error about wrong number of arguments.  ]

I do think it would be nice to have something like it, since it's an
alternative to the menus which has the advantage of being
auto-constructed, doesn't need the mouse, and focuses on the keybindings.
It would also be nice to make it usable from minibuffers as well ;-)

See some comments below based on a quick look at the code.


        Stefan


> ;;   If you've ever used packages such as `ivy' or `magit', you've

For those who haven't, it would be good to describe what your package does.

> ;;; Dependencies:
>
> ;; `cl-seq' - For `cl-position', but `cl-lib' is long part of all
> ;;            modern emcasen.

There's no such thing as a `cl-seq` package.
There's a `cl-seq.el` file, which is part of the `cl-lib` package.

> (require 'cl-seq) ;; cl-member, cl-position

Please require `cl-lib` rather than `cl-seq` since which function is
implemented in which file of `cl-lib` is an internal detail (e.g. in
Emacs-24.1 `cl-seq` does not provide `cl-position` but `cl-lib` still does).

> (defun key-assist--get-keybinding (cmd &optional key-map)
>   "Return a string with CMD's shortest keybinding."
>   (let (shortest)
>     (dolist (key (mapcar 'key-description
>                          (where-is-internal
>                            cmd key-map nil t)))
>       (when (or (not shortest)
>                 (> (length shortest) (length key)))
>         (setq shortest key)))
>     shortest))

`where-is-internal` is supposed to return the shortest binding already
when asked to return the "firstonly".  Your notion of length is slightly
different, admittedly, but I wonder what were the concrete cases that
motivated doing your own sorting (might be worth adding that as
a comment in the code).

>     (let ((not-found t))
>       (mapc (lambda (x)
>               (when (string-match x (format "%s" cmd))
>                 (setq not-found nil)))
>             key-assist-exclude-regexps)
>       not-found)))

You should move the `(format "%s" cmd)` out of the loop (and use
`dolist` instead of `mapc`).

>      ((keymapp spec)
>        (let (cmd)
>          (dolist (elem spec)

Please use `map-keymap` rather than `dolist` so as to handle all the
different keymap formats.

>      (while (not (setq choice
>                    (cl-position
>                      (substring-no-properties ; Is -no-properties necessary?
>                        (completing-read prompt choices nil t))
>                      choices :test 'equal))))

Why do you have this loop?  Doesn't the `require-match` arg of
`completing-read` make it unnecessary?

>      (funcall (car (nth choice commands))))))

This should probably be `command-execute` rather than `funcall` to fix
the wrong-number-of-arguments error I bumped into.