From 2b6ecd2a1f624c12c071320ff1ed96404d75f839 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 31 Oct 2026 12:43:21 +0100 Subject: fix: handle every rule answer, and keep its failure out of errorOccurred A second Apply overwrote the pending token, so the first rule's answer was dropped as stale although its writes had landed: no undo, no pending count, no accounts. A write's answer cannot be superseded, so the token is gone and every ruleApplied() is handled. Pending counts are keyed by generation for the same reason, since a second Apply before the first count returned dropped the first Apply entirely. applyRule() no longer emits errorOccurred(). That path reverted an unrelated in-flight user write and its message was then overwritten by "nothing to change". The failure travels in the reply instead: ruleApplied(description, changes, error). openReadOnly() and writeTags() take an optional error out-parameter so the rule's path stays quiet. The window shows the error, still records and offers undo for whatever landed first, and names the rule from the reply's description. Item 212. Co-Authored-By: Claude Opus 5.5 --- src/mainwindow.cpp | 48 ++++++++++++++++++++--------------------- src/mainwindow.h | 27 ++++++++++++----------- src/notmuchworker.cpp | 49 ++++++++++++++++++++++++++---------------- src/notmuchworker.h | 30 ++++++++++++++++---------- tests/test_mainwindow.cpp | 51 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_notmuchworker.cpp | 41 ++++++++++++++++++++++++++++------- 6 files changed, 173 insertions(+), 73 deletions(-) diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 918bd7b..3ee31aa 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -2827,10 +2827,8 @@ void MainWindow::onRuleCountsReady(const QVector &counts, // Item 212. Apply's count first: it shares this signal and the generation // sequence, and needs no dialog, since the test and a closed dialog's // last Apply both arrive here without one. - if (m_pendingApplyGeneration != 0 - && generation == m_pendingApplyGeneration) { - m_pendingApplyGeneration = 0; - confirmRuleApply(counts.value(0, -1)); + if (m_pendingApplies.contains(generation)) { + confirmRuleApply(m_pendingApplies.take(generation), counts.value(0, -1)); return; } @@ -2863,12 +2861,12 @@ void MainWindow::applyRuleToExistingMail(const TagRule &rule) const QStringList queries{ SearchTerm::changes(rule.query, rule.add, rule.remove) }; - m_pendingApplyRule = rule; - m_pendingApplyGeneration = ++m_ruleCountGeneration; + const quint64 generation = ++m_ruleCountGeneration; + m_pendingApplies.insert(generation, rule); QMetaObject::invokeMethod( m_worker, "requestMessageCounts", Qt::QueuedConnection, Q_ARG(QStringList, queries), - Q_ARG(quint64, m_pendingApplyGeneration)); + Q_ARG(quint64, generation)); } TagCommand *MainWindow::armTagCommandForTesting(const QStringList &add, @@ -2882,9 +2880,8 @@ TagCommand *MainWindow::armTagCommandForTesting(const QStringList &add, return command; } -void MainWindow::confirmRuleApply(int count) +void MainWindow::confirmRuleApply(const TagRule &rule, int count) { - const TagRule rule = m_pendingApplyRule; if (count < 0) { showTransientStatus( tr("Rule \"%1\" could not be counted, so nothing was applied.") @@ -2929,22 +2926,18 @@ void MainWindow::confirmRuleApply(int count) } const QString description = tr("Apply rule \"%1\"").arg(rule.id); - m_pendingRuleToken = ++m_lastRuleToken; QMetaObject::invokeMethod( m_worker, "applyRule", Qt::QueuedConnection, Q_ARG(QString, rule.query), Q_ARG(QStringList, rule.add), - Q_ARG(QStringList, rule.remove), Q_ARG(QString, description), - Q_ARG(quint64, m_pendingRuleToken)); + Q_ARG(QStringList, rule.remove), Q_ARG(QString, description)); announceAction(tr("%1: %n message(s)", "", count).arg(description)); } -void MainWindow::onRuleApplied(quint64 token, - const QVector &changes) +void MainWindow::onRuleApplied(const QString &description, + const QVector &changes, + const QString &error) { - if (token == 0 || token != m_pendingRuleToken) - return; - m_pendingRuleToken = 0; // One undo entry for the whole rule, one command per operation, each // built from the ids that operation MOVED (item 176). The command's first @@ -2967,15 +2960,22 @@ void MainWindow::onRuleApplied(quint64 token, m_undoStack.push(command); } - if (!anyMoved) { - // Another writer got there between the count and the write. The - // ticket bump stops announceAction() claiming "done" over this. + if (anyMoved) + m_undoStack.endMacro(); + + // The ticket bump stops announceAction() claiming "done" over either + // message below. + if (!error.isEmpty()) { ++m_announceTicket; - showTransientStatus( - tr("Rule \"%1\" has nothing to change.").arg(m_pendingApplyRule.id)); - return; + // Not transient: a failed write is state the user has to read. + m_statusLabel->setText(tr("%1 failed: %2").arg(description, error)); + } else if (!anyMoved) { + // Another writer got there between the count and the write. + ++m_announceTicket; + showTransientStatus(tr("%1: nothing to change.").arg(description)); } - m_undoStack.endMacro(); + if (!anyMoved) + return; // Every account, not the ones the model knows: a backfill reaches mail no // row shows, so its accounts cannot be resolved through the model the way diff --git a/src/mainwindow.h b/src/mainwindow.h index f962089..2766db2 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -864,7 +864,14 @@ private slots: /// meanwhile clears that queue, and the backfill's confirmation was then /// credited to the user's command, whose undo inverted it over thousands /// of messages. - void onRuleApplied(quint64 token, const QVector &changes); + /// + /// Every answer is handled: there is no token to judge one stale by, + /// because a write's answer cannot be superseded. Two Applies in flight + /// both landed, and each needs its undo and its pending count. Messages + /// name the rule from `description`, which the reply carries. A non-empty + /// `error` is shown, and whatever landed before it is still recorded. + void onRuleApplied(const QString &description, + const QVector &changes, const QString &error); private: /// What onTagsApplied() does for any confirmed write, apart from crediting @@ -874,7 +881,7 @@ private: /// The count for applyRuleToExistingMail() arrived: confirm, then send /// the rule to the worker's applyRule(). - void confirmRuleApply(int count); + void confirmRuleApply(const TagRule &rule, int count); void buildUi(); @@ -1856,16 +1863,12 @@ private: /// counter. quint64 m_ruleCountGeneration = 0; - /// The rule awaiting its change count, and the generation that count - /// will carry. Shares m_ruleCountGeneration's sequence, so a pending - /// Apply and the dialog's Count matches can never be confused. - TagRule m_pendingApplyRule; - quint64 m_pendingApplyGeneration = 0; - - /// The token the outstanding applyRule() will answer with, 0 when none. - /// A reply carrying any other token is stale and dropped. - quint64 m_pendingRuleToken = 0; - quint64 m_lastRuleToken = 0; + /// The rules awaiting their change count, keyed by the generation each + /// count will carry. A map, not one slot: a second Apply before the first + /// count returns must not make the first look stale. Shares + /// m_ruleCountGeneration's sequence, so an Apply and the dialog's Count + /// matches can never be confused. + QHash m_pendingApplies; /// The generation of a REFRESH query, run after a sync to bring the list /// up to date without disturbing it. diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index e5f5afa..ebe4851 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -390,7 +390,7 @@ QByteArray NotmuchWorker::configPathArg() const return m_configPath.isEmpty() ? QByteArray() : m_configPath.toLocal8Bit(); } -bool NotmuchWorker::openReadOnly() +bool NotmuchWorker::openReadOnly(QString *error) { if (m_db) { // A read-only handle is a Xapian SNAPSHOT taken when it was opened, so @@ -407,7 +407,7 @@ bool NotmuchWorker::openReadOnly() // results beats refusing to answer at all. const notmuch_status_t status = notmuch_database_reopen(m_db, NOTMUCH_DATABASE_MODE_READ_ONLY); - if (status != NOTMUCH_STATUS_SUCCESS) { + if (status != NOTMUCH_STATUS_SUCCESS && !error) { emit errorOccurred( QStringLiteral("Cannot refresh notmuch database: %1") .arg(QString::fromUtf8(notmuch_status_to_string(status)))); @@ -416,19 +416,26 @@ bool NotmuchWorker::openReadOnly() } const QByteArray configPath = configPathArg(); - char *error = nullptr; + char *openError = nullptr; const notmuch_status_t status = notmuch_database_open_with_config( nullptr, // let config decide path NOTMUCH_DATABASE_MODE_READ_ONLY, configPath.isEmpty() ? nullptr : configPath.constData(), nullptr, &m_db, - &error); + &openError); if (status != NOTMUCH_STATUS_SUCCESS) { - emit errorOccurred(QStringLiteral("Cannot open notmuch database: %1") - .arg(QString::fromUtf8(error ? error : notmuch_status_to_string(status)))); - free(error); + const QString message = + QStringLiteral("Cannot open notmuch database: %1") + .arg(QString::fromUtf8(openError + ? openError + : notmuch_status_to_string(status))); + if (error) + *error = message; + else + emit errorOccurred(message); + free(openError); m_db = nullptr; return false; } @@ -1092,19 +1099,20 @@ void NotmuchWorker::applyTagsToThreads(const QStringList &threadIds, void NotmuchWorker::applyRule(const QString &query, const QStringList &add, const QStringList &remove, - const QString &description, quint64 token) + const QString &description) { QVector changes; - if (!openReadOnly()) { - emit ruleApplied(token, changes); + QString error; + if (!openReadOnly(&error)) { + emit ruleApplied(description, changes, error); return; } QStringList messageIds; if (!collectMessageIds(m_db, SearchTerm::changes(query, add, remove), &messageIds)) { - emit errorOccurred(QStringLiteral("Cannot resolve the rule's query")); - emit ruleApplied(token, changes); + emit ruleApplied(description, changes, + QStringLiteral("Cannot resolve the rule's query")); return; } @@ -1117,12 +1125,12 @@ void NotmuchWorker::applyRule(const QString &query, const QStringList &add, for (TagChange &op : ops) { QStringList changed; // An empty list writes nothing; writeTags() is not even asked to open. - if (!messageIds.isEmpty() && !writeTags(op, &changed)) - break; // errorOccurred() already emitted; report what landed + if (!messageIds.isEmpty() && !writeTags(op, &changed, &error)) + break; // report what landed before the failure, and the failure op.messageIds = changed; changes.append(op); } - emit ruleApplied(token, changes); + emit ruleApplied(description, changes, error); } void NotmuchWorker::applyTags(const TagChange &change) @@ -1146,7 +1154,8 @@ void NotmuchWorker::applyTags(const TagChange &change) change.description }); } -bool NotmuchWorker::writeTags(const TagChange &change, QStringList *changedIds) +bool NotmuchWorker::writeTags(const TagChange &change, QStringList *changedIds, + QString *errorOut) { // The read-only handle must be closed first: notmuch allows only one open @@ -1174,10 +1183,14 @@ bool NotmuchWorker::writeTags(const TagChange &change, QStringList *changedIds) // // The stall a running sync DOES cause is avoided upstream, in // MainWindow, by not sending the write at all while the lock is held. - emit errorOccurred( + const QString message = QStringLiteral("Cannot open database for writing: %1") .arg(QString::fromUtf8(error ? error - : notmuch_status_to_string(status)))); + : notmuch_status_to_string(status))); + if (errorOut) + *errorOut = message; + else + emit errorOccurred(message); free(error); return false; } diff --git a/src/notmuchworker.h b/src/notmuchworker.h index a175d7a..63c5203 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -246,10 +246,11 @@ public slots: /// /// Answers with ruleApplied() ALWAYS, even when nothing moved, and never /// with tagsApplied(): the window builds the undo from the answer rather - /// than from the shared confirmation queue. + /// than from the shared confirmation queue. Never with errorOccurred() + /// either: that path reverts whatever unrelated user write is in flight, + /// so a failure travels in the reply. void applyRule(const QString &query, const QStringList &add, - const QStringList &remove, const QString &description, - quint64 token); + const QStringList &remove, const QString &description); /// Resolves whole threads to the message ids and file paths they contain. /// @@ -398,10 +399,12 @@ signals: /// applyRule()'s answer: one TagChange per single-tag operation, in the /// order applyRule() documents, each carrying the ids that operation - /// actually moved (possibly none). `token` is echoed from the request. - /// Emitted even after a write error, carrying what was written before it, - /// so the window can still offer undo for that part. - void ruleApplied(quint64 token, const QVector &changes); + /// actually moved (possibly none). `description` is echoed from the + /// request, so the window names the right rule. `error` is empty on + /// success; on a failure, `changes` holds what was written before it, so + /// the window can still offer undo for that part. + void ruleApplied(const QString &description, + const QVector &changes, const QString &error); /// One file entered or left the index outside a query (item 192). /// @@ -497,13 +500,18 @@ signals: void errorOccurred(const QString &message); private: - bool openReadOnly(); + /// With `error` given, a failure is written there instead of emitted, and + /// the non-fatal reopen failure is ignored; applyRule() reports in its + /// own reply. + bool openReadOnly(QString *error = nullptr); void close(); /// applyTags() without the signal: writes `change` and fills `changedIds` - /// with the messages whose tags really moved. False, with errorOccurred() - /// emitted, when the database could not be opened for writing. - bool writeTags(const TagChange &change, QStringList *changedIds); + /// with the messages whose tags really moved. False when the database + /// could not be opened for writing, with the reason in `error` when given + /// and through errorOccurred() otherwise. + bool writeTags(const TagChange &change, QStringList *changedIds, + QString *error = nullptr); QByteArray configPathArg() const; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 17c6f8c..5eed336 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -301,6 +301,7 @@ private slots: void applyingARuleConfirmsWithTheCountAndUndoesExactly(); void applyingARuleWhoseQueryNamesItsOwnTagWritesAllOfIt(); void aBackfillLeavesAnArmedUserCommandAlone(); + void twoRulesAppliedBackToBackEachGetTheirUndo(); void aPurgeTakesTheRowsOutOfTheViewWithoutARefresh(); void theDashboardFollowsAWriteToTheConversationItShows(); void anEditHeldByASyncSaysSoInsteadOfClaimingItLanded(); @@ -10161,6 +10162,56 @@ void TestMainWindow::aBackfillLeavesAnArmedUserCommandAlone() QStringList{ QStringLiteral("armed@example.org") }); } +void TestMainWindow::twoRulesAppliedBackToBackEachGetTheirUndo() +{ + // Item 212, from review. A second Apply while the first is in flight must + // not make the first's answer look stale: its writes landed, so it needs + // its undo entry and its pending count like any other write. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("inbox"), QStringLiteral("one@example.org"), + QStringLiteral("One"), QStringLiteral("deals@example.net"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body."))); + QVERIFY2(backed.build(), qPrintable(backed.error())); + const QString cfg = backed.fixture().configPath(); + + MainWindow window(backed.config()); + QCOMPARE(window.pendingEditCount(), 0); + + TagRule first; + first.id = QStringLiteral("first"); + first.query = QStringLiteral("from:deals@example.net"); + first.add = { QStringLiteral("ruleone") }; + TagRule second = first; + second.id = QStringLiteral("second"); + second.add = { QStringLiteral("ruletwo") }; + + // Answer every box, however they nest. + QStringList asked; + QTimer poll; + poll.setInterval(20); + QObject::connect(&poll, &QTimer::timeout, [&asked]() { + auto *box = qobject_cast(QApplication::activeModalWidget()); + if (!box) + return; + asked.append(box->text()); + box->button(QMessageBox::Yes)->click(); + }); + poll.start(); + + window.applyRuleToExistingMail(first); + window.applyRuleToExistingMail(second); + QTRY_COMPARE_WITH_TIMEOUT(asked.size(), 2, 15000); + QTRY_COMPARE_WITH_TIMEOUT(window.undoDepthForTesting(), 2, 15000); + + QCOMPARE(fixtureNotmuch(cfg, { QStringLiteral("count"), + QStringLiteral("tag:ruleone and tag:ruletwo") }), + QStringLiteral("1")); + // One message, two (message, tag) edits outstanding. + QCOMPARE(window.pendingEditCount(), 2); +} + /// Item 181, from the user: "the thread dashboard doesn't update live with the /// modifications applied to the list pane. If I mark the thread as read, the /// dash still reports N unread". diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index c4da146..68734ca 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -63,6 +63,7 @@ private slots: void applyRuleResolvesItsQueryOnceForEveryTag(); void applyRuleThatChangesNothingStillAnswers(); void applyRuleAnswersAcrossAQueuedConnection(); + void applyRuleCarriesItsFailureInTheReply(); void applyTagsToThreadsSpansMultipleThreads(); void applyTagsToThreadsWithNoThreadsDoesNothing(); @@ -1318,11 +1319,12 @@ void TestNotmuchWorker::applyRuleWritesEachTagOnlyWhereItMoves() worker.applyRule(QStringLiteral("thread:%1").arg(threadId), { QStringLiteral("ruleadd") }, { QStringLiteral("ruledrop") }, - QStringLiteral("Apply rule"), 7); + QStringLiteral("Apply rule")); QCOMPARE(applied.size(), 0); QCOMPARE(ruled.size(), 1); - QCOMPARE(ruled.first().at(0).value(), quint64(7)); + QCOMPARE(ruled.first().at(0).toString(), QStringLiteral("Apply rule")); + QVERIFY(ruled.first().at(2).toString().isEmpty()); const auto changes = ruled.first().at(1).value>(); QCOMPARE(changes.size(), 2); QCOMPARE(changes.at(0).added, QStringList{ QStringLiteral("ruleadd") }); @@ -1364,7 +1366,7 @@ void TestNotmuchWorker::applyRuleResolvesItsQueryOnceForEveryTag() worker.applyRule( QStringLiteral("thread:%1 and tag:backfill-src").arg(threadId), {}, { QStringLiteral("backfill-src"), QStringLiteral("backfill-other") }, - QStringLiteral("Apply rule"), 1); + QStringLiteral("Apply rule")); QCOMPARE(ruled.size(), 1); const auto changes = ruled.first().at(1).value>(); @@ -1390,11 +1392,11 @@ void TestNotmuchWorker::applyRuleThatChangesNothingStillAnswers() QSignalSpy ruled(&worker, &NotmuchWorker::ruleApplied); worker.applyRule(QStringLiteral("thread:%1").arg(threadId), {}, { QStringLiteral("nobody-has-this") }, - QStringLiteral("Apply rule"), 3); + QStringLiteral("Apply rule")); QCOMPARE(applied.size(), 0); QCOMPARE(ruled.size(), 1); - QCOMPARE(ruled.first().at(0).value(), quint64(3)); + QVERIFY(ruled.first().at(2).toString().isEmpty()); const auto changes = ruled.first().at(1).value>(); for (const TagChange &change : changes) QVERIFY(change.messageIds.isEmpty()); @@ -1417,7 +1419,8 @@ void TestNotmuchWorker::applyRuleAnswersAcrossAQueuedConnection() int answers = 0; QVector received; connect(&worker, &NotmuchWorker::ruleApplied, &receiver, - [&](quint64, const QVector &changes) { + [&](const QString &, const QVector &changes, + const QString &) { ++answers; received = changes; }); @@ -1426,8 +1429,7 @@ void TestNotmuchWorker::applyRuleAnswersAcrossAQueuedConnection() Q_ARG(QString, QStringLiteral("thread:%1").arg(threadId)), Q_ARG(QStringList, QStringList{ QStringLiteral("crossed") }), Q_ARG(QStringList, QStringList{}), - Q_ARG(QString, QStringLiteral("Apply rule")), - Q_ARG(quint64, 9))); + Q_ARG(QString, QStringLiteral("Apply rule")))); QTRY_COMPARE_WITH_TIMEOUT(answers, 1, 5000); QCOMPARE(received.size(), 1); @@ -1442,6 +1444,29 @@ void TestNotmuchWorker::applyRuleAnswersAcrossAQueuedConnection() QStringLiteral("restore") }); } +void TestNotmuchWorker::applyRuleCarriesItsFailureInTheReply() +{ + // A backfill's failure travels in its own reply. Through errorOccurred() + // the window would revert whatever unrelated user write was in flight, + // and its message would be overwritten by "nothing to change". + QTemporaryDir dir; + QVERIFY(dir.isValid()); + NotmuchWorker worker(dir.filePath(QStringLiteral("no-such-config"))); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + QSignalSpy ruled(&worker, &NotmuchWorker::ruleApplied); + + worker.applyRule(QStringLiteral("tag:inbox"), { QStringLiteral("x") }, {}, + QStringLiteral("Apply rule \"x\"")); + + QCOMPARE(errors.size(), 0); + QCOMPARE(ruled.size(), 1); + QCOMPARE(ruled.first().at(0).toString(), QStringLiteral("Apply rule \"x\"")); + QVERIFY(!ruled.first().at(2).toString().isEmpty()); + const auto changes = ruled.first().at(1).value>(); + for (const TagChange &change : changes) + QVERIFY(change.messageIds.isEmpty()); +} + void TestNotmuchWorker::applyTagsToThreadsSpansMultipleThreads() { const QString threadA = threadIdOf(QStringLiteral("Release notes")); -- cgit v1.2.3