allow user-configurable menucolor
PATCH 1/1 allow user-configurable menucolor
This patch allows the following to work:
set color_normal=cyan/blue
set color_highlight=white/blue
which is equivalent to this command in GRUB Legacy:
color cyan/blue white/blue
I haven't written a ChangeLog entry yet, because I'd like to receive comments on the function names. I don't really like parse_single_color_name' and parse_color_name' but I don't know what to call them :-(. What conventions are there for referring to single colors (i.e. those that describe only FG or only BG, not both) and complete colors (FG + BG) ?
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 +84 -13
@@ -1,6 +1,6 @@/* * GRUB -- GRand Unified Bootloader -* Copyright (C) 2003,2004,2005,2006,2007 Free Software Foundation, Inc. +* Copyright (C) 2003,2004,2005,2006,2007,2008 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
@@ -28,6 +28,76 @@#define GRUB_COLOR_MENU_NORMAL 0x07#define GRUB_COLOR_MENU_HIGHLIGHT 0x70 +static grub_uint8_t grub_color_menu_normal =GRUB_COLOR_MENU_NORMAL ; +static grub_uint8_t grub_color_menu_highlight =GRUB_COLOR_MENU_HIGHLIGHT ; + +static void +wait_after_message (void ) +{ +/* Wait until the user pushes any key so that the user +can see what happened. */ +grub_printf ("\nPress any key to continue..." ); + (void )grub_getkey (); +} + +/* Borrowed from GRUB Legacy */ +static char *color_list[16 ] = +{ +"black" , +"blue" , +"green" , +"cyan" , +"red" , +"magenta" , +"brown" , +"light-gray" , +"dark-gray" , +"light-blue" , +"light-green" , +"light-cyan" , +"light-red" , +"light-magenta" , +"yellow" , +"white" +}; + +static grub_uint8_t +parse_single_color_name (char *name) +{ +grub_uint8_t i; +for (i =0 ; i <0x10 ; i++) +if (!grub_strcmp (name, color_list[i])) +break ; +return i; +} + +static grub_uint8_t +parse_color_name (char *name) +{ +grub_uint8_t ret_fg, ret_bg; +char *fg, *bg; + +/* not specified by user */ +if (name ==NULL ) +return GRUB_COLOR_MENU_NORMAL ; + + fg =grub_strdup (name); + bg =grub_strchr (fg,'/' ); + *(bg++) ='\0' ; + ret_fg =parse_single_color_name (fg); + ret_bg =parse_single_color_name (bg); +grub_free (fg); + +if ((ret_fg | ret_bg) >0x0f ) + { +grub_printf ("Warning: invalid color name, reverting to defaults\n" ); +wait_after_message (); +return GRUB_COLOR_MENU_NORMAL ; + } +else +return (ret_bg <<4 ) | ret_fg; +} +static void draw_border (void ) {
@@ -108,7 +178,7 @@grub_ssize_t len;grub_uint32_t *unicode_title;grub_ssize_t i; -grub_uint8_t normal_code, highlight_code; +grub_uint8_t old_color_normal, old_color_highlight; title = entry ? entry->title :"" ; unicode_title =grub_malloc (grub_strlen (title) *sizeof (*unicode_title));
@@ -125,8 +195,8 @@return ; } -grub_getcolor (&normal_code, &highlight_code); -grub_setcolor (GRUB_COLOR_MENU_NORMAL ,GRUB_COLOR_MENU_HIGHLIGHT ); +grub_getcolor (&old_color_normal, &old_color_highlight); +grub_setcolor (grub_color_menu_normal, grub_color_menu_highlight);grub_setcolorstate (highlight ?GRUB_TERM_COLOR_HIGHLIGHT :GRUB_TERM_COLOR_NORMAL );
@@ -164,7 +234,7 @@grub_gotoxy (GRUB_TERM_CURSOR_X , y); -grub_setcolor (normal_code, highlight_code); +grub_setcolor (old_color_normal, old_color_highlight);grub_setcolorstate (GRUB_TERM_COLOR_STANDARD );grub_free (unicode_title); }
@@ -209,15 +279,19 @@void grub_menu_init_page (int nested,int edit) { -grub_uint8_t normal_code, highlight_code; -grub_getcolor (&normal_code, &highlight_code); -grub_setcolor (GRUB_COLOR_MENU_NORMAL ,GRUB_COLOR_MENU_HIGHLIGHT ); +grub_uint8_t old_color_normal, old_color_highlight; + + grub_color_menu_normal =parse_color_name (grub_env_get ("color_normal" )); + grub_color_menu_highlight =parse_color_name (grub_env_get ("color_highlight" )); + +grub_getcolor (&old_color_normal, &old_color_highlight); +grub_setcolor (grub_color_menu_normal, grub_color_menu_highlight);grub_normal_init_page ();draw_border ();print_message (nested, edit); -grub_setcolor (normal_code, highlight_code); +grub_setcolor (old_color_normal, old_color_highlight); }/* Return the current timeout. If the variable "timeout" is not set or
@@ -501,10 +575,7 @@grub_print_error (); grub_errno =GRUB_ERR_NONE ; -/* Wait until the user pushes any key so that the user -can see what happened. */ -grub_printf ("\nPress any key to continue..." ); - (void )grub_getkey (); +wait_after_message (); } } }
Quoted 7 lines · Robert Millan
This patch allows the following to work:
set color_normal=cyan/blue
set color_highlight=white/bluewhich is equivalent to this command in GRUB Legacy:
Quoted 7 lines
color cyan/blue white/blue
I haven't written a ChangeLog entry yet, because I'd like to receive comments on the function names. I don't really like
parse_single_color_name' andparse_color_name' but I don't know what to call them :-(. What conventions are there for referring to single colors (i.e. those that describe only FG or only BG, not both) and complete colors (FG + BG) ?
First some questions ;)
- What happens when you forgot to provide / in this string ;) ?
- What happens when all memory is used?
- What happens when you alter list of colors... remove one entry of it or add one. (eg. hard coding is bad... sizeof...)
Now to your naming issue...
This code is quite specific to be only local so I would propose following:
parse_single_color_name -> parse_color_name
parse_color_name -> parse_color_tuple, parse_color_pair, parse_text_color or parse_menu_color
Ideally these would be superseded by theming but should work as temporary code until this feature is implemented. That being said... I would be too concerned about its naming :)
Quoted 4 lines · Vesa Jääskeläinen
First some questions ;)
- What happens when you forgot to provide / in this string ;) ?
grub_strchr() returns NULL and... whoops! I'll fix that :-)
Quoted 1 line
- What happens when all memory is used?
What do you mean?
Quoted 2 lines
- What happens when you alter list of colors... remove one entry of it or add one.
This should never happen. The list is just a human description of GRUB color codes. GRUB VGA-style color codes are never changed in runtime, so neither should this list.
Quoted 1 line
(eg. hard coding is bad... sizeof...)
Where can sizeof be used?
Quoted 8 lines
Now to your naming issue...
This code is quite specific to be only local so I would propose following:
parse_single_color_name->parse_color_name
parse_color_name->parse_color_tuple,parse_color_pair,parse_text_colororparse_menu_color
How about `parse_color_name_pair' ?
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 3 lines · Vesa Jääskeläinen
First some questions ;)
- What happens when you forgot to provide / in this string ;) ?
grub_strchr()returns NULL and... whoops! I'll fix that :-)Quoted 1 line
- What happens when all memory is used?
What do you mean?
strdup() == NULL
Quoted 6 lines
Quoted 2 lines
- What happens when you alter list of colors... remove one entry of it or add one.
This should never happen. The list is just a human description of GRUB color codes. GRUB VGA-style color codes are never changed in runtime, so neither should this list.
Yes, but... (below)
Quoted 3 lines
Quoted 1 line
(eg. hard coding is bad... sizeof...)
Where can sizeof be used?
for (i = 0; i < 0x10; i++)
->
for (i = 0; i < sizeof(color_list) / sizeof(*color_list); i++)
or similar compile time calculation for it. If you are using construct like this then your code always indexes array correctly. This of course requires that color_list is local to source file being compiled. Point being taken here is that never use hard-coded indexes to some arrays when there is even slight possibility that it can change.
Quoted 10 lines
Quoted 8 lines
Now to your naming issue...
This code is quite specific to be only local so I would propose following:
parse_single_color_name->parse_color_name
parse_color_name->parse_color_tuple,parse_color_pair,parse_text_colororparse_menu_colorHow about `parse_color_name_pair' ?
this name is good as any and goes well with parse_color_name name.
Thanks for the pointers. While fixing them I found other mistakes, then started "torturing" the code with ill user input, found more mistakes, and ended up refactoring most of it.
See attached new patch, this time including ChangeLog entry.
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 /.)
2008-01-01 Robert Millan <[email protected]>
* normal/menu.c (grub_color_menu_normal): New variable. (grub_color_menu_highlight): Likewise. (grub_menu_run): Split `press any key' prompt from here ... (wait_after_message): ... to here (new function). (color_list): New variable (copied from grub/stage2/builtins.c). (parse_color_name): New function. (parse_color_name_pair): New function.
(print_entry): Rename normal_code' to old_color_normal'. Rename highlight_code' to old_color_highlight'.
(grub_menu_init_page): Use parse_color_name_pair() to update grub_color_menu_normal' and grub_color_menu_highlight' from user input, if applicable.
normal/menu.c +107 -13
@@ -1,6 +1,6 @@/* * GRUB -- GRand Unified Bootloader -* Copyright (C) 2003,2004,2005,2006,2007 Free Software Foundation, Inc. +* Copyright (C) 2003,2004,2005,2006,2007,2008 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
@@ -28,6 +28,99 @@#define GRUB_COLOR_MENU_NORMAL 0x07#define GRUB_COLOR_MENU_HIGHLIGHT 0x70 +static grub_uint8_t grub_color_menu_normal =GRUB_COLOR_MENU_NORMAL ; +static grub_uint8_t grub_color_menu_highlight =GRUB_COLOR_MENU_HIGHLIGHT ; + +static void +wait_after_message (void ) +{ +/* Wait until the user pushes any key so that the user +can see what happened. */ +grub_printf ("\nPress any key to continue..." ); + (void )grub_getkey (); +} + +/* Borrowed from GRUB Legacy */ +static char *color_list[16 ] = +{ +"black" , +"blue" , +"green" , +"cyan" , +"red" , +"magenta" , +"brown" , +"light-gray" , +"dark-gray" , +"light-blue" , +"light-green" , +"light-cyan" , +"light-red" , +"light-magenta" , +"yellow" , +"white" +}; + +static int +parse_color_name (grub_uint8_t *ret,char *name) +{ +grub_uint8_t i; +for (i =0 ; i <sizeof (color_list) /sizeof (*color_list); i++) +if (!grub_strcmp (name, color_list[i])) + { + *ret = i; +return 0 ; + } +return -1 ; +} + +static void +parse_color_name_pair (grub_uint8_t *ret,char *name) +{ +grub_uint8_t fg, bg; +char *fg_name, *bg_name; + +/* nothing specified by user */ +if (name ==NULL ) +return ; + + fg_name =grub_strdup (name); +if (fg_name ==NULL ) + { +/* "out of memory" message was printed by grub_strdup() */ +wait_after_message (); +return ; + } + + bg_name =grub_strchr (fg_name,'/' ); +if (bg_name ==NULL ) + { +grub_printf ("Warning: syntax error (missing slash) in `%s'\n" , fg_name); +wait_after_message (); + goto free_and_return; + } + + *(bg_name++) ='\0' ; + +if (parse_color_name (&fg, fg_name) ==-1 ) + { +grub_printf ("Warning: invalid foreground color `%s'\n" , fg_name); +wait_after_message (); + goto free_and_return; + } +if (parse_color_name (&bg, bg_name) ==-1 ) + { +grub_printf ("Warning: invalid background color `%s'\n" , bg_name); +wait_after_message (); + goto free_and_return; + } + + *ret = (bg <<4 ) | fg; + +free_and_return: +grub_free (fg_name); +} +static void draw_border (void ) {
@@ -108,7 +201,7 @@ print_entry (int y, int highlight, grub_grub_ssize_t len;grub_uint32_t *unicode_title;grub_ssize_t i; -grub_uint8_t normal_code, highlight_code; +grub_uint8_t old_color_normal, old_color_highlight; title = entry ? entry->title :"" ; unicode_title =grub_malloc (grub_strlen (title) *sizeof (*unicode_title));
@@ -125,8 +218,8 @@ print_entry (int y, int highlight, grub_return ; } -grub_getcolor (&normal_code, &highlight_code); -grub_setcolor (GRUB_COLOR_MENU_NORMAL ,GRUB_COLOR_MENU_HIGHLIGHT ); +grub_getcolor (&old_color_normal, &old_color_highlight); +grub_setcolor (grub_color_menu_normal, grub_color_menu_highlight);grub_setcolorstate (highlight ?GRUB_TERM_COLOR_HIGHLIGHT :GRUB_TERM_COLOR_NORMAL );
@@ -164,7 +257,7 @@ print_entry (int y, int highlight, grub_grub_gotoxy (GRUB_TERM_CURSOR_X , y); -grub_setcolor (normal_code, highlight_code); +grub_setcolor (old_color_normal, old_color_highlight);grub_setcolorstate (GRUB_TERM_COLOR_STANDARD );grub_free (unicode_title); }
@@ -209,15 +302,19 @@ print_entries (grub_menu_t menu, int firvoid grub_menu_init_page (int nested,int edit) { -grub_uint8_t normal_code, highlight_code; -grub_getcolor (&normal_code, &highlight_code); -grub_setcolor (GRUB_COLOR_MENU_NORMAL ,GRUB_COLOR_MENU_HIGHLIGHT ); +grub_uint8_t old_color_normal, old_color_highlight; + +parse_color_name_pair (&grub_color_menu_normal,grub_env_get ("color_normal" )); +parse_color_name_pair (&grub_color_menu_highlight,grub_env_get ("color_highlight" )); + +grub_getcolor (&old_color_normal, &old_color_highlight); +grub_setcolor (grub_color_menu_normal, grub_color_menu_highlight);grub_normal_init_page ();draw_border ();print_message (nested, edit); -grub_setcolor (normal_code, highlight_code); +grub_setcolor (old_color_normal, old_color_highlight); }/* Return the current timeout. If the variable "timeout" is not set or
@@ -501,10 +598,7 @@ grub_menu_run (grub_menu_t menu, int nesgrub_print_error (); grub_errno =GRUB_ERR_NONE ; -/* Wait until the user pushes any key so that the user -can see what happened. */ -grub_printf ("\nPress any key to continue..." ); - (void )grub_getkey (); +wait_after_message (); } } }
Some rework to support user-defined colors in non-menu as well. This is most desirable when using the new background image feature implemented by Vesa, since light-grey may not be properly readable depending on the image you're using.
Example for bios console, setting up only menu color:
set menu_color_normal=cyan/blue
set menu_color_highlight=white/blue
Example for gfxterm, setting up white foreground to match with loaded image:
insmod video
insmod vbe
insmod gfxterm
insmod tga
font (hd0)/unifont.pff
terminal gfxterm
set color_normal=white/black
set color_highlight=black/white
colorbackground_image (hd0)/bg.tga
`color' command is necessary to propagate user setting from environment variables to the internal color of our current terminal.
Please comment on the design, etc first. I still have to write a ChangeLog entry, update the missing *.rmk files, etc.
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 /.)
conf/i386-pc.rmk +2 -1
@@ -111,7 +111,7 @@ grub_emu_SOURCES = commands/boot.c comma kern/loader.c kern/main.c kern/misc.c kern/parser.c \ grub_script.tab.c kern/partition.c kern/rescue.c kern/term.c \ normal/arg.c normal/cmdline.c normal/command.c normal/function.c\ - normal/completion.c normal/main.c \ + normal/completion.c normal/main.c normal/color.c \ normal/menu.c normal/menu_entry.c normal/misc.c normal/script.c \ partmap/amiga.c partmap/apple.c partmap/pc.c partmap/sun.c \ partmap/acorn.c partmap/gpt.c \
@@ -168,6 +168,7 @@ normal_mod_DEPENDENCIES = grub_script.ta normal_mod_SOURCES = normal/arg.c normal/cmdline.c normal/command.c \ normal/completion.c normal/execute.c \ normal/function.c normal/lexer.c normal/main.c normal/menu.c \ + normal/color.c \ normal/menu_entry.c normal/misc.c grub_script.tab.c \ normal/script.c normal/i386/setjmp.S normal_mod_CFLAGS = $(COMMON_CFLAGS)
include/grub/normal.h +2 -0
@@ -154,6 +154,8 @@ grub_err_t grub_normal_print_device_infogrub_err_t grub_normal_menu_addentry (const char *title,struct grub_script *script,const char *sourcecode); +void grub_normal_color_reload (void ); +void wait_after_message (void );#ifdef GRUB_UTIL void grub_normal_init (void );
normal/color.c +119 -0
119 lines not on this page · raw message
normal/command.c +11 -0
@@ -365,6 +365,14 @@ lsmod_command (struct grub_arg_list *stareturn 0 ; } +static grub_err_t +color_command (struct grub_arg_list *state __attribute__ ((unused)), +int argc __attribute__ ((unused)),char **args __attribute__ ((unused))) +{ +grub_normal_color_reload (); +return 0 ; +} +void grub_command_init (void ) {
@@ -391,4 +399,7 @@ grub_command_init (void)grub_register_command ("lsmod" , lsmod_command,GRUB_COMMAND_FLAG_BOTH ,"lsmod" ,"Show loaded modules." ,0 ); + +grub_register_command ("color" , color_command,GRUB_COMMAND_FLAG_BOTH , +"color" ,"Set colors in the current terminal." ,0 ); }
normal/main.c +1 -6
@@ -268,12 +268,7 @@ read_config_file (const char *config, ingrub_file_close (file);if (errors >0 ) - { -/* Wait until the user pushes any key so that the user can -see what happened. */ -grub_printf ("\nPress any key to continue..." ); - (void )grub_getkey (); - } +wait_after_message ();return newmenu; }
normal/menu.c +35 -19
111 lines not on this page · raw message
normal/menu_entry.c +1 -4
@@ -1030,10 +1030,7 @@ run (struct screen *screen) {grub_print_error (); grub_errno =GRUB_ERR_NONE ; -/* Wait until the user pushes any key so that the user -can see what happened. */ -grub_printf ("\nPress any key to continue..." ); - (void )grub_getkey (); +wait_after_message (); }return 1 ;
Quoted 3 lines · Robert Millan
`color' command is necessary to propagate user setting from environment variables to the internal color of our current terminal.
Vesa suggested (on IRC) to use write hooks (grub_env_write_hook_t) in variables to avoid this. I tend to agree. What do others think?
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 6 lines · Robert Millan
Quoted 2 lines · Robert Millan
`color' command is necessary to propagate user setting from environment variables to the internal color of our current terminal.
Vesa suggested (on IRC) to use write hooks (
grub_env_write_hook_t) in variables to avoid this. I tend to agree. What do others think?
Generally speaking, if you are planning to read settings by scripting, it is better to use a variable. If you are planning to change the behavior by options, it is better to use a command.
Or, semantically, if you want to make an action, a command is suitable. If you want to set a state, a variable is more appropriate.
Anyway, you can accomplish nearly the same thing with both, thanks to the hooks, so the choice is mostly a matter of taste.
Personally, I think this should come up with variables. It is just like a prompt in bash for me.
Okuji
Quoted 12 lines · Yoshinori K. Okuji
Quoted 6 lines · Robert Millan
Quoted 2 lines · Robert Millan
`color' command is necessary to propagate user setting from environment variables to the internal color of our current terminal.
Vesa suggested (on IRC) to use write hooks (
grub_env_write_hook_t) in variables to avoid this. I tend to agree. What do others think?Generally speaking, if you are planning to read settings by scripting, it is better to use a variable. If you are planning to change the behavior by options, it is better to use a command.
Quoted 8 lines
Or, semantically, if you want to make an action, a command is suitable. If you want to set a state, a variable is more appropriate.
Anyway, you can accomplish nearly the same thing with both, thanks to the hooks, so the choice is mostly a matter of taste.
Personally, I think this should come up with variables. It is just like a prompt in bash for me.
Ok, variables then. It's also more consistent, since the `color' command I implemented was reliing on variables anyway.
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 4 lines · Robert Millan
This patch allows the following to work:
set color_normal=cyan/blue
set color_highlight=white/blue
Although I am not a native speaker, I feel that they should be better called "normal_color" and "highlight_color".
Quoted 10 lines
which is equivalent to this command in GRUB Legacy:
color cyan/blue white/blue
I haven't written a ChangeLog entry yet, because I'd like to receive comments on the function names. I don't really like
parse_single_color_name' andparse_color_name' but I don't know what to call them :-(. What conventions are there for referring to single colors (i.e. those that describe only FG or only BG, not both) and complete colors (FG + BG) ?
No such word, AFAIK. You know, it is ugly, because you want to put two separate things into one place. ;)
Just look at CSS...
div {
color: black;
background-color: white;
}
So, if you don't hesitate to create so many variables, you can simply create "normal_color", "normal_background_color", "highlight_color" and "highlight_background_color", although I don't know who would like it.
Okuji
Quoted 8 lines · Yoshinori K. Okuji
Quoted 4 lines · Robert Millan
This patch allows the following to work:
set color_normal=cyan/blue
set color_highlight=white/blueAlthough I am not a native speaker, I feel that they should be better called "
normal_color" and "highlight_color".
I like to come up with names that share their prefix, because this makes the set more readable:
set color_normal=cyan/blue
set color_highlight=white/blue
set normal_color=cyan/blue
set highlight_color=white/blue
In the first one the "color_" section is easier to identify. Althogh I don't feel strongly about this of course :-)
Quoted 16 lines
Quoted 10 lines
which is equivalent to this command in GRUB Legacy:
color cyan/blue white/blue
I haven't written a ChangeLog entry yet, because I'd like to receive comments on the function names. I don't really like
parse_single_color_name' andparse_color_name' but I don't know what to call them :-(. What conventions are there for referring to single colors (i.e. those that describe only FG or only BG, not both) and complete colors (FG + BG) ?No such word, AFAIK. You know, it is ugly, because you want to put two separate things into one place. ;)
Just look at CSS...
Quoted 8 lines
div {
color: black;
background-color: white;
}So, if you don't hesitate to create so many variables, you can simply create "
normal_color", "normal_background_color", "highlight_color" and "highlight_background_color", although I don't know who would like it.
Sounds like this could save us some code space. I'd go for _fg and _bg to preserve alignment in the names.
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 6 lines · Robert Millan
Quoted 3 lines
So, if you don't hesitate to create so many variables, you can simply create "
normal_color", "normal_background_color", "highlight_color" and "highlight_background_color", although I don't know who would like it.Sounds like this could save us some code space. I'd go for
_fgand_bgto preserve alignment in the names.
Uhm actually, splitting those in _fg and _bg was a bit of a hassle, because GRUB internally thinks of colors as (bg << 4 | fg) like vga does, so obtaining them from two separate variables didn't bring any real benefit.
See attached new patch, using variable hooks.
Note: if you're going to test this using "configfile" command, think that this opens a new context and exposes the problem with hooks I just reported in the other thread.
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 /.)
* include/grub/normal.h (grub_env_write_color_normal): New prototype. (grub_env_write_color_highlight): Likewise. (grub_wait_after_message): Likewise.
* normal/color.c: New file.
* conf/i386-pc.rmk (grub_emu_SOURCES): Add `normal/color.c'. (normal_mod_DEPENDENCIES): Likewise.
* normal/menu_entry.c (run): Rely on grub_wait_after_message() for waiting after a message is printed.
* normal/main.c (read_config_file): Likewise. (grub_normal_init): Register grub_env_write_color_normal() and grub_env_write_color_highlight() hooks. Mark color_normal' and color_highlight' variables as global.
* normal/menu.c (grub_wait_after_message): New function. (grub_color_menu_normal): New variable. Replaces ... (GRUB_COLOR_MENU_NORMAL): ... this macro. (grub_color_menu_highlight): New variable. Replaces ... (GRUB_COLOR_MENU_HIGHLIGHT): ... this macro. (draw_border): Set color state to GRUB_TERM_COLOR_NORMAL' instead of GRUB_TERM_COLOR_STANDARD'.
(print_message): Use grub_setcolorstate' to reload colors. Rename normal_code' and highlight_code' to old_color_normal' and old_color_highlight', respectively. (grub_menu_init_page): Update colors when drawing the menu, based on menu_color_normal' and `menu_color_highlight' variables. (grub_menu_run): Rely on grub_wait_after_message() for waiting after a message is printed.
conf/i386-pc.rmk +2 -1
@@ -111,7 +111,7 @@ grub_emu_SOURCES = commands/boot.c comma kern/loader.c kern/main.c kern/misc.c kern/parser.c \ grub_script.tab.c kern/partition.c kern/rescue.c kern/term.c \ normal/arg.c normal/cmdline.c normal/command.c normal/function.c\ - normal/completion.c normal/main.c \ + normal/completion.c normal/main.c normal/color.c \ normal/menu.c normal/menu_entry.c normal/misc.c normal/script.c \ partmap/amiga.c partmap/apple.c partmap/pc.c partmap/sun.c \ partmap/acorn.c partmap/gpt.c \
@@ -168,6 +168,7 @@ normal_mod_DEPENDENCIES = grub_script.ta normal_mod_SOURCES = normal/arg.c normal/cmdline.c normal/command.c \ normal/completion.c normal/execute.c \ normal/function.c normal/lexer.c normal/main.c normal/menu.c \ + normal/color.c \ normal/menu_entry.c normal/misc.c grub_script.tab.c \ normal/script.c normal/i386/setjmp.S normal_mod_CFLAGS = $(COMMON_CFLAGS)
include/grub/normal.h +4 -1
@@ -1,7 +1,7 @@/* normal.h - prototypes for the normal mode */ /* * GRUB -- GRand Unified Bootloader -* Copyright (C) 2002,2003,2005,2006,2007 Free Software Foundation, Inc. +* Copyright (C) 2002,2003,2005,2006,2007,2008 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
@@ -154,6 +154,9 @@ grub_err_t grub_normal_print_device_infogrub_err_t grub_normal_menu_addentry (const char *title,struct grub_script *script,const char *sourcecode); +char *grub_env_write_color_normal (struct grub_env_var *var,const char *val); +char *grub_env_write_color_highlight (struct grub_env_var *var,const char *val); +void grub_wait_after_message (void );#ifdef GRUB_UTIL void grub_normal_init (void );
normal/color.c +149 -0
149 lines not on this page · raw message
normal/main.c +10 -7
@@ -1,7 +1,7 @@/* main.c - the normal mode main routine */ /* * GRUB -- GRand Unified Bootloader -* Copyright (C) 2000,2001,2002,2003,2005,2006,2007 Free Software Foundation, Inc. +* Copyright (C) 2000,2001,2002,2003,2005,2006,2007,2008 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
@@ -268,12 +268,7 @@ read_config_file (const char *config, ingrub_file_close (file);if (errors >0 ) - { -/* Wait until the user pushes any key so that the user can -see what happened. */ -grub_printf ("\nPress any key to continue..." ); - (void )grub_getkey (); - } +grub_wait_after_message ();return newmenu; }
@@ -527,6 +522,14 @@ GRUB_MOD_INIT(normal)grub_rescue_register_command ("normal" , grub_rescue_cmd_normal,"enter normal mode" ); +/* Reload terminal colors when these variables are written to. */ +grub_register_variable_hook ("color_normal" ,NULL , grub_env_write_color_normal); +grub_register_variable_hook ("color_highlight" ,NULL , grub_env_write_color_highlight); + +/* Preserve hooks after context changes. */ +grub_env_export ("color_normal" ); +grub_env_export ("color_highlight" ); +/* This registers some built-in commands. */ grub_command_init (); }
normal/menu.c +35 -19
111 lines not on this page · raw message
normal/menu_entry.c +2 -5
@@ -1,6 +1,6 @@/* * GRUB -- GRand Unified Bootloader -* Copyright (C) 2005,2006,2007 Free Software Foundation, Inc. +* Copyright (C) 2005,2006,2007,2008 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
@@ -1030,10 +1030,7 @@ run (struct screen *screen) {grub_print_error (); grub_errno =GRUB_ERR_NONE ; -/* Wait until the user pushes any key so that the user -can see what happened. */ -grub_printf ("\nPress any key to continue..." ); - (void )grub_getkey (); +grub_wait_after_message (); }return 1 ;
Quoted 11 lines · Robert Millan
Quoted 5 lines · Robert Millan
Quoted 3 lines
So, if you don't hesitate to create so many variables, you can simply create "
normal_color", "normal_background_color", "highlight_color" and "highlight_background_color", although I don't know who would like it.Sounds like this could save us some code space. I'd go for
_fgand_bgto preserve alignment in the names.Uhm actually, splitting those in
_fgand_bgwas a bit of a hassle, because GRUB internally thinks of colors as (bg << 4 | fg) like vga does, so obtaining them from two separate variables didn't bring any real benefit.
Quoted 5 lines
See attached new patch, using variable hooks.
Note: if you're going to test this using "configfile" command, think that this opens a new context and exposes the problem with hooks I just reported in the other thread.
About error handling:
Why not call grub_error() with error message and just return from callback, and let prompt handle error processing (grub_print_error()). This would keep error reporting centralized.
About new context:
Shouldn't new context have clone of it's parent contexts settings?
Quoted 6 lines · Vesa Jääskeläinen
About error handling:
Why not call
grub_error()with error message and just return from callback, and let prompt handle error processing (grub_print_error()). This would keep error reporting centralized.
One thing I don't like about grub_error() is that I never know what value to pick for the first parameter. I find this confusing :-/
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 6 lines · Vesa Jääskeläinen
About error handling:
Why not call
grub_error()with error message and just return from callback, and let prompt handle error processing (grub_print_error()). This would keep error reporting centralized.One thing I don't like about
grub_error()is that I never know what value to pick for the first parameter. I find this confusing :-/
You can add errors to the enum.
--
Marco
opening new context (was: [PATCH] allow user-configurable menucolor)
In reply to Vesa JääskeläinenQuoted 4 lines · Vesa Jääskeläinen
About new context:
Shouldn't new context have clone of it's parent contexts settings?
I wouldn't want to change that without having some input from whoever designed (or understands) grub_env_export(). There's probably a reason for it, although I can't see it.
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 /.)
Re: opening new context (was: [PATCH] allow user-configurable menucolor)
Quoted 8 lines · Robert Millan
Quoted 3 lines · Vesa Jääskeläinen
About new context:
Shouldn't new context have clone of it's parent contexts settings?
I wouldn't want to change that without having some input from whoever designed (or understands)
grub_env_export(). There's probably a reason for it, although I can't see it.
It is analogous to bash. If you invoke another bash from bash, that won't inherit variables, unless they are exported.
Besides that, I think it is a good thing that some kind of "clean room" or "sandbox" is guaranteed.
Okuji
Re: opening new context (was: [PATCH] allow user-configurable menucolor)
Quoted 4 lines · Yoshinori K. Okuji
Quoted 3 lines · Robert Millan
Quoted 2 lines · Vesa Jääskeläinen
About new context:
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Shouldn't new context have clone of it's parent contexts settings?
I wouldn't want to change that without having some input from whoever designed (or understands)
grub_env_export(). There's probably a reason for it, although I can't see it.It is analogous to bash. If you invoke another bash from bash, that won't inherit variables, unless they are exported.
It's not exactly analogous. bash doesn't commonly use variable hooks (if they even exist).
Anyway, I wouldn't want to spend much time discussing this. I assume my proposed change (propagating read/write hooks for global/exported variables) sounds fine?
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 /.)
Re: opening new context (was: [PATCH] allow user-configurable menucolor)
Quoted 11 lines · Robert Millan
Quoted 10 lines · Yoshinori K. Okuji
Quoted 8 lines · Robert Millan
Quoted 3 lines · Vesa Jääskeläinen
About new context:
Shouldn't new context have clone of it's parent contexts settings?
I wouldn't want to change that without having some input from whoever designed (or understands)
grub_env_export(). There's probably a reason for it, although I can't see it.
Quoted 5 lines
Quoted 2 lines
It is analogous to bash. If you invoke another bash from bash, that won't inherit variables, unless they are exported.
It's not exactly analogous. bash doesn't commonly use variable hooks (if they even exist).
It has internally.
Quoted 3 lines
Anyway, I wouldn't want to spend much time discussing this. I assume my proposed change (propagating read/write hooks for global/exported variables) sounds fine?
Yes.
Okuji
Re: opening new context (was: [PATCH] allow user-configurable menucolor)
Quoted 5 lines · Yoshinori K. Okuji
Quoted 3 lines
Anyway, I wouldn't want to spend much time discussing this. I assume my proposed change (propagating read/write hooks for global/exported variables) sounds fine?
Yes.
Ok, 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 /.)
At Thu, 3 Jan 2008 16:35:33 +0100,
Quoted 13 lines · Robert Millan
diff -x '*~' -x '*.mk' -Nurp
grub2/include/grub/normal.hgrub2.color/include/grub/normal.hinclude/grub/normal.h +1
-1@@ -1,7 +1,7 @@/* normal.h - prototypes for the normal mode */ /* * GRUB -- GRand Unified Bootloader -* Copyright (C) 2002,2003,2005,2006,2007 Free Software Foundation, Inc. +* Copyright (C) 2002,2003,2005,2006,2007,2008 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
include/grub/normal.h grub_err_t grub_normal_print_device_info · line 159
Quoted 6 lines
include/grub/normal.h +3
-0@@ -154,6 +154,9 @@ grub_err_t grub_normal_print_device_infogrub_err_t grub_normal_menu_addentry (const char *title,struct grub_script *script,const char *sourcecode); +char *grub_env_write_color_normal (struct grub_env_var *var,const char *val); +char *grub_env_write_color_highlight (struct grub_env_var *var,const char *val); +void grub_wait_after_message (void );
You should actually also include grub/env.h if you use struct grub_env_var in the prototypes. I've fixed that in CVS.
Jeroen Dekkers
Quoted 16 lines · Jeroen Dekkers
At Thu, 3 Jan 2008 16:35:33 +0100,
Quoted 14 lines · Robert Millan
diff -x '*~' -x '*.mk' -Nurp
grub2/include/grub/normal.hgrub2.color/include/grub/normal.hinclude/grub/normal.h +1
-1@@ -1,7 +1,7 @@/* normal.h - prototypes for the normal mode */ /* * GRUB -- GRand Unified Bootloader -* Copyright (C) 2002,2003,2005,2006,2007 Free Software Foundation, Inc. +* Copyright (C) 2002,2003,2005,2006,2007,2008 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 @@ -154,6 +154,9 @@ grub_err_t grub_normal_print_device_info grub_err_t grub_normal_menu_addentry (const char *title,
Quoted 8 lines
include/grub/normal.h grub_err_t grub_normal_print_device_info · line 159
Quoted 5 lines
include/grub/normal.h +3
-0struct grub_script *script, const char *sourcecode); +char *grub_env_write_color_normal (struct grub_env_var *var,const char *val); +char *grub_env_write_color_highlight (struct grub_env_var *var,const char *val); +void grub_wait_after_message (void );You should actually also include
grub/env.hif you use structgrub_env_varin the prototypes.
Uhm, I included it in normal/color.c to satisfy the dependencies:
normal/color.c:#include <grub/env.h>
normal/color.c:grub_env_write_color_normal (struct grub_env_var *var __attribute__ ((unused)), normal/color.c:grub_env_write_color_highlight (struct grub_env_var *var __attribute__ ((unused)),
but your idea seems better. I suppose we can remove it from there now?
Quoted 1 line
I've fixed that in CVS.
Thanks.
Btw, nice to see you around here. Do you have any news about the CD-ROM GSoC ? I looked at the tarball from google.com, but CD access seems unfinished (I couldn't access the CD neither in qemu nor in real hw).
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 /.)
At Sun, 6 Jan 2008 00:42:40 +0100,
Quoted 5 lines · Robert Millan
Quoted 2 lines · Jeroen Dekkers
You should actually also include
grub/env.hif you use structgrub_env_varin the prototypes.
Quoted 7 lines
Uhm, I included it in
normal/color.cto satisfy the dependencies:normal/color.c:#include <
grub/env.h>
normal/color.c:grub_env_write_color_normal (structgrub_env_var*var__attribute__((unused)), normal/color.c:grub_env_write_color_highlight (structgrub_env_var*var__attribute__((unused)),but your idea seems better. I suppose we can remove it from there now?
Yes, but normal.h is included by lots of other files that don't always include env.h before they include normal.h.
Quoted 3 lines
Btw, nice to see you around here. Do you have any news about the CD-ROM GSoC ? I looked at the tarball from google.com, but CD access seems unfinished (I couldn't access the CD neither in qemu nor in real hw).
It should work, but I didn't have the time to test it at the end of the summer. I guess you can simply mail Alex if you have any questions...
Jeroen Dekkers
Quoted 7 lines · Jeroen Dekkers
Quoted 3 lines
Btw, nice to see you around here. Do you have any news about the CD-ROM GSoC ? I looked at the tarball from google.com, but CD access seems unfinished (I couldn't access the CD neither in qemu nor in real hw).
It should work, but I didn't have the time to test it at the end of the summer. I guess you can simply mail Alex if you have any questions...
I did. Either my mail was lost or he's too busy to reply :-(
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 /.)