Re: Proposed patch for lookup-key

[Stefan Monnier]

[ I remember thinking about this need many years ago and thinking "bah,
  I'll get to it when the need becomes pressing enough, but not now".

  FWIW currently my "best" answer in Elisp for the problem you're
  suggesting to solve is to `lookup-key` on the first n-1 events and
  then use `map-keymap` to traverse the resulting final keymap looking
  for the nᵗʰ event.  ]

> I'm wondering if the attached patch would be acceptable. The idea was to
> add an optional argument to lookup-key to prevent it from stripping this
> information about the key bindings.

Comments below.


        Stefan


> * src/keyboard.c (read_char): Adjust call to access_keymap
> (menu_bar_items): Adjust call to access_keymap
> (tool_bar_items): Adjust call to access_keymap
> (follow_key): Adjust call to access_keymap
> (access_keymap_keyremap): Adjust call to access_keymap
> * src/keymap.c: Change get_keyelt declaration
> (access_keymap_1): Add menus arg and adjust recursive calls
> (get_keyelt): Add menus arg
> (Fdefine_key): Adjust call to access_keymap
> (Fcommand_remapping): Adjust call to Flookup_key
> (Flookup_key): Add menus arg and adjust call to access_keymap
> (Fkey_binding): Adjust call to Flookup_key
> (Flocal_key_binding): Adjust call to Flookup_key
> (Fglobal_key_binding): Adjust call to Flookup_key
> (Fminor_mode_key_binding): Adjust call to Flookup_key
> (accessible_keymaps_1): Adjust call to get_keyelt
> (Faccessible_keymaps): Adjust call to Flookup_key
> (shadow_lookup): Adjust call to Flookup_key
> (where_is_internal_1): Adjust call to get_keyelt
> (describe_map_tree): Adjust call to Flookup_key
> (describe_map): Adjust calls to get_keyelt and Flookup_key
> (describe_vector): Adjust calls to get_keyelt and Flookup_key
> * src/keymap.h: Adjust access_keymap declaration

You don't need to say "Adjust calls to" for all those functions.
You can just say something like:

    (access_keymap_1): Add 'menus' arg; adjust all calls.

Otherwise the actual meat of the change gets drowned within all the
minor mechanical adjustments.

> @@ -368,7 +368,8 @@ Return PARENT.  PARENT should be nil or another keymap.  
> */)
 
>  static Lisp_Object
>  access_keymap_1 (Lisp_Object map, Lisp_Object idx,
> -              bool t_ok, bool noinherit, bool autoload)
> +              bool t_ok, bool noinherit, bool autoload,
> +                 bool menus)

Please update the comment that's just before that function to document
this `menus` argument.

>  static Lisp_Object
> -get_keyelt (Lisp_Object object, bool autoload)
> +get_keyelt (Lisp_Object object, bool autoload, bool menus)
>  {
>    while (1)
>      {
> -      if (!(CONSP (object)))
> -     /* This is really the value.  */
> +      if (!(CONSP (object)) || menus)
> +     /* This is really the value or we do not want to process
> +           menu-items.  */
>       return object;

If `menus` is true, then we will always just return `object` unchanged.
So an alternative would be to test `menus` before calling this function
(so we don't need to add the `menus` argument to this function).
Probably doesn't make much of a difference, tho.

> +A non-nil value for MENUS makes `lookup-key` return full menu-items
> +instead of just the associated definition. */)

Elements like (STRING . COMMAND) aren't only used for menus, so I'd
prefer to use another name.  I don't we actually have a good name at
hand for that, but a common name used elsewhere for those objects is
"menu item", so maybe we could use that.

BTW, one problem (and one of the reasons why I didn't implement the
feature back then using an approach like the one you suggest) is that
this treats a "menu item" as opaque (meaning it hides everything in
lower keymaps).  Yet if we do let get_keyelt do its job to look inside
the menu-item it may find that the binding is sometimes nil (e.g. for
bindings which are made dynamically conditional using a :filter
function), and other times we'll find a binding which is a keymap which
is then combined with other keymaps found in lower-precedence keymaps
(e.g. some menu bar menus have entries provided by global-map as well as
from other keymap).

To give a concrete example,

    (lookup-key (current-active-maps) [menu-bar] nil 'menu-item)

could return a menu-item containing a keymap with just one element (the
major mode's menu, or maybe one of the minor mode's menu), whereas

    (lookup-key (current-active-maps) [menu-bar])

returns a keymap with many more elements (all the top-level menus,
including those from the global map, those from the major mode and
those from the minor modes).

Solving this problem is a lot of work (because of all the different
possible cases), and will rarely make a difference, so I'm not sure that
it's a valid argument against your patch.


        Stefan