diff options
Diffstat (limited to 'docs/superpowers/specs/2026-08-13-saved-queries-design.md')
| -rw-r--r-- | docs/superpowers/specs/2026-08-13-saved-queries-design.md | 200 |
1 files changed, 200 insertions, 0 deletions
diff --git a/docs/superpowers/specs/2026-08-13-saved-queries-design.md b/docs/superpowers/specs/2026-08-13-saved-queries-design.md new file mode 100644 index 0000000..6d0019f --- /dev/null +++ b/docs/superpowers/specs/2026-08-13-saved-queries-design.md @@ -0,0 +1,200 @@ +# Saved queries in a file of their own: design + +Backlog item 23, "No way to save a search query from the UI", and item 81, "No +way to turn a saved query into a tagging rule", which depends on it. + +**Status:** design approved 2026-08-13, not implemented. + +## The problem this solves + +A saved query can only be created by hand-editing `qtmaildir.conf`. There is no +way to keep a query you have just written and are looking at the results of, +which is exactly the moment you know it is worth keeping. + +Underneath that sits a second problem the user named separately: every saved +query becomes a button in the query row (`src/mainwindow.cpp:557`), so the row +grows without bound and cannot distinguish "Inbox", which belongs on screen +permanently, from "all messages from one correspondent", which belongs in a +menu. + +## Why the storage moves, and what that decides + +The obvious implementation writes back into `[queries]`. It was rejected, and +the reason shapes the rest of this document. + +**`[queries]` cannot express order.** It is read through +`QSettings::childKeys()`, which returns keys alphabetically and never in file +order. `src/config.cpp:401` already carries a comment saying a hand-rolled +parser would be needed to change that, and the README documents the alphabetical +button order as though it were a feature. A row of pinned buttons that cannot be +arranged is not worth building. + +**QSettings would also destroy the file it wrote to.** It preserves neither +comments nor key order, so the first save from the dialog reformats a +hand-edited config wholesale. Item 1 established that machine-written state stays +out of `qtmaildir.conf` for precisely this reason. + +So saved queries move to **`~/.config/qtmaildir/queries.json`**, and gain three +things the INI could not hold: an order, a `pinned` flag, and a per-query account +scope. + +**In `~/.config/`, not `~/.local/state/`.** A named, scoped query is user intent, +hand-editable, and belongs in a config backup; a state directory is commonly +excluded from one. `uistate.conf` keeps window blobs and column widths, which is +a different kind of thing. + +### The rules.json analogy, and where it stops + +The user proposed this by analogy with `~/.config/mailrules/rules.json`. The +analogy is right about the shape and wrong about the machinery, and the +difference is worth stating because copying too much of it would be a real cost. + +`rules.json` carries unknown-field preservation and a `kFormatVersion` handshake +because it has **two independent implementations** that agree by test rather than +by shared code: `src/tagrules.cpp` here and `mailrules.py` in `../mailctl`. All +of that exists to stop two programs destroying each other's writes. + +`queries.json` has exactly one reader. What carries across: + +- a versioned document, so a future breaking change has a way to announce itself +- unknown fields preserved through a load/save round trip, so a field added by a + later build is not stripped by an older one + +What does not carry across: any notion of a second implementation, a shared +contract, or a two-repo change. This is a single-repo change and must stay one. + +## The format + +```json +{ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true }, + { "name": "Unread", "query": "tag:unread", "pinned": true }, + { "name": "Work invoices", "query": "from:billing", "account": "work" } + ] +} +``` + +`queries` is an **array**, and its order is the display order. That is the whole +point of the change; nothing may sort it on load. + +| field | required | meaning | +|---|---|---| +| `name` | yes | Display name, and what `startup_query` matches against | +| `query` | yes | notmuch query, wire format, never translated | +| `pinned` | no, default `false` | Renders as a button; otherwise it lives in the menu | +| `account` | no | An account **key**, the INI group suffix (`work` from `[account.work]`), not a path or a display name | + +**`account` stores the key, not the maildir path.** `Account::scopedQuery()` +(`src/config.cpp:42`) composes `path:"<maildir>/**"` from the account, so storing +the path would duplicate config that already exists in one place and go stale +when the user edits it. A key naming an account that no longer exists resolves to +no scope, with a config problem reported the way an unmatched `startup_query` +already is. + +**Scope composes, it does not replace.** A query with an `account` runs as +`Account::scopedQuery(query)`, which yields `path:"<maildir>/**" and (<query>)`. +The parenthesisation is load-bearing for the same reason it is in the rules hook: +`path:... and a or b` binds as `(path:... and a) or b`, so an unparenthesised +disjunction escapes its scope and matches every account. `scopedQuery` already +does this correctly; the requirement is to use it rather than concatenating. + +## Migration + +On a run where `queries.json` does not exist and `[queries]` does: read the INI +section, write the JSON, and **leave `[queries]` in place, untouched**. From then +on the JSON is the only source and `[queries]` is ignored entirely. + +**Do not strip the section.** Removing it means rewriting the hand-edited file +with QSettings, which drops comments and key order across the whole file, not +just the part being removed. That is the loss this design exists to avoid. +Leaving it costs a few stale lines the user can delete by hand whenever they +like, and keeps a downgrade to an older build working. + +Migrated entries get `pinned: true`, since they are buttons today and the +migration must not silently empty the query row. Order is alphabetical on +migration because that is genuinely all the INI knows; the user reorders once, by +hand or in the dialog, and it sticks from then on. + +**Rejected: reading both forever.** Two sources of truth for one thing, with no +answer for a name colliding across them and no obvious place for the dialog to +write. + +## Consequences for `startup_query` + +`[general] startup_query` names a saved query and resolves case-insensitively, +falling back to the first entry (`Config::startupSavedQuery()`, +`src/config.cpp:422`). It keeps working unchanged, but **its fallback quietly +changes meaning**: "the first entry" stops being "alphabetically first" and +becomes "first in the user's own order". + +That is an improvement, and it is still a user-visible behaviour change for a +config whose `startup_query` matches nothing. It belongs in the changelog, and +it makes this a minor version bump rather than a patch. + +## The UI + +### A Save query button beside the search bar + +Opens a dialog on the current query bar contents, holding: + +- **name**, required, and unique. A name that already exists offers to overwrite + rather than silently creating a duplicate the menu would render twice. +- **query**, editable, prefilled from the query bar. +- **account scope**, a dropdown of configured accounts plus "All accounts". +- **pinned**, a checkbox: button or menu. + +### The button row moves to a row of its own + +Today the query row holds the account box, the sort dropdown, the query bar and +every saved-query button (`src/mainwindow.cpp:554-587`). Pinned queries move to +their own row beneath it, and the unpinned ones go into a menu. + +**There is no `SavedQueryBar` class**, whatever older revisions of the backlog +said. These buttons are built inline in `MainWindow` and this is a change to that +layout code. + +Sent stays where it is. It is not a saved query, it is built from +`allSentQuery()` (`src/mainwindow.cpp:579`), and folding it into `queries.json` +would mean generating a per-account path query into stored config, which is the +duplication the `account`-key decision just rejected. + +## What is deliberately not here + +**Item 81, saving a query as a tagging rule.** A saved query is a view and costs +nothing if it is wrong. A rule is applied to real mail by the `post-new` hook +every ten minutes and lives in the file shared with mailctl. Keeping it out means +item 23 ships as a single-repo change; folding it in would give a presentation +change a two-repo commitment and a live blast radius. + +**Item 10's account persistence.** Per-query account scope answers item 10's +remaining complaint as a side effect, which is noted in both entries. Item 10 +itself stays postponed and is not reopened here: the user asked that the rest of +it not be proposed unprompted. + +## Testing + +`Config` is already unit-tested, and this is mostly a format with an ordering +guarantee, so most of it is testable without a window. + +- Round trip: load, save, load, and assert the document is unchanged **including + order**, which is the property the INI could not provide. +- Unknown fields survive a round trip. Write a field no build knows, load, save, + assert it is still there. +- Migration: an `[queries]` section and no JSON produces a JSON file with every + entry `pinned`, and leaves the INI file **byte-identical**. Assert on the file + bytes, not on a re-read through QSettings, which would pass against a rewrite + that happened to preserve values while dropping comments. +- A malformed or unreadable `queries.json` reports a config problem and does not + take the query row down with it. +- `startup_query` still resolves by name, and its fallback now returns the first + entry in document order. +- Scope composition yields `path:"<maildir>/**" and (<query>)` with the + parentheses present. Assert on the composed string for a query containing + `or`, which is the case that breaks without them. + +**The dialog itself needs a hand test**, per the offscreen-platform constraints +already recorded in CLAUDE.md: sizing cannot be asserted there at all, and a +dialog's Cancel path goes through `done(int)` rather than `closeEvent`, so a test +that only exercises `close()` proves less than it appears to. |
