aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/mainwindow.cpp47
-rw-r--r--src/threadlistmodel.cpp81
-rw-r--r--src/threadlistmodel.h17
-rw-r--r--tests/test_mainwindow.cpp210
4 files changed, 10 insertions, 345 deletions
diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
index ae5e8fc..fb34fe2 100644
--- a/src/mainwindow.cpp
+++ b/src/mainwindow.cpp
@@ -3355,36 +3355,16 @@ void MainWindow::markCurrentThreadRead()
}
const ThreadSummary thread = m_model->threadAt(current.row());
- if (thread.threadId != m_markReadThreadId) {
+ if (thread.threadId != m_markReadThreadId
+ || !thread.tags.contains(QStringLiteral("unread"))) {
m_markReadThreadId.clear();
return;
}
- // No thread-level `unread` check here any more. It would ask the wrong
- // question now that one message is marked rather than the thread: a thread
- // carries `unread` while ANY message in it is unread, so a read root under
- // unread replies would pass this and a write would be sent for a message
- // that is already read. The scheduling side still checks it, which stops a
- // fully-read thread from arming a timer at all; what survives to here is
- // decided per message below.
-
+ const QStringList threadIds = { m_markReadThreadId };
m_markReadThreadId.clear();
- // The MESSAGE on screen, not the thread it belongs to.
- //
- // This marked the whole thread until item 66, and that was coherent while
- // a root click rendered the whole conversation: everything marked read had
- // been displayed. Once a root began rendering a single message, the same
- // code cleared `unread` from replies the user had never seen. Not a
- // cosmetic slip: maildir.synchronize_flags is on, so removing `unread`
- // rewrites Maildir filenames and the next sync carries it to the server.
- //
- // m_currentMessageId is what the pane actually rendered, set beside the
- // loadMessage that produced it.
- if (m_currentMessageId.isEmpty())
- return;
-
- // sendMessageTagChange, NOT tagSelected: this deliberately does not go on
+ // sendThreadTagChange, NOT tagSelected: this deliberately does not go on
// the undo stack. The user never took this action, so hijacking Ctrl+Z to
// reverse it would undo something they did not do, and toggle_unread
// already gives them a direct way to put it back. Decided 2026-08-03.
@@ -3392,8 +3372,8 @@ void MainWindow::markCurrentThreadRead()
// It still funnels through the one applyTags path, per CLAUDE.md; what
// differs is only whether the inverse is pushed, which is a window-level
// decision above the worker.
- sendMessageTagChange({ m_currentMessageId }, {},
- { QStringLiteral("unread") }, tr("Mark read"));
+ sendThreadTagChange(threadIds, {}, { QStringLiteral("unread") },
+ tr("Mark read"));
}
void MainWindow::editTagsOnSelection()
@@ -3482,17 +3462,10 @@ void MainWindow::sendMessageTagChange(const QStringList &messageIds,
if (messageIds.isEmpty())
return;
- // Optimistic, but scoped to the message. applyTagChange() is keyed by
- // THREAD and would repaint the whole row as though every message in it had
- // changed, which for a one-message edit is a lie; applyMessageTagChange()
- // updates that message and lets the thread's own tags follow only when the
- // answer is unambiguous.
- //
- // Not optional for auto mark-read: without it the write goes out, the
- // status bar counts an unsynced edit, and the card stays bold with
- // `unread` on it until the next query. The user reported exactly that.
- for (const QString &messageId : messageIds)
- m_model->applyMessageTagChange(messageId, add, remove);
+ // No optimistic model update. applyTagChange is keyed by THREAD and would
+ // repaint the whole row as though every message in it had changed, which
+ // for a one-message edit is a lie the user would see and then watch
+ // silently correct itself on the next query.
// The accounts this touches, resolved through the containing threads: the
// account is a property of the thread, and the sync needs the channel
diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp
index 04a5493..3e079ed 100644
--- a/src/threadlistmodel.cpp
+++ b/src/threadlistmodel.cpp
@@ -888,84 +888,3 @@ void ThreadListModel::applyTagChange(const QString &threadId,
return;
}
}
-
-void ThreadListModel::applyMessageTagChange(const QString &messageId,
- const QStringList &added,
- const QStringList &removed)
-{
- const auto retag = [&added, &removed](QStringList *tags) {
- for (const QString &tag : removed)
- tags->removeAll(tag);
- for (const QString &tag : added) {
- if (!tags->contains(tag))
- tags->append(tag);
- }
- };
-
- for (int row = 0; row < m_threads.size(); ++row) {
- ThreadNode &node = m_threads[row];
- bool touched = false;
-
- // `first` is a copy of the opening message rather than an alias into
- // children, so both have to be updated when they name the same one.
- if (node.first.messageId == messageId) {
- retag(&node.first.tags);
- touched = true;
- }
-
- for (int child = 0; child < node.children.size(); ++child) {
- if (node.children.at(child).messageId != messageId)
- continue;
- retag(&node.children[child].tags);
- touched = true;
- const QModelIndex childIndex = index(child, 0, index(row, 0));
- emit dataChanged(childIndex, childIndex);
- }
-
- // The thread has not been expanded and does not open with this
- // message, so nothing here holds it. The summary may still need to
- // follow, which the totalCount check below decides.
- if (!touched && node.summary.firstMessageId != messageId
- && node.summary.totalCount > 1) {
- continue;
- }
-
- // The thread's own tags follow only when the answer is unambiguous.
- //
- // A thread carries `unread` while ANY of its messages does, so a
- // one-message change can only clear it from the thread when there is
- // nothing else left to carry it. With one message in the thread that
- // is certain. With more, the honest answer needs every message's tags,
- // which are only loaded once the thread has been expanded; until then
- // the summary is left alone rather than guessed at, and the next query
- // corrects it.
- const bool wholeThread =
- node.summary.totalCount <= 1
- || (!node.children.isEmpty()
- && node.children.size() >= node.summary.totalCount);
- if (!wholeThread) {
- if (touched)
- emit dataChanged(index(row, 0), index(row, 0));
- continue;
- }
-
- for (const QString &tag : removed) {
- bool stillHeld = false;
- for (const MessageNode &child : node.children) {
- if (child.messageId != messageId && child.tags.contains(tag)) {
- stillHeld = true;
- break;
- }
- }
- if (!stillHeld)
- node.summary.tags.removeAll(tag);
- }
- for (const QString &tag : added) {
- if (!node.summary.tags.contains(tag))
- node.summary.tags.append(tag);
- }
-
- emit dataChanged(index(row, 0), index(row, 0));
- return;
- }
-}
diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h
index f7abaf8..2b8d2b0 100644
--- a/src/threadlistmodel.h
+++ b/src/threadlistmodel.h
@@ -273,23 +273,6 @@ public:
void applyTagChange(const QString &threadId, const QStringList &added,
const QStringList &removed);
- /// The same, scoped to ONE message.
- ///
- /// Updates that message's own tags wherever it is held: as a child row if
- /// the thread is expanded, and as `first` when it is the thread's opening
- /// message. The thread's summary tags follow only when the change is
- /// unambiguous for the whole thread, which for `unread` means no other
- /// message still carries it, since a thread reads as unread while any of
- /// its messages does.
- ///
- /// Exists because auto mark-read touches one message and the card still
- /// has to stop looking unread. applyTagChange() above cannot serve that:
- /// it rewrites the thread's tags directly, which for a multi-message
- /// thread would claim every reply had been read.
- void applyMessageTagChange(const QString &messageId,
- const QStringList &added,
- const QStringList &removed);
-
private:
/// One thread root and the message rows expanded under it.
///
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index 5347493..eb678eb 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -82,14 +82,6 @@ class WorkerBackedWindow
public:
/// Builds the database, writes the config and loads it. Check isValid()
/// and error() before constructing the window.
- /// Extra `[general]` keys, written verbatim as `key=value` lines. Set
- /// before build(). Used for mark_read_delay_ms, where a test needs the
- /// timer to fire promptly rather than after the two-second default.
- void setGeneralKey(const QString &key, const QString &value)
- {
- m_extraGeneral.insert(key, value);
- }
-
bool build()
{
if (!m_fixture.isValid()) {
@@ -121,10 +113,6 @@ public:
// went unnoticed as broken once already.
out << "[general]\n"
<< "notmuch_config=" << m_fixture.configPath() << "\n";
- for (auto it = m_extraGeneral.cbegin();
- it != m_extraGeneral.cend(); ++it) {
- out << it.key() << "=" << it.value() << "\n";
- }
}
file.close();
@@ -145,7 +133,6 @@ private:
QTemporaryDir m_confDir;
Config m_config;
QString m_error;
- QMap<QString, QString> m_extraGeneral;
};
/// MainWindow is mostly wiring. Cases that need a real database opt into one
@@ -184,9 +171,6 @@ private slots:
void aWorkerBackedWindowReturnsRealThreads();
void selectingAThreadRootShowsItInTheMessagePane();
void anUnexpandedRootRendersOneMessageNotTheConversation();
- void aFirstClickIntoAnUnfocusedListStillRenders();
- void autoMarkReadTouchesOnlyTheMessageOnScreen();
- void autoMarkReadClearsUnreadOnTheCardImmediately();
void autoSyncIsNotArmedWhenDisabledOrWithNothingPending();
void autoSyncSkipsWhileABackgroundSyncIsRunning();
void aSuccessfulSyncRefreshesRatherThanRerunningTheQuery();
@@ -6419,198 +6403,4 @@ void TestMainWindow::anUnexpandedRootRendersOneMessageNotTheConversation()
"the pane rendered a conversation, not a single message");
}
-void TestMainWindow::aFirstClickIntoAnUnfocusedListStillRenders()
-{
- // The user's report: in a fresh session, the first click on a thread does
- // not show the message; clicking again does.
- //
- // setCurrentIndex() cannot reproduce this, which is why the earlier probe
- // passed: it updates the selection model synchronously. A real click does
- // not, and onThreadSelected returns early when the clicked row is not yet
- // selected, a guard that stops QTreeView's focus housekeeping from opening
- // and marking-read mail nobody looked at. This test clicks the viewport.
- WorkerBackedWindow backed;
- QVERIFY(backed.fixture().addMessage(
- QStringLiteral("inbox"), QStringLiteral("root@example.org"),
- QStringLiteral("A conversation"), QStringLiteral("sender@example.org"),
- QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"),
- QStringLiteral("The first message.")));
- QVERIFY(backed.fixture().addMessage(
- QStringLiteral("inbox"), QStringLiteral("reply@example.org"),
- QStringLiteral("Re: A conversation"),
- QStringLiteral("other@example.org"),
- QStringLiteral("Fri, 14 Aug 2026 11:00:00 +0200"),
- QStringLiteral("The reply."), true,
- QStringLiteral("root@example.org")));
- QVERIFY2(backed.build(), qPrintable(backed.error()));
-
- MainWindow window(backed.config());
- window.show();
- QVERIFY(QTest::qWaitForWindowExposed(&window));
-
- QLineEdit *queryEdit =
- window.findChild<QLineEdit *>(QStringLiteral("queryEdit"));
- QVERIFY2(queryEdit, "no query bar");
- auto *view = window.findChild<ThreadListView *>();
- QVERIFY2(view, "no thread list view");
- auto *model = window.findChild<ThreadListModel *>();
- QVERIFY2(model, "no thread list model");
- auto *pane = window.findChild<MessageView *>();
- QVERIFY2(pane, "no message view");
-
- queryEdit->setText(QStringLiteral("tag:inbox"));
- queryEdit->returnPressed();
- QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000);
-
- // Focus stays on the query bar, as it is after typing a query. This is the
- // "fresh session" state the user described: the list has never been
- // clicked and has no current row.
- queryEdit->setFocus();
- QVERIFY2(!view->currentIndex().isValid(),
- "the list already had a current row: not a fresh list");
- QVERIFY(pane->showingPlaceholder());
-
- const QModelIndex root = model->index(0, 0, QModelIndex());
- QVERIFY(root.isValid());
- const QRect rect = view->visualRect(root);
- QVERIFY2(rect.isValid() && rect.height() > 0,
- "the row has no geometry to click");
-
- // ONE click, the first one into a list that never had focus.
- QTest::mouseClick(view->viewport(), Qt::LeftButton, Qt::NoModifier,
- rect.center());
-
- QTRY_VERIFY_WITH_TIMEOUT(!pane->showingPlaceholder(), 15000);
-}
-
-void TestMainWindow::autoMarkReadTouchesOnlyTheMessageOnScreen()
-{
- // Reported by the user, and a real data defect rather than a cosmetic one:
- // selecting an unexpanded thread root marked EVERY message in the thread
- // read, including replies never displayed. maildir.synchronize_flags is
- // on, so that rewrites filenames and reaches the server: mail the user has
- // not seen stops being unread everywhere.
- //
- // It was coherent while a root click rendered the whole conversation. It
- // stopped being coherent when that view was removed and a root began
- // rendering one message, which is the change that made this urgent.
- WorkerBackedWindow backed;
- backed.setGeneralKey(QStringLiteral("mark_read_delay_ms"),
- QStringLiteral("0"));
- QVERIFY(backed.fixture().addMessage(
- QStringLiteral("inbox"), QStringLiteral("root@example.org"),
- QStringLiteral("A conversation"), QStringLiteral("sender@example.org"),
- QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"),
- QStringLiteral("The first message."), /*unread=*/true));
- QVERIFY(backed.fixture().addMessage(
- QStringLiteral("inbox"), QStringLiteral("reply@example.org"),
- QStringLiteral("Re: A conversation"),
- QStringLiteral("other@example.org"),
- QStringLiteral("Fri, 14 Aug 2026 11:00:00 +0200"),
- QStringLiteral("The reply."), /*unread=*/true,
- QStringLiteral("root@example.org")));
- QVERIFY2(backed.build(), qPrintable(backed.error()));
-
- MainWindow window(backed.config());
-
- QLineEdit *queryEdit =
- window.findChild<QLineEdit *>(QStringLiteral("queryEdit"));
- QVERIFY2(queryEdit, "no query bar");
- auto *view = window.findChild<ThreadListView *>();
- QVERIFY2(view, "no thread list view");
- auto *model = window.findChild<ThreadListModel *>();
- QVERIFY2(model, "no thread list model");
-
- queryEdit->setText(QStringLiteral("tag:inbox"));
- queryEdit->returnPressed();
- QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000);
-
- const QModelIndex root = model->index(0, 0, QModelIndex());
- QVERIFY(root.isValid());
- view->setCurrentIndex(root);
-
- // Assert on WHAT THE WINDOW ASKED FOR, which is synchronous and exact.
- //
- // Reading tags back does not work here, and three separate attempts failed
- // vacuously before this. TagsRole returns an empty list for a message row
- // by design (the tag strip is a thread-wide band). MessageOwnTagsRole
- // subtracts the parent thread's tags AND drops any tag drawn as a mark,
- // and `unread` is one, so it can never contain it. And the raw node tags
- // are not refreshed until onTagsApplied() confirms the write, which lands
- // after this test's window: the model reports pre-write tags whichever
- // scope was used. Each of those passed against the unfixed code.
- //
- // The two scopes reach the worker through different entry points, and the
- // window records which one it used. That is the actual difference the fix
- // makes.
- QTRY_VERIFY_WITH_TIMEOUT(
- !window.pendingMessageIdsForTesting().isEmpty()
- || !window.pendingThreadIdsForTesting().isEmpty(),
- 15000);
-
- QVERIFY2(window.pendingThreadIdsForTesting().isEmpty(),
- "the auto mark-read was sent for the whole THREAD: every reply "
- "loses `unread`, including messages never displayed, and "
- "maildir.synchronize_flags carries that to the server");
- QCOMPARE(window.pendingMessageIdsForTesting(),
- QStringList{ QStringLiteral("root@example.org") });
-}
-
-void TestMainWindow::autoMarkReadClearsUnreadOnTheCardImmediately()
-{
- // Reported by the user against the message-scoped mark-read: the write
- // went out, the status bar counted an unsynced edit, and the card stayed
- // bold with `unread` still on it. sendMessageTagChange deliberately makes
- // no optimistic model update, because applyTagChange is keyed by THREAD
- // and repainting a whole row for a one-message edit would be a lie.
- //
- // For an explicit tag edit that trade is fine. For auto mark-read it is
- // not: the visible change IS the feature, and the 2s delay exists to give
- // the user that feedback.
- //
- // One message in the thread, so the thread's own unread state and the
- // message's are the same fact and the card must stop reading as unread.
- WorkerBackedWindow backed;
- backed.setGeneralKey(QStringLiteral("mark_read_delay_ms"),
- QStringLiteral("0"));
- QVERIFY(backed.fixture().addMessage(
- QStringLiteral("inbox"), QStringLiteral("only@example.org"),
- QStringLiteral("A single message"),
- QStringLiteral("sender@example.org"),
- QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"),
- QStringLiteral("The only message."), /*unread=*/true));
- QVERIFY2(backed.build(), qPrintable(backed.error()));
-
- MainWindow window(backed.config());
-
- QLineEdit *queryEdit =
- window.findChild<QLineEdit *>(QStringLiteral("queryEdit"));
- QVERIFY2(queryEdit, "no query bar");
- auto *view = window.findChild<ThreadListView *>();
- QVERIFY2(view, "no thread list view");
- auto *model = window.findChild<ThreadListModel *>();
- QVERIFY2(model, "no thread list model");
-
- queryEdit->setText(QStringLiteral("tag:inbox"));
- queryEdit->returnPressed();
- QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000);
-
- const QModelIndex root = model->index(0, 0, QModelIndex());
- QVERIFY(root.isValid());
-
- // Unread to begin with, or the assertion below proves nothing.
- QVERIFY2(model->data(root, ThreadListModel::TagsRole)
- .toStringList()
- .contains(QStringLiteral("unread")),
- "the thread was not unread to begin with");
-
- view->setCurrentIndex(root);
-
- // The card must stop reading as unread without waiting for a new query.
- QTRY_VERIFY_WITH_TIMEOUT(!model->data(root, ThreadListModel::TagsRole)
- .toStringList()
- .contains(QStringLiteral("unread")),
- 15000);
-}
-
#include "test_mainwindow.moc"