diff options
| -rw-r--r-- | CLAUDE.md | 20 | ||||
| -rw-r--r-- | assets/images/qtmaildir-dark.png | bin | 0 -> 61700 bytes | |||
| -rw-r--r-- | assets/images/qtmaildir-light.png | bin | 0 -> 87063 bytes | |||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 332 |
4 files changed, 351 insertions, 1 deletions
@@ -169,6 +169,26 @@ loosen any of these without an explicit decision. against the chosen directory, and refuse anything escaping it. Compare resolved paths as paths, not with `startsWith` — `/tmp/safe-evil` passes a `startsWith("/tmp/safe")` check. +## At the start of a session: reconcile the backlog with the user's notes + +The backlog at `docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md` is +**downstream** of the user's own notes at +`~/Documents/Obsidian/note/notes on qtmaildir.md`. The user writes to those notes +whenever they use the application and hit something, so the backlog goes stale on +its own between sessions. + +**Read both and diff them before picking up work.** Anything in the notes with no +item in the backlog gets appended with the next free number, in the backlog's +own format (Observed / Cause / Approach / Constraints), with the cause **verified +in the code, not copied from the note**. The two documents are numbered +independently and drifted long ago; never renumber to reconcile them. + +This is not busywork. The 2026-08-04 pass found nine unrecorded entries, two of +them defects rather than enhancements, and one of those was a constraint this +backlog had already specified and that shipped unbuilt (item 29). A note saying +"X does not work" is a bug report, and it will sit in a personal notes file +indefinitely unless someone goes looking. + ## Working on this repo Implementation follows `docs/superpowers/plans/2026-08-02-qtmaildir-v1.md` (14 tasks) diff --git a/assets/images/qtmaildir-dark.png b/assets/images/qtmaildir-dark.png Binary files differnew file mode 100644 index 0000000..d380a75 --- /dev/null +++ b/assets/images/qtmaildir-dark.png diff --git a/assets/images/qtmaildir-light.png b/assets/images/qtmaildir-light.png Binary files differnew file mode 100644 index 0000000..43af85d --- /dev/null +++ b/assets/images/qtmaildir-light.png 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 277283e..4dab8a9 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 @@ -13,6 +13,12 @@ those notes called 12 is item 13 here, and item 14 here (the tag column) was never in them at all. Items 15 to 17 come from a later pass over the same notes. Cite these numbers, not the notes', and do not renumber to reconcile. +**The notes are the upstream source and they keep growing.** The user adds to +them while using the app, so this document goes stale on its own. Items 28 to 35 +came from one such pass on 2026-08-04 and included two defects that had gone +unrecorded here for a while. Compare the two at the start of a session; the +procedure is in `CLAUDE.md`. + Numbering is stable. New items append with the next free number and never renumber, so a note referring to "item 7" keeps meaning the same thing. An item that is dropped stays in the table marked `dropped` with a one-line reason. @@ -65,7 +71,15 @@ taking that too literally. | 24 | No right-click actions on the thread list | discoverability | S | open | | 25 | No select-all, and bulk actions are undiscoverable | workflow | S | open | | 26 | No way to add or remove an arbitrary tag from the UI | workflow | S | **done** | -| 27 | The UI cannot see a sync it did not start | feedback | S | open | +| 27 | The UI cannot see a sync it did not start | feedback | S | **done** | +| 28 | Re-adding `unread` counts 2 unsynced changes, not 0 | correctness | S | open | +| 29 | Sync button stays enabled during a background sync | feedback | XS | open | +| 30 | The blank right pane is wasted space | presentation | M | open | +| 31 | The quit prompt has no highlighted default button | discoverability | XS | open, needs repro | +| 32 | Esc does not blank the right pane | workflow | XS | open | +| 33 | Status bar messages never expire | feedback | S | open | +| 34 | No overview of the Maildir itself | information | M | open | +| 35 | No refresh of the thread list after a sync | workflow | M | open | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1260,8 +1274,324 @@ project has been bitten by twice:** Sync button should be disabled while the lock is held, since starting one would only produce the `EX_TEMPFAIL` skip. +### Outcome (done, 0.8.0) + +Both probes the item demanded were run before any code, and one of them changed +the design. **The lock is observable, but only through `/proc/locks`**, and two +of the three plausible ways to read it are wrong: + +- **`flock -n`, which this item proposed, is the one that must not be used.** + It acquires in order to test, so polling every two seconds opens a window + every two seconds in which a starting `mailsync.sh` is refused the lock and + exits 75. It would cause the very skips the script reports. The item's own + approach section specified this; it was wrong. +- `fcntl(F_OFD_GETLK)` never acquires and looks ideal, but reports UNLOCKED + against a lock held by `flock(2)`: separate lock namespaces in the kernel, + which cannot see each other. A silent false negative. +- `/proc/locks` is a pure read, matched by inode. It observes `flock(2)` + correctly and takes no lock, which also answers the second probe: 200 reads + left the lock table unchanged and the polling process holding nothing, so it + cannot contend with the Xapian write lock `notmuch new` holds in the same run. + +`SyncMonitor` keeps the parsing separate from the polling so the parsing is +testable, and reports **Unknown** rather than Idle where `/proc/locks` cannot be +read. "No sync is running" is the claim that would let the window quit, so it is +never guessed. + +**It reports rather than refreshes**, which is narrower than the item implies. +`runCurrentQuery()` clears the undo stack, the selection and the message pane: +right for a query the user typed, hostile for one a cron timer fired six times +an hour. The status bar says a background sync finished and suggests pressing +Enter. See item 35 for the non-destructive refresh this defers. + +**One constraint above was not built: the Sync button is still enabled while an +external sync holds the lock.** That is item 29, split out rather than left +buried here. + +**A defect found by hand testing, after the feature was believed done.** A sync +this window started reported itself as a background one, a moment after it +finished. Ownership of a lock period was decided when the lock was RELEASED, by +asking `MailSync::isRunning()`, but the process exits before the next poll sees +the lock gone, so the answer was always "not ours". Ownership is now latched +when the lock appears, and handed back on exit 75, since a skip means the lock +was never ours. + +**That fix has no regression test, deliberately recorded.** Reproducing it needs +`isRunning()` true at one transition and false at the next, which needs a +configured sync command and a live child process; that attempt left a process +running for the length of the suite and popped a dialog on the user's screen. It +was verified against a standalone model of both code paths instead. A real test +needs the notmuch fixture. + --- +## 28. Re-adding `unread` counts 2 unsynced changes, not 0 + +**Observed (user, 2026-08-04):** open a thread, wait the two seconds for the +automatic mark-read, then press **Ctrl+U** to put `unread` back. The indicator +reads **2 unsynced changes**. The mail store is in exactly the state it started +in, so the honest answer is 0. + +**Cause: the counter counts writes, not net state.** `onTagsApplied()` does a +bare `++m_pendingEdits` (`src/mainwindow.cpp:1293`) for every confirmed +mutation, and nothing ever decrements. The automatic mark-read is one confirmed +write, the manual toggle back is a second, so a round trip that cancels itself +out reads as two outstanding changes. + +This is not specific to mark-read. Any add-then-remove of the same tag inflates +it the same way: archive then undo, flag then unflag. Mark-read is simply the +one path that fires without the user asking, which is why it surfaced here. + +**The design question this item has to answer first.** "Unsynced changes" can +mean two different things and the fix depends on which: + +- *Writes the index has taken that a sync has not carried over.* Two writes did + happen, notmuch's database did change twice, and `notmuch new` will still + visit those messages. On this reading 2 is correct and the wording is what is + wrong. +- *Net difference between the index and the mail store.* On this reading the + answer is 0 and the counter needs to track state, not events. + +The user's expectation is clearly the second. Note that item 18 chose the first +deliberately: it counts at the point a write is **confirmed**, precisely so an +optimistic update the worker rejects is not counted. That reasoning stays valid; +what it did not consider is a write that undoes an earlier one. + +**Approach, if net state is the answer.** Track the set of (thread, tag) +changes rather than a count, and collapse a pair that cancels. `TagChange` +already carries add and remove lists and already knows how to invert itself +(`TagChange::inverted()`, used by the undo stack), so the machinery for +recognising an inverse exists. + +**Constraints.** + +- **A sync must still reset it to zero**, and a failed sync must still not, per + item 18. Whatever replaces the counter keeps both properties. +- The indicator is a lower bound on confidence, not a guarantee: an external + `notmuch new` can carry changes across without this window knowing. Do not let + a more precise counter imply more certainty than it has. +- Do not fix this by not counting the automatic mark-read. It is a real write to + the index, and hiding it would make the count wrong in the other direction. + +## 29. Sync button stays enabled during a background sync + +**Observed (user, 2026-08-04):** while a cron sync runs, the Sync button is +still clickable. Pressing it starts a run that can only be refused. + +**Cause: a constraint of item 27 that was specified and then not built.** That +item states plainly that "the Sync button should be disabled while the lock is +held, since starting one would only produce the `EX_TEMPFAIL` skip". +`setSyncBusy()` (`src/mainwindow.cpp:1357`) is the only thing that touches +`m_syncButton->setEnabled()`, and it is called only from this window's own sync +path. `onExternalSyncStateChanged()` shows the progress bar and writes the +status text but never touches the button. + +**Approach.** Have the external handler drive the same enable/disable that +`setSyncBusy()` does, rather than duplicating the rule. The two paths already +share the progress bar; the button is the piece that was missed. + +**Constraints.** + +- **Re-enable on `Unknown`, not only on `Idle`.** Where `/proc/locks` cannot be + read the monitor claims nothing, and a button left permanently disabled on a + platform that cannot observe the lock is worse than one that occasionally + offers a run that gets skipped. +- The button must not end up enabled during this window's own sync because the + external handler ran last. Both paths write the same widget, so whichever + fires second wins; make the rule a single function of both states rather than + two independent assignments. +- Exit 75 handling stays. Disabling the button makes the skip rarer, not + impossible: cron can take the lock between the poll and the click. + +## 30. The blank right pane is wasted space + +**Observed (user, 2026-08-04):** with no thread selected the message pane is +empty. The user wants it to carry something useful, and named three things: the +application logo, tips and tricks, and messages about the current action, giving +"N messages selected" as the example. + +**Assets exist.** The user has produced `assets/images/qtmaildir-light.png` and +`qtmaildir-dark.png` (924x540 each) for this item. + +**Cause:** `MessageView::clear()` leaves the web view showing nothing. Since +0.8.0 the pane is also blanked deliberately whenever more than one thread is +selected, which makes this item more visible than it was: multi-select is now a +routine gesture that produces an empty pane every time. + +**Approach.** A placeholder document rendered into the existing web view when +there is nothing to show, rather than a second widget stacked behind it. +`HtmlBuilder` already produces the pane's HTML and `MessageView` already knows +how to hand it a document, so the placeholder is a third document shape +alongside "one message" and "a thread". + +**The selection-count half is the one with real value**, and it pairs with the +status-bar count added in 0.8.0. The pane already blanks on a multi-row +selection; saying "3 threads selected" there answers the question the blank +raises. + +**Constraints.** + +- **Two images, because the pane must follow the desktop theme.** Item 5 already + records the rule against hardcoded colours, and a light-theme logo on a dark + desktop is exactly that fault in image form. Pick by palette, not by guessing. +- **Do not load the logo over `file:`.** The web view runs with + `LocalContentCanAccessFileUrls` false and an interceptor that blocks every + request by default, per `CLAUDE.md`. The image has to arrive as a `data:` URI + or through the existing `qtmaildir:` scheme handler, and the interceptor's + document-URL exemption must still match exactly. +- Tips and tricks is the weakest of the three and should be built last, if at + all. A tip nobody can dismiss becomes noise on the hundredth launch. +- The placeholder must not appear between a selection and its render, or every + thread open flashes a logo first. + +## 31. The quit prompt has no highlighted default button + +**Observed (user, 2026-08-04):** "quit popup has no Predefined answer (there's +no highlighted button)." + +**Needs a repro before any change: the code says otherwise.** The three-button +prompt sets one explicitly, `box.setDefaultButton(sync)` +(`src/mainwindow.cpp:191`), and the no-sync-command variant passes +`QMessageBox::Cancel` as its default argument (`:167`). Both should render a +highlighted button. + +Possible explanations, in the order worth checking: + +- The dialog the user saw was neither of those. There are five other + `QMessageBox` calls in `MainWindow` (`:205`, `:224`, `:991`, `:1260`, + `:1277`), and the two sync-failure ones appear on the exit path, so a failed + exit-sync shows a second dialog immediately after the first. +- The default is set but the style draws no visible focus ring, which is a + platform theme question rather than a code one. +- `setDefaultButton()` is being overridden by the button roles: a + `DestructiveRole` button can take precedence in some styles. + +**Approach: reproduce first, and record which dialog.** If it is one of the +sync-failure boxes, the fix is to give those an explicit default. If it is a +theme issue the item becomes a documentation note rather than a change. + +**Constraint:** whatever default is chosen must be the safe one. On a prompt +about losing unsynced work, Enter must not fall on "Quit anyway". + +## 32. Esc does not blank the right pane + +**Observed (user, 2026-08-04):** "Esc in the main window should blank the right +pane." + +**Cause:** nothing binds Escape at window level. `Key_Escape` appears once in +`src/`, in `QueryCompleter` (`src/querycompleter.cpp:474`), where it dismisses +the completion popup. The main window has no handler. + +**Approach.** A registered action like any other, so it reaches the menus, the +shortcut reference and `[keys]`, calling the same `MessageView::clear()` the +multi-select path already uses. `m_currentThreadId` must be cleared with it, or +a late-arriving `threadLoaded` will paint the thread straight back, which is the +race documented in `CLAUDE.md` and fixed in 0.8.0. + +**Constraints.** + +- **Escape must not be stolen from the completer.** A window-level shortcut + outranks the focused widget, which is exactly how `Return` broke for the query + bar and needed an event filter to claim back (item 21 records this). Verify + the popup still dismisses before shipping. +- Blanking is a view change, not a mail change: it must not clear the selection, + the query, or the undo stack. +- Decide what Escape does when the pane is already blank. Doing nothing is fine; + clearing the selection as a second step would be surprising. + +## 33. Status bar messages never expire + +**Observed (user, 2026-08-04):** "the status bar should return to default status +after showing a message for N seconds." + +**Cause:** every message is written with a bare `m_statusLabel->setText()`, +eighteen call sites in `MainWindow`, and nothing ever clears one. Whatever was +written last stays until something else overwrites it, so a transient message +like "Sync complete" persists as though it described the current state. + +**Approach.** Route transient messages through one helper that sets the text and +arms a single-shot timer to restore a default, rather than adding a timer per +call site. `QStatusBar::showMessage()` already implements exactly this with a +timeout argument, and the label is a custom widget added with `addWidget()` +rather than the status bar's own message area, so switching to it is worth +considering before writing a bespoke timer. + +**The item's real question is what "default" means.** Candidates: the thread +count from the last query (`onQueryFinished` already writes this), or empty. +The count is more useful and is what the user already sees after a query +completes. + +**Constraints.** + +- **Not every message is transient.** The selection count added in 0.8.0 + describes current state and must persist while the selection does; expiring it + would be a regression. Distinguish state from events rather than putting a + timeout on everything. +- The 0.8.0 selection-count code already takes back only a message it wrote + itself, comparing against `m_selectionMessage`. Any general mechanism should + generalise that rather than defeat it. +- An error must not vanish before it is read. Sync failures already open the log + pane, which does persist, but the status text should outlast a two-second + timeout. + +## 34. No overview of the Maildir itself + +**Observed (user, 2026-08-04):** wants "info on the maildir": total messages, +number of accounts, and possibly more. + +**Cause:** nothing in the UI reports database-level facts. Every query returns a +thread count for that query (`onQueryFinished`), but there is no path that asks +notmuch about the database as a whole. + +**Approach.** A dialog, reached from Help or File, showing what notmuch can +answer cheaply. `notmuch_database_get_all_tags()` is already wired for the +completer (item 17), so the tag count is free. A total message count needs a new +worker call. + +**Constraints.** + +- **This is a worker query like any other.** No `notmuch_*` pointer crosses the + thread boundary; the result comes back as plain values, per `CLAUDE.md`. +- **The account count comes from config, not from notmuch.** notmuch does not + model accounts at all, which is why per-account subdirectories are configured + in the first place. Do not try to derive it from the database. +- Counting every message in a large database is not free. Measure before putting + it somewhere that opens on every launch; a dialog the user asks for is the + right shape, a status-bar field refreshed continuously is not. + +## 35. No refresh of the thread list after a sync + +**Observed (user, 2026-08-04):** "auto refresh list after sync." + +**Cause: half of this works and the other half was deliberately not built.** A +sync this window starts already refreshes: `onSyncFinished()` calls +`runCurrentQuery()` on success. A background sync does not, and 0.8.0 chose that +on purpose, because `runCurrentQuery()` clears the undo stack +(`src/mainwindow.cpp`, the query-start path), the selection and the message +pane. Firing it when a cron timer finishes would discard undo history and close +the thread being read, up to six times an hour, with no action from the user. +The status bar suggests pressing Enter instead. + +**So the work is a non-destructive refresh**, not a call to the existing one. +That is a real piece of work and is why this is its own item rather than a flag. + +**Approach.** Re-run the query and reconcile the result against the current +model instead of clearing it: keep rows that are still present, add new ones, +remove the gone. The generation counter already distinguishes a stale result +from a current one, so the plumbing for a second concurrent query exists. + +**Constraints.** + +- **The undo stack is cleared on query for a real reason.** Its entries refer to + rows the new result set discards, and a stale entry inverts into a model + update that does nothing while the database change still happens, leaving undo + half-applied. A refresh that keeps the stack has to keep those references + valid, which is the hard part of this item and the reason it is sized M. +- The selected thread must stay selected and stay open if it survives the + refresh. Losing your place is the failure this item exists to avoid. +- Scroll position likewise. +- A refresh must not re-trigger mark-read for the thread already on screen. + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering |
