From 029a50e2466f4f8858fc94ece17ffaf2841a8c1d Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 11:36:06 +0200 Subject: docs: record message rows, and the user's verdict on them CLAUDE.md described a QTableView over a table model, which has not been true since the view port. Updated with the traps the port produced, each of which shipped a plausible-looking broken build before being caught: - A tree numbers rows per parent, so nothing may be keyed on a row NUMBER. - drawBranches runs before the row's cells, so an expander on a content column is painted over by the delegate's background. - setRootIsDecorated(false) removes the style's HIT AREA along with its indicator, leaving a glyph that renders and does nothing. - isExpanded and setExpanded are keyed on column 0. - A reply's indent must beat the account chip's width, and visualRect reports the indent correctly even when nothing is visibly indented. - paintEvent runs after the cells, so a full-row fill erases their text. Also the notmuch ownership rule, which is a double-free if undone: messages reached through a thread are freed with it, so walkReplies holds them raw against this file's own RAII convention. Item 20 is marked built, not done, and item 53 records why. The user's verdict on the finished result was that the table view does not fit the use, said with every cue in and working. That is a design finding rather than a defect: the item shipped exactly what its four decisions specified, and all four were the user's own choices. Recording it as a defect would misattribute the cause; recording nothing would leave the next session building on a rejected design. Item 53 carries the cause verified in code rather than guessed. A message row fills the same five columns as a thread row (threadlistmodel.cpp:275-283 mirroring :428-431), so replies land on the same rigid column boundaries as the threads around them, and the eye reads columns before indentation or tint. The reference the user gave has no column rules through its reply rows at all, and that absence is the one thing three added cues cannot supply. --- CLAUDE.md | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 64 insertions(+), 9 deletions(-) (limited to 'CLAUDE.md') diff --git a/CLAUDE.md b/CLAUDE.md index 1ebb407..5e9ab63 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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 -- cgit v1.2.3