[PATCH] ui/cocoa: Do not raise keys before QEMU resigns active

[PATCH] ui/cocoa: Do not raise keys before QEMU resigns active

[Akihiko Odaki]

ui/cocoa used to raise all keys before it resigns active to prevent a
stuck key problem caused by key up events it does not see while it is
inactive. The problem is solved by checking -[NSEvent modifierFlags] in
commit 6d73bb643aa725348aabe6a885ac5fb0b7f70252, which is better
because it handles the case that key *down* events are missed while it
is inactive.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 ui/cocoa.m | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index a7848ae0a30..ac8989947f5 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -326,7 +326,6 @@ - (BOOL) isAbsoluteEnabled;
 - (float) cdx;
 - (float) cdy;
 - (QEMUScreen) gscreen;
-- (void) raiseAllKeys;
 @end
 
 QemuCocoaView *cocoaView;
@@ -996,18 +995,6 @@ - (BOOL) isAbsoluteEnabled {return isAbsoluteEnabled;}
 - (float) cdx {return cdx;}
 - (float) cdy {return cdy;}
 - (QEMUScreen) gscreen {return screen;}
-
-/*
- * Makes the target think all down keys are being released.
- * This prevents a stuck key problem, since we will not see
- * key up events for those keys after we have lost focus.
- */
-- (void) raiseAllKeys
-{
-    with_iothread_lock(^{
-        qkbd_state_lift_all_keys(kbd);
-    });
-}
 @end
 
 
@@ -1143,13 +1130,6 @@ - (BOOL)windowShouldClose:(id)sender
     return NO;
 }
 
-/* Called when QEMU goes into the background */
-- (void) applicationWillResignActive: (NSNotification *)aNotification
-{
-    COCOA_DEBUG("QemuCocoaAppController: applicationWillResignActive\n");
-    [cocoaView raiseAllKeys];
-}
-
 /* We abstract the method called by the Enter Fullscreen menu item
  * because Mac OS 10.7 and higher disables it. This is because of the
  * menu item's old selector's name toggleFullScreen:
-- 
2.24.3 (Apple Git-128)

Re: [PATCH] ui/cocoa: Do not raise keys before QEMU resigns active

[BALATON Zoltan]

On Sun, 14 Mar 2021, Akihiko Odaki wrote:
> ui/cocoa used to raise all keys before it resigns active to prevent a
> stuck key problem caused by key up events it does not see while it is
> inactive. The problem is solved by checking -[NSEvent modifierFlags] in
> commit 6d73bb643aa725348aabe6a885ac5fb0b7f70252, which is better
> because it handles the case that key *down* events are missed while it
> is inactive.

Does that commit also handle other keys than modifier keys? It's unlikely 
those get stuck but is that possible? The most likely case to happen is 
pressing Cmd+Tab to switch away and Cmd may get stuck. Can Tab get also 
stuck? Or what if you hold down space or some other key while switching 
away either with Cmd+Tab or with the mouse? That may not be a common case 
but is this here to handle that or they are already handled elsewhere so 
this is really not necessary to prevent stuck keys?

Regards,
BALATON Zoltan

> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
> ---
> ui/cocoa.m | 20 --------------------
> 1 file changed, 20 deletions(-)
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index a7848ae0a30..ac8989947f5 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -326,7 +326,6 @@ - (BOOL) isAbsoluteEnabled;
> - (float) cdx;
> - (float) cdy;
> - (QEMUScreen) gscreen;
> -- (void) raiseAllKeys;
> @end
>
> QemuCocoaView *cocoaView;
> @@ -996,18 +995,6 @@ - (BOOL) isAbsoluteEnabled {return isAbsoluteEnabled;}
> - (float) cdx {return cdx;}
> - (float) cdy {return cdy;}
> - (QEMUScreen) gscreen {return screen;}
> -
> -/*
> - * Makes the target think all down keys are being released.
> - * This prevents a stuck key problem, since we will not see
> - * key up events for those keys after we have lost focus.
> - */
> -- (void) raiseAllKeys
> -{
> -    with_iothread_lock(^{
> -        qkbd_state_lift_all_keys(kbd);
> -    });
> -}
> @end
>
>
> @@ -1143,13 +1130,6 @@ - (BOOL)windowShouldClose:(id)sender
>     return NO;
> }
>
> -/* Called when QEMU goes into the background */
> -- (void) applicationWillResignActive: (NSNotification *)aNotification
> -{
> -    COCOA_DEBUG("QemuCocoaAppController: applicationWillResignActive\n");
> -    [cocoaView raiseAllKeys];
> -}
> -
> /* We abstract the method called by the Enter Fullscreen menu item
>  * because Mac OS 10.7 and higher disables it. This is because of the
>  * menu item's old selector's name toggleFullScreen:
>

Re: [PATCH] ui/cocoa: Do not raise keys before QEMU resigns active

[Akihiko Odaki]

2021年3月14日(日) 21:55 BALATON Zoltan <balaton@eik.bme.hu>:
>
> On Sun, 14 Mar 2021, Akihiko Odaki wrote:
> > ui/cocoa used to raise all keys before it resigns active to prevent a
> > stuck key problem caused by key up events it does not see while it is
> > inactive. The problem is solved by checking -[NSEvent modifierFlags] in
> > commit 6d73bb643aa725348aabe6a885ac5fb0b7f70252, which is better
> > because it handles the case that key *down* events are missed while it
> > is inactive.
>
> Does that commit also handle other keys than modifier keys? It's unlikely
> those get stuck but is that possible? The most likely case to happen is
> pressing Cmd+Tab to switch away and Cmd may get stuck. Can Tab get also
> stuck? Or what if you hold down space or some other key while switching
> away either with Cmd+Tab or with the mouse? That may not be a common case
> but is this here to handle that or they are already handled elsewhere so
> this is really not necessary to prevent stuck keys?

I experimented with Cmd+Tab and ui/cocoa did not respond to it at all.
Apparently events for the particular key combination is not delivered
to the application.

If you switch away while holding space or some other key, the key
remains pressed on the guest, but the guest will receive a key up
event as soon as the key are released. If an event with
NSEventTypeKeyUp is fired, a corresponding event with
NSEventTypeKeyDown will be fired eventually whether the application
remains active or not. Events with NSEventTypeFlagsChanged do not have
such assurance and the modifier state tracking requires some tricks
like the new -[NSEvent modifierFlags] checks or the obsolete solution
deleted with this patch.

Actually -[QemuCocoaView raiseAllKeys] before introducing the new
-[NSEvent modifierFlags] checks only dealt with modifiers (event
though its name suggests it raises all keys) so there should be no
regression.

Regards,
Akihiko Odaki

>
> Regards,
> BALATON Zoltan
>
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
> > ---
> > ui/cocoa.m | 20 --------------------
> > 1 file changed, 20 deletions(-)
> >
> > diff --git a/ui/cocoa.m b/ui/cocoa.m
> > index a7848ae0a30..ac8989947f5 100644
> > --- a/ui/cocoa.m
> > +++ b/ui/cocoa.m
> > @@ -326,7 +326,6 @@ - (BOOL) isAbsoluteEnabled;
> > - (float) cdx;
> > - (float) cdy;
> > - (QEMUScreen) gscreen;
> > -- (void) raiseAllKeys;
> > @end
> >
> > QemuCocoaView *cocoaView;
> > @@ -996,18 +995,6 @@ - (BOOL) isAbsoluteEnabled {return isAbsoluteEnabled;}
> > - (float) cdx {return cdx;}
> > - (float) cdy {return cdy;}
> > - (QEMUScreen) gscreen {return screen;}
> > -
> > -/*
> > - * Makes the target think all down keys are being released.
> > - * This prevents a stuck key problem, since we will not see
> > - * key up events for those keys after we have lost focus.
> > - */
> > -- (void) raiseAllKeys
> > -{
> > -    with_iothread_lock(^{
> > -        qkbd_state_lift_all_keys(kbd);
> > -    });
> > -}
> > @end
> >
> >
> > @@ -1143,13 +1130,6 @@ - (BOOL)windowShouldClose:(id)sender
> >     return NO;
> > }
> >
> > -/* Called when QEMU goes into the background */
> > -- (void) applicationWillResignActive: (NSNotification *)aNotification
> > -{
> > -    COCOA_DEBUG("QemuCocoaAppController: applicationWillResignActive\n");
> > -    [cocoaView raiseAllKeys];
> > -}
> > -
> > /* We abstract the method called by the Enter Fullscreen menu item
> >  * because Mac OS 10.7 and higher disables it. This is because of the
> >  * menu item's old selector's name toggleFullScreen:
> >

Re: [PATCH] ui/cocoa: Do not raise keys before QEMU resigns active

[Gerd Hoffmann]

On Sun, Mar 14, 2021 at 04:01:47PM +0900, Akihiko Odaki wrote:
> ui/cocoa used to raise all keys before it resigns active to prevent a
> stuck key problem caused by key up events it does not see while it is
> inactive. The problem is solved by checking -[NSEvent modifierFlags] in
> commit 6d73bb643aa725348aabe6a885ac5fb0b7f70252, which is better
> because it handles the case that key *down* events are missed while it
> is inactive.

Well, I think it is a good idea to virtually lift all keys when the
application goes inactive.  Does this cause any actual problems?

Worst case should be that we send an extra keyup + keydown if the
qemu goes through a active -> inactive -> active cycle while a modifier
key is down, which you probably wouldn't even notice unless you log all
key events inside the guest.

take care,
  Gerd

Re: [PATCH] ui/cocoa: Do not raise keys before QEMU resigns active

[Akihiko Odaki]

2021年3月15日(月) 16:32 Gerd Hoffmann <kraxel@redhat.com>:
>
> On Sun, Mar 14, 2021 at 04:01:47PM +0900, Akihiko Odaki wrote:
> > ui/cocoa used to raise all keys before it resigns active to prevent a
> > stuck key problem caused by key up events it does not see while it is
> > inactive. The problem is solved by checking -[NSEvent modifierFlags] in
> > commit 6d73bb643aa725348aabe6a885ac5fb0b7f70252, which is better
> > because it handles the case that key *down* events are missed while it
> > is inactive.
>
> Well, I think it is a good idea to virtually lift all keys when the
> application goes inactive.  Does this cause any actual problems?

No, but ui/cocoa already has so many states and I don't think there is
a room for extra complexity, especially when considering the code
checking -[NSEvent modifierFlags]. Keyboard event management concerns
iothread mutex, different handling of modifier and normal keys and
some special keyboard shortcuts. -[QemuCocoaView raiseAllKeys]
introduces exceptional behaviors to them just to raise normal keys,
something ui/cocoa didn't before introducing QemuKbdState.

Regards,
Akihiko Odaki

>
> Worst case should be that we send an extra keyup + keydown if the
> qemu goes through a active -> inactive -> active cycle while a modifier
> key is down, which you probably wouldn't even notice unless you log all
> key events inside the guest.
>
> take care,
>   Gerd
>