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 <[email protected]>

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;
 }
 
 /*!

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"