diff options
| -rw-r--r-- | CHANGELOG.md | 6 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 40 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 11 | ||||
| -rw-r--r-- | src/tagchip.cpp | 43 | ||||
| -rw-r--r-- | src/tagchip.h | 8 | ||||
| -rw-r--r-- | src/threadlistmodel.cpp | 46 | ||||
| -rw-r--r-- | src/threadlistmodel.h | 9 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 55 | ||||
| -rw-r--r-- | tests/test_threadlistmodel.cpp | 86 |
9 files changed, 287 insertions, 17 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index ef2b5be..096eb19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,12 @@ point at which they are stable. until it exits, then a single summary line, so a run of over a minute was silent and there was nothing for the status bar to report. This is not a buffering problem and `stdbuf` does not help. +- **Read threads are dimmed in the list, rather than unread being bold.** Bold + was the only thing distinguishing the two, and on some systems it renders + identically to regular, which is a Qt or fontconfig matter this application + cannot reach: read and unread mail looked exactly alike. The distinction now + rides on colour, with unread left at the palette's own text colour and read + receding toward the background. Bold is still applied where it works. ### Fixed diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index faf510a..21e7a91 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -446,14 +446,38 @@ to copy. Two concrete sub-items, from using the list rather than looking at it: - **"All items look unread (bold), maybe use regular for read items?"** - **Check this before changing anything.** Bold is ALREADY conditional: - `ThreadListModel::data()` sets it under `Qt::FontRole` only when - `thread.isUnread()` (`src/threadlistmodel.cpp:148-152`). So either the - observation is that the user's list genuinely is mostly unread, in which case - there is no bug and the fix is elsewhere (the density work below), or bold is - leaking onto read rows through some path the model does not control. Reproduce - against a query with a known mix, e.g. `tag:inbox and not tag:unread`, before - touching the font logic. + **Done 2026-08-07, and the guess written here was wrong on both branches.** + + Bold was indeed already conditional, and the user's list was not mostly + unread, and bold was not leaking. The actual cause: **bold renders + identically to regular on the user's system.** Confirmed by eye against a + bare `QTableView` holding a plain `QStandardItemModel` with no qtmaildir code + involved, so the fault is in Qt or fontconfig, below this application, and + nothing in the model could ever have reached it. Bold was unread's ONLY cue. + + The fix inverts the emphasis instead: unread rows keep the palette's text + colour and READ rows are dimmed toward the background, via + `ThreadListModel::readColour()`. The cue rides on `Qt::ForegroundRole`, costs + no column, and suits the real ratio, which was 99 unread against 4220 read. + Bold is kept, since it works on other systems, but nothing depends on it. + + **A caution for anyone adding another `ForegroundRole` cue.** Qt resolves + that role into the palette and then prefers it over `HighlightedText`, so a + model-supplied colour wins on a SELECTED row too. The dim is blended against + the unselected background, so it landed as grey on the selection highlight, + near unreadable. `SubjectDelegate::initStyleOption` reverses that, and the + delegate is installed view-wide rather than on the subject column alone so + every column gets the same handling. + + **How this was nearly missed twice.** It was first dismissed from thread + counts, which explained why two screenshots looked alike but said nothing + about rendering. It was then dismissed again by a probe that counted lit + pixels: antialiasing makes a bold and a regular glyph light a similar number, + so the metric read "identical" regardless. Text WIDTH distinguishes them + (277px against 288px for the same string) and a strict pixel diff does; an + ink count does not. The tests that now guard this strip the font from the + model's answer entirely and require the two states to still differ, which is + the assertion that was missing all along. - **A star column for flagged threads**, mirroring the paperclip column that already exists for attachments. `ThreadSummary` carries the tags and diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index bb11261..30de89c 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -514,9 +514,14 @@ void MainWindow::buildUi() column, QHeaderView::Interactive); } - // The subject cell carries the account chip in front of its text. - m_threadView->setItemDelegateForColumn(ThreadListModel::SubjectColumn, - new SubjectDelegate(this)); + // The subject cell carries the account chip in front of its text, and + // every cell needs the delegate's selection handling: the read/unread + // dimming arrives as a Qt::ForegroundRole, which Qt's default painting + // prefers over the highlight, leaving a selected read row grey on the + // selection colour. SubjectDelegate::initStyleOption reverses that, and + // its paint() falls through to the base class wherever there is no chip, + // so the other columns keep their ordinary rendering. + m_threadView->setItemDelegate(new SubjectDelegate(this)); // 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. m_threadView->setHorizontalScrollBarPolicy(Qt::ScrollBarAsNeeded); diff --git a/src/tagchip.cpp b/src/tagchip.cpp index 2e21419..1ac7623 100644 --- a/src/tagchip.cpp +++ b/src/tagchip.cpp @@ -49,6 +49,31 @@ void paint(QPainter *painter, const QRect &rect, const QString &text, } // namespace TagChip +void SubjectDelegate::initStyleOption(QStyleOptionViewItem *option, + const QModelIndex &index) const +{ + QStyledItemDelegate::initStyleOption(option, index); + + // Qt resolves Qt::ForegroundRole into the palette's Text roles, and its + // own painting then prefers those over HighlightedText: a model that + // supplies a foreground wins even on a selected row. + // + // That is wrong for the read/unread dimming. A read thread's colour is + // blended against the UNSELECTED background, so over the selection + // highlight it lands as grey on purple, near unreadable. The highlight + // already says "this row", so the dimming can yield to it while selected. + // + // Doomed threads are unaffected in practice: their fill is drawn beneath + // the selection and their white is a contrast requirement, which + // HighlightedText also satisfies. + if (option->state & QStyle::State_Selected) { + const QColor highlighted = + option->palette.color(QPalette::HighlightedText); + option->palette.setColor(QPalette::Text, highlighted); + option->palette.setColor(QPalette::WindowText, highlighted); + } +} + void SubjectDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, const QModelIndex &index) const { @@ -87,13 +112,21 @@ void SubjectDelegate::paint(QPainter *painter, const QStyleOptionViewItem &optio return; painter->save(); - // The model supplies the row's colours; honouring them keeps a deleted - // thread white-on-red here as everywhere else. + // Selection outranks the model's colour, and that order matters. A read + // thread carries a dimmed foreground blended against the UNSELECTED + // background, so painting it over the highlight leaves grey-on-purple, + // which is close to unreadable. The highlight already carries the "this + // row" signal, so the read/unread distinction can yield to it for as long + // as the row is selected. + // + // A doomed thread is the exception that proves the rule: its white is not + // a dimming but a contrast requirement against its own fill, and the fill + // is drawn under the selection too. const QVariant foreground = index.data(Qt::ForegroundRole); - if (foreground.isValid()) - painter->setPen(foreground.value<QBrush>().color()); - else if (option.state & QStyle::State_Selected) + if (option.state & QStyle::State_Selected) painter->setPen(option.palette.highlightedText().color()); + else if (foreground.isValid()) + painter->setPen(foreground.value<QBrush>().color()); else painter->setPen(option.palette.text().color()); diff --git a/src/tagchip.h b/src/tagchip.h index 9bd4e78..f43e3ff 100644 --- a/src/tagchip.h +++ b/src/tagchip.h @@ -59,4 +59,12 @@ public: const QModelIndex &index) const override; QSize sizeHint(const QStyleOptionViewItem &option, const QModelIndex &index) const override; + +protected: + /// Makes the selection highlight outrank a model-supplied foreground. + /// + /// Qt's own resolution does the opposite, which leaves a dimmed read + /// thread painting grey over the selection colour. + void initStyleOption(QStyleOptionViewItem *option, + const QModelIndex &index) const override; }; diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 2289e6c..7e0477d 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -20,6 +20,8 @@ #include <QBrush> #include <QFont> +#include <QGuiApplication> +#include <QPalette> #include <QFontDatabase> #include <QFontMetrics> @@ -57,6 +59,29 @@ QColor ThreadListModel::spamColour() return QColor(0xa8, 0x5c, 0x18); } +QColor ThreadListModel::readColour() +{ + // Derived from the palette, never hardcoded: a fixed grey that reads as + // "quiet" on a light theme is nearly invisible on a dark one, which is the + // rule item 12 established for the message pane. + // + // Mixed toward the background rather than simply made transparent, so it + // composites the same over a selected row as over an unselected one. + const QPalette palette = QGuiApplication::palette(); + const QColor text = palette.color(QPalette::Text); + const QColor background = palette.color(QPalette::Base); + + // 0.55 of the text colour: clearly recessive beside an undimmed row, and + // still comfortably readable on its own. A read thread is not disabled, + // it is simply not the thing being pointed at. + constexpr qreal kWeight = 0.55; + const qreal inverse = 1.0 - kWeight; + return QColor::fromRgbF( + text.redF() * kWeight + background.redF() * inverse, + text.greenF() * kWeight + background.greenF() * inverse, + text.blueF() * kWeight + background.blueF() * inverse); +} + ThreadListModel::ThreadListModel(QObject *parent) : QAbstractTableModel(parent) { @@ -145,6 +170,27 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const return QBrush(QColor(Qt::white)); } + // Unread's cue, and it deliberately does NOT rely on the bold below. + // + // Bold was the only cue until 2026-08-07, when it turned out to render + // identically to regular on the user's system: confirmed with a bare + // QTableView and a plain QStandardItemModel, so the fault is in Qt or + // fontconfig, below this application, and nothing here can reach it. + // + // So the emphasis is inverted instead. Unread rows are left at the + // palette's own text colour, and READ rows are dimmed toward the + // background. That way the cue rides on ForegroundRole, which the delegate + // already honours, and it costs no column. It also suits the real ratio: + // with a few dozen unread among thousands read, dimming the bulk is calmer + // than highlighting it. + // + // BELOW the doomed branch on purpose, and that ordering is the whole + // protection: a deleted or spam thread has already returned white text for + // this role above, and dimming it because it is also read would drop that + // to unreadable against the crimson. Do not hoist this. + if (role == Qt::ForegroundRole && !thread.isUnread()) + return QBrush(readColour()); + if (role == Qt::FontRole) { QFont font; bool styled = false; diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index ab9378e..152730f 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -75,6 +75,15 @@ public: static QColor deletedColour(); static QColor spamColour(); + /// The dimmed text colour a READ thread carries. + /// + /// Unread rows are left at the palette's own colour and read ones recede, + /// rather than unread being emphasised. Bold used to be the only cue and + /// cannot be relied on: on at least one system it renders identically to + /// regular, which is a Qt or fontconfig matter this application cannot + /// reach. Derived from the palette, never hardcoded. + static QColor readColour(); + explicit ThreadListModel(QObject *parent = nullptr); /// Supplies the account chip colours. Not owned; must outlive the model. diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 1fdeaf2..1af95ed 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -81,6 +81,7 @@ private slots: void aSkippedLocalSyncStillReportsTheOtherRunFinishing(); void anUnobservableLockTableLeavesTheSyncButtonUsable(); void theStatusBarFollowsTheSyncPhase(); + void aSelectedReadThreadIsNotDimmedIntoTheHighlight(); void markAllReadIsDisabledUntilTheQueryFinishes(); void markAllReadActsOnEveryRowAndUndoesInOneStep(); void markAllReadDoesNothingWhenNothingIsUnread(); @@ -370,6 +371,60 @@ static ThreadSummary makeThread(const QString &id, const QStringList &tags) return thread; } +void TestMainWindow::aSelectedReadThreadIsNotDimmedIntoTheHighlight() +{ + // Read threads carry a dimmed Qt::ForegroundRole, blended against the + // UNSELECTED background. Qt's own painting prefers a model foreground over + // HighlightedText, so without SubjectDelegate::initStyleOption reversing + // that, selecting a read row paints it grey on the selection colour, which + // is close to unreadable. Seen in a screenshot before it was caught here. + // + // Rendered rather than asserted on roles: the model is right either way, + // and the defect lives entirely in how the delegate resolves them. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + 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 }); + + window.resize(900, 300); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + view->selectAll(); + QApplication::processEvents(); + + const int rowHeight = view->rowHeight(0); + QVERIFY(rowHeight > 0); + + QImage shot(view->viewport()->size(), QImage::Format_ARGB32); + 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))); +} + void TestMainWindow::markAllReadIsDisabledUntilTheQueryFinishes() { // Threads arrive in batches, so acting mid-load would silently skip diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index a9cdb73..b880c29 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -33,6 +33,9 @@ private slots: void reportsSubjectAndAuthors(); void subjectShowsMessageCountOnlyForRealThreads(); void unreadThreadsRenderBold(); + void readThreadsAreDimmedAndUnreadAreNot(); + void theUnreadCueDoesNotDependOnFontWeight(); + void aDoomedThreadKeepsItsContrastEvenWhenRead(); void tagsAreTheFirstColumnAndSubjectTheLast(); void accountTagBecomesAChipLabel(); void unreadStylingSurvivesAnAccountChip(); @@ -164,6 +167,79 @@ void TestThreadListModel::unreadThreadsRenderBold() QVERIFY(unreadFont.value<QFont>().bold()); } +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. + // + // Read rows are dimmed instead, which inverts the emphasis: unread sits at + // full contrast and the bulk of a mostly-read list recedes. + ThreadListModel model; + ThreadSummary read = makeThread(QStringLiteral("t1"), QStringLiteral("read")); + read.tags = QStringList{ QStringLiteral("inbox") }; + model.appendBatch( + { read, makeThread(QStringLiteral("t2"), QStringLiteral("unread")) }); + + const QVariant readFg = + model.data(model.index(0, ThreadListModel::SubjectColumn), + Qt::ForegroundRole); + const QVariant unreadFg = + model.data(model.index(1, ThreadListModel::SubjectColumn), + Qt::ForegroundRole); + + QVERIFY2(readFg.isValid(), "a read thread carries no dimming"); + QVERIFY2(!unreadFg.isValid(), + "an unread thread must be left at the palette's own colour, so it " + "is the one that stands out"); +} + +void TestThreadListModel::theUnreadCueDoesNotDependOnFontWeight() +{ + // The property that matters, stated directly: strip every font from the + // model's answer and the two states must still be distinguishable. A test + // asserting only that bold is set passes on a system where bold paints + // exactly like regular, which is precisely how this went unnoticed. + ThreadListModel model; + ThreadSummary read = makeThread(QStringLiteral("t1"), QStringLiteral("read")); + read.tags = QStringList{ QStringLiteral("inbox") }; + model.appendBatch( + { read, makeThread(QStringLiteral("t2"), QStringLiteral("unread")) }); + + for (int column = 0; column < ThreadListModel::ColumnCount; ++column) { + const QVariant readFg = + model.data(model.index(0, column), Qt::ForegroundRole); + const QVariant unreadFg = + model.data(model.index(1, column), Qt::ForegroundRole); + + QVERIFY2(readFg != unreadFg, + qPrintable(QStringLiteral("column %1 renders read and unread " + "identically once the font is " + "ignored").arg(column))); + } +} + +void TestThreadListModel::aDoomedThreadKeepsItsContrastEvenWhenRead() +{ + // Both cues write ForegroundRole, so they share one channel and the order + // matters. A deleted row forces white text onto its crimson fill; dimming + // it because it also happens to be read would drop that contrast to + // unreadable. + ThreadListModel model; + ThreadSummary thread = makeThread(QStringLiteral("t1"), + QStringLiteral("doomed and read")); + thread.tags = QStringList{ QStringLiteral("inbox") }; + model.appendBatch({ thread }); + + model.applyTagChange(QStringLiteral("t1"), { QStringLiteral("deleted") }, {}); + + const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); + QCOMPARE(model.data(subject, Qt::ForegroundRole).value<QBrush>().color(), + QColor(Qt::white)); +} + void TestThreadListModel::tagsAreTheFirstColumnAndSubjectTheLast() { // Subject stretches to fill the view, so whatever sits after it is pushed @@ -325,9 +401,17 @@ void TestThreadListModel::ordinaryThreadsCarryNoRowColour() const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); QVERIFY(!model.data(subject, Qt::BackgroundRole).isValid()); - QVERIFY(!model.data(subject, Qt::ForegroundRole).isValid()); const QVariant font = model.data(subject, Qt::FontRole); QVERIFY(!font.isValid() || !font.value<QFont>().strikeOut()); + + // The foreground goes back to the dimming a read thread carries, NOT to + // nothing: this thread has no unread tag, so plain for it means dimmed. + // What matters is that the doomed white is gone. + const QVariant foreground = model.data(subject, Qt::ForegroundRole); + if (foreground.isValid()) { + QVERIFY2(foreground.value<QBrush>().color() != QColor(Qt::white), + "the doomed white text survived the undo"); + } } void TestThreadListModel::threadIdIsReachableFromAnIndex() |