diff options
| -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. |