Getting ready to Qt 6 (6/n). Replace `QCoreApplication::quit()` with `QCoreApplication::exit(0)` #586

pull hebasto wants to merge 1 commits into bitcoin-core:master from hebasto:220416-quit changing 2 files +4 −2

hebasto commented at 2:00 PM on April 16, 2022: member
Qt 5:
- no behavior change.
See https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/kernel/qcoreapplication.cpp?h=5.15#n2012:
```
void QCoreApplication::quit()
{
    exit(0);
}
```
Qt 6:
- this change avoids sending a duplicated QEvent::Quit
We use QEvent::Quit to handle macOS dock menu events. Qt 6 uses QEvent::Quit more widely. We do not want a duplicated QEvent::Quit which fires Assert(node.args); in the Shutdown() function.
hebasto added the label Qt 6 on Apr 16, 2022
hebasto commented at 2:28 PM on April 16, 2022: member
Just for a quick reference, here is a (full?) list of ways to quit the GUI:
1. Close the splash screen window
2. Press q on the splash screen
3. Close the main window (if the "Minimize on close" option is disabled)
4. Menu File --> Exit. On macOS use an application menu
5. Shortcut Ctrl+Q
6. System tray icon menu Exit. On macOS use a Dock icon menu
7. Run stop command in the Console tab
8. Receive the stop RPC command (if the "Enable RPC server" option is checked)
9. Press "Reset Options" button in the Options tab
10. Ctrl+C in a shell which runs bitcoin-qt
w0xlt approved
w0xlt commented at 2:43 PM on April 19, 2022: contributor

tACK https://github.com/bitcoin-core/gui/pull/586/commits/4717ebe71d6c8dad04be51ef00021a8586e2424f on Ubuntu 21.10, Qt 5.15.2.

Tested with all options mentioned in #586 (comment)
hebasto force-pushed on Apr 20, 2022
hebasto commented at 11:54 AM on April 20, 2022: member
Updated 4717ebe71d6c8dad04be51ef00021a8586e2424f -> e0930ad770623ffad9a83d99846d6cfc20a52e15 (pr586.01 -> pr586.02, diff):
- added the same changes into the tests
in src/qt/bitcoin.cpp:311 in e0930ad770 outdated
```
 307 | @@ -308,7 +308,7 @@ void BitcoinApplication::startThread()
 308 |  
 309 |      /*  communication to and from thread */
 310 |      connect(&m_executor.value(), &InitExecutor::initializeResult, this, &BitcoinApplication::initializeResult);
 311 | -    connect(&m_executor.value(), &InitExecutor::shutdownResult, this, &QCoreApplication::quit);
 312 | +    connect(&m_executor.value(), &InitExecutor::shutdownResult, [] { QCoreApplication::exit(0); });
```
promag commented at 2:30 PM on May 21, 2022:

The signal shutdownResult is emitted from executor's thread and this results in calling exit() in that thread because the connection lacks a context. But exit() must be called in the main thread: https://doc-snapshots.qt.io/qt6-dev/qcoreapplication.html#exit. Maybe:

connect(&m_executor.value(), &InitExecutor::shutdownResult, this, [] { QCoreApplication::exit(0); });
hebasto commented at 2:46 PM on May 21, 2022:

~Why? QCoreApplication::exit() is a static member function.~

You are right.

promag commented at 2:53 PM on May 21, 2022:

Yes, but according to the doc

Note also that this function is not thread-safe. It should be called only from the main thread

and by using https://doc.qt.io/qt-5/qobject.html#connect-5 we ensure that lambda is called on the right thread.

hebasto commented at 3:01 PM on May 21, 2022:

Thanks! Updated.

qt: Replace `QCoreApplication::quit()` with `QCoreApplication::exit(0)`

Qt 5:
 - no behavior change

Qt 6:
 - this change avoids sending a duplicated `QEvent::Quit`

252f363f2f

hebasto force-pushed on May 21, 2022
hebasto commented at 3:00 PM on May 21, 2022: member
Updated e0930ad770623ffad9a83d99846d6cfc20a52e15 -> 252f363f2feff243cae47731d59dfa1b74dd4386 (pr586.02 -> pr586.03, diff):
- addressed @promag's comment
promag approved
promag commented at 8:38 PM on May 22, 2022: contributor

Code review ACK 252f363f2feff243cae47731d59dfa1b74dd4386.
hebasto merged this on May 23, 2022
hebasto closed this on May 23, 2022
hebasto deleted the branch on May 23, 2022
sidhujag referenced this in commit 98b8450b60 on May 28, 2022
bitcoin-core locked this on May 23, 2023

Contributors

Labels

Linked (view graph)

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