diff options
| -rw-r--r-- | src/mainwindow.cpp | 179 | ||||
| -rw-r--r-- | src/mainwindow.h | 33 | ||||
| -rw-r--r-- | src/threadlistmodel.cpp | 54 | ||||
| -rw-r--r-- | src/threadlistmodel.h | 27 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 112 |
5 files changed, 405 insertions, 0 deletions
diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index c8aa811..c781d77 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -3847,6 +3847,11 @@ void MainWindow::onSelectionChanged() // emitted BEFORE the selection model is updated, so a handler reading // selectedRows() there sees the PREVIOUS selection and would label the // action for the rows the user just left (CLAUDE.md, verified Qt 6.11). + // + // Before anything else reads the model: the user has moved off whatever + // row they were on, so a row held back by syncViewMembership() leaves now. + flushDeferredEviction(); + refreshUnreadAction(); refreshScopedActionLabels(); refreshTrashActions(); @@ -5365,8 +5370,15 @@ 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. + // + // Flagged as AUTOMATIC for syncViewMembership(): the user did not ask for + // this write, so the row it changes must not be taken out from under them. + // A write they DID ask for evicts at once; the distinction is who + // initiated it, not what it does (item 177). + m_automaticWrite = true; sendMessageTagChange(messageIds, {}, { QStringLiteral("unread") }, tr("Mark read")); + m_automaticWrite = false; } QString MainWindow::currentThreadFirstMessageId() const @@ -5605,6 +5617,40 @@ void MainWindow::sendMessageTagChange(const QStringList &messageIds, m_model->messageById(m_currentMessageId).tags); } + // A row that no longer belongs in the view LEAVES it, rather than sitting + // there repainted until the next query. Beside the repaint above and + // before the sync hold below, since a held edit is applied optimistically + // too and its row is just as wrong to keep. + // + // The threads the touched messages belong to, with no filter on which + // message the card draws: a row is the conversation, so it is the UNION + // that decides, and the union is what removeThreadsWithoutTag() reads. A + // message the model does not hold names no thread, and is what the refresh + // inside the sync is for. + // + // Named only when this write MOVED the union, which is the one case a + // message edit can. applyMessageTagChange() keeps the summary in step for + // a thread of one, where the union IS the message, and deliberately leaves + // a longer thread's summary alone because one message's edit does not + // describe the conversation. Putting a longer thread up for eviction here + // would judge it on a union this write never touched: a stale answer, and + // wrong in both directions. A conversation leaves the view when a + // THREAD-scoped write empties its union, which is the other call site. + QStringList touchedThreads; + bool aRowIsMissing = false; + for (const QString &messageId : messageIds) { + const QString threadId = m_model->threadIdForMessage(messageId); + if (threadId.isEmpty()) { + aRowIsMissing = true; + continue; + } + if (m_model->threadCountFor(threadId) > 1) + continue; + if (!touchedThreads.contains(threadId)) + touchedThreads.append(threadId); + } + syncViewMembership(touchedThreads, aRowIsMissing, add, remove); + // The accounts this touches, resolved through the containing threads: the // account is a property of the thread, and the sync needs the channel // whether one message moved or seven. @@ -6488,6 +6534,130 @@ void MainWindow::sendMove(const QStringList &messageIds, Q_ARG(QString, destFolder)); } +void MainWindow::syncViewMembership(const QStringList &threadIds, + bool aRowIsMissing, + const QStringList &added, + const QStringList &removed) +{ + // Item 177. The optimistic REPAINT has always been universal; the + // optimistic MEMBERSHIP was not, and lived on the move path alone. So + // marking a thread read in the Unread view repainted its row and left it + // in a list defined by `tag:unread` that it no longer matched, until the + // next query or sync took it away. + // + // Membership is the UNION, one rule and no exceptions: a thread belongs + // to a view while any of its messages match it. The judgement itself is + // in removeThreadsWithoutTag(), which reads `summary.tags`; what happens + // here is only deciding WHICH rows to put to it and WHEN. + // + // Guarded on the VIEW's own tag, resolved from the query rather than + // assumed: a plain `tag:<x>` query is the only shape whose membership one + // tag decides. A path query (Trash, Sent, Drafts) is unaffected by a tag + // going away, and an arbitrary query the user typed cannot be reasoned + // about at all, so both are left alone and correct at the next sync. + // Without that guard, marking read in an `id:` view would empty the list. + // + // The exposure this accepts, deliberately and unchanged from the move + // path: revertPendingTagChange() repaints a rejected write but cannot + // REINSERT a row, so a write that fails leaves the row gone until the next + // query. Waiting for confirmation instead would give back exactly the lag + // this removes, and a rejected tag write is the rare case while the lag + // was every keystroke. + const QString viewTag = viewFilterTag(); + if (viewTag.isEmpty()) + return; + + // The INVERSE, which the model cannot do on its own: a row that starts + // matching cannot be inserted optimistically, since the model holds no + // summary for a thread the query never returned. A refresh is what + // expresses it, exactly as the trash view already does after a restore. + // It matters most for UNDO: undoing a mark-read in the Unread view adds + // the tag back, and without this the row stayed gone, which would make an + // undone action invisible in the view it was undone in. + // refreshCurrentQuery() clears nothing, so the selection, the expansions + // and the undo stack all survive. + // + // Only when the row is genuinely ABSENT, which is the whole cost of the + // branch. Adding the view's tag to a row still in the list is the ordinary + // case, and refreshing there re-runs the query on every such keystroke. + if (added.contains(viewTag)) { + if (aRowIsMissing) + refreshCurrentQuery(); + return; + } + + if (!removed.contains(viewTag)) + return; + + // A row is never evicted while the user is sitting on it. The automatic + // mark-read fires two seconds after selection, so evicting on it takes the + // row out from under them, with a context menu possibly open on it, before + // they can mark it spam or important. The row leaves when the selection + // moves, which flushDeferredEviction() does, so the view still empties as + // they work. A write the user ASKED for evicts at once: the distinction is + // who initiated it, not what it does. + QStringList onScreen; + if (m_automaticWrite) { + const QModelIndexList selectedRows = + m_threadView->selectionModel()->selectedRows(); + for (const QModelIndex &row : selectedRows) + onScreen.append(m_model->threadFor(row).threadId); + const QModelIndex current = m_threadView->currentIndex(); + if (current.isValid()) + onScreen.append(m_model->threadFor(current).threadId); + } + + QStringList evictable; + for (const QString &threadId : threadIds) { + if (onScreen.contains(threadId)) { + if (!m_deferredEvictions.contains(threadId)) + m_deferredEvictions.append(threadId); + continue; + } + evictable.append(threadId); + } + + m_model->removeThreadsWithoutTag(evictable, viewTag); +} + +void MainWindow::flushDeferredEviction() +{ + // The rows that stopped matching while the user was on them, taken out now + // that they have moved on. Re-checked against the model rather than + // trusted: the tag may have come back (an undo, a sync), in which case + // removeThreadsWithoutTag() correctly keeps the row. + if (m_deferredEvictions.isEmpty()) + return; + + const QString viewTag = viewFilterTag(); + if (viewTag.isEmpty()) { + m_deferredEvictions.clear(); + return; + } + + // Every thread the user is on, from the SELECTION rather than from + // currentIndex(): a click calls select() before setCurrentIndex(), so at + // the moment selectionChanged arrives the current index still names the + // row being left, and reading it would hold the eviction back for ever. + QStringList onScreen; + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + for (const QModelIndex &row : rows) + onScreen.append(m_model->threadFor(row).threadId); + + QStringList ready; + QStringList stillSelected; + for (const QString &threadId : m_deferredEvictions) { + if (onScreen.contains(threadId)) + stillSelected.append(threadId); + else + ready.append(threadId); + } + m_deferredEvictions = stillSelected; + + m_model->removeThreadsWithoutTag(ready, viewTag); +} + QString MainWindow::viewFilterTag() const { const QString query = m_queryEdit->text().trimmed(); @@ -6658,6 +6828,15 @@ void MainWindow::sendThreadTagChange(const QStringList &threadIds, m_messageView->setTags(m_model->threadFor(current).tags); } + // Same as the message path: a thread whose union stops matching the view + // leaves it now rather than at the next query. A thread-scoped write moves + // the summary, which is what the membership judgement reads, so a thread + // marked read is emptied of `unread` in one step and correctly evicted. + bool aRowIsMissing = false; + for (const QString &threadId : threadIds) + aRowIsMissing = aRowIsMissing || !m_model->hasThread(threadId); + syncViewMembership(threadIds, aRowIsMissing, add, remove); + // A sync holds notmuch's exclusive write lock, and the worker's read-write // open BLOCKS on it rather than failing: measured 9.158s against a 12s // hold, returning SUCCESS. Sending now would freeze the worker thread for diff --git a/src/mainwindow.h b/src/mainwindow.h index fa413fe..0999ef3 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -235,6 +235,16 @@ public: sendMessageTagChange(messageIds, add, remove, description); } + /// Sends a thread-scoped tag change directly. The counterpart seam to + /// sendMessageTagChangeForTesting, for the same reason. + void sendThreadTagChangeForTesting(const QStringList &threadIds, + const QStringList &add, + const QStringList &remove, + const QString &description) + { + sendThreadTagChange(threadIds, add, remove, description); + } + /// The ids the last tag change was sent for, and whether they were thread /// ids or message ids. /// @@ -1191,6 +1201,29 @@ private: /// firing in a view it cannot judge. QString viewFilterTag() const; + /// Keeps the current view's membership in step with a write: drops a row + /// whose UNION has stopped matching, and refreshes when one starts + /// matching, which the model cannot express on its own. No-op unless the + /// view has a filter tag and the write touched it. + void syncViewMembership(const QStringList &threadIds, + bool aRowIsMissing, + const QStringList &added, + const QStringList &removed); + + /// Removes the rows that stopped matching the view while the user was + /// sitting on them. Called when the selection moves. + void flushDeferredEviction(); + + /// Threads that have stopped matching the view but were selected when it + /// happened, so evicting them would have moved the list under the user. + QStringList m_deferredEvictions; + + /// True only while an AUTOMATIC write is being sent. A write the user + /// asked for evicts its row at once; one a timer made must not, or the row + /// leaves under an open context menu two seconds after it was selected. + /// The distinction is who initiated it, not what it does (item 177). + bool m_automaticWrite = false; + void onMessagesMoved(const QMap<QString, QString> &originByMessageId, const QString &destFolder); diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index fd4899b..0559694 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -206,6 +206,34 @@ void ThreadListModel::setTrashView(bool trash) } } +void ThreadListModel::removeThreadsWithoutTag(const QStringList &threadIds, + const QString &tag) +{ + if (tag.isEmpty() || threadIds.isEmpty()) + return; + + // Backwards for the same reason the sweeping form below is: each removal + // renumbers everything after it. + for (int row = m_threads.size() - 1; row >= 0; --row) { + const ThreadNode &node = m_threads.at(row); + if (!threadIds.contains(node.summary.threadId)) + continue; + + // The SUMMARY, which is notmuch's union over the conversation, and + // never `first.tags`. A thread belongs to a view while ANY of its + // messages match it (item 177), so reading the message a 44-message + // card happens to draw must not evict the conversation while two of + // its replies are still unread. `first.tags` is right for what the + // card PAINTS and wrong for whether the row belongs here at all. + if (node.summary.tags.contains(tag)) + continue; + + beginRemoveRows({}, row, row); + m_threads.remove(row); + endRemoveRows(); + } +} + void ThreadListModel::removeThreadsWithoutTag(const QString &tag) { if (tag.isEmpty() || m_threads.isEmpty()) @@ -995,6 +1023,15 @@ MessageNode ThreadListModel::messageAt(const QModelIndex &index) const QString ThreadListModel::threadIdForMessage(const QString &messageId) const { for (const ThreadNode &node : m_threads) { + // The ROOT first. A thread's first message is not among its children + // (item 109: setThreadMessages drops depth 0 because the root row + // stands for it), so a search over children alone answers "no thread" + // for every thread row's own message, which is the id an ordinary tag + // action resolves to since item 108. + if (node.first.messageId == messageId + || node.summary.firstMessageId == messageId) { + return node.summary.threadId; + } for (const MessageNode &child : node.children) { if (child.messageId == messageId) return node.summary.threadId; @@ -1003,6 +1040,23 @@ QString ThreadListModel::threadIdForMessage(const QString &messageId) const return {}; } +bool ThreadListModel::hasThread(const QString &threadId) const +{ + return std::any_of(m_threads.cbegin(), m_threads.cend(), + [&threadId](const ThreadNode &node) { + return node.summary.threadId == threadId; + }); +} + +int ThreadListModel::threadCountFor(const QString &threadId) const +{ + for (const ThreadNode &node : m_threads) { + if (node.summary.threadId == threadId) + return node.summary.totalCount; + } + return 0; +} + MessageNode ThreadListModel::messageById(const QString &messageId) const { if (messageId.isEmpty()) diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index a528c7e..93474ac 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -236,6 +236,21 @@ public: /// CURRENT VIEW requires, so a caller passes what the query filters on and /// nothing else. void removeThreadsWithoutTag(const QString &tag); + + /// Drops only the NAMED threads, and only those whose UNION no longer + /// carries \p tag. + /// + /// Two differences from the sweeping form, both deliberate. It judges the + /// named rows alone, because the sweeping form is right after a move (the + /// query itself is what changed) and wrong after a tag write: a list can + /// legitimately hold rows that never matched, and one write must not evict + /// rows it did not touch. And it judges `summary.tags`, which is notmuch's + /// union over the conversation, never the displayed message's own tags: a + /// thread belongs to a view while ANY of its messages match it (item 177), + /// so reading one message of a 44-message thread leaves the conversation + /// in the Unread view and reading the last one takes it out. + void removeThreadsWithoutTag(const QStringList &threadIds, + const QString &tag); bool flatMode() const { return m_flatMode; } QModelIndex index(int row, int column, @@ -326,6 +341,18 @@ public: /// message the user could select is always findable here. QString threadIdForMessage(const QString &messageId) const; + /// Whether a row for this thread is still in the list. + bool hasThread(const QString &threadId) const; + + /// How many messages the thread holds, from its summary, or 0 when the + /// list has no row for it. + /// + /// The question a message-scoped write asks before judging membership: a + /// thread of one has a union that IS its message, so the write moved it + /// and the row can be judged, while a longer thread's union is untouched + /// and judging it would be judging a stale answer. + int threadCountFor(const QString &threadId) const; + /// A loaded message row's node, found by id rather than by position. /// /// For callers that know WHICH message they mean and must not depend on it diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 26ec485..2d15fe8 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -554,6 +554,10 @@ private slots: void replyToAConversationAnswersItsNewestMessage(); void replyIsUntouchedOnAMessageRow(); + // Item 177, task 6: membership is the union over the conversation. + void aConversationStaysWhileAnyMessageMatches(); + void aConversationLeavesWhenItsUnionEmpties(); + private: /// Owns the throwaway lock table init() points every test at. A pointer /// rather than a value because it is rebuilt per test, and QTemporaryDir @@ -5561,6 +5565,17 @@ void TestMainWindow::aMixedThreadIsMarkedReadAndTheSecondPressIsTheWayBack() auto *view = window.findChild<QTreeView *>(); QVERIFY(view); + // A query with no filter tag, so viewFilterTag() answers empty and the + // membership sync (item 177, task 6) stays out of this. Under the default + // startup query the first press correctly EVICTS the row it just marked + // read, and the assertions below would then be reading a list that no + // longer holds the thread. What is under test here is the toggle's + // direction, which membership neither helps nor hinders. + auto *queryEdit = + window.findChild<QLineEdit *>(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("thread:T1")); + // MIXED: the union carries `unread` because some message is unread, while // others are not. A thread whose messages are all in one state answers // identically whichever way the direction is computed, so a uniform @@ -15293,4 +15308,101 @@ void TestMainWindow::replyIsUntouchedOnAMessageRow() .arg(replyAction->text()))); } +void TestMainWindow::aConversationStaysWhileAnyMessageMatches() +{ + // The reported case: a 44-message thread with two replies still unread. + // Reading one message must not evict the conversation. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + auto *queryEdit = + window.findChild<QLineEdit *>(QStringLiteral("queryEdit")); + QVERIFY(model && queryEdit); + + queryEdit->setText( + config.resolvedQuery(Config::builtinFilter(QStringLiteral("unread")), + QString())); + + ThreadSummary other = makeThread(QStringLiteral("t1"), + QStringList{ QStringLiteral("unread") }); + other.totalCount = 1; + ThreadSummary big = makeThread(QStringLiteral("t2"), + QStringList{ QStringLiteral("unread") }); + big.totalCount = 44; + model->appendBatch({ other, big }); + + // One message of the conversation is read, and specifically the one its + // ROW displays: naming any other id makes the model answer "no thread" and + // the test measures nothing, passing against an eviction that judges on + // the card's own tags. The union still says unread, so the row stays. + window.sendMessageTagChangeForTesting({ big.firstMessageId }, {}, + { QStringLiteral("unread") }, + QStringLiteral("Mark read")); + + QCOMPARE(model->rowCount(QModelIndex()), 2); + QCOMPARE(model->threadAt(1).threadId, QStringLiteral("t2")); + + // The same question put to the judgement ITSELF, which is where the union + // rule lives. The send path above reaches it only for a thread of one, so + // asserting through that path alone would let an eviction judging the + // card's own tags pass: the long thread is filtered out before the model + // is ever asked. Here the row is named directly, and the only thing + // keeping it is that its union still carries `unread` while the message + // its card draws no longer does. + QVERIFY2(!model->messageById(big.firstMessageId) + .tags.contains(QStringLiteral("unread")), + "the fixture's card message is still unread, so this assertion " + "cannot tell the union apart from the card's own tags"); + model->removeThreadsWithoutTag(QStringList{ QStringLiteral("t2") }, + QStringLiteral("unread")); + QCOMPARE(model->rowCount(QModelIndex()), 2); + + // And the counterpart, so the test is about the union and not about + // nothing ever being evicted: a one-message row's union IS its message, so + // reading it takes the row out at once. + window.sendMessageTagChangeForTesting({ other.firstMessageId }, {}, + { QStringLiteral("unread") }, + QStringLiteral("Mark read")); + + QCOMPARE(model->rowCount(QModelIndex()), 1); + QCOMPARE(model->threadAt(0).threadId, QStringLiteral("t2")); +} + +void TestMainWindow::aConversationLeavesWhenItsUnionEmpties() +{ + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + auto *view = window.findChild<QTreeView *>(); + auto *queryEdit = + window.findChild<QLineEdit *>(QStringLiteral("queryEdit")); + QVERIFY(model && view && queryEdit); + + queryEdit->setText( + config.resolvedQuery(Config::builtinFilter(QStringLiteral("unread")), + QString())); + + ThreadSummary keep = makeThread(QStringLiteral("t1"), + QStringList{ QStringLiteral("unread") }); + keep.totalCount = 1; + ThreadSummary go = makeThread(QStringLiteral("t2"), + QStringList{ QStringLiteral("unread") }); + go.totalCount = 4; + model->appendBatch({ keep, go }); + + // Selected elsewhere, so the never-evict-the-current-row rule is not what + // is being measured here. + selectThreadRow(view, 0); + QApplication::processEvents(); + + window.sendThreadTagChangeForTesting({ QStringLiteral("t2") }, {}, + { QStringLiteral("unread") }, + QStringLiteral("Mark thread read")); + + QCOMPARE(model->rowCount(QModelIndex()), 1); + QCOMPARE(model->threadAt(0).threadId, QStringLiteral("t1")); +} + #include "test_mainwindow.moc" |