diff options
Diffstat (limited to 'docs')
3 files changed, 474 insertions, 7 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 01be5d0..e008641 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 @@ -87,7 +87,7 @@ taking that too literally. | 18 | No visual cue that there are unsynced edits | feedback | S | **done** | | 19 | No prompt to sync on exit when edits are pending | behavior | S | **done** | | 20 | Thread view does not match the user's mental model | presentation | L | **done** 2026-08-10, as the card list; see 53 | -| 21 | Default shortcuts are not sensible enough | discoverability | S | open | +| 21 | Default shortcuts are not sensible enough | discoverability | S | open; **the user is drafting the table** in their own notes (`qtmaildir shortcuts and menu structure.md`), 2026-08-23. Read it first rather than proposing one. Settles `Ctrl+Return` for Send; leaves two collisions and an unfinished menu half, see the entry | | 22 | Translatability audit and i18n wiring | correctness | M | **done** 2026-08-15, unreleased; see `specs/2026-08-15-i18n-design.md`. Found eight rule-builder labels that could never be translated in any language, and twenty untranslatable warnings. Ships an Italian translation of all 355 strings | | 23 | No way to save a search query from the UI | workflow | M | **done** 2026-08-13, shipped in 0.18.0; see `specs/2026-08-13-saved-queries-design.md` | | 24 | No right-click actions on the thread list | discoverability | S | **done** | @@ -188,8 +188,8 @@ taking that too literally. | 119 | The unsynced-changes count cannot be opened to see what it counts | information | S | open, 2026-08-19, from the notes. One of the four things it sums carries no message ids at all, so a list cannot be complete without a change to how the count is kept | | 121 | The thread list shows nothing while a query is running | feedback | S | open, 2026-08-20, from the notes. Follows item 74, which fixed the status-bar half and left the list itself blank | -| 122 | The README documents a version of the app that no longer exists | documentation | M | open, 2026-08-20, from the notes. Delete-to-trash is entirely undocumented, including a config key a user must now set | -| 123 | Sending mail is not designed | v2 | L | **specified** 2026-08-20, on branch `compose-and-send`. Design in `docs/superpowers/specs/2026-08-20-compose-and-send-design.md`; read that, not this row. Send is a per-account `send_command` on stdin, so the no-network-protocol rule stands. Composer is a separate window, body is markdown via cmark-gfm, drafts autosave to the account's drafts folder. No code written | +| 122 | The README documents a version of the app that no longer exists | documentation | M | **done** 2026-08-23, unreleased, inside item 123 task 13. `trash`, `send_command` and the whole `[compose]` section were undocumented; a Composing section is added and "sending is not implemented" removed. Every default was read from `config.h` rather than from the prose, which caught `send_html` documented as false when it defaults to true | +| 123 | Sending mail is not designed | v2 | L | **specified** 2026-08-20, on branch `compose-and-send`. Design in `docs/superpowers/specs/2026-08-20-compose-and-send-design.md`; read that, not this row. Send is a per-account `send_command` on stdin, so the no-network-protocol rule stands. Composer is a separate window, body is markdown via cmark-gfm, drafts autosave to the account's drafts folder. Tasks 1 to 13 of 13 built 2026-08-20 to 2026-08-23; task 13 closed the documentation out and retired the v1/v2 split, which semver had made meaningless. **Hand tested 2026-08-22 and 2026-08-23** against a fake send command: New, Reply and Forward all send, a forwarded attachment survives intact, and the sent copy is filed. Found two defects, both fixed (the orphaned composer, and sent mail carrying `inbox`). Twenty-two defects were found in the plan's own draft code across tasks 4 to 12, so treat every code block in it as a draft | | 124 | The worker reads the index directory as the mail root | defect | S | **done** 2026-08-20, unreleased. `mailRootOf()` over `NOTMUCH_CONFIG_MAIL_ROOT`, correct under both layouts. Verified by migrating the developer's own index to NVMe the same day: cold start 38.6 s to 0.67 s | @@ -204,6 +204,20 @@ taking that too literally. | 132 | Every action must have a shortcut, and that no longer serves | policy | S | done, 2026-08-20. `everyActionHasAShortcut` is deleted and nothing replaces it: `everyActionIsReachableFromAMenu()` is the required rule and a shortcut is now a chosen subset. Nothing else needed changing, since `showShortcutReference()` already printed `(unbound)` for an empty sequence. Verified by unbinding `tag_rules` and running the suite green, which would have failed before | | 133 | The composer shows no markdown syntax highlighting | v2 | S | open, 2026-08-20, from the item 123 brainstorm. **Blocked on 123.** A `QSyntaxHighlighter` over the composer's editor, so `**bold**` reads as bold while the buffer stays plain markdown. Standard Qt, no dependency. Deliberately after 123's formatting toolbar: agreeing with the grammar about nesting and about code spans suppressing what is inside them is the expensive part, and the toolbar is what makes the feature usable | | 134 | The busy indicator is built inline and is about to be built twice | maintenance | S | done, 2026-08-20, af902e0. `BusyIndicator` (`src/busyindicator.h`) carries both modes: `MainWindow` uses the indeterminate one, and item 123's send popup takes the determinate half for its undo countdown, switching the same widget over when the command starts. Only the BAR was extracted, not the status label this row paired with it. `m_statusLabel` has 34 uses across `MainWindow` for transient messages, selection counts and sync phases, so it belongs to the window rather than to the indicator, and the send popup owns its own phase text | +| 135 | The formatting toolbar's buttons stack rather than toggle | v2 | S | open, 2026-08-21, asked for by the user during item 123 task 8 and reverted the same session. **A spec change, not a defect**: it conflicts with spec:236 ("deliberately no live toggle") and spec:187-190. Both sites need amending FIRST, and the amendment must resolve what replaces bold-then-italic, which is the gesture spec:187's preserved selection exists to serve and which a toggle makes unreachable. That question is the work; the state machine is understood and written up in the section | +| 136 | `undoMovesTheMessageBack` fails about one run in six | defect | ? | open, 2026-08-21, found while running the suite during item 123 task 10. A pre-existing race in the test or in Delete's file move, NOT caused by 123: reproduced on a clean tree with the branch's work stashed out, 1 failure in 6 runs, and the failing run took 70s against a normal 25s. Unrelated to `SendDialog`. Size unknown until the race is located | +| 137 | A reply to a message that arrived at two accounts can come from the wrong one | defect | S | open, 2026-08-22, found while building item 123 task 12. `ComposeContextBuilder::accountForReply()` takes `messagePaths` PLURAL to disambiguate, and nothing upstream ever gives it more than one path, so the disambiguation is inert | +| 138 | No Drafts filter beside Sent and Trash | workflow | S | open, 2026-08-23, from the notes. Verified: `kQueryGenerators` has no `drafts` entry, though every account already configures a `drafts` folder. Follows the `sent` generator exactly, which composes per-account folders rather than matching a tag | +| 139 | Forward is reachable only from the Message menu | discoverability | XS | open, 2026-08-23, from the notes. Verified: `forward` is added to `messageMenu` and to no toolbar. Compose and Reply are on the toolbar, so the third member of the set is the only one hidden | +| 140 | Compose, Reply and Forward belong over the message pane, not on the main toolbar | presentation | M | open, 2026-08-23, from the notes. The user's design: a bar of its own above the message pane carrying the three message actions, leaving the main toolbar for list-wide operations. Absorbs 139, which is the same three buttons in a worse place. See also 141 | +| 141 | The message pane has no button bar of its own | presentation | M | open, 2026-08-23, from the notes. The container 140 needs, and the home the user names for a `toggle_html` control. Sized as one item with 140 if built together | +| 142 | The composer's formatting buttons share a toolbar with Send and Attach | presentation | S | open, 2026-08-23, from the notes. Verified: one `addToolBar` carries Bold through Quote, then Attach, Remove attachment and Send. The user reads the row as a menu bar that is not one. Move the formatting half down to sit directly above the editor, beside the HTML checkbox | +| 143 | The formatting buttons are text, where every editor uses icons | presentation | XS | open, 2026-08-23, from the notes. Follows 142, and cheap once the row moves. `QIcon::fromTheme` per CLAUDE.md's chrome rule, with the text kept as the tooltip | +| 144 | "Also send a formatted copy" is prominent and does not say what it does | presentation | XS | open, 2026-08-23, from the notes. It means "send an HTML part as well as plain text", which the label never says. Secondary to writing the message, so it should read as such | +| 145 | Cc and Bcc are permanent rows on every composer | presentation | S | open, 2026-08-23, from the notes. Verified: both are unconditional `form->addRow` calls. Most messages use neither. Collapse behind a disclosure next to To:, expanded automatically when a draft or a reply already carries a value | +| 146 | The unsynced-changes count cannot be opened to see what it counts | information | S | **duplicate of 119**, recorded 2026-08-23 from the notes. Same request, and 119 already carries the blocker: one of the four things the count sums holds no message ids, so a list cannot be complete without changing how the count is kept | +| 147 | Toggle unread reads the same whichever way it will go | presentation | S | **duplicate of 99**, recorded 2026-08-23 from the notes. The notes ask for exactly what 99 describes: "Mark as read" on an unread message and the reverse. 99 already records that the label is harder than it looks, since a multi-row selection has no single direction | +| 148 | Ctrl+W does not close the composer | discoverability | XS | open, 2026-08-23, from the notes. Verified: nothing binds `Ctrl+W` anywhere, and the composer has no close action of its own. Belongs with item 21's table rather than bound in isolation | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -238,6 +252,45 @@ that appeared to be bound. See `KeyMap::defaultBindings()` and the query bar claims it back while focused, so a proposal that moves it must not resurrect that bug. +**The user is drafting the table, 2026-08-23.** It lives in their own notes as +`qtmaildir shortcuts and menu structure.md`, linked from the note this item +came from, and it is the specification this item was waiting for: a row per +action with the current binding, the proposed one, and an explicit "no +shortcut" column for the actions that should have none. **Read it before +starting, and do not propose a table of your own.** It is unfinished in two +known places, so it is a starting point rather than a finished spec: + +- **The menu-structure half is one line long** ("File should hold Save + message") and is where the second half of this item's work is specified. +- **The compose actions are absent from it**, because it predates them being + usable by hand. The user's position as of 2026-08-23, stated but not yet + written into their table: `Ctrl+Return` for Send is **kept**, which closes + that open question from item 123 task 11; and major actions should not go + three modifiers deep, so Reply becomes `Ctrl+R`, Reply all `Ctrl+Shift+R`, + and Forward `Ctrl+F`. + +**Two collisions that proposal creates, both to settle before building.** +`Ctrl+R` is `restore` today, and the draft's own row for it says "ok if not +needed for something else" — it now is, so Restore needs a new binding or +none. And `Ctrl+F` is Find in most applications; the draft frees it by moving +Find to `/`, so the two are coupled, and if `/` does not survive review then +Forward loses its binding with it. + +**`/` for Find needs an event filter, not a shortcut.** Qt withholds only +plain LETTERS from editable widgets, so a `/` registered as a `QAction` +shortcut is dispatched before the query bar, the tag dialog and the composer's +editor ever see it, and a user could not type a path or a URL in any of them. +This is the same trap `CLAUDE.md` records for arrow keys, and `Return` is the +worked example of the fix: claim it in `MainWindow::eventFilter` by accepting +the `ShortcutOverride`, narrowly, for the one widget that needs it. + +**Dropping a shortcut is not dropping the action.** The draft marks the five +`*_thread` actions (item 108) for removal, and the user confirmed on +2026-08-23 that this means their SHORTCUTS only. The menu entries must stay: +`everyActionIsReachableFromAMenu()` is a required rule, while item 132 made +the shortcut itself optional, so an action with no binding is now ordinary and +prints as `(unbound)` in the shortcut reference. + ## 40. No live filter over the current view **Observed (user, 2026-08-05):** "search in current view", spelled out as two @@ -1098,6 +1151,78 @@ Then Delete a message. Verified by hand on 2026-08-20; this is how it was found. **Size: S.** +## 135. The formatting toolbar's buttons stack rather than toggle + +**Observed (user, 2026-08-21):** pressing Bold a second time on already-bold +text adds another pair of asterisks rather than removing the first, so +`**this**` becomes `****this****`. Quote nests the same way: a second press on +`> one` gives `> > one`. The user asked for both to toggle. + +**A toggle was built and reverted the same session**, and the reason matters +more than the code: it was not unwanted, it **conflicts with the spec**, which +was not checked before the work started. + +- `2026-08-20-compose-and-send-design.md:236` states there is "deliberately no + live toggle that inserts and removes the quote while editing". +- `:187-190` is the complete statement of the wrap behaviour and describes only + wrapping, with no toggle anywhere. + +**Cause.** This is a **spec change, not a defect**, and both sites need +amending before any code is written again. + +Underneath sits a real design question the spec answers one way and a toggle +answers the other, which is why the two cannot simply coexist. `:187` preserves +the selection after a wrap **so that a second press applies a SECOND token** to +the same words: bold, then italic, without touching the mouse. A toggle makes +that gesture unreachable, because the second press now removes the first token +instead. **What replaces bold-then-italic is unanswered**, and answering it is +the substance of this item, not the state machine below. Possible directions, +none chosen: a modifier on the second press, a separate un-format action, or +accepting that the chord is lost and reaching nested emphasis by typing. + +**Approach.** When it is picked up, the transformation half is already +understood, so the notes below exist to stop it being rediscovered. A toggling +`wrap()` must distinguish three states, and a single "it unwraps" test passes +against most of them being broken: + +- **INSIDE** the tokens: `**this**` with `this` selected (2..6). The tokens sit + just outside the selection; the same characters stay selected afterwards. +- **AROUND** them: `**this**` selected whole (0..8). The selection shrinks to + the text that was between them. +- **PARTIALLY overlapping** one: `*this**` (6..13). Neither of the above. It + does not describe a wrapped span, and stripping would have to guess which + half of a token to keep, so wrapping is the predictable answer. + +**INSIDE must be checked before AROUND.** On `***this***` both tests match, and +only INSIDE removes the level the user actually asked for. + +**A naive adjacency test is wrong, and looks right.** Checking only whether the +characters either side of the selection equal the token means pressing *Italic* +on `**this**` finds a `*` on each side, strips one asterisk per side, and +**un-bolds text the user asked to italicise**. A strip must require the adjacent +RUN of token characters to be the token exactly, or the token plus one other +complete emphasis token: `***` is bold+italic and divisible either way, while a +run of two is one indivisible token whose half is not a token at all. This was +found by writing the italic-on-bold test, not by reading the code. + +The quote side is simpler but has one trap: a bare `>` is what the quote path +writes for a blank line, so an unquote that only recognises `"> "` leaves a +stray marker on every blank line in a round trip. Whether a mixed block (some +lines quoted, some not) quotes or unquotes is a decision; quoting it, so one +press makes the block uniform and the next unquotes it, avoids the button doing +two opposite things to two halves of one selection. + +**Constraints.** The spec amendment comes first and must resolve the +bold-then-italic question, or the same conflict recurs. `MarkdownFormat` is +painter-free and widget-free, so the whole state machine is unit-testable +without the composer; keep it that way. The toolbar shortcuts belong to the +composer window and do not touch `KeyMap`, so nothing here interacts with item +132. Note that toggling changes what the preserved selection is FOR, so +`wrappingTwiceNestsTheTokensAroundTheSameWords` and +`quotingAnAlreadyQuotedLineNestsIt` in `tests/test_formattoolbar.cpp` both +assert the current spec behaviour and would be replaced rather than extended. + + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering @@ -1140,3 +1265,261 @@ whether to open the spec at all, and leave the rest there. Name the spec `<date>-<name>-design.md`, and state in its header which backlog items it resolves, so the numbering stays traceable in both directions. + +## 137. A reply to a message that arrived at two accounts can come from the wrong one + +**Observed.** A message that exists in more than one maildir, because it was +sent to two of the user's addresses or duplicated across accounts by mbsync, +can open its reply from either account. Which one is picked is arbitrary. The +consequence is visible in the composer's From field, so it is not silent, but +it is only visible to somebody who thinks to look: the reply is otherwise +correct and sendable, and the recipient sees a From the user did not intend. + +**Cause, verified in the code.** The disambiguation exists and is unreachable. +`ComposeContextBuilder::accountForReply()` (`src/composecontext.cpp:405`) takes +`messagePaths` as a `QStringList` precisely so it can resolve this case: with +more than one candidate account it prefers the one whose own address appears +among the recipients, which is the reason the copy landed there. Nothing +upstream ever gives it more than one path. `NotmuchWorker::loadMessage()` +(`src/notmuchworker.cpp:573`) builds its `MessageRef` from +`notmuch_message_get_filename()`, the SINGULAR accessor, so `MessageRef` holds +one `filePath` and `MainWindow::openComposerFor()` can only pass a +one-element list. The plural parameter is therefore inert, and the branch that +consumes it is dead code today. + +`notmuch_message_get_filenames()`, the plural accessor that would supply the +rest, exists in libnotmuch and is used nowhere in this repository. + +**Approach.** Add `QStringList filePaths` to `MessageRef` (`src/types.h:123`) +ALONGSIDE the existing `filePath` rather than replacing it, and populate it in +`loadMessage()` from `notmuch_message_get_filenames()`. `filePath` stays as the +render path, so `MainWindow::renderMessages()` and everything else that opens +one file are untouched; only `openComposerFor()` reads the new field. That +keeps the change to two files plus the one call site. + +**Constraints.** The test has to put the same message id in two accounts' +maildirs, which `NotmuchFixture` can do by writing the same `Message-ID` into +two folders before indexing. Assert on the account CHOSEN rather than on a +count of paths: a test that only checks `filePaths.size() == 2` passes against +`accountForReply()` still ignoring them. The recipient-preference branch is +what needs covering, so the two accounts must have different addresses and the +message must be addressed to one of them, or either answer is correct and the +test proves nothing. + + +## 136. `undoMovesTheMessageBack` fails about one run in six + +**Observed.** `test_mainwindow` failed during a full-suite run while item 123 +task 10 was in the working tree. The failing function is +`TestMainWindow::undoMovesTheMessageBack`. The run that failed took 70 seconds +against a normal 25, so whatever goes wrong also blocks for a while before +giving up. + +**Not caused by item 123.** This was checked rather than assumed, because a +failure appearing during unrelated work is exactly the kind of thing that gets +blamed on the change in front of it. With the branch's work `git stash`ed out, +on a clean tree, it still failed **1 run in 6**. Nothing in `SendDialog` +touches the model, the Maildir, or the undo stack. + +**Cause, unverified.** A race around the Maildir file move that Delete +performs and Undo reverses. Whether the race is in the test's wait or in the +production move is exactly what the item has to establish, and that is why the +size is `?` rather than a guess. The two have very different consequences: a +test that waits wrongly is noise, while a move that races is mail landing in +the wrong folder, and CLAUDE.md already records that a wrong folder name from +this code path reaches the mail server. + +**Approach.** Reproduce in isolation first, with the suite's own +`QT_QPA_PLATFORM=offscreen` and a loop over `ctest -R mainwindow`, and capture +a failing run's output before theorising. The 70-second duration is the useful +clue: something is waiting on a condition that never arrives rather than +asserting immediately, so find which `QTRY_*` or `qWait` is timing out. + +**Constraints.** A fix must not restore the real `/proc/locks` (item 61), and a +flaky test must not be "fixed" by widening its timeout until it passes, which +converts a real race into a slower green. If the race turns out to be in the +production move rather than the test, this stops being a test-hygiene item and +becomes a mail-safety one. + +--- + +## 138. No Drafts filter beside Sent and Trash + +**Observed.** The query row carries Unread, Inbox, Important, Sent and Trash. +There is no Drafts button, though the composer has been writing drafts to each +account's drafts folder since item 123. + +**Cause, verified 2026-08-23.** `kQueryGenerators` in `config.cpp:62-66` is a +closed set of five, and `drafts` is not among them. Every account already +carries a `drafts` key (`config.cpp:453`), read for the composer's autosave, so +the data the filter needs is configured and unused by the query row. + +**Approach.** Follow `sent`, not `inbox`. A tag query would be wrong for the +same reason it is wrong for Sent: `draft` is a Maildir flag notmuch surfaces as +a tag, but the folder is what the user means, and a message a provider marks +differently would disagree. `Config::allSentQuery()` composes the union over +every account's folder and `Account::sentQuery()` the per-account half; both +need a drafts twin. The generator string is wire format and must stay `drafts` +in queries.json whatever the button is called in a given locale. + +**Constraints.** An account with no `drafts` key contributes NOTHING rather +than an empty term, or the button shows the whole Maildir: this is +`Config::matchNothingQuery()`'s reason for existing. The hook's carve-out +(`assets/hooks/qtmaildirconf.py`) reads the same key, so the two now agree on +what a drafts folder is; they are separate readers and neither should start +importing the other. + +--- + +## 139. Forward is reachable only from the Message menu + +**Observed.** There is no Forward button anywhere in the interface. The action +exists and works; it is in the Message menu and nowhere else. + +**Cause, verified 2026-08-23.** `mainwindow.cpp:1714` registers the action and +`1763` adds it to `messageMenu`. The toolbar block at `1975-1994` adds Compose +and Reply but never Forward, so two thirds of the message-action set are +visible and the third is not. + +**Approach.** One line, if the toolbar is where it belongs. It probably is not: +item 140 records the user's own view that all three belong over the message +pane instead, which makes this item the cheap half of that one. Build 140 and +this closes with it; build this alone only if 140 is deferred. + +**Constraints.** The no-duplicate-icons rule covers any action that can reach +the toolbar, so Forward needs an icon distinct from Reply's rather than a +variant of it. + +--- + +## 140. Compose, Reply and Forward belong over the message pane + +**Observed.** The user's note: "'Write new message' and 'reply' live next to +the other icon only buttons, but they belong in a new bar on top of the message +pane, together with 'Forward'." + +**Cause.** Not a defect. The toolbar grew by accretion and now mixes two +different scopes: Sync, Archive, Delete, Mark all read and Undo act on the LIST +or on the selection, while Compose, Reply and Forward are about a message. The +main toolbar reads as the place for everything, so the distinction is invisible. + +**Approach.** A bar above the message pane carrying the three message actions, +and the main toolbar keeping the list-wide ones. Compose is arguably neither, +since it needs no message at all; the user grouped it with the other two, and +that grouping is theirs to make. It shares the container item 141 introduces. + +**Constraints.** The actions themselves do not move: they stay in +`m_actions`, keep their shortcuts, and keep their menu entries, which is what +`everyActionIsReachableFromAMenu()` asserts on. This is a second presentation +of the same `QAction`s. Absorbs item 139. + +--- + +## 141. The message pane has no button bar of its own + +**Observed.** The user asks for "a button bar in the message pane area", and +names `toggle_html` as a control that would fit it. + +**Cause.** Nothing exists to hang such a control on. The pane is a header +label, the web view, the attachment bar and the tag strip; a per-message +control has no home, which is why `toggle_html` lives in a menu. + +**Approach.** The container item 140 needs. Whether it holds only the three +message actions, only view controls like `toggle_html`, or both is the design +question, and it should be settled with the user before building: a bar that +mixes "act on this message" with "change how I am looking at it" is the same +confusion item 140 exists to remove, one level down. + +**Constraints.** `MessageView` is built inline in its own class rather than +from named widget classes, per CLAUDE.md, and this should not become the +exception. Size assumes 140 and 141 are built together; separately they are +each S and the seam between them is wasted work. + +--- + +## 142. The composer's formatting buttons share a toolbar with Send and Attach + +**Observed.** The user reads the composer's top row as a menu bar that is not +one, and asks for the formatting controls to move down beside the HTML +checkbox, directly above the editor. + +**Cause, verified 2026-08-23.** `composewindow.cpp:326-397` builds ONE +`addToolBar`, which carries Bold, Italic, Code, the heading and list actions, +Link and Quote, then a separator, then Attach, Remove attachment and Send. +Three different scopes in one row: text formatting, message composition, and +the terminal action. + +**Approach.** Split it. The formatting half moves to a row directly above the +editor, where the text it formats is; Attach, Remove attachment and Send stay +in the window's own toolbar. The HTML checkbox already sits under the editor +and is the anchor the user names. + +**Constraints.** `setInputsEnabled()` disables `m_formatToolbar` wholesale +during a send (`composewindow.cpp:692`), so a split needs both halves disabled, +and a test for the send path that asserts on only one of them would pass +against a live Attach button during a send. + +--- + +## 143. The formatting buttons are text where every editor uses icons + +**Observed.** The user asks for icon-only formatting buttons, "like any other +text editor". + +**Cause.** They were built as text actions, and the composer's toolbar has no +icons at all. + +**Approach.** `QIcon::fromTheme` per CLAUDE.md's rule that chrome is the +system's, keeping the current text as the tooltip so nothing becomes +unnameable. Cheap once item 142 has moved the row, and awkward before, since +the same row would then mix icon-only formatting with text Send and Attach. + +**Constraints.** `format-text-bold` and its siblings are standard freedesktop +names, but a theme may not carry all of them; an action with no icon must fall +back to its text rather than rendering as an empty button. Icon-only is also +the state where the tooltip stops being decoration, so every one needs to be +right. + +--- + +## 144. "Also send a formatted copy" is prominent and says nothing + +**Observed.** The user's note: "I suppose it means 'format/send as html', but +that flag is secondary, doesn't need to be so prominent." + +**Cause, verified 2026-08-23.** `composewindow.cpp:265`. The label describes a +mechanism ("a formatted copy") without naming it, so the reader has to infer +that "formatted" means HTML and that "copy" means an additional MIME part +rather than a second message. + +**Approach.** Two independent halves, and they can ship separately. Say what it +does: something closer to "Send an HTML version as well". And demote it: it is +a per-message override of a config default (`[compose] send_html`), which is +secondary to writing the message. + +**Constraints.** The string is translated, so changing it makes the Italian +entry stale; `ctest -R translations` fails on an untranslated string, which is +the intended safety net rather than an obstacle. Nothing matches on this text, +so CLAUDE.md's "translating a string something matches on" trap does not apply. + +--- + +## 145. Cc and Bcc are permanent rows on every composer + +**Observed.** The user asks for them hidden behind a disclosure next to the To: +field. + +**Cause, verified 2026-08-23.** `composewindow.cpp:251` and `255` add both as +unconditional form rows. Most messages address neither, so two of the four +header rows are usually empty. + +**Approach.** A disclosure beside To: that reveals both together. They expand +automatically, and stay expanded, whenever either already carries a value: a +reply that carries Cc, or a reopened draft, must not hide a recipient the +message is actually addressed to. + +**Constraints.** That auto-expansion is the load-bearing half. A hidden field +holding an address is a message going somewhere the sender cannot see, which is +worse than the clutter this removes. The seeding runs before `buildUi()`'s +`markDirty()` connections per the constructor's ordering comment, so whatever +decides the initial state has to read the seeded values rather than the widgets. diff --git a/docs/superpowers/plans/2026-08-20-compose-and-send.md b/docs/superpowers/plans/2026-08-20-compose-and-send.md index 195dacc..7d5f6f1 100644 --- a/docs/superpowers/plans/2026-08-20-compose-and-send.md +++ b/docs/superpowers/plans/2026-08-20-compose-and-send.md @@ -3871,6 +3871,16 @@ popup between stages." ### Task 11: ComposeWindow +**Found during Task 4's code review, and it lands here.** `MessageBuilder::build()` +is SYNCHRONOUS and can block: a large attachment is read and base64-encoded on +the calling thread. Autosave calls it on a timer, on the GUI thread, so a +30-second debounce that hits a 25MB attachment stalls typing. The directory +hang that review found is fixed in `MessageBuilder`, but the blocking read +remains by design. Do not move it to a thread as part of this task, since +nothing here crosses the worker boundary and adding a second threading model +for one call is worse than the stall. Note it in a comment at the autosave call +site so the next person measuring a freeze knows where to look. + The only unit here that owns widgets, and the one that composes the other four. It contains no MIME and no process logic: a composer bug and a MIME bug are found in different files. @@ -3998,6 +4008,20 @@ private: `src/composewindow.cpp`. The full file is long; these are the parts that carry decisions, and the rest is ordinary widget assembly. +**One thing in this block is load-bearing and easy to drop while retyping it: +the `Qt::SingleShotConnection` on the `MessageSender::finished` connect inside +the `committed` handler.** `m_sender` is a long-lived member, so a plain +`connect()` beside a `send()` call leaks a receiver per send and the second +result runs every earlier lambda, each still holding an earlier message's bytes +by value: a sent copy of the wrong message, and `accept()` on a destroyed +dialog. `MessageSender`'s own once-only guard cannot help, because that guards +the emit and this is one emit reaching many receivers. The header for +`MessageSender::finished` states the rule and +`test_messagesender.cpp::aPerSendConnectionMustBeSingleShot` measures it (3 +deliveries for 2 sends without the flag, 2 with it). Noted here because the +plan's code blocks are drafts and this is the line whose absence still +compiles, still runs, and is wrong only on the second send. + ```cpp #include "composewindow.h" @@ -4159,6 +4183,20 @@ void ComposeWindow::send() connect(dialog, &SendDialog::committed, this, [this, dialog, built, account]() { m_sender->send(account.sendCommand, built.bytes); + // Qt::SingleShotConnection IS REQUIRED HERE, and this line is the + // correction of a defect that was in this plan's draft (found while + // building Task 6, 2026-08-21). m_sender is a long-lived member, so a + // bare connect() beside each send() accumulates a permanent receiver + // per send. Send, fail, correct the recipient, send again, and the + // second result runs BOTH lambdas: the first still holds the FIRST + // message's `built` and `account` by value, so it files a sent copy of + // the wrong message and calls accept() on a dialog it already + // deleteLater()'d. MessageSender's m_reported guard cannot prevent + // this: it collapses two QProcess signals into one emit, and this is + // one emit reaching many receivers. Measured in + // test_messagesender.cpp::aPerSendConnectionMustBeSingleShot, where + // the bare shape delivers 3 results for 2 sends and the single-shot + // shape delivers 2. connect(m_sender, &MessageSender::finished, this, [this, dialog, built, account](bool sent, const QString &error) { if (!sent) { @@ -4787,6 +4825,21 @@ replace. - Modify: `docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md` - Modify: `docs/superpowers/specs/2026-08-20-compose-and-send-design.md` +- [ ] **Step 0: Document the new keys in the README** + +Found during Task 2's code review and assigned here rather than there. The +README's sample config at `README.md:150-215` documents EVERY other +configuration key, including recently added ones, and has nothing for +`send_command` or the `[compose]` section. Without this the keys ship +undiscoverable: a user has no way to learn that sending exists. + +Take the block from the spec at +`docs/superpowers/specs/2026-08-20-compose-and-send-design.md:552-560` and +adapt it to the README's existing commented style, showing `send_command` in +an account section and every `[compose]` key with its default. Say plainly +that an account without `send_command` is receive-only, since that is the +part no reader would guess. + - [ ] **Step 1: Add the changelog entry** Under `## [Unreleased]`, in the existing `### Added` section or a new one: diff --git a/docs/superpowers/specs/2026-08-20-compose-and-send-design.md b/docs/superpowers/specs/2026-08-20-compose-and-send-design.md index aade2d1..9533602 100644 --- a/docs/superpowers/specs/2026-08-20-compose-and-send-design.md +++ b/docs/superpowers/specs/2026-08-20-compose-and-send-design.md @@ -430,7 +430,7 @@ Two structs cross boundaries, in `types.h` beside the existing ones. | `originalPath` | the `.eml` being replied to or forwarded; empty for New | | `inReplyTo` | Message-ID of the original | | `references` | the original's References plus its Message-ID | -| `to`, `cc` | pre-filled recipients, the user's own addresses already stripped | +| `to`, `cc` | pre-filled recipients, the user's own addresses already stripped; a reply to the user's OWN message is addressed to that message's recipients instead of back to the user, mirroring its To/Cc split (see Replying to oneself) | | `subject` | `Re:` / `Fwd:` prefixed, an existing prefix not doubled | | `quotedBody` | the `>`-prefixed original; empty when the action does not quote | | `seedHtml` | did the original carry a `text/html` part | @@ -447,6 +447,13 @@ Two structs cross boundaries, in `types.h` beside the existing ones. | `attachments` | local paths | | `inReplyTo`, `references` | carried through unchanged | +Message-ids are carried BARE, without angle brackets, matching what GMime hands +back when `MimeParser` reads a `Message-ID`. `MessageBuilder` adds the brackets +when it writes the header, in one place rather than in each caller: they are wire +syntax, and GMime writes an EMPTY header for a bare addr-spec rather than +complaining, so a caller that forgets them ships a reply that threads nowhere +while nothing looks wrong locally. + `In-Reply-To` and `References` are not optional. Without them a reply appears as an orphan thread in the sender's own client. @@ -505,15 +512,38 @@ Six, each needing the five places `CLAUDE.md` enumerates: `knownActions()`, | Action | Meaning | Scope | |---|---|---| | `compose` | New message | none needed | -| `reply` | Reply to the displayed message, quoted | sender only | +| `reply` | Reply to the displayed message, quoted | sender only, except when the sender is the user (see below) | | `reply_all` | Reply to all, quoted | sender + To + Cc, own addresses removed | -| `reply_no_quote` | Reply with an empty body | sender only | +| `reply_no_quote` | Reply with an empty body | sender only, same exception | | `forward` | Forward, body quoted inline, attachments carried | none | | `save_message` | Write the raw `.eml` to a chosen path | any message | `reply_all_no_quote` is deliberately absent. Six actions is already a large menu and the combination is reached by deleting the quote. +### Replying to oneself + +A reply whose sender is entirely the user's own addresses is addressed to that +message's **original recipients** rather than to the sender. A plain reply takes +its To and Cc together, having no Cc field of its own to mirror into. A +reply-all MIRRORS THE SPLIT: the original's To becomes To and its Cc becomes Cc, +because To means "addressed to you" and Cc "for information", and promoting a +Cc'd party to To is a change every recipient can see. +This is an ordinary gesture rather than an edge case: it is reached from the +Sent view, from a follow-up on mail that went unanswered, and from any thread +whose selected row is the user's own message. Addressing the sender there +addresses the user, so the reply reaches nobody it was meant for. + +"Own" means EVERY parsed sender address is the user's. A message the user sent +together with somebody else is still a reply to that co-sender, and takes the +ordinary sender-only path. + +Mail the user sent to THEMSELVES alone leaves nothing after own addresses are +removed, and there the sender is restored: the user is the correct recipient of +their own note. The rejected alternative was to strip the sender and leave To +empty, which silently drops every recipient while the message still looks +sendable. + **Every action acts on the displayed message**, resolved with `messageScopeFor()` semantics: a thread row means the one message its card shows, a reply row means itself. Not `threadFor()`. Replying to a thread is @@ -660,7 +690,8 @@ Cases: `multipart/alternative` when `sendHtml` is on and `text/plain` alone when off; `multipart/mixed` nesting with attachments; each enabled extension rendering, and tables and raw HTML **not** rendering; RFC 2047 encoding of a non-ASCII subject and display name; quoted-printable for an accented body; -`In-Reply-To` and `References` carried; `Re:` and `Fwd:` not doubling. +`In-Reply-To` and `References` carried; `Re:` and `Fwd:` not doubling, in the +non-English spellings and counted forms as well as the English ones. **`test_messagesender`** uses stub commands, not msmtp: one exiting 0, one exiting non-zero with stderr, one that does not exist. The stub writes stdin to |