ACK 70e4706

From #16348:

Once we bump Qt to at least 5.10 these can be refactored to use the invokeMethod overload that allows connecting to lambdas or member pointers, which are compile checked.

From #13478:

Qt Versions:

5.12 (LTS) Supported for 3 years post release.
5.11 Supported until May 2019
5.10 Supported until Dec 2018 (Archived).
5.9 (LTS) Supported until May 31, 2020.
5.6 (LTS) Supported until Mar. 16, 2019 (will end around the same time as the v0.18.0 release).

Any other releases older than 5.10 are no longer supported (by Qt).

Qt releases seem to be getting more frequent, and in some cases more aggressive about dropping support for OS versions. i.e macOS >10.12 is required for Qt 5.12.

It might be a good idea to bump the minimum version to 5.12 after the 0.20 branch-off. Alternatively we could duplicate the code with #IF QT_VERSION >= 0x051000 with the modern syntax, so that the compiler will catch an accidental refactor bug like this.

I think we should fix ThreadSafeMessageBox instead. Note that if modal == false Qt::QueuedConnection is used and a pointer to the local ret is passed, which goes out of scope. So by the time slot message is called it dereferences an invalid pointer. Please correct me if I'm wrong.

I think we should fix ThreadSafeMessageBox instead. Note that if modal == false Qt::QueuedConnection is used and a pointer to the local ret is passed, which goes out of scope. So by the time slot message is called it dereferences an invalid pointer. Please correct me if I'm wrong.

QMetaObject::invokeMethod() passes 4 arguments to BitcoinGUI::message() with 3 parameters signature.

@hebasto I mean that if we merge this then the problem above exists.

@promag

Note that if modal == false Qt::QueuedConnection is used and a pointer to the local ret is passed, which goes out of scope. So by the time slot message is called it dereferences an invalid pointer.

Indeed.

I think we should fix ThreadSafeMessageBox instead.

I found the only place where return value is used:

Could we replace our custom ThreadSafeMessageBox with standard QMessageBox here?

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #17966 (qt, refactor: Optimize signal-slot connections logic by hebasto)
• #17937 (gui: Remove WalletView and BitcoinGUI circular dependency by promag)
• #16224 (gui: Bilingual GUI error messages by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

I'm happy this was discovered before a release! Please do be careful with cleanup changes like this.

@promag how about this patch to master:

diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
index 6043e93f9..ea13b60c2 100644
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -1025,7 +1025,7 @@ void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate, double nVer
     progressBar->setToolTip(tooltip);
 }
 
-void BitcoinGUI::message(const QString& title, QString message, unsigned int style)
+bool BitcoinGUI::message(const QString& title, QString message, unsigned int style)
 {
     // Default title. On macOS, the window title is ignored (as required by the macOS Guidelines).
     QString strTitle{PACKAGE_NAME};
@@ -1079,9 +1079,10 @@ void BitcoinGUI::message(const QString& title, QString message, unsigned int sty
         showNormalIfMinimized();
         QMessageBox mBox(static_cast<QMessageBox::Icon>(nMBoxIcon), strTitle, message, buttons, this);
         mBox.setTextFormat(Qt::PlainText);
-        mBox.exec();
+        return mBox.exec() == QMessageBox::Ok;
     } else {
         notificator->notify(static_cast<Notificator::Class>(nNotifyIcon), strTitle, message);
+        return false;
     }
 }
 
@@ -1362,10 +1363,10 @@ static bool ThreadSafeMessageBox(BitcoinGUI* gui, const std::string& message, co
     // In case of modal message, use blocking connection to wait for user to click a button
     bool invoked = QMetaObject::invokeMethod(gui, "message",
                                modal ? GUIUtil::blockingGUIThreadConnection() : Qt::QueuedConnection,
+                               Q_RETURN_ARG(bool, ret),
                                Q_ARG(QString, QString::fromStdString(caption)),
                                Q_ARG(QString, QString::fromStdString(message)),
-                               Q_ARG(unsigned int, style),
-                               Q_ARG(bool*, &ret));
+                               Q_ARG(unsigned int, style));
     assert(invoked);
     return ret;
 }
diff --git a/src/qt/bitcoingui.h b/src/qt/bitcoingui.h
index 45fbb03aa..53c9e750b 100644
--- a/src/qt/bitcoingui.h
+++ b/src/qt/bitcoingui.h
@@ -219,8 +219,9 @@ public Q_SLOTS:
        @param[in] message   the displayed text
        @param[in] style     modality and style definitions (icon and used buttons - buttons only for message boxes)
                             @see CClientUIInterface::MessageBoxFlags
+       @return True if Ok was clicked (modal only)
     */
-    void message(const QString& title, QString message, unsigned int style);
+    bool message(const QString& title, QString message, unsigned int style);
 
 #ifdef ENABLE_WALLET
     void setCurrentWallet(WalletModel* wallet_model);

From Qt docs:

If the invocation is asynchronous, the return value cannot be evaluated.

Also I did not find any ThreadSafeMessageBox() invocation without CClientUIInterface::MODAL set.

Thanks for fixing.
Sorry for not testing #17943.
I think GUI refactoring should not be taken too easy since there are little tests that would detect possible new issues.

I think GUI refactoring should not be taken too easy since there are little tests that would detect possible new issues.

Agree.

I reverted the commits from this PR and got exactly the same.
ACK 70e4706

Post-hoc ACK 70e4706

And partial mea culpa: if I had looked more closely at 35ecf85 which I cited here I would have seen that it had been moved and retained and continued to follow it to its current location.