From 9ae43f94f5d822efe582e79b47b2f5407161a38b Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Wed, 26 Aug 2026 17:50:18 +0200 Subject: fix: correct the avatar initials, the two-tone fill and the fade Hand-testing item 169 found four defects, three of them visible on every card. The initials were taken from whatever the card's first line held, which is the raw From header on a reply row and notmuch's comma-joined author summary on a thread row. A naive space split therefore gave `T<` for `tsujan ` and one letter each from two different people for `Standreas, tsujan`, and a separator counted as a word, so `INE - Expert IT Training` drew `I-`. Avatar::initialsFor() now normalises first: the angle-addr and any quoting go, a comma takes the first entry unless the name is quoted, a bare address is not a name, and a word has to carry a letter or a digit. Avatar::fillFor() uses the same normalisation, so an address in the name's place no longer reads as a person. The two-tone fill built its gradient axis as a radius from the centre, so the 0.5 colour stop landed on the squircle's edge and one hue filled almost the whole face. The axis spans the diameter now. The fade ran left to right, which put its hard stop at 60% of the card and read as a slab rather than a wash. It runs right to left: opaque at the card's right edge, where the only hard stop is the card's own boundary, and gone before it reaches the accent bar that already states the account. And the flat views hashed the user's own address on every row, so every Sent and Drafts card shared one pattern. ThreadSummary::firstMessageRecipient rides the recipient fold, which already parses the To header, and SenderAddressRole prefers it, falling back to the sender when there is no usable To. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01P88Q3MCSCSQxKDy7pmXh9F --- tests/test_avatar.cpp | 115 +++++++++++++++++++++++++++++++++++++++++ tests/test_carddelegate.cpp | 23 ++++++--- tests/test_notmuchworker.cpp | 30 +++++++++++ tests/test_threadlistmodel.cpp | 31 +++++++++++ 4 files changed, 192 insertions(+), 7 deletions(-) (limited to 'tests') diff --git a/tests/test_avatar.cpp b/tests/test_avatar.cpp index 72e5941..cea88b3 100644 --- a/tests/test_avatar.cpp +++ b/tests/test_avatar.cpp @@ -30,10 +30,14 @@ private slots: void bareAddressTakesLocalAndDomain(); void nothingUsableFallsBackToTheAccountLabel(); void initialsAreAlwaysTwoLetters(); + void aRawFromHeaderIsNotSplitOnItsBracket(); + void aCommaJoinedAuthorListTakesTheFirstAuthor(); + void aSeparatorIsNotAWord(); void aDisplayNameMeansAPerson(); void theListOverridesADisplayName(); void aColourIsStablePerAddress(); void aPixmapIsStableAndDiffersPerSeed(); + void bothTwoToneHuesReachTheFace(); }; void TestAvatar::twoWordNameTakesOneLetterFromEach() @@ -95,6 +99,61 @@ void TestAvatar::initialsAreAlwaysTwoLetters() } } +void TestAvatar::aRawFromHeaderIsNotSplitOnItsBracket() +{ + // A reply row's first line is the RAW header, so a naive space split gave + // the name's first letter and a literal `<`. + QCOMPARE(Avatar::initialsFor( + QStringLiteral("tsujan "), + QStringLiteral("notifications@github.com"), + QStringLiteral("Work")), + QStringLiteral("TS")); + // A bare address in the name's place is not a name: the address branch + // answers, rather than the local part's first two letters. + QCOMPARE(Avatar::initialsFor(QStringLiteral("info@moomhotel.com"), + QStringLiteral("info@moomhotel.com"), + QStringLiteral("Work")), + QStringLiteral("IM")); + // And the fill agrees: neither of those is a display name. + QCOMPARE(Avatar::fillFor(QStringLiteral("info@moomhotel.com"), false), + Avatar::Fill::TwoTone); +} + +void TestAvatar::aCommaJoinedAuthorListTakesTheFirstAuthor() +{ + // notmuch's author summary joins participants with a comma, so one letter + // from each gave initials belonging to two different people. + QCOMPARE(Avatar::initialsFor(QStringLiteral("Standreas, tsujan"), + QStringLiteral("notifications@github.com"), + QStringLiteral("Work")), + QStringLiteral("ST")); + // A QUOTED name may legally contain a comma and must survive whole. + QCOMPARE(Avatar::initialsFor(QStringLiteral("\"Rossi, Mario\""), + QStringLiteral("m@example.org"), + QStringLiteral("Work")), + QStringLiteral("RM")); +} + +void TestAvatar::aSeparatorIsNotAWord() +{ + // `INE - Expert IT Training` took the dash as its second word and drew + // `I-`. A word has to carry a letter or a digit. + QCOMPARE(Avatar::initialsFor(QStringLiteral("INE - Expert IT Training"), + QStringLiteral("news@example.org"), + QStringLiteral("Work")), + QStringLiteral("IE")); + // Leading punctuation is trimmed rather than disqualifying the word. + QCOMPARE(Avatar::initialsFor(QStringLiteral("(Acme) Support"), + QStringLiteral("s@example.org"), + QStringLiteral("Work")), + QStringLiteral("AS")); + // Punctuation ONLY is no name at all: the address answers. + QCOMPARE(Avatar::initialsFor(QStringLiteral("- ---"), + QStringLiteral("news@example.org"), + QStringLiteral("Work")), + QStringLiteral("NE")); +} + void TestAvatar::aDisplayNameMeansAPerson() { // The case the user asked for by name: a corporate address that presents @@ -149,5 +208,61 @@ void TestAvatar::aPixmapIsStableAndDiffersPerSeed() QVERIFY(first.toImage() != twoTone.toImage()); } +void TestAvatar::bothTwoToneHuesReachTheFace() +{ + // The split has to cross the squircle, not graze its edge. Building the + // gradient axis as a RADIUS from the centre put the 0.5 stop on the + // boundary, so one hue filled almost the whole face and the fill read as + // flat: reported against noreply@cofidis.it. A colour count is what + // distinguishes the two, since both versions paint every pixel. + // + // Several seeds, because one unlucky angle proves nothing either way. + const QStringList seeds { QStringLiteral("noreply@cofidis.it"), + QStringLiteral("a@example.org"), + QStringLiteral("b@example.org"), + QStringLiteral("c@example.org") }; + for (const QString &seed : seeds) { + const QImage face = + Avatar::pixmapFor(seed, QStringLiteral("XX"), + Avatar::Fill::TwoTone, 64, QFont()).toImage(); + + // The two hues, as painted. Sampled by counting pixels of each rather + // than probing a corner: which corner gets which hue depends on the + // hashed angle. + const QColor base = Avatar::colourFor(seed); + const QColor dark = base.darker(135); + int light = 0, shade = 0; + for (int y = 0; y < face.height(); ++y) { + for (int x = 0; x < face.width(); ++x) { + const QColor pixel = face.pixelColor(x, y); + if (pixel.alpha() < 255) + continue; // The squircle's antialiased corners. + // Nearest of the two, not an exact match: the gradient + // interpolates in premultiplied space and the pixel format + // rounds, so an exact compare finds NEITHER hue and the probe + // reports 0 against 0 whatever the code does. + const int toBase = qAbs(pixel.red() - base.red()) + + qAbs(pixel.green() - base.green()) + + qAbs(pixel.blue() - base.blue()); + const int toDark = qAbs(pixel.red() - dark.red()) + + qAbs(pixel.green() - dark.green()) + + qAbs(pixel.blue() - dark.blue()); + if (toBase < toDark) + ++light; + else + ++shade; + } + } + + // A fifth of the face each: enough that neither is a sliver, loose + // enough that the hashed angle is free to put the split anywhere. + const int fifth = face.width() * face.height() / 5; + QVERIFY2(light > fifth && shade > fifth, + qPrintable(QStringLiteral("%1: one hue took the face, %2 " + "light against %3 dark") + .arg(seed).arg(light).arg(shade))); + } +} + QTEST_MAIN(TestAvatar) #include "test_avatar.moc" diff --git a/tests/test_carddelegate.cpp b/tests/test_carddelegate.cpp index 8b53309..bb21ae1 100644 --- a/tests/test_carddelegate.cpp +++ b/tests/test_carddelegate.cpp @@ -267,7 +267,9 @@ void TestCardDelegate::theFadeEndsAtSixtyPercentOfTheCard() { const QRect card(0, 0, 500, 60); const QRect root = CardDelegate::fadeRectFor(card, QRect()); - QCOMPARE(root.left(), card.left()); + // Anchored at the card's RIGHT edge: the hard stop belongs where the card + // ends, not 60% across it, which read as a slab. + QCOMPARE(root.right(), card.right()); QCOMPARE(root.width(), 300); } @@ -276,14 +278,21 @@ void TestCardDelegate::aReplyFadeStartsAtItsOwnSpine() const QRect card(0, 0, 500, 60); // The innermost spine of a nested reply, which is its own coloured border. const QRect spine(80, 0, 2, 60); - const QRect reply = CardDelegate::fadeRectFor(card, spine); - - // It hangs off the spine, not off the card's edge. - QCOMPARE(reply.left(), spine.left()); - // And still ends at 60% of the CARD, so a deeper reply's wash is shorter - // as well as further right. + // A spine deep enough to cut into the wash, which starts at 40% here. + const QRect deep(300, 0, 2, 60); + const QRect reply = CardDelegate::fadeRectFor(card, deep); + + // Clamped at the spine, so the wash never runs under a reply's own border. + QCOMPARE(reply.left(), deep.left()); + // Still anchored at the card's right edge, so a deeper reply's wash is + // shorter rather than displaced. QCOMPARE(reply.right(), CardDelegate::fadeRectFor(card, QRect()).right()); QVERIFY(reply.width() < CardDelegate::fadeRectFor(card, QRect()).width()); + + // A shallow spine sits left of where the wash begins and changes nothing. + const QRect shallow(80, 0, 2, 60); + QCOMPARE(CardDelegate::fadeRectFor(card, shallow), + CardDelegate::fadeRectFor(card, QRect())); } void TestCardDelegate::theDelegateAsksForAScaledSquircle() diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index 4fa8629..9a0896d 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -76,6 +76,7 @@ private slots: void recipientsAreAbsentUnlessAskedFor(); void recipientsAreFoldedWhenAskedFor(); void recipientsCrossAQueuedCall(); + void theFirstRecipientsAddressCrossesForTheAvatar(); void requestCountsAnswersOneCountPerQuery(); void requestCountsKeepsPositionOnAnInvalidQuery(); @@ -1078,6 +1079,35 @@ void TestNotmuchWorker::recipientsAreFoldedWhenAskedFor() "two plus one: %1").arg(summary))); } +void TestNotmuchWorker::theFirstRecipientsAddressCrossesForTheAvatar() +{ + // Item 169's flat-view avatar. `recipients` is a DISPLAY summary and + // carries no address at all when every recipient has a name, so the hash + // needs the bare one; it rides the same fold, so it costs nothing extra. + const QVector one = + runQuery(QStringLiteral("subject:Preventivo"), + NotmuchWorker::NewestFirst, true); + QCOMPARE(one.size(), 1); + QCOMPARE(one.at(0).firstMessageRecipient, + QStringLiteral("mario@example.org")); + + // A quoted display name containing a comma must not defeat the parse, for + // the same reason it must not defeat the summary. + const QVector many = + runQuery(QStringLiteral("subject:Riunione"), + NotmuchWorker::NewestFirst, true); + QCOMPARE(many.size(), 1); + QCOMPARE(many.at(0).firstMessageRecipient, + QStringLiteral("mario@example.org")); + + // And it stays empty when the query never asked, exactly as `recipients` + // does: it is behind the same performance contract. + const QVector unasked = + runQuery(QStringLiteral("subject:Preventivo")); + QCOMPARE(unasked.size(), 1); + QVERIFY(unasked.at(0).firstMessageRecipient.isEmpty()); +} + void TestNotmuchWorker::recipientsCrossAQueuedCall() { // The trap CLAUDE.md records for SortOrder, in the shape it takes for this diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index 5c10b6c..9ca35e3 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -104,6 +104,7 @@ private slots: void recipientsReplaceTheSenderWhenPresent(); void aRowCarriesItsSenderAndAccountAddress(); void aMessageRowCarriesItsOwnSenderAndAddress(); + void aFlatViewsAvatarFollowsTheRecipient(); }; static ThreadSummary makeThread(const QString &id, const QString &subject) @@ -560,6 +561,36 @@ void TestThreadListModel::aMessageRowCarriesItsOwnSenderAndAddress() QStringLiteral("Bob ")); } +void TestThreadListModel::aFlatViewsAvatarFollowsTheRecipient() +{ + // In a Sent or Drafts view firstMessageSender is the USER on every row, so + // hashing it gives one pattern for the whole list. The recipient is what + // the row is about, and SendersRole already follows the same rule. + ThreadListModel model; + ThreadSummary summary; + summary.threadId = QStringLiteral("t1"); + summary.subject = QStringLiteral("Subject"); + summary.authors = QStringLiteral("Me"); + summary.firstMessageId = QStringLiteral("m1"); + summary.firstMessageSender = QStringLiteral("me@example.org"); + summary.recipients = QStringLiteral("John Doe"); + summary.firstMessageRecipient = QStringLiteral("john@example.org"); + model.appendBatch({ summary }); + + QCOMPARE(model.index(0, 0).data(ThreadListModel::SenderAddressRole) + .toString(), + QStringLiteral("john@example.org")); + + // No usable To: the sender is the fallback rather than a blank seed. + ThreadListModel bare; + summary.recipients.clear(); + summary.firstMessageRecipient.clear(); + bare.appendBatch({ summary }); + QCOMPARE(bare.index(0, 0).data(ThreadListModel::SenderAddressRole) + .toString(), + QStringLiteral("me@example.org")); +} + void TestThreadListModel::theReplyCountExcludesTheRootMessage() { ThreadListModel model; -- cgit v1.2.3