* [PATCH] Fix grub-emu curses KEY_* mapping
@ 2007-11-02 21:55 Christian Franke
2007-11-09 15:17 ` Marco Gerards
0 siblings, 1 reply; 5+ messages in thread
From: Christian Franke @ 2007-11-02 21:55 UTC (permalink / raw)
To: grub-devel
[-- 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;
}
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Fix grub-emu curses KEY_* mapping
2007-11-02 21:55 [PATCH] Fix grub-emu curses KEY_* mapping Christian Franke
@ 2007-11-09 15:17 ` Marco Gerards
2007-11-10 13:09 ` Christian Franke
0 siblings, 1 reply; 5+ messages in thread
From: Marco Gerards @ 2007-11-09 15:17 UTC (permalink / raw)
To: The development of GRUB 2
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Fix grub-emu curses KEY_* mapping
2007-11-09 15:17 ` Marco Gerards
@ 2007-11-10 13:09 ` Christian Franke
2007-11-10 15:25 ` Marco Gerards
0 siblings, 1 reply; 5+ messages in thread
From: Christian Franke @ 2007-11-10 13:09 UTC (permalink / raw)
To: The development of GRUB 2
[-- 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;
}
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Fix grub-emu curses KEY_* mapping
2007-11-10 13:09 ` Christian Franke
@ 2007-11-10 15:25 ` Marco Gerards
2007-11-18 7:20 ` Robert Millan
0 siblings, 1 reply; 5+ messages in thread
From: Marco Gerards @ 2007-11-10 15:25 UTC (permalink / raw)
To: The development of GRUB 2
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Fix grub-emu curses KEY_* mapping
2007-11-10 15:25 ` Marco Gerards
@ 2007-11-18 7:20 ` Robert Millan
0 siblings, 0 replies; 5+ messages in thread
From: Robert Millan @ 2007-11-18 7:20 UTC (permalink / raw)
To: The development of GRUB 2
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 /.)
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-11-18 7:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-11-02 21:55 [PATCH] Fix grub-emu curses KEY_* mapping Christian Franke
2007-11-09 15:17 ` Marco Gerards
2007-11-10 13:09 ` Christian Franke
2007-11-10 15:25 ` Marco Gerards
2007-11-18 7:20 ` Robert Millan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.