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 <franz.hs...@canonical.com>wrote:
>
>
>
> On Wed, Sep 11, 2013 at 9:31 PM, Colin Watson <cjwat...@ubuntu.com> 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
>
>
grub2-enable-hotkey-in-silent-mode.patch
Description: Binary data
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
