diff options
| -rw-r--r-- | docs/superpowers/specs/2026-08-13-rule-builder-design.md | 17 | ||||
| -rw-r--r-- | src/tagrulesdialog.cpp | 29 | ||||
| -rw-r--r-- | src/tagrulesdialog.h | 6 | ||||
| -rw-r--r-- | tests/test_tagrules.cpp | 44 |
4 files changed, 88 insertions, 8 deletions
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" |