diff options
| -rw-r--r-- | CHANGELOG.md | 53 | ||||
| -rw-r--r-- | CLAUDE.md | 119 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 30 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-08-item-20-message-rows.md | 9 | ||||
| -rw-r--r-- | src/CMakeLists.txt | 2 | ||||
| -rw-r--r-- | src/carddelegate.cpp | 261 | ||||
| -rw-r--r-- | src/carddelegate.h | 72 | ||||
| -rw-r--r-- | src/cardlayout.cpp | 186 | ||||
| -rw-r--r-- | src/cardlayout.h | 150 | ||||
| -rw-r--r-- | src/keymap.cpp | 36 | ||||
| -rw-r--r-- | src/keymap.h | 9 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 507 | ||||
| -rw-r--r-- | src/mainwindow.h | 98 | ||||
| -rw-r--r-- | src/notmuchworker.cpp | 164 | ||||
| -rw-r--r-- | src/notmuchworker.h | 41 | ||||
| -rw-r--r-- | src/tagchip.cpp | 176 | ||||
| -rw-r--r-- | src/tagchip.h | 42 | ||||
| -rw-r--r-- | src/threadlistmodel.cpp | 499 | ||||
| -rw-r--r-- | src/threadlistmodel.h | 185 | ||||
| -rw-r--r-- | src/threadlistview.cpp | 141 | ||||
| -rw-r--r-- | src/threadlistview.h | 39 | ||||
| -rw-r--r-- | src/types.h | 58 | ||||
| -rw-r--r-- | tests/CMakeLists.txt | 1 | ||||
| -rw-r--r-- | tests/test_cardlayout.cpp | 383 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 1059 | ||||
| -rw-r--r-- | tests/test_notmuchworker.cpp | 140 | ||||
| -rw-r--r-- | tests/test_threadlistmodel.cpp | 681 |
27 files changed, 4294 insertions, 847 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index fdbb08f..5bf9256 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,29 @@ point at which they are stable. ## [Unreleased] +### Changed + +- **The thread list is now a list of cards rather than a table of columns.** + Each thread shows its sender and date, its subject with the flag, attachment + and reply-count marks, and its tags, on three lines at one uniform height. + Expanding a thread shows its replies indented under a continuous spine, + carrying only the tags the thread itself does not have, and without the `Re:` + prefix every reply used to repeat. +- **Threads can be listed newest or oldest first**, from a new control beside + the query bar. The choice is remembered between sessions. +- **An account's colour now runs down the left edge of its threads**, and down + the spine of their replies, replacing the account chip that used to sit in + front of every subject. The account dropdown shows the same colours, so which + colour means which account is readable in one place. +- **Alt+Up and Alt+Down step between threads**, alongside the existing Ctrl+J + and Ctrl+K. Plain Up and Down now step message by message through an expanded + thread, which is the view's own behaviour rather than a binding. +- **An out-of-range `message_zoom` now says so.** The documented 0.5 to 3.0 + range was already enforced on the way to the web view, so a `message_zoom` of + 500 rendered at 3.0 rather than unusably, but nothing reported that the value + in the file was not the value on screen. It is now listed with the other + configuration problems at startup. + ### Fixed - **The message pane no longer comes back as a sliver.** A splitter position is @@ -19,13 +42,18 @@ point at which they are stable. left, in one real case 29px. The pane now has a minimum width and cannot be collapsed, which covers the restore and the equivalent drag. -### Changed +- **Clicking a thread no longer scrolls the list sideways.** A card is exactly + the width of the pane, so there is nowhere to scroll to. +- **Next and previous thread no longer step onto a reply** when a thread is + expanded. They skip message rows, so they keep meaning thread-to-thread. -- **An out-of-range `message_zoom` now says so.** The documented 0.5 to 3.0 - range was already enforced on the way to the web view, so a `message_zoom` of - 500 rendered at 3.0 rather than unusably, but nothing reported that the value - in the file was not the value on screen. It is now listed with the other - configuration problems at startup. +### Upgrading + +- **Saved thread-list column widths are ignored.** There is one column now, so + the `threadlist/header` and `threadlist/columns` entries in + `~/.local/state/qtmaildir/uistate.conf` no longer do anything. Nothing needs + to be done: they are read past and can be left in place or deleted. Window + geometry, the splitter position and the message zoom are unaffected. ## [0.12.1] - 2026-08-09 @@ -62,6 +90,19 @@ if you leave it alone: ### Added +- **Threads expand in the list to show their replies.** A thread with more than + one message carries an expander; opening it lists the replies as indented rows + beneath it, marked with a thread line, a tinted background and smaller text. + The replies are fetched when you expand, not with the query, so a large result + still paints immediately. +- **Selecting a reply opens that message on its own**, rather than the whole + conversation, which is the point of having message rows at all. +- **Actions follow what you selected, and the status bar says what they will + touch.** A thread row acts on the whole thread and reports "1 thread selected + (7 messages)" before and "(whole thread)" after; a reply row acts on that one + message. Both are undoable. There is no confirmation dialog, deliberately: + undo is this application's answer to a mistaken action, and naming the scope + is what makes it usable. - **A sync now fetches only the accounts you have edited.** Tagging mail in one account and syncing no longer pulls every other account as well. A sync with nothing outstanding is a plain fetch and still covers everything, since @@ -42,10 +42,11 @@ UI thread Worker thread MainWindow NotmuchWorker ├ query row: QComboBox, QLineEdit, └ owns the only notmuch_database_t* │ saved-query QPushButtons - ├ ThreadListView ── ThreadListModel - │ (RowStyleDelegate every column, SubjectDelegate on Subject) + ├ ThreadListView (QTreeView) ── ThreadListModel (QAbstractItemModel) + │ ONE column of cards; CardDelegate paints each whole, from CardLayout └ MessageView (header QLabel, QWebEngineView, attachment bar, TagStrip) +CardLayout (pure geometry, no painting) Config (INI) KeyMap MailSync (QProcess) MimeParser (GMime) SyncMonitor (/proc/locks) TagColors QueryCompleter ThreadCidMap ``` @@ -55,27 +56,111 @@ The query row and the message-pane header are **built inline in `MainWindow` and listed `QueryBar`, `SavedQueryBar`, `HeaderWidget` and `AttachmentBar`; none of those types have ever existed, and looking for them wastes a search. The widget classes that do exist are `MessageView`, `ThreadListView`, `TagStrip`, -`TagDialog`, `RowStyleDelegate` and `SubjectDelegate`; `TagChip` is a namespace -of painting helpers, not a widget, and `ThreadCidMap` is a struct. - -**`ThreadListView` exists because a delegate cannot paint outside its column.** -The tag chips under each row are one strip spanning the whole width, so they -are drawn in the view's `paintEvent` after the cells. Consequences that are -easy to undo by accident: `SubjectDelegate` reads `AccountLabelRole`, which -belongs to the ROW, so installing it view-wide draws the account chip into -every column (a `Q_ASSERT` catches this); row height must be set on the -vertical header, since a table takes one height per row and a column's -`sizeHint` only applies if the view happens to ask that column; and because -alternating colours, the selection and the model's `BackgroundRole` are all -painted per cell, the view has to fill the strip's band itself, honouring all -three or a deleted row is cut in half and every other row shows a bare stripe. +`TagDialog`, `RowStyleDelegate` and `CardDelegate`; `TagChip` is a namespace of +painting helpers, not a widget, and `ThreadCidMap` and `CardLayout` are structs. +`SubjectDelegate` existed until item 53 and is gone. + +**`ThreadListView` survives only for the expander hit-test.** `CardDelegate` +draws the reply count, and a delegate gets no click of its own without an +editor, so the view owns the click and asks the delegate for the rect rather +than recomputing it. + +Until item 53 it also painted a row-wide strip of tag chips after the cells, +because a delegate cannot paint outside its column and the strip spanned all +five. That is why the class exists at all, and the history is worth keeping: +the arithmetic it needed produced a deleted row cut in half and every other row +showing a bare stripe, both because the view had to re-honour alternating +colours, the selection and `BackgroundRole` across cells it did not own. With +one column there is nothing to span, so the `paintEvent` and its band +arithmetic are deleted and none of that applies any more. + +**A card layout must be testable without a painter.** `CardLayout` computes +every rect on a card and touches no `QPainter` and no widget, so the geometry +has tests that a blank render cannot defeat. When changing what a card shows, +change `CardLayout` and assert there; a test that renders the delegate and +counts pixels proves nothing, for the reasons under "Rendering probes lie". +Two traps it already handles: `QRect::right()` is inclusive, so the right edge +is carried as an exclusive one, and `QFont::pointSizeF()` returns -1 for a font +set in pixels, which qt6ct does. + +**`QTreeView`'s Up/Down already walk into an expanded thread's replies**, and +that is where message-to-message navigation comes from. Do not bind arrow keys +as `QAction` shortcuts to get it: a shortcut is dispatched before the focused +widget sees the key and Qt withholds only plain LETTERS from editable widgets, +so a bare `Up` would break the query bar, the tag dialog and the web view at +once. `Alt+Up`/`Alt+Down` are chords and therefore safe; `Shift+Up`/`Down` is +the built-in extend-selection and must be left alone. Binding two sequences to +one action needs `setShortcuts`, not `setShortcut`, which keeps only the last. + +**`Q_ENUM` is not enough to send an enum across a queued connection.** It gives +the type a meta-object entry, not a metatype registered under the name +`invokeMethod` resolves, so a `Q_ARG` carrying it is dropped at runtime with a +warning and the slot runs with a default. `NotmuchWorker::SortOrder` is +registered beside the type for this reason, not in `MainWindow`, so a caller +that never constructs one still gets it. + +**It is a `QTreeView` over a `QAbstractItemModel` since item 20**, because a +thread's replies are child rows and a table can neither indent nor expand. What +did NOT survive that port is anything keyed on a row NUMBER: a tree numbers rows +per parent, so `row 0` exists once per expanded thread and `current.row() + 1` +names a sibling rather than the next thread. Navigation walks with +`indexBelow`/`indexAbove`; `QTableView::isRowSelected(int)` has no equivalent — +use `selectionModel()->isSelected(index)`. Row height comes from +`setUniformRowHeights` plus `CardDelegate::sizeHint`, since a tree has no +vertical header to carry a default section size. Indentation is +`setIndentation(0)`: `CardLayout` draws the indent inside the card's own rect, +so `visualRect` reports the SAME left edge for a thread and its reply and a +geometry probe sees no nesting in a correctly nested list. + +**Three traps in the expander, all of which shipped a plausible-looking broken +build before being caught.** `QTreeView::drawBranches` is the documented hook and +does not work when the expander sits on a content column: it runs BEFORE the +row's cells, so the delegate's background paints over it (a 60-pixel triangle +survived as 8). `CardDelegate` draws it instead, as the reply count on the +card's second line. `setRootIsDecorated(false)`, needed to stop the style +drawing its own indicator underneath, also removes the style's HIT AREA, so the +glyph renders perfectly and is inert; `ThreadListView::mousePressEvent` handles +the click, asking `CardDelegate::expanderRectFor` for the target so the drawn +and clickable rects cannot drift. A fourth trap died with the grid: `isExpanded` +is keyed on column 0, which used to disagree with the subject-column index. + +**Visible, clickable and toggling are three separate properties.** A test for +one passes against the other two being broken, which happened twice in one +session: a pixel test proved the triangle was drawn while nothing could click +it, and a click test proved it opened while it could never close. + +**Assert a reply's indent on where the TEXT lands, never on `visualRect`.** The +reason has inverted twice and the rule has not. Under item 20 the geometry was +indented while the text was not, because the delegate laid text out from its own +left edge; now `setIndentation(0)` means `visualRect` reports no indent at all +while the text is indented, because `CardLayout` draws it inside the card's rect. +A probe on `visualRect` therefore endorsed a broken layout then and would fail a +correct one now. Assert on `CardLayout::contentLeft`. + +**`paintEvent` ran AFTER the cells**, which is why anything the view filled +across a row covered the text the delegate had just drawn: the reply tint filled +the full row height in one version and erased every sender and subject, measured +at zero surviving text pixels. Recorded because it is the class of bug a view +that paints invites. `ThreadListView` no longer paints at all. **No `notmuch_*` pointer ever crosses the thread boundary.** Data crosses as the plain -value structs in `src/types.h` (`ThreadSummary`, `MessageRef`, `TagChange`), over queued +value structs in `src/types.h` (`ThreadSummary`, `MessageRef`, `MessageNode`, +`ActionScope`, `TagChange`), over queued signals in both directions. `notmuchworker.cpp` is the only file that includes `notmuch.h` outside `src/nmraii.h`; C handles are owned by the `NmQuery`/`NmMessages`/`NmThread`/… RAII aliases there so they cannot leak. +**The one exception, and it is a double-free if undone.** Messages reached +through `notmuch_thread_get_toplevel_messages` / `notmuch_message_get_replies` +are owned by the THREAD and freed with it (`notmuch.h:1637`), so `walkReplies` +in `notmuchworker.cpp` holds them as raw `notmuch_message_t*`: an `NmMessage` +wrapper would call `notmuch_message_destroy` on memory the thread frees again. +The whole walk must finish while the `NmThread` is alive. Related: replies are +unreachable from a query walk at all — `notmuch_message_get_replies` returns +NULL for a message from `notmuch_query_search_messages` (`notmuch.h:1617-1628`), +which is why `loadThreadTree` exists beside `loadThread` rather than replacing +it. + **Generation counters, not cancellation.** Each query bumps a `quint64` generation passed through to the worker and back on every result signal. The UI discards results whose generation is stale. The worker never needs to know a query was superseded. Threads are diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index 17cf434..ad6dbc0 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -77,7 +77,7 @@ taking that too literally. | 17 | No completion for tags in the query bar | workflow | M | **done** | | 18 | No visual cue that there are unsynced edits | feedback | S | **done** | | 19 | No prompt to sync on exit when edits are pending | behavior | S | **done** | -| 20 | Thread view does not match the user's mental model | presentation | L | built on branch `item-20-message-rows`, **not merged**; see 53 | +| 20 | Thread view does not match the user's mental model | presentation | L | rebuilt as cards on branch `card-list`, **not merged**; awaiting the user's verdict | | 21 | Default shortcuts are not sensible enough | discoverability | S | open | | 22 | Translatability audit and i18n wiring | correctness | M | open | | 23 | No way to save a search query from the UI | workflow | M | open | @@ -108,16 +108,16 @@ taking that too literally. | 48 | Removing a tag suggests every tag, not the thread's own | workflow | XS | **done** | | 49 | Sync runs every account regardless of what changed | workflow | M | **done** | | 50 | Esc blanks the pane but leaves the row selected | workflow | XS | **done** | -| 51 | Clicking a subject scrolls the list sideways | presentation | XS | open; resolved as a side effect of 53, do not work separately | +| 51 | Clicking a subject scrolls the list sideways | presentation | XS | built on branch `card-list`, **not merged**; a card is viewport width | | 52 | `test_querycompleter` fails under Wayland, passes offscreen | testing | XS | **done** | -| 53 | Message rows still read as a table, not as a conversation | presentation | M | open, specified 2026-08-09; see the card-list spec | +| 53 | Message rows still read as a table, not as a conversation | presentation | M | built 2026-08-10 on branch `card-list`, **not merged**; awaiting the user's verdict | | 54 | A cron sync carries the edits but the count still says pending | correctness | S | **done** | | 55 | In a narrow window the message pane is invisible | presentation | XS | **done** | | 56 | No action carries an icon, so the toolbar reserves space for nothing | presentation | S | **done** | | 57 | "Flag" would read better as "Important" or "Starred" | presentation | XS | **done** | | 58 | `message_zoom` documents a 0.5 to 3.0 range and enforces none of it | correctness | XS | **done** | | 59 | Archive and Mark all read shipped with the same icon | presentation | XS | **done** | -| 60 | Next thread dead-ends on the last reply of an expanded thread | defect | XS | open; branch only, fix as part of 53 | +| 60 | Next thread dead-ends on the last reply of an expanded thread | defect | XS | **done** on the branch; already fixed by 5487d58, see below | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -3683,11 +3683,23 @@ navigation actions against the same rule. current index, which follow visible rows across parent boundaries. For thread-to-thread jumping, skip any index whose `IsMessageRole` is true. -**Specified as part of the card-list spec** -(`docs/superpowers/specs/2026-08-09-card-list-design.md`), which also adds -Alt+Up/Down for these actions and relies on `QTreeView`'s built-in Up/Down for -stepping through replies. Fix it there rather than separately, unless the card -list is dropped. +**The cause above is wrong, and was corrected on 2026-08-10.** It was read off +`master`, where the arithmetic really is `current.row() + 1`. The branch does +not do that: `5487d58` added `MainWindow::threadRowOf()`, which walks up to the +containing thread BEFORE the arithmetic, so from the last reply of an expanded +thread `next_thread` already reached the next thread. The defect was fixed in +the same commit that could have introduced it, one commit before this entry was +written. Verified by writing both failing tests first, on the branch, and +watching them pass against unchanged code. + +The entry is kept rather than deleted, because the reasoning it records is +sound and the tests it demanded now exist. It is a reminder that a cause +"verified in code" is only verified against the branch it was read on. + +**Superseded by the card-list work all the same.** Both actions now walk with +`indexBelow`/`indexAbove` (`card-list`, 2026-08-10), so nothing in that path is +keyed on a row number, which is the rule a deeper tree would break next. +Alt+Up/Down were added alongside Ctrl+J/K there. **Constraint.** The test that would catch this must start from the **last reply of an expanded thread**. A test that arrows down a collapsed list passes against diff --git a/docs/superpowers/plans/2026-08-08-item-20-message-rows.md b/docs/superpowers/plans/2026-08-08-item-20-message-rows.md index 5935777..f4882c7 100644 --- a/docs/superpowers/plans/2026-08-08-item-20-message-rows.md +++ b/docs/superpowers/plans/2026-08-08-item-20-message-rows.md @@ -1,14 +1,5 @@ # Item 20: Message Rows in the Thread List — Implementation Plan -> **EXECUTED AND PARKED, 2026-08-08. Do not run this plan again.** -> Every task here was implemented on the branch `item-20-message-rows`, which is -> pushed to both remotes and **not merged**. master has these documents and none -> of that code. The user's verdict on the finished result was that the table -> layout does not fit the use, and item 53 in -> `2026-08-03-post-0.1.0-usability.md` carries the diagnosis. This document is -> kept for the API facts it verified and the reasoning it records, not as work -> to pick up. Read item 53 first. - > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. **Goal:** Turn the flat thread list into a tree where a thread's root row is its first message, expanding reveals the replies indented by reply depth, and selecting a reply opens that single message in the reading pane. diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index cae3bd4..fdac2c1 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -5,6 +5,8 @@ add_library(qtmaildir_lib STATIC requestinterceptor.cpp htmlbuilder.cpp cidschemehandler.cpp + cardlayout.cpp + carddelegate.cpp notmuchworker.cpp tagchip.cpp tagcolors.cpp diff --git a/src/carddelegate.cpp b/src/carddelegate.cpp new file mode 100644 index 0000000..f13205e --- /dev/null +++ b/src/carddelegate.cpp @@ -0,0 +1,261 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. <danix@danix.xyz> + * + * 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 "carddelegate.h" + +#include "cardlayout.h" +#include "threadlistmodel.h" + +#include <QApplication> +#include <QDateTime> +#include <QGuiApplication> +#include <QPainter> +#include <QRegularExpression> +#include <QStyle> + +namespace { + +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(); + return in; +} + +} // namespace + +QRect CardDelegate::expanderRectFor(const QStyleOptionViewItem &option, + const QModelIndex &index) +{ + return CardLayout::compute(inputFor(index), option.rect, option.font) + .expanderRect; +} + +QColor CardDelegate::accentLineColour(const QColor &accountColour) +{ + if (!accountColour.isValid()) + return ThreadListModel::threadLineColour(); + + // 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, + const QModelIndex &index) const +{ + Q_UNUSED(index); + // One height for every row, thread and reply alike. Asserted directly in + // test_cardlayout rather than left to two cards happening to agree. + return QSize(option.rect.width(), CardLayout::heightFor(option.font)); +} + +void CardDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, + const QModelIndex &index) const +{ + // Background, selection and any model fill first, through the style, so a + // selected or doomed card looks right before anything is drawn on top. + QStyleOptionViewItem chrome = option; + initStyleOption(&chrome, index); + chrome.text.clear(); + const QWidget *widget = option.widget; + QStyle *style = widget ? widget->style() : QApplication::style(); + style->drawControl(QStyle::CE_ItemViewItem, &chrome, painter, widget); + + const CardLayout card = + CardLayout::compute(inputFor(index), option.rect, option.font); + + painter->save(); + + // The account's colour, for both the accent bar and the spines. + // + // A reply must resolve its THREAD's colour, not its own: AccountColourRole + // is empty on a message row, and a spine that fell back to the neutral + // line under an accented root would break the one continuous edge this + // design is built on. index.parent() is the thread for a depth-1 reply and + // the containing subtree for a deeper one, so walk to the root. + QModelIndex root = index; + while (root.parent().isValid()) + root = root.parent(); + const QColor accountColour = + root.data(ThreadListModel::AccountColourRole).value<QColor>(); + const QColor lineColour = accentLineColour(accountColour); + + // The accent bar, thread cards only. Drawn after the chrome so the + // selection highlight cannot cover it: which account a card belongs to + // must stay readable on the row the user is looking at. + if (!card.accentRect.isEmpty()) + painter->fillRect(card.accentRect, 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, + // so over the highlight it lands grey-on-highlight and close to unreadable. + const QVariant foreground = index.data(Qt::ForegroundRole); + if (option.state & QStyle::State_Selected) + painter->setPen(option.palette.highlightedText().color()); + else if (foreground.isValid()) + painter->setPen(foreground.value<QBrush>().color()); + else + painter->setPen(option.palette.text().color()); + + // The model's font carries bold for unread and strike-out for deleted; + // initStyleOption resolved it into chrome.font. + painter->setFont(chrome.font); + const QFontMetrics metrics(chrome.font); + + // Line 1: sender, then the date flush right. + painter->drawText(card.senderRect, Qt::AlignVCenter | Qt::AlignLeft, + metrics.elidedText( + index.data(ThreadListModel::SendersRole).toString(), + Qt::ElideRight, card.senderRect.width())); + const QDateTime date = + index.data(ThreadListModel::DateRole).toDateTime(); + painter->drawText(card.dateRect, Qt::AlignVCenter | Qt::AlignRight, + CardLayout::formatDate(date)); + + // Line 2: the flag mark, the subject, the attachment mark. + QString subject = index.data(ThreadListModel::SubjectRole).toString(); + if (index.data(ThreadListModel::IsMessageRole).toBool()) { + // Every reply repeating "Re: <the thread's subject>" is the visual + // signature of a table of records, which is what item 53 is about. + static const QRegularExpression re( + QStringLiteral("^\\s*(?:[Rr][Ee]\\s*:\\s*)+")); + subject.remove(re); + } + QString line2; + if (index.data(ThreadListModel::IsFlaggedRole).toBool()) + line2 += ThreadListModel::flagGlyph() + QLatin1Char(' '); + line2 += subject; + if (index.data(ThreadListModel::HasAttachmentRole).toBool()) + line2 += QLatin1Char(' ') + ThreadListModel::attachmentGlyph(); + painter->drawText(card.subjectRect, Qt::AlignVCenter | Qt::AlignLeft, + metrics.elidedText(line2, Qt::ElideRight, + card.subjectRect.width())); + + // 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()) { + const int count = index.data(ThreadListModel::ReplyCountRole).toInt(); + 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(); + + // Line 3: the chips. A thread card draws its own tags; a reply draws only + // the tags its thread does not already carry, so the thread's chips are + // not repeated down the whole expansion. + const bool isMessage = + index.data(ThreadListModel::IsMessageRole).toBool(); + const QStringList tags = + index.data(isMessage ? ThreadListModel::MessageOwnTagsRole + : ThreadListModel::PillTagsRole) + .toStringList(); + const QVariantList colours = + index.data(isMessage ? ThreadListModel::MessageOwnColoursRole + : ThreadListModel::PillColoursRole) + .toList(); + + const QFont chipFont = CardLayout::smallFont(chrome.font); + const QFontMetrics chipMetrics(chipFont); + painter->save(); + painter->setFont(chipFont); + int x = card.tagRect.left(); + for (int i = 0; i < tags.size(); ++i) { + const QSize size = TagChip::sizeFor(chipMetrics, tags.at(i)); + if (x + size.width() > card.tagRect.right()) + break; // Out of room; a clipped chip reads as a rendering fault. + const QColor colour = i < colours.size() + ? colours.at(i).value<QColor>() + : QColor(0x55, 0x55, 0x5f); + TagChip::paint(painter, QRect(QPoint(x, card.tagRect.top()), size), + tags.at(i), colour); + x += size.width() + TagChip::kSpacing; + } + painter->restore(); +} diff --git a/src/carddelegate.h b/src/carddelegate.h new file mode 100644 index 0000000..7012eaa --- /dev/null +++ b/src/carddelegate.h @@ -0,0 +1,72 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. <danix@danix.xyz> + * + * 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. + */ + +#pragma once + +#include "tagchip.h" + +/// Paints a whole card: three lines, all of it, including the tag chips. +/// +/// It replaces both SubjectDelegate and ThreadListView::paintEvent. The view +/// used to paint the tag strip because a delegate cannot paint outside its +/// column and the strip spanned all five; with one column there is nothing to +/// span, so the strip comes home to the delegate and the view stops painting +/// entirely. That removes the two failure modes CLAUDE.md records for the +/// strip, a deleted row cut in half and every other row showing a bare stripe, +/// both of which existed because the view had to re-honour alternating +/// colours, the selection and BackgroundRole across cells it did not own. +/// +/// Inherits RowStyleDelegate for its one job, which still matters: Qt resolves +/// Qt::ForegroundRole into the palette's Text roles and then prefers those over +/// HighlightedText, so the read/unread dimming would otherwise win on a +/// selected row and land as grey on the highlight colour. +class CardDelegate : public RowStyleDelegate +{ + Q_OBJECT +public: + using RowStyleDelegate::RowStyleDelegate; + + void paint(QPainter *painter, const QStyleOptionViewItem &option, + const QModelIndex &index) const override; + QSize sizeHint(const QStyleOptionViewItem &option, + const QModelIndex &index) const override; + + /// The expander's rect for a row, so the VIEW can hit-test a click without + /// duplicating the layout. The delegate draws it and the view owns the + /// click, because a delegate gets no click of its own without an editor. + static QRect expanderRectFor(const QStyleOptionViewItem &option, + const QModelIndex &index); + + /// The colour the accent bar is painted in: the account's own, undiluted. + /// + /// 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 new file mode 100644 index 0000000..f542df0 --- /dev/null +++ b/src/cardlayout.cpp @@ -0,0 +1,186 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. <danix@danix.xyz> + * + * 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 <QFontMetrics> +#include <QLocale> + +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) +{ + 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. + // + // 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; + + // 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. + // + // 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(widestDateSample()); + 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 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 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()); + } + + 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; +} diff --git a/src/cardlayout.h b/src/cardlayout.h new file mode 100644 index 0000000..ef6a563 --- /dev/null +++ b/src/cardlayout.h @@ -0,0 +1,150 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. <danix@danix.xyz> + * + * 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. + */ + +#pragma once + +#include <QDateTime> +#include <QFont> +#include <QRect> +#include <QVector> + +/// Where everything on a card goes, with no painting and no widget. +/// +/// Split out from CardDelegate on purpose. A delegate needs a live QPainter and +/// an exposed view before it draws anything, which is what makes delegate tests +/// fragile: CLAUDE.md records that viewport()->render() returns a blank image in +/// several ordinary situations, and that a probe reporting "no ink anywhere" is +/// far more likely broken than the code it is testing. Every geometric claim +/// about a card is therefore made here, where a test is a function call. +/// +/// The card is three lines, always: +/// +/// sender ................................ date <- senderRect/dateRect +/// * subject @ v 3 replies <- subjectRect/expanderRect +/// [tag] [tag] <- tagRect +struct CardLayout +{ + /// What the model says about the row. Deliberately plain data: the layout + /// must be computable in a test without a model or a view. + struct Input + { + bool isMessage = false; + int depth = 0; ///< 0 for a thread root, 1 for a direct reply. + int replyCount = 0; ///< 0 means no expander. + }; + + /// Width of the account accent bar down a thread card's left edge. + /// + /// A starting value, not a settled one. Five accounts is enough that two + /// colours distinct as chips can read alike as thin stripes, and that can + /// only be judged against real cards on the user's own screen and theme + /// (Task 10). Widen it there if the accounts are not tellable apart. + static constexpr int kAccentWidth = 3; + + /// Horizontal breathing room at the card's edges, measured from the accent + /// bar rather than from the card, so text does not sit on the colour. + static constexpr int kPaddingX = 8; + + /// Vertical breathing room above the first line and below the last. + static constexpr int kPaddingY = 4; + + /// How far one level of reply nesting indents. + static constexpr int kIndentStep = 18; + + /// The depth past which nothing indents further. + /// + /// A mailing-list chain can nest a dozen deep, and without a cap the + /// sender is eventually pushed off the right edge. Item 20 accepted that + /// deep chains must be capped in the VIEW rather than flattened in the + /// model, and this is that cap. Rows past it draw at this depth's indent + /// with no marker saying so. + static constexpr int kMaxDepth = 4; + + QRect senderRect; + QRect dateRect; + QRect subjectRect; + QRect tagRect; + + /// The reply count's rect, and the click target that toggles the thread. + /// Empty when the row has no replies. + QRect expanderRect; + + /// The account accent bar down the card's left edge. + /// + /// Thread cards only. A reply's account is its thread's, stated once at the + /// head of the conversation, and a second vertical line in a reply's gutter + /// would sit a few pixels from the spine and compete with it. The spine + /// carries the accent instead, so an expansion is bounded by one colour + /// without ever drawing two lines. Empty on a reply. + QRect accentRect; + + /// One full-height vertical line per depth level, outermost first. + QVector<QRect> spines; + + /// Where the card's text starts, after any indent. + int contentLeft = 0; + + int totalHeight = 0; + + /// The height EVERY row gets, thread and reply alike. + /// + /// Uniform by design: it keeps setUniformRowHeights(true), which is the + /// single cheapest property of this layout, since no scrolling or + /// hit-testing arithmetic has to account for rows of differing size. The + /// cost is a blank third line on a card with no tags, which was accepted + /// explicitly. + static int heightFor(const QFont &font); + + /// The font the tag chips and the reply count are drawn in: a size down + /// from the card's own, so they read as annotation rather than as a third + /// column of content. + static QFont smallFont(const QFont &cardFont); + + 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/keymap.cpp b/src/keymap.cpp index 29a5f71..cdd26a1 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -18,6 +18,8 @@ #include "keymap.h" +#include <algorithm> + #include <QSettings> QStringList KeyMap::knownActions() @@ -64,6 +66,18 @@ QList<QPair<QString, QString>> KeyMap::defaultBindings() return { { QStringLiteral("Ctrl+J"), QStringLiteral("next_thread") }, { QStringLiteral("Ctrl+K"), QStringLiteral("prev_thread") }, + // Alt, because Shift+Up/Down is QTreeView's built-in extend-selection, + // which multi-row tagging depends on, and plain Up/Down is the view's + // own navigation, which already steps INTO an expanded thread's + // replies and is what gives message-to-message movement for free. + // + // These must stay chords. Every action is a QAction with + // WindowShortcut, dispatched before the focused widget sees the key, + // and Qt withholds only plain LETTERS from editable widgets: a bare + // Up bound here would break the arrow keys in the query bar, the tag + // dialog and the web view at once, exactly as Return did. + { QStringLiteral("Alt+Down"), QStringLiteral("next_thread") }, + { QStringLiteral("Alt+Up"), QStringLiteral("prev_thread") }, { QStringLiteral("Return"), QStringLiteral("open_thread") }, { QStringLiteral("Ctrl+E"), QStringLiteral("archive") }, { QStringLiteral("Ctrl+D"), QStringLiteral("delete") }, @@ -153,6 +167,28 @@ void KeyMap::loadDefaults() m_bindings.insert(normalizeSequence(binding.first), binding.second); } +QList<QKeySequence> KeyMap::sequencesFor(const QString &action) const +{ + const QKeySequence primary = sequenceFor(action); + if (primary.isEmpty()) + return {}; + + QList<QKeySequence> all{ primary }; + QList<QKeySequence> rest; + for (auto it = m_bindings.cbegin(); it != m_bindings.cend(); ++it) { + if (it.value() == action && it.key() != primary) + rest.append(it.key()); + } + // QHash iteration order is unspecified, so the tail is sorted rather than + // left to chance: an action's shortcut list must not reorder between runs. + std::sort(rest.begin(), rest.end(), + [](const QKeySequence &a, const QKeySequence &b) { + return a.toString() < b.toString(); + }); + all += rest; + return all; +} + QKeySequence KeyMap::sequenceFor(const QString &action) const { // Several sequences can reach one action: the built-in default, which diff --git a/src/keymap.h b/src/keymap.h index 1c7df5f..f0addf5 100644 --- a/src/keymap.h +++ b/src/keymap.h @@ -55,6 +55,15 @@ public: /// text, so the menu shows a stable choice rather than a hash-order one. QKeySequence sequenceFor(const QString &action) const; + /// EVERY sequence bound to an action, with sequenceFor()'s choice first. + /// + /// An action can have more than one binding, and setShortcut() keeps only + /// the last: next_thread ships with both Ctrl+J and Alt+Down, and with the + /// singular setter whichever arrived second was silently unreachable. + /// Ordered rather than hash-ordered, so the menu still advertises the same + /// binding sequenceFor() chose. + QList<QKeySequence> sequencesFor(const QString &action) const; + /// The built-in sequence for an action, ignoring any user override. static QKeySequence defaultSequenceFor(const QString &action); diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 11de1d4..b7efec5 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -42,7 +42,7 @@ #include <QSplitter> #include <QStandardPaths> #include <QStatusBar> -#include <QTableView> +#include <QScrollBar> #include <QTimer> #include <QToolBar> #include <QVBoxLayout> @@ -52,6 +52,8 @@ #include "mimeparser.h" #include "notmuchworker.h" #include "querycompleter.h" +#include "carddelegate.h" +#include "cardlayout.h" #include "tagchip.h" #include "tagdialog.h" #include "threadlistmodel.h" @@ -99,6 +101,40 @@ QString MainWindow::locksPath() return g_locksPath; } +/// The thread row containing an index: the index itself when it is already a +/// thread row, its parent when it is a message row. +/// +/// Replaces the arithmetic on row numbers that a table permitted. In a tree a +/// row number only identifies a row within one parent, so "current.row() + 1" +/// means the next SIBLING, which under an expanded thread is the next reply. +QModelIndex MainWindow::threadRowOf(const QModelIndex &index) const +{ + if (!index.isValid()) + return {}; + return index.parent().isValid() ? index.parent() : index; +} + +/// Selects a whole row, the way QTableView::selectRow did. +/// +/// QTreeView has no selectRow, and SelectRows on the selection model is not a +/// substitute: it governs what a click extends to, not what a programmatic +/// select() covers. +void MainWindow::selectRowAt(const QModelIndex &index) +{ + if (!index.isValid()) + return; + + m_threadView->selectionModel()->select( + index, QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + m_threadView->setCurrentIndex(index); +} + +/// Selects the top-level thread row at `row`. +void MainWindow::selectThreadRow(int row) +{ + selectRowAt(m_model->index(row, 0, QModelIndex())); +} + void MainWindow::restoreUiState() { QSettings state(uiStatePath(), QSettings::IniFormat); @@ -123,20 +159,15 @@ void MainWindow::restoreUiState() m_splitter->restoreState(splitter); } - // A header blob saved against a different set of columns must be - // discarded, not restored. QHeaderView::restoreState() returns TRUE for a - // blob with fewer sections than the model and applies the old widths to - // the wrong columns: adding the attachment column in front shifted every - // saved width one place right, silently mangling the layout with no error - // to detect it by (verified on Qt 6.11). The column count is stored - // alongside and the blob is only used when it still matches. - const QByteArray header = state.value(QStringLiteral("threadlist/header")) - .toByteArray(); - const int savedColumns = - state.value(QStringLiteral("threadlist/columns")).toInt(); - if (!header.isEmpty() && savedColumns == ThreadListModel::ColumnCount) { - m_threadView->horizontalHeader()->restoreState(header); - } + // No thread-list header state is read. The pane is one column drawn whole + // by CardDelegate, so there are no widths to restore; a blob saved by an + // older version is simply ignored (item 53's Upgrading note). + + // Range-guarded on read: a stale or hand-edited file can hold anything, + // and setCurrentIndex() on a value with no row silently selects nothing. + const int sort = + state.value(QStringLiteral("threadlist/sortOrder"), 0).toInt(); + m_sortOrder->setCurrentIndex(sort == 1 ? 1 : 0); // The config value is the starting point for a profile that has never // zoomed; once the user does, the state file is what they last had. @@ -153,11 +184,8 @@ void MainWindow::saveUiState() const state.setValue(QStringLiteral("window/geometry"), saveGeometry()); state.setValue(QStringLiteral("window/state"), saveState()); state.setValue(QStringLiteral("window/splitter"), m_splitter->saveState()); - state.setValue(QStringLiteral("threadlist/header"), - m_threadView->horizontalHeader()->saveState()); - // Guards the blob above: see restoreUiState(). - state.setValue(QStringLiteral("threadlist/columns"), - int(ThreadListModel::ColumnCount)); + state.setValue(QStringLiteral("threadlist/sortOrder"), + m_sortOrder->currentIndex()); state.setValue(QStringLiteral("message/zoom"), m_messageView->zoomFactor()); } @@ -289,8 +317,10 @@ MainWindow::MainWindow(const Config &config, QWidget *parent) qRegisterMetaType<MessageRef>(); qRegisterMetaType<TagChange>(); qRegisterMetaType<DatabaseStats>(); + qRegisterMetaType<MessageNode>(); qRegisterMetaType<QVector<ThreadSummary>>(); qRegisterMetaType<QVector<MessageRef>>(); + qRegisterMetaType<QVector<MessageNode>>(); m_keyMap.loadDefaults(); { @@ -397,9 +427,34 @@ void MainWindow::buildUi() // Query row. auto *queryRow = new QHBoxLayout; m_accountBox = new QComboBox(central); + m_accountBox->setObjectName(QStringLiteral("accountBox")); m_accountBox->addItem(tr("All accounts"), QString()); - for (const Account &account : m_config.accounts()) + for (const Account &account : m_config.accounts()) { m_accountBox->addItem(account.key, account.key); + // The RAW account colour here, not CardDelegate's blended line colour: + // a swatch is a filled patch like a chip, not a thin line, so it wants + // the colour the account was actually given. Qt renders a + // DecorationRole colour as a swatch itself, with no delegate. + // + // This is what makes the accent bar on a card mean anything: a colour + // down a card's edge says nothing until something maps it to a name. + m_accountBox->setItemData( + m_accountBox->count() - 1, + m_tagColors.colourFor(TagColors::tagForAccountKey(account.key)), + Qt::DecorationRole); + } + + // Sort order. Two entries, straight to notmuch: this ADDS a feature rather + // than replacing one, since the old column header was decorative and + // nothing implemented click-to-sort. + m_sortOrder = new QComboBox(central); + m_sortOrder->setObjectName(QStringLiteral("sortOrder")); + // Order matters: the index is what uistate.conf stores. + m_sortOrder->addItem(tr("Newest first")); + m_sortOrder->addItem(tr("Oldest first")); + m_sortOrder->setToolTip(tr("The order threads are listed in")); + connect(m_sortOrder, &QComboBox::currentIndexChanged, + this, &MainWindow::runCurrentQuery); m_queryEdit = new QLineEdit(central); m_queryEdit->setPlaceholderText(tr("notmuch query, e.g. tag:inbox")); @@ -488,6 +543,7 @@ void MainWindow::buildUi() // would squeeze the field, but three is the real-world case today. Item 23 // already specifies buttons-plus-menu and is where that belongs. queryRow->addWidget(m_accountBox); + queryRow->addWidget(m_sortOrder); queryRow->addWidget(m_queryEdit, 1); for (const SavedQuery &saved : m_config.savedQueries()) { auto *button = new QPushButton(saved.name, central); @@ -507,58 +563,48 @@ void MainWindow::buildUi() // delegate is confined to one column's rectangle. m_threadView = new ThreadListView(central); m_threadView->setModel(m_model); + m_threadView->setItemDelegate(new CardDelegate(this)); + m_threadView->setHeaderHidden(true); m_threadView->setSelectionBehavior(QAbstractItemView::SelectRows); m_threadView->setSelectionMode(QAbstractItemView::ExtendedSelection); - m_threadView->verticalHeader()->hide(); - m_threadView->horizontalHeader()->setStretchLastSection(false); - // Every column Interactive, Subject included: Stretch and ResizeToContents - // both compute a width and discard the user's drag. Nothing absorbs spare - // width as a result, so the columns end where they end. - for (int column = 0; column < ThreadListModel::ColumnCount; ++column) { - m_threadView->horizontalHeader()->setSectionResizeMode( - column, QHeaderView::Interactive); - } - // Two delegates, and the split is not cosmetic. RowStyleDelegate carries - // only the selection fix every column needs: the read/unread dimming - // arrives as a Qt::ForegroundRole, which Qt's painting prefers over the - // highlight, leaving a selected read row grey on the selection colour. - // - // SubjectDelegate adds the account chip and the tag pills, and must go on - // the subject column ALONE. It reads AccountLabelRole, a property of the - // row rather than of a cell, so installed view-wide it draws the chip into - // every column: tried once, and the list came out with a chip repeated - // four times per row. - m_threadView->setItemDelegate(new RowStyleDelegate(this)); - m_threadView->setItemDelegateForColumn(ThreadListModel::SubjectColumn, - new SubjectDelegate(this)); - - // One height for every row, set here rather than left to a column's - // sizeHint: a QTableView takes a single height per row, so a hint from the - // subject column alone would only apply if the view happened to ask it. - m_threadView->verticalHeader()->setDefaultSectionSize( - SubjectDelegate::rowHeightFor(m_threadView->font())); - // Widening a column past the viewport scrolls rather than squeezing the - // others. Per-pixel so the scroll does not jump a whole column at a time. - // Banding, so the eye can follow a row across four columns and a pill - // strip without losing it. The colour comes from the palette's - // AlternateBase, so it follows the desktop theme. + // No style-drawn branch decoration. CardDelegate draws the expander itself, + // because drawBranches runs BEFORE the row's cells and the delegate's own + // background paints straight over anything put there: a 60-pixel triangle + // once survived as 8. Leaving both enabled would draw the theme's dot + // underneath the delegate's glyph. + m_threadView->setRootIsDecorated(false); + + // Zero, because CardLayout draws the indent itself. Qt's own indentation + // would shift the card's rect, and every rect on the card is measured from + // that rect's left edge, so the two would compound. + m_threadView->setIndentation(0); + + // One height for every row. A QTreeView has no vertical header to carry a + // default section size, so the height comes from uniformRowHeights plus + // CardDelegate::sizeHint. + m_threadView->setUniformRowHeights(true); + + // Banding, so the eye can follow a card across the pane. The colour comes + // from the palette's AlternateBase, so it follows the desktop theme. m_threadView->setAlternatingRowColors(true); - m_threadView->setHorizontalScrollBarPolicy(Qt::ScrollBarAsNeeded); - m_threadView->setHorizontalScrollMode(QAbstractItemView::ScrollPerPixel); - - // Starting widths only; a drag overrides them, and they are what the - // saved-widths item will persist. - // Without this the attachment column cannot be narrow at all: the default - // minimum section size is 58px on this platform, and setColumnWidth() - // clamps to it silently rather than reporting the smaller value back. - m_threadView->horizontalHeader()->setMinimumSectionSize(24); - m_threadView->setColumnWidth(ThreadListModel::AttachmentColumn, 28); - m_threadView->setColumnWidth(ThreadListModel::FlagColumn, 28); - m_threadView->setColumnWidth(ThreadListModel::DateColumn, 130); - m_threadView->setColumnWidth(ThreadListModel::AuthorsColumn, 180); - m_threadView->setColumnWidth(ThreadListModel::SubjectColumn, 520); + // A card is exactly viewport width, so there is nothing to scroll to + // sideways. Turning the bar off is what closes item 51: a click used to + // scroll the list horizontally, because the subject column was wider than + // the viewport and auto-scroll brought the clicked index fully into view. + m_threadView->setHorizontalScrollBarPolicy(Qt::ScrollBarAlwaysOff); + + // Scrolling a whole card at a time rather than a fraction of one, so a + // card is never left half above the top edge. + m_threadView->verticalScrollBar()->setSingleStep( + CardLayout::heightFor(m_threadView->font())); + + // Replies are loaded when a thread is expanded, not with the query. + // Walking the reply tree of every thread in a 10k-thread result would cost + // far more than the query itself and almost none of it would be looked at. + connect(m_threadView, &QTreeView::expanded, + this, &MainWindow::onThreadExpanded); connect(m_threadView->selectionModel(), &QItemSelectionModel::currentRowChanged, @@ -617,9 +663,11 @@ QAction *MainWindow::addAction(const QString &name, const QString &text, // The binding comes from KeyMap, so a [keys] override reaches the menus // and the shortcut reference as well as the keyboard. - const QKeySequence sequence = m_keyMap.sequenceFor(name); - if (!sequence.isEmpty()) - action->setShortcut(sequence); + // Plural: an action can carry more than one binding, and setShortcut() + // keeps only the last one given. next_thread has both Ctrl+J and Alt+Down. + const QList<QKeySequence> sequences = m_keyMap.sequencesFor(name); + if (!sequences.isEmpty()) + action->setShortcuts(sequences); // Shortcuts must work while focus is in the thread list or the message // view, not only on the window itself. @@ -643,16 +691,32 @@ void MainWindow::registerActions() }); addAction(QStringLiteral("next_thread"), tr("&Next thread"), tr("Select the next thread"), [this]() { - const QModelIndex current = m_threadView->currentIndex(); - const int row = current.isValid() ? current.row() + 1 : 0; - if (row < m_model->rowCount()) - m_threadView->selectRow(row); + // Walked by INDEX, never by row number. A tree numbers rows per + // parent, so current.row() + 1 names a SIBLING: from the last reply of + // an expanded thread it asks for a row that does not exist, and from a + // thread row it counts top-level threads only by accident (item 60). + // + // The skip loop is what keeps this meaning thread-to-thread while the + // view's own Up/Down still steps message-to-message. + QModelIndex index = m_threadView->indexBelow( + m_threadView->currentIndex()); + while (index.isValid() + && index.data(ThreadListModel::IsMessageRole).toBool()) { + index = m_threadView->indexBelow(index); + } + if (index.isValid()) + selectRowAt(index); }); addAction(QStringLiteral("prev_thread"), tr("&Previous thread"), tr("Select the previous thread"), [this]() { - const QModelIndex current = m_threadView->currentIndex(); - if (current.isValid() && current.row() > 0) - m_threadView->selectRow(current.row() - 1); + QModelIndex index = m_threadView->indexAbove( + m_threadView->currentIndex()); + while (index.isValid() + && index.data(ThreadListModel::IsMessageRole).toBool()) { + index = m_threadView->indexAbove(index); + } + if (index.isValid()) + selectRowAt(index); }); addAction(QStringLiteral("open_thread"), tr("&Open thread"), tr("Focus the thread list"), [this]() { @@ -802,6 +866,7 @@ void MainWindow::registerActions() // thread straight back, which is the queued-reply race documented in // CLAUDE.md. m_currentThreadId.clear(); + m_currentMessageId.clear(); m_messageView->clear(); showPlaceholderPane(); m_markReadTimer->stop(); @@ -835,6 +900,7 @@ void MainWindow::registerActions() m_threadView->setCurrentIndex(QModelIndex()); m_currentThreadId.clear(); + m_currentMessageId.clear(); m_messageView->clear(); showPlaceholderPane(); m_markReadTimer->stop(); @@ -985,7 +1051,7 @@ void MainWindow::buildMenus() m_threadContextMenu->addAction(m_actions.value(QStringLiteral("select_all"))); m_threadView->setContextMenuPolicy(Qt::CustomContextMenu); - connect(m_threadView, &QTableView::customContextMenuRequested, + connect(m_threadView, &QWidget::customContextMenuRequested, this, &MainWindow::showThreadContextMenu); // The frequent subset only. A toolbar holding every action is as @@ -1245,6 +1311,10 @@ void MainWindow::wireWorker() this, &MainWindow::onThreadsReady); connect(m_worker, &NotmuchWorker::queryFinished, this, &MainWindow::onQueryFinished); + connect(m_worker, &NotmuchWorker::threadTreeLoaded, + this, &MainWindow::onThreadTreeLoaded); + connect(m_worker, &NotmuchWorker::messageLoaded, + this, &MainWindow::onMessageLoaded); connect(m_worker, &NotmuchWorker::threadLoaded, this, &MainWindow::onThreadLoaded); connect(m_worker, &NotmuchWorker::errorOccurred, @@ -1429,9 +1499,13 @@ void MainWindow::runCurrentQuery() m_queryComplete = false; updateViewWideActions(); + const auto sort = m_sortOrder->currentIndex() == 1 + ? NotmuchWorker::OldestFirst + : NotmuchWorker::NewestFirst; QMetaObject::invokeMethod(m_worker, "runQuery", Qt::QueuedConnection, Q_ARG(QString, query), - Q_ARG(quint64, m_generation)); + Q_ARG(quint64, m_generation), + Q_ARG(NotmuchWorker::SortOrder, sort)); } void MainWindow::onThreadsReady(const QVector<ThreadSummary> &threads, @@ -1519,41 +1593,90 @@ void MainWindow::showThreadContextMenu(const QPoint &pos) // collapsing to the clicked row here would silently narrow a deliberate // multi-row selection to one. Right-clicking outside it selects that row // instead, which is what every other list does. - if (!m_threadView->selectionModel()->isRowSelected(index.row())) - m_threadView->selectRow(index.row()); + if (!m_threadView->selectionModel()->isSelected(index)) + selectRowAt(index); m_threadContextMenu->popup(m_threadView->viewport()->mapToGlobal(pos)); } void MainWindow::onSelectionChanged() { - const int selected = m_threadView->selectionModel()->selectedRows().size(); - if (selected <= 1) { - // Clearing the count here would wipe whatever the last action reported - // ("Archive: 3 threads"), which is the more useful message once the - // selection is gone. Only a count this function wrote is taken back. - if (m_statusLabel->text() == m_selectionMessage) - m_statusLabel->clear(); - m_selectionMessage.clear(); + const QModelIndexList rows = m_threadView->selectionModel()->selectedRows(); + const int selected = rows.size(); + if (selected == 1) { + // One row selected. With two kinds of row this is exactly where the + // scope became ambiguous: a thread root stands for every message in it, + // a message row for one, and the keypress looks identical. Naming it + // here is what this project does instead of a confirmation dialog, + // which CLAUDE.md rules out for tag mutations. + const ActionScope scope = m_model->scopeFor(rows); + + if (scope.wholeThread) { + m_selectionMessage = + tr("1 thread selected (%n message(s))", "", scope.messageCount); + m_statusLabel->setText(m_selectionMessage); + m_statusTimer->stop(); + m_transientMessage.clear(); + } else { + // Reading one message is not a bulk action and gets no count. + if (m_statusLabel->text() == m_selectionMessage) + m_statusLabel->clear(); + m_selectionMessage.clear(); + } - // Collapsing a multi-row selection back to one row has to load that - // row here, and cannot be left to onThreadSelected. currentRowChanged - // is emitted BEFORE the selection model is updated (verified against - // Qt 6.11), so when a click collapses three rows to one, that handler - // still sees three selected, takes the multi-select branch and returns - // without loading anything. Only this signal sees the real count. + // Collapsing a multi-row selection back to one row has to load that row + // here, and cannot be left to onThreadSelected: currentRowChanged is + // emitted BEFORE the selection model is updated (verified against + // Qt 6.11), so that handler still sees the old count and returns + // without loading anything. + // + // Compared per row kind. A message row's row number indexes its + // siblings, so threadAt() on one answers about an unrelated thread and + // the comparison below would be against the wrong id. const QModelIndex current = m_threadView->currentIndex(); - if (current.isValid() - && m_model->threadAt(current.row()).threadId != m_currentThreadId) { - onThreadSelected(current, QModelIndex()); + if (current.isValid()) { + const bool changed = + m_model->isMessageRow(current) + ? m_model->messageAt(current).messageId != m_currentMessageId + : m_model->threadAt(current.row()).threadId + != m_currentThreadId; + if (changed) + onThreadSelected(current, QModelIndex()); } return; } + if (selected < 1) { + // Nothing selected. Clearing unconditionally would wipe whatever the + // last action reported ("Archive: 3 threads"), which is the more useful + // message once the selection is gone, so only a count this function + // wrote is taken back. + if (m_statusLabel->text() == m_selectionMessage) + m_statusLabel->clear(); + m_selectionMessage.clear(); + return; + } + // The count is the part that actually teaches multi-select: it acknowledges // the selection while it is being built, rather than only after an action // has already been applied to it. - m_selectionMessage = tr("%n thread(s) selected", "", selected); + // + // Reported per row kind rather than as a bare row count, so a mixed + // selection says what it will really touch instead of calling three replies + // "3 threads". + const ActionScope scope = m_model->scopeFor(rows); + if (!scope.threadIds.isEmpty() && scope.messageIds.isEmpty()) { + m_selectionMessage = + tr("%n thread(s) selected (%1 messages)", "", scope.threadIds.size()) + .arg(scope.messageCount); + } else if (scope.threadIds.isEmpty()) { + m_selectionMessage = + tr("%n message(s) selected", "", scope.messageIds.size()); + } else { + m_selectionMessage = + tr("%n thread(s) and %1 message(s) selected", "", + scope.threadIds.size()).arg(scope.messageIds.size()); + } m_statusLabel->setText(m_selectionMessage); // State, not an event: it must persist while the selection does. Cancel any @@ -1568,6 +1691,7 @@ void MainWindow::onSelectionChanged() m_markReadTimer->stop(); m_markReadThreadId.clear(); m_currentThreadId.clear(); + m_currentMessageId.clear(); m_messageView->clear(); showPlaceholderPane(); } @@ -1599,21 +1723,121 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, m_markReadTimer->stop(); m_markReadThreadId.clear(); m_currentThreadId.clear(); + m_currentMessageId.clear(); m_messageView->clear(); showPlaceholderPane(); return; } + // A message row renders that message ALONE. Checked before threadAt(), + // which takes a top-level row number: a child's row number indexes its + // siblings, so passing it here would silently load whichever thread happens + // to sit at that position in the list. + if (m_model->isMessageRow(current)) { + const MessageNode node = m_model->messageAt(current); + if (node.messageId.isEmpty()) + return; + + // No mark-read timer for a message row in this pass. Marking one + // message of a thread read is a per-message tag write, and the + // pending-edit map is keyed by thread; item 28 is the record of what + // happens when that count goes wrong. + m_markReadTimer->stop(); + m_markReadThreadId.clear(); + + m_currentThreadId.clear(); + m_currentMessageId = node.messageId; + m_messageView->setTags(node.tags); + QMetaObject::invokeMethod(m_worker, "loadMessage", Qt::QueuedConnection, + Q_ARG(QString, node.messageId), + Q_ARG(quint64, m_generation)); + return; + } + const ThreadSummary thread = m_model->threadAt(current.row()); 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), Q_ARG(quint64, m_generation)); } +void MainWindow::onMessageLoaded(const QVector<MessageRef> &messages, + quint64 generation) +{ + // The same two guards onThreadLoaded carries. A stale generation means the + // query moved on, and a reply landing after the selection grew past one row + // would paint a message back over a deliberately blanked pane. + if (generation != m_generation || messages.isEmpty()) + return; + if (m_threadView->selectionModel()->selectedRows().size() > 1) + return; + + // A third guard this one needs and onThreadLoaded does not: a reply that + // lands after the selection moved to a THREAD row would render one message + // where the whole conversation belongs. + if (m_currentMessageId.isEmpty()) + return; + + onThreadLoaded(messages, generation); +} + +void MainWindow::onThreadExpanded(const QModelIndex &index) +{ + if (!index.isValid() || m_model->isMessageRow(index)) + return; + + const QString threadId = + m_model->data(index, ThreadListModel::ThreadIdRole).toString(); + if (threadId.isEmpty()) + return; + + QMetaObject::invokeMethod(m_worker, "loadThreadTree", Qt::QueuedConnection, + Q_ARG(QString, threadId), + Q_ARG(QString, m_lastQuery), + Q_ARG(quint64, m_generation)); +} + +void MainWindow::onThreadTreeLoaded(const QVector<MessageNode> &nodes, + quint64 generation) +{ + // The same generation guard every other worker reply carries: an expansion + // whose query has since been replaced must not insert rows into the new + // result, where that thread may not even appear. + if (generation != m_generation || nodes.isEmpty()) + return; + + // Every node in one reply belongs to one thread, so the first one names it. + // Read from the node rather than remembered from the request: two + // expansions can be in flight at once, and pairing them by order would + // attach one thread's replies to the other. + m_model->setThreadMessages(nodes.first().threadId, nodes); +} + void MainWindow::onThreadLoaded(const QVector<MessageRef> &messages, quint64 generation) { @@ -2297,20 +2521,83 @@ void MainWindow::tagSelected(const QStringList &add, const QStringList &remove, if (rows.isEmpty()) return; - QStringList threadIds; - threadIds.reserve(rows.size()); - for (const QModelIndex &index : rows) - threadIds.append(m_model->threadAt(index.row()).threadId); + // Resolved through the model rather than by mapping rows to threads here. + // A message row's row number indexes its siblings, so the old + // threadAt(index.row()) mapping silently acted on whichever thread sat at + // that position in the list. + const ActionScope scope = m_model->scopeFor(rows); + if (scope.isEmpty()) + return; - sendThreadTagChange(threadIds, add, remove, description); + if (!scope.threadIds.isEmpty()) { + sendThreadTagChange(scope.threadIds, add, remove, description); - // Pushed for undo. The inverse re-resolves the same threads, so it works - // whether or not those rows are still selected. - m_undoStack.push(new ThreadTagCommand(this, threadIds, add, remove, - description)); + // Pushed for undo. The inverse re-resolves the same threads, so it + // works whether or not those rows are still selected. + m_undoStack.push(new ThreadTagCommand(this, scope.threadIds, add, + remove, description)); + } + + if (!scope.messageIds.isEmpty()) { + sendMessageTagChange(scope.messageIds, add, remove, description); + m_undoStack.push(new MessageTagCommand(this, scope.messageIds, add, + remove, description)); + } + // The scope named after the fact, since the selection may well be gone by + // the time the user reads it. This is what stands in for the confirmation + // dialog CLAUDE.md rules out: undo is the safety net, and undo is only + // usable if the user can tell that something larger than they meant has + // just happened. showTransientStatus( - tr("%1: %n thread(s)", "", threadIds.size()).arg(description)); + scope.wholeThread + ? tr("%1: %n message(s) (whole thread)", "", scope.messageCount) + .arg(description) + : tr("%1: %n message(s)", "", scope.messageCount).arg(description)); +} + +void MainWindow::sendMessageTagChange(const QStringList &messageIds, + const QStringList &add, + const QStringList &remove, + const QString &description) +{ + if (messageIds.isEmpty()) + return; + + // No optimistic model update. applyTagChange is keyed by THREAD and would + // repaint the whole row as though every message in it had changed, which + // for a one-message edit is a lie the user would see and then watch + // silently correct itself on the next query. + + // The accounts this touches, resolved through the containing threads: the + // account is a property of the thread, and the sync needs the channel + // whether one message moved or seven. + for (const QString &messageId : messageIds) { + const QString threadId = m_model->threadIdForMessage(messageId); + if (threadId.isEmpty()) + continue; + for (const QString &key : m_model->accountKeysForThread(threadId)) + m_editedAccounts.insert(key); + } + + // Held during a sync for exactly the reason the thread path is: the + // worker's read-write open BLOCKS on notmuch's exclusive lock rather than + // failing, so sending now would freeze the worker for the rest of the run. + if (aSyncHoldsTheWriteLock()) { + m_heldEdits.append(HeldEdit{ + {}, TagChange{ messageIds, add, remove, description } }); + m_statusLabel->setText( + tr("A sync is running; your change will be applied when it " + "finishes.")); + updatePendingIndicator(); + return; + } + + m_pendingThreadIds.clear(); + m_pendingChange = TagChange{ messageIds, add, remove, description }; + + QMetaObject::invokeMethod(m_worker, "applyTags", Qt::QueuedConnection, + Q_ARG(TagChange, m_pendingChange)); } void MainWindow::sendThreadTagChange(const QStringList &threadIds, diff --git a/src/mainwindow.h b/src/mainwindow.h index d8401a3..ccac5ad 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -41,7 +41,7 @@ class QAction; class QLineEdit; class QMenu; -class QTableView; +class ThreadListView; class QLabel; class QPushButton; class QComboBox; @@ -129,6 +129,19 @@ public: /// command was pushed, which is what "this did nothing" has to assert. int undoDepthForTesting() const { return m_undoStack.count(); } + /// The ids the last tag change was sent for, and whether they were thread + /// ids or message ids. + /// + /// Exposed because the difference is invisible from outside otherwise: a + /// message row routed down the thread path produces the same undo depth and + /// the same status text while tagging every sibling in the thread. A + /// mutation that made exactly that change passed the whole suite. + QStringList pendingThreadIdsForTesting() const { return m_pendingThreadIds; } + QStringList pendingMessageIdsForTesting() const + { + return m_pendingChange.messageIds; + } + /// The generation a worker reply must carry to be accepted. /// /// A test seam: onQueryFinished() discards a reply whose generation is @@ -164,6 +177,17 @@ private slots: /// the click lands inside. void showThreadContextMenu(const QPoint &pos); void onThreadLoaded(const QVector<MessageRef> &messages, quint64 generation); + + /// Asks the worker for a thread's reply tree when its row is expanded. + void onThreadExpanded(const QModelIndex &index); + + /// Fills in the expanded thread's message rows. + void onThreadTreeLoaded(const QVector<MessageNode> &nodes, + quint64 generation); + + /// Renders the single message a message row asked for. + void onMessageLoaded(const QVector<MessageRef> &messages, + quint64 generation); void onWorkerError(const QString &message); void onSyncFinished(bool success, int exitCode); @@ -212,6 +236,15 @@ private: /// A missing or rejected blob leaves the buildUi() defaults in place. void restoreUiState(); + /// The thread row containing an index: itself for a thread row, its parent + /// for a message row. + QModelIndex threadRowOf(const QModelIndex &index) const; + + /// Selects a whole row. QTreeView has no selectRow of its own. + void selectRowAt(const QModelIndex &index); + + /// Selects the top-level thread row at `row`. + void selectThreadRow(int row); void saveUiState() const; void registerActions(); @@ -335,6 +368,13 @@ private: const QStringList &remove, const QString &description); + /// The same for individual MESSAGES, without touching the undo stack. + /// Both tagSelected() and MessageTagCommand route through this. + void sendMessageTagChange(const QStringList &messageIds, + const QStringList &add, + const QStringList &remove, + const QString &description); + /// Undoes the optimistic model update for a write the worker rejected. void revertPendingTagChange(); @@ -368,6 +408,7 @@ private: QVector<HeldEdit> m_heldEdits; friend class ThreadTagCommand; + friend class MessageTagCommand; Config m_config; KeyMap m_keyMap; @@ -410,7 +451,10 @@ private: QLineEdit *m_queryEdit = nullptr; QueryCompleter *m_queryCompleter = nullptr; - QTableView *m_threadView = nullptr; + /// Its own type, not the QTreeView base. The strip painting and the + /// expander column are ThreadListView's, and holding the base here only + /// hid that from every reader. + ThreadListView *m_threadView = nullptr; /// Right-click menu for the thread list, holding the same QActions the /// menu bar does. @@ -424,6 +468,7 @@ private: /// placeholder's own text wraps every couple of words. static constexpr int kMinMessagePaneWidth = 300; QComboBox *m_accountBox = nullptr; + QComboBox *m_sortOrder = nullptr; QLabel *m_statusLabel = nullptr; /// Expires a transient status message. See showTransientStatus(). @@ -495,6 +540,11 @@ private: QString m_lastQuery; QString m_currentThreadId; + /// The message a MESSAGE row is showing, empty whenever the pane holds a + /// whole thread. The two are mutually exclusive and each clears the other, + /// so a late reply can tell which kind of selection it belongs to. + QString m_currentMessageId; + /// The selection count last written to the status bar, so it can be taken /// back without clobbering a message some other action put there. QString m_selectionMessage; @@ -601,3 +651,47 @@ private: QString m_description; bool m_firstRedo = true; }; + +/// Undo entry for a tag change over individual MESSAGES. +/// +/// Stores message ids, unlike ThreadTagCommand, and that difference is the +/// point rather than an inconsistency: a message row acts on one message, so +/// re-resolving its thread on undo would restore tags across every sibling the +/// action never touched. +class MessageTagCommand : public QUndoCommand +{ +public: + MessageTagCommand(MainWindow *window, const QStringList &messageIds, + const QStringList &add, const QStringList &remove, + const QString &description) + : QUndoCommand(description), m_window(window), + m_messageIds(messageIds), m_add(add), m_remove(remove), + m_description(description) {} + + /// The stack calls redo() when the command is pushed, by which point the + /// change has already been sent, so the first call is skipped. + void redo() override + { + if (m_firstRedo) { + m_firstRedo = false; + return; + } + m_window->sendMessageTagChange(m_messageIds, m_add, m_remove, + m_description); + } + + void undo() override + { + m_window->sendMessageTagChange( + m_messageIds, m_remove, m_add, + QStringLiteral("Undo %1").arg(m_description)); + } + +private: + MainWindow *m_window; + QStringList m_messageIds; + QStringList m_add; + QStringList m_remove; + QString m_description; + bool m_firstRedo = true; +}; diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 752a52c..d34c032 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -69,11 +69,75 @@ bool collectMessageIds(notmuch_database_t *db, const QString &query, return true; } +/// Walks a thread's reply structure depth-first, appending each message with +/// its depth. +/// +/// Takes RAW notmuch_message_t*, deliberately, against the rule that every +/// handle in this file is RAII-owned. Messages reached through a thread belong +/// to that thread and are freed with it (notmuch.h:1637), so wrapping one in +/// NmMessage would call notmuch_message_destroy on memory the thread frees +/// again. The NmThread in the caller is what keeps every pointer here alive, +/// and this must not outlive it. +/// +/// No match-set argument, unlike loadThread. A row is drawn for every message +/// in the thread regardless of the query: the list is where the user goes to +/// SEE the thread's shape, and hiding replies that did not match would make the +/// reply count disagree with the rows beneath it. +void walkReplies(notmuch_messages_t *messages, int depth, + QVector<MessageNode> *out) +{ + for (; notmuch_messages_valid(messages); + notmuch_messages_move_to_next(messages)) { + + notmuch_message_t *message = notmuch_messages_get(messages); + if (!message) + continue; + + MessageNode node; + node.messageId = + QString::fromUtf8(notmuch_message_get_message_id(message)); + node.threadId = + QString::fromUtf8(notmuch_message_get_thread_id(message)); + node.filePath = + QString::fromUtf8(notmuch_message_get_filename(message)); + node.from = + QString::fromUtf8(notmuch_message_get_header(message, "from")); + node.subject = + QString::fromUtf8(notmuch_message_get_header(message, "subject")); + node.date = + QDateTime::fromSecsSinceEpoch(notmuch_message_get_date(message)); + node.tags = tagsOf(message); + node.depth = depth; + out->append(node); + + // NULL is a legitimate "no replies" here: notmuch_messages_valid + // accepts it and returns FALSE (notmuch.h:1630), so a leaf needs no + // guard of its own. + walkReplies(notmuch_message_get_replies(message), depth + 1, out); + } +} + } // namespace +/// Registers SortOrder for queued calls, once, before main() runs. +/// +/// Q_ENUM alone is NOT enough for a queued Q_ARG: it gives the enum a +/// meta-object entry, not a metatype registered under the name invokeMethod +/// resolves, so MainWindow's queued runQuery would drop its sort argument at +/// runtime with a warning and every query would silently run newest-first. +/// +/// Here rather than in MainWindow's constructor, because the registration +/// belongs to the type rather than to one consumer: a caller that never +/// constructs a MainWindow (a test, or a future headless mode) needs it too, +/// and that is exactly how the first attempt at this passed by accident and +/// failed under test. +static const int kSortOrderMetaType = + qRegisterMetaType<NotmuchWorker::SortOrder>("NotmuchWorker::SortOrder"); + NotmuchWorker::NotmuchWorker(const QString ¬muchConfigPath, QObject *parent) : QObject(parent), m_configPath(notmuchConfigPath) { + Q_UNUSED(kSortOrderMetaType); } NotmuchWorker::~NotmuchWorker() @@ -122,7 +186,8 @@ void NotmuchWorker::close() } } -void NotmuchWorker::runQuery(const QString &query, quint64 generation) +void NotmuchWorker::runQuery(const QString &query, quint64 generation, + SortOrder sort) { if (!openReadOnly()) return; @@ -132,7 +197,9 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation) emit errorOccurred(QStringLiteral("Invalid query: %1").arg(query)); return; } - notmuch_query_set_sort(nmQuery.get(), NOTMUCH_SORT_NEWEST_FIRST); + notmuch_query_set_sort(nmQuery.get(), + sort == OldestFirst ? NOTMUCH_SORT_OLDEST_FIRST + : NOTMUCH_SORT_NEWEST_FIRST); notmuch_threads_t *rawThreads = nullptr; const notmuch_status_t status = @@ -242,6 +309,99 @@ void NotmuchWorker::loadThread(const QString &threadId, emit threadLoaded(result, generation); } +void NotmuchWorker::loadThreadTree(const QString &threadId, + const QString &matchQuery, + quint64 generation) +{ + // Accepted for signature symmetry with loadThread, and unused on purpose: + // see walkReplies on why every message in the thread gets a row. + Q_UNUSED(matchQuery); + + if (!openReadOnly()) + return; + + const QString query = QStringLiteral("thread:%1").arg(threadId); + NmQuery nmQuery(notmuch_query_create(m_db, query.toUtf8().constData())); + if (!nmQuery) { + emit errorOccurred( + QStringLiteral("Cannot load thread %1").arg(threadId)); + return; + } + + // search_threads, not search_messages. The messages have to come from a + // notmuch_thread_t or notmuch_message_get_replies returns NULL for every + // one of them and the walk below produces a flat list at depth 0. + notmuch_threads_t *rawThreads = nullptr; + if (notmuch_query_search_threads(nmQuery.get(), &rawThreads) + != NOTMUCH_STATUS_SUCCESS) { + emit errorOccurred( + QStringLiteral("Cannot search thread %1").arg(threadId)); + return; + } + NmThreads threads(rawThreads); + + QVector<MessageNode> nodes; + if (notmuch_threads_valid(threads.get())) { + // Held for the whole walk: every message pointer inside belongs to this + // thread and dies with it. + NmThread thread(notmuch_threads_get(threads.get())); + if (thread) { + walkReplies(notmuch_thread_get_toplevel_messages(thread.get()), 0, + &nodes); + } + } + + emit threadTreeLoaded(nodes, generation); +} + +void NotmuchWorker::loadMessage(const QString &messageId, quint64 generation) +{ + if (!openReadOnly()) + return; + + // id: is an exact-match prefix, and the id is quoted because a message id + // can legitimately contain characters notmuch's parser would otherwise read + // as query syntax. + const QString query = QStringLiteral("id:\"%1\"").arg(messageId); + NmQuery nmQuery(notmuch_query_create(m_db, query.toUtf8().constData())); + if (!nmQuery) { + emit errorOccurred( + QStringLiteral("Cannot load message %1").arg(messageId)); + return; + } + + notmuch_messages_t *rawMessages = nullptr; + if (notmuch_query_search_messages(nmQuery.get(), &rawMessages) + != NOTMUCH_STATUS_SUCCESS) { + emit errorOccurred( + QStringLiteral("Cannot search message %1").arg(messageId)); + return; + } + NmMessages messages(rawMessages); + + QVector<MessageRef> result; + if (notmuch_messages_valid(messages.get())) { + NmMessage message(notmuch_messages_get(messages.get())); + if (message) { + MessageRef ref; + ref.messageId = QString::fromUtf8( + notmuch_message_get_message_id(message.get())); + ref.filePath = QString::fromUtf8( + notmuch_message_get_filename(message.get())); + ref.tags = tagsOf(message.get()); + + // Always matched: the user asked for this message by clicking its + // row, so rendering it as a stub would answer the wrong question. + ref.matched = true; + result.append(ref); + } + } + + // Emitted even when empty, so the UI's handler runs and can decide what to + // do rather than waiting for a reply that never comes. + emit messageLoaded(result, generation); +} + void NotmuchWorker::applyTagsToThreads(const QStringList &threadIds, const QStringList &add, const QStringList &remove, diff --git a/src/notmuchworker.h b/src/notmuchworker.h index 7d6a587..1d8c8c0 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -44,10 +44,24 @@ public: /// Threads emitted per threadsReady() signal. static constexpr int kBatchSize = 200; + /// The sort orders offered to the user. + /// + /// Two, not four. notmuch also has NOTMUCH_SORT_MESSAGE_ID and + /// NOTMUCH_SORT_UNSORTED, and neither is an order a human wants. Sorting + /// by sender or subject is deliberately absent: notmuch cannot do it, so + /// the model would have to sort after results arrive, which fights the + /// batching that makes a 10k-thread query paint immediately. + enum SortOrder { + NewestFirst, + OldestFirst, + }; + Q_ENUM(SortOrder) + public slots: /// Runs a query. generation lets the UI discard results from a superseded /// query without the worker needing to know about cancellation. - void runQuery(const QString &query, quint64 generation); + void runQuery(const QString &query, quint64 generation, + SortOrder sort = NewestFirst); /// Loads the messages of one thread, oldest first. matchQuery is the /// user's current query; messages matching it render expanded, the rest @@ -55,6 +69,28 @@ public slots: void loadThread(const QString &threadId, const QString &matchQuery, quint64 generation); + /// Loads a thread as a reply TREE, for the message rows in the list. + /// + /// Separate from loadThread rather than replacing it, for a reason that is + /// not stylistic: loadThread walks notmuch_query_search_messages, and a + /// message obtained that way returns NULL from + /// notmuch_message_get_replies (notmuch.h:1617-1628), so that walk cannot + /// produce reply depth at all. The tree has to come from + /// notmuch_thread_get_toplevel_messages instead. The message pane still + /// wants the flat list; only the list wants the tree. + /// + /// matchQuery is accepted for signature symmetry with loadThread and is + /// deliberately unused: see the comment on the walk in the .cpp. + void loadThreadTree(const QString &threadId, const QString &matchQuery, + quint64 generation); + + /// Loads ONE message, for a message row selected in the list. + /// + /// Emits messageLoaded with an empty vector when the id is unknown, which + /// is an ordinary race after a reindex rather than an error worth + /// reporting. + void loadMessage(const QString &messageId, quint64 generation); + /// Applies tag changes. Opens the database read-write, applies, and closes /// immediately: notmuch's write lock is exclusive process-wide, so holding /// it would block the user's cron `notmuch new`. @@ -100,6 +136,9 @@ signals: void threadsReady(const QVector<ThreadSummary> &threads, quint64 generation); void queryFinished(int totalThreads, quint64 generation); void threadLoaded(const QVector<MessageRef> &messages, quint64 generation); + void threadTreeLoaded(const QVector<MessageNode> &nodes, + quint64 generation); + void messageLoaded(const QVector<MessageRef> &messages, quint64 generation); void tagsApplied(const TagChange &change); void allTagsReady(const QStringList &tags, quint64 generation); diff --git a/src/tagchip.cpp b/src/tagchip.cpp index 1f4ad79..d770a56 100644 --- a/src/tagchip.cpp +++ b/src/tagchip.cpp @@ -55,42 +55,6 @@ void paint(QPainter *painter, const QRect &rect, const QString &text, } // namespace TagChip -int SubjectDelegate::subjectBandHeight(const QStyleOptionViewItem &option) -{ - return QFontMetrics(option.font).height(); -} - -QFont SubjectDelegate::pillFont(const QFont &rowFont) -{ - QFont font = rowFont; - - // Two points down, floored. One point was measured to change nothing at a - // 12pt desktop font: 12 and 11 both render 17px tall, so the pills came - // out the same size as the subject and read as competing content rather - // than as annotation. - // - // pointSize() is -1 when the font was specified in pixels, which - // subtracting from would be nonsense, hence the two branches. - if (rowFont.pointSize() > 0) - font.setPointSize(qMax(6, rowFont.pointSize() - 2)); - else if (rowFont.pixelSize() > 0) - font.setPixelSize(qMax(8, rowFont.pixelSize() - 3)); - - return font; -} - -int SubjectDelegate::rowHeightFor(const QFont &rowFont) -{ - // The text band uses the ROW's font and the strip its own smaller one. - // Measuring both with one font is what put the pills over the date text. - const QFontMetrics rowMetrics(rowFont); - const QFontMetrics pillMetrics(pillFont(rowFont)); - - return rowMetrics.height() - + TagChip::sizeFor(pillMetrics, QStringLiteral("x")).height() - + kRowPadding * 2 + TagChip::kSpacing; -} - void RowStyleDelegate::initStyleOption(QStyleOptionViewItem *option, const QModelIndex &index) const { @@ -115,143 +79,9 @@ void RowStyleDelegate::initStyleOption(QStyleOptionViewItem *option, option->palette.setColor(QPalette::WindowText, highlighted); } - // Top-aligned and on one line, matching the subject beside them. - // - // The row is tall enough for a pill strip under the text, and Qt centres a - // cell's text in the whole rectangle by default: date and sender floated - // into the middle while the subject sat at the top, so the three did not - // share a baseline. Confining the rectangle to the text band puts them all - // on one. - // - // Wrapping matters more than it looks. A long sender ran to a second line, - // which reached down into the strip's band and collided with the pills; a - // cell cannot know they are there, since the view paints them afterwards. - // Eliding keeps every row's text inside its own band whatever it holds. - // Top of the row rather than centre of it, so the alignment is expressed - // without shrinking the rectangle: the rect is also what the background - // and selection fill are drawn into, and clipping it to the text band - // would leave the highlight covering only the upper part of the row. + // One line, elided. A card draws its own text through CardDelegate, but + // this still governs whatever Qt draws for the item itself, and a wrapped + // string would run past the card's own three lines. option->features &= ~QStyleOptionViewItem::WrapText; option->textElideMode = Qt::ElideRight; - - // The marker columns keep their centring. Their glyphs are the row's - // symbols rather than its text, so aligning them with the subject's - // baseline would strand them at the top of a tall row with the pill strip - // empty beneath; centred, they read as marking the whole row. - const bool marker = index.column() == ThreadListModel::AttachmentColumn - || index.column() == ThreadListModel::FlagColumn; - option->displayAlignment = marker - ? Qt::AlignCenter - : (Qt::AlignLeft | Qt::AlignTop); -} - -void SubjectDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, - const QModelIndex &index) const -{ - // AccountLabelRole is a property of the ROW, not of a cell, so this - // delegate must only ever be installed on the subject column. Installed - // view-wide it draws the account chip into every column, which is exactly - // what happened when that was tried. - Q_ASSERT(index.column() == ThreadListModel::SubjectColumn); - - const QString account = - index.data(ThreadListModel::AccountLabelRole).toString(); - if (account.isEmpty()) { - // No chip to draw, so the base class renders the text, confined to the - // upper band: the lower one belongs to the row-wide pill strip that - // ThreadListView paints after every cell. - QStyleOptionViewItem chrome = option; - initStyleOption(&chrome, index); - chrome.rect.setHeight(subjectBandHeight(option)); - QStyledItemDelegate::paint(painter, chrome, index); - - return; - } - - // Draw the row's own background and selection first, then the chip and the - // subject on top, so a selected or struck-through row still looks right. - QStyleOptionViewItem chrome = option; - initStyleOption(&chrome, index); - chrome.text.clear(); - const QWidget *widget = option.widget; - QStyle *style = widget ? widget->style() : QApplication::style(); - style->drawControl(QStyle::CE_ItemViewItem, &chrome, painter, widget); - - const QFontMetrics metrics(option.font); - const QSize chipSize = TagChip::sizeFor(metrics, account); - - // The subject and its chip occupy the upper band; ThreadListView paints - // the pill strip across the lower one. Centring the chip in the whole row - // would leave it floating beside that gap rather than beside its text. - const int textBandHeight = subjectBandHeight(option); - const int textTop = option.rect.top() + kRowPadding; - - const QRect chipRect(option.rect.left() + TagChip::kSpacing, - textTop + (textBandHeight - chipSize.height()) / 2, - chipSize.width(), chipSize.height()); - - const QColor colour = - index.data(ThreadListModel::AccountColourRole).value<QColor>(); - TagChip::paint(painter, chipRect, account, - colour.isValid() ? colour : QColor(0x55, 0x55, 0x5f)); - - // The subject follows the chip, elided so a long one cannot overflow. - QRect textRect = option.rect; - textRect.setLeft(chipRect.right() + TagChip::kSpacing * 2); - textRect.setTop(textTop); - textRect.setHeight(textBandHeight); - if (textRect.width() <= 0) - return; - - painter->save(); - // Selection outranks the model's colour, and that order matters. A read - // thread carries a dimmed foreground blended against the UNSELECTED - // background, so painting it over the highlight leaves grey-on-purple, - // which is close to unreadable. The highlight already carries the "this - // row" signal, so the read/unread distinction can yield to it for as long - // as the row is selected. - // - // A doomed thread is the exception that proves the rule: its white is not - // a dimming but a contrast requirement against its own fill, and the fill - // is drawn under the selection too. - const QVariant foreground = index.data(Qt::ForegroundRole); - if (option.state & QStyle::State_Selected) - painter->setPen(option.palette.highlightedText().color()); - else if (foreground.isValid()) - painter->setPen(foreground.value<QBrush>().color()); - else - painter->setPen(option.palette.text().color()); - - // The model's font carries bold for unread and strike-out for deleted. - // initStyleOption() already resolved it into chrome.font; using it rather - // than option.font is what keeps those cues on a delegate-drawn subject. - const QVariant fontData = index.data(Qt::FontRole); - const QFont rowFont = fontData.isValid() ? fontData.value<QFont>() - : chrome.font; - painter->setFont(rowFont); - const QFontMetrics rowMetrics(rowFont); - painter->drawText(textRect, Qt::AlignVCenter | Qt::AlignLeft, - rowMetrics.elidedText(index.data(Qt::DisplayRole).toString(), - Qt::ElideRight, textRect.width())); - painter->restore(); -} - -QSize SubjectDelegate::sizeHint(const QStyleOptionViewItem &option, - const QModelIndex &index) const -{ - QSize size = QStyledItemDelegate::sizeHint(option, index); - const QString account = - index.data(ThreadListModel::AccountLabelRole).toString(); - if (!account.isEmpty()) { - const QFontMetrics metrics(option.font); - size.setWidth(size.width() + TagChip::sizeFor(metrics, account).width() - + TagChip::kSpacing * 3); - } - - // Height comes from rowHeightFor(), applied by the view to every row at - // once. A QTableView takes ONE height per row, so a hint returned here - // would only win if the view happened to ask this column, and this - // delegate is on the subject column alone. - size.setHeight(rowHeightFor(option.font)); - return size; } diff --git a/src/tagchip.h b/src/tagchip.h index cc3b5de..5514cae 100644 --- a/src/tagchip.h +++ b/src/tagchip.h @@ -59,8 +59,8 @@ void paint(QPainter *painter, const QRect &rect, const QString &text, /// the selection highlight it lands as grey on the highlight colour, close to /// unreadable. /// -/// Applied to the columns that have no delegate of their own; SubjectDelegate -/// inherits it for the subject column. +/// Inherited by CardDelegate, which is the only delegate the thread list +/// installs. class RowStyleDelegate : public QStyledItemDelegate { Q_OBJECT @@ -72,41 +72,3 @@ protected: const QModelIndex &index) const override; }; -/// Item delegate for the subject column: draws the account chip in front of -/// the subject text, so which mailbox a thread came from reads at a glance -/// without a tags column spelling it out. -/// **Install on the subject column only.** It reads AccountLabelRole, which is -/// a property of the row rather than of a cell, so as a view-wide delegate it -/// draws the account chip into every column. -class SubjectDelegate : public RowStyleDelegate -{ - Q_OBJECT -public: - using RowStyleDelegate::RowStyleDelegate; - - void paint(QPainter *painter, const QStyleOptionViewItem &option, - const QModelIndex &index) const override; - QSize sizeHint(const QStyleOptionViewItem &option, - const QModelIndex &index) const override; - - /// Vertical breathing room above the subject and below the pill row. - static constexpr int kRowPadding = 4; - - /// The font the pill strip is drawn in: a size down from the row's own. - /// - /// At the same size the pills read as a second row of content competing - /// with the subject, rather than as annotation beneath it. Derived from - /// the row font rather than fixed, so it follows the desktop's font size. - static QFont pillFont(const QFont &rowFont); - - /// The height every row gets, tall enough for the subject and a pill strip - /// beneath it. The view applies this itself: a QTableView takes one height - /// for the whole row, so leaving it to a single column's sizeHint would - /// let whichever column the view happens to ask decide. - static int rowHeightFor(const QFont &rowFont); - -protected: - /// The height of the band the subject text occupies. Everything below it - /// belongs to ThreadListView's row-wide pill strip. - static int subjectBandHeight(const QStyleOptionViewItem &option); -}; diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 4bafca8..bdc7e96 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -64,7 +64,7 @@ QString ThreadListModel::flagGlyph() // U+2605 BLACK STAR, with the same fallback reasoning as the paperclip: an // unrenderable codepoint shows as tofu, which reads as breakage rather // than as "flagged". The solid star, not the outlined U+2606, since it has - // to register at column width beside a paperclip. + // to register at small size beside a paperclip. static const QString glyph = [] { const char32_t star = 0x2605; const QString preferred = QString::fromUcs4(&star, 1); @@ -75,6 +75,46 @@ QString ThreadListModel::flagGlyph() return glyph; } +QColor ThreadListModel::replyBackground() +{ + // Mixed from the palette rather than fixed, for the same reason as + // readColour: a tint that reads as "grouped" on a light theme is either + // invisible or muddy on a dark one. + // + // Toward Text rather than toward a hue, so it darkens on a light theme and + // lightens on a dark one without picking a colour that means something + // else. 0.07 is deliberately near the threshold of noticing: it is a + // grouping cue sitting beside the deleted and spam fills, which carry + // actual meaning and must stay the loudest thing in the list. + const QPalette palette = QGuiApplication::palette(); + const QColor base = palette.color(QPalette::Base); + const QColor text = palette.color(QPalette::Text); + + constexpr qreal kWeight = 0.07; + const qreal inverse = 1.0 - kWeight; + return QColor::fromRgbF( + text.redF() * kWeight + base.redF() * inverse, + text.greenF() * kWeight + base.greenF() * inverse, + text.blueF() * kWeight + base.blueF() * inverse); +} + +QColor ThreadListModel::threadLineColour() +{ + // Stronger than the tint, weaker than the text: the line is structure, so + // it has to be followable down a long expansion without competing with the + // senders beside it. + const QPalette palette = QGuiApplication::palette(); + const QColor base = palette.color(QPalette::Base); + const QColor text = palette.color(QPalette::Text); + + constexpr qreal kWeight = 0.35; + const qreal inverse = 1.0 - kWeight; + return QColor::fromRgbF( + text.redF() * kWeight + base.redF() * inverse, + text.greenF() * kWeight + base.greenF() * inverse, + text.blueF() * kWeight + base.blueF() * inverse); +} + QColor ThreadListModel::readColour() { // Derived from the palette, never hardcoded: a fixed grey that reads as @@ -99,37 +139,255 @@ QColor ThreadListModel::readColour() } ThreadListModel::ThreadListModel(QObject *parent) - : QAbstractTableModel(parent) + : QAbstractItemModel(parent) +{ +} + +QModelIndex ThreadListModel::index(int row, int column, + const QModelIndex &parent) const { + if (!hasIndex(row, column, parent)) + return {}; + + // A root row. -1 as the internal id marks it, so parent() can tell the two + // kinds apart without storing a node pointer per index. + if (!parent.isValid()) + return createIndex(row, column, static_cast<quintptr>(-1)); + + // A child row: the internal id is its parent's row, which is all parent() + // needs to rebuild the thread index. + return createIndex(row, column, static_cast<quintptr>(parent.row())); +} + +QModelIndex ThreadListModel::parent(const QModelIndex &child) const +{ + if (!child.isValid()) + return {}; + + const quintptr id = child.internalId(); + if (id == static_cast<quintptr>(-1)) + return {}; + + // Column 0, always. Qt requires a parent index in the first column, and + // returning the child's own column instead breaks selection and the + // expander, silently and only for the other columns. + return createIndex(static_cast<int>(id), 0, static_cast<quintptr>(-1)); } int ThreadListModel::rowCount(const QModelIndex &parent) const { - return parent.isValid() ? 0 : m_threads.size(); + if (!parent.isValid()) + return m_threads.size(); + + // Only a thread row has children, and only in its first column. A tree + // takes one set of children per row; offering them under every column makes + // the view draw an expander in each one. + if (parent.parent().isValid() || parent.column() != 0) + return 0; + + if (parent.row() < 0 || parent.row() >= m_threads.size()) + return 0; + + return m_threads.at(parent.row()).children.size(); +} + +bool ThreadListModel::hasChildren(const QModelIndex &parent) const +{ + if (!parent.isValid()) + return !m_threads.isEmpty(); + + // A message row is always a leaf. Reply depth is drawn from the node's own + // depth, not from further nesting, so nothing hangs under a reply. + if (parent.parent().isValid()) + return false; + + if (parent.column() != 0) + return false; + + if (parent.row() < 0 || parent.row() >= m_threads.size()) + return false; + + const ThreadNode &node = m_threads.at(parent.row()); + + // Once loaded the children are the truth, including "there are none", which + // is how a thread whose totalCount counted duplicates stops offering an + // expander that opens onto nothing. + if (node.loaded) + return !node.children.isEmpty(); + + // Before loading, the summary's count is all there is. A thread of one + // message has no replies and must not offer an expander. + return node.summary.totalCount > 1; } int ThreadListModel::columnCount(const QModelIndex &parent) const { - return parent.isValid() ? 0 : ColumnCount; + // One column: the card is drawn whole by CardDelegate. The five-column + // grid is what item 53 removed. + // + // Answered for a valid parent too. Returning 0 there, as the table version + // did, would give message rows no columns at all and render them blank. + Q_UNUSED(parent); + return 1; } QVariant ThreadListModel::data(const QModelIndex &index, int role) const { // A stale index from a view that has not caught up with a clear() can carry // any row or column, so both bounds are checked rather than trusted. - if (!index.isValid() || index.row() < 0 || index.row() >= m_threads.size() - || index.column() < 0 || index.column() >= ColumnCount) { + if (!index.isValid() || index.row() < 0 + || index.column() != 0) { return {}; } - const ThreadSummary &thread = m_threads.at(index.row()); + // A message row. Handled before the bounds check below, since a child row's + // number indexes its siblings, not m_threads. + if (isMessageRow(index)) { + const MessageNode node = messageAt(index); + if (node.messageId.isEmpty()) + return {}; + + switch (role) { + case IsMessageRole: + return true; + case MessageIdRole: + return node.messageId; + case MessageDepthRole: + return node.depth; + case HasRepliesRole: + // A reply never has its own expander: nesting past the first level + // is drawn from depth, not from further parent-child structure. + return false; + case ThreadIdRole: + // A message row still belongs to a thread, and a caller that only + // needs the containing thread must not have to walk up itself. + return node.threadId; + case TagsRole: + case PillTagsRole: + // No strip under a child row: the strip is a ROW-wide band carrying + // the thread's tags, and one under every reply would stripe the + // list and repeat the same tags down the whole expansion. + return QStringList(); + case PillColoursRole: + return QVariantList(); + case MessageOwnTagsRole: { + // Set difference against the parent THREAD, not against a global + // list: "own" means "not already said by the card above this one". + // The parent row indexes m_threads directly, which is the same + // mapping messageAt() uses to reach this node. + const int threadRow = index.parent().row(); + const QStringList threadTags = + (threadRow >= 0 && threadRow < m_threads.size()) + ? m_threads.at(threadRow).summary.tags + : QStringList(); + QStringList own; + for (const QString &tag : node.tags) { + if (!threadTags.contains(tag)) + own.append(tag); + } + // Sorted, so a reply does not reshuffle its own chips between + // repaints, matching what PillTagsRole already guarantees. + own.sort(); + return own; + } + case MessageOwnColoursRole: { + const QStringList own = + data(index, MessageOwnTagsRole).toStringList(); + QVariantList colours; + colours.reserve(own.size()); + for (const QString &tag : own) { + colours.append(m_tagColors ? m_tagColors->colourFor(tag) + : TagColors().colourFor(tag)); + } + return colours; + } + case AccountLabelRole: + return QString(); + case Qt::DisplayRole: + case SubjectRole: + return node.subject; + case SendersRole: + // The REPLY's sender, not the thread's author summary. Reading the + // thread's fields here would look almost right, since the first + // sender usually appears in both. + return node.from; + case DateRole: + return node.date; + case HasAttachmentRole: + return node.hasAttachment(); + case IsFlaggedRole: + return node.isFlagged(); + case ReplyCountRole: + // A reply never offers an expander: nesting past the first level is + // drawn from depth, not from further parent-child structure. + return 0; + case Qt::BackgroundRole: + // Tinted, so an expanded thread reads as one block rather than as + // more table rows. Applied per cell here; ThreadListView fills the + // same colour across the strip's band so the row does not end up + // half tinted. + return replyBackground(); + case Qt::FontRole: { + // A size down from the thread rows, so a thread reads as the + // heading and its replies as the contents. Never bold: an unread + // reply is still subordinate to the thread it belongs to, and the + // thread row above already carries the unread cue for the whole + // conversation. + QFont font = QGuiApplication::font(); + if (font.pointSize() > 0) + font.setPointSize(qMax(6, font.pointSize() - 1)); + else if (font.pixelSize() > 0) + font.setPixelSize(qMax(8, font.pixelSize() - 2)); + return font; + } + case Qt::ForegroundRole: + // Dimmed whether read or not, for the same reason as the font: a + // reply is subordinate content. An unread one is left undimmed so + // it can still be found. + return node.isUnread() ? QVariant() : QVariant(readColour()); + default: + return {}; + } + } + + if (index.row() >= m_threads.size()) + return {}; + + const ThreadSummary &thread = m_threads.at(index.row()).summary; if (role == ThreadIdRole) return thread.threadId; + // Answered rather than left to fall through as an invalid QVariant. An + // invalid one converts to false and an empty string anyway, so the + // behaviour is the same, but a role the model never mentions is a latent + // bug the next reader has to prove is harmless. + if (role == IsMessageRole) + return false; + + 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; + + if (role == HasRepliesRole) + return hasChildren(index.siblingAtColumn(0)); + if (role == TagsRole) return thread.tags; + if (role == MessageOwnTagsRole) + return QStringList(); + + if (role == MessageOwnColoursRole) + return QVariantList(); + if (role == PillTagsRole || role == PillColoursRole) { // Everything the row already says another way is dropped: the account // is the chip in the subject cell, flagged is the star column, @@ -186,46 +444,42 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const return {}; } - if (role == Qt::ToolTipRole && index.column() == AttachmentColumn) - return thread.hasAttachment() ? tr("Has an attachment") : QVariant(); - - // "Important", matching the action's own wording (item 57). The underlying - // tag is still `flagged` and isFlagged() still tests for it; only what the - // user reads changed. - if (role == Qt::ToolTipRole && index.column() == FlagColumn) - return thread.isFlagged() ? tr("Important") : QVariant(); - - // Both marker columns: a glyph reads as a marker only when it sits in the - // middle of its column rather than against the text beside it. - if (role == Qt::TextAlignmentRole - && (index.column() == AttachmentColumn || index.column() == FlagColumn)) { - return QVariant::fromValue(Qt::AlignCenter); + if (role == Qt::ToolTipRole) { + // One tooltip for the whole card, since the marks no longer have + // columns of their own to be hovered separately. "Important" matches + // the action's own wording (item 57); the underlying tag is still + // `flagged` and isFlagged() still tests for it. + QStringList marks; + if (thread.isFlagged()) + marks.append(tr("Important")); + if (thread.hasAttachment()) + marks.append(tr("Has an attachment")); + return marks.isEmpty() ? QVariant() : marks.join(QStringLiteral(", ")); } - if (role == Qt::DisplayRole) { - switch (index.column()) { - case AttachmentColumn: - // A glyph rather than an icon resource: no new asset to ship, and - // it inherits the row's font, so it strikes through with a doomed - // thread like every other cell. - return thread.hasAttachment() ? attachmentGlyph() : QString(); - case FlagColumn: - // A glyph rather than an icon, for the same reasons as the - // paperclip: no asset to ship, and it inherits the row's font so - // it strikes through with a doomed thread. - return thread.isFlagged() ? flagGlyph() : QString(); - case DateColumn: - return thread.date.toString(QStringLiteral("yyyy-MM-dd hh:mm")); - case AuthorsColumn: - return thread.authors; - case SubjectColumn: - return thread.totalCount > 1 - ? QStringLiteral("%1 (%2)").arg(thread.subject) - .arg(thread.totalCount) - : thread.subject; - default: - return {}; - } + switch (role) { + case Qt::DisplayRole: + case SubjectRole: + // Bare, with no "(3)" message-count suffix. The count is drawn on the + // card's second line as the expander, so a suffix here would state it + // twice on the same card. + return thread.subject; + case SendersRole: + return thread.authors; + case DateRole: + // The QDateTime itself. Formatting belongs to the delegate now: the + // card decides how much of a date it has room for, and a pre-formatted + // string takes that decision away from it. + return thread.date; + case HasAttachmentRole: + return thread.hasAttachment(); + case IsFlaggedRole: + return thread.isFlagged(); + case ReplyCountRole: + // totalCount includes the root message, which is the card itself. + return qMax(0, thread.totalCount - 1); + default: + break; } // A thread tagged deleted or spam is on its way out, and the user needs to @@ -283,24 +537,6 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const return {}; } -QVariant ThreadListModel::headerData(int section, Qt::Orientation orientation, - int role) const -{ - if (orientation != Qt::Horizontal || role != Qt::DisplayRole) - return {}; - - switch (section) { - // No label: any text would set a minimum width far wider than the icon, - // which defeats the point of a narrow column. - case AttachmentColumn: return QString(); - case FlagColumn: return QString(); - case DateColumn: return tr("Date"); - case AuthorsColumn: return tr("From"); - case SubjectColumn: return tr("Subject"); - default: return {}; - } -} - void ThreadListModel::appendBatch(const QVector<ThreadSummary> &batch) { // beginInsertRows with an empty range violates Qt's contract, so the guard @@ -310,7 +546,8 @@ void ThreadListModel::appendBatch(const QVector<ThreadSummary> &batch) const int first = m_threads.size(); beginInsertRows({}, first, first + batch.size() - 1); - m_threads.append(batch); + for (const ThreadSummary &summary : batch) + m_threads.append(ThreadNode{ summary, {}, {}, false }); endInsertRows(); } @@ -321,17 +558,137 @@ void ThreadListModel::clear() endResetModel(); } +void ThreadListModel::setThreadMessages(const QString &threadId, + const QVector<MessageNode> &nodes) +{ + for (int row = 0; row < m_threads.size(); ++row) { + if (m_threads.at(row).summary.threadId != threadId) + continue; + + const QModelIndex parent = index(row, 0, QModelIndex()); + + // Replace, not append. A thread reloaded after a sync would otherwise + // list every reply twice. + if (!m_threads.at(row).children.isEmpty()) { + beginRemoveRows(parent, 0, m_threads.at(row).children.size() - 1); + m_threads[row].children.clear(); + endRemoveRows(); + } + + // 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<MessageNode> 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); + m_threads[row].children = children; + endInsertRows(); + } + + // Set even when there are no replies: that is the difference between a + // single-message thread and one whose replies were never fetched. + m_threads[row].loaded = true; + return; + } +} + +bool ThreadListModel::isMessageRow(const QModelIndex &index) const +{ + return index.isValid() && index.parent().isValid(); +} + +MessageNode ThreadListModel::messageAt(const QModelIndex &index) const +{ + if (!isMessageRow(index)) + return {}; + + const int threadRow = index.parent().row(); + if (threadRow < 0 || threadRow >= m_threads.size()) + return {}; + + const QVector<MessageNode> &children = m_threads.at(threadRow).children; + if (index.row() < 0 || index.row() >= children.size()) + return {}; + + return children.at(index.row()); +} + +QString ThreadListModel::threadIdForMessage(const QString &messageId) const +{ + for (const ThreadNode &node : m_threads) { + for (const MessageNode &child : node.children) { + if (child.messageId == messageId) + return node.summary.threadId; + } + } + return {}; +} + +ActionScope ThreadListModel::scopeFor(const QModelIndexList &selection) const +{ + ActionScope scope; + + for (const QModelIndex &index : selection) { + if (isMessageRow(index)) { + const MessageNode node = messageAt(index); + if (node.messageId.isEmpty() + || scope.messageIds.contains(node.messageId)) + continue; + scope.messageIds.append(node.messageId); + scope.messageCount += 1; + continue; + } + + if (index.row() < 0 || index.row() >= m_threads.size()) + continue; + + const ThreadSummary &summary = m_threads.at(index.row()).summary; + if (scope.threadIds.contains(summary.threadId)) + continue; + + scope.threadIds.append(summary.threadId); + + // totalCount, not the loaded children: a thread that was never expanded + // still has all of its messages, and counting only what happens to be + // on screen would understate what the action does. Floored at 1, since + // a summary with no count still stands for at least the message that + // produced it. + scope.messageCount += qMax(1, summary.totalCount); + scope.wholeThread = true; + } + + return scope; +} + ThreadSummary ThreadListModel::threadAt(int row) const { if (row < 0 || row >= m_threads.size()) return {}; - return m_threads.at(row); + return m_threads.at(row).summary; } QStringList ThreadListModel::accountKeysForThread(const QString &threadId) const { QStringList keys; - for (const ThreadSummary &thread : m_threads) { + for (const ThreadNode &node : m_threads) { + const ThreadSummary &thread = node.summary; if (thread.threadId != threadId) continue; for (const QString &tag : thread.tags) { @@ -351,10 +708,10 @@ void ThreadListModel::applyTagChange(const QString &threadId, const QStringList &removed) { for (int row = 0; row < m_threads.size(); ++row) { - if (m_threads.at(row).threadId != threadId) + if (m_threads.at(row).summary.threadId != threadId) continue; - QStringList &tags = m_threads[row].tags; + QStringList &tags = m_threads[row].summary.tags; for (const QString &tag : removed) tags.removeAll(tag); for (const QString &tag : added) { @@ -362,9 +719,9 @@ void ThreadListModel::applyTagChange(const QString &threadId, tags.append(tag); } - // The whole row repaints: unread state drives the font of every column, - // not just the tags one. - emit dataChanged(index(row, 0), index(row, ColumnCount - 1)); + // The whole card repaints: unread state drives its font, and the tags + // it draws on line 3 have just changed. + emit dataChanged(index(row, 0), index(row, 0)); return; } } diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index 2eaa88e..f381ffa 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -18,39 +18,26 @@ #pragma once -#include <QAbstractTableModel> +#include <QAbstractItemModel> #include <QColor> #include <QVector> #include "tagcolors.h" #include "types.h" -/// Table model over query results, filled in batches so a large query paints +/// Tree model over query results, filled in batches so a large query paints /// its first screenful immediately. -class ThreadListModel : public QAbstractTableModel +/// +/// A tree rather than a table since item 20: a thread's replies are child rows +/// under it. The tree is at most two levels deep in the MODEL (a thread, then +/// its messages) even though the messages carry a reply depth of their own; the +/// visual nesting beyond the first level comes from that depth, not from +/// further parent-child structure. A deeper model would buy nothing and make +/// every index calculation recursive. +class ThreadListModel : public QAbstractItemModel { Q_OBJECT public: - /// No tags column: spelling out a dozen tags per row cost most of the - /// list's width and was unreadable. Functional tags moved to a chip strip - /// under the message pane, and the account tag renders as a chip in front - /// of the subject. - enum Column { - /// A paperclip when the thread has an attachment, so it is visible - /// without opening the thread. Icon only and deliberately narrow; - /// it carries no text. - AttachmentColumn = 0, - - /// A star when the thread carries the flagged tag. Beside the - /// paperclip and the same shape: icon only, narrow, no text. - FlagColumn, - - DateColumn, - AuthorsColumn, - SubjectColumn, - ColumnCount, - }; - enum Role { /// The thread id behind a row. Views hand out QModelIndexes, but the /// worker speaks thread ids, so the mapping belongs on the model @@ -77,23 +64,86 @@ public: /// model because it owns the TagColors instance; a delegate reading /// config itself would be a second source of truth. PillColoursRole, + + /// True when the row is a MESSAGE row rather than a thread root. + /// Drives both the action scope and whether the view paints a tag + /// strip under the row. + IsMessageRole, + + /// The message id behind a message row. Empty on a thread root. + MessageIdRole, + + /// The message's reply depth, for the view's indentation. 1 for a + /// direct reply, since depth 0 is the root row itself. + MessageDepthRole, + + /// True when the row is a thread that has replies to show. + /// + /// Read by SubjectDelegate, which draws the expander itself: the + /// delegate cannot call hasChildren without the model, and the same + /// answer has to reach the cell that reserves room for the glyph. + HasRepliesRole, + + /// The tags this MESSAGE carries that its thread does not. + /// + /// A reply card shows these and nothing else. Showing a reply's full + /// tag set instead was measured against the user's own database and + /// rejected: of 48691 messages, 7 carry `unread` and 75 carry + /// `flagged`, and both are already drawn another way (the sender's + /// weight, and the mark on line 2). Every other tag is applied to a + /// whole thread and is identical on all its messages, so full sets + /// would repeat the thread's own chips down the entire expansion, + /// which is the striping the old row-wide strip existed to avoid. + /// + /// Empty on a thread row, which has no thread to differ from. + MessageOwnTagsRole, + + /// The colours for MessageOwnTagsRole, in the same order. Supplied by + /// the model for the same reason as PillColoursRole: it owns the + /// TagColors instance, and a delegate reading config itself would be a + /// second source of truth. + MessageOwnColoursRole, + + /// The card's own fields, by role rather than by column. + /// + /// Five columns used to answer these through Qt::DisplayRole. One + /// column cannot, and a card needs all five values at once, so each + /// gets a role and Qt::DisplayRole answers the subject alone (which is + /// what keyboard search and accessibility read). + SubjectRole, + SendersRole, + DateRole, ///< A QDateTime. The delegate formats it. + HasAttachmentRole, ///< bool + IsFlaggedRole, ///< bool + ReplyCountRole, ///< int; 0 when a thread has no replies. }; - /// Row fill for a thread tagged `deleted`, and for one tagged `spam`. - /// Muted rather than saturated: a bulk delete paints every selected row, - /// and a wall of pure red is harder to read than the list it replaces. - /// Exposed so a test names the same colour the model uses. - /// The character shown in AttachmentColumn for a thread that has one. - /// A paperclip when the system font can draw it, "*" otherwise. + /// The mark drawn on a card's second line when the message has an + /// attachment. A paperclip when the system font can draw it, "*" otherwise. static QString attachmentGlyph(); - /// The character shown in FlagColumn for a flagged thread. + /// The mark drawn on a card's second line when the message is flagged. /// A star when the system font can draw it, "*" otherwise. static QString flagGlyph(); + /// Row fill for a thread tagged `deleted`, and for one tagged `spam`. + /// Muted rather than saturated: a bulk delete paints every selected row, + /// and a wall of pure red is harder to read than the list it replaces. + /// Exposed so a test names the same colour the model uses. static QColor deletedColour(); static QColor spamColour(); + /// Background for a reply row, so an expanded thread reads as one block + /// rather than as more table rows. + /// + /// Derived from the palette and deliberately subtle: it marks a grouping, + /// and a tint strong enough to notice on its own would compete with the + /// deleted and spam row colours, which carry real meaning. + static QColor replyBackground(); + + /// The line drawn down the left of an expanded thread's replies. + static QColor threadLineColour(); + /// The dimmed text colour a READ thread carries. /// /// Unread rows are left at the palette's own colour and read ones recede, @@ -109,17 +159,59 @@ public: /// Without one, chips fall back to a colour generated from the tag name. void setTagColors(const TagColors *colours) { m_tagColors = colours; } + QModelIndex index(int row, int column, + const QModelIndex &parent = {}) const override; + QModelIndex parent(const QModelIndex &child) const override; + int rowCount(const QModelIndex &parent = {}) const override; int columnCount(const QModelIndex &parent = {}) const override; + + /// Whether a thread row should offer an expander. + /// + /// Answered from totalCount rather than from the loaded children, and that + /// is what makes lazy loading possible at all: rowCount is 0 until the + /// worker has walked the thread, so a view left to infer this from rowCount + /// alone draws no expander, the user can never expand, and the replies are + /// never asked for. The count is already in the summary, so this costs + /// nothing. + bool hasChildren(const QModelIndex &parent = {}) const override; QVariant data(const QModelIndex &index, int role) const override; - QVariant headerData(int section, Qt::Orientation orientation, - int role) const override; void appendBatch(const QVector<ThreadSummary> &batch); void clear(); ThreadSummary threadAt(int row) const; + /// Fills in a thread's message rows once the worker has walked its tree. + /// + /// The depth-0 message is dropped: it is the thread's first message and the + /// ROOT row already stands for it. Keeping it would show a thread of seven + /// as one root and seven children, contradicting the reply count the row + /// advertises. Calling again replaces the rows rather than appending, so a + /// thread reloaded after a sync does not list its replies twice. + void setThreadMessages(const QString &threadId, + const QVector<MessageNode> &nodes); + + /// True when the index is a message row rather than a thread root. + bool isMessageRow(const QModelIndex &index) const; + + /// The message row's node, or a default-constructed one for any index that + /// is not a message row. + MessageNode messageAt(const QModelIndex &index) const; + + /// The thread a loaded message row belongs to, or empty when no expanded + /// thread holds it. Only expanded threads have message rows at all, so a + /// message the user could select is always findable here. + QString threadIdForMessage(const QString &messageId) const; + + /// Resolves a selection into what an action should touch. + /// + /// Mixed selections are honoured as given: a thread root and an unrelated + /// reply act on that whole thread and that one message. Nothing is + /// escalated or narrowed silently, which is the point of the scope being + /// visible in the first place. + ActionScope scopeFor(const QModelIndexList &selection) const; + /// The account keys behind a thread's account tags, for item 49's /// per-account sync. /// @@ -137,6 +229,33 @@ public: const QStringList &removed); private: - QVector<ThreadSummary> m_threads; + /// One thread root and the message rows expanded under it. + /// + /// Children live beside the summary rather than in a separate map keyed by + /// thread id, so a row and its expansion are appended, cleared and + /// destroyed together. The model is rebuilt wholesale on every query, so + /// nothing here has to survive a reset. + struct ThreadNode + { + ThreadSummary summary; + QVector<MessageNode> 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 + /// messages. + bool loaded = false; + }; + + QVector<ThreadNode> m_threads; const TagColors *m_tagColors = nullptr; }; diff --git a/src/threadlistview.cpp b/src/threadlistview.cpp index ef80e09..5a3ce77 100644 --- a/src/threadlistview.cpp +++ b/src/threadlistview.cpp @@ -18,123 +18,38 @@ #include "threadlistview.h" -#include "tagchip.h" +#include "carddelegate.h" #include "threadlistmodel.h" -#include <QPaintEvent> -#include <QPainter> -#include <QScrollBar> +#include <QMouseEvent> -void ThreadListView::paintEvent(QPaintEvent *event) +void ThreadListView::mousePressEvent(QMouseEvent *event) { - QTableView::paintEvent(event); - - if (!model()) - return; - - QPainter painter(viewport()); - - // Two fonts, deliberately. The row's own font fixes where the text band - // ends, and the pills are drawn a size smaller: at the same size they read - // as a second row of content competing with the subject, rather than as - // annotation beneath it. - const QFontMetrics rowMetrics(font()); - const QFont pillFont = SubjectDelegate::pillFont(font()); - const QFontMetrics metrics(pillFont); - painter.setFont(pillFont); - - // Only the rows actually on screen. Walking the whole model would paint - // thousands of strips outside the viewport on a large query. - const int first = rowAt(0); - const int last = rowAt(viewport()->height() - 1); - const int lastRow = last >= 0 ? last : model()->rowCount() - 1; - - for (int row = qMax(0, first); row <= lastRow; ++row) { - const QModelIndex index = - model()->index(row, ThreadListModel::SubjectColumn); - - const int rowTop = rowViewportPosition(row); - const int height = rowHeight(row); - if (height <= 0) - continue; - - // The strip's band, filled to match the row before anything is drawn - // on it. - // - // A QTableView paints alternating colours and the selection PER CELL, - // so nothing paints the width to the right of the last column, and - // nothing paints the band at all where a column does not reach. Left - // unfilled, an alternate-coloured or selected row shows the viewport - // background in a strip across its lower half. Filled for every - // visible row, not only tagged ones, since an untagged row has the - // same band to account for. - // Starting at the date column, NOT at the viewport edge. The two - // leading columns hold the attachment and flag glyphs, centred in the - // full row height, so a band drawn over them cuts those glyphs in half. - const int bandLeft = - columnViewportPosition(ThreadListModel::DateColumn); - const QRect band(bandLeft, rowTop + SubjectDelegate::kRowPadding - + rowMetrics.height(), - viewport()->width() - bandLeft, - height - SubjectDelegate::kRowPadding - - rowMetrics.height()); - - // The model's own row colour wins where it has one: a deleted or spam - // thread fills its cells with crimson or orange, and painting the base - // colour across the band beneath them would cut the row in half. - const QVariant background = index.data(Qt::BackgroundRole); - - if (background.isValid()) - painter.fillRect(band, background.value<QBrush>()); - else if (selectionModel() && selectionModel()->isRowSelected(row)) - painter.fillRect(band, palette().brush(QPalette::Highlight)); - else if (alternatingRowColors() && (row % 2)) - painter.fillRect(band, palette().brush(QPalette::AlternateBase)); - else - painter.fillRect(band, palette().brush(QPalette::Base)); - - const QStringList tags = - index.data(ThreadListModel::PillTagsRole).toStringList(); - if (tags.isEmpty()) - continue; - - const QVariantList colours = - index.data(ThreadListModel::PillColoursRole).toList(); - - // The band the cells leave free, below the text they draw in the - // upper one. Measured from SubjectDelegate by both sides, so neither - // can drift into the other's half. The row's own font metrics set the - // text band; the strip's smaller font must not be used for it, or the - // pills ride up over the date and sender. - const int top = rowTop + SubjectDelegate::kRowPadding - + rowMetrics.height() + TagChip::kSpacing; - - // Aligned with the first text column rather than the viewport edge: - // the two leading columns are narrow markers for the attachment and - // flag glyphs, and a strip starting at x=0 paints straight over them. - // Indented past the date column's own left edge rather than flush with - // it: a chip starting exactly where the column does reads as part of - // the column rather than as a strip laid under the row. - int x = columnViewportPosition(ThreadListModel::DateColumn) - + TagChip::kSpacing * 2; - const int available = viewport()->width() - TagChip::kSpacing; - - for (int i = 0; i < tags.size(); ++i) { - const QSize size = TagChip::sizeFor(metrics, tags.at(i)); - - // Stop rather than wrap or elide. A row that grew to fit its tags - // would break the uniform height the list depends on, and half a - // chip reads as a rendering fault. - if (x + size.width() > available) - break; - - const QColor colour = i < colours.size() - ? colours.at(i).value<QColor>() - : QColor(0x55, 0x55, 0x5f); - - TagChip::paint(&painter, QRect(x, top, size.width(), size.height()), - tags.at(i), colour); - x += size.width() + TagChip::kSpacing; + const QModelIndex index = indexAt(event->pos()); + + // The reply 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) { + + QStyleOptionViewItem option; + initViewItemOption(&option); + option.rect = visualRect(index); + // State_Open decides which way the glyph points, and the rect is the + // same either way, but pass it so the layout sees the true state. + if (isExpanded(index)) + option.state |= QStyle::State_Open; + + if (CardDelegate::expanderRectFor(option, index) + .contains(event->pos())) { + setExpanded(index, !isExpanded(index)); + // Swallowed, so expanding does not also load the thread into the + // message pane: it is a request to see the thread's shape, not to + // read it. + event->accept(); + return; } } + + QTreeView::mousePressEvent(event); } diff --git a/src/threadlistview.h b/src/threadlistview.h index 0b4eafc..3215153 100644 --- a/src/threadlistview.h +++ b/src/threadlistview.h @@ -18,30 +18,33 @@ #pragma once -#include <QTableView> +#include <QTreeView> -/// The thread list, with a row-wide strip of tag chips under each row's cells. +/// The thread list. /// -/// The strip is painted by the VIEW rather than by a delegate, and that is the -/// whole reason this class exists. A delegate is handed one cell's rectangle -/// and cannot paint outside its column, so pills drawn from the subject -/// column's delegate stop at that column's edge, losing the last tags of a -/// well-tagged thread, and start at that column's left edge, which puts them -/// under the subject instead of under the row. Painting after the cells lets -/// the strip run the full width, which is what the layout asks for: +/// It exists for ONE reason now: the expander is drawn by CardDelegate, and a +/// delegate gets no click of its own without an editor, so the view has to own +/// the hit-test. Everything else it used to do is gone. /// -/// [ date ][ from ][ subject ...................... ] -/// [ pill ][ pill ][ pill ] -/// -/// The cells confine themselves to the upper band so the lower one is free; -/// SubjectDelegate::kRowPadding and rowHeightFor() are the shared measurements -/// that keep the two halves agreeing. -class ThreadListView : public QTableView +/// Until item 53 this class also painted a row-wide strip of tag chips after +/// the cells, because a delegate cannot paint outside its column and the strip +/// spanned all five. With one column and one delegate painting the whole card, +/// that reason is gone and so is the paintEvent, along with the two faults it +/// kept producing: a deleted row cut in half, and every other row showing a +/// bare stripe, both from the view having to re-honour alternating colours, +/// the selection and BackgroundRole across cells it did not own. +class ThreadListView : public QTreeView { Q_OBJECT public: - using QTableView::QTableView; + using QTreeView::QTreeView; protected: - void paintEvent(QPaintEvent *event) override; + /// Toggles a thread when its reply count is clicked. + /// + /// Being VISIBLE and being CLICKABLE are separate properties: + /// setRootIsDecorated(false), needed to stop the style drawing its own + /// indicator underneath, also removed the style's hit area, so an expander + /// once painted correctly and did nothing at all. + void mousePressEvent(QMouseEvent *event) override; }; diff --git a/src/types.h b/src/types.h index 999d3f3..d04670e 100644 --- a/src/types.h +++ b/src/types.h @@ -63,6 +63,63 @@ struct MessageRef bool matched = true; }; +/// One message as a row in the thread list. +/// +/// Separate from MessageRef, which exists for RENDERING a thread and carries +/// only what the message pane needs. A row has to be drawn without opening the +/// message at all, so the display facts live here. +struct MessageNode +{ + QString messageId; + QString threadId; ///< The thread this message belongs to. + QString from; + QString subject; + QDateTime date; + 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. + int depth = 0; + + bool isUnread() const { return tags.contains(QStringLiteral("unread")); } + bool isFlagged() const { return tags.contains(QStringLiteral("flagged")); } + + /// notmuch applies "attachment" while indexing, so this needs no MIME + /// parsing, exactly as on ThreadSummary. + bool hasAttachment() const + { + return tags.contains(QStringLiteral("attachment")); + } +}; + +/// What an action is about to touch, resolved from the selection. +/// +/// Exists because the thread list holds two kinds of row since item 20, so a +/// keypress alone no longer says whether it hit one message or seven. Actions +/// take one of these rather than a bare list of thread ids, and the status bar +/// reports it: this project's answer to that ambiguity is to make the scope +/// visible, not to add a confirmation dialog. See CLAUDE.md on why. +struct ActionScope +{ + QStringList threadIds; ///< Whole threads to act on. + QStringList messageIds; ///< Individual messages to act on. + + /// Messages the action will touch in total, for the status bar. A whole + /// thread contributes all of its messages, a message row contributes one. + int messageCount = 0; + + /// True when any whole thread is in scope, which drives the + /// "(whole thread)" suffix in the status bar. + bool wholeThread = false; + + bool isEmpty() const + { + return threadIds.isEmpty() && messageIds.isEmpty(); + } +}; + /// One tag mutation, kept so it can be inverted for undo. struct TagChange { @@ -93,5 +150,6 @@ struct DatabaseStats Q_DECLARE_METATYPE(ThreadSummary) Q_DECLARE_METATYPE(MessageRef) +Q_DECLARE_METATYPE(MessageNode) Q_DECLARE_METATYPE(TagChange) Q_DECLARE_METATYPE(DatabaseStats) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a7bb670..4ecc04d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -41,6 +41,7 @@ add_qtmaildir_test(interceptor) add_qtmaildir_test(htmlbuilder) add_qtmaildir_test(notmuchworker) add_qtmaildir_test(tagcolors) +add_qtmaildir_test(cardlayout) add_qtmaildir_test(threadlistmodel) add_qtmaildir_test(mailsync) add_qtmaildir_test(syncmonitor) diff --git a/tests/test_cardlayout.cpp b/tests/test_cardlayout.cpp new file mode 100644 index 0000000..fdc18bf --- /dev/null +++ b/tests/test_cardlayout.cpp @@ -0,0 +1,383 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. <danix@danix.xyz> + * + * 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> +#include <QLocale> +#include <QTest> + +class TestCardLayout : public QObject +{ + Q_OBJECT + +private slots: + void everyCardIsTheSameHeight(); + void threeLinesStackWithoutOverlapping(); + void replyIndentsByDepth(); + void aDepthZeroReplyStillIndents(); + void indentStopsAtTheCap(); + void expanderSitsOnTheSecondLine(); + void expanderIsEmptyWithoutReplies(); + void theExpanderReadsAsAPillWithAWord(); + void dateIsFlushRight(); + void threadCardCarriesAnAccentBar(); + void replyCardCarriesNoAccentBar(); + void theDateFitsWhenTheCardIsBold(); + void theDateFollowsTheSystemLocale(); +}; + +namespace { + +CardLayout::Input threadInput() +{ + CardLayout::Input in; + in.isMessage = false; + in.depth = 0; + in.replyCount = 3; + return in; +} + +CardLayout::Input replyInput(int depth) +{ + CardLayout::Input in; + in.isMessage = true; + in.depth = depth; + in.replyCount = 0; + return in; +} + +} // namespace + +void TestCardLayout::everyCardIsTheSameHeight() +{ + const QFont font; + const int thread = CardLayout::heightFor(font); + + // The uniform height is the whole reason setUniformRowHeights(true) + // survives this design, so it is asserted directly rather than inferred + // from two cards happening to look alike. + // + // Note what is NOT varied here: the tag list. CardLayout reserves line 3 + // unconditionally and never sees the tags, which is exactly the property + // being asserted. A version of this test that passed a tag list in would + // be testing a parameter that does not exist. + const CardLayout threadCard = + CardLayout::compute(threadInput(), QRect(0, 0, 400, thread), font); + const CardLayout deepReply = + CardLayout::compute(replyInput(3), QRect(0, 0, 400, thread), font); + CardLayout::Input noRepliesIn = threadInput(); + noRepliesIn.replyCount = 0; + const CardLayout noReplies = + CardLayout::compute(noRepliesIn, QRect(0, 0, 400, thread), font); + + QCOMPARE(threadCard.totalHeight, thread); + QCOMPARE(deepReply.totalHeight, thread); + QCOMPARE(noReplies.totalHeight, thread); + + // The third line exists on every card, including one with nothing to put + // there. That blank band is the cost the uniform height was bought with. + QCOMPARE(noReplies.tagRect.height(), threadCard.tagRect.height()); +} + +void TestCardLayout::threeLinesStackWithoutOverlapping() +{ + const QFont font; + const int h = CardLayout::heightFor(font); + const CardLayout card = + CardLayout::compute(threadInput(), QRect(0, 0, 400, h), font); + + QVERIFY(card.senderRect.height() > 0); + QVERIFY(card.subjectRect.height() > 0); + QVERIFY(card.tagRect.height() > 0); + + // Guard: these must actually be three stacked bands. A layout that + // collapsed them all to the same rect would satisfy any assertion that + // only checked they exist. + QVERIFY(card.senderRect.bottom() <= card.subjectRect.top()); + QVERIFY(card.subjectRect.bottom() <= card.tagRect.top()); + QVERIFY(card.tagRect.bottom() <= h); +} + +void TestCardLayout::replyIndentsByDepth() +{ + const QFont font; + const int h = CardLayout::heightFor(font); + const QRect rect(0, 0, 400, h); + + const CardLayout root = CardLayout::compute(threadInput(), rect, font); + const CardLayout d1 = CardLayout::compute(replyInput(1), rect, font); + const CardLayout d2 = CardLayout::compute(replyInput(2), rect, font); + + QVERIFY(d1.contentLeft > root.contentLeft); + QVERIFY(d2.contentLeft > d1.contentLeft); + + // One spine per depth level, so the count is the depth itself. + QCOMPARE(root.spines.size(), 0); + QCOMPARE(d1.spines.size(), 1); + QCOMPARE(d2.spines.size(), 2); + + // Each spine runs the full height of the card, which is what makes an + // expansion read as one continuous block rather than as dashes. + for (const QRect &spine : d2.spines) { + QCOMPARE(spine.top(), rect.top()); + QCOMPARE(spine.bottom(), rect.bottom()); + } +} + +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; + const int h = CardLayout::heightFor(font); + const QRect rect(0, 0, 400, h); + + const CardLayout d4 = CardLayout::compute(replyInput(4), rect, font); + const CardLayout d5 = CardLayout::compute(replyInput(5), rect, font); + const CardLayout d9 = CardLayout::compute(replyInput(9), rect, font); + + QCOMPARE(d5.contentLeft, d4.contentLeft); + QCOMPARE(d9.contentLeft, d4.contentLeft); + QCOMPARE(d5.spines.size(), d4.spines.size()); + QCOMPARE(d9.spines.size(), d4.spines.size()); + + // Guard: the cap must not be so low that it has already bitten at depth 3, + // which would make the three assertions above true for the wrong reason. + const CardLayout d3 = CardLayout::compute(replyInput(3), rect, font); + QVERIFY(d3.contentLeft < d4.contentLeft); +} + +void TestCardLayout::expanderSitsOnTheSecondLine() +{ + const QFont font; + const int h = CardLayout::heightFor(font); + const CardLayout card = + CardLayout::compute(threadInput(), QRect(0, 0, 400, h), font); + + QVERIFY(!card.expanderRect.isEmpty()); + // It is the reply count, so it belongs on the line the reply count is on. + QVERIFY(card.expanderRect.top() >= card.subjectRect.top()); + QVERIFY(card.expanderRect.bottom() <= card.subjectRect.bottom()); + // And it is on the right, where the count is drawn, not in a left gutter. + QVERIFY(card.expanderRect.left() > 400 / 2); +} + +void TestCardLayout::expanderIsEmptyWithoutReplies() +{ + const QFont font; + const int h = CardLayout::heightFor(font); + CardLayout::Input in = threadInput(); + in.replyCount = 0; + + const CardLayout card = CardLayout::compute(in, QRect(0, 0, 400, h), font); + 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; + const int h = CardLayout::heightFor(font); + const QRect rect(0, 0, 400, h); + const CardLayout card = CardLayout::compute(threadInput(), rect, font); + + // Compared as exclusive edges. QRect::right() is inclusive (left + width - + // 1), so asserting card.dateRect.right() == rect.right() - kPaddingX + // demands a gap of kPaddingX - 1 pixels and is off by one against the + // padding the constant names. + QCOMPARE(card.dateRect.right() + 1, rect.right() + 1 - CardLayout::kPaddingX); + // The sender must stop before the date starts, or a long sender overwrites + // it. This is the assertion that fails if the two are laid out + // independently. + QVERIFY(card.senderRect.right() <= card.dateRect.left()); +} + +void TestCardLayout::threadCardCarriesAnAccentBar() +{ + const QFont font; + const int h = CardLayout::heightFor(font); + const QRect rect(0, 0, 400, h); + const CardLayout card = CardLayout::compute(threadInput(), rect, font); + + QCOMPARE(card.accentRect.left(), rect.left()); + QCOMPARE(card.accentRect.width(), CardLayout::kAccentWidth); + // Full height, so a run of cards from one account reads as a continuous + // edge rather than as dashes. + QCOMPARE(card.accentRect.top(), rect.top()); + QCOMPARE(card.accentRect.bottom(), rect.bottom()); + + // Nothing may be drawn on top of the colour. + QVERIFY(card.contentLeft >= card.accentRect.right()); +} + +void TestCardLayout::replyCardCarriesNoAccentBar() +{ + const QFont font; + const int h = CardLayout::heightFor(font); + const CardLayout reply = + CardLayout::compute(replyInput(1), QRect(0, 0, 400, h), font); + + // A reply's account is its thread's, stated once at the head. The spine + // carries the accent instead, so the gutter never holds two lines. + QVERIFY(reply.accentRect.isEmpty()); + 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 + // 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" diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 5031ead..922705c 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -38,6 +38,7 @@ #include <QSplitter> #include <QTableView> #include <QToolBar> +#include <QTreeView> #include <QTimer> #include "config.h" @@ -45,8 +46,16 @@ #include "mainwindow.h" #include "messageview.h" #include "notmuchworker.h" +#include "carddelegate.h" +#include "cardlayout.h" + +#include <QImage> +#include <QPainter> +#include <QComboBox> +#include <QScrollBar> #include "tagchip.h" #include "threadlistmodel.h" +#include "threadlistview.h" /// MainWindow is mostly wiring, and the parts that need a real database are /// still verified manually. What is checked here is the action registry: the @@ -93,7 +102,22 @@ private slots: void anUnobservableLockTableLeavesTheSyncButtonUsable(); void theStatusBarFollowsTheSyncPhase(); void aSelectedReadThreadIsNotDimmedIntoTheHighlight(); - void thePillRowSpansTheWholeWidthNotOneColumn(); + void childRowsAreIndentedUnderTheirThread(); + void aThreadWithRepliesDrawsAVisibleExpander(); + void cardsNeverScrollSideways(); + void selectingARootCardKeepsItsThreadForMarkRead(); + void nextThreadLeavesTheLastReply(); + void altDownSkipsReplies(); + void bothThreadStepBindingsReachTheAction(); + void sortChoiceSurvivesRestart(); + void accountEntriesCarryTheirColour(); + void replyRowsKeepTheirTextUnderTheThreadLine(); + void clickingTheExpanderTogglesTheThread(); + void selectingAMessageRowTargetsThatMessageNotItsThread(); + void selectingAThreadRowNamesHowManyMessagesItStandsFor(); + void selectingAMessageRowReportsNoBulkCount(); + void anActionOnAThreadRowSaysItHitTheWholeThread(); + void anActionOnAMessageRowTagsThatMessageNotTheThread(); void markAllReadIsDisabledUntilTheQueryFinishes(); void markAllReadActsOnEveryRowAndUndoesInOneStep(); void markAllReadDoesNothingWhenNothingIsUnread(); @@ -439,11 +463,12 @@ void TestMainWindow::headerStateFromADifferentColumnLayoutIsDiscarded() window.close(); } - // Forge a state file from an older layout: same blob, wrong column count. + // Forge a state file from the five-column layout. Nothing reads these keys + // any more, and that is exactly what must be verified: a blob saved by an + // older version has to be ignored rather than applied to a one-column view. { QSettings state(MainWindow::uiStatePath(), QSettings::IniFormat); - state.setValue(QStringLiteral("threadlist/columns"), - int(ThreadListModel::ColumnCount) - 1); + state.setValue(QStringLiteral("threadlist/columns"), 5); state.setValue(QStringLiteral("threadlist/header"), QByteArray("not a header this model could have saved")); } @@ -452,11 +477,9 @@ void TestMainWindow::headerStateFromADifferentColumnLayoutIsDiscarded() const Config config; MainWindow reopened(config); - auto *view = reopened.findChild<QTableView *>(); + auto *view = reopened.findChild<QTreeView *>(); QVERIFY(view); - QCOMPARE(view->columnWidth(ThreadListModel::AttachmentColumn), 28); - QCOMPARE(view->columnWidth(ThreadListModel::DateColumn), 130); - QCOMPARE(view->columnWidth(ThreadListModel::SubjectColumn), 520); + QCOMPARE(view->model()->columnCount(), 1); QFile::remove(MainWindow::uiStatePath()); QStandardPaths::setTestModeEnabled(false); @@ -503,6 +526,27 @@ void TestMainWindow::returnInTheQueryBarRunsTheQueryNotOpenThread() QVERIFY(!actionFired); } +/// Selects a top-level THREAD row, replacing QTableView::selectRow which a +/// QTreeView does not have. +/// +/// Not merely a rename: setCurrentIndex alone leaves the selection model empty, +/// and select() alone leaves current invalid, so every test asserting on either +/// would break in a different way. Both are set here, exactly as +/// QTableView::selectRow did. +static void selectThreadRow(QTreeView *view, int row) +{ + const QModelIndex index = view->model()->index(row, 0, QModelIndex()); + view->selectionModel()->select( + index, QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(index); +} + +/// The height of a top-level row, replacing QTableView::rowHeight(int). +static int threadRowHeight(QTreeView *view, int row) +{ + return view->visualRect(view->model()->index(row, 0, QModelIndex())).height(); +} + /// A thread summary carrying the tags a test needs. Enough to drive selection; /// nothing here touches a database. static ThreadSummary makeThread(const QString &id, const QStringList &tags) @@ -515,81 +559,870 @@ static ThreadSummary makeThread(const QString &id, const QStringList &tags) return thread; } -void TestMainWindow::thePillRowSpansTheWholeWidthNotOneColumn() +void TestMainWindow::childRowsAreIndentedUnderTheirThread() +{ + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY2(view, "the thread list is not a QTreeView, so it cannot indent"); + + // With an account tag, so the thread row draws the chip that a reply row + // does not. That asymmetry is the whole reason the indent has to be wide, + // and a test against an untagged thread never sees it. + model->appendBatch({ makeThread( + QStringLiteral("t1"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }) }); + + MessageNode first; + first.messageId = QStringLiteral("m0@example.org"); + first.threadId = QStringLiteral("t1"); + first.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t1"); + reply.from = QStringLiteral("A Replier <replier@example.org>"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t1"), { first, reply }); + + window.resize(1400, 300); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const QModelIndex root = model->index(0, 0, QModelIndex()); + view->expand(root); + QApplication::processEvents(); + + // One column, and setIndentation(0): Qt indents nothing, CardLayout draws + // the indent itself. So visualRect reports the SAME rect for a thread and + // its reply, and the indent has to be read off the layout rather than off + // the geometry. That is the trap CLAUDE.md records in reverse: there, + // visualRect reported an indent the text did not have; here it reports + // none while the text is indented. + const QModelIndex rootCell = model->index(0, 0, QModelIndex()); + const QModelIndex child = model->index(0, 0, root); + QVERIFY(child.isValid()); + + // Guards before the claim: a probe that cannot see both rows can report + // anything it likes about their relative position. + QVERIFY2(view->visualRect(rootCell).height() > 0, + "the thread row has no height, so nothing about it is measurable"); + QVERIFY2(view->visualRect(child).height() > 0, + "the reply row has no height: it is collapsed or off-screen, and " + "an indent test against it would pass without drawing anything"); + + // The indent is NOT in the geometry. setIndentation(0) means visualRect + // reports the same left edge for both rows, deliberately: CardLayout draws + // the indent inside the card's own rect. Asserting on visualRect here + // would fail against a perfectly indented list, which is the mirror of the + // trap CLAUDE.md records for item 20, where visualRect reported an indent + // the text did not have. + // + // So the real property, as before: where the TEXT lands. It is read off + // the layout, which is what the delegate paints from. + CardLayout::Input threadIn; + threadIn.isMessage = false; + threadIn.depth = 0; + CardLayout::Input replyIn; + replyIn.isMessage = true; + replyIn.depth = + model->data(child, ThreadListModel::MessageDepthRole).toInt(); + QVERIFY2(replyIn.depth > 0, + "the reply reports depth 0, so there is no nesting to measure"); + + const QRect rect = view->visualRect(rootCell); + const CardLayout threadCard = + CardLayout::compute(threadIn, rect, view->font()); + const CardLayout replyCard = + CardLayout::compute(replyIn, rect, view->font()); + + QVERIFY2(replyCard.contentLeft > threadCard.contentLeft, + qPrintable(QStringLiteral("the reply's text starts at x=%1, not " + "right of the thread's at x=%2: the " + "nesting is invisible") + .arg(replyCard.contentLeft) + .arg(threadCard.contentLeft))); + + // And the spine that makes the nesting read as one block rather than as an + // arbitrary offset. + QCOMPARE(replyCard.spines.size(), replyIn.depth); +} + +namespace { + +/// Two threads, the first with one reply, expanded. The shared fixture for the +/// two navigation tests below. +struct NavFixture +{ + QTreeView *view = nullptr; + ThreadListModel *model = nullptr; + QModelIndex root; + QModelIndex reply; +}; + +NavFixture buildNavFixture(MainWindow &window) +{ + NavFixture f; + 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"), + QStringLiteral("unread") }); + first.totalCount = 2; + ThreadSummary second = makeThread(QStringLiteral("T2"), + QStringList{ QStringLiteral("inbox") }); + second.totalCount = 1; + f.model->appendBatch({ first, second }); + + MessageNode rootNode; + rootNode.messageId = QStringLiteral("M1"); + rootNode.threadId = QStringLiteral("T1"); + rootNode.depth = 0; + MessageNode replyNode; + replyNode.messageId = QStringLiteral("M2"); + replyNode.threadId = QStringLiteral("T1"); + replyNode.depth = 1; + f.model->setThreadMessages(QStringLiteral("T1"), { rootNode, replyNode }); + + f.root = f.model->index(0, 0); + f.view->expand(f.root); + f.reply = f.model->index(0, 0, f.root); + return f; +} + +} // 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; + MainWindow window(config); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const NavFixture f = buildNavFixture(window); + QVERIFY(f.reply.isValid()); + QVERIFY2(f.view->isExpanded(f.root), + "the thread is collapsed, so this test would arrow down a flat " + "list and pass against the bug it exists to catch"); + + f.view->setCurrentIndex(f.reply); + + // The defect (item 60): selectRow(current.row() + 1) asked for row 1 UNDER + // T1, which does not exist, so the action did nothing at all. + window.findChild<QAction *>(QStringLiteral("next_thread"))->trigger(); + + QCOMPARE(f.view->currentIndex().data(ThreadListModel::ThreadIdRole) + .toString(), + QStringLiteral("T2")); +} + +void TestMainWindow::altDownSkipsReplies() +{ + const Config config; + MainWindow window(config); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const NavFixture f = buildNavFixture(window); + QVERIFY(f.view->isExpanded(f.root)); + + // From the thread ROOT with its replies showing: one step must land on the + // next THREAD, not on the first reply. That is what makes the action mean + // thread-to-thread while plain Up/Down still steps message-to-message. + f.view->setCurrentIndex(f.root); + window.findChild<QAction *>(QStringLiteral("next_thread"))->trigger(); + + QCOMPARE(f.view->currentIndex().data(ThreadListModel::ThreadIdRole) + .toString(), + QStringLiteral("T2")); + QVERIFY(!f.view->currentIndex().data(ThreadListModel::IsMessageRole) + .toBool()); + + // And back, which is the mirror case the old arithmetic also failed. + window.findChild<QAction *>(QStringLiteral("prev_thread"))->trigger(); + QCOMPARE(f.view->currentIndex().data(ThreadListModel::ThreadIdRole) + .toString(), + QStringLiteral("T1")); + QVERIFY(!f.view->currentIndex().data(ThreadListModel::IsMessageRole) + .toBool()); +} + +void TestMainWindow::bothThreadStepBindingsReachTheAction() +{ + const Config config; + MainWindow window(config); + + // Two bindings per action, which needs setShortcuts rather than + // setShortcut: Ctrl+J/K for a neomutt hand, Alt+Up/Down for a mouse one. + // Alt because Shift+arrows is QTreeView's built-in extend-selection that + // multi-row tagging depends on, and a bare arrow cannot be a window + // shortcut without breaking every text field in the window. + for (const auto &pair : { std::pair<const char *, const char *>{ + "next_thread", "Alt+Down" }, + { "prev_thread", "Alt+Up" } }) { + auto *action = + window.findChild<QAction *>(QString::fromLatin1(pair.first)); + QVERIFY2(action, pair.first); + const QList<QKeySequence> shortcuts = action->shortcuts(); + QVERIFY2(shortcuts.size() >= 2, + qPrintable(QStringLiteral("%1 carries %2 shortcut(s), so the " + "second binding is unreachable") + .arg(QString::fromLatin1(pair.first)) + .arg(shortcuts.size()))); + QVERIFY2(shortcuts.contains( + QKeySequence(QString::fromLatin1(pair.second))), + pair.second); + } +} + +void TestMainWindow::sortChoiceSurvivesRestart() +{ + QStandardPaths::setTestModeEnabled(true); + QFile::remove(MainWindow::uiStatePath()); + + { + const Config config; + MainWindow window(config); + auto *sort = window.findChild<QComboBox *>(QStringLiteral("sortOrder")); + QVERIFY(sort); + QCOMPARE(sort->count(), 2); + QCOMPARE(sort->currentIndex(), 0); // Newest first by default. + sort->setCurrentIndex(1); + window.close(); + } + + const Config config; + MainWindow second(config); + auto *sort = second.findChild<QComboBox *>(QStringLiteral("sortOrder")); + QVERIFY(sort); + QCOMPARE(sort->currentIndex(), 1); + + // A stale or hand-edited file can hold anything, which is the lesson item + // 58 recorded: an out-of-range value must fall back rather than select a + // row that does not exist. + { + QSettings state(MainWindow::uiStatePath(), QSettings::IniFormat); + state.setValue(QStringLiteral("threadlist/sortOrder"), 47); + } + MainWindow third(config); + auto *thirdSort = third.findChild<QComboBox *>(QStringLiteral("sortOrder")); + QCOMPARE(thirdSort->currentIndex(), 0); + + QFile::remove(MainWindow::uiStatePath()); + QStandardPaths::setTestModeEnabled(false); +} + +void TestMainWindow::accountEntriesCarryTheirColour() +{ + // Its own config, not the environment's. Reading the real one made this + // SKIP wherever no accounts are configured, which is a test that asserts + // nothing while reporting success. + QTemporaryDir dir; + const QString path = dir.filePath(QStringLiteral("qtmaildir.conf")); + { + QSettings s(path, QSettings::IniFormat); + s.beginGroup(QStringLiteral("account.work")); + s.setValue(QStringLiteral("maildir"), QStringLiteral("work")); + s.setValue(QStringLiteral("color"), QStringLiteral("#3d7fd1")); + s.endGroup(); + s.beginGroup(QStringLiteral("account.personal")); + s.setValue(QStringLiteral("maildir"), QStringLiteral("personal")); + s.endGroup(); + } + + Config config; + config.load(path); + QCOMPARE(config.accounts().size(), 2); + + MainWindow window(config); + auto *box = window.findChild<QComboBox *>(QStringLiteral("accountBox")); + QVERIFY(box); + QCOMPARE(box->count(), 3); + + // "All accounts" is not an account and carries no swatch. + QVERIFY(!box->itemData(0, Qt::DecorationRole).isValid()); + + // Every real account does, including the one with no color= key: + // colourFor() never fails, deriving a stable colour from the tag name, so + // adding an account and forgetting to colour it degrades to something + // usable rather than to nothing. + QSet<QRgb> seen; + for (int i = 1; i < box->count(); ++i) { + const QVariant swatch = box->itemData(i, Qt::DecorationRole); + QVERIFY2(swatch.isValid(), + qPrintable(QStringLiteral("account %1 carries no swatch") + .arg(box->itemText(i)))); + const QColor colour = swatch.value<QColor>(); + QVERIFY(colour.isValid()); + seen.insert(colour.rgb()); + } + + // Guard: two accounts sharing one colour would make the swatches useless + // as a key to the accent bars, and would let a broken lookup pass. + QCOMPARE(seen.size(), 2); +} + +void TestMainWindow::cardsNeverScrollSideways() +{ + const Config config; + MainWindow window(config); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + // A long subject, so the guard below is not vacuous: this is exactly the + // content that used to make the subject column wider than the viewport. + model->appendBatch({ makeThread( + QStringLiteral("t1"), + QStringList{ QStringLiteral("inbox") }) }); + QApplication::processEvents(); + + // Item 51: clicking a row used to scroll the list sideways, because the + // subject column was wider than the viewport and auto-scroll brought the + // clicked index fully into view. A card is exactly viewport width, so + // there is nowhere to scroll to. + QVERIFY2(view->visualRect(model->index(0, 0)).height() > 0, + "no card is drawn, so there is no layout to assert about"); + QCOMPARE(view->horizontalScrollBar()->minimum(), + view->horizontalScrollBar()->maximum()); +} + +void TestMainWindow::aThreadWithRepliesDrawsAVisibleExpander() { - // The pills are a row-wide strip under the cells, not content of the - // subject cell. Drawn from the subject column's delegate they stop at that - // column's edge, so a thread with several tags loses the last of them; and - // they inherit the column's left edge, which puts them under the subject - // rather than under the row. + // The expander is the ONLY thing saying a thread can be opened, and it took + // four wrong attempts to get on screen before item 53, each of which looked + // correct in code and none of which a geometry or role assertion could see. + // So this counts painted pixels. // - // The property: pills appear to the LEFT of where the subject column - // starts, which no per-cell delegate on that column could produce. + // Painted through the DELEGATE rather than through viewport()->render(). + // The viewport render returns a blank image here: CLAUDE.md records that it + // does so in several ordinary situations, and this test proved it again, + // reporting zero ink over a card the delegate demonstrably paints 2183 + // pixels into. A probe that sees nothing cannot report on anything. const Config config; MainWindow window(config); auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); - ThreadSummary thread = makeThread(QStringLiteral("t1"), {}); - thread.tags = QStringList{ QStringLiteral("mailing-list/SBo"), - QStringLiteral("signed") }; - model->appendBatch({ thread }); + // Two threads: one with replies, one without. The second is the control, + // and without it a test that counts ink would pass on any card. + ThreadSummary withReplies = makeThread( + QStringLiteral("t1"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }); + withReplies.totalCount = 3; + ThreadSummary lone = makeThread( + QStringLiteral("t2"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }); + lone.totalCount = 1; + model->appendBatch({ withReplies, lone }); + + 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); + + const QFont font = view->font(); + const int height = CardLayout::heightFor(font); + + const auto inkInExpander = [&](const QModelIndex &index) { + QImage shot(400, height, QImage::Format_ARGB32); + shot.fill(Qt::white); + QPainter painter(&shot); + QStyleOptionViewItem option; + option.rect = QRect(0, 0, 400, height); + option.font = font; + option.palette = QApplication::palette(); + option.state = QStyle::State_Enabled; + CardDelegate delegate; + delegate.paint(&painter, option, index); + painter.end(); + + const QRect rect = CardDelegate::expanderRectFor(option, index); + int found = 0; + for (int y = rect.top(); y <= rect.bottom() && y < shot.height(); ++y) { + for (int x = rect.left(); x <= rect.right() && x < shot.width(); + ++x) { + if ((shot.pixel(x, y) | 0xff000000) != 0xffffffffu) + ++found; + } + } + + // Guard on the probe itself: prove it can see the card's own text + // before trusting it about the expander. A probe that finds no ink + // anywhere reports "nothing was drawn" whatever the delegate did. + int anyInk = 0; + for (int y = 0; y < shot.height(); ++y) + for (int x = 0; x < shot.width(); ++x) + if ((shot.pixel(x, y) | 0xff000000) != 0xffffffffu) + ++anyInk; + return std::pair<int, int>(found, anyInk); + }; + + const auto [drawn, drawnAnywhere] = inkInExpander(first); + const auto [control, controlAnywhere] = inkInExpander(second); + + QVERIFY2(drawnAnywhere > 0 && controlAnywhere > 0, + "the probe finds no ink on either card, so it cannot report on " + "the expander either"); + + QVERIFY2(drawn > 12, + qPrintable(QStringLiteral("only %1 pixels in the expander's rect: " + "the reply count is clipped or painted " + "over").arg(drawn))); + QVERIFY2(control == 0, + qPrintable(QStringLiteral("a thread with no replies drew %1 " + "pixels where an expander would go") + .arg(control))); +} + +void TestMainWindow::selectingAThreadRowNamesHowManyMessagesItStandsFor() +{ + // With two kinds of row selectable, one selected row no longer says how + // much an action will touch. CLAUDE.md forbids a confirmation dialog for + // tag mutations, so the scope is made visible instead: this is the "before" + // half of that, and the count has to come from the thread's own total, not + // from whatever happens to be expanded. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *status = window.findChild<QLabel *>(QStringLiteral("statusMessage")); + QVERIFY(status); + + ThreadSummary t = makeThread(QStringLiteral("t1"), {}); + t.totalCount = 7; + model->appendBatch({ t }); + + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + // Guard: nothing is expanded, so a count taken from the loaded children + // would read 0 and this test would be measuring the wrong source. + QCOMPARE(model->rowCount(model->index(0, 0, QModelIndex())), 0); + + selectThreadRow(view, 0); + QApplication::processEvents(); + + QVERIFY2(status->text().contains(QStringLiteral("7")), + qPrintable(QStringLiteral("the status bar says '%1', which does " + "not name the 7 messages the thread " + "stands for") + .arg(status->text()))); +} + +void TestMainWindow::selectingAMessageRowReportsNoBulkCount() +{ + // Reading one message is not a bulk action, so it gets no count. A message + // row reporting "1 thread selected" would be actively wrong about what an + // action would touch. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *status = window.findChild<QLabel *>(QStringLiteral("statusMessage")); + QVERIFY(status); + + ThreadSummary t = makeThread(QStringLiteral("t1"), {}); + t.totalCount = 3; + model->appendBatch({ t }); + + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("t1"); + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t1"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t1"), { root, reply }); + + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const QModelIndex threadRow = model->index(0, 0, QModelIndex()); + view->expand(threadRow); + QApplication::processEvents(); + + const QModelIndex messageRow = model->index(0, 0, threadRow); + QVERIFY(model->isMessageRow(messageRow)); + + view->selectionModel()->select( + messageRow, + QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(messageRow); + QApplication::processEvents(); + + QVERIFY2(!status->text().contains(QStringLiteral("thread")), + qPrintable(QStringLiteral("a single message row reports '%1', " + "which claims a thread-wide scope it " + "does not have") + .arg(status->text()))); +} + +void TestMainWindow::anActionOnAThreadRowSaysItHitTheWholeThread() +{ + // The "after" half. Undo is the safety net this project chose over a + // confirmation dialog, and undo is only usable if the user can tell that + // something bigger than they intended just happened. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *status = window.findChild<QLabel *>(QStringLiteral("statusMessage")); + QVERIFY(status); + + ThreadSummary t = makeThread(QStringLiteral("t1"), {}); + t.totalCount = 7; + model->appendBatch({ t }); + + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + selectThreadRow(view, 0); + QApplication::processEvents(); + + auto *archive = window.findChild<QAction *>(QStringLiteral("archive")); + QVERIFY2(archive, "no archive action to trigger"); + archive->trigger(); + + // Read BEFORE processEvents, deliberately. This binary has no worker + // (backlog item 36), so the queued applyTagsToThreads reaches a throwaway + // database that has never heard of thread t1 and answers with + // errorOccurred, which overwrites the status bar. Draining the event loop + // here would assert on that error rather than on the scope message, and + // the test would fail against correct code. + const QString message = status->text(); + + QVERIFY2(message.contains(QStringLiteral("7")), + qPrintable(QStringLiteral("after archiving a 7-message thread the " + "status bar says '%1', which does not " + "say how much was touched") + .arg(message))); + + // And it must say the whole thread went, not merely how many messages: the + // count alone does not distinguish "7 messages you picked" from "7 messages + // because you picked their thread". + QVERIFY2(message.contains(QStringLiteral("whole thread")), + qPrintable(QStringLiteral("the status bar says '%1', which does " + "not say the action took the whole " + "thread") + .arg(message))); +} + +void TestMainWindow::anActionOnAMessageRowTagsThatMessageNotTheThread() +{ + // The routing itself, which nothing else here can see. A message row sent + // down the THREAD path produces the same undo depth and the same status + // text while tagging every sibling in the conversation: a mutation that did + // exactly that passed the entire suite, so this test exists because that + // gap was found rather than because the path looked risky. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + ThreadSummary t = makeThread(QStringLiteral("t1"), {}); + t.totalCount = 3; + model->appendBatch({ t }); + + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("t1"); + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t1"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t1"), { root, reply }); + + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const QModelIndex threadRow = model->index(0, 0, QModelIndex()); + view->expand(threadRow); + QApplication::processEvents(); + + const QModelIndex messageRow = model->index(0, 0, threadRow); + QVERIFY(model->isMessageRow(messageRow)); + + view->selectionModel()->select( + messageRow, + QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(messageRow); + QApplication::processEvents(); + + auto *archive = window.findChild<QAction *>(QStringLiteral("archive")); + QVERIFY(archive); + archive->trigger(); + + // The change must carry the MESSAGE id and no thread id. Sent as a thread + // id it would archive the root and every other reply along with it. + QCOMPARE(window.pendingMessageIdsForTesting(), + QStringList{ QStringLiteral("m1@example.org") }); + QVERIFY2(window.pendingThreadIdsForTesting().isEmpty(), + qPrintable(QStringLiteral("the action was sent for thread(s) %1: a " + "message row must not tag its siblings") + .arg(window.pendingThreadIdsForTesting() + .join(QStringLiteral(", "))))); + + // And it is undoable, on its own terms rather than the thread's. + QCOMPARE(window.undoDepthForTesting(), 1); +} + +void TestMainWindow::selectingAMessageRowTargetsThatMessageNotItsThread() +{ + // test_mainwindow has no worker (backlog item 36), so this cannot assert on + // what the pane renders. What it CAN assert is the decision the UI makes: + // a message row must stop tracking a current thread, or a reply arriving + // for either kind of selection cannot tell which one it belongs to. + // + // The trap this covers is specific. threadAt() takes a TOP-LEVEL row + // number, and a child's row number indexes its siblings, so handing a + // message row's number to it loads whichever thread happens to sit at that + // position in the list. Row 0 under a thread is a plausible-looking wrong + // answer, which is why the fixture puts the reply under the SECOND thread. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + ThreadSummary first = makeThread(QStringLiteral("t1"), {}); + ThreadSummary second = makeThread(QStringLiteral("t2"), {}); + second.totalCount = 2; + model->appendBatch({ first, second }); + + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("t2"); + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t2"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t2"), { root, reply }); window.resize(1400, 300); window.show(); QVERIFY(QTest::qWaitForWindowExposed(&window)); + + // Start on a thread row, so the transition to a message row is what is + // being observed rather than the initial state. + const QModelIndex threadRow = model->index(1, 0, QModelIndex()); + selectThreadRow(view, 1); QApplication::processEvents(); + QCOMPARE(window.currentThreadId(), QStringLiteral("t2")); - const int subjectLeft = - view->columnViewportPosition(ThreadListModel::SubjectColumn); - QVERIFY2(subjectLeft > 40, - qPrintable(QStringLiteral("the subject column starts at x=%1, too " - "close to the left edge to tell a " - "row-wide strip from a subject-cell one") - .arg(subjectLeft))); - // The strip must have somewhere to paint that the subject cell does not - // reach, or this test cannot fail. - QVERIFY2(subjectLeft < view->viewport()->width(), - qPrintable(QStringLiteral("the subject column is off-screen " - "(x=%1, viewport %2), so nothing it " - "draws is measurable") - .arg(subjectLeft) - .arg(view->viewport()->width()))); + view->expand(threadRow); + QApplication::processEvents(); + + const QModelIndex messageRow = model->index(0, 0, threadRow); + QVERIFY(messageRow.isValid()); + QVERIFY2(model->isMessageRow(messageRow), + "the fixture did not produce a message row, so this test would " + "assert nothing about one"); + + view->selectionModel()->select( + messageRow, + QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(messageRow); + QApplication::processEvents(); + + // The thread is no longer what the pane is about. Left set, a late + // loadThread reply would repaint the whole conversation over the single + // message the user asked for. + QVERIFY2(window.currentThreadId().isEmpty(), + qPrintable(QStringLiteral("selecting a reply left the current " + "thread set to '%1': the pane is still " + "tracking the conversation") + .arg(window.currentThreadId()))); +} + +void TestMainWindow::clickingTheExpanderTogglesTheThread() +{ + // The glyph being VISIBLE and the glyph being CLICKABLE are separate + // properties, and the pixel test for the first passes happily against a + // triangle nothing can hit. Turning off rootIsDecorated to stop the style + // drawing its own dot under ours also removed the style's hit area, so the + // expander rendered perfectly and did nothing. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + ThreadSummary t = makeThread( + QStringLiteral("t1"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }); + t.totalCount = 3; + model->appendBatch({ t }); + + window.resize(1400, 300); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + QApplication::processEvents(); + + const QModelIndex root = model->index(0, 0, QModelIndex()); + const QModelIndex subject = + model->index(0, 0, QModelIndex()); + const QRect rect = view->visualRect(subject); + + // Guards: the row is drawn, it claims to have replies, and it starts + // collapsed. Without the last one a toggle test can pass by doing nothing. + QVERIFY2(rect.height() > 0, "the thread row is not on screen"); + QVERIFY(model->data(subject, ThreadListModel::HasRepliesRole).toBool()); + QVERIFY(!view->isExpanded(root)); + + // Aimed at the rect the delegate reports, not at one reconstructed here: + // the drawn target and the clickable one cannot drift if both come from + // the same call. + QStyleOptionViewItem option; + option.rect = rect; + option.font = view->font(); + const QRect expander = CardDelegate::expanderRectFor(option, subject); + QVERIFY2(!expander.isEmpty(), "the card offers no expander to click"); + const QPoint hit = expander.center(); + + QTest::mouseClick(view->viewport(), Qt::LeftButton, Qt::NoModifier, hit); + QApplication::processEvents(); + QVERIFY2(view->isExpanded(root), + "clicking the expander did not open the thread"); + + QTest::mouseClick(view->viewport(), Qt::LeftButton, Qt::NoModifier, hit); + QApplication::processEvents(); + QVERIFY2(!view->isExpanded(root), + "clicking the expander again did not close the thread"); +} + +void TestMainWindow::replyRowsKeepTheirTextUnderTheThreadLine() +{ + // paintEvent runs AFTER the cells, so anything it fills across a reply row + // covers the text the delegate just drew. The tint and the thread line are + // both painted there, which makes this the obvious way to ship a block of + // blank rows. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + ThreadSummary t = makeThread( + QStringLiteral("t1"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }); + t.totalCount = 2; + model->appendBatch({ t }); + + MessageNode first; + first.messageId = QStringLiteral("m0@example.org"); + first.threadId = QStringLiteral("t1"); + first.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t1"); + reply.from = QStringLiteral("A Replier <replier@example.org>"); + reply.subject = QStringLiteral("Re: a subject"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t1"), { first, reply }); + + window.resize(1400, 300); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const QModelIndex root = model->index(0, 0, QModelIndex()); + view->expand(root); + QApplication::processEvents(); + + const QModelIndex child = + model->index(0, 0, root); + const QRect rect = view->visualRect(child); + QVERIFY2(rect.height() > 0, "the reply row is not on screen"); QImage shot(view->viewport()->size(), QImage::Format_ARGB32); shot.fill(Qt::transparent); view->viewport()->render(&shot); - // Count pixels matching the tag colours EXACTLY, not "saturated" pixels. - // A looser test counts the antialiased edge of the selection highlight - // blending into the background, which is several hundred distinct - // near-background colours and passes whatever the strip does. Both earlier - // versions of this test did precisely that. - QSet<QRgb> pillColours; - const QVariantList colours = - model->index(0, ThreadListModel::SubjectColumn) - .data(ThreadListModel::PillColoursRole).toList(); - QVERIFY2(!colours.isEmpty(), "the model supplied no pill colours"); - for (const QVariant &colour : colours) - pillColours.insert(colour.value<QColor>().rgb()); - - const int rowHeight = view->rowHeight(0); - QVERIFY(rowHeight > 0); - - int chipPixels = 0; - for (int y = 0; y < qMin(rowHeight, shot.height()); ++y) { - for (int x = 0; x < qMin(subjectLeft, shot.width()); ++x) { - if (pillColours.contains(shot.pixel(x, y) | 0xff000000)) - ++chipPixels; + // Count pixels in the sender cell that differ from the row's own tint. + // Text is the only thing that can produce them. + const QRgb tint = ThreadListModel::replyBackground().rgb() | 0xff000000; + int textPixels = 0; + for (int y = rect.top(); y < qMin(rect.bottom(), shot.height()); ++y) { + for (int x = rect.left(); x < qMin(rect.right(), shot.width()); ++x) { + if ((shot.pixel(x, y) | 0xff000000) != tint) + ++textPixels; } } - QVERIFY2(chipPixels > 0, - "no pill-coloured pixels left of the subject column: the strip is " - "still confined to that cell rather than spanning the row"); + QVERIFY2(textPixels > 20, + qPrintable(QStringLiteral("only %1 non-background pixels in the " + "reply's sender cell: the row was " + "painted over after its text was drawn") + .arg(textPixels))); } void TestMainWindow::aSelectedReadThreadIsNotDimmedIntoTheHighlight() @@ -607,7 +1440,7 @@ void TestMainWindow::aSelectedReadThreadIsNotDimmedIntoTheHighlight() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); // Both rows READ, so both are dimmed and neither is bold: the only thing @@ -633,10 +1466,10 @@ void TestMainWindow::aSelectedReadThreadIsNotDimmedIntoTheHighlight() // dimmed row switches it to the highlight's own text colour, so the two // rows MUST differ; comparing two identically-styled rows would pass // against a delegate that did nothing at all. - view->selectRow(0); + selectThreadRow(view, 0); QApplication::processEvents(); - const int rowHeight = view->rowHeight(0); + const int rowHeight = threadRowHeight(view, 0); QVERIFY(rowHeight > 0); QImage shot(view->viewport()->size(), QImage::Format_ARGB32); @@ -657,15 +1490,15 @@ void TestMainWindow::aSelectedReadThreadIsNotDimmedIntoTheHighlight() QVERIFY2(delegate, "the thread view has no styled delegate"); const QModelIndex index = - model->index(0, ThreadListModel::SubjectColumn); + model->index(0, 0); // initStyleOption is protected, so the resolved palette is reached the way // the painter does: through a subclass that exposes it. - struct Probe : SubjectDelegate { - using SubjectDelegate::initStyleOption; + struct Probe : CardDelegate { + using CardDelegate::initStyleOption; }; const auto *probe = static_cast<const Probe *>( - static_cast<const SubjectDelegate *>(delegate)); + static_cast<const CardDelegate *>(delegate)); probe->initStyleOption(&selected, index); probe->initStyleOption(&unselected, index); @@ -724,7 +1557,7 @@ void TestMainWindow::markAllReadActsOnEveryRowAndUndoesInOneStep() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *action = window.findChild<QAction *>(QStringLiteral("mark_all_read")); QVERIFY(action); @@ -746,7 +1579,7 @@ void TestMainWindow::markAllReadActsOnEveryRowAndUndoesInOneStep() // One row selected, to prove the action ignores the selection rather than // acting on it. - view->selectRow(0); + selectThreadRow(view, 0); action->trigger(); @@ -831,7 +1664,7 @@ void TestMainWindow::markReadTimerRestartsRatherThanStacking() QVERIFY(model); auto *timer = window.findChild<QTimer *>(QStringLiteral("markReadTimer")); QVERIFY(timer); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), @@ -841,13 +1674,13 @@ void TestMainWindow::markReadTimerRestartsRatherThanStacking() makeThread(QStringLiteral("t3"), { QStringLiteral("unread") }) }); - view->selectRow(0); + selectThreadRow(view, 0); QVERIFY2(timer->isActive(), "no timer armed for an unread thread"); // Move on before it can fire. One timer stays armed, not three. - view->selectRow(1); + selectThreadRow(view, 1); QVERIFY(timer->isActive()); - view->selectRow(2); + selectThreadRow(view, 2); QVERIFY(timer->isActive()); // Exactly one timer exists at all, which is what "restarted, not stacked" @@ -867,7 +1700,7 @@ void TestMainWindow::markReadTimerIsNotArmedForAReadThread() QVERIFY(model); auto *timer = window.findChild<QTimer *>(QStringLiteral("markReadTimer")); QVERIFY(timer); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("read"), @@ -875,16 +1708,16 @@ void TestMainWindow::markReadTimerIsNotArmedForAReadThread() makeThread(QStringLiteral("unread"), { QStringLiteral("unread") }) }); - view->selectRow(0); + selectThreadRow(view, 0); QVERIFY2(!timer->isActive(), "armed a timer for an already-read thread"); // And the unread one still arms, so this is not "never arms". - view->selectRow(1); + selectThreadRow(view, 1); QVERIFY(timer->isActive()); // Moving back to a read thread disarms it again, rather than leaving the // previous thread's timer running to fire against the wrong row. - view->selectRow(0); + selectThreadRow(view, 0); QVERIFY(!timer->isActive()); } @@ -909,12 +1742,12 @@ void TestMainWindow::markReadCanBeDisabled() QVERIFY(model); auto *timer = window.findChild<QTimer *>(QStringLiteral("markReadTimer")); QVERIFY(timer); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), { QStringLiteral("unread") }) }); - view->selectRow(0); + selectThreadRow(view, 0); QVERIFY2(!timer->isActive(), "a negative mark_read_delay_ms must disable the timer"); @@ -1091,7 +1924,7 @@ void TestMainWindow::selectAllIsBoundAndSelectsEveryRow() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), {}), @@ -1120,7 +1953,7 @@ void TestMainWindow::aMultiRowSelectionDoesNotArmTheMarkReadTimer() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *timer = window.findChild<QTimer *>(QStringLiteral("markReadTimer")); QVERIFY(timer); @@ -1134,7 +1967,7 @@ void TestMainWindow::aMultiRowSelectionDoesNotArmTheMarkReadTimer() // Sweep down as Shift+arrow does: current moves onto a row while the // selection already spans more than one. - view->selectRow(0); + selectThreadRow(view, 0); view->selectionModel()->select( model->index(1, 0), QItemSelectionModel::Select | QItemSelectionModel::Rows); @@ -1159,7 +1992,7 @@ void TestMainWindow::growingASelectionCancelsAnAlreadyArmedTimer() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *timer = window.findChild<QTimer *>(QStringLiteral("markReadTimer")); QVERIFY(timer); @@ -1169,7 +2002,7 @@ void TestMainWindow::growingASelectionCancelsAnAlreadyArmedTimer() makeThread(QStringLiteral("t2"), { QStringLiteral("unread") }) }); - view->selectRow(0); + selectThreadRow(view, 0); QVERIFY2(timer->isActive(), "no timer armed for a single unread thread"); // Extend to a second row, as Shift+click would. @@ -1191,7 +2024,7 @@ void TestMainWindow::collapsingBackToOneRowLoadsThatThreadAgain() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *timer = window.findChild<QTimer *>(QStringLiteral("markReadTimer")); QVERIFY(timer); @@ -1205,7 +2038,7 @@ void TestMainWindow::collapsingBackToOneRowLoadsThatThreadAgain() QVERIFY(!timer->isActive()); // Back to one row, as a plain click would leave it. - view->selectRow(1); + selectThreadRow(view, 1); QVERIFY2(timer->isActive(), "collapsing back to one row did not resume mark-read"); @@ -1221,7 +2054,7 @@ void TestMainWindow::theStatusBarReportsAMultiRowSelection() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *status = window.findChild<QLabel *>(QStringLiteral("statusMessage")); QVERIFY2(status, "no status label to report into"); @@ -1246,7 +2079,7 @@ void TestMainWindow::clearSelectionBlanksThePaneAndDeselects() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), {}), @@ -1256,7 +2089,7 @@ void TestMainWindow::clearSelectionBlanksThePaneAndDeselects() // and what CLAUDE.md requires: selectAll() on a fresh view emits no // currentRowChanged at all, so a test starting there passes against a // missing guard. - view->selectRow(0); + selectThreadRow(view, 0); QCOMPARE(view->selectionModel()->selectedRows().size(), 1); auto *action = window.findChild<QAction *>(QStringLiteral("clear_selection")); @@ -1302,13 +2135,13 @@ void TestMainWindow::clearPaneLeavesTheSelectionAlone() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), {}), makeThread(QStringLiteral("t2"), {}) }); - view->selectRow(0); + selectThreadRow(view, 0); QCOMPARE(view->selectionModel()->selectedRows().size(), 1); auto *action = window.findChild<QAction *>(QStringLiteral("clear_pane")); @@ -1421,7 +2254,7 @@ void TestMainWindow::theThreadListOffersAContextMenu() const Config config; MainWindow window(config); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); QCOMPARE(view->contextMenuPolicy(), Qt::CustomContextMenu); @@ -1464,7 +2297,7 @@ void TestMainWindow::aSecondRowBlanksThePaneNotOnlyAThird() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *timer = window.findChild<QTimer *>(QStringLiteral("markReadTimer")); QVERIFY(timer); @@ -1477,7 +2310,7 @@ void TestMainWindow::aSecondRowBlanksThePaneNotOnlyAThird() { QStringLiteral("unread") }) }); // One row: ordinary reading, so a timer is armed and a thread is current. - view->selectRow(0); + selectThreadRow(view, 0); QCOMPARE(view->selectionModel()->selectedRows().size(), 1); QVERIFY(timer->isActive()); @@ -1829,13 +2662,13 @@ void TestMainWindow::escapeBlanksTheMessagePane() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), {}), makeThread(QStringLiteral("t2"), {}) }); - view->selectRow(0); + selectThreadRow(view, 0); QVERIFY2(!window.currentThreadId().isEmpty(), "no thread was opened to blank"); @@ -1856,14 +2689,14 @@ void TestMainWindow::deleteTogglesOnAnAlreadyDeletedThread() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *action = window.findChild<QAction *>(QStringLiteral("delete")); QVERIFY(action); model->appendBatch({ makeThread(QStringLiteral("t1"), { QStringLiteral("deleted") }) }); - view->selectRow(0); + selectThreadRow(view, 0); action->trigger(); @@ -1884,7 +2717,7 @@ void TestMainWindow::deleteOnAMixedSelectionDeletesRatherThanSplittingIt() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *action = window.findChild<QAction *>(QStringLiteral("delete")); QVERIFY(action); @@ -1935,7 +2768,7 @@ void TestMainWindow::theSelectionCountIsStateAndDoesNotExpire() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *status = window.findChild<QLabel *>(QStringLiteral("statusMessage")); QVERIFY(status); @@ -2059,13 +2892,13 @@ void TestMainWindow::anEditDuringABackgroundSyncIsNotSentYet() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *action = window.findChild<QAction *>(QStringLiteral("flag")); QVERIFY2(action, "no flag action registered"); model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); - view->selectRow(0); + selectThreadRow(view, 0); // A cron sync takes the lock. QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", @@ -2090,13 +2923,13 @@ void TestMainWindow::aHeldEditIsSentWhenTheBackgroundSyncEnds() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *action = window.findChild<QAction *>(QStringLiteral("flag")); QVERIFY(action); model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); - view->selectRow(0); + selectThreadRow(view, 0); QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, @@ -2125,7 +2958,7 @@ void TestMainWindow::aHeldEditCountsAsUnsynced() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *action = window.findChild<QAction *>(QStringLiteral("flag")); QVERIFY(action); @@ -2134,7 +2967,7 @@ void TestMainWindow::aHeldEditCountsAsUnsynced() QVERIFY2(label->isHidden(), "the indicator starts hidden at zero"); model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); - view->selectRow(0); + selectThreadRow(view, 0); QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, @@ -2162,7 +2995,7 @@ void TestMainWindow::anUnreadableLockTableStillSendsTheEdit() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *action = window.findChild<QAction *>(QStringLiteral("flag")); QVERIFY(action); @@ -2173,7 +3006,7 @@ void TestMainWindow::anUnreadableLockTableStillSendsTheEdit() QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, SyncMonitor::State::Running)); - view->selectRow(0); + selectThreadRow(view, 0); action->trigger(); QVERIFY2(window.hasEditAwaitingSend(), "the edit was not held during a running sync, so this test is not " @@ -2189,7 +3022,7 @@ void TestMainWindow::anUnreadableLockTableStillSendsTheEdit() "platform without /proc/locks"); // And a NEW edit is sent rather than held. - view->selectRow(1); + selectThreadRow(view, 1); action->trigger(); QVERIFY2(!window.hasEditAwaitingSend(), "an unreadable lock table held a new edit, so writes never resume"); @@ -2206,7 +3039,7 @@ void TestMainWindow::aRejectedWriteKeepsEarlierUndoHistory() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); auto *flag = window.findChild<QAction *>(QStringLiteral("flag")); QVERIFY(flag); @@ -2215,7 +3048,7 @@ void TestMainWindow::aRejectedWriteKeepsEarlierUndoHistory() model->appendBatch({ makeThread(QStringLiteral("t1"), { QStringLiteral("inbox") }) }); - view->selectRow(0); + selectThreadRow(view, 0); // One edit that succeeds, so there is history worth keeping. archive->trigger(); @@ -2609,7 +3442,7 @@ void TestMainWindow::theImportantActionStillWritesTheFlaggedTag() auto *model = window.findChild<ThreadListModel *>(); QVERIFY(model); - auto *view = window.findChild<QTableView *>(); + auto *view = window.findChild<QTreeView *>(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), @@ -2619,7 +3452,7 @@ void TestMainWindow::theImportantActionStillWritesTheFlaggedTag() // below would pass against an action that did nothing at all. QVERIFY(!model->threadAt(0).isFlagged()); - view->selectRow(0); + selectThreadRow(view, 0); auto *action = window.findChild<QAction *>(QStringLiteral("flag")); QVERIFY(action); diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index b419915..88dcf0b 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -39,6 +39,8 @@ private slots: void malformedQueryYieldsNoThreads(); void unreadableConfigEmitsError(); void queryPassesGenerationThrough(); + void oldestFirstReversesTheOrder(); + void theSortOrderCrossesAQueuedCall(); void loadThreadReturnsMessagesOldestFirst(); void loadThreadMarksMatchedMessages(); @@ -58,6 +60,11 @@ private slots: void requestAllTagsReturnsSortedTags(); void requestAllTagsOnUnreadableConfigEmitsError(); + void loadMessageReturnsOnlyThatMessage(); + void loadMessageOnAnUnknownIdReturnsNothing(); + void loadThreadTreeReportsReplyDepth(); + void loadThreadTreeCarriesTheFactsARowNeeds(); + void requestCountsAnswersOneCountPerQuery(); void requestCountsKeepsPositionOnAnInvalidQuery(); void requestDatabaseStatsCountsMessagesNotThreads(); @@ -68,7 +75,9 @@ private: QStringList tagsOf(const QString &messageId); QVector<MessageRef> messagesOfThread(const QString &threadId, const QString &matchQuery = QString()); - QVector<ThreadSummary> runQuery(const QString &query); + QVector<ThreadSummary> runQuery( + const QString &query, + NotmuchWorker::SortOrder sort = NotmuchWorker::NewestFirst); QString threadIdOf(const QString &subject); NotmuchFixture m_fixture; @@ -108,13 +117,14 @@ void TestNotmuchWorker::initTestCase() QVERIFY2(m_fixture.index(), qPrintable(m_fixture.error())); } -QVector<ThreadSummary> TestNotmuchWorker::runQuery(const QString &query) +QVector<ThreadSummary> TestNotmuchWorker::runQuery( + const QString &query, NotmuchWorker::SortOrder sort) { NotmuchWorker worker(m_fixture.configPath()); QSignalSpy ready(&worker, &NotmuchWorker::threadsReady); QSignalSpy finished(&worker, &NotmuchWorker::queryFinished); - worker.runQuery(query, 1); + worker.runQuery(query, 1, sort); QVector<ThreadSummary> all; for (const QList<QVariant> &args : ready) @@ -158,6 +168,91 @@ QStringList TestNotmuchWorker::tagsOf(const QString &messageId) return {}; } +void TestNotmuchWorker::loadMessageReturnsOnlyThatMessage() +{ + // a2 is a reply in a two-message thread. Selecting a reply row must render + // that message alone; loadThread would hand back the whole thread and the + // pane would show the conversation the user was trying to look inside. + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy loaded(&worker, &NotmuchWorker::messageLoaded); + worker.loadMessage(QStringLiteral("a2@example.org"), 1); + + QCOMPARE(loaded.count(), 1); + const auto messages = loaded.first().at(0).value<QVector<MessageRef>>(); + + QCOMPARE(messages.size(), 1); + QCOMPARE(messages.first().messageId, QStringLiteral("a2@example.org")); + QVERIFY(!messages.first().filePath.isEmpty()); + + // matched, so the pane renders it expanded rather than as a stub. The user + // asked for this message by clicking it, which is as matched as it gets. + QVERIFY(messages.first().matched); +} + +void TestNotmuchWorker::loadMessageOnAnUnknownIdReturnsNothing() +{ + // Empty rather than an error: a stale row after a reindex is an ordinary + // race, not a failure worth a message in the status bar. + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy loaded(&worker, &NotmuchWorker::messageLoaded); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + + worker.loadMessage(QStringLiteral("nonexistent@example.org"), 1); + + QCOMPARE(loaded.count(), 1); + QVERIFY(loaded.first().at(0).value<QVector<MessageRef>>().isEmpty()); + QCOMPARE(errors.count(), 0); +} + +void TestNotmuchWorker::loadThreadTreeReportsReplyDepth() +{ + // Thread A is a root plus one reply carrying In-Reply-To, which is what + // notmuch threads on. Without that header the two would be separate threads + // and this test would assert nothing about depth. + const QString threadId = threadIdOf(QStringLiteral("Release notes")); + QVERIFY(!threadId.isEmpty()); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy loaded(&worker, &NotmuchWorker::threadTreeLoaded); + worker.loadThreadTree(threadId, QString(), 1); + + QCOMPARE(loaded.count(), 1); + const auto nodes = loaded.first().at(0).value<QVector<MessageNode>>(); + + QCOMPARE(nodes.size(), 2); + QCOMPARE(nodes.at(0).messageId, QStringLiteral("a1@example.org")); + QCOMPARE(nodes.at(0).depth, 0); + QCOMPARE(nodes.at(1).messageId, QStringLiteral("a2@example.org")); + QCOMPARE(nodes.at(1).depth, 1); +} + +void TestNotmuchWorker::loadThreadTreeCarriesTheFactsARowNeeds() +{ + // A row is drawn without opening the message, so the walk has to read the + // headers. loadThread does not, which is why a separate signal exists. + const QString threadId = threadIdOf(QStringLiteral("Release notes")); + QVERIFY(!threadId.isEmpty()); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy loaded(&worker, &NotmuchWorker::threadTreeLoaded); + worker.loadThreadTree(threadId, QString(), 1); + + QCOMPARE(loaded.count(), 1); + const auto nodes = loaded.first().at(0).value<QVector<MessageNode>>(); + QCOMPARE(nodes.size(), 2); + + const MessageNode &reply = nodes.at(1); + QVERIFY(reply.from.contains(QStringLiteral("bob@example.org"))); + QCOMPARE(reply.subject, QStringLiteral("Re: Release notes")); + QVERIFY(reply.date.isValid()); + QVERIFY(!reply.filePath.isEmpty()); + + // Every node names its thread, so a batch does not need the caller to keep + // track of which thread it asked about. + QCOMPARE(reply.threadId, threadId); + QCOMPARE(nodes.at(0).threadId, threadId); +} + void TestNotmuchWorker::queryReturnsAllThreads() { const QVector<ThreadSummary> threads = runQuery(QStringLiteral("*")); @@ -234,6 +329,45 @@ void TestNotmuchWorker::queryPassesGenerationThrough() QCOMPARE(finished.first().at(1).value<quint64>(), quint64(42)); } +void TestNotmuchWorker::oldestFirstReversesTheOrder() +{ + const QVector<ThreadSummary> newest = runQuery(QStringLiteral("*")); + const QVector<ThreadSummary> oldest = + runQuery(QStringLiteral("*"), NotmuchWorker::OldestFirst); + + QCOMPARE(oldest.size(), newest.size()); + + // The guard: with fewer than two threads, or with every thread carrying + // the same date, a reversal is indistinguishable from no sorting at all + // and every assertion below would pass against a hardcoded order. + QVERIFY(newest.size() >= 2); + QVERIFY(newest.first().date != newest.last().date); + + QCOMPARE(oldest.first().threadId, newest.last().threadId); + QCOMPARE(oldest.last().threadId, newest.first().threadId); +} + +void TestNotmuchWorker::theSortOrderCrossesAQueuedCall() +{ + // MainWindow reaches the worker with invokeMethod(..., QueuedConnection) + // across a thread boundary, and a Q_ARG whose type the meta-object system + // does not know FAILS AT RUNTIME with a warning, not at compile time. So + // the enum's registration is asserted here rather than assumed from Q_ENUM. + QVERIFY2(QMetaType::fromName("NotmuchWorker::SortOrder").isValid(), + "SortOrder is not a registered metatype, so the queued runQuery " + "call will drop its sort argument at runtime"); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy ready(&worker, &NotmuchWorker::threadsReady); + + // The real call shape, invoked by NAME exactly as MainWindow does. + QVERIFY(QMetaObject::invokeMethod( + &worker, "runQuery", Qt::DirectConnection, + Q_ARG(QString, QStringLiteral("*")), Q_ARG(quint64, 1), + Q_ARG(NotmuchWorker::SortOrder, NotmuchWorker::OldestFirst))); + QCOMPARE(ready.size(), 1); +} + void TestNotmuchWorker::loadThreadReturnsMessagesOldestFirst() { const QString threadId = threadIdOf(QStringLiteral("Release notes")); diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index f84bbab..aa71080 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -27,6 +27,20 @@ class TestThreadListModel : public QObject { Q_OBJECT private slots: + void messageNodeHoldsDisplayFacts(); + void rootRowsSurviveTheTreeConversion(); + void repliesBecomeChildRowsUnderTheirThread(); + void messageRowsShowTheirOwnSenderAndSubject(); + void modelHasOneColumn(); + void aFlatThreadStillListsItsReplies(); + void theRootCardKnowsItsOwnMessage(); + void replyShowsOnlyItsOwnTags(); + void replySharingEveryThreadTagShowsNone(); + void reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem(); + void anUnexpandedMultiMessageThreadOffersAnExpander(); + void scopeFollowsTheSelectedRowKind(); + void scopeCountsEveryMessageOfAnUnexpandedThread(); + void scopeHonoursAMixedSelectionWithoutEscalating(); void startsEmpty(); void accountKeysComeFromTheAccountTags(); void accountKeysCoverAThreadSpanningTwoAccounts(); @@ -35,22 +49,20 @@ private slots: void appendingEmptyBatchIsNoOp(); void clearResetsModel(); void reportsSubjectAndAuthors(); - void subjectShowsMessageCountOnlyForRealThreads(); + void theReplyCountExcludesTheRootMessage(); void unreadThreadsRenderBold(); void readThreadsAreDimmedAndUnreadAreNot(); void flaggedThreadsShowAStar(); void pillTagsExcludeWhatTheRowAlreadyShows(); - void theStarColumnIsNarrowAndCarriesNoText(); void theUnreadCueDoesNotDependOnFontWeight(); void aDoomedThreadKeepsItsContrastEvenWhenRead(); - void tagsAreTheFirstColumnAndSubjectTheLast(); void accountTagBecomesAChipLabel(); void unreadStylingSurvivesAnAccountChip(); void accountChipUsesTheConfiguredColour(); void deletedThreadsAreRedAndStruckThrough(); - void attachmentColumnIsFirstAndMarksOnlyTaggedThreads(); + void attachmentIsMarkedOnlyOnTaggedThreads(); void spamThreadsAreOrangeAndStruckThrough(); - void doomedStylingCoversEveryColumn(); + void doomedStylingCoversTheWholeCard(); void ordinaryThreadsCarryNoRowColour(); void threadIdIsReachableFromAnIndex(); void invalidIndexesReturnNothing(); @@ -76,6 +88,238 @@ static ThreadSummary makeThread(const QString &id, const QString &subject) return t; } +static MessageNode makeNode(const QString &id, int depth, + const QString &from = QStringLiteral("Alice"), + const QString &subject = QStringLiteral("Re: Hi")) +{ + MessageNode n; + n.messageId = id; + n.threadId = QStringLiteral("t1"); + n.from = from; + n.subject = subject; + n.date = QDateTime::fromSecsSinceEpoch(1750000000); + n.depth = depth; + return n; +} + +void TestThreadListModel::repliesBecomeChildRowsUnderTheirThread() +{ + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")) }); + + // Depth 0 is the thread's FIRST message and belongs on the root row, not in + // the children: the user's model is "N replies", so a thread of three shows + // one root and two children. + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1), + makeNode(QStringLiteral("m2@example.org"), 2) }); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + QCOMPARE(model.rowCount(root), 2); + + const QModelIndex child = + model.index(0, 0, root); + QVERIFY(child.isValid()); + QCOMPARE(model.parent(child), model.index(0, 0, QModelIndex())); + + QVERIFY(model.data(child, ThreadListModel::IsMessageRole).toBool()); + QCOMPARE(model.data(child, ThreadListModel::MessageIdRole).toString(), + QStringLiteral("m1@example.org")); + + // A message row still belongs to a thread, so a caller that only needs the + // containing thread does not have to walk up itself. + QCOMPARE(model.data(child, ThreadListModel::ThreadIdRole).toString(), + QStringLiteral("t1")); + + // 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()); + QCOMPARE(model.data(root, ThreadListModel::MessageIdRole).toString(), + QStringLiteral("m0@example.org")); + + QAbstractItemModelTester tester( + &model, QAbstractItemModelTester::FailureReportingMode::Warning); + Q_UNUSED(tester); +} + +void TestThreadListModel::messageRowsShowTheirOwnSenderAndSubject() +{ + // A reply's row shows the REPLY's sender, not the thread's author summary. + // Reading the thread's fields for a child row is the obvious mistake and + // would look almost right, since the first sender is usually in both. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")) }); + model.setThreadMessages( + QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1, + QStringLiteral("Bob <bob@example.org>"), + QStringLiteral("Re: A subject")) }); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + const QModelIndex reply = model.index(0, 0, root); + + QCOMPARE(model.data(reply, ThreadListModel::SendersRole).toString(), + QStringLiteral("Bob <bob@example.org>")); + QCOMPARE(model.data(reply, ThreadListModel::SubjectRole).toString(), + QStringLiteral("Re: A subject")); + + // No tag strip under a child row. The strip is a row-wide band carrying the + // THREAD's tags; one under every reply would stripe the list and repeat the + // same tags down the whole expansion. + QVERIFY(model.data(reply, ThreadListModel::PillTagsRole) + .toStringList().isEmpty()); +} + +void TestThreadListModel::reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem() +{ + // A thread reloaded after a sync must not end up listing its replies twice. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")) }); + + const QVector<MessageNode> nodes{ + makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) + }; + + model.setThreadMessages(QStringLiteral("t1"), nodes); + const QModelIndex root = model.index(0, 0, QModelIndex()); + QCOMPARE(model.rowCount(root), 1); + + model.setThreadMessages(QStringLiteral("t1"), nodes); + QCOMPARE(model.rowCount(root), 1); + + QAbstractItemModelTester tester( + &model, QAbstractItemModelTester::FailureReportingMode::Warning); + Q_UNUSED(tester); +} + +void TestThreadListModel::anUnexpandedMultiMessageThreadOffersAnExpander() +{ + // This is what makes lazy loading work at all. rowCount is 0 until the + // worker has walked the thread, so a view inferring the expander from + // rowCount alone draws none, the user can never expand, and the replies are + // never requested. hasChildren answers from the summary's count instead. + ThreadListModel model; + ThreadSummary many = makeThread(QStringLiteral("t1"), + QStringLiteral("Has replies")); + many.totalCount = 4; + ThreadSummary lone = makeThread(QStringLiteral("t2"), + QStringLiteral("Single message")); + lone.totalCount = 1; + model.appendBatch({ many, lone }); + + const QModelIndex withReplies = model.index(0, 0, QModelIndex()); + const QModelIndex single = model.index(1, 0, QModelIndex()); + + // Guard: neither is expanded, so this really is the unloaded case. + QCOMPARE(model.rowCount(withReplies), 0); + QCOMPARE(model.rowCount(single), 0); + + QVERIFY(model.hasChildren(withReplies)); + QVERIFY(!model.hasChildren(single)); + + // Once loaded the children are the truth, including "there are none": a + // thread whose count included duplicates must stop offering an expander + // that opens onto nothing. + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0) }); + QVERIFY(!model.hasChildren(withReplies)); + + // A message row is always a leaf. + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) }); + QVERIFY(model.hasChildren(withReplies)); + QVERIFY(!model.hasChildren(model.index(0, 0, withReplies))); +} + +void TestThreadListModel::scopeFollowsTheSelectedRowKind() +{ + ThreadListModel model; + ThreadSummary t = makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")); + t.totalCount = 3; + model.appendBatch({ t }); + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) }); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + const QModelIndex child = model.index(0, 0, root); + + // A thread root acts on the whole thread, and reports every message it + // stands for so the status bar can say so. + const ActionScope threadScope = model.scopeFor({ root }); + QCOMPARE(threadScope.threadIds, QStringList{ QStringLiteral("t1") }); + QVERIFY(threadScope.messageIds.isEmpty()); + QCOMPARE(threadScope.messageCount, 3); + QVERIFY(threadScope.wholeThread); + + // A message row acts on that message alone. + const ActionScope messageScope = model.scopeFor({ child }); + QVERIFY(messageScope.threadIds.isEmpty()); + QCOMPARE(messageScope.messageIds, + QStringList{ QStringLiteral("m1@example.org") }); + QCOMPARE(messageScope.messageCount, 1); + QVERIFY(!messageScope.wholeThread); +} + +void TestThreadListModel::scopeCountsEveryMessageOfAnUnexpandedThread() +{ + // totalCount, not the loaded children. A thread that was never expanded + // still has all of its messages, and counting only what happens to be on + // screen would understate what the action is about to do. + ThreadListModel model; + ThreadSummary t = makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")); + t.totalCount = 7; + model.appendBatch({ t }); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + QCOMPARE(model.rowCount(root), 0); // guard: nothing expanded + + const ActionScope scope = model.scopeFor({ root }); + QCOMPARE(scope.messageCount, 7); +} + +void TestThreadListModel::scopeHonoursAMixedSelectionWithoutEscalating() +{ + // Selecting a thread root and an unrelated reply acts on that whole thread + // AND that one message. Nothing is escalated to thread scope or narrowed to + // message scope silently, which is the point of the scope being visible. + ThreadListModel model; + ThreadSummary t1 = makeThread(QStringLiteral("t1"), QStringLiteral("One")); + t1.totalCount = 2; + ThreadSummary t2 = makeThread(QStringLiteral("t2"), QStringLiteral("Two")); + t2.totalCount = 5; + model.appendBatch({ t1, t2 }); + + MessageNode reply = makeNode(QStringLiteral("m1@example.org"), 1); + reply.threadId = QStringLiteral("t2"); + model.setThreadMessages(QStringLiteral("t2"), + { makeNode(QStringLiteral("m0@example.org"), 0), + reply }); + + const QModelIndex firstRoot = model.index(0, 0, QModelIndex()); + const QModelIndex secondRoot = model.index(1, 0, QModelIndex()); + const QModelIndex reply1 = model.index(0, 0, secondRoot); + + const ActionScope scope = model.scopeFor({ firstRoot, reply1 }); + QCOMPARE(scope.threadIds, QStringList{ QStringLiteral("t1") }); + QCOMPARE(scope.messageIds, QStringList{ QStringLiteral("m1@example.org") }); + + // 2 from the whole thread plus 1 for the lone message. + QCOMPARE(scope.messageCount, 3); + QVERIFY(scope.wholeThread); +} + void TestThreadListModel::accountKeysComeFromTheAccountTags() { // Item 49 reads this to decide which mbsync channels a sync needs. Only @@ -116,11 +360,69 @@ void TestThreadListModel::accountKeysAreEmptyForAnUnknownThread() QVERIFY(model.accountKeysForThread(QStringLiteral("nope")).isEmpty()); } +void TestThreadListModel::messageNodeHoldsDisplayFacts() +{ + // A message ROW has to be drawn without opening the message, so the display + // facts live on the node itself. MessageRef, which exists for rendering a + // thread into the pane, carries none of them. + MessageNode node; + node.messageId = QStringLiteral("id@example.org"); + node.from = QStringLiteral("A Sender <sender@example.org>"); + node.subject = QStringLiteral("Re: a subject"); + node.date = QDateTime::fromSecsSinceEpoch(1000); + node.depth = 2; + node.tags = QStringList{ QStringLiteral("unread") }; + + QCOMPARE(node.depth, 2); + QVERIFY(node.isUnread()); + QCOMPARE(node.from, QStringLiteral("A Sender <sender@example.org>")); + QCOMPARE(node.subject, QStringLiteral("Re: a subject")); + + // Depth 0 is the thread's first message, which the ROOT row stands for. + // Defaulting to 0 rather than 1 keeps "is this the root" a plain check. + const MessageNode fresh; + QCOMPARE(fresh.depth, 0); + QVERIFY(!fresh.isUnread()); +} + +void TestThreadListModel::rootRowsSurviveTheTreeConversion() +{ + // The point of this test is NOT the tree. It is that converting the base + // class from QAbstractTableModel changed nothing a thread row does: a table + // answers index() and parent() too, just trivially, and every existing test + // in this file is the real regression net beside it. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")) }); + + // A tree model reports its roots under an INVALID parent. + QCOMPARE(model.rowCount(QModelIndex()), 1); + QCOMPARE(model.columnCount(QModelIndex()), 1); + + const QModelIndex root = + model.index(0, 0, QModelIndex()); + QVERIFY(root.isValid()); + QVERIFY(!model.parent(root).isValid()); + QCOMPARE(model.data(root, ThreadListModel::ThreadIdRole).toString(), + QStringLiteral("t1")); + + // No children until a thread's messages are asked for. An expander drawn + // over a thread whose replies were never loaded would open onto nothing. + QCOMPARE(model.rowCount(root), 0); + + // Qt's own conformance check. It walks index/parent/rowCount for + // consistency and catches the classic tree-model faults, such as a parent() + // that does not round-trip, which a hand-written assertion misses. + QAbstractItemModelTester tester( + &model, QAbstractItemModelTester::FailureReportingMode::Warning); + Q_UNUSED(tester); +} + void TestThreadListModel::startsEmpty() { ThreadListModel model; QCOMPARE(model.rowCount(), 0); - QCOMPARE(model.columnCount(), ThreadListModel::ColumnCount); + QCOMPARE(model.columnCount(), 1); } void TestThreadListModel::appendsBatches() @@ -165,21 +467,21 @@ void TestThreadListModel::reportsSubjectAndAuthors() ThreadListModel model; model.appendBatch({ makeThread(QStringLiteral("t1"), QStringLiteral("hello")) }); - const QModelIndex authors = model.index(0, ThreadListModel::AuthorsColumn); - QCOMPARE(model.data(authors, Qt::DisplayRole).toString(), + // One index, every field, by role. The card draws them all at once, so + // reading them through Qt::DisplayRole as five columns did is no longer + // possible: DisplayRole answers the subject alone. + const QModelIndex card = model.index(0, 0); + QCOMPARE(model.data(card, ThreadListModel::SendersRole).toString(), QStringLiteral("Alice")); + QVERIFY(model.data(card, ThreadListModel::DateRole).toDateTime().isValid()); + QCOMPARE(model.data(card, ThreadListModel::SubjectRole).toString(), + QStringLiteral("hello")); - const QModelIndex date = model.index(0, ThreadListModel::DateColumn); - QVERIFY(!model.data(date, Qt::DisplayRole).toString().isEmpty()); - - // Tags are no longer a column; they reach the strip under the message - // pane through a role instead. - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); - QCOMPARE(model.data(subject, ThreadListModel::TagsRole).toStringList(), + QCOMPARE(model.data(card, ThreadListModel::TagsRole).toStringList(), QStringList({ QStringLiteral("inbox"), QStringLiteral("unread") })); } -void TestThreadListModel::subjectShowsMessageCountOnlyForRealThreads() +void TestThreadListModel::theReplyCountExcludesTheRootMessage() { ThreadListModel model; @@ -189,12 +491,18 @@ void TestThreadListModel::subjectShowsMessageCountOnlyForRealThreads() multi.totalCount = 4; model.appendBatch({ single, multi }); - QCOMPARE(model.data(model.index(0, ThreadListModel::SubjectColumn), - Qt::DisplayRole).toString(), - QStringLiteral("alone")); - QCOMPARE(model.data(model.index(1, ThreadListModel::SubjectColumn), - Qt::DisplayRole).toString(), - QStringLiteral("group (4)")); + // 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. + QCOMPARE(model.data(model.index(0, 0), + ThreadListModel::ReplyCountRole).toInt(), 0); + QCOMPARE(model.data(model.index(1, 0), + ThreadListModel::ReplyCountRole).toInt(), 3); + + // And the subject is bare, with no count spliced into it. + QCOMPARE(model.data(model.index(1, 0), + ThreadListModel::SubjectRole).toString(), + QStringLiteral("group")); } void TestThreadListModel::unreadThreadsRenderBold() @@ -205,11 +513,11 @@ void TestThreadListModel::unreadThreadsRenderBold() model.appendBatch({ read, makeThread(QStringLiteral("t2"), QStringLiteral("unread")) }); const QVariant readFont = - model.data(model.index(0, ThreadListModel::SubjectColumn), Qt::FontRole); + model.data(model.index(0, 0), Qt::FontRole); QVERIFY(!readFont.isValid()); const QVariant unreadFont = - model.data(model.index(1, ThreadListModel::SubjectColumn), Qt::FontRole); + model.data(model.index(1, 0), Qt::FontRole); QVERIFY(unreadFont.isValid()); QVERIFY(unreadFont.value<QFont>().bold()); } @@ -232,10 +540,10 @@ void TestThreadListModel::readThreadsAreDimmedAndUnreadAreNot() { read, makeThread(QStringLiteral("t2"), QStringLiteral("unread")) }); const QVariant readFg = - model.data(model.index(0, ThreadListModel::SubjectColumn), + model.data(model.index(0, 0), Qt::ForegroundRole); const QVariant unreadFg = - model.data(model.index(1, ThreadListModel::SubjectColumn), + model.data(model.index(1, 0), Qt::ForegroundRole); QVERIFY2(readFg.isValid(), "a read thread carries no dimming"); @@ -257,16 +565,17 @@ void TestThreadListModel::flaggedThreadsShowAStar() QStringLiteral("flagged") }; model.appendBatch({ plain, starred }); - const QString none = - model.data(model.index(0, ThreadListModel::FlagColumn), - Qt::DisplayRole).toString(); - const QString star = - model.data(model.index(1, ThreadListModel::FlagColumn), - Qt::DisplayRole).toString(); - - QVERIFY2(none.isEmpty(), "an unflagged thread shows something in the column"); - QVERIFY2(!star.isEmpty(), "a flagged thread shows nothing"); - QCOMPARE(star, ThreadListModel::flagGlyph()); + QVERIFY2(!model.data(model.index(0, 0), + ThreadListModel::IsFlaggedRole).toBool(), + "an unflagged thread reports itself flagged"); + QVERIFY2(model.data(model.index(1, 0), + ThreadListModel::IsFlaggedRole).toBool(), + "a flagged thread does not report itself flagged"); + + // The glyph the delegate draws from that flag must be something a font can + // render: an unrenderable codepoint shows as tofu, which reads as + // breakage rather than as a mark. + QVERIFY(!ThreadListModel::flagGlyph().isEmpty()); } void TestThreadListModel::pillTagsExcludeWhatTheRowAlreadyShows() @@ -290,7 +599,7 @@ void TestThreadListModel::pillTagsExcludeWhatTheRowAlreadyShows() model.appendBatch({ thread }); const QStringList pills = - model.data(model.index(0, ThreadListModel::SubjectColumn), + model.data(model.index(0, 0), ThreadListModel::PillTagsRole).toStringList(); QVERIFY2(pills.contains(QStringLiteral("SBo")), qPrintable(pills.join(','))); @@ -320,29 +629,6 @@ void TestThreadListModel::pillTagsExcludeWhatTheRowAlreadyShows() QCOMPARE(pills, sorted); } -void TestThreadListModel::theStarColumnIsNarrowAndCarriesNoText() -{ - // A marker column, like the paperclip beside it: centred, and never - // carrying the subject or anything else that would want width. - ThreadListModel model; - ThreadSummary starred = makeThread(QStringLiteral("t1"), - QStringLiteral("starred")); - starred.tags = QStringList{ QStringLiteral("flagged") }; - model.appendBatch({ starred }); - - const QModelIndex index = model.index(0, ThreadListModel::FlagColumn); - QCOMPARE(model.data(index, Qt::TextAlignmentRole).toInt(), - int(Qt::AlignCenter)); - - // The glyph is one character, whether it is the star or its fallback: a - // column sized for a marker cannot hold a word. - QCOMPARE(ThreadListModel::flagGlyph().size(), 1); - - // And it says what it means, for anyone who cannot tell the glyph apart - // from the paperclip beside it. - QVERIFY(!model.data(index, Qt::ToolTipRole).toString().isEmpty()); -} - void TestThreadListModel::theUnreadCueDoesNotDependOnFontWeight() { // The property that matters, stated directly: strip every font from the @@ -355,17 +641,14 @@ void TestThreadListModel::theUnreadCueDoesNotDependOnFontWeight() model.appendBatch( { read, makeThread(QStringLiteral("t2"), QStringLiteral("unread")) }); - for (int column = 0; column < ThreadListModel::ColumnCount; ++column) { - const QVariant readFg = - model.data(model.index(0, column), Qt::ForegroundRole); - const QVariant unreadFg = - model.data(model.index(1, column), Qt::ForegroundRole); + const QVariant readFg = + model.data(model.index(0, 0), Qt::ForegroundRole); + const QVariant unreadFg = + model.data(model.index(1, 0), Qt::ForegroundRole); - QVERIFY2(readFg != unreadFg, - qPrintable(QStringLiteral("column %1 renders read and unread " - "identically once the font is " - "ignored").arg(column))); - } + QVERIFY2(readFg != unreadFg, + "read and unread cards render identically once the font is " + "ignored"); } void TestThreadListModel::aDoomedThreadKeepsItsContrastEvenWhenRead() @@ -382,32 +665,11 @@ void TestThreadListModel::aDoomedThreadKeepsItsContrastEvenWhenRead() model.applyTagChange(QStringLiteral("t1"), { QStringLiteral("deleted") }, {}); - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); + const QModelIndex subject = model.index(0, 0); QCOMPARE(model.data(subject, Qt::ForegroundRole).value<QBrush>().color(), QColor(Qt::white)); } -void TestThreadListModel::tagsAreTheFirstColumnAndSubjectTheLast() -{ - // Subject stretches to fill the view, so whatever sits after it is pushed - // off-screen. Tags used to be there, which is why acting on a thread - // looked like it did nothing: the only column that changed was invisible. - QCOMPARE(ThreadListModel::SubjectColumn, ThreadListModel::ColumnCount - 1); - - ThreadListModel model; - model.appendBatch({ makeThread(QStringLiteral("t1"), QStringLiteral("hello")) }); - QCOMPARE(model.headerData(ThreadListModel::SubjectColumn, Qt::Horizontal, - Qt::DisplayRole).toString(), - QStringLiteral("Subject")); - - // No tags column at all: spelling out a dozen tags per row consumed most - // of the list's width and was unreadable. - for (int column = 0; column < ThreadListModel::ColumnCount; ++column) { - QVERIFY(model.headerData(column, Qt::Horizontal, Qt::DisplayRole) - .toString() != QStringLiteral("Tags")); - } -} - void TestThreadListModel::accountTagBecomesAChipLabel() { // The account tag is a different taxonomy from a functional one: which @@ -419,7 +681,7 @@ void TestThreadListModel::accountTagBecomesAChipLabel() QStringLiteral("account-webmail-personal") }; model.appendBatch({ thread }); - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); + const QModelIndex subject = model.index(0, 0); QCOMPARE(model.data(subject, ThreadListModel::AccountLabelRole).toString(), QStringLiteral("webmail-personal")); QVERIFY(model.data(subject, ThreadListModel::AccountColourRole) @@ -430,7 +692,7 @@ void TestThreadListModel::accountTagBecomesAChipLabel() ThreadSummary untagged = makeThread(QStringLiteral("t2"), QStringLiteral("hi")); untagged.tags = QStringList{ QStringLiteral("inbox") }; plain.appendBatch({ untagged }); - QVERIFY(plain.data(plain.index(0, ThreadListModel::SubjectColumn), + QVERIFY(plain.data(plain.index(0, 0), ThreadListModel::AccountLabelRole).toString().isEmpty()); } @@ -446,7 +708,7 @@ void TestThreadListModel::unreadStylingSurvivesAnAccountChip() QStringLiteral("account-webmail-personal") }; model.appendBatch({ thread }); - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); + const QModelIndex subject = model.index(0, 0); QVERIFY(!model.data(subject, ThreadListModel::AccountLabelRole) .toString().isEmpty()); @@ -469,7 +731,7 @@ void TestThreadListModel::accountChipUsesTheConfiguredColour() thread.tags = QStringList{ QStringLiteral("account-webmail-personal") }; model.appendBatch({ thread }); - QCOMPARE(model.data(model.index(0, ThreadListModel::SubjectColumn), + QCOMPARE(model.data(model.index(0, 0), ThreadListModel::AccountColourRole).value<QColor>(), QColor(QStringLiteral("#cc0000"))); } @@ -481,7 +743,7 @@ void TestThreadListModel::deletedThreadsAreRedAndStruckThrough() thread.tags = QStringList{ QStringLiteral("inbox") }; model.appendBatch({ thread }); - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); + const QModelIndex subject = model.index(0, 0); QVERIFY(!model.data(subject, Qt::BackgroundRole).isValid()); model.applyTagChange(QStringLiteral("t1"), { QStringLiteral("deleted") }, {}); @@ -506,7 +768,7 @@ void TestThreadListModel::spamThreadsAreOrangeAndStruckThrough() model.applyTagChange(QStringLiteral("t1"), { QStringLiteral("spam") }, {}); - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); + const QModelIndex subject = model.index(0, 0); QCOMPARE(model.data(subject, Qt::BackgroundRole).value<QBrush>().color(), ThreadListModel::spamColour()); QVERIFY(model.data(subject, Qt::FontRole).value<QFont>().strikeOut()); @@ -515,10 +777,12 @@ void TestThreadListModel::spamThreadsAreOrangeAndStruckThrough() QVERIFY(ThreadListModel::spamColour() != ThreadListModel::deletedColour()); } -void TestThreadListModel::doomedStylingCoversEveryColumn() +void TestThreadListModel::doomedStylingCoversTheWholeCard() { - // A cue on one column would vanish the moment that column scrolled out of - // view, which is the bug this whole change exists to fix. + // The cue is on the card itself. It used to be asserted per column, + // because a cue on one column vanished the moment that column scrolled out + // of view; one column cannot scroll away, but the roles still have to be + // answered or a deleted card looks untouched. ThreadListModel model; ThreadSummary thread = makeThread(QStringLiteral("t1"), QStringLiteral("doomed")); thread.tags = QStringList{ QStringLiteral("inbox") }; @@ -526,13 +790,11 @@ void TestThreadListModel::doomedStylingCoversEveryColumn() model.applyTagChange(QStringLiteral("t1"), { QStringLiteral("deleted") }, {}); - for (int column = 0; column < ThreadListModel::ColumnCount; ++column) { - const QModelIndex index = model.index(0, column); - QVERIFY2(model.data(index, Qt::BackgroundRole).isValid(), - qPrintable(QStringLiteral("column %1 has no background").arg(column))); - QVERIFY2(model.data(index, Qt::FontRole).value<QFont>().strikeOut(), - qPrintable(QStringLiteral("column %1 is not struck through").arg(column))); - } + const QModelIndex index = model.index(0, 0); + QVERIFY2(model.data(index, Qt::BackgroundRole).isValid(), + "a deleted card has no background"); + QVERIFY2(model.data(index, Qt::FontRole).value<QFont>().strikeOut(), + "a deleted card is not struck through"); } void TestThreadListModel::ordinaryThreadsCarryNoRowColour() @@ -546,7 +808,7 @@ void TestThreadListModel::ordinaryThreadsCarryNoRowColour() model.applyTagChange(QStringLiteral("t1"), { QStringLiteral("deleted") }, {}); model.applyTagChange(QStringLiteral("t1"), {}, { QStringLiteral("deleted") }); - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); + const QModelIndex subject = model.index(0, 0); QVERIFY(!model.data(subject, Qt::BackgroundRole).isValid()); const QVariant font = model.data(subject, Qt::FontRole); QVERIFY(!font.isValid() || !font.value<QFont>().strikeOut()); @@ -569,7 +831,7 @@ void TestThreadListModel::threadIdIsReachableFromAnIndex() model.appendBatch({ makeThread(QStringLiteral("t1"), QStringLiteral("one")), makeThread(QStringLiteral("t2"), QStringLiteral("two")) }); - const QModelIndex index = model.index(1, ThreadListModel::SubjectColumn); + const QModelIndex index = model.index(1, 0); QCOMPARE(model.data(index, ThreadListModel::ThreadIdRole).toString(), QStringLiteral("t2")); } @@ -587,7 +849,7 @@ void TestThreadListModel::invalidIndexesReturnNothing() // the reset in clear() before data() ever sees it. data() still checks its // own bounds, but that guard is unreachable defence, not something these // assertions can falsify. - QVERIFY(!model.index(0, ThreadListModel::ColumnCount).isValid()); + QVERIFY(!model.index(0, 1).isValid()); QVERIFY(!model.index(5, 0).isValid()); QVERIFY(!model.index(-1, 0).isValid()); @@ -651,9 +913,9 @@ void TestThreadListModel::tagChangeSignalsExactlyTheChangedRow() const QModelIndex bottomRight = changed.first().at(1).value<QModelIndex>(); QCOMPARE(topLeft.row(), 1); QCOMPARE(bottomRight.row(), 1); + // One column, so the range is a single index: the card repaints whole. QCOMPARE(topLeft.column(), 0); - // The whole row repaints: unread state changes the font of every column. - QCOMPARE(bottomRight.column(), ThreadListModel::ColumnCount - 1); + QCOMPARE(bottomRight.column(), 0); } void TestThreadListModel::tagChangeForUnknownThreadIsIgnored() @@ -704,12 +966,11 @@ void TestThreadListModel::modelPassesQtTester() model.clear(); } -void TestThreadListModel::attachmentColumnIsFirstAndMarksOnlyTaggedThreads() +void TestThreadListModel::attachmentIsMarkedOnlyOnTaggedThreads() { - // Leftmost, and narrow: the point is to see an attachment without opening - // the thread, which only works if the column is never scrolled away. - QCOMPARE(ThreadListModel::AttachmentColumn, 0); - + // The mark is drawn on the card's second line by CardDelegate. What the + // model owes it is the flag and the glyph, which is what this asserts: + // the column that used to carry it is gone. ThreadSummary plain = makeThread(QStringLiteral("t1"), QStringLiteral("no attachment")); ThreadSummary withFile = makeThread(QStringLiteral("t2"), @@ -722,13 +983,12 @@ void TestThreadListModel::attachmentColumnIsFirstAndMarksOnlyTaggedThreads() model.appendBatch({ plain, withFile }); const QModelIndex plainCell = - model.index(0, ThreadListModel::AttachmentColumn); + model.index(0, 0); const QModelIndex fileCell = - model.index(1, ThreadListModel::AttachmentColumn); + model.index(1, 0); - QVERIFY(model.data(plainCell, Qt::DisplayRole).toString().isEmpty()); - QCOMPARE(model.data(fileCell, Qt::DisplayRole).toString(), - ThreadListModel::attachmentGlyph()); + QVERIFY(!model.data(plainCell, ThreadListModel::HasAttachmentRole).toBool()); + QVERIFY(model.data(fileCell, ThreadListModel::HasAttachmentRole).toBool()); // The glyph must be something a font can draw. An unrenderable codepoint // shows as a tofu box, which reads as breakage rather than as a marker. @@ -738,11 +998,178 @@ void TestThreadListModel::attachmentColumnIsFirstAndMarksOnlyTaggedThreads() // have an attachment on hover. QVERIFY(model.data(plainCell, Qt::ToolTipRole).toString().isEmpty()); QVERIFY(!model.data(fileCell, Qt::ToolTipRole).toString().isEmpty()); +} + +void TestThreadListModel::modelHasOneColumn() +{ + ThreadListModel model; + ThreadSummary thread; + thread.threadId = QStringLiteral("T1"); + thread.subject = QStringLiteral("Build fails"); + thread.authors = QStringLiteral("alice@example.org"); + thread.date = QDateTime::currentDateTime(); + thread.totalCount = 1; + model.appendBatch({ thread }); + + QCOMPARE(model.columnCount(), 1); + + // Every field the five columns used to answer is still reachable, by role + // rather than by column, because the card draws them all. + const QModelIndex index = model.index(0, 0); + QCOMPARE(index.data(ThreadListModel::SubjectRole).toString(), + QStringLiteral("Build fails")); + QCOMPARE(index.data(ThreadListModel::SendersRole).toString(), + 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); +} + +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; + ThreadSummary thread; + thread.threadId = QStringLiteral("T1"); + thread.subject = QStringLiteral("Build fails"); + thread.totalCount = 2; + thread.tags = { QStringLiteral("inbox"), QStringLiteral("work") }; + model.appendBatch({ thread }); + + MessageNode reply; + reply.messageId = QStringLiteral("M2"); + reply.threadId = QStringLiteral("T1"); + reply.from = QStringLiteral("bob@example.org"); + reply.depth = 1; + // Two the thread already has, one it does not. + reply.tags = { QStringLiteral("inbox"), QStringLiteral("work"), + QStringLiteral("todo") }; + + // 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)); + const QModelIndex replyIndex = model.index(0, 0, threadIndex); + QVERIFY(replyIndex.isValid()); + + const QStringList own = + replyIndex.data(ThreadListModel::MessageOwnTagsRole).toStringList(); + QCOMPARE(own, QStringList{ QStringLiteral("todo") }); + + // The colours must line up with the names one for one, or the delegate + // walks the two lists together and paints a chip in another tag's colour. + const QVariantList colours = + replyIndex.data(ThreadListModel::MessageOwnColoursRole).toList(); + QCOMPARE(colours.size(), own.size()); + QVERIFY(colours.first().value<QColor>().isValid()); +} + +void TestThreadListModel::replySharingEveryThreadTagShowsNone() +{ + ThreadListModel model; + ThreadSummary thread; + thread.threadId = QStringLiteral("T1"); + thread.totalCount = 2; + thread.tags = { QStringLiteral("inbox"), QStringLiteral("work") }; + model.appendBatch({ thread }); - // The header carries no text: a label would set a minimum width far wider - // than the icon and defeat the narrow column. - QVERIFY(model.headerData(ThreadListModel::AttachmentColumn, Qt::Horizontal, - Qt::DisplayRole).toString().isEmpty()); + MessageNode reply; + reply.messageId = QStringLiteral("M2"); + reply.threadId = QStringLiteral("T1"); + reply.depth = 1; + reply.tags = { QStringLiteral("inbox"), QStringLiteral("work") }; + + 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()); + QVERIFY(replyIndex.data(ThreadListModel::MessageOwnTagsRole) + .toStringList() + .isEmpty()); + + // A thread row has no thread to differ from, so it never answers these: + // its own chips come from PillTagsRole. + QVERIFY(model.index(0, 0) + .data(ThreadListModel::MessageOwnTagsRole) + .toStringList() + .isEmpty()); } QTEST_MAIN(TestThreadListModel) |