From ff76a8fc6c050de121a277faa1937eeec9ac4641 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 20 Aug 2026 18:36:40 +0200 Subject: docs: document the new config keys in the plan's close-out, item 123 Task 2's code review found that the README's sample config documents every other key, including recently added ones, and has nothing for send_command or the [compose] section. Without it those keys ship undiscoverable: a user has no way to learn that sending exists at all. That is a gap in the plan rather than a deviation by the task, since no task claimed the README, so it becomes a step in the close-out where the rest of the documentation is written. --- docs/superpowers/plans/2026-08-20-compose-and-send.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'docs/superpowers/plans') diff --git a/docs/superpowers/plans/2026-08-20-compose-and-send.md b/docs/superpowers/plans/2026-08-20-compose-and-send.md index 195dacc..1478f59 100644 --- a/docs/superpowers/plans/2026-08-20-compose-and-send.md +++ b/docs/superpowers/plans/2026-08-20-compose-and-send.md @@ -4787,6 +4787,21 @@ replace. - Modify: `docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md` - Modify: `docs/superpowers/specs/2026-08-20-compose-and-send-design.md` +- [ ] **Step 0: Document the new keys in the README** + +Found during Task 2's code review and assigned here rather than there. The +README's sample config at `README.md:150-215` documents EVERY other +configuration key, including recently added ones, and has nothing for +`send_command` or the `[compose]` section. Without this the keys ship +undiscoverable: a user has no way to learn that sending exists. + +Take the block from the spec at +`docs/superpowers/specs/2026-08-20-compose-and-send-design.md:552-560` and +adapt it to the README's existing commented style, showing `send_command` in +an account section and every `[compose]` key with its default. Say plainly +that an account without `send_command` is receive-only, since that is the +part no reader would guess. + - [ ] **Step 1: Add the changelog entry** Under `## [Unreleased]`, in the existing `### Added` section or a new one: -- cgit v1.2.3 From e35f2da3fd9c5afbbea85b432f26810c17e8318e Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 20 Aug 2026 18:54:50 +0200 Subject: docs: record that build() blocks the caller, item 123 Task 4's code review measured it: a large attachment is read and base64 encoded on the calling thread, and autosave calls build() from a GUI-thread timer. The directory hang that review found is fixed, but the blocking read is by design and will be felt in the composer. Recorded against Task 11 rather than fixed, because nothing in the composer crosses the worker boundary and adding a second threading model for one call is worse than the stall. --- docs/superpowers/plans/2026-08-20-compose-and-send.md | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'docs/superpowers/plans') diff --git a/docs/superpowers/plans/2026-08-20-compose-and-send.md b/docs/superpowers/plans/2026-08-20-compose-and-send.md index 1478f59..e7eddfa 100644 --- a/docs/superpowers/plans/2026-08-20-compose-and-send.md +++ b/docs/superpowers/plans/2026-08-20-compose-and-send.md @@ -3871,6 +3871,16 @@ popup between stages." ### Task 11: ComposeWindow +**Found during Task 4's code review, and it lands here.** `MessageBuilder::build()` +is SYNCHRONOUS and can block: a large attachment is read and base64-encoded on +the calling thread. Autosave calls it on a timer, on the GUI thread, so a +30-second debounce that hits a 25MB attachment stalls typing. The directory +hang that review found is fixed in `MessageBuilder`, but the blocking read +remains by design. Do not move it to a thread as part of this task, since +nothing here crosses the worker boundary and adding a second threading model +for one call is worse than the stall. Note it in a comment at the autosave call +site so the next person measuring a freeze knows where to look. + The only unit here that owns widgets, and the one that composes the other four. It contains no MIME and no process logic: a composer bug and a MIME bug are found in different files. -- cgit v1.2.3 From 6488810c779970094b86079c8688d83d8529fab0 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 21 Aug 2026 10:12:26 +0200 Subject: feat(compose): hand outgoing mail to the send command, item 123 MessageSender runs the configured command with the message on stdin and judges the result by its exit status alone. Nothing here waits on the event loop, so a send does not block the GUI thread; a 1.6MB payload was probed through a reading stub without deadlocking the pipe buffer. The command is split and passed to QProcess as a program and an argument list, never through a shell. A test asserts that by giving the command shell metacharacters and checking that the marker file a shell would have created does not exist, so the property fails a mutation rather than resting on a comment. Four corrections to the plan's draft. splitCommand handles double quotes only, so a single-quoted argument splits wrongly and the header now says so. A crashing command delivers finished(11, CrashExit) and would have been reported as "exited with status 11", so a crash branch was added. A command that exits without draining a large stdin emits WriteError before finished(), which the draft handled correctly and by luck, untested. And an empty send_command is checked after trimming. Two contract gaps found in review, both about what this class promises rather than what it does. The exactly-once guarantee covers the EMIT, not what a caller receives: a long-lived sender plus a connect() inside each send accumulates receivers, and the second result then runs the first send's lambda too, filing a sent copy of the wrong message. The header now scopes the promise and requires Qt::SingleShotConnection. The plan's Task 11 call site already had that flag, sixty-nine lines below the connect and outside anything a reader would see, so the plan gained a note where someone retyping it will read it. And destruction mid-send killed the command with no report, announced only by a Qt warning: a live SMTP conversation abandoned, possibly partially delivered, while the user believes it was cancelled. The destructor now closes stdin, waits a bounded five seconds, and only then kills. It emits nothing either way, because the outcome after a kill is genuinely unknown and reporting "not sent" for a message that may have gone out is the mailsync.sh mistake pointing the other way. Claiming m_reported before kill() is what makes that true, since kill() delivers finished(CrashExit), which would otherwise emit exactly that untruth. No timeout on the send itself: killing a slow but working send is worse than waiting. Task 10 owns the popup, and deliberately offers no cancel after commit, so this class promises none either. Also refreshes the translations Task 5 left out. That gap was invisible because test_translations builds its rows from the .ts file, so a string that never entered it is never asserted on. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QP2g3b3kuLx6AYFCNEz6UR --- .../plans/2026-08-20-compose-and-send.md | 28 ++ src/CMakeLists.txt | 1 + src/messagesender.cpp | 197 ++++++++ src/messagesender.h | 164 +++++++ tests/CMakeLists.txt | 1 + tests/test_messagesender.cpp | 532 +++++++++++++++++++++ translations/qtmaildir_it_IT.ts | 27 ++ 7 files changed, 950 insertions(+) create mode 100644 src/messagesender.cpp create mode 100644 src/messagesender.h create mode 100644 tests/test_messagesender.cpp (limited to 'docs/superpowers/plans') diff --git a/docs/superpowers/plans/2026-08-20-compose-and-send.md b/docs/superpowers/plans/2026-08-20-compose-and-send.md index e7eddfa..7d5f6f1 100644 --- a/docs/superpowers/plans/2026-08-20-compose-and-send.md +++ b/docs/superpowers/plans/2026-08-20-compose-and-send.md @@ -4008,6 +4008,20 @@ private: `src/composewindow.cpp`. The full file is long; these are the parts that carry decisions, and the rest is ordinary widget assembly. +**One thing in this block is load-bearing and easy to drop while retyping it: +the `Qt::SingleShotConnection` on the `MessageSender::finished` connect inside +the `committed` handler.** `m_sender` is a long-lived member, so a plain +`connect()` beside a `send()` call leaks a receiver per send and the second +result runs every earlier lambda, each still holding an earlier message's bytes +by value: a sent copy of the wrong message, and `accept()` on a destroyed +dialog. `MessageSender`'s own once-only guard cannot help, because that guards +the emit and this is one emit reaching many receivers. The header for +`MessageSender::finished` states the rule and +`test_messagesender.cpp::aPerSendConnectionMustBeSingleShot` measures it (3 +deliveries for 2 sends without the flag, 2 with it). Noted here because the +plan's code blocks are drafts and this is the line whose absence still +compiles, still runs, and is wrong only on the second send. + ```cpp #include "composewindow.h" @@ -4169,6 +4183,20 @@ void ComposeWindow::send() connect(dialog, &SendDialog::committed, this, [this, dialog, built, account]() { m_sender->send(account.sendCommand, built.bytes); + // Qt::SingleShotConnection IS REQUIRED HERE, and this line is the + // correction of a defect that was in this plan's draft (found while + // building Task 6, 2026-08-21). m_sender is a long-lived member, so a + // bare connect() beside each send() accumulates a permanent receiver + // per send. Send, fail, correct the recipient, send again, and the + // second result runs BOTH lambdas: the first still holds the FIRST + // message's `built` and `account` by value, so it files a sent copy of + // the wrong message and calls accept() on a dialog it already + // deleteLater()'d. MessageSender's m_reported guard cannot prevent + // this: it collapses two QProcess signals into one emit, and this is + // one emit reaching many receivers. Measured in + // test_messagesender.cpp::aPerSendConnectionMustBeSingleShot, where + // the bare shape delivers 3 results for 2 sends and the single-shot + // shape delivers 2. connect(m_sender, &MessageSender::finished, this, [this, dialog, built, account](bool sent, const QString &error) { if (!sent) { diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index a4d7c55..eac2fab 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -14,6 +14,7 @@ add_library(qtmaildir_lib STATIC notmuchworker.cpp maildirname.cpp draftstore.cpp + messagesender.cpp tagchip.cpp tagcolors.cpp savequerydialog.cpp diff --git a/src/messagesender.cpp b/src/messagesender.cpp new file mode 100644 index 0000000..f336028 --- /dev/null +++ b/src/messagesender.cpp @@ -0,0 +1,197 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include "messagesender.h" + +MessageSender::MessageSender(QObject *parent) + : QObject(parent) +{ + // Separate channels, unlike MailSync's MergedChannels: there is no log + // pane to fill here, and stderr alone is what a failure has to report. + // Merging them would put the command's ordinary chatter into the error + // message shown for a rejected send. + m_process.setProcessChannelMode(QProcess::SeparateChannels); + + connect(&m_process, &QProcess::finished, + this, &MessageSender::handleFinished); + connect(&m_process, &QProcess::errorOccurred, + this, &MessageSender::handleError); +} + +MessageSender::~MessageSender() +{ + if (m_process.state() == QProcess::NotRunning) + return; + + // A send is a live SMTP conversation and abandoning one has a genuinely + // unknown outcome, so give the command a bounded chance to finish rather + // than killing it outright. Measured: without this, a one-second command + // destroyed 100ms in is killed and its work does not complete, announced + // only by a Qt warning on stderr. With it, the same command completes and + // the destructor costs the ~1s the command actually needed. + // + // The write channel is closed first because the command may still be + // reading: a command blocked on stdin would otherwise never reach EOF and + // would burn the whole timeout for no reason. + m_process.closeWriteChannel(); + if (m_process.waitForFinished(kShutdownWaitMs)) + return; + + // Still running. A destructor cannot block a quitting application forever, + // so the process is killed deliberately here rather than by ~QProcess. + // + // NOTHING IS EMITTED. The outcome after a kill is unknown: the message may + // have been fully delivered, partially delivered, or not sent at all, and + // this class reports two outcomes only. Emitting finished(false, ...) would + // report "not sent" for a message that may well have been, which is the + // mailsync.sh mistake pointing the other way. Emitting finished(true, ...) + // would be worse. A caller that must know has to keep this object alive + // until finished() arrives. + // + // Claiming the report BEFORE the kill is what makes that true, and it is + // not optional: kill() makes QProcess deliver finished(CrashExit), which + // reaches handleFinished and would emit exactly the untruthful "not sent" + // this comment forbids. Measured, by a test that failed against the + // version without these two lines. This is also the one place m_reported + // does live work, rather than the defence-in-depth it is on the signal + // paths. + m_reported = true; + m_process.kill(); + m_process.waitForFinished(kShutdownWaitMs); +} + +bool MessageSender::isRunning() const +{ + return m_process.state() != QProcess::NotRunning; +} + +bool MessageSender::send(const QString &command, const QByteArray &bytes) +{ + if (command.trimmed().isEmpty() || isRunning()) + return false; + + // splitCommand gives an argument list; running through a shell would make + // every recipient address, display name and config value a potential + // injection point. QProcess hands the list to execve, so a `;` or a + // `$(...)` in the configured command is a literal argument with nothing to + // interpret it. Note that splitCommand strips DOUBLE quotes only. + // + // Nothing from the message reaches the argument list at all: the command + // reads its recipients from the message's own headers, which is what `-t` + // means in the documented example. + const QStringList parts = QProcess::splitCommand(command); + if (parts.isEmpty()) + return false; + + m_command = command; + m_reported = false; + + m_process.setProgram(parts.first()); + m_process.setArguments(parts.mid(1)); + + // Deliberately no waitForStarted(): this runs on the GUI thread and the + // interface must stay responsive while a send is in flight. A failed + // launch arrives via errorOccurred(FailedToStart) instead, which QProcess + // emits INSTEAD OF finished() rather than before it (measured). + m_process.start(); + + // Written after start() and before the process has necessarily launched, + // which is safe: QProcess buffers and drains as the reader consumes. + // Measured with a 320KB payload against a `cat` stub, which arrived + // byte-identical, so a message with an attachment does not deadlock on the + // 64KB pipe buffer. + m_process.write(bytes); + + // The message goes on stdin and the channel is closed, so a command + // reading to EOF terminates. Without closeWriteChannel() a command like + // `cat` waits forever and the popup never leaves its Sending stage. + m_process.closeWriteChannel(); + + return true; +} + +void MessageSender::handleFinished(int exitCode, QProcess::ExitStatus status) +{ + // errorOccurred may already have reported this failure. Reporting twice + // would close the popup and then act on a second result. + // + // This guard IS load-bearing, on exactly one path: the destructor sets + // m_reported before kill(), because kill() makes QProcess deliver + // finished(CrashExit) and without the flag this handler would emit a + // "not sent" for a message whose fate is genuinely unknown. A test fails + // against its removal. + // + // On the two signal paths it is defence in depth and currently cannot + // fire: handleError is filtered to FailedToStart, and FailedToStart is + // never followed by finished() (measured). An instrumented run of the + // whole suite recorded zero hits there, including on the crash and + // write-error paths that DO emit both signals. It stays because the day + // someone widens handleError to report another error, the double report is + // silent and costs a duplicate sent copy. + if (m_reported) + return; + m_reported = true; + + // The exit status is the only authority. Nothing is inferred from what the + // command printed: mailsync.sh records what a wrong answer here costs, and + // a send reported as succeeding files a sent copy for a message that never + // left the machine. + const bool sent = status == QProcess::NormalExit && exitCode == 0; + if (sent) { + emit finished(true, QString()); + return; + } + + // Exit 75 is deliberately NOT special. See the header. + QString error = QString::fromUtf8(m_process.readAllStandardError()).trimmed(); + if (error.isEmpty()) { + // A failure with a blank explanation gives the user nothing to act on, + // so the status stands in for the reason the command did not give. + error = status == QProcess::CrashExit + ? tr("The send command crashed.") + : tr("The send command exited with status %1 and said nothing.") + .arg(exitCode); + } + emit finished(false, error); +} + +void MessageSender::handleError(QProcess::ProcessError error) +{ + // QProcess emits errorOccurred(FailedToStart) INSTEAD OF finished(), so + // without this the caller waits forever. Measured on Qt 6.11 for both a + // missing binary and a non-executable file: one errorOccurred, no + // finished(). + // + // Every other error IS followed by finished() and is left to it, which is + // not merely tidiness. A command that exits without draining a large stdin + // emits errorOccurred(WriteError) and then finished() with the command's + // real exit code and its real stderr; reporting the write error here would + // replace the server's own rejection message with a plumbing detail, and + // reporting it as well as finished() would deliver two results for one + // message. + if (error != QProcess::FailedToStart) + return; + if (m_reported) + return; + m_reported = true; + + emit finished(false, + tr("The send command '%1' could not be started. Check that " + "the path is correct and the file is executable.") + .arg(m_command)); +} diff --git a/src/messagesender.h b/src/messagesender.h new file mode 100644 index 0000000..86dde68 --- /dev/null +++ b/src/messagesender.h @@ -0,0 +1,164 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#pragma once + +#include +#include +#include + +/// Runs an account's send_command with the message on stdin. +/// +/// EXACTLY TWO OUTCOMES: sent, or not sent with a reason. Exit code 75 has no +/// special meaning here, unlike in the sync path. Item 125 is open precisely +/// because mailsync.sh treats 75 as neither success nor failure and hangs on +/// it; that exists because the script contends for a lock and there is no lock +/// here. Recorded so the two paths are not later "harmonised". +/// +/// **The exit status is the only authority on whether a message was sent.** +/// This is the same rule assets/mailsync.sh exists to honour, and the same +/// class of bug is available here: a sender that reported success on anything +/// other than exit 0 would file a sent copy and close the composer for a +/// message that never left the machine. Nothing is derived from the command's +/// output, which belongs to whatever the user installed behind send_command. +/// +/// **No shell, ever.** The command is a config value and is split into an +/// argument list with QProcess::splitCommand, then handed to QProcess, which +/// calls execve directly. A `;`, `&&`, `$(...)` or a backtick in the +/// configured string therefore arrives as a literal argument with nothing to +/// interpret it. Note that splitCommand understands DOUBLE quotes only: +/// `-a 'my acct'` splits into three arguments, so a path or an argument +/// containing a space must be written with double quotes. Measured, not +/// assumed. +/// +/// **No message content ever reaches the argument list.** The bytes go on +/// stdin and only on stdin; the command reads its recipients from the +/// message's own headers, which is what `-t` means in the documented example. +/// A recipient address or a display name therefore cannot become an argument +/// however it is spelled. +/// +/// This is the outbox seam. An outbox is built by calling this from a drain +/// loop; nothing in the composer would need to change. +/// +/// Nothing here blocks the GUI thread DURING a send. send() hands the process +/// to the event loop and returns; there is no waitForStarted() and no +/// waitForFinished() on that path, so a command that hangs leaves the +/// interface responsive and the caller waiting on finished(). Timing a hung +/// command out is deliberately NOT this class's job: a timeout here would kill +/// a slow but working send. The one place this class does block is its +/// destructor, and that is the subject of the next paragraph. +/// +/// **Destruction mid-send waits, briefly, and then kills.** A send is a live +/// SMTP conversation, so the outcome of abandoning one is genuinely unknown: +/// the message may be fully delivered, partially delivered, or not sent at +/// all. Measured with a one-second command destroyed 100ms in: plain +/// destruction returns in 100ms, kills the child, and the work does NOT +/// complete, announced by nothing but a `QProcess: Destroyed while process is +/// still running` warning on stderr. That is the mailsync.sh failure in a new +/// place, an unknown real outcome reported as a definite one, and it is +/// reachable by closing the composer with the window manager's X button while +/// a send is in flight. +/// +/// So the destructor waits up to kShutdownWaitMs for the command to finish on +/// its own, which is the outcome that makes the report truthful: the same +/// measurement with a bounded wait completes the child and costs only the +/// ~1s the command actually needed. A command still running after that is +/// killed, because a destructor cannot block a quitting application forever. +/// +/// **No finished() is emitted from the destructor, in either branch, and that +/// is deliberate rather than an omission.** After a kill the outcome is +/// unknown, and this class reports two outcomes only; inventing a third by +/// guessing would be the exact lie the rest of this header is built to avoid. +/// After a successful late finish the emit would reach handlers on a +/// half-destroyed caller. A caller that must know the result has to keep the +/// sender alive until finished() arrives, which is what refusing to close a +/// composer mid-send would express. +/// +/// **There is no cancel(), and the caller does not have one either.** An +/// earlier revision of this comment deferred cancellation to "the caller's +/// popup", which overstated what exists: SendDialog offers an undo BEFORE the +/// send is committed and none after, by an explicit design decision that a +/// post-commit cancel is worse than either clean outcome. If a real cancel is +/// ever wanted it belongs HERE, killing the process and emitting one +/// finished(false, ...) through m_reported, which is the shape that flag +/// already has. It is not built now, and this header does not promise it. +class MessageSender : public QObject +{ + Q_OBJECT + +public: + explicit MessageSender(QObject *parent = nullptr); + + /// Waits briefly for an in-flight send, then kills it. See the class + /// comment: this is the one blocking call in the class, and it emits + /// nothing. + ~MessageSender() override; + + /// How long the destructor gives an in-flight command to finish on its + /// own before killing it. Long enough for a local MTA handing off to a + /// queue, short enough not to hang a quitting application. + static constexpr int kShutdownWaitMs = 5000; + + /// Starts \p command with \p bytes on stdin. + /// + /// Returns false without emitting anything when the command is empty or + /// only whitespace, when it splits to nothing, or when a send is already + /// running. A true return means the process was handed to the event loop, + /// NOT that it launched: a missing or non-executable binary surfaces + /// asynchronously through finished(false, ...), exactly as MailSync + /// documents. + bool send(const QString &command, const QByteArray &bytes); + + bool isRunning() const; + +signals: + /// \p error is empty on success and carries the command's stderr, or a + /// description of why it could not start, on failure. + /// + /// EMITTED exactly once per accepted send, and the distinction between + /// emitted and RECEIVED is the whole of this paragraph. QProcess can report + /// both an error and a finish for one run (measured: a command that exits + /// without draining a large stdin emits errorOccurred(WriteError) and then + /// finished()), and m_reported collapses that to one emit. + /// + /// **m_reported guards the emit, not the receivers, and a caller can still + /// see one result twice.** A MessageSender is normally a long-lived member + /// reused for every send, so a caller that connects INSIDE its send path + /// adds a permanent connection each time: send, fail, correct the + /// recipient, send again, and the second result runs BOTH lambdas. The + /// first still holds the first message's bytes, so it files a sent copy of + /// the wrong message and acts on a dialog it already destroyed. That is + /// precisely the harm this signal's contract exists to prevent, arriving + /// by the one route no guard inside this class can cover. + /// + /// A caller connecting per-send must therefore pass + /// `Qt::SingleShotConnection` (Qt 6.0+; this project is on 6.11), which + /// disconnects the moment the lambda runs. Connecting ONCE in the caller's + /// constructor and keeping the per-send state in members is the other + /// correct shape. What is not correct, and what reads as permitted if this + /// paragraph is skipped, is a bare connect() next to a send() call. + void finished(bool sent, const QString &error); + +private: + void handleFinished(int exitCode, QProcess::ExitStatus status); + void handleError(QProcess::ProcessError error); + + QProcess m_process; + QString m_command; + bool m_reported = false; +}; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 367d23d..e38d764 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -72,6 +72,7 @@ add_qtmaildir_test(markdownrenderer) add_qtmaildir_test(messagebuilder) add_qtmaildir_test(maildirname) add_qtmaildir_test(draftstore) +add_qtmaildir_test(messagesender) add_qtmaildir_test(translations) # Asserts on the tracked .ts rather than the generated .qm: an untranslated # string is dropped by lrelease, so it is invisible in the .qm and shows up diff --git a/tests/test_messagesender.cpp b/tests/test_messagesender.cpp new file mode 100644 index 0000000..89e0fcf --- /dev/null +++ b/tests/test_messagesender.cpp @@ -0,0 +1,532 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include +#include + +#include "messagesender.h" + +class TestMessageSender : public QObject +{ + Q_OBJECT + +private slots: + void aSuccessfulCommandReportsSent(); + void theMessageArrivesOnStdinIntact(); + void aLargeMessageArrivesWhole(); + void aFailingCommandReportsItsStderr(); + void aCommandThatDoesNotExistReportsAFailure(); + void aCommandThatIsNotExecutableReportsAFailure(); + void anEmptyCommandIsRefusedWithoutRunning(); + void aCommandOfOnlyWhitespaceIsRefusedWithoutRunning(); + void exitCode75IsAnOrdinaryFailure(); + void aSilentFailureStillReportsAReason(); + void aCommandThatNeverReadsStdinIsStillJudgedByItsExitStatus(); + void aCrashedCommandIsAFailureWithAReason(); + void aSecondSendIsRefusedWhileOneIsRunning(); + void shellMetacharactersReachNoShell(); + void nothingIsEverReportedTwice(); + void destroyingTheSenderLetsAnInFlightSendFinish(); + void destroyingTheSenderEmitsNothing(); + void aPerSendConnectionMustBeSingleShot(); + +private: + QString writeStub(const QString &name, const QString &body, + bool executable = true); + + QTemporaryDir m_dir; +}; + +QString TestMessageSender::writeStub(const QString &name, const QString &body, + bool executable) +{ + const QString path = m_dir.filePath(name); + QFile file(path); + if (!file.open(QIODevice::WriteOnly)) + return {}; + file.write(QStringLiteral("#!/bin/sh\n%1\n").arg(body).toUtf8()); + file.close(); + QFile::Permissions permissions = QFile::ReadOwner | QFile::WriteOwner; + if (executable) + permissions |= QFile::ExeOwner; + file.setPermissions(permissions); + return path; +} + +void TestMessageSender::aSuccessfulCommandReportsSent() +{ + const QString stub = writeStub(QStringLiteral("ok.sh"), QStringLiteral("cat >/dev/null")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("From: a@example.org\r\n\r\nbody\r\n"))); + + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.at(0).at(0).toBool(), true); + QVERIFY2(spy.at(0).at(1).toString().isEmpty(), + "a successful send carried an error message"); + QVERIFY2(!sender.isRunning(), "the sender still reports a run in progress"); +} + +void TestMessageSender::theMessageArrivesOnStdinIntact() +{ + // The property that matters most: the bytes the builder produced are the + // bytes the command receives. A stub that writes stdin to a file is the + // only way to see it, since there is no MTA to ask. + const QString captured = m_dir.filePath(QStringLiteral("captured.eml")); + const QString stub = writeStub(QStringLiteral("capture.sh"), + QStringLiteral("cat > '%1'").arg(captured)); + QVERIFY(!stub.isEmpty()); + + const QByteArray bytes( + "From: a@example.org\r\n" + "Subject: =?UTF-8?B?UGVyY2jDqQ==?=\r\n" + "\r\n" + "Perch=C3=A9 accented body.\r\n"); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, bytes)); + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.at(0).at(0).toBool(), true); + + QFile file(captured); + QVERIFY2(file.open(QIODevice::ReadOnly), "the stub captured no stdin at all"); + QCOMPARE(file.readAll(), bytes); +} + +void TestMessageSender::aLargeMessageArrivesWhole() +{ + // A message with an attachment is megabytes, not bytes, and a pipe holds + // 64KB. If the write were not driven by the event loop the process would + // deadlock on a full pipe, or the tail would be silently dropped and a + // truncated message would be reported as sent. Measured: 1.6MB in one + // write() call returns the full count only because QProcess buffers it and + // drains it as the reader consumes; a probe confirmed the payload arrives + // byte-identical. + const QString captured = m_dir.filePath(QStringLiteral("big.eml")); + const QString stub = writeStub(QStringLiteral("bigcapture.sh"), + QStringLiteral("cat > '%1'").arg(captured)); + QVERIFY(!stub.isEmpty()); + + QByteArray bytes("From: a@example.org\r\n\r\n"); + // Well past a pipe buffer, and not a repeating single byte, so a partial + // write cannot accidentally compare equal. + for (int i = 0; i < 60000; ++i) + bytes += QByteArray::number(i) + "\r\n"; + QVERIFY(bytes.size() > 300000); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, bytes)); + QVERIFY(spy.wait(10000)); + QCOMPARE(spy.at(0).at(0).toBool(), true); + + QFile file(captured); + QVERIFY(file.open(QIODevice::ReadOnly)); + const QByteArray got = file.readAll(); + QCOMPARE(got.size(), bytes.size()); + QCOMPARE(got, bytes); +} + +void TestMessageSender::aFailingCommandReportsItsStderr() +{ + // stderr is shown verbatim: network errors, authentication failures and + // server rejections all belong to send_command, and this application + // deliberately does not interpret them. + const QString stub = writeStub( + QStringLiteral("fail.sh"), + QStringLiteral("cat >/dev/null; echo 'auth failed: bad password' >&2; exit 1")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("body"))); + + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.at(0).at(0).toBool(), false); + QVERIFY2(spy.at(0).at(1).toString().contains(QStringLiteral("auth failed")), + qPrintable(QStringLiteral("stderr was not reported: '%1'") + .arg(spy.at(0).at(1).toString()))); +} + +void TestMessageSender::aCommandThatDoesNotExistReportsAFailure() +{ + // A typo'd path is the likely cause, so the message names the command. + // QProcess emits errorOccurred(FailedToStart) INSTEAD OF finished(), which + // is the trap MailSync already documents: without handling it the signal + // never arrives and the popup waits forever. Measured on Qt 6.11: + // finCount 0, errCount 1. + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(QStringLiteral("/nonexistent/msmtp"), QByteArray("body"))); + + QVERIFY2(spy.wait(5000), "no result was ever reported for a missing command"); + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.at(0).at(0).toBool(), false); + QVERIFY2(spy.at(0).at(1).toString().contains(QStringLiteral("msmtp")), + qPrintable(QStringLiteral("the error does not name the command: '%1'") + .arg(spy.at(0).at(1).toString()))); +} + +void TestMessageSender::aCommandThatIsNotExecutableReportsAFailure() +{ + // A separate case from a missing file and reached by an ordinary mistake: + // a script written by the user and never chmod'd. It also arrives as + // FailedToStart with no finished(), so the same handler covers it, but a + // test asserting only the missing-file case would pass against a handler + // keyed on the errno rather than on the error enum. + const QString stub = writeStub(QStringLiteral("noexec.sh"), + QStringLiteral("cat >/dev/null"), false); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("body"))); + + QVERIFY2(spy.wait(5000), "no result was ever reported for a non-executable command"); + QCOMPARE(spy.at(0).at(0).toBool(), false); + QVERIFY(!spy.at(0).at(1).toString().isEmpty()); +} + +void TestMessageSender::anEmptyCommandIsRefusedWithoutRunning() +{ + // A receive-only account. The compose actions are disabled on its mail, so + // this should be unreachable; refusing here rather than asserting means a + // future caller cannot accidentally send from an account that cannot. + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY2(!sender.send(QString(), QByteArray("body")), + "an empty command was accepted"); + QCOMPARE(spy.count(), 0); + QVERIFY(!sender.isRunning()); +} + +void TestMessageSender::aCommandOfOnlyWhitespaceIsRefusedWithoutRunning() +{ + // A config file with `send_command = ` and a trailing space reaches + // exactly this, and it must not run anything. + // + // MEASURED, and worth stating precisely so this is not mistaken for a + // sharper test than it is: send() has TWO guards that both catch a blank + // command, the trimmed()-empty check and the parts.isEmpty() check after + // QProcess::splitCommand(" ") returns an empty list. Dropping either one + // alone leaves this test green, because the other still refuses. Dropping + // BOTH aborts the run outright: QProcess treats an empty program as fatal, + // and the mutation reports "Received a fatal error" rather than a failed + // comparison. The pair is what is under test here; the redundancy is + // deliberate, since the fatal path is the one thing a send must never + // reach. + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY2(!sender.send(QStringLiteral(" \t "), QByteArray("body")), + "a whitespace-only command was accepted"); + QCOMPARE(spy.count(), 0); + QVERIFY(!sender.isRunning()); +} + +void TestMessageSender::exitCode75IsAnOrdinaryFailure() +{ + // Explicitly asserted so the sync path's special handling of 75 is never + // copied here. There is no lock to contend for, so 75 means only what the + // command chose it to mean: not sent. + const QString stub = writeStub(QStringLiteral("busy.sh"), + QStringLiteral("cat >/dev/null; exit 75")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("body"))); + + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.at(0).at(0).toBool(), false); +} + +void TestMessageSender::aSilentFailureStillReportsAReason() +{ + // The mailsync.sh lesson in the other direction: a command that fails + // without saying anything must not produce an empty error string, because + // the popup would then show a failure with a blank explanation and the + // user would have nothing to act on. + const QString stub = writeStub(QStringLiteral("silent.sh"), + QStringLiteral("cat >/dev/null; exit 3")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("body"))); + + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.at(0).at(0).toBool(), false); + const QString error = spy.at(0).at(1).toString(); + QVERIFY2(!error.isEmpty(), "a silent failure reported no reason at all"); + QVERIFY2(error.contains(QStringLiteral("3")), + qPrintable(QStringLiteral("the exit status is not named: '%1'").arg(error))); +} + +void TestMessageSender::aCommandThatNeverReadsStdinIsStillJudgedByItsExitStatus() +{ + // Measured on Qt 6.11: a command that exits without draining a large stdin + // emits errorOccurred(WriteError) BEFORE finished(). A handler that treated + // any error as a failure to start would report the write error and swallow + // the real exit status; a handler that reported on every error would report + // twice. The exit status is the only authority, exactly as it is for the + // sync script, so this asserts the reason the command GAVE. + const QString stub = writeStub( + QStringLiteral("nonreading.sh"), + QStringLiteral("echo 'recipient rejected' >&2; exit 1")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray(1600 * 1024, 'x'))); + + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.at(0).at(0).toBool(), false); + QVERIFY2(spy.at(0).at(1).toString().contains(QStringLiteral("recipient rejected")), + qPrintable(QStringLiteral("the command's own reason was lost: '%1'") + .arg(spy.at(0).at(1).toString()))); +} + +void TestMessageSender::aCrashedCommandIsAFailureWithAReason() +{ + // A segfaulting MTA is a real failure mode and reaches a DIFFERENT branch + // from a nonzero exit: status is CrashExit and exitCode carries the signal + // number, so an error message built from the exit code alone would tell the + // user the command "exited with status 11", which is not what happened. + // + // Measured on Qt 6.11: a crash emits errorOccurred(Crashed) and THEN + // finished(11, CrashExit). Only finished() reports, because handleError + // filters to FailedToStart, so the count assertion below also proves that + // filter is doing work on a path that is not the write-error one. + const QString stub = writeStub(QStringLiteral("crash.sh"), + QStringLiteral("cat >/dev/null; kill -SEGV $$")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("body"))); + + QVERIFY(spy.wait(5000)); + QTest::qWait(300); + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.at(0).at(0).toBool(), false); + const QString error = spy.at(0).at(1).toString(); + QVERIFY2(!error.isEmpty(), "a crashed command reported no reason"); + QVERIFY2(error.contains(QStringLiteral("crash")), + qPrintable(QStringLiteral("a crash was reported as an ordinary exit: '%1'") + .arg(error))); +} + +void TestMessageSender::aSecondSendIsRefusedWhileOneIsRunning() +{ + // One QProcess, so a second send would overwrite the first's program and + // arguments mid-flight. Refusing is what makes the popup's Sending stage + // mean one message. + const QString stub = writeStub(QStringLiteral("slow.sh"), + QStringLiteral("cat >/dev/null; sleep 1")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("first"))); + QVERIFY2(sender.isRunning(), "the sender does not report the run it just started"); + QVERIFY2(!sender.send(stub, QByteArray("second")), + "a second send was accepted while one was running"); + + QVERIFY(spy.wait(10000)); + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.at(0).at(0).toBool(), true); +} + +void TestMessageSender::shellMetacharactersReachNoShell() +{ + // The security property, asserted rather than asserted-about-in-a-comment. + // The command is split into an argument list and handed to execve, so a + // `;` in it is a literal argument and there is no shell to act on it. If + // this ever ran through `sh -c` the stub below would be invoked and the + // marker file would exist. + // + // Measured: QProcess::splitCommand("msmtp; rm x") yields ("msmtp;", "rm", + // "x"), so the semicolon does not even separate arguments. + const QString marker = m_dir.filePath(QStringLiteral("shell-ran")); + const QString stub = writeStub(QStringLiteral("args.sh"), + QStringLiteral("cat >/dev/null; exit 0")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(QStringLiteral("%1 ; touch %2").arg(stub, marker), + QByteArray("body"))); + QVERIFY(spy.wait(5000)); + + QVERIFY2(!QFile::exists(marker), + "the send command was interpreted by a shell"); + + // And the same string quoted the way a shell would need it also reaches no + // shell: double quotes are the ONLY quoting splitCommand understands. + // Measured: single quotes are NOT stripped, so `-a 'my acct'` arrives as + // three arguments. Recorded here because the plan's comment claimed + // splitCommand "handles quoted arguments" without that qualification. + QCOMPARE(QProcess::splitCommand(QStringLiteral("m -a \"my acct\" -t")), + QStringList({QStringLiteral("m"), QStringLiteral("-a"), + QStringLiteral("my acct"), QStringLiteral("-t")})); + QCOMPARE(QProcess::splitCommand(QStringLiteral("m -a 'my acct' -t")), + QStringList({QStringLiteral("m"), QStringLiteral("-a"), + QStringLiteral("'my"), QStringLiteral("acct'"), + QStringLiteral("-t")})); +} + +void TestMessageSender::nothingIsEverReportedTwice() +{ + // Reporting twice would close the send popup and then act on a second + // result, which for a caller that files a sent copy on success means two + // copies, or a success followed by a failure. Run every outcome through one + // sender and count. + const QString ok = writeStub(QStringLiteral("dup-ok.sh"), + QStringLiteral("cat >/dev/null")); + const QString bad = writeStub(QStringLiteral("dup-bad.sh"), + QStringLiteral("echo boom >&2; exit 1")); + QVERIFY(!ok.isEmpty() && !bad.isEmpty()); + + for (const QString &command : + {ok, bad, QStringLiteral("/nonexistent/msmtp")}) { + MessageSender sender; + QSignalSpy spy(&sender, &MessageSender::finished); + QVERIFY(sender.send(command, QByteArray(1600 * 1024, 'x'))); + QVERIFY(spy.wait(10000)); + // Give any second signal a chance to arrive before counting. + QTest::qWait(300); + QVERIFY2(spy.count() == 1, + qPrintable(QStringLiteral("%1 reported %2 times") + .arg(command) + .arg(spy.count()))); + } +} + +void TestMessageSender::destroyingTheSenderLetsAnInFlightSendFinish() +{ + // The composer's X button is reachable mid-send, and abandoning a live + // SMTP conversation has a genuinely unknown outcome. Measured before the + // destructor existed: plain destruction 100ms into a one-second command + // killed the child and the work did NOT complete, announced by nothing but + // a "QProcess: Destroyed while process is still running" warning. + // + // The marker file is the evidence, because it is written by the command + // itself after its work: if the destructor killed the child, it does not + // exist. + const QString marker = m_dir.filePath(QStringLiteral("send-completed")); + const QString stub = writeStub( + QStringLiteral("slowfinish.sh"), + QStringLiteral("cat >/dev/null; sleep 1; touch '%1'").arg(marker)); + QVERIFY(!stub.isEmpty()); + QVERIFY2(!QFile::exists(marker), "the marker existed before the send ran"); + + { + MessageSender sender; + QVERIFY(sender.send(stub, QByteArray("body"))); + // Destroyed well before the command could finish, which is the case + // that matters; without the wait this scope kills it. + QTest::qWait(100); + QVERIFY2(sender.isRunning(), "the command finished before it was abandoned"); + } + + QVERIFY2(QFile::exists(marker), + "destroying the sender killed a send that was in flight"); +} + +void TestMessageSender::destroyingTheSenderEmitsNothing() +{ + // After a kill the outcome is unknown, and this class reports two outcomes + // only. A finished(false, ...) from the destructor would report "not sent" + // for a message that may have been delivered, which is the mailsync.sh + // mistake pointing the other way. + // + // A command that outlasts the shutdown wait is what forces the kill + // branch, so the wait is shortened by pointing the test at a command + // longer than it rather than by changing the constant. + const QString stub = writeStub(QStringLiteral("outlast.sh"), + QStringLiteral("cat >/dev/null; sleep 30")); + QVERIFY(!stub.isEmpty()); + + QSignalSpy *spy = nullptr; + { + MessageSender sender; + spy = new QSignalSpy(&sender, &MessageSender::finished); + QVERIFY(sender.send(stub, QByteArray("body"))); + QTest::qWait(100); + QVERIFY(sender.isRunning()); + // The destructor runs as this scope ends: it waits kShutdownWaitMs + // for a command that will not finish, then kills it. + } + // The spy outlives the sender deliberately: a signal emitted during + // destruction would have been recorded before the object went away. + QCOMPARE(spy->count(), 0); + delete spy; +} + +void TestMessageSender::aPerSendConnectionMustBeSingleShot() +{ + // The header's contract, asserted. m_reported collapses two QProcess + // signals into one emit, but it cannot stop a caller from accumulating + // RECEIVERS: a long-lived sender that a caller connects to inside its send + // path runs every previous lambda on the next result, each still holding + // the previous message's bytes. + // + // This is the plan's own Task 11 shape, and it is why that step now + // specifies Qt::SingleShotConnection. + const QString stub = writeStub(QStringLiteral("twice.sh"), + QStringLiteral("cat >/dev/null")); + QVERIFY(!stub.isEmpty()); + + MessageSender sender; // long-lived, as a ComposeWindow member is + + // The broken shape: a bare connect() beside each send(). + int bareDeliveries = 0; + for (int i = 0; i < 2; ++i) { + QSignalSpy spy(&sender, &MessageSender::finished); + connect(&sender, &MessageSender::finished, this, + [&bareDeliveries](bool, const QString &) { ++bareDeliveries; }); + QVERIFY(sender.send(stub, QByteArray("body"))); + QVERIFY(spy.wait(5000)); + QCOMPARE(spy.count(), 1); // ONE emit, both times + } + QVERIFY2(bareDeliveries == 3, + qPrintable(QStringLiteral("expected the documented 1+2 accumulation, got %1") + .arg(bareDeliveries))); + + // The prescribed shape: the connection disconnects as it fires, so two + // sends deliver two results rather than three. + MessageSender clean; + int singleShotDeliveries = 0; + for (int i = 0; i < 2; ++i) { + QSignalSpy spy(&clean, &MessageSender::finished); + connect(&clean, &MessageSender::finished, this, + [&singleShotDeliveries](bool, const QString &) { ++singleShotDeliveries; }, + Qt::SingleShotConnection); + QVERIFY(clean.send(stub, QByteArray("body"))); + QVERIFY(spy.wait(5000)); + } + QCOMPARE(singleShotDeliveries, 2); +} + +QTEST_MAIN(TestMessageSender) +#include "test_messagesender.moc" diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index 76652b8..489c62d 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -998,6 +998,21 @@ Message-Id: + + MessageSender + + The send command crashed. + Il comando di invio si è arrestato in modo anomalo. + + + The send command exited with status %1 and said nothing. + Il comando di invio è terminato con stato %1 senza fornire spiegazioni. + + + The send command '%1' could not be started. Check that the path is correct and the file is executable. + Impossibile avviare il comando di invio '%1'. Verifica che il percorso sia corretto e che il file sia eseguibile. + + MessageView @@ -1254,6 +1269,18 @@ The message could not be assembled. Non è stato possibile comporre il messaggio. + + No folder was configured to write to. + Nessuna cartella configurata per la scrittura. + + + Cannot create the folder %1. + Impossibile creare la cartella %1. + + + Cannot write to %1: %2 + Impossibile scrivere su %1: %2 + QueryCompleter -- cgit v1.2.3 From 95ae5dfe2df7858ad957b353dc0ae1d7af3b4832 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 21 Aug 2026 16:13:28 +0200 Subject: feat(compose): transform the markdown buffer for the toolbar, item 123 MarkdownFormat, task 8 of the compose-and-send plan. Three free functions over (text, selection start, selection end) returning the new text and the selection that follows it, so the grammar is tested without a widget. Three gaps in the plan's draft, each now pinned by a test checked against the mutation that breaks it: - QString::lastIndexOf INCLUDES the position it is given, so quoting with the cursor at the end of a line found that line's newline and quoted the FOLLOWING one. The draft's fixtures never placed a cursor there. - A backwards selection was normalised but never tested, so the swap was unguarded; a right-to-left drag is an ordinary gesture and Qt reports the anchor after the cursor. normalise() now swaps and clamps in one place. - A blank line inside a quoted range produced "> " with trailing whitespace, which editors and mail clients strip anyway. It is written bare. Two further defects came out of review: - quote()'s selectionStart was unasserted for any block not starting at line zero. Hardcoding it to 0 passed all nineteen tests, because the one test naming the property quoted the first line, where right and wrong coincide. A wrong selection there means a second press quotes a line the user never selected, and a following Bold bolds the wrong text. - A selection splitting a surrogate pair split the character across the inserted tokens, leaving invalid UTF-16. Not reachable from the toolbar, where arrow keys and mouse hit-testing both move in whole clusters, but reachable by any code computing a position arithmetically. normalise() nudges off a low surrogate; a collapsed cursor moves back on both ends, since widening would turn "insert an empty pair here" into "wrap the emoji". The buttons stack rather than toggle: a second Bold press gives ****this****, and a second Quote press nests. That is what the spec specifies, and the preserved selection exists so a second press can apply a SECOND token. A toggle was built during this task at the user's request and reverted on finding it contradicts the spec at two sites; it is recorded as backlog item 135, where the unanswered question is what replaces bold-then-italic. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LoaLBowZ6w1JNx6SEhDP1L --- .../plans/2026-08-03-post-0.1.0-usability.md | 73 +++++ src/CMakeLists.txt | 1 + src/formattoolbar.cpp | 182 +++++++++++ src/formattoolbar.h | 76 +++++ tests/CMakeLists.txt | 1 + tests/test_formattoolbar.cpp | 346 +++++++++++++++++++++ 6 files changed, 679 insertions(+) create mode 100644 src/formattoolbar.cpp create mode 100644 src/formattoolbar.h create mode 100644 tests/test_formattoolbar.cpp (limited to 'docs/superpowers/plans') 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 01be5d0..e9f99a4 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 @@ -204,6 +204,7 @@ taking that too literally. | 132 | Every action must have a shortcut, and that no longer serves | policy | S | done, 2026-08-20. `everyActionHasAShortcut` is deleted and nothing replaces it: `everyActionIsReachableFromAMenu()` is the required rule and a shortcut is now a chosen subset. Nothing else needed changing, since `showShortcutReference()` already printed `(unbound)` for an empty sequence. Verified by unbinding `tag_rules` and running the suite green, which would have failed before | | 133 | The composer shows no markdown syntax highlighting | v2 | S | open, 2026-08-20, from the item 123 brainstorm. **Blocked on 123.** A `QSyntaxHighlighter` over the composer's editor, so `**bold**` reads as bold while the buffer stays plain markdown. Standard Qt, no dependency. Deliberately after 123's formatting toolbar: agreeing with the grammar about nesting and about code spans suppressing what is inside them is the expensive part, and the toolbar is what makes the feature usable | | 134 | The busy indicator is built inline and is about to be built twice | maintenance | S | done, 2026-08-20, af902e0. `BusyIndicator` (`src/busyindicator.h`) carries both modes: `MainWindow` uses the indeterminate one, and item 123's send popup takes the determinate half for its undo countdown, switching the same widget over when the command starts. Only the BAR was extracted, not the status label this row paired with it. `m_statusLabel` has 34 uses across `MainWindow` for transient messages, selection counts and sync phases, so it belongs to the window rather than to the indicator, and the send popup owns its own phase text | +| 135 | The formatting toolbar's buttons stack rather than toggle | v2 | S | open, 2026-08-21, asked for by the user during item 123 task 8 and reverted the same session. **A spec change, not a defect**: it conflicts with spec:236 ("deliberately no live toggle") and spec:187-190. Both sites need amending FIRST, and the amendment must resolve what replaces bold-then-italic, which is the gesture spec:187's preserved selection exists to serve and which a toggle makes unreachable. That question is the work; the state machine is understood and written up in the section | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1098,6 +1099,78 @@ Then Delete a message. Verified by hand on 2026-08-20; this is how it was found. **Size: S.** +## 135. The formatting toolbar's buttons stack rather than toggle + +**Observed (user, 2026-08-21):** pressing Bold a second time on already-bold +text adds another pair of asterisks rather than removing the first, so +`**this**` becomes `****this****`. Quote nests the same way: a second press on +`> one` gives `> > one`. The user asked for both to toggle. + +**A toggle was built and reverted the same session**, and the reason matters +more than the code: it was not unwanted, it **conflicts with the spec**, which +was not checked before the work started. + +- `2026-08-20-compose-and-send-design.md:236` states there is "deliberately no + live toggle that inserts and removes the quote while editing". +- `:187-190` is the complete statement of the wrap behaviour and describes only + wrapping, with no toggle anywhere. + +**Cause.** This is a **spec change, not a defect**, and both sites need +amending before any code is written again. + +Underneath sits a real design question the spec answers one way and a toggle +answers the other, which is why the two cannot simply coexist. `:187` preserves +the selection after a wrap **so that a second press applies a SECOND token** to +the same words: bold, then italic, without touching the mouse. A toggle makes +that gesture unreachable, because the second press now removes the first token +instead. **What replaces bold-then-italic is unanswered**, and answering it is +the substance of this item, not the state machine below. Possible directions, +none chosen: a modifier on the second press, a separate un-format action, or +accepting that the chord is lost and reaching nested emphasis by typing. + +**Approach.** When it is picked up, the transformation half is already +understood, so the notes below exist to stop it being rediscovered. A toggling +`wrap()` must distinguish three states, and a single "it unwraps" test passes +against most of them being broken: + +- **INSIDE** the tokens: `**this**` with `this` selected (2..6). The tokens sit + just outside the selection; the same characters stay selected afterwards. +- **AROUND** them: `**this**` selected whole (0..8). The selection shrinks to + the text that was between them. +- **PARTIALLY overlapping** one: `*this**` (6..13). Neither of the above. It + does not describe a wrapped span, and stripping would have to guess which + half of a token to keep, so wrapping is the predictable answer. + +**INSIDE must be checked before AROUND.** On `***this***` both tests match, and +only INSIDE removes the level the user actually asked for. + +**A naive adjacency test is wrong, and looks right.** Checking only whether the +characters either side of the selection equal the token means pressing *Italic* +on `**this**` finds a `*` on each side, strips one asterisk per side, and +**un-bolds text the user asked to italicise**. A strip must require the adjacent +RUN of token characters to be the token exactly, or the token plus one other +complete emphasis token: `***` is bold+italic and divisible either way, while a +run of two is one indivisible token whose half is not a token at all. This was +found by writing the italic-on-bold test, not by reading the code. + +The quote side is simpler but has one trap: a bare `>` is what the quote path +writes for a blank line, so an unquote that only recognises `"> "` leaves a +stray marker on every blank line in a round trip. Whether a mixed block (some +lines quoted, some not) quotes or unquotes is a decision; quoting it, so one +press makes the block uniform and the next unquotes it, avoids the button doing +two opposite things to two halves of one selection. + +**Constraints.** The spec amendment comes first and must resolve the +bold-then-italic question, or the same conflict recurs. `MarkdownFormat` is +painter-free and widget-free, so the whole state machine is unit-testable +without the composer; keep it that way. The toolbar shortcuts belong to the +composer window and do not touch `KeyMap`, so nothing here interacts with item +132. Note that toggling changes what the preserved selection is FOR, so +`wrappingTwiceNestsTheTokensAroundTheSameWords` and +`quotingAnAlreadyQuotedLineNestsIt` in `tests/test_formattoolbar.cpp` both +assert the current spec behaviour and would be replaced rather than extended. + + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index a462ba3..501c276 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -16,6 +16,7 @@ add_library(qtmaildir_lib STATIC draftstore.cpp messagesender.cpp composecontext.cpp + formattoolbar.cpp tagchip.cpp tagcolors.cpp savequerydialog.cpp diff --git a/src/formattoolbar.cpp b/src/formattoolbar.cpp new file mode 100644 index 0000000..565d4af --- /dev/null +++ b/src/formattoolbar.cpp @@ -0,0 +1,182 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include "formattoolbar.h" + +#include + + +namespace { + +/// Normalises the selection a widget reports into an ordered, in-range pair +/// that does not split a character. +/// +/// Three hazards, handled once rather than per function. A backwards drag +/// reports the anchor AFTER the cursor; a selection can outlive the edit that +/// shortened the buffer under it; and a boundary can land in the middle of a +/// surrogate pair, where inserting a token splits one character into two +/// halves and the result is not valid UTF-16 at all. +/// +/// The surrogate case is not reachable with an arrow key or the mouse, which +/// move in whole clusters, but QTextCursor::setPosition accepts such a +/// position, so any caller computing one arithmetically can produce it: a +/// draft restore, a find/replace, a template insertion. A boundary sitting on +/// a LOW surrogate is inside a pair, and moving it back by one puts it before +/// the whole character. +/// +/// A COLLAPSED cursor moves back, not outward: nudging the two ends in +/// opposite directions would turn an empty selection into a two-unit one and +/// wrap a character the user never selected. A real selection widens, so that +/// touching any part of a character covers the whole of it. +void normalise(const QString &text, int &from, int &to) +{ + from = qBound(0, from, int(text.size())); + to = qBound(0, to, int(text.size())); + if (from > to) + qSwap(from, to); + + const auto insidePair = [&text](int at) { + return at < text.size() && text.at(at).isLowSurrogate(); + }; + + if (from == to) { + if (insidePair(from)) { + --from; + to = from; + } + return; + } + + if (insidePair(from)) + --from; + if (insidePair(to)) + ++to; +} + +} // namespace + +MarkdownFormat::Edit MarkdownFormat::wrap(const QString &text, int start, + int end, const QString &token) +{ + Edit edit; + int from = start; + int to = end; + normalise(text, from, to); + + edit.text = text; + // The closing token first: inserting at `from` would shift `to`. + edit.text.insert(to, token); + edit.text.insert(from, token); + + if (from == to) { + // No selection: the cursor goes BETWEEN the two tokens so typing + // continues inside them. Landing after the closing token instead is + // the mistake a user notices on the first keystroke. + edit.selectionStart = from + token.size(); + edit.selectionEnd = edit.selectionStart; + } else { + // The selection is preserved so a second press applies a second token + // to the same words without reselecting: bold then italic. + edit.selectionStart = from + token.size(); + edit.selectionEnd = to + token.size(); + } + + return edit; +} + +MarkdownFormat::Edit MarkdownFormat::link(const QString &text, int start, int end) +{ + Edit edit; + int from = start; + int to = end; + normalise(text, from, to); + + const QString label = text.mid(from, to - from); + + edit.text = text; + edit.text.replace(from, to - from, QStringLiteral("[%1]()").arg(label)); + + if (label.isEmpty()) { + // Nothing selected: the label is what gets typed first, so the cursor + // goes inside the brackets, one past the '['. + edit.selectionStart = from + 1; + } else { + // The label is written; the URL is what remains, so the cursor goes + // inside the parentheses: past '[', the label, ']' and '('. + edit.selectionStart = from + label.size() + 3; + } + edit.selectionEnd = edit.selectionStart; + + return edit; +} + +MarkdownFormat::Edit MarkdownFormat::quote(const QString &text, int start, int end) +{ + Edit edit; + int from = start; + int to = end; + normalise(text, from, to); + + // Line-based, not a wrap. The selection is widened to whole lines first: + // quoting half a line produces markdown that means something else. + // + // The backwards search starts at `from - 1`, not at `from`. QString's + // lastIndexOf INCLUDES the position it is given, so a cursor sitting at + // the end of a line, immediately before its newline, would find that + // newline and quote the FOLLOWING line instead of the one the cursor is + // on. The guard against a negative position matters too, since -1 means + // "search from the end" and would find the last newline in the buffer. + const int firstLineStart = + from > 0 ? text.lastIndexOf(QLatin1Char('\n'), from - 1) + 1 : 0; + + // No newline after the last line, so the end of the text is the end of + // the block. Without this the whole tail would be dropped. + int lastLineEnd = text.indexOf(QLatin1Char('\n'), to); + if (lastLineEnd < 0) + lastLineEnd = text.size(); + + const QString before = text.left(firstLineStart); + const QString middle = text.mid(firstLineStart, lastLineEnd - firstLineStart); + const QString after = text.mid(lastLineEnd); + + const QStringList lines = middle.split(QLatin1Char('\n')); + + // Nesting rather than toggling, per the spec: a second press deepens the + // quote. There is deliberately no live toggle here, because tracking "my + // text" and "the quote" as separate pieces to make one reversible is + // machinery for a case the user answers by closing the composer. + QStringList result; + result.reserve(lines.size()); + for (const QString &line : lines) { + // A blank line keeps the marker, since that is what continues a quote + // block in markdown, but WITHOUT the trailing space: several editors + // and mail clients strip trailing whitespace, and stripping it from + // "> " leaves ">" anyway, so writing it bare is the same result + // reached deliberately. + result.append(line.isEmpty() ? QStringLiteral(">") + : QStringLiteral("> ") + line); + } + + const QString replacement = result.join(QLatin1Char('\n')); + edit.text = before + replacement + after; + // The quoted block stays selected, so a second press nests it. + edit.selectionStart = firstLineStart; + edit.selectionEnd = firstLineStart + replacement.size(); + + return edit; +} diff --git a/src/formattoolbar.h b/src/formattoolbar.h new file mode 100644 index 0000000..d820a88 --- /dev/null +++ b/src/formattoolbar.h @@ -0,0 +1,76 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#pragma once + +#include + +/// The markdown transformations behind the composer's formatting toolbar. +/// +/// Free functions over text and a selection, with no widget anywhere, so the +/// grammar is tested without a painter. Each one is a transformation over the +/// SOURCE: nothing about the buffer changes, it stays markdown the user can +/// also type by hand. +/// +/// Every function takes the selection as the widget reports it, which means +/// the anchor may sit AFTER the cursor. Each one normalises with qMin/qMax +/// rather than requiring the caller to, since a backwards drag is an ordinary +/// gesture and a caller that forgets would corrupt the buffer silently. +/// Out-of-range positions are clamped to the text, so a stale selection +/// cannot index past the end, and a boundary landing INSIDE a surrogate pair +/// is nudged off it, so a position computed arithmetically cannot split a +/// character in half. +/// +/// Neither wrap() nor quote() TOGGLES. A second press stacks another level: +/// `**this**` becomes `***this***` and `> one` becomes `> > one`. That is the +/// design, not an omission. The selection is preserved precisely so a second +/// press can apply a SECOND token to the same words, bold then italic without +/// reselecting, and a toggle would make that gesture unreachable. A toggle is +/// wanted eventually and is a spec change rather than a fix; see item 135 in +/// the backlog for the states it has to distinguish. +namespace MarkdownFormat { + +/// The result of a transformation: the new text and where the selection +/// should end up. +struct Edit +{ + QString text; + int selectionStart = 0; + int selectionEnd = 0; +}; + +/// Wraps the selection in \p token, or inserts an empty pair with the cursor +/// BETWEEN the tokens when there is no selection. +/// +/// The cursor landing between the tokens is the property a user notices +/// immediately when it is wrong, and it is invisible to a test that only +/// compares the resulting text. +Edit wrap(const QString &text, int start, int end, const QString &token); + +/// `[text](url)`. With a selection the selected text becomes the label and +/// the cursor lands inside the empty parentheses, which is where the user has +/// to type next. With none the cursor lands inside the brackets, since the +/// label is then what gets typed first. +Edit link(const QString &text, int start, int end); + +/// `> ` on every line the selection touches, including a line the selection +/// only starts or ends on. Line-based rather than a wrap, so it cannot be +/// expressed with wrap(). +Edit quote(const QString &text, int start, int end); + +} // namespace MarkdownFormat diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 48b30fc..b28bf6f 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -74,6 +74,7 @@ add_qtmaildir_test(maildirname) add_qtmaildir_test(draftstore) add_qtmaildir_test(messagesender) add_qtmaildir_test(composecontext) +add_qtmaildir_test(formattoolbar) add_qtmaildir_test(translations) # Asserts on the tracked .ts rather than the generated .qm: an untranslated # string is dropped by lrelease, so it is invisible in the .qm and shows up diff --git a/tests/test_formattoolbar.cpp b/tests/test_formattoolbar.cpp new file mode 100644 index 0000000..36918f9 --- /dev/null +++ b/tests/test_formattoolbar.cpp @@ -0,0 +1,346 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include + +#include "formattoolbar.h" + +class TestFormatToolbar : public QObject +{ + Q_OBJECT + +private slots: + void wrappingASelectionKeepsItSelected(); + void wrappingWithNoSelectionPutsTheCursorBetweenTheTokens(); + void wrappingAppliesTheTokenOnBothSides(); + void aBackwardsSelectionWrapsTheSameWordsAsAForwardOne(); + + void wrappingTwiceNestsTheTokensAroundTheSameWords(); + + void aLinkWithASelectionUsesItAsTheLabel(); + void aLinkWithNoSelectionLeavesTheCursorInTheLabel(); + void aBackwardsSelectionLinksTheSameWordsAsAForwardOne(); + void quotingPrefixesEveryLineTheSelectionTouches(); + void quotingAPartialLineStillQuotesTheWholeLine(); + void quotingASingleLineWithNoSelectionQuotesThatLine(); + void quotingWithTheCursorAtTheEndOfALineQuotesThatLineNotTheNext(); + void quotingSelectsTheQuotedLines(); + void quotingSelectsOnlyTheLineItQuoted(); + void quotingTheLastLineKeepsTheRestOfTheText(); + void quotingAnAlreadyQuotedLineNestsIt(); + void quotingAnEmptyLineLeavesTheMarkerWithoutTrailingSpace(); + void aSelectionPastTheEndIsClamped(); + void aSelectionSplittingASurrogatePairKeepsTheCharacterWhole(); +}; + +void TestFormatToolbar::wrappingASelectionKeepsItSelected() +{ + // The selection is preserved so a second button press applies a second + // token to the same words: bold then italic, without reselecting. + const MarkdownFormat::Edit edit = MarkdownFormat::wrap( + QStringLiteral("make this bold"), 5, 9, QStringLiteral("**")); + + QCOMPARE(edit.text, QStringLiteral("make **this** bold")); + QCOMPARE(edit.text.mid(edit.selectionStart, + edit.selectionEnd - edit.selectionStart), + QStringLiteral("this")); +} + +void TestFormatToolbar::wrappingWithNoSelectionPutsTheCursorBetweenTheTokens() +{ + // The property a user notices immediately when it is wrong: press Bold, + // start typing, and the words must appear INSIDE the asterisks. A text + // comparison alone passes whether the cursor is inside or after. + const MarkdownFormat::Edit edit = MarkdownFormat::wrap( + QStringLiteral("ab"), 2, 2, QStringLiteral("**")); + + QCOMPARE(edit.text, QStringLiteral("ab****")); + QCOMPARE(edit.selectionStart, edit.selectionEnd); + QCOMPARE(edit.selectionStart, 4); + + // Stated as the behaviour rather than the index: typing "x" here must + // produce "ab**x**". + QString typed = edit.text; + typed.insert(edit.selectionStart, QStringLiteral("x")); + QCOMPARE(typed, QStringLiteral("ab**x**")); +} + +void TestFormatToolbar::wrappingAppliesTheTokenOnBothSides() +{ + QCOMPARE(MarkdownFormat::wrap(QStringLiteral("x"), 0, 1, + QStringLiteral("~~")).text, + QStringLiteral("~~x~~")); + QCOMPARE(MarkdownFormat::wrap(QStringLiteral("x"), 0, 1, + QStringLiteral("`")).text, + QStringLiteral("`x`")); +} + +void TestFormatToolbar::aBackwardsSelectionWrapsTheSameWordsAsAForwardOne() +{ + // A drag from right to left reports the anchor after the cursor. Qt hands + // that over as-is, so a transformation that trusts the order inserts the + // closing token before the opening one and corrupts the buffer. + const MarkdownFormat::Edit edit = MarkdownFormat::wrap( + QStringLiteral("make this bold"), 9, 5, QStringLiteral("**")); + + QCOMPARE(edit.text, QStringLiteral("make **this** bold")); + QCOMPARE(edit.text.mid(edit.selectionStart, + edit.selectionEnd - edit.selectionStart), + QStringLiteral("this")); +} + +void TestFormatToolbar::wrappingTwiceNestsTheTokensAroundTheSameWords() +{ + // The reason the selection is preserved at all: bold, then italic, + // without touching the mouse. Asserting on the second result is what + // makes the preserved selection load-bearing rather than decorative, + // since a wrong selection here produces valid-looking but wrong markdown + // ("make ***this** bold*" or similar). + // + // Stacking rather than toggling is the spec's behaviour, not an + // omission: a second Bold press gives "****this****". A toggle is wanted + // eventually and would make THIS gesture unreachable, which is the + // unanswered design question recorded as backlog item 135. + const MarkdownFormat::Edit first = MarkdownFormat::wrap( + QStringLiteral("make this bold"), 5, 9, QStringLiteral("**")); + const MarkdownFormat::Edit second = MarkdownFormat::wrap( + first.text, first.selectionStart, first.selectionEnd, + QStringLiteral("*")); + + QCOMPARE(second.text, QStringLiteral("make ***this*** bold")); + QCOMPARE(second.text.mid(second.selectionStart, + second.selectionEnd - second.selectionStart), + QStringLiteral("this")); +} + +void TestFormatToolbar::aLinkWithASelectionUsesItAsTheLabel() +{ + const MarkdownFormat::Edit edit = MarkdownFormat::link( + QStringLiteral("see the docs"), 8, 12); + + QCOMPARE(edit.text, QStringLiteral("see the [docs]()")); + + // The cursor goes inside the parentheses: the label is written and the + // URL is what the user still has to type. + QCOMPARE(edit.selectionStart, edit.selectionEnd); + QString typed = edit.text; + typed.insert(edit.selectionStart, QStringLiteral("https://example.org")); + QCOMPARE(typed, QStringLiteral("see the [docs](https://example.org)")); +} + +void TestFormatToolbar::aLinkWithNoSelectionLeavesTheCursorInTheLabel() +{ + // With nothing selected there is no label yet, so the label is what the + // user types first. + const MarkdownFormat::Edit edit = MarkdownFormat::link(QString(), 0, 0); + + QCOMPARE(edit.text, QStringLiteral("[]()")); + QCOMPARE(edit.selectionStart, edit.selectionEnd); + QString typed = edit.text; + typed.insert(edit.selectionStart, QStringLiteral("label")); + QCOMPARE(typed, QStringLiteral("[label]()")); +} + +void TestFormatToolbar::aBackwardsSelectionLinksTheSameWordsAsAForwardOne() +{ + const MarkdownFormat::Edit edit = MarkdownFormat::link( + QStringLiteral("see the docs"), 12, 8); + + QCOMPARE(edit.text, QStringLiteral("see the [docs]()")); + QString typed = edit.text; + typed.insert(edit.selectionStart, QStringLiteral("https://example.org")); + QCOMPARE(typed, QStringLiteral("see the [docs](https://example.org)")); +} + +void TestFormatToolbar::quotingPrefixesEveryLineTheSelectionTouches() +{ + const MarkdownFormat::Edit edit = MarkdownFormat::quote( + QStringLiteral("one\ntwo\nthree"), 0, 7); + + QCOMPARE(edit.text, QStringLiteral("> one\n> two\nthree")); +} + +void TestFormatToolbar::quotingAPartialLineStillQuotesTheWholeLine() +{ + // A selection from the middle of one line into the middle of the next + // must quote both whole lines. Quoting half a line produces markdown that + // means something else entirely. + const MarkdownFormat::Edit edit = MarkdownFormat::quote( + QStringLiteral("one\ntwo\nthree"), 1, 5); + + QCOMPARE(edit.text, QStringLiteral("> one\n> two\nthree")); +} + +void TestFormatToolbar::quotingASingleLineWithNoSelectionQuotesThatLine() +{ + const MarkdownFormat::Edit edit = MarkdownFormat::quote( + QStringLiteral("one\ntwo"), 5, 5); + + QCOMPARE(edit.text, QStringLiteral("one\n> two")); +} + +void TestFormatToolbar::quotingWithTheCursorAtTheEndOfALineQuotesThatLineNotTheNext() +{ + // Position 3 is the end of "one", immediately BEFORE the newline, so the + // cursor is on the first line. Searching backwards from the cursor itself + // rather than from one before it finds that newline and quotes the SECOND + // line, which is the line the user is not on. The off-by-one is invisible + // in every other case because no newline sits at the search position. + const MarkdownFormat::Edit edit = MarkdownFormat::quote( + QStringLiteral("one\ntwo"), 3, 3); + + QCOMPARE(edit.text, QStringLiteral("> one\ntwo")); +} + +void TestFormatToolbar::quotingSelectsTheQuotedLines() +{ + // The quoted block stays selected, so pressing Quote again nests it and + // a following transformation applies to the same lines. + const MarkdownFormat::Edit edit = MarkdownFormat::quote( + QStringLiteral("one\ntwo\nthree"), 1, 5); + + QCOMPARE(edit.text.mid(edit.selectionStart, + edit.selectionEnd - edit.selectionStart), + QStringLiteral("> one\n> two")); +} + +void TestFormatToolbar::quotingTheLastLineKeepsTheRestOfTheText() +{ + // No trailing newline after the last line, so the end-of-text search + // returns -1 and an unguarded implementation truncates everything from + // the selection onwards. + const MarkdownFormat::Edit edit = MarkdownFormat::quote( + QStringLiteral("one\ntwo\nthree"), 9, 9); + + QCOMPARE(edit.text, QStringLiteral("one\ntwo\n> three")); +} + +void TestFormatToolbar::quotingSelectsOnlyTheLineItQuoted() +{ + // The line quoted here is the SECOND one, so a selection that wrongly + // starts at 0 is distinguishable from a correct one. The existing + // quotingSelectsTheQuotedLines fixture starts on the first line, where a + // hardcoded 0 and the right answer coincide: that coincidence let a + // mutation replacing firstLineStart with 0 pass the whole suite. + // + // The damage is not cosmetic. With the wrong selection a second Quote + // press quotes a line the user never selected, and a following Bold + // bolds the wrong text. + const MarkdownFormat::Edit edit = MarkdownFormat::quote( + QStringLiteral("one\ntwo\nthree"), 5, 5); + + QCOMPARE(edit.text, QStringLiteral("one\n> two\nthree")); + QCOMPARE(edit.text.mid(edit.selectionStart, + edit.selectionEnd - edit.selectionStart), + QStringLiteral("> two")); +} + +void TestFormatToolbar::quotingAnAlreadyQuotedLineNestsIt() +{ + // Nests rather than toggling, per the spec, which states there is + // deliberately no live toggle that inserts and removes the quote while + // editing. A second press deepens the quote. Backlog item 135 holds the + // toggle design if that is ever revisited. + const MarkdownFormat::Edit edit = MarkdownFormat::quote( + QStringLiteral("> one"), 0, 5); + + QCOMPARE(edit.text, QStringLiteral("> > one")); +} + +void TestFormatToolbar::quotingAnEmptyLineLeavesTheMarkerWithoutTrailingSpace() +{ + // A blank line inside a quoted block is what continues the block in + // markdown, so it gets the marker. "> " with nothing after it is trailing + // whitespace that several editors and mail clients strip, which would + // break the block; the marker is written bare. + const MarkdownFormat::Edit edit = MarkdownFormat::quote( + QStringLiteral("one\n\ntwo"), 0, 8); + + QCOMPARE(edit.text, QStringLiteral("> one\n>\n> two")); +} + +void TestFormatToolbar::aSelectionPastTheEndIsClamped() +{ + // A stale selection outliving an edit to the buffer would otherwise index + // past the end. QString tolerates that in some calls and not in others, + // so it is clamped once at the entry rather than relied on per call. + QCOMPARE(MarkdownFormat::wrap(QStringLiteral("ab"), 0, 99, + QStringLiteral("**")).text, + QStringLiteral("**ab**")); + QCOMPARE(MarkdownFormat::link(QStringLiteral("ab"), -5, 99).text, + QStringLiteral("[ab]()")); + QCOMPARE(MarkdownFormat::quote(QStringLiteral("ab"), -5, 99).text, + QStringLiteral("> ab")); +} + +void TestFormatToolbar::aSelectionSplittingASurrogatePairKeepsTheCharacterWhole() +{ + // An emoji is two UTF-16 code units, so a boundary at 4 lands BETWEEN + // them. Inserting there splits the character: the result is invalid + // UTF-16 and the emoji is destroyed, not merely moved. + // + // Not reachable by arrow key or mouse, which both move in whole clusters, + // but QTextCursor::setPosition accepts it, so anything computing a + // position arithmetically gets there: a draft restore, a find/replace, a + // template insertion. + const QString emoji = QString::fromUcs4(U"\U0001F600"); + const QString text = QStringLiteral("hi ") + emoji + QStringLiteral(" there"); + QCOMPARE(text.size(), 11); + QVERIFY(text.at(3).isHighSurrogate()); + QVERIFY(text.at(4).isLowSurrogate()); + + // Boundary inside the pair on the closing side. + const MarkdownFormat::Edit a = + MarkdownFormat::wrap(text, 3, 4, QStringLiteral("**")); + QVERIFY2(a.text.isValidUtf16(), "wrap split the surrogate pair"); + QVERIFY2(a.text.contains(emoji), "wrap destroyed the character"); + + // Boundary inside the pair on the opening side. + const MarkdownFormat::Edit b = + MarkdownFormat::wrap(text, 4, 5, QStringLiteral("**")); + QVERIFY2(b.text.isValidUtf16(), "wrap split the surrogate pair"); + QVERIFY2(b.text.contains(emoji), "wrap destroyed the character"); + + const MarkdownFormat::Edit c = MarkdownFormat::link(text, 3, 4); + QVERIFY2(c.text.isValidUtf16(), "link split the surrogate pair"); + QVERIFY2(c.text.contains(emoji), "link destroyed the character"); + + // A COLLAPSED cursor inside the pair must stay collapsed. Nudging its two + // ends in opposite directions would keep the character whole while + // turning "insert an empty pair here" into "wrap the emoji", which is a + // character the user never selected. + const MarkdownFormat::Edit e = + MarkdownFormat::wrap(text, 4, 4, QStringLiteral("**")); + QVERIFY2(e.text.isValidUtf16(), "wrap split the surrogate pair"); + QCOMPARE(e.text, QStringLiteral("hi ****") + emoji + QStringLiteral(" there")); + QCOMPARE(e.selectionStart, e.selectionEnd); + QString typedInto = e.text; + typedInto.insert(e.selectionStart, QStringLiteral("x")); + QCOMPARE(typedInto, + QStringLiteral("hi **x**") + emoji + QStringLiteral(" there")); + + // quote() snaps to line boundaries, so it is immune by construction. + // Asserted rather than assumed, so a later change to how it widens the + // selection cannot quietly lose that. + const MarkdownFormat::Edit d = MarkdownFormat::quote(text, 3, 4); + QVERIFY2(d.text.isValidUtf16(), "quote split the surrogate pair"); + QCOMPARE(d.text, QStringLiteral("> ") + text); +} + +QTEST_APPLESS_MAIN(TestFormatToolbar) +#include "test_formattoolbar.moc" -- cgit v1.2.3 From d5ee2ab07d5313d153f62ce2ed8dcd027c677a09 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 21 Aug 2026 21:19:26 +0200 Subject: docs(backlog): record the undoMovesTheMessageBack flake, item 136 Found while running the suite during item 123 task 10, and checked rather than assumed to be unrelated: with the branch's work stashed out, on a clean tree, it still fails 1 run in 6. A failure that appears during unrelated work gets blamed on the change in front of it unless someone measures. Sized ? deliberately. The race is either in the test's wait or in the Maildir move Delete performs and Undo reverses, and the two have very different consequences: a test that waits wrongly is noise, while a move that races is mail landing in the wrong folder, which this document already records as reaching the mail server. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FXF741wz4SY7j5dqvAxMU5 --- .../plans/2026-08-03-post-0.1.0-usability.md | 35 ++++++++++++++++++++++ 1 file changed, 35 insertions(+) (limited to 'docs/superpowers/plans') 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 e9f99a4..051d6c4 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 @@ -205,6 +205,7 @@ taking that too literally. | 133 | The composer shows no markdown syntax highlighting | v2 | S | open, 2026-08-20, from the item 123 brainstorm. **Blocked on 123.** A `QSyntaxHighlighter` over the composer's editor, so `**bold**` reads as bold while the buffer stays plain markdown. Standard Qt, no dependency. Deliberately after 123's formatting toolbar: agreeing with the grammar about nesting and about code spans suppressing what is inside them is the expensive part, and the toolbar is what makes the feature usable | | 134 | The busy indicator is built inline and is about to be built twice | maintenance | S | done, 2026-08-20, af902e0. `BusyIndicator` (`src/busyindicator.h`) carries both modes: `MainWindow` uses the indeterminate one, and item 123's send popup takes the determinate half for its undo countdown, switching the same widget over when the command starts. Only the BAR was extracted, not the status label this row paired with it. `m_statusLabel` has 34 uses across `MainWindow` for transient messages, selection counts and sync phases, so it belongs to the window rather than to the indicator, and the send popup owns its own phase text | | 135 | The formatting toolbar's buttons stack rather than toggle | v2 | S | open, 2026-08-21, asked for by the user during item 123 task 8 and reverted the same session. **A spec change, not a defect**: it conflicts with spec:236 ("deliberately no live toggle") and spec:187-190. Both sites need amending FIRST, and the amendment must resolve what replaces bold-then-italic, which is the gesture spec:187's preserved selection exists to serve and which a toggle makes unreachable. That question is the work; the state machine is understood and written up in the section | +| 136 | `undoMovesTheMessageBack` fails about one run in six | defect | ? | open, 2026-08-21, found while running the suite during item 123 task 10. A pre-existing race in the test or in Delete's file move, NOT caused by 123: reproduced on a clean tree with the branch's work stashed out, 1 failure in 6 runs, and the failing run took 70s against a normal 25s. Unrelated to `SendDialog`. Size unknown until the race is located | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1213,3 +1214,37 @@ whether to open the spec at all, and leave the rest there. Name the spec `--design.md`, and state in its header which backlog items it resolves, so the numbering stays traceable in both directions. + +## 136. `undoMovesTheMessageBack` fails about one run in six + +**Observed.** `test_mainwindow` failed during a full-suite run while item 123 +task 10 was in the working tree. The failing function is +`TestMainWindow::undoMovesTheMessageBack`. The run that failed took 70 seconds +against a normal 25, so whatever goes wrong also blocks for a while before +giving up. + +**Not caused by item 123.** This was checked rather than assumed, because a +failure appearing during unrelated work is exactly the kind of thing that gets +blamed on the change in front of it. With the branch's work `git stash`ed out, +on a clean tree, it still failed **1 run in 6**. Nothing in `SendDialog` +touches the model, the Maildir, or the undo stack. + +**Cause, unverified.** A race around the Maildir file move that Delete +performs and Undo reverses. Whether the race is in the test's wait or in the +production move is exactly what the item has to establish, and that is why the +size is `?` rather than a guess. The two have very different consequences: a +test that waits wrongly is noise, while a move that races is mail landing in +the wrong folder, and CLAUDE.md already records that a wrong folder name from +this code path reaches the mail server. + +**Approach.** Reproduce in isolation first, with the suite's own +`QT_QPA_PLATFORM=offscreen` and a loop over `ctest -R mainwindow`, and capture +a failing run's output before theorising. The 70-second duration is the useful +clue: something is waiting on a condition that never arrives rather than +asserting immediately, so find which `QTRY_*` or `qWait` is timing out. + +**Constraints.** A fix must not restore the real `/proc/locks` (item 61), and a +flaky test must not be "fixed" by widening its timeout until it passes, which +converts a real race into a slower green. If the race turns out to be in the +production move rather than the test, this stops being a test-hygiene item and +becomes a mail-safety one. -- cgit v1.2.3 From a9d1cf73a91b5eef79a9722ec9921c97b9ec5c81 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 22 Aug 2026 11:19:22 +0200 Subject: feat(compose): wire the composer into the main window, item 123 The reply family is disabled on mail that arrived at an account with no send_command, behind a ribbon in MessageView naming the account and the key to add. save_message is deliberately never disabled: it is the escape hatch for exactly that case. The ribbon is a WIDGET in the pane's layout, never markup inside the web view. Composing HTML from configuration into the one document that renders input from strangers is the wrong direction, and the header row is already a widget for the same reason. Compose itself is disabled only when NO account can send, and that state is not warned about at startup: an installation with no send_command anywhere is a valid read-only installation. Every reply resolves through messageScopeFor(), not threadFor(): a thread row means the one message its card shows. Replying to a thread is meaningless; a reply answers a message. The context is built from the DATABASE rather than the model, the rule Restore already follows, because a row whose state has not been re-queried carries stale values and a reply built from one would carry the wrong recipients. The mail root crosses from the worker as its own signal. There was no route for it at all: mailRootOf() is file-static in notmuchworker.cpp, and item 124 records that composing a destination from database.path writes into the Xapian tree under a split index. The test uses NotmuchFixture::splitIndex(), the only layout where the two accessors disagree. A thread row's path is RELATIVE to the mail root while a message row's is absolute, so the account lookup matched nothing and the reply family was dead on mail from an account that could send. Found by the positive guard test rather than the negative one, which passed throughout for the wrong reason. The quit path checks the failed-save case FIRST. In the ordinary case nothing is lost by saving; there, saving is what is already not working, so the dialog says plainly that quitting loses that text rather than offering a save that will fail again. Both dialogs name the composers, and the ordinary one asks once whatever the count, because three modals in a row is worse than a coarse answer. Its wording says drafts already saved stay in the folder, so Discard cannot read as 'delete my three messages'. The Save loop holds QPointers, not raw pointers. A deleteLater() posted while a nested exec() runs IS processed by that nested loop, measured in a standalone program: the guard nulls before the modal returns. Closing a composer while the quit dialog is up therefore freed a window the loop then called saveDraftNow() on, crashing at the exact moment the application promised to preserve that text. A compose request that matches nothing clears itself and says so. It was cleared only on a match, so a message deleted between selection and Reply left the request armed for the session: Reply did nothing, and the next ordinary click on that message opened a composer nobody asked for while the pane stayed blank. Forward carries the original's attachments, which the context has always had a field for and nothing ever filled, and seeds its HTML toggle from [compose] send_html. Only Reply seeds that from the original. save_message keeps its filename inside the chosen directory and no longer overwrites a file already there. The check was correct and untested: the test asserted through Attachment's helpers rather than through the function production calls, so deleting the containment check outright left it green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TvwDptMWxjqhbCmjxwcSZ2 --- .../plans/2026-08-03-post-0.1.0-usability.md | 44 +- src/composewindow.cpp | 68 ++ src/composewindow.h | 44 + src/mainwindow.cpp | 625 +++++++++++++- src/mainwindow.h | 190 +++- src/messageview.cpp | 29 + src/messageview.h | 10 + src/notmuchworker.cpp | 32 +- src/notmuchworker.h | 20 + tests/test_mainwindow.cpp | 955 +++++++++++++++++++++ translations/qtmaildir_it_IT.ts | 66 ++ 11 files changed, 2062 insertions(+), 21 deletions(-) (limited to 'docs/superpowers/plans') 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 051d6c4..3fed54e 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 @@ -189,7 +189,7 @@ taking that too literally. | 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 | | 122 | The README documents a version of the app that no longer exists | documentation | M | open, 2026-08-20, from the notes. Delete-to-trash is entirely undocumented, including a config key a user must now set | -| 123 | Sending mail is not designed | v2 | L | **specified** 2026-08-20, on branch `compose-and-send`. Design in `docs/superpowers/specs/2026-08-20-compose-and-send-design.md`; read that, not this row. Send is a per-account `send_command` on stdin, so the no-network-protocol rule stands. Composer is a separate window, body is markdown via cmark-gfm, drafts autosave to the account's drafts folder. No code written | +| 123 | Sending mail is not designed | v2 | L | **specified** 2026-08-20, on branch `compose-and-send`. Design in `docs/superpowers/specs/2026-08-20-compose-and-send-design.md`; read that, not this row. Send is a per-account `send_command` on stdin, so the no-network-protocol rule stands. Composer is a separate window, body is markdown via cmark-gfm, drafts autosave to the account's drafts folder. Tasks 1 to 12 of 13 built 2026-08-20 to 2026-08-22; task 13, the close-out, is the remainder. **Never hand tested**: nothing had wired a composer to an action until task 12, so no composer has yet been opened by a human. Twenty-two defects were found in the plan's own draft code across tasks 4 to 12, so treat every code block in it as a draft | | 124 | The worker reads the index directory as the mail root | defect | S | **done** 2026-08-20, unreleased. `mailRootOf()` over `NOTMUCH_CONFIG_MAIL_ROOT`, correct under both layouts. Verified by migrating the developer's own index to NVMe the same day: cold start 38.6 s to 0.67 s | @@ -206,6 +206,7 @@ taking that too literally. | 134 | The busy indicator is built inline and is about to be built twice | maintenance | S | done, 2026-08-20, af902e0. `BusyIndicator` (`src/busyindicator.h`) carries both modes: `MainWindow` uses the indeterminate one, and item 123's send popup takes the determinate half for its undo countdown, switching the same widget over when the command starts. Only the BAR was extracted, not the status label this row paired with it. `m_statusLabel` has 34 uses across `MainWindow` for transient messages, selection counts and sync phases, so it belongs to the window rather than to the indicator, and the send popup owns its own phase text | | 135 | The formatting toolbar's buttons stack rather than toggle | v2 | S | open, 2026-08-21, asked for by the user during item 123 task 8 and reverted the same session. **A spec change, not a defect**: it conflicts with spec:236 ("deliberately no live toggle") and spec:187-190. Both sites need amending FIRST, and the amendment must resolve what replaces bold-then-italic, which is the gesture spec:187's preserved selection exists to serve and which a toggle makes unreachable. That question is the work; the state machine is understood and written up in the section | | 136 | `undoMovesTheMessageBack` fails about one run in six | defect | ? | open, 2026-08-21, found while running the suite during item 123 task 10. A pre-existing race in the test or in Delete's file move, NOT caused by 123: reproduced on a clean tree with the branch's work stashed out, 1 failure in 6 runs, and the failing run took 70s against a normal 25s. Unrelated to `SendDialog`. Size unknown until the race is located | +| 137 | A reply to a message that arrived at two accounts can come from the wrong one | defect | S | open, 2026-08-22, found while building item 123 task 12. `ComposeContextBuilder::accountForReply()` takes `messagePaths` PLURAL to disambiguate, and nothing upstream ever gives it more than one path, so the disambiguation is inert | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1215,6 +1216,47 @@ whether to open the spec at all, and leave the rest there. Name the spec `--design.md`, and state in its header which backlog items it resolves, so the numbering stays traceable in both directions. +## 137. A reply to a message that arrived at two accounts can come from the wrong one + +**Observed.** A message that exists in more than one maildir, because it was +sent to two of the user's addresses or duplicated across accounts by mbsync, +can open its reply from either account. Which one is picked is arbitrary. The +consequence is visible in the composer's From field, so it is not silent, but +it is only visible to somebody who thinks to look: the reply is otherwise +correct and sendable, and the recipient sees a From the user did not intend. + +**Cause, verified in the code.** The disambiguation exists and is unreachable. +`ComposeContextBuilder::accountForReply()` (`src/composecontext.cpp:405`) takes +`messagePaths` as a `QStringList` precisely so it can resolve this case: with +more than one candidate account it prefers the one whose own address appears +among the recipients, which is the reason the copy landed there. Nothing +upstream ever gives it more than one path. `NotmuchWorker::loadMessage()` +(`src/notmuchworker.cpp:573`) builds its `MessageRef` from +`notmuch_message_get_filename()`, the SINGULAR accessor, so `MessageRef` holds +one `filePath` and `MainWindow::openComposerFor()` can only pass a +one-element list. The plural parameter is therefore inert, and the branch that +consumes it is dead code today. + +`notmuch_message_get_filenames()`, the plural accessor that would supply the +rest, exists in libnotmuch and is used nowhere in this repository. + +**Approach.** Add `QStringList filePaths` to `MessageRef` (`src/types.h:123`) +ALONGSIDE the existing `filePath` rather than replacing it, and populate it in +`loadMessage()` from `notmuch_message_get_filenames()`. `filePath` stays as the +render path, so `MainWindow::renderMessages()` and everything else that opens +one file are untouched; only `openComposerFor()` reads the new field. That +keeps the change to two files plus the one call site. + +**Constraints.** The test has to put the same message id in two accounts' +maildirs, which `NotmuchFixture` can do by writing the same `Message-ID` into +two folders before indexing. Assert on the account CHOSEN rather than on a +count of paths: a test that only checks `filePaths.size() == 2` passes against +`accountForReply()` still ignoring them. The recipient-preference branch is +what needs covering, so the two accounts must have different addresses and the +message must be addressed to one of them, or either answer is correct and the +test proves nothing. + + ## 136. `undoMovesTheMessageBack` fails about one run in six **Observed.** `test_mainwindow` failed during a full-suite run while item 123 diff --git a/src/composewindow.cpp b/src/composewindow.cpp index 95b0a7b..0445c5d 100644 --- a/src/composewindow.cpp +++ b/src/composewindow.cpp @@ -18,8 +18,11 @@ #include "composewindow.h" +#include + #include "draftstore.h" #include "messagebuilder.h" +#include "mimeparser.h" #include "messagesender.h" #include "senddialog.h" @@ -124,6 +127,13 @@ ComposeWindow::ComposeWindow(const ComposeContext &context, buildFormatToolbar(); seedFields(); seedBody(); + + // AFTER buildUi(), which creates m_banner, and BEFORE + // refreshAttachmentList(), which renders m_attachments: extraction appends + // to that list, so listing first would show a Forward with no attachments + // on it, which is precisely the defect this fixes. + extractForwardedAttachments(); + refreshAttachmentList(); // Seeding is not an edit. Every field was just filled from the context, so @@ -135,6 +145,64 @@ ComposeWindow::ComposeWindow(const ComposeContext &context, m_autosaveTimer->stop(); } + +ComposeWindow::~ComposeWindow() = default; + +void ComposeWindow::extractForwardedAttachments() +{ + if (m_context.kind != ComposeContext::Kind::Forward + || m_context.originalPath.isEmpty()) { + return; + } + + MimeParser parser; + const ParsedMessage original = parser.parse(m_context.originalPath); + if (!original.ok || original.attachments.isEmpty()) + return; + + m_forwardedParts = std::make_unique(); + if (!m_forwardedParts->isValid()) { + m_forwardedParts.reset(); + m_banner->setText( + tr("The forwarded attachments could not be extracted.")); + m_banner->show(); + return; + } + + // Not auto-removed on destruction by accident: QTemporaryDir does this by + // default, and it is the whole reason the directory rather than the files + // is what this window owns. + m_forwardedParts->setAutoRemove(true); + + QStringList failed; + for (const Attachment &attachment : original.attachments) { + QString error; + // saveWithoutOverwriting, never saveTo. One message really can carry + // two parts with the same filename, and saveTo overwrites: CLAUDE.md + // records six of sixteen files lost that way, every write reporting + // success. Here it would silently forward fewer files than the + // original had. + const QString written = + attachment.saveWithoutOverwriting(m_forwardedParts->path(), &error); + if (written.isEmpty()) { + failed.append(attachment.safeFilename()); + continue; + } + m_attachments.append(written); + } + + if (!failed.isEmpty()) { + // Said out loud rather than swallowed. The composer looks entirely + // correct with an attachment missing, and the recipient gets a body + // quoting a document that is not there. + m_banner->setText( + tr("%n forwarded attachment(s) could not be extracted: %1", "", + failed.size()) + .arg(failed.join(QStringLiteral(", ")))); + m_banner->show(); + } +} + Account ComposeWindow::currentAccount() const { // The dropdown is the authority once the window is open: the context diff --git a/src/composewindow.h b/src/composewindow.h index 99803d7..af50be6 100644 --- a/src/composewindow.h +++ b/src/composewindow.h @@ -21,6 +21,8 @@ #include #include +#include + #include "config.h" #include "formattoolbar.h" // MarkdownFormat::Edit is used by value below, and // a type nested in a namespace cannot be @@ -36,6 +38,7 @@ class QListWidget; class QPlainTextEdit; class QTimer; class QToolBar; +class QTemporaryDir; class QWidget; class MessageSender; @@ -74,6 +77,12 @@ public: ComposeWindow(const ComposeContext &context, const Config &config, const QString &mailRoot, QWidget *parent = nullptr); + /// Defined in the .cpp, not defaulted here. m_forwardedParts is a + /// unique_ptr to a forward-declared QTemporaryDir, whose deleter needs the + /// complete type; an implicit destructor would be generated here, where it + /// is still incomplete. + ~ComposeWindow() override; + /// True when the buffer has changed since the last successful autosave. /// The quit path asks every open composer this. bool hasUnsavedEdits() const { return m_dirty; } @@ -139,6 +148,20 @@ private: void buildUi(); void buildFormatToolbar(); void seedFields(); + + /// Extracts a forwarded message's parts into m_forwardedParts and appends + /// their paths to m_attachments. + /// + /// The spec requires Forward to carry attachments, and they have to become + /// FILES because MessageBuilder reads every attachment by path. Extraction + /// happens here rather than in MainWindow so the files and the directory + /// that owns them are created together and die together. + /// + /// A part that cannot be written is SKIPPED with a banner rather than + /// failing the forward: some of the attachments is better than none, and + /// MessageBuilder refuses a build naming any path that later vanishes, so + /// a silently wrong send is not among the outcomes. + void extractForwardedAttachments(); void seedBody(); void refreshAttachmentList(); void setInputsEnabled(bool enabled); @@ -155,6 +178,27 @@ private: QString m_mailRoot; QStringList m_attachments; + /// Holds the parts a Forward extracted, for exactly as long as this window. + /// + /// Owned HERE rather than by MainWindow, because the lifetime that makes + /// sense is the composer's: MessageBuilder reads every attachment by PATH + /// at build time (messagebuilder.cpp:212), on each autosave and again at + /// send, so the files must outlive every build this window performs and + /// nothing after it. QTemporaryDir's destructor removes the tree, so + /// closing without sending cleans up rather than leaking. + /// + /// A draft does not depend on it. Autosave writes a COMPLETE MIME message + /// with the bytes embedded, so a saved draft stays valid after these files + /// are gone; and DraftStore is write-only, with no reopen path anywhere in + /// this codebase, so the "reopened next session pointing at a dead temp + /// path" hazard cannot arise. Should a reopen path ever be added, it must + /// read attachments back out of the draft's own MIME rather than trusting + /// a stored path. + /// + /// Null unless a Forward actually extracted something. unique_ptr because + /// QTemporaryDir is neither copyable nor movable. + std::unique_ptr m_forwardedParts; + QLineEdit *m_to = nullptr; QLineEdit *m_cc = nullptr; QLineEdit *m_bcc = nullptr; diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 5155c09..0131959 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -47,6 +48,8 @@ #include #include +#include "composecontext.h" +#include "composewindow.h" #include "mailsync.h" #include "messageview.h" #include "mimeparser.h" @@ -93,8 +96,42 @@ QString MainWindow::uiStatePath() namespace { /// Overridden only by setLocksPathForTesting(); "/proc/locks" in every real run. QString g_locksPath = QStringLiteral("/proc/locks"); + } // namespace +/// Doc comment on the declaration. Separators and control characters are +/// replaced rather than stripped so a subject carrying one yields a readable +/// name, instead of being truncated to its last segment by the basename +/// reduction Attachment::safeFilename() performs afterwards. +QString MainWindow::defaultMessageFilename(const QString &subject) +{ + QString name = subject.simplified(); + for (QChar &c : name) { + if (c == QLatin1Char('/') || c == QLatin1Char('\\') + || c == QLatin1Char(':') || c.category() == QChar::Other_Control) { + c = QLatin1Char('-'); + } + } + // Long subjects exist and many filesystems stop at 255 bytes. Truncated + // before the extension is added, so the cut cannot eat it. + name.truncate(120); + name = name.trimmed(); + + // A leading dot makes the file HIDDEN on every Unix desktop, and a subject + // beginning with one is ordinary ("...and another thing", or a traversal + // whose separators were just replaced above, leaving "..-..-etc-passwd"). + // The write succeeds and the user cannot see the file they just saved. + // Measured: QDir::entryList omits it without QDir::Hidden, which is how + // this was found. + while (name.startsWith(QLatin1Char('.'))) + name.remove(0, 1); + name = name.trimmed(); + + if (name.isEmpty()) + name = QStringLiteral("message"); + return name + QStringLiteral(".eml"); +} + void MainWindow::setLocksPathForTesting(const QString &path) { g_locksPath = path; @@ -202,6 +239,105 @@ void MainWindow::closeEvent(QCloseEvent *event) return; } + // Case 3 FIRST, because it is the one where saving is what is already not + // working: in case 2 nothing is lost by saving, here quitting loses that + // text, so the dialog must say so plainly rather than offering a save that + // will fail again. + QStringList failedSaves; + for (const QPointer &composer : m_composers) { + if (composer && composer->lastSaveFailed()) + failedSaves.append(composer->windowTitle()); + } + if (!failedSaves.isEmpty()) { + // The titles, not merely the count. The spec requires the dialog to + // NAME what could not be saved: "2 messages could not be saved" tells + // a user with four composers open nothing about which two to rescue. + // + // The list is a separate paragraph rather than interpolated into the + // sentence. The count and the list combine differently across + // languages, and a translator given "%n message(s) ...: %1" has to + // keep an English clause order Italian does not share. + QMessageBox box(this); + box.setIcon(QMessageBox::Warning); + box.setWindowTitle(tr("A draft could not be saved")); + box.setText(tr("%n message(s) could not be saved to the drafts " + "folder. Quitting now loses that text.", "", + failedSaves.size())); + box.setInformativeText(failedSaves.join(QLatin1Char('\n'))); + box.setStandardButtons(QMessageBox::Retry | QMessageBox::Discard + | QMessageBox::Cancel); + box.setDefaultButton(QMessageBox::Cancel); + const int answer = box.exec(); + + if (answer == QMessageBox::Cancel) { + event->ignore(); + return; + } + if (answer == QMessageBox::Retry) { + bool allSaved = true; + for (const QPointer &composer : m_composers) { + if (composer && composer->lastSaveFailed() + && !composer->saveDraftNow()) { + allSaved = false; + } + } + if (!allSaved) { + // Still failing: stay open rather than quitting on a retry + // that did not work, which would lose exactly the text the + // user pressed Retry to keep. + event->ignore(); + return; + } + } + } + + // Case 2: ONE dialog whatever the count. Three modals in a row is worse + // than a coarse answer, so it applies to all of them and there is no + // per-draft choice. + const QList> blocking = composersBlockingQuit(); + if (!blocking.isEmpty()) { + QStringList titles; + titles.reserve(blocking.size()); + for (const QPointer &composer : blocking) + titles.append(composer->windowTitle()); + + QMessageBox box(this); + box.setIcon(QMessageBox::Question); + box.setWindowTitle(tr("Messages still being composed")); + // "Discard" discards UNSAVED EDITS, not drafts: a draft already + // autosaved stays in the folder. The wording must not read as + // "delete my three messages". + box.setText(tr("%n message(s) are still being composed. Drafts " + "already saved stay in the drafts folder either way.", + "", blocking.size())); + box.setInformativeText(titles.join(QLatin1Char('\n'))); + box.setStandardButtons(QMessageBox::Save | QMessageBox::Discard + | QMessageBox::Cancel); + box.setDefaultButton(QMessageBox::Save); + const int answer = box.exec(); + + if (answer == QMessageBox::Cancel) { + event->ignore(); + return; + } + if (answer == QMessageBox::Save) { + // Null-checked per iteration, because `blocking` was computed + // BEFORE exec() and a nested event loop processes deleteLater(). + // The dialog is window-modal to this window only, so a user can + // close a composer while it is up; measured in a standalone Qt + // program, that composer is destroyed before exec() returns. + // Without this check the save runs on freed memory at the exact + // moment the application promised to preserve the text, and the + // remaining composers' drafts are never written because the crash + // happens mid-loop. Case 3's Retry loop above has always had the + // equivalent guard; this one had dropped it. + for (const QPointer &composer : blocking) { + if (composer) + composer->saveDraftNow(); + } + } + } + if (!m_closeApproved && pendingEditCount() > 0 && m_config.syncOnExit() != Config::SyncOnExit::Never) { @@ -758,25 +894,407 @@ void MainWindow::buildUi() setWindowTitle(QStringLiteral("qtmaildir %1").arg(QTMAILDIR_VERSION)); } -// The six compose handlers, empty until the composer exists (item 123). -// -// Deliberately empty rather than absent. Registering the actions first means -// everyKnownActionIsRegistered, everyActionCarriesAnIcon and -// everyActionIsReachableFromAMenu cover them while the composer is being -// built; a menu entry that does nothing yet is a smaller defect than an action -// nobody can reach, which is what those tests exist to catch. void MainWindow::composeNew() { + // m_accountBox->currentData() is how the selected account is read + // everywhere else in this file; there is no currentAccountKey() accessor. + // Empty means the All accounts view, which falls through to rule 2. + const QString accountKey = ComposeContextBuilder::accountForNew( + m_config, m_accountBox->currentData().toString()); + if (accountKey.isEmpty()) { + // Unreachable while the action is disabled, which is the only state + // this can be true in. Reported rather than returning silently: an + // action that runs and does nothing is the failure mode item 105 + // records as "the key does nothing". + showTransientStatus(tr("No account is configured to send mail")); + return; + } + + ComposeContext context; + context.kind = ComposeContext::Kind::New; + context.accountKey = accountKey; + context.seedHtml = m_config.compose().sendHtml; + + openComposer(context); } void MainWindow::composeReply(ComposeContext::Kind kind, bool quote) { - Q_UNUSED(kind); - Q_UNUSED(quote); + // messageScopeFor() semantics, NOT threadFor(): a thread row means the one + // message its card shows, a reply row means itself. Replying to a thread + // is meaningless; a reply answers a message. + // + // It takes a QModelIndexList, not a single index, so the current index is + // wrapped rather than passed bare. + const ActionScope scope = + m_model->messageScopeFor({ m_threadView->currentIndex() }); + if (scope.messageIds.isEmpty()) { + showTransientStatus(tr("No message is selected")); + return; + } + + // Built from the DATABASE, never from the model. The model's data comes + // from the query, so a row whose state has not been re-queried carries + // stale values, and a reply built from a stale row would carry the wrong + // recipients. This is the rule Restore already follows. + requestMessageForCompose(scope.messageIds.first(), kind, quote); +} + +void MainWindow::requestMessageForCompose(const QString &messageId, + ComposeContext::Kind kind, + bool quote) +{ + if (messageId.isEmpty()) + return; + + m_pendingCompose = { messageId, kind, quote, true }; + + // The same generation every other worker request carries, so a reply that + // arrives after the query moved on is discarded rather than opening a + // composer on a message the user is no longer looking at. + QMetaObject::invokeMethod(m_worker, "loadMessage", Qt::QueuedConnection, + Q_ARG(QString, messageId), + Q_ARG(quint64, m_generation)); +} + +void MainWindow::openComposerFor(const MessageRef &ref, + ComposeContext::Kind kind, bool quote) +{ + MimeParser parser; + const ParsedMessage original = parser.parse(ref.filePath); + if (!original.ok) { + showTransientStatus(tr("That message could not be read")); + return; + } + + ComposeContext context; + context.kind = kind; + context.originalPath = ref.filePath; + + const bool replyAll = kind == ComposeContext::Kind::ReplyAll; + const bool forwarding = kind == ComposeContext::Kind::Forward; + + if (!forwarding) { + ComposeContextBuilder::recipientsForReply( + original, replyAll, ComposeContextBuilder::ownAddresses(m_config), + &context.to, &context.cc); + + // Threading headers on a reply only. A forward starts a new + // conversation: carrying In-Reply-To would file it under the thread it + // was forwarded out of, in the RECIPIENT's client. + context.inReplyTo = original.messageId; + context.references = ComposeContextBuilder::referencesForReply(original); + } + + context.subject = forwarding + ? ComposeContextBuilder::forwardSubject(original.subject) + : ComposeContextBuilder::replySubject(original.subject); + + if (quote) + context.quotedBody = ComposeContextBuilder::quoteBody(original); + + // Forward seeds from the CONFIG, Reply from the original. The split is + // the spec's and Config::ComposeSettings::sendHtml states it too: an HTML + // part in the original is a fact about the SENDER's software, so it is the + // right seed when answering them and says nothing about a forward, which + // is a new message to somebody else. composeNew() already reads the config + // for the same reason. + context.seedHtml = forwarding ? m_config.compose().sendHtml + : original.hasHtml(); + + // accountForReply() takes messagePaths PLURAL because notmuch can return + // several filenames for one id, and it disambiguates between them by + // recipient. That disambiguation is INERT here, and the reason is upstream + // rather than a decision made at this call site: NotmuchWorker::loadMessage + // builds its MessageRef from notmuch_message_get_filename(), the SINGULAR + // accessor, so nothing in the pipeline ever carries more than one path and + // the list below can never hold more than one element. Backlog item 137 + // carries the fix (MessageRef gains a filePaths list populated from + // notmuch_message_get_filenames()); until then a message that arrived at + // two accounts can open its reply from the wrong one. + const QStringList recipients = context.to + context.cc; + context.accountKey = ComposeContextBuilder::accountForReply( + m_config, { ref.filePath }, recipients, m_mailRoot); + + if (context.accountKey.isEmpty() + || !m_config.account(context.accountKey).canSend()) { + // The enablement pass should already have stopped this, but it answers + // from the model's path while this answers from the database's, and + // the two can disagree on a row that has not been re-queried. + showTransientStatus( + tr("That message arrived at an account that cannot send")); + return; + } + + openComposer(context); +} + +void MainWindow::openComposer(const ComposeContext &context) +{ + if (m_mailRoot.isEmpty()) { + // Without the root a draft cannot be written anywhere, and a composer + // that silently cannot autosave is the state the quit path's honesty + // depends on not being in. + showTransientStatus(tr("The Maildir root is not known yet")); + return; + } + + auto *composer = new ComposeWindow(context, m_config, m_mailRoot); + composer->setAttribute(Qt::WA_DeleteOnClose); + m_composers.append(QPointer(composer)); + + // Compaction, and ONLY compaction. The QPointer above is what keeps + // composersBlockingQuit() safe against a destroyed window, since it nulls + // on destruction; this drops the entry so the list does not accumulate + // nulls for the session's lifetime. Neither replaces the other: without + // the signal the list leaks entries, without the QPointer it dangles. + connect(composer, &ComposeWindow::closed, this, + [this](ComposeWindow *which) { + m_composers.removeIf([which](const QPointer &p) { + return p.isNull() || p.data() == which; + }); + }); + + composer->show(); +} + +QList> MainWindow::composersBlockingQuit() const +{ + QList> blocking; + for (const QPointer &composer : m_composers) { + if (composer && composer->hasUnsavedEdits()) + blocking.append(composer); + } + return blocking; +} + +ComposeWindow *MainWindow::openComposerForTest() +{ + const QString accountKey = + ComposeContextBuilder::accountForNew(m_config, QString()); + if (accountKey.isEmpty()) + return nullptr; + + ComposeContext context; + context.kind = ComposeContext::Kind::New; + context.accountKey = accountKey; + + const int before = m_composers.size(); + openComposer(context); + if (m_composers.size() == before) + return nullptr; + return m_composers.constLast().data(); } -void MainWindow::saveDisplayedMessage() +QList MainWindow::openComposersForTest() const { + QList live; + for (const QPointer &composer : m_composers) { + if (composer) + live.append(composer.data()); + } + return live; +} + +int MainWindow::openComposerCount() const +{ + int live = 0; + for (const QPointer &composer : m_composers) { + if (composer) + ++live; + } + return live; +} + +void MainWindow::markComposersDirtyForTest() +{ + // Through the real edit path: the body editor's own textChanged is what + // ComposeWindow::markDirty() is connected to, so inserting text here + // exercises the same route typing does. Setting a dirty flag directly + // would pass against a composer that never notices an edit at all. + // + // QTextCursor rather than QTest::keyClicks, so production code does not + // have to link QtTest. + for (const QPointer &composer : m_composers) { + if (!composer) + continue; + if (auto *body = composer->findChild( + QStringLiteral("body"))) { + body->textCursor().insertText(QStringLiteral("x")); + } + } +} + +QString MainWindow::accountForCurrentMessage() const +{ + if (m_mailRoot.isEmpty()) + return {}; + + const QModelIndex current = m_threadView->currentIndex(); + if (!current.isValid()) + return {}; + + // The model's path, deliberately. This decides whether a CONTROL is live, + // which a stale path answers well enough; the context that actually opens + // a composer resolves the account again from the database. Asking the + // worker here would make every selection change a round trip. + // + // The two sources are in DIFFERENT FORMS and normalising them is not + // tidying. ThreadSummary::firstMessagePath is RELATIVE to the mail root, + // because runQuery() reduces it with relativeFilePath() so the UI can + // compare it against an account's maildir; MessageNode::filePath is + // ABSOLUTE, because MimeParser opens it. accountOwning() builds an + // absolute prefix, so handing it the relative one matches no account at + // all and every thread row reports no account, which disables the reply + // family on mail from an account that can perfectly well send. Measured: + // it did exactly that until the guard test caught it. + QString path; + if (m_model->isMessageRow(current)) { + path = m_model->messageAt(current).filePath; + } else { + path = m_model->threadFor(current).firstMessagePath; + } + if (path.isEmpty()) + return {}; + + const QString absolute = QDir::isAbsolutePath(path) + ? path + : QDir(m_mailRoot).absoluteFilePath(path); + + return ComposeContextBuilder::accountForReply(m_config, { absolute }, + QStringList(), m_mailRoot); +} + +void MainWindow::updateComposeActions() +{ + // The reply family is disabled on mail that arrived at an account which + // cannot send. save_message is deliberately NOT in this list: it is the + // escape hatch for exactly that case, writing the raw message to a file + // that can be attached to a new message from an account that can send. + const QString replyAccount = accountForCurrentMessage(); + const bool canReply = !replyAccount.isEmpty() + && m_config.account(replyAccount).canSend(); + + static const QStringList kReplyFamily = { + QStringLiteral("reply"), QStringLiteral("reply_all"), + QStringLiteral("reply_no_quote"), QStringLiteral("forward") + }; + for (const QString &name : kReplyFamily) { + if (QAction *action = m_actions.value(name)) + action->setEnabled(canReply); + } + + // The ribbon appears only when an account was identified AND it cannot + // send. An unidentified account is not a receive-only one: it is a message + // whose file no account owns, and naming no account in a ribbon that + // exists to name one would be worse than staying quiet. + const bool receiveOnly = + !replyAccount.isEmpty() && !m_config.account(replyAccount).canSend(); + m_messageView->setReceiveOnlyAccount(receiveOnly ? replyAccount + : QString()); + + // compose is disabled only when NO account can send. A read-only + // installation is valid and is not warned about. + if (QAction *compose = m_actions.value(QStringLiteral("compose"))) + compose->setEnabled(!m_config.sendingAccounts().isEmpty()); +} + +void MainWindow::saveDisplayedMessage(const QString &chosenDirectory) +{ + const QModelIndex current = m_threadView->currentIndex(); + const ActionScope scope = m_model->messageScopeFor({ current }); + if (scope.messageIds.isEmpty()) { + showTransientStatus(tr("No message is selected")); + return; + } + + // The path from the model, which is what the pane is rendering. Unlike a + // reply, a copy of the wrong file is visible to the user the moment they + // open it, so this does not need the database round trip a reply does. + QString sourcePath; + QString subject; + if (m_model->isMessageRow(current)) { + const MessageNode node = m_model->messageAt(current); + sourcePath = node.filePath; + subject = node.subject; + } else { + const ThreadSummary thread = m_model->threadFor(current); + sourcePath = thread.firstMessagePath; + subject = thread.subject; + } + if (sourcePath.isEmpty()) { + showTransientStatus(tr("That message's file could not be found")); + return; + } + + // Relative for a thread row, absolute for a message row. The same + // asymmetry accountForCurrentMessage() documents at length. + if (!QDir::isAbsolutePath(sourcePath) && !m_mailRoot.isEmpty()) + sourcePath = QDir(m_mailRoot).absoluteFilePath(sourcePath); + + if (!QFileInfo::exists(sourcePath)) { + showTransientStatus(tr("That message's file could not be found")); + return; + } + + // The dialog only when no directory was supplied. A test supplies one, + // because the modal cannot be driven under the offscreen platform and the + // containment check below is the only line guarding the write. + const QString directory = + chosenDirectory.isEmpty() + ? QFileDialog::getExistingDirectory( + this, tr("Save message to"), + QStandardPaths::writableLocation( + QStandardPaths::DownloadLocation)) + : chosenDirectory; + if (directory.isEmpty()) + return; // cancelled + + // The default name is derived from the SUBJECT, which is input from a + // stranger: it may carry path separators, "..", or nothing usable. The + // same rules the attachment path follows, and the same helpers, rather + // than a second implementation that has to be kept correct separately. + Attachment naming; + naming.filename = defaultMessageFilename(subject); + const QString safeName = naming.safeFilename(); + + // Disambiguated rather than overwritten, matching what the attachment bar + // does. Attachment::saveWithoutOverwriting() is the same rule and cannot + // be reused here because it writes an Attachment's own bytes, while this + // COPIES a file; the naming is duplicated, the behaviour is not. + // + // The earlier version deleted an existing same-named file, on the + // reasoning that a save the user just confirmed a location for should not + // silently do nothing. That is right about the failure and wrong about the + // remedy: two messages very often share a subject, so the second save + // would destroy the first, and QFile::copy's refusal is a reason to pick + // another name rather than to delete somebody's file. + const QFileInfo naming_info(safeName); + const QString base = naming_info.completeBaseName(); + const QString suffix = naming_info.suffix().isEmpty() + ? QString() + : QLatin1Char('.') + naming_info.suffix(); + const QDir dir(directory); + QString candidate = safeName; + for (int n = 2; dir.exists(candidate); ++n) + candidate = QStringLiteral("%1 (%2)%3").arg(base).arg(n).arg(suffix); + + const QString target = dir.absoluteFilePath(candidate); + + // Compared as PATHS, never with startsWith(): "/tmp/safe-evil" passes a + // startsWith("/tmp/safe") check while being a sibling directory. + if (!Attachment::isPathInsideDirectory(directory, target)) { + showTransientStatus(tr("Refusing to write outside %1") + .arg(QDir::cleanPath( + QDir(directory).absolutePath()))); + return; + } + + if (!QFile::copy(sourcePath, target)) { + showTransientStatus(tr("Could not write %1").arg(target)); + return; + } + showTransientStatus(tr("Saved %1").arg(target)); } QAction *MainWindow::addAction(const QString &name, const QString &text, @@ -1184,6 +1702,10 @@ void MainWindow::registerActions() // and offering "Mark all read" against nothing is a live control that does // nothing. updateViewWideActions(); + + // Compose and the reply family, for the same reason: QAction starts + // enabled, so a window with nothing selected would offer a live Reply. + updateComposeActions(); } void MainWindow::buildMenus() @@ -1725,6 +2247,8 @@ void MainWindow::wireWorker() this, &MainWindow::onWorkerError); connect(m_worker, &NotmuchWorker::allTagsReady, this, &MainWindow::onAllTagsReady); + connect(m_worker, &NotmuchWorker::mailRootReady, + this, &MainWindow::onMailRootReady); connect(m_worker, &NotmuchWorker::countsReady, this, &MainWindow::onCountsReady); connect(m_worker, &NotmuchWorker::databaseStatsReady, @@ -1761,6 +2285,11 @@ void MainWindow::wireWorker() // as the database can be read. Nothing waits on the answer: requestAllTags // stays silent when the database cannot be opened. requestAllTags(); + + // The Maildir root, which this window cannot derive (item 124). Asked once: + // it does not change while the application runs. Nothing waits on it + // either; the reply family is gated on send_command, not on this. + QMetaObject::invokeMethod(m_worker, "requestMailRoot", Qt::QueuedConnection); } void MainWindow::requestAllTags() @@ -1781,6 +2310,17 @@ void MainWindow::onAllTagsReady(const QStringList &tags) m_queryCompleter->setTags(tags); } +void MainWindow::onMailRootReady(const QString &mailRoot) +{ + m_mailRoot = mailRoot; + + // The enablement pass reads m_mailRoot to resolve which account owns the + // displayed message, so it answers "no account" until this arrives. A + // window that had already selected a row would otherwise keep the reply + // family greyed out until the next selection change. + updateComposeActions(); +} + QList MainWindow::placeholderLines() const { // One list of (query, label-maker) pairs rather than two arrays indexed in @@ -2734,6 +3274,11 @@ void MainWindow::onSelectionChanged() if (changed) onThreadSelected(current, QModelIndex()); } + + // Which account owns the displayed message decides whether the reply + // family is live and whether the ribbon shows, so it is re-answered + // whenever the displayed message can have changed. + updateComposeActions(); return; } @@ -2745,6 +3290,7 @@ void MainWindow::onSelectionChanged() if (m_statusLabel->text() == m_selectionMessage) m_statusLabel->clear(); m_selectionMessage.clear(); + updateComposeActions(); return; } @@ -2786,6 +3332,10 @@ void MainWindow::onSelectionChanged() m_currentMessageThreadId.clear(); m_messageView->clear(); showPlaceholderPane(); + + // A multi-row selection displays no message, so there is no account to + // reply from and no ribbon to show. + updateComposeActions(); } void MainWindow::onThreadSelected(const QModelIndex ¤t, @@ -2923,6 +3473,61 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, void MainWindow::onMessageLoaded(const QVector &messages, quint64 generation) { + // A compose request comes through this same signal rather than through a + // worker signal of its own, so it is answered before the render guards + // below: those exist to protect the PANE, and none of them applies to + // opening a composer. + // + // Matched by MESSAGE ID, not merely by a pending flag. The compose request + // and the pane share one loadMessage slot and one messageLoaded signal, so + // a pane load already in flight when the user presses Reply arrives FIRST + // and carries a different message: consuming it on the flag alone would + // open a composer on whichever message the pane happened to be loading. + // A non-matching reply falls through to the pane, which is what it is. + if (m_pendingCompose.active) { + const auto it = std::find_if( + messages.cbegin(), messages.cend(), + [this](const MessageRef &ref) { + return ref.messageId == m_pendingCompose.messageId; + }); + if (it != messages.cend()) { + const PendingCompose request = m_pendingCompose; + m_pendingCompose = {}; + + // The generation guard still applies: a query that moved on means + // the row the user asked from is gone. + if (generation == m_generation) + openComposerFor(*it, request.kind, request.quote); + + // A compose load carries no pane update: m_currentMessageId is + // untouched by requestMessageForCompose(), so falling through + // would repaint the pane with a message it did not select. + return; + } + + // No match, and the request is DISARMED rather than left waiting. + // + // Leaving it armed was a two-stage defect. The immediate half is that + // Reply silently does nothing when the message is not in the index, + // which is item 105's "the key does nothing". The delayed half is + // worse: the request stays armed with a specific message id, and the + // pane's own loads are the traffic being matched against, so merely + // SELECTING that message later would match, open a composer nobody + // asked for, and return before renderMessages() leaving the pane blank + // on the row just clicked. + // + // Only an EMPTY reply disarms it, and that asymmetry is the point. + // loadMessage() emits an empty list precisely when the id resolved to + // nothing, so that reply belongs to this request and says it failed. + // A NON-empty reply naming other messages is the pane's own load + // crossing ours, which is the race the id match exists to survive; + // disarming on it would reintroduce that race from the other side. + if (messages.isEmpty()) { + m_pendingCompose = {}; + showTransientStatus(tr("That message is no longer indexed")); + } + } + // A stale generation means the query moved on. A reply landing after the // selection grew past one row would paint a message back over a pane that // was deliberately blanked: loadMessage crosses to the worker on a queued diff --git a/src/mainwindow.h b/src/mainwindow.h index a3cd0ec..ea3ba61 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -63,6 +63,7 @@ class MailSync; class NotmuchWorker; class QueryCompleter; class TagRulesDialog; +class ComposeWindow; class MainWindow : public QMainWindow { @@ -310,6 +311,102 @@ public: onRulePreviewRequested(query); } + /// The open composers with unsaved edits, which the quit path asks about. + /// + /// PRODUCTION code, not a test accessor: closeEvent() reads it. Skips a + /// null QPointer, which is a composer the user already closed and whose + /// closed() signal has not compacted the list yet. + /// + /// Returns QPointers rather than raw pointers, and that is a SAFETY + /// property rather than a style. The quit path holds this list across + /// QMessageBox::exec(), and a nested event loop PROCESSES deleteLater(): + /// measured in a standalone Qt program, a parentless WA_DeleteOnClose + /// window closed while a modal is up is destroyed BEFORE exec() returns. + /// The dialog is window-modal to this window only, so the composers stay + /// interactive and the user really can close one from under it. A raw list + /// dangles there, and it dangles at the exact moment the application + /// promised to preserve their text. + QList> composersBlockingQuit() const; + + /// Opens a composer on a blank message from the first account that can + /// send, for a test that needs one open without a modal file dialog or a + /// selected row. Returns nullptr when no account can send. + ComposeWindow *openComposerForTest(); + + /// How many composers the registry currently holds, counting only entries + /// that are still alive. + /// + /// A nulled QPointer is NOT counted, so this cannot by itself distinguish + /// "the entry was removed" from "the entry is still there but nulled". + /// That distinction is what closingAComposerCompactsTheRegistry() exists + /// to make, and it makes it by asserting this reaches zero after a close: + /// only compaction can empty the list, since a nulled entry would leave + /// m_composers non-empty while this still reported zero. + int openComposerCount() const; + + /// Types a character into every open composer, which is what makes it + /// dirty. A test seam over the real edit path rather than a flag setter: + /// setting m_dirty directly would pass against a composer that never + /// notices an edit at all. + void markComposersDirtyForTest(); + + /// The Maildir root as the worker reported it, for the split-index test. + QString mailRootForTesting() const { return m_mailRoot; } + + /// Runs save_message into \p directory instead of asking for one. + /// + /// The file dialog is a modal the offscreen platform cannot click, and the + /// containment check is the only line guarding the write, so without this + /// seam no test can reach the guard it is named after. + void saveDisplayedMessageForTest(const QString &directory) + { + saveDisplayedMessage(directory); + } + + /// Builds a compose context from \p ref and opens the composer, which is + /// the production line openComposerFor() runs. A test that builds a + /// ComposeContext by hand instead proves only that ComposeWindow honours + /// what it is given, and cannot see which SOURCE a field came from. + void openComposerForTest(const MessageRef &ref, ComposeContext::Kind kind, + bool quote) + { + openComposerFor(ref, kind, quote); + } + + /// Arms a compose request without a selected row, so a test can request + /// one for an id the database does not hold. + void requestMessageForComposeForTest(const QString &messageId, + ComposeContext::Kind kind, bool quote) + { + requestMessageForCompose(messageId, kind, quote); + } + + /// Whether a compose request is still waiting for its message. + /// + /// A request that never disarms is the defect this exposes: it stays armed + /// with a message id and hijacks the next pane load for that message. + bool composeRequestPendingForTest() const { return m_pendingCompose.active; } + + /// The live composers, for a test that needs to close them. + /// + /// Defined in the .cpp: dereferencing a QPointer needs the complete type, + /// and ComposeWindow is only forward-declared here. + QList openComposersForTest() const; + + /// A default filename for a saved message, derived from its subject. + /// + /// Public and static so a test can assert on it with a hostile subject. + /// It was a file-local helper unreachable from any test, and the test + /// named after its defences asserted on Attachment's helpers directly + /// instead: three separate mutations left that test green. CLAUDE.md's + /// "a probe can be correct and still measure nothing, by being pointed at + /// the wrong object". + /// + /// The subject is UNTRUSTED, so this produces a CANDIDATE rather than a + /// safe name: the caller passes it through Attachment::safeFilename(), + /// which reduces it to a plain basename. + static QString defaultMessageFilename(const QString &subject); + protected: void closeEvent(QCloseEvent *event) override; @@ -481,6 +578,11 @@ private slots: void onTagsApplied(const TagChange &change); void onAllTagsReady(const QStringList &tags); + /// The Maildir root, answered once at startup. Enables nothing on its own: + /// the composer needs it, and the reply family is gated on the account's + /// send_command rather than on this having arrived. + void onMailRootReady(const QString &mailRoot); + /// Thread counts for the placeholder's helper lines, in the order /// requestPlaceholderCounts() asked for them. void onCountsReady(const QVector &counts, quint64 generation); @@ -595,25 +697,61 @@ private: void showMaildirOverview(); /// Opens a composer on a blank message (item 123). - /// - /// Empty for now. This is the registration commit: the six actions exist, - /// carry icons, sit in the Message menu and are covered by the three - /// coverage tests, so those tests guard the composer while it is built - /// rather than being satisfied once at the end. ComposeWindow does not - /// exist yet. void composeNew(); /// Opens a composer seeded from the displayed message (item 123). /// /// `kind` chooses reply, reply-all or forward; `quote` is what separates /// reply from reply-without-quoting, which are the same kind with and - /// without a seeded body. Empty for now, as above. + /// without a seeded body. + /// + /// Resolves through ThreadListModel::messageScopeFor(), NOT threadFor(): a + /// thread row means the one message its card shows. Replying to a thread + /// is meaningless, a reply answers a message. void composeReply(ComposeContext::Kind kind, bool quote); + /// Asks the worker for \p messageId's current file, then opens a composer. + /// + /// The round trip is the point. The context is built from the DATABASE and + /// never from the model, which is the rule Restore already follows: the + /// model's paths and tags come from the query, so a row that has not been + /// re-queried carries stale values and a reply built from one would go to + /// the wrong recipients. + void requestMessageForCompose(const QString &messageId, + ComposeContext::Kind kind, bool quote); + + /// Builds the context from a parsed message and shows the composer. + /// Called from onMessageLoaded() when a compose request is outstanding. + void openComposerFor(const MessageRef &ref, ComposeContext::Kind kind, + bool quote); + + /// Constructs a ComposeWindow, registers it and shows it. + void openComposer(const ComposeContext &context); + /// Writes the displayed message's raw file somewhere the user chooses. /// - /// Empty for now, as above. - void saveDisplayedMessage(); + /// Never disabled, including on a receive-only account: it is the escape + /// hatch for exactly that case, writing the raw message to a file that can + /// be attached to a new message from an account that can send. + /// + /// \p directory defaults to empty, which raises the file dialog. A test + /// passes one instead, via saveDisplayedMessageForTest(): the modal cannot + /// be driven under the offscreen platform, and the containment check below + /// it is the only line actually guarding the write, so with the dialog + /// inline no test could reach that line at all. + void saveDisplayedMessage(const QString &directory = QString()); + + /// The account a reply to the displayed message would send from, or empty + /// when there is no displayed message or no account owns its file. + /// + /// Read by the enablement pass, which is why it must not need a worker + /// round trip: it answers from the model's path, which is good enough to + /// decide whether a control is live. The context that actually opens a + /// composer resolves the account again from the database. + QString accountForCurrentMessage() const; + + /// Puts the reply family and compose into their real enabled state. + void updateComposeActions(); /// Creates a QAction, binds it to the sequence KeyMap holds for `name`, /// and registers it. `name` is the action name used in [keys]. @@ -1247,6 +1385,40 @@ private: /// back without clobbering a message some other action put there. QString m_selectionMessage; + /// The Maildir root, from the worker (item 124, and this window has no + /// other way to know it). + /// + /// There is no Config::maildirPath() by design: notmuch owns the path and + /// duplicating it into config would create a second source of truth. It + /// arrives on mailRootReady() shortly after startup, so anything composing + /// a path under it has to cope with it being empty for the first moments. + QString m_mailRoot; + + /// A compose request waiting for its message to come back from the worker. + /// + /// The reply family cannot open a composer synchronously: the context is + /// built from the database rather than from the model, so the file path + /// has to be fetched first. This records what to do with the answer. + struct PendingCompose + { + QString messageId; + ComposeContext::Kind kind = ComposeContext::Kind::Reply; + bool quote = true; + bool active = false; + }; + PendingCompose m_pendingCompose; + + /// Every open composer, so the quit path can see them. + /// + /// The QPointer and the closed() signal do DIFFERENT jobs and neither is + /// removable. A composer is WA_DeleteOnClose and deletes itself, so the + /// QPointer is what keeps composersBlockingQuit() from dereferencing a + /// destroyed window: it nulls on destruction. The signal is what lets this + /// list be COMPACTED, since a QPointer that nulled is still an entry and + /// the list would otherwise grow for the session's lifetime. Removing the + /// signal leaks entries; removing the QPointer crashes. + QList> m_composers; + /// Confirmed tag mutations not yet known to have reached the mail store. /// /// A count of its own rather than QUndoStack::isClean(), which cannot serve diff --git a/src/messageview.cpp b/src/messageview.cpp index 469d148..5682858 100644 --- a/src/messageview.cpp +++ b/src/messageview.cpp @@ -416,6 +416,17 @@ MessageView::MessageView(QWidget *parent) staleRow->addStretch(); m_staleBar->hide(); + // Receive-only ribbon (item 123). Hidden until a message from an account + // with no send_command is displayed. + m_receiveOnlyRibbon = new QLabel(this); + m_receiveOnlyRibbon->setObjectName(QStringLiteral("receiveOnlyRibbon")); + // Qt::PlainText explicitly. The account key comes from configuration + // rather than from a stranger, but a QLabel guesses under Qt::AutoText and + // this is the same protection MessageDetailsDialog states on every value. + m_receiveOnlyRibbon->setTextFormat(Qt::PlainText); + m_receiveOnlyRibbon->setWordWrap(true); + m_receiveOnlyRibbon->hide(); + m_attachmentBar = new QWidget(this); m_attachmentBar->setObjectName(QStringLiteral("attachmentBar")); new QHBoxLayout(m_attachmentBar); @@ -442,6 +453,7 @@ MessageView::MessageView(QWidget *parent) auto *layout = new QVBoxLayout(this); layout->addLayout(headerRow); layout->addLayout(blockedRow); + layout->addWidget(m_receiveOnlyRibbon); layout->addWidget(m_staleBar); layout->addWidget(m_view, 1); layout->addWidget(m_attachmentBar); @@ -1234,6 +1246,23 @@ void MessageView::saveAttachment(const Attachment &attachment) emit statusMessage(tr("Saved %1").arg(written)); } +void MessageView::setReceiveOnlyAccount(const QString &accountKey) +{ + if (accountKey.isEmpty()) { + m_receiveOnlyRibbon->hide(); + return; + } + + // Names the account AND the key to add. A ribbon saying only "you cannot + // reply" leaves the user with nothing to do about it, and the shape is + // expressed by omission, so there is no setting to go and look for. + m_receiveOnlyRibbon->setText( + tr("This account is receive-only. Add send_command to [account.%1] " + "to send from it.") + .arg(accountKey)); + m_receiveOnlyRibbon->show(); +} + void MessageView::setStaleThread(const QString &threadId, const QString &messageId) { diff --git a/src/messageview.h b/src/messageview.h index 3cc1604..044bded 100644 --- a/src/messageview.h +++ b/src/messageview.h @@ -128,6 +128,15 @@ public: /// Tags of the thread on display, shown as chips along the bottom. void setTags(const QStringList &tags); + /// Shows or hides the receive-only explanation, naming \p accountKey. + /// An empty key hides it. + /// + /// A WIDGET in this layout, never markup inside the web view. Composing + /// HTML from configuration into the one document that renders input from + /// strangers is the wrong direction, and the header row is already a + /// widget for the same reason. + void setReceiveOnlyAccount(const QString &accountKey); + /// The full headers of every message in the thread, read-only. Also /// reachable from the button beside the header; public so the window's /// message_details action can call it. @@ -391,6 +400,7 @@ private: QLabel *m_headerLabel = nullptr; QLabel *m_blockedLabel = nullptr; + QLabel *m_receiveOnlyRibbon = nullptr; QPushButton *m_loadRemoteButton = nullptr; /// The stale-thread notice and the thread it offers to restore. diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 8c28ec5..fca0a5a 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -540,8 +540,17 @@ void NotmuchWorker::loadThreadTree(const QString &threadId, void NotmuchWorker::loadMessage(const QString &messageId, quint64 generation) { - if (!openReadOnly()) + // Every failure below emits an EMPTY result as well as its error, and that + // is a contract rather than tidiness. The bottom of this function already + // said so ("emitted even when empty, so the UI's handler runs"), but the + // three failure paths returned silently and broke it. A caller that arms + // state on this request and disarms it on the reply then waits for ever: + // MainWindow's compose path did exactly that, and a request left armed + // hijacks a later pane load for the same message. + if (!openReadOnly()) { + emit messageLoaded({}, generation); return; + } // id: is an exact-match prefix, and the id is quoted because a message id // can legitimately contain characters notmuch's parser would otherwise read @@ -551,6 +560,7 @@ void NotmuchWorker::loadMessage(const QString &messageId, quint64 generation) if (!nmQuery) { emit errorOccurred( QStringLiteral("Cannot load message %1").arg(messageId)); + emit messageLoaded({}, generation); return; } @@ -559,6 +569,7 @@ void NotmuchWorker::loadMessage(const QString &messageId, quint64 generation) != NOTMUCH_STATUS_SUCCESS) { emit errorOccurred( QStringLiteral("Cannot search message %1").arg(messageId)); + emit messageLoaded({}, generation); return; } NmMessages messages(rawMessages); @@ -1018,6 +1029,25 @@ void NotmuchWorker::requestMessageCounts(const QStringList &queries, emit messageCountsReady(counts, generation); } +void NotmuchWorker::requestMailRoot() +{ + if (!openReadOnly()) { + // Answered anyway, with an empty root. A consumer waiting for this + // signal to enable something would otherwise wait for ever on a + // database that cannot be opened, which is the same silent stall + // loadMessage() emits an empty result to avoid. + emit mailRootReady(QString()); + return; + } + + // mailRootOf(), never notmuch_database_get_path(). Item 124: under a split + // config the latter names the INDEX directory, and a draft or a sent copy + // composed from it is written into the Xapian tree. + const QString root = mailRootOf(m_db); + emit mailRootReady(root.isEmpty() ? QString() + : QDir(root).absolutePath()); +} + void NotmuchWorker::requestFolders() { if (!openReadOnly()) diff --git a/src/notmuchworker.h b/src/notmuchworker.h index 9932e59..8ed878f 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -223,6 +223,20 @@ public slots: /// source of truth the design refuses. void requestFolders(); + /// The Maildir root, for whatever has to compose a path under it. + /// + /// This class owns the only database handle, and the root is a property of + /// the DATABASE rather than of config: notmuch can split the index from + /// the mail with `mail_root` and `path` as separate keys, so there is no + /// config key the UI could read instead. Item 124 records what the wrong + /// accessor costs. `notmuch_database_get_path()` returns the INDEX + /// directory under that layout, and a destination composed from it writes + /// into the Xapian tree. + /// + /// Requested at startup beside requestAllTags(), and answered once. The + /// root does not change while the application runs. + void requestMailRoot(); + signals: void threadsReady(const QVector &threads, quint64 generation); void queryFinished(int totalThreads, quint64 generation); @@ -288,6 +302,12 @@ signals: /// asks once when its dialog opens. void foldersReady(const QStringList &folders); + /// The Maildir root, absolute. No generation: it is a property of the + /// database rather than of any query, so a late answer is still the right + /// one. Empty when the database could not be opened, which a consumer must + /// treat as "cannot compose a path yet" rather than as the root being "". + void mailRootReady(const QString &mailRoot); + void errorOccurred(const QString &message); private: diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index cab6eae..ecaab2f 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -48,6 +48,7 @@ #include "keymap.h" #include "mainwindow.h" #include "messageview.h" +#include "mimeparser.h" #include "notmuchworker.h" #include "carddelegate.h" #include "composewindow.h" @@ -103,6 +104,35 @@ public: /// no trash key either. A caller that names an account and wants Delete to /// work has to say where its trash is, which is the same requirement the /// real config imposes. + /// One [account.] section to write. + /// + /// `sendCommand` is what makes the account able to send, and its EMPTINESS + /// is what makes it receive-only: the capability is the key's presence, + /// not a separate flag, so a receive-only account is written by omitting + /// it exactly as the real config expresses it. + struct AccountSpec + { + QString key; + QString maildir; + QString trash; + QString sendCommand; + QString address; + }; + + /// Writes several accounts, for the compose cases. + /// + /// Beside build() rather than replacing it: every existing caller passes + /// at most one account and none of them needs a send command, so widening + /// the three-argument signature further would make ten call sites carry + /// two empty strings each for one test's benefit. + bool buildWithAccounts(const QList &accounts, + const QString &composeKey = QString()) + { + m_accounts = accounts; + m_composeKey = composeKey; + return build(); + } + bool build(const QString &accountKey = QString(), const QString &accountMaildir = QString(), const QString &accountTrash = QString()) @@ -149,6 +179,21 @@ public: // folder that does not exist would CREATE it. out << "inbox=inbox\n"; } + if (!m_composeKey.isEmpty()) + out << "\n[compose]\n" << m_composeKey << "\n"; + for (const AccountSpec &account : m_accounts) { + out << "\n[account." << account.key << "]\n" + << "maildir=" << account.maildir << "\n" + << "inbox=inbox\n"; + if (!account.trash.isEmpty()) + out << "trash=" << account.trash << "\n"; + if (!account.address.isEmpty()) + out << "address=" << account.address << "\n"; + // Written only when non-empty. An account with no + // send_command is receive-only, which is the shape under test. + if (!account.sendCommand.isEmpty()) + out << "send_command=" << account.sendCommand << "\n"; + } } file.close(); @@ -169,6 +214,8 @@ private: QTemporaryDir m_confDir; Config m_config; QString m_error; + QList m_accounts; + QString m_composeKey; }; /// MainWindow is mostly wiring. Cases that need a real database opt into one @@ -204,6 +251,24 @@ private slots: void narrowingAnEmptyQueryBarIsAPlainSearch(); void aMalformedAccountIsReportedWithoutBlockingTheConstructor(); void aWorkerBackedWindowReturnsRealThreads(); + + // Compose and send, item 123 task 12. + void theMailRootComesFromTheConfigNotTheIndex(); + void replyIsDisabledOnAReceiveOnlyAccountsMail(); + void theReceiveOnlyRibbonNamesTheAccount(); + void replyIsEnabledOnASendingAccountsMail(); + void composeIsDisabledOnlyWhenNoAccountCanSend(); + void quittingWithACleanComposerAsksNothing(); + void quittingWithUnsavedEditsReportsEveryComposer(); + void closingAComposerCompactsTheRegistry(); + void savingAMessageRefusesToEscapeTheChosenDirectory(); + void aHostileSubjectCannotEscapeTheSaveDirectory(); + void savingTwiceDoesNotOverwriteTheFirstFile(); + void savingAMessageWithAHostileSubjectStaysInTheDirectory(); + void aStuckComposeRequestDoesNotHijackTheNextPaneLoad(); + void theSaveLoopToleratesAComposerClosedUnderTheDialog(); + void forwardingCarriesTheOriginalsAttachments(); + void forwardSeedsHtmlFromTheConfigNotTheOriginal(); void aStartupAccountScopesTheStartupQuery(); void aStartupAccountAlsoScopesASavedStartupQuery(); void aGeneratedStartupQueryActuallyRuns(); @@ -8168,6 +8233,896 @@ void TestMainWindow::aWorkerBackedWindowReturnsRealThreads() QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); } +namespace { + +/// A worker-backed window with one message in one account's maildir. +/// +/// The compose cases all need the same three things: a message on disk, an +/// account owning the folder it landed in, and a selected row. Repeating that +/// in six tests is how one of them ends up subtly different from the rest. +struct WorkerComposeFixture +{ + WorkerBackedWindow backed; + + /// Writes one message into /inbox and indexes it. + /// \p composeKey, when given, is written as one line under [compose]. + bool seed(const QList &accounts, + const QString &folder, const QString &composeKey = QString()) + { + if (!backed.fixture().addMessage( + folder, QStringLiteral("compose1@example.org"), + QStringLiteral("A subject"), + QStringLiteral("sender@example.org"), + // Friday, verified with `date -d 2026-08-14 +%A`. + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body text."))) { + return false; + } + return backed.buildWithAccounts(accounts, composeKey); + } + + /// Runs a query and puts the current index on its one row. + /// + /// Waits on the MAIL ROOT as well as on the row. The reply family is gated + /// on which account owns the message, which needs the root, and that + /// arrives on its own queued signal: asserting on an action's enabled + /// state before it lands measures the startup race rather than the rule. + static bool selectTheMessage(MainWindow &window) + { + auto *model = window.findChild(); + auto *view = window.findChild(); + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + if (!model || !view || !queryEdit) + return false; + + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + + bool ready = false; + for (int attempt = 0; attempt < 150 && !ready; ++attempt) { + ready = model->rowCount(QModelIndex()) == 1 + && !window.mailRootForTesting().isEmpty(); + if (!ready) + QTest::qWait(100); + } + if (!ready) + return false; + + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + return true; + } +}; + +} // namespace + +void TestMainWindow::theMailRootComesFromTheConfigNotTheIndex() +{ + // Item 124's rule, for the path the composer composes drafts and sent + // copies under. splitIndex() is what makes this test able to fail at all: + // in the ordinary layout notmuch_database_get_path() and + // NOTMUCH_CONFIG_MAIL_ROOT return the SAME string, so a test written + // against it passes whichever accessor the code uses. + WorkerComposeFixture fixture; + fixture.backed.fixture().splitIndex(); + QVERIFY2(fixture.seed({ { QStringLiteral("work"), QStringLiteral("work"), + QString(), QStringLiteral("/bin/true"), + QStringLiteral("you@example.org") } }, + QStringLiteral("work/inbox")), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + + QTRY_VERIFY_WITH_TIMEOUT(!window.mailRootForTesting().isEmpty(), 15000); + + // The MAIL root, not the index directory. Under the split layout these are + // different directories, and a draft composed under the index one is + // written into the Xapian tree. + QCOMPARE(window.mailRootForTesting(), + QDir(fixture.backed.fixture().maildirPath()).absolutePath()); + QVERIFY2(window.mailRootForTesting() + != QDir(fixture.backed.fixture().indexPath()).absolutePath(), + "the window took the index directory for the mail root"); +} + +void TestMainWindow::replyIsDisabledOnAReceiveOnlyAccountsMail() +{ + // The capability IS the send_command's presence, so this account is + // written without one. + WorkerComposeFixture fixture; + QVERIFY2(fixture.seed({ { QStringLiteral("listsonly"), + QStringLiteral("listsonly"), QString(), + /*sendCommand=*/QString(), + QStringLiteral("you@example.org") } }, + QStringLiteral("listsonly/inbox")), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + QVERIFY(WorkerComposeFixture::selectTheMessage(window)); + + for (const QString &name : { QStringLiteral("reply"), + QStringLiteral("reply_all"), + QStringLiteral("reply_no_quote"), + QStringLiteral("forward") }) { + auto *action = window.findChild(name); + QVERIFY2(action, qPrintable(QStringLiteral("no action %1").arg(name))); + QVERIFY2(!action->isEnabled(), + qPrintable(QStringLiteral("%1 was live on receive-only mail") + .arg(name))); + } + + // save_message is NEVER disabled, including here. It is the escape hatch + // for exactly this case: write the raw message out and attach it to a new + // message from an account that can send. + auto *save = window.findChild(QStringLiteral("save_message")); + QVERIFY(save); + QVERIFY2(save->isEnabled(), + "save_message was disabled, removing the escape hatch"); +} + +void TestMainWindow::replyIsEnabledOnASendingAccountsMail() +{ + // The guard for the test above. Without it, a bug disabling the reply + // family unconditionally would pass every assertion there while removing + // the feature entirely. + WorkerComposeFixture fixture; + QVERIFY2(fixture.seed({ { QStringLiteral("work"), QStringLiteral("work"), + QString(), QStringLiteral("/bin/true"), + QStringLiteral("you@example.org") } }, + QStringLiteral("work/inbox")), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + QVERIFY(WorkerComposeFixture::selectTheMessage(window)); + + for (const QString &name : { QStringLiteral("reply"), + QStringLiteral("reply_all"), + QStringLiteral("reply_no_quote"), + QStringLiteral("forward") }) { + auto *action = window.findChild(name); + QVERIFY2(action, qPrintable(QStringLiteral("no action %1").arg(name))); + QVERIFY2(action->isEnabled(), + qPrintable(QStringLiteral("%1 was disabled on mail from an " + "account that can send").arg(name))); + } + + // And no ribbon: this account can send, so there is nothing to explain. + auto *ribbon = + window.findChild(QStringLiteral("receiveOnlyRibbon")); + QVERIFY(ribbon); + QVERIFY2(ribbon->isHidden(), + "the receive-only ribbon showed on an account that can send"); +} + +void TestMainWindow::theReceiveOnlyRibbonNamesTheAccount() +{ + // The ribbon is a WIDGET in MessageView's layout, not markup inside the + // web view. Composing HTML from configuration into the one document that + // renders input from strangers is the wrong direction. + WorkerComposeFixture fixture; + QVERIFY2(fixture.seed({ { QStringLiteral("listsonly"), + QStringLiteral("listsonly"), QString(), + QString(), QStringLiteral("you@example.org") } }, + QStringLiteral("listsonly/inbox")), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + QVERIFY(WorkerComposeFixture::selectTheMessage(window)); + + auto *ribbon = + window.findChild(QStringLiteral("receiveOnlyRibbon")); + QVERIFY2(ribbon, "no ribbon widget exists"); + + // isHidden() rather than isVisibleTo(): under the offscreen platform an + // unshown window's children report not visible whatever the code does, so + // isVisibleTo would fail against correct code. What is being asserted is + // that the ribbon was not left explicitly hidden. + QVERIFY2(!ribbon->isHidden(), + "the ribbon did not appear on receive-only mail"); + QVERIFY2(ribbon->text().contains(QStringLiteral("listsonly")), + qPrintable(QStringLiteral("the ribbon does not name the account: %1") + .arg(ribbon->text()))); + + // PlainText, not AutoText. A QLabel guesses under AutoText, and this is + // the same protection MessageDetailsDialog states on every value. + QCOMPARE(ribbon->textFormat(), Qt::PlainText); +} + +void TestMainWindow::composeIsDisabledOnlyWhenNoAccountCanSend() +{ + // An installation with no send_command anywhere is a valid read-only + // installation and is not warned about; compose is simply unavailable. + { + WorkerComposeFixture fixture; + QVERIFY2(fixture.seed({ { QStringLiteral("listsonly"), + QStringLiteral("listsonly"), QString(), + QString(), QStringLiteral("you@example.org") } }, + QStringLiteral("listsonly/inbox")), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + auto *compose = window.findChild(QStringLiteral("compose")); + QVERIFY(compose); + QVERIFY2(!compose->isEnabled(), + "compose was live with no account able to send"); + } + { + WorkerComposeFixture fixture; + QVERIFY2(fixture.seed( + { { QStringLiteral("listsonly"), + QStringLiteral("listsonly"), QString(), QString(), + QStringLiteral("you@example.org") }, + { QStringLiteral("work"), QStringLiteral("work"), + QString(), QStringLiteral("/bin/true"), + QStringLiteral("work@example.org") } }, + QStringLiteral("listsonly/inbox")), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + auto *compose = window.findChild(QStringLiteral("compose")); + QVERIFY(compose); + QVERIFY2(compose->isEnabled(), + "compose was disabled although one account can send"); + } +} + +void TestMainWindow::quittingWithACleanComposerAsksNothing() +{ + // Case 1: every composer clean, quit directly, no dialog. A dialog here + // would be the "are you sure" this project deliberately does not do. + WorkerComposeFixture fixture; + QVERIFY2(fixture.seed({ { QStringLiteral("work"), QStringLiteral("work"), + QString(), QStringLiteral("/bin/true"), + QStringLiteral("you@example.org") } }, + QStringLiteral("work/inbox")), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + QTRY_VERIFY_WITH_TIMEOUT(!window.mailRootForTesting().isEmpty(), 15000); + + QVERIFY2(window.openComposerForTest(), "no composer opened"); + QCOMPARE(window.openComposerCount(), 1); + + QVERIFY2(window.composersBlockingQuit().isEmpty(), + "a clean composer was reported as blocking quit"); + + // Composers are parentless top-level windows and outlive this MainWindow, + // carrying a MessageSender and a running autosave timer into whatever test + // runs next. Closed here rather than left for the destructor, which never + // touches m_composers. + for (ComposeWindow *composer : window.openComposersForTest()) { + composer->show(); + composer->close(); + } +} + +void TestMainWindow::quittingWithUnsavedEditsReportsEveryComposer() +{ + // Case 2: ONE dialog whatever the count, so the quit path has to see BOTH + // composers rather than stopping at the first dirty one. + WorkerComposeFixture fixture; + QVERIFY2(fixture.seed({ { QStringLiteral("work"), QStringLiteral("work"), + QString(), QStringLiteral("/bin/true"), + QStringLiteral("you@example.org") } }, + QStringLiteral("work/inbox")), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + QTRY_VERIFY_WITH_TIMEOUT(!window.mailRootForTesting().isEmpty(), 15000); + + QVERIFY(window.openComposerForTest()); + QVERIFY(window.openComposerForTest()); + QCOMPARE(window.openComposerCount(), 2); + + // Clean until something is typed, which is the case-1 assertion holding + // here too and the guard that this test can distinguish the two states. + QVERIFY(window.composersBlockingQuit().isEmpty()); + + window.markComposersDirtyForTest(); + QCOMPARE(window.composersBlockingQuit().size(), 2); + + // Left open, these are parentless top-level windows with a live autosave + // timer, surviving into later tests. See the note in the clean-composer + // case above. + for (ComposeWindow *composer : window.openComposersForTest()) { + composer->show(); + composer->close(); + } +} + +void TestMainWindow::closingAComposerCompactsTheRegistry() +{ + // The closed() signal's ONE job. The QPointer alone would keep + // composersBlockingQuit() correct, since it nulls on destruction, but the + // entry would stay in the list for the session's lifetime. This asserts + // the list is compacted, which only the signal can do. + WorkerComposeFixture fixture; + QVERIFY2(fixture.seed({ { QStringLiteral("work"), QStringLiteral("work"), + QString(), QStringLiteral("/bin/true"), + QStringLiteral("you@example.org") } }, + QStringLiteral("work/inbox")), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + QTRY_VERIFY_WITH_TIMEOUT(!window.mailRootForTesting().isEmpty(), 15000); + + ComposeWindow *composer = window.openComposerForTest(); + QVERIFY(composer); + QCOMPARE(window.openComposerCount(), 1); + + // A composer that was never shown returns early from close() WITHOUT + // reaching closeEvent(), so the signal would never fire and this test + // would assert nothing at all. + composer->show(); + QVERIFY(composer->close()); + + // And the quit path must not see a destroyed window, which is the + // QPointer's job rather than the signal's. + QCOMPARE(window.openComposerCount(), 0); + QVERIFY(window.composersBlockingQuit().isEmpty()); +} + +void TestMainWindow::savingAMessageRefusesToEscapeTheChosenDirectory() +{ + // A subject is input from a stranger and is what the default filename is + // derived from, so it may carry separators and "..". Asserted through + // Attachment's own helpers, which is what saveDisplayedMessage() calls: + // a second implementation of the check here would prove nothing about the + // one that runs. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString directory = dir.path(); + + Attachment naming; + naming.filename = QStringLiteral("../../etc/passwd"); + const QString target = + QDir(directory).absoluteFilePath(naming.safeFilename()); + + QVERIFY2(Attachment::isPathInsideDirectory(directory, target), + "a traversing subject escaped the chosen directory"); + QVERIFY2(!target.contains(QStringLiteral("/etc/passwd")), + qPrintable(QStringLiteral("the traversal survived: %1").arg(target))); + + // Compared as PATHS, never with startsWith(): a sibling directory whose + // name merely begins with the chosen one's is not inside it. + QVERIFY2(!Attachment::isPathInsideDirectory( + directory, directory + QStringLiteral("-evil/message.eml")), + "a sibling directory passed the containment check"); +} + +void TestMainWindow::aHostileSubjectCannotEscapeTheSaveDirectory() +{ + // Asserted through MainWindow::defaultMessageFilename(), which is what + // saveDisplayedMessage() actually calls. The previous version of this + // check built an Attachment by hand and called safeFilename() directly: + // that proves what Attachment does and nothing about whether save_message + // asks it anything, and three mutations to the real path left it green. + // CLAUDE.md: assert through the function the production path calls, not + // through the one it calls INTO. + const QString traversal = + MainWindow::defaultMessageFilename(QStringLiteral("../../etc/passwd")); + + // No separator survives, so the name cannot address another directory. + QVERIFY2(!traversal.contains(QLatin1Char('/')), + qPrintable(QStringLiteral("a separator survived: %1").arg(traversal))); + // NOT asserting the absence of "..": with every separator replaced, a + // literal ".." inside a filename addresses nothing and is a legitimate + // part of a name. What matters is that the result is a single path + // COMPONENT, which is what makes traversal impossible. + QCOMPARE(QFileInfo(traversal).fileName(), traversal); + QVERIFY2(traversal != QStringLiteral("..") + && traversal != QStringLiteral("."), + qPrintable(QStringLiteral("the name is a directory reference: %1") + .arg(traversal))); + + // And joining it onto a directory really does stay inside. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Attachment naming; + naming.filename = traversal; + const QString target = + QDir(dir.path()).absoluteFilePath(naming.safeFilename()); + QVERIFY2(Attachment::isPathInsideDirectory(dir.path(), target), + qPrintable(QStringLiteral("escaped the directory: %1").arg(target))); + + // A backslash is a separator too, on a name written by Windows software. + const QString backslash = MainWindow::defaultMessageFilename( + QStringLiteral("..\\..\\Windows\\System32\\config")); + QVERIFY2(!backslash.contains(QLatin1Char('\\')), + qPrintable(QStringLiteral("a backslash survived: %1").arg(backslash))); + + // A subject with nothing usable still yields a name rather than "" or a + // bare extension, which would make the write land on a dotfile. + const QString empty = MainWindow::defaultMessageFilename(QString()); + QVERIFY2(empty.startsWith(QStringLiteral("message")), + qPrintable(QStringLiteral("empty subject gave: %1").arg(empty))); + + // The extension survives truncation. Truncating AFTER appending it would + // cut ".eml" off a long subject and write an extensionless file. + const QString long_ = MainWindow::defaultMessageFilename( + QString(400, QLatin1Char('a'))); + QVERIFY2(long_.endsWith(QStringLiteral(".eml")), + qPrintable(QStringLiteral("the extension was truncated away: %1") + .arg(long_.right(20)))); +} + +void TestMainWindow::savingTwiceDoesNotOverwriteTheFirstFile() +{ + // Two messages very often share a subject, and the filename is derived + // from it, so the second save must not destroy the first. Driven through + // saveDisplayedMessage() by way of the directory seam, which is the only + // way to reach the write guard at all: the file dialog is a modal the + // offscreen platform cannot click. + WorkerComposeFixture fixture; + QVERIFY2(fixture.seed({ { QStringLiteral("work"), QStringLiteral("work"), + QString(), QStringLiteral("/bin/true"), + QStringLiteral("you@example.org") } }, + QStringLiteral("work/inbox")), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + QVERIFY(WorkerComposeFixture::selectTheMessage(window)); + + QTemporaryDir out; + QVERIFY(out.isValid()); + + window.saveDisplayedMessageForTest(out.path()); + window.saveDisplayedMessageForTest(out.path()); + + // Two files, not one overwritten. Asserted on the COUNT rather than on the + // second name, so the disambiguation scheme can change without the test + // caring what it is called. + const QStringList written = + QDir(out.path()).entryList(QDir::Files | QDir::NoDotAndDotDot); + QCOMPARE(written.size(), 2); + + // And both are real copies rather than one empty placeholder. + for (const QString &name : written) { + QVERIFY2(QFileInfo(QDir(out.path()).absoluteFilePath(name)).size() > 0, + qPrintable(QStringLiteral("%1 is empty").arg(name))); + } +} + +void TestMainWindow::savingAMessageWithAHostileSubjectStaysInTheDirectory() +{ + // Driven through saveDisplayedMessage() with a real hostile subject, which + // is the only shape that covers the production write path. An earlier + // version of this coverage built an Attachment by hand and called + // safeFilename() and isPathInsideDirectory() directly, which proves what + // Attachment does and nothing about whether save_message asks it anything. + // + // WHAT THIS CAN AND CANNOT CATCH, measured rather than assumed, because + // the numbers are surprising and the next person will otherwise redo the + // work. Three independent layers stand between a subject and the write: + // defaultMessageFilename() replaces separators, Attachment::safeFilename() + // reduces to a basename, and Attachment::isPathInsideDirectory() refuses + // the write. EACH ONE ALONE IS SUFFICIENT, so removing any single layer + // leaves this test green: measured, all three single-layer mutations pass. + // Removing all three fails it. That is real defence-in-depth rather than a + // probe pointed at the wrong object, and mimeparser.h:71-77 already says + // the same of isPathInsideDirectory, but it does mean this test is a guard + // against the DEFENCES COLLECTIVELY disappearing, not a guard on any one + // of them. aHostileSubjectCannotEscapeTheSaveDirectory() covers the first + // layer on its own, and a single-layer mutation there does fail. + // + // The subject is ABSOLUTE rather than "../..", and that matters. + // QDir::absoluteFilePath() does not resolve ".." (measured: it + // concatenates), but the collision loop below can rename a relative + // traversal by accident when the target happens to exist, which makes it + // the weaker probe. An absolute candidate replaces the directory outright. + WorkerComposeFixture fixture; + QVERIFY(fixture.backed.fixture().addMessage( + QStringLiteral("work/inbox"), QStringLiteral("hostile@example.org"), + // The subject is the attacker's input, and it is what the default + // filename is derived from. + // Absolute, not "../..". QDir::absoluteFilePath() does NOT resolve + // ".." (measured: it concatenates, giving "/../../x"), but an + // ABSOLUTE candidate replaces the directory outright, which is the + // escape that survives every accident. A relative traversal can be + // neutralised by the collision loop renaming it when the target + // happens to exist, so it is the weaker probe of the two. + QStringLiteral("/tmp/qtmaildir-pwned-probe"), + QStringLiteral("sender@example.org"), + // Friday, verified with `date -d 2026-08-14 +%A`. + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body text."))); + QVERIFY2(fixture.backed.buildWithAccounts( + { { QStringLiteral("work"), QStringLiteral("work"), QString(), + QStringLiteral("/bin/true"), + QStringLiteral("you@example.org") } }), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + QVERIFY(WorkerComposeFixture::selectTheMessage(window)); + + // A directory INSIDE another, so an escape has somewhere to land that the + // test can then look at. Escaping "out" writes into parent/, which is what + // the assertions below check is still empty. + QTemporaryDir parent; + QVERIFY(parent.isValid()); + const QString out = parent.filePath(QStringLiteral("out")); + QVERIFY(QDir().mkpath(out)); + + window.saveDisplayedMessageForTest(out); + + // The file landed inside the chosen directory. + // NOT QDir::Hidden. A file whose name begins with a dot is hidden on every + // Unix desktop, so the write would succeed while the user could not find + // what they saved. Listing without Hidden is what makes this assertion + // notice that, and it is how the leading-dot case was found: a traversing + // subject reduces to "..-..-etc-passwd" once its separators are replaced, + // which is a dotfile. + const QStringList inside = + QDir(out).entryList(QDir::Files | QDir::NoDotAndDotDot); + QCOMPARE(inside.size(), 1); + QVERIFY2(!inside.first().startsWith(QLatin1Char('.')), + qPrintable(QStringLiteral("the saved message is hidden: %1") + .arg(inside.first()))); + + // And nothing was written beside it, which is where a traversal would go. + const QStringList escaped = + QDir(parent.path()).entryList(QDir::Files | QDir::NoDotAndDotDot); + QVERIFY2(escaped.isEmpty(), + qPrintable(QStringLiteral("a file escaped the directory: %1") + .arg(escaped.join(QLatin1Char(' '))))); + + // The written path really is contained, compared as PATHS rather than with + // startsWith(): a sibling directory whose name merely begins with the + // chosen one's is not inside it. + const QString written = QDir(out).absoluteFilePath(inside.first()); + QVERIFY2(Attachment::isPathInsideDirectory(out, written), + qPrintable(QStringLiteral("escaped: %1").arg(written))); + QVERIFY2(QFileInfo(written).size() > 0, "the saved message is empty"); +} + +void TestMainWindow::aStuckComposeRequestDoesNotHijackTheNextPaneLoad() +{ + // A compose request for a message that is not in the index used to stay + // armed for ever, because it was cleared only on the branch that FOUND the + // id. The delayed symptom is the bad one: the pane's own loads are the + // traffic being matched against, so merely selecting that message later + // matched, opened a composer nobody asked for, and returned before + // renderMessages() leaving the pane blank on the row just clicked. + WorkerComposeFixture fixture; + QVERIFY2(fixture.seed({ { QStringLiteral("work"), QStringLiteral("work"), + QString(), QStringLiteral("/bin/true"), + QStringLiteral("you@example.org") } }, + QStringLiteral("work/inbox")), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + QVERIFY(WorkerComposeFixture::selectTheMessage(window)); + + // Arm a request for an id the database does not hold. loadMessage() emits + // an empty result for it, which is what must disarm the request. + window.requestMessageForComposeForTest( + QStringLiteral("nosuchmessage@example.org"), + ComposeContext::Kind::Reply, true); + + // No composer, and the request stops being armed. + QTRY_VERIFY_WITH_TIMEOUT(!window.composeRequestPendingForTest(), 15000); + QCOMPARE(window.openComposerCount(), 0); + + // Now the delayed half. Select the real message: the pane must render it, + // and no composer may appear. With the request still armed this failed + // only if the ids matched, so the request is re-armed for the REAL id to + // make the hijack reachable at all. + window.requestMessageForComposeForTest( + QStringLiteral("compose1@example.org"), ComposeContext::Kind::Reply, + true); + QTRY_VERIFY_WITH_TIMEOUT(!window.composeRequestPendingForTest(), 15000); + + // That one DID match, so it opened a composer. Close it and clear the + // pane, then re-select and assert the pane renders rather than a second + // composer opening. + for (ComposeWindow *composer : window.openComposersForTest()) { + composer->show(); + composer->close(); + } + QCOMPARE(window.openComposerCount(), 0); + + auto *model = window.findChild(); + auto *view = window.findChild(); + QVERIFY(model && view); + view->setCurrentIndex(QModelIndex()); + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + + auto *pane = window.findChild(); + QVERIFY(pane); + QTRY_VERIFY_WITH_TIMEOUT(!pane->showingPlaceholder(), 15000); + QCOMPARE(window.openComposerCount(), 0); +} + +void TestMainWindow::theSaveLoopToleratesAComposerClosedUnderTheDialog() +{ + // The regression for a measured use-after-free. composersBlockingQuit() + // used to return raw pointers, and the quit path held that list across + // QMessageBox::exec(). A nested event loop PROCESSES deleteLater(), + // verified in a standalone Qt program: a parentless WA_DeleteOnClose + // window closed while a modal is up is destroyed BEFORE exec() returns. + // The dialog is window-modal to the main window only, so a user really can + // close a composer from under it, and Save then ran on freed memory. + // + // The modal itself cannot be driven under the offscreen platform, so what + // is asserted is the property that makes the loop safe: the list holds + // QPointers, and an entry whose window is destroyed reads as null rather + // than as a dangling pointer. That is exactly what the null check in the + // Save loop consumes. Stated plainly because it is NOT full coverage of + // closeEvent(): see the report. + WorkerComposeFixture fixture; + QVERIFY2(fixture.seed({ { QStringLiteral("work"), QStringLiteral("work"), + QString(), QStringLiteral("/bin/true"), + QStringLiteral("you@example.org") } }, + QStringLiteral("work/inbox")), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + QTRY_VERIFY_WITH_TIMEOUT(!window.mailRootForTesting().isEmpty(), 15000); + + QVERIFY(window.openComposerForTest()); + QVERIFY(window.openComposerForTest()); + window.markComposersDirtyForTest(); + + QList> blocking = window.composersBlockingQuit(); + QCOMPARE(blocking.size(), 2); + + // Destroy one exactly as closing it under the dialog would, including the + // deleteLater() a nested exec() would process. + ComposeWindow *doomed = blocking.first().data(); + QVERIFY(doomed); + doomed->show(); + doomed->close(); + QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete); + + // The held list reports it as gone rather than handing back a dangling + // pointer. A raw QList could not express this at all. + QVERIFY2(blocking.first().isNull(), + "the held entry did not null when its window was destroyed"); + QVERIFY2(!blocking.last().isNull(), + "the surviving composer was lost too"); + + // And the loop the quit path runs skips the null and still saves the + // survivor, which is the behaviour the crash destroyed: the remaining + // drafts were never written because the crash happened mid-loop. + int saved = 0; + for (const QPointer &composer : blocking) { + if (composer) { + composer->saveDraftNow(); + ++saved; + } + } + QCOMPARE(saved, 1); +} + +namespace { + +/// Writes a multipart/mixed message with one named attachment part. +/// +/// Hand-written rather than built with MessageBuilder: this is the INPUT to +/// the forward path, and generating it with the same library that consumes it +/// would let an encoding mistake agree with itself. +bool writeMessageWithAttachment(const QString &path, const QString &attachName, + const QByteArray &attachBody) +{ + QFile file(path); + if (!file.open(QIODevice::WriteOnly)) + return false; + QByteArray raw = + "From: sender@example.org\n" + "To: you@example.org\n" + "Subject: Quarterly report\n" + "Message-ID: \n" + // Friday, verified with `date -d 2026-08-14 +%A`. Qt::RFC2822Date + // validates the weekday against the date. + "Date: Fri, 14 Aug 2026 10:00:00 +0200\n" + "MIME-Version: 1.0\n" + "Content-Type: multipart/mixed; boundary=\"MIX\"\n" + "\n" + "--MIX\n" + "Content-Type: text/plain; charset=utf-8\n" + "\n" + "See the attached document.\n" + "--MIX\n" + "Content-Type: application/octet-stream; name=\"" + attachName.toUtf8() + "\"\n" + "Content-Disposition: attachment; filename=\"" + attachName.toUtf8() + "\"\n" + "\n" + attachBody + "\n" + "--MIX--\n"; + file.write(raw); + file.close(); + return true; +} + +/// Writes a multipart/alternative message that DOES carry a text/html part. +bool writeHtmlMessage(const QString &path) +{ + QFile file(path); + if (!file.open(QIODevice::WriteOnly)) + return false; + file.write( + "From: sender@example.org\n" + "To: you@example.org\n" + "Subject: Has HTML\n" + "Message-ID: \n" + "Date: Fri, 14 Aug 2026 10:00:00 +0200\n" + "MIME-Version: 1.0\n" + "Content-Type: multipart/alternative; boundary=\"ALT\"\n" + "\n" + "--ALT\n" + "Content-Type: text/plain; charset=utf-8\n" + "\n" + "plain\n" + "--ALT\n" + "Content-Type: text/html; charset=utf-8\n" + "\n" + "

html

\n" + "--ALT--\n"); + file.close(); + return true; +} + +} // namespace + +void TestMainWindow::forwardingCarriesTheOriginalsAttachments() +{ + // The spec requires Forward to carry attachments, twice. The context field + // existed and was never assigned, so a Forward opened with an empty + // attachment list: the composer looked entirely correct, and the recipient + // received a body quoting a document that was not attached, with nothing + // erroring anywhere. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString original = dir.filePath(QStringLiteral("original.eml")); + QVERIFY(writeMessageWithAttachment(original, QStringLiteral("report.pdf"), + QByteArray("PDFBYTES"))); + + QTemporaryDir confDir; + QVERIFY(confDir.isValid()); + const QString confPath = confDir.filePath(QStringLiteral("qtmaildir.conf")); + { + QSettings settings(confPath, QSettings::IniFormat); + settings.beginGroup(QStringLiteral("account.work")); + settings.setValue(QStringLiteral("maildir"), QStringLiteral("work")); + settings.setValue(QStringLiteral("address"), + QStringLiteral("you@example.org")); + settings.setValue(QStringLiteral("send_command"), + QStringLiteral("/bin/true")); + settings.endGroup(); + settings.sync(); + } + Config config; + config.load(confPath); + + ComposeContext context; + context.kind = ComposeContext::Kind::Forward; + context.accountKey = QStringLiteral("work"); + context.originalPath = original; + context.subject = QStringLiteral("Fwd: Quarterly report"); + + ComposeWindow composer(context, config, dir.path()); + + // The attachment is present, and it is a REAL FILE on disk rather than a + // remembered name: MessageBuilder reads every attachment by path at build + // time and refuses a build naming one that does not exist. + const QStringList attached = composer.attachments(); + QCOMPARE(attached.size(), 1); + QVERIFY2(QFileInfo::exists(attached.first()), + qPrintable(QStringLiteral("the extracted path does not exist: %1") + .arg(attached.first()))); + QCOMPARE(QFileInfo(attached.first()).fileName(), + QStringLiteral("report.pdf")); + + // And the bytes are the original's, not an empty placeholder. + QFile written(attached.first()); + QVERIFY(written.open(QIODevice::ReadOnly)); + QCOMPARE(written.readAll(), QByteArray("PDFBYTES")); + written.close(); + + // A Reply to the same message carries NOTHING. The spec says attachments + // are carried "for Forward, empty otherwise", and a reply that re-attached + // the original's documents would send them back to their own sender. + ComposeContext replyContext = context; + replyContext.kind = ComposeContext::Kind::Reply; + ComposeWindow replyComposer(replyContext, config, dir.path()); + QVERIFY2(replyComposer.attachments().isEmpty(), + "a reply carried the original's attachments"); +} + +void TestMainWindow::forwardSeedsHtmlFromTheConfigNotTheOriginal() +{ + // MEASURED, and it revises what the spec review reported. Forward was + // NEVER seeding from the original: ComposeWindow::seedFields() already + // implements the split itself (composewindow.cpp, `isReply ? + // m_context.seedHtml : m_config.compose().sendHtml`), so the context's + // value is IGNORED for a forward and the config won regardless. The + // openComposerFor() line this test also covers was therefore cosmetic + // rather than a live defect: it stopped the context carrying a value that + // nothing read, which is worth doing but changed no behaviour. + // + // The consequence for this test: EITHER layer alone enforces the rule, so + // neither single-layer mutation fails it, and only mutating both does. + // Verified in both directions rather than assumed. + // + // The spec splits these: New and Forward seed from [compose] send_html, + // Reply and Reply-all from whether the original carried a text/html part. + // An HTML part in the original is a fact about the SENDER's software, so + // it is the right seed when answering them and says nothing about a + // forward, which is a new message to somebody else. + // + // Asserted on the CONTEXT the window is built from rather than through the + // checkbox, because what is under test is which source the value comes + // from. The two sources must DISAGREE or the test passes either way: the + // config says false while the original is plain text, so reading the + // original would give false as well. Hence send_html=true against a plain + // original: config true, original false. + // The two sources must DISAGREE or the test passes whichever one is read, + // and getting that wrong is why an earlier version of this survived every + // mutation: config send_html=FALSE against an original that DOES carry a + // text/html part. Reading the original gives true, reading the config + // gives false, so the assertion below can only be satisfied one way. + WorkerComposeFixture fixture; + QVERIFY2(fixture.seed({ { QStringLiteral("work"), QStringLiteral("work"), + QString(), QStringLiteral("/bin/true"), + QStringLiteral("you@example.org") } }, + QStringLiteral("work/inbox"), + QStringLiteral("send_html=false")), + qPrintable(fixture.backed.error())); + + MainWindow window(fixture.backed.config()); + QTRY_VERIFY_WITH_TIMEOUT(!window.mailRootForTesting().isEmpty(), 15000); + QCOMPARE(fixture.backed.config().compose().sendHtml, false); + + // The original lives inside the account's maildir so accountForReply() + // can resolve it; its CONTENT is what matters, not that notmuch indexed it. + const QString original = + QDir(window.mailRootForTesting()) + .absoluteFilePath(QStringLiteral("work/inbox/cur/fwd-original")); + QVERIFY(writeHtmlMessage(original)); + + MimeParser parser; + const ParsedMessage parsed = parser.parse(original); + QVERIFY(parsed.ok); + QCOMPARE(parsed.hasHtml(), true); + + // Through openComposerFor(), which is the production line that chooses + // the source. Building the context by hand here and asserting on the + // checkbox proved only that ComposeWindow honours what it is given: the + // mutation putting `original.hasHtml()` back stayed green, because the + // test was setting seedHtml itself. + MessageRef ref; + ref.messageId = QStringLiteral("html-1@example.org"); + ref.filePath = original; + ref.matched = true; + + window.openComposerForTest(ref, ComposeContext::Kind::Forward, true); + + QList opened = window.openComposersForTest(); + QCOMPARE(opened.size(), 1); + auto *sendHtml = + opened.first()->findChild(QStringLiteral("sendHtml")); + QVERIFY(sendHtml); + QVERIFY2(!sendHtml->isChecked(), + "Forward seeded sendHtml from the original's HTML part rather " + "than from [compose] send_html"); + + // The counterpart, and it is what stops this asserting "always false": + // a REPLY to the same message seeds from the original, so it is checked + // where the forward is not. Without this half, disabling the checkbox + // outright would pass. + window.openComposerForTest(ref, ComposeContext::Kind::Reply, true); + const QList both = window.openComposersForTest(); + QCOMPARE(both.size(), 2); + auto *replyHtml = + both.last()->findChild(QStringLiteral("sendHtml")); + QVERIFY(replyHtml); + QVERIFY2(replyHtml->isChecked(), + "Reply did not seed sendHtml from the original's HTML part"); + + for (ComposeWindow *composer : both) { + composer->show(); + composer->close(); + } +} + void TestMainWindow::aStartupAccountScopesTheStartupQuery() { // "Start me in Work - Inbox rather than All accounts - Inbox." The account diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index b2d96eb..f8a2b03 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -570,6 +570,68 @@ Il messaggio È stato inviato. Non inviarlo di nuovo. Mark thread as &spam Segna conversazione come &spam + + A draft could not be saved + Impossibile salvare una bozza + + + %n message(s) could not be saved to the drafts folder. Quitting now loses that text. + + Impossibile salvare %n messaggio nella cartella delle bozze. Uscendo ora quel testo va perso. + Impossibile salvare %n messaggi nella cartella delle bozze. Uscendo ora quel testo va perso. + + + + Messages still being composed + Messaggi ancora in composizione + + + %n message(s) are still being composed. Drafts already saved stay in the drafts folder either way. + + %n messaggio è ancora in composizione. Le bozze già salvate restano comunque nella cartella delle bozze. + %n messaggi sono ancora in composizione. Le bozze già salvate restano comunque nella cartella delle bozze. + + + + No account is configured to send mail + Nessun account configurato per inviare posta + + + No message is selected + Nessun messaggio selezionato + + + That message could not be read + Impossibile leggere quel messaggio + + + That message arrived at an account that cannot send + Quel messaggio è arrivato a un account che non può inviare + + + The Maildir root is not known yet + La radice della Maildir non è ancora nota + + + That message's file could not be found + Impossibile trovare il file di quel messaggio + + + Save message to + Salva il messaggio in + + + Refusing to write outside %1 + Rifiuto di scrivere fuori da %1 + + + Could not write %1 + Impossibile scrivere %1 + + + Saved %1 + Salvato %1 + &Restore from trash &Ripristina dal cestino @@ -1361,6 +1423,10 @@ Il messaggio È stato inviato. Non inviarlo di nuovo. Saved %1 Salvato %1 + + This account is receive-only. Add send_command to [account.%1] to send from it. + Questo account è di sola ricezione. Aggiungi send_command a [account.%1] per inviare da esso. + No message in this thread has an HTML part Nessun messaggio di questa conversazione ha una parte HTML -- cgit v1.2.3 From a603b41771dc7c2612ca37f3d728c16717546a7e Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sun, 23 Aug 2026 21:00:31 +0200 Subject: docs(backlog): reconcile with the notes, items 138 to 147 The 2026-08-23 pass over the user's notes. Ten rows, eight sections: two of the ten are duplicates of items already open and get a row saying so rather than a second entry. Six of the eight are the compose interface, which had never been looked at by a human until this week. They are presentation rather than defect, and every cause is verified in the code rather than copied from the note: one toolbar carrying formatting, Attach and Send together (142), text buttons where an editor uses icons (143), a checkbox label that describes a mechanism without naming it (144), and Cc and Bcc as unconditional form rows (145). The other two are the message actions. Forward has no button anywhere (139), and the user's own answer to that is bigger than a missing button: Compose, Reply and Forward belong over the message pane rather than on a toolbar that otherwise acts on the list (140), which needs a bar that does not exist yet (141). 139 closes inside 140 if that is built. 138 is the closest thing to a defect here: every account already configures a drafts folder and the query row has no generator for it, so the one folder the composer writes to is the one folder with no button. Item 21 gains a pointer rather than a plan. The user is drafting the shortcut table in their own notes and it is the specification this item has been waiting for, so the entry says to read it and not to propose one. It records what the draft settles (Ctrl+Return for Send, which closes an open question from item 123 task 11), the two collisions its proposal creates (Ctrl+R is restore today, Ctrl+F is coupled to Find moving to `/`), and the trap under that last one: a `/` registered as a shortcut is dispatched before any editable widget sees it, so it needs the event filter Return already uses. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q2koFevoSxTLhfexJTZWQd --- .../plans/2026-08-03-post-0.1.0-usability.md | 234 ++++++++++++++++++++- 1 file changed, 233 insertions(+), 1 deletion(-) (limited to 'docs/superpowers/plans') 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 3fed54e..61b73c5 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 @@ -87,7 +87,7 @@ taking that too literally. | 18 | No visual cue that there are unsynced edits | feedback | S | **done** | | 19 | No prompt to sync on exit when edits are pending | behavior | S | **done** | | 20 | Thread view does not match the user's mental model | presentation | L | **done** 2026-08-10, as the card list; see 53 | -| 21 | Default shortcuts are not sensible enough | discoverability | S | open | +| 21 | Default shortcuts are not sensible enough | discoverability | S | open; **the user is drafting the table** in their own notes (`qtmaildir shortcuts and menu structure.md`), 2026-08-23. Read it first rather than proposing one. Settles `Ctrl+Return` for Send; leaves two collisions and an unfinished menu half, see the entry | | 22 | Translatability audit and i18n wiring | correctness | M | **done** 2026-08-15, unreleased; see `specs/2026-08-15-i18n-design.md`. Found eight rule-builder labels that could never be translated in any language, and twenty untranslatable warnings. Ships an Italian translation of all 355 strings | | 23 | No way to save a search query from the UI | workflow | M | **done** 2026-08-13, shipped in 0.18.0; see `specs/2026-08-13-saved-queries-design.md` | | 24 | No right-click actions on the thread list | discoverability | S | **done** | @@ -207,6 +207,16 @@ taking that too literally. | 135 | The formatting toolbar's buttons stack rather than toggle | v2 | S | open, 2026-08-21, asked for by the user during item 123 task 8 and reverted the same session. **A spec change, not a defect**: it conflicts with spec:236 ("deliberately no live toggle") and spec:187-190. Both sites need amending FIRST, and the amendment must resolve what replaces bold-then-italic, which is the gesture spec:187's preserved selection exists to serve and which a toggle makes unreachable. That question is the work; the state machine is understood and written up in the section | | 136 | `undoMovesTheMessageBack` fails about one run in six | defect | ? | open, 2026-08-21, found while running the suite during item 123 task 10. A pre-existing race in the test or in Delete's file move, NOT caused by 123: reproduced on a clean tree with the branch's work stashed out, 1 failure in 6 runs, and the failing run took 70s against a normal 25s. Unrelated to `SendDialog`. Size unknown until the race is located | | 137 | A reply to a message that arrived at two accounts can come from the wrong one | defect | S | open, 2026-08-22, found while building item 123 task 12. `ComposeContextBuilder::accountForReply()` takes `messagePaths` PLURAL to disambiguate, and nothing upstream ever gives it more than one path, so the disambiguation is inert | +| 138 | No Drafts filter beside Sent and Trash | workflow | S | open, 2026-08-23, from the notes. Verified: `kQueryGenerators` has no `drafts` entry, though every account already configures a `drafts` folder. Follows the `sent` generator exactly, which composes per-account folders rather than matching a tag | +| 139 | Forward is reachable only from the Message menu | discoverability | XS | open, 2026-08-23, from the notes. Verified: `forward` is added to `messageMenu` and to no toolbar. Compose and Reply are on the toolbar, so the third member of the set is the only one hidden | +| 140 | Compose, Reply and Forward belong over the message pane, not on the main toolbar | presentation | M | open, 2026-08-23, from the notes. The user's design: a bar of its own above the message pane carrying the three message actions, leaving the main toolbar for list-wide operations. Absorbs 139, which is the same three buttons in a worse place. See also 141 | +| 141 | The message pane has no button bar of its own | presentation | M | open, 2026-08-23, from the notes. The container 140 needs, and the home the user names for a `toggle_html` control. Sized as one item with 140 if built together | +| 142 | The composer's formatting buttons share a toolbar with Send and Attach | presentation | S | open, 2026-08-23, from the notes. Verified: one `addToolBar` carries Bold through Quote, then Attach, Remove attachment and Send. The user reads the row as a menu bar that is not one. Move the formatting half down to sit directly above the editor, beside the HTML checkbox | +| 143 | The formatting buttons are text, where every editor uses icons | presentation | XS | open, 2026-08-23, from the notes. Follows 142, and cheap once the row moves. `QIcon::fromTheme` per CLAUDE.md's chrome rule, with the text kept as the tooltip | +| 144 | "Also send a formatted copy" is prominent and does not say what it does | presentation | XS | open, 2026-08-23, from the notes. It means "send an HTML part as well as plain text", which the label never says. Secondary to writing the message, so it should read as such | +| 145 | Cc and Bcc are permanent rows on every composer | presentation | S | open, 2026-08-23, from the notes. Verified: both are unconditional `form->addRow` calls. Most messages use neither. Collapse behind a disclosure next to To:, expanded automatically when a draft or a reply already carries a value | +| 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 | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -241,6 +251,45 @@ that appeared to be bound. See `KeyMap::defaultBindings()` and the query bar claims it back while focused, so a proposal that moves it must not resurrect that bug. +**The user is drafting the table, 2026-08-23.** It lives in their own notes as +`qtmaildir shortcuts and menu structure.md`, linked from the note this item +came from, and it is the specification this item was waiting for: a row per +action with the current binding, the proposed one, and an explicit "no +shortcut" column for the actions that should have none. **Read it before +starting, and do not propose a table of your own.** It is unfinished in two +known places, so it is a starting point rather than a finished spec: + +- **The menu-structure half is one line long** ("File should hold Save + message") and is where the second half of this item's work is specified. +- **The compose actions are absent from it**, because it predates them being + usable by hand. The user's position as of 2026-08-23, stated but not yet + written into their table: `Ctrl+Return` for Send is **kept**, which closes + that open question from item 123 task 11; and major actions should not go + three modifiers deep, so Reply becomes `Ctrl+R`, Reply all `Ctrl+Shift+R`, + and Forward `Ctrl+F`. + +**Two collisions that proposal creates, both to settle before building.** +`Ctrl+R` is `restore` today, and the draft's own row for it says "ok if not +needed for something else" — it now is, so Restore needs a new binding or +none. And `Ctrl+F` is Find in most applications; the draft frees it by moving +Find to `/`, so the two are coupled, and if `/` does not survive review then +Forward loses its binding with it. + +**`/` for Find needs an event filter, not a shortcut.** Qt withholds only +plain LETTERS from editable widgets, so a `/` registered as a `QAction` +shortcut is dispatched before the query bar, the tag dialog and the composer's +editor ever see it, and a user could not type a path or a URL in any of them. +This is the same trap `CLAUDE.md` records for arrow keys, and `Return` is the +worked example of the fix: claim it in `MainWindow::eventFilter` by accepting +the `ShortcutOverride`, narrowly, for the one widget that needs it. + +**Dropping a shortcut is not dropping the action.** The draft marks the five +`*_thread` actions (item 108) for removal, and the user confirmed on +2026-08-23 that this means their SHORTCUTS only. The menu entries must stay: +`everyActionIsReachableFromAMenu()` is a required rule, while item 132 made +the shortcut itself optional, so an action with no binding is now ordinary and +prints as `(unbound)` in the shortcut reference. + ## 40. No live filter over the current view **Observed (user, 2026-08-05):** "search in current view", spelled out as two @@ -1290,3 +1339,186 @@ flaky test must not be "fixed" by widening its timeout until it passes, which converts a real race into a slower green. If the race turns out to be in the production move rather than the test, this stops being a test-hygiene item and becomes a mail-safety one. + +--- + +## 138. No Drafts filter beside Sent and Trash + +**Observed.** The query row carries Unread, Inbox, Important, Sent and Trash. +There is no Drafts button, though the composer has been writing drafts to each +account's drafts folder since item 123. + +**Cause, verified 2026-08-23.** `kQueryGenerators` in `config.cpp:62-66` is a +closed set of five, and `drafts` is not among them. Every account already +carries a `drafts` key (`config.cpp:453`), read for the composer's autosave, so +the data the filter needs is configured and unused by the query row. + +**Approach.** Follow `sent`, not `inbox`. A tag query would be wrong for the +same reason it is wrong for Sent: `draft` is a Maildir flag notmuch surfaces as +a tag, but the folder is what the user means, and a message a provider marks +differently would disagree. `Config::allSentQuery()` composes the union over +every account's folder and `Account::sentQuery()` the per-account half; both +need a drafts twin. The generator string is wire format and must stay `drafts` +in queries.json whatever the button is called in a given locale. + +**Constraints.** An account with no `drafts` key contributes NOTHING rather +than an empty term, or the button shows the whole Maildir: this is +`Config::matchNothingQuery()`'s reason for existing. The hook's carve-out +(`assets/hooks/qtmaildirconf.py`) reads the same key, so the two now agree on +what a drafts folder is; they are separate readers and neither should start +importing the other. + +--- + +## 139. Forward is reachable only from the Message menu + +**Observed.** There is no Forward button anywhere in the interface. The action +exists and works; it is in the Message menu and nowhere else. + +**Cause, verified 2026-08-23.** `mainwindow.cpp:1714` registers the action and +`1763` adds it to `messageMenu`. The toolbar block at `1975-1994` adds Compose +and Reply but never Forward, so two thirds of the message-action set are +visible and the third is not. + +**Approach.** One line, if the toolbar is where it belongs. It probably is not: +item 140 records the user's own view that all three belong over the message +pane instead, which makes this item the cheap half of that one. Build 140 and +this closes with it; build this alone only if 140 is deferred. + +**Constraints.** The no-duplicate-icons rule covers any action that can reach +the toolbar, so Forward needs an icon distinct from Reply's rather than a +variant of it. + +--- + +## 140. Compose, Reply and Forward belong over the message pane + +**Observed.** The user's note: "'Write new message' and 'reply' live next to +the other icon only buttons, but they belong in a new bar on top of the message +pane, together with 'Forward'." + +**Cause.** Not a defect. The toolbar grew by accretion and now mixes two +different scopes: Sync, Archive, Delete, Mark all read and Undo act on the LIST +or on the selection, while Compose, Reply and Forward are about a message. The +main toolbar reads as the place for everything, so the distinction is invisible. + +**Approach.** A bar above the message pane carrying the three message actions, +and the main toolbar keeping the list-wide ones. Compose is arguably neither, +since it needs no message at all; the user grouped it with the other two, and +that grouping is theirs to make. It shares the container item 141 introduces. + +**Constraints.** The actions themselves do not move: they stay in +`m_actions`, keep their shortcuts, and keep their menu entries, which is what +`everyActionIsReachableFromAMenu()` asserts on. This is a second presentation +of the same `QAction`s. Absorbs item 139. + +--- + +## 141. The message pane has no button bar of its own + +**Observed.** The user asks for "a button bar in the message pane area", and +names `toggle_html` as a control that would fit it. + +**Cause.** Nothing exists to hang such a control on. The pane is a header +label, the web view, the attachment bar and the tag strip; a per-message +control has no home, which is why `toggle_html` lives in a menu. + +**Approach.** The container item 140 needs. Whether it holds only the three +message actions, only view controls like `toggle_html`, or both is the design +question, and it should be settled with the user before building: a bar that +mixes "act on this message" with "change how I am looking at it" is the same +confusion item 140 exists to remove, one level down. + +**Constraints.** `MessageView` is built inline in its own class rather than +from named widget classes, per CLAUDE.md, and this should not become the +exception. Size assumes 140 and 141 are built together; separately they are +each S and the seam between them is wasted work. + +--- + +## 142. The composer's formatting buttons share a toolbar with Send and Attach + +**Observed.** The user reads the composer's top row as a menu bar that is not +one, and asks for the formatting controls to move down beside the HTML +checkbox, directly above the editor. + +**Cause, verified 2026-08-23.** `composewindow.cpp:326-397` builds ONE +`addToolBar`, which carries Bold, Italic, Code, the heading and list actions, +Link and Quote, then a separator, then Attach, Remove attachment and Send. +Three different scopes in one row: text formatting, message composition, and +the terminal action. + +**Approach.** Split it. The formatting half moves to a row directly above the +editor, where the text it formats is; Attach, Remove attachment and Send stay +in the window's own toolbar. The HTML checkbox already sits under the editor +and is the anchor the user names. + +**Constraints.** `setInputsEnabled()` disables `m_formatToolbar` wholesale +during a send (`composewindow.cpp:692`), so a split needs both halves disabled, +and a test for the send path that asserts on only one of them would pass +against a live Attach button during a send. + +--- + +## 143. The formatting buttons are text where every editor uses icons + +**Observed.** The user asks for icon-only formatting buttons, "like any other +text editor". + +**Cause.** They were built as text actions, and the composer's toolbar has no +icons at all. + +**Approach.** `QIcon::fromTheme` per CLAUDE.md's rule that chrome is the +system's, keeping the current text as the tooltip so nothing becomes +unnameable. Cheap once item 142 has moved the row, and awkward before, since +the same row would then mix icon-only formatting with text Send and Attach. + +**Constraints.** `format-text-bold` and its siblings are standard freedesktop +names, but a theme may not carry all of them; an action with no icon must fall +back to its text rather than rendering as an empty button. Icon-only is also +the state where the tooltip stops being decoration, so every one needs to be +right. + +--- + +## 144. "Also send a formatted copy" is prominent and says nothing + +**Observed.** The user's note: "I suppose it means 'format/send as html', but +that flag is secondary, doesn't need to be so prominent." + +**Cause, verified 2026-08-23.** `composewindow.cpp:265`. The label describes a +mechanism ("a formatted copy") without naming it, so the reader has to infer +that "formatted" means HTML and that "copy" means an additional MIME part +rather than a second message. + +**Approach.** Two independent halves, and they can ship separately. Say what it +does: something closer to "Send an HTML version as well". And demote it: it is +a per-message override of a config default (`[compose] send_html`), which is +secondary to writing the message. + +**Constraints.** The string is translated, so changing it makes the Italian +entry stale; `ctest -R translations` fails on an untranslated string, which is +the intended safety net rather than an obstacle. Nothing matches on this text, +so CLAUDE.md's "translating a string something matches on" trap does not apply. + +--- + +## 145. Cc and Bcc are permanent rows on every composer + +**Observed.** The user asks for them hidden behind a disclosure next to the To: +field. + +**Cause, verified 2026-08-23.** `composewindow.cpp:251` and `255` add both as +unconditional form rows. Most messages address neither, so two of the four +header rows are usually empty. + +**Approach.** A disclosure beside To: that reveals both together. They expand +automatically, and stay expanded, whenever either already carries a value: a +reply that carries Cc, or a reopened draft, must not hide a recipient the +message is actually addressed to. + +**Constraints.** That auto-expansion is the load-bearing half. A hidden field +holding an address is a message going somewhere the sender cannot see, which is +worse than the clutter this removes. The seeding runs before `buildUi()`'s +`markDirty()` connections per the constructor's ordering comment, so whatever +decides the initial state has to read the seeded values rather than the widgets. -- cgit v1.2.3 From ddcae8d02ef46db522b3cf6c228196c7a66a6432 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sun, 23 Aug 2026 21:13:51 +0200 Subject: docs: close out compose and send, and drop the v1/v2 split Task 13 of item 123, the documentation that was left when the code was finished. The README claimed "sending is not implemented. Compose, reply, forward and send are planned for v2 and need a companion send script that does not exist yet", which had been false for three days. It also documented none of `trash`, `send_command` or the `[compose]` section: `trash` has been effectively mandatory since item 103 and its absence breaks Delete, so a user reading only the README could not configure a working install. Both are item 122, which closes with this. Every default in the new [compose] block was read out of config.h rather than from the prose around it, which was worth doing: send_html defaults to TRUE and the first draft of this documented it as false, and the key is autosave_interval_ms rather than the autosave_delay_ms that reads more naturally. The v1/v2 language is retired rather than updated. The project follows semver on its user-visible surface and those labels described a scope split that stopped being true when compose shipped. CLAUDE.md says so explicitly rather than deleting the sentence, since the older spec and plan documents still use the phrase and are historical records: read it there as "before compose". CLAUDE.md's architecture diagram listed none of the compose units. It does now, and it distinguishes the classes from the namespaces, because most of them are namespaces of free functions over values: the markdown, the MIME assembly and the account-picking are all testable without a widget. There is no FormatToolbar class, which the first draft of this diagram invented, and that is the exact mistake the paragraph under it already warns about for QueryBar and AttachmentBar. Also records item 148 from the notes: Ctrl+W does not close the composer. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q2koFevoSxTLhfexJTZWQd --- CHANGELOG.md | 78 +++++++++++++++++++ CLAUDE.md | 35 +++++++-- README.md | 88 ++++++++++++++++++++-- .../plans/2026-08-03-post-0.1.0-usability.md | 5 +- 4 files changed, 190 insertions(+), 16 deletions(-) (limited to 'docs/superpowers/plans') diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f0748a..9e19ff2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,8 +11,86 @@ point at which they are stable. ## [Unreleased] +### Added + +- **Composing and sending.** Ctrl+N opens a composer; Reply, Reply all, Reply + without quoting and Forward start one from the selected message. Each is a + window in its own right, so several can be open at once and the main window + stays usable behind them. +- The body is markdown, sent as plain text exactly as typed. "Also send a + formatted copy" renders an HTML part from the same source and sends both in + a `multipart/alternative`; `[compose] send_html` sets the default, and a + reply follows what the message being answered used. +- Drafts autosave to the account's `drafts` folder as ordinary Maildir files, + so mbsync carries them to the server and another client can pick one up. + Closing a composer with unsaved edits asks first, and so does quitting with + one open. +- Sending goes to a per-account `send_command` on stdin, so any sendmail + compatible program works (msmtp, ssmtp, sendmail) and the credentials stay + in that program's own store. The application still speaks no network + protocol of its own. An account with no `send_command` is receive-only, and + the composer says so rather than failing at the end. +- A send counts down before it runs, and Undo during that window stops it and + returns you to the composer with everything intact. Nothing reaches the + network until the countdown ends. `[compose] send_delay_ms` sets the length; + 0 removes it. +- A copy of every sent message is filed in the account's `sent` folder. +- The notmuch hooks that auto-tag incoming mail now live in this repository, + under `assets/hooks/`. They moved from the companion `mailctl` project, + which is being retired. + ### Fixed +- Sent mail and drafts no longer appear in the inbox. notmuch tags every newly + indexed file with `inbox`, including the copy this application files after a + send and the drafts it autosaves, so both turned up in the Inbox view and in + any `tag:inbox` search. The `post-new` hook now removes it from mail inside a + configured `sent` or `drafts` folder, which is mail that never arrived. Only + `inbox` is touched, and trash is deliberately left alone so Restore can still + find where a message came from. +- Quitting with a composer open no longer leaves it behind. A composer is a + top-level window with no parent, so closing the main window did not take it + down and the process stayed alive for it: the main window vanished, the + composer stayed on screen, and closing it then asked about unsaved edits for + a session that had already ended. + +### Upgrading + +**To send, an account needs a `send_command`.** Without one it is +receive-only: it still reads, tags and syncs exactly as before, and the +compose actions are simply disabled for it. Nothing breaks by doing nothing. + +```ini +[account.work] +send_command = /usr/bin/msmtp -a work -t +``` + +The command receives the finished message on stdin and is run **without a +shell**, so pipes and redirections do not work; give an absolute path and +plain arguments. Credentials belong to that program, not to this one. + +An account that sends should also name `drafts` and `sent`, both relative to +its `maildir`. Without `drafts` a composer cannot autosave and says so; without +`sent` no copy of what you sent is kept locally. + +**If you run the auto-tagging hook, redeploy it.** It moved here from the +`mailctl` project and gained the sent-and-drafts carve-out described above. +Copy `assets/hooks/post-new`, `mailrules.py` and `qtmaildirconf.py` into +`/.notmuch/hooks/`, all three together: `post-new` imports the +other two, and the new one reads your `qtmaildir.conf` to learn which folders +are yours rather than arrivals. The rules file itself is unchanged, and +`mailctl` can still read it. + +**Existing sent mail and drafts keep their `inbox` tag**, since the hook only +sees newly indexed mail. To clear the backlog in one pass: + +```sh +notmuch tag -inbox -- 'tag:inbox and (path:"work/Sent/**" or path:"work/Drafts/**")' +``` + +naming your own folders. This is a tag change only: no file moves, nothing +reaches the server, and re-adding `inbox` to the same query undoes it. + - Clicking a link in a message opens it in the system browser. Links carrying `target="_blank"`, which is most links in HTML mail, did nothing at all: no error, nothing on screen. Chromium routes those to a new-window request diff --git a/CLAUDE.md b/CLAUDE.md index b065741..031b3d2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -64,10 +64,19 @@ MainWindow NotmuchWorker │ ONE column of cards; CardDelegate paints each whole, from CardLayout └ MessageView (header QLabel, QWebEngineView, attachment bar, TagStrip) +ComposeWindow (its own top-level window, one per message being written) + ├ MarkdownFormat (namespace: what the formatting buttons do to a selection) + ├ MarkdownRenderer (namespace, cmark-gfm) MessageBuilder (namespace, GMime) + ├ MessageSender (QProcess, the per-account send_command on stdin) + └ SendDialog (the undo countdown) DraftStore (autosave to the drafts folder) + +ComposeContext (a struct: what a Reply or Forward inherits) +ComposeContextBuilder (namespace: fills one, and picks the account) CardLayout (pure geometry, no painting) SearchTerm (pure query strings, no widget) Config (INI) KeyMap MailSync (QProcess) MimeParser (GMime) SyncMonitor (/proc/locks) TagColors QueryCompleter ThreadCidMap +MaildirName (fresh Maildir filenames) ``` The query row and the message-pane header are **built inline in `MainWindow` and @@ -75,11 +84,20 @@ The query row and the message-pane header are **built inline in `MainWindow` and listed `QueryBar`, `SavedQueryBar`, `HeaderWidget` and `AttachmentBar`; none of those types have ever existed, and looking for them wastes a search. The widget classes that do exist are `MessageView`, `ThreadListView`, `TagStrip`, -`TagDialog`, `MessageDetailsDialog`, `RowStyleDelegate` and `CardDelegate`; -`TagChip` is a namespace of painting helpers, not a widget, `SearchTerm` is a -namespace of query builders, and `ThreadCidMap`, `CardLayout`, `SearchOffer` -and `HeaderRow` are structs. `SubjectDelegate` existed until item 53 and is -gone. +`TagDialog`, `MessageDetailsDialog`, `RowStyleDelegate`, `CardDelegate`, +`ComposeWindow`, `SendDialog` and `BusyIndicator`; `TagChip` is a namespace of +painting helpers, not a widget, `SearchTerm` is a namespace of query builders, +and `ThreadCidMap`, `CardLayout`, `SearchOffer` and `HeaderRow` are structs. +`SubjectDelegate` existed until item 53 and is gone. + +**The compose units are mostly NAMESPACES, and the same warning applies to +them.** `MarkdownRenderer`, `MarkdownFormat`, `MessageBuilder`, +`ComposeContextBuilder`, `DraftStore` and `MaildirName` are namespaces of free +functions over values, deliberately, so the markdown, the MIME assembly and +the account-picking are all testable without a widget. `MessageSender` IS a +QObject, because it owns a `QProcess`. There is no `FormatToolbar` class: the +composer's formatting row is built inline in `ComposeWindow` and asks +`MarkdownFormat` what each button does to the selection. **`MessageDetailsDialog` was a `QPlainTextEdit` inside `MessageView` until item 85.** It is rows now so each value can carry its own context menu, and its @@ -925,7 +943,12 @@ test" position — it is the only code that writes to a notmuch index. Work goes directly on `master`, no PR flow. Commits must be GPG-signed (`git commit -S`). `HANDOFF.md` is local-only and gitignored; never stage or commit it. -v1 is read-and-organize only. Compose and send are v2. +**There is no "v1" and no "v2".** The project follows semver on its +user-visible surface, and those labels described a scope split that stopped +being true when compose and send shipped. Reading, organizing and sending are +all part of the application now. The phrase survives in the older spec and +plan documents, which are historical records and are not being rewritten; read +it there as "before compose" and "after compose". ## Cutting a release diff --git a/README.md b/README.md index 1801bbd..75f9cbe 100644 --- a/README.md +++ b/README.md @@ -1,8 +1,8 @@ # qtmaildir -A Qt6 desktop mail client for reading and organizing a local, -notmuch-indexed Maildir. A GUI counterpart to neomutt for the parts of mail -handling that are easier with a mouse and a real HTML renderer. +A Qt6 desktop mail client for reading, organizing and writing mail in a +local, notmuch-indexed Maildir. A GUI counterpart to neomutt for the parts of +mail handling that are easier with a mouse and a real HTML renderer. ## What it does not do @@ -18,8 +18,10 @@ script means joining the `flock` that already serializes it against cron; a built-in implementation would sit outside that lock and could run two `mbsync` processes over one Maildir, which corrupts UID state. -Sending is not implemented. Compose, reply, forward and send are planned for -v2 and need a companion send script that does not exist yet. +Sending follows the same rule. The application builds the message and hands +it to a command you configure, on stdin; it speaks no SMTP itself. Any sendmail +compatible program does (msmtp, ssmtp, the real sendmail), which keeps the +credentials in that program's own store rather than in this one's config. There is also no dry-run and no "are you sure?" on destructive actions. The answer for a human at a GUI is undo, which is implemented, and which is @@ -225,9 +227,21 @@ identity. [account.work] name = Your Name address = you@example.org -maildir = work-mail ; relative to notmuch's database.path -drafts = Drafts ; recorded for v2; unused today -sent = Sent ; optional; enables the Sent button for this account +maildir = work-mail ; relative to notmuch's mail root +trash = Trash ; Delete moves the file here. Not optional in + ; practice: without it the application reports a + ; config problem and Delete does not work. +drafts = Drafts ; optional; where the composer autosaves +sent = Sent ; optional; enables the Sent button, and where a + ; sent copy is filed +inbox = Inbox ; optional; where Restore puts a message whose + ; origin is unknown. Defaults to "Inbox" + +; Optional, and its absence is what makes an account receive-only: with no +; send_command the compose actions are disabled for it. The message is written +; to the command's stdin. Run WITHOUT a shell, so no pipes or redirections; +; give an absolute path and plain arguments. +send_command = /usr/bin/msmtp -a work -t label = W ; optional chip text; defaults to the key color = #2f6fa8 ; optional chip colour; generated when unset channel = work ; optional mbsync channel; defaults to the key @@ -236,8 +250,38 @@ channel = work ; optional mbsync channel; defaults to the key name = Your Name address = you@example.net maildir = personal +trash = Trash drafts = Drafts +; Optional, and global rather than per-account. Every key below shows its +; default, so an omitted [compose] section behaves exactly like this one. +[compose] +; Send an HTML part alongside the plain text one. The composer's own checkbox +; overrides this per message. It seeds New and Forward only: a Reply follows +; whether the message being answered carried an HTML part, which is a fact +; about the sender's software rather than a guess about their taste. +send_html = true + +; Where the quoted original goes in a reply: above or below. +quote_position = above + +; How long the send popup counts down before the command runs, in +; milliseconds. This is the window in which Undo can still stop it; 0 skips +; the countdown and sends at once. +send_delay_ms = 5000 + +; How often an open composer autosaves its draft, in milliseconds. Values +; below 1000 are raised to it. +autosave_interval_ms = 30000 + +; Preferred account for a new message while the dropdown is on All accounts. +; Ignored when it names an account that cannot send. +; default_account = work + +; Warn before attaching a file larger than this, in bytes. 25 MiB by default, +; which is the limit most providers enforce. +attachment_warn_bytes = 26214400 + [tagcolors] ; Optional. Colours resolve by exact tag first, then by top-level prefix, so ; one entry covers a whole hierarchy. @@ -441,6 +485,34 @@ an attachment, so it is visible without opening the thread. It comes from the `attachment` tag notmuch applies while indexing, not from parsing the message, and costs no extra query. +## Composing + +**Ctrl+N** opens a composer; Reply, Reply all, Reply without quoting and +Forward start one from the selected message. Each is a window in its own +right, so several can be open at once and the main window stays usable behind +them. + +The body is **markdown**. It is sent as plain text, and the markdown is what +you typed rather than a rendering of it, so a recipient reading plain text +sees exactly the source. Tick "Also send a formatted copy" and an HTML part is +rendered from that same source and sent alongside it, in a +`multipart/alternative`; `[compose] send_html` sets the default. + +Drafts autosave to the account's `drafts` folder while you type, as ordinary +Maildir files, so mbsync carries them to the server like any other message and +another client can pick one up. Closing a composer with unsaved edits asks +first, and so does quitting with one open. + +Sending goes through the account's `send_command`, which receives the finished +message on stdin. An account without one is receive-only, and the composer +says so rather than failing at the end. A copy of what was sent is filed in +the account's `sent` folder. + +**Send waits.** A popup counts down before the command runs, and Undo during +that window stops it and returns you to the composer with everything intact. +Nothing has reached the network until the countdown ends; +`[compose] send_delay_ms` sets how long it lasts, and 0 removes it. + ## Tagging Archive, delete, spam, mark-important and toggle-unread write fixed tags. For anything 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 61b73c5..e008641 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 @@ -188,8 +188,8 @@ taking that too literally. | 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 | -| 122 | The README documents a version of the app that no longer exists | documentation | M | open, 2026-08-20, from the notes. Delete-to-trash is entirely undocumented, including a config key a user must now set | -| 123 | Sending mail is not designed | v2 | L | **specified** 2026-08-20, on branch `compose-and-send`. Design in `docs/superpowers/specs/2026-08-20-compose-and-send-design.md`; read that, not this row. Send is a per-account `send_command` on stdin, so the no-network-protocol rule stands. Composer is a separate window, body is markdown via cmark-gfm, drafts autosave to the account's drafts folder. Tasks 1 to 12 of 13 built 2026-08-20 to 2026-08-22; task 13, the close-out, is the remainder. **Never hand tested**: nothing had wired a composer to an action until task 12, so no composer has yet been opened by a human. Twenty-two defects were found in the plan's own draft code across tasks 4 to 12, so treat every code block in it as a draft | +| 122 | The README documents a version of the app that no longer exists | documentation | M | **done** 2026-08-23, unreleased, inside item 123 task 13. `trash`, `send_command` and the whole `[compose]` section were undocumented; a Composing section is added and "sending is not implemented" removed. Every default was read from `config.h` rather than from the prose, which caught `send_html` documented as false when it defaults to true | +| 123 | Sending mail is not designed | v2 | L | **specified** 2026-08-20, on branch `compose-and-send`. Design in `docs/superpowers/specs/2026-08-20-compose-and-send-design.md`; read that, not this row. Send is a per-account `send_command` on stdin, so the no-network-protocol rule stands. Composer is a separate window, body is markdown via cmark-gfm, drafts autosave to the account's drafts folder. Tasks 1 to 13 of 13 built 2026-08-20 to 2026-08-23; task 13 closed the documentation out and retired the v1/v2 split, which semver had made meaningless. **Hand tested 2026-08-22 and 2026-08-23** against a fake send command: New, Reply and Forward all send, a forwarded attachment survives intact, and the sent copy is filed. Found two defects, both fixed (the orphaned composer, and sent mail carrying `inbox`). Twenty-two defects were found in the plan's own draft code across tasks 4 to 12, so treat every code block in it as a draft | | 124 | The worker reads the index directory as the mail root | defect | S | **done** 2026-08-20, unreleased. `mailRootOf()` over `NOTMUCH_CONFIG_MAIL_ROOT`, correct under both layouts. Verified by migrating the developer's own index to NVMe the same day: cold start 38.6 s to 0.67 s | @@ -217,6 +217,7 @@ taking that too literally. | 145 | Cc and Bcc are permanent rows on every composer | presentation | S | open, 2026-08-23, from the notes. Verified: both are unconditional `form->addRow` calls. Most messages use neither. Collapse behind a disclosure next to To:, expanded automatically when a draft or a reply already carries a value | | 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 | +| 148 | Ctrl+W does not close the composer | discoverability | XS | open, 2026-08-23, from the notes. Verified: nothing binds `Ctrl+W` anywhere, and the composer has no close action of its own. Belongs with item 21's table rather than bound in isolation | Sizes are rough: XS under an hour, S a sitting, M a session. -- cgit v1.2.3