[PATCH] if timeout is set to zero, don't bother drawing the menu

[PATCH] if timeout is set to zero, don't bother drawing the menu

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]


Fixes an ugly result of setting timeout=0.

When this is combined with the "sleep" patch I just sent, user can implement
hiddenmenu-like functionality via scripting:

echo -n "Press `ESC' to enter the menu... "
if sleep -v -i 10 ; then
  set timeout=10
else
  set timeout=0
fi

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

[-- Attachment #2: zero_timeout.diff --]
[-- Type: text/x-diff, Size: 649 bytes --]


	* normal/menu.c (run_menu): If timeout is set to zero, don't bother
	drawing the menu.

diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/normal/menu.c ./normal/menu.c
--- ../grub2/normal/menu.c	2008-01-15 15:05:40.000000000 +0100
+++ ./normal/menu.c	2008-02-08 17:11:41.000000000 +0100
@@ -339,6 +339,10 @@ run_menu (grub_menu_t menu, int nested)
   if (default_entry < 0 || default_entry >= menu->size)
     default_entry = 0;
 
+  /* If timeout is 0, drawing is pointless (and ugly).  */
+  if (get_timeout () == 0)
+    return default_entry;
+
   offset = default_entry;
   if (offset > GRUB_TERM_NUM_ENTRIES - 1)
     {

Re: [PATCH] if timeout is set to zero, don't bother drawing the menu

[Robert Millan]

Committed.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] if timeout is set to zero, don't bother drawing the menu

[Yoshinori K. Okuji]

On Friday 08 February 2008 17:18, Robert Millan wrote:
> Fixes an ugly result of setting timeout=0.
>
> When this is combined with the "sleep" patch I just sent, user can
> implement hiddenmenu-like functionality via scripting:
>
> echo -n "Press `ESC' to enter the menu... "
> if sleep -v -i 10 ; then
>   set timeout=10
> else
>   set timeout=0
> fi

I think this is a bit strange. I don't think anybody expects that "sleep" 
outputs a counter or is interrupted by a key input. It is too different from 
POSIX's sleep.

My preference is rather to extend "read". Bash's "read" accepts a timeout and 
some other options.

Okuji

Re: [PATCH] if timeout is set to zero, don't bother drawing the menu

[Robert Millan]

On Sat, Feb 09, 2008 at 09:03:09PM +0100, Yoshinori K. Okuji wrote:
> On Friday 08 February 2008 17:18, Robert Millan wrote:
> > Fixes an ugly result of setting timeout=0.
> >
> > When this is combined with the "sleep" patch I just sent, user can
> > implement hiddenmenu-like functionality via scripting:
> >
> > echo -n "Press `ESC' to enter the menu... "
> > if sleep -v -i 10 ; then
> >   set timeout=10
> > else
> >   set timeout=0
> > fi
> 
> I think this is a bit strange. I don't think anybody expects that "sleep" 
> outputs a counter or is interrupted by a key input. It is too different from 
> POSIX's sleep.
> 
> My preference is rather to extend "read". Bash's "read" accepts a timeout and 
> some other options.

Ok, so you suggest:

  read --timeout N --verbose --interruptible

  ?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] if timeout is set to zero, don't bother drawing the menu

[Yoshinori K. Okuji]

On Saturday 09 February 2008 23:09, Robert Millan wrote:
> On Sat, Feb 09, 2008 at 09:03:09PM +0100, Yoshinori K. Okuji wrote:
> > On Friday 08 February 2008 17:18, Robert Millan wrote:
> > > Fixes an ugly result of setting timeout=0.
> > >
> > > When this is combined with the "sleep" patch I just sent, user can
> > > implement hiddenmenu-like functionality via scripting:
> > >
> > > echo -n "Press `ESC' to enter the menu... "
> > > if sleep -v -i 10 ; then
> > >   set timeout=10
> > > else
> > >   set timeout=0
> > > fi
> >
> > I think this is a bit strange. I don't think anybody expects that "sleep"
> > outputs a counter or is interrupted by a key input. It is too different
> > from POSIX's sleep.
> >
> > My preference is rather to extend "read". Bash's "read" accepts a timeout
> > and some other options.
>
> Ok, so you suggest:
>
>   read --timeout N --verbose --interruptible
>
>   ?

"read" exits when an input is passed by default, so "interruptible" sounds 
odd. Here, I think what you want is a behavior that "read" does not wait for 
ENTER, but reads immediately any character (or only ESC).

In GRUB Legacy, the hidden menu uses ESC, because if it uses ENTER, and the 
user pushes ENTER too long, and a key repeat occurs, then the first item may 
be selected quick as a flash. But we could avoid this problem by vacuuming 
all inputs before showing a menu, so it might be acceptable to use ENTER 
instead.

(Actually, this should be implemented, regardless of this issue, because some 
BIOSes may leave garbage in a key buffer. I don't remember if this has been 
done for GRUB 2.)

To count down, I don't think it should be named "--verbose". For me, this is 
like "--print-timeout".

Okuji

Re: [PATCH] if timeout is set to zero, don't bother drawing the menu

[Robert Millan]

On Sun, Feb 10, 2008 at 08:10:13AM +0100, Yoshinori K. Okuji wrote:
> On Saturday 09 February 2008 23:09, Robert Millan wrote:
> > On Sat, Feb 09, 2008 at 09:03:09PM +0100, Yoshinori K. Okuji wrote:
> > > On Friday 08 February 2008 17:18, Robert Millan wrote:
> > > > Fixes an ugly result of setting timeout=0.
> > > >
> > > > When this is combined with the "sleep" patch I just sent, user can
> > > > implement hiddenmenu-like functionality via scripting:
> > > >
> > > > echo -n "Press `ESC' to enter the menu... "
> > > > if sleep -v -i 10 ; then
> > > >   set timeout=10
> > > > else
> > > >   set timeout=0
> > > > fi
> > >
> > > I think this is a bit strange. I don't think anybody expects that "sleep"
> > > outputs a counter or is interrupted by a key input. It is too different
> > > from POSIX's sleep.
> > >
> > > My preference is rather to extend "read". Bash's "read" accepts a timeout
> > > and some other options.
> >
> > Ok, so you suggest:
> >
> >   read --timeout N --verbose --interruptible
> >
> >   ?
> 
> "read" exits when an input is passed by default, so "interruptible" sounds 
> odd. Here, I think what you want is a behavior that "read" does not wait for 
> ENTER, but reads immediately any character (or only ESC).

We could make the behaviour be, that if ESC is pressed at any time before an
input line is completed, the operation is aborted, read returns non-zero, and
it doesn't save the line to any variable.  What do you think?

> In GRUB Legacy, the hidden menu uses ESC, because if it uses ENTER, and the 
> user pushes ENTER too long, and a key repeat occurs, then the first item may 
> be selected quick as a flash. But we could avoid this problem by vacuuming 
> all inputs before showing a menu, so it might be acceptable to use ENTER 
> instead.

This is still error-prone.  Even if you vacuum the input buffer, we still have
a race between user unholding ENTER and GRUB showing the menu.  Vacuuming the
buffer just gives us more time, but doesn't garantee good results.

> (Actually, this should be implemented, regardless of this issue, because some 
> BIOSes may leave garbage in a key buffer. I don't remember if this has been 
> done for GRUB 2.)

You mean garbage is present when control is transfered to GRUB, or generated
afterwards?  I assume the first, but then if any user input is attempted
before removing the garbage, it may be corrupted.  So it should be done as
early as possible, shouldn't it?  Perhaps in grub_console_init()?

> To count down, I don't think it should be named "--verbose". For me, this is 
> like "--print-timeout".

Ok.

I had another patch for read already, for an unrelated extension.  If you
don't mind, I'll send that in first, since it's already implemented.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] if timeout is set to zero, don't bother drawing the menu

[Yoshinori K. Okuji]

On Sunday 10 February 2008 13:55, Robert Millan wrote:
> We could make the behaviour be, that if ESC is pressed at any time before
> an input line is completed, the operation is aborted, read returns
> non-zero, and it doesn't save the line to any variable.  What do you think?

Not bad. :)

> > In GRUB Legacy, the hidden menu uses ESC, because if it uses ENTER, and
> > the user pushes ENTER too long, and a key repeat occurs, then the first
> > item may be selected quick as a flash. But we could avoid this problem by
> > vacuuming all inputs before showing a menu, so it might be acceptable to
> > use ENTER instead.
>
> This is still error-prone.  Even if you vacuum the input buffer, we still
> have a race between user unholding ENTER and GRUB showing the menu. 
> Vacuuming the buffer just gives us more time, but doesn't garantee good
> results.

Sure, but I think it would be good enough in reality.

> > (Actually, this should be implemented, regardless of this issue, because
> > some BIOSes may leave garbage in a key buffer. I don't remember if this
> > has been done for GRUB 2.)
>
> You mean garbage is present when control is transfered to GRUB, or
> generated afterwards?  I assume the first, but then if any user input is
> attempted before removing the garbage, it may be corrupted.  So it should
> be done as early as possible, shouldn't it?  Perhaps in
> grub_console_init()?

Maybe. Personally, I don't mind even if it is done at the startup.

Okuji

Re: [PATCH] if timeout is set to zero, don't bother drawing the menu

[Isaac Dupree]

Yoshinori K. Okuji wrote:
> On Sunday 10 February 2008 13:55, Robert Millan wrote:
>> We could make the behaviour be, that if ESC is pressed at any time before
>> an input line is completed, the operation is aborted, read returns
>> non-zero, and it doesn't save the line to any variable.  What do you think?
> 
> Not bad. :)

:)

>>> In GRUB Legacy, the hidden menu uses ESC, because if it uses ENTER, and
>>> the user pushes ENTER too long, and a key repeat occurs, then the first
>>> item may be selected quick as a flash. But we could avoid this problem by
>>> vacuuming all inputs before showing a menu, so it might be acceptable to
>>> use ENTER instead.
>> This is still error-prone.  Even if you vacuum the input buffer, we still
>> have a race between user unholding ENTER and GRUB showing the menu. 
>> Vacuuming the buffer just gives us more time, but doesn't garantee good
>> results.
> 
> Sure, but I think it would be good enough in reality.

most realities.  Probably it will bite someone some day.

A 1/10th second delay or so after display before listening for input 
might reduce the chances of that, though increase the very same chances 
of the enter-press for the menu being intentional but dropped.

-Isaac

Re: [PATCH] if timeout is set to zero, don't bother drawing the menu

[Robert Millan]

On Sat, Feb 09, 2008 at 09:03:09PM +0100, Yoshinori K. Okuji wrote:
> On Friday 08 February 2008 17:18, Robert Millan wrote:
> > Fixes an ugly result of setting timeout=0.
> >
> > When this is combined with the "sleep" patch I just sent, user can
> > implement hiddenmenu-like functionality via scripting:
> >
> > echo -n "Press `ESC' to enter the menu... "
> > if sleep -v -i 10 ; then
> >   set timeout=10
> > else
> >   set timeout=0
> > fi
> 
> I think this is a bit strange. I don't think anybody expects that "sleep" 
> outputs a counter or is interrupted by a key input. It is too different from 
> POSIX's sleep.
> 
> My preference is rather to extend "read". Bash's "read" accepts a timeout and 
> some other options.

I started implementing this, and then noticed that it is a bit odd that read
prints timeout at the same time it is trying to obtain input from the user.

Timeout printing uses gotoxy() and erases current line.  Don't you think it'd
look annoying?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] if timeout is set to zero, don't bother drawing the menu

[Yoshinori K. Okuji]

On Thursday 28 February 2008 11:57, Robert Millan wrote:
> On Sat, Feb 09, 2008 at 09:03:09PM +0100, Yoshinori K. Okuji wrote:
> > On Friday 08 February 2008 17:18, Robert Millan wrote:
> > > Fixes an ugly result of setting timeout=0.
> > >
> > > When this is combined with the "sleep" patch I just sent, user can
> > > implement hiddenmenu-like functionality via scripting:
> > >
> > > echo -n "Press `ESC' to enter the menu... "
> > > if sleep -v -i 10 ; then
> > >   set timeout=10
> > > else
> > >   set timeout=0
> > > fi
> >
> > I think this is a bit strange. I don't think anybody expects that "sleep"
> > outputs a counter or is interrupted by a key input. It is too different
> > from POSIX's sleep.
> >
> > My preference is rather to extend "read". Bash's "read" accepts a timeout
> > and some other options.
>
> I started implementing this, and then noticed that it is a bit odd that
> read prints timeout at the same time it is trying to obtain input from the
> user.
>
> Timeout printing uses gotoxy() and erases current line.  Don't you think
> it'd look annoying?

Maybe. But we don't care about the character entered by the user in this case, 
so we could simply disable echoing.

Okuji

Re: [PATCH] if timeout is set to zero, don't bother drawing the menu

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 284 bytes --]


Okay here's a preliminar patch.  We still need to define in which ways
can the user interrupt it.  What do we do on '\n' ?  and on ESC ?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

[-- Attachment #2: timeout.diff --]
[-- Type: text/x-diff, Size: 2929 bytes --]

diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/commands/read.c ./commands/read.c
--- ../grub2/commands/read.c	2008-02-02 21:35:08.000000000 +0100
+++ ./commands/read.c	2008-03-03 23:13:24.000000000 +0100
@@ -23,13 +23,39 @@
 #include <grub/normal.h>
 #include <grub/term.h>
 #include <grub/types.h>
+#include <grub/machine/time.h>
+
+static const struct grub_arg_option options[] =
+  {
+    {"timeout", 't', 0, "timeout (in seconds)", 0, ARG_TYPE_INT},
+    {"print-timeout", 'p', 0, "print timeout counter", 0, 0},
+    {0, 0, 0, 0, 0, 0}
+  };
+
+static grub_uint8_t x, y;
+
+static void
+do_print (int n)
+{
+  grub_gotoxy (x, y);
+  /* NOTE: Do not remove the trailing space characters.
+     They are required to clear the line.  */
+  grub_printf ("%d    ", n);
+}
 
 static char *
-grub_getline (void)
+grub_getline (int timeout, int print_timeout)
 {
   int i;
   char *line;
   char *tmp;
+  grub_uint32_t end_at, print_at;
+
+  if (timeout)
+    end_at = grub_get_rtc () + timeout * GRUB_TICKS_PER_SECOND;
+
+  if (print_timeout)
+    print_at = grub_get_rtc ();
 
   i = 0;
   line = grub_malloc (1 + i + sizeof('\0'));
@@ -38,27 +64,53 @@ grub_getline (void)
 
   while ((line[i - 1] != '\n') && (line[i - 1] != '\r'))
     {
+      /* If a timeout was requested, perform busy wait here so we can do other
+	 things...  */
+      while (timeout && (grub_checkkey () == -1))
+	{
+	  /* ...such as printing the timeout, if applicable.  */
+	  if (print_timeout && grub_get_rtc () > print_at)
+	    {
+	      do_print (grub_div_roundup (end_at - grub_get_rtc (), GRUB_TICKS_PER_SECOND));
+	      print_at += GRUB_TICKS_PER_SECOND;
+	    }
+	  /* ...or aborting when time is up.  */
+	  if (grub_get_rtc () > end_at)
+	    goto abort;
+	}
+
       line[i] = grub_getkey ();
       if (grub_isprint (line[i]))
 	grub_putchar (line[i]);
       i++;
+
       tmp = grub_realloc (line, 1 + i + sizeof('\0'));
       if (! tmp)
-	{
-	  grub_free (line);
-	  return NULL;
-	}
+	goto abort;
       line = tmp;
     }
   line[i] = '\0';
 
   return line;
+
+abort:
+  if (print_timeout)
+    do_print (0);
+  grub_free (line);
+  return NULL;
 }
 
 static grub_err_t
-grub_cmd_read (struct grub_arg_list *state UNUSED, int argc, char **args)
+grub_cmd_read (struct grub_arg_list *state, int argc, char **args)
 {
-  char *line = grub_getline ();
+  char *line;
+  grub_uint16_t xy;
+
+  xy = grub_getxy ();
+  x = xy >> 8;
+  y = xy & 0xff;
+
+  line = grub_getline ((state[0].set) ? grub_strtoul (state[0].arg, 0, 10) : 0, state[1].set);
   if (! line)
     return grub_errno;
   if (argc > 0)
@@ -72,7 +124,7 @@ grub_cmd_read (struct grub_arg_list *sta
 GRUB_MOD_INIT(read)
 {
   grub_register_command ("read", grub_cmd_read, GRUB_COMMAND_FLAG_CMDLINE,
-			 "read [ENVVAR]", "Set variable with user input", 0);
+			 "read [ENVVAR]", "Set variable with user input", options);
 }
 
 GRUB_MOD_FINI(read)