Re: [RFC][PATCH] Allow hotkeys to interrupt hidden menu

[Yang Bai]

Ping. Any updates?

On Mon, Oct 21, 2013 at 2:45 PM, Franz Hsieh <address@hidden> wrote:
> Hi Vladimir,
>   I don't know if l lose any update from you. Would you give me comments
> about
>   the latest hotkey patch?
>
>   Summary of the patch:
>   * allow function keys / delete / backspace to interrupt sleep and set
> 'hotkey' variable.
>   * remove key aliases structure.
>   * using grub_xasprintf instead of grub_snprintf
>   * unset the 'hotkey' variable quickly when it is unneeded in
> grub_menu_get_hotkey
>
>   Many thanks!
>
> Franz Hsieh
>
> On Mon, Oct 14, 2013 at 2:02 PM, Franz Hsieh <address@hidden>
> wrote:
>>
>> On 02.10.2013 10:50, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>
>> >> Hi, there
>> >>
>> >>   I had merged the patch for enabling hotkey in grub silent mode to
>> >> grub-trunk.
>> >>   The --function-key now has been removed from sleep.mod, now sleep.mod
>> >> will
>> >>   monitor all function keys, delete, tab and backspace.
>> >>
>> > Have you missed on-going opposition, discussion and improvement
>> > proposals from me?
>> > I was going to revert the patch but found out that you didn't actually
>> > commit it, please see my comments to make it acceptable and don't push
>> > it.
>>
>> Sorry I did not say it clearly, I downloaded sources from grub-trunk, add
>> my changes,
>> and finished the patch file. I haven't committed the patch to trunk yet.
>>
>> I looked at the codes, there is no ungetkey function to do like ungetc. So
>> currently
>> I made it to read all keys but only accept function keys / delete /
>> backspace and Esc
>> keys to interrupt the sleep thread.
>>
>> Only function keys / delete key / backspace key are allowed for hotkey
>>
>>
>>
>> and will be saved to the hotkey environment variable.
>>
>>
>>
>> On Wed, Oct 2, 2013 at 4:03 PM, Franz Hsieh <address@hidden>
>> wrote:
>>>
>>> Hi, there
>>>
>>>   I had merged the patch for enabling hotkey in grub silent mode to
>>> grub-trunk.
>>>   The --function-key now has been removed from sleep.mod, now sleep.mod
>>> will
>>>   monitor all function keys, delete, tab and backspace.
>>>
>>> Thanks!
>>>
>>> Franz Hsieh
>>>
>>>
>>> On Fri, Sep 13, 2013 at 5:18 PM, Franz Hsieh <address@hidden>
>>> wrote:
>>>>
>>>>
>>>>
>>>>
>>>> On Wed, Sep 11, 2013 at 9:31 PM, Colin Watson <address@hidden>
>>>> wrote:
>>>>>
>>>>> Hi Franz,
>>>>>
>>>>> Throughout this patch, please take care to adhere to the GRUB coding
>>>>> style.  This is definitely an improvement over previous versions I've
>>>>> reviewed, but it still has a number of places where functions are
>>>>> called
>>>>> or declared with no space before the opening parenthesis.  That is,
>>>>> "function()" should become "function ()".  I know it's a minor point,
>>>>> but it makes code much easier to read when it's all in the same style.
>>>>>
>>>>> On Wed, Sep 11, 2013 at 02:18:04PM +0100, Franz Hsieh (via Colin
>>>>> Watson)
>>>>> wrote:
>>>>> > +static struct
>>>>> > +{
>>>>> > +  char *name;
>>>>> > +  int key;
>>>>> > +} function_key_aliases[] =
>>>>> > +  {
>>>>> > +    {"f1", GRUB_TERM_KEY_F1},
>>>>> > +    {"f2", GRUB_TERM_KEY_F2},
>>>>> > +    {"f3", GRUB_TERM_KEY_F3},
>>>>> > +    {"f4", GRUB_TERM_KEY_F4},
>>>>> > +    {"f5", GRUB_TERM_KEY_F5},
>>>>> > +    {"f6", GRUB_TERM_KEY_F6},
>>>>> > +    {"f7", GRUB_TERM_KEY_F7},
>>>>> > +    {"f8", GRUB_TERM_KEY_F8},
>>>>> > +    {"f9", GRUB_TERM_KEY_F9},
>>>>> > +    {"f10", GRUB_TERM_KEY_F10},
>>>>> > +    {"f11", GRUB_TERM_KEY_F11},
>>>>> > +    {"f12", GRUB_TERM_KEY_F12},
>>>>> > +  };
>>>>> > +
>>>>>
>>>>> This is essentially a copy of hotkey_aliases from
>>>>> grub-core/commands/menuentry.c, and there's duplicated lookup code as
>>>>> well.  Can you find any way to share it?  Since we certainly don't want
>>>>> to put this in the kernel, and neither the sleep module nor the normal
>>>>> module really ought to depend on the other, I suspect that doing so
>>>>> would require a new module for shared menu code, which may well be
>>>>> overkill for this, but it's worth a look.
>>>>
>>>>
>>>> I also agree to remove duplicate code, but seems not easy to do it
>>>> unless
>>>> we have a shared module, right? Well it would take me some time to
>>>> evaluate.
>>>>
>>>>> At the very least it would be a good idea to bring the contents of this
>>>>> structure into sync with hotkey_aliases and add comments to encourage
>>>>> developers to keep them that way.
>>>>>
>>>>> I think we should also call this kind of thing simply "hotkey"
>>>>> consistently across the code, rather than it sometimes being
>>>>> "function_key" or "fnkey".
>>>>
>>>>
>>>>>
>>>>>
>>>>> > +      if (key == fnkey)
>>>>> > +     {
>>>>> > +       char hotkey[32] = {0};
>>>>> > +       grub_snprintf(hotkey, 32, "%d", key);
>>>>> > +       grub_env_set("hotkey", (char*)&hotkey);
>>>>> > +       return 1;
>>>>> > +     }
>>>>>
>>>>> We've generally tried to get rid of this kind of static allocation.
>>>>> Use
>>>>> grub_xasprintf instead:
>>>>>
>>>>>   if (key == fnkey)
>>>>>     {
>>>>>       char *hotkey = grub_xasprintf ("%d", key);
>>>>>       grub_env_set ("hotkey", hotkey);
>>>>>       grub_free (hotkey);
>>>>>       return 1;
>>>>>     }
>>>>>
>>>>> > +int
>>>>> > +grub_menu_get_hotkey (void)
>>>>>
>>>>> This function is only used in this file, so it should be static.
>>>>
>>>>
>>>> These idea are good and I will do these changes in my patch.
>>>>
>>>>> Rather than having to configure this explicitly, I have an alternative
>>>>> suggestion, which ties into my earlier suggestion of making the hotkey
>>>>> code a bit more shared.  Why not have sleep --interruptible look up the
>>>>> hotkeys configured in the menu and automatically honour any of those?
>>>>> That way you wouldn't have to configure hotkeys in two places (grub.cfg
>>>>> and /etc/default/grub).  Plus, I'm always more comfortable with patches
>>>>> that don't require adding configuration options.
>>>>
>>>>
>>>> Agree with Colin's points and I will update the patch.
>>>> By the way I would like to combine --function-key and --interruptible. I
>>>> think
>>>> it is a simple way that sleep.mod always checks ESC for interrupt and
>>>> f1~f12 for the
>>>> hotkey.
>>>>
>>>> Thanks for your reviews and if there is any good suggestion, please feel
>>>> free to tell me.
>>>>
>>>> Regards,
>>>> Franz Hsieh
>>>>
>>>
>>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
>