| Message ID | 20190214102816.3393-6-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ui/cocoa: Use OSX's main loop | expand |
On Thu, 14 Feb 2019, Peter Maydell wrote: > Currently the handleEvent method will directly call the NSApp > sendEvent method for any events that we want to let OSX deal > with. When we rearrange the event handling code, the way that > we say "let OSX have this event" is going to change. Prepare > for that by refactoring so that handleEvent returns a flag > indicating whether it consumed the event. > > Suggested-by: BALATON Zoltan <balaton@eik.bme.hu> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > New patch in v2 > --- > ui/cocoa.m | 49 ++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 34 insertions(+), 15 deletions(-) > > diff --git a/ui/cocoa.m b/ui/cocoa.m > index 2d943b6e2a..5a84e1aea7 100644 > --- a/ui/cocoa.m > +++ b/ui/cocoa.m > @@ -129,8 +129,9 @@ > NSTextField *pauseLabel; > NSArray * supportedImageFileTypes; > > -// Utility function to run specified code block with iothread lock held > +// Utility functions to run specified code block with iothread lock held > typedef void (^CodeBlock)(void); > +typedef bool (^BoolCodeBlock)(void); > > static void with_iothread_lock(CodeBlock block) > { > @@ -144,6 +145,21 @@ static void with_iothread_lock(CodeBlock block) > } > } > > +static bool bool_with_iothread_lock(BoolCodeBlock block) > +{ > + bool locked = qemu_mutex_iothread_locked(); > + bool val; > + Git complained about extra white space in the end of the empty line above but not sure if it was added during mailing or you have it in the original patch. > + if (!locked) { > + qemu_mutex_lock_iothread(); > + } > + val = block(); > + if (!locked) { > + qemu_mutex_unlock_iothread(); > + } > + return val; > +} > + > // Mac to QKeyCode conversion > const int mac_to_qkeycode_map[] = { > [kVK_ANSI_A] = Q_KEY_CODE_A, > @@ -320,8 +336,8 @@ - (void) grabMouse; > - (void) ungrabMouse; > - (void) toggleFullScreen:(id)sender; > - (void) handleMonitorInput:(NSEvent *)event; > -- (void) handleEvent:(NSEvent *)event; > -- (void) handleEventLocked:(NSEvent *)event; > +- (bool) handleEvent:(NSEvent *)event; > +- (bool) handleEventLocked:(NSEvent *)event; > - (void) setAbsoluteEnabled:(BOOL)tIsAbsoluteEnabled; > /* The state surrounding mouse grabbing is potentially confusing. > * isAbsoluteEnabled tracks qemu_input_is_absolute() [ie "is the emulated > @@ -664,15 +680,16 @@ - (void) handleMonitorInput:(NSEvent *)event > } > } > > -- (void) handleEvent:(NSEvent *)event > +- (bool) handleEvent:(NSEvent *)event > { > - with_iothread_lock(^{ > - [self handleEventLocked:event]; > + return bool_with_iothread_lock(^{ > + return [self handleEventLocked:event]; > }); > } If this is only ever used for this one method, wouldn't it be easier to move locking to the method below (even with some goto after setting a ret variable to unlock at the end of the method where now it returns in the middle, but maybe it could even be done without goto as the whole code is one big switch that can be exited with break and an if that can be skipped by a flag)? That may be easier to follow than this method within block within method and then you wouldn't need bool_with_iothread_lock and neither handleEvent. Unless there's something I'm missing which makes this convoluted way needed. Other than that Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> Regards, BALATON Zoltan > -- (void) handleEventLocked:(NSEvent *)event > +- (bool) handleEventLocked:(NSEvent *)event > { > + /* Return true if we handled the event, false if it should be given to OSX */ > COCOA_DEBUG("QemuCocoaView: handleEvent\n"); > int buttons = 0; > int keycode = 0; > @@ -743,8 +760,7 @@ - (void) handleEventLocked:(NSEvent *)event > if (keycode == Q_KEY_CODE_F) { > switched_to_fullscreen = true; > } > - [NSApp sendEvent:event]; > - return; > + return false; > } > > // default > @@ -759,12 +775,12 @@ - (void) handleEventLocked:(NSEvent *)event > // enable graphic console > case '1' ... '9': > console_select(key - '0' - 1); /* ascii math */ > - return; > + return true; > > // release the mouse grab > case 'g': > [self ungrabMouse]; > - return; > + return true; > } > } > } > @@ -781,7 +797,7 @@ - (void) handleEventLocked:(NSEvent *)event > // don't pass the guest a spurious key-up if we treated this > // command-key combo as a host UI action > if (!isMouseGrabbed && ([event modifierFlags] & NSEventModifierFlagCommand)) { > - return; > + return true; > } > > if (qemu_console_is_graphic(NULL)) { > @@ -875,7 +891,7 @@ - (void) handleEventLocked:(NSEvent *)event > mouse_event = false; > break; > default: > - [NSApp sendEvent:event]; > + return false; > } > > if (mouse_event) { > @@ -911,10 +927,11 @@ - (void) handleEventLocked:(NSEvent *)event > qemu_input_queue_rel(dcl->con, INPUT_AXIS_Y, (int)[event deltaY]); > } > } else { > - [NSApp sendEvent:event]; > + return false; > } > qemu_input_event_sync(); > } > + return true; > } > > - (void) grabMouse > @@ -1749,7 +1766,9 @@ static void cocoa_refresh(DisplayChangeListener *dcl) > event = [NSApp nextEventMatchingMask:NSEventMaskAny untilDate:distantPast > inMode: NSDefaultRunLoopMode dequeue:YES]; > if (event != nil) { > - [cocoaView handleEvent:event]; > + if (![cocoaView handleEvent:event]) { > + [NSApp sendEvent:event]; > + } > } > } while(event != nil); > [pool release]; >
On Thu, 14 Feb 2019 at 17:04, BALATON Zoltan <balaton@eik.bme.hu> wrote: > > On Thu, 14 Feb 2019, Peter Maydell wrote: > > Currently the handleEvent method will directly call the NSApp > > sendEvent method for any events that we want to let OSX deal > > with. When we rearrange the event handling code, the way that > > we say "let OSX have this event" is going to change. Prepare > > for that by refactoring so that handleEvent returns a flag > > indicating whether it consumed the event. > > > > Suggested-by: BALATON Zoltan <balaton@eik.bme.hu> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > +static bool bool_with_iothread_lock(BoolCodeBlock block) > > +{ > > + bool locked = qemu_mutex_iothread_locked(); > > + bool val; > > + > > Git complained about extra white space in the end of the empty line above > but not sure if it was added during mailing or you have it in the original > patch. Almost certainly an error in the original patch. I'll fix it. > > + if (!locked) { > > + qemu_mutex_lock_iothread(); > > + } > > + val = block(); > > + if (!locked) { > > + qemu_mutex_unlock_iothread(); > > + } > > + return val; > > +} > > + > > // Mac to QKeyCode conversion > > const int mac_to_qkeycode_map[] = { > > [kVK_ANSI_A] = Q_KEY_CODE_A, > > @@ -320,8 +336,8 @@ - (void) grabMouse; > > - (void) ungrabMouse; > > - (void) toggleFullScreen:(id)sender; > > - (void) handleMonitorInput:(NSEvent *)event; > > -- (void) handleEvent:(NSEvent *)event; > > -- (void) handleEventLocked:(NSEvent *)event; > > +- (bool) handleEvent:(NSEvent *)event; > > +- (bool) handleEventLocked:(NSEvent *)event; > > - (void) setAbsoluteEnabled:(BOOL)tIsAbsoluteEnabled; > > /* The state surrounding mouse grabbing is potentially confusing. > > * isAbsoluteEnabled tracks qemu_input_is_absolute() [ie "is the emulated > > @@ -664,15 +680,16 @@ - (void) handleMonitorInput:(NSEvent *)event > > } > > } > > > > -- (void) handleEvent:(NSEvent *)event > > +- (bool) handleEvent:(NSEvent *)event > > { > > - with_iothread_lock(^{ > > - [self handleEventLocked:event]; > > + return bool_with_iothread_lock(^{ > > + return [self handleEventLocked:event]; > > }); > > } > > If this is only ever used for this one method, wouldn't it be easier to > move locking to the method below (even with some goto after setting a ret > variable to unlock at the end of the method where now it returns in the > middle, but maybe it could even be done without goto as the whole code is > one big switch that can be exited with break and an if that can be skipped > by a flag)? That may be easier to follow than this method within block > within method and then you wouldn't need bool_with_iothread_lock and > neither handleEvent. Unless there's something I'm missing which makes this > convoluted way needed. The aim was to avoid having to do changes to handleEvent's code flow in order to do "run it with the lock held"; it also means that the invariant "we always unlock the lock" is easy to confirm, whereas if you do the lock/unlock inside a single handleEvent method you have to look for whether it was done right in early-exit cases. It's a bit less of a convincing argument than it was in v1 (where we were making no changes to handleEvent at all other than wrapping it in a lock), but I think it still makes sense this way. > Other than that > Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> thanks -- PMM
On Thu, 14 Feb 2019, Peter Maydell wrote: > On Thu, 14 Feb 2019 at 17:04, BALATON Zoltan <balaton@eik.bme.hu> wrote: >>> -- (void) handleEvent:(NSEvent *)event >>> +- (bool) handleEvent:(NSEvent *)event >>> { >>> - with_iothread_lock(^{ >>> - [self handleEventLocked:event]; >>> + return bool_with_iothread_lock(^{ >>> + return [self handleEventLocked:event]; >>> }); >>> } >> >> If this is only ever used for this one method, wouldn't it be easier to >> move locking to the method below (even with some goto after setting a ret >> variable to unlock at the end of the method where now it returns in the >> middle, but maybe it could even be done without goto as the whole code is >> one big switch that can be exited with break and an if that can be skipped >> by a flag)? That may be easier to follow than this method within block >> within method and then you wouldn't need bool_with_iothread_lock and >> neither handleEvent. Unless there's something I'm missing which makes this >> convoluted way needed. > > The aim was to avoid having to do changes to handleEvent's code flow > in order to do "run it with the lock held"; it also means that the > invariant "we always unlock the lock" is easy to confirm, whereas > if you do the lock/unlock inside a single handleEvent method you have > to look for whether it was done right in early-exit cases. It's > a bit less of a convincing argument than it was in v1 (where we were > making no changes to handleEvent at all other than wrapping it in a > lock), but I think it still makes sense this way. I thought of something like below which to me is simpler than your proposal and makes less changes to the original but feel free to choose whichever you like, I don't mind either way. But the comment about sendEvent near the end is now outdated after either patch so you may want to update that even in your patch. Regards, BALATON Zoltan diff --git a/ui/cocoa.m b/ui/cocoa.m index 4727f9c392..de6606e017 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -132,9 +132,8 @@ QemuSemaphore display_init_sem; QemuSemaphore app_started_sem; -// Utility functions to run specified code block with iothread lock held +// Utility function to run specified code block with iothread lock held typedef void (^CodeBlock)(void); -typedef bool (^BoolCodeBlock)(void); static void with_iothread_lock(CodeBlock block) { @@ -148,21 +147,6 @@ static void with_iothread_lock(CodeBlock block) } } -static bool bool_with_iothread_lock(BoolCodeBlock block) -{ - bool locked = qemu_mutex_iothread_locked(); - bool val; - - if (!locked) { - qemu_mutex_lock_iothread(); - } - val = block(); - if (!locked) { - qemu_mutex_unlock_iothread(); - } - return val; -} - // Mac to QKeyCode conversion const int mac_to_qkeycode_map[] = { [kVK_ANSI_A] = Q_KEY_CODE_A, @@ -341,7 +325,6 @@ - (void) ungrabMouse; - (void) toggleFullScreen:(id)sender; - (void) handleMonitorInput:(NSEvent *)event; - (bool) handleEvent:(NSEvent *)event; -- (bool) handleEventLocked:(NSEvent *)event; - (void) setAbsoluteEnabled:(BOOL)tIsAbsoluteEnabled; /* The state surrounding mouse grabbing is potentially confusing. * isAbsoluteEnabled tracks qemu_input_is_absolute() [ie "is the emulated @@ -689,15 +672,15 @@ - (void) handleMonitorInput:(NSEvent *)event - (bool) handleEvent:(NSEvent *)event { - return bool_with_iothread_lock(^{ - return [self handleEventLocked:event]; - }); -} - -- (bool) handleEventLocked:(NSEvent *)event -{ - /* Return true if we handled the event, false if it should be given to OSX */ COCOA_DEBUG("QemuCocoaView: handleEvent\n"); + /* Return true if we handled the event, false if it should be given to OSX */ + bool handled = true; + bool locked = qemu_mutex_iothread_locked(); + if (!locked) { + qemu_mutex_lock_iothread(); + } + /* Make sure to release lock so don't return before the end */ + int buttons = 0; int keycode = 0; bool mouse_event = false; @@ -767,7 +750,8 @@ - (bool) handleEventLocked:(NSEvent *)event if (keycode == Q_KEY_CODE_F) { switched_to_fullscreen = true; } - return false; + handled = false; + goto unlock; } // default @@ -782,12 +766,12 @@ - (bool) handleEventLocked:(NSEvent *)event // enable graphic console case '1' ... '9': console_select(key - '0' - 1); /* ascii math */ - return true; + goto unlock; // release the mouse grab case 'g': [self ungrabMouse]; - return true; + goto unlock; } } } @@ -804,7 +788,7 @@ - (bool) handleEventLocked:(NSEvent *)event // don't pass the guest a spurious key-up if we treated this // command-key combo as a host UI action if (!isMouseGrabbed && ([event modifierFlags] & NSEventModifierFlagCommand)) { - return true; + goto unlock; } if (qemu_console_is_graphic(NULL)) { @@ -898,16 +882,16 @@ - (bool) handleEventLocked:(NSEvent *)event mouse_event = false; break; default: - return false; + handled = false; + goto unlock; } if (mouse_event) { /* Don't send button events to the guest unless we've got a * mouse grab or window focus. If we have neither then this event * is the user clicking on the background window to activate and - * bring us to the front, which will be done by the sendEvent - * call below. We definitely don't want to pass that click through - * to the guest. + * bring us to the front, which will be done by sendEvent. + * We definitely don't want to pass that click through to the guest. */ if ((isMouseGrabbed || [[self window] isKeyWindow]) && (last_buttons != buttons)) { @@ -934,11 +918,16 @@ - (bool) handleEventLocked:(NSEvent *)event qemu_input_queue_rel(dcl->con, INPUT_AXIS_Y, (int)[event deltaY]); } } else { - return false; + handled = false; + goto unlock; } qemu_input_event_sync(); } - return true; +unlock: + if (!locked) { + qemu_mutex_unlock_iothread(); + } + return handled; } - (void) grabMouse
On Thu, Feb 14, 2019 at 10:28:14AM +0000, Peter Maydell wrote: > Currently the handleEvent method will directly call the NSApp > sendEvent method for any events that we want to let OSX deal > with. When we rearrange the event handling code, the way that > we say "let OSX have this event" is going to change. Prepare > for that by refactoring so that handleEvent returns a flag > indicating whether it consumed the event. > > Suggested-by: BALATON Zoltan <balaton@eik.bme.hu> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > New patch in v2 > --- > ui/cocoa.m | 49 ++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 34 insertions(+), 15 deletions(-) > > @@ -1749,7 +1766,9 @@ static void cocoa_refresh(DisplayChangeListener *dcl) > event = [NSApp nextEventMatchingMask:NSEventMaskAny untilDate:distantPast > inMode: NSDefaultRunLoopMode dequeue:YES]; > if (event != nil) { > - [cocoaView handleEvent:event]; > + if (![cocoaView handleEvent:event]) { > + [NSApp sendEvent:event]; > + } > } > } while(event != nil); > [pool release]; > -- > 2.17.2 (Apple Git-113) > I like the patch. It makes clear that cocoa_refresh performs the work of [NSApp run]. Besides the trailing whitespace issue, Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com> Tested-by: Roman Bolshakov <r.bolshakov@yadro.com> Thanks, Roman
diff --git a/ui/cocoa.m b/ui/cocoa.m index 2d943b6e2a..5a84e1aea7 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -129,8 +129,9 @@ NSTextField *pauseLabel; NSArray * supportedImageFileTypes; -// Utility function to run specified code block with iothread lock held +// Utility functions to run specified code block with iothread lock held typedef void (^CodeBlock)(void); +typedef bool (^BoolCodeBlock)(void); static void with_iothread_lock(CodeBlock block) { @@ -144,6 +145,21 @@ static void with_iothread_lock(CodeBlock block) } } +static bool bool_with_iothread_lock(BoolCodeBlock block) +{ + bool locked = qemu_mutex_iothread_locked(); + bool val; + + if (!locked) { + qemu_mutex_lock_iothread(); + } + val = block(); + if (!locked) { + qemu_mutex_unlock_iothread(); + } + return val; +} + // Mac to QKeyCode conversion const int mac_to_qkeycode_map[] = { [kVK_ANSI_A] = Q_KEY_CODE_A, @@ -320,8 +336,8 @@ - (void) grabMouse; - (void) ungrabMouse; - (void) toggleFullScreen:(id)sender; - (void) handleMonitorInput:(NSEvent *)event; -- (void) handleEvent:(NSEvent *)event; -- (void) handleEventLocked:(NSEvent *)event; +- (bool) handleEvent:(NSEvent *)event; +- (bool) handleEventLocked:(NSEvent *)event; - (void) setAbsoluteEnabled:(BOOL)tIsAbsoluteEnabled; /* The state surrounding mouse grabbing is potentially confusing. * isAbsoluteEnabled tracks qemu_input_is_absolute() [ie "is the emulated @@ -664,15 +680,16 @@ - (void) handleMonitorInput:(NSEvent *)event } } -- (void) handleEvent:(NSEvent *)event +- (bool) handleEvent:(NSEvent *)event { - with_iothread_lock(^{ - [self handleEventLocked:event]; + return bool_with_iothread_lock(^{ + return [self handleEventLocked:event]; }); } -- (void) handleEventLocked:(NSEvent *)event +- (bool) handleEventLocked:(NSEvent *)event { + /* Return true if we handled the event, false if it should be given to OSX */ COCOA_DEBUG("QemuCocoaView: handleEvent\n"); int buttons = 0; int keycode = 0; @@ -743,8 +760,7 @@ - (void) handleEventLocked:(NSEvent *)event if (keycode == Q_KEY_CODE_F) { switched_to_fullscreen = true; } - [NSApp sendEvent:event]; - return; + return false; } // default @@ -759,12 +775,12 @@ - (void) handleEventLocked:(NSEvent *)event // enable graphic console case '1' ... '9': console_select(key - '0' - 1); /* ascii math */ - return; + return true; // release the mouse grab case 'g': [self ungrabMouse]; - return; + return true; } } } @@ -781,7 +797,7 @@ - (void) handleEventLocked:(NSEvent *)event // don't pass the guest a spurious key-up if we treated this // command-key combo as a host UI action if (!isMouseGrabbed && ([event modifierFlags] & NSEventModifierFlagCommand)) { - return; + return true; } if (qemu_console_is_graphic(NULL)) { @@ -875,7 +891,7 @@ - (void) handleEventLocked:(NSEvent *)event mouse_event = false; break; default: - [NSApp sendEvent:event]; + return false; } if (mouse_event) { @@ -911,10 +927,11 @@ - (void) handleEventLocked:(NSEvent *)event qemu_input_queue_rel(dcl->con, INPUT_AXIS_Y, (int)[event deltaY]); } } else { - [NSApp sendEvent:event]; + return false; } qemu_input_event_sync(); } + return true; } - (void) grabMouse @@ -1749,7 +1766,9 @@ static void cocoa_refresh(DisplayChangeListener *dcl) event = [NSApp nextEventMatchingMask:NSEventMaskAny untilDate:distantPast inMode: NSDefaultRunLoopMode dequeue:YES]; if (event != nil) { - [cocoaView handleEvent:event]; + if (![cocoaView handleEvent:event]) { + [NSApp sendEvent:event]; + } } } while(event != nil); [pool release];
Currently the handleEvent method will directly call the NSApp sendEvent method for any events that we want to let OSX deal with. When we rearrange the event handling code, the way that we say "let OSX have this event" is going to change. Prepare for that by refactoring so that handleEvent returns a flag indicating whether it consumed the event. Suggested-by: BALATON Zoltan <balaton@eik.bme.hu> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- New patch in v2 --- ui/cocoa.m | 49 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 15 deletions(-) -- 2.17.2 (Apple Git-113)