diff options
Diffstat (limited to 'docs')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 383 |
1 files changed, 381 insertions, 2 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 50d6c42..dccea2d 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 @@ -91,11 +91,11 @@ taking that too literally. | 32 | Esc does not blank the right pane | workflow | XS | **done** | | 33 | Status bar messages never expire | feedback | S | **done** | | 34 | No overview of the Maildir itself | information | M | **done** | -| 35 | No refresh of the thread list after a sync | workflow | M | open | +| 35 | No refresh of the thread list after a sync | workflow | M | **done** 2026-08-10; the list now follows a sync on its own | | 36 | `test_mainwindow` cannot reach the worker | testing | S | open, on demand | | 37 | The worker stalls on a tag edit made during a background sync | correctness | S | **done** | | 38 | `test_mainwindow` fails when a real sync holds the lock | testing | XS | **done** | -| 39 | Thread list cannot be sorted by clicking a column header | workflow | S | open | +| 39 | Thread list cannot be sorted by clicking a column header | workflow | S | **dropped** 2026-08-10; the card list has no column headers to click, and 0.13.0 shipped a sort dropdown instead | | 40 | No live filter over the current view | workflow | M | open | | 41 | A message whose HTML body carries a `Content-Id` renders blank | correctness | S | **done** | | 42 | "Syncing..." says nothing about what is being synced | feedback | S | **done** | @@ -118,6 +118,8 @@ taking that too literally. | 59 | Archive and Mark all read shipped with the same icon | presentation | XS | **done** | | 60 | Next thread dead-ends on the last reply of an expanded thread | defect | XS | **done**; already fixed by 5487d58, see below | | 61 | `test_mainwindow` fails intermittently, about 1 run in 20 | testing | S | open; predates the card list, reproduced on f72dba9 | +| 62 | No config option for the date format on a card | presentation | XS | open | +| 63 | No way to see sent mail, and no filter for it | workflow | S | open | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -2175,6 +2177,292 @@ from a current one, so the plumbing for a second concurrent query exists. - Scroll position likewise. - A refresh must not re-trigger mark-read for the thread already on screen. +### Refined by the user, 2026-08-10: new mail never appears at all + +The complaint is sharper than "the list is stale". Read every message in an +Unread view and the list empties. The cron sync then runs, `notmuch new` indexes +new mail, and **the new messages do not appear**, even though the view is empty, +nothing is selected, no thread is open and the undo stack has nothing in it. The +only way to see them is to re-run the query by hand. + +**Cause, verified in code, and it is the deliberate choice above meeting its +worst case.** The app does observe the cron sync: +`MainWindow::onExternalSyncStateChanged()` (`src/mainwindow.cpp:2140`) is driven +by `SyncMonitor`'s lock polling and reaches `State::Idle` when the run ends. At +`src/mainwindow.cpp:2180-2192` it deliberately shows "Background sync completed. +Press Enter in the query bar to refresh." and calls nothing. The comment there +gives the reason: `runCurrentQuery()` clears the undo stack, the selection and +the message pane, which is hostile to fire six times an hour under a reader. + +**Every one of those costs is zero in the case the user hit.** There is nothing +to clear: no undo entries, no selection, no open thread, and the scroll position +of an empty list is meaningless. So the guard is protecting state that does not +exist, and the result is an empty Unread view sitting in front of unread mail. + +**This makes the item shippable in two stages, and the first is XS.** Refresh on +external sync completion when the refresh is provably free: the model is empty +**or** the undo stack is empty and nothing is selected. Fall back to the current +status-bar message otherwise. That fixes the reported case immediately without +needing the reconciling refresh, which stays as the M-sized second stage for the +case where the user does have state worth keeping. + +**Constraint on the cheap stage.** "Nothing selected" must be read from the +selection model, not from `currentRowChanged` state, per `CLAUDE.md`. And the +condition has to be re-checked at the moment the sync ends rather than when it +started, since the user may have selected a row during the run. + +### Outcome (done, 2026-08-10) + +**35a was superseded before it shipped, by the user's own answer to it.** Asked +whether new mail would accumulate into a POPULATED view, the answer was no, and +the requirement was restated in full: "changes should be applied automatically, +without the user needing to refresh the view, whether it's empty or populated", +new mail appearing at the top as it is fetched, and the message being read not +disappearing. So the conditional refresh was removed rather than kept beside the +reconciling one, and `externalRefreshIsFree()` no longer exists. The "press +Enter in the query bar to refresh" message is gone with it: a refresh that +changes nothing is invisible, and one that brings mail announces itself by the +mail appearing. + +**The undo constraint this item was sized around turned out not to exist.** The +text above calls keeping the undo stack "the hard part" and the reason this is +M-sized. It is not hard, because no undo entry was ever keyed on a row: +`ThreadTagCommand` stores THREAD ids and `MessageTagCommand` stores MESSAGE ids +(`src/mainwindow.h`), both re-sending through `sendThreadTagChange()` / +`sendMessageTagChange()`, and `ThreadListModel::applyTagChange()` looks its +target up by id and does nothing when the row is absent. An entry therefore +already survives its rows leaving the view. `runCurrentQuery()` clears the stack +because a query the USER typed means "show me something else", not because the +entries would corrupt anything. + +**What was built.** + +- `ThreadListModel::reconcile()` diffs a result against the current rows by + thread id: arrivals inserted, departures removed, survivors keeping their row, + their persistent index and their loaded replies. Order comes from the result, + never from a rule of the model's own, so the sort the user selected is + respected: newest-first puts new mail at the top, oldest-first at the bottom. +- `MainWindow::refreshCurrentQuery()` re-runs `m_lastQuery` under its own + generation, accumulates every batch, and reconciles ONCE at the end. Batch by + batch would be wrong in a way that looks right: reconcile decides removals + from what the result lacks, so the first batch would delete every row after it + and the next would put some back. +- `onExternalSyncStateChanged()` calls it unconditionally on `Idle`. +- A stale-thread notice in `MessageView`, shaped like the remote-content bar as + the user asked, with recovery that runs `thread:<id>`, expands it and + re-selects the message that was on screen. + +**Four defects found by the tests, three of them real.** + +1. **The first test crashed the constructor.** `SyncMonitor::start()` polls + SYNCHRONOUSLY (`src/syncmonitor.cpp:52`), so on an idle lock file it emits + `stateChanged(Idle)` from inside `buildUi()`, while the view, the model and + the worker are all still null. The old code survived only because reporting + to the status bar touches nothing built later; the first handler to + dereference a widget segfaults before the window exists. +2. **A downward-move branch that could never run.** `reconcile()` walks the + result front to back, so rows ahead of the target are already final and a + misplaced survivor is always pulled FORWARD. The branch was written with the + usual `beginMoveRows` +1 adjustment and two mutation tests passed against it + being wrong, which is the signal that a probe is not measuring what it + claims. Deleted, with `Q_ASSERT(row > target)` recording the invariant. +3. **A user query hijacked by a pending recovery.** Recovery spans two queued + round-trips, so a query typed in the middle of one found its target in the + new result and moved the selection there. `runCurrentQuery()` now abandons a + pending recovery, and `recoverStaleThread()` sets its target after calling + it. +4. **The stale notice never fired for the reader deepest in a thread.** + Selecting a message row CLEARS `m_currentThreadId` and sets + `m_currentMessageId` instead, so a notice keyed on the thread alone was + silent for exactly the case the user described, reply four of eight. The + window remembers the message's thread separately; + `ThreadListModel::threadIdForMessage()` cannot help, since it searches the + rows and by then the thread has left them. + +**A fifth defect, found by the user in hand testing rather than by any test.** +The notice outlived the message it describes: running a new query blanked the +pane and left the bar above it, still naming the previous thread, with a button +offering to recover a thread the user had deliberately navigated away from. The +bar belongs to the rendered message exactly as the remote-content bar does, and +`MessageView::clear()` already hides that one for this precise reason; the new +bar simply was not added beside it. Fixed there, which covers all six paths that +blank the pane at once, rather than at the query path where it was noticed. + +Worth recording because the tests could not have caught it as written: every +one of them asserted that the notice APPEARS, and none that it goes away. A +feature's off-switch needs its own test, and "it shows up when it should" passes +identically whether or not it ever stops showing up. + +**A sixth defect, also found by the user in hand testing, and the same mistake +in a different place.** The status bar sat on "Background sync running..." with +no sync running. That string is written straight to the label when the lock +appears, and the "Background sync completed" message on the way out was the only +thing that ever replaced it; removing that message to make the refresh silent +left the claim standing indefinitely. + +The rule this establishes is worth more than the fix: **silent means saying +nothing NEW, not leaving a stale claim on screen.** The Idle branch now retires +its own running message and nothing else, tracked by a flag rather than by +matching the text, so it cannot overwrite a selection count or a tag result the +user is reading. Both directions are pinned by mutation: never retiring +reproduces the reported bug, and always writing the default stamps over the +selection message. + +Note the shape shared with the fifth defect above. Both are a piece of UI state +that outlived the thing it described, and in both cases the tests asserted only +that the state APPEARS. An "it goes away" test is a separate test. + +**A seventh and eighth defect, one report, and the worse of the two mutates +mail.** The user came back to the window from another desktop and found the new +message the refresh had brought in already OPEN in the pane, with the stale +notice above it still naming the four-message thread they had been reading. + +- **Nothing in `MainWindow` selected it.** `QTreeView` gives itself a current + index when it takes FOCUS with none set, and current is what drives loading. + Probed rather than assumed, because the obvious hypothesis is wrong: inserting + rows into an empty view does NOT set current, focusing the view does, which is + exactly why the report came with "as I go back to the window from another + desktop" attached. Before item 35b this was unreachable, since a populated + list always had a current row; a refresh dropping mail into a view the user + read empty created the state. The consequence is not cosmetic: opening a + message arms the mark-read timer, so a cron sync plus a window switch marked + mail read that nobody looked at. `onThreadSelected()` now requires the row to + be SELECTED, which every real route (click, arrow key, `selectRowAt`) does and + Qt's housekeeping does not. +- **The notice was correct when raised and became a lie underneath.** It named + the thread that was rendered; the auto-open then replaced the pane without + touching the bar. `MessageView::clear()` retires it, but selecting a row + RE-RENDERS rather than blanking, so that path never ran. Retired in + `onThreadSelected()` as well. + +The first of these was reported as one bug and is two, and only the second was +visible on screen. Worth remembering that "the wrong thing is displayed" and +"the wrong thing happened to the mail" can arrive in the same sentence. + +**A ninth defect: recovery brought the thread back collapsed and blank.** The +user reported it as minor and livable, and it was three faults stacked, each of +which alone would have produced roughly the symptom they saw. + +- **The notice threw away a message id it had.** A thread ROOT sets BOTH + `m_currentThreadId` and `m_currentMessageId`, because the root card is the + thread's first message and the pane renders exactly that message. The notice + read the message id only when the thread id was empty, treating it as the + message-row case, so opening a thread the ordinary way lost it and recovery + had nothing to reopen. +- **Recovery never expanded.** It selected the row and returned, so the + conversation the user asked to get back to was not on screen. It expands + first now, in every branch, which is also what asks the worker for the + replies. +- **A freshly queried root does not know its own first message either.** + `MessageIdRole` on a thread row returns `first.messageId`, which is empty + until the tree loads, so the root check could not match on the pass that + matters and the code fell through to `rowCount(thread) == 0` and returned, + selecting nothing. Recovery now selects the thread PROVISIONALLY on that + pass, without clearing the target, and refines to the exact reply when the + replies arrive. + +**One change here is not demonstrated and is recorded as such.** Recovery also +moved from `setCurrentIndex()` to `selectRowAt()`, on the reasoning that +`onThreadSelected()` ignores an unselected current index since the auto-open +fix. A mutation reverting it passes the whole suite: under +`ExtendedSelection`, `setCurrentIndex()` selects as a side effect, so the two +are indistinguishable here. It is kept as the honest expression of the intent, +not as a fix, and nothing should be claimed for it. + +**A tenth defect, and the only one in this item that six rounds of reasoning +failed to find: a dangling reference across a signal.** Recovery brought the +thread back collapsed with a blank pane. The user reported it three times, each +time after a fix that was aimed at the wrong thing. + +`MessageView` emitted `staleThreadRecoveryRequested(m_staleThreadId, +m_staleMessageId)`, passing its own members. The connection is direct, so +`MainWindow::recoverStaleThread()` received REFERENCES to those members. It then +called `runCurrentQuery()`, which blanks the pane, which calls `setStaleThread()` +and assigns to exactly those members. From that line onward the slot's own +parameters read as empty, so `m_recoverThreadId = threadId` stored an empty +string and `applyPendingRecovery()` returned at its first line, forever. The +thread was re-queried and expanded correctly, which is why the symptom looked +like a layout or expansion problem rather than a lifetime one. + +**Every existing recovery test passed against it, and could not have failed.** +They all reach the slot through `QMetaObject::invokeMethod`, which COPIES its +arguments; the reference never dangles under a test. The defect needed the real +button and the real signal, which is what the new test uses. + +**Six wrong mechanisms were proposed and rejected before the log named this +one**, each one plausible and each one disproved by a probe rather than by +argument: `QTreeView::expanded` not re-firing, expansion collapsing when +children arrive, the multi-row guard blanking the pane, `selectRowAt` not +clearing the previous selection, a stale `m_refreshGeneration` swallowing the +result, and `onQueryFinished` not running at all. The thing that ended it was +instrumenting the running application and reading `RECOVER target set to ` with +nothing after the `to`, which no amount of reading the code had produced. + +**The rule worth keeping: a Qt signal argument is a reference until something +copies it.** Emitting a member across a direct connection to a slot that can +reach back and modify that member is a use-after-write, and it presents as the +value being "wrong" rather than as a crash. Copy at the emit site when the slot +can plausibly re-enter the emitter. + +**A trap the recovery had to handle.** `setThreadMessages()` drops the depth-0 +message because the root card IS the thread's first message, so a reader +recovering from message one must land on the ROOT row. Looking for it among the +children finds nothing and leaves the selection nowhere. + +**Verification.** 105 tests in `test_mainwindow`, 56 in `test_threadlistmodel`, +17/17 binaries. Every reconcile test was mutation-checked: an unconditional +refresh, a skipped move, corrupted index bookkeeping (which trips +`QAbstractItemModelTester` fatally), a per-batch reconcile, a notice that never +fires and one that always fires are each caught by a named test. The +abandoned-recovery test initially passed for the wrong reason, because its +recovery never reached its target, and was rewritten until it failed against the +missing guard. + +**One measurement worth carrying to item 61.** During this work +`test_mainwindow` failed three runs in a row on the pair recorded there +(`anActionOnAMessageRowTagsThatMessageNotTheThread`, +`aSuccessfulCronSyncDrainsTheEditedAccounts`), then passed twelve consecutive +runs unchanged. The recorded rate is about one in twenty; a cluster of three +consecutive failures does not fit an independent one-in-twenty event and +suggests the trigger is a machine state that persists across runs rather than a +per-run race. + +### Outcome (35a, superseded by the above) + +`MainWindow::externalRefreshIsFree()` gates the `State::Idle` branch of +`onExternalSyncStateChanged()`: it refreshes when the undo stack is empty, the +selection model reports no selection, and the model holds no rows, and prints +the existing "press Enter" message otherwise. The M-sized reconciling refresh +(35b) is untouched and still open. + +**The test crashed the constructor, and the crash was real.** +`SyncMonitor::start()` polls SYNCHRONOUSLY (`src/syncmonitor.cpp:52`), so on a +machine whose lock file is idle it emits `stateChanged(Idle)` from inside +`buildUi()` (`src/mainwindow.cpp:535`), while `m_threadView` and `m_model` are +still null. The old code survived that only because reporting to the status bar +touches no widget built later; the first handler to dereference a view segfaults +before the window exists. `externalRefreshIsFree()` returns false on a null view +or model, which is also correct on the merits: the startup query has not run at +that point, so there is nothing to refresh. + +**The undo check is not redundant with the row check**, and that is asserted +rather than argued. Removing it alone leaves +`aCronSyncDoesNotRefreshOverPendingUndo` failing, because an empty model with +live undo entries is reachable by tagging the last thread out of the current +view. + +**Both guard tests passed before the fix existed**, since nothing refreshed at +all, so each was verified by mutation: an unconditional `return true` fails both, +and dropping the undo check fails the undo one. + +**One existing test changed, and the change is a narrowing.** +`aSkippedLocalSyncStillReportsTheOtherRunFinishing` asserted the words +"Background" in the status bar, and its fixture is an empty list with nothing +selected, so it now takes the refresh branch and the words never appear. Its +actual subject is that a handed-back lock is attributed to the other run rather +than swallowed, so it asserts the query generation instead. Pinning the wording +there would fail again the next time this decision is revisited. + ## 36. `test_mainwindow` cannot reach the worker **Observed:** twice in one session (0.8.0), a defect could not be given a @@ -2406,6 +2694,15 @@ line and `lockHeldIn()` correctly finds no lock in it. ## 39. Thread list cannot be sorted by clicking a column header +**Dropped 2026-08-10 (user).** The item asked for a column header to click and +there is no longer one to click: the card list (items 20 and 53) collapsed the +five columns into a single column of cards, and the header is gone with them. +0.13.0 shipped a sort dropdown in the query row, which is the same capability +reached a different way, so the complaint behind this item is answered and the +mechanism it proposed is unbuildable. The analysis below is kept because its +constraints outlived it: the batched-append problem and the sort-on-timestamp +rule apply to any future sort, including the dropdown's. + **Observed (user, 2026-08-05):** "left pane columns order by clicking on the column header." @@ -3751,6 +4048,88 @@ initially mistaken for a regression that change had introduced. **Size: S**, most of it in reproducing reliably rather than in the fix. +## 62. No config option for the date format on a card + +**Observed (user, from the notes):** "option in config file for date format". + +**Cause, verified in code.** `CardLayout::formatDate()` +(`src/cardlayout.cpp:24-30`) is a single unconditional line: +`QLocale::system().toString(date, QLocale::ShortFormat)`. It reads no setting, +takes no format argument, and is the only date formatter on a card +(`src/carddelegate.cpp:176` is its only caller besides +`CardLayout`'s own `widestDate`). `Config` parses no date key: the `[general]` +keys it reads are `notmuch_config`, `startup_query`, `message_zoom`, +`completion_on_focus`, `toolbar_icon_size`, `sync_on_exit` and +`mark_read_delay_ms` (`src/config.cpp:78-184`). So there is nothing to +configure, not a setting that is being ignored. + +The current behaviour is a deliberate choice rather than an oversight, and the +comment at `src/cardlayout.cpp:26-28` says why: the system short format is what +every other application on the desktop shows, and a mail client that disagrees +looks wrong. This item is about giving the user an override, not about +replacing that default. + +**Approach.** A `[general] date_format` key, empty by default meaning "the +system short format", otherwise a `QDateTime::toString()` pattern. +`CardLayout::formatDate()` takes the format as a parameter rather than reading +`Config` itself, keeping the struct free of dependencies the way it already is, +and `CardDelegate` passes it down. + +**Constraints.** + +- **`CardLayout::widestDate()` reserves the date column's width and must agree + with whatever the format produces**, or a long custom pattern is elided or + overlaps. It currently computes the widest string the short format can return; + with a custom pattern it has to measure that pattern instead. +- An unparseable or absurd pattern must not blank the date. `toString()` with a + pattern containing no field returns the pattern itself verbatim, so validate + in `Config` and fall back to the system format with a problem reported, the + way `message_zoom` and `toolbar_icon_size` already do. +- Document all three states in the README's `[general]` block: absent, empty, + and a pattern. + +**Size: XS.** One key, one parameter, one width calculation. + +## 63. No way to see sent mail, and no filter for it + +**Observed (user, from the notes):** "Sent mail filter". + +**Cause, verified in code, and it is not one missing feature but two.** + +- **Nothing in the codebase knows what "sent" means.** `Account` carries + `name`, `address`, `maildir`, `drafts`, `label`, `channel` and `color` + (`src/config.cpp:254-270`); there is no `sent` field, and no query anywhere + composes one. `Account::scopedQuery()` (`src/config.cpp:42-45`) scopes by + `path:"<maildir>/**"`, which covers a sent folder only in the sense that it + covers everything in the account. +- **The saved-query mechanism could express it today and nothing ships one.** + `[queries]` is read wholesale from `childKeys()` (`src/config.cpp:291-297`), + so a user can already write `Sent = tag:sent` by hand. The README's example + block (`README.md:178-181`) offers Inbox, Unread and Important and no Sent, so + nothing points the user at it. + +Which of the two this item is depends on a decision the notes do not make: +whether "sent" is a **notmuch tag** the user's own filters apply, or a +**maildir path** per account. If it is a tag, this is a documentation and +defaults change and it is XS. If it is a path, `Account` needs a `sent` key +beside `drafts`, and the query has to be composed per account, which is where +the S comes from. + +**Approach, pending that decision.** Ask the user first which their setup +already produces. Their mail is filtered outside qtmaildir (`assets/mailsync.sh` +is `mbsync` plus `notmuch new`), so the answer is a property of their existing +filters, not something to design here. + +**Constraints.** + +- **Do not invent a tag qtmaildir applies itself.** v1 is read-and-organize; + nothing here sends mail, so nothing here can know a message was sent except by + where it landed or what tagged it. +- If it becomes a per-account key, it composes with `scopedQuery()` and must not + bypass it, or a Sent view in one account shows another account's mail. + +**Size: S**, and XS if the answer is "it is a tag". + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering |