From edbf393cb79d20764fd224d32a195f59d19ef743 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 27 Aug 2026 11:51:06 +0200 Subject: fix: flag a saved draft seen so it is not tagged unread DraftStore::write() was called with "D", and it uses the flag string verbatim, so every draft this application wrote landed as :2,D. With maildir.synchronize_flags on, notmuch tags any message lacking the S flag `unread`, and a draft the user authored is seen by definition. The symptom heals itself: the next sync of that folder round-trips the file, adds S, and the tag goes away. Only the newest draft in a folder that has not synced since shows it, which is why it read as intermittent and why measuring an older draft finds nothing wrong. TestComposeWindow::aSavedDraftIsFlaggedSeen() asserts both flags on the written filename, verified failing first against "D". TestMainWindow::anAutosaveWritesADraftAndClearsTheDirtyFlag() asserted endsWith(":2,D"), pinning the whole flag set where its own comment said the point was the draft flag "not left bare", so it failed against the corrected behaviour. It checks for D within the flag set now. Also reconciles the backlog with the user's notes: records the forwarded-HTML defect as item 171, closes item 169 (shipped last session, its row still read open and its section was still in the open file), and records this fix as item 172. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AtUzfNjMD8fiYfamDd3ywW --- CHANGELOG.md | 7 ++ .../2026-08-03-post-0.1.0-usability-closed.md | 83 ++++++++++++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 106 ++++++++++++--------- src/composewindow.cpp | 9 +- tests/test_composewindow.cpp | 54 +++++++++++ tests/test_mainwindow.cpp | 8 +- 6 files changed, 220 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 28e1571..69b09b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,13 @@ point at which they are stable. ### Fixed +- **A draft you wrote was marked unread.** Drafts were written to disk without + the Maildir "seen" flag, and notmuch tags anything without it `unread`, so a + draft you had just typed appeared in the Unread view. It corrected itself + the next time that folder synced, which is why it seemed to happen only + sometimes: only the newest draft, in a folder that had not synced since, + showed it. + - **New mail reached the index but not the window.** The worker never reopened its read-only notmuch handle, so nothing indexed after startup appeared in any query and the application looked like it had stopped syncing. 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 7261bd9..fe212e2 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 @@ -8708,3 +8708,86 @@ 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. + 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 69aac44..fb659ec 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 @@ -242,8 +242,10 @@ taking that too literally. | 166 | Mail you send to your own other account loses `inbox` | defect | S | **done 2026-08-25**, unreleased. `sent_only()` keeps a message only when EVERY file is inside a sent folder, which is what the carve-out's docstring already claimed. No query can express it, measured; the root comes from `database.mail_root`, with a split-index fixture the ordinary layout cannot provide. Verified read-only against the live index: 780 of 807 still stripped, 27 spared, no arrival affected | | 167 | No way to tell one build of an unreleased version from another | enhancement | XS | **done 2026-08-25**, unreleased. The user chose a counter over a git description: `QTMAILDIR_BUILD_NUMBER`, a cmake option ON by default, increments a counter in the BUILD directory on every build and writes `buildnumber.h`. `QTMAILDIR_VERSION_DISPLAY` carries it; `QTMAILDIR_VERSION` stays clean and is what the window title, `applicationVersion` and the release procedure use | | 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 | open, 2026-08-26, from the notes. The accent bar exists (`CardLayout::accentRect`, `CardDelegate::accentLineColour()`); the gradient fade and the sender avatar do not. The avatar's initials source is decided, the vCard half is blocked on item 72 | +| 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 | +| 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 | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1261,49 +1263,6 @@ id and a draft of a reply carries both. replaced correctly now, so the fork this would have mitigated no longer happens by that route. -## 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`. - ## 170. A row that stops matching the view only leaves it on the Delete path **Observed (user, from the notes):** "should we refactor the list UI to be @@ -1353,3 +1312,62 @@ every tag write passes. Move it, or call it from both send paths. thread the query never returned. - 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. diff --git a/src/composewindow.cpp b/src/composewindow.cpp index afcf6a2..9bcfab3 100644 --- a/src/composewindow.cpp +++ b/src/composewindow.cpp @@ -1227,8 +1227,15 @@ bool ComposeWindow::saveDraftNow() account.maildir + QLatin1Char('/') + account.drafts); const QString previousPath = m_draftPath; + + // "DS", not "D". `maildir.synchronize_flags` is on, so notmuch tags any + // message lacking the `S` flag `unread`, and a draft the user just wrote + // is seen by definition. Without the S it lands in the Unread view until + // the folder next syncs, at which point mbsync round-trips the file and + // the flag appears on its own; that self-healing is what made the defect + // look intermittent rather than constant. const DraftStore::Result written = - DraftStore::write(folder, built.bytes, QStringLiteral("D"), m_draftPath); + DraftStore::write(folder, built.bytes, QStringLiteral("DS"), m_draftPath); if (!written.ok()) { // A PERSISTENT banner, not a modal and not a status-bar line that diff --git a/tests/test_composewindow.cpp b/tests/test_composewindow.cpp index 779d48c..0da61ff 100644 --- a/tests/test_composewindow.cpp +++ b/tests/test_composewindow.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -62,6 +63,7 @@ private slots: void onlyTheSetterWritesTheDirtyFlag(); void theMenuBarReachesEveryComposerAction(); void saveDraftWritesAndReports(); + void aSavedDraftIsFlaggedSeen(); void theMenusReuseTheToolbarActions(); void theHtmlMenuItemTracksTheToolbarButton(); void theAgeLineFollowsTheClock(); @@ -746,6 +748,58 @@ void TestComposeWindow::saveDraftWritesAndReports() QVERIFY2(!window.isWindowModified(), "a manual save must clear the marker"); } +/// A draft is authored by the user, so it is SEEN by definition and must never +/// be tagged `unread`. +/// +/// `maildir.synchronize_flags` is on, so the tag is decided by the filename: +/// notmuch tags any message lacking the `S` flag `unread`. Writing a draft as +/// `:2,D` therefore puts it in the Unread view until the folder next syncs, +/// at which point mbsync round-trips the file and the `S` appears. That is +/// what made the defect look intermittent: only the newest draft, in a folder +/// that has not synced since, shows the symptom. Measured on the user's own +/// mail 2026-08-27, where two drafts written two minutes apart differed only +/// in whether their folder had synced afterwards. +/// +/// Asserting on the FILENAME rather than on a notmuch tag is deliberate: the +/// flags are what the code here controls, and a tag assertion would need an +/// indexed database to say the same thing less directly. +void TestComposeWindow::aSavedDraftIsFlaggedSeen() +{ + const Config config = configWithDrafts(); + + ComposeContext context; + context.kind = ComposeContext::Kind::New; + context.accountKey = QStringLiteral("work"); + + ComposeWindow window(context, config, m_dir->path()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + body->setPlainText(QStringLiteral("A draft the user wrote.")); + + QSignalSpy saved(&window, &ComposeWindow::draftSaved); + auto *save = window.findChild(QStringLiteral("compose_save")); + QVERIFY(save); + save->trigger(); + + QCOMPARE(saved.size(), 1); + const QString path = saved.first().first().toString(); + QVERIFY2(!path.isEmpty(), "the save reported no path"); + + // The guard the probe needs: without it a rename that dropped the info + // suffix entirely would pass the S check below by never reaching it. + const QString name = QFileInfo(path).fileName(); + QVERIFY2(name.contains(QStringLiteral(":2,")), + qPrintable(QStringLiteral("no Maildir info suffix in %1").arg(name))); + + const QString flags = name.section(QStringLiteral(":2,"), 1); + QVERIFY2(flags.contains(QLatin1Char('D')), + qPrintable(QStringLiteral("a draft must carry the D flag, got %1") + .arg(flags))); + QVERIFY2(flags.contains(QLatin1Char('S')), + qPrintable(QStringLiteral("a draft must carry the S flag or notmuch " + "tags it unread, got %1").arg(flags))); +} + /// The same QAction objects, shown twice over, exactly as item 140 required /// for the message pane's bar. A copy would drift: an enablement change or a /// new shortcut would reach one surface and not the other. diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index a572fd0..2fbdb20 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -13753,8 +13753,12 @@ void TestMainWindow::anAutosaveWritesADraftAndClearsTheDirtyFlag() QVERIFY(written.open(QIODevice::ReadOnly)); const QByteArray bytes = written.readAll(); QVERIFY2(bytes.contains("Draft body."), "the draft does not carry the body"); - // Written with the Maildir draft flag, not left bare. - QVERIFY2(files.first().endsWith(QStringLiteral(":2,D")), + // Written with the Maildir draft flag, not left bare. The flag SET is not + // pinned here: a draft also carries S, asserted by + // TestComposeWindow::aSavedDraftIsFlaggedSeen(), and an endsWith(":2,D") + // here would fail against that correct behaviour. + QVERIFY2(files.first().section(QStringLiteral(":2,"), 1) + .contains(QLatin1Char('D')), qPrintable(QStringLiteral("wrong maildir flags: ") + files.first())); } -- cgit v1.2.3