diff options
Diffstat (limited to 'docs/superpowers')
| -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. |