Merge bitcoin-core/gui#336: Do not exit and re-enter main event loop …

…during shutdown

451ca24 qt, refactor: Drop intermediate BitcoinApplication::shutdownResult slot (Hennadii Stepanov)
f3a17bb qt: Do not exit and re-enter main event loop during shutdown (Hennadii Stepanov)
b4e0d2c qt, refactor: Allocate SendConfirmationDialog instances on heap (Hennadii Stepanov)
332dea2 qt, refactor: Keep HelpMessageDialog in the main event loop (Hennadii Stepanov)
c8bae37 qt, refactor: Keep PSBTOperationsDialog in the main event loop (Hennadii Stepanov)
7fa91e8 qt, refactor: Keep AskPassphraseDialog in the main event loop (Hennadii Stepanov)
6f6fde3 qt, refactor: Keep EditAddressDialog in the main event loop (Hennadii Stepanov)
59f7ba4 qt, refactor: Keep CoinControlDialog in the main event loop (Hennadii Stepanov)
7830cd0 qt, refactor: Keep OptionsDialog in the main event loop (Hennadii Stepanov)
13f6188 qt: Add GUIUtil::ShowModalDialogAndDeleteOnClose (Hennadii Stepanov)

Pull request description:

  On master (1ef34ee) during shutdown `QApplication` exits the main event loop, then re-enter again.

  This PR streamlines shutdown process by removing the need to interrupt the main event loop, that is required for #59.

  Also, blocking [`QDialog::exec()`](https://doc.qt.io/qt-5/qdialog.html#exec) calls are replaced with safer [`QDialog::show()`](https://doc.qt.io/qt-5/qwidget.html#show), except for `SendConfirmationDialog` as that change is not trivial (marked as TODO).

  The [`QDialog::open()`](https://doc.qt.io/qt-5/qdialog.html#open) was not used because the actual modality mode (application modal or window modal) of a dialog depends on whether it has a parent.

  This PR does not change behavior, and all touched dialogs are still application modal.
  As a follow up, a design research could suggest to make some dialogs window modal.

  NOTE for reviewers: quitting app while a dialog is open (e.g., via systray icon menu) must work fine.

ACKs for top commit:
  laanwj:
    Code review and lighly tested ACK 451ca24
  promag:
    ACK 451ca24, just changed signal to `quitRequested`.

Tree-SHA512: ef01ab6ed803b202e776019a4e1f592e816f7bc786e00574b25a0bf16be2374ddf9db21f0a26da08700df7ef0ab9e879550df46dcfe3b6d940f5ed02ca5f8447

src/qt/addressbookpage.cpp

@@ -185,14 +185,14 @@ void AddressBookPage::onEditAction()
     if(indexes.isEmpty())
         return;
 
-    EditAddressDialog dlg(
+    auto dlg = new EditAddressDialog(
         tab == SendingTab ?
         EditAddressDialog::EditSendingAddress :
         EditAddressDialog::EditReceivingAddress, this);
-    dlg.setModel(model);
+    dlg->setModel(model);
     QModelIndex origIndex = proxyModel->mapToSource(indexes.at(0));
-    dlg.loadRow(origIndex.row());
-    dlg.exec();
+    dlg->loadRow(origIndex.row());
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
 }
 
 void AddressBookPage::on_newAddress_clicked()

src/qt/bitcoin.cpp

@@ -58,6 +58,7 @@
 #include <QThread>
 #include <QTimer>
 #include <QTranslator>
+#include <QWindow>
 
 #if defined(QT_STATIC)
 #include <QtPlugin>
@@ -256,6 +257,8 @@ void BitcoinApplication::createOptionsModel(bool resetSettings)
 void BitcoinApplication::createWindow(const NetworkStyle *networkStyle)
 {
     window = new BitcoinGUI(node(), networkStyle, nullptr);
+    connect(window, &BitcoinGUI::quitRequested, this, &BitcoinApplication::requestShutdown);
+
     pollShutdownTimer = new QTimer(window);
     connect(pollShutdownTimer, &QTimer::timeout, window, &BitcoinGUI::detectShutdown);
 }
@@ -290,7 +293,7 @@ void BitcoinApplication::startThread()
 
     /*  communication to and from thread */
     connect(&m_executor.value(), &InitExecutor::initializeResult, this, &BitcoinApplication::initializeResult);
-    connect(&m_executor.value(), &InitExecutor::shutdownResult, this, &BitcoinApplication::shutdownResult);
+    connect(&m_executor.value(), &InitExecutor::shutdownResult, this, &QCoreApplication::quit);
     connect(&m_executor.value(), &InitExecutor::runawayException, this, &BitcoinApplication::handleRunawayException);
     connect(this, &BitcoinApplication::requestedInitialize, &m_executor.value(), &InitExecutor::initialize);
     connect(this, &BitcoinApplication::requestedShutdown, &m_executor.value(), &InitExecutor::shutdown);
@@ -321,13 +324,17 @@ void BitcoinApplication::requestInitialize()
 
 void BitcoinApplication::requestShutdown()
 {
+    for (const auto w : QGuiApplication::topLevelWindows()) {
+        w->hide();
+    }
+
     // Show a simple window indicating shutdown status
     // Do this first as some of the steps may take some time below,
     // for example the RPC console may still be executing a command.
     shutdownWindow.reset(ShutdownWindow::showShutdownWindow(window));
 
     qDebug() << __func__ << ": Requesting shutdown";
-    window->hide();
+
     // Must disconnect node signals otherwise current thread can deadlock since
     // no event loop is running.
     window->unsubscribeFromCoreSignals();
@@ -407,15 +414,10 @@ void BitcoinApplication::initializeResult(bool success, interfaces::BlockAndHead
         pollShutdownTimer->start(SHUTDOWN_POLLING_DELAY);
     } else {
         Q_EMIT splashFinished(); // Make sure splash screen doesn't stick around during shutdown
-        quit(); // Exit first main loop invocation
+        requestShutdown();
     }
 }
 
-void BitcoinApplication::shutdownResult()
-{
-    quit(); // Exit second main loop invocation after shutdown finished
-}
-
 void BitcoinApplication::handleRunawayException(const QString &message)
 {
     QMessageBox::critical(
@@ -745,8 +747,6 @@ int GuiMain(int argc, char* argv[])
 #if defined(Q_OS_WIN)
             WinShutdownMonitor::registerShutdownBlockReason(QObject::tr("%1 didn't yet exit safely…").arg(PACKAGE_NAME), (HWND)app.getMainWinId());
 #endif
-            app.exec();
-            app.requestShutdown();
             app.exec();
             rv = app.getReturnValue();
         } else {

src/qt/bitcoin.h

@@ -60,8 +60,6 @@ class BitcoinApplication: public QApplication
 
     /// Request core initialization
     void requestInitialize();
-    /// Request core shutdown
-    void requestShutdown();
 
     /// Get process return value
     int getReturnValue() const { return returnValue; }
@@ -73,7 +71,8 @@ class BitcoinApplication: public QApplication
 
 public Q_SLOTS:
     void initializeResult(bool success, interfaces::BlockAndHeaderTipInfo tip_info);
-    void shutdownResult();
+    /// Request core shutdown
+    void requestShutdown();
     /// Handle runaway exceptions. Shows a message box with the problem and quits the program.
     void handleRunawayException(const QString &message);
 

src/qt/bitcoingui.cpp

@@ -475,7 +475,7 @@ void BitcoinGUI::createActions()
     m_mask_values_action->setStatusTip(tr("Mask the values in the Overview tab"));
     m_mask_values_action->setCheckable(true);
 
-    connect(quitAction, &QAction::triggered, qApp, QApplication::quit);
+    connect(quitAction, &QAction::triggered, this, &BitcoinGUI::quitRequested);
     connect(aboutAction, &QAction::triggered, this, &BitcoinGUI::aboutClicked);
     connect(aboutQtAction, &QAction::triggered, qApp, QApplication::aboutQt);
     connect(optionsAction, &QAction::triggered, this, &BitcoinGUI::optionsClicked);
@@ -1097,8 +1097,8 @@ void BitcoinGUI::aboutClicked()
     if(!clientModel)
         return;
 
-    HelpMessageDialog dlg(this, HelpMessageDialog::about);
-    dlg.exec();
+    auto dlg = new HelpMessageDialog(this, HelpMessageDialog::about);
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
 }
 
 void BitcoinGUI::showDebugWindow()
@@ -1353,13 +1353,14 @@ void BitcoinGUI::openOptionsDialogWithTab(OptionsDialog::Tab tab)
     if (!clientModel || !clientModel->getOptionsModel())
         return;
 
-    OptionsDialog dlg(this, enableWallet);
-    dlg.setCurrentTab(tab);
-    dlg.setModel(clientModel->getOptionsModel());
-    connect(&dlg, &OptionsDialog::appearanceChanged, [=]() {
+    auto dlg = new OptionsDialog(this, enableWallet);
+    connect(dlg, &OptionsDialog::quitOnReset, this, &BitcoinGUI::quitRequested);
+    dlg->setCurrentTab(tab);
+    dlg->setModel(clientModel->getOptionsModel());
+    connect(dlg, &OptionsDialog::appearanceChanged, [=]() {
         updateWidth();
     });
-    dlg.exec();
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
 
     updateCoinJoinVisibility();
 }
@@ -1703,7 +1704,7 @@ void BitcoinGUI::closeEvent(QCloseEvent *event)
             // close rpcConsole in case it was open to make some space for the shutdown window
             rpcConsole->close();
 
-            QApplication::quit();
+            Q_EMIT quitRequested();
         }
         else
         {
@@ -1997,7 +1998,7 @@ void BitcoinGUI::detectShutdown()
     {
         if(rpcConsole)
             rpcConsole->hide();
-        qApp->quit();
+        Q_EMIT quitRequested();
     }
 }
 

src/qt/bitcoingui.h

@@ -254,6 +254,7 @@ class BitcoinGUI : public QMainWindow
     void openOptionsDialogWithTab(OptionsDialog::Tab tab);
 
 Q_SIGNALS:
+    void quitRequested();
     /** Signal raised when a URI was entered or dragged to the GUI */
     void receivedURI(const QString &uri);
     /** Signal raised when RPC console shown */

src/qt/guiutil.cpp

@@ -42,6 +42,7 @@
 #include <QDateTime>
 #include <QDebug>
 #include <QDesktopServices>
+#include <QDialog>
 #include <QDialogButtonBox>
 #include <QDoubleValidator>
 #include <QFileDialog>
@@ -1949,4 +1950,11 @@ void PrintSlotException(
     PrintExceptionContinue(std::make_exception_ptr(exception), description.c_str());
 }
 
+void ShowModalDialogAndDeleteOnClose(QDialog* dialog)
+{
+    dialog->setAttribute(Qt::WA_DeleteOnClose);
+    dialog->setWindowModality(Qt::ApplicationModal);
+    dialog->show();
+}
+
 } // namespace GUIUtil

src/qt/guiutil.h

@@ -43,6 +43,7 @@ class QAbstractItemView;
 class QAction;
 class QButtonGroup;
 class QDateTime;
+class QDialog;
 class QFont;
 class QKeySequence;
 class QLineEdit;
@@ -609,6 +610,11 @@ namespace GUIUtil
             type);
     }
 
+    /**
+     * Shows a QDialog instance asynchronously, and deletes it on close.
+     */
+    void ShowModalDialogAndDeleteOnClose(QDialog* dialog);
+
 } // namespace GUIUtil
 
 #endif // BITCOIN_QT_GUIUTIL_H

src/qt/optionsdialog.cpp

@@ -406,7 +406,8 @@ void OptionsDialog::on_resetButton_clicked()
         /* reset all options and close GUI */
         model->Reset();
         model->resetSettingsOnShutdown = true;
-        QApplication::quit();
+        close();
+        Q_EMIT quitOnReset();
     }
 }
 

src/qt/optionsdialog.h

@@ -82,6 +82,7 @@ private Q_SLOTS:
 Q_SIGNALS:
     void appearanceChanged();
     void proxyIpChecks(QValidatedLineEdit *pUiProxyIp, uint16_t nProxyPort);
+    void quitOnReset();
 
 private:
     Ui::OptionsDialog *ui;

src/qt/sendcoinsdialog.cpp

@@ -476,9 +476,10 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
 
     const QString confirmation = model->wallet().privateKeysDisabled() ? tr("Confirm transaction proposal") : tr("Confirm send coins");
     const QString confirmButtonText = model->wallet().privateKeysDisabled() ? tr("Create Unsigned") : tr("Send");
-    SendConfirmationDialog confirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, confirmButtonText, this);
-    confirmationDialog.exec();
-    QMessageBox::StandardButton retval = static_cast<QMessageBox::StandardButton>(confirmationDialog.result());
+    auto confirmationDialog = new SendConfirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, confirmButtonText, this);
+    confirmationDialog->setAttribute(Qt::WA_DeleteOnClose);
+    // TODO: Replace QDialog::exec() with safer QDialog::show().
+    const auto retval = static_cast<QMessageBox::StandardButton>(confirmationDialog->exec());
 
     if(retval != QMessageBox::Yes)
     {
@@ -937,9 +938,9 @@ void SendCoinsDialog::coinControlFeatureChanged(bool checked)
 // Coin Control: button inputs -> show actual coin control dialog
 void SendCoinsDialog::coinControlButtonClicked()
 {
-    CoinControlDialog dlg(*m_coin_control, model, this);
-    dlg.exec();
-    coinControlUpdateLabels();
+    auto dlg = new CoinControlDialog(*m_coin_control, model);
+    connect(dlg, &QDialog::finished, this, &SendCoinsDialog::coinControlUpdateLabels);
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
 }
 
 // Coin Control: checkbox custom change address

src/qt/transactionview.cpp

@@ -510,22 +510,22 @@ void TransactionView::editLabel()
             // Determine type of address, launch appropriate editor dialog type
             QString type = modelIdx.data(AddressTableModel::TypeRole).toString();
 
-            EditAddressDialog dlg(
+            auto dlg = new EditAddressDialog(
                 type == AddressTableModel::Receive
                 ? EditAddressDialog::EditReceivingAddress
                 : EditAddressDialog::EditSendingAddress, this);
-            dlg.setModel(addressBook);
-            dlg.loadRow(idx);
-            dlg.exec();
+            dlg->setModel(addressBook);
+            dlg->loadRow(idx);
+            GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
         }
         else
         {
             // Add sending address
-            EditAddressDialog dlg(EditAddressDialog::NewSendingAddress,
+            auto dlg = new EditAddressDialog(EditAddressDialog::NewSendingAddress,
                 this);
-            dlg.setModel(addressBook);
-            dlg.setAddress(address);
-            dlg.exec();
+            dlg->setModel(addressBook);
+            dlg->setAddress(address);
+            GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
         }
     }
 }

src/qt/walletframe.cpp

@@ -286,10 +286,9 @@ void WalletFrame::gotoLoadPSBT(bool from_clipboard)
         return;
     }
 
-    PSBTOperationsDialog* dlg = new PSBTOperationsDialog(this, currentWalletModel(), clientModel);
+    auto dlg = new PSBTOperationsDialog(this, currentWalletModel(), clientModel);
     dlg->openWithPSBT(psbtx);
-    dlg->setAttribute(Qt::WA_DeleteOnClose);
-    dlg->exec();
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
 }
 
 void WalletFrame::encryptWallet()

src/qt/walletview.cpp

@@ -296,11 +296,10 @@ void WalletView::showOutOfSyncWarning(bool fShow)
 
 void WalletView::encryptWallet()
 {
-    AskPassphraseDialog dlg(AskPassphraseDialog::Encrypt, this);
-    dlg.setModel(walletModel);
-    dlg.exec();
-
-    Q_EMIT encryptionStatusChanged();
+    auto dlg = new AskPassphraseDialog(AskPassphraseDialog::Encrypt, this);
+    dlg->setModel(walletModel);
+    connect(dlg, &QDialog::finished, this, &WalletView::encryptionStatusChanged);
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
 }
 
 void WalletView::backupWallet()
@@ -325,19 +324,18 @@ void WalletView::backupWallet()
 
 void WalletView::changePassphrase()
 {
-    AskPassphraseDialog dlg(AskPassphraseDialog::ChangePass, this);
-    dlg.setModel(walletModel);
-    dlg.exec();
+    auto dlg = new AskPassphraseDialog(AskPassphraseDialog::ChangePass, this);
+    dlg->setModel(walletModel);
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
 }
 
 void WalletView::unlockWallet(bool fForMixingOnly)
 {
     // Unlock wallet when requested by wallet model
-    if (walletModel->getEncryptionStatus() == WalletModel::Locked || walletModel->getEncryptionStatus() == WalletModel::UnlockedForMixingOnly)
-    {
-        AskPassphraseDialog dlg(fForMixingOnly ? AskPassphraseDialog::UnlockMixing : AskPassphraseDialog::Unlock, this);
-        dlg.setModel(walletModel);
-        dlg.exec();
+    if (walletModel->getEncryptionStatus() == WalletModel::Locked || walletModel->getEncryptionStatus() == WalletModel::UnlockedForMixingOnly) {
+        auto dlg = new AskPassphraseDialog(AskPassphraseDialog::Unlock, this);
+        dlg->setModel(walletModel);
+        GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
     }
 }