From 53e7c1d4497909dc3aa9871e0e0fafb071fc8734 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 17 Aug 2026 13:49:55 +0200 Subject: docs: reconcile the backlog, close 98/100/102, drop 78 and 116 The reconciliation against the user's own notes found ZERO unrecorded entries, the first clean pass: the 2026-08-16 sweep added items 98 to 104 and those absorbed the whole current "Not done yet" list. Closed this session, sections moved to the closed-items file: 98, 100, 102. Dropped: - 78, at the user's request. Never a defect. Items 85, 23 and 81 already give the whole journey (right-click a value, search it, save the query, make a rule from it); this was only a shortcut across it, and the entry had already said to gather usage evidence first. That evidence never appeared. - 116, the same day it was raised, and its section is kept for the process failure rather than the non-bug. Copy image was reported as copying markup instead of pixels. Two explanations were eliminated by real evidence, and the conclusion drawn was that something more interesting must be wrong; the actual answer was that the measurement distinguishing them was broken. A wl-paste reading taken minutes after the copy showed text flavours only, was explicitly labelled unreliable in the entry, and was then reasoned from anyway. Run immediately after a copy it reports image/png and 30 more, and pasting into GIMP immediately works. A caveat that does not stop the reasoning it qualifies is decoration. Opened: - 112, Toggle unread on a whole thread cannot reach "all unread" on a partly-read thread. The direction comes from notmuch's UNION over the thread, so one unread message anywhere makes the action pick "mark read" and no input reaches the other branch. Third defect from that union after 110. - 113, view source as our own plain-text dialog. - 114, Save image is offered and does nothing: no downloadRequested handler exists anywhere. The user corrected the first proposal, which would have refused remote images on security grounds; once remote content is granted the bytes are already fetched, so saving them is a local copy and blocking it protects nothing. - 115, no confirmation when a copy succeeds. - 117, the pane offers no Select all. NOT caused by item 100: verified against a build with that filter reverted. Three wrong theories preceded that measurement, and the lesson is one item 100 had already written down: a menu built by hand proves nothing about the menu Chromium builds. The changelog's Unreleased section gains Important-as-a-toggle, the rules Note column, the menu fix, and two Upgrading notes. --- CHANGELOG.md | 30 ++ .../2026-08-03-post-0.1.0-usability-closed.md | 294 ++++++++++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 450 ++++++++++++--------- 3 files changed, 593 insertions(+), 181 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ca51f1..f1519a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,18 @@ Existing `[keys]` entries keep working and keep their meaning: `delete` is still `flag_thread`) and can be rebound like any other. "Mark all read" is unchanged: it never used the selection. +**Important is now a toggle.** `Ctrl+I` on something already marked important +removes the tag, where before it re-applied it and appeared to do nothing. If +you were using it to mark a batch that already contained important messages, +note that a selection where *everything* is already important now unmarks all of +it, matching how Delete and Toggle unread have always behaved. + +**The tagging rules window forgets its column widths once.** A Note column was +added to the rule list, and a saved layout describing the old set of columns +cannot be applied to the new one. The first time you open the dialog after +upgrading, the columns are sized to fit; drag them once and the new layout is +remembered as before. + ### Changed - **Reading a message no longer marks its replies read.** The two-second @@ -50,9 +62,27 @@ it never used the selection. conversation; once a thread is expanded, that row cannot say which of its messages are unread, and dimming alone was too quiet to notice. Replies stay a size smaller than their thread, so the two kinds of row still read apart. +- **Important is a toggle**, like Delete and Toggle unread beside it. It only + ever added the tag, so pressing it on something already important re-applied + a tag that was already there, which changes nothing and looked like a dead + key. Removing `flagged` previously meant opening the tag dialog. As with + Delete, one direction is chosen for the whole selection: it unmarks only when + every selected row is already important, so a single keystroke cannot leave a + selection in two states. +- The tagging rules window shows each rule's **Note** in the list. The field + explaining why a rule is shaped the way it is was only visible after + selecting that rule and reading the form, which is the wrong way round for + the one column that says what a rule is for. Long notes are elided with the + full text in the tooltip. ### Fixed +- The message pane's right-click menu offered **Back, Forward, Reload** and + **Save page**. The pane is not a browser: every message is rendered from + memory with no history and no network, so all four were inert as well as + meaningless. The menu now carries only what applies to a message: Copy, View + source, and searching for the selected text. + - Acting on a reply inside an expanded thread could read a different thread's state, because a reply's position is counted within its own thread and was being used as a position in the whole list. The first reply of any thread 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 3620684..f46fec5 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 @@ -6318,3 +6318,297 @@ rather than stepping down mid-row. does not carry, and has no second tier to show. **Size: S.** + + +## 78. No way to build a rule from something visible in a message + +**Observed.** The user would like to select an address or another piece of a +message in the main window, right-click, and be offered a rule built from it. + +**Cause.** Not a defect, unbuilt. + +**Narrowed 2026-08-14, and most of the work is already done.** The search half +shipped as item 85, which is the road a rule is made from: search for a value, +save the query, create a rule from the saved query. What remains here is a +SHORTCUT across that road, and both of its halves now exist. + +- The menus are built and every surface already extracts its value as a + finished query (`SearchOffer`, `src/searchterm.h`). A rule entry is another + action beside the two search ones, not new plumbing. +- The seeded-dialog path exists from item 81: + `MainWindow::showTagRulesDialog(const TagRule &seed)`. A rule from a message + becomes a second caller of it, with a different seed, which is what item 81's + spec anticipated when it made the seed a whole `TagRule` rather than a query + string. + +**Approach.** Decide it after using item 85 for a while. Which values are worth +promoting straight to a rule is a usage question, and the earlier answer to it +was wrong (see below), so it is worth having the evidence first. + +**Constraints.** The original approach here said to start from the thread +list's context menu "where the sender is already a value the model holds". +**That is false and item 85 verified it.** `ThreadSummary::authors` comes from +`notmuch_thread_get_authors` and is a DISPLAY SUMMARY, reading `Alice, Bob` or +`Alice| Bob`, so a `from:` built from it matches nothing. A real address comes +from `MessageNode::from` or `ParsedMessage::from`, neither of which the thread +list carries. Any thread-list entry needs an address resolved from a message +first. + +JavaScript is disabled in the profile and must stay disabled. Item 85 reads a +body selection with `QWebEnginePage::selectedText()`, which injects no script; +reuse that rather than adding anything. + +The rules file is shared with mailctl, so a rule created here must go through +`TagRules` and preserve unknown fields; see "Changing the shared rule format" +in CLAUDE.md. + +**Size: S**, down from M now that item 85 has built the menus and item 81 the +seeded dialog. + +**Dropped 2026-08-17** at the user's request, and it never was a defect. The +whole journey already exists: right-click any value to search it (item 85), save +that query (item 23), turn the saved query into a rule (item 81). This item was +only a shortcut across those three steps, and its own Approach had already said +to use 85 for a while first and see which values were worth promoting. That +evidence never appeared, which is itself the answer. Reopen if a value turns up +that is worth one click. + + +## 98. "Important" adds the tag but cannot remove it, unlike every other toggle + +**Observed (user, from the notes):** "the add 'Important' action should be a +toggle (like unread)." + +**Cause (verified in the code).** `src/mainwindow.cpp:863` registers `flag` as a +one-way add: + +```cpp +addAction(QStringLiteral("flag"), tr("&Important"), + tr("Mark the selected threads as important"), [this]() { + tagSelected({ QStringLiteral("flagged") }, {}, tr("Mark important")); +}); +``` + +Adding a tag that is already there is a no-op the user cannot see, so pressing +the key or the button on an already-important thread appears to do nothing at +all. Nothing in the UI removes `flagged` except the general tag dialog. + +**The two neighbouring actions are already toggles**, so this is an +inconsistency rather than a missing feature. `delete` +(`src/mainwindow.cpp:825`) and `toggle_unread` (`:867`) both read the current +state and choose a direction, and `delete`'s comment states the rule this should +follow: one direction for the WHOLE selection, flipping only when every selected +thread is already in the target state, because a single keystroke that leaves a +selection in two states is worse than either outcome. + +**Everything needed is already loaded.** `ThreadSummary::isFlagged()` +(`src/types.h:64`) reads the tag off the summary, so the direction can be +decided without a worker round trip, exactly as `isDeleted()` is. + +**Approach, now a two-line change.** Item 105 extracted +`MainWindow::everySelectedRowHasTag()`, which is the whole of the direction +logic: +`everySelectedRowHasTag("flagged") ? tagSelected({}, {"flagged"}, tr("Unmark important")) : the current add`. +The undo stack needs nothing new, since `TagChange::inverted()` already covers +both directions. + +**Constraints.** + +- Call `everySelectedRowHasTag()`, never a hand-rolled loop. Two separate fixes + went into that logic on 2026-08-16 (items 88 and 105) and both were bugs a + copy of the then-current `delete` loop would have inherited: resolving a + reply's row number to the wrong thread, and asking a reply's thread instead + of the reply. +- The action's tooltip says "Mark the selected threads as important" and would + become wrong. Item 99 is the same problem for `toggle_unread` and the two + should be decided together. +- The label question belongs to item 99, not here. This item is the behaviour + only: the key stops being a no-op. + +**Size: XS.** + +**Done 2026-08-17, unreleased.** Two lines, as the entry predicted, calling +`everySelectedRowHasTag()`. The tooltip became "Add or remove the important +tag", matching Delete's wording; the label itself is still item 99's. + +**The test was the work, and its first version was too weak to see the bug it +was written for.** The reply case needs THREE different states, not two: the +first thread in the list unflagged, the reply's own thread flagged, and the +reply itself unflagged. Only then do the two wrong answers (a row number +resolved against the top-level list, and the reply's thread instead of the +reply) differ from the right one. Built the obvious way, with the reply +defaulted to its thread's tags, the mutation putting item 105's bug back stayed +GREEN, measured, for exactly the reason `CLAUDE.md` records about item 88's +opposite-states requirement. The fixture helper defaults `replyTags` to the +thread's, so a test that does not pass them explicitly asserts nothing about +scope. + + +## 100. The message pane offers Back, Forward, Reload and Save page, none of which mean anything + +**Observed (user, from the notes):** "back/forward/save page in the right pane +don't make sense, shouldn't be visible." + +**Cause (verified in the code).** `MessageView::showBodyContextMenu` +(`src/messageview.cpp:619`) starts from Chromium's own menu: + +```cpp +QMenu *menu = m_view->createStandardContextMenu(); +``` + +That menu is built for a browser and carries the navigation and page actions +whole. The pane is not a browser: every document arrives through `setHtml()` +with a fixed base URL, so there is no history to go back to, nothing to reload +from, and the request interceptor blocks everything by default anyway. The +entries are inert as well as meaningless. + +**Deliberate as far as it goes.** The comment above the call says the page's own +menu comes first so "copy, select all and the rest stay exactly as they were", +which is right for the editing actions and wrong for the navigation ones. The +item is that the filter was never applied, not that the base menu was a mistake. + +**Approach.** Keep the menu, drop the actions that cannot apply. Qt names them +as `QWebEnginePage::WebAction` values (`Back`, `Forward`, `Reload`, +`SavePage`, and `ViewSource` is worth the same look), and each has a +`pageAction()` whose pointer can be matched against the standard menu's entries +and removed. Removing by matching the action pointer is safer than matching by +text, which is translated. + +**Constraints.** + +- Do not rebuild the menu from scratch. Copy, Copy link address and Select all + are the reason the standard menu is used, and item 85's search entries are + appended to it. +- `Save page` is not the attachment save. Attachments have their own bar and + their own path-traversal checks (see the web view security notes in + `CLAUDE.md`); nothing here should grow a second way to write a file. +- Verify against a real right-click on a real message. The offscreen platform + builds the menu but a screenshot of it proves nothing, and the entry list + depends on what the page reports as available at that moment. + +**Size: XS.** + +**Done 2026-08-17, unreleased.** `MessageView::removeBrowserActions(QMenu *, +QWebEnginePage *)`, static and taking both, so a test builds a menu and checks +it without a rendered document or a shown popup. `ViewSource` went with the +four the user named, for the same reason. Copy and Select all are asserted to +SURVIVE, since they are why the standard menu is used rather than rebuilt. + +Two things worth keeping. The match is on the `page->action()` POINTER, and the +test asserts through pointers too: matching on text would pass in English and +fail in every other locale, which is a defect no test written in English would +catch. And removing entries strands separators at the edges or doubles them up, +which reads as a menu that lost something, so the filter sweeps them; Qt offers +nothing for this. + +**Still worth a real right-click.** The offscreen platform builds the menu, and +what Chromium offers depends on what is under the cursor at that moment. + + +## 102. The rules table shows no note, so the field explaining a rule is invisible until it is opened + +**Observed (user, from the notes):** "add 'notes' column to the filters table." + +**Cause (verified in the code).** `TagRule` carries a `note` field +(`src/tagrules.h:37`, "Why the rule is shaped this way. Shown in the dialog"), +and the editor below the table edits it, but the table itself lists five columns +and none of them is the note (`src/tagrulesdialog.cpp:116`): + +```cpp +m_list->setHeaderLabels({ tr("On"), tr("Stage"), tr("Rule"), tr("Tags"), + tr("Matches") }); +``` + +So the one field written specifically to explain a rule can only be read one rule +at a time, by selecting it. With several rules the note is exactly the thing that +would let the user pick the right one without opening each. + +**Approach.** A sixth column. The column widths already persist (item 75), so a +new column needs a sensible default width and nothing else in the way of state. + +**Constraints.** + +- The note is free text of any length and would stretch the column. Elide it and + put the full text in the tooltip; the `Rule` column already faces the same + problem with a long query and is the pattern to match. +- `ColumnCount + 1` in `setColumnCount` is load-bearing: the enum drives the + column indices and there is a spare. Add the enum value rather than hardcoding + 5, and check every place that indexes a column by number. +- Notes are the user's own words and can be empty. An empty cell is correct + here; do not substitute a placeholder. + +**Size: XS.** + +**Done 2026-08-17, unreleased.** `ColumnNote` inserted BEFORE `ColumnCount`, +which matters: "Matches" is appended past the end of the enum, so a column added +after it would put the counts under Note. The cell is `simplified()`, because a +note is free text and a newline truncates a tree row at it; the full text is the +cell's tooltip and is untouched in the editor. + +**A second defect fell out of it, and it was not in the plan.** +`QHeaderView::restoreState` REFUSES a state saved with a different column count, +returning false and leaving the header alone, which is what every existing +`uistate.conf` now does. The restore path set `m_columnsSized` and +`m_countColumnSized` regardless, spending the one auto-size each column gets on +a restore that did nothing: the new Note column would have opened at its default +width, once, permanently. Now guarded on the return value. Upgrading costs one +reset of the rule dialog's column widths, which is unavoidable, since the saved +state genuinely describes a table that no longer exists. + + +## 116. Copy image copies markup instead of the image + +**Dropped 2026-08-17, the same day it was raised. There is no defect here**, and +the way this item was reasoned about is the part worth keeping. + +**Observed (user):** Copy image on a displayed remote image appeared to put +`` on the clipboard, and GIMP reported "no image data in +the clipboard to paste" while pasting fine from other sources. + +**Measured, correctly, on the second attempt.** `wl-paste --list-types` run +IMMEDIATELY after a copy reports: + +``` +application/x-qt-image +text/html +text/x-moz-url +image/png +image/bmp +... 30 more image flavours +``` + +Chromium puts the pixels on the clipboard, in `image/png` and +`application/x-qt-image` among many others. The clipboard is correct and nothing +in this application is involved: `grep -rn "clipboard\|Clipboard" src/` is +empty. + +**Confirmed from the other end too** (user, same day): copying and then pasting +into GIMP IMMEDIATELY works. The original failing paste was against a clipboard +that had already moved on, which is the same staleness that produced the wrong +`wl-paste` reading below. Both halves of the false report had one cause. + +**The process failure is the reason this entry survives its own drop.** An +earlier `wl-paste --list-types` reported text flavours only, and an entire cause +was built on it: "Chromium is not putting pixels on the clipboard, and the paste +target is blameless", with a three-step plan, a Wayland suspect, and a size of +`?`. That reading was taken minutes after the copy, off a clipboard that had +been overwritten in between. The staleness was even NOTED in the entry as a +caveat, and the theory was written as though it had not been. + +Three lessons, in the order they were paid for: + +- **A clipboard reading is only valid immediately after the copy.** It is + shared, global, mutable state that any application can overwrite at any + moment. Treat a late reading as no reading. +- **Do not theorise past your own caveat.** The measurement was correctly + labelled unreliable and then used as though it were reliable. A caveat that + does not stop the reasoning it qualifies is decoration. +- **Two eliminated explanations are not proof of a third.** Both original + readings were ruled out by real evidence, and the conclusion drawn was that + something more interesting must be wrong. The actual answer was that the + measurement distinguishing them was broken. + +**If the paste is still wanted**, the question is why GIMP does not take an +`image/png` that is demonstrably on the clipboard: a Wayland clipboard-manager +interaction, or GIMP's own paste path. Neither is this repository's, and neither +needs an item here until it is shown to be. 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 944dfee..82860a3 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 @@ -144,7 +144,7 @@ taking that too literally. | 75 | The tagging rules window forgets its size and its column widths | persistence | S | **done** 2026-08-13, shipped in 0.17.0. The window-kind question is left open, see the closed-items file | | 76 | Every field in the rules dialog is free text, so a rule is easy to get wrong | workflow | M | **done** 2026-08-13, shipped in 0.17.0. See `specs/2026-08-13-rule-builder-design.md` | | 77 | No way to see what a rule would collect, in the thread list | workflow | S | **done** 2026-08-13, shipped in 0.17.0 | -| 78 | No way to build a rule from something visible in a message | workflow | S | open, narrowed 2026-08-14; the search half shipped as item 85, which is the road a rule is made from. Now a shortcut across that road: the menus and the seeded-dialog path both exist. Use 85 first and see which values are worth promoting | +| 78 | No way to build a rule from something visible in a message | workflow | S | **dropped** 2026-08-17 at the user's request. Never a defect: item 85 built the road (right-click any value, search it, save the query) and item 81 the last step (a rule from a saved query), so the whole journey is available. This was only a shortcut across it, and the entry had already said to use 85 for a while before deciding which values were worth promoting. Reopen if that use turns up a value worth a one-click rule | | 80 | A rule with many conditions squeezes the rule list to one visible row | defect | XS | **done** 2026-08-13, shipped in 0.17.0. Follows item 76 | | 79 | Opening the rules dialog and saving destroys the first rule | defect | XS | **fixed on `rule-builder`** 2026-08-13, unreleased. Shipped in 0.16.0; damaged one real rule, repaired by hand | | 81 | No way to turn a saved query into a tagging rule | workflow | S | **done** 2026-08-14, unreleased; see `specs/2026-08-14-query-to-rule-design.md` | @@ -162,11 +162,11 @@ taking that too literally. | 94 | `pinned` has nothing left to decide once the buttons are built-in | maintenance | S | open; **blocked on 93**, and deliberately not part of it. A user-visible removal: the row becomes built-ins only and every saved query lives in the menu | | 96 | A query returning the thread already on display opens onto the placeholder | defect | S | **done** 2026-08-15, unreleased. Split from 66's unverified half, which had a different cause. Reproduced from two screenshots after four measured eliminations | | 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 | open, found 2026-08-16 in the notes reconciliation | +| 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 | -| 100 | The message pane offers Back, Forward, Reload and Save page, none of which mean anything | defect | XS | open, found 2026-08-16. Chromium's standard menu is added wholesale | +| 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 | open, found 2026-08-16 | +| 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 | | 103 | What Delete does to mail on the server is undocumented and unverified | clarification | S | open; a question first, possibly no code at all | | 104 | Mail visible in Thunderbird never reaches qtmaildir | defect | ? | open, reported 2026-08-16, cause NOT established. Most likely outside this repo; see the entry before writing code | | 109 | A root card's own message is invisible to a message-scoped write | defect | S | **done** 2026-08-16, unreleased. Found by hand-testing 108. `applyMessageTagChange` and `messageById` searched only the loaded replies, and a root's message is never among them, so the ORDINARY gesture repainted nothing and wiped the pane's chip row | @@ -176,6 +176,12 @@ 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+`. 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 | +| 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 by right-clicking a real image. No `downloadRequested` handler exists anywhere, so the request is emitted and never answered | +| 115 | A copy from the message pane gives no confirmation | presentation | XS | open, 2026-08-17. Copy link address, Copy image address and Copy image all work, silently. `statusMessage` already exists and already expires | +| 116 | Copy image copies markup instead of the image | defect | XS | **dropped** 2026-08-17, same day. NOT A DEFECT: `wl-paste --list-types` run immediately after a copy reports `image/png`, `application/x-qt-image` and 30 more image flavours. The clipboard is correct and Chromium is behaving. The earlier "text only" reading was taken minutes late off a clipboard that had been overwritten, and a whole cause was theorised on it | +| 117 | The message pane offers no Select all | workflow | XS | open, found 2026-08-17. NOT caused by item 100: verified by hand against a build with that filter reverted, and the menu holds Copy and the search entries either way | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -342,52 +348,6 @@ Those are three different features. **Size: `?`, unspecified**, and out of scope until v2 exists. Ask before designing anything. -## 78. No way to build a rule from something visible in a message - -**Observed.** The user would like to select an address or another piece of a -message in the main window, right-click, and be offered a rule built from it. - -**Cause.** Not a defect, unbuilt. - -**Narrowed 2026-08-14, and most of the work is already done.** The search half -shipped as item 85, which is the road a rule is made from: search for a value, -save the query, create a rule from the saved query. What remains here is a -SHORTCUT across that road, and both of its halves now exist. - -- The menus are built and every surface already extracts its value as a - finished query (`SearchOffer`, `src/searchterm.h`). A rule entry is another - action beside the two search ones, not new plumbing. -- The seeded-dialog path exists from item 81: - `MainWindow::showTagRulesDialog(const TagRule &seed)`. A rule from a message - becomes a second caller of it, with a different seed, which is what item 81's - spec anticipated when it made the seed a whole `TagRule` rather than a query - string. - -**Approach.** Decide it after using item 85 for a while. Which values are worth -promoting straight to a rule is a usage question, and the earlier answer to it -was wrong (see below), so it is worth having the evidence first. - -**Constraints.** The original approach here said to start from the thread -list's context menu "where the sender is already a value the model holds". -**That is false and item 85 verified it.** `ThreadSummary::authors` comes from -`notmuch_thread_get_authors` and is a DISPLAY SUMMARY, reading `Alice, Bob` or -`Alice| Bob`, so a `from:` built from it matches nothing. A real address comes -from `MessageNode::from` or `ParsedMessage::from`, neither of which the thread -list carries. Any thread-list entry needs an address resolved from a message -first. - -JavaScript is disabled in the profile and must stay disabled. Item 85 reads a -body selection with `QWebEnginePage::selectedText()`, which injects no script; -reuse that rather than adding anything. - -The rules file is shared with mailctl, so a rule created here must go through -`TagRules` and preserve unknown fields; see "Changing the shared rule format" -in CLAUDE.md. - -**Size: S**, down from M now that item 85 has built the menus and item 81 the -seeded dialog. - - ## 94. `pinned` has nothing left to decide once the buttons are built-in **Observed (user, 2026-08-15),** thinking past item 93 rather than from the @@ -433,59 +393,6 @@ bump either way: an ignored optional field is not a breaking change. **Size: S.** Removing a field, two UI affordances and their tests. -## 98. "Important" adds the tag but cannot remove it, unlike every other toggle - -**Observed (user, from the notes):** "the add 'Important' action should be a -toggle (like unread)." - -**Cause (verified in the code).** `src/mainwindow.cpp:863` registers `flag` as a -one-way add: - -```cpp -addAction(QStringLiteral("flag"), tr("&Important"), - tr("Mark the selected threads as important"), [this]() { - tagSelected({ QStringLiteral("flagged") }, {}, tr("Mark important")); -}); -``` - -Adding a tag that is already there is a no-op the user cannot see, so pressing -the key or the button on an already-important thread appears to do nothing at -all. Nothing in the UI removes `flagged` except the general tag dialog. - -**The two neighbouring actions are already toggles**, so this is an -inconsistency rather than a missing feature. `delete` -(`src/mainwindow.cpp:825`) and `toggle_unread` (`:867`) both read the current -state and choose a direction, and `delete`'s comment states the rule this should -follow: one direction for the WHOLE selection, flipping only when every selected -thread is already in the target state, because a single keystroke that leaves a -selection in two states is worse than either outcome. - -**Everything needed is already loaded.** `ThreadSummary::isFlagged()` -(`src/types.h:64`) reads the tag off the summary, so the direction can be -decided without a worker round trip, exactly as `isDeleted()` is. - -**Approach, now a two-line change.** Item 105 extracted -`MainWindow::everySelectedRowHasTag()`, which is the whole of the direction -logic: -`everySelectedRowHasTag("flagged") ? tagSelected({}, {"flagged"}, tr("Unmark important")) : the current add`. -The undo stack needs nothing new, since `TagChange::inverted()` already covers -both directions. - -**Constraints.** - -- Call `everySelectedRowHasTag()`, never a hand-rolled loop. Two separate fixes - went into that logic on 2026-08-16 (items 88 and 105) and both were bugs a - copy of the then-current `delete` loop would have inherited: resolving a - reply's row number to the wrong thread, and asking a reply's thread instead - of the reply. -- The action's tooltip says "Mark the selected threads as important" and would - become wrong. Item 99 is the same problem for `toggle_unread` and the two - should be decided together. -- The label question belongs to item 99, not here. This item is the behaviour - only: the key stops being a no-op. - -**Size: XS.** - ## 99. The unread action is labelled "Toggle unread" whichever way it will go **Observed (user, from the notes):** "the label for 'toggle unread' should be @@ -534,50 +441,6 @@ literals `lupdate` can see; a string built by concatenation is not translatable. **Size: S.** Mostly the mixed-selection and toolbar decisions, not the code. -## 100. The message pane offers Back, Forward, Reload and Save page, none of which mean anything - -**Observed (user, from the notes):** "back/forward/save page in the right pane -don't make sense, shouldn't be visible." - -**Cause (verified in the code).** `MessageView::showBodyContextMenu` -(`src/messageview.cpp:619`) starts from Chromium's own menu: - -```cpp -QMenu *menu = m_view->createStandardContextMenu(); -``` - -That menu is built for a browser and carries the navigation and page actions -whole. The pane is not a browser: every document arrives through `setHtml()` -with a fixed base URL, so there is no history to go back to, nothing to reload -from, and the request interceptor blocks everything by default anyway. The -entries are inert as well as meaningless. - -**Deliberate as far as it goes.** The comment above the call says the page's own -menu comes first so "copy, select all and the rest stay exactly as they were", -which is right for the editing actions and wrong for the navigation ones. The -item is that the filter was never applied, not that the base menu was a mistake. - -**Approach.** Keep the menu, drop the actions that cannot apply. Qt names them -as `QWebEnginePage::WebAction` values (`Back`, `Forward`, `Reload`, -`SavePage`, and `ViewSource` is worth the same look), and each has a -`pageAction()` whose pointer can be matched against the standard menu's entries -and removed. Removing by matching the action pointer is safer than matching by -text, which is translated. - -**Constraints.** - -- Do not rebuild the menu from scratch. Copy, Copy link address and Select all - are the reason the standard menu is used, and item 85's search entries are - appended to it. -- `Save page` is not the attachment save. Attachments have their own bar and - their own path-traversal checks (see the web view security notes in - `CLAUDE.md`); nothing here should grow a second way to write a file. -- Verify against a real right-click on a real message. The offscreen platform - builds the menu but a screenshot of it proves nothing, and the entry list - depends on what the page reports as available at that moment. - -**Size: XS.** - ## 101. Sync is account-aware for edits but not for the account the user is looking at **Observed (user, from the notes):** "sync button should be account-aware." @@ -621,40 +484,6 @@ reaches it (item 42), so most of this exists. **Size: S** for the on-demand button, XS for the visibility half. Ask which. -## 102. The rules table shows no note, so the field explaining a rule is invisible until it is opened - -**Observed (user, from the notes):** "add 'notes' column to the filters table." - -**Cause (verified in the code).** `TagRule` carries a `note` field -(`src/tagrules.h:37`, "Why the rule is shaped this way. Shown in the dialog"), -and the editor below the table edits it, but the table itself lists five columns -and none of them is the note (`src/tagrulesdialog.cpp:116`): - -```cpp -m_list->setHeaderLabels({ tr("On"), tr("Stage"), tr("Rule"), tr("Tags"), - tr("Matches") }); -``` - -So the one field written specifically to explain a rule can only be read one rule -at a time, by selecting it. With several rules the note is exactly the thing that -would let the user pick the right one without opening each. - -**Approach.** A sixth column. The column widths already persist (item 75), so a -new column needs a sensible default width and nothing else in the way of state. - -**Constraints.** - -- The note is free text of any length and would stretch the column. Elide it and - put the full text in the tooltip; the `Rule` column already faces the same - problem with a long query and is the pattern to match. -- `ColumnCount + 1` in `setColumnCount` is load-bearing: the enum drives the - column indices and there is a spare. Add the enum value rather than hardcoding - 5, and check every place that indexes a column by number. -- Notes are the user's own words and can be empty. An empty cell is correct - here; do not substitute a placeholder. - -**Size: XS.** - ## 103. What Delete does to mail on the server is undocumented and unverified **Observed (user, from the notes):** "verify how 'delete' works", with two @@ -757,6 +586,265 @@ distinguish three layers, because the fix lives in a different place for each: **Size: `?`** until reproduced. Most likely not a code change here at all. +## 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 +could be useful, we might have to implement it." + +**This item exists because item 100 removed something it was not asked to.** +The user named Back, Forward, Reload and Save page. `ViewSource` was added to +that list by the agent, on the reasoning that it was "the same kind of thing", +and it is not: the other four have nothing to act on, while view-source has a +real document and a real use, checking what a message actually contains. The +removal is recorded here rather than quietly reverted, because the reasoning +that produced it is the part worth not repeating. + +**Cause (verified in code):** restoring Chromium's entry would not work anyway, +which is why this is an implementation item rather than a one-line revert. +`QWebEnginePage::ViewSource` navigates to `view-source:`. The pane's +document is a `data:` URL (`requestinterceptor.cpp:90-105` records that +`setHtml()` navigates to data: and applies the base URL afterwards), and +`MessagePage::acceptNavigationRequest` accepts only a typed main-frame +navigation, so the attempt is refused before the interceptor even sees it. +Restoring the entry would produce a live-looking menu item that does nothing, +which is the same defect item 100 was reported for. + +**Approach.** Our own action, not Chromium's: a dialog showing the message's +HTML as plain text. The source is already in hand, since `HtmlBuilder` produced +it and `MimeParser` holds the original part; nothing needs fetching. + +**Constraints.** + +- **Plain text is a SECURITY property here, not a style.** `CLAUDE.md` states + it for `MessageDetailsDialog`, and it applies with more force to this: the + content is a stranger's markup, and the whole point of the dialog is to show + it uninterpreted. Set `Qt::PlainText` explicitly on whatever displays it; a + `QLabel` guesses under `Qt::AutoText`. A `QPlainTextEdit` cannot render + markup at all and is the obvious choice. +- Decide which source is shown: the message's ORIGINAL HTML part, or the + document `HtmlBuilder` generated around it. They are different, and the + useful one is almost certainly the original, since the wrapper is ours and + known. Say which in the dialog rather than leaving the user to guess. +- A message with no HTML part needs an answer that is not an empty window. + `messageview.cpp:933` already has the string for this case. +- Reachable from the body context menu, where the removed entry was, so the + gesture the user reached for keeps working. + +**Size: S.** + +## 114. Save image is offered on every image and does nothing + +**Observed (user, 2026-08-17):** right-clicking an image in the message pane +offers "save image", among other entries item 100 never saw because the test +built a menu by hand and no real image was ever clicked. + +**Cause (verified in code):** there is no download handling anywhere in the +tree. `grep -rn "downloadRequested\|DownloadRequest" src/` returns nothing, so +Chromium emits the request and no handler answers it. The entry is present, +looks live, and silently does nothing, which is the same class of defect as +item 100 itself. + +**Approach, and the security question it raised was resolved by the user.** +The first proposal was to scope this to `cid:` parts and refuse remote images, +on the grounds that saving a remote image means a fetch triggered from a +message. **The user pointed out that this is wrong**, and it is: once remote +content has been granted and loaded, the bytes are already fetched and cached. +Saving them is a local copy, not a new request, and blocking it adds no +security while making the entry useless. The reasoning applied to *fetching*, +which has already happened by the time the entry is reachable. + +The real constraint is the neighbouring one: **the save must not itself cause a +fetch.** An image that was never loaded, because it is remote and not granted, +has no bytes to save, and the entry should be unavailable rather than reaching +for the network to satisfy it. + +- Connect `QWebEngineProfile::downloadRequested` and let Chromium write the + file, with the directory chosen through `QFileDialog` as the attachment save + already does. +- The rejected alternative: fetching the bytes ourselves to reuse + `Attachment::saveTo()`. That is a second network request originating from a + message, which is exactly what the interceptor exists to prevent, and it + would bypass the per-render remote grant. `cid:` images would map onto that + path cleanly and remote ones cannot, which is why the uniform route wins. + +**Constraints.** + +- **The path checks are not optional and are already written.** `CLAUDE.md`'s + web-view notes: reduce to basename, strip separators, resolve against the + chosen directory, refuse anything escaping it, and compare resolved paths as + paths rather than with `startsWith`. A filename suggested by a download + request is untrusted input in exactly the way an attachment filename is. +- Do not let this become a second general download route. It saves an image the + user right-clicked, nothing else; `SavePage` stays removed. +- No overwriting. `saveWithoutOverwriting` exists because a silent overwrite + lost six of sixteen files while reporting every one as saved. +- Report the outcome through `statusMessage`, as every other save does. A save + with no feedback is the failure item 13 was about. + +**Size: S.** + +## 115. A copy from the message pane gives no confirmation + +**Observed (user, 2026-08-17):** Copy link address, Copy image address and Copy +image all work, and none of them says so. The user asked for "a small +'link copied' transient that appears and disappears after a few seconds". + +**Cause:** not a defect, unbuilt. These are Chromium's own menu entries and it +does not report success; nothing in this application is listening to them. + +**Copy image was thought to be broken and is not**; see item 116 in the +closed-items file, which is worth reading for how a stale clipboard reading +produced a confident wrong cause. + +All three work. Copy image was briefly filed as broken (item 116) and is not: +the pixels reach the clipboard correctly, and the reading that said otherwise +was taken off a stale clipboard. So this item covers the confirmation for Copy +link address, Copy image address and Copy image alike. + +**Approach.** `MessageView::statusMessage` already exists and the status bar +already expires its messages (item 33), so this is one connection per action and +no new widget, no new timer. `QWebEnginePage::action()` gives each entry's +QAction; connect `triggered` and emit the appropriate string. + +**Constraints.** + +- The message must name what was copied. "Copied" alone is worse than nothing + when three entries sit next to each other in the same menu. +- Do not build a floating overlay for this. The status bar is where this + application reports transient results, and a second mechanism for the same + job is the kind of thing item 45 recorded when two Sync buttons disagreed. +- Depends on nothing; can be built alongside 114 since both touch the same menu. + +**Size: XS.** + +## 117. The message pane offers no Select all + +**Observed (user, 2026-08-17):** right-clicking a body selection offers Copy and +the search entries, and no Select all. Noticed while hand-testing item 100. + +**NOT caused by item 100, and this was verified rather than argued.** The user +ran a build with `src/messageview.cpp` and `src/messageview.h` reverted to HEAD, +so `removeBrowserActions()` did not exist, and reported the same menu: Copy and +the search entries only. Chromium's standard menu for this pane has never +carried Select all. + +**Three wrong theories preceded that measurement**, which is the part worth +recording, because each was plausible and each cost a round trip: + +1. *The separator sweep removed it.* Disproved with a standalone program + reproducing the sweep against a realistic menu: it drops the leading + separator and keeps every action. +2. *Chromium omits it when there is no selection.* Killed by the user, who had a + selection at the time. +3. *A probe will show what the real menu holds.* `createStandardContextMenu()` + returns NULL outside an actual context-menu event, on the offscreen platform + and on a real display alike, so two probe attempts measured nothing. + +The lesson is the one item 100 had already written down and the agent did not +follow: **a menu built by hand proves nothing about the menu Chromium builds.** +`theBodyMenuDropsTheBrowsersOwnActions` constructs its own QMenu, which is right +for testing the filter and useless for testing what is offered. That test now +says so, and deliberately does NOT assert on SelectAll, since a passing +assertion there would read as a guarantee the code does not make. + +**Approach.** Add it explicitly rather than hoping Chromium supplies it. The +action already exists as `QWebEnginePage::SelectAll` and works; only the menu +entry is missing. + +- `menu->addAction(page->action(QWebEnginePage::SelectAll))` in + `showBodyContextMenu`, placed beside Copy rather than appended after the + search entries. +- Worth considering a `Ctrl+A` binding for the pane at the same time, though + note the pane is not the only focusable widget and the query bar has its own + claim on that key. Check `KeyMap::defaultBindings()` before adding one. + +**Constraints.** + +- **A test for this cannot use a hand-built menu.** That is the trap above. The + honest options are asserting the action is in the menu the production code + returns, which needs a real context-menu event, or leaving it to a hand test + and saying so. Do not write a test that constructs a QMenu and calls it + covered. +- Select all selects the rendered body, not the header label, which is a + separate widget with its own selection. That is probably the desired + behaviour but should be looked at rather than assumed. + +**Size: XS.** + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering -- cgit v1.2.3