From 4a4f82f7709ab6ee5a84ac0f3b191470b6424c36 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 14 Aug 2026 19:16:16 +0200 Subject: feat(pane): always render one message, never the conversation Selecting a thread root used to render the whole conversation, stubs plus the last messages expanded, but only until the thread had been expanded once. After that the identical click rendered a single message. The user reported the inconsistency and asked for the single-message behaviour throughout, and for the conversation view to go. The cause was a timing one, not a race. The root card stands for the thread's first message and onThreadSelected already preferred to load just that, but the model learned the id only when the replies arrived, so a fresh row fell through to a whole-thread render. ThreadSummary now carries firstMessageId from the query itself, so the id is known before any expansion and the fallback is unreachable. It is free: notmuch_thread_get_toplevel_messages reads the index, not the message files, and a walk with it is indistinguishable from one without over a 36,615-thread database. Contrast recipients, which reads every file and stays Sent-only. The Sent view keeps showing what the user sent rather than the thread's opening message, which is often someone else's. There is no matched-messages iterator in libnotmuch, only a count, so that branch walks oldest-first to the first NOTMUCH_MESSAGE_FLAG_MATCH and stops: 0.146s against a 0.143s baseline over 4,515 threads. onThreadLoaded merges into renderMessages, since onMessageLoaded was already delegating to it for the actual painting. It still takes a list because MessageView renders a list; collapsing that is a separate change to a class with its own tests. NotmuchWorker::loadThread is kept and documented as having no UI caller. It is a tested way to read a thread's messages with the match set resolved, used as a helper by the worker's own tests. Co-Authored-By: Claude Opus 5 --- src/mainwindow.cpp | 96 +++++++++++++++++++++++-------------------------- src/mainwindow.h | 3 +- src/notmuchworker.cpp | 50 ++++++++++++++++++++++++++ src/notmuchworker.h | 13 +++++++ src/threadlistmodel.cpp | 21 ++++++++--- src/types.h | 15 ++++++++ 6 files changed, 140 insertions(+), 58 deletions(-) (limited to 'src') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index ee559fe..fb34fe2 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -927,8 +927,8 @@ void MainWindow::registerActions() // undo stack are all left alone. // // m_currentThreadId is cleared with the pane, not merely alongside it. - // A threadLoaded still in flight for that id would otherwise paint the - // thread straight back, which is the queued-reply race documented in + // A messageLoaded still in flight for that row would otherwise paint + // it straight back, which is the queued-reply race documented in // CLAUDE.md. m_currentThreadId.clear(); m_currentMessageId.clear(); @@ -950,8 +950,8 @@ void MainWindow::registerActions() // BLANKING. clearSelection() leaves currentIndex() VALID, and // onSelectionChanged() then takes its "one or fewer rows" branch, finds // a current row whose id differs from m_currentThreadId, and calls - // onThreadSelected for it: the thread is re-adopted and a loadThread - // sent for the row that was just being cleared. + // onThreadSelected for it: the thread is re-adopted and a load sent + // for the row that was just being cleared. // // Clearing the selection FIRST means that runs while m_currentThreadId // still names the displayed thread, so the ids match and nothing is @@ -1451,8 +1451,6 @@ void MainWindow::wireWorker() this, &MainWindow::onThreadTreeLoaded); connect(m_worker, &NotmuchWorker::messageLoaded, this, &MainWindow::onMessageLoaded); - connect(m_worker, &NotmuchWorker::threadLoaded, - this, &MainWindow::onThreadLoaded); connect(m_worker, &NotmuchWorker::errorOccurred, this, &MainWindow::onWorkerError); connect(m_worker, &NotmuchWorker::allTagsReady, @@ -2026,7 +2024,9 @@ void MainWindow::runQuery(FlatResult flat) if (query.isEmpty()) return; - // Kept so loadThread() can work out which messages of a thread matched. + // Kept so an expansion (loadThreadTree) and a refresh can be scoped to the + // query the visible list was built from, rather than to whatever the bar + // holds by the time they run. m_lastQuery = query; // A query the user ran abandons any recovery still in flight. Recovery @@ -2389,57 +2389,55 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, scheduleMarkRead(thread); // The root card IS the thread's first message, so selecting it renders - // that message rather than the whole conversation. Loading the thread here - // made the first message unreachable: the pane showed every message with - // only the last expanded, and no row in the list offered the first one, - // since the reply rows are messages two onward. + // that message. Never the whole conversation: that path is gone (item 66). // - // Known only once the replies have been loaded, which happens when the - // thread is expanded. Until then the thread is the honest answer: it - // contains the first message, where a guess might not. + // The id comes from the query now, so it is known on a fresh row and this + // does not depend on the thread having been expanded. It used to, which + // made a first click render the conversation and every later click render + // one message, from the identical gesture. + // + // In the Sent view a row stands for what the USER sent, which is not + // always the thread's opening message. Handled below rather than here, + // because the id that matters there comes from the query's match set and + // not from the thread's shape. const QString firstId = m_model->data(current, ThreadListModel::MessageIdRole).toString(); - if (!firstId.isEmpty()) { - m_currentMessageId = firstId; - QMetaObject::invokeMethod(m_worker, "loadMessage", - Qt::QueuedConnection, - Q_ARG(QString, firstId), - Q_ARG(quint64, m_generation)); + if (firstId.isEmpty()) { + // No id at all: a thread with no toplevel message is not something + // notmuch produces, but blanking the pane is the honest answer if it + // ever happens, rather than rendering something the row does not name. + m_currentMessageId.clear(); + m_currentMessageThreadId.clear(); + m_messageView->clear(); return; } - m_currentMessageId.clear(); - m_currentMessageThreadId.clear(); - // In the Sent view the pane shows only what matched, which is what the - // user sent. Without this the flat list is right and the pane still opens - // the whole conversation, replies included, under a heading that says - // Sent: the row was never expanded, so the model never learned the - // thread's first message and this is the only path a Sent row can take. - QMetaObject::invokeMethod(m_worker, "loadThread", Qt::QueuedConnection, - Q_ARG(QString, m_currentThreadId), - Q_ARG(QString, m_lastQuery), - Q_ARG(quint64, m_generation), - Q_ARG(bool, m_sentView)); + m_currentMessageId = firstId; + QMetaObject::invokeMethod(m_worker, "loadMessage", Qt::QueuedConnection, + Q_ARG(QString, firstId), + Q_ARG(quint64, m_generation)); } void MainWindow::onMessageLoaded(const QVector &messages, quint64 generation) { - // The same two guards onThreadLoaded carries. A stale generation means the - // query moved on, and a reply landing after the selection grew past one row - // would paint a message back over a deliberately blanked pane. + // A stale generation means the query moved on. A reply landing after the + // selection grew past one row would paint a message back over a pane that + // was deliberately blanked: loadMessage crosses to the worker on a queued + // connection, so the answer arrives after onSelectionChanged() has already + // run. Without this the pane would only look right once a third row made + // the count stale-proof. if (generation != m_generation || messages.isEmpty()) return; if (m_threadView->selectionModel()->selectedRows().size() > 1) return; - // A third guard this one needs and onThreadLoaded does not: a reply that - // lands after the selection moved to a THREAD row would render one message - // where the whole conversation belongs. + // Nothing is currently meant to be on screen: a reply that lands after the + // pane was cleared must not repaint it. if (m_currentMessageId.isEmpty()) return; - onThreadLoaded(messages, generation); + renderMessages(messages); } void MainWindow::onThreadExpanded(const QModelIndex &index) @@ -2478,20 +2476,14 @@ void MainWindow::onThreadTreeLoaded(const QVector &nodes, applyPendingRecovery(); } -void MainWindow::onThreadLoaded(const QVector &messages, - quint64 generation) +void MainWindow::renderMessages(const QVector &messages) { - if (generation != m_generation || messages.isEmpty()) - return; - - // A load started while the selection was still a single row can land after - // it has grown: loadThread crosses to the worker on a queued connection, so - // the reply arrives after onSelectionChanged() has already blanked the - // pane. Without this it would paint a thread back over the blank, and the - // pane would only look right once a third row made the count stale-proof. - if (m_threadView->selectionModel()->selectedRows().size() > 1) - return; - + // Guards live in the caller. This paints what it is given. + // + // Still takes a LIST, though every caller now passes exactly one message: + // MessageView renders a list of items, and collapsing that to a single + // message is a separate change to a class with its own tests. Item 66 + // removed the whole-conversation render; it did not simplify the pane. MimeParser parser; QList items; items.reserve(messages.size()); diff --git a/src/mainwindow.h b/src/mainwindow.h index d844f1e..d861d72 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -333,7 +333,8 @@ private slots: /// Pops up the thread-list context menu, preserving a multi-row selection /// the click lands inside. void showThreadContextMenu(const QPoint &pos); - void onThreadLoaded(const QVector &messages, quint64 generation); + /// Paints `messages` into the message pane. Callers own the guards. + void renderMessages(const QVector &messages); /// Asks the worker for a thread's reply tree when its row is expanded. void onThreadExpanded(const QModelIndex &index); diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 6aae397..a3a2fd5 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -277,6 +277,56 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation, if (withRecipients) summary.recipients = recipientsOf(thread.get()); + // The message the row's card stands for. Raw pointers on purpose: + // messages reached through a thread are owned by the THREAD and freed + // with it (notmuch.h:1637), so an NmMessage wrapper here would destroy + // memory the thread frees again. Everything must be read while + // `thread` is alive, which it is for the rest of this iteration. + // + // Index-only, so it costs nothing measurable: see + // ThreadSummary::firstMessageId. + // + // Two different questions, and the Sent view asks the second one. A + // normal row stands for the thread's OPENING message. A Sent row + // stands for what the USER sent, usually a reply and often not the + // opening message at all, so it takes the first message the query + // MATCHED. withRecipients is exactly the Sent query, which is why it + // selects between them rather than carrying a second flag that could + // disagree with it. + // + // Note notmuch_thread_get_matched_messages returns a COUNT, not an + // iterator; there is no matched-messages list. The match state is a + // per-message flag, so the Sent branch walks in oldest-first order and + // stops at the first match. Measured at 0.146s against a 0.143s + // baseline over 4,515 threads: the walk stops early and reads the + // index, so it is as free as the toplevel call. + if (withRecipients) { + notmuch_messages_t *all = notmuch_thread_get_messages(thread.get()); + for (; all && notmuch_messages_valid(all); + notmuch_messages_move_to_next(all)) { + notmuch_message_t *message = notmuch_messages_get(all); + if (!message) + continue; + notmuch_bool_t matched = FALSE; + notmuch_message_get_flag_st(message, + NOTMUCH_MESSAGE_FLAG_MATCH, + &matched); + if (matched) { + summary.firstMessageId = QString::fromUtf8( + notmuch_message_get_message_id(message)); + break; + } + } + } else if (notmuch_messages_t *top = + notmuch_thread_get_toplevel_messages(thread.get())) { + if (notmuch_messages_valid(top)) { + if (notmuch_message_t *first = notmuch_messages_get(top)) { + summary.firstMessageId = QString::fromUtf8( + notmuch_message_get_message_id(first)); + } + } + } + batch.append(summary); ++total; diff --git a/src/notmuchworker.h b/src/notmuchworker.h index 9736ab8..f07e563 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -71,6 +71,19 @@ public slots: /// Loads the messages of one thread, oldest first. matchQuery is the /// user's current query; messages matching it render expanded, the rest /// as stubs. + /// **No UI caller since item 66, and that is deliberate.** This was how a + /// thread root rendered the whole conversation, stubs plus the last few + /// messages expanded. The user asked for that view to go: selecting any + /// row, root or reply, now renders exactly one message via loadMessage, + /// and `ThreadSummary::firstMessageId` is what makes the root's own + /// message known without expanding the thread first. + /// + /// Kept as a worker capability rather than deleted. It is a tested way to + /// read every message of a thread with the match set resolved, which the + /// worker's own tests use as a helper and a future feature may want. If + /// you are adding a caller, be sure you are not rebuilding the + /// conversation pane that was removed on purpose. + /// /// `matchedOnly` drops the messages that did not match `matchQuery` rather /// than rendering them as stubs. For the Sent view, where the thread is not /// the unit the user is reading: a sent message pulls in the replies it diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 20d6915..3e079ed 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -391,11 +391,22 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const return false; if (role == MessageIdRole) { - // The thread's FIRST message, once known, because the root card is - // that message: selecting it renders one message rather than the whole - // conversation. Empty before the replies are loaded, which is the - // caller's signal to load the thread instead of guessing at a message. - return m_threads.at(index.row()).first.messageId; + // The thread's FIRST message, because the root card IS that message: + // selecting it renders one message, never the whole conversation. + // + // The summary carries this from the query, so it is known before the + // thread has ever been expanded. It used to come only from `first`, + // populated when the replies loaded, which left this empty on a fresh + // row and sent the caller down a whole-thread render instead. The same + // click then behaved differently once the thread had been opened, + // which is what the user reported as item 66. + // + // `first` is still preferred when present: after an expansion it is + // the same message, read from the tree that is now authoritative for + // this thread's shape. + const ThreadNode &node = m_threads.at(index.row()); + return node.first.messageId.isEmpty() ? node.summary.firstMessageId + : node.first.messageId; } if (role == MessageDepthRole) diff --git a/src/types.h b/src/types.h index 97ab43f..f4eaeba 100644 --- a/src/types.h +++ b/src/types.h @@ -33,6 +33,21 @@ struct ThreadSummary int matchedCount = 0; QStringList tags; + /// The thread's FIRST message, which is the one the root card stands for. + /// + /// Carried by the query itself rather than learned when the thread is + /// expanded. That timing was item 66: until a thread had been opened the + /// model did not know this id, so clicking an unexpanded root fell through + /// to rendering the whole conversation, and the identical click behaved + /// differently afterwards. + /// + /// Unlike `recipients` below, this is free. It comes from + /// notmuch_thread_get_toplevel_messages, which reads the INDEX, not the + /// message files: measured indistinguishable from not collecting it at all + /// over a 36,615-thread database. Do not move it behind a flag by analogy + /// with recipients; the two have nothing in common but their position here. + QString firstMessageId; + /// Who the thread's messages were sent TO, summarised for one line. /// /// Empty unless the query asked for it, and that is a performance -- cgit v1.2.3