aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/mainwindow.cpp48
-rw-r--r--src/mainwindow.h27
-rw-r--r--src/notmuchworker.cpp49
-rw-r--r--src/notmuchworker.h30
-rw-r--r--tests/test_mainwindow.cpp51
-rw-r--r--tests/test_notmuchworker.cpp41
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<int> &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<TagChange> &changes)
+void MainWindow::onRuleApplied(const QString &description,
+ const QVector<TagChange> &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<TagChange> &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<TagChange> &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<quint64, TagRule> 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<TagChange> 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<TagChange> &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<TagChange> &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<QMessageBox *>(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>(), 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<QVector<TagChange>>();
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<QVector<TagChange>>();
@@ -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>(), quint64(3));
+ QVERIFY(ruled.first().at(2).toString().isEmpty());
const auto changes = ruled.first().at(1).value<QVector<TagChange>>();
for (const TagChange &change : changes)
QVERIFY(change.messageIds.isEmpty());
@@ -1417,7 +1419,8 @@ void TestNotmuchWorker::applyRuleAnswersAcrossAQueuedConnection()
int answers = 0;
QVector<TagChange> received;
connect(&worker, &NotmuchWorker::ruleApplied, &receiver,
- [&](quint64, const QVector<TagChange> &changes) {
+ [&](const QString &, const QVector<TagChange> &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<QVector<TagChange>>();
+ for (const TagChange &change : changes)
+ QVERIFY(change.messageIds.isEmpty());
+}
+
void TestNotmuchWorker::applyTagsToThreadsSpansMultipleThreads()
{
const QString threadA = threadIdOf(QStringLiteral("Release notes"));