diff options
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 111 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 80 | ||||
| -rw-r--r-- | src/keymap.cpp | 8 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 104 | ||||
| -rw-r--r-- | src/mainwindow.h | 17 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 212 | ||||
| -rw-r--r-- | translations/qtmaildir_it_IT.ts | 36 |
7 files changed, 467 insertions, 101 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 a0337f1..206a550 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 @@ -8202,3 +8202,114 @@ matching a sent path, 780 are still stripped and 27 are spared, every one of them two files with one in another account's Inbox. No arrival is affected. **Size: S.** Done. + +## 112. Toggle unread on a whole thread cannot reach "all unread" on a partly-read thread + +**Observed (user, 2026-08-17):** clicking a thread root and asking to mark the +whole thread unread does not do it. On a seven-message thread with two unread +replies, the result is that every message is toggled unread **except those +two**, which are left as they were. The user asks for an explicit "mark whole +thread read/unread" rather than a toggle. + +**Cause (verified in code):** the action exists, and its direction is the +defect. `toggle_unread_thread` (`src/mainwindow.cpp:931`, `Ctrl+Alt+U`) chooses +between adding and removing by asking +`everySelectedRowHasTag("unread", TagScope::Thread)`, which reads +`ThreadListModel::threadFor(index).tags`. That is notmuch's **union over the +thread** (`CLAUDE.md`, item 110), so a thread containing even one unread message +answers "unread" and the action picks *Mark thread read*. There is no input a +user can give that reaches *Mark thread unread* on a mixed thread: the only +threads that take that branch are the ones already entirely read, and the only +threads reporting "not unread" are the ones the user does not need the action +for. + +The write itself is absolute and correct. `tagSelected` with `TagScope::Thread` +adds or removes `unread` across every message, so the two unread replies in the +report are not skipped by the write. They are the reason the write ran in the +opposite direction from the one the user wanted. + +**A union is not a state, and a toggle needs a state.** This is the same class +as item 110 and the third time the union has produced a defect. Items 105 and 88 +fixed *which object* a toggle resolved; this one is about a thread having no +single answer to give. `everySelectedRowHasTag` is a two-valued predicate over a +three-valued reality: all read, all unread, or mixed. The mixed case is the one +that has no correct toggle direction, and picking either one silently is what +ships as "the action does the wrong thing". + +**Approach.** The user has already named it: stop toggling at thread scope. + +- Split `toggle_unread_thread` into two explicit actions, **Mark thread read** + and **Mark thread unread**, each with a fixed direction. Both appear in the + "Whole thread" submenu, where an entry always carries text, so a fixed label + is honest in a way a toggle's cannot be. +- The message-scoped `toggle_unread` stays a toggle. One message has a real + two-valued state, so the trap does not exist there. Do not "unify" the two: + the asymmetry is the point. + +**Constraints.** + +- **Adding an action is four places**, all enforced by tests that fail + confusingly: `KeyMap::knownActions()`, `defaultBindings()`, the icon table, + and the no-duplicate-icons exception list. See `CLAUDE.md`. Splitting one + action into two means one new entry in each, and the pair shares the twin's + icon under the existing named exemption for thread actions. +- **`Ctrl+Alt+U` is taken by the action being split**, and the whole-thread + bindings are already one modifier out from their twins because `Ctrl+Shift+U` + was claimed. Two directions need two sequences; if a second chord cannot be + found that is not worse than the menu, bind one and leave the other to the + submenu rather than inventing a three-modifier chord nobody will press. +- **This interacts with items 98 and 99**, which is the reason to decide all + three together. 99 asks for a dynamic label on the message-scoped toggle, + which is the opposite move: keep the toggle, make the label tell the truth. + A thread cannot do that, because on a mixed thread there is no true label to + show. Deciding 99 first will produce the wrong answer here by analogy. +- The undo entry must name the direction that ran (`Mark thread unread`), not + the action. `tagSelected` already takes the text, so this comes free from + splitting. +- **The test needs a MIXED thread**, which is the whole defect: a thread whose + messages are all in one state answers identically whichever way the direction + is computed, so a fixture built from a uniformly-unread thread passes against + the bug. Same trap as item 88's opposite-states requirement, recorded in + `CLAUDE.md`. + +**Built 2026-08-25 to the USER'S NOTE, not to the approach above**, which had +this half right and was shipped that way first. The approach proposed splitting +the thread toggle and explicitly said to leave the message-scoped one alone, +deciding 99 separately. The user's note is ONE design across both, and the +entry's own constraint said so ("this interacts with items 98 and 99, which is +the reason to decide all three together") without following it. The half-built +version was handed over, corrected by the user, and rebuilt. + +Four parts, all of them the note's: + +- The thread toggle splits into `mark_thread_read` and `mark_thread_unread`, + both absolute. **Neither carries a default chord**, at the user's choice: + since item 132 a shortcut is a chosen subset, and `Ctrl+Alt+U` meant + whichever direction the union happened to pick, which is what made it wrong. + It is now unbound. +- The message-scoped `toggle_unread` STAYS a toggle, because one message has a + real two-valued state, and gains a label naming the direction it will go. +- On a selection with no single state that entry is **hidden**, chosen over + disabled by the user. There is no honest label for a mixed selection, and + the thread submenu is the route the note points at. +- The label follows a WRITE as well as a selection change, keyed on the + model's `dataChanged` rather than on the six call sites that apply an + optimistic update, so a new one cannot forget. Without it, marking the + current row read left the entry offering to do it again. + +`selectionTagPresence()` is the three-valued predicate this needed; +`everySelectedRowHasTag()` now delegates to it and keeps its two-valued +answer, which is all a DIRECTION needs. A label needs the third value, and +asking a two-valued predicate a three-valued question is what this item was. + +**Three mutations fail:** restoring the union predicate reports "wrong +direction on a mixed thread: Mark thread read", which is the user's original +symptom; showing the action on a mixed selection; and dropping the +`dataChanged` refresh. The suite is 37 of 38, the one failure being item 136 on +an unrelated path, and the four new strings are translated with `lrelease` +reporting 0 unfinished. + +**Closes 99 and 147 with it**, which were the same note recorded twice. + +**Size: S.** Done, at roughly twice the entry's scope because the entry's scope +was wrong. 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 1f14e67..ab346f5 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 @@ -165,7 +165,7 @@ taking that too literally. | 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 | | 97 | An edit made during a sync is reverted in the list when the sync ends | defect | S | **done** 2026-08-15, unreleased. Found by hand-testing item 89's fix. The sync-end refresh ran BEFORE the held-edit flush, so it read a database that still carried the old tag | | 98 | "Important" adds the tag but cannot remove it, unlike every other toggle | defect | XS | **done** 2026-08-17, unreleased. Calls `everySelectedRowHasTag()`, as the entry required. Its reply test needed THREE different states (list-first thread, the reply's own thread, the reply) before it could tell the two wrong answers apart; with the reply defaulted to its thread's state the item 105 mutation stayed green, measured | -| 99 | The unread action is labelled "Toggle unread" whichever way it will go | presentation | S | open; depends on 98's toggle shape, and the label is harder than it looks | +| 99 | The unread action is labelled "Toggle unread" whichever way it will go | presentation | S | **done 2026-08-25**, unreleased, with 112: the user's note is ONE design across both. The label names the direction it will go, and the entry is hidden on a selection with no single state. `refreshUnreadAction()` reads the new three-valued `selectionTagPresence()` | | 100 | The message pane offers Back, Forward, Reload and Save page, none of which mean anything | defect | XS | **done** 2026-08-17, unreleased. `MessageView::removeBrowserActions()` filters the standard menu by `pageAction()` POINTER, never by text; `ViewSource` went with them, and stranded separators are swept | | 101 | Sync is account-aware for edits but not for the account the user is looking at | workflow | S | open; item 49 built the edit half deliberately. Needs a decision, see the entry | | 102 | The rules table shows no note, so the field explaining a rule is invisible until it is opened | workflow | XS | **done** 2026-08-17, unreleased. A Note column before `ColumnCount`, so the appended Matches column stays last. Found a second defect on the way: `restoreState` REFUSES a header state with a different column count, and the sized flags were being set regardless | @@ -178,7 +178,7 @@ taking that too literally. | 106 | A tag change made on one message during a sync is silently lost | defect | XS | **done** 2026-08-16, unreleased. Found by READING while fixing 105, never reported. `flushHeldEdits` re-sent only thread-scoped edits, so a message-scoped one was shown, counted as pending, and never written | | 107 | A thread-scoped write leaves the loaded replies showing their old tags | defect | XS | **done** 2026-08-16, unreleased. `applyTagChange` updated the summary only, so marking a thread read left its expanded replies bold | | 108 | Acting on a thread root means the whole thread, though it displays one message | workflow | M | **done** 2026-08-16, unreleased. `messageScopeFor()` beside `scopeFor()`; five `*_thread` actions in a "Whole thread" submenu on `Ctrl+Alt+<key>`. User-visible: minor bump, `### Upgrading` written | -| 112 | Toggle unread on a whole thread cannot reach "all unread" on a partly-read thread | defect | S | open, found 2026-08-17. A toggle over a UNION has no direction on a mixed thread | +| 112 | Toggle unread on a whole thread cannot reach "all unread" on a partly-read thread | defect | S | **done 2026-08-25**, unreleased. Built to the user's own note rather than to this entry's approach, which had it only half right. The thread toggle splits into two absolute actions AND the message-scoped one keeps its toggle with a dynamic label, hidden when the selection disagrees. Closes 99 and 147 with it | | 113 | No way to see a message's HTML source | information | S | open, 2026-08-17. Chromium's own View source cannot work here; needs our own plain-text dialog. Item 100 removed the dead entry, which was an overreach: the user had not asked for it | | 114 | Save image is offered on every image and does nothing | defect | S | open, found 2026-08-17, re-confirmed by hand 2026-08-20. No `downloadRequested` handler exists, so the request is emitted and never answered. The handler is per-profile, so it must decide per request or it revives the Save link item 127 removed | | 115 | A copy from the message pane gives no confirmation | presentation | XS | **done** 2026-08-19, unreleased. Four entries report, each naming what it copied; connected to the page's own QActions, so the entry is covered wherever it is triggered from | @@ -216,7 +216,7 @@ taking that too literally. | 144 | "Also send a formatted copy" is prominent and does not say what it does | presentation | XS | **done** 2026-08-24, unreleased, inside 142. "Send as HTML", icon and text, alone at the right end of the editor bar where it reads as a control of the editor rather than as a formatting button. The Italian entry was refreshed with it, and `lrelease` reports 477 finished, 0 unfinished | | 145 | Cc and Bcc are permanent rows on every composer | presentation | S | **done** 2026-08-24, unreleased, inside 142. A `QToolButton` disclosure beside To:. `revealCcBccIfUsed()` is the load-bearing half the entry called for: it only ever SHOWS, never hides, so nothing but the user's own click can make a field holding an address invisible. `ComposeContext` carries no `bcc` at all, so the seeded-Bcc case can only arrive from a reopened draft, which is what its test drives. The LABEL is hidden with each field: a `QFormLayout` holds the two as separate items, so hiding the line edit alone strands a `Cc:` over empty space | | 146 | The unsynced-changes count cannot be opened to see what it counts | information | S | **duplicate of 119**, recorded 2026-08-23 from the notes. Same request, and 119 already carries the blocker: one of the four things the count sums holds no message ids, so a list cannot be complete without changing how the count is kept | -| 147 | Toggle unread reads the same whichever way it will go | presentation | S | **duplicate of 99**, recorded 2026-08-23 from the notes. The notes ask for exactly what 99 describes: "Mark as read" on an unread message and the reverse. 99 already records that the label is harder than it looks, since a multi-row selection has no single direction | +| 147 | Toggle unread reads the same whichever way it will go | presentation | S | **duplicate of 99**, recorded 2026-08-23 from the notes, and closed with it on 2026-08-25 | | 148 | Ctrl+W does not close the composer | discoverability | XS | **done** 2026-08-24, unreleased. A `QAction` parented to the composer, so it is a WindowShortcut dispatched to the active composer only and the main window's namespace is untouched, exactly like the formatting shortcuts. It calls `close()` rather than doing anything of its own: `closeEvent()` already decides whether the draft is saved, and a second route out that skipped it would lose the message. Not registered in `KeyMap`, so item 132's rules do not apply | | 149 | A reply's cursor lands on the attribution line, not on blank space | defect | XS | **done** 2026-08-24, unreleased, in TWO passes. The first fixed the cursor within each branch (`End` under Above, `Start` under Below) and the user still saw the old layout, because the branches were already right and the DEFAULT was wrong: `above` shipped, and the layout asked for is what `below` produces. Default flipped, and the composer now focuses the body whenever To: is already filled, which a Reply and a Forward always are. Both halves were invisible to the existing `theQuotePositionDecidesWhereTheQuoteLands`, which asserts the quote's position and never the cursor's | | 150 | The receive-only ribbon stays up after the message that raised it is gone | defect | S | **done** 2026-08-24, unreleased. One line in `MessageView::clear()`, beside the blocked-content bar, the stale notice and the attachment bar it already reset by hand. Only `setReceiveOnlyAccount()` hid the ribbon, which every SELECTION change reaches, so a row-to-row move was never the reproducer: it survived the FOUR routes that blank the pane without one (`clear_pane`, `clear_selection`, a new query, a multi-row selection). The first test written for it passed against the defect for exactly that reason | @@ -537,80 +537,6 @@ reaches it (item 42), so most of this exists. **Size: S** for the on-demand button, XS for the visibility half. Ask which. -## 112. Toggle unread on a whole thread cannot reach "all unread" on a partly-read thread - -**Observed (user, 2026-08-17):** clicking a thread root and asking to mark the -whole thread unread does not do it. On a seven-message thread with two unread -replies, the result is that every message is toggled unread **except those -two**, which are left as they were. The user asks for an explicit "mark whole -thread read/unread" rather than a toggle. - -**Cause (verified in code):** the action exists, and its direction is the -defect. `toggle_unread_thread` (`src/mainwindow.cpp:931`, `Ctrl+Alt+U`) chooses -between adding and removing by asking -`everySelectedRowHasTag("unread", TagScope::Thread)`, which reads -`ThreadListModel::threadFor(index).tags`. That is notmuch's **union over the -thread** (`CLAUDE.md`, item 110), so a thread containing even one unread message -answers "unread" and the action picks *Mark thread read*. There is no input a -user can give that reaches *Mark thread unread* on a mixed thread: the only -threads that take that branch are the ones already entirely read, and the only -threads reporting "not unread" are the ones the user does not need the action -for. - -The write itself is absolute and correct. `tagSelected` with `TagScope::Thread` -adds or removes `unread` across every message, so the two unread replies in the -report are not skipped by the write. They are the reason the write ran in the -opposite direction from the one the user wanted. - -**A union is not a state, and a toggle needs a state.** This is the same class -as item 110 and the third time the union has produced a defect. Items 105 and 88 -fixed *which object* a toggle resolved; this one is about a thread having no -single answer to give. `everySelectedRowHasTag` is a two-valued predicate over a -three-valued reality: all read, all unread, or mixed. The mixed case is the one -that has no correct toggle direction, and picking either one silently is what -ships as "the action does the wrong thing". - -**Approach.** The user has already named it: stop toggling at thread scope. - -- Split `toggle_unread_thread` into two explicit actions, **Mark thread read** - and **Mark thread unread**, each with a fixed direction. Both appear in the - "Whole thread" submenu, where an entry always carries text, so a fixed label - is honest in a way a toggle's cannot be. -- The message-scoped `toggle_unread` stays a toggle. One message has a real - two-valued state, so the trap does not exist there. Do not "unify" the two: - the asymmetry is the point. - -**Constraints.** - -- **Adding an action is four places**, all enforced by tests that fail - confusingly: `KeyMap::knownActions()`, `defaultBindings()`, the icon table, - and the no-duplicate-icons exception list. See `CLAUDE.md`. Splitting one - action into two means one new entry in each, and the pair shares the twin's - icon under the existing named exemption for thread actions. -- **`Ctrl+Alt+U` is taken by the action being split**, and the whole-thread - bindings are already one modifier out from their twins because `Ctrl+Shift+U` - was claimed. Two directions need two sequences; if a second chord cannot be - found that is not worse than the menu, bind one and leave the other to the - submenu rather than inventing a three-modifier chord nobody will press. -- **This interacts with items 98 and 99**, which is the reason to decide all - three together. 99 asks for a dynamic label on the message-scoped toggle, - which is the opposite move: keep the toggle, make the label tell the truth. - A thread cannot do that, because on a mixed thread there is no true label to - show. Deciding 99 first will produce the wrong answer here by analogy. -- The undo entry must name the direction that ran (`Mark thread unread`), not - the action. `tagSelected` already takes the text, so this comes free from - splitting. -- **The test needs a MIXED thread**, which is the whole defect: a thread whose - messages are all in one state answers identically whichever way the direction - is computed, so a fixture built from a uniformly-unread thread passes against - the bug. Same trap as item 88's opposite-states requirement, recorded in - `CLAUDE.md`. - -**Size: S.** The write path is already correct and thread-scoped; the work is -the action split, the four registration sites, the binding decision, and a test -over a mixed thread. - - ## 113. No way to see a message's HTML source **Observed (user, 2026-08-17):** reviewing item 100's removals, "view source diff --git a/src/keymap.cpp b/src/keymap.cpp index 6cd965a..269a7d5 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -52,7 +52,12 @@ QStringList KeyMap::knownActions() QStringLiteral("archive_thread"), QStringLiteral("delete_thread"), QStringLiteral("spam_thread"), - QStringLiteral("toggle_unread_thread"), + // Item 112 split the thread toggle in two. Neither carries a default + // chord, at the user's choice: since item 132 a shortcut is a chosen + // subset rather than a requirement, and Ctrl+Alt+U meant whichever + // direction the union happened to pick, which is what made it wrong. + QStringLiteral("mark_thread_read"), + QStringLiteral("mark_thread_unread"), QStringLiteral("flag_thread"), // Compose and send (item 123). save_message deliberately carries no // default chord: since item 132 a shortcut is a chosen subset rather @@ -177,7 +182,6 @@ QList<QPair<QString, QString>> KeyMap::defaultBindings() { QStringLiteral("Ctrl+Alt+E"), QStringLiteral("archive_thread") }, { QStringLiteral("Ctrl+Alt+D"), QStringLiteral("delete_thread") }, { QStringLiteral("Ctrl+Alt+S"), QStringLiteral("spam_thread") }, - { QStringLiteral("Ctrl+Alt+U"), QStringLiteral("toggle_unread_thread") }, { QStringLiteral("Ctrl+Alt+I"), QStringLiteral("flag_thread") }, { QStringLiteral("Ctrl+T"), QStringLiteral("edit_tags") }, // Shifted against Ctrl+T for the same reason Ctrl+Shift+U is shifted diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 6b48880..fd954b0 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -900,6 +900,14 @@ void MainWindow::buildUi() &QItemSelectionModel::selectionChanged, this, &MainWindow::onSelectionChanged); + // The label describes the SELECTION'S STATE, which a write moves without + // touching the selection: marking the current row read has to flip the + // entry to "Mark as unread" with the same row still selected. Keyed on + // the model rather than on each of the six call sites that apply an + // optimistic update, so a new one cannot forget. + connect(m_model, &QAbstractItemModel::dataChanged, this, + [this]() { refreshUnreadAction(); }); + connect(m_threadView, &QAbstractItemView::doubleClicked, this, &MainWindow::onRowDoubleClicked); @@ -1684,21 +1692,34 @@ void MainWindow::registerActions() tagSelected({ QStringLiteral("spam") }, { QStringLiteral("inbox") }, tr("Mark thread spam"), TagScope::Thread); }); - addAction(QStringLiteral("toggle_unread_thread"), tr("Toggle &unread"), - tr("Toggle the unread tag on whole threads"), [this]() { + // Two fixed directions rather than one toggle, and the asymmetry with the + // message-scoped twin is the point (item 112). `ThreadSummary::tags` is + // notmuch's UNION over the conversation, so a thread holding even one + // unread message answers "unread" and a toggle reading that predicate + // always chose "mark read": there was no input that reached "mark thread + // unread" on a mixed thread, which is exactly the thread a user wants it + // for. A union is not a state, and a toggle needs a state. + // + // The message-scoped `toggle_unread` stays a toggle, because one message + // has a real two-valued state. Do not unify them. + addAction(QStringLiteral("mark_thread_read"), tr("Mark thread &read"), + tr("Remove the unread tag from every message of the selected " + "threads"), [this]() { + m_markReadTimer->stop(); + m_markReadMessageId.clear(); + tagSelected({}, { QStringLiteral("unread") }, + tr("Mark thread read"), TagScope::Thread); + }); + addAction(QStringLiteral("mark_thread_unread"), tr("Mark thread &unread"), + tr("Add the unread tag to every message of the selected threads"), + [this]() { // Cancels the automatic mark-read for the same reason its // message-scoped twin does: a thread marked unread by hand must not be // undone a moment later by a timer armed when it was opened. m_markReadTimer->stop(); m_markReadMessageId.clear(); - - if (everySelectedRowHasTag(QStringLiteral("unread"), TagScope::Thread)) { - tagSelected({}, { QStringLiteral("unread") }, - tr("Mark thread read"), TagScope::Thread); - } else { - tagSelected({ QStringLiteral("unread") }, {}, - tr("Mark thread unread"), TagScope::Thread); - } + tagSelected({ QStringLiteral("unread") }, {}, + tr("Mark thread unread"), TagScope::Thread); }); addAction(QStringLiteral("flag_thread"), tr("&Important"), tr("Mark every message of the selected threads as important"), @@ -2040,7 +2061,8 @@ void MainWindow::buildMenus() { QStringLiteral("archive_thread"), QStringLiteral("mail-archive") }, { QStringLiteral("delete_thread"), QStringLiteral("edit-delete") }, { QStringLiteral("spam_thread"), QStringLiteral("mail-mark-junk") }, - { QStringLiteral("toggle_unread_thread"), QStringLiteral("mail-mark-unread") }, + { QStringLiteral("mark_thread_read"), QStringLiteral("mail-mark-read") }, + { QStringLiteral("mark_thread_unread"), QStringLiteral("mail-mark-unread") }, { QStringLiteral("flag_thread"), QStringLiteral("mail-mark-important") }, // Compose and send (item 123). reply_no_quote SHARES reply's icon for @@ -3468,8 +3490,43 @@ void MainWindow::showThreadContextMenu(const QPoint &pos) m_threadContextMenu->popup(m_threadView->viewport()->mapToGlobal(pos)); } +void MainWindow::refreshUnreadAction() +{ + // The user's design (item 112 and its duplicates 99/147): the label says + // which way the action will go, and on a selection with no single state + // the entry is HIDDEN rather than labelled wrongly. The thread submenu is + // then the route, whose entries are absolute and work whatever the mix. + auto *action = m_actions.value(QStringLiteral("toggle_unread")); + if (!action) + return; + + switch (selectionTagPresence(QStringLiteral("unread"))) { + case TagPresence::Every: + action->setVisible(true); + action->setText(tr("Mark as &read")); + action->setStatusTip(tr("Remove the unread tag from the selection")); + break; + case TagPresence::None: + action->setVisible(true); + action->setText(tr("Mark as &unread")); + action->setStatusTip(tr("Add the unread tag to the selection")); + break; + case TagPresence::Mixed: + // No honest label exists, so there is no label to show. Hidden rather + // than disabled, at the user's choice. + action->setVisible(false); + break; + } +} + void MainWindow::onSelectionChanged() { + // Here rather than in the currentRowChanged handler: that signal is + // emitted BEFORE the selection model is updated, so a handler reading + // selectedRows() there sees the PREVIOUS selection and would label the + // action for the rows the user just left (CLAUDE.md, verified Qt 6.11). + refreshUnreadAction(); + const QModelIndexList rows = m_threadView->selectionModel()->selectedRows(); const int selected = rows.size(); if (selected == 1) { @@ -4915,6 +4972,16 @@ QString MainWindow::currentThreadFirstMessageId() const bool MainWindow::everySelectedRowHasTag(const QString &tag, TagScope scope) const { + // Kept as the direction question, which only has two answers to give: a + // mixed selection has to go one way, and this says which. The LABEL asks + // selectionTagPresence() instead, because a label can say "these disagree" + // and a direction cannot. + return selectionTagPresence(tag, scope) == TagPresence::Every; +} + +MainWindow::TagPresence MainWindow::selectionTagPresence(const QString &tag, + TagScope scope) const +{ // What a toggle asks before choosing its direction, for both Delete and // Toggle unread. // @@ -4932,8 +4999,9 @@ bool MainWindow::everySelectedRowHasTag(const QString &tag, const QModelIndexList rows = m_threadView->selectionModel()->selectedRows(); if (rows.isEmpty()) - return false; + return TagPresence::None; + int withTag = 0; for (const QModelIndex &index : rows) { QStringList tags; if (scope == TagScope::Thread) { @@ -4979,10 +5047,13 @@ bool MainWindow::everySelectedRowHasTag(const QString &tag, tags = own.messageId.isEmpty() ? summary.firstMessageTags : own.tags; } - if (!tags.contains(tag)) - return false; + if (tags.contains(tag)) + ++withTag; } - return true; + + if (withTag == 0) + return TagPresence::None; + return withTag == rows.size() ? TagPresence::Every : TagPresence::Mixed; } ThreadSummary MainWindow::threadForCurrentRowForTesting() const @@ -5003,7 +5074,8 @@ QMenu *MainWindow::buildThreadActionsMenu(QWidget *parent) menu->addAction(m_actions.value(QStringLiteral("delete_thread"))); menu->addAction(m_actions.value(QStringLiteral("spam_thread"))); menu->addSeparator(); - menu->addAction(m_actions.value(QStringLiteral("toggle_unread_thread"))); + menu->addAction(m_actions.value(QStringLiteral("mark_thread_read"))); + menu->addAction(m_actions.value(QStringLiteral("mark_thread_unread"))); menu->addAction(m_actions.value(QStringLiteral("flag_thread"))); return menu; } diff --git a/src/mainwindow.h b/src/mainwindow.h index 951eaa4..d3c5d15 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -887,6 +887,23 @@ private: bool everySelectedRowHasTag(const QString &tag, TagScope scope = TagScope::Message) const; + /// The three-valued version of the question above, which is what a LABEL + /// needs and a toggle's direction does not. + /// + /// `everySelectedRowHasTag` answers yes or no over a reality with three + /// states: every row has the tag, none does, or they disagree. That is + /// enough to choose a direction, since a mixed selection has to go one way + /// or the other, but it cannot name the direction honestly, and item 112 + /// is what happens when a two-valued predicate is asked a three-valued + /// question. + enum class TagPresence { None, Every, Mixed }; + TagPresence selectionTagPresence( + const QString &tag, TagScope scope = TagScope::Message) const; + + /// Relabels the unread action, and hides it when the selection has no + /// single state. Called whenever the selection changes. + void refreshUnreadAction(); + void editTagsOnSelection(); /// Set once the user has answered the exit prompt, or once a sync started diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 21c3661..53eea2f 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -383,6 +383,11 @@ private slots: void editTagsOnAReplyCountsItsOwnThreadNotTheFirstInTheList(); void markCurrentThreadReadResolvesTheThreadThroughTheIndex(); void deletingAReplyRepaintsThatReplyRow(); + void theUnreadLabelSaysWhichDirectionItWillGo(); + void theUnreadLabelFollowsAWriteWithoutReselecting(); + void theUnreadActionIsHiddenOnAMixedSelection(); + void markThreadUnreadReachesAMixedThread(); + void markThreadReadAndUnreadAreSeparateActions(); void toggleUnreadOnAReplyReadsTheReplysOwnState(); void toggleUnreadOnAReplyRepaintsItInBothDirections(); void taggingTheOpenReplyUpdatesTheMessagePaneStrip(); @@ -5207,6 +5212,207 @@ void TestMainWindow::deletingAReplyRepaintsThatReplyRow() "deleting one reply marked its whole thread deleted"); } +void TestMainWindow::theUnreadLabelSaysWhichDirectionItWillGo() +{ + // The user's note: "the label for toggle unread should be dynamic. On an + // unread message it should be Mark as read, on a read message Mark as + // unread." + // + // "Toggle unread" reads the same whichever way it will go, so the only + // way to learn what it does is to press it and look. The action stays a + // toggle, because one message has a real two-valued state; what changes + // is that the label tells the truth about the direction it has chosen. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *action = window.findChild<QAction *>(QStringLiteral("toggle_unread")); + QVERIFY(action); + + model->appendBatch({ makeThread(QStringLiteral("t1"), + { QStringLiteral("unread") }), + makeThread(QStringLiteral("t2"), {}) }); + + view->setCurrentIndex(model->index(0, 0, {})); + QVERIFY2(action->text().contains(QStringLiteral("read")), + qPrintable(action->text())); + QVERIFY2(!action->text().contains(QStringLiteral("unread")), + qPrintable(QStringLiteral("an UNREAD row must offer Mark as " + "read, not: %1").arg(action->text()))); + + view->setCurrentIndex(model->index(1, 0, {})); + QVERIFY2(action->text().contains(QStringLiteral("unread")), + qPrintable(QStringLiteral("a READ row must offer Mark as unread, " + "not: %1").arg(action->text()))); +} + +void TestMainWindow::theUnreadLabelFollowsAWriteWithoutReselecting() +{ + // The label describes the selection's STATE, and a write moves that state + // without touching the selection. Marking the current row read has to + // leave the entry offering "Mark as unread" on the same row, or the menu + // offers to do again what was just done. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *action = window.findChild<QAction *>(QStringLiteral("toggle_unread")); + QVERIFY(action); + + model->appendBatch({ makeThread(QStringLiteral("t1"), + { QStringLiteral("unread") }) }); + view->setCurrentIndex(model->index(0, 0, {})); + QVERIFY2(action->text().contains(QStringLiteral("read")) + && !action->text().contains(QStringLiteral("unread")), + qPrintable(action->text())); + + action->trigger(); + + QVERIFY2(action->text().contains(QStringLiteral("unread")), + qPrintable(QStringLiteral("the label did not follow the write: " + "still offering %1 on a row it just " + "marked read").arg(action->text()))); +} + +void TestMainWindow::theUnreadActionIsHiddenOnAMixedSelection() +{ + // The other half of the same note: "on a thread with mixed states it + // should be hidden, we have a submenu for thread actions". + // + // A selection spanning an unread row and a read one has no single state, + // so no honest label exists for it. Hiding the entry sends the user to + // the thread submenu, whose entries are absolute and work regardless of + // the mix. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *action = window.findChild<QAction *>(QStringLiteral("toggle_unread")); + QVERIFY(action); + + model->appendBatch({ makeThread(QStringLiteral("t1"), + { QStringLiteral("unread") }), + makeThread(QStringLiteral("t2"), {}) }); + + // From a row that is already current, and NOT via selectAll(): a fresh + // selectAll emits no currentRowChanged at all and leaves the current + // index invalid, so a test using it passes against a missing guard + // (CLAUDE.md). + view->setCurrentIndex(model->index(0, 0, {})); + QVERIFY2(action->isVisible(), "a single row already has no single state"); + + view->selectionModel()->select( + model->index(1, 0, {}), + QItemSelectionModel::Select | QItemSelectionModel::Rows); + QCOMPARE(view->selectionModel()->selectedRows().size(), 2); + + QVERIFY2(!action->isVisible(), + qPrintable(QStringLiteral("a mixed selection still offers the " + "unread action, labelled: %1") + .arg(action->text()))); + + // ...and it comes back when the selection agrees again, or the entry + // would be gone for the rest of the session. + view->selectionModel()->select( + model->index(1, 0, {}), + QItemSelectionModel::Deselect | QItemSelectionModel::Rows); + QVERIFY2(action->isVisible(), + "the action did not return when the selection agreed again"); +} + +void TestMainWindow::markThreadUnreadReachesAMixedThread() +{ + // Item 112. The user's report: on a thread with two unread replies, asking + // to mark the whole thread unread marked it READ instead. + // + // ThreadSummary::tags is notmuch's UNION over the conversation, so a + // thread containing even one unread message answers "unread" and a toggle + // reading that predicate always picks "mark read". There was no input that + // could reach "mark thread unread" on a mixed thread: the only threads + // taking that branch were the ones already entirely read. + // + // A union is not a state. The fix is two fixed-direction actions, so this + // asserts the direction rather than the resulting tags: on a mixed thread + // BOTH directions are reachable, which is the property that was missing. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + // MIXED: the union carries `unread` because some message is unread, while + // others are not. A thread whose messages are all in one state answers + // identically whichever way the direction is computed, so a uniform + // fixture passes against the bug (CLAUDE.md, item 88's opposite-states + // requirement). + model->appendBatch({ makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }) }); + const QModelIndex thread = model->index(0, 0, {}); + QVERIFY(thread.isValid()); + QVERIFY2(model->threadFor(thread).isUnread(), + "the fixture's union does not carry unread, so this test cannot " + "reach the branch the defect lives in"); + view->setCurrentIndex(thread); + + auto *markUnread = + window.findChild<QAction *>(QStringLiteral("mark_thread_unread")); + QVERIFY2(markUnread, "mark_thread_unread does not exist: the thread toggle " + "was not split, so a mixed thread still has no way to " + "be marked unread"); + markUnread->trigger(); + + QVERIFY2(window.undoTextForTesting().contains(QStringLiteral("unread")), + qPrintable(QStringLiteral("wrong direction on a mixed thread: %1") + .arg(window.undoTextForTesting()))); + QVERIFY2(!window.undoTextForTesting().contains(QStringLiteral("Mark thread read")), + qPrintable(QStringLiteral("marked the thread READ when asked to " + "mark it unread: %1") + .arg(window.undoTextForTesting()))); +} + +void TestMainWindow::markThreadReadAndUnreadAreSeparateActions() +{ + // The other half: the read direction must still be reachable, and must be + // its own action rather than the same one answering differently. Both are + // asserted on the SAME mixed thread, which a toggle cannot do: whichever + // direction it picks, the other is unreachable there. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + model->appendBatch({ makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }) }); + const QModelIndex thread = model->index(0, 0, {}); + view->setCurrentIndex(thread); + + auto *markRead = + window.findChild<QAction *>(QStringLiteral("mark_thread_read")); + QVERIFY(markRead); + markRead->trigger(); + QVERIFY2(window.undoTextForTesting().contains(QStringLiteral("Mark thread read")), + qPrintable(window.undoTextForTesting())); + + // The old toggle must be gone rather than left beside its replacements, + // which would leave the defect reachable from the menu it still sat in. + QVERIFY2(!window.findChild<QAction *>(QStringLiteral("toggle_unread_thread")), + "toggle_unread_thread still exists beside the split actions"); +} + void TestMainWindow::toggleUnreadOnAReplyReadsTheReplysOwnState() { // The user's report: "read/unread still doesn't trigger a repaint of the @@ -5551,7 +5757,8 @@ void TestMainWindow::theThreadSubmenuIsReachableFromBothMenus() QStringLiteral("archive_thread"), QStringLiteral("delete_thread"), QStringLiteral("spam_thread"), - QStringLiteral("toggle_unread_thread"), + QStringLiteral("mark_thread_read"), + QStringLiteral("mark_thread_unread"), QStringLiteral("flag_thread"), }; @@ -7480,7 +7687,8 @@ void TestMainWindow::noTwoActionsShareAnIcon() QStringLiteral("archive_thread"), QStringLiteral("delete_thread"), QStringLiteral("spam_thread"), - QStringLiteral("toggle_unread_thread"), + QStringLiteral("mark_thread_read"), + QStringLiteral("mark_thread_unread"), QStringLiteral("flag_thread"), QStringLiteral("reply_no_quote"), }; diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index 913d151..a976f4e 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -499,6 +499,14 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Aggiunge o rimuove l'etichetta deleted</translation> </message> <message> + <source>Mark thread &read</source> + <translation>Segna conversazione come &letta</translation> + </message> + <message> + <source>Remove the unread tag from every message of the selected threads</source> + <translation>Rimuove il tag unread da ogni messaggio delle conversazioni selezionate</translation> + </message> + <message> <source>Re&ply</source> <translation>Ris&pondi</translation> </message> @@ -752,14 +760,18 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Segna conversazione come spam</translation> </message> <message> - <source>Toggle the unread tag on whole threads</source> - <translation>Inverte l'etichetta non letto su intere conversazioni</translation> - </message> - <message> <source>Mark thread read</source> <translation>Segna conversazione come letta</translation> </message> <message> + <source>Mark thread &unread</source> + <translation>Segna conversazione come &non letta</translation> + </message> + <message> + <source>Add the unread tag to every message of the selected threads</source> + <translation>Aggiunge il tag unread a ogni messaggio delle conversazioni selezionate</translation> + </message> + <message> <source>Mark thread unread</source> <translation>Segna conversazione come non letta</translation> </message> @@ -1164,6 +1176,22 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <numerusform>%1: %n conversazioni</numerusform> </translation> </message> + <message> + <source>Mark as &read</source> + <translation>Segna come &letto</translation> + </message> + <message> + <source>Remove the unread tag from the selection</source> + <translation>Rimuove il tag unread dalla selezione</translation> + </message> + <message> + <source>Mark as &unread</source> + <translation>Segna come &non letto</translation> + </message> + <message> + <source>Add the unread tag to the selection</source> + <translation>Aggiunge il tag unread alla selezione</translation> + </message> <message numerus="yes"> <source>1 thread selected (%n message(s))</source> <translation> |