Override BitcoinApplication::event() to handle QEvent::Quit #547

1. 
   hebasto commented at 4:02 pm on February 12, 2022: member

   bitcoin-core/gui#336 introduced a regression when termination requests from a platform are not handled properly.

   This PR fixes this regression. On macOS shutdown after clicking “Quit” in Dock icon menu, and during logout works again.

   Fixes bitcoin-core/gui#545.

2. qt: Override BitcoinApplication::event() to handle QEvent::Quit e7fc50681e
3. hebasto added the label Bug on Feb 12, 2022
4. hebasto added the label UX on Feb 12, 2022
5. hebasto added this to the milestone 23.0 on Feb 12, 2022
6. hebasto requested review from jarolrod on Feb 12, 2022
7. hebasto requested review from promag on Feb 12, 2022
8. hebasto requested review from Sjors on Feb 12, 2022
9. 
   RandyMcMillan commented at 1:01 am on February 14, 2022: contributor

   tACK e7fc50681e99e3c726db2bc4d3d425ed8a0fc6b3

   Tested on MacOS Darwin ₿ 19.6.0 Darwin Kernel Version 19.6.0: Sun Nov 14 19:58:51 PST 2021; root:xnu-6153.141.50~1/RELEASE_X86_64 x86_64

   Tested on MacOS Darwin m1.deepspace.host 21.3.0 Darwin Kernel Version 21.3.0: Wed Jan 5 21:37:58 PST 2022; root:xnu-8019.80.24~20/RELEASE_ARM64_T8101 arm64

10. Sjors approved
11. 
    Sjors commented at 7:18 pm on February 15, 2022: member

    tACK e7fc50681e99e3c726db2bc4d3d425ed8a0fc6b3 (rebased on master) indeed fixes the crash described in #545
12. in src/qt/bitcoin.cpp:455 in e7fc50681e

    449@@ -450,6 +450,16 @@ WId BitcoinApplication::getMainWinId() const
    450     return window->winId();
    451 }
    452 
    453+bool BitcoinApplication::event(QEvent* e)
    454+{
    455+    if (e->type() == QEvent::Quit) {
    

    ---

    ---

    

    luke-jr commented at 2:36 am on February 20, 2022:

    Is there a reason QEvent::Quit is undocumented (and apparently unused in anything else Google finds)?

    ---

    

    hebasto commented at 9:28 am on February 20, 2022:

    ~I don’t know.~ Edit: see #547 (review)

    ---

    

    luke-jr commented at 4:58 pm on February 20, 2022:

    Would be nice if there’s a way we can fix this without undocumented (undefined?) behaviour.

    Worst case, partially reverting #336 instead? :/

    ---

    

    hebasto commented at 7:36 pm on February 20, 2022:

    Is there a reason QEvent::Quit is undocumented (and apparently unused in anything else Google finds)?

    Would be nice if there’s a way we can fix this without undocumented (undefined?) behaviour.

    Qt codease, as every other codebase, is not perfect. And if documentation diverges from code, it is an bug in documentation (https://bugreports.qt.io/browse/QTBUG-92122) which has already been fixed but not backported.

    Worst case, partially reverting #336 instead? :/

    Yes, this solution is also possible. Only there are two concerns:

    • do we really want to return to undocumented way to interrupt the main event loop during application run?
    • is it feasible for 23.0 considering release schedule?

    ---

    

    luke-jr commented at 9:32 pm on February 20, 2022:

    it is an bug in documentation (https://bugreports.qt.io/browse/QTBUG-92122) which has already been fixed but not backported.

    Ok, seems fine if that’s all it is

13. bitcoin-core deleted a comment on Feb 20, 2022
14. promag approved
15. 
    promag commented at 11:21 pm on February 21, 2022: contributor

    Tested ACK e7fc50681e99e3c726db2bc4d3d425ed8a0fc6b3 on macOS 10.15 with Qt 5.15.2.
16. hebasto merged this on Feb 22, 2022
17. hebasto closed this on Feb 22, 2022

    ---

18. hebasto deleted the branch on Feb 22, 2022
19. sidhujag referenced this in commit 703daf767e on Feb 22, 2022
20. hebasto referenced this in commit dfe11a1a7e on May 23, 2022
21. bitcoin-core locked this on Feb 22, 2023

Contributors
hebasto RandyMcMillan Sjors luke-jr promag

Review Requested
jarolrod

Labels
Bug UX

Milestone
23.0