Fix grub-emu curses KEY_* mapping
PATCH 1/1 Fix grub-emu curses KEY_* mapping
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 <[email protected]>
* 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.
util/console.c +17 -11
@@ -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 ; }
Quoted 2 lines · Christian Franke
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 :(
Quoted 4 lines
Another approach would be to remove the scancode translation in
grub_console_getkey(), and check for GRUB_CONSOLE_KEY_* ingrub_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 :-)
Quoted 12 lines
Christian
2007-11-02 Christian Franke <[email protected]>
*
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.
util/console.c grub_ncurses_getkey (void) · line 169
Quoted 9 lines
util/console.c +6
-0@@ -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.
util/console.c grub_ncurses_getkey (void) · line 174
Quoted 6 lines
util/console.c +1
-1switch (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
Quoted 15 lines · Marco Gerards
Quoted 3 lines · Christian Franke
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 :(
Quoted 5 lines
Another approach would be to remove the scancode translation in
grub_console_getkey(), and check for GRUB_CONSOLE_KEY_* ingrub_cmdline_get()instead. The GRUB_CONSOLE_KEY_* definitions are already platform specific.
Quoted 5 lines
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.
Quoted 24 lines
Review
Quoted 9 lines
...
(quoted diff) +6
-0+ /* 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.
Review
Quoted 7 lines
switch (c) { case KEY_LEFT:(quoted diff) +1
-1- c = GRUB_CONSOLE_KEY_LEFT; + c = 2; /*GRUB_CONSOLE_KEY_LEFT*/ break;
Quoted 4 lines
Please just remove the comment. As this appears to be wrong anyways :-)
Done.
Christian
2007-11-10 Christian Franke <[email protected]>
* 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.
util/console.c +11 -11
@@ -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 ; }
Quoted 23 lines · Christian Franke
Quoted 15 lines · Marco Gerards
Quoted 3 lines · Christian Franke
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 :(
Quoted 5 lines
Another approach would be to remove the scancode translation in
grub_console_getkey(), and check for GRUB_CONSOLE_KEY_* ingrub_cmdline_get()instead. The GRUB_CONSOLE_KEY_* definitions are already platform specific.Quoted 5 lines
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 :-)
Quoted 7 lines
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_* ini386/pc/startup.Scan 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.
Quoted 30 lines
Quoted 23 lines
Review
Quoted 8 lines
...
+ /* XXX: return GRUB_CONSOLE_KEY_* does not work here,(quoted diff) +5
-0+ 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.
util/console.c grub_ncurses_getkey (void) · line 168
Quoted 7 lines
switch (c) { case KEY_LEFT:util/console.c +1
-1- c =GRUB_CONSOLE_KEY_LEFT ; + c =2 ;/*GRUB_CONSOLE_KEY_LEFT*/ break ;Quoted 4 lines
Please just remove the comment. As this appears to be wrong anyways :-)
Done.
Quoted 7 lines
Christian
2007-11-10 Christian Franke <[email protected]>
*
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
Quoted 8 lines · Marco Gerards
Quoted 6 lines
2007-11-10 Christian Franke <[email protected]>
*
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.
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 /.)