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

Fix desktop lifetime non-mainwindow cancellation #17059

Merged
merged 8 commits into from
Oct 11, 2024
Merged

Fix desktop lifetime non-mainwindow cancellation #17059

merged 8 commits into from
Oct 11, 2024

Conversation

maxkatz6
Copy link
Member

What does the pull request do?

That's a rather old bug which was surfaced after my previous fix: #15438
Current PR makes Avalonia behave closer to WPF:
When ShutdownMode is set to OnMainWindowClose, only main window can cancel shutdown.

What is the current behavior?

Any window can cancel app shutdown, even after main window was closed.

What is the updated/expected behavior with this PR?

Only main window shutdown cancellation is respected, when ShutdownMode=OnMainWindowClose

Checklist

  • Added unit tests (if possible)?

Breaking changes

Possible behavioral breaking change. At the same time, new behavior might be an expected one for WPF developers.

@maxkatz6 maxkatz6 added the bug label Sep 19, 2024
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0051961-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

MrJul
MrJul reviewed Sep 19, 2024
View reviewed changes
Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

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

What do you think about ignoring WindowClosingEventArgs.Cancel in Window.CloseCore on shutdown (and document it) for the secondary windows, effectively closing them (and matching WPF)?

Currently, the extra windows won't ever have a chance of running any cleanup code.

src/Avalonia.Controls/ApplicationLifetimes/ClassicDesktopStyleApplicationLifetime.cs Show resolved Hide resolved
@maxkatz6
Copy link
Member Author

maxkatz6 commented Sep 30, 2024 •
edited
Loading

What do you think about ignoring WindowClosingEventArgs.Cancel in Window.CloseCore on shutdown (and document it) for the secondary windows, effectively closing them (and matching WPF)?

Currently, the extra windows won't ever have a chance of running any cleanup code.

But that's how it works in this PR right now.
I.e. secondary window also gets Closing event raised, but Cancel value is ignored.
Which is actually different from WPF - only MainWindow gets this event raised there.

Sorry, somehow I missed these review comments before.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0052318-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@MrJul
Copy link
Member

MrJul commented Oct 1, 2024

But that's how it works in this PR right now.
I.e. secondary window also gets Closing event raised, but Cancel value is ignored.
Which is actually different from WPF - only MainWindow gets this event raised there.

Sorry, I wasn't very clear.
It's Closed that isn't being raised for the secondary windows with this PR, preventing them from running any save/cleanup code they might need.

(My initial comment mentioned ignoring Cancel in CloseCore which I meant as "we should still close the window".)

AFAICT, in WPF, both Closing and Closed are raised for all windows when ShutdownMode == OnMainWindowClose.

@maxkatz6 maxkatz6 marked this pull request as draft October 2, 2024 22:52
@maxkatz6 maxkatz6 added the backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch label Oct 3, 2024
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052561-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 marked this pull request as ready for review October 11, 2024 02:35
@maxkatz6
Copy link
Member Author

@MrJul Window.Closed event is raised properly now.
I also changed behavior of Lifetime.Shutdown() method to match the same rules - execute Closed even though Closing was cancelled.

@maxkatz6 maxkatz6 requested a review from MrJul October 11, 2024 02:36
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052565-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@MrJul
Copy link
Member

MrJul commented Oct 11, 2024

@MrJul Window.Closed event is raised properly now.
I also changed behavior of Lifetime.Shutdown() method to match the same rules - execute Closed even though Closing was cancelled.

Nice! As a bonus I find the new code to be easier to follow :)

@MrJul MrJul added this pull request to the merge queue Oct 11, 2024
Merged via the queue into master with commit 7f2cd07 Oct 11, 2024
11 checks passed
@MrJul MrJul deleted the fix/desktop-lifetime-non-mainwindow-cancellation branch October 11, 2024 10:11
maxkatz6 added a commit that referenced this pull request Oct 27, 2024
@maxkatz6
Fix desktop lifetime non-mainwindow cancellation (#17059)
d41d5bb 
* Only check MainWindow visiblity in DoShutdown cancellation, when ShutdownMode == ShutdownMode.OnMainWindowClose

* Raise WindowClosedEvent event AFTER IsVisible/_shown properties were updated

* Add OnMainWindowClose cancellation tests

* Assert that Closing event was actually raised.

* Re-do fix by forcing window closing

* Forced .Shutdown() should also raise Window.Closed events, and not ignore them
@maxkatz6 maxkatz6 added backported-11.2.x and removed backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch labels Oct 27, 2024
@heldergoncalves92 heldergoncalves92 mentioned this pull request Nov 20, 2024
Open
grokys added a commit that referenced this pull request Nov 20, 2024
@grokys
Revert order of Closed and WindowClosed events.
d33efb7 
They were swapped in #17059 which caused problems for Outsystems. Temporarily reverting this for them until we decide what to do long-term.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrJul MrJul MrJul approved these changes

Assignees
No one assigned
Labels
backported-11.2.x bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maxkatz6 @avaloniaui-bot @MrJul