diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-10 09:23:50 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-10 09:23:50 +0200 |
| commit | 93e6a533f4b0cc1a75000b2f8c77181dfc56e199 (patch) | |
| tree | 7b62bbd049602314bbfd2fcc1207794dc5423ded /tests | |
| parent | 8767e8d33f5065ff57d7abd5bc916dbb13ada2d5 (diff) | |
| download | qtmaildir-93e6a533f4b0cc1a75000b2f8c77181dfc56e199.tar.gz qtmaildir-93e6a533f4b0cc1a75000b2f8c77181dfc56e199.zip | |
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.
Diffstat (limited to 'tests')
| -rw-r--r-- | tests/test_cardlayout.cpp | 75 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 39 | ||||
| -rw-r--r-- | tests/test_threadlistmodel.cpp | 93 |
3 files changed, 201 insertions, 6 deletions
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 <QFont> +#include <QLocale> #include <QTest> 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<QTreeView *>(); f.model = window.findChild<ThreadListModel *>(); + // 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<QTimer *>(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()); |
