From 7a8d1d76a9fdef99648c6289acd799952019707e Mon Sep 17 00:00:00 2001 From: jtnunley Date: Tue, 4 Apr 2023 11:44:40 -0700 Subject: [PATCH 1/3] Make Active a no-op on all platforms --- src/borrowed.rs | 209 +++++++----------------------------------------- 1 file changed, 28 insertions(+), 181 deletions(-) diff --git a/src/borrowed.rs b/src/borrowed.rs index b38d3d8..752285d 100644 --- a/src/borrowed.rs +++ b/src/borrowed.rs @@ -2,9 +2,7 @@ //! //! These should be 100% safe to pass around and use, no possibility of dangling or invalidity. -#[cfg(all(not(feature = "std"), target_os = "android"))] -compile_error!("Using borrowed handles on Android requires the `std` feature to be enabled."); - +use core::cell::UnsafeCell; use core::fmt; use core::hash::{Hash, Hasher}; use core::marker::PhantomData; @@ -24,43 +22,11 @@ use crate::{HasRawDisplayHandle, HasRawWindowHandle, RawDisplayHandle, RawWindow /// /// ## Explanation /// -/// On Android, there is an [Activity]-global [`ANativeWindow`] object that is used for drawing. This -/// handle is used [within the `RawWindowHandle` type] for Android NDK, since it is necessary for GFX -/// APIs to draw to the screen. -/// -/// However, the [`ANativeWindow`] type can be arbitrarily invalidated by the underlying Android runtime. -/// The reasoning for this is complicated, but this idea is exposed to native code through the -/// [`onNativeWindowCreated`] and [`onNativeWindowDestroyed`] callbacks. To save you a click, the -/// conditions associated with these callbacks are: -/// -/// - [`onNativeWindowCreated`] provides a valid [`ANativeWindow`] pointer that can be used for drawing. -/// - [`onNativeWindowDestroyed`] indicates that the previous [`ANativeWindow`] pointer is no longer -/// valid. The documentation clarifies that, *once the function returns*, the [`ANativeWindow`] pointer -/// can no longer be used for drawing without resulting in undefined behavior. -/// -/// In [`winit`], these are exposed via the [`Resumed`] and [`Suspended`] events, respectively. Therefore, -/// between the last [`Suspended`] event and the next [`Resumed`] event, it is undefined behavior to use -/// the raw window handle. This condition makes it tricky to define an API that safely wraps the raw -/// window handles, since an existing window handle can be made invalid at any time. -/// -/// The Android docs specifies that the [`ANativeWindow`] pointer is still valid while the application -/// is still in the [`onNativeWindowDestroyed`] block, and suggests that synchronization needs to take -/// place to ensure that the pointer has been invalidated before the function returns. `Active` aims -/// to be the solution to this problem. It keeps track of all currently active window handles, and -/// blocks until all of them are dropped before allowing the application to enter the suspended state. -/// -/// [Activity]: https://developer.android.com/reference/android/app/Activity -/// [`ANativeWindow`]: https://developer.android.com/ndk/reference/group/a-native-window -/// [within the `RawWindowHandle` type]: struct.AndroidNdkWindowHandle.html#structfield.a_native_window -/// [`onNativeWindowCreated`]: https://developer.android.com/ndk/reference/struct/a-native-activity-callbacks#onnativewindowcreated -/// [`onNativeWindowDestroyed`]: https://developer.android.com/ndk/reference/struct/a-native-activity-callbacks#onnativewindowdestroyed -/// [`Resumed`]: https://docs.rs/winit/latest/winit/event/enum.Event.html#variant.Resumed -/// [`Suspended`]: https://docs.rs/winit/latest/winit/event/enum.Event.html#variant.Suspended -/// [`sdl2`]: https://crates.io/crates/sdl2 -/// [`RawWindowHandle`]: https://docs.rs/raw-window-handle/latest/raw_window_handle/enum.RawWindowHandle.html -/// [`HasRawWindowHandle`]: https://docs.rs/raw-window-handle/latest/raw_window_handle/trait.HasRawWindowHandle.html -/// [`winit`]: https://crates.io/crates/winit -pub struct Active(imp::Active); +/// On Android, it was previously believed that the application could enter the suspended state +/// and immediately invalidate all window handles. However, it was later discovered that the +/// handle actually remains valid, but the window does not produce any more GPU buffers. This +/// type is a no-op and will be removed at the next major release. +pub struct Active(()); impl fmt::Debug for Active { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -77,7 +43,7 @@ impl fmt::Debug for Active { /// On non-Android platforms, this is a ZST. On Android, this is a reference counted handle that /// keeps the application active while it is alive. #[derive(Clone)] -pub struct ActiveHandle<'a>(imp::ActiveHandle<'a>); +pub struct ActiveHandle<'a>(PhantomData<&'a UnsafeCell<()>>); impl<'a> fmt::Debug for ActiveHandle<'a> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -97,7 +63,7 @@ impl Active { /// let active = Active::new(); /// ``` pub const fn new() -> Self { - Self(imp::Active::new()) + Self(()) } /// Get a live window handle. @@ -121,7 +87,7 @@ impl Active { /// active.set_inactive(); /// ``` pub fn handle(&self) -> Option> { - self.0.handle().map(ActiveHandle) + Some(ActiveHandle(PhantomData)) } /// Set the application to be inactive. @@ -140,9 +106,7 @@ impl Active { /// // Set the application to be inactive. /// active.set_inactive(); /// ``` - pub fn set_inactive(&self) { - self.0.set_inactive() - } + pub fn set_inactive(&self) {} /// Set the application to be active. /// @@ -163,9 +127,7 @@ impl Active { /// // Set the application to be inactive. /// active.set_inactive(); /// ``` - pub unsafe fn set_active(&self) { - self.0.set_active() - } + pub unsafe fn set_active(&self) {} } impl ActiveHandle<'_> { @@ -190,7 +152,23 @@ impl ActiveHandle<'_> { /// let handle = unsafe { ActiveHandle::new_unchecked() }; /// ``` pub unsafe fn new_unchecked() -> Self { - Self(imp::ActiveHandle::new_unchecked()) + Self(PhantomData) + } + + /// Create a new `ActiveHandle`. + /// + /// This is safe because the handle is always active. + /// + /// # Example + /// + /// ``` + /// use raw_window_handle::ActiveHandle; + /// let handle = ActiveHandle::new(); + /// ``` + #[allow(clippy::new_without_default)] + pub fn new() -> Self { + // SAFETY: The handle is always active. + unsafe { super::ActiveHandle::new_unchecked() } } } @@ -482,134 +460,3 @@ impl std::error::Error for HandleError {} /// _assert::>(); /// ``` fn _not_send_or_sync() {} - -#[cfg(not(any(target_os = "android", raw_window_handle_force_refcount)))] -#[cfg_attr(docsrs, doc(cfg(not(target_os = "android"))))] -mod imp { - //! We don't need to refcount the handles, so we can just use no-ops. - - use core::cell::UnsafeCell; - use core::marker::PhantomData; - - pub(super) struct Active; - - #[derive(Clone)] - pub(super) struct ActiveHandle<'a> { - _marker: PhantomData<&'a UnsafeCell<()>>, - } - - impl Active { - pub(super) const fn new() -> Self { - Self - } - - pub(super) fn handle(&self) -> Option> { - // SAFETY: The handle is always active. - Some(unsafe { ActiveHandle::new_unchecked() }) - } - - pub(super) unsafe fn set_active(&self) {} - - pub(super) fn set_inactive(&self) {} - } - - impl ActiveHandle<'_> { - pub(super) unsafe fn new_unchecked() -> Self { - Self { - _marker: PhantomData, - } - } - } - - impl Drop for ActiveHandle<'_> { - fn drop(&mut self) { - // Done for consistency with the refcounted version. - } - } - - impl super::ActiveHandle<'_> { - /// Create a new `ActiveHandle`. - /// - /// This is safe because the handle is always active. - /// - /// # Example - /// - /// ``` - /// use raw_window_handle::ActiveHandle; - /// let handle = ActiveHandle::new(); - /// ``` - #[allow(clippy::new_without_default)] - pub fn new() -> Self { - // SAFETY: The handle is always active. - unsafe { super::ActiveHandle::new_unchecked() } - } - } -} - -#[cfg(any(target_os = "android", raw_window_handle_force_refcount))] -#[cfg_attr(docsrs, doc(cfg(any(target_os = "android"))))] -mod imp { - //! We need to refcount the handles, so we use an `RwLock` to do so. - - use std::sync::{RwLock, RwLockReadGuard}; - - pub(super) struct Active { - active: RwLock, - } - - pub(super) struct ActiveHandle<'a> { - inner: Option>, - } - - struct Inner<'a> { - _read_guard: RwLockReadGuard<'a, bool>, - active: &'a Active, - } - - impl Clone for ActiveHandle<'_> { - fn clone(&self) -> Self { - Self { - inner: self.inner.as_ref().map(|inner| Inner { - _read_guard: inner.active.active.read().unwrap(), - active: inner.active, - }), - } - } - } - - impl Active { - pub(super) const fn new() -> Self { - Self { - active: RwLock::new(false), - } - } - - pub(super) fn handle(&self) -> Option> { - let active = self.active.read().ok()?; - if !*active { - return None; - } - - Some(ActiveHandle { - inner: Some(Inner { - _read_guard: active, - active: self, - }), - }) - } - - pub(super) unsafe fn set_active(&self) { - *self.active.write().unwrap() = true; - } - - pub(super) fn set_inactive(&self) { - *self.active.write().unwrap() = false; - } - } - - impl ActiveHandle<'_> { - pub(super) unsafe fn new_unchecked() -> Self { - Self { inner: None } - } - } -} From ac9636bf2941dc0da5ee32fc9c6c43503a107931 Mon Sep 17 00:00:00 2001 From: John Nunley Date: Tue, 4 Apr 2023 16:02:52 -0700 Subject: [PATCH 2/3] Add deprecated to Active et al --- src/borrowed.rs | 10 +++++++++- src/lib.rs | 1 + 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/borrowed.rs b/src/borrowed.rs index 752285d..5ebd4f3 100644 --- a/src/borrowed.rs +++ b/src/borrowed.rs @@ -26,8 +26,10 @@ use crate::{HasRawDisplayHandle, HasRawWindowHandle, RawDisplayHandle, RawWindow /// and immediately invalidate all window handles. However, it was later discovered that the /// handle actually remains valid, but the window does not produce any more GPU buffers. This /// type is a no-op and will be removed at the next major release. +#[deprecated = "Will be removed at next major release, use ActiveHandle::new() for now"] pub struct Active(()); +#[allow(deprecated)] impl fmt::Debug for Active { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.write_str("Active { .. }") @@ -51,6 +53,7 @@ impl<'a> fmt::Debug for ActiveHandle<'a> { } } +#[allow(deprecated)] impl Active { /// Create a new `Active` tracker. /// @@ -62,6 +65,7 @@ impl Active { /// use raw_window_handle::Active; /// let active = Active::new(); /// ``` + #[deprecated = "Will be removed at next major release, use ActiveHandle::new() for now"] pub const fn new() -> Self { Self(()) } @@ -86,6 +90,7 @@ impl Active { /// drop(handle); /// active.set_inactive(); /// ``` + #[deprecated = "Will be removed at next major release, use ActiveHandle::new() for now"] pub fn handle(&self) -> Option> { Some(ActiveHandle(PhantomData)) } @@ -106,6 +111,7 @@ impl Active { /// // Set the application to be inactive. /// active.set_inactive(); /// ``` + #[deprecated = "Will be removed at next major release, use ActiveHandle::new() for now"] pub fn set_inactive(&self) {} /// Set the application to be active. @@ -127,6 +133,7 @@ impl Active { /// // Set the application to be inactive. /// active.set_inactive(); /// ``` + #[deprecated = "Will be removed at next major release, use ActiveHandle::new() for now"] pub unsafe fn set_active(&self) {} } @@ -151,6 +158,7 @@ impl ActiveHandle<'_> { /// // SAFETY: The application must actually be active. /// let handle = unsafe { ActiveHandle::new_unchecked() }; /// ``` + #[deprecated = "Will be removed at next major release, use ActiveHandle::new() for now"] pub unsafe fn new_unchecked() -> Self { Self(PhantomData) } @@ -165,7 +173,7 @@ impl ActiveHandle<'_> { /// use raw_window_handle::ActiveHandle; /// let handle = ActiveHandle::new(); /// ``` - #[allow(clippy::new_without_default)] + #[allow(clippy::new_without_default, deprecated)] pub fn new() -> Self { // SAFETY: The handle is always active. unsafe { super::ActiveHandle::new_unchecked() } diff --git a/src/lib.rs b/src/lib.rs index 2088271..9fdba61 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -46,6 +46,7 @@ mod windows; pub use android::{AndroidDisplayHandle, AndroidNdkWindowHandle}; pub use appkit::{AppKitDisplayHandle, AppKitWindowHandle}; #[cfg(any(feature = "std", not(target_os = "android")))] +#[allow(deprecated)] pub use borrowed::{ Active, ActiveHandle, DisplayHandle, HandleError, HasDisplayHandle, HasWindowHandle, WindowHandle, From 141902cc5658f07220e9d9a0f2af5665131cdf37 Mon Sep 17 00:00:00 2001 From: jtnunley Date: Mon, 10 Apr 2023 09:08:30 -0700 Subject: [PATCH 3/3] Fix contradictory documentation --- src/borrowed.rs | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/src/borrowed.rs b/src/borrowed.rs index 5ebd4f3..63c01ab 100644 --- a/src/borrowed.rs +++ b/src/borrowed.rs @@ -11,17 +11,6 @@ use crate::{HasRawDisplayHandle, HasRawWindowHandle, RawDisplayHandle, RawWindow /// Keeps track of whether the application is currently active. /// -/// On certain platforms (e.g. Android), it is possible for the application to enter a "suspended" -/// state. While in this state, all previously valid window handles become invalid. Therefore, in -/// order for window handles to be valid, the application must be active. -/// -/// On platforms where the graphical user interface is always active, this type is a ZST and all -/// of its methods are noops. On Android, this type acts as a reference counter that keeps track -/// of all currently active window handles. Before the application enters the suspended state, it -/// blocks until all of the currently active window handles are dropped. -/// -/// ## Explanation -/// /// On Android, it was previously believed that the application could enter the suspended state /// and immediately invalidate all window handles. However, it was later discovered that the /// handle actually remains valid, but the window does not produce any more GPU buffers. This @@ -38,12 +27,10 @@ impl fmt::Debug for Active { /// Represents a live window handle. /// -/// This is carried around by the [`Active`] type, and is used to ensure that the application doesn't -/// enter the suspended state while there are still live window handles. See documentation on the -/// [`Active`] type for more information. -/// -/// On non-Android platforms, this is a ZST. On Android, this is a reference counted handle that -/// keeps the application active while it is alive. +/// On Android, it was previously believed that the application could enter the suspended state +/// and immediately invalidate all window handles. However, it was later discovered that the +/// handle actually remains valid, but the window does not produce any more GPU buffers. This +/// type is a no-op and will be removed at the next major release. #[derive(Clone)] pub struct ActiveHandle<'a>(PhantomData<&'a UnsafeCell<()>>); @@ -65,7 +52,6 @@ impl Active { /// use raw_window_handle::Active; /// let active = Active::new(); /// ``` - #[deprecated = "Will be removed at next major release, use ActiveHandle::new() for now"] pub const fn new() -> Self { Self(()) } @@ -90,7 +76,6 @@ impl Active { /// drop(handle); /// active.set_inactive(); /// ``` - #[deprecated = "Will be removed at next major release, use ActiveHandle::new() for now"] pub fn handle(&self) -> Option> { Some(ActiveHandle(PhantomData)) } @@ -111,7 +96,6 @@ impl Active { /// // Set the application to be inactive. /// active.set_inactive(); /// ``` - #[deprecated = "Will be removed at next major release, use ActiveHandle::new() for now"] pub fn set_inactive(&self) {} /// Set the application to be active. @@ -133,7 +117,6 @@ impl Active { /// // Set the application to be inactive. /// active.set_inactive(); /// ``` - #[deprecated = "Will be removed at next major release, use ActiveHandle::new() for now"] pub unsafe fn set_active(&self) {} }