From 51b8fd5708d23108d532be1cf38e4bb619eeb777 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sun, 6 Sep 2026 15:26:31 +0200 Subject: fix: keep one Message-ID across a draft's revisions Every autosave called MessageBuilder::build(), which generated a fresh Message-ID unconditionally, so each revision of a draft was a different message rather than a new version of one. The entry called this invisible while the file is replaced correctly, and that turned out to be wrong: mbsync uploads each revision to the drafts folder before the next save removes the local file, so the server keeps one message per revision and syncs them all back down. Measured on real mail as four independent messages for a single reply, all four carrying a ,U= infix, threading into the conversation and putting a draft tag on a Sent row. Deleting a local file does not retract an uploaded one, which is why the local cleanup, which is correct, could never fix it. The user chose a stable id while drafting, discarded at send: the sent copy is a different item from the draft, and the draft is deleted once the message goes out, which the code already did. Three links, none of which existed. OutgoingMessage::messageId is the field, where empty means generate, so the send path is unchanged by construction rather than by remembering to clear it. MessageBuilder::build() uses a supplied id when there is one. ComposeWindow::m_draftMessageId holds the identity between revisions, assigned from built.messageId so the first save adopts the id GMime just generated, and ComposeContext::draftMessageId carries it across a reopen, read in forDraft() from ParsedMessage::messageId, which the parser already provided and nothing had ever used. Five tests, because the property spans three objects and a test at any one of them passes while another link is broken. Two are the safety constraint rather than the feature: a field defaulting to a fixed value would satisfy the reuse test and make two sent messages share an id, which is far worse than the defect this fixes. One comment is corrected rather than left: the autosave's dirty check justified comparing the message rather than the built bytes with "GMime is given a fresh Date and Message-ID on every build". Half of that is no longer true. The Date still is, so the conclusion stands. Revisions already on the server are not touched by this; the four found on real mail were deleted by hand. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jq9gXquUo9W4KXDagJXMmn --- CHANGELOG.md | 10 ++ .../2026-08-03-post-0.1.0-usability-closed.md | 158 +++++++++++++++++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 106 +------------- src/composecontext.cpp | 5 + src/composewindow.cpp | 29 +++- src/composewindow.h | 11 ++ src/messagebuilder.cpp | 17 ++- src/types.h | 27 ++++ tests/test_composewindow.cpp | 93 ++++++++++++ tests/test_mainwindow.cpp | 48 +++++++ tests/test_messagebuilder.cpp | 43 ++++++ 11 files changed, 436 insertions(+), 111 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2749e0b..f024916 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,16 @@ point at which they are stable. ### Fixed +- **A draft no longer becomes a new message every time it is saved.** Each + autosave built the draft under a fresh Message-ID, and because mbsync + uploads each revision to the drafts folder before the next save removes the + local file, the server ended up holding one message per revision. Four + revisions of a single reply were found that way on real mail, all four + syncing back down and threading into the conversation. A draft now keeps one + identity across its revisions, including across being closed and reopened, + so a save replaces the message rather than adding one. The sent copy still + gets an id of its own: it is a different item from the draft, and the draft + is deleted once the message goes out. - **The Sent and Drafts views show every message you sent in a conversation, not just the first.** Both views are lists of your own messages rather than of conversations, but a thread you had replied to twice produced a single 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 af1de0c..8de26f2 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 @@ -9941,3 +9941,161 @@ sorts. **Still open and separate:** the four autosave revisions of that reply sitting in `Drafts/` with distinct Message-IDs, which is item 165 and is what puts a `draft` chip on the conversation. + +--- + +## 165. A draft gets a new Message-ID on every autosave + +**Observed (developer, 2026-08-25), while hand-testing items 163 and 164.** +Four saved drafts produced four distinct Message-IDs, and one reopen-and-edit +turned one id into another. A draft therefore has no stable identity across +its own revisions. + +**Cause (verified in the code, not inferred).** `MessageBuilder::build()` +calls `g_mime_utils_generate_message_id()` unconditionally on every call +(`messagebuilder.cpp:311`), and every autosave calls `build()`. +`OutgoingMessage` has no field to carry an existing id in, and +`ComposeContext` has no field for the draft's OWN id either: it carries +`inReplyTo` and `references`, which are the ORIGINAL's id when replying, and +`ComposeContextBuilder::forDraft()` never reads the draft's Message-ID back +out of the file it parses. So this is not a changed call site; it needs a +field that does not exist yet, threaded from `forDraft()` through +`ComposeContext` and `OutgoingMessage` into `build()`. + +**Why it matters, and why it is NOT urgent.** To notmuch and to the server, +each revision is a different MESSAGE, not a new version of one. While the +file is replaced correctly this is invisible: one file in, one file out. It +becomes visible whenever a revision is NOT replaced, and item 163 is the +proof, where a stale path forked a draft into two files that were also two +messages and that nothing will ever collapse. Item 163's fix removes the +known way to reach that state; this entry is about the property that turned a +one-file mistake into a two-message one. + +An interrupted save is the remaining route: `DraftStore::write()` unlinks the +previous revision only AFTER the new file is safely in place (deliberately, +so a failed write cannot lose the draft), so a crash between the two leaves +two files, and with two ids they are two drafts rather than one duplicated. + +**Decided by the user on 2026-09-06: a stable id while drafting, DISCARDED at +send.** The second of the three positions below. A draft keeps one identity +across all its revisions, so each save replaces the previous message rather +than adding one; the sent copy is minted fresh and is a different item from +the draft, which is the user's own framing. The draft is DELETED on a +successful send, as it already is today, and the sent message carries no +`draft` tag. + +The three positions, kept because the reasoning is what makes the choice +reviewable: + +- A stable id reused at send time makes the draft and the sent message one + message, which is what a user means by "my draft became this email". It + also means the id was in a file mbsync uploaded to the drafts folder before + the message was ever sent, and the server has seen it. **Not chosen.** +- A stable id DISCARDED at send time keeps revisions collapsed while drafting + and mints a fresh id for the sent copy. Two identities, and the sent one is + the one that threads. **Chosen.** +- The current behaviour is a third position, and its only virtue is that no + id is ever reused for two different things. + +Whichever is chosen must be checked against `In-Reply-To`/`References` on the +eventual send, since `referencesForReply()` builds those from the ORIGINAL's +id and a draft of a reply carries both. + +**What is already built, measured 2026-09-06 rather than assumed.** Two of the +three things the decision describes exist: + +- `DraftStore::write()` removes the previous revision after the new file is in + place (`draftstore.cpp:78`), so "discard what is left after a successful + save" is done LOCALLY. +- `ComposeWindow` removes the draft on a successful send + (`composewindow.cpp:1620`, through `resolveRenamed()` since item 163), so + "delete the draft on send" is done, and the user confirmed that is the + wanted behaviour rather than keeping the last revision. +- The sent copy is built fresh and filed to the Sent folder, so it never + carries `draft`. The tag the user saw on a Sent row came from the orphan + revisions threading into the conversation, not from the sent message. **No + work is needed for that clause**; it was already true. + +**So the whole remaining defect is the id, and the mechanism is the server.** +Measured on the user's own mail: four drafts of one reply, four distinct +Message-IDs, and every one of the four files carrying a `,U=` infix. mbsync +had uploaded each revision to the drafts folder BEFORE the next save removed +the local file, so the server holds four independent messages and syncs all +four back down. The local cleanup is correct and cannot help: the damage +happens between a save and the next sync, and deleting a local file does not +retract an uploaded one. + +This is what raises the item above "invisible while the file is replaced +correctly". The file IS replaced correctly and four messages still exist. +A stable id gives the server one message being revised rather than four +unrelated ones. + +**Constraints.** + +- **A Message-ID reaches the server and every recipient**, so a reused id is + not a local matter. Two different messages sharing an id is worse than two + ids for one draft, which is what makes the current behaviour defensible as + a default rather than simply wrong. +- `MessageBuilder::build()` is on the SEND path as well as the autosave path. + A change that makes ids stable must not make two different sent messages + share one. +- The comment at `messagebuilder.cpp:297` records that GMime generates + neither Date nor Message-ID unless asked, and that a message without one + cannot be threaded by anything receiving it, this application's own index + of the sent copy included. Any "just omit it while drafting" variant has to + answer that. +- Item 163's fix stands on its own and this does not block it: the file is + replaced correctly now, so the fork this would have mitigated no longer + happens by that route. + +**Built 2026-09-06.** Three links, each one a place the id had to travel and +each with its own test: + +- `OutgoingMessage::messageId` is the field that did not exist. Empty means + "mint a fresh one", so the SEND path is unchanged by construction rather + than by remembering to clear it. +- `MessageBuilder::build()` uses a supplied id when there is one and generates + otherwise. `g_mime_message_set_message_id` takes the id WITHOUT angle + brackets and adds them, which is the form `generate_message_id` returns, so + no unwrapping is needed. +- `ComposeWindow::m_draftMessageId` holds the identity between revisions, + assigned from `built.messageId` after a successful save so the FIRST save + adopts the id GMime just generated. `ComposeContext::draftMessageId` carries + it across a reopen, read in `forDraft()` from `ParsedMessage::messageId`, + which the parser already provided and nothing had ever used. + +**One comment had to be corrected rather than left alone**, and it is the kind +that rots into a false claim: the autosave's dirty check explains why it +compares the MESSAGE rather than the built bytes, and its reason was "GMime is +given a fresh Date and Message-ID on every build". Half of that is no longer +true. The Date still is, so the conclusion stands and only the reasoning +needed fixing. + +**Tests, and why there are four.** The property spans three objects, and a test +at any one of them passes while another link is broken: + +- `aSuppliedMessageIdIsUsedRatherThanAFreshOne` builds TWICE, because a single + build cannot distinguish a reused id from a freshly generated one. +- `twoBuildsWithNoSuppliedIdStillDiffer` is the safety constraint: a field + defaulting to some fixed value would satisfy the test above and make two + sent messages share an id. +- `everyRevisionOfADraftKeepsOneMessageId` is the end-to-end property, over + the real files. It changes the body between saves deliberately: without a + real change the dirty check short-circuits, no second file is written, and + the test would pass while proving nothing. +- `aResumedDraftKeepsTheMessageIdItWasSavedUnder` covers the reopen, which the + composer-only tests cannot reach, and asserts on the file written AFTER the + reopen rather than only on the context, since the context carrying the id is + half a chain. +- `aSentMessageDoesNotInheritTheDraftsMessageId` pins the constraint at the + place a future edit would break it, using the already-public + `currentMessage()` rather than adding a test accessor. + +Both wiring changes were mutation-checked: removing the autosave's assignment +fails the revision test, removing `forDraft()`'s read fails the reopen test. + +**What this does NOT do, deliberately.** It does not clean up revisions already +on the server. Those are existing messages in the drafts folder, and nothing +here retracts them; the four found on the user's own mail were deleted by hand +as a separate act. + 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 0029092..7267ea8 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 @@ -238,7 +238,7 @@ taking that too literally. | 162 | Delete fails while a sync is renaming the file underneath it | defect | S | **done, 2026-08-25.** mbsync renames an uploaded file to add its `,U=` infix and notmuch keeps the pre-`U=` name until that sync's `notmuch new` runs, so `moveMessages` renamed a path that no longer existed and Delete silently did nothing while blaming the destination folder. `moveMessages` now re-resolves by MESSAGE ID when the recorded path is gone: one reindex of that directory, then the filename that exists on disk. Bounded to one retry, so a file genuinely gone still reports. Holding the move during a sync was the other candidate and is NOT the fix: `sendMove` already refuses on notmuch's write lock, but this window sits between mbsync's rename and that sync's `notmuch new`, which touches no lock | | 163 | The message pane shows a stale path, and the composer forks the draft | defect | S | **done, 2026-08-25.** mbsync renames an uploaded file to add its `,U=` infix while the model still holds the name the query returned. `MaildirName::resolveRenamed()` returns the path unchanged when it exists, else finds the file in that one directory whose unique stem matches; it refuses an ambiguous match and yields nothing for a genuinely missing file. Wired into all THREE read sites: the pane, Reply/Forward, and the draft reopen. The reopen was the one that cost data, forking a draft into two files with two Message-IDs, both reaching the server | | 164 | A draft this application saved keeps `inbox` | defect | S | **dropped** 2026-08-27, NOT A DEFECT. The premise was a measurement artifact: its evidence was `notmuch search --output=tags`, which DISPLAYS the union over a thread, and a reply-draft under an arrived message reads `draft inbox unread` while no message carries both. Re-measured at message level: 0 of 12 drafts carry `inbox`, including nine written on or before 2026-08-25. The `unread` half was real and is item 172 | -| 165 | A draft gets a new Message-ID on every autosave | defect | S | open, 2026-08-25, **decided 2026-09-06**: a stable id while drafting, discarded at send, so the sent copy is a different item from the draft. Reclassified from enhancement after the user hit it: four revisions of one reply reached the server as four messages, because mbsync uploads each before the next save removes it locally, and they then thread into the conversation and put a `draft` tag on a Sent row. The local cleanup and the delete-on-send are already built and correct; the id is the whole remaining defect. Needs a field threaded from `forDraft()` through `ComposeContext` and `OutgoingMessage` into `build()` | +| 165 | A draft gets a new Message-ID on every autosave | defect | S | **done 2026-09-06**, unreleased. A draft keeps one identity across its revisions and across a reopen, so a save replaces the message rather than adding one; the sent copy still mints its own, at the user's decision. `OutgoingMessage::messageId` empty means "generate", so the send path is unchanged by construction. The four revisions already on the server were deleted by hand, not by code. Section in the closed file | | 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 . **Its rule is now incomplete, see item 178** (2026-08-28): the check still judges a row on `firstMessagePath`, which item 177 stopped being a conversation row's identity, so a conversation whose messages disagree about the trash answers on one of them | @@ -1115,109 +1115,7 @@ The 70-second duration recorded above fits a `QTRY_*` waiting for a file that is never going to appear, which is consistent with a wrong destination rather than a slow one. -## 165. A draft gets a new Message-ID on every autosave - -**Observed (developer, 2026-08-25), while hand-testing items 163 and 164.** -Four saved drafts produced four distinct Message-IDs, and one reopen-and-edit -turned one id into another. A draft therefore has no stable identity across -its own revisions. - -**Cause (verified in the code, not inferred).** `MessageBuilder::build()` -calls `g_mime_utils_generate_message_id()` unconditionally on every call -(`messagebuilder.cpp:311`), and every autosave calls `build()`. -`OutgoingMessage` has no field to carry an existing id in, and -`ComposeContext` has no field for the draft's OWN id either: it carries -`inReplyTo` and `references`, which are the ORIGINAL's id when replying, and -`ComposeContextBuilder::forDraft()` never reads the draft's Message-ID back -out of the file it parses. So this is not a changed call site; it needs a -field that does not exist yet, threaded from `forDraft()` through -`ComposeContext` and `OutgoingMessage` into `build()`. - -**Why it matters, and why it is NOT urgent.** To notmuch and to the server, -each revision is a different MESSAGE, not a new version of one. While the -file is replaced correctly this is invisible: one file in, one file out. It -becomes visible whenever a revision is NOT replaced, and item 163 is the -proof, where a stale path forked a draft into two files that were also two -messages and that nothing will ever collapse. Item 163's fix removes the -known way to reach that state; this entry is about the property that turned a -one-file mistake into a two-message one. - -An interrupted save is the remaining route: `DraftStore::write()` unlinks the -previous revision only AFTER the new file is safely in place (deliberately, -so a failed write cannot lose the draft), so a crash between the two leaves -two files, and with two ids they are two drafts rather than one duplicated. - -**Decided by the user on 2026-09-06: a stable id while drafting, DISCARDED at -send.** The second of the three positions below. A draft keeps one identity -across all its revisions, so each save replaces the previous message rather -than adding one; the sent copy is minted fresh and is a different item from -the draft, which is the user's own framing. The draft is DELETED on a -successful send, as it already is today, and the sent message carries no -`draft` tag. - -The three positions, kept because the reasoning is what makes the choice -reviewable: - -- A stable id reused at send time makes the draft and the sent message one - message, which is what a user means by "my draft became this email". It - also means the id was in a file mbsync uploaded to the drafts folder before - the message was ever sent, and the server has seen it. **Not chosen.** -- A stable id DISCARDED at send time keeps revisions collapsed while drafting - and mints a fresh id for the sent copy. Two identities, and the sent one is - the one that threads. **Chosen.** -- The current behaviour is a third position, and its only virtue is that no - id is ever reused for two different things. - -Whichever is chosen must be checked against `In-Reply-To`/`References` on the -eventual send, since `referencesForReply()` builds those from the ORIGINAL's -id and a draft of a reply carries both. - -**What is already built, measured 2026-09-06 rather than assumed.** Two of the -three things the decision describes exist: - -- `DraftStore::write()` removes the previous revision after the new file is in - place (`draftstore.cpp:78`), so "discard what is left after a successful - save" is done LOCALLY. -- `ComposeWindow` removes the draft on a successful send - (`composewindow.cpp:1620`, through `resolveRenamed()` since item 163), so - "delete the draft on send" is done, and the user confirmed that is the - wanted behaviour rather than keeping the last revision. -- The sent copy is built fresh and filed to the Sent folder, so it never - carries `draft`. The tag the user saw on a Sent row came from the orphan - revisions threading into the conversation, not from the sent message. **No - work is needed for that clause**; it was already true. - -**So the whole remaining defect is the id, and the mechanism is the server.** -Measured on the user's own mail: four drafts of one reply, four distinct -Message-IDs, and every one of the four files carrying a `,U=` infix. mbsync -had uploaded each revision to the drafts folder BEFORE the next save removed -the local file, so the server holds four independent messages and syncs all -four back down. The local cleanup is correct and cannot help: the damage -happens between a save and the next sync, and deleting a local file does not -retract an uploaded one. - -This is what raises the item above "invisible while the file is replaced -correctly". The file IS replaced correctly and four messages still exist. -A stable id gives the server one message being revised rather than four -unrelated ones. - -**Constraints.** - -- **A Message-ID reaches the server and every recipient**, so a reused id is - not a local matter. Two different messages sharing an id is worse than two - ids for one draft, which is what makes the current behaviour defensible as - a default rather than simply wrong. -- `MessageBuilder::build()` is on the SEND path as well as the autosave path. - A change that makes ids stable must not make two different sent messages - share one. -- The comment at `messagebuilder.cpp:297` records that GMime generates - neither Date nor Message-ID unless asked, and that a message without one - cannot be threaded by anything receiving it, this application's own index - of the sent copy included. Any "just omit it while drafting" variant has to - answer that. -- Item 163's fix stands on its own and this does not block it: the file is - replaced correctly now, so the fork this would have mitigated no longer - happens by that route. +--- ## 173. The composer is a plain-text editor, not WYSIWYG diff --git a/src/composecontext.cpp b/src/composecontext.cpp index b02b790..e594e24 100644 --- a/src/composecontext.cpp +++ b/src/composecontext.cpp @@ -572,6 +572,11 @@ ComposeContext ComposeContextBuilder::forDraft(const Config &config, // arriving one step later. context.draftPath = resolved; + // Item 165. The identity this draft already has, so the next autosave + // REPLACES the message on the server rather than minting a second one. + // The parser already reads this header; nothing used it until now. + context.draftMessageId = draft.messageId; + const auto addresses = [](const QString &header) { QStringList out; for (const Recipient &recipient : parseAddressHeader(header)) diff --git a/src/composewindow.cpp b/src/composewindow.cpp index 7952efa..218edef 100644 --- a/src/composewindow.cpp +++ b/src/composewindow.cpp @@ -902,6 +902,9 @@ void ComposeWindow::seedFields() // The draft file this composer is resuming, so the next autosave REPLACES // it rather than writing a second one beside it. m_draftPath = m_context.draftPath; + // And the identity that file already carries (item 165), so resuming does + // not start a second one. + m_draftMessageId = m_context.draftMessageId; // New and Forward seed from [compose] send_html; Reply and Reply-all seed // from whether the original carried a text/html part, ignoring the config @@ -1365,7 +1368,10 @@ bool ComposeWindow::saveDraftNow() return true; } - const OutgoingMessage message = currentMessage(); + // Not const: the draft's own Message-ID is assigned below (item 165). + // fingerprintOf() does not read that field, so the dirty check above is + // unaffected by it. + OutgoingMessage message = currentMessage(); // The dirty CHECK, not just the flag: an unchanged message means no file // is written and no sync is provoked. Every autosave produces a Maildir @@ -1374,10 +1380,13 @@ bool ComposeWindow::saveDraftNow() // // Checked BEFORE the build, and on the message rather than on the bytes. // The plan's draft compared built.bytes, which can never match: GMime is - // given a fresh Date and Message-ID on every build, so two builds of an - // unchanged message differ. That check would have read as working while - // writing a file on every debounce. Doing it first also skips the - // blocking build entirely for the no-change case, which is the common one. + // given a fresh Date on every build, so two builds of an unchanged message + // differ. That check would have read as working while writing a file on + // every debounce. Doing it first also skips the blocking build entirely + // for the no-change case, which is the common one. + // + // The Message-ID is no longer part of that difference since item 165, but + // the Date still is, so comparing bytes remains wrong. const QString fingerprint = fingerprintOf(message); if (!m_savedFingerprint.isEmpty() && fingerprint == m_savedFingerprint) { setDirty(false); @@ -1391,6 +1400,12 @@ bool ComposeWindow::saveDraftNow() // crosses the worker boundary, and a second threading model for one call // is worse than the stall. If someone is measuring a composer freeze, this // line is where to look. + // Item 165. Every revision of this draft builds under ONE id, so a save + // replaces the message on the server rather than adding one. Empty on the + // first save of a new draft, which mints one below; seeded from the file + // for a resumed draft. The SEND path deliberately does not do this. + message.messageId = m_draftMessageId; + const MessageBuilder::Result built = MessageBuilder::build(message, account); if (!built.ok()) { m_saveFailed = true; @@ -1426,6 +1441,10 @@ bool ComposeWindow::saveDraftNow() } m_draftPath = written.path; + // Kept for the NEXT revision (item 165). Assigned from the build rather + // than from m_draftMessageId so the first save adopts the id GMime just + // generated, which is what makes every later save a replacement. + m_draftMessageId = built.messageId; m_savedFingerprint = fingerprint; setDirty(false); m_saveFailed = false; diff --git a/src/composewindow.h b/src/composewindow.h index 9affce1..87b105a 100644 --- a/src/composewindow.h +++ b/src/composewindow.h @@ -371,6 +371,17 @@ private: QString m_draftPath; ///< The revision on disk, unlinked on the next write. + /// The Message-ID this draft's revisions share, empty until the first + /// save. + /// + /// Item 165. Fed back into the next build so a save REPLACES the message + /// mbsync uploaded rather than adding one; without it the server kept one + /// message per revision, measured as four for a single reply. Seeded from + /// the file when a draft is resumed, so reopening does not start a second + /// identity. Deliberately NOT used on the send path: the sent copy is a + /// different item and mints its own id. + QString m_draftMessageId; + /// A fingerprint of the message the last successful save wrote, for the /// dirty CHECK. /// diff --git a/src/messagebuilder.cpp b/src/messagebuilder.cpp index fe23862..ea428ee 100644 --- a/src/messagebuilder.cpp +++ b/src/messagebuilder.cpp @@ -308,8 +308,21 @@ Result build(const OutgoingMessage &message, const Account &account) // invariant the next early return forgets; it is assigned once, beside the // bytes, on the one path that succeeds. QString messageId; - char *generatedId = g_mime_utils_generate_message_id(domainUtf8.constData()); - if (generatedId) { + if (!message.messageId.isEmpty()) { + // Item 165. A draft revision reuses the id its previous build + // returned, so a save REPLACES the message on the server rather than + // adding one. Only the autosave path supplies this; a send leaves it + // empty and gets a fresh id below, because two sent messages must + // never share one. + // + // set_message_id takes the id WITHOUT angle brackets and adds them, + // which is the form generate_message_id returns and the form this is + // stored in. + const QByteArray supplied = message.messageId.toUtf8(); + g_mime_message_set_message_id(mime, supplied.constData()); + messageId = message.messageId; + } else if (char *generatedId = + g_mime_utils_generate_message_id(domainUtf8.constData())) { g_mime_message_set_message_id(mime, generatedId); messageId = QString::fromUtf8(generatedId); g_free(generatedId); diff --git a/src/types.h b/src/types.h index ee2627f..fb7901c 100644 --- a/src/types.h +++ b/src/types.h @@ -352,6 +352,15 @@ struct ComposeContext /// to make room. It is the message itself. QString body; + /// The Message-ID the resumed draft already has, empty for every other + /// kind. + /// + /// Item 165. Seeded into ComposeWindow::m_draftMessageId so a reopened + /// draft keeps the identity it was saved under instead of starting a + /// second one on its next autosave. Distinct from `inReplyTo`, which is + /// the ORIGINAL's id when the draft is a reply; this is the draft's own. + QString draftMessageId; + /// The draft file this composer OWNS, empty for every other kind. /// /// Seeded into ComposeWindow::m_draftPath so the next autosave REPLACES @@ -379,6 +388,24 @@ struct OutgoingMessage QString inReplyTo; QStringList references; + /// The Message-ID to build under, or empty to mint a fresh one. + /// + /// Item 165. A draft keeps ONE identity across its revisions, so an + /// autosave replaces the message it wrote last time instead of adding + /// another. Every save used to generate a new id, and mbsync uploads each + /// revision to the drafts folder before the next save removes the local + /// file, so the server ended up holding one message per revision: + /// measured as four for a single reply on the user's own mail. Deleting a + /// local file does not retract an uploaded one, which is why the local + /// cleanup, which is correct, could never fix this. + /// + /// **Empty on the SEND path, deliberately.** The sent copy is a different + /// item from the draft, at the user's own decision (item 165), and two + /// sent messages sharing an id would be far worse than two ids for one + /// draft. So this is opt-in: only ComposeWindow's autosave fills it, from + /// the id the previous build returned. + QString messageId; + /// Item 171. The forwarded original's HTML, already sanitised, appended to /// the HTML alternative below the user's own text. /// diff --git a/tests/test_composewindow.cpp b/tests/test_composewindow.cpp index d95d55d..49bb390 100644 --- a/tests/test_composewindow.cpp +++ b/tests/test_composewindow.cpp @@ -66,6 +66,8 @@ private slots: void theMenuBarReachesEveryComposerAction(); void saveDraftWritesAndReports(); void aSavedDraftIsFlaggedSeen(); + void everyRevisionOfADraftKeepsOneMessageId(); + void aSentMessageDoesNotInheritTheDraftsMessageId(); void aForwardCarriesTheOriginalHtmlAndStripsRemoteContent(); void anHtmlForwardPreviewsTheOriginalInsteadOfQuotingIt(); void theMenusReuseTheToolbarActions(); @@ -752,6 +754,97 @@ void TestComposeWindow::saveDraftWritesAndReports() QVERIFY2(!window.isWindowModified(), "a manual save must clear the marker"); } +/// Item 165. Two saves of one draft must produce ONE message, not two. +/// +/// Every save used to mint a fresh Message-ID, and mbsync uploads each +/// revision to the drafts folder before the next save removes the local file, +/// so the server kept one message per revision: measured on the user's own +/// mail as four independent messages for a single reply, which then threaded +/// into the conversation and put a `draft` tag on a Sent row. Deleting a local +/// file does not retract an uploaded one, so the local cleanup, which is +/// correct, could never fix this. +/// +/// Asserted on the FILE's header rather than on a notmuch tag, for the reason +/// the test below gives: the bytes are what the code here controls. +void TestComposeWindow::everyRevisionOfADraftKeepsOneMessageId() +{ + 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); + auto *save = window.findChild(QStringLiteral("compose_save")); + QVERIFY(save); + + const auto messageIdIn = [](const QString &path) { + QFile file(path); + if (!file.open(QIODevice::ReadOnly)) + return QString(); + const QString text = QString::fromUtf8(file.readAll()); + for (const QString &line : text.split(QLatin1Char('\n'))) { + if (line.startsWith(QStringLiteral("Message-Id:"), Qt::CaseInsensitive)) + return line.section(QLatin1Char(':'), 1).trimmed(); + if (line.trimmed().isEmpty()) + break; // end of headers + } + return QString(); + }; + + QSignalSpy saved(&window, &ComposeWindow::draftSaved); + + body->setPlainText(QStringLiteral("First revision.")); + save->trigger(); + QCOMPARE(saved.size(), 1); + const QString firstPath = saved.at(0).at(0).toString(); + const QString firstId = messageIdIn(firstPath); + QVERIFY2(!firstId.isEmpty(), "the first revision carries no Message-ID"); + + // A REAL change, or the dirty check short-circuits and no second file is + // written at all, which would pass this test while proving nothing. + body->setPlainText(QStringLiteral("Second revision, genuinely different.")); + save->trigger(); + QCOMPARE(saved.size(), 2); + const QString secondPath = saved.at(1).at(0).toString(); + QVERIFY2(secondPath != firstPath, + "the second save wrote no new file, so the ids cannot be compared"); + + QCOMPARE(messageIdIn(secondPath), firstId); +} + +/// The constraint that makes item 165 safe, and the one a future edit is most +/// likely to break: the SEND path must mint its own id. +/// +/// The user's decision was a stable id while drafting, DISCARDED at send, so +/// the sent copy is a different item from the draft. Two sent messages sharing +/// a Message-ID would be far worse than the defect this fixed, and a +/// Message-ID reaches the server and every recipient, so it is not a local +/// matter. currentMessage() leaves the field empty and the send path passes it +/// straight to build(); this pins that. +void TestComposeWindow::aSentMessageDoesNotInheritTheDraftsMessageId() +{ + const Config config = configWithDrafts(); + + ComposeContext context; + context.kind = ComposeContext::Kind::Draft; + context.accountKey = QStringLiteral("work"); + context.draftMessageId = QStringLiteral("the-draft-id@example.org"); + + ComposeWindow window(context, config, m_dir->path()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + body->setPlainText(QStringLiteral("About to send.")); + + // The message the SEND path builds from, which is what decides the id. + const OutgoingMessage outgoing = window.currentMessage(); + QVERIFY2(outgoing.messageId.isEmpty(), + "the send path carried the draft's Message-ID, so the sent copy " + "would share an id with a message already on the server"); +} + /// A draft is authored by the user, so it is SEEN by definition and must never /// be tagged `unread`. /// diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 5817ac4..f20701e 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -518,6 +518,7 @@ private slots: void doubleClickingADraftOpensTheComposer(); void aResumedDraftReplacesItsFileRatherThanAddingOne(); void aResumedDraftKeepsItsBlindRecipients(); + void aResumedDraftKeepsTheMessageIdItWasSavedUnder(); void aDraftRenamedByASyncStillReopensAndReplacesItsFile(); void theComposerSplitsItsToolbarByScope(); void ccAndBccHideBehindADisclosure(); @@ -14313,6 +14314,53 @@ void TestMainWindow::aDraftRenamedByASyncStillReopensAndReplacesItsFile() "into two files and both would reach the server"); } +/// Item 165, the third link in the chain and the one a composer-only test +/// cannot reach: reopening a draft must keep the identity the FILE already +/// has, or the next autosave starts a second one and the server ends up with +/// two messages for one draft after all. +void TestMainWindow::aResumedDraftKeepsTheMessageIdItWasSavedUnder() +{ + ComposeFixture fixture; + QVERIFY(fixture.build()); + + OutgoingMessage message; + message.accountKey = QStringLiteral("acct"); + message.to = { QStringLiteral("someone@example.org") }; + message.subject = QStringLiteral("Resumed"); + message.markdownBody = QStringLiteral("Body."); + message.messageId = QStringLiteral("already-saved-under@example.org"); + + const QString folder = fixture.mailRoot() + QStringLiteral("/acct/Drafts"); + const QString path = writeDraftFile(folder, message, + fixture.config().account( + QStringLiteral("acct"))); + QVERIFY(!path.isEmpty()); + + const ComposeContext context = + ComposeContextBuilder::forDraft(fixture.config(), path); + QCOMPARE(context.draftMessageId, + QStringLiteral("already-saved-under@example.org")); + + // And it reaches the next revision, which is the property that matters: + // the context carrying it is only half the chain. + ComposeWindow window(context, fixture.config(), fixture.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + body->setPlainText(QStringLiteral("Edited after reopening.")); + + QSignalSpy saved(&window, &ComposeWindow::draftSaved); + auto *save = window.findChild(QStringLiteral("compose_save")); + QVERIFY(save); + save->trigger(); + QCOMPARE(saved.size(), 1); + + QFile written(saved.first().first().toString()); + QVERIFY(written.open(QIODevice::ReadOnly)); + const QString text = QString::fromUtf8(written.readAll()); + QVERIFY2(text.contains(QStringLiteral("")), + "the revision written after a reopen carries a different id"); +} + void TestMainWindow::aResumedDraftKeepsItsBlindRecipients() { // MessageBuilder writes Bcc into the draft file deliberately, and says diff --git a/tests/test_messagebuilder.cpp b/tests/test_messagebuilder.cpp index 2ea14f0..56cf6a9 100644 --- a/tests/test_messagebuilder.cpp +++ b/tests/test_messagebuilder.cpp @@ -56,6 +56,8 @@ private slots: void aDirectoryAttachmentFailsRatherThanHangingTheProcess(); void anUnparseableRecipientFailsRatherThanVanishing(); void everyMessageCarriesADateAndMessageId(); + void aSuppliedMessageIdIsUsedRatherThanAFreshOne(); + void twoBuildsWithNoSuppliedIdStillDiffer(); void aForwardSendsOnePartChosenByTheHtmlToggle(); void recipientsAppearInTheirOwnHeaders(); void anAccountWithNoAddressFailsRatherThanBuildingHeaderlessMail(); @@ -419,6 +421,47 @@ void TestMessageBuilder::everyMessageCarriesADateAndMessageId() QVERIFY(!r.messageId.isEmpty()); } +/// Item 165. A draft keeps ONE identity across its revisions, so an autosave +/// replaces the message it wrote last time rather than adding another. Without +/// this every save minted a new Message-ID, and mbsync uploaded each revision +/// to the drafts folder before the next save removed the local file: measured +/// on real mail as four distinct messages on the server for one reply. +void TestMessageBuilder::aSuppliedMessageIdIsUsedRatherThanAFreshOne() +{ + OutgoingMessage message = baseMessage(); + message.messageId = QStringLiteral("kept-across-revisions@example.org"); + + const MessageBuilder::Result r = MessageBuilder::build(message, m_account); + QVERIFY2(r.ok(), qPrintable(r.error)); + + QCOMPARE(r.messageId, QStringLiteral("kept-across-revisions@example.org")); + QVERIFY2(QString::fromUtf8(r.bytes).contains( + QStringLiteral("")), + "the supplied id did not reach the headers"); + + // Twice, because the point is that a SECOND save keeps it. A test building + // once cannot tell a reused id from a freshly generated one. + const MessageBuilder::Result again = MessageBuilder::build(message, m_account); + QVERIFY2(again.ok(), qPrintable(again.error)); + QCOMPARE(again.messageId, r.messageId); +} + +/// The other half, and the constraint that makes the change safe: the SEND +/// path supplies no id, and two sent messages must never share one. A field +/// that defaulted to some fixed value would pass the test above and break +/// this. +void TestMessageBuilder::twoBuildsWithNoSuppliedIdStillDiffer() +{ + const MessageBuilder::Result first = MessageBuilder::build(baseMessage(), m_account); + const MessageBuilder::Result second = MessageBuilder::build(baseMessage(), m_account); + QVERIFY2(first.ok() && second.ok(), "a build failed"); + + QVERIFY(!first.messageId.isEmpty()); + QVERIFY(!second.messageId.isEmpty()); + QVERIFY2(first.messageId != second.messageId, + "two messages built with no supplied id share a Message-ID"); +} + /// Bcc must be PRESENT in the bytes. The documented send command is `msmtp -t`, /// which reads its recipients FROM the headers and strips Bcc itself before /// transmission. Removing it here would mean blind recipients never receive the -- cgit v1.2.3