diff options
| -rw-r--r-- | src/querycompleter.cpp | 14 | ||||
| -rw-r--r-- | src/querycompleter.h | 5 | ||||
| -rw-r--r-- | tests/test_querycompleter.cpp | 138 |
3 files changed, 156 insertions, 1 deletions
diff --git a/src/querycompleter.cpp b/src/querycompleter.cpp index a6a1aeb..7e9dfd9 100644 --- a/src/querycompleter.cpp +++ b/src/querycompleter.cpp @@ -422,6 +422,12 @@ bool QueryCompleter::eventFilter(QObject *watched, QEvent *event) if (event->type() != QEvent::KeyPress || !popupVisible()) return QObject::eventFilter(watched, event); + // A key this filter is itself redelivering. sendEvent re-runs application + // event filters, so without this the forwarded key comes straight back and + // recurses until the stack is gone. + if (m_forwarding) + return QObject::eventFilter(watched, event); + auto *keyEvent = static_cast<QKeyEvent *>(event); switch (keyEvent->key()) { case Qt::Key_Tab: @@ -452,11 +458,17 @@ bool QueryCompleter::eventFilter(QObject *watched, QEvent *event) case Qt::Key_Up: case Qt::Key_Down: case Qt::Key_PageUp: - case Qt::Key_PageDown: + case Qt::Key_PageDown: { // Navigation belongs to the popup, which is not the focus widget while // the user is typing in the bar. + // + // m_forwarding is what keeps this from recursing; see the guard at the + // top of the filter. + m_forwarding = true; QCoreApplication::sendEvent(m_popup, event); + m_forwarding = false; return true; + } default: break; } diff --git a/src/querycompleter.h b/src/querycompleter.h index 6f658f2..27161bc 100644 --- a/src/querycompleter.h +++ b/src/querycompleter.h @@ -135,6 +135,11 @@ private: const Config &m_config; QStringList m_tags; + /// Set while the filter is redelivering a key to the popup. The filter is + /// installed on the application and sendEvent re-runs application filters, + /// so without this the forwarded key returns to the filter that sent it. + bool m_forwarding = false; + QCompleter *m_completer = nullptr; QStandardItemModel *m_model = nullptr; CompletionPopup *m_popup = nullptr; diff --git a/tests/test_querycompleter.cpp b/tests/test_querycompleter.cpp index c1d7530..09eedc3 100644 --- a/tests/test_querycompleter.cpp +++ b/tests/test_querycompleter.cpp @@ -73,6 +73,14 @@ private slots: void acceptingAPrefixReopensThePopupForValues(); void acceptingAValueDoesNotReopenAnEmptyPopup(); void keysFallThroughWhileThePopupIsHidden(); + + // The application filter re-entrancy bug: sendEvent() re-runs application + // event filters, so forwarding a key to the popup from inside the filter + // hands it straight back and the recursion only ends in a stack overflow. + void arrowNavigationDoesNotRecurse(); + void returnRunsTheQueryOnceCompletionIsDone(); + void returnRunsTheQueryAfterAMouseAccept(); + void returnRunsTheQueryWhenThePopupMatchesNothing(); }; // Copied from tests/test_config.cpp rather than shared, so the two test files @@ -643,5 +651,135 @@ void TestQueryCompleter::keysFallThroughWhileThePopupIsHidden() QVERIFY(ran); } +void TestQueryCompleter::arrowNavigationDoesNotRecurse() +{ + // QCoreApplication::sendEvent re-runs application-level event filters, so a + // filter that forwards the key it just claimed to another widget is handed + // the same key back. With the popup still visible the guard still passes and + // it forwards again: unbounded recursion, and the process dies on the stack + // rather than on any assertion. Reaching the end of this test is the check. + Config config; + QLineEdit edit; + edit.show(); + QVERIFY(QTest::qWaitForWindowExposed(&edit)); + edit.setFocus(); + QueryCompleter completer(&edit, config); + + QTest::keyClicks(&edit, QStringLiteral("t")); + QVERIFY(findPopup() && findPopup()->isVisible()); + + QTest::keyClick(keyboardTarget(&edit), Qt::Key_Down); + QTest::keyClick(keyboardTarget(&edit), Qt::Key_Down); + + // And navigation actually moved, so the fix is not "swallow the key". + QListView *popup = findPopup(); + QVERIFY(popup); + QVERIFY(popup->currentIndex().isValid()); + QCOMPARE(popup->currentIndex().row(), 1); +} + +void TestQueryCompleter::returnRunsTheQueryOnceCompletionIsDone() +{ + // The second half of the user's report: with the query finished, Return has + // to reach returnPressed and run it. A popup left visible over a completed + // term swallows Return forever, and the query can never be run from the + // keyboard at all. + Config config; + QLineEdit edit; + edit.show(); + QVERIFY(QTest::qWaitForWindowExposed(&edit)); + edit.setFocus(); + QueryCompleter completer(&edit, config); + completer.setTags({ QStringLiteral("unread"), QStringLiteral("inbox") }); + + bool ran = false; + connect(&edit, &QLineEdit::returnPressed, &edit, [&ran]() { ran = true; }); + + // Drive the whole chain the way the user does: prefix, accept, value, accept. + QTest::keyClicks(&edit, QStringLiteral("t")); + QVERIFY(findPopup() && findPopup()->isVisible()); + QTest::keyClick(keyboardTarget(&edit), Qt::Key_Tab); + QCOMPARE(edit.text(), QStringLiteral("tag:")); + + QTest::keyClicks(&edit, QStringLiteral("un")); + QVERIFY(findPopup() && findPopup()->isVisible()); + QTest::keyClick(keyboardTarget(&edit), Qt::Key_Tab); + QCOMPARE(edit.text(), QStringLiteral("tag:unread")); + + // The query is complete. Return must now run it, not be eaten. + QTest::keyClick(keyboardTarget(&edit), Qt::Key_Return); + QVERIFY(ran); +} + +void TestQueryCompleter::returnRunsTheQueryAfterAMouseAccept() +{ + // The user builds the whole query with the mouse, which never goes through + // the key filter, and then Return does not run it. Clicking a row is what + // QCompleter reports as activated(), so drive that and then press Return + // exactly as the user does. + Config config; + QLineEdit edit; + edit.show(); + QVERIFY(QTest::qWaitForWindowExposed(&edit)); + edit.setFocus(); + QueryCompleter completer(&edit, config); + completer.setTags({ QStringLiteral("unread"), QStringLiteral("inbox") }); + + bool ran = false; + connect(&edit, &QLineEdit::returnPressed, &edit, [&ran]() { ran = true; }); + + QTest::keyClicks(&edit, QStringLiteral("t")); + QListView *popup = findPopup(); + QVERIFY(popup && popup->isVisible()); + + // Click the "tag:" row. + const QModelIndex prefixRow = popup->model()->index(0, 0); + QVERIFY(prefixRow.isValid()); + popup->setCurrentIndex(prefixRow); + QTest::mouseClick(popup->viewport(), Qt::LeftButton, Qt::NoModifier, + popup->visualRect(prefixRow).center()); + QCOMPARE(edit.text(), QStringLiteral("tag:")); + + // Then click a tag value in the popup the accept chained open. + popup = findPopup(); + QVERIFY(popup && popup->isVisible()); + const QModelIndex valueRow = popup->model()->index(0, 0); + QVERIFY(valueRow.isValid()); + popup->setCurrentIndex(valueRow); + QTest::mouseClick(popup->viewport(), Qt::LeftButton, Qt::NoModifier, + popup->visualRect(valueRow).center()); + QCOMPARE(edit.text(), QStringLiteral("tag:unread")); + + // The query is complete and built entirely with the mouse. Return runs it. + QTest::keyClick(keyboardTarget(&edit), Qt::Key_Return); + QVERIFY(ran); +} + +void TestQueryCompleter::returnRunsTheQueryWhenThePopupMatchesNothing() +{ + // A query the user finished by hand. The bar is mid-token, so the popup is + // still up, but nothing in it matches what was typed. Return must run the + // query: there is no completion to accept, and accepting the first row of + // an unrelated list would rewrite the query the user just wrote. + Config config; + QLineEdit edit; + edit.show(); + QVERIFY(QTest::qWaitForWindowExposed(&edit)); + edit.setFocus(); + QueryCompleter completer(&edit, config); + completer.setTags({ QStringLiteral("unread"), QStringLiteral("inbox") }); + + bool ran = false; + connect(&edit, &QLineEdit::returnPressed, &edit, [&ran]() { ran = true; }); + + // "zzz" matches no tag, so the popup has nothing to offer for it. + QTest::keyClicks(&edit, QStringLiteral("tag:zzz")); + + QTest::keyClick(keyboardTarget(&edit), Qt::Key_Return); + + QCOMPARE(edit.text(), QStringLiteral("tag:zzz")); + QVERIFY(ran); +} + QTEST_MAIN(TestQueryCompleter) #include "test_querycompleter.moc" |