Hi Stefan,

Thanks for the feedback. I squashed all of my changes into a new patch
which is attached. I responded to your comments below.


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.


I thought it was overkill, but I wasn't sure if every line should have a
comment. Fixed

> @@ -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.


Done.

 >  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.


I agree. I don't add the argument anymore. Fewer calls to update this
way...

> +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.


I don't have a good name either, but I went with your menu item suggestion.

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).


I understand that. This however is much closer to what I want. I want to be
able to inspect keymaps and provide "automatic" descriptions of key
bindings. I'm most interested in the (STRING . DEF) form for my purpose,
but I think having access to the full menu item might be useful some day.

Best,
Justin


On Thu, Dec 14, 2017 at 9:57 PM, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> [ 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
>
>
>