From b3000cf24714ae35b865037203b02bcb877ca1c2 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 13 Oct 2023 23:07:14 +0200 Subject: [PATCH] Add OwnedWindowHandle to avoid lifetime on WindowHandle<'_> --- examples/child_window.rs | 7 ++- src/platform_impl/android/mod.rs | 12 +++++ src/platform_impl/ios/mod.rs | 2 +- src/platform_impl/ios/window.rs | 13 ++++++ src/platform_impl/linux/mod.rs | 37 +++++++++++++++ src/platform_impl/linux/x11/window.rs | 12 ++--- src/platform_impl/macos/mod.rs | 2 +- src/platform_impl/macos/window.rs | 67 +++++++++++++++++++-------- src/platform_impl/orbital/mod.rs | 2 +- src/platform_impl/orbital/window.rs | 12 +++++ src/platform_impl/web/mod.rs | 4 +- src/platform_impl/web/window.rs | 12 +++++ src/platform_impl/windows/mod.rs | 2 +- src/platform_impl/windows/window.rs | 53 ++++++++++++--------- src/window.rs | 12 ++--- 15 files changed, 183 insertions(+), 66 deletions(-) diff --git a/examples/child_window.rs b/examples/child_window.rs index 9234d9e4e6..f507de2366 100644 --- a/examples/child_window.rs +++ b/examples/child_window.rs @@ -17,7 +17,7 @@ fn main() -> Result<(), impl std::error::Error> { dpi::{LogicalPosition, LogicalSize, Position}, event::{ElementState, Event, KeyEvent, WindowEvent}, event_loop::{EventLoop, EventLoopWindowTarget}, - raw_window_handle::HasRawWindowHandle, + raw_window_handle::HasWindowHandle, window::{Window, WindowBuilder, WindowId}, }; @@ -26,14 +26,13 @@ fn main() -> Result<(), impl std::error::Error> { event_loop: &EventLoopWindowTarget<()>, windows: &mut HashMap, ) { - let parent = parent.raw_window_handle().unwrap(); + let parent = parent.window_handle().unwrap(); let mut builder = WindowBuilder::new() .with_title("child window") .with_inner_size(LogicalSize::new(200.0f32, 200.0f32)) .with_position(Position::Logical(LogicalPosition::new(0.0, 0.0))) .with_visible(true); - // `with_parent_window` is unsafe. Parent window must be a valid window. - builder = unsafe { builder.with_parent_window(Some(parent)) }; + builder = builder.with_parent_window(Some(parent)); let child_window = builder.build(event_loop).unwrap(); let id = child_window.id(); diff --git a/src/platform_impl/android/mod.rs b/src/platform_impl/android/mod.rs index 2f9bbd413b..eb3bc144d1 100644 --- a/src/platform_impl/android/mod.rs +++ b/src/platform_impl/android/mod.rs @@ -759,6 +759,18 @@ impl DeviceId { #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] pub struct PlatformSpecificWindowBuilderAttributes; +#[derive(Debug, Clone)] +pub(crate) struct OwnedWindowHandle {} + +impl OwnedWindowHandle { + #[cfg(feature = "rwh_06")] + pub(crate) fn new_parent_window(_handle: rwh_06::WindowHandle<'_>) -> Self { + // Parent windows are currently unsupported, though owned window + // handles would be implementable. + Self {} + } +} + pub(crate) struct Window { app: AndroidApp, redraw_requester: RedrawRequester, diff --git a/src/platform_impl/ios/mod.rs b/src/platform_impl/ios/mod.rs index dec71cdeff..25ed374438 100644 --- a/src/platform_impl/ios/mod.rs +++ b/src/platform_impl/ios/mod.rs @@ -73,7 +73,7 @@ pub(crate) use self::{ EventLoop, EventLoopProxy, EventLoopWindowTarget, PlatformSpecificEventLoopAttributes, }, monitor::{MonitorHandle, VideoMode}, - window::{PlatformSpecificWindowBuilderAttributes, Window, WindowId}, + window::{OwnedWindowHandle, PlatformSpecificWindowBuilderAttributes, Window, WindowId}, }; use self::uikit::UIScreen; diff --git a/src/platform_impl/ios/window.rs b/src/platform_impl/ios/window.rs index 3d5bf2b82d..edb87c1244 100644 --- a/src/platform_impl/ios/window.rs +++ b/src/platform_impl/ios/window.rs @@ -25,6 +25,19 @@ use crate::{ }, }; +#[derive(Debug, Clone)] +pub(crate) struct OwnedWindowHandle {} + +impl OwnedWindowHandle { + #[cfg(feature = "rwh_06")] + pub(crate) fn new_parent_window(_handle: rwh_06::WindowHandle<'_>) -> Self { + // Parent windows are currently unsupported, though owned window + // handles would be implementable (would work similar to macOS). + warn!("parent windows are unsupported on iOS"); + Self {} + } +} + pub struct Inner { window: Id, view_controller: Id, diff --git a/src/platform_impl/linux/mod.rs b/src/platform_impl/linux/mod.rs index 23715411c8..50eb68396d 100644 --- a/src/platform_impl/linux/mod.rs +++ b/src/platform_impl/linux/mod.rs @@ -138,6 +138,43 @@ impl fmt::Display for OsError { } } +#[derive(Debug, Clone)] +#[allow(dead_code)] +pub(crate) enum OwnedWindowHandle { + #[cfg(x11_platform)] + X(x11rb::protocol::xproto::Window), + #[cfg(wayland_platform)] + Wayland, +} + +impl OwnedWindowHandle { + #[cfg(feature = "rwh_06")] + pub(crate) fn new_parent_window(handle: rwh_06::WindowHandle<'_>) -> Self { + // TODO: Do we need to do something extra to extend the lifetime of + // the window lives beyond the passed-in handle? + match handle.as_raw() { + #[cfg(x11_platform)] + rwh_06::RawWindowHandle::Xlib(handle) => { + Self::X(handle.window as x11rb::protocol::xproto::Window) + } + #[cfg(x11_platform)] + rwh_06::RawWindowHandle::Xcb(handle) => Self::X(handle.window.get()), + #[cfg(wayland_platform)] + rwh_06::RawWindowHandle::Wayland(_handle) => { + // Wayland does not currently support parent windows, but it + // could support owned handles. + Self::Wayland + } + #[cfg(not(x11_platform))] + handle => panic!("invalid window handle {handle:?} on Wayland"), + #[cfg(not(wayland_platform))] + handle => panic!("invalid window handle {handle:?} on X11"), + #[cfg(all(x11_platform, wayland_platform))] + handle => panic!("invalid window handle {handle:?} on X11 or Wayland"), + } + } +} + pub(crate) enum Window { #[cfg(x11_platform)] X(x11::Window), diff --git a/src/platform_impl/linux/x11/window.rs b/src/platform_impl/linux/x11/window.rs index 9c6d6da1ab..af22e3b309 100644 --- a/src/platform_impl/linux/x11/window.rs +++ b/src/platform_impl/linux/x11/window.rs @@ -22,7 +22,8 @@ use crate::{ event_loop::AsyncRequestSerial, platform_impl::{ x11::{atoms::*, MonitorHandle as X11MonitorHandle, WakeSender, X11Error}, - Fullscreen, MonitorHandle as PlatformMonitorHandle, OsError, PlatformIcon, + Fullscreen, MonitorHandle as PlatformMonitorHandle, OsError, + OwnedWindowHandle as PlatformOwnedWindowHandle, PlatformIcon, PlatformSpecificWindowBuilderAttributes, VideoMode as PlatformVideoMode, }, window::{ @@ -144,15 +145,12 @@ impl UnownedWindow { ) -> Result { let xconn = &event_loop.xconn; let atoms = xconn.atoms(); - #[cfg(feature = "rwh_06")] let root = match window_attrs.parent_window { - 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"), + Some(PlatformOwnedWindowHandle::X(handle)) => handle, + #[cfg(wayland_platform)] + Some(handle) => panic!("invalid window handle {handle:?} on X11"), None => event_loop.root, }; - #[cfg(not(feature = "rwh_06"))] - let root = event_loop.root; let mut monitors = leap!(xconn.available_monitors()); let guessed_monitor = if monitors.is_empty() { diff --git a/src/platform_impl/macos/mod.rs b/src/platform_impl/macos/mod.rs index 7169ca9dbf..e59ea83f1a 100644 --- a/src/platform_impl/macos/mod.rs +++ b/src/platform_impl/macos/mod.rs @@ -27,7 +27,7 @@ pub(crate) use self::{ }; use crate::event::DeviceId as RootDeviceId; -pub(crate) use self::window::Window; +pub(crate) use self::window::{OwnedWindowHandle, Window}; pub(crate) use crate::icon::NoIcon as PlatformIcon; pub(crate) use crate::platform_impl::Fullscreen; diff --git a/src/platform_impl/macos/window.rs b/src/platform_impl/macos/window.rs index 9c823c8e94..2ef5bf1e1e 100644 --- a/src/platform_impl/macos/window.rs +++ b/src/platform_impl/macos/window.rs @@ -68,7 +68,8 @@ impl Window { ) -> Result { let mtm = MainThreadMarker::new() .expect("windows can only be created on the main thread on macOS"); - let (window, _delegate) = autoreleasepool(|_| WinitWindow::new(attributes, pl_attribs))?; + let (window, _delegate) = + autoreleasepool(|_| WinitWindow::new(mtm, attributes, pl_attribs))?; Ok(Window { window: MainThreadBound::new(window, mtm), _delegate: MainThreadBound::new(_delegate, mtm), @@ -109,6 +110,40 @@ impl From for WindowId { } } +#[derive(Debug)] +pub(crate) struct OwnedWindowHandle { + ns_view: MainThreadBound>, +} + +impl Clone for OwnedWindowHandle { + fn clone(&self) -> Self { + Self { + ns_view: MainThreadMarker::run_on_main(|mtm| { + MainThreadBound::new(self.ns_view.get(mtm).clone(), mtm) + }), + } + } +} + +impl OwnedWindowHandle { + #[cfg(feature = "rwh_06")] + pub(crate) fn new_parent_window(handle: rwh_06::WindowHandle<'_>) -> Self { + let mtm = + MainThreadMarker::new().expect("can only have handles on macOS on the main thread"); + let ns_view = match handle.as_raw() { + rwh_06::RawWindowHandle::AppKit(handle) => { + // SAFETY: Taking `WindowHandle<'_>` ensures that the pointer is valid. + // Unwrap is fine, since the pointer comes from `NonNull`. + unsafe { Id::retain(handle.ns_view.as_ptr().cast()) }.unwrap() + } + handle => panic!("invalid window handle {handle:?} on macOS"), + }; + Self { + ns_view: MainThreadBound::new(ns_view, mtm), + } + } +} + #[derive(Clone)] pub struct PlatformSpecificWindowBuilderAttributes { pub movable_by_window_background: bool, @@ -289,6 +324,7 @@ impl Drop for SharedStateMutexGuard<'_> { impl WinitWindow { #[allow(clippy::type_complexity)] fn new( + mtm: MainThreadMarker, attrs: WindowAttributes, pl_attrs: PlatformSpecificWindowBuilderAttributes, ) -> Result<(Id, Id), RootOsError> { @@ -448,25 +484,16 @@ impl WinitWindow { }) .ok_or_else(|| os_error!(OsError::CreationError("Couldn't create `NSWindow`")))?; - #[cfg(feature = "rwh_06")] - match attrs.parent_window { - Some(rwh_06::RawWindowHandle::AppKit(handle)) => { - // SAFETY: Caller ensures the pointer is valid or NULL - // Unwrap is fine, since the pointer comes from `NonNull`. - let parent_view: Id = - unsafe { Id::retain(handle.ns_view.as_ptr().cast()) }.unwrap(); - let parent = parent_view.window().ok_or_else(|| { - os_error!(OsError::CreationError( - "parent view should be installed in a window" - )) - })?; - - // SAFETY: We know that there are no parent -> child -> parent cycles since the only place in `winit` - // where we allow making a window a child window is right here, just after it's been created. - unsafe { parent.addChildWindow(&this, NSWindowOrderingMode::NSWindowAbove) }; - } - Some(raw) => panic!("Invalid raw window handle {raw:?} on macOS"), - None => (), + if let Some(parent_window) = attrs.parent_window { + let parent = parent_window.ns_view.get(mtm).window().ok_or_else(|| { + os_error!(OsError::CreationError( + "parent view should be installed in a window" + )) + })?; + + // SAFETY: We know that there are no parent -> child -> parent cycles since the only place in `winit` + // where we allow making a window a child window is right here, just after it's been created. + unsafe { parent.addChildWindow(&this, NSWindowOrderingMode::NSWindowAbove) }; } let view = WinitView::new(&this, pl_attrs.accepts_first_mouse); diff --git a/src/platform_impl/orbital/mod.rs b/src/platform_impl/orbital/mod.rs index 5aa56328a7..444d385104 100644 --- a/src/platform_impl/orbital/mod.rs +++ b/src/platform_impl/orbital/mod.rs @@ -9,7 +9,7 @@ use crate::dpi::{PhysicalPosition, PhysicalSize}; pub use self::event_loop::{EventLoop, EventLoopProxy, EventLoopWindowTarget}; mod event_loop; -pub use self::window::Window; +pub(crate) use self::window::{OwnedWindowHandle, Window}; mod window; struct RedoxSocket { diff --git a/src/platform_impl/orbital/window.rs b/src/platform_impl/orbital/window.rs index ac7682d7ad..4fa8d61535 100644 --- a/src/platform_impl/orbital/window.rs +++ b/src/platform_impl/orbital/window.rs @@ -25,6 +25,18 @@ const ORBITAL_FLAG_BORDERLESS: char = 'l'; const ORBITAL_FLAG_RESIZABLE: char = 'r'; const ORBITAL_FLAG_TRANSPARENT: char = 't'; +#[derive(Debug, Clone)] +pub(crate) struct OwnedWindowHandle {} + +impl OwnedWindowHandle { + #[cfg(feature = "rwh_06")] + pub(crate) fn new_parent_window(_handle: rwh_06::WindowHandle<'_>) -> Self { + // Parent windows are currently unsupported, though owned window + // handles would be implementable. + Self {} + } +} + pub struct Window { window_socket: Arc, redraws: Arc>>, diff --git a/src/platform_impl/web/mod.rs b/src/platform_impl/web/mod.rs index 3abd268414..906b33da25 100644 --- a/src/platform_impl/web/mod.rs +++ b/src/platform_impl/web/mod.rs @@ -34,7 +34,9 @@ pub(crate) use self::event_loop::{ EventLoop, EventLoopProxy, EventLoopWindowTarget, PlatformSpecificEventLoopAttributes, }; pub use self::monitor::{MonitorHandle, VideoMode}; -pub use self::window::{PlatformSpecificWindowBuilderAttributes, Window, WindowId}; +pub(crate) use self::window::{ + OwnedWindowHandle, PlatformSpecificWindowBuilderAttributes, Window, WindowId, +}; pub(crate) use self::keyboard::KeyEventExtra; pub(crate) use crate::icon::NoIcon as PlatformIcon; diff --git a/src/platform_impl/web/window.rs b/src/platform_impl/web/window.rs index 42252f49d5..a4ae53e80a 100644 --- a/src/platform_impl/web/window.rs +++ b/src/platform_impl/web/window.rs @@ -17,6 +17,18 @@ use std::rc::Rc; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; +#[derive(Debug, Clone)] +pub(crate) struct OwnedWindowHandle {} + +impl OwnedWindowHandle { + #[cfg(feature = "rwh_06")] + pub(crate) fn new_parent_window(_handle: rwh_06::WindowHandle<'_>) -> Self { + // Parent windows are currently unsupported, though owned window + // handles would be implementable. + Self {} + } +} + pub struct Window { inner: Dispatcher, } diff --git a/src/platform_impl/windows/mod.rs b/src/platform_impl/windows/mod.rs index bc6caeb6f5..4db02407f5 100644 --- a/src/platform_impl/windows/mod.rs +++ b/src/platform_impl/windows/mod.rs @@ -12,7 +12,7 @@ pub(crate) use self::{ }, icon::WinIcon, monitor::{MonitorHandle, VideoMode}, - window::Window, + window::{OwnedWindowHandle, Window}, }; pub use self::icon::WinIcon as PlatformIcon; diff --git a/src/platform_impl/windows/window.rs b/src/platform_impl/windows/window.rs index 58c004c0b3..1743089303 100644 --- a/src/platform_impl/windows/window.rs +++ b/src/platform_impl/windows/window.rs @@ -76,6 +76,23 @@ use crate::{ }, }; +#[derive(Debug, Clone)] +pub(crate) struct OwnedWindowHandle { + hwnd: HWND, +} + +impl OwnedWindowHandle { + #[cfg(feature = "rwh_06")] + pub(crate) fn new_parent_window(handle: rwh_06::WindowHandle<'_>) -> Self { + // TODO: Do we need to do something to extend the lifetime of the window handle? + let hwnd = match handle.as_raw() { + rwh_06::RawWindowHandle::Win32(handle) => handle.hwnd.get() as HWND, + handle => panic!("invalid window handle {handle:?} on Windows"), + }; + Self { hwnd } + } +} + /// The Win32 implementation of the main `Window` object. pub(crate) struct Window { /// Main handle for the window. @@ -1177,33 +1194,25 @@ where // so the diffing later can work. window_flags.set(WindowFlags::CLOSABLE, true); - let mut fallback_parent = || match pl_attribs.owner { - Some(parent) => { - window_flags.set(WindowFlags::POPUP, true); - Some(parent) + let parent = if let Some(parent_window) = &attributes.parent_window { + window_flags.set(WindowFlags::CHILD, true); + if pl_attribs.menu.is_some() { + warn!("Setting a menu on a child window is unsupported"); } - None => { - window_flags.set(WindowFlags::ON_TASKBAR, true); - None - } - }; - - #[cfg(feature = "rwh_06")] - let parent = match attributes.parent_window { - Some(rwh_06::RawWindowHandle::Win32(handle)) => { - window_flags.set(WindowFlags::CHILD, true); - if pl_attribs.menu.is_some() { - warn!("Setting a menu on a child window is unsupported"); + Some(parent_window.hwnd) + } else { + match pl_attribs.owner { + Some(parent) => { + window_flags.set(WindowFlags::POPUP, true); + Some(parent) + } + None => { + window_flags.set(WindowFlags::ON_TASKBAR, true); + None } - Some(handle.hwnd.get() as HWND) } - Some(raw) => unreachable!("Invalid raw window handle {raw:?} on Windows"), - None => fallback_parent(), }; - #[cfg(not(feature = "rwh_06"))] - let parent = fallback_parent(); - let mut initdata = InitData { event_loop, attributes, diff --git a/src/window.rs b/src/window.rs index 8f7af7bbc8..b1647dc98b 100644 --- a/src/window.rs +++ b/src/window.rs @@ -150,8 +150,7 @@ pub struct WindowAttributes { pub resize_increments: Option, pub content_protected: bool, pub window_level: WindowLevel, - #[cfg(feature = "rwh_06")] - pub parent_window: Option, + pub(crate) parent_window: Option, pub active: bool, } @@ -176,7 +175,6 @@ impl Default for WindowAttributes { preferred_theme: None, resize_increments: None, content_protected: false, - #[cfg(feature = "rwh_06")] parent_window: None, active: true, } @@ -461,11 +459,9 @@ impl WindowBuilder { /// - **Android / iOS / Wayland / Web:** Unsupported. #[cfg(feature = "rwh_06")] #[inline] - pub unsafe fn with_parent_window( - mut self, - parent_window: Option, - ) -> Self { - self.window.parent_window = parent_window; + pub fn with_parent_window(mut self, parent_window: Option>) -> Self { + self.window.parent_window = + parent_window.map(platform_impl::OwnedWindowHandle::new_parent_window); self }