diff options
| -rw-r--r-- | CHANGELOG.md | 7 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 83 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 106 | ||||
| -rw-r--r-- | src/composewindow.cpp | 9 | ||||
| -rw-r--r-- | tests/test_composewindow.cpp | 54 | ||||
| -rw-r--r-- | 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 <QComboBox> #include <QDir> #include <QFile> +#include <QFileInfo> #include <QMenu> #include <QPlainTextEdit> #include <QSignalSpy> @@ -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<QPlainTextEdit *>(QStringLiteral("body")); + QVERIFY(body); + body->setPlainText(QStringLiteral("A draft the user wrote.")); + + QSignalSpy saved(&window, &ComposeWindow::draftSaved); + auto *save = window.findChild<QAction *>(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())); } |