[Merged by Bors] - Allow closing windows at runtime by DJMcNab · Pull Request #3575 · bevyengine/bevy

DJMcNab · 2022-01-06T20:09:47Z

Objective

Fixes #3180, builds from #2898

Solution

Support requesting a window to be closed and closing a window in bevy_window, and handle this in bevy_winit.

This is a stopgap until we move to windows as entites, which I'm sure I'll get around to eventually.

Changelog

Added

Window::close to allow closing windows.
WindowClosed to allow reacting to windows being closed.

Changed

Replaced bevy::system::exit_on_esc_system with bevy::window::close_on_esc.

Fixed

The app no longer exits when any window is closed. This difference is only observable when there are multiple windows.

Migration Guide

bevy::input::system::exit_on_esc_system has been removed. Use bevy::window::close_on_esc instead.
CloseWindow has been removed. Use Window::close instead.
The Close variant has been added to WindowCommand. Handle this by closing the relevant window.

crates/bevy_window/src/system.rs

alice-i-cecile

Looks good! Well-engineered and useful. I carefully reviewed the whole thing, but all I could find were a couple nits.

alice-i-cecile · 2022-04-25T16:59:40Z

@superdump, once you're happy with this, feel free to merge it in.

DJMcNab · 2022-04-25T17:09:19Z

I still want to finish cleaning up the docs. E.g. there's a sentence fragment remaining.

Edit: I think it's now ready for final review/merge

alice-i-cecile · 2022-04-26T17:54:51Z

crates/bevy_window/src/lib.rs

+    /// If true, this plugin will add [`close_when_requested`] to [`CoreStage::Update`].
+    /// If this system (or a replacement) is not running, the close button will have no effect.
+    /// This may surprise your users. It is recommended to leave this setting as `true`.
+    pub close_when_requested: bool,


I'm not sure this field should exist. I agree with you that it's unintuitive behavior to set this to false; when would app developers want to do this?

For example, if the user has unsaved changes, it's traditional to give an 'unsaved changes' warning window.

Ah, got it. In that case, this should be exposed as a resource: that behavior needs to be able to be changed dynamically.

Not really? If a developer wants this functionality, they can easily enough add it themselves. This is merely making sure the default behaviour is correct, and we give an escape hatch for when people need more complicated logic.

That is, I don't really want to encourage disabling this, but I want to make the option available.

alice-i-cecile · 2022-04-26T17:56:45Z

crates/bevy_winit/src/lib.rs

+                bevy_window::WindowCommand::Close => {
+                    let window = winit_windows.remove_window(id);
+                    // Close the window
+                    drop(window);


Why do we need this explicit drop call? Shouldn't this just happen automatically at the end of the scope?

I'm not sure. I think I was trying to clarify that the drop is what actually closes the operating system window. Since Window owns that window, I thought it better to be explicit about the side effects, and we can be sure that this drops at the expected time.

Hmm. I think I'd prefer to just call winit_windows.remove_window(id) and have a comment explaining this mechanism.

alice-i-cecile

One blocking question about the necessity of the close_when_requested field.

I would also love to see tests for this, but I don't think that's feasible yet.

superdump

Looks good to me. I tested multiple_windows, hit esc twice, both windows closed and the app exited. Very nice!

superdump · 2022-05-05T01:35:32Z

bors r+

# Objective Fixes #3180, builds from #2898 ## Solution Support requesting a window to be closed and closing a window in `bevy_window`, and handle this in `bevy_winit`. This is a stopgap until we move to windows as entites, which I'm sure I'll get around to eventually. ## Changelog ### Added - `Window::close` to allow closing windows. - `WindowClosed` to allow reacting to windows being closed. ### Changed Replaced `bevy::system::exit_on_esc_system` with `bevy::window::close_on_esc`. ## Fixed The app no longer exits when any window is closed. This difference is only observable when there are multiple windows. ## Migration Guide `bevy::system::exit_on_esc_system` has been removed. Use `bevy::window::close_on_esc` instead. `CloseWindow` has been removed. Use `Window::close` instead. The `Close` variant has been added to `WindowCommand`. Handle this by closing the relevant window.

bors · 2022-05-05T01:40:45Z

Build failed:

ci

Make the rest of bevy work with multiple windows Remove exit_on_esc Add suggested docs

Since their brokeness isn't my fault

superdump · 2022-05-05T13:35:27Z

bors r+

# Objective Fixes #3180, builds from #2898 ## Solution Support requesting a window to be closed and closing a window in `bevy_window`, and handle this in `bevy_winit`. This is a stopgap until we move to windows as entites, which I'm sure I'll get around to eventually. ## Changelog ### Added - `Window::close` to allow closing windows. - `WindowClosed` to allow reacting to windows being closed. ### Changed Replaced `bevy::system::exit_on_esc_system` with `bevy::window::close_on_esc`. ## Fixed The app no longer exits when any window is closed. This difference is only observable when there are multiple windows. ## Migration Guide `bevy::input::system::exit_on_esc_system` has been removed. Use `bevy::window::close_on_esc` instead. `CloseWindow` has been removed. Use `Window::close` instead. The `Close` variant has been added to `WindowCommand`. Handle this by closing the relevant window.

bors · 2022-05-05T13:54:49Z

Pull request successfully merged into main.

Build succeeded:

# Objective Fixes bevyengine#3180, builds from bevyengine#2898 ## Solution Support requesting a window to be closed and closing a window in `bevy_window`, and handle this in `bevy_winit`. This is a stopgap until we move to windows as entites, which I'm sure I'll get around to eventually. ## Changelog ### Added - `Window::close` to allow closing windows. - `WindowClosed` to allow reacting to windows being closed. ### Changed Replaced `bevy::system::exit_on_esc_system` with `bevy::window::close_on_esc`. ## Fixed The app no longer exits when any window is closed. This difference is only observable when there are multiple windows. ## Migration Guide `bevy::input::system::exit_on_esc_system` has been removed. Use `bevy::window::close_on_esc` instead. `CloseWindow` has been removed. Use `Window::close` instead. The `Close` variant has been added to `WindowCommand`. Handle this by closing the relevant window.

# Objective - Avoid unbounded HashMap growth for opening/closing windows. ## Solution - Remove map entry in `WinitWindows::remove_window`. ## Migration Guide - `WinitWindows::get_window_entity` now returns `None` after a window is closed, instead of a dead entity. --- ## Comments The comment this PR replaces was added in #3575. Since `get_window_entity` now returns an `Entity` instead of a `WindowId`, this no longer seems useful. Note that `get_window_entity` is only used [here](https://github.com/bevyengine/bevy/blob/56bcbb097552b45e3ff48c48947ed8ee4e2c24b1/crates/bevy_winit/src/lib.rs#L436), immediately followed by a warning if the entity returned doesn't exist.

DJMcNab changed the title ~~Allow closing~~ Allow closing windows at runtime Jan 6, 2022

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 6, 2022

DJMcNab added A-Windowing Platform-agnostic interface layer to run your app in C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Jan 6, 2022

alice-i-cecile reviewed Jan 6, 2022

View reviewed changes

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

alice-i-cecile reviewed Jan 6, 2022

View reviewed changes

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

alice-i-cecile reviewed Jan 6, 2022

View reviewed changes

DJMcNab force-pushed the close-windows branch from 18c65a8 to ff3b0d0 Compare January 6, 2022 23:05

DJMcNab requested a review from alice-i-cecile January 11, 2022 09:10

DJMcNab force-pushed the close-windows branch from ff3b0d0 to 25da101 Compare January 11, 2022 09:22

DJMcNab requested a review from TheRawMeatball January 11, 2022 10:06

DJMcNab force-pushed the close-windows branch from 25da101 to 1f835c1 Compare February 14, 2022 17:43

alice-i-cecile approved these changes Feb 14, 2022

View reviewed changes

TheRawMeatball approved these changes Feb 14, 2022

View reviewed changes

fowenix mentioned this pull request Apr 19, 2022

Windows as entities #4530

Closed

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 Apr 25, 2022

DJMcNab force-pushed the close-windows branch 2 times, most recently from 4f4d042 to e5d5be4 Compare April 25, 2022 15:21

alice-i-cecile requested a review from superdump April 25, 2022 16:59

DJMcNab added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 26, 2022

DJMcNab requested a review from alice-i-cecile April 26, 2022 10:34

alice-i-cecile reviewed Apr 26, 2022

View reviewed changes

alice-i-cecile added the A-Rendering Drawing game state to the screen label May 2, 2022

superdump approved these changes May 5, 2022

View reviewed changes

DJMcNab added 6 commits May 5, 2022 08:40

Add support to bevy_window for closing windows

fb9fb17

Make the rest of bevy work with multiple windows Remove exit_on_esc Add suggested docs

Update docs

72eaf1f

Update docs some more

fb42b8b

Don't update RequestRedraw docs

bbce82b

Since their brokeness isn't my fault

Remove surplus comment

2510dea

Remove explicit drop

c9deedb

DJMcNab force-pushed the close-windows branch from 26ddd5d to c9deedb Compare May 5, 2022 07:40

Fix resizing compilation

aaaf85c

DJMcNab requested a review from superdump May 5, 2022 10:08

bors bot changed the title ~~Allow closing windows at runtime~~ [Merged by Bors] - Allow closing windows at runtime May 5, 2022

bors bot closed this May 5, 2022

DJMcNab deleted the close-windows branch May 5, 2022 17:16

infmagic2047 mentioned this pull request May 6, 2022

Closing the window causes panic in 2D #4678

Closed

UkoeHB mentioned this pull request Mar 27, 2024

Clean up WinitWindows::remove_window #12749

Merged

Uh oh!

Conversation

DJMcNab commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Added

Changed

Fixed

Migration Guide

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Apr 25, 2022

Uh oh!

DJMcNab commented Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

DJMcNab Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

DJMcNab Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

DJMcNab Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

superdump left a comment

Choose a reason for hiding this comment

Uh oh!

superdump commented May 5, 2022

Uh oh!

bors bot commented May 5, 2022

Uh oh!

superdump commented May 5, 2022

Uh oh!

bors bot commented May 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DJMcNab commented Jan 6, 2022 •

edited

Loading

DJMcNab commented Apr 25, 2022 •

edited

Loading

DJMcNab Apr 26, 2022 •

edited

Loading