| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
An edit made while a sync is running is held rather than sent, because the
worker's read-write open blocks on notmuch's exclusive lock. At sync end
onExternalSyncStateChanged() refreshed the list first and flushed the held
edits afterwards, so the refresh read a database that still carried the old
tag, reconciled it into the model, and overwrote the optimistic update the hold
had deliberately left applied. The flush then wrote the tag correctly.
The database ended up right and the list ended up wrong, with nothing scheduled
to re-read it, which is why it looked like the edit had been lost. Reported by
hand: a message read during a sync went back to unread when the sync finished.
The flush moves ahead of the refresh and keeps both properties it already had.
It stays outside the Idle branch, so edits held when /proc/locks becomes
unreadable are not stranded waiting for an Idle that never comes, and it stays
after the status-bar retire, so its own "N held changes sent" message survives.
Both orders leave identical end state, so the first version of the test passed
against the defect: after the handler returns the queue is empty and the write
has been sent whichever ran first. flushGenerationForTesting() stamps the query
generation at flush time, which is what separates them, and the test fails
against the old order with Actual: 3, Expected: 2.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
runAutoSync() returned without rescheduling when a sync was already in flight.
The comment defending it argued the edits were not lost, because they reached
the mail store at edit time and the running sync was "very likely" to carry
them. Very likely is not always: an edit made after mbsync has already passed
that account's mailbox is not carried by it, the timer had fired, nothing
re-armed it, and the pending count sat non-zero until a manual sync or the next
cron run.
Skipping is unchanged and still required by item 71: the cron job holds the same
lock and mbsync fails on a second concurrent run. What changes is that the skip
schedules another attempt. scheduleAutoSync() re-checks the delay, the sync
command and the pending count on the way in, so this cannot arm a sync for
nothing, and against a long external sync it re-arms once per debounce interval,
which is a timer rather than a sync.
The test fires the timer by hand and asserts it is active again afterwards, at
the configured interval rather than a shorter one, with the pending indicator
still showing. It fails against the old skip path.
Item 89's other half is dropped rather than built. The list churn it described
is a tag-defined view working as intended: a thread that loses `unread` leaves
the Unread view, and the user resolved it by living in the Inbox view instead.
Three designs were drafted before asking and none is worth building.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Running a query blanks the message pane but left m_currentThreadId,
m_currentMessageId and m_currentMessageThreadId naming the thread that had
been showing. Both selection handlers compare a newly selected row against
those to decide whether it is already on display, so a result containing that
same thread was recognised as "already showing" and onThreadSelected() was
never called. The card painted as selected, the status bar reported one
thread, and the pane stayed on the placeholder.
This is why it looked like an `id:` query defect. The id is copied out of the
details dialog of the message being read, so that thread is current at the
moment the query replaces the view. Any query returning a different thread
hides the fault entirely.
Filed as the unverified half of item 66 and assumed to be the same
empty-MessageIdRole failure. It is not: 66's fix was correct and this
reproduced against it, so it is recorded as item 96. Four hypotheses were
eliminated by measurement first: the row does carry the message id, the
account-scoped query does return it, MimeParser parses the reported message
(ok, 40701 bytes of HTML), and both real ids resolve bare and quoted.
The regression test's first query must open the SAME thread the second one
returns; with two different threads it passes against the defect, which is how
the first version of it was green. Reverting the fix fails it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The four filter buttons gave no sign of which one you were looking at, so
the row said what you could do and never where you were.
The active filter is drawn as a checked QToolButton, which lets the style
paint its own pressed look: a hand-picked highlight colour would have to be
picked once per theme and would still be wrong under a third.
The check state is derived from the query TEXT rather than from the last
button clicked, which is the whole design decision. A record of what was
pressed goes on lying the moment the query is edited into something else,
where a highlight that follows the query clears itself and lights again
when a filter's query is typed by hand. It is resolved against the account
box, so changing account recomputes it rather than dropping it: the same
filter under two accounts is two different query strings and both are
still "Inbox".
Buttons are held in a hash keyed by generator, cleared at the top of the
row build because the row is rebuilt wholesale on every saved-query edit
and stale entries would dangle. The connections are owned by the row
widget, so a rebuild takes them with it rather than leaving a second copy
firing at deleted buttons.
Unread opens already highlighted, which is correct rather than incidental:
startup_query defaults to it, so the window opens on that view. The test
asserts it, so the assertions that follow are known to be a change of
state rather than a button that happened to start unchecked.
Mutation checked against the design that was rejected: deriving the state
from the click instead of the query fails all three tests, each naming the
behaviour it protects.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Answers "start me in work - Inbox rather than All accounts - Inbox". The key
names an account by its [account.<key>] suffix and the dropdown is set to it
before the startup query runs; because a built-in filter composes with the
dropdown, that is the whole mechanism and the key never reaches a query
builder. Validated on load: a name matching no account is reported and cleared,
since the dropdown has no entry for it and would sit on All accounts without
saying why.
Which side applies the scope depends on what the startup entry is, and getting
it wrong is silent in both directions. A generated filter comes back from
resolvedQuery() already scoped, so letting runQuery() apply the dropdown again
gives path:"work/**" and (path:"work/**" and (tag:inbox)). A saved query does
not, because resolvedQuery() ignores the account key for one, so claiming it
was already scoped leaves it unscoped with the dropdown pointing at Work.
The first of those shipped in this session's working tree and passed its test,
because the assertion used contains() and the double-scoped string contains the
scope too. It asserts the exact query now. The second was found by writing the
test for the case rather than by reading, and is covered by
aStartupAccountAlsoScopesASavedStartupQuery.
The README's startup_query documentation was wrong on two counts after the
previous commit: the fallback is the Unread filter rather than the first query
in the file, and the name can now match a built-in filter.
|
|
Two defects, both reachable only after item 93.
startupSavedQuery() searched the saved queries alone, so a startup_query of
"Inbox" matched nothing once Inbox shipped as a built-in filter and the
duplicated entry was removed from queries.json. It then fell back to
m_savedQueries.first(), which is an arbitrary choice that used to look
reasonable while every install carried an Inbox entry: with the duplicates gone
it opened on a leftover search for one sender, and an empty queries.json opened
on nothing at all. The search now covers the saved queries first, so the user's
own entry wins a name collision, then the built-in filters; the fallback is the
Unread filter, which is always present.
The default startup name has always been "Unread" and now resolves for the
first time: before this it named nothing unless the user happened to have such
an entry.
The constructor also read startup.query directly, and a generated entry stores
no query at all, so even a matching filter opened an empty bar. It resolves
through Config::resolvedQuery() now, unscoped, since the account dropdown
starts on "All accounts".
Icons per the user's choices: a star for Important rather than the flag
action's own icon, since on the query row an icon reads as a category rather
than as an instruction, and mail-folder-sent rather than mail-sent.
Three tests changed rather than adapted, because their premises were the
defect. Two asserted the first-saved-query fallback.
aCronSyncDoesNotRefreshBeforeAnyQueryHasRun assumed a fresh window had run no
query, which is no longer true; it is now
aCronSyncRefreshesTheLastRunQueryNotTheQueryBar and asserts the property that
actually matters on a cron timer, through a new lastRunQueryForTesting() seam,
since a legitimate refresh bumps the generation and the counter cannot tell the
two apart.
|
|
Item 57 renamed the `flag` action to "Important" in 0.14.0, chosen over
"Starred" partly because &I was free where &S collided with Mark spam. Item 93
then shipped the filter for the same tag as "Flagged", so one window offered
both names for one thing. The generator keeps its own name, `flagged`: that
string is stored in queries.json and matched against a closed set, so it is
wire format rather than a label.
The filters are QToolButtons now, like the Save button at the other end of the
row, carrying a themed icon with the text beside it. Icon AND text for the
reason the Save button already records: this row is a row of text buttons, so
an icon alone reads as a different kind of control than it is. Theme icons
rather than the shipped SVGs in Marks, because item 70's split is that the
panes are ours and the chrome is the system's, and the query row is chrome.
mail-mark-important matches the `flag` action's own icon, since the filter
finds what the action marks.
The icon test asserts a NAME was requested rather than that the icon resolved:
QIcon::fromTheme returns null where no icon theme is installed, so isNull()
would fail for a reason unrelated to this code. Dropping the setIcon call fails
it.
Widening the buttons to QToolButton broke eleven tests that reached them
through findChild<QPushButton *>, which does not match a sibling type. The
helpers and the filter lookups take QAbstractButton; savedQueryButton() stays
on QPushButton, since the user's own queries really are those.
|
|
An unpinned query could not be run. Its menu action carried both a triggered
connection and a submenu of edit actions, and Qt does not emit triggered for an
action that owns a menu: clicking the entry only opened the submenu, so the
connection had never fired.
It shipped unnoticed because the menu was the rarely-used half while the user's
queries were pinned buttons. Item 93 moved every query into the menu, which is
how it surfaced, and item 94 makes the menu their only home, so this is now the
path that has to work.
Running is an item inside the submenu, first and above a separator, with the
edit actions below it. The entry keeps its submenu because an unpinned query
must still be editable and deletable.
The test asserts the Run item exists and is first, then that triggering it
reaches the query, then that Edit and Delete survived beside it. Restoring the
old wiring fails it on the first of those, naming the Qt behaviour rather than
just reporting a wrong query string.
|
|
Item 93, the UI half. Unread, Inbox, Flagged and Sent are buttons the
application ships, sitting first on the row, ahead of the user's pinned saved
queries.
runFilter() is runSavedQuery()'s opposite in the one way that matters: it READS
the account box and never writes it. That is item 90's defect. A filter narrows
what the user is already looking at, so the dropdown is its input rather than
something it resets on the way past. A saved query keeps setting the account
from what it stored, because it is a destination and states its own scope.
runQuery() gains an AccountScope parameter. A filter's text arrives already
resolved in the selected account's scope, and scoping it again would put
path:"work/Sent/**" inside path:"work/**".
Two migration changes, both of which unpin rather than delete:
- Sent is no longer migrated from the INI into queries.json. The built-in
filter covers it, and migrating one too would put two Sent buttons on the
row, one editable and one not.
- A stored entry naming a known generator is unpinned on load, which is what
every install upgraded through 0.19.0 carries. It keeps its name and its
generator and moves to the menu. Deleting it would be data loss on a file
whose readers are supposed to preserve what they do not own.
The test suite needed the same distinction the design makes.
savedQueryButtonLabels() now skips the filters, and savedQueryButton(window,
label) replaces five positional row->findChild<QPushButton *>() lookups that
were silently returning Unread.
One rendering probe had to be fixed rather than adapted.
replyRowsKeepTheirTextUnderTheThreadLine resized the window to 300px, and four
more buttons pushed the reply row below the viewport: the pixel loop then ran
zero times and reported "0 pixels, the row was painted over", which is a
different defect from the one it exists to catch. It gets 600px and a guard
asserting the row is really inside the viewport, so the next person to shrink
it gets told the truth. Verified by putting 300 back: the guard names the row
at 83..165 in an 82px viewport.
|
|
Item 74. runQuery() set the status bar once and only queryFinished cleared
it, so the bar kept claiming a query was running for the whole walk while
rows were visibly arriving behind it. Measured cold against a 1.1 GB index:
the first batch reaches the model at 642 ms and the walk finishes at 5714 ms,
so five seconds of a slow query read as a frozen one.
onThreadsReady now sets the bar from the model's own row count after each
batch, which is the number of rows the user can actually see. No timing
changes; this only stops the bar from lying.
The refresh branch returns before the new line, so a background refresh stays
silent exactly as onQueryFinished already keeps it. That silence has its own
test, which fails when the write is moved above the guard.
beginRefreshForTesting() is a new seam: refreshCurrentQuery() returns early
without a worker and a bare window has none, so a test cannot otherwise reach
the refresh path.
|
|
Reverts bde7409 and 66f1159. The user hit the worst possible symptom:
clicking one message marked a DIFFERENT, unrelated message read.
The cause is in markCurrentThreadRead, which reads
m_model->threadAt(current.row()). CLAUDE.md records this exact trap: a
tree numbers rows PER PARENT, so a reply's row() indexes its siblings
and threadAt() on it answers about an unrelated thread near the top of
the list. The guards then compared the right ids against the wrong
thread and let a write through for whatever message the timer's state
named.
That fault predates these commits, but they made it reachable and
harmful: while the write was thread-scoped the mismatch was mostly
masked, and scoping it to a single message turned it into "a random
message is now read".
Reverting rather than fixing forward. Marking the wrong mail read syncs
out to the server and cannot be undone from here, so the safe state is
the previous behaviour, which is too broad but predictable. The item 66
work in 4a4f82f stands: a thread root still renders one message.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Follows the message-scoped mark-read. The user reported the write going
out with nothing visible changing: the status bar counted an unsynced
edit while the card stayed bold and the message pane still showed the
`unread` tag, until the next query corrected it.
sendMessageTagChange made no optimistic model update on purpose, because
applyTagChange is keyed by THREAD and repainting a whole row for a
one-message edit would claim every reply had changed too. That trade is
right for an explicit tag edit and wrong for auto mark-read, where the
visible change IS the feature and the delay exists to deliver it.
ThreadListModel::applyMessageTagChange updates the message wherever it
is held, as a child row and as `first`, and lets the thread's summary
follow only when the answer is unambiguous: a thread reads as unread
while ANY message does, so the tag is cleared from the thread only when
no other message still carries it. For an unexpanded multi-message
thread the per-message tags are not loaded, so the summary is left for
the next query rather than guessed at.
Mutation checked: without the call the card holds `unread` for the full
timeout.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Reported by the user: selecting an unexpanded thread root marked every
message in the thread read, replies included. maildir.synchronize_flags
is on, so removing `unread` rewrites Maildir filenames and the next sync
carries it to the server: mail the user never saw stops being unread
everywhere.
This was coherent while a root click rendered the whole conversation,
because everything marked read had been displayed. Removing that view
made a root render one message and left the thread-wide write in place,
so the defect arrived with the previous commit.
markCurrentThreadRead now sends m_currentMessageId, which is what the
pane rendered, through sendMessageTagChange. The thread-level `unread`
guard is dropped with it: a thread carries `unread` while ANY message in
it is unread, so it would pass a read root under unread replies and send
a write for a message already read. Scheduling still checks it, which
keeps a fully-read thread from arming a timer.
The test asserts on which worker entry point the window used, because
reading tags back cannot answer this. Three earlier versions passed
against the unfixed code: TagsRole is empty for a message row by design,
MessageOwnTagsRole subtracts thread tags and drops marks so it can never
hold `unread`, and raw node tags are not refreshed until onTagsApplied
confirms, which lands after the assertion. Mutation checked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Selecting a thread root used to render the whole conversation, stubs
plus the last messages expanded, but only until the thread had been
expanded once. After that the identical click rendered a single message.
The user reported the inconsistency and asked for the single-message
behaviour throughout, and for the conversation view to go.
The cause was a timing one, not a race. The root card stands for the
thread's first message and onThreadSelected already preferred to load
just that, but the model learned the id only when the replies arrived,
so a fresh row fell through to a whole-thread render.
ThreadSummary now carries firstMessageId from the query itself, so the
id is known before any expansion and the fallback is unreachable. It is
free: notmuch_thread_get_toplevel_messages reads the index, not the
message files, and a walk with it is indistinguishable from one without
over a 36,615-thread database. Contrast recipients, which reads every
file and stays Sent-only.
The Sent view keeps showing what the user sent rather than the thread's
opening message, which is often someone else's. There is no
matched-messages iterator in libnotmuch, only a count, so that branch
walks oldest-first to the first NOTMUCH_MESSAGE_FLAG_MATCH and stops:
0.146s against a 0.143s baseline over 4,515 threads.
onThreadLoaded merges into renderMessages, since onMessageLoaded was
already delegating to it for the actual painting. It still takes a list
because MessageView renders a list; collapsing that is a separate change
to a class with its own tests.
NotmuchWorker::loadThread is kept and documented as having no UI caller.
It is a tested way to read a thread's messages with the match set
resolved, used as a helper by the worker's own tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
showWarnings() did two separable things and one of them could not be
reached from a test. It set the status label, which is harmless, and it
raised a QMessageBox from the MainWindow constructor, which under the
offscreen platform nothing can dismiss: the constructor never returned
and the suite hung with no output, reading as an infrastructure failure
rather than a test one.
It splits in two. applyWarnings() keeps the status label and stays in
the constructor. configProblems() returns the list, and main.cpp raises
the dialog after show(), which also gives it a visible parent to sit on.
The distinction between warnings and problems is preserved exactly: a
keybinding being ignored interrupts startup, "no sync command
configured" does not.
The warning path now has its first test, using the config shape that
caused the original hang. Mutation checked by putting the modal back in
the constructor: the test times out at 124 rather than failing, which is
the behaviour this removes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The menus cannot read the query bar and must not. MainWindow already
watched textChanged for the Save button; the same lambda now also tells
MessageView, which passes it to the details dialog at construction,
where it cannot go stale.
Nothing consumes it yet.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Four signatures, no behaviour change: the two shipped operations map to
Replace and Narrow. runSearchFromPane becomes a switch and gains the
Exclude arm, which nothing can reach until the menu entry exists.
Seven call sites across three test files moved with it, two more than
the plan predicted: test_messageview and test_mainwindow also drive
these signals directly.
mainwindow.h and messagedetailsdialog.h now include searchterm.h for
the type; messageview.h already did.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The panes carry a finished query and know nothing of the query bar; the window
sets the field and calls the existing runner, so the account scope and the
generation counter keep working as they do for a typed query.
Narrowing combines here rather than in a pane, because only the window can see
what the bar currently holds. The tag strip's chips join the header, the body
selection and the details dialog as a fourth surface.
Also fixes the details dialog to actually close when a search is chosen: the
comment above the connection already described this requirement, but nothing
called accept() or reject(), so the dialog stayed open, the query ran behind
it, and the modal exec() never returned. This hung the whole test suite on
QT_QPA_PLATFORM=offscreen once a covering test was added.
|
|
Right-click a stored saved query and the rules dialog opens on a new
rule carrying its query, with the tags left empty and focused. Generated
entries are excluded: their query is composed from the accounts, so a
rule made from one would freeze a snapshot that goes stale.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The dialog is non-modal and single-instance, so a second Create tagging
rule reaches one that is already up. Seeding it beats dropping the
request, which would read as a broken menu item.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Item 82. Saving a query worked and nothing else did: changing one field meant
retyping the whole query under the same name, and deleting one meant editing
the file by hand. An action that creates something the UI cannot then change or
remove is incomplete, and the user hit it within minutes of the first hand test.
Right-clicking a saved query, on its button or its menu entry, now offers Edit,
Move to menu / Show as a button, and Delete. Every path funnels through one
replaceSavedQuery(), which matches on the name the dialog was OPENED with rather
than the one it returns, so a rename replaces the entry instead of leaving the
original behind beside a new one, and which merges the stored entry's unknown
fields in a single place rather than in three. Delete confirms first: the rule
against confirmation dialogs covers tag mutations, which the undo stack can take
back, and this writes user config that it cannot.
Two cases the item did not anticipate. A generated entry has no query to edit,
so the dialog shows its composed query read-only rather than offering a field
that changes nothing, and carries `generated` and `flat` through an edit rather
than letting it decay into a plain entry holding a snapshot of what it resolved
to today. And the overwrite notice had to learn to ignore the entry being
edited, since warning that "Inbox" already exists while editing Inbox is noise.
This also fixes a defect that predated it and was already reachable from the
save path. rebuildSavedQueryRow() called deleteLater() on the old row, which
defers destruction to the event loop, so the stale row went on answering
findChild() and every lookup after a rebuild reported the state from before the
edit. Nothing looked wrong on screen, which is why it surfaced only as three
tests failing against a row that had in fact been rebuilt correctly.
Five tests, three mutations. Matching on the returned name fails two, never
writing the file fails three, and dropping the unknown-field merge fails one.
That last one initially proved nothing: it drove UNPIN, which copies the stored
entry and so carries `unknown` along by itself, and passed with the merge
deleted. It now goes through the edit path with a replacement that has none,
which is what the dialog actually returns.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The user asked whether the default queries could be unified with Sent. The
answer runs the other way: Sent joins the saved queries rather than the saved
queries becoming hardcoded.
Inbox, Unread and Important are complete strings that depend on nothing and can
never go stale, so generating them would buy nothing and would cost the four
things the file just gained: reordering, unpinning, renaming and deleting.
Hardcoding them would also make them undeletable, which is a regression for
anyone who does not want one of them.
Sent is different only in that its query CANNOT be stored: it is composed from
every account's `sent` key, so a stored copy goes stale the moment a folder is
renamed. That is a property of Sent, not of "default queries". Storing the
GENERATOR rather than its output keeps both halves: `"generated": "sent"` still
resolves from the accounts at click time, and the entry is an ordinary row that
can be reordered, renamed, unpinned or removed. The row now follows one rule
instead of carrying one member the user did not own.
Two properties had to travel with the entry. The composed query, resolved
through Config::resolvedQuery() so what lands in the bar is what actually ran;
and FLAT mode, since a sent view lists messages and a threaded one folds every
reply back into the conversation the user sent one message into. The sent
generator implies flat rather than trusting the file to say so, because a
hand-edited row would otherwise produce a threaded sent view.
An unknown generator is reported but the row is KEPT: a later build may know it,
and dropping it here would delete it from the file on the next save, which is
the same data loss the unknown-field handling exists to prevent. A generator
whose accounts configure nothing is skipped entirely, exactly as the hardcoded
button was hidden rather than offering one that finds nothing.
Eight new tests. The four pre-existing Sent tests reach this through migration
and were left alone, which is what proves the migrated path still behaves; the
new ones cover a STORED file, which is the path every launch after the first
takes. Mutations: a generator resolving to nothing fails three, ignoring flat
fails two, and not skipping an empty generator fails one. A rename test guards
the property the change exists for, since anything keyed on the literal name
"Sent" would break it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
document-save is the floppy/disk shape. It means "write a file somewhere",
which leaves the user to guess what is being written, and next to a row of
saved-query buttons it reads as an unrelated control. Saving a query is
bookmarking a search, so bookmark-new is the icon every desktop already uses
for "keep this for later". Verified to resolve with real art in the desktop's
actual theme rather than assumed present.
The button also shows its label now instead of the icon alone. It sits among
text buttons, and an icon on its own next to them reads as a different kind of
control; it is also the one action whose meaning an icon cannot carry, since
"save" is a familiar shape whose question is always "save what?". The toolbar
is unaffected and still follows the desktop's own button style.
The label is the button's own text rather than the action's. "&Save query..."
is menu phrasing, and setDefaultAction copies it verbatim, so the button
rendered an accelerator ampersand and the ellipsis that promises a dialog. The
action keeps both for the menu it lives in, and the test asserts the override
survives setDefaultAction rather than trusting that it does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
nothing is pinned
The saved-query buttons are the row's content and read as a set; the overflow
menu is a control over that set, so it belongs apart from them rather than
trailing the last button. Moving the stretch above it pushes it to the right
edge.
Doing that exposed a latent defect in the same function. The row hid itself when
its layout held nothing but the stretch, which was written as a count of one and
happened to be right only because the stretch went last. With the stretch moved
the count changes, and the condition as written would have hidden a row holding
only the menu: a config with saved queries but none pinned would have had no
route to any of them, the menu buried along with the row. The check now counts
the content added before the stretch and treats an unpinned query as content in
its own right.
Both are mutation-checked. Putting the stretch back at the end fails the
alignment test, and restoring the old hide condition fails the new one, which
asserts the row survives with nothing but unpinned queries in it. The alignment
is asserted on the layout's own ordering rather than on x coordinates, since a
geometry assertion would also pass for a row that merely ran out of width.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The spec asked for "a Save query button beside the search bar" and what shipped
was a menu entry and Ctrl+S. The user went looking for the button where the
design said it would be and did not find it.
Saving is a thing you decide on while looking at the results, so it belongs
where the results came from rather than behind a menu or a remembered chord.
The button takes the action through setDefaultAction rather than a second
connect, so it inherits the text, icon, tooltip and enabled state and cannot end
up offering to save an empty query while the menu entry correctly refuses. The
mutation that replaces it with a plain clicked() connection fails the test.
Also records item 82: a saved query cannot be edited, unpinned or deleted from
the UI. Item 23 specified saving and nothing else, and that is exactly what was
built, so the only way to unpin a query is a text editor or retyping it in full
under the same name. An action that creates something the UI cannot then change
or remove is incomplete, and this was found within minutes of the first hand
test. It is filed as a defect rather than an enhancement, and the spec now says
so where a reader would otherwise take the design for complete.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Second half of item 23, on top of the storage change. A query can now be kept
without hand-editing a file, and the row of buttons no longer grows without
bound.
Ctrl+S opens a dialog on whatever is in the query bar, taking a name, an
optional account scope and whether the query is pinned. It preselects the
account already chosen in the dropdown, since that is the scope the user is
looking at, and it says so when a name is about to replace an existing query
rather than refusing the name: overwriting a saved query on purpose is a normal
edit, and the only thing worth preventing is doing it without noticing. Saving
over an entry keeps the stored entry's unknown fields rather than the dialog's
fresh value, so a field written by a later build survives being edited here.
The saved queries move to a row of their own beneath the query bar, pinned ones
as buttons and the rest behind a More queries menu that only exists when
something is in it. The ponytail note that stood in the query row predicted
exactly this: an unbounded list of buttons sharing the row squeezed the field.
Sent moves down with them and is still not a saved query, for the reason already
recorded there.
A saved query's account scope goes through the account DROPDOWN rather than
being baked into the query text. runQuery() already wraps the query in the
selected account's path, so pre-scoping here would apply it twice, and setting
the dropdown also shows the user which scope they are in. An unscoped query
clears the selection rather than inheriting whatever the last one left, which is
the same defect the rules preview had.
Seven tests, three mutations. Ignoring the pinned flag fails two of them,
pre-scoping the text instead of setting the dropdown fails two, and letting an
unscoped query inherit the previous account fails one. The menu-absence test
initially passed against no implementation at all, since it only asserted a
widget was missing; it now proves the row was populated first, which is the
guard that class of test needs.
Two existing invariants caught real omissions rather than needing adjustment:
every registered action must appear in KeyMap::knownActions(), which is what
gives it a configurable binding, and every action needs its own icon.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Item 77. The dialog could say how many messages a rule matched and not
which ones. A Preview in list button now runs the selected rule's query
in the main window; the dialog stays open, since comparing the rule
against its results is the point.
Two constraints from the backlog entry, both now asserted and both
mutation-checked.
The query runs exactly as stored, with no tag:new and no wrapping
parentheses. The post-new hook adds those when it applies a rule, and a
preview that copied them would match nothing outside a sync window,
since tag:new is set only on mail that has just arrived.
The account selector is cleared first. runQuery() wraps the bar's text
in the selected account's scope, and a rule query usually names its own
path already, so previewing one with an account selected would scope it
twice and show an empty list, which reads as "this rule collects no
mail".
The second mutation only fails once the test's config has an account to
select: with the default empty config the selector sits on "All
accounts" anyway, and asserting that a preview leaves it there passed
against the mutation. Recorded in the test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The dropdown was built from config, which names one subtree per account
and nothing below it, so it offered five entries and no way to say
Drafts or Sent. A rule wants to target those as often as a whole
account.
NotmuchWorker gains requestFolders/foldersReady, walking the tree from
notmuch_database_get_path() and listing every directory holding cur/.
It belongs there because the database root is notmuch's database.path
and the worker owns the only handle that can answer for it; putting the
root in config would be the second source of truth the design refuses.
From the disk rather than from the index: a folder mbsync created and
nothing has landed in yet is still a folder a rule may target, and a
list derived from indexed message paths would not offer it.
The two tests build their own fixture rather than extending the shared
one, which needs a nested folder and would otherwise move seven count
assertions in unrelated tests. Mutation-checked: flattening the walk to
non-recursive fails the listing test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
|
|
Counts are generation-stamped and dropped when stale or when the dialog
has closed: counting every rule against a cold index takes seconds, so
an in-flight reply outliving its dialog is ordinary rather than rare.
The stamp is its own counter, not m_generation as drafted. That one is
the QUERY generation, compared against directly by every thread, tree
and message load, so bumping it to count rules would discard whatever
the user was opening at the time and blank the message pane for an
unrelated reason.
Registering an action obliges two more entries, both enforced by tests:
the name in KeyMap::knownActions(), and a default binding, since every
action carries one. Ctrl+Shift+T, shifted against Ctrl+T for edit_tags
the way Ctrl+Shift+U is shifted against Ctrl+U.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Items 70 and 69, the second folded into the first as item 70's own size note
predicted it should be.
The panes drew their state marks as font glyphs: U+1F4CE for an attachment and
U+2605 for a flagged thread, each with a fallback for a font that cannot render
it. Both fell back to "*", so on such a font a flagged thread and one carrying
an attachment were indistinguishable, which is a defect the fallback introduced
rather than prevented. What a mark looks like was also the desktop's decision
rather than this application's, and the panes are exactly where it should not
be: the user asked for the toolbar and menus to keep following their icon theme
while the panes stop.
Six marks now ship in assets/icons/marks/: flagged, attachment, passed, replied
and the two expander triangles. QIcon::fromTheme still resolves every toolbar
and menu icon and was not touched.
Licensing chose the shapes. The look came from a GPL3 icon theme, and this
project is GPLv2-only, which are incompatible: GPLv2's "no further
restrictions" clause bars shipping GPL3 assets in a v2-only work. The six were
drawn fresh in the same idiom instead, with no path data copied. The idiom is
generic: solid single-path silhouettes at 16x16 with no strokes.
They are compiled in as string literals rather than loaded from a .qrc.
src/CMakeLists.txt already records why resources belong to the executable: a
qrc in the static library registers itself from a global initialiser the linker
drops. The tests link the library, so a resource-based mark would be missing
exactly where it needs asserting. assets/icons/marks/ stays the editable
source.
One asset serves both palettes. Every payload paints with fill="currentColor",
which QSvgRenderer renders black rather than resolving, so Marks::pixmap
composites the wanted colour with CompositionMode_SourceIn. A mark then takes
the card's own pen colour and follows selection and the read/unread dimming
without a second variant to keep in step.
CardLayout reserves a rect per mark and CardDelegate paints into it. The marks
were glyphs inside the subject STRING, so their width came free from the text
metrics; as icons the geometry has to know they are there or the subject runs
underneath them. The expander pill had the same trap, its triangle being a
glyph in expanderLabel(), and now reserves that width explicitly.
Item 69's part: passed and replied were words in the tag strip and are marks
beside the subject now. The message pane's header carries the flagged and
attachment marks next to the subject, per the user's decision that the right
pane needs those two and only outside the message area.
A duplicate that no test caught is worth recording. Every geometry assertion
passed while a card showed passed as BOTH an arrow and a green tag chip: the
chip filter had no reason to know a mark had appeared. It was found by
rendering real cards to an image and looking at them. isDrawnAsAMark() is now
one list consulted by both PillTagsRole and MessageOwnTagsRole, since two
copies drifting apart is how a tag ends up drawn twice on one row and not at
all on another.
Fourteen tests: nine in test_marks, four in test_cardlayout, one in
test_threadlistmodel. Mutation-checked at four points, each failing a test: the
subject ignoring the marks, the flag not indenting the subject, the pill
forgetting the triangle's width, and the recolour composite removed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Item 71. A tag edit reached the notmuch index at edit time and then sat there
until the user clicked Sync or their cron job fired, so "mark all read" updated
the view while the change itself waited, sometimes for ten minutes.
A confirmed edit now arms a debounce that runs the existing sync path. The delay
is auto_sync_delay_ms in [general], defaulting to 2000, and follows
mark_read_delay_ms exactly, including that zero and negative are not errors:
zero syncs on the next trip through the event loop, and any negative value
disables the behaviour, which is the switch for a user who wants only their cron
job.
It is armed from onTagsApplied, where a write is confirmed and the pending count
is already current, rather than where one is sent: a sync scheduled for a write
the worker went on to reject would run for nothing. A debounce rather than a
schedule, restarted by each edit, because "mark all read" confirms one write per
thread in the view and an arm-per-edit timer would be the storm of syncs the
debounce exists to prevent. Nothing is armed when no sync command is configured
or when the pending count is zero, the case where an edit was netted against its
own inverse. When the timer fires with a sync already running, local or cron, it
skips rather than queues: mbsync's own answer to a second run is to fail on it,
and the edits stay pending rather than being lost.
Also fixes a pane blanked out from under the reader, found by hand testing this
feature. onSyncFinished called runCurrentQuery() where the cron path calls
refreshCurrentQuery(), and a re-run clears the model, the undo stack and the
message pane. The stale-thread notice handles a thread that stops matching the
query and has since item 35, but a re-run left nothing for it to describe. The
two paths had no reason to differ; before this item a local sync only followed a
click on Sync, so the difference went unnoticed. Reading a message in the Unread
view, having it marked read, and watching the pane go blank two seconds later is
what surfaced it.
Its test asserts on the undo stack rather than the pane: both paths issue a
queued query test_mainwindow has no worker to answer, so the pane ends up blank
either way and an assertion on it would pass against both, while the undo stack
is cleared by one and kept by the other.
Nine tests, four in test_config and five in test_mainwindow, each
mutation-checked: removing the schedule call, honouring a negative delay,
dropping the nothing-pending guard, dropping the already-running guard, and
restoring runCurrentQuery() each fail a test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Item 67. The pane counted unread, flagged and inbox from three fixed
tag: queries. Sent and drafts cannot join that list as tags: tag:draft
counts 0 against a real database and no draft-ish tag exists in it at
all, so a tag-based line would be a permanent zero that reads as working
code. Both are composed from each account's folder keys instead, the
same way the Sent view already composes its query.
The drafts key was parsed and documented as unused in v1. Composing
drafts is still v2; counting them is not, so Account::draftsQuery() and
Config::allDraftsQuery() now mirror the sent pair. The shared body moved
into folderQuery() and joinAccountQueries(), so the load-bearing quoting
(a provider nests both folders under a bracketed parent, and [ and ] are
Xapian syntax) and the bare-"or" guard exist once rather than once per
folder type.
The fixed array is gone rather than extended. It held queries and labels
in two lists indexed in parallel, which is a hazard that grows with the
list: an entry inserted in one and not the other prints a real number
against the wrong name and looks entirely plausible. placeholderLines()
carries each query beside the callable that labels it, so the two cannot
drift, and the count reply stays paired by position as the worker
requires.
A line is omitted when no account configures that folder rather than
shown as 0, following item 63: a missing folder is a real configuration,
and "0 sent" claims the user has sent nothing.
Measured against the real config: 4 sent terms over 601 threads, 5
drafts terms over 3, the extra drafts term coming from the one account
that configures drafts and no sent, which is what proves the two are
collected independently.
Four tests here and four in test_config, mutation-checked at three
points: dropping the drafts line, an off-by-one in the label pairing,
and removing the -1 guard for an uncountable query. Each mutation fails
a test.
|
|
Adds a `sent` key to [account.*] naming that account's sent folder, and a
Sent button beside the saved queries that composes its query from every
account carrying one. An account without the key is omitted silently, as a
real account may keep no sent mail locally. With no account selected the
button spans all of them; selecting one narrows it through the existing
scope wrap rather than a second path.
Composed at run time rather than shipped as a [queries] entry. A saved query
is one fixed string: it cannot narrow to the selected account, and it goes
stale the moment an account is added or a provider renames a folder.
The design and the measurements behind it are in
docs/superpowers/specs/2026-08-11-sent-mail-design.md. Three things there are
worth repeating here.
The composed path is QUOTED, and that is load-bearing. A real provider nests
its sent folder under a bracketed parent, and "[" and "]" are Xapian syntax:
unquoted, the query parses rather than matches and returns nothing while
looking entirely plausible. Composition happens in one place so there is one
chance to get it right, and a bracketed path is pinned in a test.
Recipients are opt-in per query, which is a performance contract rather than
a preference. notmuch_message_get_header(m, "To") is not served from the
index, it reads the message file: folding every thread of a 4411-thread
inbox took 38.2 seconds against 251 ms for the 601-thread sent view. The
worker skips the walk entirely unless asked, and the refresh path carries the
same flag so a background sync cannot blank the column mid-read. Always
folding is mutation-tested: the data would be right and only the cost wrong,
which nothing else here would notice.
The messages reached through the thread are owned by it and freed with it, so
recipientsOf() holds them raw and finishes while the thread is alive, exactly
as walkReplies does. An NmMessage wrapper there is a double-free.
Sent mail is presented flat, and the pane follows. A message you sent
otherwise drags in the replies you received, so a view labelled Sent shows
conversations rather than what you sent. ThreadListModel::setFlatMode() makes
hasChildren() and ReplyCountRole answer differently and changes nothing else;
runQuery() sets it on EVERY run, so any other query restores the tree on its
way through and the flag cannot outlive the button that set it. The pane
needed its own fix for the same reason: the single-message path depends on a
field only filled when a thread is expanded, which never happens in a flat
list, so loadThread() gained matchedOnly and drops the messages that did not
match instead of rendering them as stubs.
Recipients replace the sender through the existing SendersRole rather than a
new one, so the delegate needs no branch and cannot disagree with the model
about which name a row shows. It falls back to the sender when a To header is
absent or unparseable, since a blank where a name belongs reads as a
rendering fault.
Address parsing uses GMime: a display name may contain a comma, so
"Rossi, Mario" <m@example.org>, info@example.net is two addresses and
splitting reports three. internet_address_list_parse returns NULL for an
empty string, which is a crash if unguarded.
Backlog item 63.
|
|
Adds [general] date_format, a QDateTime pattern for the date a thread card
shows. Absent or empty means the system locale's short format, which is what
every other application on the desktop uses and stays the default.
The format reaches the LAYOUT, not only the painter. CardLayout::compute()
reserves the date's width from widestDateSample(), so a pattern that arrived
only at the drawText call would be elided into a rect sized for the old
format, which is the clipping the bold-font fault already produced once. It
rides on CardLayout::Input and defaults to an empty string, leaving every
existing call site unchanged. Confirmed by mutation: making the width ignore
the format fails the test.
widestDateSample() memoised its result in a static, which would have sized
every format after the first from whichever arrived first. It is a plain call
now, costing one QLocale lookup per row, the same as formatting the date.
Validation rejects only a pattern whose output is CONSTANT, found by
formatting two different instants and comparing. QDateTime::toString() treats
nearly every letter as a field, so "banana" formats as "bpmnpmnpm" and
"hello" as "22ello": nonsense, but they vary with the instant, and a check
claiming to find "no date field" cannot reject them. What harms the user is
the pattern that prints the same text on every card, and that is what is
refused, with the value named in the message.
The model supplies the pattern through DateFormatRole for the same reason it
supplies the tag colours: it is the one object here holding config, and a
delegate reading config itself would be a second source of truth.
Backlog item 62.
|
|
The Sync button used mail-receive, a mailbox glyph, which reads as "mail"
rather than "fetch again". The toolbar follows the desktop's tool button
style, so on an icon-only desktop the icon is the whole control and has to
carry the meaning by itself.
view-refresh is the standard freedesktop name for the action. The existing
noTwoActionsShareAnIcon test covers the collision risk that the 0.12.0
Archive/Mark-all-read defect came from, and passes.
Also records the backlog reconciliation this came from: items 64 and 65,
appended from the user's notes with their causes verified in code. 65 is
"full code review and optimization", which names no symptom or measurement
and is filed unspecified rather than given a design.
Backlog item 64.
|
|
The thread list now updates itself when a sync finishes, whether it is
empty or populated. New threads appear where the sort puts them, threads
that stopped matching leave, and threads whose state changed repaint.
Refreshing used to mean re-running the query, which cleared the model,
the selection, the message pane and the undo stack, so 0.8.0 declined to
do it on a cron timer and asked the user to press Enter instead. The
result was a list that quietly disagreed with the database: mail indexed
by cron never appeared, and an Unread view read to the end sat empty in
front of it.
ThreadListModel::reconcile() diffs a result against the current rows by
thread id instead, so a surviving thread keeps its row, its persistent
index and its loaded replies. Order comes from the result and is never
imposed here, which is what makes the sort dropdown authoritative.
The undo constraint this was sized around did not exist: no undo entry
was ever keyed on a row. ThreadTagCommand stores thread ids and
MessageTagCommand stores message ids, and applyTagChange() looks its
target up by id, so an entry already survived its rows leaving the view.
A thread read out of the current view now leaves the list, which is
correct and would otherwise strand the reader, so MessageView grows a
notice saying the open thread no longer matches, with a button that
re-queries it. Recovery lists the whole conversation, expands it, and
restores the message that was on screen rather than reopening at the
first one.
Ten defects were found building this, nine of them by hand testing:
- SyncMonitor::start() polls synchronously, so an idle lock file emits
stateChanged(Idle) from inside buildUi() and the first handler to
touch a widget segfaults before the window exists.
- QTreeView sets a current index when it takes focus with none set, and
current drives loading, so new mail opened itself and was marked read
without the user having looked at it. Selection is now required.
- The notice outlived what it described, both when the pane was blanked
and when another message replaced it.
- Retiring the "Background sync completed" message left the bar claiming
a sync was still running: silent means saying nothing new, not leaving
a stale claim on screen.
- A thread root sets both the thread id and the message id, so treating
the message id as the message-row case discarded it for the commonest
way to open a thread.
- A freshly queried root does not know its own first message until the
tree loads, so recovery selected nothing and left the pane blank.
- A user query mid-recovery had its result hijacked by the pending
selection.
- MessageView emitted the recovery signal with its own members, so a
direct connection handed MainWindow references that runCurrentQuery()
then cleared by blanking the pane. The ids went empty mid-slot and no
recovery ever ran. Every test passed against this, because reaching a
slot through invokeMethod copies its arguments.
A Qt signal argument is a reference until something copies it. Emitting
a member to a slot that can re-enter the emitter is a use-after-write,
and it presents as a wrong value rather than as a crash.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Four faults from the first hand test, two of them behavioural.
An expander that opened onto nothing. setThreadMessages kept only nodes with
depth > 0, and notmuch_thread_get_toplevel_messages returns every message at
depth 0 when a thread carries no usable In-Reply-To, so a flat thread
contributed no children while its card still advertised the count. Measured in
the user's database: of 396 inbox threads three are flat, one of them nine
messages long, and every two-message thread of that kind was affected, which is
exactly why the fault looked like "the expander only works with more than one
reply". The rule is now position, not depth: every message except the first,
which is the root card itself. That is also the correct rule rather than a
workaround, since the row under the root is the second message however notmuch
chose to nest it.
The thread's first message was unreachable. Selecting a root card loaded the
whole thread, so the pane showed every message with only the last expanded, and
no row in the list offered the first one: the reply rows are messages two
onward. The root card now renders its own message, which is what the card
already claims to be. It keeps its thread id, unlike the message-row path, so
mark-read and the tag-change repaint still work; that is asserted, because
clearing it is the obvious way to write this and silently disables both. Before
the replies are loaded the model has no first message to name and the whole
thread stays the honest answer.
Dates ignored the locale. One hardcoded "yyyy-MM-dd hh:mm" produced a US-looking
format on an Italian desktop; QLocale::system() now formats it, and the width
reserved for the date comes from the same function so a longer locale cannot
clip.
The expander was a bare number on the card's own background. It is a pill now,
carrying "3 replies" (and "1 reply", singular), sized from the label actually
drawn and measured in both glyph states so it does not resize under the pointer
on click. Its fill is blended from Text toward Base rather than taken from
QPalette::Button, which is #2b2b2b against a Base of #2b2b2b on the user's
theme: byte identical, so the pill was invisible. A theme may make any two roles
equal; a blend is defined against the surface it sits on and cannot collide with
it. Checked by rendering both a dark and a light palette and looking.
|
|
Two entries, straight to notmuch. This adds a feature rather than replacing one:
the column header was decorative and nothing implemented click-to-sort, so
removing the header with the grid lost nothing.
Stored in uistate.conf, never in the hand-edited config, and range-guarded on
read: a stale file can hold anything, which is the lesson item 58 recorded.
SortOrder needed qRegisterMetaType despite carrying Q_ENUM. Q_ENUM gives the
enum a meta-object entry, not a metatype registered under the name invokeMethod
resolves, so the queued runQuery would have dropped its sort argument at runtime
and every query would have silently run newest-first. Nothing in the suite
exercises a real worker thread, so this was asserted directly rather than left
to a warning nobody would see. It is registered beside the type rather than in
MainWindow's constructor: a first attempt put it there and passed only because
the test that catches it never constructs a MainWindow.
The account dropdown's entries now carry their account's colour as a swatch,
which is what makes the accent bar on a card mean anything: a colour down a
card's edge says nothing until something maps it to a name. Raw colour here
rather than the blended line colour, since a swatch is a filled patch like a
chip rather than a thin line. Its test builds its own two-account config: reading
the environment's made it SKIP wherever no accounts are configured, which is a
test that asserts nothing while reporting success.
|
|
next_thread and prev_thread now walk with indexBelow/indexAbove, skipping
message rows, so they keep meaning thread-to-thread whatever is expanded.
Stepping message-to-message needs no code: QTreeView's own Up/Down walk VISIBLE
rows and already enter an expanded thread, and being the view's key handling
rather than a shortcut they stay inert when the message pane, a menu or an entry
bar has focus.
Item 60 turns out to have been fixed already, in 5487d58 on this branch, by
threadRowOf() walking up to the containing thread before doing the arithmetic.
The backlog entry was written against master, where that helper does not exist,
so it described a defect this branch had resolved a commit earlier. Verified by
writing both failing tests first and watching them pass: from the last reply of
an expanded thread, and from a thread root with its replies showing. They are
kept, because the property they assert is the one this change must not lose.
What the rewrite buys is that nothing is keyed on a row number any more, which
is the rule a deeper tree would break next.
Alt+Up/Down added alongside Ctrl+J/K. That required KeyMap::sequencesFor and a
move from setShortcut to setShortcuts, because the singular setter keeps only
the last binding and the second one was silently unreachable. Alt because
Shift+arrows is the built-in extend-selection that multi-row tagging depends on,
and because a bare arrow cannot be a window shortcut without breaking every text
field in the window, as Return already demonstrated. sequencesFor puts
sequenceFor's own choice first so the menus advertise an unchanged binding, and
sorts the tail, since QHash order is unspecified.
|
|
ThreadListView::paintEvent and its band arithmetic are deleted. The view existed
to paint a strip across five columns; with one column and one delegate painting
the whole card there is nothing to span, and the two faults that arithmetic kept
producing go with it: a deleted row cut in half, and every other row showing a
bare stripe.
What survives is the expander hit-test, because a delegate gets no click of its
own without an editor. It now asks CardDelegate for the rect rather than
recomputing it, so the drawn target and the clickable one cannot drift. The
siblingAtColumn(0) dance is gone: with one column, the index already is column 0.
Item 51 closes here rather than separately. A card is exactly viewport width, so
the view has no horizontal scroll range for a click to scroll into, and the test
asserts that directly.
Two rendering tests had to change how they measure, not merely which index they
name. The indent test asserted on visualRect, which now reports the SAME rect
for a thread and its reply by design, since setIndentation(0) leaves the indent
to CardLayout: it reads contentLeft off the layout instead. And the expander
test reported zero ink over a card the delegate paints 2183 pixels into, because
viewport()->render() returned a blank image, exactly as CLAUDE.md warns; it now
paints the delegate into an image directly and carries a guard proving the probe
can see ink before it reports finding none. Both were mutation-checked.
Two tests are deleted rather than ported. Both existed to prove the row-wide
strip spanned columns a delegate could not reach, which is a property of code
that no longer exists.
|
|
tagSelected resolved rows to threads with threadAt(index.row()), which is wrong
for a message row: a child's row number indexes its siblings, so acting on a
reply tagged whichever thread sat at that position in the list. It now routes
through ThreadListModel::scopeFor, and a message row's change is sent as message
ids down applyTags with its own MessageTagCommand for undo.
MessageTagCommand stores message ids where ThreadTagCommand stores thread ids,
and that difference is the point rather than an inconsistency: re-resolving the
thread on undo would restore tags across every sibling the action never touched.
sendMessageTagChange deliberately skips the optimistic model update.
applyTagChange is keyed by thread and would repaint the whole row as though
every message in it had changed, which for a one-message edit is a lie the user
watches correct itself on the next query. It keeps the two things that are NOT
optional: the edited-account set, resolved through the containing thread since
the account is a property of the thread, and holding the edit when a sync holds
notmuch's write lock, since the worker's read-write open blocks rather than
failing.
The scope is now stated before an action and after it, naming both the message
count and whether a whole thread went. This is what stands in for the
confirmation dialog CLAUDE.md rules out: undo is the safety net, and undo is
only usable if the user can tell that something larger than they meant has just
happened. Selecting a single message reports no count at all, since reading one
message is not a bulk action.
A mutation that routed message rows down the thread path SURVIVED the whole
suite: undo depth and status text are identical either way while every sibling
gets tagged. anActionOnAMessageRowTagsThatMessageNotTheThread exists because
that gap was found, and asserts on the ids actually sent.
anActionOnAThreadRowSaysItHitTheWholeThread reads the status bar BEFORE draining
the event loop. This binary has no worker, backlog item 36, so the queued write
reaches a database that has never heard of the thread and answers with
errorOccurred, which overwrites the status bar: draining first asserts on that
error and fails against correct code.
|
|
loadMessage queries by id: and returns one MessageRef, always matched, since the
user asked for that message by clicking its row and a stub would answer the
wrong question. An unknown id emits an empty vector rather than an error: a
stale row after a reindex is an ordinary race, not a failure worth the status
bar. The signal fires even when empty so the UI handler runs instead of waiting
for a reply that never comes.
The branch in onThreadSelected is placed BEFORE threadAt(), which is the whole
trap. threadAt takes a top-level row number and a child's row number indexes its
siblings, so handing a message row's number to it loads whichever thread happens
to sit at that position. Mutation-checked: with the branch disabled the test
reports thread 't1' for a reply belonging to 't2', a wrong answer plausible
enough to survive review.
m_currentMessageId and m_currentThreadId are mutually exclusive and each clears
the other, so a queued reply can tell which kind of selection it belongs to.
onMessageLoaded carries a third guard onThreadLoaded does not need: a reply
landing after the selection moved to a thread row would render one message where
the conversation belongs.
No mark-read timer for a message row in this pass. Marking one message of a
thread read is a per-message tag write and the pending-edit map is keyed by
thread; item 28 is the record of what happens when that count goes wrong.
|
|
Both were reported from the running application after the previous commit
claimed them working, and the tests that passed could not see either fault.
The expander took four attempts, each of which looked right in code:
- QTreeView::drawBranches is the documented hook and does not work here. It
runs BEFORE the row's cells, so with the expander on a content column the
delegate's own background paints over it. A 60-pixel triangle survived as
8, indistinguishable from the theme's near-invisible dot.
- Sizing the glyph from the row rather than the branch rect put most of it
outside that rect.
- Moving it into SubjectDelegate but calling it from only the no-chip branch
left every real row without one, since every real row has an account chip
and takes the other branch.
It is now drawn by the delegate, which owns the cell and paints after the
background, from both branches, with setRootIsDecorated(false) so the style
does not draw its dot underneath.
The indent was 20px and invisible for a reason the geometry could not show: a
thread row draws an account chip before its subject and a reply row does not, so
a reply's text already starts about a chip's width LEFT of its thread's. The
indent has to beat that before any nesting reads at all, hence 72px.
The indent test asserted on visualRect, which was correctly indented the whole
time, and so passed against a build with no visible nesting. It now measures
where the TEXT lands, accounting for the chip, and fails at 20px. The new
expander test counts painted pixels of the glyph colour against a control row
with no replies, and fails when the call is dropped from either branch.
|
|
Replies are fetched on expansion rather than with the query: walking the reply
tree of every thread in a 10k-thread result would cost more than the query and
almost none of it would be looked at.
hasChildren is what makes that lazy loading work, and its absence would have
shipped the feature unreachable. rowCount is 0 until the worker has walked the
thread, so a view left to infer the expander from rowCount alone draws none, the
user can never expand, and the replies are never requested. It answers from the
summary's totalCount before loading and from the children afterwards, so a
thread whose count included duplicates stops offering an expander that opens
onto nothing.
onThreadTreeLoaded reads the thread id from the reply rather than remembering it
from the request. Two expansions can be in flight at once, and pairing them by
order would attach one thread's replies to the other.
|
|
The strip survived the port because every geometry call it needs exists on both
classes. What did not survive is anything keyed on a row NUMBER: a tree numbers
rows per parent, so row 0 exists once per expanded thread and the old flat 0..N
walk would paint the first thread's strip over every one of them. The walk now
goes by index, and the alternating colour follows visual position rather than
index.row() for the same reason.
QTableView::isRowSelected(int) has no QTreeView equivalent; isSelected on the
index replaces it. MainWindow loses verticalHeader and selectRow, so row height
comes from uniformRowHeights and three helpers replace the row arithmetic.
next_thread and prev_thread now resolve the containing thread first: in a tree
current.row() + 1 is the next SIBLING, which under an expanded thread is the
next reply, not the next thread.
Two test defects found by mutation and worth recording, since both produced a
green suite over a broken assertion:
The indent test asserted on column 0. A QTreeView indents only the column
holding the expander, verified against Qt 6.11: with setTreePosition(4), column
0 reports the same left edge for a thread and its reply while column 4 reports
420 against 440. It was failing against a correctly indented tree.
The strip test passed with the view's skip deleted, because the real model
already returns no pills for a child row, so the view's guard was never the
thing under test. It now runs against a stub model that hands pills to every
row, which leaves the view's skip as the only thing that can keep replies clean.
That rewrite then failed for a third reason: without the delegates MainWindow
installs, rows take the default height, the band is measured against
SubjectDelegate::rowHeightFor and overflows into the row below, and the thread's
own strip paints across the reply. Reads exactly like a missing skip and is not
one.
|
|
loadThread could not be extended to do this. It walks
notmuch_query_search_messages, and a message obtained that way returns NULL from
notmuch_message_get_replies (notmuch.h:1617-1628), so that walk cannot produce
reply depth at all. The tree comes from notmuch_thread_get_toplevel_messages
instead, and the pane keeps the flat list it wants.
walkReplies takes raw notmuch_message_t*, against this file's rule that every
handle is RAII-owned. Messages reached through a thread are freed with it
(notmuch.h:1637), so an NmMessage wrapper would destroy memory the thread frees
again. The NmThread in the caller is what keeps them alive.
Every message in the thread gets a node regardless of the query: the list is
where the reply count is read, and hiding unmatched replies would make that
count disagree with the rows under it.
Both tests mutation-checked. Flattening depth fails the depth assertion, and
skipping the thread walk fails it too, so neither passes against the two
mistakes the notmuch API invites.
|
|
A splitter position is saved in pixels, so one saved in a wide window
does not fit a narrower one: QSplitter::restoreState() honours the first
pane's saved size verbatim and gives the second whatever is left. A real
1285/1252 split restored into a 1136px window left the message pane 29px
wide, a sliver of rendered mail beside a full-width thread list. The
wider the window ever was, the worse the next narrower session.
Fixed with a minimum width on the pane and setCollapsible(1, false),
which covers the restore and the equivalent drag. A restore-time repair
running from showEvent() was written first and deleted: with the floor
in place it was mutation-tested to be redundant, since the minimum width
constrains restoreState() as much as it constrains a drag.
The floor is 300px rather than a bare "visible" width, because it is
reached only when a position does not fit and should land somewhere mail
is readable; at 200 the placeholder's own text wraps every few words.
The test asserts against the pane's own minimumWidth() rather than a
repeated literal, with a guard on the floor itself so it cannot pass
against a lowered one.
Note for later work in test_mainwindow: the offscreen platform chooses
the window width itself and has been seen to choose differently between
two runs of the same binary (1181 and 779), and it ignores resize(),
setFixedWidth() and a resize of the splitter on a shown window. Any
assertion on a pane ratio, or on the second pane's pixels, is measuring
that choice rather than this code.
Backlog item 55, whose recorded cause blamed the thread view's size hint
and first-run layout. Measured, that hint is 256px, not the 886 the item
computed from the column widths, and a freshly built window splits
correctly; the entry has been corrected in place.
|
|
Item 59, reported by the user against 0.12.0. Archive and Mark all read
both used `mail-mark-read`, so with the toolbar following a desktop set
to icon-only the two buttons were indistinguishable, despite doing
different things: archive removes `inbox` from the selection, mark all
read removes `unread` from the whole view.
Introduced by item 56 in the same session. `archive` had that name from
before, when only eight actions carried icons, and item 56 assigned it
to `mark_all_read` as well without checking the table for duplicates.
`mail-archive` is also the more accurate name, since `mail-mark-read`
describes read state rather than archiving.
The test asserts the class rather than the reported pair: a hand-written
table of twenty-four names has more plausible duplicates in it, so
noTwoActionsShareAnIcon compares every action against every other. It
compares cacheKey() rather than the theme name, because two different
names resolving to the same art are equally ambiguous on screen, and it
guards on every action having an icon first, since on a theme that
resolves nothing the comparison loop would never run and pass vacuously.
Mutation-checked with a different collision.
Item 56's own probe is what let this through: it verified every name
resolves to non-null art, which is true of two names resolving to the
same art. Resolving and being distinguishable are separate properties.
The replacement was picked by rendering both at 24px and comparing the
images.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Follow-up to item 56. With the toolbar now following the desktop's
button style, an "icon only" desktop makes the icon the whole control,
and this style reports PM_ToolBarIconSize as 16px, which is a small
target for a button with no text beside it.
A [general] toolbar_icon_size key, 16 to 64, defaulting to 24 rather
than to the style's own metric. Setting it to 16 restores the theme's
value.
Clamped and reported, unlike message_zoom, which documents a 0.5 to 3.0
range in the README and enforces none of it. Both ends here break the
UI that would be used to fix them: too small is an invisible icon, too
large is a toolbar taller than the window.
The unenforced message_zoom range is recorded as item 58 rather than
fixed here, since it is a separate defect that predates this change.
Also documents in the README that saved-query button labels are the key
names from the user's own [queries] section, which is why the "Flagged"
button still read that way after the action was renamed: it is a user's
query name, not a string this code owns. The sample config now shows
`Important = tag:flagged` to teach the wording the UI uses.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
