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 <franz.hs...@canonical.com>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 <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
>>
>>
>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
