From c35fdc8d6140a32ad67eb06e833911860cd71abc Mon Sep 17 00:00:00 2001 From: Aleksi Juvani <3168386+aleksijuvani@users.noreply.github.com> Date: Tue, 18 Jun 2019 09:34:27 +0300 Subject: [PATCH] Fix thread-safety of `set_maximized` and `set_title` on macOS (#922) --- src/platform_impl/macos/util/async.rs | 120 +++++++++++++++++++++++++- src/platform_impl/macos/window.rs | 64 ++++---------- 2 files changed, 135 insertions(+), 49 deletions(-) diff --git a/src/platform_impl/macos/util/async.rs b/src/platform_impl/macos/util/async.rs index 9c972f81e0..e553aa378d 100644 --- a/src/platform_impl/macos/util/async.rs +++ b/src/platform_impl/macos/util/async.rs @@ -1,14 +1,17 @@ -use std::{os::raw::c_void, sync::{Mutex, Weak}}; +use std::{ + os::raw::c_void, + sync::{Mutex, Weak}, +}; use cocoa::{ - appkit::{CGFloat, NSWindow, NSWindowStyleMask}, + appkit::{CGFloat, NSScreen, NSWindow, NSWindowStyleMask}, base::{id, nil}, - foundation::{NSAutoreleasePool, NSPoint, NSSize}, + foundation::{NSAutoreleasePool, NSPoint, NSSize, NSString}, }; use dispatch::ffi::{dispatch_async_f, dispatch_get_main_queue, dispatch_sync_f}; use crate::dpi::LogicalSize; -use crate::platform_impl::platform::{ffi, window::SharedState}; +use crate::platform_impl::platform::{ffi, util::IdRef, window::SharedState}; unsafe fn set_style_mask(ns_window: id, ns_view: id, mask: NSWindowStyleMask) { ns_window.setStyleMask_(mask); @@ -237,6 +240,83 @@ pub unsafe fn toggle_full_screen_async( ); } +struct SetMaximizedData { + ns_window: id, + is_zoomed: bool, + maximized: bool, + shared_state: Weak>, +} +impl SetMaximizedData { + fn new_ptr( + ns_window: id, + is_zoomed: bool, + maximized: bool, + shared_state: Weak>, + ) -> *mut Self { + Box::into_raw(Box::new(SetMaximizedData { + ns_window, + is_zoomed, + maximized, + shared_state, + })) + } +} +extern "C" fn set_maximized_callback(context: *mut c_void) { + unsafe { + let context_ptr = context as *mut SetMaximizedData; + { + let context = &*context_ptr; + + if let Some(shared_state) = context.shared_state.upgrade() { + trace!("Locked shared state in `set_maximized`"); + let mut shared_state_lock = shared_state.lock().unwrap(); + + // Save the standard frame sized if it is not zoomed + if !context.is_zoomed { + shared_state_lock.standard_frame = Some(NSWindow::frame(context.ns_window)); + } + + shared_state_lock.maximized = context.maximized; + + let curr_mask = context.ns_window.styleMask(); + if shared_state_lock.fullscreen.is_some() { + // Handle it in window_did_exit_fullscreen + return; + } else if curr_mask.contains(NSWindowStyleMask::NSResizableWindowMask) { + // Just use the native zoom if resizable + context.ns_window.zoom_(nil); + } else { + // if it's not resizable, we set the frame directly + let new_rect = if context.maximized { + let screen = NSScreen::mainScreen(nil); + NSScreen::visibleFrame(screen) + } else { + shared_state_lock.saved_standard_frame() + }; + context.ns_window.setFrame_display_(new_rect, 0); + } + + trace!("Unlocked shared state in `set_maximized`"); + } + } + Box::from_raw(context_ptr); + } +} +// `setMaximized` is not thread-safe +pub unsafe fn set_maximized_async( + ns_window: id, + is_zoomed: bool, + maximized: bool, + shared_state: Weak>, +) { + let context = SetMaximizedData::new_ptr(ns_window, is_zoomed, maximized, shared_state); + dispatch_async_f( + dispatch_get_main_queue(), + context as *mut _, + set_maximized_callback, + ); +} + struct OrderOutData { ns_window: id, } @@ -295,6 +375,38 @@ pub unsafe fn make_key_and_order_front_async(ns_window: id) { ); } +struct SetTitleData { + ns_window: id, + title: String, +} +impl SetTitleData { + fn new_ptr(ns_window: id, title: String) -> *mut Self { + Box::into_raw(Box::new(SetTitleData { ns_window, title })) + } +} +extern "C" fn set_title_callback(context: *mut c_void) { + unsafe { + let context_ptr = context as *mut SetTitleData; + { + let context = &*context_ptr; + let title = IdRef::new(NSString::alloc(nil).init_str(&context.title)); + context.ns_window.setTitle_(*title); + } + Box::from_raw(context_ptr); + } +} +// `setTitle:` isn't thread-safe. Calling it from another thread invalidates the +// window drag regions, which throws an exception when not done in the main +// thread +pub unsafe fn set_title_async(ns_window: id, title: String) { + let context = SetTitleData::new_ptr(ns_window, title); + dispatch_async_f( + dispatch_get_main_queue(), + context as *mut _, + set_title_callback, + ); +} + struct CloseData { ns_window: id, } diff --git a/src/platform_impl/macos/window.rs b/src/platform_impl/macos/window.rs index a7a9b4c14d..708a656366 100644 --- a/src/platform_impl/macos/window.rs +++ b/src/platform_impl/macos/window.rs @@ -217,12 +217,19 @@ pub struct SharedState { pub resizable: bool, pub fullscreen: Option, pub maximized: bool, - standard_frame: Option, + pub standard_frame: Option, is_simple_fullscreen: bool, pub saved_style: Option, save_presentation_opts: Option, } +impl SharedState { + pub fn saved_standard_frame(&self) -> NSRect { + self.standard_frame + .unwrap_or_else(|| NSRect::new(NSPoint::new(50.0, 50.0), NSSize::new(800.0, 600.0))) + } +} + impl From for SharedState { fn from(attribs: WindowAttributes) -> Self { SharedState { @@ -378,8 +385,7 @@ impl UnownedWindow { pub fn set_title(&self, title: &str) { unsafe { - let title = IdRef::new(NSString::alloc(nil).init_str(title)); - self.ns_window.setTitle_(*title); + util::set_title_async(*self.ns_window, title.to_string()); } } @@ -570,13 +576,6 @@ impl UnownedWindow { } } - fn saved_standard_frame(shared_state: &mut SharedState) -> NSRect { - shared_state.standard_frame.unwrap_or_else(|| NSRect::new( - NSPoint::new(50.0, 50.0), - NSSize::new(800.0, 600.0), - )) - } - pub(crate) fn restore_state_from_fullscreen(&self) { let maximized = { trace!("Locked shared state in `restore_state_from_fullscreen`"); @@ -596,42 +595,17 @@ impl UnownedWindow { #[inline] pub fn set_maximized(&self, maximized: bool) { let is_zoomed = self.is_zoomed(); - if is_zoomed == maximized { return }; - - trace!("Locked shared state in `set_maximized`"); - let mut shared_state_lock = self.shared_state.lock().unwrap(); - - // Save the standard frame sized if it is not zoomed - if !is_zoomed { - unsafe { - shared_state_lock.standard_frame = Some(NSWindow::frame(*self.ns_window)); - } - } - - shared_state_lock.maximized = maximized; - - let curr_mask = unsafe { self.ns_window.styleMask() }; - if shared_state_lock.fullscreen.is_some() { - // Handle it in window_did_exit_fullscreen + if is_zoomed == maximized { return; - } else if curr_mask.contains(NSWindowStyleMask::NSResizableWindowMask) { - // Just use the native zoom if resizable - unsafe { self.ns_window.zoom_(nil) }; - } else { - // if it's not resizable, we set the frame directly - unsafe { - let new_rect = if maximized { - let screen = NSScreen::mainScreen(nil); - NSScreen::visibleFrame(screen) - } else { - Self::saved_standard_frame(&mut *shared_state_lock) - }; - // This probably isn't thread-safe! - self.ns_window.setFrame_display_(new_rect, 0); - } + }; + unsafe { + util::set_maximized_async( + *self.ns_window, + is_zoomed, + maximized, + Arc::downgrade(&self.shared_state), + ); } - - trace!("Unlocked shared state in `set_maximized`"); } #[inline] @@ -847,7 +821,7 @@ impl WindowExtMacOS for UnownedWindow { app.setPresentationOptions_(presentation_opts); } - let frame = Self::saved_standard_frame(&mut *shared_state_lock); + let frame = shared_state_lock.saved_standard_frame(); NSWindow::setFrame_display_(*self.ns_window, frame, YES); NSWindow::setMovable_(*self.ns_window, YES);