From f897153a1196f23fe0d82dc703d98df1363bf3fc Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 14 Aug 2026 18:41:10 +0200 Subject: docs: close item 36, narrow item 66 with a negative result Item 36's section moves to the closed file and its three traps go into CLAUDE.md, which is where they will be read: the worker is unreachable by findChild, rowCount on an unexpanded thread row is 0 by design, and currentThreadId reports intent rather than content. The claim that this class of bug cannot be reproduced in test_mainwindow is now false, so it is corrected rather than left standing beside its replacement. One in-test reference to item 36 as a permanent limitation is reworded: bare-window cases still have no worker, but that is now a choice per case rather than a property of the binary. Item 66 stays open with the simple case ruled out. The negative result sharpens this entry's own candidate rather than contradicting it: the test drives setCurrentIndex, which updates the selection model synchronously, while the suspect guard turns on a real click not having done so yet. Two cheaper conditions are named as still unexcluded. Co-Authored-By: Claude Opus 5 --- .../2026-08-03-post-0.1.0-usability-closed.md | 44 ++++++++++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 67 ++++++++-------------- 2 files changed, 67 insertions(+), 44 deletions(-) (limited to 'docs') diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index 221b3b2..ed88693 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md @@ -4893,3 +4893,47 @@ a keybinding and nothing is broken. **The consequence for item 36.** That item could not add fixture-backed tests while a malformed config could hang the constructor. It can now, which is why 84 was done first. + +## 36. `test_mainwindow` cannot reach the worker + +**Done 2026-08-14, unreleased.** See +`specs/2026-08-14-mainwindow-worker-fixture-design.md`. + +**Observed:** twice in one session (0.8.0), a defect could not be given a +regression test because `MainWindow` in tests had no notmuch database, so its +`NotmuchWorker` never emitted `threadLoaded`. One of those, the guard in +`onThreadLoaded()` against a queued load repainting a pane a multi-row selection +had just blanked, is now covered: the reproduction written here fails when that +handler is disabled. + +**Shipped as `WorkerBackedWindow` in `test_mainwindow.cpp`, opt-in per test, +with NO production change.** `wireWorker()` already built the worker from +`m_config.notmuchConfig()`, an ordinary config key, so a written +`qtmaildir.conf` pointing at the fixture exercises the shipping path. The entry +had read as bigger than it was since 2026-08-04 because it assumed a hook was +needed. + +**Three traps found while writing it, each of which had made an earlier version +of the test worthless. All three are worth knowing before touching this area.** + +The worker is **unreachable by `findChild`**. `wireWorker()` creates it +parentless and immediately moves it to its own thread, so it is not in the +window's object hierarchy. The tests wait with `QTRY_VERIFY_WITH_TIMEOUT` on +observable state instead, which is the better assertion anyway. + +**`rowCount()` on a thread row is 0 until the thread is expanded**, because +children are populated by the expansion. `hasChildren()` is the correct +pre-expansion question, falling back to `summary.totalCount > 1`, and it is also +what proves two messages threaded rather than arriving as two rows. An assertion +on `rowCount` fails against correct code. + +**`currentThreadId()` reports intent, not content.** It is assigned +synchronously in the selection handler at `src/mainwindow.cpp:2387`, before any +worker round-trip, so a test asserting on it passed with `onThreadLoaded()` +disabled entirely. `MessageView::showingPlaceholder()` is what the user sees. +The test asserts the pane IS blank before the click, so the assertion after it +means something. + +**Item 66 did not reproduce**, which is recorded in that item rather than +worked around here. The test was not widened until it failed; the negative +result and what it narrows to are in item 66's own entry. 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 9240ad6..2594890 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 @@ -100,7 +100,7 @@ taking that too literally. | 33 | Status bar messages never expire | feedback | S | **done** | | 34 | No overview of the Maildir itself | information | M | **done** | | 35 | No refresh of the thread list after a sync | workflow | M | **done** 2026-08-10; the list now follows a sync on its own | -| 36 | `test_mainwindow` cannot reach the worker | testing | S-M | open, specced 2026-08-14; see `specs/2026-08-14-mainwindow-worker-fixture-design.md`. No longer on demand: item 66 needs it. Smaller than it reads, `wireWorker()` already builds the worker from a config key | +| 36 | `test_mainwindow` cannot reach the worker | testing | S-M | **done** 2026-08-14, unreleased; see `specs/2026-08-14-mainwindow-worker-fixture-design.md`. `WorkerBackedWindow`, opt-in per test, no production change. Its first use ruled out the simple case of item 66 | | 37 | The worker stalls on a tag edit made during a background sync | correctness | S | **done** | | 38 | `test_mainwindow` fails when a real sync holds the lock | testing | XS | **done** | | 39 | Thread list cannot be sorted by clicking a column header | workflow | S | **dropped** 2026-08-10; the card list has no column headers to click, and 0.13.0 shipped a sort dropdown instead | @@ -130,7 +130,7 @@ taking that too literally. | 63 | No way to see sent mail, and no filter for it | workflow | M | **done** 2026-08-11; see `specs/2026-08-11-sent-mail-design.md` | | 64 | The Sync button carries a mailbox icon, not a refresh one | presentation | XS | **done** 2026-08-11 | | 65 | No full code review and optimization pass | correctness | ? | open, unspecified | -| 66 | Selecting a thread root leaves the message pane blank until a reply has been selected | defect | S | open; needs a reproduction before a fix | +| 66 | Selecting a thread root leaves the message pane blank until a reply has been selected | defect | S | open; the simple case is RULED OUT, measured 2026-08-14 with the item 36 fixture. Needs a condition nobody has named yet, see the section | | 67 | The placeholder pane counts unread, flagged and inbox, but not sent or drafts | information | XS | **done** 2026-08-11, shipped in 0.15.0 | | 68 | A forwarded subject gets no `passed` tag | workflow | S | open; no subject rule exists, measured 2026-08-11. Decision needed: display mark (XS) or write the flag (S, syncs out) | | 69 | `passed` and `replied` read as words where every other state is a glyph | presentation | S | **done** 2026-08-11, inside item 70 | @@ -214,42 +214,6 @@ literal, the descriptions are prose. **Verification:** run `lupdate` and read the generated `.ts`. A string that does not appear there is not translatable, whatever the source looks like. -## 36. `test_mainwindow` cannot reach the worker - -**Observed:** twice in one session (0.8.0), a defect could not be given a -regression test because `MainWindow` in tests has no notmuch database, so its -`NotmuchWorker` never emits `threadLoaded`. Both were verified by hand and by a -standalone model instead: - -- A queued `threadLoaded` repainting a pane that a multi-row selection had just - blanked. The fix guards `onThreadLoaded()`, and that guard is untested: the - test passes with it reverted. -- A local sync reporting itself as a background one. Staging it needs - `MailSync::isRunning()` true at one transition and false at the next, which - needs a configured sync command and a live child process. That was tried and - abandoned: it left a process running for the length of the suite and popped a - dialog on the user's screen. - -**The machinery already exists and is not the gap.** `tests/notmuchfixture.h` -builds a throwaway Maildir in a `QTemporaryDir`, runs `notmuch new` with -`NOTMUCH_CONFIG` scoped to the test process, and `addMessage()` takes -message-id, subject, from, date, body, unread and in-reply-to, so threads come -from real `In-Reply-To` chains. `test_notmuchworker` uses it. `test_mainwindow` -does not. - -**Explicitly rejected: a corpus built from the user's real mail.** Considered -and declined 2026-08-04. The global rule against personal data in fixtures -forbids it, git hooks enforce it, and this repository is public. Scrubbing is -also the fragile path: one surviving `In-Reply-To` or `References` header names a -real correspondent, and a scrubbed message no longer demonstrates what the real -one did. Where a real message exposes a parser bug, hand-write a minimal `.eml` -reproducing its **shape**, as `truncated.eml` and `hostile_filename.eml` already -do. - -**Specced 2026-08-14: read -`specs/2026-08-14-mainwindow-worker-fixture-design.md` rather than planning from -here.** - **No longer on demand.** The entry said to do this when a defect needs it, not before (user, 2026-08-04). Item 66 needs it now, so the deliverable is a RED reproduction of that defect, not fixture wiring on its own. Fixing 66 is @@ -263,10 +227,6 @@ writes a `qtmaildir.conf` pointing at the fixture gets a real worker through the shipping path with nothing in `src/` changed. Mind the `[general]` prefix trap when writing that file. -**Constraint:** every existing `test_mainwindow` case constructs a bare -`MainWindow` and must keep working. The fixture is opt-in per test, not a -suite-wide `initTestCase`, or every case pays for a `notmuch new`. - ## 40. No live filter over the current view **Observed (user, 2026-08-05):** "search in current view", spelled out as two @@ -360,8 +320,27 @@ load and `onSelectionChanged` only reloads when the row CHANGED kind of plausible cause the project has been burned by twice; instrument the running application and watch the order of `currentRowChanged`, `selectionChanged` and the worker reply for one first click on a cold view. -`test_mainwindow` cannot reproduce it: it has no worker and never fires -`threadLoaded`. + +**Ruled out 2026-08-14: the simple case does not reproduce.** Item 36 shipped +the fixture this entry said was missing, and +`selectingAThreadRootShowsItInTheMessagePane` in `test_mainwindow` now builds a +real database, runs a real query and selects an unexpanded thread root with a +real `threadLoaded` arriving. The pane renders. The test is mutation-confirmed +to fail when `onThreadLoaded()` is disabled, so it is a probe with teeth rather +than a green that proves nothing. + +**What that leaves, and it sharpens this entry's own candidate.** The test uses +`setCurrentIndex()`, which updates the selection model SYNCHRONOUSLY. A real +mouse click does not, and the guard at `src/mainwindow.cpp:1804` returns without +loading when the index is not yet selected. So the negative result is consistent +with the candidate above rather than against it: it says the defect is in the +CLICK path, not in the load path. Reproducing it needs `QTest::mouseClick` on +the viewport at the row's `visualRect`, not a programmatic selection. + +Two other conditions remain unexcluded and are cheaper to try than a click +harness: a thread of more than two messages, and a cold page cache, which item +74 measures as seconds on this machine and which is exactly the window in which +an ordering fault would show. **Constraints:** the mark-read guard at 1804 must survive whatever is done, it is item 35b's fix and removing it re-opens marking mail read without the user having -- cgit v1.2.3