diff options
author | Axel Spoerl <axel.spoerl@qt.io> | 2023-03-10 17:04:16 +0100 |
---|---|---|
committer | Axel Spoerl <axel.spoerl@qt.io> | 2023-03-14 21:15:31 +0100 |
commit | 425e635ecd8d588a87da66a1ef89fbf9ef469f1e (patch) | |
tree | ac0fd88eb66d4b9d6e63bf85c50c7a1120c35db5 | |
parent | c99614711ed9df0d7b942585a258537c9d12d298 (diff) |
QCompleter::setPopup() - refactor and cleanup
QCompleter::setPopup() sets window flags, focus policy, parent, focus
proxy, item delate and installs an event filter, before the popup
argument is assigned to d->popup.
In the QCompleter::eventFilter override, QObject::eventFilter is called
(under more) if d->popup is nullptr.
If a custom class is inherited from QCompleter and it overrides
QObject::eventFilter(), this causes an infinite loop.
This patch re-factors the method.
- early return is added if the new popup != d->popup
- remembering the existing widget's focus policy is constified and
moved ahead of the delete secion.
- assignment of d->popup to popup argument is moved after the delete
section.
- after assignment, the argument variable is no longer used.
The refactoring eliminates two issues:
- potential risk of double-installing event filter due to missing
early return.
- inifite loop if inherited class installs another event filter.
The patch adds a test function to tst_QCompleter, which implements an
inherited class, installs an event filter on a popup and checks if a
ChildAdded event hass been recorded.
Fixes: QTBUG-111869
Pick-to: 6.5 6.2
Change-Id: I3f7a2434a11476077a5260e7686a912da9f6c60d
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>
-rw-r--r-- | src/widgets/util/qcompleter.cpp | 45 | ||||
-rw-r--r-- | tests/auto/widgets/util/qcompleter/tst_qcompleter.cpp | 38 |
2 files changed, 63 insertions, 20 deletions
diff --git a/src/widgets/util/qcompleter.cpp b/src/widgets/util/qcompleter.cpp index 1b519bf4c5..9664004455 100644 --- a/src/widgets/util/qcompleter.cpp +++ b/src/widgets/util/qcompleter.cpp @@ -1207,50 +1207,55 @@ Qt::MatchFlags QCompleter::filterMode() const */ void QCompleter::setPopup(QAbstractItemView *popup) { + Q_ASSERT(popup); Q_D(QCompleter); - Q_ASSERT(popup != nullptr); + if (popup == d->popup) + return; + + // Remember existing widget's focus policy, default to NoFocus + const Qt::FocusPolicy origPolicy = d->widget ? d->widget->focusPolicy() + : Qt::NoFocus; + + // If popup existed already, disconnect signals and delete object if (d->popup) { QObject::disconnect(d->popup->selectionModel(), nullptr, this, nullptr); QObject::disconnect(d->popup, nullptr, this, nullptr); - } - if (d->popup != popup) delete d->popup; - if (popup->model() != d->proxy) - popup->setModel(d->proxy); - popup->hide(); + } - Qt::FocusPolicy origPolicy = Qt::NoFocus; - if (d->widget) - origPolicy = d->widget->focusPolicy(); + // Assign new object, set model and hide + d->popup = popup; + if (d->popup->model() != d->proxy) + d->popup->setModel(d->proxy); + d->popup->hide(); // Mark the widget window as a popup, so that if the last non-popup window is closed by the // user, the application should not be prevented from exiting. It needs to be set explicitly via // setWindowFlag(), because passing the flag via setParent(parent, windowFlags) does not call // QWidgetPrivate::adjustQuitOnCloseAttribute(), and causes an application not to exit if the // popup ends up being the last window. - popup->setParent(nullptr); - popup->setWindowFlag(Qt::Popup); - popup->setFocusPolicy(Qt::NoFocus); + d->popup->setParent(nullptr); + d->popup->setWindowFlag(Qt::Popup); + d->popup->setFocusPolicy(Qt::NoFocus); if (d->widget) d->widget->setFocusPolicy(origPolicy); - popup->setFocusProxy(d->widget); - popup->installEventFilter(this); - popup->setItemDelegate(new QCompleterItemDelegate(popup)); + d->popup->setFocusProxy(d->widget); + d->popup->installEventFilter(this); + d->popup->setItemDelegate(new QCompleterItemDelegate(d->popup)); #if QT_CONFIG(listview) - if (QListView *listView = qobject_cast<QListView *>(popup)) { + if (QListView *listView = qobject_cast<QListView *>(d->popup)) { listView->setModelColumn(d->column); } #endif - QObject::connect(popup, SIGNAL(clicked(QModelIndex)), + QObject::connect(d->popup, SIGNAL(clicked(QModelIndex)), this, SLOT(_q_complete(QModelIndex))); QObject::connect(this, SIGNAL(activated(QModelIndex)), - popup, SLOT(hide())); + d->popup, SLOT(hide())); - QObject::connect(popup->selectionModel(), SIGNAL(selectionChanged(QItemSelection,QItemSelection)), + QObject::connect(d->popup->selectionModel(), SIGNAL(selectionChanged(QItemSelection,QItemSelection)), this, SLOT(_q_completionSelected(QItemSelection))); - d->popup = popup; } /*! diff --git a/tests/auto/widgets/util/qcompleter/tst_qcompleter.cpp b/tests/auto/widgets/util/qcompleter/tst_qcompleter.cpp index e8ebaf1491..030bb1cf5b 100644 --- a/tests/auto/widgets/util/qcompleter/tst_qcompleter.cpp +++ b/tests/auto/widgets/util/qcompleter/tst_qcompleter.cpp @@ -123,6 +123,7 @@ private slots: void QTBUG_52028_tabAutoCompletes(); void QTBUG_51889_activatedSentTwice(); void showPopupInGraphicsView(); + void inheritedEventFilter(); private: void filter(bool assync = false); @@ -1863,5 +1864,42 @@ void tst_QCompleter::showPopupInGraphicsView() QVERIFY(lineEdit.completer()->popup()->geometry().bottom() < lineEdit.mapToGlobal(QPoint(0, 0)).y()); } +void tst_QCompleter::inheritedEventFilter() +{ + class Completer : public QCompleter + { + public: + explicit Completer(QWidget *parent) : QCompleter(parent) + { + Q_ASSERT(parent); + setPopup(new QListView()); + popup()->installEventFilter(this); + } + + bool m_popupChildAdded = false; + + protected: + bool eventFilter(QObject *watched, QEvent *event) override + { + if (watched == popup() && event->type() == QEvent::ChildAdded) + m_popupChildAdded = true; + + return QCompleter::eventFilter(watched, event); + } + }; + + QComboBox comboBox; + comboBox.setEditable(true); + Completer *completer = new Completer(&comboBox); + comboBox.setCompleter(completer); + + // comboBox.show() must not crash with an infinite loop in the event filter + comboBox.show(); + QVERIFY(QTest::qWaitForWindowExposed(&comboBox)); + + // Since event orders are platform dependent, only the the ChildAdded event is checked. + QVERIFY(QTest::qWaitFor([completer](){return completer->m_popupChildAdded; })); +} + QTEST_MAIN(tst_QCompleter) #include "tst_qcompleter.moc" |