Override BitcoinApplication::event() to handle QEvent::Quit #547
Conversation
|
tACK e7fc506 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 |
|
|
||
| bool BitcoinApplication::event(QEvent* e) | ||
| { | ||
| if (e->type() == QEvent::Quit) { |
There was a problem hiding this comment.
Is there a reason QEvent::Quit is undocumented (and apparently unused in anything else Google finds)?
There was a problem hiding this comment.
Would be nice if there's a way we can fix this without undocumented (undefined?) behaviour.
Worst case, partially reverting #336 instead? :/
There was a problem hiding this comment.
Is there a reason
QEvent::Quitis 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?
There was a problem hiding this comment.
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
…andle QEvent::Quit e7fc506 qt: Override BitcoinApplication::event() to handle QEvent::Quit (Hennadii Stepanov) Pull request description: 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. ACKs for top commit: RandyMcMillan: tACK e7fc506 Sjors: tACK e7fc506 (rebased on master) indeed fixes the crash described in #545 promag: Tested ACK e7fc506 on macOS 10.15 with Qt 5.15.2. Tree-SHA512: 236a483dc0828f22999469e133b8ac9f0b6267ec2a27004c3ebaa967689ddb972ea1fa90c1dd41f3bff3d17bf571a707babcef53bd79fd711fda98cfbf120131
…uit()` with `QCoreApplication::exit(0)` 252f363 qt: Replace `QCoreApplication::quit()` with `QCoreApplication::exit(0)` (Hennadii Stepanov) Pull request description: ### Qt 5: - no behavior change. See https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/kernel/qcoreapplication.cpp?h=5.15#n2012: ```cpp void QCoreApplication::quit() { exit(0); } ``` ### Qt 6: - this change avoids sending a duplicated `QEvent::Quit` We use `QEvent::Quit` to [handle](#547) macOS dock menu events. Qt 6 uses `QEvent::Quit` more [widely](qt/qtbase@89f7a27). We do not want a duplicated `QEvent::Quit` which fires `Assert(node.args);` in the [`Shutdown()`](https://github.com/bitcoin-core/gui/blob/d1b3dfb275fd98e37cfe8a0f7cea7d03595af2e8/src/init.cpp#L200) function. ACKs for top commit: promag: Code review ACK 252f363. Tree-SHA512: 6a04cbcf523c0375158a59b29afadf18da99738c8db8b8728f99319a8cdc10806d2f06dc5a7d3b8b0e1a5f1711be778a75d4ecdefef7cf66e26ae2848f7f57db
5 participants
#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 #545.