[PATCH v2] ui/cocoa: Comment about modifier key input quirks

[PATCH v2] ui/cocoa: Comment about modifier key input quirks

[Akihiko Odaki]

Based-on: <20210310042348.21931-1-akihiko.odaki@gmail.com>
Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 ui/cocoa.m | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 035f96aab04..3d292269c11 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -700,13 +700,35 @@ - (bool) handleEventLocked:(NSEvent *)event
     NSPoint p = [self screenLocationOfEvent:event];
     NSUInteger modifiers = [event modifierFlags];
 
-    // emulate caps lock keydown and keyup
+    /*
+     * If -[NSEvent modifierFlags] has NSEventModifierFlagCapsLock,
+     * toggle the current CapsLock state by firing "up" and "down" events in
+     * sequence.
+     */
     if (!!(modifiers & NSEventModifierFlagCapsLock) !=
         qkbd_state_modifier_get(kbd, QKBD_MOD_CAPSLOCK)) {
         qkbd_state_key_event(kbd, Q_KEY_CODE_CAPS_LOCK, true);
         qkbd_state_key_event(kbd, Q_KEY_CODE_CAPS_LOCK, false);
     }
 
+    /*
+     * Check for other flags of -[NSEvent modifierFlags].
+     *
+     * If a flag is not set, fire "up" events for all keys which correspond to
+     * the flag. Note that "down" events are not fired here because the flags
+     * checked here do not tell what exact keys are down.
+     *
+     * These operations are performed for any events because the modifier state
+     * may change while the application is inactive (i.e. no events fire) and
+     * we want to detect it as early as possible.
+     *
+     * Up events corresponding to a modifier flag update the current modifier
+     * state tracked with QKbdState but they are not fired unless all keys which
+     * match to the flag are up. Therefore, it cannot synchornize Cocoa and
+     * QkbdState if one of the keys is down. It is still nice that any
+     * desynchronization can be fixed by completely leaving your hands from the
+     * keyboard.
+     */
     if (!(modifiers & NSEventModifierFlagShift)) {
         qkbd_state_key_event(kbd, Q_KEY_CODE_SHIFT, false);
         qkbd_state_key_event(kbd, Q_KEY_CODE_SHIFT_R, false);
@@ -726,6 +748,14 @@ - (bool) handleEventLocked:(NSEvent *)event
 
     switch ([event type]) {
         case NSEventTypeFlagsChanged:
+            /*
+             * If the state of the key corresponding to -[NSEvent keyCode] is
+             * not updated by checking -[NSEvent modifierFlags], update it here.
+             * It means -[NSEvent keyCode] does not represent CapsLock and
+             * the corresponding modifier flag is set.
+             * [self toggleKey] peeks QkbdState so it correctly works only when
+             * it is synchornized with Cocoa.
+             */
             switch ([event keyCode]) {
                 case kVK_Shift:
                     if (!!(modifiers & NSEventModifierFlagShift)) {
-- 
2.24.3 (Apple Git-128)

Re: [PATCH v2] ui/cocoa: Comment about modifier key input quirks

[Gerd Hoffmann]

On Fri, Mar 12, 2021 at 12:12:03AM +0900, Akihiko Odaki wrote:
> Based-on: <20210310042348.21931-1-akihiko.odaki@gmail.com>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>

Well, the comments should not describe what the code is doing, that is
usually pretty clear from reading the code.  The comment should say
*why* the code is doing that, i.e. describe the problem we have to deal
with.

In this case specifically the NSEventTypeFlagsChanged event quirks we
have to deal with:

  (1) we get the keyCode but *not* whenever it was a up or down event.
  (2) we get the modifierFlags but we have only one bit for both shift
      keys so you can't tell whenever the left or right or both shift
      keys are down.

We handle this by using the modifierFlags plus our own state tracking to
generate "up" events, and we have to check both keyCode and
modifierFlags for "down" events.

> -    // emulate caps lock keydown and keyup
> +    /*
> +     * If -[NSEvent modifierFlags] has NSEventModifierFlagCapsLock,
> +     * toggle the current CapsLock state by firing "up" and "down" events in
> +     * sequence.
> +     */

Not very helpful.

> +    /*
> +     * Check for other flags of -[NSEvent modifierFlags].
> +     *
> +     * If a flag is not set, fire "up" events for all keys which correspond to
> +     * the flag. Note that "down" events are not fired here because the flags
> +     * checked here do not tell what exact keys are down.
> +     *
> +     * These operations are performed for any events because the modifier state
> +     * may change while the application is inactive (i.e. no events fire) and
> +     * we want to detect it as early as possible.

Ah, right, (3) for the list above: no updates for inactive apps.

> +     * Up events corresponding to a modifier flag update the current modifier
> +     * state tracked with QKbdState but they are not fired unless all keys which
> +     * match to the flag are up. Therefore, it cannot synchornize Cocoa and
> +     * QkbdState if one of the keys is down. It is still nice that any
> +     * desynchronization can be fixed by completely leaving your hands from the
> +     * keyboard.
> +     */

Better, but description of the NSEventTypeFlagsChanged event issues
should be added to make the motivation for that logic clear.  Feel free
to cut+paste from my lines above.

take care,
  Gerd

[PATCH v2] ui/cocoa: Comment about modifier key input quirks

[Akihiko Odaki]

Based-on: <20210310042348.21931-1-akihiko.odaki@gmail.com>
Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 ui/cocoa.m | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 035f96aab04..35fdc92a51f 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -700,7 +700,43 @@ - (bool) handleEventLocked:(NSEvent *)event
     NSPoint p = [self screenLocationOfEvent:event];
     NSUInteger modifiers = [event modifierFlags];
 
-    // emulate caps lock keydown and keyup
+    /*
+     * Check -[NSEvent modifierFlags] here.
+     *
+     * There is a NSEventType for an event notifying the change of
+     * -[NSEvent modifierFlags], NSEventTypeFlagsChanged but these operations
+     * are performed for any events because a modifier state may change while
+     * the application is inactive (i.e. no events fire) and we don't want to
+     * wait for another modifier state change to detect such a change.
+     *
+     * NSEventModifierFlagCapsLock requires a special treatment. The other flags
+     * are handled in similar manners.
+     *
+     * NSEventModifierFlagCapsLock
+     * ---------------------------
+     *
+     * If CapsLock state is changed, "up" and "down" events will be fired in
+     * sequence, effectively updates CapsLock state on the guest.
+     *
+     * The other flags
+     * ---------------
+     *
+     * If a flag is not set, fire "up" events for all keys which correspond to
+     * the flag. Note that "down" events are not fired here because the flags
+     * checked here do not tell what exact keys are down.
+     *
+     * If one of the keys corresponding to a flag is down, we rely on
+     * -[NSEvent keyCode] of an event whose -[NSEvent type] is
+     * NSEventTypeFlagsChanged to know the exact key which is down, which has
+     * the following two downsides:
+     * - It does not work when the application is inactive as described above.
+     * - It malfactions *after* the modifier state is changed while the
+     *   application is inactive. It is because -[NSEvent keyCode] does not tell
+     *   if the key is up or down, and requires to infer the current state from
+     *   the previous state. It is still possible to fix such a malfanction by
+     *   completely leaving your hands from the keyboard, which hopefully makes
+     *   this implementation usable enough.
+     */
     if (!!(modifiers & NSEventModifierFlagCapsLock) !=
         qkbd_state_modifier_get(kbd, QKBD_MOD_CAPSLOCK)) {
         qkbd_state_key_event(kbd, Q_KEY_CODE_CAPS_LOCK, true);
-- 
2.24.3 (Apple Git-128)