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

[Colin Watson]

On Mon, Oct 21, 2013 at 02:45:04PM +0800, Franz Hsieh wrote:
>   I don't know if l lose any update from you. Would you give me comments
> about the latest hotkey patch?

Vladimir and I talked about this on IRC today.  We share a dislike for
the hardcoded hotkey names.  Between us, we came up with a better plan:

 * The simplest way to work out what hotkeys to honour is to introspect
   the menu structure itself.  However, this can only be done after all
   the top-level commands in grub.cfg have been executed, so it can't be
   done in the "sleep" command.
 * There's nothing particular that says that we have to implement the
   hidden timeout in a "sleep" command, although we have to take some
   care to ensure configuration file compatibility with older modules.
   Since the main timeout is implemented in normal.mod, it makes some
   sense for the hidden timeout to go there too, where we can get at the
   menu structure.
 * The simplest way to arrange for configuration file compatibility is
   to add a new "hiddentimeout" command that sets a "hidden_timeout"
   environment variable; we could also set the environment variable
   directly, but having a command allows us to detect the absence of
   that command and infer that we need to fall back to the old
   mechanism.

Here's a candidate patch.  This does duplicate "sleep" a little bit, but
it has the great advantages of not duplicating the list of valid hotkeys
and of only honouring hotkeys that are associated with menu entries.

 Add a new "hiddentimeout" command.  If this is used, then the
 menu will use a hidden timeout much as was previously done with "sleep
 --interruptible", except that pressing any hotkeys associated with menu
 entries will boot the corresponding menu entry immediately.

---
 ChangeLog                          |   7 ++
 docs/grub.texi                     |  39 ++++++--
 grub-core/Makefile.core.def        |   5 ++
 grub-core/commands/hiddentimeout.c |  68 ++++++++++++++
 grub-core/normal/menu.c            | 177 ++++++++++++++++++++++++++++++++-----
 util/grub.d/00_header.in           |   6 +-
 6 files changed, 271 insertions(+), 31 deletions(-)
 create mode 100644 grub-core/commands/hiddentimeout.c

diff --git a/ChangeLog b/ChangeLog
index d24f533..cfca08a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2013-11-27  Colin Watson  <address@hidden>
+
+       Add a new "hiddentimeout" command.  If this is used, then the menu
+       will use a hidden timeout much as was previously done with "sleep
+       --interruptible", except that pressing any hotkeys associated with
+       menu entries will boot the corresponding menu entry immediately.
+
 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..3b92105 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -1299,13 +1299,19 @@ immediately without displaying the menu, or to 
@samp{-1} to wait
 indefinitely.
 
 @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 for @key{ESC} or a hotkey associated with a menu
+entry to be pressed before displaying the menu.  If @key{ESC} is pressed,
+display the menu and wait for input according to @samp{GRUB_TIMEOUT}.  If a
+hotkey 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.
+
+We expect that most people who use @samp{GRUB_HIDDEN_TIMEOUT} will want to
+have @samp{GRUB_TIMEOUT} set to @samp{0} so that the menu is not displayed
+at all unless @key{ESC} is pressed.
+
+This option is unset by default.
 
 @item GRUB_HIDDEN_TIMEOUT_QUIET
 In conjunction with @samp{GRUB_HIDDEN_TIMEOUT}, set this to @samp{true} to
@@ -3736,6 +3742,7 @@ you forget a command, you can run the command 
@command{help}
 * halt::                        Shut down your computer
 * hashsum::                     Compute or check hash checksum
 * help::                        Show help messages
+* hiddentimeout::               Configure the hidden menu timeout
 * initrd::                      Load a Linux initrd
 * initrd16::                    Load a Linux initrd (16-bit mode)
 * insmod::                      Insert a module
@@ -4243,6 +4250,24 @@ about each of the commands whose names begin with those 
@var{patterns}.
 @end deffn
 
 
address@hidden hiddentimeout
address@hidden hiddentimeout
+
address@hidden Command hiddentimeout address@hidden timeout] timeout
+Configure the @samp{normal} module (@pxref{normal}) to wait @var{timeout}
+seconds for @key{ESC} or a hotkey associated with a menu entry to be pressed
+before displaying the menu.  If @key{ESC} is pressed, display the menu and
+wait for input according to the value of the @samp{timeout} variable
+(@pxref{timeout}).  If a hotkey 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 the @samp{timeout} variable
+before booting the default entry.
+
+If the @option{--visible-timeout} option is given, then configure the
address@hidden module to set the @samp{timeout} variable to its value if the
+timeout expires without any of the recognised keys being pressed.
address@hidden deffn
+
 @node initrd
 @subsection initrd
 
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 5cd84b1..41114a8 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -2247,3 +2247,8 @@ module = {
   name = progress;
   common = lib/progress.c;
 };
+
+module = {
+  name = hiddentimeout;
+  common = commands/hiddentimeout.c;
+};
diff --git a/grub-core/commands/hiddentimeout.c 
b/grub-core/commands/hiddentimeout.c
new file mode 100644
index 0000000..99fdaff
--- /dev/null
+++ b/grub-core/commands/hiddentimeout.c
@@ -0,0 +1,68 @@
+/* hiddentimeout.c - command to configure the hidden menu timeout  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2013  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/dl.h>
+#include <grub/misc.h>
+#include <grub/env.h>
+#include <grub/extcmd.h>
+#include <grub/i18n.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+static const struct grub_arg_option options[] =
+  {
+    {"verbose", 'v', 0, N_("Verbose countdown."), 0, 0},
+    {"visible-timeout", 0, 0,
+     N_("Timeout for the visible menu after the hidden timeout expires."),
+     0, ARG_TYPE_INT},
+    {0, 0, 0, 0, 0, 0}
+  };
+
+static grub_err_t
+grub_cmd_hiddentimeout (grub_extcmd_context_t ctxt, int argc, char **args)
+{
+  struct grub_arg_list *state = ctxt->state;
+
+  if (argc != 1)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument expected"));
+
+  grub_env_set ("hidden_timeout", args[0]);
+  grub_env_set ("hidden_timeout_verbose", state[0].set ? "1" : "0");
+
+  if (state[1].set)
+    grub_env_set ("visible_timeout", state[1].arg);
+
+  return GRUB_ERR_NONE;
+}
+
+static grub_extcmd_t cmd;
+
+GRUB_MOD_INIT(hiddentimeout)
+{
+  cmd = grub_register_extcmd ("hiddentimeout", grub_cmd_hiddentimeout, 0,
+                             N_("NUMBER_OF_SECONDS"),
+                             N_("Show hidden menu for a specified number of "
+                                "seconds."),
+                             options);
+}
+
+GRUB_MOD_FINI(hiddentimeout)
+{
+  grub_unregister_extcmd (cmd);
+}
diff --git a/grub-core/normal/menu.c b/grub-core/normal/menu.c
index 9b88290..cea8d47 100644
--- a/grub-core/normal/menu.c
+++ b/grub-core/normal/menu.c
@@ -70,6 +70,21 @@ 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 current timeout. If the variable "timeout" is not set or
    invalid, return -1.  */
 int
@@ -113,6 +128,46 @@ grub_menu_set_timeout (int timeout)
     }
 }
 
+/* Return the current hidden timeout.  If the variable "hidden_timeout" is
+   not set or invalid, return -1.  */
+static int
+get_hidden_timeout (void)
+{
+  const char *val;
+  int hidden_timeout;
+
+  val = grub_env_get ("hidden_timeout");
+  if (! val)
+    return -1;
+
+  grub_error_push ();
+
+  hidden_timeout = (int) grub_strtoul (val, 0, 0);
+
+  /* If the value is invalid, unset the variable.  */
+  if (grub_errno != GRUB_ERR_NONE)
+    {
+      grub_env_unset ("hidden_timeout");
+      grub_errno = GRUB_ERR_NONE;
+      hidden_timeout = -1;
+    }
+
+  grub_error_pop ();
+
+  return hidden_timeout;
+}
+
+/* Return 1 if the hidden timeout should show a verbose countdown, otherwise
+   0.  */
+static int
+get_hidden_timeout_verbose (void)
+{
+  const char *val;
+
+  val = grub_env_get ("hidden_timeout_verbose");
+  return val && grub_strtoul (val, 0, 0) != 0;
+}
+
 /* Get the first entry number from the value of the environment variable NAME,
    which is a space-separated list of non-negative integers.  The entry number
    which is returned is stripped from the value of NAME.  If no entry number
@@ -488,6 +543,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
@@ -501,7 +583,7 @@ run_menu (grub_menu_t menu, int nested, int *auto_boot)
 {
   grub_uint64_t saved_time;
   int default_entry, current_entry;
-  int timeout;
+  int hidden_timeout, timeout;
 
   default_entry = get_entry_number (menu, "default");
 
@@ -510,6 +592,65 @@ run_menu (grub_menu_t menu, int nested, int *auto_boot)
   if (default_entry < 0 || default_entry >= menu->size)
     default_entry = 0;
 
+  hidden_timeout = get_hidden_timeout ();
+  if (hidden_timeout != -1)
+    {
+      int verbose = get_hidden_timeout_verbose ();
+      static struct grub_term_coordinate *pos;
+      int entry = -1;
+
+      if (verbose && hidden_timeout)
+       {
+         pos = grub_term_save_pos ();
+         print_countdown (pos, hidden_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)
+           break;
+
+         if (hidden_timeout > 0 && has_second_elapsed (&saved_time))
+           {
+             hidden_timeout--;
+             if (verbose)
+               print_countdown (pos, hidden_timeout);
+           }
+
+         if (hidden_timeout == 0)
+           {
+             const char *visible_timeout;
+
+             visible_timeout = grub_env_get ("visible_timeout");
+             if (visible_timeout)
+               grub_env_set ("timeout", visible_timeout);
+             break;
+           }
+       }
+
+      /* Don't do this again.  */
+      grub_env_unset ("hidden_timeout");
+      grub_env_unset ("visible_timeout");
+
+      if (entry >= 0)
+       {
+         *auto_boot = 1;
+         return entry;
+       }
+    }
+
   /* If timeout is 0, drawing is pointless (and ugly).  */
   if (grub_menu_get_timeout () == 0)
     {
@@ -540,18 +681,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 +787,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.d/00_header.in b/util/grub.d/00_header.in
index 9838720..6d2f074 100644
--- a/util/grub.d/00_header.in
+++ b/util/grub.d/00_header.in
@@ -289,8 +289,10 @@ make_timeout ()
            verbose=" --verbose"
        fi
        cat << EOF
-if sleep$verbose --interruptible ${1} ; then
-  set timeout=${2}
+if ! hiddentimeout$verbose --visible-timeout=${2} ${1} ; then
+  if sleep$verbose --interruptible ${1} ; then
+    set timeout=${2}
+  fi
 fi
 EOF
     else
-- 
1.8.4.4

-- 
Colin Watson                                       address@hidden