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

Commit Message

Akihiko Odaki March 11, 2021, 3:12 p.m. UTC

Based-on: <[e-mail]>
Signed-off-by: Akihiko Odaki <[e-mail]>
---
 ui/cocoa.m | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

Comments

Gerd Hoffmann March 12, 2021, 10:19 a.m. UTC | #1

On Fri, Mar 12, 2021 at 12:12:03AM +0900, Akihiko Odaki wrote:
> Based-on: <[e-mail]>
> Signed-off-by: Akihiko Odaki <[e-mail]>

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

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)) {