Re: [Qemu-devel] [PATCH] ui/cocoa.m: prevent stuck key situation

[Programmingkid]

On Sep 23, 2015, at 2:04 PM, Peter Maydell wrote:

> On 18 September 2015 at 14:46, Programmingkid <address@hidden> wrote:
>> When the user puts QEMU in the background while holding down a key, QEMU
>> will
>> not receive the keyup event when the user lets go of the key. When the user
>> goes
>> back to QEMU, QEMU will think the key is still down causing stuck key
>> symptoms.
>> This patch fixes this problem by releasing all keys when QEMU goes into the
>> background.
> 
> Looks like maybe you're not wrapping lines early enough in your
> commit messages, resulting in this ugly effect when they're
> quoted. It's best to not have lines longer than 75 chars or so.

Sorry, will make the lines shorter in the future.

> 
>> Signed-off-by: John Arbuckle <address@hidden>
>> 
>> ---
>> ui/cocoa.m |   17 ++++++++++++++++-
>> 1 files changed, 16 insertions(+), 1 deletions(-)
>> 
>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>> index 334e6f6..d07b22d 100644
>> --- a/ui/cocoa.m
>> +++ b/ui/cocoa.m
>> @@ -69,6 +69,7 @@ char **gArgv;
>> bool stretch_video;
>> NSTextField *pauseLabel;
>> NSArray * supportedImageFileTypes;
>> +int modifiers_state[256];
> 
> Rather than making this global, could we have the applicationWillResignActive
> method call a new raiseAllKeys method on the NSView?

We could do that, but isn't this more of an app controller function?

>> 
>> 
>> 
>> // keymap conversion
>> int keymap[] =
>> @@ -274,7 +275,6 @@ static void handleAnyDeviceErrors(Error * err)
>>     NSWindow *fullScreenWindow;
>>     float cx,cy,cw,ch,cdx,cdy;
>>     CGDataProviderRef dataProviderRef;
>> -    int modifiers_state[256];
>>     BOOL isMouseGrabbed;
>>     BOOL isFullscreen;
>>     BOOL isAbsoluteEnabled;
>> @@ -933,6 +933,21 @@ QemuCocoaView *cocoaView;
>>     return YES;
>> }
>> 
>> 
>> 
>> +/* Called when QEMU goes into the background */
>> +- (void) applicationWillResignActive: (NSNotification *)aNotification
>> +{
>> +    COCOA_DEBUG("QemuCocoaAppController: applicationWillResignActive\n");
>> +    int keycode, index;
>> +    const int max_index = 126; /* This is the size of the keymap array */
> 
> Hardcoding array sizes is never a good idea. We have an ARRAY_SIZE
> macro which automatically gets it right.

Didn't know about this macro. Will use it in the next patch.

> 
> 
>> +
>> +    /* Release all the keys to prevent a stuck key situation */
>> +    for(index = 0; index <= max_index; index++) {
>> +        keycode = keymap[index];
>> +        modifiers_state[keycode] = 0;
>> +        qemu_input_event_send_key_number(dcl->con, keycode, false);
>> +    }
> 
> This will send key-up events even for keys which are already up.
> Instead you can just send events for only the keys which are down
> (and avoid the lookup in keymap[] too):
> 
> 
>    for (i = 0; i < ARRAY_SIZE(modifiers_state); i++) {
>        if (modifiers_state[i])) {
>            modifiers_state[i] = 0;
>            qemu_input_event_send_key_number(dcl->con, i, false);
>        }
>    }

Sounds good. Will use it.