grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Franz Hsieh
Subject: Re: [RFC][PATCH] Allow hotkeys to interrupt hidden menu
Date: Mon, 21 Oct 2013 14:45:04 +0800

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



Attachment: grub2-enable-hotkey-in-silent-mode.patch
Description: Binary data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]