| Message ID | 20210314070147.34731-1-akihiko.odaki@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ui/cocoa: Do not raise keys before QEMU resigns active | expand |
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: >
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: > >
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
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 >
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:
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(-)