diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-29 09:49:07 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-29 09:49:07 +0200 |
| commit | d128919ad80ecc37b1f58d2ba83a7487362c126f (patch) | |
| tree | 2617a84894f366bdd7c4b866974a7cfb5ab93d4c | |
| parent | d7c4d03f7d583767bc23406579e11bcc884dec18 (diff) | |
| download | qtmaildir-d128919ad80ecc37b1f58d2ba83a7487362c126f.tar.gz qtmaildir-d128919ad80ecc37b1f58d2ba83a7487362c126f.zip | |
docs: move CLAUDE.md to AGENTS.md, and reconcile the backlog
AGENTS.md is the single source of truth for how to work on this
repository, shared across every agent tool; CLAUDE.md is now a thin
pointer to it, mirroring the arrangement in the user's own config.
Prose throughout the specs, plans, source comments and tests refers to
this document as CLAUDE.md, which is what it was called until now.
Those references are historical records and are correct about the
content, so they are left alone and AGENTS.md says how to read them.
Two rules added while reconciling:
- The reconcile writes its open-items table to the user's vault as well,
rewritten whole each time so a closed item cannot linger there.
- Build a test only when it asserts something measurable, and hand a
visual change to the user instead. This is the conclusion the
"Rendering probes lie" section was already building toward, and it
sits at the end of it. Guarded against the opposite reading: it does
not license dropping a test that can fail, and items 87, 105 and 176
are each a case where the missing test was the whole problem.
The 2026-08-29 pass over the user's notes found two entries with no item
here, both causes verified in the code rather than copied from the note:
- 179, undo is one level deep in practice and there is no Redo. The
QUndoStack is real and multi-level; what is missing is a redo action
and an answer to the stack being cleared on every new query.
- 180, the repaint rules are discovered one hole at a time. A question
rather than a defect, and the user's own framing: items 105, 107, 109,
110 and 170 are each one hole in the same surface, all found by hand.
| -rw-r--r-- | AGENTS.md | 1244 | ||||
| -rw-r--r-- | CLAUDE.md | 1194 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 101 |
3 files changed, 1350 insertions, 1189 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. @@ -1,1192 +1,8 @@ # CLAUDE.md -This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. +This file is intentionally thin. AGENTS.md is the single source of truth for +how to work on this repository, shared across every agent tool (Claude Code, +opencode, etc). Do not re-populate this file with duplicated content, edit +AGENTS.md instead. -## 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. - -## 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. - -## 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. +@AGENTS.md 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 5b84b15..bf347c8 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 @@ -252,6 +252,8 @@ taking that too literally. | 178 | Delete and Restore judge a conversation on one message | defect | XS | open, 2026-08-28, split out of item 168 by item 177 and flagged in that spec's "Open, deliberately". `MainWindow::everySelectedRowIsInATrashFolder()` reads `ThreadSummary::firstMessagePath` for any row that is not a message row, which was correct while a thread row MEANT that message and is not correct now that it means the conversation. A conversation is in the trash when ALL of its messages are, so a partly-trashed thread currently answers on whichever message the query returned first: Delete can be hidden on a conversation that still has mail outside the trash, and Restore offered on one that mostly does not. Not data-affecting, both actions are no-ops in the wrong direction, but it is an inconsistency the row-kind rule was supposed to remove. Needs the summary to carry the answer, or the paths of every message, which the digest walk already reads | | 174 | An external `notmuch new` reaches the index without the pending count noticing | defect | S | open, 2026-08-28, from the notes. Item 54 cleared the count for a sync run by `mailsync.sh`, which is what `SyncMonitor` watches; a bare `notmuch new` (a hand run, or a cron entry that is not the script) takes notmuch's own write lock and touches `/tmp/mbsync.lock` not at all, so nothing observes it. The user's framing is the approach: we own `mailsync.sh` and the whole process | | 175 | The send countdown says Undo, and cannot be skipped | presentation | XS | open, 2026-08-28, from the notes. Two changes in one control: the button reads Abort, and a second button sends immediately rather than waiting the countdown out | +| 179 | Undo is one level deep in practice, and there is no Redo | workflow | ? | open, 2026-08-29, from the notes. The `QUndoStack` is real and multi-level; what is missing is a `redo` action (absent from `knownActions()`, never called) and an answer to the stack being CLEARED on every new query (`mainwindow.cpp:3458`), which is what makes a deep stack behave like a shallow one. The clear has a correct reason and cannot simply be removed. Redo re-applies a write to real mail, so item 176's rule binds it too | +| 180 | The repaint rules are discovered one hole at a time | maintenance | S-L | open, 2026-08-29, from the notes, and a QUESTION rather than a defect. Items 105, 107, 109, 110 and 170 are each one hole in the same surface, all found by hand. Three mechanisms (optimistic repaint, `syncViewMembership()`, revert) agree by documentation rather than by code. Cheapest answer is one invariant test, not a rewrite; the user decides which, and that decides the size | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1280,3 +1282,102 @@ call the button would make, so skipping is stopping the timer and calling it. A skip button must not become a default, or the protection is gone for everyone who learns to press it. - Which button is the default on Return matters here and is the user's call. + +## 179. Undo is one level deep and has no Redo + +**Observed (user, from the notes):** "ctrl+Z should work like in any other +application, keep track of the past N actions and allow to undo them. This +would also bring in Redo, which would act as the opposite of Undo. As it is +today it feels hackish and half-implemented." + +**Cause, verified in the code.** The stack is real, not a single slot: +`MainWindow` owns a `QUndoStack` (`mainwindow.h:1434`) and every mutation +pushes a command onto it, so multi-level undo is already there and works. +What is missing is the other half of the facility around it: + +- **There is no `redo` action at all.** `KeyMap::knownActions()` lists `undo` + and nothing else (`keymap.cpp:69`), `Ctrl+Z` is its only binding + (`keymap.cpp:201`), and `QUndoStack::redo()` is never called from + `mainwindow.cpp`. The `redo()` overrides in the command classes exist only + because `QUndoStack` calls them on push; nothing reaches them a second time. +- **The stack is cleared on every new query** (`mainwindow.cpp:3458`), with a + correct reason recorded beside it: the entries invert model updates against + rows that are about to be discarded. That is what makes a stack that is N + deep behave like one that is one deep, since running a query is the ordinary + thing a user does between actions. +- No `setUndoLimit()` is set anywhere, so the depth is unbounded until a query + clears it. + +**Approach.** Two separable pieces, and the second is the real work. + +1. Add `redo` as an action: `KeyMap::knownActions()`, an icon, a menu entry, + and `Ctrl+Shift+Z` as the conventional default. Per CLAUDE.md that is five + places, four of them enforced by tests. Small and self-contained. +2. Decide what survives a query. The clear is not gratuitous, and removing it + without an answer reintroduces the half-applied undo it prevents: the + database changes and the list does not. The candidate answer is that a + command should be able to re-resolve its rows against the current model + rather than assuming the ones it was built with are still there, at which + point a query no longer invalidates the stack. + +**Constraints.** +- The undo stack is this application's substitute for a confirmation dialog, + per CLAUDE.md, so an undo that damages state is worse than the dialog it + replaces. Item 176 is exactly that failure and is one commit old. +- Redo re-applies a write to real mail. It carries the same requirement item + 176 established for undo: it must cover what the write actually CHANGED, not + what it asked for. +- `m_awaitingTagConfirmation` holds pointers the stack owns and is cleared + beside it (`mainwindow.cpp:3462`). Anything that changes the clear has to + keep those two in step or it is a dangling pointer, not a stale row. +- Sizing the second piece needs the first: `?` until the query-clear question + is answered. + +## 180. The repaint rules are discovered one hole at a time + +**Observed (user, from the notes):** "should we refactor the list UI to be +responsive so changes are applied immediately instead of waiting for a view +change to repaint? I feel like we are chasing our own tail with the repaint +issue, we are effectively plugging holes, every time we have to stumble on a +view that doesn't refresh when it should, or worse, it refreshes when it +shouldn't." + +**Cause, verified in the code.** This is a maintenance item about a pattern, +not a defect with a reproduction. The observation is accurate and the history +is in this backlog: items 105, 107, 109, 110 and 170 are each one hole in the +same surface, every one found by hand-testing rather than by a test, and each +fixed in place. The current shape is three separate mechanisms that must agree +by hand: + +- the optimistic REPAINT, `applyTagChange` for a thread and + `applyMessageTagChange` for a message, which item 105 established must exist + in both scopes and item 107 established must reach loaded replies; +- the optimistic MEMBERSHIP, `MainWindow::syncViewMembership()`, added by item + 170 and called from three funnels (`mainwindow.cpp:5861`, `:7052`, and the + deferred path at `:3899`); +- the revert, `revertPendingTagChange()`, which must cover every scope the + other two apply. + +CLAUDE.md already carries the rule that keeps them in step ("Every path a +thread-scoped write travels, a message-scoped one travels too"), which is a +statement that the coupling is enforced by documentation and review rather +than by the code. + +**Approach.** Not a rewrite. The cheapest thing that would end the pattern is +a single test that asserts the invariant directly, rather than one test per +hole: for each scope and each funnel, a write leaves the model, the membership +and the revert path agreeing. That converts "we stumble on a view that does +not refresh" from a hand-test finding into a suite failure. + +**Constraints.** +- The deliberate lag documented in CLAUDE.md is NOT a hole and must not be + "fixed" by such a test: reading the last unread message of a long + conversation leaves the row in place because `applyMessageTagChange` leaves + a long thread's summary alone, and judging on a stale union would be wrong + in both directions. The invariant has to be written to permit it. +- Likewise the two deliberate asymmetries: a row is never evicted while it is + current, and an automatic write defers eviction where a requested one does + not. +- This needs the user to say whether they want the test or the refactor. The + note asks a question rather than reporting a fault, and the answer changes + the size from S to L. |
