diff options
| -rw-r--r-- | CLAUDE.md | 15 | ||||
| -rw-r--r-- | src/tagdialog.cpp | 71 | ||||
| -rw-r--r-- | tests/test_tagdialog.cpp | 80 |
3 files changed, 165 insertions, 1 deletions
@@ -94,6 +94,21 @@ via `MainWindow::uiStatePath()`. Never write window blobs into the hand-edited c Build the path from `QStandardPaths::GenericStateLocation`, not `StateLocation`: the latter appends both the organization and the application name, and both are `qtmaildir`. +**`QLineEdit::setCompleter` is wrong for any field holding more than one +value.** It hands completion to the line edit, which then overwrites the +completer's `completionPrefix` with the widget's **entire text** on every +keystroke. In a field holding a list, the first value completes and nothing +after it ever does, because "unread, fl" is matched whole against the +candidates. Setting the prefix from a `textEdited` handler does not help: the +line edit sets it again afterwards. Use `setCompleter` only for a field whose +whole contents are the thing being completed; otherwise attach with +`QCompleter::setWidget` and drive `setCompletionPrefix` and `complete()` +yourself, and replace the token under the cursor on `activated` rather than +letting QCompleter overwrite the field. This has been hit twice, in +`QueryCompleter` (01ba356) and in `TagDialog`; the trap belongs to Qt, not to +either class. A test that uses `setText()` passes against the bug, since +`setText` does not drive a completer at all: the keys must be typed. + **Do not conclude a key binding is dead from `QTest::keyClick()`.** Whether a symbol needs Shift is a layout property, not a Qt one. `Ctrl++` is the shipped `zoom_in` default and is exactly what the `+` key emits on an Italian layout, while synthetic input never delivers diff --git a/src/tagdialog.cpp b/src/tagdialog.cpp index 94d2e05..75e6b03 100644 --- a/src/tagdialog.cpp +++ b/src/tagdialog.cpp @@ -18,6 +18,7 @@ #include "tagdialog.h" +#include <QAbstractItemView> #include <QCompleter> #include <QCoreApplication> #include <QDialogButtonBox> @@ -97,6 +98,42 @@ QStringList splitTags(const QString &text) return tags; } +/// The tag the cursor sits in, which is what completion should match against. +/// +/// The field holds a comma-separated list, so the last comma before the cursor +/// bounds the token. Leading space is dropped so "unread, fl" completes on "fl" +/// rather than on " fl", which would match nothing. +QString currentToken(const QLineEdit *edit) +{ + const QString text = edit->text(); + const int cursor = qBound(0, edit->cursorPosition(), int(text.size())); + + const int start = text.lastIndexOf(QLatin1Char(','), qMax(0, cursor - 1)) + 1; + return text.mid(start, cursor - start).trimmed(); +} + +/// Overwrites the token under the cursor with `value`, leaving the rest of the +/// list alone, and puts the caret after what was inserted. +void replaceCurrentToken(QLineEdit *edit, const QString &value) +{ + const QString text = edit->text(); + const int cursor = qBound(0, edit->cursorPosition(), int(text.size())); + + const int start = text.lastIndexOf(QLatin1Char(','), qMax(0, cursor - 1)) + 1; + + // Keep the separator's spacing as the user typed it: replacing from `start` + // would eat the space after the comma and give "unread,flagged". + int tokenStart = start; + while (tokenStart < cursor && text.at(tokenStart).isSpace()) + ++tokenStart; + + QString updated = text; + updated.replace(tokenStart, cursor - tokenStart, value); + + edit->setText(updated); + edit->setCursorPosition(tokenStart + value.size()); +} + } // namespace TagDialog::TagDialog(const QStringList &knownTags, @@ -129,7 +166,39 @@ TagDialog::TagDialog(const QStringList &knownTags, // Hierarchies are the reason this matters: typing "amazon" should find // "shopping/amazon". completer->setFilterMode(Qt::MatchContains); - edit->setCompleter(completer); + + // setWidget, NOT QLineEdit::setCompleter. These fields hold a + // comma-separated LIST, and setCompleter makes the line edit drive + // completion, overwriting the prefix with the widget's ENTIRE text on + // every keystroke. Once the field reads "unread, fl" that whole string + // is matched against the tag names, nothing matches, and completion + // silently stops working after the first tag. Setting the prefix from a + // textEdited handler does not help: the line edit sets it again + // afterwards. + // + // setWidget keeps the popup anchored without ceding control of the + // prefix, which then becomes ours to drive per token. Exactly the fix + // QueryCompleter needed in 01ba356; the trap belongs to + // QLineEdit::setCompleter, not to either class. + completer->setWidget(edit); + + connect(edit, &QLineEdit::textEdited, this, [edit, completer]() { + const QString token = currentToken(edit); + completer->setCompletionPrefix(token); + if (token.isEmpty() || completer->completionCount() == 0) { + completer->popup()->hide(); + return; + } + completer->complete(); + }); + + // Accepting a candidate has to replace the token under the cursor + // rather than the whole field, or taking "flagged" would discard every + // tag already typed. + connect(completer, QOverload<const QString &>::of(&QCompleter::activated), + this, [edit](const QString &value) { + replaceCurrentToken(edit, value); + }); } form->addRow(tr("Add:"), m_addEdit); diff --git a/tests/test_tagdialog.cpp b/tests/test_tagdialog.cpp index f41850e..df0fece 100644 --- a/tests/test_tagdialog.cpp +++ b/tests/test_tagdialog.cpp @@ -19,6 +19,7 @@ #include <QtTest> #include <QCheckBox> +#include <QCompleter> #include <QLineEdit> #include <QListWidget> @@ -42,6 +43,8 @@ private slots: void aPartialTagLeftAloneChangesNothing(); void aPartialTagCheckedIsAddedEverywhere(); void nothingTouchedYieldsNoChange(); + void completionFollowsTheTagAfterAComma(); + void acceptingACandidateKeepsTheOtherTags(); }; void TestTagDialog::validNamesAreAccepted() @@ -239,5 +242,82 @@ void TestTagDialog::nothingTouchedYieldsNoChange() QVERIFY(dialog.tagsToRemove().isEmpty()); } +void TestTagDialog::completionFollowsTheTagAfterAComma() +{ + // Reported by the user: the first tag completes, the second does not. + // + // QLineEdit::setCompleter matches against the widget's ENTIRE text, so once + // the field reads "unread, fl" that whole string becomes the completion + // prefix and nothing matches. The completer has to be driven on the token + // under the cursor instead. This is the same defect QueryCompleter hit in + // 01ba356, in a second place. + // + // Typed rather than setText(): setText does not drive a completer at all, + // so a test using it passes against the broken code. + TagDialog dialog({ QStringLiteral("inbox"), QStringLiteral("unread"), + QStringLiteral("flagged") }, {}, 1); + dialog.show(); + QVERIFY(QTest::qWaitForWindowExposed(&dialog)); + + const QList<QLineEdit *> edits = dialog.findChildren<QLineEdit *>(); + QVERIFY(edits.size() >= 2); + QLineEdit *addEdit = edits.at(0); + addEdit->setFocus(); + QTRY_COMPARE(QApplication::focusWidget(), addEdit); + + // findChild, not QLineEdit::completer(): the completer is attached with + // setWidget() rather than setCompleter(), for the reason the fix documents, + // so the line edit does not report one. It is parented to the edit, which + // is what makes it reachable here. + QCompleter *completer = addEdit->findChild<QCompleter *>(); + QVERIFY(completer); + + // First tag: this much always worked. + QTest::keyClicks(addEdit, QStringLiteral("un")); + QCOMPARE(completer->completionPrefix(), QStringLiteral("un")); + QVERIFY(completer->completionCount() > 0); + + // Second tag, after a comma and a space. The prefix must be the new token, + // not the whole line. + QTest::keyClicks(addEdit, QStringLiteral("read, fl")); + QCOMPARE(addEdit->text(), QStringLiteral("unread, fl")); + + QCOMPARE(completer->completionPrefix(), QStringLiteral("fl")); + QVERIFY2(completer->completionCount() > 0, + "no candidate for the tag after the comma: the completer is " + "matching against the whole line"); +} + +void TestTagDialog::acceptingACandidateKeepsTheOtherTags() +{ + // Driving the prefix per token is only half the fix. Accepting a candidate + // has to overwrite that token too: QCompleter's own insertion replaces the + // whole field, so taking "flagged" here would discard "unread" with it. + TagDialog dialog({ QStringLiteral("inbox"), QStringLiteral("unread"), + QStringLiteral("flagged") }, {}, 1); + dialog.show(); + QVERIFY(QTest::qWaitForWindowExposed(&dialog)); + + const QList<QLineEdit *> edits = dialog.findChildren<QLineEdit *>(); + QVERIFY(edits.size() >= 2); + QLineEdit *addEdit = edits.at(0); + addEdit->setFocus(); + QTRY_COMPARE(QApplication::focusWidget(), addEdit); + + QCompleter *completer = addEdit->findChild<QCompleter *>(); + QVERIFY(completer); + + QTest::keyClicks(addEdit, QStringLiteral("unread, fl")); + QCOMPARE(completer->completionPrefix(), QStringLiteral("fl")); + + // What clicking a row emits. + emit completer->activated(QStringLiteral("flagged")); + + QCOMPARE(addEdit->text(), QStringLiteral("unread, flagged")); + // And the separator's spacing survives: replacing from the comma itself + // would have produced "unread,flagged". + QVERIFY(addEdit->text().contains(QStringLiteral(", "))); +} + QTEST_MAIN(TestTagDialog) #include "test_tagdialog.moc" |