diff options
| -rw-r--r-- | src/mainwindow.cpp | 21 | ||||
| -rw-r--r-- | src/mainwindow.h | 5 | ||||
| -rw-r--r-- | 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<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. /// /// 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<QLabel *>(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 |