diff options
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 144 | ||||
| -rw-r--r-- | docs/superpowers/specs/2026-08-13-rule-builder-design.md | 368 |
2 files changed, 512 insertions, 0 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 26464ba..1216542 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,6 +131,10 @@ 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 | +| 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 | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -4722,6 +4726,146 @@ inaccurate, not because anything is expected to happen. here and should not be attempted: prefaulting 1.1 GB at startup to make one query look fast is a worse trade than the wait. +## 75. The tagging rules window forgets its size and its column widths + +**Observed.** The rules window opens at the same size every time, whatever +size it was left at, and the rule table's columns reset to their computed +widths on every open. The user also asks whether it should present as a +primary window rather than a popup. + +**Cause.** `TagRulesDialog::TagRulesDialog` calls `resize(760, 520)` +unconditionally (`src/tagrulesdialog.cpp:59`) and never reads or writes a +saved geometry; there is no `saveGeometry`/`restoreGeometry` pair anywhere in +the file, and `MainWindow` is the only class that touches `uiStatePath()` +(`src/mainwindow.cpp:141,183`). The columns reset because `reloadList()` +calls `resizeColumnToContents` for the enabled and stage columns on every +repopulate (`src/tagrulesdialog.cpp:213-214`), and `onCountsReady` does the +same for the count column (`:326`); a width the user dragged is discarded by +the next reload, not only by a close. + +**Approach.** Save `saveGeometry()` and `m_list->header()->saveState()` into +the machine-written UI state file under keys of their own, and restore both +in the constructor, keeping the current `resize` as the fallback for a first +run. Drop the unconditional `resizeColumnToContents` calls once a saved +header state exists, or the restore is undone on the first reload. + +The "popup or primary window" question is a separate decision and not a +defect: the class is a `QDialog` (`src/tagrulesdialog.h:41`), which is what +makes it modal to the main window and what puts it above it. Changing it to a +top-level window means it can be left open beside the main window and can go +behind it, and the rule edits would then need to survive that. Ask before +changing it. + +**Constraints.** UI state goes to `~/.local/state/qtmaildir/uistate.conf` via +`MainWindow::uiStatePath()`, never into the hand-edited config. A geometry +restore under the offscreen platform is what item 46 already tripped over, so +the test asserts on the saved value rather than on the resulting frame. + +**Size: S.** + +## 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 +would rather choose from buttons, radios and completion, with typing reduced +to the parts that genuinely have to be typed. + +**Cause.** Not a defect, this is what shipped. The form is four bare +`QLineEdit`s for id, add, remove and query plus one `QCheckBox` +(`src/tagrulesdialog.cpp:88-96`), and none of them is attached to a +completer. The tag completion machinery already exists as `QueryCompleter` +(`src/querycompleter.h:90`) and is not used here. + +**Approach.** Designed 2026-08-13. **Read +`specs/2026-08-13-rule-builder-design.md` instead of planning from this +entry.** A `RuleQuery` value type parses and compiles the query string, and a +row builder in the dialog edits it, in the shape the user asked for after +showing Thunderbird's filter window: field and operator dropdowns, `+`/`-` +buttons, an all/any radio, and a separate "but not" block. + +Three constraints decide whether to open the spec at all. **The stored format +does not change**, so this is a single-repo change and mailctl needs no edit. +**The query string stays authoritative**, so a rule the builder cannot +represent still opens, saves and runs, in a text mode that every rule +carries. And **the string is rewritten only when the rows actually changed**, +compared against the parsed value rather than tracked with a dirty flag, +which Qt would set during programmatic population. + +Measured against the seventeen real rules: sixteen are flat, one nests an +`or` group inside an `and` chain, which is what the exclusion block exists +for. + +**Completion is the other half of this item** and is independent of the +builder; it can land before or after. It reuses `QueryCompleter` +(`src/querycompleter.h:90`) for tag names on the add and remove fields. + +**Constraints.** A multi-value field must not use `QLineEdit::setCompleter`. +CLAUDE.md records this trap twice over: the line edit overwrites the +completer's prefix with the widget's whole text, so the first tag completes +and nothing after it does. Attach with `QCompleter::setWidget` and drive the +prefix by hand. And a test that uses `setText()` passes against the bug, +because `setText` never drives a completer at all, so the keys have to be +typed. + +The hook refuses to remove `unread` or `inbox`, so a builder that offers +those as removable tags produces a rule that silently does nothing. Say so in +the UI rather than letting the hook decline it invisibly. + +**Size: M.** + +## 77. No way to see what a rule would collect, in the thread list + +**Observed.** The user wants a button that runs the rule's query in the main +window, to look at what it would collect rather than at how many. + +**Cause.** Not a defect. The dialog already counts matches, over +`requestMessageCounts` and `messageCountsReady` +(`src/notmuchworker.h:172`, `src/mainwindow.cpp:1322-1335`), which answers +"how many" and cannot answer "which". Nothing carries a query from the dialog +back to the query bar. + +**Approach.** One signal from the dialog carrying the rule's query string, +and a slot on `MainWindow` that puts it in the query bar and runs it. The +dialog stays open, since the point is to compare the two. + +**Constraints.** The stored query carries no scope on purpose: the hook +supplies `tag:new` and wraps the rule's query in parentheses. A preview must +therefore run the query WITHOUT `tag:new`, or it shows nothing at all outside +a sync window, and it must not add the parentheses silently either, since +what the user is checking is the query as stored. + +A count request must never bump `m_generation`; item 44 already had to add +`m_ruleCountGeneration` for exactly this reason (`src/mainwindow.h:657-664`). +A preview is a real query and does bump it, which is correct, but it also +means the preview discards whatever thread load was in flight. + +**Size: S.** + +## 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 +message in the main window, right-click, and be offered a rule built from it. + +**Cause.** Not a defect, unbuilt. The thread list has a context menu (item +24) and the message pane is a `QWebEngineView` whose selection is inside the +render process. + +**Approach.** Start from the thread list's own context menu, where the +sender is already a value the model holds, rather than from a text selection +in the web view. A "Create rule from sender" entry that opens the rules +dialog with the query prefilled covers the case the user described and needs +no new plumbing. + +**Constraints.** JavaScript is disabled in the profile and must stay +disabled, so reading a selection out of the web view means +`QWebEnginePage::selectedText()` and nothing that injects script. Do that +part only if the sender case turns out not to be enough. + +The rules file is shared with mailctl, so a rule created here must go through +`TagRules` and preserve unknown fields; see "Changing the shared rule format" +in CLAUDE.md. + +**Size: M.** + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/docs/superpowers/specs/2026-08-13-rule-builder-design.md b/docs/superpowers/specs/2026-08-13-rule-builder-design.md new file mode 100644 index 0000000..c11ef47 --- /dev/null +++ b/docs/superpowers/specs/2026-08-13-rule-builder-design.md @@ -0,0 +1,368 @@ +# A row builder for tagging rules: design + +Backlog item 76, "Every field in the rules dialog is free text, so a rule is +easy to get wrong". + +**Status:** design approved 2026-08-13, not implemented. + +## The problem this solves + +Item 44 shipped a rules dialog whose query field is a bare `QLineEdit` +(`src/tagrulesdialog.cpp:96`). A rule is written by typing a notmuch query by +hand, which puts the whole burden of the syntax on the user at exactly the +moment they are least likely to catch a mistake, because **notmuch's parser +rejects almost nothing**. `from:((((` parses cleanly and matches nothing. A +mistyped `path:` without its `/**` suffix matches nothing. Neither reports an +error anywhere; the rule simply stops tagging, silently, until somebody notices +mail is not being filed. + +The user asked for the shape Thunderbird uses: dropdowns to build the logic, +`+`/`-` buttons to add and remove conditions, radio buttons for the joining +logic, and typing reduced to the values that genuinely have to be typed. + +## The structural difference from Thunderbird, and why it decides the design + +Thunderbird owns its filter format. Its rows **are** the storage. + +Here they cannot be. The storage is a notmuch query string in +`~/.config/mailrules/rules.json`, shared with the companion `mailctl` project +and executed by the `post-new` hook. The hook runs the query; it knows nothing +about rows and never will. + +So the builder is a **view over a string**, not a store. Everything below +follows from that: + +- Rows compile to a query. That direction is easy. +- A query must parse back into rows. That direction is a parser, and every + existing rule was written by hand. +- A rule the parser cannot represent must still open, still save, and still + run, unchanged. + +## What is being built + +A `RuleQuery` value type that parses and compiles the query string, and a +builder section in `TagRulesDialog` that edits it. **The stored format does not +change.** `TagRule` gains no field, `rules.json` gains no key, and `mailctl` +needs no edit. This is a single-repo change, which is the main thing the design +buys by leaving the query string authoritative. + +## Decisions taken, with the alternatives that were rejected + +### Flat rows plus a separate exclusion block + +Thunderbird offers one exclusive radio: match **all** conditions, or match +**any**. Measured against the user's seventeen real rules, that covers sixteen. +The seventeenth is an `or` group nested inside an `and` chain, which is the +shape a person reaches for when they mean "from any of these senders, but not +when the subject looks like this". + +The builder therefore has two sections: a positive section governed by the +all/any radio, and a **"but not" block** whose rows are always joined `and not`. +That takes the real corpus from 16/17 to 17/17. + +Rejected: flat-only, which would leave a permanent second-class rule that the +user looks at often. + +### The query string is only rewritten when rows actually changed + +Opening a rule, looking at it, and closing must not rewrite the file. A +recompile that is semantically identical but textually different +(`a or b` becoming `(a or b)`) churns a file that a second tool reads. + +The dialog keeps the `RuleQuery` it parsed and compares the current widget state +against it on save. Equal means the stored string is written back byte for byte. + +Rejected: a dirty flag driven by widget `changed` signals. Qt emits those during +programmatic population, so loading a rule into the form would mark it dirty +before the user touched anything, rewriting the file on open. The comparison +approach has no such failure, and it correctly treats an edit that was manually +undone as clean. + +Rejected: storing the rows in the JSON alongside the query. That is a two-repo +format change, obliges `mailctl` to preserve a field it does not use, and makes +the rows authoritative over the query the hook actually runs. + +### The exclusion block is the preferred reading of a trailing negation + +Per-row negation and the exclusion block overlap: `not subject:x` can be a +negated row in the positive section or a row in the block. Both compile +correctly. When parsing, the block wins whenever the negations sit at the end of +an `and` chain, because that is how the user describes these rules in words and +how the original shell hook's comments reasoned about them. + +Consequence, accepted: a query written with per-row negations may come back +displayed as block exclusions. Semantically identical, and nothing is rewritten +unless the rule is edited. + +### Text mode is a toggle on every rule, not an error state + +An unparseable rule opens with the toggle already flipped. There is no special +mode, no disabled builder, and no error. Every rule has both views, which also +gives an escape hatch on rules the builder *can* express but clumsily. + +Rejected: hiding the builder, which reads as something breaking; and showing it +disabled and empty, which is a false affordance. + +## `RuleQuery` + +`src/rulequery.h`, `src/rulequery.cpp`. A plain value type with no widget +dependency, following `TagRules` and `CardLayout`: fully unit-testable without a +UI or a painter. + +```cpp +struct RuleTerm +{ + enum Field { From, To, Cc, Subject, Tag, Folder, Attachment, Date }; + enum Op { Contains, ContainsNot, Is, IsNot, Has, HasNot, Before, After }; + + Field field; + Op op; + QString value; +}; + +struct RuleQuery +{ + enum Join { All, Any }; ///< and / or, over the positive terms only. + + Join join = All; + QList<RuleTerm> terms; ///< Positive section. + QList<RuleTerm> exclusions; ///< The "but not" block, always joined and-not. + + /// False when the query cannot be represented as rows. NOT an error: the + /// rule opens in text mode and saves unchanged. + bool parsed = false; + + static RuleQuery parse(const QString &query); + QString compile() const; +}; + +bool operator==(const RuleQuery &a, const RuleQuery &b); +``` + +`parse` never fails and never throws; it returns a value with `parsed = false`. +`compile` is called only on a parsed value whose rows were edited. + +### Data flow + +Opening a rule: + +``` +TagRule.query ──parse()──> RuleQuery ──> builder widgets + │ + └─ parsed == false ──> text mode, toggle flipped +``` + +Saving: + +``` +rows edited? yes ──compile()──> TagRule.query + no ─────────────> TagRule.query unchanged, byte for byte +``` + +## Grammar + +### What compiles + +| Field | Operators | Compiles to | +|---|---|---| +| From | contains / is | `from:x` / `from:"x"` | +| To | contains / is | `to:x` / `to:"x"` | +| Cc | contains / is | `cc:x` / `cc:"x"` | +| Subject | contains / is | `subject:x` / `subject:"x"` | +| Tag | is | `tag:x` | +| Folder | is | `path:"x/**"` | +| Attachment | has | `attachment:x` | +| Date | before / after | `date:..x` / `date:x..` | + +Every operator has a negated twin (`contains not`, `is not`, `has not`) which +prefixes `not `. Date has none: "not before" is "after". Tag and Folder carry +only is / is not, because "contains" is meaningless for an exact token and an +exact path. + +**Folder appends the `/**` suffix itself.** A `path:` without it matches +nothing, and notmuch does not report that. + +**Quoting.** `is` quotes, `contains` does not. A value containing a space is +quoted regardless, or the query breaks. A value containing a `"` is refused at +the row rather than escaped: notmuch's quoting rules inside a quoted phrase are +not worth modelling for a case that has never occurred. + +### How a query assembles + +Positive terms joined by the radio, then exclusions appended as `and not`: + +``` +join = Any +terms = [ from:vendor.example.org, from:vendor.example.net ] +exclusions = [ subject:receipt, subject:refund ] + + → (from:vendor.example.org or from:vendor.example.net) + and not subject:receipt and not subject:refund +``` + +**Parenthesisation rule:** the positive group is parenthesised when +`join == Any` **and** there is at least one exclusion. Otherwise no parens. + +This is the same binding hazard `CLAUDE.md` already records for the hook's +`tag:new` scope: `tag:new and a or b` binds as `(tag:new and a) or b`. Inside a +rule the same mistake turns a narrow disjunction into a filter that matches +everything. + +### What parses + +A query parses into rows when it is: + +- a flat `and` chain of recognised terms, or +- a flat `or` chain of recognised terms, or +- a parenthesised `or` chain followed by `and not` terms, all flat. + +**An empty query parses to zero rows**, not to a failure. One existing rule has +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 `/**`. + +### The parser is strict, and that is the safety property + +**Recognise the whole query or reject it whole. Never partially.** + +A lenient parser that salvages the parts it understands is how a `not` clause +gets silently dropped and a filter quietly widens. Strict rejection means the +worst outcome is text mode, never a wrong rule. + +**This parser is not notmuch's parser and must not pretend to be.** It +recognises the shapes this builder emits plus the shapes the existing rules use. +A query it rejects is not invalid: notmuch accepts `from:((((` happily. The +wording in the UI is "can't be shown as rows", never "invalid". + +## Extending this later + +The design is future-proof in one specific dimension, and it is worth being +precise about which. + +**What can never need a redesign:** any query notmuch accepts stays expressible, +saveable and runnable, whether or not the builder understands it. The builder is +not the storage. A rule written years from now with a prefix nobody anticipated +opens in text mode, saves correctly, and runs correctly. No migration, nothing +lost. + +**Three extension points, in ascending cost:** + +| Change | Cost | +|---|---| +| A new field (`body:`, `reply-to:`, `mid:`) | One entry in the enum, the compile switch, the parse table. Additive. | +| A new operator on an existing field | Same, additive. | +| A new query **shape** (nested `or` within `or`, three-level parens, `xor`) | A parser change. | + +Only the third is real work, and hitting it costs a parser extension rather than +a redesign, because storage never depended on the parser. For calibration: the +user's seventeen rules use two shapes, and Thunderbird offers exactly two after +decades. + +**`parsed = false` is a first-class state, not an error path**, and is tested +with real unparseable queries rather than asserted unreachable. + +## The dialog + +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 + [From v] [contains v] [vendor.example.org ] [+] [-] + [From v] [contains v] [vendor.example.net ] [+] [-] +But not + [Subject v] [contains v] [receipt ] [+] [-] + [Subject v] [contains v] [refund ] [+] [-] + [+] add exclusion + +Add tags [ vendor, receipts ] +Remove tags [ ] +Note [ ... ] + +Query (from:vendor.example.org or ...) and not subject:receipt [Count matches] +``` + +**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 +editable: one widget, two states. + +**"Edit as text" is always present.** Flipping it on shows the compiled query, +editable. Flipping it back re-parses: on success the builder repopulates, on +failure the checkbox refuses to clear and says why. + +**The "But not" block appears only when it has rows**, plus an "add exclusion" +affordance. Sixteen of seventeen rules have no exclusions and an empty block on +every rule is noise. + +**Folder rows get a dropdown** populated from the accounts in `Config`, so `/**` +is never typed. A folder present in the file but absent from the config still +displays, as an editable entry, or opening an old rule would silently blank it. + +**Count matches is unchanged.** It already works and is generation-stamped on +`m_ruleCountGeneration` (`src/mainwindow.h:657-664`), which must stay separate +from `m_generation`: bumping the query generation for a count discards any +thread load in flight and blanks the message pane. In text mode it counts what +was typed. + +**Completion**, the other half of item 76, is independent of this design and can +land before or after it: tag names on the add and remove fields, the query +grammar in text mode. It carries its own trap, recorded in `CLAUDE.md`: a +multi-value field must not use `QLineEdit::setCompleter`, because the line edit +overwrites the completer's prefix with the widget's entire text, so the first +tag completes and nothing after it does. Attach with `QCompleter::setWidget` and +drive the prefix by hand. A test using `setText()` passes against that bug, +since `setText` never drives a completer; the keys must be typed. + +## Testing + +`tests/test_rulequery.cpp`, a plain unit test with no widget. + +**The corpus test is the one that matters.** Every real query shape, asserted to +parse, compile back byte-identical, and compare equal after a round trip. That +single test is the whole "the user's file does not churn" guarantee, and it is +what catches a parenthesisation slip on the one nested rule. + +Those queries go in as **generic placeholders**, never the user's real senders +or account names, per the standing rule that nothing personal reaches a commit. +The shapes are what is under test and they survive substitution intact: a +disjunction of eight job-alert senders becomes eight `from:jobs<n>.example.org` +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. + +**Compile tests** for every field and operator pair including both negations, +and the parenthesisation rule at its boundary: `join == Any` with zero +exclusions gets no parens, with one exclusion gets parens. + +**A mutation check on the corpus test.** Make `compile()` always parenthesise; if +the corpus test still passes it is not testing what it claims. Per `CLAUDE.md`, a +passing test here proves nothing without one. + +**Dialog-level tests**, added to `test_tagrules`: + +- Open a rule, change nothing, save, assert the stored string is **byte-identical**. +- Open a rule with exclusions, toggle to text and back, assert the builder state survives. +- Open an unparseable rule, assert the toggle starts flipped and refuses to clear. + +Not tested, deliberately: populating the folder dropdown from a live notmuch +index. That needs the fixture database for little value; tests populate it from +`Config`. + +## Out of scope + +**Item 77, previewing a rule's matches in the thread list**, and **item 78, +building a rule from a right-click**. Both touch this dialog and both are +independent of the builder. Item 78 wants this design to land first, so the +created rule arrives in a form that can hold it. + +**Backfill** remains out of scope, as item 44's spec records. Nothing here +changes that. |