diff options
Diffstat (limited to 'AGENTS.md')
| -rw-r--r-- | AGENTS.md | 1244 |
1 files changed, 1244 insertions, 0 deletions
diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..a642840 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,1244 @@ +# AGENTS.md + +This file is the single source of truth for how to work on this repository, +shared across every agent tool (Claude Code, opencode, etc). `CLAUDE.md` beside +it is a pointer to this file and holds no content of its own. + +Prose throughout this repository, in the specs, the plans, the source comments +and the tests, refers to this document as `CLAUDE.md`, which is what it was +called until it moved. Those references are historical records and are correct +about the content; read them as naming this file. + +## Build and test + +```bash +cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug +cmake --build build +ctest --test-dir build --output-on-failure +``` + +Run a single test binary directly for a tighter loop, **always with the +offscreen platform**: + +```bash +QT_QPA_PLATFORM=offscreen ./build/tests/test_keymap +``` + +Or by ctest name (the name is the suffix, not the binary): `ctest --test-dir build -R keymap`. + +**Never run a test binary without `QT_QPA_PLATFORM=offscreen`, and never launch +`./build/src/qtmaildir` unasked.** `tests/CMakeLists.txt` sets that variable for +ctest only, so a binary invoked directly inherits the desktop's own setting +(`wayland;xcb` here) and throws real windows onto the user's screen. Each test +function builds its own `MainWindow`, so one direct run of `test_mainwindow` +flashes over a hundred windows across the desktop. This is not cosmetic: the +user has asked for it to stop, having been given a headache by it. + +The same applies to the application. Running it is a hand test and belongs to +the user; ask rather than launching it, and when a change genuinely needs +looking at, say what to look for and let them run it. + +Adding a test: create `tests/test_<name>.cpp` and add `add_qtmaildir_test(<name>)` to +`tests/CMakeLists.txt`. That function links `qtmaildir_lib` and `Qt6::Test` and registers +the test. Fixture-driven tests get `FIXTURE_DIR` via `target_compile_definitions` (see +`test_mimeparser`). + +Adding a source file: add the `.cpp` to the `qtmaildir_lib` list in `src/CMakeLists.txt`. +All logic lives in that static library; the `qtmaildir` executable is only `main.cpp`, so +tests can link everything without duplicating source lists. + +## Environment constraints (verified 2026-08-02, Slackware) + +- **notmuch installs no `notmuch.pc`.** CMake locates it with `find_path`/`find_library`. + Never convert it to `pkg_check_modules`. GMime does ship `gmime-3.0.pc` and uses pkg-config. +- Qt 6.11.1 including WebEngine ships in Slackware's monolithic `qt6` package; there is no + separate `qt6-webengine`. +- CMake 4.3.4 rejects `cmake_minimum_required(VERSION <3.5)`. Keep 3.21. +- **gmime headers must be included before any Qt header in the same translation unit.** + glib declares a struct field named `signals`, which Qt defines as a macro. + +## Architecture + +One process, two threads. A GUI counterpart to neomutt over a local notmuch-indexed +Maildir. **No network protocol work at all** — fetching and sending are external scripts. + +``` +UI thread Worker thread +MainWindow NotmuchWorker + ├ query row: QComboBox, QLineEdit, └ owns the only notmuch_database_t* + │ saved-query QPushButtons + ├ ThreadListView (QTreeView) ── ThreadListModel (QAbstractItemModel) + │ ONE column of cards; CardDelegate paints each whole, from CardLayout + └ MessageView (a QStackedWidget over two pages) + ├ the message page (header QLabel, QWebEngineView, attachment bar, TagStrip) + └ ThreadDashboard (a conversation, from a ThreadDigest; its own TagStrip) + +ComposeWindow (its own top-level window, one per message being written) + ├ MarkdownFormat (namespace: what the formatting buttons do to a selection) + ├ MarkdownRenderer (namespace, cmark-gfm) MessageBuilder (namespace, GMime) + ├ MessageSender (QProcess, the per-account send_command on stdin) + └ SendDialog (the undo countdown) DraftStore (autosave to the drafts folder) + +ComposeContext (a struct: what a Reply or Forward inherits) +ComposeContextBuilder (namespace: fills one, and picks the account) +CardLayout (pure geometry, no painting) +SearchTerm (pure query strings, no widget) +Config (INI) KeyMap MailSync (QProcess) MimeParser (GMime) +SyncMonitor (/proc/locks) TagColors QueryCompleter ThreadCidMap +MaildirName (fresh Maildir filenames) +``` + +The query row and the message-pane header are **built inline in `MainWindow` and +`MessageView`**, not as named widget classes. Earlier revisions of this diagram +listed `QueryBar`, `SavedQueryBar`, `HeaderWidget` and `AttachmentBar`; none of +those types have ever existed, and looking for them wastes a search. The widget +classes that do exist are `MessageView`, `ThreadListView`, `TagStrip`, +`TagDialog`, `MessageDetailsDialog`, `PendingChangesDialog`, +`RowStyleDelegate`, `CardDelegate`, `ComposeWindow`, `SendDialog`, +`ThreadDashboard` and `BusyIndicator`; `TagChip` is a namespace of +painting helpers, not a widget, `SearchTerm` is a namespace of query builders, +and `ThreadCidMap`, `CardLayout`, `SearchOffer`, `HeaderRow`, `ThreadDigest` +and `PendingChangeRow` are structs. +`SubjectDelegate` existed until item 53 and is gone. + +**The compose units are mostly NAMESPACES, and the same warning applies to +them.** `MarkdownRenderer`, `MarkdownFormat`, `MessageBuilder`, +`ComposeContextBuilder`, `DraftStore` and `MaildirName` are namespaces of free +functions over values, deliberately, so the markdown, the MIME assembly and +the account-picking are all testable without a widget. `MessageSender` IS a +QObject, because it owns a `QProcess`. There is no `FormatToolbar` class: the +composer's formatting row is built inline in `ComposeWindow` and asks +`MarkdownFormat` what each button does to the selection. + +**`MessageDetailsDialog` was a `QPlainTextEdit` inside `MessageView` until item +85.** It is rows now so each value can carry its own context menu, and its +plain-textness was a SECURITY property rather than a style: header values come +from strangers and plain text cannot interpret markup. Every value label states +`Qt::PlainText` explicitly, because a `QLabel` guesses under `Qt::AutoText`. +Escaping into a rich-text label is the same protection one mistake away from +failing, so do not "simplify" it back. + +**`ThreadListView` survives only for the expander hit-test.** `CardDelegate` +draws the reply count, and a delegate gets no click of its own without an +editor, so the view owns the click and asks the delegate for the rect rather +than recomputing it. + +Until item 53 it also painted a row-wide strip of tag chips after the cells, +because a delegate cannot paint outside its column and the strip spanned all +five. That is why the class exists at all, and the history is worth keeping: +the arithmetic it needed produced a deleted row cut in half and every other row +showing a bare stripe, both because the view had to re-honour alternating +colours, the selection and `BackgroundRole` across cells it did not own. With +one column there is nothing to span, so the `paintEvent` and its band +arithmetic are deleted and none of that applies any more. + +**The panes' marks are shipped SVGs, not font glyphs and not a `.qrc`.** `Marks` +(`src/marks.h`) carries six payloads as compiled-in string literals, generated +from `assets/icons/marks/*.svg`, which stay the editable originals. Not a +resource, because `src/CMakeLists.txt` already records that a qrc in the static +library registers itself from a global initialiser the linker drops, and the +tests link the library rather than the executable. Every payload paints with +`fill="currentColor"`, which `QSvgRenderer` renders BLACK rather than resolving; +`Marks::pixmap` composites the real colour with `CompositionMode_SourceIn`, which +is what lets one asset serve a light and a dark palette. The toolbar and menus +still use `QIcon::fromTheme` and must keep doing so: the split between "panes are +ours, chrome is the system's" is item 70's whole point. A tag drawn as a mark +must not also appear as a chip, which `isDrawnAsAMark()` in `threadlistmodel.cpp` +enforces for both roles at once; the duplicate survived every geometry test and +was found only by rendering a card and looking at it. + +**A card layout must be testable without a painter.** `CardLayout` computes +every rect on a card and touches no `QPainter` and no widget, so the geometry +has tests that a blank render cannot defeat. When changing what a card shows, +change `CardLayout` and assert there; a test that renders the delegate and +counts pixels proves nothing, for the reasons under "Rendering probes lie". +Two traps it already handles: `QRect::right()` is inclusive, so the right edge +is carried as an exclusive one, and `QFont::pointSizeF()` returns -1 for a font +set in pixels, which qt6ct does. + +**`QTreeView`'s Up/Down already walk into an expanded thread's replies**, and +that is where message-to-message navigation comes from. Do not bind arrow keys +as `QAction` shortcuts to get it: a shortcut is dispatched before the focused +widget sees the key and Qt withholds only plain LETTERS from editable widgets, +so a bare `Up` would break the query bar, the tag dialog and the web view at +once. `Alt+Up`/`Alt+Down` are chords and therefore safe; `Shift+Up`/`Down` is +the built-in extend-selection and must be left alone. Binding two sequences to +one action needs `setShortcuts`, not `setShortcut`, which keeps only the last. + +**`Q_ENUM` is not enough to send an enum across a queued connection.** It gives +the type a meta-object entry, not a metatype registered under the name +`invokeMethod` resolves, so a `Q_ARG` carrying it is dropped at runtime with a +warning and the slot runs with a default. `NotmuchWorker::SortOrder` is +registered beside the type for this reason, not in `MainWindow`, so a caller +that never constructs one still gets it. + +**It is a `QTreeView` over a `QAbstractItemModel` since item 20**, because a +thread's replies are child rows and a table can neither indent nor expand. What +did NOT survive that port is anything keyed on a row NUMBER: a tree numbers rows +per parent, so `row 0` exists once per expanded thread and `current.row() + 1` +names a sibling rather than the next thread. Navigation walks with +`indexBelow`/`indexAbove`; `QTableView::isRowSelected(int)` has no equivalent — +use `selectionModel()->isSelected(index)`. Row height comes from +`setUniformRowHeights` plus `CardDelegate::sizeHint`, since a tree has no +vertical header to carry a default section size. Indentation is +`setIndentation(0)`: `CardLayout` draws the indent inside the card's own rect, +so `visualRect` reports the SAME left edge for a thread and its reply and a +geometry probe sees no nesting in a correctly nested list. + +**Three traps in the expander, all of which shipped a plausible-looking broken +build before being caught.** `QTreeView::drawBranches` is the documented hook and +does not work when the expander sits on a content column: it runs BEFORE the +row's cells, so the delegate's background paints over it (a 60-pixel triangle +survived as 8). `CardDelegate` draws it instead, as the reply count on the +card's second line. `setRootIsDecorated(false)`, needed to stop the style +drawing its own indicator underneath, also removes the style's HIT AREA, so the +glyph renders perfectly and is inert; `ThreadListView::mousePressEvent` handles +the click, asking `CardDelegate::expanderRectFor` for the target so the drawn +and clickable rects cannot drift. A fourth trap died with the grid: `isExpanded` +is keyed on column 0, which used to disagree with the subject-column index. + +**Visible, clickable and toggling are three separate properties.** A test for +one passes against the other two being broken, which happened twice in one +session: a pixel test proved the triangle was drawn while nothing could click +it, and a click test proved it opened while it could never close. + +**Assert a reply's indent on where the TEXT lands, never on `visualRect`.** The +reason has inverted twice and the rule has not. Under item 20 the geometry was +indented while the text was not, because the delegate laid text out from its own +left edge; now `setIndentation(0)` means `visualRect` reports no indent at all +while the text is indented, because `CardLayout` draws it inside the card's rect. +A probe on `visualRect` therefore endorsed a broken layout then and would fail a +correct one now. Assert on `CardLayout::contentLeft`. + +**`paintEvent` ran AFTER the cells**, which is why anything the view filled +across a row covered the text the delegate had just drawn: the reply tint filled +the full row height in one version and erased every sender and subject, measured +at zero surviving text pixels. Recorded because it is the class of bug a view +that paints invites. `ThreadListView` no longer paints at all. + +**No `notmuch_*` pointer ever crosses the thread boundary.** Data crosses as the plain +value structs in `src/types.h` (`ThreadSummary`, `MessageRef`, `MessageNode`, +`ActionScope`, `TagChange`), over queued +signals in both directions. `notmuchworker.cpp` is the only file that includes `notmuch.h` +outside `src/nmraii.h`; C handles are owned by the `NmQuery`/`NmMessages`/`NmThread`/… +RAII aliases there so they cannot leak. + +**The one exception, and it is a double-free if undone.** Messages reached +through `notmuch_thread_get_toplevel_messages` / `notmuch_message_get_replies` +are owned by the THREAD and freed with it (`notmuch.h:1637`), so `walkReplies` +in `notmuchworker.cpp` holds them as raw `notmuch_message_t*`: an `NmMessage` +wrapper would call `notmuch_message_destroy` on memory the thread frees again. +The whole walk must finish while the `NmThread` is alive. Related: replies are +unreachable from a query walk at all — `notmuch_message_get_replies` returns +NULL for a message from `notmuch_query_search_messages` (`notmuch.h:1617-1628`), +which is why `loadThreadTree` exists beside `loadThread` rather than replacing +it. + +**Generation counters, not cancellation.** Each query bumps a `quint64` generation passed +through to the worker and back on every result signal. The UI discards results whose +generation is stale. The worker never needs to know a query was superseded. Threads are +emitted in batches of `kBatchSize` (200) so a 10k-thread query paints immediately. + +**Read-only by default, read-write in bursts.** notmuch's write lock is exclusive +process-wide, so holding it open would block the user's cron `notmuch new`. `applyTags` +closes the read-only handle, opens read-write, applies, closes. notmuch permits only one +open handle per process, so that close-first ordering is required, not stylistic. + +**No dry-run, no destructive-action confirmation.** Those gates belonged to the retired +`mailctl` CLI, where they restrained an agent; a human at a GUI gets **undo** instead — every +mutation pushes its inverse (`TagChange::inverted()`) onto a `QUndoStack`. Do not add +confirmation dialogs for tag mutations. + +**An undo covers what the write CHANGED, never what it asked for, and getting +this wrong rewrote real mail.** Item 176. `ThreadTagCommand::undo()` inverted +the tags and kept the THREAD as its scope, so undoing "mark thread read" on a +conversation of 44 messages that held 2 unread added `unread` to all 44 and left +43 unread. `maildir.synchronize_flags` is on, so the Maildir filenames were +rewritten and the next sync would have carried it to the mail server; it was +repaired by hand. The undo stack is this application's substitute for a +confirmation dialog, so an undo that damages state is worse than the dialog it +replaces. + +`NotmuchWorker::applyTags()` is the only place that can know: it reads each +message's tags before writing and reports only the ids whose tags actually +moved. A `TagCommand` base carries that effective set for both +`ThreadTagCommand` and `MessageTagCommand`, which had the same shape and the +same defect on a multi-row selection. Two consequences that look like details +and are not: + +- **`tagsApplied` does NOT fire when the effective list is empty.** Emitting an + empty change would push an undo entry whose inverse adds a tag no message ever + carried, which is the same bug one step later. +- **`sendThreadTagChange` takes an `onlyMessageIds` parameter** so it keeps its + thread-scoped REPAINT while restricting the WRITE. Those two scopes differ on + purpose: the conversation card is what changed on screen, and a subset of its + messages is what changed on disk. Do not collapse them. + +A test for this needs a thread whose messages DISAGREE about the tag, for the +same reason item 87 records: two messages in the same state answer identically +whichever way the code resolves them. + +**There is exactly ONE exception, and its shape is the rule's own logic rather +than a hole in it.** `empty_trash` (item 118) destroys files and index entries, +so it has no inverse to push, and the protection the rule actually provides — +that a user never loses work to a keystroke — has to come from somewhere else. +It therefore asks, naming the count and the account, defaulting to Cancel, and +it carries **no default shortcut** for the same reason. `NotmuchWorker::purgeMessages()` +is a separate entry point from `moveMessages()` deliberately: the two look +alike and only one of them can be undone. A second confirmation anywhere is a +defect unless the action is likewise irreversible. All actions funnel through one `applyTags` path; +multi-row selections go through `applyTagsToThreads`, which resolves every thread in ONE +combined `thread:a or thread:b` query rather than one query per thread. + +**`notmuch_database_get_path()` is not the mail root, and assuming it is +moves mail somewhere mbsync cannot see.** notmuch can split the index from the +mail with `mail_root` and `path` as separate keys, which is how the Xapian +index goes on faster storage while the Maildir stays put. Under that layout +`get_path()` returns the INDEX directory. `mailRootOf()` in `notmuchworker.cpp` +wraps `notmuch_config_get(NOTMUCH_CONFIG_MAIL_ROOT)`, which is correct under +BOTH layouts, so there is no conditional and no reason to reach for the old +accessor again. Item 124, and the developer's own index has run split since +2026-08-20, so this is live rather than hypothetical. + +The consequences were asymmetric, which is why it is worth remembering: a wrong +root made message paths resolve to `../..` escapes that match no account, a +display defect, but `moveMessages` composes its destination from the same root, +so Delete would have written into the Xapian tree. Two related traps sit outside +the code. `database.hook_dir` defaults to `<database.path>/.notmuch/hooks`, so a +split config silently stops running `post-new` while `notmuch new` still reports +success; it must be set explicitly. And a test cannot see any of this in the +ordinary fixture layout, where the index lives inside the mail root and both +accessors return the same string: `NotmuchFixture::splitIndex()` exists for +that, and a test without it passes whichever accessor the code uses. + +**The Maildir path is deliberately not configurable.** notmuch stores it as +`database.path` and libnotmuch reads it; duplicating it would create two sources of truth. +The only escape hatch is `general/notmuch_config`, pointing at an alternate notmuch config. +Per-account subdirectories *are* configured, since notmuch does not model accounts at all. + +**Delete MOVES the file, and a wrong folder name reaches the mail server.** +Item 103. Every account carries a mandatory `trash` key and an optional +`inbox` one, both naming a folder relative to `maildir`. Naming a folder that +does not exist does not fail: the move CREATES it, mbsync adopts it and writes +state files for it, and under `Create Both` it then propagates to the server, +where every other client sees it. This is not theoretical. A folder name +containing a space was truncated by the origin tag, a bogus folder was created +beside the real one, and four messages of a thread were stranded in it on the +user's real mail. Treat any code that composes a folder name as reaching the +server, because it does. + +**A message records where it came from in a tag, because nothing else can.** +`deleted-from:<folder>` is written when Delete moves the file, and read back by +Restore. The file has moved, so neither the path nor anything in notmuch still +knows the original folder. A notmuch tag MAY contain a space, so tags crossing +the thread boundary are joined by a TAB rather than a space; joining on a space +truncated every folder name containing one. A message trashed by another client +carries no such tag at all, which is why the trash view is path-based and why +Restore falls back to the account's inbox rather than refusing. + +**Restore reads the DATABASE, never the model.** The model's tags come from the +query, so a row whose delete has not been re-queried still carries its pre-delete +tags: measured `[inbox,unread]` on a message already in the trash, one run in +three. The origin tag is then not found, the message falls into the no-origin +branch, and it goes to the inbox instead of where it came from, silently and +irreversibly. A restore must be right about its destination or it is worse than +doing nothing. + +**The sync script lives here, in `assets/mailsync.sh`.** It moved from the +retired `mailctl` project, which never called it: the script is `mbsync` plus +`notmuch new` with a lock, and qtmaildir is the only thing that runs it +programmatically. Two properties exist for this application's sake and +must survive any edit. It **prints to stdout as well as its log file**, because +`MailSync` shows what the command prints and a self-redirecting script leaves +the pane empty; and it **exits with the real status**, because a `0` from a +failed sync makes qtmaildir report success, clear the unsynced-changes count, +and quit on a sync that never happened. + +**Every user-facing string is translatable.** Wrap UI text in `tr()`, including strings +that are only ever shown in passing: status bar messages, tooltips, dialog prose, +completion descriptions. Query syntax itself is not user-facing text — notmuch keywords +like `tag:` and `date:` are wire format and must never be translated, only the prose +describing them. The tree was audited against this rule by item 22 on +2026-08-15, and an Italian translation ships, so a new string that misses +`tr()` is now a regression rather than pre-existing debt. + +**`tr()` alone does not make a string translatable, and the source cannot tell +you which.** A literal in an ARRAY or any other place with no enclosing class +needs the context named on the literal itself: +`QT_TRANSLATE_NOOP("TheClass", "Text")`. `QT_TR_NOOP` there compiles, reads +correctly, and extracts NOTHING — `lupdate` prints "tr() cannot be called +without context" and skips it, while the use site's `tr()` looks it up at +runtime under a context no `.ts` file contains. That shipped for the eight +rule-builder field labels in `tagrulesdialog.cpp` and made every one of them +permanently untranslatable in any language. + +`Q_DECLARE_TR_FUNCTIONS` is NOT the fix for that case, though it is the fix for +a free FUNCTION calling `tr()` (which is what `querycompleter.cpp` uses it +for). Measured: a class carrying the macro beside the array still extracts 0. +The context must be on the literal. + +**`lupdate` output is the evidence, never reading**, and `ctest -R +translations` encodes it: it fails on a string with no translation and on one +`lupdate` cannot see. Refresh with `lupdate-qt6 src/ -ts +translations/qtmaildir_it_IT.ts -no-obsolete -locations none` after changing +any user-facing string; a clean run reports zero context warnings, and +`lrelease` must report 0 unfinished, since it silently DROPS an unfinished +string and ships it as English inside an otherwise Italian UI. + +A `QTranslator` must live on `main`'s stack: one scoped to a helper function +unloads on return and every string reverts to English with nothing to see. + +**Translating a string that something MATCHES on breaks config in a language +the author never runs.** The built-in filters' names are labels and are +translated; `startup_query` resolved by comparing the config's text against +those names, so `startup_query = Inbox` matched nothing under `LANG=it_IT` +where the filter is called "In arrivo". The application opened the wrong view +AND warned that the user's own working config was invalid. It resolves on the +GENERATOR as well now, which is stored in queries.json and identical in every +locale. Before wrapping a string in `tr()`, ask whether anything compares +against it; if so, match on the wire-format identifier and treat the +translated name as an additional convenience, never as the identity. The +regression test installs a real `QTranslator` rather than a stub, because the +bug lives in the gap between the stored string and the displayed one and only +a real translation opens it. + +A second trap sits under that test and cost a wrong green: the warning it +asserts on is guarded by `!m_savedQueries.isEmpty()`, so a test with no +`queries.json` never reaches the branch and passes against a broken check. It +writes one, and asserts the file loaded before asserting on what it produced. + +**One config file has two readers, and both are now in this repo.** +`~/.config/mailrules/rules.json` is read and written by `src/tagrules.cpp` and +by `assets/hooks/mailrules.py`, which share no code and agree by test. +**Before changing anything about that file's format, read "Changing the rule +format" at the bottom of this document.** It used to be a cross-repo coupling +with the `mailctl` CLI; that project is retired and the hooks moved here on +2026-08-23, so a format change is now one repo and two suites. + +**The auto-tagging rules live in a config file, not in the source, and notmuch's +parser rejects almost nothing.** Rules are in `~/.config/mailrules/rules.json`, +applied by the notmuch `post-new` hook in `assets/hooks/`, which the live +`database.hook_dir` symlinks to; `TagRules` here reads and writes the same file +and `TagRulesDialog` edits it. +Two things bite. A stored query carries NO scope: the hook supplies `tag:new` +and wraps the query in parentheses, because `tag:new and a or b` binds as +`(tag:new and a) or b` and a rule that is a disjunction of senders would escape +its scope and match everything. And **a malformed query is not an error to +notmuch**: `from:((((` parses cleanly and matches nothing, so a test asserting +a failure or a `-1` count fails against correct code. This was recorded in +`test_notmuchworker.cpp` for thread counts and then learned again, twice, while +building the rules. Assert on the positional contract, never on a provoked +failure. + +**Qt emits no `triggered` for a `QAction` that owns a menu.** Setting a submenu +on an action makes clicking it open that submenu and nothing else, so any +`connect(action, &QAction::triggered, ...)` on the same action is dead code that +compiles, links and never runs. The saved-query overflow menu shipped this way: +every entry carried both a run connection and a submenu of edit actions, and no +entry in that menu had ever been runnable. It went unnoticed because the menu was +the rarely-used half while the user's queries were pinned buttons, and surfaced +only when item 93 moved every query into it. An action that must both run +something and offer actions needs the run as an item INSIDE its submenu. + +**A generator must be asked for one account's query, never handed its +all-accounts query to wrap.** `Config::resolvedQuery(query, accountKey)` exists +for this. Wrapping produces `path:"a/**" and (path:"a/Sent/**" or +path:"b/Sent/**")`, which returns exactly the right rows, because `path:` is +hierarchical and the other account's half cannot match inside `a`. That is why +it is dangerous: a row-count assertion passes against it, so the tests assert on +the generated STRING. The mutation putting the wrap back fails two of them. +Related: an EMPTY query means "match everything" to notmuch, so a generator with +nothing to match returns `Config::matchNothingQuery()` rather than an empty +string. An account that configures no sent folder would otherwise give a button +labelled Sent that shows the whole Maildir. + +**Every query this application builds goes through `SearchTerm` +(`src/searchterm.h`), and that is what stops five surfaces growing five quoting +rules.** It holds no widget, so the grammar is tested without a painter or a web +engine. Two of its rules are load-bearing rather than cosmetic. `quote()` +escapes backslashes BEFORE quotes, since the other order escapes the +backslashes it just added; it truncates before escaping, so a cut cannot land +mid-escape. And `extend()` parenthesises BOTH sides, because the query bar can +hold a hand-written disjunction and `a or b AND c` binds as `a or (b AND c)`, +which widens a search the user asked to narrow, reporting nothing. This is the +same trap the `post-new` hook handles when it scopes a rule with `tag:new`. + +**A writer that does not validate what its reader requires loses data +silently.** `TagRules::save()` wrote any id and `load()` required +`^[a-z0-9][a-z0-9-]*$`, so a rule named `justeat orders` in a field labelled +**Name** was written correctly, dropped on every read, invisible in the dialog, +still occupying the file, and never applied by the hook. The next save from the +dialog would have deleted it outright. `TagRules::validate()` is now the single +predicate both sides use; a bad id loads REPAIRED rather than dropped, so the +rule can be seen and fixed. Two lessons beyond the fix. The load warning already +existed and was correct and useless, because the rule it named could not be +reached, and a warning the user cannot act on teaches them to ignore warnings. +And the repair belongs in the editor, not in `mailrules.py`: the hook tags real +mail unattended, where a silent rename is worse than a drop. + +**Rule counts must count MESSAGES.** `requestCounts` counts threads, which is +right for the placeholder pane because a click there produces thread rows. A +rule tags messages, so a thread count understates every rule matching part of a +large thread; `requestMessageCounts` exists beside it for that reason. The two +are separate signals with separate generation counters, and a count request +must never bump `m_generation`: that is the *query* generation, and bumping it +discards any thread load in flight, blanking the message pane because the user +asked for counts. + +**Config format gotcha:** QSettings treats `/` in a section name as a group separator, so +account sections are `[account.work]`, not `[account/work]`. `childKeys` returns keys +sorted alphabetically, never in file order. **`[general]` keys are read WITHOUT the +`general/` prefix** — QSettings' INI backend treats a section literally named `[general]` +as its own fallback section and strips it, so a `general/<key>` lookup silently matches +nothing (this is how `notmuch_config` went unnoticed as broken). Config lives at +`~/.config/qtmaildir/qtmaildir.conf`. + +Machine-written UI state is a **separate** file, `~/.local/state/qtmaildir/uistate.conf` +via `MainWindow::uiStatePath()`. Never write window blobs into the hand-edited config. +Build the path from `QStandardPaths::GenericStateLocation`, not `StateLocation`: the +latter appends both the organization and the application name, and both are `qtmaildir`. + +**`QLineEdit::setCompleter` is wrong for any field holding more than one +value.** It hands completion to the line edit, which then overwrites the +completer's `completionPrefix` with the widget's **entire text** on every +keystroke. In a field holding a list, the first value completes and nothing +after it ever does, because "unread, fl" is matched whole against the +candidates. Setting the prefix from a `textEdited` handler does not help: the +line edit sets it again afterwards. Use `setCompleter` only for a field whose +whole contents are the thing being completed; otherwise attach with +`QCompleter::setWidget` and drive `setCompletionPrefix` and `complete()` +yourself, and replace the token under the cursor on `activated` rather than +letting QCompleter overwrite the field. This has been hit twice, in +`QueryCompleter` (01ba356) and in `TagDialog`; the trap belongs to Qt, not to +either class. A test that uses `setText()` passes against the bug, since +`setText` does not drive a completer at all: the keys must be typed. + +**No test may read the real `/proc/locks`, and restoring it after a test is a +BUG, not cleanup.** `TestMainWindow::init()` points every test at an empty lock +table in its own `QTemporaryDir`. Without that the suite observes the machine's +real sync state, so a `mailsync.sh` run makes `SyncMonitor` report a sync in +progress and tests that never mention syncing fail: measured 0 failures in 30 +runs with no lock held, 30 in 30 with one held, and it caused three separate +misdiagnoses (item 61). Reproduce with `flock /tmp/mbsync.lock -c 'sleep 60'` in +one shell and the suite in another. The three tests that observe a sync write +their own table content; none of them restores `"/proc/locks"` at the end any +more, because doing so handed the real table to the next test and re-exposed the +whole suite. `noTestCanSeeTheRealLockTable` fails if that protection is ever +lost. + +**`QItemSelectionModel::currentRowChanged` is emitted BEFORE the selection model is +updated.** A handler on it reading `selectedRows()` sees the *previous* selection, not the +one the user just made. Verified against Qt 6.11. This produced two separate faults in one +change (987a9e7): a Ctrl+click taking a selection from one row to two arrived reporting +one, and a click collapsing three rows to one arrived reporting three. Any decision that +depends on how many rows are selected belongs in a `selectionChanged` handler, which does +see the true count; `currentRowChanged` is only safe for "which row is current". + +The related trap: **`selectAll()` emits no `currentRowChanged` at all** and leaves the +current index invalid when nothing was current. A test that calls `selectAll()` on a fresh +view therefore passes against a missing selection guard, because no signal ever fires. Test +multi-select from a row that is already current, which is also how a user reaches it. + +**A `QDialog`'s buttons do not send a `QCloseEvent`.** `accept()` and `reject()` +go through `done(int)`, which hides the dialog without ever closing a window, so +a `closeEvent` override runs only for the window manager's X button. Anything a +dialog must persist on the way out belongs in a `done(int)` override, which both +buttons and `close()` reach. This shipped wrong in the rules dialog and the test +covering it passed, because the test used `close()` and the user used Cancel: +one route out of three. Assert every route. Underneath sits a second trap: +`close()` on a widget that was never shown returns early WITHOUT reaching +`done()`, so a test for the closed path has to `show()` the dialog first or it +asserts nothing at all. + +**A modal dialog must close BEFORE the action it asked for runs, not after.** +A signal from a dialog to its parent is a DIRECT connection, so the emit runs +the handler synchronously while `exec()` is still on the stack: the details +dialog's search ran the query, cleared the model and blanked the message pane +while the dialog was still up, holding the `m_items` it was built from. Call +`accept()` first, then emit. The mutation check for this HANGS rather than +failing, since without the `accept()` nothing ever leaves `exec()`, and a hung +test binary is item 84's second trap waiting to mislead the next run. + +**`Qt::RFC2822Date` validates the weekday against the date.** `Thu, 14 Aug +2026` parses as INVALID because that day is a Friday, and an invalid parse here +is indistinguishable from the trailing-comment trap `MimeParser::parseDate` +exists to handle. Two fixtures carried a wrong weekday, one of them +pre-existing and unnoticed until something finally parsed it. Write a date +fixture with `date -d <yyyy-mm-dd> +%A`, never from memory. + +**Under a tiling compositor a window's size is not the application's to +restore, and the user's desktop is Hyprland.** `saveGeometry` stores +`frameGeometry` and `normalGeometry`; `restoreGeometry` restores the NORMAL +one. When the compositor tiles the window to fill its slot, the size the user +drags is the tile's, and `normalGeometry` keeps whatever the code last passed to +`resize()`. Measured against the real state file after a hand test: frame +2248x806, normal 760x664, so the dialog correctly restored 760 and correctly +looked broken. A whole session went into "the geometry restore is broken" before +the blob was decoded. Decode the stored geometry before theorising, and expect +`maximized` to read as a value no bool should hold, which is the tiled state Qt +records and does not round-trip. + +The corollary for tests: **the offscreen platform cannot test window sizing at +all.** It prints "This plugin does not support propagateSizeHints()" and returns +an identical frame for a correct restore and a broken one, verified in a +standalone program containing none of this project's code. A size assertion +there passes against both, and a mutation putting the bug back leaves the suite +green. Assert on the stored value, and leave the frame to a hand test. + +**`ThreadListModel::threadAt(int)` takes a ROW and is wrong for any index that +might be a reply.** A tree numbers rows per parent, so a reply's `row()` indexes +its siblings and `threadAt(current.row())` on the first reply of any thread +returns the FIRST THREAD IN THE LIST. This shipped in `markCurrentThreadRead`, +was mostly masked while the write it guarded was thread-wide, and became "a +random message was marked read" the moment a fix scoped that write to one +message (items 87 and 88). + +**Use `threadFor(const QModelIndex &)`**, which resolves a message row through +its parent and a thread row through itself. Item 88 added it on 2026-08-16 and +converted every caller; `threadAt(int)` survives only for loops over +`rowCount()`, which genuinely hold a top-level number. A new caller that has an +index and reaches for `threadAt(index.row())` is reintroducing the bug. + +**The audit found four live sites, not the one that was reported**, which is the +part worth remembering: `delete` and `toggle_unread` each chose their DIRECTION +from the wrong thread, and the tag dialog counted the wrong thread's tags. All +three were reachable by clicking a reply, none had a test, and the reported +symptom named only `markCurrentThreadRead` (which was in fact protected by an +unrelated guard and could not fire). One bad accessor produced four defects with +one symptom between them. + +**A thread's first message is NOT among its children, and two lookups forgot +it.** `setThreadMessages` drops depth 0 because the root row stands for that +message, so `children` never holds it. `applyMessageTagChange` and +`messageById` both search the root first now (item 109); before that, a +message-scoped write to a root card repainted nothing, and the strip refresh set +the pane's chips to the empty node the lookup returned, destroying a strip that +had been correct. Item 108 made that the ordinary gesture rather than an edge +case: the two changes were each correct and broken together. + +**A defensive counter for an unreachable case is worse than nothing, because +it blocks the feature that needs the data.** `pendingEditCount()` summed a +fourth term, a bare int for confirmed changes carrying no message ids, added +on the sound reasoning that an edit which cannot be netted must not be lost. +It made the count impossible to LIST: three groups could name what they held +and the remainder could not, so item 119 sat open for a week carrying "a list +cannot be complete without changing how the count is kept". + +The remainder was empty. `NotmuchWorker::applyTags()` is the only emitter of +`tagsApplied()` and returns early on an empty id list, which is the exact +condition the counter required. Reading the code said it was reachable, and +that reading was wrong twice before it was measured: a `qFatal` in the branch +fired in 4 of 70 `test_mainwindow` cases, all four building a `TagChange` by +hand and invoking the slot directly, and a `Q_ASSERT` before the worker's own +emit never fired across the whole suite. **Instrument the branch and run the +suite; do not conclude reachability by reading.** The tests that exercised it +were the evidence it was live, and they were driving it from outside the +production path. + +**`PendingChangesDialog` groups by a RUN, not by a map**, and the snapshot it +consumes is sorted with `std::stable_sort` for that reason: the actions under +one message have to keep the order they were made in, and `QHash` has no order +of its own, so a map would reshuffle the list between openings. +`PendingChangeRow::startsMessage` is carried rather than inferred from a +non-empty subject, because an id the index no longer holds has an EMPTY +subject and must still open a run; inferring it folds a stale row's actions +under the message above it, which is a worse lie than "subject unknown". + +**A queued call carrying a container needs the same suspicion as a `Q_ENUM`.** +An unregistered metatype is dropped at runtime with a warning and the slot +runs with a default. Measured on Qt 6.11: `QList<bool>` and `QList<int>` both +cross `resolvePendingSubjects()` intact with no registration, but a standalone +probe showed `QMetaType::fromName("QList<int>")` invalid while +`QList<bool>` resolved, so the property is not obvious from the type. The test +that drives it across a real thread is what says it works, and what fails if +it stops. + +**`ThreadSummary::tags` is notmuch's UNION over the thread, and a conversation +row draws exactly that.** This REVERSES items 110 and 111, deliberately, under +item 177 on 2026-08-28, and the reversal is the point rather than a regression: +both of those solved the problem of a card standing for one message while +carrying its thread's tags, and item 177 removed the premise instead. A row +with replies IS the conversation, so the union is the honest answer and needs +no correction. A row with `totalCount == 1` has a union that IS its message, so +the union is honest there too. There is no case left where a card claims a tag +its subject does not have. + +What went with them: `setRootMessageTags()`, `ThreadSummary::firstMessageTags`, +the two-tier `PillTagsRole` / `PillOwnCountRole` split, `CardLayout::siblingFont()` +and `CardDelegate::mutedChipColour()`. The chips are one tier, in one font, in +one colour treatment. **The user was asked and confirmed they are happy to lose +the second tier**, so do not restore it on the reasoning that item 111 recorded: +that reasoning was sound for a card that stood for one message, and no card does +any more. Restoring it would reintroduce a per-message identity on a row that +has none, which is exactly the ambiguity item 177 exists to remove. + +The one thing to keep from that history: `applyMessageTagChange` keeps the +summary in step only when `totalCount <= 1`, where the union IS the message. +On a longer thread it deliberately leaves the summary alone, which is why the +membership rule below has a documented lag. + +**`TagStrip::visibleTags()` measures the LAYOUT, not the data.** It is one row +that collapses the overflow into a trailing "+N" chip, and an unshown window +under the offscreen platform has no width, so nearly everything lands in +`hiddenTags()`. A test asserting on `visibleTags()` alone passes or fails on how +many tags happened to fit; two shipped that way before it was noticed. Assert on +`visibleTags() + hiddenTags()`. + +**The message pane holds TWO `TagStrip`s since item 177**, one in the message +view and one in the dashboard, so an unqualified `findChild<TagStrip *>()` +returns whichever the object tree happens to yield first and a test written that +way asserts about the wrong widget. Both carry object names: +`messageTagStrip` and `dashboardTagStrip`. Find by name. + +**A message-scoped write repaints the MESSAGE's row, never the thread's.** +`ThreadListModel::applyMessageTagChange()` is the counterpart to +`applyTagChange()` and exists because there was no optimistic update at all for +a one-message edit: the correct observation that repainting a thread card for a +one-message change is a lie was turned into the wrong conclusion that nothing +should repaint, so Delete and Toggle unread on a reply moved the pending count +and changed nothing on screen (item 105). The thread card deliberately stays +put; one deleted reply does not doom the conversation. + +**A row is either a CONVERSATION or a MESSAGE, and one question decides which.** +Item 177, 2026-08-28. This REVERSES item 108, which had made a thread row mean +the one message its card displayed. That was a coherent answer to a real +ambiguity and it did not hold: a row that means a message needs a second set of +actions to reach its thread, and the two sets then disagree about what the +gesture the user just made was for. Item 177 answers it at the row instead. + +`ThreadListModel::isConversationRow()` is the single predicate. A summary with +replies is the conversation; a summary with `totalCount == 1` is its message and +behaves exactly as it always has, opening on one click; a reply row inside an +expanded thread is its own message. `scopeForSelection()` is the ONE resolver, +replacing the `scopeFor()` / `messageScopeFor()` pair, because a pair made the +CALLER choose and that choice was the ambiguity. It resolves per row, so a mixed +selection carries both kinds. + +**Do not restore `messageScopeFor()`, the five `*_thread` action names or the +"Whole thread" submenu.** They are removed, with an `### Upgrading` note in the +changelog. A second set of actions is a second answer to a settled question, and +`ThreadSummary::firstMessageId` no longer decides a thread row's scope at all +(it survives for other callers). A new action asks `isConversationRow()`; it +does not add a `_thread` twin. + +**Delete and Archive are ABSENT on a reply row, not disabled**, at the user's +own decision: removing one reply from a conversation is not offered. A thread of +one keeps them, since there the two acts are the same, so the test in +`refreshActionLabels()` is "is this a reply", which only a message row can be. +Forward and Save disappear on a conversation row, and Reply is one entry, +"Reply to this thread", which is reply-all and quotes nothing. + +**The unread toggle is a CATCH-ALL, and that reverses item 112's hiding rule.** +Item 112 hid the toggle whenever the selection disagreed with itself, on the +correct reasoning that a union is not a state. That was affordable only because +the `Whole thread` submenu carried absolute entries beside it; with the submenu +gone, hiding leaves no way to act at all. So: ANY unread message, a mixed +conversation included, reads "Mark thread as read" and marks every message read; +only a fully read selection reads "Mark thread as unread". Two presses reach +either state from anywhere. **The write direction must move with the label**, or +a mixed conversation gets marked unread under a label promising read. + +The automatic mark-read still follows item 87's rule for a message row +(`m_markReadMessageId`, scoped to the displayed message). It is NOT armed for a +conversation row, because a conversation puts no single message on display and +there is nothing to mark. + +**Membership is the union, with no exceptions.** A thread belongs to a view +while ANY of its messages match it, so reading one message of five does not take +the conversation out of Unread. `MainWindow::syncViewMembership()` is the guard +and is called from all three funnels (message, thread, move). Three properties +of it are decisions rather than implementation: + +- A row is **never evicted while it is current.** The automatic mark-read fires + two seconds after selection, so evicting would take the row out from under the + user mid-read. +- A write the user **asked for** evicts at once; an **automatic** one defers + until the selection moves (`m_deferredEvictions`). The deferral keys on the + write being automatic, so a test that calls the send path directly exercises + the opposite branch: drive it through the mark-read TIMER. +- The inverse case **refreshes rather than inserting.** The model holds no + summary for a thread the query never returned, so a row that starts matching + cannot be inserted optimistically. Without this an undone mark-read stayed + invisible in the view it was undone in. + +**One lag is deliberate and is not a wrong answer.** Reading the LAST unread +message of a long conversation does not evict it immediately, because +`applyMessageTagChange` leaves a long thread's summary alone (see the union rule +above), so the union the model holds is stale and judging on it would be wrong +in BOTH directions. `threadCountFor()` is the question a message-scoped write +asks before judging: a thread of one has a union that moved, a longer one does +not. The row leaves at the next query or sync. + +**Adding an action is FIVE places, and three of them are enforced by tests that +fail in confusing ways.** `KeyMap::knownActions()` (a `Q_ASSERT` in the +constructor fires otherwise, and it surfaces in whichever suite happens to build +a `MainWindow` first — `test_tagrules` did), `defaultBindings()` (OPTIONAL +since item 132: a shortcut is a chosen subset, not a requirement, so an action +nobody would press a chord for simply gets no entry and the shortcut reference +prints it as `(unbound)`), the icon table (every action must carry one), and +a MENU. The no-duplicate-icons rule is narrowed to actions that can reach the +toolbar, by a named exception list, and the test asserts none of the exempt +actions is on the toolbar so the exemption cannot be abused. The list held six +whole-thread actions until item 177 deleted them; it is down to +`reply_no_quote`, which is a menu entry that always carries its text. Note it is +named for the PROPERTY that earns the exemption, not for the tier that first +needed one, which is why it survived that tier's deletion unchanged. + +**The menu was the fifth place, and this document said four until item 103.** +Nothing enforced it, so `restore` shipped on the trash branch reachable by +`Ctrl+R` and by nothing a user could see or discover. The three existing +coverage tests each assert a different property and all three pass against an +action that appears nowhere in the interface. +`everyActionIsReachableFromAMenu()` closes it, walking every menu and submenu +from the menu bar; it found three more of the same the moment it was written +(`open_thread`, `clear_pane`, `clear_selection`). The toolbar is deliberately +NOT the test's instrument: it is a small chosen subset and always will be. An +action owning a submenu is not itself counted as reachable, since Qt emits no +`triggered` for it. + +**A toggle must read the state of what the row STANDS FOR, not of its thread.** +`MainWindow::everySelectedRowHasTag()` is the one question `delete` and +`toggle_unread` both ask; a reply row answers from its message, a thread row +from its thread. Reading the thread makes a toggle ONE-WAY on a reply, and the +failure is silent in a specific way worth knowing: the write is message-scoped, +so it never changes the thread's tags, so the answer never moves however many +times the key is pressed. The second press re-sends a tag the message already +has, which is a no-op, and a no-op repaints nothing. The user reports this as +"the key does nothing", not as "the key did the wrong thing" (item 105). + +This is the SECOND fix to the same three lines. Item 88 corrected which thread +they resolved; that was necessary and not sufficient, because a reply needs a +message read rather than a better thread. "Resolved through the index" and +"resolved to the right object" are separate properties, and a test for the first +passes against the second being wrong. + +**Any state a thread row draws, a reply row has to draw too, and this was +missed once already.** The message-row branch of `data()` is a separate switch +from the thread branch, so a cue added to one is simply absent from the other +with nothing to flag it. The doomed fill and the strike-out were thread-only +from item 13 until item 105, which is why updating the node was not enough on +its own to make Delete visible. When adding a visual state, check both branches. +One asymmetry is deliberate and must survive: a reply carries no tag strip. A +reply is now BOLD when unread, at the user's request on 2026-08-16, combining +with the dimming for the same two-cue reason a thread row has both; the smaller +reply font is what keeps it subordinate. A `deleted` reply deliberately shows +the chip as well as the fill and strike-out, matching the thread row, confirmed +with the user rather than treated as redundancy to remove. + +**And the reverse: a thread-scoped write must reach the thread's LOADED +replies.** `applyTagChange` updated the summary only, so marking an expanded +thread read left every reply bold and undimmed until the next query (item 107). +The symptom reads as a missed repaint and is not: the rows were redrawn from +data that had not changed. When a model update looks like it did not paint, +check whether the data behind those rows actually moved. + +**Every path a thread-scoped write travels, a message-scoped one travels too, +and each one was missed separately.** Three of them, found one hand-test round +apart: the optimistic model update (item 105), the message pane's tag strip +(also 105, keyed on `m_currentMessageId` and read by id through +`ThreadListModel::messageById()`, never from `currentIndex()`), and +`flushHeldEdits()`, which re-sent only thread edits and therefore DROPPED any +message edit made during a sync after showing it and counting it as pending +(item 106, data loss, never reported). When adding anything to +`sendThreadTagChange`, check whether `sendMessageTagChange` needs it. Escalating +a message edit to its thread is never the fix: it deletes a whole conversation +when the user deleted one reply. + +**Anything applied optimistically must also be reverted.** +`revertPendingTagChange()` keyed on `m_pendingThreadIds` alone, so adding the +message-scoped optimistic update would have left a FAILED message write showing +its optimistic state for good. Both scopes revert now. Undo needs nothing extra: +`MessageTagCommand` routes back through `sendMessageTagChange`. + +**Testing this needs two things that are easy to miss.** Put the reply under the +SECOND thread, so the wrong answer is plausible rather than accidentally right, +and give the two threads OPPOSITE states, since two threads in the same state +answer identically whichever way the code resolves them. That second point is +why the reverted item 87 fix was mutation-checked and green while corrupting +real mail. For a toggle, assert on `undoText()`: both directions push one +command over the same rows, so depth and ids cannot tell them apart. + +**A test for a mutation on a data-writing path must exercise the REPLY case, +not only the root.** The reverted fix above was mutation-checked and green: it +asserted on a root selection, which is the one case where `current.row()` is +correct. A green mutation check proves the test can fail, not that it covers the +case that matters. + +**`test_mainwindow` can now drive a real worker, and three things about it will +waste a session each.** `WorkerBackedWindow` builds a throwaway notmuch +database and writes a `qtmaildir.conf` pointing at it; `wireWorker()` reads +`notmuch_config` like any other key, so no production hook exists or is needed. +It is opt-in per test because the fifty-odd bare-window cases must not pay for a +`notmuch new`. The three traps, all found by a probe that reported success while +measuring nothing: + +- **The worker is unreachable by `findChild`.** It is created parentless and + moved to its own thread, so it is not in the window's hierarchy. Wait on + observable state with `QTRY_VERIFY_WITH_TIMEOUT`, never on worker signals and + never on a fixed `qWait(n)`, which passes when the result never arrives. +- **`rowCount()` on a thread row is 0 until the thread is expanded**, since + children are populated by the expansion. `hasChildren()` is the pre-expansion + question and falls back to `summary.totalCount > 1`. An assertion on + `rowCount` fails against correct code. +- **A `ThreadSummary` fixture needs `totalCount` set to what it MEANS**, since + item 177 reads exactly that to decide whether a row is a conversation or a + message. A summary left at the default is a message row, so a test meaning to + exercise a conversation quietly exercises the other branch and passes for the + wrong reason. `makeThread()` sets it; a hand-built summary must too. + `firstMessageId` still matters for a `totalCount == 1` row, which resolves to + its message: without one it names nothing and every action on it silently does + nothing. Ten tests failed that way at once under item 108, all reporting "the + action did not happen", which reads as a defect in the action rather than a + gap in the fixture. +- **`currentThreadId()` reports INTENT, not content.** It is assigned + synchronously in the selection handler before any worker round-trip, so a test + asserting on it passes with `onThreadLoaded()` disabled entirely, measured. + `MessageView::showingPlaceholder()` is what the user sees; assert the pane is + blank BEFORE the gesture so the check after it means something. +- **A `QStackedWidget` takes the LARGEST minimum width of all its pages**, so a + page that is not showing can inflate the pane's minimum and squeeze the widget + beside it. The dashboard raised the message pane's minimum to 395px over + `MainWindow`'s own 300px floor, and the thread list lost the difference. Fixed + with `setMinimumWidth(0)` and `QSizePolicy::Ignored` on the stack. An existing + resize test caught it, which is the argument for keeping resize tests that + look like they assert nothing interesting. + +**Five tests changed under item 177 and must not be restored as they were.** +Two are RETIRED because the behaviour they asserted is the behaviour that was +reversed: `selectingARootCardKeepsItsThreadForMarkRead` (replaced by +`selectingAConversationArmsNoMarkRead`, since a conversation arms no mark-read +at all) and `anUnexpandedRootRendersOneMessageNotTheConversation` (replaced by +`anUnexpandedRootShowsTheDashboardLikeAnExpandedOne`, which keeps item 66's real +value, consistency across the expansion boundary, and inverts only which way +that consistency runs). Three are RETARGETED to `totalCount == 1` rows, where +their assertions still hold and still matter: +`autoMarkReadTouchesOnlyTheMessageOnDisplay` (item 87's data-safety property, +still asserted), `theStaleNoticeKeepsTheMessageOfAThreadRootToo` and +`aLoadedMessageCorrectsTheStripFromTheThreadsUnion`. + +**A queued load can outlive the state that started it.** `loadThread` crosses to the worker +on a queued connection, so its reply lands after whatever the UI did in the meantime. The +generation counter covers a superseded *query*, not a superseded *selection*: blanking the +pane and then receiving an in-flight thread repaints it. `onThreadLoaded` therefore drops a +reply that arrives while more than one row is selected. This class of bug USED to be +unreproducible in `test_mainwindow`, which had no worker and never fired `threadLoaded`. +Item 36 changed that: `WorkerBackedWindow` (above) gives a test a real worker, and the +`onThreadLoaded` guard is covered by one. + +**Do not conclude a key binding is dead from `QTest::keyClick()`.** Whether a symbol needs +Shift is a layout property, not a Qt one. `Ctrl++` is the shipped `zoom_in` default and is +exactly what the `+` key emits on an Italian layout, while synthetic input never delivers +it. Verify against a real keyboard before changing a default on reachability grounds. The +separate, real trap `normalizeSequence()` handles is a **bare capital** (`N` parses to +unshifted Key_N, which no keystroke emits). + +**Rendering probes lie in specific, repeatable ways.** A whole session was spent chasing +a defect that did not exist because of these; each was believed until it was contradicted. + +- **Counting lit pixels cannot tell bold from regular.** Antialiasing lights a similar + number either way, so an "ink count" reads identical whichever is true, in both + directions. It measures nothing. **Text width** distinguishes weights (277px against + 306px for one string at 12pt), and a **strict pixel diff** distinguishes renders. +- **`viewport()->render()` returns a blank image** in several ordinary situations: before + the widget is exposed, when the content sits outside a viewport narrower than the + columns, and sometimes with no discernible cause. A probe that reports "no ink anywhere" + is far more likely broken than the code it is testing. Check that it finds the thing it + expects to find *before* trusting it to report the thing it expects to miss. +- **A row scrolled out of the viewport reports a `visualRect` with a real height**, so a + guard asserting `rect.height() > 0` passes while the pixel loop below it walks zero + rows and reports "0 pixels, the row was painted over". Item 93 hit this by adding four + buttons to the query row: the thread list shrank, and a test sizing its window to 300px + started failing with a message naming a defect that did not exist. Assert the rect is + INSIDE the viewport, not merely non-empty. +- **A probe can be correct and still measure nothing, by being pointed at the + wrong object.** A test for the sibling chip's padding called + `TagChip::sizeFor()` directly: that proves what the function does and nothing + about whether the delegate asks it for a scaled padding, so a mutation + dropping the scale at the call site stayed green. Assert through the function + the production path actually calls (`CardDelegate::chipSize()`), not through + the one it calls INTO. +- **A "saturated pixel" threshold catches antialiased edges of the selection highlight**, + hundreds of distinct near-background colours, and will pass whatever the code does. Match + the exact colours the model supplies instead. Two versions of one test passed under + mutation before this was noticed. +- Every rendering test needs a **mutation check** and a guard proving it *can* fail: assert + the geometry it depends on (a column is on screen, a row has non-zero height) rather than + assuming it. + +The bug that started all this was not in the code at all: the desktop's Qt font was +configured **Bold** in qt6ct, so every row rendered bold and `setBold(true)` changed +nothing. Before concluding a Qt facility is broken, check the desktop's own font and theme +configuration. + +**`QString::arg()` does not collapse `%%` into `%`.** `printf` does, and the habit +transfers silently. In generated CSS this is quietly destructive: every percentage written +`%%` to escape it reaches the browser malformed, and a browser does not report a bad +declaration, it **drops that one rule and renders the rest**. The 0.11.0 placeholder lost +its mask, its glow and both radial gradients this way while still painting a plausible +pane, so nothing looked broken. Write `%` directly; `arg()` only ever consumes `%1`..`%99`. + +The reason it survived review is worth more than the rule: **a geometry probe endorsed the +layout**, because it measured only properties that carried no percentage. A probe that +cannot see the thing that breaks will report success forever. When asserting on generated +CSS, assert on the **generated string** as well as on the rendered result, and make sure +the assertion covers the declarations that actually went missing. + +**So: build a test only when it asserts something measurable, and hand a visual +change to the user instead.** This is the rule the whole section above serves, +stated by the user on 2026-08-26, and it is the cheaper half of it: the traps +listed here are all ways a test over a visual property reports success forever, +and the reliable way to avoid them is not to write that test. + +Test the things with a right answer: the geometry (`CardLayout`), the generated +string, the tag write, the parse, the query `SearchTerm` builds. Do not test +"does it look right". When unsure which side a property falls on, ask whether a +mutation could make the test fail; if the answer is no, the test is not worth +building, whatever it appears to cover. + +**A green suite is not evidence a UI design is right, and the precedent cost a +whole feature.** Item 20's message rows were finished, tested and green when the +user rejected them on sight: "I don't think the table view fits our use." The +card list replaced them and shipped. So for any change the user will SEE, hand +the build over and let them look before treating it as done, whatever the suite +says. Say what to look for; do not launch the application yourself, per the rule +at the top of this file. Branch substantial UI work so master stays shippable, +since the user runs it daily while it is being built. + +Nothing here licenses dropping a test that CAN fail. The data-safety properties +are exactly the measurable kind, and items 87, 105 and 176 are each a case where +the missing test was the whole problem. + +## Web view security + +The most security-sensitive area: a browser engine pointed at input from strangers. Do not +loosen any of these without an explicit decision. + +- Off-the-record `QWebEngineProfile`, JavaScript disabled, `LocalContentCanAccessRemoteUrls` + and `LocalContentCanAccessFileUrls` both false. +- The interceptor **blocks every request by default** and **fails closed**: with no document + URL set, every `qtmaildir:` URL is denied. The document-load exemption matches the + **exact** base URL passed to `setHtml()`, never the `qtmaildir:` scheme as a whole — a + scheme-wide allow would let a hostile body reference `qtmaildir://anything` and be trusted. + Consequence: `MessageView` **must** call `setDocumentUrl()` with the same URL it gives + `setHtml()`, or nothing renders. +- The pane renders a LIST of messages into one HTML document in one web view (a + `QWebEngineView` per message would spawn a Chromium render process each). That makes `cid:` + ids collide across messages, so every reference is rewritten to `cid:<prefix>!<id>`. + **A `cidPrefix` must never contain `!`** — it is the namespace separator. + Since item 66 removed the conversation view every caller passes exactly ONE message, so the + collision cannot currently arise; the prefixing stays because the list-rendering path does, + and a security property must not rest on every caller happening to pass one item. +- Remote content grants are per-render and never sticky. +- **Attachment filenames are untrusted input.** Reduce to basename, strip separators, resolve + against the chosen directory, and refuse anything escaping it. Compare resolved paths as + paths, not with `startsWith` — `/tmp/safe-evil` passes a `startsWith("/tmp/safe")` check. + +## At the start of a session: reconcile the backlog with the user's notes + +The backlog at `docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md` is +**downstream** of the user's own notes at +`~/Documents/Obsidian/note/notes on qtmaildir.md`. The user writes to those notes +whenever they use the application and hit something, so the backlog goes stale on +its own between sessions. + +**The backlog holds the status table and the open sections only.** A closed +item's section lives in `2026-08-03-post-0.1.0-usability-closed.md` beside it, +moved there by item 73 on 2026-08-13, so grepping the backlog for a done item's +evidence finds the table row and nothing else. Both files use one numbering +sequence: item 42 is `## 42.` in whichever file holds it. When an item closes, +move its section across on the same commit rather than leaving it for a later +cleanup, which is exactly how the file reached five thousand lines the first +time. + +**Read both and diff them before picking up work.** Anything in the notes with no +item in the backlog gets appended with the next free number, in the backlog's +own format (Observed / Cause / Approach / Constraints), with the cause **verified +in the code, not copied from the note**. The two documents are numbered +independently and drifted long ago; never renumber to reconcile them. + +This is not busywork. The 2026-08-04 pass found nine unrecorded entries, two of +them defects rather than enhancements, and one of those was a constraint this +backlog had already specified and that shipped unbuilt (item 29). A note saying +"X does not work" is a bug report, and it will sit in a personal notes file +indefinitely unless someone goes looking. + +**The backlog covers the mail system, not only this binary.** An item can land +in `assets/hooks/` rather than in `src/`, and item 166 is one: the tagging hook +is part of the mail system the user sees, so a defect there gets an item here +like any other. Item 44 predates that and shipped as commits in this repo and in +the retired `mailctl`, which is why older entries mention a sibling repo; there +is no longer one to split work across. + +**Then print the open items as a table, and stop.** The user picks what to work +on; do not start on one, and do not recommend a single item as though the choice +were made. Read the status table for anything not marked `done`, `dropped` or +`postponed`, and render: + +| # | Item | Size | Note | +|---|------|------|------| + +- **Item** is a short description, not the table's own terse title. Say what the + user would notice, not the internal name. +- **Size** is the backlog's own: XS under an hour, S a sitting, M a session, + `?` for an item whose shape is not known yet. +- **Note** is the one thing that decides whether it can be picked up now: a + defect rather than an enhancement, a decision needed from the user first, a + dependency on another item, or a constraint that makes it bigger than it + looks. Leave it empty when there is nothing of the sort. + +Flag defects separately from enhancements. They read alike in a numbered list +and do not deserve equal billing: item 28 sat as "a counter is wrong" while the +indicator was quietly lying about whether the user's work was safe to quit on. + +Items marked `open, unspecified` (20, 21) cannot be planned from the backlog +alone; they need the user to describe what they pictured. Say so in the Note +rather than proposing a design. + +Two gotchas when reading the status table. Item 12 lives in the **"Deferred, +unsized, or split out"** table further down, which has different columns and +carries no size, so a grep across `^| <n> |` picks it up with its description +where the size should be. And an item's status cell is prose, not a keyword: +`open, on demand` (36) and `open, unspecified` (20) are both open. Read the +cell, do not match on `open` alone. + +**Then write the same table to the user's vault**, at +`~/Documents/Obsidian/note/qtmaildir - backlog.md`. It exists so the user can +follow the backlog from their own notes rather than by reading a five-thousand +line file in the repo, and it is a SNAPSHOT: the repo's status table stays the +source of truth, and this note is regenerated from it. So rewrite it whole on +every reconcile rather than editing rows in place, which is what keeps a closed +item from lingering there after it has left the backlog. + +Three things about it are conventions of the vault, not of this repo, and a +file written without them reads as foreign there: no frontmatter, links to the +sibling notes as wikilinks (`[[notes on qtmaildir]]`, `[[qtmaildir - Done]]`, +`[[qtmaildir shortcuts and menu structure]]`), and the same defect/enhancement +split the terminal table uses. Keep the header line stating the date and saying +the numbers are the backlog's own, since they do not match the numbering in the +user's notes and never will. + +Nothing about the personal-data rule is relaxed by writing outside the repo: +the note is in a vault the user syncs, so it carries item numbers and +descriptions, never mail content. + +## Working on this repo + +Implementation follows `docs/superpowers/plans/2026-08-02-qtmaildir-v1.md` (14 tasks) +against `docs/superpowers/specs/2026-08-02-qtmaildir-design.md`. + +**Treat every code block in the plan document as a draft, not as correct.** Nine defects +have already been found in code that was written confidently into it, two of them +security-relevant. Verify Qt API assumptions empirically rather than from memory — several +of those defects were wrong assumptions (e.g. `QKeySequence::fromString` never returns an +empty sequence for garbage input). + +TDD, per the user's global preference. `NotmuchWorker` is unit-tested against a throwaway +notmuch database built in a temporary directory (generated Maildir + `notmuch new` + +`NOTMUCH_CONFIG` scoped to the test process), superseding the spec's original "no unit +test" position — it is the only code that writes to a notmuch index. + +Work goes directly on `master`, no PR flow. Commits must be GPG-signed (`git commit -S`). +`HANDOFF.md` is local-only and gitignored; never stage or commit it. + +**There is no "v1" and no "v2".** The project follows semver on its +user-visible surface, and those labels described a scope split that stopped +being true when compose and send shipped. Reading, organizing and sending are +all part of the application now. The phrase survives in the older spec and +plan documents, which are historical records and are not being rewritten; read +it there as "before compose" and "after compose". + +## Cutting a release + +Five steps, and **the GitHub Release is part of cutting a release, not a +follow-up.** The repo had fourteen tags and zero releases until 2026-08-09, +because the first four steps were treated as the whole job. + +1. Move the `[Unreleased]` entries under a new `## [X.Y.Z] - YYYY-MM-DD` + heading, leaving `[Unreleased]` in place and empty above it. Add a one or + two sentence summary under the heading, and an `### Upgrading` section when + a user's own config or habits need to change. +2. Bump `project(qtmaildir VERSION ...)` in `CMakeLists.txt`, the only place + the version lives. Reconfigure, build, and check `./build/src/qtmaildir + --version`. An ordinary dev build answers `X.Y.Z build N`, because + `QTMAILDIR_BUILD_NUMBER` counts rebuilds so one binary of an unreleased + version can be told from another (item 167). That is the DISPLAY version; + what a release ships is the clean one, from + `-DQTMAILDIR_BUILD_NUMBER=OFF`, which is what the SlackBuild configures and + what a tarball with no build directory produces anyway. Check the clean + form before tagging. +3. Commit as `release: X.Y.Z`, then `git tag -s vX.Y.Z -m "qtmaildir X.Y.Z"`. + Tags are annotated and GPG-signed, matching every existing one. +4. `git push && git push --tags`. `origin` carries two push URLs, the personal + server and GitHub, so one push reaches both. +5. Create the GitHub Release from the tag, with the body taken from that + version's changelog section rather than written fresh: + + ```bash + gh release create vX.Y.Z --repo danixland/qtmaildir \ + --title "qtmaildir X.Y.Z" --notes-file <section>.md --verify-tag + ``` + + Normal release, never `--prerelease`: GitHub's "latest" badge ignores + prereleases, and every 0.x here is a real release the user runs daily. The + pre-1.0 stability caveat is already stated at the top of the changelog. + +**Version choice is semver on the user-visible surface**, pre-1.0 included. A +changed label, a changed default, or anything in an `### Upgrading` section is a +minor bump, not a patch: 0.12.0 renamed an action and changed the toolbar's +button style, and both are things a user notices without reading the changelog. + +**The SlackBuild is no longer in this repository.** It moved on 2026-08-15 to +the `my-slackbuilds` repo, under `qtmaildir/`, SBo compliant and building from +the GitHub release tarball. It is **not** part of this procedure and never +was: bumping it is a task in that repo, which has its own workflow in its +`AGENTS.md` (edit the version in the `.SlackBuild` and the `.info`, two-pass +`sbodl` for the checksum, `sbolint`, then wait to be told to commit). It also +tracks upstream through an nvchecker stanza there, so a release here is picked +up by that repo's own sweep. + +## Changing the rule format + +`~/.config/mailrules/rules.json` has **two independent implementations**, both +in this repo, and they agree by test rather than by sharing code: + +| | reads/writes | applies rules | +|---|---|---| +| `src/tagrules.cpp` | yes | no | +| `assets/hooks/mailrules.py` | yes | via the `post-new` hook | + +They are two languages either side of one file, so nothing but the format +couples them. Both readers preserve fields they do not understand +(`TagRule::unknown`, `Rule.unknown`), which is what lets one save a file the +other wrote without stripping it. This was a cross-repo coupling with the +`mailctl` CLI until that project was retired and the hooks moved here on +2026-08-23; the discipline below survives the move because the two readers do. + +**The live hook runs every ten minutes on real mail.** Before touching the +schema: + +1. Change both readers, not one. A field added on one side and not the other is + silently dropped on the next save from the other, which looks like data loss + with no error anywhere. +2. Bump `kFormatVersion` / `FORMAT_VERSION` together only for a BREAKING change. + Both readers refuse a file whose version they do not know, which is the + correct behaviour and also means a half-applied bump stops the hook from + tagging. Adding an optional field needs no bump. +3. Run both suites: `ctest --test-dir build -R tagrules`, and + `./test_post_new.py && ./test_mailrules.py` from `assets/hooks/`. +4. Verify the round trip by hand, since no automated test spans the C++ and the + Python: save from the dialog, then run the hook over a throwaway index, and + confirm the rule count and a note survive. + +**Two hook properties are safety-critical, and being ours now is not a reason to +weaken them.** +The hook refuses to remove `unread` or `inbox` (`maildir.synchronize_flags` is +true, so removing `unread` rewrites Maildir filenames and reaches the server), +and it does not consume the `tag:new` marker when the rules fail to load +(clearing it while rules did not run orphans that mail permanently and +invisibly). A dialog here that offers to write such a rule would produce one the +hook then refuses; that is the correct direction, but say so in the UI rather +than letting it fail silently. + +Backfill, applying a rule to existing mail, is deliberately unbuilt. See +`docs/superpowers/specs/2026-08-12-tagging-rules-design.md` for what it needs +first, including the revision it forces to the no-confirmation rule above. |