[PATCH] Fix grub-emu curses KEY_* mapping

[PATCH] Fix grub-emu curses KEY_* mapping

[Christian Franke]

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

Curses function keys do not work in grub-emu, this patch fixes this in 
grub_ncurses_getkey().

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.

Christian

2007-11-02  Christian Franke  <franke@computer.org>

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



[-- Attachment #2: grub2-console-key.patch --]
[-- Type: text/x-patch, Size: 1836 bytes --]

--- grub2.orig/util/console.c	2007-07-22 01:32:31.000000000 +0200
+++ grub2/util/console.c	2007-10-13 16:13:46.000000000 +0200
@@ -161,53 +161,59 @@ grub_ncurses_getkey (void)
       c = getch ();
     }
 
+  /* 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(). */
+
   switch (c)
     {
     case KEY_LEFT:
-      c = GRUB_CONSOLE_KEY_LEFT;
+      c = 2; /*GRUB_CONSOLE_KEY_LEFT*/
       break;
 
     case KEY_RIGHT:
-      c = GRUB_CONSOLE_KEY_RIGHT;
+      c = 6; /*GRUB_CONSOLE_KEY_RIGHT*/
       break;
       
     case KEY_UP:
-      c = GRUB_CONSOLE_KEY_UP;
+      c = 16; /*GRUB_CONSOLE_KEY_UP*/
       break;
 
     case KEY_DOWN:
-      c = GRUB_CONSOLE_KEY_DOWN;
+      c = 14; /*GRUB_CONSOLE_KEY_DOWN*/
       break;
 
     case KEY_IC:
-      c = GRUB_CONSOLE_KEY_IC;
+      c = 24; /*GRUB_CONSOLE_KEY_IC*/
       break;
 
     case KEY_DC:
-      c = GRUB_CONSOLE_KEY_DC;
+      c = 4; /*GRUB_CONSOLE_KEY_DC*/
       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; /*GRUB_CONSOLE_KEY_BACKSPACE*/;
       break;
 
     case KEY_HOME:
-      c = GRUB_CONSOLE_KEY_HOME;
+      c = 1; /*GRUB_CONSOLE_KEY_HOME*/
       break;
 
     case KEY_END:
-      c = GRUB_CONSOLE_KEY_END;
+      c = 5; /*GRUB_CONSOLE_KEY_END*/
       break;
 
     case KEY_NPAGE:
-      c = GRUB_CONSOLE_KEY_NPAGE;
+      c = 3; /*GRUB_CONSOLE_KEY_NPAGE*/
       break;
 
     case KEY_PPAGE:
-      c = GRUB_CONSOLE_KEY_PPAGE;
+      c = 7; /*GRUB_CONSOLE_KEY_PPAGE*/
       break;
     }
 

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

[Marco Gerards]

Christian Franke <Christian.Franke@t-online.de> 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 :-)

> Christian
>
> 2007-11-02  Christian Franke  <franke@computer.org>
>
> 	* 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-10-13 16:13:46.000000000 +0200
> @@ -161,53 +161,59 @@ grub_ncurses_getkey (void)
>        c = getch ();
>      }
>  
> +  /* 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
:-)

Thanks,
Marco

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

[Christian Franke]

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

Marco Gerards wrote:

> Christian Franke <Christian.Franke@t-online.de> 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  <franke@computer.org>

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




[-- Attachment #2: grub2-console-key-2.patch --]
[-- Type: text/x-patch, Size: 1241 bytes --]

--- 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;
     }
 

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

[Marco Gerards]

Christian Franke <Christian.Franke@t-online.de> writes:

> Marco Gerards wrote:
>
>> Christian Franke <Christian.Franke@t-online.de> 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.

The other definitions have to be used.  I do not like using constant
values in asm/C code, instead I like using macros for this because in
that case its meaning is obvious.

>>> ...
>>>  +  /* 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  <franke@computer.org>
>
> 	* 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.

This looks fine to me.  Now we have to wait for the paperwork :-/

--
Marco

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

[Robert Millan]

On Sat, Nov 10, 2007 at 04:25:56PM +0100, Marco Gerards wrote:
> >
> > 2007-11-10  Christian Franke  <franke@computer.org>
> >
> > 	* 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.
> 
> This looks fine to me.  Now we have to wait for the paperwork :-/

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