diff options
Diffstat (limited to 'docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 481 |
1 files changed, 481 insertions, 0 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 39026cb..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 @@ -8607,3 +8607,484 @@ that instead. **Not built, and left as the item's own recommendation:** writing `P` from a subject heuristic. Rejected on the same grounds the entry gave before the work started. + + +## 119. The unsynced-changes count cannot be opened to see what it counts + +**Observed (user, from the notes):** "the bottom left statusbar message needs to +be clickable and show what 'N unsynced changes' are in a modal window". + +**Cause (verified in the code).** `m_pendingLabel` is a plain `QLabel` added to +the status bar with `addPermanentWidget` (`src/mainwindow.cpp:502-505`). A +`QLabel` has no clicked signal and none is installed, so there is nothing to +click and no route to a list. It carries a tooltip and nothing else. + +**The count is a SUM OVER FOUR SOURCES, and that is what makes this bigger than +it looks.** `pendingEditCount()` returns +`m_pendingTagEdits.size() + m_unnettablePendingEdits + held + heldMoves`. +Three of those can name what they hold: `m_pendingTagEdits` is a +`QHash<QString, bool>` keyed by message id, `m_heldEdits` and `m_heldMoves` are +queues of edits waiting for a sync to end. **`m_unnettablePendingEdits` is a +bare `int`** (`src/mainwindow.h:1248`), deliberately so: it counts confirmed +changes that carry no message ids and therefore cannot be netted against +anything. + +So a dialog built from what is currently kept would list three of the four +groups and then have to account for a remainder it cannot describe. Showing "and +3 more" is worse than the tooltip, because the user opened the window +specifically to find out what those were. + +**Approach.** Two halves, and the second is the real work. + +- The clickable half is small: a label that emits on click (an event filter, or + a flat `QToolButton` styled as a label), plus a dialog listing what the three + describable groups hold. The message pane already resolves an id to a subject. +- The complete half needs `m_unnettablePendingEdits` to become something that + can name its entries. Its comment says why it is an int: understating the + indicator is the direction that costs the user work, so it counts what it + cannot identify rather than dropping it. Making it describable means finding + out what those changes actually are and whether they can carry an id. + +**Constraints.** + +- **The count is deliberately conservative and must stay so.** Item 28 and item + 54 both landed on this indicator being wrong in the direction that made the + user think their work was safe. A dialog that lists fewer changes than the + count claims is the same failure in a new place: reconcile the two, or state + the remainder honestly rather than hiding it. +- **An external `notmuch` run can clear pending changes without this count + noticing**, which the tooltip already admits. A dialog makes that staleness + much more visible, since a listed change may no longer exist. Worth deciding + whether the dialog re-verifies against the database before showing. +- Read-only. This is an information window, not a place to retry or discard a + change; either would be a new mutation path with its own undo question. + +**Size: S** for the clickable half over the three describable groups. **Unknown** +for the fourth, and the item is not complete without it. + +**Closed 2026-08-26.** The blocker above was investigated first and did not +survive: `m_unnettablePendingEdits` counted confirmed changes carrying no +message ids, and `NotmuchWorker::applyTags()` (the only emitter of +`tagsApplied`) returns early on an empty id list, which is that exact +condition. `applyTagsToThreads()` resolves through a query and errors out on +an empty result, so it cannot hand `applyTags()` an empty list either. + +**Measured rather than read**, twice, because reading is what produced the +wrong answer the first time: a `qFatal` in the branch fired in 4 of 70 +`test_mainwindow` cases, all four building a `TagChange` by hand and invoking +the slot directly with no worker, and a `Q_ASSERT` before the worker's own +emit never fired across the whole suite. The counter was deleted and the +guard it shadowed is pinned where it lives, by +`applyTagsWithNoIdsDoesNothing()` in `test_notmuchworker`. + +Built in four commits: the snapshot, the subject resolve, the dialog and the +click, then a sizing fix after a hand test. + +Three decisions the user made, each of which shapes the code: + +- **Scope follows the ACTION, not the storage.** A thread action shows one + thread row with the count of messages it covered; a message action shows + its message. The three queues already encoded this, so nothing is expanded + and nothing is escalated: `HeldEdit` is thread-scoped because a `*_thread` + action made it, and everything else carries message ids. +- **A snapshot, frozen.** Taken at the click and never refreshed under the + user, who asked for exactly this: "if I keep the popup open for 20 minutes, + I don't want the popup to keep updating the info it's showing me." +- **Subjects resolved, stale rows kept.** An id the index no longer holds + still gets a row saying its subject is unknown, because the count the user + clicked has to equal the list they are shown. + +The re-verify question this item worried about mostly dissolved: item 54 +already clears the count when an external sync carries the edits, so a change +applied by cron does not survive to be clicked on. + +`resolvePendingSubjects()` answers POSITIONALLY, one subject per input row, +because one id can legitimately appear on several rows and a combined query +returns a set. `PendingChangeRow::startsMessage` is carried rather than +inferred from a non-empty subject, so an unresolved id still opens a run of +its own instead of folding its actions under the message above it. + +Read-only, per the constraint above. The dialog's height is sized to its +content; that is a hand test, since the offscreen platform returns an +identical frame either way. + + +## 169. A card shows the account only as a bar, with no fade and no avatar + +**Observed (user, from the notes):** "the left border of a card expresses the +account the mail belongs to. the background color of the card should fade left +to right from the account color to the current background color we are using (or +to transparent to work both in light and dark themes). On the left we should +leave room for an account avatar (a squircle), for now it could be extracted +from the sender name "From: john doe" becomes "JD" in the avatar. As soon as we +include khard (or some other vcard provider/manager) we will switch to images if +the corresponding vCard has one." + +**Cause (verified in the code):** not a defect. Half of it shipped. The account +colour is drawn as a solid bar down the left edge, `CardLayout::accentRect` +placed by `CardLayout`, filled by `CardDelegate::paint()` with +`CardDelegate::accentLineColour()`. There is no gradient anywhere on a card, and +nothing draws an avatar: `CardLayout` reserves no rect for one, so the geometry +would have to grow before the painting could. + +**Approach.** Two separable pieces, and the avatar is the one that changes the +layout. + +- The fade is a `QLinearGradient` fill over the card rect, from the accent + colour to the pane's background. `accentLineColour()` already records why + blending toward the background is wrong for a CHIP; a card's background is + exactly where such a blend belongs, so the constraint does not carry over. + Both themes come free if the far stop is the palette's own base rather than + a literal. +- The avatar needs a rect in `CardLayout`, which is where it becomes testable + without a painter, and it shifts `contentLeft` for every card. The initials + come from the display name already carried on the summary; a sender with no + display name (an address only) needs an answer before this is built. + +**Constraints.** + +- The vCard half is blocked on item 72, which is itself unspecified. Build the + initials only; do not design the image path in advance. +- A gradient behind the text has to keep the text readable at the left edge in + both themes, which is the same failure mode `accentLineColour()` guards + against on a dark palette. +- This is a looks question, so it is settled by the user looking at it rather + than by a test: assert the geometry in `CardLayout`, and hand the appearance + over per `tests-only-for-measurable-things`. + +## 172. A draft this application writes is tagged `unread` + +**Observed (user, 2026-08-27):** a draft they had edited was sitting in the +Unread view. Reported first as "in the inbox view", corrected to Unread. + +**Cause (measured, 2026-08-27).** `ComposeWindow::saveDraftNow()` called +`DraftStore::write(folder, bytes, "D", ...)`, and `DraftStore::write()` uses +the flag string verbatim, so every draft this application wrote landed as +`:2,D`. `maildir.synchronize_flags` is on, and notmuch tags any message +lacking the `S` (seen) flag `unread`. A draft the user authored is seen by +definition, so the tag was wrong the moment the file was written. + +**Why it looked intermittent, which is the part worth keeping.** The symptom +heals itself: the next mbsync of that folder round-trips the file, adds `S`, +and the tag goes away. On the developer's own mail two drafts written two +minutes apart differed only in whether their folder had synced afterwards: +one account's drafts folder had synced the next morning and its file read +`,DS`, while the other's had last synced two minutes after the write and read +`,D`. So only the newest draft in a folder that has not synced since shows +it, and an investigation that measures an older draft finds nothing wrong. + +**A measurement trap sat in front of this and cost the first answer.** +`notmuch search --output=tags` reports the union over a THREAD. A reply-draft +attached to an inbox message therefore reads `draft inbox unread` while no +single message carries both, which is the same union recorded for +`ThreadSummary::tags` under item 110. The first pass here read that union as +a draft carrying `inbox` and concluded there was no defect at all. Measure +drafts with `--output=messages`; item 164's evidence is a thread-level +reading and should be re-measured before it is worked on. + +**Fixed** by passing `"DS"`. `TestComposeWindow::aSavedDraftIsFlaggedSeen()` +asserts both flags on the written filename, verified failing first (`got D`). + +`TestMainWindow::anAutosaveWritesADraftAndClearsTheDirtyFlag()` had to be +repaired in the same commit: it asserted `endsWith(":2,D")`, pinning the whole +flag set when its own comment said the point was the draft flag "not left +bare". It therefore failed against the corrected behaviour. An +over-specified assertion of this shape blocks the fix rather than the bug. + +## 164. A draft this application saved keeps `inbox` + +**Observed (developer, 2026-08-25):** `notmuch search --output=tags` on a +draft this application had just written reported `draft inbox unread`. + +**The first cause recorded here was WRONG, and the correction is the useful +part.** It said `strip_inbox_from_sent()` reads a sent-only folder list and +that `qtmaildirconf.py` has no drafts equivalent. Neither is true: + +- `NOT_ARRIVALS` is `("sent", "drafts")`, so `sent_folders()` already returns + both. The name says "sent" and the contents do not, which is what made the + wrong reading plausible. +- Run against the real config it returns every account's drafts folder. +- `notmuch count "(<carve-out query>) and id:<the draft>"` returns **1**. The + query the hook builds MATCHES the affected message. + +So the folder list and the query are correct, and the fix is not there. + +**What is actually established.** + +- The carve-out is scoped to `SCOPE = "tag:new"` (`post-new:106`). +- The affected draft carries `inbox`, and `notmuch count tag:new` is **0**. +- The installed hooks are SYMLINKS into this repository, so the code read is + the code that runs. Verified rather than assumed. +- An mbsync-style rename does **not** re-apply `new.tags`: measured in a + throwaway database, a file renamed to add `,U=4` and reindexed kept the tags + it had. The "the rename retags it" theory is therefore also out. + +**What is NOT established, and must be before any code is written:** which +pass put `inbox` on this file, and why it was not carrying `tag:new` when the +hook's carve-out ran. The likely shape is an ordering one, since item 158 +indexes a draft from the application itself, outside `notmuch new`, and a file +already known to the database is not a new file on the next pass. But that is +a hypothesis and the last two hypotheses here were both wrong. + +**The reproducer was built (2026-08-25) and it settles the mechanism.** Seven +variants were driven in throwaway databases, modelling `indexDraftFile()` with +a real `notmuch_database_index_file` call rather than the CLI, because no CLI +command indexes an untracked path without applying `new.tags`. + +What the sweep established, each measured rather than reasoned: + +- `index_file` applies **no tags at all**. A draft the application indexes is + therefore never in `tag:new` scope, and the hook has nothing to carve out. +- Whenever the file IS in `tag:new` scope, the carve-out strips `inbox` + correctly, in every filename shape tried: `:2,DS`, `:2,D`, no info suffix, + in `cur/` and in `new/`, with and without the `,U=4` infix. The real file's + shape (`,U=4:2,D`) is among them. +- It survives the orderings too: `notmuch new` first then the app's index, + the app's index first then the rename, an autosave landing between + `notmuch new` and the hook, and the stale-path `remove_message` that makes + the renamed file arrive as new mail. All six left the draft clean. +- The `D` flag is what puts `draft` on the message (`synchronize_flags`), and + the `S` flag is what removes `unread`. The affected file is `:2,D`, which is + why it carries `unread`, and that matches the reported tag set exactly. + +**The one variant that reproduces it** is the general shape rather than a +filename detail: a pass where `inbox` is applied while `tag:new` has ALREADY +been consumed. Modelled as a file indexed at a path the carve-out does not +cover and moved into the drafts folder afterwards, it ends in precisely the +live end state, `draft inbox unread` in Drafts with `,U=4` and `tag:new` at 0. +Nothing revisits a message once the marker is gone, so the tag is permanent. + +**What is still NOT established, and the next step.** The affected account +writes drafts straight to `<account>/Drafts`, which the carve-out +covers (verified against the live config and the live query, which matches the +message by id today), so the reproducing variant's premise does not hold for +it as written. The live log for the pass that added it reads + + 10:10:52 Added 1 new message to the database. Detected 9 file renames. + 10:10:52 post-new: sent-folder carve-out applied over 9 folder(s) + +so the hook DID run on that pass, over a path the query covers, and logged +success. The remaining candidates are all about what the path or the marker +looked like at that instant, not about the query text: the carve-out logs +"applied" on a `notmuch tag` that matched zero messages, so a successful log +line is not evidence the message was in scope. Instrumenting the hook to log +the carve-out's MATCH COUNT, and leaving it to run until the next draft, is +the cheapest way to close it, and is a log-only change to code that tags real +mail unattended. + +The filename also rules one thing in: `1787645266.M802P16149Q3.<host>` is +exactly `MaildirName::fresh()` output, so the application wrote this file. It +is not a draft another client left behind. + +The reproducer scripts are throwaway and were not kept; `indexfile.c` is +fifteen lines around one `notmuch_database_index_file` call and is trivial to +rebuild from this entry if the instrumentation points back at the hook. + +**Constraints.** + +- **The hook tags real mail unattended every ten minutes.** Nothing here is + worth a speculative change. +- The 0.27.0 changelog claims sent mail and drafts both stay out of the inbox. + Whatever the cause, that claim is currently false for drafts and the entry + needs correcting with the fix. +- Only `inbox` may be touched. A draft legitimately carries `draft` and + `unread`, and `maildir.synchronize_flags` means removing `unread` rewrites + the filename and reaches the server. +- The hook must keep refusing to consume `tag:new` when a carve-out fails. +- `test_post_new.py` and `test_qtmaildirconf.py` both live beside the hook and + have sent-carve-out tests to copy. + +--- + +**RE-MEASURED 2026-08-27, and the item is DROPPED: there was never an `inbox` +tag on a draft.** Everything above this line is the investigation of a defect +that did not exist, and it is kept because the way it went wrong is worth more +than the conclusion. + +The premise came from `notmuch search --output=tags`, which reports the union +over a THREAD. A draft replying to an arrived message sits in that message's +thread, so the union reads `draft inbox unread` while the two tags live on two +different messages. Measured today on the thread that produced the original +report: + +- the arrived mail: `['account-<acct>', 'inbox']` +- the draft reply: `['draft', 'unread']` + +Neither carries both. Across the whole index, `notmuch count --output=messages +'tag:draft and tag:inbox'` is **0** against 12 drafts, nine of which were +written on or before 2026-08-25 and so were present when this was filed. + +**The trap has a second half that makes it much easier to fall into.** A +thread-level `notmuch count 'tag:draft and tag:inbox'` ALSO returns 0, because +search terms match per message even in a thread query. So the count and the +displayed tag list disagree, and the displayed list is the one that looks like +evidence. Use `--output=messages` and `notmuch show` when asking what tags a +message carries; `--output=tags` answers a different question than it appears +to. + +This is the same union recorded for `ThreadSummary::tags` under item 110, where +it made a card claim a tag its message did not have. It cost this item a week +open, two wrong causes, and a seven-variant reproducer built to explain an end +state that a union produces for free. It also caught a fresh reader of this +backlog on 2026-08-27, who read the same union and reported that drafts were +carrying `inbox` before measuring at message level. + +**The `unread` half of the original observation WAS real** and is item 172: the +app wrote drafts as `:2,D`, and notmuch tags anything without `S` as `unread`. +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. + |