Skip to contentSkip to conversation

colored menu

PATCH In review Robert Millan 4 participants 10 messages Dec 23, 2007 – Jan 23, 2008

lore
Share & tools
lore

Robert Millan

author
#0 lore raw

PATCH 1/1 colored menu

This patch adds a nice looking colored menu like the one you'd obtain on GRUB Legacy with "color cyan/blue white/blue" command. Screenshot is attached as well.

A pair of notes:

- The *_setcolor function stubs are being removed from some terminals because grub_setcolor() from kernel already skips undefined *_setcolor implementations so there's no need for an empty stub. In fact, not needed for this patch but adds confusion (I had to inspect all them to see if *_getcolor() had to be added).

- I know that hardcoding colors is not so nice, but I was unsure how would the selection interface have to look like (and lacking time to implement it), and in comparison with defaulting to hardcoded 0x07 (grey on black) I see it as an improvement.

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/term.h (struct grub_term): Add `getcolor' function. (grub_getcolor): New function.

* kern/term.c (grub_getcolor): New function.
* normal/menu.c (GRUB_COLOR_MENU_NORMAL): New macro. (GRUB_COLOR_MENU_HIGHLIGHT): New macro. (print_entry): Set normal and highlight colors to GRUB_COLOR_MENU_NORMAL' and GRUB_COLOR_MENU_HIGHLIGHT', respectively, before printing and restore them to old values afterwards.
(run_menu): Likewise.

* term/efi/console.c (grub_console_getcolor): New function. (struct grub_console_term.getcolor): New variable.
* term/gfxterm.c (grub_virtual_screen_getcolor): New function. (struct grub_console_term.getcolor): New variable.
* term/i386/pc/console.c (grub_console_getcolor): New function. (struct grub_console_term.getcolor): New variable.
* term/ieee1275/ofconsole.c (grub_ofconsole_getcolor): New function. (struct grub_console_term.getcolor): New variable.

* term/i386/pc/serial.c (grub_serial_setcolor): Remove function. (struct grub_console_term.setcolor): Remove variable.
* term/i386/pc/vesafb.c (grub_virtual_screen_setcolor): Remove function. (struct grub_console_term.setcolor): Remove variable.
* term/i386/pc/vga.c (grub_vga_setcolor): Remove function. (struct grub_console_term.setcolor): Remove variable.

include/grub/term.h renamed from grub2/include/grub/term.h +6 -0
diff -x '*~' -x configure -Nurp ../grub2/include/grub/term.h ./include/grub/term.h
@@ -164,6 +164,10 @@ struct grub_term
      color is VGA's.  */
   void (*setcolor) (grub_uint8_t normal_color, grub_uint8_t highlight_color);
   
+  /* Get the normal color and the highlight color. The format of each
+     color is VGA's.  */
+  void (*getcolor) (grub_uint8_t *normal_color, grub_uint8_t *highlight_color);
+  
   /* Turn on/off the cursor.  */
   void (*setcursor) (int on);
 

@@ -197,6 +201,8 @@ void EXPORT_FUNC(grub_cls) (void);
 void EXPORT_FUNC(grub_setcolorstate) (grub_term_color_state state);
 void EXPORT_FUNC(grub_setcolor) (grub_uint8_t normal_color,
 				 grub_uint8_t highlight_color);
+void EXPORT_FUNC(grub_getcolor) (grub_uint8_t *normal_color,
+				 grub_uint8_t *highlight_color);
 int EXPORT_FUNC(grub_setcursor) (int on);
 int EXPORT_FUNC(grub_getcursor) (void);
 void EXPORT_FUNC(grub_refresh) (void);
kern/term.c renamed from grub2/kern/term.c +7 -0
diff -x '*~' -x configure -Nurp ../grub2/kern/term.c ./kern/term.c
@@ -230,6 +230,13 @@ grub_setcolor (grub_uint8_t normal_color
     (grub_cur_term->setcolor) (normal_color, highlight_color);
 }
 
+void
+grub_getcolor (grub_uint8_t *normal_color, grub_uint8_t *highlight_color)
+{
+  if (grub_cur_term->getcolor)
+    (grub_cur_term->getcolor) (normal_color, highlight_color);
+}
+
 int
 grub_setcursor (int on)
 {
normal/menu.c renamed from grub2/normal/menu.c +13 -2
diff -x '*~' -x configure -Nurp ../grub2/normal/menu.c ./normal/menu.c
@@ -25,6 +25,9 @@
 #include <grub/env.h>
 #include <grub/script.h>
 
+#define GRUB_COLOR_MENU_NORMAL		0x13
+#define GRUB_COLOR_MENU_HIGHLIGHT	0x1f
+
 static void
 draw_border (void)
 {

@@ -105,7 +108,8 @@ 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;
+
   title = entry ? entry->title : "";
   unicode_title = grub_malloc (grub_strlen (title) * sizeof (*unicode_title));
   if (! unicode_title)

@@ -121,6 +125,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_setcolorstate (highlight
 		      ? GRUB_TERM_COLOR_HIGHLIGHT
 		      : GRUB_TERM_COLOR_NORMAL);

@@ -128,7 +134,7 @@ print_entry (int y, int highlight, grub_
   grub_gotoxy (GRUB_TERM_LEFT_BORDER_X + GRUB_TERM_MARGIN, y);
 
   for (x = GRUB_TERM_LEFT_BORDER_X + GRUB_TERM_MARGIN + 1, i = 0;
-       x < GRUB_TERM_LEFT_BORDER_X + GRUB_TERM_BORDER_WIDTH - GRUB_TERM_MARGIN;
+       x <= GRUB_TERM_LEFT_BORDER_X + GRUB_TERM_BORDER_WIDTH - GRUB_TERM_MARGIN;
        i++)
     {
       if (i < len

@@ -155,6 +161,7 @@ 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_free (unicode_title);
 }

@@ -279,6 +286,7 @@ run_menu (grub_menu_t menu, int nested)
   int first, offset;
   unsigned long saved_time;
   int default_entry;
+  grub_uint8_t normal_code, highlight_code;
   
   first = 0;
   

@@ -301,8 +309,11 @@ run_menu (grub_menu_t menu, int nested)
 
  refresh:
   grub_setcursor (0);
+  grub_getcolor (&normal_code, &highlight_code);
+  grub_setcolor (GRUB_COLOR_MENU_NORMAL, GRUB_COLOR_MENU_HIGHLIGHT);
   grub_menu_init_page (nested, 0);
   print_entries (menu, first, offset);
+  grub_setcolor (normal_code, highlight_code);
   grub_refresh ();
 
   while (1)
term/efi/console.c renamed from grub2/term/efi/console.c +8 -0
diff -x '*~' -x configure -Nurp ../grub2/term/efi/console.c ./term/efi/console.c
@@ -260,6 +260,13 @@ grub_console_setcolor (grub_uint8_t norm
 }
 
 static void
+grub_console_getcolor (grub_uint8_t *normal_color, grub_uint8_t *highlight_color)
+{
+  *normal_color = grub_console_normal_color;
+  *highlight_color = grub_console_highlight_color;
+}
+
+static void
 grub_console_setcursor (int on)
 {
   grub_efi_simple_text_output_interface_t *o;

@@ -283,6 +290,7 @@ static struct grub_term grub_console_ter
     .cls = grub_console_cls,
     .setcolorstate = grub_console_setcolorstate,
     .setcolor = grub_console_setcolor,
+    .getcolor = grub_console_getcolor,
     .setcursor = grub_console_setcursor,
     .flags = 0,
     .next = 0
term/gfxterm.c renamed from grub2/term/gfxterm.c +8 -0
diff -x '*~' -x configure -Nurp ../grub2/term/gfxterm.c ./term/gfxterm.c
@@ -848,6 +848,13 @@ grub_virtual_screen_setcolor (grub_uint8
 }
 
 static void
+grub_virtual_screen_getcolor (grub_uint8_t *normal_color, grub_uint8_t *highlight_color)
+{
+  *normal_color = virtual_screen.fg_color_setting;
+  *highlight_color = virtual_screen.bg_color_setting;
+}
+
+static void
 grub_gfxterm_setcursor (int on)
 {
   if (virtual_screen.cursor_state != on)

@@ -883,6 +890,7 @@ static struct grub_term grub_video_term 
     .cls = grub_gfxterm_cls,
     .setcolorstate = grub_virtual_screen_setcolorstate,
     .setcolor = grub_virtual_screen_setcolor,
+    .getcolor = grub_virtual_screen_getcolor,
     .setcursor = grub_gfxterm_setcursor,
     .refresh = grub_gfxterm_refresh,
     .flags = 0,
term/i386/pc/console.c renamed from grub2/term/i386/pc/console.c +8 -0
diff -x '*~' -x configure -Nurp ../grub2/term/i386/pc/console.c ./term/i386/pc/console.c
@@ -117,6 +117,13 @@ grub_console_setcolor (grub_uint8_t norm
   grub_console_highlight_color = highlight_color;
 }
 
+static void
+grub_console_getcolor (grub_uint8_t *normal_color, grub_uint8_t *highlight_color)
+{
+  *normal_color = grub_console_normal_color;
+  *highlight_color = grub_console_highlight_color;
+}
+
 static struct grub_term grub_console_term =
   {
     .name = "console",

@@ -132,6 +139,7 @@ static struct grub_term grub_console_ter
     .cls = grub_console_cls,
     .setcolorstate = grub_console_setcolorstate,
     .setcolor = grub_console_setcolor,
+    .getcolor = grub_console_getcolor,
     .setcursor = grub_console_setcursor,
     .flags = 0,
     .next = 0
term/i386/pc/serial.c renamed from grub2/term/i386/pc/serial.c +0 -8
diff -x '*~' -x configure -Nurp ../grub2/term/i386/pc/serial.c ./term/i386/pc/serial.c
@@ -454,13 +454,6 @@ grub_serial_setcolorstate (const grub_te
 }
 
 static void
-grub_serial_setcolor (grub_uint8_t normal_color __attribute__ ((unused)),
-                      grub_uint8_t highlight_color __attribute__ ((unused)))
-{
-  /* FIXME */
-}
-
-static void
 grub_serial_setcursor (const int on)
 {
   if (on)

@@ -483,7 +476,6 @@ static struct grub_term grub_serial_term
   .gotoxy = grub_serial_gotoxy,
   .cls = grub_serial_cls,
   .setcolorstate = grub_serial_setcolorstate,
-  .setcolor = grub_serial_setcolor,
   .setcursor = grub_serial_setcursor,
   .flags = 0,
   .next = 0
term/i386/pc/vesafb.c renamed from grub2/term/i386/pc/vesafb.c +0 -8
diff -x '*~' -x configure -Nurp ../grub2/term/i386/pc/vesafb.c ./term/i386/pc/vesafb.c
@@ -565,13 +565,6 @@ grub_virtual_screen_setcolorstate (grub_
 }
 
 static void
-grub_virtual_screen_setcolor (grub_uint8_t normal_color __attribute__ ((unused)),
-			      grub_uint8_t highlight_color __attribute__ ((unused)))
-{
-  /* FIXME */
-}
-
-static void
 grub_vesafb_setcursor (int on)
 {
   if (virtual_screen.cursor_state != on)

@@ -599,7 +592,6 @@ static struct grub_term grub_vesafb_term
     .gotoxy = grub_vesafb_gotoxy,
     .cls = grub_vesafb_cls,
     .setcolorstate = grub_virtual_screen_setcolorstate,
-    .setcolor = grub_virtual_screen_setcolor,
     .setcursor = grub_vesafb_setcursor,
     .flags = 0,
     .next = 0
term/i386/pc/vga.c renamed from grub2/term/i386/pc/vga.c +0 -8
diff -x '*~' -x configure -Nurp ../grub2/term/i386/pc/vga.c ./term/i386/pc/vga.c
@@ -460,13 +460,6 @@ grub_vga_setcolorstate (grub_term_color_
 }
 
 static void
-grub_vga_setcolor (grub_uint8_t normal_color __attribute__ ((unused)),
-		   grub_uint8_t highlight_color __attribute__ ((unused)))
-{
-  /* FIXME */
-}
-
-static void
 grub_vga_setcursor (int on)
 {
   if (cursor_state != on)

@@ -494,7 +487,6 @@ static struct grub_term grub_vga_term =
     .gotoxy = grub_vga_gotoxy,
     .cls = grub_vga_cls,
     .setcolorstate = grub_vga_setcolorstate,
-    .setcolor = grub_vga_setcolor,
     .setcursor = grub_vga_setcursor,
     .flags = 0,
     .next = 0
term/ieee1275/ofconsole.c renamed from grub2/term/ieee1275/ofconsole.c +8 -0
diff -x '*~' -x configure -Nurp ../grub2/term/ieee1275/ofconsole.c ./term/ieee1275/ofconsole.c
@@ -129,6 +129,13 @@ grub_ofconsole_setcolor (grub_uint8_t no
   bgcolor = highlight_color;
 }
 
+static void
+grub_ofconsole_getcolor (grub_uint8_t *normal_color, grub_uint8_t *highlight_color)
+{
+  *normal_color = fgcolor;
+  *highlight_color = bgcolor;
+}
+
 static int
 grub_ofconsole_readkey (int *key)
 {

@@ -364,6 +371,7 @@ static struct grub_term grub_ofconsole_t
     .cls = grub_ofconsole_cls,
     .setcolorstate = grub_ofconsole_setcolorstate,
     .setcolor = grub_ofconsole_setcolor,
+    .getcolor = grub_ofconsole_getcolor,
     .setcursor = grub_ofconsole_setcursor,
     .refresh = grub_ofconsole_refresh,
     .flags = 0,
author
#1 lore raw

New patch. Corrects a minor mistake when filling the last colum in the menu. Also, it doesn't change the colors on gfxterm since some reasjustments were needed for that, and there's also a bug that makes it impossible to change colors on the fly with gfxterm/vbe (I'll write more on that later).

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/term.h (struct grub_term): Add `getcolor' function. (grub_getcolor): New function.

* kern/term.c (grub_getcolor): New function.
* normal/menu.c (GRUB_COLOR_MENU_NORMAL): New macro. (GRUB_COLOR_MENU_HIGHLIGHT): New macro.
(print_entry): Set normal and highlight colors to GRUB_COLOR_MENU_NORMAL' and GRUB_COLOR_MENU_HIGHLIGHT', respectively, before printing and restore them to old values afterwards.
(grub_menu_init_page): Likewise. Fill an additional colored space that would otherwise be left blank.

* term/efi/console.c (grub_console_getcolor): New function. (struct grub_console_term.getcolor): New variable.
* term/i386/pc/console.c (grub_console_getcolor): New function. (struct grub_console_term.getcolor): New variable.
* term/ieee1275/ofconsole.c (grub_ofconsole_getcolor): New function. (struct grub_console_term.getcolor): New variable.

* term/i386/pc/serial.c (grub_serial_setcolor): Remove function. (struct grub_console_term.setcolor): Remove variable.
* term/i386/pc/vesafb.c (grub_virtual_screen_setcolor): Remove function. (struct grub_console_term.setcolor): Remove variable.
* term/i386/pc/vga.c (grub_vga_setcolor): Remove function. (struct grub_console_term.setcolor): Remove variable.
* term/gfxterm.c (grub_virtual_screen_setcolor): Remove function. (struct grub_console_term.setcolor): Remove variable.

include/grub/term.h +6 -0
diff -ur grub2/include/grub/term.h grub2.color/include/grub/term.h
@@ -164,6 +164,10 @@
      color is VGA's.  */
   void (*setcolor) (grub_uint8_t normal_color, grub_uint8_t highlight_color);
   
+  /* Get the normal color and the highlight color. The format of each
+     color is VGA's.  */
+  void (*getcolor) (grub_uint8_t *normal_color, grub_uint8_t *highlight_color);
+  
   /* Turn on/off the cursor.  */
   void (*setcursor) (int on);
 

@@ -197,6 +201,8 @@
 void EXPORT_FUNC(grub_setcolorstate) (grub_term_color_state state);
 void EXPORT_FUNC(grub_setcolor) (grub_uint8_t normal_color,
 				 grub_uint8_t highlight_color);
+void EXPORT_FUNC(grub_getcolor) (grub_uint8_t *normal_color,
+				 grub_uint8_t *highlight_color);
 int EXPORT_FUNC(grub_setcursor) (int on);
 int EXPORT_FUNC(grub_getcursor) (void);
 void EXPORT_FUNC(grub_refresh) (void);
kern/term.c +7 -0
diff -ur grub2/kern/term.c grub2.color/kern/term.c
@@ -230,6 +230,13 @@
     (grub_cur_term->setcolor) (normal_color, highlight_color);
 }
 
+void
+grub_getcolor (grub_uint8_t *normal_color, grub_uint8_t *highlight_color)
+{
+  if (grub_cur_term->getcolor)
+    (grub_cur_term->getcolor) (normal_color, highlight_color);
+}
+
 int
 grub_setcursor (int on)
 {
normal/menu.c +17 -1
diff -ur grub2/normal/menu.c grub2.color/normal/menu.c
@@ -25,6 +25,9 @@
 #include <grub/env.h>
 #include <grub/script.h>
 
+#define GRUB_COLOR_MENU_NORMAL		0x13
+#define GRUB_COLOR_MENU_HIGHLIGHT	0x1f
+
 static void
 draw_border (void)
 {

@@ -105,7 +108,8 @@
   grub_ssize_t len;
   grub_uint32_t *unicode_title;
   grub_ssize_t i;
-  
+  grub_uint8_t normal_code, highlight_code;
+
   title = entry ? entry->title : "";
   unicode_title = grub_malloc (grub_strlen (title) * sizeof (*unicode_title));
   if (! unicode_title)

@@ -121,6 +125,8 @@
       return;
     }
   
+  grub_getcolor (&normal_code, &highlight_code);
+  grub_setcolor (GRUB_COLOR_MENU_NORMAL, GRUB_COLOR_MENU_HIGHLIGHT);
   grub_setcolorstate (highlight
 		      ? GRUB_TERM_COLOR_HIGHLIGHT
 		      : GRUB_TERM_COLOR_NORMAL);

@@ -153,8 +159,12 @@
 	  x++;
 	}
     }
+  grub_setcolorstate (GRUB_TERM_COLOR_NORMAL);
+  grub_putchar (' ');
+
   grub_gotoxy (GRUB_TERM_CURSOR_X, y);
 
+  grub_setcolor (normal_code, highlight_code);
   grub_setcolorstate (GRUB_TERM_COLOR_STANDARD);
   grub_free (unicode_title);
 }

@@ -199,9 +209,15 @@
 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_normal_init_page ();
   draw_border ();
   print_message (nested, edit);
+
+  grub_setcolor (normal_code, highlight_code);
 }
 
 /* Return the current timeout. If the variable "timeout" is not set or
term/efi/console.c +8 -0
diff -ur grub2/term/efi/console.c grub2.color/term/efi/console.c
@@ -260,6 +260,13 @@
 }
 
 static void
+grub_console_getcolor (grub_uint8_t *normal_color, grub_uint8_t *highlight_color)
+{
+  *normal_color = grub_console_normal_color;
+  *highlight_color = grub_console_highlight_color;
+}
+
+static void
 grub_console_setcursor (int on)
 {
   grub_efi_simple_text_output_interface_t *o;

@@ -283,6 +290,7 @@
     .cls = grub_console_cls,
     .setcolorstate = grub_console_setcolorstate,
     .setcolor = grub_console_setcolor,
+    .getcolor = grub_console_getcolor,
     .setcursor = grub_console_setcursor,
     .flags = 0,
     .next = 0
term/gfxterm.c +0 -9
diff -ur grub2/term/gfxterm.c grub2.color/term/gfxterm.c
@@ -840,14 +840,6 @@
 }
 
 static void
-grub_virtual_screen_setcolor (grub_uint8_t normal_color,
-                              grub_uint8_t highlight_color)
-{
-  virtual_screen.fg_color_setting = grub_video_map_color (normal_color);
-  virtual_screen.bg_color_setting = grub_video_map_color (highlight_color);
-}
-
-static void
 grub_gfxterm_setcursor (int on)
 {
   if (virtual_screen.cursor_state != on)

@@ -882,7 +874,6 @@
     .gotoxy = grub_gfxterm_gotoxy,
     .cls = grub_gfxterm_cls,
     .setcolorstate = grub_virtual_screen_setcolorstate,
-    .setcolor = grub_virtual_screen_setcolor,
     .setcursor = grub_gfxterm_setcursor,
     .refresh = grub_gfxterm_refresh,
     .flags = 0,
term/i386/pc/console.c +8 -0
diff -ur grub2/term/i386/pc/console.c grub2.color/term/i386/pc/console.c
@@ -117,6 +117,13 @@
   grub_console_highlight_color = highlight_color;
 }
 
+static void
+grub_console_getcolor (grub_uint8_t *normal_color, grub_uint8_t *highlight_color)
+{
+  *normal_color = grub_console_normal_color;
+  *highlight_color = grub_console_highlight_color;
+}
+
 static struct grub_term grub_console_term =
   {
     .name = "console",

@@ -132,6 +139,7 @@
     .cls = grub_console_cls,
     .setcolorstate = grub_console_setcolorstate,
     .setcolor = grub_console_setcolor,
+    .getcolor = grub_console_getcolor,
     .setcursor = grub_console_setcursor,
     .flags = 0,
     .next = 0
term/i386/pc/serial.c +0 -8
diff -ur grub2/term/i386/pc/serial.c grub2.color/term/i386/pc/serial.c
@@ -454,13 +454,6 @@
 }
 
 static void
-grub_serial_setcolor (grub_uint8_t normal_color __attribute__ ((unused)),
-                      grub_uint8_t highlight_color __attribute__ ((unused)))
-{
-  /* FIXME */
-}
-
-static void
 grub_serial_setcursor (const int on)
 {
   if (on)

@@ -483,7 +476,6 @@
   .gotoxy = grub_serial_gotoxy,
   .cls = grub_serial_cls,
   .setcolorstate = grub_serial_setcolorstate,
-  .setcolor = grub_serial_setcolor,
   .setcursor = grub_serial_setcursor,
   .flags = 0,
   .next = 0
term/i386/pc/vesafb.c +0 -8
diff -ur grub2/term/i386/pc/vesafb.c grub2.color/term/i386/pc/vesafb.c
@@ -565,13 +565,6 @@
 }
 
 static void
-grub_virtual_screen_setcolor (grub_uint8_t normal_color __attribute__ ((unused)),
-			      grub_uint8_t highlight_color __attribute__ ((unused)))
-{
-  /* FIXME */
-}
-
-static void
 grub_vesafb_setcursor (int on)
 {
   if (virtual_screen.cursor_state != on)

@@ -599,7 +592,6 @@
     .gotoxy = grub_vesafb_gotoxy,
     .cls = grub_vesafb_cls,
     .setcolorstate = grub_virtual_screen_setcolorstate,
-    .setcolor = grub_virtual_screen_setcolor,
     .setcursor = grub_vesafb_setcursor,
     .flags = 0,
     .next = 0
term/i386/pc/vga.c +0 -8
diff -ur grub2/term/i386/pc/vga.c grub2.color/term/i386/pc/vga.c
@@ -460,13 +460,6 @@
 }
 
 static void
-grub_vga_setcolor (grub_uint8_t normal_color __attribute__ ((unused)),
-		   grub_uint8_t highlight_color __attribute__ ((unused)))
-{
-  /* FIXME */
-}
-
-static void
 grub_vga_setcursor (int on)
 {
   if (cursor_state != on)

@@ -494,7 +487,6 @@
     .gotoxy = grub_vga_gotoxy,
     .cls = grub_vga_cls,
     .setcolorstate = grub_vga_setcolorstate,
-    .setcolor = grub_vga_setcolor,
     .setcursor = grub_vga_setcursor,
     .flags = 0,
     .next = 0
term/ieee1275/ofconsole.c +8 -0
diff -ur grub2/term/ieee1275/ofconsole.c grub2.color/term/ieee1275/ofconsole.c
@@ -129,6 +129,13 @@
   bgcolor = highlight_color;
 }
 
+static void
+grub_ofconsole_getcolor (grub_uint8_t *normal_color, grub_uint8_t *highlight_color)
+{
+  *normal_color = fgcolor;
+  *highlight_color = bgcolor;
+}
+
 static int
 grub_ofconsole_readkey (int *key)
 {

@@ -364,6 +371,7 @@
     .cls = grub_ofconsole_cls,
     .setcolorstate = grub_ofconsole_setcolorstate,
     .setcolor = grub_ofconsole_setcolor,
+    .getcolor = grub_ofconsole_getcolor,
     .setcursor = grub_ofconsole_setcursor,
     .refresh = grub_ofconsole_refresh,
     .flags = 0,
author
#2 lore raw

gfxterm / grub_virtual_screen_setcolorstate()

Quoted 5 lines · Robert Millan

New patch. Corrects a minor mistake when filling the last colum in the menu. Also, it doesn't change the colors on gfxterm since some reasjustments were needed for that, and there's also a bug that makes it impossible to change colors on the fly with gfxterm/vbe (I'll write more on that later).

This patch (relative to previous one) basicaly makes it work with a big ugly kludge (see "| 0xff000000" line). The problem seems to be that alpha channel is set to 0 for all return values of grub_video_map_color() after gfxterm initialization. I checked the few places in the code where those variables are zeroed, but to no avail. I assume the problem is just they haven't been initialised, but I just have no idea where they're supposed to.

Any clues?

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 /.)
term/gfxterm.c +36 -19
diff -ur grub2.color/term/gfxterm.c grub2.gfxterm/term/gfxterm.c
@@ -38,10 +38,13 @@
 
 #define DEFAULT_BORDER_WIDTH	10
 
-#define DEFAULT_FG_COLOR    	0x0a
-#define DEFAULT_BG_COLOR    	0x00
 #define DEFAULT_CURSOR_COLOR	0x0f
 
+static grub_uint8_t grub_gfxterm_cur_color = 0x7;
+static grub_uint8_t grub_gfxterm_standard_color = 0x7;
+static grub_uint8_t grub_gfxterm_normal_color = 0x7;
+static grub_uint8_t grub_gfxterm_highlight_color = 0x70;
+
 struct grub_dirty_region
 {
   int top_left_x;

@@ -90,8 +93,6 @@
   int cursor_state;
 
   /* Color settings.  */
-  grub_video_color_t fg_color_setting;
-  grub_video_color_t bg_color_setting;
   grub_video_color_t fg_color;
   grub_video_color_t bg_color;
   grub_video_color_t cursor_color;

@@ -175,10 +176,8 @@
      we can only have those after mode is initialized.  */
   grub_video_set_active_render_target (text_layer);
 
-  virtual_screen.fg_color_setting = grub_video_map_color (DEFAULT_FG_COLOR);
-  virtual_screen.bg_color_setting = grub_video_map_color (DEFAULT_BG_COLOR);
-  virtual_screen.fg_color = virtual_screen.fg_color_setting;
-  virtual_screen.bg_color = virtual_screen.bg_color_setting;
+  virtual_screen.fg_color = grub_video_map_color (grub_gfxterm_cur_color & 0x0f);
+  virtual_screen.bg_color = grub_video_map_color (grub_gfxterm_cur_color >> 4);
   virtual_screen.cursor_color = grub_video_map_color (DEFAULT_CURSOR_COLOR);
 
   grub_video_set_active_render_target (GRUB_VIDEO_RENDER_TARGET_DISPLAY);

@@ -805,15 +804,12 @@
 static void
 grub_gfxterm_cls (void)
 {
-  grub_video_color_t color;
-
   /* Clear virtual screen.  */
   grub_virtual_screen_cls ();
 
   /* Clear text layer.  */
   grub_video_set_active_render_target (text_layer);
-  color = virtual_screen.bg_color_setting;
-  grub_video_fill_rect (color, 0, 0, mode_info.width, mode_info.height);
+  grub_video_fill_rect (virtual_screen.bg_color, 0, 0, mode_info.width, mode_info.height);
   grub_video_set_active_render_target (GRUB_VIDEO_RENDER_TARGET_DISPLAY);
 
   /* Mark virtual screen to be redrawn.  */

@@ -823,20 +819,39 @@
 static void
 grub_virtual_screen_setcolorstate (grub_term_color_state state)
 {
-  switch (state)
-    {
+  grub_video_color_t tmp;
+
+  switch (state) {
     case GRUB_TERM_COLOR_STANDARD:
+      grub_gfxterm_cur_color = grub_gfxterm_standard_color;
+      break;
     case GRUB_TERM_COLOR_NORMAL:
-      virtual_screen.fg_color = virtual_screen.fg_color_setting;
-      virtual_screen.bg_color = virtual_screen.bg_color_setting;
+      grub_gfxterm_cur_color = grub_gfxterm_normal_color;
       break;
     case GRUB_TERM_COLOR_HIGHLIGHT:
-      virtual_screen.fg_color = virtual_screen.bg_color_setting;
-      virtual_screen.bg_color = virtual_screen.fg_color_setting;
+      grub_gfxterm_cur_color = grub_gfxterm_highlight_color;
       break;
     default:
       break;
-    }
+  }
+
+  virtual_screen.fg_color = grub_video_map_color (grub_gfxterm_cur_color & 0x0f) | 0xff000000;
+  virtual_screen.bg_color = grub_video_map_color (grub_gfxterm_cur_color >> 4);
+}
+
+static void
+grub_virtual_screen_setcolor (grub_uint8_t normal_color,
+                              grub_uint8_t highlight_color)
+{
+  grub_gfxterm_normal_color = normal_color;
+  grub_gfxterm_highlight_color = highlight_color;
+}
+
+static void
+grub_virtual_screen_getcolor (grub_uint8_t *normal_color, grub_uint8_t *highlight_color)
+{
+  *normal_color = grub_gfxterm_normal_color;
+  *highlight_color = grub_gfxterm_highlight_color;
 }
 
 static void

@@ -874,6 +889,8 @@
     .gotoxy = grub_gfxterm_gotoxy,
     .cls = grub_gfxterm_cls,
     .setcolorstate = grub_virtual_screen_setcolorstate,
+    .setcolor = grub_virtual_screen_setcolor,
+    .getcolor = grub_virtual_screen_getcolor,
     .setcursor = grub_gfxterm_setcursor,
     .refresh = grub_gfxterm_refresh,
     .flags = 0,
video/i386/pc/vbe.c +2 -0
diff -ur grub2.color/video/i386/pc/vbe.c grub2.gfxterm/video/i386/pc/vbe.c
@@ -722,6 +722,8 @@
       value |= blue << render_target->mode_info.blue_field_pos;
       value |= alpha << render_target->mode_info.reserved_field_pos;
 
+      grub_printf ("alpha=%x, bits=%x\n", alpha, render_target->mode_info.reserved_field_pos);
+
       return value;
     }
#3 lore raw

Re: gfxterm / grub_virtual_screen_setcolorstate()

Quoted 6 lines · Robert Millan
Quoted 4 lines · Robert Millan

New patch. Corrects a minor mistake when filling the last colum in the menu. Also, it doesn't change the colors on gfxterm since some reasjustments were needed for that, and there's also a bug that makes it impossible to change colors on the fly with gfxterm/vbe (I'll write more on that later).

Quoted 8 lines

This patch (relative to previous one) basicaly makes it work with a big ugly kludge (see "| 0xff000000" line). The problem seems to be that alpha channel is set to 0 for all return values of grub_video_map_color() after gfxterm initialization. I checked the few places in the code where those variables are zeroed, but to no avail. I assume the problem is just they haven't been initialised, but I just have no idea where they're supposed to.

Any clues?

Ok... Your "fix" with OR'ing is not a good one as that makes an assumption about display mode settings. Idea of mapping colors is to get most optimal color settings for desired video mode. Nevertheless I think I know the problem and I get back to you shortly... now I sleep :)

That zero alpha setting most likely comes from color table being specified. (or from video mode settings currently in use, eg. no alpha/reserved bits)

#4 lore raw

Re: gfxterm / grub_virtual_screen_setcolorstate()

Quoted 16 lines · Vesa Jääskeläinen
Quoted 6 lines · Robert Millan
Quoted 4 lines · Robert Millan

New patch. Corrects a minor mistake when filling the last colum in the menu. Also, it doesn't change the colors on gfxterm since some reasjustments were needed for that, and there's also a bug that makes it impossible to change colors on the fly with gfxterm/vbe (I'll write more on that later).

Quoted 8 lines

This patch (relative to previous one) basicaly makes it work with a big ugly kludge (see "| 0xff000000" line). The problem seems to be that alpha channel is set to 0 for all return values of grub_video_map_color() after gfxterm initialization. I checked the few places in the code where those variables are zeroed, but to no avail. I assume the problem is just they haven't been initialised, but I just have no idea where they're supposed to.

Any clues?

Quoted 8 lines

Ok... Your "fix" with OR'ing is not a good one as that makes an assumption about display mode settings. Idea of mapping colors is to get most optimal color settings for desired video mode. Nevertheless I think I know the problem and I get back to you shortly... now I sleep :)

That zero alpha setting most likely comes from color table being specified. (or from video mode settings currently in use, eg. no alpha/reserved bits)

This was fixed already?

--
Marco

author
#5 lore raw

Re: gfxterm / grub_virtual_screen_setcolorstate()

Quoted 23 lines · Marco Gerards
Quoted 21 lines · Vesa Jääskeläinen
Quoted 6 lines · Robert Millan
Quoted 4 lines · Robert Millan

New patch. Corrects a minor mistake when filling the last colum in the menu. Also, it doesn't change the colors on gfxterm since some reasjustments were needed for that, and there's also a bug that makes it impossible to change colors on the fly with gfxterm/vbe (I'll write more on that later).

Quoted 8 lines

This patch (relative to previous one) basicaly makes it work with a big ugly kludge (see "| 0xff000000" line). The problem seems to be that alpha channel is set to 0 for all return values of grub_video_map_color() after gfxterm initialization. I checked the few places in the code where those variables are zeroed, but to no avail. I assume the problem is just they haven't been initialised, but I just have no idea where they're supposed to.

Any clues?

Ok... Your "fix" with OR'ing is not a good one as that makes an assumption about display mode settings. Idea of mapping colors is to get most optimal color settings for desired video mode. Nevertheless I think I know the problem and I get back to you shortly... now I sleep :)

Quoted 5 lines
Quoted 3 lines

That zero alpha setting most likely comes from color table being specified. (or from video mode settings currently in use, eg. no alpha/reserved bits)

This was fixed already?

Vesa fixed 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 /.)
#6 lore raw
In reply to Robert Millan
Quoted 12 lines · Robert Millan

This patch adds a nice looking colored menu like the one you'd obtain on GRUB Legacy with "color cyan/blue white/blue" command. Screenshot is attached as well.

A pair of notes:

- The *_setcolor function stubs are being removed from some terminals because grub_setcolor() from kernel already skips undefined *_setcolor implementations so there's no need for an empty stub. In fact, not
needed for this patch but adds confusion (I had to inspect all them to see if *_getcolor() had to be added).

Quoted 4 lines

- I know that hardcoding colors is not so nice, but I was unsure how
would the selection interface have to look like (and lacking time to implement it), and in comparison with defaulting to hardcoded 0x07 (grey on black) I see it as an improvement.

I still prefer to keep the default as it is. I like that a user (or a distributor) would modify the colors in grub.cfg.

Anyway, this will be a temporary hack, right? I'd love to see a fancier interface built in. Mmh, I really need to allocate time to implement one...

Okuji

author
#7 lore raw
Quoted 3 lines · Yoshinori K. Okuji

I still prefer to keep the default as it is. I like that a user (or a distributor) would modify the colors in grub.cfg.

Distributors (/me puts Debian hat on) can easily patch it.

Anyway, which interface to use? It seems to me that env variables would be more fitting than the "color" command we had in GRUB Legacy. I suggest using two: E.g. "menu_normal_color=fg/bg" and "menu_highlight_color=fg/bg", with the same *color_list[16] from GRUB Legacy.

Quoted 1 line

Anyway, this will be a temporary hack, right?

No, not really. That patch will be necessary when colors can be modified in grub.cfg, too. The only difference will be that it'll only act based on user input.

I plan to implement user input too, just not right now. In the meantime it'd be nice to have this feature in CVS. If you like 0x07 / 0x70 more than 0x13 / 0x1f, we can have that as default so that distributors (/me puts debian hat) can change it easily?

Quoted 2 lines

I'd love to see a fancier
interface built in.

Will we have that anytime soon?

Quoted 1 line

Mmh, I really need to allocate time to implement one...

Would be very nice to see you back on the playground! :-)

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 /.)
#8 lore raw
Quoted 10 lines · Robert Millan
Quoted 2 lines · Yoshinori K. Okuji

I still prefer to keep the default as it is. I like that a user (or a distributor) would modify the colors in grub.cfg.

Distributors (/me puts Debian hat on) can easily patch it.

Anyway, which interface to use? It seems to me that env variables would be more fitting than the "color" command we had in GRUB Legacy. I suggest using two: E.g. "menu_normal_color=fg/bg" and "menu_highlight_color=fg/bg", with the same *color_list[16] from GRUB Legacy.

As you might know, my preference is to implement a style sheet and apply it to a text-based interface as well as a graphic-based interface (which is still vaporware). But this will need time, so I can accept whatever a short-term hack.

Quoted 10 lines
Quoted 1 line

Anyway, this will be a temporary hack, right?

No, not really. That patch will be necessary when colors can be modified in grub.cfg, too. The only difference will be that it'll only act based on user input.

I plan to implement user input too, just not right now. In the meantime it'd be nice to have this feature in CVS. If you like 0x07 / 0x70 more than 0x13 / 0x1f, we can have that as default so that distributors (/me puts debian hat) can change it easily?

It is okay for me.

Quoted 4 lines
Quoted 2 lines

I'd love to see a fancier
interface built in.

Will we have that anytime soon?

I hope so. ;)

Quoted 3 lines
Quoted 1 line

Mmh, I really need to allocate time to implement one...

Would be very nice to see you back on the playground! :-)

Thanks.

Okuji

author
#9 lore raw
Quoted 6 lines · Yoshinori K. Okuji
Quoted 6 lines
Quoted 1 line

Anyway, this will be a temporary hack, right?

No, not really. That patch will be necessary when colors can be modified in grub.cfg, too. The only difference will be that it'll only act based on user input.

Quoted 6 lines
Quoted 4 lines

I plan to implement user input too, just not right now. In the meantime it'd be nice to have this feature in CVS. If you like 0x07 / 0x70 more than 0x13 / 0x1f, we can have that as default so that distributors (/me puts debian hat) can change it easily?

It is okay for me.

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