diff options
Diffstat (limited to 'docs/superpowers')
3 files changed, 222 insertions, 16 deletions
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..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 @@ -131,10 +131,12 @@ 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` | -| 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 | +| 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 | **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 | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -4763,6 +4765,55 @@ the test asserts on the saved value rather than on the resulting frame. **Size: S.** +**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", +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. + +**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 +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 @@ -4840,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 @@ -4866,6 +4929,98 @@ 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 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 +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. + +## 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/docs/superpowers/plans/2026-08-13-rule-builder.md b/docs/superpowers/plans/2026-08-13-rule-builder.md index fb39cca..96fddc6 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) @@ -1099,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); @@ -1112,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..4d6c001 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 @@ -270,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 @@ -282,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 @@ -334,10 +352,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 |