diff options
Diffstat (limited to 'docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 101 |
1 files changed, 101 insertions, 0 deletions
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 5b84b15..bf347c8 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 @@ -252,6 +252,8 @@ taking that too literally. | 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 | +| 179 | Undo is one level deep in practice, and there is no Redo | workflow | ? | open, 2026-08-29, from the notes. The `QUndoStack` is real and multi-level; what is missing is a `redo` action (absent from `knownActions()`, never called) and an answer to the stack being CLEARED on every new query (`mainwindow.cpp:3458`), which is what makes a deep stack behave like a shallow one. The clear has a correct reason and cannot simply be removed. Redo re-applies a write to real mail, so item 176's rule binds it too | +| 180 | The repaint rules are discovered one hole at a time | maintenance | S-L | open, 2026-08-29, from the notes, and a QUESTION rather than a defect. Items 105, 107, 109, 110 and 170 are each one hole in the same surface, all found by hand. Three mechanisms (optimistic repaint, `syncViewMembership()`, revert) agree by documentation rather than by code. Cheapest answer is one invariant test, not a rewrite; the user decides which, and that decides the size | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1280,3 +1282,102 @@ 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. + +## 179. Undo is one level deep and has no Redo + +**Observed (user, from the notes):** "ctrl+Z should work like in any other +application, keep track of the past N actions and allow to undo them. This +would also bring in Redo, which would act as the opposite of Undo. As it is +today it feels hackish and half-implemented." + +**Cause, verified in the code.** The stack is real, not a single slot: +`MainWindow` owns a `QUndoStack` (`mainwindow.h:1434`) and every mutation +pushes a command onto it, so multi-level undo is already there and works. +What is missing is the other half of the facility around it: + +- **There is no `redo` action at all.** `KeyMap::knownActions()` lists `undo` + and nothing else (`keymap.cpp:69`), `Ctrl+Z` is its only binding + (`keymap.cpp:201`), and `QUndoStack::redo()` is never called from + `mainwindow.cpp`. The `redo()` overrides in the command classes exist only + because `QUndoStack` calls them on push; nothing reaches them a second time. +- **The stack is cleared on every new query** (`mainwindow.cpp:3458`), with a + correct reason recorded beside it: the entries invert model updates against + rows that are about to be discarded. That is what makes a stack that is N + deep behave like one that is one deep, since running a query is the ordinary + thing a user does between actions. +- No `setUndoLimit()` is set anywhere, so the depth is unbounded until a query + clears it. + +**Approach.** Two separable pieces, and the second is the real work. + +1. Add `redo` as an action: `KeyMap::knownActions()`, an icon, a menu entry, + and `Ctrl+Shift+Z` as the conventional default. Per CLAUDE.md that is five + places, four of them enforced by tests. Small and self-contained. +2. Decide what survives a query. The clear is not gratuitous, and removing it + without an answer reintroduces the half-applied undo it prevents: the + database changes and the list does not. The candidate answer is that a + command should be able to re-resolve its rows against the current model + rather than assuming the ones it was built with are still there, at which + point a query no longer invalidates the stack. + +**Constraints.** +- The undo stack is this application's substitute for a confirmation dialog, + per CLAUDE.md, so an undo that damages state is worse than the dialog it + replaces. Item 176 is exactly that failure and is one commit old. +- Redo re-applies a write to real mail. It carries the same requirement item + 176 established for undo: it must cover what the write actually CHANGED, not + what it asked for. +- `m_awaitingTagConfirmation` holds pointers the stack owns and is cleared + beside it (`mainwindow.cpp:3462`). Anything that changes the clear has to + keep those two in step or it is a dangling pointer, not a stale row. +- Sizing the second piece needs the first: `?` until the query-clear question + is answered. + +## 180. The repaint rules are discovered one hole at a time + +**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? I feel like we are chasing our own tail with the repaint +issue, we are effectively plugging holes, every time we have to stumble on a +view that doesn't refresh when it should, or worse, it refreshes when it +shouldn't." + +**Cause, verified in the code.** This is a maintenance item about a pattern, +not a defect with a reproduction. The observation is accurate and the history +is in this backlog: items 105, 107, 109, 110 and 170 are each one hole in the +same surface, every one found by hand-testing rather than by a test, and each +fixed in place. The current shape is three separate mechanisms that must agree +by hand: + +- the optimistic REPAINT, `applyTagChange` for a thread and + `applyMessageTagChange` for a message, which item 105 established must exist + in both scopes and item 107 established must reach loaded replies; +- the optimistic MEMBERSHIP, `MainWindow::syncViewMembership()`, added by item + 170 and called from three funnels (`mainwindow.cpp:5861`, `:7052`, and the + deferred path at `:3899`); +- the revert, `revertPendingTagChange()`, which must cover every scope the + other two apply. + +CLAUDE.md already carries the rule that keeps them in step ("Every path a +thread-scoped write travels, a message-scoped one travels too"), which is a +statement that the coupling is enforced by documentation and review rather +than by the code. + +**Approach.** Not a rewrite. The cheapest thing that would end the pattern is +a single test that asserts the invariant directly, rather than one test per +hole: for each scope and each funnel, a write leaves the model, the membership +and the revert path agreeing. That converts "we stumble on a view that does +not refresh" from a hand-test finding into a suite failure. + +**Constraints.** +- The deliberate lag documented in CLAUDE.md is NOT a hole and must not be + "fixed" by such a test: reading the last unread message of a long + conversation leaves the row in place because `applyMessageTagChange` leaves + a long thread's summary alone, and judging on a stale union would be wrong + in both directions. The invariant has to be written to permit it. +- Likewise the two deliberate asymmetries: a row is never evicted while it is + current, and an automatic write defers eviction where a requested one does + not. +- This needs the user to say whether they want the test or the refactor. The + note asks a question rather than reporting a fault, and the answer changes + the size from S to L. |
