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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Support monitor selection for all window modes. #5878

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Support monitor selection for all window modes. #5878

wants to merge 4 commits into from

Conversation

tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Sep 4, 2022 •
edited
Loading

Objective

Support monitor selection for all window modes.
Fixes #5875.

Changelog

  • Moved MonitorSelection out of WindowPosition::Centered, into WindowDescriptor.
  • WindowPosition::At is now relative to the monitor instead of being in 'desktop space'.
  • Renamed MonitorSelection::Number to MonitorSelection::Index for clarity.
  • Added WindowMode to the prelude.
  • Window::set_position is now relative to a monitor and takes a MonitorSelection as argument.

Migration Guide

MonitorSelection was moved out of WindowPosition::Centered, into WindowDescriptor.
MonitorSelection::Number was renamed to MonitorSelection::Index.

// Before
.insert_resource(WindowDescriptor {
    position: WindowPosition::Centered(MonitorSelection::Number(1)),
    ..default()
})
// After
.insert_resource(WindowDescriptor {
    monitor: MonitorSelection::Index(1),
    position: WindowPosition::Centered,
    ..default()
})

Window::set_position now takes a MonitorSelection as argument.

window.set_position(MonitorSelection::Current, position);

@Weibye Weibye 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 Sep 5, 2022
Weibye
Weibye approved these changes Sep 5, 2022
View reviewed changes
Copy link
Contributor

@Weibye Weibye left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

@Weibye Weibye 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 Sep 5, 2022
@tim-blackbird tim-blackbird mentioned this pull request Sep 5, 2022
Closed
mockersf
mockersf reviewed Sep 5, 2022
View reviewed changes
crates/bevy_window/src/window.rs Outdated
Comment on lines 754 to 756
/// The window's top-left corner will be placed at the specified position (in pixels).
///
/// Note that this does not account for window decorations.
Centered(MonitorSelection),
/// The window's top-left corner will be placed at the specified position (in pixels)
///
/// (0,0) represents top-left corner of screen space.
/// (0,0) represents top-left corner of the selected monitor.
Copy link
Member

@mockersf mockersf Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

these two lines feels a lot redundant (and miss how to specify the selected monitor)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

window's top-left corner and top-left corner of the monitor are not redundant.

@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 Sep 6, 2022
@alice-i-cecile
Copy link
Member

I'm going to wait on the doc improvements and then merge this in :) Let me know if you'd like help.

@tim-blackbird
Copy link
Contributor Author

I've modified Window::set_position to be relative to a monitor and changed some of the docs.

alice-i-cecile
alice-i-cecile approved these changes Sep 6, 2022
View reviewed changes
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.

Excellent docs, much nicer behavior, clear and straightforward code. I love this, thank you.

@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Sep 6, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 6, 2022
@tim-blackbird
Support monitor selection for all window modes. (#5878)
28c16b9 
# Objective
Support monitor selection for all window modes.
Fixes #5875.

## Changelog

* Moved `MonitorSelection` out of `WindowPosition::Centered`, into `WindowDescriptor`.
* `WindowPosition::At` is now relative to the monitor instead of being in 'desktop space'.
* Renamed `MonitorSelection::Number` to `MonitorSelection::Index` for clarity.
* Added `WindowMode` to the prelude.
* `Window::set_position` is now relative to a monitor and takes a `MonitorSelection` as argument.

## Migration Guide

`MonitorSelection` was moved out of `WindowPosition::Centered`, into `WindowDescriptor`.
`MonitorSelection::Number` was renamed to `MonitorSelection::Index`.
```rust
// Before
.insert_resource(WindowDescriptor {
    position: WindowPosition::Centered(MonitorSelection::Number(1)),
    ..default()
})
// After
.insert_resource(WindowDescriptor {
    monitor: MonitorSelection::Index(1),
    position: WindowPosition::Centered,
    ..default()
})
```
`Window::set_position` now takes a `MonitorSelection` as argument.
```rust
window.set_position(MonitorSelection::Current, position);
```

Co-authored-by: devil-ira <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 6, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Support monitor selection for all window modes. [Merged by Bors] - Support monitor selection for all window modes. Sep 6, 2022
@bors bors bot closed this Sep 6, 2022
nicopap pushed a commit to nicopap/bevy that referenced this pull request Sep 12, 2022
@tim-blackbird @nicopap
Support monitor selection for all window modes. (bevyengine#5878)
c9c1a91 
# Objective
Support monitor selection for all window modes.
Fixes bevyengine#5875.

## Changelog

* Moved `MonitorSelection` out of `WindowPosition::Centered`, into `WindowDescriptor`.
* `WindowPosition::At` is now relative to the monitor instead of being in 'desktop space'.
* Renamed `MonitorSelection::Number` to `MonitorSelection::Index` for clarity.
* Added `WindowMode` to the prelude.
* `Window::set_position` is now relative to a monitor and takes a `MonitorSelection` as argument.

## Migration Guide

`MonitorSelection` was moved out of `WindowPosition::Centered`, into `WindowDescriptor`.
`MonitorSelection::Number` was renamed to `MonitorSelection::Index`.
```rust
// Before
.insert_resource(WindowDescriptor {
    position: WindowPosition::Centered(MonitorSelection::Number(1)),
    ..default()
})
// After
.insert_resource(WindowDescriptor {
    monitor: MonitorSelection::Index(1),
    position: WindowPosition::Centered,
    ..default()
})
```
`Window::set_position` now takes a `MonitorSelection` as argument.
```rust
window.set_position(MonitorSelection::Current, position);
```

Co-authored-by: devil-ira <[email protected]>
@tim-blackbird tim-blackbird deleted the monitor_selection branch October 21, 2022 15:09
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@tim-blackbird @james7132
Support monitor selection for all window modes. (bevyengine#5878)
56cfa5c 
# Objective
Support monitor selection for all window modes.
Fixes bevyengine#5875.

## Changelog

* Moved `MonitorSelection` out of `WindowPosition::Centered`, into `WindowDescriptor`.
* `WindowPosition::At` is now relative to the monitor instead of being in 'desktop space'.
* Renamed `MonitorSelection::Number` to `MonitorSelection::Index` for clarity.
* Added `WindowMode` to the prelude.
* `Window::set_position` is now relative to a monitor and takes a `MonitorSelection` as argument.

## Migration Guide

`MonitorSelection` was moved out of `WindowPosition::Centered`, into `WindowDescriptor`.
`MonitorSelection::Number` was renamed to `MonitorSelection::Index`.
```rust
// Before
.insert_resource(WindowDescriptor {
    position: WindowPosition::Centered(MonitorSelection::Number(1)),
    ..default()
})
// After
.insert_resource(WindowDescriptor {
    monitor: MonitorSelection::Index(1),
    position: WindowPosition::Centered,
    ..default()
})
```
`Window::set_position` now takes a `MonitorSelection` as argument.
```rust
window.set_position(MonitorSelection::Current, position);
```

Co-authored-by: devil-ira <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@tim-blackbird @ItsDoot
Support monitor selection for all window modes. (bevyengine#5878)
3118bc8 
# Objective
Support monitor selection for all window modes.
Fixes bevyengine#5875.

## Changelog

* Moved `MonitorSelection` out of `WindowPosition::Centered`, into `WindowDescriptor`.
* `WindowPosition::At` is now relative to the monitor instead of being in 'desktop space'.
* Renamed `MonitorSelection::Number` to `MonitorSelection::Index` for clarity.
* Added `WindowMode` to the prelude.
* `Window::set_position` is now relative to a monitor and takes a `MonitorSelection` as argument.

## Migration Guide

`MonitorSelection` was moved out of `WindowPosition::Centered`, into `WindowDescriptor`.
`MonitorSelection::Number` was renamed to `MonitorSelection::Index`.
```rust
// Before
.insert_resource(WindowDescriptor {
    position: WindowPosition::Centered(MonitorSelection::Number(1)),
    ..default()
})
// After
.insert_resource(WindowDescriptor {
    monitor: MonitorSelection::Index(1),
    position: WindowPosition::Centered,
    ..default()
})
```
`Window::set_position` now takes a `MonitorSelection` as argument.
```rust
window.set_position(MonitorSelection::Current, position);
```

Co-authored-by: devil-ira <[email protected]>
@tim-blackbird tim-blackbird mentioned this pull request Mar 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf left review comments

@Sheepyhead Sheepyhead Sheepyhead approved these changes

@Weibye Weibye Weibye approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@afonsolage afonsolage afonsolage approved these changes

Assignees
No one assigned
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

winit ignores monitor selection in fullscreen
6 participants
@tim-blackbird @alice-i-cecile @afonsolage @mockersf @Weibye @Sheepyhead