From 02d218358347e5d7a29164bc70a346c57fe098ab 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 ++++++++++++++++++++++++++++++++++++------ src/threadlistmodel.h | 37 +++++++++++++++++--- tests/test_threadlistmodel.cpp | 34 ++++++++++++++++++ 3 files changed, 136 insertions(+), 14 deletions(-) diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index c675488..dabc447 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; @@ -307,7 +364,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(); } @@ -322,13 +380,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) { @@ -348,10 +407,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) { diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index 2eaa88e..1aa0271 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -18,16 +18,23 @@ #pragma once -#include +#include #include #include #include "tagcolors.h" #include "types.h" -/// Table model over query results, filled in batches so a large query paints +/// Tree model over query results, filled in batches so a large query paints /// its first screenful immediately. -class ThreadListModel : public QAbstractTableModel +/// +/// A tree rather than a table since item 20: a thread's replies are child rows +/// under it. The tree is at most two levels deep in the MODEL (a thread, then +/// its messages) even though the messages carry a reply depth of their own; the +/// visual nesting beyond the first level comes from that depth, not from +/// further parent-child structure. A deeper model would buy nothing and make +/// every index calculation recursive. +class ThreadListModel : public QAbstractItemModel { Q_OBJECT public: @@ -109,6 +116,10 @@ public: /// Without one, chips fall back to a colour generated from the tag name. void setTagColors(const TagColors *colours) { m_tagColors = colours; } + QModelIndex index(int row, int column, + const QModelIndex &parent = {}) const override; + QModelIndex parent(const QModelIndex &child) const override; + int rowCount(const QModelIndex &parent = {}) const override; int columnCount(const QModelIndex &parent = {}) const override; QVariant data(const QModelIndex &index, int role) const override; @@ -137,6 +148,24 @@ public: const QStringList &removed); private: - QVector m_threads; + /// One thread root and the message rows expanded under it. + /// + /// Children live beside the summary rather than in a separate map keyed by + /// thread id, so a row and its expansion are appended, cleared and + /// destroyed together. The model is rebuilt wholesale on every query, so + /// nothing here has to survive a reset. + struct ThreadNode + { + ThreadSummary summary; + QVector children; ///< Empty until the thread is expanded. + + /// 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 + /// messages. + bool loaded = false; + }; + + QVector m_threads; const TagColors *m_tagColors = nullptr; }; diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index e2ca09f..9684637 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -28,6 +28,7 @@ class TestThreadListModel : public QObject Q_OBJECT private slots: void messageNodeHoldsDisplayFacts(); + void rootRowsSurviveTheTreeConversion(); void startsEmpty(); void accountKeysComeFromTheAccountTags(); void accountKeysCoverAThreadSpanningTwoAccounts(); @@ -142,6 +143,39 @@ void TestThreadListModel::messageNodeHoldsDisplayFacts() QVERIFY(!fresh.isUnread()); } +void TestThreadListModel::rootRowsSurviveTheTreeConversion() +{ + // The point of this test is NOT the tree. It is that converting the base + // class from QAbstractTableModel changed nothing a thread row does: a table + // answers index() and parent() too, just trivially, and every existing test + // in this file is the real regression net beside it. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")) }); + + // A tree model reports its roots under an INVALID parent. + QCOMPARE(model.rowCount(QModelIndex()), 1); + QCOMPARE(model.columnCount(QModelIndex()), ThreadListModel::ColumnCount); + + const QModelIndex root = + model.index(0, ThreadListModel::SubjectColumn, QModelIndex()); + QVERIFY(root.isValid()); + QVERIFY(!model.parent(root).isValid()); + QCOMPARE(model.data(root, ThreadListModel::ThreadIdRole).toString(), + QStringLiteral("t1")); + + // No children until a thread's messages are asked for. An expander drawn + // over a thread whose replies were never loaded would open onto nothing. + QCOMPARE(model.rowCount(root), 0); + + // Qt's own conformance check. It walks index/parent/rowCount for + // consistency and catches the classic tree-model faults, such as a parent() + // that does not round-trip, which a hand-written assertion misses. + QAbstractItemModelTester tester( + &model, QAbstractItemModelTester::FailureReportingMode::Warning); + Q_UNUSED(tester); +} + void TestThreadListModel::startsEmpty() { ThreadListModel model; -- cgit v1.2.3