Ping. Any updates?
On Mon, Oct 21, 2013 at 2:45 PM, Franz Hsieh <franz.hs...@canonical.com> 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 <franz.hs...@canonical.com>
> 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 <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
>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
