Skip to content

Broadcast window attributes#2926

Open
casperstorm wants to merge 5 commits intorust-windowing:masterfrom
casperstorm:feat/brodcast-window-attr
Open

Broadcast window attributes#2926
casperstorm wants to merge 5 commits intorust-windowing:masterfrom
casperstorm:feat/brodcast-window-attr

Conversation

@casperstorm
Copy link
Copy Markdown
Contributor

Fixes #2334.

This PR adds a new WindowEvent::WindowAttribute which tells if the window is in fullscreen, minimize or normal mode.
Currently, macOS and Windows has been implemented, since that's the two platforms I am able to develop on.

  • 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

@casperstorm casperstorm force-pushed the feat/brodcast-window-attr branch from a1feb22 to 1d4983e Compare July 7, 2023 11:16
@casperstorm casperstorm changed the title Brodcast window attributes Broadcast window attributes Jul 7, 2023
Copy link
Copy Markdown
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.

In general, such information should be a part of the Resized event, since it's a part of the state change and resize will be generated as well.

I'm not sure if we should expose minimize for that, maybe we should, idk.

Also, could we maybe use something like that https://wayland.app/protocols/xdg-shell#xdg_toplevel:enum:state ? Or at least map some of them. Are tiled states exposed on Windows for example? What about macOS and how it all maps to it?

The new Resized event should be called Configure I guess to follow the Wayland naming since it sorts of makes sense and have a size just as a part of it's field and other field is the bitflags with the state.

We may have minimized there as well.

@casperstorm
Copy link
Copy Markdown
Contributor Author

In general, such information should be a part of the Resized event, since it's a part of the state change and resize will be generated as well.

I'm not sure if we should expose minimize for that, maybe we should, idk.

Also, could we maybe use something like that https://wayland.app/protocols/xdg-shell#xdg_toplevel:enum:state ? Or at least map some of them. Are tiled states exposed on Windows for example? What about macOS and how it all maps to it?

The new Resized event should be called Configure I guess to follow the Wayland naming since it sorts of makes sense and have a size just as a part of it's field and other field is the bitflags with the state.

We may have minimized there as well.

We can rename it Attribute to Configuration and include Resized(size) and then do:

pub enum Configuration {
    /// The window is in normal mode
    Normal,
    /// The window is in fullscreen mode
    Fullscreen,
    /// The window is minimized
    Minimized,
    /// The size of the window was resized. Contains the client area's new dimensions.
    Resized(PhysicalSize<u32>),
}

As for tiling; to my knowledge there's no easy way to get tiling information. On macOS if you tile left or right, the full-screen selector is triggered.

@kchibisov
Copy link
Copy Markdown
Member

It should be

pub struct Configure {
    size: PhysicalSize<u32>,
    state: WindowState, // bitflags, TILED_LEFT, RIGHT, etc, | MAXIMIZED | FULLSCREEN | MINIMIZED
}

I could send this API change myself and you could open PR against my branch then.

@casperstorm
Copy link
Copy Markdown
Contributor Author

casperstorm commented Jul 7, 2023

It should be

pub struct Configure {
    size: PhysicalSize<u32>,
    state: WindowState, // bitflags, TILED_LEFT, RIGHT, etc, | MAXIMIZED | FULLSCREEN | MINIMIZED
}

I could send this API change myself and you could open PR against my branch then.

Sure, if thats how you want it you can open a PR and and I'll follow up! 👍🏻

@kchibisov
Copy link
Copy Markdown
Member

Could you send your patches against #2929 ?

@casperstorm
Copy link
Copy Markdown
Contributor Author

Could you send your patches against #2929 ?

Yes will look at this when back from vacation in a couple of days.

@casperstorm casperstorm force-pushed the feat/brodcast-window-attr branch from 1d4983e to 0136429 Compare July 9, 2023 18:25
@casperstorm casperstorm force-pushed the feat/brodcast-window-attr branch from 0136429 to 5ac9357 Compare July 9, 2023 18:26
Copy link
Copy Markdown
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.

The maximized is missing on both macOS and Windows

Comment on lines +384 to +392
if let Some(screen) = self.window.screen() {
let rect = screen.frame();
let logical_size =
LogicalSize::new(rect.size.width as f64, rect.size.height as f64);
let size = logical_size.to_physical::<u32>(self.window.scale_factor());

self.queue_event(WindowEvent::Configured {
size,
state: Default::default(),
});
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In general, won't it all trigger the frame changes or other events? Can't we simply get the all the stiles of zoomed, fullscreen inside the resize handle or macOS won't resize to the same sizes? Do we have, maybe, some state changes hooks?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We should use the delegate methods now they are there, imo. That's cleanest imo.

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 <mads@marquart.dk>
Co-authored-by: Mads Marquart <mads@marquart.dk>
@casperstorm casperstorm force-pushed the feat/brodcast-window-attr branch from f05d266 to 80f80fc Compare July 11, 2023 19:31
@casperstorm casperstorm force-pushed the feat/brodcast-window-attr branch from 80f80fc to 0fc3605 Compare July 11, 2023 19:34
@kchibisov kchibisov added C - waiting on author Waiting for a response or another PR C - waiting on maintainer A maintainer must review this code and removed C - waiting on author Waiting for a response or another PR labels Aug 3, 2023
@casperstorm casperstorm requested a review from notgull 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 maintainer A maintainer must review this code

Development

Successfully merging this pull request may close these issues.

Broadcast window mode changes

3 participants