[Qemu-devel] [PATCH 1/1] Improve Cocoa modifier key handling

[Qemu-devel] [PATCH 1/1] Improve Cocoa modifier key handling

[Ian McKellar]

I had two problems with QEMU on macOS:
 1) Sometimes when alt-tabbing to QEMU it would act as if the 'a' key
    was pressed so I'd get 'aaaaaaaaa....'.
 2) Using Sikuli to programatically send keys to the QEMU window text
    like "foo_bar" would come out as "fooa-bar".

They looked similar and after much digging the problem turned out to be
the same. When QEMU's ui/cocoa.m received an NSFlagsChanged NSEvent it
looked at the keyCode to determine what modifier key changed. This
usually works fine but sometimes the keyCode is 0 and the app should
instead be looking at the modifierFlags bitmask. Key code 0 is the 'a'
key.

I added code that handles keyCode == 0 differently. It checks the
modifierFlags and if they differ from QEMU's idea of which modifier
keys are currently pressed it toggles those changed keys.

This fixes my problems and seems work fine.

Signed-off-by: Ian McKellar <ianloic@google.com>
---
 ui/cocoa.m | 48 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 207555edf7..e645befa13 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -52,6 +52,8 @@
 /* macOS 10.12 deprecated many constants, #define the new names for older SDKs */
 #if MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_12
 #define NSEventMaskAny                  NSAnyEventMask
+#define NSEventModifierFlagCapsLock     NSAlphaShiftKeyMask
+#define NSEventModifierFlagShift        NSShiftKeyMask
 #define NSEventModifierFlagCommand      NSCommandKeyMask
 #define NSEventModifierFlagControl      NSControlKeyMask
 #define NSEventModifierFlagOption       NSAlternateKeyMask
@@ -536,6 +538,16 @@ QemuCocoaView *cocoaView;
     }
 }
 
+- (void) toggleModifier: (int)keycode {
+    if (modifiers_state[keycode] == 0) { // keydown
+        qemu_input_event_send_key_qcode(dcl->con, keycode, true);
+        modifiers_state[keycode] = 1;
+    } else { // keyup
+        qemu_input_event_send_key_qcode(dcl->con, keycode, false);
+        modifiers_state[keycode] = 0;
+    }
+}
+
 - (void) handleEvent:(NSEvent *)event
 {
     COCOA_DEBUG("QemuCocoaView: handleEvent\n");
@@ -547,7 +559,33 @@ QemuCocoaView *cocoaView;
 
     switch ([event type]) {
         case NSEventTypeFlagsChanged:
-            keycode = cocoa_keycode_to_qemu([event keyCode]);
+            if ([event keyCode] == 0) {
+                // When the Cocoa keyCode is zero that means keys should be
+                // synthesized based on the values in in the eventModifiers
+                // bitmask.
+
+                if (qemu_console_is_graphic(NULL)) {
+                    NSEventModifierFlags modifiers = [event modifierFlags];
+
+                    if (!!(modifiers & NSEventModifierFlagCapsLock) != !!modifiers_state[Q_KEY_CODE_CAPS_LOCK]) {
+                        [self toggleModifier:Q_KEY_CODE_CAPS_LOCK];
+                    }
+                    if (!!(modifiers & NSEventModifierFlagShift) != !!modifiers_state[Q_KEY_CODE_SHIFT]) {
+                        [self toggleModifier:Q_KEY_CODE_SHIFT];
+                    }
+                    if (!!(modifiers & NSEventModifierFlagControl) != !!modifiers_state[Q_KEY_CODE_CTRL]) {
+                        [self toggleModifier:Q_KEY_CODE_CTRL];
+                    }
+                    if (!!(modifiers & NSEventModifierFlagOption) != !!modifiers_state[Q_KEY_CODE_ALT]) {
+                        [self toggleModifier:Q_KEY_CODE_ALT];
+                    }
+                    if (!!(modifiers & NSEventModifierFlagCommand) != !!modifiers_state[Q_KEY_CODE_META_L]) {
+                        [self toggleModifier:Q_KEY_CODE_META_L];
+                    }
+                }
+            } else {
+                keycode = cocoa_keycode_to_qemu([event keyCode]);
+            }
 
             if ((keycode == Q_KEY_CODE_META_L || keycode == Q_KEY_CODE_META_R)
                && !isMouseGrabbed) {
@@ -562,13 +600,7 @@ QemuCocoaView *cocoaView;
                     qemu_input_event_send_key_qcode(dcl->con, keycode, true);
                     qemu_input_event_send_key_qcode(dcl->con, keycode, false);
                 } else if (qemu_console_is_graphic(NULL)) {
-                    if (modifiers_state[keycode] == 0) { // keydown
-                        qemu_input_event_send_key_qcode(dcl->con, keycode, true);
-                        modifiers_state[keycode] = 1;
-                    } else { // keyup
-                        qemu_input_event_send_key_qcode(dcl->con, keycode, false);
-                        modifiers_state[keycode] = 0;
-                    }
+                  [self toggleModifier:keycode];
                 }
             }
 
-- 
2.11.0.390.gc69c2f50cf-goog

Re: [Qemu-devel] [PATCH 1/1] Improve Cocoa modifier key handling

[Gerd Hoffmann]

  Hi,

> looked at the keyCode to determine what modifier key changed. This
> usually works fine but sometimes the keyCode is 0 and the app should
> instead be looking at the modifierFlags bitmask. Key code 0 is the
> 'a'
> key.
> 
> I added code that handles keyCode == 0 differently. It checks the
> modifierFlags and if they differ from QEMU's idea of which modifier
> keys are currently pressed it toggles those changed keys.

Sounds like this happens in case there is a modifier state change
without linked key event, such as state change while qemu did not have
the keyboard focus.  Nice that macos sends notifications in that case.

I'm wondering whenever we should just use modifierFlags all the time.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/1] Improve Cocoa modifier key handling

[Ian McKellar]

On Tue, May 23, 2017 at 3:03 AM Gerd Hoffmann <kraxel@redhat.com> wrote:

> Sounds like this happens in case there is a modifier state change
> without linked key event, such as state change while qemu did not have
> the keyboard focus.  Nice that macos sends notifications in that case.
>

Yeah, I guess it makes sense to send an event to update modifier state when
keyboard focus returns.

>
> I'm wondering whenever we should just use modifierFlags all the time.
>

Probably. My initial patch tried to be minimally intrusive but I can try
reworking the NSEventTypeFlagsChanged handling to compare the new
modifierFlags to the last we've seen and synthesize the appropriate key
down/up events.

Ian

Re: [Qemu-devel] [PATCH 1/1] Improve Cocoa modifier key handling

[Ian McKellar]

On Tue, May 23, 2017 at 8:52 AM Ian McKellar <ianloic@google.com> wrote:

> On Tue, May 23, 2017 at 3:03 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>>
>> I'm wondering whenever we should just use modifierFlags all the time.
>>
>
> Probably. My initial patch tried to be minimally intrusive but I can try
> reworking the NSEventTypeFlagsChanged handling to compare the new
> modifierFlags to the last we've seen and synthesize the appropriate key
> down/up events.
>
> After a little more experimentation I think that the approach in this
patch is the right one. modifierFlags doesn't[1] indicate which instance of
a modifier (ie: left or right) is being held. Except for
the NSEventTypeFlagsChanged that's synthesized when a window takes focus
the event's keyCode indicates which physical key is affected. That first
event after focus has keyCode==0 which we can detect because 0 is the
keyCode of the 'A' key which isn't a modifier.

Ian

[1] actually there are undocumented flags outside the
NSEventModifierFlagDeviceIndependentFlagsMask mask that indicate left/right
keys but I don't think we should use them.

Re: [Qemu-devel] [PATCH 1/1] Improve Cocoa modifier key handling

[Gerd Hoffmann]

  Hi,

> After a little more experimentation I think that the approach in this
> patch is the right one. modifierFlags doesn't[1] indicate which
> instance of a modifier (ie: left or right) is being held.

Ok, makes sense.  One more thing:  I think capslock must be handled
differently as keydown + keyup toggle state.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/1] Improve Cocoa modifier key handling

[Ian McKellar]

Sent another patch that does a better job of toggling caps-lock. I couldn't
make it fail with the old patch but I think the new patch is somewhat
better.

Ian

On Tue, May 23, 2017 at 11:17 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
>
> > After a little more experimentation I think that the approach in this
> > patch is the right one. modifierFlags doesn't[1] indicate which
> > instance of a modifier (ie: left or right) is being held.
>
> Ok, makes sense.  One more thing:  I think capslock must be handled
> differently as keydown + keyup toggle state.
>
> cheers,
>   Gerd
>
>

[Qemu-devel] [PATCH 1/1] Improve Cocoa modifier key handling

[G 3]

On May 13, 2017, at 11:58 AM, qemu-devel-request@nongnu.org wrote:

>
> I had two problems with QEMU on macOS:
>  1) Sometimes when alt-tabbing to QEMU it would act as if the 'a' key
>     was pressed so I'd get 'aaaaaaaaa....'.
>  2) Using Sikuli to programatically send keys to the QEMU window text
>     like "foo_bar" would come out as "fooa-bar".
>
> They looked similar and after much digging the problem turned out  
> to be
> the same. When QEMU's ui/cocoa.m received an NSFlagsChanged NSEvent it
> looked at the keyCode to determine what modifier key changed. This
> usually works fine but sometimes the keyCode is 0 and the app should
> instead be looking at the modifierFlags bitmask. Key code 0 is the 'a'
> key.
>
> I added code that handles keyCode == 0 differently. It checks the
> modifierFlags and if they differ from QEMU's idea of which modifier
> keys are currently pressed it toggles those changed keys.
>
> This fixes my problems and seems work fine.
>
> The patch itself is an attachment.
>
> Ian
> -------------- next part --------------
> A non-text attachment was scrubbed...
> Name: 0001-Improve-Cocoa-modifier-key-handling.patch
> Type: application/octet-stream
> Size: 4920 bytes
> Desc: not available
> URL: <http://lists.nongnu.org/archive/html/qemu-devel/attachments/ 
> 20170512/990496cd/attachment.obj>

Could you send your patch via email to the qemu developer list? I am  
interested in testing it. Attachments don't work here.

[Qemu-devel] [PATCH 1/1] Improve Cocoa modifier key handling

[Ian McKellar]

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

I had two problems with QEMU on macOS:
 1) Sometimes when alt-tabbing to QEMU it would act as if the 'a' key
    was pressed so I'd get 'aaaaaaaaa....'.
 2) Using Sikuli to programatically send keys to the QEMU window text
    like "foo_bar" would come out as "fooa-bar".

They looked similar and after much digging the problem turned out to be
the same. When QEMU's ui/cocoa.m received an NSFlagsChanged NSEvent it
looked at the keyCode to determine what modifier key changed. This
usually works fine but sometimes the keyCode is 0 and the app should
instead be looking at the modifierFlags bitmask. Key code 0 is the 'a'
key.

I added code that handles keyCode == 0 differently. It checks the
modifierFlags and if they differ from QEMU's idea of which modifier
keys are currently pressed it toggles those changed keys.

This fixes my problems and seems work fine.

The patch itself is an attachment.

Ian

[-- Attachment #2: 0001-Improve-Cocoa-modifier-key-handling.patch --]
[-- Type: application/octet-stream, Size: 4921 bytes --]

From d70f1ad147348008514e80e3dc417f6260e39aa2 Mon Sep 17 00:00:00 2001
From: Ian McKellar <ianloic@google.com>
Date: Fri, 12 May 2017 11:08:23 -0700
Subject: [PATCH 1/1] Improve Cocoa modifier key handling

I had two problems with QEMU on macOS:
 1) Sometimes when alt-tabbing to QEMU it would act as if the 'a' key
    was pressed so I'd get 'aaaaaaaaa....'.
 2) Using Sikuli to programatically send keys to the QEMU window text
    like "foo_bar" would come out as "fooa-bar".

They looked similar and after much digging the problem turned out to be
the same. When QEMU's ui/cocoa.m received an NSFlagsChanged NSEvent it
looked at the keyCode to determine what modifier key changed. This
usually works fine but sometimes the keyCode is 0 and the app should
instead be looking at the modifierFlags bitmask. Key code 0 is the 'a'
key.

I added code that handles keyCode == 0 differently. It checks the
modifierFlags and if they differ from QEMU's idea of which modifier
keys are currently pressed it toggles those changed keys.

This fixes my problems and seems work fine.

Signed-off-by: Ian McKellar <ianloic@google.com>
---
 ui/cocoa.m | 48 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 207555edf7..e645befa13 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -52,6 +52,8 @@
 /* macOS 10.12 deprecated many constants, #define the new names for older SDKs */
 #if MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_12
 #define NSEventMaskAny                  NSAnyEventMask
+#define NSEventModifierFlagCapsLock     NSAlphaShiftKeyMask
+#define NSEventModifierFlagShift        NSShiftKeyMask
 #define NSEventModifierFlagCommand      NSCommandKeyMask
 #define NSEventModifierFlagControl      NSControlKeyMask
 #define NSEventModifierFlagOption       NSAlternateKeyMask
@@ -536,6 +538,16 @@ QemuCocoaView *cocoaView;
     }
 }
 
+- (void) toggleModifier: (int)keycode {
+    if (modifiers_state[keycode] == 0) { // keydown
+        qemu_input_event_send_key_qcode(dcl->con, keycode, true);
+        modifiers_state[keycode] = 1;
+    } else { // keyup
+        qemu_input_event_send_key_qcode(dcl->con, keycode, false);
+        modifiers_state[keycode] = 0;
+    }
+}
+
 - (void) handleEvent:(NSEvent *)event
 {
     COCOA_DEBUG("QemuCocoaView: handleEvent\n");
@@ -547,7 +559,33 @@ QemuCocoaView *cocoaView;
 
     switch ([event type]) {
         case NSEventTypeFlagsChanged:
-            keycode = cocoa_keycode_to_qemu([event keyCode]);
+            if ([event keyCode] == 0) {
+                // When the Cocoa keyCode is zero that means keys should be
+                // synthesized based on the values in in the eventModifiers
+                // bitmask.
+
+                if (qemu_console_is_graphic(NULL)) {
+                    NSEventModifierFlags modifiers = [event modifierFlags];
+
+                    if (!!(modifiers & NSEventModifierFlagCapsLock) != !!modifiers_state[Q_KEY_CODE_CAPS_LOCK]) {
+                        [self toggleModifier:Q_KEY_CODE_CAPS_LOCK];
+                    }
+                    if (!!(modifiers & NSEventModifierFlagShift) != !!modifiers_state[Q_KEY_CODE_SHIFT]) {
+                        [self toggleModifier:Q_KEY_CODE_SHIFT];
+                    }
+                    if (!!(modifiers & NSEventModifierFlagControl) != !!modifiers_state[Q_KEY_CODE_CTRL]) {
+                        [self toggleModifier:Q_KEY_CODE_CTRL];
+                    }
+                    if (!!(modifiers & NSEventModifierFlagOption) != !!modifiers_state[Q_KEY_CODE_ALT]) {
+                        [self toggleModifier:Q_KEY_CODE_ALT];
+                    }
+                    if (!!(modifiers & NSEventModifierFlagCommand) != !!modifiers_state[Q_KEY_CODE_META_L]) {
+                        [self toggleModifier:Q_KEY_CODE_META_L];
+                    }
+                }
+            } else {
+                keycode = cocoa_keycode_to_qemu([event keyCode]);
+            }
 
             if ((keycode == Q_KEY_CODE_META_L || keycode == Q_KEY_CODE_META_R)
                && !isMouseGrabbed) {
@@ -562,13 +600,7 @@ QemuCocoaView *cocoaView;
                     qemu_input_event_send_key_qcode(dcl->con, keycode, true);
                     qemu_input_event_send_key_qcode(dcl->con, keycode, false);
                 } else if (qemu_console_is_graphic(NULL)) {
-                    if (modifiers_state[keycode] == 0) { // keydown
-                        qemu_input_event_send_key_qcode(dcl->con, keycode, true);
-                        modifiers_state[keycode] = 1;
-                    } else { // keyup
-                        qemu_input_event_send_key_qcode(dcl->con, keycode, false);
-                        modifiers_state[keycode] = 0;
-                    }
+                  [self toggleModifier:keycode];
                 }
             }
 
-- 
2.11.0.390.gc69c2f50cf-goog