From 0c987310113f148e97b59da74c446f78356b98ff 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 + 6 files changed, 102 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 44feb0b..d092f92 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 df083fc..4828229 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; + }; -- cgit v1.2.3