Re: [Qemu-devel] [PATCH v2 5/7] ui/cocoa: Don't call NSApp sendEvent directly from handleEvent

[Peter Maydell]

On Thu, 14 Feb 2019 at 17:04, BALATON Zoltan <address@hidden> 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 <address@hidden>
> > Signed-off-by: Peter Maydell <address@hidden>
> > ---

> > +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 <address@hidden>

thanks
-- PMM