diff options
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 68 | ||||
| -rw-r--r-- | src/keymap.cpp | 13 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 33 | ||||
| -rw-r--r-- | 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<QPair<QString, QString>> 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<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTableView *>(); + 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<QAction *>(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<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTableView *>(); + QVERIFY(view); + + model->appendBatch({ makeThread(QStringLiteral("t1"), {}), + makeThread(QStringLiteral("t2"), {}) }); + + view->selectRow(0); + QCOMPARE(view->selectionModel()->selectedRows().size(), 1); + + auto *action = window.findChild<QAction *>(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<QAction *>(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<ThreadListModel *>(); QVERIFY(model); |