bug#13948: no key-binding-locus

[Stefan Monnier <monnier@iro.umontreal.ca>]

> Here's another attempt. Like the previous patch, I define
> key-binding-keymap which finds the keymap by mimicking key-binding, and
> describe-key--binding-locus which matches the keymap to a symbol and
> makes a description suitable for describe-key.

Looks pretty good.  Feel free to install it into `trunk'.
See some nitpicks below.

> +(defun key-binding-keymap (key &optional accept-default no-remap position)

Please name it (and all the functions/vars you add in help*.el) with
a leading "help-".  I'd name it help--key-binding-keymap.

> +  "Return a keymap holding a binding for KEY within current keymaps.
> +The effect of the arguments KEY, ACCEPT-DEFAULT, NO-REMAP and
> +POSITION is as documented in the function `key-binding'."
> +  (let* ((active-maps (current-active-maps t position))
> +         map found)
> +    ;; we loop over active maps like key-binding does.

Please capitalize your comments.

> +      (setq found (lookup-key
> +                   map
> +                   key
> +                   accept-default))

I think this all fits on a single line.

> +      (when (integerp found)
> +        ;; prefix was found but not the whole sequence
> +        (setq found nil)))

If key-binding returned a command, then we should never hit this case.
It's OK to keep the code, but you might like to add a comment mentioning
that we don't expect this case to happen.

> +    (when found
> +      (if (and (symbolp found)
> +               (not no-remap)
> +               (command-remapping found))
> +          (key-binding-keymap (vector 'remap found))

Please add a comment here mentioning that to be really thorough, in the
remap case the use would like to know in which map the final command was
found but also in which map the remapping was found.

> +                                     (mapcar
> +                                      (lambda (mode)
> +                                        (when
> +                                            (symbol-value mode)
> +                                          (intern
> +                                           (format "%s-map" mode))))
> +                                      minor-mode-list))

Use minor-mode-map-alist instead (which requires a tweak to the body
of the mapcar, of course).

Also I'd use intern-soft here (tho it probably doesn't make any
difference in practice, here).

> +         ;; look into these advertised symbols first
> +         (while (and (not found) advertised-syms)
> +           (let ((sym (pop advertised-syms)))
> +             (setq found
> +                   (when (and
> +                          (boundp sym)
> +                          (eq map (symbol-value sym)))
> +                     sym))))
> +         ;; only look in other symbols otherwise
> +         (when (not found)
> +           (mapatoms
> +            (lambda (x)
> +              (when (and (boundp x)
> +                         ;; avoid let-bound symbols
> +                         (special-variable-p x)
> +                         (eq (symbol-value x) map))
> +                (push x found))))
> +           (when (and (consp found)
> +                      (null (cdr found)))
> +             (setq found (car found))))

I think this code will be simpler if you use catch/throw (you can then
use dolist over advertised-syms, for example and you don't need `found').

> +         (when found
> +           (format " (found in %s)" found)))))
> +   ""))

I think it's better to make this function return the symbol rather than
a string, and let the caller turn it into a string.


        Stefan