| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
The three new strings from the cleanup action, translated into Italian.
lrelease reports 383 finished and 0 unfinished; an unfinished string is
silently dropped and ships as English inside an otherwise Italian UI.
The changelog gains an Upgrading section for the mandatory `trash` key, the new
optional `inbox` key and the `Del` binding, and states the consequence that
cost real mail on this branch: a folder name that does not match the server is
created rather than reported, mbsync adopts it, and under Create Both it
propagates to the server where other clients see it.
CLAUDE.md is corrected on two counts. Adding an action is five places, not
four; the fifth is a menu, and nothing enforced it until this branch added
everyActionIsReachableFromAMenu(). And the trash design is recorded: why the
origin lives in a tag, why those tags are joined by a tab rather than a space,
and why Restore resolves against the database rather than the model.
Also repairs a race in deletingTwiceLeavesNoOriginTagBehind(). Its guard ran a
query through the bar in the gap between the file rename and the tag writes,
and a query bar run in that gap returns zero rows forever, since QTRY_VERIFY
re-reads rowCount() and never re-runs the query. Measured 3 failures in 12
runs, each burning a full 15s timeout; 0 in 8 after asking the database
directly, with the runtime down from 45s to 0.3s.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Every version before item 103 tagged a message `deleted` and left its file
exactly where it was, so deleted mail accumulated in the inboxes with only a
chip to say otherwise. `Find stranded deleted mail` runs the query that finds
it: tagged `deleted`, and not inside any configured trash folder.
It reports and moves nothing. Acting on its own would be a bulk delete with no
selection behind it, and the user asked for something they could come back to
and review. Repeatable rather than a startup migration, for the same reason:
mail reaches this state again whenever another client tags without moving.
A menu entry only, at the user's request, so it cannot be confused with the
Trash filter beside the other four.
Also adds everyActionIsReachableFromAMenu(), which asserts the fifth
registration site nothing enforced. CLAUDE.md documents four places; a menu is
the fifth, and `restore` shipped on this branch reachable by a chord and by
nothing a user could see. The new test found three more of the same:
open_thread, clear_pane and clear_selection were all keyboard-only. All three
now sit in the View menu.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Del is the key a user reaches for and Ctrl+D is not a guess anyone makes.
Both are bound; Del is listed FIRST because that is the one the menus
advertise.
Bare, which is safe here for a reason that does not generalise to other
bare keys. A QAction shortcut is dispatched before the focused widget sees
the key, and Qt withholds only plain LETTERS from editable widgets, so by
the argument that made bare Return break the query bar this should delete
mail while the user edits a query. It does not: QLineEdit accepts the
ShortcutOverride for Delete itself, because Delete is one of its own
editing keys, which Return is not. Measured with and without an explicit
filter, the action fires 0 times either way, so no filter is added.
theDeleteKeyEditsTextInTheQueryBar() pins that Qt behaviour, since the
binding rests on it.
**Two defects surfaced from the second binding, both real.**
An action can now have more than one default, and KeyMap did not allow for
it. sequenceFor() decided "is this a built-in?" by comparing against
defaultSequenceFor(), which returns only the FIRST default, so the second
looked like a user override and won the "a user binding beats the default"
rule. The menus advertised Ctrl+D to a user who had configured nothing, and
sequenceFor() and defaultSequenceFor() disagreed about an untouched action.
isDefaultBinding() asks whether a sequence is ANY of the action's defaults;
when two defaults tie, the one defaultBindings() lists first wins, which is
the author's stated preference rather than an alphabetical accident.
And Restore read each message's origin tag FROM THE MODEL. The model's tags
come from the query, so a row whose delete has not been re-queried still
carries its pre-delete tags: measured `[inbox,unread]` on a message already
sitting in the trash, one run in three. No origin tag was found, the
message took the no-origin branch, and Restore moved it to the INBOX
instead of the folder it came from, silently, with the origin tag left
behind as the only evidence. A restore has to be right about the
destination or it is worse than doing nothing.
The trash-view Restore now resolves its messages against the DATABASE
first, through a new NotmuchWorker::resolveMessages(). That and
resolveThreadMessages() share one walk, resolveQuery(), rather than
growing a near-duplicate: they differ only in whether the terms are `id:`
or `thread:`. restoreSelectedThreads() already worked this way; this is the
same reasoning applied to the message-scoped path.
The flake was found by running one test five times rather than trusting a
single green, and the fix verified the same way: 5 of 5, then the full
suite three times over.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Task 6. Delete moved mail into the trash and the only ways back out were a
second press of Delete or Ctrl+Z, both of which act on a row the user has
to have deleted in this session. Browsing the trash and putting something
back needed an action of its own.
`restore` is enabled from the QUERY, not from the selection's tags. The
trash view is path-based precisely so that mail trashed by another client
appears in it, and such a message carries no tag of ours: deciding from
`tag:deleted` would disable Restore on exactly the messages that most need
it. isShowingTrash() compares the current query against the trash
generator's own, for both the per-account and the all-accounts scope, so it
follows the account dropdown like every other filter.
A message with NO origin tag is the foreign-trashed case, and it is why
this is not simply restoreSelected() under a new name. The two callers want
opposite things from a missing origin, which `fallbackToInbox` selects.
From the trash view the message is demonstrably in the trash and refusing
to move it leaves the user looking at mail they cannot get out, so it goes
to the inbox and the status bar says so. From a second press of Delete the
message is not in the trash at all and merely wears a stale `deleted` tag
from an older version or a hand-written notmuch command; moving that to the
inbox would relocate mail the user never asked to move, so the tag comes
off and the file stays put.
The inbox FOLDER is a new optional per-account `inbox` key, defaulting to
"Inbox". It is configurable rather than hardcoded because the name is not
ours to assume: naming a folder that does not exist CREATES it, beside the
real one, and under mbsync's `Create Both` that folder reaches the mail
server. That is not hypothetical, it is what a truncated origin folder did
to real mail while this branch was being tested. Unlike `trash` the key is
optional, since the default is right for any ordinary Maildir and a wrong
value here only affects the fallback.
Ctrl+R, which was free. The action is only enabled in the trash view, so
the key is inert elsewhere rather than doing something surprising. It sits
in the Message menu beside Delete and in the thread context menu, greyed
outside the trash rather than hidden: an action that vanishes teaches
nothing, while a disabled entry with its shortcut beside it says both that
it exists and where it applies.
**Adding an action is FIVE places, not four.** knownActions(),
defaultBindings() and the icon table are each enforced by a test that fails
loudly, and being REACHABLE is a fifth that nothing checked: this shipped
registered, bound, iconned, correctly enabled, and present in no menu at
all, which a green suite reported as complete. Ctrl+R is not a shortcut
anyone guesses, so it was effectively invisible.
restoreIsReachableWithoutTheKeyboard() closes that, and deliberately
excludes the context menu from its menu-bar assertion, since findChildren
returns both and one check would otherwise satisfy the other.
Four tests, each mutation-checked. Two worth keeping: the hardcoded "Inbox"
mutation fails against the fixture's lowercase folders exactly as it would
against a Maildir that spells its inbox differently, and the reachability
mutation reproduces the keyboard-only state this shipped in.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Item 103's implementation was committed unreviewed and never hand-tested.
Reviewing it, and then hand-testing it against real mail, found seven
defects. Six of them lose or corrupt state and none was caught by the
suite, which was green throughout.
**Undo pushed a command instead of consuming one.** onMessagesMoved()
pushed a MoveCommand for every confirmed move, including the move an undo
had just made, so undoText went "Delete", "Undo Delete", "Undo Undo
Delete". A second press of undo re-deleted the message the first had
rescued. PendingMove carries a fromUndo flag, which has to survive the
queued round trip and so cannot be a window-wide "am I undoing" flag.
**Held moves were invisible to the quit guard.** pendingEditCount() summed
the held tag edits and not the held moves, so a Delete pressed during a
sync left the count at zero: the indicator stayed hidden and closeEvent()'s
guard never fired, discarding the move on quit with no prompt. That is item
106's data loss with a worse shape, because a dropped move leaves the file
in the folder the user asked it out of.
**Two moves to one folder dropped the second's tags.** m_pendingMoves was
keyed on the destination, so two Deletes in one account before the first
confirmation both named `acct/Trash` and the second insert overwrote the
first. That file reached the trash carrying neither `deleted` nor
`deleted-from:`, unrestorable and invisible to a `tag:deleted` query. It is
a FIFO now: the worker moves one batch at a time and emits in request
order, so position alone matches a confirmation to its request.
**Second Delete left the origin tag behind.** The restore passed the origin
PLACEHOLDER in its removal list, and onMessagesMoved() resolves that from
the folder the worker reports, which on a restore is the trash. It asked to
remove `deleted-from:Trash`, a tag never written, while the real
`deleted-from:inbox` was never named. A restore does not need the
placeholder: it already read the origin to decide where to send the file.
originTagFor() is now the one derivation both sides use.
**Ctrl+Z left it behind too**, for a different reason: MoveCommand was
constructed with the unresolved pending.add. The command carries the
resolved tags now, and is pushed per origin group rather than once per
batch, because the placeholder resolves to a different tag per origin.
**A thread root re-deleted itself.** everySelectedRowHasTag() asked a
thread row about its THREAD's tags, which notmuch gives as a union. Delete
the root of a three-message thread and the replies are untouched, so the
union carries no `deleted` and a second press ran Delete again: the message
moved trash-to-trash and came out with `deleted`, `deleted-from:inbox` AND
`deleted-from:Trash`, with no way back. The union was a documented
approximation, called bounded because the worst case for a TAG toggle was
re-applying a tag the message already had. A MOVE re-applies the move.
Resolved through messageById(), NOT through ThreadSummary::firstMessageTags,
which is the value the query delivered and is never refreshed by an
optimistic update: after a delete the node reads `deleted` while the
summary still reads `unread`.
**Delete thread never moved anything.** It was left calling tagSelected()
when Delete became a move, so a whole conversation sat in the inbox wearing
a `deleted` chip. It moves every message now, each with its own origin, so
a thread spanning folders reassembles on restore. A reply row resolves to
its own thread through selectedThreadIds(): scopeFor() reports a reply
under messageIds and leaves threadIds empty, which made a thread action on
a reply row do nothing at all.
**And the root card did not repaint** until it was clicked, while its
replies did. sendMove() had no optimistic update at all, so nothing moved
until the worker answered; and applyMessageTagChange() deliberately leaves
a multi-message thread's SUMMARY alone, which is correct for a one-message
edit and wrong for a thread-scoped one. The replies have nodes and
repainted; the root card reads the summary. The thread paths repaint
synchronously with applyTagChange() before the worker is asked, which also
keeps the toggle's direction readable for the next press.
Every fix carries a test and every test was mutation-checked. Three false
greens were found while writing them and are recorded at their assertions:
a disjunction that emptied on the wrong term, a QTRY_VERIFY(rowCount() == 0)
satisfied by the interval before the worker answers, and a query issued
before the confirming write had landed. Absence is asked of notmuch
directly through a new notmuchCount() helper for that reason.
Two bare-window tests moved off assertions about synchronous pending writes
onto the model, since the thread actions now round-trip through the worker.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Delete added the `deleted` tag and moved nothing, so deleted mail sat in
the inbox indefinitely with only a chip saying otherwise. It now moves the
file into the account's trash, records where it came from, and moves it
back on undo.
The origin is derived in the WORKER, not in the UI, because nowhere else
knows it. A Maildir filename does not record the folder a message came
from and notmuch cannot answer once the file has moved, so the moment the
old filename exists inside moveMessages() is the only place it can be
read. It travels back on a new messagesMovedFrom() signal, and the UI
turns it into a `deleted-from:<folder>` tag that Restore reads days later.
The account is resolved from the message's PATH rather than from its
account tag: that tag is optional config, so resolving through it would
silently make an account undeletable. That needed ThreadSummary to carry
the first message's path, since an unexpanded thread row is the ordinary
case and held no path at all. It is reported relative to the database
root, because the UI knows accounts only by their maildir, itself a
database-relative prefix.
accountForMessagePath() accepts both an absolute and a relative path, and
that is load-bearing rather than defensive: a thread row's path is
relative while a reply row's is absolute, since MimeParser has to open it.
Matching only one form left Delete on a reply resolving to no account and
moving nothing, which is the thread-row/reply-row asymmetry this file has
been bitten by before.
Tags are applied only once the worker CONFIRMS the move. Tagging first
would leave a message marked deleted in a folder it never left when a
rename fails, which is the half-done state this removes. A move made
during a sync is held in its own queue and flushed like a tag edit: the
existing queue carries tag changes only, so a move pushed through it would
apply `deleted` and never move the file.
An account with no trash configured reports through the status bar and
tags nothing, as a second line of defence behind the config-load warning.
Six existing tests used `delete` as a stand-in for a message-scoped tag
action on bare windows with no account; they move to `spam` and
`delete_thread`, which stayed tag-only, keeping the property each was
actually testing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Adds trash as a fifth built-in query filter beside Unread, Inbox,
Important and Sent, composing per-account exactly as Sent does:
Config::resolvedQuery() asks each account for its own trashQuery()
rather than wrapping the all-accounts union, and an account with no
trash folder resolves to matchNothingQuery() rather than "match
everything".
Also gives the Trash button a toolbar icon (user-trash) and a trash
key to the mainwindow fixture that asserts every filter button carries
one; without it the button is skipped from the row entirely (no
account configured a trash folder), and the existing icon test found
no button to check.
|
|
The `flag` action only ever added the `flagged` tag, so pressing Ctrl+I on a
thread or message that was already important re-applied a tag it already had.
Re-applying a tag changes nothing and repaints nothing, so the key read as
dead, and removing `flagged` meant opening the tag dialog.
It now reads the current state and picks a direction, exactly as `delete` and
`toggle_unread` beside it do. 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 direction comes from everySelectedRowHasTag(), never a hand-rolled loop.
Two separate bugs went into that helper on 2026-08-16 (items 88 and 105), and
a copy of the then-current `delete` loop would have inherited both: resolving
a reply's row number against the top-level list, and asking a reply's THREAD
where the write is message-scoped, which makes a toggle one-way.
The reply test needs THREE different states to mean anything: the first thread
in the list unflagged, the reply's own thread flagged, and the reply itself
unflagged. With the reply left in its thread's state, the mutation putting
item 105's bug back stayed green, measured. The fixture helper defaults
replyTags to the thread's, so a test that does not pass them explicitly
asserts nothing about scope.
Backlog item 98.
|
|
A thread's card has rendered one message since item 66, but every tag
action still acted on the entire conversation. Delete, Archive,
Important, Mark spam and Toggle unread now act on the message the card
shows; the whole-thread versions move to a "Whole thread" submenu in the
Message menu and the thread list's context menu, on Ctrl+Alt+<key>.
Closes items 87, 88, 105, 106, 107, 108, 109, 110 and 111.
The defects fixed along the way, several found by reading rather than by
report:
- threadAt(current.row()) answered about the wrong thread for a reply
row, because a tree numbers rows per parent. The audit found four live
sites, not the one reported: Delete and Toggle unread each chose their
DIRECTION from an unrelated thread, and the tag dialog counted the
wrong thread's tags. threadFor(index) replaces them.
- A message-scoped write made no optimistic model update and no reply
row carried a doomed cue, so acting on a reply moved the pending-edit
count and changed nothing on screen.
- Both toggles read the state of a reply's THREAD, which a
message-scoped write never changes, so they were one-way: the second
press re-sent a tag the message already had.
- flushHeldEdits() re-sent only thread-scoped edits, so a tag change
made on one message during a sync was applied to the row, counted as
unsynced, and then dropped without ever being written.
- applyTagChange() updated a thread's summary but not its loaded
replies, leaving an expanded thread's rows describing a state the
database no longer held.
- A thread's first message is not among its children, so both
message-scoped lookups missed it: acting on a root card repainted
nothing and emptied the message pane's chip row.
- ThreadSummary::tags is notmuch's union over the thread, so a card
standing for one message drew tags belonging to its siblings. The
worker now reads that message's own tags in the walk that already
finds its id, so the split is known before a row is ever opened.
The card shows both tiers: its own message's tags at full size, the rest
of the conversation's smaller and muted, so nothing appears to vanish
when a row is selected.
Auto mark-read is message-scoped as a result, and now arms for a reply,
which it never did. With maildir.synchronize_flags on, the old
thread-wide write reached the server for mail that had never been
displayed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Double-clicking any row drills into its thread: the list becomes that thread
alone, expanded, and the pane shows the double-clicked row's own message. A
reply therefore opens its WHOLE thread with itself selected, never itself alone,
which is what the user asked for and is not the obvious reading of "open it by
itself".
This is recoverStaleThread() triggered by a gesture. That function already ran
thread:<id>, expanded the thread when the row arrived, selected the target
message once the replies landed, and fell back to the root when the message had
gone; all three cases are existing paths through it, so the new code resolves a
row to a thread id and a message id and hands both over.
The row is reached through the INDEX and never through index.row(): a tree
numbers rows per parent, so threadAt(row) on a reply answers about an unrelated
thread. That is item 88's trap, avoided here by construction.
The first click of a double-click arms the mark-read timer, and the handler
cancels it, because a gesture that navigates must not mutate mail. The timer is
armed again for whichever row the recovery lands on, so only the arming for the
row being left is cancelled. Its test asserts the timer was active beforehand,
so it cannot pass by the timer never having been armed at all.
The expander keeps its own double-click: ThreadListView::mousePressEvent accepts
a press inside its rect and returns, so Qt never pairs one into a double-click
there.
Nothing is built for getting back. The filter buttons already are that, per the
user.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
An edit made while a sync is running is held rather than sent, because the
worker's read-write open blocks on notmuch's exclusive lock. At sync end
onExternalSyncStateChanged() refreshed the list first and flushed the held
edits afterwards, so the refresh read a database that still carried the old
tag, reconciled it into the model, and overwrote the optimistic update the hold
had deliberately left applied. The flush then wrote the tag correctly.
The database ended up right and the list ended up wrong, with nothing scheduled
to re-read it, which is why it looked like the edit had been lost. Reported by
hand: a message read during a sync went back to unread when the sync finished.
The flush moves ahead of the refresh and keeps both properties it already had.
It stays outside the Idle branch, so edits held when /proc/locks becomes
unreadable are not stranded waiting for an Idle that never comes, and it stays
after the status-bar retire, so its own "N held changes sent" message survives.
Both orders leave identical end state, so the first version of the test passed
against the defect: after the handler returns the queue is empty and the write
has been sent whichever ran first. flushGenerationForTesting() stamps the query
generation at flush time, which is what separates them, and the test fails
against the old order with Actual: 3, Expected: 2.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
runAutoSync() returned without rescheduling when a sync was already in flight.
The comment defending it argued the edits were not lost, because they reached
the mail store at edit time and the running sync was "very likely" to carry
them. Very likely is not always: an edit made after mbsync has already passed
that account's mailbox is not carried by it, the timer had fired, nothing
re-armed it, and the pending count sat non-zero until a manual sync or the next
cron run.
Skipping is unchanged and still required by item 71: the cron job holds the same
lock and mbsync fails on a second concurrent run. What changes is that the skip
schedules another attempt. scheduleAutoSync() re-checks the delay, the sync
command and the pending count on the way in, so this cannot arm a sync for
nothing, and against a long external sync it re-arms once per debounce interval,
which is a timer rather than a sync.
The test fires the timer by hand and asserts it is active again afterwards, at
the configured interval rather than a shorter one, with the pending indicator
still showing. It fails against the old skip path.
Item 89's other half is dropped rather than built. The list churn it described
is a tag-defined view working as intended: a thread that loses `unread` leaves
the Unread view, and the user resolved it by living in the Inbox view instead.
Three designs were drafted before asking and none is worth building.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Running a query blanks the message pane but left m_currentThreadId,
m_currentMessageId and m_currentMessageThreadId naming the thread that had
been showing. Both selection handlers compare a newly selected row against
those to decide whether it is already on display, so a result containing that
same thread was recognised as "already showing" and onThreadSelected() was
never called. The card painted as selected, the status bar reported one
thread, and the pane stayed on the placeholder.
This is why it looked like an `id:` query defect. The id is copied out of the
details dialog of the message being read, so that thread is current at the
moment the query replaces the view. Any query returning a different thread
hides the fault entirely.
Filed as the unverified half of item 66 and assumed to be the same
empty-MessageIdRole failure. It is not: 66's fix was correct and this
reproduced against it, so it is recorded as item 96. Four hypotheses were
eliminated by measurement first: the row does carry the message id, the
account-scoped query does return it, MimeParser parses the reported message
(ok, 40701 bytes of HTML), and both real ids resolve bare and quoted.
The regression test's first query must open the SAME thread the second one
returns; with two different threads it passes against the defect, which is how
the first version of it was green. Reverting the fix fails it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The four filter buttons gave no sign of which one you were looking at, so
the row said what you could do and never where you were.
The active filter is drawn as a checked QToolButton, which lets the style
paint its own pressed look: a hand-picked highlight colour would have to be
picked once per theme and would still be wrong under a third.
The check state is derived from the query TEXT rather than from the last
button clicked, which is the whole design decision. A record of what was
pressed goes on lying the moment the query is edited into something else,
where a highlight that follows the query clears itself and lights again
when a filter's query is typed by hand. It is resolved against the account
box, so changing account recomputes it rather than dropping it: the same
filter under two accounts is two different query strings and both are
still "Inbox".
Buttons are held in a hash keyed by generator, cleared at the top of the
row build because the row is rebuilt wholesale on every saved-query edit
and stale entries would dangle. The connections are owned by the row
widget, so a rebuild takes them with it rather than leaving a second copy
firing at deleted buttons.
Unread opens already highlighted, which is correct rather than incidental:
startup_query defaults to it, so the window opens on that view. The test
asserts it, so the assertions that follow are known to be a change of
state rather than a button that happened to start unchecked.
Mutation checked against the design that was rejected: deriving the state
from the click instead of the query fails all three tests, each naming the
behaviour it protects.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Answers "start me in work - Inbox rather than All accounts - Inbox". The key
names an account by its [account.<key>] suffix and the dropdown is set to it
before the startup query runs; because a built-in filter composes with the
dropdown, that is the whole mechanism and the key never reaches a query
builder. Validated on load: a name matching no account is reported and cleared,
since the dropdown has no entry for it and would sit on All accounts without
saying why.
Which side applies the scope depends on what the startup entry is, and getting
it wrong is silent in both directions. A generated filter comes back from
resolvedQuery() already scoped, so letting runQuery() apply the dropdown again
gives path:"work/**" and (path:"work/**" and (tag:inbox)). A saved query does
not, because resolvedQuery() ignores the account key for one, so claiming it
was already scoped leaves it unscoped with the dropdown pointing at Work.
The first of those shipped in this session's working tree and passed its test,
because the assertion used contains() and the double-scoped string contains the
scope too. It asserts the exact query now. The second was found by writing the
test for the case rather than by reading, and is covered by
aStartupAccountAlsoScopesASavedStartupQuery.
The README's startup_query documentation was wrong on two counts after the
previous commit: the fallback is the Unread filter rather than the first query
in the file, and the name can now match a built-in filter.
|
|
Two defects, both reachable only after item 93.
startupSavedQuery() searched the saved queries alone, so a startup_query of
"Inbox" matched nothing once Inbox shipped as a built-in filter and the
duplicated entry was removed from queries.json. It then fell back to
m_savedQueries.first(), which is an arbitrary choice that used to look
reasonable while every install carried an Inbox entry: with the duplicates gone
it opened on a leftover search for one sender, and an empty queries.json opened
on nothing at all. The search now covers the saved queries first, so the user's
own entry wins a name collision, then the built-in filters; the fallback is the
Unread filter, which is always present.
The default startup name has always been "Unread" and now resolves for the
first time: before this it named nothing unless the user happened to have such
an entry.
The constructor also read startup.query directly, and a generated entry stores
no query at all, so even a matching filter opened an empty bar. It resolves
through Config::resolvedQuery() now, unscoped, since the account dropdown
starts on "All accounts".
Icons per the user's choices: a star for Important rather than the flag
action's own icon, since on the query row an icon reads as a category rather
than as an instruction, and mail-folder-sent rather than mail-sent.
Three tests changed rather than adapted, because their premises were the
defect. Two asserted the first-saved-query fallback.
aCronSyncDoesNotRefreshBeforeAnyQueryHasRun assumed a fresh window had run no
query, which is no longer true; it is now
aCronSyncRefreshesTheLastRunQueryNotTheQueryBar and asserts the property that
actually matters on a cron timer, through a new lastRunQueryForTesting() seam,
since a legitimate refresh bumps the generation and the counter cannot tell the
two apart.
|
|
Item 57 renamed the `flag` action to "Important" in 0.14.0, chosen over
"Starred" partly because &I was free where &S collided with Mark spam. Item 93
then shipped the filter for the same tag as "Flagged", so one window offered
both names for one thing. The generator keeps its own name, `flagged`: that
string is stored in queries.json and matched against a closed set, so it is
wire format rather than a label.
The filters are QToolButtons now, like the Save button at the other end of the
row, carrying a themed icon with the text beside it. Icon AND text for the
reason the Save button already records: this row is a row of text buttons, so
an icon alone reads as a different kind of control than it is. Theme icons
rather than the shipped SVGs in Marks, because item 70's split is that the
panes are ours and the chrome is the system's, and the query row is chrome.
mail-mark-important matches the `flag` action's own icon, since the filter
finds what the action marks.
The icon test asserts a NAME was requested rather than that the icon resolved:
QIcon::fromTheme returns null where no icon theme is installed, so isNull()
would fail for a reason unrelated to this code. Dropping the setIcon call fails
it.
Widening the buttons to QToolButton broke eleven tests that reached them
through findChild<QPushButton *>, which does not match a sibling type. The
helpers and the filter lookups take QAbstractButton; savedQueryButton() stays
on QPushButton, since the user's own queries really are those.
|
|
An unpinned query could not be run. Its menu action carried both a triggered
connection and a submenu of edit actions, and Qt does not emit triggered for an
action that owns a menu: clicking the entry only opened the submenu, so the
connection had never fired.
It shipped unnoticed because the menu was the rarely-used half while the user's
queries were pinned buttons. Item 93 moved every query into the menu, which is
how it surfaced, and item 94 makes the menu their only home, so this is now the
path that has to work.
Running is an item inside the submenu, first and above a separator, with the
edit actions below it. The entry keeps its submenu because an unpinned query
must still be editable and deletable.
The test asserts the Run item exists and is first, then that triggering it
reaches the query, then that Edit and Delete survived beside it. Restoring the
old wiring fails it on the first of those, naming the Qt behaviour rather than
just reporting a wrong query string.
|
|
Item 93, the UI half. Unread, Inbox, Flagged and Sent are buttons the
application ships, sitting first on the row, ahead of the user's pinned saved
queries.
runFilter() is runSavedQuery()'s opposite in the one way that matters: it READS
the account box and never writes it. That is item 90's defect. A filter narrows
what the user is already looking at, so the dropdown is its input rather than
something it resets on the way past. A saved query keeps setting the account
from what it stored, because it is a destination and states its own scope.
runQuery() gains an AccountScope parameter. A filter's text arrives already
resolved in the selected account's scope, and scoping it again would put
path:"work/Sent/**" inside path:"work/**".
Two migration changes, both of which unpin rather than delete:
- Sent is no longer migrated from the INI into queries.json. The built-in
filter covers it, and migrating one too would put two Sent buttons on the
row, one editable and one not.
- A stored entry naming a known generator is unpinned on load, which is what
every install upgraded through 0.19.0 carries. It keeps its name and its
generator and moves to the menu. Deleting it would be data loss on a file
whose readers are supposed to preserve what they do not own.
The test suite needed the same distinction the design makes.
savedQueryButtonLabels() now skips the filters, and savedQueryButton(window,
label) replaces five positional row->findChild<QPushButton *>() lookups that
were silently returning Unread.
One rendering probe had to be fixed rather than adapted.
replyRowsKeepTheirTextUnderTheThreadLine resized the window to 300px, and four
more buttons pushed the reply row below the viewport: the pixel loop then ran
zero times and reported "0 pixels, the row was painted over", which is a
different defect from the one it exists to catch. It gets 600px and a guard
asserting the row is really inside the viewport, so the next person to shrink
it gets told the truth. Verified by putting 300 back: the guard names the row
at 83..165 in an 82px viewport.
|
|
Item 74. runQuery() set the status bar once and only queryFinished cleared
it, so the bar kept claiming a query was running for the whole walk while
rows were visibly arriving behind it. Measured cold against a 1.1 GB index:
the first batch reaches the model at 642 ms and the walk finishes at 5714 ms,
so five seconds of a slow query read as a frozen one.
onThreadsReady now sets the bar from the model's own row count after each
batch, which is the number of rows the user can actually see. No timing
changes; this only stops the bar from lying.
The refresh branch returns before the new line, so a background refresh stays
silent exactly as onQueryFinished already keeps it. That silence has its own
test, which fails when the write is moved above the guard.
beginRefreshForTesting() is a new seam: refreshCurrentQuery() returns early
without a worker and a bare window has none, so a test cannot otherwise reach
the refresh path.
|
|
Reverts bde7409 and 66f1159. The user hit the worst possible symptom:
clicking one message marked a DIFFERENT, unrelated message read.
The cause is in markCurrentThreadRead, which reads
m_model->threadAt(current.row()). CLAUDE.md records this exact trap: a
tree numbers rows PER PARENT, so a reply's row() indexes its siblings
and threadAt() on it answers about an unrelated thread near the top of
the list. The guards then compared the right ids against the wrong
thread and let a write through for whatever message the timer's state
named.
That fault predates these commits, but they made it reachable and
harmful: while the write was thread-scoped the mismatch was mostly
masked, and scoping it to a single message turned it into "a random
message is now read".
Reverting rather than fixing forward. Marking the wrong mail read syncs
out to the server and cannot be undone from here, so the safe state is
the previous behaviour, which is too broad but predictable. The item 66
work in 4a4f82f stands: a thread root still renders one message.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Follows the message-scoped mark-read. The user reported the write going
out with nothing visible changing: the status bar counted an unsynced
edit while the card stayed bold and the message pane still showed the
`unread` tag, until the next query corrected it.
sendMessageTagChange made no optimistic model update on purpose, because
applyTagChange is keyed by THREAD and repainting a whole row for a
one-message edit would claim every reply had changed too. That trade is
right for an explicit tag edit and wrong for auto mark-read, where the
visible change IS the feature and the delay exists to deliver it.
ThreadListModel::applyMessageTagChange updates the message wherever it
is held, as a child row and as `first`, and lets the thread's summary
follow only when the answer is unambiguous: a thread reads as unread
while ANY message does, so the tag is cleared from the thread only when
no other message still carries it. For an unexpanded multi-message
thread the per-message tags are not loaded, so the summary is left for
the next query rather than guessed at.
Mutation checked: without the call the card holds `unread` for the full
timeout.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Reported by the user: selecting an unexpanded thread root marked every
message in the thread read, replies included. maildir.synchronize_flags
is on, so removing `unread` rewrites Maildir filenames and the next sync
carries it to the server: mail the user never saw stops being unread
everywhere.
This was coherent while a root click rendered the whole conversation,
because everything marked read had been displayed. Removing that view
made a root render one message and left the thread-wide write in place,
so the defect arrived with the previous commit.
markCurrentThreadRead now sends m_currentMessageId, which is what the
pane rendered, through sendMessageTagChange. The thread-level `unread`
guard is dropped with it: a thread carries `unread` while ANY message in
it is unread, so it would pass a read root under unread replies and send
a write for a message already read. Scheduling still checks it, which
keeps a fully-read thread from arming a timer.
The test asserts on which worker entry point the window used, because
reading tags back cannot answer this. Three earlier versions passed
against the unfixed code: TagsRole is empty for a message row by design,
MessageOwnTagsRole subtracts thread tags and drops marks so it can never
hold `unread`, and raw node tags are not refreshed until onTagsApplied
confirms, which lands after the assertion. Mutation checked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Selecting a thread root used to render the whole conversation, stubs
plus the last messages expanded, but only until the thread had been
expanded once. After that the identical click rendered a single message.
The user reported the inconsistency and asked for the single-message
behaviour throughout, and for the conversation view to go.
The cause was a timing one, not a race. The root card stands for the
thread's first message and onThreadSelected already preferred to load
just that, but the model learned the id only when the replies arrived,
so a fresh row fell through to a whole-thread render.
ThreadSummary now carries firstMessageId from the query itself, so the
id is known before any expansion and the fallback is unreachable. It is
free: notmuch_thread_get_toplevel_messages reads the index, not the
message files, and a walk with it is indistinguishable from one without
over a 36,615-thread database. Contrast recipients, which reads every
file and stays Sent-only.
The Sent view keeps showing what the user sent rather than the thread's
opening message, which is often someone else's. There is no
matched-messages iterator in libnotmuch, only a count, so that branch
walks oldest-first to the first NOTMUCH_MESSAGE_FLAG_MATCH and stops:
0.146s against a 0.143s baseline over 4,515 threads.
onThreadLoaded merges into renderMessages, since onMessageLoaded was
already delegating to it for the actual painting. It still takes a list
because MessageView renders a list; collapsing that is a separate change
to a class with its own tests.
NotmuchWorker::loadThread is kept and documented as having no UI caller.
It is a tested way to read a thread's messages with the match set
resolved, used as a helper by the worker's own tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Item 36's section moves to the closed file and its three traps go into
CLAUDE.md, which is where they will be read: the worker is unreachable
by findChild, rowCount on an unexpanded thread row is 0 by design, and
currentThreadId reports intent rather than content.
The claim that this class of bug cannot be reproduced in test_mainwindow
is now false, so it is corrected rather than left standing beside its
replacement. One in-test reference to item 36 as a permanent limitation
is reworded: bare-window cases still have no worker, but that is now a
choice per case rather than a property of the binary.
Item 66 stays open with the simple case ruled out. The negative result
sharpens this entry's own candidate rather than contradicting it: the
test drives setCurrentIndex, which updates the selection model
synchronously, while the suspect guard turns on a real click not having
done so yet. Two cheaper conditions are named as still unexcluded.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
WorkerBackedWindow builds a throwaway notmuch database, writes a config
pointing at it, and hands back a Config. Opt-in per test: the fifty-odd
existing cases construct a bare window and pay nothing.
No production change. wireWorker() already builds the worker from
m_config.notmuchConfig(), so a written qtmaildir.conf exercises the
shipping path rather than a parallel one built for tests.
Three findings, each of which had made an earlier version of this test
worthless:
The worker is unreachable by findChild. wireWorker() creates it
parentless and moves it to its own thread, so the tests wait with
QTRY_VERIFY on observable state instead of on worker signals.
rowCount on a thread row is 0 until the thread is expanded, since
children are populated by the expansion. hasChildren is the correct
pre-expansion assertion and also proves the two messages threaded.
currentThreadId() is assigned synchronously in the selection handler,
before any worker round-trip, so it reports intent rather than content:
the test passed with onThreadLoaded() disabled entirely. It asserts on
MessageView::showingPlaceholder() now, checked blank before the click so
the check after it means something, and mutation-confirmed to fail when
the load is disabled.
Item 66 does NOT reproduce here: selecting an unexpanded thread root in
a two-message thread shows the message. Recorded rather than widened.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
showWarnings() did two separable things and one of them could not be
reached from a test. It set the status label, which is harmless, and it
raised a QMessageBox from the MainWindow constructor, which under the
offscreen platform nothing can dismiss: the constructor never returned
and the suite hung with no output, reading as an infrastructure failure
rather than a test one.
It splits in two. applyWarnings() keeps the status label and stays in
the constructor. configProblems() returns the list, and main.cpp raises
the dialog after show(), which also gives it a visible parent to sit on.
The distinction between warnings and problems is preserved exactly: a
keybinding being ignored interrupts startup, "no sync command
configured" does not.
The warning path now has its first test, using the config shape that
caused the original hang. Mutation checked by putting the modal back in
the constructor: the test times out at 124 rather than failing, which is
the behaviour this removes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Four signatures, no behaviour change: the two shipped operations map to
Replace and Narrow. runSearchFromPane becomes a switch and gains the
Exclude arm, which nothing can reach until the menu entry exists.
Seven call sites across three test files moved with it, two more than
the plan predicted: test_messageview and test_mainwindow also drive
these signals directly.
mainwindow.h and messagedetailsdialog.h now include searchterm.h for
the type; messageview.h already did.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The panes carry a finished query and know nothing of the query bar; the window
sets the field and calls the existing runner, so the account scope and the
generation counter keep working as they do for a typed query.
Narrowing combines here rather than in a pane, because only the window can see
what the bar currently holds. The tag strip's chips join the header, the body
selection and the details dialog as a fourth surface.
Also fixes the details dialog to actually close when a search is chosen: the
comment above the connection already described this requirement, but nothing
called accept() or reject(), so the dialog stayed open, the query ran behind
it, and the modal exec() never returned. This hung the whole test suite on
QT_QPA_PLATFORM=offscreen once a covering test was added.
|
|
Both halves asserted together, plus a guard proving the generated button
carries a menu at all: a test for the absence of a menu item passes
against no implementation, which item 82 recorded the hard way.
The account section is load-bearing rather than scenery. A generated
entry resolving to an empty query is skipped entirely, so without a
configured maildir and sent folder the Sent button is never built and
the assertion would pass by finding nothing. An account section missing
maildir hangs the test outright, which is how this was found.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Item 82. Saving a query worked and nothing else did: changing one field meant
retyping the whole query under the same name, and deleting one meant editing
the file by hand. An action that creates something the UI cannot then change or
remove is incomplete, and the user hit it within minutes of the first hand test.
Right-clicking a saved query, on its button or its menu entry, now offers Edit,
Move to menu / Show as a button, and Delete. Every path funnels through one
replaceSavedQuery(), which matches on the name the dialog was OPENED with rather
than the one it returns, so a rename replaces the entry instead of leaving the
original behind beside a new one, and which merges the stored entry's unknown
fields in a single place rather than in three. Delete confirms first: the rule
against confirmation dialogs covers tag mutations, which the undo stack can take
back, and this writes user config that it cannot.
Two cases the item did not anticipate. A generated entry has no query to edit,
so the dialog shows its composed query read-only rather than offering a field
that changes nothing, and carries `generated` and `flat` through an edit rather
than letting it decay into a plain entry holding a snapshot of what it resolved
to today. And the overwrite notice had to learn to ignore the entry being
edited, since warning that "Inbox" already exists while editing Inbox is noise.
This also fixes a defect that predated it and was already reachable from the
save path. rebuildSavedQueryRow() called deleteLater() on the old row, which
defers destruction to the event loop, so the stale row went on answering
findChild() and every lookup after a rebuild reported the state from before the
edit. Nothing looked wrong on screen, which is why it surfaced only as three
tests failing against a row that had in fact been rebuilt correctly.
Five tests, three mutations. Matching on the returned name fails two, never
writing the file fails three, and dropping the unknown-field merge fails one.
That last one initially proved nothing: it drove UNPIN, which copies the stored
entry and so carries `unknown` along by itself, and passed with the merge
deleted. It now goes through the edit path with a replacement that has none,
which is what the dialog actually returns.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The user asked whether the default queries could be unified with Sent. The
answer runs the other way: Sent joins the saved queries rather than the saved
queries becoming hardcoded.
Inbox, Unread and Important are complete strings that depend on nothing and can
never go stale, so generating them would buy nothing and would cost the four
things the file just gained: reordering, unpinning, renaming and deleting.
Hardcoding them would also make them undeletable, which is a regression for
anyone who does not want one of them.
Sent is different only in that its query CANNOT be stored: it is composed from
every account's `sent` key, so a stored copy goes stale the moment a folder is
renamed. That is a property of Sent, not of "default queries". Storing the
GENERATOR rather than its output keeps both halves: `"generated": "sent"` still
resolves from the accounts at click time, and the entry is an ordinary row that
can be reordered, renamed, unpinned or removed. The row now follows one rule
instead of carrying one member the user did not own.
Two properties had to travel with the entry. The composed query, resolved
through Config::resolvedQuery() so what lands in the bar is what actually ran;
and FLAT mode, since a sent view lists messages and a threaded one folds every
reply back into the conversation the user sent one message into. The sent
generator implies flat rather than trusting the file to say so, because a
hand-edited row would otherwise produce a threaded sent view.
An unknown generator is reported but the row is KEPT: a later build may know it,
and dropping it here would delete it from the file on the next save, which is
the same data loss the unknown-field handling exists to prevent. A generator
whose accounts configure nothing is skipped entirely, exactly as the hardcoded
button was hidden rather than offering one that finds nothing.
Eight new tests. The four pre-existing Sent tests reach this through migration
and were left alone, which is what proves the migrated path still behaves; the
new ones cover a STORED file, which is the path every launch after the first
takes. Mutations: a generator resolving to nothing fails three, ignoring flat
fails two, and not skipping an empty generator fails one. A rename test guards
the property the change exists for, since anything keyed on the literal name
"Sent" would break it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
document-save is the floppy/disk shape. It means "write a file somewhere",
which leaves the user to guess what is being written, and next to a row of
saved-query buttons it reads as an unrelated control. Saving a query is
bookmarking a search, so bookmark-new is the icon every desktop already uses
for "keep this for later". Verified to resolve with real art in the desktop's
actual theme rather than assumed present.
The button also shows its label now instead of the icon alone. It sits among
text buttons, and an icon on its own next to them reads as a different kind of
control; it is also the one action whose meaning an icon cannot carry, since
"save" is a familiar shape whose question is always "save what?". The toolbar
is unaffected and still follows the desktop's own button style.
The label is the button's own text rather than the action's. "&Save query..."
is menu phrasing, and setDefaultAction copies it verbatim, so the button
rendered an accelerator ampersand and the ellipsis that promises a dialog. The
action keeps both for the menu it lives in, and the test asserts the override
survives setDefaultAction rather than trusting that it does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
nothing is pinned
The saved-query buttons are the row's content and read as a set; the overflow
menu is a control over that set, so it belongs apart from them rather than
trailing the last button. Moving the stretch above it pushes it to the right
edge.
Doing that exposed a latent defect in the same function. The row hid itself when
its layout held nothing but the stretch, which was written as a count of one and
happened to be right only because the stretch went last. With the stretch moved
the count changes, and the condition as written would have hidden a row holding
only the menu: a config with saved queries but none pinned would have had no
route to any of them, the menu buried along with the row. The check now counts
the content added before the stretch and treats an unpinned query as content in
its own right.
Both are mutation-checked. Putting the stretch back at the end fails the
alignment test, and restoring the old hide condition fails the new one, which
asserts the row survives with nothing but unpinned queries in it. The alignment
is asserted on the layout's own ordering rather than on x coordinates, since a
geometry assertion would also pass for a row that merely ran out of width.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The spec asked for "a Save query button beside the search bar" and what shipped
was a menu entry and Ctrl+S. The user went looking for the button where the
design said it would be and did not find it.
Saving is a thing you decide on while looking at the results, so it belongs
where the results came from rather than behind a menu or a remembered chord.
The button takes the action through setDefaultAction rather than a second
connect, so it inherits the text, icon, tooltip and enabled state and cannot end
up offering to save an empty query while the menu entry correctly refuses. The
mutation that replaces it with a plain clicked() connection fails the test.
Also records item 82: a saved query cannot be edited, unpinned or deleted from
the UI. Item 23 specified saving and nothing else, and that is exactly what was
built, so the only way to unpin a query is a text editor or retyping it in full
under the same name. An action that creates something the UI cannot then change
or remove is incomplete, and this was found within minutes of the first hand
test. It is filed as a defect rather than an enhancement, and the spec now says
so where a reader would otherwise take the design for complete.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Second half of item 23, on top of the storage change. A query can now be kept
without hand-editing a file, and the row of buttons no longer grows without
bound.
Ctrl+S opens a dialog on whatever is in the query bar, taking a name, an
optional account scope and whether the query is pinned. It preselects the
account already chosen in the dropdown, since that is the scope the user is
looking at, and it says so when a name is about to replace an existing query
rather than refusing the name: overwriting a saved query on purpose is a normal
edit, and the only thing worth preventing is doing it without noticing. Saving
over an entry keeps the stored entry's unknown fields rather than the dialog's
fresh value, so a field written by a later build survives being edited here.
The saved queries move to a row of their own beneath the query bar, pinned ones
as buttons and the rest behind a More queries menu that only exists when
something is in it. The ponytail note that stood in the query row predicted
exactly this: an unbounded list of buttons sharing the row squeezed the field.
Sent moves down with them and is still not a saved query, for the reason already
recorded there.
A saved query's account scope goes through the account DROPDOWN rather than
being baked into the query text. runQuery() already wraps the query in the
selected account's path, so pre-scoping here would apply it twice, and setting
the dropdown also shows the user which scope they are in. An unscoped query
clears the selection rather than inheriting whatever the last one left, which is
the same defect the rules preview had.
Seven tests, three mutations. Ignoring the pinned flag fails two of them,
pre-scoping the text instead of setting the dropdown fails two, and letting an
unscoped query inherit the previous account fails one. The menu-absence test
initially passed against no implementation at all, since it only asserted a
widget was missing; it now proves the row was populated first, which is the
guard that class of test needs.
Two existing invariants caught real omissions rather than needing adjustment:
every registered action must appear in KeyMap::knownActions(), which is what
gives it a configurable binding, and every action needs its own icon.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Item 61. An init() fixture gives every test its own empty lock table in a
QTemporaryDir, so no test observes the machine's real sync state. The
failure was never intermittent in the usual sense: 0 failures in 30 runs
with no lock held, 30 in 30 with one held. It presented as three tests
failing that never mention syncing, and cost three misdiagnoses.
The three tests that already used the seam each restored "/proc/locks"
when finished, which was itself the defect: it handed the real table to
whichever test ran next, so one test opting in re-exposed all the others.
Those restores are gone and cleanup() leaves the temporary path in place.
noTestCanSeeTheRealLockTable guards the fixture, since a silent revert
would go back to failing for reasons no assertion mentions.
Verified with the lock deliberately held: 3 failures before, 119/119
after, full suite 19/19. Mutation-checked by disabling the fixture, where
the guard fails first and a real test fails behind it.
|
|
Item 71. A tag edit reached the notmuch index at edit time and then sat there
until the user clicked Sync or their cron job fired, so "mark all read" updated
the view while the change itself waited, sometimes for ten minutes.
A confirmed edit now arms a debounce that runs the existing sync path. The delay
is auto_sync_delay_ms in [general], defaulting to 2000, and follows
mark_read_delay_ms exactly, including that zero and negative are not errors:
zero syncs on the next trip through the event loop, and any negative value
disables the behaviour, which is the switch for a user who wants only their cron
job.
It is armed from onTagsApplied, where a write is confirmed and the pending count
is already current, rather than where one is sent: a sync scheduled for a write
the worker went on to reject would run for nothing. A debounce rather than a
schedule, restarted by each edit, because "mark all read" confirms one write per
thread in the view and an arm-per-edit timer would be the storm of syncs the
debounce exists to prevent. Nothing is armed when no sync command is configured
or when the pending count is zero, the case where an edit was netted against its
own inverse. When the timer fires with a sync already running, local or cron, it
skips rather than queues: mbsync's own answer to a second run is to fail on it,
and the edits stay pending rather than being lost.
Also fixes a pane blanked out from under the reader, found by hand testing this
feature. onSyncFinished called runCurrentQuery() where the cron path calls
refreshCurrentQuery(), and a re-run clears the model, the undo stack and the
message pane. The stale-thread notice handles a thread that stops matching the
query and has since item 35, but a re-run left nothing for it to describe. The
two paths had no reason to differ; before this item a local sync only followed a
click on Sync, so the difference went unnoticed. Reading a message in the Unread
view, having it marked read, and watching the pane go blank two seconds later is
what surfaced it.
Its test asserts on the undo stack rather than the pane: both paths issue a
queued query test_mainwindow has no worker to answer, so the pane ends up blank
either way and an assertion on it would pass against both, while the undo stack
is cleared by one and kept by the other.
Nine tests, four in test_config and five in test_mainwindow, each
mutation-checked: removing the schedule call, honouring a negative delay,
dropping the nothing-pending guard, dropping the already-running guard, and
restoring runCurrentQuery() each fail a test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Item 67. The pane counted unread, flagged and inbox from three fixed
tag: queries. Sent and drafts cannot join that list as tags: tag:draft
counts 0 against a real database and no draft-ish tag exists in it at
all, so a tag-based line would be a permanent zero that reads as working
code. Both are composed from each account's folder keys instead, the
same way the Sent view already composes its query.
The drafts key was parsed and documented as unused in v1. Composing
drafts is still v2; counting them is not, so Account::draftsQuery() and
Config::allDraftsQuery() now mirror the sent pair. The shared body moved
into folderQuery() and joinAccountQueries(), so the load-bearing quoting
(a provider nests both folders under a bracketed parent, and [ and ] are
Xapian syntax) and the bare-"or" guard exist once rather than once per
folder type.
The fixed array is gone rather than extended. It held queries and labels
in two lists indexed in parallel, which is a hazard that grows with the
list: an entry inserted in one and not the other prints a real number
against the wrong name and looks entirely plausible. placeholderLines()
carries each query beside the callable that labels it, so the two cannot
drift, and the count reply stays paired by position as the worker
requires.
A line is omitted when no account configures that folder rather than
shown as 0, following item 63: a missing folder is a real configuration,
and "0 sent" claims the user has sent nothing.
Measured against the real config: 4 sent terms over 601 threads, 5
drafts terms over 3, the extra drafts term coming from the one account
that configures drafts and no sent, which is what proves the two are
collected independently.
Four tests here and four in test_config, mutation-checked at three
points: dropping the drafts line, an off-by-one in the label pairing,
and removing the -1 guard for an uncountable query. Each mutation fails
a test.
|
|
Adds a `sent` key to [account.*] naming that account's sent folder, and a
Sent button beside the saved queries that composes its query from every
account carrying one. An account without the key is omitted silently, as a
real account may keep no sent mail locally. With no account selected the
button spans all of them; selecting one narrows it through the existing
scope wrap rather than a second path.
Composed at run time rather than shipped as a [queries] entry. A saved query
is one fixed string: it cannot narrow to the selected account, and it goes
stale the moment an account is added or a provider renames a folder.
The design and the measurements behind it are in
docs/superpowers/specs/2026-08-11-sent-mail-design.md. Three things there are
worth repeating here.
The composed path is QUOTED, and that is load-bearing. A real provider nests
its sent folder under a bracketed parent, and "[" and "]" are Xapian syntax:
unquoted, the query parses rather than matches and returns nothing while
looking entirely plausible. Composition happens in one place so there is one
chance to get it right, and a bracketed path is pinned in a test.
Recipients are opt-in per query, which is a performance contract rather than
a preference. notmuch_message_get_header(m, "To") is not served from the
index, it reads the message file: folding every thread of a 4411-thread
inbox took 38.2 seconds against 251 ms for the 601-thread sent view. The
worker skips the walk entirely unless asked, and the refresh path carries the
same flag so a background sync cannot blank the column mid-read. Always
folding is mutation-tested: the data would be right and only the cost wrong,
which nothing else here would notice.
The messages reached through the thread are owned by it and freed with it, so
recipientsOf() holds them raw and finishes while the thread is alive, exactly
as walkReplies does. An NmMessage wrapper there is a double-free.
Sent mail is presented flat, and the pane follows. A message you sent
otherwise drags in the replies you received, so a view labelled Sent shows
conversations rather than what you sent. ThreadListModel::setFlatMode() makes
hasChildren() and ReplyCountRole answer differently and changes nothing else;
runQuery() sets it on EVERY run, so any other query restores the tree on its
way through and the flag cannot outlive the button that set it. The pane
needed its own fix for the same reason: the single-message path depends on a
field only filled when a thread is expanded, which never happens in a flat
list, so loadThread() gained matchedOnly and drops the messages that did not
match instead of rendering them as stubs.
Recipients replace the sender through the existing SendersRole rather than a
new one, so the delegate needs no branch and cannot disagree with the model
about which name a row shows. It falls back to the sender when a To header is
absent or unparseable, since a blank where a name belongs reads as a
rendering fault.
Address parsing uses GMime: a display name may contain a comma, so
"Rossi, Mario" <m@example.org>, info@example.net is two addresses and
splitting reports three. internet_address_list_parse returns NULL for an
empty string, which is a crash if unguarded.
Backlog item 63.
|
|
The thread list now updates itself when a sync finishes, whether it is
empty or populated. New threads appear where the sort puts them, threads
that stopped matching leave, and threads whose state changed repaint.
Refreshing used to mean re-running the query, which cleared the model,
the selection, the message pane and the undo stack, so 0.8.0 declined to
do it on a cron timer and asked the user to press Enter instead. The
result was a list that quietly disagreed with the database: mail indexed
by cron never appeared, and an Unread view read to the end sat empty in
front of it.
ThreadListModel::reconcile() diffs a result against the current rows by
thread id instead, so a surviving thread keeps its row, its persistent
index and its loaded replies. Order comes from the result and is never
imposed here, which is what makes the sort dropdown authoritative.
The undo constraint this was sized around did not exist: no undo entry
was ever keyed on a row. ThreadTagCommand stores thread ids and
MessageTagCommand stores message ids, and applyTagChange() looks its
target up by id, so an entry already survived its rows leaving the view.
A thread read out of the current view now leaves the list, which is
correct and would otherwise strand the reader, so MessageView grows a
notice saying the open thread no longer matches, with a button that
re-queries it. Recovery lists the whole conversation, expands it, and
restores the message that was on screen rather than reopening at the
first one.
Ten defects were found building this, nine of them by hand testing:
- SyncMonitor::start() polls synchronously, so an idle lock file emits
stateChanged(Idle) from inside buildUi() and the first handler to
touch a widget segfaults before the window exists.
- QTreeView sets a current index when it takes focus with none set, and
current drives loading, so new mail opened itself and was marked read
without the user having looked at it. Selection is now required.
- The notice outlived what it described, both when the pane was blanked
and when another message replaced it.
- Retiring the "Background sync completed" message left the bar claiming
a sync was still running: silent means saying nothing new, not leaving
a stale claim on screen.
- A thread root sets both the thread id and the message id, so treating
the message id as the message-row case discarded it for the commonest
way to open a thread.
- A freshly queried root does not know its own first message until the
tree loads, so recovery selected nothing and left the pane blank.
- A user query mid-recovery had its result hijacked by the pending
selection.
- MessageView emitted the recovery signal with its own members, so a
direct connection handed MainWindow references that runCurrentQuery()
then cleared by blanking the pane. The ids went empty mid-slot and no
recovery ever ran. Every test passed against this, because reaching a
slot through invokeMethod copies its arguments.
A Qt signal argument is a reference until something copies it. Emitting
a member to a slot that can re-enter the emitter is a use-after-write,
and it presents as a wrong value rather than as a crash.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Four faults from the first hand test, two of them behavioural.
An expander that opened onto nothing. setThreadMessages kept only nodes with
depth > 0, and notmuch_thread_get_toplevel_messages returns every message at
depth 0 when a thread carries no usable In-Reply-To, so a flat thread
contributed no children while its card still advertised the count. Measured in
the user's database: of 396 inbox threads three are flat, one of them nine
messages long, and every two-message thread of that kind was affected, which is
exactly why the fault looked like "the expander only works with more than one
reply". The rule is now position, not depth: every message except the first,
which is the root card itself. That is also the correct rule rather than a
workaround, since the row under the root is the second message however notmuch
chose to nest it.
The thread's first message was unreachable. Selecting a root card loaded the
whole thread, so the pane showed every message with only the last expanded, and
no row in the list offered the first one: the reply rows are messages two
onward. The root card now renders its own message, which is what the card
already claims to be. It keeps its thread id, unlike the message-row path, so
mark-read and the tag-change repaint still work; that is asserted, because
clearing it is the obvious way to write this and silently disables both. Before
the replies are loaded the model has no first message to name and the whole
thread stays the honest answer.
Dates ignored the locale. One hardcoded "yyyy-MM-dd hh:mm" produced a US-looking
format on an Italian desktop; QLocale::system() now formats it, and the width
reserved for the date comes from the same function so a longer locale cannot
clip.
The expander was a bare number on the card's own background. It is a pill now,
carrying "3 replies" (and "1 reply", singular), sized from the label actually
drawn and measured in both glyph states so it does not resize under the pointer
on click. Its fill is blended from Text toward Base rather than taken from
QPalette::Button, which is #2b2b2b against a Base of #2b2b2b on the user's
theme: byte identical, so the pill was invisible. A theme may make any two roles
equal; a blend is defined against the surface it sits on and cannot collide with
it. Checked by rendering both a dark and a light palette and looking.
|
|
Two entries, straight to notmuch. This adds a feature rather than replacing one:
the column header was decorative and nothing implemented click-to-sort, so
removing the header with the grid lost nothing.
Stored in uistate.conf, never in the hand-edited config, and range-guarded on
read: a stale file can hold anything, which is the lesson item 58 recorded.
SortOrder needed qRegisterMetaType despite carrying Q_ENUM. Q_ENUM gives the
enum a meta-object entry, not a metatype registered under the name invokeMethod
resolves, so the queued runQuery would have dropped its sort argument at runtime
and every query would have silently run newest-first. Nothing in the suite
exercises a real worker thread, so this was asserted directly rather than left
to a warning nobody would see. It is registered beside the type rather than in
MainWindow's constructor: a first attempt put it there and passed only because
the test that catches it never constructs a MainWindow.
The account dropdown's entries now carry their account's colour as a swatch,
which is what makes the accent bar on a card mean anything: a colour down a
card's edge says nothing until something maps it to a name. Raw colour here
rather than the blended line colour, since a swatch is a filled patch like a
chip rather than a thin line. Its test builds its own two-account config: reading
the environment's made it SKIP wherever no accounts are configured, which is a
test that asserts nothing while reporting success.
|
|
next_thread and prev_thread now walk with indexBelow/indexAbove, skipping
message rows, so they keep meaning thread-to-thread whatever is expanded.
Stepping message-to-message needs no code: QTreeView's own Up/Down walk VISIBLE
rows and already enter an expanded thread, and being the view's key handling
rather than a shortcut they stay inert when the message pane, a menu or an entry
bar has focus.
Item 60 turns out to have been fixed already, in 5487d58 on this branch, by
threadRowOf() walking up to the containing thread before doing the arithmetic.
The backlog entry was written against master, where that helper does not exist,
so it described a defect this branch had resolved a commit earlier. Verified by
writing both failing tests first and watching them pass: from the last reply of
an expanded thread, and from a thread root with its replies showing. They are
kept, because the property they assert is the one this change must not lose.
What the rewrite buys is that nothing is keyed on a row number any more, which
is the rule a deeper tree would break next.
Alt+Up/Down added alongside Ctrl+J/K. That required KeyMap::sequencesFor and a
move from setShortcut to setShortcuts, because the singular setter keeps only
the last binding and the second one was silently unreachable. Alt because
Shift+arrows is the built-in extend-selection that multi-row tagging depends on,
and because a bare arrow cannot be a window shortcut without breaking every text
field in the window, as Return already demonstrated. sequencesFor puts
sequenceFor's own choice first so the menus advertise an unchanged binding, and
sorts the tail, since QHash order is unspecified.
|
|
ThreadListView::paintEvent and its band arithmetic are deleted. The view existed
to paint a strip across five columns; with one column and one delegate painting
the whole card there is nothing to span, and the two faults that arithmetic kept
producing go with it: a deleted row cut in half, and every other row showing a
bare stripe.
What survives is the expander hit-test, because a delegate gets no click of its
own without an editor. It now asks CardDelegate for the rect rather than
recomputing it, so the drawn target and the clickable one cannot drift. The
siblingAtColumn(0) dance is gone: with one column, the index already is column 0.
Item 51 closes here rather than separately. A card is exactly viewport width, so
the view has no horizontal scroll range for a click to scroll into, and the test
asserts that directly.
Two rendering tests had to change how they measure, not merely which index they
name. The indent test asserted on visualRect, which now reports the SAME rect
for a thread and its reply by design, since setIndentation(0) leaves the indent
to CardLayout: it reads contentLeft off the layout instead. And the expander
test reported zero ink over a card the delegate paints 2183 pixels into, because
viewport()->render() returned a blank image, exactly as CLAUDE.md warns; it now
paints the delegate into an image directly and carries a guard proving the probe
can see ink before it reports finding none. Both were mutation-checked.
Two tests are deleted rather than ported. Both existed to prove the row-wide
strip spanned columns a delegate could not reach, which is a property of code
that no longer exists.
|
|
theImportantActionStillWritesTheFlaggedTag arrived from master, where the
thread list is a QTableView, onto a branch where item 20 made it a
QTreeView. findChild<QTableView*> returned null and the test failed on its
first QVERIFY, before reaching anything it was written to check.
Uses the branch's selectThreadRow helper, which is what every other test
here already calls: QTreeView has no selectRow of its own.
|
|
tagSelected resolved rows to threads with threadAt(index.row()), which is wrong
for a message row: a child's row number indexes its siblings, so acting on a
reply tagged whichever thread sat at that position in the list. It now routes
through ThreadListModel::scopeFor, and a message row's change is sent as message
ids down applyTags with its own MessageTagCommand for undo.
MessageTagCommand stores message ids where ThreadTagCommand stores thread ids,
and that difference is the point rather than an inconsistency: re-resolving the
thread on undo would restore tags across every sibling the action never touched.
sendMessageTagChange deliberately skips the optimistic model update.
applyTagChange is keyed by thread and would repaint the whole row as though
every message in it had changed, which for a one-message edit is a lie the user
watches correct itself on the next query. It keeps the two things that are NOT
optional: the edited-account set, resolved through the containing thread since
the account is a property of the thread, and holding the edit when a sync holds
notmuch's write lock, since the worker's read-write open blocks rather than
failing.
The scope is now stated before an action and after it, naming both the message
count and whether a whole thread went. This is what stands in for the
confirmation dialog CLAUDE.md rules out: undo is the safety net, and undo is
only usable if the user can tell that something larger than they meant has just
happened. Selecting a single message reports no count at all, since reading one
message is not a bulk action.
A mutation that routed message rows down the thread path SURVIVED the whole
suite: undo depth and status text are identical either way while every sibling
gets tagged. anActionOnAMessageRowTagsThatMessageNotTheThread exists because
that gap was found, and asserts on the ids actually sent.
anActionOnAThreadRowSaysItHitTheWholeThread reads the status bar BEFORE draining
the event loop. This binary has no worker, backlog item 36, so the queued write
reaches a database that has never heard of the thread and answers with
errorOccurred, which overwrites the status bar: draining first asserts on that
error and fails against correct code.
|
|
loadMessage queries by id: and returns one MessageRef, always matched, since the
user asked for that message by clicking its row and a stub would answer the
wrong question. An unknown id emits an empty vector rather than an error: a
stale row after a reindex is an ordinary race, not a failure worth the status
bar. The signal fires even when empty so the UI handler runs instead of waiting
for a reply that never comes.
The branch in onThreadSelected is placed BEFORE threadAt(), which is the whole
trap. threadAt takes a top-level row number and a child's row number indexes its
siblings, so handing a message row's number to it loads whichever thread happens
to sit at that position. Mutation-checked: with the branch disabled the test
reports thread 't1' for a reply belonging to 't2', a wrong answer plausible
enough to survive review.
m_currentMessageId and m_currentThreadId are mutually exclusive and each clears
the other, so a queued reply can tell which kind of selection it belongs to.
onMessageLoaded carries a third guard onThreadLoaded does not need: a reply
landing after the selection moved to a thread row would render one message where
the conversation belongs.
No mark-read timer for a message row in this pass. Marking one message of a
thread read is a per-message tag write and the pending-edit map is keyed by
thread; item 28 is the record of what happens when that count goes wrong.
|
|
Indentation alone still read as a table, which was the user's original
complaint about the whole item. Three cues now say the rows belong to the
thread above them: a spine down the left of the expanded block with a stub out
to each reply, a background tint, and text a size down and undimmed only when
unread.
Both colours are mixed from the palette rather than fixed, the same rule
readColour follows: a tint that reads as grouping on a light theme is invisible
or muddy on a dark one. The tint is deliberately near the threshold of noticing,
since it sits beside the deleted and spam fills, which carry real meaning and
must stay the loudest thing in the list.
The spine is accumulated across the visible reply rows and drawn once after the
loop. Drawn per row it left a gap at every row boundary and read as a column of
dashes rather than as the structure holding the block together.
Two bugs fixed here, both mine, both from the previous commit:
Clicking the expander did nothing. setRootIsDecorated(false), needed to stop
the style painting its own indicator under ours, also removed the style's hit
area, so the glyph rendered perfectly and was inert. ThreadListView handles the
press itself now, over the strip the delegate reserves, leaving the rest of the
subject cell to select the row.
Every click then expanded rather than toggling, because isExpanded and
setExpanded are keyed on column 0 and were being asked about the subject-column
index, which always answers false.
Visible, clickable and toggling are three separate properties and a test for
one passes against the other two being broken: the pixel test proved the
triangle was drawn while it could not be clicked, and the first click test
proved it opened while it could never close. The test now clicks twice and
asserts open then closed.
replyRowsKeepTheirTextUnderTheThreadLine covers the other trap. paintEvent runs
AFTER the cells, so the first version of the tint filled the whole reply row and
erased the sender and subject the delegate had just drawn: zero surviving text
pixels, a block of blank tinted rows. The fill and the stub stay in the band
below the text, where the tag strip lives on thread rows.
|
|
Both were reported from the running application after the previous commit
claimed them working, and the tests that passed could not see either fault.
The expander took four attempts, each of which looked right in code:
- QTreeView::drawBranches is the documented hook and does not work here. It
runs BEFORE the row's cells, so with the expander on a content column the
delegate's own background paints over it. A 60-pixel triangle survived as
8, indistinguishable from the theme's near-invisible dot.
- Sizing the glyph from the row rather than the branch rect put most of it
outside that rect.
- Moving it into SubjectDelegate but calling it from only the no-chip branch
left every real row without one, since every real row has an account chip
and takes the other branch.
It is now drawn by the delegate, which owns the cell and paints after the
background, from both branches, with setRootIsDecorated(false) so the style
does not draw its dot underneath.
The indent was 20px and invisible for a reason the geometry could not show: a
thread row draws an account chip before its subject and a reply row does not, so
a reply's text already starts about a chip's width LEFT of its thread's. The
indent has to beat that before any nesting reads at all, hence 72px.
The indent test asserted on visualRect, which was correctly indented the whole
time, and so passed against a build with no visible nesting. It now measures
where the TEXT lands, accounting for the chip, and fails at 20px. The new
expander test counts painted pixels of the glyph colour against a control row
with no replies, and fails when the call is dropped from either branch.
|
