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] - update winit to 0.28 #7480

Closed
wants to merge 5 commits into from
Closed

[Merged by Bors] - update winit to 0.28 #7480

wants to merge 5 commits into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Feb 3, 2023

Objective

  • Update winit to 0.28

Solution

  • Small API change
  • A security advisory has been added for a unmaintained crate used by a dependency of winit build script for wayland

I didn't do anything for Android support in this PR though it should be fixable, it should be done in a separate one, maybe #6830

Changelog

  • window.always_on_top has been removed, you can now use window.window_level

Migration Guide

before:

    app.new()
        .add_plugins(DefaultPlugins.set(WindowPlugin {
            primary_window: Some(Window {
                always_on_top: true,
                ..default()
            }),
            ..default()
        }));

after:

    app.new()
        .add_plugins(DefaultPlugins.set(WindowPlugin {
            primary_window: Some(Window {
                window_level: bevy::window::WindowLevel::AlwaysOnTop,
                ..default()
            }),
            ..default()
        }));

@mockersf mockersf added A-Windowing Platform-agnostic interface layer to run your app in C-Dependencies A change to the crates that Bevy depends on labels Feb 3, 2023
@mockersf mockersf mentioned this pull request Feb 3, 2023
Closed
@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 3, 2023
@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Feb 3, 2023
hymm
hymm approved these changes Feb 3, 2023
View reviewed changes
examples/window/window_settings.rs Outdated Show resolved Hide resolved
@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 Feb 3, 2023
@rparrett
Copy link
Contributor

rparrett commented Feb 3, 2023

This warning in multiple_windows seems new:

2023-02-03T03:15:37.429478Z  WARN bevy_winit: Window 4v0 is missing `Window` component, skipping event Destroyed

james7132
james7132 approved these changes Feb 3, 2023
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot pushed a commit that referenced this pull request Feb 3, 2023
@mockersf
update winit to 0.28 (#7480)
1406963 
# Objective

- Update winit to 0.28

## Solution

- Small API change 
- A security advisory has been added for a unmaintained crate used by a dependency of winit build script for wayland

I didn't do anything for Android support in this PR though it should be fixable, it should be done in a separate one, maybe #6830 

---

## Changelog

- `window.always_on_top` has been removed, you can now use `window.window_level`

## Migration Guide

before:
```rust
    app.new()
        .add_plugins(DefaultPlugins.set(WindowPlugin {
            primary_window: Some(Window {
                always_on_top: true,
                ..default()
            }),
            ..default()
        }));
```

after:
```rust
    app.new()
        .add_plugins(DefaultPlugins.set(WindowPlugin {
            primary_window: Some(Window {
                window_level: bevy::window::WindowLevel::AlwaysOnTop,
                ..default()
            }),
            ..default()
        }));
```
@bors
Copy link
Contributor

bors bot commented Feb 3, 2023

Build failed:

@james7132
Copy link
Member

Looks like we need at least a few Android changes to allow it to properly build.

bors r-

@james7132 james7132 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 Feb 3, 2023
@mockersf
Copy link
Member Author

mockersf commented Feb 3, 2023

This warning in multiple_windows seems new:

2023-02-03T03:15:37.429478Z  WARN bevy_winit: Window 4v0 is missing `Window` component, skipping event Destroyed

Are you on macOS? It seems that event was not sent before, which they fixed on winit 0.28 in rust-windowing/winit#2574

But I think that warning was already present on other platforms. And as that event is sent after the window has been closed and despawned on Bevy side, there's not much we can do about it, maybe ignore it? But that's another issue

@mockersf
Copy link
Member Author

mockersf commented Feb 3, 2023

bors try

bors bot added a commit that referenced this pull request Feb 3, 2023
@bors
Try #7480:
d5b768c
@bors
Copy link
Contributor

bors bot commented Feb 3, 2023

try

Build succeeded:

@mockersf
Copy link
Member Author

mockersf commented Feb 3, 2023 •
edited
Loading

the android example "works" on my android device, as in it crashes with the usual RustStdoutStderr: thread '<unnamed>' panicked at 'Cannot get the native window, it's null and will always be null before Event::Resumed and after Event::Suspended. Make sure you only call this function between those events.'

@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 Feb 3, 2023
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Feb 3, 2023
@mockersf
update winit to 0.28 (#7480)
3900b48 
# Objective

- Update winit to 0.28

## Solution

- Small API change 
- A security advisory has been added for a unmaintained crate used by a dependency of winit build script for wayland

I didn't do anything for Android support in this PR though it should be fixable, it should be done in a separate one, maybe #6830 

---

## Changelog

- `window.always_on_top` has been removed, you can now use `window.window_level`

## Migration Guide

before:
```rust
    app.new()
        .add_plugins(DefaultPlugins.set(WindowPlugin {
            primary_window: Some(Window {
                always_on_top: true,
                ..default()
            }),
            ..default()
        }));
```

after:
```rust
    app.new()
        .add_plugins(DefaultPlugins.set(WindowPlugin {
            primary_window: Some(Window {
                window_level: bevy::window::WindowLevel::AlwaysOnTop,
                ..default()
            }),
            ..default()
        }));
```
@bors
Copy link
Contributor

bors bot commented Feb 3, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title update winit to 0.28 [Merged by Bors] - update winit to 0.28 Feb 3, 2023
@bors bors bot closed this Feb 3, 2023
This was referenced Feb 3, 2023
Closed
Closed
@alice-i-cecile alice-i-cecile mentioned this pull request Feb 4, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VitalyAnkh VitalyAnkh VitalyAnkh approved these changes

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

@james7132 james7132 james7132 approved these changes

@hymm hymm hymm approved these changes

Assignees
No one assigned
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Dependencies A change to the crates that Bevy depends on 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
0.10
Development

Successfully merging this pull request may close these issues.
6 participants
@mockersf @rparrett @james7132 @alice-i-cecile @hymm @VitalyAnkh