From cff230c8e9dba7a95c8ee28932b7a299fc0d994a Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 11:10:35 +0200 Subject: docs: correct the claim that a paren-bearing value is unrepresentable The spec listed from:(((( among the queries the parser must reject, and the plan's Task 6 asserted that rejection. Probing the built parser shows it accepts the query as a From row whose value is the literal text, and compiles it back byte for byte. That is correct behaviour, not a leak in the strictness rule. notmuch reads those parens as characters to search for rather than as grouping, so the query is meaningful and the row displaying it tells the truth. Rejecting it would buy nothing and would push a representable rule into text mode. The distinction the documents were missing: a parenthesis inside a VALUE is not a shape question at all, only a parenthesis in grouping position is. Restate both documents accordingly, and replace the assertion with a round-trip one, which is the property that actually matters here. --- .../specs/2026-08-13-rule-builder-design.md | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) (limited to 'docs/superpowers/specs') diff --git a/docs/superpowers/specs/2026-08-13-rule-builder-design.md b/docs/superpowers/specs/2026-08-13-rule-builder-design.md index c11ef47..a571506 100644 --- a/docs/superpowers/specs/2026-08-13-rule-builder-design.md +++ b/docs/superpowers/specs/2026-08-13-rule-builder-design.md @@ -221,7 +221,12 @@ an empty query and must open in the builder ready to receive a row. Everything else sets `parsed = false`: nested parens beyond that one shape, mixed `and`/`or` without parens, `xor`, an unrecognised prefix (`body:`, `mid:`, -`folder:`), a bare word with no prefix, a `path:` not ending in `/**`. +`folder:`), a bare word with no prefix, a `path:` not ending in `/**`, a +two-sided `date:` range, a trailing operator, an unterminated quote. + +A parenthesis **inside a value** is not a shape at all: `from:((((` is a From +term whose value happens to contain parens, and it parses and round-trips like +any other. Only a parenthesis in grouping position is a shape question. ### The parser is strict, and that is the safety property @@ -334,10 +339,16 @@ terms and tests exactly the same thing. **Rejection tests**, which carry the safety property. Queries that must set `parsed = false` and must not partially parse: nested `or` inside `or`, mixed -`and`/`or` without parens, `body:foo`, a bare word, a `path:` without `/**`, and -`from:((((`. That last asserts **our** rejection, never a provoked notmuch -failure: `CLAUDE.md` records twice that notmuch accepts it cleanly, and a test -expecting an error there fails against correct code. +`and`/`or` without parens, `body:foo`, a bare word, a `path:` without `/**`, a +two-sided `date:` range, and a trailing operator. + +**`from:((((` is not among them, and the reason is worth stating.** notmuch +treats those parens as characters to search for rather than as grouping, so the +query is meaningful, matches nothing, and reports no error. This parser accepts +it as a From row whose value is that literal text, which is what it means. The +assertion there is the **round trip**, never a rejection and never a provoked +notmuch failure: `CLAUDE.md` records twice that notmuch accepts it cleanly, and +a test expecting an error fails against correct code. **Compile tests** for every field and operator pair including both negations, and the parenthesisation rule at its boundary: `join == Any` with zero -- cgit v1.2.3 From e7cdd7fd3ae960572976cb4a053ecf192baa17c2 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 11:59:30 +0200 Subject: fix(rules): keep the text-mode toggle reachable Ticking "Edit as text" was a one-way trip: the only way back to the rows was closing the dialog and reopening it. The checkbox was parented to the builder widget and sat on the match row, and switching to text mode hides that widget, so the toggle disappeared along with the rows it governs. Move it to the query row, which is visible in both modes. The existing tests all passed against this, because they drove the toggle through setChecked and then asserted on the checked STATE. A hidden checkbox reports its state perfectly well, so every one of those assertions held while the widget was unreachable. The new test asks the question that matters, whether the toggle would be on screen, and it uses isVisibleTo since nothing is isVisible on a dialog that was never shown. Worth recording how close the mutation check came to endorsing this too. Reparenting the checkbox alone left it in the query row's layout, so it stayed visible and the test still passed. Only restoring the full shipped shape, parent and layout together, reproduced the fault and failed the test. A mutation that does not reproduce the original bug proves nothing about the test that is meant to catch it. The spec's layout sketch carried the same error and is corrected, with the reason, so the next reader does not reintroduce it. --- .../specs/2026-08-13-rule-builder-design.md | 17 ++++++++- src/tagrulesdialog.cpp | 29 +++++++++++--- src/tagrulesdialog.h | 6 +++ tests/test_tagrules.cpp | 44 ++++++++++++++++++++++ 4 files changed, 88 insertions(+), 8 deletions(-) (limited to 'docs/superpowers/specs') diff --git a/docs/superpowers/specs/2026-08-13-rule-builder-design.md b/docs/superpowers/specs/2026-08-13-rule-builder-design.md index a571506..4d6c001 100644 --- a/docs/superpowers/specs/2026-08-13-rule-builder-design.md +++ b/docs/superpowers/specs/2026-08-13-rule-builder-design.md @@ -275,7 +275,7 @@ The builder replaces the query line edit. Everything else in the form stays. ``` Id [ vendor-receipts ] Stage [ 50 ] [x] Applied on every sync -Match (o) all ( ) any [ ] Edit as text +Match (o) all ( ) any [From v] [contains v] [vendor.example.org ] [+] [-] [From v] [contains v] [vendor.example.net ] [+] [-] But not @@ -287,9 +287,22 @@ Add tags [ vendor, receipts ] Remove tags [ ] Note [ ... ] -Query (from:vendor.example.org or ...) and not subject:receipt [Count matches] +Query (from:vendor.example.org or ...) and not subject:receipt + [ ] Edit as text ``` +**The "Edit as text" toggle belongs to the QUERY row, not to the match row.** +An earlier draft of this sketch put it beside the all/any radios, which is +where it reads best and is also wrong: switching to text mode hides the +builder, and a checkbox living inside the builder disappears with it, leaving +no way back except closing the dialog. That shipped and a hand test found it +within minutes. The query row is visible in both modes, so a toggle there is +always reachable. + +The test for this must assert **reachability**, not the checked state. A +hidden checkbox reports its state perfectly well, so a state assertion passes +against the broken layout. + **The query line stays visible in builder mode, read-only.** It is what ships to the hook, and watching it update as rows change is what makes the builder trustworthy rather than a black box. In text mode the same widget becomes diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index c81450c..75a9cbe 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -131,14 +131,9 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) auto *matchGroup = new QButtonGroup(this); matchGroup->addButton(m_matchAll); matchGroup->addButton(m_matchAny); - m_textMode = new QCheckBox(tr("Edit as &text"), m_builder); - m_textMode->setToolTip( - tr("Edit the notmuch query directly. A rule too complex to show as " - "rows opens this way.")); matchRow->addWidget(m_matchAll); matchRow->addWidget(m_matchAny); matchRow->addStretch(); - matchRow->addWidget(m_textMode); builderLayout->addLayout(matchRow); m_rowsLayout = new QVBoxLayout; @@ -153,7 +148,21 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) builderLayout->addWidget(m_addExclusion, 0, Qt::AlignLeft); form->addRow(tr("Match"), m_builder); - form->addRow(tr("Query"), m_query); + + // The toggle sits with the QUERY line, not inside m_builder, because + // switching to text mode HIDES m_builder. A checkbox parented there + // vanishes with the rows it governs, leaving no way back except closing + // the dialog, which is exactly what shipped in the first draft of this + // builder. The query row is visible in both modes, so the toggle is + // always reachable. + auto *queryRow = new QHBoxLayout; + m_textMode = new QCheckBox(tr("Edit as &text"), this); + m_textMode->setToolTip( + tr("Edit the notmuch query directly. A rule too complex to show as " + "rows opens this way.")); + queryRow->addWidget(m_query, 1); + queryRow->addWidget(m_textMode); + form->addRow(tr("Query"), queryRow); // The query line shows what the rows compile to. Read-only in builder // mode: it is what actually ships to the hook, and watching it change is @@ -475,6 +484,14 @@ void TagRulesDialog::setTextModeForTest(bool on) m_textMode->setChecked(on); } +bool TagRulesDialog::textModeToggleIsReachableForTest() const +{ + // isVisibleTo rather than isVisible: nothing is isVisible() on a dialog + // that was never shown, so that would report unreachable in both the + // working and the broken case. + return m_textMode->isVisibleTo(this); +} + QString TagRulesDialog::warningTextForTest() const { // isVisible() is false for every child of a dialog that was never shown, diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index c91dc0c..9559918 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -82,6 +82,12 @@ public: void setTextModeForTest(bool on); QString warningTextForTest() const; + /// Whether the text-mode toggle would be on screen. A toggle that hides + /// itself when switched on is a one-way trip, and asserting only on the + /// checked STATE passes against that, since the state is still readable + /// when the widget is not. + bool textModeToggleIsReachableForTest() const; + signals: /// Asks the owner to run countQueries() through the worker. void countsRequested(); diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index 8207b3c..cf7dadb 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -48,6 +48,7 @@ private slots: void aTextModeRuleStaysTextWhenAnotherRuleIsVisited(); void leavingTextModeIsRefusedWhenTheQueryCannotBeShownAsRows(); void aFolderRowUsesTheDropdownAndKeepsItsSuffix(); + void theTextModeToggleSurvivesBeingSwitchedOn(); private: QString writeRules(const QString &json); @@ -590,5 +591,48 @@ void TestTagRules::aFolderRowUsesTheDropdownAndKeepsItsSuffix() QStringLiteral("path:\"account-two/**\"")); } +void TestTagRules::theTextModeToggleSurvivesBeingSwitchedOn() +{ + // The toggle governs the builder, so it must not live INSIDE the builder: + // switching to text mode hides that widget, and a checkbox parented there + // disappears along with the rows, leaving no way back except closing the + // dialog. That shipped in the first draft and a user found it by hand. + // + // Asserting on the checked state alone passes against the bug, because a + // hidden widget still reports its state perfectly well. The question is + // reachability. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + QFile out(configHome.filePath(QStringLiteral("mailrules/rules.json"))); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(R"({ + "version": 1, + "rules": [ + {"id": "vendor", "query": "from:vendor.example.org", + "add": ["vendor"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + QVERIFY(dialog.textModeToggleIsReachableForTest()); + + dialog.setTextModeForTest(true); + QVERIFY2(dialog.textModeToggleIsReachableForTest(), + "the toggle must survive switching to text, or there is no " + "way back to the rows"); + + // And the round trip works, which is the behaviour the user wanted. + dialog.setTextModeForTest(false); + QVERIFY(!dialog.textModeForTest()); + QVERIFY(dialog.textModeToggleIsReachableForTest()); + QCOMPARE(dialog.rowCountForTest(), 1); + QCOMPARE(dialog.queryLineForTest(), + QStringLiteral("from:vendor.example.org")); +} + QTEST_MAIN(TestTagRules) #include "test_tagrules.moc" -- cgit v1.2.3