diff options
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 100 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 68 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 54 | ||||
| -rw-r--r-- | src/mainwindow.h | 4 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 142 |
5 files changed, 301 insertions, 67 deletions
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 ef5c68f..30271d5 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 @@ -5524,3 +5524,103 @@ name their tags, so the UI can answer from `~/.config/mailrules/rules.json` at the moment it is asked, retroactively and with nothing stored. Reopen from there, not from the provenance design, unless real use shows the per-message distinction is what is wanted. + +## 91. Double-clicking a thread should open it on its own + +**Observed (user, notes):** "double clicking a message/thread should open it in +its own view (?) UX not sure." + +**Specified by the user, 2026-08-15**, in three cases. The view is always the +whole THREAD; what changes is which message the pane shows: + +- Double-click a single message: that message in a view by itself, loaded in + the right pane. +- Double-click a thread: the whole thread, expanded, in a view by itself, with + the FIRST message in the right pane. +- Double-click a reply inside a thread: the whole thread, expanded, in a view by + itself, with THAT REPLY in the right pane. + +So a reply does not drill down to itself alone. It drills to its thread, with +itself selected. That is the part an implementation is most likely to get wrong, +because the obvious reading of "open it by itself" is an `id:` query on the +reply, and it is not what was asked for. + +**It is NOT a second window.** The user ruled that out explicitly: "I didn't +mean open in a new window." No second `QWebEngineView`, no extra Chromium render +process, nothing reparented. + +**Cause (verified in code):** nothing is connected to `doubleClicked` on +`ThreadListView`, so a double-click today is two single-clicks and selects the +row twice. + +**Approach: this is `recoverStaleThread()` triggered by a gesture.** That +function already does the entire job for the stale-thread notice: it sets +`m_recoverThreadId` and `m_recoverMessageId`, runs a `thread:` query, expands +the thread when the row arrives, selects the target reply once the replies land, +and falls back to selecting the thread when the message has gone. Every case +above maps onto it, with the recovery target set to the double-clicked row's own +message. Reuse it rather than writing a second selection-after-query path. + +Two properties of that code are exactly what this item needs and are already +commented in place: the root card IS the thread's first message and is not among +the children, so a thread double-click is the empty-target case; and a reply +cannot be selected until the replies exist, so the first pass selects the thread +provisionally and refines on the next. + +**Expansion is not automatic and must be asked for.** Nothing in the tree +auto-expands: `expandAll` and `setExpanded` appear nowhere in `mainwindow.cpp`, +so a plain `thread:` query lands on ONE COLLAPSED CARD and the user would still +have to click the expander. `recoverStaleThread()` calls `expand()` on the +thread, which is also what asks the worker for the replies. + +**How the user gets back: nothing is built for it.** Answered by the user: +"I didn't think of a back action, usually I'd go back to a known list like +unread or inbox at that point." The filter buttons already are that and are one +click away in every view. A Back action, a history stack and a +restore-the-previous-query scheme were all drafted and are all unnecessary. + +**The undo-stack cost is inherited and acceptable.** `runQuery()` clears the +undo stack because its rows are about to be discarded, and a drill-down discards +them exactly as a typed query does. The user already expects that from the query +bar, so the gesture inherits consistent behaviour rather than a surprise. No +filter-over-the-model alternative is needed, which is what would have made this +item 40's machinery and M rather than S. + +**Constraint.** A double-click also delivers a single click first, which selects +the row and arms the mark-read timer. The drill-down must not mark a message +read that the user only passed through, and the existing single-click path is +what arms it, so the double-click handler has to cancel it the way the +multi-row branch of `onThreadSelected()` does. + +**Outcome, 2026-08-15. Shipped, hand-confirmed green by the user.** + +`onRowDoubleClicked()` resolves the row to a thread id and a message id and +hands both to `recoverStaleThread()`, which already did the entire job for the +stale-thread notice: run `thread:<id>`, expand the thread when the row arrives, +select the target message once the replies land, fall back to the root when the +message has gone. All three of the user's cases are existing paths through it, +so the feature is a connection and a resolver rather than new machinery. + +Two details that were nearly built wrong: + +- **A reply drills to its THREAD, not to itself.** The obvious reading of "open + it by itself" is `id:<reply>`, and one of the three tests exists specifically + to fail against that. +- **The row is reached through the INDEX, never `index.row()`.** A tree numbers + rows per parent, so `threadAt(row)` on a reply answers about an unrelated + thread. This is item 88's trap, avoided here by construction. + +**The first click of a double-click arms the mark-read timer**, and the handler +cancels it: a gesture that navigates must not mutate mail. The timer is armed +again for whichever row the recovery lands on, so this cancels only the arming +for the row being left. Its test asserts the timer WAS active before the +double-click, so it cannot pass by the timer never having been armed. + +**The expander keeps its own double-click.** `ThreadListView::mousePressEvent` +accepts a press inside the expander rect and returns, so Qt never pairs one into +a double-click there: double-clicking the reply count toggles expansion twice, +double-clicking anywhere else on the card drills in. That is the right split and +it falls out of the existing code rather than needing a guard. + +**Nothing was built for getting back**, per the user: the filter buttons already +are that. 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 aefa6a5..7b44e7e 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 @@ -155,7 +155,7 @@ taking that too literally. | 86 | A right-click search can replace or narrow, but never exclude | workflow | S | **done** 2026-08-14, unreleased; see `specs/2026-08-14-exclude-from-search-design.md`. Follows 85. The `extend` bool became a `SearchMode` enum across four signatures | | 89 | A sync moves the list under the user's hands, and the auto-sync skips rather than retries | workflow | XS | **done** 2026-08-15, unreleased. The timer half only: a skipped auto-sync re-arms instead of giving up. The list-churn half is **dropped**, not built: the user resolved it as a mental-model question, an Unread view is SUPPOSED to be volatile | | 90 | A saved-query button clears the account selection | workflow | S | **folded into 93** 2026-08-15. Not fixed in place: the button that misbehaves stops being a saved query at all. See `specs/2026-08-15-builtin-filters-design.md` | -| 91 | Double-clicking a thread should open it on its own | workflow | S | open, fully specified 2026-08-15 and ready to build. The view is always the whole thread, EXPANDED; the pane shows whichever row was double-clicked, so a reply drills to its thread and not to itself. It is `recoverStaleThread()` triggered by a gesture | +| 91 | Double-clicking a thread should open it on its own | workflow | S | **done** 2026-08-15, unreleased. The view is always the whole thread, EXPANDED; the pane shows whichever row was double-clicked, so a reply drills to its thread and not to itself. Reuses `recoverStaleThread()` outright | | 92 | Nothing distinguishes a tag written by a rule from one the user applied | information | M | **postponed** 2026-08-15 at the user's request: "I don't see the utility, so I don't really know how to answer." Needs per-MESSAGE provenance nothing records, a two-repo format change blank on all existing mail. Reopen only if the need appears in use | | 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 | @@ -493,72 +493,6 @@ bump either way: an ignored optional field is not a breaking change. **Size: S.** Removing a field, two UI affordances and their tests. -## 91. Double-clicking a thread should open it on its own - -**Observed (user, notes):** "double clicking a message/thread should open it in -its own view (?) UX not sure." - -**Specified by the user, 2026-08-15**, in three cases. The view is always the -whole THREAD; what changes is which message the pane shows: - -- Double-click a single message: that message in a view by itself, loaded in - the right pane. -- Double-click a thread: the whole thread, expanded, in a view by itself, with - the FIRST message in the right pane. -- Double-click a reply inside a thread: the whole thread, expanded, in a view by - itself, with THAT REPLY in the right pane. - -So a reply does not drill down to itself alone. It drills to its thread, with -itself selected. That is the part an implementation is most likely to get wrong, -because the obvious reading of "open it by itself" is an `id:` query on the -reply, and it is not what was asked for. - -**It is NOT a second window.** The user ruled that out explicitly: "I didn't -mean open in a new window." No second `QWebEngineView`, no extra Chromium render -process, nothing reparented. - -**Cause (verified in code):** nothing is connected to `doubleClicked` on -`ThreadListView`, so a double-click today is two single-clicks and selects the -row twice. - -**Approach: this is `recoverStaleThread()` triggered by a gesture.** That -function already does the entire job for the stale-thread notice: it sets -`m_recoverThreadId` and `m_recoverMessageId`, runs a `thread:` query, expands -the thread when the row arrives, selects the target reply once the replies land, -and falls back to selecting the thread when the message has gone. Every case -above maps onto it, with the recovery target set to the double-clicked row's own -message. Reuse it rather than writing a second selection-after-query path. - -Two properties of that code are exactly what this item needs and are already -commented in place: the root card IS the thread's first message and is not among -the children, so a thread double-click is the empty-target case; and a reply -cannot be selected until the replies exist, so the first pass selects the thread -provisionally and refines on the next. - -**Expansion is not automatic and must be asked for.** Nothing in the tree -auto-expands: `expandAll` and `setExpanded` appear nowhere in `mainwindow.cpp`, -so a plain `thread:` query lands on ONE COLLAPSED CARD and the user would still -have to click the expander. `recoverStaleThread()` calls `expand()` on the -thread, which is also what asks the worker for the replies. - -**How the user gets back: nothing is built for it.** Answered by the user: -"I didn't think of a back action, usually I'd go back to a known list like -unread or inbox at that point." The filter buttons already are that and are one -click away in every view. A Back action, a history stack and a -restore-the-previous-query scheme were all drafted and are all unnecessary. - -**The undo-stack cost is inherited and acceptable.** `runQuery()` clears the -undo stack because its rows are about to be discarded, and a drill-down discards -them exactly as a typed query does. The user already expects that from the query -bar, so the gesture inherits consistent behaviour rather than a surprise. No -filter-over-the-model alternative is needed, which is what would have made this -item 40's machinery and M rather than S. - -**Constraint.** A double-click also delivers a single click first, which selects -the row and arms the mark-read timer. The drill-down must not mark a message -read that the user only passed through, and the existing single-click path is -what arms it, so the double-click handler has to cancel it the way the -multi-row branch of `onThreadSelected()` does. ## Deferred, unsized, or split out diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index ba803bc..fcf97df 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -709,6 +709,9 @@ void MainWindow::buildUi() &QItemSelectionModel::selectionChanged, this, &MainWindow::onSelectionChanged); + connect(m_threadView, &QAbstractItemView::doubleClicked, + this, &MainWindow::onRowDoubleClicked); + m_messageView = new MessageView(central); m_messageView->setTagColors(&m_tagColors); connect(m_messageView, &MessageView::statusMessage, @@ -3104,6 +3107,57 @@ void MainWindow::recoverStaleThread(const QString &threadId, m_recoverMessageId = messageId; } +void MainWindow::onRowDoubleClicked(const QModelIndex &index) +{ + if (!index.isValid()) + return; + + // The whole thread in every case, and the double-clicked row's own message + // in the pane. A reply therefore drills to its THREAD with itself selected, + // never to itself alone: "double click on a reply in a thread should still + // load the whole thread expanded in a view by itself, with the reply I + // clicked on visible in the right pane" (item 91). An id: query on the + // reply is the obvious reading of "open it by itself" and is the wrong one. + // + // Reached through the INDEX rather than through index.row(): a tree numbers + // rows per parent, so a reply's row indexes its siblings and threadAt() on + // one answers about an unrelated thread. + QString threadId; + QString messageId; + if (m_model->isMessageRow(index)) { + const MessageNode node = m_model->messageAt(index); + threadId = node.threadId; + messageId = node.messageId; + } else { + threadId = m_model->data(index, ThreadListModel::ThreadIdRole).toString(); + // The thread's first message, so the pane opens on it rather than on + // nothing. Empty is fine and means the same thing to the recovery: land + // on the root, which IS that message. + messageId = m_model->data(index, ThreadListModel::MessageIdRole).toString(); + } + + if (threadId.isEmpty()) + return; + + // The first click of the double-click already selected this row and armed + // the mark-read timer. The user is passing through on their way into the + // thread, and a gesture that navigates must not mutate mail, so the timer + // goes the same way it does for a multi-row selection. + // + // Not a correction of the single click's behaviour: the thread is about to + // be opened and its message read, which arms the timer again for the row + // the recovery selects. What is cancelled is the arming for a row the user + // is leaving. + m_markReadTimer->stop(); + m_markReadThreadId.clear(); + + // Reuses the stale-thread recovery outright, which already runs thread:<id>, + // expands the thread when the row arrives, selects the target message once + // the replies land, and falls back to the root when the message has gone. + // Every one of item 91's three cases is one of those paths. + recoverStaleThread(threadId, messageId); +} + void MainWindow::applyPendingRecovery() { if (m_recoverThreadId.isEmpty()) diff --git a/src/mainwindow.h b/src/mainwindow.h index 2a86d12..614430f 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -384,6 +384,10 @@ private slots: /// point the button uses. void recoverStaleThread(const QString &threadId, const QString &messageId); + /// Drills into the double-clicked row: the whole thread, expanded, alone in + /// the view, with that row's own message in the pane. + void onRowDoubleClicked(const QModelIndex &index); + /// Selects the remembered message once its thread's rows have loaded. void applyPendingRecovery(); void onThreadsReady(const QVector<ThreadSummary> &threads, quint64 generation); diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 253d3e6..f4357c6 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -233,6 +233,9 @@ private slots: void recoveringAStaleThreadQueriesTheWholeThread(); void recoveryReselectsTheMessageThatWasBeingRead(); void recoveryOnTheFirstMessageSelectsTheThreadRow(); + void doubleClickingAThreadOpensThatThreadAlone(); + void doubleClickingAReplyOpensItsThreadNotTheReplyAlone(); + void doubleClickingDoesNotLeaveTheMarkReadTimerArmed(); void aUserQueryAbandonsAPendingRecovery(); void blankingThePaneAlsoDropsTheStaleNotice(); void aNewQueryDropsTheStaleNotice(); @@ -3163,6 +3166,145 @@ void TestMainWindow::recoveryOnTheFirstMessageSelectsTheThreadRow() QCOMPARE(model->threadAt(current.row()).threadId, QStringLiteral("T1")); } +void TestMainWindow::doubleClickingAThreadOpensThatThreadAlone() +{ + // Item 91, the thread case: "double click on a thread loads the whole + // thread expanded in a view by itself and load the first message in the + // right pane." + // + // The view becomes thread:<id>. Asserted on the query text because that is + // what makes it a view "by itself"; a gesture that only expanded the row in + // place would leave every other thread on screen and pass any assertion + // about the expansion alone. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + + ThreadSummary first = makeThread(QStringLiteral("T1"), {}); + first.totalCount = 3; + model->appendBatch({ first, makeThread(QStringLiteral("T2"), {}) }); + QCOMPARE(model->rowCount(QModelIndex()), 2); + + const QModelIndex thread = model->index(0, 0, QModelIndex()); + QVERIFY(thread.isValid()); + + emit view->doubleClicked(thread); + + QCOMPARE(queryEdit->text(), QStringLiteral("thread:T1")); + + // The recovery target is what carries the expansion and the selection + // across the two round-trips this takes. Without it the query would run and + // land on a collapsed card with a blank pane. + QVERIFY2(window.hasPendingRecoveryForTesting(), + "the double-click ran a query but asked for nothing to be " + "expanded or selected in the result"); +} + +void TestMainWindow::doubleClickingAReplyOpensItsThreadNotTheReplyAlone() +{ + // Item 91, the case most likely to be built wrong: "double click on a reply + // in a thread should still load the whole thread expanded in a view by + // itself, with the reply I clicked on visible in the right pane." + // + // So NOT id:<reply>. The obvious reading of "open it by itself" is a query + // for that one message, and it is not what was asked for: the view is the + // thread, the pane is the reply. + const Config config; + MainWindow window(config); + + auto *queryEdit = window.findChild<QLineEdit *>(); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<ThreadListView *>(); + QVERIFY(view); + + ThreadSummary summary = makeThread(QStringLiteral("T1"), {}); + summary.totalCount = 3; + model->appendBatch({ summary }); + + const QModelIndex thread = model->index(0, 0, QModelIndex()); + QVERIFY(thread.isValid()); + + // Replies exist only once the tree has loaded, which is what gives this + // test a child row to double-click at all. + 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 replyRow = model->index(0, 0, thread); + QVERIFY2(replyRow.isValid(), "the fixture built no reply row"); + QVERIFY2(model->isMessageRow(replyRow), "that row is not a message row"); + + emit view->doubleClicked(replyRow); + + // The THREAD, not the reply. A query of id:m1@example.org here would be the + // defect this test exists for. + QCOMPARE(queryEdit->text(), QStringLiteral("thread:T1")); + QVERIFY2(window.hasPendingRecoveryForTesting(), + "nothing was remembered to select the reply once it comes back"); +} + +void TestMainWindow::doubleClickingDoesNotLeaveTheMarkReadTimerArmed() +{ + // A double-click delivers a single click FIRST, which selects the row and + // arms the mark-read timer. The user is passing through on their way to + // opening the thread, so the message must not be marked read behind the + // drill-down: the timer that the first click armed has to be cancelled. + // + // This is the same reasoning the multi-row branch of onThreadSelected() + // uses, where a selection gesture must not mutate mail. + 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 *timer = window.findChild<QTimer *>(QStringLiteral("markReadTimer")); + QVERIFY(timer); + + model->appendBatch({ makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }) }); + const QModelIndex thread = model->index(0, 0, QModelIndex()); + QVERIFY(thread.isValid()); + + // The single click a real double-click delivers first. Asserted, so this + // test cannot pass by the timer never having been armed at all. + selectThreadRow(view, 0); + QVERIFY2(timer->isActive(), + "the selection did not arm the timer, so this proves nothing"); + + emit view->doubleClicked(thread); + + QVERIFY2(!timer->isActive(), + "the drill-down left a mark-read armed for a message the user " + "only passed through"); +} + void TestMainWindow::aUserQueryAbandonsAPendingRecovery() { // A recovery spans two round-trips, so the user can type a query in the |