diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/composewindow.cpp | 68 | ||||
| -rw-r--r-- | src/composewindow.h | 44 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 625 | ||||
| -rw-r--r-- | src/mainwindow.h | 190 | ||||
| -rw-r--r-- | src/messageview.cpp | 29 | ||||
| -rw-r--r-- | src/messageview.h | 10 | ||||
| -rw-r--r-- | src/notmuchworker.cpp | 32 | ||||
| -rw-r--r-- | src/notmuchworker.h | 20 |
8 files changed, 998 insertions, 20 deletions
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 <QTemporaryDir> + #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<QTemporaryDir>(); + 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 <QMainWindow> #include <QStringList> +#include <memory> + #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<QTemporaryDir> 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 <QDialog> #include <QDialogButtonBox> #include <QDir> +#include <QFileDialog> #include <QFileInfo> #include <QHBoxLayout> #include <QHeaderView> @@ -47,6 +48,8 @@ #include <QToolButton> #include <QVBoxLayout> +#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<ComposeWindow> &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<ComposeWindow> &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<QPointer<ComposeWindow>> blocking = composersBlockingQuit(); + if (!blocking.isEmpty()) { + QStringList titles; + titles.reserve(blocking.size()); + for (const QPointer<ComposeWindow> &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<ComposeWindow> &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<ComposeWindow>(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<ComposeWindow> &p) { + return p.isNull() || p.data() == which; + }); + }); + + composer->show(); +} + +QList<QPointer<ComposeWindow>> MainWindow::composersBlockingQuit() const +{ + QList<QPointer<ComposeWindow>> blocking; + for (const QPointer<ComposeWindow> &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<ComposeWindow *> MainWindow::openComposersForTest() const { + QList<ComposeWindow *> live; + for (const QPointer<ComposeWindow> &composer : m_composers) { + if (composer) + live.append(composer.data()); + } + return live; +} + +int MainWindow::openComposerCount() const +{ + int live = 0; + for (const QPointer<ComposeWindow> &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<ComposeWindow> &composer : m_composers) { + if (!composer) + continue; + if (auto *body = composer->findChild<QPlainTextEdit *>( + 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::PlaceholderLine> 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<MessageRef> &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<QPointer<ComposeWindow>> 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<ComposeWindow *> 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<int> &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<QPointer<ComposeWindow>> 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<ThreadSummary> &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: |
