diff options
Diffstat (limited to 'docs/superpowers/plans')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 168 |
1 files changed, 168 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 e82c8fc..cefdfc3 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 @@ -97,6 +97,10 @@ taking that too literally. | 46 | `uiStateSurvivesARestart` fails under the offscreen platform | testing | XS | **done** | | 47 | The query bar looks unfinished, and cannot be cleared by mouse | presentation | XS | **done** | | 48 | Removing a tag suggests every tag, not the thread's own | workflow | XS | **done** | +| 49 | Sync runs every account regardless of what changed | workflow | M | open | +| 50 | Esc blanks the pane but leaves the row selected | workflow | XS | open | +| 51 | Clicking a subject scrolls the list sideways | presentation | XS | open | +| 52 | `test_querycompleter` fails under Wayland, passes offscreen | testing | XS | open | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -2774,6 +2778,170 @@ assert the remove field's completer offers only those. The keys must be typed, not `setText()`, since `setText` does not drive a completer at all, which `CLAUDE.md` records. +## 49. Sync always runs every account, even when one account was touched + +**Observed (user, 2026-08-07):** "make the manual and closing sync operation +work only on affected accounts (it now runs for all accounts, independent from +actual modifications present). If no modifications exists, assume it's for all +accounts and run normally." + +**Cause (verified in code):** the account set is not a parameter anywhere on the +path. `MailSync::start()` (`src/mailsync.cpp:170`) takes no arguments; it splits +the configured `sync_command` string and runs it verbatim +(`src/mailsync.cpp:177-184`), so the command line is fixed at config time and +identical for every run. The shipped script then hardcodes the whole-store +sweep: `mbsync -V -a` (`assets/mailsync.sh:83`), where `-a` means all channels. +Nothing between the tag edit and mbsync carries which account changed. + +**Approach.** Three pieces, and the first is the real work. + +- **Track which accounts have unsynced edits.** Item 18's `m_pendingEdits` is a + bare count; this needs the set of account tags behind it, accumulated in the + same `onTagsApplied()` handler and cleared on the same successful-sync reset. + `ThreadSummary` already carries the account tag that `ThreadListModel` renders + as the account chip, so the mapping exists and needs no worker query. +- **Let `start()` take accounts.** An optional `QStringList` appended to the + configured command's arguments, empty meaning today's behaviour. +- **Let the script accept channel names.** `mailsync.sh` passes them to mbsync in + place of `-a` when given, keeping `-a` when not. + +**Constraints.** + +- **Empty set means all accounts, per the user's own wording.** A sync with + nothing pending is a fetch, and fetching one account because that is where the + last edit happened to be would be wrong. +- **A notmuch account tag is not necessarily an mbsync channel name.** The + config already separates the two ideas (`[account.<key>]` has a notmuch tag and + a display `label`); mapping to a channel needs either a new per-account key or + an explicit decision that the key IS the channel. Settle this with the user + before building, it is the one design question in the item. +- **`notmuch new` still runs over everything**, and must. Restricting the fetch + does not restrict the index. +- **The script's two shipped properties survive**: it prints to stdout as well as + its log, and it exits with the real status. `CLAUDE.md` records why. +- Item 42's status-bar parsing reads the account name out of `mbsync -V` output; + a narrowed run must keep `-V` or that regresses. + +**Verification:** by hand, since it ends in a real fetch. Tag in one account, +sync, and confirm from the log that only that channel ran. Then sync with +nothing pending and confirm all channels run. + +## 50. Esc blanks the pane but leaves the row selected + +**Observed (user, 2026-08-07):** "esc in the main window should deselect whatever +is selected in the left pane while still blanking the right pane. Two actions +instead of one." + +**Cause (verified in code):** deliberate, and now reconsidered. The `clear_pane` +action (`src/mainwindow.cpp:772`) is bound to `Esc` by default +(`src/keymap.cpp:88`), and its own comment states the intent: "A view change, +not a mail change: the selection, the query and the undo stack are all left +alone." It calls `m_messageView->clear()` and drops `m_currentThreadId` +(`src/mainwindow.cpp:783`) and touches the selection model not at all. Item 32 +built exactly what was asked for then; the user now wants the selection cleared +as well. + +**Approach.** The note asks for "two actions instead of one", so this is not a +matter of adding `clearSelection()` to the existing one: + +- `clear_pane` keeps its current behaviour and its name, still available to a + user who binds it. +- A second action clears the pane AND the thread list selection, and takes the + `Esc` default. + +**Constraints.** + +- **Clearing the selection must not resurrect the pane.** The selection handler + reacts to `selectionChanged`, so clearing it fires that path; confirm it + leaves the pane blank rather than re-rendering or repainting a placeholder, + and order the two operations accordingly. This is the whole risk in the item. +- **Interacts with item 30.** Once the blank pane shows a placeholder, "blank" + means "show the placeholder", and a multi-selection message is one of the + things item 30 specifies the placeholder saying. Build 30 first or the two + will disagree about what an empty pane looks like. +- The in-flight-load guard must keep working: `m_currentThreadId` is cleared + with the pane precisely so a `threadLoaded` arriving afterwards is dropped. +- Both actions need entries in the shortcut reference, which is generated, so + the descriptions must distinguish them in one line. + +## 51. Clicking a subject scrolls the list sideways + +**Observed (user, 2026-08-07):** "when selecting a row by clicking on the subject, +the table scrolls horizontally to accomodate the whole subject column into view. +Minor UX." + +**Cause (verified in code):** `QAbstractItemView`'s auto-scroll, which is on by +default and scrolls the clicked index fully into view. The Subject column +stretches and holds long text, so the view has somewhere to scroll to; +`src/mainwindow.cpp:548` sets `setHorizontalScrollMode(ScrollPerPixel)`, which +makes the movement smooth rather than by whole columns but does not cause it. +Nothing calls `scrollTo()` explicitly, and `setAutoScroll` is not set anywhere. + +**Approach.** Cheapest first, and check it is enough before going further: +`setAutoScroll(false)` on the thread view suppresses the scroll-into-view on +click. If that proves too blunt, override `scrollTo()` to ignore the horizontal +component and defer to the base class for the vertical one. + +**Constraints.** + +- **Keyboard navigation must still scroll vertically.** Arrowing past the bottom + of the viewport has to follow the current row, and `setAutoScroll(false)` is + the flag that governs that too. Verify with the keyboard, not only the mouse; + if it breaks, the `scrollTo()` override is the answer rather than the flag. +- Drag-select auto-scroll rides on the same flag. Selecting past the edge of the + viewport is a real gesture on a list this long. +- The item is cosmetic and must not grow into column-sizing work. The Subject + column being wider than the viewport is the precondition, not the defect. + +## 52. `test_querycompleter` fails under Wayland, passes offscreen + +**Observed 2026-08-07**, while verifying an unrelated change: `ctest` reports +`theDescriptionSurvivesAModestPopupWidth` failing, while running the same binary +directly passes. Reproduced on a clean checkout, so it predates that work. + +**Cause (verified):** the test grabs the completer popup and asserts the image +is not null (`tests/test_querycompleter.cpp:824`). Under the Wayland platform +plugin the grab returns a null pixmap, with the warning + +``` +qt.qpa.wayland: Failed to create grabbing popup. Ensure popup ... has a +transientParent set and that parent window has received input. +``` + +Wayland will not create a grabbing popup for a window that has never received +input, which a test window has not. `ctest` sets no `QT_QPA_PLATFORM`, so it +inherits the session's Wayland plugin, whereas a developer running the binary +by hand usually exports `offscreen` and never sees it. + +**Why this matters more than one red test.** The suite's result depends on how +it is invoked. A test that only passes under a platform plugin nobody sets is +not protecting anything, and worse, it trains you to read a real failure as +environmental. It cost a wrong diagnosis on the day it was found: the failure +was initially attributed to the change in flight, because the clean-tree +comparison was run under `offscreen` while `ctest` ran under Wayland. Comparing +two environments and reading the difference as a regression is exactly the +mistake this item exists to stop repeating. + +**Approach.** Pin the platform for the tests that need one rather than for all +of them, the same shape as item 46: + +- `set_tests_properties(... ENVIRONMENT QT_QPA_PLATFORM=offscreen)` for the + tests that grab widgets, so `ctest` is deterministic however the session is + configured. +- Preferable if it works: give the popup a `transientParent` and show the parent + first, which fixes the grab under Wayland rather than avoiding it. Try this + before reaching for the environment override, since a test that can run under + the real plugin is worth more than one that opts out. + +**Constraints.** + +- **Do not simply delete the assertion.** It exists because a null grab is + precisely how this class of rendering test silently passes, which `CLAUDE.md` + records at length. Weakening it to `if (!shot.isNull())` would make it pass + everywhere and check nothing. +- Whatever is chosen must hold for `test_mainwindow` and `test_messageview` too, + which create widgets and could grow the same dependency. + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering |
