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

Rename Resized to Configured and expose state #2929

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

Conversation

kchibisov
Copy link
Member

Notify clients about the window state changes, most underlying systems do change the window states.

Fixes #2334.

  • 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

@kchibisov
Copy link
Member Author

@daxpedda for web, I'm not sure how much of that works.

@notgull should we maybe listen to property changes (the _NEW_WM_STATE) instead?

@kchibisov
Copy link
Member Author

I'm not quite sure how to approach other states, like wayland has for example Resizing state indicating live resizes.

Should we really abuse the Resized here and change it to carry the state, maybe we should expose state as a separate event, though, the issue is that such state is usually tied with resize events, for example, fullscreen/maximized do change the window state.

@notgull
Copy link
Member

notgull commented Jul 8, 2023

@notgull should we maybe listen to property changes (the _NEW_WM_STATE) instead?

For the atom changes we need to listen to PropertyNotify, IIRC.

@kchibisov
Copy link
Member Author

For the atom changes we need to listen to PropertyNotify, IIRC.

Yeah, I've read it, but I'm not sure whether we really need to, due to a fact that ConfigureNotify is talking about some state/scene changes and both fullscreen/maximize should involve that.

The question is whether the Minimized is also signaled there or not.

src/event.rs Outdated Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved

bitflags::bitflags! {
#[derive(Default, Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct WindowState: u32 {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think a bit flag is appropriate here, since the states are mutually exclusive, are they not?

Perhaps an enum like this is better (names up for bikeshedding):

enum WindowFrameState {
    Fullscreen, // Possibly containing `Fullscreen`?
    Maximized,
    Minimized,
    Normal,
}

And given that we now have this enum, perhaps we can unify it with setting the window (frame) state? So e.g. window set_frame_state(WindowFrameState::Maximized) instead of window.set_maximized(true)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The state might not be mutually exclusive in the future, at least If I expose more stuff from Wayland it won't be.

I don't think we should change the functions, because fullscreen is attached to the particular monitor. At least for now I'd keep it the way it is.

Copy link
Member

Choose a reason for hiding this comment

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

The state might not be mutually exclusive in the future, at least If I expose more stuff from Wayland it won't be.

Maybe we should figure this out when we get there, even on Wayland the current states are mutually exclusive, so maybe additional states for Wayland should be dealt with in a different way while allowing states that are mutually exclusive to stay that way.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are tiled states, saying that some edge of the window is being tiled, it's basically the same as maximized.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know what tiled is, but what does "basically the same" mean? Is it mutually exclusive or not?
At least on other backends it is mutually exclusive, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are not, you can have them attached to any state, you can have 4 of them at the same time, etc.

Tiled means that your window is, well, tiled to some screen edge or just generally tiled.

Copy link
Member

Choose a reason for hiding this comment

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

Wow, that's really strange.
Wouldn't this completely change the meaning of what's exposed here? Like it doesn't make the API really cross-platform because the meaning is so different.

In any case, if it's not mutually exclusive, we can leave it as is.

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Web is implemented.

I noticed that the fullscreen API we use has some compatibility issues with Safari, even before this PR. I will open an issue and address this afterwards.

EDIT: #2935.

@daxpedda
Copy link
Member

daxpedda commented Jul 8, 2023

Rebased (and added some fixes to Web).

@casperstorm
Copy link
Contributor

Rebased #2926 on top of this.

kchibisov and others added 4 commits July 11, 2023 00:36
Notify clients about the window state changes, most underlying systems
do change the window states.

Fixes rust-windowing#2334.
Co-authored-by: Mads Marquart <[email protected]>
Co-authored-by: Mads Marquart <[email protected]>
@daxpedda
Copy link
Member

Rebased on a bunch of churn in Web.

@kchibisov kchibisov requested a review from MarijnS95 as a code owner February 27, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - waiting on author Waiting for a response or another PR
Development

Successfully merging this pull request may close these issues.

Broadcast window mode changes
5 participants