Don't override QDialog::setVisible() to implement native dialogs

Clients who called the base-class implementation in QDialog would
as a result start hitting the canBeNativeDialog code path at the
start of QDialog::setVisible(), which would show the native dialog,
but without updating the QWidget visibility state.

To keep things 100% compatible, we shuffle the implementation of
QDialog::setVisible() into QDialogPrivate, which allows us to
override it in QMessageBoxPrivate and QErrorMessagePrivate.

The existing subclasses of QDialog that override setVisible have
been left as is, to not cause any unintended behavior change.

Change-Id: Icafe31a7b84a75049365e4e04b80492de08614d2
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
(cherry picked from commit e0bb9e81ab1a9d71f2893844ea82430467422e21)
Reviewed-by: Tor Arne Vestbø <tor.arne.vestbo@qt.io>

src/widgets/dialogs/qdialog.cpp

@@ -742,35 +742,41 @@ void QDialog::closeEvent(QCloseEvent *e)
 void QDialog::setVisible(bool visible)
 {
     Q_D(QDialog);
-    if (!testAttribute(Qt::WA_DontShowOnScreen) && d->canBeNativeDialog() && d->setNativeDialogVisible(visible))
+    d->setVisible(visible);
+}
+
+void QDialogPrivate::setVisible(bool visible)
+{
+    Q_Q(QDialog);
+    if (!q->testAttribute(Qt::WA_DontShowOnScreen) && canBeNativeDialog() && setNativeDialogVisible(visible))
         return;
 
     // We should not block windows by the invisible modal dialog
     // if a platform-specific dialog is implemented as an in-process
     // Qt window, because in this case it will also be blocked.
-    const bool dontBlockWindows = testAttribute(Qt::WA_DontShowOnScreen)
+    const bool dontBlockWindows = q->testAttribute(Qt::WA_DontShowOnScreen)
-            && d->styleHint(QPlatformDialogHelper::DialogIsQtWindow).toBool();
+            && styleHint(QPlatformDialogHelper::DialogIsQtWindow).toBool();
     Qt::WindowModality oldModality;
     bool wasModalitySet;
 
     if (dontBlockWindows) {
-        oldModality = windowModality();
+        oldModality = q->windowModality();
-        wasModalitySet = testAttribute(Qt::WA_SetWindowModality);
+        wasModalitySet = q->testAttribute(Qt::WA_SetWindowModality);
-        setWindowModality(Qt::NonModal);
+        q->setWindowModality(Qt::NonModal);
     }
 
     if (visible) {
-        if (testAttribute(Qt::WA_WState_ExplicitShowHide) && !testAttribute(Qt::WA_WState_Hidden))
+        if (q->testAttribute(Qt::WA_WState_ExplicitShowHide) && !q->testAttribute(Qt::WA_WState_Hidden))
             return;
 
-        QWidget::setVisible(visible);
+        q->QWidget::setVisible(visible);
 
         // Window activation might be prevented. We can't test isActiveWindow here,
         // as the window will be activated asynchronously by the window manager.
-        if (!testAttribute(Qt::WA_ShowWithoutActivating)) {
+        if (!q->testAttribute(Qt::WA_ShowWithoutActivating)) {
-            QWidget *fw = window()->focusWidget();
+            QWidget *fw = q->window()->focusWidget();
             if (!fw)
-                fw = this;
+                fw = q;
 
             /*
             The following block is to handle a special case, and does not
@@ -783,14 +789,14 @@ void QDialog::setVisible(bool visible)
             have to use [widget*]->setFocus() themselves...
             */
 #if QT_CONFIG(pushbutton)
-            if (d->mainDef && fw->focusPolicy() == Qt::NoFocus) {
+            if (mainDef && fw->focusPolicy() == Qt::NoFocus) {
                 QWidget *first = fw;
                 while ((first = first->nextInFocusChain()) != fw && first->focusPolicy() == Qt::NoFocus)
                     ;
-                if (first != d->mainDef && qobject_cast<QPushButton*>(first))
+                if (first != mainDef && qobject_cast<QPushButton*>(first))
-                    d->mainDef->setFocus();
+                    mainDef->setFocus();
             }
-            if (!d->mainDef && isWindow()) {
+            if (!mainDef && q->isWindow()) {
                 QWidget *w = fw;
                 while ((w = w->nextInFocusChain()) != fw) {
                     QPushButton *pb = qobject_cast<QPushButton *>(w);
@@ -808,37 +814,37 @@ void QDialog::setVisible(bool visible)
         }
 
 #if QT_CONFIG(accessibility)
-        QAccessibleEvent event(this, QAccessible::DialogStart);
+        QAccessibleEvent event(q, QAccessible::DialogStart);
         QAccessible::updateAccessibility(&event);
 #endif
 
     } else {
-        if (testAttribute(Qt::WA_WState_ExplicitShowHide) && testAttribute(Qt::WA_WState_Hidden))
+        if (q->testAttribute(Qt::WA_WState_ExplicitShowHide) && q->testAttribute(Qt::WA_WState_Hidden))
             return;
 
 #if QT_CONFIG(accessibility)
-        if (isVisible()) {
+        if (q->isVisible()) {
-            QAccessibleEvent event(this, QAccessible::DialogEnd);
+            QAccessibleEvent event(q, QAccessible::DialogEnd);
             QAccessible::updateAccessibility(&event);
         }
 #endif
 
         // Reimplemented to exit a modal event loop when the dialog is hidden.
-        QWidget::setVisible(visible);
+        q->QWidget::setVisible(visible);
-        if (d->eventLoop)
+        if (eventLoop)
-            d->eventLoop->exit();
+            eventLoop->exit();
     }
 
     if (dontBlockWindows) {
-        setWindowModality(oldModality);
+        q->setWindowModality(oldModality);
-        setAttribute(Qt::WA_SetWindowModality, wasModalitySet);
+        q->setAttribute(Qt::WA_SetWindowModality, wasModalitySet);
     }
 
 #if QT_CONFIG(pushbutton)
     const QPlatformTheme *theme = QGuiApplicationPrivate::platformTheme();
-    if (d->mainDef && isActiveWindow()
+    if (mainDef && q->isActiveWindow()
         && theme->themeHint(QPlatformTheme::DialogSnapToDefaultButton).toBool())
-        QCursor::setPos(d->mainDef->mapToGlobal(d->mainDef->rect().center()));
+        QCursor::setPos(mainDef->mapToGlobal(mainDef->rect().center()));
 #endif
 }
 

src/widgets/dialogs/qdialog_p.h

@@ -51,6 +51,8 @@ public:
         {}
     ~QDialogPrivate();
 
+    virtual void setVisible(bool visible);
+
     QWindow *transientParentWindow() const;
     bool setNativeDialogVisible(bool visible);
     QVariant styleHint(QPlatformDialogHelper::StyleHint hint) const;

src/widgets/dialogs/qerrormessage.cpp

@@ -53,6 +53,8 @@ public:
     bool nextPending();
     void retranslateStrings();
 
+    void setVisible(bool) override;
+
 private:
     void initHelper(QPlatformDialogHelper *) override;
     void helperPrepareShow(QPlatformDialogHelper *) override;
@@ -403,21 +405,21 @@ void QErrorMessage::showMessage(const QString &message, const QString &type)
         show();
 }
 
-void QErrorMessage::setVisible(bool visible)
+void QErrorMessagePrivate::setVisible(bool visible)
 {
-    if (testAttribute(Qt::WA_WState_ExplicitShowHide) && testAttribute(Qt::WA_WState_Hidden) != visible)
+    Q_Q(QErrorMessage);
+    if (q->testAttribute(Qt::WA_WState_ExplicitShowHide) && q->testAttribute(Qt::WA_WState_Hidden) != visible)
         return;
 
-    Q_D(QErrorMessage);
+    if (canBeNativeDialog())
-    if (d->canBeNativeDialog())
+        setNativeDialogVisible(visible);
-        d->setNativeDialogVisible(visible);
 
     // Update WA_DontShowOnScreen based on whether the native dialog was shown,
     // so that QDialog::setVisible(visible) below updates the QWidget state correctly,
     // but skips showing the non-native version.
-    setAttribute(Qt::WA_DontShowOnScreen, d->nativeDialogInUse);
+    q->setAttribute(Qt::WA_DontShowOnScreen, nativeDialogInUse);
 
-    QDialog::setVisible(visible);
+    QDialogPrivate::setVisible(visible);
 }
 
 /*!

src/widgets/dialogs/qerrormessage.h

@@ -28,8 +28,6 @@ public Q_SLOTS:
     void showMessage(const QString &message);
     void showMessage(const QString &message, const QString &type);
 
-    void setVisible(bool) override;
-
 protected:
     void done(int) override;
     void changeEvent(QEvent *e) override;

src/widgets/dialogs/qmessagebox.cpp

@@ -188,6 +188,8 @@ public:
     int layoutMinimumWidth();
     void retranslateStrings();
 
+    void setVisible(bool visible) override;
+
     static int showOldMessageBox(QWidget *parent, QMessageBox::Icon icon,
                                  const QString &title, const QString &text,
                                  int button0, int button1, int button2);
@@ -1563,21 +1565,21 @@ void QMessageBox::open(QObject *receiver, const char *member)
     QDialog::open();
 }
 
-void QMessageBox::setVisible(bool visible)
+void QMessageBoxPrivate::setVisible(bool visible)
 {
-    if (testAttribute(Qt::WA_WState_ExplicitShowHide) && testAttribute(Qt::WA_WState_Hidden) != visible)
+    Q_Q(QMessageBox);
+    if (q->testAttribute(Qt::WA_WState_ExplicitShowHide) && q->testAttribute(Qt::WA_WState_Hidden) != visible)
         return;
 
-    Q_D(QMessageBox);
+    if (canBeNativeDialog())
-    if (d->canBeNativeDialog())
+        setNativeDialogVisible(visible);
-        d->setNativeDialogVisible(visible);
 
     // Update WA_DontShowOnScreen based on whether the native dialog was shown,
     // so that QDialog::setVisible(visible) below updates the QWidget state correctly,
     // but skips showing the non-native version.
-    setAttribute(Qt::WA_DontShowOnScreen, d->nativeDialogInUse);
+    q->setAttribute(Qt::WA_DontShowOnScreen, nativeDialogInUse);
 
-    QDialog::setVisible(visible);
+    QDialogPrivate::setVisible(visible);
 }
 
 /*!

src/widgets/dialogs/qmessagebox.h

@@ -113,8 +113,6 @@ public:
     using QDialog::open;
     void open(QObject *receiver, const char *member);
 
-    void setVisible(bool visible) override;
-
     QList<QAbstractButton *> buttons() const;
     ButtonRole buttonRole(QAbstractButton *button) const;
 

tests/auto/widgets/dialogs/qerrormessage/tst_qerrormessage.cpp

@@ -18,6 +18,7 @@ private slots:
 
     void dontShowAgain();
     void dontShowCategoryAgain();
+    void baseClassSetVisible();
 
 };
 
@@ -138,5 +139,13 @@ void tst_QErrorMessage::dontShowCategoryAgain()
     QVERIFY(errorMessageDialog.isVisible());
 }
 
+void tst_QErrorMessage::baseClassSetVisible()
+{
+    QErrorMessage errorMessage;
+    errorMessage.QDialog::setVisible(true);
+    QCOMPARE(errorMessage.isVisible(), true);
+    errorMessage.close();
+}
+
 QTEST_MAIN(tst_QErrorMessage)
 #include "tst_qerrormessage.moc"

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

@@ -27,6 +27,7 @@ private slots:
     void init();
 
     void sanityTest();
+    void baseClassSetVisible();
     void defaultButton();
     void escapeButton();
     void clickedButton();
@@ -174,6 +175,15 @@ void tst_QMessageBox::sanityTest()
     msgBox.exec();
 }
 
+void tst_QMessageBox::baseClassSetVisible()
+{
+    QMessageBox msgBox;
+    msgBox.setText("Hello World");
+    msgBox.QDialog::setVisible(true);
+    QCOMPARE(msgBox.isVisible(), true);
+    msgBox.close();
+}
+
 void tst_QMessageBox::button()
 {
     QMessageBox msgBox;