Re: [Qemu-devel] [PATCH] ui/cocoa.m: Replace pc/xt keyboard keycode array with QKeyCode

[Programmingkid]

On Mar 1, 2016, at 7:25 PM, Eric Blake wrote:

> On 03/01/2016 05:12 PM, Programmingkid wrote:
> 
>>> 
>>>> +
>>>> +    [MAC_KEY_ESC] = Q_KEY_CODE_ESC,
>>>> +    //[MAC_KEY_F1] = Q_KEY_CODE_POWER, // Just in case you need the power 
>>>> key
>>>> +    [MAC_KEY_F1] = Q_KEY_CODE_F1,
>>> 
>>> The comment looks weird. Probably worth a mention in the commit message
>>> why you need it.
>> 
>> Maybe this:
>> 
>> [MAC_KEY_ESC] = Q_KEY_CODE_ESC,
>> //[MAC_KEY_F1] = Q_KEY_CODE_POWER, // You might need this key if you use 
>> Macsbug. 
>> [MAC_KEY_F1] = Q_KEY_CODE_F1,
> 
> Not a code comment, but a commit message comment stating why you aren't
> providing a mapping for q_KEY_CODE_POWER.  For that matter, I don't
> think a code comment is useful, just nuke the entire line (the comment
> doesn't add anything).
> 
>> 
>> 
>>> 
>>>> 
>>>> static int cocoa_keycode_to_qemu(int keycode)
>>>> {
>>>> -    if (ARRAY_SIZE(keymap) <= keycode) {
>>>> +    if (ARRAY_SIZE(macToQKeyCodeMap) <= keycode) {
>>>>        fprintf(stderr, "(cocoa) warning unknown keycode 0x%x\n", keycode);
>>> 
>>> Pre-existing, but we should fix this to avoid fprintf.
>> 
>> What do you mean by pre-existing?
> 
> You weren't the original cause of the bug, so it is not necessarily this
> patch's job to fix the bug.  Therefore, "pre-existing".  But since the
> bug was observed during review of your patch, you may want to fix it
> anyways, probably as a separate patch.

So you want this:

if (ARRAY_SIZE(macToQKeyCodeMap) <= keycode) {
       error_report("(cocoa) warning unknown keycode 0x%x\n", keycode);