| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
A composer is deliberately parentless, so that it appears in the task
switcher and stays usable while the main window is. Qt therefore does not
take it down with that window, and being a live top-level it kept the process
alive: the main window vanished, the composer stayed on screen with nothing
behind it, and closing it then raised the unsaved-edits dialog for a session
the user had already ended.
The quit path already ASKED about those edits and saved them. What it never
did was close the windows afterwards.
Closing rather than deleting: WA_DeleteOnClose is set on every composer, so
close() is what frees them, and it lets ComposeWindow::closeEvent() run its
own draft handling on the way out. Iterating a copy of the list, since
closing runs the `closed` handler and that mutates m_composers.
Placed last, after every route that turns back has returned: reaching it means
the application really is quitting.
The test opens TWO composers, so a fix that closed only the last one cannot
pass it. Mutation-checked by removing the loop.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q2koFevoSxTLhfexJTZWQd
|
|
The reply family is disabled on mail that arrived at an account with no
send_command, behind a ribbon in MessageView naming the account and the key
to add. save_message is deliberately never disabled: it is the escape hatch
for exactly that case.
The ribbon is a WIDGET in the pane's layout, never markup inside the web
view. Composing HTML from configuration into the one document that renders
input from strangers is the wrong direction, and the header row is already a
widget for the same reason.
Compose itself is disabled only when NO account can send, and that state is
not warned about at startup: an installation with no send_command anywhere
is a valid read-only installation.
Every reply resolves through messageScopeFor(), not threadFor(): a thread
row means the one message its card shows. Replying to a thread is
meaningless; a reply answers a message. The context is built from the
DATABASE rather than the model, the rule Restore already follows, because a
row whose state has not been re-queried carries stale values and a reply
built from one would carry the wrong recipients.
The mail root crosses from the worker as its own signal. There was no route
for it at all: mailRootOf() is file-static in notmuchworker.cpp, and item
124 records that composing a destination from database.path writes into the
Xapian tree under a split index. The test uses NotmuchFixture::splitIndex(),
the only layout where the two accessors disagree.
A thread row's path is RELATIVE to the mail root while a message row's is
absolute, so the account lookup matched nothing and the reply family was
dead on mail from an account that could send. Found by the positive guard
test rather than the negative one, which passed throughout for the wrong
reason.
The quit path checks the failed-save case FIRST. In the ordinary case
nothing is lost by saving; there, saving is what is already not working, so
the dialog says plainly that quitting loses that text rather than offering a
save that will fail again. Both dialogs name the composers, and the ordinary
one asks once whatever the count, because three modals in a row is worse
than a coarse answer. Its wording says drafts already saved stay in the
folder, so Discard cannot read as 'delete my three messages'.
The Save loop holds QPointers, not raw pointers. A deleteLater() posted
while a nested exec() runs IS processed by that nested loop, measured in a
standalone program: the guard nulls before the modal returns. Closing a
composer while the quit dialog is up therefore freed a window the loop then
called saveDraftNow() on, crashing at the exact moment the application
promised to preserve that text.
A compose request that matches nothing clears itself and says so. It was
cleared only on a match, so a message deleted between selection and Reply
left the request armed for the session: Reply did nothing, and the next
ordinary click on that message opened a composer nobody asked for while the
pane stayed blank.
Forward carries the original's attachments, which the context has always had
a field for and nothing ever filled, and seeds its HTML toggle from
[compose] send_html. Only Reply seeds that from the original.
save_message keeps its filename inside the chosen directory and no longer
overwrites a file already there. The check was correct and untested: the
test asserted through Attachment's helpers rather than through the function
production calls, so deleting the containment check outright left it green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TvwDptMWxjqhbCmjxwcSZ2
|
|
A separate top-level QMainWindow, one per draft, several open at once. A
modal dialog cannot consult another message while writing, which is most of
what replying is, and taking over the message pane fights the pane that
exists to show what is being replied to.
No geometry save and no restore, deliberately. Under a tiling compositor
saveGeometry stores normalGeometry while the compositor owns the tile, so
the restore is correct and looks broken; a whole session went into that
once.
Autosave is a debounce AND a dirty check: an unchanged message writes no
file and provokes no sync. The check is on a fingerprint of the
OutgoingMessage, NOT on the built bytes as the plan drafted. GMime is given
a fresh Date and Message-ID on every build, so two builds of an unchanged
message never compare equal; a check on the bytes would have read as
working while writing a file, and an mbsync upload, on every debounce.
Checking before the build also skips the blocking build for the no-change
case, which is the common one.
closeEvent writes the draft when the buffer is dirty. Without it the
debounce is a hole rather than a delay: typing a paragraph and pressing the
window manager's X inside the interval loses it silently, since
WA_DeleteOnClose destroys the window immediately afterwards. A failed save
there does NOT refuse the close, because a window that will not close
because it cannot save is worse than one that closes having raised the
banner, which is what the quit path reads.
One flag covers a send, countdown included. An earlier revision had two,
and the narrower "committed and running" one reads as the honest thing to
guard a live SMTP conversation with. It is not: a close during the
countdown destroys the parented SendDialog, committed() never fires, and
the user pressed Send, watched a countdown, and believes the mail went. The
narrow flag was also written in three places and read in none.
A failed draft write raises a persistent banner rather than a modal or a
fading status line. A modal mid-sentence is hostile while the user is
typing, but the warning must survive until it is dealt with, because the
quit path escalates exactly this state to a dialog on the way out. An
account with no drafts folder reports success rather than failure: nothing
was written and nothing failed, and a false there would make the quit path
offer a retry no retry can change.
A failed send saves the draft before reporting. send() builds from the
widgets without saving, so the revision on disk is whatever the last
debounce wrote: edit, send, fail, close, and the user gets the older text
back, having watched their correction be sent.
A failed sent copy after a successful send is a modal, and never a send
failure: the message went, and reporting otherwise makes someone send it
twice. It is the one failure here that silently diverges what the recipient
received from what the local archive shows, and nobody discovers a missing
sent copy by noticing a line that appeared for a few seconds.
The formatting toolbar applies its edits through a QTextCursor document
replacement inside one edit block, NOT setPlainText as the plan drafted.
Measured against a real widget: setPlainText destroys the document's undo
stack and resets the cursor to 0, so every toolbar press would throw away
everything the user could undo. The cursor route leaves undo available,
collapses to a single undo step, and emits textChanged once. The seeded
quote is cleared off the undo stack afterwards, since it is not an edit the
user made and one Ctrl+Z on a fresh composer must not wipe it.
The per-send connect carries Qt::SingleShotConnection. MessageSender is a
long-lived member, so a bare connect accumulates a permanent receiver per
send and the second result runs both lambdas, the first still holding the
first message's bytes: it files a sent copy of the wrong message and acts
on a dialog it already destroyed. Covered by a test that sends, fails,
corrects and sends again; without the flag it segfaults in QLabel::setText
on the destroyed dialog. Its companion disconnect takes the specific
connection handle rather than every finished receiver on this object, so a
later observer cannot be killed silently.
The attachment warning states sizes with a decimal and a stepped unit.
Integer MB division read as "'x' is 0 MB. Many mail servers refuse messages
above about 0 MB." for any attachment_warn_bytes below a megabyte, in both
halves of one sentence.
The autosave timer is created before buildUi(), which is load-bearing:
buildUi connects every field to markDirty and seeding then fills those
fields, so markDirty runs during construction. Created afterwards it is a
null dereference on the first seeded field, which is every composer.
Twenty-six cases in test_mainwindow, each mutation-checked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Handlers are empty for now; this commit is the registration, so the three
coverage tests guard every later task rather than being satisfied at the end.
Two corrections to the spec, both found in the code rather than assumed. It
calls for a new top-level Message menu and one already exists, so these join
it; two menus named Message would be a defect. And it says every action
needs a binding, which item 132 changed while this was being planned:
save_message ships with no chord, since it is the rarely-used escape hatch
and menu reachability is now the rule that must hold.
reply_no_quote shares reply's icon and is added to the no-duplicate-icons
exception list for the same reason the five thread actions are: it never
reaches the toolbar, and a menu entry always carries its text. That list is
renamed menuOnlySharedIconActions, after the property that earns the
exemption rather than the tier that first needed it.
Bindings are provisional. The user intends to rework them, and Ctrl+Alt+R
for reply_no_quote is an imperfect fit since that tier elsewhere means a
wider scope rather than a variant.
The six labels went through a mnemonic pass that nothing enforced before.
Four of them collided inside the Message menu on first writing, and the
whole class was invisible to a green suite: Qt does not error on a duplicate
mnemonic, it cycles the highlight instead of activating, so the key simply
stops working. Item 57 had already decided this rule by rejecting a label
that would have collided, but it lived in prose and in one test's comment,
which is precisely why it was broken again here.
noMenuHasTwoEntriesSharingAMnemonic() enforces it now, scoped per menu since
a mnemonic resolves among the open menu's entries, and keyed on
QKeySequence::mnemonic() rather than on parsing & by hand, because && is a
literal ampersand and only Qt answers which key it will dispatch. Three
pre-existing collisions are a named freeze list rather than a silent fix or
a narrowed test: Alt+R three ways and Alt+S twice in Message, Alt+O in View.
Renaming entries a user has had in their fingers since 0.1.0 belongs to the
shortcuts rework, and the freeze is written as exact groups so a new entry
joining any of them still fails.
Two of the test's own design choices came from mutation checks that failed
for the right reason while reporting the wrong thing. Reporting collisions
as pairs was order-dependent, so a new colliding entry re-keyed a frozen
pair and the fresh defect read as "a frozen collision no longer happens";
matching frozen entries by whole string broke the same way, since a growing
group stopped matching its frozen text. It reports whole groups and matches
on menu plus key.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FXF741wz4SY7j5dqvAxMU5
|
|
everyActionHasAShortcut() was written when the action list was short and
every action plausibly deserved a chord. Item 123 adds six more, and under
that rule each one consumes a key sequence whether or not anyone would ever
press it. Rarely-used actions were being given chords to satisfy a test
rather than because a user wanted them.
everyActionIsReachableFromAMenu() is the rule that actually matters, and it
already has the right shape: it is what stops an action shipping invisible,
which is the defect item 103 found when `restore` was reachable by a chord
and by nothing a user could see. Discoverability comes from the menu. A
shortcut is an accelerator for the things done often.
Nothing replaces the deleted test and nothing else needed changing:
showShortcutReference() already prints `(unbound)` for an empty sequence, so
the code anticipated this and only the test forbade it.
Verified rather than assumed: with `tag_rules` unbound in defaultBindings(),
an action that is registered, menu-reachable and carries an icon but has no
chord at all, the full suite passes. Before this commit it failed.
CLAUDE.md's "adding an action is FIVE places" paragraph is updated, including
its count of how many are test-enforced, which drops from four to three.
|
|
mbsync's manual, under "the more efficient default UID mapping scheme": "it is
important that the MUA renames files when moving them between Maildir folders",
and "the general expectation is that a completely new filename is generated as
if the message was new".
qtmaildir is that MUA and did not rename. moveMessages() kept
QFileInfo(from).fileName() verbatim, `,U=<n>` included. That infix is mbsync's
per-folder IMAP UID, so carrying it across a folder boundary makes it a claim
about a folder the file is no longer in; moving a message out and back then
reinserts a UID the server has since reassigned. Reported by the user as
`Maildir error: duplicate UID 1`, and measured on the real Maildir: four
collisions in one folder, eight distinct messages, none lost.
freshMaildirName() regenerates the unique part and keeps ONLY the `:2,<flags>`
suffix. Keeping the flags is not a contradiction of "as if the message was new":
they record seen, flagged and replied, and maildir.synchronize_flags is true, so
dropping them would mark every deleted message unread and lose Important on the
way to the trash.
Two things fell out of the change and both were defects waiting to happen. The
already-in-the-destination guard compared full PATHS, which worked only because
the name was carried across; with a fresh name it can never be true, so a
message already in the destination would be renamed on every move. It compares
directories now. And test_mainwindow's folderHasMessageFile() matched on the
filename stem, so all fifty-odd assertions using it began reporting "the file is
not there" about files that were there. It reads the Message-ID out of each file
instead, which is what those assertions always meant.
Three mutations fail: the old name carried across, the flags dropped, and the
uniqueness counter frozen so two messages moved in one batch collide.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Reported from a hand test: Restore moved the message correctly and the row it
came from sat in the trash list until the Trash filter was clicked again.
The trash view is path-based, so a restored message stops matching the query
the list was built from. That is a state no tag change can express, and nothing
in onMessagesMoved() removes a row, deliberately: in an ordinary view a deleted
message's card should stay put, since one deleted message does not doom the
conversation.
refreshCurrentQuery(), not runCurrentQuery(). The refresh runs immediately
after the undo entry is pushed, and re-running the query outright clears the
undo stack, which would make Restore the one mutation in the window with no way
back. Gated on isShowingTrash() rather than on the destination, because a
Delete is a move too and reaches the same slot.
Three tests, each catching a different mutation: the row leaves, undo survives
the refresh and still moves the file back, and a delete outside the trash view
leaves its row alone.
The third one was wrong on its first draft and passed against the mutation it
existed to catch. It used a `tag:inbox` view, which looks ordinary but which a
deleted message keeps matching, since Delete adds `deleted` and the origin tag
and removes nothing. A path query on the inbox folder is the honest instrument:
the file really leaves, so the row survives only because nothing refreshed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The spec's ordering bullet had only half a test. moveMessagesReportsOnlyWhatMoved()
covers the worker refusing to report a message it did not move; nothing covered
the window writing tags only for what the worker reported.
The failure is provoked by putting a FILE where the trash folder must go, so
the Maildir subdirectories cannot be created under it. A read-only directory
would be ignored by a test running as root, which this must not depend on.
A tag written anyway is the exact half-done state item 103 removes: a message
marked deleted, its file still in the inbox, and its origin tag lying about
where it went. The mutation putting that back fails this test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
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.
|
