summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-08-29 10:11:59 +0200
committerDanilo M. <danix@danix.xyz>2026-08-29 10:11:59 +0200
commite2bd68a863513209a5881696164fd463ddc83503 (patch)
treefe9fc90462030f9f99d5298eb09785018cc60f40
parentfeb15bcec261501eb49ad0de1fdc05a9992227c2 (diff)
downloadqtmaildir-e2bd68a863513209a5881696164fd463ddc83503.tar.gz
qtmaildir-e2bd68a863513209a5881696164fd463ddc83503.zip
fix: say an edit is waiting for the sync instead of claiming it landed
Item 182, found by hand: a thread of 9 messages with 5 unread, marked read while a sync was running, reported "<subject>: mark as read" and then reported the same work again when the sync finished. The user read it as double reporting. Not a double write, and the mail was correct. It is one action reported twice because the FIRST report was the wrong one. A sync holds notmuch's exclusive write lock and the worker's read-write open blocks on it rather than failing, so an edit made during a sync is held and sent when the lock frees. All three hold branches say exactly that, in a label chosen deliberately: NOT transient, because it describes state lasting until the sync ends, and a message that expired would leave rows showing a tag the database has not got and no explanation of why. That label never survived. Every caller announced the action itself a line later through showTransientStatus(), which overwrote it, so the user was told the write had happened and the hold was never mentioned. The flush at the end of the sync then reported the same work again and read as a duplicate rather than as its completion. announceAction() asks whether a sync holds the lock and, when one does, sets a non-transient label naming the action AND the wait. The action is still named because that announcement is what stands in for the confirmation dialog this project rules out: it is how a user tells that something larger than they meant has just happened, so the hold is added to it rather than replacing it. The flush message is untouched and is the only signal that held work actually landed, whose absence was item 106. The test drives toggle_unread, the route the user took, and asserts both halves: the text mentions the sync, and it still says what is waiting. Asserting only the first would pass against an announcement that dropped the action entirely. Mutation-checked by forcing the non-held branch, which fails with the exact text the user reported. The new string is translated, since one that misses the Italian ships as English inside an otherwise Italian UI: lupdate found it with no context warnings, lrelease reports 552 finished and 0 unfinished. Suite: 42 of 43, with undoMovesTheMessageBack failing as it does on master (item 136).
-rw-r--r--CHANGELOG.md7
-rw-r--r--docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md85
-rw-r--r--docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md1
-rw-r--r--src/mainwindow.cpp21
-rw-r--r--src/mainwindow.h20
-rw-r--r--tests/test_mainwindow.cpp65
-rw-r--r--translations/qtmaildir_it_IT.ts4
7 files changed, 200 insertions, 3 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md
index cf7082d..9920c1a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -143,6 +143,13 @@ point at which they are stable.
### Fixed
+- **An edit made during a sync now says it is waiting.** A sync holds
+ notmuch's write lock, so an edit made while one runs is queued and applied
+ when it finishes. The status bar said so and was then immediately
+ overwritten with the bare action, which claimed the change had been applied;
+ when the queued work was really sent at the end of the sync it was reported
+ a second time, reading as a duplicate. The announcement now names the action
+ AND the wait, and stays up until the sync ends.
- **The conversation dashboard now follows what you do to the conversation.**
Marking a thread read left the pane reporting the unread count, the progress
bar and the "Waiting for you" list it had when you opened it, including when
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 27b4e28..b3cda60 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
@@ -9374,3 +9374,88 @@ assertion after it means something, and mutation-checked by removing the call.
Note the failing runs take 45s and the passing one 295ms: the `QTRY` timeouts
expire when the refresh never comes, which reads as a slow test rather than a
missing feature.
+
+## 182. An edit made during a sync is announced twice and never says it is waiting
+
+**Done 2026-08-29**, unreleased, on `thread-row-identity`.
+
+**Observed (user, by hand, 2026-08-29):** a thread of 9 messages with 5 unread,
+marked read while a sync was running. The status bar showed "<subject>: mark as
+read"; at the end of the sync it showed the 5 messages reported again. "Double
+reporting before/after the sync."
+
+**Cause, verified in the code.** Not a double write, and the mail is correct.
+It is one action reported twice because the FIRST report is the wrong one.
+
+A sync holds notmuch's exclusive write lock, and the worker's read-write open
+blocks on it rather than failing, so an edit made during a sync is HELD and
+sent when the lock frees. All three hold branches say so, in a label chosen
+deliberately:
+
+- `sendThreadTagChange()` (`mainwindow.cpp:7122`)
+- `sendMessageTagChange()` (`:5937`)
+- `sendMove()` (`:6722`)
+
+Each sets `m_statusLabel` directly to "A sync is running; your change will be
+applied when it finishes.", with a comment stating that this is NOT transient
+because it describes state lasting until the sync ends, and that a message
+which expired would leave the user with rows showing a tag the database has not
+got and no explanation of why.
+
+That label never survives. Every caller announces the action itself immediately
+afterwards, through `showTransientStatus()`, which overwrites it:
+
+- `markThreadsRead()` (`:3641`), the path in the report
+- the tag-action funnel (`:5846`)
+- `deleteSelection()` (`:6104`)
+
+So the user is told "mark as read", which claims the write happened, and the
+hold is never mentioned. When `flushHeldEdits()` later reports "N held changes
+sent now that the sync has finished" (`:4580`), that reads as a second report
+of the same action rather than as its completion.
+
+The two counts differ (threads against messages) because the two messages count
+different things, which is why the report describes 5 messages where the first
+message named the thread.
+
+**Approach.** The caller must not overwrite a hold that the funnel has just
+explained. `aSyncHoldsTheWriteLock()` is already a const predicate, so a caller
+can ask it directly and skip its own announcement; no funnel signature changes.
+
+The flush message stays. It is the only signal that held work actually landed,
+and its absence was item 106, silent data loss on this exact path.
+
+**Constraints.**
+
+- The hold label must stay NON-transient, for the reason its own comment gives.
+ A fix that makes it transient reintroduces the unexplained state it exists to
+ prevent.
+- Three call sites, and a fourth path (`sendMove`) whose callers must be checked
+ rather than assumed: the same defect is one caller away in each.
+- The announcement is what stands in for the confirmation dialog this project
+ rules out, per the comment at `:5842`: it tells the user when something larger
+ than they meant has just happened. Suppressing it entirely during a sync would
+ cost that, so what replaces it has to name the action AND the hold, not drop
+ the action.
+- The two messages counting different things (threads against messages) is a
+ separate readability question and is NOT part of this item.
+
+**Built as `MainWindow::announceAction()`**, which the three sites call in
+place of `showTransientStatus()`. It asks `aSyncHoldsTheWriteLock()` and, when
+one does, sets a non-transient label reading "<action>, waiting for the running
+sync to finish". The action is still named for the reason the constraint above
+gives: that announcement is what stands in for the confirmation dialog this
+project rules out, so the hold is ADDED to it rather than replacing it. The
+flush message at the end of the sync is untouched, and now reads as the
+completion of the first message rather than a repeat of it.
+
+The test drives `toggle_unread`, the route the user took, after putting the
+window into the state the sync monitor puts it in. It asserts BOTH halves: the
+text mentions the sync, and it still says what is waiting. Asserting only the
+first would pass against an announcement that dropped the action entirely,
+which is the thing the constraint forbids. Mutation-checked by forcing the
+non-held branch, which fails with the exact text the user reported.
+
+The string is translated, since a user-facing string that misses the Italian
+translation ships as English inside an otherwise Italian UI: `lupdate` found it
+with no context warnings and `lrelease` reports 552 finished, 0 unfinished.
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 68c080c..54e4b43 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
@@ -255,6 +255,7 @@ taking that too literally.
| 179 | Undo is one level deep in practice, and there is no Redo | workflow | ? | open, 2026-08-29, from the notes. The `QUndoStack` is real and multi-level; what is missing is a `redo` action (absent from `knownActions()`, never called) and an answer to the stack being CLEARED on every new query (`mainwindow.cpp:3458`), which is what makes a deep stack behave like a shallow one. The clear has a correct reason and cannot simply be removed. Redo re-applies a write to real mail, so item 176's rule binds it too |
| 180 | The repaint rules are discovered one hole at a time | maintenance | S-L | open, 2026-08-29, from the notes, and a QUESTION rather than a defect. Items 105, 107, 109, 110 and 170 are each one hole in the same surface, all found by hand. Three mechanisms (optimistic repaint, `syncViewMembership()`, revert) agree by documentation rather than by code. Cheapest answer is one invariant test, not a rewrite; the user decides which, and that decides the size |
| 181 | The thread dashboard does not follow a write to the conversation it shows | defect | XS | **done 2026-08-29**, unreleased, on `thread-row-identity`, from the notes. The dashboard draws a `ThreadDigest` built by the worker from the INDEX, which arrived only on selection, so a tag write moved the model and the card and left the pane reporting the count the conversation had when it was opened. Reachable from the dashboard's OWN Mark all read button. Re-requested from `onTagsApplied()`, where the write is confirmed: queued beside the write it races it and answers from the state before it, which is how the first fix passed review and failed the test. Section in the closed file |
+| 182 | An edit made during a sync is announced twice and never says it is waiting | defect | XS | **done 2026-08-29**, unreleased, on `thread-row-identity`, found by hand. The hold branches set a deliberately NON-transient label; all three callers overwrote it a line later with the bare action, so the user was told the write had landed and then told again when it really did. `announceAction()` adds the wait to the action rather than replacing it, since that announcement is what stands in for the confirmation dialog this project rules out. Section in the closed file |
Sizes are rough: XS under an hour, S a sitting, M a session.
diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
index df7062d..1da9ac7 100644
--- a/src/mainwindow.cpp
+++ b/src/mainwindow.cpp
@@ -3638,7 +3638,7 @@ void MainWindow::markAllRead()
description);
m_undoStack.push(command);
- showTransientStatus(
+ announceAction(
tr("%1: %n thread(s)", "", threadIds.size()).arg(description));
}
@@ -5198,6 +5198,21 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state)
}
+void MainWindow::announceAction(const QString &text)
+{
+ if (!aSyncHoldsTheWriteLock()) {
+ showTransientStatus(text);
+ return;
+ }
+
+ // Not showTransientStatus(): this describes state that lasts until the
+ // sync ends. The hold branch that queued the edit set the same kind of
+ // label and this replaces it, naming the action it was silent about.
+ m_statusLabel->setText(
+ tr("%1, waiting for the running sync to finish").arg(text));
+ m_statusTimer->stop();
+}
+
void MainWindow::showTransientStatus(const QString &text)
{
m_transientMessage = text;
@@ -5843,7 +5858,7 @@ void MainWindow::tagSelected(const QStringList &add, const QStringList &remove,
// dialog CLAUDE.md rules out: undo is the safety net, and undo is only
// usable if the user can tell that something larger than they meant has
// just happened.
- showTransientStatus(
+ announceAction(
scope.wholeThread
? tr("%1: %n message(s) (whole thread)", "", scope.messageCount)
.arg(description)
@@ -6101,7 +6116,7 @@ void MainWindow::trashMessages(const QStringList &messageIds,
tr("Delete"), false, wholeThreadIds);
}
- showTransientStatus(
+ announceAction(
tr("%1: %n message(s)", "", messageCount).arg(tr("Delete")));
}
diff --git a/src/mainwindow.h b/src/mainwindow.h
index 1f68733..2b217d2 100644
--- a/src/mainwindow.h
+++ b/src/mainwindow.h
@@ -680,6 +680,26 @@ private slots:
/// drive it through the meta-object.
void showTransientStatus(const QString &text);
+ /// Announces an action that has just been sent, saying so when a sync is
+ /// holding it rather than claiming it landed.
+ ///
+ /// Item 182. The three hold branches set a deliberately NON-transient label
+ /// explaining that the change will be applied when the sync finishes, and
+ /// every caller overwrote it a line later with the bare action, which
+ /// claims a write that has not happened. The user then saw the same work
+ /// reported again by flushHeldEdits() and read it as double reporting.
+ ///
+ /// The action is still named, because that announcement is what stands in
+ /// for the confirmation dialog this project rules out: it is how a user
+ /// tells that something larger than they meant has just happened. So the
+ /// hold is ADDED to it rather than replacing it.
+ ///
+ /// Held text is not transient, for the reason the hold branches give: it
+ /// describes state lasting until the sync ends, and a message that expired
+ /// would leave rows showing a tag the database has not got and no
+ /// explanation of why.
+ void announceAction(const QString &text);
+
/// Reacts to a sync started outside this window, by cron or by hand.
///
/// A private slot rather than a plain method so tests can drive it through
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index a8e6284..bbdad19 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -270,6 +270,7 @@ private slots:
void aWorkerBackedWindowReturnsRealThreads();
void aPurgeTakesTheRowsOutOfTheViewWithoutARefresh();
void theDashboardFollowsAWriteToTheConversationItShows();
+ void anEditHeldByASyncSaysSoInsteadOfClaimingItLanded();
// Compose and send, item 123 task 12.
void theMailRootComesFromTheConfigNotTheIndex();
@@ -9226,6 +9227,70 @@ void TestMainWindow::aWorkerBackedWindowReturnsRealThreads()
/// Reachable from the dashboard's own Mark all read button, which is the worst
/// version of it: the user presses a button and the number above it does not
/// move.
+/// Item 182, found by hand: a thread marked read DURING a sync reported
+/// "<subject>: mark as read", and then reported the same work again when the
+/// sync finished, never once saying it was waiting.
+///
+/// The write is held, correctly: a sync holds notmuch's exclusive lock and the
+/// worker's read-write open BLOCKS on it, so sending would freeze the worker
+/// for the rest of the run. Every hold branch sets a deliberately NON-transient
+/// label explaining that, and every caller then overwrote it a line later with
+/// the generic announcement, which claims the write happened.
+///
+/// Asserted on the LABEL rather than on the write: what the mail does was
+/// already right, and what the user was told was not.
+void TestMainWindow::anEditHeldByASyncSaysSoInsteadOfClaimingItLanded()
+{
+ QTemporaryDir dir;
+ QVERIFY(dir.isValid());
+ const Config config = configWithTrash(dir);
+ MainWindow window(config);
+
+ auto *model = window.findChild<ThreadListModel *>();
+ QVERIFY(model);
+ auto *view = window.findChild<QTreeView *>();
+ QVERIFY(view);
+ auto *status = window.findChild<QLabel *>(QStringLiteral("statusMessage"));
+ QVERIFY(status);
+
+ ThreadSummary thread = threadAtPath(QStringLiteral("t1"),
+ QStringLiteral("acct/inbox/cur/1:2,S"),
+ { QStringLiteral("unread") });
+ thread.totalCount = 9;
+ model->appendBatch({ thread });
+ view->setCurrentIndex(model->index(0, 0, {}));
+
+ // The sync starts. This is the same slot the sync monitor calls, so the
+ // window reaches the state a real background sync puts it in.
+ QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged",
+ Q_ARG(SyncMonitor::State,
+ SyncMonitor::State::Running));
+
+ // The toggle, which is the route the user took: the thread has unread
+ // messages, so this reads "Mark thread as read" and sends the write.
+ auto *toggle = window.findChild<QAction *>(QStringLiteral("toggle_unread"));
+ QVERIFY(toggle);
+ toggle->trigger();
+
+ // The hold has to be what the user is told about. Without the fix the
+ // label reads "Mark as read: 1 thread(s)", which claims a write that has
+ // not happened and cannot happen until the sync ends.
+ QVERIFY2(status->text().contains(QStringLiteral("sync")),
+ qPrintable(QStringLiteral("the status bar never mentions the "
+ "sync that is holding the edit: %1")
+ .arg(status->text())));
+
+ // And it still names the action, which is what stands in for the
+ // confirmation dialog this project rules out: the user has to be able to
+ // tell that something larger than they meant has just happened.
+ QVERIFY2(status->text().contains(QStringLiteral("read"), Qt::CaseInsensitive),
+ qPrintable(QStringLiteral("the status bar no longer says WHAT is "
+ "waiting: %1").arg(status->text())));
+
+ // No restore to "/proc/locks": init() points every test at its own
+ // table, and handing the real one back would re-expose the next test.
+}
+
void TestMainWindow::theDashboardFollowsAWriteToTheConversationItShows()
{
WorkerBackedWindow backed;
diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts
index 5249149..5d59b44 100644
--- a/translations/qtmaildir_it_IT.ts
+++ b/translations/qtmaildir_it_IT.ts
@@ -1387,6 +1387,10 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation>
<translation>Sincronizzazione in corso in background...</translation>
</message>
<message>
+ <source>%1, waiting for the running sync to finish</source>
+ <translation>%1, in attesa che termini la sincronizzazione in corso</translation>
+ </message>
+ <message>
<source>Syncing...</source>
<translation>Sincronizzazione in corso...</translation>
</message>