From 78733fbc5ab8c1d4707444b5bce3990dbc0a9fcb Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 10:35:06 +0200 Subject: refactor(view): make ThreadListView a QTreeView for message rows The strip survived the port because every geometry call it needs exists on both classes. What did not survive is anything keyed on a row NUMBER: a tree numbers rows per parent, so row 0 exists once per expanded thread and the old flat 0..N walk would paint the first thread's strip over every one of them. The walk now goes by index, and the alternating colour follows visual position rather than index.row() for the same reason. QTableView::isRowSelected(int) has no QTreeView equivalent; isSelected on the index replaces it. MainWindow loses verticalHeader and selectRow, so row height comes from uniformRowHeights and three helpers replace the row arithmetic. next_thread and prev_thread now resolve the containing thread first: in a tree current.row() + 1 is the next SIBLING, which under an expanded thread is the next reply, not the next thread. Two test defects found by mutation and worth recording, since both produced a green suite over a broken assertion: The indent test asserted on column 0. A QTreeView indents only the column holding the expander, verified against Qt 6.11: with setTreePosition(4), column 0 reports the same left edge for a thread and its reply while column 4 reports 420 against 440. It was failing against a correctly indented tree. The strip test passed with the view's skip deleted, because the real model already returns no pills for a child row, so the view's guard was never the thing under test. It now runs against a stub model that hands pills to every row, which leaves the view's skip as the only thing that can keep replies clean. That rewrite then failed for a third reason: without the delegates MainWindow installs, rows take the default height, the band is measured against SubjectDelegate::rowHeightFor and overflows into the row below, and the thread's own strip paints across the reply. Reads exactly like a missing skip and is not one. --- src/mainwindow.cpp | 85 ++++++++++++++++++++++++++++++++++++++++---------- src/mainwindow.h | 17 ++++++++-- src/threadlistview.cpp | 45 ++++++++++++++++++-------- src/threadlistview.h | 18 +++++++++-- 4 files changed, 130 insertions(+), 35 deletions(-) (limited to 'src') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 7866524..25b7a50 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -99,6 +99,40 @@ QString MainWindow::locksPath() return g_locksPath; } +/// The thread row containing an index: the index itself when it is already a +/// thread row, its parent when it is a message row. +/// +/// Replaces the arithmetic on row numbers that a table permitted. In a tree a +/// row number only identifies a row within one parent, so "current.row() + 1" +/// means the next SIBLING, which under an expanded thread is the next reply. +QModelIndex MainWindow::threadRowOf(const QModelIndex &index) const +{ + if (!index.isValid()) + return {}; + return index.parent().isValid() ? index.parent() : index; +} + +/// Selects a whole row, the way QTableView::selectRow did. +/// +/// QTreeView has no selectRow, and SelectRows on the selection model is not a +/// substitute: it governs what a click extends to, not what a programmatic +/// select() covers. +void MainWindow::selectRowAt(const QModelIndex &index) +{ + if (!index.isValid()) + return; + + m_threadView->selectionModel()->select( + index, QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + m_threadView->setCurrentIndex(index); +} + +/// Selects the top-level thread row at `row`. +void MainWindow::selectThreadRow(int row) +{ + selectRowAt(m_model->index(row, 0, QModelIndex())); +} + void MainWindow::restoreUiState() { QSettings state(uiStatePath(), QSettings::IniFormat); @@ -135,7 +169,7 @@ void MainWindow::restoreUiState() const int savedColumns = state.value(QStringLiteral("threadlist/columns")).toInt(); if (!header.isEmpty() && savedColumns == ThreadListModel::ColumnCount) { - m_threadView->horizontalHeader()->restoreState(header); + m_threadView->header()->restoreState(header); } // The config value is the starting point for a profile that has never @@ -154,7 +188,7 @@ void MainWindow::saveUiState() const state.setValue(QStringLiteral("window/state"), saveState()); state.setValue(QStringLiteral("window/splitter"), m_splitter->saveState()); state.setValue(QStringLiteral("threadlist/header"), - m_threadView->horizontalHeader()->saveState()); + m_threadView->header()->saveState()); // Guards the blob above: see restoreUiState(). state.setValue(QStringLiteral("threadlist/columns"), int(ThreadListModel::ColumnCount)); @@ -511,16 +545,24 @@ void MainWindow::buildUi() m_threadView->setModel(m_model); m_threadView->setSelectionBehavior(QAbstractItemView::SelectRows); m_threadView->setSelectionMode(QAbstractItemView::ExtendedSelection); - m_threadView->verticalHeader()->hide(); - m_threadView->horizontalHeader()->setStretchLastSection(false); + m_threadView->header()->setStretchLastSection(false); // Every column Interactive, Subject included: Stretch and ResizeToContents // both compute a width and discard the user's drag. Nothing absorbs spare // width as a result, so the columns end where they end. for (int column = 0; column < ThreadListModel::ColumnCount; ++column) { - m_threadView->horizontalHeader()->setSectionResizeMode( + m_threadView->header()->setSectionResizeMode( column, QHeaderView::Interactive); } + // The expander goes on the subject column, not on column 0. Column 0 is the + // narrow attachment marker, and an expander there has no room: it pushes the + // 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); + // Two delegates, and the split is not cosmetic. RowStyleDelegate carries // only the selection fix every column needs: the read/unread dimming // arrives as a Qt::ForegroundRole, which Qt's painting prefers over the @@ -535,11 +577,13 @@ void MainWindow::buildUi() m_threadView->setItemDelegateForColumn(ThreadListModel::SubjectColumn, new SubjectDelegate(this)); - // One height for every row, set here rather than left to a column's - // sizeHint: a QTableView takes a single height per row, so a hint from the - // subject column alone would only apply if the view happened to ask it. - m_threadView->verticalHeader()->setDefaultSectionSize( - SubjectDelegate::rowHeightFor(m_threadView->font())); + // One height for every row. A QTreeView has no vertical header to carry a + // default section size, so the height comes from uniformRowHeights plus the + // delegate's own sizeHint. uniformRowHeights is not merely an optimisation + // here: without it the tree measures every row separately and the tag strip, + // which is painted OUTSIDE any cell, is not accounted for in any of those + // measurements, so rows collapse to text height and the strip is clipped. + m_threadView->setUniformRowHeights(true); // Widening a column past the viewport scrolls rather than squeezing the // others. Per-pixel so the scroll does not jump a whole column at a time. // Banding, so the eye can follow a row across four columns and a pill @@ -555,7 +599,7 @@ void MainWindow::buildUi() // Without this the attachment column cannot be narrow at all: the default // minimum section size is 58px on this platform, and setColumnWidth() // clamps to it silently rather than reporting the smaller value back. - m_threadView->horizontalHeader()->setMinimumSectionSize(24); + m_threadView->header()->setMinimumSectionSize(24); m_threadView->setColumnWidth(ThreadListModel::AttachmentColumn, 28); m_threadView->setColumnWidth(ThreadListModel::FlagColumn, 28); m_threadView->setColumnWidth(ThreadListModel::DateColumn, 130); @@ -634,16 +678,23 @@ void MainWindow::registerActions() }); addAction(QStringLiteral("next_thread"), tr("&Next thread"), tr("Select the next thread"), [this]() { + // The THREAD after this one, which is not "the next row" once replies + // are expanded: from a thread row the next row may be its own first + // reply, and from a reply row the row number counts siblings, not + // threads. Both are resolved by walking up to the containing thread + // first. const QModelIndex current = m_threadView->currentIndex(); - const int row = current.isValid() ? current.row() + 1 : 0; + const QModelIndex thread = threadRowOf(current); + const int row = thread.isValid() ? thread.row() + 1 : 0; if (row < m_model->rowCount()) - m_threadView->selectRow(row); + selectThreadRow(row); }); addAction(QStringLiteral("prev_thread"), tr("&Previous thread"), tr("Select the previous thread"), [this]() { const QModelIndex current = m_threadView->currentIndex(); - if (current.isValid() && current.row() > 0) - m_threadView->selectRow(current.row() - 1); + const QModelIndex thread = threadRowOf(current); + if (thread.isValid() && thread.row() > 0) + selectThreadRow(thread.row() - 1); }); addAction(QStringLiteral("open_thread"), tr("&Open thread"), tr("Focus the thread list"), [this]() { @@ -1457,8 +1508,8 @@ void MainWindow::showThreadContextMenu(const QPoint &pos) // collapsing to the clicked row here would silently narrow a deliberate // multi-row selection to one. Right-clicking outside it selects that row // instead, which is what every other list does. - if (!m_threadView->selectionModel()->isRowSelected(index.row())) - m_threadView->selectRow(index.row()); + if (!m_threadView->selectionModel()->isSelected(index)) + selectRowAt(index); m_threadContextMenu->popup(m_threadView->viewport()->mapToGlobal(pos)); } diff --git a/src/mainwindow.h b/src/mainwindow.h index 2d41b5e..0c6865b 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -41,7 +41,7 @@ class QAction; class QLineEdit; class QMenu; -class QTableView; +class ThreadListView; class QLabel; class QPushButton; class QComboBox; @@ -211,6 +211,16 @@ private: /// Restores window geometry, splitter and thread-list header widths. /// A missing or rejected blob leaves the buildUi() defaults in place. void restoreUiState(); + + /// The thread row containing an index: itself for a thread row, its parent + /// for a message row. + QModelIndex threadRowOf(const QModelIndex &index) const; + + /// Selects a whole row. QTreeView has no selectRow of its own. + void selectRowAt(const QModelIndex &index); + + /// Selects the top-level thread row at `row`. + void selectThreadRow(int row); void saveUiState() const; void registerActions(); @@ -405,7 +415,10 @@ private: QLineEdit *m_queryEdit = nullptr; QueryCompleter *m_queryCompleter = nullptr; - QTableView *m_threadView = nullptr; + /// Its own type, not the QTreeView base. The strip painting and the + /// expander column are ThreadListView's, and holding the base here only + /// hid that from every reader. + ThreadListView *m_threadView = nullptr; /// Right-click menu for the thread list, holding the same QActions the /// menu bar does. diff --git a/src/threadlistview.cpp b/src/threadlistview.cpp index ef80e09..ebca4cc 100644 --- a/src/threadlistview.cpp +++ b/src/threadlistview.cpp @@ -27,7 +27,7 @@ void ThreadListView::paintEvent(QPaintEvent *event) { - QTableView::paintEvent(event); + QTreeView::paintEvent(event); if (!model()) return; @@ -43,18 +43,34 @@ void ThreadListView::paintEvent(QPaintEvent *event) const QFontMetrics metrics(pillFont); painter.setFont(pillFont); - // Only the rows actually on screen. Walking the whole model would paint - // thousands of strips outside the viewport on a large query. - const int first = rowAt(0); - const int last = rowAt(viewport()->height() - 1); - const int lastRow = last >= 0 ? last : model()->rowCount() - 1; + // Only the rows actually on screen, walked by INDEX rather than by row + // number. A tree numbers rows per parent, so row 0 exists once per expanded + // thread and the old flat 0..N walk would paint the first thread's strip + // over every one of them. + QModelIndex walk = indexAt(QPoint(0, 0)); + + // Counts the rows actually painted, for the alternating colour. In a tree + // that has to follow VISUAL position: row 0 under three different threads + // is three different stripes, and using index.row() would give all three + // the same one. + int visualRow = 0; + + 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()) + continue; - for (int row = qMax(0, first); row <= lastRow; ++row) { - const QModelIndex index = - model()->index(row, ThreadListModel::SubjectColumn); + const QModelIndex index = walk.siblingAtColumn( + ThreadListModel::SubjectColumn); - const int rowTop = rowViewportPosition(row); - const int height = rowHeight(row); + const int rowTop = rowRect.top(); + const int height = rowRect.height(); if (height <= 0) continue; @@ -86,9 +102,12 @@ void ThreadListView::paintEvent(QPaintEvent *event) if (background.isValid()) painter.fillRect(band, background.value()); - else if (selectionModel() && selectionModel()->isRowSelected(row)) + // isSelected on the index, not isRowSelected(int): a QTreeView has no + // such overload, and a row number alone cannot name a row in a tree + // anyway since it is only unique under one parent. + else if (selectionModel() && selectionModel()->isSelected(index)) painter.fillRect(band, palette().brush(QPalette::Highlight)); - else if (alternatingRowColors() && (row % 2)) + else if (alternatingRowColors() && (visualRow % 2)) painter.fillRect(band, palette().brush(QPalette::AlternateBase)); else painter.fillRect(band, palette().brush(QPalette::Base)); diff --git a/src/threadlistview.h b/src/threadlistview.h index 0b4eafc..520d610 100644 --- a/src/threadlistview.h +++ b/src/threadlistview.h @@ -18,7 +18,7 @@ #pragma once -#include +#include /// The thread list, with a row-wide strip of tag chips under each row's cells. /// @@ -36,11 +36,23 @@ /// The cells confine themselves to the upper band so the lower one is free; /// SubjectDelegate::kRowPadding and rowHeightFor() are the shared measurements /// that keep the two halves agreeing. -class ThreadListView : public QTableView +/// +/// A QTreeView rather than a QTableView since item 20: a thread's replies are +/// child rows, and a table can neither indent nor expand. The strip survived +/// the port because every geometry call it needs (visualRect, +/// columnViewportPosition, indexAt, indexBelow) exists on both. What did NOT +/// survive is anything keyed on a row NUMBER: a tree numbers rows per parent, +/// so row 0 exists once per expanded thread and a flat 0..N walk paints the +/// first thread's strip over every one of them. The walk below goes by index. +/// +/// The strip is painted for THREAD rows only. It carries the thread's tags, so +/// one under each reply would stripe the list and repeat identical tags down +/// the whole expansion. +class ThreadListView : public QTreeView { Q_OBJECT public: - using QTableView::QTableView; + using QTreeView::QTreeView; protected: void paintEvent(QPaintEvent *event) override; -- cgit v1.2.3