From 2dd35b2327975a16e81018f8cd704c32a1e68cf5 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 10:20:14 +0200 Subject: refactor(model): convert ThreadListModel to QAbstractItemModel A table cannot indent or expand, so message rows need a tree. This task changes only the base class and the index plumbing: no children are produced yet, so the 30 pre-existing tests in test_threadlistmodel are the regression net proving a thread row still behaves exactly as it did, and QAbstractItemModelTester checks the index/parent round trip a hand-written assertion would miss. Two things the table version could leave wrong and a tree cannot. columnCount returned 0 for a valid parent, which would give message rows no columns and render them blank. And rowCount now answers only for column 0, since a tree takes one set of children per row and offering them under every column draws an expander in each. The model stays two levels deep even though replies carry a reply depth of their own. The visual nesting past the first level comes from that depth, not from further parent-child structure, so no index calculation has to recurse. --- src/threadlistmodel.cpp | 79 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 69 insertions(+), 10 deletions(-) (limited to 'src/threadlistmodel.cpp') diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 4bafca8..3194859 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -99,30 +99,87 @@ QColor ThreadListModel::readColour() } ThreadListModel::ThreadListModel(QObject *parent) - : QAbstractTableModel(parent) + : QAbstractItemModel(parent) { } +QModelIndex ThreadListModel::index(int row, int column, + const QModelIndex &parent) const +{ + if (!hasIndex(row, column, parent)) + return {}; + + // A root row. -1 as the internal id marks it, so parent() can tell the two + // kinds apart without storing a node pointer per index. + if (!parent.isValid()) + return createIndex(row, column, static_cast(-1)); + + // A child row: the internal id is its parent's row, which is all parent() + // needs to rebuild the thread index. + return createIndex(row, column, static_cast(parent.row())); +} + +QModelIndex ThreadListModel::parent(const QModelIndex &child) const +{ + if (!child.isValid()) + return {}; + + const quintptr id = child.internalId(); + if (id == static_cast(-1)) + return {}; + + // Column 0, always. Qt requires a parent index in the first column, and + // returning the child's own column instead breaks selection and the + // expander, silently and only for the other columns. + return createIndex(static_cast(id), 0, static_cast(-1)); +} + int ThreadListModel::rowCount(const QModelIndex &parent) const { - return parent.isValid() ? 0 : m_threads.size(); + if (!parent.isValid()) + return m_threads.size(); + + // Only a thread row has children, and only in its first column. A tree + // takes one set of children per row; offering them under every column makes + // the view draw an expander in each one. + if (parent.parent().isValid() || parent.column() != 0) + return 0; + + if (parent.row() < 0 || parent.row() >= m_threads.size()) + return 0; + + return m_threads.at(parent.row()).children.size(); } int ThreadListModel::columnCount(const QModelIndex &parent) const { - return parent.isValid() ? 0 : ColumnCount; + // Every level has the same columns. Returning 0 for a valid parent, as the + // table version did, would give message rows no columns at all and render + // them blank. + Q_UNUSED(parent); + return ColumnCount; } QVariant ThreadListModel::data(const QModelIndex &index, int role) const { // A stale index from a view that has not caught up with a clear() can carry // any row or column, so both bounds are checked rather than trusted. - if (!index.isValid() || index.row() < 0 || index.row() >= m_threads.size() + if (!index.isValid() || index.row() < 0 || index.column() < 0 || index.column() >= ColumnCount) { return {}; } - const ThreadSummary &thread = m_threads.at(index.row()); + // Message rows are handled in Task 4; until then only thread rows exist and + // a child index cannot be produced. The bound is checked against the thread + // list only after establishing this IS a thread row, since a child row's + // number indexes its siblings, not m_threads. + if (index.parent().isValid()) + return {}; + + if (index.row() >= m_threads.size()) + return {}; + + const ThreadSummary &thread = m_threads.at(index.row()).summary; if (role == ThreadIdRole) return thread.threadId; @@ -310,7 +367,8 @@ void ThreadListModel::appendBatch(const QVector &batch) const int first = m_threads.size(); beginInsertRows({}, first, first + batch.size() - 1); - m_threads.append(batch); + for (const ThreadSummary &summary : batch) + m_threads.append(ThreadNode{ summary, {}, false }); endInsertRows(); } @@ -325,13 +383,14 @@ ThreadSummary ThreadListModel::threadAt(int row) const { if (row < 0 || row >= m_threads.size()) return {}; - return m_threads.at(row); + return m_threads.at(row).summary; } QStringList ThreadListModel::accountKeysForThread(const QString &threadId) const { QStringList keys; - for (const ThreadSummary &thread : m_threads) { + for (const ThreadNode &node : m_threads) { + const ThreadSummary &thread = node.summary; if (thread.threadId != threadId) continue; for (const QString &tag : thread.tags) { @@ -351,10 +410,10 @@ void ThreadListModel::applyTagChange(const QString &threadId, const QStringList &removed) { for (int row = 0; row < m_threads.size(); ++row) { - if (m_threads.at(row).threadId != threadId) + if (m_threads.at(row).summary.threadId != threadId) continue; - QStringList &tags = m_threads[row].tags; + QStringList &tags = m_threads[row].summary.tags; for (const QString &tag : removed) tags.removeAll(tag); for (const QString &tag : added) { -- cgit v1.2.3 From 879a117cba62df57dfcb0c0dfd4383308fa07f13 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 10:22:33 +0200 Subject: feat(model): expose a thread's replies as child rows setThreadMessages drops the depth-0 message: it is the thread's first message and the root row already stands for it. Keeping it would show a thread of seven as one root and seven children, contradicting the reply count the row advertises. Calling again replaces rather than appends, so a thread reloaded after a sync does not list its replies twice. A message row reports its own sender and subject, not the thread's. That is the mistake worth guarding: the thread's author summary usually contains the first sender too, so reading it renders something plausible for the root's own reply and wrong for every other one. Mutation-checked, and the wrong version returns 'Alice' where 'Bob' belongs. Child rows carry no tag pills. The strip is a row-wide band of the thread's tags; one under each reply would stripe the list and repeat identical tags down the expansion. --- src/threadlistmodel.cpp | 129 +++++++++++++++++++++++++++++++++++++++-- src/threadlistmodel.h | 29 +++++++++ tests/test_threadlistmodel.cpp | 115 ++++++++++++++++++++++++++++++++++++ 3 files changed, 268 insertions(+), 5 deletions(-) (limited to 'src/threadlistmodel.cpp') diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 3194859..6fc2177 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -169,12 +169,60 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const return {}; } - // Message rows are handled in Task 4; until then only thread rows exist and - // a child index cannot be produced. The bound is checked against the thread - // list only after establishing this IS a thread row, since a child row's + // A message row. Handled before the bounds check below, since a child row's // number indexes its siblings, not m_threads. - if (index.parent().isValid()) - return {}; + if (isMessageRow(index)) { + const MessageNode node = messageAt(index); + if (node.messageId.isEmpty()) + return {}; + + switch (role) { + case IsMessageRole: + return true; + case MessageIdRole: + return node.messageId; + case MessageDepthRole: + return node.depth; + case ThreadIdRole: + // A message row still belongs to a thread, and a caller that only + // needs the containing thread must not have to walk up itself. + return node.threadId; + case TagsRole: + case PillTagsRole: + // No strip under a child row: the strip is a ROW-wide band carrying + // the thread's tags, and one under every reply would stripe the + // list and repeat the same tags down the whole expansion. + return QStringList(); + case PillColoursRole: + return QVariantList(); + case AccountLabelRole: + return QString(); + case Qt::DisplayRole: + switch (index.column()) { + case AuthorsColumn: + // The REPLY's sender, not the thread's author summary. Reading + // the thread's fields here would look almost right, since the + // first sender usually appears in both. + return node.from; + case SubjectColumn: + return node.subject; + case DateColumn: + return node.date.toString(QStringLiteral("yyyy-MM-dd hh:mm")); + case AttachmentColumn: + return node.hasAttachment() ? attachmentGlyph() : QString(); + case FlagColumn: + return node.isFlagged() ? flagGlyph() : QString(); + default: + return {}; + } + case Qt::ForegroundRole: + // Same rule as a thread row: read recedes, unread stays at the + // palette's own colour. + return node.isUnread() ? QVariant() : QVariant(readColour()); + default: + return {}; + } + } if (index.row() >= m_threads.size()) return {}; @@ -184,6 +232,19 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const if (role == ThreadIdRole) return thread.threadId; + // Answered rather than left to fall through as an invalid QVariant. An + // invalid one converts to false and an empty string anyway, so the + // behaviour is the same, but a role the model never mentions is a latent + // bug the next reader has to prove is harmless. + if (role == IsMessageRole) + return false; + + if (role == MessageIdRole) + return QString(); + + if (role == MessageDepthRole) + return 0; + if (role == TagsRole) return thread.tags; @@ -379,6 +440,64 @@ void ThreadListModel::clear() endResetModel(); } +void ThreadListModel::setThreadMessages(const QString &threadId, + const QVector &nodes) +{ + for (int row = 0; row < m_threads.size(); ++row) { + if (m_threads.at(row).summary.threadId != threadId) + continue; + + const QModelIndex parent = index(row, 0, QModelIndex()); + + // Replace, not append. A thread reloaded after a sync would otherwise + // list every reply twice. + if (!m_threads.at(row).children.isEmpty()) { + beginRemoveRows(parent, 0, m_threads.at(row).children.size() - 1); + m_threads[row].children.clear(); + endRemoveRows(); + } + + QVector children; + children.reserve(nodes.size()); + for (const MessageNode &node : nodes) { + if (node.depth > 0) + children.append(node); + } + + if (!children.isEmpty()) { + beginInsertRows(parent, 0, children.size() - 1); + m_threads[row].children = children; + endInsertRows(); + } + + // Set even when there are no replies: that is the difference between a + // single-message thread and one whose replies were never fetched. + m_threads[row].loaded = true; + return; + } +} + +bool ThreadListModel::isMessageRow(const QModelIndex &index) const +{ + return index.isValid() && index.parent().isValid(); +} + +MessageNode ThreadListModel::messageAt(const QModelIndex &index) const +{ + if (!isMessageRow(index)) + return {}; + + const int threadRow = index.parent().row(); + if (threadRow < 0 || threadRow >= m_threads.size()) + return {}; + + const QVector &children = m_threads.at(threadRow).children; + if (index.row() < 0 || index.row() >= children.size()) + return {}; + + return children.at(index.row()); +} + ThreadSummary ThreadListModel::threadAt(int row) const { if (row < 0 || row >= m_threads.size()) diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index 1aa0271..a7ce5d3 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -84,6 +84,18 @@ public: /// model because it owns the TagColors instance; a delegate reading /// config itself would be a second source of truth. PillColoursRole, + + /// True when the row is a MESSAGE row rather than a thread root. + /// Drives both the action scope and whether the view paints a tag + /// strip under the row. + IsMessageRole, + + /// The message id behind a message row. Empty on a thread root. + MessageIdRole, + + /// The message's reply depth, for the view's indentation. 1 for a + /// direct reply, since depth 0 is the root row itself. + MessageDepthRole, }; /// Row fill for a thread tagged `deleted`, and for one tagged `spam`. @@ -131,6 +143,23 @@ public: ThreadSummary threadAt(int row) const; + /// Fills in a thread's message rows once the worker has walked its tree. + /// + /// The depth-0 message is dropped: it is the thread's first message and the + /// ROOT row already stands for it. Keeping it would show a thread of seven + /// as one root and seven children, contradicting the reply count the row + /// advertises. Calling again replaces the rows rather than appending, so a + /// thread reloaded after a sync does not list its replies twice. + void setThreadMessages(const QString &threadId, + const QVector &nodes); + + /// True when the index is a message row rather than a thread root. + bool isMessageRow(const QModelIndex &index) const; + + /// The message row's node, or a default-constructed one for any index that + /// is not a message row. + MessageNode messageAt(const QModelIndex &index) const; + /// The account keys behind a thread's account tags, for item 49's /// per-account sync. /// diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index 9684637..b4be527 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -29,6 +29,9 @@ class TestThreadListModel : public QObject private slots: void messageNodeHoldsDisplayFacts(); void rootRowsSurviveTheTreeConversion(); + void repliesBecomeChildRowsUnderTheirThread(); + void messageRowsShowTheirOwnSenderAndSubject(); + void reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem(); void startsEmpty(); void accountKeysComeFromTheAccountTags(); void accountKeysCoverAThreadSpanningTwoAccounts(); @@ -78,6 +81,118 @@ static ThreadSummary makeThread(const QString &id, const QString &subject) return t; } +static MessageNode makeNode(const QString &id, int depth, + const QString &from = QStringLiteral("Alice"), + const QString &subject = QStringLiteral("Re: Hi")) +{ + MessageNode n; + n.messageId = id; + n.threadId = QStringLiteral("t1"); + n.from = from; + n.subject = subject; + n.date = QDateTime::fromSecsSinceEpoch(1750000000); + n.depth = depth; + return n; +} + +void TestThreadListModel::repliesBecomeChildRowsUnderTheirThread() +{ + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")) }); + + // Depth 0 is the thread's FIRST message and belongs on the root row, not in + // the children: the user's model is "N replies", so a thread of three shows + // one root and two children. + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1), + makeNode(QStringLiteral("m2@example.org"), 2) }); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + QCOMPARE(model.rowCount(root), 2); + + const QModelIndex child = + model.index(0, ThreadListModel::SubjectColumn, root); + QVERIFY(child.isValid()); + QCOMPARE(model.parent(child), model.index(0, 0, QModelIndex())); + + QVERIFY(model.data(child, ThreadListModel::IsMessageRole).toBool()); + QCOMPARE(model.data(child, ThreadListModel::MessageIdRole).toString(), + QStringLiteral("m1@example.org")); + + // A message row still belongs to a thread, so a caller that only needs the + // containing thread does not have to walk up itself. + QCOMPARE(model.data(child, ThreadListModel::ThreadIdRole).toString(), + QStringLiteral("t1")); + + // A thread root is not a message row and carries no message id. + QVERIFY(!model.data(root, ThreadListModel::IsMessageRole).toBool()); + QVERIFY(model.data(root, ThreadListModel::MessageIdRole) + .toString().isEmpty()); + + QAbstractItemModelTester tester( + &model, QAbstractItemModelTester::FailureReportingMode::Warning); + Q_UNUSED(tester); +} + +void TestThreadListModel::messageRowsShowTheirOwnSenderAndSubject() +{ + // A reply's row shows the REPLY's sender, not the thread's author summary. + // Reading the thread's fields for a child row is the obvious mistake and + // would look almost right, since the first sender is usually in both. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")) }); + model.setThreadMessages( + QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1, + QStringLiteral("Bob "), + QStringLiteral("Re: A subject")) }); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + const QModelIndex authors = + model.index(0, ThreadListModel::AuthorsColumn, root); + const QModelIndex subject = + model.index(0, ThreadListModel::SubjectColumn, root); + + QCOMPARE(model.data(authors, Qt::DisplayRole).toString(), + QStringLiteral("Bob ")); + QCOMPARE(model.data(subject, Qt::DisplayRole).toString(), + QStringLiteral("Re: A subject")); + + // No tag strip under a child row. The strip is a row-wide band carrying the + // THREAD's tags; one under every reply would stripe the list and repeat the + // same tags down the whole expansion. + QVERIFY(model.data(subject, ThreadListModel::PillTagsRole) + .toStringList().isEmpty()); +} + +void TestThreadListModel::reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem() +{ + // A thread reloaded after a sync must not end up listing its replies twice. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")) }); + + const QVector nodes{ + makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) + }; + + model.setThreadMessages(QStringLiteral("t1"), nodes); + const QModelIndex root = model.index(0, 0, QModelIndex()); + QCOMPARE(model.rowCount(root), 1); + + model.setThreadMessages(QStringLiteral("t1"), nodes); + QCOMPARE(model.rowCount(root), 1); + + QAbstractItemModelTester tester( + &model, QAbstractItemModelTester::FailureReportingMode::Warning); + Q_UNUSED(tester); +} + void TestThreadListModel::accountKeysComeFromTheAccountTags() { // Item 49 reads this to decide which mbsync channels a sync needs. Only -- cgit v1.2.3 From c80c060a593fb802f7149223d0d2f8495bc1f443 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 10:24:16 +0200 Subject: feat(model): resolve action scope from the selected row kind ActionScope is what an action is about to touch, resolved from the selection in one place so no call site reinvents the mapping. A thread root contributes the whole thread, a message row contributes one message, and messageCount is what the status bar reports. The count comes from totalCount, not from the loaded children. A thread that was never expanded still has all of its messages, and counting only the rows that happen to be on screen would understate what the action does: mutation-checked, and the wrong version reports 1 message where 7 are about to be tagged. A mixed selection is honoured as given rather than escalated to thread scope or narrowed to message scope. Silently widening it would defeat the reason the scope is shown at all. --- src/threadlistmodel.cpp | 36 ++++++++++++++++++ src/threadlistmodel.h | 8 ++++ src/types.h | 26 +++++++++++++ tests/test_threadlistmodel.cpp | 83 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 153 insertions(+) (limited to 'src/threadlistmodel.cpp') diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 6fc2177..19e1153 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -498,6 +498,42 @@ MessageNode ThreadListModel::messageAt(const QModelIndex &index) const return children.at(index.row()); } +ActionScope ThreadListModel::scopeFor(const QModelIndexList &selection) const +{ + ActionScope scope; + + for (const QModelIndex &index : selection) { + if (isMessageRow(index)) { + const MessageNode node = messageAt(index); + if (node.messageId.isEmpty() + || scope.messageIds.contains(node.messageId)) + continue; + scope.messageIds.append(node.messageId); + scope.messageCount += 1; + continue; + } + + if (index.row() < 0 || index.row() >= m_threads.size()) + continue; + + const ThreadSummary &summary = m_threads.at(index.row()).summary; + if (scope.threadIds.contains(summary.threadId)) + continue; + + scope.threadIds.append(summary.threadId); + + // totalCount, not the loaded children: a thread that was never expanded + // still has all of its messages, and counting only what happens to be + // on screen would understate what the action does. Floored at 1, since + // a summary with no count still stands for at least the message that + // produced it. + scope.messageCount += qMax(1, summary.totalCount); + scope.wholeThread = true; + } + + return scope; +} + ThreadSummary ThreadListModel::threadAt(int row) const { if (row < 0 || row >= m_threads.size()) diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index a7ce5d3..bc9fcbf 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -160,6 +160,14 @@ public: /// is not a message row. MessageNode messageAt(const QModelIndex &index) const; + /// Resolves a selection into what an action should touch. + /// + /// Mixed selections are honoured as given: a thread root and an unrelated + /// reply act on that whole thread and that one message. Nothing is + /// escalated or narrowed silently, which is the point of the scope being + /// visible in the first place. + ActionScope scopeFor(const QModelIndexList &selection) const; + /// The account keys behind a thread's account tags, for item 49's /// per-account sync. /// diff --git a/src/types.h b/src/types.h index ef822b9..d04670e 100644 --- a/src/types.h +++ b/src/types.h @@ -94,6 +94,32 @@ struct MessageNode } }; +/// What an action is about to touch, resolved from the selection. +/// +/// Exists because the thread list holds two kinds of row since item 20, so a +/// keypress alone no longer says whether it hit one message or seven. Actions +/// take one of these rather than a bare list of thread ids, and the status bar +/// reports it: this project's answer to that ambiguity is to make the scope +/// visible, not to add a confirmation dialog. See CLAUDE.md on why. +struct ActionScope +{ + QStringList threadIds; ///< Whole threads to act on. + QStringList messageIds; ///< Individual messages to act on. + + /// Messages the action will touch in total, for the status bar. A whole + /// thread contributes all of its messages, a message row contributes one. + int messageCount = 0; + + /// True when any whole thread is in scope, which drives the + /// "(whole thread)" suffix in the status bar. + bool wholeThread = false; + + bool isEmpty() const + { + return threadIds.isEmpty() && messageIds.isEmpty(); + } +}; + /// One tag mutation, kept so it can be inverted for undo. struct TagChange { diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index b4be527..74b8dd1 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -32,6 +32,9 @@ private slots: void repliesBecomeChildRowsUnderTheirThread(); void messageRowsShowTheirOwnSenderAndSubject(); void reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem(); + void scopeFollowsTheSelectedRowKind(); + void scopeCountsEveryMessageOfAnUnexpandedThread(); + void scopeHonoursAMixedSelectionWithoutEscalating(); void startsEmpty(); void accountKeysComeFromTheAccountTags(); void accountKeysCoverAThreadSpanningTwoAccounts(); @@ -193,6 +196,86 @@ void TestThreadListModel::reloadingAThreadReplacesItsRepliesRatherThanRepeatingT Q_UNUSED(tester); } +void TestThreadListModel::scopeFollowsTheSelectedRowKind() +{ + ThreadListModel model; + ThreadSummary t = makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")); + t.totalCount = 3; + model.appendBatch({ t }); + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) }); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + const QModelIndex child = model.index(0, 0, root); + + // A thread root acts on the whole thread, and reports every message it + // stands for so the status bar can say so. + const ActionScope threadScope = model.scopeFor({ root }); + QCOMPARE(threadScope.threadIds, QStringList{ QStringLiteral("t1") }); + QVERIFY(threadScope.messageIds.isEmpty()); + QCOMPARE(threadScope.messageCount, 3); + QVERIFY(threadScope.wholeThread); + + // A message row acts on that message alone. + const ActionScope messageScope = model.scopeFor({ child }); + QVERIFY(messageScope.threadIds.isEmpty()); + QCOMPARE(messageScope.messageIds, + QStringList{ QStringLiteral("m1@example.org") }); + QCOMPARE(messageScope.messageCount, 1); + QVERIFY(!messageScope.wholeThread); +} + +void TestThreadListModel::scopeCountsEveryMessageOfAnUnexpandedThread() +{ + // totalCount, not the loaded children. A thread that was never expanded + // still has all of its messages, and counting only what happens to be on + // screen would understate what the action is about to do. + ThreadListModel model; + ThreadSummary t = makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")); + t.totalCount = 7; + model.appendBatch({ t }); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + QCOMPARE(model.rowCount(root), 0); // guard: nothing expanded + + const ActionScope scope = model.scopeFor({ root }); + QCOMPARE(scope.messageCount, 7); +} + +void TestThreadListModel::scopeHonoursAMixedSelectionWithoutEscalating() +{ + // Selecting a thread root and an unrelated reply acts on that whole thread + // AND that one message. Nothing is escalated to thread scope or narrowed to + // message scope silently, which is the point of the scope being visible. + ThreadListModel model; + ThreadSummary t1 = makeThread(QStringLiteral("t1"), QStringLiteral("One")); + t1.totalCount = 2; + ThreadSummary t2 = makeThread(QStringLiteral("t2"), QStringLiteral("Two")); + t2.totalCount = 5; + model.appendBatch({ t1, t2 }); + + MessageNode reply = makeNode(QStringLiteral("m1@example.org"), 1); + reply.threadId = QStringLiteral("t2"); + model.setThreadMessages(QStringLiteral("t2"), + { makeNode(QStringLiteral("m0@example.org"), 0), + reply }); + + const QModelIndex firstRoot = model.index(0, 0, QModelIndex()); + const QModelIndex secondRoot = model.index(1, 0, QModelIndex()); + const QModelIndex reply1 = model.index(0, 0, secondRoot); + + const ActionScope scope = model.scopeFor({ firstRoot, reply1 }); + QCOMPARE(scope.threadIds, QStringList{ QStringLiteral("t1") }); + QCOMPARE(scope.messageIds, QStringList{ QStringLiteral("m1@example.org") }); + + // 2 from the whole thread plus 1 for the lone message. + QCOMPARE(scope.messageCount, 3); + QVERIFY(scope.wholeThread); +} + void TestThreadListModel::accountKeysComeFromTheAccountTags() { // Item 49 reads this to decide which mbsync channels a sync needs. Only -- cgit v1.2.3 From 98250d51021aee8929d9f5084a440647c43132b0 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 10:37:48 +0200 Subject: feat(ui): load a thread's replies when its row is expanded Replies are fetched on expansion rather than with the query: walking the reply tree of every thread in a 10k-thread result would cost more than the query and almost none of it would be looked at. hasChildren is what makes that lazy loading work, and its absence would have shipped the feature unreachable. rowCount is 0 until the worker has walked the thread, so a view left to infer the expander from rowCount alone draws none, the user can never expand, and the replies are never requested. It answers from the summary's totalCount before loading and from the children afterwards, so a thread whose count included duplicates stops offering an expander that opens onto nothing. onThreadTreeLoaded reads the thread id from the reply rather than remembering it from the request. Two expansions can be in flight at once, and pairing them by order would attach one thread's replies to the other. --- src/mainwindow.cpp | 40 ++++++++++++++++++++++++++++++++++++++++ src/mainwindow.h | 7 +++++++ src/threadlistmodel.cpp | 29 +++++++++++++++++++++++++++++ src/threadlistmodel.h | 10 ++++++++++ tests/test_threadlistmodel.cpp | 41 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 127 insertions(+) (limited to 'src/threadlistmodel.cpp') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 740edbe..720ec60 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -606,6 +606,12 @@ void MainWindow::buildUi() m_threadView->setColumnWidth(ThreadListModel::AuthorsColumn, 180); m_threadView->setColumnWidth(ThreadListModel::SubjectColumn, 520); + // Replies are loaded when a thread is expanded, not with the query. + // Walking the reply tree of every thread in a 10k-thread result would cost + // far more than the query itself and almost none of it would be looked at. + connect(m_threadView, &QTreeView::expanded, + this, &MainWindow::onThreadExpanded); + connect(m_threadView->selectionModel(), &QItemSelectionModel::currentRowChanged, this, &MainWindow::onThreadSelected); @@ -1298,6 +1304,8 @@ void MainWindow::wireWorker() this, &MainWindow::onThreadsReady); connect(m_worker, &NotmuchWorker::queryFinished, this, &MainWindow::onQueryFinished); + connect(m_worker, &NotmuchWorker::threadTreeLoaded, + this, &MainWindow::onThreadTreeLoaded); connect(m_worker, &NotmuchWorker::threadLoaded, this, &MainWindow::onThreadLoaded); connect(m_worker, &NotmuchWorker::errorOccurred, @@ -1667,6 +1675,38 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, Q_ARG(quint64, m_generation)); } +void MainWindow::onThreadExpanded(const QModelIndex &index) +{ + if (!index.isValid() || m_model->isMessageRow(index)) + return; + + const QString threadId = + m_model->data(index, ThreadListModel::ThreadIdRole).toString(); + if (threadId.isEmpty()) + return; + + QMetaObject::invokeMethod(m_worker, "loadThreadTree", Qt::QueuedConnection, + Q_ARG(QString, threadId), + Q_ARG(QString, m_lastQuery), + Q_ARG(quint64, m_generation)); +} + +void MainWindow::onThreadTreeLoaded(const QVector &nodes, + quint64 generation) +{ + // The same generation guard every other worker reply carries: an expansion + // whose query has since been replaced must not insert rows into the new + // result, where that thread may not even appear. + if (generation != m_generation || nodes.isEmpty()) + return; + + // Every node in one reply belongs to one thread, so the first one names it. + // Read from the node rather than remembered from the request: two + // expansions can be in flight at once, and pairing them by order would + // attach one thread's replies to the other. + m_model->setThreadMessages(nodes.first().threadId, nodes); +} + void MainWindow::onThreadLoaded(const QVector &messages, quint64 generation) { diff --git a/src/mainwindow.h b/src/mainwindow.h index 992cdd6..7014855 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -164,6 +164,13 @@ private slots: /// the click lands inside. void showThreadContextMenu(const QPoint &pos); void onThreadLoaded(const QVector &messages, quint64 generation); + + /// Asks the worker for a thread's reply tree when its row is expanded. + void onThreadExpanded(const QModelIndex &index); + + /// Fills in the expanded thread's message rows. + void onThreadTreeLoaded(const QVector &nodes, + quint64 generation); void onWorkerError(const QString &message); void onSyncFinished(bool success, int exitCode); diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 19e1153..d9882dc 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -151,6 +151,35 @@ int ThreadListModel::rowCount(const QModelIndex &parent) const return m_threads.at(parent.row()).children.size(); } +bool ThreadListModel::hasChildren(const QModelIndex &parent) const +{ + if (!parent.isValid()) + return !m_threads.isEmpty(); + + // A message row is always a leaf. Reply depth is drawn from the node's own + // depth, not from further nesting, so nothing hangs under a reply. + if (parent.parent().isValid()) + return false; + + if (parent.column() != 0) + return false; + + if (parent.row() < 0 || parent.row() >= m_threads.size()) + return false; + + const ThreadNode &node = m_threads.at(parent.row()); + + // Once loaded the children are the truth, including "there are none", which + // is how a thread whose totalCount counted duplicates stops offering an + // expander that opens onto nothing. + if (node.loaded) + return !node.children.isEmpty(); + + // Before loading, the summary's count is all there is. A thread of one + // message has no replies and must not offer an expander. + return node.summary.totalCount > 1; +} + int ThreadListModel::columnCount(const QModelIndex &parent) const { // Every level has the same columns. Returning 0 for a valid parent, as the diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index bc9fcbf..e39ade6 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -134,6 +134,16 @@ public: int rowCount(const QModelIndex &parent = {}) const override; int columnCount(const QModelIndex &parent = {}) const override; + + /// Whether a thread row should offer an expander. + /// + /// Answered from totalCount rather than from the loaded children, and that + /// is what makes lazy loading possible at all: rowCount is 0 until the + /// worker has walked the thread, so a view left to infer this from rowCount + /// alone draws no expander, the user can never expand, and the replies are + /// never asked for. The count is already in the summary, so this costs + /// nothing. + bool hasChildren(const QModelIndex &parent = {}) const override; QVariant data(const QModelIndex &index, int role) const override; QVariant headerData(int section, Qt::Orientation orientation, int role) const override; diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index 74b8dd1..3d131cf 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -32,6 +32,7 @@ private slots: void repliesBecomeChildRowsUnderTheirThread(); void messageRowsShowTheirOwnSenderAndSubject(); void reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem(); + void anUnexpandedMultiMessageThreadOffersAnExpander(); void scopeFollowsTheSelectedRowKind(); void scopeCountsEveryMessageOfAnUnexpandedThread(); void scopeHonoursAMixedSelectionWithoutEscalating(); @@ -196,6 +197,46 @@ void TestThreadListModel::reloadingAThreadReplacesItsRepliesRatherThanRepeatingT Q_UNUSED(tester); } +void TestThreadListModel::anUnexpandedMultiMessageThreadOffersAnExpander() +{ + // This is what makes lazy loading work at all. rowCount is 0 until the + // worker has walked the thread, so a view inferring the expander from + // rowCount alone draws none, the user can never expand, and the replies are + // never requested. hasChildren answers from the summary's count instead. + ThreadListModel model; + ThreadSummary many = makeThread(QStringLiteral("t1"), + QStringLiteral("Has replies")); + many.totalCount = 4; + ThreadSummary lone = makeThread(QStringLiteral("t2"), + QStringLiteral("Single message")); + lone.totalCount = 1; + model.appendBatch({ many, lone }); + + const QModelIndex withReplies = model.index(0, 0, QModelIndex()); + const QModelIndex single = model.index(1, 0, QModelIndex()); + + // Guard: neither is expanded, so this really is the unloaded case. + QCOMPARE(model.rowCount(withReplies), 0); + QCOMPARE(model.rowCount(single), 0); + + QVERIFY(model.hasChildren(withReplies)); + QVERIFY(!model.hasChildren(single)); + + // Once loaded the children are the truth, including "there are none": a + // thread whose count included duplicates must stop offering an expander + // that opens onto nothing. + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0) }); + QVERIFY(!model.hasChildren(withReplies)); + + // A message row is always a leaf. + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) }); + QVERIFY(model.hasChildren(withReplies)); + QVERIFY(!model.hasChildren(model.index(0, 0, withReplies))); +} + void TestThreadListModel::scopeFollowsTheSelectedRowKind() { ThreadListModel model; -- cgit v1.2.3 From 10ff78629b3d60810b85110a2f194e0d1b87752a Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 10:56:58 +0200 Subject: fix(ui): make the expander visible and the reply indent readable Both were reported from the running application after the previous commit claimed them working, and the tests that passed could not see either fault. The expander took four attempts, each of which looked right in code: - QTreeView::drawBranches is the documented hook and does not work here. It runs BEFORE the row's cells, so with the expander on a content column the delegate's own background paints over it. A 60-pixel triangle survived as 8, indistinguishable from the theme's near-invisible dot. - Sizing the glyph from the row rather than the branch rect put most of it outside that rect. - Moving it into SubjectDelegate but calling it from only the no-chip branch left every real row without one, since every real row has an account chip and takes the other branch. It is now drawn by the delegate, which owns the cell and paints after the background, from both branches, with setRootIsDecorated(false) so the style does not draw its dot underneath. The indent was 20px and invisible for a reason the geometry could not show: a thread row draws an account chip before its subject and a reply row does not, so a reply's text already starts about a chip's width LEFT of its thread's. The indent has to beat that before any nesting reads at all, hence 72px. The indent test asserted on visualRect, which was correctly indented the whole time, and so passed against a build with no visible nesting. It now measures where the TEXT lands, accounting for the chip, and fails at 20px. The new expander test counts painted pixels of the glyph colour against a control row with no replies, and fails when the call is dropped from either branch. --- src/mainwindow.cpp | 21 ++++++-- src/tagchip.cpp | 56 +++++++++++++++++++- src/tagchip.h | 14 +++++ src/threadlistmodel.cpp | 7 +++ src/threadlistmodel.h | 7 +++ src/threadlistview.h | 1 + tests/test_mainwindow.cpp | 130 +++++++++++++++++++++++++++++++++++++++++++++- 7 files changed, 231 insertions(+), 5 deletions(-) (limited to 'src/threadlistmodel.cpp') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 720ec60..8bab2e8 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -559,9 +559,24 @@ void MainWindow::buildUi() // paperclip out of a 28px column entirely. m_threadView->setTreePosition(ThreadListModel::SubjectColumn); - // The root thread rows are the top level, so no decoration for them beyond - // the expander a thread with replies gets on its own. - m_threadView->setRootIsDecorated(true); + // No style-drawn branch decoration. SubjectDelegate draws the expander + // itself, because drawBranches runs BEFORE the row's cells and the + // delegate's own background paints straight over anything put there: a + // 60-pixel triangle survived as 8 pixels, indistinguishable from the + // near-invisible dot this replaces. Leaving both enabled would draw the + // theme's dot underneath the delegate's glyph. + m_threadView->setRootIsDecorated(false); + + // Wider than Qt's default 20px, and the reason is specific rather than + // aesthetic. A thread row carries an account chip in front of its subject + // and a reply row does not, so a reply's text already starts roughly a + // chip's width (~60px) to the LEFT of its thread's. At the default indent + // the 20px shift is swallowed by that difference and the replies read as + // flush with the thread, or even outdented. Verified against the running + // app, not assumed: visualRect reported a correct 20px indent while the + // rendered text showed none, because the geometry is indented and the + // delegate then lays the text out from its own left edge. + m_threadView->setIndentation(SubjectDelegate::kReplyIndent); // Two delegates, and the split is not cosmetic. RowStyleDelegate carries // only the selection fix every column needs: the read/unread dimming diff --git a/src/tagchip.cpp b/src/tagchip.cpp index 1f4ad79..b3e743e 100644 --- a/src/tagchip.cpp +++ b/src/tagchip.cpp @@ -156,6 +156,51 @@ void SubjectDelegate::paint(QPainter *painter, const QStyleOptionViewItem &optio const QString account = index.data(ThreadListModel::AccountLabelRole).toString(); + + // The expander is drawn HERE and not in QTreeView::drawBranches, which is + // the obvious place and does not work. drawBranches runs before the row's + // cells, so with the expander column set to the subject the delegate's own + // background fills straight over it: measured at 8 surviving pixels of a + // 60-pixel triangle, which is exactly the near-invisible dot that made this + // override necessary in the first place. The delegate owns this cell and + // paints after the background, so it is the only place the glyph survives. + const auto drawExpander = [&](const QRect &cell) { + if (!index.data(ThreadListModel::HasRepliesRole).toBool()) + return; + + const int size = qMax(7, qMin(cell.height() / 3, 10)); + const QPoint centre(cell.left() + size, + cell.top() + subjectBandHeight(option) / 2 + + kRowPadding); + + QPolygon triangle; + if (option.state & QStyle::State_Open) { + triangle << QPoint(centre.x() - size / 2, centre.y() - size / 4) + << QPoint(centre.x() + size / 2, centre.y() - size / 4) + << QPoint(centre.x(), centre.y() + size / 2); + } else { + triangle << QPoint(centre.x() - size / 4, centre.y() - size / 2) + << QPoint(centre.x() + size / 2, centre.y()) + << QPoint(centre.x() - size / 4, centre.y() + size / 2); + } + + painter->save(); + painter->setRenderHint(QPainter::Antialiasing, true); + painter->setPen(Qt::NoPen); + // From the palette, so it survives a theme change, and undimmed: this + // is the only cue that a thread can be opened at all. + painter->setBrush(option.palette.color(QPalette::Text)); + painter->drawPolygon(triangle); + painter->restore(); + }; + // Room for the expander in front of whatever follows, on a thread row that + // has one. Reserved before either branch draws, so the chip and the bare + // subject are indented identically and a thread with replies does not sit + // a few pixels left of one without. + const bool hasReplies = + index.data(ThreadListModel::HasRepliesRole).toBool(); + const int expanderWidth = hasReplies ? kExpanderWidth : 0; + if (account.isEmpty()) { // No chip to draw, so the base class renders the text, confined to the // upper band: the lower one belongs to the row-wide pill strip that @@ -163,8 +208,10 @@ void SubjectDelegate::paint(QPainter *painter, const QStyleOptionViewItem &optio QStyleOptionViewItem chrome = option; initStyleOption(&chrome, index); chrome.rect.setHeight(subjectBandHeight(option)); + chrome.rect.setLeft(chrome.rect.left() + expanderWidth); QStyledItemDelegate::paint(painter, chrome, index); + drawExpander(option.rect); return; } @@ -186,7 +233,7 @@ void SubjectDelegate::paint(QPainter *painter, const QStyleOptionViewItem &optio const int textBandHeight = subjectBandHeight(option); const int textTop = option.rect.top() + kRowPadding; - const QRect chipRect(option.rect.left() + TagChip::kSpacing, + const QRect chipRect(option.rect.left() + expanderWidth + TagChip::kSpacing, textTop + (textBandHeight - chipSize.height()) / 2, chipSize.width(), chipSize.height()); @@ -234,6 +281,13 @@ void SubjectDelegate::paint(QPainter *painter, const QStyleOptionViewItem &optio rowMetrics.elidedText(index.data(Qt::DisplayRole).toString(), Qt::ElideRight, textRect.width())); painter->restore(); + + // Last, so the chrome fill above cannot cover it. BOTH branches of this + // function have to call it: a thread row with an account chip takes this + // one, and that is every row in the real application, so calling it only + // from the no-chip branch leaves the feature invisible in practice while + // still passing any test built on an untagged thread. + drawExpander(option.rect); } QSize SubjectDelegate::sizeHint(const QStyleOptionViewItem &option, diff --git a/src/tagchip.h b/src/tagchip.h index cc3b5de..3145358 100644 --- a/src/tagchip.h +++ b/src/tagchip.h @@ -92,6 +92,20 @@ public: /// Vertical breathing room above the subject and below the pill row. static constexpr int kRowPadding = 4; + /// How far a reply row is indented under its thread. + /// + /// Deliberately far wider than Qt's 20px default. A thread row carries an + /// account chip in front of its subject and a reply row does not, so a + /// reply's text starts about a chip's width to the LEFT of its thread's + /// before any indent is applied. 20px does not cover that, and the replies + /// come out looking flush or outdented; this has to beat a chip's width to + /// read as nesting at all. + static constexpr int kReplyIndent = 72; + + /// Horizontal room reserved in front of a thread's subject for the + /// expander glyph the delegate draws. + static constexpr int kExpanderWidth = 18; + /// The font the pill strip is drawn in: a size down from the row's own. /// /// At the same size the pills read as a second row of content competing diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index d9882dc..043e982 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -212,6 +212,10 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const return node.messageId; case MessageDepthRole: return node.depth; + case HasRepliesRole: + // A reply never has its own expander: nesting past the first level + // is drawn from depth, not from further parent-child structure. + return false; case ThreadIdRole: // A message row still belongs to a thread, and a caller that only // needs the containing thread must not have to walk up itself. @@ -274,6 +278,9 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const if (role == MessageDepthRole) return 0; + if (role == HasRepliesRole) + return hasChildren(index.siblingAtColumn(0)); + if (role == TagsRole) return thread.tags; diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index e39ade6..700b3a6 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -96,6 +96,13 @@ public: /// The message's reply depth, for the view's indentation. 1 for a /// direct reply, since depth 0 is the root row itself. MessageDepthRole, + + /// True when the row is a thread that has replies to show. + /// + /// Read by SubjectDelegate, which draws the expander itself: the + /// delegate cannot call hasChildren without the model, and the same + /// answer has to reach the cell that reserves room for the glyph. + HasRepliesRole, }; /// Row fill for a thread tagged `deleted`, and for one tagged `spam`. diff --git a/src/threadlistview.h b/src/threadlistview.h index 520d610..9e0da28 100644 --- a/src/threadlistview.h +++ b/src/threadlistview.h @@ -56,4 +56,5 @@ public: protected: void paintEvent(QPaintEvent *event) override; + }; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 7b6e55d..474d856 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -97,6 +97,7 @@ private slots: void aSelectedReadThreadIsNotDimmedIntoTheHighlight(); void thePillRowSpansTheWholeWidthNotOneColumn(); void childRowsAreIndentedUnderTheirThread(); + void aThreadWithRepliesDrawsAVisibleExpander(); void noTagStripIsPaintedUnderAMessageRow(); void markAllReadIsDisabledUntilTheQueryFinishes(); void markAllReadActsOnEveryRowAndUndoesInOneStep(); @@ -627,7 +628,12 @@ void TestMainWindow::childRowsAreIndentedUnderTheirThread() auto *view = window.findChild(); QVERIFY2(view, "the thread list is not a QTreeView, so it cannot indent"); - model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); + // With an account tag, so the thread row draws the chip that a reply row + // does not. That asymmetry is the whole reason the indent has to be wide, + // and a test against an untagged thread never sees it. + model->appendBatch({ makeThread( + QStringLiteral("t1"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }) }); MessageNode first; first.messageId = QStringLiteral("m0@example.org"); @@ -668,6 +674,128 @@ void TestMainWindow::childRowsAreIndentedUnderTheirThread() QVERIFY2(view->visualRect(child).left() > view->visualRect(rootCell).left(), "the reply is not indented relative to its thread"); + + // The geometry being indented is NOT the same as the reply LOOKING + // indented, and asserting only the former shipped a build with no visible + // nesting at all. A thread row draws an account chip before its subject and + // a reply row does not, so the reply's text starts about a chip's width to + // the left of the thread's; at Qt's default 20px indent that difference + // swallows the shift entirely. + // + // So the real property: where the TEXT lands. The reply's subject must + // begin to the right of the thread's, which is what the eye reads as + // nesting. + const int chipWidth = + TagChip::sizeFor(QFontMetrics(view->font()), + model->data(rootCell, ThreadListModel::AccountLabelRole) + .toString()).width(); + QVERIFY2(chipWidth > 0, + "the thread row has no account chip, so this test cannot measure " + "the offset it is meant to compensate for"); + + const int threadTextLeft = view->visualRect(rootCell).left() + chipWidth; + QVERIFY2(view->visualRect(child).left() > threadTextLeft, + qPrintable(QStringLiteral("the reply's text starts at x=%1, not " + "right of the thread's text at x=%2: the " + "indent does not beat the account chip " + "and the nesting is invisible") + .arg(view->visualRect(child).left()) + .arg(threadTextLeft))); +} + +void TestMainWindow::aThreadWithRepliesDrawsAVisibleExpander() +{ + // The expander is the ONLY thing saying a thread can be opened, and it took + // four wrong attempts to get on screen, each of which looked correct in + // code: + // + // - QTreeView::drawBranches, the documented hook, runs BEFORE the row's + // cells, so with the expander on a content column the delegate's own + // background paints over it. A 60-pixel triangle survived as 8. + // - Sizing it from the row rather than the branch rect put most of it + // outside that rect. + // - Moving it into the delegate but calling it from only one of the two + // branches left every real row without one, since every real row has an + // account chip and takes the other branch. + // + // None of those is visible to a test that asserts on geometry or on model + // roles, so this one counts painted pixels of the palette colour the glyph + // is drawn in. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + + // Two threads: one with replies, one without. The second is the control, + // and without it a test that counts text pixels would pass on any row. + ThreadSummary withReplies = makeThread( + QStringLiteral("t1"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }); + withReplies.totalCount = 3; + ThreadSummary lone = makeThread( + QStringLiteral("t2"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }); + lone.totalCount = 1; + model->appendBatch({ withReplies, lone }); + + window.resize(1400, 300); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + QApplication::processEvents(); + + const QModelIndex first = + model->index(0, ThreadListModel::SubjectColumn, QModelIndex()); + const QModelIndex second = + model->index(1, ThreadListModel::SubjectColumn, QModelIndex()); + + // Guards: both rows on screen, and the model agreeing about which has + // replies. Without these a zero count could mean anything. + QVERIFY2(view->visualRect(first).height() > 0, "the first row is not drawn"); + QVERIFY2(view->visualRect(second).height() > 0, + "the control row is not drawn"); + QVERIFY(model->data(first, ThreadListModel::HasRepliesRole).toBool()); + QVERIFY(!model->data(second, ThreadListModel::HasRepliesRole).toBool()); + + QImage shot(view->viewport()->size(), QImage::Format_ARGB32); + shot.fill(Qt::transparent); + view->viewport()->render(&shot); + + // The exact colour the glyph is filled with, matched exactly rather than by + // a brightness threshold, which would count antialiased subject text. + const QRgb glyph = view->palette().color(QPalette::Text).rgb(); + + // Only the strip in front of the subject text, so the subject's own glyphs + // cannot be counted. kExpanderWidth is the room the delegate reserves. + const auto countGlyphPixels = [&](const QModelIndex &index) { + const QRect rect = view->visualRect(index); + int found = 0; + for (int y = rect.top(); y < qMin(rect.bottom(), shot.height()); ++y) { + for (int x = rect.left(); + x < qMin(rect.left() + SubjectDelegate::kExpanderWidth, + shot.width()); + ++x) { + if ((shot.pixel(x, y) | 0xff000000) == (glyph | 0xff000000)) + ++found; + } + } + return found; + }; + + const int drawn = countGlyphPixels(first); + const int control = countGlyphPixels(second); + + QVERIFY2(drawn > 12, + qPrintable(QStringLiteral("only %1 expander pixels: the glyph is " + "clipped or painted over, which is how " + "it shipped as an invisible dot") + .arg(drawn))); + + // The control must have none, or the count above is measuring something + // every row draws. + QCOMPARE(control, 0); } void TestMainWindow::noTagStripIsPaintedUnderAMessageRow() -- cgit v1.2.3 From 1304ecf7c683a7874b8f571433379caf72e0483b Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 11:07:03 +0200 Subject: feat(ui): mark replies with a thread line, a tint and dimmer text Indentation alone still read as a table, which was the user's original complaint about the whole item. Three cues now say the rows belong to the thread above them: a spine down the left of the expanded block with a stub out to each reply, a background tint, and text a size down and undimmed only when unread. Both colours are mixed from the palette rather than fixed, the same rule readColour follows: a tint that reads as grouping on a light theme is invisible or muddy on a dark one. The tint is deliberately near the threshold of noticing, since it sits beside the deleted and spam fills, which carry real meaning and must stay the loudest thing in the list. The spine is accumulated across the visible reply rows and drawn once after the loop. Drawn per row it left a gap at every row boundary and read as a column of dashes rather than as the structure holding the block together. Two bugs fixed here, both mine, both from the previous commit: Clicking the expander did nothing. setRootIsDecorated(false), needed to stop the style painting its own indicator under ours, also removed the style's hit area, so the glyph rendered perfectly and was inert. ThreadListView handles the press itself now, over the strip the delegate reserves, leaving the rest of the subject cell to select the row. Every click then expanded rather than toggling, because isExpanded and setExpanded are keyed on column 0 and were being asked about the subject-column index, which always answers false. Visible, clickable and toggling are three separate properties and a test for one passes against the other two being broken: the pixel test proved the triangle was drawn while it could not be clicked, and the first click test proved it opened while it could never close. The test now clicks twice and asserts open then closed. replyRowsKeepTheirTextUnderTheThreadLine covers the other trap. paintEvent runs AFTER the cells, so the first version of the tint filled the whole reply row and erased the sender and subject the delegate had just drawn: zero surviving text pixels, a block of blank tinted rows. The fill and the stub stay in the band below the text, where the tag strip lives on thread rows. --- src/threadlistmodel.cpp | 64 +++++++++++++++++++++++- src/threadlistmodel.h | 11 +++++ src/threadlistview.cpp | 109 ++++++++++++++++++++++++++++++++++++++-- src/threadlistview.h | 10 ++++ tests/test_mainwindow.cpp | 123 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 311 insertions(+), 6 deletions(-) (limited to 'src/threadlistmodel.cpp') diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 043e982..f9efee7 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -75,6 +75,46 @@ QString ThreadListModel::flagGlyph() return glyph; } +QColor ThreadListModel::replyBackground() +{ + // Mixed from the palette rather than fixed, for the same reason as + // readColour: a tint that reads as "grouped" on a light theme is either + // invisible or muddy on a dark one. + // + // Toward Text rather than toward a hue, so it darkens on a light theme and + // lightens on a dark one without picking a colour that means something + // else. 0.07 is deliberately near the threshold of noticing: it is a + // grouping cue sitting beside the deleted and spam fills, which carry + // actual meaning and must stay the loudest thing in the list. + const QPalette palette = QGuiApplication::palette(); + const QColor base = palette.color(QPalette::Base); + const QColor text = palette.color(QPalette::Text); + + constexpr qreal kWeight = 0.07; + const qreal inverse = 1.0 - kWeight; + return QColor::fromRgbF( + text.redF() * kWeight + base.redF() * inverse, + text.greenF() * kWeight + base.greenF() * inverse, + text.blueF() * kWeight + base.blueF() * inverse); +} + +QColor ThreadListModel::threadLineColour() +{ + // Stronger than the tint, weaker than the text: the line is structure, so + // it has to be followable down a long expansion without competing with the + // senders beside it. + const QPalette palette = QGuiApplication::palette(); + const QColor base = palette.color(QPalette::Base); + const QColor text = palette.color(QPalette::Text); + + constexpr qreal kWeight = 0.35; + const qreal inverse = 1.0 - kWeight; + return QColor::fromRgbF( + text.redF() * kWeight + base.redF() * inverse, + text.greenF() * kWeight + base.greenF() * inverse, + text.blueF() * kWeight + base.blueF() * inverse); +} + QColor ThreadListModel::readColour() { // Derived from the palette, never hardcoded: a fixed grey that reads as @@ -248,9 +288,29 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const default: return {}; } + case Qt::BackgroundRole: + // Tinted, so an expanded thread reads as one block rather than as + // more table rows. Applied per cell here; ThreadListView fills the + // same colour across the strip's band so the row does not end up + // half tinted. + return replyBackground(); + case Qt::FontRole: { + // A size down from the thread rows, so a thread reads as the + // heading and its replies as the contents. Never bold: an unread + // reply is still subordinate to the thread it belongs to, and the + // thread row above already carries the unread cue for the whole + // conversation. + QFont font = QGuiApplication::font(); + if (font.pointSize() > 0) + font.setPointSize(qMax(6, font.pointSize() - 1)); + else if (font.pixelSize() > 0) + font.setPixelSize(qMax(8, font.pixelSize() - 2)); + return font; + } case Qt::ForegroundRole: - // Same rule as a thread row: read recedes, unread stays at the - // palette's own colour. + // Dimmed whether read or not, for the same reason as the font: a + // reply is subordinate content. An unread one is left undimmed so + // it can still be found. return node.isUnread() ? QVariant() : QVariant(readColour()); default: return {}; diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index 700b3a6..c56e80a 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -120,6 +120,17 @@ public: static QColor deletedColour(); static QColor spamColour(); + /// Background for a reply row, so an expanded thread reads as one block + /// rather than as more table rows. + /// + /// Derived from the palette and deliberately subtle: it marks a grouping, + /// and a tint strong enough to notice on its own would compete with the + /// deleted and spam row colours, which carry real meaning. + static QColor replyBackground(); + + /// The line drawn down the left of an expanded thread's replies. + static QColor threadLineColour(); + /// The dimmed text colour a READ thread carries. /// /// Unread rows are left at the palette's own colour and read ones recede, diff --git a/src/threadlistview.cpp b/src/threadlistview.cpp index ebca4cc..1ffec04 100644 --- a/src/threadlistview.cpp +++ b/src/threadlistview.cpp @@ -21,10 +21,44 @@ #include "tagchip.h" #include "threadlistmodel.h" +#include #include #include #include +void ThreadListView::mousePressEvent(QMouseEvent *event) +{ + const QModelIndex index = indexAt(event->pos()); + + // Only a thread row, only the subject column, only the strip the delegate + // reserved for the glyph. Anything wider would swallow clicks meant to + // select the row, which is what the rest of the subject cell is for. + if (event->button() == Qt::LeftButton && index.isValid() + && !index.parent().isValid() + && index.column() == ThreadListModel::SubjectColumn + && index.data(ThreadListModel::HasRepliesRole).toBool()) { + + const QRect rect = visualRect(index); + if (event->pos().x() >= rect.left() + && event->pos().x() < rect.left() + SubjectDelegate::kExpanderWidth) { + // Column 0, not the clicked index. Expansion state belongs to the + // ROW, and QTreeView keys it on the first column: asking + // isExpanded() about the subject-column index always answers false, + // so every click expanded again instead of toggling. + const QModelIndex row = index.siblingAtColumn(0); + setExpanded(row, !isExpanded(row)); + + // Swallowed, so the click that opened a thread does not also load + // it into the message pane: expanding is a request to see the + // thread's shape, not to read it. + event->accept(); + return; + } + } + + QTreeView::mousePressEvent(event); +} + void ThreadListView::paintEvent(QPaintEvent *event) { QTreeView::paintEvent(event); @@ -55,16 +89,74 @@ void ThreadListView::paintEvent(QPaintEvent *event) // the same one. int visualRow = 0; + // The spine's extent, collected across the reply rows and drawn once after + // the loop. Per-row segments leave a gap at every row boundary and read as + // a column of dashes rather than as one line. + int spineX = -1; + int spineTop = std::numeric_limits::max(); + int spineBottom = std::numeric_limits::min(); + for (; walk.isValid(); walk = indexBelow(walk), ++visualRow) { const QRect rowRect = visualRect(walk); if (rowRect.top() > viewport()->height()) break; - // No strip under a message row. The strip carries the THREAD's tags, so - // one under each reply would stripe the list and repeat identical tags - // down the whole expansion. - if (walk.parent().isValid()) + // A message row: no tag strip, but it does get the band filled to its + // own tint and a thread line down its left. + // + // The band has to be filled here for the same reason a thread row's is. + // The cells paint the tint per cell, so nothing covers the width to the + // right of the last column or the lower band the strip normally + // occupies, and an untouched reply row comes out tinted across its text + // and bare underneath it. + if (walk.parent().isValid()) { + // Only the band BELOW the text, never the whole row. paintEvent + // runs after the cells, so filling the row's full height paints + // over the sender and subject the delegate just drew: measured at + // zero surviving text pixels, a block of blank tinted rows. + const int bandTop = rowRect.top() + SubjectDelegate::kRowPadding + + rowMetrics.height(); + const QRect band(columnViewportPosition(ThreadListModel::DateColumn), + bandTop, + viewport()->width() + - columnViewportPosition( + ThreadListModel::DateColumn), + rowRect.bottom() - bandTop + 1); + + if (selectionModel() + && selectionModel()->isSelected( + walk.siblingAtColumn(ThreadListModel::SubjectColumn))) { + painter.fillRect(band, palette().brush(QPalette::Highlight)); + } else { + painter.fillRect(band, ThreadListModel::replyBackground()); + } + + // The spine is NOT drawn here. Drawing it per row leaves a gap + // wherever consecutive rows do not abut exactly, which is every row + // boundary once the rows carry padding: the result reads as a + // column of dashes rather than as one line. It is drawn as a single + // continuous run after this loop, from the collected extents below. + const int subjectLeft = + columnViewportPosition(ThreadListModel::SubjectColumn); + const int lineX = subjectLeft + SubjectDelegate::kExpanderWidth / 2; + + if (spineX < 0) + spineX = lineX; + spineTop = qMin(spineTop, rowRect.top()); + spineBottom = qMax(spineBottom, rowRect.bottom() + 1); + + // A stub out to the row, so each reply is visibly attached to the + // spine rather than merely beside it. Drawn in the LOWER band, not + // at the row's midpoint: the midpoint crosses the sender text, and + // this paints after the cells. + const int stubY = bandTop + (rowRect.bottom() - bandTop) / 2; + painter.setPen(QPen(ThreadListModel::threadLineColour(), 2)); + painter.drawLine(lineX, stubY, + subjectLeft + SubjectDelegate::kReplyIndent + - TagChip::kSpacing * 2, + stubY); continue; + } const QModelIndex index = walk.siblingAtColumn( ThreadListModel::SubjectColumn); @@ -156,4 +248,13 @@ void ThreadListView::paintEvent(QPaintEvent *event) x += size.width() + TagChip::kSpacing; } } + + // One continuous spine over every visible reply row, drawn last so no + // cell fill can break it. Segments drawn per row left a dash at every row + // boundary, which read as a dotted decoration rather than as the structure + // holding the block together. + if (spineX >= 0 && spineBottom > spineTop) { + painter.setPen(QPen(ThreadListModel::threadLineColour(), 2)); + painter.drawLine(spineX, spineTop, spineX, spineBottom); + } } diff --git a/src/threadlistview.h b/src/threadlistview.h index 9e0da28..0ebe3ea 100644 --- a/src/threadlistview.h +++ b/src/threadlistview.h @@ -57,4 +57,14 @@ public: protected: void paintEvent(QPaintEvent *event) override; + /// Toggles a thread when its expander glyph is clicked. + /// + /// The view owns this because the glyph is drawn by SubjectDelegate and a + /// delegate gets no click of its own without an editor. Being VISIBLE and + /// being CLICKABLE are separate properties: setRootIsDecorated(false), + /// needed to stop the style drawing its own indicator underneath ours, also + /// removed the style's hit area, so the expander painted correctly and did + /// nothing at all. + void mousePressEvent(QMouseEvent *event) override; + }; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 474d856..cee32c6 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -99,6 +99,8 @@ private slots: void childRowsAreIndentedUnderTheirThread(); void aThreadWithRepliesDrawsAVisibleExpander(); void noTagStripIsPaintedUnderAMessageRow(); + void replyRowsKeepTheirTextUnderTheThreadLine(); + void clickingTheExpanderTogglesTheThread(); void markAllReadIsDisabledUntilTheQueryFinishes(); void markAllReadActsOnEveryRowAndUndoesInOneStep(); void markAllReadDoesNothingWhenNothingIsUnread(); @@ -798,6 +800,127 @@ void TestMainWindow::aThreadWithRepliesDrawsAVisibleExpander() QCOMPARE(control, 0); } +void TestMainWindow::clickingTheExpanderTogglesTheThread() +{ + // The glyph being VISIBLE and the glyph being CLICKABLE are separate + // properties, and the pixel test for the first passes happily against a + // triangle nothing can hit. Turning off rootIsDecorated to stop the style + // drawing its own dot under ours also removed the style's hit area, so the + // expander rendered perfectly and did nothing. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + + ThreadSummary t = makeThread( + QStringLiteral("t1"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }); + t.totalCount = 3; + model->appendBatch({ t }); + + window.resize(1400, 300); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + QApplication::processEvents(); + + const QModelIndex root = model->index(0, 0, QModelIndex()); + const QModelIndex subject = + model->index(0, ThreadListModel::SubjectColumn, QModelIndex()); + const QRect rect = view->visualRect(subject); + + // Guards: the row is drawn, it claims to have replies, and it starts + // collapsed. Without the last one a toggle test can pass by doing nothing. + QVERIFY2(rect.height() > 0, "the thread row is not on screen"); + QVERIFY(model->data(subject, ThreadListModel::HasRepliesRole).toBool()); + QVERIFY(!view->isExpanded(root)); + + // Aimed at the glyph itself: the delegate reserves kExpanderWidth at the + // left of the subject cell and centres the triangle in it. + const QPoint hit(rect.left() + SubjectDelegate::kExpanderWidth / 2, + rect.top() + SubjectDelegate::kRowPadding + + QFontMetrics(view->font()).height() / 2); + + QTest::mouseClick(view->viewport(), Qt::LeftButton, Qt::NoModifier, hit); + QApplication::processEvents(); + QVERIFY2(view->isExpanded(root), + "clicking the expander did not open the thread"); + + QTest::mouseClick(view->viewport(), Qt::LeftButton, Qt::NoModifier, hit); + QApplication::processEvents(); + QVERIFY2(!view->isExpanded(root), + "clicking the expander again did not close the thread"); +} + +void TestMainWindow::replyRowsKeepTheirTextUnderTheThreadLine() +{ + // paintEvent runs AFTER the cells, so anything it fills across a reply row + // covers the text the delegate just drew. The tint and the thread line are + // both painted there, which makes this the obvious way to ship a block of + // blank rows. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + + ThreadSummary t = makeThread( + QStringLiteral("t1"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }); + t.totalCount = 2; + model->appendBatch({ t }); + + MessageNode first; + first.messageId = QStringLiteral("m0@example.org"); + first.threadId = QStringLiteral("t1"); + first.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t1"); + reply.from = QStringLiteral("A Replier "); + reply.subject = QStringLiteral("Re: a subject"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t1"), { first, reply }); + + window.resize(1400, 300); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const QModelIndex root = model->index(0, 0, QModelIndex()); + view->expand(root); + QApplication::processEvents(); + + const QModelIndex child = + model->index(0, ThreadListModel::AuthorsColumn, root); + const QRect rect = view->visualRect(child); + QVERIFY2(rect.height() > 0, "the reply row is not on screen"); + + QImage shot(view->viewport()->size(), QImage::Format_ARGB32); + shot.fill(Qt::transparent); + view->viewport()->render(&shot); + + // Count pixels in the sender cell that differ from the row's own tint. + // Text is the only thing that can produce them. + const QRgb tint = ThreadListModel::replyBackground().rgb() | 0xff000000; + int textPixels = 0; + for (int y = rect.top(); y < qMin(rect.bottom(), shot.height()); ++y) { + for (int x = rect.left(); x < qMin(rect.right(), shot.width()); ++x) { + if ((shot.pixel(x, y) | 0xff000000) != tint) + ++textPixels; + } + } + + QVERIFY2(textPixels > 20, + qPrintable(QStringLiteral("only %1 non-background pixels in the " + "reply's sender cell: the row was " + "painted over after its text was drawn") + .arg(textPixels))); +} + void TestMainWindow::noTagStripIsPaintedUnderAMessageRow() { // The strip is a row-wide band of the THREAD's tags. Painted under every -- cgit v1.2.3 From 7c3648676e188344dabb24e084f91b2b47e87633 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 11:22:39 +0200 Subject: feat(ui): scope actions to the selected row kind and name it tagSelected resolved rows to threads with threadAt(index.row()), which is wrong for a message row: a child's row number indexes its siblings, so acting on a reply tagged whichever thread sat at that position in the list. It now routes through ThreadListModel::scopeFor, and a message row's change is sent as message ids down applyTags with its own MessageTagCommand for undo. MessageTagCommand stores message ids where ThreadTagCommand stores thread ids, and that difference is the point rather than an inconsistency: re-resolving the thread on undo would restore tags across every sibling the action never touched. sendMessageTagChange deliberately skips the 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 watches correct itself on the next query. It keeps the two things that are NOT optional: the edited-account set, resolved through the containing thread since the account is a property of the thread, and holding the edit when a sync holds notmuch's write lock, since the worker's read-write open blocks rather than failing. The scope is now stated before an action and after it, naming both the message count and whether a whole thread went. This is what stands in for the confirmation dialog CLAUDE.md rules out: undo is the safety net, and undo is only usable if the user can tell that something larger than they meant has just happened. Selecting a single message reports no count at all, since reading one message is not a bulk action. A mutation that routed message rows down the thread path SURVIVED the whole suite: undo depth and status text are identical either way while every sibling gets tagged. anActionOnAMessageRowTagsThatMessageNotTheThread exists because that gap was found, and asserts on the ids actually sent. anActionOnAThreadRowSaysItHitTheWholeThread reads the status bar BEFORE draining the event loop. This binary has no worker, backlog item 36, so the queued write reaches a database that has never heard of the thread and answers with errorOccurred, which overwrites the status bar: draining first asserts on that error and fails against correct code. --- src/mainwindow.cpp | 168 ++++++++++++++++++++++++++++++------- src/mainwindow.h | 65 ++++++++++++++ src/threadlistmodel.cpp | 11 +++ src/threadlistmodel.h | 5 ++ tests/test_mainwindow.cpp | 210 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 431 insertions(+), 28 deletions(-) (limited to 'src/threadlistmodel.cpp') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index d9aa57a..423aa7b 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1607,33 +1607,82 @@ void MainWindow::showThreadContextMenu(const QPoint &pos) void MainWindow::onSelectionChanged() { - const int selected = m_threadView->selectionModel()->selectedRows().size(); - if (selected <= 1) { - // Clearing the count here would wipe whatever the last action reported - // ("Archive: 3 threads"), which is the more useful message once the - // selection is gone. Only a count this function wrote is taken back. - if (m_statusLabel->text() == m_selectionMessage) - m_statusLabel->clear(); - m_selectionMessage.clear(); + const QModelIndexList rows = m_threadView->selectionModel()->selectedRows(); + const int selected = rows.size(); + if (selected == 1) { + // One row selected. With two kinds of row this is exactly where the + // scope became ambiguous: a thread root stands for every message in it, + // a message row for one, and the keypress looks identical. Naming it + // here is what this project does instead of a confirmation dialog, + // which CLAUDE.md rules out for tag mutations. + const ActionScope scope = m_model->scopeFor(rows); + + if (scope.wholeThread) { + m_selectionMessage = + tr("1 thread selected (%n message(s))", "", scope.messageCount); + m_statusLabel->setText(m_selectionMessage); + m_statusTimer->stop(); + m_transientMessage.clear(); + } else { + // Reading one message is not a bulk action and gets no count. + if (m_statusLabel->text() == m_selectionMessage) + m_statusLabel->clear(); + m_selectionMessage.clear(); + } - // Collapsing a multi-row selection back to one row has to load that - // row here, and cannot be left to onThreadSelected. currentRowChanged - // is emitted BEFORE the selection model is updated (verified against - // Qt 6.11), so when a click collapses three rows to one, that handler - // still sees three selected, takes the multi-select branch and returns - // without loading anything. Only this signal sees the real count. + // Collapsing a multi-row selection back to one row has to load that row + // here, and cannot be left to onThreadSelected: currentRowChanged is + // emitted BEFORE the selection model is updated (verified against + // Qt 6.11), so that handler still sees the old count and returns + // without loading anything. + // + // Compared per row kind. A message row's row number indexes its + // siblings, so threadAt() on one answers about an unrelated thread and + // the comparison below would be against the wrong id. const QModelIndex current = m_threadView->currentIndex(); - if (current.isValid() - && m_model->threadAt(current.row()).threadId != m_currentThreadId) { - onThreadSelected(current, QModelIndex()); + if (current.isValid()) { + const bool changed = + m_model->isMessageRow(current) + ? m_model->messageAt(current).messageId != m_currentMessageId + : m_model->threadAt(current.row()).threadId + != m_currentThreadId; + if (changed) + onThreadSelected(current, QModelIndex()); } return; } + if (selected < 1) { + // Nothing selected. Clearing unconditionally would wipe whatever the + // last action reported ("Archive: 3 threads"), which is the more useful + // message once the selection is gone, so only a count this function + // wrote is taken back. + if (m_statusLabel->text() == m_selectionMessage) + m_statusLabel->clear(); + m_selectionMessage.clear(); + return; + } + // The count is the part that actually teaches multi-select: it acknowledges // the selection while it is being built, rather than only after an action // has already been applied to it. - m_selectionMessage = tr("%n thread(s) selected", "", selected); + // + // Reported per row kind rather than as a bare row count, so a mixed + // selection says what it will really touch instead of calling three replies + // "3 threads". + const ActionScope scope = m_model->scopeFor(rows); + if (!scope.threadIds.isEmpty() && scope.messageIds.isEmpty()) { + m_selectionMessage = + tr("%n thread(s) selected (%1 messages)", "", scope.threadIds.size()) + .arg(scope.messageCount); + } else if (scope.threadIds.isEmpty()) { + m_selectionMessage = + tr("%n message(s) selected", "", scope.messageIds.size()); + } else { + m_selectionMessage = + tr("%n thread(s) and %1 message(s) selected", "", + scope.threadIds.size()).arg(scope.messageIds.size()); + } m_statusLabel->setText(m_selectionMessage); // State, not an event: it must persist while the selection does. Cancel any @@ -2457,20 +2506,83 @@ void MainWindow::tagSelected(const QStringList &add, const QStringList &remove, if (rows.isEmpty()) return; - QStringList threadIds; - threadIds.reserve(rows.size()); - for (const QModelIndex &index : rows) - threadIds.append(m_model->threadAt(index.row()).threadId); + // Resolved through the model rather than by mapping rows to threads here. + // A message row's row number indexes its siblings, so the old + // threadAt(index.row()) mapping silently acted on whichever thread sat at + // that position in the list. + const ActionScope scope = m_model->scopeFor(rows); + if (scope.isEmpty()) + return; - sendThreadTagChange(threadIds, add, remove, description); + if (!scope.threadIds.isEmpty()) { + sendThreadTagChange(scope.threadIds, add, remove, description); - // Pushed for undo. The inverse re-resolves the same threads, so it works - // whether or not those rows are still selected. - m_undoStack.push(new ThreadTagCommand(this, threadIds, add, remove, - description)); + // Pushed for undo. The inverse re-resolves the same threads, so it + // works whether or not those rows are still selected. + m_undoStack.push(new ThreadTagCommand(this, scope.threadIds, add, + remove, description)); + } + if (!scope.messageIds.isEmpty()) { + sendMessageTagChange(scope.messageIds, add, remove, description); + m_undoStack.push(new MessageTagCommand(this, scope.messageIds, add, + remove, description)); + } + + // The scope named after the fact, since the selection may well be gone by + // the time the user reads it. This is what stands in for the confirmation + // dialog CLAUDE.md rules out: undo is the safety net, and undo is only + // usable if the user can tell that something larger than they meant has + // just happened. showTransientStatus( - tr("%1: %n thread(s)", "", threadIds.size()).arg(description)); + scope.wholeThread + ? tr("%1: %n message(s) (whole thread)", "", scope.messageCount) + .arg(description) + : tr("%1: %n message(s)", "", scope.messageCount).arg(description)); +} + +void MainWindow::sendMessageTagChange(const QStringList &messageIds, + const QStringList &add, + const QStringList &remove, + const QString &description) +{ + if (messageIds.isEmpty()) + return; + + // 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 + // whether one message moved or seven. + for (const QString &messageId : messageIds) { + const QString threadId = m_model->threadIdForMessage(messageId); + if (threadId.isEmpty()) + continue; + for (const QString &key : m_model->accountKeysForThread(threadId)) + m_editedAccounts.insert(key); + } + + // Held during a sync for exactly the reason the thread path is: the + // worker's read-write open BLOCKS on notmuch's exclusive lock rather than + // failing, so sending now would freeze the worker for the rest of the run. + if (aSyncHoldsTheWriteLock()) { + m_heldEdits.append(HeldEdit{ + {}, TagChange{ messageIds, add, remove, description } }); + m_statusLabel->setText( + tr("A sync is running; your change will be applied when it " + "finishes.")); + updatePendingIndicator(); + return; + } + + m_pendingThreadIds.clear(); + m_pendingChange = TagChange{ messageIds, add, remove, description }; + + QMetaObject::invokeMethod(m_worker, "applyTags", Qt::QueuedConnection, + Q_ARG(TagChange, m_pendingChange)); } void MainWindow::sendThreadTagChange(const QStringList &threadIds, diff --git a/src/mainwindow.h b/src/mainwindow.h index a4eca20..6b9e557 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -129,6 +129,19 @@ public: /// command was pushed, which is what "this did nothing" has to assert. int undoDepthForTesting() const { return m_undoStack.count(); } + /// The ids the last tag change was sent for, and whether they were thread + /// ids or message ids. + /// + /// Exposed because the difference is invisible from outside otherwise: a + /// message row routed down the thread path produces the same undo depth and + /// the same status text while tagging every sibling in the thread. A + /// mutation that made exactly that change passed the whole suite. + QStringList pendingThreadIdsForTesting() const { return m_pendingThreadIds; } + QStringList pendingMessageIdsForTesting() const + { + return m_pendingChange.messageIds; + } + /// The generation a worker reply must carry to be accepted. /// /// A test seam: onQueryFinished() discards a reply whose generation is @@ -355,6 +368,13 @@ private: const QStringList &remove, const QString &description); + /// The same for individual MESSAGES, without touching the undo stack. + /// Both tagSelected() and MessageTagCommand route through this. + void sendMessageTagChange(const QStringList &messageIds, + const QStringList &add, + const QStringList &remove, + const QString &description); + /// Undoes the optimistic model update for a write the worker rejected. void revertPendingTagChange(); @@ -388,6 +408,7 @@ private: QVector m_heldEdits; friend class ThreadTagCommand; + friend class MessageTagCommand; Config m_config; KeyMap m_keyMap; @@ -629,3 +650,47 @@ private: QString m_description; bool m_firstRedo = true; }; + +/// Undo entry for a tag change over individual MESSAGES. +/// +/// Stores message ids, unlike ThreadTagCommand, and that difference is the +/// point rather than an inconsistency: a message row acts on one message, so +/// re-resolving its thread on undo would restore tags across every sibling the +/// action never touched. +class MessageTagCommand : public QUndoCommand +{ +public: + MessageTagCommand(MainWindow *window, const QStringList &messageIds, + const QStringList &add, const QStringList &remove, + const QString &description) + : QUndoCommand(description), m_window(window), + m_messageIds(messageIds), m_add(add), m_remove(remove), + m_description(description) {} + + /// The stack calls redo() when the command is pushed, by which point the + /// change has already been sent, so the first call is skipped. + void redo() override + { + if (m_firstRedo) { + m_firstRedo = false; + return; + } + m_window->sendMessageTagChange(m_messageIds, m_add, m_remove, + m_description); + } + + void undo() override + { + m_window->sendMessageTagChange( + m_messageIds, m_remove, m_add, + QStringLiteral("Undo %1").arg(m_description)); + } + +private: + MainWindow *m_window; + QStringList m_messageIds; + QStringList m_add; + QStringList m_remove; + QString m_description; + bool m_firstRedo = true; +}; diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index f9efee7..9a74041 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -594,6 +594,17 @@ MessageNode ThreadListModel::messageAt(const QModelIndex &index) const return children.at(index.row()); } +QString ThreadListModel::threadIdForMessage(const QString &messageId) const +{ + for (const ThreadNode &node : m_threads) { + for (const MessageNode &child : node.children) { + if (child.messageId == messageId) + return node.summary.threadId; + } + } + return {}; +} + ActionScope ThreadListModel::scopeFor(const QModelIndexList &selection) const { ActionScope scope; diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index c56e80a..b80488b 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -188,6 +188,11 @@ public: /// is not a message row. MessageNode messageAt(const QModelIndex &index) const; + /// The thread a loaded message row belongs to, or empty when no expanded + /// thread holds it. Only expanded threads have message rows at all, so a + /// message the user could select is always findable here. + QString threadIdForMessage(const QString &messageId) const; + /// Resolves a selection into what an action should touch. /// /// Mixed selections are honoured as given: a thread root and an unrelated diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 3a375d4..0a916c4 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -102,6 +102,10 @@ private slots: void replyRowsKeepTheirTextUnderTheThreadLine(); void clickingTheExpanderTogglesTheThread(); void selectingAMessageRowTargetsThatMessageNotItsThread(); + void selectingAThreadRowNamesHowManyMessagesItStandsFor(); + void selectingAMessageRowReportsNoBulkCount(); + void anActionOnAThreadRowSaysItHitTheWholeThread(); + void anActionOnAMessageRowTagsThatMessageNotTheThread(); void markAllReadIsDisabledUntilTheQueryFinishes(); void markAllReadActsOnEveryRowAndUndoesInOneStep(); void markAllReadDoesNothingWhenNothingIsUnread(); @@ -801,6 +805,212 @@ void TestMainWindow::aThreadWithRepliesDrawsAVisibleExpander() QCOMPARE(control, 0); } +void TestMainWindow::selectingAThreadRowNamesHowManyMessagesItStandsFor() +{ + // With two kinds of row selectable, one selected row no longer says how + // much an action will touch. CLAUDE.md forbids a confirmation dialog for + // tag mutations, so the scope is made visible instead: this is the "before" + // half of that, and the count has to come from the thread's own total, not + // from whatever happens to be expanded. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + auto *status = window.findChild(QStringLiteral("statusMessage")); + QVERIFY(status); + + ThreadSummary t = makeThread(QStringLiteral("t1"), {}); + t.totalCount = 7; + model->appendBatch({ t }); + + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + // Guard: nothing is expanded, so a count taken from the loaded children + // would read 0 and this test would be measuring the wrong source. + QCOMPARE(model->rowCount(model->index(0, 0, QModelIndex())), 0); + + selectThreadRow(view, 0); + QApplication::processEvents(); + + QVERIFY2(status->text().contains(QStringLiteral("7")), + qPrintable(QStringLiteral("the status bar says '%1', which does " + "not name the 7 messages the thread " + "stands for") + .arg(status->text()))); +} + +void TestMainWindow::selectingAMessageRowReportsNoBulkCount() +{ + // Reading one message is not a bulk action, so it gets no count. A message + // row reporting "1 thread selected" would be actively wrong about what an + // action would touch. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + auto *status = window.findChild(QStringLiteral("statusMessage")); + QVERIFY(status); + + ThreadSummary t = makeThread(QStringLiteral("t1"), {}); + t.totalCount = 3; + model->appendBatch({ t }); + + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("t1"); + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t1"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t1"), { root, reply }); + + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const QModelIndex threadRow = model->index(0, 0, QModelIndex()); + view->expand(threadRow); + QApplication::processEvents(); + + const QModelIndex messageRow = model->index(0, 0, threadRow); + QVERIFY(model->isMessageRow(messageRow)); + + view->selectionModel()->select( + messageRow, + QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(messageRow); + QApplication::processEvents(); + + QVERIFY2(!status->text().contains(QStringLiteral("thread")), + qPrintable(QStringLiteral("a single message row reports '%1', " + "which claims a thread-wide scope it " + "does not have") + .arg(status->text()))); +} + +void TestMainWindow::anActionOnAThreadRowSaysItHitTheWholeThread() +{ + // The "after" half. Undo is the safety net this project chose over a + // confirmation dialog, and undo is only usable if the user can tell that + // something bigger than they intended just happened. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + auto *status = window.findChild(QStringLiteral("statusMessage")); + QVERIFY(status); + + ThreadSummary t = makeThread(QStringLiteral("t1"), {}); + t.totalCount = 7; + model->appendBatch({ t }); + + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + selectThreadRow(view, 0); + QApplication::processEvents(); + + auto *archive = window.findChild(QStringLiteral("archive")); + QVERIFY2(archive, "no archive action to trigger"); + archive->trigger(); + + // Read BEFORE processEvents, deliberately. This binary has no worker + // (backlog item 36), so the queued applyTagsToThreads reaches a throwaway + // database that has never heard of thread t1 and answers with + // errorOccurred, which overwrites the status bar. Draining the event loop + // here would assert on that error rather than on the scope message, and + // the test would fail against correct code. + const QString message = status->text(); + + QVERIFY2(message.contains(QStringLiteral("7")), + qPrintable(QStringLiteral("after archiving a 7-message thread the " + "status bar says '%1', which does not " + "say how much was touched") + .arg(message))); + + // And it must say the whole thread went, not merely how many messages: the + // count alone does not distinguish "7 messages you picked" from "7 messages + // because you picked their thread". + QVERIFY2(message.contains(QStringLiteral("whole thread")), + qPrintable(QStringLiteral("the status bar says '%1', which does " + "not say the action took the whole " + "thread") + .arg(message))); +} + +void TestMainWindow::anActionOnAMessageRowTagsThatMessageNotTheThread() +{ + // The routing itself, which nothing else here can see. A message row sent + // down the THREAD path produces the same undo depth and the same status + // text while tagging every sibling in the conversation: a mutation that did + // exactly that passed the entire suite, so this test exists because that + // gap was found rather than because the path looked risky. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + + ThreadSummary t = makeThread(QStringLiteral("t1"), {}); + t.totalCount = 3; + model->appendBatch({ t }); + + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("t1"); + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t1"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t1"), { root, reply }); + + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const QModelIndex threadRow = model->index(0, 0, QModelIndex()); + view->expand(threadRow); + QApplication::processEvents(); + + const QModelIndex messageRow = model->index(0, 0, threadRow); + QVERIFY(model->isMessageRow(messageRow)); + + view->selectionModel()->select( + messageRow, + QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(messageRow); + QApplication::processEvents(); + + auto *archive = window.findChild(QStringLiteral("archive")); + QVERIFY(archive); + archive->trigger(); + + // The change must carry the MESSAGE id and no thread id. Sent as a thread + // id it would archive the root and every other reply along with it. + QCOMPARE(window.pendingMessageIdsForTesting(), + QStringList{ QStringLiteral("m1@example.org") }); + QVERIFY2(window.pendingThreadIdsForTesting().isEmpty(), + qPrintable(QStringLiteral("the action was sent for thread(s) %1: a " + "message row must not tag its siblings") + .arg(window.pendingThreadIdsForTesting() + .join(QStringLiteral(", "))))); + + // And it is undoable, on its own terms rather than the thread's. + QCOMPARE(window.undoDepthForTesting(), 1); +} + void TestMainWindow::selectingAMessageRowTargetsThatMessageNotItsThread() { // test_mainwindow has no worker (backlog item 36), so this cannot assert on -- cgit v1.2.3 From 2d062241732f7396e8cb12295962d3f3e5c1a2b0 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 08:28:22 +0200 Subject: feat(model): expose the tags a reply has and its thread does not A reply card shows only these. The alternative, a reply's full tag set, was rejected on measurement rather than taste: in the user's database 7 of 48691 messages carry unread and 75 carry flagged, both already drawn another way, and every other tag is applied per thread and identical on all its messages. Full sets would repeat the thread's chips down the whole expansion, which is the striping the row-wide strip was built to avoid. --- src/threadlistmodel.cpp | 37 ++++++++++++++++++++++ src/threadlistmodel.h | 20 ++++++++++++ tests/test_threadlistmodel.cpp | 69 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 126 insertions(+) (limited to 'src/threadlistmodel.cpp') diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 9a74041..0956e6b 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -268,6 +268,37 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const return QStringList(); case PillColoursRole: return QVariantList(); + case MessageOwnTagsRole: { + // Set difference against the parent THREAD, not against a global + // list: "own" means "not already said by the card above this one". + // The parent row indexes m_threads directly, which is the same + // mapping messageAt() uses to reach this node. + const int threadRow = index.parent().row(); + const QStringList threadTags = + (threadRow >= 0 && threadRow < m_threads.size()) + ? m_threads.at(threadRow).summary.tags + : QStringList(); + QStringList own; + for (const QString &tag : node.tags) { + if (!threadTags.contains(tag)) + own.append(tag); + } + // Sorted, so a reply does not reshuffle its own chips between + // repaints, matching what PillTagsRole already guarantees. + own.sort(); + return own; + } + case MessageOwnColoursRole: { + const QStringList own = + data(index, MessageOwnTagsRole).toStringList(); + QVariantList colours; + colours.reserve(own.size()); + for (const QString &tag : own) { + colours.append(m_tagColors ? m_tagColors->colourFor(tag) + : TagColors().colourFor(tag)); + } + return colours; + } case AccountLabelRole: return QString(); case Qt::DisplayRole: @@ -344,6 +375,12 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const if (role == TagsRole) return thread.tags; + if (role == MessageOwnTagsRole) + return QStringList(); + + if (role == MessageOwnColoursRole) + return QVariantList(); + if (role == PillTagsRole || role == PillColoursRole) { // Everything the row already says another way is dropped: the account // is the chip in the subject cell, flagged is the star column, diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index b80488b..b13165c 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -103,6 +103,26 @@ public: /// delegate cannot call hasChildren without the model, and the same /// answer has to reach the cell that reserves room for the glyph. HasRepliesRole, + + /// The tags this MESSAGE carries that its thread does not. + /// + /// A reply card shows these and nothing else. Showing a reply's full + /// tag set instead was measured against the user's own database and + /// rejected: of 48691 messages, 7 carry `unread` and 75 carry + /// `flagged`, and both are already drawn another way (the sender's + /// weight, and the mark on line 2). Every other tag is applied to a + /// whole thread and is identical on all its messages, so full sets + /// would repeat the thread's own chips down the entire expansion, + /// which is the striping the old row-wide strip existed to avoid. + /// + /// Empty on a thread row, which has no thread to differ from. + MessageOwnTagsRole, + + /// The colours for MessageOwnTagsRole, in the same order. Supplied by + /// the model for the same reason as PillColoursRole: it owns the + /// TagColors instance, and a delegate reading config itself would be a + /// second source of truth. + MessageOwnColoursRole, }; /// Row fill for a thread tagged `deleted`, and for one tagged `spam`. diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index 3d131cf..4df0269 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -31,6 +31,8 @@ private slots: void rootRowsSurviveTheTreeConversion(); void repliesBecomeChildRowsUnderTheirThread(); void messageRowsShowTheirOwnSenderAndSubject(); + void replyShowsOnlyItsOwnTags(); + void replySharingEveryThreadTagShowsNone(); void reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem(); void anUnexpandedMultiMessageThreadOffersAnExpander(); void scopeFollowsTheSelectedRowKind(); @@ -1044,5 +1046,72 @@ void TestThreadListModel::attachmentColumnIsFirstAndMarksOnlyTaggedThreads() Qt::DisplayRole).toString().isEmpty()); } +void TestThreadListModel::replyShowsOnlyItsOwnTags() +{ + ThreadListModel model; + ThreadSummary thread; + thread.threadId = QStringLiteral("T1"); + thread.subject = QStringLiteral("Build fails"); + thread.totalCount = 2; + thread.tags = { QStringLiteral("inbox"), QStringLiteral("work") }; + model.appendBatch({ thread }); + + MessageNode reply; + reply.messageId = QStringLiteral("M2"); + reply.threadId = QStringLiteral("T1"); + reply.from = QStringLiteral("bob@example.org"); + reply.depth = 1; + // Two the thread already has, one it does not. + reply.tags = { QStringLiteral("inbox"), QStringLiteral("work"), + QStringLiteral("todo") }; + model.setThreadMessages(QStringLiteral("T1"), { reply }); + + const QModelIndex threadIndex = model.index(0, 0); + QVERIFY(model.hasChildren(threadIndex)); + const QModelIndex replyIndex = model.index(0, 0, threadIndex); + QVERIFY(replyIndex.isValid()); + + const QStringList own = + replyIndex.data(ThreadListModel::MessageOwnTagsRole).toStringList(); + QCOMPARE(own, QStringList{ QStringLiteral("todo") }); + + // The colours must line up with the names one for one, or the delegate + // walks the two lists together and paints a chip in another tag's colour. + const QVariantList colours = + replyIndex.data(ThreadListModel::MessageOwnColoursRole).toList(); + QCOMPARE(colours.size(), own.size()); + QVERIFY(colours.first().value().isValid()); +} + +void TestThreadListModel::replySharingEveryThreadTagShowsNone() +{ + ThreadListModel model; + ThreadSummary thread; + thread.threadId = QStringLiteral("T1"); + thread.totalCount = 2; + thread.tags = { QStringLiteral("inbox"), QStringLiteral("work") }; + model.appendBatch({ thread }); + + MessageNode reply; + reply.messageId = QStringLiteral("M2"); + reply.threadId = QStringLiteral("T1"); + reply.depth = 1; + reply.tags = { QStringLiteral("inbox"), QStringLiteral("work") }; + model.setThreadMessages(QStringLiteral("T1"), { reply }); + + const QModelIndex replyIndex = model.index(0, 0, model.index(0, 0)); + QVERIFY(replyIndex.isValid()); + QVERIFY(replyIndex.data(ThreadListModel::MessageOwnTagsRole) + .toStringList() + .isEmpty()); + + // A thread row has no thread to differ from, so it never answers these: + // its own chips come from PillTagsRole. + QVERIFY(model.index(0, 0) + .data(ThreadListModel::MessageOwnTagsRole) + .toStringList() + .isEmpty()); +} + QTEST_MAIN(TestThreadListModel) #include "test_threadlistmodel.moc" -- cgit v1.2.3 From ceaec34eb17d741536f91bad66876e94aad1c45d Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 08:35:26 +0200 Subject: refactor(model): collapse the thread list to a single column Five columns answered through Qt::DisplayRole; one column cannot, and a card needs every field at once, so each gets its own role. Qt::DisplayRole keeps answering the subject, which is what keyboard search and accessibility read. Three things change shape rather than moving. DateRole hands over the QDateTime itself, since the card decides how much of a date it has room for and a pre-formatted string takes that decision away from the delegate. The subject loses its "(3)" message-count suffix, which the reply count on line 2 now states. And the two per-column tooltips become one card-wide tooltip, because the marks no longer have columns of their own to hover. The build is red at this commit: the view and the delegates still name the deleted Column enumerators and are rewritten in the commits that follow. --- src/threadlistmodel.cpp | 144 ++++++++++++++++++----------------------- src/threadlistmodel.h | 49 ++++++-------- tests/test_threadlistmodel.cpp | 28 ++++++++ 3 files changed, 110 insertions(+), 111 deletions(-) (limited to 'src/threadlistmodel.cpp') diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 0956e6b..d8412f9 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -64,7 +64,7 @@ QString ThreadListModel::flagGlyph() // U+2605 BLACK STAR, with the same fallback reasoning as the paperclip: an // unrenderable codepoint shows as tofu, which reads as breakage rather // than as "flagged". The solid star, not the outlined U+2606, since it has - // to register at column width beside a paperclip. + // to register at small size beside a paperclip. static const QString glyph = [] { const char32_t star = 0x2605; const QString preferred = QString::fromUcs4(&star, 1); @@ -222,11 +222,13 @@ bool ThreadListModel::hasChildren(const QModelIndex &parent) const int ThreadListModel::columnCount(const QModelIndex &parent) const { - // Every level has the same columns. Returning 0 for a valid parent, as the - // table version did, would give message rows no columns at all and render - // them blank. + // One column: the card is drawn whole by CardDelegate. The five-column + // grid is what item 53 removed. + // + // Answered for a valid parent too. Returning 0 there, as the table version + // did, would give message rows no columns at all and render them blank. Q_UNUSED(parent); - return ColumnCount; + return 1; } QVariant ThreadListModel::data(const QModelIndex &index, int role) const @@ -234,7 +236,7 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const // A stale index from a view that has not caught up with a clear() can carry // any row or column, so both bounds are checked rather than trusted. if (!index.isValid() || index.row() < 0 - || index.column() < 0 || index.column() >= ColumnCount) { + || index.column() != 0) { return {}; } @@ -302,23 +304,23 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const case AccountLabelRole: return QString(); case Qt::DisplayRole: - switch (index.column()) { - case AuthorsColumn: - // The REPLY's sender, not the thread's author summary. Reading - // the thread's fields here would look almost right, since the - // first sender usually appears in both. - return node.from; - case SubjectColumn: - return node.subject; - case DateColumn: - return node.date.toString(QStringLiteral("yyyy-MM-dd hh:mm")); - case AttachmentColumn: - return node.hasAttachment() ? attachmentGlyph() : QString(); - case FlagColumn: - return node.isFlagged() ? flagGlyph() : QString(); - default: - return {}; - } + case SubjectRole: + return node.subject; + case SendersRole: + // The REPLY's sender, not the thread's author summary. Reading the + // thread's fields here would look almost right, since the first + // sender usually appears in both. + return node.from; + case DateRole: + return node.date; + case HasAttachmentRole: + return node.hasAttachment(); + case IsFlaggedRole: + return node.isFlagged(); + case ReplyCountRole: + // A reply never offers an expander: nesting past the first level is + // drawn from depth, not from further parent-child structure. + return 0; case Qt::BackgroundRole: // Tinted, so an expanded thread reads as one block rather than as // more table rows. Applied per cell here; ThreadListView fills the @@ -437,46 +439,42 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const return {}; } - if (role == Qt::ToolTipRole && index.column() == AttachmentColumn) - return thread.hasAttachment() ? tr("Has an attachment") : QVariant(); - - // "Important", matching the action's own wording (item 57). The underlying - // tag is still `flagged` and isFlagged() still tests for it; only what the - // user reads changed. - if (role == Qt::ToolTipRole && index.column() == FlagColumn) - return thread.isFlagged() ? tr("Important") : QVariant(); - - // Both marker columns: a glyph reads as a marker only when it sits in the - // middle of its column rather than against the text beside it. - if (role == Qt::TextAlignmentRole - && (index.column() == AttachmentColumn || index.column() == FlagColumn)) { - return QVariant::fromValue(Qt::AlignCenter); + if (role == Qt::ToolTipRole) { + // One tooltip for the whole card, since the marks no longer have + // columns of their own to be hovered separately. "Important" matches + // the action's own wording (item 57); the underlying tag is still + // `flagged` and isFlagged() still tests for it. + QStringList marks; + if (thread.isFlagged()) + marks.append(tr("Important")); + if (thread.hasAttachment()) + marks.append(tr("Has an attachment")); + return marks.isEmpty() ? QVariant() : marks.join(QStringLiteral(", ")); } - if (role == Qt::DisplayRole) { - switch (index.column()) { - case AttachmentColumn: - // A glyph rather than an icon resource: no new asset to ship, and - // it inherits the row's font, so it strikes through with a doomed - // thread like every other cell. - return thread.hasAttachment() ? attachmentGlyph() : QString(); - case FlagColumn: - // A glyph rather than an icon, for the same reasons as the - // paperclip: no asset to ship, and it inherits the row's font so - // it strikes through with a doomed thread. - return thread.isFlagged() ? flagGlyph() : QString(); - case DateColumn: - return thread.date.toString(QStringLiteral("yyyy-MM-dd hh:mm")); - case AuthorsColumn: - return thread.authors; - case SubjectColumn: - return thread.totalCount > 1 - ? QStringLiteral("%1 (%2)").arg(thread.subject) - .arg(thread.totalCount) - : thread.subject; - default: - return {}; - } + switch (role) { + case Qt::DisplayRole: + case SubjectRole: + // Bare, with no "(3)" message-count suffix. The count is drawn on the + // card's second line as the expander, so a suffix here would state it + // twice on the same card. + return thread.subject; + case SendersRole: + return thread.authors; + case DateRole: + // The QDateTime itself. Formatting belongs to the delegate now: the + // card decides how much of a date it has room for, and a pre-formatted + // string takes that decision away from it. + return thread.date; + case HasAttachmentRole: + return thread.hasAttachment(); + case IsFlaggedRole: + return thread.isFlagged(); + case ReplyCountRole: + // totalCount includes the root message, which is the card itself. + return qMax(0, thread.totalCount - 1); + default: + break; } // A thread tagged deleted or spam is on its way out, and the user needs to @@ -534,24 +532,6 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const return {}; } -QVariant ThreadListModel::headerData(int section, Qt::Orientation orientation, - int role) const -{ - if (orientation != Qt::Horizontal || role != Qt::DisplayRole) - return {}; - - switch (section) { - // No label: any text would set a minimum width far wider than the icon, - // which defeats the point of a narrow column. - case AttachmentColumn: return QString(); - case FlagColumn: return QString(); - case DateColumn: return tr("Date"); - case AuthorsColumn: return tr("From"); - case SubjectColumn: return tr("Subject"); - default: return {}; - } -} - void ThreadListModel::appendBatch(const QVector &batch) { // beginInsertRows with an empty range violates Qt's contract, so the guard @@ -720,9 +700,9 @@ void ThreadListModel::applyTagChange(const QString &threadId, tags.append(tag); } - // The whole row repaints: unread state drives the font of every column, - // not just the tags one. - emit dataChanged(index(row, 0), index(row, ColumnCount - 1)); + // The whole card repaints: unread state drives its font, and the tags + // it draws on line 3 have just changed. + emit dataChanged(index(row, 0), index(row, 0)); return; } } diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index b13165c..9cb774e 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -38,26 +38,6 @@ class ThreadListModel : public QAbstractItemModel { Q_OBJECT public: - /// No tags column: spelling out a dozen tags per row cost most of the - /// list's width and was unreadable. Functional tags moved to a chip strip - /// under the message pane, and the account tag renders as a chip in front - /// of the subject. - enum Column { - /// A paperclip when the thread has an attachment, so it is visible - /// without opening the thread. Icon only and deliberately narrow; - /// it carries no text. - AttachmentColumn = 0, - - /// A star when the thread carries the flagged tag. Beside the - /// paperclip and the same shape: icon only, narrow, no text. - FlagColumn, - - DateColumn, - AuthorsColumn, - SubjectColumn, - ColumnCount, - }; - enum Role { /// The thread id behind a row. Views hand out QModelIndexes, but the /// worker speaks thread ids, so the mapping belongs on the model @@ -123,20 +103,33 @@ public: /// TagColors instance, and a delegate reading config itself would be a /// second source of truth. MessageOwnColoursRole, + + /// The card's own fields, by role rather than by column. + /// + /// Five columns used to answer these through Qt::DisplayRole. One + /// column cannot, and a card needs all five values at once, so each + /// gets a role and Qt::DisplayRole answers the subject alone (which is + /// what keyboard search and accessibility read). + SubjectRole, + SendersRole, + DateRole, ///< A QDateTime. The delegate formats it. + HasAttachmentRole, ///< bool + IsFlaggedRole, ///< bool + ReplyCountRole, ///< int; 0 when a thread has no replies. }; - /// Row fill for a thread tagged `deleted`, and for one tagged `spam`. - /// Muted rather than saturated: a bulk delete paints every selected row, - /// and a wall of pure red is harder to read than the list it replaces. - /// Exposed so a test names the same colour the model uses. - /// The character shown in AttachmentColumn for a thread that has one. - /// A paperclip when the system font can draw it, "*" otherwise. + /// The mark drawn on a card's second line when the message has an + /// attachment. A paperclip when the system font can draw it, "*" otherwise. static QString attachmentGlyph(); - /// The character shown in FlagColumn for a flagged thread. + /// The mark drawn on a card's second line when the message is flagged. /// A star when the system font can draw it, "*" otherwise. static QString flagGlyph(); + /// Row fill for a thread tagged `deleted`, and for one tagged `spam`. + /// Muted rather than saturated: a bulk delete paints every selected row, + /// and a wall of pure red is harder to read than the list it replaces. + /// Exposed so a test names the same colour the model uses. static QColor deletedColour(); static QColor spamColour(); @@ -183,8 +176,6 @@ public: /// nothing. bool hasChildren(const QModelIndex &parent = {}) const override; QVariant data(const QModelIndex &index, int role) const override; - QVariant headerData(int section, Qt::Orientation orientation, - int role) const override; void appendBatch(const QVector &batch); void clear(); diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index 4df0269..2e3eede 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -31,6 +31,7 @@ private slots: void rootRowsSurviveTheTreeConversion(); void repliesBecomeChildRowsUnderTheirThread(); void messageRowsShowTheirOwnSenderAndSubject(); + void modelHasOneColumn(); void replyShowsOnlyItsOwnTags(); void replySharingEveryThreadTagShowsNone(); void reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem(); @@ -1046,6 +1047,33 @@ void TestThreadListModel::attachmentColumnIsFirstAndMarksOnlyTaggedThreads() Qt::DisplayRole).toString().isEmpty()); } +void TestThreadListModel::modelHasOneColumn() +{ + ThreadListModel model; + ThreadSummary thread; + thread.threadId = QStringLiteral("T1"); + thread.subject = QStringLiteral("Build fails"); + thread.authors = QStringLiteral("alice@example.org"); + thread.date = QDateTime::currentDateTime(); + thread.totalCount = 1; + model.appendBatch({ thread }); + + QCOMPARE(model.columnCount(), 1); + + // Every field the five columns used to answer is still reachable, by role + // rather than by column, because the card draws them all. + const QModelIndex index = model.index(0, 0); + QCOMPARE(index.data(ThreadListModel::SubjectRole).toString(), + QStringLiteral("Build fails")); + QCOMPARE(index.data(ThreadListModel::SendersRole).toString(), + QStringLiteral("alice@example.org")); + QVERIFY(index.data(ThreadListModel::DateRole).toDateTime().isValid()); + + // A single-message thread offers no expander: totalCount includes the root + // message, which is the card itself. + QCOMPARE(index.data(ThreadListModel::ReplyCountRole).toInt(), 0); +} + void TestThreadListModel::replyShowsOnlyItsOwnTags() { ThreadListModel model; -- cgit v1.2.3 From 93e6a533f4b0cc1a75000b2f8c77181dfc56e199 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 09:23:50 +0200 Subject: fix(ui): expand flat threads, reach the first message, localise the date Four faults from the first hand test, two of them behavioural. An expander that opened onto nothing. setThreadMessages kept only nodes with depth > 0, and notmuch_thread_get_toplevel_messages returns every message at depth 0 when a thread carries no usable In-Reply-To, so a flat thread contributed no children while its card still advertised the count. Measured in the user's database: of 396 inbox threads three are flat, one of them nine messages long, and every two-message thread of that kind was affected, which is exactly why the fault looked like "the expander only works with more than one reply". The rule is now position, not depth: every message except the first, which is the root card itself. That is also the correct rule rather than a workaround, since the row under the root is the second message however notmuch chose to nest it. The thread's first message was unreachable. Selecting a root card loaded the whole thread, so the pane showed every message with only the last expanded, and no row in the list offered the first one: the reply rows are messages two onward. The root card now renders its own message, which is what the card already claims to be. It keeps its thread id, unlike the message-row path, so mark-read and the tag-change repaint still work; that is asserted, because clearing it is the obvious way to write this and silently disables both. Before the replies are loaded the model has no first message to name and the whole thread stays the honest answer. Dates ignored the locale. One hardcoded "yyyy-MM-dd hh:mm" produced a US-looking format on an Italian desktop; QLocale::system() now formats it, and the width reserved for the date comes from the same function so a longer locale cannot clip. The expander was a bare number on the card's own background. It is a pill now, carrying "3 replies" (and "1 reply", singular), sized from the label actually drawn and measured in both glyph states so it does not resize under the pointer on click. Its fill is blended from Text toward Base rather than taken from QPalette::Button, which is #2b2b2b against a Base of #2b2b2b on the user's theme: byte identical, so the pill was invisible. A theme may make any two roles equal; a blend is defined against the surface it sits on and cannot collide with it. Checked by rendering both a dark and a light palette and looking. --- src/carddelegate.cpp | 53 ++++++++++++++++++++---- src/cardlayout.cpp | 58 +++++++++++++++++++++++--- src/cardlayout.h | 31 ++++++++++++++ src/mainwindow.cpp | 23 ++++++++++- src/threadlistmodel.cpp | 37 +++++++++++++---- src/threadlistmodel.h | 9 ++++ tests/test_cardlayout.cpp | 75 ++++++++++++++++++++++++++++++++++ tests/test_mainwindow.cpp | 39 +++++++++++++++++- tests/test_threadlistmodel.cpp | 93 +++++++++++++++++++++++++++++++++++++++--- 9 files changed, 388 insertions(+), 30 deletions(-) (limited to 'src/threadlistmodel.cpp') diff --git a/src/carddelegate.cpp b/src/carddelegate.cpp index a3a846d..f13205e 100644 --- a/src/carddelegate.cpp +++ b/src/carddelegate.cpp @@ -157,7 +157,7 @@ void CardDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, const QDateTime date = index.data(ThreadListModel::DateRole).toDateTime(); painter->drawText(card.dateRect, Qt::AlignVCenter | Qt::AlignRight, - date.toString(QStringLiteral("yyyy-MM-dd hh:mm"))); + CardLayout::formatDate(date)); // Line 2: the flag mark, the subject, the attachment mark. QString subject = index.data(ThreadListModel::SubjectRole).toString(); @@ -178,16 +178,51 @@ void CardDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, metrics.elidedText(line2, Qt::ElideRight, card.subjectRect.width())); - // The reply count, which is also the expander. + // The reply count, which is also the expander, drawn as a PILL. + // + // A bare "3" on the card's own background read as an unexplained number + // beside the subject and gave no hint that it could be clicked. The chip + // shape says "this is a control", matching the tag chips on line 3, and the + // word says what the number counts. if (!card.expanderRect.isEmpty()) { - painter->setFont(CardLayout::smallFont(chrome.font)); const int count = index.data(ThreadListModel::ReplyCountRole).toInt(); - const QString glyph = (option.state & QStyle::State_Open) - ? QStringLiteral("▾") - : QStringLiteral("▸"); - painter->drawText(card.expanderRect, Qt::AlignVCenter | Qt::AlignRight, - QStringLiteral("%1 %2").arg(glyph).arg(count)); - painter->setFont(chrome.font); + const QString label = CardLayout::expanderLabel( + count, option.state & QStyle::State_Open); + + painter->save(); + painter->setFont(CardLayout::smallFont(chrome.font)); + + // Blended from Text toward Base rather than taken from a palette ROLE. + // QPalette::Button is the role this obviously wants and it is + // #2b2b2b against a Base of #2b2b2b on the user's theme: byte + // identical, so the pill was invisible. A theme is free to make any two + // roles equal, and several do; a blend cannot collide with the surface + // it sits on because it is defined relative to it. + // + // Toward Text, so it darkens on a light theme and lightens on a dark + // one, the same trick replyBackground() and threadLineColour() use. + const QColor base = option.palette.color(QPalette::Base); + const QColor text = option.palette.color(QPalette::Text); + constexpr qreal kFillWeight = 0.18; + const QColor fill = QColor::fromRgbF( + text.redF() * kFillWeight + base.redF() * (1.0 - kFillWeight), + text.greenF() * kFillWeight + base.greenF() * (1.0 - kFillWeight), + text.blueF() * kFillWeight + base.blueF() * (1.0 - kFillWeight)); + painter->setRenderHint(QPainter::Antialiasing, true); + painter->setPen(Qt::NoPen); + painter->setBrush(fill); + // Fully rounded ends, the same shape TagChip paints: the radius is half + // the height, so the pill cannot look like a rectangle with soft corners. + const qreal radius = card.expanderRect.height() / 2.0; + painter->drawRoundedRect(card.expanderRect, radius, radius); + + // The pen is restored from the card's own text colour rather than + // ButtonText, which belongs to the role that just proved unreliable. + painter->setPen(option.state & QStyle::State_Selected + ? option.palette.highlightedText().color() + : text); + painter->drawText(card.expanderRect, Qt::AlignCenter, label); + painter->restore(); } painter->restore(); diff --git a/src/cardlayout.cpp b/src/cardlayout.cpp index 1a79e3b..0e118ab 100644 --- a/src/cardlayout.cpp +++ b/src/cardlayout.cpp @@ -19,6 +19,44 @@ #include "cardlayout.h" #include +#include + +QString CardLayout::formatDate(const QDateTime &date) +{ + // The system locale's own short format, not a hardcoded pattern: an + // Italian desktop writes 10/08/2025, not 2025-08-10, and a mail client + // that disagrees with every other application on screen is simply wrong. + return QLocale::system().toString(date, QLocale::ShortFormat); +} + +QString CardLayout::expanderLabel(int replyCount, bool expanded) +{ + // "3 replies", not a bare "3". The count alone reads as an unexplained + // number beside the subject, and the word is what says the card opens. + // + // Not translated through tr() here because CardLayout is a plain struct + // rather than a QObject; the delegate is where a translated build would + // wrap this, and the string is deliberately kept in one place so there is + // exactly one thing to change. + const QString glyph = expanded ? QStringLiteral("\u25be") + : QStringLiteral("\u25b8"); + const QString word = replyCount == 1 ? QStringLiteral("reply") + : QStringLiteral("replies"); + return QStringLiteral("%1 %2 %3").arg(glyph).arg(replyCount).arg(word); +} + +QString CardLayout::widestDateSample() +{ + // A real date run through the same formatter, with the wide digits and a + // two-digit day and month, so the reserved width matches what is drawn + // whatever the locale's pattern turns out to be. Guessing a pattern here + // would reintroduce the clipping this exists to prevent. + static const QString sample = [] { + const QDateTime wide(QDate(2028, 12, 28), QTime(22, 58)); + return formatDate(wide); + }(); + return sample; +} QFont CardLayout::smallFont(const QFont &cardFont) { @@ -96,8 +134,8 @@ CardLayout CardLayout::compute(const Input &input, const QRect &rect, // painted. QFont dateFont = font; dateFont.setBold(true); - const int dateWidth = QFontMetrics(dateFont).horizontalAdvance( - QStringLiteral("8888-88-88 88:88")); + const int dateWidth = + QFontMetrics(dateFont).horizontalAdvance(widestDateSample()); out.dateRect = QRect(right - dateWidth, lineOneTop, dateWidth, metrics.height()); out.senderRect = QRect(out.contentLeft, lineOneTop, @@ -105,10 +143,20 @@ CardLayout CardLayout::compute(const Input &input, const QRect &rect, - kPaddingX), metrics.height()); - // The expander is the reply count, on line two and on the right. + // The expander is the reply count as a PILL, on line two and on the right. + // + // Sized from the label actually drawn rather than from a fixed sample, so + // the background and the text inside it cannot disagree. Both states of the + // glyph are measured because the rect must not change width when the card + // is expanded: a pill that resized on click would shift the subject's + // elision under the pointer. if (input.replyCount > 0) { - const int countWidth = smallMetrics.horizontalAdvance( - QStringLiteral("▾ 8888 replies")); + const int collapsed = smallMetrics.horizontalAdvance( + expanderLabel(input.replyCount, false)); + const int expanded = smallMetrics.horizontalAdvance( + expanderLabel(input.replyCount, true)); + const int countWidth = + qMax(collapsed, expanded) + kPillPaddingX * 2; out.expanderRect = QRect(right - countWidth, lineTwoTop, countWidth, metrics.height()); } diff --git a/src/cardlayout.h b/src/cardlayout.h index d06ed92..ef6a563 100644 --- a/src/cardlayout.h +++ b/src/cardlayout.h @@ -18,6 +18,7 @@ #pragma once +#include #include #include #include @@ -116,4 +117,34 @@ struct CardLayout static CardLayout compute(const Input &input, const QRect &rect, const QFont &font); + + /// How a card writes a date, in the user's own locale. + /// + /// Never a hardcoded pattern. "yyyy-MM-dd hh:mm" is a US-looking format + /// that an Italian desktop does not use, and the whole point of asking the + /// system locale is that the user reads dates the way their desktop writes + /// them everywhere else. + /// + /// Shared with the layout so the width reserved for the date and the text + /// drawn into it come from one place: a locale whose short format is + /// longer than the reserved rect would clip, which is exactly the fault + /// bold text produced. + static QString formatDate(const QDateTime &date); + + /// The widest string formatDate() can return, for reserving space. + static QString widestDateSample(); + + /// The expander's label: the reply count with its glyph, as drawn. + /// + /// Shared with the layout for the same reason as formatDate: the rect + /// reserved for the pill and the text put inside it must come from one + /// place, or a count wider than the sample the layout guessed at spills + /// out of its own background. + /// + /// `expanded` chooses which way the triangle points. + static QString expanderLabel(int replyCount, bool expanded); + + /// Padding inside the expander pill, matching a tag chip's, so the two read + /// as the same kind of object on the card. + static constexpr int kPillPaddingX = 8; }; diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 5881fae..b7efec5 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1755,10 +1755,31 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, } const ThreadSummary thread = m_model->threadAt(current.row()); - m_currentMessageId.clear(); m_currentThreadId = thread.threadId; m_messageView->setTags(thread.tags); 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. + // + // 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. + 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)); + return; + } + + m_currentMessageId.clear(); QMetaObject::invokeMethod(m_worker, "loadThread", Qt::QueuedConnection, Q_ARG(QString, m_currentThreadId), Q_ARG(QString, m_lastQuery), diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index d8412f9..bdc7e96 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -365,8 +365,13 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const if (role == IsMessageRole) return false; - if (role == MessageIdRole) - return QString(); + 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; + } if (role == MessageDepthRole) return 0; @@ -542,7 +547,7 @@ void ThreadListModel::appendBatch(const QVector &batch) const int first = m_threads.size(); beginInsertRows({}, first, first + batch.size() - 1); for (const ThreadSummary &summary : batch) - m_threads.append(ThreadNode{ summary, {}, false }); + m_threads.append(ThreadNode{ summary, {}, {}, false }); endInsertRows(); } @@ -570,12 +575,26 @@ void ThreadListModel::setThreadMessages(const QString &threadId, endRemoveRows(); } - QVector children; - children.reserve(nodes.size()); - for (const MessageNode &node : nodes) { - if (node.depth > 0) - children.append(node); - } + // Every message EXCEPT the first, which is the root card itself. + // + // Selecting on depth > 0 instead was wrong, and wrong in a way that + // only showed on real mail: notmuch_thread_get_toplevel_messages + // returns every message at depth 0 when a thread carries no usable + // In-Reply-To, so a flat thread contributed no children at all. The + // card advertised "3 replies" and expanded onto nothing. Measured in + // the user's database: of 396 inbox threads, three are flat, one of + // them nine messages long, and every two-message thread of this kind + // was affected, which is why the fault looked like "the expander only + // works with more than one reply". + // + // Position also happens to be the right rule rather than a workaround. + // The root card IS the thread's first message, so the row under it is + // the second message whatever depth notmuch assigns it. + QVector children = nodes.mid(1); + + // Kept so the root card can render its own message. It is the card the + // user clicks to read the thread's opening message. + m_threads[row].first = nodes.isEmpty() ? MessageNode() : nodes.first(); if (!children.isEmpty()) { beginInsertRows(parent, 0, children.size() - 1); diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index 9cb774e..f381ffa 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -240,6 +240,15 @@ private: ThreadSummary summary; QVector children; ///< Empty until the thread is expanded. + /// The thread's FIRST message, which the root card itself draws. + /// + /// Kept because the root card is that message: selecting it must + /// render one message rather than the whole conversation, and without + /// this the first message of every thread is unreachable, since the + /// only rows offering a message are the replies and it is not one of + /// them. Empty until the replies are loaded. + MessageNode first; + /// Distinguishes "this thread has no replies" from "its replies have /// not been asked for yet". Without it an expander would be drawn over /// every thread, including the ones that turn out to be single diff --git a/tests/test_cardlayout.cpp b/tests/test_cardlayout.cpp index c38f728..48bba25 100644 --- a/tests/test_cardlayout.cpp +++ b/tests/test_cardlayout.cpp @@ -19,6 +19,7 @@ #include "cardlayout.h" #include +#include #include class TestCardLayout : public QObject @@ -32,10 +33,12 @@ private slots: void indentStopsAtTheCap(); void expanderSitsOnTheSecondLine(); void expanderIsEmptyWithoutReplies(); + void theExpanderReadsAsAPillWithAWord(); void dateIsFlushRight(); void threadCardCarriesAnAccentBar(); void replyCardCarriesNoAccentBar(); void theDateFitsWhenTheCardIsBold(); + void theDateFollowsTheSystemLocale(); }; namespace { @@ -183,6 +186,42 @@ void TestCardLayout::expanderIsEmptyWithoutReplies() QVERIFY(card.expanderRect.isEmpty()); } +void TestCardLayout::theExpanderReadsAsAPillWithAWord() +{ + // A bare "3" beside the subject reads as an unexplained number and gives + // no hint that it can be clicked. The label carries the word, and the rect + // carries padding for the pill drawn behind it. + QCOMPARE(CardLayout::expanderLabel(3, false), + QStringLiteral("\u25b8 3 replies")); + QCOMPARE(CardLayout::expanderLabel(3, true), + QStringLiteral("\u25be 3 replies")); + + // Singular, because "1 replies" is the kind of detail that makes an + // interface look unfinished. + QCOMPARE(CardLayout::expanderLabel(1, false), + QStringLiteral("\u25b8 1 reply")); + + const QFont font; + const int h = CardLayout::heightFor(font); + const CardLayout card = + CardLayout::compute(threadInput(), QRect(0, 0, 400, h), font); + const QFontMetrics small(CardLayout::smallFont(font)); + + // The rect must hold the label AND its padding, or the pill's background + // is narrower than the text sitting on it. + QVERIFY2(card.expanderRect.width() + >= small.horizontalAdvance(CardLayout::expanderLabel(3, false)) + + CardLayout::kPillPaddingX * 2, + "the expander rect is too narrow for its own label and padding"); + + // And it must NOT change width when the card opens: a pill that resized on + // click would shift the subject's elision under the pointer. + CardLayout::Input open = threadInput(); + const CardLayout expanded = + CardLayout::compute(open, QRect(0, 0, 400, h), font); + QCOMPARE(expanded.expanderRect.width(), card.expanderRect.width()); +} + void TestCardLayout::dateIsFlushRight() { const QFont font; @@ -232,6 +271,42 @@ void TestCardLayout::replyCardCarriesNoAccentBar() QCOMPARE(reply.spines.size(), 1); } +void TestCardLayout::theDateFollowsTheSystemLocale() +{ + const QDateTime when(QDate(2025, 8, 10), QTime(6, 26)); + + // The system locale's own rendering, whatever it is. Asserting a specific + // string would only restate the hardcoded pattern this replaced, and would + // fail on any machine but the one that wrote it. + QCOMPARE(CardLayout::formatDate(when), + QLocale::system().toString(when, QLocale::ShortFormat)); + + // The specific fault: an ISO-looking pattern on a desktop that does not + // use one. Guarded so this test says nothing on a locale that genuinely + // formats that way. + if (QLocale::system().toString(when, QLocale::ShortFormat) + != QStringLiteral("2025-08-10 06:26")) { + QVERIFY2(CardLayout::formatDate(when) + != QStringLiteral("2025-08-10 06:26"), + "the date is hardcoded to yyyy-MM-dd hh:mm rather than " + "following the desktop's locale"); + } + + // And the reserved width has to follow the same formatter, or a locale + // whose dates are longer clips them exactly as the bold font did. + QFont font; + const int h = CardLayout::heightFor(font); + const CardLayout card = + CardLayout::compute(threadInput(), QRect(0, 0, 400, h), font); + QFont bold = font; + bold.setBold(true); + QVERIFY2(card.dateRect.width() + >= QFontMetrics(bold).horizontalAdvance( + CardLayout::formatDate(when)), + "the reserved date width is narrower than this locale's own " + "formatting of a date"); +} + void TestCardLayout::theDateFitsWhenTheCardIsBold() { // An UNREAD card draws BOLD, and bold is wider. The layout is computed from diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index cdb08ca..922705c 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -105,6 +105,7 @@ private slots: void childRowsAreIndentedUnderTheirThread(); void aThreadWithRepliesDrawsAVisibleExpander(); void cardsNeverScrollSideways(); + void selectingARootCardKeepsItsThreadForMarkRead(); void nextThreadLeavesTheLastReply(); void altDownSkipsReplies(); void bothThreadStepBindingsReachTheAction(); @@ -667,8 +668,12 @@ NavFixture buildNavFixture(MainWindow &window) f.view = window.findChild(); f.model = window.findChild(); + // unread, so a selection arms the mark-read timer: scheduleMarkRead() + // returns early for a thread that is already read, and a fixture without + // it would make a mark-read assertion pass for the wrong reason. ThreadSummary first = makeThread(QStringLiteral("T1"), - QStringList{ QStringLiteral("inbox") }); + QStringList{ QStringLiteral("inbox"), + QStringLiteral("unread") }); first.totalCount = 2; ThreadSummary second = makeThread(QStringLiteral("T2"), QStringList{ QStringLiteral("inbox") }); @@ -693,6 +698,38 @@ NavFixture buildNavFixture(MainWindow &window) } // namespace +void TestMainWindow::selectingARootCardKeepsItsThreadForMarkRead() +{ + // A root card is BOTH a message and a thread: it renders the thread's + // first message, and it is still the thread that gets marked read and + // repainted on a tag change. The message-row path deliberately clears the + // current thread id; doing that here too would silently disable mark-read + // and the tag-change repaint for every thread root in the list. + const Config config; + MainWindow window(config); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const NavFixture f = buildNavFixture(window); + f.view->setCurrentIndex(f.root); + QApplication::processEvents(); + + // Guard: the fixture loads replies, so the root knows its own message and + // the branch under test is the one that runs. + QVERIFY2(!f.model->data(f.root, ThreadListModel::MessageIdRole) + .toString().isEmpty(), + "the root card does not know its first message, so this exercises " + "the fallback rather than the path it is written for"); + + // A mark-read timer armed for the thread is what proves the thread id + // survived: scheduleMarkRead() is only reached on the thread-row path. + auto *timer = window.findChild(QStringLiteral("markReadTimer")); + QVERIFY(timer); + QVERIFY2(timer->isActive(), + "no mark-read timer for a selected root card: its thread id was " + "cleared along with the switch to rendering one message"); +} + void TestMainWindow::nextThreadLeavesTheLastReply() { const Config config; diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index 49b8894..aa71080 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -32,6 +32,8 @@ private slots: void repliesBecomeChildRowsUnderTheirThread(); void messageRowsShowTheirOwnSenderAndSubject(); void modelHasOneColumn(); + void aFlatThreadStillListsItsReplies(); + void theRootCardKnowsItsOwnMessage(); void replyShowsOnlyItsOwnTags(); void replySharingEveryThreadTagShowsNone(); void reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem(); @@ -131,10 +133,13 @@ void TestThreadListModel::repliesBecomeChildRowsUnderTheirThread() QCOMPARE(model.data(child, ThreadListModel::ThreadIdRole).toString(), QStringLiteral("t1")); - // A thread root is not a message row and carries no message id. + // A thread root is not a message ROW, but it does carry a message id: the + // root card is the thread's first message, and selecting it renders that + // message alone. It used to answer nothing here, which is what made the + // first message of every thread unreachable. QVERIFY(!model.data(root, ThreadListModel::IsMessageRole).toBool()); - QVERIFY(model.data(root, ThreadListModel::MessageIdRole) - .toString().isEmpty()); + QCOMPARE(model.data(root, ThreadListModel::MessageIdRole).toString(), + QStringLiteral("m0@example.org")); QAbstractItemModelTester tester( &model, QAbstractItemModelTester::FailureReportingMode::Warning); @@ -1022,6 +1027,72 @@ void TestThreadListModel::modelHasOneColumn() QCOMPARE(index.data(ThreadListModel::ReplyCountRole).toInt(), 0); } +void TestThreadListModel::aFlatThreadStillListsItsReplies() +{ + // A thread whose messages carry no reply structure: notmuch returns them + // all from get_toplevel_messages at depth 0, which is what happens when the + // mail has no usable In-Reply-To. Measured in the user's own database: + // of 396 inbox threads, three are like this, one of them nine messages + // deep, and every one of them showed a reply count that expanded to + // nothing because the model kept only nodes with depth > 0. + ThreadListModel model; + ThreadSummary thread = makeThread(QStringLiteral("t1"), + QStringLiteral("flat thread")); + thread.totalCount = 3; + model.appendBatch({ thread }); + + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 0), + makeNode(QStringLiteral("m2@example.org"), 0) }); + + const QModelIndex root = model.index(0, 0); + + // Two children, not zero: the FIRST message is the root card itself, and + // the rest are its replies however flat the thread is. + QCOMPARE(model.rowCount(root), 2); + QCOMPARE(model.index(0, 0, root).data(ThreadListModel::MessageIdRole) + .toString(), + QStringLiteral("m1@example.org")); + + // And the count the card advertises must agree with the rows beneath it, + // or the expander opens onto nothing. + QCOMPARE(root.data(ThreadListModel::ReplyCountRole).toInt(), + model.rowCount(root)); +} + +void TestThreadListModel::theRootCardKnowsItsOwnMessage() +{ + // The root card IS the thread's first message, so it has to be able to say + // which message that is. Without this the pane renders the whole thread + // when the root is selected, and the first message is unreachable: the + // only rows offering it are the replies, and it is not one of them. + ThreadListModel model; + ThreadSummary thread = makeThread(QStringLiteral("t1"), + QStringLiteral("a subject")); + thread.totalCount = 2; + model.appendBatch({ thread }); + + const QModelIndex root = model.index(0, 0); + + // Before the replies are loaded there is nothing to report, and the caller + // must fall back to loading the whole thread rather than a wrong message. + QVERIFY(root.data(ThreadListModel::MessageIdRole).toString().isEmpty()); + + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) }); + + QCOMPARE(root.data(ThreadListModel::MessageIdRole).toString(), + QStringLiteral("m0@example.org")); + + // And it is the FIRST message, not just any of them: the reply must still + // report its own. + QCOMPARE(model.index(0, 0, root).data(ThreadListModel::MessageIdRole) + .toString(), + QStringLiteral("m1@example.org")); +} + void TestThreadListModel::replyShowsOnlyItsOwnTags() { ThreadListModel model; @@ -1040,7 +1111,14 @@ void TestThreadListModel::replyShowsOnlyItsOwnTags() // Two the thread already has, one it does not. reply.tags = { QStringLiteral("inbox"), QStringLiteral("work"), QStringLiteral("todo") }; - model.setThreadMessages(QStringLiteral("T1"), { reply }); + + // Led by the thread's FIRST message, which is what the worker sends and + // what the root card draws. setThreadMessages drops it by position. + MessageNode root; + root.messageId = QStringLiteral("M1"); + root.threadId = QStringLiteral("T1"); + root.depth = 0; + model.setThreadMessages(QStringLiteral("T1"), { root, reply }); const QModelIndex threadIndex = model.index(0, 0); QVERIFY(model.hasChildren(threadIndex)); @@ -1073,7 +1151,12 @@ void TestThreadListModel::replySharingEveryThreadTagShowsNone() reply.threadId = QStringLiteral("T1"); reply.depth = 1; reply.tags = { QStringLiteral("inbox"), QStringLiteral("work") }; - model.setThreadMessages(QStringLiteral("T1"), { reply }); + + MessageNode root; + root.messageId = QStringLiteral("M1"); + root.threadId = QStringLiteral("T1"); + root.depth = 0; + model.setThreadMessages(QStringLiteral("T1"), { root, reply }); const QModelIndex replyIndex = model.index(0, 0, model.index(0, 0)); QVERIFY(replyIndex.isValid()); -- cgit v1.2.3