Re: [Qemu-devel] [PATCH v2 7/7] ui/cocoa: Perform UI operations only on the main thread

[Roman Bolshakov]

On Thu, Feb 14, 2019 at 10:28:16AM +0000, Peter Maydell wrote:
> The OSX Mojave release is more picky about enforcing the Cocoa API
> restriction that only the main thread may perform UI calls. To
> accommodate this we need to restructure the Cocoa code:
>  * the special OSX main() creates a second thread and uses
>    that to call the vl.c qemu_main(); the original main
>    thread goes into the OSX event loop
>  * the refresh, switch and update callbacks asynchronously
>    tell the main thread to do the necessary work
>  * the refresh callback no longer does the "get events from the
>    UI event queue and handle them" loop, since we now use
>    the stock OSX event loop. Instead our NSApplication sendEvent
>    method will either deal with them or pass them on to OSX
> 
> All these things have to be changed in one commit, to avoid
> breaking bisection.
> 
> Note that since we use dispatch_get_main_queue(), this bumps
> our minimum version requirement to OSX 10.10 Yosemite (released
> in 2014, unsupported by Apple since 2017).
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> v2 changes: call handleEvent from sendEvent
> ---
>  ui/cocoa.m | 191 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 113 insertions(+), 78 deletions(-)
> 
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 184fbd877d..201a294ed4 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -129,6 +129,9 @@
>  NSTextField *pauseLabel;
>  NSArray * supportedImageFileTypes;
>  
> +QemuSemaphore display_init_sem;
> +QemuSemaphore app_started_sem;
> +

They might have file scope.

> @@ -535,13 +539,16 @@ - (void) switchSurface:(pixman_image_t *)image
>      }
>  
>      // update screenBuffer
> -    if (dataProviderRef)
> +    if (dataProviderRef) {
>          CGDataProviderRelease(dataProviderRef);
> +        pixman_image_unref(pixman_image);
> +    }

pixman_image also needs to be unreferenced in dealloc.

> @@ -1485,7 +1482,9 @@ @implementation QemuApplication
>  - (void)sendEvent:(NSEvent *)event
>  {
>      COCOA_DEBUG("QemuApplication: sendEvent\n");
> -    [super sendEvent: event];
> +    if (!cocoaView || ![cocoaView handleEvent:event]) {
> +        [super sendEvent: event];
> +    }
>  }
>  @end
>  

if (!cocoaView || ![cocoaView handleEvent:event]) {

can be written as

if (![cocoaView handleEvent:event]) {

It's valid to send a message to nil and it will return 0/false/NO.

Thank you for working on the patch series. It definitely improves UI event
handling.

Besides the pixman_image leak,

Reviewed-by: Roman Bolshakov <address@hidden>
Tested-by: Roman Bolshakov <address@hidden>

Roman