diff options
Diffstat (limited to 'docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 294 |
1 files changed, 294 insertions, 0 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 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 +`<img src="https://..."/>` 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. |
