[Merged by Bors] - Add option to center a window #4999

LoipesMas · 2022-06-12T22:03:01Z

Objective

Fixes Export the needed parts of Winit or add better utilities for centering a window. #4993

Solution

~~Add centered property to WindowDescriptor~~
Add WindowPosition enum
WindowDescriptor.position is now WindowPosition instead of Option<Vec2>
Add center_window function to Window

Migration Guide

If using WindowDescriptor, replace position: None with position: WindowPosition::Default and position: Some(vec2) with WindowPosition::At(vec2).

I'm not sure if this is the best approach, so feel free to give any feedback.
Also I'm not sure how Options should be handled in bevy_winit/src/lib.rs:161.

Also, on window creation we can't (or at least I couldn't) get outer_size, so this doesn't include decorations in calculations.

mockersf

I would prefer position to become an enum, something like

enum WindowPosition {
    Default,    // equivalent to position = None
    Centered,   // equivalent to centered = true
    At(Vec2),   // equivalent to position = Some(vec2)
}

CalinZBaenen · 2022-06-13T07:26:03Z

@mockersf I really like that idea. I also like the naming of At.
If this does become a thing, I hope Centered will include the borders in calculation, that would be really cool.

LoipesMas · 2022-06-13T09:38:17Z

@mockersf I considered something like this, but wasn't too convinced, because this would make it a breaking change. But it makes sense, so I'll update the PR.

LoipesMas · 2022-06-13T09:46:13Z

@CalinZBaenen That would be nice, but it would require deferring the centering to after the window is initialized and I'm not familiar enough with bevy+winit to do it (I don't know if we have any foundations for that or if that even is possible). You can use startup system as an bandaid (and this should include borders), but then it takes a frame for it to take effect:

fn setup_window(mut windows: ResMut<Windows>) {
    if let Some(window) = windows.get_primary_mut() {
        window.center_window();
    } 
}

If you do the WindowDescriptor centering and this startup system, then the adjustment will be minimal (i.e. size of the decorations) so it would probably look decent enough.

crates/bevy_window/src/window.rs

CalinZBaenen · 2022-06-13T18:19:18Z

@LoipesMas I believe WindowCommands::Center is title case, but when viewing window.rs I see an instance of WindowCommands::center in lowercase.

LoipesMas · 2022-06-13T19:49:56Z

@CalinZBaenen sorry, I don't know what you're referring to. Could you give me a precise location?

CalinZBaenen · 2022-06-13T19:58:39Z

@LoipesMas It seems to be fixed now, but previously in window.rs the center_window function (ln 380) had the code:

self.command_queue.push(WindowCommand::center);

Since then it has seemingly been given the correct casing, but I'd take this as an opportunity to double check the casing of anything.

alice-i-cecile

This will need to be rebased to get CI to pass, but I'm very happy with this API now. Good work to both @LoipesMas and @CalinZBaenan :D

mockersf · 2022-06-13T22:09:56Z

crates/bevy_window/src/window.rs

@@ -641,8 +667,8 @@ pub struct WindowDescriptor {
    /// May vary from the physical height due to different pixel density on different monitors.
    pub height: f32,
    /// The position on the screen that the window will be centered at.
-    /// If set to `None`, some platform-specific position will be chosen.
-    pub position: Option<Vec2>,
+    /// See [`WindowPosition`] for available values.


Suggested change

/// See [`WindowPosition`] for available values.

this line is not very useful, cargo doc will already link to the enum on the field type

I wanted to explicitly point to WindowPosition page, because just the name "WindowPosition" doesn't convey that it could be something else than just raw coordinates (at least in my opinion). But maybe you're right that this is unnecessary. Or maybe different wording/naming would fit this better?

@LoipesMas If you want a slightly better name: I recommend WindowLocatoon if that's not taken.
It's a synonym, but it makes sense that Center is a "Location".

Yeah, maybe something like that would be clearer. But TBF I'm probably too involved(?) to make an objective judgement here, so I'll rely on other people's opinion.

I like WindowLocation; I think that would help communicate that there's multiple subtle variants.

mockersf · 2022-06-13T22:14:11Z

crates/bevy_window/src/window.rs

+    /// Modifies the position of the window to be in the center of the current monitor
+    ///
+    /// # Platform-specific
+    /// - iOS: Can only be called on the main thread.
+    /// - Web / Android / Wayland: Unsupported.
+    #[inline]
+    pub fn center_window(&mut self) {
+        self.command_queue.push(WindowCommand::Center);
+    }
+


I don't think this should be a new method, you could change the existing set_position to take a WindowPosition. It would support all possible extensions like if we add WindowPosition::BottomRight... but then you could call set_position(WindowPosition::Automatic) which doesn't make sense...

What do you think?

I think your proposed design is better. I would just document that setting the window position to automatic is a no-op.

@mockersf Interesting idea suggesting other places to put the window except in the center.

[P.S. : I feel like this sounds unnecessarily passive-aggressive, so I wanted to clarify it's not meant to be. I just have no other comment to tack on.]

I considered the fact that we might want to add more positions, such as bottom-right etc., but I wasn't sure if there would be a use case for that and where do we draw the line of being a window manager.
But if you expect that there would be a use case for more values, then changing set_position signature makes sense.
To avoid code duplication (between set_position and create_window) we would need to abstract out WindowPosition -> actual position code, right? So perhaps a function that takes WindowPosition, window size and screen size and returns screen space position?

CalinZBaenen · 2022-06-13T23:53:58Z

This will need to be rebased to get CI to pass, but I'm very happy with this API now. Good work to both @LoipesMas and @CalinZBaenan :D

Thank you.
I share the same happiness for this change.

BTW, what is "CI"?

alice-i-cecile · 2022-06-14T00:16:42Z

CI == "Continuous Integration". It's the automated checks that you can see at the bottom of this thread that ensure that the code is working correctly :)

LoipesMas · 2022-06-14T09:20:30Z

@LoipesMas It seems to be fixed now, but previously in window.rs the center_window function (ln 380) had the code:
self.command_queue.push(WindowCommand::center);
Since then it has seemingly been given the correct casing, but I'd take this as an opportunity to double check the casing of anything.

@CalinZBaenen Not sure what could have happened there. But WindowCommand::center would be invalid code so aforementioned CI would catch that anyway ;]

crates/bevy_winit/src/lib.rs

alice-i-cecile · 2022-06-20T17:16:50Z

@LoipesMas can you rebase this? This is almost ready but the merge conflicts need to be resolved.

Reviewers: should we block on the docs discussion?

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Weibye · 2022-06-20T18:22:11Z

Reviewers: should we block on the docs discussion?

The multi-monitor user-story in Bevy atm is not very good. Most input and UI assumes only one monitor, as does a lot of positioning and events. I think we should sit down and flesh out what is the desired user-story here, but that is outside the scope of this PR hence I don't think it's worth blocking on.

LoipesMas · 2022-06-20T18:31:25Z

Rebased!
I thought we wanted to resolve those docs/API issues first, but it can be done in another PR as well.

alice-i-cecile · 2022-06-20T18:44:09Z

I think we should sit down and flesh out what is the desired user-story here, but that is outside the scope of this PR hence I don't think it's worth blocking on.

I think that's reasonable. @Weibye are you comfortable making that issue?

alice-i-cecile

Took another look; there's a few easy wins here. We should make the failure behavior less panicky, and allow users to specify which window they want to center this on with a good default.

crates/bevy_winit/src/lib.rs

Weibye · 2022-06-20T19:33:06Z

I think that's reasonable. @Weibye are you comfortable making that issue?

Not quite sure if this should be an issue or a discussion yet, but will definitely set this up once I have some time to map out the current capabilities (or lack of thereof)

CalinZBaenen · 2022-07-03T20:37:40Z

Since ðis is marked for S-Ready-For-Final-Review, can someone notify me once ðis feature fully makes it in.
Þanks!

alice-i-cecile · 2022-07-04T12:47:22Z

@CalinZBaenen merging now!

bors r+

# Objective - Fixes #4993 ## Solution - ~~Add `centered` property to `WindowDescriptor`~~ - Add `WindowPosition` enum - `WindowDescriptor.position` is now `WindowPosition` instead of `Option<Vec2>` - Add `center_window` function to `Window` ## Migration Guide - If using `WindowDescriptor`, replace `position: None` with `position: WindowPosition::Default` and `position: Some(vec2)` with `WindowPosition::At(vec2)`. I'm not sure if this is the best approach, so feel free to give any feedback. Also I'm not sure how `Option`s should be handled in `bevy_winit/src/lib.rs:161`. Also, on window creation we can't (or at least I couldn't) get `outer_size`, so this doesn't include decorations in calculations.

bors · 2022-07-04T13:36:21Z

Pull request successfully merged into main.

Build succeeded:

# Objective - Fixes bevyengine#4993 ## Solution - ~~Add `centered` property to `WindowDescriptor`~~ - Add `WindowPosition` enum - `WindowDescriptor.position` is now `WindowPosition` instead of `Option<Vec2>` - Add `center_window` function to `Window` ## Migration Guide - If using `WindowDescriptor`, replace `position: None` with `position: WindowPosition::Default` and `position: Some(vec2)` with `WindowPosition::At(vec2)`. I'm not sure if this is the best approach, so feel free to give any feedback. Also I'm not sure how `Option`s should be handled in `bevy_winit/src/lib.rs:161`. Also, on window creation we can't (or at least I couldn't) get `outer_size`, so this doesn't include decorations in calculations.

ItsSunnyMonster · 2023-06-05T06:20:32Z

If the user interface is scaled (I use 125% on Windows) the window appears in the wrong place. (as shown in the images below) Should I create a new issue?

alice-i-cecile · 2023-06-05T18:54:59Z

Yes please; this seems incorrect to me and a new issue is the correct way to track this.

mockersf requested changes Jun 12, 2022

View reviewed changes

alice-i-cecile added A-Windowing Platform-agnostic interface layer to run your app in C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jun 13, 2022

LoipesMas force-pushed the center_window branch 2 times, most recently from d191ba2 to 490aa35 Compare June 13, 2022 11:27

LoipesMas mentioned this pull request Jun 13, 2022

Export the needed parts of Winit or add better utilities for centering a window. #4993

Closed

LoipesMas force-pushed the center_window branch from 490aa35 to f6fee60 Compare June 13, 2022 12:52

LoipesMas requested a review from mockersf June 13, 2022 13:05

alice-i-cecile reviewed Jun 13, 2022

View reviewed changes

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jun 13, 2022

View reviewed changes

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jun 13, 2022

View reviewed changes

crates/bevy_window/src/window.rs Show resolved Hide resolved

alice-i-cecile reviewed Jun 13, 2022

View reviewed changes

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved

Weibye approved these changes Jun 13, 2022

View reviewed changes

alice-i-cecile approved these changes Jun 13, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 13, 2022

mockersf reviewed Jun 13, 2022

View reviewed changes

LoipesMas commented Jun 14, 2022

View reviewed changes

crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved

alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 20, 2022

Add option to center a window

7d6f900

LoipesMas and others added 2 commits June 20, 2022 20:12

Apply suggestions from code review

96ef2b1

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Improve documentation and naming

655d747

LoipesMas force-pushed the center_window branch from e1d977e to 655d747 Compare June 20, 2022 18:14

alice-i-cecile requested changes Jun 20, 2022

View reviewed changes

crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved

crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved

Change window centering to use enum for monitor

872bae7

alice-i-cecile approved these changes Jul 3, 2022

View reviewed changes

Sheepyhead approved these changes Jul 3, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 3, 2022

bors bot changed the title ~~Add option to center a window~~ [Merged by Bors] - Add option to center a window Jul 4, 2022

bors bot closed this Jul 4, 2022

This was referenced Jul 17, 2022

Center Window on Windows #4876

Closed

Game window opens off-screen on Windows #5358

Open

Weibye mentioned this pull request Aug 10, 2022

[Merged by Bors] - Windows as Entities #5589

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Add option to center a window #4999

[Merged by Bors] - Add option to center a window #4999

LoipesMas commented Jun 12, 2022 •

edited

Loading

mockersf left a comment

CalinZBaenen commented Jun 13, 2022

LoipesMas commented Jun 13, 2022

LoipesMas commented Jun 13, 2022

CalinZBaenen commented Jun 13, 2022

LoipesMas commented Jun 13, 2022

CalinZBaenen commented Jun 13, 2022

alice-i-cecile left a comment •

edited

Loading

mockersf Jun 13, 2022 •

edited

Loading

LoipesMas Jun 14, 2022

CalinZBaenen Jun 14, 2022

LoipesMas Jun 14, 2022

alice-i-cecile Jun 14, 2022

mockersf Jun 13, 2022

alice-i-cecile Jun 13, 2022

CalinZBaenen Jun 14, 2022 •

edited

Loading

LoipesMas Jun 14, 2022

CalinZBaenen commented Jun 13, 2022

alice-i-cecile commented Jun 14, 2022

LoipesMas commented Jun 14, 2022

alice-i-cecile commented Jun 20, 2022

Weibye commented Jun 20, 2022

LoipesMas commented Jun 20, 2022

alice-i-cecile commented Jun 20, 2022

alice-i-cecile left a comment

Weibye commented Jun 20, 2022

CalinZBaenen commented Jul 3, 2022

alice-i-cecile commented Jul 4, 2022

bors bot commented Jul 4, 2022

ItsSunnyMonster commented Jun 5, 2023

alice-i-cecile commented Jun 5, 2023

[Merged by Bors] - Add option to center a window #4999

[Merged by Bors] - Add option to center a window #4999

Conversation

LoipesMas commented Jun 12, 2022 • edited Loading

Objective

Solution

Migration Guide

mockersf left a comment

Choose a reason for hiding this comment

CalinZBaenen commented Jun 13, 2022

LoipesMas commented Jun 13, 2022

LoipesMas commented Jun 13, 2022

CalinZBaenen commented Jun 13, 2022

LoipesMas commented Jun 13, 2022

CalinZBaenen commented Jun 13, 2022

alice-i-cecile left a comment • edited Loading

Choose a reason for hiding this comment

mockersf Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

LoipesMas Jun 14, 2022

Choose a reason for hiding this comment

CalinZBaenen Jun 14, 2022

Choose a reason for hiding this comment

LoipesMas Jun 14, 2022

Choose a reason for hiding this comment

alice-i-cecile Jun 14, 2022

Choose a reason for hiding this comment

mockersf Jun 13, 2022

Choose a reason for hiding this comment

alice-i-cecile Jun 13, 2022

Choose a reason for hiding this comment

CalinZBaenen Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

LoipesMas Jun 14, 2022

Choose a reason for hiding this comment

CalinZBaenen commented Jun 13, 2022

alice-i-cecile commented Jun 14, 2022

LoipesMas commented Jun 14, 2022

alice-i-cecile commented Jun 20, 2022

Weibye commented Jun 20, 2022

LoipesMas commented Jun 20, 2022

alice-i-cecile commented Jun 20, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

Weibye commented Jun 20, 2022

CalinZBaenen commented Jul 3, 2022

alice-i-cecile commented Jul 4, 2022

bors bot commented Jul 4, 2022

ItsSunnyMonster commented Jun 5, 2023

alice-i-cecile commented Jun 5, 2023

LoipesMas commented Jun 12, 2022 •

edited

Loading

alice-i-cecile left a comment •

edited

Loading

mockersf Jun 13, 2022 •

edited

Loading

CalinZBaenen Jun 14, 2022 •

edited

Loading