diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-28 19:17:06 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-28 19:17:06 +0200 |
| commit | db38ca136c45f34f5fa2d342ee180f068073be37 (patch) | |
| tree | e5ec786f47989bb594fca896802849b365056ddc /docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | |
| parent | ae74237ca11640ceb68887b2c6d09ecf1befb342 (diff) | |
| download | qtmaildir-db38ca136c45f34f5fa2d342ee180f068073be37.tar.gz qtmaildir-db38ca136c45f34f5fa2d342ee180f068073be37.zip | |
docs: close items 170, 176 and 177
Diffstat (limited to 'docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 159 |
1 files changed, 159 insertions, 0 deletions
diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index 81705a2..f062106 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 @@ -9090,3 +9090,162 @@ forwards correctly with the strip box both checked and unchecked. The recipient sees the original's formatting either way; with the box checked the remote images are gone and the layout survives without them. +## 170. A row that stops matching the view only leaves it on the Delete path + +**Observed (user, from the notes):** "should we refactor the list UI to be +responsive so changes are applied immediately instead of waiting for a view +change to repaint?" + +**Cause (verified in the code, 2026-08-26).** Two different properties were +being called "responsive", and only one of them was built. + +The optimistic **repaint** is universal. `ThreadListModel::applyTagChange()` +covers a thread-scoped write, `applyMessageTagChange()` a message-scoped one +(items 105 to 111), and `revertPendingTagChange()` undoes either if the write +is rejected. A chip, a bold row and a dimmed row all move the moment the user +acts. + +The optimistic **membership** is not. `ThreadListModel::removeThreadsWithoutTag()` +has exactly ONE caller, in `trashMessages()`, added last session because Delete +strips `inbox` and a deleted message sat in the Inbox view across restarts. The +ordinary tag path never calls it: neither `sendMessageTagChange()` nor +`sendThreadTagChange()` asks whether the row still belongs in the view. + +So in the Unread view, marking a message read repaints the row and leaves it in +a list defined by `tag:unread`, which it no longer matches. Un-flagging in the +Flagged view is the same, and so is removing `inbox` by hand from the Inbox +view. It corrects itself at the next query or sync, which is exactly the "waits +for a view change" the note describes. + +**Approach.** Not a refactor. `viewFilterTag()` already resolves the view's own +tag from the query, and `removeThreadsWithoutTag()` already does the removal. +The gap is that the guard sits in `trashMessages()` rather than at the funnel +every tag write passes. Move it, or call it from both send paths. + +**Constraints.** + +- The guard's existing reasoning is what makes this safe and must be kept: only + a plain `tag:<x>` view has a membership one tag decides. A path query (Trash, + Sent, Drafts) is unaffected by a tag going away, and a hand-typed query cannot + be reasoned about. Both are left alone. Without that, marking read in an `id:` + view would empty the list. +- A row leaving is not revertible by `revertPendingTagChange()`, which repaints + rather than reinserts. A REJECTED write would leave the row gone until the + next query. The move path already carries that exposure; check whether it is + acceptable at the tag path's much higher frequency, or make the removal wait + for confirmation there. +- The inverse case is deliberately out of scope: a row that starts matching + cannot be inserted optimistically, since the model has no summary for a + thread the query never returned. +- Undo goes back through the same funnel, so a removal must not make an undone + mark-read invisible in the view it was undone in. + +**Resolved 2026-08-28**, on `thread-row-identity`, released unreleased. + +`MainWindow::syncViewMembership()` is the guard. It was moved off the move +path, where `trashMessages()` was its only caller, and is now called from all +three funnels: the message write, the thread write and the move. The existing +reasoning under Constraints is kept unchanged, so only a plain `tag:<x>` view +is acted on; a path query and a hand-typed query are still left alone. + +Three things the section did not anticipate: + +- **The inverse case is built after all.** A write that puts the view's tag + BACK refreshes the list. The model still cannot insert a row for a thread the + query never returned, which is why it is a refresh rather than an insertion, + but leaving it out made an undone mark-read invisible in the view it was + undone in, which is worse than the query cost. +- **A row is never evicted while it is current**, and an AUTOMATIC write defers + its eviction until the selection moves. The automatic mark-read fires two + seconds after selection, so evicting on it takes the row out from under the + user mid-read. A write the user asked for evicts at once. +- **A second defect was found while testing it.** + `ThreadListModel::applyTagChange()` never updated the ROOT message's own + tags, which a thread row's card drew in preference to the summary since item + 110, so an archived thread both kept drawing `inbox` and was judged to still + match. Item 177 has since removed that per-message tier entirely, so the + card and the judgement now read the same union. + +One lag is documented rather than fixed, in the spec and in CLAUDE.md: reading +the LAST unread message of a long conversation does not evict it immediately, +because `applyMessageTagChange` deliberately leaves a long thread's summary +alone and judging on a stale union would be wrong in both directions. The row +leaves at the next query or sync. + +## 176. Undoing a thread-scoped action applies its inverse to messages it never changed + +**Observed (user, 2026-08-28):** found while hand-testing item 170. A +thread-scoped `Mark thread read`, then Ctrl+Z, left almost the whole +conversation unread rather than restoring the two messages that had been. + +**Cause (measured, not read).** `ThreadTagCommand::undo()` +(`src/mainwindow.h`) sends `sendThreadTagChange(m_threadIds, m_remove, m_add, +...)`: the tags are inverted and the SCOPE is not. `applyTags()` is a blind +add/remove over whatever ids the thread resolves to, so the inverse of +"remove `unread` from 44 messages" is "add `unread` to 44 messages", +regardless of which of them carried it. + +Measured on the live index: thread of 44 messages, 2 unread. Mark thread read +resolved 44 and removed the tag; the undo resolved 44 and added it, leaving +43 unread. The 42 that were read before the user touched anything were +rewritten. `maildir.synchronize_flags` is on, so the Maildir filenames were +rewritten too and the next sync would have carried it to the server. + +**Approach (not decided).** The command has to record what the write actually +CHANGED, not what it asked for. `applyTags()` is the only place that knows: +it holds each message open and can report the ids whose tags actually moved. +That is a worker change (`tagsApplied` carrying the effective set) plus a +command that stores it. + +**Constraints.** +- The undo stack is this application's substitute for confirmation dialogs + (CLAUDE.md), so an undo that damages state is worse than the dialog it + replaces. +- `MessageTagCommand` has the same shape. It is harmless on a single message, + where asked and changed agree, and has the same defect on a multi-row + selection. +- The five `*_thread` actions are the ones that resolve to a large id set, so + they carry almost all of the exposure. +- A test needs a thread whose messages DISAGREE about the tag. Two messages in + the same state answer identically whichever way the code resolves them, + which is the trap CLAUDE.md already records for item 87. + +**Resolved 2026-08-28**, on `thread-row-identity`, unreleased. + +The Approach was right about where the knowledge lives. +`NotmuchWorker::applyTags()` now reads each message's tags before writing and +reports back only the ids whose tags actually MOVED. A `TagCommand` base +carries that effective set, and both `ThreadTagCommand` and +`MessageTagCommand` derive from it, so the multi-row message case named under +Constraints is fixed by the same change rather than left behind. + +Two details that are decisions rather than implementation: + +- **`tagsApplied` does not fire when the effective list is empty.** Emitting an + empty change would push an undo entry whose inverse adds a tag no message + ever carried, which is this same defect one step later. +- **`sendThreadTagChange` takes an `onlyMessageIds` parameter** so it keeps the + THREAD as its repaint scope while restricting the WRITE to that set. The card + that changed on screen and the messages that changed on disk are genuinely + different sets, and collapsing them would either under-repaint or over-write. + +**The spec's own order of work was wrong about this item**, and the correction +is worth keeping because the wrong version reads as sound. It said item 177 +would make a thread action's undo "honestly what the user asked for", leaving +only the multi-row message case exposed. An undo is not the inverse of a SCOPE, +it is the inverse of an EFFECT: the user asking to mark a whole conversation +read does not make it honest to mark the whole conversation unread afterwards, +because most of it was already read before they touched anything. Item 177 +changed nothing about the exposure except to make the largest case reachable by +the ordinary gesture rather than by a submenu entry. Corrected in +`specs/2026-08-28-thread-row-identity-design.md` rather than deleted from it. + +**Evidence.** Measured on the user's live index before the fix: a conversation +of 44 messages holding 2 unread was marked read, then undone, and came back +with 43 unread. The 42 that had been read before the user touched anything were +rewritten. `maildir.synchronize_flags` is on, so the Maildir filenames were +rewritten too and the next sync would have carried it to the mail server. The +mail was repaired by hand the same day. The regression test builds a thread +whose messages DISAGREE about the tag, since two messages in the same state +answer identically whichever way the code resolves them, which is the trap item +87 already records. |
