aboutsummaryrefslogtreecommitdiffstats
path: root/docs/superpowers/plans
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-08-27 13:05:17 +0200
committerDanilo M. <danix@danix.xyz>2026-08-27 13:05:17 +0200
commit5570d0e7495a42a90acf951d396c9185b0319eb9 (patch)
tree4052c9060b6ade33577c9bcb285565a1477bd3eb /docs/superpowers/plans
parent12e841b8e2c4c225ea79de87dc7bb50f0404ee69 (diff)
downloadqtmaildir-5570d0e7495a42a90acf951d396c9185b0319eb9.tar.gz
qtmaildir-5570d0e7495a42a90acf951d396c9185b0319eb9.zip
feat: forward an HTML message with its formattingHEADmaster
Item 171. A forward carried only the plain-text version of the original, so formatting was lost; and an original with no plain-text part at all (30 of 342 sampled inbox messages, ~9%) forwarded as an empty quote with its content silently gone. A forward now sends ONE part chosen by the Send-as-HTML toggle: the original's markup when on, the text quote when off. Not a multipart/alternative, at the user's decision: a forward's shape is already decided by that toggle, and sending both hands the choice to the recipient's client. The toggle is honoured even for an HTML-only original, which then forwards as a text fallback. HtmlSanitiser strips remote content from the forwarded markup, checked by default with a per-forward opt-out. This is the security-critical part: the markup leaves this process and is rendered by the recipient's client, where none of MessageView's protections apply, so forwarding a tracking pixel forwards the tracking. It is an ALLOW-LIST, unlike HtmlBuilder::namespaceCids(), because a missed rewrite is a broken image while a missed strip is a beacon reaching the recipient. An HTML forward does not seed a text quote into the editor. The first build did, then subtracted it when building the HTML part, so the user could edit a quote whose edits were discarded; what the composer shows must be what gets sent. The forwarded message appears in a read-only pane beside the editor instead, a QSplitter at 60/40 with a toggle in the Format menu. A plain forward is unchanged. ComposeContextBuilder::quoteBody() renders htmlBody down to text when there is no plain part, so the plain path never emits an empty quote. Design in docs/superpowers/specs/2026-08-27-forward-html-design.md. Two tests repaired for the splitter: the 60/40 assertion reads stretch factors rather than pixels, since the offscreen platform gives the splitter no width and reports 49/49 whatever the code asks; and theComposerSplitsItsToolbarByScope looked for the body directly in the composer's column. Not yet hand-tested in this arrangement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtUzfNjMD8fiYfamDd3ywW
Diffstat (limited to 'docs/superpowers/plans')
-rw-r--r--docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md153
-rw-r--r--docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md114
2 files changed, 207 insertions, 60 deletions
diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md
index 4556b48..77d4726 100644
--- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md
+++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md
@@ -8935,3 +8935,156 @@ That is fixed. The reported tag set `draft inbox unread` is fully explained:
`unread` from the missing `S` flag on the draft, `inbox` from the arrived
message sharing its thread.
+## 171. A forwarded HTML message reaches the recipient as plain text
+
+**Observed (user, from the notes):** "forwarding an html message doesn't
+maintain the html formatting of the original message. #bug"
+
+**Cause (verified in the code, 2026-08-27).** The forward path builds its body
+through `ComposeContextBuilder::quoteBody()` (`src/composecontext.cpp`), which
+reads `message.plainBody` and nothing else. `MimeParser` parses both halves and
+`ParsedMessage` carries `htmlBody` beside `plainBody` (`src/mimeparser.h:150`),
+so the HTML is available and simply never asked for.
+
+Two consequences follow, and they are not the same severity:
+
+- An original with both parts forwards its text/plain alternative, losing the
+ sender's formatting. Recoverable-looking, since the words survive.
+- An original with an HTML part ONLY has an empty `plainBody`, so the forward
+ carries the attribution line and an empty quote. The message's content is
+ gone, and nothing says so.
+
+`MainWindow::composeReply()` already treats the two kinds differently for the
+composer's own HTML state: `context.seedHtml` is the CONFIG's `sendHtml` for a
+forward and `original.hasHtml()` for a reply, on the stated reasoning that an
+HTML part is a fact about the sender's software. That reasoning is sound for
+how the user WRITES and does not decide what the forward CARRIES, which is the
+question here.
+
+**Approach.** The decision comes first; this is not a changed call site.
+
+A forward is a different act from a reply: the point is to hand somebody else
+what arrived, and quoting is the wrong shape for it. Three candidates, in
+increasing fidelity:
+
+- Render `htmlBody` down to text when `plainBody` is empty, so nothing is
+ silently lost. The smallest fix, and it does not answer the note: formatting
+ is still gone.
+- Carry the original as a `message/rfc822` part, which is what item 130 already
+ describes and what GMime builds natively. Perfect fidelity, and every
+ attachment comes with it, but the recipient sees an attached message rather
+ than a body.
+- Build the forward as `multipart/alternative` with the original's HTML nested
+ in the HTML half, which is what Thunderbird's inline forward does.
+
+**Constraints.**
+
+- **The HTML is input from a stranger and the composer is not the message
+ pane.** The pane's protections (off-the-record profile, JavaScript off, the
+ interceptor blocking every request) are `MessageView`'s, not
+ `ComposeWindow`'s. Any route that puts the original's markup into an outgoing
+ message must decide what it strips, and remote references in particular:
+ forwarding a tracking pixel forwards the tracking to the new recipient.
+- Item 130 overlaps and may subsume this. Decide the two together rather than
+ building `message/rfc822` twice.
+- The markdown body is the composer's source of truth, and markdown has no
+ syntax for arbitrary HTML the user can then edit. A route that keeps the
+ original's markup has to keep it OUTSIDE the editable buffer, which is the
+ same nesting problem item 129 carries.
+- `quoteBody()` is shared with Reply. A change there reaches both; the
+ behaviour asked for is the forward's alone.
+
+---
+
+**BUILT 2026-08-27.** Design in
+`docs/superpowers/specs/2026-08-27-forward-html-design.md`, which is the
+document to read; this entry records only what changed and what was learned.
+
+The user chose **carrying the original's markup inline** over attaching the
+original as `message/rfc822` (item 130's mechanism, still open for its own
+sake) and over a text-only fallback, and chose **strip remote content by
+default with a per-forward opt-out** over always stripping and over keeping
+everything.
+
+**Amended the same day, after the first build**: a forward sends ONE part
+rather than a `multipart/alternative`, chosen by the Send-as-HTML toggle. The
+first build sent both halves and also FORCED html on when there was markup to
+carry; both were reversed. A forward's shape is something the user has already
+decided by flipping that toggle, and sending both hands the choice to the
+recipient's client. The consequence was put to the user explicitly and
+accepted: with the toggle off, an HTML-only original forwards as the text
+fallback and its formatting is lost.
+
+Four parts, each independently useful:
+
+1. **`HtmlSanitiser`** (`src/htmlsanitiser.h/.cpp`), a namespace of free
+ functions so the security property is testable without a widget.
+2. **`quoteBody()`'s empty-plain fallback**, via
+ `QTextDocumentFragment::fromHtml().toPlainText()`. Closes the silent half
+ on its own.
+3. **The MIME nesting** in `MessageBuilder`, asserted by parsing the result
+ back through `MimeParser` rather than by reading the RFC.
+4. **The composer control**, created only when the original actually carries
+ remote content.
+
+**The allow-list rule is the part to preserve.** `HtmlBuilder::namespaceCids()`
+is a block-list and documents scoping `srcset=` out; that trade is right for
+rewriting and wrong for stripping, because a missed rewrite is a broken image
+and a missed strip is a beacon reaching the recipient. `HtmlSanitiser` judges
+every attribute by its VALUE, so `srcset`, `poster`, `data-*` and whatever HTML
+adds next are handled by the default, which is removal.
+
+**A real bug was caught by the tests and is worth recording**: the walk used
+`QRegularExpression::globalMatch()` while also advancing `pos` past a removed
+element's content. `globalMatch` iterates over matches found against the
+ORIGINAL string, so it handed back tags from inside the region just skipped;
+the output duplicated content and an `<iframe>` survived. It matches by hand
+from `pos` now. A tidy test would not have found this: it needed a removed
+element with content, mid-document, followed by more markup.
+
+**Measured, not assumed:** 30 of 342 sampled inbox messages (~9%) declare
+`text/html` with no `text/plain`, so the silent-loss half was not an edge case.
+
+**Hand-tested 2026-08-27, and it found two defects, the second of which
+changed the design.**
+
+1. The original shipped TWICE inside the one HTML part: the composer seeds the
+ text quote into the editable body, so `markdownBody` already carried a
+ flattened copy, and the markup was appended to it. It read as two messages
+ stacked, the first with its URLs naked and mangled. A screenshot of a real
+ forward is what showed it; no test had covered the composer and the builder
+ together.
+
+2. The first fix subtracted the quote in `MessageBuilder`. **The user rejected
+ it**: the quote was still shown in the composer and no longer sent, so it
+ could be edited and the edits silently discarded. "If it is in the composer
+ but it's not sent, is worse." That is the right objection and the general
+ principle behind it, **what the composer shows must be what gets sent**, is
+ what the design now follows.
+
+So an HTML forward **does not seed a text quote at all**. The buffer holds the
+user's own note alone, and the forwarded message appears in a read-only pane
+BESIDE the editor, a `QSplitter` at 60/40 with a toggle in the Format menu (the
+user's chosen arrangement). A plain forward is untouched: its quote is in the
+buffer, where it is both editable and sent, so WYSIWYG already held there.
+
+The pane is a `QTextBrowser`, not a `QWebEngineView`: a web view would mean a
+second Chromium render process per composer and a second copy of MessageView's
+protections. The cost is that Qt's HTML subset is narrower than a mail
+client's, so the pane shows the original ROUGHLY. Its label says the message
+is sent as it arrived, so the pane is not mistaken for what travels.
+
+**The user asked for a real rich-text composer as the proper answer**, recorded
+as item 173, which supersedes this middle ground and subsumes item 133.
+
+Two test traps hit while building it, both already in CLAUDE.md and both hit
+anyway: the offscreen platform gives a `QSplitter` no width, so `sizes()`
+reports 49/49 whatever the code asks and a pixel assertion fails against
+correct code (assert the stretch factors, which land in the child's size
+policy since `QSplitter` has no getter); and moving the editor into a splitter
+broke `theComposerSplitsItsToolbarByScope`, which looked for the body directly
+in the composer's column.
+
+Still to do: the hand test of the new arrangement. Forward a real HTML message
+with the box checked and unchecked and confirm what arrives.
+
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 fddb746..9cb8ba9 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
@@ -202,7 +202,7 @@ taking that too literally.
| 130 | A message cannot be attached to another message directly | v2 | S | open, 2026-08-20, from the item 123 brainstorm. **Blocked on 123.** A `message/rfc822` part, which GMime builds natively. The manual route exists from 123's first commit: `save_message` writes the `.eml` and it is attached as a file |
| 131 | The markdown dialect and extensions are fixed | v2 | S | open, 2026-08-20, from the item 123 brainstorm. **Blocked on 123.** Configurable in the shape Hugo's config uses. Deliberately fixed initially: CommonMark plus autolink, strikethrough and tasklist |
| 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 |
+| 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 . **Subsumed by item 173** 2026-08-27: that is a rich-text editor, this is the cheap answer to the same want. Build one or the other, never both |
| 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 when run ALONE, passes in the full suite | defect | ? | open, 2026-08-21, re-measured 2026-08-24 and it is not what the row said. Filed as an intermittent race (1 in 6); it is in fact **deterministic on the selection**: 6 failures in 6 when named on the command line, and, as of 2026-08-24, it fails in the FULL run too: measured at 58f13ad with the day's work stashed out, 274 passed and this one failed. The "passes in the suite" half of this row is therefore no longer true, and the selection-dependence it was named for may not be either. Re-measure before theorising. All three of its 15s `QTRY` timeouts expire, giving 45s against a 25s whole-suite run, so undo never moves the file rather than losing a race. A test that needs its predecessors is the likely shape (the `init()` lock-table fixture of item 61 is one candidate), which makes it a TEST defect until shown otherwise. Not caused by item 149, and re-confirmed 2026-08-24 as not caused by item 152 either, by running the test at the preceding commit in a throwaway worktree. The assertion that fails names the real question: the restored file is in NEITHER `cur` nor `new` of the account inbox, so establish where it went before theorising about a race |
@@ -244,8 +244,9 @@ taking that too literally.
| 168 | Delete is offered on mail already in the trash, and does nothing | defect | S | **done 2026-08-25**, unreleased. Delete is hidden when every selected row is already in its account's trash, Restore when none is, both keyed on the PATH rather than the `deleted` tag. Delete also drops `unread` now, in the same TagChange so one undo returns the folder and the tag together |
| 169 | A card shows the account only as a bar, with no fade and no avatar | presentation | M | **done** 2026-08-26, unreleased, on `card-avatars`, merged fast-forward. Both halves: a `QLinearGradient` from the account colour to the pane's base across the card, and a squircle avatar with initials, given a rect in `CardLayout` so the geometry is asserted without a painter. **Hand-testing found four defects**, all fixed in 9ae43f9: `Avatar::initialsFor()` normalises the display name first (drops the angle-addr, takes the first comma-separated author, unwraps quotes, treats a bare address as no name, requires a word to carry a letter or digit); the two-tone gradient axis spans the DIAMETER rather than a radius, which was letting one hue fill the whole face; the account fade runs right to left, anchored opaque at the card's right edge; and a flat view hashes `ThreadSummary::firstMessageRecipient` rather than the user's own address. The vCard half stays blocked on item 72 |
| 170 | A row that stops matching the view only leaves it on the Delete path | defect | S | open, 2026-08-26, from the notes, **cause found the same day and the premise is NOT stale**. The optimistic REPAINT is universal; the optimistic MEMBERSHIP is not. `removeThreadsWithoutTag()` has exactly one caller, on the move path, so marking a message read in the Unread view repaints the row and leaves it in a list it no longer belongs to |
-| 171 | A forwarded HTML message reaches the recipient as plain text | defect | S | open, 2026-08-27, from the notes. `ComposeContextBuilder::quoteBody()` reads `ParsedMessage::plainBody` only, so the original's `htmlBody` is dropped whatever the composer's own Send-as-HTML state is. Needs a DECISION on what a forward carries before any code, see the entry |
+| 171 | A forwarded HTML message reaches the recipient as plain text | defect | M | **done** 2026-08-27, unreleased. Design in `specs/2026-08-27-forward-html-design.md`. The user chose inline `multipart/alternative` over attaching the original, and remote content stripped by default with a per-forward opt-out. Four parts: `HtmlSanitiser` (an ALLOW-LIST, unlike `namespaceCids()`, because a missed strip is a beacon where a missed rewrite is a broken image), a text fallback for the ~9% of mail with no plain part, the MIME nesting, and the composer control |
| 172 | A draft this application writes is tagged `unread` | defect | XS | **done** 2026-08-27, unreleased. `DraftStore::write()` was called with `"D"`, and `maildir.synchronize_flags` makes notmuch tag anything without `S` as `unread`. Self-healing on the next sync of that folder, which is what made it look intermittent |
+| 173 | The composer is a plain-text editor, not WYSIWYG | v2 | L | open, 2026-08-27, **asked for by the user** while hand-testing 171. This is a GUI mail client and should edit rich text the way one does: the forwarded original, and the user's own formatting, visible and editable in place. Supersedes the preview 171 shipped as a middle ground, and **subsumes item 133** (markdown syntax highlighting), which is the same want answered cheaply. See the entry: the draft format and the markdown-as-source-of-truth model both change |
Sizes are rough: XS under an hour, S a sitting, M a session.
@@ -1210,61 +1211,54 @@ every tag write passes. Move it, or call it from both send paths.
- Undo goes back through the same funnel, so a removal must not make an undone
mark-read invisible in the view it was undone in.
-## 171. A forwarded HTML message reaches the recipient as plain text
-
-**Observed (user, from the notes):** "forwarding an html message doesn't
-maintain the html formatting of the original message. #bug"
-
-**Cause (verified in the code, 2026-08-27).** The forward path builds its body
-through `ComposeContextBuilder::quoteBody()` (`src/composecontext.cpp`), which
-reads `message.plainBody` and nothing else. `MimeParser` parses both halves and
-`ParsedMessage` carries `htmlBody` beside `plainBody` (`src/mimeparser.h:150`),
-so the HTML is available and simply never asked for.
-
-Two consequences follow, and they are not the same severity:
-
-- An original with both parts forwards its text/plain alternative, losing the
- sender's formatting. Recoverable-looking, since the words survive.
-- An original with an HTML part ONLY has an empty `plainBody`, so the forward
- carries the attribution line and an empty quote. The message's content is
- gone, and nothing says so.
-
-`MainWindow::composeReply()` already treats the two kinds differently for the
-composer's own HTML state: `context.seedHtml` is the CONFIG's `sendHtml` for a
-forward and `original.hasHtml()` for a reply, on the stated reasoning that an
-HTML part is a fact about the sender's software. That reasoning is sound for
-how the user WRITES and does not decide what the forward CARRIES, which is the
-question here.
-
-**Approach.** The decision comes first; this is not a changed call site.
-
-A forward is a different act from a reply: the point is to hand somebody else
-what arrived, and quoting is the wrong shape for it. Three candidates, in
-increasing fidelity:
-
-- Render `htmlBody` down to text when `plainBody` is empty, so nothing is
- silently lost. The smallest fix, and it does not answer the note: formatting
- is still gone.
-- Carry the original as a `message/rfc822` part, which is what item 130 already
- describes and what GMime builds natively. Perfect fidelity, and every
- attachment comes with it, but the recipient sees an attached message rather
- than a body.
-- Build the forward as `multipart/alternative` with the original's HTML nested
- in the HTML half, which is what Thunderbird's inline forward does.
-
-**Constraints.**
-
-- **The HTML is input from a stranger and the composer is not the message
- pane.** The pane's protections (off-the-record profile, JavaScript off, the
- interceptor blocking every request) are `MessageView`'s, not
- `ComposeWindow`'s. Any route that puts the original's markup into an outgoing
- message must decide what it strips, and remote references in particular:
- forwarding a tracking pixel forwards the tracking to the new recipient.
-- Item 130 overlaps and may subsume this. Decide the two together rather than
- building `message/rfc822` twice.
-- The markdown body is the composer's source of truth, and markdown has no
- syntax for arbitrary HTML the user can then edit. A route that keeps the
- original's markup has to keep it OUTSIDE the editable buffer, which is the
- same nesting problem item 129 carries.
-- `quoteBody()` is shared with Reply. A change there reaches both; the
- behaviour asked for is the forward's alone.
+## 173. The composer is a plain-text editor, not WYSIWYG
+
+**Observed (user, 2026-08-27):** asked for directly while hand-testing item
+171. "This is a GUI mail client and should have a wysiwyg editor."
+
+**Where it came from.** Item 171 forwards an HTML message by carrying the
+original's markup, and the markup cannot be shown in a `QPlainTextEdit`. The
+first build seeded a text quote into the editable buffer and then dropped it
+when building the HTML part, so the user could edit a quote whose edits were
+silently discarded. The user's objection is the right one and is more general
+than that bug: **what the composer shows should be what gets sent.**
+
+171 shipped the middle ground, a read-only preview beside the editor. This
+item is the real answer.
+
+**Cause (verified in the code).** The composer is a `QPlainTextEdit` over
+markdown, deliberately: `OutgoingMessage::markdownBody` is "the source text,
+exactly as typed", `MarkdownRenderer` turns it into HTML at build time, and
+`DraftStore` autosaves that same markdown. There is nowhere in that model for
+a stranger's markup, or for the user's own rich text, to live and be edited.
+
+**Approach.** A rich-text editor for the body, which is what every graphical
+mail client does. Thunderbird is the reference: forwarding HTML opens a
+rich-text composer with the original inside it, editable.
+
+**This is not a widget swap, and the constraints are why it is L.**
+
+- **The draft format changes.** A draft currently round-trips as markdown; a
+ rich-text composer means storing HTML, and a draft written by one and read
+ by the other loses formatting silently. Items 163 and 165 are already about
+ draft identity and are worth settling first.
+- **`QTextEdit`'s HTML subset is narrow.** It is not a browser: real
+ newsletter markup (tables, modern CSS) degrades in it. So a naive swap makes
+ the FIDELITY of a forward worse than what 171 currently sends, while making
+ the editing better. Measure before committing to it.
+- **The formatting toolbar has two masters.** `MarkdownFormat` answers "what
+ does this button do to a selection" over markdown text. A rich-text editor
+ has its own notion, and the toolbar must drive whichever is active without
+ the two disagreeing.
+- **Plain text must stay reachable.** Not every message should be HTML, and
+ the `send_html` config default plus the per-message toggle both already say
+ so. A rich-text composer that cannot produce clean plain text regresses the
+ common case.
+- **The security work does not go away.** A forwarded original is still input
+ from a stranger. Whatever renders it for editing must not fetch remote
+ content, and `HtmlSanitiser` (item 171) is what already answers that.
+
+**It subsumes item 133**, markdown syntax highlighting: that is a
+`QSyntaxHighlighter` over the plain editor, which is the cheap answer to the
+same want ("show me what I am writing"). If this is built, 133 is moot; if
+this is deferred, 133 is the thing to do instead. Do not build both.