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

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

[Programmingkid]

[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]

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. 

Signed-off-by: John Arbuckle <programmingkidx@gmail.com>

---
 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];
 
 // 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 */
+
+    /* 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);
+    }
+}
+
 - (void)startEmulationWithArgc:(int)argc argv:(char**)argv
 {
     COCOA_DEBUG("QemuCocoaAppController: startEmulationWithArgc\n");
-- 
1.7.5.4


[-- Attachment #2: Type: text/html, Size: 10335 bytes --]

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

[Peter Maydell]

On 18 September 2015 at 22:46, Programmingkid <programmingkidx@gmail.com> 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.
>
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>
> ---
>  ui/cocoa.m |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)

Can you please fix how you're sending patch emails not to send
them as HTML? This really does mess up some of the tools we
use to process patchmail, and it's a pain to have to handle
them manually (also error-prone; last patch of yours I
applied I messed up fixing the authorship on).

The git send-email tool is one way of doing this.

thanks
-- PMM

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

[Programmingkid]

On Sep 19, 2015, at 11:07 AM, Peter Maydell wrote:

> On 18 September 2015 at 22:46, Programmingkid <programmingkidx@gmail.com> 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.
>> 
>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>> 
>> ---
>> ui/cocoa.m |   17 ++++++++++++++++-
>> 1 files changed, 16 insertions(+), 1 deletions(-)
> 
> Can you please fix how you're sending patch emails not to send
> them as HTML?

I just copy and paste them into Apple's Mail application. Didn't know it makes it into html.

> This really does mess up some of the tools we
> use to process patchmail, and it's a pain to have to handle
> them manually (also error-prone; last patch of yours I
> applied I messed up fixing the authorship on).
> 
> The git send-email tool is one way of doing this.

I will learn this send-email tool today. 

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

[Peter Maydell]

On 18 September 2015 at 14:46, Programmingkid <programmingkidx@gmail.com> 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.

> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>
> ---
>  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?


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


> +
> +    /* 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);
        }
    }

thanks
-- PMM

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 <programmingkidx@gmail.com> 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 <programmingkidx@gmail.com>
>> 
>> ---
>> 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.

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

[Peter Maydell]

On 23 September 2015 at 14:44, Programmingkid <programmingkidx@gmail.com> wrote:
>
> On Sep 23, 2015, at 2:04 PM, Peter Maydell wrote:
>
>> On 18 September 2015 at 14:46, Programmingkid <programmingkidx@gmail.com> 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 <programmingkidx@gmail.com>
>>>
>>> ---
>>> 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?

I don't feel strongly about where things should live, globals just
make me think we could arrange things better somehow.

-- PMM