Skip to contentSkip to conversation

allow user-configurable menucolor

PATCH In review Robert Millan 5 participants 24 messages Jan 1, 2008 – Jan 23, 2008

lore
Share & tools
lore
#0 lore raw

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
diff -ur grub2.vesa/normal/menu.c grub2.gfxterm/normal/menu.c
@@ -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 ();
 	}
     }
 }
#1 lore raw
Quoted 7 lines · Robert Millan

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:

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' 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) ?

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 :)

#2 lore raw
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_color or parse_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 /.)
#3 lore raw
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_color or parse_menu_color

How about `parse_color_name_pair' ?

this name is good as any and goes well with parse_color_name name.

#4 lore raw

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
diff -urp grub2/normal/menu.c grub2.color/normal/menu.c
@@ -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 fir
 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;
+
+  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 nes
 	  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 ();
 	}
     }
 }
#5 lore raw

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
color
background_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
diff -x '*.mk' -Nurp grub2/conf/i386-pc.rmk grub2.color/conf/i386-pc.rmk
@@ -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
diff -x '*.mk' -Nurp grub2/include/grub/normal.h grub2.color/include/grub/normal.h
@@ -154,6 +154,8 @@ grub_err_t grub_normal_print_device_info
 grub_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
diff -x '*.mk' -Nurp grub2/normal/command.c grub2.color/normal/command.c
@@ -365,6 +365,14 @@ lsmod_command (struct grub_arg_list *sta
   return 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
diff -x '*.mk' -Nurp grub2/normal/main.c grub2.color/normal/main.c
@@ -268,12 +268,7 @@ read_config_file (const char *config, in
   grub_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
diff -x '*.mk' -Nurp grub2/normal/menu_entry.c grub2.color/normal/menu_entry.c
@@ -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;
#6 lore raw
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 /.)
#7 lore raw
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

#8 lore raw
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 /.)
#9 lore raw
In reply to Robert Millan
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' 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) ?

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

#10 lore raw
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/blue

Although 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' 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) ?

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 /.)
#11 lore raw
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 _fg and _bg to 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
diff -x '*~' -x '*.mk' -Nurp grub2/conf/i386-pc.rmk grub2.color/conf/i386-pc.rmk
@@ -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
diff -x '*~' -x '*.mk' -Nurp grub2/include/grub/normal.h grub2.color/include/grub/normal.h
@@ -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,
 				      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
diff -x '*~' -x '*.mk' -Nurp grub2/normal/main.c grub2.color/normal/main.c
@@ -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, in
   grub_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
diff -x '*~' -x '*.mk' -Nurp grub2/normal/menu_entry.c grub2.color/normal/menu_entry.c
@@ -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;
#12 lore raw
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 _fg and _bg to 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.

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?

#13 lore raw
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 /.)
#14 lore raw
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

#15 lore raw

opening new context (was: [PATCH] allow user-configurable menucolor)

In reply to Vesa Jääskeläinen
Quoted 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 /.)
#16 lore raw

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

#17 lore raw

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 /.)
#18 lore raw

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

#19 lore raw

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 /.)
#20 lore raw
In reply to Robert Millan

At Thu, 3 Jan 2008 16:35:33 +0100,

Quoted 13 lines · Robert Millan

diff -x '*~' -x '*.mk' -Nurp grub2/include/grub/normal.h grub2.color/include/grub/normal.h

include/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_info
 grub_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

#21 lore raw
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.h grub2.color/include/grub/normal.h

include/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 -0
 				      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.

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 /.)
#22 lore raw

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.h if you use struct grub_env_var in the prototypes.

Quoted 7 lines

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?

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

#23 lore raw
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 /.)