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

[Vladimir 'phcoder' Serbinenko]

On Nov 28, 2013 3:31 AM, "Colin Watson" <address@hidden> wrote:
>
> Following discussion with Vladimir on IRC, here's another attempt.  The
> preferred user-facing configuration mechanism is now simpler, although
> some unfortunate compatibility code is required to make all this work.
>
> Revamp hidden timeout handling
>
> Add a new timeout_style environment variable and a corresponding
> GRUB_TIMEOUT_STYLE configuration key for grub-mkconfig.  This
> controls hidden-timeout handling more simply than the previous
> arrangements, and pressing any hotkeys associated with menu entries
> during the hidden timeout will now boot the corresponding menu entry
> immediately.
>
> GRUB_HIDDEN_TIMEOUT=<non-empty> + GRUB_TIMEOUT=<non-zero> now
> generates a warning, and if it shows the menu it will do so as if
> the second timeout were not present.  Other combinations are
> translated into reasonable equivalents.
> ---
>  ChangeLog                |  14 ++++
>  docs/grub.texi           |  57 ++++++++++++---
>  grub-core/normal/main.c  |   2 +-
>  grub-core/normal/menu.c  | 175 +++++++++++++++++++++++++++++++++++++++++------
>  util/grub-mkconfig.in    |   1 +
>  util/grub.d/00_header.in |  35 +++++++++-
>  6 files changed, 251 insertions(+), 33 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index d24f533..4cc4562 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,17 @@
> +2013-11-28  Colin Watson  <address@hidden>
> +
> +       Add a new timeout_style environment variable and a corresponding
> +       GRUB_TIMEOUT_STYLE configuration key for grub-mkconfig.  This
> +       controls hidden-timeout handling more simply than the previous
> +       arrangements, and pressing any hotkeys associated with menu entries
> +       during the hidden timeout will now boot the corresponding menu entry
> +       immediately.
> +
> +       GRUB_HIDDEN_TIMEOUT=<non-empty> + GRUB_TIMEOUT=<non-zero> now
> +       generates a warning, and if it shows the menu it will do so as if
> +       the second timeout were not present.  Other combinations are
> +       translated into reasonable equivalents.
> +
>  2013-11-27  Vladimir Serbinenko  <address@hidden>
>
>         Eliminate variable length arrays in grub_vsnprintf_real.
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 6aee292..f494a3d 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -1298,19 +1298,46 @@ a key is pressed.  The default is @samp{5}.  Set to @samp{0} to boot
>  immediately without displaying the menu, or to @samp{-1} to wait
>  indefinitely.
>
> +If @samp{GRUB_TIMEOUT_STYLE} is set to @samp{countdown} or @samp{hidden},
> +the timeout is instead counted before the menu is displayed.
> +
> address@hidden GRUB_TIMEOUT_STYLE
> +If this option is unset or set to @samp{menu}, then GRUB will display the
> +menu and then wait for the timeout set by @samp{GRUB_TIMEOUT} to expire
> +before booting the default entry.  Pressing a key interrupts the timeout.
> +
> +If this option is set to @samp{countdown} or @samp{hidden}, then, before
> +displaying the menu, GRUB will wait for the timeout set by
> address@hidden to expire.  If @key{ESC} is pressed during that time, it
> +will display the menu and wait for input according to @samp{GRUB_TIMEOUT}.
> +If a hotkey associated with a menu entry is pressed, it will boot the
> +associated menu entry immediately.  If the timeout expires before either of
> +these happens, it will display the menu.
What you describe here doesn‘t serm what code is doing. Copypaste error?
> In the @samp{countdown} case, it
> +will show a one-line indication of the remaining time.
> +
> address@hidden GRUB_HIDDEN_TIMEOUT
> -Wait this many seconds for @key{ESC} to be pressed before displaying the menu.
> -If no @key{ESC} is pressed during that time, display the menu for the number of
> -seconds specified in GRUB_TIMEOUT before booting the default entry. We expect
> -that most people who use GRUB_HIDDEN_TIMEOUT will want to have GRUB_TIMEOUT set
> -to @samp{0} so that the menu is not displayed at all unless @key{ESC} is
> -pressed.
> -Unset by default.
> +Wait this many seconds before displaying the menu.  If @key{ESC} is pressed
> +during that time, display the menu and wait for input according to
> address@hidden  If a hotkey associated with a menu entry is pressed,
> +boot the associated menu entry immediately.  If the timeout expires before
> +either of these happens, display the menu for the number of seconds
> +specified in @samp{GRUB_TIMEOUT} before booting the default entry.
> +
> +If you set @samp{GRUB_HIDDEN_TIMEOUT}, you should also set
> address@hidden so that the menu is not displayed at all unless
> address@hidden is pressed.
> +
> +This option is unset by default, and is deprecated in favour of the less
> +confusing @samp{GRUB_TIMEOUT_STYLE=countdown} or
> address@hidden
>
> address@hidden GRUB_HIDDEN_TIMEOUT_QUIET
>  In conjunction with @samp{GRUB_HIDDEN_TIMEOUT}, set this to @samp{true} to
>  suppress the verbose countdown while waiting for a key to be pressed before
> -displaying the menu.  Unset by default.
> +displaying the menu.
> +
> +This option is unset by default, and is deprecated in favour of the less
> +confusing @samp{GRUB_TIMEOUT_STYLE=countdown}.
>
> address@hidden GRUB_DEFAULT_BUTTON
> address@hidden GRUB_TIMEOUT_BUTTON
> @@ -3030,6 +3057,7 @@ These variables have special meaning to GRUB.
>  * superusers::
>  * theme::
>  * timeout::
> +* timeout_style::
> address@hidden menu
>
>
> @@ -3462,10 +3490,23 @@ keyboard input before booting the default menu entry.  A timeout of @samp{0}
>  means to boot the default entry immediately without displaying the menu; a
>  timeout of @samp{-1} (or unset) means to wait indefinitely.
>
> +If @samp{timeout_style} (@pxref{timeout_style}) is set to @samp{countdown}
> +or @samp{hidden}, the timeout is instead counted before the menu is
> +displayed.
> +
>  This variable is often set by @samp{GRUB_TIMEOUT} or
> address@hidden (@pxref{Simple configuration}).
>
>
> address@hidden timeout_style
> address@hidden timeout_style
> +
> +This variable may be set to @samp{menu}, @samp{countdown}, or @samp{hidden}
> +to control the way in which the timeout (@pxref{timeout}) interacts with
> +displaying the menu.  See the documentation of @samp{GRUB_TIMEOUT_STYLE}
> +(@pxref{Simple configuration}) for details.
> +
> +
> address@hidden Environment block
> address@hidden The GRUB environment block
>
> diff --git a/grub-core/normal/main.c b/grub-core/normal/main.c
> index ad36273..778de61 100644
> --- a/grub-core/normal/main.c
> +++ b/grub-core/normal/main.c
> @@ -523,7 +523,7 @@ static const char *features[] = {
>    "feature_chainloader_bpb", "feature_ntldr", "feature_platform_search_hint",
>    "feature_default_font_path", "feature_all_video_module",
>    "feature_menuentry_id", "feature_menuentry_options", "feature_200_final",
> -  "feature_nativedisk_cmd"
> +  "feature_nativedisk_cmd", "feature_timeout_style"
>  };
>
>  GRUB_MOD_INIT(normal)
> diff --git a/grub-core/normal/menu.c b/grub-core/normal/menu.c
> index 9b88290..fa85c35 100644
> --- a/grub-core/normal/menu.c
> +++ b/grub-core/normal/menu.c
> @@ -40,6 +40,22 @@
>  grub_err_t (*grub_gfxmenu_try_hook) (int entry, grub_menu_t menu,
>                                      int nested) = NULL;
>
> +enum timeout_style {
> +  TIMEOUT_STYLE_MENU,
> +  TIMEOUT_STYLE_COUNTDOWN,
> +  TIMEOUT_STYLE_HIDDEN
> +};
> +
> +struct timeout_style_name {
> +  const char *name;
> +  enum timeout_style style;
> +} timeout_style_names[] = {
> +  {"menu", TIMEOUT_STYLE_MENU},
> +  {"countdown", TIMEOUT_STYLE_COUNTDOWN},
> +  {"hidden", TIMEOUT_STYLE_HIDDEN},
> +  {NULL, 0}
> +};
> +
>  /* Wait until the user pushes any key so that the user
>     can see what happened.  */
>  void
> @@ -70,6 +86,38 @@ grub_menu_get_entry (grub_menu_t menu, int no)
>    return e;
>  }
>
> +/* Get the index of a menu entry associated with a given hotkey, or -1.  */
> +static int
> +get_entry_index_by_hotkey (grub_menu_t menu, int hotkey)
> +{
> +  grub_menu_entry_t entry;
> +  int i;
> +
> +  for (i = 0, entry = menu->entry_list; i < menu->size;
> +       i++, entry = entry->next)
> +    if (entry->hotkey == hotkey)
> +      return i;
> +
> +  return -1;
> +}
> +
> +/* Return the timeout style.  If the variable "timeout_style" is not set or
> +   invalid, default to TIMEOUT_STYLE_MENU.  */
> +static enum timeout_style
> +get_timeout_style (void)
> +{
> +  const char *val;
> +  struct timeout_style_name *style_name;
> +
> +  val = grub_env_get ("timeout_style");
> +  if (val)
> +    for (style_name = timeout_style_names; style_name->name; style_name++)
> +      if (grub_strcmp (style_name->name, val) == 0)
> +       return style_name->style;
> +
> +  return TIMEOUT_STYLE_MENU;
> +}
> +
>  /* Return the current timeout. If the variable "timeout" is not set or
>     invalid, return -1.  */
>  int
> @@ -488,6 +536,33 @@ get_entry_number (grub_menu_t menu, const char *name)
>    return entry;
>  }
>
> +/* Check whether a second has elapsed since the last tick.  If so, adjust
> +   the timer and return 1; otherwise, return 0.  */
> +static int
> +has_second_elapsed (grub_uint64_t *saved_time)
> +{
> +  grub_uint64_t current_time;
> +
> +  current_time = grub_get_time_ms ();
> +  if (current_time - *saved_time >= 1000)
> +    {
> +      *saved_time = current_time;
> +      return 1;
> +    }
> +  else
> +    return 0;
> +}
> +
> +static void
> +print_countdown (struct grub_term_coordinate *pos, int n)
> +{
> +  grub_term_restore_pos (pos);
> +  /* NOTE: Do not remove the trailing space characters.
> +     They are required to clear the line.  */
> +  grub_printf ("%d    ", n);
> +  grub_refresh ();
> +}
> +
>  #define GRUB_MENU_PAGE_SIZE 10
>
>  /* Show the menu and handle menu entry selection.  Returns the menu entry
> @@ -502,6 +577,7 @@ run_menu (grub_menu_t menu, int nested, int *auto_boot)
>    grub_uint64_t saved_time;
>    int default_entry, current_entry;
>    int timeout;
> +  enum timeout_style timeout_style;
>
>    default_entry = get_entry_number (menu, "default");
>
> @@ -510,8 +586,71 @@ run_menu (grub_menu_t menu, int nested, int *auto_boot)
>    if (default_entry < 0 || default_entry >= menu->size)
>      default_entry = 0;
>
> +  timeout = grub_menu_get_timeout ();
> +  if (timeout < 0)
> +    /* If there is no timeout, the "countdown" and "hidden" styles result in
> +       the system doing nothing and providing no or very little indication
> +       why.  Technically this is what the user asked for, but it's not very
> +       useful and likely to be a source of confusion, so we disallow this.  */
> +    grub_env_unset ("timeout_style");
> +
> +  timeout_style = get_timeout_style ();
> +
> +  if (timeout_style == TIMEOUT_STYLE_COUNTDOWN
> +      || timeout_style == TIMEOUT_STYLE_HIDDEN)
> +    {
> +      static struct grub_term_coordinate *pos;
> +      int entry = -1;
> +
> +      if (timeout_style == TIMEOUT_STYLE_COUNTDOWN && timeout)
> +       {
> +         pos = grub_term_save_pos ();
> +         print_countdown (pos, timeout);
> +       }
> +
> +      /* Enter interruptible sleep until Escape or a menu hotkey is pressed,
> +         or the timeout expires.  */
> +      saved_time = grub_get_time_ms ();
> +      while (1)
> +       {
> +         int key;
> +
> +         key = grub_getkey_noblock ();
> +         if (key != GRUB_TERM_NO_KEY)
> +           {
> +             entry = get_entry_index_by_hotkey (menu, key);
> +             if (entry >= 0)
> +               break;
> +           }
> +         if (key == GRUB_TERM_ESC)
> +           {
> +             timeout = -1;
> +             break;
> +           }
> +
> +         if (timeout > 0 && has_second_elapsed (&saved_time))
> +           {
> +             timeout--;
> +             if (timeout_style == TIMEOUT_STYLE_COUNTDOWN)
> +               print_countdown (pos, timeout);
> +           }
> +
> +         if (timeout == 0)
> +           /* We will fall through to auto-booting the default entry.  */
> +           break;
> +       }
> +
> +      grub_env_unset ("timeout");
> +      grub_env_unset ("timeout_style");
> +      if (entry >= 0)
> +       {
> +         *auto_boot = 0;
> +         return entry;
> +       }
> +    }
> +
>    /* If timeout is 0, drawing is pointless (and ugly).  */
> -  if (grub_menu_get_timeout () == 0)
> +  if (timeout == 0)
>      {
>        *auto_boot = 1;
>        return default_entry;
> @@ -540,18 +679,11 @@ run_menu (grub_menu_t menu, int nested, int *auto_boot)
>        if (grub_normal_exit_level)
>         return -1;
>
> -      if (timeout > 0)
> +      if (timeout > 0 && has_second_elapsed (&saved_time))
>         {
> -         grub_uint64_t current_time;
> -
> -         current_time = grub_get_time_ms ();
> -         if (current_time - saved_time >= 1000)
> -           {
> -             timeout--;
> -             grub_menu_set_timeout (timeout);
> -             saved_time = current_time;
> -             menu_print_timeout (timeout);
> -           }
> +         timeout--;
> +         grub_menu_set_timeout (timeout);
> +         menu_print_timeout (timeout);
>         }
>
>        if (timeout == 0)
> @@ -653,16 +785,15 @@ run_menu (grub_menu_t menu, int nested, int *auto_boot)
>
>             default:
>               {
> -               grub_menu_entry_t entry;
> -               int i;
> -               for (i = 0, entry = menu->entry_list; i < menu->size;
> -                    i++, entry = entry->next)
> -                 if (entry->hotkey == c)
> -                   {
> -                     menu_fini ();
> -                     *auto_boot = 0;
> -                     return i;
> -                   }
> +               int entry;
> +
> +               entry = get_entry_index_by_hotkey (menu, c);
> +               if (entry >= 0)
> +                 {
> +                   menu_fini ();
> +                   *auto_boot = 0;
> +                   return entry;
> +                 }
>               }
>               break;
>             }
> diff --git a/util/grub-mkconfig.in b/util/grub-mkconfig.in
> index ba1d4ef..50f73aa 100644
> --- a/util/grub-mkconfig.in
> +++ b/util/grub-mkconfig.in
> @@ -186,6 +186,7 @@ export GRUB_DEFAULT \
>    GRUB_HIDDEN_TIMEOUT \
>    GRUB_HIDDEN_TIMEOUT_QUIET \
>    GRUB_TIMEOUT \
> +  GRUB_TIMEOUT_STYLE \
you need button variant as well
>    GRUB_DEFAULT_BUTTON \
>    GRUB_HIDDEN_TIMEOUT_BUTTON \
>    GRUB_TIMEOUT_BUTTON \
> diff --git a/util/grub.d/00_header.in b/util/grub.d/00_header.in
> index 9838720..d5cf342 100644
> --- a/util/grub.d/00_header.in
> +++ b/util/grub.d/00_header.in
> @@ -282,14 +282,45 @@ fi
>
>  make_timeout ()
>  {
> -    if [ "x${1}" != "x" ] ; then
> +    if [ "x${GRUB_TIMEOUT_STYLE}" != "x" ] ; then
> +       cat << EOF
> +if [ x\$feature_timeout_style = xy ] ; then
> +  set timeout_style=${GRUB_TIMEOUT_STYLE}
> +  set timeout=${2}
> +EOF
> +       if [ "x${GRUB_TIMEOUT_STYLE}" != "xmenu" ] ; then
> +           # Fallback hidden-timeout code in case the timeout_style feature
> +           # is unavailable.  Note that we now ignore GRUB_HIDDEN_TIMEOUT
> +           # and take the hidden-timeout from GRUB_TIMEOUT instead.
> +           if [ "x${GRUB_TIMEOUT_STYLE}" = "xhidden" ] ; then
> +               verbose=
> +           else
> +               verbose=" --verbose"
> +           fi
> +           cat << EOF
> +elif sleep$verbose --interruptible ${2} ; then
> +  set timeout=0
> +EOF
> +       fi
> +       cat << EOF
> +fi
> +EOF
> +    elif [ "x${1}" != "x" ] ; then
> +       if [ "x${2}" != "x0" ] ; then
> +           grub_warn "$(gettext "Setting GRUB_TIMEOUT to a non-zero value when GRUB_HIDDEN_TIMEOUT is set is no longer supported.")"
> +       fi
>         if [ "x${GRUB_HIDDEN_TIMEOUT_QUIET}" = "xtrue" ] ; then
>             verbose=
> +           style="hidden"
>         else
>             verbose=" --verbose"
> +           style="countdown"
>         fi
>         cat << EOF
> -if sleep$verbose --interruptible ${1} ; then
> +if [ x\$feature_timeout_style = xy ] ; then
> +  set timeout_style=$style
> +  set timeout=${1}
> +elif sleep$verbose --interruptible ${1} ; then
>    set timeout=${2}
Is behaviour mismatch between both versions intentional?
I see 2 ways of handling double timeout: either not supporting at all anymore or generate old code for it. This one seems to be mix of both
>  fi
>  EOF
> --
> 1.8.4.4
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel