diff options
| -rw-r--r-- | CHANGELOG.md | 101 | ||||
| -rw-r--r-- | CLAUDE.md | 195 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 694 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 417 | ||||
| -rw-r--r-- | src/carddelegate.cpp | 85 | ||||
| -rw-r--r-- | src/carddelegate.h | 18 | ||||
| -rw-r--r-- | src/cardlayout.cpp | 26 | ||||
| -rw-r--r-- | src/cardlayout.h | 13 | ||||
| -rw-r--r-- | src/keymap.cpp | 29 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 460 | ||||
| -rw-r--r-- | src/mainwindow.h | 101 | ||||
| -rw-r--r-- | src/notmuchworker.cpp | 6 | ||||
| -rw-r--r-- | src/tagchip.cpp | 14 | ||||
| -rw-r--r-- | src/tagchip.h | 13 | ||||
| -rw-r--r-- | src/threadlistmodel.cpp | 383 | ||||
| -rw-r--r-- | src/threadlistmodel.h | 120 | ||||
| -rw-r--r-- | src/types.h | 23 | ||||
| -rw-r--r-- | tests/test_carddelegate.cpp | 128 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 1001 | ||||
| -rw-r--r-- | tests/test_notmuchworker.cpp | 50 | ||||
| -rw-r--r-- | tests/test_threadlistmodel.cpp | 613 |
21 files changed, 4255 insertions, 235 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 7235340..9ca51f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,107 @@ point at which they are stable. ## [Unreleased] +### Upgrading + +**Acting on a thread row now acts on the message it displays, not on the whole +conversation.** Selecting a thread's card has shown one message since 0.22.0, +but Delete, Archive, Important, Mark spam and Toggle unread still acted on every +message in the thread. They now act on the message you are looking at. + +The whole-thread versions are still there, under **Whole thread** in the Message +menu and in the thread list's right-click menu, with new bindings a modifier out +from their old ones: + +| Action | Message | Whole thread | +|---|---|---| +| Archive | `Ctrl+E` | `Ctrl+Alt+E` | +| Delete | `Ctrl+D` | `Ctrl+Alt+D` | +| Mark spam | `Ctrl+Shift+S` | `Ctrl+Alt+S` | +| Toggle unread | `Ctrl+U` | `Ctrl+Alt+U` | +| Important | `Ctrl+I` | `Ctrl+Alt+I` | + +Existing `[keys]` entries keep working and keep their meaning: `delete` is still +`delete`, now scoped to one message. The thread actions are separate names +(`delete_thread`, `archive_thread`, `spam_thread`, `toggle_unread_thread`, +`flag_thread`) and can be rebound like any other. "Mark all read" is unchanged: +it never used the selection. + +### Changed + +- **Reading a message no longer marks its replies read.** The two-second + automatic mark-read applied to the whole thread, so opening a conversation + marked messages read that had never been shown. Because + `maildir.synchronize_flags` is on, that reached the server and could not be + undone from here. It now marks only the message on display, and it applies to + a reply as well, which previously was never marked read at all. +- An unread reply is bold as well as undimmed, which is the pair of cues an + unread thread has carried since 0.11.0. Replies were deliberately plain + before, on the grounds that the thread row above already flags the + conversation; once a thread is expanded, that row cannot say which of its + messages are unread, and dimming alone was too quiet to notice. Replies stay + a size smaller than their thread, so the two kinds of row still read apart. + +### Fixed + +- Acting on a reply inside an expanded thread could read a different thread's + state, because a reply's position is counted within its own thread and was + being used as a position in the whole list. The first reply of any thread + therefore answered as the first thread in the list. Three actions chose what + to do from that wrong answer: **Delete** could undelete a thread that was + never deleted, **Toggle unread** could go the wrong way, and **Edit tags** + offered to remove tags the selected message did not carry, while showing the + ones it did as unset. Every action that acts on a selection now resolves the + thread through the row itself. +- Acting on a reply gave no visual feedback at all: Delete and Toggle unread + moved the unsynced-changes count and left the row looking exactly as it did + before. A change scoped to one message now repaints that message's own row. + A deleted or spam reply is filled and struck through, as a thread in the same + state already was. The thread's own card deliberately does not change, since + one deleted reply does not delete the conversation. +- Marking an expanded thread read left its replies looking unread. The write + reached every message, but the list only updated the thread's own card, so + the replies stayed bold and undimmed until the next query corrected them. + They now follow a thread-wide change as the card does. +- Acting on a thread's own card did not repaint it, and emptied the message + pane's chip row. A message-scoped write was only ever applied to a thread's + loaded replies, and a thread's first message is not one of them: the card + stands for it. So Delete or Toggle unread on a root card changed nothing on + screen, and the pane's chips were replaced with the empty tag list the lookup + returned. Both now find a root card's own message. +- **A thread's card showed tags belonging to other messages in the thread**, and + so did the message pane. notmuch reports a thread's tags as the union over + its messages, so a four-message thread whose third message is signed read as + signed everywhere, including on a card that stands for the first message and + in a pane showing only that message. Opening a message now records what it + really carries, and the card and the pane both use it. + + A card still shows the whole conversation's tags, since a card sits above a + thread: its own message's tags come first at full size, and the ones only its + siblings carry follow, smaller and muted. The split arrives with the query, + so a row reads correctly before it has ever been selected. + + This is also what was stopping a card from repainting: with no per-message + tags for a thread's first message, marking it read or deleting it had nothing + to change, so the row stayed bold or unstruck while the write went through. +- The message pane's tag chips did not follow an edit to the message on + display. Tagging a reply repainted its row in the list and left the pane + describing the message as it was, until you selected another message and came + back. The pane now follows a message-scoped edit, as it already did a + thread-scoped one. +- **A tag change made on a single message during a sync was silently lost.** + Edits made while a sync holds notmuch's write lock are held and sent when it + finishes; that queue only ever re-sent whole-thread changes, so a + message-scoped one was applied to the row, counted in the unsynced-changes + indicator, and then dropped without ever being written. The change appeared + to have been made and reported itself as pending right up until it vanished. +- Delete and Toggle unread chose their direction from a reply's THREAD rather + than from the reply itself, which made both one-way on a reply. A + message-scoped write never changes the thread's tags, so the answer never + moved: pressing Toggle unread on an unread reply re-added the tag it already + had, and pressing Delete twice on a reply deleted it twice instead of putting + it back. Re-applying a tag a message already carries changes nothing, which + is why the key looked dead. Each row is now asked about what it stands for. + ## [0.24.0] - 2026-08-15 Double-clicking a row now opens its thread on its own, expanded, with the row @@ -473,13 +473,181 @@ its siblings and `threadAt(current.row())` on the first reply of any thread returns the FIRST THREAD IN THE LIST. This shipped in `markCurrentThreadRead`, was mostly masked while the write it guarded was thread-wide, and became "a random message was marked read" the moment a fix scoped that write to one -message (items 87 and 88). Reach the thread through the INDEX, never through a -row number, unless you have already established the index is a thread row. - -The general rule is in the item 20 note further down, "anything keyed on a row -NUMBER did not survive the port to a tree". This is a surviving instance, found -by corrupting real mail rather than by reading, so treat every remaining -`.row()` in `mainwindow.cpp` as suspect until checked. +message (items 87 and 88). + +**Use `threadFor(const QModelIndex &)`**, which resolves a message row through +its parent and a thread row through itself. Item 88 added it on 2026-08-16 and +converted every caller; `threadAt(int)` survives only for loops over +`rowCount()`, which genuinely hold a top-level number. A new caller that has an +index and reaches for `threadAt(index.row())` is reintroducing the bug. + +**The audit found four live sites, not the one that was reported**, which is the +part worth remembering: `delete` and `toggle_unread` each chose their DIRECTION +from the wrong thread, and the tag dialog counted the wrong thread's tags. All +three were reachable by clicking a reply, none had a test, and the reported +symptom named only `markCurrentThreadRead` (which was in fact protected by an +unrelated guard and could not fire). One bad accessor produced four defects with +one symptom between them. + +**A thread's first message is NOT among its children, and two lookups forgot +it.** `setThreadMessages` drops depth 0 because the root row stands for that +message, so `children` never holds it. `applyMessageTagChange` and +`messageById` both search the root first now (item 109); before that, a +message-scoped write to a root card repainted nothing, and the strip refresh set +the pane's chips to the empty node the lookup returned, destroying a strip that +had been correct. Item 108 made that the ordinary gesture rather than an edge +case: the two changes were each correct and broken together. + +**`ThreadSummary::tags` is notmuch's UNION over the thread, and a card that +stands for one message must not draw it.** A four-message thread whose third +message is `signed` reads as signed, so the root card and the message pane both +claimed a tag the displayed message did not have (item 110). `MessageRef` +carries the message's own tags and arrives on every load; +`ThreadListModel::setRootMessageTags()` records them on `ThreadNode::first`, and +a thread row's `data()` substitutes `first.tags` for the summary's when that +node exists. Only the TAGS are substituted: the subject, authors, date and reply +count describe the thread and are correct. The summary itself is never +rewritten, because the thread-scoped actions and the query read it. + +That was also why a root card could not repaint: with no per-message tags, a +message-scoped write had nothing to change. `applyMessageTagChange` keeps the +summary in step only when `totalCount <= 1`, where the union IS the message. + +**The card shows BOTH tiers, and that is item 111 rather than a leftover.** +`PillTagsRole` returns the displayed message's tags first and the thread's other +tags after; `PillOwnCountRole` is the boundary the delegate switches fonts at. +The second tier is drawn in `CardLayout::siblingFont()` and +`CardDelegate::mutedChipColour()`. + +**The split comes from the QUERY, not from the message load**, and that +distinction was worth a whole round trip. `ThreadSummary::firstMessageTags` is +read by the same worker walk that finds `firstMessageId`, so an UNOPENED row +already knows which tags are its own. Deriving it from the load instead left +every unopened row drawing one tier and correcting itself on selection, which is +most of the list and is exactly the "chip changed when I clicked" the feature +exists to remove. `nodeFor()` seeds the node on arrival; `reconcile()` must +refresh it AND compare it, since a survivor keeps its node and a sync can move +the root's tags while the thread's union stands still. + +**A size step must be a FRACTION, not a subtraction, and the padding has to +follow it.** One point off a 14pt desktop font is a 7% step and reads as the +same size; the user reported exactly that. `CardLayout::siblingFont()` is 0.70 +of the card font. `TagChip::kPaddingX` is a fixed 9px a side, so an unscaled +sibling chip is 18px of padding around ~30px of text and stays wide while its +letters shrink: `TagChip::sizeFor()` takes a scale, and +`CardDelegate::chipSize()` is where the tier chooses it. Assert on ratios rather +than sizes, so the test is about the distinction and not the constant. + +Muting is **saturation only**. Hue stays so the tag is recognisable; lightness +stays so `TagColors::textColourOn()` keeps its choice and the chip cannot become +unreadable. Do not blend toward the background here: `accentLineColour()` +records what that costs on a dark theme, and a chip is worse because its fill +carries text. + +**`TagStrip::visibleTags()` measures the LAYOUT, not the data.** It is one row +that collapses the overflow into a trailing "+N" chip, and an unshown window +under the offscreen platform has no width, so nearly everything lands in +`hiddenTags()`. A test asserting on `visibleTags()` alone passes or fails on how +many tags happened to fit; two shipped that way before it was noticed. Assert on +`visibleTags() + hiddenTags()`. + +**A message-scoped write repaints the MESSAGE's row, never the thread's.** +`ThreadListModel::applyMessageTagChange()` is the counterpart to +`applyTagChange()` and exists because there was no optimistic update at all for +a one-message edit: the correct observation that repainting a thread card for a +one-message change is a lie was turned into the wrong conclusion that nothing +should repaint, so Delete and Toggle unread on a reply moved the pending count +and changed nothing on screen (item 105). The thread card deliberately stays +put; one deleted reply does not doom the conversation. + +**A thread ROW means the one message its card displays, not the conversation.** +Item 108, 2026-08-16. `ThreadListModel::messageScopeFor()` is what the ordinary +tag actions resolve through; `scopeFor()` still returns whole threads and is +what the five `*_thread` actions use. A thread row's message is +`ThreadSummary::firstMessageId`, carried from the query, so no expansion is +needed; in the Sent view that is the first MATCHED message, which is right for +the same reason it is right on the card. A row with no id contributes NOTHING +rather than falling back to its thread: that fallback is the silent escalation +this removed. + +The automatic mark-read follows the same rule (item 87): `m_markReadMessageId`, +armed for a reply as well as a root. One approximation is deliberate and +documented at the call site: a thread row arms from `ThreadSummary::isUnread()`, +a union over the conversation, so it can arm for a thread whose displayed +message is already read. The write is still scoped to that message, so the cost +is a no-op rather than a wrong write. + +**Adding an action is four places, and three of them are enforced by tests that +fail in confusing ways.** `KeyMap::knownActions()` (a `Q_ASSERT` in the +constructor fires otherwise, and it surfaces in whichever suite happens to build +a `MainWindow` first — `test_tagrules` did), `defaultBindings()` (every action +must be keyboard-reachable), and the icon table (every action must carry one). +The no-duplicate-icons rule is narrowed to actions that can reach the toolbar, +by a named exception list; the five thread actions share their twins' icons +because a submenu entry always carries text, and the test asserts none of them +is on the toolbar so the exemption cannot be abused. + +**A toggle must read the state of what the row STANDS FOR, not of its thread.** +`MainWindow::everySelectedRowHasTag()` is the one question `delete` and +`toggle_unread` both ask; a reply row answers from its message, a thread row +from its thread. Reading the thread makes a toggle ONE-WAY on a reply, and the +failure is silent in a specific way worth knowing: the write is message-scoped, +so it never changes the thread's tags, so the answer never moves however many +times the key is pressed. The second press re-sends a tag the message already +has, which is a no-op, and a no-op repaints nothing. The user reports this as +"the key does nothing", not as "the key did the wrong thing" (item 105). + +This is the SECOND fix to the same three lines. Item 88 corrected which thread +they resolved; that was necessary and not sufficient, because a reply needs a +message read rather than a better thread. "Resolved through the index" and +"resolved to the right object" are separate properties, and a test for the first +passes against the second being wrong. + +**Any state a thread row draws, a reply row has to draw too, and this was +missed once already.** The message-row branch of `data()` is a separate switch +from the thread branch, so a cue added to one is simply absent from the other +with nothing to flag it. The doomed fill and the strike-out were thread-only +from item 13 until item 105, which is why updating the node was not enough on +its own to make Delete visible. When adding a visual state, check both branches. +One asymmetry is deliberate and must survive: a reply carries no tag strip. A +reply is now BOLD when unread, at the user's request on 2026-08-16, combining +with the dimming for the same two-cue reason a thread row has both; the smaller +reply font is what keeps it subordinate. A `deleted` reply deliberately shows +the chip as well as the fill and strike-out, matching the thread row, confirmed +with the user rather than treated as redundancy to remove. + +**And the reverse: a thread-scoped write must reach the thread's LOADED +replies.** `applyTagChange` updated the summary only, so marking an expanded +thread read left every reply bold and undimmed until the next query (item 107). +The symptom reads as a missed repaint and is not: the rows were redrawn from +data that had not changed. When a model update looks like it did not paint, +check whether the data behind those rows actually moved. + +**Every path a thread-scoped write travels, a message-scoped one travels too, +and each one was missed separately.** Three of them, found one hand-test round +apart: the optimistic model update (item 105), the message pane's tag strip +(also 105, keyed on `m_currentMessageId` and read by id through +`ThreadListModel::messageById()`, never from `currentIndex()`), and +`flushHeldEdits()`, which re-sent only thread edits and therefore DROPPED any +message edit made during a sync after showing it and counting it as pending +(item 106, data loss, never reported). When adding anything to +`sendThreadTagChange`, check whether `sendMessageTagChange` needs it. Escalating +a message edit to its thread is never the fix: it deletes a whole conversation +when the user deleted one reply. + +**Anything applied optimistically must also be reverted.** +`revertPendingTagChange()` keyed on `m_pendingThreadIds` alone, so adding the +message-scoped optimistic update would have left a FAILED message write showing +its optimistic state for good. Both scopes revert now. Undo needs nothing extra: +`MessageTagCommand` routes back through `sendMessageTagChange`. + +**Testing this needs two things that are easy to miss.** Put the reply under the +SECOND thread, so the wrong answer is plausible rather than accidentally right, +and give the two threads OPPOSITE states, since two threads in the same state +answer identically whichever way the code resolves them. That second point is +why the reverted item 87 fix was mutation-checked and green while corrupting +real mail. For a toggle, assert on `undoText()`: both directions push one +command over the same rows, so depth and ids cannot tell them apart. **A test for a mutation on a data-writing path must exercise the REPLY case, not only the root.** The reverted fix above was mutation-checked and green: it @@ -503,6 +671,12 @@ measuring nothing: children are populated by the expansion. `hasChildren()` is the pre-expansion question and falls back to `summary.totalCount > 1`. An assertion on `rowCount` fails against correct code. +- **A `ThreadSummary` fixture needs `firstMessageId`.** Since item 108 an + ordinary tag action resolves a thread row to that id, so a summary without one + names no message and every action on it silently does nothing. Ten tests + failed this way at once, all reporting "the action did not happen", which + reads as a defect in the action rather than a gap in the fixture. + `makeThread()` sets it; a hand-built summary must too. - **`currentThreadId()` reports INTENT, not content.** It is assigned synchronously in the selection handler before any worker round-trip, so a test asserting on it passes with `onThreadLoaded()` disabled entirely, measured. @@ -543,6 +717,13 @@ a defect that did not exist because of these; each was believed until it was con buttons to the query row: the thread list shrank, and a test sizing its window to 300px started failing with a message naming a defect that did not exist. Assert the rect is INSIDE the viewport, not merely non-empty. +- **A probe can be correct and still measure nothing, by being pointed at the + wrong object.** A test for the sibling chip's padding called + `TagChip::sizeFor()` directly: that proves what the function does and nothing + about whether the delegate asks it for a scaled padding, so a mutation + dropping the scale at the call site stayed green. Assert through the function + the production path actually calls (`CardDelegate::chipSize()`), not through + the one it calls INTO. - **A "saturated pixel" threshold catches antialiased edges of the selection highlight**, hundreds of distinct near-background colours, and will pass whatever the code does. Match the exact colours the model supplies instead. Two versions of one test passed under 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+<same key>`, 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.** 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 7b44e7e..944dfee 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 @@ -131,8 +131,8 @@ taking that too literally. | 64 | The Sync button carries a mailbox icon, not a refresh one | presentation | XS | **done** 2026-08-11 | | 65 | No full code review and optimization pass | correctness | ? | open, unspecified | | 66 | Selecting a thread root leaves the message pane blank until a reply has been selected | defect | S | **done** 2026-08-14, unreleased. Not the blank pane it was filed as: the root rendered the CONVERSATION until the thread had been expanded once, then one message. Now always one message, and the conversation view is removed at the user's request. **One case unverified by hand:** the notes also report a single-message `id:` query whose card would not open, which is the same empty-`MessageIdRole` failure and should be gone; confirmed 2026-08-15 as a SEPARATE defect with a different cause, see item 96 | -| 87 | Auto mark-read marks a whole thread, including replies never displayed | defect | S | open; measured 2026-08-14. Reachable only after 66 removed the conversation view. Blocked on 88 | -| 88 | `threadAt(current.row())` answers about the wrong thread for a reply row | defect | S | open; found 2026-08-14 by shipping a fix that marked an unrelated message read. Row numbers are per parent in a tree | +| 87 | Auto mark-read marks a whole thread, including replies never displayed | defect | S | **done** 2026-08-16, unreleased. Built on 108, which is why it stayed small: the timer tracks a MESSAGE id now, and arms for a reply too, which it never did before | +| 88 | `threadAt(current.row())` answers about the wrong thread for a reply row | defect | M | **done** 2026-08-16, unreleased. The audit found FOUR live sites, not one. `ThreadListModel::threadFor(index)` resolves a reply through its parent; every caller holding a selected index converted, and no `.row()` on a selected index remains in `mainwindow.cpp`. Unblocks 87 | | 67 | The placeholder pane counts unread, flagged and inbox, but not sent or drafts | information | XS | **done** 2026-08-11, shipped in 0.15.0 | | 68 | A forwarded subject gets no `passed` tag | workflow | S | open; no subject rule exists, measured 2026-08-11. Decision needed: display mark (XS) or write the flag (S, syncs out) | | 69 | `passed` and `replied` read as words where every other state is a glyph | presentation | S | **done** 2026-08-11, inside item 70 | @@ -162,6 +162,20 @@ taking that too literally. | 94 | `pinned` has nothing left to decide once the buttons are built-in | maintenance | S | open; **blocked on 93**, and deliberately not part of it. A user-visible removal: the row becomes built-ins only and every saved query lives in the menu | | 96 | A query returning the thread already on display opens onto the placeholder | defect | S | **done** 2026-08-15, unreleased. Split from 66's unverified half, which had a different cause. Reproduced from two screenshots after four measured eliminations | | 97 | An edit made during a sync is reverted in the list when the sync ends | defect | S | **done** 2026-08-15, unreleased. Found by hand-testing item 89's fix. The sync-end refresh ran BEFORE the held-edit flush, so it read a database that still carried the old tag | +| 98 | "Important" adds the tag but cannot remove it, unlike every other toggle | defect | XS | open, found 2026-08-16 in the notes reconciliation | +| 99 | The unread action is labelled "Toggle unread" whichever way it will go | presentation | S | open; depends on 98's toggle shape, and the label is harder than it looks | +| 100 | The message pane offers Back, Forward, Reload and Save page, none of which mean anything | defect | XS | open, found 2026-08-16. Chromium's standard menu is added wholesale | +| 101 | Sync is account-aware for edits but not for the account the user is looking at | workflow | S | open; item 49 built the edit half deliberately. Needs a decision, see the entry | +| 102 | The rules table shows no note, so the field explaining a rule is invisible until it is opened | workflow | XS | open, found 2026-08-16 | +| 103 | What Delete does to mail on the server is undocumented and unverified | clarification | S | open; a question first, possibly no code at all | +| 104 | Mail visible in Thunderbird never reaches qtmaildir | defect | ? | open, reported 2026-08-16, cause NOT established. Most likely outside this repo; see the entry before writing code | +| 109 | A root card's own message is invisible to a message-scoped write | defect | S | **done** 2026-08-16, unreleased. Found by hand-testing 108. `applyMessageTagChange` and `messageById` searched only the loaded replies, and a root's message is never among them, so the ORDINARY gesture repainted nothing and wiped the pane's chip row | +| 110 | A card and the message pane show tags belonging to a message's siblings | defect | S | **done** 2026-08-16, unreleased. Found by hand-testing 109 against a real 4-message thread. `ThreadSummary::tags` is notmuch's UNION; a card standing for one message drew it. Also the reason a root card could not repaint at all | +| 111 | A card should show its siblings' tags smaller, not drop them | presentation | S | **done** 2026-08-16, unreleased. The user's own design, from looking at 110's result: own tags full size, the thread's others smaller and muted, so nothing appears to vanish on selection | +| 105 | Acting on a reply changes the counter and nothing on screen | defect | M | **done** 2026-08-16, unreleased. Found by hand-testing 88, and took three passes. FOUR causes: no optimistic update for a message-scoped write, no doomed cue on a reply row, both toggles reading the reply's THREAD state so they were one-way, and the message pane's strip not following a message edit. Also bolds an unread reply, at the user's request | +| 106 | A tag change made on one message during a sync is silently lost | defect | XS | **done** 2026-08-16, unreleased. Found by READING while fixing 105, never reported. `flushHeldEdits` re-sent only thread-scoped edits, so a message-scoped one was shown, counted as pending, and never written | +| 107 | A thread-scoped write leaves the loaded replies showing their old tags | defect | XS | **done** 2026-08-16, unreleased. `applyTagChange` updated the summary only, so marking a thread read left its expanded replies bold | +| 108 | Acting on a thread root means the whole thread, though it displays one message | workflow | M | **done** 2026-08-16, unreleased. `messageScopeFor()` beside `scopeFor()`; five `*_thread` actions in a "Whole thread" submenu on `Ctrl+Alt+<key>`. User-visible: minor bump, `### Upgrading` written | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -255,81 +269,6 @@ recorded. **Size: `?`, unspecified.** Do not propose a design for this; ask. -## 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: blocked on item 88, and that ordering is 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". Fix 88 first. - -**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. -- The card must repaint. `sendMessageTagChange` deliberately makes no - optimistic model update, since `applyTagChange` is thread-keyed; without a - message-scoped equivalent the write lands, the unsynced count rises, and the - card stays bold, which the user reported as a separate fault. -- 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. - -## 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. - -**Size: S** for the one site, **M** if the audit finds several. - ## 68. A forwarded subject gets no `passed` tag **Observed (user, from the notes):** "passed tag should appear when subject is @@ -494,6 +433,330 @@ bump either way: an ignored optional field is not a breaking change. **Size: S.** Removing a field, two UI affordances and their tests. +## 98. "Important" adds the tag but cannot remove it, unlike every other toggle + +**Observed (user, from the notes):** "the add 'Important' action should be a +toggle (like unread)." + +**Cause (verified in the code).** `src/mainwindow.cpp:863` registers `flag` as a +one-way add: + +```cpp +addAction(QStringLiteral("flag"), tr("&Important"), + tr("Mark the selected threads as important"), [this]() { + tagSelected({ QStringLiteral("flagged") }, {}, tr("Mark important")); +}); +``` + +Adding a tag that is already there is a no-op the user cannot see, so pressing +the key or the button on an already-important thread appears to do nothing at +all. Nothing in the UI removes `flagged` except the general tag dialog. + +**The two neighbouring actions are already toggles**, so this is an +inconsistency rather than a missing feature. `delete` +(`src/mainwindow.cpp:825`) and `toggle_unread` (`:867`) both read the current +state and choose a direction, and `delete`'s comment states the rule this should +follow: one direction for the WHOLE selection, flipping only when every selected +thread is already in the target state, because a single keystroke that leaves a +selection in two states is worse than either outcome. + +**Everything needed is already loaded.** `ThreadSummary::isFlagged()` +(`src/types.h:64`) reads the tag off the summary, so the direction can be +decided without a worker round trip, exactly as `isDeleted()` is. + +**Approach, now a two-line change.** Item 105 extracted +`MainWindow::everySelectedRowHasTag()`, which is the whole of the direction +logic: +`everySelectedRowHasTag("flagged") ? tagSelected({}, {"flagged"}, tr("Unmark important")) : the current add`. +The undo stack needs nothing new, since `TagChange::inverted()` already covers +both directions. + +**Constraints.** + +- Call `everySelectedRowHasTag()`, never a hand-rolled loop. Two separate fixes + went into that logic on 2026-08-16 (items 88 and 105) and both were bugs a + copy of the then-current `delete` loop would have inherited: resolving a + reply's row number to the wrong thread, and asking a reply's thread instead + of the reply. +- The action's tooltip says "Mark the selected threads as important" and would + become wrong. Item 99 is the same problem for `toggle_unread` and the two + should be decided together. +- The label question belongs to item 99, not here. This item is the behaviour + only: the key stops being a no-op. + +**Size: XS.** + +## 99. The unread action is labelled "Toggle unread" whichever way it will go + +**Observed (user, from the notes):** "the label for 'toggle unread' should be +dynamic: on an 'unread' message it should be 'Mark as read', on a 'read' +message it should be 'Mark as unread'." + +**Cause (verified in the code).** `src/mainwindow.cpp:867` registers one static +label, `tr("Toggle &unread")`, and the lambda decides the direction at +invocation time from the current row. The action carries that text in three +places at once: the Message menu (`:1060`), the thread context menu (`:1167`) +and the toolbar (`:1122`, with the `mail-mark-unread` icon). Nothing updates it +when the selection changes. + +**Not as simple as reading the current row**, which is why this is S and not XS. + +- The action applies to the WHOLE selection and picks one direction from the + current row, so with a mixed selection any label naming a single outcome is + either wrong for some rows or has to describe the rule ("Mark all as read"). +- A menu action's text is read when the menu opens, but a TOOLBAR button's text + is on screen continuously, so it has to track `selectionChanged` rather than + being computed at popup time. `currentRowChanged` is the wrong signal for + anything selection-shaped, per `CLAUDE.md`. +- The accelerator is inside the word (`Toggle &unread`). Two different labels + need two accelerators chosen so neither collides in the Message menu, which + already holds "Mark &spam" and "&Important". +- The shortcut list (Help > Keyboard shortcuts) and the config's `[keys]` + section both name the action `toggle_unread`. The action NAME must not change + with the label, or every user's config breaks. Same rule as item 57, which + changed "Flag" to "Important" on screen and left the action and tag alone. + +**Approach.** Compute the label from the same state the lambda already uses, +which since item 105 is `MainWindow::everySelectedRowHasTag("unread")`, update +it on `selectionChanged`, and keep a neutral fallback for an empty or mixed +selection. Decide with item 98, which raises the identical question for +"Important". + +**Use that helper rather than re-deriving the state**, or the label and the +action can disagree. It already encodes the two things this gets wrong on its +own: a reply answers about its MESSAGE, not its thread, and the answer is over +the whole selection rather than the current row. + +**Constraints.** Every label is user-facing and needs `tr()`. Since the strings +are chosen at runtime rather than written once, all of them must exist as +literals `lupdate` can see; a string built by concatenation is not translatable. +`ctest -R translations` is the check. + +**Size: S.** Mostly the mixed-selection and toolbar decisions, not the code. + +## 100. The message pane offers Back, Forward, Reload and Save page, none of which mean anything + +**Observed (user, from the notes):** "back/forward/save page in the right pane +don't make sense, shouldn't be visible." + +**Cause (verified in the code).** `MessageView::showBodyContextMenu` +(`src/messageview.cpp:619`) starts from Chromium's own menu: + +```cpp +QMenu *menu = m_view->createStandardContextMenu(); +``` + +That menu is built for a browser and carries the navigation and page actions +whole. The pane is not a browser: every document arrives through `setHtml()` +with a fixed base URL, so there is no history to go back to, nothing to reload +from, and the request interceptor blocks everything by default anyway. The +entries are inert as well as meaningless. + +**Deliberate as far as it goes.** The comment above the call says the page's own +menu comes first so "copy, select all and the rest stay exactly as they were", +which is right for the editing actions and wrong for the navigation ones. The +item is that the filter was never applied, not that the base menu was a mistake. + +**Approach.** Keep the menu, drop the actions that cannot apply. Qt names them +as `QWebEnginePage::WebAction` values (`Back`, `Forward`, `Reload`, +`SavePage`, and `ViewSource` is worth the same look), and each has a +`pageAction()` whose pointer can be matched against the standard menu's entries +and removed. Removing by matching the action pointer is safer than matching by +text, which is translated. + +**Constraints.** + +- Do not rebuild the menu from scratch. Copy, Copy link address and Select all + are the reason the standard menu is used, and item 85's search entries are + appended to it. +- `Save page` is not the attachment save. Attachments have their own bar and + their own path-traversal checks (see the web view security notes in + `CLAUDE.md`); nothing here should grow a second way to write a file. +- Verify against a real right-click on a real message. The offscreen platform + builds the menu but a screenshot of it proves nothing, and the entry list + depends on what the page reports as available at that moment. + +**Size: XS.** + +## 101. Sync is account-aware for edits but not for the account the user is looking at + +**Observed (user, from the notes):** "sync button should be account-aware." + +**Cause (verified in the code).** `MainWindow::pendingSyncChannels()` +(`src/mainwindow.cpp:3550`) resolves channels from `m_editedAccounts`, the set of +accounts the user has made EDITS in, and from nothing else. The account dropdown +is not consulted. With nothing pending it returns empty on purpose, and +`mailsync.sh` turns that into `mbsync -a`, every channel. + +**Item 49 built exactly this and the reasoning still holds.** The comment states +it: with nothing pending the run is a FETCH, and narrowing a fetch to wherever +the last edit happened would "quietly stop collecting mail everywhere else". +Fetching is global by nature; carrying edits is not. + +**So this needs a decision, not a fix.** The note does not say which of two +things the user means, and they are different features: + +*Sync only the selected account, on demand.* A deliberate "sync this account" +that ignores the pending set, presumably beside the existing Sync rather than +replacing it. Useful when one account is slow and the user wants their mail from +another one now. The risk is the one item 49 named: a button that looks like +Sync and quietly does not collect the rest of the mail. + +*Show which accounts a sync will cover.* No behaviour change at all, just making +the existing account-awareness visible, since today the user cannot tell whether +a run is narrowed or full. The status bar already names each channel as mbsync +reaches it (item 42), so most of this exists. + +**Constraints.** + +- The account dropdown is a VIEW filter. Making it also steer sync couples two + things the user may reasonably want apart: looking at one account while + fetching all of them is the normal case, not an edge case. +- Whatever narrows a run must still carry every pending edit, or an edit is + stranded with nothing on screen to say so. `pendingSyncChannels()` already + falls back to a full sync when it cannot resolve a channel for an edited + account, and that safety must survive. +- An account with no `[account.<key>]` section, or one whose section names no + channel, has no channel to sync. The fallback covers it today. + +**Size: S** for the on-demand button, XS for the visibility half. Ask which. + +## 102. The rules table shows no note, so the field explaining a rule is invisible until it is opened + +**Observed (user, from the notes):** "add 'notes' column to the filters table." + +**Cause (verified in the code).** `TagRule` carries a `note` field +(`src/tagrules.h:37`, "Why the rule is shaped this way. Shown in the dialog"), +and the editor below the table edits it, but the table itself lists five columns +and none of them is the note (`src/tagrulesdialog.cpp:116`): + +```cpp +m_list->setHeaderLabels({ tr("On"), tr("Stage"), tr("Rule"), tr("Tags"), + tr("Matches") }); +``` + +So the one field written specifically to explain a rule can only be read one rule +at a time, by selecting it. With several rules the note is exactly the thing that +would let the user pick the right one without opening each. + +**Approach.** A sixth column. The column widths already persist (item 75), so a +new column needs a sensible default width and nothing else in the way of state. + +**Constraints.** + +- The note is free text of any length and would stretch the column. Elide it and + put the full text in the tooltip; the `Rule` column already faces the same + problem with a long query and is the pattern to match. +- `ColumnCount + 1` in `setColumnCount` is load-bearing: the enum drives the + column indices and there is a spare. Add the enum value rather than hardcoding + 5, and check every place that indexes a column by number. +- Notes are the user's own words and can be empty. An empty cell is correct + here; do not substitute a placeholder. + +**Size: XS.** + +## 103. What Delete does to mail on the server is undocumented and unverified + +**Observed (user, from the notes):** "verify how 'delete' works", with two +sub-questions of their own: "trash bin (?)" and "delete from server (?)". + +**This is a question first.** The user is not reporting a defect; they are saying +they do not know what the button does to their mail, which for a destructive +action is its own problem regardless of the answer. + +**What the code does (verified).** `src/mainwindow.cpp:825` adds and removes the +`deleted` tag, and nothing else. It is a toggle, it goes through the undo stack, +and it writes a notmuch tag. + +**What that means downstream is what needs verifying, and it is NOT in this +repo.** `maildir.synchronize_flags` is true, so notmuch maps certain tags to +Maildir filename flags, and mbsync carries filename flags to the server. Whether +`deleted` is one of those, whether the user's `~/.mbsyncrc` has `Expunge Both` +(it does, on every channel), and what each provider does with a message flagged +deleted, together decide whether this button is reversible. The undo stack makes +the TAG reversible; it says nothing about what a sync did with it in between. + +**Approach.** Measure before designing anything: the notmuch config's +`maildir.synchronize_flags` and its tag-to-flag mapping, one real message tagged +and synced in a test account, and what the server shows afterwards. Then decide +whether the UI needs to say what it does, whether "Delete" is even the right +word for it, and whether a trash view is wanted. + +**Constraints.** + +- **This is the one place the no-confirmation rule should be re-examined rather + than assumed.** `CLAUDE.md` records that a human at a GUI gets undo instead of + confirmation dialogs, and that is right for tags. If the measurement shows the + next sync expunges mail from the server, then undo does not in fact cover this + action, and the premise the rule rests on does not hold for it. +- Do not test this against the user's real accounts. A message that is expunged + to prove that it is expunged is still gone. +- Any answer that involves a trash view is a much larger item and should be + split out rather than folded in here. + +**Size: S** for the investigation and whatever the UI needs to say. Unknown +beyond that, and deliberately not sized further until the measurement exists. + +## 104. Mail visible in Thunderbird never reaches qtmaildir + +**Observed (user, from the notes):** "sync doesn't work compared to thunderbird. +New mail received on thunderbird did not appear in qtmaildir. Need to investigate +further." + +**Cause: NOT established.** Recorded because it is a defect report about mail +going missing, which is the most serious kind this backlog carries, and it has +been sitting in the notes unrecorded. What follows is one measured mechanism that +would produce exactly this symptom, not a diagnosis. + +**qtmaildir cannot show what mbsync did not fetch, and mbsync fetches folders by +pattern.** Three of the five channels in the user's `~/.mbsyncrc` name their +folders explicitly: + +``` +Patterns "INBOX" "[Gmail]/Posta inviata" "[Gmail]/Bozze" "[Gmail]/Speciali" +``` + +and one names only `"INBOX"`. The two non-Gmail channels use `Patterns *`. +Gmail applies labels, and a message whose label is not one of those four is in a +folder mbsync never asks for. Thunderbird speaks IMAP directly and sees every +folder, so the same message is visible there and absent locally. This is a +configuration property of the user's mbsyncrc, outside this repository entirely. + +**One inconsistency worth reporting regardless**, found while checking the +above: one of the Gmail accounts is configured in `qtmaildir.conf` with +`sent = [Gmail]/Posta inviata` and `drafts = [Gmail]/Bozze`, while its mbsync +channel has `Patterns "INBOX"` and fetches neither. The Sent and Drafts filters +for that account can therefore only ever be empty. That is real, and it is +independent of whatever this item turns out to be. + +**Approach.** Reproduce before anything else, and the reproduction has to +distinguish three layers, because the fix lives in a different place for each: + +1. Is the message on disk? `find` in the Maildir, or `notmuch count` on a term + from it. If not, this is mbsync or `.mbsyncrc`, and there is nothing to + change here. +2. If it is on disk, is it indexed? `notmuch new` and count again. If not, this + is notmuch config, `new.ignore` or the hook. +3. Only if it is indexed and still not shown is this qtmaildir's defect, and + then the question is which query hid it: the account scope, the built-in + filter, or a rule that tagged it out of the inbox. + +**Constraints.** + +- Ask the user for one concrete example before investigating: which account, + roughly when, and what Thunderbird shows for it. A general "sync doesn't work" + cannot be reproduced, and the last four defects in this backlog were all found + from a specific message. +- The `post-new` hook from mailctl tags mail unattended. A rule that removes + `inbox` would make a correctly fetched, correctly indexed message vanish from + the default view, which looks identical to a sync failure from the outside. + `notmuch search` without a filter is what tells them apart. +- Do not change `.mbsyncrc` as part of this. It is the user's, it is outside the + repo, and a Patterns change refetches folders. + +**Size: `?`** until reproduced. Most likely not a code change here at all. + + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/src/carddelegate.cpp b/src/carddelegate.cpp index d7d06aa..4e27e6e 100644 --- a/src/carddelegate.cpp +++ b/src/carddelegate.cpp @@ -20,6 +20,7 @@ #include "cardlayout.h" #include "marks.h" +#include "tagchip.h" #include "threadlistmodel.h" #include <QApplication> @@ -31,6 +32,12 @@ namespace { +/// How much of a full-size chip's padding a SIBLING chip keeps. +/// +/// Matched to CardLayout::siblingFont()'s own scale, so the chip shrinks as a +/// whole rather than keeping full-size margins around smaller letters. +constexpr qreal kSiblingPaddingScale = 0.70; + CardLayout::Input inputFor(const QModelIndex &index) { CardLayout::Input in; @@ -58,6 +65,39 @@ QRect CardDelegate::expanderRectFor(const QStyleOptionViewItem &option, .expanderRect; } +QSize CardDelegate::chipSize(const QFontMetrics &metrics, const QString &text, + bool own) +{ + // The padding shrinks with the font for a sibling chip. Left fixed it is + // 18px around roughly 30px of text, so the chip stays wide while its + // letters shrink and the tier reads as "same chip, smaller text". + return own ? TagChip::sizeFor(metrics, text) + : TagChip::sizeFor(metrics, text, kSiblingPaddingScale); +} + +QColor CardDelegate::mutedChipColour(const QColor &chipColour) +{ + if (!chipColour.isValid()) + return chipColour; + + // Saturation only, and NOT a blend toward the background. The accent bar + // above records what blending toward Base costs: on a dark theme it lands + // on the background and the thing disappears. A chip is worse, because its + // fill also has to carry legible text on top of it. + // + // Hue is untouched, so a muted `signed` is still recognisably the same + // colour as a full-size `signed` elsewhere in the list. Lightness is + // untouched too, which is what keeps TagColors::textColourOn() picking the + // same text colour: draining saturation alone moves the fill toward grey + // without moving it toward either black or white, so contrast is preserved + // by construction rather than by hoping. + constexpr float kSaturationScale = 0.45f; + + float h = 0, s = 0, l = 0, a = 0; + chipColour.getHslF(&h, &s, &l, &a); + return QColor::fromHslF(h, s * kSaturationScale, l, a); +} + QColor CardDelegate::accentLineColour(const QColor &accountColour) { if (!accountColour.isValid()) @@ -304,20 +344,47 @@ void CardDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, : ThreadListModel::PillColoursRole) .toList(); - const QFont chipFont = CardLayout::smallFont(chrome.font); - const QFontMetrics chipMetrics(chipFont); + // A thread card draws its own tags at full size and the rest of the + // conversation's smaller and muted (item 111). The count is where the two + // tiers meet; a message row has no such split and reports its whole list. + // + // Shown rather than dropped, at the user's request: a card sits above a + // conversation, so what its siblings carry is worth seeing, just not at + // the same weight. Before the row has been opened everything is in the own + // tier, so a chip SHRINKS when the split becomes known and none vanishes. + const int ownCount = + isMessage ? tags.size() + : index.data(ThreadListModel::PillOwnCountRole).toInt(); + + const QFont ownFont = CardLayout::smallFont(chrome.font); + const QFont siblingFont = CardLayout::siblingFont(chrome.font); + const QFontMetrics ownMetrics(ownFont); + const QFontMetrics siblingMetrics(siblingFont); + painter->save(); - painter->setFont(chipFont); int x = card.tagRect.left(); for (int i = 0; i < tags.size(); ++i) { - const QSize size = TagChip::sizeFor(chipMetrics, tags.at(i)); + const bool own = i < ownCount; + const QFontMetrics &metrics = own ? ownMetrics : siblingMetrics; + + const QSize size = chipSize(metrics, tags.at(i), own); if (x + size.width() > card.tagRect.right()) break; // Out of room; a clipped chip reads as a rendering fault. - const QColor colour = i < colours.size() - ? colours.at(i).value<QColor>() - : QColor(0x55, 0x55, 0x5f); - TagChip::paint(painter, QRect(QPoint(x, card.tagRect.top()), size), - tags.at(i), colour); + + QColor colour = i < colours.size() ? colours.at(i).value<QColor>() + : QColor(0x55, 0x55, 0x5f); + if (!own) + colour = mutedChipColour(colour); + + // Bottom-aligned, so a smaller chip sits on the same baseline as its + // neighbours rather than floating in the middle of the row. Top + // alignment would step the tier down and read as a layout fault. + const int top = card.tagRect.top() + + (ownMetrics.height() - metrics.height()); + + painter->setFont(own ? ownFont : siblingFont); + TagChip::paint(painter, QRect(QPoint(x, top), size), tags.at(i), + colour); x += size.width() + TagChip::kSpacing; } painter->restore(); diff --git a/src/carddelegate.h b/src/carddelegate.h index 74dee8e..1846359 100644 --- a/src/carddelegate.h +++ b/src/carddelegate.h @@ -76,4 +76,22 @@ public: /// /// Falls back to threadLineColour() for a thread with no account tag. static QColor accentLineColour(const QColor &accountColour); + + /// A tag chip's colour, drained for the SIBLING tier (item 111). + /// + /// Saturation only: hue stays so the tag is still recognisable, and + /// lightness stays so `TagColors::textColourOn()` keeps choosing the same + /// text colour and the chip cannot become unreadable. Exposed for a test, + /// since "muted" has to be asserted on rather than eyeballed. + static QColor mutedChipColour(const QColor &chipColour); + + /// The size of one tag chip on a card, for either tier. + /// + /// The delegate's own arithmetic rather than a duplicate of it: a test + /// calling `TagChip::sizeFor` directly proves what that function does and + /// nothing about what the delegate ASKS for, which is where the padding + /// scale is chosen. A mutation dropping the scale at the call site + /// survived exactly that kind of test. + static QSize chipSize(const QFontMetrics &metrics, const QString &text, + bool own); }; diff --git a/src/cardlayout.cpp b/src/cardlayout.cpp index 0febb4b..3719591 100644 --- a/src/cardlayout.cpp +++ b/src/cardlayout.cpp @@ -107,6 +107,32 @@ QFont CardLayout::smallFont(const QFont &cardFont) return small; } +QFont CardLayout::siblingFont(const QFont &cardFont) +{ + // A FRACTION of the card's font, not a fixed number of points off it. + // + // Subtracting one point was the first attempt and the user reported the + // tiers as indistinguishable. The reason is arithmetic: their desktop is + // 14pt, so the two chip tiers were 13 and 12, a 7% step. Subtraction gives + // a step whose size depends on the desktop font, which is exactly backwards + // — it is largest where the text is already small enough to be fragile. + // + // 0.70 of the card font, against smallFont()'s one point off, so on a 14pt + // desktop the tiers are 13 and 10. Chosen with the user against rendered + // sizes rather than picked. + constexpr qreal kSiblingScale = 0.70; + + QFont small = cardFont; + // pointSizeF() returns -1 for a font set in PIXELS, which qt6ct does, and + // scaling -1 asks for an invalid size that Qt silently ignores, leaving + // both tiers identical. Same split as smallFont(), same reason. + if (small.pointSizeF() > 0.0) + small.setPointSizeF(qMax(6.0, cardFont.pointSizeF() * kSiblingScale)); + else if (small.pixelSize() > 0) + small.setPixelSize(qMax(8, qRound(cardFont.pixelSize() * kSiblingScale))); + return small; +} + int CardLayout::heightFor(const QFont &font) { const QFontMetrics metrics(font); diff --git a/src/cardlayout.h b/src/cardlayout.h index 3512242..de4c41f 100644 --- a/src/cardlayout.h +++ b/src/cardlayout.h @@ -160,6 +160,19 @@ struct CardLayout /// column of content. static QFont smallFont(const QFont &cardFont); + /// The font a SIBLING's tag chip is drawn in: a size down again from + /// smallFont(). + /// + /// A card stands for one message but sits above a conversation, and shows + /// both tiers (item 111). Size is what says which is which, so the two + /// must be visibly different; taking one more step from the same base + /// keeps it following the desktop's font rather than being fixed. + /// + /// Floored like smallFont(), and the floor really is reachable: a desktop + /// at the minimum size gives both tiers the same size, which is a legible + /// degradation rather than an illegible chip. + static QFont siblingFont(const QFont &cardFont); + static CardLayout compute(const Input &input, const QRect &rect, const QFont &font); diff --git a/src/keymap.cpp b/src/keymap.cpp index f6ef6a4..c731bbb 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -37,6 +37,21 @@ QStringList KeyMap::knownActions() QStringLiteral("edit_tags"), QStringLiteral("tag_rules"), QStringLiteral("flag"), + // The whole-thread counterparts (item 108). The names above act on the + // message a row displays; these act on its entire thread. Separate + // names rather than a scope flag, because a name is what a user writes + // in [keys]: giving `delete` new semantics would silently change an + // existing config, and renaming it would break one that mentions it. + // + // Unbound by default. They are reached through the "Whole thread" + // submenu, and inventing five more default chords for actions most + // users will rarely want is worse than leaving them to bind what they + // use. + QStringLiteral("archive_thread"), + QStringLiteral("delete_thread"), + QStringLiteral("spam_thread"), + QStringLiteral("toggle_unread_thread"), + QStringLiteral("flag_thread"), QStringLiteral("focus_query"), QStringLiteral("complete_query"), QStringLiteral("save_query"), @@ -90,6 +105,20 @@ QList<QPair<QString, QString>> KeyMap::defaultBindings() // action takes the harder chord rather than the easier one. { QStringLiteral("Ctrl+Shift+U"), QStringLiteral("mark_all_read") }, { QStringLiteral("Ctrl+I"), QStringLiteral("flag") }, + // The whole-thread tier (item 108), one modifier out from each + // message-scoped twin: Ctrl+D deletes the message a row displays, + // Ctrl+Alt+D deletes its conversation. + // + // Ctrl+ALT, not Ctrl+Shift. The obvious pairing is taken twice over: + // Ctrl+Shift+S is `spam` and Ctrl+Shift+U is `mark_all_read`, both + // shipped and both in users' fingers. Reusing either would silently + // change what an existing key does, which is the same objection that + // made these separate action names rather than a flag on the old ones. + { QStringLiteral("Ctrl+Alt+E"), QStringLiteral("archive_thread") }, + { QStringLiteral("Ctrl+Alt+D"), QStringLiteral("delete_thread") }, + { QStringLiteral("Ctrl+Alt+S"), QStringLiteral("spam_thread") }, + { QStringLiteral("Ctrl+Alt+U"), QStringLiteral("toggle_unread_thread") }, + { QStringLiteral("Ctrl+Alt+I"), QStringLiteral("flag_thread") }, { QStringLiteral("Ctrl+T"), QStringLiteral("edit_tags") }, // Shifted against Ctrl+T for the same reason Ctrl+Shift+U is shifted // against Ctrl+U: this is the standing version of tagging, applied to diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index fcf97df..ee883b0 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -832,15 +832,14 @@ void MainWindow::registerActions() // independently would leave one keystroke with the selection in two // states, which is worse than either outcome, so undelete only when // every selected thread is already deleted. - const QModelIndexList rows = - m_threadView->selectionModel()->selectedRows(); - bool allDeleted = !rows.isEmpty(); - for (const QModelIndex &index : rows) { - if (!m_model->threadAt(index.row()).isDeleted()) { - allDeleted = false; - break; - } - } + // + // Each row's own state, message or thread: a reply row is asked about + // the MESSAGE it stands for. Asking its thread made Delete one-way on + // a reply, since a message-scoped write never changes the thread's + // tags and the answer therefore stayed "not deleted" however many + // times it was pressed. Item 88 fixed which thread was read here; this + // is about reading a message at all. + const bool allDeleted = everySelectedRowHasTag(QStringLiteral("deleted")); if (allDeleted) tagSelected({}, { QStringLiteral("deleted") }, tr("Undelete")); @@ -866,21 +865,26 @@ void MainWindow::registerActions() }); addAction(QStringLiteral("toggle_unread"), tr("Toggle &unread"), tr("Toggle the unread tag"), [this]() { - // The direction comes from the current row, but the change applies to - // the whole selection, so a mixed selection lands in one consistent - // state rather than each row flipping its own way. - const QModelIndex current = m_threadView->currentIndex(); - if (!current.isValid()) - return; - const ThreadSummary thread = m_model->threadAt(current.row()); + // The state of whatever the rows STAND FOR, which for a reply is the + // message and not its thread. See everySelectedRowHasTag(): reading + // the thread here made the key dead on a reply. + // + // Item 88 fixed WHICH thread this read. That was necessary and not + // sufficient: a reply needs a message read, not a better thread. + // + // Per selection rather than per current row, matching Delete. The old + // comment said the direction came from the current row while the + // change applied to the whole selection, which is the same split that + // makes a mixed selection land in two states. + const bool unread = everySelectedRowHasTag(QStringLiteral("unread")); // An explicit toggle overrides the automatic one. Without this, marking // a thread unread by hand would be undone a moment later by a timer // armed when it was opened, and the key would look broken. m_markReadTimer->stop(); - m_markReadThreadId.clear(); + m_markReadMessageId.clear(); - if (thread.isUnread()) + if (unread) tagSelected({}, { QStringLiteral("unread") }, tr("Mark read")); else tagSelected({ QStringLiteral("unread") }, {}, tr("Mark unread")); @@ -894,6 +898,58 @@ void MainWindow::registerActions() tr("Add or remove any tag on the selected threads"), [this]() { editTagsOnSelection(); }); + + // The whole-thread counterparts (item 108). Separate action NAMES, because + // a name is what a user writes in [keys]: reusing `delete` with new + // semantics would silently change what an existing config does, and + // renaming it would break one that mentions it. These are unbound by + // default; the submenu is how they are reached. + // + // Each one is its message-scoped twin with TagScope::Thread, so the two + // cannot drift in what they write, only in what they write it to. + addAction(QStringLiteral("archive_thread"), tr("&Archive thread"), + tr("Remove inbox from every message of the selected threads"), + [this]() { + tagSelected({}, { QStringLiteral("inbox") }, tr("Archive thread"), + TagScope::Thread); + }); + addAction(QStringLiteral("delete_thread"), tr("&Delete thread"), + tr("Add or remove the deleted tag on whole threads"), [this]() { + if (everySelectedRowHasTag(QStringLiteral("deleted"), TagScope::Thread)) { + tagSelected({}, { QStringLiteral("deleted") }, + tr("Undelete thread"), TagScope::Thread); + } else { + tagSelected({ QStringLiteral("deleted") }, {}, tr("Delete thread"), + TagScope::Thread); + } + }); + addAction(QStringLiteral("spam_thread"), tr("Mark thread as &spam"), + tr("Add spam and remove inbox on whole threads"), [this]() { + tagSelected({ QStringLiteral("spam") }, { QStringLiteral("inbox") }, + tr("Mark thread spam"), TagScope::Thread); + }); + addAction(QStringLiteral("toggle_unread_thread"), tr("Toggle &unread"), + tr("Toggle the unread tag on whole threads"), [this]() { + // Cancels the automatic mark-read for the same reason its + // message-scoped twin does: a thread marked unread by hand must not be + // undone a moment later by a timer armed when it was opened. + m_markReadTimer->stop(); + m_markReadMessageId.clear(); + + if (everySelectedRowHasTag(QStringLiteral("unread"), TagScope::Thread)) { + tagSelected({}, { QStringLiteral("unread") }, + tr("Mark thread read"), TagScope::Thread); + } else { + tagSelected({ QStringLiteral("unread") }, {}, + tr("Mark thread unread"), TagScope::Thread); + } + }); + addAction(QStringLiteral("flag_thread"), tr("&Important"), + tr("Mark every message of the selected threads as important"), + [this]() { + tagSelected({ QStringLiteral("flagged") }, {}, + tr("Mark thread important"), TagScope::Thread); + }); addAction(QStringLiteral("tag_rules"), tr("Tagging &rules..."), tr("Edit the rules that tag mail as it arrives"), [this]() { showTagRulesDialog(); @@ -975,7 +1031,7 @@ void MainWindow::registerActions() m_messageView->clear(); showPlaceholderPane(); m_markReadTimer->stop(); - m_markReadThreadId.clear(); + m_markReadMessageId.clear(); }); addAction(QStringLiteral("clear_selection"), tr("Clear &selection"), tr("Blank the message pane and deselect every thread"), @@ -1010,7 +1066,7 @@ void MainWindow::registerActions() m_messageView->clear(); showPlaceholderPane(); m_markReadTimer->stop(); - m_markReadThreadId.clear(); + m_markReadMessageId.clear(); }); addAction(QStringLiteral("select_all"), tr("Select &all threads"), tr("Select every thread in the current result list"), [this]() { @@ -1061,6 +1117,8 @@ void MainWindow::buildMenus() messageMenu->addAction(m_actions.value(QStringLiteral("mark_all_read"))); messageMenu->addAction(m_actions.value(QStringLiteral("edit_tags"))); messageMenu->addAction(m_actions.value(QStringLiteral("flag"))); + messageMenu->addSeparator(); + messageMenu->addMenu(buildThreadActionsMenu(messageMenu)); // Separated from the entries above: those act on the selection, this edits // a rule store shared with mailctl and changes nothing that is on screen. messageMenu->addSeparator(); @@ -1141,6 +1199,20 @@ void MainWindow::buildMenus() { QStringLiteral("zoom_in"), QStringLiteral("zoom-in") }, { QStringLiteral("zoom_out"), QStringLiteral("zoom-out") }, { QStringLiteral("zoom_reset"), QStringLiteral("zoom-original") }, + + // The whole-thread tier (item 108) deliberately SHARES each icon with + // its message-scoped twin. The no-duplicates rule exists because the + // toolbar can be icon-only, where the icon is the entire control; + // these five never reach the toolbar. They live in a submenu whose + // entries always carry text, and "Delete thread" beside the delete + // icon is the honest pairing: the same operation, a wider scope, with + // the words saying which. Inventing five different shapes for the same + // five operations would be less clear, not more. + { QStringLiteral("archive_thread"), QStringLiteral("mail-archive") }, + { QStringLiteral("delete_thread"), QStringLiteral("edit-delete") }, + { QStringLiteral("spam_thread"), QStringLiteral("mail-mark-junk") }, + { QStringLiteral("toggle_unread_thread"), QStringLiteral("mail-mark-unread") }, + { QStringLiteral("flag_thread"), QStringLiteral("mail-mark-important") }, }; for (auto it = themeIcons.cbegin(); it != themeIcons.cend(); ++it) { QAction *action = m_actions.value(it.key()); @@ -1168,6 +1240,8 @@ void MainWindow::buildMenus() m_threadContextMenu->addAction(m_actions.value(QStringLiteral("flag"))); m_threadContextMenu->addAction(m_actions.value(QStringLiteral("edit_tags"))); m_threadContextMenu->addSeparator(); + m_threadContextMenu->addMenu(buildThreadActionsMenu(m_threadContextMenu)); + m_threadContextMenu->addSeparator(); m_threadContextMenu->addAction(m_actions.value(QStringLiteral("select_all"))); m_threadView->setContextMenuPolicy(Qt::CustomContextMenu); @@ -2384,7 +2458,7 @@ void MainWindow::markAllRead() // An automatic mark-read armed for the open thread would fire after this // and push a second, redundant command onto the stack. m_markReadTimer->stop(); - m_markReadThreadId.clear(); + m_markReadMessageId.clear(); const QString description = tr("Mark all read"); sendThreadTagChange(threadIds, {}, { QStringLiteral("unread") }, @@ -2448,16 +2522,16 @@ void MainWindow::onSelectionChanged() // Qt 6.11), so that handler still sees the old count and returns // without loading anything. // - // Compared per row kind. A message row's row number indexes its - // siblings, so threadAt() on one answers about an unrelated thread and - // the comparison below would be against the wrong id. + // Compared per row kind: a message row is identified by its message id + // and a thread row by its thread id, which are different questions. + // threadFor() resolves the thread either way, so the row-number trap + // (item 88) cannot be re-entered here even if this branch changes. const QModelIndex current = m_threadView->currentIndex(); if (current.isValid()) { const bool changed = m_model->isMessageRow(current) ? m_model->messageAt(current).messageId != m_currentMessageId - : m_model->threadAt(current.row()).threadId - != m_currentThreadId; + : m_model->threadFor(current).threadId != m_currentThreadId; if (changed) onThreadSelected(current, QModelIndex()); } @@ -2507,7 +2581,7 @@ void MainWindow::onSelectionChanged() // current, so onThreadSelected never runs and its guard never fires. The // pane and the pending timer have to be dealt with here as well. m_markReadTimer->stop(); - m_markReadThreadId.clear(); + m_markReadMessageId.clear(); m_currentThreadId.clear(); m_currentMessageId.clear(); m_currentMessageThreadId.clear(); @@ -2563,7 +2637,7 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, // pane that no longer shows the thread. if (m_threadView->selectionModel()->selectedRows().size() > 1) { m_markReadTimer->stop(); - m_markReadThreadId.clear(); + m_markReadMessageId.clear(); m_currentThreadId.clear(); m_currentMessageId.clear(); m_currentMessageThreadId.clear(); @@ -2572,21 +2646,22 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, return; } - // A message row renders that message ALONE. Checked before threadAt(), - // which takes a top-level row number: a child's row number indexes its - // siblings, so passing it here would silently load whichever thread happens - // to sit at that position in the list. + // A message row renders that message ALONE, so the kind of row still has + // to be checked here: this is a different render path, not a different way + // of naming the same thread. if (m_model->isMessageRow(current)) { const MessageNode node = m_model->messageAt(current); if (node.messageId.isEmpty()) return; - // No mark-read timer for a message row in this pass. Marking one - // message of a thread read is a per-message tag write, and the - // pending-edit map is keyed by thread; item 28 is the record of what - // happens when that count goes wrong. + // Armed for a reply too, since item 87. It deliberately was not + // before, because the write was thread-scoped and reading one reply + // would have marked the whole conversation read. With the write scoped + // to one message that objection is gone, and leaving it unarmed would + // make the message the user is actually reading the one kind that + // never gets marked read. m_markReadTimer->stop(); - m_markReadThreadId.clear(); + m_markReadMessageId.clear(); m_currentThreadId.clear(); m_currentMessageId = node.messageId; @@ -2594,16 +2669,27 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, // thread it came from is what the refreshed list is checked against. m_currentMessageThreadId = node.threadId; m_messageView->setTags(node.tags); + + // After m_currentMessageId is set: the handler compares against it to + // tell "still showing this" from "the selection moved on". + scheduleMarkRead(node.messageId, node.isUnread()); + QMetaObject::invokeMethod(m_worker, "loadMessage", Qt::QueuedConnection, Q_ARG(QString, node.messageId), Q_ARG(quint64, m_generation)); return; } - const ThreadSummary thread = m_model->threadAt(current.row()); + const ThreadSummary thread = m_model->threadFor(current); m_currentThreadId = thread.threadId; m_messageView->setTags(thread.tags); - scheduleMarkRead(thread); + + // The message the card displays, not the thread. The summary's `unread` is + // a union over the conversation, so this can arm for a thread whose first + // message is already read; the write is scoped to that message either way, + // so the cost is a no-op rather than a wrong write. Narrowing it properly + // needs per-message state in ThreadSummary, which nothing carries yet. + scheduleMarkRead(thread.firstMessageId, thread.isUnread()); // The root card IS the thread's first message, so selecting it renders // that message. Never the whole conversation: that path is gone (item 66). @@ -2654,6 +2740,25 @@ void MainWindow::onMessageLoaded(const QVector<MessageRef> &messages, if (m_currentMessageId.isEmpty()) return; + // The worker's answer is the authority on what THIS message carries, and + // it is the only place that truth arrives. Until it does, a thread row can + // only offer ThreadSummary::tags, which is notmuch's union over the + // conversation: a four-message thread whose third message is signed makes + // the root card and the pane both claim `signed` for a message that is not + // (item 110). Recording it here corrects the card and gives a + // message-scoped write something to update, which is why marking a root + // message read left the row bold before. + // + // A reply already has its own node from the thread tree, and + // setRootMessageTags ignores anything that is not a root. + for (const MessageRef &ref : messages) + m_model->setRootMessageTags(ref.messageId, ref.tags); + + // The pane follows the same correction. setTags() at selection time can + // only have used the union. + if (messages.size() == 1) + m_messageView->setTags(messages.first().tags); + renderMessages(messages); } @@ -2741,7 +2846,12 @@ void MainWindow::renderMessages(const QVector<MessageRef> &messages) void MainWindow::revertPendingTagChange() { - if (m_pendingThreadIds.isEmpty()) + // Either scope can be in flight: a thread-scoped write names threads, a + // message-scoped one names messages, and both are now applied + // optimistically. Checking only the thread ids left a failed message write + // showing its optimistic state for good, with nothing to correct it until + // the next query. + if (m_pendingThreadIds.isEmpty() && m_pendingChange.messageIds.isEmpty()) return; // Put the rows back the way they were. Only the model is touched: the @@ -2750,6 +2860,10 @@ void MainWindow::revertPendingTagChange() m_model->applyTagChange(threadId, m_pendingChange.removed, m_pendingChange.added); } + for (const QString &messageId : m_pendingChange.messageIds) { + m_model->applyMessageTagChange(messageId, m_pendingChange.removed, + m_pendingChange.added); + } // The undo entry describes a change that never landed, so it would apply a // spurious inverse if the user pressed undo. @@ -2805,18 +2919,37 @@ void MainWindow::flushHeldEdits() m_flushGeneration = m_generation; for (const HeldEdit &edit : edits) { - // Take the optimistic update back before sending, because - // sendThreadTagChange() applies it again. applyTagChange() is - // idempotent per tag so the rows do not visibly flicker; without this - // the change is applied twice and a later revert undoes only one of - // them, leaving a row showing a tag the database never got. + // Take the optimistic update back before sending, because the send + // applies it again. Both apply functions are idempotent per tag so the + // rows do not visibly flicker; without this the change is applied + // twice and a later revert undoes only one of them, leaving a row + // showing a tag the database never got. for (const QString &threadId : edit.threadIds) { m_model->applyTagChange(threadId, edit.change.removed, edit.change.added); } + for (const QString &messageId : edit.change.messageIds) { + m_model->applyMessageTagChange(messageId, edit.change.removed, + edit.change.added); + } - sendThreadTagChange(edit.threadIds, edit.change.added, - edit.change.removed, edit.change.description); + // By SCOPE. A held edit is one or the other, never both: a + // message-scoped edit carries no thread ids, so sending it through + // sendThreadTagChange() sent an empty list, which returns immediately. + // The edit was applied to the row, counted as unsynced and then + // dropped without ever being written, which is data loss with a + // pending count claiming the opposite. + // + // Escalating it to its thread instead would be worse: Delete on one + // reply would delete every message in the conversation. + if (!edit.threadIds.isEmpty()) { + sendThreadTagChange(edit.threadIds, edit.change.added, + edit.change.removed, edit.change.description); + } + if (!edit.change.messageIds.isEmpty()) { + sendMessageTagChange(edit.change.messageIds, edit.change.added, + edit.change.removed, edit.change.description); + } } // Held edits stop counting as held; what counts now is whatever @@ -3149,7 +3282,7 @@ void MainWindow::onRowDoubleClicked(const QModelIndex &index) // the recovery selects. What is cancelled is the arming for a row the user // is leaving. m_markReadTimer->stop(); - m_markReadThreadId.clear(); + m_markReadMessageId.clear(); // Reuses the stale-thread recovery outright, which already runs thread:<id>, // expands the thread when the row arrives, selects the target message once @@ -3604,27 +3737,32 @@ void MainWindow::updatePendingIndicator() m_pendingLabel->show(); } -void MainWindow::scheduleMarkRead(const ThreadSummary &thread) +void MainWindow::scheduleMarkRead(const QString &messageId, bool unread) { - // Any pending timer belongs to a thread that is no longer on screen. + // Any pending timer belongs to a message that is no longer on screen. // Stopping unconditionally is what makes this a restart rather than a - // stack: arrowing down ten threads must mark only the one still selected + // stack: arrowing down ten rows must mark only the one still selected // when the timer finally fires. m_markReadTimer->stop(); - m_markReadThreadId.clear(); + m_markReadMessageId.clear(); // Negative disables the behaviour entirely, per the config key. const int delay = m_config.markReadDelayMs(); if (delay < 0) return; - // Nothing to do for a thread that is already read. Checked here rather - // than in the handler so no timer is even armed, which keeps a read thread - // from arming one that would fire into a no-op write. - if (!thread.tags.contains(QStringLiteral("unread"))) + // A row the model cannot name a message for. Marking its thread instead + // would be the escalation item 108 removed. + if (messageId.isEmpty()) + return; + + // Nothing to do for a message that is already read. Checked here rather + // than in the handler so no timer is even armed, which keeps a read + // message from arming one that would fire into a no-op write. + if (!unread) return; - m_markReadThreadId = thread.threadId; + m_markReadMessageId = messageId; // Zero means immediately, and a zero-interval timer still fires through // the event loop rather than reentering the selection handler. @@ -3633,62 +3771,167 @@ void MainWindow::scheduleMarkRead(const ThreadSummary &thread) void MainWindow::markCurrentThreadRead() { - if (m_markReadThreadId.isEmpty()) + if (m_markReadMessageId.isEmpty()) return; // The selection can have moved on between the timer being armed and it - // firing, and the thread can have been marked read by hand in that window. - // Both mean this timer has nothing left to do. - if (m_markReadThreadId != m_currentThreadId) { - m_markReadThreadId.clear(); - return; - } - - const QModelIndex current = m_threadView->currentIndex(); - if (!current.isValid()) { - m_markReadThreadId.clear(); - return; - } - - const ThreadSummary thread = m_model->threadAt(current.row()); - if (thread.threadId != m_markReadThreadId - || !thread.tags.contains(QStringLiteral("unread"))) { - m_markReadThreadId.clear(); + // firing, and the message can have been marked read by hand in that + // window. Both mean this timer has nothing left to do. + // + // Compared against what the PANE is showing rather than against the + // selection: those are the same thing for both kinds of row, and the pane + // is what "the message the user is reading" means. + const QString showing = m_currentMessageId.isEmpty() + ? currentThreadFirstMessageId() + : m_currentMessageId; + if (m_markReadMessageId != showing) { + m_markReadMessageId.clear(); return; } - const QStringList threadIds = { m_markReadThreadId }; - m_markReadThreadId.clear(); + const QStringList messageIds = { m_markReadMessageId }; + m_markReadMessageId.clear(); - // sendThreadTagChange, NOT tagSelected: this deliberately does not go on + // sendMessageTagChange, NOT tagSelected: this deliberately does not go on // the undo stack. The user never took this action, so hijacking Ctrl+Z to // reverse it would undo something they did not do, and toggle_unread // already gives them a direct way to put it back. Decided 2026-08-03. // + // MESSAGE-scoped since item 87. The thread-wide write was coherent while a + // root card rendered the whole conversation; item 66 made it render one + // message and left the write alone, so reading one message marked replies + // read that had never been displayed. maildir.synchronize_flags is on, so + // that reached the server and nothing here could put it back. + // // It still funnels through the one applyTags path, per CLAUDE.md; what // differs is only whether the inverse is pushed, which is a window-level // decision above the worker. - sendThreadTagChange(threadIds, {}, { QStringLiteral("unread") }, - tr("Mark read")); + sendMessageTagChange(messageIds, {}, { QStringLiteral("unread") }, + tr("Mark read")); } -void MainWindow::editTagsOnSelection() +QString MainWindow::currentThreadFirstMessageId() const +{ + // The message a selected THREAD row displays. m_currentThreadId is what + // the pane was opened from, so this resolves through the model rather than + // through the selection, which can have moved. + if (m_currentThreadId.isEmpty()) + return {}; + + for (int row = 0; row < m_model->rowCount(QModelIndex()); ++row) { + const ThreadSummary thread = m_model->threadAt(row); + if (thread.threadId == m_currentThreadId) + return thread.firstMessageId; + } + return {}; +} + +bool MainWindow::everySelectedRowHasTag(const QString &tag, + TagScope scope) const { + // What a toggle asks before choosing its direction, for both Delete and + // Toggle unread. + // + // Per ROW, and each row is asked about what it stands for: a reply row + // reports the message's tags, a thread row the thread's. Asking a reply's + // THREAD is the trap both toggles fell into. The write is message-scoped, + // so it never changes the thread's tags; the thread's answer therefore + // never moves however many times the key is pressed, and the toggle + // becomes one-way. On the second press it re-sends a tag the message + // already has, which is a no-op, and a no-op repaints nothing. + // + // One direction for the WHOLE selection, which is the rule Delete + // established: toggling each row independently would leave one keystroke + // with the selection in two states, which is worse than either outcome. const QModelIndexList rows = m_threadView->selectionModel()->selectedRows(); - if (rows.isEmpty()) { - showTransientStatus(tr("Select a thread first")); - return; + if (rows.isEmpty()) + return false; + + for (const QModelIndex &index : rows) { + QStringList tags; + if (scope == TagScope::Thread) { + tags = m_model->threadFor(index).tags; + } else if (m_model->isMessageRow(index)) { + tags = m_model->messageAt(index).tags; + } else { + // The thread's summary, and this is a KNOWN approximation rather + // than an oversight. A thread row acts on the message its card + // displays, but that message's own tags are never in the model: + // setThreadMessages drops depth 0 because the root row stands for + // it, so there is no node to read and messageById() cannot find + // one. The summary is a union over the thread, so it answers + // "unread" while ANY message is. + // + // The consequence is bounded and only affects the DIRECTION a + // toggle picks, never what it writes: on a thread whose first + // message is read while a later one is not, Toggle unread reads + // the thread as unread and marks the first message read again, a + // no-op. Fixing it properly needs per-message state in + // ThreadSummary, which is the same thing item 87 needs; leave it + // for that item rather than guessing here. + tags = m_model->threadFor(index).tags; + } + if (!tags.contains(tag)) + return false; } + return true; +} + +ThreadSummary MainWindow::threadForCurrentRowForTesting() const +{ + return m_model->threadFor(m_threadView->currentIndex()); +} - // How many of the selected threads carry each tag, which is what tells a - // tag that is on all of them from one that is on some. +QMenu *MainWindow::buildThreadActionsMenu(QWidget *parent) +{ + // Built per call rather than shared. A QMenu belongs to one place in one + // menu tree, and adding the same instance to both the menu bar and the + // context menu gives whichever added it last the object. The ACTIONS are + // shared, which is what has to stay consistent; the menu holding them is + // just a container. + auto *menu = new QMenu(tr("&Whole thread"), parent); + menu->setObjectName(QStringLiteral("threadActionsMenu")); + menu->addAction(m_actions.value(QStringLiteral("archive_thread"))); + menu->addAction(m_actions.value(QStringLiteral("delete_thread"))); + menu->addAction(m_actions.value(QStringLiteral("spam_thread"))); + menu->addSeparator(); + menu->addAction(m_actions.value(QStringLiteral("toggle_unread_thread"))); + menu->addAction(m_actions.value(QStringLiteral("flag_thread"))); + return menu; +} + +QHash<QString, int> MainWindow::selectionTagCounts() const +{ + // How many of the selected rows carry each tag, which is what tells a tag + // that is on all of them from one that is on some. The dialog's tri-state + // checkboxes are built from this, so a wrong count offers to remove a tag + // the selection does not have. + // + // threadFor(index), NOT threadAt(index.row()): a reply's row number named + // an unrelated thread, so selecting one counted the tags of whichever + // thread sat at that position in the list (item 88). QHash<QString, int> counts; + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); for (const QModelIndex &index : rows) { - const ThreadSummary thread = m_model->threadAt(index.row()); + const ThreadSummary thread = m_model->threadFor(index); for (const QString &tag : thread.tags) counts[tag] += 1; } + return counts; +} + +void MainWindow::editTagsOnSelection() +{ + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + if (rows.isEmpty()) { + showTransientStatus(tr("Select a thread first")); + return; + } + + const QHash<QString, int> counts = selectionTagCounts(); // m_knownTags is the same list the query completer uses, so the dialog // offers every tag in the database without a round trip. @@ -3708,7 +3951,7 @@ void MainWindow::editTagsOnSelection() } void MainWindow::tagSelected(const QStringList &add, const QStringList &remove, - const QString &description) + const QString &description, TagScope tagScope) { const QModelIndexList rows = m_threadView->selectionModel()->selectedRows(); @@ -3719,7 +3962,13 @@ void MainWindow::tagSelected(const QStringList &add, const QStringList &remove, // A message row's row number indexes its siblings, so the old // threadAt(index.row()) mapping silently acted on whichever thread sat at // that position in the list. - const ActionScope scope = m_model->scopeFor(rows); + // + // Message scope by default since item 108: a thread row displays one + // message, so acting on it acts on that message. Thread scope is what the + // "Whole thread" actions ask for explicitly. + const ActionScope scope = tagScope == TagScope::Thread + ? m_model->scopeFor(rows) + : m_model->messageScopeFor(rows); if (scope.isEmpty()) return; @@ -3758,10 +4007,33 @@ void MainWindow::sendMessageTagChange(const QStringList &messageIds, if (messageIds.isEmpty()) return; - // No optimistic model update. applyTagChange is keyed by THREAD and would - // repaint the whole row as though every message in it had changed, which - // for a one-message edit is a lie the user would see and then watch - // silently correct itself on the next query. + // Optimistically applied to each MESSAGE's own row. applyTagChange is + // keyed by thread and would repaint the whole card as though every message + // in it had changed, which for a one-message edit is a lie; that is why + // this path had no optimistic update at all, and the cost was that Delete + // and Toggle unread on a reply moved the pending count and changed nothing + // the user could see. The reply's own row is where the feedback belongs. + for (const QString &messageId : messageIds) + m_model->applyMessageTagChange(messageId, add, remove); + + // The strip shows the tags of the message ON DISPLAY, so it has to follow + // an edit to that message rather than waiting for the next selection. The + // thread path has carried this since the strip existed; without it here, a + // message-scoped edit repainted the list row and left the pane's chips + // describing the message as it was, until the user selected away and back. + // + // Keyed on m_currentMessageId, which is set only for a message row, so a + // write to some other reply cannot repaint the open one with its tags. + // + // Read by ID, not from currentIndex(): the two agree today, and a guard + // that depends on them agreeing would put the WRONG message's tags in the + // pane on the day they do not. The id is what the pane is actually + // showing. + if (!m_currentMessageId.isEmpty() + && messageIds.contains(m_currentMessageId)) { + m_messageView->setTags( + m_model->messageById(m_currentMessageId).tags); + } // The accounts this touches, resolved through the containing threads: the // account is a property of the thread, and the sync needs the channel @@ -3821,7 +4093,7 @@ void MainWindow::sendThreadTagChange(const QStringList &threadIds, if (threadIds.contains(m_currentThreadId)) { const QModelIndex current = m_threadView->currentIndex(); if (current.isValid()) - m_messageView->setTags(m_model->threadAt(current.row()).tags); + m_messageView->setTags(m_model->threadFor(current).tags); } // A sync holds notmuch's exclusive write lock, and the worker's read-write diff --git a/src/mainwindow.h b/src/mainwindow.h index 614430f..e741416 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -167,6 +167,37 @@ public: /// command was pushed, which is what "this did nothing" has to assert. int undoDepthForTesting() const { return m_undoStack.count(); } + /// The text of the command on top of the undo stack. + /// + /// A test seam for the DIRECTION a toggle chose. Delete and Undelete both + /// push one command and touch the same rows, so a depth or an id says + /// nothing about which way the toggle went, which is exactly what item 88 + /// got wrong. + QString undoTextForTesting() const { return m_undoStack.undoText(); } + + /// The tag counts the tag dialog would be built from, for the current + /// selection. A test seam: the dialog is modal, so the counts cannot be + /// observed through it. + QHash<QString, int> selectionTagCountsForTesting() const + { + return selectionTagCounts(); + } + + /// The thread the current row belongs to. A test seam for item 88's + /// resolution itself, reachable when the write it guards is not. + ThreadSummary threadForCurrentRowForTesting() const; + + /// Sends a message-scoped tag change directly. A test seam for the cases + /// where driving the action would move the selection, which is sometimes + /// the very thing under test. + void sendMessageTagChangeForTesting(const QStringList &messageIds, + const QStringList &add, + const QStringList &remove, + const QString &description) + { + sendMessageTagChange(messageIds, add, remove, description); + } + /// The ids the last tag change was sent for, and whether they were thread /// ids or message ids. /// @@ -180,6 +211,11 @@ public: return m_pendingChange.messageIds; } + /// The whole change last sent, for tests about WHAT was written rather + /// than what it was written to. The tags are the same under either scope, + /// so a test about a tag name should read this instead of a model row. + TagChange pendingChangeForTesting() const { return m_pendingChange; } + /// The generation a worker reply must carry to be accepted. /// /// A test seam: onQueryFinished() discards a reply whose generation is @@ -563,13 +599,32 @@ private: const QString &description, const std::function<void()> &handler); + /// What a tag action acts on. + /// + /// Since item 108 a thread ROW means the one message its card displays, so + /// Message is the default and Thread is the explicit choice the user makes + /// through the "Whole thread" submenu. Before that there was no choice: + /// a thread row always meant the conversation. + enum class TagScope { + Message, ///< The message each selected row displays. + Thread, ///< Every message of each selected row's thread. + }; + void tagSelected(const QStringList &add, const QStringList &remove, - const QString &description); + const QString &description, + TagScope scope = TagScope::Message); /// Starts, restarts or cancels the mark-read timer for a newly opened - /// thread. Cancels outright for a thread that is not unread, so an already - /// read thread never schedules a write that would change nothing. - void scheduleMarkRead(const ThreadSummary &thread); + /// MESSAGE. Cancels outright for one that is not unread, so an already read + /// message never schedules a write that would change nothing. + /// + /// Takes the id and the state separately because the two come from + /// different places: a reply row has a MessageNode, and a thread row has + /// only its summary, whose `unread` is a union over the conversation. + void scheduleMarkRead(const QString &messageId, bool unread); + + /// The message id of the thread the pane was opened from, or empty. + QString currentThreadFirstMessageId() const; /// Removes `unread` from the thread the timer was armed for, if it is still /// the one on screen. @@ -631,6 +686,29 @@ private: /// /// The only route to an arbitrary tag: every other tag action writes a /// hardcoded name. + /// The "Whole thread" submenu, built fresh for each parent that needs one. + /// + /// A QMenu lives in one menu tree, so the menu bar and the context menu get + /// their own instance. The actions inside are shared, which is what has to + /// stay consistent between them. + QMenu *buildThreadActionsMenu(QWidget *parent); + + /// Per-tag counts across the selected rows, for the tag dialog. + QHash<QString, int> selectionTagCounts() const; + + /// True when every selected row already carries \p tag, which is what a + /// toggle asks before choosing its direction. + /// + /// Under Message scope each row answers about what it STANDS FOR: a reply + /// row about its message, a thread row about the message its card + /// displays. Asking a reply's thread makes a toggle one-way, since the + /// message-scoped write never changes the thread's tags. + /// + /// Under Thread scope a row answers about its whole thread, so the + /// question matches the write the thread actions are about to make. + bool everySelectedRowHasTag(const QString &tag, + TagScope scope = TagScope::Message) const; + void editTagsOnSelection(); /// Set once the user has answered the exit prompt, or once a sync started @@ -981,10 +1059,17 @@ private: /// fires, not each one passed through. QTimer *m_markReadTimer = nullptr; - /// The thread m_markReadTimer will mark read. Compared against the current - /// selection when it fires, so a timer that outlives its thread does - /// nothing rather than marking the wrong one. - QString m_markReadThreadId; + /// The MESSAGE m_markReadTimer will mark read. Compared against what the + /// pane is showing when it fires, so a timer that outlives its message + /// does nothing rather than marking the wrong one. + /// + /// A message id, not a thread id, since item 87. The timer used to mark + /// the whole thread, which was coherent while a root card rendered the + /// whole conversation and stopped being so when item 66 made it render + /// one message: reading one message marked replies read that had never + /// been displayed, and with maildir.synchronize_flags on that reaches the + /// server. + QString m_markReadMessageId; /// Debounces the automatic sync that follows a tag edit (item 71). /// diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index a3a2fd5..b152830 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -314,6 +314,9 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation, if (matched) { summary.firstMessageId = QString::fromUtf8( notmuch_message_get_message_id(message)); + // The card's own tags, beside the thread's union above. + // Same walk, same index read, no extra query. + summary.firstMessageTags = tagsOf(message); break; } } @@ -323,6 +326,9 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation, if (notmuch_message_t *first = notmuch_messages_get(top)) { summary.firstMessageId = QString::fromUtf8( notmuch_message_get_message_id(first)); + // The card's own tags, beside the thread's union above. + // Same walk, same index read, no extra query. + summary.firstMessageTags = tagsOf(first); } } } diff --git a/src/tagchip.cpp b/src/tagchip.cpp index d770a56..ec83641 100644 --- a/src/tagchip.cpp +++ b/src/tagchip.cpp @@ -29,8 +29,18 @@ namespace TagChip { QSize sizeFor(const QFontMetrics &metrics, const QString &text) { - return QSize(metrics.horizontalAdvance(text) + kPaddingX * 2, - metrics.height() + kPaddingY * 2); + return sizeFor(metrics, text, 1.0); +} + +QSize sizeFor(const QFontMetrics &metrics, const QString &text, qreal scale) +{ + // Floored at 2 a side: the corner radius is half the chip's height, so the + // leftmost and rightmost pixels of the fill are curve rather than usable + // width, and text set flush against it touches the round end. + const int padX = qMax(2, qRound(kPaddingX * scale)); + const int padY = qMax(0, qRound(kPaddingY * scale)); + return QSize(metrics.horizontalAdvance(text) + padX * 2, + metrics.height() + padY * 2); } void paint(QPainter *painter, const QRect &rect, const QString &text, diff --git a/src/tagchip.h b/src/tagchip.h index 5514cae..68e1e55 100644 --- a/src/tagchip.h +++ b/src/tagchip.h @@ -43,6 +43,19 @@ constexpr int kSpacing = 4; QSize sizeFor(const QFontMetrics &metrics, const QString &text); +/// The same, with the padding scaled by \p scale. +/// +/// The padding is a fixed pixel count, which is right for one chip size and +/// wrong the moment there are two: at 1.0 it is 18px around roughly 30px of +/// text on a sibling chip, so the chip stays wide while its text shrinks and +/// the tier reads as "same size, smaller letters" rather than as a smaller +/// chip. Scaling it with the font is what makes the second tier actually look +/// smaller (item 111). +/// +/// Floored at 2px a side, because the corner radius is half the height and a +/// chip with no horizontal padding has its text touching the curve. +QSize sizeFor(const QFontMetrics &metrics, const QString &text, qreal scale); + /// Paints the chip into `rect`, using `text` and `background`. The text colour /// is derived from the fill so it stays legible. void paint(QPainter *painter, const QRect &rect, const QString &text, diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 3e079ed..6ddd85c 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -347,6 +347,15 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const case DateFormatRole: return m_dateFormat; case Qt::BackgroundRole: + // Doomed first: a reply tagged deleted or spam is on its way out + // and the user has to see that the moment they act, exactly as a + // thread row does. Without this branch a message-scoped Delete + // repainted a reply identically to an undeleted one, so the + // pending count moved and nothing on screen did. + if (node.isDoomed()) + return QBrush(node.isDeleted() ? deletedColour() + : spamColour()); + // Tinted, so an expanded thread reads as one block rather than as // more table rows. Applied per cell here; ThreadListView fills the // same colour across the strip's band so the row does not end up @@ -354,18 +363,47 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const return replyBackground(); case Qt::FontRole: { // A size down from the thread rows, so a thread reads as the - // heading and its replies as the contents. Never bold: an unread - // reply is still subordinate to the thread it belongs to, and the - // thread row above already carries the unread cue for the whole - // conversation. + // heading and its replies as the contents. The size is what keeps + // a reply subordinate; bold on top of it is the unread cue, at the + // user's request on 2026-08-16. + // + // Replies were unbolded deliberately at first, on the reasoning + // that the thread row above already says the conversation has + // unread mail. That is true of the THREAD and useless for the + // reply: once a thread is expanded, the row telling the user which + // messages in it are unread is the only one that can, and dimming + // alone left the user unable to see a read/unread change at all. QFont font = QGuiApplication::font(); if (font.pointSize() > 0) font.setPointSize(qMax(6, font.pointSize() - 1)); else if (font.pixelSize() > 0) font.setPixelSize(qMax(8, font.pixelSize() - 2)); + + // Bold combines with the dimming rather than replacing it: two + // cues for one state, which is what the thread row has had since + // 2026-08-07 and for the same reason. If the desktop's own font is + // configured Bold, setBold() changes nothing and the dimming is + // the whole cue, which CLAUDE.md records as a real configuration + // on this user's machine. + if (node.isUnread()) + font.setBold(true); + + // Struck through when doomed, for the same reason the thread row + // is: the state then survives a screenshot, a colourblind reader, + // and a theme that overrides the background. A reply had neither + // this nor the fill, so a message-scoped Delete was invisible. + if (node.isDoomed()) + font.setStrikeOut(true); return font; } case Qt::ForegroundRole: + // White over the doomed fill, matching the thread row. The dimmed + // read colour is mixed toward the BACKGROUND, so leaving it here + // would compute a grey against the pane's base and then paint it + // over red. + if (node.isDoomed()) + return QBrush(QColor(Qt::white)); + // Dimmed whether read or not, for the same reason as the font: a // reply is subordinate content. An unread one is left undimmed so // it can still be found. @@ -378,7 +416,26 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const if (index.row() >= m_threads.size()) return {}; - const ThreadSummary &thread = m_threads.at(index.row()).summary; + const ThreadNode &rowNode = m_threads.at(index.row()); + + // A card stands for ONE message since item 108, so it must draw that + // message's tags and not the thread's. `ThreadSummary::tags` is notmuch's + // UNION over the conversation: a four-message thread whose third message + // is signed reads as signed, and the card said so about a message that was + // not (item 110). + // + // Only the tags are substituted. Everything else on the card, the subject, + // the authors, the date and the reply count, describes the THREAD and is + // correct as it stands; only the tags were ever the union that lied. + // + // `first.tags` is populated when the message is loaded, which is when the + // user selects the row. Before that the union is the only answer available + // and is what the card shows, which is why an unopened row can still + // display a sibling's mark. Narrowing that further needs per-message state + // in the query itself. + ThreadSummary thread = rowNode.summary; + if (!rowNode.first.messageId.isEmpty()) + thread.tags = rowNode.first.tags; if (role == ThreadIdRole) return thread.threadId; @@ -424,7 +481,8 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const if (role == MessageOwnColoursRole) return QVariantList(); - if (role == PillTagsRole || role == PillColoursRole) { + if (role == PillTagsRole || role == PillColoursRole + || role == PillOwnCountRole) { // Everything the row already says another way is dropped: the account // is the chip in the subject cell, flagged is the star column, // attachment is the paperclip, unread is the row not being dimmed, and @@ -443,16 +501,43 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const QStringLiteral("unread"), }; - QStringList pills; - for (const QString &tag : thread.tags) { - if (hidden.contains(tag) || isDrawnAsAMark(tag) - || TagColors::isAccountTag(tag)) - continue; - pills.append(tag); + const auto pillsFrom = [&](const QStringList &tags) { + QStringList pills; + for (const QString &tag : tags) { + if (hidden.contains(tag) || isDrawnAsAMark(tag) + || TagColors::isAccountTag(tag)) + continue; + pills.append(tag); + } + // Sorted rather than in notmuch's order, which is not guaranteed + // stable: a row whose pills reordered between repaints would + // flicker. + pills.sort(); + return pills; + }; + + // `thread.tags` is the displayed message's own tags once the row has + // been opened, and the thread's union before that (see the + // substitution above). The union is always the full set, so the + // difference is what belongs only to siblings. + QStringList pills = pillsFrom(thread.tags); + const int ownCount = pills.size(); + + // The sibling tier, appended after the message's own. Shown rather + // than dropped at the user's request: a card sits above a + // conversation, so what the rest of it carries is worth seeing, just + // not at the same weight. The delegate draws these smaller and muted. + // + // Empty until the row has been opened, because before that + // `thread.tags` IS the union and the difference is nothing. That is + // what makes a chip shrink rather than appear. + for (const QString &tag : pillsFrom(rowNode.summary.tags)) { + if (!pills.contains(tag)) + pills.append(tag); } - // Sorted rather than in notmuch's order, which is not guaranteed - // stable: a row whose pills reordered between repaints would flicker. - pills.sort(); + + if (role == PillOwnCountRole) + return ownCount; if (role == PillTagsRole) return pills; @@ -598,6 +683,23 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const return {}; } +ThreadListModel::ThreadNode +ThreadListModel::nodeFor(const ThreadSummary &summary) +{ + ThreadNode node{ summary, {}, {}, false }; + + // Only when the query actually supplied them. An empty list here would be + // indistinguishable from "this message carries nothing", which would put + // every chip in the sibling tier and mute the whole card. + if (!summary.firstMessageId.isEmpty() + && !summary.firstMessageTags.isEmpty()) { + node.first.messageId = summary.firstMessageId; + node.first.threadId = summary.threadId; + node.first.tags = summary.firstMessageTags; + } + return node; +} + void ThreadListModel::appendBatch(const QVector<ThreadSummary> &batch) { // beginInsertRows with an empty range violates Qt's contract, so the guard @@ -608,7 +710,7 @@ void ThreadListModel::appendBatch(const QVector<ThreadSummary> &batch) const int first = m_threads.size(); beginInsertRows({}, first, first + batch.size() - 1); for (const ThreadSummary &summary : batch) - m_threads.append(ThreadNode{ summary, {}, {}, false }); + m_threads.append(nodeFor(summary)); endInsertRows(); } @@ -657,7 +759,7 @@ void ThreadListModel::reconcile(const QVector<ThreadSummary> &threads) if (it == present.constEnd()) { const int at = qMin(target, m_threads.size()); beginInsertRows({}, at, at); - m_threads.insert(at, ThreadNode{ summary, {}, {}, false }); + m_threads.insert(at, nodeFor(summary)); endInsertRows(); // Every later row shifted by one, and the map is read again on the @@ -707,8 +809,26 @@ void ThreadListModel::reconcile(const QVector<ThreadSummary> &threads) || m_threads.at(row).summary.authors != summary.authors || m_threads.at(row).summary.date != summary.date || m_threads.at(row).summary.totalCount != summary.totalCount - || m_threads.at(row).summary.matchedCount != summary.matchedCount) { + || m_threads.at(row).summary.matchedCount != summary.matchedCount + // The card's OWN message, which can move while the thread's union + // does not: a root read elsewhere leaves the thread unread as long + // as any reply is. Without this the card kept the tags it was + // first given, and the sibling tier with them. + || m_threads.at(row).summary.firstMessageTags + != summary.firstMessageTags) { m_threads[row].summary = summary; + + // The node too, since the card draws its tags from there. Only the + // tags: the node's children and loaded flag are the expansion + // state this whole method exists to preserve, and `first` carries + // no children. + if (!summary.firstMessageId.isEmpty() + && !summary.firstMessageTags.isEmpty()) { + m_threads[row].first.messageId = summary.firstMessageId; + m_threads[row].first.threadId = summary.threadId; + m_threads[row].first.tags = summary.firstMessageTags; + } + emit dataChanged(index(row, 0), index(row, 0)); } } @@ -804,6 +924,108 @@ QString ThreadListModel::threadIdForMessage(const QString &messageId) const return {}; } +void ThreadListModel::setRootMessageTags(const QString &messageId, + const QStringList &tags) +{ + if (messageId.isEmpty()) + return; + + for (int row = 0; row < m_threads.size(); ++row) { + ThreadNode &node = m_threads[row]; + if (node.summary.firstMessageId != messageId + && node.first.messageId != messageId) { + continue; + } + + if (node.first.tags == tags && !node.first.messageId.isEmpty()) + return; // Nothing changed; do not churn the view. + + // Enough of a node for the card to draw from. The rest of the display + // still comes from the summary, which is correct for it: the subject, + // the authors and the date describe the thread, and only the TAGS were + // ever the union that lied about this message. + node.first.messageId = messageId; + node.first.threadId = node.summary.threadId; + node.first.tags = tags; + + const QModelIndex threadIndex = index(row, 0, QModelIndex()); + emit dataChanged(threadIndex, threadIndex); + return; + } +} + +MessageNode ThreadListModel::messageById(const QString &messageId) const +{ + if (messageId.isEmpty()) + return {}; + + for (const ThreadNode &node : m_threads) { + // The root's own message first, and it is not among the children: + // setThreadMessages drops depth 0 because the root row stands for it. + // Searching only the children returned a default-constructed node for + // every root message, and a caller that trusted it set the message + // pane's tag strip to that empty tag list, wiping a strip that had + // been correct. + if (!node.first.messageId.isEmpty() + && node.first.messageId == messageId) { + return node.first; + } + + // Before expansion there is no node, so the answer is assembled from + // the summary: for a thread of one, its tags ARE this message's, since + // a thread's tags are a union over its messages. For a longer thread + // they are a union over messages this one is only part of, which is + // wider than the truth but is also exactly what the card shows, so a + // caller repainting from it stays consistent with the row beside it. + if (node.summary.firstMessageId == messageId) { + MessageNode root; + root.messageId = node.summary.firstMessageId; + root.threadId = node.summary.threadId; + root.subject = node.summary.subject; + root.date = node.summary.date; + root.tags = node.summary.tags; + return root; + } + + for (const MessageNode &child : node.children) { + if (child.messageId == messageId) + return child; + } + } + return {}; +} + +ActionScope ThreadListModel::messageScopeFor( + const QModelIndexList &selection) const +{ + ActionScope scope; + + for (const QModelIndex &index : selection) { + QString messageId; + if (isMessageRow(index)) { + messageId = messageAt(index).messageId; + } else { + if (index.row() < 0 || index.row() >= m_threads.size()) + continue; + // The message the CARD displays, which the query already named. + // Not the loaded children: a thread the user never expanded still + // shows its first message, and this must work without one. + messageId = m_threads.at(index.row()).summary.firstMessageId; + } + + // Skipped rather than widened. Falling back to the thread here would + // silently act on messages the row does not display, which is the + // behaviour item 108 removed. + if (messageId.isEmpty() || scope.messageIds.contains(messageId)) + continue; + + scope.messageIds.append(messageId); + scope.messageCount += 1; + } + + return scope; +} + ActionScope ThreadListModel::scopeFor(const QModelIndexList &selection) const { ActionScope scope; @@ -847,6 +1069,19 @@ ThreadSummary ThreadListModel::threadAt(int row) const return m_threads.at(row).summary; } +ThreadSummary ThreadListModel::threadFor(const QModelIndex &index) const +{ + if (!index.isValid()) + return {}; + + // The parent's row for a message, its own for a thread. Both are top-level + // numbers by the time threadAt() sees them, which is the whole point: the + // conversion happens once, here, instead of at every call site that has to + // remember which kind of row it is holding. + const QModelIndex threadIndex = isMessageRow(index) ? index.parent() : index; + return threadAt(threadIndex.row()); +} + QStringList ThreadListModel::accountKeysForThread(const QString &threadId) const { QStringList keys; @@ -884,7 +1119,117 @@ void ThreadListModel::applyTagChange(const QString &threadId, // The whole card repaints: unread state drives its font, and the tags // it draws on line 3 have just changed. - emit dataChanged(index(row, 0), index(row, 0)); + const QModelIndex threadIndex = index(row, 0); + emit dataChanged(threadIndex, threadIndex); + + // And every LOADED reply, because a thread-scoped write reaches every + // message in the thread. Updating only the summary left an expanded + // thread showing replies that still carried the old tags: marking a + // thread read repainted the card and left its replies bold and + // undimmed, describing a state the database no longer held. They + // corrected themselves on the next query, which is what made it look + // like a repaint bug rather than a stale model. + // + // Only the loaded ones exist to update. An unexpanded thread has no + // child rows, and the replies it does not hold are the database's + // business, not this model's. + QVector<MessageNode> &children = m_threads[row].children; + if (children.isEmpty()) + return; + + for (MessageNode &child : children) { + for (const QString &tag : removed) + child.tags.removeAll(tag); + for (const QString &tag : added) { + if (!child.tags.contains(tag)) + child.tags.append(tag); + } + } + + // One span for the whole expansion rather than a signal per reply: the + // rows are contiguous under this parent and a view coalesces them + // anyway. + emit dataChanged(index(0, 0, threadIndex), + index(children.size() - 1, 0, threadIndex)); + return; + } +} + +void ThreadListModel::applyMessageTagChange(const QString &messageId, + const QStringList &added, + const QStringList &removed) +{ + if (messageId.isEmpty()) return; + + const auto retag = [&](QStringList &tags) { + for (const QString &tag : removed) + tags.removeAll(tag); + for (const QString &tag : added) { + if (!tags.contains(tag)) + tags.append(tag); + } + }; + + for (int row = 0; row < m_threads.size(); ++row) { + ThreadNode &node = m_threads[row]; + const QModelIndex threadIndex = index(row, 0, QModelIndex()); + + // The ROOT card's own message, which is not among the children: + // setThreadMessages drops depth 0 because the root row stands for it. + // Searching only the children meant a write to the message a root card + // displays found nothing and repainted nothing, and item 108 made that + // the ordinary gesture rather than an edge case. + // + // Matched on the summary's id as well as the loaded node's, because the + // node is empty until the thread has been expanded and the user acts on + // unexpanded threads constantly. + const bool isRoot = + node.summary.firstMessageId == messageId + || (!node.first.messageId.isEmpty() + && node.first.messageId == messageId); + if (isRoot) { + // The root's own node, which is what the card draws its tags from + // once the message has been loaded. Seeded from the summary when + // the message has never been loaded, so an edit made before the + // row was ever opened still has somewhere to land; the summary is + // the union, which is the widest honest starting point. + if (node.first.messageId.isEmpty()) { + node.first.messageId = node.summary.firstMessageId; + node.first.threadId = node.summary.threadId; + node.first.tags = node.summary.tags; + } + retag(node.first.tags); + + // The SUMMARY only for a single-message thread. A thread's tags are + // a UNION over its messages: for a thread of one that union IS this + // message, so keeping the two in step is exact; for a longer + // thread, deleting one message does not delete the conversation, + // and the summary must keep describing the conversation because + // that is what the thread-scoped actions and the query read. + // + // The CARD does not depend on this either way: since item 110 it + // draws its tags from first.tags, which was just updated. This + // keeps the summary honest for everything else that reads it. + if (node.summary.totalCount <= 1) + retag(node.summary.tags); + + emit dataChanged(threadIndex, threadIndex); + return; + } + + QVector<MessageNode> &children = node.children; + for (int child = 0; child < children.size(); ++child) { + if (children.at(child).messageId != messageId) + continue; + + retag(children[child].tags); + + // The reply's own row, and only that row. Its chips, its marks, + // its dimming and its doomed fill all read the node's tags. + const QModelIndex replyIndex = index(child, 0, threadIndex); + emit dataChanged(replyIndex, replyIndex); + return; + } } } diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index 2b8d2b0..717537c 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -65,6 +65,21 @@ public: /// config itself would be a second source of truth. PillColoursRole, + /// How many of PillTagsRole's entries belong to the message the card + /// DISPLAYS, the rest belonging only to its siblings. + /// + /// The card stands for one message but sits above a conversation, so + /// it shows both: the message's own tags first at full size, then the + /// thread's other tags smaller and muted. Without the split a card + /// either claimed a sibling's tag as its own (item 110) or dropped it + /// and looked like it had lost information. + /// + /// Equals the whole list until the row has been opened, since the + /// per-message tags arrive with the message load and before that the + /// union is the only answer there is. Chips therefore SHRINK when the + /// split becomes known; none ever disappears. + PillOwnCountRole, + /// True when the row is a MESSAGE row rather than a thread root. /// Drives both the action scope and whether the view paints a tag /// strip under the row. @@ -225,8 +240,29 @@ public: /// contradict the sort the user selected. void reconcile(const QVector<ThreadSummary> &threads); + /// The thread at a TOP-LEVEL row. + /// + /// **Wrong for any index that might be a reply**, and that is item 88. A + /// tree 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, and the caller acts on unrelated mail while every id it + /// compares looks right. Safe only for a row number that came from a loop + /// over rowCount(), never from an index the user selected. + /// + /// Prefer threadFor(index), which cannot be handed the wrong number. ThreadSummary threadAt(int row) const; + /// The thread an index belongs to, whichever kind of row it is. + /// + /// A thread row resolves to itself; a message row resolves through its + /// PARENT rather than through its own row number. This is the accessor + /// every caller holding a QModelIndex wants, and it exists because the + /// row-taking one above silently answers about unrelated mail for a reply. + /// + /// An invalid or unknown index gives a default-constructed summary, whose + /// empty threadId every caller here already treats as "nothing to do". + ThreadSummary threadFor(const QModelIndex &index) const; + /// Fills in a thread's message rows once the worker has walked its tree. /// /// The depth-0 message is dropped: it is the thread's first message and the @@ -249,14 +285,58 @@ public: /// message the user could select is always findable here. QString threadIdForMessage(const QString &messageId) const; - /// Resolves a selection into what an action should touch. + /// Records the tags a MESSAGE really carries, as the worker reported them. + /// + /// Exists because `ThreadSummary::tags` is notmuch's UNION over the + /// thread, which is right for a card standing for a conversation and wrong + /// for one standing for a message. A four-message thread whose third + /// message is signed makes the whole thread read as signed, so the root + /// card and the message pane both claimed a tag the displayed message did + /// not have. + /// + /// Only the ROOT needs this: reply rows already carry their own nodes from + /// setThreadMessages. Calling it for anything else is a no-op. + /// + /// The thread's summary is deliberately NOT rewritten. It describes the + /// conversation, and three unread siblings do not stop being unread + /// because this message was read. + void setRootMessageTags(const QString &messageId, const QStringList &tags); + + /// A loaded message row's node, found by id rather than by position. /// - /// Mixed selections are honoured as given: a thread root and an unrelated - /// reply act on that whole thread and that one message. Nothing is - /// escalated or narrowed silently, which is the point of the scope being - /// visible in the first place. + /// For callers that know WHICH message they mean and must not depend on it + /// being the row the user has selected. Default-constructed when no + /// expanded thread holds it. + MessageNode messageById(const QString &messageId) const; + + /// Resolves a selection into whole THREADS, for the thread-scoped actions. + /// + /// A thread row contributes its thread; a message row still contributes + /// only itself, since a reply's own row cannot be widened into its + /// conversation without escalating silently. Mixed selections are honoured + /// as given: a thread root and an unrelated reply act on that whole thread + /// and that one message. + /// + /// **Not the default any more.** Since item 108 the ordinary actions use + /// messageScopeFor(); this is what the explicit "whole thread" submenu + /// resolves through. ActionScope scopeFor(const QModelIndexList &selection) const; + /// Resolves a selection into individual MESSAGES, which is what the + /// ordinary tag actions act on since item 108. + /// + /// A thread row contributes the ONE message its card displays, not its + /// whole conversation. That is `ThreadSummary::firstMessageId`, carried + /// from the query, so this needs no expansion and no worker round trip. + /// In the Sent view that field is the first MATCHED message rather than + /// the thread's opening one, which is right here for the same reason it is + /// right on the card: both answer "the message this row shows". + /// + /// A thread row whose `firstMessageId` is empty contributes nothing. That + /// is a row the model cannot name a message for, and acting on the whole + /// thread instead would be the silent escalation this exists to remove. + ActionScope messageScopeFor(const QModelIndexList &selection) const; + /// The account keys behind a thread's account tags, for item 49's /// per-account sync. /// @@ -273,6 +353,21 @@ public: void applyTagChange(const QString &threadId, const QStringList &added, const QStringList &removed); + /// The same, for a change scoped to ONE message. + /// + /// Repaints that message's own row and leaves the thread alone. The thread + /// row deliberately does not follow: it stands for the whole conversation, + /// so redrawing it for a one-message edit would claim every message in it + /// had changed. That reasoning is why no optimistic update existed here at + /// all, which left Delete and Toggle unread on a reply moving the pending + /// count and changing nothing on screen. + /// + /// A message id that no expanded thread holds is a no-op: only expanded + /// threads have message rows, so there is nothing to repaint. + void applyMessageTagChange(const QString &messageId, + const QStringList &added, + const QStringList &removed); + private: /// One thread root and the message rows expanded under it. /// @@ -301,6 +396,21 @@ private: bool loaded = false; }; + /// A newly arrived thread, with its card's own message seeded from the + /// query. + /// + /// `ThreadSummary::tags` is notmuch's union over the conversation, and the + /// card stands for ONE message. The worker reads that message's own tags + /// in the same walk that finds its id, so the two tiers are known from the + /// first paint (item 111). Deriving them from the message LOAD instead + /// left every unopened row drawing one tier and correcting itself when the + /// user selected it, which is most of the list. + /// + /// `first` is left empty when the query carried no per-message tags, so + /// anything that supplies only a summary keeps the old behaviour rather + /// than claiming the union as one message's. + static ThreadNode nodeFor(const ThreadSummary &summary); + QVector<ThreadNode> m_threads; const TagColors *m_tagColors = nullptr; QString m_dateFormat; diff --git a/src/types.h b/src/types.h index f4eaeba..409ce79 100644 --- a/src/types.h +++ b/src/types.h @@ -48,6 +48,20 @@ struct ThreadSummary /// with recipients; the two have nothing in common but their position here. QString firstMessageId; + /// The tags of that ONE message, as opposed to `tags` above, which is + /// notmuch's union over the whole thread. + /// + /// A card stands for one message but sits above a conversation, and shows + /// both: its own tags at full size, the thread's others smaller (item + /// 111). Without this the split is unknown until the row is opened and the + /// message loads, so every chip renders as the card's own and then shrinks + /// on selection, which is what the user reported. + /// + /// Free, for the same reason `firstMessageId` is: the walk that finds that + /// message is already happening and this reads the INDEX, not the message + /// file. Do not move it behind a flag by analogy with `recipients`. + QStringList firstMessageTags; + /// Who the thread's messages were sent TO, summarised for one line. /// /// Empty unless the query asked for it, and that is a performance @@ -140,6 +154,15 @@ struct MessageNode { return tags.contains(QStringLiteral("attachment")); } + + bool isDeleted() const { return tags.contains(QStringLiteral("deleted")); } + bool isSpam() const { return tags.contains(QStringLiteral("spam")); } + + /// True while the message is tagged for removal, exactly as the thread + /// predicate of the same name. A reply carries its own fate: a + /// message-scoped Delete tags one message, and the reply's row is the only + /// place the user can see that happen. + bool isDoomed() const { return isDeleted() || isSpam(); } }; /// What an action is about to touch, resolved from the selection. diff --git a/tests/test_carddelegate.cpp b/tests/test_carddelegate.cpp index d5e55b5..a14671d 100644 --- a/tests/test_carddelegate.cpp +++ b/tests/test_carddelegate.cpp @@ -18,6 +18,9 @@ #include "carddelegate.h" +#include "cardlayout.h" +#include "tagchip.h" +#include "tagcolors.h" #include "threadlistmodel.h" #include <QTest> @@ -30,6 +33,9 @@ private slots: void theAccentLiftsAMutedAccountColour(); void theAccentKeepsEachAccountTellableApart(); void anAccountWithNoColourFallsBackToTheNeutralLine(); + void aSiblingChipIsMutedButStaysLegibleAndRecognisable(); + void aSiblingChipFontIsSmallerThanItsOwnTier(); + void aSiblingChipsPaddingShrinksWithItsFont(); }; namespace { @@ -131,5 +137,127 @@ void TestCardDelegate::anAccountWithNoColourFallsBackToTheNeutralLine() ThreadListModel::threadLineColour()); } +void TestCardDelegate::aSiblingChipIsMutedButStaysLegibleAndRecognisable() +{ + // Item 111: a card shows its own tags at full size and the rest of the + // conversation's smaller and muted. "Muted" has two hard requirements that + // a look at the screen will not catch, so they are asserted here. + for (const QColor &colour : sampleAccounts()) { + const QColor muted = CardDelegate::mutedChipColour(colour); + + // Actually muted, or the tier is not distinguishable at all. + QVERIFY2(saturationOf(muted) < saturationOf(colour), + qPrintable(QStringLiteral("%1 was not drained at all") + .arg(colour.name()))); + + // Same HUE. A sibling's `signed` has to stay recognisably the same + // colour as a full-size `signed` elsewhere in the list, or the muting + // reads as a different tag rather than a quieter one. + float h1 = 0, h2 = 0, s = 0, l = 0, a = 0; + colour.getHslF(&h1, &s, &l, &a); + muted.getHslF(&h2, &s, &l, &a); + QVERIFY2(qAbs(h1 - h2) < 0.001f, + qPrintable(QStringLiteral("%1 changed hue when muted") + .arg(colour.name()))); + + // Same LIGHTNESS, which is what keeps the text legible: TagColors + // picks the text colour from the fill, and a fill that drifted toward + // black or white could flip that choice or land mid-grey where neither + // works. Blending toward the background would do exactly that, which + // is the mistake accentLineColour() records. + QCOMPARE(lightnessOf(muted), lightnessOf(colour)); + QCOMPARE(TagColors::textColourOn(muted), + TagColors::textColourOn(colour)); + } + + // An invalid colour stays invalid rather than becoming a real one. + QVERIFY(!CardDelegate::mutedChipColour(QColor()).isValid()); +} + +void TestCardDelegate::aSiblingChipFontIsSmallerThanItsOwnTier() +{ + // Size is what says whose tag a chip is, so the two tiers must differ, and + // by enough to SEE. The first version subtracted a point from smallFont(), + // and the user reported the tiers as indistinguishable: on their 14pt + // desktop that gave 13 and 12, a 7% step. + // + // The step is now a fraction of the card font, so it does not shrink as + // the desktop's font grows. Asserted as a ratio rather than as a size, to + // keep this about the DISTINCTION rather than about the constant. + QFont card; + card.setPointSizeF(14.0); // The user's own desktop size. + const qreal own = CardLayout::smallFont(card).pointSizeF(); + const qreal sibling = CardLayout::siblingFont(card).pointSizeF(); + + QVERIFY(sibling < own); + QVERIFY2(sibling < own * 0.85, + qPrintable(QStringLiteral("sibling %1pt against own %2pt is under " + "a 15%% step, which reads as the same " + "size") + .arg(sibling) + .arg(own))); + + // Proportional, not a fixed subtraction: the step must survive a larger + // desktop font rather than becoming proportionally smaller. + QFont big; + big.setPointSizeF(28.0); + QVERIFY(CardLayout::siblingFont(big).pointSizeF() + < CardLayout::smallFont(big).pointSizeF() * 0.85); + + // The pixel branch too: qt6ct sets fonts in PIXELS, and pointSizeF() is -1 + // for those, so a point-only implementation silently returns the original + // size and both tiers render identically. CLAUDE.md records this trap. + QFont pixels; + pixels.setPixelSize(14); + QVERIFY(pixels.pointSizeF() < 0); + QVERIFY2(CardLayout::siblingFont(pixels).pixelSize() + < CardLayout::smallFont(pixels).pixelSize(), + "a pixel-sized desktop font gives both tiers the same size, so " + "the distinction disappears entirely"); + + // Floored rather than shrinking without limit. + QFont tiny; + tiny.setPointSizeF(6.0); + QVERIFY(CardLayout::siblingFont(tiny).pointSizeF() >= 6.0); +} + +void TestCardDelegate::aSiblingChipsPaddingShrinksWithItsFont() +{ + // Half of "smaller" is the padding, and leaving it fixed is why the first + // version still looked the same size. kPaddingX is 9 a side: on a sibling + // chip that is 18px of padding around roughly 30px of text, so the chip + // stayed wide while its letters shrank, which reads as "same chip, smaller + // text" rather than as a smaller chip. + QFont card; + card.setPointSizeF(14.0); + const QFontMetrics ownMetrics(CardLayout::smallFont(card)); + const QFontMetrics siblingMetrics(CardLayout::siblingFont(card)); + + const QString tag = QStringLiteral("signed"); + // Through CardDelegate::chipSize(), which is what the paint loop calls. + // Calling TagChip::sizeFor() directly here proved what THAT function does + // and nothing about whether the delegate asks it for a scaled padding: a + // mutation dropping the scale at the call site survived that version of + // this test. + const QSize own = CardDelegate::chipSize(ownMetrics, tag, true); + const QSize scaled = CardDelegate::chipSize(siblingMetrics, tag, false); + const QSize unscaled = TagChip::sizeFor(siblingMetrics, tag); + + // The font alone is not enough: scaling the padding as well takes off + // measurably more width. + QVERIFY2(scaled.width() < unscaled.width(), + "the padding did not scale, so the chip keeps full-size margins " + "around smaller letters"); + QVERIFY(scaled.width() < own.width()); + QVERIFY(scaled.height() < own.height()); + + // Floored rather than collapsing to nothing: the corner radius is half the + // height, so a chip with no horizontal padding has its text on the curve. + const QSize tiny = TagChip::sizeFor(siblingMetrics, tag, 0.0); + QVERIFY2(tiny.width() > siblingMetrics.horizontalAdvance(tag), + "a zero scale left no horizontal padding at all, so the text sits " + "on the chip's rounded end"); +} + QTEST_MAIN(TestCardDelegate) #include "test_carddelegate.moc" diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index f4357c6..07cc56b 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -57,6 +57,7 @@ #include <QComboBox> #include <QScrollBar> #include "tagchip.h" +#include "tagstrip.h" #include "threadlistmodel.h" #include "threadlistview.h" #include "notmuchfixture.h" @@ -273,6 +274,23 @@ private slots: void escapeBlanksTheMessagePane(); void deleteTogglesOnAnAlreadyDeletedThread(); void deleteOnAMixedSelectionDeletesRatherThanSplittingIt(); + void deleteOnAReplyReadsItsOwnThreadNotTheFirstInTheList(); + void toggleUnreadOnAReplyReadsItsOwnThreadNotTheFirstInTheList(); + void editTagsOnAReplyCountsItsOwnThreadNotTheFirstInTheList(); + void markCurrentThreadReadResolvesTheThreadThroughTheIndex(); + void deletingAReplyRepaintsThatReplyRow(); + void toggleUnreadOnAReplyReadsTheReplysOwnState(); + void toggleUnreadOnAReplyRepaintsItInBothDirections(); + void taggingTheOpenReplyUpdatesTheMessagePaneStrip(); + void taggingAnUnrelatedReplyLeavesTheStripAlone(); + void aHeldMessageEditIsSentWhenTheSyncEnds(); + void anActionOnAThreadRowActsOnTheMessageItDisplays(); + void theThreadSubmenuIsReachableFromBothMenus(); + void autoMarkReadTouchesOnlyTheMessageOnDisplay(); + void autoMarkReadArmsForAReplyToo(); + void taggingTheOpenRootMessageKeepsTheStripPopulated(); + void aLoadedMessageCorrectsTheStripFromTheThreadsUnion(); + void aLoadedRootMessageGivesTheCardItsOwnTags(); void aTransientStatusMessageExpires(); void theSelectionCountIsStateAndDoesNotExpire(); void anEditUndoneNettsBackToZero(); @@ -790,6 +808,13 @@ static ThreadSummary makeThread(const QString &id, const QStringList &tags) thread.subject = QStringLiteral("Subject ") + id; thread.authors = QStringLiteral("Someone <someone@example.org>"); thread.tags = tags; + + // The message the row displays, which the real worker fills in from the + // query. Required since item 108: an ordinary tag action resolves a thread + // row to THIS id, so a summary without one names no message and every + // action on it does nothing. A fixture missing it fails with "the action + // did not happen", which reads as a defect in the action. + thread.firstMessageId = id + QStringLiteral("-first@example.org"); return thread; } @@ -1372,8 +1397,11 @@ void TestMainWindow::anActionOnAThreadRowSaysItHitTheWholeThread() selectThreadRow(view, 0); QApplication::processEvents(); - auto *archive = window.findChild<QAction *>(QStringLiteral("archive")); - QVERIFY2(archive, "no archive action to trigger"); + // The THREAD action since item 108. The plain `archive` now acts on the + // one message a card displays, and would rightly not claim to have taken + // the whole thread; this suffix belongs to the action that really does. + auto *archive = window.findChild<QAction *>(QStringLiteral("archive_thread")); + QVERIFY2(archive, "no archive_thread action to trigger"); archive->trigger(); // Read BEFORE processEvents, deliberately. This binary has no worker @@ -4327,7 +4355,13 @@ void TestMainWindow::aHeldEditIsSentBeforeTheSyncEndRefreshReadsTheDatabase() // The write went out. Nothing else in this handler sends one, so its // presence is what proves the flush ran, and the refresh below is what it // has to have run BEFORE. - QVERIFY2(!window.pendingThreadIdsForTesting().isEmpty(), + // + // Either scope counts. The gesture is Toggle unread on a thread row, which + // is message-scoped since item 108, so the ids land in the message list; + // the ORDER this test exists for is the same either way, and pinning the + // scope here would make it fail for a reason it does not care about. + QVERIFY2(!window.pendingThreadIdsForTesting().isEmpty() + || !window.pendingMessageIdsForTesting().isEmpty(), "the held edit was dropped rather than sent"); const quint64 after = window.currentGenerationForTesting(); @@ -4593,7 +4627,10 @@ void TestMainWindow::deleteTogglesOnAnAlreadyDeletedThread() QVERIFY(model); auto *view = window.findChild<QTreeView *>(); QVERIFY(view); - auto *action = window.findChild<QAction *>(QStringLiteral("delete")); + // The THREAD action, since this is about a THREAD's state. Item 108 made + // the plain `delete` act on the one message a card displays, and a thread + // summary carrying `deleted` says nothing about that message's own tags. + auto *action = window.findChild<QAction *>(QStringLiteral("delete_thread")); QVERIFY(action); model->appendBatch({ makeThread(QStringLiteral("t1"), @@ -4621,7 +4658,7 @@ void TestMainWindow::deleteOnAMixedSelectionDeletesRatherThanSplittingIt() QVERIFY(model); auto *view = window.findChild<QTreeView *>(); QVERIFY(view); - auto *action = window.findChild<QAction *>(QStringLiteral("delete")); + auto *action = window.findChild<QAction *>(QStringLiteral("delete_thread")); QVERIFY(action); model->appendBatch({ makeThread(QStringLiteral("t1"), @@ -4637,6 +4674,889 @@ void TestMainWindow::deleteOnAMixedSelectionDeletesRatherThanSplittingIt() "a mixed selection split instead of deleting the whole selection"); } +/// Builds a window whose SECOND thread is expanded and carries one reply, with +/// the two threads deliberately in opposite states. +/// +/// Item 88's shape in one place. A tree numbers rows per parent, so the first +/// reply of any thread has row() == 0 and threadAt(0) answers about the FIRST +/// THREAD IN THE LIST. Every test below selects that reply and asserts on +/// behaviour that can only be right if the thread was resolved through the +/// index: with the row number, each one reads t1's state while acting on t2. +/// +/// The opposite states are what makes the tests able to fail. Two threads in +/// the same state give the same answer either way, which is how the reverted +/// item 87 fix passed while corrupting mail. +/// +/// \p replyTags defaults to the thread's own tags, which is the usual case. +/// Pass it explicitly to make a reply DISAGREE with its thread, which is what +/// separates "reads the right thread" from "reads the right message": a reply +/// can be unread inside a thread that is not, and vice versa. +static QModelIndex expandSecondThreadAndSelectItsReply( + QTreeView *view, ThreadListModel *model, const QStringList &firstTags, + const QStringList &secondTags, + const std::optional<QStringList> &replyTags = std::nullopt) +{ + ThreadSummary first = makeThread(QStringLiteral("t1"), firstTags); + ThreadSummary second = makeThread(QStringLiteral("t2"), secondTags); + second.totalCount = 2; + model->appendBatch({ first, second }); + + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("t2"); + root.tags = secondTags; + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t2"); + reply.tags = replyTags.value_or(secondTags); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t2"), { root, reply }); + + const QModelIndex threadRow = model->index(1, 0, QModelIndex()); + view->expand(threadRow); + + const QModelIndex replyRow = model->index(0, 0, threadRow); + if (!replyRow.isValid() || !model->isMessageRow(replyRow)) + return {}; + + // Row 0 under its parent, which is the trap: the number is a plausible + // top-level row and threadAt() cannot tell the difference. + if (replyRow.row() != 0) + return {}; + + view->selectionModel()->select( + replyRow, QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(replyRow); + QApplication::processEvents(); + return replyRow; +} + +void TestMainWindow::deleteOnAReplyReadsItsOwnThreadNotTheFirstInTheList() +{ + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *action = window.findChild<QAction *>(QStringLiteral("delete")); + QVERIFY(action); + + // t1 deleted, t2 not. Reading t1's state for a reply of t2 makes the + // toggle choose UNDELETE for a thread that was never deleted. + const QModelIndex reply = expandSecondThreadAndSelectItsReply( + view, model, { QStringLiteral("deleted") }, {}); + QVERIFY2(reply.isValid(), + "the fixture did not produce a reply row at row 0, so this test " + "would assert nothing about item 88's trap"); + + action->trigger(); + + // Delete, because the message's own thread is not deleted. The write goes + // through scopeFor() and lands on the message either way; what is under + // test is the DIRECTION, which is chosen from the state that was read. + QVERIFY2(window.pendingMessageIdsForTesting().contains( + QStringLiteral("m1@example.org")), + "Delete on a reply did not act on that reply"); + QCOMPARE(window.undoDepthForTesting(), 1); + QVERIFY2(window.undoTextForTesting().contains(QStringLiteral("Delete")), + qPrintable(QStringLiteral( + "Delete on a reply of an undeleted thread chose " + "the wrong direction: %1. It read the FIRST " + "thread's state, which is deleted.") + .arg(window.undoTextForTesting()))); +} + +void TestMainWindow::toggleUnreadOnAReplyReadsItsOwnThreadNotTheFirstInTheList() +{ + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *action = window.findChild<QAction *>(QStringLiteral("toggle_unread")); + QVERIFY(action); + + // t1 unread, t2 read. Reading t1's state marks an already-read message + // read again, which is a no-op write the user sees as a dead key. + const QModelIndex reply = expandSecondThreadAndSelectItsReply( + view, model, { QStringLiteral("unread") }, {}); + QVERIFY2(reply.isValid(), "the fixture did not produce a reply row at row 0"); + + action->trigger(); + + QCOMPARE(window.undoDepthForTesting(), 1); + QVERIFY2(window.undoTextForTesting().contains(QStringLiteral("Mark unread")), + qPrintable(QStringLiteral( + "Toggle unread on a reply of a READ thread chose " + "the wrong direction: %1. It read the FIRST " + "thread's state, which is unread.") + .arg(window.undoTextForTesting()))); +} + +void TestMainWindow::editTagsOnAReplyCountsItsOwnThreadNotTheFirstInTheList() +{ + // The tag dialog is modal, so what is tested is the count it is BUILT + // from. Those counts drive its tri-state checkboxes, so a wrong count + // offers to remove a tag the message does not carry and shows the ones it + // does as unset. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + const QModelIndex reply = expandSecondThreadAndSelectItsReply( + view, model, { QStringLiteral("t1only") }, { QStringLiteral("t2only") }); + QVERIFY2(reply.isValid(), "the fixture did not produce a reply row at row 0"); + + const QHash<QString, int> counts = window.selectionTagCountsForTesting(); + + QVERIFY2(counts.contains(QStringLiteral("t2only")), + "the tag dialog would not offer the tag the selected reply " + "actually carries"); + QVERIFY2(!counts.contains(QStringLiteral("t1only")), + "the tag dialog counted the FIRST thread's tags for a reply of " + "the second, so it would offer to remove a tag that is not there"); +} + +void TestMainWindow::markCurrentThreadReadResolvesTheThreadThroughTheIndex() +{ + // Item 87 is blocked on this and will scope the write to one message. Today + // an unrelated guard hides the defect: onThreadSelected clears + // m_currentThreadId for a message row, so markCurrentThreadRead returns + // before it can read the wrong thread. That guard is not the protection + // this needs, and item 87 does not remove it, so the assertion here is on + // the resolution itself rather than on a write that cannot currently + // happen. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + const QModelIndex reply = expandSecondThreadAndSelectItsReply( + view, model, { QStringLiteral("unread") }, { QStringLiteral("unread") }); + QVERIFY2(reply.isValid(), "the fixture did not produce a reply row at row 0"); + + // The question the timer's handler asks, in isolation: which thread is the + // current row part of. With the row number this answers "t1" for a reply + // of t2. + QCOMPARE(window.threadForCurrentRowForTesting().threadId, + QStringLiteral("t2")); +} + +void TestMainWindow::deletingAReplyRepaintsThatReplyRow() +{ + // The user's report, at the gesture level: "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." The model-level test proves + // applyMessageTagChange works; this proves the action reaches it, which is + // the half that was actually missing. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *action = window.findChild<QAction *>(QStringLiteral("delete")); + QVERIFY(action); + + const QModelIndex reply = + expandSecondThreadAndSelectItsReply(view, model, {}, {}); + QVERIFY2(reply.isValid(), "the fixture did not produce a reply row at row 0"); + + // Nothing to see before the gesture, so the assertion after it means + // something. + QVERIFY(!model->messageAt(reply).isDeleted()); + const QVariant before = model->data(reply, Qt::BackgroundRole); + + QSignalSpy spy(model, &QAbstractItemModel::dataChanged); + action->trigger(); + + QVERIFY2(model->messageAt(reply).isDeleted(), + "Delete on a reply left the reply's own row unchanged, so the " + "pending count moved and the user saw nothing"); + QVERIFY2(spy.count() >= 1, "no repaint was requested for the reply's row"); + QVERIFY2(model->data(reply, Qt::BackgroundRole) != before, + "the deleted reply paints exactly as it did before"); + + // The THREAD row must not follow: it stands for the whole conversation, + // and one deleted reply does not doom it. + const QModelIndex threadRow = reply.parent(); + QVERIFY2(!model->threadFor(threadRow).isDeleted(), + "deleting one reply marked its whole thread deleted"); +} + +void TestMainWindow::toggleUnreadOnAReplyReadsTheReplysOwnState() +{ + // The user's report: "read/unread still doesn't trigger a repaint of the + // reply". The write was already message-scoped and the model already + // repaints a message row, so neither was the fault. The DIRECTION was: + // the action read threadFor(current).isUnread(), the THREAD's state, even + // when the selected row is a reply. + // + // The consequence is a dead key rather than a wrong write. On a read + // thread the answer is always "add unread", so pressing it on an + // already-unread reply re-adds a tag it has, which is a no-op the model + // correctly declines to repaint. Item 88 fixed WHICH thread this reads; + // this is about reading a message at all. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *action = window.findChild<QAction *>(QStringLiteral("toggle_unread")); + QVERIFY(action); + + // A thread that is READ carrying a reply that is UNREAD. That disagreement + // is the whole test: with the thread's state the answer is "mark unread", + // with the message's it is "mark read". + const QModelIndex reply = expandSecondThreadAndSelectItsReply( + view, model, {}, {}, QStringList{ QStringLiteral("unread") }); + QVERIFY2(reply.isValid(), "the fixture did not produce a reply row at row 0"); + QVERIFY(model->messageAt(reply).isUnread()); + QVERIFY2(!model->threadFor(reply).isUnread(), + "the fixture's thread is unread too, so this test cannot tell the " + "two sources apart"); + + action->trigger(); + + QVERIFY2(!model->messageAt(reply).isUnread(), + "Toggle unread on an unread reply did not mark it read: the " + "direction came from the THREAD, which is already read, so it " + "re-added a tag the reply already had and nothing changed"); + QVERIFY2(window.undoTextForTesting().contains(QStringLiteral("Mark read")), + qPrintable(QStringLiteral("wrong direction: %1") + .arg(window.undoTextForTesting()))); +} + +void TestMainWindow::toggleUnreadOnAReplyRepaintsItInBothDirections() +{ + // Visible BOTH ways. The user reached the repaint only by deleting and + // undoing, which is a different write forcing the row to redraw; the + // unread change itself has to do it on its own, in each direction. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *action = window.findChild<QAction *>(QStringLiteral("toggle_unread")); + QVERIFY(action); + + const QModelIndex reply = expandSecondThreadAndSelectItsReply( + view, model, {}, {}, QStringList{ QStringLiteral("unread") }); + QVERIFY(reply.isValid()); + + const QVariant unreadForeground = model->data(reply, Qt::ForegroundRole); + const QVariant unreadFont = model->data(reply, Qt::FontRole); + + QSignalSpy spy(model, &QAbstractItemModel::dataChanged); + action->trigger(); + + QVERIFY2(spy.count() >= 1, "marking a reply read requested no repaint"); + QVERIFY2(model->data(reply, Qt::ForegroundRole) != unreadForeground, + "a reply marked read paints exactly as it did while unread"); + QVERIFY2(model->data(reply, Qt::FontRole) != unreadFont, + "a reply marked read keeps the unread font"); + + // And back. A toggle that is only visible one way is half a toggle. + spy.clear(); + action->trigger(); + QVERIFY(model->messageAt(reply).isUnread()); + QVERIFY2(spy.count() >= 1, "marking a reply unread again requested no repaint"); + QCOMPARE(model->data(reply, Qt::ForegroundRole), unreadForeground); + QCOMPARE(model->data(reply, Qt::FontRole), unreadFont); +} + +void TestMainWindow::taggingTheOpenReplyUpdatesTheMessagePaneStrip() +{ + // The user's report: "the right pane chips are not [repainted], for it to + // sync I have to change message and go back to the edited one". + // + // sendThreadTagChange refreshes the strip when the edited thread is the + // open one. sendMessageTagChange had no equivalent, so a message-scoped + // write updated the list row and left the pane's chips describing the + // message as it was before the edit. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *strip = window.findChild<TagStrip *>(); + QVERIFY2(strip, "no tag strip in the message pane"); + + // visible + hidden: TagStrip collapses what does not fit into a "+N" chip, + // and an unshown window has no width, so visibleTags() alone measures the + // layout rather than the data. + const auto stripTags = [strip]() { + return strip->visibleTags() + strip->hiddenTags(); + }; + auto *action = window.findChild<QAction *>(QStringLiteral("delete")); + QVERIFY(action); + + // A tag the strip will actually draw. Account tags are filtered out by the + // strip, and `unread` and `inbox` are hidden on the card but not here, so + // the fixture uses a plain functional tag to keep the assertion honest. + const QModelIndex reply = expandSecondThreadAndSelectItsReply( + view, model, {}, {}, QStringList{ QStringLiteral("todo") }); + QVERIFY2(reply.isValid(), "the fixture did not produce a reply row at row 0"); + + // Selecting the reply is what puts it in the pane, and the strip has to be + // showing the reply's own tags before the edit or this asserts nothing. + QVERIFY2(stripTags().contains(QStringLiteral("todo")), + "the strip does not show the selected reply's tags, so this test " + "cannot tell a missing refresh from a strip that never had them"); + QVERIFY(!stripTags().contains(QStringLiteral("deleted"))); + + action->trigger(); + + QVERIFY2(stripTags().contains(QStringLiteral("deleted")), + "the message pane's chips still describe the reply as it was " + "before the edit; the user has to select away and back to see it"); +} + +void TestMainWindow::taggingAnUnrelatedReplyLeavesTheStripAlone() +{ + // The guard, not the refresh. The strip describes the message ON DISPLAY, + // so a write to a different message must not repaint it with that + // message's tags. The thread path has the same guard, keyed on + // m_currentThreadId. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *strip = window.findChild<TagStrip *>(); + QVERIFY(strip); + + // visible + hidden: TagStrip collapses what does not fit into a "+N" chip, + // and an unshown window has no width, so visibleTags() alone measures the + // layout rather than the data. + const auto stripTags = [strip]() { + return strip->visibleTags() + strip->hiddenTags(); + }; + + ThreadSummary thread = makeThread(QStringLiteral("t1"), {}); + thread.totalCount = 3; + model->appendBatch({ thread }); + + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("t1"); + root.depth = 0; + MessageNode first; + first.messageId = QStringLiteral("m1@example.org"); + first.threadId = QStringLiteral("t1"); + first.tags = QStringList{ QStringLiteral("todo") }; + first.depth = 1; + MessageNode second; + second.messageId = QStringLiteral("m2@example.org"); + second.threadId = QStringLiteral("t1"); + second.tags = QStringList{ QStringLiteral("later") }; + second.depth = 1; + model->setThreadMessages(QStringLiteral("t1"), { root, first, second }); + + const QModelIndex threadRow = model->index(0, 0, QModelIndex()); + view->expand(threadRow); + + // The FIRST reply is the one on display. + const QModelIndex displayed = model->index(0, 0, threadRow); + view->selectionModel()->select( + displayed, + QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(displayed); + QApplication::processEvents(); + QVERIFY(stripTags().contains(QStringLiteral("todo"))); + + // A write to the OTHER reply, reaching the send path directly: driving it + // through the action would move the selection and change what is on + // display, which is the thing being held still. + window.sendMessageTagChangeForTesting({ QStringLiteral("m2@example.org") }, + { QStringLiteral("deleted") }, {}, + QStringLiteral("Delete")); + + QVERIFY2(!stripTags().contains(QStringLiteral("deleted")), + "the strip took on the tags of a message that is not the one in " + "the pane"); + QVERIFY2(stripTags().contains(QStringLiteral("todo")), + "the strip stopped describing the message on display"); +} + +void TestMainWindow::aHeldMessageEditIsSentWhenTheSyncEnds() +{ + // Found by reading while fixing the strip refresh, not reported. + // + // flushHeldEdits() looped over edit.threadIds and called + // sendThreadTagChange() only. A message-scoped edit held during a sync + // carries no thread ids at all, so the loop did nothing, the send + // early-returned on an empty list, and the edit was DROPPED: applied + // optimistically to the row, counted as unsynced, and never written. The + // user would have seen the change, been told it was pending, and lost it. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *action = window.findChild<QAction *>(QStringLiteral("delete")); + QVERIFY(action); + + const QModelIndex reply = + expandSecondThreadAndSelectItsReply(view, model, {}, {}); + QVERIFY(reply.isValid()); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Running)); + action->trigger(); + QVERIFY2(window.hasEditAwaitingSend(), + "a message edit made during a sync was not held"); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + + QVERIFY2(!window.hasEditAwaitingSend(), + "the sync ending did not send the held message edit, so it was " + "silently dropped: shown on the row, counted as pending, never " + "written"); + + // Sent for the MESSAGE, not escalated to its thread. Losing the scope on + // the way out of the hold would delete every message in the thread. + QVERIFY2(window.pendingMessageIdsForTesting().contains( + QStringLiteral("m1@example.org")), + "the held edit was not sent with its message scope"); + QVERIFY2(window.pendingThreadIdsForTesting().isEmpty(), + "a held MESSAGE edit was sent as a thread edit, which would tag " + "every message in the thread"); + + // And the row still shows it: the flush takes the optimistic update back + // before re-sending, so a bug there leaves the row wrong in the other + // direction. + QVERIFY2(model->messageAt(reply).isDeleted(), + "sending the held edit lost the tag from the reply's row"); +} + +void TestMainWindow::anActionOnAThreadRowActsOnTheMessageItDisplays() +{ + // Item 108, the whole point of it. A root card renders ONE message since + // item 66, so acting on it acts on that message; the conversation is + // reached through the explicit thread actions. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + ThreadSummary t = makeThread(QStringLiteral("t1"), {}); + t.totalCount = 7; + model->appendBatch({ t }); + selectThreadRow(view, 0); + + auto *deleteAction = window.findChild<QAction *>(QStringLiteral("delete")); + QVERIFY(deleteAction); + deleteAction->trigger(); + + QCOMPARE(window.pendingMessageIdsForTesting(), + QStringList{ QStringLiteral("t1-first@example.org") }); + QVERIFY2(window.pendingThreadIdsForTesting().isEmpty(), + "the ordinary Delete still acted on the whole thread, so it " + "touched six messages the card does not display"); + + // The thread action is how the conversation is reached, and it must still + // work from the same selection. + auto *deleteThread = + window.findChild<QAction *>(QStringLiteral("delete_thread")); + QVERIFY(deleteThread); + deleteThread->trigger(); + + QCOMPARE(window.pendingThreadIdsForTesting(), + QStringList{ QStringLiteral("t1") }); + + // Two commands, one per gesture, each recording the scope it used: a thread + // action that pushed the message command would undo a fraction of what it + // did. + QCOMPARE(window.undoDepthForTesting(), 2); +} + +void TestMainWindow::theThreadSubmenuIsReachableFromBothMenus() +{ + // The user asked for "a submenu when right clicking and the same submenu + // under Message in the top menu". Both, not one: the context menu is where + // the gesture starts and the menu bar is where a shortcut is discovered. + // + // A QMenu belongs to ONE menu tree, so these are two instances holding the + // same actions. Adding a single instance to both silently gives it to + // whichever added it last, which is the failure this pins. + const Config config; + MainWindow window(config); + + auto *context = + window.findChild<QMenu *>(QStringLiteral("threadContextMenu")); + QVERIFY(context); + + const QStringList expected = { + QStringLiteral("archive_thread"), + QStringLiteral("delete_thread"), + QStringLiteral("spam_thread"), + QStringLiteral("toggle_unread_thread"), + QStringLiteral("flag_thread"), + }; + + // Every submenu instance in the window, wherever it was added. + const QList<QMenu *> submenus = + window.findChildren<QMenu *>(QStringLiteral("threadActionsMenu")); + QVERIFY2(submenus.size() >= 2, + qPrintable(QStringLiteral("expected the thread submenu in both " + "the context menu and the menu bar, " + "found %1 instance(s)") + .arg(submenus.size()))); + + for (QMenu *menu : submenus) { + QStringList names; + for (QAction *action : menu->actions()) { + if (!action->isSeparator()) + names.append(action->objectName()); + } + QCOMPARE(names, expected); + } + + // One of them is the context menu's own, reached as a submenu rather than + // as a loose action. + bool inContextMenu = false; + for (QAction *action : context->actions()) { + if (action->menu() + && action->menu()->objectName() + == QStringLiteral("threadActionsMenu")) { + inContextMenu = true; + break; + } + } + QVERIFY2(inContextMenu, + "right-clicking a thread offers no whole-thread submenu"); +} + +void TestMainWindow::autoMarkReadTouchesOnlyTheMessageOnDisplay() +{ + // Item 87, reported 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." + // + // 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 it again by hand. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString path = dir.filePath(QStringLiteral("qtmaildir.conf")); + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write("[general]\nmark_read_delay_ms = 0\n"); + file.close(); + + Config config; + config.load(path); + QCOMPARE(config.markReadDelayMs(), 0); + + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *timer = window.findChild<QTimer *>(QStringLiteral("markReadTimer")); + QVERIFY(timer); + + ThreadSummary t = makeThread(QStringLiteral("t1"), + { QStringLiteral("unread") }); + t.totalCount = 7; + model->appendBatch({ t }); + + selectThreadRow(view, 0); + QApplication::processEvents(); + QVERIFY2(timer->isActive() || !window.pendingMessageIdsForTesting().isEmpty(), + "selecting an unread thread armed no mark-read at all"); + + // Fire it. A zero-interval timer still goes through the event loop. + QTRY_VERIFY_WITH_TIMEOUT(!timer->isActive(), 2000); + QApplication::processEvents(); + + // ONE message, the one the card renders, and named rather than merely + // counted: a thread of seven whose first message is the target is exactly + // the case where a count of one could still be the wrong one. + QCOMPARE(window.pendingMessageIdsForTesting(), + QStringList{ QStringLiteral("t1-first@example.org") }); + QVERIFY2(window.pendingThreadIdsForTesting().isEmpty(), + "the automatic mark-read still wrote to the whole thread, so six " + "messages the user never displayed were marked read and the next " + "sync carries that to the server"); + + // Still not on the undo stack. The user never took this action, so + // hijacking Ctrl+Z to reverse it would undo something they did not do. + QCOMPARE(window.undoDepthForTesting(), 0); +} + +void TestMainWindow::autoMarkReadArmsForAReplyToo() +{ + // Selecting a reply displays that message, so the same rule applies to it. + // Before item 87 the timer was deliberately not armed for a message row, + // because the write it would have made was thread-scoped and would have + // marked the whole conversation read. With the write scoped to one message + // that objection is gone, and leaving it unarmed would mean the message + // the user is reading is the one kind that never gets marked read. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString path = dir.filePath(QStringLiteral("qtmaildir.conf")); + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write("[general]\nmark_read_delay_ms = 0\n"); + file.close(); + + Config config; + config.load(path); + + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *timer = window.findChild<QTimer *>(QStringLiteral("markReadTimer")); + QVERIFY(timer); + + // The reply is unread; its thread is not, so a thread-keyed timer would + // have declined to arm at all. + // + // No "is it still unread" guard before the wait: the delay is 0 and the + // helper pumps the event loop, so the write has already happened by the + // time selection returns. The assertions below are on the write itself, + // which is what this test is about, and the fixture above is what + // establishes the reply started unread. + const QModelIndex reply = expandSecondThreadAndSelectItsReply( + view, model, {}, {}, QStringList{ QStringLiteral("unread") }); + QVERIFY(reply.isValid()); + + QTRY_VERIFY_WITH_TIMEOUT(!timer->isActive(), 2000); + QApplication::processEvents(); + + QCOMPARE(window.pendingMessageIdsForTesting(), + QStringList{ QStringLiteral("m1@example.org") }); + QVERIFY2(window.pendingThreadIdsForTesting().isEmpty(), + "reading one reply marked its whole thread read"); + + // And the row shows it, which is the half item 105 built. + QVERIFY2(!model->messageAt(reply).isUnread(), + "the reply was marked read without its row following"); +} + +void TestMainWindow::taggingTheOpenRootMessageKeepsTheStripPopulated() +{ + // The user, 2026-08-16: "right pane loses the chip row when repainting, it + // simply disappears". + // + // The strip refresh added for item 105 reads the message's tags through + // messageById(), which searches only the loaded CHILDREN. A root card's + // message is never among them, so the lookup returned a default-constructed + // node and the refresh set the strip to that node's empty tag list, wiping + // a strip that had been correct a moment earlier. Worse than not + // refreshing: it actively destroyed what was there. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *strip = window.findChild<TagStrip *>(); + QVERIFY(strip); + + ThreadSummary t = makeThread(QStringLiteral("t1"), + { QStringLiteral("todo") }); + t.totalCount = 1; + model->appendBatch({ t }); + + selectThreadRow(view, 0); + QApplication::processEvents(); + + // visible + hidden, not visible alone. TagStrip is a single row that + // collapses whatever does not fit into a trailing "+N" chip, and this + // window is never shown, so it has no width to lay out with and puts + // almost everything in the hidden half. Asserting on visibleTags() alone + // measures the layout, not the data, and fails for a reason this test does + // not care about. + const auto stripTags = [strip]() { + return strip->visibleTags() + strip->hiddenTags(); + }; + + // The guard: the strip has to be showing something before the edit, or + // this cannot tell "wiped" from "never populated". + QVERIFY2(stripTags().contains(QStringLiteral("todo")), + "the strip never showed the selected thread's tags"); + + auto *action = window.findChild<QAction *>(QStringLiteral("delete")); + QVERIFY(action); + action->trigger(); + + QVERIFY2(!stripTags().isEmpty(), + "the chip row was emptied: the refresh looked the message up " + "among the loaded replies, where a root card's message never is, " + "and set the strip to the resulting empty tag list"); + QVERIFY2(stripTags().contains(QStringLiteral("todo")), + "the strip lost the tag the message still carries"); + QVERIFY2(stripTags().contains(QStringLiteral("deleted")), + "the strip did not pick up the tag just written"); +} + +void TestMainWindow::aLoadedMessageCorrectsTheStripFromTheThreadsUnion() +{ + // Reported by hand, 2026-08-16, against a real four-message thread whose + // root carried `unread` and whose THIRD message carried `signed`: + // "the right pane chips update and both signed and unread disappear ... + // changing message and going back makes them reappear". + // + // Neither half was the write's doing. Selecting a thread row sets the strip + // from ThreadSummary::tags, which is notmuch's UNION over the thread, so + // the pane claimed the root message was `signed` when a sibling was. The + // mark-read write then replaced it with the root's real tags, correctly + // dropping both, and reselecting put the union back. The pane was lying + // BEFORE the write, not after it. + // + // The load is the authority: MessageRef carries the message's own tags. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *strip = window.findChild<TagStrip *>(); + QVERIFY(strip); + + const auto stripTags = [strip]() { + return strip->visibleTags() + strip->hiddenTags(); + }; + + // The union carries `signed`; the root message does not. + ThreadSummary t = makeThread(QStringLiteral("t1"), + { QStringLiteral("inbox"), + QStringLiteral("signed"), + QStringLiteral("unread") }); + t.totalCount = 4; + model->appendBatch({ t }); + + selectThreadRow(view, 0); + QApplication::processEvents(); + + // Before the load the strip can only show the union, which is what the + // model holds. That is the state the user saw and reported. + QVERIFY(stripTags().contains(QStringLiteral("signed"))); + + // The worker answers with the message's OWN tags. + MessageRef ref; + ref.messageId = QStringLiteral("t1-first@example.org"); + ref.tags = QStringList{ QStringLiteral("inbox"), QStringLiteral("unread") }; + QMetaObject::invokeMethod( + &window, "onMessageLoaded", Qt::DirectConnection, + Q_ARG(QVector<MessageRef>, QVector<MessageRef>{ ref }), + Q_ARG(quint64, window.currentGenerationForTesting())); + QApplication::processEvents(); + + QVERIFY2(!stripTags().contains(QStringLiteral("signed")), + "the pane still claims the root message is signed, which is a " + "sibling's tag: it is showing the thread's union rather than the " + "message on display"); + QVERIFY2(stripTags().contains(QStringLiteral("unread")), + "the pane lost a tag the message really carries"); +} + +void TestMainWindow::aLoadedRootMessageGivesTheCardItsOwnTags() +{ + // The same correction, reaching the MODEL, which is what fixes the two + // repaint reports: "if I mark the root message read the left pane entry + // doesn't repaint (stays bold)" and the same for delete. + // + // The card could not repaint because the model had no per-message tags for + // a root at all, so a message-scoped write updated the thread summary only + // when the thread was a single message. A load gives the root the same + // per-message node a reply has had all along, and from then on the card + // draws the message it displays. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + ThreadSummary t = makeThread(QStringLiteral("t1"), + { QStringLiteral("inbox"), + QStringLiteral("signed"), + QStringLiteral("unread") }); + t.totalCount = 4; + model->appendBatch({ t }); + + selectThreadRow(view, 0); + QApplication::processEvents(); + + MessageRef ref; + ref.messageId = QStringLiteral("t1-first@example.org"); + ref.tags = QStringList{ QStringLiteral("inbox"), QStringLiteral("unread") }; + QMetaObject::invokeMethod( + &window, "onMessageLoaded", Qt::DirectConnection, + Q_ARG(QVector<MessageRef>, QVector<MessageRef>{ ref }), + Q_ARG(quint64, window.currentGenerationForTesting())); + QApplication::processEvents(); + + // The model now knows what the root message itself carries. + const MessageNode root = + model->messageById(QStringLiteral("t1-first@example.org")); + QCOMPARE(root.messageId, QStringLiteral("t1-first@example.org")); + QVERIFY2(!root.tags.contains(QStringLiteral("signed")), + "the root's node still carries a sibling's tag"); + + const QModelIndex threadIndex = model->index(0, 0, QModelIndex()); + QSignalSpy spy(model, &QAbstractItemModel::dataChanged); + + auto *toggle = window.findChild<QAction *>(QStringLiteral("toggle_unread")); + QVERIFY(toggle); + toggle->trigger(); + + QVERIFY2(spy.count() >= 1, "marking the root message read repainted nothing"); + QVERIFY2(!model->messageById(QStringLiteral("t1-first@example.org")) + .isUnread(), + "the root message is still unread after being marked read"); + + // The card now draws that message, so it stops looking unread. Asserted on + // the FONT, which is what the user means by "stays bold". + QVERIFY2(!model->data(threadIndex, Qt::FontRole).value<QFont>().bold(), + "the card still reads as unread, so the row stays bold and the " + "user sees nothing"); + QVERIFY2(model->threadAt(0).isUnread(), + "the thread summary was rewritten, claiming a four-message thread " + "is read when three of its messages still are not"); +} + void TestMainWindow::aTransientStatusMessageExpires() { // "Sync complete" describes an event, not a state, and reads as though it @@ -4796,7 +5716,10 @@ void TestMainWindow::anEditDuringABackgroundSyncIsNotSentYet() QVERIFY(model); auto *view = window.findChild<QTreeView *>(); QVERIFY(view); - auto *action = window.findChild<QAction *>(QStringLiteral("flag")); + // The THREAD action: this test asserts on the thread ROW, which a + // message-scoped write deliberately leaves alone since item 108. What + // is under test is the HOLD, which is identical either way. + auto *action = window.findChild<QAction *>(QStringLiteral("flag_thread")); QVERIFY2(action, "no flag action registered"); model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); @@ -4827,7 +5750,10 @@ void TestMainWindow::aHeldEditIsSentWhenTheBackgroundSyncEnds() QVERIFY(model); auto *view = window.findChild<QTreeView *>(); QVERIFY(view); - auto *action = window.findChild<QAction *>(QStringLiteral("flag")); + // The THREAD action: this test asserts on the thread ROW, which a + // message-scoped write deliberately leaves alone since item 108. What + // is under test is the HOLD, which is identical either way. + auto *action = window.findChild<QAction *>(QStringLiteral("flag_thread")); QVERIFY(action); model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); @@ -5360,15 +6286,19 @@ void TestMainWindow::theImportantActionStillWritesTheFlaggedTag() QVERIFY(action); action->trigger(); - // sendThreadTagChange() applies the change to the model optimistically, so - // the tag the action really wrote is observable here without a worker. - QVERIFY2(model->threadAt(0).isFlagged(), + // Asserted on the CHANGE that was sent rather than on the thread's row. + // Since item 108 this action is message-scoped, so it writes to the + // message the card displays and the thread summary is deliberately left + // alone. The tag name is what this test is about, and the change carries + // it whichever scope the action uses. + const TagChange sent = window.pendingChangeForTesting(); + QVERIFY2(sent.added.contains(QStringLiteral("flagged")), "the renamed action no longer writes the `flagged` tag"); - QVERIFY2(model->threadAt(0).tags.contains(QStringLiteral("flagged")), - "the tag written was not `flagged`"); - QVERIFY2(!model->threadAt(0).tags.contains(QStringLiteral("important")), + QVERIFY2(!sent.added.contains(QStringLiteral("important")), "the rename reached the mail store: an `important` tag was " "written, which no other tool reading this Maildir knows"); + QVERIFY2(sent.removed.isEmpty(), + "marking important removed a tag, which it must not"); } void TestMainWindow::theToolbarUsesTheConfiguredIconSize() @@ -5756,20 +6686,59 @@ void TestMainWindow::noTwoActionsShareAnIcon() // Compared by cacheKey() rather than by the theme NAME, which this window // does not keep. Two distinct names that resolve to the same art on a given // theme are just as ambiguous on screen, and that is what the user sees. + // Narrowed by item 108 to the actions that can reach the TOOLBAR, which is + // where the rule comes from: an icon-only toolbar makes the icon the whole + // control. The five whole-thread actions live only in the "Whole thread" + // submenu, whose entries always carry text, and each deliberately shares + // the icon of its message-scoped twin: same operation, wider scope, with + // the words saying which. Giving them five invented shapes would be less + // clear than the pairing. + // + // Named as an exception list rather than by asking the toolbar what it + // holds, so that PUTTING one of these on the toolbar fails this test + // rather than silently passing it. + static const QStringList menuOnlyThreadActions = { + QStringLiteral("archive_thread"), + QStringLiteral("delete_thread"), + QStringLiteral("spam_thread"), + QStringLiteral("toggle_unread_thread"), + QStringLiteral("flag_thread"), + }; + const Config config; MainWindow window(config); + // The exception must not become a hiding place: every one of them still + // has to carry an icon, which everyActionCarriesAnIcon asserts, and none + // may sit on the toolbar. + auto *toolBar = window.findChild<QToolBar *>(); + QVERIFY(toolBar); + for (const QString &name : menuOnlyThreadActions) { + auto *action = window.findChild<QAction *>(name); + QVERIFY2(action, qPrintable(QStringLiteral("no action named %1").arg(name))); + QVERIFY2(!toolBar->actions().contains(action), + qPrintable(QStringLiteral("%1 is on the toolbar, where a " + "shared icon is ambiguous, so it " + "cannot be exempt from this rule") + .arg(name))); + } + QHash<qint64, QString> owners; QStringList collisions; int withIcons = 0; + int compared = 0; for (const QString &name : KeyMap::knownActions()) { auto *action = window.findChild<QAction *>(name); QVERIFY2(action, qPrintable(QStringLiteral("no action named %1").arg(name))); + if (!action->icon().isNull()) + ++withIcons; + if (menuOnlyThreadActions.contains(name)) + continue; if (action->icon().isNull()) continue; + ++compared; - ++withIcons; const qint64 key = action->icon().cacheKey(); const auto existing = owners.constFind(key); if (existing != owners.constEnd()) { @@ -5789,6 +6758,10 @@ void TestMainWindow::noTwoActionsShareAnIcon() .arg(withIcons) .arg(KeyMap::knownActions().size()))); + // And the exception list did not swallow the comparison itself. + QCOMPARE(compared, KeyMap::knownActions().size() + - menuOnlyThreadActions.size()); + QVERIFY2(collisions.isEmpty(), qPrintable(QStringLiteral("actions sharing one icon: %1") .arg(collisions.join(QStringLiteral("; "))))); diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index de96dbf..899fd11 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -52,6 +52,7 @@ private slots: void applyTagsIgnoresUnknownMessageIds(); void applyTagsWithNoIdsDoesNothing(); void queryStillWorksAfterWrite(); + void aThreadCarriesItsCardMessagesOwnTags(); void applyTagsToThreadsTagsEveryMessage(); void applyTagsToThreadsSpansMultipleThreads(); @@ -274,6 +275,55 @@ void TestNotmuchWorker::aQueryCarriesEachThreadsFirstMessageId() QVERIFY2(sawTheThread, "the two-message thread was not in the results"); } +void TestNotmuchWorker::aThreadCarriesItsCardMessagesOwnTags() +{ + // Item 111. A card draws its own message's tags at full size and the rest + // of the conversation's smaller, so it needs BOTH: `tags` is notmuch's + // union over the thread and `firstMessageTags` is the one message the card + // stands for. + // + // Derived from the message LOAD at first, which meant an unopened row had + // no split and drew everything as its own, correcting itself only when the + // user selected it. The user reported exactly that. The query knows, and + // the walk that finds firstMessageId is already holding the message, so + // this is the same index read rather than a second pass. + + // A tag on the REPLY only, which is the case that separates the two: a1 is + // the card's message, a2 its reply. + NotmuchWorker writer(m_fixture.configPath()); + writer.applyTags(TagChange{ { QStringLiteral("a2@example.org") }, + { QStringLiteral("signed") }, + {}, + QStringLiteral("Sign the reply") }); + + const QVector<ThreadSummary> threads = runQuery(QStringLiteral("*")); + bool sawTheThread = false; + for (const ThreadSummary &t : threads) { + if (t.subject != QStringLiteral("Release notes")) + continue; + sawTheThread = true; + + QCOMPARE(t.firstMessageId, QStringLiteral("a1@example.org")); + + // The union carries the reply's tag, as notmuch reports it. + QVERIFY2(t.tags.contains(QStringLiteral("signed")), + "the thread's own tags stopped being the union, which the " + "sibling tier is derived from"); + + // The card's message does not, and this is the whole point: without it + // the card claims a tag belonging to a message it does not display. + QVERIFY2(!t.firstMessageTags.isEmpty(), + "the query carried no per-message tags, so an unopened row " + "has no split and draws every chip at full size"); + QVERIFY2(!t.firstMessageTags.contains(QStringLiteral("signed")), + "the card's message was given its reply's tag"); + + // And it does carry its own. + QVERIFY(t.firstMessageTags.contains(QStringLiteral("inbox"))); + } + QVERIFY2(sawTheThread, "the two-message thread was not in the results"); +} + void TestNotmuchWorker::aSentQueryCarriesTheMatchedMessageNotTheThreadsFirst() { // THE case the Sent branch exists for, and the one hardest to get right. diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index e338661..811b3e3 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -68,6 +68,20 @@ private slots: void threadIdIsReachableFromAnIndex(); void invalidIndexesReturnNothing(); void threadAtOutOfRangeIsSafe(); + void threadForResolvesAReplyThroughItsParent(); + void applyMessageTagChangeRepaintsThatReplyAlone(); + void aDeletedReplyIsPaintedAsDoomed(); + void aDeletedReplyIsStruckThrough(); + void markingAReplyReadChangesItsForeground(); + void anUnreadReplyIsBoldAndStillSmallerThanItsThread(); + void aThreadTagChangeReachesItsLoadedReplies(); + void messageScopeResolvesAThreadRowToTheMessageItDisplays(); + void messageScopeSkipsAThreadRowItCannotNameAMessageFor(); + void aMessageTagChangeReachesTheRootCardsOwnMessage(); + void aMessageTagChangeOnOneOfManyLeavesTheThreadSummaryAlone(); + void aCardListsItsOwnTagsBeforeItsSiblings(); + void theSplitIsKnownBeforeTheRowIsEverOpened(); + void reconcileRefreshesASurvivorsOwnMessageTags(); void updatesTagsForMessage(); void tagChangeIsIdempotent(); void tagChangeSignalsExactlyTheChangedRow(); @@ -877,6 +891,605 @@ void TestThreadListModel::threadAtOutOfRangeIsSafe() QCOMPARE(model.threadAt(0).threadId, QStringLiteral("t1")); } +void TestThreadListModel::threadForResolvesAReplyThroughItsParent() +{ + // Item 88. threadAt() takes a top-level row and a tree numbers rows per + // parent, so the first reply of ANY thread has row() == 0 and threadAt(0) + // answers "t1" for a reply of t2. threadFor() resolves through the parent + // instead, which is what every caller holding an index needs. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), QStringLiteral("one")), + makeThread(QStringLiteral("t2"), QStringLiteral("two")) }); + model.setThreadMessages(QStringLiteral("t2"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) }); + + const QModelIndex second = model.index(1, 0, QModelIndex()); + QCOMPARE(model.threadFor(second).threadId, QStringLiteral("t2")); + + const QModelIndex reply = model.index(0, 0, second); + QVERIFY(reply.isValid()); + QVERIFY2(model.isMessageRow(reply), + "the fixture did not produce a message row"); + QCOMPARE(reply.row(), 0); // The trap: a plausible top-level row number. + + QCOMPARE(model.threadFor(reply).threadId, QStringLiteral("t2")); + + // And the row-taking overload still does the wrong thing for that index, + // which is why it is documented as unsafe rather than merely deprecated. + QCOMPARE(model.threadAt(reply.row()).threadId, QStringLiteral("t1")); + + // An invalid index gives an empty summary, which every caller treats as + // "nothing to do" rather than acting on row 0. + QVERIFY(model.threadFor(QModelIndex()).threadId.isEmpty()); +} + +void TestThreadListModel::applyMessageTagChangeRepaintsThatReplyAlone() +{ + // The user's report: hitting Delete or Ctrl+U on a reply moved the pending + // count and changed nothing on screen. sendMessageTagChange made no + // optimistic update at all, on the correct reasoning that repainting the + // THREAD row would claim every message in it had changed. The row that + // should have repainted is the reply's own. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), QStringLiteral("one")) }); + + MessageNode root = makeNode(QStringLiteral("m0@example.org"), 0); + MessageNode reply = makeNode(QStringLiteral("m1@example.org"), 1); + reply.tags = QStringList{ QStringLiteral("unread") }; + model.setThreadMessages(QStringLiteral("t1"), { root, reply }); + + const QModelIndex threadIndex = model.index(0, 0, QModelIndex()); + const QModelIndex replyIndex = model.index(0, 0, threadIndex); + QVERIFY(model.isMessageRow(replyIndex)); + + const QStringList threadTagsBefore = + model.data(threadIndex, ThreadListModel::TagsRole).toStringList(); + + QSignalSpy spy(&model, &QAbstractItemModel::dataChanged); + + model.applyMessageTagChange(QStringLiteral("m1@example.org"), + { QStringLiteral("deleted") }, + { QStringLiteral("unread") }); + + // The node carries the change, which is what every reply-row role reads. + QCOMPARE(model.messageAt(replyIndex).isDeleted(), true); + QCOMPARE(model.messageAt(replyIndex).isUnread(), false); + + // And the view was told, or the change is invisible until something else + // happens to repaint the row. + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.at(0).at(0).toModelIndex(), replyIndex); + + // The THREAD is untouched. Claiming the whole thread changed is the lie + // the missing update was avoiding, and it must stay avoided. + QCOMPARE(model.data(threadIndex, ThreadListModel::TagsRole).toStringList(), + threadTagsBefore); + QCOMPARE(model.messageAt(model.index(0, 0, threadIndex)).messageId, + QStringLiteral("m1@example.org")); + + // An unknown message is a no-op rather than a wrong row repainted. + spy.clear(); + model.applyMessageTagChange(QStringLiteral("nobody@example.org"), + { QStringLiteral("deleted") }, {}); + QCOMPARE(spy.count(), 0); +} + +void TestThreadListModel::aDeletedReplyIsPaintedAsDoomed() +{ + // Updating the node is not enough on its own: a reply row had no doomed + // branch at all, so a deleted reply repainted identically to an undeleted + // one and the user still saw nothing. The thread row has carried this cue + // since item 13. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), QStringLiteral("one")) }); + + MessageNode root = makeNode(QStringLiteral("m0@example.org"), 0); + MessageNode reply = makeNode(QStringLiteral("m1@example.org"), 1); + model.setThreadMessages(QStringLiteral("t1"), { root, reply }); + + const QModelIndex threadIndex = model.index(0, 0, QModelIndex()); + const QModelIndex replyIndex = model.index(0, 0, threadIndex); + + const QVariant plainBackground = + model.data(replyIndex, Qt::BackgroundRole); + + model.applyMessageTagChange(QStringLiteral("m1@example.org"), + { QStringLiteral("deleted") }, {}); + + const QVariant doomedBackground = + model.data(replyIndex, Qt::BackgroundRole); + QVERIFY2(doomedBackground != plainBackground, + "a deleted reply paints exactly like an undeleted one, so the " + "user has no way to see that Delete did anything"); + QCOMPARE(doomedBackground.value<QBrush>().color(), + ThreadListModel::deletedColour()); + + // Spam is the other half of isDoomed() and gets its own colour, so the two + // are told apart by hue rather than by shade. + model.applyMessageTagChange(QStringLiteral("m1@example.org"), + { QStringLiteral("spam") }, + { QStringLiteral("deleted") }); + QCOMPARE(model.data(replyIndex, Qt::BackgroundRole).value<QBrush>().color(), + ThreadListModel::spamColour()); +} + +void TestThreadListModel::aDeletedReplyIsStruckThrough() +{ + // The fill is not the only cue, deliberately: a strike-out survives a + // screenshot, a colourblind reader and a theme that overrides the + // background. The thread row has carried both since item 13; a reply had + // neither. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), QStringLiteral("one")) }); + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) }); + + const QModelIndex replyIndex = + model.index(0, 0, model.index(0, 0, QModelIndex())); + + QVERIFY(!model.data(replyIndex, Qt::FontRole).value<QFont>().strikeOut()); + + model.applyMessageTagChange(QStringLiteral("m1@example.org"), + { QStringLiteral("deleted") }, {}); + + QVERIFY2(model.data(replyIndex, Qt::FontRole).value<QFont>().strikeOut(), + "a deleted reply is not struck through, so the only cue it has " + "is a background colour"); + + // The reply's smaller font is not lost to the strike-out branch: a reply + // reads as subordinate whatever its tags say. + const QFont threadFont = + model.data(model.index(0, 0, QModelIndex()), Qt::FontRole).value<QFont>(); + const QFont replyFont = + model.data(replyIndex, Qt::FontRole).value<QFont>(); + if (threadFont.pointSize() > 0 && replyFont.pointSize() > 0) + QVERIFY(replyFont.pointSize() < threadFont.pointSize()); +} + +void TestThreadListModel::markingAReplyReadChangesItsForeground() +{ + // The user's second report: marking a reply read or unread moved the + // counter with no visible change. A reply is deliberately NEVER bold, so + // unlike a thread row its only cue is the foreground dimming. That cue has + // to at least exist and change, which is what this pins. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), QStringLiteral("one")) }); + + MessageNode root = makeNode(QStringLiteral("m0@example.org"), 0); + MessageNode reply = makeNode(QStringLiteral("m1@example.org"), 1); + reply.tags = QStringList{ QStringLiteral("unread") }; + model.setThreadMessages(QStringLiteral("t1"), { root, reply }); + + const QModelIndex replyIndex = + model.index(0, 0, model.index(0, 0, QModelIndex())); + + const QVariant unreadForeground = + model.data(replyIndex, Qt::ForegroundRole); + + model.applyMessageTagChange(QStringLiteral("m1@example.org"), {}, + { QStringLiteral("unread") }); + + const QVariant readForeground = model.data(replyIndex, Qt::ForegroundRole); + QVERIFY2(readForeground != unreadForeground, + "marking a reply read changed nothing about how its row paints"); + QCOMPARE(readForeground.value<QBrush>().color(), + ThreadListModel::readColour()); + + // And back, so the toggle is visible in both directions rather than only + // on the way to read. + model.applyMessageTagChange(QStringLiteral("m1@example.org"), + { QStringLiteral("unread") }, {}); + QCOMPARE(model.data(replyIndex, Qt::ForegroundRole), unreadForeground); +} + +void TestThreadListModel::anUnreadReplyIsBoldAndStillSmallerThanItsThread() +{ + // Requested by the user on 2026-08-16: "I prefer the bold on replies + // combined with the dimming." Replies were deliberately never bold before + // that, so this pins the decision rather than describing the code. + // + // Both halves matter. Bold is the second cue, next to the dimming; the + // smaller size is what still separates a reply from the thread heading + // above it, and dropping it would make an unread reply indistinguishable + // from a thread row. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), QStringLiteral("one")) }); + + MessageNode root = makeNode(QStringLiteral("m0@example.org"), 0); + MessageNode reply = makeNode(QStringLiteral("m1@example.org"), 1); + reply.tags = QStringList{ QStringLiteral("unread") }; + model.setThreadMessages(QStringLiteral("t1"), { root, reply }); + + const QModelIndex threadIndex = model.index(0, 0, QModelIndex()); + const QModelIndex replyIndex = model.index(0, 0, threadIndex); + + const QFont unreadFont = + model.data(replyIndex, Qt::FontRole).value<QFont>(); + QVERIFY2(unreadFont.bold(), "an unread reply is not bold"); + + const QFont threadFont = + model.data(threadIndex, Qt::FontRole).value<QFont>(); + if (threadFont.pointSize() > 0 && unreadFont.pointSize() > 0) { + QVERIFY2(unreadFont.pointSize() < threadFont.pointSize(), + "a bold reply is the same size as its thread row, so the two " + "kinds of row no longer read apart"); + } + + // Bold is the unread cue specifically, not decoration on every reply. + model.applyMessageTagChange(QStringLiteral("m1@example.org"), {}, + { QStringLiteral("unread") }); + QVERIFY2(!model.data(replyIndex, Qt::FontRole).value<QFont>().bold(), + "a read reply is still bold, so bold says nothing"); +} + +void TestThreadListModel::aThreadTagChangeReachesItsLoadedReplies() +{ + // The user's report: "if I hit read/unread on the main thread message [...] + // only the main message is repainted [...] the replies don't get + // repainted." + // + // A thread-scoped write reaches every message in the thread IN THE + // DATABASE. applyTagChange only ever updated the thread's summary, so an + // expanded thread kept showing replies with their old tags: bold, undimmed + // and unstruck, describing a state the database no longer held. The rows + // corrected themselves on the next query, which is what made this look + // like a repaint problem rather than a stale-model one. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), QStringLiteral("one")) }); + + MessageNode root = makeNode(QStringLiteral("m0@example.org"), 0); + root.tags = QStringList{ QStringLiteral("unread") }; + MessageNode reply = makeNode(QStringLiteral("m1@example.org"), 1); + reply.tags = QStringList{ QStringLiteral("unread") }; + model.setThreadMessages(QStringLiteral("t1"), { root, reply }); + + const QModelIndex threadIndex = model.index(0, 0, QModelIndex()); + const QModelIndex replyIndex = model.index(0, 0, threadIndex); + QVERIFY(model.messageAt(replyIndex).isUnread()); + + QSignalSpy spy(&model, &QAbstractItemModel::dataChanged); + + model.applyTagChange(QStringLiteral("t1"), {}, + { QStringLiteral("unread") }); + + QVERIFY2(!model.messageAt(replyIndex).isUnread(), + "a thread marked read left its loaded replies carrying unread, so " + "the rows describe a state the database does not hold"); + + // The reply's row was told to repaint, not merely mutated behind the view. + bool replyRepainted = false; + for (const QList<QVariant> &call : spy) { + const QModelIndex from = call.at(0).toModelIndex(); + const QModelIndex to = call.at(1).toModelIndex(); + if (from.parent() == threadIndex && replyIndex.row() >= from.row() + && replyIndex.row() <= to.row()) { + replyRepainted = true; + break; + } + } + QVERIFY2(replyRepainted, + "no dataChanged covered the reply rows, so the view has no reason " + "to redraw them"); + + // Both directions, since a toggle is only fixed if it is visible each way. + model.applyTagChange(QStringLiteral("t1"), { QStringLiteral("unread") }, {}); + QVERIFY(model.messageAt(replyIndex).isUnread()); +} + +void TestThreadListModel::messageScopeResolvesAThreadRowToTheMessageItDisplays() +{ + // Item 108. A thread root RENDERS one message since item 66, so acting on + // it acts on that message. The thread's other messages are reached through + // the explicit thread actions, which still resolve through scopeFor(). + ThreadListModel model; + ThreadSummary t = makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")); + t.totalCount = 7; + t.firstMessageId = QStringLiteral("m0@example.org"); + model.appendBatch({ t }); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + + // Unexpanded, which is the case that matters: the id comes from the query, + // so this needs no children loaded. + QCOMPARE(model.rowCount(root), 0); + + const ActionScope scope = model.messageScopeFor({ root }); + QCOMPARE(scope.messageIds, QStringList{ QStringLiteral("m0@example.org") }); + QVERIFY2(scope.threadIds.isEmpty(), + "a thread row still resolved to its whole thread, so every action " + "on a root card would touch messages it does not display"); + QCOMPARE(scope.messageCount, 1); + QVERIFY2(!scope.wholeThread, + "the status bar would claim '(whole thread)' for a one-message " + "action"); + + // The old resolver is unchanged and is what the thread actions use. + const ActionScope threadScope = model.scopeFor({ root }); + QCOMPARE(threadScope.threadIds, QStringList{ QStringLiteral("t1") }); + QCOMPARE(threadScope.messageCount, 7); + QVERIFY(threadScope.wholeThread); + + // A reply row is unchanged in both: it always stood for one message. + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) }); + const QModelIndex reply = model.index(0, 0, root); + QCOMPARE(model.messageScopeFor({ reply }).messageIds, + QStringList{ QStringLiteral("m1@example.org") }); + + // A root and one of its own replies is two DISTINCT messages, not one + // deduplicated to the thread. + const ActionScope both = model.messageScopeFor({ root, reply }); + QCOMPARE(both.messageIds, + (QStringList{ QStringLiteral("m0@example.org"), + QStringLiteral("m1@example.org") })); + QCOMPARE(both.messageCount, 2); +} + +void TestThreadListModel::messageScopeSkipsAThreadRowItCannotNameAMessageFor() +{ + // firstMessageId is populated by the worker from the query. A summary that + // arrived without one names no message, and the tempting fallback is to + // act on the whole thread instead. That is exactly the silent escalation + // item 108 exists to remove: the user would ask to act on one message and + // hit the conversation. + ThreadListModel model; + ThreadSummary t = makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")); + t.totalCount = 4; + t.firstMessageId.clear(); + model.appendBatch({ t }); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + const ActionScope scope = model.messageScopeFor({ root }); + + QVERIFY2(scope.isEmpty(), + "a thread row with no message id was escalated to its whole " + "thread rather than skipped"); + QCOMPARE(scope.messageCount, 0); +} + +void TestThreadListModel::aMessageTagChangeReachesTheRootCardsOwnMessage() +{ + // The user, 2026-08-16: "delete single message on the root message of a + // thread doesn't trigger the repaint, delete whole thread does". + // + // applyMessageTagChange only searched `children`, and the root message is + // never there: setThreadMessages drops depth 0 because the root row stands + // for it. So a write to the message a root card displays found nothing, + // updated nothing and repainted nothing, while the same write on a reply + // worked. Item 108 made this the ORDINARY case, so the every-day gesture + // was the broken one. + ThreadListModel model; + ThreadSummary t = makeThread(QStringLiteral("t1"), QStringLiteral("one")); + t.totalCount = 1; // A single-message thread. + t.firstMessageId = QStringLiteral("m0@example.org"); + t.tags = QStringList{ QStringLiteral("inbox") }; + model.appendBatch({ t }); + + const QModelIndex threadIndex = model.index(0, 0, QModelIndex()); + QSignalSpy spy(&model, &QAbstractItemModel::dataChanged); + + model.applyMessageTagChange(QStringLiteral("m0@example.org"), + { QStringLiteral("deleted") }, {}); + + // The card has to repaint, which is the whole report. + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.at(0).at(0).toModelIndex(), threadIndex); + + // And it has to LOOK deleted. For a one-message thread the thread's tags + // ARE that message's tags: notmuch_thread_get_tags is a union over the + // thread, and a union over one message is that message. + QVERIFY2(model.threadAt(0).isDeleted(), + "the root card does not show the state of the message it " + "displays, so Delete on it looks like it did nothing"); + + // Works before the thread has ever been expanded, which is the case the + // user hits: nothing loads a root's node until then. + QCOMPARE(model.rowCount(threadIndex), 0); +} + +void TestThreadListModel::aMessageTagChangeOnOneOfManyLeavesTheThreadSummaryAlone() +{ + // The other half, and the reason the fix is not "write it to the summary". + // A thread's tags are a UNION over its messages, so deleting one message of + // seven does not make the conversation deleted, and painting the card + // crimson would claim it did. + ThreadListModel model; + ThreadSummary t = makeThread(QStringLiteral("t1"), QStringLiteral("one")); + t.totalCount = 7; + t.firstMessageId = QStringLiteral("m0@example.org"); + t.tags = QStringList{ QStringLiteral("inbox"), QStringLiteral("unread") }; + model.appendBatch({ t }); + + const QModelIndex threadIndex = model.index(0, 0, QModelIndex()); + QSignalSpy spy(&model, &QAbstractItemModel::dataChanged); + + model.applyMessageTagChange(QStringLiteral("m0@example.org"), + { QStringLiteral("deleted") }, + { QStringLiteral("unread") }); + + // Still repaints: MessageIdRole and anything else keyed on the root's own + // node has changed, and the row is what the user is looking at. + QCOMPARE(spy.count(), 1); + + QVERIFY2(!model.threadAt(0).isDeleted(), + "deleting one message of a seven-message thread painted the whole " + "conversation as deleted"); + QVERIFY2(model.threadAt(0).isUnread(), + "marking one message of a seven-message thread read claimed the " + "whole conversation was read, though six messages still are not"); +} + +void TestThreadListModel::aCardListsItsOwnTagsBeforeItsSiblings() +{ + // The user, 2026-08-16, looking at a real four-message thread: the card + // showed `mailing-list/SBo` and `signed`, and `signed` vanished the moment + // the row was selected, because it belongs to a SIBLING and item 110 made + // the card stop claiming it. + // + // Their answer, which is better than either extreme: show both, and let + // size say whose is whose. Own tags first at full size, the thread's other + // tags after, smaller and muted. Nothing disappears; a chip only shrinks + // once the split becomes known. + ThreadListModel model; + ThreadSummary t = makeThread(QStringLiteral("t1"), QStringLiteral("one")); + t.totalCount = 4; + t.firstMessageId = QStringLiteral("m0@example.org"); + // The UNION, as notmuch reports it: `signed` is a sibling's. + t.tags = QStringList{ QStringLiteral("inbox"), + QStringLiteral("mailing-list/SBo"), + QStringLiteral("signed"), + QStringLiteral("unread") }; + model.appendBatch({ t }); + + const QModelIndex threadIndex = model.index(0, 0, QModelIndex()); + + // Before the row is opened there is no per-message answer, so every chip + // is in the own tier. This is what stops anything from appearing to vanish + // later: the split narrows the tier, it does not remove a chip. + const QStringList before = + model.data(threadIndex, ThreadListModel::PillTagsRole).toStringList(); + QVERIFY(before.contains(QStringLiteral("mailing-list/SBo"))); + QVERIFY(before.contains(QStringLiteral("signed"))); + QCOMPARE(model.data(threadIndex, ThreadListModel::PillOwnCountRole).toInt(), + before.size()); + + // The message loads, carrying what it really has. + model.setRootMessageTags(QStringLiteral("m0@example.org"), + { QStringLiteral("inbox"), + QStringLiteral("mailing-list/SBo"), + QStringLiteral("unread") }); + + const QStringList after = + model.data(threadIndex, ThreadListModel::PillTagsRole).toStringList(); + + // Same chips, still all present. The user explicitly did not want the + // sibling's tag dropped. + QVERIFY2(after.contains(QStringLiteral("signed")), + "the sibling's tag was dropped from the card rather than being " + "shown smaller, which is what looked like a bug"); + QVERIFY2(after.contains(QStringLiteral("mailing-list/SBo")), + "the card lost a tag the message really carries"); + + // Own first, siblings after, and the count is where the delegate switches + // fonts. + const int own = + model.data(threadIndex, ThreadListModel::PillOwnCountRole).toInt(); + QVERIFY2(own > 0 && own < after.size(), + "the split did not happen: every chip is in one tier"); + QCOMPARE(after.mid(0, own), + QStringList{ QStringLiteral("mailing-list/SBo") }); + QCOMPARE(after.mid(own), QStringList{ QStringLiteral("signed") }); + + // Colours stay aligned with the tags, since the delegate walks them in + // step and a shift would colour a chip with its neighbour's colour. + QCOMPARE(model.data(threadIndex, ThreadListModel::PillColoursRole) + .toList() + .size(), + after.size()); +} + +void TestThreadListModel::theSplitIsKnownBeforeTheRowIsEverOpened() +{ + // The user, 2026-08-16: "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." + // + // The first version derived the split from the message LOAD, so an unopened + // row had no per-message answer and put every chip in the own tier. That is + // honest and useless: the 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. + // + // The query knows. The worker already walks to the card's message to get + // its id, so it reads that message's tags in the same pass and the split + // arrives with the row. + ThreadListModel model; + ThreadSummary t = makeThread(QStringLiteral("t1"), QStringLiteral("one")); + t.totalCount = 4; + t.firstMessageId = QStringLiteral("m0@example.org"); + t.tags = QStringList{ QStringLiteral("inbox"), + QStringLiteral("mailing-list/SBo"), + QStringLiteral("signed"), + QStringLiteral("unread") }; + // What the worker now supplies: the CARD's message, not the thread. + t.firstMessageTags = QStringList{ QStringLiteral("inbox"), + QStringLiteral("mailing-list/SBo"), + QStringLiteral("unread") }; + model.appendBatch({ t }); + + const QModelIndex threadIndex = model.index(0, 0, QModelIndex()); + + // Never opened, never expanded. + QCOMPARE(model.rowCount(threadIndex), 0); + + const QStringList pills = + model.data(threadIndex, ThreadListModel::PillTagsRole).toStringList(); + const int own = + model.data(threadIndex, ThreadListModel::PillOwnCountRole).toInt(); + + QVERIFY2(own < pills.size(), + "an unopened row still puts every chip in the own tier, so the " + "card renders them all at full size and only corrects itself " + "when the row is selected"); + QCOMPARE(pills.mid(0, own), QStringList{ QStringLiteral("mailing-list/SBo") }); + QCOMPARE(pills.mid(own), QStringList{ QStringLiteral("signed") }); + + // And a message-scoped write still lands, without a load having happened. + model.applyMessageTagChange(QStringLiteral("m0@example.org"), + { QStringLiteral("deleted") }, {}); + QVERIFY(model.messageById(QStringLiteral("m0@example.org")).isDeleted()); + QVERIFY2(!model.threadAt(0).isDeleted(), + "the thread summary was rewritten for a one-message edit on a " + "four-message thread"); +} + +void TestThreadListModel::reconcileRefreshesASurvivorsOwnMessageTags() +{ + // reconcile() keeps a surviving row's NODE, deliberately: its children and + // its loaded flag are the expansion state the method exists to preserve. + // That means the per-message tags have to be refreshed explicitly, and the + // change detector has to notice when only they moved. + // + // The case: a sync where the root message alone changed, which is exactly + // what an external `notmuch tag` or another client does. The thread's union + // can be identical while the card's own message is not. + ThreadListModel model; + ThreadSummary before = makeThread(QStringLiteral("t1"), + QStringLiteral("one")); + before.totalCount = 2; + before.firstMessageId = QStringLiteral("m0@example.org"); + before.tags = QStringList{ QStringLiteral("inbox"), + QStringLiteral("unread") }; + before.firstMessageTags = QStringList{ QStringLiteral("inbox"), + QStringLiteral("unread") }; + model.appendBatch({ before }); + + QVERIFY(model.messageById(QStringLiteral("m0@example.org")).isUnread()); + + // The root was read elsewhere. The THREAD is still unread, because its + // reply is, so the union does not move at all. + ThreadSummary after = before; + after.firstMessageTags = QStringList{ QStringLiteral("inbox") }; + + QSignalSpy spy(&model, &QAbstractItemModel::dataChanged); + model.reconcile({ after }); + + QVERIFY2(!model.messageById(QStringLiteral("m0@example.org")).isUnread(), + "a sync that changed only the card's own message left the row " + "showing the old per-message tags"); + QVERIFY2(spy.count() >= 1, + "the change was applied without telling the view, so the card " + "keeps its old pixels until something else repaints it"); + + // The expansion state is still what reconcile() exists to preserve. + QCOMPARE(model.threadAt(0).threadId, QStringLiteral("t1")); +} + void TestThreadListModel::updatesTagsForMessage() { ThreadListModel model; |