[PATCH] allow user-configurable menucolor

[PATCH] allow user-configurable menucolor

[Robert Millan]

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


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

-- 
Robert Millan

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

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

diff -ur grub2.vesa/normal/menu.c grub2.gfxterm/normal/menu.c
--- grub2.vesa/normal/menu.c	2007-12-25 12:10:46.000000000 +0100
+++ grub2.gfxterm/normal/menu.c	2008-01-01 13:44:15.000000000 +0100
@@ -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 ();
 	}
     }
 }

Re: [PATCH] allow user-configurable menucolor

[Vesa Jääskeläinen]

Robert Millan wrote:
> 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) ?

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

Re: [PATCH] allow user-configurable menucolor

[Robert Millan]

On Tue, Jan 01, 2008 at 03:14:50PM +0200, Vesa Jääskeläinen wrote:
> 
> First some questions ;)
> 
> - What happens when you forgot to provide / in this string ;) ?

grub_strchr() returns NULL and... whoops!  I'll fix that :-)

> - What happens when all memory is used?

What do you mean?

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

> (eg. hard coding is bad... sizeof...)

Where can sizeof be used?

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

-- 
Robert Millan

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

Re: [PATCH] allow user-configurable menucolor

[Vesa Jääskeläinen]

Robert Millan wrote:
> On Tue, Jan 01, 2008 at 03:14:50PM +0200, Vesa Jääskeläinen wrote:
>> First some questions ;)
>>
>> - What happens when you forgot to provide / in this string ;) ?
> 
> grub_strchr() returns NULL and... whoops!  I'll fix that :-)
> 
>> - What happens when all memory is used?
> 
> What do you mean?

strdup() == NULL

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

>> (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.

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

Re: [PATCH] allow user-configurable menucolor

[Robert Millan]

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


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.

-- 
Robert Millan

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

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

2008-01-01  Robert Millan  <rmh@aybabtu.com>

	* 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.

diff -urp grub2/normal/menu.c grub2.color/normal/menu.c
--- grub2/normal/menu.c	2007-12-25 12:10:46.000000000 +0100
+++ grub2.color/normal/menu.c	2008-01-01 15:34:15.000000000 +0100
@@ -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 ();
 	}
     }
 }

Re: [PATCH] allow user-configurable menucolor

[Robert Millan]

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


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.

-- 
Robert Millan

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

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

diff -x '*.mk' -Nurp grub2/conf/i386-pc.rmk grub2.color/conf/i386-pc.rmk
--- grub2/conf/i386-pc.rmk	2007-12-26 08:51:18.000000000 +0100
+++ grub2.color/conf/i386-pc.rmk	2008-01-01 17:43:27.000000000 +0100
@@ -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)
diff -x '*.mk' -Nurp grub2/include/grub/normal.h grub2.color/include/grub/normal.h
--- grub2/include/grub/normal.h	2007-07-22 01:32:22.000000000 +0200
+++ grub2.color/include/grub/normal.h	2008-01-01 18:06:36.000000000 +0100
@@ -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);
diff -x '*.mk' -Nurp grub2/normal/color.c grub2.color/normal/color.c
--- grub2/normal/color.c	1970-01-01 01:00:00.000000000 +0100
+++ grub2.color/normal/color.c	2008-01-01 18:03:28.000000000 +0100
@@ -0,0 +1,119 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 1999,2000,2001,2002,2003,2004,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
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/misc.h>
+#include <grub/mm.h>
+#include <grub/normal.h>
+#include <grub/term.h>
+
+/* 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;
+}
+
+void
+grub_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);
+}
+
+/* Replace default console colors with the ones specified by
+   user (if any).  */
+void
+grub_normal_color_reload (void)
+{
+  grub_uint8_t color_normal, color_highlight;
+
+  grub_getcolor (&color_normal, &color_highlight);
+
+  grub_parse_color_name_pair (&color_normal, grub_env_get ("color_normal"));
+  grub_parse_color_name_pair (&color_highlight, grub_env_get ("color_highlight"));
+
+  grub_setcolor (color_normal, color_highlight);
+  grub_setcolorstate (GRUB_TERM_COLOR_NORMAL);
+}
diff -x '*.mk' -Nurp grub2/normal/command.c grub2.color/normal/command.c
--- grub2/normal/command.c	2007-07-22 01:32:29.000000000 +0200
+++ grub2.color/normal/command.c	2008-01-01 18:06:16.000000000 +0100
@@ -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);
 }
diff -x '*.mk' -Nurp grub2/normal/main.c grub2.color/normal/main.c
--- grub2/normal/main.c	2007-07-22 01:32:29.000000000 +0200
+++ grub2.color/normal/main.c	2008-01-01 18:04:33.000000000 +0100
@@ -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;
 }
diff -x '*.mk' -Nurp grub2/normal/menu.c grub2.color/normal/menu.c
--- grub2/normal/menu.c	2007-12-25 12:10:46.000000000 +0100
+++ grub2.color/normal/menu.c	2008-01-01 18:29:05.000000000 +0100
@@ -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
@@ -25,8 +25,17 @@
 #include <grub/env.h>
 #include <grub/script.h>
 
-#define GRUB_COLOR_MENU_NORMAL		0x07
-#define GRUB_COLOR_MENU_HIGHLIGHT	0x70
+static grub_uint8_t grub_color_menu_normal;
+static grub_uint8_t grub_color_menu_highlight;
+
+/* Wait until the user pushes any key so that the user
+   can see what happened.  */
+void
+wait_after_message (void)
+{
+  grub_printf ("\nPress any key to continue...");
+  (void) grub_getkey ();
+}
 
 static void
 draw_border (void)
@@ -57,7 +66,7 @@ draw_border (void)
     grub_putcode (GRUB_TERM_DISP_HLINE);
   grub_putcode (GRUB_TERM_DISP_LR);
 
-  grub_setcolorstate (GRUB_TERM_COLOR_STANDARD);
+  grub_setcolorstate (GRUB_TERM_COLOR_NORMAL);
 
   grub_gotoxy (GRUB_TERM_MARGIN,
 	       (GRUB_TERM_TOP_BORDER_Y + GRUB_TERM_NUM_ENTRIES
@@ -67,6 +76,8 @@ draw_border (void)
 static void
 print_message (int nested, int edit)
 {
+  grub_setcolorstate (GRUB_TERM_COLOR_NORMAL);
+
   if (edit)
     {
       grub_printf ("\n\
@@ -108,7 +119,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));
@@ -124,9 +135,9 @@ print_entry (int y, int highlight, grub_
       grub_free (unicode_title);
       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,8 +175,8 @@ print_entry (int y, int highlight, grub_
 
   grub_gotoxy (GRUB_TERM_CURSOR_X, y);
 
-  grub_setcolor (normal_code, highlight_code);
-  grub_setcolorstate (GRUB_TERM_COLOR_STANDARD);
+  grub_setcolor (old_color_normal, old_color_highlight);
+  grub_setcolorstate (GRUB_TERM_COLOR_NORMAL);
   grub_free (unicode_title);
 }
 
@@ -209,15 +220,23 @@ 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;
+
+  grub_getcolor (&old_color_normal, &old_color_highlight);
+
+  /* By default, use the same colors for the menu.  */
+  grub_color_menu_normal = old_color_normal;
+  grub_color_menu_highlight = old_color_highlight;
+
+  /* Then give user a chance to replace them.  */
+  grub_parse_color_name_pair (&grub_color_menu_normal, grub_env_get ("menu_color_normal"));
+  grub_parse_color_name_pair (&grub_color_menu_highlight, grub_env_get ("menu_color_highlight"));
 
   grub_normal_init_page ();
+  grub_setcolor (grub_color_menu_normal, grub_color_menu_highlight);
   draw_border ();
+  grub_setcolor (old_color_normal, old_color_highlight);
   print_message (nested, edit);
-
-  grub_setcolor (normal_code, highlight_code);
 }
 
 /* Return the current timeout. If the variable "timeout" is not set or
@@ -501,10 +520,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 ();
 	}
     }
 }
diff -x '*.mk' -Nurp grub2/normal/menu_entry.c grub2.color/normal/menu_entry.c
--- grub2/normal/menu_entry.c	2007-12-30 09:52:05.000000000 +0100
+++ grub2.color/normal/menu_entry.c	2008-01-01 17:30:37.000000000 +0100
@@ -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;

Re: [PATCH] allow user-configurable menucolor

[Robert Millan]

On Tue, Jan 01, 2008 at 06:38:06PM +0100, Robert Millan wrote:
> 
> `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?

-- 
Robert Millan

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

Re: [PATCH] allow user-configurable menucolor

[Yoshinori K. Okuji]

On Wednesday 02 January 2008 22:48, Robert Millan wrote:
> On Tue, Jan 01, 2008 at 06:38:06PM +0100, Robert Millan wrote:
> > `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

Re: [PATCH] allow user-configurable menucolor

[Robert Millan]

On Thu, Jan 03, 2008 at 12:55:48AM +0100, Yoshinori K. Okuji wrote:
> On Wednesday 02 January 2008 22:48, Robert Millan wrote:
> > On Tue, Jan 01, 2008 at 06:38:06PM +0100, Robert Millan wrote:
> > > `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.

Ok, variables then.  It's also more consistent, since the `color' command I
implemented was reliing on variables anyway.

-- 
Robert Millan

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

Re: [PATCH] allow user-configurable menucolor

[Yoshinori K. Okuji]

On Tuesday 01 January 2008 13:47, Robert Millan wrote:
> 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".

> 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

Re: [PATCH] allow user-configurable menucolor

[Robert Millan]

On Thu, Jan 03, 2008 at 12:42:01AM +0100, Yoshinori K. Okuji wrote:
> On Tuesday 01 January 2008 13:47, Robert Millan wrote:
> > 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 :-)

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

Sounds like this could save us some code space.  I'd go for _fg and _bg to
preserve alignment in the names.

-- 
Robert Millan

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

Re: [PATCH] allow user-configurable menucolor

[Robert Millan]

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

On Thu, Jan 03, 2008 at 02:04:14AM +0100, Robert Millan wrote:
> > 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.

-- 
Robert Millan

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

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


	* 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.

diff -x '*~' -x '*.mk' -Nurp grub2/conf/i386-pc.rmk grub2.color/conf/i386-pc.rmk
--- grub2/conf/i386-pc.rmk	2007-12-26 08:51:18.000000000 +0100
+++ grub2.color/conf/i386-pc.rmk	2008-01-03 15:01:09.000000000 +0100
@@ -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)
diff -x '*~' -x '*.mk' -Nurp grub2/include/grub/normal.h grub2.color/include/grub/normal.h
--- grub2/include/grub/normal.h	2007-07-22 01:32:22.000000000 +0200
+++ grub2.color/include/grub/normal.h	2008-01-03 16:12:53.000000000 +0100
@@ -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);
diff -x '*~' -x '*.mk' -Nurp grub2/normal/color.c grub2.color/normal/color.c
--- grub2/normal/color.c	1970-01-01 01:00:00.000000000 +0100
+++ grub2.color/normal/color.c	2008-01-03 15:44:48.000000000 +0100
@@ -0,0 +1,149 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 1999,2000,2001,2002,2003,2004,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
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/env.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
+#include <grub/normal.h>
+#include <grub/term.h>
+
+/* 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;
+}
+
+void
+grub_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() */
+      grub_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);
+      grub_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);
+      grub_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);
+      grub_wait_after_message ();
+      goto free_and_return;
+    }
+
+  *ret = (bg << 4) | fg;
+
+free_and_return:
+  grub_free (fg_name);
+}
+
+/* Replace default `normal' colors with the ones specified by user (if any).  */
+char *
+grub_env_write_color_normal (struct grub_env_var *var __attribute__ ((unused)),
+			     const char *val)
+{
+  grub_uint8_t color_normal, color_highlight;
+
+  /* Use old settings in case grub_parse_color_name_pair() has no effect.  */
+  grub_getcolor (&color_normal, &color_highlight);
+
+  grub_parse_color_name_pair (&color_normal, val);
+
+  /* Reloads terminal `normal' and `highlight' colors.  */
+  grub_setcolor (color_normal, color_highlight);
+
+  /* Propagates `normal' color to terminal current color.  */
+  grub_setcolorstate (GRUB_TERM_COLOR_NORMAL);
+
+  return grub_strdup (val);
+}
+
+/* Replace default `highlight' colors with the ones specified by user (if any).  */
+char *
+grub_env_write_color_highlight (struct grub_env_var *var __attribute__ ((unused)),
+				const char *val)
+{
+  grub_uint8_t color_normal, color_highlight;
+
+  /* Use old settings in case grub_parse_color_name_pair() has no effect.  */
+  grub_getcolor (&color_normal, &color_highlight);
+
+  grub_parse_color_name_pair (&color_highlight, val);
+
+  /* Reloads terminal `normal' and `highlight' colors.  */
+  grub_setcolor (color_normal, color_highlight);
+
+  /* Propagates `normal' color to terminal current color.
+     Note: Using GRUB_TERM_COLOR_NORMAL here rather than
+     GRUB_TERM_COLOR_HIGHLIGHT is intentional.  We don't want to switch
+     to highlight state just because color was reloaded.  */
+  grub_setcolorstate (GRUB_TERM_COLOR_NORMAL);
+
+  return grub_strdup (val);
+}
diff -x '*~' -x '*.mk' -Nurp grub2/normal/main.c grub2.color/normal/main.c
--- grub2/normal/main.c	2007-07-22 01:32:29.000000000 +0200
+++ grub2.color/normal/main.c	2008-01-03 16:13:37.000000000 +0100
@@ -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 ();
 }
diff -x '*~' -x '*.mk' -Nurp grub2/normal/menu.c grub2.color/normal/menu.c
--- grub2/normal/menu.c	2007-12-25 12:10:46.000000000 +0100
+++ grub2.color/normal/menu.c	2008-01-03 15:31:37.000000000 +0100
@@ -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
@@ -25,8 +25,17 @@
 #include <grub/env.h>
 #include <grub/script.h>
 
-#define GRUB_COLOR_MENU_NORMAL		0x07
-#define GRUB_COLOR_MENU_HIGHLIGHT	0x70
+static grub_uint8_t grub_color_menu_normal;
+static grub_uint8_t grub_color_menu_highlight;
+
+/* Wait until the user pushes any key so that the user
+   can see what happened.  */
+void
+grub_wait_after_message (void)
+{
+  grub_printf ("\nPress any key to continue...");
+  (void) grub_getkey ();
+}
 
 static void
 draw_border (void)
@@ -57,7 +66,7 @@ draw_border (void)
     grub_putcode (GRUB_TERM_DISP_HLINE);
   grub_putcode (GRUB_TERM_DISP_LR);
 
-  grub_setcolorstate (GRUB_TERM_COLOR_STANDARD);
+  grub_setcolorstate (GRUB_TERM_COLOR_NORMAL);
 
   grub_gotoxy (GRUB_TERM_MARGIN,
 	       (GRUB_TERM_TOP_BORDER_Y + GRUB_TERM_NUM_ENTRIES
@@ -67,6 +76,8 @@ draw_border (void)
 static void
 print_message (int nested, int edit)
 {
+  grub_setcolorstate (GRUB_TERM_COLOR_NORMAL);
+
   if (edit)
     {
       grub_printf ("\n\
@@ -108,7 +119,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));
@@ -124,9 +135,9 @@ print_entry (int y, int highlight, grub_
       grub_free (unicode_title);
       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,8 +175,8 @@ print_entry (int y, int highlight, grub_
 
   grub_gotoxy (GRUB_TERM_CURSOR_X, y);
 
-  grub_setcolor (normal_code, highlight_code);
-  grub_setcolorstate (GRUB_TERM_COLOR_STANDARD);
+  grub_setcolor (old_color_normal, old_color_highlight);
+  grub_setcolorstate (GRUB_TERM_COLOR_NORMAL);
   grub_free (unicode_title);
 }
 
@@ -209,15 +220,23 @@ 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;
+
+  grub_getcolor (&old_color_normal, &old_color_highlight);
+
+  /* By default, use the same colors for the menu.  */
+  grub_color_menu_normal = old_color_normal;
+  grub_color_menu_highlight = old_color_highlight;
+
+  /* Then give user a chance to replace them.  */
+  grub_parse_color_name_pair (&grub_color_menu_normal, grub_env_get ("menu_color_normal"));
+  grub_parse_color_name_pair (&grub_color_menu_highlight, grub_env_get ("menu_color_highlight"));
 
   grub_normal_init_page ();
+  grub_setcolor (grub_color_menu_normal, grub_color_menu_highlight);
   draw_border ();
+  grub_setcolor (old_color_normal, old_color_highlight);
   print_message (nested, edit);
-
-  grub_setcolor (normal_code, highlight_code);
 }
 
 /* Return the current timeout. If the variable "timeout" is not set or
@@ -501,10 +520,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 ();
+	  grub_wait_after_message ();
 	}
     }
 }
diff -x '*~' -x '*.mk' -Nurp grub2/normal/menu_entry.c grub2.color/normal/menu_entry.c
--- grub2/normal/menu_entry.c	2007-12-30 09:52:05.000000000 +0100
+++ grub2.color/normal/menu_entry.c	2008-01-03 16:13:59.000000000 +0100
@@ -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;

Re: [PATCH] allow user-configurable menucolor

[Vesa Jääskeläinen]

Robert Millan wrote:
> On Thu, Jan 03, 2008 at 02:04:14AM +0100, Robert Millan wrote:
>>> 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.

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?

Re: [PATCH] allow user-configurable menucolor

[Robert Millan]

On Thu, Jan 03, 2008 at 06:04:59PM +0200, Vesa Jääskeläinen wrote:
> 
> 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 :-/

-- 
Robert Millan

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

Re: [PATCH] allow user-configurable menucolor

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Thu, Jan 03, 2008 at 06:04:59PM +0200, Vesa Jääskeläinen wrote:
>> 
>> 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)

[Robert Millan]

On Thu, Jan 03, 2008 at 06:04:59PM +0200, Vesa Jääskeläinen wrote:
> 
> 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.

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

[Yoshinori K. Okuji]

On Friday 04 January 2008 09:02, Robert Millan wrote:
> On Thu, Jan 03, 2008 at 06:04:59PM +0200, Vesa Jääskeläinen wrote:
> > 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)

[Robert Millan]

On Sat, Jan 05, 2008 at 02:34:38AM +0100, Yoshinori K. Okuji wrote:
> On Friday 04 January 2008 09:02, Robert Millan wrote:
> > On Thu, Jan 03, 2008 at 06:04:59PM +0200, Vesa Jääskeläinen wrote:
> > > 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.

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?

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

[Yoshinori K. Okuji]

On Saturday 05 January 2008 12:49, Robert Millan wrote:
> On Sat, Jan 05, 2008 at 02:34:38AM +0100, Yoshinori K. Okuji wrote:
> > On Friday 04 January 2008 09:02, Robert Millan wrote:
> > > On Thu, Jan 03, 2008 at 06:04:59PM +0200, Vesa Jääskeläinen wrote:
> > > > 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.
>
> It's not exactly analogous.  bash doesn't commonly use variable hooks (if
> they even exist).

It has internally.

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

[Robert Millan]

On Sat, Jan 05, 2008 at 01:03:41PM +0100, Yoshinori K. Okuji wrote:
> > 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.

-- 
Robert Millan

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

Re: [PATCH] allow user-configurable menucolor

[Jeroen Dekkers]

At Thu, 3 Jan 2008 16:35:33 +0100,
Robert Millan wrote:
> diff -x '*~' -x '*.mk' -Nurp grub2/include/grub/normal.h grub2.color/include/grub/normal.h
> --- grub2/include/grub/normal.h	2007-07-22 01:32:22.000000000 +0200
> +++ grub2.color/include/grub/normal.h	2008-01-03 16:12:53.000000000 +0100
> @@ -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);

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

Re: [PATCH] allow user-configurable menucolor

[Robert Millan]

On Sat, Jan 05, 2008 at 10:45:46PM +0100, Jeroen Dekkers wrote:
> At Thu, 3 Jan 2008 16:35:33 +0100,
> Robert Millan wrote:
> > diff -x '*~' -x '*.mk' -Nurp grub2/include/grub/normal.h grub2.color/include/grub/normal.h
> > --- grub2/include/grub/normal.h	2007-07-22 01:32:22.000000000 +0200
> > +++ grub2.color/include/grub/normal.h	2008-01-03 16:12:53.000000000 +0100
> > @@ -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);
> 
> 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?

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

-- 
Robert Millan

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

Re: [PATCH] allow user-configurable menucolor

[Jeroen Dekkers]

At Sun, 6 Jan 2008 00:42:40 +0100,
Robert Millan wrote:
> 
> On Sat, Jan 05, 2008 at 10:45:46PM +0100, Jeroen Dekkers wrote:
> > 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?

Yes, but normal.h is included by lots of other files that don't always
include env.h before they include normal.h.
 
> 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

Re: [PATCH] allow user-configurable menucolor

[Robert Millan]

On Sun, Jan 06, 2008 at 12:33:29PM +0100, Jeroen Dekkers wrote:
> > 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 :-(

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