Re: [PATCH 07/10] kern/term: Accept ESC, F4 and holding SHIFT as user interrupt keys

[Daniel Kiper]

On Fri, Mar 13, 2020 at 08:15:02PM +0100, Javier Martinez Canillas wrote:
> From: Hans de Goede <address@hidden>
>
> On some devices the ESC key is the hotkey to enter the BIOS/EFI setup
> screen, making it really hard to time pressing it right. Besides that
> ESC is also pretty hard to discover for a user who does not know it
> will unhide the menu.
>
> This commit makes F4, which used to be the hotkey to show the Windows
> boot menu during boot for a long long time, also interrupt sleeps /
> stop the menu countdown.
>
> This solves the ESC gets into the BIOS setup and also somewhat solves
> the discoverability issue, but leaves the timing issue unresolved.
>
> This commit fixes the timing issue by also adding support for keeping
> SHIFT pressed during boot to stop the menu countdown. This matches
> what Ubuntu is doing, which should also help with discoverability.
>
> Signed-off-by: Hans de Goede <address@hidden>
> Signed-off-by: Javier Martinez Canillas <address@hidden>
>

Please drop this empty line...

> ---
>
>  grub-core/commands/sleep.c |  2 +-
>  grub-core/kern/term.c      | 16 ++++++++++++++++
>  grub-core/normal/menu.c    |  2 +-
>  include/grub/term.h        |  1 +
>  4 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/commands/sleep.c b/grub-core/commands/sleep.c
> index e77e7900fac..a1370b710c9 100644
> --- a/grub-core/commands/sleep.c
> +++ b/grub-core/commands/sleep.c
> @@ -55,7 +55,7 @@ grub_interruptible_millisleep (grub_uint32_t ms)
>    start = grub_get_time_ms ();
>
>    while (grub_get_time_ms () - start < ms)
> -    if (grub_getkey_noblock () == GRUB_TERM_ESC)
> +    if (grub_key_is_interrupt (grub_getkey_noblock ()))
>        return 1;
>
>    return 0;
> diff --git a/grub-core/kern/term.c b/grub-core/kern/term.c
> index 93bd3378d18..8b6ef0f3e26 100644
> --- a/grub-core/kern/term.c
> +++ b/grub-core/kern/term.c
> @@ -138,6 +138,22 @@ grub_getkeystatus (void)
>    return status;
>  }
>
> +int
> +grub_key_is_interrupt (int key)
> +{
> +  /* ESC sometimes is the BIOS setup hotkey and may be hard to discover, also
> +     check F4, which was the key to get the Windows bootmenu for a long 
> time. */

Incorrect comment formating...

> +  if (key == GRUB_TERM_ESC || key == GRUB_TERM_KEY_F4)
> +    return 1;
> +
> +  /* Pressing keys at the right time during boot is hard to time, also allow
> +     interrupting sleeps / the menu countdown by keeping shift pressed. */

Ditto...

Otherwise LGTM == my RB.

Daniel