From 497c56a962512949d606c26ae5159621b58a5e7b Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 08:32:36 +0200 Subject: feat(view): compute a card's geometry with no painting Split from the delegate deliberately. A delegate needs a live painter and an exposed view, which is what makes delegate tests fragile: viewport()->render() returns a blank image in several ordinary situations, and a probe reporting no ink is likelier broken than the code it tests. Every geometric claim about a card is made here, where a test is a function call. Three lines at a uniform height, so setUniformRowHeights(true) survives. Indent caps at depth 4 with qMin rather than a branch, so depth 5 and depth 50 land in the same place. The date is measured before the sender, so a long sender elides instead of painting over it. Two traps handled that a first pass gets wrong. QRect::right() is inclusive, so the right edge is carried as an exclusive one and everything sized from it lands where the padding constant says rather than a pixel short. And QFont::pointSizeF returns -1 for a font set in pixels, which qt6ct does, so smallFont branches on which unit the font actually carries instead of silently returning the card's own size. --- src/cardlayout.cpp | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 src/cardlayout.cpp (limited to 'src/cardlayout.cpp') diff --git a/src/cardlayout.cpp b/src/cardlayout.cpp new file mode 100644 index 0000000..8d952b2 --- /dev/null +++ b/src/cardlayout.cpp @@ -0,0 +1,119 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include "cardlayout.h" + +#include + +QFont CardLayout::smallFont(const QFont &cardFont) +{ + QFont small = cardFont; + // Derived from the card's font rather than fixed, so it follows the + // desktop's font size instead of shrinking to nothing on a large one. + // + // pointSizeF() returns -1 for a font set in PIXELS, which qt6ct and some + // styles do. Subtracting from -1 would ask for an invalid size and Qt + // would silently keep the original, making the small font the same size as + // the card's; the pixel branch avoids that. + if (small.pointSizeF() > 0.0) + small.setPointSizeF(qMax(6.0, cardFont.pointSizeF() - 1.0)); + else if (small.pixelSize() > 0) + small.setPixelSize(qMax(8, cardFont.pixelSize() - 1)); + return small; +} + +int CardLayout::heightFor(const QFont &font) +{ + const QFontMetrics metrics(font); + const QFontMetrics smallMetrics(smallFont(font)); + // Two lines at the card's font, one at the small one, plus the padding + // above the first and below the last. + return kPaddingY * 2 + metrics.height() * 2 + smallMetrics.height(); +} + +CardLayout CardLayout::compute(const Input &input, const QRect &rect, + const QFont &font) +{ + CardLayout out; + const QFontMetrics metrics(font); + const QFontMetrics smallMetrics(smallFont(font)); + + out.totalHeight = rect.height(); + + // The accent bar sits flush against the card's left edge, on thread cards + // only, and everything else starts after it so no text sits on the colour. + if (!input.isMessage) { + out.accentRect = + QRect(rect.left(), rect.top(), kAccentWidth, rect.height()); + } + const int textLeft = rect.left() + kAccentWidth; + + // Indent, capped. qMin rather than a branch so depth 5 and depth 50 land + // in exactly the same place. + const int depth = qMin(input.depth, kMaxDepth); + const int indent = depth * kIndentStep; + out.contentLeft = textLeft + kPaddingX + indent; + + // One spine per level actually indented, each running the card's full + // height so an expansion reads as one continuous block. + for (int level = 0; level < depth; ++level) { + const int x = textLeft + kPaddingX + level * kIndentStep + + kIndentStep / 2; + out.spines.append(QRect(x, rect.top(), 2, rect.height())); + } + + // The EXCLUSIVE right edge: one past the last pixel a card may draw on. + // QRect::right() is inclusive (left + width - 1), so building widths from + // it directly lands everything one pixel short of the intended padding. + const int right = rect.right() + 1 - kPaddingX; + const int lineOneTop = rect.top() + kPaddingY; + const int lineTwoTop = lineOneTop + metrics.height(); + const int lineThreeTop = lineTwoTop + metrics.height(); + + // The date is measured first and the sender gets what is left, so a long + // sender is elided rather than painting over the date. + const int dateWidth = metrics.horizontalAdvance( + QStringLiteral("8888-88-88 88:88")); + out.dateRect = QRect(right - dateWidth, lineOneTop, dateWidth, + metrics.height()); + out.senderRect = QRect(out.contentLeft, lineOneTop, + qMax(0, out.dateRect.left() - out.contentLeft + - kPaddingX), + metrics.height()); + + // The expander is the reply count, on line two and on the right. + if (input.replyCount > 0) { + const int countWidth = smallMetrics.horizontalAdvance( + QStringLiteral("▾ 8888 replies")); + out.expanderRect = QRect(right - countWidth, lineTwoTop, countWidth, + metrics.height()); + } + + const int subjectRight = out.expanderRect.isEmpty() + ? right + : out.expanderRect.left() - kPaddingX; + out.subjectRect = QRect(out.contentLeft, lineTwoTop, + qMax(0, subjectRight - out.contentLeft), + metrics.height()); + + out.tagRect = QRect(out.contentLeft, lineThreeTop, + qMax(0, right - out.contentLeft), + smallMetrics.height()); + + return out; +} -- cgit v1.2.3 From 8767e8d33f5065ff57d7abd5bc916dbb13ada2d5 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 08:58:48 +0200 Subject: fix(view): stop clipping the date, and make the accent bar visible Both found by rendering real cards to an image and looking at them, not by any assertion. The suite was green through both. The date lost the leading digit of its year on every UNREAD card. The layout reserves the date's width from the font it is handed, which is the view's regular font, while the delegate paints with the bold one the model supplies for unread: 154px reserved against 170px needed. CardLayout now measures the date bold whatever font it is given, so the reserved width cannot be narrower than what is drawn. A few pixels are wasted on a read card, which is the cheap side of the trade. The accent bar was painted correctly and was invisible. Blending the account colour 0.35 toward the palette's Base, as the plan specified, is a fraction OF THE ACCOUNT COLOUR, so on a dark theme it produced (0.18, 0.22, 0.26) against a Base of (0.169, 0.169, 0.169): the background. The blend is dropped entirely. An account colour is already chosen to be a chip's fill carrying legible text, so it is muted to begin with, and nothing is drawn on the bar that needs that contrast. The spine keeps a blend, at 0.55, because it runs the full height of every reply in an expansion and is a different problem from a 3px edge marker. The bar is still faint at 3px on a dark theme, since the account colours are chosen as chip fills. Whether kAccentWidth needs raising cannot be settled without the user's own accounts, screen and theme; that is Task 10's open question and it is left open. --- src/carddelegate.cpp | 44 +++++++++++++++++++++++++++++++------------- src/carddelegate.h | 26 +++++++++++++++----------- src/cardlayout.cpp | 11 ++++++++++- tests/test_cardlayout.cpp | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 25 deletions(-) (limited to 'src/cardlayout.cpp') diff --git a/src/carddelegate.cpp b/src/carddelegate.cpp index bc03b5a..a3a846d 100644 --- a/src/carddelegate.cpp +++ b/src/carddelegate.cpp @@ -53,15 +53,21 @@ QColor CardDelegate::accentLineColour(const QColor &accountColour) if (!accountColour.isValid()) return ThreadListModel::threadLineColour(); - // The same 0.35 weight threadLineColour() uses, toward Base rather than - // toward Text, so the two kinds of line sit at the same visual strength. - const QColor base = QGuiApplication::palette().color(QPalette::Base); - constexpr qreal kWeight = 0.35; - const qreal inverse = 1.0 - kWeight; - return QColor::fromRgbF( - accountColour.redF() * kWeight + base.redF() * inverse, - accountColour.greenF() * kWeight + base.greenF() * inverse, - accountColour.blueF() * kWeight + base.blueF() * inverse); + // 0.35 toward Base was the first attempt and produced an INVISIBLE bar on + // a dark theme: rendered against a Base of (0.169, 0.169, 0.169) it landed + // at (0.18, 0.22, 0.26), which is the background. The weight is a fraction + // OF THE ACCOUNT COLOUR, so a low one keeps the background, not the hue. + // + // The bar is the account's colour, undiluted. Blending it toward Base at + // all was the mistake: a chip's colour is chosen to carry text on top and + // is therefore already muted, and three pixels of a muted colour on a dark + // background is nothing at all. There is no text on this bar, so nothing + // needs the contrast a chip's fill was picked for. + // + // What DOES step back is the spine, below: a line running the height of a + // whole expansion has to be followable without competing with the senders + // beside it, which is a different problem from a 3px edge marker. + return accountColour; } QSize CardDelegate::sizeHint(const QStyleOptionViewItem &option, @@ -110,10 +116,22 @@ void CardDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, if (!card.accentRect.isEmpty()) painter->fillRect(card.accentRect, lineColour); - // Spines, under everything else, in the same accent so an expanded thread - // is bounded by one colour from its root to its last reply. - for (const QRect &spine : card.spines) - painter->fillRect(spine, lineColour); + // Spines, under everything else, in the account's hue so an expanded thread + // is bounded by one colour from its root to its last reply. Muted against + // the pane's own background, unlike the accent bar: this line runs the full + // height of every reply and at full strength it shouts. + if (!card.spines.isEmpty()) { + const QColor base = + QGuiApplication::palette().color(QPalette::Base); + constexpr qreal kSpineWeight = 0.55; + const qreal inverse = 1.0 - kSpineWeight; + const QColor spineColour = QColor::fromRgbF( + lineColour.redF() * kSpineWeight + base.redF() * inverse, + lineColour.greenF() * kSpineWeight + base.greenF() * inverse, + lineColour.blueF() * kSpineWeight + base.blueF() * inverse); + for (const QRect &spine : card.spines) + painter->fillRect(spine, spineColour); + } // Selection outranks the model's foreground, and the order matters: a read // card carries a dimmed colour blended against the UNSELECTED background, diff --git a/src/carddelegate.h b/src/carddelegate.h index 317c78a..7012eaa 100644 --- a/src/carddelegate.h +++ b/src/carddelegate.h @@ -52,17 +52,21 @@ public: static QRect expanderRectFor(const QStyleOptionViewItem &option, const QModelIndex &index); - /// An account's colour as a thin LINE rather than as a chip's fill. + /// The colour the accent bar is painted in: the account's own, undiluted. /// - /// Never use the raw account colour for the accent bar or the spine. That - /// colour is chosen to be a background with legible text drawn on top - /// (TagColors::textColourOn picks black or white against it). The same - /// colour as a few pixels of line on the pane's own background is a - /// different problem: it has to be followable down a long expansion - /// WITHOUT competing with the senders beside it, which is the constraint - /// threadLineColour() states and meets by blending 0.35 toward the - /// palette's text. This blends the account colour toward the palette's - /// Base by the same weight, keeping the hue that identifies the account - /// and dropping the saturation that would shout. + /// Blending it toward the palette's Base was tried first, at the 0.35 + /// weight threadLineColour() uses, and produced an INVISIBLE bar on a dark + /// theme: against a Base of (0.169, 0.169, 0.169) it landed at (0.18, 0.22, + /// 0.26), which is the background. The weight is a fraction OF THE ACCOUNT + /// COLOUR, so a low one keeps the background rather than the hue. + /// + /// An account colour is already chosen to be a chip's fill with legible + /// text on top, so it is muted to begin with; three pixels of a muted + /// colour is nothing. Nothing is drawn on this bar, so it needs none of the + /// contrast that choice was made for. The SPINE is where the muting belongs + /// and is blended in paint(): it runs the full height of every reply in an + /// expansion and has to be followable without competing with the senders. + /// + /// Falls back to threadLineColour() for a thread with no account tag. static QColor accentLineColour(const QColor &accountColour); }; diff --git a/src/cardlayout.cpp b/src/cardlayout.cpp index 8d952b2..1a79e3b 100644 --- a/src/cardlayout.cpp +++ b/src/cardlayout.cpp @@ -87,7 +87,16 @@ CardLayout CardLayout::compute(const Input &input, const QRect &rect, // The date is measured first and the sender gets what is left, so a long // sender is elided rather than painting over the date. - const int dateWidth = metrics.horizontalAdvance( + // + // Measured BOLD whatever font this is handed. An unread card draws bold and + // the delegate computes its layout from the view's regular font, so a rect + // sized regular clips a bold date: 154px reserved against 170px needed, one + // digit of the year gone from every unread card. Reserving the wider of the + // two costs a few pixels on a read card and cannot disagree with what is + // painted. + QFont dateFont = font; + dateFont.setBold(true); + const int dateWidth = QFontMetrics(dateFont).horizontalAdvance( QStringLiteral("8888-88-88 88:88")); out.dateRect = QRect(right - dateWidth, lineOneTop, dateWidth, metrics.height()); diff --git a/tests/test_cardlayout.cpp b/tests/test_cardlayout.cpp index 1082f4c..c38f728 100644 --- a/tests/test_cardlayout.cpp +++ b/tests/test_cardlayout.cpp @@ -35,6 +35,7 @@ private slots: void dateIsFlushRight(); void threadCardCarriesAnAccentBar(); void replyCardCarriesNoAccentBar(); + void theDateFitsWhenTheCardIsBold(); }; namespace { @@ -231,5 +232,41 @@ void TestCardLayout::replyCardCarriesNoAccentBar() QCOMPARE(reply.spines.size(), 1); } +void TestCardLayout::theDateFitsWhenTheCardIsBold() +{ + // An UNREAD card draws BOLD, and bold is wider. The layout is computed from + // option.font, which is the view's regular font, while the text is painted + // with the font initStyleOption resolved from the model's Qt::FontRole. So + // a date measured regular and drawn bold overflows its rect: measured at + // 154px reserved against 170px needed, which clipped the leading digit of + // the year off every unread card. + // + // The fix is in the layout rather than in the delegate: it reserves the + // BOLD width whatever font it is handed, so the two can never disagree. + QFont regular; + regular.setBold(false); + QFont bold = regular; + bold.setBold(true); + + const QString sample = QStringLiteral("2025-08-10 06:26"); + const int boldWidth = QFontMetrics(bold).horizontalAdvance(sample); + + // Guard: bold must actually be wider here, or this asserts nothing. + QVERIFY2(boldWidth > QFontMetrics(regular).horizontalAdvance(sample), + "bold is not wider than regular in this environment, so this test " + "cannot detect the overflow it exists for"); + + const int h = CardLayout::heightFor(regular); + const CardLayout card = + CardLayout::compute(threadInput(), QRect(0, 0, 400, h), regular); + + QVERIFY2(card.dateRect.width() >= boldWidth, + qPrintable(QStringLiteral("a layout computed from the REGULAR " + "font reserves %1px, and the date needs " + "%2px when the card draws bold") + .arg(card.dateRect.width()) + .arg(boldWidth))); +} + QTEST_MAIN(TestCardLayout) #include "test_cardlayout.moc" -- cgit v1.2.3 From 93e6a533f4b0cc1a75000b2f8c77181dfc56e199 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 09:23:50 +0200 Subject: fix(ui): expand flat threads, reach the first message, localise the date Four faults from the first hand test, two of them behavioural. An expander that opened onto nothing. setThreadMessages kept only nodes with depth > 0, and notmuch_thread_get_toplevel_messages returns every message at depth 0 when a thread carries no usable In-Reply-To, so a flat thread contributed no children while its card still advertised the count. Measured in the user's database: of 396 inbox threads three are flat, one of them nine messages long, and every two-message thread of that kind was affected, which is exactly why the fault looked like "the expander only works with more than one reply". The rule is now position, not depth: every message except the first, which is the root card itself. That is also the correct rule rather than a workaround, since the row under the root is the second message however notmuch chose to nest it. The thread's first message was unreachable. Selecting a root card loaded the whole thread, so the pane showed every message with only the last expanded, and no row in the list offered the first one: the reply rows are messages two onward. The root card now renders its own message, which is what the card already claims to be. It keeps its thread id, unlike the message-row path, so mark-read and the tag-change repaint still work; that is asserted, because clearing it is the obvious way to write this and silently disables both. Before the replies are loaded the model has no first message to name and the whole thread stays the honest answer. Dates ignored the locale. One hardcoded "yyyy-MM-dd hh:mm" produced a US-looking format on an Italian desktop; QLocale::system() now formats it, and the width reserved for the date comes from the same function so a longer locale cannot clip. The expander was a bare number on the card's own background. It is a pill now, carrying "3 replies" (and "1 reply", singular), sized from the label actually drawn and measured in both glyph states so it does not resize under the pointer on click. Its fill is blended from Text toward Base rather than taken from QPalette::Button, which is #2b2b2b against a Base of #2b2b2b on the user's theme: byte identical, so the pill was invisible. A theme may make any two roles equal; a blend is defined against the surface it sits on and cannot collide with it. Checked by rendering both a dark and a light palette and looking. --- src/carddelegate.cpp | 53 ++++++++++++++++++++---- src/cardlayout.cpp | 58 +++++++++++++++++++++++--- src/cardlayout.h | 31 ++++++++++++++ src/mainwindow.cpp | 23 ++++++++++- src/threadlistmodel.cpp | 37 +++++++++++++---- src/threadlistmodel.h | 9 ++++ tests/test_cardlayout.cpp | 75 ++++++++++++++++++++++++++++++++++ tests/test_mainwindow.cpp | 39 +++++++++++++++++- tests/test_threadlistmodel.cpp | 93 +++++++++++++++++++++++++++++++++++++++--- 9 files changed, 388 insertions(+), 30 deletions(-) (limited to 'src/cardlayout.cpp') diff --git a/src/carddelegate.cpp b/src/carddelegate.cpp index a3a846d..f13205e 100644 --- a/src/carddelegate.cpp +++ b/src/carddelegate.cpp @@ -157,7 +157,7 @@ void CardDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, const QDateTime date = index.data(ThreadListModel::DateRole).toDateTime(); painter->drawText(card.dateRect, Qt::AlignVCenter | Qt::AlignRight, - date.toString(QStringLiteral("yyyy-MM-dd hh:mm"))); + CardLayout::formatDate(date)); // Line 2: the flag mark, the subject, the attachment mark. QString subject = index.data(ThreadListModel::SubjectRole).toString(); @@ -178,16 +178,51 @@ void CardDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, metrics.elidedText(line2, Qt::ElideRight, card.subjectRect.width())); - // The reply count, which is also the expander. + // The reply count, which is also the expander, drawn as a PILL. + // + // A bare "3" on the card's own background read as an unexplained number + // beside the subject and gave no hint that it could be clicked. The chip + // shape says "this is a control", matching the tag chips on line 3, and the + // word says what the number counts. if (!card.expanderRect.isEmpty()) { - painter->setFont(CardLayout::smallFont(chrome.font)); const int count = index.data(ThreadListModel::ReplyCountRole).toInt(); - const QString glyph = (option.state & QStyle::State_Open) - ? QStringLiteral("▾") - : QStringLiteral("▸"); - painter->drawText(card.expanderRect, Qt::AlignVCenter | Qt::AlignRight, - QStringLiteral("%1 %2").arg(glyph).arg(count)); - painter->setFont(chrome.font); + const QString label = CardLayout::expanderLabel( + count, option.state & QStyle::State_Open); + + painter->save(); + painter->setFont(CardLayout::smallFont(chrome.font)); + + // Blended from Text toward Base rather than taken from a palette ROLE. + // QPalette::Button is the role this obviously wants and it is + // #2b2b2b against a Base of #2b2b2b on the user's theme: byte + // identical, so the pill was invisible. A theme is free to make any two + // roles equal, and several do; a blend cannot collide with the surface + // it sits on because it is defined relative to it. + // + // Toward Text, so it darkens on a light theme and lightens on a dark + // one, the same trick replyBackground() and threadLineColour() use. + const QColor base = option.palette.color(QPalette::Base); + const QColor text = option.palette.color(QPalette::Text); + constexpr qreal kFillWeight = 0.18; + const QColor fill = QColor::fromRgbF( + text.redF() * kFillWeight + base.redF() * (1.0 - kFillWeight), + text.greenF() * kFillWeight + base.greenF() * (1.0 - kFillWeight), + text.blueF() * kFillWeight + base.blueF() * (1.0 - kFillWeight)); + painter->setRenderHint(QPainter::Antialiasing, true); + painter->setPen(Qt::NoPen); + painter->setBrush(fill); + // Fully rounded ends, the same shape TagChip paints: the radius is half + // the height, so the pill cannot look like a rectangle with soft corners. + const qreal radius = card.expanderRect.height() / 2.0; + painter->drawRoundedRect(card.expanderRect, radius, radius); + + // The pen is restored from the card's own text colour rather than + // ButtonText, which belongs to the role that just proved unreliable. + painter->setPen(option.state & QStyle::State_Selected + ? option.palette.highlightedText().color() + : text); + painter->drawText(card.expanderRect, Qt::AlignCenter, label); + painter->restore(); } painter->restore(); diff --git a/src/cardlayout.cpp b/src/cardlayout.cpp index 1a79e3b..0e118ab 100644 --- a/src/cardlayout.cpp +++ b/src/cardlayout.cpp @@ -19,6 +19,44 @@ #include "cardlayout.h" #include +#include + +QString CardLayout::formatDate(const QDateTime &date) +{ + // The system locale's own short format, not a hardcoded pattern: an + // Italian desktop writes 10/08/2025, not 2025-08-10, and a mail client + // that disagrees with every other application on screen is simply wrong. + return QLocale::system().toString(date, QLocale::ShortFormat); +} + +QString CardLayout::expanderLabel(int replyCount, bool expanded) +{ + // "3 replies", not a bare "3". The count alone reads as an unexplained + // number beside the subject, and the word is what says the card opens. + // + // Not translated through tr() here because CardLayout is a plain struct + // rather than a QObject; the delegate is where a translated build would + // wrap this, and the string is deliberately kept in one place so there is + // exactly one thing to change. + const QString glyph = expanded ? QStringLiteral("\u25be") + : QStringLiteral("\u25b8"); + const QString word = replyCount == 1 ? QStringLiteral("reply") + : QStringLiteral("replies"); + return QStringLiteral("%1 %2 %3").arg(glyph).arg(replyCount).arg(word); +} + +QString CardLayout::widestDateSample() +{ + // A real date run through the same formatter, with the wide digits and a + // two-digit day and month, so the reserved width matches what is drawn + // whatever the locale's pattern turns out to be. Guessing a pattern here + // would reintroduce the clipping this exists to prevent. + static const QString sample = [] { + const QDateTime wide(QDate(2028, 12, 28), QTime(22, 58)); + return formatDate(wide); + }(); + return sample; +} QFont CardLayout::smallFont(const QFont &cardFont) { @@ -96,8 +134,8 @@ CardLayout CardLayout::compute(const Input &input, const QRect &rect, // painted. QFont dateFont = font; dateFont.setBold(true); - const int dateWidth = QFontMetrics(dateFont).horizontalAdvance( - QStringLiteral("8888-88-88 88:88")); + const int dateWidth = + QFontMetrics(dateFont).horizontalAdvance(widestDateSample()); out.dateRect = QRect(right - dateWidth, lineOneTop, dateWidth, metrics.height()); out.senderRect = QRect(out.contentLeft, lineOneTop, @@ -105,10 +143,20 @@ CardLayout CardLayout::compute(const Input &input, const QRect &rect, - kPaddingX), metrics.height()); - // The expander is the reply count, on line two and on the right. + // The expander is the reply count as a PILL, on line two and on the right. + // + // Sized from the label actually drawn rather than from a fixed sample, so + // the background and the text inside it cannot disagree. Both states of the + // glyph are measured because the rect must not change width when the card + // is expanded: a pill that resized on click would shift the subject's + // elision under the pointer. if (input.replyCount > 0) { - const int countWidth = smallMetrics.horizontalAdvance( - QStringLiteral("▾ 8888 replies")); + const int collapsed = smallMetrics.horizontalAdvance( + expanderLabel(input.replyCount, false)); + const int expanded = smallMetrics.horizontalAdvance( + expanderLabel(input.replyCount, true)); + const int countWidth = + qMax(collapsed, expanded) + kPillPaddingX * 2; out.expanderRect = QRect(right - countWidth, lineTwoTop, countWidth, metrics.height()); } diff --git a/src/cardlayout.h b/src/cardlayout.h index d06ed92..ef6a563 100644 --- a/src/cardlayout.h +++ b/src/cardlayout.h @@ -18,6 +18,7 @@ #pragma once +#include #include #include #include @@ -116,4 +117,34 @@ struct CardLayout static CardLayout compute(const Input &input, const QRect &rect, const QFont &font); + + /// How a card writes a date, in the user's own locale. + /// + /// Never a hardcoded pattern. "yyyy-MM-dd hh:mm" is a US-looking format + /// that an Italian desktop does not use, and the whole point of asking the + /// system locale is that the user reads dates the way their desktop writes + /// them everywhere else. + /// + /// Shared with the layout so the width reserved for the date and the text + /// drawn into it come from one place: a locale whose short format is + /// longer than the reserved rect would clip, which is exactly the fault + /// bold text produced. + static QString formatDate(const QDateTime &date); + + /// The widest string formatDate() can return, for reserving space. + static QString widestDateSample(); + + /// The expander's label: the reply count with its glyph, as drawn. + /// + /// Shared with the layout for the same reason as formatDate: the rect + /// reserved for the pill and the text put inside it must come from one + /// place, or a count wider than the sample the layout guessed at spills + /// out of its own background. + /// + /// `expanded` chooses which way the triangle points. + static QString expanderLabel(int replyCount, bool expanded); + + /// Padding inside the expander pill, matching a tag chip's, so the two read + /// as the same kind of object on the card. + static constexpr int kPillPaddingX = 8; }; diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 5881fae..b7efec5 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1755,10 +1755,31 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, } const ThreadSummary thread = m_model->threadAt(current.row()); - m_currentMessageId.clear(); m_currentThreadId = thread.threadId; m_messageView->setTags(thread.tags); scheduleMarkRead(thread); + + // The root card IS the thread's first message, so selecting it renders + // that message rather than the whole conversation. Loading the thread here + // made the first message unreachable: the pane showed every message with + // only the last expanded, and no row in the list offered the first one, + // since the reply rows are messages two onward. + // + // Known only once the replies have been loaded, which happens when the + // thread is expanded. Until then the thread is the honest answer: it + // contains the first message, where a guess might not. + const QString firstId = + m_model->data(current, ThreadListModel::MessageIdRole).toString(); + if (!firstId.isEmpty()) { + m_currentMessageId = firstId; + QMetaObject::invokeMethod(m_worker, "loadMessage", + Qt::QueuedConnection, + Q_ARG(QString, firstId), + Q_ARG(quint64, m_generation)); + return; + } + + m_currentMessageId.clear(); QMetaObject::invokeMethod(m_worker, "loadThread", Qt::QueuedConnection, Q_ARG(QString, m_currentThreadId), Q_ARG(QString, m_lastQuery), diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index d8412f9..bdc7e96 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -365,8 +365,13 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const if (role == IsMessageRole) return false; - if (role == MessageIdRole) - return QString(); + if (role == MessageIdRole) { + // The thread's FIRST message, once known, because the root card is + // that message: selecting it renders one message rather than the whole + // conversation. Empty before the replies are loaded, which is the + // caller's signal to load the thread instead of guessing at a message. + return m_threads.at(index.row()).first.messageId; + } if (role == MessageDepthRole) return 0; @@ -542,7 +547,7 @@ void ThreadListModel::appendBatch(const QVector &batch) const int first = m_threads.size(); beginInsertRows({}, first, first + batch.size() - 1); for (const ThreadSummary &summary : batch) - m_threads.append(ThreadNode{ summary, {}, false }); + m_threads.append(ThreadNode{ summary, {}, {}, false }); endInsertRows(); } @@ -570,12 +575,26 @@ void ThreadListModel::setThreadMessages(const QString &threadId, endRemoveRows(); } - QVector children; - children.reserve(nodes.size()); - for (const MessageNode &node : nodes) { - if (node.depth > 0) - children.append(node); - } + // Every message EXCEPT the first, which is the root card itself. + // + // Selecting on depth > 0 instead was wrong, and wrong in a way that + // only showed on real mail: notmuch_thread_get_toplevel_messages + // returns every message at depth 0 when a thread carries no usable + // In-Reply-To, so a flat thread contributed no children at all. The + // card advertised "3 replies" and expanded onto nothing. Measured in + // the user's database: of 396 inbox threads, three are flat, one of + // them nine messages long, and every two-message thread of this kind + // was affected, which is why the fault looked like "the expander only + // works with more than one reply". + // + // Position also happens to be the right rule rather than a workaround. + // The root card IS the thread's first message, so the row under it is + // the second message whatever depth notmuch assigns it. + QVector children = nodes.mid(1); + + // Kept so the root card can render its own message. It is the card the + // user clicks to read the thread's opening message. + m_threads[row].first = nodes.isEmpty() ? MessageNode() : nodes.first(); if (!children.isEmpty()) { beginInsertRows(parent, 0, children.size() - 1); diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index 9cb774e..f381ffa 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -240,6 +240,15 @@ private: ThreadSummary summary; QVector children; ///< Empty until the thread is expanded. + /// The thread's FIRST message, which the root card itself draws. + /// + /// Kept because the root card is that message: selecting it must + /// render one message rather than the whole conversation, and without + /// this the first message of every thread is unreachable, since the + /// only rows offering a message are the replies and it is not one of + /// them. Empty until the replies are loaded. + MessageNode first; + /// Distinguishes "this thread has no replies" from "its replies have /// not been asked for yet". Without it an expander would be drawn over /// every thread, including the ones that turn out to be single diff --git a/tests/test_cardlayout.cpp b/tests/test_cardlayout.cpp index c38f728..48bba25 100644 --- a/tests/test_cardlayout.cpp +++ b/tests/test_cardlayout.cpp @@ -19,6 +19,7 @@ #include "cardlayout.h" #include +#include #include class TestCardLayout : public QObject @@ -32,10 +33,12 @@ private slots: void indentStopsAtTheCap(); void expanderSitsOnTheSecondLine(); void expanderIsEmptyWithoutReplies(); + void theExpanderReadsAsAPillWithAWord(); void dateIsFlushRight(); void threadCardCarriesAnAccentBar(); void replyCardCarriesNoAccentBar(); void theDateFitsWhenTheCardIsBold(); + void theDateFollowsTheSystemLocale(); }; namespace { @@ -183,6 +186,42 @@ void TestCardLayout::expanderIsEmptyWithoutReplies() QVERIFY(card.expanderRect.isEmpty()); } +void TestCardLayout::theExpanderReadsAsAPillWithAWord() +{ + // A bare "3" beside the subject reads as an unexplained number and gives + // no hint that it can be clicked. The label carries the word, and the rect + // carries padding for the pill drawn behind it. + QCOMPARE(CardLayout::expanderLabel(3, false), + QStringLiteral("\u25b8 3 replies")); + QCOMPARE(CardLayout::expanderLabel(3, true), + QStringLiteral("\u25be 3 replies")); + + // Singular, because "1 replies" is the kind of detail that makes an + // interface look unfinished. + QCOMPARE(CardLayout::expanderLabel(1, false), + QStringLiteral("\u25b8 1 reply")); + + const QFont font; + const int h = CardLayout::heightFor(font); + const CardLayout card = + CardLayout::compute(threadInput(), QRect(0, 0, 400, h), font); + const QFontMetrics small(CardLayout::smallFont(font)); + + // The rect must hold the label AND its padding, or the pill's background + // is narrower than the text sitting on it. + QVERIFY2(card.expanderRect.width() + >= small.horizontalAdvance(CardLayout::expanderLabel(3, false)) + + CardLayout::kPillPaddingX * 2, + "the expander rect is too narrow for its own label and padding"); + + // And it must NOT change width when the card opens: a pill that resized on + // click would shift the subject's elision under the pointer. + CardLayout::Input open = threadInput(); + const CardLayout expanded = + CardLayout::compute(open, QRect(0, 0, 400, h), font); + QCOMPARE(expanded.expanderRect.width(), card.expanderRect.width()); +} + void TestCardLayout::dateIsFlushRight() { const QFont font; @@ -232,6 +271,42 @@ void TestCardLayout::replyCardCarriesNoAccentBar() QCOMPARE(reply.spines.size(), 1); } +void TestCardLayout::theDateFollowsTheSystemLocale() +{ + const QDateTime when(QDate(2025, 8, 10), QTime(6, 26)); + + // The system locale's own rendering, whatever it is. Asserting a specific + // string would only restate the hardcoded pattern this replaced, and would + // fail on any machine but the one that wrote it. + QCOMPARE(CardLayout::formatDate(when), + QLocale::system().toString(when, QLocale::ShortFormat)); + + // The specific fault: an ISO-looking pattern on a desktop that does not + // use one. Guarded so this test says nothing on a locale that genuinely + // formats that way. + if (QLocale::system().toString(when, QLocale::ShortFormat) + != QStringLiteral("2025-08-10 06:26")) { + QVERIFY2(CardLayout::formatDate(when) + != QStringLiteral("2025-08-10 06:26"), + "the date is hardcoded to yyyy-MM-dd hh:mm rather than " + "following the desktop's locale"); + } + + // And the reserved width has to follow the same formatter, or a locale + // whose dates are longer clips them exactly as the bold font did. + QFont font; + const int h = CardLayout::heightFor(font); + const CardLayout card = + CardLayout::compute(threadInput(), QRect(0, 0, 400, h), font); + QFont bold = font; + bold.setBold(true); + QVERIFY2(card.dateRect.width() + >= QFontMetrics(bold).horizontalAdvance( + CardLayout::formatDate(when)), + "the reserved date width is narrower than this locale's own " + "formatting of a date"); +} + void TestCardLayout::theDateFitsWhenTheCardIsBold() { // An UNREAD card draws BOLD, and bold is wider. The layout is computed from diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index cdb08ca..922705c 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -105,6 +105,7 @@ private slots: void childRowsAreIndentedUnderTheirThread(); void aThreadWithRepliesDrawsAVisibleExpander(); void cardsNeverScrollSideways(); + void selectingARootCardKeepsItsThreadForMarkRead(); void nextThreadLeavesTheLastReply(); void altDownSkipsReplies(); void bothThreadStepBindingsReachTheAction(); @@ -667,8 +668,12 @@ NavFixture buildNavFixture(MainWindow &window) f.view = window.findChild(); f.model = window.findChild(); + // unread, so a selection arms the mark-read timer: scheduleMarkRead() + // returns early for a thread that is already read, and a fixture without + // it would make a mark-read assertion pass for the wrong reason. ThreadSummary first = makeThread(QStringLiteral("T1"), - QStringList{ QStringLiteral("inbox") }); + QStringList{ QStringLiteral("inbox"), + QStringLiteral("unread") }); first.totalCount = 2; ThreadSummary second = makeThread(QStringLiteral("T2"), QStringList{ QStringLiteral("inbox") }); @@ -693,6 +698,38 @@ NavFixture buildNavFixture(MainWindow &window) } // namespace +void TestMainWindow::selectingARootCardKeepsItsThreadForMarkRead() +{ + // A root card is BOTH a message and a thread: it renders the thread's + // first message, and it is still the thread that gets marked read and + // repainted on a tag change. The message-row path deliberately clears the + // current thread id; doing that here too would silently disable mark-read + // and the tag-change repaint for every thread root in the list. + const Config config; + MainWindow window(config); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const NavFixture f = buildNavFixture(window); + f.view->setCurrentIndex(f.root); + QApplication::processEvents(); + + // Guard: the fixture loads replies, so the root knows its own message and + // the branch under test is the one that runs. + QVERIFY2(!f.model->data(f.root, ThreadListModel::MessageIdRole) + .toString().isEmpty(), + "the root card does not know its first message, so this exercises " + "the fallback rather than the path it is written for"); + + // A mark-read timer armed for the thread is what proves the thread id + // survived: scheduleMarkRead() is only reached on the thread-row path. + auto *timer = window.findChild(QStringLiteral("markReadTimer")); + QVERIFY(timer); + QVERIFY2(timer->isActive(), + "no mark-read timer for a selected root card: its thread id was " + "cleared along with the switch to rendering one message"); +} + void TestMainWindow::nextThreadLeavesTheLastReply() { const Config config; diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index 49b8894..aa71080 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -32,6 +32,8 @@ private slots: void repliesBecomeChildRowsUnderTheirThread(); void messageRowsShowTheirOwnSenderAndSubject(); void modelHasOneColumn(); + void aFlatThreadStillListsItsReplies(); + void theRootCardKnowsItsOwnMessage(); void replyShowsOnlyItsOwnTags(); void replySharingEveryThreadTagShowsNone(); void reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem(); @@ -131,10 +133,13 @@ void TestThreadListModel::repliesBecomeChildRowsUnderTheirThread() QCOMPARE(model.data(child, ThreadListModel::ThreadIdRole).toString(), QStringLiteral("t1")); - // A thread root is not a message row and carries no message id. + // A thread root is not a message ROW, but it does carry a message id: the + // root card is the thread's first message, and selecting it renders that + // message alone. It used to answer nothing here, which is what made the + // first message of every thread unreachable. QVERIFY(!model.data(root, ThreadListModel::IsMessageRole).toBool()); - QVERIFY(model.data(root, ThreadListModel::MessageIdRole) - .toString().isEmpty()); + QCOMPARE(model.data(root, ThreadListModel::MessageIdRole).toString(), + QStringLiteral("m0@example.org")); QAbstractItemModelTester tester( &model, QAbstractItemModelTester::FailureReportingMode::Warning); @@ -1022,6 +1027,72 @@ void TestThreadListModel::modelHasOneColumn() QCOMPARE(index.data(ThreadListModel::ReplyCountRole).toInt(), 0); } +void TestThreadListModel::aFlatThreadStillListsItsReplies() +{ + // A thread whose messages carry no reply structure: notmuch returns them + // all from get_toplevel_messages at depth 0, which is what happens when the + // mail has no usable In-Reply-To. Measured in the user's own database: + // of 396 inbox threads, three are like this, one of them nine messages + // deep, and every one of them showed a reply count that expanded to + // nothing because the model kept only nodes with depth > 0. + ThreadListModel model; + ThreadSummary thread = makeThread(QStringLiteral("t1"), + QStringLiteral("flat thread")); + thread.totalCount = 3; + model.appendBatch({ thread }); + + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 0), + makeNode(QStringLiteral("m2@example.org"), 0) }); + + const QModelIndex root = model.index(0, 0); + + // Two children, not zero: the FIRST message is the root card itself, and + // the rest are its replies however flat the thread is. + QCOMPARE(model.rowCount(root), 2); + QCOMPARE(model.index(0, 0, root).data(ThreadListModel::MessageIdRole) + .toString(), + QStringLiteral("m1@example.org")); + + // And the count the card advertises must agree with the rows beneath it, + // or the expander opens onto nothing. + QCOMPARE(root.data(ThreadListModel::ReplyCountRole).toInt(), + model.rowCount(root)); +} + +void TestThreadListModel::theRootCardKnowsItsOwnMessage() +{ + // The root card IS the thread's first message, so it has to be able to say + // which message that is. Without this the pane renders the whole thread + // when the root is selected, and the first message is unreachable: the + // only rows offering it are the replies, and it is not one of them. + ThreadListModel model; + ThreadSummary thread = makeThread(QStringLiteral("t1"), + QStringLiteral("a subject")); + thread.totalCount = 2; + model.appendBatch({ thread }); + + const QModelIndex root = model.index(0, 0); + + // Before the replies are loaded there is nothing to report, and the caller + // must fall back to loading the whole thread rather than a wrong message. + QVERIFY(root.data(ThreadListModel::MessageIdRole).toString().isEmpty()); + + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) }); + + QCOMPARE(root.data(ThreadListModel::MessageIdRole).toString(), + QStringLiteral("m0@example.org")); + + // And it is the FIRST message, not just any of them: the reply must still + // report its own. + QCOMPARE(model.index(0, 0, root).data(ThreadListModel::MessageIdRole) + .toString(), + QStringLiteral("m1@example.org")); +} + void TestThreadListModel::replyShowsOnlyItsOwnTags() { ThreadListModel model; @@ -1040,7 +1111,14 @@ void TestThreadListModel::replyShowsOnlyItsOwnTags() // Two the thread already has, one it does not. reply.tags = { QStringLiteral("inbox"), QStringLiteral("work"), QStringLiteral("todo") }; - model.setThreadMessages(QStringLiteral("T1"), { reply }); + + // Led by the thread's FIRST message, which is what the worker sends and + // what the root card draws. setThreadMessages drops it by position. + MessageNode root; + root.messageId = QStringLiteral("M1"); + root.threadId = QStringLiteral("T1"); + root.depth = 0; + model.setThreadMessages(QStringLiteral("T1"), { root, reply }); const QModelIndex threadIndex = model.index(0, 0); QVERIFY(model.hasChildren(threadIndex)); @@ -1073,7 +1151,12 @@ void TestThreadListModel::replySharingEveryThreadTagShowsNone() reply.threadId = QStringLiteral("T1"); reply.depth = 1; reply.tags = { QStringLiteral("inbox"), QStringLiteral("work") }; - model.setThreadMessages(QStringLiteral("T1"), { reply }); + + MessageNode root; + root.messageId = QStringLiteral("M1"); + root.threadId = QStringLiteral("T1"); + root.depth = 0; + model.setThreadMessages(QStringLiteral("T1"), { root, reply }); const QModelIndex replyIndex = model.index(0, 0, model.index(0, 0)); QVERIFY(replyIndex.isValid()); -- cgit v1.2.3 From e1dba2987a9a1e87b92801959df9c9d4f1375d2f Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 09:30:37 +0200 Subject: fix(view): indent a flat thread's replies like any other A reply in a thread with no usable In-Reply-To carries depth 0, because that is how notmuch reports every message of such a thread. CardLayout read depth 0 as "not nested", so those replies drew flush against their own thread with no spine, while a nested thread's replies indented normally: the list showed two different shapes for the same relationship, side by side. A MESSAGE row is nested at least one level whatever depth it reports. Being a child row IS the nesting; the depth only says how much further to go. This is the third fault from the same root. The depth numbering was trusted to mean structure when it only ever meant "how notmuch happened to thread this": first it hid a flat thread's replies entirely, then it left the first message unreachable, and now it drew the survivors without their indent. --- src/cardlayout.cpp | 12 +++++++++++- tests/test_cardlayout.cpp | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) (limited to 'src/cardlayout.cpp') diff --git a/src/cardlayout.cpp b/src/cardlayout.cpp index 0e118ab..f542df0 100644 --- a/src/cardlayout.cpp +++ b/src/cardlayout.cpp @@ -103,7 +103,17 @@ CardLayout CardLayout::compute(const Input &input, const QRect &rect, // Indent, capped. qMin rather than a branch so depth 5 and depth 50 land // in exactly the same place. - const int depth = qMin(input.depth, kMaxDepth); + // + // A MESSAGE row is nested at least one level whatever depth it reports. + // notmuch numbers every message of a thread with no usable In-Reply-To as + // depth 0, so a flat thread's replies arrived here claiming no nesting and + // drew flush against their own thread with no spine, while a nested + // thread's replies indented normally: two different shapes on screen for + // the same relationship. Being a child row IS the nesting; the depth only + // says how much further to go. + const int effectiveDepth = + input.isMessage ? qMax(1, input.depth) : input.depth; + const int depth = qMin(effectiveDepth, kMaxDepth); const int indent = depth * kIndentStep; out.contentLeft = textLeft + kPaddingX + indent; diff --git a/tests/test_cardlayout.cpp b/tests/test_cardlayout.cpp index 48bba25..fdc18bf 100644 --- a/tests/test_cardlayout.cpp +++ b/tests/test_cardlayout.cpp @@ -30,6 +30,7 @@ private slots: void everyCardIsTheSameHeight(); void threeLinesStackWithoutOverlapping(); void replyIndentsByDepth(); + void aDepthZeroReplyStillIndents(); void indentStopsAtTheCap(); void expanderSitsOnTheSecondLine(); void expanderIsEmptyWithoutReplies(); @@ -139,6 +140,41 @@ void TestCardLayout::replyIndentsByDepth() } } +void TestCardLayout::aDepthZeroReplyStillIndents() +{ + // A reply in a FLAT thread carries depth 0, because notmuch reports every + // message of a thread with no usable In-Reply-To as a top-level message. + // It is still a reply: it is a child row under the root card, and it has + // to read as one. + // + // Treating depth 0 as "no nesting" left those replies flush against their + // thread with no spine, while a nested thread's replies indented normally, + // so the list showed two different shapes for the same relationship. + const QFont font; + const int h = CardLayout::heightFor(font); + const QRect rect(0, 0, 400, h); + + CardLayout::Input flatReply; + flatReply.isMessage = true; + flatReply.depth = 0; + + const CardLayout root = CardLayout::compute(threadInput(), rect, font); + const CardLayout reply = CardLayout::compute(flatReply, rect, font); + + QVERIFY2(reply.contentLeft > root.contentLeft, + "a depth-0 reply sits flush with its thread, so a flat thread's " + "replies look like more threads"); + QVERIFY2(!reply.spines.isEmpty(), + "a depth-0 reply has no spine, so nothing joins it to the thread " + "above it"); + + // And it lands at the same place a depth-1 reply does: the two are the + // same relationship and notmuch's numbering is the only difference. + const CardLayout nested = CardLayout::compute(replyInput(1), rect, font); + QCOMPARE(reply.contentLeft, nested.contentLeft); + QCOMPARE(reply.spines.size(), nested.spines.size()); +} + void TestCardLayout::indentStopsAtTheCap() { const QFont font; -- cgit v1.2.3