diff options
Diffstat (limited to 'docs')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 132 | ||||
| -rw-r--r-- | docs/superpowers/specs/2026-08-26-card-avatars-design.md | 300 |
2 files changed, 424 insertions, 8 deletions
diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index ffb41ae..5e95804 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 @@ -131,7 +131,7 @@ taking that too literally. | 62 | No config option for the date format on a card | presentation | XS | **done** 2026-08-11 | | 63 | No way to see sent mail, and no filter for it | workflow | M | **done** 2026-08-11; see `specs/2026-08-11-sent-mail-design.md` | | 64 | The Sync button carries a mailbox icon, not a refresh one | presentation | XS | **done** 2026-08-11 | -| 65 | No full code review and optimization pass | correctness | ? | open, unspecified | +| 65 | No full code review and optimization pass | correctness | ? | open, **narrowed 2026-08-26**: the notes now name it as a dead-code and duplication sweep, not a performance or security pass. Produces a LIST for the user to decide on, not a diff. See the entry | | 66 | Selecting a thread root leaves the message pane blank until a reply has been selected | defect | S | **done** 2026-08-14, unreleased. Not the blank pane it was filed as: the root rendered the CONVERSATION until the thread had been expanded once, then one message. Now always one message, and the conversation view is removed at the user's request. **One case unverified by hand:** the notes also report a single-message `id:` query whose card would not open, which is the same empty-`MessageIdRole` failure and should be gone; confirmed 2026-08-15 as a SEPARATE defect with a different cause, see item 96 | | 87 | Auto mark-read marks a whole thread, including replies never displayed | defect | S | **done** 2026-08-16, unreleased. Built on 108, which is why it stayed small: the timer tracks a MESSAGE id now, and arms for a reply too, which it never did before | | 88 | `threadAt(current.row())` answers about the wrong thread for a reply row | defect | M | **done** 2026-08-16, unreleased. The audit found FOUR live sites, not one. `ThreadListModel::threadFor(index)` resolves a reply through its parent; every caller holding a selected index converted, and no `.row()` on a selected index remains in `mainwindow.cpp`. Unblocks 87 | @@ -242,6 +242,8 @@ taking that too literally. | 166 | Mail you send to your own other account loses `inbox` | defect | S | **done 2026-08-25**, unreleased. `sent_only()` keeps a message only when EVERY file is inside a sent folder, which is what the carve-out's docstring already claimed. No query can express it, measured; the root comes from `database.mail_root`, with a split-index fixture the ordinary layout cannot provide. Verified read-only against the live index: 780 of 807 still stripped, 27 spared, no arrival affected | | 167 | No way to tell one build of an unreleased version from another | enhancement | XS | **done 2026-08-25**, unreleased. The user chose a counter over a git description: `QTMAILDIR_BUILD_NUMBER`, a cmake option ON by default, increments a counter in the BUILD directory on every build and writes `buildnumber.h`. `QTMAILDIR_VERSION_DISPLAY` carries it; `QTMAILDIR_VERSION` stays clean and is what the window title, `applicationVersion` and the release procedure use | | 168 | Delete is offered on mail already in the trash, and does nothing | defect | S | **done 2026-08-25**, unreleased. Delete is hidden when every selected row is already in its account's trash, Restore when none is, both keyed on the PATH rather than the `deleted` tag. Delete also drops `unread` now, in the same TagChange so one undo returns the folder and the tag together | +| 169 | A card shows the account only as a bar, with no fade and no avatar | presentation | M | open, 2026-08-26, from the notes. The accent bar exists (`CardLayout::accentRect`, `CardDelegate::accentLineColour()`); the gradient fade and the sender avatar do not. The avatar's initials source is decided, the vCard half is blocked on item 72 | +| 170 | A row that stops matching the view only leaves it on the Delete path | defect | S | open, 2026-08-26, from the notes, **cause found the same day and the premise is NOT stale**. The optimistic REPAINT is universal; the optimistic MEMBERSHIP is not. `removeThreadsWithoutTag()` has exactly one caller, on the move path, so marking a message read in the Unread view repaints the row and leaves it in a list it no longer belongs to | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -365,14 +367,35 @@ batches of 200 so a 10k-thread query paints immediately) already holds. An optimization pass with no measurement behind it is the kind of work that produces a large diff and no change a user can notice. -**What it needs before it can be sized.** The user saying which of these they -meant: a correctness/security review of a named area, a specific operation that -feels slow with the query that makes it slow, a dead-code and duplication sweep, -or the translatability audit that is already item 22. The first three are -different pieces of work with different sizes, and the fourth is already -recorded. +**Narrowed by the user, 2026-08-26.** The notes now name two sub-bullets, and +they are the same piece of work rather than two: "deduplication of +functionalities" and "check for dead code (functionalities superseded by other +additions, rendering them useless now)". So this is a dead-code and duplication +sweep, NOT a performance pass and not a security review. Nothing slow has been +reported, and the translatability audit it might have meant is item 22, already +done. + +**What that makes it.** A read of the whole tree looking for a function with a +newer twin and for a path nothing reaches any more. The codebase has precedent +for both: `threadAt(int)` survives beside `threadFor(index)` for one legitimate +caller, `SubjectDelegate` was deleted outright at item 53, and item 132 deleted +a whole test rule that had stopped serving. The output is a LIST first, one +entry per candidate with the evidence that it is dead or duplicated, not a +diff; the user decides what goes. -**Size: `?`, unspecified.** Do not propose a design for this; ask. +**Constraints.** + +- "Unreachable from the UI" is not the same as dead. Item 16's + double-press-to-undelete branch reads as dead and is not, because stranded + mail reaches it. Every candidate needs the reachability argument written out + before it is cut. +- A test is a caller. Deleting production code with only test callers is + usually right; deleting the test with it needs saying so explicitly. +- The sweep is worth nothing if it is not run against a green suite before and + after, since the whole value is that nothing observable changed. + +**Size: still `?` until the list exists.** The sweep that produces the list is +S to M; what it finds is the work. ## 72. No khard/khal integration @@ -1291,3 +1314,96 @@ id and a draft of a reply carries both. - Item 163's fix stands on its own and this does not block it: the file is replaced correctly now, so the fork this would have mitigated no longer happens by that route. + +## 169. A card shows the account only as a bar, with no fade and no avatar + +**Observed (user, from the notes):** "the left border of a card expresses the +account the mail belongs to. the background color of the card should fade left +to right from the account color to the current background color we are using (or +to transparent to work both in light and dark themes). On the left we should +leave room for an account avatar (a squircle), for now it could be extracted +from the sender name "From: john doe" becomes "JD" in the avatar. As soon as we +include khard (or some other vcard provider/manager) we will switch to images if +the corresponding vCard has one." + +**Cause (verified in the code):** not a defect. Half of it shipped. The account +colour is drawn as a solid bar down the left edge, `CardLayout::accentRect` +placed by `CardLayout`, filled by `CardDelegate::paint()` with +`CardDelegate::accentLineColour()`. There is no gradient anywhere on a card, and +nothing draws an avatar: `CardLayout` reserves no rect for one, so the geometry +would have to grow before the painting could. + +**Approach.** Two separable pieces, and the avatar is the one that changes the +layout. + +- The fade is a `QLinearGradient` fill over the card rect, from the accent + colour to the pane's background. `accentLineColour()` already records why + blending toward the background is wrong for a CHIP; a card's background is + exactly where such a blend belongs, so the constraint does not carry over. + Both themes come free if the far stop is the palette's own base rather than + a literal. +- The avatar needs a rect in `CardLayout`, which is where it becomes testable + without a painter, and it shifts `contentLeft` for every card. The initials + come from the display name already carried on the summary; a sender with no + display name (an address only) needs an answer before this is built. + +**Constraints.** + +- The vCard half is blocked on item 72, which is itself unspecified. Build the + initials only; do not design the image path in advance. +- A gradient behind the text has to keep the text readable at the left edge in + both themes, which is the same failure mode `accentLineColour()` guards + against on a dark palette. +- This is a looks question, so it is settled by the user looking at it rather + than by a test: assert the geometry in `CardLayout`, and hand the appearance + over per `tests-only-for-measurable-things`. + +## 170. A row that stops matching the view only leaves it on the Delete path + +**Observed (user, from the notes):** "should we refactor the list UI to be +responsive so changes are applied immediately instead of waiting for a view +change to repaint?" + +**Cause (verified in the code, 2026-08-26).** Two different properties were +being called "responsive", and only one of them was built. + +The optimistic **repaint** is universal. `ThreadListModel::applyTagChange()` +covers a thread-scoped write, `applyMessageTagChange()` a message-scoped one +(items 105 to 111), and `revertPendingTagChange()` undoes either if the write +is rejected. A chip, a bold row and a dimmed row all move the moment the user +acts. + +The optimistic **membership** is not. `ThreadListModel::removeThreadsWithoutTag()` +has exactly ONE caller, in `trashMessages()`, added last session because Delete +strips `inbox` and a deleted message sat in the Inbox view across restarts. The +ordinary tag path never calls it: neither `sendMessageTagChange()` nor +`sendThreadTagChange()` asks whether the row still belongs in the view. + +So in the Unread view, marking a message read repaints the row and leaves it in +a list defined by `tag:unread`, which it no longer matches. Un-flagging in the +Flagged view is the same, and so is removing `inbox` by hand from the Inbox +view. It corrects itself at the next query or sync, which is exactly the "waits +for a view change" the note describes. + +**Approach.** Not a refactor. `viewFilterTag()` already resolves the view's own +tag from the query, and `removeThreadsWithoutTag()` already does the removal. +The gap is that the guard sits in `trashMessages()` rather than at the funnel +every tag write passes. Move it, or call it from both send paths. + +**Constraints.** + +- The guard's existing reasoning is what makes this safe and must be kept: only + a plain `tag:<x>` view has a membership one tag decides. A path query (Trash, + Sent, Drafts) is unaffected by a tag going away, and a hand-typed query cannot + be reasoned about. Both are left alone. Without that, marking read in an `id:` + view would empty the list. +- A row leaving is not revertible by `revertPendingTagChange()`, which repaints + rather than reinserts. A REJECTED write would leave the row gone until the + next query. The move path already carries that exposure; check whether it is + acceptable at the tag path's much higher frequency, or make the removal wait + for confirmation there. +- The inverse case is deliberately out of scope: a row that starts matching + cannot be inserted optimistically, since the model has no summary for a + thread the query never returned. +- Undo goes back through the same funnel, so a removal must not make an undone + mark-read invisible in the view it was undone in. diff --git a/docs/superpowers/specs/2026-08-26-card-avatars-design.md b/docs/superpowers/specs/2026-08-26-card-avatars-design.md new file mode 100644 index 0000000..14829c0 --- /dev/null +++ b/docs/superpowers/specs/2026-08-26-card-avatars-design.md @@ -0,0 +1,300 @@ +# Card avatars and the account fade + +Design for backlog item 169. Brainstormed with the user on 2026-08-26. + +**Status:** specified, unbuilt. + +## The problem + +The user's note asks for two things about a card: + +> the left border of a card expresses the account the mail belongs to. the +> background color of the card should fade left to right from the account color +> to the current background color we are using (or to transparent to work both +> in light and dark themes). On the left we should leave room for an account +> avatar (a squircle), for now it could be extracted from the sender name "From: +> john doe" becomes "JD" in the avatar. As soon as we include khard (or some +> other vcard provider/manager) we will switch to images if the corresponding +> vCard has one. + +Half of it already shipped. The account colour is drawn as a solid bar down the +left edge (`CardLayout::accentRect`, filled in `CardDelegate::paint()` through +`CardDelegate::accentLineColour()`), and a reply's spines carry the same colour +muted against the pane's base. What does not exist is any gradient, and any +avatar: `CardLayout` reserves no rect for one, so the geometry has to grow +before the painting can. + +The note calls it an "account avatar", but the brainstorm settled that it is +the SENDER's, not the account's. The account is already expressed twice, by the +accent bar and by the fade; an avatar in the account's colour would conflate +"which mailbox received this" with "who wrote it", which are different facts. + +## What the card looks like + +A card stays exactly three lines and every row keeps the same height, so +`setUniformRowHeights(true)` survives untouched. + +``` ++--+-------+------------------------------------------------+ +| | | sender 26/08 09:14 | +|##| [AV] | * subject @ v 3 replies | +| | | [tag] [tag] | ++--+-------+------------------------------------------------+ + ^ ^ + | +-- avatarRect, full height, its own gutter + +-------- accentRect, unchanged +``` + +The avatar is a full-height squircle in its own gutter, on roots and replies +alike. `contentLeft` shifts right by the gutter, on top of the existing indent. + +Three alternatives were shown to the user and rejected: a two-line squircle +with the tag strip running full width beneath it, and a small squircle inline +on the sender line costing no width. The full-height form won on presence, with +the cost accepted explicitly: at `kMaxDepth` the subject loses the gutter's +width on top of the indent it already loses. + +## The fade + +A horizontal gradient of the account colour, drawn after the chrome and before +the text, so the selection highlight and the deleted-row tint still cover it. +That a selected row reads mostly as selection is expected, not a fault. + +- **On a thread root** it starts at the card's left edge. +- **On a reply** it starts at that reply's INNERMOST spine, which is its own + coloured left border. The wash therefore steps right with the nesting, and a + deeper reply's outer spines stand in plain background. +- **It ends at 60% of the card's width**, in both cases. The end is + proportional, so a reply's wash is shorter as well as further right. +- **A reply's gradient is weaker than a root's**, so an expanded thread reads as + one coloured block with the root leading it. + +The user chose the reply origin against the alternative of starting every fade +at the card's left edge. The spine reading is the correct one because the +spines are ALREADY the account colour: `carddelegate.cpp` muted them at 0.55 +against `QPalette::Base` and resolves a reply's colour by walking to its root, +so the coloured border the fade hangs off is a thing that exists rather than +one this item introduces. + +60% was chosen by the user over 50%. It is a percentage of card WIDTH, which +means the wash grows with the window; a fixed pixel distance and an +anchor-to-the-layout variant were both offered and declined. + +## Whose face + +The **sender's**, except in the flat views (Sent and Drafts) where it is the +**recipient's**, since those views already show recipients in the sender's slot +and `authors` is the user on every row there. + +This costs no extra query. `MainWindow::m_sentView` is really "flat view": it is +assigned from `FlatResult`, and `generatorIsFlat()` in `config.cpp` is the closed +set `{sent, drafts}`, so both already request the recipients fold. The name is +misleading and deserves a comment, but not a rename inside this item. + +For a thread row the sender is the one belonging to `firstMessageId`, the +message the card already stands for. Nothing new is resolved. + +## The initials + +Two letters, always, so every squircle reads the same shape: + +| Input | Rule | Example | +|---|---|---| +| Display name, two or more words | first letter of the first two words | `John Doe` -> `JD` | +| Display name, one word | first two letters of that word | `Cofidis` -> `CO` | +| No display name | first of the local part, first of the domain | `noreply@cofidis.it` -> `NC` | +| Nothing usable at all | first two of the account's label | see the fallback below | + +The user chose the local-plus-domain form over first-two-of-local (`NO`) and +over a single letter (`N`), because it never degrades to one letter and never +reads as a truncated word. + +## The fill + +Two fills, both generated locally from a hash of the sender's address, both +stable per sender. + +- **Identicon** - a 5x5 symmetric grid from the hash bits, with a translucent + dark veil between the pattern and the letters so the initials stay readable + whatever the pattern does. +- **Two-tone** - two related hues and a split angle from the hash, initials on a + large flat field. + +Which one, in order: + +1. The address is listed in `business-senders` -> **two-tone**, whatever display + name it presents. +2. Otherwise a display name is present -> **identicon**. +3. Otherwise -> **two-tone**. + +Rule 2 is the one the user asked for by name: `Ian Farrell +<notifications@github.com>` gets `IF` and the identicon, because it presented +itself as a person, even though the address is corporate. Rule 1 is the override +that lets a listed sender be forced back to two-tone. + +Colours are generated at a FIXED lightness, so `TagColors::textColourOn()`-style +contrast reasoning holds and the initials stay legible in both themes. + +**None of this is Gravatar in the sense of contacting Gravatar.** A real lookup +would send a hash of every correspondent's address to a third party on each +repaint, which is out on the project's privacy stance and on the no-network +rule. Only the generated half of the idea is taken, and it works entirely +offline with `QCryptographicHash`, which Qt Core already provides. + +Item 72's vCard photo would later replace the fill without touching the rect. + +## The fallback + +There is always an avatar. When no sender address is available at all, the +squircle is hashed from the **account's own address**, which the card always +knows through its account, and the initials are the first two letters of the +account's label. This keeps a stable, themed squircle rather than a hole, and +it cannot be confused with a real sender because the two-letter source is the +account. + +## `ThreadSummary::firstMessageSender` + +**This is the one structural change, and it exists because the card currently +has no address to hash.** + +`ThreadSummary::authors` is notmuch's own summarised string and carries display +names ONLY. Measured against the real index on 2026-08-26: `'Standreas'`, +`'Randstad Italia'`, `'Ryanair'`, `'The Hacker News tramite LinkedIn'`. No `@` +anywhere. The initials rule survives that, but two things do not: + +- the identicon has nothing stable to hash, and +- the `business-senders` list has nothing to match. + +Hashing the display name instead was considered and rejected: notmuch builds +those strings, so one sender varies its identity as the string varies +(`The Hacker News tramite LinkedIn` is a constructed label, not a header value). + +So `ThreadSummary` gains `firstMessageSender`, the bare address of the message +the card stands for. It is filled by the SAME worker walk that already fills +`firstMessageId` and `firstMessageTags`, from the same message, and `From` is +served from notmuch's index rather than from the file. This is exactly the +pattern item 111 used for `firstMessageTags` and it carries the same "free, for +the same reason" note. + +The Sent/normal split is already resolved in that walk and must be respected: +`withRecipients` selects the first MATCHED message for a flat view and the +thread's opening message otherwise, so the address follows whichever message +the card is standing for. + +**Measured cost of reading senders from the index**, on the developer's own +database: 1322 distinct senders deduplicated in 12 ms, and 5105 messages +enumerated in 76 ms. There is no cost problem here, which is what makes the +whole feature and its list practical. + +## The `business-senders` list + +`~/.config/qtmaildir/business-senders`. Plain text, one entry per line, `#` +comments, blank lines ignored. + +Deliberately NOT in `qtmaildir.conf` and deliberately not INI. The user's stated +workflow is grep-and-edit ("if a personal email ends in the list by mistake I +can grep it out and remove it"), QSettings would fight a bare list, and the main +config is already large. + +An entry is either: + +- an exact address, `noreply@cofidis.it`, or +- a whole domain, `@cofidis.it`, which is what a company sending from six + addresses actually needs. + +**No globs.** A pattern language nobody asked for is a rule the user cannot grep +for literally, which defeats the file's whole purpose. + +Read once at startup and on an explicit reload. Never per repaint, and never +stat-per-row. + +## How the list fills itself + +At the end of every sync the application runs, the worker collects the senders +of the newly arrived mail and appends CANDIDATES, commented out: + +``` +# noreply@cofidis.it (47 messages) +``` + +A candidate is an address whose local part is in a small built-in word list +(`noreply`, `no-reply`, `donotreply`, `info`, `support`, `billing`, +`newsletter`, `notifications`, `mailer-daemon`), or one that recurs with no +display name. + +Two rules make this safe, and both are borrowed from `mailrules.py`'s existing +discipline: + +- **Never writes an uncommented entry.** Nothing on screen changes until the + user uncomments a line. A step that silently reclassified forty senders would + have to be audited line by line anyway. +- **Never removes, and never re-adds.** An address already present in the file + in ANY form, commented or not, is skipped. An entry the user grepped out + therefore stays out, instead of reappearing within ten minutes with no + explanation. + +### Why the application writes it, and not the `post-new` hook + +Putting the scan in `assets/hooks/` was considered at length and rejected. It +is the better placement on paper: `post-new` runs after EVERY `notmuch new` +whatever started it, including the user's cron, whereas `mailsync.sh` is only +the route qtmaildir drives, and the hook is already scoped to `tag:new` so it +would see only new mail. + +It was rejected on weight. The hook is Python and the reader is C++, so it would +recreate the two-implementations-of-one-format situation that `CLAUDE.md` +documents for `rules.json`, with the same requirement that both sides change +together and the same by-test-only agreement. That is a heavy contract for a +cosmetic feature. + +The application writing it has one implementation, already knows when a sync +finished, and the scan is free. The only case lost is mail indexed by cron while +qtmaildir is not running, and those senders are picked up at the next run. + +## What is testable, and what is not + +Per the standing rule (`tests-only-for-measurable-things`), this feature is +mostly judged by looking. Split accordingly: + +**Assert in tests:** + +- `CardLayout` places `avatarRect` and moves `contentLeft`, at several depths + and for a root against a reply. Geometry only, no painter, per the existing + rule that a card layout must be testable without one. +- The initials function, over all four rules in the table above, including the + one-word and no-display-name cases and an address with no `@` at all. +- The fill CHOICE, over the three ordered rules, including a listed address + that presents a display name. +- Hash stability: the same address yields the same fill twice, and two + different addresses differ. +- `business-senders` parsing: exact entries, `@domain` entries, comments, blank + lines, and an entry with leading or trailing space. +- The append step: never writes an uncommented line, and skips an address + already present commented out. This is the data-loss-adjacent property, so it + is the one worth the most care. +- `ThreadSummary::firstMessageSender` is filled by the query walk, for a normal + view and for a flat one, where the two must resolve DIFFERENT messages. + +**Hand to the user to look at:** the fade's weight and its 60% end, the two +fills against real mail, whether the initials are legible over an identicon at +the desktop's own font size, and whether the gutter costs too much subject at +depth. + +A rendering probe must not be used to judge any of that, for the reasons +`CLAUDE.md` records under "Rendering probes lie". + +## Risks + +- **The gutter compounds with the indent.** A deep reply already loses + `kMaxDepth * kIndentStep`; it now loses the avatar gutter as well. If it reads + badly, the cap is the knob to turn, not the avatar. +- **An identicon behind two letters is the classic legibility failure.** The + veil exists for exactly that and its opacity is a value to tune against the + real font, not to fix on the first guess. +- **`firstMessageSender` is a new field on a struct crossing the thread + boundary.** It is a plain `QString` and crosses like the rest, so there is no + ownership question, but the walk that fills it must finish while the + `NmThread` is alive, exactly as the surrounding code already documents. +- **The candidate word list is a guess.** It will miss senders and propose + wrong ones, which is precisely why nothing it writes takes effect until the + user uncomments it. |