| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
Spelled out per row, tags ran to 500 pixels of largely repeated text and
took most of the thread list's width. The column is gone; tags render as
coloured chips split by what they actually mean.
An account tag says which mailbox a thread arrived in, and draws as a
chip in front of the subject. A functional tag says what state a thread
is in, and those fill one row under the message pane. One row keeps the
message area from shifting between threads with different tag counts, so
whatever does not fit collapses into a +N chip that names the rest in its
tooltip.
TagColors resolves a colour by exact tag first, then by top-level prefix,
so a single "shopping" entry covers shopping/amazon and shopping/nike
while shopping/amazon can still override its own. That matters at 96
tags. Built-in defaults cover the usual state tags, and anything left
unconfigured falls back to a hash of the name, stable so a chip does not
change colour as the list scrolls.
|
|
Selecting a thread and hitting Delete changed nothing on screen, so
there was no way to tell the action had stuck.
The tag was always applied: applyTagChange() emitted dataChanged across
the row, and the Tags column did update. But Subject was set to stretch
while Tags came after it, so Subject took all free width and pushed Tags
out of view. The feedback lived in the one column that could not be seen.
Columns are now Tags, Date, From, Subject, with Subject stretching last
so nothing can be pushed off the right edge. A thread tagged deleted or
spam fills its whole row, muted red or orange with white struck-through
text, through the background, foreground and font roles, so no cue
depends on one column remaining visible.
Strike-through accompanies the fill on purpose: it survives a theme that
overrides backgrounds and reads without colour. Bold for unread still
composes with it.
Archive adds no tag, so an archived row is left unstyled for now.
|
|
Actions were a QHash of std::function dispatched by an event filter,
which nothing could put in a menu. They are QActions now, bound from
KeyMap so a [keys] override reaches the menus as well as the keyboard.
Menu bar covers every action; the toolbar carries only Sync, Archive,
Delete and Undo. Help > Keyboard shortcuts is generated from the actions,
so it shows what the keys really do rather than a copy that drifts.
spam and load_remote gained defaults, having been unreachable without a
hand-written binding.
The event filter is gone. Probing showed QAction shortcuts are dispatched
before the focused widget sees the key, so they beat QAbstractItemView's
type-to-search without one, and Qt already suppresses plain-letter
shortcuts while an editable widget has focus. Dropping the filter's
blanket guard also lets Ctrl+Q work while the query bar has focus.
registeredActionNames() is derived from the actions rather than
hand-maintained, so the two drift tests it needed are replaced by checks
that a configured binding reaches its action.
No confirmation dialogs: tag mutations still answer to undo.
|
|
Typing a capital emits Shift+<key>, but QKeySequence::fromString() folds
the case of a bare letter away: "N" parsed to plain Key_N, a combination
no keystroke produces. The N, F and G defaults (toggle_unread, flag and
sync) therefore never fired, and neither would any hand-written capital
in [keys].
normalizeSequence() rewrites a bare capital to Shift+<letter> and is
shared by the defaults and the override pass. As a side effect "y" and
"Y" become distinct keys rather than a collision that dropped one.
Defaults move to modifier shortcuts throughout. A single letter cannot
be a QAction shortcut without stealing that letter from every text field
in the window, and the menus in the next commit need real accelerators.
defaultBindings() is now the one source of truth for them.
|
|
Confirmed with the maintainer as v2-only rather than v2-or-later. LICENSE is
the official text from gnu.org. Every file under src/ and tests/ carries the
matching notice.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Clicking a thread left the pane blank. Two independent bugs, both from the
same false premise: that setHtml() navigates to the base URL it is given.
It does not. setHtml() navigates to a data: URL carrying the markup and
applies the base URL afterwards, purely as the document's origin. Verified
empirically on Qt 6.11.
Built on that wrong assumption were:
- MessagePage::acceptNavigationRequest compared the navigation's URL
against documentUrl() and rejected everything else, so the document load
was refused. It now accepts a typed main-frame navigation, which is one
we initiated ourselves.
- RequestInterceptor exempted exactly the qtmaildir: base URL and denied
everything else, so the data: document load was blocked too.
The interceptor fix is scoped to ResourceTypeMainFrame rather than allowing
the data: scheme outright. A blanket allow would have been a real hole: a
message body can write <img src="data:..."> or an iframe, and the existing
dataSchemeBlocked test in test_interceptor.cpp was right to fail when that
was tried. Sub-resource data: URLs remain denied.
Note this was never working. The drafted version had the same defect in a
different spelling (it compared url.scheme() rather than the whole URL, and
would have rejected the data: navigation just the same), and task 11 shipped
with no runtime test to catch it. test_messageview.cpp now pins all three
facts: the document loads, its text reaches the page, and a data: image
inside a hostile body stays blocked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Found while walking the task 13 checklist against real mail. Item 1
("startup shows no configuration warnings with a valid config") failed:
with a perfectly valid config that simply had no [sync] command, every
launch opened a blocking modal that had to be dismissed before the window
could be used.
Config now separates the two cases. A problem is something configured but
wrong (a sync command that does not exist, an account with no maildir);
those still open a dialog, as does every KeyMap warning, since each one
means a binding the user wrote is being ignored. A notice is an optional
feature simply not being configured; it reports to the status bar only.
Nothing is broken in that case, and a modal on every launch teaches the
user to dismiss dialogs unread, which defeats the ones that matter.
problems() is a subset of warnings(), so callers wanting everything need
only the latter.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Wires the worker thread, thread list, message pane, sync process and undo
stack together, and replaces the placeholder main() with real startup:
custom URL schemes registered before QApplication, a libnotmuch ABI check,
and config loading.
Four fixes against the drafted version:
- onWorkerError() only set a status label. Its own comment elsewhere
claimed it reverted the optimistic update, and the spec requires that;
it did not, so a rejected write left the list showing a tag the database
never received. The pending change is now recorded and rolled back, and
a confirmed tagsApplied clears it so a later unrelated error cannot undo
a write that succeeded.
- runCurrentQuery() cleared the model but left the undo stack pointing at
rows that no longer exist. Undoing after a new query would have written
to the database while the visible list stayed put. The stack is cleared
with the model.
- m_currentMessages was assigned on every thread load and never read.
Removed.
- buildUi() connected sync output to m_syncLog and errors to m_statusLabel
before either existed. Both are constructed before the wiring now.
cidPrefix generation lives here, this being its only producer in the
application, and is pinned by tests: it must never contain '!' and must be
distinct per message, which are the invariants the cid: namespacing rests
on. A second test holds registeredActionNames() against
KeyMap::knownActions(), since those two hand-maintained lists drifting
either way silently breaks a user's key binding. Mutation-verified that
dropping an action fails the test by name.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Off-the-record profile, JavaScript off, deny-by-default interceptor, and a
page subclass that hands link clicks to the system browser so a message can
never navigate the pane.
Honours the obligation task 5 recorded: the interceptor trusts exactly one
qtmaildir: URL and fails closed otherwise, so setHtml() and setDocumentUrl()
must agree or the pane renders nothing. Rather than pairing those calls at
each site, every load goes through one setDocument() and the URL comes from
a single documentUrl() accessor. Verified against the real interceptor that
this URL is allowed while siblings, subpaths, remote and file: are not.
Three fixes against the drafted version:
- showError() called setHtml() with a base URL but never setDocumentUrl(),
so an error card would have rendered blank. Now impossible to repeat.
- clear() and showError() left the previous thread's inline parts in the
scheme handler and its cids in the interceptor. Both now empty the policy,
so no thread's parts outlive it.
- MessagePage trusted the whole qtmaildir: scheme for typed navigations,
which is the same blanket-trust mistake task 5 removed from the
interceptor. It now matches the exact document URL.
The parts-flattening is extracted into buildThreadCidMap() so it can be
tested without a live profile, and a cidPrefix containing '!' is sanitized
rather than trusted, since Q_ASSERT is compiled out in release and this map
decides which bytes a message can name. The sanitizer escapes '_' before
replacing '!', because a plain replace would map "m0!x" and "m0_x" onto one
key and merge two messages, which is the very collision the namespacing
exists to prevent. Mutation-verified: the naive replace fails the
distinctness test, and dropping the sanitizer trips the assert.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Runs the configured sync script through QProcess, merging stdout and
stderr into one log so a failing mbsync run has something to show. The
script is never run through a shell: the command is a config value, and
splitCommand keeps its arguments literal.
Two fixes against the drafted version:
- start() no longer calls waitForStarted(). It blocked the UI thread for
up to five seconds, which contradicts the spec's requirement that the UI
stay usable during sync, and it swallowed launch failures into a bare
false return. A missing script now surfaces asynchronously through
errorOccurred as finished(false, -1) with an explanatory log line, so
the spinner cannot hang with nothing to explain it.
- Removed a double-emit guard I had added on the assumption that QProcess
follows errorOccurred(FailedToStart) with finished(). Verified it does
not: FailedToStart is emitted instead of finished, never before it. The
guard was dead state and the comment justifying it was wrong.
Also corrects the sync interval throughout: the user's cron runs every 10
minutes, not hourly. The shorter interval strengthens the flock rationale
rather than weakening it, since collisions with a manual sync are that
much more likely.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
QAbstractTableModel over query results, appended in batches so a large
query paints its first screenful immediately. Tag changes apply locally
for optimistic UI; reverting a failed write means calling applyTagChange
again with added and removed swapped, which the round-trip test pins.
Two additions to the drafted version:
- A ThreadIdRole, so a view's QModelIndex maps back to the thread id the
worker speaks without every caller reaching around the model.
- data() checks its own row and column bounds. Qt will not hand out an
out-of-range index and invalidates persistent ones on reset, so this is
unreachable defence rather than a live path; the test says so instead of
pretending to cover it.
Verified by mutation that the empty-batch guard, the ThreadIdRole, and the
full-row dataChanged range each fail exactly one test when removed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
Owns the only notmuch database handle. Queries run read-only and emit
threads in batches of 200 with a generation counter so the UI can discard
superseded results. Tag mutation closes the read-only handle, opens
read-write, applies, and closes, holding the process-wide write lock for
milliseconds rather than blocking a concurrent `notmuch new`.
Tested against a throwaway database built in a QTemporaryDir, superseding
the spec's original "no unit test" position: applyTags is the only code
here that writes to a notmuch index. The fixture never touches ~/Mail or
~/.notmuch-config.
Two fixes against the drafted implementation, both caught by mutating the
code and confirming exactly one test failed:
- loadThread conflated "no query given" with "query matched nothing in
this thread", so filtering a thread down to zero matches rendered every
message expanded. Tracked with an explicit haveMatchSet flag.
- applyTags now documents why a stale message id must skip rather than
abort: notmuch_database_find_message reports SUCCESS with a null message
for an unknown id, and the live ids alongside it still need tagging.
Note for fixture authors: notmuch synchronizes maildir flags with tags at
index time, so a file named `...:2,S` is indexed without the unread tag no
matter what [new] tags requests. Unread fixture messages go in new/.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
The cid: namespacing scheme (cid:<prefix>!<id>) is only unambiguous
because the prefix half is guaranteed free of '!': the first '!' in the
result is always the separator, so an attacker-controlled Content-ID
containing '!' only extends the id half rather than colliding with a
different prefix. That invariant previously existed only as a comment.
Add Q_ASSERT_X at both independent call sites that perform this
concatenation (CidSchemeHandler::namespacedKey and
HtmlBuilder::namespaceCids) so a future prefix generator that violates it
traps in debug builds, per Task 5's precedent of not letting one unit's
correctness depend silently on another's future behaviour. Since
Q_ASSERT compiles out in release, pin the property that actually matters
release builds too test: distinct (prefix, id) pairs across a documented
"m<index>" prefix set and hostile Content-IDs (containing '!', percent-
encoded '!', empty, leading/trailing '!') never collide, and the key
always splits at its first '!' back to the exact original prefix.
|
|
HtmlBuilder renders parsed messages (and whole threads, as one document,
so newsletter threads don't spawn one Chromium process per message) into
the HTML string the web view loads. Plain text is escaped and quote lines
marked; the cid: rewrite is namespaced per message ("<prefix>!<id>") so
two thread messages sharing a Content-ID don't collide.
Hardened namespaceCids beyond the initial sketch after attacking it:
handles unquoted cid: attribute values, background=/poster= (not just
src/href), and CSS url(cid:...) in both style="" attributes and <style>
blocks, all case-insensitively. Replaced the greedy [^"']+ capture with
per-quote-style alternation so two cid: refs on one line can't bleed into
each other.
CidSchemeHandler serves cid: requests from the thread's inline-parts map,
keyed by the same namespaced string, replaced wholesale per thread.
|
|
Whole-scheme allow meant a hostile message body could reference any
qtmaildir: URL (e.g. <img src="qtmaildir://other">) and have it pass,
with safety depending entirely on Task 11's still-unwritten scheme
handler. Add setDocumentUrl() and require an exact QUrl match; deny
all qtmaildir: URLs when it is unset (fail closed). Document URL
survives resetForNewMessage() since it is a property of the view, not
of a message.
|
|
|
|
Attachment::saveTo()'s escape guard compared paths with a bare
QString::startsWith(), which is not a path-boundary test: "/tmp/safe-evil"
textually starts with "/tmp/safe", so a sibling directory whose name merely
extends the target's name would incorrectly pass as contained within it.
Extract the check into Attachment::isPathInsideDirectory(), comparing
QDir::cleanPath()'d absolute paths and requiring an exact match or a prefix
ending at a '/' boundary. Not exploitable today since safeFilename() always
reduces the name to a bare basename before saveTo() builds the target, so
the guard is unreachable via saveTo()'s public interface; comments on both
now say so plainly instead of implying it is currently load-bearing.
Add pathInsideDirectoryRejectsSiblingPrefix, testing the guard directly
(independent of safeFilename(), which would mask a broken guard by never
producing an escaping path), and safeFilenameStripsPathComponents, testing
the sanitiser that actually stops traversal today.
|
|
|
|
Accounts use [account.work] rather than [account/work]: QSettings' INI
backend treats "/" as its own hierarchical group separator, so a literal
slash in a section header parses as a nested group and trips
QSettings::FormatError, silently breaking childGroups() enumeration. A
dot carries no such meaning and keeps the format flat.
Saved-query order is alphabetical (QSettings::childKeys() sorts), not
file order; documented in code and tests rather than left to a false
assumption.
|
|
loadOverrides() inserted straight into m_bindings, so two override
lines that normalize to the same QKeySequence (e.g. "y" and "Y", both
"Y" per QKeySequence) silently overwrote each other with zero warning,
contradicting the "a typo cannot bind silently" contract on
knownActions().
Track sequences seen within the current override pass separately from
m_bindings (which already holds the defaults) so overriding a default
key stays silent, but two colliding override lines produce one warning
naming both actions.
|
|
Maps key sequences to action name strings, with hardcoded vim-style
defaults and QSettings-based [keys] overrides. Unknown actions and
unparseable sequences are collected as warnings rather than treated
as fatal, so a typo in the config cannot silently misbind or crash.
Note: QKeySequence::fromString() on Qt 6.11 does not return an empty
sequence for unparseable input (e.g. "NotAKey++") -- it returns a
non-empty sequence whose toString() is empty. Detection uses that
instead of isEmpty().
|
|
|
