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 ----------- 3 files changed, 10 insertions(+), 135 deletions(-) (limited to 'src') 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. /// -- cgit v1.2.3