aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/mainwindow.cpp21
-rw-r--r--src/tagchip.cpp56
-rw-r--r--src/tagchip.h14
-rw-r--r--src/threadlistmodel.cpp7
-rw-r--r--src/threadlistmodel.h7
-rw-r--r--src/threadlistview.h1
-rw-r--r--tests/test_mainwindow.cpp130
7 files changed, 231 insertions, 5 deletions
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;
+
};
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index 58768b7..37bfaef 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -92,6 +92,7 @@ private slots:
void aSelectedReadThreadIsNotDimmedIntoTheHighlight();
void thePillRowSpansTheWholeWidthNotOneColumn();
void childRowsAreIndentedUnderTheirThread();
+ void aThreadWithRepliesDrawsAVisibleExpander();
void noTagStripIsPaintedUnderAMessageRow();
void markAllReadIsDisabledUntilTheQueryFinishes();
void markAllReadActsOnEveryRowAndUndoesInOneStep();
@@ -490,7 +491,12 @@ void TestMainWindow::childRowsAreIndentedUnderTheirThread()
auto *view = window.findChild<QTreeView *>();
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");
@@ -531,6 +537,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<ThreadListModel *>();
+ QVERIFY(model);
+ auto *view = window.findChild<QTreeView *>();
+ 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()