aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/mainwindow.cpp156
-rw-r--r--src/mainwindow.h40
-rw-r--r--tests/test_mainwindow.cpp141
3 files changed, 263 insertions, 74 deletions
diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
index 8b1c537..918bd7b 100644
--- a/src/mainwindow.cpp
+++ b/src/mainwindow.cpp
@@ -118,21 +118,6 @@ namespace {
/// Overridden only by setLocksPathForTesting(); "/proc/locks" in every real run.
QString g_locksPath = QStringLiteral("/proc/locks");
-/// A rule as single-tag operations: `+a`, `+b`, `-c`, each as (add, remove).
-/// Item 212: the worker's effective set is per MESSAGE, not per tag, so one
-/// `+promo -inbox` write reports a message that already had promo (it lost
-/// inbox) and its undo would strip promo from it. One operation per tag makes
-/// each effective set, and therefore each undo, exact.
-QList<QPair<QStringList, QStringList>> ruleOperations(const TagRule &rule)
-{
- QList<QPair<QStringList, QStringList>> ops;
- for (const QString &tag : rule.add)
- ops.append({ { tag }, {} });
- for (const QString &tag : rule.remove)
- ops.append({ {}, { tag } });
- return ops;
-}
-
} // namespace
/// Doc comment on the declaration. Separators and control characters are
@@ -2845,7 +2830,7 @@ void MainWindow::onRuleCountsReady(const QVector<int> &counts,
if (m_pendingApplyGeneration != 0
&& generation == m_pendingApplyGeneration) {
m_pendingApplyGeneration = 0;
- confirmRuleApply(counts);
+ confirmRuleApply(counts.value(0, -1));
return;
}
@@ -2873,13 +2858,10 @@ void MainWindow::applyRuleToExistingMail(const TagRule &rule)
return;
}
- // One count for the whole rule (what the confirmation names), then one per
- // single-tag operation, in the order ruleOperations() lists them. The
- // per-operation counts decide which writes are sent at all; see
- // confirmRuleApply() for why the rule is split.
- QStringList queries{ SearchTerm::changes(rule.query, rule.add, rule.remove) };
- for (const auto &op : ruleOperations(rule))
- queries.append(SearchTerm::changes(rule.query, op.first, op.second));
+ // The whole rule's count, which is what the confirmation names. The
+ // worker resolves the ids again when it writes; see applyRule().
+ const QStringList queries{ SearchTerm::changes(rule.query, rule.add,
+ rule.remove) };
m_pendingApplyRule = rule;
m_pendingApplyGeneration = ++m_ruleCountGeneration;
@@ -2889,11 +2871,27 @@ void MainWindow::applyRuleToExistingMail(const TagRule &rule)
Q_ARG(quint64, m_pendingApplyGeneration));
}
-void MainWindow::confirmRuleApply(const QVector<int> &counts)
+TagCommand *MainWindow::armTagCommandForTesting(const QStringList &add,
+ const QStringList &remove)
+{
+ auto *command = new MessageTagCommand(
+ this, { QStringLiteral("armed@example.org") }, add, remove,
+ QStringLiteral("test"));
+ awaitTagConfirmation(command);
+ m_undoStack.push(command);
+ return command;
+}
+
+void MainWindow::confirmRuleApply(int count)
{
const TagRule rule = m_pendingApplyRule;
- const int count = counts.value(0, 0);
- if (count <= 0) {
+ if (count < 0) {
+ showTransientStatus(
+ tr("Rule \"%1\" could not be counted, so nothing was applied.")
+ .arg(rule.id));
+ return;
+ }
+ if (count == 0) {
showTransientStatus(
tr("Rule \"%1\" has nothing to change.").arg(rule.id));
return;
@@ -2913,9 +2911,10 @@ void MainWindow::confirmRuleApply(const QVector<int> &counts)
box.setIcon(QMessageBox::Question);
box.setWindowTitle(tr("Apply rule"));
box.setText(tr("Apply rule \"%1\" to %n message(s)?", "", count).arg(rule.id));
+ // Until the next search, because runQuery() clears the undo stack.
box.setInformativeText(what.join(QStringLiteral(" "))
+ QStringLiteral("\n")
- + tr("You can undo this from the main window."));
+ + tr("You can undo this until the next search."));
box.setStandardButtons(QMessageBox::Yes | QMessageBox::Cancel);
box.setDefaultButton(QMessageBox::Cancel);
if (box.exec() != QMessageBox::Yes)
@@ -2930,35 +2929,64 @@ void MainWindow::confirmRuleApply(const QVector<int> &counts)
}
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));
+
+ announceAction(tr("%1: %n message(s)", "", count).arg(description));
+}
+
+void MainWindow::onRuleApplied(quint64 token,
+ const QVector<TagChange> &changes)
+{
+ if (token == 0 || token != m_pendingRuleToken)
+ return;
+ m_pendingRuleToken = 0;
- // One write per single-tag operation, grouped so one Ctrl+Z reverts the
- // whole rule (see ruleOperations() for why it is split). An operation
- // whose count was 0 is NOT sent: applyTags() stays silent on an empty
- // write, and an armed command that never hears back would steal the next
- // write's confirmation. Commands are built with no ids; the worker
- // resolves them and the confirmation fills each one (item 176).
- const auto ops = ruleOperations(rule);
- m_undoStack.beginMacro(description);
- bool first = true;
- for (int i = 0; i < ops.size(); ++i) {
- if (counts.value(i + 1, 0) <= 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
+ // redo, on push, is the no-op it always is; with the ids known, a later
+ // redo would also be correct.
+ bool anyMoved = false;
+ for (const TagChange &change : changes) {
+ if (change.messageIds.isEmpty())
continue;
- auto *command = new MessageTagCommand(this, {}, ops.at(i).first,
- ops.at(i).second, description);
- awaitTagConfirmation(command, first);
- first = false;
- QMetaObject::invokeMethod(
- m_worker, "applyTagsToQuery", Qt::QueuedConnection,
- Q_ARG(QString, rule.query), Q_ARG(QStringList, ops.at(i).first),
- Q_ARG(QStringList, ops.at(i).second), Q_ARG(QString, description));
+ if (!anyMoved)
+ m_undoStack.beginMacro(change.description);
+ anyMoved = true;
+ recordConfirmedTagChange(change);
+ auto *command = new MessageTagCommand(this, change.messageIds,
+ change.added, change.removed,
+ change.description);
+ // What undo() reads: the ids this operation moved, which here are
+ // known up front rather than filled in by a later confirmation.
+ command->addChangedMessageIds(change.messageIds);
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.
+ ++m_announceTicket;
+ showTransientStatus(
+ tr("Rule \"%1\" has nothing to change.").arg(m_pendingApplyRule.id));
+ return;
+ }
m_undoStack.endMacro();
+ // 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
+ // sendMessageTagChange() does it. Missing one would let a narrowed sync
+ // clear the pending count without pushing that account's flag changes.
+ for (const Account &account : m_config.accounts())
+ m_editedAccounts.insert(account.key);
+
// The rows a backfill touches are mostly not in the model, so the list is
- // re-queried once the write has run rather than updated optimistically.
- announceAction(tr("%1: %n message(s)", "", count).arg(description),
- [this]() { refreshCurrentQuery(); });
+ // re-queried rather than updated optimistically.
+ refreshCurrentQuery();
}
void MainWindow::showAbout()
@@ -3040,6 +3068,8 @@ void MainWindow::wireWorker()
this, &MainWindow::onDatabaseStatsReady);
connect(m_worker, &NotmuchWorker::messageCountsReady,
this, &MainWindow::onRuleCountsReady);
+ connect(m_worker, &NotmuchWorker::ruleApplied,
+ this, &MainWindow::onRuleApplied);
// Sender counts feed the business-senders candidate list after a sync.
// The connection is queued, so the QHash argument must be a registered
@@ -5294,12 +5324,6 @@ void MainWindow::onTagsApplied(const TagChange &change)
m_pendingChange = {};
m_pendingThreadIds.clear();
- // Item 181. HERE, where a write is CONFIRMED, and not where one is sent:
- // the digest is rebuilt from the INDEX, so a refresh queued beside the
- // write would race it and answer from the state before it. The dashboard
- // covers every write for the same reason the indicator below does.
- refreshDashboardDigest();
-
// Item 176. The ids the worker reports are the ones whose tags really
// moved, which is what the undo entry has to invert. Inverting the REQUEST
// instead turned the undo of "mark 44 read" into "mark 44 unread": 2
@@ -5310,6 +5334,17 @@ void MainWindow::onTagsApplied(const TagChange &change)
command->addChangedMessageIds(change.messageIds);
}
+ recordConfirmedTagChange(change);
+}
+
+void MainWindow::recordConfirmedTagChange(const TagChange &change)
+{
+ // Item 181. HERE, where a write is CONFIRMED, and not where one is sent:
+ // the digest is rebuilt from the INDEX, so a refresh queued beside the
+ // write would race it and answer from the state before it. The dashboard
+ // covers every write for the same reason the indicator below does.
+ refreshDashboardDigest();
+
// 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.
@@ -6007,14 +6042,11 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state)
}
-void MainWindow::announceAction(const QString &text,
- const std::function<void()> &onDone)
+void MainWindow::announceAction(const QString &text)
{
if (!aSyncHoldsTheWriteLock()) {
if (!m_worker) {
showTransientStatus(text);
- if (onDone)
- onDone();
return;
}
@@ -6032,10 +6064,8 @@ void MainWindow::announceAction(const QString &text,
m_statusTimer->stop();
m_statusLabel->setText(tr("%1, applying…").arg(text));
const quint64 ticket = ++m_announceTicket;
- QMetaObject::invokeMethod(m_worker, [this, text, ticket, onDone]() {
- QMetaObject::invokeMethod(this, [this, text, ticket, onDone]() {
- if (onDone)
- onDone();
+ QMetaObject::invokeMethod(m_worker, [this, text, ticket]() {
+ QMetaObject::invokeMethod(this, [this, text, ticket]() {
// A newer announcement, or an error, owns the label now.
if (ticket == m_announceTicket)
showTransientStatus(tr("%1, done").arg(text));
diff --git a/src/mainwindow.h b/src/mainwindow.h
index 5c9e87b..f962089 100644
--- a/src/mainwindow.h
+++ b/src/mainwindow.h
@@ -167,6 +167,12 @@ public:
/// the worker resolves its query.
void applyRuleToExistingMail(const TagRule &rule);
+ /// Arms one MessageTagCommand on the confirmation queue as a user gesture
+ /// would, opening a new gesture, and pushes it. Returned, owned by the
+ /// undo stack, so a test can show a backfill leaves it alone.
+ TagCommand *armTagCommandForTesting(const QStringList &add,
+ const QStringList &remove);
+
/// Whether the undo stack still holds anything. Exposed so a test can show
/// that a rejected write did not take unrelated history down with it.
bool canUndo() const { return m_undoStack.canUndo(); }
@@ -787,13 +793,7 @@ private slots:
/// describes state lasting until the sync ends, and a message that expired
/// would leave rows showing a tag the database has not got and no
/// explanation of why.
- ///
- /// `onDone`, when given, runs once the worker has reached the write: the
- /// caller's follow-up (a refresh), not a label, so it runs whichever
- /// announcement owns the label by then. Not run on the sync-held branch;
- /// no caller passes one while a sync holds the lock.
- void announceAction(const QString &text,
- const std::function<void()> &onDone = {});
+ void announceAction(const QString &text);
/// Reacts to a sync started outside this window, by cron or by hand.
///
@@ -857,11 +857,24 @@ private slots:
/// request has superseded this one.
void onRuleCountsReady(const QVector<int> &counts, quint64 generation);
+ /// The worker's answer to applyRule(), one change per single-tag
+ /// operation, each carrying the ids it actually moved. Builds the undo
+ /// entry from those KNOWN ids. A backfill never arms a command on the
+ /// shared confirmation queue: it can run for a long time, a gesture made
+ /// 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);
+
private:
- /// The count for applyRuleToExistingMail() arrived: confirm, then write.
- /// `counts` holds the whole rule's count first, then one per single-tag
- /// operation in the order ruleOperations() lists them.
- void confirmRuleApply(const QVector<int> &counts);
+ /// What onTagsApplied() does for any confirmed write, apart from crediting
+ /// an armed undo command: pending edits, the indicator, the auto-sync,
+ /// the dashboard, and the known-tag list. Shared with onRuleApplied().
+ void recordConfirmedTagChange(const TagChange &change);
+
+ /// The count for applyRuleToExistingMail() arrived: confirm, then send
+ /// the rule to the worker's applyRule().
+ void confirmRuleApply(int count);
void buildUi();
@@ -1849,6 +1862,11 @@ private:
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 generation of a REFRESH query, run after a sync to bring the list
/// up to date without disturbing it.
///
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index 4887647..17c6f8c 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -299,6 +299,8 @@ private slots:
void aWorkerBackedWindowReturnsRealThreads();
void aWriteSaysItIsApplyingUntilTheWorkerHasRunIt();
void applyingARuleConfirmsWithTheCountAndUndoesExactly();
+ void applyingARuleWhoseQueryNamesItsOwnTagWritesAllOfIt();
+ void aBackfillLeavesAnArmedUserCommandAlone();
void aPurgeTakesTheRowsOutOfTheViewWithoutARefresh();
void theDashboardFollowsAWriteToTheConversationItShows();
void anEditHeldByASyncSaysSoInsteadOfClaimingItLanded();
@@ -10005,6 +10007,9 @@ void TestMainWindow::applyingARuleConfirmsWithTheCountAndUndoesExactly()
QTRY_COMPARE_WITH_TIMEOUT(
notmuch({ QStringLiteral("count"), QStringLiteral("tag:promo") }),
QStringLiteral("2"), 15000);
+ // The undo entry is built from the worker's answer, which can trail the
+ // write it describes.
+ QTRY_COMPARE_WITH_TIMEOUT(window.undoDepthForTesting(), 1, 15000);
undo->trigger();
QTRY_COMPARE_WITH_TIMEOUT(
@@ -10020,6 +10025,142 @@ void TestMainWindow::applyingARuleConfirmsWithTheCountAndUndoesExactly()
QStringLiteral("1"));
}
+namespace {
+/// Runs the notmuch CLI against a test fixture's config, never the user's.
+QString fixtureNotmuch(const QString &configPath, const QStringList &args)
+{
+ QProcess p;
+ QProcessEnvironment env = QProcessEnvironment::systemEnvironment();
+ env.insert(QStringLiteral("NOTMUCH_CONFIG"), configPath);
+ p.setProcessEnvironment(env);
+ p.start(QStringLiteral("notmuch"), args);
+ p.waitForFinished();
+ return QString::fromUtf8(p.readAllStandardOutput()).trimmed();
+}
+
+/// Clicks Yes on the next QMessageBox, recording its text into `asked`.
+void answerNextBoxYes(QTimer &poll, QString &asked)
+{
+ poll.setInterval(20);
+ QObject::connect(&poll, &QTimer::timeout, [&poll, &asked]() {
+ auto *box = qobject_cast<QMessageBox *>(QApplication::activeModalWidget());
+ if (!box)
+ return;
+ asked = box->text();
+ poll.stop();
+ box->button(QMessageBox::Yes)->click();
+ });
+ poll.start();
+}
+} // namespace
+
+void TestMainWindow::applyingARuleWhoseQueryNamesItsOwnTagWritesAllOfIt()
+{
+ // Item 212. The query names `inbox`, which the rule removes. Resolving
+ // the query again per tag would let `-inbox` empty the match before
+ // `-todo` ran. The hook applies a rule once over one query; Apply must
+ // too, and undo must put back both tags.
+ WorkerBackedWindow backed;
+ const QString date = QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200");
+ QVERIFY(backed.fixture().addMessage(
+ QStringLiteral("inbox"), QStringLiteral("one@example.org"),
+ QStringLiteral("One"), QStringLiteral("deals@example.net"), date,
+ QStringLiteral("Body.")));
+ QVERIFY(backed.fixture().addMessage(
+ QStringLiteral("inbox"), QStringLiteral("two@example.org"),
+ QStringLiteral("Two"), QStringLiteral("deals@example.net"), date,
+ QStringLiteral("Body.")));
+ QVERIFY2(backed.build(), qPrintable(backed.error()));
+ const QString cfg = backed.fixture().configPath();
+ fixtureNotmuch(cfg, { QStringLiteral("tag"), QStringLiteral("+todo"),
+ QStringLiteral("--"),
+ QStringLiteral("from:deals@example.net") });
+ QCOMPARE(fixtureNotmuch(cfg, { QStringLiteral("count"),
+ QStringLiteral("tag:inbox and tag:todo") }),
+ QStringLiteral("2"));
+
+ MainWindow window(backed.config());
+ auto *undo = window.findChild<QAction *>(QStringLiteral("undo"));
+ QVERIFY(undo);
+
+ TagRule rule;
+ rule.id = QStringLiteral("deals");
+ rule.query = QStringLiteral("from:deals@example.net and tag:inbox");
+ rule.remove = { QStringLiteral("inbox"), QStringLiteral("todo") };
+
+ QString asked;
+ QTimer poll;
+ answerNextBoxYes(poll, asked);
+ window.applyRuleToExistingMail(rule);
+ QTRY_VERIFY_WITH_TIMEOUT(!asked.isEmpty(), 15000);
+
+ QTRY_COMPARE_WITH_TIMEOUT(window.undoDepthForTesting(), 1, 15000);
+ QCOMPARE(fixtureNotmuch(cfg, { QStringLiteral("count"),
+ QStringLiteral("tag:inbox") }),
+ QStringLiteral("0"));
+ QCOMPARE(fixtureNotmuch(cfg, { QStringLiteral("count"),
+ QStringLiteral("tag:todo") }),
+ QStringLiteral("0"));
+
+ undo->trigger();
+ QTRY_COMPARE_WITH_TIMEOUT(
+ fixtureNotmuch(cfg, { QStringLiteral("count"),
+ QStringLiteral("tag:inbox and tag:todo") }),
+ QStringLiteral("2"), 15000);
+}
+
+void TestMainWindow::aBackfillLeavesAnArmedUserCommandAlone()
+{
+ // Item 212, from review. A backfill can run for a long time. If it armed
+ // commands on the shared confirmation queue, a gesture made meanwhile
+ // would clear them and take the backfill's confirmation instead, and its
+ // undo would then invert the backfill over every message it touched. A
+ // backfill arms nothing, so a command already waiting stays waiting.
+ 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());
+ TagCommand *user = window.armTagCommandForTesting(
+ {}, { QStringLiteral("inbox") });
+ QVERIFY(user);
+ QCOMPARE(window.undoDepthForTesting(), 1);
+
+ TagRule rule;
+ rule.id = QStringLiteral("deals");
+ rule.query = QStringLiteral("from:deals@example.net");
+ rule.remove = { QStringLiteral("inbox") };
+
+ QString asked;
+ QTimer poll;
+ answerNextBoxYes(poll, asked);
+ window.applyRuleToExistingMail(rule);
+ QTRY_VERIFY_WITH_TIMEOUT(!asked.isEmpty(), 15000);
+ QTRY_COMPARE_WITH_TIMEOUT(window.undoDepthForTesting(), 2, 15000);
+ QCOMPARE(fixtureNotmuch(cfg, { QStringLiteral("count"),
+ QStringLiteral("tag:inbox") }),
+ QStringLiteral("0"));
+
+ // The backfill's ids went nowhere near the user's command.
+ QVERIFY2(user->changedMessageIds().isEmpty(),
+ qPrintable(user->changedMessageIds().join(u',')));
+
+ // And it is still the one waiting: its own confirmation reaches it.
+ TagChange change;
+ change.messageIds = { QStringLiteral("armed@example.org") };
+ change.removed = { QStringLiteral("inbox") };
+ change.description = QStringLiteral("test");
+ QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied",
+ Q_ARG(TagChange, change)));
+ QCOMPARE(user->changedMessageIds(),
+ QStringList{ QStringLiteral("armed@example.org") });
+}
+
/// 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".