fix flickering timeout message for slow terminals (gfxterm)
PATCH 1/1 fix flickering timeout message for slow terminals (gfxterm)
As subject says. Based on suggestions from Vesa.
Signature · 5 lines
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 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.
normal/menu.c +20 -11
@@ -314,6 +314,20 @@ run_menu (grub_menu_t menu, int nested)int first, offset;unsigned long saved_time;int default_entry; +int timeout; + + autovoid 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 )
Quoted 1 line · Robert Millan
As subject says. Based on suggestions from Vesa.
Why do you want to use a nested function?
Okuji
Quoted 4 lines · Yoshinori K. Okuji
Quoted 1 line · Robert Millan
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.
Signature · 5 lines
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 /.)
Quoted 10 lines · Robert Millan
Quoted 4 lines · Yoshinori K. Okuji
Quoted 1 line · Robert Millan
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
Quoted 6 lines · Marco Gerards
Quoted 4 lines · Robert Millan
Quoted 3 lines · Yoshinori K. Okuji
Quoted 1 line · Robert Millan
As subject says. Based on suggestions from Vesa.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
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.
Signature · 5 lines
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.
normal/menu.c +21 -11
@@ -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 )
Quoted 9 lines · Robert Millan
Quoted 8 lines · Marco Gerards
Quoted 6 lines · Robert Millan
Quoted 4 lines · Yoshinori K. Okuji
Quoted 1 line · Robert Millan
As subject says. Based on suggestions from Vesa.
Why do you want to use a nested function?
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
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 :)
Quoted 18 lines
Signature · 14 lines
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
normal/menu.c line 312
Quoted 5 lines
normal/menu.c +2
-0@@ -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
Quoted 6 lines · Marco Gerards
normal/menu.c line 312
Quoted 2 lines
normal/menu.c +2
-0+void +print_timeout (int timeout,int offset,int second_stage)please make this function static.
Otherwise, I do not see any problems.
Committed.
Signature · 5 lines
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 /.)
Hi Robert,
normal/menu.c run_menu (grub_menu_t menu, int nested) · line 327
Quoted 2 lines · Robert Millan
normal/menu.c +2
-0+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.
normal/menu.c run_menu (grub_menu_t menu, int nested) · line 367
Quoted 3 lines
normal/menu.c +0
-3-grub_printf ("\ -The highlighted entry will be booted automatically in %d s. " , - timeout);
This one is ok, though.
Jordi
Signature · 3 lines
Jordi Mallach Pérez -- Debian developer http://www.debian.org/ [email protected] [email protected] http://www.sindominio.net/ GnuPG public key information available at http://oskuro.net/
Quoted 9 lines · Jordi Mallach
Hi Robert,
Review
Quoted 2 lines · Robert Millan
(quoted diff) +2
-0+ 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?
Signature · 5 lines
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 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.
normal/menu.c +21 -11
@@ -314,6 +314,21 @@ run_menu (grub_menu_t menu, int nested)int first, offset;unsigned long saved_time;int default_entry; +int timeout; + + autovoid 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 )
normal/menu.c run_menu (grub_menu_t menu, int nested) · line 328
Quoted 5 lines · Robert Millan
Is this better?
normal/menu.c +4
-0+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
Signature · 3 lines
Jordi Mallach Pérez -- Debian developer http://www.debian.org/ [email protected] [email protected] http://www.sindominio.net/ GnuPG public key information available at http://oskuro.net/
Quoted 10 lines · Jordi Mallach
normal/menu.c run_menu (grub_menu_t menu, int nested) · line 328
Quoted 5 lines · Robert Millan
Is this better?
normal/menu.c +4
-0+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
Quoted 9 lines · Marco Gerards
Quoted 7 lines · Jordi Mallach
Quoted 5 lines · Robert Millan
Is this better?
normal/menu.c +4
-0+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);
Quoted 5 lines
Quoted 3 lines
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.
Signature · 2 lines
Regards, Pavel Roskin
Quoted 3 lines · Pavel Roskin
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.
Signature · 5 lines
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 /.)
Quoted 20 lines · Pavel Roskin
Quoted 9 lines · Marco Gerards
Quoted 7 lines · Jordi Mallach
Quoted 5 lines · Robert Millan
Is this better?
normal/menu.c +4
-0+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);Quoted 5 lines
Quoted 3 lines
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.
Quoted 6 lines
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
Quoted 4 lines · Marco Gerards
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?
Signature · 5 lines
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 /.)
Quoted 9 lines · Robert Millan
Quoted 4 lines · Marco Gerards
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
Quoted 8 lines · Marco Gerards
Quoted 6 lines · Robert Millan
Quoted 4 lines · Marco Gerards
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.
Quoted 5 lines
Quoted 3 lines
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 :-)
Signature · 5 lines
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 /.)