diff options
| -rw-r--r-- | CHANGELOG.md | 19 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 383 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 293 | ||||
| -rw-r--r-- | src/mainwindow.h | 105 | ||||
| -rw-r--r-- | src/messageview.cpp | 56 | ||||
| -rw-r--r-- | src/messageview.h | 39 | ||||
| -rw-r--r-- | src/threadlistmodel.cpp | 104 | ||||
| -rw-r--r-- | src/threadlistmodel.h | 14 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 1059 | ||||
| -rw-r--r-- | tests/test_threadlistmodel.cpp | 248 |
10 files changed, 2299 insertions, 21 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index a8f0a55..cb213d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,25 @@ point at which they are stable. ## [Unreleased] +### Changed + +- The thread list now follows a background sync on its own. New mail appears + where the sort puts it, threads that stopped matching leave, and threads whose + state changed repaint, with no keystroke. Previously the status bar asked you + to press Enter, because refreshing meant re-running the query, which cleared + the list, the selection and the message pane; the list is now reconciled + instead, so an expanded thread stays expanded, the selection stays put and the + message being read stays on screen. The "Background sync completed" message is + gone: a refresh that changes nothing should be invisible. + +### Added + +- A notice above the message when the thread being read no longer matches the + current query, with a button that brings it back. Reading a thread to the end + of an Unread view now removes it from the list as it should, and this is the + way back to it: the whole thread is listed, and the message that was on screen + is re-selected, so returning to reply four of eight lands on reply four. + ## [0.13.0] - 2026-08-10 The thread list stops being a table. Each thread is a card of three lines, 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 50d6c42..dccea2d 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 @@ -91,11 +91,11 @@ taking that too literally. | 32 | Esc does not blank the right pane | workflow | XS | **done** | | 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 | open | +| 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 | open, on demand | | 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 | open | +| 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 | | 40 | No live filter over the current view | workflow | M | open | | 41 | A message whose HTML body carries a `Content-Id` renders blank | correctness | S | **done** | | 42 | "Syncing..." says nothing about what is being synced | feedback | S | **done** | @@ -118,6 +118,8 @@ taking that too literally. | 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 | **done**; already fixed by 5487d58, see below | | 61 | `test_mainwindow` fails intermittently, about 1 run in 20 | testing | S | open; predates the card list, reproduced on f72dba9 | +| 62 | No config option for the date format on a card | presentation | XS | open | +| 63 | No way to see sent mail, and no filter for it | workflow | S | open | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -2175,6 +2177,292 @@ from a current one, so the plumbing for a second concurrent query exists. - Scroll position likewise. - A refresh must not re-trigger mark-read for the thread already on screen. +### Refined by the user, 2026-08-10: new mail never appears at all + +The complaint is sharper than "the list is stale". Read every message in an +Unread view and the list empties. The cron sync then runs, `notmuch new` indexes +new mail, and **the new messages do not appear**, even though the view is empty, +nothing is selected, no thread is open and the undo stack has nothing in it. The +only way to see them is to re-run the query by hand. + +**Cause, verified in code, and it is the deliberate choice above meeting its +worst case.** The app does observe the cron sync: +`MainWindow::onExternalSyncStateChanged()` (`src/mainwindow.cpp:2140`) is driven +by `SyncMonitor`'s lock polling and reaches `State::Idle` when the run ends. At +`src/mainwindow.cpp:2180-2192` it deliberately shows "Background sync completed. +Press Enter in the query bar to refresh." and calls nothing. The comment there +gives the reason: `runCurrentQuery()` clears the undo stack, the selection and +the message pane, which is hostile to fire six times an hour under a reader. + +**Every one of those costs is zero in the case the user hit.** There is nothing +to clear: no undo entries, no selection, no open thread, and the scroll position +of an empty list is meaningless. So the guard is protecting state that does not +exist, and the result is an empty Unread view sitting in front of unread mail. + +**This makes the item shippable in two stages, and the first is XS.** Refresh on +external sync completion when the refresh is provably free: the model is empty +**or** the undo stack is empty and nothing is selected. Fall back to the current +status-bar message otherwise. That fixes the reported case immediately without +needing the reconciling refresh, which stays as the M-sized second stage for the +case where the user does have state worth keeping. + +**Constraint on the cheap stage.** "Nothing selected" must be read from the +selection model, not from `currentRowChanged` state, per `CLAUDE.md`. And the +condition has to be re-checked at the moment the sync ends rather than when it +started, since the user may have selected a row during the run. + +### Outcome (done, 2026-08-10) + +**35a was superseded before it shipped, by the user's own answer to it.** Asked +whether new mail would accumulate into a POPULATED view, the answer was no, and +the requirement was restated in full: "changes should be applied automatically, +without the user needing to refresh the view, whether it's empty or populated", +new mail appearing at the top as it is fetched, and the message being read not +disappearing. So the conditional refresh was removed rather than kept beside the +reconciling one, and `externalRefreshIsFree()` no longer exists. The "press +Enter in the query bar to refresh" message is gone with it: a refresh that +changes nothing is invisible, and one that brings mail announces itself by the +mail appearing. + +**The undo constraint this item was sized around turned out not to exist.** The +text above calls keeping the undo stack "the hard part" and the reason this is +M-sized. It is not hard, because no undo entry was ever keyed on a row: +`ThreadTagCommand` stores THREAD ids and `MessageTagCommand` stores MESSAGE ids +(`src/mainwindow.h`), both re-sending through `sendThreadTagChange()` / +`sendMessageTagChange()`, and `ThreadListModel::applyTagChange()` looks its +target up by id and does nothing when the row is absent. An entry therefore +already survives its rows leaving the view. `runCurrentQuery()` clears the stack +because a query the USER typed means "show me something else", not because the +entries would corrupt anything. + +**What was built.** + +- `ThreadListModel::reconcile()` diffs a result against the current rows by + thread id: arrivals inserted, departures removed, survivors keeping their row, + their persistent index and their loaded replies. Order comes from the result, + never from a rule of the model's own, so the sort the user selected is + respected: newest-first puts new mail at the top, oldest-first at the bottom. +- `MainWindow::refreshCurrentQuery()` re-runs `m_lastQuery` under its own + generation, accumulates every batch, and reconciles ONCE at the end. Batch by + batch would be wrong in a way that looks right: reconcile decides removals + from what the result lacks, so the first batch would delete every row after it + and the next would put some back. +- `onExternalSyncStateChanged()` calls it unconditionally on `Idle`. +- A stale-thread notice in `MessageView`, shaped like the remote-content bar as + the user asked, with recovery that runs `thread:<id>`, expands it and + re-selects the message that was on screen. + +**Four defects found by the tests, three of them real.** + +1. **The first test crashed the constructor.** `SyncMonitor::start()` polls + SYNCHRONOUSLY (`src/syncmonitor.cpp:52`), so on an idle lock file it emits + `stateChanged(Idle)` from inside `buildUi()`, while the view, the model and + the worker are all still null. The old code survived only because reporting + to the status bar touches nothing built later; the first handler to + dereference a widget segfaults before the window exists. +2. **A downward-move branch that could never run.** `reconcile()` walks the + result front to back, so rows ahead of the target are already final and a + misplaced survivor is always pulled FORWARD. The branch was written with the + usual `beginMoveRows` +1 adjustment and two mutation tests passed against it + being wrong, which is the signal that a probe is not measuring what it + claims. Deleted, with `Q_ASSERT(row > target)` recording the invariant. +3. **A user query hijacked by a pending recovery.** Recovery spans two queued + round-trips, so a query typed in the middle of one found its target in the + new result and moved the selection there. `runCurrentQuery()` now abandons a + pending recovery, and `recoverStaleThread()` sets its target after calling + it. +4. **The stale notice never fired for the reader deepest in a thread.** + Selecting a message row CLEARS `m_currentThreadId` and sets + `m_currentMessageId` instead, so a notice keyed on the thread alone was + silent for exactly the case the user described, reply four of eight. The + window remembers the message's thread separately; + `ThreadListModel::threadIdForMessage()` cannot help, since it searches the + rows and by then the thread has left them. + +**A fifth defect, found by the user in hand testing rather than by any test.** +The notice outlived the message it describes: running a new query blanked the +pane and left the bar above it, still naming the previous thread, with a button +offering to recover a thread the user had deliberately navigated away from. The +bar belongs to the rendered message exactly as the remote-content bar does, and +`MessageView::clear()` already hides that one for this precise reason; the new +bar simply was not added beside it. Fixed there, which covers all six paths that +blank the pane at once, rather than at the query path where it was noticed. + +Worth recording because the tests could not have caught it as written: every +one of them asserted that the notice APPEARS, and none that it goes away. A +feature's off-switch needs its own test, and "it shows up when it should" passes +identically whether or not it ever stops showing up. + +**A sixth defect, also found by the user in hand testing, and the same mistake +in a different place.** The status bar sat on "Background sync running..." with +no sync running. That string is written straight to the label when the lock +appears, and the "Background sync completed" message on the way out was the only +thing that ever replaced it; removing that message to make the refresh silent +left the claim standing indefinitely. + +The rule this establishes is worth more than the fix: **silent means saying +nothing NEW, not leaving a stale claim on screen.** The Idle branch now retires +its own running message and nothing else, tracked by a flag rather than by +matching the text, so it cannot overwrite a selection count or a tag result the +user is reading. Both directions are pinned by mutation: never retiring +reproduces the reported bug, and always writing the default stamps over the +selection message. + +Note the shape shared with the fifth defect above. Both are a piece of UI state +that outlived the thing it described, and in both cases the tests asserted only +that the state APPEARS. An "it goes away" test is a separate test. + +**A seventh and eighth defect, one report, and the worse of the two mutates +mail.** The user came back to the window from another desktop and found the new +message the refresh had brought in already OPEN in the pane, with the stale +notice above it still naming the four-message thread they had been reading. + +- **Nothing in `MainWindow` selected it.** `QTreeView` gives itself a current + index when it takes FOCUS with none set, and current is what drives loading. + Probed rather than assumed, because the obvious hypothesis is wrong: inserting + rows into an empty view does NOT set current, focusing the view does, which is + exactly why the report came with "as I go back to the window from another + desktop" attached. Before item 35b this was unreachable, since a populated + list always had a current row; a refresh dropping mail into a view the user + read empty created the state. The consequence is not cosmetic: opening a + message arms the mark-read timer, so a cron sync plus a window switch marked + mail read that nobody looked at. `onThreadSelected()` now requires the row to + be SELECTED, which every real route (click, arrow key, `selectRowAt`) does and + Qt's housekeeping does not. +- **The notice was correct when raised and became a lie underneath.** It named + the thread that was rendered; the auto-open then replaced the pane without + touching the bar. `MessageView::clear()` retires it, but selecting a row + RE-RENDERS rather than blanking, so that path never ran. Retired in + `onThreadSelected()` as well. + +The first of these was reported as one bug and is two, and only the second was +visible on screen. Worth remembering that "the wrong thing is displayed" and +"the wrong thing happened to the mail" can arrive in the same sentence. + +**A ninth defect: recovery brought the thread back collapsed and blank.** The +user reported it as minor and livable, and it was three faults stacked, each of +which alone would have produced roughly the symptom they saw. + +- **The notice threw away a message id it had.** A thread ROOT sets BOTH + `m_currentThreadId` and `m_currentMessageId`, because the root card is the + thread's first message and the pane renders exactly that message. The notice + read the message id only when the thread id was empty, treating it as the + message-row case, so opening a thread the ordinary way lost it and recovery + had nothing to reopen. +- **Recovery never expanded.** It selected the row and returned, so the + conversation the user asked to get back to was not on screen. It expands + first now, in every branch, which is also what asks the worker for the + replies. +- **A freshly queried root does not know its own first message either.** + `MessageIdRole` on a thread row returns `first.messageId`, which is empty + until the tree loads, so the root check could not match on the pass that + matters and the code fell through to `rowCount(thread) == 0` and returned, + selecting nothing. Recovery now selects the thread PROVISIONALLY on that + pass, without clearing the target, and refines to the exact reply when the + replies arrive. + +**One change here is not demonstrated and is recorded as such.** Recovery also +moved from `setCurrentIndex()` to `selectRowAt()`, on the reasoning that +`onThreadSelected()` ignores an unselected current index since the auto-open +fix. A mutation reverting it passes the whole suite: under +`ExtendedSelection`, `setCurrentIndex()` selects as a side effect, so the two +are indistinguishable here. It is kept as the honest expression of the intent, +not as a fix, and nothing should be claimed for it. + +**A tenth defect, and the only one in this item that six rounds of reasoning +failed to find: a dangling reference across a signal.** Recovery brought the +thread back collapsed with a blank pane. The user reported it three times, each +time after a fix that was aimed at the wrong thing. + +`MessageView` emitted `staleThreadRecoveryRequested(m_staleThreadId, +m_staleMessageId)`, passing its own members. The connection is direct, so +`MainWindow::recoverStaleThread()` received REFERENCES to those members. It then +called `runCurrentQuery()`, which blanks the pane, which calls `setStaleThread()` +and assigns to exactly those members. From that line onward the slot's own +parameters read as empty, so `m_recoverThreadId = threadId` stored an empty +string and `applyPendingRecovery()` returned at its first line, forever. The +thread was re-queried and expanded correctly, which is why the symptom looked +like a layout or expansion problem rather than a lifetime one. + +**Every existing recovery test passed against it, and could not have failed.** +They all reach the slot through `QMetaObject::invokeMethod`, which COPIES its +arguments; the reference never dangles under a test. The defect needed the real +button and the real signal, which is what the new test uses. + +**Six wrong mechanisms were proposed and rejected before the log named this +one**, each one plausible and each one disproved by a probe rather than by +argument: `QTreeView::expanded` not re-firing, expansion collapsing when +children arrive, the multi-row guard blanking the pane, `selectRowAt` not +clearing the previous selection, a stale `m_refreshGeneration` swallowing the +result, and `onQueryFinished` not running at all. The thing that ended it was +instrumenting the running application and reading `RECOVER target set to ` with +nothing after the `to`, which no amount of reading the code had produced. + +**The rule worth keeping: a Qt signal argument is a reference until something +copies it.** Emitting a member across a direct connection to a slot that can +reach back and modify that member is a use-after-write, and it presents as the +value being "wrong" rather than as a crash. Copy at the emit site when the slot +can plausibly re-enter the emitter. + +**A trap the recovery had to handle.** `setThreadMessages()` drops the depth-0 +message because the root card IS the thread's first message, so a reader +recovering from message one must land on the ROOT row. Looking for it among the +children finds nothing and leaves the selection nowhere. + +**Verification.** 105 tests in `test_mainwindow`, 56 in `test_threadlistmodel`, +17/17 binaries. Every reconcile test was mutation-checked: an unconditional +refresh, a skipped move, corrupted index bookkeeping (which trips +`QAbstractItemModelTester` fatally), a per-batch reconcile, a notice that never +fires and one that always fires are each caught by a named test. The +abandoned-recovery test initially passed for the wrong reason, because its +recovery never reached its target, and was rewritten until it failed against the +missing guard. + +**One measurement worth carrying to item 61.** During this work +`test_mainwindow` failed three runs in a row on the pair recorded there +(`anActionOnAMessageRowTagsThatMessageNotTheThread`, +`aSuccessfulCronSyncDrainsTheEditedAccounts`), then passed twelve consecutive +runs unchanged. The recorded rate is about one in twenty; a cluster of three +consecutive failures does not fit an independent one-in-twenty event and +suggests the trigger is a machine state that persists across runs rather than a +per-run race. + +### Outcome (35a, superseded by the above) + +`MainWindow::externalRefreshIsFree()` gates the `State::Idle` branch of +`onExternalSyncStateChanged()`: it refreshes when the undo stack is empty, the +selection model reports no selection, and the model holds no rows, and prints +the existing "press Enter" message otherwise. The M-sized reconciling refresh +(35b) is untouched and still open. + +**The test crashed the constructor, and the crash was real.** +`SyncMonitor::start()` polls SYNCHRONOUSLY (`src/syncmonitor.cpp:52`), so on a +machine whose lock file is idle it emits `stateChanged(Idle)` from inside +`buildUi()` (`src/mainwindow.cpp:535`), while `m_threadView` and `m_model` are +still null. The old code survived that only because reporting to the status bar +touches no widget built later; the first handler to dereference a view segfaults +before the window exists. `externalRefreshIsFree()` returns false on a null view +or model, which is also correct on the merits: the startup query has not run at +that point, so there is nothing to refresh. + +**The undo check is not redundant with the row check**, and that is asserted +rather than argued. Removing it alone leaves +`aCronSyncDoesNotRefreshOverPendingUndo` failing, because an empty model with +live undo entries is reachable by tagging the last thread out of the current +view. + +**Both guard tests passed before the fix existed**, since nothing refreshed at +all, so each was verified by mutation: an unconditional `return true` fails both, +and dropping the undo check fails the undo one. + +**One existing test changed, and the change is a narrowing.** +`aSkippedLocalSyncStillReportsTheOtherRunFinishing` asserted the words +"Background" in the status bar, and its fixture is an empty list with nothing +selected, so it now takes the refresh branch and the words never appear. Its +actual subject is that a handed-back lock is attributed to the other run rather +than swallowed, so it asserts the query generation instead. Pinning the wording +there would fail again the next time this decision is revisited. + ## 36. `test_mainwindow` cannot reach the worker **Observed:** twice in one session (0.8.0), a defect could not be given a @@ -2406,6 +2694,15 @@ line and `lockHeldIn()` correctly finds no lock in it. ## 39. Thread list cannot be sorted by clicking a column header +**Dropped 2026-08-10 (user).** The item asked for a column header to click and +there is no longer one to click: the card list (items 20 and 53) collapsed the +five columns into a single column of cards, and the header is gone with them. +0.13.0 shipped a sort dropdown in the query row, which is the same capability +reached a different way, so the complaint behind this item is answered and the +mechanism it proposed is unbuildable. The analysis below is kept because its +constraints outlived it: the batched-append problem and the sort-on-timestamp +rule apply to any future sort, including the dropdown's. + **Observed (user, 2026-08-05):** "left pane columns order by clicking on the column header." @@ -3751,6 +4048,88 @@ initially mistaken for a regression that change had introduced. **Size: S**, most of it in reproducing reliably rather than in the fix. +## 62. No config option for the date format on a card + +**Observed (user, from the notes):** "option in config file for date format". + +**Cause, verified in code.** `CardLayout::formatDate()` +(`src/cardlayout.cpp:24-30`) is a single unconditional line: +`QLocale::system().toString(date, QLocale::ShortFormat)`. It reads no setting, +takes no format argument, and is the only date formatter on a card +(`src/carddelegate.cpp:176` is its only caller besides +`CardLayout`'s own `widestDate`). `Config` parses no date key: the `[general]` +keys it reads are `notmuch_config`, `startup_query`, `message_zoom`, +`completion_on_focus`, `toolbar_icon_size`, `sync_on_exit` and +`mark_read_delay_ms` (`src/config.cpp:78-184`). So there is nothing to +configure, not a setting that is being ignored. + +The current behaviour is a deliberate choice rather than an oversight, and the +comment at `src/cardlayout.cpp:26-28` says why: the system short format is what +every other application on the desktop shows, and a mail client that disagrees +looks wrong. This item is about giving the user an override, not about +replacing that default. + +**Approach.** A `[general] date_format` key, empty by default meaning "the +system short format", otherwise a `QDateTime::toString()` pattern. +`CardLayout::formatDate()` takes the format as a parameter rather than reading +`Config` itself, keeping the struct free of dependencies the way it already is, +and `CardDelegate` passes it down. + +**Constraints.** + +- **`CardLayout::widestDate()` reserves the date column's width and must agree + with whatever the format produces**, or a long custom pattern is elided or + overlaps. It currently computes the widest string the short format can return; + with a custom pattern it has to measure that pattern instead. +- An unparseable or absurd pattern must not blank the date. `toString()` with a + pattern containing no field returns the pattern itself verbatim, so validate + in `Config` and fall back to the system format with a problem reported, the + way `message_zoom` and `toolbar_icon_size` already do. +- Document all three states in the README's `[general]` block: absent, empty, + and a pattern. + +**Size: XS.** One key, one parameter, one width calculation. + +## 63. No way to see sent mail, and no filter for it + +**Observed (user, from the notes):** "Sent mail filter". + +**Cause, verified in code, and it is not one missing feature but two.** + +- **Nothing in the codebase knows what "sent" means.** `Account` carries + `name`, `address`, `maildir`, `drafts`, `label`, `channel` and `color` + (`src/config.cpp:254-270`); there is no `sent` field, and no query anywhere + composes one. `Account::scopedQuery()` (`src/config.cpp:42-45`) scopes by + `path:"<maildir>/**"`, which covers a sent folder only in the sense that it + covers everything in the account. +- **The saved-query mechanism could express it today and nothing ships one.** + `[queries]` is read wholesale from `childKeys()` (`src/config.cpp:291-297`), + so a user can already write `Sent = tag:sent` by hand. The README's example + block (`README.md:178-181`) offers Inbox, Unread and Important and no Sent, so + nothing points the user at it. + +Which of the two this item is depends on a decision the notes do not make: +whether "sent" is a **notmuch tag** the user's own filters apply, or a +**maildir path** per account. If it is a tag, this is a documentation and +defaults change and it is XS. If it is a path, `Account` needs a `sent` key +beside `drafts`, and the query has to be composed per account, which is where +the S comes from. + +**Approach, pending that decision.** Ask the user first which their setup +already produces. Their mail is filtered outside qtmaildir (`assets/mailsync.sh` +is `mbsync` plus `notmuch new`), so the answer is a property of their existing +filters, not something to design here. + +**Constraints.** + +- **Do not invent a tag qtmaildir applies itself.** v1 is read-and-organize; + nothing here sends mail, so nothing here can know a message was sent except by + where it landed or what tagged it. +- If it becomes a per-account key, it composes with `scopedQuery()` and must not + bypass it, or a Sent view in one account shows another account's mail. + +**Size: S**, and XS if the answer is "it is a tag". + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index b7efec5..bc5fa92 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -626,6 +626,8 @@ void MainWindow::buildUi() this, [this](const QString &text) { m_statusLabel->setText(text); }); connect(m_messageView, &MessageView::queryRequested, this, &MainWindow::onPlaceholderQueryRequested); + connect(m_messageView, &MessageView::staleThreadRecoveryRequested, + this, &MainWindow::recoverStaleThread); m_splitter = new QSplitter(Qt::Horizontal, central); m_splitter->addWidget(m_threadView); @@ -867,6 +869,7 @@ void MainWindow::registerActions() // CLAUDE.md. m_currentThreadId.clear(); m_currentMessageId.clear(); + m_currentMessageThreadId.clear(); m_messageView->clear(); showPlaceholderPane(); m_markReadTimer->stop(); @@ -901,6 +904,7 @@ void MainWindow::registerActions() m_currentThreadId.clear(); m_currentMessageId.clear(); + m_currentMessageThreadId.clear(); m_messageView->clear(); showPlaceholderPane(); m_markReadTimer->stop(); @@ -1480,6 +1484,15 @@ void MainWindow::runCurrentQuery() // Kept so loadThread() can work out which messages of a thread matched. m_lastQuery = query; + // A query the user ran abandons any recovery still in flight. Recovery + // spans two round-trips, so a query typed in the middle of one would + // otherwise have its result hijacked: the pending selection finds its + // thread in a result the user asked for something else from, and the view + // jumps. recoverStaleThread() sets the target AFTER calling this, so its + // own query does not clear it. + m_recoverThreadId.clear(); + m_recoverMessageId.clear(); + ++m_generation; m_model->clear(); m_messageView->clear(); @@ -1513,6 +1526,17 @@ void MainWindow::onThreadsReady(const QVector<ThreadSummary> &threads, { if (generation != m_generation) return; // Superseded by a newer query. + + // A refresh accumulates instead of appending. Its batches must not reach + // the model one at a time: reconcile() decides what to REMOVE from what the + // result does not contain, so applying the first batch alone would delete + // every row after it, then the next batch would put some back. The list + // would churn and every expanded thread would collapse. + if (generation == m_refreshGeneration) { + m_refreshThreads.append(threads); + return; + } + m_model->appendBatch(threads); } @@ -1520,6 +1544,30 @@ void MainWindow::onQueryFinished(int total, quint64 generation) { if (generation != m_generation) return; + + // The refresh's result is complete only now, so this is where it lands. + // One reconcile for the whole set, not one per batch. + if (generation == m_refreshGeneration) { + m_refreshGeneration = 0; + m_model->reconcile(m_refreshThreads); + m_refreshThreads.clear(); + + // The count in the status bar describes the current view and has just + // changed, but a refresh is meant to be silent, so it updates the + // FALLBACK text without stamping over whatever the bar is showing. + m_defaultStatus = tr("%n thread(s)", "", total); + + // A refresh leaves the view complete exactly as a query does: every + // matching row is present, so view-wide actions stay honest. + m_queryComplete = true; + updateViewWideActions(); + + // The open thread may have stopped matching, which the user has to be + // told about: the pane keeps rendering it while the list no longer + // offers it anywhere. + updateStaleThreadNotice(); + return; + } // The query's own result is what the bar says when nothing more pressing // is happening, so a transient message falls back to it rather than to // nothing. @@ -1530,6 +1578,11 @@ void MainWindow::onQueryFinished(int total, quint64 generation) // a thing that can honestly be acted on. m_queryComplete = true; updateViewWideActions(); + + // A recovery's own thread:<id> query landing. The rows exist now, so the + // thread can be expanded; the message inside it is selected once its + // replies arrive. + applyPendingRecovery(); } void MainWindow::updateViewWideActions() @@ -1692,6 +1745,7 @@ void MainWindow::onSelectionChanged() m_markReadThreadId.clear(); m_currentThreadId.clear(); m_currentMessageId.clear(); + m_currentMessageThreadId.clear(); m_messageView->clear(); showPlaceholderPane(); } @@ -1702,6 +1756,29 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, if (!current.isValid()) return; + // A current index the user did not put there. QTreeView gives itself one + // when it takes FOCUS with none set (verified against Qt 6.11: inserting + // rows does not do it, focusing the view does), and it sets current WITHOUT + // selecting. Before item 35b nothing could reach that state, because a + // populated list always had a current row; now a refresh can drop mail into + // a view the user read empty, and coming back to the window from another + // desktop would open the new message and mark it read two seconds later + // without them ever having looked at it. + // + // Every real route here (a click, an arrow key, selectRowAt) selects the + // row as well, so requiring a selection separates the user's intent from + // Qt's housekeeping without weakening any of them. + if (!m_threadView->selectionModel()->isSelected(current)) + return; + + // The notice belongs to whatever the pane is showing, and it is about to + // show something else. Retired here rather than only in MessageView::clear() + // because selecting a row RE-RENDERS the pane instead of blanking it, so + // the bar would otherwise sit over a message it does not describe. That is + // the second half of the reported defect: the pane had moved on and the + // notice had not. + m_messageView->setStaleThread(QString(), QString()); + // A selection spanning more than one row is aimed at a bulk action, not at // reading. current follows the keyboard cursor as the selection extends, so // without this every row swept through would be rendered and, worse, @@ -1724,6 +1801,7 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, m_markReadThreadId.clear(); m_currentThreadId.clear(); m_currentMessageId.clear(); + m_currentMessageThreadId.clear(); m_messageView->clear(); showPlaceholderPane(); return; @@ -1747,6 +1825,9 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, m_currentThreadId.clear(); m_currentMessageId = node.messageId; + // Remembered for the stale notice: the pane shows one message, but the + // thread it came from is what the refreshed list is checked against. + m_currentMessageThreadId = node.threadId; m_messageView->setTags(node.tags); QMetaObject::invokeMethod(m_worker, "loadMessage", Qt::QueuedConnection, Q_ARG(QString, node.messageId), @@ -1780,6 +1861,7 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, } m_currentMessageId.clear(); + m_currentMessageThreadId.clear(); QMetaObject::invokeMethod(m_worker, "loadThread", Qt::QueuedConnection, Q_ARG(QString, m_currentThreadId), Q_ARG(QString, m_lastQuery), @@ -1836,6 +1918,10 @@ void MainWindow::onThreadTreeLoaded(const QVector<MessageNode> &nodes, // expansions can be in flight at once, and pairing them by order would // attach one thread's replies to the other. m_model->setThreadMessages(nodes.first().threadId, nodes); + + // A stale-thread recovery waits for exactly this: the message it wants to + // select does not exist as a row until the replies land. + applyPendingRecovery(); } void MainWindow::onThreadLoaded(const QVector<MessageRef> &messages, @@ -2137,6 +2223,169 @@ void MainWindow::onTagsApplied(const TagChange &change) } } +void MainWindow::refreshCurrentQuery() +{ + // The null guard is not defensive padding, it is a reachable path found by + // this item's own test crashing the constructor. SyncMonitor::start() polls + // SYNCHRONOUSLY (src/syncmonitor.cpp:52), so a machine whose lock file is + // idle at that moment emits stateChanged(Idle) from inside buildUi(), while + // m_model and the worker are still null. Nothing to refresh at that point + // anyway: the startup query has not run. + if (!m_model || !m_worker) + return; + + // m_lastQuery, not the text in the query bar: the bar holds whatever the + // user has typed since, which may be a query they never ran. Refreshing to + // that would execute a search they did not ask for. + if (m_lastQuery.isEmpty()) + return; + + // Nothing is cleared. No m_model->clear(), no m_undoStack.clear(), no + // m_messageView->clear(): that list is exactly what runCurrentQuery() + // destroys and what makes it unusable on a cron timer. + m_refreshGeneration = ++m_generation; + m_refreshThreads.clear(); + + const auto sort = m_sortOrder->currentIndex() == 1 + ? NotmuchWorker::OldestFirst + : NotmuchWorker::NewestFirst; + QMetaObject::invokeMethod(m_worker, "runQuery", Qt::QueuedConnection, + Q_ARG(QString, m_lastQuery), + Q_ARG(quint64, m_refreshGeneration), + Q_ARG(NotmuchWorker::SortOrder, sort)); +} + +void MainWindow::updateStaleThreadNotice() +{ + // Which thread the pane is showing depends on what was selected: a thread + // row sets m_currentThreadId, a message row clears it and sets + // m_currentMessageId instead, so the message case has to be resolved back + // to its thread. Reading only m_currentThreadId would leave a reader who is + // three replies deep with no notice at all, which is the commonest way to + // be deep in a thread in the first place. + // The message id is carried whenever there IS one, whichever row kind put + // it there. A thread ROOT sets both: the root card is the thread's first + // message and the pane renders that message alone, so treating the message + // id as the message-row case only threw it away for the commonest way to + // open a thread, and recovery then had nothing to reopen. + QString threadId = m_currentThreadId; + const QString messageId = m_currentMessageId; + if (threadId.isEmpty()) + threadId = m_currentMessageThreadId; + + if (threadId.isEmpty()) { + m_messageView->setStaleThread(QString(), QString()); + return; + } + + // Present means matching: the model holds exactly the query's result after + // a reconcile. + for (int row = 0; row < m_model->rowCount(QModelIndex()); ++row) { + if (m_model->threadAt(row).threadId == threadId) { + m_messageView->setStaleThread(QString(), QString()); + return; + } + } + + m_messageView->setStaleThread(threadId, messageId); +} + +void MainWindow::recoverStaleThread(const QString &threadId, + const QString &messageId) +{ + if (threadId.isEmpty()) + return; + + // thread:<id> lists the WHOLE conversation rather than the single message, + // which is what the user asked for: eight messages, with the fourth + // selected, matching what the pane already shows. + m_queryEdit->setText(QStringLiteral("thread:%1").arg(threadId)); + runCurrentQuery(); + + // Set AFTER the query, which clears any pending recovery: this one is the + // query's own reason for running and must survive it. + // + // Remembered across the two queued round-trips this takes: the query has to + // come back before the thread can be expanded, and the expansion before the + // message row exists to select. + m_recoverThreadId = threadId; + m_recoverMessageId = messageId; +} + +void MainWindow::applyPendingRecovery() +{ + if (m_recoverThreadId.isEmpty()) + return; + + for (int row = 0; row < m_model->rowCount(QModelIndex()); ++row) { + const QModelIndex thread = m_model->index(row, 0, QModelIndex()); + if (m_model->threadAt(row).threadId != m_recoverThreadId) + continue; + + // Expanded in every case, and FIRST. The user was reading a + // conversation, so bringing it back collapsed hides the thing they + // asked to get back to, whether their message was the root or a reply. + // Expanding is also what asks the worker for the replies, so it has to + // happen before any attempt to find one. + m_threadView->expand(thread); + + // The thread's first message IS the root card rather than a child row: + // setThreadMessages drops depth 0 because the root stands for it, so + // looking for it among the children finds nothing and the selection + // would silently land nowhere. + // + // selectRowAt(), not setCurrentIndex(): a current index without a + // selection is what QTreeView sets by itself on focus, and + // onThreadSelected() deliberately ignores that, so pointing at the row + // renders nothing and leaves the pane blank. + if (m_recoverMessageId.isEmpty() + || m_model->data(thread, ThreadListModel::MessageIdRole).toString() + == m_recoverMessageId) { + selectRowAt(thread); + m_recoverThreadId.clear(); + m_recoverMessageId.clear(); + return; + } + + // A reply cannot be selected until the replies exist. The expand above + // asked for them, and this runs again when they arrive. + // + // The thread is selected NOW rather than waiting, because a freshly + // queried row does not know its own first message either: the root's + // MessageIdRole is empty until the tree loads + // (`src/threadlistmodel.cpp`), so the root check above cannot match yet + // and returning here would leave the user looking at a collapsed thread + // and a blank pane until the replies happen to arrive. Selecting the + // thread renders its first message immediately, which is the right + // answer outright when that is what they were reading, and is refined + // to the correct reply on the next pass when it is not. + // + // The target is deliberately NOT cleared: this pass is provisional. + if (m_model->rowCount(thread) == 0) { + selectRowAt(thread); + return; + } + + for (int child = 0; child < m_model->rowCount(thread); ++child) { + const QModelIndex reply = m_model->index(child, 0, thread); + if (m_model->messageAt(reply).messageId != m_recoverMessageId) + continue; + selectRowAt(reply); + m_recoverThreadId.clear(); + m_recoverMessageId.clear(); + return; + } + + // The thread came back without the message: it was deleted, or moved + // between accounts. Land on the thread rather than leaving the user + // with nothing selected. + selectRowAt(thread); + m_recoverThreadId.clear(); + m_recoverMessageId.clear(); + return; + } +} + void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) { if (state == SyncMonitor::State::Running) { @@ -2154,6 +2403,7 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) m_externalSyncBusy = true; updateSyncControls(); m_statusLabel->setText(tr("Background sync running...")); + m_announcedExternalSync = true; return; } @@ -2177,19 +2427,40 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) m_externalSyncBusy = false; updateSyncControls(); - // Deliberately reports rather than refreshes. runCurrentQuery() clears the - // undo stack, the selection and the message pane, which is right for a - // query the user typed and hostile for one fired by a cron timer: with a - // sync every ten minutes it would discard undo history and close the thread - // being read, up to six times an hour, with no action from the user. + // Refreshes, unconditionally, and says nothing about it. + // + // 0.8.0 refused to refresh here because runCurrentQuery() clears the undo + // stack, the selection and the message pane, which is right for a query the + // user typed and hostile for one fired by a cron timer. The status bar + // asked the user to press Enter instead. That made the list quietly stale: + // new mail indexed by cron never appeared, and an Unread view read to the + // end stayed empty in front of it. + // + // The answer is not to weigh the cost, it is to remove it. + // refreshCurrentQuery() reconciles the result into the model instead of + // resetting it, so a surviving thread keeps its row, its expansion and its + // selection, and the message being read stays on screen. Nothing has to be + // preserved by declining to run. // - // Unknown is not worth reporting either. It means the lock table could not - // be read, so nothing was observed, and "sync finished" would be a claim - // this cannot support. + // No status message: a refresh that changes nothing must be invisible, and + // one that adds mail is announced by the mail appearing. Six "sync + // completed" messages an hour are noise reporting the expected. + // + // Unknown is not refreshed. It means the lock table could not be read, so + // no sync was observed, and refreshing on it would re-query on every failed + // poll rather than after a sync. + // Retire our own running message, and only that one. The refresh below says + // nothing, which is right for a sync that changed nothing, but "says + // nothing" must not mean "leaves 'Background sync running...' on screen + // after it stopped". Anything else in the bar belongs to the user (a + // selection count, a tag result) and is left alone. + if (m_announcedExternalSync) { + m_announcedExternalSync = false; + m_statusLabel->setText(m_defaultStatus); + } + if (state == SyncMonitor::State::Idle) { - showTransientStatus( - tr("Background sync completed. Press Enter in the query bar to " - "refresh.")); + refreshCurrentQuery(); // Item 54. A cron sync carries the edits to the mail store exactly as a // local one does, so the count it cleared has to be cleared here too. diff --git a/src/mainwindow.h b/src/mainwindow.h index ccac5ad..a7ea5c3 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -155,6 +155,16 @@ public: /// reopened, so a test standing in for the worker needs the current value. quint64 statsGenerationForTesting() const { return m_statsGeneration; } + /// Whether a stale-thread recovery is still waiting for its result. + /// + /// A test seam. The recovery target is cleared as a matter of course by any + /// query the user runs, so "is it still set immediately after the button" + /// is the only way to see that it survived the slot that set it. + bool hasPendingRecoveryForTesting() const + { + return !m_recoverThreadId.isEmpty(); + } + protected: void closeEvent(QCloseEvent *event) override; @@ -165,6 +175,23 @@ protected: private slots: void runCurrentQuery(); + + /// Brings back a thread that stopped matching, and restores the reader's + /// place inside it. + /// + /// Runs `thread:<id>` so the whole conversation is listed rather than the + /// single message, then expands it and selects `messageId` once the rows + /// exist. Both steps are queued round-trips to the worker, so the ids are + /// remembered in m_recoverThreadId / m_recoverMessageId and acted on as the + /// replies arrive. + /// + /// A slot because MessageView's notice connects to it, and because the + /// sequencing above is only testable by driving it through the same entry + /// point the button uses. + void recoverStaleThread(const QString &threadId, const QString &messageId); + + /// Selects the remembered message once its thread's rows have loaded. + void applyPendingRecovery(); void onThreadsReady(const QVector<ThreadSummary> &threads, quint64 generation); void onQueryFinished(int total, quint64 generation); void onThreadSelected(const QModelIndex ¤t, const QModelIndex &previous); @@ -389,6 +416,27 @@ private: /// Sends every edit held while the lock was busy, oldest first. void flushHeldEdits(); + /// Re-runs the current query and reconciles the result into the model. + /// + /// The non-destructive counterpart to `runCurrentQuery()`, and what a sync + /// fires: nothing is cleared, so the selection, the expanded threads, the + /// undo stack and the message being read all survive. New threads appear + /// where the sort puts them and threads that stopped matching leave. + /// + /// Does nothing when no query has run yet, since there is nothing to + /// re-run. + void refreshCurrentQuery(); + + /// Shows or hides the message pane's "no longer matches" notice. + /// + /// Called after a refresh, which is the only thing that can remove a row + /// from under a reader. A thread read out of an Unread view is the ordinary + /// case: the pane keeps rendering it, correctly, while the list no longer + /// offers it anywhere, and without this the message quietly becomes an + /// orphan with no route back. + void updateStaleThreadNotice(); + + /// A tag change not yet sent to the worker, because a sync held the write /// lock when the user made it. /// @@ -513,6 +561,63 @@ private: /// and reopened, so an old answer cannot fill in a newer dialog. quint64 m_statsGeneration = 0; + /// The generation of a REFRESH query, run after a sync to bring the list + /// up to date without disturbing it. + /// + /// Numbered from the same counter as an ordinary query, so a refresh and a + /// user query can never share an id, but tracked separately because the two + /// consume their results differently: an ordinary query appends into a + /// cleared model as batches arrive, while a refresh accumulates every batch + /// and reconciles once at the end. Zero when no refresh is in flight. + /// + /// A user query started while a refresh is running silently supersedes it: + /// the refresh's batches are still collected but its result is dropped, for + /// the same reason the generation counter exists at all. Reconciling it + /// would fight the query the user just typed. + quint64 m_refreshGeneration = 0; + + /// Threads collected from a refresh query, complete only once its + /// queryFinished arrives. + /// + /// Held rather than applied per batch because reconcile() needs the WHOLE + /// result to tell a thread that stopped matching from one that simply has + /// not arrived yet. Reconciling batch by batch would delete every row the + /// first batch did not contain, emptying the list and refilling it, which + /// is the reset this exists to avoid. + QVector<ThreadSummary> m_refreshThreads; + + /// The thread and message a stale-thread recovery is waiting to select. + /// + /// Recovery spans two queued round-trips (the query, then the reply walk), + /// so the target cannot be a local variable. Cleared once the selection + /// lands, or by any query the user runs in the meantime: that is them + /// choosing to go somewhere else, and restoring a selection into a result + /// they did not ask for would yank the view. + QString m_recoverThreadId; + QString m_recoverMessageId; + + /// The thread the pane's current MESSAGE belongs to. + /// + /// Selecting a message row clears m_currentThreadId (the pane shows one + /// message, not a conversation), so without this a reader three replies + /// deep has no thread to check against the refreshed list, and the stale + /// notice never appears for them. Not obtainable from the model after the + /// fact: ThreadListModel::threadIdForMessage() searches the rows, and by + /// the time this is needed the thread has left them. + QString m_currentMessageThreadId; + + /// True while the status bar is showing this window's own "Background sync + /// running..." message. + /// + /// A refresh after a cron sync is deliberately silent, so it writes nothing + /// to the bar. That left the running message standing after the sync + /// finished, because the "completed" message it replaced was the only thing + /// that ever cleared it. Silence means saying nothing NEW, not leaving a + /// stale claim on screen: this marks the one string the Idle branch is + /// entitled to retire, so it cannot overwrite a selection count or anything + /// else the user is actually reading. + bool m_announcedExternalSync = false; + /// Holds the sync log and its close button, so the pane can be dismissed. QWidget *m_syncLogPane = nullptr; QPlainTextEdit *m_syncLog = nullptr; diff --git a/src/messageview.cpp b/src/messageview.cpp index b804d37..b6c3fa3 100644 --- a/src/messageview.cpp +++ b/src/messageview.cpp @@ -188,6 +188,47 @@ MessageView::MessageView(QWidget *parent) blockedRow->addWidget(m_loadRemoteButton); blockedRow->addStretch(); + // The stale-thread notice, deliberately the same shape as the row above: + // a sentence and a button, above the message, leaving it readable. The + // user asked for this rather than for a dialog, and a dialog would be + // wrong anyway, since nothing here needs an answer before the message can + // be read. + m_staleBar = new QWidget(this); + m_staleBar->setObjectName(QStringLiteral("staleThreadBar")); + m_staleLabel = new QLabel( + tr("This thread no longer matches the current query."), m_staleBar); + m_staleButton = new QPushButton(tr("Show it anyway"), m_staleBar); + m_staleButton->setObjectName(QStringLiteral("staleThreadButton")); + connect(m_staleButton, &QPushButton::clicked, this, [this] { + if (m_staleThreadId.isEmpty()) + return; + + // COPIES, not the members themselves, and this is load-bearing rather + // than tidy. A direct connection passes these by reference all the way + // into MainWindow::recoverStaleThread(), which calls runCurrentQuery(), + // which blanks the pane, which calls setStaleThread() and assigns to + // the very members those references name. The ids then read as empty + // for the rest of the slot, so the recovery target was stored as an + // empty string and nothing was ever recovered: the thread came back + // collapsed with a blank pane, which is exactly the reported symptom. + // + // Invisible to a test that reaches the slot through invokeMethod, + // because that copies the arguments; it needs the real signal. + const QString threadId = m_staleThreadId; + const QString messageId = m_staleMessageId; + + // The message on screen goes with the request. Recovering the thread + // alone would reopen it at its first message, and the user was reading + // message four of eight. + emit staleThreadRecoveryRequested(threadId, messageId); + }); + auto *staleRow = new QHBoxLayout(m_staleBar); + staleRow->setContentsMargins(0, 0, 0, 0); + staleRow->addWidget(m_staleLabel); + staleRow->addWidget(m_staleButton); + staleRow->addStretch(); + m_staleBar->hide(); + m_attachmentBar = new QWidget(this); m_attachmentBar->setObjectName(QStringLiteral("attachmentBar")); new QHBoxLayout(m_attachmentBar); @@ -200,6 +241,7 @@ MessageView::MessageView(QWidget *parent) auto *layout = new QVBoxLayout(this); layout->addLayout(headerRow); layout->addLayout(blockedRow); + layout->addWidget(m_staleBar); layout->addWidget(m_view, 1); layout->addWidget(m_attachmentBar); layout->addWidget(m_tagStrip); @@ -279,6 +321,12 @@ void MessageView::clear() m_blockedLabel->hide(); m_loadRemoteButton->hide(); + // The stale notice describes the message that WAS rendered, so it goes with + // it, for the same reason as the blocked-content bar above. Left behind, it + // sits over a blank pane naming a thread that is no longer shown, and its + // button offers to recover a thread the user has navigated away from. + setStaleThread(QString(), QString()); + // clear() does not go through render(), so the bar has to be emptied // here or the previous thread's attachments stay offered. rebuildAttachmentBar(); @@ -654,6 +702,14 @@ void MessageView::saveAttachment(const Attachment &attachment) emit statusMessage(tr("Saved %1").arg(written)); } +void MessageView::setStaleThread(const QString &threadId, + const QString &messageId) +{ + m_staleThreadId = threadId; + m_staleMessageId = messageId; + m_staleBar->setVisible(!threadId.isEmpty()); +} + void MessageView::toggleHtml() { const bool anyHtml = std::any_of( diff --git a/src/messageview.h b/src/messageview.h index 135c85d..63fd6d8 100644 --- a/src/messageview.h +++ b/src/messageview.h @@ -104,6 +104,28 @@ public: qreal zoomFactor() const; void setZoomFactor(qreal factor); + /// Shows or hides the notice saying the rendered thread no longer matches + /// the current query. + /// + /// Modelled on the remote-content bar rather than on a dialog: the message + /// stays readable underneath, and the way back is one click. Passing an + /// empty id hides it. + /// + /// The pane does not decide this for itself. It renders whatever it was + /// last given and has no idea what the thread list holds, so the window + /// tells it after a refresh. + /// `messageId` is the message on screen, empty when a whole thread is + /// rendered. It rides along so recovery can restore the reader's place + /// rather than reopening the thread at its first message. + void setStaleThread(const QString &threadId, const QString &messageId); + + /// The thread the stale notice offers to bring back, empty when hidden. + QString staleThreadId() const { return m_staleThreadId; } + + /// The message the stale notice would restore, empty when a whole thread + /// is rendered or the notice is hidden. + QString staleMessageId() const { return m_staleMessageId; } + public slots: void toggleHtml(); void loadRemoteContent(); @@ -125,6 +147,16 @@ signals: /// app" is a boundary worth keeping shut rather than arguing about. void queryRequested(const QString &query); + /// The user asked to see a thread that stopped matching the current query. + /// + /// Carries the thread id and the message that was on screen, because + /// recovering the thread alone would land the user on its first message + /// rather than the one they were reading. The window runs the query, + /// expands the thread and restores the selection; the view knows none of + /// that. + void staleThreadRecoveryRequested(const QString &threadId, + const QString &messageId); + protected: /// Turns Ctrl+wheel over the body into zoom, and Ctrl+middle-click into a /// reset. Both events are delivered to the web view's internal QQuickWidget @@ -185,6 +217,13 @@ private: QLabel *m_headerLabel = nullptr; QLabel *m_blockedLabel = nullptr; QPushButton *m_loadRemoteButton = nullptr; + + /// The stale-thread notice and the thread it offers to restore. + QWidget *m_staleBar = nullptr; + QLabel *m_staleLabel = nullptr; + QPushButton *m_staleButton = nullptr; + QString m_staleThreadId; + QString m_staleMessageId; QPushButton *m_detailsButton = nullptr; QWidget *m_attachmentBar = nullptr; TagStrip *m_tagStrip = nullptr; diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index bdc7e96..21a6378 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -18,6 +18,8 @@ #include "threadlistmodel.h" +#include <QSet> + #include <QBrush> #include <QFont> #include <QGuiApplication> @@ -551,6 +553,108 @@ void ThreadListModel::appendBatch(const QVector<ThreadSummary> &batch) endInsertRows(); } +void ThreadListModel::reconcile(const QVector<ThreadSummary> &threads) +{ + // Removals first, walking BACKWARDS. Each beginRemoveRows renumbers + // everything after it, so a forward walk would delete by stale indices; a + // backward one only ever disturbs rows it has already passed. + // + // One signal per contiguous run rather than per row: a view rebuilds its + // selection and its persistent indexes on every one, and an Unread view + // emptied by a sync can drop dozens at once. + QSet<QString> wanted; + wanted.reserve(threads.size()); + for (const ThreadSummary &summary : threads) + wanted.insert(summary.threadId); + + for (int row = m_threads.size() - 1; row >= 0; --row) { + if (wanted.contains(m_threads.at(row).summary.threadId)) + continue; + int first = row; + while (first > 0 + && !wanted.contains(m_threads.at(first - 1).summary.threadId)) + --first; + beginRemoveRows({}, first, row); + m_threads.remove(first, row - first + 1); + endRemoveRows(); + row = first; + } + + // What survived, by id, so the second pass can tell an arrival from a + // thread that merely moved. + QHash<QString, int> present; + present.reserve(m_threads.size()); + for (int row = 0; row < m_threads.size(); ++row) + present.insert(m_threads.at(row).summary.threadId, row); + + // Insertions, forwards, at the position the RESULT gives them. Walking the + // result in order means each new thread is placed against rows already + // agreed on, so the model ends in the result's order without this having to + // know what that order means. + for (int target = 0; target < threads.size(); ++target) { + const ThreadSummary &summary = threads.at(target); + const auto it = present.constFind(summary.threadId); + + if (it == present.constEnd()) { + const int at = qMin(target, m_threads.size()); + beginInsertRows({}, at, at); + m_threads.insert(at, ThreadNode{ summary, {}, {}, false }); + endInsertRows(); + + // Every later row shifted by one, and the map is read again on the + // next iteration. + for (auto entry = present.begin(); entry != present.end(); ++entry) { + if (entry.value() >= at) + ++entry.value(); + } + continue; + } + + // A survivor that MOVED, which is neither an arrival nor a departure + // and is the commonest reordering there is: a new reply bumps an old + // thread to the front under newest-first. beginMoveRows, not a + // remove-and-insert pair, because a removed row takes its persistent + // index, its selection and its expansion with it, which is exactly what + // this method exists to keep. + // + // Always UPWARDS, and that is a property of the walk rather than an + // assumption about the data. Positions ahead of `target` are already + // final, so a survivor found at a later row is pulled forward and one + // found earlier cannot exist: it would have been placed on a previous + // iteration. A downward branch here would be unreachable, so there + // isn't one, and the destination needs no adjustment (Qt reads it + // before the source is removed, which only shifts a downward move). + int row = it.value(); + if (row != target) { + Q_ASSERT(row > target); + beginMoveRows({}, row, row, {}, target); + m_threads.move(row, target); + endMoveRows(); + + // Every row between the two shifted one place later. + for (auto entry = present.begin(); entry != present.end(); ++entry) { + if (entry.value() >= target && entry.value() < row) + ++entry.value(); + } + present[summary.threadId] = target; + row = target; + } + + // Keep the ROW, replace the summary. The node is not reconstructed, + // because its children and its loaded flag are the expansion state this + // whole method exists to preserve. + if (m_threads.at(row).summary.tags != summary.tags + || m_threads.at(row).summary.subject != summary.subject + || m_threads.at(row).summary.authors != summary.authors + || m_threads.at(row).summary.date != summary.date + || m_threads.at(row).summary.totalCount != summary.totalCount + || m_threads.at(row).summary.matchedCount != summary.matchedCount) { + m_threads[row].summary = summary; + emit dataChanged(index(row, 0), index(row, 0)); + } + } +} + void ThreadListModel::clear() { beginResetModel(); diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index f381ffa..01dd9d0 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -180,6 +180,20 @@ public: void appendBatch(const QVector<ThreadSummary> &batch); void clear(); + /// Brings the model to `threads` without resetting it. + /// + /// Used by the automatic refresh after a sync, where clear() plus + /// appendBatch() is the wrong tool: a reset invalidates every index, so the + /// selection, the expanded threads and the message being read all go with + /// it. Rows are matched by thread id, so a surviving thread keeps its + /// identity, its persistent index and its loaded replies. + /// + /// Order comes from `threads` and is never imposed here. The worker sorts + /// the query, so a new thread lands at the front under newest-first and at + /// the back under oldest-first; forcing new rows to the top would + /// contradict the sort the user selected. + void reconcile(const QVector<ThreadSummary> &threads); + ThreadSummary threadAt(int row) const; /// Fills in a thread's message rows once the worker has walked its tree. diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 922705c..504c10c 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -20,6 +20,7 @@ #include <QAction> #include <QApplication> +#include <QFocusEvent> #include <QCloseEvent> #include <QDir> #include <QKeyEvent> @@ -99,6 +100,27 @@ private slots: void aLocalSyncIsNotReportedAsABackgroundOne(); void aLocalSyncsOwnLockIsNeverReportedAsBackground(); void aSkippedLocalSyncStillReportsTheOtherRunFinishing(); + void aCronSyncRefreshesTheListWithoutAQuery(); + void aCronSyncRefreshesOverASelectionWithoutClearingIt(); + void aCronSyncDoesNotRefreshBeforeAnyQueryHasRun(); + void aRefreshAddsNewMailAndDropsWhatStoppedMatching(); + void theOpenThreadLeavingTheListRaisesTheStaleNotice(); + void aThreadStillMatchingRaisesNoStaleNotice(); + void theStaleNoticeCarriesTheMessageBeingRead(); + void recoveringAStaleThreadQueriesTheWholeThread(); + void recoveryReselectsTheMessageThatWasBeingRead(); + void recoveryOnTheFirstMessageSelectsTheThreadRow(); + void aUserQueryAbandonsAPendingRecovery(); + void blankingThePaneAlsoDropsTheStaleNotice(); + void aNewQueryDropsTheStaleNotice(); + void aFinishedBackgroundSyncStopsSayingItIsRunning(); + void aRefreshDoesNotStampOverASelectionMessage(); + void aRefreshDoesNotOpenNewMailByItself(); + void openingAnotherMessageDropsTheStaleNoticeOfThePreviousOne(); + void theStaleNoticeKeepsTheMessageOfAThreadRootToo(); + void recoveryExpandsTheThreadAndSelectsRatherThanOnlyPointing(); + void recoveryFromAnExpandedThreadRestoresTheReply(); + void theRecoveryButtonSurvivesThePaneBeingBlanked(); void anUnobservableLockTableLeavesTheSyncButtonUsable(); void theStatusBarFollowsTheSyncPhase(); void aSelectedReadThreadIsNotDimmedIntoTheHighlight(); @@ -2357,12 +2379,18 @@ void TestMainWindow::aLocalSyncIsNotReportedAsABackgroundOne() qPrintable(QStringLiteral("a background sync was not announced, " "status says '%1'").arg(status->text()))); + // The finish is no longer ANNOUNCED, since item 35b made the refresh + // silent, but it must still be acted on: the running message it wrote is + // retired. Asserting the absence of "running" rather than the presence of + // "completed" keeps the test on this window's subject, which is that the + // lock period was attributed to a background sync rather than to a local + // one, without pinning wording that has already changed once. QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, SyncMonitor::State::Idle)); - QVERIFY2(status->text().contains(QStringLiteral("Background")), - qPrintable(QStringLiteral("a finished background sync was not " - "announced, status says '%1'") + QVERIFY2(!status->text().contains(QStringLiteral("running")), + qPrintable(QStringLiteral("a finished background sync left the bar " + "claiming it was still running: '%1'") .arg(status->text()))); } @@ -2437,14 +2465,1029 @@ void TestMainWindow::aSkippedLocalSyncStillReportsTheOtherRunFinishing() qPrintable(QStringLiteral("a skip was reported as a failure: '%1'") .arg(status->text()))); - // The other run finishing must still be announced. + // The other run finishing must still be ACTED ON. What that means depends + // on the state of the list, and this fixture has an empty one with nothing + // selected, so item 35a's free-refresh branch is what a handed-back lock + // reaches: the window re-runs the query rather than printing "press Enter". + // + // Asserting the generation rather than the status text is deliberate. The + // property under test is that the Idle was attributed to the OTHER run + // instead of being swallowed as this window's own; which of the two + // responses it then produces is item 35a's business, and pinning the + // wording here would fail every time that decision is revisited. + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + const quint64 before = window.currentGenerationForTesting(); + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, SyncMonitor::State::Idle)); - QVERIFY2(status->text().contains(QStringLiteral("Background")), - qPrintable(QStringLiteral("after a skipped local sync, the other " - "run finishing was swallowed; status " - "says '%1'").arg(status->text()))); + QVERIFY2(window.currentGenerationForTesting() > before, + "after a skipped local sync, the other run finishing was " + "swallowed rather than attributed to the background sync"); +} + +void TestMainWindow::aCronSyncRefreshesTheListWithoutAQuery() +{ + // Item 35b. A background sync brings the list up to date on its own, with + // no keystroke: this is the whole point of the item, and it holds whether + // the list is empty or full. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + // The refresh is observed as a NEW query being issued. There is no worker + // in this fixture, so no result ever arrives; what is asserted is that the + // query went out at all, which is what used to be missing. + const quint64 before = window.currentGenerationForTesting(); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + + QVERIFY2(window.currentGenerationForTesting() > before, + "a finished cron sync did not refresh the list"); +} + +void TestMainWindow::aCronSyncRefreshesOverASelectionWithoutClearingIt() +{ + // The behaviour 0.8.0 refused to build and the reason it refused: a refresh + // used to mean runCurrentQuery(), which clears the model, the selection and + // the message pane, so firing it on a cron timer would close the thread + // being read six times an hour. + // + // refreshCurrentQuery() reconciles instead, so the refresh runs AND the + // selection survives. A test that only checked the query was issued would + // pass against the destructive version, which is the version this item + // exists to avoid. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + + model->appendBatch({ makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }) }); + const QModelIndex first = model->index(0, 0, QModelIndex()); + QVERIFY(first.isValid()); + view->setCurrentIndex(first); + QVERIFY2(view->selectionModel()->hasSelection(), + "the fixture failed to select a row, so this proves nothing"); + + const quint64 before = window.currentGenerationForTesting(); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + + QVERIFY2(window.currentGenerationForTesting() > before, + "a cron sync did not refresh a populated list"); + + // The rows are untouched until the result comes back, and the selection is + // still there. A clear() would have emptied both. + QCOMPARE(model->rowCount(QModelIndex()), 1); + QVERIFY2(view->selectionModel()->hasSelection(), + "the refresh cleared the selection, which is what made the old " + "one unusable on a cron timer"); +} + +void TestMainWindow::aCronSyncDoesNotRefreshBeforeAnyQueryHasRun() +{ + // The query bar holds text the user has typed but not run, and a refresh + // must not execute it: that is a search they never asked for. The refresh + // re-runs the LAST RUN query, so with none there is nothing to do. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:draft-i-was-typing")); + + const quint64 before = window.currentGenerationForTesting(); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + + QCOMPARE(window.currentGenerationForTesting(), before); +} + +void TestMainWindow::aRefreshAddsNewMailAndDropsWhatStoppedMatching() +{ + // The round trip end to end, driven through the real handlers: the refresh + // query goes out, its batches accumulate, and the result reconciles into + // the model in one go. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + model->appendBatch({ makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }), + makeThread(QStringLiteral("T2"), + { QStringLiteral("unread") }) }); + QCOMPARE(model->rowCount(QModelIndex()), 2); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + const quint64 refresh = window.currentGenerationForTesting(); + + // T2 was read elsewhere and no longer matches; T3 is new mail. + const QVector<ThreadSummary> result{ + makeThread(QStringLiteral("T3"), { QStringLiteral("unread") }), + makeThread(QStringLiteral("T1"), { QStringLiteral("unread") }) + }; + QMetaObject::invokeMethod(&window, "onThreadsReady", + Q_ARG(QVector<ThreadSummary>, result), + Q_ARG(quint64, refresh)); + + // Nothing has changed yet: a refresh applies its result whole, never batch + // by batch, or the first batch would delete every row after it. + QCOMPARE(model->rowCount(QModelIndex()), 2); + QCOMPARE(model->threadAt(0).threadId, QStringLiteral("T1")); + + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 2), Q_ARG(quint64, refresh)); + + QCOMPARE(model->rowCount(QModelIndex()), 2); + QCOMPARE(model->threadAt(0).threadId, QStringLiteral("T3")); + QCOMPARE(model->threadAt(1).threadId, QStringLiteral("T1")); +} + +void TestMainWindow::theOpenThreadLeavingTheListRaisesTheStaleNotice() +{ + // The user is reading a thread when a refresh drops it: read the last + // unread message and the thread stops matching tag:unread. The pane keeps + // rendering it, correctly, so without a notice the message becomes an + // orphan with no route back to its thread. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + auto *pane = window.findChild<MessageView *>(); + QVERIFY(pane); + + model->appendBatch({ makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }) }); + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + QCOMPARE(window.currentThreadId(), QStringLiteral("T1")); + QVERIFY(pane->staleThreadId().isEmpty()); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + const quint64 refresh = window.currentGenerationForTesting(); + + // The refresh comes back empty: the thread was read and is gone. + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 0), Q_ARG(quint64, refresh)); + + QCOMPARE(model->rowCount(QModelIndex()), 0); + QCOMPARE(pane->staleThreadId(), QStringLiteral("T1")); +} + +void TestMainWindow::aThreadStillMatchingRaisesNoStaleNotice() +{ + // The common case, and the guard on the test above: a refresh that changes + // nothing must be invisible. A notice on every sync would be noise, and it + // would be a lie. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + auto *pane = window.findChild<MessageView *>(); + QVERIFY(pane); + + model->appendBatch({ makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }) }); + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + const quint64 refresh = window.currentGenerationForTesting(); + + const QVector<ThreadSummary> result{ + makeThread(QStringLiteral("T1"), { QStringLiteral("unread") }) + }; + QMetaObject::invokeMethod(&window, "onThreadsReady", + Q_ARG(QVector<ThreadSummary>, result), + Q_ARG(quint64, refresh)); + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 1), Q_ARG(quint64, refresh)); + + QCOMPARE(model->rowCount(QModelIndex()), 1); + QVERIFY2(pane->staleThreadId().isEmpty(), + "a thread that still matches was reported as stale"); +} + +void TestMainWindow::theStaleNoticeCarriesTheMessageBeingRead() +{ + // Reading reply four of eight when the thread drops out. Recovery has to + // restore the READER'S place, so the notice carries the message id as well + // as the thread; without it the thread reopens at its first message. + // + // Selecting a message row clears m_currentThreadId, so a notice keyed on + // that alone never fires for exactly the reader who is deepest into a + // thread. That is the case this pins. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + auto *pane = window.findChild<MessageView *>(); + QVERIFY(pane); + + ThreadSummary thread = makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }); + thread.totalCount = 3; + model->appendBatch({ thread }); + + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("T1"); + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("T1"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("T1"), { root, reply }); + + const QModelIndex threadIndex = model->index(0, 0, QModelIndex()); + const QModelIndex replyIndex = model->index(0, 0, threadIndex); + QVERIFY(replyIndex.isValid()); + view->setCurrentIndex(replyIndex); + + // A message row, so the window is tracking a message rather than a thread. + QVERIFY2(window.currentThreadId().isEmpty(), + "the fixture selected a thread row, so this proves nothing"); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + const quint64 refresh = window.currentGenerationForTesting(); + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 0), Q_ARG(quint64, refresh)); + + QCOMPARE(pane->staleThreadId(), QStringLiteral("T1")); + QCOMPARE(pane->staleMessageId(), QStringLiteral("m1@example.org")); +} + +void TestMainWindow::recoveringAStaleThreadQueriesTheWholeThread() +{ + // Clicking "Show it anyway" runs thread:<id>, not a query for the single + // message: the user asked to get the whole conversation back, with their + // place in it, so the list has to offer every message of it. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + QMetaObject::invokeMethod(&window, "recoverStaleThread", + Q_ARG(QString, QStringLiteral("T1")), + Q_ARG(QString, QStringLiteral("m1@example.org"))); + + QCOMPARE(queryEdit->text(), QStringLiteral("thread:T1")); +} + +void TestMainWindow::recoveryReselectsTheMessageThatWasBeingRead() +{ + // The whole point of carrying the message id: reading reply four of eight, + // the thread comes back, and the selection lands on reply four rather than + // at the top of the thread. + // + // Driven through the real handlers because that is the only way the + // sequencing is exercised: the query has to come back before the thread + // can be expanded, and the expansion before the reply row exists at all. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + + QMetaObject::invokeMethod(&window, "recoverStaleThread", + Q_ARG(QString, QStringLiteral("T1")), + Q_ARG(QString, QStringLiteral("m2@example.org"))); + const quint64 generation = window.currentGenerationForTesting(); + + ThreadSummary thread = makeThread(QStringLiteral("T1"), {}); + thread.totalCount = 3; + const QVector<ThreadSummary> result{ thread }; + QMetaObject::invokeMethod(&window, "onThreadsReady", + Q_ARG(QVector<ThreadSummary>, result), + Q_ARG(quint64, generation)); + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 1), Q_ARG(quint64, generation)); + + // The thread is listed, and nothing can be selected inside it yet: its + // replies are not loaded, so the recovery is still pending. + QCOMPARE(model->rowCount(QModelIndex()), 1); + + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("T1"); + root.depth = 0; + MessageNode first; + first.messageId = QStringLiteral("m1@example.org"); + first.threadId = QStringLiteral("T1"); + first.depth = 1; + MessageNode target; + target.messageId = QStringLiteral("m2@example.org"); + target.threadId = QStringLiteral("T1"); + target.depth = 1; + const QVector<MessageNode> nodes{ root, first, target }; + QMetaObject::invokeMethod(&window, "onThreadTreeLoaded", + Q_ARG(QVector<MessageNode>, nodes), + Q_ARG(quint64, generation)); + + const QModelIndex current = view->currentIndex(); + QVERIFY2(current.isValid(), "recovery selected nothing"); + QVERIFY2(model->isMessageRow(current), + "recovery landed on the thread rather than on the message"); + QCOMPARE(model->messageAt(current).messageId, + QStringLiteral("m2@example.org")); +} + +void TestMainWindow::recoveryOnTheFirstMessageSelectsTheThreadRow() +{ + // The trap in the model: setThreadMessages DROPS the depth-0 message, + // because the root card is that message. So a reader recovering from the + // thread's first message must land on the ROOT row; looking for it among + // the children finds nothing and would leave the selection nowhere. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + + QMetaObject::invokeMethod(&window, "recoverStaleThread", + Q_ARG(QString, QStringLiteral("T1")), + Q_ARG(QString, QStringLiteral("m0@example.org"))); + const quint64 generation = window.currentGenerationForTesting(); + + ThreadSummary thread = makeThread(QStringLiteral("T1"), {}); + thread.totalCount = 2; + const QVector<ThreadSummary> result{ thread }; + QMetaObject::invokeMethod(&window, "onThreadsReady", + Q_ARG(QVector<ThreadSummary>, result), + Q_ARG(quint64, generation)); + + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("T1"); + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("T1"); + reply.depth = 1; + const QVector<MessageNode> nodes{ root, reply }; + QMetaObject::invokeMethod(&window, "onThreadTreeLoaded", + Q_ARG(QVector<MessageNode>, nodes), + Q_ARG(quint64, generation)); + + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 1), Q_ARG(quint64, generation)); + + const QModelIndex current = view->currentIndex(); + QVERIFY2(current.isValid(), "recovery selected nothing"); + QVERIFY2(!model->isMessageRow(current), + "the thread's first message is the ROOT row, not a child"); + QCOMPARE(model->threadAt(current.row()).threadId, QStringLiteral("T1")); +} + +void TestMainWindow::aUserQueryAbandonsAPendingRecovery() +{ + // A recovery spans two round-trips, so the user can type a query in the + // middle of one. That is them choosing to go somewhere else, and the + // pending selection must not follow them there: restoring a thread's + // message into a result the user asked for something else from would yank + // the view out from under them. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + + // Recovering the thread with NO message pinned, so the pending recovery + // selects its thread row the moment that thread appears. A recovery + // waiting on a specific reply would pass this test without the guard, + // simply by never reaching its target: it would sit expanding a thread + // whose replies this fixture never delivers. + QMetaObject::invokeMethod(&window, "recoverStaleThread", + Q_ARG(QString, QStringLiteral("T1")), + Q_ARG(QString, QString())); + + // The user changes their mind before the recovery's query comes back. + queryEdit->setText(QStringLiteral("tag:flagged")); + queryEdit->returnPressed(); + const quint64 generation = window.currentGenerationForTesting(); + + // That query happens to contain the same thread, which is what makes this + // a trap rather than a theoretical case: the recovery would find its + // target and select it. + ThreadSummary thread = makeThread(QStringLiteral("T1"), {}); + thread.totalCount = 2; + const QVector<ThreadSummary> result{ + makeThread(QStringLiteral("T9"), {}), thread + }; + QMetaObject::invokeMethod(&window, "onThreadsReady", + Q_ARG(QVector<ThreadSummary>, result), + Q_ARG(quint64, generation)); + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 2), Q_ARG(quint64, generation)); + + QVERIFY2(!view->currentIndex().isValid(), + "an abandoned recovery selected a row in the query the user ran " + "instead"); +} + +void TestMainWindow::blankingThePaneAlsoDropsTheStaleNotice() +{ + // Reported by the user against the first build of item 35b. The notice + // outlived the message it describes: blanking the pane left the bar sitting + // above an empty pane, still naming a thread that was no longer shown, with + // a button offering to recover it. + // + // The bar belongs to the rendered message, exactly as the remote-content + // bar does, and MessageView::clear() already hides that one. This is the + // same rule applied to the same place. + const Config config; + MainWindow window(config); + + auto *pane = window.findChild<MessageView *>(); + QVERIFY(pane); + + pane->setStaleThread(QStringLiteral("T1"), + QStringLiteral("m1@example.org")); + QCOMPARE(pane->staleThreadId(), QStringLiteral("T1")); + + pane->clear(); + + QVERIFY2(pane->staleThreadId().isEmpty(), + "the stale notice survived the pane being blanked, so it names a " + "message that is no longer displayed"); + QVERIFY2(pane->staleMessageId().isEmpty(), + "the stale notice kept the message id of a cleared pane"); +} + +void TestMainWindow::aNewQueryDropsTheStaleNotice() +{ + // The user's actual route to the bug: read a thread out of a view, get the + // notice, then type a new query. The pane blanks and the bar must go with + // it. Driven through the window rather than through MessageView::clear() + // directly, because the defect was that nothing on this path called it. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + auto *pane = window.findChild<MessageView *>(); + QVERIFY(pane); + + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + + model->appendBatch({ makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }) }); + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + const quint64 refresh = window.currentGenerationForTesting(); + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 0), Q_ARG(quint64, refresh)); + QCOMPARE(pane->staleThreadId(), QStringLiteral("T1")); + + // The user runs a different query. + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + QVERIFY2(pane->staleThreadId().isEmpty(), + "after a new query the notice still offered to recover the thread " + "from the previous view"); +} + +void TestMainWindow::aFinishedBackgroundSyncStopsSayingItIsRunning() +{ + // Reported by the user against item 35b. "Background sync running..." is + // written straight to the status label when the lock appears, and the + // "Background sync completed" message on the way out was the only thing + // that ever replaced it. Removing that message, so a refresh could be + // silent, left the bar claiming a sync was running long after it finished. + // + // Silent means "says nothing NEW", not "leaves a stale claim standing". + const Config config; + MainWindow window(config); + + auto *status = window.findChild<QLabel *>(QStringLiteral("statusMessage")); + QVERIFY(status); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Running)); + QVERIFY2(status->text().contains(QStringLiteral("running")), + qPrintable(QStringLiteral("the fixture never announced a running " + "sync, status says '%1'") + .arg(status->text()))); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + + QVERIFY2(!status->text().contains(QStringLiteral("running")), + qPrintable(QStringLiteral("the status bar still claims a sync is " + "running after it finished: '%1'") + .arg(status->text()))); +} + +void TestMainWindow::aRefreshDoesNotStampOverASelectionMessage() +{ + // The other half, and the reason this is not simply "always write the + // thread count". A refresh runs on a cron timer under a user who may be + // doing something, and the bar carries their selection count while they + // are. Overwriting that every ten minutes is the noise the silence rule + // exists to prevent. + const Config config; + MainWindow window(config); + + auto *status = window.findChild<QLabel *>(QStringLiteral("statusMessage")); + QVERIFY(status); + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + + model->appendBatch({ makeThread(QStringLiteral("T1"), {}), + makeThread(QStringLiteral("T2"), {}) }); + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + view->selectionModel()->select(model->index(1, 0, QModelIndex()), + QItemSelectionModel::Select); + + const QString before = status->text(); + QVERIFY2(!before.isEmpty(), + "the fixture left the status bar empty, so this proves nothing"); + + // A refresh completes with no sync ever having been announced. + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + const quint64 refresh = window.currentGenerationForTesting(); + const QVector<ThreadSummary> result{ + makeThread(QStringLiteral("T1"), {}), makeThread(QStringLiteral("T2"), {}) + }; + QMetaObject::invokeMethod(&window, "onThreadsReady", + Q_ARG(QVector<ThreadSummary>, result), + Q_ARG(quint64, refresh)); + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 2), Q_ARG(quint64, refresh)); + + QCOMPARE(status->text(), before); +} + +void TestMainWindow::aRefreshDoesNotOpenNewMailByItself() +{ + // Reported by the user against item 35b. A thread read to the end empties + // an Unread view; the refresh then brings in one new message, and it opens + // ITSELF in the message pane, marking it read two seconds later without + // the user ever having looked at it. + // + // Nothing in MainWindow selects it: QTreeView sets a current index of its + // own when rows are inserted into a model that had none, and selecting a + // row is what loads it. An automatic refresh must not do that, or a cron + // timer decides what the user is reading. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + // The view has been read empty: no rows, nothing current. + QCOMPARE(model->rowCount(QModelIndex()), 0); + QVERIFY(!view->currentIndex().isValid()); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + const quint64 refresh = window.currentGenerationForTesting(); + + const QVector<ThreadSummary> result{ + makeThread(QStringLiteral("NEW"), { QStringLiteral("unread") }) + }; + QMetaObject::invokeMethod(&window, "onThreadsReady", + Q_ARG(QVector<ThreadSummary>, result), + Q_ARG(quint64, refresh)); + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 1), Q_ARG(quint64, refresh)); + + QCOMPARE(model->rowCount(QModelIndex()), 1); + + // Insertion alone does not do it, which is why the bug only showed up when + // the user came back from another desktop: QTreeView gives itself a current + // index when it takes FOCUS with none set. Reproduced here rather than + // asserted from the report, since a test that only inserts rows passes + // against the defect. + view->setFocus(); + QApplication::sendEvent(view, new QFocusEvent(QEvent::FocusIn)); + + QVERIFY2(window.currentThreadId().isEmpty(), + "the refresh opened the new mail in the message pane, which marks " + "it read without the user having looked at it"); +} + +void TestMainWindow::openingAnotherMessageDropsTheStaleNoticeOfThePreviousOne() +{ + // The other half of the user's report: the pane showed the new message + // while the notice above it still named the thread they had been reading. + // + // The notice itself was right at the moment it was raised. What made it a + // lie was the pane being replaced underneath it, so this pins the rule that + // the notice belongs to whatever is currently rendered: selecting anything + // else retires it, exactly as blanking the pane does. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + auto *pane = window.findChild<MessageView *>(); + QVERIFY(pane); + + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + model->appendBatch({ makeThread(QStringLiteral("OLD"), + { QStringLiteral("unread") }) }); + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + + // The refresh drops the thread being read and brings in new mail. + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + const quint64 refresh = window.currentGenerationForTesting(); + const QVector<ThreadSummary> result{ + makeThread(QStringLiteral("NEW"), { QStringLiteral("unread") }) + }; + QMetaObject::invokeMethod(&window, "onThreadsReady", + Q_ARG(QVector<ThreadSummary>, result), + Q_ARG(quint64, refresh)); + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 1), Q_ARG(quint64, refresh)); + + QCOMPARE(pane->staleThreadId(), QStringLiteral("OLD")); + + // The user chooses to open the new mail themselves. + const QModelIndex fresh = model->index(0, 0, QModelIndex()); + QVERIFY(fresh.isValid()); + QCOMPARE(model->threadAt(0).threadId, QStringLiteral("NEW")); + view->setCurrentIndex(fresh); + view->selectionModel()->select(fresh, QItemSelectionModel::Select); + + QVERIFY2(pane->staleThreadId().isEmpty(), + "the notice still named the previous thread while the pane showed " + "a different message"); +} + +void TestMainWindow::theStaleNoticeKeepsTheMessageOfAThreadRootToo() +{ + // Reported by the user: recovering a thread they were reading brought the + // thread back collapsed, with the pane blank, instead of reopening the + // message they had been on. + // + // The cause is that a thread ROOT sets both ids. The root card IS the + // thread's first message and the pane renders exactly that message, so + // m_currentThreadId and m_currentMessageId are both filled; the notice read + // the message id only when the thread id was EMPTY, so the root case threw + // away a message id it had. Recovery then had nothing to restore, landed on + // the thread row and never expanded it. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + auto *pane = window.findChild<MessageView *>(); + QVERIFY(pane); + + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + ThreadSummary thread = makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }); + thread.totalCount = 4; + model->appendBatch({ thread }); + + // The root knows its own message once the tree is loaded, which is what + // makes the pane show one message rather than the conversation. + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("T1"); + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("T1"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("T1"), { root, reply }); + + const QModelIndex threadIndex = model->index(0, 0, QModelIndex()); + view->setCurrentIndex(threadIndex); + view->selectionModel()->select(threadIndex, QItemSelectionModel::Select); + QCOMPARE(window.currentThreadId(), QStringLiteral("T1")); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + const quint64 refresh = window.currentGenerationForTesting(); + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 0), Q_ARG(quint64, refresh)); + + QCOMPARE(pane->staleThreadId(), QStringLiteral("T1")); + QVERIFY2(!pane->staleMessageId().isEmpty(), + "the notice dropped the message of a thread root, so recovery has " + "nothing to reopen and lands on a collapsed thread"); + QCOMPARE(pane->staleMessageId(), QStringLiteral("m0@example.org")); +} + +void TestMainWindow::recoveryExpandsTheThreadAndSelectsRatherThanOnlyPointing() +{ + // The rest of the same report: recovery brought the thread back COLLAPSED + // with the pane BLANK. Two separate faults behind one symptom. + // + // setCurrentIndex() alone sets a current row without selecting it, and + // since the fix for the auto-open defect onThreadSelected() ignores exactly + // that: an unselected current index is Qt's housekeeping, not the user. So + // recovery pointed at the row and nothing rendered. + // + // And nothing expanded the thread, so the reply the user had been reading + // was not on screen even when it was the target. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + // Recovering onto the thread's FIRST message, which is the root card: the + // case the user hit by opening a thread rather than a reply. + QMetaObject::invokeMethod(&window, "recoverStaleThread", + Q_ARG(QString, QStringLiteral("T1")), + Q_ARG(QString, QStringLiteral("m0@example.org"))); + const quint64 generation = window.currentGenerationForTesting(); + + ThreadSummary thread = makeThread(QStringLiteral("T1"), {}); + thread.totalCount = 4; + const QVector<ThreadSummary> result{ thread }; + QMetaObject::invokeMethod(&window, "onThreadsReady", + Q_ARG(QVector<ThreadSummary>, result), + Q_ARG(quint64, generation)); + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 1), Q_ARG(quint64, generation)); + + // No tree reply. This is the user's actual case and the one the earlier + // test missed: the thread comes back from the query with its replies NOT + // yet loaded, which is the normal state of a freshly queried row. Recovery + // has to ask for them rather than assuming they are already there. + const QModelIndex threadIndex = model->index(0, 0, QModelIndex()); + QVERIFY(threadIndex.isValid()); + QCOMPARE(model->rowCount(threadIndex), 0); + + QVERIFY2(view->selectionModel()->hasSelection(), + "recovery pointed at the row without selecting it, so nothing " + "renders and the pane stays blank"); + QCOMPARE(window.currentThreadId(), QStringLiteral("T1")); + QVERIFY2(view->isExpanded(threadIndex), + "recovery brought the thread back collapsed, so the conversation " + "the user was reading is not on screen"); +} + +void TestMainWindow::recoveryFromAnExpandedThreadRestoresTheReply() +{ + // The user's case, staged exactly: a thread ALREADY EXPANDED with the + // fourth reply selected and rendered, dropped by a refresh, then recovered. + // Reported twice as still broken while the earlier recovery tests passed, + // which means those tests were not reproducing it. + // + // What they missed is the whole round trip. Recovery re-runs thread:<id>, + // and that query REPLACES the model contents, so the recovered thread + // arrives collapsed with no replies loaded whatever state the old row was + // in. The reply the user wants therefore does not exist as a row at the + // moment recovery first runs, and the only thing that can create it is the + // tree reply arriving after an expand. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + // Reading reply 4 of a 4-message thread, expanded. + ThreadSummary thread = makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }); + thread.totalCount = 4; + model->appendBatch({ thread }); + + QVector<MessageNode> nodes; + for (int i = 0; i < 4; ++i) { + MessageNode n; + n.messageId = QStringLiteral("m%1@example.org").arg(i); + n.threadId = QStringLiteral("T1"); + n.depth = i == 0 ? 0 : 1; + nodes.append(n); + } + model->setThreadMessages(QStringLiteral("T1"), nodes); + + const QModelIndex threadIndex = model->index(0, 0, QModelIndex()); + view->expand(threadIndex); + QCOMPARE(model->rowCount(threadIndex), 3); + + const QModelIndex fourth = model->index(2, 0, threadIndex); + QVERIFY(fourth.isValid()); + QCOMPARE(model->messageAt(fourth).messageId, + QStringLiteral("m3@example.org")); + view->setCurrentIndex(fourth); + view->selectionModel()->select(fourth, QItemSelectionModel::Select); + + // The refresh drops it, and the user follows the notice. + QMetaObject::invokeMethod(&window, "recoverStaleThread", + Q_ARG(QString, QStringLiteral("T1")), + Q_ARG(QString, QStringLiteral("m3@example.org"))); + const quint64 generation = window.currentGenerationForTesting(); + + // The recovery query comes back: ONE collapsed thread, no replies. This is + // what the query really returns, and the state the earlier tests skipped. + const QVector<ThreadSummary> recovered{ thread }; + QMetaObject::invokeMethod(&window, "onThreadsReady", + Q_ARG(QVector<ThreadSummary>, recovered), + Q_ARG(quint64, generation)); + QMetaObject::invokeMethod(&window, "onQueryFinished", + Q_ARG(int, 1), Q_ARG(quint64, generation)); + + const QModelIndex back = model->index(0, 0, QModelIndex()); + QVERIFY(back.isValid()); + QVERIFY2(view->isExpanded(back), + "the recovered thread came back collapsed"); + + // Expanding asks the worker for the tree; that reply is what creates the + // reply rows. Without it the conversation is not on screen at all. + QMetaObject::invokeMethod(&window, "onThreadTreeLoaded", + Q_ARG(QVector<MessageNode>, nodes), + Q_ARG(quint64, generation)); + + QVERIFY2(view->isExpanded(back), + "the thread collapsed again once its replies arrived"); + QCOMPARE(model->rowCount(back), 3); + + const QModelIndex current = view->currentIndex(); + QVERIFY2(current.isValid(), "recovery left nothing selected"); + QVERIFY2(model->isMessageRow(current), + "recovery landed on the thread rather than on the reply the user " + "was reading"); + QCOMPARE(model->messageAt(current).messageId, + QStringLiteral("m3@example.org")); +} + +void TestMainWindow::theRecoveryButtonSurvivesThePaneBeingBlanked() +{ + // The defect that survived six wrong diagnoses and every other recovery + // test in this file, because all of them reach the slot through + // invokeMethod, which COPIES its arguments. + // + // MessageView emitted the signal with its own members, so a direct + // connection handed MainWindow::recoverStaleThread() references to them. + // That slot calls runCurrentQuery(), which blanks the pane, which calls + // setStaleThread() and assigns to those very members. The ids the slot was + // still holding went empty mid-call, the recovery target was stored as an + // empty string, and nothing was ever recovered: the thread came back + // collapsed with the pane blank. + // + // Driven through the real button so the real signal runs. A test that + // calls the slot directly cannot see this and will pass against it. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + auto *pane = window.findChild<MessageView *>(); + QVERIFY(pane); + auto *button = + pane->findChild<QPushButton *>(QStringLiteral("staleThreadButton")); + QVERIFY(button); + + queryEdit->setText(QStringLiteral("tag:unread")); + queryEdit->returnPressed(); + + pane->setStaleThread(QStringLiteral("T1"), + QStringLiteral("m3@example.org")); + QCOMPARE(pane->staleThreadId(), QStringLiteral("T1")); + + button->click(); + + // The query the button ran is the thread's own, which is only true if the + // id survived the round trip. + QCOMPARE(queryEdit->text(), QStringLiteral("thread:T1")); + + // And the target is still pending, waiting for the result. Empty here means + // the reference was clobbered and the recovery is already dead. + QVERIFY2(window.hasPendingRecoveryForTesting(), + "the recovery target was lost during the slot, so the thread will " + "come back collapsed with a blank pane"); } void TestMainWindow::theSyncActionIsDisabledWhileABackgroundSyncHoldsTheLock() diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index aa71080..947a6b3 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -73,6 +73,14 @@ private slots: void tagChangeForUnknownThreadIsIgnored(); void tagChangeRoundTripsForRevert(); void modelPassesQtTester(); + void reconcileAddsNewThreadsInTheOrderGiven(); + void reconcileRemovesThreadsThatNoLongerMatch(); + void reconcileKeepsSurvivingRowsAndTheirExpansion(); + void reconcileUpdatesTagsOnASurvivingThread(); + void reconcileOnAnEmptyModelFillsIt(); + void reconcileWithAnIdenticalResultChangesNothing(); + void reconcileMovesAThreadBumpedByANewReply(); + void reconcileKeepsAMovedRowsPersistentIndex(); }; static ThreadSummary makeThread(const QString &id, const QString &subject) @@ -1172,5 +1180,245 @@ void TestThreadListModel::replySharingEveryThreadTagShowsNone() .isEmpty()); } +void TestThreadListModel::reconcileAddsNewThreadsInTheOrderGiven() +{ + // Item 35b. The auto-refresh hands the model a fresh result set and the + // model works out the difference, rather than being cleared and refilled. + // + // Position comes from the result, never from a rule of this model's own: + // the query is sorted by the worker, so with newest-first a new thread + // arrives at the front and with oldest-first at the back. A model that + // forced new rows to the top would contradict the sort the user chose. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("One")), + makeThread(QStringLiteral("t2"), + QStringLiteral("Two")) }); + + model.reconcile({ makeThread(QStringLiteral("t3"), + QStringLiteral("Newest")), + makeThread(QStringLiteral("t1"), + QStringLiteral("One")), + makeThread(QStringLiteral("t2"), + QStringLiteral("Two")) }); + + QCOMPARE(model.rowCount(), 3); + QCOMPARE(model.threadAt(0).threadId, QStringLiteral("t3")); + QCOMPARE(model.threadAt(1).threadId, QStringLiteral("t1")); + QCOMPARE(model.threadAt(2).threadId, QStringLiteral("t2")); +} + +void TestThreadListModel::reconcileRemovesThreadsThatNoLongerMatch() +{ + // A thread read out of an Unread view stops matching, and the list has to + // say so. Leaving it would make the list disagree with its own query, and + // every view-wide action (Mark all read) acts on what the list holds. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("One")), + makeThread(QStringLiteral("t2"), + QStringLiteral("Two")) }); + + model.reconcile({ makeThread(QStringLiteral("t2"), + QStringLiteral("Two")) }); + + QCOMPARE(model.rowCount(), 1); + QCOMPARE(model.threadAt(0).threadId, QStringLiteral("t2")); +} + +void TestThreadListModel::reconcileKeepsSurvivingRowsAndTheirExpansion() +{ + // The whole point of reconciling rather than clearing. A surviving thread + // must keep the SAME row identity, because the view's selection, its + // expanded state and the open message all hang off persistent indexes: a + // beginResetModel drops every one of them, which is what made the old + // refresh close the thread being read. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("One")), + makeThread(QStringLiteral("t2"), + QStringLiteral("Two")) }); + + MessageNode root = makeNode(QStringLiteral("m1"), 0); + MessageNode reply = makeNode(QStringLiteral("m2"), 1); + model.setThreadMessages(QStringLiteral("t1"), { root, reply }); + QCOMPARE(model.rowCount(model.index(0, 0)), 1); + + const QPersistentModelIndex survivor(model.index(0, 0)); + QVERIFY(survivor.isValid()); + + // t2 leaves, t3 arrives, t1 stays put. + model.reconcile({ makeThread(QStringLiteral("t1"), + QStringLiteral("One")), + makeThread(QStringLiteral("t3"), + QStringLiteral("Three")) }); + + QVERIFY2(survivor.isValid(), + "reconciling invalidated a surviving row, so the selection and " + "the open thread would be lost exactly as a reset loses them"); + QCOMPARE(model.threadAt(survivor.row()).threadId, QStringLiteral("t1")); + + // Its loaded replies survive too, or the thread collapses under the reader. + QCOMPARE(model.rowCount(model.index(survivor.row(), 0)), 1); +} + +void TestThreadListModel::reconcileUpdatesTagsOnASurvivingThread() +{ + // A thread that stays but changed state: read elsewhere, tagged by a + // filter, flagged on the phone. The row has to repaint, or the list shows + // stale state while claiming to be current. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("One")) }); + QVERIFY(model.threadAt(0).tags.contains(QStringLiteral("unread"))); + + ThreadSummary readNow = makeThread(QStringLiteral("t1"), + QStringLiteral("One")); + readNow.tags = QStringList{ QStringLiteral("inbox") }; + + QSignalSpy changed(&model, &QAbstractItemModel::dataChanged); + model.reconcile({ readNow }); + + QCOMPARE(model.rowCount(), 1); + QVERIFY2(!model.threadAt(0).tags.contains(QStringLiteral("unread")), + "a surviving thread kept its stale tags"); + QVERIFY2(!changed.isEmpty(), + "the row's new state was stored without repainting it"); +} + +void TestThreadListModel::reconcileOnAnEmptyModelFillsIt() +{ + // Item 35a's case, now reached through the same path as every other + // refresh rather than through a special one: read the view empty, cron + // indexes new mail, it appears. + ThreadListModel model; + QCOMPARE(model.rowCount(), 0); + + model.reconcile({ makeThread(QStringLiteral("t1"), + QStringLiteral("New")) }); + + QCOMPARE(model.rowCount(), 1); + QCOMPARE(model.threadAt(0).threadId, QStringLiteral("t1")); +} + +void TestThreadListModel::reconcileWithAnIdenticalResultChangesNothing() +{ + // The common case: the sync brought nothing this query cares about. It + // runs every ten minutes under a reader, so it must not churn rows, and + // must not emit a reset that would collapse an expanded thread. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("One")), + makeThread(QStringLiteral("t2"), + QStringLiteral("Two")) }); + + const QPersistentModelIndex kept(model.index(1, 0)); + QSignalSpy reset(&model, &QAbstractItemModel::modelReset); + QSignalSpy inserted(&model, &QAbstractItemModel::rowsInserted); + QSignalSpy removed(&model, &QAbstractItemModel::rowsRemoved); + + model.reconcile({ makeThread(QStringLiteral("t1"), + QStringLiteral("One")), + makeThread(QStringLiteral("t2"), + QStringLiteral("Two")) }); + + QCOMPARE(model.rowCount(), 2); + QVERIFY2(reset.isEmpty(), "an unchanged result reset the model"); + QVERIFY2(inserted.isEmpty(), "an unchanged result inserted rows"); + QVERIFY2(removed.isEmpty(), "an unchanged result removed rows"); + QVERIFY(kept.isValid()); + QCOMPARE(kept.row(), 1); +} + +void TestThreadListModel::reconcileMovesAThreadBumpedByANewReply() +{ + // The case the other reconcile tests all miss, and the commonest reordering + // there is: an old thread gets a new reply, so under newest-first the + // worker returns it at the FRONT although it was already on screen. It is + // neither an arrival nor a departure, and a reconcile that only handles + // those two leaves it where it was, showing an order the query does not + // agree with. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("One")), + makeThread(QStringLiteral("t2"), + QStringLiteral("Two")), + makeThread(QStringLiteral("t3"), + QStringLiteral("Three")) }); + + // t3 was replied to and now sorts first. + model.reconcile({ makeThread(QStringLiteral("t3"), + QStringLiteral("Three")), + makeThread(QStringLiteral("t1"), + QStringLiteral("One")), + makeThread(QStringLiteral("t2"), + QStringLiteral("Two")) }); + + QCOMPARE(model.rowCount(), 3); + QCOMPARE(model.threadAt(0).threadId, QStringLiteral("t3")); + QCOMPARE(model.threadAt(1).threadId, QStringLiteral("t1")); + QCOMPARE(model.threadAt(2).threadId, QStringLiteral("t2")); +} + +void TestThreadListModel::reconcileKeepsAMovedRowsPersistentIndex() +{ + // A reordering seen from the VIEW's side rather than the data's. + // + // reconcile() places rows with beginMoveRows, and the assertions on + // threadAt() cannot tell a correct move from a broken one: QVector::move + // reorders the storage whatever Qt was told, so the data lands right even + // if the signal is wrong and only a persistent index reports the + // difference. A real view's selection rides on exactly that. + // + // Every move reconcile() makes is upwards, which is a property of the walk + // and not of this data: the result is walked front to back, so rows ahead + // of the target are already final and a misplaced survivor is always + // pulled forward. The model asserts that invariant. + ThreadListModel model; + + // Fatal, and attached BEFORE the move. The tester is what actually checks + // the beginMoveRows arguments against the rows that end up moving; the + // assertions below read m_threads, which QVector::move reorders correctly + // whatever destination Qt was told. Without this a wrong destination + // corrupts only what the VIEW is told, and every assertion here still + // passes while a real view's selection lands on the wrong row. + QAbstractItemModelTester tester( + &model, QAbstractItemModelTester::FailureReportingMode::Fatal); + Q_UNUSED(tester); + + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("One")), + makeThread(QStringLiteral("t2"), + QStringLiteral("Two")), + makeThread(QStringLiteral("t3"), + QStringLiteral("Three")) }); + + const QPersistentModelIndex moved(model.index(0, 0)); + QVERIFY(moved.isValid()); + + // t1 ends last. Reached by t2 and t3 each being pulled forward past it, + // which is what makes t1's persistent index the thing under test: it is + // displaced twice without ever being the row that moves. + model.reconcile({ makeThread(QStringLiteral("t2"), + QStringLiteral("Two")), + makeThread(QStringLiteral("t3"), + QStringLiteral("Three")), + makeThread(QStringLiteral("t1"), + QStringLiteral("One")) }); + + QCOMPARE(model.rowCount(), 3); + QCOMPARE(model.threadAt(0).threadId, QStringLiteral("t2")); + QCOMPARE(model.threadAt(1).threadId, QStringLiteral("t3")); + QCOMPARE(model.threadAt(2).threadId, QStringLiteral("t1")); + + // The persistent index followed the row rather than being invalidated, + // which is what keeps a selection on a thread that reordered under it. + // This is the assertion the destination adjustment is answerable to: with + // the wrong destination the DATA still lands correctly (QVector::move does + // not care what Qt was told) and only this reports the difference. + QVERIFY2(moved.isValid(), "a moved row lost its persistent index"); + QCOMPARE(moved.row(), 2); +} + QTEST_MAIN(TestThreadListModel) #include "test_threadlistmodel.moc" |