From 801fddbfcf48dbd1071199330d07f51dac2c28c3 Mon Sep 17 00:00:00 2001 From: Kirill Chibisov Date: Tue, 17 Oct 2023 04:54:12 +0400 Subject: [PATCH] Make `WindowBuilder` `Send + Sync` Window builder is always accessed by winit on the thread event loop is on, thus it's safe to mark the data it gets as `Send + Sync`. Each unsafe object is marked individually as `Send + Sync` instead of just implementing `Send` and `Sync` for the whole builder. --- CHANGELOG.md | 1 + src/lib.rs | 15 ++++++ src/platform/web.rs | 7 +-- src/platform_impl/ios/view.rs | 2 +- src/platform_impl/ios/window.rs | 2 +- src/platform_impl/linux/wayland/window/mod.rs | 2 +- src/platform_impl/linux/x11/window.rs | 6 +-- src/platform_impl/macos/window.rs | 8 +-- src/platform_impl/web/web_sys/canvas.rs | 4 +- src/platform_impl/web/window.rs | 5 +- src/platform_impl/windows/window.rs | 6 +-- src/window.rs | 51 ++++++++++--------- tests/send_objects.rs | 5 ++ tests/sync_object.rs | 5 ++ 14 files changed, 72 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64d8398d4d..6380f6810b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ And please only add new entries to the top of this list, right below the `# Unre - On Wayland, support `Occluded` event with xdg-shell v6 - Implement `AsFd`/`AsRawFd` for `EventLoop` on X11 and Wayland. - **Breaking:** Bump `ndk` version to `0.8.0`, ndk-sys to `0.5.0`, `android-activity` to `0.5.0`. +- Make `WindowBuilder` `Send + Sync`. # 0.29.1-beta diff --git a/src/lib.rs b/src/lib.rs index b505961f30..54b43cd4f5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -163,3 +163,18 @@ mod platform_impl; pub mod window; pub mod platform; + +/// Wrapper for objects which winit will access on the main thread so they are effectively `Send` +/// and `Sync`, since they always excute on a single thread. +/// +/// # Safety +/// +/// Winit can run only one event loop at the time and the event loop itself is tied to some thread. +/// The objects could be send across the threads, but once passed to winit, they execute on the +/// mean thread if the platform demands it. Thus marking such objects as `Send + Sync` is safe. +#[doc(hidden)] +#[derive(Clone, Debug)] +pub(crate) struct SendSyncWrapper(pub(crate) T); + +unsafe impl Send for SendSyncWrapper {} +unsafe impl Sync for SendSyncWrapper {} diff --git a/src/platform/web.rs b/src/platform/web.rs index 3b721af441..8a9a3d01ff 100644 --- a/src/platform/web.rs +++ b/src/platform/web.rs @@ -31,6 +31,7 @@ use crate::event::Event; use crate::event_loop::EventLoop; use crate::event_loop::EventLoopWindowTarget; use crate::window::{Window, WindowBuilder}; +use crate::SendSyncWrapper; use web_sys::HtmlCanvasElement; @@ -81,26 +82,22 @@ pub trait WindowBuilderExtWebSys { impl WindowBuilderExtWebSys for WindowBuilder { fn with_canvas(mut self, canvas: Option) -> Self { - self.platform_specific.canvas = canvas; - + self.platform_specific.canvas = SendSyncWrapper(canvas); self } fn with_prevent_default(mut self, prevent_default: bool) -> Self { self.platform_specific.prevent_default = prevent_default; - self } fn with_focusable(mut self, focusable: bool) -> Self { self.platform_specific.focusable = focusable; - self } fn with_append(mut self, append: bool) -> Self { self.platform_specific.append = append; - self } } diff --git a/src/platform_impl/ios/view.rs b/src/platform_impl/ios/view.rs index 206e8f5025..e564d86f15 100644 --- a/src/platform_impl/ios/view.rs +++ b/src/platform_impl/ios/view.rs @@ -473,7 +473,7 @@ impl WinitUIWindow { this.setRootViewController(Some(view_controller)); - match window_attributes.fullscreen.clone().map(Into::into) { + match window_attributes.fullscreen.0.clone().map(Into::into) { Some(Fullscreen::Exclusive(ref video_mode)) => { let monitor = video_mode.monitor(); let screen = monitor.ui_screen(mtm); diff --git a/src/platform_impl/ios/window.rs b/src/platform_impl/ios/window.rs index 8864c58b3a..cd82a20542 100644 --- a/src/platform_impl/ios/window.rs +++ b/src/platform_impl/ios/window.rs @@ -422,7 +422,7 @@ impl Window { // TODO: transparency, visible let main_screen = UIScreen::main(mtm); - let fullscreen = window_attributes.fullscreen.clone().map(Into::into); + let fullscreen = window_attributes.fullscreen.0.clone().map(Into::into); let screen = match fullscreen { Some(Fullscreen::Exclusive(ref video_mode)) => video_mode.monitor.ui_screen(mtm), Some(Fullscreen::Borderless(Some(ref monitor))) => monitor.ui_screen(mtm), diff --git a/src/platform_impl/linux/wayland/window/mod.rs b/src/platform_impl/linux/wayland/window/mod.rs index b8d539953a..96c6931e77 100644 --- a/src/platform_impl/linux/wayland/window/mod.rs +++ b/src/platform_impl/linux/wayland/window/mod.rs @@ -151,7 +151,7 @@ impl Window { window_state.set_resizable(attributes.resizable); // Set startup mode. - match attributes.fullscreen.map(Into::into) { + match attributes.fullscreen.0.map(Into::into) { Some(Fullscreen::Exclusive(_)) => { warn!("`Fullscreen::Exclusive` is ignored on Wayland"); } diff --git a/src/platform_impl/linux/x11/window.rs b/src/platform_impl/linux/x11/window.rs index 085be78634..0ceca722fc 100644 --- a/src/platform_impl/linux/x11/window.rs +++ b/src/platform_impl/linux/x11/window.rs @@ -150,7 +150,7 @@ impl UnownedWindow { let xconn = &event_loop.xconn; let atoms = xconn.atoms(); #[cfg(feature = "rwh_06")] - let root = match window_attrs.parent_window { + let root = match window_attrs.parent_window.0 { Some(rwh_06::RawWindowHandle::Xlib(handle)) => handle.window as xproto::Window, Some(rwh_06::RawWindowHandle::Xcb(handle)) => handle.window.get(), Some(raw) => unreachable!("Invalid raw window handle {raw:?} on X11"), @@ -547,10 +547,10 @@ impl UnownedWindow { if window_attrs.maximized { leap!(window.set_maximized_inner(window_attrs.maximized)).ignore_error(); } - if window_attrs.fullscreen.is_some() { + if window_attrs.fullscreen.0.is_some() { if let Some(flusher) = leap!(window - .set_fullscreen_inner(window_attrs.fullscreen.clone().map(Into::into))) + .set_fullscreen_inner(window_attrs.fullscreen.0.clone().map(Into::into))) { flusher.ignore_error() } diff --git a/src/platform_impl/macos/window.rs b/src/platform_impl/macos/window.rs index 5bb3d04268..31d6166e2f 100644 --- a/src/platform_impl/macos/window.rs +++ b/src/platform_impl/macos/window.rs @@ -295,7 +295,7 @@ impl WinitWindow { trace_scope!("WinitWindow::new"); let this = autoreleasepool(|_| { - let screen = match attrs.fullscreen.clone().map(Into::into) { + let screen = match attrs.fullscreen.0.clone().map(Into::into) { Some(Fullscreen::Borderless(Some(monitor))) | Some(Fullscreen::Exclusive(VideoMode { monitor, .. })) => { monitor.ns_screen().or_else(NSScreen::main) @@ -449,7 +449,7 @@ impl WinitWindow { .ok_or_else(|| os_error!(OsError::CreationError("Couldn't create `NSWindow`")))?; #[cfg(feature = "rwh_06")] - match attrs.parent_window { + match attrs.parent_window.0 { Some(rwh_06::RawWindowHandle::AppKit(handle)) => { // SAFETY: Caller ensures the pointer is valid or NULL // Unwrap is fine, since the pointer comes from `NonNull`. @@ -520,14 +520,14 @@ impl WinitWindow { } } - let delegate = WinitWindowDelegate::new(&this, attrs.fullscreen.is_some()); + let delegate = WinitWindowDelegate::new(&this, attrs.fullscreen.0.is_some()); // XXX Send `Focused(false)` right after creating the window delegate, so we won't // obscure the real focused events on the startup. delegate.queue_event(WindowEvent::Focused(false)); // Set fullscreen mode after we setup everything - this.set_fullscreen(attrs.fullscreen.map(Into::into)); + this.set_fullscreen(attrs.fullscreen.0.map(Into::into)); // Setting the window as key has to happen *after* we set the fullscreen // state, since otherwise we'll briefly see the window at normal size diff --git a/src/platform_impl/web/web_sys/canvas.rs b/src/platform_impl/web/web_sys/canvas.rs index 1196ae8dc7..b2dc6536da 100644 --- a/src/platform_impl/web/web_sys/canvas.rs +++ b/src/platform_impl/web/web_sys/canvas.rs @@ -63,7 +63,7 @@ impl Canvas { attr: &WindowAttributes, platform_attr: PlatformSpecificWindowBuilderAttributes, ) -> Result { - let canvas = match platform_attr.canvas { + let canvas = match platform_attr.canvas.0 { Some(canvas) => canvas, None => document .create_element("canvas") @@ -127,7 +127,7 @@ impl Canvas { super::set_canvas_position(&common.document, &common.raw, &common.style, position); } - if attr.fullscreen.is_some() { + if attr.fullscreen.0.is_some() { common.fullscreen_handler.request_fullscreen(); } diff --git a/src/platform_impl/web/window.rs b/src/platform_impl/web/window.rs index 6d456247b3..7d54528702 100644 --- a/src/platform_impl/web/window.rs +++ b/src/platform_impl/web/window.rs @@ -5,6 +5,7 @@ use crate::window::{ CursorGrabMode, CursorIcon, ImePurpose, ResizeDirection, Theme, UserAttentionType, WindowAttributes, WindowButtons, WindowId as RootWI, WindowLevel, }; +use crate::SendSyncWrapper; use web_sys::HtmlCanvasElement; @@ -455,7 +456,7 @@ impl From for WindowId { #[derive(Clone)] pub struct PlatformSpecificWindowBuilderAttributes { - pub(crate) canvas: Option, + pub(crate) canvas: SendSyncWrapper>, pub(crate) prevent_default: bool, pub(crate) focusable: bool, pub(crate) append: bool, @@ -464,7 +465,7 @@ pub struct PlatformSpecificWindowBuilderAttributes { impl Default for PlatformSpecificWindowBuilderAttributes { fn default() -> Self { Self { - canvas: None, + canvas: SendSyncWrapper(None), prevent_default: true, focusable: true, append: false, diff --git a/src/platform_impl/windows/window.rs b/src/platform_impl/windows/window.rs index ed539c2059..cda5563df8 100644 --- a/src/platform_impl/windows/window.rs +++ b/src/platform_impl/windows/window.rs @@ -1185,8 +1185,8 @@ impl<'a, T: 'static> InitData<'a, T> { win.set_enabled_buttons(attributes.enabled_buttons); - if attributes.fullscreen.is_some() { - win.set_fullscreen(attributes.fullscreen.map(Into::into)); + if attributes.fullscreen.0.is_some() { + win.set_fullscreen(attributes.fullscreen.0.map(Into::into)); unsafe { force_window_active(win.window.0) }; } else { let size = attributes @@ -1272,7 +1272,7 @@ where }; #[cfg(feature = "rwh_06")] - let parent = match attributes.parent_window { + let parent = match attributes.parent_window.0 { Some(rwh_06::RawWindowHandle::Win32(handle)) => { window_flags.set(WindowFlags::CHILD, true); if pl_attribs.menu.is_some() { diff --git a/src/window.rs b/src/window.rs index 1e7c3a996e..8800b62cf6 100644 --- a/src/window.rs +++ b/src/window.rs @@ -6,7 +6,7 @@ use crate::{ error::{ExternalError, NotSupportedError, OsError}, event_loop::EventLoopWindowTarget, monitor::{MonitorHandle, VideoMode}, - platform_impl, + platform_impl, SendSyncWrapper, }; pub use crate::icon::{BadIcon, Icon}; @@ -141,7 +141,6 @@ pub struct WindowAttributes { pub resizable: bool, pub enabled_buttons: WindowButtons, pub title: String, - pub fullscreen: Option, pub maximized: bool, pub visible: bool, pub transparent: bool, @@ -152,9 +151,10 @@ pub struct WindowAttributes { pub resize_increments: Option, pub content_protected: bool, pub window_level: WindowLevel, - #[cfg(feature = "rwh_06")] - pub parent_window: Option, pub active: bool, + #[cfg(feature = "rwh_06")] + pub(crate) parent_window: SendSyncWrapper>, + pub(crate) fullscreen: SendSyncWrapper>, } impl Default for WindowAttributes { @@ -169,7 +169,7 @@ impl Default for WindowAttributes { enabled_buttons: WindowButtons::all(), title: "winit window".to_owned(), maximized: false, - fullscreen: None, + fullscreen: SendSyncWrapper(None), visible: true, transparent: false, blur: false, @@ -180,12 +180,25 @@ impl Default for WindowAttributes { resize_increments: None, content_protected: false, #[cfg(feature = "rwh_06")] - parent_window: None, + parent_window: SendSyncWrapper(None), active: true, } } } +impl WindowAttributes { + /// Get the parent window stored on the attributes. + #[cfg(feature = "rwh_06")] + pub fn parent_window(&self) -> Option<&rwh_06::RawWindowHandle> { + self.parent_window.0.as_ref() + } + + /// Get `Fullscreen` option stored on the attributes. + pub fn fullscreen(&self) -> Option<&Fullscreen> { + self.fullscreen.0.as_ref() + } +} + impl WindowBuilder { /// Initializes a new builder with default values. #[inline] @@ -303,7 +316,7 @@ impl WindowBuilder { /// See [`Window::set_fullscreen`] for details. #[inline] pub fn with_fullscreen(mut self, fullscreen: Option) -> Self { - self.window.fullscreen = fullscreen; + self.window.fullscreen = SendSyncWrapper(fullscreen); self } @@ -479,7 +492,7 @@ impl WindowBuilder { mut self, parent_window: Option, ) -> Self { - self.window.parent_window = parent_window; + self.window.parent_window = SendSyncWrapper(parent_window); self } @@ -1510,12 +1523,9 @@ impl Window { #[cfg(feature = "rwh_06")] impl rwh_06::HasWindowHandle for Window { fn window_handle(&self) -> Result, rwh_06::HandleError> { - struct Wrapper(rwh_06::RawWindowHandle); - unsafe impl Send for Wrapper {} - let raw = self .window - .maybe_wait_on_main(|w| w.raw_window_handle_rwh_06().map(Wrapper))? + .maybe_wait_on_main(|w| w.raw_window_handle_rwh_06().map(SendSyncWrapper))? .0; // SAFETY: The window handle will never be deallocated while the window is alive. @@ -1526,12 +1536,9 @@ impl rwh_06::HasWindowHandle for Window { #[cfg(feature = "rwh_06")] impl rwh_06::HasDisplayHandle for Window { fn display_handle(&self) -> Result, rwh_06::HandleError> { - struct Wrapper(rwh_06::RawDisplayHandle); - unsafe impl Send for Wrapper {} - let raw = self .window - .maybe_wait_on_main(|w| w.raw_display_handle_rwh_06().map(Wrapper))? + .maybe_wait_on_main(|w| w.raw_display_handle_rwh_06().map(SendSyncWrapper))? .0; // SAFETY: The window handle will never be deallocated while the window is alive. @@ -1542,10 +1549,8 @@ impl rwh_06::HasDisplayHandle for Window { #[cfg(feature = "rwh_05")] unsafe impl rwh_05::HasRawWindowHandle for Window { fn raw_window_handle(&self) -> rwh_05::RawWindowHandle { - struct Wrapper(rwh_05::RawWindowHandle); - unsafe impl Send for Wrapper {} self.window - .maybe_wait_on_main(|w| Wrapper(w.raw_window_handle_rwh_05())) + .maybe_wait_on_main(|w| SendSyncWrapper(w.raw_window_handle_rwh_05())) .0 } } @@ -1557,10 +1562,8 @@ unsafe impl rwh_05::HasRawDisplayHandle for Window { /// /// [`EventLoop`]: crate::event_loop::EventLoop fn raw_display_handle(&self) -> rwh_05::RawDisplayHandle { - struct Wrapper(rwh_05::RawDisplayHandle); - unsafe impl Send for Wrapper {} self.window - .maybe_wait_on_main(|w| Wrapper(w.raw_display_handle_rwh_05())) + .maybe_wait_on_main(|w| SendSyncWrapper(w.raw_display_handle_rwh_05())) .0 } } @@ -1568,10 +1571,8 @@ unsafe impl rwh_05::HasRawDisplayHandle for Window { #[cfg(feature = "rwh_04")] unsafe impl rwh_04::HasRawWindowHandle for Window { fn raw_window_handle(&self) -> rwh_04::RawWindowHandle { - struct Wrapper(rwh_04::RawWindowHandle); - unsafe impl Send for Wrapper {} self.window - .maybe_wait_on_main(|w| Wrapper(w.raw_window_handle_rwh_04())) + .maybe_wait_on_main(|w| SendSyncWrapper(w.raw_window_handle_rwh_04())) .0 } } diff --git a/tests/send_objects.rs b/tests/send_objects.rs index 9462d0736f..7941b16769 100644 --- a/tests/send_objects.rs +++ b/tests/send_objects.rs @@ -16,6 +16,11 @@ fn window_send() { needs_send::(); } +#[test] +fn window_builder_send() { + needs_send::(); +} + #[test] fn ids_send() { // ensures that the various `..Id` types implement `Send` diff --git a/tests/sync_object.rs b/tests/sync_object.rs index dad6520248..23c6012545 100644 --- a/tests/sync_object.rs +++ b/tests/sync_object.rs @@ -6,3 +6,8 @@ fn window_sync() { // ensures that `winit::Window` implements `Sync` needs_sync::(); } + +#[test] +fn window_builder_sync() { + needs_sync::(); +}