From 4a4849421ddac044212cd0e17f9aee8ff2606292 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 14 Aug 2026 20:06:08 +0200 Subject: Revert the message-scoped auto mark-read Reverts bde7409 and 66f1159. The user hit the worst possible symptom: clicking one message marked a DIFFERENT, unrelated message read. The cause is in markCurrentThreadRead, which reads m_model->threadAt(current.row()). CLAUDE.md records this exact trap: a tree numbers rows PER PARENT, so a reply's row() indexes its siblings and threadAt() on it answers about an unrelated thread near the top of the list. The guards then compared the right ids against the wrong thread and let a write through for whatever message the timer's state named. That fault predates these commits, but they made it reachable and harmful: while the write was thread-scoped the mismatch was mostly masked, and scoping it to a single message turned it into "a random message is now read". Reverting rather than fixing forward. Marking the wrong mail read syncs out to the server and cannot be undone from here, so the safe state is the previous behaviour, which is too broad but predictable. The item 66 work in 4a4f82f stands: a thread root still renders one message. Co-Authored-By: Claude Opus 5 --- src/mainwindow.cpp | 47 +++-------- src/threadlistmodel.cpp | 81 ------------------ src/threadlistmodel.h | 17 ---- tests/test_mainwindow.cpp | 210 ---------------------------------------------- 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 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(QStringLiteral("queryEdit")); - QVERIFY2(queryEdit, "no query bar"); - auto *view = window.findChild(); - QVERIFY2(view, "no thread list view"); - auto *model = window.findChild(); - QVERIFY2(model, "no thread list model"); - auto *pane = window.findChild(); - 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(QStringLiteral("queryEdit")); - QVERIFY2(queryEdit, "no query bar"); - auto *view = window.findChild(); - QVERIFY2(view, "no thread list view"); - auto *model = window.findChild(); - 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(QStringLiteral("queryEdit")); - QVERIFY2(queryEdit, "no query bar"); - auto *view = window.findChild(); - QVERIFY2(view, "no thread list view"); - auto *model = window.findChild(); - 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" -- cgit v1.2.3