diff options
Diffstat (limited to 'docs/superpowers')
| -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 |
2 files changed, 1034 insertions, 77 deletions
diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md 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 |