summaryrefslogtreecommitdiffstats
path: root/docs/superpowers
diff options
context:
space:
mode:
Diffstat (limited to 'docs/superpowers')
-rw-r--r--docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md168
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