From df735d794fd2e545c18b9e345e833422bcd64329 Mon Sep 17 00:00:00 2001 From: Axel Spoerl Date: Sat, 24 Jun 2023 11:38:31 +0200 Subject: [PATCH] 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 Pick-to: 6.6 6.5 Change-Id: I8a2bc355e3816c3c826c10cd96194c89bf0ae510 Reviewed-by: Volker Hilsheimer --- src/widgets/CMakeLists.txt | 2 +- src/widgets/widgets/qdialogbuttonbox.cpp | 190 ++++++++++++----------- src/widgets/widgets/qdialogbuttonbox.h | 2 - src/widgets/widgets/qdialogbuttonbox_p.h | 72 +++++++++ 4 files changed, 172 insertions(+), 94 deletions(-) create mode 100644 src/widgets/widgets/qdialogbuttonbox_p.h diff --git a/src/widgets/CMakeLists.txt b/src/widgets/CMakeLists.txt index aaa81286ab..e0b8d8cab3 100644 --- a/src/widgets/CMakeLists.txt +++ b/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 diff --git a/src/widgets/widgets/qdialogbuttonbox.cpp b/src/widgets/widgets/qdialogbuttonbox.cpp index 5afecc8d0a..8d8c82e67b 100644 --- a/src/widgets/widgets/qdialogbuttonbox.cpp +++ b/src/widgets/widgets/qdialogbuttonbox.cpp @@ -14,6 +14,7 @@ #include #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 buttonLists[QDialogButtonBox::NRoles]; - QHash 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 &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(role), doLayout); + addButton(button, static_cast(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); - QObject::connect(button, SIGNAL(clicked()), q, SLOT(_q_handleButtonClicked())); - QObject::connect(button, SIGNAL(destroyed()), q, SLOT(_q_handleButtonDestroyed())); + QObjectPrivate::connect(button, &QAbstractButton::clicked, this, &QDialogButtonBoxPrivate::handleButtonClicked); + QObjectPrivate::connect(button, &QAbstractButton::destroyed, this, &QDialogButtonBoxPrivate::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) { - createButton(QDialogButtonBox::StandardButton(i), false); - } + if (i & buttons) + createButton(QDialogButtonBox::StandardButton(i), LayoutRule::SkipLayout); i = i << 1; } layoutButtons(); @@ -420,13 +394,10 @@ void QDialogButtonBoxPrivate::createStandardButtons(QDialogButtonBox::StandardBu void QDialogButtonBoxPrivate::retranslateStrings() { - typedef QHash::iterator Iterator; - - const Iterator end = standardButtonHash.end(); - for (Iterator it = standardButtonHash.begin(); it != end; ++it) { - const QString text = QGuiApplicationPrivate::platformTheme()->standardButtonText(it.value()); + for (auto &&[key, value] : std::as_const(standardButtonHash).asKeyValueRange()) { + const QString text = QGuiApplicationPrivate::platformTheme()->standardButtonText(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 &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(button)); - for (int i = 0; i < NRoles; ++i) { - QList &list = d->buttonLists[i]; - for (int j = 0; j < list.size(); ++j) { - if (list.at(j) == button) { - list.takeAt(j); - if (!d->internalRemove) { - disconnect(button, SIGNAL(clicked()), this, SLOT(_q_handleButtonClicked())); - disconnect(button, SIGNAL(destroyed()), this, SLOT(_q_handleButtonDestroyed())); - } - break; - } - } - } - if (!d->internalRemove) + standardButtonHash.remove(reinterpret_cast(button)); + for (int i = 0; i < QDialogButtonBox::NRoles; ++i) + buttonLists[i].removeOne(button); + + switch (rule) { + case RemoveRule::Disconnect: 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(button)); } -void QDialogButtonBoxPrivate::_q_handleButtonClicked() +void QDialogButtonBoxPrivate::handleButtonClicked() { Q_Q(QDialogButtonBox); if (QAbstractButton *button = qobject_cast(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()) { - QBoolBlocker skippy(internalRemove); - q->removeButton(reinterpret_cast(object)); - } + if (QObject *object = q->sender()) + removeButton(reinterpret_cast(object), RemoveRule::KeepConnections); } /*! @@ -918,36 +896,66 @@ void QDialogButtonBox::changeEvent(QEvent *event) } } +void QDialogButtonBoxPrivate::ensureFirstAcceptIsDefault() +{ + Q_Q(QDialogButtonBox); + const QList &acceptRoleList = buttonLists[QDialogButtonBox::AcceptRole]; + QPushButton *firstAcceptButton = acceptRoleList.isEmpty() + ? nullptr + : qobject_cast(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(p))) + break; + } + + QWidget *parent = dialog ? dialog : q; + Q_ASSERT(parent); + + const auto pushButtons = parent->findChildren(); + 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(); + for (auto *button : buttons) + button->disconnect(q); +} + /*! \reimp */ bool QDialogButtonBox::event(QEvent *event) { Q_D(QDialogButtonBox); - if (event->type() == QEvent::Show) { - QList acceptRoleList = d->buttonLists[AcceptRole]; - QPushButton *firstAcceptButton = acceptRoleList.isEmpty() ? 0 : qobject_cast(acceptRoleList.at(0)); - bool hasDefault = false; - QWidget *dialog = nullptr; - QWidget *p = this; - while (p && !p->isWindow()) { - p = p->parentWidget(); - if ((dialog = qobject_cast(p))) - break; - } + switch (event->type()) { + case QEvent::Show: + d->ensureFirstAcceptIsDefault(); + break; - const auto pbs = (dialog ? dialog : this)->findChildren(); - for (QPushButton *pb : pbs) { - if (pb->isDefault() && pb != firstAcceptButton) { - hasDefault = true; - break; - } - } - if (!hasDefault && firstAcceptButton) - firstAcceptButton->setDefault(true); - }else if (event->type() == QEvent::LanguageChange) { + case QEvent::LanguageChange: d->retranslateStrings(); + break; + + default: break; } + return QWidget::event(event); } diff --git a/src/widgets/widgets/qdialogbuttonbox.h b/src/widgets/widgets/qdialogbuttonbox.h index f6a0371d6c..d967494d0d 100644 --- a/src/widgets/widgets/qdialogbuttonbox.h +++ b/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) diff --git a/src/widgets/widgets/qdialogbuttonbox_p.h b/src/widgets/widgets/qdialogbuttonbox_p.h new file mode 100644 index 0000000000..6e02512b2b --- /dev/null +++ b/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 +#include + +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 buttonLists[QDialogButtonBox::NRoles]; + QHash standardButtonHash; + QHash 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 &buttonList, bool reverse); + void ensureFirstAcceptIsDefault(); + void retranslateStrings(); + void disconnectAll(); + QList allButtons() const; + QList visibleButtons() const; + QDialogButtonBox::ButtonRole buttonRole(QAbstractButton *button) const; +}; + +QT_END_NAMESPACE + +#endif // QDIALOGBUTTONBOX_P_H