diff options
Diffstat (limited to 'docs/superpowers/specs/2026-08-13-rule-builder-design.md')
| -rw-r--r-- | docs/superpowers/specs/2026-08-13-rule-builder-design.md | 368 |
1 files changed, 368 insertions, 0 deletions
diff --git a/docs/superpowers/specs/2026-08-13-rule-builder-design.md b/docs/superpowers/specs/2026-08-13-rule-builder-design.md 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. |