| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
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
|
|
MarkdownFormat, task 8 of the compose-and-send plan. Three free functions
over (text, selection start, selection end) returning the new text and the
selection that follows it, so the grammar is tested without a widget.
Three gaps in the plan's draft, each now pinned by a test checked against the
mutation that breaks it:
- QString::lastIndexOf INCLUDES the position it is given, so quoting with the
cursor at the end of a line found that line's newline and quoted the
FOLLOWING one. The draft's fixtures never placed a cursor there.
- A backwards selection was normalised but never tested, so the swap was
unguarded; a right-to-left drag is an ordinary gesture and Qt reports the
anchor after the cursor. normalise() now swaps and clamps in one place.
- A blank line inside a quoted range produced "> " with trailing whitespace,
which editors and mail clients strip anyway. It is written bare.
Two further defects came out of review:
- quote()'s selectionStart was unasserted for any block not starting at line
zero. Hardcoding it to 0 passed all nineteen tests, because the one test
naming the property quoted the first line, where right and wrong coincide.
A wrong selection there means a second press quotes a line the user never
selected, and a following Bold bolds the wrong text.
- A selection splitting a surrogate pair split the character across the
inserted tokens, leaving invalid UTF-16. Not reachable from the toolbar,
where arrow keys and mouse hit-testing both move in whole clusters, but
reachable by any code computing a position arithmetically. normalise()
nudges off a low surrogate; a collapsed cursor moves back on both ends,
since widening would turn "insert an empty pair here" into "wrap the emoji".
The buttons stack rather than toggle: a second Bold press gives ****this****,
and a second Quote press nests. That is what the spec specifies, and the
preserved selection exists so a second press can apply a SECOND token. A
toggle was built during this task at the user's request and reverted on
finding it contradicts the spec at two sites; it is recorded as backlog item
135, where the unanswered question is what replaces bold-then-italic.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LoaLBowZ6w1JNx6SEhDP1L
|
|
ComposeContext, task 7 of the compose-and-send plan. Address parsing,
recipient derivation, the References chain, subject prefixing and account
resolution, as free functions over values so they test without a painter.
Recipient derivation was designed from the spec rather than transcribed: the
plan's draft omitted it and its tests could not compile, calling
QVERIFY(config.load(path)) against a void return.
Six defects found in review, each pinned by a test checked against the
mutation that breaks it:
- Message-ids reached GMime bare, and GMime writes an EMPTY header for a bare
addr-spec rather than complaining. In-Reply-To and References both shipped
blank, so every reply would have arrived as an orphan thread with nothing
wrong to see locally. MessageBuilder now brackets on write, in the one place
that composes those headers rather than in each caller.
- internet_address_to_string was called with FALSE for the encode flag, so a
display name carrying a raw newline rendered with the newline intact. That
is a header-injection primitive.
- A reply to the user's own message addressed the user. It now goes to that
message's original recipients, mirroring their To/Cc split, which is what
the Sent view and a follow-up on unanswered mail need.
- A From parsing to no mailbox left To empty, reachable from real mail
("From: Mailer Daemon"). MessageBuilder treats an empty recipient list as
success, so the message would have been handed to the send command with
nobody to deliver to and filed in Sent looking sent.
- The References header was split on whitespace alone, so a client's
non-conformant "<a@x>,<b@y>" became one token and the bracket strip produced
the fabricated id "a@x>,<b@y".
- Reply and forward prefixes were recognised in English only, doubling every
AW:, SV:, WG: and Re[2]: a mixed-locale mailbox receives. Single-letter
spellings are deliberately excluded: with R: recognised, "R: report on Q3"
reads as a prefix and a genuine first reply threads nowhere.
The mailbox-only guard in parseAddressHeader survived its first mutation
check, because removing it still yields no recipients: the invalid GObject
cast makes GMime's own assertion return NULL. That is undefined behaviour
papered over by an assertion G_DISABLE_CHECKS compiles out, so the test now
asserts on the emitted critical rather than on the count. Registering the log
handler on a NULL domain catches nothing; the criticals carry "GLib-GObject"
and "gmime".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LoaLBowZ6w1JNx6SEhDP1L
|
|
MessageSender runs the configured command with the message on stdin and
judges the result by its exit status alone. Nothing here waits on the
event loop, so a send does not block the GUI thread; a 1.6MB payload was
probed through a reading stub without deadlocking the pipe buffer.
The command is split and passed to QProcess as a program and an argument
list, never through a shell. A test asserts that by giving the command
shell metacharacters and checking that the marker file a shell would have
created does not exist, so the property fails a mutation rather than
resting on a comment.
Four corrections to the plan's draft. splitCommand handles double quotes
only, so a single-quoted argument splits wrongly and the header now says
so. A crashing command delivers finished(11, CrashExit) and would have
been reported as "exited with status 11", so a crash branch was added. A
command that exits without draining a large stdin emits WriteError before
finished(), which the draft handled correctly and by luck, untested. And
an empty send_command is checked after trimming.
Two contract gaps found in review, both about what this class promises
rather than what it does.
The exactly-once guarantee covers the EMIT, not what a caller receives: a
long-lived sender plus a connect() inside each send accumulates
receivers, and the second result then runs the first send's lambda too,
filing a sent copy of the wrong message. The header now scopes the
promise and requires Qt::SingleShotConnection. The plan's Task 11 call
site already had that flag, sixty-nine lines below the connect and
outside anything a reader would see, so the plan gained a note where
someone retyping it will read it.
And destruction mid-send killed the command with no report, announced
only by a Qt warning: a live SMTP conversation abandoned, possibly
partially delivered, while the user believes it was cancelled. The
destructor now closes stdin, waits a bounded five seconds, and only then
kills. It emits nothing either way, because the outcome after a kill is
genuinely unknown and reporting "not sent" for a message that may have
gone out is the mailsync.sh mistake pointing the other way. Claiming
m_reported before kill() is what makes that true, since kill() delivers
finished(CrashExit), which would otherwise emit exactly that untruth.
No timeout on the send itself: killing a slow but working send is worse
than waiting. Task 10 owns the popup, and deliberately offers no cancel
after commit, so this class promises none either.
Also refreshes the translations Task 5 left out. That gap was invisible
because test_translations builds its rows from the .ts file, so a string
that never entered it is never asserted on.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QP2g3b3kuLx6AYFCNEz6UR
|
|
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
|
|
Task 4's code review measured it: a large attachment is read and base64
encoded on the calling thread, and autosave calls build() from a GUI-thread
timer. The directory hang that review found is fixed, but the blocking read
is by design and will be felt in the composer.
Recorded against Task 11 rather than fixed, because nothing in the composer
crosses the worker boundary and adding a second threading model for one call
is worse than the stall.
|
|
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
|
|
Task 2's code review found that the README's sample config documents every
other key, including recently added ones, and has nothing for send_command
or the [compose] section. Without it those keys ship undiscoverable: a user
has no way to learn that sending exists at all.
That is a gap in the plan rather than a deviation by the task, since no task
claimed the README, so it becomes a step in the close-out where the rest of
the documentation is written.
|
|
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
|
|
Thirteen tasks, ninety-nine steps, against the spec committed earlier on this
branch. Written on master so it is readable from either branch; the
implementation goes on compose-and-send.
Every API assumption was verified against this machine rather than written
from memory, which found five things the spec had wrong or unstated:
libcmark-gfm-extensions ships NO pkg-config file although libcmark-gfm does,
so CMake needs find_library beside pkg_check_modules. All three enabled
extensions live in that second library, so finding only the first yields a
build that compiles and silently renders plain CommonMark.
GMime defaults to iso-8859-1, emits no Date or Message-ID unless asked, and
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. All three fail only on accented text, which for this
user is every message. The plan builds the content stream directly and
carries a working probe's output as evidence.
MessageNode has no body or date field, so quoting takes a ParsedMessage.
ThreadListModel::messageScopeFor() takes a QModelIndexList rather than a
single index. There is no Config::maildirPath(): the mail root comes from
notmuch_config_get(NOTMUCH_CONFIG_MAIL_ROOT) via a file-static helper in the
worker, and item 124 records that composing a destination from the wrong
root would write into the Xapian tree.
Two spec statements are corrected in the plan rather than followed. It calls
for a new top-level Message menu and one already exists at
mainwindow.cpp:1156. And it requires a shortcut per action, which item 132
changed while this was being planned, so save_message ships without one.
|
|
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.
|
|
The seven brainstorm commits are documentation of decisions already taken:
the spec at docs/superpowers/specs/2026-08-20-compose-and-send-design.md,
the rewritten item 123 pointing at it, and the seven backlog items the
brainstorm opened (128 to 134). No implementation of 123 exists yet, so
nothing premature reaches master.
They are merged now because item 134 shipped to master as af902e0 while the
row defining it sat on the branch, which left master carrying code for an
item its own backlog had no record of. Item 132 is the same shape and is
next.
|
|
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.
|
|
Three rows in every state, so nothing reflows: status label, bar,
right-aligned Undo. The bar changes mode rather than place, determinate
and draining during the countdown because that has measurable progress,
indeterminate once send_command starts because a send does not.
Undo stays visible after it disables. A control that vanishes re-lays
out the popup mid-operation, and a greyed one says why cancelling is no
longer possible where an absent one looks like it was never offered.
The status label sizes from the longest string it can hold in the
current language rather than from its content: Italian "Rimozione della
bozza..." is longer than "Removing draft...", so a content-sized label
resizes the popup between stages, which is the jumping the fixed layout
exists to prevent.
Item 134 gains a requirement from this: the extracted widget must expose
both bar modes, not only the indeterminate one MainWindow happens to
need today.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
The user asked for Gmail's undo-send, and it answers a question the
spec had left open: what Cancel means during a send.
It means nothing, if offered while send_command is running. Killing an
SMTP client mid-transaction leaves an unknown send, since the message
may have reached the server in full before the kill, and that is worse
than either clean outcome. Moving the cancel window before the command
starts makes Undo mean genuinely nothing happened.
The popup owns the whole operation, countdown through completion,
rather than a countdown popup handing over to a status bar. One widget
changing state in one place, and it keeps the eye-catching surface the
user asked for. Modal to the composer only, so a second composer and
the main window stay usable. No close button and no Escape: during the
countdown a dismissal cannot say whether it means cancel or send now.
send_delay_ms defaults to 5000, and zero skips it.
The test for this asserts a negative: Undo leaves the stub command never
run. A test asserting only that the composer reopened would pass against
a design that ran the command and discarded the result, which is exactly
what the delay exists to prevent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
Two corrections from the user, both of which the spec had wrong.
A failed sent-copy write was put in the main window's status bar, on
the reasoning that the composer closes so the message needs somewhere
persistent. Wrong instinct: the fix for "the window is gone" is a
dialog, not a quieter surface. It is the one failure here that produces
a silent divergence between what the recipient received and what the
local archive holds, and nobody discovers that from a line that showed
for a few seconds. It gets a modal.
A failed autosave stays in the composer but as a persistent banner
rather than a status-area line, since the quit path already escalates
that state to a dialog and depends on it surviving.
Stated as a rule at the head of the section, because the user's point
was general: modal for silent divergence, banner for mid-task, status
bar only for what is already obvious.
Second correction: the composer's busy indicator is not built inline. A
second instance of MainWindow's progress-bar-plus-label pairing is
where a widget class earns itself, and "this codebase builds small UI
inline" describes what the code does rather than justifying repeating
it. Item 134 extracts it and converts MainWindow.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
The spec said "disabled with a spinner" without saying where, which
leaves a popup as a reasonable reading of it. A popup is wrong here: it
would be modal over a window that is already disabled, and it can be
dismissed while the operation continues, which is the indicator
ambiguity items 18, 19, 28 and 54 each closed once.
Progress goes in the composer's own status bar, through the three
stages the operation actually has, since a failure filing the sent copy
means something different from a failure sending. The window closing is
the success message.
The indicator is an indeterminate QProgressBar built inline, matching
MainWindow's m_syncProgress rather than factoring out a shared widget:
this codebase builds small UI inline, and two progress bars do not
justify a third class.
Also settles what the staged display implies for a sent-copy write that
fails after a successful send: the composer still closes, because
holding it open for a message already sent invites sending it twice,
and the warning goes to the main window's status bar instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
The spec said "the editor is plain text" and left it there, which reads
as "you are on your own with the syntax". Storage format and editing
affordances are separate decisions and only the first was stated.
The toolbar is text transformation over the markdown source, not
rich-text editing: bold, italic, code, strikethrough, link and quote,
selection-aware, with the cursor landing between the tokens when there
is no selection.
Its shortcuts belong to the composer window's own scope and do not
touch KeyMap, which matters for item 132: the two namespaces should not
be conflated when that rule is revisited.
Live syntax highlighting is a follow-up (item 133) rather than part of
this: agreeing with the grammar about nesting and about code spans is
the expensive half, and it is better judged after living with the
toolbar.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
The spec called it a new dependency needing a SlackBuild REQUIRES entry.
It is a new dependency, but /var/log/packages/ shows
cmark-gfm-0.29.0.gfm.13-x86_64-3 with no _danix tag, so it is stock and
REQUIRES lists only non-stock dependencies.
Also records the staleness cost accepted with it: cmark-gfm tracks an
older CommonMark base (0.29 era) than the stock plain cmark (0.31.2).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
Brainstormed with the user. Design only, no code, which is what the
item's #plan-only tag asked for.
The decision that shaped everything: there is no MTA on the machine, so
"an external script on the same model as mailsync.sh" had no model to
copy. Send becomes a per-account send_command taking the message on
stdin, exactly as [sync] command already works, which keeps the
no-network-protocol rule intact without naming an MTA.
An account with no send_command is receive-only by construction, which
is how one of the five accounts is meant to work. Reply, reply-all and
forward are disabled on its mail behind a ribbon that says why.
The body is markdown parsed by cmark-gfm rather than a hand-written
parser for a limited set: the two share no code, so the small one is
deleted wholesale the moment the set widens.
Four new units, three of them widget-free and testable without a
painter. MessageSender is deliberately a separate unit rather than a
method on the composer, so a future outbox wraps the funnel instead of
reworking it.
Item 123's section is replaced by a pointer to the spec, per this
document's own rule for a fully specified item. The brainstorm opened
items 128 to 132, including a review of the every-action-has-a-shortcut
rule, which the user raised: six more actions takes it past the point
where a chord for everything is useful.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
The entry said sending should be an external script on the same model as
mailsync.sh. Measured on the machine, there is no such model to copy: the
fetch side has mbsync and the send side has nothing.
No MTA is installed at all, msmtp and sendmail are both absent, and neomutt
sends over its own built-in SMTP configured per account in
~/.config/neomutt/accounts/*.rc. So the working setup this application
mirrors has no external send path either.
That makes the first question a non-UX one, ahead of everything the note
lists: sending needs either an MTA the user chooses to install and
configure, which is the only shape that keeps the no-network-protocol rule
intact, or a decision to relax that rule. Recorded so the brainstorm does
not assume the tidier answer, since installing and configuring an MTA is
work he has not asked for and the credentials already exist elsewhere.
Also notes that all five accounts already configure a drafts folder, so the
draft half has somewhere to live before anything is decided.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
Not a regression and not something we removed: Save image is item 114,
still open. Item 127 removed Save LINK and deliberately left this one.
The circumstances the user reported sharpen it twice. The image had already
had its remote content loaded, so m_allowRemote was still true at the click.
That flag is live on the shared interceptor and cleared by the next
showThread(), so a download handler is subject to whatever it says at the
moment of the click rather than at render time. A naive handler therefore
looks perfect in exactly this case and fails once the grant is gone, which
makes "it worked when I tried it" worthless as evidence. The entry records
that both cases must be tested against a message whose grant has been
cleared.
The second is a corollary of item 127. downloadRequested is per-profile, so
connecting it lights up every download entry Chromium offers at once,
including the Save link just removed from the menu. An entry being absent
from a menu is not the same as the capability being absent, so the handler
must decide per request rather than merely exist.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
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
|
|
The first diagnosis was wrong and the user's own follow-up disproved it: a
plain-text GitHub mail opens its links correctly while an HTML newsletter
does not. If RequestInterceptor blocking https were the cause, neither
would work.
Verified against the two messages named. The difference is target="_blank".
An anchor with no target navigates the main frame and reaches
acceptNavigationRequest, which hands it to QDesktopServices::openUrl; that
path works today. An anchor asking for a new window is routed by Chromium
to QWebEnginePage::createWindow(), which MessagePage does not override, so
the base implementation returns nullptr and the click is discarded before
any existing code observes it. Marketing HTML uses _blank almost
universally, which is what makes it read as "HTML mail is broken".
Both messages render HTML, so this was never a text-versus-HTML
distinction: the GitHub mail is multipart/alternative and its HTML part is
what the pane shows.
The entry also drops the proposal to let main-frame navigations through the
interceptor. That would have weakened the remote-content protection to fix
something it was not causing. Nothing here needs m_allowRemote relaxed: the
URL goes to an external browser and the pane fetches nothing.
Records the trap that decides the fix's shape: createWindow() receives no
URL, only a WebWindowType, so an override returning nullptr discards the
target before it can be read.
Item 127 is updated to match. OpenLinkInNewTab and OpenLinkInNewWindow fail
through the same missing createWindow(), so fixing 126 may make them start
working, which is worse rather than better.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
Two defects found by hand, related but separate.
126: clicking a link in a message does nothing. The handler is already
there and correct, calling QDesktopServices::openUrl from
acceptNavigationRequest, and it has presumably never run. RequestInterceptor
denies http and https whenever m_allowRemote is false, which is the default
for every message, and it runs on the request before the page is asked
whether to accept the navigation. The click is dropped at the network layer
with no error, no navigation and no browser. That is the remote-content
protection working as designed; the bug is that a deliberate click is
indistinguishable from a resource the document fetched itself, at the layer
where the decision is currently made.
127: a link's context menu still offers Open in new tab, Open in new
window, Save link and Copy link. Item 100 removed the page-level actions
and its list names four of them; the link actions are different WebAction
values that Chromium adds only over a link, so item 100 never saw them.
Two are dead (there are no tabs, and a second view is deliberately never
created), one belongs with item 114's missing download handler, and Copy
link works and is currently the user's whole workaround for 126. It
follows 126, since a working click makes one "Open link" entry the right
answer rather than a removal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
|
|
Item 124 shipped and is proven: the index moved from a 7200rpm platter to
NVMe with the mail staying at /data/Mail. Cold start went from 38618 ms to
668 ms for a complete walk, and 2008 ms to 50 ms for the first rows.
Counts held at 49174 messages / 5594 inbox / 100 tags at every step, and
Delete then Restore round-tripped through the account's trash by hand.
Item 121 stays open, and its entry now says why. The measured platter
figures are the evidence FOR building the indicator, not against it: a
mechanical disk is the cheap configuration, not an exotic one, and a user
with a large Maildir on spinning rust has nowhere to migrate to. Fixing
one developer's hardware is not fixing the application. The constraint
that pointed at item 124 as the answer is replaced by one saying the
opposite, and prefaulting stays rejected on its own merits since it is
worst on the low-memory machines most likely to have a slow disk.
Item 125 is new, found by hand during the migration. mailsync.sh exits 75
(EX_TEMPFAIL) when another run holds the lock, and the sync indicator
never clears; because an edit made during a sync is held until the sync
ends, a Delete sat queued for a completion that could not arrive and
looked like it had done nothing. Nothing was lost, since held edits reach
the disk, but the user cannot tell that.
Also records in CLAUDE.md that notmuch_database_get_path() is not the mail
root, that database.hook_dir defaults into the index directory and
silently stops post-new under a split config, and that the ordinary
fixture layout cannot tell the two accessors apart.
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
|
|
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Item 115's closed section, which the previous commit failed to update: its
python edit asserted on wording that did not match the file and aborted, so the
code shipped and the record did not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
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>
|
|
Two entries in the notes had no item here. Both causes verified in the code
rather than copied from the note.
Item 119, the unsynced-changes count being clickable, is bigger than it reads.
pendingEditCount() sums four sources and one of them, m_unnettablePendingEdits,
is a bare int by design: it counts confirmed changes carrying no message ids, so
a dialog built from what is currently kept can list three groups and then owes
the user a remainder it cannot describe.
Item 120 goes to the deferred table, matching where the user filed it. It is not
plannable as it stands: nothing records which rule tagged a message, so the
information does not exist to display, and creating it means the post-new hook
storing something per message, which is a shared-format change across both
repos.
Everything else in the notes maps to an existing item.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Delete moves mail into the account's trash folder instead of only tagging it,
with a Trash filter, Restore from trash, and a repeatable cleanup for the mail
the old behaviour stranded.
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 step of the plan is done. Item 103's section moves to the closed file on
the same commit, per this repo's own rule, with its outcome recorded: what was
built, the ten defects hand testing found that the suite did not, and the two
process gaps closed alongside them.
The fact worth carrying forward is the one that damaged real mail. Under
mbsync's Create Both, a wrongly named origin folder propagates to the mail
server, so any code composing a folder name reaches the server whether it means
to or not.
Item 118, emptying the trash, remains deferred at the user's request.
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>
|
