diff options
| -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" |