From 2a58b785fed2a3746f7c7eebce95bce67ddfd27c Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Tue, 29 Nov 2022 12:58:35 +0100 Subject: [PATCH] Refactor `SharedState` so that it is no longer behind an `Arc` (#2573) * Refactor SharedState so that it is no longer behind an Arc * Always use `Window::lock_shared_state` --- src/platform_impl/macos/util/async.rs | 80 +++++++++------------- src/platform_impl/macos/view.rs | 7 +- src/platform_impl/macos/window.rs | 55 ++++++--------- src/platform_impl/macos/window_delegate.rs | 5 +- 4 files changed, 55 insertions(+), 92 deletions(-) diff --git a/src/platform_impl/macos/util/async.rs b/src/platform_impl/macos/util/async.rs index f7135206..9f62b7f8 100644 --- a/src/platform_impl/macos/util/async.rs +++ b/src/platform_impl/macos/util/async.rs @@ -1,6 +1,5 @@ use std::mem; use std::ops::Deref; -use std::sync::{Mutex, Weak}; use dispatch::Queue; use objc2::foundation::{is_main_thread, CGFloat, NSPoint, NSSize, NSString}; @@ -11,7 +10,7 @@ use crate::{ platform_impl::platform::{ appkit::{NSScreen, NSWindow, NSWindowLevel, NSWindowStyleMask}, ffi, - window::{SharedState, SharedStateMutexGuard}, + window::WinitWindow, }, }; @@ -95,13 +94,8 @@ pub(crate) fn set_ignore_mouse_events(window: &NSWindow, ignore: bool) { // `toggleFullScreen` is thread-safe, but our additional logic to account for // window styles isn't. -pub(crate) fn toggle_full_screen_async( - window: &NSWindow, - not_fullscreen: bool, - shared_state: Weak>, -) { - let window = unsafe { MainThreadSafe(mem::transmute::<&NSWindow, &'static NSWindow>(window)) }; - let shared_state = MainThreadSafe(shared_state); +pub(crate) fn toggle_full_screen_async(window: Id, not_fullscreen: bool) { + let window = MainThreadSafe(window); Queue::main().exec_async(move || { // `toggleFullScreen` doesn't work if the `StyleMask` is none, so we // set a normal style temporarily. The previous state will be @@ -112,13 +106,9 @@ pub(crate) fn toggle_full_screen_async( NSWindowStyleMask::NSTitledWindowMask | NSWindowStyleMask::NSResizableWindowMask; if !curr_mask.contains(required) { set_style_mask(&window, required); - if let Some(shared_state) = shared_state.upgrade() { - let mut shared_state_lock = SharedStateMutexGuard::new( - shared_state.lock().unwrap(), - "toggle_full_screen_callback", - ); - shared_state_lock.saved_style = Some(curr_mask); - } + window + .lock_shared_state("toggle_full_screen_async") + .saved_style = Some(curr_mask); } } // Window level must be restored from `CGShieldingWindowLevel() @@ -141,46 +131,40 @@ pub(crate) unsafe fn restore_display_mode_async(ns_screen: u32) { // `setMaximized` is not thread-safe pub(crate) fn set_maximized_async( - window: &NSWindow, + window: Id, is_zoomed: bool, maximized: bool, - shared_state: Weak>, ) { - let window = unsafe { MainThreadSafe(mem::transmute::<&NSWindow, &'static NSWindow>(window)) }; - let shared_state = MainThreadSafe(shared_state); + let window = MainThreadSafe(window); Queue::main().exec_async(move || { - if let Some(shared_state) = shared_state.upgrade() { - let mut shared_state_lock = - SharedStateMutexGuard::new(shared_state.lock().unwrap(), "set_maximized"); + let mut shared_state = window.lock_shared_state("set_maximized_async"); + // Save the standard frame sized if it is not zoomed + if !is_zoomed { + shared_state.standard_frame = Some(window.frame()); + } - // Save the standard frame sized if it is not zoomed - if !is_zoomed { - shared_state_lock.standard_frame = Some(window.frame()); - } + shared_state.maximized = maximized; - shared_state_lock.maximized = maximized; + if shared_state.fullscreen.is_some() { + // Handle it in window_did_exit_fullscreen + return; + } - if shared_state_lock.fullscreen.is_some() { - // Handle it in window_did_exit_fullscreen - return; - } - - if window - .styleMask() - .contains(NSWindowStyleMask::NSResizableWindowMask) - { - // Just use the native zoom if resizable - window.zoom(None); + if window + .styleMask() + .contains(NSWindowStyleMask::NSResizableWindowMask) + { + // Just use the native zoom if resizable + window.zoom(None); + } else { + // if it's not resizable, we set the frame directly + let new_rect = if maximized { + let screen = NSScreen::main().expect("no screen found"); + screen.visibleFrame() } else { - // if it's not resizable, we set the frame directly - let new_rect = if maximized { - let screen = NSScreen::main().expect("no screen found"); - screen.visibleFrame() - } else { - shared_state_lock.saved_standard_frame() - }; - window.setFrame_display(new_rect, false); - } + shared_state.saved_standard_frame() + }; + window.setFrame_display(new_rect, false); } }); } diff --git a/src/platform_impl/macos/view.rs b/src/platform_impl/macos/view.rs index dd1e17d5..b6e9c69a 100644 --- a/src/platform_impl/macos/view.rs +++ b/src/platform_impl/macos/view.rs @@ -148,7 +148,7 @@ declare_class!( #[sel(initWithId:acceptsFirstMouse:)] fn init_with_id( &mut self, - window: *mut WinitWindow, + window: &WinitWindow, accepts_first_mouse: bool, ) -> Option<&mut Self> { let this: Option<&mut Self> = unsafe { msg_send![super(self), init] }; @@ -164,10 +164,7 @@ declare_class!( forward_key_to_app: false, }; - Ivar::write( - &mut this._ns_window, - unsafe { Id::retain(window) }.expect("non-null window"), - ); + Ivar::write(&mut this._ns_window, window.retain()); Ivar::write(&mut this.state, Box::new(state)); Ivar::write(&mut this.marked_text, NSMutableAttributedString::new()); Ivar::write(&mut this.accepts_first_mouse, accepts_first_mouse); diff --git a/src/platform_impl/macos/window.rs b/src/platform_impl/macos/window.rs index 94b98473..a6e8a7c5 100644 --- a/src/platform_impl/macos/window.rs +++ b/src/platform_impl/macos/window.rs @@ -4,7 +4,7 @@ use std::{ os::raw::c_void, sync::{ atomic::{AtomicBool, Ordering}, - Arc, Mutex, MutexGuard, + Mutex, MutexGuard, }, }; @@ -105,7 +105,7 @@ declare_class!( pub(crate) struct WinitWindow { // TODO: Fix unnecessary boxing here // SAFETY: These are initialized in WinitWindow::new, right after it is created. - shared_state: IvarDrop>>>, + shared_state: IvarDrop>>, decorations: IvarDrop>, } @@ -169,7 +169,7 @@ pub(crate) struct SharedStateMutexGuard<'a> { impl<'a> SharedStateMutexGuard<'a> { #[inline] - pub(crate) fn new(guard: MutexGuard<'a, SharedState>, called_from_fn: &'static str) -> Self { + fn new(guard: MutexGuard<'a, SharedState>, called_from_fn: &'static str) -> Self { trace!("Locked shared state in `{}`", called_from_fn); Self { guard, @@ -301,10 +301,7 @@ impl WinitWindow { maximized: attrs.maximized, ..Default::default() }; - Ivar::write( - &mut this.shared_state, - Box::new(Arc::new(Mutex::new(state))), - ); + Ivar::write(&mut this.shared_state, Box::new(Mutex::new(state))); Ivar::write( &mut this.decorations, Box::new(AtomicBool::new(attrs.decorations)), @@ -414,11 +411,11 @@ impl WinitWindow { match attrs.preferred_theme { Some(theme) => { set_ns_theme(Some(theme)); - let mut state = this.shared_state.lock().unwrap(); + let mut state = this.lock_shared_state("WinitWindow::new"); state.current_theme = Some(theme); } None => { - let mut state = this.shared_state.lock().unwrap(); + let mut state = this.lock_shared_state("WinitWindow::new"); state.current_theme = Some(get_ns_theme()); } } @@ -443,6 +440,12 @@ impl WinitWindow { Ok((this, delegate)) } + pub(super) fn retain(&self) -> Id { + // SAFETY: The pointer is valid, and the window is always `Shared` + // TODO(madsmtm): Remove the need for unsafety here + unsafe { Id::retain(self as *const Self as *mut Self).unwrap() } + } + pub(super) fn view(&self) -> Id { // SAFETY: The view inside WinitWindow is always `WinitView` unsafe { Id::cast(self.contentView()) } @@ -826,12 +829,7 @@ impl WinitWindow { if is_zoomed == maximized { return; }; - util::set_maximized_async( - self, - is_zoomed, - maximized, - Arc::downgrade(&*self.shared_state), - ); + util::set_maximized_async(self.retain(), is_zoomed, maximized); } #[inline] @@ -966,30 +964,18 @@ impl WinitWindow { match (&old_fullscreen, &fullscreen) { (&None, &Some(_)) => { - util::toggle_full_screen_async( - self, - old_fullscreen.is_none(), - Arc::downgrade(&*self.shared_state), - ); + util::toggle_full_screen_async(self.retain(), old_fullscreen.is_none()); } (&Some(Fullscreen::Borderless(_)), &None) => { // State is restored by `window_did_exit_fullscreen` - util::toggle_full_screen_async( - self, - old_fullscreen.is_none(), - Arc::downgrade(&*self.shared_state), - ); + util::toggle_full_screen_async(self.retain(), old_fullscreen.is_none()); } (&Some(Fullscreen::Exclusive(ref video_mode)), &None) => { unsafe { util::restore_display_mode_async(video_mode.monitor().native_identifier()) }; // Rest of the state is restored by `window_did_exit_fullscreen` - util::toggle_full_screen_async( - self, - old_fullscreen.is_none(), - Arc::downgrade(&*self.shared_state), - ); + util::toggle_full_screen_async(self.retain(), old_fullscreen.is_none()); } (&Some(Fullscreen::Borderless(_)), &Some(Fullscreen::Exclusive(_))) => { // If we're already in fullscreen mode, calling @@ -1182,8 +1168,7 @@ impl WinitWindow { #[inline] pub fn theme(&self) -> Option { - let state = self.shared_state.lock().unwrap(); - state.current_theme + self.lock_shared_state("theme").current_theme } #[inline] @@ -1219,13 +1204,13 @@ impl WindowExtMacOS for WinitWindow { #[inline] fn simple_fullscreen(&self) -> bool { - let shared_state_lock = self.shared_state.lock().unwrap(); - shared_state_lock.is_simple_fullscreen + self.lock_shared_state("simple_fullscreen") + .is_simple_fullscreen } #[inline] fn set_simple_fullscreen(&self, fullscreen: bool) -> bool { - let mut shared_state_lock = self.shared_state.lock().unwrap(); + let mut shared_state_lock = self.lock_shared_state("set_simple_fullscreen"); let app = NSApp(); let is_native_fullscreen = shared_state_lock.fullscreen.is_some(); diff --git a/src/platform_impl/macos/window_delegate.rs b/src/platform_impl/macos/window_delegate.rs index 92f8ecd3..51f758bb 100644 --- a/src/platform_impl/macos/window_delegate.rs +++ b/src/platform_impl/macos/window_delegate.rs @@ -57,10 +57,7 @@ declare_class!( this.map(|this| { let scale_factor = window.scale_factor(); - Ivar::write(&mut this.window, unsafe { - let window: *const WinitWindow = window; - Id::retain(window as *mut WinitWindow).unwrap() - }); + Ivar::write(&mut this.window, window.retain()); Ivar::write(&mut this.initial_fullscreen, initial_fullscreen); Ivar::write(&mut this.previous_position, None); Ivar::write(&mut this.previous_scale_factor, scale_factor);