diff options
| -rw-r--r-- | CHANGELOG.md | 6 | ||||
| -rw-r--r-- | src/carddelegate.cpp | 6 | ||||
| -rw-r--r-- | src/cardlayout.cpp | 33 | ||||
| -rw-r--r-- | src/cardlayout.h | 17 | ||||
| -rw-r--r-- | src/threadlistmodel.cpp | 18 | ||||
| -rw-r--r-- | src/threadlistmodel.h | 8 | ||||
| -rw-r--r-- | src/threadlistview.cpp | 4 | ||||
| -rw-r--r-- | src/threadlistview.h | 2 | ||||
| -rw-r--r-- | src/types.h | 6 | ||||
| -rw-r--r-- | tests/test_cardlayout.cpp | 24 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 9 | ||||
| -rw-r--r-- | tests/test_threadlistmodel.cpp | 39 | ||||
| -rw-r--r-- | translations/qtmaildir_it_IT.ts | 11 |
13 files changed, 111 insertions, 72 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 56d98e4..4bdc536 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -115,6 +115,12 @@ point at which they are stable. removes the row at once while an automatic one waits until you move on. Reading the last unread message of a long conversation is the one case that still waits for the next query or sync. +- **A conversation's card counts its MESSAGES, not its replies.** The pill + that opens the thread read "N replies" against the total the row actually + lists: a thread of one message and four replies said "4 replies" over rows + for all five messages. It now reads "5 messages", which is what the + conversation is. A thread of one message still shows no count at all, since + there is nothing for the pill to open. - **Delete and Restore appear only where they apply.** Delete is hidden on mail already in the trash, where it reported success and did nothing, and Restore is hidden on mail that was never deleted. diff --git a/src/carddelegate.cpp b/src/carddelegate.cpp index fbdf13f..02f1df0 100644 --- a/src/carddelegate.cpp +++ b/src/carddelegate.cpp @@ -40,7 +40,7 @@ CardLayout::Input inputFor(const QModelIndex &index) CardLayout::Input in; in.isMessage = index.data(ThreadListModel::IsMessageRole).toBool(); in.depth = index.data(ThreadListModel::MessageDepthRole).toInt(); - in.replyCount = index.data(ThreadListModel::ReplyCountRole).toInt(); + in.messageCount = index.data(ThreadListModel::MessageCountRole).toInt(); in.dateFormat = index.data(ThreadListModel::DateFormatRole).toString(); // Item 70's marks. The layout reserves a rect for each, so these have to @@ -305,14 +305,14 @@ void CardDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, drawMark(card.receivedForwardRect, Marks::Mark::ReceivedForward); drawMark(card.repliedRect, Marks::Mark::Replied); - // The reply count, which is also the expander, drawn as a PILL. + // The message 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()) { - const int count = index.data(ThreadListModel::ReplyCountRole).toInt(); + const int count = index.data(ThreadListModel::MessageCountRole).toInt(); const QString label = CardLayout::expanderLabel( count, option.state & QStyle::State_Open); diff --git a/src/cardlayout.cpp b/src/cardlayout.cpp index ec22635..f89d3ec 100644 --- a/src/cardlayout.cpp +++ b/src/cardlayout.cpp @@ -18,6 +18,7 @@ #include "cardlayout.h" +#include <QCoreApplication> #include <QFontMetrics> #include <QLocale> @@ -52,9 +53,9 @@ int CardLayout::markSide(const QFont &font) return qMax(8, side); } -QString CardLayout::expanderLabel(int replyCount, bool expanded) +QString CardLayout::expanderLabel(int messageCount, bool expanded) { - // "3 replies", not a bare "3". The count alone reads as an unexplained + // "5 messages", not a bare "5". The count alone reads as an unexplained // number beside the subject, and the word is what says the card opens. // // The triangle is NO LONGER part of this string. Item 70 made it a drawn @@ -65,14 +66,20 @@ QString CardLayout::expanderLabel(int replyCount, bool expanded) // when the card opens, and a caller that stopped passing the state would // hide that requirement rather than satisfy it. // - // 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. + // CardLayout is a plain struct rather than a QObject, so this cannot use + // tr(); QCoreApplication::translate() with an explicit context is the + // equivalent, and the literal context string is what lupdate extracts + // under. The word is translated rather than the whole "%1 %2", because + // %n's untranslated fallback on this Qt does not pluralise (measured: + // "%n message(s)" stays literally "(s)"), and the two forms read cleanly + // in the .ts. Q_UNUSED(expanded); - const QString word = replyCount == 1 ? QStringLiteral("reply") - : QStringLiteral("replies"); - return QStringLiteral("%1 %2").arg(replyCount).arg(word); + const QString word = messageCount == 1 + ? QCoreApplication::translate("CardLayout", + "message") + : QCoreApplication::translate("CardLayout", + "messages"); + return QStringLiteral("%1 %2").arg(messageCount).arg(word); } QString CardLayout::widestDateSample(const QString &format) @@ -196,18 +203,18 @@ CardLayout CardLayout::compute(const Input &input, const QRect &rect, - kPaddingX), metrics.height()); - // The expander is the reply count as a PILL, on line two and on the right. + // The expander is the message 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) { + if (input.messageCount > 0) { const int collapsed = smallMetrics.horizontalAdvance( - expanderLabel(input.replyCount, false)); + expanderLabel(input.messageCount, false)); const int expanded = smallMetrics.horizontalAdvance( - expanderLabel(input.replyCount, true)); + expanderLabel(input.messageCount, true)); // The triangle is a drawn mark since item 70, so the pill has to // reserve its width explicitly. It came free from the text metrics // while it was a glyph in the label, which is exactly the kind of diff --git a/src/cardlayout.h b/src/cardlayout.h index 57edca7..9620900 100644 --- a/src/cardlayout.h +++ b/src/cardlayout.h @@ -35,7 +35,7 @@ /// The card is three lines, always: /// /// sender ................................ date <- senderRect/dateRect -/// * subject @ v 3 replies <- subjectRect/expanderRect +/// * subject @ v 5 messages <- subjectRect/expanderRect /// [tag] [tag] <- tagRect struct CardLayout { @@ -45,7 +45,7 @@ struct CardLayout { bool isMessage = false; int depth = 0; ///< 0 for a thread root, 1 for a direct reply. - int replyCount = 0; ///< 0 means no expander. + int messageCount = 0; ///< Messages in the conversation; 0 means no expander. /// A QDateTime::toString() pattern from [general] date_format, or empty /// for the system's short format. @@ -105,8 +105,8 @@ struct CardLayout QRect subjectRect; QRect tagRect; - /// The reply count's rect, and the click target that toggles the thread. - /// Empty when the row has no replies. + /// The message count's rect, and the click target that toggles the thread. + /// Empty when the row has no expander. QRect expanderRect; /// The flagged mark, at the start of line two before the subject. Empty @@ -197,15 +197,18 @@ struct CardLayout /// The widest string formatDate() can return, for reserving space. static QString widestDateSample(const QString &format = QString()); - /// The expander's label: the reply count with its glyph, as drawn. + /// The expander's label: the message count with its word, 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); + /// `expanded` chooses which way the triangle points. The word is + /// translated: a pill reading a foreign count in English next to an + /// otherwise local interface is a tiny broken window, and this is the one + /// user-facing string this plain struct owns. + static QString expanderLabel(int messageCount, 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. diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 8bb9849..5c44dd4 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -439,7 +439,7 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const m_forwardPrefixes); case IsRepliedRole: return node.isReplied(); - case ReplyCountRole: + case MessageCountRole: // A reply never offers an expander: nesting past the first level is // drawn from depth, not from further parent-child structure. return 0; @@ -714,15 +714,19 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const m_forwardPrefixes); case IsRepliedRole: return thread.isReplied(); - case ReplyCountRole: + case MessageCountRole: // Zero in a flat list, so the card draws no expander pill. The count - // and hasChildren() must agree: a card advertising "3 replies" that - // cannot be opened is the inert-glyph defect this project has already - // shipped once. + // and hasChildren() must agree: a card advertising a count for a thread + // that cannot be opened is the inert-glyph defect this project has + // already shipped once. if (m_flatMode) return 0; - // totalCount includes the root message, which is the card itself. - return qMax(0, thread.totalCount - 1); + // The number of MESSAGES in the conversation, not the replies alone: + // the row stands for the conversation now (item 177), so a thread of + // one message and four replies reads "5 messages". A thread of one + // shows nothing: its row is the message, there is nothing to expand, + // and the pill is the expander. + return thread.totalCount > 1 ? thread.totalCount : 0; case DateFormatRole: return m_dateFormat; default: diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index 93474ac..fee4a1f 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -83,7 +83,8 @@ public: MessageIdRole, /// The message's reply depth, for the view's indentation. 1 for a - /// direct reply, since depth 0 is the root row itself. + /// direct reply; 0 is the thread's first message, which sits under the + /// conversation row unindented. MessageDepthRole, /// True when the row is a thread that has replies to show. @@ -138,7 +139,8 @@ public: /// bool; the message was replied to, from the Maildir "R" flag. IsRepliedRole, - ReplyCountRole, ///< int; 0 when a thread has no replies. + MessageCountRole, ///< int; messages in the conversation, 0 when the + ///< thread has one message and nothing to expand. /// The [general] date_format pattern, or empty for the system's short /// format. Same row value for every row. @@ -207,7 +209,7 @@ public: /// that is what keeps it from leaking: it is off by default, only the Sent /// button turns it on, and every other query turns it off again. The /// expander already comes from hasChildren() and the card's count from - /// ReplyCountRole, so flat mode is those two answering differently and + /// MessageCountRole, so flat mode is those two answering differently and /// nothing else changes. /// /// The children are not discarded, only hidden. Leaving flat mode restores diff --git a/src/threadlistview.cpp b/src/threadlistview.cpp index 5a3ce77..019d7c9 100644 --- a/src/threadlistview.cpp +++ b/src/threadlistview.cpp @@ -27,10 +27,10 @@ void ThreadListView::mousePressEvent(QMouseEvent *event) { const QModelIndex index = indexAt(event->pos()); - // The reply count IS the expander. Anything outside its rect selects the + // The message count IS the expander. Anything outside its rect selects the // card and opens it, which is what the rest of the card is for. if (event->button() == Qt::LeftButton && index.isValid() - && index.data(ThreadListModel::ReplyCountRole).toInt() > 0) { + && index.data(ThreadListModel::MessageCountRole).toInt() > 0) { QStyleOptionViewItem option; initViewItemOption(&option); diff --git a/src/threadlistview.h b/src/threadlistview.h index 3215153..a509389 100644 --- a/src/threadlistview.h +++ b/src/threadlistview.h @@ -40,7 +40,7 @@ public: using QTreeView::QTreeView; protected: - /// Toggles a thread when its reply count is clicked. + /// Toggles a thread when its message count is clicked. /// /// Being VISIBLE and being CLICKABLE are separate properties: /// setRootIsDecorated(false), needed to stop the style drawing its own diff --git a/src/types.h b/src/types.h index 8e24cd1..ee2627f 100644 --- a/src/types.h +++ b/src/types.h @@ -190,9 +190,9 @@ struct MessageNode QStringList tags; QString filePath; - /// Reply depth within the thread. 0 is the thread's first message, which - /// occupies the ROOT row rather than a child row: the user's model is - /// "N replies", so a thread of 7 shows 1 root and 6 descendants. + /// Reply depth within the thread. 0 is the thread's first message: since + /// item 177 a conversation lists it as a child row too, but it carries no + /// nesting. 1 is a direct reply. int depth = 0; bool isUnread() const { return tags.contains(QStringLiteral("unread")); } diff --git a/tests/test_cardlayout.cpp b/tests/test_cardlayout.cpp index c81296f..09afc5f 100644 --- a/tests/test_cardlayout.cpp +++ b/tests/test_cardlayout.cpp @@ -58,7 +58,7 @@ CardLayout::Input threadInput() CardLayout::Input in; in.isMessage = false; in.depth = 0; - in.replyCount = 3; + in.messageCount = 3; return in; } @@ -67,7 +67,7 @@ CardLayout::Input replyInput(int depth) CardLayout::Input in; in.isMessage = true; in.depth = depth; - in.replyCount = 0; + in.messageCount = 0; return in; } @@ -91,7 +91,7 @@ void TestCardLayout::everyCardIsTheSameHeight() const CardLayout deepReply = CardLayout::compute(replyInput(3), QRect(0, 0, 400, thread), font); CardLayout::Input noRepliesIn = threadInput(); - noRepliesIn.replyCount = 0; + noRepliesIn.messageCount = 0; const CardLayout noReplies = CardLayout::compute(noRepliesIn, QRect(0, 0, 400, thread), font); @@ -225,7 +225,7 @@ void TestCardLayout::expanderIsEmptyWithoutReplies() const QFont font; const int h = CardLayout::heightFor(font); CardLayout::Input in = threadInput(); - in.replyCount = 0; + in.messageCount = 0; const CardLayout card = CardLayout::compute(in, QRect(0, 0, 400, h), font); QVERIFY(card.expanderRect.isEmpty()); @@ -240,12 +240,12 @@ void TestCardLayout::theExpanderReadsAsAPillWithAWord() // NO triangle in the label since item 70: it is a drawn mark now, and a // glyph left here would be a second triangle beside the drawn one. The // label is the words alone, and the state no longer changes it. - QCOMPARE(CardLayout::expanderLabel(3, false), QStringLiteral("3 replies")); - QCOMPARE(CardLayout::expanderLabel(3, true), QStringLiteral("3 replies")); + QCOMPARE(CardLayout::expanderLabel(3, false), QStringLiteral("3 messages")); + QCOMPARE(CardLayout::expanderLabel(3, true), QStringLiteral("3 messages")); - // Singular, because "1 replies" is the kind of detail that makes an + // Singular, because "1 messages" is the kind of detail that makes an // interface look unfinished. - QCOMPARE(CardLayout::expanderLabel(1, false), QStringLiteral("1 reply")); + QCOMPARE(CardLayout::expanderLabel(1, false), QStringLiteral("1 message")); // The glyphs are gone from the label entirely. Asserted rather than assumed, // because a stray one would draw underneath the mark and look like a @@ -377,7 +377,11 @@ void TestCardLayout::marksDoNotCollideWithEachOtherOrTheExpander() // densest line two can get. Nothing may overlap anything. const QFont font; const int h = CardLayout::heightFor(font); - const QRect rect(0, 0, 400, h); + // 460 rather than 400: the pill reads "3 messages" now, a character wider + // than "3 replies" was, and 400 left the elastic subject narrower than the + // avatar gutter with all four marks out. The width is a stress value, not + // a spec. + const QRect rect(0, 0, 460, h); CardLayout::Input in = threadInput(); in.flagged = true; @@ -421,7 +425,7 @@ void TestCardLayout::marksDoNotCollideWithEachOtherOrTheExpander() // not leave the subject narrower than the avatar gutter beside it. QVERIFY2(card.subjectRect.width() > card.avatarRect.width(), "four marks left the subject narrower than the avatar gutter on a " - "400px card"); + "460px card"); } void TestCardLayout::dateIsFlushRight() diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index aecefb7..2e7d220 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -1396,10 +1396,11 @@ void TestMainWindow::aThreadWithRepliesDrawsAVisibleExpander() const QModelIndex first = model->index(0, 0, QModelIndex()); const QModelIndex second = model->index(1, 0, QModelIndex()); - // Guards: the model agrees about which thread has replies, and only that - // one is offered an expander at all. - QCOMPARE(model->data(first, ThreadListModel::ReplyCountRole).toInt(), 2); - QCOMPARE(model->data(second, ThreadListModel::ReplyCountRole).toInt(), 0); + // Guards: the model agrees about which thread has an expander, and only + // that one is offered one at all. The count is MESSAGES: the three-message + // thread reads 3, the lone message reads 0. + QCOMPARE(model->data(first, ThreadListModel::MessageCountRole).toInt(), 3); + QCOMPARE(model->data(second, ThreadListModel::MessageCountRole).toInt(), 0); const QFont font = view->font(); const int height = CardLayout::heightFor(font); diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index fba2d2d..28d3a8b 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -49,7 +49,7 @@ private slots: void appendingEmptyBatchIsNoOp(); void clearResetsModel(); void reportsSubjectAndAuthors(); - void theReplyCountExcludesTheRootMessage(); + void thePillCountsTheThreadsMessages(); void unreadThreadsRenderBold(); void readThreadsAreDimmedAndUnreadAreNot(); void flaggedThreadsShowAStar(); @@ -519,7 +519,7 @@ void TestThreadListModel::aFlatViewsAvatarFollowsTheRecipient() QStringLiteral("me@example.org")); } -void TestThreadListModel::theReplyCountExcludesTheRootMessage() +void TestThreadListModel::thePillCountsTheThreadsMessages() { ThreadListModel model; @@ -530,12 +530,14 @@ void TestThreadListModel::theReplyCountExcludesTheRootMessage() model.appendBatch({ single, multi }); // The count used to be a "(4)" suffix on the subject. It is the expander - // on the card's second line now, and it counts REPLIES: totalCount - // includes the root message, which is the card itself. + // on the card's second line now, and it counts MESSAGES: the row stands + // for the conversation since item 177, so a thread of one message and + // three replies reads "4 messages". A thread of one shows nothing to + // expand, so its count stays 0. QCOMPARE(model.data(model.index(0, 0), - ThreadListModel::ReplyCountRole).toInt(), 0); + ThreadListModel::MessageCountRole).toInt(), 0); QCOMPARE(model.data(model.index(1, 0), - ThreadListModel::ReplyCountRole).toInt(), 3); + ThreadListModel::MessageCountRole).toInt(), 4); // And the subject is bare, with no count spliced into it. QCOMPARE(model.data(model.index(1, 0), @@ -1483,9 +1485,9 @@ void TestThreadListModel::modelHasOneColumn() QStringLiteral("alice@example.org")); QVERIFY(index.data(ThreadListModel::DateRole).toDateTime().isValid()); - // A single-message thread offers no expander: totalCount includes the root - // message, which is the card itself. - QCOMPARE(index.data(ThreadListModel::ReplyCountRole).toInt(), 0); + // A single-message thread offers no expander: it is its own message and + // has nothing to open onto. + QCOMPARE(index.data(ThreadListModel::MessageCountRole).toInt(), 0); } void TestThreadListModel::aFlatThreadStillListsItsReplies() @@ -1520,12 +1522,11 @@ void TestThreadListModel::aFlatThreadStillListsItsReplies() .toString(), QStringLiteral("m1@example.org")); - // The pill counts REPLIES while the rows are MESSAGES, so since item 177 - // the rows are one more than the pill: the conversation lists its first - // message too. They must still move together, or the expander opens onto a - // number the card never promised. - QCOMPARE(root.data(ThreadListModel::ReplyCountRole).toInt(), - model.rowCount(root) - 1); + // The pill counts MESSAGES, which is exactly what the rows are: since item + // 177 a conversation lists every message under itself, so the number on + // the card and the number of rows it opens onto agree. + QCOMPARE(root.data(ThreadListModel::MessageCountRole).toInt(), + model.rowCount(root)); } void TestThreadListModel::theRootCardKnowsItsOwnMessage() @@ -1893,7 +1894,7 @@ void TestThreadListModel::flatModeOffersNoExpanderAndNoReplyCount() // the replies they received under a view that claims to be their outbox. // // Deliberately not a second model or a filtered query. The expander is - // driven by hasChildren() and the card's count by ReplyCountRole, both + // driven by hasChildren() and the card's count by MessageCountRole, both // already here, so flat mode is those two answering differently. ThreadListModel model; model.appendBatch({ makeThread(QStringLiteral("t1"), @@ -1906,13 +1907,13 @@ void TestThreadListModel::flatModeOffersNoExpanderAndNoReplyCount() // below: a test whose subject was already flat would pass either way. QVERIFY2(model.hasChildren(thread), "the fixture thread is not expandable, so this proves nothing"); - QCOMPARE(model.data(thread, ThreadListModel::ReplyCountRole).toInt(), 1); + QCOMPARE(model.data(thread, ThreadListModel::MessageCountRole).toInt(), 2); model.setFlatMode(true); QVERIFY2(!model.hasChildren(thread), "a flat list still offered an expander"); - QCOMPARE(model.data(thread, ThreadListModel::ReplyCountRole).toInt(), 0); + QCOMPARE(model.data(thread, ThreadListModel::MessageCountRole).toInt(), 0); // rowCount has to agree, or the view draws an expander it cannot open, or // opens onto rows the card said were not there. @@ -1943,7 +1944,7 @@ void TestThreadListModel::flatModeIsOffByDefaultAndReversible() model.setFlatMode(false); QVERIFY2(model.hasChildren(thread), "leaving flat mode did not restore the tree"); - QCOMPARE(model.data(thread, ThreadListModel::ReplyCountRole).toInt(), 1); + QCOMPARE(model.data(thread, ThreadListModel::MessageCountRole).toInt(), 2); } void TestThreadListModel::recipientsReplaceTheSenderWhenPresent() diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index d158380..5249149 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -2,6 +2,17 @@ <!DOCTYPE TS> <TS version="2.1" language="it_IT"> <context> + <name>CardLayout</name> + <message> + <source>message</source> + <translation>messaggio</translation> + </message> + <message> + <source>messages</source> + <translation>messaggi</translation> + </message> +</context> +<context> <name>ComposeWindow</name> <message> <source>Compose[*]</source> |