From bf92f3e97b997293e3c621021ed4a4a99233ab91 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Wed, 30 Nov 2022 14:30:32 +0100 Subject: [PATCH] macOS: Run tasks synchronously on main thread instead of asynchronously (#2574) * Close windows synchronously on main thread * Set style mask synchronously on main thread * Set title synchronously on main thread * Set visibility and focus synchronously on main thread * Set window level synchronously on main thread * Set position and size synchronously on main thread * Set cursor hittest synchronously on main thread * Add changelog entry --- CHANGELOG.md | 1 + src/platform_impl/macos/mod.rs | 2 +- src/platform_impl/macos/util/async.rs | 79 ++++++++++++--------------- src/platform_impl/macos/window.rs | 35 ++++++------ 4 files changed, 54 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fcaf0fec..d6200eaf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ And please only add new entries to the top of this list, right below the `# Unre # Unreleased +- On macOS, run most actions on the main thread, which is strictly more correct, but might make multithreaded applications block slightly more. - On macOS, fix panic when getting current monitor without any monitor attached. - On Windows and MacOS, add API to enable/disable window buttons (close, minimize, ...etc). - On Windows, macOS, X11 and Wayland, add `Window::set_theme`. diff --git a/src/platform_impl/macos/mod.rs b/src/platform_impl/macos/mod.rs index f0df52e4..378a8075 100644 --- a/src/platform_impl/macos/mod.rs +++ b/src/platform_impl/macos/mod.rs @@ -57,7 +57,7 @@ pub(crate) struct Window { impl Drop for Window { fn drop(&mut self) { // Ensure the window is closed - util::close_async(Id::into_super(self.window.clone())); + util::close_sync(&self.window); } } diff --git a/src/platform_impl/macos/util/async.rs b/src/platform_impl/macos/util/async.rs index 9f62b7f8..f85d39bd 100644 --- a/src/platform_impl/macos/util/async.rs +++ b/src/platform_impl/macos/util/async.rs @@ -1,4 +1,3 @@ -use std::mem; use std::ops::Deref; use dispatch::Queue; @@ -29,6 +28,14 @@ impl Deref for MainThreadSafe { } } +fn run_on_main(f: impl FnOnce() -> R + Send) -> R { + if is_main_thread() { + f() + } else { + Queue::main().exec_sync(f) + } +} + fn set_style_mask(window: &NSWindow, mask: NSWindowStyleMask) { window.setStyleMask(mask); // If we don't do this, key handling will break @@ -40,54 +47,43 @@ fn set_style_mask(window: &NSWindow, mask: NSWindowStyleMask) { // `setStyleMask:` isn't thread-safe, so we have to use Grand Central Dispatch. // Otherwise, this would vomit out errors about not being on the main thread // and fail to do anything. -pub(crate) fn set_style_mask_async(window: &NSWindow, mask: NSWindowStyleMask) { - // TODO(madsmtm): Remove this 'static hack! - let window = unsafe { MainThreadSafe(mem::transmute::<&NSWindow, &'static NSWindow>(window)) }; - Queue::main().exec_async(move || { - set_style_mask(&window, mask); - }); -} pub(crate) fn set_style_mask_sync(window: &NSWindow, mask: NSWindowStyleMask) { - if is_main_thread() { - set_style_mask(window, mask); - } else { - let window = MainThreadSafe(window); - Queue::main().exec_sync(move || { - set_style_mask(&window, mask); - }) - } + let window = MainThreadSafe(window); + run_on_main(move || { + set_style_mask(&window, mask); + }) } // `setContentSize:` isn't thread-safe either, though it doesn't log any errors // and just fails silently. Anyway, GCD to the rescue! -pub(crate) fn set_content_size_async(window: &NSWindow, size: LogicalSize) { - let window = unsafe { MainThreadSafe(mem::transmute::<&NSWindow, &'static NSWindow>(window)) }; - Queue::main().exec_async(move || { +pub(crate) fn set_content_size_sync(window: &NSWindow, size: LogicalSize) { + let window = MainThreadSafe(window); + run_on_main(move || { window.setContentSize(NSSize::new(size.width as CGFloat, size.height as CGFloat)); }); } // `setFrameTopLeftPoint:` isn't thread-safe, but fortunately has the courtesy // to log errors. -pub(crate) fn set_frame_top_left_point_async(window: &NSWindow, point: NSPoint) { - let window = unsafe { MainThreadSafe(mem::transmute::<&NSWindow, &'static NSWindow>(window)) }; - Queue::main().exec_async(move || { +pub(crate) fn set_frame_top_left_point_sync(window: &NSWindow, point: NSPoint) { + let window = MainThreadSafe(window); + run_on_main(move || { window.setFrameTopLeftPoint(point); }); } // `setFrameTopLeftPoint:` isn't thread-safe, and fails silently. -pub(crate) fn set_level_async(window: &NSWindow, level: NSWindowLevel) { - let window = unsafe { MainThreadSafe(mem::transmute::<&NSWindow, &'static NSWindow>(window)) }; - Queue::main().exec_async(move || { +pub(crate) fn set_level_sync(window: &NSWindow, level: NSWindowLevel) { + let window = MainThreadSafe(window); + run_on_main(move || { window.setLevel(level); }); } // `setIgnoresMouseEvents_:` isn't thread-safe, and fails silently. -pub(crate) fn set_ignore_mouse_events(window: &NSWindow, ignore: bool) { - let window = unsafe { MainThreadSafe(mem::transmute::<&NSWindow, &'static NSWindow>(window)) }; - Queue::main().exec_async(move || { +pub(crate) fn set_ignore_mouse_events_sync(window: &NSWindow, ignore: bool) { + let window = MainThreadSafe(window); + run_on_main(move || { window.setIgnoresMouseEvents(ignore); }); } @@ -171,18 +167,18 @@ pub(crate) fn set_maximized_async( // `orderOut:` isn't thread-safe. Calling it from another thread actually works, // but with an odd delay. -pub(crate) fn order_out_async(window: &NSWindow) { - let window = unsafe { MainThreadSafe(mem::transmute::<&NSWindow, &'static NSWindow>(window)) }; - Queue::main().exec_async(move || { +pub(crate) fn order_out_sync(window: &NSWindow) { + let window = MainThreadSafe(window); + run_on_main(move || { window.orderOut(None); }); } // `makeKeyAndOrderFront:` isn't thread-safe. Calling it from another thread // actually works, but with an odd delay. -pub(crate) fn make_key_and_order_front_async(window: &NSWindow) { - let window = unsafe { MainThreadSafe(mem::transmute::<&NSWindow, &'static NSWindow>(window)) }; - Queue::main().exec_async(move || { +pub(crate) fn make_key_and_order_front_sync(window: &NSWindow) { + let window = MainThreadSafe(window); + run_on_main(move || { window.makeKeyAndOrderFront(None); }); } @@ -190,21 +186,18 @@ pub(crate) fn make_key_and_order_front_async(window: &NSWindow) { // `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(crate) fn set_title_async(window: &NSWindow, title: String) { - let window = unsafe { MainThreadSafe(mem::transmute::<&NSWindow, &'static NSWindow>(window)) }; - Queue::main().exec_async(move || { - window.setTitle(&NSString::from_str(&title)); +pub(crate) fn set_title_sync(window: &NSWindow, title: &str) { + let window = MainThreadSafe(window); + run_on_main(move || { + window.setTitle(&NSString::from_str(title)); }); } // `close:` is thread-safe, but we want the event to be triggered from the main // thread. Though, it's a good idea to look into that more... -// -// ArturKovacs: It's important that this operation keeps the underlying window alive -// through the `Id` because otherwise it would dereference free'd memory -pub(crate) fn close_async(window: Id) { +pub(crate) fn close_sync(window: &NSWindow) { let window = MainThreadSafe(window); - Queue::main().exec_async(move || { + run_on_main(move || { autoreleasepool(move |_| { window.close(); }); diff --git a/src/platform_impl/macos/window.rs b/src/platform_impl/macos/window.rs index a6e8a7c5..faf40ae0 100644 --- a/src/platform_impl/macos/window.rs +++ b/src/platform_impl/macos/window.rs @@ -459,10 +459,6 @@ impl WinitWindow { SharedStateMutexGuard::new(self.shared_state.lock().unwrap(), called_from_fn) } - fn set_style_mask_async(&self, mask: NSWindowStyleMask) { - util::set_style_mask_async(self, mask); - } - fn set_style_mask_sync(&self, mask: NSWindowStyleMask) { util::set_style_mask_sync(self, mask); } @@ -472,13 +468,13 @@ impl WinitWindow { } pub fn set_title(&self, title: &str) { - util::set_title_async(self, title.to_string()); + util::set_title_sync(self, title); } pub fn set_visible(&self, visible: bool) { match visible { - true => util::make_key_and_order_front_async(self), - false => util::order_out_async(self), + true => util::make_key_and_order_front_sync(self), + false => util::order_out_sync(self), } } @@ -514,7 +510,7 @@ impl WinitWindow { pub fn set_outer_position(&self, position: Position) { let scale_factor = self.scale_factor(); let position = position.to_logical(scale_factor); - util::set_frame_top_left_point_async(self, util::window_position(position)); + util::set_frame_top_left_point_sync(self, util::window_position(position)); } #[inline] @@ -536,7 +532,7 @@ impl WinitWindow { #[inline] pub fn set_inner_size(&self, size: Size) { let scale_factor = self.scale_factor(); - util::set_content_size_async(self, size.to_logical(scale_factor)); + util::set_content_size_sync(self, size.to_logical(scale_factor)); } pub fn set_min_inner_size(&self, dimensions: Option) { @@ -632,8 +628,9 @@ impl WinitWindow { } else { mask &= !NSWindowStyleMask::NSResizableWindowMask; } - self.set_style_mask_async(mask); - } // Otherwise, we don't change the mask until we exit fullscreen. + self.set_style_mask_sync(mask); + } + // Otherwise, we don't change the mask until we exit fullscreen. } #[inline] @@ -754,7 +751,7 @@ impl WinitWindow { #[inline] pub fn set_cursor_hittest(&self, hittest: bool) -> Result<(), ExternalError> { - util::set_ignore_mouse_events(self, !hittest); + util::set_ignore_mouse_events_sync(self, !hittest); Ok(()) } @@ -774,7 +771,7 @@ impl WinitWindow { // Roll back temp styles if needs_temp_mask { - self.set_style_mask_async(curr_mask); + self.set_style_mask_sync(curr_mask); } is_zoomed @@ -805,7 +802,7 @@ impl WinitWindow { drop(shared_state_lock); - self.set_style_mask_async(mask); + self.set_style_mask_sync(mask); self.set_maximized(maximized); } @@ -885,7 +882,7 @@ impl WinitWindow { // The coordinate system here has its origin at bottom-left // and Y goes up screen_frame.origin.y += screen_frame.size.height; - util::set_frame_top_left_point_async(self, screen_frame.origin); + util::set_frame_top_left_point_sync(self, screen_frame.origin); } } @@ -1054,7 +1051,7 @@ impl WinitWindow { } new_mask }; - self.set_style_mask_async(new_mask); + self.set_style_mask_sync(new_mask); } } @@ -1070,7 +1067,7 @@ impl WinitWindow { WindowLevel::AlwaysOnBottom => NSWindowLevel::BELOW_NORMAL, WindowLevel::Normal => NSWindowLevel::Normal, }; - util::set_level_async(self, level); + util::set_level_sync(self, level); } #[inline] @@ -1106,7 +1103,7 @@ impl WinitWindow { if !is_minimized && is_visible { NSApp().activateIgnoringOtherApps(true); - util::make_key_and_order_front_async(self); + util::make_key_and_order_front_sync(self); } } @@ -1255,7 +1252,7 @@ impl WindowExtMacOS for WinitWindow { true } else { let new_mask = self.saved_style(&mut shared_state_lock); - self.set_style_mask_async(new_mask); + self.set_style_mask_sync(new_mask); shared_state_lock.is_simple_fullscreen = false; if let Some(presentation_opts) = shared_state_lock.save_presentation_opts {