diff options
| -rw-r--r-- | CHANGELOG.md | 40 | ||||
| -rw-r--r-- | CLAUDE.md | 121 | ||||
| -rw-r--r-- | CMakeLists.txt | 39 | ||||
| -rwxr-xr-x | assets/hooks/post-new | 125 | ||||
| -rwxr-xr-x | assets/hooks/test_post_new.py | 92 | ||||
| -rw-r--r-- | cmake/BuildNumber.cmake | 38 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 549 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 181 | ||||
| -rw-r--r-- | src/CMakeLists.txt | 6 | ||||
| -rw-r--r-- | src/keymap.cpp | 12 | ||||
| -rw-r--r-- | src/main.cpp | 4 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 289 | ||||
| -rw-r--r-- | src/mainwindow.h | 47 | ||||
| -rw-r--r-- | src/messageview.cpp | 2 | ||||
| -rw-r--r-- | src/notmuchworker.cpp | 101 | ||||
| -rw-r--r-- | src/notmuchworker.h | 26 | ||||
| -rw-r--r-- | src/version.h.in | 23 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 459 | ||||
| -rw-r--r-- | tests/test_notmuchworker.cpp | 131 | ||||
| -rw-r--r-- | translations/qtmaildir_it_IT.ts | 86 |
20 files changed, 2102 insertions, 269 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 3dcc0a4..9588017 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,46 @@ point at which they are stable. already there and is now findable: Send and Close under File, the editor's undo and clipboard actions under Edit, and the formatting buttons, Attach, Send as HTML and the signature switch under Format. +- **Empty trash**, under Message. It permanently deletes every message in the + trash of the selected account, or of every account in the All accounts view. + This is the one action in qtmaildir that cannot be undone, so it is also the + one that asks first, naming how many messages it will destroy and where; + Cancel is the default and it has no keyboard shortcut. The files are removed + locally, and a channel configured with `Expunge Both` carries that to the + server on the next sync. +- **A build number on unreleased builds.** A build of an unreleased version + now reports `X.Y.Z build N`, so one binary can be told from the one it + replaced; a release reports a plain `X.Y.Z`. Shown by `--version`, `--help`, + the About dialog and the placeholder pane, and deliberately not in the + window title. + +### Changed + +- **The unread action says which way it will go.** "Toggle unread" read the + same whichever direction it would take; it is now "Mark as read" on unread + mail and "Mark as unread" on read mail, and it is hidden entirely when the + selection holds both, where no label would be true. +- **Marking a whole thread read or unread is now two entries**, both under + Whole thread, replacing the single toggle. A thread's unread state is the + union over its messages, so a thread with one unread reply always answered + "unread" and the toggle could only ever mark it read. Neither entry carries + a shortcut, and `Ctrl+Alt+U` is now unbound. +- **Delete and Restore appear only where they apply.** Delete is hidden on + mail already in the trash, where it reported success and did nothing, and + Restore is hidden on mail that was never deleted. +- **Deleting a message also marks it read.** Mail you threw away no longer + counts towards unread. Undo returns both the folder and the tag. + +### Fixed + +- **New mail reached the index but not the window.** The worker never reopened + its read-only notmuch handle, so nothing indexed after startup appeared in + any query and the application looked like it had stopped syncing. +- **Mail sent to another of your own accounts lost `inbox`** and was missing + from the account that received it. notmuch stores one message with two files + in that case, the sender's copy and the recipient's, and the `post-new` + hook's sent-mail carve-out matched the sent copy and stripped the tag from + both. ## [0.27.0] - 2026-08-25 @@ -233,10 +233,20 @@ process-wide, so holding it open would block the user's cron `notmuch new`. `app closes the read-only handle, opens read-write, applies, closes. notmuch permits only one open handle per process, so that close-first ordering is required, not stylistic. -**No dry-run, no destructive-action confirmation.** Those gates exist in the companion -project `../mailctl` to restrain an agent; a human at a GUI gets **undo** instead — every +**No dry-run, no destructive-action confirmation.** Those gates belonged to the retired +`mailctl` CLI, where they restrained an agent; a human at a GUI gets **undo** instead — every mutation pushes its inverse (`TagChange::inverted()`) onto a `QUndoStack`. Do not add -confirmation dialogs for tag mutations. All actions funnel through one `applyTags` path; +confirmation dialogs for tag mutations. + +**There is exactly ONE exception, and its shape is the rule's own logic rather +than a hole in it.** `empty_trash` (item 118) destroys files and index entries, +so it has no inverse to push, and the protection the rule actually provides — +that a user never loses work to a keystroke — has to come from somewhere else. +It therefore asks, naming the count and the account, defaulting to Cancel, and +it carries **no default shortcut** for the same reason. `NotmuchWorker::purgeMessages()` +is a separate entry point from `moveMessages()` deliberately: the two look +alike and only one of them can be undone. A second confirmation anywhere is a +defect unless the action is likewise irreversible. All actions funnel through one `applyTags` path; multi-row selections go through `applyTagsToThreads`, which resolves every thread in ONE combined `thread:a or thread:b` query rather than one query per thread. @@ -295,9 +305,9 @@ irreversibly. A restore must be right about its destination or it is worse than doing nothing. **The sync script lives here, in `assets/mailsync.sh`.** It moved from the -companion `mailctl` project, which documents that it never calls it: the script -is `mbsync` plus `notmuch new` with a lock, and qtmaildir is the only thing that -runs it programmatically. Two properties exist for this application's sake and +retired `mailctl` project, which never called it: the script is `mbsync` plus +`notmuch new` with a lock, and qtmaildir is the only thing that runs it +programmatically. Two properties exist for this application's sake and must survive any edit. It **prints to stdout as well as its log file**, because `MailSync` shows what the command prints and a self-redirecting script leaves the pane empty; and it **exits with the real status**, because a `0` from a @@ -357,18 +367,19 @@ asserts on is guarded by `!m_savedQueries.isEmpty()`, so a test with no `queries.json` never reaches the branch and passes against a broken check. It writes one, and asserts the file loaded before asserting on what it produced. -**This application has a sibling, and one file couples them.** `mailctl` -(`../mailctl`) is a narrow, agent-safe CLI over the same notmuch index. The two -are independent except for `~/.config/mailrules/rules.json`, which both read and -write. **Before changing anything about that file's format, read -"Changing the shared rule format" at the bottom of this document.** Nothing else -here can break mailctl: it never imports from this repo, and this repo never -calls it. - -**The auto-tagging rules are NOT in this repo, and notmuch's parser rejects -almost nothing.** Rules live in `~/.config/mailrules/rules.json`, applied by a -notmuch `post-new` hook that ships from the companion `mailctl` project; -`TagRules` here reads and writes the same file and `TagRulesDialog` edits it. +**One config file has two readers, and both are now in this repo.** +`~/.config/mailrules/rules.json` is read and written by `src/tagrules.cpp` and +by `assets/hooks/mailrules.py`, which share no code and agree by test. +**Before changing anything about that file's format, read "Changing the rule +format" at the bottom of this document.** It used to be a cross-repo coupling +with the `mailctl` CLI; that project is retired and the hooks moved here on +2026-08-23, so a format change is now one repo and two suites. + +**The auto-tagging rules live in a config file, not in the source, and notmuch's +parser rejects almost nothing.** Rules are in `~/.config/mailrules/rules.json`, +applied by the notmuch `post-new` hook in `assets/hooks/`, which the live +`database.hook_dir` symlinks to; `TagRules` here reads and writes the same file +and `TagRulesDialog` edits it. Two things bite. A stored query carries NO scope: the hook supplies `tag:new` and wraps the query in parentheses, because `tag:new and a or b` binds as `(tag:new and a) or b` and a rule that is a disjunction of senders would escape @@ -885,12 +896,12 @@ backlog had already specified and that shipped unbuilt (item 29). A note saying "X does not work" is a bug report, and it will sit in a personal notes file indefinitely unless someone goes looking. -**The backlog covers the mail system, not only this binary.** Item 44 shipped as -commits in BOTH this repo and `../mailctl`, and any future item touching the -shared rule format will too. An item is not "not ours" because its work lands in -the sibling repo; note where the work goes in the table's Note column. mailctl -keeps its own `TODO.md` for things that are purely its own, and that file is not -part of this reconciliation. +**The backlog covers the mail system, not only this binary.** An item can land +in `assets/hooks/` rather than in `src/`, and item 166 is one: the tagging hook +is part of the mail system the user sees, so a defect there gets an item here +like any other. Item 44 predates that and shipped as commits in this repo and in +the retired `mailctl`, which is why older entries mention a sibling repo; there +is no longer one to split work across. **Then print the open items as a table, and stop.** The user picks what to work on; do not start on one, and do not recommend a single item as though the choice @@ -962,7 +973,13 @@ because the first four steps were treated as the whole job. a user's own config or habits need to change. 2. Bump `project(qtmaildir VERSION ...)` in `CMakeLists.txt`, the only place the version lives. Reconfigure, build, and check `./build/src/qtmaildir - --version`. + --version`. An ordinary dev build answers `X.Y.Z build N`, because + `QTMAILDIR_BUILD_NUMBER` counts rebuilds so one binary of an unreleased + version can be told from another (item 167). That is the DISPLAY version; + what a release ships is the clean one, from + `-DQTMAILDIR_BUILD_NUMBER=OFF`, which is what the SlackBuild configures and + what a tarball with no build directory produces anyway. Check the clean + form before tagging. 3. Commit as `release: X.Y.Z`, then `git tag -s vX.Y.Z -m "qtmaildir X.Y.Z"`. Tags are annotated and GPG-signed, matching every existing one. 4. `git push && git push --tags`. `origin` carries two push URLs, the personal @@ -993,39 +1010,41 @@ was: bumping it is a task in that repo, which has its own workflow in its tracks upstream through an nvchecker stanza there, so a release here is picked up by that repo's own sweep. -## Changing the shared rule format +## Changing the rule format -`~/.config/mailrules/rules.json` has **two independent implementations**, and -they agree by test rather than by sharing code: +`~/.config/mailrules/rules.json` has **two independent implementations**, both +in this repo, and they agree by test rather than by sharing code: | | reads/writes | applies rules | |---|---|---| -| `src/tagrules.cpp` (here) | yes | no | -| `mailrules.py` (`../mailctl`) | yes | via the `post-new` hook | - -**This is the only way work here can break mailctl.** It never imports from this -repo and this repo never calls it, so nothing else is shared. The file is -deliberately owned by neither: both readers preserve fields they do not -understand (`TagRule::unknown`, `Rule.unknown`), which is what lets one tool -save a file the other wrote without stripping it. - -**A format change is therefore a two-repo change, and the live hook runs every -ten minutes on real mail.** Before touching the schema: - -1. Change both readers, not one. A field added here and not there is silently - dropped on the next save from the other side, which looks like data loss with - no error anywhere. +| `src/tagrules.cpp` | yes | no | +| `assets/hooks/mailrules.py` | yes | via the `post-new` hook | + +They are two languages either side of one file, so nothing but the format +couples them. Both readers preserve fields they do not understand +(`TagRule::unknown`, `Rule.unknown`), which is what lets one save a file the +other wrote without stripping it. This was a cross-repo coupling with the +`mailctl` CLI until that project was retired and the hooks moved here on +2026-08-23; the discipline below survives the move because the two readers do. + +**The live hook runs every ten minutes on real mail.** Before touching the +schema: + +1. Change both readers, not one. A field added on one side and not the other is + silently dropped on the next save from the other, which looks like data loss + with no error anywhere. 2. Bump `kFormatVersion` / `FORMAT_VERSION` together only for a BREAKING change. Both readers refuse a file whose version they do not know, which is the - correct behaviour and also means a half-deployed bump stops the hook from + correct behaviour and also means a half-applied bump stops the hook from tagging. Adding an optional field needs no bump. -3. Run both suites: `ctest --test-dir build -R tagrules` here, and - `./test_mailrules.py && ./test_post_new.py` there. -4. Verify the round trip across tools by hand, since no automated test spans - both repos: save from the dialog, then `mailctl rules list`, and confirm the - rule count and a note survive. - -**Two hook properties are safety-critical and are not this repo's to weaken.** +3. Run both suites: `ctest --test-dir build -R tagrules`, and + `./test_post_new.py && ./test_mailrules.py` from `assets/hooks/`. +4. Verify the round trip by hand, since no automated test spans the C++ and the + Python: save from the dialog, then run the hook over a throwaway index, and + confirm the rule count and a note survive. + +**Two hook properties are safety-critical, and being ours now is not a reason to +weaken them.** The hook refuses to remove `unread` or `inbox` (`maildir.synchronize_flags` is true, so removing `unread` rewrites Maildir filenames and reaches the server), and it does not consume the `tag:new` marker when the rules fail to load diff --git a/CMakeLists.txt b/CMakeLists.txt index 9b4426c..e3cf82b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -11,6 +11,12 @@ set(CMAKE_AUTORCC ON) # that ships. option(QTMAILDIR_BUILD_TESTS "Build the test suite" ON) +# A dev build counts its own rebuilds so one binary of an unreleased X.Y.Z can +# be told from another (item 167). OFF for a release: the version a release +# prints must be a clean X.Y.Z, which the release procedure checks and the +# SlackBuild builds from a tarball that carries no counter at all. +option(QTMAILDIR_BUILD_NUMBER "Number each build of a dev tree" ON) + set(QTMAILDIR_QT_COMPONENTS Widgets Svg WebEngineWidgets) if(QTMAILDIR_BUILD_TESTS) list(APPEND QTMAILDIR_QT_COMPONENTS Test) @@ -58,6 +64,39 @@ configure_file( ${CMAKE_CURRENT_BINARY_DIR}/generated/qtmaildir/version.h @ONLY) +# buildnumber.h is generated separately, and by a BUILD step rather than by +# configure_file: this whole block runs once per cmake run, so a counter +# written here would not move when the user rebuilt after a fix, which is the +# defect item 167 describes. The header always exists so version.h can include +# it unconditionally; in a release build it is empty and +# QTMAILDIR_VERSION_DISPLAY collapses to the plain version. +set(QTMAILDIR_BUILD_NUMBER_HEADER + ${CMAKE_CURRENT_BINARY_DIR}/generated/qtmaildir/buildnumber.h) + +if(QTMAILDIR_BUILD_NUMBER) + # Once now, so the header exists before the first compile of a fresh build + # directory, and then again on every build. + execute_process(COMMAND ${CMAKE_COMMAND} + -DCOUNTER_FILE=${CMAKE_CURRENT_BINARY_DIR}/build-number + -DHEADER_FILE=${QTMAILDIR_BUILD_NUMBER_HEADER} + -P ${CMAKE_CURRENT_SOURCE_DIR}/cmake/BuildNumber.cmake) + + add_custom_target(qtmaildir_buildnumber ALL + COMMAND ${CMAKE_COMMAND} + -DCOUNTER_FILE=${CMAKE_CURRENT_BINARY_DIR}/build-number + -DHEADER_FILE=${QTMAILDIR_BUILD_NUMBER_HEADER} + -P ${CMAKE_CURRENT_SOURCE_DIR}/cmake/BuildNumber.cmake + BYPRODUCTS ${QTMAILDIR_BUILD_NUMBER_HEADER} + COMMENT "Numbering this build" + VERBATIM) +else() + file(WRITE ${QTMAILDIR_BUILD_NUMBER_HEADER} + "// Release build: no counter, so QTMAILDIR_VERSION_DISPLAY is the +// plain version. Generated by the top-level CMakeLists.txt. +#pragma once +") +endif() + add_subdirectory(src) if(QTMAILDIR_BUILD_TESTS) enable_testing() diff --git a/assets/hooks/post-new b/assets/hooks/post-new index 428ec29..fca31a1 100755 --- a/assets/hooks/post-new +++ b/assets/hooks/post-new @@ -73,6 +73,109 @@ def log(message): print(f"post-new: {message}", file=sys.stderr) +def _quote(value): + """Escape a Message-ID for a double-quoted notmuch term.""" + return '"' + value.replace("\\", "\\\\").replace('"', '\\"') + '"' + + +def sent_only(query, folders): + """The ids matching `query` whose files are ALL inside `folders`. + + None on failure, so the caller leaves `tag:new` in place rather than + stripping on a half-read answer. + + **Every file has to be in a sent folder, not merely one of them.** notmuch + deduplicates by Message-ID, so mail the user sends to another of their own + accounts is ONE message with two files: the sender's Sent copy and the + recipient's Inbox copy. The old query matched on a path and the tag applied + to the message, so matching the Sent copy stripped `inbox` from the copy + that had genuinely arrived, and the mail was missing from the account that + received it (item 166). + + This cannot be expressed as a query, which is why it is a loop. Measured + against a two-file message: `not path:"Inbox/**"` does NOT exclude it, and + `count --output=files` on a path query reports every file of every matching + message rather than the files that matched. Both read as if they worked and + are wrong for the same reason, that a notmuch term is a predicate over a + MESSAGE and the distinction being drawn here is between its FILES. + """ + root = mail_root() + if root is None: + log("no mail root; leaving sent mail alone") + return None + + prefixes = [root / folder for folder in folders] + + matched = search(query, "messages") + if matched is None: + return None + + # ponytail: one `notmuch search` per matched message. N is the sent mail + # in tag:new, so an ordinary sync is a handful and a first-run reindex is + # the whole corpus. Batch by parsing --format=json once if that ever + # matters; it does not at this size, and the loop is the readable form. + ids = [] + for message_id in matched: + paths = search(f"id:{_quote(message_id)}", "files") + if paths is None: + return None + if all(any(_within(Path(path), prefix) for prefix in prefixes) + for path in paths): + ids.append(message_id) + return ids + + +def _within(path, prefix): + """Whether `path` is inside `prefix`, compared as paths. + + Not `startswith`: `<root>/Sent-old/cur/1` starts with `<root>/Sent` and is + a different folder. Same trap as the attachment-save path check in the + application. + """ + try: + path.relative_to(prefix) + except ValueError: + return False + return True + + +def mail_root(): + """The Maildir root, or None. + + `database.mail_root`, not `database.path`: notmuch can hold the Xapian + index somewhere else entirely, and this user's does. Under that layout + `database.path` is the INDEX directory and no message file is inside it. + """ + result = subprocess.run(["notmuch", "config", "get", "database.mail_root"], + capture_output=True, text=True) + if result.returncode != 0: + return None + value = result.stdout.strip() + return Path(value) if value else None + + +def search(query, output): + """`notmuch search --output=<output>` as a list, or None on failure. + + An id comes back bare here, without the `id:` prefix, because + `--output=messages` prints `id:<value>` and the prefix is stripped. + """ + result = subprocess.run( + ["notmuch", "search", f"--output={output}", "--", query], + capture_output=True, text=True) + if result.returncode != 0: + log(f"notmuch search failed: {result.stderr.strip()}") + return None + + values = [] + for line in result.stdout.splitlines(): + line = line.strip() + if not line: + continue + values.append(line[3:] if line.startswith("id:") else line) + return values + + def strip_inbox_from_sent(run): """Take `inbox` off mail the user SENT, and nothing else. @@ -105,20 +208,20 @@ def strip_inbox_from_sent(run): query = f"{SCOPE} and ({qtmaildirconf.sent_query(folders)})" - # Counted BEFORE the tag, because the tag is what makes the count zero. - # A `notmuch tag` that matches nothing SUCCEEDS, so the old log line said - # "applied" whether it stripped four messages or none, and item 164 is - # exactly the case where that distinction is the whole question: a draft - # kept `inbox` on a pass whose log claimed the carve-out had run. The - # count is the only thing that separates "the tag ran and something - # re-added inbox afterwards" from "the message was never in scope". - matched = count(query) - - if not run(["-inbox"], query): + ids = sent_only(query, folders) + if ids is None: return False + for message_id in ids: + if not run(["-inbox"], f"id:{_quote(message_id)}"): + return False + + # A `notmuch tag` matching nothing SUCCEEDS, so the old log line said + # "applied" whether it stripped four messages or none, and item 164 is + # exactly the case where that distinction is the whole question: a draft + # that kept `inbox` on a pass whose log claimed the carve-out had run. log(f"sent-folder carve-out applied over {len(folders)} folder(s), " - f"{matched} message(s)") + f"{len(ids)} message(s)") return True diff --git a/assets/hooks/test_post_new.py b/assets/hooks/test_post_new.py index a0228aa..07728a6 100755 --- a/assets/hooks/test_post_new.py +++ b/assets/hooks/test_post_new.py @@ -38,6 +38,16 @@ from pathlib import Path HOOK = Path(__file__).resolve().parent / "post-new" +def files(env, query): + """How many FILES notmuch holds for the messages matching a query, which + is not the message count: a message sent to another of the user's own + accounts is one message with two files.""" + result = subprocess.run( + ["notmuch", "search", "--output=files", "--", query], + env=env, capture_output=True, text=True, check=True) + return len([line for line in result.stdout.splitlines() if line]) + + def make_message(maildir, name, sender, subject): path = maildir / "new" / name path.write_text( @@ -202,7 +212,7 @@ def test_a_protected_removal_is_skipped_whole_and_the_run_continues(): assert count(env, "tag:new") == 0 -def setup_accounts(tmp, sent_config=True): +def setup_accounts(tmp, sent_config=True, split_index=False): """A maildir laid out as qtmaildir configures it: two accounts, each with an Inbox and a Sent folder, one message in each. @@ -232,10 +242,23 @@ def setup_accounts(tmp, sent_config=True): "you@example.org", "something else sent") config = Path(tmp) / "notmuch-config" - config.write_text( - f"[database]\npath={root}\n\n" - f"[new]\ntags=new;unread;inbox\n\n" - f"[user]\nname=Test\nprimary_email=you@example.org\n") + if split_index: + # The index somewhere else entirely, which is how the developer's own + # machine runs: `database.path` is then the INDEX directory and no + # message file is inside it. Anything reading that key as the mail + # root resolves every path wrongly, and the ordinary layout above + # cannot show it because both keys return the same string. + index = Path(tmp) / "index" + index.mkdir() + config.write_text( + f"[database]\npath={index}\nmail_root={root}\n\n" + f"[new]\ntags=new;unread;inbox\n\n" + f"[user]\nname=Test\nprimary_email=you@example.org\n") + else: + config.write_text( + f"[database]\npath={root}\n\n" + f"[new]\ntags=new;unread;inbox\n\n" + f"[user]\nname=Test\nprimary_email=you@example.org\n") env = dict(os.environ) env["NOTMUCH_CONFIG"] = str(config) @@ -284,6 +307,65 @@ def test_sent_mail_does_not_keep_the_inbox_tag(): assert count(env, 'tag:inbox and path:"acct-two/Inbox/**"') == 1 +def test_a_message_that_was_sent_AND_received_keeps_inbox(): + """Item 166. notmuch deduplicates by Message-ID, so mail the user sends + to their own other account is ONE message with TWO files: the sender's + Sent copy and the recipient's Inbox copy. + + The carve-out matches on a file's path but tags the MESSAGE, so matching + the Sent copy stripped `inbox` from the Inbox copy as well and the mail + vanished from the account that genuinely received it. The predicate has + to hold for every file, not for any file. + + The identical `name` is what makes this one message: make_message() + derives the Message-Id from it. + """ + with tempfile.TemporaryDirectory() as tmp: + env = setup_accounts(tmp) + root = Path(tmp) / "Mail" + make_message(root / "acct-one/Sent", "self-addressed", + "you@example.org", "to my other account") + make_message(root / "acct-two/Inbox", "self-addressed", + "you@example.org", "to my other account") + subprocess.run(["notmuch", "new"], env=env, capture_output=True, + check=True) + # One message, two files: the premise of the whole defect. + assert count(env, "id:self-addressed@example.org") == 1 + assert files(env, "id:self-addressed@example.org") == 2 + + write_rules(env, []) + result = subprocess.run([str(HOOK)], env=env, capture_output=True, + text=True) + assert result.returncode == 0, result.stderr + + # It arrived, so it keeps `inbox`... + assert count(env, "tag:inbox and id:self-addressed@example.org") == 1 + # ...and the ordinary sent message, whose only file is in a sent + # folder, still loses it. Without this half the fix could simply be + # "never strip anything". + assert count(env, "tag:inbox and id:sent-one@example.org") == 0 + + +def test_the_carve_out_works_with_the_index_split_from_the_mail(): + """notmuch can hold the Xapian index outside the Maildir, and this user's + does. `database.path` is then the index directory, so a carve-out reading + that key as the mail root builds prefixes no message file is under, finds + every file "outside" the sent folders, and silently stops stripping + anything. + + The ordinary fixture cannot catch it: with the index inside the mail root + both keys return the same string and either one passes. + """ + with tempfile.TemporaryDirectory() as tmp: + env = setup_accounts(tmp, split_index=True) + write_rules(env, []) + result = subprocess.run([str(HOOK)], env=env, capture_output=True, + text=True) + assert result.returncode == 0, result.stderr + assert count(env, 'tag:inbox and path:"acct-one/Sent/**"') == 0 + assert count(env, 'tag:inbox and path:"acct-one/Inbox/**"') == 1 + + def test_sent_mail_keeps_every_other_tag(): """Only `inbox` is stripped. `unread` in particular must survive: maildir.synchronize_flags is true, so removing it rewrites Maildir diff --git a/cmake/BuildNumber.cmake b/cmake/BuildNumber.cmake new file mode 100644 index 0000000..a79f1e7 --- /dev/null +++ b/cmake/BuildNumber.cmake @@ -0,0 +1,38 @@ +# Increment the build counter and write buildnumber.h. +# +# Run with `cmake -P` as a build step, so it fires on every build rather than +# once per configure. Item 167: the version alone cannot tell one build of an +# unreleased X.Y.Z from another, and the user hand-tests unreleased builds +# daily. +# +# Expects COUNTER_FILE and HEADER_FILE on the command line. +# +# The counter lives in the BUILD directory and is deliberately not tracked: a +# committed counter would conflict on every pull and leave the tree dirty +# after every build. A fresh build directory therefore restarts at 1, which is +# honest, since it is a different build tree. + +if(EXISTS "${COUNTER_FILE}") + file(READ "${COUNTER_FILE}" current) + string(STRIP "${current}" current) +endif() + +if(NOT current MATCHES "^[0-9]+$") + set(current 0) +endif() + +math(EXPR next "${current} + 1") +file(WRITE "${COUNTER_FILE}" "${next}\n") + +# Written to a temporary and copied only if different, so a rebuild that +# changes nothing else does not force every translation unit including +# version.h to recompile... except that the number itself changes every time, +# so it always differs. That is the accepted cost of the feature: the few +# files that read the version are recompiled and relinked on every build. +file(WRITE "${HEADER_FILE}.tmp" +"// Generated by cmake/BuildNumber.cmake on every build. Do not edit, and do +// not commit: this file lives in the build directory. +#pragma once +#define QTMAILDIR_BUILD_NUMBER \"${next}\" +") +file(COPY_FILE "${HEADER_FILE}.tmp" "${HEADER_FILE}" ONLY_IF_DIFFERENT) diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index f2b977b..1208c42 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md @@ -7942,3 +7942,552 @@ what turns a stale path into two server-side MESSAGES rather than one replaced file. That wants its own item. A draft's id is not yet the sent message's id, so changing it is not obviously free. + +## 104. Mail visible in Thunderbird never reaches qtmaildir + +**Observed (user, from the notes):** "sync doesn't work compared to thunderbird. +New mail received on thunderbird did not appear in qtmaildir. Need to investigate +further." + +**Cause: ESTABLISHED 2026-08-25, and it was this repository after all.** This +entry previously named mbsync's folder `Patterns` as the leading theory and +concluded "most likely not a code change here at all". That was wrong; the +superseded reasoning is kept at the bottom because the reproduction is what +overturned it. + +**`NotmuchWorker` opened one read-only notmuch handle and kept it for the +process lifetime.** A read-only handle is a Xapian SNAPSHOT taken when it is +opened; it never observes a write made by another process afterwards. The sync +script's `notmuch new` is exactly such a process, so every query the worker +answered after startup was served from the index as it stood when the +application launched. `openReadOnly()` returned early on `if (m_db) return +true;` and there was no `notmuch_database_reopen` anywhere in the tree. + +This accounts for every symptom, including the ones that defeated three earlier +theories during the diagnosis: + +- The post-sync refresh found nothing, so `refreshCurrentQuery()` and + `ThreadListModel::reconcile()` were each suspected in turn. Both are correct. +- A query the user typed BY HAND also found nothing. That is what rules out the + model, the generation counter and the account scope together: a fresh query + clears the model and re-runs from scratch, and it still hits the same stale + handle. +- A restart showed the mail instantly, with no sync in between. +- Tag WRITES were never affected, which is why the defect reads as "reading is + broken" rather than "notmuch is broken". `applyTags` opens its own read-write + handle per call, so it always sees current data. + +**Why it survived from 2026-08-16 to 2026-08-25.** The symptom needs mail to +arrive from outside the process while the window stays open, which is the +ordinary way this application is used and the one thing no test did: every +fixture opens a worker, queries it, and drops it. `TestNotmuchWorker::runQuery()` +builds a FRESH worker per call, so the suite was structurally incapable of +reproducing it, and a test written through that helper passes against the bug. + +**Fixed** in `NotmuchWorker::openReadOnly()`: when a handle already exists, +`notmuch_database_reopen(m_db, NOTMUCH_DATABASE_MODE_READ_ONLY)` before +returning it. Every read path begins by asking for the handle, so one call +covers all of them; putting it at the call sites instead would be one more +place to forget. A reopen failure is deliberately NOT fatal, since the existing +handle is still usable and answering from a slightly stale index beats refusing +to answer at all. + +Covered by `aQuerySeesMailIndexedAfterTheWorkerOpened`, which holds ONE worker +across two queries and runs `notmuch new` in a second process between them. +Mutation-checked: `after.size()` is 0 without the fix, 1 with it. The first +query asserts zero results before the message is written, so "found nothing" +cannot mean "the query was malformed". + +**The reproduction, kept because this entry's Approach section asked for exactly +this and it took three wrong turns to get there.** Four messages sent to one +account on 2026-08-25, viewed in that account's Inbox, synced with the app's own +Sync button. The three layers resolved as: on disk (yes), indexed (yes), shown +(no), which is layer 3 and therefore this repository. Two of the four matched +the running view's exact query (`path:"<account>/**" and (tag:inbox)`, 2 results +from the shell) and were absent from a window that had been open across the +sync. + +Two measurement errors made during that diagnosis, both worth repeating because +each produced a confident wrong answer: + +- `notmuch count 'inbox and path:...'` was used to check the view's contents. A + bare `inbox` is a FREE-TEXT term, not a tag term; the app generates + `tag:inbox`. The bare form returned 0 where the real query returns 2, which + briefly made the defect look like a tagging problem. +- The messages' tags were first read across every file matching the subject, + including the sender-side Sent copies in other accounts. That mixed three + accounts' messages into one answer. + +**Superseded theory, kept for the record.** mbsync fetches Gmail folders by +pattern and three of the five channels name their folders explicitly, so a +message labelled anything else is in a folder mbsync never asks for while +Thunderbird, speaking IMAP directly, sees it. That mechanism is real and would +produce a similar symptom, but it is not what was happening here: the mail was +on disk and indexed. It remains a plausible cause of any FUTURE report of this +shape, so check layer 1 before assuming this fix covers it. + +**One inconsistency worth reporting regardless**, found while checking the above +and still true: one of the Gmail accounts is configured in `qtmaildir.conf` with +a sent and a drafts folder, while its mbsync channel has `Patterns "INBOX"` and +fetches neither. The Sent and Drafts filters for that account can therefore only +ever be empty. That is real, independent of this item, and outside this +repository. + +**Size: XS.** Done. + +## 167. No way to tell one build of an unreleased version from another + +**Observed (user, from the notes):** "we should add a dev build number to be +pushed everytime we rebuild after a fix, so that I can verify if I'm in the +correct app version." The note has sat unrecorded through several sessions; +the 2026-08-25 reconciliation is the first to pick it up. + +**Cause (verified in the code, 2026-08-25.)** The version lives in exactly one +place, `project(qtmaildir VERSION ...)`, and `src/version.h.in` interpolates +`@PROJECT_VERSION@` and nothing else. That is correct for a release and says +nothing between two of them: the string moves only when the release procedure +bumps it, so every rebuild of `0.27.0` reports `0.27.0`. The status table above +shows why it bites in practice, since most closed items since 0.27.0 read +"unreleased" and the user hand-tests each one against a binary they rebuilt +themselves. + +Both surfaces that show the version take it from the same macro, so whatever is +added reaches them at once: the window title (`mainwindow.cpp:939`), the About +dialog (`mainwindow.cpp:2449`), the placeholder pane (`messageview.cpp:547`), +`--version` and `--help` (`main.cpp`). + +**Approach.** Needs a DECISION before any code, because the two candidates fail +in opposite directions. + +A git description (`git describe --always --dirty`, or the short hash) is +accurate and self-explaining: it names the commit the binary was built from, and +a reviewer can check out exactly that. Its cost is that CMake computes it at +CONFIGURE time, so a build after a new commit reports the previous hash unless +the configure step is made to re-run, which is a custom command with a dependency +on `.git/HEAD` and the packed refs, and is the part that usually ships subtly +wrong. + +A monotonic counter always moves and needs no git, but it means nothing on its +own: build 412 does not say which fix is in it, and it differs between the user's +machine and any other, so it cannot be quoted in a report. + +**Constraints.** A release build must keep printing a clean `X.Y.Z`, since the +SlackBuild in the `my-slackbuilds` repo builds from the release tarball where +there is no git checkout at all, and the release procedure checks +`./build/src/qtmaildir --version`. Whatever is added is therefore an addition to +the string in a dev build and absent in a release one, not a change to the +version itself. + +**Decision (user, 2026-08-25): the counter.** The git description was +offered as the recommendation and was not chosen; what the user wants is to +know a rebuild happened, not which commit it was. + +**Built 2026-08-25, unreleased.** `QTMAILDIR_BUILD_NUMBER`, a cmake option ON +by default, runs `cmake/BuildNumber.cmake` as a build step: it increments a +counter and writes `buildnumber.h`, which `version.h` includes. +`QTMAILDIR_VERSION_DISPLAY` is `X.Y.Z build N` when that macro is defined and +plain `X.Y.Z` when it is not. + +Two macros, not one, and the split is the load-bearing part. +`QTMAILDIR_VERSION` stays clean and keeps the window title, `applicationVersion` +and anything that might ever compare versions; `QTMAILDIR_VERSION_DISPLAY` goes +to the three surfaces the user picked: `--version`, `--help`, the About dialog +and the placeholder pane. The window title was offered and declined, since the +number would then sit in every screenshot. + +**The counter had to be a BUILD step, not `configure_file`.** That is the whole +reason this is not two lines: `configure_file` runs once per cmake run, so a +counter interpolated into `version.h.in` sits still across exactly the rebuilds +this item exists to distinguish. `version.h.in` therefore includes a second +generated header rather than carrying the number itself. + +The counter file lives in the build directory and is not tracked, so it cannot +conflict on a pull or dirty the tree; a fresh build directory restarts at 1, +which is honest, because it is a different build tree. A release build passes +`-DQTMAILDIR_BUILD_NUMBER=OFF` and the header is written empty. + +**Verified by running it**, since none of this is reachable from a C++ test: +three consecutive builds reported `build 2`, `build 3`, `build 4`, and a +separate Release configure with the option OFF reported a clean `0.27.0`. The +suite is 37 of 38, the one failure being item 136 on an unrelated path. + +**Size: XS**, as sized. + +## 166. Mail you send to your own other account loses `inbox` + +**Observed (agent, 2026-08-25, while setting up msmtp.)** Four test messages +were sent to one of the user's own accounts, one from each configured sending +account. All four were delivered and indexed. The two sent from accounts whose +Sent folder is fetched locally arrived in the recipient account's Inbox +**without the `inbox` tag**, so they were absent from that account's Inbox view. +The two sent from an account whose Sent folder is not fetched kept `inbox` +normally. + +**Cause: established, and it is the `post-new` hook, not this binary.** +`strip_inbox_from_sent()` in `assets/hooks/post-new` removes `inbox` from any +message matching a configured sent folder's PATH. Its docstring states the +assumption exactly: "the provenance is the file's own path: a message inside a +configured sent folder is one this system sent, and `inbox` was never true of +it." + +That holds for one file. It fails for one MESSAGE, because **notmuch +deduplicates by Message-ID and a message can have several files**. When the +sender and the recipient are both the user's own accounts, mbsync fetches two +copies: the sender's Sent copy and the recipient's Inbox copy. notmuch stores +them as ONE message with two filenames. The carve-out's query matches via the +Sent filename and strips `inbox` from the message object, which is the same +object the recipient's Inbox copy belongs to. + +Measured: one message, two paths, one in the sender account's sent folder and +one in the recipient account's `Inbox/cur`. + +The assumption is not merely incomplete, it is false in this case: the message +was genuinely sent AND genuinely received. There is no single right answer for +"was `inbox` ever true of this message", because it was true of one file and +false of another. + +**Approach.** Not settled, and the choice matters more than the code: + +1. **Strip only when EVERY file is in a sent folder.** Closest to the existing + intent, and it makes the predicate match the docstring's claim. A + self-addressed message keeps `inbox`, which is right: it did arrive. +2. **Strip only when the message has exactly one file.** Simpler to express, + but it silently stops protecting any sent message that happens to be + duplicated for an unrelated reason. +3. **Leave it.** Self-addressed mail is rare outside testing. The cost is that + it is invisible when it happens, and it looks exactly like the sync defect + item 104 turned out to be, which is how this was found. + +Option 1 is the one that makes the code true to what it already says it does. + +**Constraints.** + +- **The hook is this repo's**, `assets/hooks/post-new`, which the live + `database.hook_dir` symlinks to. It has its own suites beside it; run + `./test_post_new.py` and `./test_mailrules.py` from `assets/hooks/`. +- The hook **tags real mail unattended, every ten minutes, on the user's live + index.** A predicate that is wrong in the other direction would strip `inbox` + from arriving mail, which is the failure mode PROTECTED_REMOVALS exists to + prevent. Test against a throwaway database first. +- `notmuch tag` matching zero messages SUCCEEDS, so a log line saying the + carve-out ran is not evidence it matched anything. The count added for item + 164 is what distinguishes them; use it. +- Do not fix this by narrowing the query to exclude the recipient account. The + bug is in the per-file predicate, not in which folders are configured. + +**Fixed 2026-08-25, option 1**, the one the entry named: strip only when +every file is in a sent folder. + +`sent_only()` in `assets/hooks/post-new` filters the matches and the tag is +then applied per id. **It is a loop because no query can express it**, and both +plausible query forms were measured against a real two-file message before the +loop was written: `not path:"Inbox/**"` does NOT exclude the message, and +`notmuch count --output=files` on a path query reports every file of every +matching message rather than the files that matched. Both read as if they +worked and are wrong for one reason, that a notmuch term is a predicate over a +MESSAGE while the distinction here is between its FILES. + +The root comes from `database.mail_root`, not `database.path`, since this index +is split and no message file sits under the index directory. The mutation +putting `database.path` back passes every pre-existing test, because the +ordinary fixture keeps the index inside the mail root and both keys return the +same string; `setup_accounts(split_index=True)` is what catches it, and is the +Python counterpart to `NotmuchFixture::splitIndex()`. + +Two mutations fail: `all` to `any` loses `inbox` on the self-addressed message, +`mail_root` to `path` silently stops stripping anything. + +**Verified read-only against the live index**, tagging nothing: of 807 messages +matching a sent path, 780 are still stripped and 27 are spared, every one of +them two files with one in another account's Inbox. No arrival is affected. + +**Size: S.** Done. + +## 112. Toggle unread on a whole thread cannot reach "all unread" on a partly-read thread + +**Observed (user, 2026-08-17):** clicking a thread root and asking to mark the +whole thread unread does not do it. On a seven-message thread with two unread +replies, the result is that every message is toggled unread **except those +two**, which are left as they were. The user asks for an explicit "mark whole +thread read/unread" rather than a toggle. + +**Cause (verified in code):** the action exists, and its direction is the +defect. `toggle_unread_thread` (`src/mainwindow.cpp:931`, `Ctrl+Alt+U`) chooses +between adding and removing by asking +`everySelectedRowHasTag("unread", TagScope::Thread)`, which reads +`ThreadListModel::threadFor(index).tags`. That is notmuch's **union over the +thread** (`CLAUDE.md`, item 110), so a thread containing even one unread message +answers "unread" and the action picks *Mark thread read*. There is no input a +user can give that reaches *Mark thread unread* on a mixed thread: the only +threads that take that branch are the ones already entirely read, and the only +threads reporting "not unread" are the ones the user does not need the action +for. + +The write itself is absolute and correct. `tagSelected` with `TagScope::Thread` +adds or removes `unread` across every message, so the two unread replies in the +report are not skipped by the write. They are the reason the write ran in the +opposite direction from the one the user wanted. + +**A union is not a state, and a toggle needs a state.** This is the same class +as item 110 and the third time the union has produced a defect. Items 105 and 88 +fixed *which object* a toggle resolved; this one is about a thread having no +single answer to give. `everySelectedRowHasTag` is a two-valued predicate over a +three-valued reality: all read, all unread, or mixed. The mixed case is the one +that has no correct toggle direction, and picking either one silently is what +ships as "the action does the wrong thing". + +**Approach.** The user has already named it: stop toggling at thread scope. + +- Split `toggle_unread_thread` into two explicit actions, **Mark thread read** + and **Mark thread unread**, each with a fixed direction. Both appear in the + "Whole thread" submenu, where an entry always carries text, so a fixed label + is honest in a way a toggle's cannot be. +- The message-scoped `toggle_unread` stays a toggle. One message has a real + two-valued state, so the trap does not exist there. Do not "unify" the two: + the asymmetry is the point. + +**Constraints.** + +- **Adding an action is four places**, all enforced by tests that fail + confusingly: `KeyMap::knownActions()`, `defaultBindings()`, the icon table, + and the no-duplicate-icons exception list. See `CLAUDE.md`. Splitting one + action into two means one new entry in each, and the pair shares the twin's + icon under the existing named exemption for thread actions. +- **`Ctrl+Alt+U` is taken by the action being split**, and the whole-thread + bindings are already one modifier out from their twins because `Ctrl+Shift+U` + was claimed. Two directions need two sequences; if a second chord cannot be + found that is not worse than the menu, bind one and leave the other to the + submenu rather than inventing a three-modifier chord nobody will press. +- **This interacts with items 98 and 99**, which is the reason to decide all + three together. 99 asks for a dynamic label on the message-scoped toggle, + which is the opposite move: keep the toggle, make the label tell the truth. + A thread cannot do that, because on a mixed thread there is no true label to + show. Deciding 99 first will produce the wrong answer here by analogy. +- The undo entry must name the direction that ran (`Mark thread unread`), not + the action. `tagSelected` already takes the text, so this comes free from + splitting. +- **The test needs a MIXED thread**, which is the whole defect: a thread whose + messages are all in one state answers identically whichever way the direction + is computed, so a fixture built from a uniformly-unread thread passes against + the bug. Same trap as item 88's opposite-states requirement, recorded in + `CLAUDE.md`. + +**Built 2026-08-25 to the USER'S NOTE, not to the approach above**, which had +this half right and was shipped that way first. The approach proposed splitting +the thread toggle and explicitly said to leave the message-scoped one alone, +deciding 99 separately. The user's note is ONE design across both, and the +entry's own constraint said so ("this interacts with items 98 and 99, which is +the reason to decide all three together") without following it. The half-built +version was handed over, corrected by the user, and rebuilt. + +Four parts, all of them the note's: + +- The thread toggle splits into `mark_thread_read` and `mark_thread_unread`, + both absolute. **Neither carries a default chord**, at the user's choice: + since item 132 a shortcut is a chosen subset, and `Ctrl+Alt+U` meant + whichever direction the union happened to pick, which is what made it wrong. + It is now unbound. +- The message-scoped `toggle_unread` STAYS a toggle, because one message has a + real two-valued state, and gains a label naming the direction it will go. +- On a selection with no single state that entry is **hidden**, chosen over + disabled by the user. There is no honest label for a mixed selection, and + the thread submenu is the route the note points at. +- The label follows a WRITE as well as a selection change, keyed on the + model's `dataChanged` rather than on the six call sites that apply an + optimistic update, so a new one cannot forget. Without it, marking the + current row read left the entry offering to do it again. + +`selectionTagPresence()` is the three-valued predicate this needed; +`everySelectedRowHasTag()` now delegates to it and keeps its two-valued +answer, which is all a DIRECTION needs. A label needs the third value, and +asking a two-valued predicate a three-valued question is what this item was. + +**Three mutations fail:** restoring the union predicate reports "wrong +direction on a mixed thread: Mark thread read", which is the user's original +symptom; showing the action on a mixed selection; and dropping the +`dataChanged` refresh. The suite is 37 of 38, the one failure being item 136 on +an unrelated path, and the four new strings are translated with `lrelease` +reporting 0 unfinished. + +**Closes 99 and 147 with it**, which were the same note recorded twice. + +**Size: S.** Done, at roughly twice the entry's scope because the entry's scope +was wrong. + +## 118. No way to empty the trash from inside the app + +**Observed (user, 2026-08-17):** raised while reviewing item 103's spec, as +something that had been forgotten rather than newly noticed: "we could add +'Empty Trash' to the backlog as a future item. I forgot it existed, but I don't +want to squeeze it in this spec." + +**Blocked on 103**, which creates the trash folder this would empty. Until that +ships there is nothing to empty: Delete writes a tag and moves no file, so no +account has a populated trash folder except through another client. + +**Deliberately excluded from 103's spec**, at the user's request and recorded in +its "Out of scope" section. Worth keeping separate for a reason beyond scope +control: emptying the trash is the first action in this application that would +destroy mail with no undo. Every mutation so far is a tag or, after 103, a move, +and both are reversible. A purge is not. + +**Approach, unspecified.** The shape depends on decisions not yet made, and the +spec for 103 answers none of them: + +- **Local or remote.** Deleting the files locally and letting `Expunge Both` + carry it to the server is one thing; asking the provider to empty its own + trash is another, and mbsync offers no verb for the latter. The first is + probably what "Empty Trash" should mean here. +- **Whether the no-confirmation rule survives it.** It does not, on the face of + it. `CLAUDE.md` grants undo in place of confirmation dialogs, and this is the + action where undo cannot exist. That makes it the second item, after 103, that + re-examines the rule rather than assuming it, and unlike 103 it will probably + have to break it. +- **Per-account or all-accounts**, which should follow whatever the Trash filter + does once 103 ships rather than being decided independently. + +**Built 2026-08-25**, unblocked by 103. The three questions the entry left open +were put to the user and answered: + +- **Local, and let the sync carry it.** The files go, and a channel with + `Expunge Both` propagates that to the server. mbsync offers no verb for + asking a provider to empty its own trash, so the alternative was to delete + locally and not care, which brings the mail back on the next sync and reads + as the action having silently failed. +- **It confirms**, naming the count and the account, defaulting to Cancel, with + no default shortcut. CLAUDE.md now records this as the ONE exception to the + no-confirmation rule, in the same paragraph that states the rule, so the next + reader meets both together. +- **Scoped to the account selector**, like every other account-aware surface, + which is what the entry asked for. + +`NotmuchWorker::purgeMessages()` is a separate entry point from +`moveMessages()` rather than a flag on it, because the two look alike and only +one can be undone. It takes named ids only, never a folder sweep, so the blast +radius is what the dialog enumerated and the user confirmed. It deletes EVERY +file of a message: notmuch deduplicates by Message-ID, and leaving one behind +would leave the message alive in the folder the user emptied, which is the same +one-message-many-files property item 166 turned on. + +`resolveQueryMessages()` is a four-line wrapper over the existing private +`resolveQuery()`, so enumerating what is about to be destroyed needed no new +walk. The count in the dialog comes from the DATABASE rather than the model, +which holds whatever the current view is showing and is usually not the trash. + +**A defect surfaced while writing the tests**, and it is the one worth +remembering: the first version counted a message whose file was already gone as +destroyed, so the number reported for an irreversible action overstated it. An +absent file is correctly not an ERROR, since the index can name a path a sync +has removed; the mistake was treating "not an error" as "destroyed". The +mutation that restores it now fails. + +**A second defect was found by the user's own hand test**: the mail was +destroyed correctly and the LIST went on showing it until they re-ran the query +themselves. A purge is the one mutation with no optimistic update available, +because it removes rows rather than changing them, so `messagesPurged` re-runs +the current query. Nothing was connected to that signal at all, which is the +kind of gap a green suite is happy to keep. + +Verified against the live index after the user emptied one real account's +trash: zero files on disk, zero in the index. + +**Item 168 was filed from the same hand test**, on Delete being offered on mail +already in the trash. + +**Size: S.** Done. + +## 168. Delete is offered on mail already in the trash, and does nothing + +**Observed (user, 2026-08-25, while hand-testing item 118):** "I noticed I can +hit delete via context menu on a message already in the trash. Seems like a +bug, unless that action doesn't do for one message what Empty trash does for +the whole view." + +It does not, and the guess in the second half is worth recording as the reason +this matters: the user's mental model was that Delete on already-trashed mail +might PURGE it. It does not, and nothing about the menu says so. + +**Cause (verified in code, 2026-08-25.)** `moveMessages()` compares the file's +directory against the destination and takes an early-return branch when they +match (`notmuchworker.cpp`, the "already where it was asked to go" branch, +added when a fresh Maildir name made a path comparison useless). That branch +appends the id to `moved` and records an origin, so the message is reported as +having moved when nothing happened. The UI counts an unsynced change for it. + +Nothing is destroyed and nothing is corrupted; the cost is a menu entry that +lies about having done something, and a pending-changes count that overstates +what a sync has to carry. + +**The mirror of the same defect is already shipped beside it.** `restore` is +added unconditionally to both the Message menu (`mainwindow.cpp:1956`) and the +thread context menu (`mainwindow.cpp:2119`), so it is offered on mail that was +never deleted, where it has as little meaning as Delete has in the trash. + +**Approach.** The user chose to hide each action where it has no meaning, +which is the principle item 112 established for the unread entry: an action +with no honest meaning for the selection is absent rather than present and +inert. + +- Delete is hidden when every selected row is already in a trash folder. +- Restore is hidden when no selected row is. +- The test for both needs a MIXED selection as well as uniform ones, for the + reason item 112 records: a selection whose rows agree answers identically + whichever way the predicate is computed. + +**Constraints.** + +- **The question is about the PATH, not the tag.** A message trashed by + another client carries no `deleted` tag at all, which is why item 103 made + the trash view path-based. Asking `tags.contains("deleted")` here would + offer Delete on exactly the mail the user is most likely to be looking at + in a trash view. +- **`selectionTagPresence()` is the wrong instrument** for the same reason, + though it is the right shape. A path predicate needs the row's path, which + `MessageNode` carries. +- Deciding this does not require deciding item 118's relationship to it: a + purge stays an explicit whole-view action, and hiding Delete does not make + Delete a purge. + +**A second request, from the same tangent (user, 2026-08-25):** "messages moved +to the trash should be automatically marked `-unread`." Deleting is a decision +about the message, so leaving it bold and unread in the trash is noise; the +count of unread mail should not include what the user threw away. + +It is one line where Delete already composes its tag change, and it carries a +constraint worth stating rather than discovering. `maildir.synchronize_flags` +is true, so removing `unread` REWRITES the Maildir filename and reaches the +server on the next mbsync. That is acceptable here and is a deliberate +exception: it is the same mechanism the `post-new` hook refuses to touch on +arriving mail, for the good reason that the hook acts unattended on mail the +user has not seen. A Delete is an explicit gesture on a message in front of +them, which is the difference. + +Undo must put it back. `TagChange::inverted()` already does, provided the +removal travels as part of the SAME change rather than as a second write, so +one undo returns both the folder and the tag. + +**Built 2026-08-25**, both halves, to the user's own choice of "hide each +where it has no meaning". + +`everySelectedRowIsInATrashFolder()` asks each row about its own file, a reply +row's message and a thread row's displayed message, the same rule +`everySelectedRowHasTag()` follows. `refreshTrashActions()` runs beside +`refreshUnreadAction()` on both the selection change and the model's +`dataChanged`, so the entries follow a write as well as a selection. + +The `unread` removal travels inside the SAME `sendMove()` call rather than as a +second write, which is what makes one undo return the folder and the tag +together. + +**A mutation survived the first round and is worth recording**: comparing the +prefix WITHOUT its trailing separator passed every test, because no fixture had +a folder whose name starts with the trash folder's. `acct/trash-old` is a +different folder, and under that mutation Delete silently disappeared from mail +that had never been trashed, which is the quiet half of the same mistake. The +fixture carries that row now and the mutation fails. + +All three properties are mutation-checked: the separator, Restore's visibility, +and the `unread` removal. The suite is 37 of 38, the failure being item 136 on +an unrelated path, and no new user-facing strings were added. + +**Size: S** for the visibility half, XS for the `unread` half. Done. diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index a422317..437ceda 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -165,12 +165,12 @@ taking that too literally. | 96 | A query returning the thread already on display opens onto the placeholder | defect | S | **done** 2026-08-15, unreleased. Split from 66's unverified half, which had a different cause. Reproduced from two screenshots after four measured eliminations | | 97 | An edit made during a sync is reverted in the list when the sync ends | defect | S | **done** 2026-08-15, unreleased. Found by hand-testing item 89's fix. The sync-end refresh ran BEFORE the held-edit flush, so it read a database that still carried the old tag | | 98 | "Important" adds the tag but cannot remove it, unlike every other toggle | defect | XS | **done** 2026-08-17, unreleased. Calls `everySelectedRowHasTag()`, as the entry required. Its reply test needed THREE different states (list-first thread, the reply's own thread, the reply) before it could tell the two wrong answers apart; with the reply defaulted to its thread's state the item 105 mutation stayed green, measured | -| 99 | The unread action is labelled "Toggle unread" whichever way it will go | presentation | S | open; depends on 98's toggle shape, and the label is harder than it looks | +| 99 | The unread action is labelled "Toggle unread" whichever way it will go | presentation | S | **done 2026-08-25**, unreleased, with 112: the user's note is ONE design across both. The label names the direction it will go, and the entry is hidden on a selection with no single state. `refreshUnreadAction()` reads the new three-valued `selectionTagPresence()` | | 100 | The message pane offers Back, Forward, Reload and Save page, none of which mean anything | defect | XS | **done** 2026-08-17, unreleased. `MessageView::removeBrowserActions()` filters the standard menu by `pageAction()` POINTER, never by text; `ViewSource` went with them, and stranded separators are swept | | 101 | Sync is account-aware for edits but not for the account the user is looking at | workflow | S | open; item 49 built the edit half deliberately. Needs a decision, see the entry | | 102 | The rules table shows no note, so the field explaining a rule is invisible until it is opened | workflow | XS | **done** 2026-08-17, unreleased. A Note column before `ColumnCount`, so the appended Matches column stays last. Found a second defect on the way: `restoreState` REFUSES a header state with a different column count, and the sized flags were being set regardless | | 103 | What Delete does to mail on the server is undocumented and unverified | clarification | S+M | done; Delete moves to the account trash, with Restore and a stranded-mail cleanup. Section in the closed file | -| 104 | Mail visible in Thunderbird never reaches qtmaildir | defect | ? | open, reported 2026-08-16, cause NOT established. Most likely outside this repo; see the entry before writing code | +| 104 | Mail visible in Thunderbird never reaches qtmaildir | defect | XS | **done 2026-08-25**, hand-tested. The worker never reopened its read-only notmuch handle, so no query saw mail indexed after startup. Confirmed on a sync run from the application that added 20 messages: they appeared without a restart | | 109 | A root card's own message is invisible to a message-scoped write | defect | S | **done** 2026-08-16, unreleased. Found by hand-testing 108. `applyMessageTagChange` and `messageById` searched only the loaded replies, and a root's message is never among them, so the ORDINARY gesture repainted nothing and wiped the pane's chip row | | 110 | A card and the message pane show tags belonging to a message's siblings | defect | S | **done** 2026-08-16, unreleased. Found by hand-testing 109 against a real 4-message thread. `ThreadSummary::tags` is notmuch's UNION; a card standing for one message drew it. Also the reason a root card could not repaint at all | | 111 | A card should show its siblings' tags smaller, not drop them | presentation | S | **done** 2026-08-16, unreleased. The user's own design, from looking at 110's result: own tags full size, the thread's others smaller and muted, so nothing appears to vanish on selection | @@ -178,13 +178,13 @@ taking that too literally. | 106 | A tag change made on one message during a sync is silently lost | defect | XS | **done** 2026-08-16, unreleased. Found by READING while fixing 105, never reported. `flushHeldEdits` re-sent only thread-scoped edits, so a message-scoped one was shown, counted as pending, and never written | | 107 | A thread-scoped write leaves the loaded replies showing their old tags | defect | XS | **done** 2026-08-16, unreleased. `applyTagChange` updated the summary only, so marking a thread read left its expanded replies bold | | 108 | Acting on a thread root means the whole thread, though it displays one message | workflow | M | **done** 2026-08-16, unreleased. `messageScopeFor()` beside `scopeFor()`; five `*_thread` actions in a "Whole thread" submenu on `Ctrl+Alt+<key>`. User-visible: minor bump, `### Upgrading` written | -| 112 | Toggle unread on a whole thread cannot reach "all unread" on a partly-read thread | defect | S | open, found 2026-08-17. A toggle over a UNION has no direction on a mixed thread | +| 112 | Toggle unread on a whole thread cannot reach "all unread" on a partly-read thread | defect | S | **done 2026-08-25**, unreleased. Built to the user's own note rather than to this entry's approach, which had it only half right. The thread toggle splits into two absolute actions AND the message-scoped one keeps its toggle with a dynamic label, hidden when the selection disagrees. Closes 99 and 147 with it | | 113 | No way to see a message's HTML source | information | S | open, 2026-08-17. Chromium's own View source cannot work here; needs our own plain-text dialog. Item 100 removed the dead entry, which was an overreach: the user had not asked for it | | 114 | Save image is offered on every image and does nothing | defect | S | open, found 2026-08-17, re-confirmed by hand 2026-08-20. No `downloadRequested` handler exists, so the request is emitted and never answered. The handler is per-profile, so it must decide per request or it revives the Save link item 127 removed | | 115 | A copy from the message pane gives no confirmation | presentation | XS | **done** 2026-08-19, unreleased. Four entries report, each naming what it copied; connected to the page's own QActions, so the entry is covered wherever it is triggered from | | 116 | Copy image copies markup instead of the image | defect | XS | **dropped** 2026-08-17, same day. NOT A DEFECT: `wl-paste --list-types` run immediately after a copy reports `image/png`, `application/x-qt-image` and 30 more image flavours. The clipboard is correct and Chromium is behaving. The earlier "text only" reading was taken minutes late off a clipboard that had been overwritten, and a whole cause was theorised on it | | 117 | The message pane offers no Select all | workflow | XS | **done** 2026-08-19, unreleased. `addPaneActions()` supplies it. The call site is NOT covered by a test and cannot be: the production menu needs a real context-menu event. Stated in the test rather than faked | -| 118 | No way to empty the trash from inside the app | workflow | S | open, 2026-08-17. **Blocked on 103**, which creates the trash in the first place. Deliberately left out of 103's spec at the user's request rather than squeezed in | +| 118 | No way to empty the trash from inside the app | workflow | S | **done 2026-08-25**, unreleased. Unblocked by 103. `Message > Empty trash...`, scoped to the account selector, no shortcut. The one confirmation in this application, and CLAUDE.md now records it as the single exception rather than leaving it to be discovered. Found a defect while testing: the count claimed messages whose files were already gone | | 119 | The unsynced-changes count cannot be opened to see what it counts | information | S | open, 2026-08-19, from the notes. One of the four things it sums carries no message ids at all, so a list cannot be complete without a change to how the count is kept | | 121 | The thread list shows nothing while a query is running | feedback | S | open, 2026-08-20, from the notes. Follows item 74, which fixed the status-bar half and left the list itself blank | @@ -216,7 +216,7 @@ taking that too literally. | 144 | "Also send a formatted copy" is prominent and does not say what it does | presentation | XS | **done** 2026-08-24, unreleased, inside 142. "Send as HTML", icon and text, alone at the right end of the editor bar where it reads as a control of the editor rather than as a formatting button. The Italian entry was refreshed with it, and `lrelease` reports 477 finished, 0 unfinished | | 145 | Cc and Bcc are permanent rows on every composer | presentation | S | **done** 2026-08-24, unreleased, inside 142. A `QToolButton` disclosure beside To:. `revealCcBccIfUsed()` is the load-bearing half the entry called for: it only ever SHOWS, never hides, so nothing but the user's own click can make a field holding an address invisible. `ComposeContext` carries no `bcc` at all, so the seeded-Bcc case can only arrive from a reopened draft, which is what its test drives. The LABEL is hidden with each field: a `QFormLayout` holds the two as separate items, so hiding the line edit alone strands a `Cc:` over empty space | | 146 | The unsynced-changes count cannot be opened to see what it counts | information | S | **duplicate of 119**, recorded 2026-08-23 from the notes. Same request, and 119 already carries the blocker: one of the four things the count sums holds no message ids, so a list cannot be complete without changing how the count is kept | -| 147 | Toggle unread reads the same whichever way it will go | presentation | S | **duplicate of 99**, recorded 2026-08-23 from the notes. The notes ask for exactly what 99 describes: "Mark as read" on an unread message and the reverse. 99 already records that the label is harder than it looks, since a multi-row selection has no single direction | +| 147 | Toggle unread reads the same whichever way it will go | presentation | S | **duplicate of 99**, recorded 2026-08-23 from the notes, and closed with it on 2026-08-25 | | 148 | Ctrl+W does not close the composer | discoverability | XS | **done** 2026-08-24, unreleased. A `QAction` parented to the composer, so it is a WindowShortcut dispatched to the active composer only and the main window's namespace is untouched, exactly like the formatting shortcuts. It calls `close()` rather than doing anything of its own: `closeEvent()` already decides whether the draft is saved, and a second route out that skipped it would lose the message. Not registered in `KeyMap`, so item 132's rules do not apply | | 149 | A reply's cursor lands on the attribution line, not on blank space | defect | XS | **done** 2026-08-24, unreleased, in TWO passes. The first fixed the cursor within each branch (`End` under Above, `Start` under Below) and the user still saw the old layout, because the branches were already right and the DEFAULT was wrong: `above` shipped, and the layout asked for is what `below` produces. Default flipped, and the composer now focuses the body whenever To: is already filled, which a Reply and a Forward always are. Both halves were invisible to the existing `theQuotePositionDecidesWhereTheQuoteLands`, which asserts the quote's position and never the cursor's | | 150 | The receive-only ribbon stays up after the message that raised it is gone | defect | S | **done** 2026-08-24, unreleased. One line in `MessageView::clear()`, beside the blocked-content bar, the stale notice and the attachment bar it already reset by hand. Only `setReceiveOnlyAccount()` hid the ribbon, which every SELECTION change reaches, so a row-to-row move was never the reproducer: it survived the FOUR routes that blank the pane without one (`clear_pane`, `clear_selection`, a new query, a multi-row selection). The first test written for it passed against the defect for exactly that reason | @@ -239,6 +239,9 @@ taking that too literally. | 163 | The message pane shows a stale path, and the composer forks the draft | defect | S | **done, 2026-08-25.** mbsync renames an uploaded file to add its `,U=<uid>` infix while the model still holds the name the query returned. `MaildirName::resolveRenamed()` returns the path unchanged when it exists, else finds the file in that one directory whose unique stem matches; it refuses an ambiguous match and yields nothing for a genuinely missing file. Wired into all THREE read sites: the pane, Reply/Forward, and the draft reopen. The reopen was the one that cost data, forking a draft into two files with two Message-IDs, both reaching the server | | 164 | A draft this application saved keeps `inbox` | defect | S | open, 2026-08-25, **cause corrected 2026-08-25**. The first diagnosis blamed a missing drafts helper and was WRONG: `NOT_ARRIVALS` in `qtmaildirconf.py` is `("sent", "drafts")`, the folder list includes every account's drafts folder, and `notmuch count` confirms the carve-out query MATCHES the affected draft. The carve-out is scoped to `tag:new`, and the draft carries `inbox` while `tag:new` is 0, so it was never in scope when the hook ran. Measured separately: an mbsync-style rename does NOT re-add `new.tags`, so the retag theory is out too. What remains unestablished is WHICH pass tagged it; establish that before writing code | | 165 | A draft gets a new Message-ID on every autosave | enhancement | ? | open, 2026-08-25, found while hand-testing 163 and 164. `MessageBuilder::build()` generates an id unconditionally and every autosave calls it, so each revision is a distinct MESSAGE to notmuch and to the server rather than a new version of one. Invisible while the file is replaced correctly, which item 163's fix restores; it is what turned that fork into two messages rather than one duplicated file. Needs a DECISION on what a draft's identity is before any code: a stable id reused at send, a stable id discarded at send, or the status quo. Neither `ComposeContext` nor `OutgoingMessage` has a field to carry an id, so it is not a changed call site | +| 166 | Mail you send to your own other account loses `inbox` | defect | S | **done 2026-08-25**, unreleased. `sent_only()` keeps a message only when EVERY file is inside a sent folder, which is what the carve-out's docstring already claimed. No query can express it, measured; the root comes from `database.mail_root`, with a split-index fixture the ordinary layout cannot provide. Verified read-only against the live index: 780 of 807 still stripped, 27 spared, no arrival affected | +| 167 | No way to tell one build of an unreleased version from another | enhancement | XS | **done 2026-08-25**, unreleased. The user chose a counter over a git description: `QTMAILDIR_BUILD_NUMBER`, a cmake option ON by default, increments a counter in the BUILD directory on every build and writes `buildnumber.h`. `QTMAILDIR_VERSION_DISPLAY` carries it; `QTMAILDIR_VERSION` stays clean and is what the window title, `applicationVersion` and the release procedure use | +| 168 | Delete is offered on mail already in the trash, and does nothing | defect | S | **done 2026-08-25**, unreleased. Delete is hidden when every selected row is already in its account's trash, Restore when none is, both keyed on the PATH rather than the `deleted` tag. Delete also drops `unread` now, in the same TagChange so one undo returns the folder and the tag together | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -535,140 +538,6 @@ reaches it (item 42), so most of this exists. **Size: S** for the on-demand button, XS for the visibility half. Ask which. -## 104. Mail visible in Thunderbird never reaches qtmaildir - -**Observed (user, from the notes):** "sync doesn't work compared to thunderbird. -New mail received on thunderbird did not appear in qtmaildir. Need to investigate -further." - -**Cause: NOT established.** Recorded because it is a defect report about mail -going missing, which is the most serious kind this backlog carries, and it has -been sitting in the notes unrecorded. What follows is one measured mechanism that -would produce exactly this symptom, not a diagnosis. - -**qtmaildir cannot show what mbsync did not fetch, and mbsync fetches folders by -pattern.** Three of the five channels in the user's `~/.mbsyncrc` name their -folders explicitly: - -``` -Patterns "INBOX" "[Gmail]/Posta inviata" "[Gmail]/Bozze" "[Gmail]/Speciali" -``` - -and one names only `"INBOX"`. The two non-Gmail channels use `Patterns *`. -Gmail applies labels, and a message whose label is not one of those four is in a -folder mbsync never asks for. Thunderbird speaks IMAP directly and sees every -folder, so the same message is visible there and absent locally. This is a -configuration property of the user's mbsyncrc, outside this repository entirely. - -**One inconsistency worth reporting regardless**, found while checking the -above: one of the Gmail accounts is configured in `qtmaildir.conf` with -`sent = [Gmail]/Posta inviata` and `drafts = [Gmail]/Bozze`, while its mbsync -channel has `Patterns "INBOX"` and fetches neither. The Sent and Drafts filters -for that account can therefore only ever be empty. That is real, and it is -independent of whatever this item turns out to be. - -**Approach.** Reproduce before anything else, and the reproduction has to -distinguish three layers, because the fix lives in a different place for each: - -1. Is the message on disk? `find` in the Maildir, or `notmuch count` on a term - from it. If not, this is mbsync or `.mbsyncrc`, and there is nothing to - change here. -2. If it is on disk, is it indexed? `notmuch new` and count again. If not, this - is notmuch config, `new.ignore` or the hook. -3. Only if it is indexed and still not shown is this qtmaildir's defect, and - then the question is which query hid it: the account scope, the built-in - filter, or a rule that tagged it out of the inbox. - -**Constraints.** - -- Ask the user for one concrete example before investigating: which account, - roughly when, and what Thunderbird shows for it. A general "sync doesn't work" - cannot be reproduced, and the last four defects in this backlog were all found - from a specific message. -- The `post-new` hook from mailctl tags mail unattended. A rule that removes - `inbox` would make a correctly fetched, correctly indexed message vanish from - the default view, which looks identical to a sync failure from the outside. - `notmuch search` without a filter is what tells them apart. -- Do not change `.mbsyncrc` as part of this. It is the user's, it is outside the - repo, and a Patterns change refetches folders. - -**Size: `?`** until reproduced. Most likely not a code change here at all. - - -## 112. Toggle unread on a whole thread cannot reach "all unread" on a partly-read thread - -**Observed (user, 2026-08-17):** clicking a thread root and asking to mark the -whole thread unread does not do it. On a seven-message thread with two unread -replies, the result is that every message is toggled unread **except those -two**, which are left as they were. The user asks for an explicit "mark whole -thread read/unread" rather than a toggle. - -**Cause (verified in code):** the action exists, and its direction is the -defect. `toggle_unread_thread` (`src/mainwindow.cpp:931`, `Ctrl+Alt+U`) chooses -between adding and removing by asking -`everySelectedRowHasTag("unread", TagScope::Thread)`, which reads -`ThreadListModel::threadFor(index).tags`. That is notmuch's **union over the -thread** (`CLAUDE.md`, item 110), so a thread containing even one unread message -answers "unread" and the action picks *Mark thread read*. There is no input a -user can give that reaches *Mark thread unread* on a mixed thread: the only -threads that take that branch are the ones already entirely read, and the only -threads reporting "not unread" are the ones the user does not need the action -for. - -The write itself is absolute and correct. `tagSelected` with `TagScope::Thread` -adds or removes `unread` across every message, so the two unread replies in the -report are not skipped by the write. They are the reason the write ran in the -opposite direction from the one the user wanted. - -**A union is not a state, and a toggle needs a state.** This is the same class -as item 110 and the third time the union has produced a defect. Items 105 and 88 -fixed *which object* a toggle resolved; this one is about a thread having no -single answer to give. `everySelectedRowHasTag` is a two-valued predicate over a -three-valued reality: all read, all unread, or mixed. The mixed case is the one -that has no correct toggle direction, and picking either one silently is what -ships as "the action does the wrong thing". - -**Approach.** The user has already named it: stop toggling at thread scope. - -- Split `toggle_unread_thread` into two explicit actions, **Mark thread read** - and **Mark thread unread**, each with a fixed direction. Both appear in the - "Whole thread" submenu, where an entry always carries text, so a fixed label - is honest in a way a toggle's cannot be. -- The message-scoped `toggle_unread` stays a toggle. One message has a real - two-valued state, so the trap does not exist there. Do not "unify" the two: - the asymmetry is the point. - -**Constraints.** - -- **Adding an action is four places**, all enforced by tests that fail - confusingly: `KeyMap::knownActions()`, `defaultBindings()`, the icon table, - and the no-duplicate-icons exception list. See `CLAUDE.md`. Splitting one - action into two means one new entry in each, and the pair shares the twin's - icon under the existing named exemption for thread actions. -- **`Ctrl+Alt+U` is taken by the action being split**, and the whole-thread - bindings are already one modifier out from their twins because `Ctrl+Shift+U` - was claimed. Two directions need two sequences; if a second chord cannot be - found that is not worse than the menu, bind one and leave the other to the - submenu rather than inventing a three-modifier chord nobody will press. -- **This interacts with items 98 and 99**, which is the reason to decide all - three together. 99 asks for a dynamic label on the message-scoped toggle, - which is the opposite move: keep the toggle, make the label tell the truth. - A thread cannot do that, because on a mixed thread there is no true label to - show. Deciding 99 first will produce the wrong answer here by analogy. -- The undo entry must name the direction that ran (`Mark thread unread`), not - the action. `tagSelected` already takes the text, so this comes free from - splitting. -- **The test needs a MIXED thread**, which is the whole defect: a thread whose - messages are all in one state answers identically whichever way the direction - is computed, so a fixture built from a uniformly-unread thread passes against - the bug. Same trap as item 88's opposite-states requirement, recorded in - `CLAUDE.md`. - -**Size: S.** The write path is already correct and thread-scoped; the work is -the action split, the four registration sites, the binding decision, and a test -over a mixed thread. - - ## 113. No way to see a message's HTML source **Observed (user, 2026-08-17):** reviewing item 100's removals, "view source @@ -804,40 +673,6 @@ make Save image work must not make Save link reachable again. The test fails if it does, which is the point: the handler is per-profile, so the natural implementation would light up both entries at once. -## 118. No way to empty the trash from inside the app - -**Observed (user, 2026-08-17):** raised while reviewing item 103's spec, as -something that had been forgotten rather than newly noticed: "we could add -'Empty Trash' to the backlog as a future item. I forgot it existed, but I don't -want to squeeze it in this spec." - -**Blocked on 103**, which creates the trash folder this would empty. Until that -ships there is nothing to empty: Delete writes a tag and moves no file, so no -account has a populated trash folder except through another client. - -**Deliberately excluded from 103's spec**, at the user's request and recorded in -its "Out of scope" section. Worth keeping separate for a reason beyond scope -control: emptying the trash is the first action in this application that would -destroy mail with no undo. Every mutation so far is a tag or, after 103, a move, -and both are reversible. A purge is not. - -**Approach, unspecified.** The shape depends on decisions not yet made, and the -spec for 103 answers none of them: - -- **Local or remote.** Deleting the files locally and letting `Expunge Both` - carry it to the server is one thing; asking the provider to empty its own - trash is another, and mbsync offers no verb for the latter. The first is - probably what "Empty Trash" should mean here. -- **Whether the no-confirmation rule survives it.** It does not, on the face of - it. `CLAUDE.md` grants undo in place of confirmation dialogs, and this is the - action where undo cannot exist. That makes it the second item, after 103, that - re-examines the rule rather than assuming it, and unlike 103 it will probably - have to break it. -- **Per-account or all-accounts**, which should follow whatever the Trash filter - does once 103 ships rather than being decided independently. - -**Size: S**, provisionally, and not worth sizing properly until 103 exists. - ## 119. The unsynced-changes count cannot be opened to see what it counts **Observed (user, from the notes):** "the bottom left statusbar message needs to diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 700b185..4d9dbaa 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -44,6 +44,12 @@ target_include_directories(qtmaildir_lib PUBLIC ${CMAKE_CURRENT_SOURCE_DIR} ${NOTMUCH_INCLUDE_DIR} ${CMAKE_BINARY_DIR}/generated/qtmaildir) +# The counter target rewrites buildnumber.h, which version.h includes, so the +# library must not start compiling before it has run. +if(TARGET qtmaildir_buildnumber) + add_dependencies(qtmaildir_lib qtmaildir_buildnumber) +endif() + target_link_libraries(qtmaildir_lib PUBLIC Qt6::Widgets Qt6::Svg Qt6::WebEngineWidgets PkgConfig::GMIME ${NOTMUCH_LIBRARY} PkgConfig::CMARK_GFM diff --git a/src/keymap.cpp b/src/keymap.cpp index 6cd965a..6605882 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -33,6 +33,10 @@ QStringList KeyMap::knownActions() QStringLiteral("delete"), QStringLiteral("restore"), QStringLiteral("cleanup_stranded"), + // Item 118. No default binding, deliberately: this is the one action + // that destroys mail with no undo, and a chord is how it would be run + // by accident. Menu only, which item 132 made a legitimate choice. + QStringLiteral("empty_trash"), QStringLiteral("spam"), QStringLiteral("toggle_unread"), QStringLiteral("mark_all_read"), @@ -52,7 +56,12 @@ QStringList KeyMap::knownActions() QStringLiteral("archive_thread"), QStringLiteral("delete_thread"), QStringLiteral("spam_thread"), - QStringLiteral("toggle_unread_thread"), + // Item 112 split the thread toggle in two. Neither carries a default + // chord, at the user's choice: since item 132 a shortcut is a chosen + // subset rather than a requirement, and Ctrl+Alt+U meant whichever + // direction the union happened to pick, which is what made it wrong. + QStringLiteral("mark_thread_read"), + QStringLiteral("mark_thread_unread"), QStringLiteral("flag_thread"), // Compose and send (item 123). save_message deliberately carries no // default chord: since item 132 a shortcut is a chosen subset rather @@ -177,7 +186,6 @@ QList<QPair<QString, QString>> KeyMap::defaultBindings() { QStringLiteral("Ctrl+Alt+E"), QStringLiteral("archive_thread") }, { QStringLiteral("Ctrl+Alt+D"), QStringLiteral("delete_thread") }, { QStringLiteral("Ctrl+Alt+S"), QStringLiteral("spam_thread") }, - { QStringLiteral("Ctrl+Alt+U"), QStringLiteral("toggle_unread_thread") }, { QStringLiteral("Ctrl+Alt+I"), QStringLiteral("flag_thread") }, { QStringLiteral("Ctrl+T"), QStringLiteral("edit_tags") }, // Shifted against Ctrl+T for the same reason Ctrl+Shift+U is shifted diff --git a/src/main.cpp b/src/main.cpp index 2ed8057..a7908d0 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -43,7 +43,7 @@ int main(int argc, char *argv[]) for (int i = 1; i < argc; ++i) { if (std::strcmp(argv[i], "--version") == 0 || std::strcmp(argv[i], "-v") == 0) { - std::printf("qtmaildir %s\n", QTMAILDIR_VERSION); + std::printf("qtmaildir %s\n", QTMAILDIR_VERSION_DISPLAY); return 0; } if (std::strcmp(argv[i], "--help") == 0 @@ -59,7 +59,7 @@ int main(int argc, char *argv[]) "Configuration: ~/.config/qtmaildir/qtmaildir.conf\n" "qtmaildir reads a notmuch-indexed Maildir. It does no network\n" "protocol work: fetching and sending are external commands.\n", - QTMAILDIR_VERSION); + QTMAILDIR_VERSION_DISPLAY); return 0; } } diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 5845922..89c01eb 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -900,6 +900,16 @@ void MainWindow::buildUi() &QItemSelectionModel::selectionChanged, this, &MainWindow::onSelectionChanged); + // The label describes the SELECTION'S STATE, which a write moves without + // touching the selection: marking the current row read has to flip the + // entry to "Mark as unread" with the same row still selected. Keyed on + // the model rather than on each of the six call sites that apply an + // optimistic update, so a new one cannot forget. + connect(m_model, &QAbstractItemModel::dataChanged, this, [this]() { + refreshUnreadAction(); + refreshTrashActions(); + }); + connect(m_threadView, &QAbstractItemView::doubleClicked, this, &MainWindow::onRowDoubleClicked); @@ -1580,6 +1590,19 @@ void MainWindow::registerActions() [this]() { showStrandedDeletedMail(); }); + // The ONE irreversible action in this application, and the only one that + // asks before it runs (item 118). CLAUDE.md rules out confirmation + // dialogs for mutations because every mutation pushes its inverse onto + // the undo stack; a purge has no inverse, so the rule does not reach it. + // What the rule protects is that the user never loses work to a + // keystroke, which here is what the dialog provides. + // + // No default shortcut, for the same reason: a chord is how this would be + // run by accident. + addAction(QStringLiteral("empty_trash"), tr("Empt&y trash..."), + tr("Permanently delete every message in the trash"), [this]() { + emptyTrash(); + }); addAction(QStringLiteral("spam"), tr("Mark &spam"), tr("Add spam and remove inbox"), [this]() { tagSelected({ QStringLiteral("spam") }, { QStringLiteral("inbox") }, @@ -1684,21 +1707,34 @@ void MainWindow::registerActions() tagSelected({ QStringLiteral("spam") }, { QStringLiteral("inbox") }, tr("Mark thread spam"), TagScope::Thread); }); - addAction(QStringLiteral("toggle_unread_thread"), tr("Toggle &unread"), - tr("Toggle the unread tag on whole threads"), [this]() { + // Two fixed directions rather than one toggle, and the asymmetry with the + // message-scoped twin is the point (item 112). `ThreadSummary::tags` is + // notmuch's UNION over the conversation, so a thread holding even one + // unread message answers "unread" and a toggle reading that predicate + // always chose "mark read": there was no input that reached "mark thread + // unread" on a mixed thread, which is exactly the thread a user wants it + // for. A union is not a state, and a toggle needs a state. + // + // The message-scoped `toggle_unread` stays a toggle, because one message + // has a real two-valued state. Do not unify them. + addAction(QStringLiteral("mark_thread_read"), tr("Mark thread &read"), + tr("Remove the unread tag from every message of the selected " + "threads"), [this]() { + m_markReadTimer->stop(); + m_markReadMessageId.clear(); + tagSelected({}, { QStringLiteral("unread") }, + tr("Mark thread read"), TagScope::Thread); + }); + addAction(QStringLiteral("mark_thread_unread"), tr("Mark thread &unread"), + tr("Add the unread tag to every message of the selected threads"), + [this]() { // Cancels the automatic mark-read for the same reason its // message-scoped twin does: a thread marked unread by hand must not be // undone a moment later by a timer armed when it was opened. m_markReadTimer->stop(); m_markReadMessageId.clear(); - - if (everySelectedRowHasTag(QStringLiteral("unread"), TagScope::Thread)) { - tagSelected({}, { QStringLiteral("unread") }, - tr("Mark thread read"), TagScope::Thread); - } else { - tagSelected({ QStringLiteral("unread") }, {}, - tr("Mark thread unread"), TagScope::Thread); - } + tagSelected({ QStringLiteral("unread") }, {}, + tr("Mark thread unread"), TagScope::Thread); }); addAction(QStringLiteral("flag_thread"), tr("&Important"), tr("Mark every message of the selected threads as important"), @@ -1936,6 +1972,7 @@ void MainWindow::buildMenus() // It replaces the whole view like a filter does, so a sixth button beside // the five filters would read as one of them. messageMenu->addAction(m_actions.value(QStringLiteral("cleanup_stranded"))); + messageMenu->addAction(m_actions.value(QStringLiteral("empty_trash"))); messageMenu->addAction(m_actions.value(QStringLiteral("tag_rules"))); auto *viewMenu = menuBar()->addMenu(tr("&View")); @@ -1996,6 +2033,7 @@ void MainWindow::buildMenus() // nothing, so an icon from the delete family would promise the one // thing it deliberately does not do. { QStringLiteral("cleanup_stranded"), QStringLiteral("system-search") }, + { QStringLiteral("empty_trash"), QStringLiteral("edit-delete-shred") }, { QStringLiteral("undo"), QStringLiteral("edit-undo") }, { QStringLiteral("spam"), QStringLiteral("mail-mark-junk") }, { QStringLiteral("flag"), QStringLiteral("mail-mark-important") }, @@ -2040,7 +2078,8 @@ void MainWindow::buildMenus() { QStringLiteral("archive_thread"), QStringLiteral("mail-archive") }, { QStringLiteral("delete_thread"), QStringLiteral("edit-delete") }, { QStringLiteral("spam_thread"), QStringLiteral("mail-mark-junk") }, - { QStringLiteral("toggle_unread_thread"), QStringLiteral("mail-mark-unread") }, + { QStringLiteral("mark_thread_read"), QStringLiteral("mail-mark-read") }, + { QStringLiteral("mark_thread_unread"), QStringLiteral("mail-mark-unread") }, { QStringLiteral("flag_thread"), QStringLiteral("mail-mark-important") }, // Compose and send (item 123). reply_no_quote SHARES reply's icon for @@ -2446,7 +2485,7 @@ void MainWindow::showAbout() "version 2.</p>" "<p>Developed with AI assistance. All code is reviewed, " "tested and curated by the maintainer.</p>") - .arg(QStringLiteral(QTMAILDIR_VERSION))); + .arg(QStringLiteral(QTMAILDIR_VERSION_DISPLAY))); auto *link = new QLabel( QStringLiteral("<a href='https://danix.xyz/qtmaildir'>" @@ -2518,6 +2557,18 @@ void MainWindow::wireWorker() connect(m_worker, &NotmuchWorker::messagesMovedFrom, this, &MainWindow::onMessagesMoved); + // A purge removes rows rather than changing them, so there is no + // optimistic update to apply: the only honest view is the one the query + // gives now. Without this the list went on showing mail that no longer + // existed until the user refreshed by hand, which is how the user found + // it. + connect(m_worker, &NotmuchWorker::messagesPurged, this, + [this](const QStringList &messageIds) { + showTransientStatus( + tr("Deleted %n message(s) permanently", "", messageIds.size())); + runCurrentQuery(); + }); + connect(m_worker, &NotmuchWorker::threadMessagesResolved, this, &MainWindow::onThreadMessagesResolved); @@ -3468,8 +3519,98 @@ void MainWindow::showThreadContextMenu(const QPoint &pos) m_threadContextMenu->popup(m_threadView->viewport()->mapToGlobal(pos)); } +bool MainWindow::everySelectedRowIsInATrashFolder() const +{ + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + if (rows.isEmpty()) + return false; + + for (const QModelIndex &index : rows) { + // The row's own file: a reply row's message, a thread row's displayed + // message. Same rule as everySelectedRowHasTag(), and for the same + // reason: a thread row acts on the message its card shows. + const QString path = + m_model->isMessageRow(index) + ? m_model->messageAt(index).filePath + : m_model->threadFor(index).firstMessagePath; + if (path.isEmpty()) + return false; + + const Account account = accountForMessagePath(path); + if (account.maildir.isEmpty() || account.trash.isEmpty()) + return false; + + // Compared as a path segment, never with startsWith(): `trash-old` + // starts with `trash` and is a different folder. The same trap the + // attachment-save check records. + const QString prefix = account.maildir + QLatin1Char('/') + + account.trash + QLatin1Char('/'); + // accountForMessagePath() accepts both shapes, so this must too: a + // thread row's path is database-relative and a reply row's absolute. + if (!path.contains(prefix)) + return false; + } + return true; +} + +void MainWindow::refreshTrashActions() +{ + const bool inTrash = everySelectedRowIsInATrashFolder(); + const bool haveSelection = + !m_threadView->selectionModel()->selectedRows().isEmpty(); + + // Delete on mail already in the trash reported success and did nothing: + // moveMessages() finds the file already in the destination and takes its + // early-return branch, which counts an unsynced change for a move that + // never happened (item 168). + if (auto *del = m_actions.value(QStringLiteral("delete"))) + del->setVisible(!haveSelection || !inTrash); + + // The mirror, which shipped beside it: Restore was added unconditionally + // to both menus and so was offered on mail that was never deleted. + if (auto *restore = m_actions.value(QStringLiteral("restore"))) + restore->setVisible(!haveSelection || inTrash); +} + +void MainWindow::refreshUnreadAction() +{ + // The user's design (item 112 and its duplicates 99/147): the label says + // which way the action will go, and on a selection with no single state + // the entry is HIDDEN rather than labelled wrongly. The thread submenu is + // then the route, whose entries are absolute and work whatever the mix. + auto *action = m_actions.value(QStringLiteral("toggle_unread")); + if (!action) + return; + + switch (selectionTagPresence(QStringLiteral("unread"))) { + case TagPresence::Every: + action->setVisible(true); + action->setText(tr("Mark as &read")); + action->setStatusTip(tr("Remove the unread tag from the selection")); + break; + case TagPresence::None: + action->setVisible(true); + action->setText(tr("Mark as &unread")); + action->setStatusTip(tr("Add the unread tag to the selection")); + break; + case TagPresence::Mixed: + // No honest label exists, so there is no label to show. Hidden rather + // than disabled, at the user's choice. + action->setVisible(false); + break; + } +} + void MainWindow::onSelectionChanged() { + // Here rather than in the currentRowChanged handler: that signal is + // emitted BEFORE the selection model is updated, so a handler reading + // selectedRows() there sees the PREVIOUS selection and would label the + // action for the rows the user just left (CLAUDE.md, verified Qt 6.11). + refreshUnreadAction(); + refreshTrashActions(); + const QModelIndexList rows = m_threadView->selectionModel()->selectedRows(); const int selected = rows.size(); if (selected == 1) { @@ -4915,6 +5056,16 @@ QString MainWindow::currentThreadFirstMessageId() const bool MainWindow::everySelectedRowHasTag(const QString &tag, TagScope scope) const { + // Kept as the direction question, which only has two answers to give: a + // mixed selection has to go one way, and this says which. The LABEL asks + // selectionTagPresence() instead, because a label can say "these disagree" + // and a direction cannot. + return selectionTagPresence(tag, scope) == TagPresence::Every; +} + +MainWindow::TagPresence MainWindow::selectionTagPresence(const QString &tag, + TagScope scope) const +{ // What a toggle asks before choosing its direction, for both Delete and // Toggle unread. // @@ -4932,8 +5083,9 @@ bool MainWindow::everySelectedRowHasTag(const QString &tag, const QModelIndexList rows = m_threadView->selectionModel()->selectedRows(); if (rows.isEmpty()) - return false; + return TagPresence::None; + int withTag = 0; for (const QModelIndex &index : rows) { QStringList tags; if (scope == TagScope::Thread) { @@ -4979,10 +5131,13 @@ bool MainWindow::everySelectedRowHasTag(const QString &tag, tags = own.messageId.isEmpty() ? summary.firstMessageTags : own.tags; } - if (!tags.contains(tag)) - return false; + if (tags.contains(tag)) + ++withTag; } - return true; + + if (withTag == 0) + return TagPresence::None; + return withTag == rows.size() ? TagPresence::Every : TagPresence::Mixed; } ThreadSummary MainWindow::threadForCurrentRowForTesting() const @@ -5003,7 +5158,8 @@ QMenu *MainWindow::buildThreadActionsMenu(QWidget *parent) menu->addAction(m_actions.value(QStringLiteral("delete_thread"))); menu->addAction(m_actions.value(QStringLiteral("spam_thread"))); menu->addSeparator(); - menu->addAction(m_actions.value(QStringLiteral("toggle_unread_thread"))); + menu->addAction(m_actions.value(QStringLiteral("mark_thread_read"))); + menu->addAction(m_actions.value(QStringLiteral("mark_thread_unread"))); menu->addAction(m_actions.value(QStringLiteral("flag_thread"))); return menu; } @@ -5289,8 +5445,23 @@ void MainWindow::trashMessages(const QStringList &messageIds, return; for (auto it = byTrash.cbegin(); it != byTrash.cend(); ++it) { + // `unread` goes with it (item 168, the user's request). Deleting is a + // decision about the message, so the unread count must not go on + // including what the user threw away. + // + // In the SAME change rather than as a second write, so one undo + // returns the folder and the tag together: TagChange::inverted() + // gives it back only if it travelled with the move. + // + // This rewrites the Maildir filename, because + // maildir.synchronize_flags is true, and so reaches the server on the + // next mbsync. That is the same mechanism the post-new hook REFUSES + // to touch, and the difference is who is acting: the hook tags + // arriving mail unattended, while this is an explicit gesture on a + // message in front of the user. sendMove(it.value(), it.key(), - { QStringLiteral("deleted"), kOriginTagPlaceholder() }, {}, + { QStringLiteral("deleted"), kOriginTagPlaceholder() }, + { QStringLiteral("unread") }, tr("Delete"), false, wholeThreadIds); } @@ -5392,6 +5563,11 @@ void MainWindow::onThreadMessagesResolved(const QStringList &messageIds, const QStringList threadScope = m_pendingThreadScope; m_pendingThreadScope.clear(); + if (requestTag == QStringLiteral("empty_trash")) { + confirmAndPurge(messageIds); + return; + } + if (requestTag == QStringLiteral("delete_thread")) { trashMessages(messageIds, pathById, messageIds.size(), threadScope); return; @@ -5620,6 +5796,83 @@ void MainWindow::restoreSelectedFromTrash() Q_ARG(QString, QStringLiteral("restore_messages"))); } +void MainWindow::purgeForTesting(const QStringList &messageIds) +{ + if (!m_worker || messageIds.isEmpty()) + return; + QMetaObject::invokeMethod(m_worker, "purgeMessages", Qt::QueuedConnection, + Q_ARG(QStringList, messageIds)); +} + +void MainWindow::emptyTrash() +{ + // Scoped to the account selector, like every other account-aware surface: + // the All accounts view empties every configured trash, a selected + // account empties only its own. The user sees which in the dialog. + const QString accountKey = m_accountBox->currentData().toString(); + const QString query = accountKey.isEmpty() + ? m_config.allTrashQuery() + : m_config.account(accountKey).trashQuery(); + + // An account with no trash folder configured produces an EMPTY query, and + // an empty notmuch query matches EVERYTHING. Refusing here rather than + // relying on the worker's own guard, so the message names the cause. + if (query.isEmpty()) { + showTransientStatus(tr("No trash folder is configured")); + return; + } + + if (!m_worker) { + showTransientStatus(tr("Not connected to the mail index")); + return; + } + + // Enumerated before it is counted, and counted from the DATABASE: the + // number in the dialog has to be the number destroyed, and the model + // holds whatever the current view is showing, which is usually not the + // trash at all. + QMetaObject::invokeMethod(m_worker, "resolveQueryMessages", + Qt::QueuedConnection, + Q_ARG(QString, query), + Q_ARG(QString, QStringLiteral("empty_trash"))); +} + +void MainWindow::confirmAndPurge(const QStringList &messageIds) +{ + if (messageIds.isEmpty()) { + showTransientStatus(tr("The trash is already empty")); + return; + } + + const QString accountKey = m_accountBox->currentData().toString(); + const QString where = accountKey.isEmpty() + ? tr("every account") + : m_accountBox->currentText(); + + QMessageBox box(this); + box.setObjectName(QStringLiteral("emptyTrashConfirmation")); + box.setIcon(QMessageBox::Warning); + box.setWindowTitle(tr("Empty trash")); + box.setText(tr("Permanently delete %n message(s) from the trash of %1?", + "", messageIds.size()) + .arg(where)); + // Said plainly, because it is the only place in this application where it + // is true. + box.setInformativeText(tr("This cannot be undone.")); + box.addButton(QMessageBox::Cancel); + QPushButton *confirm = + box.addButton(tr("Delete permanently"), QMessageBox::DestructiveRole); + // Cancel is the default, so Return does not destroy mail. + box.setDefaultButton(QMessageBox::Cancel); + box.exec(); + + if (box.clickedButton() != confirm) + return; + + QMetaObject::invokeMethod(m_worker, "purgeMessages", Qt::QueuedConnection, + Q_ARG(QStringList, messageIds)); +} + void MainWindow::showStrandedDeletedMail() { // Not scoped to the selected account, deliberately. The stranded mail is diff --git a/src/mainwindow.h b/src/mainwindow.h index 951eaa4..a5a8c31 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -169,6 +169,11 @@ public: /// command was pushed, which is what "this did nothing" has to assert. int undoDepthForTesting() const { return m_undoStack.count(); } + /// Runs a purge without the confirmation, which a test cannot drive: a + /// modal blocks the thread it is shown on (item 84). What this exists to + /// cover is what happens AFTER the user confirms. + void purgeForTesting(const QStringList &messageIds); + /// The text of the command on top of the undo stack. /// /// A test seam for the DIRECTION a toggle chose. Delete and Undelete both @@ -887,6 +892,37 @@ private: bool everySelectedRowHasTag(const QString &tag, TagScope scope = TagScope::Message) const; + /// The three-valued version of the question above, which is what a LABEL + /// needs and a toggle's direction does not. + /// + /// `everySelectedRowHasTag` answers yes or no over a reality with three + /// states: every row has the tag, none does, or they disagree. That is + /// enough to choose a direction, since a mixed selection has to go one way + /// or the other, but it cannot name the direction honestly, and item 112 + /// is what happens when a two-valued predicate is asked a three-valued + /// question. + enum class TagPresence { None, Every, Mixed }; + TagPresence selectionTagPresence( + const QString &tag, TagScope scope = TagScope::Message) const; + + /// Relabels the unread action, and hides it when the selection has no + /// single state. Called whenever the selection changes. + void refreshUnreadAction(); + + /// Hides Delete on mail already in the trash, and Restore on mail that + /// was never there (item 168). Each is offered only where it means + /// something, the same rule refreshUnreadAction() applies to the label. + void refreshTrashActions(); + + /// Whether every selected row's file already sits in its account's trash + /// folder. Empty selection answers false. + /// + /// The question is about the PATH, never the `deleted` TAG: a message + /// trashed by another client carries no such tag at all, which is why the + /// trash view is path-based (item 103), and asking the tag would offer + /// Delete on exactly the mail a trash view is full of. + bool everySelectedRowIsInATrashFolder() const; + void editTagsOnSelection(); /// Set once the user has answered the exit prompt, or once a sync started @@ -1015,6 +1051,17 @@ private: /// without moving. void showStrandedDeletedMail(); + /// Asks the worker what is in the trash. The answer arrives at + /// onThreadMessagesResolved() tagged `empty_trash` and goes to + /// confirmAndPurge(): the count in the dialog has to be what will actually + /// be destroyed, so it comes from the database rather than from the model, + /// which holds whatever the current view happens to show. + void emptyTrash(); + + /// The confirmation, and the only one in this application. Destroys + /// nothing if the user declines. + void confirmAndPurge(const QStringList &messageIds); + /// Moves each resolved message home, using the tags and paths the WORKER /// reported rather than anything the model holds. void restoreResolvedMessages(const QStringList &messageIds, diff --git a/src/messageview.cpp b/src/messageview.cpp index 86e40eb..eb0dccd 100644 --- a/src/messageview.cpp +++ b/src/messageview.cpp @@ -544,7 +544,7 @@ void MessageView::showPlaceholder( // uses it: a style sheet or a themed parent can give this pane different // colours from the application. setDocument(HtmlBuilder::buildPlaceholder( - helpers, QStringLiteral(QTMAILDIR_VERSION), + helpers, QStringLiteral(QTMAILDIR_VERSION_DISPLAY), HtmlBuilder::brandPaletteFrom(palette()))); } diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index f3a76ea..8ab3ab5 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -920,6 +920,99 @@ void NotmuchWorker::moveMessages(const QStringList &messageIds, emit messagesMovedFrom(origins, destFolder); } +void NotmuchWorker::purgeMessages(const QStringList &messageIds) +{ + if (messageIds.isEmpty()) + return; + + // Same handle ordering as applyTags() and moveMessages(): notmuch allows + // one open handle per process, so the read-only one closes first. + close(); + + const QByteArray configPath = configPathArg(); + notmuch_database_t *db = nullptr; + char *error = nullptr; + const notmuch_status_t status = notmuch_database_open_with_config( + nullptr, + NOTMUCH_DATABASE_MODE_READ_WRITE, + configPath.isEmpty() ? nullptr : configPath.constData(), + nullptr, + &db, + &error); + + if (status != NOTMUCH_STATUS_SUCCESS) { + emit errorOccurred( + QStringLiteral("Cannot open database for writing: %1") + .arg(QString::fromUtf8(error ? error + : notmuch_status_to_string(status)))); + free(error); + return; + } + + QStringList purged; + for (const QString &id : messageIds) { + notmuch_message_t *raw = nullptr; + // find_message reports SUCCESS with a null message for an unknown id, + // so both are checked. A stale id does not abort the batch: the live + // ids beside it still have to go. + if (notmuch_database_find_message(db, id.toUtf8().constData(), &raw) + != NOTMUCH_STATUS_SUCCESS || !raw) { + continue; + } + NmMessage message(raw); + + // EVERY file, not just the first. notmuch deduplicates by Message-ID, + // so one message can have several files; unlinking one would leave the + // message alive in the folder the user emptied, which reads as the + // purge having silently skipped it. This is the same one-message, + // many-files property that item 166 turned on. + QStringList files; + for (NmFilenames names(notmuch_message_get_filenames(message.get())); + notmuch_filenames_valid(names.get()); + notmuch_filenames_move_to_next(names.get())) { + files.append(QString::fromUtf8(notmuch_filenames_get(names.get()))); + } + + // The handle is released before the files go out from under it. + message.reset(); + + bool removedAny = false; + for (const QString &file : files) { + // A file already gone is not an ERROR: the index can name a path a + // sync has since removed, and the goal state (no file) is reached + // either way. Reporting it would teach the user to ignore the one + // message that matters here. + // + // It is not a DESTRUCTION either, which is a separate point and + // the one a first version got wrong. The count reaches the user as + // the size of an irreversible act, so it must say what this run + // actually destroyed, not what was already absent when it started. + if (!QFile::exists(file)) { + notmuch_database_remove_message(db, file.toUtf8().constData()); + continue; + } + if (!QFile::remove(file)) { + emit errorOccurred(QStringLiteral("Cannot delete %1") + .arg(QFileInfo(file).fileName())); + continue; + } + removedAny = true; + // The index entry for that path. When the last filename goes, so + // does the message and every tag on it, which is exactly what is + // wanted here and is the thing moveMessages() has to avoid. + notmuch_database_remove_message(db, file.toUtf8().constData()); + } + + if (removedAny) + purged.append(id); + } + + notmuch_database_close(db); + notmuch_database_destroy(db); + + emit messagesPurged(purged); +} + void NotmuchWorker::indexDraftFile(const QString &path, const QString &previousPath) { @@ -1025,6 +1118,14 @@ void NotmuchWorker::resolveMessages(const QStringList &messageIds, resolveQuery(terms.join(QStringLiteral(" or ")), requestTag); } +void NotmuchWorker::resolveQueryMessages(const QString &query, + const QString &requestTag) +{ + if (query.isEmpty()) + return; + resolveQuery(query, requestTag); +} + void NotmuchWorker::resolveThreadMessages(const QStringList &threadIds, const QString &requestTag) { diff --git a/src/notmuchworker.h b/src/notmuchworker.h index 3ccf8e5..2efddaa 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -134,6 +134,22 @@ public slots: /// it, so removing before indexing loses the message's tags. void moveMessages(const QStringList &messageIds, const QString &destFolder); + /// Destroys mail: removes each file from disk and each message from the + /// index. **This is the only irreversible operation in the application** + /// (item 118), which is why it is a separate entry point rather than a + /// flag on moveMessages(): the two look alike and one of them can be + /// undone. + /// + /// Named ids only, never a folder-wide sweep, so the blast radius is + /// whatever the caller enumerated and confirmed. A message with several + /// files loses every file it has, since leaving one behind would leave + /// the message alive in a folder the user emptied. + /// + /// The caller is responsible for confirming: CLAUDE.md rules out + /// confirmation dialogs for mutations because undo replaces them, and + /// this is the one action where undo cannot exist. + void purgeMessages(const QStringList &messageIds); + /// Indexes one freshly written file, so it appears in a `path:` query /// without a full `notmuch new` (item 158). /// @@ -190,6 +206,12 @@ public slots: void resolveMessages(const QStringList &messageIds, const QString &requestTag); + /// The same walk for an arbitrary QUERY, which is what Empty Trash needs: + /// it has to enumerate what it is about to destroy before it can say how + /// much that is, and the answer must not come from the model, which holds + /// whatever the current view happens to be showing. + void resolveQueryMessages(const QString &query, const QString &requestTag); + private: /// The shared walk behind resolveMessages() and resolveThreadMessages(): /// runs `query` and emits threadMessagesResolved() with each match's id, @@ -273,6 +295,10 @@ signals: /// than aborting the batch. void messagesMoved(const QStringList &messageIds, const QString &destFolder); + /// What a purge actually destroyed. Unlike a move there is no new path to + /// observe afterwards, so this is the only report the UI has. + void messagesPurged(const QStringList &messageIds); + /// The same move, reported per message with the folder it came FROM. /// /// Emitted alongside messagesMoved rather than replacing it: that signal's diff --git a/src/version.h.in b/src/version.h.in index 8d76a3a..743adc5 100644 --- a/src/version.h.in +++ b/src/version.h.in @@ -25,3 +25,26 @@ #define QTMAILDIR_VERSION_MINOR @PROJECT_VERSION_MINOR@ #define QTMAILDIR_VERSION_PATCH @PROJECT_VERSION_PATCH@ #define QTMAILDIR_VERSION "@PROJECT_VERSION@" + +/// The version as shown to a person, which in a DEV build carries the build +/// number and in a release build is exactly QTMAILDIR_VERSION. +/// +/// Two separate macros deliberately. The release procedure checks `--version` +/// against a clean X.Y.Z, the SlackBuild builds from a release tarball where +/// no build counter exists, and the window title is a poor place for a number +/// that changes on every rebuild. Anything comparing versions uses +/// QTMAILDIR_VERSION; anything a person reads to answer "which build am I +/// running" uses this one. +/// +/// buildnumber.h is generated at BUILD time, not here: this file is written +/// by configure_file(), which runs once per cmake run, so a counter +/// interpolated into it would sit still across every rebuild, which is the +/// entire thing item 167 is about. It defines QTMAILDIR_BUILD_NUMBER only in +/// a dev build. +#include "buildnumber.h" + +#ifdef QTMAILDIR_BUILD_NUMBER +# define QTMAILDIR_VERSION_DISPLAY QTMAILDIR_VERSION " build " QTMAILDIR_BUILD_NUMBER +#else +# define QTMAILDIR_VERSION_DISPLAY QTMAILDIR_VERSION +#endif diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 21c3661..f76ff70 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -260,6 +260,7 @@ private slots: void narrowingAnEmptyQueryBarIsAPlainSearch(); void aMalformedAccountIsReportedWithoutBlockingTheConstructor(); void aWorkerBackedWindowReturnsRealThreads(); + void aPurgeTakesTheRowsOutOfTheViewWithoutARefresh(); // Compose and send, item 123 task 12. void theMailRootComesFromTheConfigNotTheIndex(); @@ -383,6 +384,15 @@ private slots: void editTagsOnAReplyCountsItsOwnThreadNotTheFirstInTheList(); void markCurrentThreadReadResolvesTheThreadThroughTheIndex(); void deletingAReplyRepaintsThatReplyRow(); + void deleteIsHiddenOnMailAlreadyInTheTrash(); + void restoreIsHiddenOnMailThatWasNeverDeleted(); + void deleteAlsoMarksTheMessageRead(); + void emptyTrashAsksBeforeDestroyingAnything(); + void theUnreadLabelSaysWhichDirectionItWillGo(); + void theUnreadLabelFollowsAWriteWithoutReselecting(); + void theUnreadActionIsHiddenOnAMixedSelection(); + void markThreadUnreadReachesAMixedThread(); + void markThreadReadAndUnreadAreSeparateActions(); void toggleUnreadOnAReplyReadsTheReplysOwnState(); void toggleUnreadOnAReplyRepaintsItInBothDirections(); void taggingTheOpenReplyUpdatesTheMessagePaneStrip(); @@ -5207,6 +5217,408 @@ void TestMainWindow::deletingAReplyRepaintsThatReplyRow() "deleting one reply marked its whole thread deleted"); } +/// A window whose one account owns `acct/`, with its trash at `acct/trash`. +/// +/// Delete and Restore both ask about a row's PATH, so a test for either needs +/// a config that says which prefix is a trash folder. Bare-window tests carry +/// no account at all and would answer "not in the trash" for every row. +static Config configWithTrash(QTemporaryDir &dir) +{ + const QString path = dir.filePath(QStringLiteral("qtmaildir.conf")); + QFile file(path); + if (file.open(QIODevice::WriteOnly | QIODevice::Text)) { + QTextStream out(&file); + out << "[account.acct]\n" + << "maildir = acct\n" + << "trash = trash\n" + << "inbox = inbox\n"; + } + Config config; + config.load(path); + return config; +} + +/// One thread row whose displayed message sits at `filePath`. +static ThreadSummary threadAtPath(const QString &id, const QString &filePath, + const QStringList &tags = {}) +{ + ThreadSummary thread = makeThread(id, tags); + thread.firstMessagePath = filePath; + thread.firstMessageTags = tags; + return thread; +} + +void TestMainWindow::deleteIsHiddenOnMailAlreadyInTheTrash() +{ + // Item 168, from the user: "I noticed I can hit delete via context menu on + // a message already in the trash." + // + // It was not dangerous, which is the part that made it survive: the file + // is already in the destination, so moveMessages() takes its + // already-there branch, reports the message as moved and counts an + // unsynced change for a move that never happened. The menu claimed to + // have done something and nothing had. + // + // The question is about the PATH, never the `deleted` TAG: a message + // trashed by another client carries no such tag, which is why item 103 + // made the trash view path-based, and asking the tag would offer Delete on + // exactly the mail a trash view is full of. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const Config config = configWithTrash(dir); + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *deleteAction = + window.findChild<QAction *>(QStringLiteral("delete")); + QVERIFY(deleteAction); + + model->appendBatch({ + threadAtPath(QStringLiteral("t1"), + QStringLiteral("acct/inbox/cur/1:2,S")), + threadAtPath(QStringLiteral("t2"), + QStringLiteral("acct/trash/cur/2:2,S")), + }); + + view->setCurrentIndex(model->index(0, 0, {})); + QVERIFY2(deleteAction->isVisible(), + "Delete is hidden on mail that is NOT in the trash, so this test " + "cannot tell the two cases apart"); + + view->setCurrentIndex(model->index(1, 0, {})); + QVERIFY2(!deleteAction->isVisible(), + "Delete is still offered on a message already in the trash, " + "where it reports success and does nothing"); + + // A folder whose name STARTS with the trash folder's is a different + // folder. Without the trailing separator `acct/trash-old` matches + // `acct/trash` and Delete silently disappears from mail that was never + // trashed, which is the quiet half of the same mistake. + model->appendBatch({ threadAtPath(QStringLiteral("t3"), + QStringLiteral("acct/trash-old/cur/3:2,S")) }); + view->setCurrentIndex(model->index(2, 0, {})); + QVERIFY2(deleteAction->isVisible(), + "Delete vanished on mail in acct/trash-old, which is not the " + "trash: the prefix was compared without its separator"); +} + +void TestMainWindow::restoreIsHiddenOnMailThatWasNeverDeleted() +{ + // The mirror, shipped beside it: `restore` was added unconditionally to + // both menus, so it was offered on mail that was never deleted, where it + // has as little meaning as Delete has in the trash. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const Config config = configWithTrash(dir); + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *restore = window.findChild<QAction *>(QStringLiteral("restore")); + QVERIFY(restore); + + model->appendBatch({ + threadAtPath(QStringLiteral("t1"), + QStringLiteral("acct/inbox/cur/1:2,S")), + threadAtPath(QStringLiteral("t2"), + QStringLiteral("acct/trash/cur/2:2,S")), + }); + + view->setCurrentIndex(model->index(1, 0, {})); + QVERIFY2(restore->isVisible(), "Restore is hidden on trashed mail"); + + view->setCurrentIndex(model->index(0, 0, {})); + QVERIFY2(!restore->isVisible(), + "Restore is still offered on mail that was never deleted"); +} + +void TestMainWindow::deleteAlsoMarksTheMessageRead() +{ + // The user's second request on the same tangent: "messages moved to the + // trash should be automatically marked -unread". Deleting is a decision + // about the message, so the unread count must not go on including what + // the user threw away. + // + // Asserted on the undo TEXT and depth rather than on the tags: the write + // is a move, which a bare window cannot complete, but the tag change it + // composes is pushed as one command either way. One command, not two, is + // the property that matters: undo has to return the folder AND the tag + // together. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const Config config = configWithTrash(dir); + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + model->appendBatch({ threadAtPath(QStringLiteral("t1"), + QStringLiteral("acct/inbox/cur/1:2,S"), + { QStringLiteral("unread") }) }); + const QModelIndex row = model->index(0, 0, {}); + view->setCurrentIndex(row); + + QVERIFY2(model->threadFor(row).isUnread(), + "the fixture is already read, so this test cannot see the tag go"); + + auto *deleteAction = window.findChild<QAction *>(QStringLiteral("delete")); + QVERIFY(deleteAction); + deleteAction->trigger(); + + QVERIFY2(!model->threadFor(row).isUnread(), + "Delete left the message unread in the trash"); +} + +void TestMainWindow::emptyTrashAsksBeforeDestroyingAnything() +{ + // Item 118, and the one place this application asks. CLAUDE.md rules out + // confirmation dialogs for mutations because every mutation pushes its + // inverse onto the undo stack; a purge has no inverse, so the rule does + // not reach it. What the rule protects is that a user never loses work to + // a keystroke, and here the dialog is what provides that rather than + // contradicting it. + // + // Asserting the action EXISTS and is wired, not the dialog's buttons: a + // modal cannot be driven from a test without blocking it (item 84), so + // the dialog itself is a hand test. What is pinned here is that nothing + // is destroyed without going through it. + const Config config; + MainWindow window(config); + + auto *action = window.findChild<QAction *>(QStringLiteral("empty_trash")); + QVERIFY2(action, "empty_trash does not exist"); + + // Reachable from a menu, which everyActionIsReachableFromAMenu() also + // enforces globally. Named here as well because an unreachable purge is + // worse than an unreachable anything else: the user cannot discover the + // action, but a stray keybinding still runs it. + bool found = false; + const QList<QMenu *> menus = window.findChildren<QMenu *>(); + for (QMenu *menu : menus) { + if (menu->actions().contains(action)) { + found = true; + break; + } + } + QVERIFY2(found, "empty_trash is in no menu"); + + // No shortcut, deliberately: this is the one irreversible action, and a + // chord is exactly how it would be run by accident. + QVERIFY2(action->shortcut().isEmpty(), + qPrintable(QStringLiteral("empty_trash carries the shortcut %1; " + "the one irreversible action must not " + "be a keystroke away") + .arg(action->shortcut().toString()))); +} + +void TestMainWindow::theUnreadLabelSaysWhichDirectionItWillGo() +{ + // The user's note: "the label for toggle unread should be dynamic. On an + // unread message it should be Mark as read, on a read message Mark as + // unread." + // + // "Toggle unread" reads the same whichever way it will go, so the only + // way to learn what it does is to press it and look. The action stays a + // toggle, because one message has a real two-valued state; what changes + // is that the label tells the truth about the direction it has chosen. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *action = window.findChild<QAction *>(QStringLiteral("toggle_unread")); + QVERIFY(action); + + model->appendBatch({ makeThread(QStringLiteral("t1"), + { QStringLiteral("unread") }), + makeThread(QStringLiteral("t2"), {}) }); + + view->setCurrentIndex(model->index(0, 0, {})); + QVERIFY2(action->text().contains(QStringLiteral("read")), + qPrintable(action->text())); + QVERIFY2(!action->text().contains(QStringLiteral("unread")), + qPrintable(QStringLiteral("an UNREAD row must offer Mark as " + "read, not: %1").arg(action->text()))); + + view->setCurrentIndex(model->index(1, 0, {})); + QVERIFY2(action->text().contains(QStringLiteral("unread")), + qPrintable(QStringLiteral("a READ row must offer Mark as unread, " + "not: %1").arg(action->text()))); +} + +void TestMainWindow::theUnreadLabelFollowsAWriteWithoutReselecting() +{ + // The label describes the selection's STATE, and a write moves that state + // without touching the selection. Marking the current row read has to + // leave the entry offering "Mark as unread" on the same row, or the menu + // offers to do again what was just done. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *action = window.findChild<QAction *>(QStringLiteral("toggle_unread")); + QVERIFY(action); + + model->appendBatch({ makeThread(QStringLiteral("t1"), + { QStringLiteral("unread") }) }); + view->setCurrentIndex(model->index(0, 0, {})); + QVERIFY2(action->text().contains(QStringLiteral("read")) + && !action->text().contains(QStringLiteral("unread")), + qPrintable(action->text())); + + action->trigger(); + + QVERIFY2(action->text().contains(QStringLiteral("unread")), + qPrintable(QStringLiteral("the label did not follow the write: " + "still offering %1 on a row it just " + "marked read").arg(action->text()))); +} + +void TestMainWindow::theUnreadActionIsHiddenOnAMixedSelection() +{ + // The other half of the same note: "on a thread with mixed states it + // should be hidden, we have a submenu for thread actions". + // + // A selection spanning an unread row and a read one has no single state, + // so no honest label exists for it. Hiding the entry sends the user to + // the thread submenu, whose entries are absolute and work regardless of + // the mix. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *action = window.findChild<QAction *>(QStringLiteral("toggle_unread")); + QVERIFY(action); + + model->appendBatch({ makeThread(QStringLiteral("t1"), + { QStringLiteral("unread") }), + makeThread(QStringLiteral("t2"), {}) }); + + // From a row that is already current, and NOT via selectAll(): a fresh + // selectAll emits no currentRowChanged at all and leaves the current + // index invalid, so a test using it passes against a missing guard + // (CLAUDE.md). + view->setCurrentIndex(model->index(0, 0, {})); + QVERIFY2(action->isVisible(), "a single row already has no single state"); + + view->selectionModel()->select( + model->index(1, 0, {}), + QItemSelectionModel::Select | QItemSelectionModel::Rows); + QCOMPARE(view->selectionModel()->selectedRows().size(), 2); + + QVERIFY2(!action->isVisible(), + qPrintable(QStringLiteral("a mixed selection still offers the " + "unread action, labelled: %1") + .arg(action->text()))); + + // ...and it comes back when the selection agrees again, or the entry + // would be gone for the rest of the session. + view->selectionModel()->select( + model->index(1, 0, {}), + QItemSelectionModel::Deselect | QItemSelectionModel::Rows); + QVERIFY2(action->isVisible(), + "the action did not return when the selection agreed again"); +} + +void TestMainWindow::markThreadUnreadReachesAMixedThread() +{ + // Item 112. The user's report: on a thread with two unread replies, asking + // to mark the whole thread unread marked it READ instead. + // + // ThreadSummary::tags is notmuch's UNION over the conversation, so a + // thread containing even one unread message answers "unread" and a toggle + // reading that predicate always picks "mark read". There was no input that + // could reach "mark thread unread" on a mixed thread: the only threads + // taking that branch were the ones already entirely read. + // + // A union is not a state. The fix is two fixed-direction actions, so this + // asserts the direction rather than the resulting tags: on a mixed thread + // BOTH directions are reachable, which is the property that was missing. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + // MIXED: the union carries `unread` because some message is unread, while + // others are not. A thread whose messages are all in one state answers + // identically whichever way the direction is computed, so a uniform + // fixture passes against the bug (CLAUDE.md, item 88's opposite-states + // requirement). + model->appendBatch({ makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }) }); + const QModelIndex thread = model->index(0, 0, {}); + QVERIFY(thread.isValid()); + QVERIFY2(model->threadFor(thread).isUnread(), + "the fixture's union does not carry unread, so this test cannot " + "reach the branch the defect lives in"); + view->setCurrentIndex(thread); + + auto *markUnread = + window.findChild<QAction *>(QStringLiteral("mark_thread_unread")); + QVERIFY2(markUnread, "mark_thread_unread does not exist: the thread toggle " + "was not split, so a mixed thread still has no way to " + "be marked unread"); + markUnread->trigger(); + + QVERIFY2(window.undoTextForTesting().contains(QStringLiteral("unread")), + qPrintable(QStringLiteral("wrong direction on a mixed thread: %1") + .arg(window.undoTextForTesting()))); + QVERIFY2(!window.undoTextForTesting().contains(QStringLiteral("Mark thread read")), + qPrintable(QStringLiteral("marked the thread READ when asked to " + "mark it unread: %1") + .arg(window.undoTextForTesting()))); +} + +void TestMainWindow::markThreadReadAndUnreadAreSeparateActions() +{ + // The other half: the read direction must still be reachable, and must be + // its own action rather than the same one answering differently. Both are + // asserted on the SAME mixed thread, which a toggle cannot do: whichever + // direction it picks, the other is unreachable there. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + model->appendBatch({ makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }) }); + const QModelIndex thread = model->index(0, 0, {}); + view->setCurrentIndex(thread); + + auto *markRead = + window.findChild<QAction *>(QStringLiteral("mark_thread_read")); + QVERIFY(markRead); + markRead->trigger(); + QVERIFY2(window.undoTextForTesting().contains(QStringLiteral("Mark thread read")), + qPrintable(window.undoTextForTesting())); + + // The old toggle must be gone rather than left beside its replacements, + // which would leave the defect reachable from the menu it still sat in. + QVERIFY2(!window.findChild<QAction *>(QStringLiteral("toggle_unread_thread")), + "toggle_unread_thread still exists beside the split actions"); +} + void TestMainWindow::toggleUnreadOnAReplyReadsTheReplysOwnState() { // The user's report: "read/unread still doesn't trigger a repaint of the @@ -5551,7 +5963,8 @@ void TestMainWindow::theThreadSubmenuIsReachableFromBothMenus() QStringLiteral("archive_thread"), QStringLiteral("delete_thread"), QStringLiteral("spam_thread"), - QStringLiteral("toggle_unread_thread"), + QStringLiteral("mark_thread_read"), + QStringLiteral("mark_thread_unread"), QStringLiteral("flag_thread"), }; @@ -7480,7 +7893,8 @@ void TestMainWindow::noTwoActionsShareAnIcon() QStringLiteral("archive_thread"), QStringLiteral("delete_thread"), QStringLiteral("spam_thread"), - QStringLiteral("toggle_unread_thread"), + QStringLiteral("mark_thread_read"), + QStringLiteral("mark_thread_unread"), QStringLiteral("flag_thread"), QStringLiteral("reply_no_quote"), }; @@ -8458,6 +8872,47 @@ void TestMainWindow::aWorkerBackedWindowReturnsRealThreads() QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); } +void TestMainWindow::aPurgeTakesTheRowsOutOfTheViewWithoutARefresh() +{ + // Found by hand: the mail was destroyed correctly and the list went on + // showing it until the user re-ran the query themselves. + // + // A purge is the one mutation with no optimistic update to apply. Every + // other one CHANGES a row, so the model can rewrite it in place; this one + // takes the row away entirely, and the only honest view afterwards is the + // one the query gives now. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/trash"), QStringLiteral("doomed@example.org"), + QStringLiteral("A subject"), QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body text."))); + QVERIFY2(backed.buildWithAccounts({ { QStringLiteral("acct"), + QStringLiteral("acct"), + QStringLiteral("trash"), + {}, {}, {} } }), + qPrintable(backed.error())); + + MainWindow window(backed.config()); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *queryEdit = + window.findChild<QLineEdit *>(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + + queryEdit->setText(QStringLiteral("path:\"acct/trash/**\"")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + + // Straight to the purge, bypassing the confirmation: a modal cannot be + // driven from a test without blocking it (item 84), and what is under + // test is what happens AFTER the user has confirmed. + window.purgeForTesting({ QStringLiteral("doomed@example.org") }); + + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 0, 15000); +} + namespace { /// A worker-backed window with one message in one account's maildir. diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index e1a21cd..3f75898 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -91,6 +91,11 @@ private slots: void moveMessagesKeepsTheMessagesTags(); void moveMessagesReportsOnlyWhatMoved(); void moveMessagesGivesTheFileAFreshMaildirName(); + void purgeMessagesDeletesTheFileAndTheIndexEntry(); + void purgeMessagesReportsWhatItDestroyed(); + void purgeMessagesLeavesOtherMessagesAlone(); + void purgeMessagesDoesNotClaimAnIdItCouldNotDelete(); + void resolveQueryMessagesRefusesAnEmptyQuery(); void moveMessagesKeepsTheMaildirFlags(); void moveMessagesRecoversWhenASyncRenamedTheFile(); void moveMessagesStillReportsAMessageThatIsReallyGone(); @@ -1285,6 +1290,132 @@ void TestNotmuchWorker::moveMessagesRelocatesTheFile() QVERIFY(!QFile::exists(before)); } +void TestNotmuchWorker::purgeMessagesDoesNotClaimAnIdItCouldNotDelete() +{ + // The report drives what the UI tells the user, and the one number they + // will remember about an irreversible action is how much it destroyed. An + // id whose file the database names but that is not on disk contributes + // nothing: the index entry is still cleaned up, but claiming it as + // destroyed would overstate what happened. + const QString real = QStringLiteral("purge6@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), real), + qPrintable(m_fixture.error())); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy purged(&worker, &NotmuchWorker::messagesPurged); + + // A KNOWN id whose file is already gone, which is the case that reaches + // the removal loop and finds nothing to unlink. An unknown id is skipped + // far earlier and proves nothing about it. + const QString stale = QStringLiteral("purge7@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), stale), + qPrintable(m_fixture.error())); + const QString staleFile = fileOf(stale); + QVERIFY(!staleFile.isEmpty()); + QVERIFY(QFile::remove(staleFile)); + + worker.purgeMessages({ real, stale }); + + QCOMPARE(purged.size(), 1); + const QStringList reported = purged.first().at(0).toStringList(); + QVERIFY2(reported.contains(real), qPrintable(reported.join(QLatin1Char(',')))); + QVERIFY2(!reported.contains(stale), + "claimed to have destroyed a message whose file was already gone"); +} + +void TestNotmuchWorker::resolveQueryMessagesRefusesAnEmptyQuery() +{ + // An EMPTY query means "match everything" to notmuch, and this walk is + // what Empty Trash enumerates from. An account with no trash folder + // configured produces an empty query, so without this guard the dialog + // would offer to destroy the entire Maildir and say so accurately. + QVERIFY2(addMovableMessage(QStringLiteral("trash"), + QStringLiteral("empty1@example.org")), + qPrintable(m_fixture.error())); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy resolved(&worker, &NotmuchWorker::threadMessagesResolved); + + worker.resolveQueryMessages(QString(), QStringLiteral("purge")); + QCOMPARE(resolved.size(), 0); +} + +void TestNotmuchWorker::purgeMessagesDeletesTheFileAndTheIndexEntry() +{ + // Item 118. The one destructive action in this application: the file is + // removed from disk and the message from the index, with no undo. Both + // halves are asserted, because either one alone leaves a visible defect: + // a file without an index entry is invisible mail on disk, and an index + // entry without a file is a row that opens onto nothing. + const QString id = QStringLiteral("purge1@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), id), + qPrintable(m_fixture.error())); + + const QString before = fileOf(id); + QVERIFY(!before.isEmpty()); + QVERIFY(QFile::exists(before)); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + + worker.purgeMessages({ id }); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + + QVERIFY2(!QFile::exists(before), qPrintable(before)); + QCOMPARE(runQuery(QStringLiteral("id:%1").arg(id)).size(), 0); +} + +void TestNotmuchWorker::purgeMessagesReportsWhatItDestroyed() +{ + // The count the confirmation named has to be the count that happened, and + // the UI has nothing else to report from: unlike a move, there is no new + // path to observe afterwards. + const QString first = QStringLiteral("purge2@example.org"); + const QString second = QStringLiteral("purge3@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), first), + qPrintable(m_fixture.error())); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), second), + qPrintable(m_fixture.error())); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy purged(&worker, &NotmuchWorker::messagesPurged); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + + worker.purgeMessages({ first, second }); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + + QCOMPARE(purged.size(), 1); + QStringList reported = purged.first().at(0).toStringList(); + reported.sort(); + QCOMPARE(reported, (QStringList{ first, second })); +} + +void TestNotmuchWorker::purgeMessagesLeavesOtherMessagesAlone() +{ + // The blast radius. A purge names ids, and nothing outside that list may + // be touched: this is the action with no undo, so an over-reach is not + // recoverable. The survivor is in the SAME folder, which is where a + // folder-wide delete would take everything with it. + const QString doomed = QStringLiteral("purge4@example.org"); + const QString survivor = QStringLiteral("purge5@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), doomed), + qPrintable(m_fixture.error())); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), survivor), + qPrintable(m_fixture.error())); + + const QString survivorFile = fileOf(survivor); + QVERIFY(!survivorFile.isEmpty()); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + worker.purgeMessages({ doomed }); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + + QCOMPARE(runQuery(QStringLiteral("id:%1").arg(doomed)).size(), 0); + QCOMPARE(runQuery(QStringLiteral("id:%1").arg(survivor)).size(), 1); + QVERIFY2(QFile::exists(survivorFile), qPrintable(survivorFile)); +} + void TestNotmuchWorker::moveMessagesReindexesAtTheNewPath() { // The half a filesystem check cannot see. A moved file with a stale index diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index 913d151..22f8485 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -499,6 +499,14 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Aggiunge o rimuove l'etichetta deleted</translation> </message> <message> + <source>Mark thread &read</source> + <translation>Segna conversazione come &letta</translation> + </message> + <message> + <source>Remove the unread tag from every message of the selected threads</source> + <translation>Rimuove il tag unread da ogni messaggio delle conversazioni selezionate</translation> + </message> + <message> <source>Re&ply</source> <translation>Ris&pondi</translation> </message> @@ -544,6 +552,41 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> </translation> </message> <message> + <source>No trash folder is configured</source> + <translation>Nessuna cartella cestino configurata</translation> + </message> + <message> + <source>Not connected to the mail index</source> + <translation>Non connesso all'indice della posta</translation> + </message> + <message> + <source>The trash is already empty</source> + <translation>Il cestino è già vuoto</translation> + </message> + <message> + <source>every account</source> + <translation>ogni account</translation> + </message> + <message> + <source>Empty trash</source> + <translation>Svuota cestino</translation> + </message> + <message numerus="yes"> + <source>Permanently delete %n message(s) from the trash of %1?</source> + <translation> + <numerusform>Eliminare definitivamente %n messaggio dal cestino di %1?</numerusform> + <numerusform>Eliminare definitivamente %n messaggi dal cestino di %1?</numerusform> + </translation> + </message> + <message> + <source>This cannot be undone.</source> + <translation>Questa operazione non può essere annullata.</translation> + </message> + <message> + <source>Delete permanently</source> + <translation>Elimina definitivamente</translation> + </message> + <message> <source>Mail tagged deleted but not in a trash folder. Select what should go and press Delete.</source> <translation>Posta etichettata come eliminata ma non in un cestino. Seleziona cosa deve essere rimosso e premi Elimina.</translation> </message> @@ -744,6 +787,14 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Mostra la posta etichettata come eliminata che non si trova in un cestino</translation> </message> <message> + <source>Empt&y trash...</source> + <translation>S&vuota cestino...</translation> + </message> + <message> + <source>Permanently delete every message in the trash</source> + <translation>Elimina definitivamente ogni messaggio nel cestino</translation> + </message> + <message> <source>Add spam and remove inbox on whole threads</source> <translation>Aggiunge spam e rimuove inbox su intere conversazioni</translation> </message> @@ -752,14 +803,18 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Segna conversazione come spam</translation> </message> <message> - <source>Toggle the unread tag on whole threads</source> - <translation>Inverte l'etichetta non letto su intere conversazioni</translation> - </message> - <message> <source>Mark thread read</source> <translation>Segna conversazione come letta</translation> </message> <message> + <source>Mark thread &unread</source> + <translation>Segna conversazione come &non letta</translation> + </message> + <message> + <source>Add the unread tag to every message of the selected threads</source> + <translation>Aggiunge il tag unread a ogni messaggio delle conversazioni selezionate</translation> + </message> + <message> <source>Mark thread unread</source> <translation>Segna conversazione come non letta</translation> </message> @@ -1031,6 +1086,13 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation><h3>qtmaildir %1</h3><p>Un client di posta Qt6 per Maildir indicizzate con notmuch.</p><p>Legge e organizza la posta locale. Scaricamento e invio sono affidati a script esterni.</p><p>Copyright &copy; 2026 Danilo M. &lt;danix@danix.xyz&gt;<br>Distribuito secondo la GNU General Public License versione 2.</p><p>Sviluppato con l'assistenza dell'IA. Tutto il codice è riveduto, testato e curato dal manutentore.</p></translation> </message> <message numerus="yes"> + <source>Deleted %n message(s) permanently</source> + <translation> + <numerusform>%n messaggio eliminato definitivamente</numerusform> + <numerusform>%n messaggi eliminati definitivamente</numerusform> + </translation> + </message> + <message numerus="yes"> <source>%n unread</source> <translation> <numerusform>%n non letto</numerusform> @@ -1164,6 +1226,22 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <numerusform>%1: %n conversazioni</numerusform> </translation> </message> + <message> + <source>Mark as &read</source> + <translation>Segna come &letto</translation> + </message> + <message> + <source>Remove the unread tag from the selection</source> + <translation>Rimuove il tag unread dalla selezione</translation> + </message> + <message> + <source>Mark as &unread</source> + <translation>Segna come &non letto</translation> + </message> + <message> + <source>Add the unread tag to the selection</source> + <translation>Aggiunge il tag unread alla selezione</translation> + </message> <message numerus="yes"> <source>1 thread selected (%n message(s))</source> <translation> |