Update to new raw-window-handle strategy by notgull · Pull Request #3126 · rust-windowing/winit

notgull · 2023-10-01T23:42:45Z

Tested on all platforms changed
Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
Created or updated an example program if it would help users understand this functionality
Updated feature matrix, if new features were added or implemented

Essentially a combination of #3075 and #2943. This PR adds explicit support for raw-window-handle v0.6 through the rwh_06 feature, which is enabled by default. It also adds optional support for raw-window-handle v0.5 and raw-window-handle v0.4, which is exposed through features that are not exposed by default.

This also adds a lifetime to WindowAttributes.

daxpedda

Web is LGTM.

madsmtm

Thanks for following up on this @notgull!

macOS and iOS impl approved, have opened some discussion on the API though that I would like to see resolved.

src/window.rs

madsmtm · 2023-10-02T14:04:26Z

src/window.rs

    /// Returns a [`raw_window_handle::RawDisplayHandle`] used by the [`EventLoop`] that
    /// created a window.
    ///
    /// [`EventLoop`]: crate::event_loop::EventLoop


Nit: This documentation is also relevant for v0.6

you mean, that this should also be added to the implementation of HasDisplayHandle for v0.6?

src/lib.rs

examples/child_window.rs

src/platform/windows.rs

src/platform_impl/ios/window.rs

src/platform_impl/macos/window.rs

kchibisov

Could you expect instead of doing unchecked? I'd rather not have more unsafe and have an actual assert that it got a Null for whatever reason.

It'll basically never happen, but the cost of checking null in a function you rarely call is non-existent.

Cargo.toml

examples/util/fill.rs

TornaxO7 · 2023-10-04T20:12:39Z

@notgull maybe not needed, but just in case, I fixed the CI build in this PR

TornaxO7 · 2023-10-06T10:17:41Z

Could you expect instead of doing unchecked? I'd rather not have more unsafe and have an actual assert that it got a Null for whatever reason.

It'll basically never happen, but the cost of checking null in a function you rarely call is non-existent.

may I ask which unchecked places you mean?
rg -trust "unchecked" returns no entries, if I'm seeing it correctly, related with the HasDisplayHandle and HasWindowHandle.

kchibisov · 2023-10-06T10:23:27Z

@TornaxO7 comments are getting resolved over time. This particular one was removed. See 9e15334

MarijnS95 · 2023-10-07T15:59:07Z

src/platform_impl/android/mod.rs

+        if let Some(native_window) = self.app.native_window().as_ref() {
+            let handle = rwh_06::AndroidNdkWindowHandle::new(native_window.ptr().cast());
+            Ok(rwh_06::RawWindowHandle::AndroidNdk(handle))
+        } else {


I'm implementing this in the NDK, as part of version bumps.

Done in rust-mobile/ndk#434.

May I ask what that means? Does anything has to do be done here or can this be marked as resolved?

This means that once I publish ndk 0.8.0, winit should upgrade to it (it's on -beta.0 now) and reuse the traits provided there.

Does this block this PR? I think that this should be merged in its current state and then we can upgrade to the latest ndk later.

It does not and I can patch this when submitting the final ndk bump inside winit.

MarijnS95 · 2023-10-07T16:10:03Z

src/platform_impl/android/mod.rs

+    }
+
+    #[cfg(feature = "rwh_06")]
+    pub fn raw_window_handle_rwh_06(&self) -> Result<rwh_06::RawWindowHandle, rwh_06::HandleError> {


Question while implementing the ndk portion for this: implementing the Has**RAW**WindowHandle trait is deprecated and there are blanket implementations when HasWindowHandle is implemented. Should this function also return a non-raw handle (in terms of that implementation when it is released)?

I considered this, but it would add an additional unsafe { WIndowHandle::borrow_raw } call for each backend. I could go either way.

Right. At least for Android I'm willing to have that unsafe { borrow_raw() } inside the ndk with appropriate // SAFETY docs, so you could call through to that. But it'd have to be consistent across all the backends...

Did you consider whether or not the safe borrow handles should be implemented by winit?

For the time being it leads to a lot more unsafe throughout the codebase. So I think that we shouldn't for now.

…0.5) Following [the same strategy in winit], move `raw-window-handle 0.5` behind a `rwh_05` feature and add `raw-window-handle 0.6` behind a corresponding `rwh_06` feature. Implement the new non-raw trait and **lifetimed** type for `0.6` (where `raw-window-handle` provides blanket implementations to coerce/convert back to `Raw*` types). [the same strategy in winit]: rust-windowing/winit#3126

…aintain 0.5) Following [the same strategy in winit], move `raw-window-handle 0.5` behind a `rwh_05` feature and add `raw-window-handle 0.4` and the newly released `raw-window-handle 0.6` behind a corresponding `rwh04`/`rwh_06` feature. The new **non-raw** trait and **lifetimed** type since `0.6` have been implemented instead, as `raw-window-handle` provides blanket implementations to coerce/convert back to `Raw*` types. [the same strategy in winit]: rust-windowing/winit#3126

…aintain 0.5) (#434) Following [the same strategy in winit], move `raw-window-handle 0.5` behind a `rwh_05` feature and add `raw-window-handle 0.4` and the newly released `raw-window-handle 0.6` behind a corresponding `rwh04`/`rwh_06` feature. The new **non-raw** trait and **lifetimed** type since `0.6` have been implemented instead, as `raw-window-handle` provides blanket implementations to coerce/convert back to `Raw*` types. [the same strategy in winit]: rust-windowing/winit#3126

Signed-off-by: John Nunley <[email protected]>

Co-Authored-By: TornaxO7 <[email protected]> Signed-off-by: John Nunley <[email protected]>

MarijnS95

Approving the android changes, but still have a question about borrowed handles.

Depends on #3126, rust-mobile/android-activity#128

This allows using accesskit with current versions of wgpu. Winit has a similar system to widden its support. See rust-windowing/winit#3126

notgull requested review from daxpedda, jackpot51, kchibisov, madsmtm and msiglreith as code owners October 1, 2023 23:42

daxpedda approved these changes Oct 2, 2023

View reviewed changes

madsmtm requested changes Oct 2, 2023

View reviewed changes

kchibisov requested changes Oct 2, 2023

View reviewed changes

Cargo.toml Show resolved Hide resolved

examples/util/fill.rs Outdated Show resolved Hide resolved

TornaxO7 mentioned this pull request Oct 3, 2023

Bump raw window handle to 0.6.0 gfx-rs/wgpu#4202

Merged

3 tasks

madsmtm approved these changes Oct 3, 2023

View reviewed changes

notgull force-pushed the notgull/rwh06 branch from 79c5961 to 101905c Compare October 4, 2023 23:54

MarijnS95 requested changes Oct 7, 2023

View reviewed changes

MarijnS95 reviewed Oct 7, 2023

View reviewed changes

MarijnS95 mentioned this pull request Oct 7, 2023

ndk/native_window: Implement HasWindowHandle for 0.4 and 0.6 (and maintain 0.5) rust-mobile/ndk#434

Merged

pengiie mentioned this pull request Oct 10, 2023

Bump raw-window-handle to 0.6.0 ash-rs/ash#795

Closed

madsmtm mentioned this pull request Oct 13, 2023

Make with_parent_window safe #3142

Draft

3 tasks

ids1024 mentioned this pull request Oct 13, 2023

Use raw-window-handle version 0.6 rust-windowing/softbuffer#132

Merged

notgull force-pushed the notgull/rwh06 branch from 101905c to bd80244 Compare October 14, 2023 03:35

notgull added 2 commits October 14, 2023 09:12

Make the compile errors go away on X11

5692a26

Signed-off-by: John Nunley <[email protected]>

Make the errors go away on macOS

ebab7cb

Signed-off-by: John Nunley <[email protected]>

notgull and others added 10 commits October 14, 2023 09:12

Make the compile errors go away on WIndows

4a89e32

Signed-off-by: John Nunley <[email protected]>

Make the compile errors go away on web

3013c3c

Signed-off-by: John Nunley <[email protected]>

Make the Android build errors go away

8509157

Signed-off-by: John Nunley <[email protected]>

Finish the iOS and Orbital backends

36439bd

Signed-off-by: John Nunley <[email protected]>

Fix CI errors

dee38df

Signed-off-by: John Nunley <[email protected]>

More fixes for CI errors

2cf5efa

Signed-off-by: John Nunley <[email protected]>

Fix X11 CI error

e2d1301

Signed-off-by: John Nunley <[email protected]>

Partially fix review comments

b20c0ce

Signed-off-by: John Nunley <[email protected]>

Remaining review comments

9a861b6

Signed-off-by: John Nunley <[email protected]>

Fix windows lifetimes

2933b0a

Co-Authored-By: TornaxO7 <[email protected]> Signed-off-by: John Nunley <[email protected]>

notgull force-pushed the notgull/rwh06 branch from bd80244 to 2933b0a Compare October 14, 2023 16:12

notgull mentioned this pull request Oct 14, 2023

Port to use raw-window-handle v0.6 #2943

Closed

5 tasks

notgull requested review from MarijnS95 and kchibisov October 14, 2023 17:39

MarijnS95 approved these changes Oct 14, 2023

View reviewed changes

MarijnS95 added a commit that referenced this pull request Oct 14, 2023

Upgrade ndk(-sys) to stable 0.8 / 0.5 releases

f5b4a5d

Depends on #3126, rust-mobile/android-activity#128

MarijnS95 mentioned this pull request Oct 14, 2023

Upgrade to ndk 0.8, ndk-sys 0.5 and android-activity 0.5 releases #3145

Merged

kchibisov approved these changes Oct 15, 2023

View reviewed changes

kchibisov merged commit e41fac8 into master Oct 15, 2023

kchibisov deleted the notgull/rwh06 branch October 15, 2023 02:07

ThierryBerger mentioned this pull request Oct 22, 2023

Update winit dependency to 0.29 bevyengine/bevy#8745

Closed

11 tasks

ThierryBerger mentioned this pull request Dec 13, 2023

fix: Use raw-window-handle 0.6 AccessKit/accesskit#310

Merged

nicopap added a commit to nicopap/accesskit that referenced this pull request Dec 14, 2023

Add a rwh_05 feature flag

6443c22

This allows using accesskit with current versions of wgpu. Winit has a similar system to widden its support. See rust-windowing/winit#3126

nicopap mentioned this pull request Dec 14, 2023

Add a rwh_05 feature flag AccessKit/accesskit#319

Merged

3 tasks

nicopap added a commit to nicopap/accesskit that referenced this pull request Dec 14, 2023

Add a rwh_05 feature flag

daa6c6c

This allows using accesskit with current versions of wgpu. Winit has a similar system to widden its support. See rust-windowing/winit#3126

Conversation

notgull commented Oct 1, 2023

Uh oh!

daxpedda left a comment

Choose a reason for hiding this comment

Uh oh!

madsmtm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kchibisov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TornaxO7 commented Oct 4, 2023

Uh oh!

TornaxO7 commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kchibisov commented Oct 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarijnS95 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

TornaxO7 commented Oct 6, 2023 •

edited

Loading