| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
A reply in a thread with no usable In-Reply-To carries depth 0, because that is
how notmuch reports every message of such a thread. CardLayout read depth 0 as
"not nested", so those replies drew flush against their own thread with no
spine, while a nested thread's replies indented normally: the list showed two
different shapes for the same relationship, side by side.
A MESSAGE row is nested at least one level whatever depth it reports. Being a
child row IS the nesting; the depth only says how much further to go.
This is the third fault from the same root. The depth numbering was trusted to
mean structure when it only ever meant "how notmuch happened to thread this":
first it hid a flat thread's replies entirely, then it left the first message
unreachable, and now it drew the survivors without their indent.
|
|
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.
|
|
Both found by rendering real cards to an image and looking at them, not by any
assertion. The suite was green through both.
The date lost the leading digit of its year on every UNREAD card. The layout
reserves the date's width from the font it is handed, which is the view's
regular font, while the delegate paints with the bold one the model supplies for
unread: 154px reserved against 170px needed. CardLayout now measures the date
bold whatever font it is given, so the reserved width cannot be narrower than
what is drawn. A few pixels are wasted on a read card, which is the cheap side of
the trade.
The accent bar was painted correctly and was invisible. Blending the account
colour 0.35 toward the palette's Base, as the plan specified, is a fraction OF
THE ACCOUNT COLOUR, so on a dark theme it produced (0.18, 0.22, 0.26) against a
Base of (0.169, 0.169, 0.169): the background. The blend is dropped entirely.
An account colour is already chosen to be a chip's fill carrying legible text,
so it is muted to begin with, and nothing is drawn on the bar that needs that
contrast. The spine keeps a blend, at 0.55, because it runs the full height of
every reply in an expansion and is a different problem from a 3px edge marker.
The bar is still faint at 3px on a dark theme, since the account colours are
chosen as chip fills. Whether kAccentWidth needs raising cannot be settled
without the user's own accounts, screen and theme; that is Task 10's open
question and it is left open.
|
|
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.
|
|
Replaces SubjectDelegate. The tag chips come home from the view: the strip was
painted there only because a delegate cannot paint outside its column and the
strip spanned all five, and with one column there is nothing to span.
RowStyleDelegate is inherited rather than dropped. Its job survives the
redesign: Qt resolves ForegroundRole into the palette's Text roles and prefers
those over HighlightedText, so the read/unread dimming would win on a selected
row and land as grey on the highlight. What it loses is the rest of its body,
which aligned cells against a text band and centred two marker columns; both
described a grid that no longer exists.
A reply's Re: prefix is stripped here. Every reply repeating the thread's
subject is the visual signature of a table of records, which is the thing item
53 is about.
The account chip becomes a bar down the card's left edge, and the reply spines
inherit its colour, so an expanded thread is bounded by one accent from its root
to its last reply without a second line in the gutter. Neither uses the raw
account colour: that colour is chosen to be a chip's fill with legible text on
top, and the same value as a thin line has to be followable down an expansion
without competing with the senders, so it is blended toward the palette's Base
by the weight threadLineColour() already uses. A reply resolves its THREAD's
colour by walking to the root, since AccountColourRole is empty on a message
row and a neutral spine under an accented root would break the continuous edge.
The build is red at this commit; the view and window still name the old
delegate.
|
|
Five columns answered through Qt::DisplayRole; one column cannot, and a card
needs every field at once, so each gets its own role. Qt::DisplayRole keeps
answering the subject, which is what keyboard search and accessibility read.
Three things change shape rather than moving. DateRole hands over the QDateTime
itself, since the card decides how much of a date it has room for and a
pre-formatted string takes that decision away from the delegate. The subject
loses its "(3)" message-count suffix, which the reply count on line 2 now
states. And the two per-column tooltips become one card-wide tooltip, because
the marks no longer have columns of their own to hover.
The build is red at this commit: the view and the delegates still name the
deleted Column enumerators and are rewritten in the commits that follow.
|
|
Split from the delegate deliberately. A delegate needs a live painter and an
exposed view, which is what makes delegate tests fragile: viewport()->render()
returns a blank image in several ordinary situations, and a probe reporting no
ink is likelier broken than the code it tests. Every geometric claim about a
card is made here, where a test is a function call.
Three lines at a uniform height, so setUniformRowHeights(true) survives. Indent
caps at depth 4 with qMin rather than a branch, so depth 5 and depth 50 land in
the same place. The date is measured before the sender, so a long sender elides
instead of painting over it.
Two traps handled that a first pass gets wrong. QRect::right() is inclusive, so
the right edge is carried as an exclusive one and everything sized from it lands
where the padding constant says rather than a pixel short. And QFont::pointSizeF
returns -1 for a font set in pixels, which qt6ct does, so smallFont branches on
which unit the font actually carries instead of silently returning the card's
own size.
|
|
Sorting was hardcoded NEWEST_FIRST. Two orders only: notmuch's other two are
MESSAGE_ID and UNSORTED, neither of which is an order a human wants, and sorting
by sender or subject would have to happen in the model after results arrive,
which fights the batching that makes a large query paint immediately.
loadThread keeps OLDEST_FIRST unconditionally: a thread reads chronologically
whichever way the list is sorted.
|
|
A reply card shows only these. The alternative, a reply's full tag set, was
rejected on measurement rather than taste: in the user's database 7 of 48691
messages carry unread and 75 carry flagged, both already drawn another way, and
every other tag is applied per thread and identical on all its messages. Full
sets would repeat the thread's chips down the whole expansion, which is the
striping the row-wide strip was built to avoid.
|
|
tagSelected resolved rows to threads with threadAt(index.row()), which is wrong
for a message row: a child's row number indexes its siblings, so acting on a
reply tagged whichever thread sat at that position in the list. It now routes
through ThreadListModel::scopeFor, and a message row's change is sent as message
ids down applyTags with its own MessageTagCommand for undo.
MessageTagCommand stores message ids where ThreadTagCommand stores thread ids,
and that difference is the point rather than an inconsistency: re-resolving the
thread on undo would restore tags across every sibling the action never touched.
sendMessageTagChange deliberately skips the optimistic model update.
applyTagChange is keyed by thread and would repaint the whole row as though
every message in it had changed, which for a one-message edit is a lie the user
watches correct itself on the next query. It keeps the two things that are NOT
optional: the edited-account set, resolved through the containing thread since
the account is a property of the thread, and holding the edit when a sync holds
notmuch's write lock, since the worker's read-write open blocks rather than
failing.
The scope is now stated before an action and after it, naming both the message
count and whether a whole thread went. This is what stands in for the
confirmation dialog CLAUDE.md rules out: undo is the safety net, and undo is
only usable if the user can tell that something larger than they meant has just
happened. Selecting a single message reports no count at all, since reading one
message is not a bulk action.
A mutation that routed message rows down the thread path SURVIVED the whole
suite: undo depth and status text are identical either way while every sibling
gets tagged. anActionOnAMessageRowTagsThatMessageNotTheThread exists because
that gap was found, and asserts on the ids actually sent.
anActionOnAThreadRowSaysItHitTheWholeThread reads the status bar BEFORE draining
the event loop. This binary has no worker, backlog item 36, so the queued write
reaches a database that has never heard of the thread and answers with
errorOccurred, which overwrites the status bar: draining first asserts on that
error and fails against correct code.
|
|
loadMessage queries by id: and returns one MessageRef, always matched, since the
user asked for that message by clicking its row and a stub would answer the
wrong question. An unknown id emits an empty vector rather than an error: a
stale row after a reindex is an ordinary race, not a failure worth the status
bar. The signal fires even when empty so the UI handler runs instead of waiting
for a reply that never comes.
The branch in onThreadSelected is placed BEFORE threadAt(), which is the whole
trap. threadAt takes a top-level row number and a child's row number indexes its
siblings, so handing a message row's number to it loads whichever thread happens
to sit at that position. Mutation-checked: with the branch disabled the test
reports thread 't1' for a reply belonging to 't2', a wrong answer plausible
enough to survive review.
m_currentMessageId and m_currentThreadId are mutually exclusive and each clears
the other, so a queued reply can tell which kind of selection it belongs to.
onMessageLoaded carries a third guard onThreadLoaded does not need: a reply
landing after the selection moved to a thread row would render one message where
the conversation belongs.
No mark-read timer for a message row in this pass. Marking one message of a
thread read is a per-message tag write and the pending-edit map is keyed by
thread; item 28 is the record of what happens when that count goes wrong.
|
|
Indentation alone still read as a table, which was the user's original
complaint about the whole item. Three cues now say the rows belong to the
thread above them: a spine down the left of the expanded block with a stub out
to each reply, a background tint, and text a size down and undimmed only when
unread.
Both colours are mixed from the palette rather than fixed, the same rule
readColour follows: a tint that reads as grouping on a light theme is invisible
or muddy on a dark one. The tint is deliberately near the threshold of noticing,
since it sits beside the deleted and spam fills, which carry real meaning and
must stay the loudest thing in the list.
The spine is accumulated across the visible reply rows and drawn once after the
loop. Drawn per row it left a gap at every row boundary and read as a column of
dashes rather than as the structure holding the block together.
Two bugs fixed here, both mine, both from the previous commit:
Clicking the expander did nothing. setRootIsDecorated(false), needed to stop
the style painting its own indicator under ours, also removed the style's hit
area, so the glyph rendered perfectly and was inert. ThreadListView handles the
press itself now, over the strip the delegate reserves, leaving the rest of the
subject cell to select the row.
Every click then expanded rather than toggling, because isExpanded and
setExpanded are keyed on column 0 and were being asked about the subject-column
index, which always answers false.
Visible, clickable and toggling are three separate properties and a test for
one passes against the other two being broken: the pixel test proved the
triangle was drawn while it could not be clicked, and the first click test
proved it opened while it could never close. The test now clicks twice and
asserts open then closed.
replyRowsKeepTheirTextUnderTheThreadLine covers the other trap. paintEvent runs
AFTER the cells, so the first version of the tint filled the whole reply row and
erased the sender and subject the delegate had just drawn: zero surviving text
pixels, a block of blank tinted rows. The fill and the stub stay in the band
below the text, where the tag strip lives on thread rows.
|
|
Both were reported from the running application after the previous commit
claimed them working, and the tests that passed could not see either fault.
The expander took four attempts, each of which looked right in code:
- QTreeView::drawBranches is the documented hook and does not work here. It
runs BEFORE the row's cells, so with the expander on a content column the
delegate's own background paints over it. A 60-pixel triangle survived as
8, indistinguishable from the theme's near-invisible dot.
- Sizing the glyph from the row rather than the branch rect put most of it
outside that rect.
- Moving it into SubjectDelegate but calling it from only the no-chip branch
left every real row without one, since every real row has an account chip
and takes the other branch.
It is now drawn by the delegate, which owns the cell and paints after the
background, from both branches, with setRootIsDecorated(false) so the style
does not draw its dot underneath.
The indent was 20px and invisible for a reason the geometry could not show: a
thread row draws an account chip before its subject and a reply row does not, so
a reply's text already starts about a chip's width LEFT of its thread's. The
indent has to beat that before any nesting reads at all, hence 72px.
The indent test asserted on visualRect, which was correctly indented the whole
time, and so passed against a build with no visible nesting. It now measures
where the TEXT lands, accounting for the chip, and fails at 20px. The new
expander test counts painted pixels of the glyph colour against a control row
with no replies, and fails when the call is dropped from either branch.
|
|
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.
|
|
ActionScope is what an action is about to touch, resolved from the selection in
one place so no call site reinvents the mapping. A thread root contributes the
whole thread, a message row contributes one message, and messageCount is what
the status bar reports.
The count comes from totalCount, not from the loaded children. A thread that was
never expanded still has all of its messages, and counting only the rows that
happen to be on screen would understate what the action does: mutation-checked,
and the wrong version reports 1 message where 7 are about to be tagged.
A mixed selection is honoured as given rather than escalated to thread scope or
narrowed to message scope. Silently widening it would defeat the reason the
scope is shown at all.
|
|
setThreadMessages drops the depth-0 message: it is the thread's first message
and the root row already stands for it. Keeping it would show a thread of seven
as one root and seven children, contradicting the reply count the row
advertises. Calling again replaces rather than appends, so a thread reloaded
after a sync does not list its replies twice.
A message row reports its own sender and subject, not the thread's. That is the
mistake worth guarding: the thread's author summary usually contains the first
sender too, so reading it renders something plausible for the root's own reply
and wrong for every other one. Mutation-checked, and the wrong version returns
'Alice' where 'Bob' belongs.
Child rows carry no tag pills. The strip is a row-wide band of the thread's
tags; one under each reply would stripe the list and repeat identical tags down
the expansion.
|
|
A table cannot indent or expand, so message rows need a tree. This task changes
only the base class and the index plumbing: no children are produced yet, so the
30 pre-existing tests in test_threadlistmodel are the regression net proving a
thread row still behaves exactly as it did, and QAbstractItemModelTester checks
the index/parent round trip a hand-written assertion would miss.
Two things the table version could leave wrong and a tree cannot. columnCount
returned 0 for a valid parent, which would give message rows no columns and
render them blank. And rowCount now answers only for column 0, since a tree
takes one set of children per row and offering them under every column draws an
expander in each.
The model stays two levels deep even though replies carry a reply depth of their
own. The visual nesting past the first level comes from that depth, not from
further parent-child structure, so no index calculation has to recurse.
|
|
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 message row has to be drawn without opening the message, so it needs sender,
subject and date. MessageRef carries none of them: it exists for rendering a
thread into the pane and holds only id, path, tags and matched.
depth defaults to 0, the thread's first message, which the root row stands for
rather than a child row. threadId is carried so a batch of nodes names the
thread it belongs to without the caller tracking it alongside.
|
|
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.
|
|
The documented 0.5 to 3.0 range was already enforced, by
MessageView::clampZoom(), so message_zoom = 500 rendered at 3.0 rather
than unusably. What was missing is the report: the key parses, so
nothing ever told the user that the value in their file is not the value
on screen.
Reported rather than clamped a second time. MessageView owns the bounds
and does the work; a copy of the range in Config would be free to drift
from the one that matters, so config.cpp reports against kMinZoom and
kMaxZoom directly. This is where it differs from toolbar_icon_size,
which has no widget-side enforcement to defer to.
Backlog item 58, whose recorded cause was wrong on this point and 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>
|
|
Items 56 and 57, done together because both touch the action registry.
56. The complaint was inconsistency, not absence: eight of twenty-four
actions had themed icons, so two adjacent entries in one menu disagreed
and the toolbar laid out an empty slot for the other sixteen. The
themeIcons table now covers them all. The fifteen names added were
probed against a live icon theme first rather than taken from the
freedesktop spec on faith, and the existing null-icon guard still lets a
theme that lacks one fall back to text.
The second half of the note asked that buttons honour the desktop's
"Icon only" setting. They could not: the hardcoded setToolButtonStyle
overrode it whatever the user had chosen. It now reads
SH_ToolButtonStyle. Dropping the call entirely was tried and rejected,
since a bare QToolBar defaults to ToolButtonIconOnly rather than to the
platform hint, which ignores the setting just as thoroughly the other
way. This is a visible change: on a desktop set to "Icon only" the
toolbar now shows icons without text.
57. "Important" over "Starred", the user's pick; the Message menu
already has "Mark &spam", so "Starred" would have needed an accelerator
from inside the word. Changed the action text, its status tip, the undo
description and the star column's tooltip, which still read "Flagged".
The tag stays `flagged`. It is wire format that neomutt, the user's
saved queries and ThreadSummary::isFlagged() all read, and following the
label through to the tag would rewrite the mail store and desynchronise
every other tool over the same Maildir. The action name stays `flag`
too, since that is the key users write in [keys].
Four tests. everyActionCarriesAnIcon names every action missing one and
guards against passing on an empty list; it reported all sixteen before
the change. theImportantActionStillWritesTheFlaggedTag asserts on the
tag the model actually received, and mutating it to `important` fails
that test plus two pre-existing held-edit tests.
Also adds the changelog entry for the cron-sync indicator fix, which the
commit that made it omitted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Item 54. A sync fired by the user's cron carries tag edits to the mail
store exactly as a local one does, but only onSyncFinished() cleared the
pending state, so the indicator kept reporting work that had already
shipped and the exit prompt asked to sync for it. Verified against a
real cron run: 31 changes, cleared with no manual sync.
The window cannot see an external run's exit status, and /proc/locks
carries no outcome. It does not need to: mailsync.sh already ends every
run with a "RUN END ... status=OK" banner in its log, which outlives the
process that wrote it. MailSync::lastRunOutcome() reads a bounded tail
of that file and takes the last completed marker, so no change to the
script and no optimistic guessing were needed.
Only a definite OK clears anything. A failed run, a missing or
unreadable log, and a State::Unknown lock reading all leave the count
alone: over-reporting costs a redundant sync, under-reporting costs the
user their edits.
m_editedAccounts is drained in the same place, before flushHeldEdits()
and matching the local path's ordering. Item 49 uses it to choose which
mbsync channels a run syncs, and a count that reached zero while the set
stayed full would look correct and still sync the wrong channels.
The log path comes from a new optional [sync] log key, defaulting to
where the script writes, so a test never reads the developer's own log.
Two notes on the verification, both recorded in the backlog:
- A timing probe endorsed a tail read that was not happening. The
first version of the huge-log test required the call under 100 ms
and passed with the seek deleted, because reading 10 MB is fast
either way. Replaced with an assertion on content.
- Every fixture was invented and the first batch had the wrong
timestamp format, since the script uses date -Iseconds. The tests
passed anyway, because the parser keys on the prefix and the status
token. One test now builds the banner the way the script does.
The before-flushHeldEdits ordering has no test: without a held lock the
flush is a no-op, so both orderings pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
A sync ran mbsync -a regardless of what changed, so tagging mail in one
account fetched all of them. The account set was not a parameter anywhere
on the path: MailSync::start() took no arguments and the script hardcoded
-a, so nothing between a tag edit and mbsync carried which account changed.
Track which accounts have edits and pass their mbsync channels through to
the script, which now takes channel names and falls back to -a when given
none. An empty set means all accounts, per the request: a sync with nothing
pending is a fetch, and narrowing that to wherever the last edit landed
would quietly stop collecting mail everywhere else.
The channel is a new optional per-account key rather than the section key.
The two names genuinely diverge, because a QSettings section key may carry
dots that the channel does not, and mbsync treats an unknown channel as
fatal rather than skipping it, so key-as-channel would fail those accounts'
syncs outright rather than degrade. It defaults to the key, so accounts
whose two names already agree need no config change.
The edited-account set is deliberately not netted the way the pending-edit
map is: that map tracks the index, where a tag removed and re-added leaves
nothing outstanding, while this tracks the mail store, where both writes
have already renamed files that mbsync still has to propagate. It is also
snapshotted before flushHeldEdits(), which inserts into it synchronously
rather than on a queued reply, so a successful sync cannot clear accounts
whose edits it never carried.
Closes item 49.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Nothing in the UI reported database-level facts: every query gave a
thread count for that query, and nothing said how much mail there is
overall. A dialog under Help now shows messages, threads and tags from
notmuch, plus the account list from config, since notmuch does not model
accounts at all.
A separate worker call rather than a reuse of requestCounts, which counts
threads to match the row count of a query. This counts messages, which is
what a user means by "how much mail is in here". The test pins 4 messages
in 3 threads against the fixture and fails if they are ever made equal,
so routing both through one count cannot pass unnoticed.
Every field starts at -1 and renders as "unknown" when notmuch could not
answer it. Printing 0 would say the Maildir is empty, and telling someone
their mail is gone is the worst way to report an index that failed to
open.
The dialog opens showing "Counting..." rather than blocking, since
counting every message is not free on a large database. That makes two
lifetimes matter: the reply can arrive after the dialog is closed, so the
label is a QPointer, and the dialog can be closed and reopened while a
count runs, so a generation counter drops the older answer. The test
drains DeferredDelete before firing the late reply, because close()
deletes through deleteLater and without that the dangling case is never
actually reached.
|
|
Blanking the pane while leaving the row highlighted reads as half an
action, and deselecting is what Escape means nearly everywhere else. The
user asked for two actions rather than a changed one, so clear_pane keeps
its behaviour and moves to Shift+Esc; clear_selection takes Escape and
does both. Shift+Esc rather than unbound because every action carries a
default and a test enforces it.
Clearing the selection re-adopts the thread it just cleared, unless done
in exactly the right way. clearSelection() leaves currentIndex() valid,
so onSelectionChanged takes its "one or fewer rows" branch, sees a
current row whose id differs from m_currentThreadId, and calls
onThreadSelected for it. Clearing the selection before blanking lets that
run while the id still matches, so nothing reloads, and clearing current
stops a later collapse-to-one-row reaching the same row. All four
arrangements were tried; only this one passes.
The first version of the test could not distinguish any of them. It
asserted showingPlaceholder(), which passes regardless because this
fixture has no worker, so loadThread never replies and the pane is never
repainted. currentThreadId() and currentIndex() are observable without
one, and asserting those is what made the test discriminate.
|
|
An empty right pane said nothing, and multi-select made it a routine
sight. It now carries the wordmark, thread counts that run their query
when clicked, and a sync line that appears only when something needs
attention.
Rendered into the existing web view as a third document shape, so there
is one document path and one set of security rules. The brand palette is
a deliberate exception to deriving colours from the desktop theme, since
a logo is brand rather than chrome; the theme still picks which of the
two sets is used.
Counts refresh when the pane is about to show rather than in the
background: one goes stale the moment a tag is edited, and refreshing one
nobody is looking at is work for nothing. A generation counter discards a
superseded reply, and a late answer cannot repaint over an opened thread.
The helper lines are real links because JavaScript is off in this
profile. The handler is gated on the placeholder actually being
displayed, so the same URL inside a message body is dropped: a stranger's
mail must not drive the thread list, even to run a harmless query.
Three defects found while building, all silent:
- Every CSS percentage was invalid. QString::arg does not collapse "%%"
into "%", so the document carried "50%%" and the browser dropped each
declaration holding one, disabling the mask, the glow and both radial
gradients while still rendering something plausible. Substitution is by
named token now, which cannot collide with a percent sign.
- A geometry probe endorsed the layout while that was live, because it
measured only properties without percentages.
- The font test passed against a build with one face missing, since the
other satisfied both of its checks on its own.
The mockup's light values needed correcting against a real pane: the grid
vanished at a 2% luminance step on white, and the glow subtracts light
there rather than adding it, washing the pane. Strength only, not hue.
|
|
The thread list was uniform and cramped: every row one line tall, with
nothing to say what a thread was about before opening it. Rows are now
roughly double height, carrying a strip of tag chips beneath the text,
with alternating row colours and a star column for flagged threads
beside the existing paperclip.
The strip is painted by the VIEW rather than by a delegate, which is
why ThreadListView exists. A delegate is handed one cell's rectangle
and cannot paint outside its column, so a strip drawn from the subject
column stops at that column's edge, losing the last tags of a
well-tagged thread, and starts at its left edge, putting the chips
under the subject instead of under the row.
Tags the row already shows another way are left out: inbox as
structure, unread as the dimming, flagged as the star, attachment as
the paperclip, and the account as the chip in the subject cell. Sorted,
since notmuch's order is not guaranteed stable and a row whose chips
reordered between repaints would flicker.
Six defects were introduced and fixed on the way here, all of them one
consequence: a QTableView paints per cell, and a row-wide strip is not
a cell. SubjectDelegate installed view-wide drew the account chip into
every column, since AccountLabelRole belongs to the row; it is split
into RowStyleDelegate for every column and SubjectDelegate for the
subject alone, with a Q_ASSERT guarding that. Row height returned from
sizeHint did nothing, because a table takes one height per row. The
strip painted from x=0 over the marker columns, via a protected
viewportMargins() that returns 0. Measuring the text band and the strip
with one font put the pills over the date. Alternating colours and the
selection are per-cell too, so the band showed bare viewport background
until the view filled it, honouring the model's own BackgroundRole
first so a deleted row is not cut in half. And that fill spanned the
full width, cutting the centred marker glyphs at their midpoint.
Closes item 5.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Bold was unread's only cue, and it renders identically to regular on the
user's system: confirmed by eye against a bare QTableView holding a
plain QStandardItemModel, with no code from this project involved. The
fault is in Qt or fontconfig, below this application, and nothing in the
model could ever have reached it. Read and unread mail looked exactly
alike.
The emphasis is inverted instead. Unread rows keep the palette's own
text colour and read rows are dimmed toward the background, so the cue
rides on Qt::ForegroundRole, which the delegate already honours, and
costs no column. It also suits the real ratio, measured at 99 unread
against 4220 read: dimming the bulk is calmer than highlighting it. The
dim colour is derived from the palette, never hardcoded, per the rule
item 12 established. Bold is kept for systems where it works, but
nothing depends on it now.
That exposed a second defect, visible the moment it shipped. Qt resolves
ForegroundRole into the palette and then prefers it over
HighlightedText, so a model-supplied colour wins on a SELECTED row too.
The dim is blended against the unselected background, so a selected read
row painted grey on the selection colour, near unreadable.
SubjectDelegate::initStyleOption now reverses that, and the delegate is
installed view-wide rather than on the subject column alone, so every
column gets the same handling instead of three of them keeping Qt's
ordering.
The guarding tests state the property rather than the mechanism: strip
the font from the model's answer and the two states must still differ.
A test asserting only that bold is set passes on a system where bold
paints like regular, which is exactly how this survived. The selection
test renders two rows identical but for the unread tag, selects both,
and requires zero differing pixels.
Part of item 5; the density work and the star column remain.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The stylesheet hardcoded #bbb, #555, #000, #666, #ddd and #4a6f8a, and
set no background at all, so on a dark desktop plain-text mail rendered
as black on white inside a dark window and the web view's own default
showed through.
The colours now come from a Palette struct derived from QPalette, and
are passed into the builder rather than read from qApp inside it, so
the stylesheet can be tested against a known palette with no running
application. Base and Text rather than Window and WindowText: the pane
is a content surface like a text edit, and on many themes those differ.
The secondary colours are blends of text and background, not fixed
greys. That is the part that makes it work both ways round, since a
#555 chosen to read as subtle on white is nearly invisible on #2b2b2b.
The quote colour keeps its hue, because "this is quoted" is carried by
being a different colour rather than a dimmer one, but is pulled toward
the background so it stays readable instead of glowing on dark.
A sender's own HTML is deliberately left alone, and a test asserts that
so it cannot drift: rewriting a sender's styling would break layouts
that depend on it, and a newsletter setting a white background is
entitled to stay white. This themes the plain-text render and the
chrome around messages, nothing more.
MessageView passes its own widget palette rather than the
application's, since a style sheet or a themed parent can give the pane
different colours from qApp, and re-renders on PaletteChange: the
document's colours are baked into its stylesheet at build time, so
unlike a widget it does not restyle itself when the desktop theme
changes.
The load-bearing test asserts the negative, that no hex colour appears
in the style block which the palette did not supply. A test checking
only that the palette's colours are present passes with a leftover
literal still there, and one leftover literal is the whole defect.
Confirmed by mutation.
Closes item 12.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
An action removing "unread" from every thread in the current view,
on the toolbar, the Message menu and Ctrl+Shift+U. It deliberately
ignores the selection, which makes it the one action in the window
that does, and it routes through the same funnel as every other tag
change, so it is one write rather than one per thread.
Disabled until the query reports its total. Threads arrive in batches,
so before then the model holds only what has landed, and an action
saying "all" must not silently skip the rest. A greyed control says
"not yet" without needing a dialog or a stall the user cannot see.
The state is also set at registration, since QAction starts enabled
and a window that has not run a query has nothing to act on.
Two things came out differently from the plan, both forced by existing
code. It carries a default binding, because everyActionHasAShortcut
requires every registered action to have one: an unbound action is
unreachable from the keyboard, and that invariant is deliberate, so the
action was given Ctrl+Shift+U rather than the invariant relaxed. And
only the threads that are actually unread are sent, because sending the
rest would inflate the pending-edit count with writes that change
nothing, and the quit prompt reads that count. A view with nothing
unread does nothing, pushes no command and says so: an undo entry that
restores nothing is worse than none, since it absorbs a Ctrl+Z meant
for the previous action.
undoDepthForTesting() is new and exists for a reason worth recording:
undo->isEnabled() cannot answer "was a command pushed", because the
undo QAction is always enabled and tests canUndo() when triggered. The
first version of the no-op test asserted on it and passed against a
mutant with the unread filter removed.
Closes item 43.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
"Syncing..." was set once and never updated, so a run that takes over a
minute reported nothing about what it was doing.
The original diagnosis in the backlog was half wrong, and two further
wrong ones were made and discarded before the real cause: plain
"mbsync -a" prints NOTHING until it exits, then a single summary line.
Measured on a real run, one line at 11:11:08 then 73 within the second
11:11:33, at the end of a 46-second run. So there was no stream to read
for the part of a sync that takes time. It is not buffering, so stdbuf
changes nothing, and the account name is not unavailable either, which
was the second wrong conclusion.
mbsync -V is what changes both: it announces each channel as it reaches
it, which is at once the progress and the account name originally
asked for. The shipped script now passes it.
SyncPhaseTracker derives a short status from the output as it streams:
the channel being synced, the summary counts when mbsync ends, then the
notmuch reindex. It lives beside MailSync rather than in the window so
the matching rules are one testable thing, and it holds no widget.
Matching is loose and case-insensitive, since the wording varies by
version, and nothing in it decides success or failure: the exit status
remains the only authority on that.
Lines are reassembled in MainWindow before being fed, because
QProcess::readAll() splits wherever it happens to and a half-line would
match nothing. Every status is sanitised and truncated: the channel
name comes from a config file this app does not own, and a long one
must not stretch the status bar.
Two defects in existing code, fixed with it. setSyncBusy(true) ran
after start(), so a fast run's output arrived before the per-run reset
and wiped its own phase. And a first draft deferred phases while a
transient message showed, which let a "Background sync completed"
message armed before the sync began suppress the whole run: a running
sync's state outranks an expiring event message.
Verified by replaying real captured mbsync -V output through the
tracker, not only against fixtures. The MainWindow test paces its
script with sleeps, since a script that prints everything at once
arrives in one readyRead and makes every intermediate phase
unobservable.
Closes item 42.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
collectParts() filed any part with a Content-Id into inlineParts and
returned before the text/plain and text/html branches. Setting a
Content-Id on the text/html body is legal and common in bulk-sender
output, and such a message parsed with both body slots empty, so
hasHtml() was false, HtmlBuilder fell through to an empty plain body,
and the pane rendered nothing. Both halves of the report, the blank
message and "no HTML part", came from that one ordering.
A content id makes a part referenceable, not undisplayable. The two are
independent. The branch now registers the part and falls through rather
than returning, so the body still fills its slot. Registering first
keeps a part that is both the body and a cid: target reachable under
its id for any sibling referencing it.
Content-Disposition is deliberately not used as the discriminator: it
is absent far more often than it is correct, and a body part commonly
carries none. The existing attachment check remains the only test for
"not a body", and the first-one-wins isEmpty() guard still stops an
inline image displacing a real body, since an image matches neither
text branch.
Verified against a hand-written fixture whose text/html part carries a
Content-Id, asserting the body renders, the id still resolves, and the
sibling image is unaffected. Load-bearing by mutation: restoring the
early return fails the test. The user could not relocate the message
that prompted the report, so the end-to-end path is unconfirmed.
Closes item 41.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Both TagDialog fields built their completer from knownTags, the whole
database's tag list, so removing a tag offered every tag in existence
rather than the handful the selected threads actually carry.
The candidates were already in the dialog: currentTags, used until now
only to render the checkbox list. The constructor now walks two
(field, vocabulary) pairs instead of two fields sharing one list, with
knownTags for Add and currentTags.keys() for Remove. On a multi-thread
selection that is the union, not the intersection, since removing a tag
two of three threads carry is a meaningful request.
The setWidget and per-token prefix machinery is untouched: these fields
hold a comma-separated list, and QLineEdit::setCompleter is the trap
this dialog already works around. Only the candidate list changed.
Completion stays a suggestion, never a whitelist, so a tag absent from
the candidates still applies.
Tests type keys rather than using setText, which does not drive a
completer at all. Verified load-bearing by mutation: reverting the
Remove vocabulary to knownTags fails the new test.
Closes item 48.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Sync had two controls that behaved differently. The QPushButton beside the
query bar cleared the log, opened the pane and disabled itself; the QAction
behind the toolbar, menu and shortcut called start() and did nothing else,
discarding its return value so a rejected start was silent. Worse, item 29's
"disable Sync during a background sync" set the button only, so the toolbar
entry stayed clickable through a cron sync and could only produce the
script's EX_TEMPFAIL skip.
startSync() is now the single handler behind every route in, and the enabled
state lives on the QAction, which reaches the toolbar, the menu and the
shortcut at once. It also reports when no sync command is configured rather
than doing nothing.
The QPushButton is gone. It read as a Search button given it sat beside a
text field, which is the user's own observation and the reason the toolbar
one survives instead. Its unavailable-command tooltip moved to the action,
since that is the only thing that says why the control is dead.
Removing it left the query field running flush to the window edge, so the
saved-query buttons move from their own row onto the query row. The bar is
now framed by the account dropdown on the left and the saved queries on the
right, the empty row is gone, and the thread list gains the space. The field
also gains setClearButtonEnabled, which is Qt's own themed clear icon rather
than a hand-rolled button. A "Search" button was considered and rejected:
Return already runs the query.
No overflow handling for [queries], which is unbounded. Three entries fit;
item 23 already specifies buttons-plus-menu and is where that belongs.
CLAUDE.md's architecture diagram named four widget classes that have never
existed, QueryBar, SavedQueryBar, HeaderWidget and AttachmentBar. The query
row and the message header are built inline. Corrected, and the components
that do exist but were missing from it added.
Tests: the new action test was verified red first and load-bearing by
mutation. The old button test is deleted rather than repointed, being an
exact duplicate of it, and the unobservable-lock test now drives the action.
The clear button and the row layout were confirmed by hand; no test clicks
the icon, which is a mouse path.
Backlog: 45 done and reclassified as a defect rather than a cosmetic
redundancy, 47 added for the bar.
|
|
Both are the same class of defect: a test that reads real machine state and
so passes or fails on circumstance rather than on the code under test.
Item 38. Every MainWindow a test builds constructed its SyncMonitor on the
live /proc/locks, so a window observed the machine's actual sync state and
the sync-button assertion failed whenever the user's cron sync happened to
be running. Cron fires every ten minutes and a run lasts ~35s, which is
roughly 6% of runs, and it read as flakiness. SyncMonitor already took an
injectable locks path for exactly this; MainWindow did not expose it. It
does now, as a test seam rather than a config key: /proc/locks is not
something a user would set, and a wrong value silently disables background
sync detection instead of failing loudly.
The monitor is still constructed and started, per the item's own constraint.
Only the table it reads is redirected, to an empty file in the test's own
temporary directory.
Item 46. uiStateSurvivesARestart asserted a 940px width, and the offscreen
platform reports an 800x800 screen. restoreGeometry() clamps to the
available area, so the width came back as 798 while the 620 height, which
fits, restored untouched. That asymmetry was the tell that persistence was
fine and the test was wrong. The size is now 640x560 and carries no meaning
beyond differing from the default.
Verified by reproducing the original conditions rather than by waiting for
them: the suite run under flock -n /tmp/mbsync.lock fails item 38's
assertion with the seam bypassed and passes with it in place, and item 46
now passes under offscreen where it failed. One dud mutation is recorded in
the backlog, writing an unparseable line into the injected lock table does
not fail the test, because lockHeldIn() correctly finds no lock in it.
Suite: 15/15 offscreen with the lock held, and green on Wayland except the
pre-existing querycompleter screenshot flake, which fails to grab under
Wayland and passes offscreen.
|
|
A tag edit sent while another process holds notmuch's write lock does not
fail: the read-write open blocks and then succeeds. Measured against
Slackware's notmuch, 9.158s against a 12s hold, status SUCCESS. Since the
worker is a single thread, that blocked open holds up every read queued
behind it, so the message pane freezes on whichever thread was selected
first and replays the queue when the lock releases.
The window now defers instead. While SyncMonitor reports a sync running, a
tag change is held rather than sent, and flushed when the sync ends. The
optimistic update stands in the meantime, so the row keeps its tag and the
edit still counts toward the unsynced indicator, which is what the quit
prompt reads.
The original diagnosis was that the open fails and the edit is discarded,
and a retry was built on it. That was wrong: the error branch in
notmuchworker.cpp is unreachable through lock contention. The premise was
taken from a plausible-looking error path without provoking the condition,
and measurement disproved it. The backlog entry records this rather than
quietly correcting it.
Verified by hand against a real blocking open, which the tests cannot reach:
they drive the deferral through the meta-object and never take a lock. Both
locks held for 100s with a tag edit made during the hold. Row kept the tag,
status did not expire, indicator rose, window stayed responsive, held edit
sent itself on release.
The 2s SyncMonitor polling window is knowingly left open: a sync starting
between polls is invisible for up to 2s and an edit there still blocks.
SyncMonitor::lockHeldIn() would close it at the cost of one file read per
tag action, and is recorded as the option to revisit.
Also fixes revertPendingTagChange() clearing the entire undo stack after any
rejected write, found while working on this.
Backlog: item 37 done, and item 46 added for a test that fails only under
the offscreen platform, where an 800x800 screen clamps a restored 940px
window. Pre-existing and unrelated; the suite is green otherwise.
|
|
Item 28, reported by the user: open a thread, let the automatic
mark-read remove `unread`, then press Ctrl+U to put it back. The
indicator read "2 unsynced change(s)" with the mail store exactly where
it started.
The counter incremented per confirmed write and never decremented, so
any add-then-remove of the same tag inflated it. Mark-read is simply the
path that fires without being asked, which is why it surfaced there.
The user's call was net state: an edit and its inverse are zero
outstanding changes, because what the indicator answers is whether
quitting now would strand work. A QHash keyed "<messageId>\n<tag>"
replaces the int, and a pair that reverts is erased rather than stored
with the new direction, so the map cannot grow without bound across a
long session of tagging and untagging.
Keyed per (message, tag) rather than per message: removing `unread` and
adding `flagged` on one message are independent changes and must not
cancel each other. A change carrying no message ids cannot be netted
against anything and is counted separately, since dropping it would
understate the indicator, which is the direction that costs work.
Both properties item 18 established still hold: a successful sync clears
everything, a failed one clears nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Items 16 and 33.
Delete now removes the `deleted` tag when every selected thread already
carries it, so pressing it twice puts a thread back. One direction for
the whole selection, never per row: toggling each independently would
leave a single keystroke with the selection in two states, which is
worse than either outcome.
Status messages are classified rather than blanket-timed, which is the
substance of item 33. Events expire after six seconds and fall back to
the last query's thread count: "Sync complete", "Nothing to undo", the
skip notice, the per-action "Archive: 3 threads". State does not expire:
"Searching...", "Syncing...", the selection count, and "Sync failed
(exit N)", because an error must not vanish before it is read.
A test caught a mistake in that routing. Making the per-action message
transient armed the timer during select-all, since tagSelected() runs on
a selection onSelectionChanged() had just described, and the count would
then be replaced while it was still true. Writing the count now cancels
any transient still counting down.
QStatusBar::showMessage() would give the same behaviour but the label is
added with addWidget() beside permanent widgets, so adopting it means
reworking that arrangement. One timer beside the label is the smaller
change.
Both fixes verified by reverting them and watching the tests fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Item 31. The user could not tell which button Enter would press on the
unsynced-changes dialog.
The code was already correct: setDefaultButton() is called, and Qt agrees,
isDefault() and hasFocus() are both true on "Sync and quit". The active
style, qt6ct-style, simply draws no visible default-button decoration. The
GIMP dialog offered for comparison is GTK drawing its own focus ring, a
different toolkit.
Naming the default in the text rather than restyling the button:
overriding the appearance means fighting the user's theme, which is worse
than one word. The safe option was already the default, so no behaviour
changed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Items 29 and 32.
29 was a constraint item 27 specified and that shipped unbuilt: while a
cron sync held the lock the Sync button stayed clickable, and pressing it
could only produce the EX_TEMPFAIL skip. The progress bar and the button
are now written by one updateSyncControls() taking both sync sources,
which the item asked for by name: two independent assignments, one per
path, means whichever finishes second wins, so a background sync ending
would re-enable the button in the middle of a local run.
Unknown re-enables the button, deliberately. It means /proc/locks could
not be read and nothing was observed, so leaving the button disabled
would strand it permanently wherever the lock cannot be seen.
32 adds a clear_pane action on Esc. It clears m_currentThreadId with the
pane, not merely alongside it, or a threadLoaded still in flight would
paint the thread straight back; and it cancels any pending mark-read,
since a thread blanked from view must not be marked read two seconds
later. The selection, the query and the undo stack are untouched.
The one real risk in 32 was Escape being stolen from the query
completer, the way Return was once lost to a window shortcut. Probed
rather than reasoned about: a popup consumes the key before a
window-level shortcut sees it, so the completer still dismisses.
Every test here was verified by reverting the code it covers.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Reported from hand testing: a manual sync ended with "Sync finished
elsewhere" stamped over its own result.
Ownership of a lock period was being decided when the lock was RELEASED,
by asking MailSync::isRunning(). That question cannot be answered then:
the process exits, so isRunning() goes false, and only afterwards does
the next poll observe the lock gone. The guard therefore suppressed the
message while the sync ran and let it through at the end, up to two
seconds after onSyncFinished() had already said what happened.
Ownership is now latched when the lock APPEARS, which is the moment
isRunning() can still answer, and the matching release is swallowed.
onSyncFinished() hands the latch back when it sees exit 75, because a
skip means the lock was never ours: if a manual run and the cron run
start inside one poll interval, the lock would otherwise be latched as
local and that other run's completion swallowed with it.
Also renames the messages to "Background sync running/completed" per the
user: "finished elsewhere" reads as though the application does not know
what is syncing the Maildir, when in fact it is the same script.
The tests added here cover the external path and the Unknown state. They
do NOT reproduce the reported bug, and were checked against a reverted
fix to confirm that: staging it needs isRunning() true at the Running
transition and false at the Idle one, which cannot be arranged in
test_mainwindow without a configured sync command and a live child
process. That was tried and abandoned, it left a process running for the
length of the suite and popped a dialog. The ordering and the fix were
instead verified against a standalone model of both code paths.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The user's cron runs mailsync.sh every ten minutes, so mail arrives and
tags change while the window sits idle, and nothing here noticed. The
script already holds an flock for the whole run, so that lock is the
signal: no status file is needed, and a kernel lock cannot go stale
because it dies with the process holding it.
The observation method is the part that matters, and two of the three
plausible ones are wrong. Both were probed on Linux 6.18 before any of
this was written:
- flock -n acquires in order to test, so polling every two seconds
would open a window every two seconds in which a starting
mailsync.sh is refused the lock and exits 75. It would cause the
very skips the script reports.
- fcntl(F_OFD_GETLK) never acquires and looks ideal, but reports
UNLOCKED against a lock held by flock(2): separate lock namespaces
in the kernel, which cannot see each other. A silent false negative.
- /proc/locks is a pure read. It observes flock(2) correctly, and
since it takes no lock at all it can never contend with the Xapian
write lock notmuch new holds during the same run. Confirmed: 200
reads left the lock table unchanged and this process holding
nothing.
SyncMonitor keeps the parsing separate from the polling so the parsing
is testable, and it reports Unknown rather than Idle where /proc/locks
cannot be read: "no sync is running" is the claim that would let the
window quit, so it must never be guessed. Verified against a real flock
end to end, not only against synthetic content.
It reports rather than refreshes. runCurrentQuery() clears the undo
stack, the selection and the message pane, which is right for a query
the user typed and hostile for one a cron timer fired: it would discard
undo history and close the thread being read up to six times an hour,
with no action from the user.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Multi-select already worked by Ctrl+click and Shift+click, but nothing in
the UI said so and every tag action was keyboard-only, so the Ctrl+T tag
dialog could not be reached with a mouse at all.
Adds a select_all action on Ctrl+A, registered like every other action so
it reaches the Edit menu, the shortcut reference and [keys]; a right-click
menu on the thread list built from the same QActions rather than parallel
copies; a selection count in the status bar, which is the part that
actually teaches the feature by acknowledging a selection while it is
being built; and a note in the shortcut dialog for the mouse gestures,
which are view behaviour and cannot appear in the generated table.
A selection gesture must not open mail or mutate it. Selecting several
rows now blanks the message pane and cancels any pending mark-read,
rather than rendering each row swept through and queueing it to be marked
read.
Two Qt behaviours shaped this, both established by probe rather than from
memory:
- selectAll() emits no currentRowChanged at all and leaves the current
index invalid.
- currentRowChanged is emitted BEFORE the selection model is updated.
The second one caused two distinct faults. Collapsing a multi-row
selection back to one row reported the old count, so the guard swallowed
the load and the pane stayed blank; that case is handled in
onSelectionChanged, which sees the true count. And a Ctrl+click taking
the selection from one row to two also reported one, so the thread was
loaded, blanked, and then painted back when the queued reply returned
from the worker. By the third row the id was already cleared and the
reply was discarded, which is why the fault presented as an off-by-one in
the threshold rather than as a race.
Tests cover the synchronous half. The late-reply guard has no test:
MainWindow in tests has no worker, so threadLoaded never fires and the
repaint cannot be reproduced in process. Verified by hand instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Real Maildir account keys had reached comments and test data:
provider-and-mailbox names across three source files, one of them
carrying a surname, plus a real address used as example data in the
notmuch test fixture and the design spec.
The user's standing rule is that maildir and account names never reach a
commit, and this is about to become a public repository, which is what
makes it consequential rather than untidy. Replaced with generic keys
that carry the same shape, since the length is the point in every one of
these comments: a 33-character account tag is why the chip label exists
and why the tag column was removed.
The measurements stay. They are the evidence behind those decisions and
are not personal details.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
