diff options
Diffstat (limited to 'docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 4534 |
1 files changed, 4534 insertions, 0 deletions
diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md new file mode 100644 index 0000000..4ea175a --- /dev/null +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md @@ -0,0 +1,4534 @@ +# Post-0.1.0 usability backlog: closed items + +Companion to `2026-08-03-post-0.1.0-usability.md`, which stays the backlog. +This file is the history: the full Observed / Cause / Approach section of every +item that is done, dropped or postponed, moved here by item 73 so the backlog +reads as what is still open. + +**Nothing here was deleted and nothing was renumbered.** The status table in the +backlog is unchanged and remains the index: it carries every item's number, size +and outcome, including the ones whose sections live here. Section numbering is +this document's own in the same sequence, so item 42 is `## 42.` in whichever +file holds it. + +Sections appear in their original order. A trap recorded here that is still true +of the code belongs in `CLAUDE.md`, where it will actually be read; several +already are, and this file is not a substitute for that. + +## 1. Splitter and column widths do not survive restart + +**Observed:** resizing the thread list pane, or a column inside it, is undone by +the next launch. + +**Cause:** `MainWindow::buildUi()` (`src/mainwindow.cpp:195`) builds the +`QSplitter` fresh every launch, sets a stretch factor, and never saves state. +`resize(1200, 800)` at `src/mainwindow.cpp:206` hardcodes window size too. There +is no `QSettings` window-state read or write anywhere in the class. + +**Approach:** one `saveState`/`restoreState` pair in +`MainWindow`, driven from a `QSettings` object separate from the hand-written +config file. + +- Save on `closeEvent`, restore at the end of `buildUi()`. +- Persist: `QMainWindow::saveGeometry()`, `QMainWindow::saveState()`, + `QSplitter::saveState()`, `QHeaderView::saveState()` for the thread list. +- Restore must be a no-op when the stored blob is absent or rejected, falling + back to the current hardcoded defaults. `restoreGeometry()` returns `false` + in that case; do not assume it succeeded. + +**Where the state file goes.** The hand-edited config lives at +`~/.config/qtmaildir/qtmaildir.conf` and is the user's to own. Machine-written +window blobs must not land in it: a base64 `QByteArray` appearing in a file the +user edits by hand is hostile, and rewriting that file on exit risks clobbering +comments and formatting QSettings does not preserve. Use a **separate** +`QSettings` instance for UI state, at +`~/.local/state/qtmaildir/uistate.conf` or the `QStandardPaths` equivalent, and +keep `Config` untouched. + +**Confirmed by the user, 2026-08-03.** This decision covers items 1, 4, and 10 +as well. Establish it once, in whichever lands first. + +**Verification:** manual. Resize both the window and the splitter, restart, +confirm both held. Then delete the state file and confirm the app still starts +with the 1200x800 default rather than a zero-size window. + +### Outcome (done) + +Built as described. `MainWindow::uiStatePath()` establishes the state file the +plan calls for, so items 4 and 10 inherit it. Two things worth recording: + +- **`QStandardPaths::StateLocation` is the wrong enum here.** It appends both + the organization and the application name, and this app sets both to + `qtmaildir`, so it yields `~/.local/state/qtmaildir/qtmaildir/`. The path is + built from `GenericStateLocation` plus an explicit `/qtmaildir`, the same + shape as `Config::defaultPath()`. A test pins the component count. +- **`restoreUiState()` runs after `buildMenus()`, not at the end of + `buildUi()`** as the plan proposed. `QMainWindow::restoreState()` matches + toolbars by object name, so a toolbar that does not exist yet has its + position silently dropped. + +Every restore is guarded on a non-empty blob, so absent state leaves the +`buildUi()` defaults rather than producing a zero-size window. + +## 2. No way to see full message details + +**Observed:** From, To, Cc, Subject and the rest are not visible for the +selected message. + +**Cause:** partially true rather than wholly. `MessageView::updateHeader()` +(`src/messageview.cpp:219`) shows only the thread subject and a message count. +Per-message From and Date *are* rendered inside the HTML body as `.msg-header` +(`src/htmlbuilder.cpp:241`), styled at 9pt grey, which is easy to miss and does +not include To or Cc at all. + +**Check before building:** does `MimeParser` already extract To and Cc into the +message struct, or does `src/types.h` / `MimeParser`'s output need extending +first? If the fields are not parsed, that is the real first task and it is +larger than the UI work. + +**Answered, 2026-08-04: they are already parsed.** `MimeParser::parse()` fills +both (`src/mimeparser.cpp:344-345`, into `ParsedMessage::to` and `::cc`, +declared at `src/mimeparser.h:105`). The larger task the check warned about +does not exist. + +They are parsed and then **dropped at the renderer**: `HtmlBuilder` interpolates +only `from`, `subject` and `date` (`src/htmlbuilder.cpp:216`, `:244-245`), and +`to`/`cc` appear nowhere in it, in `messageview.cpp`, or in `mainwindow.cpp`. +So this is UI work only, as the item's own two-part approach assumes. + +**Approach, two parts:** + +- Widen the persistent header at the top of the message pane to show the + selected message's From, To, Cc, Date and Subject. This is the note's stated + preference ("should appear on top in right pane"). +- Add a shortcut and menu entry for a full raw-header dialog, for the cases the + summary omits (Message-Id, List-Id, Received chain). Read-only, selectable + text, no rendering. + +Both, not one: the header widget answers "who is this from" at a glance, the +dialog answers "what actually happened to this message". They are different +questions. + +### Decided (user, 2026-08-04): the header adapts to the item count + +The pane shows a thread, not a message, so From/To/Cc are per-message while the +header is one strip. Rather than pick a message arbitrarily, the header shows +only what it can say honestly: + +- **One message in the thread:** From, To, Cc, Subject. The natural spot, and + every field is unambiguous. +- **N messages:** Subject and the thread count. Nothing more. +- **Everything else** lives in the popup, reached by a **button on the right of + the header** plus a keyboard shortcut. + +**No recipient line on a thread (user, 2026-08-04).** An earlier draft of this +decision put To on the thread header too, which forced a choice between the +union of recipients and their intersection: once the user has replied, message +1 is To: them and message 2 is To: the other party, so the intersection is +frequently empty and the union is really a participants list wearing the wrong +label. The user's call was that this is overcomplicating, and it is: the +per-message detail is what the popup is for. + +The thread header therefore keeps showing exactly what it shows today, subject +and count (`MessageView::updateHeader()`, `src/messageview.cpp:259`), and only +the single-message case gains fields. + +**Consequence: no address parsing is needed.** `ParsedMessage::to` and `::cc` +are raw header strings (`src/mimeparser.h:104-105`), and with no union or dedup +to compute they can be displayed as they stand. Splitting them into address +lists, which would have needed GMime's `internet_address_list_parse` to survive +a display name containing a comma, is not part of this item. + +**Deferred, not rejected (user, 2026-08-04): a participants line for threads.** +The union-of-recipients idea is worth revisiting as its own pass, where it can +be designed as a participants list rather than smuggled in under a "To:" label +that misdescribes it. It needs the address parsing above, so it is a genuine +piece of work rather than a display tweak. Build this item as specced first. + +**Noted for later, not now:** the user's mental model of the thread view +differs from what was built. That is a separate refactor and should not be +folded into this item. + +**Constraint:** header values are untrusted input. The existing header label is +`Qt::RichText` (`src/messageview.cpp:104`), so every value must be +`toHtmlEscaped()` before interpolation, exactly as `updateHeader()` already +does. A `From` display name containing markup must never be able to inject into +the label. The raw-header dialog should use `Qt::PlainText` and sidestep the +question entirely. + +### Outcome (done) + +Built as decided. `MessageView::updateHeader()` branches on the item count: one +message shows From, To and Cc under the subject, several show the subject and +the count exactly as before. `showDetailsDialog()` lists every message's +Subject, From, To, Cc, Date and Message-Id, numbered when there is more than +one, in a read-only `QPlainTextEdit`. A `Details...` button sits to the right of +the header, and `message_details` binds it to `Ctrl+Shift+D` (shifted because +`Ctrl+D` is delete, and the destructive binding keeps the key it had). + +- **Rendered and inspected**, not only asserted: both header shapes were grabbed + to PNG and looked at. The single-message case shows three rows under the + subject, the thread case shows the count and no recipients. +- **An empty Cc omits its row** rather than printing a label with nothing after + it, which reads as a rendering fault. +- **A test caught a latent flaw in an older test.** `attachmentButtonLabels()` + identified attachment buttons by excluding the one other button's label, so + the new details button was counted as an attachment the moment it existed. + It now finds the bar by object name and looks only at its children, which is + what it should have done: an exclusion list silently adopts every button + added later. + +**No address parsing was needed**, as the decision above anticipated. The header +prints `ParsedMessage::to` and `::cc` as they stand. + +## 3, 8, 9. Discoverability: menu bar, toolbar, shortcut reference + +Grouped because they are one piece of work. Item 3 is the complaint, items 8 +and 9 are two of its symptoms. + +**Observed:** for a GUI app there is almost nothing to click; every action needs +a memorized key. Archive and undo have no buttons. There is no way to see the +configured bindings without opening a terminal. + +**Cause:** `MainWindow` has no `menuBar()` and no `QToolBar`. Actions are not +`QAction`s at all: `registerActions()` (`src/mainwindow.cpp:209`) fills a +`QHash<QString, std::function<void()>>` consumed by an `eventFilter`. Nothing in +that structure can appear in a menu, because a menu needs `QAction` objects. + +**Approach: convert the action registry to `QAction`s.** This is the core of the +work and everything else follows from it. + +- Each entry becomes a `QAction` with text, an object name matching the current + action key, and a shortcut set from `KeyMap`. +- `MainWindow::registeredActionNames()` and the `KeyMap::knownActions()` drift + test (already noted in `mainwindow.h` as hand-maintained) must keep working. + If the conversion lets both lists derive from one source, that test becomes + unnecessary, which is a real win. Check whether it can. +- The `eventFilter` route may become redundant once shortcuts live on the + `QAction`s. Removing it is the goal, but verify: the filter may be handling + focus cases (keys while the query line edit has focus) that `QAction` + shortcuts resolve differently. Do not delete it on the assumption that + `QAction` covers everything. +- Menu bar: File (Sync, Quit), Edit (Undo, Redo), Message (Archive, Delete, + Spam, Toggle unread, Toggle HTML, Load remote content), View (font size, see + item 4), Help (Shortcuts, About). +- Toolbar: the frequent subset only. Sync, Archive, Delete, Undo. A toolbar + holding every action is as unreadable as no toolbar. +- Shortcut reference (item 9): a dialog listing action, description, and current + binding, generated from the same `QAction` list. Generated, never hand-written + in parallel, or it drifts the way the two action lists already do. + +**Constraint:** undo must stay unconfirmed. `CLAUDE.md` is explicit that tag +mutations get undo instead of confirmation dialogs. Adding menu entries must not +smuggle in a "Are you sure?" for Delete. + +**Verification:** the existing keymap test must still pass unchanged, proving +user bindings survive the conversion. That is the load-bearing check here. + +### Outcome (done) + +Built as described: menu bar, toolbar, and a generated shortcut reference. +Four things the plan did not anticipate, all verified by probe rather than +assumed: + +- **The event filter was removable, but not for the stated reason.** The plan + worried that `QAction` shortcuts might lose to `QAbstractItemView`'s + type-to-search. They do not: shortcut dispatch runs before the focused + widget sees the key. The filter is gone, and the thread view no longer + needs its own. +- **Qt already solves the query-bar case.** A plain-letter shortcut is + suppressed while an editable widget has focus, so the `hasFocus()` guard + was unnecessary. Removing it also fixed `Ctrl+Q`, which the old filter + swallowed while typing a query. +- **Three default bindings had never worked.** `N`, `F` and `G` stored the + unshifted key, which no keystroke emits, so `toggle_unread`, `flag` and + `sync` were dead in 0.1.0. Fixed in `KeyMap::normalizeSequence()` and + committed separately from the menu work. +- **The drift test did become unnecessary**, as the plan hoped. + `registeredActionNames()` is now derived from the `QAction`s, and + `defaultBindings()` is the single source for the defaults. The two tests + that pinned the hand-maintained lists together were replaced by ones that + check a configured binding actually reaches its action. + +Defaults moved to modifier shortcuts, since a single letter cannot be a menu +accelerator without claiming that letter window-wide. Existing `[keys]` +entries are unaffected. + +## 4. Message-pane font size does not survive restart + +**Observed:** described as "very annoying", more so than item 1. + +**Confirmed by the user:** Ctrl+`+` / Ctrl+`-` do change the message pane font +size. It is only the persistence that is missing. + +**Cause:** qtmaildir does not implement that zoom. There is no `setZoomFactor`, +no zoom action, and no `Ctrl+=`/`Ctrl+-` binding anywhere in `src/`; grep finds +nothing. The behavior comes from `QWebEngineView`, which handles zoom keys +natively in Chromium. `htmlbuilder.cpp:26` separately hardcodes +`font-size: 10pt` as the document's base size, which the browser zoom then +scales. + +This changes the shape of the work. There is no application-side value to save, +because the application never learns the zoom changed: Chromium handles the key +and adjusts the factor without telling anyone. Persistence therefore requires +taking ownership of zoom first, rather than hooking a save onto something that +already exists. + +**Approach:** + +- Add explicit zoom in, zoom out and reset actions calling + `QWebEngineView::setZoomFactor()`, tracking the current factor in + `MessageView`. Use `setZoomFactor` rather than rewriting the CSS: it scales + the rendered document uniformly, needs no re-render, and does not disturb + `HtmlBuilder`'s output or its tests. +- Bind them to the same Ctrl+`+` / Ctrl+`-` the user already has in their + fingers, so the change is invisible except that it now sticks. Verify the + application binding actually takes precedence over Chromium's built-in + handling; if the web view swallows the key first, the action never fires and + the factor silently diverges from what is on screen. This is the one real + risk in the item and is worth checking before building the rest. +- Persist the factor to the UI state file from item 1, and reapply it on every + `setDocument()`. Do not assume zoom survives a load; `QWebEngineView` may + reset it on navigation. Verify empirically. +- Config entry as the user suggested: a `[general]` key for the starting zoom, + with the state file remembering runtime changes on top of it. The config value + is the default for a fresh profile, the state file is what the user last had. +- Clamp to a sane range. An accidental 0.1 or 25.0 leaves the pane unusable and + the user with no visible way back. Chromium's own limits are roughly 0.25 to + 5.0; match or tighten, never widen. +- Route the actions through item 3's `QAction` conversion so they appear in the + View menu, which also makes the reset discoverable. + +### Outcome (done) + +Built as described, and both of the plan's stated risks turned out not to +exist. Probed rather than assumed: + +- **The application `QAction` wins over Chromium's native zoom key.** The plan + called this "the one real risk in the item". It is not one: the action fires + and the web view's own handling never runs, so the tracked factor cannot + diverge from what is on screen. +- **Zoom survives `setHtml()`.** The plan expected the view might reset it on + navigation and asked for a reapply per render. Not needed; the web view keeps + the factor, so it is the single source of truth and there is no second copy. +- **Do not test key reachability with synthetic input.** A probe using + `QTest::keyClick()` reported `Ctrl++` as a dead binding, and a test was + written asserting it. Both were wrong: `Ctrl++` is exactly what the `+` key + emits on an Italian layout, confirmed against the real keyboard, and it is + the shipped default. Whether a symbol needs Shift is a property of the + layout, not of Qt, and `keyClick()` reproduces neither. The test now only + checks that every default parses. +- `Ctrl+=` is a second binding for reset, skipped when `[keys]` gives `Ctrl+=` + to something else. Ctrl+wheel zooms and Ctrl+middle-click resets, both + filtered by ancestry from an application-level filter: the events land on an + internal `QQuickWidget` the web view creates lazily, so a filter installed on + the view itself never sees them. + +**A pre-existing bug surfaced while adding the config key.** `[general]` +entries were read as `general/<key>`, which matches nothing: QSettings' INI +backend treats a section literally named `[general]` as its own fallback +section and strips the prefix. `notmuch_config` had therefore never worked. +Both keys are now read without the prefix; the file format the user writes is +unchanged. Regression test in `test_config`. + +## 5. Thread list is cramped + +**Observed:** rows are tightly packed, everything is uniform, the UI reads as +"stuffed". + +**Approach:** presentation only, no model changes. + +- Row height: give the `QTableView` vertical breathing room. +- Alternating row colors, or a subtle separator. +- Make unread threads visually distinct (bold), which is the one distinction + that carries real information and currently does not exist. +- Consider dropping a column. Look at what `ThreadListModel` exposes and ask + whether every column earns its width. + +**Caution:** do not hardcode colors. The app should follow the desktop palette; +a hand-picked grey that looks right on a light theme is unreadable on a dark +one. Use `QPalette` roles. This applied to `HtmlBuilder`'s CSS too, which was +split out as item 12 and **done on 2026-08-07**: its colours now derive from +the palette, with the secondary ones blended rather than fixed. The same rule +governs whatever this item adds to the thread list, and item 12's test, which +asserts that no colour appears that the palette did not supply, is the pattern +to copy. + +### Refined by the user, 2026-08-04 + +Two concrete sub-items, from using the list rather than looking at it: + +- **"All items look unread (bold), maybe use regular for read items?"** + **Resolved 2026-08-07. The cause was a misconfigured desktop font, not code.** + + The user's Qt font was set to **Bold in qt6ct**, so every row rendered bold + and nothing could stand out. Bold in the model was working correctly the + whole time. Correcting the qt6ct setting fixed the original complaint on its + own. + + **Read this before trusting any measurement in this file.** Three wrong + conclusions were reached before that came out, and the reasoning behind each + is worth keeping, because the same mistakes are easy to repeat. + + 1. Dismissed from thread counts (99 unread against 4220 read), which + explained why two screenshots looked alike but said nothing about whether + bold rendered. + 2. Dismissed again by a probe counting lit pixels. Antialiasing makes a bold + and a regular glyph light a similar number, so the metric read "identical" + regardless of the truth. **Text width is the honest measure**: with the + font misconfigured both weights measured 277px, and once corrected they + measured 277px against 306px. + 3. Concluded that Qt or fontconfig was broken, from a bare `QTableView` with + a plain `QStandardItemModel` painting two rows identically. That test was + correct and its conclusion was wrong: the baseline font was already bold, + so `setBold(true)` genuinely changed nothing. + + **The dimming was kept anyway**, and stands on its own merits rather than on + that mistaken diagnosis. `ThreadListModel::readColour()` dims READ rows + toward the background while unread keeps the palette's text colour. With 99 + unread among 4220 read, dimming the bulk carries the list better than + emphasising the few, and it is a second cue that survives a font setting like + the one that caused this. Bold still applies on top. + + **A caution for anyone adding another `ForegroundRole` cue.** Qt resolves + that role into the palette and then prefers it over `HighlightedText`, so a + model-supplied colour wins on a SELECTED row too. The dim is blended against + the unselected background, so it landed as grey on the selection highlight, + near unreadable. `SubjectDelegate::initStyleOption` reverses that, and the + delegate is installed view-wide rather than on the subject column alone so + every column gets the same handling. + + **One test had to be rewritten when the font was corrected.** + `aSelectedReadThreadIsNotDimmedIntoTheHighlight` originally compared a + selected read row against a selected unread one and required them to paint + identically. That only held because every row was bold; with bold working, + the unread row differs legitimately. It now asserts the resolved palette + rather than pixels, which is the property the fix actually changes. + +- **A star column for flagged threads**, mirroring the paperclip column that + already exists for attachments. `ThreadSummary` carries the tags and + `flagged` is an ordinary notmuch tag, so this needs no new worker query, the + same way item 15's paperclip did not. Keep it narrow: an icon column, no + text. **Done 2026-08-07**, as `FlagColumn` beside `AttachmentColumn`, using + the same glyph-with-ASCII-fallback pattern (`flagGlyph()`). + +### Built 2026-08-07, and what the layout cost + +The row is roughly doubled in height, with the tags shown as chips beneath the +subject, alternating row colours, and the star column above. + +**The tag strip is painted by the VIEW, not by a delegate**, which is why +`ThreadListView` exists at all. A delegate is handed one cell's rectangle and +cannot paint outside its column, so a strip drawn from the subject column's +delegate stops at that column's edge, losing the last tags of a well-tagged +thread, and starts at that column's left edge, which puts it under the subject +rather than under the row. The user asked for it under the whole row: + +``` +[ date ][ from ][ subject ...................... ] + [ pill ][ pill ][ pill ] +``` + +**Which tags appear.** Everything except `inbox`, `unread`, `flagged`, +`attachment` and the account tag, since the row already shows those as +structure, dimming, the star, the paperclip and the chip. Sorted, because +notmuch's order is not guaranteed stable and a row whose chips reordered +between repaints would flicker. + +**Six defects were introduced and fixed while building this**, every one of +them a consequence of the same thing: a `QTableView` paints PER CELL, and a +row-wide strip is not a cell. Worth listing, because each is easy to +reintroduce. + +1. `SubjectDelegate` was installed view-wide to spread the selection fix + across every column. It reads `AccountLabelRole`, which belongs to the row, + so every column drew the account chip. Split into `RowStyleDelegate` (the + selection fix, every column) and `SubjectDelegate` (chip, subject column + only), with a `Q_ASSERT` guarding the latter. +2. Row height was returned from `sizeHint`, which does nothing: a table takes + ONE height per row, so a hint from a single column applies only if the view + happens to ask that column. Set on the vertical header instead. +3. The strip used `viewportMargins().left()`, which is 0, so it painted from + the viewport edge across the marker columns. It compiled because the method + is protected and the call was inside the subclass. +4. The text band and the strip were measured with one font, so the pills rode + up over the date and sender. +5. Alternating colours and the selection are painted per cell, so the strip's + band showed the bare viewport background as a stripe across every other + row. The view now fills that band itself, and must honour three cases: the + model's own `BackgroundRole` first (a deleted thread's fill would otherwise + be cut in half), then the selection, then the alternating colour. +6. That fill spanned the full width, and the marker glyphs are centred in the + full row height, so its top edge cut the paperclip and star at their + midpoint. The band starts at the date column now. + +**A note on verifying any of this.** Several rendering probes written during +this work returned results that were confidently wrong: counting "lit" pixels +cannot tell bold from regular, since antialiasing lights a similar number +either way, and `viewport()->render()` returned a blank image more than once. +Text width distinguishes weights; a strict pixel diff distinguishes renders; +an ink count distinguishes nothing. Two versions of the strip's own test passed +under mutation before one was written that matched the exact chip colours the +model supplies. + +## 6. Opened message stays unread + +**Observed:** opening a message leaves it tagged unread; the user expects it to +become read. + +**Decision (user, 2026-08-03): mark read after a 2 second delay, configurable.** + +**Approach:** + +- A `QTimer` started in `onThreadSelected()` (`src/mainwindow.cpp:373`), fired + once, removing the `unread` tag from the displayed thread. +- The timer **must** be restarted, not stacked, when the selection changes. + Arrowing quickly down a list must not mark ten threads read; only the one + still selected when the timer fires. +- **Decided (user, 2026-08-03): the automatic mark-read does NOT go on the undo + stack.** The tag change routes through `sendThreadTagChange()` directly, + bypassing the `ThreadTagCommand` push, exactly as an explicit non-undoable + mutation would. Rationale: a manual toggle-unread action already exists + (`toggle_unread`, `src/mainwindow.cpp:242`), so a user who wants the message + back as unread has a direct route and does not need undo for it. Leaving a + message read after undoing an unrelated archive is acceptable; hijacking + Ctrl+Z to undo an action the user never took is not. +- Consequence to keep in mind: `applyTags` is the funnel for all mutations per + `CLAUDE.md`, and that stays true. What changes is only whether the inverse is + pushed onto the `QUndoStack`, which is a `MainWindow` decision made above the + worker. Do not add a second write path to the worker for this. +- Config key in `[general]`, e.g. `mark_read_delay_ms`, default `2000`, with `0` + meaning "immediately" and a negative value meaning "never". Document all + three in the README. +- Interaction with item 3's toggle-unread action: if the user explicitly marks a + message unread, the timer must not immediately re-mark it read. Cancel the + pending timer on any manual unread toggle. + +**Verification:** unit-testable against the throwaway notmuch database the +`NotmuchWorker` tests already build, but the timer logic itself is UI-side and +easier to check by hand. At minimum, verify the rapid-arrow case manually. + +### Outcome (done) + +Built as specced, including every decision recorded above: a 2000 ms default, +`mark_read_delay_ms` in `[general]`, the automatic change kept off the undo +stack via `sendThreadTagChange()`, and an explicit `toggle_unread` cancelling +any pending timer. + +**The rapid-arrow case is unit-tested, not left to hand-checking.** The plan +expected it to need a database and a person; it needs neither. `ThreadListModel` +takes threads directly through `appendBatch()`, so a test builds three unread +rows, arrows through them, and asserts one timer stays armed. The timer carries +an object name so the test observes it through `findChild` rather than the +window exposing it. + +**The three tests were verified by breaking the code**, since a passing test +proves nothing until it has been seen to fail: + +- Removing the already-read check arms a timer for a read thread, caught. +- Creating a fresh timer per selection instead of restarting one, which is + precisely the stacking the plan warns about, fails two of the three. + +**Two guards the plan did not call for**, both from asking what happens when +the timer outlives its thread. `scheduleMarkRead()` refuses to arm for a thread +that is not unread, so a read thread never schedules a write that would change +nothing. `markCurrentThreadRead()` re-checks that the thread it was armed for +is still selected AND still unread before writing, so a timer that survives a +selection change or a manual toggle does nothing rather than tagging the wrong +thread. + +## 7. HTML view should be default for HTML messages + +**Verify before doing anything.** `MessageView::m_preferHtml` is already +initialized to `true` (`src/messageview.h`), and `clear()` resets it to `true` +(`src/messageview.cpp:164`). HTML should already be preferred where a message +offers it. + +Possible explanations for the observation: + +- The messages in question are `multipart/alternative` and `HtmlBuilder`'s + `PreferHtml` mode is not selecting the HTML part correctly. +- The HTML renders, but with remote content blocked it looks like plain text. +- `m_preferHtml` is being reset between messages by a `clear()` the user did not + intend to trigger. + +Reproduce first with a specific message, then decide. If it turns out to work +correctly, the item becomes a documentation gap rather than a bug, and the +user-preference key mentioned in the note (`prefer_html`, `[general]`) is still +worth adding for people who want plain text by default. + +**Do not**, in the course of this, relax anything in the web view security +section of `CLAUDE.md`. Preferring HTML is orthogonal to remote content, which +stays blocked and per-render. + +### Outcome (done, 2026-08-04): nothing was broken + +**Verified by the user against real mail: HTML messages do open as HTML.** The +item was raised on an observation that could not be reproduced afterwards, and +the code was already correct: `m_preferHtml` initialises to `true` and `clear()` +resets it to `true`, so every thread starts in `PreferHtml`. + +No code changed. Recorded as done rather than dropped, since the behaviour the +item asked for is the behaviour that ships. + +The `prefer_html` config key the item floated for people who want plain text by +default was **not** added: nobody has asked for it, and `toggle_html` +(`Ctrl+H`) already switches a thread by hand. Add it if someone wants the +default flipped, not before. + +## 10. Reaching an account's inbox takes two steps + +**Observed:** select account from the dropdown, then click inbox or unread. + +**Approach:** cheapest useful fix first. + +- Persist the selected account across restarts (uses item 1's state file). If + the user reads one account 90% of the time, this alone removes most of the + friction. +- Then: per-account entries in a menu, or saved queries that carry their own + account scope, so one action gets there. `Account::scopedQuery()` already + exists in `Config`, so the composition is available; it is a UI question, not + a query question. + +Do not build a full account sidebar for this. Persisting the selection may +resolve the complaint entirely, and it is a fraction of the work. Reassess after. + +### Partly done + +**The startup query is now chosen by name**, not by sort order. `[queries]` is +read through `childKeys()`, which sorts alphabetically, so the old +`savedQueries().first()` opened whichever entry happened to sort first, which +is why the app came up on Inbox. `[general] startup_query` names the entry, +defaults to `Unread`, and falls back to the first saved query when the name +matches nothing. Only a name the user wrote is worth a warning: the built-in +default naming a query they never created is not something they got wrong. + +Neither half of item 10 proper is done: the account selection still resets on +restart, and reaching an account's inbox is still two steps. + +### Postponed (user, 2026-08-04) + +**The user does not intend to go this route as of now.** Postponed rather than +dropped: the complaint was real, and the cheap fix the item proposes (persist +the account selection across restarts) is still the right first move if it is +picked up again. Nothing here is invalidated, it is simply not wanted yet. + +Only the startup-query half shipped, in 0.3.0. Do not propose the remaining +work unprompted. + +## 11. Icon, `.desktop` file, SlackBuild + +Packaging, independent of everything above, and can proceed in parallel. + +- **Icon:** an SVG plus rendered PNGs at the standard hicolor sizes. Needs a + design decision, not just code. +- **`.desktop` file:** `Categories=Network;Email;`, `Terminal=false`, + `MimeType=x-scheme-handler/mailto;` only if a `mailto:` handler is actually + implemented, which it is not in 0.1.0. Do not claim the MIME type until it + works; a desktop entry that registers as the mail handler and then does + nothing is worse than not registering. +- **CMake install rules:** icon into `share/icons/hicolor/<size>/apps/`, desktop + file into `share/applications/`. Neither exists yet. +- **SlackBuild:** per the global workflow, sources are authored here and the + user builds. Deliverables are `qtmaildir.SlackBuild`, `.info`, `README`, + `slack-desc`, plus an nvchecker stanza. Depends on a tagged release existing + to point `DOWNLOAD` at, so it follows a tag rather than leading it. + +--- + +## 12. Message pane is light-theme only + +**Split from item 5**, which recorded the rule against hardcoded colours. +Listed in the deferred table until it was picked up on 2026-08-07. + +**Observed:** the user runs a dark desktop (`color-scheme: prefer-dark`), and +plain-text mail rendered as black on white inside a dark window. + +**Cause (verified in code):** `kStyle` in `src/htmlbuilder.cpp` hardcoded +`#bbb`, `#555`, `#000`, `#666`, `#ddd` and `#4a6f8a`, and set **no background +at all**, so the web view's own default showed through whatever the desktop +was. + +**Approach as built.** A `HtmlBuilder::Palette` struct passed into +`build`/`buildThread`, derived from a `QPalette` by `paletteFrom()`. Passed in +rather than read from `qApp` inside the builder, so the stylesheet can be +tested against a known palette with no running application. + +- **`Base` and `Text`, not `Window` and `WindowText`.** The pane is a content + surface like a text edit, and on many themes `Base` differs from `Window`. +- **The derived colours are blends, not fixed greys.** This is the part that + makes it work both ways round: a `#555` chosen to read as "subtle" on white + is nearly invisible on `#2b2b2b`. `dim` and `border` are mixes of text and + background, so they land at the right contrast whichever way the theme goes. +- The quote colour keeps its hue, since "this is quoted" is carried by being a + different colour rather than a dimmer one, but it is pulled toward the + background so it stays readable rather than glowing on dark. + +Measured on the user's actual theme: background `#2b2b2b`, text `#dedede`, dim +`#969696`, border `#585858`, quote `#6490b0`. + +**Scope, and it is asserted in a test so it cannot drift.** A message that +brings its own HTML brings its own colours, and those are left alone. +Rewriting a sender's styling would break layouts that depend on it, and a +newsletter that sets a white background is entitled to stay white. This item +themes the plain-text render and the chrome around messages, nothing else. So +HTML-heavy mail will still look light, correctly. + +**`MessageView` passes its own widget palette**, not the application's: a style +sheet or a themed parent can give the pane different colours from `qApp`. It +also re-renders on `QEvent::PaletteChange`, because the document's colours are +baked into its stylesheet at build time and it does not restyle itself the way +a widget does; without that, switching the desktop theme would leave the open +thread on the old palette until the next selection. + +**Verification.** The load-bearing test asserts the **negative**: no hex colour +appears in the `<style>` block that the palette did not supply. A test that +only checks the palette's colours are present passes with a leftover literal +still in place, and a single leftover literal is the entire defect. Confirmed +by mutation: putting one hardcoded colour back fails it. + +## 13. No visual feedback that an action stuck + +**Observed:** selecting a thread and hitting Delete changed nothing on screen. +No way to tell whether the thread was really going to be deleted on the next +sync, which is bad UX for every tag action, not only delete. + +**Cause:** not a missing update. `ThreadListModel::applyTagChange()` already +added the tag and emitted `dataChanged` across the whole row, so the Tags +column did change. But `SubjectColumn` was set to `QHeaderView::Stretch` while +`TagsColumn` came after it, so Subject absorbed all free width and pushed Tags +out of view. The feedback existed in the one column that could not be seen. + +**Approach:** two changes, since the cause was two things. + +- Column order is now Tags, Date, From, Subject. Subject stretches and is + last, so nothing sits to its right to be pushed out. The other three size + to their contents. +- A thread tagged `deleted` or `spam` styles its entire row: muted dark red + (`#8b2c2c`) or orange (`#a85c18`) fill, white text, struck through. Applied + through `Qt::BackgroundRole`, `Qt::ForegroundRole` and `Qt::FontRole` for + every column, so no cue depends on a single column staying visible. + +Strike-through rides along with the fill deliberately: it survives a theme +that overrides background colours, a colourblind reader, and a screenshot. +Bold for unread still composes with it. + +**Decisions:** no status-bar or toast changes, the existing `tagSelected()` +message stays as it is. Archive removes `inbox` and adds nothing, so an +archived thread gets no row styling; whether it should disappear from an inbox +query is deliberately left open rather than guessed at. + +**Verification:** four model tests covering the colours, the strike-through, +that styling spans every column, and that undo restores a plain row. Rendered +and inspected: normal, unread, deleted, spam, and deleted-plus-unread rows. + +--- + +## 14. Tag column unreadable + +**Observed:** with tags spelled out per row the column ran to 500 pixels of +mostly repeated text (a 33-character account tag followed by "attachment +flagged inbox passed replied"), dominated by the account prefix, and consumed +most of the list's width. + +**Cause:** presentation, not data. 96 tags in this database, many hierarchical +(`shopping/amazon`, `mailing-list/SBo`), rendered as a joined string. + +**Approach:** the column is gone. Tags now render as coloured chips in two +places, split by taxonomy: + +- The **account tag** says which mailbox a thread came from. It draws as a chip + in front of the subject, coloured and labelled from its own `[account.<key>]` + stanza via new `color` and `label` keys. `label` is display-only; the notmuch + tag is never renamed. +- **Functional tags** say what state a thread is in. They fill a single row + under the message pane, with overflow collapsing into a `+N` chip whose + tooltip lists the hidden ones. A single row keeps the message area from + shifting between threads with different tag counts. + +Colours resolve exact tag first, then top-level prefix, so one `shopping` entry +covers the hierarchy without listing all 96. Unconfigured tags fall back to a +hash of the name, stable so a chip never changes colour as the list scrolls. + +**Defect found while building:** QSettings treats `/` in a key as a group +separator, so `shopping/amazon` becomes a nested key that `childKeys()` never +returns. Reading `[tagcolors]` with `childKeys()` silently dropped every +hierarchical tag, and each fell through to its prefix colour. Fixed by reading +`allKeys()`, with a regression test. The same gotcha is already documented in +`CLAUDE.md` for `[account.work]` section names. + +**Deferred:** clicking a chip to search that tag. Display only for now. + +## 15. Attachments are parsed but unreachable from the UI + +**Observed, 2026-08-03, with a screenshot.** A thread known to carry +attachments shows the `attachment` chip and a body that refers to them, but +there is no way to download or open one. The user also has no way to tell a +message has an attachment before opening it. + +**Cause: the attachment bar is an empty placeholder.** +`MessageView` creates `m_attachmentBar` and gives it a layout +(`src/messageview.cpp:130`), adds it to the pane (`:142`), and then **nothing +ever puts anything in it**. `m_attachmentBar` appears nowhere else in the +codebase, and neither `render()` nor `showThread()` reads +`ParsedMessage::attachments`. The bar has never displayed an attachment. + +This is a gap in the UI only. The backend is complete and already hardened: +`MimeParser` fills `attachments`, and `Attachment` has `safeFilename()`, +`saveTo()` and `isPathInsideDirectory()` with the path-traversal guard +`CLAUDE.md` describes. None of that work needs redoing; it needs calling. + +**Approach, two independent pieces.** + +- *Populate the bar.* For each attachment on each rendered message, one button + showing the safe filename and the size. Clicking saves, through a + `QFileDialog` for the target directory, then `Attachment::saveTo()`. Report + the written path in the status bar, since a silent save is the same UX + failure as item 13. +- *A paperclip column in the thread list*, so an attachment is visible before + opening. `ThreadSummary` already carries the thread's tags and notmuch + applies `attachment`, so the column can be driven from the existing tag data + with **no new worker query**. Keep it narrow: an icon column, no text. + +**Constraints.** + +- **Filenames are untrusted.** Display `safeFilename()`, never the raw + `filename`, and never interpolate either into rich text without escaping. + The save path must go through `saveTo()`, which is where the boundary check + lives. +- **Do not add "open in default application" in the same change.** That means + handing a file from a stranger to `xdg-open`, which is a materially + different security decision from writing it to a directory the user picked. + If it is wanted, it is its own item with its own reasoning. +- A thread renders as one document, so the bar must make clear which message + an attachment belongs to once a thread has several. Grouping by message, or + a per-message row inside the HTML, are both plausible; decide when building. + +**Verification:** a message with a known attachment saves a byte-identical +file. A message with an attachment named `../../etc/passwd` writes inside the +chosen directory under a sanitised name and nowhere else. + +### Outcome (done) + +Both halves built. The paperclip column needed no new worker query, as the +plan expected: notmuch applies the `attachment` tag itself, so +`ThreadSummary::hasAttachment()` reads what is already there. + +**The bar holds ONE button, not one per file.** The plan's "one button showing +the filename and size" was built first and was wrong: a thread with sixteen +attachments made the bar as wide as the window, pushed the splitter over, and +left the thread list a few pixels wide. It is now `Attachments (N)...` opening +a dialog that lists message number, filename and size with a `Save...` each, +plus `Save all...` when there is more than one. No filename reaches the bar, +so no filename length can resize anything. + +**`Save all` writes into a new subdirectory** named `<date> <subject>`, inside +a parent the user picks. Chosen over zipping: Qt ships no zip API, so a real +`.zip` meant either a new build dependency (quazip, libzip) or shelling to +`/usr/bin/zip` at runtime, and the actual requirement was "do not drop sixteen +files loose among hundreds of others". The picker's title names the subfolder +before the user commits to a location. + +**Two defects found only by using it, both silent:** + +- **`saveTo()` overwrites, which destroyed six of sixteen files.** Several + messages in one thread commonly attach the same filename; each write landed + on the previous one and every one reported success, so the status line said + 16 while the directory held 10. `saveWithoutOverwriting()` now backs the + batch path, appending " (2)" before the extension and keeping a compound + extension whole. `saveTo()` still overwrites, which is right for a single + save the user just chose a location for. +- **`Qt::RFC2822Date` rejects a date carrying a timezone comment.** A header + ending `+0200 (CEST)` is legal per RFC 5322 and common in real mail, and Qt + refuses the whole string rather than the comment, so every such message lost + its date prefix. Comments are stripped before parsing. + +**The subject is untrusted and becomes a directory name.** +`attachmentFolderName()` lives beside the other guards in `mimeparser.cpp`, +strips separators, control characters and leading dots, caps length at 120, +and falls back to a generated name. Its test drives `../../etc`, +`/etc/passwd`, `..`, `.hidden`, a backslash and a null byte, then asserts each +result still resolves inside the parent through `isPathInsideDirectory()`. + +**Deferred, as the plan required:** opening an attachment in its default +application. That hands a stranger's file to `xdg-open` and is a separate +decision from writing it to a directory the user chose. + +## 16. Delete on an already-deleted thread should undelete + +**Observed:** hitting Delete twice on the same message is a natural way to +express "no, put it back", but the second press does nothing visible because +adding a tag that is already present is a no-op. + +**Approach:** make `delete` a toggle, the way `toggle_unread` already is. +Deleting a thread that already carries `deleted` removes it instead. + +**Constraints.** + +- **A multi-row selection must not split.** If some selected threads are + deleted and others are not, toggling each independently leaves the selection + in two states from one keystroke, which is worse than either outcome. Decide + one direction for the whole selection: the natural rule is "if every + selected thread is already deleted, undelete them all; otherwise delete them + all." +- Undo already covers the mistake case, so this is a convenience, not a safety + fix. It must not grow a confirmation dialog. +- The same question applies to `spam` and `flag`. Do not change those in this + item; note whether the answer generalises once `delete` is built. + +### Outcome (done) + +Built as specced, including the all-or-nothing rule: undelete only when every +selected thread already carries `deleted`, otherwise delete the whole selection. +A test covers the mixed case and passed before the change, since the old +always-delete behaviour satisfies it; it is there to stop a later "improvement" +from toggling per row. + +**It generalises to `spam` and `flag`, and they were still left alone.** The +same shape would work, but neither has been asked for, and `flag` in particular +is already reachable both ways through the tag dialog. + +## 17. No completion for tags in the query bar + +**Observed:** typing a query means remembering the exact tag name, including +hierarchy (`shopping/amazon`). + +**Approach:** a `QCompleter` on the query bar, offering tag names after +`tag:`. + +**Cause of the work being larger than it looks:** there is **no way to list +tags today**. `NotmuchWorker` has no all-tags call, so this needs a new +worker request and result signal, following the existing generation-counter +pattern. `notmuch_database_get_all_tags()` is the underlying call. + +**Constraints.** + +- The tag list must be fetched on the worker thread like everything else. No + `notmuch_*` pointer crosses the thread boundary; the result is a + `QStringList`. +- Refresh after a sync, since a sync can introduce tags. Do not refetch per + keystroke. +- Completion should trigger on the `tag:` prefix specifically rather than on + every word, or it will offer tag names where a `from:` value belongs. + +--- + +## 18. No visual cue that there are unsynced edits + +**Observed (user, 2026-08-04):** tagging changes the notmuch index immediately, +but nothing in the UI says those changes have not reached the mail store. The +user cannot tell, at a glance, whether quitting now would leave work stranded. + +**Cause: nothing tracks it, and the obvious candidate cannot.** `MainWindow` +holds a `QUndoStack` (`src/mainwindow.h:142`) which looks like a record of +pending edits, but it is **cleared on every query** +(`src/mainwindow.cpp:805`, in the query-start path) because undo entries refer +to rows the new result set is about to discard. Tag a thread, then run any +query, and the stack is empty while the database change is still unsynced. The +same clear happens on a failed write (`:902`). + +So `QUndoStack::isClean()` is **not** usable as the signal here, and neither is +`canUndo()`. This needs its own counter, one that only a successful sync +resets. + +**Approach:** + +- Count confirmed mutations, incremented where `tagsApplied` is handled (the + same place that already clears `m_pendingChange`), and reset to zero on + `MailSync::finished(true, ...)`. A count, not a bool, so the indicator can + say how many. +- Show it in the status bar, next to the existing sync status rather than as a + new widget competing with it. Wording should name the unit the user thinks + in: threads or messages changed, not "mutations". +- Nothing modal, nothing blocking. This item is the passive cue only; the + question of interrupting the user belongs to item 19. + +**Constraints:** + +- **A failed sync must not clear the count.** `finished(false, ...)` means the + edits are still unsynced, and clearing there would assert the opposite. +- The count must survive a query, which is the whole reason it cannot ride on + the undo stack. Do not tie its lifetime to the model. +- An external `notmuch new` from the user's cron can sync changes without the + app knowing. The count is therefore a lower bound on confidence, not a + guarantee, and the wording should not promise more than it knows. + +**Verification:** tag a thread, run an unrelated query, confirm the cue +survives. Then sync and confirm it clears. Then make the sync fail and confirm +it does not. + +### Outcome (done) + +Built as specced. `m_pendingEdits` counts confirmed mutations, incremented in +`onTagsApplied()` and reset only by `onSyncFinished(true, ...)`. The count is +shown as a permanent status-bar widget, hidden entirely at zero. + +**Counted where a write is confirmed, not where one is sent.** An optimistic +update the worker later rejects must not leave the indicator claiming an edit +that never landed, so the increment sits in the `tagsApplied` handler. + +That handler was a lambda; it is now a named slot, which is both better +structure and what lets a test drive it. An earlier draft tried to reach the +worker with `findChild` to emit the real signal: the worker is deliberately +parentless because it moves to its own thread, so that cannot work, and +contorting the test to reach it was the wrong instinct. What matters is the +counter's behaviour, not the signal's origin. + +**Both tests were verified by breaking the code.** Clearing the count on any +sync outcome is caught, and so is clearing it where the undo stack is cleared, +which is the naive design this item exists to avoid. + +## 19. No prompt to sync on exit when edits are pending + +**Observed (user, 2026-08-04):** quitting with unsynced edits is silent. The +user asked for a blocking prompt offering to sync first, and suggested a +`sync_on_exit` config option. + +**Depends on item 18.** Both need the same "are there pending edits" counter, +and 18 establishes it. Build 18 first; this is the interruption layer on top. + +**Cause:** `MainWindow::closeEvent()` (`src/mainwindow.cpp:145`) saves UI state +and accepts unconditionally. It never consults sync state, and cannot today, +for the reason item 18 documents. + +**Approach:** + +- On close with a non-zero pending count, a modal question: sync now, quit + without syncing, or cancel. Three options, not two: a user who hit Quit by + mistake needs a way back that is not "sync". +- `[general] sync_on_exit`, as the user suggested. Sensible values are `ask` + (default, the prompt above), `always` (sync without asking), and `never` + (quit silently, today's behavior). A bare true/false cannot express all + three. +- Choosing to sync means the window must stay alive until `MailSync::finished` + arrives, since killing the process mid-sync is exactly the data loss the + prompt exists to prevent. Ignore the close event, show progress, and close on + the finished signal. + +**Constraints:** + +- **This is not a destructive-action confirmation** and does not contradict + `CLAUDE.md`'s rule against those. That rule is about tag mutations, which + keep undo instead of a dialog. This asks about *losing* work at a point where + undo no longer helps, which is the opposite situation. +- A sync that fails on exit must not silently discard the user's choice. Report + it and leave the window open rather than quitting as if it had worked. +- Quitting must remain possible when the sync command is not configured at all. + `MailSync::isAvailable()` is already false in that case, so the prompt should + degrade to a plain warning with no sync option rather than offering one that + cannot run. + +**Verification:** by hand. Tag, quit, take each of the three branches. Then set +each `sync_on_exit` value and confirm the behavior matches. Then quit with a +deliberately broken sync command and confirm the app neither hangs nor lies. + +### Outcome (done) + +Built as specced, with the user confirming the three-value key over their +original on/off idea. `closeEvent()` consults the count, and a sync started for +exit holds the window open until `finished` arrives rather than being killed +mid-run. + +**A failed exit-sync does not quit.** It restores the window, shows the log and +says what happened. Quitting there would discard the user's choice silently, +which is the exact failure the prompt exists to prevent. + +**With no sync command configured the prompt degrades** to a warning offering +Discard or Cancel, rather than offering a sync that cannot run. +`MailSync::isAvailable()` is already false in that case. + +**Testing a modal needs care, and the first attempt was wrong.** A test that +sends a close event hangs forever if an unexpected dialog opens, because the +modal spins its own event loop. The first version appeared to pass in 19 +seconds; it had actually opened a real dialog on the user's screen, and the +user dismissed it. `CloseProbe` now polls for `activeModalWidget()`, closes it, +and records that one appeared, turning "a dialog opened" into an assertion +instead of a hang or a prompt for whoever is watching. + +**One mutation test was a dud and is worth recording.** Removing the `Never` +guard from `closeEvent` did NOT make the test fail, because the branches below +match only `Ask` and `Always`, so `Never` fell through to closing anyway. The +guard is redundant belt-and-braces rather than load-bearing. Confirming the +probe really detects a prompt needed the config mutated to `ask` instead, which +does fail it. + +## 20. Thread view does not match the user's mental model + +**Observed (user, 2026-08-04), in passing while deciding item 2:** "My view for +the thread visualization was different than what was built, but that's ground +for a small refactor later." + +**Specified 2026-08-08.** The user described the model with three Thunderbird +screenshots and four decisions. It is **not a message-pane redesign**: the +change is in the LEFT pane. A thread occupies one row carrying an "N replies" +affordance; expanding it reveals the replies as rows in the same list, indented +by reply depth; clicking one opens that single message in the reading pane. The +unit of selection stops being the thread and becomes the message, which is the +substance of the item and the source of its risk. + +**Decisions taken (user, 2026-08-08), each closing an option that the +screenshots alone did not settle:** + +1. **The root row IS the thread's first message**, not a thread-level summary + row. Expanding reveals only the REPLIES, so a thread of 7 shows 1 root and 6 + children, matching "6 risposte" in the screenshot. +2. **A true reply tree, indented by depth**, not a flat chronological list of + replies. notmuch supplies the structure via `notmuch_message_get_replies`. + Deep chains indenting off-screen and malformed `References` headers producing + misleading trees are accepted costs, to be capped in the view rather than + flattened in the model. +3. **Action scope follows the selected row.** A message row acts on that + message, a thread root acts on the whole thread. +4. **No confirmation dialog**, per this project's standing rule; the scope is + made visible instead, in the status bar, BEFORE the action ("1 thread + selected (7 messages)") and AFTER it ("Deleted 7 messages (whole thread)"). + The hazard this design introduces is not destruction, since `deleted` is a + tag and every mutation is already invertible on the undo stack. It is + **ambiguity**: with two kinds of row selectable, a keypress alone no longer + says whether it hit one message or seven. Naming the scope removes the + ambiguity where it exists, which a dialog on every correct action would not. + The user asked for a warning popup, was shown the rule in `CLAUDE.md`, and + chose this instead. + +**Improvement the user named but deferred:** moving from one message to the next +within a thread without returning to the list. An addition on top, not part of +this item. + +**No longer deferred, 2026-08-09.** Folded into the card-list spec, because it +turned out to be a defect repair rather than an addition: `next_thread` is +`selectRow(current.row() + 1)`, and in a tree that names a sibling, so from the +last reply of an expanded thread the action does nothing at all. Up/Down get the +behaviour for free from `QTreeView`'s own navigation, and Alt+Up/Down keep the +thread-to-thread jump. + +**What exists today**, as the starting point: a thread is +one HTML document in one web view, messages stacked in chronological order, +each with a small grey `.msg-header` carrying From and Date +(`src/htmlbuilder.cpp:241`). Messages that did not match the query render as +stubs (`MessageRef::matched`, `src/types.h:63`). It is deliberately flat: +`CLAUDE.md` records that reply structure is available from notmuch but is not +drawn as an indented tree. + +The single-document design is not incidental and constrains any redesign: a +`QWebEngineView` per message would spawn a Chromium render process each, which +is why the thread is one document and why `cid:` references are namespaced per +message. A refactor that splits messages into separate views has to answer that +cost first. + +**What the codebase does not have yet**, verified 2026-08-08 rather than +assumed: + +| Needed | Status today | +|---|---| +| Per-message From / Subject / Date | Absent. `MessageRef` (`src/types.h:54`) carries only id, path, tags, matched | +| Reply parent, for indentation | Not carried at all. notmuch has it, the worker never asks | +| A tree-capable left pane | `ThreadListModel` is a `QAbstractTableModel`; `ThreadListView` is a `QTableView` | +| Loading ONE message into the pane | Absent. The worker exposes `loadThread` only | +| An expand affordance on the row | Nothing | + +**The two loads bearing the risk.** + +*The view port.* `ThreadListView` exists because a delegate cannot paint outside +its column, so the tag strip is painted across the full row width in +`paintEvent`. That code is written against a table. Mitigating fact, checked +rather than hoped: every geometry call it uses (`rowAt`, +`rowViewportPosition`, `rowHeight`, `columnViewportPosition`) exists on +`QTreeView` with the same semantics, so this is a port, not a rewrite. What does +NOT carry over is `isRowSelected(int)`, and the strip must not paint under +child rows the way it does under thread roots. + +*The selection semantics.* Every action path today assumes a selected row is a +thread: `applyTagsToThreads` resolves `thread:a or thread:b`, and the undo stack +and pending-edit map are thread-scoped throughout. Mitigating fact: they all +funnel through `m_model->threadAt(current.row())`, so the row-to-thread mapping +is centralised rather than smeared across call sites. + +**Size: L.** Larger than anything else in this backlog, and the first item to +warrant a feature branch (`item-20-message-rows`, 2026-08-08). It is a left-pane +model and view replacement plus a selection-scope change, not a refactor. + +**Built 2026-08-08 across ten tasks, on the branch `item-20-message-rows`, and +rejected on sight.** All four decisions were implemented: message rows in the +left pane, the root row as the thread's first message, replies indented by +depth, action scope following the selected row kind and named in the status bar. +15 test binaries green, 65 tests in `test_mainwindow` alone, ten mutation checks. + +**That branch was never merged and is kept at 029a50e as the record of the +rejected presentation.** What reached master on 2026-08-10 is the card list, +built on top of it: the model, the reply walk and `MessageNode` survived intact, +and the five-column grid they were drawn into did not. Read item 53 for the +diagnosis. + +**The user's verdict on the result: not convinced it fits.** Recorded here +rather than in a commit message, because the item shipped working and the +reservation is about the DESIGN, not about a defect. See item 53, which carries +the diagnosis. Do not treat item 20 as a success story to build on without +reading it. + +**A cheaper alternative was offered and declined.** Collapsible `<details>` +blocks per message inside the existing single-document message pane would have +delivered per-message inspection at size S, touching only `htmlbuilder.cpp` and +neither fragile area, but gives no message rows in the list and no reply-tree +indentation. The user chose the full model deliberately. + +## 24. No right-click actions on the thread list + +**Observed (user, 2026-08-04):** "right click actions on the list (left pane)". + +**Cause: there is no context menu anywhere in the application.** No +`contextMenuEvent` override and no `Qt::CustomContextMenu` policy in `src/`; +grep finds neither. Right-clicking a thread does nothing at all. + +**Approach.** The actions already exist as `QAction`s from item 3, so this is +presentation rather than new behaviour: set `Qt::CustomContextMenu` on the +thread view and build the menu from `m_actions`, exactly as `buildMenus()` +already does. Nothing should be reachable from the context menu that is not +reachable from the menu bar, or the two drift. + +**Constraints.** + +- **Show the shortcut in the menu**, which a `QAction` does for free. The + context menu is where a mouse user discovers the key for next time, and this + backlog exists because the app did not teach its own bindings. +- The menu must act on the **selection**, not on the row under the cursor, or + right-clicking inside a multi-row selection would silently act on one thread. + Qt does not do this for you: right-clicking does not change the selection, so + the row under the cursor and the selected rows can differ. +- Include the destructive entries (archive, delete, spam) without a + confirmation dialog, per `CLAUDE.md`. Undo covers them. + +## 25. No select-all, and bulk actions are undiscoverable + +**Observed (user, 2026-08-04):** "bulk select/select all for defined actions? +tags, read/unread, archive, delete". + +**Cause: half of this already works, and nothing says so.** The thread view is +already `QAbstractItemView::ExtendedSelection` (`src/mainwindow.cpp:408`), and +`tagSelected()` already acts on every `selectedRows()` entry, resolving them in +ONE combined query per `CLAUDE.md`. Ctrl+click and Shift+click therefore do +bulk tagging today. + +What is genuinely missing is smaller than the item sounds: + +- **No select-all action.** `selectAll` appears once in `src/`, on the query + bar, not the thread list. There is no `Ctrl+A` for the list and no menu entry. +- **No indication that multi-select exists.** With no context menu (item 24) and + no selection count anywhere, a user has no reason to think Ctrl+click will do + anything useful. + +**Approach.** Add a `select_all` action bound to `Ctrl+A`, scoped to the thread +list rather than the window, so it does not steal Ctrl+A from the query bar. +Then surface the selection size: the status bar already reports "%1: %n +thread(s)" after a tag action, and saying how many rows are selected before one +is the same idea a moment earlier. + +**Constraint: verify what a large selection costs before encouraging one.** +Select-all over a 10k-thread query makes `tagSelected()` build a 10k-id list and +`applyTagsToThreads()` one enormous `thread:a or thread:b or ...` query. The +combined-query design is what makes this plausible at all, but "plausible" is +not "measured", and this item is the one that turns a rare accident into a +routine keystroke. Measure it against a real query before shipping the binding. + +## 26. No way to add or remove an arbitrary tag from the UI + +**Observed (user, 2026-08-04), as a question:** "as it is today, how do I add a +new tag to a message?" The answer is that you cannot. The only route is +`notmuch tag` in a terminal, or the companion `mailctl`. + +**Cause: every tagging action writes a hardcoded tag name.** `registerActions()` +offers exactly five: `archive` (removes `inbox`), `delete` (adds `deleted`), +`spam` (adds `spam`, removes `inbox`), `flag` (adds `flagged`) and +`toggle_unread`. Nothing accepts a tag the user types. For an application whose +entire purpose is organising mail by tag, that is a conspicuous hole, and it is +the reason this item exists at all rather than being folded into item 25. + +**The machinery is already complete.** `tagSelected(add, remove, description)` +takes arbitrary lists and is the single funnel every mutation already uses; +`applyTagsToThreads()` resolves a multi-row selection in one combined query; +undo works through `TagChange::inverted()`; and `QueryCompleter` already holds +every tag in the database, refreshed on the worker thread. This item is a +dialog and two actions, not new plumbing. + +**Approach.** + +- An `add_tag` action opening a small dialog with a line edit, and a `remove_tag` + counterpart. Both act on the whole selection, like every other tag action. +- **Complete against the known tag list.** The completer's tag vocabulary is the + same data, so offering it here costs nothing and prevents the obvious failure + mode: typing `shoppping` and silently creating a new tag next to `shopping`. + This is the strongest argument for the dialog over a free-text prompt. +- Removing should offer the tags actually present on the selection rather than + every tag in the database, since removing a tag that is not there is a no-op + the user cannot see. + +**Constraints.** + +- **A tag name is not free text.** notmuch accepts a lot, but a leading `-`, an + embedded space or an empty string will produce confusing results or a failed + write. Validate before sending, and say what was rejected. +- No confirmation dialog, per `CLAUDE.md`; undo covers a mistyped tag. +- A tag the user invents is new to the completer, and `onTagsApplied()` already + refreshes the list when a mutation introduces an unknown tag, so that path is + in place and should be relied on rather than duplicated. + +### Outcome (done) + +Built as the user chose: one **Edit tags** dialog on `Ctrl+T` rather than +separate add and remove actions, since filing something under a new tag while +dropping `inbox` is one thought. + +`TagDialog` is pure UI in `qtmaildir_lib`. It is handed the vocabulary and the +selection's current tags and returns two lists; it contacts no worker and holds +no database handle, which is what lets fifteen tests run without a notmuch +database. Integration is one call to the existing `tagSelected()`, so undo, the +optimistic model update, the one-query multi-row resolution and the completer +refresh all come for free. + +**Tri-state is the part that needed the tests.** With several threads selected a +tag can be on some, and `PartiallyChecked` means "leave alone" rather than +"apply to all". The opposite reading silently tags threads the user never +looked at. `m_fullyTagged` exists for the neighbouring case: a tag already on +every thread and left checked is not a change and must not be sent as one. + +**Validation is a free function** so the rules are testable directly. It rejects +empty, a leading `-` (notmuch's CLI reads that as removal, so such a tag is a +trap), whitespace, and control characters. Nothing is applied until every name +passes, since a half-applied change is worse than none: the user cannot tell +which half landed. + +**One test assumption was wrong and the code was right.** A case asserted that +`QStringLiteral("null\0byte")` truncates at the null and reads as Empty. It +does not; the literal is kept whole, so the null is caught as a control +character. The test was corrected, not the validator. + +Rendered and inspected rather than only asserted. + +## 27. The UI cannot see a sync it did not start + +**Observed (user, 2026-08-04), as a question:** "will the status bar be aware of +cronjob fired syncs?" It is not. The progress bar and the busy state are driven +by `MailSync`'s own `QProcess`, so they know only about syncs this window +started. The user's cron timer fires every ten minutes and the application is +blind to it. + +**The lock file is already the signal; no new file is needed.** `mailsync.sh` +does `exec 200>"$LOCKFILE"` and then `flock -n 200` on `/tmp/mbsync.lock`. +Another process can test whether that lock is held, without disturbing it, by +opening the same path on its own descriptor and attempting `flock(LOCK_EX | +LOCK_NB)`: the attempt fails exactly when someone holds it, and succeeds +otherwise, in which case the tester drops it again immediately. + +This is better than the status file first proposed. A kernel lock **cannot go +stale**: it is released when the holding process dies, however it dies. A status +file written by the script survives a `kill -9` and would leave the UI claiming +a sync is running forever, which then needs a heartbeat and a staleness +timeout, none of which the lock needs. + +**Decided (user, 2026-08-04): poll continuously, not only while quitting.** A +narrower version that polled only during the exit prompt was offered and +declined: the user wants the status bar informed at all times, not only at the +one moment it changes a decision. + +**Approach.** + +- A `syncLockHeld()` helper: `open()` the lock path, `flock(LOCK_EX|LOCK_NB)`, + close. Held when the attempt fails with `EWOULDBLOCK`. +- A `QTimer` polling it. **One second is finer than this needs**; a sync runs + for tens of seconds, so two seconds is plenty and halves a wakeup that never + stops. +- When the lock is held and `MailSync` is NOT the holder, show the busy state + with wording that says so: "Syncing (started elsewhere)". The existing + `setSyncBusy()` covers the widgets; this adds a third state between "idle" + and "we are syncing". +- On the exit path, this replaces a hedge with a fact. Today, quitting while + cron syncs says the window "cannot see it finish". With the lock watched it + can wait for the lock to clear and then quit, which is what the user actually + wants to happen. + +**Verify before building, both cheap and both the kind of assumption this +project has been bitten by twice:** + +- **That the lock is observable at all.** `flock` semantics across processes, + where the holder opened the file with `exec 200>`, are an assumption about + Linux behaviour, not a certainty. A twenty-line probe settles it. Do not + design on top of it unproven. +- **That polling it cannot disturb notmuch.** `mailsync.sh` runs `notmuch new`, + and notmuch's write lock is process-exclusive. Touching `/tmp/mbsync.lock` + should be unrelated, but the interaction is worth one look rather than an + assumption. + +**Constraints.** + +- The lock path becomes a contract between the script and the application, + where today the application knows nothing about it. It has to be documented + on both sides, and the script cannot move or rename it casually afterwards. +- The poll must not run a query or touch the database. It reports what another + process is doing; the existing `runCurrentQuery()` on a completed sync of our + own already handles refreshing, and a cron sync that finishes will be picked + up the next time the user runs a query. +- Do not report an externally-started sync as one this window can cancel. The + Sync button should be disabled while the lock is held, since starting one + would only produce the `EX_TEMPFAIL` skip. + +### Outcome (done, 0.8.0) + +Both probes the item demanded were run before any code, and one of them changed +the design. **The lock is observable, but only through `/proc/locks`**, and two +of the three plausible ways to read it are wrong: + +- **`flock -n`, which this item proposed, is the one that must not be used.** + It acquires in order to test, so polling every two seconds opens a window + every two seconds in which a starting `mailsync.sh` is refused the lock and + exits 75. It would cause the very skips the script reports. The item's own + approach section specified this; it was wrong. +- `fcntl(F_OFD_GETLK)` never acquires and looks ideal, but reports UNLOCKED + against a lock held by `flock(2)`: separate lock namespaces in the kernel, + which cannot see each other. A silent false negative. +- `/proc/locks` is a pure read, matched by inode. It observes `flock(2)` + correctly and takes no lock, which also answers the second probe: 200 reads + left the lock table unchanged and the polling process holding nothing, so it + cannot contend with the Xapian write lock `notmuch new` holds in the same run. + +`SyncMonitor` keeps the parsing separate from the polling so the parsing is +testable, and reports **Unknown** rather than Idle where `/proc/locks` cannot be +read. "No sync is running" is the claim that would let the window quit, so it is +never guessed. + +**It reports rather than refreshes**, which is narrower than the item implies. +`runCurrentQuery()` clears the undo stack, the selection and the message pane: +right for a query the user typed, hostile for one a cron timer fired six times +an hour. The status bar says a background sync finished and suggests pressing +Enter. See item 35 for the non-destructive refresh this defers. + +**One constraint above was not built: the Sync button is still enabled while an +external sync holds the lock.** That is item 29, split out rather than left +buried here. + +**A defect found by hand testing, after the feature was believed done.** A sync +this window started reported itself as a background one, a moment after it +finished. Ownership of a lock period was decided when the lock was RELEASED, by +asking `MailSync::isRunning()`, but the process exits before the next poll sees +the lock gone, so the answer was always "not ours". Ownership is now latched +when the lock appears, and handed back on exit 75, since a skip means the lock +was never ours. + +**That fix has no regression test, deliberately recorded.** Reproducing it needs +`isRunning()` true at one transition and false at the next, which needs a +configured sync command and a live child process; that attempt left a process +running for the length of the suite and popped a dialog on the user's screen. It +was verified against a standalone model of both code paths instead. A real test +needs the notmuch fixture. + +--- + +## 28. Re-adding `unread` counts 2 unsynced changes, not 0 + +**Observed (user, 2026-08-04):** open a thread, wait the two seconds for the +automatic mark-read, then press **Ctrl+U** to put `unread` back. The indicator +reads **2 unsynced changes**. The mail store is in exactly the state it started +in, so the honest answer is 0. + +**Cause: the counter counts writes, not net state.** `onTagsApplied()` does a +bare `++m_pendingEdits` (`src/mainwindow.cpp:1293`) for every confirmed +mutation, and nothing ever decrements. The automatic mark-read is one confirmed +write, the manual toggle back is a second, so a round trip that cancels itself +out reads as two outstanding changes. + +This is not specific to mark-read. Any add-then-remove of the same tag inflates +it the same way: archive then undo, flag then unflag. Mark-read is simply the +one path that fires without the user asking, which is why it surfaced here. + +**The design question this item has to answer first.** "Unsynced changes" can +mean two different things and the fix depends on which: + +- *Writes the index has taken that a sync has not carried over.* Two writes did + happen, notmuch's database did change twice, and `notmuch new` will still + visit those messages. On this reading 2 is correct and the wording is what is + wrong. +- *Net difference between the index and the mail store.* On this reading the + answer is 0 and the counter needs to track state, not events. + +The user's expectation is clearly the second. Note that item 18 chose the first +deliberately: it counts at the point a write is **confirmed**, precisely so an +optimistic update the worker rejects is not counted. That reasoning stays valid; +what it did not consider is a write that undoes an earlier one. + +**Approach, if net state is the answer.** Track the set of (thread, tag) +changes rather than a count, and collapse a pair that cancels. `TagChange` +already carries add and remove lists and already knows how to invert itself +(`TagChange::inverted()`, used by the undo stack), so the machinery for +recognising an inverse exists. + +**Constraints.** + +- **A sync must still reset it to zero**, and a failed sync must still not, per + item 18. Whatever replaces the counter keeps both properties. +- The indicator is a lower bound on confidence, not a guarantee: an external + `notmuch new` can carry changes across without this window knowing. Do not let + a more precise counter imply more certainty than it has. +- Do not fix this by not counting the automatic mark-read. It is a real write to + the index, and hiding it would make the count wrong in the other direction. + +### Outcome (done) + +**Decided by the user, 2026-08-04: net state.** "If I undo delete it's 0 edits, +not 2." The counter is replaced by a `QHash<QString, bool>` keyed +`"<messageId>\n<tag>"`, and a pair that reverts is **erased** rather than stored +with the new direction, so an edit and its inverse leave nothing behind and the +map cannot grow without bound over a long session of tagging and untagging. + +**Keyed per (message, tag), not per message.** Removing `unread` and adding +`flagged` on one message are two independent changes; a per-message key would +have cancelled them against each other. A test pins this, and it passed before +the change, so it exists to stop a later simplification from over-netting. + +**A change carrying no message ids still counts**, tracked in a separate +`m_unnettablePendingEdits`. It cannot be netted against anything, and dropping +it would understate the indicator, which is the direction that costs the user +work. This also keeps the older tests honest: they emit a `TagChange` with no +ids, and would otherwise have started reporting zero. + +Both properties item 18 established survive: a successful sync clears everything, +a failed one clears nothing. + +## 29. Sync button stays enabled during a background sync + +**Observed (user, 2026-08-04):** while a cron sync runs, the Sync button is +still clickable. Pressing it starts a run that can only be refused. + +**Cause: a constraint of item 27 that was specified and then not built.** That +item states plainly that "the Sync button should be disabled while the lock is +held, since starting one would only produce the `EX_TEMPFAIL` skip". +`setSyncBusy()` (`src/mainwindow.cpp:1357`) is the only thing that touches +`m_syncButton->setEnabled()`, and it is called only from this window's own sync +path. `onExternalSyncStateChanged()` shows the progress bar and writes the +status text but never touches the button. + +**Approach.** Have the external handler drive the same enable/disable that +`setSyncBusy()` does, rather than duplicating the rule. The two paths already +share the progress bar; the button is the piece that was missed. + +**Constraints.** + +- **Re-enable on `Unknown`, not only on `Idle`.** Where `/proc/locks` cannot be + read the monitor claims nothing, and a button left permanently disabled on a + platform that cannot observe the lock is worse than one that occasionally + offers a run that gets skipped. +- The button must not end up enabled during this window's own sync because the + external handler ran last. Both paths write the same widget, so whichever + fires second wins; make the rule a single function of both states rather than + two independent assignments. +- Exit 75 handling stays. Disabling the button makes the skip rarer, not + impossible: cron can take the lock between the poll and the click. + +### Outcome (done) + +`updateSyncControls()` is the single function the constraint called for, taking +`m_localSyncBusy` and `m_externalSyncBusy` and writing both the progress bar and +the button. Neither sync path touches those widgets directly any more. + +**The external state is tracked as its own flag rather than read back from +`SyncMonitor`.** An earlier version called `m_syncMonitor->state()` inside the +update, which meant the handler received a state and then ignored it in favour +of re-reading the source. Acting on what you were told is both easier to follow +and testable without a live monitor. + +Unknown clears the busy flag exactly as Idle does, per the constraint. Verified +by reverting that half: leaving it set on anything but Idle strands the button +disabled, and the test catches it. + +## 30. The blank right pane is wasted space + +**Observed (user, 2026-08-04):** with no thread selected the message pane is +empty. The user wants it to carry something useful, and named three things: the +application logo, tips and tricks, and messages about the current action, giving +"N messages selected" as the example. + +**Assets exist.** The user has produced `assets/images/qtmaildir-light.png` and +`qtmaildir-dark.png` (924x540 each) for this item. + +**Cause:** `MessageView::clear()` leaves the web view showing nothing. Since +0.8.0 the pane is also blanked deliberately whenever more than one thread is +selected, which makes this item more visible than it was: multi-select is now a +routine gesture that produces an empty pane every time. + +**Approach.** A placeholder document rendered into the existing web view when +there is nothing to show, rather than a second widget stacked behind it. +`HtmlBuilder` already produces the pane's HTML and `MessageView` already knows +how to hand it a document, so the placeholder is a third document shape +alongside "one message" and "a thread". + +**The selection-count half is the one with real value**, and it pairs with the +status-bar count added in 0.8.0. The pane already blanks on a multi-row +selection; saying "3 threads selected" there answers the question the blank +raises. + +**Constraints.** + +- **Two images, because the pane must follow the desktop theme.** Item 5 already + records the rule against hardcoded colours, and a light-theme logo on a dark + desktop is exactly that fault in image form. Pick by palette, not by guessing. +- **Do not load the logo over `file:`.** The web view runs with + `LocalContentCanAccessFileUrls` false and an interceptor that blocks every + request by default, per `CLAUDE.md`. The image has to arrive as a `data:` URI + or through the existing `qtmaildir:` scheme handler, and the interceptor's + document-URL exemption must still match exactly. +- Tips and tricks is the weakest of the three and should be built last, if at + all. A tip nobody can dismiss becomes noise on the hundredth launch. +- The placeholder must not appear between a selection and its render, or every + thread open flashes a logo first. + +### Specified with the user 2026-08-07; fonts committed, pane not built + +**The design source is the user's own HTML mockup**, not the PNGs recorded +above. Those were rendered *from* it. Both mockups are in the user's Downloads +as `qtMailDir Background {dark,light} Mockup.zip`, each holding +`qtmaildir-mockup-bg.html` and a `COLOR-REFERENCE.md` giving the complete brand +palette for both modes. Build from that HTML; do not trace the PNGs. + +**Two things in the mockup cannot survive the port, by design.** Its +`@import` of Google Fonts is blocked by the interceptor, deliberately, and its +`fit()` script cannot run because JavaScript is off in this profile. The fonts +are dealt with (below); the scaling script is unnecessary, since CSS can centre +the block without it. + +**Decided with the user:** + +- **The brand palette wins over the desktop palette**, a deliberate exception + to item 12's rule. A logo is brand rather than chrome. The dark or light set + is chosen by the desktop theme, so it still flips correctly. +- **Layout is a header ROW, not the mockup's centred lockup**: icon on the + left, wordmark on the right, with the glow and grid still centred behind as + background. Helpers below the header. +- **No fixed vertical split.** The user first suggested 40/60; this pane is a + splitter panel whose height varies enormously, so a ratio breaks at one + extreme or the other. The block takes its natural height and is centred in + whatever the pane gives. +- **The helpers are counts and sync state**, NOT the selected-thread count the + section above proposes. The user rejected that: the status bar already says + it. Instead: unread, flagged and inbox counts, each clickable to run that + query, and a sync line that appears only when something needs attention + (last sync failed, or edits waiting to sync). Nothing when all is well, so it + cannot become wallpaper. +- **Footer**: copyright, version, and the website as a WORKING link. Clicking + hands off to the desktop browser through the existing + `NavigationTypeLinkClicked` path in `messageview.cpp`. +- **Tips and tricks stays dropped**, as the section above already argues. +- **The AI-assisted development notice does not go here.** It belongs in the + README, per the user's global preference, and in the About dialog if wanted. + +**Already done (commit "build: bundle the fonts…"):** Oxanium ExtraBold and +IBM Plex Sans Regular are in `assets/fonts/`, subset, with their OFL licences, +and the README records them. Nothing references them yet. + +**Still to build:** `resources.qrc` entries; +`HtmlBuilder::buildPlaceholder(...)` porting the mockup's CSS with the fonts as +`@font-face` data URIs; `MessageView::showPlaceholder(...)`; and the four +`MessageView::clear()` call sites choosing between the placeholder and a blank +pane. The **helper counts are the largest piece**: three `notmuch count` calls +crossing to the worker, plus a refresh policy, since a count goes stale the +moment a tag is edited. + +### Outcome (done 2026-08-07) + +Built as specified, and confirmed against the running application in both +themes. Decisions taken while building, and three defects worth recording. + +**The counts refresh when the pane is about to show**, chosen by the user over +refreshing on the tag-list triggers or only after a sync. `showPlaceholderPane()` +is the single route to a blank pane, so the numbers are fetched exactly when +they are about to be read and never in the background. A generation counter +discards a superseded reply, and `onCountsReady` repaints only while the +placeholder is still displayed, so a late answer cannot replace an opened +thread. Counts are of THREADS, matching what a click on the line produces. + +**The helper lines are real links**, since JavaScript is off in this profile and +a count cannot be clickable any other way. They carry a `qtmaildir-query:` URL +caught in `acceptNavigationRequest`, and **the handler is gated on the +placeholder actually being displayed**. A message body is attacker-controlled +HTML and can carry the same URL; without the gate a link in a stranger's mail +could drive the thread list. The consequence would be mild (a query runs, +nothing mutates or is sent) but it is a boundary worth keeping shut. +`showThread`, `clear` and `showError` all close the gate, and a test asserts it. + +**Sizes are clamped, not fixed and not fluid**, per the user's "a mix of both". +The pane is a splitter panel whose width runs from a couple of hundred pixels to +most of a screen. Measured: the wordmark renders 26px in a 300px pane and 57.6px +in a 900px one, with `bodyScrollW == clientW` at every width tested. + +#### Three defects, all invisible + +1. **Every CSS percentage was invalid, and the pane still looked plausible.** + The stylesheet was built with `QString::arg` and `%%` for each percentage, + but **`arg()` does not collapse `%%` into `%`**: the document reached the + browser carrying `50%%`, and every declaration containing one was dropped. + That silently disabled the grid mask, the glow and both radial gradients. + The pane rendered, and the flat result read as "close but not like the + mockup" rather than as a fault. Fixed by substituting **named tokens** + (`@ACCENT@`, `@GRID@`) with `replace()`, which cannot collide with a percent + sign. `placeholderStyleHasNoUnsubstitutedTokens` pins it. + +2. **A geometry probe confirmed the layout while that bug was live.** It + measured `.title`, `.content` and `.icon-tile`, none of which carry a + percentage, so it reported everything correct. This is the failure mode + `CLAUDE.md` already warns about, in a new costume: the probe found what it + looked for and was trusted to report what it never checked. **A probe over a + stylesheet must assert the properties that the suspected fault would break**, + not the ones that happen to be convenient to measure. + +3. **The font test passed against a broken build.** Pointing one `@font-face` + at a nonexistent resource survived it: the other face alone satisfied both + the `data:font/woff2` check and the document-size check. Rewritten to require + both faces with a payload each, and verified by mutation. + +#### The mockup's light values do not survive a real pane + +Only the dark set ported cleanly. Rendered side by side, three light values +failed, all of them contrast rather than hue, because the mockup is a full-bleed +1920x1080 render and this is a pane against white: + +- The grid at `#d9dfe8` on white is roughly a 2% luminance step and vanished + outright, where `#182840` on `#060b10` reads clearly at the same opacity. +- The glow **subtracts** light on a light background instead of adding it, so + 14% washed most of the pane purple. +- The tile at `#f0f3f7` inside a `#d9dfe8` border did not separate from the + background, leaving the icon floating. + +`glowAlpha` and `gridOpacity` are therefore per-set rather than shared, and +`tileBorder` is its own colour rather than reusing `grid`. Landed at +`#b9c4d4` @ 45% for the grid, 6% for the glow, `#c7d0dd` for the tile border. +The dark set is unchanged. + +**The mask and the glow are sized relative to the pane**, the one deliberate +departure from the mockup's numbers. Its `circle` (farthest-corner) mask +completes its fade inside a 1920x1080 frame; the same ratio in a ~990x650 pane +puts the fade past the corners, so the grid ran uniform to the edges. +`closest-side` pins it to the nearer edge, and the glow is `min(95%, 900px)` +rather than a flat 900px that was taller than a short pane. + +**`resources.qrc` is now compiled into every test binary.** Library code reads +`:/fonts/` and a qrc inside the static library registers from a global +initialiser the linker drops, so without this a test would exercise only the +missing-resource fallback and pass against a broken build. + +## 31. The quit prompt has no highlighted default button + +**Observed (user, 2026-08-04):** "quit popup has no Predefined answer (there's +no highlighted button)." + +**Needs a repro before any change: the code says otherwise.** The three-button +prompt sets one explicitly, `box.setDefaultButton(sync)` +(`src/mainwindow.cpp:191`), and the no-sync-command variant passes +`QMessageBox::Cancel` as its default argument (`:167`). Both should render a +highlighted button. + +Possible explanations, in the order worth checking: + +- The dialog the user saw was neither of those. There are five other + `QMessageBox` calls in `MainWindow` (`:205`, `:224`, `:991`, `:1260`, + `:1277`), and the two sync-failure ones appear on the exit path, so a failed + exit-sync shows a second dialog immediately after the first. +- The default is set but the style draws no visible focus ring, which is a + platform theme question rather than a code one. +- `setDefaultButton()` is being overridden by the button roles: a + `DestructiveRole` button can take precedence in some styles. + +**Approach: reproduce first, and record which dialog.** If it is one of the +sync-failure boxes, the fix is to give those an explicit default. If it is a +theme issue the item becomes a documentation note rather than a change. + +**Constraint:** whatever default is chosen must be the safe one. On a prompt +about losing unsynced work, Enter must not fall on "Quit anyway". + +### Outcome (done): the code was right, the theme draws nothing + +Reproduced from a screenshot: it is the three-button `ask` prompt. Probed rather +than guessed, and Qt agrees the default is set. On that dialog `isDefault()` and +`hasFocus()` are both true on "Sync and quit", and `defaultButton()` returns it. + +**The active style is `qt6ct-style`, which draws no visible default-button +decoration.** The GIMP dialog the user compared against is GTK drawing its own +focus ring, a different toolkit, so the two are not comparable and this was never +a qtmaildir bug. + +Fixed by naming the default in the button text rather than restyling it. +Overriding a button's appearance means fighting the user's chosen theme, which +is a worse outcome than one word. The safe option was already the default, per +the constraint above, so no behaviour changed. + +## 32. Esc does not blank the right pane + +**Observed (user, 2026-08-04):** "Esc in the main window should blank the right +pane." + +**Cause:** nothing binds Escape at window level. `Key_Escape` appears once in +`src/`, in `QueryCompleter` (`src/querycompleter.cpp:474`), where it dismisses +the completion popup. The main window has no handler. + +**Approach.** A registered action like any other, so it reaches the menus, the +shortcut reference and `[keys]`, calling the same `MessageView::clear()` the +multi-select path already uses. `m_currentThreadId` must be cleared with it, or +a late-arriving `threadLoaded` will paint the thread straight back, which is the +race documented in `CLAUDE.md` and fixed in 0.8.0. + +**Constraints.** + +- **Escape must not be stolen from the completer.** A window-level shortcut + outranks the focused widget, which is exactly how `Return` broke for the query + bar and needed an event filter to claim back (item 21 records this). Verify + the popup still dismisses before shipping. +- Blanking is a view change, not a mail change: it must not clear the selection, + the query, or the undo stack. +- Decide what Escape does when the pane is already blank. Doing nothing is fine; + clearing the selection as a second step would be surprising. + +### Outcome (done) + +A `clear_pane` action bound to `Esc`, registered like every other action so it +reaches the menus, the shortcut reference and `[keys]`. It clears +`m_currentThreadId` alongside the pane, and cancels any pending mark-read: a +thread blanked from view must not be marked read two seconds later. + +**The completer keeps its Escape, verified by probe rather than by reading the +code.** A popup consumes the key before a window-level shortcut sees it: with +the popup open the popup's filter fires and the action does not, and with it +closed the action fires. This was the one real risk in the item, since `Return` +had already been lost to a window shortcut this way (item 21). + +Blanking is a view change only: the selection, the query and the undo stack are +untouched, which a test pins. + +## 33. Status bar messages never expire + +**Observed (user, 2026-08-04):** "the status bar should return to default status +after showing a message for N seconds." + +**Cause:** every message is written with a bare `m_statusLabel->setText()`, +eighteen call sites in `MainWindow`, and nothing ever clears one. Whatever was +written last stays until something else overwrites it, so a transient message +like "Sync complete" persists as though it described the current state. + +**Approach.** Route transient messages through one helper that sets the text and +arms a single-shot timer to restore a default, rather than adding a timer per +call site. `QStatusBar::showMessage()` already implements exactly this with a +timeout argument, and the label is a custom widget added with `addWidget()` +rather than the status bar's own message area, so switching to it is worth +considering before writing a bespoke timer. + +**The item's real question is what "default" means.** Candidates: the thread +count from the last query (`onQueryFinished` already writes this), or empty. +The count is more useful and is what the user already sees after a query +completes. + +**Constraints.** + +- **Not every message is transient.** The selection count added in 0.8.0 + describes current state and must persist while the selection does; expiring it + would be a regression. Distinguish state from events rather than putting a + timeout on everything. +- The 0.8.0 selection-count code already takes back only a message it wrote + itself, comparing against `m_selectionMessage`. Any general mechanism should + generalise that rather than defeat it. +- An error must not vanish before it is read. Sync failures already open the log + pane, which does persist, but the status text should outlast a two-second + timeout. + +### Outcome (done) + +`showTransientStatus()` sets the text and arms a 6 s single-shot timer that +restores the last query's thread count. Messages were classified rather than +blanket-timed, which is the whole substance of the item: + +- **Events expire:** "Sync complete", "Nothing to undo", "Sync already + running", the skip notice, the background-sync notice, and the per-action + "Archive: 3 threads". +- **State persists:** "Searching...", "Syncing...", "Syncing before + quitting...", "Background sync running...", the selection count, and + **"Sync failed (exit N)"**, per the constraint that an error must not vanish + before it is read. + +**A test caught a real mistake while routing them.** Making the per-action +message transient armed the timer during `selectAll()`, because `tagSelected()` +runs on a selection that `onSelectionChanged()` had just described. The count is +state and must outlive any transient still counting down, so writing it now +cancels the timer. + +`QStatusBar::showMessage()` was considered and not used: the label is added with +`addWidget()` alongside permanent widgets, so switching would mean reworking that +arrangement for the same behaviour. + +## 34. No overview of the Maildir itself + +**Observed (user, 2026-08-04):** wants "info on the maildir": total messages, +number of accounts, and possibly more. + +**Cause:** nothing in the UI reports database-level facts. Every query returns a +thread count for that query (`onQueryFinished`), but there is no path that asks +notmuch about the database as a whole. + +**Approach.** A dialog, reached from Help or File, showing what notmuch can +answer cheaply. `notmuch_database_get_all_tags()` is already wired for the +completer (item 17), so the tag count is free. A total message count needs a new +worker call. + +**Constraints.** + +- **This is a worker query like any other.** No `notmuch_*` pointer crosses the + thread boundary; the result comes back as plain values, per `CLAUDE.md`. +- **The account count comes from config, not from notmuch.** notmuch does not + model accounts at all, which is why per-account subdirectories are configured + in the first place. Do not try to derive it from the database. +- Counting every message in a large database is not free. Measure before putting + it somewhere that opens on every launch; a dialog the user asks for is the + right shape, a status-bar field refreshed continuously is not. + +### Revisited 2026-08-07, after item 30 shipped + +The placeholder pane did not exist when this was written, and it now displays +database-level counts, so "where does this live" was worth asking again. **The +answer is unchanged: a dialog.** Recorded because the reasoning is not obvious +and would otherwise be re-litigated. + +**Not on the placeholder**, even though the counts machinery is right there. +Item 30's own decision was that the pane carries counts the user ACTS on, each +one a link to the query it names, plus a sync line that appears only when +something needs attention, specifically so the pane cannot become wallpaper. +Total messages, account count and tag count are reference material read once, +not things to click. Putting them there dilutes exactly what that decision +protects. + +**What did change is the cost, not the location.** This item claims a total +message count "needs a new worker call". It no longer does, quite: +`NotmuchWorker::requestCounts(QStringList, quint64)` already exists, crosses on +a queued connection and takes an arbitrary list of queries, so the dialog can +ask for whatever it wants in one round trip. + +**One trap in reusing it.** `requestCounts` counts **threads** +(`notmuch_query_count_threads`), because item 30's pane says "N in inbox" beside +a list that shows threads. This item wants **messages**. Those differ by roughly +the reply depth of the database and the difference is not small. Reusing the +call as it stands would report a confidently wrong number under the right +label. Either add a second signal, or give the existing request a mode; do not +quietly reinterpret what it returns. + +**Still true from the constraints above:** the account count comes from config, +never from notmuch, and the tag list is already fetched for the completer. + +### Outcome (done 2026-08-07) + +A dialog under Help, as decided. `requestDatabaseStats` answers messages, +threads and tags in one round trip; the account list comes from `Config`. + +**A separate worker call, not a reuse of `requestCounts`**, exactly as the +revision above warned. That one counts THREADS to match the row count of a +query; this one counts MESSAGES, which is what a user means by "how much mail +is in here". The test asserts 4 messages in 3 threads against the fixture and +fails if they are ever made equal, so a later "simplification" that routes both +through one count cannot pass. + +**Unknown is not zero.** Every field starts at -1, and a field notmuch could not +answer renders as "unknown". Printing 0 would say the Maildir is empty, which is +a claim, and telling someone their mail is gone is the worst available way to +report an index that failed to open. Verified by mutation: removing the guard +puts a literal `-1` on screen. + +**The dialog opens before the answer arrives**, showing "Counting...". Counting +every message is not free on a large database, and a dialog that blocks first is +worse than one that fills in. + +Two lifetime problems follow from that, both handled and both tested: + +- The reply can arrive after the dialog is closed. The label is held in a + `QPointer`, since `WA_DeleteOnClose` means a raw pointer dangles for exactly + as long as the count takes, which is when the user is most likely to have + given up and closed it. The test drains `DeferredDelete` before firing the + late reply, because `close()` deletes through `deleteLater` and without the + drain the case being tested is not the one that occurs. +- The dialog can be closed and reopened while a count runs, so a generation + counter drops the older answer rather than filling in the newer dialog with + numbers that predate the reopen. + +## 35. No refresh of the thread list after a sync + +**Observed (user, 2026-08-04):** "auto refresh list after sync." + +**Cause: half of this works and the other half was deliberately not built.** A +sync this window starts already refreshes: `onSyncFinished()` calls +`runCurrentQuery()` on success. A background sync does not, and 0.8.0 chose that +on purpose, because `runCurrentQuery()` clears the undo stack +(`src/mainwindow.cpp`, the query-start path), the selection and the message +pane. Firing it when a cron timer finishes would discard undo history and close +the thread being read, up to six times an hour, with no action from the user. +The status bar suggests pressing Enter instead. + +**So the work is a non-destructive refresh**, not a call to the existing one. +That is a real piece of work and is why this is its own item rather than a flag. + +**Approach.** Re-run the query and reconcile the result against the current +model instead of clearing it: keep rows that are still present, add new ones, +remove the gone. The generation counter already distinguishes a stale result +from a current one, so the plumbing for a second concurrent query exists. + +**Constraints.** + +- **The undo stack is cleared on query for a real reason.** Its entries refer to + rows the new result set discards, and a stale entry inverts into a model + update that does nothing while the database change still happens, leaving undo + half-applied. A refresh that keeps the stack has to keep those references + valid, which is the hard part of this item and the reason it is sized M. +- The selected thread must stay selected and stay open if it survives the + refresh. Losing your place is the failure this item exists to avoid. +- Scroll position likewise. +- A refresh must not re-trigger mark-read for the thread already on screen. + +### Refined by the user, 2026-08-10: new mail never appears at all + +The complaint is sharper than "the list is stale". Read every message in an +Unread view and the list empties. The cron sync then runs, `notmuch new` indexes +new mail, and **the new messages do not appear**, even though the view is empty, +nothing is selected, no thread is open and the undo stack has nothing in it. The +only way to see them is to re-run the query by hand. + +**Cause, verified in code, and it is the deliberate choice above meeting its +worst case.** The app does observe the cron sync: +`MainWindow::onExternalSyncStateChanged()` (`src/mainwindow.cpp:2140`) is driven +by `SyncMonitor`'s lock polling and reaches `State::Idle` when the run ends. At +`src/mainwindow.cpp:2180-2192` it deliberately shows "Background sync completed. +Press Enter in the query bar to refresh." and calls nothing. The comment there +gives the reason: `runCurrentQuery()` clears the undo stack, the selection and +the message pane, which is hostile to fire six times an hour under a reader. + +**Every one of those costs is zero in the case the user hit.** There is nothing +to clear: no undo entries, no selection, no open thread, and the scroll position +of an empty list is meaningless. So the guard is protecting state that does not +exist, and the result is an empty Unread view sitting in front of unread mail. + +**This makes the item shippable in two stages, and the first is XS.** Refresh on +external sync completion when the refresh is provably free: the model is empty +**or** the undo stack is empty and nothing is selected. Fall back to the current +status-bar message otherwise. That fixes the reported case immediately without +needing the reconciling refresh, which stays as the M-sized second stage for the +case where the user does have state worth keeping. + +**Constraint on the cheap stage.** "Nothing selected" must be read from the +selection model, not from `currentRowChanged` state, per `CLAUDE.md`. And the +condition has to be re-checked at the moment the sync ends rather than when it +started, since the user may have selected a row during the run. + +### Outcome (done, 2026-08-10) + +**35a was superseded before it shipped, by the user's own answer to it.** Asked +whether new mail would accumulate into a POPULATED view, the answer was no, and +the requirement was restated in full: "changes should be applied automatically, +without the user needing to refresh the view, whether it's empty or populated", +new mail appearing at the top as it is fetched, and the message being read not +disappearing. So the conditional refresh was removed rather than kept beside the +reconciling one, and `externalRefreshIsFree()` no longer exists. The "press +Enter in the query bar to refresh" message is gone with it: a refresh that +changes nothing is invisible, and one that brings mail announces itself by the +mail appearing. + +**The undo constraint this item was sized around turned out not to exist.** The +text above calls keeping the undo stack "the hard part" and the reason this is +M-sized. It is not hard, because no undo entry was ever keyed on a row: +`ThreadTagCommand` stores THREAD ids and `MessageTagCommand` stores MESSAGE ids +(`src/mainwindow.h`), both re-sending through `sendThreadTagChange()` / +`sendMessageTagChange()`, and `ThreadListModel::applyTagChange()` looks its +target up by id and does nothing when the row is absent. An entry therefore +already survives its rows leaving the view. `runCurrentQuery()` clears the stack +because a query the USER typed means "show me something else", not because the +entries would corrupt anything. + +**What was built.** + +- `ThreadListModel::reconcile()` diffs a result against the current rows by + thread id: arrivals inserted, departures removed, survivors keeping their row, + their persistent index and their loaded replies. Order comes from the result, + never from a rule of the model's own, so the sort the user selected is + respected: newest-first puts new mail at the top, oldest-first at the bottom. +- `MainWindow::refreshCurrentQuery()` re-runs `m_lastQuery` under its own + generation, accumulates every batch, and reconciles ONCE at the end. Batch by + batch would be wrong in a way that looks right: reconcile decides removals + from what the result lacks, so the first batch would delete every row after it + and the next would put some back. +- `onExternalSyncStateChanged()` calls it unconditionally on `Idle`. +- A stale-thread notice in `MessageView`, shaped like the remote-content bar as + the user asked, with recovery that runs `thread:<id>`, expands it and + re-selects the message that was on screen. + +**Four defects found by the tests, three of them real.** + +1. **The first test crashed the constructor.** `SyncMonitor::start()` polls + SYNCHRONOUSLY (`src/syncmonitor.cpp:52`), so on an idle lock file it emits + `stateChanged(Idle)` from inside `buildUi()`, while the view, the model and + the worker are all still null. The old code survived only because reporting + to the status bar touches nothing built later; the first handler to + dereference a widget segfaults before the window exists. +2. **A downward-move branch that could never run.** `reconcile()` walks the + result front to back, so rows ahead of the target are already final and a + misplaced survivor is always pulled FORWARD. The branch was written with the + usual `beginMoveRows` +1 adjustment and two mutation tests passed against it + being wrong, which is the signal that a probe is not measuring what it + claims. Deleted, with `Q_ASSERT(row > target)` recording the invariant. +3. **A user query hijacked by a pending recovery.** Recovery spans two queued + round-trips, so a query typed in the middle of one found its target in the + new result and moved the selection there. `runCurrentQuery()` now abandons a + pending recovery, and `recoverStaleThread()` sets its target after calling + it. +4. **The stale notice never fired for the reader deepest in a thread.** + Selecting a message row CLEARS `m_currentThreadId` and sets + `m_currentMessageId` instead, so a notice keyed on the thread alone was + silent for exactly the case the user described, reply four of eight. The + window remembers the message's thread separately; + `ThreadListModel::threadIdForMessage()` cannot help, since it searches the + rows and by then the thread has left them. + +**A fifth defect, found by the user in hand testing rather than by any test.** +The notice outlived the message it describes: running a new query blanked the +pane and left the bar above it, still naming the previous thread, with a button +offering to recover a thread the user had deliberately navigated away from. The +bar belongs to the rendered message exactly as the remote-content bar does, and +`MessageView::clear()` already hides that one for this precise reason; the new +bar simply was not added beside it. Fixed there, which covers all six paths that +blank the pane at once, rather than at the query path where it was noticed. + +Worth recording because the tests could not have caught it as written: every +one of them asserted that the notice APPEARS, and none that it goes away. A +feature's off-switch needs its own test, and "it shows up when it should" passes +identically whether or not it ever stops showing up. + +**A sixth defect, also found by the user in hand testing, and the same mistake +in a different place.** The status bar sat on "Background sync running..." with +no sync running. That string is written straight to the label when the lock +appears, and the "Background sync completed" message on the way out was the only +thing that ever replaced it; removing that message to make the refresh silent +left the claim standing indefinitely. + +The rule this establishes is worth more than the fix: **silent means saying +nothing NEW, not leaving a stale claim on screen.** The Idle branch now retires +its own running message and nothing else, tracked by a flag rather than by +matching the text, so it cannot overwrite a selection count or a tag result the +user is reading. Both directions are pinned by mutation: never retiring +reproduces the reported bug, and always writing the default stamps over the +selection message. + +Note the shape shared with the fifth defect above. Both are a piece of UI state +that outlived the thing it described, and in both cases the tests asserted only +that the state APPEARS. An "it goes away" test is a separate test. + +**A seventh and eighth defect, one report, and the worse of the two mutates +mail.** The user came back to the window from another desktop and found the new +message the refresh had brought in already OPEN in the pane, with the stale +notice above it still naming the four-message thread they had been reading. + +- **Nothing in `MainWindow` selected it.** `QTreeView` gives itself a current + index when it takes FOCUS with none set, and current is what drives loading. + Probed rather than assumed, because the obvious hypothesis is wrong: inserting + rows into an empty view does NOT set current, focusing the view does, which is + exactly why the report came with "as I go back to the window from another + desktop" attached. Before item 35b this was unreachable, since a populated + list always had a current row; a refresh dropping mail into a view the user + read empty created the state. The consequence is not cosmetic: opening a + message arms the mark-read timer, so a cron sync plus a window switch marked + mail read that nobody looked at. `onThreadSelected()` now requires the row to + be SELECTED, which every real route (click, arrow key, `selectRowAt`) does and + Qt's housekeeping does not. +- **The notice was correct when raised and became a lie underneath.** It named + the thread that was rendered; the auto-open then replaced the pane without + touching the bar. `MessageView::clear()` retires it, but selecting a row + RE-RENDERS rather than blanking, so that path never ran. Retired in + `onThreadSelected()` as well. + +The first of these was reported as one bug and is two, and only the second was +visible on screen. Worth remembering that "the wrong thing is displayed" and +"the wrong thing happened to the mail" can arrive in the same sentence. + +**A ninth defect: recovery brought the thread back collapsed and blank.** The +user reported it as minor and livable, and it was three faults stacked, each of +which alone would have produced roughly the symptom they saw. + +- **The notice threw away a message id it had.** A thread ROOT sets BOTH + `m_currentThreadId` and `m_currentMessageId`, because the root card is the + thread's first message and the pane renders exactly that message. The notice + read the message id only when the thread id was empty, treating it as the + message-row case, so opening a thread the ordinary way lost it and recovery + had nothing to reopen. +- **Recovery never expanded.** It selected the row and returned, so the + conversation the user asked to get back to was not on screen. It expands + first now, in every branch, which is also what asks the worker for the + replies. +- **A freshly queried root does not know its own first message either.** + `MessageIdRole` on a thread row returns `first.messageId`, which is empty + until the tree loads, so the root check could not match on the pass that + matters and the code fell through to `rowCount(thread) == 0` and returned, + selecting nothing. Recovery now selects the thread PROVISIONALLY on that + pass, without clearing the target, and refines to the exact reply when the + replies arrive. + +**One change here is not demonstrated and is recorded as such.** Recovery also +moved from `setCurrentIndex()` to `selectRowAt()`, on the reasoning that +`onThreadSelected()` ignores an unselected current index since the auto-open +fix. A mutation reverting it passes the whole suite: under +`ExtendedSelection`, `setCurrentIndex()` selects as a side effect, so the two +are indistinguishable here. It is kept as the honest expression of the intent, +not as a fix, and nothing should be claimed for it. + +**A tenth defect, and the only one in this item that six rounds of reasoning +failed to find: a dangling reference across a signal.** Recovery brought the +thread back collapsed with a blank pane. The user reported it three times, each +time after a fix that was aimed at the wrong thing. + +`MessageView` emitted `staleThreadRecoveryRequested(m_staleThreadId, +m_staleMessageId)`, passing its own members. The connection is direct, so +`MainWindow::recoverStaleThread()` received REFERENCES to those members. It then +called `runCurrentQuery()`, which blanks the pane, which calls `setStaleThread()` +and assigns to exactly those members. From that line onward the slot's own +parameters read as empty, so `m_recoverThreadId = threadId` stored an empty +string and `applyPendingRecovery()` returned at its first line, forever. The +thread was re-queried and expanded correctly, which is why the symptom looked +like a layout or expansion problem rather than a lifetime one. + +**Every existing recovery test passed against it, and could not have failed.** +They all reach the slot through `QMetaObject::invokeMethod`, which COPIES its +arguments; the reference never dangles under a test. The defect needed the real +button and the real signal, which is what the new test uses. + +**Six wrong mechanisms were proposed and rejected before the log named this +one**, each one plausible and each one disproved by a probe rather than by +argument: `QTreeView::expanded` not re-firing, expansion collapsing when +children arrive, the multi-row guard blanking the pane, `selectRowAt` not +clearing the previous selection, a stale `m_refreshGeneration` swallowing the +result, and `onQueryFinished` not running at all. The thing that ended it was +instrumenting the running application and reading `RECOVER target set to ` with +nothing after the `to`, which no amount of reading the code had produced. + +**The rule worth keeping: a Qt signal argument is a reference until something +copies it.** Emitting a member across a direct connection to a slot that can +reach back and modify that member is a use-after-write, and it presents as the +value being "wrong" rather than as a crash. Copy at the emit site when the slot +can plausibly re-enter the emitter. + +**A trap the recovery had to handle.** `setThreadMessages()` drops the depth-0 +message because the root card IS the thread's first message, so a reader +recovering from message one must land on the ROOT row. Looking for it among the +children finds nothing and leaves the selection nowhere. + +**Verification.** 105 tests in `test_mainwindow`, 56 in `test_threadlistmodel`, +17/17 binaries. Every reconcile test was mutation-checked: an unconditional +refresh, a skipped move, corrupted index bookkeeping (which trips +`QAbstractItemModelTester` fatally), a per-batch reconcile, a notice that never +fires and one that always fires are each caught by a named test. The +abandoned-recovery test initially passed for the wrong reason, because its +recovery never reached its target, and was rewritten until it failed against the +missing guard. + +**One measurement worth carrying to item 61.** During this work +`test_mainwindow` failed three runs in a row on the pair recorded there +(`anActionOnAMessageRowTagsThatMessageNotTheThread`, +`aSuccessfulCronSyncDrainsTheEditedAccounts`), then passed twelve consecutive +runs unchanged. The recorded rate is about one in twenty; a cluster of three +consecutive failures does not fit an independent one-in-twenty event and +suggests the trigger is a machine state that persists across runs rather than a +per-run race. + +### Outcome (35a, superseded by the above) + +`MainWindow::externalRefreshIsFree()` gates the `State::Idle` branch of +`onExternalSyncStateChanged()`: it refreshes when the undo stack is empty, the +selection model reports no selection, and the model holds no rows, and prints +the existing "press Enter" message otherwise. The M-sized reconciling refresh +(35b) is untouched and still open. + +**The test crashed the constructor, and the crash was real.** +`SyncMonitor::start()` polls SYNCHRONOUSLY (`src/syncmonitor.cpp:52`), so on a +machine whose lock file is idle it emits `stateChanged(Idle)` from inside +`buildUi()` (`src/mainwindow.cpp:535`), while `m_threadView` and `m_model` are +still null. The old code survived that only because reporting to the status bar +touches no widget built later; the first handler to dereference a view segfaults +before the window exists. `externalRefreshIsFree()` returns false on a null view +or model, which is also correct on the merits: the startup query has not run at +that point, so there is nothing to refresh. + +**The undo check is not redundant with the row check**, and that is asserted +rather than argued. Removing it alone leaves +`aCronSyncDoesNotRefreshOverPendingUndo` failing, because an empty model with +live undo entries is reachable by tagging the last thread out of the current +view. + +**Both guard tests passed before the fix existed**, since nothing refreshed at +all, so each was verified by mutation: an unconditional `return true` fails both, +and dropping the undo check fails the undo one. + +**One existing test changed, and the change is a narrowing.** +`aSkippedLocalSyncStillReportsTheOtherRunFinishing` asserted the words +"Background" in the status bar, and its fixture is an empty list with nothing +selected, so it now takes the refresh branch and the words never appear. Its +actual subject is that a handed-back lock is attributed to the other run rather +than swallowed, so it asserts the query generation instead. Pinning the wording +there would fail again the next time this decision is revisited. + +## 37. The worker stalls on a tag edit made during a background sync + +**Observed:** the user's note asks whether edits made while a background sync is +running are carried by that same job or need a manual sync afterwards. The +answer splits in two, and the second half is a defect rather than a question. + +**This entry was rewritten on 2026-08-04 after its original cause was +disproved by measurement.** It first claimed the read-write open *fails* during +a sync and the edit is discarded. It does not fail. That claim was written from +the plausible reading of the error path at `src/notmuchworker.cpp:298-305` +without ever provoking the condition, and a fix was built on it before anyone +checked. Recorded here rather than quietly corrected, because the same +false-cause-from-a-plausible-error-path mistake is cheap to repeat. + +**Cause, part one: reaching the disk is not the problem.** `applyTags` calls +`notmuch_message_tags_to_maildir_flags()` (`src/notmuchworker.cpp:327`) +immediately after thawing, so a `seen`/`flagged` change renames the file in the +Maildir at edit time. No manual sync is needed for the change to exist on disk. +Whether the *running* mbsync carries it is a matter of ordering: mbsync scans +each mailbox once per run, so an edit landing after that box was scanned goes +out on the next run. That is expected behaviour, not a bug, and the ten-minute +cron interval bounds the delay. This half of the note is a question, answered. + +**Cause, part two: the write blocks, it does not fail.** Measured 2026-08-04 +against Slackware's notmuch, with the lock held deliberately rather than by +racing cron: + +- `notmuch_database_open_with_config(NOTMUCH_DATABASE_MODE_READ_WRITE, …)`, + the exact call `applyTags` makes, **blocks until the lock is free and then + returns `NOTMUCH_STATUS_SUCCESS`**. A C probe against a lock held for 12s + returned after 9.158s with status 0; the same call with no lock held returns + in 0.001s. It was never observed to return an error or to time out. +- The `notmuch` CLI behaves identically (waits 3.6s and 13.2s against 5s and + 15s holds, always exit 0), so this is libnotmuch's behaviour and not a + wrapper's retry loop. +- Therefore the error branch at `src/notmuchworker.cpp:298-305` is **not + reachable through lock contention at all**. It fires only for a genuinely + broken open: bad permissions, a corrupt index, a missing database. + +**The real defect is a stall.** `applyTagsToThreads` is invoked on the worker +thread through a queued connection (`src/mainwindow.cpp:1788`), so a blocking +open freezes *the worker*, not the UI. The window keeps painting and the rows +show the optimistic update, but every later query, thread load and tag write +sits behind that open in the worker's event queue until the lock frees. Nothing +is lost and no error appears; the application simply stops responding to +selections for the duration. + +**How bad in practice.** Bounded by how long `notmuch new` holds the lock, which +on this user's already-indexed Maildir is a fraction of a second at roughly +T+32s into a ~35s run (measured from `~/.local/state/mailsync.log`: runs start +at :00 and reach "Processed N total files" 32-40s later). The stall is +therefore usually invisible, and becomes user-visible only when `notmuch new` +has real work: a first index, a large delivery, a `notmuch reindex`. That is +also why it cannot be reproduced by clicking during a normal sync, and why the +reproduction below holds the lock on purpose. + +**Reproducing it.** Racing cron does not work. Hold the lock deliberately: +`notmuch tag --batch` keeps the write lock for a whole session and releases it +when stdin closes, so feeding it a slow stream of no-op tag commands holds the +lock for a controllable time. Verified: a competing writer blocks for exactly +the remaining hold. + +**Approach.** Do not send a write the worker will block on. `SyncMonitor` +(item 27) already reports whether a sync holds the lock, so `MainWindow` can +hold the edit while `State::Running` and send it on the transition to `Idle`, +which is a signal that already exists and already fires. The rows keep showing +the change meanwhile, which is honest: it is what the user asked for and it is +going to be applied. + +**Explicitly rejected: retrying on error.** That was the first implementation +and it is dead code against this cause, since the error it keys off never +arrives from lock contention. Keying on the monitor's state is also strictly +better: it avoids the stall rather than recovering from it. + +**Constraints.** The optimistic-update-then-revert contract must survive: a +held edit is still unsynced and must keep counting toward the pending indicator, +or the quit prompt will let the user leave on work that never landed (the +failure item 28 and the 0.9.0 net-state fix were both about). Do not widen the +write window by holding the read-write handle open, per the read-only-by-default +rule in `CLAUDE.md`. A held edit must re-resolve its thread ids when it is +finally sent: `notmuch new` may have renamed files underneath it. And +`SyncMonitor::State::Unknown` must not gate writes, or a platform that cannot +read `/proc/locks` would never send an edit at all. + +**What the stall looks like, observed 2026-08-04.** Confirmed by hand with the +Xapian lock held deliberately: switching between threads left the message pane +showing the FIRST thread selected, and when the lock released the pane stepped +through the three or four threads selected in the meantime, in sequence. That +is the worker's queue draining, and it is the user-visible shape of this defect. + +**Reads are NOT blocked by the write lock.** Measured the same day, and it +bounds how bad this is. A read-only open and a 200-thread query take 0.001s and +0.015s whether or not another process holds the write lock, identical to +baseline. So `loadThread` never blocks on the lock itself. The stall is purely +head-of-line blocking on the single worker thread: one blocked `applyTags` holds +up every read queued behind it. Do not "fix" this by making reads lock-aware; +there is nothing there to fix. + +**Residual gap: the 2s polling window. Accepted for now (user, 2026-08-06), +and deliberately left open rather than closed.** `SyncMonitor` polls every two +seconds, so a sync that starts between polls is invisible to the window for up +to 2s, and a tag edit in that window is still sent straight into a blocking +open. The window is 2s wide, `notmuch new` holds the lock for well under a +second on an already-indexed Maildir, and reads are unaffected either way, so +the exposure is small and the shipped behaviour is the pre-existing one. + +**The option stays on the table, to revisit:** check the lock at send time. +`SyncMonitor::lockHeldIn()` is already a static, pure function over +`/proc/locks` content, so `sendThreadTagChange` can call it for the cost of one +small file read per tag action. That closes the window entirely. It was not +taken now because it would add untested code to a change that had just been +verified by hand, which is the wrong order. + +### Hand test (2026-08-06): passed + +Verified against a real blocking open, which the unit tests cannot reach: they +drive the deferral through the meta-object and never take a lock. Both locks +held for 100s by the throwaway scaffold, with a tag edit made during the hold. +All six expected behaviours confirmed by the user: the row kept the tag, the +status message did not expire, the unsynced indicator rose, the Sync button was +disabled, **the window stayed responsive**, and the held edit sent itself on +release without a click. + +The responsiveness check is the one that mattered. The stall observed on +2026-08-04 left the message pane frozen on the first thread selected and +replayed the queue on release; that no longer happens. + +**Related:** item 35 (refresh after sync) touches the same `Idle` transition, +and both want a non-destructive path that does not clear the undo stack. + +## 38. `test_mainwindow` fails when a real sync holds the lock + +**Observed 2026-08-04:** `theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock` +failed once during a full run and passed on every rerun. The cause is not +ordering or pollution between tests: the user's cron sync happened to be running +at that moment. + +**Cause (verified in code).** `MainWindow::buildUi()` constructs a real +`SyncMonitor` on `SyncMonitor::defaultLockPath()` and the live `/proc/locks` +(`src/mainwindow.cpp:467`) and starts it. Every `MainWindow` a test builds +therefore observes the machine's actual sync state. The test asserts +`button->isEnabled()` on a freshly built window, which is false whenever a real +sync holds `/tmp/mbsync.lock`. With cron firing every ten minutes and a run +lasting ~35s, roughly 6% of test runs land inside one. + +**Not caused by the item 37 work**, though that is when it was noticed. The +test and the monitor both predate it; confirmed by stashing the item 37 changes +and seeing the suite pass, then reproducing the failure with a sync live. + +**Approach.** The monitor is already injectable: its constructor takes a +`locksPath` precisely so tests can drive transitions without real locks +(`src/syncmonitor.h:59-64`), and `test_syncmonitor` uses that. `MainWindow` does +not expose it. Either let the window take a locks path (config or a setter used +only by tests), or have the test point `SyncMonitor` at a temporary file. The +existing tests that drive `onExternalSyncStateChanged` through the meta-object +are unaffected either way; it is only the construction-time state that leaks in. + +**Constraint:** do not simply stop starting the monitor in tests. The +construction-time state IS the behaviour under test for this case, and a window +that never polls would pass the assertion for the wrong reason. + +### Outcome (done) + +`MainWindow::setLocksPathForTesting()` / `locksPath()` give the window the seam +`SyncMonitor` already had, and the test points it at an empty file in its own +`QTemporaryDir` so construction observes no sync. The constraint above is +respected: the monitor is still constructed and still started, it simply reads a +lock table the test controls. + +**A test seam, deliberately not a config key.** `/proc/locks` is not something a +user would ever set, and a wrong value fails silently by disabling background +sync detection rather than loudly. A `[general]` key was considered and rejected +for that reason. + +**The override is process-wide and is reset at the end of the test**, since the +`QTemporaryDir` holding the file is destroyed with it; leaving it set would +point every later window at a path that no longer exists. + +**Verified by reproducing the original failure rather than waiting for cron.** +Running the suite under `flock -n /tmp/mbsync.lock` fails the assertion exactly +as reported when the seam is bypassed, and passes with it in place. The first +mutation attempted was a dud worth recording: writing `MUTANT` into the injected +lock table does not fail the test, because it is not a parseable `/proc/locks` +line and `lockHeldIn()` correctly finds no lock in it. + +## 39. Thread list cannot be sorted by clicking a column header + +**Dropped 2026-08-10 (user).** The item asked for a column header to click and +there is no longer one to click: the card list (items 20 and 53) collapsed the +five columns into a single column of cards, and the header is gone with them. +0.13.0 shipped a sort dropdown in the query row, which is the same capability +reached a different way, so the complaint behind this item is answered and the +mechanism it proposed is unbuildable. The analysis below is kept because its +constraints outlived it: the batched-append problem and the sort-on-timestamp +rule apply to any future sort, including the dropdown's. + +**Observed (user, 2026-08-05):** "left pane columns order by clicking on the +column header." + +**Cause (verified in code):** nothing sorts. `setSortingEnabled` appears nowhere +in `src/`, no `QSortFilterProxyModel` exists, and `ThreadListModel` implements no +`sort()`. The order is whatever the worker emitted, and that is fixed: +`NotmuchWorker::runQuery()` calls +`notmuch_query_set_sort(..., NOTMUCH_SORT_NEWEST_FIRST)` +(`src/notmuchworker.cpp:135`). Clicking a header does nothing because the header +was never made interactive. + +**Approach.** Sort in the model, not in the query. The worker's sort is over +notmuch's own ordering and cannot express "by From" or "by Subject" at all, and +re-querying per header click would send the user's place away for a presentation +change. + +- `ThreadListView::setSortingEnabled(true)` plus a `sort()` on the model, or a + `QSortFilterProxyModel` between them. +- Persist the sort column and order into `uistate.conf`, per item 1's rule. A + sort that resets on restart is item 1 restated. + +**Constraints.** + +- **Batched appends are the real difficulty.** Threads arrive in batches of 200 + through `appendBatch()` while the query is still running, so a sorted view is + re-sorted on every batch and rows move under a selection the user is already + working in. Decide explicitly: sort only once the query completes, or accept + the movement. A proxy model makes this worse rather than better, since it + re-sorts on every insert by default. +- The date column is displayed text but must sort as a timestamp, not as a + string. `ThreadSummary` carries the real value; sort on that, not on the + rendered cell. +- Row styling (item 13) and the unread bold are per-row, so they follow the row + and need nothing here. Verify anyway after a proxy is introduced: a proxy that + forwards only `DisplayRole` drops them silently. + +## 41. A message whose HTML body carries a `Content-Id` renders blank + +**Observed (user, 2026-08-05):** a specific message from a bulk sender opens +blank, and the app reports it has no HTML part. + +**Cause (verified in code): the inline-part branch runs before the body +branches, and returns.** `collectParts()` in `src/mimeparser.cpp` tests +`g_mime_part_get_content_id()` at `:133` and, whenever a part has one, files it +into `out.inlineParts` and returns at `:139`. The `text/plain` and `text/html` +assignments at `:142-146` are never reached for that part. + +Setting a `Content-Id` on the `text/html` body part is legal and common in +bulk-sender output. Such a message parses with an empty `htmlBody` and an empty +`plainBody`, so `ParsedMessage::hasHtml()` (`src/mimeparser.h:115`) is false, +`HtmlBuilder` falls through to a plain body that is also empty +(`src/htmlbuilder.cpp:208`), and the pane renders nothing. Both halves of the +user's observation follow from one wrong ordering. + +**Approach.** A `Content-Id` makes a part *referenceable*, not non-displayable. +The two are independent, and the current code treats them as exclusive. + +- Register the part in `inlineParts` as today, and then still let a + `text/plain` or `text/html` part fill the corresponding body slot when that + slot is empty. Do not return early on the presence of a content id alone. +- The existing "first one wins" rule (`out.htmlBody.isEmpty()`) already keeps a + genuinely inline image from displacing a real body, so a part that is not text + is unaffected by this change. + +**Constraints.** + +- **Do not use `Content-Disposition: inline` as the discriminator.** It is + absent far more often than it is correct, and a body part commonly carries no + disposition at all. The `attachment` check above it (`:116-117`) is already the + right test for "not a body" and should stay the only one. +- A part that is both the body and a `cid:` target must remain reachable under + its id, or a sibling referencing it breaks. Register first, then assign. +- This is `MimeParser`, which is fixture-tested: the fix needs a fixture message + whose `text/html` part carries a `Content-Id`, asserting both that the body + renders and that the id still resolves. Write the fixture by hand rather than + from real mail, per the no-personal-details rule. + +**Verification:** the user's original message renders. The `cid:` rewriting of +item 15's namespacing is unaffected, which the existing mimeparser tests already +cover. + +**Confirmed on real mail (2026-08-07).** Affected messages are not easy to find, +because the common case, a `Content-Id` on an inline image, always worked and +swamps a naive grep. The narrow case is a `Content-Id` in the *same header +block* as a `Content-Type: text/html`: + +```bash +notmuch search --output=files 'tag:inbox' | while read -r f; do + awk 'BEGIN { IGNORECASE=1; html=0; cid=0 } + /^$/ { if (html && cid) { print "yes"; exit }; html=0; cid=0; next } + /^Content-Type:[ \t]*text\/html/ { html=1; next } + /^Content-ID:/ { cid=1; next } + END { if (html && cid) print "yes" }' "$f" | grep -q yes && printf '%s\n' "$f" +done +``` + +That found 96 messages in one inbox, all from bulk senders, with 57 from a +single one. Pull `Message-ID` from a match and paste it into the query bar as +`id:<the-id>` to open it. Note `file:` is **not** a notmuch search term, so a +path cannot be turned into a query directly; read the header instead. + +## 42. "Syncing..." says nothing about what is being synced + +**Observed (user, 2026-08-05):** the only feedback during a manual sync is +"Syncing" in the status bar. The user asked for the account being synced +(e.g. `Syncing provider-work`) and the operation in progress (mbsync, notmuch). + +**Cause (verified in code): the information is already arriving and is thrown +away.** `assets/mailsync.sh` streams every mbsync and `notmuch new` line, +timestamped, through `tee` (`assets/mailsync.sh:75-96`), and `MailSync` emits +each chunk as `outputReceived` (`src/mailsync.cpp:65-74`), which fills the sync +log pane. The status label is set once to `tr("Syncing...")` +(`src/mainwindow.cpp:1569`) and never updated until the run finishes. + +**Correction (2026-08-07, measured): the paragraph above was half wrong, and +the "no script change needed" claim with it.** `notmuch new` does stream, but +plain `mbsync -a` prints **nothing at all** until it exits, then one summary +line. Measured on a real run: one line at 11:11:08, then 73 lines within the +single second 11:11:33, at the end of a 46-second run. So for the part of a +sync that actually takes time there was no output to read, and no parsing of +the existing stream could have fixed that. + +Two wrong diagnoses were made and discarded before the real one. It is **not** +buffering, so `stdbuf` does nothing: the output streams fine, there simply is +none. And the account name **is** available, contrary to the first reading of +this item, which concluded it was not and proposed shipping phases only. + +`mbsync -V` is what changes both: it announces each channel as it reaches it +(`Channel <name>`), which is at once the progress indication and the account +name the user asked for. The shipped script now passes it. + +**Approach.** Derive a short status from the output already being received. + +- Recognise the phase from the stream: lines before `notmuch new` starts are + mbsync's, and `notmuch new` announces itself. Show "Syncing mail (mbsync)" + then "Reindexing (notmuch)". +- mbsync prints the channel it is working on **only under `-V`**, which is the + account name the user wants to see. Take it from the output rather than from + config, so what is shown is what is actually happening, and in the order it + actually happens. + +**Constraints.** + +- **Sync output is untrusted-ish input.** It comes from a local script, but it is + interpolated into a status label; keep it plain text and truncate it, so a long + or hostile line cannot resize the status bar or inject markup. +- The status label is shared with transient messages, which expire (item 33). + A sync phase is not transient and must not be cleared by that timer, nor + clobber a message the user is reading. +- Do not parse the output to decide success or failure. The exit status is the + authority, deliberately (`assets/mailsync.sh:101-108`), and a second opinion + derived from text would eventually disagree with it. +- Match loosely. mbsync's exact wording varies by version, and a status line that + goes blank because a string moved is worse than the current fixed one. + +**Two defects in existing code, found while building this and fixed with it.** + +- `startSync()` called `setSyncBusy(true)` **after** `m_sync->start()`, so any + per-run state reset there happened after the process had already produced + output. A short run delivers everything before control returns, which wiped + the phase those lines had produced. The reset now happens before the launch. +- The first draft deferred a phase while a transient message was still showing, + reading the constraint above as "never clobber a message the user is + reading". That let a `Background sync completed` message armed **before** the + sync started suppress the entire run's phases, which is how the first hand + test came back red. A running sync's state outranks an expiring event + message, so the deferral was removed. The constraint it was serving is + satisfied the other way round: a phase is written directly rather than + through `showTransientStatus()`, so the timer never reclaims it. + +**Verification note.** A test script that prints its lines at once is delivered +in a single `readyRead`, so the tracker sees the whole run in one call and only +the final phase is ever painted, which makes every intermediate one +unobservable. `test_mainwindow`'s script therefore paces itself with `sleep`, +standing in for a real sync's tens of seconds. The parser itself was checked by +replaying real captured `mbsync -V` output through it. + +## 43. No "Mark all read" for the current view + +**Observed (user, 2026-08-05):** a "Mark All Read" button next to Sync, Archive, +Delete and Undo, applying to the current view. + +**Cause (verified in code):** no such action exists. The registered actions are +the list at `src/mainwindow.cpp:590-756`; there is `toggle_unread`, which acts on +the selection, and nothing that acts on a whole result set. + +**Approach.** The machinery is already there and this is mostly a question of +scope. `applyTagsToThreads` resolves a multi-thread selection in one combined +query, per `CLAUDE.md`, so marking many threads read is one write, not N. + +- An action removing `unread` from every thread in the current view, routed + through the same funnel, with its inverse pushed onto the undo stack as a + single command. +- Item 25 already established select-all, so "select all, then toggle unread" is + the manual route today. Decide whether this item is that, or genuinely + view-wide regardless of selection. + +**Constraints.** + +- **"The current view" is not the same as "the loaded rows".** Threads arrive in + batches and a large query may still be running, so an action taken mid-load + would silently skip whatever has not arrived. Either act on the model's rows + and say so, or wait for the query to complete. Do not describe it as "all" if + it is not. +- One undo entry for the whole operation, not one per thread. A user who marks + 400 threads read and then hits Ctrl+Z expects one press to be enough. +- No confirmation dialog, per `CLAUDE.md`, even though this touches many threads. + Undo is the answer here as everywhere else. +- The pending-edit count must move by the real number of threads changed, or the + quit prompt understates the work at risk. + +**Resolved (2026-08-07).** The scope question above was decided by the user: +the action is **disabled until the query reports its total**, rather than +acting on a partial set or stalling on a wait. `m_queryComplete` gates it, +cleared in `runCurrentQuery()` and set in `onQueryFinished()`. A greyed control +says "not yet" without a dialog, and the honesty constraint is satisfied by +construction rather than by wording. + +Two things came out differently from the plan, both forced by existing code. + +- **It carries a default binding, `Ctrl+Shift+U`**, shifted against `Ctrl+U` + for `toggle_unread`. The intent was toolbar and menu only, but + `everyActionHasAShortcut` requires every registered action to have one: an + unbound action is unreachable from the keyboard, and an empty shortcut means + the action list and the default table have drifted apart. The invariant is + deliberate, so the action was given a binding rather than the invariant being + relaxed. +- **Only the threads that are actually unread are sent.** Sending every row + would inflate the pending-edit count with writes that change nothing, and the + quit prompt reads that count. A view with nothing unread does nothing at all, + pushes no command, and says so: an undo entry that restores nothing is worse + than none, since it absorbs a Ctrl+Z meant for the previous action. + +**A test-seam note worth keeping.** `undo->isEnabled()` cannot answer "was a +command pushed": the undo `QAction` is always enabled and tests `canUndo()` +when triggered. A first version of the no-op test asserted on it and passed +against a mutant with the unread filter removed. `undoDepthForTesting()` exists +because of that, and the mutation is caught now. + +## 44. No way to manage the filters applied at sync time + +**Observed (user, 2026-08-05):** "manage filters to be applied when syncing (view +existing, edit, delete, create new, copy as new, dry-run)." + +**Unspecified, and blocked on a question the user has to answer first: there are +no such filters in this application.** Nothing in `src/` applies rules at sync +time; `MailSync` runs one configured command and shows its output, and +`assets/mailsync.sh` is mbsync plus `notmuch new` under a lock, with no rule +engine anywhere in it. + +So the item is not "expose the existing filters in the UI". It is one of: + +- **A UI over rules that live somewhere else**, e.g. the companion `mailctl` + project or a hand-written notmuch tagging script the user runs after + `notmuch new`. If those exist, this item is an editor for that file and its + shape follows that file's format. +- **A rule engine in qtmaildir**, which is a materially larger piece of work and + a change to what this application is: v1 is read-and-organize over an index + someone else fills. + +**Answered and specified 2026-08-12.** It is the first option. The rules exist, +in the notmuch `post-new` hook inside the user's Maildir: hand-written +`notmuch tag` lines, each scoped to `tag:new`, tag-only by design. They carry +substantial reasoning in shell comments about which senders each rule +deliberately excludes. + +The design is `specs/2026-08-12-tagging-rules-design.md`. In short: the rules +move to `~/.config/mailrules/rules.json`, a tool-neutral store both qtmaildir +and `mailctl` read, with unknown fields preserved across a write by either tool +so neither owns the format. `post-new` becomes a Python loop over that file, +living in the mailctl repository. A rule stores no scope, so the same rule +serves the hook (scoped `tag:new`), a dry run (whole corpus, counts only) and a +future backfill. qtmaildir gets `TagRules` plus a management dialog; mailctl +gets read-only `rules list|show|dry-run`. + +**The constraint below was considered and is not triggered.** No rule engine is +built in qtmaildir and nothing rewrites the Maildir: the tagging still happens +in the notmuch hook, and qtmaildir edits the rule file and counts matches. + +**Backfill is deliberately out of v1**, per the user's decision, and is the one +piece that will force a revision to `CLAUDE.md`'s "no destructive-action +confirmation, undo instead" rule, which the user has said is due for revision +anyway. A rule that is safe against arrivals is not safe unscoped. + +**Constraint if a rule engine were ever built here:** `CLAUDE.md` records that +this project does no network protocol work at all and that fetching is external. +A filter engine that rewrites the Maildir would not violate that literally, but +it would put qtmaildir in the business of moving mail, which is a decision to +take deliberately rather than by implementing a dialog. + +### Outcome (done 2026-08-13) + +The rules moved from the shell `post-new` hook to +`~/.config/mailrules/rules.json`, read by both qtmaildir and `mailctl`. The +hook and the shared `mailrules.py` live in the mailctl repository; this repo +has `TagRules`, `tests/test_tagrules.cpp`, a management dialog on the Message +menu, and `NotmuchWorker::requestMessageCounts` for the dry run. Seventeen real +rules were converted and each one's shell comment became its `note`. + +**Four things learned that outlive the item.** + +The parenthesisation of a rule's own query is load-bearing. `tag:new and a or b` +binds as `(tag:new and a) or b`, so a rule that is a disjunction of senders +escapes its scope and matches the whole corpus. Several real rules have exactly +that shape. `mailrules.scoped_query` is the only place that string is built. + +The hook must not consume `tag:new` when the rules failed to load. Clearing the +marker while the rules did not run orphans that mail permanently and silently, +and the gap would surface months later as "why did this stop being tagged". + +**notmuch's query parser rejects almost nothing**, which invalidated two +assertions written into the plan from memory. `from:((((` parses cleanly and +matches nothing rather than failing, so a test expecting a non-zero exit or a +`-1` count fails against correct code. `tests/test_notmuchworker.cpp` already +recorded this for thread counts and the lesson had to be learned twice. + +`requestCounts` counts THREADS, which is right for the placeholder pane and +wrong for a rule: a rule tags messages, so a thread count understates any rule +matching part of a large thread. Hence `requestMessageCounts` beside it rather +than a change to it. + +**Verification, since a rules file that tags the wrong mail is expensive.** All +seventeen converted rules were counted against the real index and matched the +shell hook exactly, tags and counts, before anything was installed. The staged +hook was then run against real mail with a tag deliberately removed, and +restored it. The live swap was confirmed by a real sync: `status=OK`, +`applied 17 rule(s)`, marker consumed, and the per-rule counts moved as new mail +arrived. + +**Backfill remains out of scope**, and is the piece that will force a revision +to the "no destructive-action confirmation, undo instead" rule in `CLAUDE.md`. +The user has said that rule is due for revision anyway. See the spec's "Out of +scope" section. + +## 45. Two Sync buttons on the main window + +**Observed (user, 2026-08-05):** "there's currently 2 Sync buttons on the main +interface. UX redundant." + +**Cause (verified in code): they are two separate widgets built by two separate +passes.** `m_syncButton` is a `QPushButton` created in `buildUi()` +(`src/mainwindow.cpp:441`) and placed in the query bar row. Independently, the +`sync` action registered at `:721` appears on both the File menu and the +toolbar, from item 3's menu work. Nothing removed the original button when the +toolbar gained one, so the window shows both. + +### Revised 2026-08-06: this is a defect, not a cosmetic cleanup + +**The two controls do different things**, which the original write-up assumed +away by treating them as duplicates. Confirmed in code after the user reported +that the toolbar one "doesn't perform a sync": + +- The `QPushButton` handler (`src/mainwindow.cpp:464`) starts the sync, clears + the log pane, shows it, disables the button, and reports "Sync already + running" when `start()` returns false. +- The `sync` QAction handler (`src/mainwindow.cpp:721`) is + `if (m_sync->isAvailable()) m_sync->start();` and nothing else. No log pane, + no disable, and the return value is discarded, so a rejected start is silent. + +So the toolbar button most likely *does* start a sync; every piece of evidence +that it did lives in the other handler. That is item 13's failure restated: the +feedback exists where the user cannot see it. + +**Worse, item 29 shipped for one widget only.** `onExternalSyncStateChanged` +disables `m_syncButton` during a background sync (`src/mainwindow.cpp:1606`) and +never touches the action. During a cron sync the toolbar Sync stays clickable +and can only produce the EX_TEMPFAIL skip, which is the exact behaviour item 29 +exists to prevent. The user's note about "2 sync buttons" is therefore sitting +on top of a live defect rather than a redundancy. + +**The user's preference (2026-08-06):** keep the top-left one, next to Archive, +Delete and Undo. The one beside the query bar reads instinctively as a Search +button, which is a real misaffordance given what sits next to it. + +**Approach.** Not "pick a survivor". Move the button's handler onto the action, +so the two behave identically, then drop the now-redundant `QPushButton`. The +action carries its shortcut, its enabled state and its menu entry from one +place, which is the whole point of item 3's conversion; the loose button is the +last widget that predates it. + +Route the enabled state through the action too, so `setEnabled` has one target +rather than two that can disagree. `QAction::setEnabled` propagates to every +widget showing it, which is what makes this smaller than it looks. + +**Constraints.** + +- **The button is not just a button today.** It is disabled while a sync runs, + including one started externally (item 27/29), and `test_mainwindow` asserts on + it by name. Whatever replaces it has to carry that state, and the tests need + pointing at the action rather than at the widget. Three tests find it via + `findChild<QPushButton *>("syncButton")`, including the one item 38 just + fixed, so they move together with the widget. + +- **A test must cover the toolbar path specifically.** The whole defect is that + one of two controls was never given the behaviour, and a test that drives only + the surviving widget would have passed throughout. Assert on the action's + enabled state during a background sync, which is the half that silently never + worked. + +### Outcome (done) + +Built in the order the user asked for: make the toolbar control work, prove it, +then remove the other one. + +`startSync()` is now the single handler behind every route in, the toolbar, the +File menu, the shortcut and, until it was removed, the button. The old action +handler was `if (m_sync->isAvailable()) m_sync->start();`, which cleared no log, +opened no pane, disabled nothing and discarded `start()`'s return value, so a +rejected start was silent. It now also reports when no sync command is +configured rather than doing nothing at all. + +`setSyncBusy()` sets the enabled state on the QAction, which reaches the toolbar +button, the menu entry and the shortcut at once. That was the actual defect: +item 29 set a separate QPushButton and never touched the action. + +**Verified red first, then load-bearing.** +`theSyncActionIsDisabledWhileABackgroundSyncHoldsTheLock` fails before the fix +with the action still enabled during a background sync, and fails again when the +action's `setEnabled` is removed afterwards. The pre-existing button test passed +throughout, which is exactly why the defect survived item 29: it drove the half +that worked. + +**Confirmed by hand before the widget was removed**, per the user's condition: +reading mail grew the unsynced count, the toolbar Sync ran the sync, and the app +refreshed and reported "Sync complete" at the end. + +**Then the QPushButton went.** Its unavailable-command tooltip moved to the +action, since with no command configured the control is disabled and the tooltip +is the only thing that says why. The old button test was deleted rather than +repointed, being an exact duplicate of the new action test, and the +unobservable-lock-table test now asserts on the action. +- Check for other loose widgets doing the same thing before touching this one, so + the fix is not repeated per widget later. +- Removing a visible control is the kind of change that looks like a regression. + Confirm with the user which of the two survives; the note says redundant, not + which one is wanted. + +## 46. `uiStateSurvivesARestart` fails under the offscreen platform + +**Observed 2026-08-06:** the full suite is green on the user's Wayland session +but `TestMainWindow::uiStateSurvivesARestart` fails under +`QT_QPA_PLATFORM=offscreen`, which is how the suite is run when a session must +not open windows on the user's screen. Only the width is wrong: + +``` +Actual (reopened.size()): QSize(798x620) +Expected (resized) : QSize(940x620) +``` + +**Cause (verified by probe, not assumed).** The offscreen platform reports an +800x800 screen. `QMainWindow::restoreGeometry()` clamps a restored window to the +available screen area, so the test's 940 width comes back as 798 while its 620 +height, which fits, restores untouched. That asymmetry is the tell: the state +file is written and read correctly, and the zoom factor in the same test +restores fine. Nothing is broken in item 1's persistence. + +**Not a state-file collision.** The test already scopes itself properly with +`QStandardPaths::setTestModeEnabled(true)` and removes the file at both ends +(`tests/test_mainwindow.cpp:229-255`), so it never touches +`~/.local/state/qtmaildir/uistate.conf`. An earlier reading of this failure +blamed the real state file being held by a running app; that was wrong, and the +test source disproves it. + +**Approach.** Pick a size the smallest plausible test screen can hold, well +under 800x800, and assert on that. The test is about persistence, not about +large windows, so the specific number carries no meaning and only needs to +differ from the default. + +**Constraint:** do not "fix" this by widening the assertion to a tolerance or by +skipping under offscreen. Both would hide a genuine restore failure later, and +the property under test (the size that went in comes back out) is exact. + +**Related:** the same class as item 38. Both are tests that silently depend on +the machine they run on, and both were found by running the suite in a context +its author had not tried rather than by reading it. + +### Outcome (done) + +The asserted size is now 640x560, which fits the offscreen platform's 800x800 +screen. Nothing in `MainWindow` changed: the persistence was never broken, only +the test's choice of a window wider than the smallest screen it runs against. + +Verified both ways round, since this one passed on Wayland throughout: 45 of 45 +under offscreen where it previously failed, and still green on the real +platform. + +## 47. The query bar looks unfinished, and cannot be cleared by mouse + +**Observed (user, 2026-08-06), immediately after item 45 removed the Sync +button:** the bar "having no button seems kind of incomplete", and the user +asked whether a clear icon could be shown in it. + +**Cause:** the query field was the last stretching item in its row, so with the +Sync button gone it ran flush to the window edge with nothing terminating it. +Clearing it needed the keyboard; `QLineEdit` does not draw a clear button unless +asked. + +**Approach, and what was deliberately NOT built.** The user's first instinct was +a "🔎 Search" button. That was argued against and dropped: Return already runs +the query, and a button beside a text field is exactly what read as Search and +got removed in item 45. Adding one back would restate items 13 and 45 in a new +spot. + +What was built instead: + +- `setClearButtonEnabled(true)` on the query field. Qt draws the ✕ inside the + field, shows it only when there is text, and themes it from the desktop. One + line, no icon asset, no new widget. +- The saved-query buttons moved from their own row onto the query row, after the + field, so the bar is framed by the account dropdown on the left and the saved + queries on the right. The row they left is gone and the thread list gains that + vertical space. This was the user's own proposal and it addresses the + "incomplete" reading directly, without adding a control. + +**Constraints.** + +- **`[queries]` is unbounded.** Three entries fit comfortably; enough of them + would squeeze the field. No overflow handling was built, marked with a + `ponytail:` comment pointing at item 23, which already specifies + buttons-plus-menu and is where that belongs. +- Button order follows `childKeys()`, which sorts alphabetically, so the buttons + read Flagged, Inbox, Unread regardless of the order written in the config. + Pre-existing, and its own item if it matters. + +**Known behaviour, accepted:** clicking ✕ focuses the field, and an empty field +makes `QueryCompleter` offer everything, so the popup opens. Confirmed by the +user as acceptable; suppressible if it becomes annoying in daily use. + +**Verification:** by hand. The tests do not click the ✕, which is a mouse path. +The user confirmed the icon renders correctly, is themed, and clears the field. + +## 48. Removing a tag suggests every tag, not the thread's own + +**Observed (user, 2026-08-06):** "when removing a tag from a thread using the +input box, I get suggested all tags, not only those relevant to the message +being edited." + +**Cause (verified in code):** both fields share one completer setup. +`TagDialog`'s constructor loops over `{ m_addEdit, m_removeEdit }` and builds +`new QCompleter(knownTags, edit)` for each (`src/tagdialog.cpp:163-164`), and +`knownTags` is every tag in the database. That is right for Add, where the point +is to reach any tag and even create one, and wrong for Remove, where the only +tags that can be removed are the ones the selected threads already carry. + +**The data is already in the dialog.** The constructor takes `currentTags`, a +`QHash<QString, int>` of tag to how many selected threads carry it +(`src/tagdialog.h:66-67`), and uses it at `:212` to render the existing-tag +display. It is simply never given to the remove field's completer, so this needs +no new plumbing and no worker query. + +**Approach.** Build the two completers from different vocabularies rather than +in one loop: `knownTags` for Add, `currentTags.keys()` for Remove. + +**Constraints.** + +- **Keep the setWidget/prefix machinery exactly as it is.** Both fields hold a + comma-separated list, and `QLineEdit::setCompleter` is the documented trap + this dialog already works around, hit twice in this codebase. Only the + candidate list changes; the wiring does not. +- **Completion stays a suggestion, not a whitelist.** The dialog's own comment + records that a tag absent from the list is exactly what it exists to create. + For Remove that matters less, but typing a tag not in the list must still be + possible rather than blocked, so nothing may start validating input against + the candidates. +- On a multi-thread selection `currentTags` is the union across the selection, + with counts. That is the right set to offer, since removing a tag two of three + threads carry is meaningful. Do not filter to tags every thread has. + +**Verification:** a test can construct the dialog with a known `currentTags` and +assert the remove field's completer offers only those. The keys must be typed, +not `setText()`, since `setText` does not drive a completer at all, which +`CLAUDE.md` records. + +## 49. Sync always runs every account, even when one account was touched + +**Observed (user, 2026-08-07):** "make the manual and closing sync operation +work only on affected accounts (it now runs for all accounts, independent from +actual modifications present). If no modifications exists, assume it's for all +accounts and run normally." + +**Cause (verified in code):** the account set is not a parameter anywhere on the +path. `MailSync::start()` (`src/mailsync.cpp:170`) takes no arguments; it splits +the configured `sync_command` string and runs it verbatim +(`src/mailsync.cpp:177-184`), so the command line is fixed at config time and +identical for every run. The shipped script then hardcodes the whole-store +sweep: `mbsync -V -a` (`assets/mailsync.sh:83`), where `-a` means all channels. +Nothing between the tag edit and mbsync carries which account changed. + +**Approach.** Three pieces, and the first is the real work. + +- **Track which accounts have unsynced edits.** Item 18's `m_pendingEdits` is a + bare count; this needs the set of account tags behind it, accumulated in the + same `onTagsApplied()` handler and cleared on the same successful-sync reset. + `ThreadSummary` already carries the account tag that `ThreadListModel` renders + as the account chip, so the mapping exists and needs no worker query. +- **Let `start()` take accounts.** An optional `QStringList` appended to the + configured command's arguments, empty meaning today's behaviour. +- **Let the script accept channel names.** `mailsync.sh` passes them to mbsync in + place of `-a` when given, keeping `-a` when not. + +**Constraints.** + +- **Empty set means all accounts, per the user's own wording.** A sync with + nothing pending is a fetch, and fetching one account because that is where the + last edit happened to be would be wrong. +- **A notmuch account tag is not necessarily an mbsync channel name.** The + config already separates the two ideas (`[account.<key>]` has a notmuch tag and + a display `label`); mapping to a channel needs either a new per-account key or + an explicit decision that the key IS the channel. Settle this with the user + before building, it is the one design question in the item. + + **Resolved 2026-08-07 by reading the user's real mbsync config against their + qtmaildir config: a new key is required.** Three of five accounts match their + channel name exactly, and two do not, because a QSettings section key may + carry dots that the channel does not: a section `[account.mail-first.last]` + against a channel `mail-firstlast`. Key-as-channel would therefore name two + channels mbsync does not know, and mbsync treats an unknown channel as fatal, + so those two accounts' syncs would fail outright rather than degrade. The + `maildir` key tracks the section key rather than the channel and is no help. + Built as an optional `channel` key defaulting to the section key, so the three + matching accounts need no config edit. +- **`notmuch new` still runs over everything**, and must. Restricting the fetch + does not restrict the index. +- **The script's two shipped properties survive**: it prints to stdout as well as + its log, and it exits with the real status. `CLAUDE.md` records why. +- Item 42's status-bar parsing reads the account name out of `mbsync -V` output; + a narrowed run must keep `-V` or that regresses. + +**Verification:** by hand, since it ends in a real fetch. Tag in one account, +sync, and confirm from the log that only that channel ran. Then sync with +nothing pending and confirm all channels run. + +## 50. Esc blanks the pane but leaves the row selected + +**Observed (user, 2026-08-07):** "esc in the main window should deselect whatever +is selected in the left pane while still blanking the right pane. Two actions +instead of one." + +**Cause (verified in code):** deliberate, and now reconsidered. The `clear_pane` +action (`src/mainwindow.cpp:772`) is bound to `Esc` by default +(`src/keymap.cpp:88`), and its own comment states the intent: "A view change, +not a mail change: the selection, the query and the undo stack are all left +alone." It calls `m_messageView->clear()` and drops `m_currentThreadId` +(`src/mainwindow.cpp:783`) and touches the selection model not at all. Item 32 +built exactly what was asked for then; the user now wants the selection cleared +as well. + +**Approach.** The note asks for "two actions instead of one", so this is not a +matter of adding `clearSelection()` to the existing one: + +- `clear_pane` keeps its current behaviour and its name, still available to a + user who binds it. +- A second action clears the pane AND the thread list selection, and takes the + `Esc` default. + +**Constraints.** + +- **Clearing the selection must not resurrect the pane.** The selection handler + reacts to `selectionChanged`, so clearing it fires that path; confirm it + leaves the pane blank rather than re-rendering or repainting a placeholder, + and order the two operations accordingly. This is the whole risk in the item. +- **Interacts with item 30.** Once the blank pane shows a placeholder, "blank" + means "show the placeholder", and a multi-selection message is one of the + things item 30 specifies the placeholder saying. Build 30 first or the two + will disagree about what an empty pane looks like. +- The in-flight-load guard must keep working: `m_currentThreadId` is cleared + with the pane precisely so a `threadLoaded` arriving afterwards is dropped. +- Both actions need entries in the shortcut reference, which is generated, so + the descriptions must distinguish them in one line. + +### Outcome (done 2026-08-07) + +Built as two actions, per the user's "two actions instead of one". +`clear_selection` takes `Esc` and does both; `clear_pane` keeps its existing +behaviour on `Shift+Esc`. It needed a default rather than being left unbound: +every action carries one, and `everyActionHasAShortcut` enforces it. + +**The reload hazard is real, and both guards against it are load-bearing.** +`clearSelection()` leaves `currentIndex()` VALID, so `onSelectionChanged()` +takes its "one or fewer rows" branch, finds a current row whose id differs from +`m_currentThreadId`, and calls `onThreadSelected` for it: the thread is +re-adopted and a `loadThread` sent for the row being cleared. Clearing the +selection BEFORE blanking means that runs while `m_currentThreadId` still names +the displayed thread, so the ids match and nothing reloads; `setCurrentIndex()` +then stops a later collapse-to-one-row reaching the same row. + +All four arrangements were tried, and **only one passes**: dropping +`setCurrentIndex()` fails, and moving either line after the blanking fails. + +**The first version of the test could not tell any of them apart.** It asserted +`showingPlaceholder()`, which passes whatever the code does, because +`test_mainwindow` has no worker: `loadThread` never replies, so nothing ever +repaints the pane. That is the standing limitation `CLAUDE.md` records, met +head-on. What IS observable without a worker is `currentThreadId()`, the id the +window sets on its way to sending the request, and `currentIndex()`. Asserting +those two is what made the test discriminate. + +**A caution about mutation testing, learned the hard way here.** Two of these +conclusions were reached and reversed before the four-way comparison settled it, +once because a mutation crashed the build and the crash was read as a test +failure. A mutation that does not compile, or that dies before the assertion, +proves nothing. Check the run actually reached the assertion before believing +what it says. + +## 51. Clicking a subject scrolls the list sideways + +**Observed (user, 2026-08-07):** "when selecting a row by clicking on the subject, +the table scrolls horizontally to accomodate the whole subject column into view. +Minor UX." + +**Cause (verified in code):** `QAbstractItemView`'s auto-scroll, which is on by +default and scrolls the clicked index fully into view. The Subject column +stretches and holds long text, so the view has somewhere to scroll to; +`src/mainwindow.cpp:548` sets `setHorizontalScrollMode(ScrollPerPixel)`, which +makes the movement smooth rather than by whole columns but does not cause it. +Nothing calls `scrollTo()` explicitly, and `setAutoScroll` is not set anywhere. + +**Approach.** Cheapest first, and check it is enough before going further: +`setAutoScroll(false)` on the thread view suppresses the scroll-into-view on +click. If that proves too blunt, override `scrollTo()` to ignore the horizontal +component and defer to the base class for the vertical one. + +**Constraints.** + +- **Keyboard navigation must still scroll vertically.** Arrowing past the bottom + of the viewport has to follow the current row, and `setAutoScroll(false)` is + the flag that governs that too. Verify with the keyboard, not only the mouse; + if it breaks, the `scrollTo()` override is the answer rather than the flag. +- Drag-select auto-scroll rides on the same flag. Selecting past the edge of the + viewport is a real gesture on a list this long. +- The item is cosmetic and must not grow into column-sizing work. The Subject + column being wider than the viewport is the precondition, not the defect. + +## 52. `test_querycompleter` fails under Wayland, passes offscreen + +**Observed 2026-08-07**, while verifying an unrelated change: `ctest` reports +`theDescriptionSurvivesAModestPopupWidth` failing, while running the same binary +directly passes. Reproduced on a clean checkout, so it predates that work. + +**Cause (verified):** the test grabs the completer popup and asserts the image +is not null (`tests/test_querycompleter.cpp:824`). Under the Wayland platform +plugin the grab returns a null pixmap, with the warning + +``` +qt.qpa.wayland: Failed to create grabbing popup. Ensure popup ... has a +transientParent set and that parent window has received input. +``` + +Wayland will not create a grabbing popup for a window that has never received +input, which a test window has not. `ctest` sets no `QT_QPA_PLATFORM`, so it +inherits the session's Wayland plugin, whereas a developer running the binary +by hand usually exports `offscreen` and never sees it. + +**Why this matters more than one red test.** The suite's result depends on how +it is invoked. A test that only passes under a platform plugin nobody sets is +not protecting anything, and worse, it trains you to read a real failure as +environmental. It cost a wrong diagnosis on the day it was found: the failure +was initially attributed to the change in flight, because the clean-tree +comparison was run under `offscreen` while `ctest` ran under Wayland. Comparing +two environments and reading the difference as a regression is exactly the +mistake this item exists to stop repeating. + +**Approach.** Pin the platform for the tests that need one rather than for all +of them, the same shape as item 46: + +- `set_tests_properties(... ENVIRONMENT QT_QPA_PLATFORM=offscreen)` for the + tests that grab widgets, so `ctest` is deterministic however the session is + configured. +- Preferable if it works: give the popup a `transientParent` and show the parent + first, which fixes the grab under Wayland rather than avoiding it. Try this + before reaching for the environment override, since a test that can run under + the real plugin is worth more than one that opts out. + +**Constraints.** + +- **Do not simply delete the assertion.** It exists because a null grab is + precisely how this class of rendering test silently passes, which `CLAUDE.md` + records at length. Weakening it to `if (!shot.isNull())` would make it pass + everywhere and check nothing. +- Whatever is chosen must hold for `test_mainwindow` and `test_messageview` too, + which create widgets and could grow the same dependency. + +### Outcome (done 2026-08-07) + +**The warning named the wrong cause, and the real one was worse.** Wayland's +"failed to create grabbing popup" message points at a transientParent, so the +plan above proposed setting one. Instrumenting the test first showed what +actually reaches the assertion: the popup viewport measured **1278x0**. The zero +height is why the grab returned a null pixmap, but the 1278 is the important +half. This test sizes a line edit to 550px and exists to prove the description +survives a popup that size; under Wayland it was handed a popup more than twice +that wide, so a working grab would have measured a different popup and **passed +while proving nothing**. Offscreen gives 548x40, the geometry the test means. + +So pinning the platform is the correct fix rather than the cheap one, and the +preference recorded above was based on a misreading. Deterministic geometry is a +requirement of the test, not a convenience: a compositor is entitled to size a +popup how it likes. + +- `set_tests_properties(... ENVIRONMENT QT_QPA_PLATFORM=offscreen)` is applied + in `add_qtmaildir_test()`, so it covers every test including `test_mainwindow` + and `test_messageview` as the constraint required, and any test added later. +- **The test also guards its own geometry now**, since the CMake setting only + covers `ctest` and the binary is often run directly. It asserts the viewport + has a non-zero height and is no wider than 700px, each with a message naming + the cause. A bare `QVERIFY(!shot.isNull())` reported nothing useful; the guard + now says "popup viewport has no height (1278x0)". +- Both guards were **verified by mutation**: widening the line edit to 1200px + makes the width guard fail with its explanation, where before the change that + case would have passed. + +## 53. Message rows still read as a table, not as a conversation + +**Observed (user, 2026-08-08)**, on the finished item 20: *"I'm not very +convinced about this session's work. I don't think the table view fits our +use."* Said after the expander, the indent, the thread spine, the tint and the +dimmed text were all in and working, so it is not a report that a cue is +missing. It is a judgement on the result. + +**This is a design finding, not a defect.** Item 20 shipped exactly what its +four decisions specified and every one of them was the user's own choice. The +work is sound; what it produced is not what was wanted. Recording it as a defect +would misattribute the cause, and recording nothing would leave the next session +building on a design the user has already rejected. + +**Cause (verified in code, 2026-08-08).** A message row fills the SAME five +columns as a thread row: `ThreadListModel::data` answers `DateColumn`, +`AuthorsColumn` and `SubjectColumn` for message rows in the same switch that +answers them for threads. One model, one column grid, both row kinds. So every +reply lands on the same rigid column boundaries as the threads around it, and +the eye reads columns before it reads indentation or tint. + +**Line numbers deliberately omitted.** That code is on the branch +`item-20-message-rows`, not on master, where the same lines are unrelated. To +read it: `git show item-20-message-rows:src/threadlistmodel.cpp` and find the +`isMessageRow(index)` branch of `data()`. + +Compare the reference the user gave. In the Thunderbird screenshots the reply +rows carry sender and date only, laid out freely on a plain band, with no column +rules running through them. The structure comes from the ABSENCE of the grid, +which is the one thing three added cues cannot supply. + +Two aggravating details, both visible in the 2026-08-08 screenshots: + +- Every reply repeats `Re: <the thread's subject>`, near-identical down the + whole block, which is exactly the visual signature of a table of records. + Dropping the redundant prefix was offered and not chosen; it is worth + revisiting first because it is the cheapest of these by a wide margin. +- Reply rows keep the same row height as thread rows, since + `setUniformRowHeights(true)` is required for the tag strip's band arithmetic + (on the branch, in `MainWindow`'s view setup). A conversation view would want + tighter replies. + +**Approach: unspecified, and deliberately so.** Ask the user what to change +before proposing anything, exactly as item 20 required. The plausible directions +differ enormously in cost and are not interchangeable: + +1. **Span the columns for message rows.** Draw a reply as one free-form band + (sender, date, no grid) rather than as cells. `QTreeView::setFirstColumnSpanned` + does this per row without a second model. Cheapest real change, keeps + everything else built. +2. **Drop the `Re:` prefix and shrink what a reply shows.** XS on its own, and + worth trying before anything structural. +3. **Two different row shapes.** A delegate that paints a reply row entirely + itself, ignoring the columns. More control, and it fights `uniformRowHeights`. +4. **Abandon message rows in the list** and revisit the `<details>`-per-message + design in the message pane, which was offered at size S on 2026-08-08 and + declined in favour of this. Item 20's work would largely be reverted. + +**Constraint that shapes all of them.** The tag strip is why `ThreadListView` +exists, and its band arithmetic assumes a uniform row height and a known column +layout. Anything that varies row height or removes columns for one row kind has +to answer for the strip on thread rows, which must not change. + +**Specified 2026-08-09.** The user's answer was that the grid is wrong for the +WHOLE left pane, not only for reply rows, which is wider than any of the four +directions above. Threads and replies both become cards in a single column, with +no column grid at all. The design is at +`docs/superpowers/specs/2026-08-09-card-list-design.md`; read that rather than +this entry, which records only the finding. + +Two things settled here that the directions above got wrong. Direction 2's `Re:` +prefix removal is folded in rather than tried first, since the grid goes anyway. +And the constraint about the tag strip inverts: the strip is not something the +design has to answer for, it is deleted, because `ThreadListView` exists ONLY to +paint across columns that no longer exist. This item is a net removal of code. + +**Item 51 is resolved by this design as a side effect** and should not be worked +separately. Cards are exactly viewport width, so the pane has no horizontal +scroll range for a click to scroll into. + +## 54. A cron sync carries the edits but the count still says pending + +**Observed (user, 2026-08-08):** "If I apply some edits in the program, then the +sync runs from crontab, my edits should go through the sync, instead I still see +`N changes pending`." + +**Cause, verified in code.** The edits really do go out; only the count is +wrong. A tag edit reaches the notmuch index at edit time, so a `notmuch new` +fired by cron carries it to the mail store exactly as a local sync would. But +the pending count is cleared in **one place only**: the success branch of the +local sync-finished handler, `m_pendingTagEdits.clear()` at +`src/mainwindow.cpp:1719`, reached from the `MailSync` process this window +started. + +The external path never touches it. `onExternalSyncStateChanged()` +(`src/mainwindow.cpp:1852`) is driven by `SyncMonitor` watching the lock in +`/proc/locks`. On `State::Idle` it clears `m_externalSyncBusy`, calls +`flushHeldEdits()`, and shows "Background sync completed. Press Enter in the +query bar to refresh." (`src/mainwindow.cpp:1901`). It does not clear +`m_pendingTagEdits`, does not reset `m_unnettablePendingEdits`, and does not +drain `m_editedAccounts`. So the indicator keeps counting edits that have +already shipped, until the user runs a sync from the window. + +This is a **defect, not an enhancement**, and it is the same class as item 28: +the indicator exists to answer "is my work safe to quit on", and here it says no +when the answer is yes. It also feeds the exit prompt (`pendingEditCount()` at +`src/mainwindow.cpp:173`), so the user is asked to sync on quit for work that a +cron run already carried. Adjacent to item 49, which made the account set drive +which channels a sync runs: `m_editedAccounts` is stale in exactly the same way. + +**Approach.** Clear the same three pieces of state on an observed external +`Idle` that a successful local sync clears. Two things make this harder than +copying the block, and both must be answered before it is written: + +- **The monitor sees a lock, not an outcome.** A cron run that fails releases + the lock exactly as a successful one does, and the comment at + `src/mainwindow.cpp:1715` records the rule that only a *successful* sync may + clear the count. An external run's exit status is not observable from + `/proc/locks`. Either the count is cleared optimistically on any external + release, or `mailsync.sh` grows a status file the window can read. Ask before + choosing: the optimistic version can clear a count whose edits a failed cron + run did not carry. + + **Resolved 2026-08-09, and the dilemma was false: the script already writes + the outcome.** Every run ends with a + `===== RUN END: <ts> status=OK =====` or `status=FAILED mbsync=<n> + notmuch=<n>` banner in its log (`assets/mailsync.sh:115-117`), which survives + the process that wrote it. `MailSync::lastRunOutcome()` reads it, so neither + the optimistic clear nor a change to the sync script was needed. The user + chose this over both alternatives. +- **The held-edit race is already solved for the local path and must hold + here.** `flushHeldEdits()` is called on the external `Idle` branch too, and it + calls `sendThreadTagChange()`, which writes `m_editedAccounts` + **synchronously**, while the pending map is written on the worker's queued + reply. The local path handles this by snapshotting `m_editedAccounts` before + the flush (`src/mainwindow.cpp:1711`) and subtracting only the snapshot. + Anything written here has to do the same, or edits made during the cron run + are marked as carried by the sync that did not carry them. + +**Constraints.** + +- `State::Unknown` must not clear anything. It means `/proc/locks` could not be + read, so nothing was observed, and the existing code is careful to distinguish + that from `Idle`. +- The local path must keep working unchanged, including the `m_localSyncHoldsLock` + early return at `src/mainwindow.cpp:1874`, which exists so a local sync's own + lock release is not mistaken for an external one. + +**Verification.** `test_mainwindow` can drive `onExternalSyncStateChanged()` +directly, which is how the existing external-sync tests work, so this does not +need a real cron run. Assert on the count and on the exit prompt, and mutate: +the test must fail if the clear is removed. Item 49's account-set behaviour +needs its own assertion, since a count that reaches zero while +`m_editedAccounts` stays full would look correct and still sync the wrong +channels. + +**Built 2026-08-09.** `SyncOutcome` and `MailSync::lastRunOutcome()` parse the +banner; `Config::syncLog()` supplies the path, defaulting to the script's own +and overridable through a new `[sync] log` key so a test never reads the +developer's real log. `onExternalSyncStateChanged()` clears the map, the +unnettable counter and the account set on `Idle` **and** a definite `OK`, before +`flushHeldEdits()`, matching the local path's ordering. 13 tests across +`test_mailsync`, `test_config` and `test_mainwindow`; 15/15 binaries green. + +Two notes on the verification, both worth more than the passing count: + +- **A timing probe endorsed a tail read that was not happening.** The first + version of `lastRunOutcomeReadsATailOfAHugeLog` required the call under + 100 ms, and it **passed with the seek deleted**, because reading 10 MB is + fast either way. It measured nothing, exactly as CLAUDE.md's rendering-probe + entry describes. Replaced with an assertion on content: a marker reachable + only from the head of a large file must be invisible to a tail read, plus a + guard appending a marker within reach to prove the parser can still find one. + That version fails when the seek is removed. +- **Every fixture was invented, and the first batch was wrong.** They wrote the + banner as `RUN END: 2026-08-09 10:20:03`, where the script uses + `date -Iseconds` (`assets/mailsync.sh:111`) and so emits + `2026-08-09T09:20:33+02:00`. The tests passed anyway, because the parser keys + on the `===== RUN END:` prefix and the `status=` token and never looks at the + timestamp. Found only by reading the user's real log while waiting for a cron + run, not by any test. A fixture invented to match the code tests the code + against itself. `lastRunOutcomeReadsABannerTheScriptActuallyWrote` now builds + the line by running `date -Iseconds` the way the script does, and the parser + was confirmed against the real `~/.local/state/mailsync.log`, which it reads + as `Ok`. +- **The ordering claim is not covered by any test.** With no lock file present + `aSyncHoldsTheWriteLock()` is false throughout the suite, so `flushHeldEdits()` + is a no-op and moving the clear after it changes no result. The ordering is + inherited from the local path rather than independently verified; a test for + it needs a held lock, the way item 37's tests stage one. + +## 55. In a narrow window the message pane is invisible + +**Observed (user, 2026-08-08):** "when opening in a squared window, the left +pane takes the whole width (right pane almost invisible)". + +**Cause, verified in code, and NOT what this item first recorded.** The +original entry blamed the thread view's size hint, computed as the sum of its +fixed column widths (~886px) against a `setStretchFactor(1, 2)` with no leftover +space to distribute. Measured, the hint is **256px**: a `QTableView` does not +put its column sum in its size hint, so that mechanism never applied and a +freshly built window splits correctly. + +The real trigger is the **restore**, not the first run. A splitter position is +saved in pixels (`window/splitter`, `src/mainwindow.cpp:120-124`), and the +user's own state file held `1285/1252`, saved from a wide session. Reopened at +1136px, `QSplitter::restoreState()` honours the first pane's 1285 verbatim and +gives the second whatever is left, which is **29px**. That is the sliver in the +screenshot. It gets worse the wider the window ever was, which is why item 1's +persistence is where this comes from and why widening the default window would +have changed nothing. + +**Approach, as built.** A `setMinimumWidth()` on the message view plus +`setCollapsible(1, false)`, and nothing else. A restore-time repair was written +first, running from `showEvent()` because the splitter has no laid-out width +until the window is shown, and was then **deleted**: with the floor in place it +was mutation-tested to be redundant, since the minimum width constrains +`restoreState()` as much as it constrains a drag. Two mechanisms for one fault +is one too many. + +**Constraints.** + +- Must not fight the restore path. A `setSizes()` call that runs after + `restoreState()` would throw away the user's saved position, which is item 1. +- A minimum width on the thread view as well would reintroduce the same problem + from the other side. Only the message pane needs one. +- `resize(1200, 800)` at `src/mainwindow.cpp:206` is the default only when no + geometry is saved; the reported case is a user-sized window, so widening the + default fixes nothing. + +## 56. No action carries an icon, so the toolbar reserves space for nothing + +**Observed (user, 2026-08-09):** "icons inconsistency. All buttons should have +them, at least in the main window interface. Better if also the popups have +them." And: "Buttons should honor the 'Icon only' option." + +**Cause, verified in code.** The mechanism exists and is used; it simply does not +cover most actions. `src/mainwindow.cpp:902-919` holds a `themeIcons` table +mapping action names to freedesktop icon names and calls `setIcon()` for each, +guarded so a name the theme lacks leaves the action with text alone. It covers +**eight** actions: `sync`, `archive`, `delete`, `undo`, `spam`, `flag`, `quit`, +`focus_query`. + +Everything else registered through `MainWindow::addAction()` +(`src/mainwindow.cpp:598`) gets none, and there are roughly twenty of them: +`toggle_unread`, `mark_all_read`, `edit_tags`, `select_all`, `clear_pane`, +`clear_selection`, `zoom_in`, `zoom_out`, `zoom_reset`, `toggle_html`, +`load_remote`, `message_details`, `complete_query`, `next_thread`, +`prev_thread`, `open_thread`. That is the reported inconsistency: adjacent +entries in one menu, some with an icon and some without, which reads worse than +none having one. + +The toolbar is `setToolButtonStyle(Qt::ToolButtonTextBesideIcon)` +(`src/mainwindow.cpp:948`), so an action without an icon lays out an empty slot +beside its text. + +**The names are not the obstacle.** Probed against this desktop's theme +(`Material-Black-Plum-Suru`) with a throwaway Qt program: the eight in use all +resolve, and so do `mail-mark-unread`, `view-refresh`, `edit-select-all`, +`zoom-in`, `zoom-out`, `zoom-original`, `tag`, `mail-message-new`, `edit-clear` +and `help-about`. So the missing icons are missing because no name was assigned, +not because the theme lacks art. On another desktop the guard already handles a +name that does not resolve. + +**The "Icon only" half is separate, and is a genuine override.** Qt takes the +desktop's toolbar button style from the platform theme, and the hardcoded +`setToolButtonStyle()` at line 948 **overrides** the user's choice, so an "Icon +only" setting cannot take effect. Honouring it means dropping that call, or +reading `QApplication::style()->styleHint(QStyle::SH_ToolButtonStyle)` instead of +asserting a style. This is the smaller of the two halves and is independent of +the first. + +**Dialogs are a third case.** `QDialogButtonBox` standard buttons take their +icons from the platform style, so whether Ok and Cancel carry one is a style +question rather than something this code decides. + +**Approach.** Extend the existing `themeIcons` table to the remaining actions, +and stop overriding the toolbar button style. No new mechanism is needed; the +table and its null guard already do the work. + +**Constraints.** + +- Keep the null-icon guard. It is what makes a theme without a given name + degrade to text rather than to an empty slot. +- Prefer theme names over shipped art. Per-action bitmaps in `src/resources.qrc` + would grow the package for something the desktop already provides; that file + currently carries only the application icon and two subsetted fonts. +- Item 3 (done) added the buttons and menu entries. This is decoration on top of + that: no action should be added, removed or rebound here. +- `flag` already maps to `mail-mark-important`, which agrees with item 57's + proposed rename. Do the two together if 57 is picked up. + +**Verification.** `test_mainwindow` can assert that every action in `m_actions` +has a non-null icon. Note what that test does and does not prove: it passes only +on a machine whose icon theme resolves the names, so it is an assertion about +this desktop as much as about the code, and it says nothing about whether the +icon chosen is the *right* one. + +**Built 2026-08-09, with item 57.** The `themeIcons` table now covers all 24 +registered actions; the fifteen names added were probed against a live theme +before being written, not taken from the freedesktop spec on faith. The null +guard is kept, so a theme missing a name still degrades to text. + +`setToolButtonStyle()` now reads `QStyle::SH_ToolButtonStyle` instead of +asserting `TextBesideIcon`. Removing the call entirely was considered and +rejected: verified empirically that a bare `QToolBar` defaults to +`Qt::ToolButtonIconOnly` rather than to the platform hint, which would have +ignored the user's setting just as thoroughly in the other direction. + +**This changes the toolbar's appearance on the developer's desktop.** Its hint +reads `0` (`ToolButtonIconOnly`), so the toolbar shows icons without text where +it previously showed both. That is the setting being honoured, which is what the +note asked for, but it is a visible change rather than a silent one. + +Two tests: `everyActionCarriesAnIcon`, which iterates +`KeyMap::knownActions()` and names every action missing one (it reported all +sixteen before the change), and +`theToolbarDoesNotOverrideTheDesktopButtonStyle`. The first carries a guard +asserting the window really registered its actions, so it cannot pass by +iterating an empty list. + +## 57. "Flag" would read better as "Important" or "Starred" + +**Observed (user, 2026-08-09):** "Flagged to be renamed as 'Important' or +'Starred', with a ⭐ as icon." + +**Cause.** Naming, not a defect. The action is created as `tr("&Flag")` at +`src/mainwindow.cpp:684` and applies the tag through +`tagSelected({ QStringLiteral("flagged") }, {}, tr("Flag"))` at +`src/mainwindow.cpp:686`, the second `tr("Flag")` being the undo-stack +description the user sees in the Undo entry. + +**The rename must not reach the tag.** `flagged` is a notmuch tag, and notmuch +tags are wire format: `src/types.h:37` tests for it, `src/tagcolors.cpp:31` +colours it, `src/threadlistmodel.cpp:145` lists it among the tags a row shows +another way, and the user's own `neomutt` and saved queries refer to it. Renaming +the tag would rewrite the mail store and desynchronise every other tool. Only the +**label** changes: the action text, the undo description, and any prose that says +"flag". + +**The star already exists.** `ThreadListModel::flagGlyph()` +(`src/threadlistmodel.cpp:62`) returns a solid star, with a `*` fallback when the +system font cannot draw it, and the flag column already paints it +(`src/threadlistmodel.cpp:213`). So the icon half of the note is shipped for the +list; what is missing is an icon on the *action*, which is item 56's business. + +**Approach.** Pick one of "Important" or "Starred" and change the action text, +the undo description, and the keyboard-shortcut reference entry. Ask the user +which word: they offered two and the choice is theirs, and it should agree with +whatever the icon depicts (a star suggests "Starred"). + +**Constraints.** + +- Every changed string is inside `tr()` and must stay there. +- The accelerator `&F` is taken from "Flag" and a new word needs a new one. The + Message menu currently holds `&Archive`, `&Delete`, `Mark &spam`, `Toggle + &unread`, `Mark all &read`, `Edit &tags`, `&Flag` + (`src/mainwindow.cpp:864-871`), so **"Important" can take `&I` freely, while + "Starred" collides with `Mark &spam` on `&S`** and would need a letter from + inside the word. A small argument for "Important". +- `src/tagdialog.cpp:211` mentions `flagged` in a comment about token completion. + That is the tag, not a label, and must not be touched. + +**Built 2026-08-09, with item 56.** The user chose "Important". Changed: the +action text to `tr("&Important")`, its status tip, the undo description to +`tr("Mark important")`, and the flag column's tooltip +(`src/threadlistmodel.cpp:192`), which still read "Flagged". The README's +keybinding row and tagging prose followed. + +Unchanged, deliberately: the action **name** `flag`, which is the key a user +writes in `[keys]` and whose rename would silently break every existing +binding; the tag `flagged`; `ThreadSummary::isFlagged()`; the `[tagcolors]` +entry; and the `Flagged = tag:flagged` saved query in the README's sample +config, which is a user's own query name rather than one of our labels. + +`Ctrl+I` was already the binding, which happens to fit the new word. + +`theImportantActionStillWritesTheFlaggedTag` is the test that matters: it +triggers the action and asserts on the tag the model actually received, with a +guard proving the thread did not already carry it. Mutating the tag to +`important` fails it, and also fails two pre-existing held-edit tests, which is +independent confirmation that `flagged` is load-bearing across the suite. + +## 58. `message_zoom` documents a 0.5 to 3.0 range and enforces none of it + +**Observed:** not by the user. Found on 2026-08-09 while adding +`toolbar_icon_size`, by reading `message_zoom` as the model for a bounded +numeric key and noticing it is not bounded. + +**Cause, verified in code, and narrower than this item first recorded.** The +entry claimed the value is applied unclamped and "the first render is +unusable". It is not. `MessageView::clampZoom()` (`src/messageview.cpp:716`) +bounds every value to `kMinZoom`/`kMaxZoom`, which are 0.5 and 3.0 +(`src/messageview.h:96-97`), exactly the README's numbers, and +`MainWindow::restoreUiState()` routes the config value through +`setZoomFactor()`, so `message_zoom = 500` renders at 3.0. + +What is genuinely missing is the **report**. `src/config.cpp` parses with +`toDouble()` and, on success, assigns without comment, so nothing ever tells +the user the 500 in their file is not what they are looking at. That silence is +what the item's own Constraints section asks for. + +**Approach, as built.** Report only, no second clamp. `MessageView` owns the +bounds and already enforces them; a copy in `Config` would be free to drift +from the one that does the work, so `config.cpp` includes `messageview.h` and +reports against `MessageView::kMinZoom`/`kMaxZoom`. This is where it differs +from `toolbar_icon_size`, which has no widget-side enforcement to defer to. + +**Constraints.** + +- Report rather than silently clamp. Silence is how this went unnoticed: the + key parses, so nothing ever said the value was not being honoured. +- Do not extend this to `mark_read_delay_ms`, whose zero and negative values are + meaningful and deliberately unclamped (`src/config.cpp:96-102`). + +**Verification.** `test_config` already has the pattern in +`toolbarIconSizeIsClampedAndReported`; mirror it. Note that +`messageZoomDefaultsAndValidates` exists and passes today, so it is asserting +only on the parse and not on the range. + +## 59. Archive and Mark all read shipped with the same icon + +**Observed (user, 2026-08-09), against 0.12.0:** "Archive" and "Mark all read" +share the same icon, and in an icon-only setup they are not distinguishable. + +**Cause.** Introduced by item 56, in this session. The `themeIcons` table gave +`archive` the name `mail-mark-read` (it predates item 56, from when only eight +actions had icons and `mark_all_read` had none), and item 56 then assigned +`mail-mark-read` to `mark_all_read` as well without checking the table for +duplicates. Twenty-four entries were added or reviewed by hand and this one +overlap was not noticed. + +It only became visible because of the other half of item 56. While the toolbar +forced `TextBesideIcon` the label disambiguated the two buttons; once it follows +a desktop set to icon-only, the icon **is** the whole control, and two buttons +whose consequences differ (`archive` removes `inbox` from the selection, +`mark_all_read` removes `unread` from the entire view) looked identical. + +**Fix.** `archive` now uses `mail-archive`, which is also the more accurate +name: `mail-mark-read` describes read state, which is what `mark_all_read` +does, not what archiving does. + +**The verification is the point of this entry.** A test for the reported pair +would have been worthless, since the defect is the class and not the instance: a +hand-written table of twenty-four names has more plausible duplicates in it. +`noTwoActionsShareAnIcon` compares every action against every other and names +any pair that matches. Two details matter: + +- It compares `QIcon::cacheKey()`, not the theme name, which the window does not + keep. Two *different* names that resolve to the same art on some theme are + equally ambiguous on screen, and that is what the user actually sees. +- It carries a guard requiring every action to have an icon before comparing. + On a theme that resolves nothing, every icon is null, the loop body never + runs, and the assertion would pass having compared nothing. + +Mutation-checked by introducing a *different* collision (`zoom_out` pointed at +`zoom-in`); the test named that pair rather than the one it was written for. + +**Also worth recording: the icon-name probe endorsed the wrong thing.** Item 56 +verified that every name resolves to non-null art, and that check passes +happily for two names resolving to the *same* art. Resolving and being +distinguishable are separate properties, and only the first was tested. The +candidate replacement was therefore checked by rendering both icons at 24px and +comparing the images, not by asking whether the name existed. + +## 60. Next thread dead-ends on the last reply of an expanded thread + +**Observed (found while specifying 53, 2026-08-09), not user-reported.** On the +`item-20-message-rows` branch, with a thread expanded and the last reply +selected, `next_thread` (Ctrl+J) does nothing. It should move to the next +thread. + +**Cause (verified in code).** `mainwindow.cpp:644-655` implements both actions +as arithmetic on a row NUMBER: + +``` +const int row = current.isValid() ? current.row() + 1 : 0; +if (row < m_model->rowCount()) + m_threadView->selectRow(row); +``` + +A `QTableView` numbers rows once for the whole view, so this was correct before +item 20. A tree numbers them **per parent**: the last reply of a thread is row +N of that thread, `row + 1` names a sibling that does not exist, and +`m_model->rowCount()` with no argument counts top-level threads rather than the +current parent's children. `prev_thread` fails the mirror case, moving from the +first reply to nowhere instead of to the thread root. + +This is a fresh instance of the rule the branch's own commit message states: +**nothing may be keyed on a row NUMBER**, because a tree numbers rows per +parent. That commit lists it for the tag strip's paint walk. Nobody checked the +navigation actions against the same rule. + +**Approach.** Walk with `QTreeView::indexBelow()` / `indexAbove()` from the +current index, which follow visible rows across parent boundaries. For +thread-to-thread jumping, skip any index whose `IsMessageRole` is true. + +**The cause above is wrong, and was corrected on 2026-08-10.** It was read off +`master`, where the arithmetic really is `current.row() + 1`. The branch does +not do that: `5487d58` added `MainWindow::threadRowOf()`, which walks up to the +containing thread BEFORE the arithmetic, so from the last reply of an expanded +thread `next_thread` already reached the next thread. The defect was fixed in +the same commit that could have introduced it, one commit before this entry was +written. Verified by writing both failing tests first, on the branch, and +watching them pass against unchanged code. + +The entry is kept rather than deleted, because the reasoning it records is +sound and the tests it demanded now exist. It is a reminder that a cause +"verified in code" is only verified against the branch it was read on. + +**Superseded by the card-list work all the same.** Both actions now walk with +`indexBelow`/`indexAbove` (`card-list`, 2026-08-10), so nothing in that path is +keyed on a row number, which is the rule a deeper tree would break next. +Alt+Up/Down were added alongside Ctrl+J/K there. + +**Constraint.** The test that would catch this must start from the **last reply +of an expanded thread**. A test that arrows down a collapsed list passes against +the bug, since with nothing expanded every row is top-level and the arithmetic +is accidentally correct. + +## 61. `test_mainwindow` fails intermittently, about 1 run in 20 + +**Observed (2026-08-10), not user-reported.** A full `test_mainwindow` run +occasionally fails with one or both of: + +- `anActionOnAMessageRowTagsThatMessageNotTheThread`: `pendingMessageIdsForTesting()` + is empty where one id is expected. +- `aSuccessfulCronSyncDrainsTheEditedAccounts`: `work-channel` is still queued + after a successful sync. + +**It predates the card list, and that was measured rather than assumed.** A +worktree at `f72dba9`, the commit before any of this work, failed 3 of 12 runs, +which is a HIGHER rate than the branch's. Neither test was touched by the card +list: `aSuccessfulCronSyncDrainsTheEditedAccounts` arrived in `d213bbf` on +master, and `anActionOnAMessageRowTagsThatMessageNotTheThread` in `7c36486`, +before the redesign began. + +**Not order dependence.** Both pass 15/15 when run alone by name, and a full +suite passed 15/15 immediately after failing twice on the same binary. What +distinguished the failing runs was other work happening on the machine at the +time, which points at timing rather than at leaked state between tests. + +**Cause: established 2026-08-11, and it is the user's own cron sync.** The +trigger is another process holding the mbsync lock while the suite runs, not +machine load: measured 0 failures in 30 runs with no lock held, and 30 failures +in 30 runs with one held. Reproduce deterministically with +`flock /tmp/mbsync.lock -c 'sleep 60'` in one shell and the suite in another. +This supersedes the earlier "not established" reading and the load hypothesis, +which synthetic CPU load had already failed to confirm. + +**Approach.** Item 38 already built the seam: `MainWindow::setLocksPathForTesting` +is a static hook that points the lock check at a path the test controls, so a +test that sets it cannot see the user's real sync. The fix is giving the rest of +the suite that same seam, most likely from a fixture or an init hook rather than +per-test, so a newly added test gets it without having to remember. One of item +71's tests already uses the seam, so there is a worked example to copy. + +**Constraint.** A flaky test is worse than a missing one, because it teaches +everyone to re-run the suite instead of reading it. This one already cost a +false "green suite" report: it fired during the card-list merge check and was +initially mistaken for a regression that change had introduced. + +**Size: S**, most of it in reproducing reliably rather than in the fix. + +### Outcome (done 2026-08-13) + +`TestMainWindow::init()` builds a `QTemporaryDir` per test and points +`MainWindow::setLocksPathForTesting` at an empty file inside it, so no test +reads the real `/proc/locks`. An empty table is the honest representation of +"no sync is running"; the three tests that want to observe a sync write their +own content, as they already did. + +**The three existing users of the seam each restored `"/proc/locks"` when they +finished, and that restoration was itself a defect**: it handed the real table +back to whichever test ran next, so one test opting in re-exposed every test +after it. All three restores are removed, and `cleanup()` deliberately leaves +the path pointing at the temporary file. + +`noTestCanSeeTheRealLockTable` guards the fixture, since a suite that silently +reverts to the real table would go back to failing for reasons no assertion +mentions. + +Verified rather than assumed, using the reproduction above. With +`flock /tmp/mbsync.lock -c 'sleep 30'` held: 3 failures before +(`aRefreshDoesNotStampOverASelectionMessage`, +`anActionOnAMessageRowTagsThatMessageNotTheThread`, +`aSuccessfulCronSyncDrainsTheEditedAccounts`), 119/119 after, and the full suite +19/19 with the lock held. Mutation-checked by disabling the fixture: the guard +fails first with its diagnostic, and a real test fails behind it. + +## 62. No config option for the date format on a card + +**Observed (user, from the notes):** "option in config file for date format". + +**Cause, verified in code.** `CardLayout::formatDate()` +(`src/cardlayout.cpp:24-30`) is a single unconditional line: +`QLocale::system().toString(date, QLocale::ShortFormat)`. It reads no setting, +takes no format argument, and is the only date formatter on a card +(`src/carddelegate.cpp:176` is its only caller besides +`CardLayout`'s own `widestDate`). `Config` parses no date key: the `[general]` +keys it reads are `notmuch_config`, `startup_query`, `message_zoom`, +`completion_on_focus`, `toolbar_icon_size`, `sync_on_exit` and +`mark_read_delay_ms` (`src/config.cpp:78-184`). So there is nothing to +configure, not a setting that is being ignored. + +The current behaviour is a deliberate choice rather than an oversight, and the +comment at `src/cardlayout.cpp:26-28` says why: the system short format is what +every other application on the desktop shows, and a mail client that disagrees +looks wrong. This item is about giving the user an override, not about +replacing that default. + +**Approach.** A `[general] date_format` key, empty by default meaning "the +system short format", otherwise a `QDateTime::toString()` pattern. +`CardLayout::formatDate()` takes the format as a parameter rather than reading +`Config` itself, keeping the struct free of dependencies the way it already is, +and `CardDelegate` passes it down. + +**Constraints.** + +- **`CardLayout::widestDate()` reserves the date column's width and must agree + with whatever the format produces**, or a long custom pattern is elided or + overlaps. It currently computes the widest string the short format can return; + with a custom pattern it has to measure that pattern instead. +- An unparseable or absurd pattern must not blank the date. `toString()` with a + pattern containing no field returns the pattern itself verbatim, so validate + in `Config` and fall back to the system format with a problem reported, the + way `message_zoom` and `toolbar_icon_size` already do. +- Document all three states in the README's `[general]` block: absent, empty, + and a pattern. + +**Size: XS.** One key, one parameter, one width calculation. + +### Outcome (done) + +Built as specced: `[general] date_format`, empty by default, passed down as a +parameter rather than read inside `CardLayout`. Three things worth recording. + +- **The format reaches the LAYOUT, not only the painter.** It sits on + `CardLayout::Input`, because `compute()` reserves the date's width from + `widestDateSample()`. A pattern that reached only the `drawText` call would be + elided into a rect sized for the system format, which is the same clipping + the bold-font fault produced. The test asserts both halves and was confirmed + by mutation: making the width ignore the format fails it. +- **`widestDateSample()`'s static cache had to go.** It memoised one sample, so + whichever format arrived first would have sized every later one. It is now a + plain call, at the cost of one `QLocale` lookup per row, which is what + formatting the date itself already costs. +- **Validating a pattern is harder than it looks, and the first test fixture + was wrong.** `toString()` treats nearly every letter as a field, so `banana` + formats as `bpmnpmnpm` (`a` is AM/PM, `n` the minute) and `hello` as `22ello`. + Those are nonsense but they vary with the instant, so a "does this contain a + field" check cannot reject them and should not pretend to. What `Config` + rejects is the case that actually harms: a pattern whose output is CONSTANT, + found by formatting two different instants and comparing. `xyz` is such a + pattern and is what the test uses. + +## 63. No way to see sent mail, and no filter for it + +**Observed (user, from the notes):** "Sent mail filter". + +**Cause, verified in code, and it is not one missing feature but two.** + +- **Nothing in the codebase knows what "sent" means.** `Account` carries + `name`, `address`, `maildir`, `drafts`, `label`, `channel` and `color` + (`src/config.cpp:254-270`); there is no `sent` field, and no query anywhere + composes one. `Account::scopedQuery()` (`src/config.cpp:42-45`) scopes by + `path:"<maildir>/**"`, which covers a sent folder only in the sense that it + covers everything in the account. +- **The saved-query mechanism could express it today and nothing ships one.** + `[queries]` is read wholesale from `childKeys()` (`src/config.cpp:291-297`), + so a user can already write `Sent = tag:sent` by hand. The README's example + block (`README.md:178-181`) offers Inbox, Unread and Important and no Sent, so + nothing points the user at it. + +Which of the two this item is depends on a decision the notes do not make: +whether "sent" is a **notmuch tag** the user's own filters apply, or a +**maildir path** per account. If it is a tag, this is a documentation and +defaults change and it is XS. If it is a path, `Account` needs a `sent` key +beside `drafts`, and the query has to be composed per account, which is where +the S comes from. + +### Answered and specified 2026-08-11 + +**It is a PATH, not a tag**, so the XS branch above is dead. Measured against +the user's own database: no `sent` tag exists at all, every account keeps sent +mail in a folder, and the folders disagree across three shapes, with one +account having no sent folder whatsoever. That is what forces a per-account key +rather than a `<maildir>/Sent` convention. + +The design is at `docs/superpowers/specs/2026-08-11-sent-mail-design.md`; read +that rather than this entry, which records only the finding. It carries the +measured folder table, the user's four decisions, and the constraints, of which +three are worth knowing before opening it: the bracketed provider paths contain `[` and `]` +and are Xapian syntax, so quoting is load-bearing; notmuch has no recipients +call at any level, so the To summary is folded per message in the worker under +the thread-ownership rule; and GMime's address parser returns NULL for an empty +string. + +One thing settled there that reverses nothing: item 2 refused a `To:` line on a +thread header and that ruling stands. It was scoped to a MIXED conversation, +where the union of recipients misdescribes itself as "To:". A Sent view is +one-directional, so the ambiguity it avoided is absent and recipients on the +card are well posed. + +**Size: M**, revised up from S. The query half is the S scoped here; the +recipients half is a new `ThreadSummary` field, a worker-side per-message walk, +the first GMime address parsing in this codebase, and a card that has to know +which view it is in. + +## 64. The Sync button carries a mailbox icon, not a refresh one + +**Observed (user, from the notes):** "the sync button should show the 'refresh' +icon". + +**Cause, verified in code.** The theme-icon table in `MainWindow` maps +`sync` to `mail-receive` (`src/mainwindow.cpp:1000`), which is a mailbox glyph +with an arrow into it. Every other entry in that table is a single line of the +same shape, so this is one string. + +The neighbouring comment (`src/mainwindow.cpp:1001-1003`) records why `archive` +was moved off `mail-mark-read` in 0.12.0: with the toolbar icon-only, the icon +IS the control, and two buttons with different consequences must not look +alike. That reasoning applies here in reverse. `view-refresh` is the standard +freedesktop name and reads as "fetch again" at a glance. + +**Approach.** Change the one mapping to `view-refresh`. The lookup already +guards on `QIcon::fromTheme` returning null (`:1034`), so a theme lacking the +name leaves the action iconless rather than broken. + +**Constraints.** + +- **Check it does not now collide with another action's glyph.** No entry in + the table currently uses `view-refresh`, but the 0.12.0 defect was exactly a + collision, so confirm against the rendered toolbar rather than the table. +- Icon-only is the desktop's choice, honoured through `SH_ToolButtonStyle` + (`:1073`), so the icon may be the only label the user ever sees. + +**Size: XS.** One string. + +### Outcome (done) + +One string: `mail-receive` became `view-refresh` (`src/mainwindow.cpp:1000`). + +The collision the constraint asked about is covered by a test that already +existed, `noTwoActionsShareAnIcon`, which passes. That is a better check than +looking at the toolbar, since it covers every action rather than the handful +currently on it. + +## 67. The placeholder pane counts unread, flagged and inbox, but not sent or drafts + +**Observed (user, from the notes):** "stats in the blank pane should show also +drafts and sent emails." + +**Cause:** `kPlaceholderQueries` drives three counts and the labels are written +positionally against them, `%n unread`, `%n flagged`, `%n in inbox` +(`src/mainwindow.cpp:1410-1412`). The worker's `requestCounts` takes an arbitrary +list of queries and answers one count per query +(`src/notmuchworker.cpp:621-648`), so the machinery is already general; only the +list and its labels are fixed. + +**Approach:** add two entries. Sent is not `tag:sent`: item 63 established that +the sent query is composed from the configured per-account folders, and +`allSentQuery()` already builds it. Drafts has no such composition yet and the +obvious `tag:draft` should be checked against the real database before it is +shipped, since a count that always reads 0 is worse than no count. + +**Constraints:** the -1 convention is load-bearing. The worker returns -1 rather +than skipping an entry precisely because the caller pairs answers with labels by +position (`src/notmuchworker.cpp:632-634`), so a new entry must be added to the +query list and the label list at the same index. + +**Size: XS.** + +**Done 2026-08-11, shipped in 0.15.0 (72812c0).** `tag:draft` was checked against +the real database as the Approach required and counts 0, with no draft-ish tag +present at all, so both lines are folder-composed rather than tag-based: +`Account::draftsQuery()` and `Config::allDraftsQuery()` now mirror the sent pair, +and the shared body lives in `folderQuery()`/`joinAccountQueries()` so the quoting +and the bare-`or` guard exist once. `kPlaceholderQueries` was deleted rather than +extended: it was the parallel-list arrangement the Constraints section warns +about, and `placeholderLines()` now carries each query beside the callable that +labels it, which removes the pairing hazard instead of documenting it. Measured +4 sent terms over 601 threads, 5 drafts terms over 3 threads; the extra drafts +term is the account configuring `drafts` and no `sent`. + +## 69. `passed` and `replied` read as words where every other state is a glyph + +**Observed (user, from the notes):** "tags like Passed and Replied should use +icons instead." + +**Cause:** both are ordinary tags, drawn as text chips with a built-in colour +(`src/tagcolors.cpp:36-37`) by the same `TagChip` painting helpers as every other +tag. Flagged already went the other way in item 57 and is a star, so the card +mixes one glyph state with two word states. + +**Approach:** treat this as an extension of what item 57 built rather than as new +machinery, a small map from tag name to glyph consulted before the chip painter. +Assert in `CardLayout`, not on a render: a glyph and a chip reserve different +widths, and the date's reserved width is computed from a sample string, which is +the trap item 62 hit. + +**Constraints:** depends on 68 only in that the meaning of `passed` may change +under it. The glyph must survive a card that carries neither tag without leaving +a gap, and the tag must remain readable to a user who does not know the glyph, +so keep the tooltip or the accessible name carrying the word. + +**Size: S.** + +## 70. Pane icons are a private set where the main window uses the system theme + +**Observed (user, from the notes):** "we should rework the icons used in the two +panes, leaving the main UI to use system icons." + +**Cause:** every action icon is resolved through `QIcon::fromTheme` +(`src/mainwindow.cpp:1057`), which is the system theme and is what the user wants +kept. The panes are the other half: the card and the message pane draw their own +marks, and item 57's star and item 15's paperclip arrived independently of each +other and of the theme. + +**Approach:** unspecified in shape until the user says what they pictured for the +panes, but the split they stated is clear and is the constraint worth recording +now: the toolbar and menus stay on `fromTheme`, the panes get a deliberate, +self-contained set that does not change under the user's icon theme. + +**Constraints:** an icon shipped as an asset needs to work on both light and dark +message-pane CSS, which item 12 already made theme-aware. `noTwoActionsShareAnIcon` +covers actions only and will not catch a collision between pane marks. + +**Size: M**, and it overlaps 69, which should probably be done inside it rather +than before it. + +**Done 2026-08-11, with item 69 folded in as the Size note predicted.** Six +marks ship with the application in `assets/icons/marks/`: flagged, attachment, +passed, replied and the two expander triangles. The toolbar and menus still +resolve through `QIcon::fromTheme` and were not touched, which is the split the +user stated. + +**Licensing decided the shapes.** The user pointed at the Material-Black-Plum-Suru +theme as the look they wanted. That set is GPL3 (`index.theme` names Sam Hewitt +and the licence) and this project is GPLv2-ONLY (`src/main.cpp:6`, no "or +later"), which are incompatible: GPLv2's "no further restrictions" clause bars +shipping GPL3 assets in a v2-only work. The user chose to have the six drawn +fresh in the same idiom rather than relicense, so no Suru path data was copied. +The idiom itself is generic: solid single-path silhouettes at 16x16, no strokes. + +**Not a .qrc.** `src/CMakeLists.txt` already records that a qrc compiled into the +static library registers itself from a global initialiser the linker drops, so +resources belong to the executable. The tests link the LIBRARY, so a +resource-based mark would be absent exactly where it needs asserting. The +payloads are compiled in as string literals in `src/marks.cpp`, generated from +the assets, which stay the editable originals. + +**One asset per mark, not one per theme.** Every payload paints with +`fill="currentColor"`, which `QSvgRenderer` does not resolve: it renders black. +`Marks::pixmap` composites the wanted colour with `CompositionMode_SourceIn`, +so a mark takes the card's own pen colour and follows selection and the +read/unread dimming for free. Cached by (mark, size, colour, ratio), since a +delegate repaints these per row per frame. + +**`CardLayout` reserves the rects; `CardDelegate` paints them.** The marks were +glyphs inside the subject STRING, so their width came free from the text +metrics; as icons the geometry has to know they exist or the subject runs +underneath them. That is why `Input` grew four bools. The same trap bit the +expander pill, whose triangle was a glyph in `expanderLabel()` and now needs its +width reserved explicitly. + +**What the tests could not catch, and the render did.** Every geometry +assertion passed while a card showed `passed` as BOTH an arrow and a green tag +chip: the chip filter had no reason to know a mark had appeared. Found by +rendering real cards to a PNG and looking at it. `isDrawnAsAMark()` is now one +list consulted by both `PillTagsRole` and `MessageOwnTagsRole`, since two copies +drifting is how a tag ends up drawn twice on one row and not at all on another. + +Nine tests in `test_marks` and four in `test_cardlayout`, plus one in +`test_threadlistmodel` for the de-duplication. Mutation-checked at four points: +the subject ignoring the marks, the flag not indenting the subject, the pill +forgetting the triangle's width, and the recolour composite removed. Each failed +a test. The old `flagGlyph()`/`attachmentGlyph()` and their `*` fallback are +deleted; that fallback was a latent defect of its own, since both collapsed to +the same character and made a flagged thread indistinguishable from one with an +attachment. + +## 71. A toolbar action does not sync, so the edit sits until the next cron run + +**Observed (user, from the notes):** "clicking one action in the toolbar should be +synced automatically (maybe after a configurable delay). EG I hit 'mark all read', +the view is updated but I still have to sync manually or wait for the cronjob." + +**Cause:** by design, and the design is recorded. Tag edits reach the notmuch +index at edit time and are held as pending until a sync carries them out to the +server, which is what the unsynced-changes indicator counts (items 18, 28, 54). +Nothing schedules that sync on the user's behalf. + +**Approach:** a debounced timer after a mutation, firing the existing sync path. +Item 49 already narrowed a sync to the accounts that actually changed, so the +automatic one is not the whole-mailbox operation it would have been before that. + +**Constraints and the decision needed.** The delay is the user's call and the +default matters: too short and every keystroke of tagging spawns an mbsync, too +long and it is indistinguishable from the cron job they already have. A sync +started this way must not fight the cron one, `SyncMonitor` watches +`/tmp/mbsync.lock` and the automatic sync has to skip rather than queue when the +lock is held. The undo stack has to survive it, which is what item 35 built. + +**Size: S** once the delay is chosen. + +**Done 2026-08-11.** The delay is 2000ms by default, chosen by the user, and +configurable as `auto_sync_delay_ms` in `[general]`. It follows +`mark_read_delay_ms` exactly, including that zero and negative are not errors: +zero syncs on the next trip through the event loop and any negative value +restores the pre-0.16.0 behaviour, which is the switch for a user who wants only +their cron job. + +Armed from `onTagsApplied`, where a write is CONFIRMED and the pending count is +already current, rather than where one is sent: a sync scheduled for a write the +worker went on to reject would run for nothing. It is a debounce and not a +schedule, restarted by each confirmed edit, because "mark all read" confirms one +write per thread in the view and an arm-per-edit timer would be the storm of +syncs the debounce exists to prevent. Nothing is armed when the delay is +negative, when no sync command is configured, or when the pending count is zero, +which is the case where an edit was netted against its own inverse (item 28). + +The constraints held: `runAutoSync` skips rather than queues when +`m_externalSyncBusy` or a local sync is running, and the edits stay pending +rather than being lost. Item 35's refresh keeps the undo stack. + +Four tests in `test_mainwindow` and four in `test_config`, each mutation-checked: +removing the `scheduleAutoSync()` call, honouring a negative delay, dropping the +nothing-pending guard and dropping the already-running guard each failed a test. +**Follow-up, found by hand testing the same day.** Reading a message in the +Unread view, the automatic mark-read tagged it, the automatic sync fired two +seconds later, and the message pane went blank. The stale-thread notice (item 35) +exists for exactly this and was not the problem: `onSyncFinished` called +`runCurrentQuery()` where the cron path calls `refreshCurrentQuery()`, and a +re-run clears the model, the undo stack and the pane, so there was nothing left +for the notice to describe. The two paths had no reason to differ; before item 71 +a local sync followed only a click on Sync, where blanking was at least +explicable, so the difference went unnoticed. `onSyncFinished` now refreshes. + +Its test asserts on the UNDO STACK, not on the pane. Both paths issue a queued +query that `test_mainwindow` has no worker to answer, so the pane ends up blank +either way and an assertion on it passes against both; the undo stack is cleared +by one and kept by the other, so it names which path ran. Mutation-checked by +restoring `runCurrentQuery()`. + +Two traps met while writing them and worth keeping. The helper first wrote +`general/auto_sync_delay_ms` and the key silently matched nothing, leaving the +default in place, exactly the QSettings `[general]` behaviour recorded in +CLAUDE.md. And a debounce assertion comparing `remainingTime()` before and after +with `>` is FLAKY, since both reads can land in the same millisecond; assert the +remaining time went back up near the full interval instead. + +## 73. This backlog is past four thousand lines + +**Observed (user, from the notes):** "cleanup pass on the backlog in the project. +~4K lines is starting to become a problem." + +**Cause:** every item keeps its full Observed/Cause/Approach section forever, +including the fifty-odd that are done. The rule that would have prevented it +exists now, in "Adding to this document", but it was written on 2026-08-11 for +item 63 and nothing has been applied retroactively. + +**Approach:** the done items are the bulk, and their sections are history rather +than backlog. Move the closed ones out to a companion file, leaving the status +table intact and each section replaced by nothing at all, the table row already +carries the date and the outcome. Where a closed item records a trap that is +still true, that trap belongs in CLAUDE.md, which is where it would actually be +read, and several already are. + +**Constraints:** do not renumber and do not delete. The numbering is referenced +from commit messages, from CLAUDE.md, and from the specs, so a moved section has +to stay findable under its number. + +**Size: S**, and it is bookkeeping, so it competes with real work rather than +blocking it. + +**Done 2026-08-13.** 5056 lines to 578, with 66 sections covering 68 items moved +here. The split was done by script rather than by hand and verified by set +difference: every non-blank line of the original appears in one of the two +files, 0 missing, and the only lines not in the original are this file's header. +The status table stayed in the backlog untouched, so all 80 numbers still +resolve there. + +Two things were fixed that the approach above did not anticipate. **Three +cross-references said "see below" and their targets had just moved**, so rows 60 +and 75 now say "see the closed-items file" and the header's note about item 20's +parked branch says where that entry went. And the real cause was never the fifty +done sections, it was that **nothing moved a section on the day its item +closed**: a one-off cleanup buys a few months and then item 73 returns. The rule +in "Adding to this document" now requires the move on the closing commit, which +is the part that keeps this from recurring. + +## 75. The tagging rules window forgets its size and its column widths + +**Observed.** The rules window opens at the same size every time, whatever +size it was left at, and the rule table's columns reset to their computed +widths on every open. The user also asks whether it should present as a +primary window rather than a popup. + +**Cause.** `TagRulesDialog::TagRulesDialog` calls `resize(760, 520)` +unconditionally (`src/tagrulesdialog.cpp:59`) and never reads or writes a +saved geometry; there is no `saveGeometry`/`restoreGeometry` pair anywhere in +the file, and `MainWindow` is the only class that touches `uiStatePath()` +(`src/mainwindow.cpp:141,183`). The columns reset because `reloadList()` +calls `resizeColumnToContents` for the enabled and stage columns on every +repopulate (`src/tagrulesdialog.cpp:213-214`), and `onCountsReady` does the +same for the count column (`:326`); a width the user dragged is discarded by +the next reload, not only by a close. + +**Approach.** Save `saveGeometry()` and `m_list->header()->saveState()` into +the machine-written UI state file under keys of their own, and restore both +in the constructor, keeping the current `resize` as the fallback for a first +run. Drop the unconditional `resizeColumnToContents` calls once a saved +header state exists, or the restore is undone on the first reload. + +The "popup or primary window" question is a separate decision and not a +defect: the class is a `QDialog` (`src/tagrulesdialog.h:41`), which is what +makes it modal to the main window and what puts it above it. Changing it to a +top-level window means it can be left open beside the main window and can go +behind it, and the rule edits would then need to survive that. Ask before +changing it. + +**Constraints.** UI state goes to `~/.local/state/qtmaildir/uistate.conf` via +`MainWindow::uiStatePath()`, never into the hand-edited config. A geometry +restore under the offscreen platform is what item 46 already tripped over, so +the test asserts on the saved value rather than on the resulting frame. + +**Size: S.** + +**Done 2026-08-13** on `rule-builder`, unreleased, for the COLUMN WIDTHS. +`saveGeometry()` and the list header's `saveState()` go to `tagrules/geometry` +and `tagrules/header` in `uistate.conf`, written on `done(int)` so they survive +Cancel as well as Save; `resize(760, 520)` stays as the first-run fallback. + +**The window SIZE does not come back, and that half of the item cannot be +fixed here.** The user's desktop is Hyprland, a tiling compositor. It tiles the +window to fill its slot, so the size dragged is the tile's; `saveGeometry` +records `frameGeometry` and `normalGeometry` and `restoreGeometry` restores the +NORMAL one, which stays at whatever `resize()` last set. Decoded from the real +state file after a hand test: frame 2248x806, normal 760x664. The code restores +760 faithfully and the window still opens tiled. + +Three wrong diagnoses were tried and each was disproved by a probe rather than +by argument: that `restoreGeometry` rejected the blob as off-screen (it returns +true on the real display; the negative y is the DP-1 origin), that the layout +overrode a geometry set before the first show (a `showEvent` restore produced +the identical size), and that the offscreen test could tell the two apart (it +returns the same frame for both, so the mutation survived). + +Nothing worth building remains unless the user wants the dialog to open at a +remembered size when floated, which needs a Hyprland window rule rather than +code here. + +**The approach above was wrong on one point, and a test caught it.** It said to +drop the `resizeColumnToContents` calls "once a saved header state exists", +which fixes the restore and leaves the original defect standing: with no saved +state, a width the user had just dragged was still discarded by the next add or +delete. The rule shipped instead is that each column is auto-sized ONCE, on its +first fill, after which its width belongs to the user however it was set. Two +flags, because the count column is filled later by a reply from the worker. + +**It then shipped broken once more, and the test that covered it passed.** The +save was written in `closeEvent`, and the test asserted with `close()`. Neither +button goes anywhere near either: Cancel calls `reject()`, Save calls +`accept()`, and only the window manager's X button sends a `QCloseEvent`. So +the size was kept for the one route out of three that the buttons never take, +and the user found it in one try by resizing and pressing Cancel. The save now +overrides `done(int)`, which both buttons funnel through and `close()` reaches, +and the test asserts all three routes rather than trusting one to stand for the +others. A second trap sits underneath: `close()` on a widget that was never +shown returns early without reaching `done()`, so that leg of the test has to +`show()` first or it proves nothing. + +The **popup or primary window** question was put to the user and deliberately +not taken: it stays a `QDialog`. Reopening it needs the unsaved-edit story that +being modal currently sidesteps, and that is its own decision rather than part +of this item. + +## 76. Every field in the rules dialog is free text, so a rule is easy to get wrong + +**Observed.** A rule is written by typing into four line edits, and the user +would rather choose from buttons, radios and completion, with typing reduced +to the parts that genuinely have to be typed. + +**Cause.** Not a defect, this is what shipped. The form is four bare +`QLineEdit`s for id, add, remove and query plus one `QCheckBox` +(`src/tagrulesdialog.cpp:88-96`), and none of them is attached to a +completer. The tag completion machinery already exists as `QueryCompleter` +(`src/querycompleter.h:90`) and is not used here. + +**Approach.** Designed 2026-08-13. **Read +`specs/2026-08-13-rule-builder-design.md` instead of planning from this +entry.** A `RuleQuery` value type parses and compiles the query string, and a +row builder in the dialog edits it, in the shape the user asked for after +showing Thunderbird's filter window: field and operator dropdowns, `+`/`-` +buttons, an all/any radio, and a separate "but not" block. + +Three constraints decide whether to open the spec at all. **The stored format +does not change**, so this is a single-repo change and mailctl needs no edit. +**The query string stays authoritative**, so a rule the builder cannot +represent still opens, saves and runs, in a text mode that every rule +carries. And **the string is rewritten only when the rows actually changed**, +compared against the parsed value rather than tracked with a dirty flag, +which Qt would set during programmatic population. + +Measured against the seventeen real rules: sixteen are flat, one nests an +`or` group inside an `and` chain, which is what the exclusion block exists +for. + +**Completion is the other half of this item** and is independent of the +builder; it can land before or after. It reuses `QueryCompleter` +(`src/querycompleter.h:90`) for tag names on the add and remove fields. + +**Constraints.** A multi-value field must not use `QLineEdit::setCompleter`. +CLAUDE.md records this trap twice over: the line edit overwrites the +completer's prefix with the widget's whole text, so the first tag completes +and nothing after it does. Attach with `QCompleter::setWidget` and drive the +prefix by hand. And a test that uses `setText()` passes against the bug, +because `setText` never drives a completer at all, so the keys have to be +typed. + +The hook refuses to remove `unread` or `inbox`, so a builder that offers +those as removable tags produces a rule that silently does nothing. Say so in +the UI rather than letting the hook decline it invisibly. + +**Size: M.** + +## 77. No way to see what a rule would collect, in the thread list + +**Observed.** The user wants a button that runs the rule's query in the main +window, to look at what it would collect rather than at how many. + +**Cause.** Not a defect. The dialog already counts matches, over +`requestMessageCounts` and `messageCountsReady` +(`src/notmuchworker.h:172`, `src/mainwindow.cpp:1322-1335`), which answers +"how many" and cannot answer "which". Nothing carries a query from the dialog +back to the query bar. + +**Approach.** One signal from the dialog carrying the rule's query string, +and a slot on `MainWindow` that puts it in the query bar and runs it. The +dialog stays open, since the point is to compare the two. + +**Constraints.** The stored query carries no scope on purpose: the hook +supplies `tag:new` and wraps the rule's query in parentheses. A preview must +therefore run the query WITHOUT `tag:new`, or it shows nothing at all outside +a sync window, and it must not add the parentheses silently either, since +what the user is checking is the query as stored. + +A count request must never bump `m_generation`; item 44 already had to add +`m_ruleCountGeneration` for exactly this reason (`src/mainwindow.h:657-664`). +A preview is a real query and does bump it, which is correct, but it also +means the preview discards whatever thread load was in flight. + +**Size: S.** + +**Done 2026-08-13** on `rule-builder`, unreleased. A **Preview in list** +button emits `previewRequested(query)`; `MainWindow::onRulePreviewRequested` +clears the account selector, puts the query in the bar and runs it, then +raises itself. The dialog stays open, which is the point. + +Both constraints above became assertions, and BOTH mutations were needed: a +test that emitted the hook's `tag:new and (...)` wrapping fails, and one that +skips the account reset fails. The second only bites once the test config +actually has an account to select, since the default empty config leaves the +selector on "All accounts" already and the assertion passed against the +mutation until that was fixed. + +## 79. Opening the rules dialog and saving destroys the first rule + +**Observed.** Open Tagging rules, press Save, change nothing. The first rule +in the list loses its query and its tags. It then vanishes entirely the next +time anything reads the file, because a rule with an empty query is dropped +as malformed on load. + +Found while building item 76, by a test written to catch a different problem. +Reproduced against the released tag rather than the branch, with a throwaway +worktree at 9585674 and a two-rule fixture: after constructing the dialog and +calling its save path, the store held one rule instead of two. + +**Cause.** `TagRulesDialog::onSelectionChanged` blocks signals for `m_note` +only. Two lines later, `m_enabled->setChecked(rule.enabled)` emits `toggled`, +which is connected to `applyEditsToCurrentRule()`. That handler writes every +field of the current rule from the widgets, and it runs BEFORE +`m_query->setText(rule.query)` has filled the query widget, so it writes the +previous rule's text. On the first open there is no previous rule and the +widgets are empty, so rule 0 gets an empty query and an empty tag list. +`TagRules::load` then drops it (`src/tagrules.cpp:150`). + +The existing comment above the `QSignalBlocker` shows the hazard was known for +`m_note` and simply not extended to `m_enabled`. A blocker per widget is the +wrong shape: the whole load needs one guard. + +**Fix.** Raise `m_reloading` for the duration of `onSelectionChanged` and +restore it afterwards, replacing the single-widget blocker. `m_reloading` +already exists for exactly this class of problem and +`applyEditsToCurrentRule()` already honours it. Also take the rule by value +rather than by const reference: the reference points into `m_working`, which +the handler mutates, so it could be read back half overwritten. + +Fixed on the `rule-builder` branch as part of item 76, with +`switchingRulesDoesNotLeakRowsBetweenThem` in `test_tagrules` as the +regression test. It fails against the unfixed code. + +**Damage in the field, and the repair.** The live rules file had exactly one +casualty: the account rule sitting first in the list, with its `query`, its +`add` and its `note` all empty while every sibling account rule was intact. + +**The note was missed on the first pass of the repair**, because the shell +backup was read for the tagging command and the note comes from the comment +block ABOVE it, which the migration had given to all five account rules +alike. The user spotted the gap. `applyEditsToCurrentRule()` writes every +field, so every field is equally exposed: repair work here must check the +whole rule, not the fields that first drew attention. Restored from the four +siblings, which carry byte-identical notes. +Restored from `post-new.shell-backup`, which item 44's migration kept, and +verified by loading the file through mailctl's own reader: 17 rules, correct +scoping. The rule had stopped tagging, but only one message had arrived in the +meantime (14968 of 14969 in that account still carried the tag); it was tagged +by hand and the account is now complete. + +**Constraints.** The user chose to leave the fix on the branch rather than cut +a patch release, so 0.16.0 in the field still has it. Do not open that dialog +in a released build. + +**Size: XS** for the fix. The reproduction and the field repair were the work. + +## 80. A rule with many conditions squeezes the rule list to one visible row + +**Observed.** A rule with eight From conditions left the rule list showing +about one and a half rows, with the second rule half cut off under the first. +Reported with a screenshot; the builder filled the window and the list it sits +under kept almost nothing. + +**Cause.** `m_list` was added to the dialog's `QVBoxLayout` with stretch 1 +(`src/tagrulesdialog.cpp:109`) and the form below it with none, which looks +like the list wins. It does not: a stretch factor only distributes space ABOVE +each widget's minimum, and the form's minimum grows with every condition row, +so each row came directly out of the list. Measured on the builder's size hint: +120px with one row, 414px with eight. + +**Approach.** Done 2026-08-13. A `QSplitter` divides the list from the editor, +so the balance is the user's and is saved to `uistate.conf` beside the column +widths, and the condition rows sit in a `QScrollArea` capped at 190px so the +editor cannot grow without bound whatever the splitter is set to. The scroll +area rather than the builder is what text mode hides, since hiding the inner +widget would leave an empty frame. + +**Constraints.** Three measures were tried before one distinguished the bug +from the fix, and two passed against broken code: the dialog's +`minimumSizeHint` does not track form rows and read 580 either way, and a +`qMin` against the scroll area's own size hint read small whether or not the +cap was set, because an uncapped `maximumHeight` is `QWIDGETSIZE_MAX`. The +assertions that survive mutation are the editor pane's minimum inside the +splitter, and the cap read directly. A row's size hint is also invalid until +the event loop has run, so the test needs `processEvents` after selecting a +rule or it measures one row's height twice. + +**Size: XS.** |