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

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

Closed
wants to merge 4 commits into from

Conversation

LoipesMas
Copy link
Contributor

@LoipesMas LoipesMas commented Jun 12, 2022

Objective

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.

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

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)
}

@alice-i-cecile 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
@CalinZBaenen
Copy link

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

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

@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.

@CalinZBaenen
Copy link

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

@LoipesMas
Copy link
Contributor Author

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

@CalinZBaenen
Copy link

@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.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

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

@alice-i-cecile 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
@@ -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.
Copy link
Member

@mockersf mockersf Jun 13, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

@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".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines 374 to 430
/// 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);
}

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 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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

@CalinZBaenen CalinZBaenen Jun 14, 2022

Choose a reason for hiding this comment

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

@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.]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

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

@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 ;]

@alice-i-cecile 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
@alice-i-cecile
Copy link
Member

@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?

LoipesMas and others added 2 commits June 20, 2022 20:12
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@Weibye
Copy link
Contributor

Weibye commented Jun 20, 2022

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

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

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?

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved
@Weibye
Copy link
Contributor

Weibye commented Jun 20, 2022

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)

@alice-i-cecile 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
@CalinZBaenen
Copy link

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

@CalinZBaenen merging now!

bors r+

bors bot pushed a commit that referenced this pull request Jul 4, 2022
# 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 bors bot changed the title Add option to center a window [Merged by Bors] - Add option to center a window Jul 4, 2022
@bors bors bot closed this Jul 4, 2022
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
# 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.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# 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.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# 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
Copy link

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?
image
image

@alice-i-cecile
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export the needed parts of Winit or add better utilities for centering a window.
7 participants