QDialogButtonBox - code cleanup

Replace const char * based connect statements by modern API and remove
'_q_' slot prefixes.
This requires explicit disconnection in the destructor -> add.
Replace bool traps for layouting and removing buttons by enum classes.
Replace if/elseif event handler by switch.
Replace iterator typedef with auto.
Encapsulate logic to ensure defaulting to first accept button in a
member function for better readability.
Remove dead code.
Move private header declaration from cpp to a new _p.h.

Task-number: QTBUG-114377
Change-Id: I8a2bc355e3816c3c826c10cd96194c89bf0ae510
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
(cherry picked from commit df735d794fd2e545c18b9e345e833422bcd64329)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

src/widgets/CMakeLists.txt

@@ -510,7 +510,7 @@ qt_internal_extend_target(Widgets CONDITION QT_FEATURE_resizehandler
 
 qt_internal_extend_target(Widgets CONDITION QT_FEATURE_dialogbuttonbox
     SOURCES
-        widgets/qdialogbuttonbox.cpp widgets/qdialogbuttonbox.h
+        widgets/qdialogbuttonbox.cpp widgets/qdialogbuttonbox.h widgets/qdialogbuttonbox_p.h
 )
 
 qt_internal_extend_target(Widgets CONDITION QT_FEATURE_rubberband

src/widgets/widgets/qdialogbuttonbox.cpp

@@ -14,6 +14,7 @@
 #include <QtGui/qaction.h>
 
 #include "qdialogbuttonbox.h"
+#include "qdialogbuttonbox_p.h"
 
 QT_BEGIN_NAMESPACE
 
@@ -112,37 +113,8 @@ QT_BEGIN_NAMESPACE
     \sa QMessageBox, QPushButton, QDialog
 */
 
-class QDialogButtonBoxPrivate : public QWidgetPrivate
-{
-    Q_DECLARE_PUBLIC(QDialogButtonBox)
-
-public:
-    QDialogButtonBoxPrivate(Qt::Orientation orient);
-
-    QList<QAbstractButton *> buttonLists[QDialogButtonBox::NRoles];
-    QHash<QPushButton *, QDialogButtonBox::StandardButton> standardButtonHash;
-
-    Qt::Orientation orientation;
-    QDialogButtonBox::ButtonLayout layoutPolicy;
-    QBoxLayout *buttonLayout;
-    bool internalRemove;
-    bool center;
-
-    void createStandardButtons(QDialogButtonBox::StandardButtons buttons);
-
-    void layoutButtons();
-    void initLayout();
-    void resetLayout();
-    QPushButton *createButton(QDialogButtonBox::StandardButton button, bool doLayout = true);
-    void addButton(QAbstractButton *button, QDialogButtonBox::ButtonRole role, bool doLayout = true);
-    void _q_handleButtonDestroyed();
-    void _q_handleButtonClicked();
-    void addButtonsToLayout(const QList<QAbstractButton *> &buttonList, bool reverse);
-    void retranslateStrings();
-};
-
 QDialogButtonBoxPrivate::QDialogButtonBoxPrivate(Qt::Orientation orient)
-    : orientation(orient), buttonLayout(nullptr), internalRemove(false), center(false)
+    : orientation(orient), buttonLayout(nullptr), center(false)
 {
 }
 
@@ -177,7 +149,6 @@ void QDialogButtonBoxPrivate::initLayout()
 
 void QDialogButtonBoxPrivate::resetLayout()
 {
-    //delete buttonLayout;
     initLayout();
     layoutButtons();
 }
@@ -311,7 +282,7 @@ void QDialogButtonBoxPrivate::layoutButtons()
 }
 
 QPushButton *QDialogButtonBoxPrivate::createButton(QDialogButtonBox::StandardButton sbutton,
-                                                   bool doLayout)
+                                                   LayoutRule layoutRule)
 {
     Q_Q(QDialogButtonBox);
     int icon = 0;
@@ -386,7 +357,7 @@ QPushButton *QDialogButtonBoxPrivate::createButton(QDialogButtonBox::StandardBut
     if (Q_UNLIKELY(role == QPlatformDialogHelper::InvalidRole))
         qWarning("QDialogButtonBox::createButton: Invalid ButtonRole, button not added");
     else
-        addButton(button, static_cast<QDialogButtonBox::ButtonRole>(role), doLayout);
+        addButton(button, static_cast<QDialogButtonBox::ButtonRole>(role), layoutRule);
 #if QT_CONFIG(shortcut)
     const QKeySequence standardShortcut = QGuiApplicationPrivate::platformTheme()->standardButtonShortcut(sbutton);
     if (!standardShortcut.isEmpty())
@@ -396,23 +367,26 @@ QPushButton *QDialogButtonBoxPrivate::createButton(QDialogButtonBox::StandardBut
 }
 
 void QDialogButtonBoxPrivate::addButton(QAbstractButton *button, QDialogButtonBox::ButtonRole role,
-                                        bool doLayout)
+                                        LayoutRule layoutRule)
 {
-    Q_Q(QDialogButtonBox);
+    QObjectPrivate::connect(button, &QAbstractButton::clicked, this, &QDialogButtonBoxPrivate::handleButtonClicked);
-    QObject::connect(button, SIGNAL(clicked()), q, SLOT(_q_handleButtonClicked()));
+    QObjectPrivate::connect(button, &QAbstractButton::destroyed, this, &QDialogButtonBoxPrivate::handleButtonDestroyed);
-    QObject::connect(button, SIGNAL(destroyed()), q, SLOT(_q_handleButtonDestroyed()));
     buttonLists[role].append(button);
-    if (doLayout)
+    switch (layoutRule) {
+    case LayoutRule::DoLayout:
         layoutButtons();
+        break;
+    case LayoutRule::SkipLayout:
+        break;
+    }
 }
 
 void QDialogButtonBoxPrivate::createStandardButtons(QDialogButtonBox::StandardButtons buttons)
 {
     uint i = QDialogButtonBox::FirstButton;
     while (i <= QDialogButtonBox::LastButton) {
-        if (i & buttons) {
+        if (i & buttons)
-            createButton(QDialogButtonBox::StandardButton(i), false);
+            createButton(QDialogButtonBox::StandardButton(i), LayoutRule::SkipLayout);
-        }
         i = i << 1;
     }
     layoutButtons();
@@ -420,13 +394,10 @@ void QDialogButtonBoxPrivate::createStandardButtons(QDialogButtonBox::StandardBu
 
 void QDialogButtonBoxPrivate::retranslateStrings()
 {
-    typedef QHash<QPushButton *, QDialogButtonBox::StandardButton>::iterator Iterator;
+    for (auto &&[key, value] : std::as_const(standardButtonHash).asKeyValueRange()) {
-
+        const QString text = QGuiApplicationPrivate::platformTheme()->standardButtonText(value);
-    const Iterator end = standardButtonHash.end();
-    for (Iterator it = standardButtonHash.begin(); it != end; ++it) {
-        const QString text = QGuiApplicationPrivate::platformTheme()->standardButtonText(it.value());
         if (!text.isEmpty())
-            it.key()->setText(text);
+            key->setText(text);
     }
 }
 
@@ -482,6 +453,11 @@ QDialogButtonBox::QDialogButtonBox(StandardButtons buttons, Qt::Orientation orie
 */
 QDialogButtonBox::~QDialogButtonBox()
 {
+    // QObjectPrivate::connect requires explicit disconnect in destructor
+    // otherwise the connection may kick in on child destruction and reach
+    // the parent's destroyed private object
+    Q_D(QDialogButtonBox);
+    d->disconnectAll();
 }
 
 /*!
@@ -638,7 +614,8 @@ void QDialogButtonBox::clear()
         QList<QAbstractButton *> &list = d->buttonLists[i];
         while (list.size()) {
             QAbstractButton *button = list.takeAt(0);
-            QObject::disconnect(button, SIGNAL(destroyed()), this, SLOT(_q_handleButtonDestroyed()));
+            QObjectPrivate::disconnect(button, &QAbstractButton::destroyed,
+                                       d, &QDialogButtonBoxPrivate::handleButtonDestroyed);
             delete button;
         }
     }
@@ -688,27 +665,30 @@ QDialogButtonBox::ButtonRole QDialogButtonBox::buttonRole(QAbstractButton *butto
 void QDialogButtonBox::removeButton(QAbstractButton *button)
 {
     Q_D(QDialogButtonBox);
+    d->removeButton(button, QDialogButtonBoxPrivate::RemoveRule::Disconnect);
+}
 
+void QDialogButtonBoxPrivate::removeButton(QAbstractButton *button, RemoveRule rule)
+{
     if (!button)
         return;
 
     // Remove it from the standard button hash first and then from the roles
-    d->standardButtonHash.remove(reinterpret_cast<QPushButton *>(button));
+    standardButtonHash.remove(reinterpret_cast<QPushButton *>(button));
-    for (int i = 0; i < NRoles; ++i) {
+    for (int i = 0; i < QDialogButtonBox::NRoles; ++i)
-        QList<QAbstractButton *> &list = d->buttonLists[i];
+        buttonLists[i].removeOne(button);
-        for (int j = 0; j < list.size(); ++j) {
+
-            if (list.at(j) == button) {
+    switch (rule) {
-                list.takeAt(j);
+    case RemoveRule::Disconnect:
-                if (!d->internalRemove) {
-                    disconnect(button, SIGNAL(clicked()), this, SLOT(_q_handleButtonClicked()));
-                    disconnect(button, SIGNAL(destroyed()), this, SLOT(_q_handleButtonDestroyed()));
-                }
-                break;
-            }
-        }
-    }
-    if (!d->internalRemove)
         button->setParent(nullptr);
+        QObjectPrivate::disconnect(button, &QAbstractButton::clicked,
+                                   this, &QDialogButtonBoxPrivate::handleButtonClicked);
+        QObjectPrivate::disconnect(button, &QAbstractButton::destroyed,
+                                   this, &QDialogButtonBoxPrivate::handleButtonDestroyed);
+        break;
+    case RemoveRule::KeepConnections:
+        break;
+    }
 }
 
 /*!
@@ -778,7 +758,7 @@ void QDialogButtonBox::setStandardButtons(StandardButtons buttons)
 {
     Q_D(QDialogButtonBox);
     // Clear out all the old standard buttons, then recreate them.
-    qDeleteAll(d->standardButtonHash.keys());
+    qDeleteAll(d->standardButtonHash.keyBegin(), d->standardButtonHash.keyEnd());
     d->standardButtonHash.clear();
 
     d->createStandardButtons(buttons);
@@ -820,7 +800,7 @@ QDialogButtonBox::StandardButton QDialogButtonBox::standardButton(QAbstractButto
     return d->standardButtonHash.value(static_cast<QPushButton *>(button));
 }
 
-void QDialogButtonBoxPrivate::_q_handleButtonClicked()
+void QDialogButtonBoxPrivate::handleButtonClicked()
 {
     Q_Q(QDialogButtonBox);
     if (QAbstractButton *button = qobject_cast<QAbstractButton *>(q->sender())) {
@@ -854,13 +834,11 @@ void QDialogButtonBoxPrivate::_q_handleButtonClicked()
     }
 }
 
-void QDialogButtonBoxPrivate::_q_handleButtonDestroyed()
+void QDialogButtonBoxPrivate::handleButtonDestroyed()
 {
     Q_Q(QDialogButtonBox);
-    if (QObject *object = q->sender()) {
+    if (QObject *object = q->sender())
-        QBoolBlocker skippy(internalRemove);
+        removeButton(reinterpret_cast<QAbstractButton *>(object), RemoveRule::KeepConnections);
-        q->removeButton(reinterpret_cast<QAbstractButton *>(object));
-    }
 }
 
 /*!
@@ -918,36 +896,66 @@ void QDialogButtonBox::changeEvent(QEvent *event)
     }
 }
 
+void QDialogButtonBoxPrivate::ensureFirstAcceptIsDefault()
+{
+    Q_Q(QDialogButtonBox);
+    const QList<QAbstractButton *> &acceptRoleList = buttonLists[QDialogButtonBox::AcceptRole];
+    QPushButton *firstAcceptButton = acceptRoleList.isEmpty()
+                                   ? nullptr
+                                   : qobject_cast<QPushButton *>(acceptRoleList.at(0));
+
+    if (!firstAcceptButton)
+        return;
+
+    bool hasDefault = false;
+    QWidget *dialog = nullptr;
+    QWidget *p = q;
+    while (p && !p->isWindow()) {
+        p = p->parentWidget();
+        if ((dialog = qobject_cast<QDialog *>(p)))
+            break;
+    }
+
+    QWidget *parent = dialog ? dialog : q;
+    Q_ASSERT(parent);
+
+    const auto pushButtons = parent->findChildren<QPushButton *>();
+    for (QPushButton *pushButton : pushButtons) {
+        if (pushButton->isDefault() && pushButton != firstAcceptButton) {
+            hasDefault = true;
+            break;
+        }
+    }
+    if (!hasDefault && firstAcceptButton)
+        firstAcceptButton->setDefault(true);
+}
+
+void QDialogButtonBoxPrivate::disconnectAll()
+{
+    Q_Q(QDialogButtonBox);
+    const auto buttons = q->findChildren<QAbstractButton *>();
+    for (auto *button : buttons)
+        button->disconnect(q);
+}
+
 /*!
     \reimp
 */
 bool QDialogButtonBox::event(QEvent *event)
 {
     Q_D(QDialogButtonBox);
-    if (event->type() == QEvent::Show) {
+    switch (event->type()) {
-        QList<QAbstractButton *> acceptRoleList = d->buttonLists[AcceptRole];
+    case QEvent::Show:
-        QPushButton *firstAcceptButton = acceptRoleList.isEmpty() ? 0 : qobject_cast<QPushButton *>(acceptRoleList.at(0));
+        d->ensureFirstAcceptIsDefault();
-        bool hasDefault = false;
+        break;
-        QWidget *dialog = nullptr;
-        QWidget *p = this;
-        while (p && !p->isWindow()) {
-            p = p->parentWidget();
-            if ((dialog = qobject_cast<QDialog *>(p)))
-                break;
-        }
 
-        const auto pbs = (dialog ? dialog : this)->findChildren<QPushButton *>();
+    case QEvent::LanguageChange:
-        for (QPushButton *pb : pbs) {
-            if (pb->isDefault() && pb != firstAcceptButton) {
-                hasDefault = true;
-                break;
-            }
-        }
-        if (!hasDefault && firstAcceptButton)
-            firstAcceptButton->setDefault(true);
-    }else if (event->type() == QEvent::LanguageChange) {
         d->retranslateStrings();
+        break;
+
+    default: break;
     }
+
     return QWidget::event(event);
 }
 

src/widgets/widgets/qdialogbuttonbox.h

@@ -120,8 +120,6 @@ protected:
 private:
     Q_DISABLE_COPY(QDialogButtonBox)
     Q_DECLARE_PRIVATE(QDialogButtonBox)
-    Q_PRIVATE_SLOT(d_func(), void _q_handleButtonClicked())
-    Q_PRIVATE_SLOT(d_func(), void _q_handleButtonDestroyed())
 };
 
 Q_DECLARE_OPERATORS_FOR_FLAGS(QDialogButtonBox::StandardButtons)

src/widgets/widgets/qdialogbuttonbox_p.h

@@ -0,0 +1,72 @@
+// Copyright (C) 2023 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR LGPL-3.0-only OR GPL-2.0-only OR GPL-3.0-only
+
+#ifndef QDIALOGBUTTONBOX_P_H
+#define QDIALOGBUTTONBOX_P_H
+
+//
+//  W A R N I N G
+//  -------------
+//
+// This file is not part of the Qt API.  It exists purely as an
+// implementation detail.  This header file may change from version to
+// version without notice, or even be removed.
+//
+// We mean it.
+//
+
+#include <private/qwidget_p.h>
+#include <qdialogbuttonbox.h>
+
+QT_BEGIN_NAMESPACE
+class Q_AUTOTEST_EXPORT QDialogButtonBoxPrivate : public QWidgetPrivate
+{
+    Q_DECLARE_PUBLIC(QDialogButtonBox)
+
+public:
+    enum class RemoveRule {
+        KeepConnections,
+        Disconnect,
+    };
+    enum class LayoutRule {
+        DoLayout,
+        SkipLayout,
+    };
+
+    QDialogButtonBoxPrivate(Qt::Orientation orient);
+
+    QList<QAbstractButton *> buttonLists[QDialogButtonBox::NRoles];
+    QHash<QPushButton *, QDialogButtonBox::StandardButton> standardButtonHash;
+    QHash<QAbstractButton *, QDialogButtonBox::ButtonRole> hiddenButtons;
+
+    Qt::Orientation orientation;
+    QDialogButtonBox::ButtonLayout layoutPolicy;
+    QBoxLayout *buttonLayout;
+    bool center;
+    bool byPassEventFilter = false;
+
+    void createStandardButtons(QDialogButtonBox::StandardButtons buttons);
+
+    void removeButton(QAbstractButton *button, RemoveRule rule);
+    void layoutButtons();
+    void initLayout();
+    void resetLayout();
+    QPushButton *createButton(QDialogButtonBox::StandardButton button,
+                              LayoutRule layoutRule = LayoutRule::DoLayout);
+    void addButton(QAbstractButton *button, QDialogButtonBox::ButtonRole role,
+                   LayoutRule layoutRule = LayoutRule::DoLayout);
+    void handleButtonDestroyed();
+    void handleButtonClicked();
+    bool handleButtonShowAndHide(QAbstractButton *button, QEvent *event);
+    void addButtonsToLayout(const QList<QAbstractButton *> &buttonList, bool reverse);
+    void ensureFirstAcceptIsDefault();
+    void retranslateStrings();
+    void disconnectAll();
+    QList<QAbstractButton *> allButtons() const;
+    QList<QAbstractButton *> visibleButtons() const;
+    QDialogButtonBox::ButtonRole buttonRole(QAbstractButton *button) const;
+};
+
+QT_END_NAMESPACE
+
+#endif // QDIALOGBUTTONBOX_P_H