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

[Colin Watson]

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.  In the @samp{countdown} case, it
+will show a one-line indication of the remaining time.
+
 @item 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
 
 @item 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}.
 
 @item GRUB_DEFAULT_BUTTON
 @itemx GRUB_TIMEOUT_BUTTON
@@ -3030,6 +3057,7 @@ These variables have special meaning to GRUB.
 * superusers::
 * theme::
 * timeout::
+* timeout_style::
 @end 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
 @samp{GRUB_HIDDEN_TIMEOUT} (@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.
+
+
 @node Environment block
 @section 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 \
   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}
 fi
 EOF
-- 
1.8.4.4