Skip to content

Commit

Permalink
Improve docs and naming for RawWindowHandle functionality (bevyengine…
Browse files Browse the repository at this point in the history
…#4335)

# Objective

- As noticed in bevyengine#4333 by @x-52, the exact purpose and logic of `HasRawWIndowHandleWrapper` is unclear
- Unfortunately, there are rather good reasons why this design is needed (and why we can't just `impl HasRawWindowHandle for RawWindowHandleWrapper` 

## Solution

- Rename `HasRawWindowHandleWrapper` to `ThreadLockedRawWindowHandleWrapper`, reflecting the primary distinction
- Document how this design is intended to be used
- Leave comments explaining why this design must exist


## Migration Guide

- renamed `HasRawWindowHandleWrapper` to `ThreadLockedRawWindowHandleWrapper`
  • Loading branch information
alice-i-cecile authored and james7132 committed Jun 7, 2022
1 parent 11749b3 commit ed63acd
Showing 1 changed file with 25 additions and 8 deletions.
33 changes: 25 additions & 8 deletions crates/bevy_window/src/raw_window_handle.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use raw_window_handle::{HasRawWindowHandle, RawWindowHandle};

/// This wrapper exist to enable safely passing a [`RawWindowHandle`] across threads. Extracting the handle
/// is still an unsafe operation, so the caller must still validate that using the raw handle is safe for a given context.
/// A wrapper over [`RawWindowHandle`] that allows us to safely pass it across threads.
///
/// Depending on the platform, the underlying pointer-containing handle cannot be used on all threads,
/// and so we cannot simply make it (or any type that has a safe operation to get a [`RawWindowHandle`])
/// thread-safe.
#[derive(Debug, Clone)]
pub struct RawWindowHandleWrapper(RawWindowHandle);

Expand All @@ -10,12 +13,14 @@ impl RawWindowHandleWrapper {
Self(handle)
}

/// Returns a [`HasRawWindowHandle`] impl, which exposes [`RawWindowHandle`].
///
/// # Safety
/// This returns a [`HasRawWindowHandle`] impl, which exposes [`RawWindowHandle`]. Some platforms
/// have constraints on where/how this handle can be used. For example, some platforms don't support doing window
///
/// Some platforms have constraints on where/how this handle can be used. For example, some platforms don't support doing window
/// operations off of the main thread. The caller must ensure the [`RawWindowHandle`] is only used in valid contexts.
pub unsafe fn get_handle(&self) -> HasRawWindowHandleWrapper {
HasRawWindowHandleWrapper(self.0)
pub unsafe fn get_handle(&self) -> ThreadLockedRawWindowHandleWrapper {
ThreadLockedRawWindowHandleWrapper(self.0)
}
}

Expand All @@ -27,10 +32,22 @@ impl RawWindowHandleWrapper {
unsafe impl Send for RawWindowHandleWrapper {}
unsafe impl Sync for RawWindowHandleWrapper {}

pub struct HasRawWindowHandleWrapper(RawWindowHandle);
/// A [`RawWindowHandleWrapper`] that cannot be sent across threads.
///
/// This safely exposes a [`RawWindowHandle`], but care must be taken to ensure that the construction itself is correct.
///
/// This can only be constructed via the [`RawWindowHandleWrapper::get_handle()`] method;
/// be sure to read the safety docs there about platform-specific limitations.
/// In many cases, this should only be constructed on the main thread.
pub struct ThreadLockedRawWindowHandleWrapper(RawWindowHandle);

// SAFE: the caller has validated that this is a valid context to get RawWindowHandle
unsafe impl HasRawWindowHandle for HasRawWindowHandleWrapper {
// as otherwise an instance of this type could not have been constructed
// NOTE: we cannot simply impl HasRawWindowHandle for RawWindowHandleWrapper,
// as the `raw_window_handle` method is safe. We cannot guarantee that all calls
// of this method are correct (as it may be off the main thread on an incompatible platform),
// and so exposing a safe method to get a `RawWindowHandle` directly would be UB.
unsafe impl HasRawWindowHandle for ThreadLockedRawWindowHandleWrapper {
fn raw_window_handle(&self) -> RawWindowHandle {
self.0
}
Expand Down

0 comments on commit ed63acd

Please sign in to comment.