Re: [PATCH] Fix grub-emu curses KEY_* mapping

[Christian Franke]

Marco Gerards wrote:

> Christian Franke <address@hidden> writes:
>   
> > Curses function keys do not work in grub-emu, this patch fixes this in
> > grub_ncurses_getkey().
> >     
>
> Ha!  I once noticed this and forgot to document this bug :(
>
>   
> > Another approach would be to remove the scancode translation in
> > grub_console_getkey(), and check for GRUB_CONSOLE_KEY_* in
> > grub_cmdline_get() instead. The GRUB_CONSOLE_KEY_* definitions are
> > already platform specific.
> >     
>
> Yes, this is really weird!  All archs define these macros and expect
> for i386-pc none of them are used.  I actually wonder if any of those
> work :-)
>
>   

Even on i386-pc, GRUB_CONSOLE_KEY_* are only used once in the 
translation table of grub_console_getkey(). The defines are not part of 
the console interface definition.

I would suggest to remove the defines from all 
include/grub/PLATFORM/console.h. The GRUB_CONSOLE_KEY_* in 
i386/pc/startup.S can be replaced with the constants itself.


> > ...
> >  
> > +  /* XXX: return GRUB_CONSOLE_KEY_* does not work here,
> > +     grub_cmdline_get() does not check for these constants.
> > +     At least on i386-pc, GRUB_CONSOLE_KEY_* are in fact keyboard
> > +     scancodes which are converted into control chars by
> > +     grub_console_getkey(). */
> > +
> >     
>
> I do not think this comment is needed.  You explained it in the
> email.  I do not like comments that explain why we aren't doing some
> things, it is usually better to explain why we do something if it is a
> hack or not obvious.  In this case I prefer no comment.
>
>   
> >    switch (c)
> >      {
> >      case KEY_LEFT:
> > -      c = GRUB_CONSOLE_KEY_LEFT;
> > +      c = 2; /*GRUB_CONSOLE_KEY_LEFT*/
> >        break;
> >     
>
> Please just remove the comment.  As this appears to be wrong anyways
> :-)
>
>   

Done.

Christian

2007-11-10  Christian Franke  <address@hidden>

        * util/console.c (grub_ncurses_getkey): Change curses KEY_* mapping,
        now return control chars instead of GRUB_CONSOLE_KEY_* constants.
        This fixes the problem that function keys did not work in grub-emu.



--- grub2.orig/util/console.c   2007-07-22 01:32:31.000000000 +0200
+++ grub2/util/console.c        2007-11-10 13:20:02.171875000 +0100
@@ -164,50 +164,50 @@ grub_ncurses_getkey (void)
   switch (c)
     {
     case KEY_LEFT:
-      c = GRUB_CONSOLE_KEY_LEFT;
+      c = 2;
       break;
 
     case KEY_RIGHT:
-      c = GRUB_CONSOLE_KEY_RIGHT;
+      c = 6;
       break;
       
     case KEY_UP:
-      c = GRUB_CONSOLE_KEY_UP;
+      c = 16;
       break;
 
     case KEY_DOWN:
-      c = GRUB_CONSOLE_KEY_DOWN;
+      c = 14;
       break;
 
     case KEY_IC:
-      c = GRUB_CONSOLE_KEY_IC;
+      c = 24;
       break;
 
     case KEY_DC:
-      c = GRUB_CONSOLE_KEY_DC;
+      c = 4;
       break;
 
     case KEY_BACKSPACE:
       /* XXX: For some reason ncurses on xterm does not return
         KEY_BACKSPACE.  */
     case 127: 
-      c = GRUB_CONSOLE_KEY_BACKSPACE;
+      c = 8;
       break;
 
     case KEY_HOME:
-      c = GRUB_CONSOLE_KEY_HOME;
+      c = 1;
       break;
 
     case KEY_END:
-      c = GRUB_CONSOLE_KEY_END;
+      c = 5;
       break;
 
     case KEY_NPAGE:
-      c = GRUB_CONSOLE_KEY_NPAGE;
+      c = 3;
       break;
 
     case KEY_PPAGE:
-      c = GRUB_CONSOLE_KEY_PPAGE;
+      c = 7;
       break;
     }