diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-07 16:26:22 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-07 16:26:22 +0200 |
| commit | 39cbde74a560407e24b05e171df883421aa2153e (patch) | |
| tree | a7390b036f57507b17aa4291511ce33d3f297f81 /tests | |
| parent | de884b036689b253d10ff48daa3a05cca20ba61d (diff) | |
| download | qtmaildir-39cbde74a560407e24b05e171df883421aa2153e.tar.gz qtmaildir-39cbde74a560407e24b05e171df883421aa2153e.zip | |
feat(ui): show each thread's tags under its row
The thread list was uniform and cramped: every row one line tall, with
nothing to say what a thread was about before opening it. Rows are now
roughly double height, carrying a strip of tag chips beneath the text,
with alternating row colours and a star column for flagged threads
beside the existing paperclip.
The strip is painted by the VIEW rather than by a delegate, which is
why ThreadListView exists. A delegate is handed one cell's rectangle
and cannot paint outside its column, so a strip drawn from the subject
column stops at that column's edge, losing the last tags of a
well-tagged thread, and starts at its left edge, putting the chips
under the subject instead of under the row.
Tags the row already shows another way are left out: inbox as
structure, unread as the dimming, flagged as the star, attachment as
the paperclip, and the account as the chip in the subject cell. Sorted,
since notmuch's order is not guaranteed stable and a row whose chips
reordered between repaints would flicker.
Six defects were introduced and fixed on the way here, all of them one
consequence: a QTableView paints per cell, and a row-wide strip is not
a cell. SubjectDelegate installed view-wide drew the account chip into
every column, since AccountLabelRole belongs to the row; it is split
into RowStyleDelegate for every column and SubjectDelegate for the
subject alone, with a Q_ASSERT guarding that. Row height returned from
sizeHint did nothing, because a table takes one height per row. The
strip painted from x=0 over the marker columns, via a protected
viewportMargins() that returns 0. Measuring the text band and the strip
with one font put the pills over the date. Alternating colours and the
selection are per-cell too, so the band showed bare viewport background
until the view filled it, honouring the model's own BackgroundRole
first so a deleted row is not cut in half. And that fill spanned the
full width, cutting the centred marker glyphs at their midpoint.
Closes item 5.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Diffstat (limited to 'tests')
| -rw-r--r-- | tests/test_mainwindow.cpp | 153 | ||||
| -rw-r--r-- | tests/test_threadlistmodel.cpp | 118 |
2 files changed, 244 insertions, 27 deletions
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 1af95ed..7ee8a5a 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -29,6 +29,7 @@ #include <QPushButton> #include <QProgressBar> #include <QFile> +#include <QSet> #include <QSettings> #include <QStandardPaths> #include <QTemporaryDir> @@ -41,6 +42,7 @@ #include "mainwindow.h" #include "messageview.h" #include "notmuchworker.h" +#include "tagchip.h" #include "threadlistmodel.h" /// MainWindow is mostly wiring, and the parts that need a real database are @@ -82,6 +84,7 @@ private slots: void anUnobservableLockTableLeavesTheSyncButtonUsable(); void theStatusBarFollowsTheSyncPhase(); void aSelectedReadThreadIsNotDimmedIntoTheHighlight(); + void thePillRowSpansTheWholeWidthNotOneColumn(); void markAllReadIsDisabledUntilTheQueryFinishes(); void markAllReadActsOnEveryRowAndUndoesInOneStep(); void markAllReadDoesNothingWhenNothingIsUnread(); @@ -371,6 +374,83 @@ static ThreadSummary makeThread(const QString &id, const QStringList &tags) return thread; } +void TestMainWindow::thePillRowSpansTheWholeWidthNotOneColumn() +{ + // The pills are a row-wide strip under the cells, not content of the + // subject cell. Drawn from the subject column's delegate they stop at that + // column's edge, so a thread with several tags loses the last of them; and + // they inherit the column's left edge, which puts them under the subject + // rather than under the row. + // + // The property: pills appear to the LEFT of where the subject column + // starts, which no per-cell delegate on that column could produce. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTableView *>(); + QVERIFY(view); + + ThreadSummary thread = makeThread(QStringLiteral("t1"), {}); + thread.tags = QStringList{ QStringLiteral("mailing-list/SBo"), + QStringLiteral("signed") }; + model->appendBatch({ thread }); + + window.resize(1400, 300); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + QApplication::processEvents(); + + const int subjectLeft = + view->columnViewportPosition(ThreadListModel::SubjectColumn); + QVERIFY2(subjectLeft > 40, + qPrintable(QStringLiteral("the subject column starts at x=%1, too " + "close to the left edge to tell a " + "row-wide strip from a subject-cell one") + .arg(subjectLeft))); + // The strip must have somewhere to paint that the subject cell does not + // reach, or this test cannot fail. + QVERIFY2(subjectLeft < view->viewport()->width(), + qPrintable(QStringLiteral("the subject column is off-screen " + "(x=%1, viewport %2), so nothing it " + "draws is measurable") + .arg(subjectLeft) + .arg(view->viewport()->width()))); + + QImage shot(view->viewport()->size(), QImage::Format_ARGB32); + shot.fill(Qt::transparent); + view->viewport()->render(&shot); + + // Count pixels matching the tag colours EXACTLY, not "saturated" pixels. + // A looser test counts the antialiased edge of the selection highlight + // blending into the background, which is several hundred distinct + // near-background colours and passes whatever the strip does. Both earlier + // versions of this test did precisely that. + QSet<QRgb> pillColours; + const QVariantList colours = + model->index(0, ThreadListModel::SubjectColumn) + .data(ThreadListModel::PillColoursRole).toList(); + QVERIFY2(!colours.isEmpty(), "the model supplied no pill colours"); + for (const QVariant &colour : colours) + pillColours.insert(colour.value<QColor>().rgb()); + + const int rowHeight = view->rowHeight(0); + QVERIFY(rowHeight > 0); + + int chipPixels = 0; + for (int y = 0; y < qMin(rowHeight, shot.height()); ++y) { + for (int x = 0; x < qMin(subjectLeft, shot.width()); ++x) { + if (pillColours.contains(shot.pixel(x, y) | 0xff000000)) + ++chipPixels; + } + } + + QVERIFY2(chipPixels > 0, + "no pill-coloured pixels left of the subject column: the strip is " + "still confined to that cell rather than spanning the row"); +} + void TestMainWindow::aSelectedReadThreadIsNotDimmedIntoTheHighlight() { // Read threads carry a dimmed Qt::ForegroundRole, blended against the @@ -389,20 +469,30 @@ void TestMainWindow::aSelectedReadThreadIsNotDimmedIntoTheHighlight() auto *view = window.findChild<QTableView *>(); QVERIFY(view); - // Identical but for the unread tag, so any pixel difference between the - // two selected rows is the dimming leaking through. - ThreadSummary read = makeThread(QStringLiteral("t1"), {}); - ThreadSummary unread = - makeThread(QStringLiteral("t2"), { QStringLiteral("unread") }); - read.subject = unread.subject = QStringLiteral("Same subject both rows"); - read.authors = unread.authors = QStringLiteral("Someone <s@example.org>"); - model->appendBatch({ read, unread }); + // Both rows READ, so both are dimmed and neither is bold: the only thing + // that could differ is how the dimming composites against the selection. + // + // Comparing a read row against an unread one would not work, and an + // earlier version of this test did exactly that. Unread also paints bold, + // so the rows differ legitimately and the comparison says nothing about + // the selection. That version passed only because the machine it was + // written on had its Qt font configured Bold, which made every row bold + // and hid the difference. + ThreadSummary first = makeThread(QStringLiteral("t1"), {}); + ThreadSummary second = makeThread(QStringLiteral("t2"), {}); + first.subject = second.subject = QStringLiteral("Same subject both rows"); + first.authors = second.authors = QStringLiteral("Someone <s@example.org>"); + model->appendBatch({ first, second }); window.resize(900, 300); window.show(); QVERIFY(QTest::qWaitForWindowExposed(&window)); - view->selectAll(); + // Row 0 selected, row 1 not. The property under test is that selecting a + // dimmed row switches it to the highlight's own text colour, so the two + // rows MUST differ; comparing two identically-styled rows would pass + // against a delegate that did nothing at all. + view->selectRow(0); QApplication::processEvents(); const int rowHeight = view->rowHeight(0); @@ -412,17 +502,40 @@ void TestMainWindow::aSelectedReadThreadIsNotDimmedIntoTheHighlight() shot.fill(Qt::transparent); view->viewport()->render(&shot); - int differing = 0; - for (int y = 0; y < rowHeight && y + rowHeight < shot.height(); ++y) - for (int x = 0; x < shot.width(); ++x) - if (shot.pixel(x, y) != shot.pixel(x, y + rowHeight)) - ++differing; - - QVERIFY2(differing == 0, - qPrintable(QStringLiteral("a selected read row paints differently " - "from a selected unread one (%1 pixels): " - "the dimming is overriding the selection " - "highlight").arg(differing))); + // What the delegate resolves for each row, which is the thing the fix + // changes. Rendering alone cannot separate "used the highlight colour" + // from "used the dim over a highlighted background". + QStyleOptionViewItem selected; + selected.initFrom(view); + selected.state |= QStyle::State_Selected; + QStyleOptionViewItem unselected; + unselected.initFrom(view); + unselected.state &= ~QStyle::State_Selected; + + auto *delegate = qobject_cast<QStyledItemDelegate *>(view->itemDelegate()); + QVERIFY2(delegate, "the thread view has no styled delegate"); + + const QModelIndex index = + model->index(0, ThreadListModel::SubjectColumn); + + // initStyleOption is protected, so the resolved palette is reached the way + // the painter does: through a subclass that exposes it. + struct Probe : SubjectDelegate { + using SubjectDelegate::initStyleOption; + }; + const auto *probe = static_cast<const Probe *>( + static_cast<const SubjectDelegate *>(delegate)); + + probe->initStyleOption(&selected, index); + probe->initStyleOption(&unselected, index); + + QVERIFY2(selected.palette.color(QPalette::Text) + == selected.palette.color(QPalette::HighlightedText), + "a selected row still resolves to the dimmed text colour, so the " + "dimming will paint over the selection highlight"); + QVERIFY2(unselected.palette.color(QPalette::Text) + != selected.palette.color(QPalette::Text), + "an unselected read row lost its dimming"); } void TestMainWindow::markAllReadIsDisabledUntilTheQueryFinishes() diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index b880c29..82686e5 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -20,6 +20,7 @@ #include <QSignalSpy> #include <QtTest> +#include "tagcolors.h" #include "threadlistmodel.h" class TestThreadListModel : public QObject @@ -34,6 +35,9 @@ private slots: void subjectShowsMessageCountOnlyForRealThreads(); void unreadThreadsRenderBold(); void readThreadsAreDimmedAndUnreadAreNot(); + void flaggedThreadsShowAStar(); + void pillTagsExcludeWhatTheRowAlreadyShows(); + void theStarColumnIsNarrowAndCarriesNoText(); void theUnreadCueDoesNotDependOnFontWeight(); void aDoomedThreadKeepsItsContrastEvenWhenRead(); void tagsAreTheFirstColumnAndSubjectTheLast(); @@ -169,14 +173,15 @@ void TestThreadListModel::unreadThreadsRenderBold() void TestThreadListModel::readThreadsAreDimmedAndUnreadAreNot() { - // Bold was unread's ONLY cue, and on the user's system it renders - // identically to regular: verified with a bare QTableView and a plain - // QStandardItemModel, so the fault is below this application, in Qt or - // fontconfig, and no model change can reach it. Bold is kept, since it - // works elsewhere, but the state can no longer depend on it. + // Bold was unread's ONLY cue, which leaves nothing to see when the + // desktop's own font is configured bold: every row renders bold and + // setBold() changes nothing. That is what the original report turned out + // to be, a qt6ct setting rather than a defect here, but a cue with one + // point of failure is worth reinforcing. // - // Read rows are dimmed instead, which inverts the emphasis: unread sits at - // full contrast and the bulk of a mostly-read list recedes. + // Read rows are dimmed as well, which inverts the emphasis: unread sits at + // full contrast and the bulk of a mostly-read list recedes. Bold still + // applies on top. ThreadListModel model; ThreadSummary read = makeThread(QStringLiteral("t1"), QStringLiteral("read")); read.tags = QStringList{ QStringLiteral("inbox") }; @@ -196,6 +201,105 @@ void TestThreadListModel::readThreadsAreDimmedAndUnreadAreNot() "is the one that stands out"); } +void TestThreadListModel::flaggedThreadsShowAStar() +{ + // "flagged" is an ordinary notmuch tag already carried in ThreadSummary, + // so this needs no worker query, exactly as the paperclip did not. + ThreadListModel model; + ThreadSummary plain = makeThread(QStringLiteral("t1"), QStringLiteral("plain")); + plain.tags = QStringList{ QStringLiteral("inbox") }; + ThreadSummary starred = makeThread(QStringLiteral("t2"), + QStringLiteral("starred")); + starred.tags = QStringList{ QStringLiteral("inbox"), + QStringLiteral("flagged") }; + model.appendBatch({ plain, starred }); + + const QString none = + model.data(model.index(0, ThreadListModel::FlagColumn), + Qt::DisplayRole).toString(); + const QString star = + model.data(model.index(1, ThreadListModel::FlagColumn), + Qt::DisplayRole).toString(); + + QVERIFY2(none.isEmpty(), "an unflagged thread shows something in the column"); + QVERIFY2(!star.isEmpty(), "a flagged thread shows nothing"); + QCOMPARE(star, ThreadListModel::flagGlyph()); +} + +void TestThreadListModel::pillTagsExcludeWhatTheRowAlreadyShows() +{ + // The pills exist to say what the row does not already say. Repeating the + // account, the flag, the attachment or the read state as text beside the + // chip, the star, the paperclip and the dimming would spend the new space + // on things already visible. + ThreadListModel model; + ThreadSummary thread = makeThread(QStringLiteral("t1"), + QStringLiteral("noisy")); + thread.tags = QStringList{ + QStringLiteral("inbox"), // structural, always true here + QStringLiteral("unread"), // shown by not being dimmed + QStringLiteral("flagged"), // shown by the star column + QStringLiteral("attachment"), // shown by the paperclip column + QStringLiteral("account-work"), // shown as the chip + QStringLiteral("SBo"), // worth showing + QStringLiteral("shopping/amazon"), + }; + model.appendBatch({ thread }); + + const QStringList pills = + model.data(model.index(0, ThreadListModel::SubjectColumn), + ThreadListModel::PillTagsRole).toStringList(); + + QVERIFY2(pills.contains(QStringLiteral("SBo")), qPrintable(pills.join(','))); + QVERIFY2(pills.contains(QStringLiteral("shopping/amazon")), + qPrintable(pills.join(','))); + + for (const QString &hidden : { QStringLiteral("inbox"), + QStringLiteral("unread"), + QStringLiteral("flagged"), + QStringLiteral("attachment") }) { + QVERIFY2(!pills.contains(hidden), + qPrintable(QStringLiteral("'%1' is repeated as a pill") + .arg(hidden))); + } + + // The account tag is matched by shape rather than by name, since the key + // varies per user: whatever TagColors calls an account tag is excluded. + for (const QString &tag : pills) { + QVERIFY2(!TagColors::isAccountTag(tag), + qPrintable(QStringLiteral("account tag '%1' repeated as a pill") + .arg(tag))); + } + + // Stable order, so a row does not reshuffle its own pills between repaints. + QStringList sorted = pills; + sorted.sort(); + QCOMPARE(pills, sorted); +} + +void TestThreadListModel::theStarColumnIsNarrowAndCarriesNoText() +{ + // A marker column, like the paperclip beside it: centred, and never + // carrying the subject or anything else that would want width. + ThreadListModel model; + ThreadSummary starred = makeThread(QStringLiteral("t1"), + QStringLiteral("starred")); + starred.tags = QStringList{ QStringLiteral("flagged") }; + model.appendBatch({ starred }); + + const QModelIndex index = model.index(0, ThreadListModel::FlagColumn); + QCOMPARE(model.data(index, Qt::TextAlignmentRole).toInt(), + int(Qt::AlignCenter)); + + // The glyph is one character, whether it is the star or its fallback: a + // column sized for a marker cannot hold a word. + QCOMPARE(ThreadListModel::flagGlyph().size(), 1); + + // And it says what it means, for anyone who cannot tell the glyph apart + // from the paperclip beside it. + QVERIFY(!model.data(index, Qt::ToolTipRole).toString().isEmpty()); +} + void TestThreadListModel::theUnreadCueDoesNotDependOnFontWeight() { // The property that matters, stated directly: strip every font from the |
