rust-windowing / winit

Window handling library in pure Rust
https://docs.rs/winit/
Apache License 2.0
4.88k stars 913 forks source link

Split out `Surface` from `Window` #3942

Open jgcodes2020 opened 1 month ago

jgcodes2020 commented 1 month ago

This PR splits some methods of Window into a new supertrait Surface, laying groundwork for the implementation of #3928.

As stated there, this split is needed because popups and subsurfaces may not necessarily allow the same operations as windows.

kchibisov commented 1 month ago

Still wonder whether to use View/Subview for that instead of Surface/Subsurface, since the Subsurface doesn't make much sense for people not familiar with Wayland.

Also not sure about the set_cursor API and it's not clear how Ids should generally work here. Like our WindowId should become some kind of Surface/ViewId and then when it comes to events surface/window/desktop is not well established throughout the API, so I'd probably hold a bit on that.

kchibisov commented 1 month ago

I'd also add a as_window(&self) -> Option<&dyn Window> on Surface trait to help with upcasting when passing just Surface around and raw-window-handle should be for Surface.

jgcodes2020 commented 1 month ago

Still wonder whether to use View/Subview for that instead of Surface/Subsurface, since the Subsurface doesn't make much sense for people not familiar with Wayland.

I'll argue that Surface is used by graphics APIs and Wayland. I don't want this to devolve into bikeshedding, so I'll leave it as Surface unless the maintainers collectively agree that View is the better naming convention.

Also not sure about the set_cursor API

The set_cursor API is copied verbatim from the original window trait. It should be possible to have cursors per NSView; if not, that can be emulated by changing the cursor on PointerMoved events.

and it's not clear how Ids should generally work here. Like our WindowId should become some kind of Surface/ViewId and then when it comes to events surface/window/desktop is not well established throughout the API, so I'd probably hold a bit on that.

I already addressed event handling in my proposal. Summary:

jgcodes2020 commented 1 month ago

With the Windows commit, that's every platform I can test on my machine at the moment. I don't have a Mac nor do I have Android development set up at the moment.

MarijnS95 commented 1 month ago

Now that I'm assigned for review: I'd like this PR description and the resulting squashed commit to contain a description and justification of the respective changes, rather than merely mentioning that it addresses some part of a large discussion thread in an issue.

That should make it easier to review and backtrack.

jgcodes2020 commented 1 month ago

Now that I'm assigned for review: I'd like this PR description and the resulting squashed commit to contain a description and justification of the respective changes, rather than merely mentioning that it addresses some part of a large discussion thread in an issue.

That should make it easier to review and backtrack.

I've updated the PR description.

jgcodes2020 commented 1 month ago

Note to maintainers: I'm able to test Linux (X11 and Wayland) and Windows personally, but not any of the other platforms. As of now, I'm currently relying on CI to ensure the other platforms work (which given we don't have integration tests, I can't confirm they work). What should I do?

kchibisov commented 1 month ago

Given that PR should simply move code around it's unilkely to break things, unless you decide to do something fancy, though, PRs like that require approval from maintainers on the platforms you change stuff, so don't worry about it.

jgcodes2020 commented 1 month ago

The implementation should be complete. Not sure why the formatting CI run failed, since I ran cargo fmt on my end already.

kchibisov commented 1 month ago

it requires nightly.

jgcodes2020 commented 1 month ago

This should be ready for review now.

mahkoh commented 1 month ago

keyboard events can only be received by windows.

That's not correct on wayland.

kchibisov commented 1 month ago

That's not correct on wayland.

that's true, though not in the context of what winit has. Also, you can safely bubble them to ensure that all the platforms are consistent here.