fix(completion): filter keys on the application, not the line edit

Showing the popup takes focus away from the query bar and the popup
window grabs the keyboard, so keys pressed while it is up are delivered
to the popup. An event filter installed on the line edit therefore never
ran at the one moment it had to, leaving Tab to move focus to the next
widget and Return to reach the thread list and open a message.

Install the key filter on the application instead, which sees events
before any widget receives them. It returns immediately unless our own
popup is visible, so it cannot affect keyboard handling elsewhere. The
FocusIn filter stays on the line edit, where it is correctly scoped: it
only fires with the popup down.

Accepting a completion now also reopens the popup when the caret lands
somewhere more can be offered, so taking "tag:" goes straight on to the
tag list instead of needing a second complete_query. The chain stops on
a stem that is already a complete candidate, which is what every accept
produces. The mouse path chains identically.

The previous tests passed against the broken code because they posted
events straight to the line edit, bypassing the delivery path a real
keypress takes. The new tests route keys through the active popup and
run against a real X display; offscreen does not grab the keyboard and
cannot reproduce this class of bug.

3 files changed, 208 insertions, 3 deletions

diff --git a/src/querycompleter.cpp b/src/querycompleter.cpp
index b4968f5..a6a1aeb 100644
--- a/src/querycompleter.cpp
+++ b/src/querycompleter.cpp
@@ -367,12 +367,27 @@ QueryCompleter::QueryCompleter(QLineEdit *edit, const Config &config,
 
     connect(m_completer, QOverload<const QModelIndex &>::of(&QCompleter::activated),
             this, [this](const QModelIndex &index) {
+        // The mouse path. It must chain exactly like Tab does: the user's
+        // report was that clicking "tag:" offered no tags afterwards.
         acceptCompletion(index.data(Qt::DisplayRole).toString());
+        continueCompletion();
     });
 
-    // Always filtered, not only for completion_on_focus: the popup is ours to
-    // drive now, so Tab, Enter, Escape and the arrows must be routed by hand.
+    // Two filters, because the two jobs need different vantage points.
+    //
+    // The line edit filter handles FocusIn, which by definition arrives while
+    // the popup is down and the edit is the delivery target, so watching the
+    // widget is both sufficient and correctly scoped.
     m_edit->installEventFilter(this);
+
+    // The key filter must be application-wide. Showing the popup takes focus
+    // away from the line edit (focusWidget() becomes null) and the popup window
+    // grabs the keyboard, so keys pressed while it is up are delivered to the
+    // popup and a filter on the line edit never runs. That is precisely when
+    // Tab and Return need to be intercepted. Only an application filter sees
+    // those events. It is inert unless our own popup is visible.
+    if (QCoreApplication *app = QCoreApplication::instance())
+        app->installEventFilter(this);
 }
 
 void QueryCompleter::triggerCompletion()
@@ -401,6 +416,9 @@ bool QueryCompleter::eventFilter(QObject *watched, QEvent *event)
         return QObject::eventFilter(watched, event);
     }
 
+    // This filter is installed on the application, so it sees every key in the
+    // process. Claim nothing unless our own popup is on screen, otherwise the
+    // keyboard breaks everywhere else in the window.
     if (event->type() != QEvent::KeyPress || !popupVisible())
         return QObject::eventFilter(watched, event);
 
@@ -419,9 +437,13 @@ bool QueryCompleter::eventFilter(QObject *watched, QEvent *event)
             return QObject::eventFilter(watched, event);
 
         acceptCompletion(index.data(Qt::DisplayRole).toString());
+        // Hide before reopening: accepting "tag:" moves the caret into value
+        // position, and the popup has to be rebuilt around the new context
+        // rather than left showing the prefix list.
         m_popup->hide();
+        continueCompletion();
         // Consume it. Tab would otherwise move focus to the next widget, and
-        // Return would run the half-typed query.
+        // Return would run the half-typed query or reach the thread list.
         return true;
     }
     case Qt::Key_Escape:
@@ -474,6 +496,39 @@ void QueryCompleter::acceptCompletion(const QString &value)
     m_context = completionContext(m_edit->text(), m_edit->cursorPosition());
 }
 
+void QueryCompleter::continueCompletion()
+{
+    if (!m_edit || !m_completer)
+        return;
+
+    // acceptCompletion() already recomputed the context from the caret it
+    // placed, so this reads the situation the accept created.
+    if (m_context.kind == CompletionContext::None)
+        return;
+
+    // Reopen only for a value whose keyword actually offers candidates.
+    // Accepting a prefix ("tag:") lands here with an empty stem and the tag
+    // list waiting, which is the case worth reopening for. Accepting a value
+    // ("tag:unread") leaves a stem that already equals the only match, so
+    // reopening would show a one-entry popup that swallows the next Return.
+    if (m_context.kind != CompletionContext::Value)
+        return;
+
+    const QStringList candidates = candidatesFor(m_context);
+    if (candidates.isEmpty())
+        return;
+
+    // A stem that is already a complete candidate needs nothing more. This is
+    // also what stops the reopen from recurring: the next accept always
+    // produces such a stem, so the chain terminates after one step.
+    if (candidates.contains(m_context.stem, Qt::CaseInsensitive))
+        return;
+
+    rebuildModel(m_context);
+    m_completer->setCompletionPrefix(m_context.stem);
+    m_completer->complete();
+}
+
 void QueryCompleter::updateContext()
 {
     if (!m_edit)
diff --git a/src/querycompleter.h b/src/querycompleter.h
index 350b21c..6f658f2 100644
--- a/src/querycompleter.h
+++ b/src/querycompleter.h
@@ -124,6 +124,10 @@ private:
     /// otherwise.
     bool popupVisible() const;
 
+    /// Reopens the popup when an accepted completion leaves the caret somewhere
+    /// more can be offered, so taking "tag:" goes straight on to the tag list.
+    void continueCompletion();
+
     QList<CompletionEntry> entriesFor(const CompletionContext &context) const;
     void rebuildModel(const CompletionContext &context);
 
diff --git a/tests/test_querycompleter.cpp b/tests/test_querycompleter.cpp
index 5ae9312..c1d7530 100644
--- a/tests/test_querycompleter.cpp
+++ b/tests/test_querycompleter.cpp
@@ -63,6 +63,16 @@ private slots:
     void tabIsIgnoredWhileThePopupIsHidden();
     void returnIsIgnoredWhileThePopupIsHidden();
     void focusOpensThePopupOnlyWhenConfigured();
+
+    // Delivered to the widget the window system actually gives the key to,
+    // rather than straight to the line edit. While the popup is up that is the
+    // popup, which has grabbed the keyboard, and a filter on the line edit
+    // never runs. Sending to the edit hides exactly the bug the user reports.
+    void tabAcceptsWhenTheKeyGoesToTheGrabbingPopup();
+    void returnAcceptsWhenTheKeyGoesToTheGrabbingPopup();
+    void acceptingAPrefixReopensThePopupForValues();
+    void acceptingAValueDoesNotReopenAnEmptyPopup();
+    void keysFallThroughWhileThePopupIsHidden();
 };
 
 // Copied from tests/test_config.cpp rather than shared, so the two test files
@@ -497,5 +507,141 @@ void TestQueryCompleter::focusOpensThePopupOnlyWhenConfigured()
     }
 }
 
+// The widget the window system would hand the next key to. While the popup is
+// up it has grabbed the keyboard, so that is the popup and NOT the line edit,
+// which has by then lost focus entirely. Routing test keys through here is what
+// makes these tests reproduce the user's experience instead of a synthetic one.
+static QWidget *keyboardTarget(QLineEdit *edit)
+{
+    if (QWidget *popup = QApplication::activePopupWidget())
+        return popup;
+    return edit;
+}
+
+void TestQueryCompleter::tabAcceptsWhenTheKeyGoesToTheGrabbingPopup()
+{
+    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_Tab);
+
+    QCOMPARE(edit.text(), QStringLiteral("tag:"));
+}
+
+void TestQueryCompleter::returnAcceptsWhenTheKeyGoesToTheGrabbingPopup()
+{
+    // Return must be consumed too, or it reaches the thread list and opens a
+    // thread, which is what the user sees.
+    Config config;
+    QLineEdit edit;
+    edit.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&edit));
+    edit.setFocus();
+    QueryCompleter completer(&edit, config);
+
+    bool ran = false;
+    connect(&edit, &QLineEdit::returnPressed, &edit, [&ran]() { ran = true; });
+
+    QTest::keyClicks(&edit, QStringLiteral("t"));
+    QVERIFY(findPopup() && findPopup()->isVisible());
+
+    QTest::keyClick(keyboardTarget(&edit), Qt::Key_Return);
+
+    QCOMPARE(edit.text(), QStringLiteral("tag:"));
+    QVERIFY(!ran);
+}
+
+void TestQueryCompleter::acceptingAPrefixReopensThePopupForValues()
+{
+    // The user's third complaint: after taking "tag:" the caret sits where a
+    // tag value goes, so the values must be offered without a second Ctrl+Space.
+    Config config;
+    QLineEdit edit;
+    edit.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&edit));
+    edit.setFocus();
+    QueryCompleter completer(&edit, config);
+    completer.setTags({ QStringLiteral("unread"), QStringLiteral("inbox") });
+
+    QTest::keyClicks(&edit, QStringLiteral("t"));
+    QVERIFY(findPopup() && findPopup()->isVisible());
+
+    QTest::keyClick(keyboardTarget(&edit), Qt::Key_Tab);
+    QCOMPARE(edit.text(), QStringLiteral("tag:"));
+
+    QListView *popup = findPopup();
+    QVERIFY(popup);
+    QVERIFY(popup->isVisible());
+    QStringList offered;
+    for (int row = 0; row < popup->model()->rowCount(); ++row)
+        offered << popup->model()->index(row, 0).data().toString();
+    QCOMPARE(offered, QStringList({ QStringLiteral("unread"),
+                                    QStringLiteral("inbox") }));
+
+    // And the chain completes: typing into the reopened popup and accepting
+    // yields the finished term.
+    QTest::keyClicks(&edit, QStringLiteral("un"));
+    QVERIFY(findPopup() && findPopup()->isVisible());
+    QTest::keyClick(keyboardTarget(&edit), Qt::Key_Tab);
+    QCOMPARE(edit.text(), QStringLiteral("tag:unread"));
+}
+
+void TestQueryCompleter::acceptingAValueDoesNotReopenAnEmptyPopup()
+{
+    // "tag:unread" is complete. Reopening here would put an empty list under
+    // the caret and swallow the next Return.
+    Config config;
+    QLineEdit edit;
+    edit.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&edit));
+    edit.setFocus();
+    QueryCompleter completer(&edit, config);
+    completer.setTags({ QStringLiteral("unread") });
+
+    QTest::keyClicks(&edit, QStringLiteral("tag:un"));
+    QVERIFY(findPopup() && findPopup()->isVisible());
+
+    QTest::keyClick(keyboardTarget(&edit), Qt::Key_Tab);
+
+    QCOMPARE(edit.text(), QStringLiteral("tag:unread"));
+    QVERIFY(!findPopup() || !findPopup()->isVisible());
+}
+
+void TestQueryCompleter::keysFallThroughWhileThePopupIsHidden()
+{
+    // The filter is application-wide, so proving it does nothing with the popup
+    // down is what keeps it from breaking the rest of the application.
+    Config config;
+    QLineEdit edit;
+    edit.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&edit));
+    edit.setFocus();
+    QueryCompleter completer(&edit, config);
+
+    if (QListView *popup = findPopup())
+        popup->hide();
+
+    QLineEdit other;
+    other.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&other));
+    other.setFocus();
+
+    bool ran = false;
+    connect(&other, &QLineEdit::returnPressed, &other, [&ran]() { ran = true; });
+
+    QTest::keyClicks(&other, QStringLiteral("hello"));
+    QTest::keyClick(&other, Qt::Key_Return);
+
+    QCOMPARE(other.text(), QStringLiteral("hello"));
+    QVERIFY(ran);
+}
+
 QTEST_MAIN(TestQueryCompleter)
 #include "test_querycompleter.moc"