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

From: Akihiko Odaki <akihiko.odaki@gmail.com>
To: BALATON Zoltan <balaton@eik.bme.hu>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	qemu Developers <qemu-devel@nongnu.org>,
	Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [PATCH] ui/cocoa: Do not raise keys before QEMU resigns active
Date: Mon, 15 Mar 2021 03:07:21 +0900	[thread overview]
Message-ID: <CAMVc7JWUkDNtg23Z1H4TKfKzTixRB7=2wRtRfBocYSQ3gXY+WA@mail.gmail.com> (raw)
In-Reply-To: <924e0f6-1981-8c7f-4568-a03c2b17d02c@eik.bme.hu>

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:
> >