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

[Marco Gerards]

Christian Franke <address@hidden> writes:

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

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

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

--
Marco