From d10a5860b290ee35f657e95e509c8dac7608f809 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 11:26:30 +0200 Subject: feat(rules): load a rule into the builder rows Selecting a rule now parses its stored query and rebuilds the builder rows from it, and a row edit compiles back onto the query line and into the working copy. Populating the form was already able to write the rule just loaded over whichever rule is current: m_enabled's toggled runs applyEditsToCurrentRule while m_query still holds the previous rule's text, which emptied the first rule's query on open. The existing m_reloading guard now covers the whole load rather than one signal blocker on the note, which also covers the combo boxes rebuildRows populates. --- tests/test_tagrules.cpp | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) (limited to 'tests/test_tagrules.cpp') diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index 8c7e1a9..fa5c0b5 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -19,7 +19,9 @@ #include #include +#include "rulequery.h" #include "tagrules.h" +#include "tagrulesdialog.h" /// The risk in TagRules is the format, not painting: a field silently dropped /// on save mis-tags real mail on the next sync, and does it quietly. These @@ -38,6 +40,8 @@ private slots: void aQueryWithQuotesRoundTrips(); void aMissingFileIsEmptyNotAnError(); void aNewerVersionIsRefused(); + void openingARuleFillsTheBuilderRows(); + void switchingRulesDoesNotLeakRowsBetweenThem(); private: QString writeRules(const QString &json); @@ -241,5 +245,73 @@ void TestTagRules::aNewerVersionIsRefused() QCOMPARE(rules.warnings().size(), 1); } +void TestTagRules::openingARuleFillsTheBuilderRows() +{ + // The dialog reads the shared store from its default path, so point the + // whole process at a temporary one. XDG_CONFIG_HOME is what + // TagRules::defaultPath() honours. + 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 and subject:receipt", + "add": ["vendor"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + + QCOMPARE(dialog.rowCountForTest(), 2); + QCOMPARE(dialog.queryLineForTest(), + QStringLiteral("from:vendor.example.org and subject:receipt")); +} + +void TestTagRules::switchingRulesDoesNotLeakRowsBetweenThem() +{ + 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": "one", "query": "from:one.example.org", + "add": ["one"], "stage": 50, "enabled": true}, + {"id": "two", + "query": "from:two.example.org or from:three.example.org", + "add": ["two"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + + // The first rule is selected on open: one row, joined All by default. + QCOMPARE(dialog.rowCountForTest(), 1); + QCOMPARE(dialog.queryLineForTest(), QStringLiteral("from:one.example.org")); + + dialog.selectRuleForTest(1); + QCOMPARE(dialog.rowCountForTest(), 2); + QCOMPARE(dialog.queryLineForTest(), + QStringLiteral("from:two.example.org or from:three.example.org")); + + // And back, to prove the first rule was not overwritten by loading the + // second. + dialog.selectRuleForTest(0); + QCOMPARE(dialog.rowCountForTest(), 1); + QCOMPARE(dialog.queryLineForTest(), QStringLiteral("from:one.example.org")); +} + QTEST_MAIN(TestTagRules) #include "test_tagrules.moc" -- cgit v1.2.3 From 2ad9b17d53e19418de089cc98820af0776c3bfba Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 11:39:49 +0200 Subject: feat(rules): text mode, and leave untouched rules unwritten --- src/tagrulesdialog.cpp | 70 ++++++++++++++++++- src/tagrulesdialog.h | 7 ++ tests/test_tagrules.cpp | 181 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 257 insertions(+), 1 deletion(-) (limited to 'tests/test_tagrules.cpp') diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 797cd92..1d31d07 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -162,6 +162,8 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) connect(m_matchAll, &QRadioButton::toggled, this, &TagRulesDialog::syncQueryLine); + connect(m_textMode, &QCheckBox::toggled, + this, &TagRulesDialog::setTextMode); connect(m_addExclusion, &QPushButton::clicked, this, [this] { addRow(true); syncQueryLine(); @@ -326,6 +328,16 @@ void TagRulesDialog::onSelectionChanged() // rule and closing it cannot rewrite the file mailctl also reads. m_loadedQuery = RuleQuery::parse(rule.query); + // A rule the builder cannot show opens as text, and one it can show + // returns to the builder. Blocked, because letting setChecked run + // setTextMode() here would recompile and overwrite m_query mid-load. + { + const QSignalBlocker blockTextMode(m_textMode); + m_textMode->setChecked(!m_loadedQuery.parsed); + } + m_builder->setVisible(m_loadedQuery.parsed); + m_query->setReadOnly(m_loadedQuery.parsed); + if (m_loadedQuery.parsed) rebuildRows(m_loadedQuery); @@ -347,7 +359,16 @@ void TagRulesDialog::applyEditsToCurrentRule() rule.enabled = m_enabled->isChecked(); rule.add = splitTags(m_add->text()); rule.remove = splitTags(m_remove->text()); - rule.query = m_query->text().trimmed(); + if (m_textMode->isChecked()) { + rule.query = m_query->text().trimmed(); + } else { + const RuleQuery current = currentQueryFromRows(); + // Unchanged rows mean the stored string is left exactly as it was + // read. Recompiling an untouched rule would churn a file the + // companion tool also reads, showing a diff the user never made. + if (!(current == m_loadedQuery)) + rule.query = current.compile(); + } rule.note = m_note->toPlainText(); fillItem(m_list->topLevelItem(index), rule); @@ -431,6 +452,19 @@ QString TagRulesDialog::queryLineForTest() const return m_query->text(); } +bool TagRulesDialog::textModeForTest() const +{ + return m_textMode->isChecked(); +} + +void TagRulesDialog::setRowValueForTest(int index, const QString &value) +{ + if (index < 0 || index >= m_rows.size()) + return; + m_rows.at(index).value->setText(value); + syncQueryLine(); +} + void TagRulesDialog::selectRuleForTest(int index) { if (index >= 0 && index < m_list->topLevelItemCount()) @@ -587,6 +621,40 @@ RuleQuery TagRulesDialog::currentQueryFromRows() const return query; } +void TagRulesDialog::setTextMode(bool on) +{ + if (on) { + // Show what the rows currently mean, then hand the string over. + if (m_loadedQuery.parsed) + m_query->setText(currentQueryFromRows().compile()); + m_builder->setVisible(false); + m_query->setReadOnly(false); + return; + } + + // Going back needs the typed query to be representable. If it is not, the + // checkbox cannot clear: there are no rows that mean this query. + const RuleQuery parsed = RuleQuery::parse(m_query->text().trimmed()); + if (!parsed.parsed) { + const QSignalBlocker block(m_textMode); + m_textMode->setChecked(true); + QMessageBox::information( + this, tr("Cannot show as rows"), + tr("This query is more than the builder can show, so it stays " + "as text. It is still saved and applied normally.")); + return; + } + + const bool wasReloading = m_reloading; + m_reloading = true; + rebuildRows(parsed); + m_reloading = wasReloading; + + m_loadedQuery = parsed; + m_builder->setVisible(true); + m_query->setReadOnly(true); +} + void TagRulesDialog::rebuildRows(const RuleQuery &query) { while (!m_rows.isEmpty()) diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index 6676da9..9cee163 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -64,6 +64,11 @@ public: /// Selects the rule at `index` as a click on the list would. void selectRuleForTest(int index); + bool textModeForTest() const; + void setRowValueForTest(int index, const QString &value); + /// Runs the Save path without showing the dialog. + void saveForTest() { onSave(); } + signals: /// Asks the owner to run countQueries() through the worker. void countsRequested(); @@ -104,6 +109,8 @@ private: void updateExclusionsVisibility(); void syncQueryLine(); + void setTextMode(bool on); + void rebuildRows(const RuleQuery &query); void applyTermToRow(Row *row, const RuleTerm &term); RuleQuery currentQueryFromRows() const; diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index fa5c0b5..49abf8a 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -42,6 +42,10 @@ private slots: void aNewerVersionIsRefused(); void openingARuleFillsTheBuilderRows(); void switchingRulesDoesNotLeakRowsBetweenThem(); + void openingARuleWithoutEditingLeavesItByteIdentical(); + void anUnrepresentableRuleOpensInTextMode(); + void editingARowRewritesTheQuery(); + void aTextModeRuleStaysTextWhenAnotherRuleIsVisited(); private: QString writeRules(const QString &json); @@ -313,5 +317,182 @@ void TestTagRules::switchingRulesDoesNotLeakRowsBetweenThem() QCOMPARE(dialog.queryLineForTest(), QStringLiteral("from:one.example.org")); } +void TestTagRules::openingARuleWithoutEditingLeavesItByteIdentical() +{ + // Recompiling on open would rewrite the shared file for no reason, and + // the companion tool would see a diff the user never made. Semantically + // equal is not enough: the bytes must match. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(R"({ + "version": 1, + "rules": [ + {"id": "handwritten", + "query": "not subject:receipt and from:plain.example.net", + "add": ["handwritten"], "stage": 50, "enabled": true}, + {"id": "vendor", + "query": "(from:vendor.example.org or from:vendor.example.net) and not subject:receipt", + "add": ["vendor"], "stage": 50, "enabled": true}, + {"id": "plain", "query": "from:plain.example.org", + "add": ["plain"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + { + TagRulesDialog dialog; + dialog.selectRuleForTest(2); + dialog.selectRuleForTest(1); + dialog.selectRuleForTest(0); + dialog.saveForTest(); + } + + TagRules reloaded; + reloaded.load(stored); + QCOMPARE(reloaded.rules().size(), 3); + // Hand-written spacing and an exclusion ahead of the positive term. Both + // are things compile() normalises away, and this rule is deliberately the + // one left current at Save, since that is the only rule the save path + // writes at all. The two below round trip byte for byte on their own, so + // neither could catch a save path that recompiles regardless. + QCOMPARE(reloaded.rules().at(0).query, + QStringLiteral("not subject:receipt and " + "from:plain.example.net")); + QCOMPARE(reloaded.rules().at(1).query, + QStringLiteral("(from:vendor.example.org or " + "from:vendor.example.net) and not subject:receipt")); + QCOMPARE(reloaded.rules().at(2).query, + QStringLiteral("from:plain.example.org")); +} + +void TestTagRules::anUnrepresentableRuleOpensInTextMode() +{ + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + // body: is a perfectly good notmuch prefix this builder does not model. + out.write(R"({ + "version": 1, + "rules": [ + {"id": "deep", "query": "body:receipt", + "add": ["deep"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + { + TagRulesDialog dialog; + QVERIFY(dialog.textModeForTest()); + dialog.saveForTest(); + } + + // Unrepresentable is not invalid: it must survive a save untouched. + TagRules reloaded; + reloaded.load(stored); + QCOMPARE(reloaded.rules().size(), 1); + QCOMPARE(reloaded.rules().at(0).query, QStringLiteral("body:receipt")); +} + +void TestTagRules::editingARowRewritesTheQuery() +{ + // The other half of the guarantee: when rows DO change, the stored query + // must follow. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + 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; + dialog.setRowValueForTest(0, QStringLiteral("other.example.org")); + dialog.saveForTest(); + } + + TagRules reloaded; + reloaded.load(stored); + QCOMPARE(reloaded.rules().size(), 1); + QCOMPARE(reloaded.rules().at(0).query, + QStringLiteral("from:other.example.org")); +} + +void TestTagRules::aTextModeRuleStaysTextWhenAnotherRuleIsVisited() +{ + // The cross-rule question, asked directly. Text mode and m_loadedQuery are + // per-rule state on a dialog that has one set of widgets, so visiting a + // representable rule and coming back must not leave the unrepresentable one + // holding the other rule's mode or its parsed query. Getting that wrong + // recompiles a query the builder never modelled, which is data loss. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(R"({ + "version": 1, + "rules": [ + {"id": "deep", "query": "body:receipt", + "add": ["deep"], "stage": 50, "enabled": true}, + {"id": "plain", "query": "from:plain.example.org", + "add": ["plain"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + { + TagRulesDialog dialog; + QVERIFY(dialog.textModeForTest()); + + dialog.selectRuleForTest(1); + QVERIFY2(!dialog.textModeForTest(), + "a representable rule must return to the builder"); + QCOMPARE(dialog.queryLineForTest(), + QStringLiteral("from:plain.example.org")); + + dialog.selectRuleForTest(0); + QVERIFY2(dialog.textModeForTest(), + "coming back to an unrepresentable rule must be text again"); + QCOMPARE(dialog.queryLineForTest(), QStringLiteral("body:receipt")); + + dialog.saveForTest(); + } + + TagRules reloaded; + reloaded.load(stored); + QCOMPARE(reloaded.rules().size(), 2); + QCOMPARE(reloaded.rules().at(0).query, QStringLiteral("body:receipt")); + QCOMPARE(reloaded.rules().at(1).query, + QStringLiteral("from:plain.example.org")); +} + QTEST_MAIN(TestTagRules) #include "test_tagrules.moc" -- cgit v1.2.3 From 8913a1190c48ff2672f8783af8594ca11e49ccf7 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 11:43:16 +0200 Subject: feat(rules): report the text-mode refusal without a modal Leaving text mode with a query the builder cannot represent has to refuse, since there are no rows that mean that query. It announced this with a QMessageBox, which made the branch untestable: a modal blocks the test that reaches it, so the one path that can strand a user was the one path shipping unverified. Say it in the warning label the dialog already has instead. That also suits the moment better, since it does not interrupt someone mid-edit to tell them something the label can hold while they keep typing, and it matches how the tag dialog reports a bad tag. Returning to the rows now calls showWarnings(), because the refusal writes into the same label the load warnings use and a stale complaint would otherwise outlive the query that caused it. The test drives the refusal and the recovery, and asserts the warning appears and then clears. Verified by mutation: letting the checkbox clear regardless fails it. warningTextForTest uses isVisibleTo rather than isVisible. Every child of a dialog that was never shown reports isVisible() false, so the seam would have reported no warning whatever the label held, which is a probe that cannot see the thing it checks. --- src/tagrulesdialog.cpp | 39 ++++++++++++++++++++++++++++++++++---- src/tagrulesdialog.h | 7 +++++++ tests/test_tagrules.cpp | 50 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 4 deletions(-) (limited to 'tests/test_tagrules.cpp') diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 1d31d07..86566ba 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -465,6 +465,26 @@ void TagRulesDialog::setRowValueForTest(int index, const QString &value) syncQueryLine(); } +void TagRulesDialog::setQueryTextForTest(const QString &text) +{ + m_query->setText(text); +} + +void TagRulesDialog::setTextModeForTest(bool on) +{ + m_textMode->setChecked(on); +} + +QString TagRulesDialog::warningTextForTest() const +{ + // isVisible() is false for every child of a dialog that was never shown, + // so it would report no warning whatever the label held. isVisibleTo() + // answers the question actually being asked: would this be on screen if + // the dialog were. + return m_warningLabel->isVisibleTo(this) ? m_warningLabel->text() + : QString(); +} + void TagRulesDialog::selectRuleForTest(int index) { if (index >= 0 && index < m_list->topLevelItemCount()) @@ -634,14 +654,19 @@ void TagRulesDialog::setTextMode(bool on) // Going back needs the typed query to be representable. If it is not, the // checkbox cannot clear: there are no rows that mean this query. + // + // Said in the warning label rather than a modal. A modal here would block + // any test that reaches this branch, which is how a refusal path ends up + // shipping unverified, and it interrupts someone who is mid-edit to tell + // them something the label can hold while they keep typing. const RuleQuery parsed = RuleQuery::parse(m_query->text().trimmed()); if (!parsed.parsed) { const QSignalBlocker block(m_textMode); m_textMode->setChecked(true); - QMessageBox::information( - this, tr("Cannot show as rows"), - tr("This query is more than the builder can show, so it stays " - "as text. It is still saved and applied normally.")); + m_warningLabel->setText( + tr("This query is more than the builder can show, so it stays as " + "text. It is still saved and applied normally.")); + m_warningLabel->setVisible(true); return; } @@ -653,6 +678,12 @@ void TagRulesDialog::setTextMode(bool on) m_loadedQuery = parsed; m_builder->setVisible(true); m_query->setReadOnly(true); + + // The refusal above writes into the same label the load warnings use, so + // a successful return to the rows must clear it or a stale complaint + // outlives the query that caused it. showWarnings() restores whatever the + // file itself had to say. + showWarnings(); } void TagRulesDialog::rebuildRows(const RuleQuery &query) diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index 9cee163..948ebe2 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -69,6 +69,13 @@ public: /// Runs the Save path without showing the dialog. void saveForTest() { onSave(); } + /// Types into the query field and toggles the mode, so the refusal path + /// can be reached without a synthetic click. The refusal reports through + /// the warning label rather than a modal precisely so this is testable. + void setQueryTextForTest(const QString &text); + void setTextModeForTest(bool on); + QString warningTextForTest() 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 49abf8a..2302111 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -46,6 +46,7 @@ private slots: void anUnrepresentableRuleOpensInTextMode(); void editingARowRewritesTheQuery(); void aTextModeRuleStaysTextWhenAnotherRuleIsVisited(); + void leavingTextModeIsRefusedWhenTheQueryCannotBeShownAsRows(); private: QString writeRules(const QString &json); @@ -494,5 +495,54 @@ void TestTagRules::aTextModeRuleStaysTextWhenAnotherRuleIsVisited() QStringLiteral("from:plain.example.org")); } +void TestTagRules::leavingTextModeIsRefusedWhenTheQueryCannotBeShownAsRows() +{ + // The refusal is the only path that can strand a user, so it is the one + // most worth pinning. It reports through the warning label rather than a + // modal, which is what lets this test exist at all: a modal would block + // here and the branch would ship unverified. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + 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.textModeForTest()); + + dialog.setTextModeForTest(true); + QVERIFY(dialog.textModeForTest()); + + // Type something notmuch accepts and this builder does not model. + dialog.setQueryTextForTest(QStringLiteral("body:receipt")); + dialog.setTextModeForTest(false); + + QVERIFY2(dialog.textModeForTest(), + "the checkbox must refuse to clear: no rows mean this query"); + QVERIFY2(!dialog.warningTextForTest().isEmpty(), + "the refusal must say why, not fail silently"); + + // And a representable query lets the builder back, clearing the warning. + dialog.setQueryTextForTest(QStringLiteral("from:other.example.org")); + dialog.setTextModeForTest(false); + + QVERIFY2(!dialog.textModeForTest(), "a representable query must return"); + QCOMPARE(dialog.rowCountForTest(), 1); + QVERIFY2(dialog.warningTextForTest().isEmpty(), + "a stale refusal must not outlive the query that caused it"); +} + QTEST_MAIN(TestTagRules) #include "test_tagrules.moc" -- cgit v1.2.3 From 4a9bbff4cb157538106b588bc84f39dd7fd38289 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 11:46:16 +0200 Subject: feat(rules): a folder dropdown, so the path suffix is never typed --- src/mainwindow.cpp | 11 +++++++ src/tagrulesdialog.cpp | 78 +++++++++++++++++++++++++++++++++++++++++++++---- src/tagrulesdialog.h | 15 ++++++++++ tests/test_tagrules.cpp | 46 +++++++++++++++++++++++++++++ 4 files changed, 144 insertions(+), 6 deletions(-) (limited to 'tests/test_tagrules.cpp') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 331354f..f622036 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1313,6 +1313,17 @@ void MainWindow::showTagRulesDialog() auto *dialog = new TagRulesDialog(this); dialog->setAttribute(Qt::WA_DeleteOnClose); + + // The Folder row's dropdown. Config holds the account subdirectories and + // the dialog holds no Config, so they are handed over here. Account::maildir + // is the subtree name the Folder term compiles a path: against; the account + // key is a config section name and would match nothing. + QStringList folders; + for (const Account &account : m_config.accounts()) { + if (!account.maildir.isEmpty() && !folders.contains(account.maildir)) + folders.append(account.maildir); + } + dialog->setFolders(folders); m_tagRulesDialog = dialog; connect(dialog, &TagRulesDialog::countsRequested, this, [this, dialog]() { diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 86566ba..c81450c 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -461,7 +461,7 @@ void TagRulesDialog::setRowValueForTest(int index, const QString &value) { if (index < 0 || index >= m_rows.size()) return; - m_rows.at(index).value->setText(value); + setRowValue(&m_rows[index], value); syncQueryLine(); } @@ -491,6 +491,44 @@ void TagRulesDialog::selectRuleForTest(int index) m_list->setCurrentItem(m_list->topLevelItem(index)); } +QString TagRulesDialog::rowValue(const Row &row) const +{ + const bool isFolder = + RuleTerm::Field(row.field->currentData().toInt()) == RuleTerm::Folder; + return (isFolder ? row.folder->currentText() : row.value->text()).trimmed(); +} + +void TagRulesDialog::setRowValue(Row *row, const QString &value) +{ + if (RuleTerm::Field(row->field->currentData().toInt()) == RuleTerm::Folder) + row->folder->setCurrentText(value); + else + row->value->setText(value); +} + +void TagRulesDialog::setFolders(const QStringList &folders) +{ + m_folders = folders; + + // Rows already exist by the time this is called: the constructor loads the + // first rule and builds its rows before the caller can hand the list over. + // Repopulating them here rather than only in addRow() is what stops the row + // on screen from opening with an empty dropdown. The current text is + // preserved across the refill, since the combo is editable and may hold a + // folder the config does not list. + const auto refill = [&folders](const QList &rows) { + for (const Row &row : rows) { + const QString had = row.folder->currentText(); + QSignalBlocker block(row.folder); + row.folder->clear(); + row.folder->addItems(folders); + row.folder->setCurrentText(had); + } + }; + refill(m_rows); + refill(m_exclusionRows); +} + TagRulesDialog::Row *TagRulesDialog::addRow(bool exclusion) { Row row; @@ -505,6 +543,13 @@ TagRulesDialog::Row *TagRulesDialog::addRow(bool exclusion) row.op = new QComboBox(row.container); row.value = new QLineEdit(row.container); + row.folder = new QComboBox(row.container); + // Editable so a folder present in the file but absent from the config + // still displays and still saves, rather than being silently blanked. + row.folder->setEditable(true); + row.folder->addItems(m_folders); + row.folder->setVisible(false); + auto *plus = new QPushButton(QStringLiteral("+"), row.container); auto *minus = new QPushButton(QStringLiteral("-"), row.container); plus->setFixedWidth(30); @@ -513,6 +558,7 @@ TagRulesDialog::Row *TagRulesDialog::addRow(bool exclusion) layout->addWidget(row.field); layout->addWidget(row.op); layout->addWidget(row.value, 1); + layout->addWidget(row.folder, 1); layout->addWidget(plus); layout->addWidget(minus); @@ -521,15 +567,28 @@ TagRulesDialog::Row *TagRulesDialog::addRow(bool exclusion) rows.append(row); target->addWidget(row.container); + // The three widgets by pointer, never the Row by value: the row lives in a + // QList that reallocates as rows are added, so a copy taken here would be + // compared against, or written through, after that list has moved. + QComboBox *field = row.field; + QLineEdit *value = row.value; + QComboBox *folder = row.folder; connect(row.field, &QComboBox::currentIndexChanged, this, - [this, exclusion](int) { + [this, exclusion, field, value, folder](int) { populateOperators(exclusion); + const bool isFolder = + RuleTerm::Field(field->currentData().toInt()) + == RuleTerm::Folder; + value->setVisible(!isFolder); + folder->setVisible(isFolder); syncQueryLine(); }); connect(row.op, &QComboBox::currentIndexChanged, this, [this](int) { syncQueryLine(); }); connect(row.value, &QLineEdit::textEdited, this, [this](const QString &) { syncQueryLine(); }); + connect(row.folder, &QComboBox::currentTextChanged, + this, [this](const QString &) { syncQueryLine(); }); connect(plus, &QPushButton::clicked, this, [this, exclusion] { addRow(exclusion); syncQueryLine(); @@ -562,7 +621,7 @@ void TagRulesDialog::removeRow(bool exclusion, int index) // empty query, which is reachable by clearing the value rather than by // deleting the last row out from under the user. if (!exclusion && rows.size() == 1) { - rows[0].value->clear(); + setRowValue(&rows[0], QString()); return; } @@ -622,7 +681,7 @@ RuleQuery TagRulesDialog::currentQueryFromRows() const query.join = m_matchAny->isChecked() ? RuleQuery::Any : RuleQuery::All; for (const Row &row : m_rows) { - const QString value = row.value->text().trimmed(); + const QString value = rowValue(row); if (value.isEmpty()) continue; query.terms.append({RuleTerm::Field(row.field->currentData().toInt()), @@ -630,7 +689,7 @@ RuleQuery TagRulesDialog::currentQueryFromRows() const value}); } for (const Row &row : m_exclusionRows) { - const QString value = row.value->text().trimmed(); + const QString value = rowValue(row); if (value.isEmpty()) continue; query.exclusions.append( @@ -712,11 +771,18 @@ void TagRulesDialog::applyTermToRow(Row *row, const RuleTerm &term) const QSignalBlocker blockField(row->field); const QSignalBlocker blockOp(row->op); const QSignalBlocker blockValue(row->value); + const QSignalBlocker blockFolder(row->folder); const int fieldIndex = row->field->findData(int(term.field)); if (fieldIndex >= 0) row->field->setCurrentIndex(fieldIndex); + // The field's own handler is blocked here, so the swap it would have done + // has to happen explicitly or a Folder row opens showing the line edit. + const bool isFolder = term.field == RuleTerm::Folder; + row->value->setVisible(!isFolder); + row->folder->setVisible(isFolder); + // The operator list depends on the field just set, so it must be rebuilt // before the operator can be found in it. populateOperators(false); @@ -726,7 +792,7 @@ void TagRulesDialog::applyTermToRow(Row *row, const RuleTerm &term) if (opIndex >= 0) row->op->setCurrentIndex(opIndex); - row->value->setText(term.value); + setRowValue(row, term.value); } void TagRulesDialog::syncQueryLine() diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index 948ebe2..c91dc0c 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -19,6 +19,7 @@ #pragma once #include +#include #include "rulequery.h" #include "tagrules.h" @@ -55,6 +56,11 @@ public: /// handle and notmuch permits one per process. QStringList countQueries() const; + /// Account subdirectory names, for the Folder row's dropdown. Supplied by + /// the caller rather than read here: Config knows them, and this dialog + /// deliberately holds no Config of its own. + void setFolders(const QStringList &folders); + /// Test seams. The builder's state is otherwise reachable only through /// synthetic clicks on widgets whose geometry the offscreen platform does /// not guarantee. @@ -108,8 +114,15 @@ private: QComboBox *field = nullptr; QComboBox *op = nullptr; QLineEdit *value = nullptr; + /// Shown in place of `value` for a Folder row. Never visible together. + QComboBox *folder = nullptr; }; + /// Reads a row's value from whichever of its two widgets the field selects, + /// so the read and write paths cannot disagree about where it lives. + QString rowValue(const Row &row) const; + void setRowValue(Row *row, const QString &value); + Row *addRow(bool exclusion); void removeRow(bool exclusion, int index); void populateOperators(bool exclusion); @@ -127,6 +140,8 @@ private: /// left alone. RuleQuery m_loadedQuery; + QStringList m_folders; + QList m_rows; QList m_exclusionRows; diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index 2302111..8207b3c 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -47,6 +47,7 @@ private slots: void editingARowRewritesTheQuery(); void aTextModeRuleStaysTextWhenAnotherRuleIsVisited(); void leavingTextModeIsRefusedWhenTheQueryCannotBeShownAsRows(); + void aFolderRowUsesTheDropdownAndKeepsItsSuffix(); private: QString writeRules(const QString &json); @@ -544,5 +545,50 @@ void TestTagRules::leavingTextModeIsRefusedWhenTheQueryCannotBeShownAsRows() "a stale refusal must not outlive the query that caused it"); } +void TestTagRules::aFolderRowUsesTheDropdownAndKeepsItsSuffix() +{ + // A path: without its suffix matches nothing and notmuch says nothing + // about it, so the suffix must never depend on the user typing it. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(R"({ + "version": 1, + "rules": [ + {"id": "account", "query": "path:\"account-one/**\"", + "add": ["account-one"], "stage": 10, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + dialog.setFolders({QStringLiteral("account-one"), + QStringLiteral("account-two")}); + + // The stored rule round-trips: the row holds the bare name, and the + // query keeps the suffix. + QCOMPARE(dialog.rowCountForTest(), 1); + QCOMPARE(dialog.queryLineForTest(), + QStringLiteral("path:\"account-one/**\"")); + + dialog.setRowValueForTest(0, QStringLiteral("account-two")); + QCOMPARE(dialog.queryLineForTest(), + QStringLiteral("path:\"account-two/**\"")); + + dialog.saveForTest(); + + TagRules reloaded; + reloaded.load(stored); + QCOMPARE(reloaded.rules().size(), 1); + QCOMPARE(reloaded.rules().at(0).query, + QStringLiteral("path:\"account-two/**\"")); +} + QTEST_MAIN(TestTagRules) #include "test_tagrules.moc" -- 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 'tests/test_tagrules.cpp') 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 From 3b771d03f2df572de29af3156817de3cb7ef6bef Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 16:15:57 +0200 Subject: feat(rules): the rules window keeps its size and column widths Item 75. saveGeometry() and the rule list header's saveState() go to uistate.conf under keys of their own, written on closeEvent so a size survives Cancel as well as Save. The 760x520 resize stays as the first-run fallback. The backlog's approach was wrong on one point and a test caught it. It said to drop the resizeColumnToContents calls once a saved header state exists, which fixes the restore and leaves the original defect standing: with nothing saved, a width the user had just dragged was still discarded by the next add or delete. Each column is instead auto-sized once, on its first fill, after which the width belongs to the user however it was set. Two flags, because the count column is filled later by a reply from the worker. The window stays a QDialog. Making it a top-level window needs the unsaved-edit story that being modal currently sidesteps, and that is its own decision rather than part of this item. Both tests redirect XDG_STATE_HOME as well as XDG_CONFIG_HOME, so they cannot write the real uistate.conf. The geometry is asserted on the stored value rather than the reopened frame, per item 46: the offscreen platform does not honour a resize. Also corrects setFolders' doc comment, which still described the folder list as coming from Config. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 15 ++-- .../plans/2026-08-03-post-0.1.0-usability.md | 22 ++++- src/tagrulesdialog.cpp | 93 +++++++++++++++++++- src/tagrulesdialog.h | 39 ++++++++- tests/test_tagrules.cpp | 99 ++++++++++++++++++++++ 5 files changed, 255 insertions(+), 13 deletions(-) (limited to 'tests/test_tagrules.cpp') diff --git a/CHANGELOG.md b/CHANGELOG.md index 03b5046..0637cd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,11 +19,16 @@ point at which they are stable. stays visible and is what gets saved, so a rule the builder cannot show opens as text and still works. Opening a rule without editing it leaves the stored query untouched. -- A Folder condition picks from your configured accounts rather than being - typed. A folder path with a typo matches nothing and notmuch reports no - error, so the rule would simply never fire. The list stays editable, so a - folder that is in the rules file but not in your config still opens and - still saves. +- A Folder condition picks from a list of every folder in your Maildir rather + than being typed, Drafts and Sent included, not only the top of each + account. A folder path with a typo matches nothing and notmuch reports no + error, so the rule would simply never fire. The list is read from the tree + on disk, so a folder that exists but has no mail in it yet is still offered. + It stays editable, so a folder in the rules file that is no longer on disk + still opens and still saves. +- The tagging rules window remembers its size and the widths of the rule + list's columns. A column you widen also survives adding or deleting a rule, + which previously reset it. ### Fixed diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index 00a7f76..d55336d 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -131,8 +131,8 @@ taking that too literally. | 72 | No khard/khal integration | workflow | ? | open, unspecified; the user places it after send, so v2 at the earliest | | 73 | This backlog is past four thousand lines | maintenance | S | open | | 74 | "Searching..." keeps claiming a query is running while rows are already arriving | feedback | XS | open; cause measured 2026-08-11, the delay itself is the cold page cache and is not fixable here | -| 75 | The tagging rules window forgets its size and its column widths | persistence | S | open; follows item 44 | -| 76 | Every field in the rules dialog is free text, so a rule is easy to get wrong | workflow | M | open; design approved 2026-08-13, see `specs/2026-08-13-rule-builder-design.md` | +| 75 | The tagging rules window forgets its size and its column widths | persistence | S | **done** 2026-08-13 on `rule-builder`, unreleased. The window-kind question is left open, see below | +| 76 | Every field in the rules dialog is free text, so a rule is easy to get wrong | workflow | M | **done** 2026-08-13 on `rule-builder`, unreleased. See `specs/2026-08-13-rule-builder-design.md` | | 77 | No way to see what a rule would collect, in the thread list | workflow | S | open; the dialog counts matches, it cannot show them | | 78 | No way to build a rule from something visible in a message | workflow | M | open; wants 76 first, so the created rule lands in a form that can hold it | | 79 | Opening the rules dialog and saving destroys the first rule | defect | XS | **fixed on `rule-builder`** 2026-08-13, unreleased. Shipped in 0.16.0; damaged one real rule, repaired by hand | @@ -4764,6 +4764,24 @@ the test asserts on the saved value rather than on the resulting frame. **Size: S.** +**Done 2026-08-13** on `rule-builder`, unreleased. `saveGeometry()` and the +list header's `saveState()` go to `tagrules/geometry` and `tagrules/header` in +`uistate.conf`, written on `closeEvent` so a size survives Cancel as well as +Save; `resize(760, 520)` stays as the first-run fallback. + +**The approach above was wrong on one point, and a test caught it.** It said to +drop the `resizeColumnToContents` calls "once a saved header state exists", +which fixes the restore and leaves the original defect standing: with no saved +state, a width the user had just dragged was still discarded by the next add or +delete. The rule shipped instead is that each column is auto-sized ONCE, on its +first fill, after which its width belongs to the user however it was set. Two +flags, because the count column is filled later by a reply from the worker. + +The **popup or primary window** question was put to the user and deliberately +not taken: it stays a `QDialog`. Reopening it needs the unsaved-edit story that +being modal currently sidesteps, and that is its own decision rather than part +of this item. + ## 76. Every field in the rules dialog is free text, so a rule is easy to get wrong **Observed.** A rule is written by typing into four line edits, and the user diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 75a9cbe..61f2c21 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -20,8 +20,11 @@ #include #include +#include #include #include +#include +#include #include #include #include @@ -31,10 +34,13 @@ #include #include #include +#include #include #include #include +#include "mainwindow.h" + namespace { /// Columns of the rule list. @@ -72,6 +78,9 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) : QDialog(parent) { setWindowTitle(tr("Tagging rules")); + // The fallback for a first run. restoreUiState() overwrites it when a + // size was saved, and is called at the end of this constructor because + // the header state cannot be restored before the columns exist. resize(760, 520); m_rules.load(); @@ -244,6 +253,72 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) reloadList(); showWarnings(); + + // Last, and after reloadList(): a header state cannot be applied before + // the columns it describes exist, and reloadList is what fills them. + restoreUiState(); +} + +/// Reads the window size and the rule list's header layout back. +/// +/// The same file MainWindow uses, under keys of its own. Machine-written +/// state, never the hand-edited config: a column width is not something +/// anyone edits by hand, and mixing the two puts a blob in a file the user +/// reads. +void TagRulesDialog::restoreUiState() +{ + QSettings state(MainWindow::uiStatePath(), QSettings::IniFormat); + + const QByteArray geometry = + state.value(QStringLiteral("tagrules/geometry")).toByteArray(); + if (!geometry.isEmpty()) + restoreGeometry(geometry); + + const QByteArray header = + state.value(QStringLiteral("tagrules/header")).toByteArray(); + if (!header.isEmpty()) { + m_list->header()->restoreState(header); + // Counts as the one auto-size each column gets, so the restore is not + // immediately overwritten. reloadList() runs before this in the + // constructor and has already sized the first two; the count column + // has not been filled yet, and would otherwise resize over the + // restored width as soon as the first counts arrived. + m_columnsSized = true; + m_countColumnSized = true; + } +} + +void TagRulesDialog::saveUiState() +{ + QDir().mkpath(QFileInfo(MainWindow::uiStatePath()).absolutePath()); + QSettings state(MainWindow::uiStatePath(), QSettings::IniFormat); + state.setValue(QStringLiteral("tagrules/geometry"), saveGeometry()); + state.setValue(QStringLiteral("tagrules/header"), + m_list->header()->saveState()); +} + +/// Saves on close rather than on accept, so a size the user chose is kept +/// whether they pressed Save or Cancel. The window's shape is not part of the +/// edit being confirmed. +void TagRulesDialog::closeEvent(QCloseEvent *event) +{ + saveUiState(); + QDialog::closeEvent(event); +} + +int TagRulesDialog::columnWidthForTest(int column) const +{ + return m_list->columnWidth(column); +} + +void TagRulesDialog::setColumnWidthForTest(int column, int width) +{ + m_list->setColumnWidth(column, width); +} + +void TagRulesDialog::reloadListForTest() +{ + reloadList(); } void TagRulesDialog::showWarnings() @@ -286,8 +361,15 @@ void TagRulesDialog::reloadList() auto *item = new QTreeWidgetItem(m_list); fillItem(item, rule); } - m_list->resizeColumnToContents(ColumnEnabled); - m_list->resizeColumnToContents(ColumnStage); + // Auto-sized on the FIRST fill only. After that the widths belong to the + // user, whether they came from a restored header or from a drag in this + // session, and resizing on every repopulate threw both away on the next + // add or delete. + if (!m_columnsSized) { + m_list->resizeColumnToContents(ColumnEnabled); + m_list->resizeColumnToContents(ColumnStage); + m_columnsSized = true; + } m_reloading = false; if (!m_working.isEmpty()) @@ -439,7 +521,12 @@ void TagRulesDialog::setCounts(const QVector &counts) counts.at(i) < 0 ? tr("invalid") : QString::number(counts.at(i))); } - m_list->resizeColumnToContents(ColumnCount); + // Once, like the columns in reloadList. The counts arrive after the first + // fill, so this column gets its own flag rather than sharing that one. + if (!m_countColumnSized) { + m_list->resizeColumnToContents(ColumnCount); + m_countColumnSized = true; + } } void TagRulesDialog::onSave() diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index 9559918..f86d210 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -56,9 +56,14 @@ public: /// handle and notmuch permits one per process. QStringList countQueries() const; - /// Account subdirectory names, for the Folder row's dropdown. Supplied by - /// the caller rather than read here: Config knows them, and this dialog - /// deliberately holds no Config of its own. + /// Maildir folder paths, for the Folder row's dropdown, relative to the + /// database root. Supplied by the caller rather than read here: they come + /// from a scan of the tree under notmuch's database.path, which only + /// NotmuchWorker can answer for, and this dialog reaches neither it nor + /// Config. + /// + /// Arrives AFTER the dialog is on screen, since the scan crosses to the + /// worker on a queued call, so this refills the rows that already exist. void setFolders(const QStringList &folders); /// Test seams. The builder's state is otherwise reachable only through @@ -88,6 +93,20 @@ public: /// when the widget is not. bool textModeToggleIsReachableForTest() const; + /// Width of one rule-list column. The geometry restore is asserted on the + /// saved and reread VALUE rather than on the resulting frame: item 46 + /// records that the offscreen platform does not honour a window resize, + /// so a test comparing frames there passes or fails for reasons that have + /// nothing to do with the code. + int columnWidthForTest(int column) const; + void setColumnWidthForTest(int column, int width); + + /// Repopulates the rule list, as adding or deleting a rule does. Exposed + /// because a restored column width has to survive one of these, not only + /// a close and reopen: `resizeColumnToContents` on every reload discarded + /// the width the user had dragged. + void reloadListForTest(); + signals: /// Asks the owner to run countQueries() through the worker. void countsRequested(); @@ -104,8 +123,13 @@ private slots: void applyEditsToCurrentRule(); void onSave(); +protected: + void closeEvent(QCloseEvent *event) override; + private: void reloadList(); + void restoreUiState(); + void saveUiState(); void showWarnings(); int currentIndex() const; @@ -160,6 +184,15 @@ private: /// row's text, which applyEditsToCurrentRule() has no chance to flush. bool m_reloading = false; + /// Each column is auto-sized to its contents ONCE, on its first fill. + /// After that its width belongs to the user, whether it came from a + /// restored header or from a drag, and resizeColumnToContents on every + /// repopulate threw both away on the next add or delete. Two flags rather + /// than one because the count column is filled later than the rest, by a + /// reply from the worker. + bool m_columnsSized = false; + bool m_countColumnSized = false; + QTreeWidget *m_list = nullptr; QLineEdit *m_id = nullptr; QLineEdit *m_add = nullptr; diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index cf7dadb..af30938 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -16,9 +16,11 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include #include #include +#include "mainwindow.h" #include "rulequery.h" #include "tagrules.h" #include "tagrulesdialog.h" @@ -49,6 +51,8 @@ private slots: void leavingTextModeIsRefusedWhenTheQueryCannotBeShownAsRows(); void aFolderRowUsesTheDropdownAndKeepsItsSuffix(); void theTextModeToggleSurvivesBeingSwitchedOn(); + void theWindowSizeAndColumnWidthsSurviveAReopen(); + void aReloadDoesNotDiscardARestoredColumnWidth(); private: QString writeRules(const QString &json); @@ -634,5 +638,100 @@ void TestTagRules::theTextModeToggleSurvivesBeingSwitchedOn() QStringLiteral("from:vendor.example.org")); } +namespace { + +/// Writes a two-rule file under a throwaway XDG_CONFIG_HOME. Two rules rather +/// than one because the column-width tests reload the list, and a list with a +/// single row hides an off-by-one in the repopulate. +void writeTwoRules(const QTemporaryDir &configHome) +{ + 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}, + {"id": "lists", "query": "to:list.example.org", + "add": ["lists"], "stage": 60, "enabled": true} + ] + })"); + out.close(); +} + +} // namespace + +void TestTagRules::theWindowSizeAndColumnWidthsSurviveAReopen() +{ + // The window opened at 760x520 whatever size it was left at, and the + // columns reset to their computed widths on every open. + // + // XDG_STATE_HOME is redirected as well as XDG_CONFIG_HOME: the state file + // is where this writes, and a test must not touch the user's real + // ~/.local/state/qtmaildir/uistate.conf. + QTemporaryDir configHome; + QTemporaryDir stateHome; + QVERIFY(configHome.isValid()); + QVERIFY(stateHome.isValid()); + const QByteArray previousState = qgetenv("XDG_STATE_HOME"); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + qputenv("XDG_STATE_HOME", stateHome.path().toUtf8()); + writeTwoRules(configHome); + + { + TagRulesDialog dialog; + dialog.resize(900, 640); + dialog.setColumnWidthForTest(0, 123); + // The save is on close, matching where MainWindow writes its own. + dialog.close(); + } + + // Asserted on the stored VALUE, not on the reopened frame. Item 46: the + // offscreen platform does not honour a resize, so a frame comparison here + // would report a failure the code did not cause. + QSettings state(MainWindow::uiStatePath(), QSettings::IniFormat); + QCOMPARE(state.value(QStringLiteral("tagrules/geometry")).toByteArray() + .isEmpty(), false); + + { + TagRulesDialog reopened; + QCOMPARE(reopened.columnWidthForTest(0), 123); + } + + if (previousState.isEmpty()) + qunsetenv("XDG_STATE_HOME"); + else + qputenv("XDG_STATE_HOME", previousState); +} + +void TestTagRules::aReloadDoesNotDiscardARestoredColumnWidth() +{ + // The width did not survive a close, and it did not survive an ADD or a + // DELETE either: reloadList called resizeColumnToContents on every + // repopulate, so a restore was undone by the first thing the user did in + // the window. Restoring on open and reverting on the next click is worse + // than never restoring at all, because it looks like the setting is + // broken rather than absent. + QTemporaryDir configHome; + QTemporaryDir stateHome; + QVERIFY(configHome.isValid()); + QVERIFY(stateHome.isValid()); + const QByteArray previousState = qgetenv("XDG_STATE_HOME"); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + qputenv("XDG_STATE_HOME", stateHome.path().toUtf8()); + writeTwoRules(configHome); + + TagRulesDialog dialog; + dialog.setColumnWidthForTest(0, 137); + dialog.reloadListForTest(); + QCOMPARE(dialog.columnWidthForTest(0), 137); + + if (previousState.isEmpty()) + qunsetenv("XDG_STATE_HOME"); + else + qputenv("XDG_STATE_HOME", previousState); +} + QTEST_MAIN(TestTagRules) #include "test_tagrules.moc" -- cgit v1.2.3 From 0422205803c8f6980a447fdaa1dc270a486970cd Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 16:21:42 +0200 Subject: fix(rules): save the window size on Cancel and Save, not only on X The geometry was saved from closeEvent, and neither dialog button sends one: Cancel calls reject(), Save calls accept(), and only the window manager's X button produces a QCloseEvent. So the size and the column widths were kept for the one route out of three that a user almost never takes, and a resize followed by Cancel came back forgotten. The save moves to a done(int) override, which both buttons funnel through and which QWidget::close() also reaches. The test that covered this passed against the bug because it asserted with close(). It now drives all three routes rather than trusting one to stand for the others, and shows the dialog before the close leg: close() on a widget that was never visible returns early without reaching done(), so that assertion would otherwise prove nothing. Both traps recorded in CLAUDE.md, since neither is specific to this dialog. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 11 ++++ .../plans/2026-08-03-post-0.1.0-usability.md | 12 +++++ src/tagrulesdialog.cpp | 20 +++++--- src/tagrulesdialog.h | 7 ++- tests/test_tagrules.cpp | 58 +++++++++++++++++++++- 5 files changed, 98 insertions(+), 10 deletions(-) (limited to 'tests/test_tagrules.cpp') diff --git a/CLAUDE.md b/CLAUDE.md index 67c0794..107fe28 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -317,6 +317,17 @@ current index invalid when nothing was current. A test that calls `selectAll()` view therefore passes against a missing selection guard, because no signal ever fires. Test multi-select from a row that is already current, which is also how a user reaches it. +**A `QDialog`'s buttons do not send a `QCloseEvent`.** `accept()` and `reject()` +go through `done(int)`, which hides the dialog without ever closing a window, so +a `closeEvent` override runs only for the window manager's X button. Anything a +dialog must persist on the way out belongs in a `done(int)` override, which both +buttons and `close()` reach. This shipped wrong in the rules dialog and the test +covering it passed, because the test used `close()` and the user used Cancel: +one route out of three. Assert every route. Underneath sits a second trap: +`close()` on a widget that was never shown returns early WITHOUT reaching +`done()`, so a test for the closed path has to `show()` the dialog first or it +asserts nothing at all. + **A queued load can outlive the state that started it.** `loadThread` crosses to the worker on a queued connection, so its reply lands after whatever the UI did in the meantime. The generation counter covers a superseded *query*, not a superseded *selection*: blanking the diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index d55336d..bce5742 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -4777,6 +4777,18 @@ delete. The rule shipped instead is that each column is auto-sized ONCE, on its first fill, after which its width belongs to the user however it was set. Two flags, because the count column is filled later by a reply from the worker. +**It then shipped broken once more, and the test that covered it passed.** The +save was written in `closeEvent`, and the test asserted with `close()`. Neither +button goes anywhere near either: Cancel calls `reject()`, Save calls +`accept()`, and only the window manager's X button sends a `QCloseEvent`. So +the size was kept for the one route out of three that the buttons never take, +and the user found it in one try by resizing and pressing Cancel. The save now +overrides `done(int)`, which both buttons funnel through and `close()` reaches, +and the test asserts all three routes rather than trusting one to stand for the +others. A second trap sits underneath: `close()` on a widget that was never +shown returns early without reaching `done()`, so that leg of the test has to +`show()` first or it proves nothing. + The **popup or primary window** question was put to the user and deliberately not taken: it stays a `QDialog`. Reopening it needs the unsaved-edit story that being modal currently sidesteps, and that is its own decision rather than part diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 61f2c21..54600c3 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -20,7 +20,6 @@ #include #include -#include #include #include #include @@ -297,13 +296,22 @@ void TagRulesDialog::saveUiState() m_list->header()->saveState()); } -/// Saves on close rather than on accept, so a size the user chose is kept -/// whether they pressed Save or Cancel. The window's shape is not part of the -/// edit being confirmed. -void TagRulesDialog::closeEvent(QCloseEvent *event) +/// Saves on the way out, whichever way that is. +/// +/// done() rather than closeEvent, and this distinction shipped broken: Cancel +/// calls reject() and Save calls accept(), and NEITHER sends a QCloseEvent. +/// Only the window manager's X button does. Saving from closeEvent therefore +/// kept the size for the one route the buttons never take, which is how a +/// resize followed by Cancel came back forgotten. Both buttons funnel through +/// done(), and QWidget::close() reaches it too. +/// +/// On every route, not only on accept: the window's shape is not part of the +/// edit being confirmed, so Cancel should discard the rule changes and keep +/// the size. +void TagRulesDialog::done(int result) { saveUiState(); - QDialog::closeEvent(event); + QDialog::done(result); } int TagRulesDialog::columnWidthForTest(int column) const diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index f86d210..bdcaff9 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -123,8 +123,11 @@ private slots: void applyEditsToCurrentRule(); void onSave(); -protected: - void closeEvent(QCloseEvent *event) override; +public slots: + /// Saves the window's size and column widths on the way out. Overridden + /// here rather than closeEvent because Save and Cancel do not send a + /// close event at all, only the window manager's X button does. + void done(int result) override; private: void reloadList(); diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index af30938..0d81df7 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -52,6 +52,7 @@ private slots: void aFolderRowUsesTheDropdownAndKeepsItsSuffix(); void theTextModeToggleSurvivesBeingSwitchedOn(); void theWindowSizeAndColumnWidthsSurviveAReopen(); + void theWindowSizeIsSavedOnEveryWayOutOfTheDialog(); void aReloadDoesNotDiscardARestoredColumnWidth(); private: @@ -683,8 +684,13 @@ void TestTagRules::theWindowSizeAndColumnWidthsSurviveAReopen() TagRulesDialog dialog; dialog.resize(900, 640); dialog.setColumnWidthForTest(0, 123); - // The save is on close, matching where MainWindow writes its own. - dialog.close(); + // CANCEL, not close(). The first version of this saved from + // closeEvent and asserted with close(), which passes while the real + // dialog forgets everything: Cancel calls reject() and Save calls + // accept(), and neither sends a QCloseEvent. Only the window + // manager's X button does, so the test exercised the one path the + // buttons never take. The user found it by hand in one try. + dialog.reject(); } // Asserted on the stored VALUE, not on the reopened frame. Item 46: the @@ -705,6 +711,54 @@ void TestTagRules::theWindowSizeAndColumnWidthsSurviveAReopen() qputenv("XDG_STATE_HOME", previousState); } +void TestTagRules::theWindowSizeIsSavedOnEveryWayOutOfTheDialog() +{ + // There are three ways out and they take different code paths: Cancel + // calls reject(), Save calls accept(), and the window manager's X button + // sends a QCloseEvent. Saving from closeEvent alone covers only the + // third, which is how the first version of this shipped and forgot the + // size on both buttons. done(int) is the funnel the two buttons share and + // close() also reaches, so all three are asserted here rather than + // trusting one to stand for the others. + QTemporaryDir configHome; + QTemporaryDir stateHome; + QVERIFY(configHome.isValid()); + QVERIFY(stateHome.isValid()); + const QByteArray previousState = qgetenv("XDG_STATE_HOME"); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + qputenv("XDG_STATE_HOME", stateHome.path().toUtf8()); + writeTwoRules(configHome); + + const auto widthAfter = [&](int width, const char *how) { + QFile::remove(MainWindow::uiStatePath()); + TagRulesDialog dialog; + // Shown, because QWidget::close() on a widget that was never visible + // returns early without reaching done(). The X button it stands for + // only exists on a window that is on screen, so testing the closed + // path from a hidden dialog proves nothing about it. + dialog.show(); + dialog.setColumnWidthForTest(0, width); + if (qstrcmp(how, "reject") == 0) + dialog.reject(); + else if (qstrcmp(how, "accept") == 0) + dialog.saveForTest(); + else + dialog.close(); + + TagRulesDialog reopened; + return reopened.columnWidthForTest(0); + }; + + QCOMPARE(widthAfter(121, "reject"), 121); + QCOMPARE(widthAfter(122, "accept"), 122); + QCOMPARE(widthAfter(123, "close"), 123); + + if (previousState.isEmpty()) + qunsetenv("XDG_STATE_HOME"); + else + qputenv("XDG_STATE_HOME", previousState); +} + void TestTagRules::aReloadDoesNotDiscardARestoredColumnWidth() { // The width did not survive a close, and it did not survive an ADD or a -- cgit v1.2.3 From 1dd39414fd6139f2934554fc90b1c5721603d14b Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 16:35:37 +0200 Subject: docs: the window size cannot be restored under a tiling compositor Item 75 shipped claiming the rules window remembers its size. It does not, and no code here can make it. Hyprland tiles the window to fill its slot, so the size dragged belongs to the tile. saveGeometry stores frameGeometry beside normalGeometry and restoreGeometry restores the normal one, which stays at whatever resize() last set it to. Decoded from the real state file after a hand test: frame 2248x806, normal 760x664. The dialog restores 760 correctly and still opens tiled. Three diagnoses were tried before this one and each was disproved by a probe rather than argued away: that restoreGeometry rejected the blob as off-screen, that the layout overrode a geometry applied before the first show, and that a test could tell the broken and fixed versions apart. The last one matters most: the offscreen platform returns an identical frame for both, so a size assertion passed against the bug and a mutation restoring it left the suite green. That assertion is not reinstated. The column widths, which are what actually works, keep their test. The changelog and the backlog entry are corrected to say what ships, and CLAUDE.md gains both the tiling-compositor trap and the rule that the offscreen platform cannot test window sizing at all. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 8 ++++--- CLAUDE.md | 19 +++++++++++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 27 ++++++++++++++++++---- tests/test_tagrules.cpp | 8 +++++++ 4 files changed, 55 insertions(+), 7 deletions(-) (limited to 'tests/test_tagrules.cpp') diff --git a/CHANGELOG.md b/CHANGELOG.md index 0637cd9..bbcc854 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,9 +26,11 @@ point at which they are stable. on disk, so a folder that exists but has no mail in it yet is still offered. It stays editable, so a folder in the rules file that is no longer on disk still opens and still saves. -- The tagging rules window remembers its size and the widths of the rule - list's columns. A column you widen also survives adding or deleting a rule, - which previously reset it. +- The tagging rules window remembers the widths of the rule list's columns. A + column you widen also survives adding or deleting a rule, which previously + reset it. The window's own size is saved too, but a tiling window manager + sizes the window itself, so there it opens at whatever size the tile gives + it. ### Fixed diff --git a/CLAUDE.md b/CLAUDE.md index 107fe28..6ab47b3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -328,6 +328,25 @@ one route out of three. Assert every route. Underneath sits a second trap: `done()`, so a test for the closed path has to `show()` the dialog first or it asserts nothing at all. +**Under a tiling compositor a window's size is not the application's to +restore, and the user's desktop is Hyprland.** `saveGeometry` stores +`frameGeometry` and `normalGeometry`; `restoreGeometry` restores the NORMAL +one. When the compositor tiles the window to fill its slot, the size the user +drags is the tile's, and `normalGeometry` keeps whatever the code last passed to +`resize()`. Measured against the real state file after a hand test: frame +2248x806, normal 760x664, so the dialog correctly restored 760 and correctly +looked broken. A whole session went into "the geometry restore is broken" before +the blob was decoded. Decode the stored geometry before theorising, and expect +`maximized` to read as a value no bool should hold, which is the tiled state Qt +records and does not round-trip. + +The corollary for tests: **the offscreen platform cannot test window sizing at +all.** It prints "This plugin does not support propagateSizeHints()" and returns +an identical frame for a correct restore and a broken one, verified in a +standalone program containing none of this project's code. A size assertion +there passes against both, and a mutation putting the bug back leaves the suite +green. Assert on the stored value, and leave the frame to a hand test. + **A queued load can outlive the state that started it.** `loadThread` crosses to the worker on a queued connection, so its reply lands after whatever the UI did in the meantime. The generation counter covers a superseded *query*, not a superseded *selection*: blanking the diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index bce5742..e9f18d9 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -4764,10 +4764,29 @@ the test asserts on the saved value rather than on the resulting frame. **Size: S.** -**Done 2026-08-13** on `rule-builder`, unreleased. `saveGeometry()` and the -list header's `saveState()` go to `tagrules/geometry` and `tagrules/header` in -`uistate.conf`, written on `closeEvent` so a size survives Cancel as well as -Save; `resize(760, 520)` stays as the first-run fallback. +**Done 2026-08-13** on `rule-builder`, unreleased, for the COLUMN WIDTHS. +`saveGeometry()` and the list header's `saveState()` go to `tagrules/geometry` +and `tagrules/header` in `uistate.conf`, written on `done(int)` so they survive +Cancel as well as Save; `resize(760, 520)` stays as the first-run fallback. + +**The window SIZE does not come back, and that half of the item cannot be +fixed here.** The user's desktop is Hyprland, a tiling compositor. It tiles the +window to fill its slot, so the size dragged is the tile's; `saveGeometry` +records `frameGeometry` and `normalGeometry` and `restoreGeometry` restores the +NORMAL one, which stays at whatever `resize()` last set. Decoded from the real +state file after a hand test: frame 2248x806, normal 760x664. The code restores +760 faithfully and the window still opens tiled. + +Three wrong diagnoses were tried and each was disproved by a probe rather than +by argument: that `restoreGeometry` rejected the blob as off-screen (it returns +true on the real display; the negative y is the DP-1 origin), that the layout +overrode a geometry set before the first show (a `showEvent` restore produced +the identical size), and that the offscreen test could tell the two apart (it +returns the same frame for both, so the mutation survived). + +Nothing worth building remains unless the user wants the dialog to open at a +remembered size when floated, which needs a Hyprland window rule rather than +code here. **The approach above was wrong on one point, and a test caught it.** It said to drop the `resizeColumnToContents` calls "once a saved header state exists", diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index 0d81df7..8ca7671 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -696,6 +696,14 @@ void TestTagRules::theWindowSizeAndColumnWidthsSurviveAReopen() // Asserted on the stored VALUE, not on the reopened frame. Item 46: the // offscreen platform does not honour a resize, so a frame comparison here // would report a failure the code did not cause. + // + // And on a TILING compositor the frame is not the dialog's to restore at + // all. saveGeometry stores frameGeometry beside normalGeometry, and + // restoreGeometry restores the NORMAL one; under Hyprland the window is + // tiled to fill its slot, so the size the user drags belongs to the tile + // while normalGeometry stays at whatever the code last resize()d it to. + // Measured against the real state file: frame 2248x806, normal 760x664. + // Restoring 760 there is correct behaviour, not the bug it looks like. QSettings state(MainWindow::uiStatePath(), QSettings::IniFormat); QCOMPARE(state.value(QStringLiteral("tagrules/geometry")).toByteArray() .isEmpty(), false); -- cgit v1.2.3 From 1ab12862fd47bf3816f7fad7b8a96eceb8f84272 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 16:44:16 +0200 Subject: fix(rules): a long rule no longer squeezes the rule list away Item 80. A rule with eight From conditions left the list showing about one and a half rows. The list was added with stretch 1 and the form below it with none, which looks decisive and is not: a stretch factor only distributes space above each widget's minimum, and the form's minimum grew with every condition row, so each row came straight out of the list. The builder asked for 120px with one row and 414px with eight. A QSplitter now divides the list from the editor, so the balance is the user's and is saved beside the column widths, and the condition rows sit in a QScrollArea capped at 190px so the editor cannot grow without bound however the splitter is set. The scroll area is what text mode hides; hiding the builder inside it would leave an empty frame. Three measures were tried in the test before one told the bug and the fix apart, and two passed against broken code: the dialog's minimumSizeHint does not track form rows and read 580 either way, and a qMin against the scroll area's own hint read small whether or not the cap was set, since an uncapped maximumHeight is QWIDGETSIZE_MAX. What survives mutation is the editor pane's minimum inside the splitter, plus the cap read directly, and both are asserted. A row's size hint is invalid until the event loop runs, so the test calls processEvents after selecting a rule or it measures the same height twice. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 5 ++ .../plans/2026-08-03-post-0.1.0-usability.md | 34 ++++++++++ src/tagrulesdialog.cpp | 76 ++++++++++++++++++++-- src/tagrulesdialog.h | 30 +++++++++ tests/test_tagrules.cpp | 72 ++++++++++++++++++++ 5 files changed, 211 insertions(+), 6 deletions(-) (limited to 'tests/test_tagrules.cpp') diff --git a/CHANGELOG.md b/CHANGELOG.md index bbcc854..e46584a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,11 @@ point at which they are stable. on disk, so a folder that exists but has no mail in it yet is still offered. It stays editable, so a folder in the rules file that is no longer on disk still opens and still saves. +- The rule list and the rule editor are now divided by a draggable splitter, + and the condition rows scroll instead of growing without limit. A rule with + eight senders used to squeeze the list to about one visible row, since the + editor grew with every condition and the list gave up the space. Where you + leave the divider is remembered. - The tagging rules window remembers the widths of the rule list's columns. A column you widen also survives adding or deleting a rule, which previously reset it. The window's own size is saved too, but a tiling window manager diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index e9f18d9..110938d 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -135,6 +135,7 @@ taking that too literally. | 76 | Every field in the rules dialog is free text, so a rule is easy to get wrong | workflow | M | **done** 2026-08-13 on `rule-builder`, unreleased. See `specs/2026-08-13-rule-builder-design.md` | | 77 | No way to see what a rule would collect, in the thread list | workflow | S | open; the dialog counts matches, it cannot show them | | 78 | No way to build a rule from something visible in a message | workflow | M | open; wants 76 first, so the created rule lands in a form that can hold it | +| 80 | A rule with many conditions squeezes the rule list to one visible row | defect | XS | **done** 2026-08-13 on `rule-builder`, unreleased. Follows item 76 | | 79 | Opening the rules dialog and saving destroys the first rule | defect | XS | **fixed on `rule-builder`** 2026-08-13, unreleased. Shipped in 0.16.0; damaged one real rule, repaired by hand | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -4975,6 +4976,39 @@ in a released build. **Size: XS** for the fix. The reproduction and the field repair were the work. +## 80. A rule with many conditions squeezes the rule list to one visible row + +**Observed.** A rule with eight From conditions left the rule list showing +about one and a half rows, with the second rule half cut off under the first. +Reported with a screenshot; the builder filled the window and the list it sits +under kept almost nothing. + +**Cause.** `m_list` was added to the dialog's `QVBoxLayout` with stretch 1 +(`src/tagrulesdialog.cpp:109`) and the form below it with none, which looks +like the list wins. It does not: a stretch factor only distributes space ABOVE +each widget's minimum, and the form's minimum grows with every condition row, +so each row came directly out of the list. Measured on the builder's size hint: +120px with one row, 414px with eight. + +**Approach.** Done 2026-08-13. A `QSplitter` divides the list from the editor, +so the balance is the user's and is saved to `uistate.conf` beside the column +widths, and the condition rows sit in a `QScrollArea` capped at 190px so the +editor cannot grow without bound whatever the splitter is set to. The scroll +area rather than the builder is what text mode hides, since hiding the inner +widget would leave an empty frame. + +**Constraints.** Three measures were tried before one distinguished the bug +from the fix, and two passed against broken code: the dialog's +`minimumSizeHint` does not track form rows and read 580 either way, and a +`qMin` against the scroll area's own size hint read small whether or not the +cap was set, because an uncapped `maximumHeight` is `QWIDGETSIZE_MAX`. The +assertions that survive mutation are the editor pane's minimum inside the +splitter, and the cap read directly. A row's size hint is also invalid until +the event loop has run, so the test needs `processEvents` after selecting a +rule or it measures one row's height twice. + +**Size: XS.** + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 54600c3..285043a 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -33,8 +33,10 @@ #include #include #include +#include #include #include +#include #include #include @@ -106,7 +108,26 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) tr("Matches") }); m_list->setRootIsDecorated(false); m_list->setUniformRowHeights(true); - layout->addWidget(m_list, 1); + + // The list and the editor go in a splitter, and the editor's own widget + // holds the form. A plain QVBoxLayout gave the list stretch 1 and still + // let a rule with eight condition rows squeeze it to about one visible + // row: a stretch factor only shares out space ABOVE each widget's + // minimum, and the form's grew with every row. Measured before the fix, + // the editor asked for 120px with one row and 414px with eight. + auto *editor = new QWidget(this); + auto *editorLayout = new QVBoxLayout(editor); + editorLayout->setContentsMargins(0, 0, 0, 0); + + m_splitter = new QSplitter(Qt::Vertical, this); + m_splitter->addWidget(m_list); + m_splitter->addWidget(editor); + // Neither pane collapses to nothing by dragging the handle past the end, + // which would hide the thing the user was trying to make room for. + m_splitter->setChildrenCollapsible(false); + m_splitter->setStretchFactor(0, 1); + m_splitter->setStretchFactor(1, 0); + layout->addWidget(m_splitter, 1); auto *form = new QFormLayout; m_id = new QLineEdit(this); @@ -155,7 +176,19 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) m_addExclusion = new QPushButton(tr("Add e&xclusion"), m_builder); builderLayout->addWidget(m_addExclusion, 0, Qt::AlignLeft); - form->addRow(tr("Match"), m_builder); + // The rows scroll rather than growing without bound. The splitter alone + // fixes the squeeze, but only until the user drags the handle down; this + // caps what the editor can ever demand, so a rule with thirty senders + // stays as workable as one with two. + m_builderScroll = new QScrollArea(this); + m_builderScroll->setWidget(m_builder); + m_builderScroll->setWidgetResizable(true); + m_builderScroll->setFrameShape(QFrame::NoFrame); + m_builderScroll->setHorizontalScrollBarPolicy(Qt::ScrollBarAlwaysOff); + // Roughly four rows. Enough that the common rule needs no scrolling at + // all, small enough that the list keeps most of the window. + m_builderScroll->setMaximumHeight(190); + form->addRow(tr("Match"), m_builderScroll); // The toggle sits with the QUERY line, not inside m_builder, because // switching to text mode HIDES m_builder. A checkbox parented there @@ -187,7 +220,7 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) }); form->addRow(tr("Note"), m_note); - layout->addLayout(form); + editorLayout->addLayout(form); auto *buttons = new QHBoxLayout; auto *addButton = new QPushButton(tr("&New"), this); @@ -273,6 +306,11 @@ void TagRulesDialog::restoreUiState() if (!geometry.isEmpty()) restoreGeometry(geometry); + const QByteArray splitter = + state.value(QStringLiteral("tagrules/splitter")).toByteArray(); + if (!splitter.isEmpty()) + m_splitter->restoreState(splitter); + const QByteArray header = state.value(QStringLiteral("tagrules/header")).toByteArray(); if (!header.isEmpty()) { @@ -294,6 +332,8 @@ void TagRulesDialog::saveUiState() state.setValue(QStringLiteral("tagrules/geometry"), saveGeometry()); state.setValue(QStringLiteral("tagrules/header"), m_list->header()->saveState()); + state.setValue(QStringLiteral("tagrules/splitter"), + m_splitter->saveState()); } /// Saves on the way out, whichever way that is. @@ -314,6 +354,30 @@ void TagRulesDialog::done(int result) QDialog::done(result); } +int TagRulesDialog::heightDemandedBelowListForTest() const +{ + // The builder's PREFERRED height, which is what grows with each condition + // row and what the list ends up paying for. minimumSizeHint is the wrong + // measure and reads 580 either way: a QFormLayout's minimum does not + // track its rows, so a test on it passes against the bug. + // The EDITOR PANE's minimum, which is what the splitter refuses to + // shrink below and therefore what the rule list actually pays. Not the + // builder's size hint: that grows with every row by design and is capped + // by the scroll area rather than reduced. Not minimumSizeHint on the + // dialog either, which does not track form rows at all and reads the + // same whether the bug is present or not. + return m_splitter->widget(1)->minimumSizeHint().height(); +} + +int TagRulesDialog::conditionAreaHeightForTest() const +{ + // The CAP itself, not a qMin against the scroll area's own size hint: a + // QScrollArea reports a small hint whether or not it is capped, and an + // uncapped maximumHeight is QWIDGETSIZE_MAX, so qMin picked the hint and + // the assertion passed with the cap removed. + return m_builderScroll->maximumHeight(); +} + int TagRulesDialog::columnWidthForTest(int column) const { return m_list->columnWidth(column); @@ -434,7 +498,7 @@ void TagRulesDialog::onSelectionChanged() const QSignalBlocker blockTextMode(m_textMode); m_textMode->setChecked(!m_loadedQuery.parsed); } - m_builder->setVisible(m_loadedQuery.parsed); + m_builderScroll->setVisible(m_loadedQuery.parsed); m_query->setReadOnly(m_loadedQuery.parsed); if (m_loadedQuery.parsed) @@ -818,7 +882,7 @@ void TagRulesDialog::setTextMode(bool on) // Show what the rows currently mean, then hand the string over. if (m_loadedQuery.parsed) m_query->setText(currentQueryFromRows().compile()); - m_builder->setVisible(false); + m_builderScroll->setVisible(false); m_query->setReadOnly(false); return; } @@ -847,7 +911,7 @@ void TagRulesDialog::setTextMode(bool on) m_reloading = wasReloading; m_loadedQuery = parsed; - m_builder->setVisible(true); + m_builderScroll->setVisible(true); m_query->setReadOnly(true); // The refusal above writes into the same label the load warnings use, so diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index bdcaff9..de35a55 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -31,6 +31,8 @@ class QLineEdit; class QPlainTextEdit; class QPushButton; class QRadioButton; +class QScrollArea; +class QSplitter; class QSpinBox; class QTreeWidget; class QTreeWidgetItem; @@ -107,6 +109,26 @@ public: /// the width the user had dragged. void reloadListForTest(); + /// The height the condition-row editor asks for. This is what squeezed + /// the rule list: a stretch factor only shares out space ABOVE each + /// widget's minimum, so every row added here came out of the list. + /// Measured at 120px for one row and 414px for eight before the scroll + /// area capped it. + /// + /// Asserted on instead of the list's rendered height because the + /// offscreen platform does not honour a window size, so the rendered + /// height there is not evidence of anything (see CLAUDE.md). Note the + /// size HINT, not minimumSizeHint: a QFormLayout's minimum does not track + /// its rows and reads the same either way, which passed against the bug. + int heightDemandedBelowListForTest() const; + + /// How tall the condition-row area may become. The scroll area caps it; + /// without the cap the rows grow without bound and a long rule fills the + /// window again, scrolling instead of squeezing. Asserted separately + /// because removing the cap leaves heightDemandedBelowListForTest + /// unchanged, so that measure alone does not cover it. + int conditionAreaHeightForTest() const; + signals: /// Asks the owner to run countQueries() through the worker. void countsRequested(); @@ -211,6 +233,14 @@ private: QRadioButton *m_matchAny = nullptr; QCheckBox *m_textMode = nullptr; QWidget *m_builder = nullptr; + /// Scrolls the condition rows, so a rule with many of them cannot grow + /// the editor without bound. Shown and hidden in place of m_builder for + /// text mode: hiding the inner widget would leave an empty scroll area. + QScrollArea *m_builderScroll = nullptr; + /// Divides the rule list from the editor. The list had stretch 1 and was + /// still squeezed, because a stretch factor only shares out space above + /// each widget's minimum and the form's grew with every condition row. + QSplitter *m_splitter = nullptr; QVBoxLayout *m_rowsLayout = nullptr; QVBoxLayout *m_exclusionsLayout = nullptr; QLabel *m_exclusionsHeader = nullptr; diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index 8ca7671..268c41f 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -54,6 +54,7 @@ private slots: void theWindowSizeAndColumnWidthsSurviveAReopen(); void theWindowSizeIsSavedOnEveryWayOutOfTheDialog(); void aReloadDoesNotDiscardARestoredColumnWidth(); + void manyConditionRowsDoNotSqueezeTheRuleList(); private: QString writeRules(const QString &json); @@ -321,6 +322,11 @@ void TestTagRules::switchingRulesDoesNotLeakRowsBetweenThem() // And back, to prove the first rule was not overwritten by loading the // second. dialog.selectRuleForTest(0); + // The row widgets are created during the select and their size hints are + // not valid until the layout has run, which needs the event loop: without + // this the builder reports the same height for one row and for eight, and + // the test passes against the bug. + QCoreApplication::processEvents(); QCOMPARE(dialog.rowCountForTest(), 1); QCOMPARE(dialog.queryLineForTest(), QStringLiteral("from:one.example.org")); } @@ -795,5 +801,71 @@ void TestTagRules::aReloadDoesNotDiscardARestoredColumnWidth() qputenv("XDG_STATE_HOME", previousState); } +void TestTagRules::manyConditionRowsDoNotSqueezeTheRuleList() +{ + // A rule with eight senders left the rule list showing about one and a + // half rows: the list had stretch 1, but a stretch factor only shares out + // space ABOVE each widget's minimum, and the form below it has no ceiling, + // so every condition row added to the minimum the list had to give up. + // + // Measured as the height the layout demands below the list. A rule with + // many rows must not demand materially more than a rule with one; what it + // needs beyond that belongs in the builder's own scroll area. + 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": "one", "query": "from:a.example.org", + "add": ["x"], "stage": 50, "enabled": true}, + {"id": "many", "query": + "from:a.example.org or from:b.example.org or from:c.example.org or from:d.example.org or from:e.example.org or from:f.example.org or from:g.example.org or from:h.example.org", + "add": ["y"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + dialog.show(); + + dialog.selectRuleForTest(0); + // The row widgets are created during the select and their size hints are + // not valid until the layout has run, which needs the event loop: without + // this the builder reports the same height for one row and for eight, and + // the test passes against the bug. + QCoreApplication::processEvents(); + QCOMPARE(dialog.rowCountForTest(), 1); + const int withOneRow = dialog.heightDemandedBelowListForTest(); + + dialog.selectRuleForTest(1); + QCoreApplication::processEvents(); + // Guard: the fixture must actually produce the many-row case, or this + // test passes by measuring the same rule twice. + QCOMPARE(dialog.rowCountForTest(), 8); + const int withEightRows = dialog.heightDemandedBelowListForTest(); + + // Seven extra rows at roughly 30px each would be over 200px of growth. + // A small increase is fine (the scroll area still has a minimum), a + // proportional one is the bug. + QVERIFY2(withEightRows - withOneRow < 100, + qPrintable(QStringLiteral("one row demands %1, eight demand %2") + .arg(withOneRow).arg(withEightRows))); + + // And the rows are CAPPED, not merely allowed to grow inside a scroll + // area that has no ceiling. Asserted separately because removing the cap + // leaves the assertion above green: the editor's minimum stays flat + // either way, so only the visible height of the row area distinguishes + // them. Without a cap a thirty-sender rule fills the window again, this + // time scrolling instead of squeezing. + QVERIFY2(dialog.conditionAreaHeightForTest() <= 200, + qPrintable(QStringLiteral("condition area is %1px tall") + .arg(dialog.conditionAreaHeightForTest()))); +} + QTEST_MAIN(TestTagRules) #include "test_tagrules.moc" -- cgit v1.2.3 From b8de7ce746718c7193e40b0f70cd3e2178f4ed8a Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 17:00:55 +0200 Subject: feat(rules): preview a rule's mail in the thread list Item 77. The dialog could say how many messages a rule matched and not which ones. A Preview in list button now runs the selected rule's query in the main window; the dialog stays open, since comparing the rule against its results is the point. Two constraints from the backlog entry, both now asserted and both mutation-checked. The query runs exactly as stored, with no tag:new and no wrapping parentheses. The post-new hook adds those when it applies a rule, and a preview that copied them would match nothing outside a sync window, since tag:new is set only on mail that has just arrived. The account selector is cleared first. runQuery() wraps the bar's text in the selected account's scope, and a rule query usually names its own path already, so previewing one with an account selected would scope it twice and show an empty list, which reads as "this rule collects no mail". The second mutation only fails once the test's config has an account to select: with the default empty config the selector sits on "All accounts" anyway, and asserting that a preview leaves it there passed against the mutation. Recorded in the test. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 6 ++ .../plans/2026-08-03-post-0.1.0-usability.md | 14 +++- src/mainwindow.cpp | 43 +++++++++++++ src/mainwindow.h | 26 ++++++++ src/tagrulesdialog.cpp | 24 +++++++ src/tagrulesdialog.h | 14 ++++ tests/test_tagrules.cpp | 74 ++++++++++++++++++++++ 7 files changed, 200 insertions(+), 1 deletion(-) (limited to 'tests/test_tagrules.cpp') diff --git a/CHANGELOG.md b/CHANGELOG.md index e46584a..901734d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,12 @@ point at which they are stable. on disk, so a folder that exists but has no mail in it yet is still offered. It stays editable, so a folder in the rules file that is no longer on disk still opens and still saves. +- A **Preview in list** button in the tagging rules dialog runs the selected + rule's query in the main window, so you can see which mail a rule collects + rather than only how many messages it matches. The dialog stays open. The + query runs exactly as stored, without the `tag:new` scope the hook adds, and + the account selector is cleared first, since a rule query that names its own + folder would otherwise be scoped twice and match nothing. - The rule list and the rule editor are now divided by a draggable splitter, and the condition rows scroll instead of growing without limit. A rule with eight senders used to squeeze the list to about one visible row, since the diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index 110938d..eec544e 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -133,7 +133,7 @@ taking that too literally. | 74 | "Searching..." keeps claiming a query is running while rows are already arriving | feedback | XS | open; cause measured 2026-08-11, the delay itself is the cold page cache and is not fixable here | | 75 | The tagging rules window forgets its size and its column widths | persistence | S | **done** 2026-08-13 on `rule-builder`, unreleased. The window-kind question is left open, see below | | 76 | Every field in the rules dialog is free text, so a rule is easy to get wrong | workflow | M | **done** 2026-08-13 on `rule-builder`, unreleased. See `specs/2026-08-13-rule-builder-design.md` | -| 77 | No way to see what a rule would collect, in the thread list | workflow | S | open; the dialog counts matches, it cannot show them | +| 77 | No way to see what a rule would collect, in the thread list | workflow | S | **done** 2026-08-13 on `rule-builder`, unreleased | | 78 | No way to build a rule from something visible in a message | workflow | M | open; wants 76 first, so the created rule lands in a form that can hold it | | 80 | A rule with many conditions squeezes the rule list to one visible row | defect | XS | **done** 2026-08-13 on `rule-builder`, unreleased. Follows item 76 | | 79 | Opening the rules dialog and saving destroys the first rule | defect | XS | **fixed on `rule-builder`** 2026-08-13, unreleased. Shipped in 0.16.0; damaged one real rule, repaired by hand | @@ -4891,6 +4891,18 @@ means the preview discards whatever thread load was in flight. **Size: S.** +**Done 2026-08-13** on `rule-builder`, unreleased. A **Preview in list** +button emits `previewRequested(query)`; `MainWindow::onRulePreviewRequested` +clears the account selector, puts the query in the bar and runs it, then +raises itself. The dialog stays open, which is the point. + +Both constraints above became assertions, and BOTH mutations were needed: a +test that emitted the hook's `tag:new and (...)` wrapping fails, and one that +skips the account reset fails. The second only bites once the test config +actually has an account to select, since the default empty config leaves the +selector on "All accounts" already and the assertion passed against the +mutation until that was fixed. + ## 78. No way to build a rule from something visible in a message **Observed.** The user would like to select an address or another piece of a diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index e8b4dda..e2df6de 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1324,6 +1324,9 @@ void MainWindow::showTagRulesDialog() // setFolders refills the rows that exist by then. QMetaObject::invokeMethod(m_worker, "requestFolders", Qt::QueuedConnection); + connect(dialog, &TagRulesDialog::previewRequested, + this, &MainWindow::onRulePreviewRequested); + connect(dialog, &TagRulesDialog::countsRequested, this, [this, dialog]() { QMetaObject::invokeMethod( m_worker, "requestMessageCounts", Qt::QueuedConnection, @@ -1570,6 +1573,46 @@ void MainWindow::onCountsReady(const QVector &counts, quint64 generation) m_messageView->showPlaceholder(placeholderHelpers()); } +QString MainWindow::queryTextForTesting() const +{ + return m_queryEdit->text(); +} + +QString MainWindow::selectedAccountForTesting() const +{ + return m_accountBox->currentData().toString(); +} + +void MainWindow::selectAccountForTesting(const QString &key) +{ + const int index = m_accountBox->findData(key); + if (index >= 0) + m_accountBox->setCurrentIndex(index); +} + +void MainWindow::onRulePreviewRequested(const QString &query) +{ + // Unscoped, deliberately. runQuery() wraps the bar's text in the selected + // account's scope, and a rule query usually names its own path already + // (path:"work/**" is what every account rule looks like), so previewing + // one with an account selected would scope it twice and match nothing. + // That reads as "this rule collects no mail", which is the opposite of + // what the preview is for. + m_accountBox->setCurrentIndex(0); + + // Through the query bar, like onPlaceholderQueryRequested: the bar then + // shows what is on screen and the user can edit the rule's query there + // before deciding to change the rule itself. + m_queryEdit->setText(query); + runCurrentQuery(); + + // The dialog is a separate window and may be covering this one or sitting + // beside it. Raising makes the result visible either way, and the dialog + // stays open so the two can be compared. + raise(); + activateWindow(); +} + void MainWindow::onPlaceholderQueryRequested(const QString &query) { // Through the query bar rather than straight to the worker, so the bar diff --git a/src/mainwindow.h b/src/mainwindow.h index adf63b0..18fbbb4 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -185,6 +185,27 @@ public: /// The generation the next counts reply must carry to be accepted. quint64 countsGenerationForTesting() const { return m_countsGeneration; } + /// The query bar's text, and the account the selector is scoped to + /// (empty for "All accounts"). Both are what a rule preview writes: the + /// bar so the user can see and edit what ran, and the selector because + /// runQuery() wraps the text in the selected account's scope, which would + /// double-scope a rule query that already names its own path. + QString queryTextForTesting() const; + QString selectedAccountForTesting() const; + + /// Scopes the view to one account, as choosing it in the selector does. + /// A test for the rule preview needs this: with no account selected the + /// box already sits at "All accounts", so asserting that a preview leaves + /// it there passes whether or not the preview clears it. + void selectAccountForTesting(const QString &key); + + /// Runs a rule preview without the dialog, which the offscreen platform + /// cannot click a button in. + void previewRuleQueryForTesting(const QString &query) + { + onRulePreviewRequested(query); + } + protected: void closeEvent(QCloseEvent *event) override; @@ -293,6 +314,11 @@ private slots: /// Runs a query the user clicked on the placeholder pane. void onPlaceholderQueryRequested(const QString &query); + /// Runs one tagging rule's query in the thread list, so the user can see + /// which mail it collects. The rules dialog stays open; the point is to + /// compare the rule against its results. + void onRulePreviewRequested(const QString &query); + /// Opens the auto-tagging rules editor, or raises the one already open. void showTagRulesDialog(); diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 285043a..4fca971 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -234,6 +234,11 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) buttons->addWidget(copyButton); buttons->addWidget(deleteButton); buttons->addStretch(); + m_previewButton = new QPushButton(tr("&Preview in list"), this); + m_previewButton->setToolTip( + tr("Run this rule's query in the main window, to see which mail it " + "collects. This does not tag anything.")); + buttons->addWidget(m_previewButton); buttons->addWidget(refreshButton); layout->addLayout(buttons); @@ -251,6 +256,8 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) this, &TagRulesDialog::onCopyRule); connect(deleteButton, &QPushButton::clicked, this, &TagRulesDialog::onDeleteRule); + connect(m_previewButton, &QPushButton::clicked, + this, &TagRulesDialog::previewForTest); connect(refreshButton, &QPushButton::clicked, this, &TagRulesDialog::countsRequested); connect(box, &QDialogButtonBox::accepted, @@ -369,6 +376,23 @@ int TagRulesDialog::heightDemandedBelowListForTest() const return m_splitter->widget(1)->minimumSizeHint().height(); } +void TagRulesDialog::previewForTest() +{ + // Flush any half-typed edit first, so previewing shows what the rule + // says NOW rather than what it said when the row was selected. + applyEditsToCurrentRule(); + + const int index = currentIndex(); + if (index < 0 || index >= m_working.size()) + return; + + const QString query = m_working.at(index).query; + if (query.isEmpty()) + return; + + emit previewRequested(query); +} + int TagRulesDialog::conditionAreaHeightForTest() const { // The CAP itself, not a qMin against the scroll area's own size hint: a diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index de35a55..e8979fe 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -109,6 +109,10 @@ public: /// the width the user had dragged. void reloadListForTest(); + /// Presses Preview, which is otherwise reachable only through a click on + /// a button whose geometry the offscreen platform does not guarantee. + void previewForTest(); + /// The height the condition-row editor asks for. This is what squeezed /// the rule list: a stretch factor only shares out space ABOVE each /// widget's minimum, so every row added here came out of the list. @@ -133,6 +137,15 @@ signals: /// Asks the owner to run countQueries() through the worker. void countsRequested(); + /// Asks the owner to run one rule's query in the main window, so the user + /// can see WHICH mail a rule collects rather than how much. + /// + /// The query goes out exactly as stored: no `tag:new`, and no wrapping + /// parentheses. The post-new hook adds both when it applies a rule, and a + /// preview that copied them would match nothing outside a sync window, + /// since `tag:new` is set only on mail that has just arrived. + void previewRequested(const QString &query); + public slots: /// Corpus counts, positionally paired with countQueries(). void setCounts(const QVector &counts); @@ -241,6 +254,7 @@ private: /// still squeezed, because a stretch factor only shares out space above /// each widget's minimum and the form's grew with every condition row. QSplitter *m_splitter = nullptr; + QPushButton *m_previewButton = nullptr; QVBoxLayout *m_rowsLayout = nullptr; QVBoxLayout *m_exclusionsLayout = nullptr; QLabel *m_exclusionsHeader = nullptr; diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index 268c41f..ed3a06d 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -20,6 +20,7 @@ #include #include +#include "config.h" #include "mainwindow.h" #include "rulequery.h" #include "tagrules.h" @@ -55,6 +56,8 @@ private slots: void theWindowSizeIsSavedOnEveryWayOutOfTheDialog(); void aReloadDoesNotDiscardARestoredColumnWidth(); void manyConditionRowsDoNotSqueezeTheRuleList(); + void previewEmitsTheRuleQueryAsStored(); + void previewClearsTheAccountScope(); private: QString writeRules(const QString &json); @@ -867,5 +870,76 @@ void TestTagRules::manyConditionRowsDoNotSqueezeTheRuleList() .arg(dialog.conditionAreaHeightForTest()))); } +void TestTagRules::previewEmitsTheRuleQueryAsStored() +{ + // The query goes out EXACTLY as stored: no tag:new, no wrapping + // parentheses. The hook adds both when it applies a rule, and a preview + // that copied it would show nothing at all outside a sync window, since + // tag:new is only set on mail that has just arrived. + 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": "promo", "query": "from:a.example.org or from:b.example.org", + "add": ["promo"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + QSignalSpy spy(&dialog, &TagRulesDialog::previewRequested); + + dialog.selectRuleForTest(0); + dialog.previewForTest(); + + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.first().at(0).toString(), + QStringLiteral("from:a.example.org or from:b.example.org")); +} + +void TestTagRules::previewClearsTheAccountScope() +{ + // runQuery() wraps the bar's text in the selected account's scope. A rule + // query usually names its own path already (path:"work/**"), so previewing + // one while an account is selected would scope it twice and show nothing, + // which reads as "the rule matches no mail" rather than as a UI fault. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + + // An account that can actually BE selected. With the default empty config + // the selector holds only "All accounts", so it sits at index 0 already + // and the assertion below passes whether or not the preview clears it: + // measured, the mutation removing the reset survived until this config + // was added. + const QString confPath = configHome.filePath(QStringLiteral("q.conf")); + QFile conf(confPath); + QVERIFY(conf.open(QIODevice::WriteOnly | QIODevice::Text)); + conf.write("[account.work]\nmaildir=work-mail\n"); + conf.close(); + + Config config; + config.load(confPath); + QCOMPARE(config.accounts().size(), 1); + + MainWindow window(config); + window.selectAccountForTesting(QStringLiteral("work")); + QCOMPARE(window.selectedAccountForTesting(), QStringLiteral("work")); + + window.previewRuleQueryForTesting(QStringLiteral("from:a.example.org")); + + QCOMPARE(window.queryTextForTesting(), + QStringLiteral("from:a.example.org")); + QVERIFY2(window.selectedAccountForTesting().isEmpty(), + "a preview must run unscoped, or an account-scoped rule query " + "is wrapped twice and matches nothing"); +} + QTEST_MAIN(TestTagRules) #include "test_tagrules.moc" -- cgit v1.2.3