diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-04 20:05:01 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-04 20:05:01 +0200 |
| commit | 8485283bbc30c886e28722516dc6f0b11c3bf82d (patch) | |
| tree | 1caf649b37b8ac693991f4bdf6d12247f9a0eefa | |
| parent | bd1b907ba22464bb869bcc10cc9327ed74c44791 (diff) | |
| download | qtmaildir-8485283bbc30c886e28722516dc6f0b11c3bf82d.tar.gz qtmaildir-8485283bbc30c886e28722516dc6f0b11c3bf82d.zip | |
fix(sync): count unsynced edits as net state, not as writes
Item 28, reported by the user: open a thread, let the automatic
mark-read remove `unread`, then press Ctrl+U to put it back. The
indicator read "2 unsynced change(s)" with the mail store exactly where
it started.
The counter incremented per confirmed write and never decremented, so
any add-then-remove of the same tag inflated it. Mark-read is simply the
path that fires without being asked, which is why it surfaced there.
The user's call was net state: an edit and its inverse are zero
outstanding changes, because what the indicator answers is whether
quitting now would strand work. A QHash keyed "<messageId>\n<tag>"
replaces the int, and a pair that reverts is erased rather than stored
with the new direction, so the map cannot grow without bound across a
long session of tagging and untagging.
Keyed per (message, tag) rather than per message: removing `unread` and
adding `flagged` on one message are independent changes and must not
cancel each other. A change carrying no message ids cannot be netted
against anything and is counted separately, since dropping it would
understate the indicator, which is the direction that costs work.
Both properties item 18 established still hold: a successful sync clears
everything, a failed one clears nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 24 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 64 | ||||
| -rw-r--r-- | src/mainwindow.h | 22 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 93 |
4 files changed, 192 insertions, 11 deletions
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 67668d4..c3a1f4e 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 @@ -72,7 +72,7 @@ taking that too literally. | 25 | No select-all, and bulk actions are undiscoverable | workflow | S | **done** | | 26 | No way to add or remove an arbitrary tag from the UI | workflow | S | **done** | | 27 | The UI cannot see a sync it did not start | feedback | S | **done** | -| 28 | Re-adding `unread` counts 2 unsynced changes, not 0 | correctness | S | open | +| 28 | Re-adding `unread` counts 2 unsynced changes, not 0 | correctness | S | **done** | | 29 | Sync button stays enabled during a background sync | feedback | XS | **done** | | 30 | The blank right pane is wasted space | presentation | M | open | | 31 | The quit prompt has no highlighted default button | discoverability | XS | **done** | @@ -1386,6 +1386,28 @@ recognising an inverse exists. - Do not fix this by not counting the automatic mark-read. It is a real write to the index, and hiding it would make the count wrong in the other direction. +### Outcome (done) + +**Decided by the user, 2026-08-04: net state.** "If I undo delete it's 0 edits, +not 2." The counter is replaced by a `QHash<QString, bool>` keyed +`"<messageId>\n<tag>"`, and a pair that reverts is **erased** rather than stored +with the new direction, so an edit and its inverse leave nothing behind and the +map cannot grow without bound over a long session of tagging and untagging. + +**Keyed per (message, tag), not per message.** Removing `unread` and adding +`flagged` on one message are two independent changes; a per-message key would +have cancelled them against each other. A test pins this, and it passed before +the change, so it exists to stop a later simplification from over-netting. + +**A change carrying no message ids still counts**, tracked in a separate +`m_unnettablePendingEdits`. It cannot be netted against anything, and dropping +it would understate the indicator, which is the direction that costs the user +work. This also keeps the older tests honest: they emit a `TagChange` with no +ids, and would otherwise have started reporting zero. + +Both properties item 18 established survive: a successful sync clears everything, +a failed one clears nothing. + ## 29. Sync button stays enabled during a background sync **Observed (user, 2026-08-04):** while a cron sync runs, the Sync button is diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index a7f9b40..7cd2769 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -154,7 +154,7 @@ void MainWindow::closeEvent(QCloseEvent *event) return; } - if (!m_closeApproved && m_pendingEdits > 0 + if (!m_closeApproved && pendingEditCount() > 0 && m_config.syncOnExit() != Config::SyncOnExit::Never) { // Not a destructive-action confirmation, which CLAUDE.md forbids for @@ -167,7 +167,7 @@ void MainWindow::closeEvent(QCloseEvent *event) const auto answer = QMessageBox::warning( this, tr("Unsynced changes"), tr("%n tag change(s) have not been synced, and no sync command " - "is configured. Quit anyway?", "", m_pendingEdits), + "is configured. Quit anyway?", "", pendingEditCount()), QMessageBox::Discard | QMessageBox::Cancel, QMessageBox::Cancel); if (answer == QMessageBox::Cancel) { @@ -181,7 +181,7 @@ void MainWindow::closeEvent(QCloseEvent *event) box.setIcon(QMessageBox::Question); box.setWindowTitle(tr("Unsynced changes")); box.setText(tr("%n tag change(s) have not been synced.", "", - m_pendingEdits)); + pendingEditCount())); box.setInformativeText(tr("Sync before quitting?")); QPushButton *sync = box.addButton(tr("Sync and quit"), QMessageBox::AcceptRole); @@ -1299,7 +1299,8 @@ void MainWindow::onSyncFinished(bool success, int exitCode) // Only a SUCCESSFUL sync clears the count. Clearing on failure would // assert the edits had reached the mail store when the sync is exactly // what failed to put them there. - m_pendingEdits = 0; + m_pendingTagEdits.clear(); + m_unnettablePendingEdits = 0; updatePendingIndicator(); showTransientStatus(tr("Sync complete")); @@ -1363,10 +1364,33 @@ void MainWindow::onTagsApplied(const TagChange &change) m_pendingChange = {}; m_pendingThreadIds.clear(); - // Counted here, where a write is CONFIRMED, rather than where one is sent: + // Recorded here, where a write is CONFIRMED, rather than where one is sent: // an optimistic update the worker later rejects must not leave the // indicator claiming an edit that never landed. - ++m_pendingEdits; + // + // NET state, not a count of writes. An edit and its inverse leave the mail + // store where it started, so they must leave the indicator at zero: the + // automatic mark-read followed by Ctrl+U used to read as 2 unsynced + // changes when nothing was outstanding. What the user needs to know is + // whether quitting now would strand work. + // + // Keyed per (message, tag): removing `unread` and adding `flagged` on one + // message are two independent changes and must not cancel each other. + for (const QString &messageId : change.messageIds) { + for (const QString &tag : change.added) + recordPendingEdit(messageId, tag, true); + for (const QString &tag : change.removed) + recordPendingEdit(messageId, tag, false); + } + + // A change carrying no message ids cannot be netted against anything, and + // must still register: losing an edit understates the indicator, which is + // the direction that costs the user work. + if (change.messageIds.isEmpty() + && !(change.added.isEmpty() && change.removed.isEmpty())) { + ++m_unnettablePendingEdits; + } + updatePendingIndicator(); // A tag the user has just created is the one they are most likely to type @@ -1469,16 +1493,40 @@ void MainWindow::updateSyncControls() m_syncButton->setEnabled(!busy && m_sync && m_sync->isAvailable()); } +void MainWindow::recordPendingEdit(const QString &messageId, const QString &tag, + bool added) +{ + const QString key = messageId + QLatin1Char('\n') + tag; + + // A tag put back the way it was is not an outstanding change. Erase rather + // than store the new direction, or the ledger grows without bound over a + // long session of tagging and untagging. + const auto existing = m_pendingTagEdits.constFind(key); + if (existing != m_pendingTagEdits.constEnd()) { + if (*existing != added) + m_pendingTagEdits.erase(m_pendingTagEdits.find(key)); + return; + } + + m_pendingTagEdits.insert(key, added); +} + +int MainWindow::pendingEditCount() const +{ + return m_pendingTagEdits.size() + m_unnettablePendingEdits; +} + void MainWindow::updatePendingIndicator() { - if (m_pendingEdits <= 0) { + const int pending = pendingEditCount(); + if (pending <= 0) { m_pendingLabel->hide(); return; } // "Changes" and not "mutations": the unit the user thinks in is the tagging // they did, not the writes it became. - m_pendingLabel->setText(tr("%n unsynced change(s)", "", m_pendingEdits)); + m_pendingLabel->setText(tr("%n unsynced change(s)", "", pending)); m_pendingLabel->setToolTip( tr("Tag changes made here that a sync has not yet carried to the mail " "store. An external notmuch run can clear them without this count " diff --git a/src/mainwindow.h b/src/mainwindow.h index 2edeb79..cf9aca6 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -176,9 +176,17 @@ private: /// the one on screen. void markCurrentThreadRead(); - /// Redraws the unsynced-edits indicator from m_pendingEdits. + /// Redraws the unsynced-edits indicator from pendingEditCount(). void updatePendingIndicator(); + /// Records one confirmed (message, tag) change, cancelling it against an + /// opposite change already outstanding for the same pair. + void recordPendingEdit(const QString &messageId, const QString &tag, + bool added); + + /// Net changes the index holds that a sync has not carried over. + int pendingEditCount() const; + /// Shows or hides the "syncing" state: the progress bar and a disabled /// Sync button. /// @@ -318,7 +326,17 @@ private: /// /// A lower bound on what is outstanding, never a guarantee: the user's cron /// can run notmuch new without the application noticing. - int m_pendingEdits = 0; + /// + /// NET state rather than a tally of writes. Keyed "<messageId>\n<tag>", + /// value true for added and false for removed; a pair that reverts is + /// erased rather than stored, so an edit and its inverse leave nothing + /// behind and the map cannot grow without bound. + QHash<QString, bool> m_pendingTagEdits; + + /// Confirmed changes carrying no message ids, which cannot be netted + /// against anything. Counted separately rather than dropped: understating + /// the indicator is the direction that costs the user work. + int m_unnettablePendingEdits = 0; /// Marks the open thread read once it has been on screen long enough. /// diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index f791d8f..d133cfb 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -86,6 +86,9 @@ private slots: void deleteOnAMixedSelectionDeletesRatherThanSplittingIt(); void aTransientStatusMessageExpires(); void theSelectionCountIsStateAndDoesNotExpire(); + void anEditUndoneNettsBackToZero(); + void aDifferentTagOnTheSameMessageStillCounts(); + void anEditWithNoMessageIdsStillCounts(); }; void TestMainWindow::everyKnownActionIsRegistered() @@ -1180,6 +1183,96 @@ void TestMainWindow::theSelectionCountIsStateAndDoesNotExpire() "not an event"); } +void TestMainWindow::anEditUndoneNettsBackToZero() +{ + // Reported by the user: open a thread, let the 2 s auto-mark-read remove + // `unread`, then press Ctrl+U to put it back. The indicator read 2 unsynced + // changes when the mail store was exactly where it started. + // + // The count tracks NET state, not writes. Two writes did happen, but their + // effect cancels, and what the user needs to know is whether quitting now + // would strand work. + const Config config; + MainWindow window(config); + + auto *label = window.findChild<QLabel *>(QStringLiteral("pendingEdits")); + QVERIFY(label); + QVERIFY(label->isHidden()); + + // The automatic mark-read: remove `unread` from one message. + TagChange off; + off.messageIds = { QStringLiteral("m1") }; + off.removed = { QStringLiteral("unread") }; + off.description = QStringLiteral("Mark read"); + QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", + Q_ARG(TagChange, off))); + QVERIFY2(!label->isHidden(), "one edit must show the indicator"); + + // Ctrl+U puts it back on the same message. + TagChange on; + on.messageIds = { QStringLiteral("m1") }; + on.added = { QStringLiteral("unread") }; + on.description = QStringLiteral("Mark unread"); + QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", + Q_ARG(TagChange, on))); + + QVERIFY2(label->isHidden(), + qPrintable(QStringLiteral("an edit and its inverse left the " + "indicator showing '%1'") + .arg(label->text()))); +} + +void TestMainWindow::aDifferentTagOnTheSameMessageStillCounts() +{ + // Netting must be per (message, tag), not per message. Removing `unread` + // and adding `flagged` on one message are two independent changes, and + // neither cancels the other. + const Config config; + MainWindow window(config); + + auto *label = window.findChild<QLabel *>(QStringLiteral("pendingEdits")); + QVERIFY(label); + + TagChange a; + a.messageIds = { QStringLiteral("m1") }; + a.removed = { QStringLiteral("unread") }; + a.description = QStringLiteral("Mark read"); + QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", + Q_ARG(TagChange, a))); + + TagChange b; + b.messageIds = { QStringLiteral("m1") }; + b.added = { QStringLiteral("flagged") }; + b.description = QStringLiteral("Flag"); + QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", + Q_ARG(TagChange, b))); + + QVERIFY2(!label->isHidden(), + "two different tags on one message cancelled each other"); +} + +void TestMainWindow::anEditWithNoMessageIdsStillCounts() +{ + // A TagChange carrying no message ids cannot be netted against anything, + // and must still register rather than silently counting as zero. Losing an + // edit understates the indicator, which is the direction that costs the + // user work. + const Config config; + MainWindow window(config); + + auto *label = window.findChild<QLabel *>(QStringLiteral("pendingEdits")); + QVERIFY(label); + + TagChange change; + change.added = { QStringLiteral("deleted") }; + change.description = QStringLiteral("Delete"); + QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", + Q_ARG(TagChange, change))); + + QVERIFY2(!label->isHidden(), + "an edit with no message ids was not counted at all"); +} + // Constructing a MainWindow needs a QApplication and a platform plugin. The // test has no display under ctest, so it runs offscreen unless the caller // asked for something else. |
