From fe5703419f2ac2a5e619b3d527530a71a9a9499e Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 11:03:38 +0200 Subject: docs,rulequery: state the tag quoting rule rather than the test The draft compile() quoted every Is/IsNot term, which contradicted the same task's own assertion that a negated tag compiles to . The implementer resolved it in the direction the tests specify, and the resolution is right: notmuch reads tag:inbox and tag:"inbox" identically, counting 5322 either way against the live index, so quoting a tag would change the stored string without changing what it matches. That breaks the byte-for-byte round trip this type exists to guarantee. Restate the comment as the rule rather than as a note about what a test expects, correct the plan's draft so the remaining tasks do not inherit the contradiction, and warn the parser task that a quoted tag must not be read back as a quoting operator. --- docs/superpowers/plans/2026-08-13-rule-builder.md | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) (limited to 'docs/superpowers/plans') diff --git a/docs/superpowers/plans/2026-08-13-rule-builder.md b/docs/superpowers/plans/2026-08-13-rule-builder.md index fb39cca..4f65f7d 100644 --- a/docs/superpowers/plans/2026-08-13-rule-builder.md +++ b/docs/superpowers/plans/2026-08-13-rule-builder.md @@ -376,9 +376,20 @@ bool needsQuotes(const RuleTerm &term) { if (term.field == RuleTerm::Folder) return true; - if (term.op == RuleTerm::Is || term.op == RuleTerm::IsNot) + if (term.value.contains(QLatin1Char(' '))) return true; - return term.value.contains(QLatin1Char(' ')); + // Is/IsNot means an exact phrase, and only the free-text fields need + // quotes to express one. A tag or an attachment name is a single bare + // token to notmuch, which reads `tag:inbox` and `tag:"inbox"` identically + // (both count 5322 against the live index). Quoting them would therefore + // change the stored string without changing what it matches, and this + // type's whole contract is that an unedited rule compiles back byte for + // byte. + if (term.op == RuleTerm::Is || term.op == RuleTerm::IsNot) { + return term.field == RuleTerm::From || term.field == RuleTerm::To + || term.field == RuleTerm::Cc || term.field == RuleTerm::Subject; + } + return false; } QString compileTerm(const RuleTerm &term) @@ -793,6 +804,9 @@ bool parseTerm(const QString &token, RuleTerm *out) return !value.isEmpty(); } + // Tag and Attachment compile unquoted (see needsQuotes in Task 2), so + // their operator must not be inferred from the quoting: reading a quoted + // tag back as Is would compile it unquoted and change the stored string. if (field == RuleTerm::Attachment) out->op = RuleTerm::Has; else if (field == RuleTerm::Tag) -- cgit v1.2.3 From cff230c8e9dba7a95c8ee28932b7a299fc0d994a Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 11:10:35 +0200 Subject: docs: correct the claim that a paren-bearing value is unrepresentable The spec listed from:(((( among the queries the parser must reject, and the plan's Task 6 asserted that rejection. Probing the built parser shows it accepts the query as a From row whose value is the literal text, and compiles it back byte for byte. That is correct behaviour, not a leak in the strictness rule. notmuch reads those parens as characters to search for rather than as grouping, so the query is meaningful and the row displaying it tells the truth. Rejecting it would buy nothing and would push a representable rule into text mode. The distinction the documents were missing: a parenthesis inside a VALUE is not a shape question at all, only a parenthesis in grouping position is. Restate both documents accordingly, and replace the assertion with a round-trip one, which is the property that actually matters here. --- docs/superpowers/plans/2026-08-13-rule-builder.md | 21 +++++++++++++++++---- .../specs/2026-08-13-rule-builder-design.md | 21 ++++++++++++++++----- 2 files changed, 33 insertions(+), 9 deletions(-) (limited to 'docs/superpowers/plans') diff --git a/docs/superpowers/plans/2026-08-13-rule-builder.md b/docs/superpowers/plans/2026-08-13-rule-builder.md index 4f65f7d..96fddc6 100644 --- a/docs/superpowers/plans/2026-08-13-rule-builder.md +++ b/docs/superpowers/plans/2026-08-13-rule-builder.md @@ -1113,6 +1113,11 @@ void TestRuleQuery::anUnrepresentableQueryRejectsWhole() QStringLiteral("date:2026-01-01..2026-02-01"), // two-sided range QStringLiteral("from:a.example.org xor subject:x"), }; + // NOT in this list: `from:((((`. It parses, as a From row whose value is + // the literal text `((((`, and round-trips byte for byte. That is exactly + // what the query means to notmuch, which treats the parens as characters + // to search for rather than as grouping, so the row tells the truth and + // rejecting it would buy nothing. See the test below. for (const QString &query : unrepresentable) { const RuleQuery q = RuleQuery::parse(query); @@ -1126,11 +1131,19 @@ void TestRuleQuery::anUnrepresentableQueryRejectsWhole() void TestRuleQuery::aMalformedQueryIsRejectedNotDiagnosed() { - // notmuch accepts `from:((((` cleanly and matches nothing, so there is no - // failure to observe and a test asserting one fails against correct code. - // The assertion is on OUR rejection only. + // notmuch accepts `from:((((` cleanly and matches nothing: the parens are + // characters it searches for, not grouping. So there is no failure to + // observe, and a test asserting one fails against correct code. + // + // This parser accepts it too, as a From row whose value is that literal + // text, which is what the query actually means. What must hold is the + // round trip, not a rejection: displaying it as a row and compiling it + // back must not alter the stored string. const RuleQuery q = RuleQuery::parse(QStringLiteral("from:((((")); - QVERIFY(!q.parsed); + QVERIFY(q.parsed); + QCOMPARE(q.terms.size(), 1); + QCOMPARE(q.terms.at(0).value, QStringLiteral("((((")); + QCOMPARE(q.compile(), QStringLiteral("from:((((")); } ``` diff --git a/docs/superpowers/specs/2026-08-13-rule-builder-design.md b/docs/superpowers/specs/2026-08-13-rule-builder-design.md index c11ef47..a571506 100644 --- a/docs/superpowers/specs/2026-08-13-rule-builder-design.md +++ b/docs/superpowers/specs/2026-08-13-rule-builder-design.md @@ -221,7 +221,12 @@ an empty query and must open in the builder ready to receive a row. Everything else sets `parsed = false`: nested parens beyond that one shape, mixed `and`/`or` without parens, `xor`, an unrecognised prefix (`body:`, `mid:`, -`folder:`), a bare word with no prefix, a `path:` not ending in `/**`. +`folder:`), a bare word with no prefix, a `path:` not ending in `/**`, a +two-sided `date:` range, a trailing operator, an unterminated quote. + +A parenthesis **inside a value** is not a shape at all: `from:((((` is a From +term whose value happens to contain parens, and it parses and round-trips like +any other. Only a parenthesis in grouping position is a shape question. ### The parser is strict, and that is the safety property @@ -334,10 +339,16 @@ terms and tests exactly the same thing. **Rejection tests**, which carry the safety property. Queries that must set `parsed = false` and must not partially parse: nested `or` inside `or`, mixed -`and`/`or` without parens, `body:foo`, a bare word, a `path:` without `/**`, and -`from:((((`. That last asserts **our** rejection, never a provoked notmuch -failure: `CLAUDE.md` records twice that notmuch accepts it cleanly, and a test -expecting an error there fails against correct code. +`and`/`or` without parens, `body:foo`, a bare word, a `path:` without `/**`, a +two-sided `date:` range, and a trailing operator. + +**`from:((((` is not among them, and the reason is worth stating.** notmuch +treats those parens as characters to search for rather than as grouping, so the +query is meaningful, matches nothing, and reports no error. This parser accepts +it as a From row whose value is that literal text, which is what it means. The +assertion there is the **round trip**, never a rejection and never a provoked +notmuch failure: `CLAUDE.md` records twice that notmuch accepts it cleanly, and +a test expecting an error fails against correct code. **Compile tests** for every field and operator pair including both negations, and the parenthesisation rule at its boundary: `join == Any` with zero -- cgit v1.2.3 From 4cb43886ceb18cc6078ab2e8ec28fa15a1c701f2 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 11:33:09 +0200 Subject: docs: record the rules dialog data-loss defect as item 79 Opening the tagging rules dialog and pressing Save destroyed the first rule in the list, without any editing. The rule lost its query and its tags, then vanished entirely on the next load, since a rule with an empty query is dropped as malformed. Reproduced against the released tag rather than the branch, in a throwaway worktree at 9585674 with a two-rule fixture: constructing the dialog and running its save path left one rule of two. onSelectionChanged blocked signals for the note widget only, while m_enabled::toggled two lines later reached applyEditsToCurrentRule, which writes every field from widgets the loader has not filled yet. The existing comment there shows the hazard was known for one widget and not extended to the other. The fix landed with the builder work: the reloading flag now covers the whole load, and switchingRulesDoesNotLeakRowsBetweenThem is the regression test, verified by mutation to fail without the guard. The live rules file had one casualty, the account rule sitting first in the list, with both its query and its tags empty while every sibling was intact. Restored from the shell backup that the earlier migration kept and verified through mailctl's own reader. The rule had stopped tagging, but only one message had arrived meanwhile; that message is now tagged and the account is complete again at 14969 of 14969. --- .../plans/2026-08-03-post-0.1.0-usability.md | 52 ++++++++++++++++++++++ src/tagrulesdialog.cpp | 5 ++- 2 files changed, 55 insertions(+), 2 deletions(-) (limited to 'docs/superpowers/plans') 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 1216542..f9f8c53 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 | open; design approved 2026-08-13, 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 | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -4866,6 +4867,57 @@ in CLAUDE.md. **Size: M.** +## 79. Opening the rules dialog and saving destroys the first rule + +**Observed.** Open Tagging rules, press Save, change nothing. The first rule +in the list loses its query and its tags. It then vanishes entirely the next +time anything reads the file, because a rule with an empty query is dropped +as malformed on load. + +Found while building item 76, by a test written to catch a different problem. +Reproduced against the released tag rather than the branch, with a throwaway +worktree at 9585674 and a two-rule fixture: after constructing the dialog and +calling its save path, the store held one rule instead of two. + +**Cause.** `TagRulesDialog::onSelectionChanged` blocks signals for `m_note` +only. Two lines later, `m_enabled->setChecked(rule.enabled)` emits `toggled`, +which is connected to `applyEditsToCurrentRule()`. That handler writes every +field of the current rule from the widgets, and it runs BEFORE +`m_query->setText(rule.query)` has filled the query widget, so it writes the +previous rule's text. On the first open there is no previous rule and the +widgets are empty, so rule 0 gets an empty query and an empty tag list. +`TagRules::load` then drops it (`src/tagrules.cpp:150`). + +The existing comment above the `QSignalBlocker` shows the hazard was known for +`m_note` and simply not extended to `m_enabled`. A blocker per widget is the +wrong shape: the whole load needs one guard. + +**Fix.** Raise `m_reloading` for the duration of `onSelectionChanged` and +restore it afterwards, replacing the single-widget blocker. `m_reloading` +already exists for exactly this class of problem and +`applyEditsToCurrentRule()` already honours it. Also take the rule by value +rather than by const reference: the reference points into `m_working`, which +the handler mutates, so it could be read back half overwritten. + +Fixed on the `rule-builder` branch as part of item 76, with +`switchingRulesDoesNotLeakRowsBetweenThem` in `test_tagrules` as the +regression test. It fails against the unfixed code. + +**Damage in the field, and the repair.** The live rules file had exactly one +casualty: the account rule sitting first in the list, with both `query` and +`add` empty while every sibling account rule was intact. +Restored from `post-new.shell-backup`, which item 44's migration kept, and +verified by loading the file through mailctl's own reader: 17 rules, correct +scoping. The rule had stopped tagging, but only one message had arrived in the +meantime (14968 of 14969 in that account still carried the tag); it was tagged +by hand and the account is now complete. + +**Constraints.** The user chose to leave the fix on the branch rather than cut +a patch release, so 0.16.0 in the field still has it. Do not open that dialog +in a released build. + +**Size: XS** for the fix. The reproduction and the field repair were the work. + ## 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 9f4cf6e..797cd92 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -321,8 +321,9 @@ void TagRulesDialog::onSelectionChanged() m_query->setText(rule.query); m_note->setPlainText(rule.note); - // Parse once, on load, and keep it: Task 10's save path compares against - // this to decide whether the stored string may be left alone. + // Parse once, on load, and keep it: the save path compares against this to + // decide whether the stored string may be left alone, so that opening a + // rule and closing it cannot rewrite the file mailctl also reads. m_loadedQuery = RuleQuery::parse(rule.query); if (m_loadedQuery.parsed) -- cgit v1.2.3 From 2471356c99f506b6227c4f9a9399f051b85ef76c Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 11:54:28 +0200 Subject: docs: the data-loss defect took the note too The first pass of the field repair restored the query and the tags and stopped there. The note was also blank, which the user noticed: every sibling account rule carries an identical note and only the damaged rule had none. The reason it was missed is worth keeping. The shell backup was read for the tagging command, and the note comes from the comment block above it, which the migration had given to all five account rules alike. The handler at fault writes every field of a rule, so every field is equally exposed, and a repair that checks only the fields that first drew attention will leave some of the damage in place. Restored from the four siblings, which are byte-identical, and the whole file re-audited: no rule now has an empty query, id, note or tag list. --- docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'docs/superpowers/plans') 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 f9f8c53..00a7f76 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 @@ -4904,8 +4904,16 @@ Fixed on the `rule-builder` branch as part of item 76, with regression test. It fails against the unfixed code. **Damage in the field, and the repair.** The live rules file had exactly one -casualty: the account rule sitting first in the list, with both `query` and -`add` empty while every sibling account rule was intact. +casualty: the account rule sitting first in the list, with its `query`, its +`add` and its `note` all empty while every sibling account rule was intact. + +**The note was missed on the first pass of the repair**, because the shell +backup was read for the tagging command and the note comes from the comment +block ABOVE it, which the migration had given to all five account rules +alike. The user spotted the gap. `applyEditsToCurrentRule()` writes every +field, so every field is equally exposed: repair work here must check the +whole rule, not the fields that first drew attention. Restored from the four +siblings, which carry byte-identical notes. Restored from `post-new.shell-backup`, which item 44's migration kept, and verified by loading the file through mailctl's own reader: 17 rules, correct scoping. The rule had stopped tagging, but only one message had arrived in the -- 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 'docs/superpowers/plans') 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 'docs/superpowers/plans') 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 'docs/superpowers/plans') 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 'docs/superpowers/plans') 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 'docs/superpowers/plans') 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