From 380a59ab01ef578292e772c9e308f37124ab7e30 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 10:11:51 +0200 Subject: docs(item-20): specify message rows in the thread list and plan the work The item sat as 'open, unspecified' since 2026-08-04, recording only that the user's mental model differed from what was built. Described now from three screenshots plus four decisions: the left pane gains message rows, the root row IS the thread's first message, replies indent by true reply depth, and action scope follows the selected row kind. No confirmation dialog, per this project's standing rule. The hazard the design introduces is ambiguity rather than destruction, since deleted is a tag and every mutation is invertible, so the scope is named in the status bar before and after the action instead. Sized L, the largest item in the backlog and the first to warrant a branch. The plan records four API facts verified against the installed headers, each contradicting the obvious approach: replies are unreachable from a query walk, thread-derived messages must not be RAII-wrapped, QTreeView lacks isRowSelected(int), and a tree numbers rows per parent. --- docs/superpowers/plans/2026-08-08-item-20-message-rows.md | 9 --------- 1 file changed, 9 deletions(-) (limited to 'docs') diff --git a/docs/superpowers/plans/2026-08-08-item-20-message-rows.md b/docs/superpowers/plans/2026-08-08-item-20-message-rows.md index 5935777..f4882c7 100644 --- a/docs/superpowers/plans/2026-08-08-item-20-message-rows.md +++ b/docs/superpowers/plans/2026-08-08-item-20-message-rows.md @@ -1,14 +1,5 @@ # Item 20: Message Rows in the Thread List — Implementation Plan -> **EXECUTED AND PARKED, 2026-08-08. Do not run this plan again.** -> Every task here was implemented on the branch `item-20-message-rows`, which is -> pushed to both remotes and **not merged**. master has these documents and none -> of that code. The user's verdict on the finished result was that the table -> layout does not fit the use, and item 53 in -> `2026-08-03-post-0.1.0-usability.md` carries the diagnosis. This document is -> kept for the API facts it verified and the reasoning it records, not as work -> to pick up. Read item 53 first. - > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. **Goal:** Turn the flat thread list into a tree where a thread's root row is its first message, expanding reveals the replies indented by reply depth, and selecting a reply opens that single message in the reading pane. -- cgit v1.2.3 From 1a96ce7d2a2ba5575502db85bce8ed0efe799665 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 08:55:13 +0200 Subject: 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 paragraph of history, since it explains the file's shape, but no longer stated as current behaviour. Two traps are recorded inverted rather than deleted, because the rule survived its own reason changing. The reply indent is still asserted on where the TEXT lands, but where visualRect lies has flipped: it used to report an indent the text did not have, and now reports none while the text is indented. And Q_ENUM is documented as insufficient for a queued Q_ARG, which cost a silently dropped sort argument. Item 60's recorded cause was wrong and is corrected in place. It was read off master, where the row arithmetic really is current.row() + 1; the branch had already fixed it a commit earlier with threadRowOf(). The entry stays, with the correction, because the reasoning was sound and the tests it demanded now exist. Items 20, 51 and 53 are marked built on the branch rather than done. Nothing is merged and the user has not seen it, which is the whole point of Task 10. --- CHANGELOG.md | 40 +++++-- CLAUDE.md | 120 +++++++++++++-------- .../plans/2026-08-03-post-0.1.0-usability.md | 30 ++++-- 3 files changed, 130 insertions(+), 60 deletions(-) (limited to 'docs') diff --git a/CHANGELOG.md b/CHANGELOG.md index 1444c3a..5bf9256 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,29 @@ point at which they are stable. ## [Unreleased] +### Changed + +- **The thread list is now a list of cards rather than a table of columns.** + Each thread shows its sender and date, its subject with the flag, attachment + and reply-count marks, and its tags, on three lines at one uniform height. + Expanding a thread shows its replies indented under a continuous spine, + carrying only the tags the thread itself does not have, and without the `Re:` + prefix every reply used to repeat. +- **Threads can be listed newest or oldest first**, from a new control beside + the query bar. The choice is remembered between sessions. +- **An account's colour now runs down the left edge of its threads**, and down + the spine of their replies, replacing the account chip that used to sit in + front of every subject. The account dropdown shows the same colours, so which + colour means which account is readable in one place. +- **Alt+Up and Alt+Down step between threads**, alongside the existing Ctrl+J + and Ctrl+K. Plain Up and Down now step message by message through an expanded + thread, which is the view's own behaviour rather than a binding. +- **An out-of-range `message_zoom` now says so.** The documented 0.5 to 3.0 + range was already enforced on the way to the web view, so a `message_zoom` of + 500 rendered at 3.0 rather than unusably, but nothing reported that the value + in the file was not the value on screen. It is now listed with the other + configuration problems at startup. + ### Fixed - **The message pane no longer comes back as a sliver.** A splitter position is @@ -19,13 +42,18 @@ point at which they are stable. left, in one real case 29px. The pane now has a minimum width and cannot be collapsed, which covers the restore and the equivalent drag. -### Changed +- **Clicking a thread no longer scrolls the list sideways.** A card is exactly + the width of the pane, so there is nowhere to scroll to. +- **Next and previous thread no longer step onto a reply** when a thread is + expanded. They skip message rows, so they keep meaning thread-to-thread. -- **An out-of-range `message_zoom` now says so.** The documented 0.5 to 3.0 - range was already enforced on the way to the web view, so a `message_zoom` of - 500 rendered at 3.0 rather than unusably, but nothing reported that the value - in the file was not the value on screen. It is now listed with the other - configuration problems at startup. +### Upgrading + +- **Saved thread-list column widths are ignored.** There is one column now, so + the `threadlist/header` and `threadlist/columns` entries in + `~/.local/state/qtmaildir/uistate.conf` no longer do anything. Nothing needs + to be done: they are read past and can be left in place or deleted. Window + geometry, the splitter position and the message zoom are unaffected. ## [0.12.1] - 2026-08-09 diff --git a/CLAUDE.md b/CLAUDE.md index f53f0d9..ded87e2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -43,10 +43,10 @@ MainWindow NotmuchWorker ├ query row: QComboBox, QLineEdit, └ owns the only notmuch_database_t* │ saved-query QPushButtons ├ ThreadListView (QTreeView) ── ThreadListModel (QAbstractItemModel) - │ thread rows, expanding to message rows; RowStyleDelegate every - │ column, SubjectDelegate on Subject (chip, subject, expander) + │ ONE column of cards; CardDelegate paints each whole, from CardLayout └ MessageView (header QLabel, QWebEngineView, attachment bar, TagStrip) +CardLayout (pure geometry, no painting) Config (INI) KeyMap MailSync (QProcess) MimeParser (GMime) SyncMonitor (/proc/locks) TagColors QueryCompleter ThreadCidMap ``` @@ -56,62 +56,92 @@ The query row and the message-pane header are **built inline in `MainWindow` and listed `QueryBar`, `SavedQueryBar`, `HeaderWidget` and `AttachmentBar`; none of those types have ever existed, and looking for them wastes a search. The widget classes that do exist are `MessageView`, `ThreadListView`, `TagStrip`, -`TagDialog`, `RowStyleDelegate` and `SubjectDelegate`; `TagChip` is a namespace -of painting helpers, not a widget, and `ThreadCidMap` is a struct. - -**`ThreadListView` exists because a delegate cannot paint outside its column.** -The tag chips under each row are one strip spanning the whole width, so they -are drawn in the view's `paintEvent` after the cells. Consequences that are -easy to undo by accident: `SubjectDelegate` reads `AccountLabelRole`, which -belongs to the ROW, so installing it view-wide draws the account chip into -every column (a `Q_ASSERT` catches this); and because alternating colours, the -selection and the model's `BackgroundRole` are all painted per cell, the view -has to fill the strip's band itself, honouring all three or a deleted row is -cut in half and every other row shows a bare stripe. +`TagDialog`, `RowStyleDelegate` and `CardDelegate`; `TagChip` is a namespace of +painting helpers, not a widget, and `ThreadCidMap` and `CardLayout` are structs. +`SubjectDelegate` existed until item 53 and is gone. + +**`ThreadListView` survives only for the expander hit-test.** `CardDelegate` +draws the reply count, and a delegate gets no click of its own without an +editor, so the view owns the click and asks the delegate for the rect rather +than recomputing it. + +Until item 53 it also painted a row-wide strip of tag chips after the cells, +because a delegate cannot paint outside its column and the strip spanned all +five. That is why the class exists at all, and the history is worth keeping: +the arithmetic it needed produced a deleted row cut in half and every other row +showing a bare stripe, both because the view had to re-honour alternating +colours, the selection and `BackgroundRole` across cells it did not own. With +one column there is nothing to span, so the `paintEvent` and its band +arithmetic are deleted and none of that applies any more. + +**A card layout must be testable without a painter.** `CardLayout` computes +every rect on a card and touches no `QPainter` and no widget, so the geometry +has tests that a blank render cannot defeat. When changing what a card shows, +change `CardLayout` and assert there; a test that renders the delegate and +counts pixels proves nothing, for the reasons under "Rendering probes lie". +Two traps it already handles: `QRect::right()` is inclusive, so the right edge +is carried as an exclusive one, and `QFont::pointSizeF()` returns -1 for a font +set in pixels, which qt6ct does. + +**`QTreeView`'s Up/Down already walk into an expanded thread's replies**, and +that is where message-to-message navigation comes from. Do not bind arrow keys +as `QAction` shortcuts to get it: a shortcut is dispatched before the focused +widget sees the key and Qt withholds only plain LETTERS from editable widgets, +so a bare `Up` would break the query bar, the tag dialog and the web view at +once. `Alt+Up`/`Alt+Down` are chords and therefore safe; `Shift+Up`/`Down` is +the built-in extend-selection and must be left alone. Binding two sequences to +one action needs `setShortcuts`, not `setShortcut`, which keeps only the last. + +**`Q_ENUM` is not enough to send an enum across a queued connection.** It gives +the type a meta-object entry, not a metatype registered under the name +`invokeMethod` resolves, so a `Q_ARG` carrying it is dropped at runtime with a +warning and the slot runs with a default. `NotmuchWorker::SortOrder` is +registered beside the type for this reason, not in `MainWindow`, so a caller +that never constructs one still gets it. **It is a `QTreeView` over a `QAbstractItemModel` since item 20**, because a thread's replies are child rows and a table can neither indent nor expand. What did NOT survive that port is anything keyed on a row NUMBER: a tree numbers rows -per parent, so row 0 exists once per expanded thread and a flat `0..N` walk -paints the first thread's strip over every one of them. The strip walk goes by -index, alternating colour follows visual position rather than `index.row()`, and -`QTableView::isRowSelected(int)` has no equivalent — use -`selectionModel()->isSelected(index)`. Row height comes from -`setUniformRowHeights` plus the delegate's `sizeHint`, since a tree has no -vertical header to carry a default section size. - -**Four traps in the expander, all of which shipped a plausible-looking broken +per parent, so `row 0` exists once per expanded thread and `current.row() + 1` +names a sibling rather than the next thread. Navigation walks with +`indexBelow`/`indexAbove`; `QTableView::isRowSelected(int)` has no equivalent — +use `selectionModel()->isSelected(index)`. Row height comes from +`setUniformRowHeights` plus `CardDelegate::sizeHint`, since a tree has no +vertical header to carry a default section size. Indentation is +`setIndentation(0)`: `CardLayout` draws the indent inside the card's own rect, +so `visualRect` reports the SAME left edge for a thread and its reply and a +geometry probe sees no nesting in a correctly nested list. + +**Three traps in the expander, all of which shipped a plausible-looking broken build before being caught.** `QTreeView::drawBranches` is the documented hook and does not work when the expander sits on a content column: it runs BEFORE the row's cells, so the delegate's background paints over it (a 60-pixel triangle -survived as 8). `SubjectDelegate` draws it instead, from BOTH of its branches — -calling it only from the no-chip branch leaves every real row without one, since -every real row has an account chip. `setRootIsDecorated(false)`, needed to stop -the style drawing its own indicator underneath, also removes the style's HIT -AREA, so the glyph renders perfectly and is inert; `ThreadListView::mousePressEvent` -handles the click. And `isExpanded`/`setExpanded` are keyed on **column 0**, so -asking them about the subject-column index always answers false and every click -expands again instead of toggling. +survived as 8). `CardDelegate` draws it instead, as the reply count on the +card's second line. `setRootIsDecorated(false)`, needed to stop the style +drawing its own indicator underneath, also removes the style's HIT AREA, so the +glyph renders perfectly and is inert; `ThreadListView::mousePressEvent` handles +the click, asking `CardDelegate::expanderRectFor` for the target so the drawn +and clickable rects cannot drift. A fourth trap died with the grid: `isExpanded` +is keyed on column 0, which used to disagree with the subject-column index. **Visible, clickable and toggling are three separate properties.** A test for one passes against the other two being broken, which happened twice in one session: a pixel test proved the triangle was drawn while nothing could click it, and a click test proved it opened while it could never close. -**A reply row's indent must beat the account chip's width.** A thread row draws -a chip before its subject and a reply row does not, so a reply's text starts -roughly a chip-width to the LEFT of its thread's before any indent applies. -Qt's 20px default is swallowed entirely by that difference and the replies read -as flush or outdented. `SubjectDelegate::kReplyIndent` is 72px for this reason. -Note that `visualRect` reports the indent correctly the whole time, so a -geometry probe endorses a layout with no visible nesting: assert on where the -TEXT lands. - -**`paintEvent` runs AFTER the cells.** Anything it fills across a row covers the -text the delegate just drew: the reply tint filled the full row height in its -first version and erased every sender and subject, measured at zero surviving -text pixels. The fill and the thread-line stub stay in the band below the text, -where the tag strip lives on thread rows. +**Assert a reply's indent on where the TEXT lands, never on `visualRect`.** The +reason has inverted twice and the rule has not. Under item 20 the geometry was +indented while the text was not, because the delegate laid text out from its own +left edge; now `setIndentation(0)` means `visualRect` reports no indent at all +while the text is indented, because `CardLayout` draws it inside the card's rect. +A probe on `visualRect` therefore endorsed a broken layout then and would fail a +correct one now. Assert on `CardLayout::contentLeft`. + +**`paintEvent` ran AFTER the cells**, which is why anything the view filled +across a row covered the text the delegate had just drawn: the reply tint filled +the full row height in one version and erased every sender and subject, measured +at zero surviving text pixels. Recorded because it is the class of bug a view +that paints invites. `ThreadListView` no longer paints at all. **No `notmuch_*` pointer ever crosses the thread boundary.** Data crosses as the plain value structs in `src/types.h` (`ThreadSummary`, `MessageRef`, `MessageNode`, diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index 17cf434..ad6dbc0 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -77,7 +77,7 @@ taking that too literally. | 17 | No completion for tags in the query bar | workflow | M | **done** | | 18 | No visual cue that there are unsynced edits | feedback | S | **done** | | 19 | No prompt to sync on exit when edits are pending | behavior | S | **done** | -| 20 | Thread view does not match the user's mental model | presentation | L | built on branch `item-20-message-rows`, **not merged**; see 53 | +| 20 | Thread view does not match the user's mental model | presentation | L | rebuilt as cards on branch `card-list`, **not merged**; awaiting the user's verdict | | 21 | Default shortcuts are not sensible enough | discoverability | S | open | | 22 | Translatability audit and i18n wiring | correctness | M | open | | 23 | No way to save a search query from the UI | workflow | M | open | @@ -108,16 +108,16 @@ taking that too literally. | 48 | Removing a tag suggests every tag, not the thread's own | workflow | XS | **done** | | 49 | Sync runs every account regardless of what changed | workflow | M | **done** | | 50 | Esc blanks the pane but leaves the row selected | workflow | XS | **done** | -| 51 | Clicking a subject scrolls the list sideways | presentation | XS | open; resolved as a side effect of 53, do not work separately | +| 51 | Clicking a subject scrolls the list sideways | presentation | XS | built on branch `card-list`, **not merged**; a card is viewport width | | 52 | `test_querycompleter` fails under Wayland, passes offscreen | testing | XS | **done** | -| 53 | Message rows still read as a table, not as a conversation | presentation | M | open, specified 2026-08-09; see the card-list spec | +| 53 | Message rows still read as a table, not as a conversation | presentation | M | built 2026-08-10 on branch `card-list`, **not merged**; awaiting the user's verdict | | 54 | A cron sync carries the edits but the count still says pending | correctness | S | **done** | | 55 | In a narrow window the message pane is invisible | presentation | XS | **done** | | 56 | No action carries an icon, so the toolbar reserves space for nothing | presentation | S | **done** | | 57 | "Flag" would read better as "Important" or "Starred" | presentation | XS | **done** | | 58 | `message_zoom` documents a 0.5 to 3.0 range and enforces none of it | correctness | XS | **done** | | 59 | Archive and Mark all read shipped with the same icon | presentation | XS | **done** | -| 60 | Next thread dead-ends on the last reply of an expanded thread | defect | XS | open; branch only, fix as part of 53 | +| 60 | Next thread dead-ends on the last reply of an expanded thread | defect | XS | **done** on the branch; already fixed by 5487d58, see below | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -3683,11 +3683,23 @@ navigation actions against the same rule. current index, which follow visible rows across parent boundaries. For thread-to-thread jumping, skip any index whose `IsMessageRole` is true. -**Specified as part of the card-list spec** -(`docs/superpowers/specs/2026-08-09-card-list-design.md`), which also adds -Alt+Up/Down for these actions and relies on `QTreeView`'s built-in Up/Down for -stepping through replies. Fix it there rather than separately, unless the card -list is dropped. +**The cause above is wrong, and was corrected on 2026-08-10.** It was read off +`master`, where the arithmetic really is `current.row() + 1`. The branch does +not do that: `5487d58` added `MainWindow::threadRowOf()`, which walks up to the +containing thread BEFORE the arithmetic, so from the last reply of an expanded +thread `next_thread` already reached the next thread. The defect was fixed in +the same commit that could have introduced it, one commit before this entry was +written. Verified by writing both failing tests first, on the branch, and +watching them pass against unchanged code. + +The entry is kept rather than deleted, because the reasoning it records is +sound and the tests it demanded now exist. It is a reminder that a cause +"verified in code" is only verified against the branch it was read on. + +**Superseded by the card-list work all the same.** Both actions now walk with +`indexBelow`/`indexAbove` (`card-list`, 2026-08-10), so nothing in that path is +keyed on a row number, which is the rule a deeper tree would break next. +Alt+Up/Down were added alongside Ctrl+J/K there. **Constraint.** The test that would catch this must start from the **last reply of an expanded thread**. A test that arrows down a collapsed list passes against -- cgit v1.2.3