diff options
| -rw-r--r-- | CHANGELOG.md | 13 | ||||
| -rw-r--r-- | CLAUDE.md | 73 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 76 |
3 files changed, 152 insertions, 10 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 851e318..56987f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,19 @@ point at which they are stable. ### Added +- **Threads expand in the list to show their replies.** A thread with more than + one message carries an expander; opening it lists the replies as indented rows + beneath it, marked with a thread line, a tinted background and smaller text. + The replies are fetched when you expand, not with the query, so a large result + still paints immediately. +- **Selecting a reply opens that message on its own**, rather than the whole + conversation, which is the point of having message rows at all. +- **Actions follow what you selected, and the status bar says what they will + touch.** A thread row acts on the whole thread and reports "1 thread selected + (7 messages)" before and "(whole thread)" after; a reply row acts on that one + message. Both are undoable. There is no confirmation dialog, deliberately: + undo is this application's answer to a mistaken action, and naming the scope + is what makes it usable. - **A sync now fetches only the accounts you have edited.** Tagging mail in one account and syncing no longer pulls every other account as well. A sync with nothing outstanding is a plain fetch and still covers everything, since @@ -42,8 +42,9 @@ UI thread Worker thread MainWindow NotmuchWorker ├ query row: QComboBox, QLineEdit, └ owns the only notmuch_database_t* │ saved-query QPushButtons - ├ ThreadListView ── ThreadListModel - │ (RowStyleDelegate every column, SubjectDelegate on Subject) + ├ ThreadListView (QTreeView) ── ThreadListModel (QAbstractItemModel) + │ thread rows, expanding to message rows; RowStyleDelegate every + │ column, SubjectDelegate on Subject (chip, subject, expander) └ MessageView (header QLabel, QWebEngineView, attachment bar, TagStrip) Config (INI) KeyMap MailSync (QProcess) MimeParser (GMime) @@ -63,19 +64,73 @@ The tag chips under each row are one strip spanning the whole width, so they are drawn in the view's `paintEvent` after the cells. Consequences that are easy to undo by accident: `SubjectDelegate` reads `AccountLabelRole`, which belongs to the ROW, so installing it view-wide draws the account chip into -every column (a `Q_ASSERT` catches this); row height must be set on the -vertical header, since a table takes one height per row and a column's -`sizeHint` only applies if the view happens to ask that column; and because -alternating colours, the selection and the model's `BackgroundRole` are all -painted per cell, the view has to fill the strip's band itself, honouring all -three or a deleted row is cut in half and every other row shows a bare stripe. +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. + +**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 +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. + +**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. **No `notmuch_*` pointer ever crosses the thread boundary.** Data crosses as the plain -value structs in `src/types.h` (`ThreadSummary`, `MessageRef`, `TagChange`), over queued +value structs in `src/types.h` (`ThreadSummary`, `MessageRef`, `MessageNode`, +`ActionScope`, `TagChange`), over queued signals in both directions. `notmuchworker.cpp` is the only file that includes `notmuch.h` outside `src/nmraii.h`; C handles are owned by the `NmQuery`/`NmMessages`/`NmThread`/… RAII aliases there so they cannot leak. +**The one exception, and it is a double-free if undone.** Messages reached +through `notmuch_thread_get_toplevel_messages` / `notmuch_message_get_replies` +are owned by the THREAD and freed with it (`notmuch.h:1637`), so `walkReplies` +in `notmuchworker.cpp` holds them as raw `notmuch_message_t*`: an `NmMessage` +wrapper would call `notmuch_message_destroy` on memory the thread frees again. +The whole walk must finish while the `NmThread` is alive. Related: replies are +unreachable from a query walk at all — `notmuch_message_get_replies` returns +NULL for a message from `notmuch_query_search_messages` (`notmuch.h:1617-1628`), +which is why `loadThreadTree` exists beside `loadThread` rather than replacing +it. + **Generation counters, not cancellation.** Each query bumps a `quint64` generation passed through to the worker and back on every result signal. The UI discards results whose generation is stale. The worker never needs to know a query was superseded. Threads are diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index a925cd8..9afbd7a 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 @@ -68,7 +68,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 | open, specified 2026-08-08, branch `item-20-message-rows` | +| 20 | Thread view does not match the user's mental model | presentation | L | **built 2026-08-08**, branch `item-20-message-rows`; see 53 | | 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 | @@ -101,6 +101,7 @@ taking that too literally. | 50 | Esc blanks the pane but leaves the row selected | workflow | XS | **done** | | 51 | Clicking a subject scrolls the list sideways | presentation | XS | open | | 52 | `test_querycompleter` fails under Wayland, passes offscreen | testing | XS | **done** | +| 53 | Message rows still read as a table, not as a conversation | presentation | ? | open, unspecified | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1231,6 +1232,18 @@ is centralised rather than smeared across call sites. warrant a feature branch (`item-20-message-rows`, 2026-08-08). It is a left-pane model and view replacement plus a selection-scope change, not a refactor. +**Built 2026-08-08 across ten tasks**, all four decisions implemented: message +rows in the left pane, the root row as the thread's first message, replies +indented by depth, action scope following the selected row kind and named in the +status bar. 15 test binaries green, 65 tests in `test_mainwindow` alone, ten +mutation checks. + +**The user's verdict on the result: not convinced it fits.** Recorded here +rather than in a commit message, because the item shipped working and the +reservation is about the DESIGN, not about a defect. See item 53, which carries +the diagnosis. Do not treat item 20 as a success story to build on without +reading it. + **A cheaper alternative was offered and declined.** Collapsible `<details>` blocks per message inside the existing single-document message pane would have delivered per-message inspection at size S, touching only `htmlbuilder.cpp` and @@ -3147,6 +3160,67 @@ popup how it likes. makes the width guard fail with its explanation, where before the change that case would have passed. +## 53. Message rows still read as a table, not as a conversation + +**Observed (user, 2026-08-08)**, on the finished item 20: *"I'm not very +convinced about this session's work. I don't think the table view fits our +use."* Said after the expander, the indent, the thread spine, the tint and the +dimmed text were all in and working, so it is not a report that a cue is +missing. It is a judgement on the result. + +**This is a design finding, not a defect.** Item 20 shipped exactly what its +four decisions specified and every one of them was the user's own choice. The +work is sound; what it produced is not what was wanted. Recording it as a defect +would misattribute the cause, and recording nothing would leave the next session +building on a design the user has already rejected. + +**Cause (verified in code, 2026-08-08).** A message row fills the SAME five +columns as a thread row: `ThreadListModel::data` answers `DateColumn`, +`AuthorsColumn` and `SubjectColumn` for message rows at +`src/threadlistmodel.cpp:275-283`, mirroring the thread-row branch at +`:428-431`. One model, one column grid, both row kinds. So every reply lands on +the same rigid column boundaries as the threads around it, and the eye reads +columns before it reads indentation or tint. + +Compare the reference the user gave. In the Thunderbird screenshots the reply +rows carry sender and date only, laid out freely on a plain band, with no column +rules running through them. The structure comes from the ABSENCE of the grid, +which is the one thing three added cues cannot supply. + +Two aggravating details, both visible in the 2026-08-08 screenshots: + +- Every reply repeats `Re: <the thread's subject>`, near-identical down the + whole block, which is exactly the visual signature of a table of records. + Dropping the redundant prefix was offered and not chosen; it is worth + revisiting first because it is the cheapest of these by a wide margin. +- Reply rows keep the same row height as thread rows, since + `setUniformRowHeights(true)` is required for the tag strip's band arithmetic + (`src/mainwindow.cpp`). A conversation view would want tighter replies. + +**Approach: unspecified, and deliberately so.** Ask the user what to change +before proposing anything, exactly as item 20 required. The plausible directions +differ enormously in cost and are not interchangeable: + +1. **Span the columns for message rows.** Draw a reply as one free-form band + (sender, date, no grid) rather than as cells. `QTreeView::setFirstColumnSpanned` + does this per row without a second model. Cheapest real change, keeps + everything else built. +2. **Drop the `Re:` prefix and shrink what a reply shows.** XS on its own, and + worth trying before anything structural. +3. **Two different row shapes.** A delegate that paints a reply row entirely + itself, ignoring the columns. More control, and it fights `uniformRowHeights`. +4. **Abandon message rows in the list** and revisit the `<details>`-per-message + design in the message pane, which was offered at size S on 2026-08-08 and + declined in favour of this. Item 20's work would largely be reverted. + +**Constraint that shapes all of them.** The tag strip is why `ThreadListView` +exists, and its band arithmetic assumes a uniform row height and a known column +layout. Anything that varies row height or removes columns for one row kind has +to answer for the strip on thread rows, which must not change. + +**Do not start any of these from this description.** It says what is and why, +not what was wanted. + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering |