diff options
Diffstat (limited to 'docs/superpowers/plans')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 159 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 95 |
2 files changed, 163 insertions, 91 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. 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 7e7c040..5b84b15 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 @@ -241,14 +241,15 @@ taking that too literally. | 165 | A draft gets a new Message-ID on every autosave | enhancement | ? | open, 2026-08-25, found while hand-testing 163 and 164. `MessageBuilder::build()` generates an id unconditionally and every autosave calls it, so each revision is a distinct MESSAGE to notmuch and to the server rather than a new version of one. Invisible while the file is replaced correctly, which item 163's fix restores; it is what turned that fork into two messages rather than one duplicated file. Needs a DECISION on what a draft's identity is before any code: a stable id reused at send, a stable id discarded at send, or the status quo. Neither `ComposeContext` nor `OutgoingMessage` has a field to carry an id, so it is not a changed call site | | 166 | Mail you send to your own other account loses `inbox` | defect | S | **done 2026-08-25**, unreleased. `sent_only()` keeps a message only when EVERY file is inside a sent folder, which is what the carve-out's docstring already claimed. No query can express it, measured; the root comes from `database.mail_root`, with a split-index fixture the ordinary layout cannot provide. Verified read-only against the live index: 780 of 807 still stripped, 27 spared, no arrival affected | | 167 | No way to tell one build of an unreleased version from another | enhancement | XS | **done 2026-08-25**, unreleased. The user chose a counter over a git description: `QTMAILDIR_BUILD_NUMBER`, a cmake option ON by default, increments a counter in the BUILD directory on every build and writes `buildnumber.h`. `QTMAILDIR_VERSION_DISPLAY` carries it; `QTMAILDIR_VERSION` stays clean and is what the window title, `applicationVersion` and the release procedure use | -| 168 | Delete is offered on mail already in the trash, and does nothing | defect | S | **done 2026-08-25**, unreleased. Delete is hidden when every selected row is already in its account's trash, Restore when none is, both keyed on the PATH rather than the `deleted` tag. Delete also drops `unread` now, in the same TagChange so one undo returns the folder and the tag together | +| 168 | Delete is offered on mail already in the trash, and does nothing | defect | S | **done 2026-08-25**, unreleased. Delete is hidden when every selected row is already in its account's trash, Restore when none is, both keyed on the PATH rather than the `deleted` tag. Delete also drops `unread` now, in the same TagChange so one undo returns the folder and the tag together . **Its rule is now incomplete, see item 178** (2026-08-28): the check still judges a row on `firstMessagePath`, which item 177 stopped being a conversation row's identity, so a conversation whose messages disagree about the trash answers on one of them | | 169 | A card shows the account only as a bar, with no fade and no avatar | presentation | M | **done** 2026-08-26, unreleased, on `card-avatars`, merged fast-forward. Both halves: a `QLinearGradient` from the account colour to the pane's base across the card, and a squircle avatar with initials, given a rect in `CardLayout` so the geometry is asserted without a painter. **Hand-testing found four defects**, all fixed in 9ae43f9: `Avatar::initialsFor()` normalises the display name first (drops the angle-addr, takes the first comma-separated author, unwraps quotes, treats a bare address as no name, requires a word to carry a letter or digit); the two-tone gradient axis spans the DIAMETER rather than a radius, which was letting one hue fill the whole face; the account fade runs right to left, anchored opaque at the card's right edge; and a flat view hashes `ThreadSummary::firstMessageRecipient` rather than the user's own address. The vCard half stays blocked on item 72 | | 170 | A row that stops matching the view only leaves it on the Delete path | defect | S | **done 2026-08-28**, unreleased. `MainWindow::syncViewMembership()` is the guard, moved off the move path and called from all three funnels (message, thread, move). It does the INVERSE too: a write that adds the view's tag back refreshes, since the model cannot insert a row for a thread the query never returned, and without it an undone mark-read stayed invisible in the view it was undone in. Found a second defect while testing: `ThreadListModel::applyTagChange()` never updated the ROOT message's own tags, which a thread row's card draws in preference to the summary since item 110, so an archived thread both kept drawing `inbox` and was judged to still match | | 171 | A forwarded HTML message reaches the recipient as plain text | defect | M | **done** 2026-08-27, unreleased. Design in `specs/2026-08-27-forward-html-design.md`. The forward sends ONE part chosen by the Send-as-HTML toggle, with the original shown in a read-only pane beside the editor, and remote content stripped by default with a per-forward opt-out. Hand-tested 2026-08-28. Four parts: `HtmlSanitiser` (an ALLOW-LIST, unlike `namespaceCids()`, because a missed strip is a beacon where a missed rewrite is a broken image), a text fallback for the ~9% of mail with no plain part, the MIME nesting, and the composer control | | 172 | A draft this application writes is tagged `unread` | defect | XS | **done** 2026-08-27, unreleased. `DraftStore::write()` was called with `"D"`, and `maildir.synchronize_flags` makes notmuch tag anything without `S` as `unread`. Self-healing on the next sync of that folder, which is what made it look intermittent | | 173 | The composer is a plain-text editor, not WYSIWYG | v2 | L | open, 2026-08-27, **asked for by the user** while hand-testing 171. This is a GUI mail client and should edit rich text the way one does: the forwarded original, and the user's own formatting, visible and editable in place. Supersedes the preview 171 shipped as a middle ground, and **subsumes item 133** (markdown syntax highlighting), which is the same want answered cheaply. See the entry: the draft format and the markdown-as-source-of-truth model both change | -| 176 | Undoing a thread-scoped action applies its inverse to messages it never changed | defect | S | open, 2026-08-28, found by hand while testing 170, **data-affecting and live**. `ThreadTagCommand::undo()` inverts the tags and keeps the THREAD as scope, so undoing `Mark thread read` adds `unread` to every message in the conversation rather than to the ones the write removed it from. Measured on the user's own mail: a 44-message thread had 2 unread, was marked read, and the undo left 43 unread. `maildir.synchronize_flags` is on, so it rewrote the files and would have reached the server. Repaired by hand the same day. `MessageTagCommand` has the same shape on a multi-message scope | -| 177 | A thread row means both a message and a conversation, and neither consistently | design | L | open, 2026-08-28, **specified**: `specs/2026-08-28-thread-row-identity-design.md`, read that rather than this row. A thread row becomes the CONVERSATION: it carries no first message, shows a dashboard rather than a message, and every action on it is thread-scoped. A summary with `totalCount == 1` stays a message row and does not change. Reverses items 108, 110 and 111 deliberately, deletes the `*_thread` submenu and its five action names (an `### Upgrading` note), and makes membership the union with no exceptions. **Item 170 is blocked on this** and item 176 gets easier under it | +| 176 | Undoing a thread-scoped action applies its inverse to messages it never changed | defect | S | **done 2026-08-28**, unreleased, on `thread-row-identity`. `NotmuchWorker::applyTags()` reads each message's tags before writing and reports only the ids whose tags actually MOVED; a `TagCommand` base carries that effective set for both `ThreadTagCommand` and `MessageTagCommand`, which had the same defect on a multi-row selection. `tagsApplied` does NOT fire on an empty effective list, since an empty change would push an undo entry whose inverse adds a tag no message ever carried, the same bug one step later. `sendThreadTagChange` gained `onlyMessageIds` so it keeps its thread-scoped REPAINT while restricting the WRITE: the card that changed on screen and the messages that changed on disk are different sets on purpose. **The spec's own plan said item 177 would make a thread undo honest and shrink this to the multi-row case; that was wrong and is corrected in the spec**, an undo inverts an EFFECT, not a scope | +| 177 | A thread row means both a message and a conversation, and neither consistently | design | L | **done 2026-08-28**, unreleased, on `thread-row-identity`, eleven commits. Spec: `specs/2026-08-28-thread-row-identity-design.md`. `ThreadListModel::isConversationRow()` is the single predicate and `scopeForSelection()` the single resolver, replacing the `scopeFor()`/`messageScopeFor()` pair that made the CALLER choose. A summary with `totalCount == 1` is unchanged. **Reverses items 108, 110 and 111**, and the user confirmed they are happy to lose the two-tier chips; the `*_thread` submenu and its five action names are deleted with an `### Upgrading` note. Item 112's hiding rule is reversed too: with the absolute entries gone, hiding the toggle on a mixed selection leaves no way to act, so it is a catch-all and the write direction moves with the label. Membership is the union, with two user decisions kept (never evict the current row; an asked-for write evicts at once, an automatic one defers) and one documented lag (a long thread's summary is not updated by a message write, so reading its last unread message waits for the next query). Dashboard from a `ThreadDigest` read by its own worker walk. Two traps found while building: a `QStackedWidget` takes the LARGEST minimum width of its pages and the hidden dashboard was raising the pane's minimum to 395px over MainWindow's 300px floor, caught by an existing resize test; and the pane now holds two `TagStrip`s, so both are named | +| 178 | Delete and Restore judge a conversation on one message | defect | XS | open, 2026-08-28, split out of item 168 by item 177 and flagged in that spec's "Open, deliberately". `MainWindow::everySelectedRowIsInATrashFolder()` reads `ThreadSummary::firstMessagePath` for any row that is not a message row, which was correct while a thread row MEANT that message and is not correct now that it means the conversation. A conversation is in the trash when ALL of its messages are, so a partly-trashed thread currently answers on whichever message the query returned first: Delete can be hidden on a conversation that still has mail outside the trash, and Restore offered on one that mostly does not. Not data-affecting, both actions are no-ops in the wrong direction, but it is an inconsistency the row-kind rule was supposed to remove. Needs the summary to carry the answer, or the paths of every message, which the digest walk already reads | | 174 | An external `notmuch new` reaches the index without the pending count noticing | defect | S | open, 2026-08-28, from the notes. Item 54 cleared the count for a sync run by `mailsync.sh`, which is what `SyncMonitor` watches; a bare `notmuch new` (a hand run, or a cron entry that is not the script) takes notmuch's own write lock and touches `/tmp/mbsync.lock` not at all, so nothing observes it. The user's framing is the approach: we own `mailsync.sh` and the whole process | | 175 | The send countdown says Undo, and cannot be skipped | presentation | XS | open, 2026-08-28, from the notes. Two changes in one control: the button reads Abort, and a second button sends immediately rather than waiting the countdown out | @@ -1165,56 +1166,6 @@ id and a draft of a reply carries both. replaced correctly now, so the fork this would have mitigated no longer happens by that route. -## 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. - ## 173. The composer is a plain-text editor, not WYSIWYG **Observed (user, 2026-08-27):** asked for directly while hand-testing item @@ -1329,41 +1280,3 @@ call the button would make, so skipping is stopping the timer and calling it. A skip button must not become a default, or the protection is gone for everyone who learns to press it. - Which button is the default on Return matters here and is the user's call. - -## 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. |