Thanks for the CR, Kevin.

* On 07 Dec 2013, Kevin J. McCarthy wrote: 
> 
> Overall this patch looks good.  I have a few small questions and
> suggestions, but nothing that should hold up the patch.  +1.
> 
> It looks like, with this patch, the usages are now:
>    bind    { map | * }  key  { function | noop }
>    bind    { map | * }  *
>    unbind  { map | * } { key | * }
>    unmacro { map | * } { key | * }
> with the latter three added by the patch.
> 
> I think it would be helpful if the "Usage" section for bind listed both
> modes, and the unbind and unmacro "Usage" showed the * option for the
> map.

Agreed.  I thought I had done more there, so thanks for pointing it out.
Fixed.


> Do we still need the "noop" binding with the patch?

Since binding noop has been in use.. well, forever, I'm hesitant to
remove it (breaking everyone's muttrc) just because it's redundant.  We
could perhaps remove the documentation for it though.


> > +<literal>enter-command</literal> prompt is actually usable, and an
> > +<literal>exit</literal> bindind is added to the <literal>pager</literal>
> 
> typo: bindind -> binding

Fixed, thanks.


> > +  if (mutt_strcmp(key, "*") == 0)
> > +  {
> > +    int neededitorbindings = 0;
> > +
> > +    /* Unbind all in selected menus */
> > +    for (i = 0; i < nummenus; ++i)
> > +    {
> > +      if (menu[i] == -1)
> > +   continue;
> > +      dprint(1, (debugfile, "unbind %s %s\n", mutt_getnamebyvalue(menu[i], 
> > Menus), key));
> > +      km_unbind (&Keymaps[menu[i]], NULL, data);
> > +
> > +      /* If we unbound * in generic, then rebind <enter-command> */
> > +      if ((menu[i] == MENU_GENERIC) && ((data & M_UNBIND) == M_UNBIND))
> > +      {
> > +   neededitorbindings++;
> > +   km_bindkey (":", MENU_GENERIC, OP_ENTER_COMMAND);
> > +      }
> > +
> > +      /* <enter-command> will need basic editor mappings.
> > +       * If editor bindings were removed, remember that. */
> > +      if (menu[i] == MENU_EDITOR)
> > +   neededitorbindings++;
> > +
> > +      /* pager needs at least an exit */
> > +      if (menu[i] == MENU_PAGER)
> > +   km_bindkey ("q", MENU_PAGER, OP_EXIT);
> > +    }
> > +    if (neededitorbindings == 2)
> > +      create_bindings (OpEditor, MENU_EDITOR);
> 
> Why do we only restore the bindings when needitorbindings == 2?
> Should we restore them if we do "unbind editor *"?

You're right, I suspect.  I think I had in mind a scenario that allows
you to:

  unbind * *
  unbind editor *

to get really everything unbound and start from scratch, but I'm not
sure that's really very useful or much desired.  We can put in a means
of forcing all to be unbound in a separate patch, if wanted. ("unbind **
**" or such.)

Revised patch upcoming.

-- 
David Champion • [email protected]

Attachment: pgpdSsNGW2Bjt.pgp
Description: PGP signature

Reply via email to