feat: Add a safe method for cross-crate interop to winit by notgull · Pull Request #2744 · rust-windowing/winit

notgull · 2023-03-17T18:35:16Z

Tested on all platforms changed (quite a few platforms here)
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

One of the main complaints with the current Rust GUI ecosystem is that interoperability requires unsafe code. As of now, the raw-window-handle crate is used for interoperability; however, it has two disadvantages:

There is no lifetime parameter attached to the handle, meaning that the window can be closed at any time.
The display handle and all of its windows can become invalid at any time on Android due to the app being suspended.

raw-window-handle probably isn't at the appropriate level of abstraction to be fixing this anyways. However, this has led to some friction in crates that depend on it (gfx-rs/wgpu#1463 and rust-windowing/softbuffer#80 off the top of my head). I created a window-handle crate in an attempt to solve this. The new features are:

A lifetime parameter is attached to the handles to prevent them from outliving their sources.
An Active struct is used to access the window handles. Active can only exist when the application isn't suspended.

This PR adds the current implementation of window-handle to this crate and implements its traits. This PR also serves as a request for comments on the window-handle crate. I'm not familiar with all of the platforms, so it's likely that there's some invariant that I've missed.

kchibisov · 2023-03-17T18:41:54Z

cc @Lokathor

Lokathor · 2023-03-17T19:14:32Z

I think that the new crate should also have the Zlib license as a license option the same as the raw-window-handle crate does, but otherwise it seems like an entirely appropriate "one level up" abstraction

notgull · 2023-03-17T19:23:44Z

I think that the new crate should also have the Zlib license as a license option the same as the raw-window-handle crate does,

Sounds good to me, done in notgull/window-handle@64e1dfa!

kchibisov · 2023-03-17T19:24:57Z

I wonder whether raw-window-handle itself should provide such abstraction in addition, simply because it's also a no_std single rwhd dep crate? So we'll have the same crate for such handles which we could stabilize and use?

Managing 2 separate crates for handles could be not that easy, because you'd need to update both at the same time if raw-window-handle break something(like we want to add Result, when you can't build the handle).

notgull · 2023-03-17T19:27:32Z

I wonder whether raw-window-handle itself should provide such abstraction in addition, simply because it's also a no_std single rwhd dep crate? So we'll have the same crate for such handles which we could stabilize and use?

Managing 2 separate crates for handles could be not that easy, because you'd need to update both at the same time if raw-window-handle break something(like we want to add Result, when you can't build the handle).

I would support this, I can file a PR if you want @Lokathor

Lokathor · 2023-03-17T19:56:32Z

I'd be entirely fine with adding this right into the existing crate.

notgull · 2023-03-17T20:16:01Z

Updated for rust-windowing/raw-window-handle#110

notgull · 2023-03-17T21:02:46Z

CI failures are due to having two versions of raw-window-handle in the dependency tree. Should be fixed once the patch is released.

Lokathor · 2023-03-17T21:29:32Z

i can do a 0.5.1 later tonight

notgull · 2023-03-17T21:30:54Z

i can do a 0.5.1 later tonight

That'd be great. That being said, I'd like to make sure that we aren't too hasty to commit to any design decisions, and give some time to let people respond to rust-windowing/raw-window-handle#111

Lokathor · 2023-03-17T21:45:46Z

Alright, good point.

Since it's just one CI pass out of many, and since this is just a draft, we'll wait for more feedback.

notgull · 2023-03-24T12:32:07Z

Ported to rust-windowing/raw-window-handle#116

Implements HasDisplayHandle so the handle can be used easily

notgull · 2023-03-31T21:25:53Z

raw-window-handle version 0.5.2 has been published, so we can use that now. I'd say this PR is ready for review now.

notgull · 2023-03-31T21:38:51Z

Apple CI is failing because of madsmtm/objc2#432, and the other CI is failing because of syn 2.0.

kchibisov · 2023-04-01T16:21:53Z

src/event_loop.rs

+/// querying the display's resolution or DPI, without having to create a window. It
+/// implements `HasDisplayHandle`.
+#[derive(Clone)]
+pub struct OwnedDisplayHandle {


I don't really understand the purpose of this type. The EventLoop itself is what should have the HasDisplayHandle impl, and its lifetime is what should be used. The fact that you're not using this type in examples at all suggests me that there's no clear case when it should be used(at least you should use it inside the examples).

The EventLoopWindowTarget must have the same lifetime as the EventLoop itself, because it's simply a field on EventLoop in all the backends.

The Clone is also sort of questionable, because it simply will discard the lifetime attached to it, given that there's only a way to get &OwnedDisplayHandle not the owned type on its own?

if that's intent on lifetime casting, different method should be used with unsafe bit on it(because it's simply mem::tranmsute like thingy of donig 'static lifetime).

The reason why this type exists is for the glutin case. The scenario is that you need an object that has a display handle before a window is created, since you need to query the display. You can't use EventLoop, &EventLoop or a DisplayHandle<'_> taken from an EventLoop because the EventLoop is owned/borrowed mutably for event handling. You can't use the EventLoopWindowTarget that is provided in the event handler, since it disappears between events, which means it can't be used persistently. Normally I'd use a Window, but for glutin (outside of Win32) you haven't created a Window yet.

Therefore I created this type to fill that hole. Something that implements HasDisplayHandle that can be used according to Rust's borrowing system. This way, it can be used in the display position without any unsafe hacks.

~~If the Clone is a deal breaker I can get rid of it.~~ actually, it might be necessary for the borrowing rules to check out in this case.

actually, it might be necessary for the borrowing rules to check out in this case.

I just think that it's unsafe, so you might have different method like to_static, which casts away lifetime.

I have added documentation clarifying the purpose of OwnedWindowHandle and why it is safe.

kchibisov · 2023-04-01T21:53:44Z

src/event_loop.rs

+/// This type allows one to take advantage of the display's capabilities, such as
+/// querying the display's resolution or DPI, without having to create a window. It
+/// implements [`HasDisplayHandle`].
+///


I think you can't query anything from it.

I removed the part about querying. "Capabilities" should be enough to describe what it can be used for.

src/event_loop.rs

kchibisov · 2023-04-01T22:01:54Z

src/platform_impl/linux/x11/mod.rs


+#[derive(Clone)]
+pub struct OwnedDisplayHandle {
+    xconn: Arc<XConnection>,


you can use Rc for it given that it's neither send nor sync.

The object that's being passed around uses an Arc because it's being stored in a static variable.

winit/src/platform_impl/linux/mod.rs

Lines 118 to 120 in fbea75d

#[cfg(x11_platform)]

pub(crate) static X11_BACKEND: Lazy<Mutex<Result<Arc<XConnection>, XNotSupported>>> =

Lazy::new(|| Mutex::new(XConnection::new(Some(x_error_callback)).map(Arc::new)));

If we want to transition away from this model, it'd probably need to take place outside of this PR.

Hm, that's fair.

src/platform_impl/linux/wayland/event_loop/mod.rs

examples/borrowed_window.rs

rib · 2023-04-02T14:29:18Z

Hey, sorry to be late catching up with these changes but I was away for a week and missed the RFC poke on rust-windowing/raw-window-handle#111

I'm a little worried that we may have made quite a significant design change that perhaps wasn't required for Android, while I don't currently know that there was a safety/soundness issue.

Following the bread crumbs I wrote some initial thoughts here: rust-windowing/raw-window-handle#111 (comment)

a quick comment on rust-windowing/raw-window-handle#110 (comment)

and a comment on rust-windowing/raw-window-handle#116

Maybe there are multiple reasons for the changes, and i'm just looking at this from the pov of the Android backend, but still figured I should follow up

notgull · 2023-04-02T16:06:11Z

Hey, sorry to be late catching up with these changes but I was away for a week and missed the RFC poke on rust-windowing/raw-window-handle#111

No worries! We're still in the early days of this feature.

I responded to your posts. The overarching theme is that the docs seem to indicate that you cannot access the native window handle after it's been dropped.

You MUST ensure that you do not touch the window object after returning from this function

src/event_loop.rs

notgull · 2023-07-09T22:14:23Z

Superseded by #2943

feat: Integrate window-handle into winit

59a4144

notgull changed the title ~~feat: Integrate window-handle into winit~~ feat: Add a safe method for cross-crate interop to winit Mar 17, 2023

notgull mentioned this pull request Mar 17, 2023

feat: Add safer borrowed handles rust-windowing/raw-window-handle#110

Merged

Moved traits to raw-window-handle

c3616e1

notgull mentioned this pull request Mar 20, 2023

feat: Use the safer API for window handles rust-windowing/softbuffer#82

Closed

notgull added 2 commits March 24, 2023 05:03

Try the new branch

a735cd9

Use pathces in Cargo.toml

1db45e2

notgull added 3 commits March 24, 2023 10:29

Fix errors

2e9b655

Add an OwnedDisplayHandle structure

902a8ca

Implements HasDisplayHandle so the handle can be used easily

Use crates.io rwh

6bdb342

notgull marked this pull request as ready for review March 31, 2023 21:25

notgull requested review from jackpot51, kchibisov, madsmtm and msiglreith as code owners March 31, 2023 21:25

Fix handler on Android

61e4180

kchibisov requested changes Apr 1, 2023

View reviewed changes

Add more documentation to OwnedDisplayHandle

1fcc441

notgull force-pushed the window-handle branch from 16c4162 to 1fcc441 Compare April 1, 2023 18:29

kchibisov requested changes Apr 1, 2023

View reviewed changes

notgull added 2 commits April 1, 2023 15:24

Review comments

506f08c

Fix import error

0767b89

notgull mentioned this pull request Apr 4, 2023

Remove Android-specific platform differences rust-windowing/raw-window-handle#118

Merged

madsmtm reviewed Apr 14, 2023

View reviewed changes

src/event_loop.rs Show resolved Hide resolved

notgull marked this pull request as draft April 18, 2023 16:08

notgull closed this Jul 9, 2023

notgull deleted the window-handle branch July 9, 2023 22:14

MarijnS95 mentioned this pull request Jul 1, 2024

Possible solution to keep created surface consistent #3695

Open

	#[cfg(x11_platform)]
	pub(crate) static X11_BACKEND: Lazy<Mutex<Result<Arc<XConnection>, XNotSupported>>> =
	Lazy::new(\|\| Mutex::new(XConnection::new(Some(x_error_callback)).map(Arc::new)));

Conversation

notgull commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kchibisov commented Mar 17, 2023

Uh oh!

Lokathor commented Mar 17, 2023

Uh oh!

notgull commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kchibisov commented Mar 17, 2023

Uh oh!

notgull commented Mar 17, 2023

Uh oh!

Lokathor commented Mar 17, 2023

Uh oh!

notgull commented Mar 17, 2023

Uh oh!

notgull commented Mar 17, 2023

Uh oh!

Lokathor commented Mar 17, 2023

Uh oh!

notgull commented Mar 17, 2023

Uh oh!

Lokathor commented Mar 17, 2023

Uh oh!

notgull commented Mar 24, 2023

Uh oh!

notgull commented Mar 31, 2023

Uh oh!

notgull commented Mar 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notgull Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

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!

rib commented Apr 2, 2023

Uh oh!

notgull commented Apr 2, 2023

Uh oh!

Uh oh!

notgull commented Jul 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

notgull commented Mar 17, 2023 •

edited

Loading

notgull commented Mar 17, 2023 •

edited

Loading

notgull Apr 1, 2023 •

edited

Loading