[PATCH] fix flickering timeout message for slow terminals (gfxterm)

[PATCH] fix flickering timeout message for slow terminals (gfxterm)

[Robert Millan]

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


As subject says.  Based on suggestions from Vesa.

-- 
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: flickering_timeout.diff --]
[-- Type: text/x-diff, Size: 2060 bytes --]


	* normal/menu.c (run_menu): Move timeout message to a separate
	(local) function, print_timeout().  Use print_timeout() once during
	initial draw to print the whole message, and again in every clock
	tick to update only the number of seconds.

diff -x '*~' -x configure -x config.h.in -urp grub2/normal/menu.c test/normal/menu.c
--- grub2/normal/menu.c	2008-01-05 13:10:28.000000000 +0100
+++ test/normal/menu.c	2008-01-14 14:55:16.000000000 +0100
@@ -314,6 +314,20 @@ run_menu (grub_menu_t menu, int nested)
   int first, offset;
   unsigned long saved_time;
   int default_entry;
+  int timeout;
+
+  auto void print_timeout (int);
+  void print_timeout (int second_stage)
+    {
+      char *prelude = "   The highlighted entry will be booted automatically in";
+
+      grub_gotoxy (second_stage ? grub_strlen (prelude) : 0, GRUB_TERM_HEIGHT - 3);
+      /* NOTE: Do not remove the trailing space characters.
+         They are required to clear the line.  */
+      grub_printf ("%s %ds.    ", second_stage ? "" : prelude, timeout);
+      grub_gotoxy (GRUB_TERM_CURSOR_X, GRUB_TERM_FIRST_ENTRY_Y + offset);
+      grub_refresh ();
+    };
   
   first = 0;
   
@@ -340,11 +354,14 @@ run_menu (grub_menu_t menu, int nested)
   print_entries (menu, first, offset);
   grub_refresh ();
 
+  timeout = get_timeout ();
+
+  if (timeout > 0)
+    print_timeout (0);
+
   while (1)
     {
       int c;
-      int timeout;
-
       timeout = get_timeout ();
       
       if (timeout > 0)
@@ -357,16 +374,8 @@ run_menu (grub_menu_t menu, int nested)
 	      timeout--;
 	      set_timeout (timeout);
 	      saved_time = current_time;
+	      print_timeout (1);
 	    }
-	  
-	  grub_gotoxy (0, GRUB_TERM_HEIGHT - 3);
-	  /* NOTE: Do not remove the trailing space characters.
-	     They are required to clear the line.  */
-	  grub_printf ("\
-   The highlighted entry will be booted automatically in %d s.    ",
-		       timeout);
-	  grub_gotoxy (GRUB_TERM_CURSOR_X, GRUB_TERM_FIRST_ENTRY_Y + offset);
-	  grub_refresh ();
 	}
 
       if (timeout == 0)

Re: [PATCH] fix flickering timeout message for slow terminals (gfxterm)

[Jordi Mallach]

Hi Robert,

On Mon, Jan 14, 2008 at 03:10:15PM +0100, Robert Millan wrote:
> +      char *prelude = "   The highlighted entry will be booted automatically in";
> +      grub_printf ("%s %ds.    ", second_stage ? "" : prelude, timeout);

As you feared, this is not nice from a i18n POV. Concatenating strings
like this will make the message untranslatable for a number of
languages.

> -	  grub_printf ("\
> -   The highlighted entry will be booted automatically in %d s.    ",
> -		       timeout);

This one is ok, though.

Jordi
-- 
Jordi Mallach Pérez  --  Debian developer     http://www.debian.org/
jordi@sindominio.net     jordi@debian.org     http://www.sindominio.net/
GnuPG public key information available at http://oskuro.net/

Re: [PATCH] fix flickering timeout message for slow terminals (gfxterm)

[Yoshinori K. Okuji]

On Monday 14 January 2008 15:10, Robert Millan wrote:
> As subject says.  Based on suggestions from Vesa.

Why do you want to use a nested function?

Okuji

Re: [PATCH] fix flickering timeout message for slow terminals (gfxterm)

[Robert Millan]

On Mon, Jan 14, 2008 at 09:32:39PM +0100, Yoshinori K. Okuji wrote:
> On Monday 14 January 2008 15:10, Robert Millan wrote:
> > As subject says.  Based on suggestions from Vesa.
> 
> Why do you want to use a nested function?

No special reason;  just to restrict the namespace use to the scope where
it'll be needed, and avoid passing of parameters (offset, timeout).

I'm fine with moving it out if that's preferred.

-- 
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] fix flickering timeout message for slow terminals (gfxterm)

[Robert Millan]

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

On Mon, Jan 14, 2008 at 09:32:25PM +0100, Jordi Mallach wrote:
> Hi Robert,
> 
> On Mon, Jan 14, 2008 at 03:10:15PM +0100, Robert Millan wrote:
> > +      char *prelude = "   The highlighted entry will be booted automatically in";
> > +      grub_printf ("%s %ds.    ", second_stage ? "" : prelude, timeout);
> 
> As you feared, this is not nice from a i18n POV. Concatenating strings
> like this will make the message untranslatable for a number of
> languages.

Is this better?

-- 
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: flickering_timeout.diff --]
[-- Type: text/x-diff, Size: 2092 bytes --]


	* normal/menu.c (run_menu): Move timeout message to a separate
	(local) function, print_timeout().  Use print_timeout() once during
	initial draw to print the whole message, and again in every clock
	tick to update only the number of seconds.

diff -x '*~' -x configure -x config.h.in -urp grub2/normal/menu.c time/normal/menu.c
--- grub2/normal/menu.c	2008-01-05 13:10:28.000000000 +0100
+++ time/normal/menu.c	2008-01-15 00:18:12.000000000 +0100
@@ -314,6 +314,21 @@ run_menu (grub_menu_t menu, int nested)
   int first, offset;
   unsigned long saved_time;
   int default_entry;
+  int timeout;
+
+  auto void print_timeout (int);
+  void print_timeout (int second_stage)
+    {
+      /* NOTE: Do not remove the trailing space characters.
+         They are required to clear the line.  */
+      char *msg = "   The highlighted entry will be booted automatically in %ds.    ";
+      char *msg_end = grub_strchr (msg, '%');
+
+      grub_gotoxy (second_stage ? (msg_end - msg) : 0, GRUB_TERM_HEIGHT - 3);
+      grub_printf (second_stage ? msg_end : msg, timeout);
+      grub_gotoxy (GRUB_TERM_CURSOR_X, GRUB_TERM_FIRST_ENTRY_Y + offset);
+      grub_refresh ();
+    };
   
   first = 0;
   
@@ -340,11 +355,14 @@ run_menu (grub_menu_t menu, int nested)
   print_entries (menu, first, offset);
   grub_refresh ();
 
+  timeout = get_timeout ();
+
+  if (timeout > 0)
+    print_timeout (0);
+
   while (1)
     {
       int c;
-      int timeout;
-
       timeout = get_timeout ();
       
       if (timeout > 0)
@@ -357,16 +375,8 @@ run_menu (grub_menu_t menu, int nested)
 	      timeout--;
 	      set_timeout (timeout);
 	      saved_time = current_time;
+	      print_timeout (1);
 	    }
-	  
-	  grub_gotoxy (0, GRUB_TERM_HEIGHT - 3);
-	  /* NOTE: Do not remove the trailing space characters.
-	     They are required to clear the line.  */
-	  grub_printf ("\
-   The highlighted entry will be booted automatically in %d s.    ",
-		       timeout);
-	  grub_gotoxy (GRUB_TERM_CURSOR_X, GRUB_TERM_FIRST_ENTRY_Y + offset);
-	  grub_refresh ();
 	}
 
       if (timeout == 0)

Re: [PATCH] fix flickering timeout message for slow terminals (gfxterm)

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Mon, Jan 14, 2008 at 09:32:39PM +0100, Yoshinori K. Okuji wrote:
>> On Monday 14 January 2008 15:10, Robert Millan wrote:
>> > As subject says.  Based on suggestions from Vesa.
>> 
>> Why do you want to use a nested function?
>
> No special reason;  just to restrict the namespace use to the scope where
> it'll be needed, and avoid passing of parameters (offset, timeout).
>
> I'm fine with moving it out if that's preferred.

If it doesn't uglify your code, that's usually a better thing to do.

--
Marco

Re: [PATCH] fix flickering timeout message for slow terminals (gfxterm)

[Robert Millan]

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

On Tue, Jan 15, 2008 at 01:25:25PM +0100, Marco Gerards wrote:
> Robert Millan <rmh@aybabtu.com> writes:
> 
> > On Mon, Jan 14, 2008 at 09:32:39PM +0100, Yoshinori K. Okuji wrote:
> >> On Monday 14 January 2008 15:10, Robert Millan wrote:
> >> > As subject says.  Based on suggestions from Vesa.
> >> 
> >> Why do you want to use a nested function?
> >
> > No special reason;  just to restrict the namespace use to the scope where
> > it'll be needed, and avoid passing of parameters (offset, timeout).
> >
> > I'm fine with moving it out if that's preferred.
> 
> If it doesn't uglify your code, that's usually a better thing to do.

Ok, here's a new patch.

-- 
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: flickering_timeout.diff --]
[-- Type: text/x-diff, Size: 2051 bytes --]


	* normal/menu.c (run_menu): Move timeout message from here ...
	(print_timeout): ... to here.
	(run_menu): Use print_timeout() once during initial draw to print
	the whole message, and again in every clock tick to update only
	the number of seconds.

diff -x '*~' -x configure -x config.h.in -ur grub2/normal/menu.c flickery/normal/menu.c
--- grub2/normal/menu.c	2008-01-05 13:10:28.000000000 +0100
+++ flickery/normal/menu.c	2008-01-15 13:37:54.000000000 +0100
@@ -308,12 +308,27 @@
   return entry;
 }
 
+void
+print_timeout (int timeout, int offset, int second_stage)
+{
+  /* NOTE: Do not remove the trailing space characters.
+     They are required to clear the line.  */
+  char *msg = "   The highlighted entry will be booted automatically in %ds.    ";
+  char *msg_end = grub_strchr (msg, '%');
+  
+  grub_gotoxy (second_stage ? (msg_end - msg) : 0, GRUB_TERM_HEIGHT - 3);
+  grub_printf (second_stage ? msg_end : msg, timeout);
+  grub_gotoxy (GRUB_TERM_CURSOR_X, GRUB_TERM_FIRST_ENTRY_Y + offset);
+  grub_refresh ();
+};
+
 static int
 run_menu (grub_menu_t menu, int nested)
 {
   int first, offset;
   unsigned long saved_time;
   int default_entry;
+  int timeout;
   
   first = 0;
   
@@ -340,11 +355,14 @@
   print_entries (menu, first, offset);
   grub_refresh ();
 
+  timeout = get_timeout ();
+
+  if (timeout > 0)
+    print_timeout (timeout, offset, 0);
+
   while (1)
     {
       int c;
-      int timeout;
-
       timeout = get_timeout ();
       
       if (timeout > 0)
@@ -357,16 +375,8 @@
 	      timeout--;
 	      set_timeout (timeout);
 	      saved_time = current_time;
+	      print_timeout (timeout, offset, 1);
 	    }
-	  
-	  grub_gotoxy (0, GRUB_TERM_HEIGHT - 3);
-	  /* NOTE: Do not remove the trailing space characters.
-	     They are required to clear the line.  */
-	  grub_printf ("\
-   The highlighted entry will be booted automatically in %d s.    ",
-		       timeout);
-	  grub_gotoxy (GRUB_TERM_CURSOR_X, GRUB_TERM_FIRST_ENTRY_Y + offset);
-	  grub_refresh ();
 	}
 
       if (timeout == 0)

Re: [PATCH] fix flickering timeout message for slow terminals (gfxterm)

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Tue, Jan 15, 2008 at 01:25:25PM +0100, Marco Gerards wrote:
>> Robert Millan <rmh@aybabtu.com> writes:
>> 
>> > On Mon, Jan 14, 2008 at 09:32:39PM +0100, Yoshinori K. Okuji wrote:
>> >> On Monday 14 January 2008 15:10, Robert Millan wrote:
>> >> > As subject says.  Based on suggestions from Vesa.
>> >> 
>> >> Why do you want to use a nested function?
>> >
>> > No special reason;  just to restrict the namespace use to the scope where
>> > it'll be needed, and avoid passing of parameters (offset, timeout).
>> >
>> > I'm fine with moving it out if that's preferred.
>> 
>> If it doesn't uglify your code, that's usually a better thing to do.
>
> Ok, here's a new patch.

Here's a review :)

> -- 
> 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 /.)
>
>
> 	* normal/menu.c (run_menu): Move timeout message from here ...
> 	(print_timeout): ... to here.
> 	(run_menu): Use print_timeout() once during initial draw to print
> 	the whole message, and again in every clock tick to update only
> 	the number of seconds.
>
> diff -x '*~' -x configure -x config.h.in -ur grub2/normal/menu.c flickery/normal/menu.c
> --- grub2/normal/menu.c	2008-01-05 13:10:28.000000000 +0100
> +++ flickery/normal/menu.c	2008-01-15 13:37:54.000000000 +0100
> @@ -308,12 +308,27 @@
>    return entry;
>  }
>  
> +void
> +print_timeout (int timeout, int offset, int second_stage)

please make this function static.

Otherwise, I do not see any problems.

--
Marco

Re: [PATCH] fix flickering timeout message for slow terminals (gfxterm)

[Robert Millan]

On Tue, Jan 15, 2008 at 01:51:23PM +0100, Marco Gerards wrote:
> > +void
> > +print_timeout (int timeout, int offset, int second_stage)
> 
> please make this function static.
> 
> Otherwise, I do not see any problems.

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] fix flickering timeout message for slow terminals (gfxterm)

[Jordi Mallach]

On Tue, Jan 15, 2008 at 12:18:55AM +0100, Robert Millan wrote:
> Is this better?
> +      char *msg = "   The highlighted entry will be booted automatically in %ds.    ";
> +      char *msg_end = grub_strchr (msg, '%');
> +
> +      grub_printf (second_stage ? msg_end : msg, timeout);

Yes; however as discussed on IRC we'll eventually have to take into
account plural form handling. I need to discuss this with Danilo Segan
or Christian Perrier for this particular case.

Jordi
-- 
Jordi Mallach Pérez  --  Debian developer     http://www.debian.org/
jordi@sindominio.net     jordi@debian.org     http://www.sindominio.net/
GnuPG public key information available at http://oskuro.net/

Re: [PATCH] fix flickering timeout message for slow terminals (gfxterm)

[Marco Gerards]

Jordi Mallach <jordi@gnu.org> writes:

> On Tue, Jan 15, 2008 at 12:18:55AM +0100, Robert Millan wrote:
>> Is this better?
>> +      char *msg = "   The highlighted entry will be booted automatically in %ds.    ";
>> +      char *msg_end = grub_strchr (msg, '%');
>> +
>> +      grub_printf (second_stage ? msg_end : msg, timeout);
>
> Yes; however as discussed on IRC we'll eventually have to take into
> account plural form handling. I need to discuss this with Danilo Segan
> or Christian Perrier for this particular case.

Is gettext capable of this?

--
Marco

Re: [PATCH] fix flickering timeout message for slow terminals (gfxterm)

[Pavel Roskin]

On Mon, 2008-01-21 at 11:33 +0100, Marco Gerards wrote:
> Jordi Mallach <jordi@gnu.org> writes:
> 
> > On Tue, Jan 15, 2008 at 12:18:55AM +0100, Robert Millan wrote:
> >> Is this better?
> >> +      char *msg = "   The highlighted entry will be booted automatically in %ds.    ";
> >> +      char *msg_end = grub_strchr (msg, '%');
> >> +
> >> +      grub_printf (second_stage ? msg_end : msg, timeout);
> >
> > Yes; however as discussed on IRC we'll eventually have to take into
> > account plural form handling. I need to discuss this with Danilo Segan
> > or Christian Perrier for this particular case.
> 
> Is gettext capable of this?

Yes.  It's very good at it.  But I don't know how heavy the runtime part
is going to be.  I don't think we can just link a library into GRUB, so
perhaps it will need to be copied to the GRUB sources and stripped down.

But I still think it would be an overkill.  Even the Linux kernel
doesn't offer any translation for its boot messages.

And if we go with the translation, I think it should be possible for any
language to rephrase messages in a way that would not require plural
handling.

-- 
Regards,
Pavel Roskin

Re: [PATCH] fix flickering timeout message for slow terminals (gfxterm)

[Robert Millan]

On Mon, Jan 21, 2008 at 09:18:19AM -0500, Pavel Roskin wrote:
> 
> But I still think it would be an overkill.  Even the Linux kernel
> doesn't offer any translation for its boot messages.

But Linux has no user interface.  Well, other than cmdline params, dmesg
and such, but no real user interface that the majority of users would be
exposed to.

-- 
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] fix flickering timeout message for slow terminals (gfxterm)

[Marco Gerards]

Pavel Roskin <proski@gnu.org> writes:

> On Mon, 2008-01-21 at 11:33 +0100, Marco Gerards wrote:
>> Jordi Mallach <jordi@gnu.org> writes:
>> 
>> > On Tue, Jan 15, 2008 at 12:18:55AM +0100, Robert Millan wrote:
>> >> Is this better?
>> >> +      char *msg = "   The highlighted entry will be booted automatically in %ds.    ";
>> >> +      char *msg_end = grub_strchr (msg, '%');
>> >> +
>> >> +      grub_printf (second_stage ? msg_end : msg, timeout);
>> >
>> > Yes; however as discussed on IRC we'll eventually have to take into
>> > account plural form handling. I need to discuss this with Danilo Segan
>> > or Christian Perrier for this particular case.
>> 
>> Is gettext capable of this?
>
> Yes.  It's very good at it.  But I don't know how heavy the runtime part
> is going to be.  I don't think we can just link a library into GRUB, so
> perhaps it will need to be copied to the GRUB sources and stripped down.
>
> But I still think it would be an overkill.  Even the Linux kernel
> doesn't offer any translation for its boot messages.
>
> And if we go with the translation, I think it should be possible for any
> language to rephrase messages in a way that would not require plural
> handling.

If done efficiently, code shouldn't get too big.  Perhaps it is better
to integrate this into normal.mod?  I wouldn't mind a few extra KB
there for such a feature.

--
Marco

Re: [PATCH] fix flickering timeout message for slow terminals (gfxterm)

[Robert Millan]

On Mon, Jan 21, 2008 at 04:10:37PM +0100, Marco Gerards wrote:
> 
> If done efficiently, code shouldn't get too big.  Perhaps it is better
> to integrate this into normal.mod?  I wouldn't mind a few extra KB
> there for such a feature.

I don't think code size matters that much for a non-kernel feature.  Anyway,
I'd still prefer not bloating normal.mod with it.  It's very good that users
have choice of which features they want;  why not preserve that philosophy?

-- 
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] fix flickering timeout message for slow terminals (gfxterm)

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Mon, Jan 21, 2008 at 04:10:37PM +0100, Marco Gerards wrote:
>> 
>> If done efficiently, code shouldn't get too big.  Perhaps it is better
>> to integrate this into normal.mod?  I wouldn't mind a few extra KB
>> there for such a feature.
>
> I don't think code size matters that much for a non-kernel feature.  Anyway,
> I'd still prefer not bloating normal.mod with it.  It's very good that users
> have choice of which features they want;  why not preserve that philosophy?

Ok, agreed.  You are thinking of a gettext.mod?

--
Marco

Re: [PATCH] fix flickering timeout message for slow terminals (gfxterm)

[Robert Millan]

On Mon, Jan 21, 2008 at 05:52:50PM +0100, Marco Gerards wrote:
> Robert Millan <rmh@aybabtu.com> writes:
> 
> > On Mon, Jan 21, 2008 at 04:10:37PM +0100, Marco Gerards wrote:
> >> 
> >> If done efficiently, code shouldn't get too big.  Perhaps it is better
> >> to integrate this into normal.mod?  I wouldn't mind a few extra KB
> >> there for such a feature.
> >
> > I don't think code size matters that much for a non-kernel feature.  Anyway,
> > I'd still prefer not bloating normal.mod with it.  It's very good that users
> > have choice of which features they want;  why not preserve that philosophy?
> 
> Ok, agreed.  You are thinking of a gettext.mod?

Of course :-)

-- 
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 /.)