diff options
| -rw-r--r-- | CHANGELOG.md | 4 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 65 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 2 | ||||
| -rw-r--r-- | src/config.cpp | 37 | ||||
| -rw-r--r-- | tests/test_config.cpp | 36 |
5 files changed, 124 insertions, 20 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 38d2a58..fee2199 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,7 +31,9 @@ point at which they are stable. - **A Drafts filter** in the query row, beside Sent and Trash. It matches each account's `drafts` folder, so it finds what the composer actually writes rather than trusting a flag. An account that configures no drafts folder - contributes nothing and shows no button. + contributes nothing and shows no button. Like Sent, it lists messages rather + than threads: a draft reply gets a row of its own instead of being folded + into the conversation it answers, where it could not be opened. - `Ctrl+W` closes a composer, the way it closes a window elsewhere. The draft is saved or discarded exactly as it is when the window is closed by any other route. 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 d6adc98..7a52984 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 @@ -7548,3 +7548,68 @@ needed. The drafts view is path-based, so zero tags is exactly enough. **Size: S.** One worker slot, one signal, and their tests. **Closed 2026-08-24** (unreleased). See the status table row for the outcome. + + +## 159. The Drafts view lists threads, so a draft is unreachable by double-click + +**Observed (user, 2026-08-25):** "drafts should be treated like \"Sent\", +listing only actual draft messages and not threads, otherwise I can double +click on a thread message and nothing happens." + +**Cause (verified in code):** `Config::builtinFilter()` in `src/config.cpp` +sets `filter.flat = true` for the `sent` generator only, at line 1011. The +`drafts` branch below it leaves the default `false` with a comment stating the +choice explicitly: "NOT flat, like Trash and unlike Sent: a draft reply +belongs with the conversation it answers." That was item 138's decision and it +is the thing the note contradicts. + +The consequence the user reports follows from it. A thread row stands for +`ThreadSummary::firstMessageId`, which in a Drafts view is the first MATCHED +message of the conversation, and that is not necessarily the draft. Item 153 +gated `edit_draft` on the file living in a configured drafts folder precisely +so that opening ordinary mail this way cannot make the first autosave delete a +received message, so the row is inert rather than harmful. Inert is still +"nothing happens". + +**Built 2026-08-25**, after confirming the reversal with the user. + +**Not one line, and the reason is the part worth keeping.** The obvious fix is +`filter.flat = true` in the `drafts` branch. That ships a defect: the view mode +was decided in THREE places that each hardcoded a comparison against `"sent"`, +and they have to agree. + +- `builtinFilter()` sets it for the button. +- `loadSavedQueries()` reapplies it on read, so a hand-edited or migrated file + cannot produce a threaded Sent view. +- `saveSavedQueries()` SKIPS writing it when the generator already implies it, + because a key carrying no information is one a hand-editor must read past. + +Setting only the first does not break the save/load pair, and it is worth being +exact about why: the writer's skip knew only about `sent`, so it would have +STORED `"flat": true` for drafts, and the reader would have honoured it. That +round trip survives by accident. + +What does NOT survive is a file that carries no `flat` key: one written by an +older build, migrated from elsewhere, or hand-edited, which is the case the +reader's fallback exists for. It comes back THREADED against a flat button, and +the writer then persists that disagreement on the next save. The reader is the +load-bearing site, and it is the one a per-branch fix leaves untouched. + +`generatorIsFlat()` is the fix: one closed set beside `generatorTag()`, called +from all three sites. `builtinFilter()` sets `filter.flat` once from it rather +than inside a branch, so the set cannot drift from the labels below it. + +**Trash deliberately did not follow.** A deleted message still belongs to its +conversation, and nothing in the trash has to be reachable for editing. The +test asserts this, so a future change that flattens every folder filter fails +rather than passing quietly. + +**Testing.** `theDraftsFilterIsThreadedNotFlat` asserted the old behaviour and +is inverted rather than deleted, keeping the history in its comment. The +round-trip is covered by extending `aGeneratedEntryWritesNoRedundantKeys`, +which already asserted exactly that property for `sent`, rather than by a +second test that would have restated it. Mutation-checked: reverting +`generatorIsFlat()` to `sent` alone fails both. + +Suite 37 of 38; the failure is `undoMovesTheMessageBack`, item 136, +pre-existing and on an unrelated path. 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 2d99704..4aeff26 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 @@ -230,6 +230,8 @@ taking that too literally. | 158 | A freshly saved draft is invisible until a sync indexes it | defect | S | **done** 2026-08-24, unreleased. `saveDraftNow()` emits `draftSaved`, which `MainWindow` connects to a new `NotmuchWorker::indexDraftFile()` that indexes the one file (previous revision removed, so a rewrite leaves no ghost), and `draftRemoved` drops the entry when a sent draft is unlinked. Measured: `index_file` assigns NO tags, so no stripping and no tag:inbox leak. See the section | +| 159 | The Drafts view lists threads, so a draft is unreachable by double-click | defect | S | **done** 2026-08-25, unreleased. Reverses item 138's own decision, confirmed with the user. `generatorIsFlat()` in `config.cpp` is now the single closed set of flat generators, replacing three hardcoded comparisons against `"sent"`: the built-in filter, the reader that reapplies the mode, and the writer that skips storing what the generator implies. Those three had to agree and nothing made them; a `drafts` entry saved and reloaded would otherwise have come back THREADED while the button was flat. `builtinFilter()` sets `flat` once from the helper rather than in a branch, so the set cannot drift from the labels | + Sizes are rough: XS under an hour, S a sitting, M a session. --- diff --git a/src/config.cpp b/src/config.cpp index 534ba72..d91259a 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -80,6 +80,18 @@ QString generatorTag(const QString &generator) return QString(); } +/// Whether a generator lists MESSAGES rather than threads. "sent" folds a +/// user's own message back into the conversation it answers, and "drafts" is +/// worse: a thread row stands for its first matched message, which for a draft +/// reply is the message being replied TO, so the draft itself is unreachable. +/// "trash" stays threaded, since a deleted message still belongs to its +/// conversation. Closed set, and the one place the three views are decided. +bool generatorIsFlat(const QString &generator) +{ + return generator == QStringLiteral("sent") + || generator == QStringLiteral("drafts"); +} + } // namespace QString Account::scopedQuery(const QString &query) const @@ -838,14 +850,14 @@ void Config::loadSavedQueries(const QString &configPath, QSettings &settings) query.query = object.value(QStringLiteral("query")).toString(); query.account = object.value(QStringLiteral("account")).toString(); query.generated = object.value(QStringLiteral("generated")).toString(); - // A generator carries its own view mode, so "sent" is flat whether or - // not the file says so. Storing it as a plain field would let a + // A generator carries its own view mode, so a flat one is flat whether + // or not the file says so. Storing it as a plain field would let a // hand-edited or migrated-from-elsewhere row produce a THREADED sent // view, which folds every reply back into the conversation the user // sent one message into. The file may still set it for an ordinary // query. query.flat = object.value(QStringLiteral("flat")).toBool(false) - || query.generated == QStringLiteral("sent"); + || generatorIsFlat(query.generated); if (query.isGenerated() && !kQueryGenerators.contains(query.generated)) { @@ -905,7 +917,7 @@ bool Config::saveSavedQueries() const object.insert(QStringLiteral("account"), query.account); // Skipped when the generator already implies it, which loadSavedQueries // reapplies on the way back in. - if (query.flat && query.generated != QStringLiteral("sent")) + if (query.flat && !generatorIsFlat(query.generated)) object.insert(QStringLiteral("flat"), true); for (auto it = query.unknown.begin(); it != query.unknown.end(); ++it) object.insert(it.key(), it.value()); @@ -987,6 +999,9 @@ SavedQuery Config::builtinFilter(const QString &generator) SavedQuery filter; filter.generated = generator; + // One source for the view mode, shared with the saved-query round trip, so + // a branch below cannot disagree with what loadSavedQueries reapplies. + filter.flat = generatorIsFlat(generator); // Translated, because these are the labels on the buttons. The GENERATOR // name is not: it is stored in queries.json and matched against a closed @@ -1005,16 +1020,18 @@ SavedQuery Config::builtinFilter(const QString &generator) filter.name = tr("Important"); } else if (generator == QStringLiteral("sent")) { filter.name = tr("Sent"); - // Messages rather than threads, and the only filter that sets this. A - // thread would fold the user's sent message back into the conversation - // it belongs to, which is item 63's finding. - filter.flat = true; + // Flat, per generatorIsFlat(): a thread would fold the user's sent + // message back into the conversation it belongs to, item 63's finding. } else if (generator == QStringLiteral("drafts")) { // The LABEL is translated; the generator stays `drafts`, which is what // queries.json stores and what a closed set is matched against. filter.name = tr("Drafts"); - // NOT flat, like Trash and unlike Sent: a draft reply belongs with the - // conversation it answers. + // Flat, per generatorIsFlat(). Item 138 chose threaded, reasoning that + // a draft reply belongs with the conversation it answers; item 159 + // reversed it on what that cost. A thread row stands for its first + // MATCHED message, which for a draft reply is the message being + // replied TO, so the draft itself had no row of its own and + // double-clicking the conversation opened nothing. } else if (generator == QStringLiteral("trash")) { filter.name = tr("Trash"); // NOT flat, unlike Sent. A deleted message still belongs to its diff --git a/tests/test_config.cpp b/tests/test_config.cpp index 17b8e1d..e69a073 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -122,7 +122,7 @@ private slots: void anAccountWithoutATrashFolderWarns(); void theDraftsFilterComposesPerAccount(); void theDraftsFilterMatchesNothingWithoutAFolder(); - void theDraftsFilterIsThreadedNotFlat(); + void theDraftsFilterIsFlatLikeSent(); void theTrashFilterComposesPerAccount(); void theTrashFilterMatchesNothingWithoutAFolder(); void anAccountWithoutASendCommandIsReceiveOnly(); @@ -1076,17 +1076,24 @@ void TestConfig::theDraftsFilterMatchesNothingWithoutAFolder() Config::matchNothingQuery()); } -void TestConfig::theDraftsFilterIsThreadedNotFlat() +void TestConfig::theDraftsFilterIsFlatLikeSent() { - // Unlike Sent, and deliberately. Sent is flat because a thread would fold - // the user's own message back into the conversation it answers, which is - // item 63's finding. A draft reply belongs with its conversation for the - // same reason a trashed message does, so drafts follow trash here. + // Item 138 shipped this THREADED, reasoning that a draft reply belongs + // with the conversation it answers. Item 159 reversed it on what that + // cost: a thread row stands for its first MATCHED message, which for a + // draft reply is the message being replied TO, so the draft had no row of + // its own and double-clicking the conversation opened nothing. const SavedQuery drafts = Config::builtinFilter(QStringLiteral("drafts")); - QVERIFY2(!drafts.flat, "the drafts filter is flat, like Sent"); + QVERIFY2(drafts.flat, "the drafts filter went back to threaded, so a draft " + "reply has no row of its own (item 159)"); const SavedQuery sent = Config::builtinFilter(QStringLiteral("sent")); QVERIFY2(sent.flat, "Sent stopped being flat, which item 63 requires"); + + // Trash deliberately did NOT follow. A deleted message still belongs to + // its conversation, and nothing has to be reachable for editing there. + const SavedQuery trash = Config::builtinFilter(QStringLiteral("trash")); + QVERIFY2(!trash.flat, "trash became flat; only sent and drafts should be"); } void TestConfig::theTrashFilterComposesPerAccount() @@ -2357,6 +2364,7 @@ void TestConfig::aGeneratedEntryWritesNoRedundantKeys() "version": 1, "queries": [ { "name": "Sent", "generated": "sent", "pinned": true }, + { "name": "Drafts", "generated": "drafts", "pinned": true }, { "name": "Inbox", "query": "tag:inbox", "pinned": true } ] })")); @@ -2381,18 +2389,28 @@ void TestConfig::aGeneratedEntryWritesNoRedundantKeys() QVERIFY2(!sent.contains(QStringLiteral("flat")), "the sent generator implies flat; storing it says nothing"); + // Drafts is the second flat generator (item 159) and must be skipped by + // the same rule, not by a second one that could disagree with it. + const QJsonObject drafts = array.at(1).toObject(); + QCOMPARE(drafts.value(QStringLiteral("generated")).toString(), + QStringLiteral("drafts")); + QVERIFY2(!drafts.contains(QStringLiteral("flat")), + "the drafts generator implies flat; storing it says nothing"); + // The ordinary entry is untouched by any of that. - const QJsonObject inbox = array.at(1).toObject(); + const QJsonObject inbox = array.at(2).toObject(); QCOMPARE(inbox.value(QStringLiteral("query")).toString(), QStringLiteral("tag:inbox")); // And it all still reads back the same. Config reloaded; reloaded.load(path); - QCOMPARE(reloaded.savedQueries().size(), 2); + QCOMPARE(reloaded.savedQueries().size(), 3); QVERIFY(reloaded.savedQueries().at(0).isGenerated()); QVERIFY2(reloaded.savedQueries().at(0).flat, "flat must come back from the generator, not from the file"); + QVERIFY2(reloaded.savedQueries().at(1).flat, + "drafts must come back flat too, from the same rule"); } void TestConfig::anAccountWithoutASendCommandIsReceiveOnly() |