diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-15 15:44:38 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-15 15:44:38 +0200 |
| commit | c3dd54e390d236b7d68bae456ef39c5fa7fb49d5 (patch) | |
| tree | 433c2c15a30a1cb2038f20ec78b53b83d429f0c9 | |
| parent | 19f0ea0000f8e61a4077d06609c5c7c329edf8bb (diff) | |
| download | qtmaildir-c3dd54e390d236b7d68bae456ef39c5fa7fb49d5.tar.gz qtmaildir-c3dd54e390d236b7d68bae456ef39c5fa7fb49d5.zip | |
fix(ui): load a thread that was already displayed when the query ran
Running a query blanks the message pane but left m_currentThreadId,
m_currentMessageId and m_currentMessageThreadId naming the thread that had
been showing. Both selection handlers compare a newly selected row against
those to decide whether it is already on display, so a result containing that
same thread was recognised as "already showing" and onThreadSelected() was
never called. The card painted as selected, the status bar reported one
thread, and the pane stayed on the placeholder.
This is why it looked like an `id:` query defect. The id is copied out of the
details dialog of the message being read, so that thread is current at the
moment the query replaces the view. Any query returning a different thread
hides the fault entirely.
Filed as the unverified half of item 66 and assumed to be the same
empty-MessageIdRole failure. It is not: 66's fix was correct and this
reproduced against it, so it is recorded as item 96. Four hypotheses were
eliminated by measurement first: the row does carry the message id, the
account-scoped query does return it, MimeParser parses the reported message
(ok, 40701 bytes of HTML), and both real ids resolve bare and quoted.
The regression test's first query must open the SAME thread the second one
returns; with two different threads it passes against the defect, which is how
the first version of it was green. Reverting the fix fails it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| -rw-r--r-- | CHANGELOG.md | 9 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 48 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 3 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 15 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 105 |
5 files changed, 178 insertions, 2 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index b76b5f3..6701fae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,15 @@ point at which they are stable. ## [Unreleased] +### Fixed + +- A query whose result contains the thread already open left the message pane + on the placeholder: the card selected, the status bar reporting one thread, + and nothing rendered. Running a query blanked the pane but kept the ids + naming what it had been showing, so selecting that thread again was read as + "already displayed" and never loaded it. This is why an `id:` query copied + out of a message's own details dialog produced a card that would not open. + ## [0.23.0] - 2026-08-15 qtmaildir speaks Italian. Nothing loaded a translation before this release, so diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index 1704a28..d5f58dd 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md @@ -5290,3 +5290,51 @@ defect when `QT_TRANSLATE_NOOP` is reverted to `QT_TR_NOOP`. **Not done:** no language-selection UI, and no second language. `QLocale()` reading the environment is how a Linux desktop already chooses; add a selector when there is something to choose between. + +## 96. A query returning the thread already on display opens onto the placeholder + +**Observed (user, 2026-08-15, with two screenshots):** an `id:` query produces +exactly one card, the status bar reports "1 thread selected (1 message)", the +card paints as selected, and the message pane stays on the placeholder. Two +different messages from two different providers, with and without an account +selected. + +**Filed originally as the unverified half of item 66** and assumed to be the +same empty-`MessageIdRole` failure. It is not, and that assumption is what kept +it open: 66's fix was correct and this reproduced against it. + +**Cause (verified in code and by instrumented run):** `runQuery()` blanks the +message pane but leaves `m_currentThreadId`, `m_currentMessageId` and +`m_currentMessageThreadId` naming the thread that was showing. Both selection +handlers compare a newly selected row against those to decide whether it is +already displayed, so a result containing that same thread is recognised as +"already showing" and `onThreadSelected()` is never called. An instrumented run +printed `currentThreadId after runQuery: "0000000000000001"` against an +identical row thread id with the pane on the placeholder. + +**Why an `id:` query and nothing else.** The id is copied out of the details +dialog of the message being read, so that thread is current at the exact moment +the query replaces the view. Any query returning a different thread hides the +fault completely, which is why it survived from 2026-08-04 to 2026-08-15 and +why the first version of the regression test passed against it. + +**Four hypotheses eliminated by measurement before the cause was found**, each +of which looked likely from reading: + +- `MessageIdRole` empty on a fresh single-message row. The row carries the id. +- Account scoping mangling the query. `path:"acct/**" and (id:...)` returns the + row, and the user reproduced it with no account selected. +- `MimeParser` failing on the real file. A probe against it reported `ok true`, + 9614 bytes of plain text and 40701 of HTML. +- The id needing quoting. Both real ids resolve bare and quoted, one of them + carrying a `=` before the `@`. + +**Fix.** Clear the three ids where the pane is blanked, in `runQuery()`. They +describe what the pane is showing, so they cannot outlive it. + +**The regression test's first query must open the SAME thread the second one +returns.** With two different threads it passes against the defect, which is +how the first version of it was green. It asserts the ids are cleared, because +that is the fix's contract, and then that the pane leaves the placeholder, +which is what the user sees. Mutation check: reverting the fix fails it with +"runQuery blanked the pane but still names a current thread". 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 e921e89..c376f6e 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 @@ -130,7 +130,7 @@ taking that too literally. | 63 | No way to see sent mail, and no filter for it | workflow | M | **done** 2026-08-11; see `specs/2026-08-11-sent-mail-design.md` | | 64 | The Sync button carries a mailbox icon, not a refresh one | presentation | XS | **done** 2026-08-11 | | 65 | No full code review and optimization pass | correctness | ? | open, unspecified | -| 66 | Selecting a thread root leaves the message pane blank until a reply has been selected | defect | S | **done** 2026-08-14, unreleased. Not the blank pane it was filed as: the root rendered the CONVERSATION until the thread had been expanded once, then one message. Now always one message, and the conversation view is removed at the user's request. **One case unverified by hand:** the notes also report a single-message `id:` query whose card would not open, which is the same empty-`MessageIdRole` failure and should be gone; confirm against an `id:` query before treating it as closed | +| 66 | Selecting a thread root leaves the message pane blank until a reply has been selected | defect | S | **done** 2026-08-14, unreleased. Not the blank pane it was filed as: the root rendered the CONVERSATION until the thread had been expanded once, then one message. Now always one message, and the conversation view is removed at the user's request. **One case unverified by hand:** the notes also report a single-message `id:` query whose card would not open, which is the same empty-`MessageIdRole` failure and should be gone; confirmed 2026-08-15 as a SEPARATE defect with a different cause, see item 96 | | 87 | Auto mark-read marks a whole thread, including replies never displayed | defect | S | open; measured 2026-08-14. Reachable only after 66 removed the conversation view. Blocked on 88 | | 88 | `threadAt(current.row())` answers about the wrong thread for a reply row | defect | S | open; found 2026-08-14 by shipping a fix that marked an unrelated message read. Row numbers are per parent in a tree | | 67 | The placeholder pane counts unread, flagged and inbox, but not sent or drafts | information | XS | **done** 2026-08-11, shipped in 0.15.0 | @@ -160,6 +160,7 @@ taking that too literally. | 93 | The query buttons are whatever the user pinned, not a designed set of filters | workflow | M | **done** 2026-08-15, unreleased; see `specs/2026-08-15-builtin-filters-design.md`. Absorbs item 90. Four built-in filters composing with the account dropdown; the user's own queries unpinned, never deleted | | 95 | A query in the overflow menu cannot be run | defect | XS | **done** 2026-08-15, unreleased. Pre-existing and not caused by 93: the entry's action owned a submenu, and Qt emits no `triggered` for those, so the connection had never fired. Surfaced because 93 moved every query into the menu | | 94 | `pinned` has nothing left to decide once the buttons are built-in | maintenance | S | open; **blocked on 93**, and deliberately not part of it. A user-visible removal: the row becomes built-ins only and every saved query lives in the menu | +| 96 | A query returning the thread already on display opens onto the placeholder | defect | S | **done** 2026-08-15, unreleased. Split from 66's unverified half, which had a different cause. Reproduced from two screenshots after four measured eliminations | Sizes are rough: XS under an hour, S a sitting, M a session. diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 7ece2f3..2858514 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -2230,6 +2230,21 @@ void MainWindow::runQuery(FlatResult flat, AccountScope scope) m_messageView->clear(); showPlaceholderPane(); + // Cleared WITH the pane, not merely alongside it. These three name what the + // pane is showing, and both selection handlers use them to decide whether a + // newly selected row is already displayed. Left set across a query they + // describe a pane that was just blanked, so a result containing that same + // thread is recognised as "already showing" and never loaded. + // + // That is not a corner case, it is the ordinary way an `id:` query is run: + // the id is copied out of the details dialog of the message being read, so + // the thread is current at the moment the query replaces the view, and its + // one card opens onto the placeholder. A query returning any OTHER thread + // hides it, which is why it took a screenshot to find. + m_currentThreadId.clear(); + m_currentMessageId.clear(); + m_currentMessageThreadId.clear(); + // Undo entries refer to rows that are about to be discarded. The model // update they invert would be a no-op against the new result set, leaving // undo half-applied: the database would change and the list would not. diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index c569f05..b96b158 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -82,7 +82,11 @@ class WorkerBackedWindow public: /// Builds the database, writes the config and loads it. Check isValid() /// and error() before constructing the window. - bool build() + /// `accountKey` and `accountMaildir` add one [account.<key>] section, which + /// is what makes runQuery() scope the bar's text with scopedQuery(). A test + /// that never selects an account can leave them empty. + bool build(const QString &accountKey = QString(), + const QString &accountMaildir = QString()) { if (!m_fixture.isValid()) { m_error = QStringLiteral("fixture directory invalid"); @@ -113,6 +117,12 @@ public: // went unnoticed as broken once already. out << "[general]\n" << "notmuch_config=" << m_fixture.configPath() << "\n"; + if (!accountKey.isEmpty()) { + // QSettings reads `/` in a section name as a group separator, + // so the section is [account.key], never [account/key]. + out << "\n[account." << accountKey << "]\n" + << "maildir=" << accountMaildir << "\n"; + } } file.close(); @@ -188,6 +198,7 @@ private slots: void aRefreshsBatchesLeaveTheStatusBarAlone(); void selectingAThreadRootShowsItInTheMessagePane(); void anUnexpandedRootRendersOneMessageNotTheConversation(); + void aSingleMessageIdQuerysCardOpensInTheMessagePane(); void autoSyncIsNotArmedWhenDisabledOrWithNothingPending(); void autoSyncSkipsWhileABackgroundSyncIsRunning(); void aSuccessfulSyncRefreshesRatherThanRerunningTheQuery(); @@ -7184,4 +7195,96 @@ void TestMainWindow::anUnexpandedRootRendersOneMessageNotTheConversation() "the pane rendered a conversation, not a single message"); } +void TestMainWindow::aSingleMessageIdQuerysCardOpensInTheMessagePane() +{ + // Item 66's unverified half, reported again against 0.23.0 with a + // screenshot: an `id:` query in the bar produces exactly one card, the + // status bar reports "1 thread selected (1 message)", and the pane stays + // on the placeholder. + // + // The id is shaped like the real one that fails: dots, digits and an @. + // + // An ACCOUNT is configured and selected, because that is the state the + // report was made from and it is the only thing that changes the query: + // runQuery() wraps the bar's text in path:"<maildir>/**" and (...). + WorkerBackedWindow backed; + const QString wanted = + QStringLiteral("1786718040388.1f6b48f1-64d1@mail.example.org"); + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/Inbox"), wanted, + QStringLiteral("A single message"), + QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("The only message."))); + QVERIFY2(backed.build(QStringLiteral("acct"), QStringLiteral("acct")), + qPrintable(backed.error())); + + MainWindow window(backed.config()); + + QLineEdit *queryEdit = + window.findChild<QLineEdit *>(QStringLiteral("queryEdit")); + QVERIFY2(queryEdit, "no query bar: the window was never built"); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY2(view, "no thread list view"); + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY2(model, "no thread list model"); + auto *pane = window.findChild<MessageView *>(); + QVERIFY2(pane, "no message view"); + + auto *accountBox = + window.findChild<QComboBox *>(QStringLiteral("accountBox")); + QVERIFY2(accountBox, "no account dropdown"); + const int account = accountBox->findData(QStringLiteral("acct")); + QVERIFY2(account >= 0, "the configured account is not in the dropdown"); + accountBox->setCurrentIndex(account); + + // THE SAME MESSAGE IS READ FIRST, and that is the whole defect. The id is + // copied out of the details dialog of a message being read, so the `id:` + // query is always typed while that very thread is the current one. + // runQuery() blanks the pane but leaves m_currentThreadId naming it, so + // when the row comes back onSelectionChanged() compares the two, finds them + // equal, and never calls onThreadSelected: nothing is ever loaded. + // + // A first query returning a DIFFERENT thread passes against the bug, which + // is why the earlier version of this test was green. + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + QTRY_VERIFY_WITH_TIMEOUT(!pane->showingPlaceholder(), 15000); + const QString firstThreadId = window.currentThreadId(); + QVERIFY2(!firstThreadId.isEmpty(), "the first view never opened a thread"); + + // Exactly what the user types, `id:` and the bare id, unquoted. + queryEdit->setText(QStringLiteral("id:%1").arg(wanted)); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + + const QModelIndex root = model->index(0, 0, QModelIndex()); + QVERIFY(root.isValid()); + + // The row must name the message before the click can be blamed for + // anything: an empty MessageIdRole is a different defect and would make + // the assertion below true for the wrong reason. + QCOMPARE(model->data(root, ThreadListModel::MessageIdRole).toString(), + wanted); + + QVERIFY2(pane->showingPlaceholder(), + "the pane was not blank to begin with"); + // The row IS the thread that was showing when the query ran. Asserted so a + // fixture change that made them different threads could not quietly turn + // this back into the passing test it was before the cause was found. + QCOMPARE(model->data(root, ThreadListModel::ThreadIdRole).toString(), + firstThreadId); + + // The pane was blanked, so nothing is on display and the window must not + // still claim otherwise. This is the fix's own contract: leave it set and + // the selection below is read as "already showing" and never loads. + QVERIFY2(window.currentThreadId().isEmpty(), + "runQuery blanked the pane but still names a current thread"); + + view->setCurrentIndex(root); + QTRY_VERIFY_WITH_TIMEOUT(!pane->showingPlaceholder(), 15000); +} + #include "test_mainwindow.moc" |
