From 06b0435830daaed49a2d5231dcb6f02ff0124d5d Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Wed, 26 Aug 2026 18:53:54 +0200 Subject: refactor: drop the unnettable pending-edit counter Item 119's stated blocker, removed by finding out what it held: nothing. pendingEditCount() summed four sources, three of which can name the messages they hold and one of which was a bare int. That int counted confirmed changes carrying no message ids, on the reasoning that an edit which cannot be netted must still register rather than be lost. It was what made the count impossible to open and list, since a dialog would have shown three groups and then owed the user a remainder it could not describe. The remainder is empty. NotmuchWorker::applyTags() is the only emitter of tagsApplied(), and its first statement returns on an empty id list, which is the exact condition the counter required. applyTagsToThreads() resolves threads to message ids through a query and errors out when that comes back empty, so it can only ever hand applyTags() a non-empty list. Measured rather than read. A qFatal in the branch fired in 4 of 70 test_mainwindow cases, all four building a TagChange by hand and invoking the slot directly with no worker involved; an assertion before the worker's own emit never fired across the whole suite, worker-backed tests included. The worker's guard stays and is pinned where it lives, by applyTagsWithNoIdsDoesNothing() in test_notmuchworker. The MainWindow test that asserted the deleted branch is replaced by one for the consequence: a change reaching the indicator names its messages, and an edit with its inverse nets back to nothing, which is the property a growing-only counter could never have. Three tests that leaned on the counter to show the indicator now carry message ids, as a real edit always does. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01P88Q3MCSCSQxKDy7pmXh9F --- src/mainwindow.cpp | 21 +++++++++----------- src/mainwindow.h | 5 ----- tests/test_mainwindow.cpp | 50 +++++++++++++++++++++++++++++++++++++---------- 3 files changed, 49 insertions(+), 27 deletions(-) diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index c62013d..59edf53 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -4312,7 +4312,6 @@ void MainWindow::onSyncFinished(bool success, int exitCode) // assert the edits had reached the mail store when the sync is exactly // what failed to put them there. m_pendingTagEdits.clear(); - m_unnettablePendingEdits = 0; // Only what this run actually carried, per the snapshot above. An // account added by flushHeldEdits() stays, because its edit reaches the @@ -4446,14 +4445,6 @@ void MainWindow::onTagsApplied(const TagChange &change) 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(); // Item 71. Armed here, where a write is CONFIRMED and the pending count is @@ -4808,7 +4799,6 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) // is the absence of evidence rather than evidence of success. if (MailSync::lastRunOutcome(m_config.syncLog()) == SyncOutcome::Ok) { m_pendingTagEdits.clear(); - m_unnettablePendingEdits = 0; // Cleared HERE, before flushHeldEdits() below, and the ordering is // load-bearing for the reason spelled out on the local path at @@ -5051,6 +5041,14 @@ int MainWindow::pendingEditCount() const // an edit waiting on a lock is precisely the work quitting would lose. // Each held edit counts as one whatever its size, since it carries thread // ids rather than message ids and cannot be netted against the map. + // + // There is no fourth term. A counter for confirmed changes carrying no + // message ids stood here until item 119 looked for what it held and found + // nothing: NotmuchWorker::applyTags() is the only emitter of tagsApplied() + // and returns early on an empty id list, so the change that counter + // existed for cannot reach this window. Every pending change can name the + // messages it touches, which is what lets the indicator be opened and + // listed in full. const int held = int(m_heldEdits.size()); // Held MOVES count for exactly the same reason, and were missed. With no // tag edit queued the count was 0, so the indicator stayed hidden and @@ -5059,8 +5057,7 @@ int MainWindow::pendingEditCount() const // is item 106's data loss, and worse here, because a dropped move leaves // the file in the folder the user asked it out of. const int heldMoves = int(m_heldMoves.size()); - return m_pendingTagEdits.size() + m_unnettablePendingEdits + held - + heldMoves; + return m_pendingTagEdits.size() + held + heldMoves; } void MainWindow::updatePendingIndicator() diff --git a/src/mainwindow.h b/src/mainwindow.h index e65c07d..25a9a7f 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -1541,11 +1541,6 @@ private: /// behind and the map cannot grow without bound. QHash 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. /// /// Single-shot and RESTARTED on every selection change, never stacked: diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 5d2316f..a120ce6 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -415,7 +415,7 @@ private slots: void theSelectionCountIsStateAndDoesNotExpire(); void anEditUndoneNettsBackToZero(); void aDifferentTagOnTheSameMessageStillCounts(); - void anEditWithNoMessageIdsStillCounts(); + void everyPendingChangeCanNameItsMessages(); void anEditDuringABackgroundSyncIsNotSentYet(); void aHeldEditIsSentWhenTheBackgroundSyncEnds(); void aHeldEditCountsAsUnsynced(); @@ -2239,6 +2239,9 @@ void TestMainWindow::pendingEditCountSurvivesAQuery() // Confirm a write the way the worker really does, by emitting the signal // the window listens to. No test-only entry point on MainWindow. TagChange change; + // A real edit always names the messages it touched: the worker's only + // emitter of tagsApplied() returns early without them (item 119). + change.messageIds = { QStringLiteral("pq1@example.org") }; change.added = { QStringLiteral("deleted") }; change.description = QStringLiteral("Delete"); QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", @@ -2271,6 +2274,8 @@ void TestMainWindow::aFailedSyncDoesNotClearThePendingCount() QVERIFY(label); TagChange change; + // See item 119: a change with no message ids never reaches this slot. + change.messageIds = { QStringLiteral("fs1@example.org") }; change.added = { QStringLiteral("flagged") }; QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", Q_ARG(TagChange, change))); @@ -2362,6 +2367,8 @@ void TestMainWindow::syncOnExitNeverClosesSilently() // Give it something to lose, so this is not passing for the same reason // the previous test does. TagChange change; + // See item 119: a change with no message ids never reaches this slot. + change.messageIds = { QStringLiteral("se1@example.org") }; change.added = { QStringLiteral("deleted") }; QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", Q_ARG(TagChange, change))); @@ -6437,26 +6444,49 @@ void TestMainWindow::aDifferentTagOnTheSameMessageStillCounts() "two different tags on one message cancelled each other"); } -void TestMainWindow::anEditWithNoMessageIdsStillCounts() +void TestMainWindow::everyPendingChangeCanNameItsMessages() { - // 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. + // Item 119. The count summed a fourth term, a bare int for confirmed + // changes carrying no message ids, and that term is what made the count + // impossible to open and list: three groups could name what they held and + // the remainder could not. + // + // The remainder was empty. NotmuchWorker::applyTags() is the only emitter + // of tagsApplied() and returns early on an empty id list, which is the + // exact condition the counter required, so the change it existed for + // cannot reach this window. Measured before removing it: a qFatal in the + // branch fired in 4 of 70 cases here, all four invoking the slot directly + // with a hand-built TagChange, and an assertion before the worker's emit + // never fired across the whole suite. + // + // The guard itself is pinned in test_notmuchworker by + // applyTagsWithNoIdsDoesNothing(), which is where it lives. What this test + // holds is the consequence: a change that reaches the indicator names its + // messages, so the indicator can be listed in full. const Config config; MainWindow window(config); auto *label = window.findChild(QStringLiteral("pendingEdits")); QVERIFY(label); + QVERIFY(label->isHidden()); + // A change with ids counts, and the count is the number of (message, tag) + // pairs it carries rather than one per signal. TagChange change; - change.added = { QStringLiteral("deleted") }; - change.description = QStringLiteral("Delete"); + change.messageIds = { QStringLiteral("a@example.org"), + QStringLiteral("b@example.org") }; + change.added = { QStringLiteral("flagged") }; + change.description = QStringLiteral("Mark important"); QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", Q_ARG(TagChange, change))); + QVERIFY(!label->isHidden()); - QVERIFY2(!label->isHidden(), - "an edit with no message ids was not counted at all"); + // And its inverse nets it back to nothing, which is the property the + // fourth term could never have: an unnettable count only ever grew. + QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", + Q_ARG(TagChange, change.inverted()))); + QVERIFY2(label->isHidden(), + "an edit and its inverse left the indicator claiming work"); } // Item 37. A tag edit made while a background sync holds notmuch's write lock -- cgit v1.2.3