From 4c025ca9c73bda320f0e7577a4fbd141c31d7c8c Mon Sep 17 00:00:00 2001 From: Gabriel de Dietrich Date: Mon, 28 Aug 2017 10:04:35 +0700 Subject: [PATCH] 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 --- src/widgets/widgets/qcombobox.cpp | 23 +++- src/widgets/widgets/qwidgetlinecontrol.cpp | 10 -- .../util/qcompleter/tst_qcompleter.cpp | 105 ++++++++++++++++++ 3 files changed, 126 insertions(+), 12 deletions(-) diff --git a/src/widgets/widgets/qcombobox.cpp b/src/widgets/widgets/qcombobox.cpp index 54d33a7eea..e0bc198d2e 100644 --- a/src/widgets/widgets/qcombobox.cpp +++ b/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); diff --git a/src/widgets/widgets/qwidgetlinecontrol.cpp b/src/widgets/widgets/qwidgetlinecontrol.cpp index 32944b3918..4f4a6f70b5 100644 --- a/src/widgets/widgets/qwidgetlinecontrol.cpp +++ b/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 } diff --git a/tests/auto/widgets/util/qcompleter/tst_qcompleter.cpp b/tests/auto/widgets/util/qcompleter/tst_qcompleter.cpp index f8095badb8..3818b83584 100644 --- a/tests/auto/widgets/util/qcompleter/tst_qcompleter.cpp +++ b/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::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::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"