QCompleter: Send activated() signal only once on return key

Due to the complex event forwarding logic between QCompleter,
QComboBox, QLineEdit and QWidgetLineControl, in some cases the
same single user return key press could result in duplicated
activated() signals being emitted by QComboBox. The first one
would be emitted because QLineEdit emitted editingFinished()
as a result of QCompleter::eventFilter() having forwarded the
return key press event to QComboBox. The second one, would
happen right after, as QCompleter::eventFilter() would process
the same event on behalf of its popup.

(We recall that QCompleter is installed as its own popup event
filter. That's also the case for the completer's widget, although
the purpose there is limited to focus-out events).

The current fix consists on skipping the emit as a result of
QLineEdit::editingFinished() if the completer's popup is still
active. For this to be accurate, it helps to test whether the
completer's popup is visible, so we will not be hiding it in
QWidgetLineControl::processKeyEvent() anymore. Indeed, we know
that if the popup is visible, that means that processKeyEvent()
was called after being forwarded by the completer's popup event
filter. Furthermore, the popup will be hidden by its event filter
shortly after it returns from said event forwarding call.

Based on a patch by Alexey Chernov <4ernov@gmail.com>.

Task-number: QTBUG-51858
Task-number: QTBUG-51889
Change-Id: I013f6c3000ae37b5b0ec20eaf5cf7746c9c903e3
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>

src/widgets/widgets/qcombobox.cpp

@@ -1210,8 +1210,27 @@ Qt::MatchFlags QComboBoxPrivate::matchFlags() const
 void QComboBoxPrivate::_q_editingFinished()
 {
     Q_Q(QComboBox);
-    if (lineEdit && !lineEdit->text().isEmpty() && itemText(currentIndex) != lineEdit->text()) {
-        const int index = q_func()->findText(lineEdit->text(), matchFlags());
+    if (!lineEdit)
+        return;
+    const auto leText = lineEdit->text();
+    if (!leText.isEmpty() && itemText(currentIndex) != leText) {
+#if QT_CONFIG(completer)
+        const auto *leCompleter = lineEdit->completer();
+        const auto *popup = leCompleter ? QCompleterPrivate::get(leCompleter)->popup : nullptr;
+        if (popup && popup->isVisible()) {
+            // QLineEdit::editingFinished() will be emitted before the code flow returns
+            // to QCompleter::eventFilter(), where QCompleter::activated() may be emitted.
+            // We know that the completer popup will still be visible at this point, and
+            // that any selection should be valid.
+            const QItemSelectionModel *selModel = popup->selectionModel();
+            const QModelIndex curIndex = popup->currentIndex();
+            const bool completerIsActive = selModel && selModel->selectedIndexes().contains(curIndex);
+
+            if (completerIsActive)
+                return;
+        }
+#endif
+        const int index = q_func()->findText(leText, matchFlags());
         if (index != -1) {
             q->setCurrentIndex(index);
             emitActivated(currentIndex);

src/widgets/widgets/qwidgetlinecontrol.cpp

@@ -1642,16 +1642,6 @@ void QWidgetLineControl::processKeyEvent(QKeyEvent* event)
             case Qt::Key_Escape:
                 event->ignore();
                 return;
-            case Qt::Key_Enter:
-            case Qt::Key_Return:
-            case Qt::Key_F4:
-#ifdef QT_KEYPAD_NAVIGATION
-            case Qt::Key_Select:
-                if (!QApplication::keypadNavigationEnabled())
-                    break;
-#endif
-                popup->hide(); // just hide. will end up propagating to parent
-                break;
             default:
                 break; // normal key processing
             }

tests/auto/widgets/util/qcompleter/tst_qcompleter.cpp

@@ -143,6 +143,8 @@ private slots:
     void task253125_lineEditCompletion();
     void task247560_keyboardNavigation();
     void QTBUG_14292_filesystem();
+    void QTBUG_52028_tabAutoCompletes();
+    void QTBUG_51889_activatedSentTwice();
 
 private:
     void filter(bool assync = false);
@@ -1742,5 +1744,108 @@ void tst_QCompleter::QTBUG_14292_filesystem()
     QVERIFY(!comp.popup()->isVisible());
 }
 
+void tst_QCompleter::QTBUG_52028_tabAutoCompletes()
+{
+    QStringList words;
+    words << "foobar1" << "foobar2" << "hux";
+
+    QWidget w;
+    w.setLayout(new QVBoxLayout);
+
+    QComboBox cbox;
+    cbox.setEditable(true);
+    cbox.setInsertPolicy(QComboBox::NoInsert);
+    cbox.addItems(words);
+
+    cbox.completer()->setCaseSensitivity(Qt::CaseInsensitive);
+    cbox.completer()->setCompletionMode(QCompleter::PopupCompletion);
+
+    w.layout()->addWidget(&cbox);
+
+    // Adding a line edit is a good reason for tab to do something unrelated
+    QLineEdit le;
+    w.layout()->addWidget(&le);
+
+    const auto pos = QApplication::desktop()->availableGeometry(&w).topLeft() + QPoint(200,200);
+    w.move(pos);
+    w.show();
+    QApplication::setActiveWindow(&w);
+    QVERIFY(QTest::qWaitForWindowActive(&w));
+
+    QSignalSpy activatedSpy(&cbox, QOverload<int>::of(&QComboBox::activated));
+
+    // Tab key will complete but not activate
+    cbox.lineEdit()->clear();
+    QTest::keyClick(&cbox, Qt::Key_H);
+    QVERIFY(cbox.completer()->popup());
+    QTRY_VERIFY(cbox.completer()->popup()->isVisible());
+    QTest::keyClick(cbox.completer()->popup(), Qt::Key_Tab);
+    QCOMPARE(cbox.completer()->currentCompletion(), QLatin1String("hux"));
+    QCOMPARE(activatedSpy.count(), 0);
+    QEXPECT_FAIL("", "QTBUG-52028 will not be fixed today.", Abort);
+    QCOMPARE(cbox.currentText(), QLatin1String("hux"));
+    QCOMPARE(activatedSpy.count(), 0);
+    QVERIFY(!le.hasFocus());
+}
+
+void tst_QCompleter::QTBUG_51889_activatedSentTwice()
+{
+    QStringList words;
+    words << "foobar1" << "foobar2" << "bar" <<"hux";
+
+    QWidget w;
+    w.setLayout(new QVBoxLayout);
+
+    QComboBox cbox;
+    setFrameless(&cbox);
+    cbox.setEditable(true);
+    cbox.setInsertPolicy(QComboBox::NoInsert);
+    cbox.addItems(words);
+
+    cbox.completer()->setCaseSensitivity(Qt::CaseInsensitive);
+    cbox.completer()->setCompletionMode(QCompleter::PopupCompletion);
+
+    w.layout()->addWidget(&cbox);
+
+    QLineEdit le;
+    w.layout()->addWidget(&le);
+
+    const auto pos = QApplication::desktop()->availableGeometry(&w).topLeft() + QPoint(200,200);
+    w.move(pos);
+    w.show();
+    QApplication::setActiveWindow(&w);
+    QVERIFY(QTest::qWaitForWindowActive(&w));
+
+    QSignalSpy activatedSpy(&cbox, QOverload<int>::of(&QComboBox::activated));
+
+    // Navigate + enter activates only once (first item)
+    cbox.lineEdit()->clear();
+    QTest::keyClick(&cbox, Qt::Key_F);
+    QVERIFY(cbox.completer()->popup());
+    QTRY_VERIFY(cbox.completer()->popup()->isVisible());
+    QTest::keyClick(cbox.completer()->popup(), Qt::Key_Down);
+    QTest::keyClick(cbox.completer()->popup(), Qt::Key_Return);
+    QTRY_COMPARE(activatedSpy.count(), 1);
+
+    // Navigate + enter activates only once (non-first item)
+    cbox.lineEdit()->clear();
+    activatedSpy.clear();
+    QTest::keyClick(&cbox, Qt::Key_H);
+    QVERIFY(cbox.completer()->popup());
+    QTRY_VERIFY(cbox.completer()->popup()->isVisible());
+    QTest::keyClick(cbox.completer()->popup(), Qt::Key_Down);
+    QTest::keyClick(cbox.completer()->popup(), Qt::Key_Return);
+    QTRY_COMPARE(activatedSpy.count(), 1);
+
+    // Full text + enter activates only once
+    cbox.lineEdit()->clear();
+    activatedSpy.clear();
+    QTest::keyClicks(&cbox, "foobar1");
+    QVERIFY(cbox.completer()->popup());
+    QTRY_VERIFY(cbox.completer()->popup()->isVisible());
+    QTest::keyClick(&cbox, Qt::Key_Return);
+    QTRY_COMPARE(activatedSpy.count(), 1);
+}
+
 QTEST_MAIN(tst_QCompleter)
 #include "tst_qcompleter.moc"