Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split out Surface from Window #3942

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

jgcodes2020
Copy link

@jgcodes2020 jgcodes2020 commented Oct 9, 2024

  • Tested on all platforms changed (note: passes CI, examples seem to work on Wayland)
  • Added an entry to the changelog module 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 (note: this won't really make sense until popups/subsurfaces are added)
  • Updated feature matrix, if new features were added or implemented (N/A)

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
Copy link
Member

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
Copy link
Member

kchibisov commented Oct 10, 2024

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
Copy link
Author

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:

  • I agree with WindowId becoming SurfaceId.
  • Pointer events can be received by any surface (window, popup, or subsurface), but keyboard events can only be received by windows.
  • Events aren't bubbled. Not every platform does this, and the platforms that do should be able to stop them from being bubbled.

@jgcodes2020
Copy link
Author

jgcodes2020 commented Oct 19, 2024

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.

@jgcodes2020 jgcodes2020 marked this pull request as ready for review October 19, 2024 00:09
@MarijnS95
Copy link
Member

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
Copy link
Author

jgcodes2020 commented Oct 19, 2024

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
Copy link
Author

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
Copy link
Member

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 jgcodes2020 changed the title [WIP] Split out Surface from Window Split out Surface from Window Oct 23, 2024
@jgcodes2020
Copy link
Author

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

@kchibisov
Copy link
Member

it requires nightly.

@jgcodes2020
Copy link
Author

This should be ready for review now.

@mahkoh
Copy link
Contributor

mahkoh commented Oct 30, 2024

keyboard events can only be received by windows.

That's not correct on wayland.

@kchibisov
Copy link
Member

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.

@madsmtm madsmtm added the C - nominated Nominated for discussion in the next meeting label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - nominated Nominated for discussion in the next meeting
Development

Successfully merging this pull request may close these issues.

5 participants