From 84e3205ddcba3263e6c07fa314437318309d4b76 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 21 Aug 2026 20:12:30 +0200 Subject: feat(compose): register the six compose actions, item 123 Handlers are empty for now; this commit is the registration, so the three coverage tests guard every later task rather than being satisfied at the end. Two corrections to the spec, both found in the code rather than assumed. It calls for a new top-level Message menu and one already exists, so these join it; two menus named Message would be a defect. And it says every action needs a binding, which item 132 changed while this was being planned: save_message ships with no chord, since it is the rarely-used escape hatch and menu reachability is now the rule that must hold. reply_no_quote shares reply's icon and is added to the no-duplicate-icons exception list for the same reason the five thread actions are: it never reaches the toolbar, and a menu entry always carries its text. That list is renamed menuOnlySharedIconActions, after the property that earns the exemption rather than the tier that first needed it. Bindings are provisional. The user intends to rework them, and Ctrl+Alt+R for reply_no_quote is an imperfect fit since that tier elsewhere means a wider scope rather than a variant. The six labels went through a mnemonic pass that nothing enforced before. Four of them collided inside the Message menu on first writing, and the whole class was invisible to a green suite: Qt does not error on a duplicate mnemonic, it cycles the highlight instead of activating, so the key simply stops working. Item 57 had already decided this rule by rejecting a label that would have collided, but it lived in prose and in one test's comment, which is precisely why it was broken again here. noMenuHasTwoEntriesSharingAMnemonic() enforces it now, scoped per menu since a mnemonic resolves among the open menu's entries, and keyed on QKeySequence::mnemonic() rather than on parsing & by hand, because && is a literal ampersand and only Qt answers which key it will dispatch. Three pre-existing collisions are a named freeze list rather than a silent fix or a narrowed test: Alt+R three ways and Alt+S twice in Message, Alt+O in View. Renaming entries a user has had in their fingers since 0.1.0 belongs to the shortcuts rework, and the freeze is written as exact groups so a new entry joining any of them still fails. Two of the test's own design choices came from mutation checks that failed for the right reason while reporting the wrong thing. Reporting collisions as pairs was order-dependent, so a new colliding entry re-keyed a frozen pair and the fresh defect read as "a frozen collision no longer happens"; matching frozen entries by whole string broke the same way, since a growing group stopped matching its frozen text. It reports whole groups and matches on menu plus key. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FXF741wz4SY7j5dqvAxMU5 --- tests/test_mainwindow.cpp | 195 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 191 insertions(+), 4 deletions(-) (limited to 'tests/test_mainwindow.cpp') diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 4d70a29..0ad45ee 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -331,6 +331,7 @@ private slots: void everyActionCarriesAnIcon(); void everyActionIsReachableFromAMenu(); + void noMenuHasTwoEntriesSharingAMnemonic(); void theToolbarDoesNotOverrideTheDesktopButtonStyle(); void theImportantActionIsLabelledImportant(); void theImportantActionStillWritesTheFlaggedTag(); @@ -6444,6 +6445,185 @@ void TestMainWindow::everyActionIsReachableFromAMenu() .arg(unreachable.join(QStringLiteral(", "))))); } +void TestMainWindow::noMenuHasTwoEntriesSharingAMnemonic() +{ + // The sibling of everyActionIsReachableFromAMenu(), and it exists because + // the rule it enforces had lived only in prose and in one other test's + // COMMENT, and was duly broken the first time a batch of entries was added + // to a menu (item 123: `&Reply` against the pre-existing `&Restore from + // trash`, both Alt+R). + // + // Qt does not error on a duplicate mnemonic. It CYCLES between the + // colliding entries instead of activating either, so the key silently + // stops working and merely moves a highlight. That is worse than it + // sounds in the Message menu, where `restore` is deliberately greyed + // outside the trash view: the ordinary case was pressing Alt+R and landing + // on a disabled entry. + // + // Item 57 already decided this is a property rather than a taste. It + // rejected the label "Starred" for `flag` precisely because it would have + // collided with `Mark &spam`, and theImportantActionIsLabelledImportant() + // pins the surviving label with that reasoning in its comment. A decision + // recorded only in prose is one nobody re-derives. + // + // Scoped PER MENU, which is what the collision actually is: a mnemonic is + // resolved among the entries of the menu that is open, so the same letter + // in File and in View is not a conflict. + const Config config; + MainWindow window(config); + + auto *bar = window.menuBar(); + QVERIFY(bar); + + // The menu bar's own top-level titles are one such scope too, so the walk + // starts by treating the bar as a menu and then descends. + QList>> scopes; + scopes.append({ QStringLiteral("the menu bar"), bar->actions() }); + + QList pending; + const auto topLevel = bar->actions(); + for (QAction *action : topLevel) { + if (action->menu()) + pending.append(action->menu()); + } + QVERIFY2(!pending.isEmpty(), "the menu bar holds no menus"); + + while (!pending.isEmpty()) { + QMenu *menu = pending.takeFirst(); + const auto entries = menu->actions(); + scopes.append({ menu->title(), entries }); + for (QAction *entry : entries) { + if (QMenu *sub = entry->menu()) + pending.append(sub); + } + } + + // The four collisions that PREDATE this test, measured by running it + // against the tree before item 123 touched any label. They are listed + // rather than fixed, and rather than being hidden by narrowing the test, + // because renaming a shipped menu entry is the user's call and not a + // test's: three of them are in menus a user has had in their fingers + // since 0.1.0. + // + // Listed as exact pairs, not as "ignore Alt+R", so this is a freeze and + // not an amnesty: a NEW entry colliding on any of these same keys still + // fails, because its pair is not on this list. Fixing one is then a + // one-line deletion here, which is the point of writing them out. + // Written as the FULL GROUP of labels sharing one key in one menu, not as + // a pair. A pair is keyed on which entry the walk happened to see first, + // so adding a colliding entry ABOVE a frozen one silently re-pairs it and + // the new defect gets reported as "a frozen collision no longer happens", + // which names the wrong thing entirely. Measured: reinstating `&Reply` + // did exactly that before this was changed. A group is order-independent, + // so a new entry grows the group and fails as a new collision. + static const QStringList knownPreExistingCollisions = { + QStringLiteral("&Message: Alt+R shared by \"&Restore from trash\", \"Mark all &read\", \"Tagging &rules...\""), + QStringLiteral("&Message: Alt+S shared by \"Mark &spam\", \"Find &stranded deleted mail\""), + QStringLiteral("&View: Alt+O shared by \"&Open thread\", \"Zoom &out\""), + }; + + QStringList collisions; + int compared = 0; + + for (const auto &scope : scopes) { + // Keyed on the mnemonic Qt itself derives, not on a hand-parsed '&'. + // The question is which key Qt will dispatch, and only Qt answers it: + // "&&" is a literal ampersand and carries no mnemonic at all. + // + // A QMap rather than a QHash so the groups come out in a stable key + // order, which is what lets the frozen list above be written once and + // stay matching. + QMap byMnemonic; + for (QAction *entry : scope.second) { + if (entry->isSeparator()) + continue; + const QKeySequence mnemonic = QKeySequence::mnemonic(entry->text()); + if (mnemonic.isEmpty()) + continue; + ++compared; + byMnemonic[mnemonic.toString(QKeySequence::NativeText)] + .append(QStringLiteral("\"%1\"").arg(entry->text())); + } + + for (auto it = byMnemonic.cbegin(); it != byMnemonic.cend(); ++it) { + if (it.value().size() < 2) + continue; + // Names the menu, the key and EVERY label in the group, so a + // future failure says what to rename without anyone going looking. + collisions.append(QStringLiteral("%1: %2 shared by %3") + .arg(scope.first, it.key(), + it.value().join(QStringLiteral(", ")))); + } + } + + // The guard, and it is not ceremonial: every assertion below is a loop + // that reports success when it runs zero times. A walk that found no + // mnemonics at all would pass this test against any label whatsoever. + QVERIFY2(compared > 20, + qPrintable(QStringLiteral("only %1 menu entries carried a " + "mnemonic, so this probe measured " + "almost nothing") + .arg(compared))); + + // Matched on the menu and key only, with the labels compared separately + // below. Comparing whole strings made a GROWING group read as a frozen one + // disappearing: adding `&Reply` took Alt+R from three labels to four, the + // frozen three-label string stopped matching, and the failure said "this + // collision no longer happens" about the very key that had just got worse. + // Measured twice, once per attempt, which is why the two questions are + // asked separately. + const auto scopeAndKey = [](const QString &collision) { + return collision.left(collision.indexOf(QStringLiteral(" shared by "))); + }; + + QHash frozen; + for (const QString &known : knownPreExistingCollisions) + frozen.insert(scopeAndKey(known), known); + + QStringList unexpected; + QSet stillPresent; + for (const QString &collision : collisions) { + const QString key = scopeAndKey(collision); + const auto known = frozen.constFind(key); + if (known == frozen.constEnd()) { + // A collision on a key nothing froze: entirely new. + unexpected.append(collision); + continue; + } + stillPresent.insert(key); + if (*known != collision) { + // The key was already colliding, but the CAST has changed, which + // for a frozen entry means an entry joined it. Reported as the + // new collision it is, naming both what was frozen and what is + // there now. + unexpected.append( + QStringLiteral("%1 (frozen as [%2], now [%3])") + .arg(key, *known, collision)); + } + } + + // A frozen entry that has since been FIXED must not stay on the list + // silently, or the list becomes a place stale claims accumulate. + QStringList stale; + for (const QString &known : knownPreExistingCollisions) { + if (!stillPresent.contains(scopeAndKey(known))) + stale.append(known); + } + QVERIFY2(stale.isEmpty(), + qPrintable(QStringLiteral("%1 frozen collision(s) no longer " + "happen, so delete them from " + "knownPreExistingCollisions: %2") + .arg(stale.size()) + .arg(stale.join(QStringLiteral("; "))))); + + QVERIFY2(unexpected.isEmpty(), + qPrintable(QStringLiteral("%1 menu mnemonic collision(s), where " + "Qt cycles the highlight instead of " + "activating: %2") + .arg(unexpected.size()) + .arg(unexpected.join(QStringLiteral("; "))))); +} + void TestMainWindow::everyActionCarriesAnIcon() { // Item 56. The complaint was inconsistency, not absence: eight actions had @@ -6967,15 +7147,22 @@ void TestMainWindow::noTwoActionsShareAnIcon() // the words saying which. Giving them five invented shapes would be less // clear than the pairing. // + // reply_no_quote joined them in item 123 for exactly the same reason: it + // shares reply's icon, it is a menu entry that always carries its text, + // and it is not on the toolbar. The list is therefore no longer only the + // thread tier, which is why it is named for the PROPERTY that earns the + // exemption rather than for the tier that first needed it. + // // Named as an exception list rather than by asking the toolbar what it // holds, so that PUTTING one of these on the toolbar fails this test // rather than silently passing it. - static const QStringList menuOnlyThreadActions = { + static const QStringList menuOnlySharedIconActions = { QStringLiteral("archive_thread"), QStringLiteral("delete_thread"), QStringLiteral("spam_thread"), QStringLiteral("toggle_unread_thread"), QStringLiteral("flag_thread"), + QStringLiteral("reply_no_quote"), }; const Config config; @@ -6986,7 +7173,7 @@ void TestMainWindow::noTwoActionsShareAnIcon() // may sit on the toolbar. auto *toolBar = window.findChild(); QVERIFY(toolBar); - for (const QString &name : menuOnlyThreadActions) { + for (const QString &name : menuOnlySharedIconActions) { auto *action = window.findChild(name); QVERIFY2(action, qPrintable(QStringLiteral("no action named %1").arg(name))); QVERIFY2(!toolBar->actions().contains(action), @@ -7006,7 +7193,7 @@ void TestMainWindow::noTwoActionsShareAnIcon() QVERIFY2(action, qPrintable(QStringLiteral("no action named %1").arg(name))); if (!action->icon().isNull()) ++withIcons; - if (menuOnlyThreadActions.contains(name)) + if (menuOnlySharedIconActions.contains(name)) continue; if (action->icon().isNull()) continue; @@ -7033,7 +7220,7 @@ void TestMainWindow::noTwoActionsShareAnIcon() // And the exception list did not swallow the comparison itself. QCOMPARE(compared, KeyMap::knownActions().size() - - menuOnlyThreadActions.size()); + - menuOnlySharedIconActions.size()); QVERIFY2(collisions.isEmpty(), qPrintable(QStringLiteral("actions sharing one icon: %1") -- cgit v1.2.3 From 141bc9b98213b9c4ba9f3bcba041c3e3108c12a3 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 22 Aug 2026 10:32:44 +0200 Subject: feat(compose): the composer window, item 123 A separate top-level QMainWindow, one per draft, several open at once. A modal dialog cannot consult another message while writing, which is most of what replying is, and taking over the message pane fights the pane that exists to show what is being replied to. No geometry save and no restore, deliberately. Under a tiling compositor saveGeometry stores normalGeometry while the compositor owns the tile, so the restore is correct and looks broken; a whole session went into that once. Autosave is a debounce AND a dirty check: an unchanged message writes no file and provokes no sync. The check is on a fingerprint of the OutgoingMessage, NOT on the built bytes as the plan drafted. GMime is given a fresh Date and Message-ID on every build, so two builds of an unchanged message never compare equal; a check on the bytes would have read as working while writing a file, and an mbsync upload, on every debounce. Checking before the build also skips the blocking build for the no-change case, which is the common one. closeEvent writes the draft when the buffer is dirty. Without it the debounce is a hole rather than a delay: typing a paragraph and pressing the window manager's X inside the interval loses it silently, since WA_DeleteOnClose destroys the window immediately afterwards. A failed save there does NOT refuse the close, because a window that will not close because it cannot save is worse than one that closes having raised the banner, which is what the quit path reads. One flag covers a send, countdown included. An earlier revision had two, and the narrower "committed and running" one reads as the honest thing to guard a live SMTP conversation with. It is not: a close during the countdown destroys the parented SendDialog, committed() never fires, and the user pressed Send, watched a countdown, and believes the mail went. The narrow flag was also written in three places and read in none. A failed draft write raises a persistent banner rather than a modal or a fading status line. A modal mid-sentence is hostile while the user is typing, but the warning must survive until it is dealt with, because the quit path escalates exactly this state to a dialog on the way out. An account with no drafts folder reports success rather than failure: nothing was written and nothing failed, and a false there would make the quit path offer a retry no retry can change. A failed send saves the draft before reporting. send() builds from the widgets without saving, so the revision on disk is whatever the last debounce wrote: edit, send, fail, close, and the user gets the older text back, having watched their correction be sent. A failed sent copy after a successful send is a modal, and never a send failure: the message went, and reporting otherwise makes someone send it twice. It is the one failure here that silently diverges what the recipient received from what the local archive shows, and nobody discovers a missing sent copy by noticing a line that appeared for a few seconds. The formatting toolbar applies its edits through a QTextCursor document replacement inside one edit block, NOT setPlainText as the plan drafted. Measured against a real widget: setPlainText destroys the document's undo stack and resets the cursor to 0, so every toolbar press would throw away everything the user could undo. The cursor route leaves undo available, collapses to a single undo step, and emits textChanged once. The seeded quote is cleared off the undo stack afterwards, since it is not an edit the user made and one Ctrl+Z on a fresh composer must not wipe it. The per-send connect carries Qt::SingleShotConnection. MessageSender is a long-lived member, so a bare connect accumulates a permanent receiver per send and the second result runs both lambdas, the first still holding the first message's bytes: it files a sent copy of the wrong message and acts on a dialog it already destroyed. Covered by a test that sends, fails, corrects and sends again; without the flag it segfaults in QLabel::setText on the destroyed dialog. Its companion disconnect takes the specific connection handle rather than every finished receiver on this object, so a later observer cannot be killed silently. The attachment warning states sizes with a decimal and a stepped unit. Integer MB division read as "'x' is 0 MB. Many mail servers refuse messages above about 0 MB." for any attachment_warn_bytes below a megabyte, in both halves of one sentence. The autosave timer is created before buildUi(), which is load-bearing: buildUi connects every field to markDirty and seeding then fills those fields, so markDirty runs during construction. Created afterwards it is a null dereference on the first seeded field, which is every composer. Twenty-six cases in test_mainwindow, each mutation-checked. Co-Authored-By: Claude Opus 5 --- src/CMakeLists.txt | 1 + src/composewindow.cpp | 851 +++++++++++++++++++++++++++++ src/composewindow.h | 223 ++++++++ tests/test_mainwindow.cpp | 1154 +++++++++++++++++++++++++++++++++++++++ translations/qtmaildir_it_IT.ts | 143 +++++ 5 files changed, 2372 insertions(+) create mode 100644 src/composewindow.cpp create mode 100644 src/composewindow.h (limited to 'tests/test_mainwindow.cpp') diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 83981b2..2cebfef 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -20,6 +20,7 @@ add_library(qtmaildir_lib STATIC tagchip.cpp tagcolors.cpp senddialog.cpp + composewindow.cpp savequerydialog.cpp tagdialog.cpp tagrules.cpp diff --git a/src/composewindow.cpp b/src/composewindow.cpp new file mode 100644 index 0000000..95b0a7b --- /dev/null +++ b/src/composewindow.cpp @@ -0,0 +1,851 @@ +/* + * 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 "composewindow.h" + +#include "draftstore.h" +#include "messagebuilder.h" +#include "messagesender.h" +#include "senddialog.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +/// Splits a comma-separated recipient field into addresses. +/// +/// Splitting on commas is WRONG for a raw header, which is why +/// ComposeContextBuilder::parseAddressHeader parses instead. It is right here +/// and only here: this is a field the user typed, and the composer's own +/// rendering of it joins with ", ". A display name containing a comma has to +/// be quoted by the user, exactly as it has to be in the wire format, and +/// MessageBuilder is what turns each entry into a mailbox. +QStringList splitRecipients(const QString &text) +{ + QStringList out; + const QStringList parts = text.split(QLatin1Char(','), Qt::SkipEmptyParts); + for (const QString &part : parts) { + const QString trimmed = part.trimmed(); + if (!trimmed.isEmpty()) + out.append(trimmed); + } + return out; +} + +/// Everything about a message the user can change, as one comparable string. +/// +/// Joined with a character no field can contain, because concatenating them +/// bare lets a change move a boundary without changing the whole: a subject +/// "ab" with body "c" and a subject "a" with body "bc" would produce the same +/// string and the second edit would never be saved. A unit separator (U+001F) +/// cannot be typed into a QLineEdit or a QPlainTextEdit and cannot appear in a +/// file path. +QString fingerprintOf(const OutgoingMessage &message) +{ + const QChar sep(QChar(0x1F)); + return message.accountKey + sep + message.to.join(sep) + sep + + message.cc.join(sep) + sep + message.bcc.join(sep) + sep + + message.subject + sep + message.markdownBody + sep + + (message.sendHtml ? QStringLiteral("1") : QStringLiteral("0")) + sep + + message.attachments.join(sep); +} + +} // namespace + +ComposeWindow::ComposeWindow(const ComposeContext &context, + const Config &config, const QString &mailRoot, + QWidget *parent) + : QMainWindow(parent) + , m_context(context) + , m_config(config) + , m_mailRoot(mailRoot) + , m_attachments(context.attachments) +{ + // A window in its own right, not a child dialog: it must appear in the + // task switcher and be reachable while the main window is used. Passing a + // parent still makes Qt treat it as a window because of Qt::Window, which + // QMainWindow carries. + setAttribute(Qt::WA_DeleteOnClose); + setWindowTitle(tr("Compose")); + + // A sensible default. NOT restored and NOT saved; see the header. + resize(760, 640); + + // BEFORE buildUi(), and this ordering is load-bearing rather than + // stylistic. buildUi() connects every field to markDirty(), and seeding + // then fills those fields, so markDirty() runs during construction and + // calls m_autosaveTimer->start(). Created afterwards, that is a null + // dereference on the first seeded field, which is every composer. + m_autosaveTimer = new QTimer(this); + m_autosaveTimer->setObjectName(QStringLiteral("autosave")); + m_autosaveTimer->setSingleShot(true); + m_autosaveTimer->setInterval(m_config.compose().autosaveIntervalMs); + connect(m_autosaveTimer, &QTimer::timeout, this, &ComposeWindow::autosave); + + m_sender = new MessageSender(this); + + buildUi(); + buildFormatToolbar(); + seedFields(); + seedBody(); + refreshAttachmentList(); + + // Seeding is not an edit. Every field was just filled from the context, so + // the widgets have emitted their change signals and left the window dirty + // before the user has typed anything; a composer opened and closed at once + // would then write a draft nobody asked for. The timer is stopped as well + // as the flag cleared, since markDirty() started it. + m_dirty = false; + m_autosaveTimer->stop(); +} + +Account ComposeWindow::currentAccount() const +{ + // The dropdown is the authority once the window is open: the context + // chooses the initial account and the user may then change it, and every + // build after that must use what the From field shows. Reading + // m_context.accountKey here instead would send from the seeded account + // however the dropdown was set, with the interface saying otherwise. + if (m_from && m_from->currentIndex() >= 0) { + const QString key = m_from->currentData().toString(); + if (!key.isEmpty()) + return m_config.account(key); + } + return m_config.account(m_context.accountKey); +} + +void ComposeWindow::buildUi() +{ + auto *central = new QWidget(this); + central->setObjectName(QStringLiteral("composeCentral")); + auto *layout = new QVBoxLayout(central); + + // The failed-save banner, above everything: a warning that must survive + // until it is dealt with does not belong below the fold. Hidden until + // there is something to say. + m_banner = new QLabel(central); + m_banner->setObjectName(QStringLiteral("draftBanner")); + m_banner->setWordWrap(true); + // PlainText explicitly. The text carries a filesystem error string and a + // path, neither of which is ours, and a QLabel guesses under AutoText. + m_banner->setTextFormat(Qt::PlainText); + m_banner->hide(); + layout->addWidget(m_banner); + + auto *form = new QFormLayout; + + m_from = new QComboBox(central); + m_from->setObjectName(QStringLiteral("from")); + form->addRow(tr("From:"), m_from); + + m_to = new QLineEdit(central); + m_to->setObjectName(QStringLiteral("to")); + form->addRow(tr("To:"), m_to); + + m_cc = new QLineEdit(central); + m_cc->setObjectName(QStringLiteral("cc")); + form->addRow(tr("Cc:"), m_cc); + + m_bcc = new QLineEdit(central); + m_bcc->setObjectName(QStringLiteral("bcc")); + form->addRow(tr("Bcc:"), m_bcc); + + m_subject = new QLineEdit(central); + m_subject->setObjectName(QStringLiteral("subject")); + form->addRow(tr("Subject:"), m_subject); + + layout->addLayout(form); + + // Labelled for what it does, a formatted copy riding along with the plain + // text, rather than "HTML", which reads as an either/or that it is not. + m_sendHtml = new QCheckBox(tr("Also send a formatted copy"), central); + m_sendHtml->setObjectName(QStringLiteral("sendHtml")); + m_sendHtml->setToolTip( + tr("Sends the message as plain text with a formatted version " + "alongside it. The plain text is what you typed.")); + layout->addWidget(m_sendHtml); + + m_body = new QPlainTextEdit(central); + m_body->setObjectName(QStringLiteral("body")); + layout->addWidget(m_body, 1); + + m_attachmentList = new QListWidget(central); + m_attachmentList->setObjectName(QStringLiteral("attachments")); + m_attachmentList->setMaximumHeight(90); + m_attachmentList->hide(); + layout->addWidget(m_attachmentList); + + // The send-failure pane, in the shape MainWindow's sync log already has: + // a header with a Close button and a read-only QPlainTextEdit under it. A + // QPlainTextEdit has no close affordance of its own, so the two travel + // together as one widget. + m_sendLogPane = new QWidget(central); + m_sendLogPane->setObjectName(QStringLiteral("sendLogPane")); + auto *logLayout = new QVBoxLayout(m_sendLogPane); + logLayout->setContentsMargins(0, 0, 0, 0); + logLayout->setSpacing(2); + + auto *logHeader = new QHBoxLayout; + logHeader->addWidget(new QLabel(tr("Send output"), m_sendLogPane)); + logHeader->addStretch(); + auto *closeLog = new QPushButton(tr("Close"), m_sendLogPane); + closeLog->setObjectName(QStringLiteral("closeSendLog")); + connect(closeLog, &QPushButton::clicked, m_sendLogPane, &QWidget::hide); + logHeader->addWidget(closeLog); + logLayout->addLayout(logHeader); + + m_sendLog = new QPlainTextEdit(m_sendLogPane); + m_sendLog->setObjectName(QStringLiteral("sendLog")); + m_sendLog->setReadOnly(true); + m_sendLog->setMaximumHeight(140); + logLayout->addWidget(m_sendLog); + + m_sendLogPane->hide(); + layout->addWidget(m_sendLogPane); + + setCentralWidget(central); + + // Every field marks the buffer dirty. The subject and the recipients are + // part of the message as much as the body is, and a draft that saved the + // body but not the address it was going to would be worse than none. + connect(m_body, &QPlainTextEdit::textChanged, this, + &ComposeWindow::markDirty); + for (QLineEdit *field : { m_to, m_cc, m_bcc, m_subject }) + connect(field, &QLineEdit::textChanged, this, &ComposeWindow::markDirty); + connect(m_sendHtml, &QCheckBox::toggled, this, &ComposeWindow::markDirty); + connect(m_from, &QComboBox::currentIndexChanged, this, + &ComposeWindow::markDirty); +} + +void ComposeWindow::buildFormatToolbar() +{ + m_formatToolbar = addToolBar(tr("Formatting")); + m_formatToolbar->setObjectName(QStringLiteral("formatToolbar")); + + // A QAction parented to THIS WINDOW, not registered in KeyMap. Its + // shortcut is therefore scoped to the composer: Qt dispatches a + // WindowShortcut to the active window only, so the main window's Ctrl+B is + // untouched and the two namespaces stay apart. These six do not + // participate in item 132's reachability rule for the same reason. + const auto addFormat = [this](const QString &name, const QString &text, + const QString &token, + const QKeySequence &shortcut) { + QAction *action = m_formatToolbar->addAction(text); + action->setObjectName(name); + if (!shortcut.isEmpty()) + action->setShortcut(shortcut); + connect(action, &QAction::triggered, this, + [this, token]() { applyFormat(token); }); + }; + + addFormat(QStringLiteral("format_bold"), tr("Bold"), + QStringLiteral("**"), QKeySequence(QStringLiteral("Ctrl+B"))); + addFormat(QStringLiteral("format_italic"), tr("Italic"), + QStringLiteral("*"), QKeySequence(QStringLiteral("Ctrl+I"))); + addFormat(QStringLiteral("format_code"), tr("Code"), + QStringLiteral("`"), QKeySequence(QStringLiteral("Ctrl+`"))); + // No shortcut, per the spec's table. + addFormat(QStringLiteral("format_strike"), tr("Strikethrough"), + QStringLiteral("~~"), QKeySequence()); + + // Link and Quote are not wraps and cannot go through applyFormat(). + QAction *link = m_formatToolbar->addAction(tr("Link")); + link->setObjectName(QStringLiteral("format_link")); + link->setShortcut(QKeySequence(QStringLiteral("Ctrl+K"))); + connect(link, &QAction::triggered, this, [this]() { + const QTextCursor cursor = m_body->textCursor(); + applyEdit(MarkdownFormat::link(m_body->toPlainText(), + cursor.selectionStart(), + cursor.selectionEnd())); + }); + + QAction *quote = m_formatToolbar->addAction(tr("Quote")); + quote->setObjectName(QStringLiteral("format_quote")); + connect(quote, &QAction::triggered, this, [this]() { + const QTextCursor cursor = m_body->textCursor(); + applyEdit(MarkdownFormat::quote(m_body->toPlainText(), + cursor.selectionStart(), + cursor.selectionEnd())); + }); + + m_formatToolbar->addSeparator(); + + m_attachAction = m_formatToolbar->addAction(tr("Attach...")); + m_attachAction->setObjectName(QStringLiteral("compose_attach")); + connect(m_attachAction, &QAction::triggered, this, [this]() { + const QStringList chosen = QFileDialog::getOpenFileNames( + this, tr("Attach files")); + for (const QString &path : chosen) + attachFile(path); + }); + + m_detachAction = m_formatToolbar->addAction(tr("Remove attachment")); + m_detachAction->setObjectName(QStringLiteral("compose_detach")); + connect(m_detachAction, &QAction::triggered, this, [this]() { + const int row = m_attachmentList->currentRow(); + if (row < 0 || row >= m_attachments.size()) + return; + m_attachments.removeAt(row); + refreshAttachmentList(); + markDirty(); + }); + + m_sendAction = m_formatToolbar->addAction(tr("Send")); + m_sendAction->setObjectName(QStringLiteral("compose_send")); + m_sendAction->setShortcut(QKeySequence(QStringLiteral("Ctrl+Return"))); + connect(m_sendAction, &QAction::triggered, this, &ComposeWindow::send); +} + +void ComposeWindow::seedFields() +{ + // Only accounts that can send. An account without a send_command is + // receive-only by construction, and offering it in a From field would + // produce a message that cannot be sent from the account it says it is + // from. + const QList senders = m_config.sendingAccounts(); + for (const Account &account : senders) { + const QString label = account.name.isEmpty() + ? account.address + : account.name + QStringLiteral(" <") + + account.address + QLatin1Char('>'); + m_from->addItem(label, account.key); + } + const int index = m_from->findData(m_context.accountKey); + if (index >= 0) + m_from->setCurrentIndex(index); + + m_to->setText(m_context.to.join(QStringLiteral(", "))); + m_cc->setText(m_context.cc.join(QStringLiteral(", "))); + m_subject->setText(m_context.subject); + + // New and Forward seed from [compose] send_html; Reply and Reply-all seed + // from whether the original carried a text/html part, ignoring the config + // value. An HTML part in the original is a fact about the sender's + // software, not a guess about their taste. + const bool isReply = m_context.kind == ComposeContext::Kind::Reply + || m_context.kind == ComposeContext::Kind::ReplyAll; + m_sendHtml->setChecked(isReply ? m_context.seedHtml + : m_config.compose().sendHtml); +} + +void ComposeWindow::seedBody() +{ + if (m_context.quotedBody.isEmpty()) + return; + + // Applied when the window opens and never again. The buffer is text the + // user owns after that, and there is deliberately no live toggle: + // tracking "my text" and "the quote" as separate pieces to make a toggle + // reversible is machinery for a case answered by closing the composer and + // reopening it. + if (m_config.compose().quotePosition + == ComposeSettings::QuotePosition::Above) { + // The quote first, then a blank line for the reply to be typed into. + m_body->setPlainText(m_context.quotedBody + QStringLiteral("\n\n")); + } else { + m_body->setPlainText(QStringLiteral("\n\n") + m_context.quotedBody); + } + + // The cursor at the very top in both cases: with the quote below, the + // blank lines the reply goes into are at the top; with it above, the user + // scrolls past what they are answering, which is what quoting above means. + m_body->moveCursor(QTextCursor::Start); + + // The seeded quote is not an edit the user made, so it must not survive as + // an undo step: one Ctrl+Z on a fresh composer would otherwise wipe the + // quote and read as the buffer losing its content. + m_body->document()->clearUndoRedoStacks(); +} + +void ComposeWindow::refreshAttachmentList() +{ + m_attachmentList->clear(); + for (const QString &path : m_attachments) + m_attachmentList->addItem(QFileInfo(path).fileName()); + m_attachmentList->setVisible(!m_attachments.isEmpty()); +} + +bool ComposeWindow::attachmentNeedsWarning(qint64 size) const +{ + const qint64 limit = m_config.compose().attachmentWarnBytes; + // A limit of zero or less disables the warning outright. Treating it as a + // threshold would warn about every attachment including an empty one, + // which is the opposite of what turning a warning off means. + return limit > 0 && size > limit; +} + +/// A byte count as a figure a person reads, with one decimal below 10 units. +/// +/// Integer MB division is what this replaces and it produced "'x' is 0 MB. +/// Many mail servers refuse messages above about 0 MB.", which is what any +/// attachment_warn_bytes under a megabyte reads as. The unit steps down as +/// well, so a small configured limit is stated in KB rather than as zero of a +/// larger unit. +QString ComposeWindow::humanSize(qint64 bytes) +{ + constexpr qint64 kKb = 1024; + constexpr qint64 kMb = 1024 * 1024; + + if (bytes >= kMb) { + const double mb = double(bytes) / double(kMb); + // One decimal only while the figure is small enough for it to say + // something; 26.2 MB is informative, 1234.6 MB is noise. + return mb < 10.0 ? QObject::tr("%1 MB").arg(mb, 0, 'f', 1) + : QObject::tr("%1 MB").arg(qRound(mb)); + } + if (bytes >= kKb) { + const double kb = double(bytes) / double(kKb); + return kb < 10.0 ? QObject::tr("%1 KB").arg(kb, 0, 'f', 1) + : QObject::tr("%1 KB").arg(qRound(kb)); + } + return QObject::tr("%1 bytes").arg(bytes); +} + +void ComposeWindow::attachFile(const QString &path) +{ + const QFileInfo info(path); + + if (attachmentNeedsWarning(info.size())) { + const qint64 limit = m_config.compose().attachmentWarnBytes; + const auto answer = QMessageBox::question( + this, tr("Large attachment"), + tr("'%1' is %2. Many mail servers refuse messages above about " + "%3. Attach it anyway?") + .arg(info.fileName(), humanSize(info.size()), + humanSize(limit)), + QMessageBox::Yes | QMessageBox::No); + if (answer != QMessageBox::Yes) + return; + } + + m_attachments.append(path); + refreshAttachmentList(); + markDirty(); +} + +OutgoingMessage ComposeWindow::currentMessage() const +{ + OutgoingMessage message; + message.accountKey = currentAccount().key; + message.to = splitRecipients(m_to->text()); + message.cc = splitRecipients(m_cc->text()); + message.bcc = splitRecipients(m_bcc->text()); + message.subject = m_subject->text(); + message.markdownBody = m_body->toPlainText(); + message.sendHtml = m_sendHtml->isChecked(); + message.attachments = m_attachments; + message.inReplyTo = m_context.inReplyTo; + message.references = m_context.references; + return message; +} + +void ComposeWindow::applyEdit(const MarkdownFormat::Edit &edit) +{ + // A QTextCursor replacement rather than setPlainText(), and this is a + // correction of the plan's draft. Measured under the offscreen platform: + // setPlainText() DESTROYS the document's undo stack (isUndoAvailable goes + // from true to false) and resets the cursor to position 0, so every + // toolbar press would throw away everything the user could undo. A + // document-wide select and insertText inside one edit block leaves undo + // available, collapses to a SINGLE undo step, and emits textChanged once. + QTextCursor cursor = m_body->textCursor(); + cursor.beginEditBlock(); + cursor.select(QTextCursor::Document); + cursor.insertText(edit.text); + cursor.endEditBlock(); + + // Restore the selection the transformation asked for. The cursor is left + // at the end of the inserted text, so without this every button press + // sends it to the bottom of the message; the empty-selection case relies + // on it to land BETWEEN the tokens, which is the property a user notices + // immediately when it is wrong. + // + // Clamped rather than trusted: QTextCursor::setPosition() past the end + // warns on stderr and silently clamps, so a stale or arithmetic position + // would produce noise rather than an error. MarkdownFormat clamps its own + // output too, so this is a second line rather than the only one. + const int length = m_body->toPlainText().length(); + const int start = qBound(0, edit.selectionStart, length); + const int end = qBound(start, edit.selectionEnd, length); + + QTextCursor restored = m_body->textCursor(); + restored.setPosition(start); + restored.setPosition(end, QTextCursor::KeepAnchor); + m_body->setTextCursor(restored); + m_body->setFocus(); +} + +void ComposeWindow::applyFormat(const QString &token) +{ + const QTextCursor cursor = m_body->textCursor(); + applyEdit(MarkdownFormat::wrap(m_body->toPlainText(), + cursor.selectionStart(), + cursor.selectionEnd(), token)); +} + +void ComposeWindow::markDirty() +{ + m_dirty = true; + // Debounced: the timer restarts on every keystroke, so a write happens + // once the user has paused, not once per character. Every autosave + // produces a Maildir write that mbsync uploads, which is what the debounce + // and the dirty check together keep to a few revisions per message. + m_autosaveTimer->start(); +} + +void ComposeWindow::autosave() +{ + if (!m_dirty) + return; + saveDraftNow(); +} + +bool ComposeWindow::saveDraftNow() +{ + const Account account = currentAccount(); + if (account.drafts.isEmpty()) { + // Configured without a drafts folder. Warned about at startup; there + // is nothing to do here and nothing to report a second time. Reported + // as success because nothing failed: a false here would make the quit + // path offer a retry that cannot change anything. + return true; + } + + const OutgoingMessage message = currentMessage(); + + // The dirty CHECK, not just the flag: an unchanged message means no file + // is written and no sync is provoked. Every autosave produces a Maildir + // write that mbsync uploads, so this and the debounce together are what + // keep a message to a few revisions rather than dozens. + // + // Checked BEFORE the build, and on the message rather than on the bytes. + // The plan's draft compared built.bytes, which can never match: GMime is + // given a fresh Date and Message-ID on every build, so two builds of an + // unchanged message differ. That check would have read as working while + // writing a file on every debounce. Doing it first also skips the + // blocking build entirely for the no-change case, which is the common one. + const QString fingerprint = fingerprintOf(message); + if (!m_savedFingerprint.isEmpty() && fingerprint == m_savedFingerprint) { + m_dirty = false; + return true; + } + + // MessageBuilder::build() is SYNCHRONOUS and can block: a large attachment + // is read and base64-encoded on this thread, which is the GUI thread. A + // debounce firing with a 25MB attachment therefore stalls typing for as + // long as the read takes. Deliberately not moved to a thread: nothing here + // crosses the worker boundary, and a second threading model for one call + // is worse than the stall. If someone is measuring a composer freeze, this + // line is where to look. + const MessageBuilder::Result built = MessageBuilder::build(message, account); + if (!built.ok()) { + m_saveFailed = true; + m_banner->setText(tr("The draft could not be saved: %1").arg(built.error)); + m_banner->show(); + return false; + } + + const QString folder = QDir(m_mailRoot).absoluteFilePath( + account.maildir + QLatin1Char('/') + account.drafts); + + const DraftStore::Result written = + DraftStore::write(folder, built.bytes, QStringLiteral("D"), m_draftPath); + + if (!written.ok()) { + // A PERSISTENT banner, not a modal and not a status-bar line that + // fades. A modal mid-sentence is hostile while the user is typing, but + // the warning must survive until it is dealt with, because the quit + // path's honesty depends on it. + m_saveFailed = true; + m_banner->setText( + tr("The draft could not be saved: %1").arg(written.error)); + m_banner->show(); + return false; + } + + m_draftPath = written.path; + m_savedFingerprint = fingerprint; + m_dirty = false; + m_saveFailed = false; + m_banner->hide(); + return true; +} + +void ComposeWindow::setInputsEnabled(bool enabled) +{ + // Every input for the WHOLE operation, countdown included. The message + // must not change between the user pressing Send and the bytes being + // built. The send-failure pane is deliberately left alone: it is read-only + // and disabling it would make the stderr it carries unreadable. + m_to->setEnabled(enabled); + m_cc->setEnabled(enabled); + m_bcc->setEnabled(enabled); + m_subject->setEnabled(enabled); + m_from->setEnabled(enabled); + m_body->setReadOnly(!enabled); + m_sendHtml->setEnabled(enabled); + m_attachmentList->setEnabled(enabled); + m_formatToolbar->setEnabled(enabled); +} + +void ComposeWindow::showSendFailure(const QString &stderrText) +{ + m_sendLog->setPlainText(stderrText.isEmpty() + ? tr("The send command reported no output.") + : stderrText); + m_sendLogPane->show(); +} + +void ComposeWindow::send() +{ + // Refused outright while a send operation is up, countdown included. + // setInputsEnabled(false) disables the toolbar the Send action lives on + // and SendDialog is window-modal, so a user cannot reach this twice; the + // guard covers the programmatic route, where a second call would put a + // second dialog over the first and start a send MessageSender then + // refuses, leaving a popup with no result coming for it. + if (m_sendInFlight) + return; + m_sendInFlight = true; + + const Account account = currentAccount(); + + if (!account.canSend()) { + QMessageBox::warning( + this, tr("Cannot send"), + tr("The account '%1' has no send command configured.") + .arg(account.key)); + m_sendInFlight = false; + return; + } + + const MessageBuilder::Result built = + MessageBuilder::build(currentMessage(), account); + if (!built.ok()) { + // A missing attachment lands here, before anything runs. + QMessageBox::warning(this, tr("Cannot send"), built.error); + m_sendInFlight = false; + return; + } + + // Every input is disabled for the WHOLE operation, countdown included. + setInputsEnabled(false); + + auto *dialog = new SendDialog(m_config.compose().sendDelayMs, this); + + connect(dialog, &SendDialog::undone, this, [this, dialog]() { + // Nothing reached a server. The composer returns exactly as it was, + // editable, popup gone, nothing sent. + // + // deleteLater(), never delete: this runs SYNCHRONOUSLY inside + // SendDialog::undo(), which emits undone() and then calls reject() on + // itself (senddialog.cpp), so the dialog is still on the stack here. + // This is CLAUDE.md's "a modal dialog must close BEFORE the action it + // asked for runs" arriving from the other side, and deleteLater is + // what makes it safe: it posts a deletion event rather than freeing + // the object the caller is about to keep using. A plain delete here + // would return into a destroyed SendDialog's reject(). + m_sendInFlight = false; + setInputsEnabled(true); + dialog->deleteLater(); + }); + + connect(dialog, &SendDialog::committed, this, + [this, dialog, built, account]() { + // No setStage(Sending) here: SendDialog::commit() sets it before + // emitting committed(), so doing it again would be a second owner of + // the same state. + + // Qt::SingleShotConnection IS REQUIRED HERE. 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. + const QMetaObject::Connection resultConnection = connect( + m_sender, &MessageSender::finished, this, + [this, dialog, built, account](bool sent, const QString &error) { + m_sendInFlight = false; + + if (!sent) { + dialog->accept(); + dialog->deleteLater(); + setInputsEnabled(true); + + // The draft stays, and it must be the draft of what was just + // attempted. send() builds from the widgets without saving, so + // the revision on disk is whatever the last debounce wrote: + // edit, send, fail, close, and the user gets the OLDER text + // back, having watched their correction be sent. No retry + // loop, but the text that failed to go is kept. + saveDraftNow(); + + showSendFailure(error); + return; + } + + dialog->setStage(SendDialog::Stage::FilingSentCopy); + bool sentCopyFailed = false; + QString sentCopyError; + + if (!account.sent.isEmpty()) { + const QString folder = QDir(m_mailRoot).absoluteFilePath( + account.maildir + QLatin1Char('/') + account.sent); + const DraftStore::Result filed = + DraftStore::write(folder, built.bytes, QStringLiteral("S")); + if (!filed.ok()) { + sentCopyFailed = true; + sentCopyError = filed.error; + } + } + + dialog->setStage(SendDialog::Stage::RemovingDraft); + if (!m_draftPath.isEmpty()) { + QFile::remove(m_draftPath); + m_draftPath.clear(); + } + + dialog->accept(); + dialog->deleteLater(); + + if (sentCopyFailed) { + // A MODAL, never a status-bar line, and never reported as a + // send failure. The message went; reporting otherwise makes + // someone send it twice. This is the one failure in the whole + // design that produces a silent divergence between what the + // recipient received and what the local archive shows, and + // nobody discovers a missing sent copy by noticing a line that + // appeared for a few seconds. + QMessageBox::warning( + this, tr("Sent, but not filed"), + tr("The message was sent, but the copy could not be " + "written to '%1' for account '%2':\n\n%3\n\n" + "The message HAS been sent. Do not send it again.") + .arg(account.sent, account.key, sentCopyError)); + } + + // The composer closes either way: the message went, and holding a + // composer open for a message already sent invites sending it + // twice. m_finished stops closeEvent() saving a draft for a + // message that is gone, and stops it refusing the close. + m_finished = true; + m_dirty = false; + close(); + }, Qt::SingleShotConnection); + + if (!m_sender->send(account.sendCommand, built.bytes)) { + // Refused before any process started, so no finished() will ever + // arrive and the single-shot connection above would sit there for + // good. Disconnected here rather than left, since the next send + // would then have two receivers, which is exactly the defect the + // flag exists to prevent. + // + // THE HANDLE, not disconnect(m_sender, &finished, this, nullptr). + // That form drops every finished receiver on this object, so one + // connection added anywhere else would be killed here silently, + // and the failure it produces is not a wrong value but silence: a + // send whose result nobody processes, leaving the popup on + // "Sending...", the composer disabled, and no error anywhere. + // + // UNTESTED, and deliberately so rather than by omission. This + // branch is currently UNREACHABLE: MessageSender::send() returns + // false only for an empty command or a command already running, + // and canSend() rejects the first while m_sendInFlight rejects the + // second before either can arrive here. QSettings also unquotes + // every INI value, so no configured string survives trimming yet + // splits to nothing. A test would have to reach past the public + // surface to provoke it, and a test that cannot fail is worse than + // none. Kept because it costs nothing and stops being dead the + // moment send() grows a third refusal, which is the shape an + // outbox drain loop would add. + m_sendInFlight = false; + disconnect(resultConnection); + dialog->accept(); + dialog->deleteLater(); + setInputsEnabled(true); + showSendFailure(tr("The send command could not be started.")); + } + }); + + dialog->open(); +} + +void ComposeWindow::closeEvent(QCloseEvent *event) +{ + // Refused for the WHOLE send, countdown included, and the countdown half + // is the one easily lost. A guard that starts at commit leaves the five + // seconds before it unprotected: closing then destroys this window, takes + // the parented SendDialog down with it, and committed() never fires, so + // the user pressed Send, watched a countdown, and believes the mail went. + // After commit the reason is the one MessageSender's destructor + // documents: a live SMTP conversation abandoned is an outcome nobody can + // report truthfully. + // + // Both windows close themselves when the operation ends, so refusing here + // strands nothing. + if (m_sendInFlight && !m_finished) { + event->ignore(); + return; + } + + // The last-moment autosave, and the reason it is here rather than in the + // quit path: the debounce means a composer closed inside its interval has + // unwritten text, and WA_DeleteOnClose destroys the window immediately + // after this. Without this call, typing a paragraph and pressing the + // window manager's X inside thirty seconds loses it silently, with no + // prompt and no write, which is exactly the loss the autosave design + // exists to prevent. + // + // Its failure is deliberately NOT allowed to refuse the close. A window + // that will not close because it cannot save is worse than one that closes + // having said so: the banner is already up from saveDraftNow(), and the + // quit path reads lastSaveFailed() to escalate. Task 12 owns that dialog; + // this call is what makes there be something to escalate ABOUT. + if (m_dirty && !m_finished) + saveDraftNow(); + + emit closed(this); + QMainWindow::closeEvent(event); +} diff --git a/src/composewindow.h b/src/composewindow.h new file mode 100644 index 0000000..99803d7 --- /dev/null +++ b/src/composewindow.h @@ -0,0 +1,223 @@ +/* + * 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 "config.h" +#include "formattoolbar.h" // MarkdownFormat::Edit is used by value below, and + // a type nested in a namespace cannot be + // forward-declared from outside it. +#include "types.h" + +class QAction; +class QCheckBox; +class QComboBox; +class QLabel; +class QLineEdit; +class QListWidget; +class QPlainTextEdit; +class QTimer; +class QToolBar; +class QWidget; + +class MessageSender; + +/// One draft. A separate top-level window, several open at once. +/// +/// A QMainWindow rather than a dialog: a modal dialog cannot consult another +/// message while writing, which is most of what replying is, and taking over +/// the message pane fights the pane that exists to show what is being replied +/// to. +/// +/// NO GEOMETRY RESTORE and no geometry save. CLAUDE.md records what +/// saveGeometry does under a tiling compositor: it stores normalGeometry, the +/// compositor owns the tile, and the restore is correct while looking broken. +/// A whole session went into that once. The composer opens at a sensible +/// default size and the compositor places it. +/// +/// It contains no MIME and no process logic: a composer bug and a MIME bug are +/// found in different files. Everything it does with a message goes through +/// MessageBuilder, DraftStore, MessageSender, MarkdownFormat and SendDialog. +class ComposeWindow : public QMainWindow +{ + Q_OBJECT + +public: + /// \p mailRoot is the Maildir root, passed in rather than derived. + /// + /// There is NO Config::maildirPath(). The root comes from + /// notmuch_config_get(NOTMUCH_CONFIG_MAIL_ROOT), wrapped by mailRootOf() + /// which is file-static inside notmuchworker.cpp and needs the database + /// handle. Item 124 records why this matters: notmuch can split the index + /// from the mail, and under that layout database.path is the INDEX + /// directory. Composing a destination from the wrong root would write + /// drafts and sent copies into the Xapian tree. MainWindow already + /// receives the root from the worker; it passes it here. + ComposeWindow(const ComposeContext &context, const Config &config, + const QString &mailRoot, QWidget *parent = nullptr); + + /// 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; } + + /// True when the LAST autosave attempt failed. Escalated to its own + /// dialog on the way out, because saving is what is already not working + /// and quitting therefore loses that text. + bool lastSaveFailed() const { return m_saveFailed; } + + /// Writes the current buffer to the drafts folder now. Returns false and + /// leaves the banner up on failure. + /// + /// Returns TRUE when the account configures no drafts folder: nothing was + /// written and nothing failed, and reporting a failure would make the quit + /// path offer a retry for a state no retry can change. The composer + /// running without draft protection is warned about at startup instead. + bool saveDraftNow(); + + /// What the composer would send or save right now. + /// + /// Public so a test can assert on the message the widgets produce without + /// building MIME, and so the quit path can be reasoned about from values. + OutgoingMessage currentMessage() const; + + /// The paths currently attached, in the order they were attached. + QStringList attachments() const { return m_attachments; } + + /// Attaches \p path, asking first when it is larger than + /// [compose] attachment_warn_bytes. + /// + /// A warning rather than a refusal: the limit belongs to the recipient's + /// server, which this application cannot know, so the user decides. + void attachFile(const QString &path); + + /// A byte count as a figure a person reads. + /// + /// Static and public so the formatting is testable without a modal. The + /// integer MB division this replaces produced "0 MB" for any + /// attachment_warn_bytes under a megabyte, in both halves of the same + /// sentence. + static QString humanSize(qint64 bytes); + + /// Whether \p size would raise the large-attachment question. + /// + /// Split out so the threshold is testable without a modal. A limit of zero + /// or less disables the warning outright rather than warning about + /// everything. + bool attachmentNeedsWarning(qint64 size) const; + +signals: + /// The composer finished with its message, one way or another, and the + /// registry should forget it. + /// + /// Emitted from the close path, so a registry connected to it can drop its + /// pointer before WA_DeleteOnClose destroys the window. + void closed(ComposeWindow *window); + +protected: + /// The one place the registry is told, whichever route closes the window. + void closeEvent(QCloseEvent *event) override; + +private: + void buildUi(); + void buildFormatToolbar(); + void seedFields(); + void seedBody(); + void refreshAttachmentList(); + void setInputsEnabled(bool enabled); + void showSendFailure(const QString &stderrText); + void applyEdit(const MarkdownFormat::Edit &edit); + void markDirty(); + void autosave(); + void send(); + void applyFormat(const QString &token); + Account currentAccount() const; + + ComposeContext m_context; + Config m_config; + QString m_mailRoot; + QStringList m_attachments; + + QLineEdit *m_to = nullptr; + QLineEdit *m_cc = nullptr; + QLineEdit *m_bcc = nullptr; + QLineEdit *m_subject = nullptr; + QComboBox *m_from = nullptr; + QPlainTextEdit *m_body = nullptr; + QCheckBox *m_sendHtml = nullptr; + QLabel *m_banner = nullptr; + QListWidget *m_attachmentList = nullptr; + QWidget *m_sendLogPane = nullptr; + QPlainTextEdit *m_sendLog = nullptr; + QToolBar *m_formatToolbar = nullptr; + QAction *m_sendAction = nullptr; + QAction *m_attachAction = nullptr; + QAction *m_detachAction = nullptr; + + QTimer *m_autosaveTimer = nullptr; + MessageSender *m_sender = nullptr; + + QString m_draftPath; ///< The revision on disk, unlinked on the next write. + + /// A fingerprint of the message the last successful save wrote, for the + /// dirty CHECK. + /// + /// NOT the built bytes, and that is a correction of the plan's draft. + /// MessageBuilder generates a fresh Date and Message-ID on every build + /// (measured, messagebuilder.cpp around the g_mime_message_set_date call), + /// so two builds of an unchanged message never compare equal and a check + /// on the bytes can never fire. It would read as working while writing a + /// file, and an mbsync upload, on every debounce. + QString m_savedFingerprint; + bool m_dirty = false; + bool m_saveFailed = false; + + /// True from the moment Send is pressed until the operation ends, however + /// it ends: the countdown, the command, the sent copy. + /// + /// ONE flag, covering the whole operation, and an earlier revision had two + /// because a narrower "committed and running" flag reads as the honest + /// thing to guard a live SMTP conversation with. It is not: every question + /// this window has to answer while sending has the same answer through the + /// countdown as after it. A close during the countdown destroys the + /// parented SendDialog and committed() never fires, so the user watches a + /// countdown for a message that is never sent, and a second Send during + /// the countdown opens a second popup. Splitting the two left the narrower + /// flag written in three places and read in none. + bool m_sendInFlight = false; + + /// Set once the message has gone, so the close that follows a successful + /// send is neither refused nor made to write a draft. + /// + /// The close-REFUSAL half is load-bearing: m_sendInFlight is cleared in + /// the same handler, and without m_finished the composer's own close would + /// depend on that clear having already happened, which is a race rather + /// than a guarantee. + /// + /// The last-moment-SAVE half is deliberately redundant, and it is worth + /// saying so rather than letting the next reader mistake it for load + /// bearing: the send handler already clears m_dirty, so either condition + /// alone stops the save. Measured, each survives the other's removal and + /// only dropping both puts the draft of an already-sent message back on + /// disk. Kept because the two say different things, "nothing to write" and + /// "this window is done", and a future path that finishes without clearing + /// m_dirty would otherwise resurrect a sent message's draft silently. + bool m_finished = false; +}; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 0ad45ee..cab6eae 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -50,6 +50,13 @@ #include "messageview.h" #include "notmuchworker.h" #include "carddelegate.h" +#include "composewindow.h" +#include "senddialog.h" +#include "messagesender.h" +#include +#include +#include +#include #include "cardlayout.h" #include @@ -392,6 +399,37 @@ private slots: void theRefreshAfterARestoreLeavesUndoIntact(); void deletingOutsideTheTrashViewLeavesTheRowInPlace(); + // ComposeWindow, item 123. These need a window but no worker: the composer + // never touches NotmuchWorker, it reads its context from the value struct + // MainWindow hands it, so a Config written to a temporary INI is the whole + // fixture. + void aComposerOpensClean(); + void typingMarksTheComposerDirty(); + void anAutosaveWritesADraftAndClearsTheDirtyFlag(); + void anUnwritableDraftsFolderRaisesThePersistentBanner(); + void aSuccessfulSaveClearsTheBanner(); + void anAccountWithoutADraftsFolderReportsNoFailure(); + void aRewrittenDraftUnlinksThePreviousRevision(); + void theComposerBuildsTheMessageItsWidgetsShow(); + void theFromDropdownDecidesWhichAccountSends(); + void aFormatEditPreservesTheUndoStack(); + void aFormatEditRestoresTheSelectionItAsksFor(); + void aFormatEditOnAnEmptySelectionLandsBetweenTheTokens(); + void theAttachmentWarningRespectsTheConfiguredThreshold(); + void aDisabledAttachmentWarningWarnsAboutNothing(); + void theQuotePositionDecidesWhereTheQuoteLands(); + void theSeededQuoteIsNotAnUndoStep(); + void aReplySeedsTheHtmlToggleFromTheOriginal(); + void aNewMessageSeedsTheHtmlToggleFromConfig(); + void disablingInputsCoversEveryFieldAndTheToolbar(); + void aFailedSendCanBeRetriedWithoutFilingTheWrongCopy(); + void anUnchangedMessageIsNotWrittenAgain(); + void closingInsideTheDebounceStillSavesTheDraft(); + void closingAfterASendWritesNoFurtherDraft(); + void aCloseDuringTheCountdownIsRefused(); + void aFailedSendKeepsTheTextThatFailedToGo(); + void aSmallSizeLimitIsNotDescribedAsZeroMegabytes(); + private: /// Owns the throwaway lock table init() points every test at. A pointer /// rather than a value because it is rebuilt per test, and QTemporaryDir @@ -10770,4 +10808,1120 @@ void TestMainWindow::deletingOutsideTheTrashViewLeavesTheRowInPlace() QCOMPARE(model->rowCount(QModelIndex()), 1); } +// --------------------------------------------------------------------------- +// ComposeWindow, item 123. +// +// The composer owns widgets and nothing else here does, which is why its cases +// live in this file. What is asserted is deliberately NOT what it looks like: +// the autosave dirty check, the banner state, the message its widgets produce, +// the format edits and the seeding rules, all of which are observable without +// a painter. CLAUDE.md's "Rendering probes lie" section covers why counting +// pixels here would prove nothing. +// --------------------------------------------------------------------------- + +namespace { + +/// A Config written to a temporary INI, plus a Maildir root to write into. +/// +/// No notmuch database and no worker: the composer never touches +/// NotmuchWorker, so building one would only cost every case a `notmuch new`. +/// The mail root is passed to ComposeWindow explicitly, exactly as MainWindow +/// passes what the worker reported (item 124: it is NOT database.path). +class ComposeFixture +{ +public: + /// `drafts` and `sent` are written only when non-empty, so a test can + /// build the account-without-a-drafts-folder case by passing an empty + /// string rather than by needing a second fixture. + /// `secondAccount` writes a SECOND sending account, which is what makes + /// the From dropdown have something to choose between. Off by default: + /// every other case here wants exactly one, so a two-account fixture + /// everywhere would let a test pass by picking the only entry there is. + bool build(const QString &drafts = QStringLiteral("Drafts"), + const QString &sent = QStringLiteral("Sent"), + const QString &extraCompose = QString(), + bool secondAccount = false) + { + if (!m_confDir.isValid() || !m_mailDir.isValid()) + return false; + + const QString path = m_confDir.filePath(QStringLiteral("qtmaildir.conf")); + QFile file(path); + if (!file.open(QIODevice::WriteOnly | QIODevice::Text)) + return false; + { + QTextStream out(&file); + // QSettings reads `/` in a section name as a group separator, so + // the section is [account.acct], never [account/acct]. + out << "[account.acct]\n" + << "name=Test User\n" + << "address=user@example.org\n" + << "maildir=acct\n" + << "trash=Trash\n"; + if (!drafts.isEmpty()) + out << "drafts=" << drafts << "\n"; + if (!sent.isEmpty()) + out << "sent=" << sent << "\n"; + // A command that exists and does nothing. canSend() is what the + // From dropdown filters on, so an account without this one line + // would not appear in it at all. + out << "send_command=/bin/true\n"; + if (secondAccount) { + out << "\n[account.other]\n" + << "name=Other User\n" + << "address=other@example.org\n" + << "maildir=other\n" + << "trash=Trash\n" + << "drafts=Drafts\n" + << "sent=Sent\n" + << "send_command=/bin/true\n"; + } + out << "\n[compose]\n"; + if (!extraCompose.isEmpty()) + out << extraCompose << "\n"; + } + file.close(); + + m_config.load(path); + return true; + } + + const Config &config() const { return m_config; } + QString mailRoot() const { return m_mailDir.path(); } + + /// The account's drafts folder, as the composer will resolve it. + QString draftsCur() const + { + return m_mailDir.path() + QStringLiteral("/acct/Drafts/cur"); + } + + /// The second account's drafts folder. + QString otherDraftsCur() const + { + return m_mailDir.path() + QStringLiteral("/other/Drafts/cur"); + } + + /// How many message files sit in the drafts folder. + int draftCount() const + { + return QDir(draftsCur(), {}, QDir::Name, QDir::Files).count(); + } + +private: + QTemporaryDir m_confDir; + QTemporaryDir m_mailDir; + Config m_config; +}; + +/// A minimal New-message context for the fixture's one account. +ComposeContext newContext() +{ + ComposeContext context; + context.accountKey = QStringLiteral("acct"); + context.kind = ComposeContext::Kind::New; + return context; +} + +} // namespace + +void TestMainWindow::aComposerOpensClean() +{ + ComposeFixture fixture; + QVERIFY(fixture.build()); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + + // Seeding fills every field, which emits every field's change signal. A + // composer that counted those as edits would autosave a draft nobody + // asked for, and would tell the quit path there is unsaved work in a + // window the user opened and closed without typing. + QVERIFY(!window.hasUnsavedEdits()); + QVERIFY(!window.lastSaveFailed()); + + auto *timer = window.findChild(QStringLiteral("autosave")); + QVERIFY2(timer, "no autosave timer: the window was never built"); + QVERIFY2(!timer->isActive(), + "seeding armed the autosave timer, so a untouched composer writes"); +} + +void TestMainWindow::typingMarksTheComposerDirty() +{ + ComposeFixture fixture; + QVERIFY(fixture.build()); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + + QVERIFY(!window.hasUnsavedEdits()); + body->setPlainText(QStringLiteral("Some text.")); + QVERIFY(window.hasUnsavedEdits()); + + // The subject is part of the message as much as the body is: a draft that + // saved the body but not the address it was going to would be worse than + // none. + ComposeWindow second(newContext(), fixture.config(), fixture.mailRoot()); + auto *subject = second.findChild(QStringLiteral("subject")); + QVERIFY(subject); + QVERIFY(!second.hasUnsavedEdits()); + subject->setText(QStringLiteral("A subject")); + QVERIFY(second.hasUnsavedEdits()); +} + +void TestMainWindow::anAutosaveWritesADraftAndClearsTheDirtyFlag() +{ + ComposeFixture fixture; + QVERIFY(fixture.build()); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + body->setPlainText(QStringLiteral("Draft body.")); + QVERIFY(window.hasUnsavedEdits()); + + QVERIFY2(window.saveDraftNow(), "the draft write reported failure"); + + QCOMPARE(fixture.draftCount(), 1); + QVERIFY2(!window.hasUnsavedEdits(), + "the flag survived a successful save, so the quit path would ask"); + QVERIFY(!window.lastSaveFailed()); + + // The bytes really are the message, not an empty file: the draft is + // byte-identical to what would be sent, which is the property the one + // built message exists for. + const QStringList files = + QDir(fixture.draftsCur(), {}, QDir::Name, QDir::Files).entryList(); + QCOMPARE(files.size(), 1); + QFile written(fixture.draftsCur() + QLatin1Char('/') + files.first()); + QVERIFY(written.open(QIODevice::ReadOnly)); + const QByteArray bytes = written.readAll(); + QVERIFY2(bytes.contains("Draft body."), "the draft does not carry the body"); + // Written with the Maildir draft flag, not left bare. + QVERIFY2(files.first().endsWith(QStringLiteral(":2,D")), + qPrintable(QStringLiteral("wrong maildir flags: ") + files.first())); +} + +void TestMainWindow::anUnwritableDraftsFolderRaisesThePersistentBanner() +{ + ComposeFixture fixture; + QVERIFY(fixture.build()); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + body->setPlainText(QStringLiteral("Draft body.")); + + // A FILE where the folder must go. mkpath then fails, which is a real + // failure mode and needs no permission games that root would defeat. + const QString accountDir = fixture.mailRoot() + QStringLiteral("/acct"); + QVERIFY(QDir().mkpath(accountDir)); + QFile blocker(accountDir + QStringLiteral("/Drafts")); + QVERIFY(blocker.open(QIODevice::WriteOnly)); + blocker.write("not a directory"); + blocker.close(); + + QVERIFY2(!window.saveDraftNow(), "an unwritable folder reported success"); + + auto *banner = window.findChild(QStringLiteral("draftBanner")); + QVERIFY2(banner, "no banner widget"); + QVERIFY2(!banner->text().isEmpty(), "the banner says nothing"); + QVERIFY2(window.lastSaveFailed(), + "lastSaveFailed() is false after a failed write, so the quit " + "path would let the text go"); + QVERIFY2(window.hasUnsavedEdits(), + "a failed save cleared the dirty flag, which claims the text is " + "safe on disk when it is not"); +} + +void TestMainWindow::aSuccessfulSaveClearsTheBanner() +{ + ComposeFixture fixture; + QVERIFY(fixture.build()); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + body->setPlainText(QStringLiteral("First.")); + + const QString accountDir = fixture.mailRoot() + QStringLiteral("/acct"); + QVERIFY(QDir().mkpath(accountDir)); + QFile blocker(accountDir + QStringLiteral("/Drafts")); + QVERIFY(blocker.open(QIODevice::WriteOnly)); + blocker.close(); + + QVERIFY(!window.saveDraftNow()); + QVERIFY(window.lastSaveFailed()); + + // Remove the obstruction and save again. The banner must go: a warning + // that stays after the thing it warned about is fixed teaches the user to + // ignore warnings, which is the second lesson in the TagRules entry. + QVERIFY(QFile::remove(accountDir + QStringLiteral("/Drafts"))); + body->setPlainText(QStringLiteral("Second.")); + + QVERIFY2(window.saveDraftNow(), "the retry failed"); + QVERIFY2(!window.lastSaveFailed(), "lastSaveFailed() stayed set"); + + auto *banner = window.findChild(QStringLiteral("draftBanner")); + QVERIFY(banner); + QVERIFY2(banner->isHidden(), "the banner is still up after a good save"); +} + +void TestMainWindow::anAccountWithoutADraftsFolderReportsNoFailure() +{ + ComposeFixture fixture; + // No drafts key at all: a real configuration, warned about at startup. + QVERIFY(fixture.build(QString())); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + body->setPlainText(QStringLiteral("Nowhere to save this.")); + + // Nothing was written and nothing failed. Reporting a failure here would + // make the quit path offer a retry for a state no retry can change. + QVERIFY2(window.saveDraftNow(), + "a missing drafts folder was reported as a save failure"); + QVERIFY2(!window.lastSaveFailed(), "the banner state was set"); + + auto *banner = window.findChild(QStringLiteral("draftBanner")); + QVERIFY(banner); + QVERIFY(banner->isHidden()); +} + +void TestMainWindow::aRewrittenDraftUnlinksThePreviousRevision() +{ + ComposeFixture fixture; + QVERIFY(fixture.build()); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + + body->setPlainText(QStringLiteral("Revision one.")); + QVERIFY(window.saveDraftNow()); + QCOMPARE(fixture.draftCount(), 1); + + body->setPlainText(QStringLiteral("Revision two.")); + QVERIFY(window.saveDraftNow()); + + // ONE file, not two. Maildir has no in-place edit, so a draft rewritten + // every thirty seconds would otherwise accumulate one file per pause, and + // every one of them is a message mbsync uploads. + QCOMPARE(fixture.draftCount(), 1); + + const QStringList files = + QDir(fixture.draftsCur(), {}, QDir::Name, QDir::Files).entryList(); + QFile written(fixture.draftsCur() + QLatin1Char('/') + files.first()); + QVERIFY(written.open(QIODevice::ReadOnly)); + const QByteArray bytes = written.readAll(); + QVERIFY2(bytes.contains("Revision two."), "the surviving file is the old one"); +} + +void TestMainWindow::theComposerBuildsTheMessageItsWidgetsShow() +{ + ComposeFixture fixture; + QVERIFY(fixture.build()); + + ComposeContext context = newContext(); + context.kind = ComposeContext::Kind::Reply; + context.inReplyTo = QStringLiteral("original@example.org"); + context.references = { QStringLiteral("root@example.org"), + QStringLiteral("original@example.org") }; + context.to = { QStringLiteral("one@example.org") }; + context.subject = QStringLiteral("Re: a subject"); + + ComposeWindow window(context, fixture.config(), fixture.mailRoot()); + + auto *cc = window.findChild(QStringLiteral("cc")); + auto *bcc = window.findChild(QStringLiteral("bcc")); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(cc && bcc && body); + + // A field the user typed, split on commas. That is wrong for a RAW header + // and right here: this is the composer's own rendering, which joins with + // ", ". + cc->setText(QStringLiteral("two@example.org, three@example.org")); + bcc->setText(QStringLiteral(" four@example.org ")); + body->setPlainText(QStringLiteral("The body.")); + + const OutgoingMessage message = window.currentMessage(); + QCOMPARE(message.accountKey, QStringLiteral("acct")); + QCOMPARE(message.to, QStringList{ QStringLiteral("one@example.org") }); + QCOMPARE(message.cc, (QStringList{ QStringLiteral("two@example.org"), + QStringLiteral("three@example.org") })); + // Trimmed, or the whitespace reaches the wire as part of the address. + QCOMPARE(message.bcc, QStringList{ QStringLiteral("four@example.org") }); + QCOMPARE(message.subject, QStringLiteral("Re: a subject")); + QCOMPARE(message.markdownBody, QStringLiteral("The body.")); + + // NOT optional. Without them a reply appears as an orphan thread in the + // sender's own client, which is invisible locally. + QCOMPARE(message.inReplyTo, QStringLiteral("original@example.org")); + QCOMPARE(message.references.size(), 2); + QCOMPARE(message.references.last(), QStringLiteral("original@example.org")); +} + +void TestMainWindow::theFromDropdownDecidesWhichAccountSends() +{ + // TWO sending accounts, because a dropdown with one entry cannot be + // changed and a test against it passes whether the code reads the dropdown + // or the context. The first revision of this test did exactly that: it + // asserted count() == 1 and then re-asserted a property another case + // already covers, and a mutation making currentAccount() read + // m_context.accountKey survived it. + ComposeFixture fixture; + QVERIFY(fixture.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QString(), /*secondAccount=*/true)); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + auto *from = window.findChild(QStringLiteral("from")); + QVERIFY2(from, "no From dropdown"); + + // Both sending accounts are offered, seeded to the context's. + QCOMPARE(from->count(), 2); + QCOMPARE(from->currentData().toString(), QStringLiteral("acct")); + QCOMPARE(window.currentMessage().accountKey, QStringLiteral("acct")); + + // Now change it. The dropdown is the authority once the window is open: + // reading the context here would send from the seeded account while the + // interface said otherwise. + const int other = from->findData(QStringLiteral("other")); + QVERIFY2(other >= 0, "the second account is not in the dropdown"); + from->setCurrentIndex(other); + + QCOMPARE(window.currentMessage().accountKey, QStringLiteral("other")); + + // And the choice reaches the DRAFT's destination, not just the value: + // a draft is written into the sending account's own folder, so a composer + // that read the context would file it under the wrong account. + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + body->setPlainText(QStringLiteral("From the other account.")); + QVERIFY(window.saveDraftNow()); + + QCOMPARE(QDir(fixture.otherDraftsCur(), {}, QDir::Name, QDir::Files).count(), + 1u); + QCOMPARE(QDir(fixture.draftsCur(), {}, QDir::Name, QDir::Files).count(), 0u); +} + +void TestMainWindow::aFormatEditPreservesTheUndoStack() +{ + ComposeFixture fixture; + QVERIFY(fixture.build()); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + + // Typed through a cursor, which is what makes it an undoable edit; + // setPlainText() would not be one. + QTextCursor typing = body->textCursor(); + typing.insertText(QStringLiteral("hello")); + QVERIFY(body->document()->isUndoAvailable()); + + QTextCursor selection = body->textCursor(); + selection.setPosition(0); + selection.setPosition(5, QTextCursor::KeepAnchor); + body->setTextCursor(selection); + + auto *bold = window.findChild(QStringLiteral("format_bold")); + QVERIFY2(bold, "no bold action"); + bold->trigger(); + + QCOMPARE(body->toPlainText(), QStringLiteral("**hello**")); + + // The property the plan's setPlainText() draft would have lost. Measured + // in a standalone probe: setPlainText() takes isUndoAvailable from true to + // false, so every toolbar press would throw away everything the user could + // undo. + QVERIFY2(body->document()->isUndoAvailable(), + "the format edit destroyed the undo stack"); + + // And it is ONE undo step, not one per character: a whole-document + // replacement inside an edit block collapses to a single entry, so one + // Ctrl+Z takes the tokens off and leaves the typed word. + body->undo(); + QCOMPARE(body->toPlainText(), QStringLiteral("hello")); +} + +void TestMainWindow::aFormatEditRestoresTheSelectionItAsksFor() +{ + ComposeFixture fixture; + QVERIFY(fixture.build()); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + body->setPlainText(QStringLiteral("hello world")); + + // A BACKWARDS selection, anchor after the cursor, which is what a + // right-to-left drag produces and an ordinary gesture. Measured against a + // real widget: selectionStart()/selectionEnd() come back normalised even + // then, so the anchor's side does not reach MarkdownFormat. + QTextCursor selection = body->textCursor(); + selection.setPosition(5); + selection.setPosition(0, QTextCursor::KeepAnchor); + body->setTextCursor(selection); + QCOMPARE(body->textCursor().selectionStart(), 0); + QCOMPARE(body->textCursor().selectionEnd(), 5); + + auto *italic = window.findChild(QStringLiteral("format_italic")); + QVERIFY(italic); + italic->trigger(); + + QCOMPARE(body->toPlainText(), QStringLiteral("*hello* world")); + + // The selection is preserved precisely so a second press can apply a + // SECOND token to the same words, bold then italic without reselecting. + QCOMPARE(body->textCursor().selectedText(), QStringLiteral("hello")); + + auto *bold = window.findChild(QStringLiteral("format_bold")); + QVERIFY(bold); + bold->trigger(); + QCOMPARE(body->toPlainText(), QStringLiteral("***hello*** world")); +} + +void TestMainWindow::aFormatEditOnAnEmptySelectionLandsBetweenTheTokens() +{ + ComposeFixture fixture; + QVERIFY(fixture.build()); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + body->setPlainText(QStringLiteral("ab")); + + QTextCursor cursor = body->textCursor(); + cursor.setPosition(1); + body->setTextCursor(cursor); + + auto *bold = window.findChild(QStringLiteral("format_bold")); + QVERIFY(bold); + bold->trigger(); + + QCOMPARE(body->toPlainText(), QStringLiteral("a****b")); + + // The property a user notices immediately when it is wrong, and the one + // invisible to a test that only compares the resulting text: typing must + // continue INSIDE the pair, not after it. + QCOMPARE(body->textCursor().position(), 3); + QVERIFY(!body->textCursor().hasSelection()); + + QTextCursor typing = body->textCursor(); + typing.insertText(QStringLiteral("x")); + QCOMPARE(body->toPlainText(), QStringLiteral("a**x**b")); +} + +void TestMainWindow::theAttachmentWarningRespectsTheConfiguredThreshold() +{ + ComposeFixture fixture; + QVERIFY(fixture.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("attachment_warn_bytes=1000"))); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + + // The threshold, not the modal. The question itself needs a user, so what + // is asserted is the predicate that decides whether to ask. + QVERIFY2(!window.attachmentNeedsWarning(999), "warned below the limit"); + QVERIFY2(!window.attachmentNeedsWarning(1000), + "warned AT the limit, which is not above it"); + QVERIFY2(window.attachmentNeedsWarning(1001), "did not warn above the limit"); +} + +void TestMainWindow::aDisabledAttachmentWarningWarnsAboutNothing() +{ + ComposeFixture fixture; + QVERIFY(fixture.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("attachment_warn_bytes=0"))); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + + // Zero means off, not "warn about everything". Read as a threshold it + // would question an empty file, which is the opposite of what turning a + // warning off means. + QVERIFY(!window.attachmentNeedsWarning(0)); + QVERIFY(!window.attachmentNeedsWarning(1)); + QVERIFY(!window.attachmentNeedsWarning(100LL * 1024 * 1024)); +} + +void TestMainWindow::theQuotePositionDecidesWhereTheQuoteLands() +{ + const QString quote = QStringLiteral("> the original"); + + { + ComposeFixture above; + QVERIFY(above.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("quote_position=above"))); + ComposeContext context = newContext(); + context.kind = ComposeContext::Kind::Reply; + context.quotedBody = quote; + + ComposeWindow window(context, above.config(), above.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + QVERIFY2(body->toPlainText().startsWith(quote), + "quote_position=above did not put the quote first"); + } + + { + ComposeFixture below; + QVERIFY(below.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("quote_position=below"))); + ComposeContext context = newContext(); + context.kind = ComposeContext::Kind::Reply; + context.quotedBody = quote; + + ComposeWindow window(context, below.config(), below.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + QVERIFY2(body->toPlainText().endsWith(quote), + "quote_position=below did not put the quote last"); + QVERIFY2(!body->toPlainText().startsWith(quote), + "the quote is at the top under quote_position=below"); + } +} + +void TestMainWindow::theSeededQuoteIsNotAnUndoStep() +{ + ComposeFixture fixture; + QVERIFY(fixture.build()); + + ComposeContext context = newContext(); + context.kind = ComposeContext::Kind::Reply; + context.quotedBody = QStringLiteral("> the original"); + + ComposeWindow window(context, fixture.config(), fixture.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + QVERIFY(!body->toPlainText().isEmpty()); + + // The seeded quote is not an edit the user made. One Ctrl+Z on a fresh + // composer must not wipe it, which reads as the buffer losing its content. + // + // Worth knowing before judging this test dead weight: removing + // clearUndoRedoStacks() alone leaves it GREEN, because setPlainText() + // already leaves undo unavailable. The line it guards becomes load-bearing + // the moment seedBody() stops using setPlainText, which is a change with + // reasons to happen: applyEdit() switched to a QTextCursor replacement for + // exactly the undo-stack property this asserts, and a later revision + // seeding the quote the same way would put it on the stack. The combined + // mutation (seed through a cursor AND drop the clear) does kill this. + QVERIFY2(!body->document()->isUndoAvailable(), + "the seeded quote is on the undo stack"); +} + +void TestMainWindow::aReplySeedsTheHtmlToggleFromTheOriginal() +{ + ComposeFixture fixture; + // Config says yes; the original says no. The original wins for a reply: + // an HTML part in it is a fact about the sender's software, not a guess + // about their taste. + QVERIFY(fixture.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("send_html=true"))); + + ComposeContext context = newContext(); + context.kind = ComposeContext::Kind::Reply; + context.seedHtml = false; + + ComposeWindow window(context, fixture.config(), fixture.mailRoot()); + auto *toggle = window.findChild(QStringLiteral("sendHtml")); + QVERIFY2(toggle, "no send-html toggle"); + QVERIFY2(!toggle->isChecked(), + "a reply seeded from config rather than from the original"); + + // And the other way round, so the test cannot pass by always reading + // false: a plain-text config with an HTML original still offers HTML. + ComposeFixture plain; + QVERIFY(plain.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("send_html=false"))); + ComposeContext htmlReply = newContext(); + htmlReply.kind = ComposeContext::Kind::ReplyAll; + htmlReply.seedHtml = true; + + ComposeWindow second(htmlReply, plain.config(), plain.mailRoot()); + auto *secondToggle = + second.findChild(QStringLiteral("sendHtml")); + QVERIFY(secondToggle); + QVERIFY2(secondToggle->isChecked(), + "a reply-all ignored an HTML original"); +} + +void TestMainWindow::aNewMessageSeedsTheHtmlToggleFromConfig() +{ + ComposeFixture off; + QVERIFY(off.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("send_html=false"))); + + // seedHtml is deliberately TRUE here and must be ignored: a New message + // has no original to take evidence from, so a composer reading it would be + // reading a field nothing filled in. + ComposeContext context = newContext(); + context.seedHtml = true; + + ComposeWindow window(context, off.config(), off.mailRoot()); + auto *toggle = window.findChild(QStringLiteral("sendHtml")); + QVERIFY(toggle); + QVERIFY2(!toggle->isChecked(), "a New message ignored [compose] send_html"); + + ComposeFixture on; + QVERIFY(on.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("send_html=true"))); + ComposeContext forward = newContext(); + forward.kind = ComposeContext::Kind::Forward; + forward.seedHtml = false; + + ComposeWindow second(forward, on.config(), on.mailRoot()); + auto *secondToggle = + second.findChild(QStringLiteral("sendHtml")); + QVERIFY(secondToggle); + QVERIFY2(secondToggle->isChecked(), + "a Forward seeded from the original rather than from config"); +} + +void TestMainWindow::disablingInputsCoversEveryFieldAndTheToolbar() +{ + ComposeFixture fixture; + // Zero delay: the countdown is skipped and the send commits at once, which + // is the state the inputs must already be disabled in. + QVERIFY(fixture.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("send_delay_ms=0"))); + + ComposeContext context = newContext(); + context.to = { QStringLiteral("someone@example.org") }; + + // Heap-allocated and tracked with a QPointer, because ComposeWindow sets + // WA_DeleteOnClose and this case really does complete a send: the window + // deletes itself on the way out, so a stack instance would be destroyed + // twice. Every other case here stays on the stack, since none of them + // closes. + QPointer window = + new ComposeWindow(context, fixture.config(), fixture.mailRoot()); + auto *body = window->findChild(QStringLiteral("body")); + QVERIFY(body); + body->setPlainText(QStringLiteral("Text.")); + + auto *toolbar = window->findChild(QStringLiteral("formatToolbar")); + auto *to = window->findChild(QStringLiteral("to")); + auto *subject = window->findChild(QStringLiteral("subject")); + auto *from = window->findChild(QStringLiteral("from")); + auto *toggle = window->findChild(QStringLiteral("sendHtml")); + QVERIFY(toolbar && to && subject && from && toggle); + + QVERIFY(to->isEnabled()); + QVERIFY(!body->isReadOnly()); + + auto *sendAction = window->findChild(QStringLiteral("compose_send")); + QVERIFY2(sendAction, "no send action"); + sendAction->trigger(); + + // The message must not change between pressing Send and the bytes being + // built, so every input goes down for the WHOLE operation, countdown + // included. The body is made read-only rather than disabled, so its text + // stays selectable and legible while the send runs. + QVERIFY2(!to->isEnabled(), "the To field is still editable during a send"); + QVERIFY2(!subject->isEnabled(), "the subject is still editable"); + QVERIFY2(!from->isEnabled(), "the account can still be changed"); + QVERIFY2(!toggle->isEnabled(), "the HTML toggle can still be flipped"); + QVERIFY2(body->isReadOnly(), "the body is still writable during a send"); + QVERIFY2(!toolbar->isEnabled(), "the formatting toolbar is still live"); + auto *attachments = + window->findChild(QStringLiteral("attachments")); + QVERIFY(attachments); + QVERIFY2(!attachments->isEnabled(), + "the attachment list is still live during a send"); + + // /bin/true is the fixture's send command, so the send succeeds and the + // composer closes itself: the message went, and holding a composer open + // for a message already sent invites sending it twice. Waited on rather + // than asserted immediately, since the process is handed to the event loop + // and nothing here blocks on it. WA_DeleteOnClose then destroys the + // window, which is what the QPointer observes. + QTRY_VERIFY_WITH_TIMEOUT(window.isNull(), 15000); + + // And the sent copy really was filed, which is the stage after the send + // and the one whose failure the design treats as the worst outcome here. + const QString sentCur = + fixture.mailRoot() + QStringLiteral("/acct/Sent/cur"); + QCOMPARE(QDir(sentCur, {}, QDir::Name, QDir::Files).count(), 1u); +} + +void TestMainWindow::aFailedSendCanBeRetriedWithoutFilingTheWrongCopy() +{ + ComposeFixture fixture; + QVERIFY(fixture.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("send_delay_ms=0"))); + + // A stub whose outcome is switched by a sentinel file, so ONE configured + // command can fail and then succeed. It appends its stdin to a log, which + // is what makes the delivery count observable: the defect this guards + // against files a sent copy of the FIRST message when the second finishes, + // and a receiver count is the only thing that shows it. + QTemporaryDir stubDir; + QVERIFY(stubDir.isValid()); + const QString sentinel = stubDir.filePath(QStringLiteral("succeed")); + const QString stub = stubDir.filePath(QStringLiteral("send.sh")); + { + QFile script(stub); + QVERIFY(script.open(QIODevice::WriteOnly | QIODevice::Text)); + QTextStream out(&script); + out << "#!/bin/sh\n" + << "cat >> " << stubDir.filePath(QStringLiteral("stdin.log")) << "\n" + << "[ -f " << sentinel << " ] || { echo 'refused' >&2; exit 1; }\n" + << "exit 0\n"; + } + QVERIFY(QFile::setPermissions( + stub, QFileDevice::ReadOwner | QFileDevice::WriteOwner + | QFileDevice::ExeOwner)); + + // A FRESH Config, not a copy of the fixture's reloaded: Config::load() + // does not clear what a previous load put there, so a copy keeps the + // fixture's /bin/true and this test would silently exercise a command that + // always succeeds. Measured, and it produced a green nothing. + Config config; + { + const QString path = QStringLiteral("%1/retry.conf").arg(stubDir.path()); + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + QTextStream out(&file); + out << "[account.acct]\n" + << "name=Test User\n" + << "address=user@example.org\n" + << "maildir=acct\n" + << "trash=Trash\n" + << "drafts=Drafts\n" + << "sent=Sent\n" + << "send_command=" << stub << "\n" + << "\n[compose]\n" + << "send_delay_ms=0\n"; + file.close(); + config.load(path); + } + + ComposeContext context = newContext(); + context.to = { QStringLiteral("someone@example.org") }; + + QPointer window = + new ComposeWindow(context, config, fixture.mailRoot()); + auto *body = window->findChild(QStringLiteral("body")); + auto *sendAction = window->findChild(QStringLiteral("compose_send")); + QVERIFY(body && sendAction); + + body->setPlainText(QStringLiteral("FIRST attempt.")); + sendAction->trigger(); + + // The failure re-enables the composer intact and shows the stderr; the + // window stays open and the draft stays. + auto *pane = window->findChild(QStringLiteral("sendLogPane")); + QVERIFY(pane); + QTRY_VERIFY_WITH_TIMEOUT(!pane->isHidden(), 15000); + + QVERIFY2(!window.isNull(), "a failed send closed the composer"); + QVERIFY2(body->isEnabled() && !body->isReadOnly(), + "a failed send left the composer disabled"); + + // Correct the message and send again, this time succeeding. Without + // Qt::SingleShotConnection on the per-send connect, the first send's + // lambda is still attached: the second result runs BOTH, and 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. + QFile marker(sentinel); + QVERIFY(marker.open(QIODevice::WriteOnly)); + marker.close(); + + body->setPlainText(QStringLiteral("SECOND attempt.")); + sendAction->trigger(); + + QTRY_VERIFY_WITH_TIMEOUT(window.isNull(), 15000); + + // Exactly ONE sent copy, and it is the second message. Two files, or one + // carrying the first attempt, is the accumulated-receiver defect. + const QString sentCur = fixture.mailRoot() + QStringLiteral("/acct/Sent/cur"); + const QStringList filed = + QDir(sentCur, {}, QDir::Name, QDir::Files).entryList(); + QCOMPARE(filed.size(), 1); + + QFile copy(sentCur + QLatin1Char('/') + filed.first()); + QVERIFY(copy.open(QIODevice::ReadOnly)); + const QByteArray bytes = copy.readAll(); + QVERIFY2(bytes.contains("SECOND attempt."), + "the filed copy is not the message that was sent"); + QVERIFY2(!bytes.contains("FIRST attempt."), + "the filed copy is the FIRST message, which never went"); +} + +void TestMainWindow::anUnchangedMessageIsNotWrittenAgain() +{ + ComposeFixture fixture; + QVERIFY(fixture.build()); + + ComposeWindow window(newContext(), fixture.config(), fixture.mailRoot()); + auto *body = window.findChild(QStringLiteral("body")); + QVERIFY(body); + + body->setPlainText(QStringLiteral("Once.")); + QVERIFY(window.saveDraftNow()); + QCOMPARE(fixture.draftCount(), 1); + + const QStringList first = + QDir(fixture.draftsCur(), {}, QDir::Name, QDir::Files).entryList(); + QCOMPARE(first.size(), 1); + + // Nothing has changed, so nothing is written. Every autosave produces a + // Maildir write that mbsync uploads, so this check and the debounce + // together are what keep a message to a few revisions rather than dozens. + // + // The FILENAME is what shows it: DraftStore always generates a fresh name + // and unlinks the previous one, so a redundant write leaves exactly one + // file too, and a count alone cannot tell a skipped write from a repeated + // one. Two runs of this test asserting only on the count would pass + // against no check at all. + QVERIFY2(window.saveDraftNow(), "the redundant save reported failure"); + QCOMPARE(fixture.draftCount(), 1); + const QStringList second = + QDir(fixture.draftsCur(), {}, QDir::Name, QDir::Files).entryList(); + QCOMPARE(second, first); + + // And a real change still writes: a check that skipped everything would + // pass the assertion above and lose the user's text. + body->setPlainText(QStringLiteral("Twice.")); + QVERIFY(window.saveDraftNow()); + const QStringList third = + QDir(fixture.draftsCur(), {}, QDir::Name, QDir::Files).entryList(); + QCOMPARE(third.size(), 1); + QVERIFY2(third != first, "a changed message was not written"); +} + +void TestMainWindow::closingInsideTheDebounceStillSavesTheDraft() +{ + ComposeFixture fixture; + // A debounce far longer than this test, so the timer provably never fires + // and the only thing that can write is the close itself. + QVERIFY(fixture.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("autosave_interval_ms=600000"))); + + // Heap-allocated: WA_DeleteOnClose destroys the window on the way out, so + // a stack instance would be destroyed twice. + QPointer window = + new ComposeWindow(newContext(), fixture.config(), fixture.mailRoot()); + auto *body = window->findChild(QStringLiteral("body")); + QVERIFY(body); + + body->setPlainText(QStringLiteral("A paragraph typed and not yet saved.")); + QVERIFY(window->hasUnsavedEdits()); + + // The timer has NOT fired. Asserted rather than assumed: if it had, the + // draft below would prove nothing about the close path. + auto *timer = window->findChild(QStringLiteral("autosave")); + QVERIFY(timer); + QVERIFY2(timer->isActive(), "the debounce is not running"); + QCOMPARE(fixture.draftCount(), 0); + + // The window manager's X button, which is the route that reaches + // closeEvent. Typing a paragraph and pressing it inside the debounce + // interval must not lose the text. + window->close(); + QTRY_VERIFY_WITH_TIMEOUT(window.isNull(), 5000); + + QCOMPARE(fixture.draftCount(), 1); + const QStringList files = + QDir(fixture.draftsCur(), {}, QDir::Name, QDir::Files).entryList(); + QCOMPARE(files.size(), 1); + QFile written(fixture.draftsCur() + QLatin1Char('/') + files.first()); + QVERIFY(written.open(QIODevice::ReadOnly)); + QVERIFY2(written.readAll().contains("A paragraph typed and not yet saved."), + "the close wrote a draft that is not the text that was typed"); +} + +void TestMainWindow::closingAfterASendWritesNoFurtherDraft() +{ + ComposeFixture fixture; + QVERIFY(fixture.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("send_delay_ms=0"))); + + ComposeContext context = newContext(); + context.to = { QStringLiteral("someone@example.org") }; + + QPointer window = + new ComposeWindow(context, fixture.config(), fixture.mailRoot()); + auto *body = window->findChild(QStringLiteral("body")); + auto *sendAction = window->findChild(QStringLiteral("compose_send")); + QVERIFY(body && sendAction); + + body->setPlainText(QStringLiteral("Text that is about to be sent.")); + + // A draft on disk first, so the send's removal of it is observable and the + // close-path save has something it could wrongly put back. + QVERIFY(window->saveDraftNow()); + QCOMPARE(fixture.draftCount(), 1); + + // Now edit again WITHOUT saving, so m_dirty is true at the moment the + // send completes. This is what makes the m_finished guard load-bearing: + // without it the close that follows a successful send would write a draft + // for a message already sent, restoring the file the send just unlinked. + body->setPlainText(QStringLiteral("Text that is about to be sent, edited.")); + QVERIFY(window->hasUnsavedEdits()); + + sendAction->trigger(); + QTRY_VERIFY_WITH_TIMEOUT(window.isNull(), 15000); + + // The message went, so the drafts folder is EMPTY. A draft left behind is + // a message the user sees waiting to be finished when it has already been + // delivered. + QCOMPARE(fixture.draftCount(), 0); + + // And the sent copy is there, so this is a completed send rather than a + // send that never happened leaving nothing behind either way. + const QString sentCur = fixture.mailRoot() + QStringLiteral("/acct/Sent/cur"); + QCOMPARE(QDir(sentCur, {}, QDir::Name, QDir::Files).count(), 1u); +} + +void TestMainWindow::aCloseDuringTheCountdownIsRefused() +{ + ComposeFixture fixture; + // A countdown long enough to close inside. The default is 5000; this is + // the window the guard exists for and it must be provably still open when + // the close is attempted. + QVERIFY(fixture.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("send_delay_ms=30000"))); + + ComposeContext context = newContext(); + context.to = { QStringLiteral("someone@example.org") }; + + QPointer window = + new ComposeWindow(context, fixture.config(), fixture.mailRoot()); + auto *body = window->findChild(QStringLiteral("body")); + auto *sendAction = window->findChild(QStringLiteral("compose_send")); + QVERIFY(body && sendAction); + body->setPlainText(QStringLiteral("Sent after a countdown.")); + + sendAction->trigger(); + + // Still counting down: the popup is up and nothing has been sent. The + // sent folder is the evidence, since it is written only after the command + // succeeds. + auto *dialog = window->findChild(); + QVERIFY2(dialog, "no send popup"); + QVERIFY2(!dialog->isCommitted(), "the countdown already committed"); + + // Close during the countdown. Refused: accepting it would destroy this + // window, take the parented SendDialog down with it, and committed() would + // never fire. The user pressed Send, watched a countdown, and would + // believe the mail went. + window->close(); + + // Given a moment for a deletion event to be delivered if one was posted, + // then asserted still alive. An immediate check would pass against a + // deleteLater() already queued. + QTest::qWait(300); + QVERIFY2(!window.isNull(), + "the close was accepted during the countdown, so the send was " + "silently abandoned after the user pressed Send"); + QVERIFY2(window->isVisible() || !window.isNull(), "the window went away"); + + // The send never happened, which is the point: nothing was filed. + const QString sentCur = fixture.mailRoot() + QStringLiteral("/acct/Sent/cur"); + QCOMPARE(QDir(sentCur, {}, QDir::Name, QDir::Files).count(), 0u); + + // Cleaned up by hand, since the window refuses to close while the popup is + // up and the test must not leak it into the next case. + delete window; +} + +void TestMainWindow::aFailedSendKeepsTheTextThatFailedToGo() +{ + ComposeFixture fixture; + QVERIFY(fixture.build(QStringLiteral("Drafts"), QStringLiteral("Sent"), + QStringLiteral("send_delay_ms=0"))); + + QTemporaryDir stubDir; + QVERIFY(stubDir.isValid()); + const QString stub = stubDir.filePath(QStringLiteral("fail.sh")); + { + QFile script(stub); + QVERIFY(script.open(QIODevice::WriteOnly | QIODevice::Text)); + QTextStream out(&script); + out << "#!/bin/sh\ncat > /dev/null\necho 'refused' >&2\nexit 1\n"; + } + QVERIFY(QFile::setPermissions( + stub, QFileDevice::ReadOwner | QFileDevice::WriteOwner + | QFileDevice::ExeOwner)); + + Config config; + { + const QString path = stubDir.filePath(QStringLiteral("fail.conf")); + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + QTextStream out(&file); + out << "[account.acct]\n" + << "name=Test User\naddress=user@example.org\n" + << "maildir=acct\ntrash=Trash\ndrafts=Drafts\nsent=Sent\n" + << "send_command=" << stub << "\n" + << "\n[compose]\nsend_delay_ms=0\n"; + file.close(); + config.load(path); + } + + ComposeContext context = newContext(); + context.to = { QStringLiteral("someone@example.org") }; + + QPointer window = + new ComposeWindow(context, config, fixture.mailRoot()); + auto *body = window->findChild(QStringLiteral("body")); + auto *sendAction = window->findChild(QStringLiteral("compose_send")); + QVERIFY(body && sendAction); + + // An OLD revision on disk, then an edit that is not saved. send() builds + // from the widgets without saving, so without the fix the file left behind + // after the failure is the old text: the user watches their correction be + // sent, sees it fail, and gets the uncorrected version back. + body->setPlainText(QStringLiteral("The ORIGINAL text.")); + QVERIFY(window->saveDraftNow()); + QCOMPARE(fixture.draftCount(), 1); + + body->setPlainText(QStringLiteral("The CORRECTED text.")); + sendAction->trigger(); + + auto *pane = window->findChild(QStringLiteral("sendLogPane")); + QVERIFY(pane); + QTRY_VERIFY_WITH_TIMEOUT(!pane->isHidden(), 15000); + QVERIFY2(!window.isNull(), "a failed send closed the composer"); + + // Exactly one draft, and it is the text that was attempted. + QCOMPARE(fixture.draftCount(), 1); + const QStringList files = + QDir(fixture.draftsCur(), {}, QDir::Name, QDir::Files).entryList(); + QCOMPARE(files.size(), 1); + QFile written(fixture.draftsCur() + QLatin1Char('/') + files.first()); + QVERIFY(written.open(QIODevice::ReadOnly)); + const QByteArray bytes = written.readAll(); + QVERIFY2(bytes.contains("The CORRECTED text."), + "the draft kept after a failed send is not what was attempted"); + QVERIFY2(!bytes.contains("The ORIGINAL text."), + "the draft kept after a failed send is the PRE-EDIT revision"); + + delete window; +} + +void TestMainWindow::aSmallSizeLimitIsNotDescribedAsZeroMegabytes() +{ + // Integer MB division made every figure under a megabyte read as "0 MB", + // in BOTH halves of the same sentence: "'x' is 0 MB. Many mail servers + // refuse messages above about 0 MB." + QVERIFY2(!ComposeWindow::humanSize(500 * 1024).contains(QStringLiteral("0 MB")), + "half a megabyte is described as 0 MB"); + QVERIFY2(!ComposeWindow::humanSize(1000).contains(QStringLiteral("0 MB")), + "a kilobyte is described as 0 MB"); + + // The unit steps down rather than reporting zero of a larger one. + QVERIFY(ComposeWindow::humanSize(500 * 1024).contains(QStringLiteral("KB"))); + QVERIFY(ComposeWindow::humanSize(512).contains(QStringLiteral("bytes"))); + + // A decimal while the figure is small enough for it to say something, so + // 26 MB and 26.2 MB are not the same string. + QVERIFY(ComposeWindow::humanSize(26214400).contains(QStringLiteral("MB"))); + QVERIFY2(ComposeWindow::humanSize(1024 * 1024 * 3 / 2) + .contains(QStringLiteral(".")), + "1.5 MB lost its decimal"); +} + #include "test_mainwindow.moc" diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index 83ac087..b2d96eb 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -1,6 +1,137 @@ + + ComposeWindow + + Compose + Componi + + + From: + Da: + + + To: + A: + + + Cc: + Cc: + + + Bcc: + Ccn: + + + Subject: + Oggetto: + + + Also send a formatted copy + Invia anche una copia formattata + + + Sends the message as plain text with a formatted version alongside it. The plain text is what you typed. + Invia il messaggio come testo semplice con accanto una versione formattata. Il testo semplice è quello che hai scritto. + + + Send output + Output dell’invio + + + Close + Chiudi + + + Formatting + Formattazione + + + Bold + Grassetto + + + Italic + Corsivo + + + Code + Codice + + + Strikethrough + Barrato + + + Link + Collegamento + + + Quote + Citazione + + + Attach... + Allega... + + + Attach files + Allega file + + + Remove attachment + Rimuovi allegato + + + Send + Invia + + + Large attachment + Allegato di grandi dimensioni + + + '%1' is %2. Many mail servers refuse messages above about %3. Attach it anyway? + '%1' occupa %2. Molti server di posta rifiutano messaggi oltre i %3 circa. Allegarlo comunque? + + + The draft could not be saved: %1 + Non è stato possibile salvare la bozza: %1 + + + The send command reported no output. + Il comando di invio non ha prodotto alcun output. + + + Cannot send + Impossibile inviare + + + The account '%1' has no send command configured. + L’account '%1' non ha un comando di invio configurato. + + + Sent, but not filed + Inviato, ma non archiviato + + + The message was sent, but the copy could not be written to '%1' for account '%2': + +%3 + +The message HAS been sent. Do not send it again. + Il messaggio è stato inviato, ma non è stato possibile scrivere la copia in '%1' per l’account '%2': + +%3 + +Il messaggio È stato inviato. Non inviarlo di nuovo. + + + The send command could not be started. + Non è stato possibile avviare il comando di invio. + + Config @@ -1329,6 +1460,18 @@ Cannot write to %1: %2 Impossibile scrivere su %1: %2 + + %1 MB + %1 MB + + + %1 KB + %1 KB + + + %1 bytes + %1 byte + QueryCompleter -- 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 'tests/test_mainwindow.cpp') 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 0c6d43d62cae7815096919239b64891843864896 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sun, 23 Aug 2026 20:39:17 +0200 Subject: fix(compose): close every composer when the main window quits A composer is deliberately parentless, so that it appears in the task switcher and stays usable while the main window is. Qt therefore does not take it down with that window, and being a live top-level it kept the process alive: the main window vanished, the composer stayed on screen with nothing behind it, and closing it then raised the unsaved-edits dialog for a session the user had already ended. The quit path already ASKED about those edits and saved them. What it never did was close the windows afterwards. Closing rather than deleting: WA_DeleteOnClose is set on every composer, so close() is what frees them, and it lets ComposeWindow::closeEvent() run its own draft handling on the way out. Iterating a copy of the list, since closing runs the `closed` handler and that mutates m_composers. Placed last, after every route that turns back has returned: reaching it means the application really is quitting. The test opens TWO composers, so a fix that closed only the last one cannot pass it. Mutation-checked by removing the loop. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q2koFevoSxTLhfexJTZWQd --- src/mainwindow.cpp | 27 +++++++++++++++++++++++++++ tests/test_mainwindow.cpp | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) (limited to 'tests/test_mainwindow.cpp') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 0131959..9a94a2e 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -422,6 +422,33 @@ void MainWindow::closeEvent(QCloseEvent *event) } } + // Every composer goes with the window, and this is the LAST thing before + // the close is accepted: every route that turns back (Cancel, a failed + // sync, a refused save) has already returned above, so reaching here means + // the application really is quitting. + // + // A composer is deliberately parentless, so that it appears in the task + // switcher and stays usable while the main window is. Qt therefore does not + // take it down with this window, and it kept the process alive: the main + // window vanished, the composer stayed on screen with nothing behind it, + // and closing it then raised the unsaved-edits dialog for a session that + // had already ended. + // + // Closing rather than deleting. WA_DeleteOnClose is set on every composer, + // so close() is what frees them, and it lets ComposeWindow::closeEvent() + // run its own draft handling on the way out. The drafts have already been + // saved by the dialogs above, so that pass has nothing left to do; going + // through it anyway keeps ONE exit path rather than a second one that has + // to be kept in step. + // + // Iterating a COPY: closing a composer runs the `closed` handler, which + // mutates m_composers, and mutating a container mid-iteration is undefined. + const QList> composers = m_composers; + for (const QPointer &composer : composers) { + if (composer) + composer->close(); + } + saveUiState(); QMainWindow::closeEvent(event); } diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index ecaab2f..98dae12 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -267,6 +267,7 @@ private slots: void savingAMessageWithAHostileSubjectStaysInTheDirectory(); void aStuckComposeRequestDoesNotHijackTheNextPaneLoad(); void theSaveLoopToleratesAComposerClosedUnderTheDialog(); + void quittingClosesEveryComposerRatherThanOrphaningIt(); void forwardingCarriesTheOriginalsAttachments(); void forwardSeedsHtmlFromTheConfigNotTheOriginal(); void aStartupAccountScopesTheStartupQuery(); @@ -8833,6 +8834,43 @@ void TestMainWindow::aStuckComposeRequestDoesNotHijackTheNextPaneLoad() QCOMPARE(window.openComposerCount(), 0); } +void TestMainWindow::quittingClosesEveryComposerRatherThanOrphaningIt() +{ + // A composer is a parentless top-level window, deliberately: it must appear + // in the task switcher and be usable while the main window is. The cost is + // that closing the main window does NOT take it down, so quitting left a + // composer on screen with no application behind it, and Qt kept the process + // alive for it. Reported from a hand test: the main window closed, the + // orphan stayed, and its own close then raised the unsaved-edits dialog for + // a session the user had already ended. + // + // The quit path already ASKS about those edits and saves them; what it + // never did was close the windows afterwards. + 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); + + // Two, so the fix cannot be "close the last one" and pass. + QVERIFY2(window.openComposerForTest(), "no composer opened"); + QVERIFY2(window.openComposerForTest(), "no second composer opened"); + QCOMPARE(window.openComposerCount(), 2); + + // Clean composers: the point here is the CLOSE, not the unsaved-edits + // dialog, which has its own tests and would block this one on a modal. + window.show(); + window.close(); + + // deleteLater() is how a composer goes away, so the count settles on the + // next event-loop pass rather than synchronously. + QTRY_COMPARE_WITH_TIMEOUT(window.openComposerCount(), 0, 5000); +} + void TestMainWindow::theSaveLoopToleratesAComposerClosedUnderTheDialog() { // The regression for a measured use-after-free. composersBlockingQuit() -- cgit v1.2.3