diff options
Diffstat (limited to 'docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 4534 |
1 files changed, 24 insertions, 4510 deletions
diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index eec544e..98a2cd7 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -26,13 +26,21 @@ Numbering is stable. New items append with the next free number and never renumber, so a note referring to "item 7" keeps meaning the same thing. An item that is dropped stays in the table marked `dropped` with a one-line reason. +**The status table below is the index of every item; the sections are only the +open ones.** Item 73 moved the done, dropped and postponed sections out to +`2026-08-03-post-0.1.0-usability-closed.md`, which took this file from just over +five thousand lines to under six hundred. Nothing was deleted and nothing was +renumbered: a closed item keeps its row here, with its date and outcome, and its +full Observed/Cause/Approach section is in that file under the same number. Look +there when a row cites evidence you need. + **Items 20 and 53 are both on master since 2026-08-10**, as the card list. Item 20's original presentation, the one the user rejected on sight, is preserved on the branch `item-20-message-rows` at 029a50e and was never merged; the branch `card-list` carries the work that was. Any file or line reference in item 20's -entry below points at that PARKED branch, not at master, where the same lines -are unrelated. Item 53 records why the first attempt was rejected and is worth -reading before changing the thread pane again. +entry, now in the closed-items file, points at that PARKED branch, not at +master, where the same lines are unrelated. Item 53 records why the first +attempt was rejected and is worth reading before changing the thread pane again. ## Theme @@ -116,7 +124,7 @@ taking that too literally. | 57 | "Flag" would read better as "Important" or "Starred" | presentation | XS | **done** | | 58 | `message_zoom` documents a 0.5 to 3.0 range and enforces none of it | correctness | XS | **done** | | 59 | Archive and Mark all read shipped with the same icon | presentation | XS | **done** | -| 60 | Next thread dead-ends on the last reply of an expanded thread | defect | XS | **done**; already fixed by 5487d58, see below | +| 60 | Next thread dead-ends on the last reply of an expanded thread | defect | XS | **done**; already fixed by 5487d58, see the closed-items file | | 61 | `test_mainwindow` fails intermittently, about 1 run in 20 | testing | S | **done** 2026-08-13; an `init()` fixture points every test at its own lock table | | 62 | No config option for the date format on a card | presentation | XS | **done** 2026-08-11 | | 63 | No way to see sent mail, and no filter for it | workflow | M | **done** 2026-08-11; see `specs/2026-08-11-sent-mail-design.md` | @@ -129,9 +137,9 @@ taking that too literally. | 70 | Pane icons are a private set where the main window uses the system theme | presentation | M | **done** 2026-08-11; six shipped SVGs | | 71 | A toolbar action does not sync, so the edit sits until the next cron run | workflow | S | **done** 2026-08-11; 2s default, `auto_sync_delay_ms` | | 72 | No khard/khal integration | workflow | ? | open, unspecified; the user places it after send, so v2 at the earliest | -| 73 | This backlog is past four thousand lines | maintenance | S | open | +| 73 | This backlog is past four thousand lines | maintenance | S | **done** 2026-08-13; 5056 lines to 578, closed sections moved to `2026-08-03-post-0.1.0-usability-closed.md` | | 74 | "Searching..." keeps claiming a query is running while rows are already arriving | feedback | XS | open; cause measured 2026-08-11, the delay itself is the cold page cache and is not fixable here | -| 75 | The tagging rules window forgets its size and its column widths | persistence | S | **done** 2026-08-13 on `rule-builder`, unreleased. The window-kind question is left open, see below | +| 75 | The tagging rules window forgets its size and its column widths | persistence | S | **done** 2026-08-13 on `rule-builder`, unreleased. The window-kind question is left open, see the closed-items file | | 76 | Every field in the rules dialog is free text, so a rule is easy to get wrong | workflow | M | **done** 2026-08-13 on `rule-builder`, unreleased. See `specs/2026-08-13-rule-builder-design.md` | | 77 | No way to see what a rule would collect, in the thread list | workflow | S | **done** 2026-08-13 on `rule-builder`, unreleased | | 78 | No way to build a rule from something visible in a message | workflow | M | open; wants 76 first, so the created rule lands in a form that can hold it | @@ -142,1162 +150,6 @@ Sizes are rough: XS under an hour, S a sitting, M a session. --- -## 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. - ## 21. Default shortcuts are not sensible enough **Observed (user, 2026-08-04):** "improve the default shortcuts to some sensed @@ -1384,1102 +236,6 @@ exactly this: "saved queries that carry their own account scope, so one action gets there". If saved queries gain an account scope here, item 10 may be answered as a side effect rather than needing its own work. -## 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. - ## 36. `test_mainwindow` cannot reach the worker **Observed:** twice in one session (0.8.0), a defect could not be given a @@ -2524,238 +280,6 @@ rather than modelled. `MainWindow` and must keep working. The fixture is per-test, not a suite-wide `initTestCase`, or every case pays for a `notmuch new`. -## 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. - ## 40. No live filter over the current view **Observed (user, 2026-08-05):** "search in current view", spelled out as two @@ -2792,1529 +316,6 @@ filters rows already fetched, without touching notmuch. - Interaction with item 39: a filter and a sort over the same rows want the same proxy. Whichever is built first should leave room for the other. -## 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. - ## 65. No full code review and optimization pass **Observed (user, from the notes):** "full code review and optimization." @@ -4382,43 +383,6 @@ be timing-dependent. **Size: S**, and a defect rather than an enhancement. -## 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`. - ## 68. A forwarded subject gets no `passed` tag **Observed (user, from the notes):** "passed tag should appear when subject is @@ -4476,173 +440,6 @@ the code. **Status:** left open deliberately on 2026-08-11. The cause is settled and the options are costed; the user has not chosen, and no code was written. -## 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. - ## 72. No khard/khal integration **Observed (user, from the notes):** "investigate khard/khal integration (light @@ -4659,30 +456,6 @@ Those are three different features. **Size: `?`, unspecified**, and out of scope until v2 exists. Ask before designing anything. -## 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. - ## 74. The first query after boot sits on "Searching..." for seconds **Observed (user, 2026-08-11):** the first start of the day takes noticeably @@ -4728,181 +501,6 @@ inaccurate, not because anything is expected to happen. here and should not be attempted: prefaulting 1.1 GB at startup to make one query look fast is a worse trade than the wait. -## 75. The tagging rules window forgets its size and its column widths - -**Observed.** The rules window opens at the same size every time, whatever -size it was left at, and the rule table's columns reset to their computed -widths on every open. The user also asks whether it should present as a -primary window rather than a popup. - -**Cause.** `TagRulesDialog::TagRulesDialog` calls `resize(760, 520)` -unconditionally (`src/tagrulesdialog.cpp:59`) and never reads or writes a -saved geometry; there is no `saveGeometry`/`restoreGeometry` pair anywhere in -the file, and `MainWindow` is the only class that touches `uiStatePath()` -(`src/mainwindow.cpp:141,183`). The columns reset because `reloadList()` -calls `resizeColumnToContents` for the enabled and stage columns on every -repopulate (`src/tagrulesdialog.cpp:213-214`), and `onCountsReady` does the -same for the count column (`:326`); a width the user dragged is discarded by -the next reload, not only by a close. - -**Approach.** Save `saveGeometry()` and `m_list->header()->saveState()` into -the machine-written UI state file under keys of their own, and restore both -in the constructor, keeping the current `resize` as the fallback for a first -run. Drop the unconditional `resizeColumnToContents` calls once a saved -header state exists, or the restore is undone on the first reload. - -The "popup or primary window" question is a separate decision and not a -defect: the class is a `QDialog` (`src/tagrulesdialog.h:41`), which is what -makes it modal to the main window and what puts it above it. Changing it to a -top-level window means it can be left open beside the main window and can go -behind it, and the rule edits would then need to survive that. Ask before -changing it. - -**Constraints.** UI state goes to `~/.local/state/qtmaildir/uistate.conf` via -`MainWindow::uiStatePath()`, never into the hand-edited config. A geometry -restore under the offscreen platform is what item 46 already tripped over, so -the test asserts on the saved value rather than on the resulting frame. - -**Size: S.** - -**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. - ## 78. No way to build a rule from something visible in a message **Observed.** The user would like to select an address or another piece of a @@ -4929,98 +527,6 @@ in CLAUDE.md. **Size: M.** -## 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.** - ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering @@ -5038,11 +544,19 @@ and line, verified not assumed), **Approach**, **Constraints**, and **Verification** where it is not obvious. Do not renumber. Do not delete: mark `dropped` with a reason. +**When an item closes, move its section to +`2026-08-03-post-0.1.0-usability-closed.md`** and leave the status table row +here with its date and outcome. This is what keeps the file readable, and it is +the step that was missing for seventy items: doing it only once, as item 73 did, +buys a few months and then the problem returns. Move the section on the commit +that closes the item, not in a later cleanup pass. Where the closed section +records a trap that is still true of the code, that trap belongs in `CLAUDE.md`, +which is where it will actually be read. + **A fully specified item goes in its own file under `docs/superpowers/specs/`, not inline here.** This document is a backlog: its job is to say what is open, how big it is, and what decides whether it can be picked up. A design that runs -to a hundred lines buries that under itself, and this file is already past four -thousand. +to a hundred lines buries that under itself. The split is by depth, not by size on the day. An entry stays here while it records an observation, a cause and an approach. It moves out once it carries |