From 2d451e35e95a580d0e81c2e1bf7220d879490864 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 10:42:56 +0200 Subject: docs: record items 75-78 and design the rule builder The standing backlog reconciliation found four unrecorded entries in the user's notes, all fallout from item 44's rules dialog now that it is in daily use: the window forgets its geometry and column widths (75), every field is free text (76), a rule cannot be previewed against the thread list (77), and there is no way to build a rule from something visible in a message (78). Each cause is verified in the code rather than copied from the note. Item 76 then went through a brainstorming pass and has a design. The shape is Thunderbird's filter window, which the user supplied as the reference: field and operator dropdowns, +/- buttons per row, an all/any radio, and a separate "but not" block. The structural point is that Thunderbird owns its filter format and this project does not. The storage is a notmuch query string shared with mailctl and executed by the post-new hook, so the builder is a view over a string rather than a store. That decides the rest: the stored format is untouched and this stays a single-repo change; a query the builder cannot represent still opens, saves and runs, in a text mode 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 sets during programmatic population. Measured against the seventeen rules in the live store, sixteen are flat and one nests an or group inside an and chain, which is what the exclusion block exists for. The parser is strict by design: it recognises a query whole or rejects it whole, because a lenient parser that salvages what it understands is how a not clause gets dropped and a filter silently widens. --- .../plans/2026-08-03-post-0.1.0-usability.md | 144 ++++++++ .../specs/2026-08-13-rule-builder-design.md | 368 +++++++++++++++++++++ 2 files changed, 512 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-13-rule-builder-design.md (limited to 'docs') 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 terms; ///< Positive section. + QList 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.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. -- cgit v1.2.3