[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] ui/cocoa.m: Replace pc/xt keyboard keycode arra
From: |
Programmingkid |
Subject: |
Re: [Qemu-devel] [PATCH] ui/cocoa.m: Replace pc/xt keyboard keycode array with QKeyCode |
Date: |
Tue, 1 Mar 2016 20:20:47 -0500 |
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);
Re: [Qemu-devel] [PATCH] ui/cocoa.m: Replace pc/xt keyboard keycode array with QKeyCode, Peter Maydell, 2016/03/01