aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/notmuchworker.cpp77
-rw-r--r--src/notmuchworker.h38
-rw-r--r--tests/test_notmuchworker.cpp156
3 files changed, 221 insertions, 50 deletions
diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp
index d41b54b..e5f5afa 100644
--- a/src/notmuchworker.cpp
+++ b/src/notmuchworker.cpp
@@ -362,12 +362,19 @@ static const int kSenderCountsMetaType =
static const int kThreadDigestMetaType =
qRegisterMetaType<ThreadDigest>("ThreadDigest");
+/// And for applyRule()'s answer, which crosses the queued ruleApplied
+/// connection as a container. An unregistered one is dropped with only a
+/// warning, and the window would then never build the rule's undo entry.
+static const int kTagChangesMetaType =
+ qRegisterMetaType<QVector<TagChange>>("QVector<TagChange>");
+
NotmuchWorker::NotmuchWorker(const QString &notmuchConfigPath, QObject *parent)
: QObject(parent), m_configPath(notmuchConfigPath)
{
Q_UNUSED(kSortOrderMetaType);
Q_UNUSED(kSenderCountsMetaType);
Q_UNUSED(kThreadDigestMetaType);
+ Q_UNUSED(kTagChangesMetaType);
}
NotmuchWorker::~NotmuchWorker()
@@ -1083,25 +1090,39 @@ void NotmuchWorker::applyTagsToThreads(const QStringList &threadIds,
applyTags(TagChange{ messageIds, add, remove, description });
}
-void NotmuchWorker::applyTagsToQuery(const QString &query,
- const QStringList &add,
- const QStringList &remove,
- const QString &description)
+void NotmuchWorker::applyRule(const QString &query, const QStringList &add,
+ const QStringList &remove,
+ const QString &description, quint64 token)
{
- if (!openReadOnly())
+ QVector<TagChange> changes;
+ if (!openReadOnly()) {
+ emit ruleApplied(token, changes);
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);
return;
}
- // Empty is not an error: the rule has nothing left to do. applyTags()
- // returns at once on an empty list, and stays silent, as it must
- // (CLAUDE.md, item 176).
- applyTags(TagChange{ messageIds, add, remove, description });
+ QList<TagChange> ops;
+ for (const QString &tag : add)
+ ops.append(TagChange{ messageIds, { tag }, {}, description });
+ for (const QString &tag : remove)
+ ops.append(TagChange{ messageIds, {}, { tag }, description });
+
+ 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
+ op.messageIds = changed;
+ changes.append(op);
+ }
+ emit ruleApplied(token, changes);
}
void NotmuchWorker::applyTags(const TagChange &change)
@@ -1109,6 +1130,25 @@ void NotmuchWorker::applyTags(const TagChange &change)
if (change.messageIds.isEmpty())
return;
+ QStringList changedIds;
+ if (!writeTags(change, &changedIds))
+ return;
+
+ // Nothing moved, so there is nothing to record as pending, nothing to sync
+ // and nothing to undo. Emitting an empty change would push an undo entry
+ // whose inverse adds a tag no message ever carried, which is item 176 one
+ // step further along. The same silence as the empty-id early return above,
+ // for the same reason.
+ if (changedIds.isEmpty())
+ return;
+
+ emit tagsApplied(TagChange{ changedIds, change.added, change.removed,
+ change.description });
+}
+
+bool NotmuchWorker::writeTags(const TagChange &change, QStringList *changedIds)
+{
+
// The read-only handle must be closed first: notmuch allows only one open
// handle per process.
close();
@@ -1139,7 +1179,7 @@ void NotmuchWorker::applyTags(const TagChange &change)
.arg(QString::fromUtf8(error ? error
: notmuch_status_to_string(status))));
free(error);
- return;
+ return false;
}
// Item 176. What the caller ASKED for is not what the write does: a
@@ -1149,8 +1189,8 @@ void NotmuchWorker::applyTags(const TagChange &change)
// unread", measured on real mail as 2 unread becoming 43. With
// maildir.synchronize_flags on, that rewrites filenames and reaches the
// server.
- QStringList changedIds;
- changedIds.reserve(change.messageIds.size());
+ changedIds->clear();
+ changedIds->reserve(change.messageIds.size());
for (const QString &id : change.messageIds) {
notmuch_message_t *raw = nullptr;
@@ -1196,7 +1236,7 @@ void NotmuchWorker::applyTags(const TagChange &change)
moves = moves || !before.contains(tag);
moves = moves || !strippedOrigins.isEmpty();
if (moves)
- changedIds.append(id);
+ changedIds->append(id);
notmuch_message_freeze(message.get());
for (const QString &tag : change.removed)
@@ -1215,16 +1255,7 @@ void NotmuchWorker::applyTags(const TagChange &change)
notmuch_database_close(db);
notmuch_database_destroy(db);
- // Nothing moved, so there is nothing to record as pending, nothing to sync
- // and nothing to undo. Emitting an empty change would push an undo entry
- // whose inverse adds a tag no message ever carried, which is item 176 one
- // step further along. The same silence as the empty-id early return above,
- // for the same reason.
- if (changedIds.isEmpty())
- return;
-
- emit tagsApplied(TagChange{ changedIds, change.added, change.removed,
- change.description });
+ return true;
}
void NotmuchWorker::moveMessages(const QStringList &messageIds,
diff --git a/src/notmuchworker.h b/src/notmuchworker.h
index fa223e6..a175d7a 100644
--- a/src/notmuchworker.h
+++ b/src/notmuchworker.h
@@ -232,14 +232,24 @@ public slots:
const QStringList &remove,
const QString &description);
- /// Applies `+add -remove` to every message the change would move within
- /// `query` (item 212, a rule applied to existing mail). The query is
- /// narrowed with SearchTerm::changes() so already-correct messages are
- /// never written, then the ids go through applyTags(), which reports the
- /// effective set for undo. Emits nothing when nothing would change.
- void applyTagsToQuery(const QString &query, const QStringList &add,
- const QStringList &remove,
- const QString &description);
+ /// Applies a tagging rule to existing mail (item 212). Resolves the
+ /// messages the WHOLE rule would move once, with SearchTerm::changes(),
+ /// then writes one tag at a time over that fixed list: `+a` for each add,
+ /// `-r` for each remove.
+ ///
+ /// One write per tag because the effective set is per MESSAGE: a single
+ /// `+promo -inbox` write reports a message that already had promo (it lost
+ /// inbox), and the undo built from it would strip promo. One resolution
+ /// because a query may name a tag the rule removes, and re-resolving after
+ /// the first write would match nothing for the rest, writing half the
+ /// rule where the hook writes all of it.
+ ///
+ /// 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.
+ void applyRule(const QString &query, const QStringList &add,
+ const QStringList &remove, const QString &description,
+ quint64 token);
/// Resolves whole threads to the message ids and file paths they contain.
///
@@ -386,6 +396,13 @@ signals:
void threadDigestLoaded(const ThreadDigest &digest, quint64 generation);
void tagsApplied(const TagChange &change);
+ /// 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);
+
/// One file entered or left the index outside a query (item 192).
///
/// Emitted by indexDraftFile() and removeIndexedFile(), the two entry
@@ -483,6 +500,11 @@ private:
bool openReadOnly();
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);
+
QByteArray configPathArg() const;
QString m_configPath;
diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp
index 9b64f8e..c4da146 100644
--- a/tests/test_notmuchworker.cpp
+++ b/tests/test_notmuchworker.cpp
@@ -59,7 +59,10 @@ private slots:
void aThreadCarriesItsCardMessagesOwnTags();
void applyTagsToThreadsTagsEveryMessage();
- void applyTagsToQueryWritesOnlyWhatWouldChange();
+ void applyRuleWritesEachTagOnlyWhereItMoves();
+ void applyRuleResolvesItsQueryOnceForEveryTag();
+ void applyRuleThatChangesNothingStillAnswers();
+ void applyRuleAnswersAcrossAQueuedConnection();
void applyTagsToThreadsSpansMultipleThreads();
void applyTagsToThreadsWithNoThreadsDoesNothing();
@@ -1291,36 +1294,151 @@ void TestNotmuchWorker::applyTagsToThreadsTagsEveryMessage()
restore.applyTags(emitted.inverted());
}
-void TestNotmuchWorker::applyTagsToQueryWritesOnlyWhatWouldChange()
+void TestNotmuchWorker::applyRuleWritesEachTagOnlyWhereItMoves()
{
- // Item 212. Two messages in OPPOSITE states (item 87's lesson): a1
- // already carries the tag, a2 does not. Only a2 may be reported, or the
- // undo would strip the tag from a message that had it before.
+ // Item 212. Two messages in OPPOSITE states (item 87's lesson): a1 already
+ // carries the added tag, a2 does not, and both carry the removed one. The
+ // effective set is per message, so a single `+add -drop` write would
+ // report a1 for both tags and its undo would strip `add` from a1. One
+ // operation per tag reports a1 only where it moved.
const QString threadId = threadIdOf(QStringLiteral("Release notes"));
QVERIFY(!threadId.isEmpty());
+ const QString a1 = QStringLiteral("a1@example.org");
+ const QString a2 = QStringLiteral("a2@example.org");
NotmuchWorker setup(m_fixture.configPath());
- setup.applyTags(TagChange{ { QStringLiteral("a1@example.org") },
- { QStringLiteral("backfill") }, {},
+ setup.applyTags(TagChange{ { a1 }, { QStringLiteral("ruleadd") }, {},
+ QStringLiteral("seed") });
+ setup.applyTags(TagChange{ { a1, a2 }, { QStringLiteral("ruledrop") }, {},
QStringLiteral("seed") });
NotmuchWorker worker(m_fixture.configPath());
QSignalSpy applied(&worker, &NotmuchWorker::tagsApplied);
- worker.applyTagsToQuery(QStringLiteral("thread:%1").arg(threadId),
- { QStringLiteral("backfill") }, {},
- QStringLiteral("Apply rule"));
+ QSignalSpy ruled(&worker, &NotmuchWorker::ruleApplied);
+ worker.applyRule(QStringLiteral("thread:%1").arg(threadId),
+ { QStringLiteral("ruleadd") },
+ { QStringLiteral("ruledrop") },
+ QStringLiteral("Apply rule"), 7);
+
+ QCOMPARE(applied.size(), 0);
+ QCOMPARE(ruled.size(), 1);
+ QCOMPARE(ruled.first().at(0).value<quint64>(), quint64(7));
+ const auto changes = ruled.first().at(1).value<QVector<TagChange>>();
+ QCOMPARE(changes.size(), 2);
+ QCOMPARE(changes.at(0).added, QStringList{ QStringLiteral("ruleadd") });
+ QVERIFY(changes.at(0).removed.isEmpty());
+ QCOMPARE(changes.at(0).messageIds, QStringList{ a2 });
+ QVERIFY(changes.at(1).added.isEmpty());
+ QCOMPARE(changes.at(1).removed, QStringList{ QStringLiteral("ruledrop") });
+ QCOMPARE(QSet<QString>(changes.at(1).messageIds.cbegin(),
+ changes.at(1).messageIds.cend()),
+ (QSet<QString>{ a1, a2 }));
+ QCOMPARE(changes.at(0).description, QStringLiteral("Apply rule"));
+ QVERIFY(tagsOf(a2).contains(QStringLiteral("ruleadd")));
+ QVERIFY(!tagsOf(a1).contains(QStringLiteral("ruledrop")));
- QCOMPARE(applied.size(), 1);
- const TagChange emitted = applied.first().at(0).value<TagChange>();
- QCOMPARE(emitted.messageIds, QStringList{ QStringLiteral("a2@example.org") });
- QCOMPARE(emitted.description, QStringLiteral("Apply rule"));
- QVERIFY(tagsOf(QStringLiteral("a2@example.org"))
- .contains(QStringLiteral("backfill")));
+ NotmuchWorker restore(m_fixture.configPath());
+ restore.applyTags(TagChange{ { a1, a2 }, {}, { QStringLiteral("ruleadd") },
+ QStringLiteral("restore") });
+}
+
+void TestNotmuchWorker::applyRuleResolvesItsQueryOnceForEveryTag()
+{
+ // A rule whose query names a tag it removes. Re-resolving the query per
+ // operation would let `-src` empty the match before `-other` ran, writing
+ // half the rule. The hook runs a rule once over one query, so the ids are
+ // resolved once.
+ const QString threadId = threadIdOf(QStringLiteral("Release notes"));
+ QVERIFY(!threadId.isEmpty());
+ const QString a1 = QStringLiteral("a1@example.org");
+ const QString a2 = QStringLiteral("a2@example.org");
+
+ NotmuchWorker setup(m_fixture.configPath());
+ setup.applyTags(TagChange{ { a1, a2 },
+ { QStringLiteral("backfill-src"),
+ QStringLiteral("backfill-other") }, {},
+ QStringLiteral("seed") });
+
+ NotmuchWorker worker(m_fixture.configPath());
+ QSignalSpy ruled(&worker, &NotmuchWorker::ruleApplied);
+ worker.applyRule(
+ QStringLiteral("thread:%1 and tag:backfill-src").arg(threadId), {},
+ { QStringLiteral("backfill-src"), QStringLiteral("backfill-other") },
+ QStringLiteral("Apply rule"), 1);
+
+ QCOMPARE(ruled.size(), 1);
+ const auto changes = ruled.first().at(1).value<QVector<TagChange>>();
+ QCOMPARE(changes.size(), 2);
+ QCOMPARE(changes.at(0).messageIds.size(), 2);
+ QCOMPARE(changes.at(1).messageIds.size(), 2);
+ for (const QString &id : { a1, a2 }) {
+ QVERIFY(!tagsOf(id).contains(QStringLiteral("backfill-src")));
+ QVERIFY2(!tagsOf(id).contains(QStringLiteral("backfill-other")),
+ qPrintable(id));
+ }
+}
+
+void TestNotmuchWorker::applyRuleThatChangesNothingStillAnswers()
+{
+ // Unlike applyTags(), which stays silent on an empty write, a rule always
+ // answers: the window is waiting on its token.
+ const QString threadId = threadIdOf(QStringLiteral("Release notes"));
+ QVERIFY(!threadId.isEmpty());
+
+ NotmuchWorker worker(m_fixture.configPath());
+ QSignalSpy applied(&worker, &NotmuchWorker::tagsApplied);
+ QSignalSpy ruled(&worker, &NotmuchWorker::ruleApplied);
+ worker.applyRule(QStringLiteral("thread:%1").arg(threadId), {},
+ { QStringLiteral("nobody-has-this") },
+ QStringLiteral("Apply rule"), 3);
+
+ QCOMPARE(applied.size(), 0);
+ QCOMPARE(ruled.size(), 1);
+ QCOMPARE(ruled.first().at(0).value<quint64>(), quint64(3));
+ const auto changes = ruled.first().at(1).value<QVector<TagChange>>();
+ for (const TagChange &change : changes)
+ QVERIFY(change.messageIds.isEmpty());
+}
+
+void TestNotmuchWorker::applyRuleAnswersAcrossAQueuedConnection()
+{
+ // QVector<TagChange> crosses from the worker thread to the UI, and an
+ // unregistered container is dropped there with only a warning. Received
+ // by a lambda living on THIS thread, so the delivery is genuinely queued.
+ const QString threadId = threadIdOf(QStringLiteral("Release notes"));
+ QVERIFY(!threadId.isEmpty());
+
+ NotmuchWorker worker(m_fixture.configPath());
+ QThread thread;
+ worker.moveToThread(&thread);
+ thread.start();
+
+ QObject receiver;
+ int answers = 0;
+ QVector<TagChange> received;
+ connect(&worker, &NotmuchWorker::ruleApplied, &receiver,
+ [&](quint64, const QVector<TagChange> &changes) {
+ ++answers;
+ received = changes;
+ });
+ QVERIFY(QMetaObject::invokeMethod(
+ &worker, "applyRule", Qt::QueuedConnection,
+ 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)));
+
+ QTRY_COMPARE_WITH_TIMEOUT(answers, 1, 5000);
+ QCOMPARE(received.size(), 1);
+ QCOMPARE(received.at(0).messageIds.size(), 2);
+
+ thread.quit();
+ QVERIFY(thread.wait(5000));
NotmuchWorker restore(m_fixture.configPath());
- restore.applyTags(TagChange{ { QStringLiteral("a1@example.org"),
- QStringLiteral("a2@example.org") },
- {}, { QStringLiteral("backfill") },
+ restore.applyTags(TagChange{ received.at(0).messageIds, {},
+ { QStringLiteral("crossed") },
QStringLiteral("restore") });
}