| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
DraftStore::write() renders a built message into <folder>/cur with the
given Maildir flags, writing through QSaveFile and unlinking the previous
revision only after the new file is in place.
Two orderings here are load-bearing and both are covered by a test that
was checked against the mutation that breaks it.
The unlink runs only after the write has succeeded, so a failed save
leaves the previous revision intact rather than losing both. Provoking
that failure needs care: the plan's version used an unwritable path where
mkpath() fails and the function returns before reaching either the write
or the unlink, so a mutation moving the unlink up survived it. The test
uses an existing but read-only cur/ instead, where the failure lands at
the write.
And the size comparison stays ahead of commit() in the condition, because
QSaveFile::commit() returns true after a short write and renames the
truncated bytes into place: measured, write 4096 of 65536 with commit
reporting true and the file left in the listing. What leaves the
directory empty is the short-circuit returning before commit() is
reached, after which ~QSaveFile() discards the uncommitted scratch file.
Reducing the condition to !file.commit() looks like a simplification and
writes a truncated draft into cur/, where notmuch would index it and
mbsync would upload it.
Resolving the mail root stays the caller's job, per item 124; this takes
an absolute folder path and composes nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QP2g3b3kuLx6AYFCNEz6UR
|
|
Two silent failures on the path that produces bytes for other people.
A directory passed the attachment guard, because QFileInfo reports a
directory as existing and readable, and opening one read-only is legal. GMime's
base64 encoder then looped on read() returning EISDIR without advancing:
measured at 2.1 million failed reads in twenty seconds and still going. Since
build() runs synchronously from autosave on the GUI thread, dragging a folder
into a composer froze the whole application with the draft unrecoverable.
isFile() also excludes device nodes and FIFOs, which block the same way.
An unparseable recipient was dropped rather than reported. The old code
skipped anything that failed to parse and then only wrote the header if what
survived was non-empty, so a message whose only recipient was mistyped was
built with no To: header at all and reported success. With msmtp -t taking its
recipients from the headers, that is a message handed to the send command with
nobody to deliver to, and a copy filed in Sent that looks sent and reached no
one. A recipient the user typed and this cannot understand now stops the send,
the way a missing attachment already does.
The directory test carries a timeout deliberately: a regression there hangs
the binary rather than failing it. Two details make that work and the first
draft had neither. It must not join the worker, since a thread stuck in the
defect never returns and the join reproduces the hang instead of reporting it,
verified by reverting the fix: with the join the binary had to be killed at
150s with no verdict, without it it reports a FAIL and exits in 15s. The
result is shared through a shared_ptr so the leaked thread cannot write into a
returned stack frame.
Also: the no-address error names the account, since it matters once several
exist; messageId is assigned once on the success path rather than set early
and cleared on each failure, which is an invariant the next early return would
forget; and the Bcc comment now records that keeping the header stores the
blind list in plaintext in the sent copy and any draft, which mbsync syncs to
the server. That is accepted knowingly, and saying so stops a later reader
"fixing" it and silently breaking blind delivery.
One correction to the review that prompted this. The claim that
internet_address_list_parse returns a zero-length list rather than NULL did not
reproduce: measured on GMime 3.2 with a standalone probe, every garbage input
tried returned NULL, and no input was found producing a non-null empty list.
The length check is kept as defensive code and is documented as such rather
than as observed behaviour, since no fixture reaches it and a mutation on it
survives the suite. The defect itself was real and is what the test kills.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015muoUo2GdxmBDSp5vjYcbE
|
|
One built message serves three consumers: the autosaved draft, the bytes on
the send command's stdin, and the sent copy. A draft is therefore
byte-identical to what would be sent.
Three GMime defaults are wrong for this application and each is corrected
explicitly, because all three fail only on accented text and this user
writes Italian:
GMime encodes as iso-8859-1 unless told otherwise, so the subject carries an
explicit utf-8 argument. g_mime_text_part_set_text() encodes with whatever
charset is set when it is CALLED, so setting the charset afterwards produces
a part labelled utf-8 carrying latin-1 bytes; the content stream is built
directly instead. And neither Date nor Message-ID is generated unless asked
for, and a message without a Message-ID cannot be threaded by anything that
receives it.
Attachments are checked at build time rather than at attach time: a file can
vanish in between, and a message missing the thing it was written to carry
must never reach the send command.
An account with no address fails the build rather than producing a message
with an empty From. Config::account() returns a default-constructed Account
for an unknown key rather than failing, so without that guard a bad key
would produce silently malformed mail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015muoUo2GdxmBDSp5vjYcbE
|
|
toInt() and toLongLong() return 0 on failure rather than the default, so a
typo in autosave_interval_ms produced a zero-interval timer. That timer is
restarted on every keystroke, so it would fire on the next event-loop pass
and turn a 30 second debounce into a Maildir write per keystroke, each one
uploaded by mbsync: exactly the behaviour the debounce exists to prevent.
This file already had the right shape in five places, a checked parse that
reports the bad value and keeps the default. The [compose] keys were the
only numerics skipping it. The interval is also clamped, since nothing
assigns a meaning to a zero or negative autosave.
quote_position now warns on an unrecognised value, matching sync_on_exit,
language and date_format; the only silent fallbacks in this file are for
absent keys rather than malformed ones. And a missing `sent` folder is a
notice rather than a problem, because the spec blesses that configuration
and a modal on every launch for a permanently correct setup is how users
learn to dismiss dialogs unread.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015muoUo2GdxmBDSp5vjYcbE
|
|
DraftStore needs the same filename generation moveMessages() already has,
and duplicating it would duplicate a correctness property rather than a
convenience: the comment records that carrying mbsync's ,U= infix across a
folder boundary produced 'Maildir error: duplicate UID' on real mail.
A pure move with no behaviour change, committed on its own so a bisect can
tell it apart from the feature that needed it. The function gains its own
tests, including the UID-infix case that previously had none.
|
|
The header still credited CMARK_OPT_SAFE after the .cpp comment and the test
were corrected, which left the wrong mechanism named in the file
MessageBuilder's author will actually read.
Three test weaknesses, each measured rather than assumed. The accented-text
test survived a SYMMETRIC latin-1 mutation, since the round trip cancels for
codepoints under U+0100, so it now carries a character latin-1 cannot
represent. The tasklist test asserted on the bare word "checked", which
ordinary prose would satisfy, and now asserts the attribute. And the
extension registration is wrapped in a function-local static: cmark-gfm's
registry has no once-guard, and this project has a worker thread, so the
first call racing itself would tear the registry rather than crash cleanly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015muoUo2GdxmBDSp5vjYcbE
|
|
An account's ability to send IS its send_command's presence. Not a separate
receive_only key: with one key there is nothing to keep in step and nothing
to contradict, and a receive-only account is expressed by omission, which is
how one real account here is meant to work.
Startup validation follows the startup_query pattern, and is deliberately
asymmetric. A default_account that cannot send is warned about, because the
user named an account and expects mail to come from it. An installation
where NO account can send is not: that is a valid read-only installation,
and warning about it would train the user to ignore warnings.
Every [compose] key reads through value(key, default) rather than testing
contains(), because send_delay_ms = 0 is a real setting meaning 'send at
once' that a zero-test would mistake for unset.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015muoUo2GdxmBDSp5vjYcbE
|
|
CMARK_OPT_SAFE has had no effect since cmark-gfm made safe mode the
default; the flag is retained for API compatibility and the real
protection is that CMARK_OPT_UNSAFE is never set. Measured against
0.29.0.gfm.13: rendering with OPT_DEFAULT alone, with OPT_SAFE, and with
OPT_UNSAFE shows the first two suppress a script element and a
javascript: link while the third leaks both.
The comment credited the flag, which would have sent the next reader to
the wrong place, and the test could not tell the two apart: it would
have passed just as well with the flag deleted. What it has to guard
against is OPT_UNSAFE being introduced, so it now also asserts that
unsafe links are stripped, which is a protection this gets for free and
previously asserted nothing about.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015muoUo2GdxmBDSp5vjYcbE
|
|
The composer's body is markdown and the text/html part is generated from it.
cmark-gfm rather than plain cmark for autolink: under CommonMark a bare URL
in a mail body is not a link, and in mail it is expected to be clickable.
Three extensions are enabled and tables are deliberately not, since they
render badly across mail clients whoever generates them. Raw HTML in the
input is suppressed with CMARK_OPT_SAFE: the body is the user's own text,
but a body that can inject markup into its own generated HTML part is a
sharp edge with no upside.
The build needs TWO lookups. Only the core library ships a pkg-config file;
libcmark-gfm-extensions has none and is located with find_library, the way
notmuch already is. All three extensions live in that second library, so
finding only the first produces a build that compiles and silently renders
plain CommonMark.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015muoUo2GdxmBDSp5vjYcbE
|
|
The status bar's sync bar was a bare QProgressBar configured inline in
MainWindow, and item 123's send popup needs the same thing again. It also
needs the half MainWindow does not use: the popup drains a determinate bar
through its cancellable countdown and switches THE SAME widget to
indeterminate when send_command starts and the duration stops being
knowable. Building that inline a second time is what this removes.
BusyIndicator carries both modes. setBusy() resets the range as well as the
visibility, so the switch out of the countdown cannot leave the bar drawing
its last fraction, and setProgress() treats a total of zero as busy rather
than passing it through: setRange(0, 0) IS the indeterminate range, so a
zero total would otherwise hand the caller an animating bar while it
believed it had drawn an empty one.
Only the bar is extracted, not the status label the backlog row mentions
beside it. m_statusLabel has 34 uses across MainWindow for transient
messages, selection counts and sync phases; it belongs to the window rather
than to the indicator, and the send popup owns its own phase text.
The hidden-on-construction test needs a shown parent, which cost a mutation
to find. Measured against a standalone Qt program: a parentless widget
reports isVisible() false and isHidden() true whether or not hide() was ever
called, so both obvious assertions passed against a constructor with the
hide() deleted. What differs is WA_WState_ExplicitShowHide, and the
behaviour it produces appears only once a parent is shown, which is how the
status bar holds this widget.
All five mutations checked and killed: the zero-total guard, the range reset
in setBusy(), the value clamp, the show() in setProgress() and the hide() in
the constructor.
|
|
Reported by hand after the item 127 fix: right-clicking a link still
offered Save link.
It had been deferred to item 114 alongside Save image, on the grounds that
both are inert without a downloadRequested handler. That is true and it was
the wrong conclusion, because the two are not the same question.
Save image is content the message already carries, and item 114 is about
making it work. Save link fetches a remote URL chosen by the sender,
through the pane's profile, which is the one profile in this application
that must never fetch remote content: that is what m_allowRemote and the
interceptor exist to prevent. Answering it with a download handler would
put a network fetch of attacker-controlled content behind one context-menu
entry.
Saving what the user actually wants already has a path that never touches
the network: saveAttachment(), which writes a MIME part already parsed into
memory and sanitises the filename.
So it is removed rather than implemented, and the test asserts its absence.
Item 114 now carries the constraint that follows: a downloadRequested
handler added to make Save image work must not make Save link reachable
again, which the natural per-profile implementation would do by default.
Mutation checked: dropping the entry from the filter fails the test with
"a link action survived: Save link".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
Items 126 and 127, in one sitting because the second is only safe after
the first.
126: an anchor carrying target="_blank" did nothing when clicked, with no
error and nothing on screen. Chromium routes such a click to
QWebEnginePage::createWindow() rather than to acceptNavigationRequest, and
MessagePage did not override it, so the base implementation returned
nullptr and the URL was discarded before any of our code saw it. Plain
anchors were unaffected and already worked, which is why this presented as
"HTML mail is broken" while a text mail's links opened: marketing HTML sets
_blank on practically every anchor.
createWindow() receives a WebWindowType and no URL, so an override cannot
simply read the target: it arrives afterwards as a navigation on whatever
page is returned. LinkRelayPage is that page. It has no view, hands the URL
to the same handler the plain-link path uses, refuses the navigation, and
deletes itself. Nothing is ever fetched and no second QWebEngineView is
created.
127: OpenLinkInNewTab, OpenLinkInNewWindow and OpenLinkInThisWindow join
removeBrowserActions()'s list. Item 100's list is the PAGE actions and was
tested by right-clicking the page; these appear only over a link, so it
never saw them. CopyLinkToClipboard stays, being the fallback for any link
that will not open. The order matters: 126 gives the page a working
createWindow(), so those entries would have stopped being dead and started
opening links into a tab that does not exist.
Testing needed two seams. The click cannot be synthesised, since JavaScript
is off in this profile (measured: runJavaScript returns an invalid QVariant)
and a synthetic press would depend on the anchor's rect and the desktop's
fonts; setUrl() is no substitute because it arrives as NavigationTypeTyped.
clickLinkForTest() and relayBlankTargetForTest() drive the real overrides on
the real page, and setLinkOpener() substitutes a recorder for
QDesktopServices::openUrl.
Both routes are asserted rather than only the broken one, since they share a
handler now. Three mutations checked and caught, including the filter also
removing CopyLinkToClipboard, which a later sweep of "dead link actions"
would otherwise take silently.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
notmuch can be configured with `mail_root` and `path` as separate keys,
which puts the Xapian index outside the Maildir. Under that layout
notmuch_database_get_path() returns the INDEX directory, and the worker
treated it as the mail root at four sites.
The consequences are not symmetric. Message paths resolved to `../..`
escapes that match no account prefix, which is a display defect. But
moveMessages() composes its destination from the same root, so Delete
would have written into the Xapian tree: outside the Maildir, invisible
to mbsync, and gone from every other client. That is the stranded-mail
failure of item 103 with a new cause.
notmuch_config_get(NOTMUCH_CONFIG_MAIL_ROOT) is correct under both
layouts, so no conditional is needed. Verified against the live database:
with only `path` set it returns the same string as get_path(), making
this a no-op for the current configuration.
The fixture gains an opt-in splitIndex(). That is load-bearing rather
than convenience: in the ordinary layout the index lives inside the mail
root and both accessors return the same string, so a test written
against it passes whichever one the code uses. All three new tests fail
against the old accessor, confirmed by mutation.
Also records the finding as backlog item 124, and corrects item 121's
timings, which had been copied from item 74 rather than measured. A cold
run seven minutes after boot, with the index verifiably unread, gives
2008 ms to the first rows and 38618 ms to a complete list, against the
642 ms and 5714 ms recorded there.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
The user's preference after seeing item 115 ship: a small transient in the
bottom right of the pane with a checkmark, rather than a status bar message at
the far end of the window. A copy happens in the pane, so the confirmation
belongs there.
Three properties are load-bearing and each has a mutation that fails. The toast
is a hand-placed CHILD rather than a layout item, because it floats over the
message instead of taking a strip away from it: nothing reflows when it appears
and the text just copied does not jump. That is why resizeEvent() is overridden,
since a hand-placed child does not follow its parent. It is autoFillBackground
and painted from the theme's ToolTipBase/ToolTipText, so it stays readable over
a rendered message and follows the desktop theme the way the document already
does. And its timer is restarted rather than started, so a second copy gets its
own full reading time instead of inheriting what is left of the first.
The resize test was wrong on its first draft and passed against the mutation it
exists to catch. It grew the pane, which moves the right and bottom edges away,
so a toast left at its old position still satisfied "inside the pane"; measured
green with the reposition deleted. It shrinks now, where a stale position lands
outside the new rect, which is also what the user would see.
No new strings: the four messages are unchanged, only where they appear.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
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>
|
|
Items 115 and 117, both from the user's notes.
Select all was never in Chromium's menu for this pane, measured by hand with a
selection active and against a build with removeBrowserActions() reverted, so
the filter is not what removed it. MessageView::addPaneActions() supplies it,
static and taking the menu, mirroring removeBrowserActions() beside it. Two
comments claiming the standard menu already offered it are corrected; either
would have sent the next reader down the same three wrong theories the item
records.
The copy entries all worked and none of them said so. Four now report through
the pane's existing statusMessage, each naming what it copied rather than saying
"Copied", which is the item's own constraint when three of them sit together in
one menu. Connected to the page's own QActions, so the report follows the entry
wherever it is triggered from.
The two differ in what can be tested, and the tests say so rather than papering
over it. The copy path is fully covered: triggering the action runs the
production path, and mutations for a duplicated message and an unwired entry
both fail. addPaneActions() is covered, but showBodyContextMenu() CALLING it is
not and cannot be, since createStandardContextMenu() returns nothing outside a
real context-menu event; a mutation deleting that call leaves the suite green,
measured. The call site is a hand test and the test file records that so nobody
adds an assertion that appears to cover it.
The copy strings are QT_TR_NOOP inside an array, which CLAUDE.md warns extracts
nothing at file scope. Verified rather than assumed: lupdate found all four
under the MessageView context, because the array sits inside a member function.
387 finished, 0 unfinished.
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>
|
|
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 thread MOVES every message now, and a move needs message ids and
file paths that the UI does not hold: a thread the user never expanded has
no nodes in the model for its replies, so those live only in the database.
resolveThreadMessages() answers with both, in one combined query, for the
same reason applyTagsToThreads() resolves threads here rather than in the
UI: a query per thread reopens the same Xapian cursor once per selected
row.
Paths come back RELATIVE to the database root, matching
ThreadSummary::firstMessagePath, because the UI knows accounts only by
their maildir, itself a database-relative prefix. Without them the caller
would know which messages to move and not where any of them belongs.
Tags come back too, because Restore reads a message's `deleted-from:` tag
to decide where to send it and an unexpanded thread's messages have no node
to read tags from.
They are joined by a TAB, not a space. A notmuch tag may absolutely contain
a space: the folder "Inbox/SlackBuilds users" produces
`deleted-from:Inbox/SlackBuilds users`, and splitting that on spaces
truncated the folder to "Inbox/SlackBuilds". Restore then moved the
messages into a folder of that name, CREATING it, so real messages ended up
in a directory mbsync does not sync and read as missing. Under `Create
Both` that folder can propagate to the mail server. A tab cannot appear in
a tag, since notmuch's own dump format is line-based and
whitespace-delimited.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
validateTagName() ran on the removal list as well as the addition list, so
a tag whose name contains a space could be seen on a message and never
deleted: the one dialog that could clear it refused the only text that
names it, and it did so with a modal warning, so the dialog would not even
close.
Whether a tag SHOULD exist is a separate question from whether the user may
delete one that already does, and the answer to the second is always yes.
Validation now runs on additions only, which is where the rule earns its
keep: it stops a troublesome name being created.
Reached by a real Maildir folder named "Inbox/SlackBuilds users", whose
origin tag carried the space through. Only the TYPED route was ever
blocked; unchecking the tag in the list appends to the removal list after
validation has run and worked throughout. The test asserts both routes for
that reason, and asserts that ADDING a spaced tag is still refused, since
the fix must not weaken the rule it narrows.
The natural mutation for this test hangs rather than fails: restoring the
validation raises a modal warning with nothing to dismiss it. The test
avoids calling accept() on the add-rejection case and asks the validator
directly, and the mutation check drops the tag silently instead.
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>
|
|
The first mutation here that is not a notmuch tag. Indexes the new path
before dropping the old one, since removing the last filename for a
message id deletes the database entry and every tag on it.
|
|
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 trash key is mandatory: Delete moves a file into it, so an
account without one cannot delete at all. Report it as a config
problem naming the account and the key, rather than degrading
Delete silently, per the existing "a warning the user cannot act on
teaches them to ignore warnings" rule (item 83).
Several existing test fixtures loaded accounts with no trash key and
asserted zero problems/warnings; added trash=Trash to those where it
was incidental to what the test actually covers.
|
|
|
|
The `note` field explains why a rule is shaped the way it is, and it was
reachable only by selecting the rule and reading the editor form, which is the
wrong way round for the one field that says what a rule is for.
Note is the LAST column, after Matches, at the user's request: a note is prose
and the widest thing in the table, so it belongs where it can run on without
pushing the narrow columns off screen. That is fiddlier than it looks, because
"Matches" is not in the Column enum at all: it is appended past the end at
index ColumnCount. Note therefore has to be declared before ColumnCount and
still draw after it, and setColumnCount takes a new ColumnTotal rather than
ColumnCount + 1. Both columns hold text, so a mix-up puts the counts under
Note and looks entirely plausible; the test asserts the counts land under
Matches as well as asserting the header order, since the header assertion
alone passes with the two swapped.
The cell is simplified(), because a note is free text and a newline truncates
a tree row at it. The full text is the cell's tooltip and is untouched in the
editor.
Also fixes a defect found on the way, which is not in the backlog entry.
QHeaderView::restoreState REFUSES a state saved with a different column count,
returning false and leaving the header untouched, which is what every existing
uistate.conf now does. The restore path set m_columnsSized and
m_countColumnSized regardless, spending the one auto-size each column gets on
a restore that did nothing: the new Note column would have opened at its
default width, once, permanently. Now guarded on the return value. Upgrading
costs one reset of this dialog's column widths, which is unavoidable, since
the saved state genuinely describes a table that no longer exists.
Backlog item 102.
|
|
The pane's context menu started from QWebEngineView::createStandardContextMenu()
and kept it whole, so it offered Back, Forward, Reload and Save page. None of
them can apply: every message is rendered with setHtml() from memory, so there
is no history to go back to and nothing to reload, and the request interceptor
blocks everything by default. They were inert as well as meaningless.
removeBrowserActions() matches on the QAction pointer returned by
page->action(), never on the entry's text, which is translated: a text match
would work in English and fail in every other locale, which is a defect no
test written in English would catch. Removing entries also strands separators
at the edges or doubles them up, which reads as a menu that lost something, so
the filter sweeps them; Qt offers nothing for this.
View source is deliberately NOT filtered. It was removed with the other four
at first, which was an overreach: the user asked for four and view-source has
a real document and a real use. Chromium's own entry cannot work here either,
since it navigates to view-source:<url> and MessagePage refuses that, so
backlog item 113 implements it as our own plain-text dialog.
The test builds a menu by hand, which is right for testing the filter and
proves nothing about what Chromium's real menu contains. That limit is stated
at the test, and is why it does not assert on SelectAll: the real menu has
never offered it, verified by hand against a build with this filter reverted
(backlog item 117).
Backlog item 100.
|
|
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>
|
|
The interface language followed the environment and nothing else, so
choosing it meant setting LANG for the whole application. [general]
language overrides it in both directions: it selects Italian on an English
desktop, and en_US forces English on an Italian one.
A short code or a full locale name both work, since Qt resolves "it" to
it_IT when the QLocale is built and QTranslator::load falls back from
qtmaildir_it_IT to qtmaildir_it. "system" is the default written down, so
the default can be expressed rather than only reached by deleting the key.
Validated on the locale NAME rather than on whether a translation loads,
because those are different questions and only one is an error. QLocale
accepts any string and degrades an unrecognised one to C rather than
failing, so `language = itallian` loads no translation and is otherwise
indistinguishable from asking for English on purpose; meanwhile
`language = en_US` legitimately loads nothing, English being the source
language and shipping no .qm. Checking the name separates the typo from
the deliberate choice, and the typo is reported.
The translator is now installed after Config is loaded, since the config
is what chooses it. The cost is that config warnings are generated before
the translator exists and are therefore built in English; retranslating
them would mean re-running load(), and a warning about the config file is
the one string a user can still act on in either language.
Verified against the real loader across six configurations, under both
LANG=en_US and LANG=it_IT: short and full codes select Italian, system and
an absent key follow the environment, en_US forces English whatever the
environment says, and a bad name reports a problem and falls back. Two
mutations checked: dropping the name validation and treating "system" as a
locale name each fail a test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Nothing loaded a translation before this: no QTranslator, no .ts file and
no build rule, so every string was English whatever the locale said. The
language now comes from the environment, LANG=it_IT.UTF-8, and any other
locale runs in English as before.
The audit found that the tr() discipline was largely holding, and found
eight strings that could never be translated into any language. kFields[]
in tagrulesdialog.cpp declared the rule-builder field labels with
QT_TR_NOOP inside an anonymous namespace, where lupdate reports "tr()
cannot be called without context" and extracts nothing, while the use site
calls TagRulesDialog::tr() on them at runtime. From, To, Cc, Subject, Tag,
Folder, Attachment and Date: the whole vocabulary of the rule builder,
absent from every translation file that could ever exist. The source
compiles and reads correctly; only lupdate reveals it.
Q_DECLARE_TR_FUNCTIONS is not the fix for that case, though it is the fix
for a free function calling tr(). Measured against lupdate: a class
carrying the macro beside the array still extracts 0 strings, because the
context must be attached to the literal itself. QT_TRANSLATE_NOOP names it
explicitly and matches the tr() that already reads them, so the use site
needed no change.
Twenty configuration and keybinding warnings were not translatable either.
They are user-facing, reaching the status label and the "Configuration
problems" dialog. Config already had the tr() macro; KeyMap needed it.
Translating the filter labels then broke startup_query, found in hand
testing: a filter's name is a translated label, so `startup_query = Inbox`
matched nothing where the filter shows as "In arrivo". The application
opened a different view and reported the user's own working config as
invalid. Resolution matches the generator as well now, which is stored in
queries.json and identical in every locale; the translated name still
works. The regression test installs a real QTranslator rather than a stub,
since the bug lives in the gap between the stored string and the displayed
one, and it writes a queries.json because the warning it asserts on is
guarded by a non-empty saved-query list: without one the branch never runs
and the test passes against a broken check.
main.cpp's --help and --version stay bare printf, as they run before
QApplication exists and no translator could serve them.
Verified per the backlog's own standard, that lupdate output is the
evidence rather than reading: 355 strings extracted with zero context
warnings, where before there were 327 with eight; lrelease reporting 355
finished and 0 unfinished; the built .qm loaded in a standalone probe
printing "From -> Da" and both Italian plural forms; and the install rule
placing it where main.cpp looks. test_translations guards it and was
mutation checked, failing on an emptied translation and naming the defect
when QT_TRANSLATE_NOOP is reverted.
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 93, the Config half. Four built-in filters, Unread, Inbox, Flagged and
Sent, as generated entries in kQueryGenerators, which was already a closed set
validated on load for Sent alone.
resolvedQuery() gains an overload taking an account key, and that is what makes
a filter compose with the account dropdown instead of fighting it. A generator
is asked for the account's OWN query rather than having its all-accounts query
wrapped in a scope: wrapping gives
path:"a/**" and (path:"a/Sent/**" or path:"b/Sent/**")
which returns the right rows only because path: is hierarchical, so a row-count
test passes against it. The tests assert on the query string for that reason,
and the mutation putting the wrap back fails two of them.
An ordinary saved query ignores the account key and keeps resolving through its
own stored account, which is the behaviour item 90 leaves alone.
matchNothingQuery() exists because an empty query means "match everything" to
notmuch: an account configuring no sent folder would otherwise give a button
labelled Sent that shows the entire Maildir.
Config gains Q_DECLARE_TR_FUNCTIONS for the filter names, which are button
labels. The generator names are not translated: they are matched against the
closed set and stored in queries.json, so translating them would make a file
written in one locale unreadable in another.
No UI yet, and no migration: the query row still builds from pinned saved
queries.
|
|
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>
|
