From 019117aa8e52ce39cab58f77b57a9a67f510696f Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sun, 16 Aug 2026 21:58:18 +0200 Subject: feat(ui): act on the message a row displays, not its whole thread A thread's card has rendered one message since item 66, but every tag action still acted on the entire conversation. Delete, Archive, Important, Mark spam and Toggle unread now act on the message the card shows; the whole-thread versions move to a "Whole thread" submenu in the Message menu and the thread list's context menu, on Ctrl+Alt+. Closes items 87, 88, 105, 106, 107, 108, 109, 110 and 111. The defects fixed along the way, several found by reading rather than by report: - threadAt(current.row()) answered about the wrong thread for a reply row, because a tree numbers rows per parent. The audit found four live sites, not the one reported: Delete and Toggle unread each chose their DIRECTION from an unrelated thread, and the tag dialog counted the wrong thread's tags. threadFor(index) replaces them. - A message-scoped write made no optimistic model update and no reply row carried a doomed cue, so acting on a reply moved the pending-edit count and changed nothing on screen. - Both toggles read the state of a reply's THREAD, which a message-scoped write never changes, so they were one-way: the second press re-sent a tag the message already had. - flushHeldEdits() re-sent only thread-scoped edits, so a tag change made on one message during a sync was applied to the row, counted as unsynced, and then dropped without ever being written. - applyTagChange() updated a thread's summary but not its loaded replies, leaving an expanded thread's rows describing a state the database no longer held. - A thread's first message is not among its children, so both message-scoped lookups missed it: acting on a root card repainted nothing and emptied the message pane's chip row. - ThreadSummary::tags is notmuch's union over the thread, so a card standing for one message drew tags belonging to its siblings. The worker now reads that message's own tags in the walk that already finds its id, so the split is known before a row is ever opened. The card shows both tiers: its own message's tags at full size, the rest of the conversation's smaller and muted, so nothing appears to vanish when a row is selected. Auto mark-read is message-scoped as a result, and now arms for a reply, which it never did. With maildir.synchronize_flags on, the old thread-wide write reached the server for mail that had never been displayed. Co-Authored-By: Claude Opus 5 --- .../2026-08-03-post-0.1.0-usability-closed.md | 694 +++++++++++++++++++++ 1 file changed, 694 insertions(+) (limited to 'docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md') diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index 30271d5..3620684 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md @@ -5624,3 +5624,697 @@ it falls out of the existing code rather than needing a guard. **Nothing was built for getting back**, per the user: the filter buttons already are that. + +## 88. `threadAt(current.row())` answers about the wrong thread for a reply row + +**Observed (user, 2026-08-14):** "I clicked on a message and another random +message was marked read." + +**Cause (verified).** `markCurrentThreadRead` calls +`m_model->threadAt(current.row())`. A `QTreeView` numbers rows PER PARENT, so a +reply's `row()` indexes its siblings: `threadAt(0)` on the first reply of any +thread returns the FIRST THREAD IN THE LIST. The guards then compare correct ids +against the wrong thread and let a write through. + +**`CLAUDE.md` already records this trap**, from item 20: "what did NOT survive +that port is anything keyed on a row NUMBER". This is a surviving instance, not +a new discovery, which is the uncomfortable part. + +**Mostly masked before item 87's attempted fix.** With a thread-wide write the +mismatch usually resolved to the same thread or to a no-op; scoping the write to +one message made it visible immediately, as unrelated mail being marked read. + +**Approach.** `threadAt(int)` takes a row and cannot be safe here. Every caller +that has a `QModelIndex` should reach the thread through the index: for a +message row, `ThreadListModel::threadIdForMessage()` or the parent index; for a +thread row, `index.row()` is correct. Audit every `threadAt(` and every +`.row()` in `mainwindow.cpp` rather than fixing this one site, since the same +shape almost certainly appears elsewhere. + +**Consider deleting the row-taking overload** once the callers are converted, so +the trap cannot be re-entered. That is the change that makes this permanent +rather than fixed once. + +**A second live site, found 2026-08-16** during the notes reconciliation, before +any of the audit was done. The `delete` action's toggle +(`src/mainwindow.cpp:825`) decides its direction by looping over the selected +rows with `m_model->threadAt(index.row()).isDeleted()`. With a reply row in the +selection that reads an unrelated thread's state, so Delete can choose the wrong +direction: pressing it on a reply of an undeleted thread can undelete instead of +delete, or the reverse. Less damaging than item 87's case, since the write itself +goes through `scopeFor(rows)` and lands on the right threads, but the direction +is chosen from the wrong data. The audit is therefore confirmed as necessary +rather than precautionary, and this is at least the second site. Item 98 wants to +add a THIRD in the same shape, by copying this loop for "Important"; fix this +first or it propagates. + +**Size: S** for the one site, **M** if the audit finds several. The second site +above pushes this toward M. + +**Outcome, 2026-08-16: M. The audit found FOUR live sites, not one**, which +settles the question of whether auditing was worth it over fixing the reported +call. Every one of them read a thread's STATE for a selected index, and every one +of them was wrong the same way for a reply: + +| Site | What it decided | What went wrong for a reply | +|---|---|---| +| `delete` action | delete or undelete | direction read from the first thread in the list | +| `toggle_unread` action | mark read or unread | same, so the key went the wrong way | +| `selectionTagCounts` (was inline in `editTagsOnSelection`) | the tag dialog's tri-state boxes | offered to remove tags the selection did not carry | +| `markCurrentThreadRead` | whether to write | unreachable today, see below | + +**The fix is one accessor, not four patches.** +`ThreadListModel::threadFor(const QModelIndex &)` resolves a message row through +its PARENT and a thread row through itself, so the conversion happens once +instead of at every call site that has to remember which kind of row it is +holding. The row-taking `threadAt(int)` survives for the three loops over +`rowCount()` that legitimately have a top-level row number, and now carries a +doc comment saying why it is wrong for anything else. No `.row()` on a selected +index remains in `mainwindow.cpp`. + +**`markCurrentThreadRead` was already protected, by accident.** +`onThreadSelected` clears `m_currentThreadId` for a message row, and the handler +returns early when that does not match `m_markReadThreadId`, so a reply could not +reach the bad read. That is incidental protection standing in front of a real +defect, and item 87 does not remove it, so the test asserts on the RESOLUTION +rather than on a write it cannot currently provoke. Recorded because "it cannot +happen today" is exactly the reasoning that left this in place after item 20. + +**Two things the tests had to do that the reverted attempt did not.** The +fixture puts the reply under the SECOND thread, so `threadAt(0)` returns a +plausible wrong answer rather than the right one by luck; and the two threads are +in OPPOSITE states, since two threads in the same state give the same answer +either way. That second point is why the reverted fix was mutation-checked and +still green. All four tests were confirmed failing against the old behaviour, +each naming the wrong direction it chose, before the fix went back in. + +**The `undoText` seam is what made the toggles testable at all.** Delete and +Undelete push one command each and touch the same rows, so depth and ids are +identical between right and wrong; only the description differs. + + +## 105. Acting on a reply changes the counter and nothing on screen + +**Observed (user, 2026-08-16),** hand-testing item 88's fix: "I'm hitting delete +on a reply to a thread, I see the edits counter increasing but I have no +feedback if that message is being deleted. Same issue when marking read/unread +on a reply from a thread." + +**Two independent causes, and fixing either alone leaves the symptom.** + +*The write made no optimistic update.* `sendMessageTagChange` deliberately +skipped one, on reasoning that was correct as far as it went: `applyTagChange` +is keyed by THREAD, so calling it for a one-message edit would repaint the whole +card as though every message in it had changed. The conclusion did not follow. +The reply has its own row, and that is where the feedback belongs. +`ThreadListModel::applyMessageTagChange()` updates the node and emits +`dataChanged` for that row alone. + +*A reply row had no doomed cue at all.* Thread rows have been filled crimson and +struck through since item 13; the message-row branch of `data()` returned +`replyBackground()` unconditionally and never consulted the node's tags. So even +with the node updated, a deleted reply painted identically to an undeleted one. +Both the fill and the strike-out are now mirrored onto reply rows, and the +foreground goes white over the fill, since `readColour()` is mixed toward the +BACKGROUND and would otherwise compute a grey against the pane's base and paint +it over crimson. + +**The read/unread half was NOT fixed by the above, and the first attempt +stopped one layer short.** Hand-tested the same day: Delete now repainted and +read/unread still did nothing. The model was right and the write was right; the +DIRECTION was wrong. Both toggles read the state of the row's THREAD, and a +message-scoped write never changes a thread's tags, so the answer never moved. +On a read thread `toggle_unread` therefore evaluated to "add unread" on every +press, re-adding a tag the reply already carried; re-applying a tag a message +has is a no-op, and a no-op repaints nothing. The user found the repaint only by +deleting and undoing, which is a DIFFERENT write forcing the row to redraw. + +`MainWindow::everySelectedRowHasTag()` is now the single question both toggles +ask, and each row answers about what it STANDS FOR: a reply row about its +message, a thread row about its thread. `delete` had the identical latent bug, +found by reading rather than by report: pressing Delete twice on a reply deleted +it twice instead of undeleting, since its thread never became deleted. + +**Item 88 fixed WHICH thread these read, and that was necessary but not +sufficient.** A reply does not need a better thread, it needs a message. Two +fixes in two sessions on the same three lines, and the second was only found by +the user pressing the key: worth remembering that "resolved through the index" +and "resolved to the right OBJECT" are separate properties. + +**Bold on replies, at the user's request.** Replies were deliberately never +bold, on the reasoning that the thread row above already carries the unread cue +for the conversation. True of the thread and useless for the reply: once a +thread is expanded, the reply row is the only thing that can say which messages +in it are unread, and dimming alone (white against `#a8a8a8`) was too quiet. +Bold now combines with the dimming, which is the same two-cue pair a thread row +has had since 0.11.0 and for the same recorded reason: a desktop configured with +a Bold UI font makes `setBold()` a no-op, so a single cue has a single point of +failure. The smaller reply font stays, since that is what separates a reply from +a thread heading. + +`CardLayout` needed no change: it already measures the date rect BOLD whatever +font it is handed, precisely so an unread card cannot clip its date. Checked +rather than assumed, since a newly bold row is exactly what that guard exists +for. + +**A third pass: the message pane's chips.** Hand-tested again after the toggle +fix, and the LIST was correct while the pane was not: "for it to sync I have to +change message and go back to the edited one". `sendThreadTagChange` had always +refreshed the strip when the edited thread was the open one; `sendMessageTagChange` +had no equivalent, so the chips kept describing the message as it was before the +edit. Mirrored now, keyed on `m_currentMessageId`, and reading the tags by ID via +the new `ThreadListModel::messageById()` rather than from `currentIndex()`: the +two agree today, and a guard that depends on them agreeing puts the WRONG +message's tags in the pane on the day they do not. + +Both halves are separately mutation-checked, because a refresh with no guard and +a guard with no refresh each pass the other's test: removing the refresh +reproduces the user's report, and dropping the guard makes an edit to a +different reply repaint the open one with that reply's tags. + +**Three hand-test rounds for one defect, each finding a real and different +cause.** Optimistic update, then direction, then the pane. Recorded as the +argument for handing UI work back after every round rather than assuming the +last fix finished it, which is the lesson the memory file already carries and +this is the clearest instance of it. + +**Reading that flush turned up item 106**, a silent data loss on the same path, +which had never been reported and would not have been. + +**The redundant `deleted` chip is deliberate and was left alone.** A deleted +reply shows the fill, the strike-out AND a `deleted` chip. The user raised it; +the thread row has always done the same, with the model recording that "a doomed +thread is rare and worth naming", so replies match rather than diverge. +Confirmed with the user 2026-08-16 rather than silently changed. + +**One consequence that had to be fixed with it.** +`revertPendingTagChange()` checked only `m_pendingThreadIds`, so with an +optimistic update now applied for message writes, a FAILED message write would +have kept showing its optimistic state permanently, with nothing to correct it +until the next query. It reverts both scopes now. Undo needed no change: +`MessageTagCommand` routes back through `sendMessageTagChange`, so it repaints +by the same path. + +**Testing note.** The model test proves `applyMessageTagChange` works; it does +not prove the ACTION reaches it, which was the half actually missing. The +window-level test triggers the real Delete action on a real reply row and +asserts the row's background changed, and that the thread's did not. Both were +mutation-checked: no-oping the new method fails them with the user's own +symptom printed back. + +## 106. A tag change made on one message during a sync is silently lost + +**Observed: nowhere.** No user report. Found by reading `flushHeldEdits()` while +fixing item 105's strip refresh, which is worth recording because the failure is +invisible from the outside until the mail is already gone. + +**Cause (verified).** An edit made while notmuch's write lock is held is queued +as a `HeldEdit` and re-sent when the sync ends. The flush looped over +`edit.threadIds` and called `sendThreadTagChange()` and nothing else. A +message-scoped edit carries NO thread ids: `sendMessageTagChange()` queues +`HeldEdit{ {}, TagChange{ messageIds, ... } }`. So the loop ran zero times, the +send returned immediately on an empty list, and the edit was dropped. + +**What the user would have seen, and why it is worse than a plain loss.** The +optimistic update had already been applied, so the row showed the change. The +pending-edit indicator counted it, so the window reported an unsynced change. On +the next sync end the count cleared. Every signal the application gives said the +edit was made and then safely written; none of it happened. Item 28 is the +record of a wrong pending count being treated as serious on its own, and this is +that failure pointed at real mail. + +**Reachability.** Present since message-scoped writes shipped, and rare in +practice: it needs an edit on a REPLY (or any message row) made in the window +where a sync holds the lock, which is around 35 seconds per run. Item 105 made +message-scoped edits considerably easier to make deliberately, so the odds were +about to go up. + +**Fix.** The flush dispatches by scope: thread ids through +`sendThreadTagChange`, `change.messageIds` through `sendMessageTagChange`, each +guarded on being non-empty. The optimistic update is taken back for both kinds +before re-sending, since both send paths re-apply it. + +**The tempting wrong fix is escalation.** Sending a held message edit as a +thread edit would make the queue "work" and would delete every message in a +conversation when the user deleted one reply. The test asserts the scope +survives the hold, not merely that something was sent. + +**Size: XS**, once seen. The whole cost was in noticing. + +## 107. A thread-scoped write leaves the loaded replies showing their old tags + +**Observed (user, 2026-08-16):** "if I hit read/unread on the main thread +message, the status bar announces 'mark as read: N messages (whole thread)'. +When this happens only the main message is repainted, together with its chip in +the right pane. The replies don't get repainted." + +**Cause (verified).** `ThreadListModel::applyTagChange()` updated +`m_threads[row].summary.tags` and emitted `dataChanged` for that one row. It +never touched `m_threads[row].children`, so an expanded thread's reply rows kept +the tags they were loaded with. + +**Not a repaint bug, which is what the symptom looks like.** The rows were +repainted; they were repainted from data that had not changed. A thread-scoped +write reaches every message in the thread in the DATABASE, so the replies were +genuinely read while the model still described them as unread: bold, undimmed, +and correct-looking. They fixed themselves on the next query, which is exactly +the signature of a stale model rather than a missed signal. + +**Fix.** The same tag arithmetic is applied to every loaded child, followed by +one `dataChanged` spanning the whole expansion rather than a signal per reply. +Only LOADED replies are updated: an unexpanded thread has no child rows, and +messages the model does not hold are the database's business. + +**The test asserts both halves**, because they fail independently: that the +child nodes carry the change, and that a `dataChanged` actually covered the +reply rows. Mutating the node without telling the view leaves the old pixels on +screen; signalling without mutating repaints the same wrong data. + +**Size: XS.** Found while the user was hand-testing item 105. + +## 108. Acting on a thread root means the whole thread, though it displays one message + +**Observed (user, 2026-08-16),** after item 107: + +> we should split marking one message read/unread from marking the whole thread +> read/unread, as they are two different actions + +and, asked what the root card should mean: + +> since clicking on a thread main message displays the main message only, acting +> on it should affect that message only. A separate menu for thread actions +> would be ideal, like a submenu when right clicking and the same submenu under +> "Message" in the top menu. + +**Cause: a deliberate design that item 66 invalidated and nobody revisited.** +`ThreadListModel::scopeFor()` maps a thread row to `threadIds` and a message row +to `messageIds`. That was coherent when a root card RENDERED the whole +conversation: what you saw was what you acted on. Item 66 made the root render +one message and left the scope alone, so the card now shows one message and acts +on all of them. The status bar's "(whole thread)" suffix is the only thing that +says so, and it says it after the fact. + +**The end state the user described.** A thread row means the message it +displays, exactly as a reply row already does. Whole-thread actions still exist, +in their own submenu, reachable from the thread list's context menu and from the +`Message` menu, which today are two views of one set of `QAction` objects. + +**Approach.** + +- `scopeFor()` resolves a thread row to its first message id rather than its + thread id. `ThreadSummary::firstMessageId` is already carried from the query + for exactly this kind of question (item 66), so no expansion is needed. +- A second set of actions, thread-scoped, resolving the selection's threads and + going through `sendThreadTagChange`. These are the current actions' bodies, + unchanged. +- One submenu built once and added to both menus, per the user's description. +- The toolbar keeps the message-scoped actions. It is the beginner surface and + the frequent case; a submenu on a toolbar button is a worse affordance than + the menu entry it duplicates. + +**Constraints, and these are what make it M rather than S.** + +- **Every keybinding keeps its meaning name.** `Ctrl+U` stays `toggle_unread` + and now means one message; the thread action needs its OWN action name for + `[keys]`, since a user's config names actions. Do not reuse a name with new + semantics, and do not rename an existing one, which silently breaks a config + that mentions it. +- **`markAllRead` is not part of this.** It deliberately ignores the selection + and acts on every row in the view, and it is thread-scoped by nature. Leave + it alone; it is the one action where "whole thread" is not a scope question. +- **The undo stack must record the scope it actually used.** `MessageTagCommand` + and `ThreadTagCommand` already differ; a thread action that pushed the message + command would undo a fraction of what it did. +- **Auto mark-read is item 87 and gets simpler, not harder.** That item exists + because the timer marks a whole thread read while showing one message. Under + this change the natural scope for it is the message, which is what 87 asks + for. Do 108 first and 87 becomes small; doing them in the other order means + writing the same decision twice. +- **The status bar's "(whole thread)" suffix stays**, and becomes rarer and more + meaningful: it will mean the user chose the thread action rather than that the + application widened the scope for them. +- A multi-row selection mixing thread rows and reply rows stays honest, as + `ActionScope` already is: nothing is escalated or narrowed silently. + +**This is a user-visible behaviour change to every tag action.** Anyone with the +current habits will find `Ctrl+D` on a thread root deleting one message where it +used to delete the conversation. Semver on the user-visible surface makes it a +minor bump with an `### Upgrading` section, and the changelog has to say plainly +that the old behaviour moved rather than disappeared. + +**Verification.** The scope decision is testable without a painter, which is +where the assertions belong: `scopeFor()` on a thread row returns a message id, +the thread action returns a thread id, and each pushes the matching undo +command. A rendering probe adds nothing here. + +**Size: M.** The code is small and touches every action; the review cost is in +the naming and the config compatibility. + +**Outcome, 2026-08-16. The estimate held: the code was small and the review cost +was where it was predicted to be.** + +`ThreadListModel::messageScopeFor()` sits beside `scopeFor()` rather than +replacing it, so both meanings stay expressible and the thread actions reuse the +already-tested path. `MainWindow::TagScope` defaults to `Message`, so every +existing caller kept its line. Five `*_thread` actions, each its message-scoped +twin with `TagScope::Thread`, in a "Whole thread" submenu added to both the +`Message` menu and the thread list's context menu. + +**Three things the plan did not anticipate, all caught by existing tests rather +than by reading.** + +- **`Q_ASSERT(m_actions.size() == KeyMap::knownActions().size())` fired**, from + `test_tagrules` of all places, because that test builds a `MainWindow`. Five + new actions and no entries in `knownActions()`. The assertion is eight months + old and did exactly its job; worth noting that the failure surfaced in a suite + with nothing to do with actions. +- **"Unbound by default" was wrong and the suite said so.** + `everyActionHasAShortcut` requires every action to be keyboard-reachable. The + bindings are `Ctrl+Alt+`, one modifier out from each twin. + `Ctrl+Shift` was the obvious pairing and is taken twice over, by `spam` and + `mark_all_read`, both shipped. +- **The no-duplicate-icons rule had to be narrowed, with the user's agreement.** + These five deliberately share their twin's icon. That rule exists because the + toolbar can be icon-only, where the icon is the whole control; these live only + in a submenu, whose entries always carry text. The exception is a NAMED list, + and the test now also asserts none of them is on the toolbar, so putting one + there fails rather than silently passing. + +**The fixture change worth remembering.** `makeThread()` in `test_mainwindow` +set no `firstMessageId`, so after this change every thread row resolved to no +message and ten tests failed with "the action did not happen" — which reads as a +defect in the action rather than a gap in the fixture. The real worker fills +that field from the query; the fixture now does too. + +**Tests that encoded the OLD meaning were repointed, not deleted.** Three of +them (the delete toggle, the mixed selection, the "(whole thread)" suffix) are +about thread semantics and now drive the thread actions; three held-edit tests +assert on a thread row and drive `flag_thread` for the same reason. One, about +the `flagged` tag NAME, was rewritten to assert on the change that was sent +rather than on a model row, since the tag is the same under either scope. + +## 87. Auto mark-read marks a whole thread, including replies never displayed + +**Observed (user, 2026-08-14):** "with the first message in a thread selected +(not expanded), the 2s delay that marks it read applies to the whole thread, so +all answers are marked read as well." + +**Cause (verified in the code).** `markCurrentThreadRead` +(`src/mainwindow.cpp`) sends `sendThreadTagChange` with the thread id, so +`unread` is removed from every message in the thread. + +**This was coherent until item 66 and is not any more.** While a thread root +rendered the whole conversation, everything marked read HAD been displayed. +Item 66 made a root render one message; the thread-wide write stayed. + +**Not cosmetic.** `maildir.synchronize_flags` is on, so removing `unread` +rewrites Maildir filenames and the next sync carries it to the server. Mail the +user never opened stops being unread everywhere, and nothing here can put it +back except reading the messages again by hand. + +**Approach: unblocked 2026-08-16, and the ordering was the lesson.** The obvious +fix is to send `sendMessageTagChange` for the message on screen. That was +written, tested, mutation-checked, shipped and reverted the same evening, +because it exposed item 88: the guards above the write read +`threadAt(current.row())`, which answers about an unrelated thread when a reply +row is current, and a fix that narrows the write turns that mismatch into "a +random message is now read". + +Item 88 is now done. `markCurrentThreadRead` resolves through +`ThreadListModel::threadFor(index)`, so the guards read the right thread for a +reply. **Do not take that as the whole of it:** 88's fix deliberately left the +`m_currentThreadId` guard in place, which is what keeps a reply from reaching +the write at all today. Narrowing the write means deciding what that guard +should be once a message row is a legitimate target, rather than assuming 88 +cleared the road. + +**Constraints for the second attempt.** + +- **Test selecting a REPLY, not only a root.** The reverted work asserted on a + root selection, which is the one case where `current.row()` is correct, so a + mutation-checked test passed against a bug that corrupted mail. +- **Consider doing item 108 first.** It makes a thread row mean the message it + displays, which is the same decision this item has to make for the timer, in + the same place. Done in that order this becomes small; done the other way the + decision is written twice and has to agree with itself. +- **The repaint half is done**, as item 105 on 2026-08-16. + `ThreadListModel::applyMessageTagChange()` exists, `sendMessageTagChange` + calls it, and a reply row now carries the doomed fill and strike-out it had + never had. What remains for this item is the THREAD card, below. +- A thread reads as unread while ANY message does, so the card can only stop + looking unread when nothing else in it carries the tag. For an unexpanded + thread the per-message tags are not loaded, so the honest answer needs + per-message counts in `ThreadSummary` or nothing at all. + +**Size: S**, after 88. + + +**Outcome, 2026-08-16, and doing 108 first was the whole saving.** The decision +"what does a thread row stand for" belongs to 108; once it was made, this item +was a rename and two call sites. + +`m_markReadThreadId` became `m_markReadMessageId`, `scheduleMarkRead` takes an +id and an unread flag rather than a `ThreadSummary`, and the handler sends +`sendMessageTagChange`. The guard now compares against what the PANE is showing +rather than against the selection, which is the same thing for both kinds of row +and is what "the message the user is reading" actually means. + +**A reply arms the timer now, which it never did.** The old code stopped it +explicitly for a message row, with a comment saying a per-message write did not +fit a thread-keyed pending map. That objection died with item 105's +message-scoped path, and leaving it would have made the message the user is +actually reading the one kind that is never marked read. + +**The approximation that remains, and it is deliberate.** A thread row arms from +`ThreadSummary::isUnread()`, which is a union over the conversation, so a thread +whose first message is already read but whose reply is not will arm and then +write `unread` off a message that does not carry it. That is a no-op, not a +wrong write: the id is the displayed message's either way. Removing it needs +per-message state in `ThreadSummary`, which is the same thing the third +constraint below asks for and which nothing carries yet. + +**The third constraint looked unmet and was closed two items later.** A thread +reads as unread while ANY message does, so an unexpanded card could not honestly +stop looking unread when one message in it was marked read, and the automatic +mark-read therefore had no visible effect on an unexpanded row. Item 110 settled +it from the other direction: the card stops drawing the thread's union at all +and draws the tags of the message it displays, which arrive with the message +load. No per-message counts in `ThreadSummary` were needed. What remains is +narrower: a row that has never been opened has no per-message answer and still +shows the union. + +**Mutation-checked** by putting the thread-wide write back: both tests fail, one +naming the six undisplayed messages. + +## 109. A root card's own message is invisible to a message-scoped write + +**Observed (user, 2026-08-16),** hand-testing item 108: "delete single message +on the root message of a thread doesn't trigger the repaint, delete whole thread +does (correctly repaints all messages) - right pane loses the chip row when +repainting, it simply disappears", and the same for read/unread. + +**Cause: one assumption, two symptoms.** `setThreadMessages` drops the depth-0 +message because the root ROW stands for it, so a thread's first message is never +in `children`. Both message-scoped lookups searched `children` and nothing else: + +- `applyMessageTagChange` found no node, updated nothing, emitted no + `dataChanged`. The card did not repaint. +- `messageById` returned a DEFAULT-CONSTRUCTED node, and the strip refresh + (item 105) set the pane's chips to its empty tag list. That is worse than not + refreshing: it destroyed a strip that had been correct. + +**Item 108 turned this from an edge case into the ordinary gesture.** Before it, +a thread row meant the whole thread and took the thread-scoped path, which works; +the message-scoped path was only ever reached from a reply row, where the node +does exist. Making a root card act on its own message walked straight into the +one case neither lookup handled. The two changes were correct separately and +broken together, which is the argument for hand-testing after each one rather +than at the end. + +**Fix.** Both look at the root first, matching on `ThreadNode::first` when it is +loaded and on `ThreadSummary::firstMessageId` when it is not, since the user acts +on unexpanded threads constantly. `messageById` synthesises a `MessageNode` from +the summary in the unexpanded case. + +**The summary is updated only for a single-message thread**, and that +distinction is load-bearing rather than cautious. `notmuch_thread_get_tags` is a +UNION over the thread's messages: for a thread of one it IS that message's tags, +so writing to it is exact; for a thread of seven, deleting one message does not +delete the conversation, and repainting the card crimson would claim it did. The +test asserts both halves, because a fix that updated the summary unconditionally +passes the first and is a lie in the second. + +**What that leaves, stated rather than hidden.** On a multi-message thread the +root card still shows no change for a one-message edit, because the card draws +the thread's state and the thread's state has not moved. The message pane's +chips follow, and the reply rows are individually correct once expanded. Making +the card honest needs per-message counts in `ThreadSummary`, which is the same +thing item 87's third constraint asks for. Recorded in the changelog as a +deliberate limit. + +**A test-only trap found on the way**, and it had already been shipped twice in +item 105's tests without being noticed. `TagStrip` is a single row that collapses +whatever does not fit into a trailing "+N" chip, so under the offscreen platform +with an unshown window `visibleTags()` returns almost nothing and `hiddenTags()` +holds the rest. Asserting on `visibleTags()` measures the LAYOUT, not the data. +The two earlier tests passed only because their tag counts happened to fit. All +three now assert on `visibleTags() + hiddenTags()`. + +**Size: S.** + +## 110. A card and the message pane show tags belonging to a message's siblings + +**Observed (user, 2026-08-16),** hand-testing item 109 against a real +four-message thread whose root carried `unread` and whose THIRD message carried +`signed`: "If I select the root message and wait 2s, the right pane chips update +and both signed and unread disappear. Changing message and going back to the +root message makes them reappear." + +Plus two repaint reports in the same message: marking the root message read left +the row bold, and deleting it left the row unpainted, while the whole-thread +versions of both worked. + +**One cause under all three, and the first report is the one that revealed it.** +`ThreadSummary::tags` comes from `notmuch_thread_get_tags`, which is the UNION +over the thread's messages. Measured against the user's thread: + +| | tags | +|---|---| +| thread union | `inbox mailing-list/SBo signed unread` | +| root message | `inbox mailing-list/SBo unread` | + +So `signed` belonged to a sibling and the pane was claiming it for the root +message. The 2s mark-read write replaced the strip with the root's REAL tags and +correctly dropped both `signed` (never its own) and `unread` (just removed). +**The pane was lying before the write, not after it**, and reselecting restored +the lie because selection sets the strip from the summary. + +**The same gap explains the missing repaints.** The model held no per-message +tags for a thread's first message at all, so a message-scoped write had nothing +to update and the card kept drawing the summary. Item 109 had made the write +find the root, but the card still read the union, which had not changed. + +**Fix: the load is the authority.** `MessageRef` already carries the message's +own tags and arrives on every open. `ThreadListModel::setRootMessageTags()` +records them on `ThreadNode::first`, and a thread row's `data()` substitutes +`first.tags` for the summary's when that node exists. `MainWindow::onMessageLoaded` +calls it and re-sets the pane's strip from the same source. + +**Only the TAGS are substituted.** The subject, the authors, the date and the +reply count on a card describe the THREAD and are correct as they are; only the +tags were ever the union that lied. And the summary itself is not rewritten: it +is what the thread-scoped actions and the query read, and three unread siblings +do not stop being unread because this message was read. + +**What is still approximate, and it is now much narrower.** Before a row has +ever been opened there is no per-message answer, so an unopened card shows the +union and can display a sibling's mark. Narrowing that further means asking the +query for per-message tags, which is a worker change rather than a UI one. + +**This closes item 87's third constraint by accident**, which had said an +unexpanded card could not honestly stop looking unread. It can now, for the +message it displays, because it draws that message's tags rather than the +conversation's. + +**Both halves mutation-checked**: making the card read the summary again fails +the repaint assertion, and removing the recording from the load fails the +sibling-tag assertion. + +**Size: S.** + +## 111. A card should show its siblings' tags smaller, not drop them + +**Observed (user, 2026-08-16),** with a screenshot of the unselected card from +item 110's thread, still showing `mailing-list/SBo` and `signed`: + +> I don't mind it actually, but "signed" disappears as soon as I select the root +> thread, so it still looks like a bug even though it is not. The root thread +> should rightfully display its own info only, but maybe we could leverage this +> "bug" in another way, by showing the unioned tags smaller (and only on the +> left pane) instead of removing them completely. + +**The design is the user's and it is better than either extreme.** Item 110 was +correct that a card must not CLAIM a sibling's tag as the displayed message's, +and the honest fix made a chip vanish on selection, which reads as a fault +whatever the reasoning behind it. Showing both tiers keeps the card's real job: +it stands for one message but sits above a conversation, and what the rest of +that conversation carries is worth seeing at a lower weight. + +**No new data was needed.** The model already holds both: `ThreadNode::first` +(the displayed message, from the message load) and `ThreadSummary::tags` (the +union, from the query). `PillTagsRole` now returns own-then-sibling and +`PillOwnCountRole` says where the boundary is. + +**The first attempt derived the split from the message LOAD, and the user +rejected it immediately:** "not selecting the thread shows the chips at 'main' +size, not smaller, not dimmed. After selecting the thread the unioned chips +repaint to the correct size/color." + +That was honest and useless. A list is mostly UNOPENED rows, so the feature was +invisible exactly where it was meant to be read, and selecting a row still +changed the card, which is the thing this item exists to stop. + +**The query knows, and it was already looking.** The worker walks to the card's +message to read its id; `ThreadSummary::firstMessageTags` reads that same +message's tags in the same pass, from the index, so the split arrives with the +row. `ThreadListModel::nodeFor()` seeds `ThreadNode::first` from it, and +`reconcile()` refreshes both the field and the node for a survivor. + +**The reconcile half was a real gap found by reading**, not by the report: a +surviving row deliberately keeps its NODE, because that is the expansion state +reconcile exists to preserve, so the per-message tags had to be refreshed +explicitly AND added to the change detector. A sync where only the root message +changed leaves the thread's union identical, so nothing else would have noticed. + +**`first` is left empty when the query carried no per-message tags.** An empty +list would be indistinguishable from "this message carries nothing", which would +put every chip in the sibling tier and mute the whole card. + +**Two visual decisions, both settled with the user and both tested rather than +eyeballed.** + +*Muting is saturation only.* Not a blend toward the background, which is the +mistake `accentLineColour()` already records: on a dark theme that lands on the +background and the chip disappears. Worse here, because a chip's fill has to +carry legible text. Hue is untouched so a muted `signed` is recognisably the +same colour as a full-size one, and LIGHTNESS is untouched so +`TagColors::textColourOn()` keeps choosing the same text colour. The test +asserts that last property directly, since contrast is exactly what a look at +the screen will not reliably catch. + +*The smaller font is a FRACTION of the card's, not a fixed subtraction.* The +first version took one point off `smallFont()` and the user reported the tiers +as indistinguishable. The arithmetic is why: their desktop is 14pt, so the two +tiers were 13 and 12, a 7% step. Subtraction gives a step whose size depends on +the desktop font, which is backwards — it is largest exactly where the text is +already small enough to be fragile. `CardLayout::siblingFont()` is 0.70 of the +card font, giving 13 and 10 there, and the test asserts a RATIO rather than a +size so it stays about the distinction rather than about the constant. + +*The padding has to scale with it, and this was half the problem.* +`TagChip::kPaddingX` is 9 a side, so a sibling chip was 18px of padding around +roughly 30px of text: the chip stayed wide while its letters shrank, which reads +as "same chip, smaller text". `TagChip::sizeFor()` takes a scale, floored at 2px +a side because the corner radius is half the height and text flush against it +sits on the curve. Measured on the user's own font, `signed` goes from 71px to +52px and the height from 25 to 20. + +The pixel branch is covered specifically: `pointSizeF()` is -1 for a font set in +pixels, which qt6ct does, so a point-only implementation returns the original +size and both tiers render identically. That trap is in `CLAUDE.md` and would +have shipped silently on the user's own desktop. + +**A test that measured nothing, caught by mutation.** The padding test first +called `TagChip::sizeFor()` directly, which proves what that function does and +nothing about whether the delegate ASKS for a scaled padding. Dropping the scale +at the call site left it green. `CardDelegate::chipSize()` now holds that +decision and the test goes through it. This is the "rendering probes lie" rule +in a new shape: the probe was fine, it was pointed at the wrong object. + +Chips are bottom-aligned so a smaller one shares a baseline with its neighbours +rather than stepping down mid-row. + +**Message rows are unaffected**: a reply already draws only the tags its thread +does not carry, and has no second tier to show. + +**Size: S.** -- cgit v1.2.3