diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-09 12:31:47 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-09 12:31:47 +0200 |
| commit | d990f7c6775b4b921c6d6d8d539c7018279346be (patch) | |
| tree | f42f876fcb1dbf49ba78d20025eea10f63fff42d /docs/superpowers/plans | |
| parent | 14218dc6e6ba8d8648b9b574457214b298bcf75f (diff) | |
| download | qtmaildir-d990f7c6775b4b921c6d6d8d539c7018279346be.tar.gz qtmaildir-d990f7c6775b4b921c6d6d8d539c7018279346be.zip | |
docs: plan the card list implementation
Eleven tasks against the card-list spec, TDD throughout, each with its own
mutation check.
Three decisions the plan makes that the spec did not:
- Task 0 rebases the branch first. item-20-message-rows is 15 commits behind
master and conflicts in eight files, one of which carries the item 55
splitter fix in the same file this plan rewrites heavily. Resolving that
while also building a feature would mean debugging a merge and a feature at
once, so it is its own task with its own verification step.
- CardLayout is a separate file from CardDelegate, holding all the geometry
and touching no painter or widget. That is a direct response to the
rendering-probe lessons in CLAUDE.md: a delegate test can be defeated by a
blank render, a pure function cannot.
- Tasks 2, 5 and 6 knowingly leave the build red between commits, since
removing a column across three files cannot be atomic without one enormous
commit. The plan says so at each one and warns against merging mid-plan.
Also recorded: the account chip is specified but has no task, because its
placement on a card was never specified and inventing one would be a guess.
Diffstat (limited to 'docs/superpowers/plans')
| -rw-r--r-- | docs/superpowers/plans/2026-08-09-card-list.md | 1958 |
1 files changed, 1958 insertions, 0 deletions
diff --git a/docs/superpowers/plans/2026-08-09-card-list.md b/docs/superpowers/plans/2026-08-09-card-list.md new file mode 100644 index 0000000..582d8b9 --- /dev/null +++ b/docs/superpowers/plans/2026-08-09-card-list.md @@ -0,0 +1,1958 @@ +# Card List Implementation Plan + +> **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:** Replace the thread pane's five-column grid with a single-column list of three-line cards, so threads and replies read as a conversation rather than as a table of records. + +**Architecture:** The model collapses from five columns to one and keeps every existing role, gaining one pair for a reply's own tags. A new `CardDelegate` paints the entire card, replacing `SubjectDelegate` and taking over the tag chips that `ThreadListView::paintEvent` draws today; that `paintEvent` and its band arithmetic are deleted, leaving the view owning only the expander hit-test. Navigation moves off row-number arithmetic onto `indexBelow`/`indexAbove`. + +**Tech Stack:** C++17, Qt 6.11 (Widgets), CMake + Ninja, QtTest. No new dependencies. + +**Spec:** `docs/superpowers/specs/2026-08-09-card-list-design.md` + +--- + +## Before you start + +**Read these first.** This plan assumes you have read the spec above and the +"Architecture" and "Rendering probes lie" sections of `CLAUDE.md`. Several steps +below will look like busywork if you have not. + +**Branch.** This work continues `item-20-message-rows`, which carries the tree +model, the worker's reply walk, and `MessageNode`. All of that is kept. The +branch is 15 commits behind `master`, so Task 0 rebases it first. + +**Build and test:** + +```bash +cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug +cmake --build build +ctest --test-dir build --output-on-failure +``` + +Single test binary, for a tighter loop: `./build/tests/test_threadlistmodel`. + +**Two rules from `CLAUDE.md` that this plan leans on constantly:** + +1. **Nothing may be keyed on a row NUMBER.** A tree numbers rows per parent, so + `index.row()` is a position among siblings, not a position in the list. +2. **Never count lit pixels to verify rendering.** Antialiasing makes bold and + regular light a similar number either way. Assert on computed geometry + instead, which is why Task 4 exists before any painting is written. + +**Commit style.** GPG-signed (`git commit -S`), on the branch, no PR flow. + +--- + +## File Structure + +| File | Change | Responsibility after this plan | +|---|---|---| +| `src/types.h` | none | `ThreadSummary`, `MessageNode` unchanged. `MessageNode::tags` already exists and is what the set difference reads. | +| `src/threadlistmodel.h` | modify | One column. Adds `MessageOwnTagsRole`, `MessageOwnColoursRole`. Drops the five `Column` enumerators. | +| `src/threadlistmodel.cpp` | modify | `data()` answers one column; `headerData` gone. Computes the tag set difference. | +| `src/cardlayout.h` | **create** | Pure geometry: given a row's properties and a width, returns every rect on the card. No painting, no Qt widgets. This is what the tests assert on. | +| `src/cardlayout.cpp` | **create** | Implementation of the above. | +| `src/carddelegate.h` | **create** | `CardDelegate`, painting a card from a `CardLayout`. Inherits `RowStyleDelegate`. | +| `src/carddelegate.cpp` | **create** | Implementation. | +| `src/tagchip.h` | modify | Keeps the `TagChip` namespace and `RowStyleDelegate`. `SubjectDelegate` deleted. | +| `src/tagchip.cpp` | modify | Same. | +| `src/threadlistview.h` | modify | Loses `paintEvent`. Keeps `mousePressEvent`. | +| `src/threadlistview.cpp` | modify | Same. ~260 lines down to ~60. | +| `src/mainwindow.cpp` | modify | View setup for one column; sort dropdown; navigation rewrite. | +| `src/notmuchworker.h/.cpp` | modify | `runQuery` takes a sort order. | +| `tests/test_cardlayout.cpp` | **create** | Geometry assertions. The bulk of the new tests. | + +**Why `CardLayout` is a separate file from `CardDelegate`.** A delegate needs a +`QPainter` and a live view to do anything, which is exactly 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" is more +likely broken than the code. Splitting the arithmetic out gives every geometric +claim a test that needs no painting at all. + +--- + +## Task 0: Rebase the branch onto master + +**Files:** no source changes of your own; you are resolving other people's. + +- [ ] **Step 1: Confirm where you are** + +```bash +git checkout item-20-message-rows +git log --oneline -1 +``` + +Expected: `029a50e docs: record message rows, and the user's verdict on them` + +- [ ] **Step 2: See what is coming** + +```bash +git log --oneline item-20-message-rows..master +``` + +Expected: 15 commits, including `751ca62 fix(ui): stop a restored splitter +position collapsing the message pane` and `99709c6 fix(config): report an +out-of-range message_zoom`. + +- [ ] **Step 3: Rebase** + +```bash +git rebase master +``` + +Eight files conflict. Resolve as follows: + +- `CHANGELOG.md`, `CLAUDE.md`, `docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md`: + **take master's version entirely** (`git checkout --theirs <file>` during the + rebase), then re-apply nothing. Master's copies already describe the branch, + because `69281b2` carried those entries across deliberately. +- `docs/superpowers/plans/2026-08-08-item-20-message-rows.md`: keep the + branch's version. +- `src/mainwindow.cpp`, `src/mainwindow.h`: master added `setMinimumWidth(300)` + and `setCollapsible(1, false)` on the splitter (item 55) and an + out-of-range zoom warning. **Both must survive.** The branch's changes here + are the view setup; they do not overlap the splitter lines. +- `src/threadlistmodel.cpp`, `tests/test_mainwindow.cpp`: take both sides; + master's changes are the icon work and the splitter test. + +- [ ] **Step 4: Verify the rebase kept master's fixes** + +```bash +grep -n "setMinimumWidth(300)\|setCollapsible(1, false)" src/mainwindow.cpp +``` + +Expected: both present. If either is missing you dropped an item-55 hunk; +redo the conflict resolution rather than patching it back by hand. + +- [ ] **Step 5: Build and run the whole suite** + +```bash +cmake --build build && ctest --test-dir build --output-on-failure +``` + +Expected: all tests pass. If `test_mainwindow` fails here, fix it before any +new work: you are now debugging a merge, and every later task would be +debugging a merge plus a feature. + +- [ ] **Step 6: Commit** + +The rebase commits itself. Verify signatures survived: + +```bash +git log --format='%h %G? %s' -5 +``` + +Expected: `G` in every second column. If any say `N`, re-sign: +`git rebase --exec 'git commit --amend --no-edit -S' master` + +--- + +## Task 1: The model's own tags for a reply + +The set difference from the spec: a reply card shows only the tags its message +has and its thread does not. Model-side only, no worker change, because +`MessageNode::tags` already carries the message's tags. + +**Files:** +- Modify: `src/threadlistmodel.h` (the `Role` enum) +- Modify: `src/threadlistmodel.cpp` (the `isMessageRow` branch of `data()`) +- Test: `tests/test_threadlistmodel.cpp` + +- [ ] **Step 1: Write the failing test** + +Add to `tests/test_threadlistmodel.cpp`. Declare both in the `private slots:` +block at the top of the class, then add the bodies: + +```cpp +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") }; + model.setReplies(QStringLiteral("T1"), { 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") }); +} + +void TestThreadListModel::replySharingEveryThreadTagShowsNone() +{ + ThreadListModel model; + ThreadSummary thread; + thread.threadId = QStringLiteral("T1"); + thread.totalCount = 2; + thread.tags = { QStringLiteral("inbox"), QStringLiteral("work") }; + model.appendBatch({ thread }); + + MessageNode reply; + reply.messageId = QStringLiteral("M2"); + reply.threadId = QStringLiteral("T1"); + reply.depth = 1; + reply.tags = { QStringLiteral("inbox"), QStringLiteral("work") }; + model.setReplies(QStringLiteral("T1"), { reply }); + + const QModelIndex replyIndex = model.index(0, 0, model.index(0, 0)); + QVERIFY(replyIndex.data(ThreadListModel::MessageOwnTagsRole) + .toStringList() + .isEmpty()); +} +``` + +**Check the method name first.** This plan calls the reply-filling method +`setReplies`; the branch may name it differently. Run +`grep -n "void set.*Replies\|void.*[Rr]eplies" src/threadlistmodel.h` and use +whatever is there rather than renaming it. + +- [ ] **Step 2: Run it and watch it fail** + +```bash +cmake --build build && ./build/tests/test_threadlistmodel +``` + +Expected: compile error, `MessageOwnTagsRole` is not a member. That is the +correct first failure. + +- [ ] **Step 3: Add the roles** + +In `src/threadlistmodel.h`, at the END of the `Role` enum, after +`HasRepliesRole`: + +```cpp + /// 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, +``` + +- [ ] **Step 4: Implement the difference** + +In `src/threadlistmodel.cpp`, in the message-row branch of `data()`, beside the +existing `case PillTagsRole:`: + +```cpp + case MessageOwnTagsRole: { + // Set difference against the parent thread, not against a global + // list: "own" means "not already said by the card above this one". + const ThreadSummary *thread = threadFor(node.threadId); + const QStringList threadTags = thread ? thread->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; + for (const QString &tag : own) + colours.append(pillColourFor(tag)); + return colours; + } +``` + +`threadFor` and `pillColourFor` are helpers the branch may or may not have under +those names. Check with `grep -n "threadFor\|pillColourFor\|ThreadSummary \*" +src/threadlistmodel.h src/threadlistmodel.cpp`. If the colour lookup does not +exist as a helper, copy whatever `case PillColoursRole:` does today. + +Also add, in the THREAD-row branch, so a thread card never draws these: + +```cpp + case MessageOwnTagsRole: + return QStringList(); + case MessageOwnColoursRole: + return QVariantList(); +``` + +- [ ] **Step 5: Run the tests** + +```bash +cmake --build build && ./build/tests/test_threadlistmodel +``` + +Expected: PASS, both new tests. + +- [ ] **Step 6: Mutation-check it** + +Temporarily change `if (!threadTags.contains(tag))` to `if (true)`. Rerun. +Expected: `replyShowsOnlyItsOwnTags` FAILS (it would report all three tags) and +`replySharingEveryThreadTagShowsNone` FAILS. **Revert the mutation.** If either +still passed, the test is not testing what it claims. + +- [ ] **Step 7: Commit** + +```bash +git add src/threadlistmodel.h src/threadlistmodel.cpp tests/test_threadlistmodel.cpp +git commit -S -m "feat(model): expose the tags a reply has and its thread does not + +A reply card shows only these. The alternative, a reply's full tag set, was +rejected on measurement rather than taste: in the user's database 7 of 48691 +messages carry unread and 75 carry flagged, both already drawn another way, and +every other tag is applied per thread and identical on all its messages. Full +sets would repeat the thread's chips down the whole expansion, which is the +striping the row-wide strip was built to avoid." +``` + +--- + +## Task 2: Collapse the model to one column + +**Files:** +- Modify: `src/threadlistmodel.h`, `src/threadlistmodel.cpp` +- Test: `tests/test_threadlistmodel.cpp` + +This task deliberately breaks the build in several places. That is expected: the +view and delegate still reference the old columns and are fixed in Tasks 5-6. +Work through the compiler errors. + +- [ ] **Step 1: Write the failing test** + +```cpp +void TestThreadListModel::modelHasOneColumn() +{ + ThreadListModel model; + ThreadSummary thread; + thread.threadId = QStringLiteral("T1"); + thread.subject = QStringLiteral("Build fails"); + thread.authors = QStringLiteral("alice@example.org"); + 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).isValid()); +} +``` + +- [ ] **Step 2: Run it and watch it fail** + +```bash +cmake --build build && ./build/tests/test_threadlistmodel +``` + +Expected: compile error on `SubjectRole`. + +- [ ] **Step 3: Replace the Column enum with per-field roles** + +In `src/threadlistmodel.h`, DELETE the whole `Column` enum: + +```cpp + // DELETE THIS ENTIRE BLOCK + enum Column { + AttachmentColumn = 0, + FlagColumn, + DateColumn, + AuthorsColumn, + SubjectColumn, + ColumnCount, + }; +``` + +Add to the END of the `Role` enum: + +```cpp + /// 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. +``` + +- [ ] **Step 4: Make columnCount answer 1** + +In `src/threadlistmodel.cpp`: + +```cpp +int ThreadListModel::columnCount(const QModelIndex &parent) const +{ + Q_UNUSED(parent); + // One column: the card is drawn whole by CardDelegate. The five-column + // grid is what item 53 removed. + return 1; +} +``` + +- [ ] **Step 5: Rewrite data() to answer by role** + +In the THREAD-row branch of `data()`, replace the `case Qt::DisplayRole:` switch +over columns with: + +```cpp + case Qt::DisplayRole: + case SubjectRole: + return summary.subject; + case SendersRole: + return summary.authors; + case DateRole: + return summary.date; + case HasAttachmentRole: + return summary.hasAttachment(); + case IsFlaggedRole: + return summary.isFlagged(); + case ReplyCountRole: + // totalCount includes the root message, which is the card itself. + return qMax(0, summary.totalCount - 1); +``` + +In the MESSAGE-row branch, the same shape: + +```cpp + case Qt::DisplayRole: + case SubjectRole: + return node.subject; + case SendersRole: + 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; +``` + +Delete `headerData` entirely, and its declaration in the header. + +- [ ] **Step 6: Run the model tests** + +```bash +cmake --build build 2>&1 | head -40 +``` + +The library will not link yet: `mainwindow.cpp`, `threadlistview.cpp` and +`tagchip.cpp` still name the deleted columns. **That is expected.** To run this +task's test now, build only the model test target if your generator allows it; +otherwise accept a red build and verify at the end of Task 6. + +- [ ] **Step 7: Commit (a knowingly broken build)** + +```bash +git add src/threadlistmodel.h src/threadlistmodel.cpp tests/test_threadlistmodel.cpp +git commit -S -m "refactor(model): collapse the thread list to a single column + +Five columns answered through Qt::DisplayRole; one column cannot, and a card +needs every field at once, so each gets its own role. Qt::DisplayRole keeps +answering the subject, which is what keyboard search and accessibility read. + +The build is red at this commit: the view and the delegates still name the +deleted Column enumerators and are rewritten in the commits that follow." +``` + +--- + +## Task 3: Sort order through the worker + +Independent of the painting work. Doing it here keeps it out of the way of the +delegate tasks. + +**Files:** +- Modify: `src/notmuchworker.h`, `src/notmuchworker.cpp:135` +- Test: `tests/test_notmuchworker.cpp` + +- [ ] **Step 1: Write the failing test** + +```cpp +void TestNotmuchWorker::oldestFirstReversesTheOrder() +{ + // The fixture database has at least two threads with different dates. + NotmuchWorker worker; + worker.openDatabase(m_configPath); + + QVector<ThreadSummary> newest; + QVector<ThreadSummary> oldest; + + connect(&worker, &NotmuchWorker::threadsReady, this, + [&](const QVector<ThreadSummary> &batch, quint64, bool) { + newest.append(batch); + }); + worker.runQuery(QStringLiteral("*"), 1, NotmuchWorker::NewestFirst); + QTRY_VERIFY(newest.size() >= 2); + + disconnect(&worker, nullptr, this, nullptr); + connect(&worker, &NotmuchWorker::threadsReady, this, + [&](const QVector<ThreadSummary> &batch, quint64, bool) { + oldest.append(batch); + }); + worker.runQuery(QStringLiteral("*"), 2, NotmuchWorker::OldestFirst); + QTRY_VERIFY(oldest.size() >= 2); + + QCOMPARE(oldest.size(), newest.size()); + QCOMPARE(oldest.first().threadId, newest.last().threadId); + QCOMPARE(oldest.last().threadId, newest.first().threadId); +} +``` + +**Match the existing signatures.** `runQuery` and `threadsReady` already exist +with a particular shape on the branch. Run +`grep -n "runQuery\|threadsReady" src/notmuchworker.h` and adapt the test to +what is there, adding only the sort parameter. + +- [ ] **Step 2: Run it and watch it fail** + +```bash +cmake --build build && ./build/tests/test_notmuchworker +``` + +Expected: compile error, `runQuery` takes no such argument. + +- [ ] **Step 3: Add the enum and the parameter** + +In `src/notmuchworker.h`, inside the class: + +```cpp + /// 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) +``` + +Change `runQuery`'s declaration to take `SortOrder sort = NewestFirst`. + +- [ ] **Step 4: Use it** + +In `src/notmuchworker.cpp`, replace the hardcoded line 135: + +```cpp + notmuch_query_set_sort(nmQuery.get(), + sort == OldestFirst ? NOTMUCH_SORT_OLDEST_FIRST + : NOTMUCH_SORT_NEWEST_FIRST); +``` + +Leave `loadThread`'s `NOTMUCH_SORT_OLDEST_FIRST` (line ~216) alone: a thread's +messages read in chronological order regardless of how the list is sorted. + +- [ ] **Step 5: Run the test** + +```bash +cmake --build build && ./build/tests/test_notmuchworker +``` + +Expected: PASS. + +- [ ] **Step 6: Mutation-check** + +Change the ternary to always pass `NOTMUCH_SORT_NEWEST_FIRST`. Rerun. Expected: +FAIL. **Revert.** + +- [ ] **Step 7: Commit** + +```bash +git add src/notmuchworker.h src/notmuchworker.cpp tests/test_notmuchworker.cpp +git commit -S -m "feat(worker): let a query choose newest or oldest first + +Sorting was hardcoded NEWEST_FIRST. Two orders only: notmuch's other two are +MESSAGE_ID and UNSORTED, neither of which is an order a human wants, and sorting +by sender or subject would have to happen in the model after results arrive, +which fights the batching that makes a large query paint immediately. + +loadThread keeps OLDEST_FIRST unconditionally: a thread reads chronologically +whichever way the list is sorted." +``` + +--- + +## Task 4: `CardLayout`, the geometry with no painting + +**This is the most important task in the plan.** Everything the delegate draws +is positioned by this file, and because it touches no `QPainter` and no widget, +every claim about the card's shape gets a test that cannot be defeated by a +blank render. + +**Files:** +- Create: `src/cardlayout.h`, `src/cardlayout.cpp` +- Create: `tests/test_cardlayout.cpp` +- Modify: `src/CMakeLists.txt`, `tests/CMakeLists.txt` + +- [ ] **Step 1: Register the new files** + +In `src/CMakeLists.txt`, add `cardlayout.cpp` to the `qtmaildir_lib` source +list (alphabetically, after `cidschemehandler.cpp`). + +In `tests/CMakeLists.txt`, add: + +```cmake +add_qtmaildir_test(cardlayout) +``` + +- [ ] **Step 2: Write the failing test** + +Create `tests/test_cardlayout.cpp`: + +```cpp +/* + * 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 <QTest> + +class TestCardLayout : public QObject +{ + Q_OBJECT + +private slots: + void everyCardIsTheSameHeight(); + void threeLinesStackWithoutOverlapping(); + void replyIndentsByDepth(); + void indentStopsAtTheCap(); + void expanderSitsOnTheSecondLine(); + void expanderIsEmptyWithoutReplies(); + void dateIsFlushRight(); +}; + +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::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::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); + + QCOMPARE(card.dateRect.right(), rect.right() - 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()); +} + +QTEST_MAIN(TestCardLayout) +#include "test_cardlayout.moc" +``` + +- [ ] **Step 3: Run it and watch it fail** + +```bash +cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug +cmake --build build 2>&1 | head -20 +``` + +Expected: `cardlayout.h: No such file or directory`. + +- [ ] **Step 4: Write the header** + +Create `src/cardlayout.h` (GPL header as in every other file, then): + +```cpp +#pragma once + +#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. + }; + + /// Horizontal breathing room at the card's edges. + 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; + + /// 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); +}; +``` + +- [ ] **Step 5: Write the implementation** + +Create `src/cardlayout.cpp` (GPL header, then): + +```cpp +#include "cardlayout.h" + +#include <QFontMetrics> + +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. + small.setPointSizeF(qMax(6.0, cardFont.pointSizeF() - 1.0)); + 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(); + + // Indent, capped. qMin rather than a branch so depth 5 and depth 50 land + // in exactly the same place. + const int depth = qMin(input.depth, kMaxDepth); + const int indent = depth * kIndentStep; + out.contentLeft = rect.left() + 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 = rect.left() + kPaddingX + level * kIndentStep + + kIndentStep / 2; + out.spines.append(QRect(x, rect.top(), 2, rect.height())); + } + + const int right = rect.right() - kPaddingX; + const int lineOneTop = rect.top() + kPaddingY; + const int lineTwoTop = lineOneTop + metrics.height(); + const int lineThreeTop = lineTwoTop + metrics.height(); + + // The date is measured first and the sender gets what is left, so a long + // sender is elided rather than painting over the date. + const int dateWidth = metrics.horizontalAdvance( + QStringLiteral("8888-88-88 88:88")); + out.dateRect = QRect(right - dateWidth, lineOneTop, dateWidth, + metrics.height()); + out.senderRect = QRect(out.contentLeft, lineOneTop, + qMax(0, out.dateRect.left() - out.contentLeft + - kPaddingX), + metrics.height()); + + // The expander is the reply count, on line two and on the right. + if (input.replyCount > 0) { + const int countWidth = smallMetrics.horizontalAdvance( + QStringLiteral("▾ 8888 replies")); + out.expanderRect = QRect(right - countWidth, lineTwoTop, countWidth, + metrics.height()); + } + + const int subjectRight = out.expanderRect.isEmpty() + ? right + : out.expanderRect.left() - kPaddingX; + out.subjectRect = QRect(out.contentLeft, lineTwoTop, + qMax(0, subjectRight - out.contentLeft), + metrics.height()); + + out.tagRect = QRect(out.contentLeft, lineThreeTop, + qMax(0, right - out.contentLeft), + smallMetrics.height()); + + return out; +} +``` + +- [ ] **Step 6: Run the tests** + +```bash +cmake --build build && ./build/tests/test_cardlayout +``` + +Expected: all 7 PASS. + +- [ ] **Step 7: Mutation-check three of them** + +Each of these must make its own test fail. Revert after each. + +1. Change `qMin(input.depth, kMaxDepth)` to `input.depth`. + Expected: `indentStopsAtTheCap` FAILS. +2. Change `heightFor` to `metrics.height() * 2` (dropping the third line). + Expected: `threeLinesStackWithoutOverlapping` FAILS. +3. Change `out.senderRect`'s width to `right - out.contentLeft`. + Expected: `dateIsFlushRight` FAILS. + +If any mutation leaves every test green, that test is decorative. Fix it before +moving on. + +- [ ] **Step 8: Commit** + +```bash +git add src/cardlayout.h src/cardlayout.cpp tests/test_cardlayout.cpp src/CMakeLists.txt tests/CMakeLists.txt +git commit -S -m "feat(view): compute a card's geometry with no painting + +Split from the delegate deliberately. A delegate needs a live painter and an +exposed view, which is what makes delegate tests fragile: viewport()->render() +returns a blank image in several ordinary situations, and a probe reporting no +ink is likelier broken than the code it tests. Every geometric claim about a +card is made here, where a test is a function call. + +Three lines at a uniform height, so setUniformRowHeights(true) survives. Indent +caps at depth 4 with qMin rather than a branch, so depth 5 and depth 50 land in +the same place. The date is measured before the sender, so a long sender elides +instead of painting over it." +``` + +--- + +## Task 5: `CardDelegate` + +**Files:** +- Create: `src/carddelegate.h`, `src/carddelegate.cpp` +- Modify: `src/CMakeLists.txt` +- Modify: `src/tagchip.h`, `src/tagchip.cpp` (delete `SubjectDelegate`) + +- [ ] **Step 1: Register the file** + +Add `carddelegate.cpp` to `qtmaildir_lib` in `src/CMakeLists.txt`. + +- [ ] **Step 2: Write the header** + +Create `src/carddelegate.h` (GPL header, then): + +```cpp +#pragma once + +#include "tagchip.h" + +class CardLayout; + +/// 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); +}; +``` + +- [ ] **Step 3: Write the implementation** + +Create `src/carddelegate.cpp` (GPL header, then): + +```cpp +#include "carddelegate.h" + +#include "cardlayout.h" +#include "threadlistmodel.h" + +#include <QApplication> +#include <QDateTime> +#include <QPainter> +#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; +} + +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(); + + // Spines, under everything else. + for (const QRect &spine : card.spines) + painter->fillRect(spine, ThreadListModel::threadLineColour()); + + // 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, + date.toString(QStringLiteral("yyyy-MM-dd hh:mm"))); + + // 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. + if (!card.expanderRect.isEmpty()) { + painter->setFont(CardLayout::smallFont(chrome.font)); + const int count = index.data(ThreadListModel::ReplyCountRole).toInt(); + const QString glyph = (option.state & QStyle::State_Open) + ? QStringLiteral("▾") + : QStringLiteral("▸"); + painter->drawText(card.expanderRect, Qt::AlignVCenter | Qt::AlignRight, + QStringLiteral("%1 %2").arg(glyph).arg(count)); + painter->setFont(chrome.font); + } + + 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(); +} +``` + +Add `#include <QRegularExpression>` to the includes. + +- [ ] **Step 4: Delete `SubjectDelegate`** + +In `src/tagchip.h`, delete the entire `class SubjectDelegate` block (the one +starting `/// Item delegate for the subject column:`). **Keep the `TagChip` +namespace and `RowStyleDelegate`**, both of which `CardDelegate` uses. + +In `src/tagchip.cpp`, delete every `SubjectDelegate::` member definition. + +- [ ] **Step 5: Build** + +```bash +cmake --build build 2>&1 | head -30 +``` + +Expect errors in `threadlistview.cpp` and `mainwindow.cpp`, which still name +`SubjectDelegate` and the columns. Task 6 fixes them. Do not patch them here. + +- [ ] **Step 6: Commit** + +```bash +git add src/carddelegate.h src/carddelegate.cpp src/tagchip.h src/tagchip.cpp src/CMakeLists.txt +git commit -S -m "feat(view): paint the whole card in one delegate + +Replaces SubjectDelegate. The tag chips come home from the view: the strip was +painted there only because a delegate cannot paint outside its column and the +strip spanned all five, and with one column there is nothing to span. + +RowStyleDelegate is inherited rather than dropped. Its job survives the +redesign: Qt resolves ForegroundRole into the palette's Text roles and prefers +those over HighlightedText, so the read/unread dimming would win on a selected +row and land as grey on the highlight. + +A reply's Re: prefix is stripped here. Every reply repeating the thread's +subject is the visual signature of a table of records, which is the thing item +53 is about. + +The build is red at this commit; the view and window still name the old +delegate." +``` + +--- + +## Task 6: Strip the view, wire up the window + +**Files:** +- Modify: `src/threadlistview.h`, `src/threadlistview.cpp` +- Modify: `src/mainwindow.cpp` + +- [ ] **Step 1: Delete the view's painting** + +In `src/threadlistview.h`, delete the `paintEvent` declaration and replace the +class comment (which is now describing something that no longer happens): + +```cpp +/// The thread list. +/// +/// 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. +/// +/// 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 QTreeView::QTreeView; + +protected: + void mousePressEvent(QMouseEvent *event) override; +}; +``` + +In `src/threadlistview.cpp`, delete `ThreadListView::paintEvent` entirely and +every include it alone needed (`QPainter`, `QScrollBar`, `tagchip.h` if unused). + +- [ ] **Step 2: Rewrite the hit-test** + +Replace `ThreadListView::mousePressEvent` with: + +```cpp +void ThreadListView::mousePressEvent(QMouseEvent *event) +{ + 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); +} +``` + +The old `siblingAtColumn(0)` dance is gone: with one column, `index` already +is column 0. + +- [ ] **Step 3: Update the window's view setup** + +In `src/mainwindow.cpp`, find the thread-view setup (around line 548-580 on the +branch). Replace the per-column delegate installation and header configuration +with: + +```cpp + m_threadView->setModel(m_model); + m_threadView->setItemDelegate(new CardDelegate(m_threadView)); + m_threadView->setHeaderHidden(true); + m_threadView->setUniformRowHeights(true); + m_threadView->setRootIsDecorated(false); + m_threadView->setIndentation(0); // CardLayout draws the indent itself. + m_threadView->setSelectionBehavior(QAbstractItemView::SelectRows); + m_threadView->setSelectionMode(QAbstractItemView::ExtendedSelection); + m_threadView->verticalScrollBar()->setSingleStep( + CardLayout::heightFor(m_threadView->font())); +``` + +Delete every `setColumnWidth`, `header()->` call, and `setHorizontalScrollMode` +line for this view. Delete `setHorizontalScrollBarPolicy` too: with one column +there is nothing to scroll to, which is what closes item 51. + +Set the row height on the vertical header as the branch already does, using +`CardLayout::heightFor` in place of `SubjectDelegate::rowHeightFor`. + +- [ ] **Step 4: Build clean** + +```bash +cmake --build build 2>&1 | head -30 +``` + +Expected: no errors. Chase any remaining reference to a deleted column or +delegate. + +- [ ] **Step 5: Run the whole suite** + +```bash +ctest --test-dir build --output-on-failure +``` + +`test_mainwindow` will have failures referencing columns. Fix them by reading +what each asserts: a test naming `SubjectColumn` either moves to a role or is +deleted if the column was its whole subject. + +- [ ] **Step 6: Add the item 51 regression test** + +In `tests/test_mainwindow.cpp`: + +```cpp +void TestMainWindow::cardsNeverScrollSideways() +{ + MainWindow window; + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + + // 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. + QCOMPARE(view->horizontalScrollBar()->minimum(), + view->horizontalScrollBar()->maximum()); +} +``` + +- [ ] **Step 7: Commit** + +```bash +git add src/threadlistview.h src/threadlistview.cpp src/mainwindow.cpp tests/test_mainwindow.cpp +git commit -S -m "refactor(view): stop the view painting, and hit-test the reply count + +ThreadListView::paintEvent and its band arithmetic are deleted. The view existed +to paint a strip across five columns; with one column and one delegate painting +the whole card there is nothing to span, and the two faults that arithmetic kept +producing go with it: a deleted row cut in half, and every other row showing a +bare stripe. + +What survives is the expander hit-test, because a delegate gets no click of its +own without an editor. It now asks CardDelegate for the rect rather than +recomputing it, so the drawn target and the clickable one cannot drift. The +siblingAtColumn(0) dance is gone: with one column, the index already is column 0. + +Item 51 closes here rather than separately. A card is exactly viewport width, so +the view has no horizontal scroll range for a click to scroll into, and the test +asserts that directly." +``` + +--- + +## Task 7: Navigation (item 60) + +**Files:** +- Modify: `src/mainwindow.cpp:644-655` (both actions), and `addAction` +- Modify: `src/keymap.cpp` (defaults) +- Test: `tests/test_mainwindow.cpp` + +- [ ] **Step 1: Write the failing test** + +```cpp +void TestMainWindow::nextThreadLeavesTheLastReply() +{ + MainWindow window; + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + auto *view = window.findChild<ThreadListView *>(); + auto *model = qobject_cast<ThreadListModel *>(view->model()); + QVERIFY(model); + + ThreadSummary first; + first.threadId = QStringLiteral("T1"); + first.totalCount = 2; + ThreadSummary second; + second.threadId = QStringLiteral("T2"); + second.totalCount = 1; + model->appendBatch({ first, second }); + + MessageNode reply; + reply.messageId = QStringLiteral("M2"); + reply.threadId = QStringLiteral("T1"); + reply.depth = 1; + model->setReplies(QStringLiteral("T1"), { reply }); + + const QModelIndex root = model->index(0, 0); + view->expand(root); + const QModelIndex lastReply = model->index(0, 0, root); + QVERIFY(lastReply.isValid()); + view->setCurrentIndex(lastReply); + + // The defect: selectRow(current.row() + 1) asks for row 1 UNDER T1, which + // does not exist, so the action did nothing. It must land on T2. + window.findChild<QAction *>(QStringLiteral("next_thread"))->trigger(); + + QCOMPARE(view->currentIndex().data(ThreadListModel::ThreadIdRole).toString(), + QStringLiteral("T2")); +} +``` + +**This test must start from the last reply of an EXPANDED thread.** A test that +arrows down a collapsed list passes against the bug, because with nothing +expanded every row is top-level and the arithmetic is accidentally correct. + +The action needs a findable name; if `addAction` does not call +`setObjectName(name)`, add it there. + +- [ ] **Step 2: Run it and watch it fail** + +```bash +cmake --build build && ./build/tests/test_mainwindow -functions | grep nextThread +./build/tests/test_mainwindow nextThreadLeavesTheLastReply +``` + +Expected: FAIL, current index still on the reply. + +- [ ] **Step 3: Rewrite both actions** + +Replace the bodies at `src/mainwindow.cpp:644-655`: + +```cpp + addAction(QStringLiteral("next_thread"), tr("&Next thread"), + tr("Select the next thread"), [this]() { + // Walk by INDEX, never by row number. A tree numbers rows per parent, + // so current.row() + 1 named a sibling: from the last reply of an + // expanded thread it asked for a row that does not exist and the + // action silently did nothing (item 60). + QModelIndex index = m_threadView->indexBelow( + m_threadView->currentIndex()); + while (index.isValid() + && index.data(ThreadListModel::IsMessageRole).toBool()) { + index = m_threadView->indexBelow(index); + } + if (index.isValid()) + m_threadView->setCurrentIndex(index); + }); + addAction(QStringLiteral("prev_thread"), tr("&Previous thread"), + tr("Select the previous thread"), [this]() { + QModelIndex index = m_threadView->indexAbove( + m_threadView->currentIndex()); + while (index.isValid() + && index.data(ThreadListModel::IsMessageRole).toBool()) { + index = m_threadView->indexAbove(index); + } + if (index.isValid()) + m_threadView->setCurrentIndex(index); + }); +``` + +- [ ] **Step 4: Allow two shortcuts per action** + +In `src/mainwindow.cpp`, `addAction` currently calls `setShortcut` (singular) at +about line 622. Change it to collect every sequence bound to the action: + +```cpp + const QList<QKeySequence> sequences = m_keymap.sequencesFor(name); + if (!sequences.isEmpty()) + action->setShortcuts(sequences); +``` + +`zoom_reset` already uses `setShortcuts` at about line 773; follow that shape. +If `KeyMap` has no `sequencesFor`, add it: the bindings map is +sequence-to-action, so it is a loop collecting every key whose value matches. + +- [ ] **Step 5: Add the Alt defaults** + +In `src/keymap.cpp`, beside the existing Ctrl+J and Ctrl+K entries: + +```cpp + { 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") }, +``` + +- [ ] **Step 6: Run the tests** + +```bash +cmake --build build && ./build/tests/test_mainwindow +``` + +Expected: PASS, including `everyActionHasAShortcut`, which must survive the +move to `setShortcuts`. + +- [ ] **Step 7: Add the skip test and mutation-check** + +```cpp +void TestMainWindow::altDownSkipsReplies() +{ + // Same fixture as nextThreadLeavesTheLastReply, but starting from the + // thread ROOT with its replies showing: one step must land on the next + // thread, not on the first reply. + // ... build model, expand T1, setCurrentIndex(root) ... + window.findChild<QAction *>(QStringLiteral("next_thread"))->trigger(); + QCOMPARE(view->currentIndex().data(ThreadListModel::ThreadIdRole).toString(), + QStringLiteral("T2")); + QVERIFY(!view->currentIndex().data(ThreadListModel::IsMessageRole).toBool()); +} +``` + +Mutation: delete the `while` loop that skips message rows. Expected: +`altDownSkipsReplies` FAILS (it lands on the reply). **Revert.** + +- [ ] **Step 8: Commit** + +```bash +git add src/mainwindow.cpp src/keymap.cpp tests/test_mainwindow.cpp +git commit -S -m "fix(ui): step by index, not by row number (item 60) + +next_thread was selectRow(current.row() + 1). A QTableView numbers rows once for +the whole view, so that was correct before message rows; a tree numbers them per +parent, so from the last reply of an expanded thread it asked for a sibling that +does not exist and did nothing at all. prev_thread failed the mirror case. +rowCount() with no argument compounded it by counting top-level threads. + +Both now walk with indexBelow/indexAbove, skipping message rows, so they keep +meaning thread-to-thread. Stepping message-to-message needs no code: QTreeView's +own Up/Down walk VISIBLE rows and already enter an expanded thread, and being +the view's key handling rather than a shortcut they stay inert when the message +pane, a menu or an entry bar has focus. + +Alt+Up/Down added alongside Ctrl+J/K, which required addAction to move from +setShortcut to setShortcuts. Alt because Shift+arrows is the built-in +extend-selection that multi-row tagging depends on, and because a bare arrow +cannot be a window shortcut without breaking every text field in the window, as +Return already demonstrated. + +The test starts from the last reply of an expanded thread deliberately: one that +arrows down a collapsed list passes against the bug." +``` + +--- + +## Task 8: The sort dropdown + +**Files:** +- Modify: `src/mainwindow.cpp` (query row, `runCurrentQuery`, UI state) +- Test: `tests/test_mainwindow.cpp` + +- [ ] **Step 1: Write the failing test** + +```cpp +void TestMainWindow::sortChoiceSurvivesRestart() +{ + { + MainWindow window; + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + auto *sort = window.findChild<QComboBox *>(QStringLiteral("sortOrder")); + QVERIFY(sort); + QCOMPARE(sort->currentIndex(), 0); // Newest first by default. + sort->setCurrentIndex(1); + window.close(); + } + + MainWindow second; + second.show(); + QVERIFY(QTest::qWaitForWindowExposed(&second)); + auto *sort = second.findChild<QComboBox *>(QStringLiteral("sortOrder")); + QCOMPARE(sort->currentIndex(), 1); +} +``` + +**This writes to the real UI state file.** Follow whatever +`test_mainwindow` already does to redirect `QStandardPaths` (it must, since it +tests window geometry); if it uses `QStandardPaths::setTestModeEnabled(true)`, +this test needs it too. + +- [ ] **Step 2: Run it and watch it fail** + +Expected: FAIL, no such child. + +- [ ] **Step 3: Add the combo box** + +In the query-row setup: + +```cpp + m_sortOrder = new QComboBox(this); + 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); +``` + +Add `QComboBox *m_sortOrder = nullptr;` to `mainwindow.h`. + +- [ ] **Step 4: Pass it to the worker** + +In `runCurrentQuery`, where the query is handed across: + +```cpp + const auto sort = m_sortOrder->currentIndex() == 1 + ? NotmuchWorker::OldestFirst + : NotmuchWorker::NewestFirst; + emit queryRequested(query, ++m_generation, sort); +``` + +Update the signal's declaration and the queued connection to carry it. Since +this crosses threads, `SortOrder` needs `qRegisterMetaType` unless `Q_ENUM` on a +`QObject` subclass already covers it; if the connection warns at runtime about +an unregistered type, add the registration in `main.cpp`. + +- [ ] **Step 5: Persist it** + +In `saveUiState()`: + +```cpp + settings.setValue(QStringLiteral("sortOrder"), m_sortOrder->currentIndex()); +``` + +In the restore, guarding the range because a hand-edited or stale file can hold +anything: + +```cpp + const int sort = settings.value(QStringLiteral("sortOrder"), 0).toInt(); + m_sortOrder->setCurrentIndex(sort == 1 ? 1 : 0); +``` + +- [ ] **Step 6: Run the test** + +```bash +cmake --build build && ./build/tests/test_mainwindow +``` + +Expected: PASS. + +- [ ] **Step 7: Commit** + +```bash +git add src/mainwindow.cpp src/mainwindow.h tests/test_mainwindow.cpp +git commit -S -m "feat(ui): let the user choose newest or oldest first + +Two entries, straight to notmuch. This adds a feature rather than replacing one: +the column header was decorative and nothing implemented click-to-sort, so +removing the header with the grid lost nothing. + +Stored in uistate.conf, never in the hand-edited config, and range-guarded on +read: a stale file can hold anything, which is the lesson item 58 recorded." +``` + +--- + +## Task 9: Documentation and the changelog + +**Files:** +- Modify: `CLAUDE.md`, `CHANGELOG.md` +- Modify: `docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md` + +- [ ] **Step 1: Correct the architecture diagram** + +`CLAUDE.md`'s diagram and its `ThreadListView` paragraph both describe the tag +strip as the reason that class exists. That stops being true here. Rewrite the +paragraph beginning **"`ThreadListView` exists because a delegate cannot paint +outside its column"** to say what is true now: one column, one delegate that +paints the whole card, and the view surviving only for the expander hit-test. + +Keep the historical lesson in one sentence, because it is why the file looks the +way it does, but do not leave it stated as current behaviour. + +- [ ] **Step 2: Add the new traps** + +Append to `CLAUDE.md`, near the other Qt traps: + +```markdown +**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". + +**`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. +``` + +- [ ] **Step 3: Changelog** + +Under `## [Unreleased]`, in `### Changed`: + +```markdown +- 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. +- Threads can be listed newest or oldest first, from a new control beside the + query bar. The choice is remembered. +``` + +In `### Fixed`: + +```markdown +- Next and previous thread no longer do nothing when the last reply of an + expanded thread is selected. +- Clicking a thread no longer scrolls the list sideways. +``` + +Add an `### Upgrading` note: the column widths in a user's `uistate.conf` are +now ignored, which is silent and harmless but worth saying. + +- [ ] **Step 4: Close the backlog items** + +Mark 51, 53 and 60 `done` in the status table, and item 20 `done` (the +presentation it specified now exists in its revised form). Do not renumber. + +- [ ] **Step 5: Commit** + +```bash +git add CLAUDE.md CHANGELOG.md docs/ +git commit -S -m "docs: record the card list, and correct what CLAUDE.md claims + +The architecture section described ThreadListView as existing to paint a strip +across columns. That was true until this change and is now the opposite of +true: it survives only for the expander hit-test. Kept as one sentence of +history, since it explains the file's shape, but no longer stated as current +behaviour. + +Items 20, 51, 53 and 60 close together." +``` + +--- + +## Task 10: Hand verification + +Automated tests cannot cover the parts of this that matter most. `CLAUDE.md` +records that a whole session was lost to rendering probes that lied, and that +the original bug was the desktop's own font being configured Bold in qt6ct. + +- [ ] **Step 1: Run it against the real database** + +```bash +./build/src/qtmaildir +``` + +- [ ] **Step 2: Check each of these by eye** + +- Cards are all the same height, tagged and untagged alike. +- An expanded thread's replies indent, with a continuous line down each level. +- A reply shows no `Re:` prefix. +- A reply with a tag its thread lacks shows that tag; ordinary replies show + none. +- Clicking the reply count expands; clicking anywhere else on the card opens + the message and does not expand. +- **Up/Down step into an expanded thread's replies.** This is the one claim in + the whole design that rests on built-in Qt behaviour rather than on code + written here, and `CLAUDE.md` warns that `QTest::keyClick` is weak evidence + about key reachability. Verify it on a real keyboard. +- Alt+Up/Alt+Down skip replies entirely. +- Arrow keys still move the cursor in the query bar and scroll the message + pane, rather than moving the list. +- The sort dropdown reverses the list, and the choice survives a restart. +- The list does not scroll sideways on any click, at any window width. + +- [ ] **Step 3: Check the theme you do not use** + +Switch the desktop between light and dark and confirm the spine, the chips and +the dimmed reply text are all still legible. Every colour here comes from the +palette for this reason, but the only proof is looking. + +- [ ] **Step 4: Report back** + +Per the user's standing preference, hand the result over rather than declaring +it done. Say what was checked and what was not. + +--- + +## Self-Review Notes + +**Spec coverage.** Every section of the spec maps to a task: the card (4, 5), +reply line 3 (1), the spine and indent cap (4), the expander (5, 6), sorting +(3, 8), what is deleted (5, 6), what is kept (verified by the suite staying +green), the new roles (1, 2), testing (each task), keyboard navigation (7), and +returning to a whole thread (nothing to build, it is the root card). + +**Known gap, deliberately left.** The spec says `CardDelegate` should draw the +account chip, and no task does. It is an addition to Task 5's `paint`, on line +1 in front of the sender, using `AccountLabelRole` and `AccountColourRole` +exactly as `SubjectDelegate` did. It is left out because the layout for it was +not specified and inventing one here would be a guess; add it as Task 5b after +seeing the cards on screen, or leave the chip out if the account is already +clear from the sender. + +**Ordering risk.** Tasks 2, 5 and 6 leave the build red between commits. That +is deliberate, since splitting a column removal across three files cannot be +atomic without one enormous commit, but it means **the branch must not be +merged mid-plan**. Finish through Task 6 before pushing anywhere the user might +build from. |
