QMessageBox: Respect explicit accept/reject after closing dialog

If the dialog is closed by pressing a button, the button will be
reflected via clickedButton(), and the result() will reflect either
the QMessageBox::StandardButton value, or an opaque value for custom
buttons.

Depending on the role if the buttons, the accepted or rejected
signals are emitted.

If the user called accept() or rejecct() on a dialog that had
already been closed by a button, we would as a result of
1f70c073d4 emit a signal based
on the original button that was clicked, instead of respecting
the newly triggered function.

It's a questionable use-case, as the clickedButton() is still
the original button e.g., but we should still avoid regressing
this, so we now emit the signals based on the newly stored
result code instead of using the clicked button as the source
of truth.

To allow this we had to change the opaque result() value for
custom buttons to stay out of the QDialog::DialogCode enum,
but this should be fine as the documentation explicitly says
that this is an opaque value.

Fixes: QTBUG-118226
Pick-to: 6.7 6.6 6.5
Change-Id: Ia2966cecc6694efce66493c401854402658332b4
Reviewed-by: Axel Spoerl <axel.spoerl@qt.io>

src/widgets/dialogs/qmessagebox.cpp

@@ -447,20 +447,34 @@ static int oldButton(int button)
 
 int QMessageBoxPrivate::execReturnCode(QAbstractButton *button)
 {
-    int ret = buttonBox->standardButton(button);
-    if (ret == QMessageBox::NoButton) {
-        ret = customButtonList.indexOf(button); // if button == 0, correctly sets ret = -1
-    } else if (compatMode) {
-        ret = oldButton(ret);
+    if (int standardButton = buttonBox->standardButton(button)) {
+        // When using a QMessageBox with standard buttons, the return
+        // code is a StandardButton value indicating the standard button
+        // that was clicked.
+        if (compatMode)
+            return oldButton(standardButton);
+        else
+            return standardButton;
+    } else {
+        // When using QMessageBox with custom buttons, the return code
+        // is an opaque value, and the user is expected to use clickedButton()
+        // to determine which button was clicked. We make sure to keep the opaque
+        // value out of the QDialog::DialogCode range, so we can distinguish them.
+        auto customButtonIndex = customButtonList.indexOf(button);
+        if (customButtonIndex >= 0)
+            return QDialog::DialogCode::Accepted + customButtonIndex + 1;
+        else
+            return customButtonIndex; // Not found, return -1
     }
-    return ret;
 }
 
 int QMessageBoxPrivate::dialogCode() const
 {
     Q_Q(const QMessageBox);
 
-    if (clickedButton) {
+    if (rescode <= QDialog::Accepted) {
+        return rescode;
+    } else if (clickedButton) {
         switch (q->buttonRole(clickedButton)) {
         case QMessageBox::AcceptRole:
         case QMessageBox::YesRole:

tests/auto/widgets/dialogs/qmessagebox/tst_qmessagebox.cpp

@@ -62,6 +62,8 @@ private slots:
 
     void hideNativeByDestruction();
 
+    void explicitDoneAfterButtonClicked();
+
     void cleanup();
 };
 
@@ -437,7 +439,7 @@ void tst_QMessageBox::shortcut()
     msgBox.addButton("&Maybe", QMessageBox::YesRole);
     ExecCloseHelper closeHelper;
     closeHelper.start(Qt::Key_M, &msgBox);
-    QCOMPARE(msgBox.exec(), 2);
+    QCOMPARE(msgBox.exec(), 4);
 }
 #endif
 
@@ -509,7 +511,7 @@ QT_WARNING_DISABLE_DEPRECATED
     // the button text versions
     closeHelper.start(Qt::Key_Enter);
     ret = QMessageBox::information(nullptr, "title", "text", "Yes", "No", QString(), 1);
-    COMPARE(ret, 1);
+    COMPARE(ret, 3); // Custom button opaque result
     QVERIFY(closeHelper.done());
 #endif // QT_DEPRECATED_SINCE(6, 2)
 #undef COMPARE
@@ -538,9 +540,9 @@ void tst_QMessageBox::instanceSourceCompat()
 #ifndef Q_OS_MAC
     // mnemonics are not used on OS X
     closeHelper.start(QKeyCombination(Qt::ALT | Qt::Key_R).toCombined(), &mb);
-    QCOMPARE(mb.exec(), 0);
+    QCOMPARE(mb.exec(), 2);
     closeHelper.start(QKeyCombination(Qt::ALT | Qt::Key_Z).toCombined(), &mb);
-    QCOMPARE(mb.exec(), 1);
+    QCOMPARE(mb.exec(), 3);
 #endif
 }
 
@@ -815,5 +817,51 @@ void tst_QMessageBox::hideNativeByDestruction()
     QVERIFY(QTest::qWaitFor(windowActive));
 }
 
+void tst_QMessageBox::explicitDoneAfterButtonClicked()
+{
+    QMessageBox msgBox;
+    auto *standardButton = msgBox.addButton(QMessageBox::Ok);
+    auto *customButton = msgBox.addButton("Custom", QMessageBox::RejectRole);
+
+    QSignalSpy acceptedSpy(&msgBox, &QDialog::accepted);
+    QSignalSpy rejectedSpy(&msgBox, &QDialog::rejected);
+
+    msgBox.setDefaultButton(standardButton);
+    ExecCloseHelper closeHelper;
+    closeHelper.start(Qt::Key_Enter, &msgBox);
+    msgBox.exec();
+    QCOMPARE(msgBox.clickedButton(), standardButton);
+    QCOMPARE(msgBox.result(), QMessageBox::Ok);
+    QCOMPARE(acceptedSpy.size(), 1);
+    QCOMPARE(rejectedSpy.size(), 0);
+
+    msgBox.accept();
+    QCOMPARE(msgBox.result(), QDialog::Accepted);
+    QCOMPARE(acceptedSpy.size(), 2);
+    QCOMPARE(rejectedSpy.size(), 0);
+    msgBox.reject();
+    QCOMPARE(msgBox.result(), QDialog::Rejected);
+    QCOMPARE(acceptedSpy.size(), 2);
+    QCOMPARE(rejectedSpy.size(), 1);
+
+    msgBox.setDefaultButton(customButton);
+    closeHelper.start(Qt::Key_Enter, &msgBox);
+    msgBox.exec();
+    QCOMPARE(msgBox.clickedButton(), customButton);
+    QVERIFY(msgBox.result() != QDialog::Accepted);
+    QVERIFY(msgBox.result() != QDialog::Rejected);
+    QCOMPARE(acceptedSpy.size(), 2);
+    QCOMPARE(rejectedSpy.size(), 2);
+
+    msgBox.accept();
+    QCOMPARE(msgBox.result(), QDialog::Accepted);
+    QCOMPARE(acceptedSpy.size(), 3);
+    QCOMPARE(rejectedSpy.size(), 2);
+    msgBox.reject();
+    QCOMPARE(msgBox.result(), QDialog::Rejected);
+    QCOMPARE(acceptedSpy.size(), 3);
+    QCOMPARE(rejectedSpy.size(), 3);
+}
+
 QTEST_MAIN(tst_QMessageBox)
 #include "tst_qmessagebox.moc"