From 5c72c34f9dae4721905e718f2429f904cce89262 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 2 Sep 2022 22:07:46 +0200 Subject: [PATCH] Refactor window delegate to use much less unsafe --- src/platform_impl/macos/window.rs | 2 +- src/platform_impl/macos/window_delegate.rs | 448 +++++++++------------ 2 files changed, 187 insertions(+), 263 deletions(-) diff --git a/src/platform_impl/macos/window.rs b/src/platform_impl/macos/window.rs index 572e5e72be..6a4fcb188c 100644 --- a/src/platform_impl/macos/window.rs +++ b/src/platform_impl/macos/window.rs @@ -387,7 +387,7 @@ impl WinitWindow { unsafe { NSFilenamesPboardType }.copy() ])); - let delegate = WinitWindowDelegate::new(this.clone(), attrs.fullscreen.is_some()); + let delegate = WinitWindowDelegate::new(&this, attrs.fullscreen.is_some()); // Set fullscreen mode after we setup everything this.set_fullscreen(attrs.fullscreen); diff --git a/src/platform_impl/macos/window_delegate.rs b/src/platform_impl/macos/window_delegate.rs index 2c3a28df39..44378289dd 100644 --- a/src/platform_impl/macos/window_delegate.rs +++ b/src/platform_impl/macos/window_delegate.rs @@ -1,6 +1,6 @@ -use std::os::raw::c_void; use std::ptr; +use objc2::declare::{Ivar, IvarDrop}; use objc2::foundation::{NSArray, NSObject, NSString}; use objc2::rc::{autoreleasepool, Id, Shared}; use objc2::runtime::Object; @@ -21,98 +21,24 @@ use crate::{ window::{Fullscreen, WindowId}, }; -struct WindowDelegateState { - window: Id, - - // TODO: It's possible for delegate methods to be called asynchronously, - // causing data races / `RefCell` panics. - - // This is set when WindowBuilder::with_fullscreen was set, - // see comments of `window_did_fail_to_enter_fullscreen` - initial_fullscreen: bool, - - // During `windowDidResize`, we use this to only send Moved if the position changed. - previous_position: Option<(f64, f64)>, - - // Used to prevent redundant events. - previous_scale_factor: f64, -} - -impl WindowDelegateState { - fn new(window: Id, initial_fullscreen: bool) -> Self { - let scale_factor = window.scale_factor(); - let mut delegate_state = WindowDelegateState { - window, - initial_fullscreen, - previous_position: None, - previous_scale_factor: scale_factor, - }; - - if scale_factor != 1.0 { - delegate_state.emit_static_scale_factor_changed_event(); - } - - delegate_state - } - - // TODO: Remove this - fn with_window(&mut self, callback: F) -> Option - where - F: FnOnce(&WinitWindow) -> T, - { - Some(callback(&self.window.clone())) - } - - fn emit_event(&mut self, event: WindowEvent<'static>) { - let event = Event::WindowEvent { - window_id: WindowId(self.window.id()), - event, - }; - AppState::queue_event(EventWrapper::StaticEvent(event)); - } - - fn emit_static_scale_factor_changed_event(&mut self) { - let scale_factor = self.get_scale_factor(); - if scale_factor == self.previous_scale_factor { - return; - }; - - self.previous_scale_factor = scale_factor; - let wrapper = EventWrapper::EventProxy(EventProxy::DpiChangedProxy { - window: self.window.clone(), - suggested_size: self.view_size(), - scale_factor, - }); - AppState::queue_event(wrapper); - } +declare_class!( + #[derive(Debug)] + pub(crate) struct WinitWindowDelegate { + window: IvarDrop>, - fn emit_move_event(&mut self) { - let rect = self.window.frame(); - let x = rect.origin.x as f64; - let y = util::bottom_left_to_top_left(rect); - let moved = self.previous_position != Some((x, y)); - if moved { - self.previous_position = Some((x, y)); - let scale_factor = self.get_scale_factor(); - let physical_pos = LogicalPosition::::from((x, y)).to_physical(scale_factor); - self.emit_event(WindowEvent::Moved(physical_pos)); - } - } + // TODO: It's possible for delegate methods to be called asynchronously, + // causing data races / `RefCell` panics. - fn get_scale_factor(&self) -> f64 { - self.window.scale_factor() - } + // This is set when WindowBuilder::with_fullscreen was set, + // see comments of `window_did_fail_to_enter_fullscreen` + initial_fullscreen: bool, - fn view_size(&self) -> LogicalSize { - let size = self.window.contentView().frame().size; - LogicalSize::new(size.width as f64, size.height as f64) - } -} + // During `windowDidResize`, we use this to only send Moved if the position changed. + // TODO: Remove unnecessary boxing here + previous_position: IvarDrop>>, -declare_class!( - #[derive(Debug)] - pub(crate) struct WinitWindowDelegate { - state: *mut c_void, + // Used to prevent redundant events. + previous_scale_factor: f64, } unsafe impl ClassType for WinitWindowDelegate { @@ -120,21 +46,28 @@ declare_class!( } unsafe impl WinitWindowDelegate { - #[sel(dealloc)] - fn dealloc(&mut self) { - self.with_state(|state| unsafe { - drop(Box::from_raw(state as *mut WindowDelegateState)); - }); - } - #[sel(initWithWinit:)] - fn init_with_winit(&mut self, state: *mut c_void) -> Option<&mut Self> { + fn init_with_winit( + &mut self, + window: &WinitWindow, + initial_fullscreen: bool, + ) -> Option<&mut Self> { let this: Option<&mut Self> = unsafe { msg_send![self, init] }; this.map(|this| { - *this.state = state; - this.with_state(|state| { - state.window.setDelegate(Some(&*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.initial_fullscreen, initial_fullscreen); + Ivar::write(&mut this.previous_position, None); + Ivar::write(&mut this.previous_scale_factor, scale_factor); + + if scale_factor != 1.0 { + this.emit_static_scale_factor_changed_event(); + } + this.window.setDelegate(Some(this)); this }) } @@ -145,83 +78,71 @@ declare_class!( #[sel(windowShouldClose:)] fn window_should_close(&self, _: Option<&Object>) -> bool { trace_scope!("windowShouldClose:"); - self.with_state(|state| state.emit_event(WindowEvent::CloseRequested)); + self.emit_event(WindowEvent::CloseRequested); false } #[sel(windowWillClose:)] fn window_will_close(&self, _: Option<&Object>) { trace_scope!("windowWillClose:"); - self.with_state(|state| { - // `setDelegate:` retains the previous value and then autoreleases it - autoreleasepool(|_| { - // Since El Capitan, we need to be careful that delegate methods can't - // be called after the window closes. - state.window.setDelegate(None); - }); - state.emit_event(WindowEvent::Destroyed); + // `setDelegate:` retains the previous value and then autoreleases it + autoreleasepool(|_| { + // Since El Capitan, we need to be careful that delegate methods can't + // be called after the window closes. + self.window.setDelegate(None); }); + self.emit_event(WindowEvent::Destroyed); } #[sel(windowDidResize:)] - fn window_did_resize(&self, _: Option<&Object>) { + fn window_did_resize(&mut self, _: Option<&Object>) { trace_scope!("windowDidResize:"); - self.with_state(|state| { - // NOTE: WindowEvent::Resized is reported in frameDidChange. - state.emit_move_event(); - }); + // NOTE: WindowEvent::Resized is reported in frameDidChange. + self.emit_move_event(); } // This won't be triggered if the move was part of a resize. #[sel(windowDidMove:)] - fn window_did_move(&self, _: Option<&Object>) { + fn window_did_move(&mut self, _: Option<&Object>) { trace_scope!("windowDidMove:"); - self.with_state(|state| { - state.emit_move_event(); - }); + self.emit_move_event(); } #[sel(windowDidChangeBackingProperties:)] - fn window_did_change_backing_properties(&self, _: Option<&Object>) { + fn window_did_change_backing_properties(&mut self, _: Option<&Object>) { trace_scope!("windowDidChangeBackingProperties:"); - self.with_state(|state| { - state.emit_static_scale_factor_changed_event(); - }); + self.emit_static_scale_factor_changed_event(); } #[sel(windowDidBecomeKey:)] fn window_did_become_key(&self, _: Option<&Object>) { trace_scope!("windowDidBecomeKey:"); - self.with_state(|state| { - // TODO: center the cursor if the window had mouse grab when it - // lost focus - state.emit_event(WindowEvent::Focused(true)); - }); + // TODO: center the cursor if the window had mouse grab when it + // lost focus + self.emit_event(WindowEvent::Focused(true)); } #[sel(windowDidResignKey:)] fn window_did_resign_key(&self, _: Option<&Object>) { trace_scope!("windowDidResignKey:"); - self.with_state(|state| { - // It happens rather often, e.g. when the user is Cmd+Tabbing, that the - // NSWindowDelegate will receive a didResignKey event despite no event - // being received when the modifiers are released. This is because - // flagsChanged events are received by the NSView instead of the - // NSWindowDelegate, and as a result a tracked modifiers state can quite - // easily fall out of synchrony with reality. This requires us to emit - // a synthetic ModifiersChanged event when we lose focus. - - // TODO(madsmtm): Remove the need for this unsafety - let mut view = unsafe { Id::from_shared(state.window.view()) }; - - // Both update the state and emit a ModifiersChanged event. - if !view.state.modifiers.is_empty() { - view.state.modifiers = ModifiersState::empty(); - state.emit_event(WindowEvent::ModifiersChanged(view.state.modifiers)); - } - - state.emit_event(WindowEvent::Focused(false)); - }); + // It happens rather often, e.g. when the user is Cmd+Tabbing, that the + // NSWindowDelegate will receive a didResignKey event despite no event + // being received when the modifiers are released. This is because + // flagsChanged events are received by the NSView instead of the + // NSWindowDelegate, and as a result a tracked modifiers state can quite + // easily fall out of synchrony with reality. This requires us to emit + // a synthetic ModifiersChanged event when we lose focus. + + // TODO(madsmtm): Remove the need for this unsafety + let mut view = unsafe { Id::from_shared(self.window.view()) }; + + // Both update the state and emit a ModifiersChanged event. + if !view.state.modifiers.is_empty() { + view.state.modifiers = ModifiersState::empty(); + self.emit_event(WindowEvent::ModifiersChanged(view.state.modifiers)); + } + + self.emit_event(WindowEvent::Focused(false)); } /// Invoked when the dragged image enters destination bounds or frame @@ -237,10 +158,7 @@ declare_class!( filenames.into_iter().for_each(|file| { let path = PathBuf::from(file.to_string()); - - self.with_state(|state| { - state.emit_event(WindowEvent::HoveredFile(path)); - }); + self.emit_event(WindowEvent::HoveredFile(path)); }); true @@ -266,10 +184,7 @@ declare_class!( filenames.into_iter().for_each(|file| { let path = PathBuf::from(file.to_string()); - - self.with_state(|state| { - state.emit_event(WindowEvent::DroppedFile(path)); - }); + self.emit_event(WindowEvent::DroppedFile(path)); }); true @@ -285,7 +200,7 @@ declare_class!( #[sel(draggingExited:)] fn dragging_exited(&self, _: Option<&Object>) { trace_scope!("draggingExited:"); - self.with_state(|state| state.emit_event(WindowEvent::HoveredFileCancelled)); + self.emit_event(WindowEvent::HoveredFileCancelled); } /// Invoked when before enter fullscreen @@ -293,30 +208,28 @@ declare_class!( fn window_will_enter_fullscreen(&self, _: Option<&Object>) { trace_scope!("windowWillEnterFullscreen:"); - self.with_state(|state| { - state.with_window(|window| { - let mut shared_state = window.lock_shared_state("window_will_enter_fullscreen"); - shared_state.maximized = window.is_zoomed(); - let fullscreen = shared_state.fullscreen.as_ref(); - match fullscreen { - // Exclusive mode sets the state in `set_fullscreen` as the user - // can't enter exclusive mode by other means (like the - // fullscreen button on the window decorations) - Some(Fullscreen::Exclusive(_)) => (), - // `window_will_enter_fullscreen` was triggered and we're already - // in fullscreen, so we must've reached here by `set_fullscreen` - // as it updates the state - Some(Fullscreen::Borderless(_)) => (), - // Otherwise, we must've reached fullscreen by the user clicking - // on the green fullscreen button. Update state! - None => { - let current_monitor = Some(window.current_monitor_inner()); - shared_state.fullscreen = Some(Fullscreen::Borderless(current_monitor)) - } - } - shared_state.in_fullscreen_transition = true; - }) - }); + let mut shared_state = self + .window + .lock_shared_state("window_will_enter_fullscreen"); + shared_state.maximized = self.window.is_zoomed(); + let fullscreen = shared_state.fullscreen.as_ref(); + match fullscreen { + // Exclusive mode sets the state in `set_fullscreen` as the user + // can't enter exclusive mode by other means (like the + // fullscreen button on the window decorations) + Some(Fullscreen::Exclusive(_)) => (), + // `window_will_enter_fullscreen` was triggered and we're already + // in fullscreen, so we must've reached here by `set_fullscreen` + // as it updates the state + Some(Fullscreen::Borderless(_)) => (), + // Otherwise, we must've reached fullscreen by the user clicking + // on the green fullscreen button. Update state! + None => { + let current_monitor = Some(self.window.current_monitor_inner()); + shared_state.fullscreen = Some(Fullscreen::Borderless(current_monitor)) + } + } + shared_state.in_fullscreen_transition = true; } /// Invoked when before exit fullscreen @@ -324,12 +237,8 @@ declare_class!( fn window_will_exit_fullscreen(&self, _: Option<&Object>) { trace_scope!("windowWillExitFullScreen:"); - self.with_state(|state| { - state.with_window(|window| { - let mut shared_state = window.lock_shared_state("window_will_exit_fullscreen"); - shared_state.in_fullscreen_transition = true; - }); - }); + let mut shared_state = self.window.lock_shared_state("window_will_exit_fullscreen"); + shared_state.in_fullscreen_transition = true; } #[sel(window:willUseFullScreenPresentationOptions:)] @@ -348,37 +257,30 @@ declare_class!( // we don't, for consistency. If we do, it should be documented that the // user-provided options are ignored in exclusive fullscreen. let mut options = proposed_options; - self.with_state(|state| { - state.with_window(|window| { - let shared_state = - window.lock_shared_state("window_will_use_fullscreen_presentation_options"); - if let Some(Fullscreen::Exclusive(_)) = shared_state.fullscreen { - options = NSApplicationPresentationOptions::NSApplicationPresentationFullScreen - | NSApplicationPresentationOptions::NSApplicationPresentationHideDock - | NSApplicationPresentationOptions::NSApplicationPresentationHideMenuBar; - } - }) - }); + let shared_state = self + .window + .lock_shared_state("window_will_use_fullscreen_presentation_options"); + if let Some(Fullscreen::Exclusive(_)) = shared_state.fullscreen { + options = NSApplicationPresentationOptions::NSApplicationPresentationFullScreen + | NSApplicationPresentationOptions::NSApplicationPresentationHideDock + | NSApplicationPresentationOptions::NSApplicationPresentationHideMenuBar; + } options } /// Invoked when entered fullscreen #[sel(windowDidEnterFullscreen:)] - fn window_did_enter_fullscreen(&self, _: Option<&Object>) { + fn window_did_enter_fullscreen(&mut self, _: Option<&Object>) { trace_scope!("windowDidEnterFullscreen:"); - self.with_state(|state| { - state.initial_fullscreen = false; - state.with_window(|window| { - let mut shared_state = window.lock_shared_state("window_did_enter_fullscreen"); - shared_state.in_fullscreen_transition = false; - let target_fullscreen = shared_state.target_fullscreen.take(); - drop(shared_state); - if let Some(target_fullscreen) = target_fullscreen { - window.set_fullscreen(target_fullscreen); - } - }); - }); + *self.initial_fullscreen = false; + let mut shared_state = self.window.lock_shared_state("window_did_enter_fullscreen"); + shared_state.in_fullscreen_transition = false; + let target_fullscreen = shared_state.target_fullscreen.take(); + drop(shared_state); + if let Some(target_fullscreen) = target_fullscreen { + self.window.set_fullscreen(target_fullscreen); + } } /// Invoked when exited fullscreen @@ -386,18 +288,14 @@ declare_class!( fn window_did_exit_fullscreen(&self, _: Option<&Object>) { trace_scope!("windowDidExitFullscreen:"); - self.with_state(|state| { - state.with_window(|window| { - window.restore_state_from_fullscreen(); - let mut shared_state = window.lock_shared_state("window_did_exit_fullscreen"); - shared_state.in_fullscreen_transition = false; - let target_fullscreen = shared_state.target_fullscreen.take(); - drop(shared_state); - if let Some(target_fullscreen) = target_fullscreen { - window.set_fullscreen(target_fullscreen); - } - }) - }); + self.window.restore_state_from_fullscreen(); + let mut shared_state = self.window.lock_shared_state("window_did_exit_fullscreen"); + shared_state.in_fullscreen_transition = false; + let target_fullscreen = shared_state.target_fullscreen.take(); + drop(shared_state); + if let Some(target_fullscreen) = target_fullscreen { + self.window.set_fullscreen(target_fullscreen); + } } /// Invoked when fail to enter fullscreen @@ -419,61 +317,87 @@ declare_class!( #[sel(windowDidFailToEnterFullscreen:)] fn window_did_fail_to_enter_fullscreen(&self, _: Option<&Object>) { trace_scope!("windowDidFailToEnterFullscreen:"); - self.with_state(|state| { - state.with_window(|window| { - let mut shared_state = - window.lock_shared_state("window_did_fail_to_enter_fullscreen"); - shared_state.in_fullscreen_transition = false; - shared_state.target_fullscreen = None; - }); - if state.initial_fullscreen { - unsafe { - let _: () = msg_send![ - &*state.window, - performSelector: sel!(toggleFullScreen:), - withObject: ptr::null::(), - afterDelay: 0.5, - ]; - }; - } else { - state.with_window(|window| window.restore_state_from_fullscreen()); - } - }); + let mut shared_state = self + .window + .lock_shared_state("window_did_fail_to_enter_fullscreen"); + shared_state.in_fullscreen_transition = false; + shared_state.target_fullscreen = None; + if *self.initial_fullscreen { + unsafe { + let _: () = msg_send![ + &*self.window, + performSelector: sel!(toggleFullScreen:), + withObject: ptr::null::(), + afterDelay: 0.5, + ]; + }; + } else { + self.window.restore_state_from_fullscreen(); + } } // Invoked when the occlusion state of the window changes #[sel(windowDidChangeOcclusionState:)] fn window_did_change_occlusion_state(&self, _: Option<&Object>) { trace_scope!("windowDidChangeOcclusionState:"); - self.with_state(|state| { - state.emit_event(WindowEvent::Occluded( - !state - .window - .occlusionState() - .contains(NSWindowOcclusionState::NSWindowOcclusionStateVisible), - )) - }); + self.emit_event(WindowEvent::Occluded( + !self + .window + .occlusionState() + .contains(NSWindowOcclusionState::NSWindowOcclusionStateVisible), + )) } } ); impl WinitWindowDelegate { - pub fn new(window: Id, initial_fullscreen: bool) -> Id { - let state = WindowDelegateState::new(window, initial_fullscreen); + pub fn new(window: &WinitWindow, initial_fullscreen: bool) -> Id { unsafe { - // This is free'd in `dealloc` - let state_ptr = Box::into_raw(Box::new(state)) as *mut c_void; - msg_send_id![msg_send_id![Self::class(), alloc], initWithWinit: state_ptr] + msg_send_id![ + msg_send_id![Self::class(), alloc], + initWithWindow: window, + initialFullscreen: initial_fullscreen, + ] } } - // This function is definitely unsafe (&self -> &mut state), but labeling that - // would increase boilerplate and wouldn't really clarify anything... - fn with_state T, T>(&self, callback: F) { - let state_ptr = unsafe { - let state_ptr: *mut c_void = *self.state; - &mut *(state_ptr as *mut WindowDelegateState) + fn emit_event(&self, event: WindowEvent<'static>) { + let event = Event::WindowEvent { + window_id: WindowId(self.window.id()), + event, + }; + AppState::queue_event(EventWrapper::StaticEvent(event)); + } + + fn emit_static_scale_factor_changed_event(&mut self) { + let scale_factor = self.window.scale_factor(); + if scale_factor == *self.previous_scale_factor { + return; }; - callback(state_ptr); + + *self.previous_scale_factor = scale_factor; + let wrapper = EventWrapper::EventProxy(EventProxy::DpiChangedProxy { + window: self.window.clone(), + suggested_size: self.view_size(), + scale_factor, + }); + AppState::queue_event(wrapper); + } + + fn emit_move_event(&mut self) { + let rect = self.window.frame(); + let x = rect.origin.x as f64; + let y = util::bottom_left_to_top_left(rect); + if self.previous_position.as_deref() != Some(&(x, y)) { + *self.previous_position = Some(Box::new((x, y))); + let scale_factor = self.window.scale_factor(); + let physical_pos = LogicalPosition::::from((x, y)).to_physical(scale_factor); + self.emit_event(WindowEvent::Moved(physical_pos)); + } + } + + fn view_size(&self) -> LogicalSize { + let size = self.window.contentView().frame().size; + LogicalSize::new(size.width as f64, size.height as f64) } }