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

Child windows on Wayland #3487

Closed

Conversation

jgcodes2020
Copy link

@jgcodes2020 jgcodes2020 commented Feb 12, 2024

  • 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

This PR will add child windows on Wayland via the wl_subsurface role. Below are design issues I'd like to work out before starting work.

  • Unlike child windows on X11 and Windows, wl_subsurfaces aren't clipped to their parent. Do we want to implement clipping client-side via wp_viewport?

@kchibisov
Copy link
Member

I'd just note that I was planning to do child windows myself in or after 0.31.0 since they are a bit complex to get right cross platform. The present winit is not really suited for them and they are a bit hacked up in other backends. Probably the same reason you don't see them on macOS. popups are also not suited with the architecture we have right now.

Clipping with viewporter doesn't sound like a really good idea, Though, I understand where it's coming from. I also believe that you can control clipping on other platforms, but it's a bit special.

jgcodes2020 and others added 10 commits February 15, 2024 14:51
Mainly fix typos in comments, but also some minor code changes:

* Rename `apply_on_poiner` to `apply_on_pointer`.
* Rename `ImeState::Commited` to `ImeState::Committed`
* Correct `cfg_attr` usage: `wayland_platfrom` -> `wayland_platform`.
@jgcodes2020
Copy link
Author

jgcodes2020 commented Feb 19, 2024

Seems to be working for the most part, but subsurfaces don't seem to pick up seat events like they're supposed to. (unless I'm missing something)

EDIT: Apparently Wayland is sending the events, but they aren't picked up by winit

@kchibisov
Copy link
Member

I'd just not that I don't have real interest to look into subsurfaces especially how messy they get due to a fact that making subsurface a regular window is a design PITA, the same goes for other child windows that were hacked up.

In general, I can do all of that myself with a proper API in 0.31.0, because making people rely and design around broken API is not what I want.

Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work.

But as I said multiple times the way you try to implement it into what we have now is just an unmaintainable mess, which is not your fault, but rather the toplevel API design, since regular window and subview is treated the same, while in reality it's not the case.

The set of APIs is completely different, the semantics are also different, and the fact that subview never has focus, but has pointer focus is also not really expressed. The relationships are not really clear, the lifetime between child and parent are not directly exposed leading to unsound code, so I'd rather not users rely on things like that and do proper API for all platforms with 0.31.0 release.

If you want to continue working on child windows, I'd suggest to look into creating common cross platform API detached from the window itself, and how parent/child should be communicated through out winit events, since it's basically undefined as it is now.

@daxpedda daxpedda removed their request for review February 20, 2024 08:40
@jgcodes2020
Copy link
Author

@kchibisov

Thanks for your work.

But as I said multiple times the way you try to implement it into what we have now is just an unmaintainable mess, which is not your fault, but rather the toplevel API design, since regular window and subview is treated the same, while in reality it's not the case.

The set of APIs is completely different, the semantics are also different, and the fact that subview never has focus, but has pointer focus is also not really expressed. The relationships are not really clear, the lifetime between child and parent are not directly exposed leading to unsound code, so I'd rather not users rely on things like that and do proper API for all platforms with 0.31.0 release.

If you want to continue working on child windows, I'd suggest to look into creating common cross platform API detached from the window itself, and how parent/child should be communicated through out winit events, since it's basically undefined as it is now.

Looking at #3433 and where parts of this PR fit in, I'd need to wait until this step:

Make traits in winit-core, that implements the desired functionality, and move each backend to also implement those traits.

This is where we'd write a ChildWindow trait of some sort.

@kchibisov
Copy link
Member

This is where we'd write a ChildWindow trait of some sort.

Can follow this #3506 . The X11/Windows backends tricked child windows to be what they shouldn't be, because it was more about toplevel parent, but it end up with subsurface like API.

And setting toplevel as a parent is not supported in winit, but I think you can do so in the wayland protocol sense.

@declantsien
Copy link

try to implement it into what we have now is just an unmaintainable mess, which is not your fault, but rather the toplevel API design, since regular window and subview is treated the same, while in reality it's not the case.

Emacs has a well tested child window design and well documented.

@jgcodes2020
Copy link
Author

I'll close this PR and open an issue to discuss how we can support child windows and popups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants