wxOSX perform cleanup when applicationWillTerminate: is called (PR #26542)
Ryan Ogurek
In the handling of OSXOnWillTerminate(), the TLW is Close()d, but not deleted (the pending cleanup never occurs), additionally, OnExit() is never called.
When clicking "Quit" from the Dock, Cocoa ends up in -[NSApplication terminate:], which calls exit(0) more or less immediately after sending applicationWillTerminate:, and the documentation reads:
Don’t bother to put final cleanup code in your app’s main() function—it will never be executed. If cleanup is necessary, perform that cleanup in the delegate’s applicationWillTerminate: method.
So then, this PR changes wxOSX's wxApp::OnEndSession() to do cleanup similarly to the wxMSW port, making sure that OnExit() and all other cleanup is performed (part of that cleanup is deleting TLWs, so I've removed the Close()).
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26542
Commit Summary
- b871033 wxOSX perform cleanup when applicationWillTerminate: is called
File Changes
(1 file)
- M src/osx/carbon/app.cpp (12)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26542.patch
- https://github.com/wxWidgets/wxWidgets/pull/26542.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz approved this pull request.
Thanks, assuming the comment is correct about there being no better place to do this, this looks the right thing to do to me.
Of course, as always, I'd prefer @csomor's confirmation before merging this if possible.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Stefan Csomor
csomor left a comment (wxWidgets/wxWidgets#26542)Actually, the sequence of calling all the app callbacks was once preserved. I'll have to investigate.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Stefan Csomor
csomor left a comment (wxWidgets/wxWidgets#26542)Ok, some flows are not the way they used to be, the system does not emulate a quit-app apple event anymore, but still putting these things in wxApp::OnEndSession seems too general to me, I'd rather add
wxTheApp->CallOnExit();
wxEntryCleanup();
at the end of - (void)applicationWillTerminate: directly, since for this we are given the contract, that exit will soon be called
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
ryancog left a comment (wxWidgets/wxWidgets#26542)@csomor Would you still want to keep the TLW Close in OnEndSession()? I’m not sure what problem is solved having that there…
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Stefan Csomor
csomor left a comment (wxWidgets/wxWidgets#26542)@ryancog yes, please leave it in there, although the LTW should be closed from within OnQueryEndSession, if there is a veto from the TWL it won't. So I'd still like to leave it in there in case there is an overrule and the system still calls applicationWillTerminate, I prefer to be defensive here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
@ryancog pushed 1 commit.
- ba22353 wxOSX perform cleanup when applicationWillTerminate: is called
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
ryancog left a comment (wxWidgets/wxWidgets#26542)Alright, moved to that to applicationWillTerminate:, then.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26542)@csomor Is this ok to merge now?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Stefan Csomor
csomor left a comment (wxWidgets/wxWidgets#26542)@csomor Is this ok to merge now?
@vadz yes, thanks a lot @ryancog
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26542)Will push to master in a moment, but I think this might be also worth applying to 3.2, right?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Stefan Csomor
csomor left a comment (wxWidgets/wxWidgets#26542)Will push to master in a moment, but I think this might be also worth applying to 3.2, right?
yes, definitely, thanks
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26542)Oops, backporting it to 3.2 didn't work because it doesn't have CallOnExit() — and the commit adding it can't be backported because it adds a virtual function, so I'm backing the backport out.
If somebody can create a version of this change appropriate for 3.2 before 3.2.11 release, it would be great, but I won't be able to do it, sorry.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Stefan Csomor
csomor left a comment (wxWidgets/wxWidgets#26542)@vadz the additional methods used in CallOnExit do not exist yet in 3.2, so I'd suggest doing the same as wxApp::OnEndSession does for MSW in 3.2
just call wxApp->OnExit() instead of wxApp-> CallOnExit() there ? and leave the wxEntryCleanup() unchanged
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26542)CallOnExit() was introduce to solve a problem with OnExit() being called with some TLWs still existing (among other things), and we'd run into this problem here, so I think we need to at least close all of them before calling it. But this really needs to be tested, which is why I can't/don't have time to do it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Stefan Csomor
csomor left a comment (wxWidgets/wxWidgets#26542)
CallOnExit()was introduce to solve a problem withOnExit()being called with some TLWs still existing (among other things), and we'd run into this problem here, so I think we need to at least close all of them before calling it. But this really needs to be tested, which is why I can't/don't have time to do it.
ok, thanks for the explanation
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.