From 48c243d7aee3382713f7a4653be2179d313b432c Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 7 Aug 2026 18:22:45 +0200 Subject: feat(ui): make Escape deselect the row as well as blank the pane Blanking the pane while leaving the row highlighted reads as half an action, and deselecting is what Escape means nearly everywhere else. The user asked for two actions rather than a changed one, so clear_pane keeps its behaviour and moves to Shift+Esc; clear_selection takes Escape and does both. Shift+Esc rather than unbound because every action carries a default and a test enforces it. Clearing the selection re-adopts the thread it just cleared, unless done in exactly the right way. clearSelection() leaves currentIndex() valid, so onSelectionChanged takes its "one or fewer rows" branch, sees a current row whose id differs from m_currentThreadId, and calls onThreadSelected for it. Clearing the selection before blanking lets that run while the id still matches, so nothing reloads, and clearing current stops a later collapse-to-one-row reaching the same row. All four arrangements were tried; only this one passes. The first version of the test could not distinguish any of them. It asserted showingPlaceholder(), which passes regardless because this fixture has no worker, so loadThread never replies and the pane is never repainted. currentThreadId() and currentIndex() are observable without one, and asserting those is what made the test discriminate. --- .../plans/2026-08-03-post-0.1.0-usability.md | 68 +++++++++++++++- src/keymap.cpp | 13 +++- src/mainwindow.cpp | 33 ++++++++ tests/test_mainwindow.cpp | 90 +++++++++++++++++++++- 4 files changed, 201 insertions(+), 3 deletions(-) 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 6405e75..2e0930c 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 @@ -98,7 +98,7 @@ taking that too literally. | 47 | The query bar looks unfinished, and cannot be cleared by mouse | presentation | XS | **done** | | 48 | Removing a tag suggests every tag, not the thread's own | workflow | XS | **done** | | 49 | Sync runs every account regardless of what changed | workflow | M | open | -| 50 | Esc blanks the pane but leaves the row selected | workflow | XS | open | +| 50 | Esc blanks the pane but leaves the row selected | workflow | XS | **done** | | 51 | Clicking a subject scrolls the list sideways | presentation | XS | open | | 52 | `test_querycompleter` fails under Wayland, passes offscreen | testing | XS | **done** | @@ -1969,6 +1969,38 @@ worker call. it somewhere that opens on every launch; a dialog the user asks for is the right shape, a status-bar field refreshed continuously is not. +### Revisited 2026-08-07, after item 30 shipped + +The placeholder pane did not exist when this was written, and it now displays +database-level counts, so "where does this live" was worth asking again. **The +answer is unchanged: a dialog.** Recorded because the reasoning is not obvious +and would otherwise be re-litigated. + +**Not on the placeholder**, even though the counts machinery is right there. +Item 30's own decision was that the pane carries counts the user ACTS on, each +one a link to the query it names, plus a sync line that appears only when +something needs attention, specifically so the pane cannot become wallpaper. +Total messages, account count and tag count are reference material read once, +not things to click. Putting them there dilutes exactly what that decision +protects. + +**What did change is the cost, not the location.** This item claims a total +message count "needs a new worker call". It no longer does, quite: +`NotmuchWorker::requestCounts(QStringList, quint64)` already exists, crosses on +a queued connection and takes an arbitrary list of queries, so the dialog can +ask for whatever it wants in one round trip. + +**One trap in reusing it.** `requestCounts` counts **threads** +(`notmuch_query_count_threads`), because item 30's pane says "N in inbox" beside +a list that shows threads. This item wants **messages**. Those differ by roughly +the reply depth of the database and the difference is not small. Reusing the +call as it stands would report a confidently wrong number under the right +label. Either add a second signal, or give the existing request a mode; do not +quietly reinterpret what it returns. + +**Still true from the constraints above:** the account count comes from config, +never from notmuch, and the tag list is already fetched for the completer. + ## 35. No refresh of the thread list after a sync **Observed (user, 2026-08-04):** "auto refresh list after sync." @@ -2864,6 +2896,40 @@ matter of adding `clearSelection()` to the existing one: - Both actions need entries in the shortcut reference, which is generated, so the descriptions must distinguish them in one line. +### Outcome (done 2026-08-07) + +Built as two actions, per the user's "two actions instead of one". +`clear_selection` takes `Esc` and does both; `clear_pane` keeps its existing +behaviour on `Shift+Esc`. It needed a default rather than being left unbound: +every action carries one, and `everyActionHasAShortcut` enforces it. + +**The reload hazard is real, and both guards against it are load-bearing.** +`clearSelection()` leaves `currentIndex()` VALID, so `onSelectionChanged()` +takes its "one or fewer rows" branch, finds a current row whose id differs from +`m_currentThreadId`, and calls `onThreadSelected` for it: the thread is +re-adopted and a `loadThread` sent for the row being cleared. Clearing the +selection BEFORE blanking means that runs while `m_currentThreadId` still names +the displayed thread, so the ids match and nothing reloads; `setCurrentIndex()` +then stops a later collapse-to-one-row reaching the same row. + +All four arrangements were tried, and **only one passes**: dropping +`setCurrentIndex()` fails, and moving either line after the blanking fails. + +**The first version of the test could not tell any of them apart.** It asserted +`showingPlaceholder()`, which passes whatever the code does, because +`test_mainwindow` has no worker: `loadThread` never replies, so nothing ever +repaints the pane. That is the standing limitation `CLAUDE.md` records, met +head-on. What IS observable without a worker is `currentThreadId()`, the id the +window sets on its way to sending the request, and `currentIndex()`. Asserting +those two is what made the test discriminate. + +**A caution about mutation testing, learned the hard way here.** Two of these +conclusions were reached and reversed before the four-way comparison settled it, +once because a mutation crashed the build and the crash was read as a test +failure. A mutation that does not compile, or that dies before the assertion, +proves nothing. Check the run actually reached the assertion before believing +what it says. + ## 51. Clicking a subject scrolls the list sideways **Observed (user, 2026-08-07):** "when selecting a row by clicking on the subject, diff --git a/src/keymap.cpp b/src/keymap.cpp index a16a700..29a5f71 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -38,6 +38,7 @@ QStringList KeyMap::knownActions() QStringLiteral("complete_query"), QStringLiteral("select_all"), QStringLiteral("clear_pane"), + QStringLiteral("clear_selection"), QStringLiteral("toggle_html"), QStringLiteral("load_remote"), QStringLiteral("message_details"), @@ -85,7 +86,17 @@ QList> KeyMap::defaultBindings() // Escape is not claimed by anything else at window level. The query // completer handles its own Escape while its popup is up, and a popup // consumes the key before a window shortcut sees it. - { QStringLiteral("Esc"), QStringLiteral("clear_pane") }, + // + // It clears the SELECTION as well as the pane (item 50). Deselecting is + // what Escape means nearly everywhere else, and blanking a pane while + // leaving the row highlighted reads as half an action. + // + // clear_pane keeps the narrower behaviour on Shift+Esc: same key, and + // the modifier reads as "less than the plain one". It needs SOME + // default rather than being left unbound, since every action carries + // one and everyActionHasAShortcut enforces exactly that. + { QStringLiteral("Esc"), QStringLiteral("clear_selection") }, + { QStringLiteral("Shift+Esc"), QStringLiteral("clear_pane") }, { QStringLiteral("Ctrl+H"), QStringLiteral("toggle_html") }, { QStringLiteral("Ctrl+M"), QStringLiteral("load_remote") }, // Shifted because Ctrl+D is delete. Both are "D for details/delete" diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 5af6624..3f8a53d 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -787,6 +787,39 @@ void MainWindow::registerActions() m_markReadTimer->stop(); m_markReadThreadId.clear(); }); + addAction(QStringLiteral("clear_selection"), tr("Clear &selection"), + tr("Blank the message pane and deselect every thread"), + [this]() { + // Item 50, and the user's wording was "two actions instead of one": + // clear_pane above still blanks without touching the selection, this + // one does both. Esc defaults here, since deselecting is what Esc means + // nearly everywhere else. + // + // BOTH LINES BELOW ARE LOAD-BEARING, AND SO IS THEIR PLACE ABOVE THE + // BLANKING. clearSelection() leaves currentIndex() VALID, and + // onSelectionChanged() then takes its "one or fewer rows" branch, finds + // a current row whose id differs from m_currentThreadId, and calls + // onThreadSelected for it: the thread is re-adopted and a loadThread + // sent for the row that was just being cleared. + // + // Clearing the selection FIRST means that runs while m_currentThreadId + // still names the displayed thread, so the ids match and nothing is + // reloaded; setCurrentIndex() then stops any later collapse-to-one-row + // reaching the same row again. + // + // All four arrangements were tried against + // clearSelectionBlanksThePaneAndDeselects, and only this one passes: + // dropping setCurrentIndex() fails, and moving either line after the + // blanking fails. + m_threadView->clearSelection(); + m_threadView->setCurrentIndex(QModelIndex()); + + m_currentThreadId.clear(); + m_messageView->clear(); + showPlaceholderPane(); + m_markReadTimer->stop(); + m_markReadThreadId.clear(); + }); addAction(QStringLiteral("select_all"), tr("Select &all threads"), tr("Select every thread in the current result list"), [this]() { // A registered action rather than the view's built-in SelectAll key, so diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 7ee8a5a..2625bf5 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -76,6 +76,8 @@ private slots: void growingASelectionCancelsAnAlreadyArmedTimer(); void collapsingBackToOneRowLoadsThatThreadAgain(); void theStatusBarReportsAMultiRowSelection(); + void clearSelectionBlanksThePaneAndDeselects(); + void clearPaneLeavesTheSelectionAlone(); void theThreadListOffersAContextMenu(); void aSecondRowBlanksThePaneNotOnlyAThird(); void aLocalSyncIsNotReportedAsABackgroundOne(); @@ -1096,6 +1098,87 @@ void TestMainWindow::theStatusBarReportsAMultiRowSelection() "size, it says '%1'").arg(status->text()))); } +void TestMainWindow::clearSelectionBlanksThePaneAndDeselects() +{ + // Item 50: the user asked for "two actions instead of one", so this is the + // new action and clearPaneLeavesTheSelectionAlone() below pins the old one. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + + model->appendBatch({ makeThread(QStringLiteral("t1"), {}), + makeThread(QStringLiteral("t2"), {}) }); + + // From a row that is already current, which is both how a user reaches this + // and what CLAUDE.md requires: selectAll() on a fresh view emits no + // currentRowChanged at all, so a test starting there passes against a + // missing guard. + view->selectRow(0); + QCOMPARE(view->selectionModel()->selectedRows().size(), 1); + + auto *action = window.findChild(QStringLiteral("clear_selection")); + QVERIFY2(action, "no clear_selection action"); + action->trigger(); + + QVERIFY2(view->selectionModel()->selectedRows().isEmpty(), + "the row is still selected: this action's whole point is that it " + "deselects as well as blanking"); + + // The hazard: clearSelection() leaves currentIndex() VALID, so + // onSelectionChanged() takes its "one or fewer rows" branch, sees a current + // row whose id differs from the just-cleared m_currentThreadId, and calls + // onThreadSelected for it, which sets m_currentThreadId again and sends a + // loadThread. The pane would then repaint itself a moment later. + // + // **currentThreadId() is what detects that, not the pane.** This fixture + // has no worker, so loadThread never replies and nothing ever repaints; + // asserting showingPlaceholder() here passes whatever the code does, which + // CLAUDE.md records as the standing limit of test_mainwindow. What IS + // observable is the id the window set on its way to that request. + QVERIFY2(window.currentThreadId().isEmpty(), + qPrintable(QStringLiteral("a thread was re-adopted after the " + "selection was cleared: currentThreadId " + "is '%1', and a loadThread for it is " + "already in flight") + .arg(window.currentThreadId()))); + + // And current itself is gone, so no later collapse-to-one-row can reload + // it either. + QVERIFY2(!view->currentIndex().isValid(), + "currentIndex is still valid, so onSelectionChanged can reload " + "that row on the next selection change"); +} + +void TestMainWindow::clearPaneLeavesTheSelectionAlone() +{ + // The pre-existing action keeps its behaviour. Item 32 built it to blank + // WITHOUT touching the selection, and a user who binds it is entitled to + // that; item 50 adds a second action rather than changing this one. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + + model->appendBatch({ makeThread(QStringLiteral("t1"), {}), + makeThread(QStringLiteral("t2"), {}) }); + + view->selectRow(0); + QCOMPARE(view->selectionModel()->selectedRows().size(), 1); + + auto *action = window.findChild(QStringLiteral("clear_pane")); + QVERIFY2(action, "no clear_pane action"); + action->trigger(); + + QCOMPARE(view->selectionModel()->selectedRows().size(), 1); +} + void TestMainWindow::theThreadListOffersAContextMenu() { // Right-click is the other half of discoverability: until now every tag @@ -1503,7 +1586,12 @@ void TestMainWindow::escapeBlanksTheMessagePane() auto *action = window.findChild(QStringLiteral("clear_pane")); QVERIFY2(action, "no clear_pane action registered"); - QCOMPARE(action->shortcut(), QKeySequence(Qt::Key_Escape)); + + // Shift+Esc since item 50: plain Escape now clears the selection too, and + // this narrower action kept the same key with a modifier. The behaviour + // asserted below is unchanged, which is the point of keeping both. + QCOMPARE(action->shortcut(), + QKeySequence(Qt::ShiftModifier | Qt::Key_Escape)); auto *model = window.findChild(); QVERIFY(model); -- cgit v1.2.3