From e125d970aa2f4ad6cd494e5f410ba1c5e53f5308 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 17 Aug 2026 20:04:05 +0200 Subject: feat(worker): move messages between maildir folders The first mutation here that is not a notmuch tag. Indexes the new path before dropping the old one, since removing the last filename for a message id deletes the database entry and every tag on it. --- src/notmuchworker.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) (limited to 'src/notmuchworker.h') diff --git a/src/notmuchworker.h b/src/notmuchworker.h index f07e563..d8d8ff8 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -120,6 +120,19 @@ public slots: /// it would block the user's cron `notmuch new`. void applyTags(const TagChange &change); + /// Moves messages into `destFolder`, relative to the database path. + /// + /// A folder NAME rather than a "move to trash" call, because v2's Send + /// needs exactly this operation for Drafts and Sent. Nothing + /// trash-specific belongs here. + /// + /// The first mutation in this class that is not a notmuch tag: a rename on + /// disk plus a reindex. Ordering is rename, index the new path, drop the + /// old one. Indexing first is required, not stylistic: removing the last + /// filename for a message id deletes the database entry and every tag on + /// it, so removing before indexing loses the message's tags. + void moveMessages(const QStringList &messageIds, const QString &destFolder); + /// Batch tagging over whole threads. The UI holds thread ids, not message /// ids, for rows it has not opened, so the resolution happens here where /// the database handle lives. This is the path the archive/flag/delete @@ -184,6 +197,11 @@ signals: quint64 generation); void messageLoaded(const QVector &messages, quint64 generation); void tagsApplied(const TagChange &change); + + /// Carries the ids that ACTUALLY moved, which may be fewer than requested. + /// A stale id, a missing folder or a failed rename drops out here rather + /// than aborting the batch. + void messagesMoved(const QStringList &messageIds, const QString &destFolder); void allTagsReady(const QStringList &tags, quint64 generation); /// One entry per requested query, in the order they were asked for. A query -- cgit v1.2.3 From 262174407eabcb986f15c116d39b7ab98fdf0150 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 17 Aug 2026 21:31:31 +0200 Subject: feat(delete): move the message to the account's trash folder Delete added the `deleted` tag and moved nothing, so deleted mail sat in the inbox indefinitely with only a chip saying otherwise. It now moves the file into the account's trash, records where it came from, and moves it back on undo. The origin is derived in the WORKER, not in the UI, because nowhere else knows it. A Maildir filename does not record the folder a message came from and notmuch cannot answer once the file has moved, so the moment the old filename exists inside moveMessages() is the only place it can be read. It travels back on a new messagesMovedFrom() signal, and the UI turns it into a `deleted-from:` tag that Restore reads days later. The account is resolved from the message's PATH rather than from its account tag: that tag is optional config, so resolving through it would silently make an account undeletable. That needed ThreadSummary to carry the first message's path, since an unexpanded thread row is the ordinary case and held no path at all. It is reported relative to the database root, because the UI knows accounts only by their maildir, itself a database-relative prefix. accountForMessagePath() accepts both an absolute and a relative path, and that is load-bearing rather than defensive: a thread row's path is relative while a reply row's is absolute, since MimeParser has to open it. Matching only one form left Delete on a reply resolving to no account and moving nothing, which is the thread-row/reply-row asymmetry this file has been bitten by before. Tags are applied only once the worker CONFIRMS the move. Tagging first would leave a message marked deleted in a folder it never left when a rename fails, which is the half-done state this removes. A move made during a sync is held in its own queue and flushed like a tag edit: the existing queue carries tag changes only, so a move pushed through it would apply `deleted` and never move the file. An account with no trash configured reports through the status bar and tags nothing, as a second line of defence behind the config-load warning. Six existing tests used `delete` as a stand-in for a message-scoped tag action on bare windows with no account; they move to `spam` and `delete_thread`, which stayed tag-only, keeping the property each was actually testing. Co-Authored-By: Claude Opus 5 --- src/mainwindow.cpp | 292 +++++++++++++++++++++++++++- src/mainwindow.h | 139 ++++++++++++++ src/notmuchworker.cpp | 56 ++++++ src/notmuchworker.h | 20 ++ src/threadlistmodel.cpp | 11 ++ src/types.h | 20 ++ tests/test_mainwindow.cpp | 415 ++++++++++++++++++++++++++++++++++++++-- translations/qtmaildir_it_IT.ts | 7 + 8 files changed, 938 insertions(+), 22 deletions(-) (limited to 'src/notmuchworker.h') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 67d3ee4..d3f2dc7 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -841,10 +841,13 @@ void MainWindow::registerActions() // is about reading a message at all. const bool allDeleted = everySelectedRowHasTag(QStringLiteral("deleted")); + // Item 103. A MOVE now, not only a tag: Delete used to add `deleted` + // and leave the file exactly where it was, so deleted mail sat in the + // inbox indefinitely and only the chip said otherwise. if (allDeleted) - tagSelected({}, { QStringLiteral("deleted") }, tr("Undelete")); + restoreSelected(); else - tagSelected({ QStringLiteral("deleted") }, {}, tr("Delete")); + trashSelected(); }); addAction(QStringLiteral("spam"), tr("Mark &spam"), tr("Add spam and remove inbox"), [this]() { @@ -1604,6 +1607,12 @@ void MainWindow::wireWorker() connect(m_worker, &NotmuchWorker::tagsApplied, this, &MainWindow::onTagsApplied); + // messagesMovedFrom rather than messagesMoved: the tags a move carries can + // only be resolved once the origins are known, and that signal is the one + // that reports them. + connect(m_worker, &NotmuchWorker::messagesMovedFrom, + this, &MainWindow::onMessagesMoved); + m_workerThread.start(); // Queued behind the thread start, so the completer has real tags as soon @@ -2922,6 +2931,21 @@ bool MainWindow::aSyncHoldsTheWriteLock() const void MainWindow::flushHeldEdits() { + // Moves first, and they are flushed even when no tag edit is waiting: the + // early return below used to be the whole guard, so a held move with an + // empty edit queue would never have been sent at all. That is item 106's + // data loss with a worse shape, since a dropped move leaves the file where + // the user asked it not to be. + if (!m_heldMoves.isEmpty()) { + const QVector moves = m_heldMoves; + m_heldMoves.clear(); + for (const HeldMove &move : moves) { + sendMove(move.messageIds, move.destFolder, move.add, move.remove, + move.description); + } + updatePendingIndicator(); + } + if (m_heldEdits.isEmpty()) return; @@ -4082,6 +4106,270 @@ void MainWindow::sendMessageTagChange(const QStringList &messageIds, Q_ARG(TagChange, m_pendingChange)); } +const QString &MainWindow::kOriginTagPlaceholder() +{ + // Not wrapped in tr(). It is never displayed: onMessagesMoved() replaces + // it with a real tag before anything reaches the worker, and a translated + // placeholder would stop matching in the one locale that translated it, + // which is the trap CLAUDE.md records for startup_query. + static const QString placeholder = + QStringLiteral("\x01qtmaildir-origin-placeholder"); + return placeholder; +} + +Account MainWindow::accountForMessagePath(const QString &path) const +{ + // From the PATH, not from the thread's account tag. The tag is optional + // config, so resolving through it would silently disable Delete for an + // account that never set one; a message's maildir prefix is what makes it + // belong to an account at all. + // + // Longest maildir wins, so nested account maildirs (`mail` and + // `mail/work`) resolve to the more specific one rather than to whichever + // happens to be listed first. + // + // BOTH path shapes are accepted, and that is not defensive coding. A + // thread row's path comes from ThreadSummary::firstMessagePath and is + // database-RELATIVE; a reply row's comes from MessageNode::filePath and is + // ABSOLUTE, because MimeParser has to open it. Matching only the relative + // form resolved every reply to no account, so Delete on a reply reported + // "no trash folder configured" and moved nothing, which is exactly the + // thread-row/reply-row asymmetry this file has been bitten by before. + // + // A `/` is required after the maildir in both cases, so `acctX` cannot + // match an account whose maildir is `acct`. + Account best; + int bestLength = -1; + for (const Account &account : m_config.accounts()) { + if (account.maildir.isEmpty()) + continue; + const QString segment = QLatin1Char('/') + account.maildir + + QLatin1Char('/'); + const bool matches = + path.startsWith(account.maildir + QLatin1Char('/')) + || path.contains(segment); + if (!matches) + continue; + if (account.maildir.length() > bestLength) { + best = account; + bestLength = account.maildir.length(); + } + } + return best; +} + +void MainWindow::trashSelected() +{ + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + if (rows.isEmpty()) + return; + + // Message scope, exactly as tagSelected() uses by default: a thread row + // stands for the ONE message its card displays. Escalating to the thread + // would move a whole conversation into the trash because the user deleted + // one reply. + const ActionScope scope = m_model->messageScopeFor(rows); + if (scope.messageIds.isEmpty()) + return; + + // Grouped by destination, because moveMessages() takes one folder per call + // and a selection can span accounts with different trash folders. + QHash byTrash; + QStringList unconfigured; + for (const QString &messageId : scope.messageIds) { + const QString path = m_model->messageById(messageId).filePath; + const Account account = accountForMessagePath(path); + if (account.trash.isEmpty()) { + unconfigured.append(messageId); + continue; + } + byTrash[account.maildir + QLatin1Char('/') + account.trash] + .append(messageId); + } + + // Task 2 warns at config load; this is the second line of defence, for a + // user who never fixed it. Reported rather than silently doing nothing, + // and NOT tagged either: a `deleted` tag on a file still in the inbox is + // precisely the half-done state this item removes. + if (!unconfigured.isEmpty()) { + m_statusLabel->setText( + tr("%n message(s) could not be deleted: no trash folder is " + "configured for their account.", "", int(unconfigured.size()))); + } + + if (byTrash.isEmpty()) + return; + + for (auto it = byTrash.cbegin(); it != byTrash.cend(); ++it) { + sendMove(it.value(), it.key(), + { QStringLiteral("deleted"), kOriginTagPlaceholder() }, {}, + tr("Delete")); + } + + showTransientStatus( + tr("%1: %n message(s)", "", scope.messageCount).arg(tr("Delete"))); +} + +void MainWindow::restoreSelected() +{ + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + if (rows.isEmpty()) + return; + + const ActionScope scope = m_model->messageScopeFor(rows); + if (scope.messageIds.isEmpty()) + return; + + // Where each message came from, read back off its own tag. This is what + // the tag exists for: the file has moved, so nothing on disk and nothing + // in notmuch still records the original folder. + const QString prefix = QStringLiteral("deleted-from:"); + QHash byOrigin; + QStringList unknown; + for (const QString &messageId : scope.messageIds) { + const MessageNode node = m_model->messageById(messageId); + QString origin; + for (const QString &tag : node.tags) { + if (tag.startsWith(prefix)) { + origin = tag.mid(prefix.length()); + break; + } + } + // An account prefix is needed to name a folder to the worker, which + // works in database-relative paths. The origin tag stores the folder + // relative to the ACCOUNT, so the two are recomposed here. + const Account account = accountForMessagePath(node.filePath); + if (origin.isEmpty() || account.maildir.isEmpty()) { + unknown.append(messageId); + continue; + } + byOrigin[account.maildir + QLatin1Char('/') + origin].append(messageId); + } + + if (!unknown.isEmpty()) { + // No origin recorded, which is the case for mail deleted by an older + // version or tagged by hand. The tag comes off so the row stops + // claiming to be deleted, but no file moves: guessing a folder would + // put the message somewhere the user never had it. + sendMessageTagChange(unknown, {}, { QStringLiteral("deleted") }, + tr("Undelete")); + m_undoStack.push(new MessageTagCommand( + this, unknown, {}, { QStringLiteral("deleted") }, tr("Undelete"))); + } + + for (auto it = byOrigin.cbegin(); it != byOrigin.cend(); ++it) { + sendMove(it.value(), it.key(), {}, + { QStringLiteral("deleted"), kOriginTagPlaceholder() }, + tr("Undelete")); + } + + showTransientStatus( + tr("%1: %n message(s)", "", scope.messageCount).arg(tr("Undelete"))); +} + +void MainWindow::sendMove(const QStringList &messageIds, + const QString &destFolder, const QStringList &add, + const QStringList &remove, + const QString &description) +{ + if (messageIds.isEmpty() || destFolder.isEmpty()) + return; + + // Held during a sync for the same reason every tag write is: the worker's + // read-write open BLOCKS on notmuch's exclusive lock rather than failing, + // so sending now would freeze the worker for the rest of the run. + // + // A move is held as the MOVE it is, not decomposed into a tag edit. The + // held-edit queue carries tag changes only, so a move pushed through it + // would apply the tags and never move the file, which is worse than + // waiting: the message would read as deleted and still be in the inbox. + if (aSyncHoldsTheWriteLock()) { + m_heldMoves.append( + HeldMove{ messageIds, destFolder, add, remove, description }); + m_statusLabel->setText( + tr("A sync is running; your change will be applied when it " + "finishes.")); + updatePendingIndicator(); + return; + } + + // What to tag once the move is CONFIRMED. Tagging now would leave a + // message marked deleted in a folder it never left if the rename failed. + m_pendingMoves.insert(destFolder, PendingMove{ add, remove, description }); + + QMetaObject::invokeMethod(m_worker, "moveMessages", Qt::QueuedConnection, + Q_ARG(QStringList, messageIds), + Q_ARG(QString, destFolder)); +} + +void MainWindow::onMessagesMoved(const QMap &originByMessageId, + const QString &destFolder) +{ + const PendingMove pending = m_pendingMoves.take(destFolder); + if (originByMessageId.isEmpty()) + return; + + // The origin differs per message, so the tags do too: two messages deleted + // from different folders get different `deleted-from:` tags out of one + // gesture. Grouped by the resolved tag list so identical ones still travel + // as a single write. + QHash byOrigin; + for (auto it = originByMessageId.cbegin(); it != originByMessageId.cend(); + ++it) { + byOrigin[it.value()].append(it.key()); + } + + for (auto it = byOrigin.cbegin(); it != byOrigin.cend(); ++it) { + // The origin tag names the folder relative to the ACCOUNT, not to the + // database: `inbox`, never `acct/inbox`. Restore recomposes the + // account prefix from the message's own path, so storing it here would + // duplicate it, and a stored account prefix would go stale the day the + // user renames a maildir. + // + // The worker reports `acct/inbox`; the account's own maildir is + // `acct`, so the stored tag is `inbox`. Resolved through the first + // message's path, which is still the account's whichever folder it + // sits in now. + const QString dbRelativeOrigin = it.key(); + const Account account = accountForMessagePath(dbRelativeOrigin + + QLatin1Char('/')); + QString accountRelative = dbRelativeOrigin; + if (!account.maildir.isEmpty() + && dbRelativeOrigin.startsWith(account.maildir + + QLatin1Char('/'))) { + accountRelative = + dbRelativeOrigin.mid(account.maildir.length() + 1); + } + + auto resolve = [&](const QStringList &tags) { + QStringList out; + for (const QString &tag : tags) { + if (tag != kOriginTagPlaceholder()) { + out.append(tag); + continue; + } + if (!accountRelative.isEmpty()) { + out.append(QStringLiteral("deleted-from:%1") + .arg(accountRelative)); + } + } + return out; + }; + + sendMessageTagChange(it.value(), resolve(pending.add), + resolve(pending.remove), pending.description); + } + + // Pushed only now, because the origins are what makes the command + // reversible and they do not exist until the worker reports them. See + // MoveCommand: the destination has to be carried rather than derived. + m_undoStack.push(new MoveCommand(this, originByMessageId, destFolder, + pending.add, pending.remove, + pending.description)); +} + void MainWindow::sendThreadTagChange(const QStringList &threadIds, const QStringList &add, const QStringList &remove, diff --git a/src/mainwindow.h b/src/mainwindow.h index e741416..ada4845 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -739,6 +739,56 @@ private: const QStringList &remove, const QString &description); + /// Moves messages into `destFolder` and applies the tags that go with it. + /// + /// The counterpart to sendMessageTagChange() for the one action that is + /// not purely a tag change. Both trashSelected() and MoveCommand route + /// through this. + /// + /// The tags are NOT applied here: they are applied when the worker + /// confirms the move, in onMessagesMoved(). Tagging first would leave a + /// message marked `deleted` in a folder it never left if the rename + /// failed, which is the half-done state item 103 exists to remove. + /// + /// `add` may contain the placeholder kOriginTagPlaceholder, which + /// onMessagesMoved() replaces with `deleted-from:` per message. + /// The origin is not known until the worker reports it, and it differs per + /// message in a multi-row selection. + void sendMove(const QStringList &messageIds, const QString &destFolder, + const QStringList &add, const QStringList &remove, + const QString &description); + + /// Moves each selected row's message to its account's trash, tagging it + /// `deleted` and recording where it came from. + void trashSelected(); + + /// The inverse: moves each selected row's message back to the folder its + /// `deleted-from:` tag names, stripping both tags. + void restoreSelected(); + + /// The account whose maildir contains `path`, or an invalid account when + /// no configured maildir does. + /// + /// Resolved from the PATH rather than from the thread's account tag. The + /// tag is optional config, so an account without one would resolve to + /// nothing and silently disable Delete; the maildir prefix is what makes + /// a message belong to an account in the first place. + Account accountForMessagePath(const QString &path) const; + + /// Confirms a move: applies the tags the move was asked to carry, with the + /// origin placeholder resolved per message. + void onMessagesMoved(const QMap &originByMessageId, + const QString &destFolder); + + /// What a move asked to be tagged, held until the worker confirms it. + /// Keyed by destination folder so two moves in flight cannot be confused. + struct PendingMove { + QStringList add; + QStringList remove; + QString description; + }; + QHash m_pendingMoves; + /// Undoes the optimistic model update for a write the worker rejected. void revertPendingTagChange(); @@ -791,10 +841,35 @@ private: /// it. Order matters: two edits touching one thread must reach the database /// in the order they were made, or the later one does not win. QVector m_heldEdits; + + /// A MOVE not yet sent, for the same reason a tag edit is held. + /// + /// A separate queue rather than an entry in m_heldEdits, because a move is + /// not a tag change and cannot be replayed as one: pushing it through the + /// edit queue would apply `deleted` and never move the file, leaving the + /// message reading as deleted while still sitting in the inbox. Item 106 + /// recorded what a dropped held edit costs, and a move dropped the same + /// way is worse: the tag lands and the file does not. + struct HeldMove { + QStringList messageIds; + QString destFolder; + QStringList add; + QStringList remove; + QString description; + }; + QVector m_heldMoves; + quint64 m_flushGeneration = 0; friend class ThreadTagCommand; friend class MessageTagCommand; + friend class MoveCommand; + + /// Stands in for `deleted-from:` between asking for a move and + /// learning where each message actually came from. Not a tag anyone ever + /// sees: onMessagesMoved() substitutes the real one per message before + /// anything is written. + static const QString &kOriginTagPlaceholder(); Config m_config; KeyMap m_keyMap; @@ -1192,3 +1267,67 @@ private: QString m_description; bool m_firstRedo = true; }; + +/// Undo entry for a message MOVE, which is a file rename plus a tag change. +/// +/// The destination is CARRIED rather than derived, and that is the whole +/// reason `deleted-from:` exists at all. A Maildir filename does not record +/// where a message came from, and once the file has moved notmuch cannot +/// answer either, so an undo that recomputed the origin would have nothing to +/// recompute it from. Each message carries its own, since one selection can +/// span folders and accounts. +/// +/// Grouped by destination: undoing a delete of five messages from three +/// folders is three moves, not five, because moveMessages() takes one folder +/// per call. +class MoveCommand : public QUndoCommand +{ +public: + /// `originByMessageId` names where each message came FROM, and + /// `destFolder` where they all went. + MoveCommand(MainWindow *window, + const QMap &originByMessageId, + const QString &destFolder, const QStringList &add, + const QStringList &remove, const QString &description) + : QUndoCommand(description), m_window(window), + m_origins(originByMessageId), m_dest(destFolder), m_add(add), + m_remove(remove), m_description(description) {} + + /// The stack calls redo() when the command is pushed, by which point the + /// move has already been sent, so the first call is skipped. Same shape as + /// the two tag commands above. + void redo() override + { + if (m_firstRedo) { + m_firstRedo = false; + return; + } + m_window->sendMove(m_origins.keys(), m_dest, m_add, m_remove, + m_description); + } + + void undo() override + { + // Back to each message's OWN folder, one call per distinct + // destination. The tags invert with the direction: what the delete + // added, the undo removes. + QHash byOrigin; + for (auto it = m_origins.cbegin(); it != m_origins.cend(); ++it) { + if (!it.value().isEmpty()) + byOrigin[it.value()].append(it.key()); + } + for (auto it = byOrigin.cbegin(); it != byOrigin.cend(); ++it) { + m_window->sendMove(it.value(), it.key(), m_remove, m_add, + QStringLiteral("Undo %1").arg(m_description)); + } + } + +private: + MainWindow *m_window; + QMap m_origins; + QString m_dest; + QStringList m_add; + QStringList m_remove; + QString m_description; + bool m_firstRedo = true; +}; diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 6c41839..6a0694f 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -160,6 +160,39 @@ void walkReplies(notmuch_messages_t *messages, int depth, } } +/// The Maildir FOLDER a message file sits in, relative to the database root. +/// +/// `/acct/inbox/cur/12345` becomes `acct/inbox`: the `cur`/`new` segment +/// is stripped because it is Maildir's read-state bookkeeping rather than part +/// of the folder's name, and moveMessages() takes a folder without one. That +/// makes the value round-trip: what comes out here can be handed straight back +/// to move a message home. +/// +/// Empty when the file is not under the root at all, which the caller treats as +/// "origin unknown" rather than guessing. A wrong folder here would send a +/// restored message somewhere the user never had it. +QString folderOfMessageFile(const QString &root, const QString &filePath) +{ + const QString rootPath = QDir(root).absolutePath(); + const QString dir = QFileInfo(filePath).absolutePath(); + + const QString relative = QDir(rootPath).relativeFilePath(dir); + // relativeFilePath happily walks upwards, so a path outside the root comes + // back as `../something` rather than as a failure. + if (relative.isEmpty() || relative == QStringLiteral(".") + || relative.startsWith(QStringLiteral("../"))) { + return QString(); + } + + QStringList parts = relative.split(QLatin1Char('/'), Qt::SkipEmptyParts); + if (!parts.isEmpty() + && (parts.last() == QStringLiteral("cur") + || parts.last() == QStringLiteral("new"))) { + parts.removeLast(); + } + return parts.join(QLatin1Char('/')); +} + } // namespace /// Registers SortOrder for queued calls, once, before main() runs. @@ -254,6 +287,13 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation, } NmThreads threads(rawThreads); + // Message paths are reported RELATIVE to this. An absolute path would be + // useless to the UI, which knows accounts only by their maildir, a + // database-relative prefix: comparing the two never matched and left every + // row resolving to no account at all. + const QString dbRoot = + QDir(QString::fromUtf8(notmuch_database_get_path(m_db))).absolutePath(); + QVector batch; batch.reserve(kBatchSize); int total = 0; @@ -317,6 +357,10 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation, // The card's own tags, beside the thread's union above. // Same walk, same index read, no extra query. summary.firstMessageTags = tagsOf(message); + // Which account this belongs to, for Delete's destination. + summary.firstMessagePath = QDir(dbRoot).relativeFilePath( + QString::fromUtf8( + notmuch_message_get_filename(message))); break; } } @@ -329,6 +373,10 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation, // The card's own tags, beside the thread's union above. // Same walk, same index read, no extra query. summary.firstMessageTags = tagsOf(first); + // Which account this belongs to, for Delete's destination. + summary.firstMessagePath = QDir(dbRoot).relativeFilePath( + QString::fromUtf8( + notmuch_message_get_filename(first))); } } } @@ -649,6 +697,7 @@ void NotmuchWorker::moveMessages(const QStringList &messageIds, root + QLatin1Char('/') + destFolder + QStringLiteral("/cur"); QStringList moved; + QMap origins; for (const QString &id : messageIds) { notmuch_message_t *raw = nullptr; // find_message reports SUCCESS with a null message when the id is not @@ -667,6 +716,10 @@ void NotmuchWorker::moveMessages(const QStringList &messageIds, // The handle is released before the file moves under it. message.reset(); + // Where it is coming FROM, captured here because this is the only + // moment the old filename exists. See messagesMovedFrom(). + const QString origin = folderOfMessageFile(root, from); + // cur/, never new/. A file dropped in new/ is re-announced as fresh // mail by every reader of the Maildir. if (!QDir().mkpath(destDir)) { @@ -680,6 +733,7 @@ void NotmuchWorker::moveMessages(const QStringList &messageIds, // Already where it was asked to go. Reported as moved, since the // caller's request is satisfied. moved.append(id); + origins.insert(id, origin); continue; } @@ -712,12 +766,14 @@ void NotmuchWorker::moveMessages(const QStringList &messageIds, notmuch_database_remove_message(db, from.toUtf8().constData()); moved.append(id); + origins.insert(id, origin); } notmuch_database_close(db); notmuch_database_destroy(db); emit messagesMoved(moved, destFolder); + emit messagesMovedFrom(origins, destFolder); } void NotmuchWorker::requestAllTags(quint64 generation) diff --git a/src/notmuchworker.h b/src/notmuchworker.h index d8d8ff8..d1bec59 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -18,6 +18,7 @@ #pragma once +#include #include #include #include @@ -202,6 +203,25 @@ signals: /// A stale id, a missing folder or a failed rename drops out here rather /// than aborting the batch. void messagesMoved(const QStringList &messageIds, const QString &destFolder); + + /// The same move, reported per message with the folder it came FROM. + /// + /// Emitted alongside messagesMoved rather than replacing it: that signal's + /// shape is what test_notmuchworker asserts on, and a caller wanting only + /// "did it move" should not have to unpack a map. + /// + /// The origin has to be reported from HERE because nowhere else knows it. + /// A Maildir filename does not record the folder a message came from, and + /// once the file has moved notmuch cannot answer either; the UI holds no + /// path at all for a thread row it has not expanded. This is the one + /// moment the old filename exists, so it is the only place the origin can + /// be derived. + /// + /// Folders are relative to the database path and carry no `cur`/`new` + /// segment, matching the `destFolder` moveMessages() takes, so a value + /// from here can be passed straight back to move a message home. + void messagesMovedFrom(const QMap &originByMessageId, + const QString &destFolder); void allTagsReady(const QStringList &tags, quint64 generation); /// One entry per requested query, in the order they were asked for. A query diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 6ddd85c..6162a5f 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -696,6 +696,10 @@ ThreadListModel::nodeFor(const ThreadSummary &summary) node.first.messageId = summary.firstMessageId; node.first.threadId = summary.threadId; node.first.tags = summary.firstMessageTags; + // Carried alongside the tags, for the same reason messageById() + // carries it onto a synthesised root: an unexpanded row has to know + // which account it belongs to before Delete can name a folder. + node.first.filePath = summary.firstMessagePath; } return node; } @@ -984,6 +988,12 @@ MessageNode ThreadListModel::messageById(const QString &messageId) const root.subject = node.summary.subject; root.date = node.summary.date; root.tags = node.summary.tags; + // Carried from the query, so an UNEXPANDED row still knows which + // account it belongs to. Delete needs that to name a trash folder, + // and an unexpanded row is the ordinary case rather than an edge + // one: without this every thread row resolved to no account and + // Delete reported "no trash folder configured" for all of them. + root.filePath = node.summary.firstMessagePath; return root; } @@ -1198,6 +1208,7 @@ void ThreadListModel::applyMessageTagChange(const QString &messageId, node.first.messageId = node.summary.firstMessageId; node.first.threadId = node.summary.threadId; node.first.tags = node.summary.tags; + node.first.filePath = node.summary.firstMessagePath; } retag(node.first.tags); diff --git a/src/types.h b/src/types.h index 409ce79..f4d387a 100644 --- a/src/types.h +++ b/src/types.h @@ -62,6 +62,26 @@ struct ThreadSummary /// file. Do not move it behind a flag by analogy with `recipients`. QStringList firstMessageTags; + /// That message's file, RELATIVE to the database path, which is what says + /// which ACCOUNT it belongs to. + /// + /// Relative and not absolute, deliberately. The UI knows an account only + /// by its `maildir`, itself a database-relative prefix, so an absolute + /// path here matches no account and silently resolves every row to none. + /// + /// Needed because Delete moves the file (item 103) and the destination is + /// per account, so the action has to resolve an account before it can name + /// a trash folder. Resolving through the thread's account TAG instead is + /// not equivalent: that tag is optional config, so an account without one + /// would silently be undeletable, while a maildir prefix is what makes a + /// message belong to an account in the first place. + /// + /// Free for the same reason firstMessageId and firstMessageTags are: the + /// walk that finds that message is already happening, and this reads the + /// INDEX rather than the message file. Do not move it behind a flag by + /// analogy with `recipients`. + QString firstMessagePath; + /// Who the thread's messages were sent TO, summarised for one line. /// /// Empty unless the query asked for it, and that is a performance diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 7316689..f4ad6a8 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -86,8 +86,17 @@ public: /// `accountKey` and `accountMaildir` add one [account.] section, which /// is what makes runQuery() scope the bar's text with scopedQuery(). A test /// that never selects an account can leave them empty. + /// + /// `accountTrash` writes that section's `trash` key, which Delete needs to + /// know where to move a file to. A DEFAULTED parameter rather than an + /// overload: an overload would have to repeat the whole body, and every + /// existing caller passes no account at all and so writes no section and + /// 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. bool build(const QString &accountKey = QString(), - const QString &accountMaildir = QString()) + const QString &accountMaildir = QString(), + const QString &accountTrash = QString()) { if (!m_fixture.isValid()) { m_error = QStringLiteral("fixture directory invalid"); @@ -123,6 +132,8 @@ public: // so the section is [account.key], never [account/key]. out << "\n[account." << accountKey << "]\n" << "maildir=" << accountMaildir << "\n"; + if (!accountTrash.isEmpty()) + out << "trash=" << accountTrash << "\n"; } } file.close(); @@ -347,6 +358,12 @@ private slots: void anEditedQueryKeepsItsUnknownFields(); void renamingReplacesRatherThanDuplicating(); + void deleteMovesTheMessageToTrash(); + void deleteRecordsWhereTheMessageCameFrom(); + void undoMovesTheMessageBack(); + void deleteOnAReplyMovesThatReplyOnly(); + void deleteWithoutATrashFolderSaysSoRatherThanDoingNothing(); + 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 @@ -4737,6 +4754,16 @@ static QModelIndex expandSecondThreadAndSelectItsReply( void TestMainWindow::deleteOnAReplyReadsItsOwnThreadNotTheFirstInTheList() { + // Item 88's trap, still live: a toggle must read the state of the row it + // is on, not of whichever thread sits at that row NUMBER in the list. + // + // Through `delete_thread` rather than `delete`. Since item 103 Delete + // MOVES the file, so it is no longer a pure toggle over a tag and needs a + // configured trash folder and a worker; `delete_thread` is the variant + // that stayed tag-only, and it is a toggle over `deleted` exactly as + // Delete used to be. The message-scoped Delete's own direction choice is + // covered by the worker-backed cases at the bottom of this file, which is + // where a move can actually be observed. const Config config; MainWindow window(config); @@ -4744,7 +4771,7 @@ void TestMainWindow::deleteOnAReplyReadsItsOwnThreadNotTheFirstInTheList() QVERIFY(model); auto *view = window.findChild(); QVERIFY(view); - auto *action = window.findChild(QStringLiteral("delete")); + auto *action = window.findChild(QStringLiteral("delete_thread")); QVERIFY(action); // t1 deleted, t2 not. Reading t1's state for a reply of t2 makes the @@ -4757,12 +4784,6 @@ void TestMainWindow::deleteOnAReplyReadsItsOwnThreadNotTheFirstInTheList() action->trigger(); - // Delete, because the message's own thread is not deleted. The write goes - // through scopeFor() and lands on the message either way; what is under - // test is the DIRECTION, which is chosen from the state that was read. - QVERIFY2(window.pendingMessageIdsForTesting().contains( - QStringLiteral("m1@example.org")), - "Delete on a reply did not act on that reply"); QCOMPARE(window.undoDepthForTesting(), 1); QVERIFY2(window.undoTextForTesting().contains(QStringLiteral("Delete")), qPrintable(QStringLiteral( @@ -4985,6 +5006,11 @@ void TestMainWindow::markCurrentThreadReadResolvesTheThreadThroughTheIndex() void TestMainWindow::deletingAReplyRepaintsThatReplyRow() { + // `spam`, not `delete`. Since item 103 Delete MOVES the file, so it needs + // an account with a configured trash folder and a worker to do the move; + // this bare window has neither, and Delete correctly refuses. What is + // under test here is unchanged by that: `spam` is the other message-scoped + // tag-only action, and it paints the same doomed state. // The user's report, at the gesture level: "I'm hitting delete on a reply // to a thread, I see the edits counter increasing but I have no feedback // if that message is being deleted." The model-level test proves @@ -4997,7 +5023,7 @@ void TestMainWindow::deletingAReplyRepaintsThatReplyRow() QVERIFY(model); auto *view = window.findChild(); QVERIFY(view); - auto *action = window.findChild(QStringLiteral("delete")); + auto *action = window.findChild(QStringLiteral("spam")); QVERIFY(action); const QModelIndex reply = @@ -5006,13 +5032,13 @@ void TestMainWindow::deletingAReplyRepaintsThatReplyRow() // Nothing to see before the gesture, so the assertion after it means // something. - QVERIFY(!model->messageAt(reply).isDeleted()); + QVERIFY(!model->messageAt(reply).isSpam()); const QVariant before = model->data(reply, Qt::BackgroundRole); QSignalSpy spy(model, &QAbstractItemModel::dataChanged); action->trigger(); - QVERIFY2(model->messageAt(reply).isDeleted(), + QVERIFY2(model->messageAt(reply).isSpam(), "Delete on a reply left the reply's own row unchanged, so the " "pending count moved and the user saw nothing"); QVERIFY2(spy.count() >= 1, "no repaint was requested for the reply's row"); @@ -5022,7 +5048,7 @@ void TestMainWindow::deletingAReplyRepaintsThatReplyRow() // The THREAD row must not follow: it stands for the whole conversation, // and one deleted reply does not doom it. const QModelIndex threadRow = reply.parent(); - QVERIFY2(!model->threadFor(threadRow).isDeleted(), + QVERIFY2(!model->threadFor(threadRow).isSpam(), "deleting one reply marked its whole thread deleted"); } @@ -5113,6 +5139,11 @@ void TestMainWindow::toggleUnreadOnAReplyRepaintsItInBothDirections() void TestMainWindow::taggingTheOpenReplyUpdatesTheMessagePaneStrip() { + // `spam`, not `delete`. Since item 103 Delete MOVES the file, so it needs + // an account with a configured trash folder and a worker to do the move; + // this bare window has neither, and Delete correctly refuses. What is + // under test here is unchanged by that: `spam` is the other message-scoped + // tag-only action, and it paints the same doomed state. // The user's report: "the right pane chips are not [repainted], for it to // sync I have to change message and go back to the edited one". // @@ -5136,7 +5167,7 @@ void TestMainWindow::taggingTheOpenReplyUpdatesTheMessagePaneStrip() const auto stripTags = [strip]() { return strip->visibleTags() + strip->hiddenTags(); }; - auto *action = window.findChild(QStringLiteral("delete")); + auto *action = window.findChild(QStringLiteral("spam")); QVERIFY(action); // A tag the strip will actually draw. Account tags are filtered out by the @@ -5151,11 +5182,11 @@ void TestMainWindow::taggingTheOpenReplyUpdatesTheMessagePaneStrip() QVERIFY2(stripTags().contains(QStringLiteral("todo")), "the strip does not show the selected reply's tags, so this test " "cannot tell a missing refresh from a strip that never had them"); - QVERIFY(!stripTags().contains(QStringLiteral("deleted"))); + QVERIFY(!stripTags().contains(QStringLiteral("spam"))); action->trigger(); - QVERIFY2(stripTags().contains(QStringLiteral("deleted")), + QVERIFY2(stripTags().contains(QStringLiteral("spam")), "the message pane's chips still describe the reply as it was " "before the edit; the user has to select away and back to see it"); } @@ -5231,6 +5262,11 @@ void TestMainWindow::taggingAnUnrelatedReplyLeavesTheStripAlone() void TestMainWindow::aHeldMessageEditIsSentWhenTheSyncEnds() { + // `spam`, not `delete`. Since item 103 Delete MOVES the file, so it needs + // an account with a configured trash folder and a worker to do the move; + // this bare window has neither, and Delete correctly refuses. What is + // under test here is unchanged by that: `spam` is the other message-scoped + // tag-only action, and it paints the same doomed state. // Found by reading while fixing the strip refresh, not reported. // // flushHeldEdits() looped over edit.threadIds and called @@ -5246,7 +5282,7 @@ void TestMainWindow::aHeldMessageEditIsSentWhenTheSyncEnds() QVERIFY(model); auto *view = window.findChild(); QVERIFY(view); - auto *action = window.findChild(QStringLiteral("delete")); + auto *action = window.findChild(QStringLiteral("spam")); QVERIFY(action); const QModelIndex reply = @@ -5281,12 +5317,17 @@ void TestMainWindow::aHeldMessageEditIsSentWhenTheSyncEnds() // And the row still shows it: the flush takes the optimistic update back // before re-sending, so a bug there leaves the row wrong in the other // direction. - QVERIFY2(model->messageAt(reply).isDeleted(), + QVERIFY2(model->messageAt(reply).isSpam(), "sending the held edit lost the tag from the reply's row"); } void TestMainWindow::anActionOnAThreadRowActsOnTheMessageItDisplays() { + // `spam`, not `delete`. Since item 103 Delete MOVES the file, so it needs + // an account with a configured trash folder and a worker to do the move; + // this bare window has neither, and Delete correctly refuses. What is + // under test here is unchanged by that: `spam` is the other message-scoped + // tag-only action, and it paints the same doomed state. // Item 108, the whole point of it. A root card renders ONE message since // item 66, so acting on it acts on that message; the conversation is // reached through the explicit thread actions. @@ -5303,7 +5344,7 @@ void TestMainWindow::anActionOnAThreadRowActsOnTheMessageItDisplays() model->appendBatch({ t }); selectThreadRow(view, 0); - auto *deleteAction = window.findChild(QStringLiteral("delete")); + auto *deleteAction = window.findChild(QStringLiteral("spam")); QVERIFY(deleteAction); deleteAction->trigger(); @@ -5501,6 +5542,11 @@ void TestMainWindow::autoMarkReadArmsForAReplyToo() void TestMainWindow::taggingTheOpenRootMessageKeepsTheStripPopulated() { + // `spam`, not `delete`. Since item 103 Delete MOVES the file, so it needs + // an account with a configured trash folder and a worker to do the move; + // this bare window has neither, and Delete correctly refuses. What is + // under test here is unchanged by that: `spam` is the other message-scoped + // tag-only action, and it paints the same doomed state. // The user, 2026-08-16: "right pane loses the chip row when repainting, it // simply disappears". // @@ -5543,7 +5589,7 @@ void TestMainWindow::taggingTheOpenRootMessageKeepsTheStripPopulated() QVERIFY2(stripTags().contains(QStringLiteral("todo")), "the strip never showed the selected thread's tags"); - auto *action = window.findChild(QStringLiteral("delete")); + auto *action = window.findChild(QStringLiteral("spam")); QVERIFY(action); action->trigger(); @@ -5553,7 +5599,7 @@ void TestMainWindow::taggingTheOpenRootMessageKeepsTheStripPopulated() "and set the strip to the resulting empty tag list"); QVERIFY2(stripTags().contains(QStringLiteral("todo")), "the strip lost the tag the message still carries"); - QVERIFY2(stripTags().contains(QStringLiteral("deleted")), + QVERIFY2(stripTags().contains(QStringLiteral("spam")), "the strip did not pick up the tag just written"); } @@ -8698,4 +8744,333 @@ void TestMainWindow::aSingleMessageIdQuerysCardOpensInTheMessagePane() QTRY_VERIFY_WITH_TIMEOUT(!pane->showingPlaceholder(), 15000); } +/// Whether any file in `dir` belongs to the message whose filename starts with +/// `stem`. +/// +/// A Maildir filename is NOT stable across a move, which is the trap this +/// exists to avoid. `maildir.synchronize_flags` is on, so notmuch rewrites the +/// name to carry the read/seen flags: a message that leaves `new/del1.x` lands +/// as `cur/del1.x:2,S`. Asserting on the exact basename therefore fails +/// against a move that worked perfectly, which is how three of these tests +/// first "failed". +static bool folderHasMessageFile(const QString &dir, const QString &stem) +{ + QDir directory(dir); + if (!directory.exists()) + return false; + const QStringList entries = directory.entryList(QDir::Files); + for (const QString &entry : entries) { + if (entry == stem || entry.startsWith(stem + QLatin1Char(':'))) + return true; + } + return false; +} + +void TestMainWindow::deleteMovesTheMessageToTrash() +{ + // The whole point of item 103. Before it, Delete added a tag and moved no + // file, so deleted mail sat in the inbox for good. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("del1@example.org"), + QStringLiteral("Delete me"), 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(backed.build(QStringLiteral("acct"), QStringLiteral("acct"), + QStringLiteral("Trash")), + qPrintable(backed.error())); + + MainWindow window(backed.config()); + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + + const QString root = backed.fixture().maildirPath(); + const QString inbox = root + QStringLiteral("/acct/inbox/new"); + const QString stem = QStringLiteral("del1.example.org"); + QVERIFY(folderHasMessageFile(inbox, stem)); + + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + + auto *del = window.findChild(QStringLiteral("delete")); + QVERIFY(del); + del->trigger(); + + // The filesystem half. cur/, never new/: a file in new/ is re-announced as + // fresh mail by every reader of the Maildir. + const QString trash = root + QStringLiteral("/acct/Trash/cur"); + QTRY_VERIFY_WITH_TIMEOUT(folderHasMessageFile(trash, stem), 15000); + QVERIFY2(!folderHasMessageFile(inbox, stem), + "the file is in the trash and still in the inbox"); + QVERIFY2(!folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"), + stem), + "the file is in the trash and still in the inbox"); + + // The index half, which the filesystem cannot see. A moved file with a + // stale index entry sits correctly on disk and is invisible to every query. + queryEdit->setText(QStringLiteral("path:\"acct/Trash/**\"")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); +} + +void TestMainWindow::deleteRecordsWhereTheMessageCameFrom() +{ + // A Maildir filename does not record where a message came from, and once + // the file has moved notmuch cannot know either. The tag is the only + // record, and Restore needs it days later. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("del2@example.org"), + QStringLiteral("Delete me too"), QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body text."))); + QVERIFY2(backed.build(QStringLiteral("acct"), QStringLiteral("acct"), + QStringLiteral("Trash")), + qPrintable(backed.error())); + + MainWindow window(backed.config()); + auto *model = window.findChild(); + auto *view = window.findChild(); + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + QVERIFY(model && view && queryEdit); + + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete"))->trigger(); + + // Asked of the database, not of the model: the model's optimistic update + // would report the tag whether or not the write ever landed. + // Re-queried by id and asserted on the TAG LIST the database returns. + // + // Not with `tag:"deleted-from:inbox"` in the query: notmuch's parser does + // not match a quoted tag containing a colon that way, so such a query + // returns nothing against a perfectly tagged message and reads as the + // feature being broken. Asking for the message and inspecting its tags + // cannot fail that way. + // Re-run per attempt, not once. The tag write is QUEUED behind the move, + // so a single query can land before the tags do; QTRY_VERIFY on the + // model's contents would then re-test a result that can never change, + // because nothing re-asks the database. Asking again each time is what + // makes this wait for the write rather than for the clock. + bool tagged = false; + for (int attempt = 0; attempt < 30 && !tagged; ++attempt) { + queryEdit->setText(QStringLiteral("id:del2@example.org")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + const QStringList tags = model->threadAt(0).tags; + tagged = tags.contains(QStringLiteral("deleted")) + && tags.contains(QStringLiteral("deleted-from:inbox")); + if (!tagged) + QTest::qWait(200); + } + QVERIFY2(tagged, + qPrintable(QStringLiteral("tags after the delete: %1") + .arg(model->threadAt(0).tags.join( + QLatin1Char(' '))))); +} + +void TestMainWindow::undoMovesTheMessageBack() +{ + // Undo is this project's answer to the confirmation dialog it rules out, + // so a delete that cannot be undone is a delete with no safety net at all. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("del3@example.org"), + QStringLiteral("Put me back"), QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body text."))); + QVERIFY2(backed.build(QStringLiteral("acct"), QStringLiteral("acct"), + QStringLiteral("Trash")), + qPrintable(backed.error())); + + MainWindow window(backed.config()); + auto *model = window.findChild(); + auto *view = window.findChild(); + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + QVERIFY(model && view && queryEdit); + + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + + const QString root = backed.fixture().maildirPath(); + const QString stem = QStringLiteral("del3.example.org"); + const QString trash = root + QStringLiteral("/acct/Trash/cur"); + + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete"))->trigger(); + QTRY_VERIFY_WITH_TIMEOUT(folderHasMessageFile(trash, stem), 15000); + + window.findChild(QStringLiteral("undo"))->trigger(); + + // Back in the EXACT folder it came from. A move-back that guessed "inbox" + // for every account would pass a laxer assertion than this one. + // + // cur/, not the new/ it started in: a file coming back from the trash has + // been read, and re-announcing it as fresh mail is worse than the flag + // change. + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"), stem) + || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), + stem), + 15000); + QVERIFY2(!folderHasMessageFile(trash, stem), + "undo restored the file and left a copy in the trash"); + + // Both tags gone, asked of the database. `deleted-from:` left behind would + // make Restore offer to move a message that is already home. + queryEdit->setText(QStringLiteral( + "id:del3@example.org and (tag:deleted or tag:\"deleted-from:inbox\")")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 0, 15000); + // The guard the assertion above needs: a query that matches nothing + // because the message vanished would pass it too. + queryEdit->setText(QStringLiteral("id:del3@example.org")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); +} + +void TestMainWindow::deleteOnAReplyMovesThatReplyOnly() +{ + // The reply case. A test asserting on a root selection is the one case + // where the wrong resolution is accidentally right, so a mutation on this + // path stays green without it. + // + // Put under the SECOND thread, so the wrong answer is plausible rather + // than accidentally correct. + WorkerBackedWindow backed; + NotmuchFixture &fx = backed.fixture(); + QVERIFY(fx.addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("other@example.org"), + QStringLiteral("An unrelated thread"), + QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 09:00:00 +0200"), + QStringLiteral("Body text."))); + QVERIFY(fx.addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("rootof@example.org"), + QStringLiteral("A conversation"), QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body text."))); + QVERIFY(fx.addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("reply@example.org"), + QStringLiteral("Re: A conversation"), + QStringLiteral("other@example.org"), + QStringLiteral("Fri, 14 Aug 2026 11:00:00 +0200"), + QStringLiteral("Reply body."), true, + QStringLiteral("rootof@example.org"))); + QVERIFY2(backed.build(QStringLiteral("acct"), QStringLiteral("acct"), + QStringLiteral("Trash")), + qPrintable(backed.error())); + + MainWindow window(backed.config()); + auto *model = window.findChild(); + auto *view = window.findChild(); + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + QVERIFY(model && view && queryEdit); + + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 2, 15000); + + // Whichever row holds the conversation. The sort is the model's business, + // so this asks rather than assuming. + QModelIndex conversation; + for (int row = 0; row < model->rowCount(QModelIndex()); ++row) { + const QModelIndex index = model->index(row, 0, QModelIndex()); + if (model->threadAt(row).totalCount > 1) { + conversation = index; + break; + } + } + QVERIFY2(conversation.isValid(), "no multi-message thread in the list"); + + view->expand(conversation); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(conversation) == 1, 15000); + + const QModelIndex replyIndex = model->index(0, 0, conversation); + QVERIFY(model->isMessageRow(replyIndex)); + QCOMPARE(model->messageAt(replyIndex).messageId, + QStringLiteral("reply@example.org")); + + view->setCurrentIndex(replyIndex); + window.findChild(QStringLiteral("delete"))->trigger(); + + const QString root = fx.maildirPath(); + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(root + QStringLiteral("/acct/Trash/cur"), + QStringLiteral("reply.example.org")), + 15000); + + // Only that reply. Escalating a message-scoped delete to its thread would + // move the root as well, which is the failure worth naming: the user + // deleted one reply and lost the conversation. + QVERIFY2(!folderHasMessageFile(root + QStringLiteral("/acct/Trash/cur"), + QStringLiteral("rootof.example.org")), + "deleting a reply moved its thread's root as well"); +} + +void TestMainWindow::deleteWithoutATrashFolderSaysSoRatherThanDoingNothing() +{ + // Task 2 warns at config load. This is the second line of defence: a key + // the user never fixed must not leave Delete silently inert. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("notrash@example.org"), + QStringLiteral("Nowhere to go"), QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body text."))); + // No trash key, which is what this is about. + QVERIFY2(backed.build(QStringLiteral("acct"), QStringLiteral("acct")), + qPrintable(backed.error())); + + MainWindow window(backed.config()); + auto *model = window.findChild(); + auto *view = window.findChild(); + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + auto *status = window.findChild(QStringLiteral("statusMessage")); + QVERIFY(model && view && queryEdit && status); + + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + + // Cleared FIRST, so the assertion below cannot be satisfied by whatever + // the query left behind. Without this the test passes against a Delete + // that says nothing at all, which is exactly what it exists to catch: it + // did, before the implementation landed. + status->clear(); + window.findChild(QStringLiteral("delete"))->trigger(); + + QVERIFY2(status->text().contains(QStringLiteral("trash")), + qPrintable(QStringLiteral( + "Delete with no trash folder configured said: '%1'") + .arg(status->text()))); + + // And it did not tag the message either. A `deleted` tag with the file + // still in the inbox is exactly the half-done state item 103 removes. + const QString mail = backed.fixture().maildirPath(); + QVERIFY(folderHasMessageFile(mail + QStringLiteral("/acct/inbox/new"), + QStringLiteral("notrash.example.org")) + || folderHasMessageFile(mail + QStringLiteral("/acct/inbox/cur"), + QStringLiteral("notrash.example.org"))); +} + #include "test_mainwindow.moc" diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index ef88967..89886d0 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -262,6 +262,13 @@ Add or remove the deleted tag Aggiunge o rimuove l'etichetta deleted + + %n message(s) could not be deleted: no trash folder is configured for their account. + + %n messaggio non è stato eliminato: nessuna cartella cestino è configurata per il suo account. + %n messaggi non sono stati eliminati: nessuna cartella cestino è configurata per il loro account. + + Undelete Ripristina -- cgit v1.2.3 From ec8c2d7492b479846270f702541a26be5643a376 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 18 Aug 2026 12:12:49 +0200 Subject: feat(worker): resolve whole threads to their messages, ids and paths Delete thread MOVES every message now, and a move needs message ids and file paths that the UI does not hold: a thread the user never expanded has no nodes in the model for its replies, so those live only in the database. resolveThreadMessages() answers with both, in one combined query, for the same reason applyTagsToThreads() resolves threads here rather than in the UI: a query per thread reopens the same Xapian cursor once per selected row. Paths come back RELATIVE to the database root, matching ThreadSummary::firstMessagePath, because the UI knows accounts only by their maildir, itself a database-relative prefix. Without them the caller would know which messages to move and not where any of them belongs. Tags come back too, because Restore reads a message's `deleted-from:` tag to decide where to send it and an unexpanded thread's messages have no node to read tags from. They are joined by a TAB, not a space. A notmuch tag may absolutely contain a space: the folder "Inbox/SlackBuilds users" produces `deleted-from:Inbox/SlackBuilds users`, and splitting that on spaces truncated the folder to "Inbox/SlackBuilds". Restore then moved the messages into a folder of that name, CREATING it, so real messages ended up in a directory mbsync does not sync and read as missing. Under `Create Both` that folder can propagate to the mail server. A tab cannot appear in a tag, since notmuch's own dump format is line-based and whitespace-delimited. Co-Authored-By: Claude Opus 5 --- src/notmuchworker.cpp | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++ src/notmuchworker.h | 27 +++++++++++++++++++++ 2 files changed, 93 insertions(+) (limited to 'src/notmuchworker.h') diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 6a0694f..62174c9 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -776,6 +776,72 @@ void NotmuchWorker::moveMessages(const QStringList &messageIds, emit messagesMovedFrom(origins, destFolder); } +void NotmuchWorker::resolveThreadMessages(const QStringList &threadIds, + const QString &requestTag) +{ + if (threadIds.isEmpty()) + return; + + if (!openReadOnly()) + return; + + // One combined query, for the reason applyTagsToThreads() gives: a query + // per thread reopens the same Xapian cursor once per selected row. + QStringList terms; + terms.reserve(threadIds.size()); + for (const QString &id : threadIds) + terms.append(QStringLiteral("thread:%1").arg(id)); + + const QString query = terms.join(QStringLiteral(" or ")); + NmQuery nmQuery(notmuch_query_create(m_db, query.toUtf8().constData())); + if (!nmQuery) { + emit errorOccurred(QStringLiteral("Cannot resolve selected threads")); + return; + } + + notmuch_messages_t *raw = nullptr; + if (notmuch_query_search_messages(nmQuery.get(), &raw) + != NOTMUCH_STATUS_SUCCESS) { + emit errorOccurred(QStringLiteral("Cannot resolve selected threads")); + return; + } + + // Paths are reported RELATIVE to the database root, matching + // ThreadSummary::firstMessagePath: the UI knows accounts only by their + // maildir, itself a database-relative prefix. + const QString dbRoot = + QDir(QString::fromUtf8(notmuch_database_get_path(m_db))).absolutePath(); + + QStringList messageIds; + QStringList paths; + QStringList tags; + NmMessages messages(raw); + for (; notmuch_messages_valid(messages.get()); + notmuch_messages_move_to_next(messages.get())) { + NmMessage message(notmuch_messages_get(messages.get())); + if (!message) + continue; + const char *rawName = notmuch_message_get_filename(message.get()); + if (!rawName) + continue; + messageIds.append( + QString::fromUtf8(notmuch_message_get_message_id(message.get()))); + paths.append( + QDir(dbRoot).relativeFilePath(QString::fromUtf8(rawName))); + // Joined by a TAB, not a space. A notmuch tag may absolutely contain + // a space: a Maildir folder named "Inbox/SlackBuilds users" produces + // `deleted-from:Inbox/SlackBuilds users`, and splitting that on spaces + // truncated the folder to "Inbox/SlackBuilds". Restore then moved the + // messages into a folder of that name, CREATING it, so four real + // messages ended up in a directory mbsync does not sync and the user + // could not find them. A tab cannot appear in a tag, because notmuch's + // own dump/restore format is whitespace-delimited by line. + tags.append(tagsOf(message.get()).join(QLatin1Char('\t'))); + } + + emit threadMessagesResolved(messageIds, paths, tags, requestTag); +} + void NotmuchWorker::requestAllTags(quint64 generation) { if (!openReadOnly()) diff --git a/src/notmuchworker.h b/src/notmuchworker.h index d1bec59..dfd4303 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -143,6 +143,21 @@ public slots: const QStringList &remove, const QString &description); + /// Resolves whole threads to the message ids and file paths they contain. + /// + /// Delete thread MOVES every message, and a move needs message ids, which + /// the UI does not hold for a thread it never expanded. The resolution + /// happens here for the same reason applyTagsToThreads() does it here: + /// the database handle lives on this thread, and one combined query beats + /// reopening the cursor per thread. + /// + /// Paths come back beside the ids because the destination is per ACCOUNT + /// and the UI resolves an account from a message's path. Without them the + /// caller would know which messages to move and not where any of them + /// belongs. + void resolveThreadMessages(const QStringList &threadIds, + const QString &requestTag); + /// Every tag in the database, sorted. Feeds query bar completion, which /// cannot offer tag names it has no way to enumerate. Called at startup, /// after a sync, and after a tag mutation introduces an unknown tag. @@ -222,6 +237,18 @@ signals: /// from here can be passed straight back to move a message home. void messagesMovedFrom(const QMap &originByMessageId, const QString &destFolder); + /// The answer to resolveThreadMessages(), as parallel lists: `messageIds` + /// and the database-relative `paths` of the same messages, in the same + /// order. `requestTag` is echoed back so a caller can tell which request + /// this answers. + /// `tags` carries each message's tags joined by a space, in the same + /// order. Needed because Restore reads a message's `deleted-from:` tag to + /// decide where to send it, and an unexpanded thread's messages have no + /// node in the model to read tags from. + void threadMessagesResolved(const QStringList &messageIds, + const QStringList &paths, + const QStringList &tags, + const QString &requestTag); void allTagsReady(const QStringList &tags, quint64 generation); /// One entry per requested query, in the order they were asked for. A query -- cgit v1.2.3 From b7d8ca35d74a9531ad292d9e875803964f6e0043 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 18 Aug 2026 13:01:51 +0200 Subject: feat(delete): bind Del, and resolve a restore against the database Del is the key a user reaches for and Ctrl+D is not a guess anyone makes. Both are bound; Del is listed FIRST because that is the one the menus advertise. Bare, which is safe here for a reason that does not generalise to other bare keys. A QAction shortcut is dispatched before the focused widget sees the key, and Qt withholds only plain LETTERS from editable widgets, so by the argument that made bare Return break the query bar this should delete mail while the user edits a query. It does not: QLineEdit accepts the ShortcutOverride for Delete itself, because Delete is one of its own editing keys, which Return is not. Measured with and without an explicit filter, the action fires 0 times either way, so no filter is added. theDeleteKeyEditsTextInTheQueryBar() pins that Qt behaviour, since the binding rests on it. **Two defects surfaced from the second binding, both real.** An action can now have more than one default, and KeyMap did not allow for it. sequenceFor() decided "is this a built-in?" by comparing against defaultSequenceFor(), which returns only the FIRST default, so the second looked like a user override and won the "a user binding beats the default" rule. The menus advertised Ctrl+D to a user who had configured nothing, and sequenceFor() and defaultSequenceFor() disagreed about an untouched action. isDefaultBinding() asks whether a sequence is ANY of the action's defaults; when two defaults tie, the one defaultBindings() lists first wins, which is the author's stated preference rather than an alphabetical accident. And Restore read each message's origin tag FROM THE MODEL. The model's tags come from the query, so a row whose delete has not been re-queried still carries its pre-delete tags: measured `[inbox,unread]` on a message already sitting in the trash, one run in three. No origin tag was found, the message took the no-origin branch, and Restore moved it to the INBOX instead of the folder it came from, silently, with the origin tag left behind as the only evidence. A restore has to be right about the destination or it is worse than doing nothing. The trash-view Restore now resolves its messages against the DATABASE first, through a new NotmuchWorker::resolveMessages(). That and resolveThreadMessages() share one walk, resolveQuery(), rather than growing a near-duplicate: they differ only in whether the terms are `id:` or `thread:`. restoreSelectedThreads() already worked this way; this is the same reasoning applied to the message-scoped path. The flake was found by running one test five times rather than trusting a single green, and the fix verified the same way: 5 of 5, then the full suite three times over. Co-Authored-By: Claude Opus 5 --- src/keymap.cpp | 46 +++++++++++++++++- src/keymap.h | 8 ++++ src/mainwindow.cpp | 120 +++++++++++++++++++++++++++++++++++++++++++++- src/mainwindow.h | 19 ++++++++ src/notmuchworker.cpp | 27 +++++++++-- src/notmuchworker.h | 18 +++++++ tests/test_mainwindow.cpp | 78 ++++++++++++++++++++++++++++-- 7 files changed, 307 insertions(+), 9 deletions(-) (limited to 'src/notmuchworker.h') diff --git a/src/keymap.cpp b/src/keymap.cpp index 319bc53..7a08a58 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -98,6 +98,22 @@ QList> KeyMap::defaultBindings() { QStringLiteral("Alt+Up"), QStringLiteral("prev_thread") }, { QStringLiteral("Return"), QStringLiteral("open_thread") }, { QStringLiteral("Ctrl+E"), QStringLiteral("archive") }, + // Del FIRST, and the order matters twice over. defaultSequenceFor() + // returns the first match, and sequenceFor() prefers any binding that + // is not that default, treating it as a user override; listing Del + // second therefore made it the "override" of Ctrl+D and left the two + // functions disagreeing about which key the menus should advertise. + // First also makes it the ADVERTISED one, which is the point: it is + // the key a user reaches for, and Ctrl+D is not a guess anyone makes. + // + // Bare, which is safe for a reason that does NOT generalise to other + // bare keys. Delete is not a letter, so Qt's protection for editable + // widgets does not cover it, but QLineEdit accepts the + // ShortcutOverride for Delete itself, because it is one of its own + // editing keys. Return is not, which is why that one needed an + // explicit filter in MainWindow::eventFilter() and this one does not. + // Measured both ways; see theDeleteKeyEditsTextInTheQueryBar(). + { QStringLiteral("Del"), QStringLiteral("delete") }, { QStringLiteral("Ctrl+D"), QStringLiteral("delete") }, // Restore is only enabled in the trash view, so its key is dead // elsewhere rather than doing something surprising. @@ -250,7 +266,7 @@ QKeySequence KeyMap::sequenceFor(const QString &action) const if (it.value() != action) continue; - const bool isBuiltIn = !builtIn.isEmpty() && it.key() == builtIn; + const bool isBuiltIn = isDefaultBinding(it.key(), action); if (best.isEmpty()) { best = it.key(); bestIsBuiltIn = isBuiltIn; @@ -260,6 +276,13 @@ QKeySequence KeyMap::sequenceFor(const QString &action) const if (bestIsBuiltIn && !isBuiltIn) { best = it.key(); bestIsBuiltIn = false; + } else if (bestIsBuiltIn && isBuiltIn) { + // Both are defaults, so the ADVERTISED one is whichever + // defaultBindings() lists first: that order is the author's + // preference and is why Del is listed before Ctrl+D. Falling back + // to alphabetical here would advertise Ctrl+D instead. + if (it.key() == builtIn) + best = it.key(); } else if (bestIsBuiltIn == isBuiltIn && it.key().toString() < best.toString()) { best = it.key(); @@ -268,6 +291,27 @@ QKeySequence KeyMap::sequenceFor(const QString &action) const return best; } +bool KeyMap::isDefaultBinding(const QKeySequence &sequence, + const QString &action) +{ + // ANY of the action's defaults, not just the first. + // + // An action can ship with more than one binding: `delete` has Del and + // Ctrl+D. sequenceFor() compares against defaultSequenceFor(), which + // returns only the first, so the second looked like a USER binding and + // won the "a user binding always beats the default" rule. The menus then + // advertised Ctrl+D for a user who had configured nothing, and + // sequenceFor() and defaultSequenceFor() disagreed about an untouched + // action. + for (const auto &binding : defaultBindings()) { + if (binding.second == action + && normalizeSequence(binding.first) == sequence) { + return true; + } + } + return false; +} + QKeySequence KeyMap::defaultSequenceFor(const QString &action) { for (const auto &binding : defaultBindings()) { diff --git a/src/keymap.h b/src/keymap.h index 81e1813..8774209 100644 --- a/src/keymap.h +++ b/src/keymap.h @@ -73,6 +73,14 @@ public: /// The built-in sequence for an action, ignoring any user override. static QKeySequence defaultSequenceFor(const QString &action); + /// Whether `sequence` is ANY of `action`'s default bindings. + /// + /// Not the same question as `sequence == defaultSequenceFor(action)`: an + /// action can ship several, and comparing against only the first makes the + /// others look like user overrides. + static bool isDefaultBinding(const QKeySequence &sequence, + const QString &action); + /// Every action name carrying a built-in binding. static QStringList defaultActions(); diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index da7128f..055e783 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -309,6 +309,18 @@ bool MainWindow::eventFilter(QObject *watched, QEvent *event) keyEvent->accept(); return true; } + // Delete needs NO entry here, and that is worth stating because the + // reasoning that says it does is nearly right. It is bound bare to + // `delete`, and Qt's protection for editable widgets covers plain + // LETTERS only, so by the same argument that made Return a problem it + // should trigger the action while the user edits a query. + // + // It does not, because QLineEdit accepts the ShortcutOverride for + // Delete itself: Delete is one of its own editing keys, which Return + // is not. Measured both ways, with this branch present and absent: + // the action fires 0 times either way and the text is edited either + // way. Adding a guard here would be dead code carrying a test that + // cannot fail. } return QMainWindow::eventFilter(watched, event); @@ -851,7 +863,7 @@ void MainWindow::registerActions() }); addAction(QStringLiteral("restore"), tr("&Restore from trash"), tr("Move the selected messages out of the trash"), [this]() { - restoreSelected(true); + restoreSelectedFromTrash(); }); addAction(QStringLiteral("spam"), tr("Mark &spam"), tr("Add spam and remove inbox"), [this]() { @@ -4414,6 +4426,11 @@ void MainWindow::onThreadMessagesResolved(const QStringList &messageIds, return; } + if (requestTag == QStringLiteral("restore_messages")) { + restoreResolvedMessages(messageIds, paths, tags); + return; + } + if (requestTag != QStringLiteral("undelete_thread")) return; @@ -4531,6 +4548,107 @@ QString MainWindow::inboxFolderFor(const Account &account) const return QStringLiteral("Inbox"); } +void MainWindow::restoreResolvedMessages(const QStringList &messageIds, + const QStringList &paths, + const QStringList &tags) +{ + if (messageIds.size() != paths.size() || messageIds.size() != tags.size()) + return; + + const QString prefix = QStringLiteral("deleted-from:"); + QHash byOrigin; + QHash byInbox; + QStringList stranded; + + for (int i = 0; i < messageIds.size(); ++i) { + const QStringList messageTags = + tags.at(i).split(QLatin1Char('\t'), Qt::SkipEmptyParts); + QString origin; + for (const QString &tag : messageTags) { + if (tag.startsWith(prefix)) { + origin = tag.mid(prefix.length()); + break; + } + } + + const Account account = accountForMessagePath(paths.at(i)); + if (account.maildir.isEmpty()) { + stranded.append(messageIds.at(i)); + continue; + } + + if (origin.isEmpty()) { + // Trashed by another client, so there is no record of where it + // belongs. Inbox is the documented fallback, and it is reported: + // a guess the user is not told about is worse than the guess. + byInbox[account.maildir + QLatin1Char('/') + + account.inboxFolder()] + .append(messageIds.at(i)); + continue; + } + byOrigin[account.maildir + QLatin1Char('/') + origin] + .append(messageIds.at(i)); + } + + for (auto it = byOrigin.cbegin(); it != byOrigin.cend(); ++it) { + // The origin tag is named here rather than left as the placeholder, + // which onMessagesMoved() would resolve to the folder the message is + // coming FROM, namely the trash. + const QString origin = originTagFor(it.key()); + QStringList remove{ QStringLiteral("deleted") }; + if (!origin.isEmpty()) + remove.append(origin); + sendMove(it.value(), it.key(), {}, remove, tr("Restore")); + } + + for (auto it = byInbox.cbegin(); it != byInbox.cend(); ++it) { + sendMove(it.value(), it.key(), {}, { QStringLiteral("deleted") }, + tr("Restore")); + } + + if (!byInbox.isEmpty()) { + m_statusLabel->setText( + tr("%n message(s) had no record of where they came from and were " + "moved to the inbox.", "", int(byInbox.size()))); + } + if (!stranded.isEmpty()) { + m_statusLabel->setText( + tr("%n message(s) could not be restored: they belong to no " + "configured account.", "", int(stranded.size()))); + } +} + +void MainWindow::restoreSelectedFromTrash() +{ + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + if (rows.isEmpty()) + return; + + const ActionScope scope = m_model->messageScopeFor(rows); + if (scope.messageIds.isEmpty()) + return; + + // Resolved by the WORKER, not read from the model. + // + // The model's tags come from the QUERY, and a row whose delete has not yet + // been re-queried still carries its pre-delete tags: measured + // `[inbox,unread]` on a message already in the trash, one run in three. + // The origin tag is then not found, the message falls into the + // no-origin branch, and Restore sends it to the INBOX instead of the + // folder it came from, silently and irreversibly. + // + // A restore has to be right about the destination or it is worse than + // doing nothing, so it asks the database rather than trusting a view that + // may be a moment behind. restoreSelectedThreads() already worked this + // way; this is the same reasoning applied to the message-scoped path. + m_pendingRestoreIds = scope.messageIds; + QMetaObject::invokeMethod( + m_worker, "resolveMessages", Qt::QueuedConnection, + Q_ARG(QStringList, scope.messageIds), + Q_ARG(QString, QStringLiteral("restore_messages"))); +} + void MainWindow::restoreSelected(bool fallbackToInbox) { const QModelIndexList rows = diff --git a/src/mainwindow.h b/src/mainwindow.h index 937d87c..f9a075d 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -817,6 +817,25 @@ private: /// tag, so the tag comes off and the file stays where it is. void restoreSelected(bool fallbackToInbox = false); + /// Restore as reached from the TRASH VIEW: resolves each selected + /// message against the database first, then moves it. + /// + /// Asynchronous, unlike restoreSelected(), and that is the point. The + /// model's tags come from the query, so a row whose delete has not been + /// re-queried still carries its pre-delete tags; reading the origin from + /// there found none and sent the message to the INBOX instead of the + /// folder it came from, one run in three. + void restoreSelectedFromTrash(); + + /// Moves each resolved message home, using the tags and paths the WORKER + /// reported rather than anything the model holds. + void restoreResolvedMessages(const QStringList &messageIds, + const QStringList &paths, + const QStringList &tags); + + /// The messages a resolveMessages() request was made for. + QStringList m_pendingRestoreIds; + /// The account's inbox FOLDER name, discovered from its inbox query. /// /// Never hardcoded: the real Maildir has `Inbox` and a fixture has diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 62174c9..2c03226 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -776,15 +776,26 @@ void NotmuchWorker::moveMessages(const QStringList &messageIds, emit messagesMovedFrom(origins, destFolder); } +void NotmuchWorker::resolveMessages(const QStringList &messageIds, + const QString &requestTag) +{ + if (messageIds.isEmpty()) + return; + + QStringList terms; + terms.reserve(messageIds.size()); + for (const QString &id : messageIds) + terms.append(QStringLiteral("id:%1").arg(id)); + + resolveQuery(terms.join(QStringLiteral(" or ")), requestTag); +} + void NotmuchWorker::resolveThreadMessages(const QStringList &threadIds, const QString &requestTag) { if (threadIds.isEmpty()) return; - if (!openReadOnly()) - return; - // One combined query, for the reason applyTagsToThreads() gives: a query // per thread reopens the same Xapian cursor once per selected row. QStringList terms; @@ -792,7 +803,15 @@ void NotmuchWorker::resolveThreadMessages(const QStringList &threadIds, for (const QString &id : threadIds) terms.append(QStringLiteral("thread:%1").arg(id)); - const QString query = terms.join(QStringLiteral(" or ")); + resolveQuery(terms.join(QStringLiteral(" or ")), requestTag); +} + +void NotmuchWorker::resolveQuery(const QString &query, + const QString &requestTag) +{ + if (!openReadOnly()) + return; + NmQuery nmQuery(notmuch_query_create(m_db, query.toUtf8().constData())); if (!nmQuery) { emit errorOccurred(QStringLiteral("Cannot resolve selected threads")); diff --git a/src/notmuchworker.h b/src/notmuchworker.h index dfd4303..9932e59 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -158,6 +158,24 @@ public slots: void resolveThreadMessages(const QStringList &threadIds, const QString &requestTag); + /// The same walk for a set of MESSAGE ids rather than thread ids. + /// + /// Restore needs each message's tags and path to decide where to send it, + /// and must not read them from the model: the model's tags come from the + /// query, so a row whose delete has not been re-queried still carries its + /// pre-delete tags and the origin tag is missing. A restore that guesses + /// the destination is worse than one that does nothing. + void resolveMessages(const QStringList &messageIds, + const QString &requestTag); + +private: + /// The shared walk behind resolveMessages() and resolveThreadMessages(): + /// runs `query` and emits threadMessagesResolved() with each match's id, + /// database-relative path and tab-joined tags. + void resolveQuery(const QString &query, const QString &requestTag); + +public slots: + /// Every tag in the database, sorted. Feeds query bar completion, which /// cannot offer tag names it has no way to enumerate. Called at startup, /// after a sync, and after a tag mutation introduces an unknown tag. diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index cefd686..85418a4 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -377,6 +377,8 @@ private slots: void deletingAThreadRootTwiceRestoresItRatherThanRedeleting(); void deleteThreadMovesEveryMessageAndRepaintsTheRootCard(); void aFolderNameWithASpaceSurvivesTheRoundTrip(); + void deleteIsBoundToTheDeleteKey(); + void theDeleteKeyEditsTextInTheQueryBar(); void restoreIsReachableWithoutTheKeyboard(); void restoreIsOnlyEnabledInTheTrashView(); void restoreReturnsAMessageToItsOriginFolder(); @@ -9466,6 +9468,68 @@ void TestMainWindow::aFolderNameWithASpaceSurvivesTheRoundTrip() "messages are somewhere mbsync will never sync"); } +void TestMainWindow::deleteIsBoundToTheDeleteKey() +{ + // Del is the key a user reaches for, and Ctrl+D is not a guess anyone + // makes. Both are bound; this asserts the bare one is really there, + // since setShortcut() keeps only the LAST of several and silently drops + // the rest, which would leave the documented binding absent. + const Config config; + MainWindow window(config); + + auto *action = window.findChild(QStringLiteral("delete")); + QVERIFY(action); + + QVERIFY2(action->shortcuts().contains(QKeySequence(Qt::Key_Delete)), + qPrintable(QStringLiteral("delete is not on the Del key; it has: %1") + .arg(QKeySequence::listToString(action->shortcuts())))); +} + +void TestMainWindow::theDeleteKeyEditsTextInTheQueryBar() +{ + // `delete` is bound to bare Del, and a QAction shortcut is dispatched + // BEFORE the focused widget sees the key. Qt withholds only plain LETTERS + // from editable widgets, so by the argument that made bare Return break + // the query bar, Delete should move mail to the trash while the user is + // editing a query. + // + // It does not: QLineEdit accepts the ShortcutOverride for Delete itself, + // because Delete is one of its own editing keys, which Return is not. That + // is a property of Qt rather than of this code, which is exactly why it is + // pinned here: it is the assumption the bare binding rests on, and if a + // future Qt or a future focus proxy changes it, mail gets deleted while + // someone types. + // + // Asserted on the ACTION not firing, not on the ShortcutOverride phase. A + // probe on the override reports notify=1 accepted=1 whether or not this + // window filters the key, since QLineEdit accepts it either way, so it + // cannot distinguish the two and passes against any implementation. + // Measured, while trying to write this test the obvious way. + const Config config; + MainWindow window(config); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + auto *deleteAction = window.findChild(QStringLiteral("delete")); + QVERIFY(queryEdit && deleteAction); + + int fired = 0; + QObject::connect(deleteAction, &QAction::triggered, + [&fired]() { ++fired; }); + + queryEdit->setFocus(); + QTRY_VERIFY(queryEdit->hasFocus()); + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->setCursorPosition(0); + + QTest::keyClick(queryEdit, Qt::Key_Delete); + + QCOMPARE(fired, 0); + QCOMPARE(queryEdit->text(), QStringLiteral("ag:inbox")); +} + void TestMainWindow::restoreIsReachableWithoutTheKeyboard() { // Restore shipped as a keyboard shortcut and nothing else: registered, @@ -9603,15 +9667,23 @@ void TestMainWindow::restoreReturnsAMessageToItsOriginFolder() || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), stem), 15000); + // Waited on the ORIGIN tag, not on `deleted`. + // + // Both come off in one write, but the file rename and the tag write are + // separate operations and the assertions below raced the second one: + // measured 1 failure in 3 runs waiting on `deleted` alone, reporting the + // origin tag still present. Waiting on the tag this test is actually about + // removes the race rather than papering over it with a longer timeout. + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, QStringLiteral("id:ro1@example.org and " + "tag:\"deleted-from:inbox\"")) == 0, + 15000); QTRY_VERIFY_WITH_TIMEOUT( notmuchCount(cfg, QStringLiteral("id:ro1@example.org and tag:deleted")) == 0, 15000); QCOMPARE(notmuchCount(cfg, QStringLiteral("id:ro1@example.org")), 1); - QCOMPARE(notmuchCount(cfg, QStringLiteral("id:ro1@example.org and " - "tag:\"deleted-from:inbox\"")), - 0); QVERIFY(!folderHasMessageFile(root + QStringLiteral("/acct/Trash/cur"), stem)); } -- cgit v1.2.3