From 9899b9af523ad2aef55300c9132d21a687a0a19d Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 17 Aug 2026 19:51:27 +0200 Subject: feat(config): warn when an account configures no trash folder The trash key is mandatory: Delete moves a file into it, so an account without one cannot delete at all. Report it as a config problem naming the account and the key, rather than degrading Delete silently, per the existing "a warning the user cannot act on teaches them to ignore warnings" rule (item 83). Several existing test fixtures loaded accounts with no trash key and asserted zero problems/warnings; added trash=Trash to those where it was incidental to what the test actually covers. --- translations/qtmaildir_it_IT.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) (limited to 'translations') diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index 036ce4b..fd51547 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -43,6 +43,10 @@ [completion] extra_mimetypes: entry '%1' has no mimetype; ignoring it. [completion] extra_mimetypes: la voce '%1' non ha un mimetype; verrà ignorata. + + Account '%1' has no trash folder configured; add a 'trash' key to its section. Delete will not work for this account until it does. + L'account '%1' non ha un cestino configurato; aggiungere una chiave 'trash' alla sua sezione. L'eliminazione non funzionerà per questo account finché non verrà fatto. + Startup account '%1' is not a configured account; starting on all accounts. L'account iniziale '%1' non è un account configurato; si parte da tutti gli account. @@ -280,7 +284,7 @@ Add or remove the important tag - Aggiunge o rimuove l'etichetta importante + Aggiunge o rimuove l'etichetta importante Unmark important @@ -340,7 +344,7 @@ Add or remove the deleted tag on whole threads - Aggiunge o rimuove l'etichetta eliminato su intere conversazioni + Aggiunge o rimuove l'etichetta eliminato su intere conversazioni Undelete thread @@ -364,7 +368,7 @@ Toggle the unread tag on whole threads - Inverte l'etichetta non letto su intere conversazioni + Inverte l'etichetta non letto su intere conversazioni Mark thread read -- cgit v1.2.3 From d5e9ec157946635294001b57ce8353a109ec5051 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 17 Aug 2026 19:58:51 +0200 Subject: feat(config): add the trash query generator Adds trash as a fifth built-in query filter beside Unread, Inbox, Important and Sent, composing per-account exactly as Sent does: Config::resolvedQuery() asks each account for its own trashQuery() rather than wrapping the all-accounts union, and an account with no trash folder resolves to matchNothingQuery() rather than "match everything". Also gives the Trash button a toolbar icon (user-trash) and a trash key to the mainwindow fixture that asserts every filter button carries one; without it the button is skipped from the row entirely (no account configured a trash folder), and the existing icon test found no button to check. --- src/config.cpp | 27 +++++++++++++++++++- src/config.h | 7 ++++++ src/mainwindow.cpp | 1 + tests/test_config.cpp | 55 +++++++++++++++++++++++++++++++++++++++-- tests/test_mainwindow.cpp | 16 +++++++++--- translations/qtmaildir_it_IT.ts | 4 +++ 6 files changed, 104 insertions(+), 6 deletions(-) (limited to 'translations') diff --git a/src/config.cpp b/src/config.cpp index 8294e4b..1799ac6 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -60,7 +60,8 @@ constexpr int kQueriesFormatVersion = 1; const QStringList kQueryGenerators = { QStringLiteral("unread"), QStringLiteral("inbox"), QStringLiteral("flagged"), - QStringLiteral("sent") }; + QStringLiteral("sent"), + QStringLiteral("trash") }; /// The tag a generator matches, for the three filters that are a plain tag /// query. Empty for "sent", which composes from each account's folder instead @@ -152,6 +153,11 @@ QString Config::allDraftsQuery() const return joinAccountQueries(m_accounts, &Account::draftsQuery); } +QString Config::allTrashQuery() const +{ + return joinAccountQueries(m_accounts, &Account::trashQuery); +} + QString Config::defaultPath() { const QString base = @@ -737,6 +743,8 @@ QString Config::resolvedQuery(const SavedQuery &query) const if (query.isGenerated()) { if (query.generated == QStringLiteral("sent")) return allSentQuery(); + if (query.generated == QStringLiteral("trash")) + return allTrashQuery(); // An unknown generator was reported on load. Empty rather than the // bare stored query, which for a generated entry is empty anyway and // would otherwise run as "match everything". @@ -809,6 +817,11 @@ SavedQuery Config::builtinFilter(const QString &generator) // thread would fold the user's sent message back into the conversation // it belongs to, which is item 63's finding. filter.flat = true; + } else if (generator == QStringLiteral("trash")) { + filter.name = tr("Trash"); + // NOT flat, unlike Sent. A deleted message still belongs to its + // conversation, and folding it back is what Sent had to avoid rather + // than something every folder filter wants. } return filter; @@ -833,6 +846,10 @@ QString Config::resolvedQuery(const SavedQuery &query, const QString all = allSentQuery(); return all.isEmpty() ? matchNothingQuery() : all; } + if (query.generated == QStringLiteral("trash")) { + const QString all = allTrashQuery(); + return all.isEmpty() ? matchNothingQuery() : all; + } return QStringLiteral("tag:%1").arg(generatorTag(query.generated)); } @@ -853,6 +870,14 @@ QString Config::resolvedQuery(const SavedQuery &query, return sent.isEmpty() ? matchNothingQuery() : sent; } + if (query.generated == QStringLiteral("trash")) { + // The account's OWN trash query, for the reason spelled out above the + // sent case: wrapping the all-accounts query in this account's path + // works by accident of path: being hierarchical. + const QString trash = scope.trashQuery(); + return trash.isEmpty() ? matchNothingQuery() : trash; + } + // A tag filter carries no path of its own, so scoping is exactly what // scopedQuery() does. Its parentheses are load-bearing: `path:... and a or // b` binds as `(path:... and a) or b`. diff --git a/src/config.h b/src/config.h index 1feeeda..f60e7cc 100644 --- a/src/config.h +++ b/src/config.h @@ -291,6 +291,13 @@ public: /// open-coded at the call site. QString allSentQuery() const; + /// Matches every configured account's trash, or empty when none has one. + /// + /// Joins only the NON-EMPTY trashQuery() results, for the same reason + /// allSentQuery() does: notmuch accepts a bare "or" without complaint and + /// silently answers a different question. + QString allTrashQuery() const; + /// Matches every configured account's drafts, or empty when none has one. /// /// Joins only the NON-EMPTY draftsQuery() results, for the same reason diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 594535b..67d3ee4 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1893,6 +1893,7 @@ void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) { QStringLiteral("inbox"), QStringLiteral("mail-inbox") }, { QStringLiteral("flagged"), QStringLiteral("starred") }, { QStringLiteral("sent"), QStringLiteral("mail-folder-sent") }, + { QStringLiteral("trash"), QStringLiteral("user-trash") }, }; button->setIcon( QIcon::fromTheme(filterIcons.value(filter.generated))); diff --git a/tests/test_config.cpp b/tests/test_config.cpp index 10dcf68..ea5c363 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -119,6 +119,8 @@ private slots: void anAccountCarriesItsTrashFolder(); void aBracketedTrashFolderIsQuoted(); void anAccountWithoutATrashFolderWarns(); + void theTrashFilterComposesPerAccount(); + void theTrashFilterMatchesNothingWithoutAFolder(); }; static QString writeIni(const QTemporaryDir &dir, const QString &body) @@ -1009,6 +1011,54 @@ void TestConfig::anAccountWithoutATrashFolderWarns() QVERIFY(joined.contains(QStringLiteral("trash"))); } +void TestConfig::theTrashFilterComposesPerAccount() +{ + // Two accounts, one with a plain folder and one nested under a bracketed + // parent, since the real setup has both shapes. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[account.work]\n" + "maildir=work\n" + "trash=Trash\n" + "\n" + "[account.personal]\n" + "maildir=personal\n" + "trash=[Provider]/Cestino\n"))); + + const SavedQuery trash = Config::builtinFilter(QStringLiteral("trash")); + QVERIFY(trash.isGenerated()); + + // All accounts: the union, never a bare path that would match one account. + const QString all = config.resolvedQuery(trash, QString()); + QVERIFY(all.contains(QStringLiteral("path:\"work/Trash/**\""))); + QVERIFY(all.contains( + QStringLiteral("path:\"personal/[Provider]/Cestino/**\""))); + + // One account: that account's OWN query. Asserting on the STRING, not on a + // row count: the all-accounts query wrapped in this account's path returns + // exactly the right rows, because path: is hierarchical, so a count passes + // against the wrong thing. Config::resolvedQuery documents this trap. + const QString scoped = config.resolvedQuery(trash, QStringLiteral("work")); + QCOMPARE(scoped, QStringLiteral("path:\"work/Trash/**\"")); + QVERIFY(!scoped.contains(QStringLiteral("personal"))); +} + +void TestConfig::theTrashFilterMatchesNothingWithoutAFolder() +{ + // An empty query means "match everything" to notmuch, so a filter with + // nothing to match must say so explicitly. A button labelled Trash that + // showed the whole Maildir is the failure this prevents. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[account.work]\n" + "maildir=work\n"))); + + const SavedQuery trash = Config::builtinFilter(QStringLiteral("trash")); + QCOMPARE(config.resolvedQuery(trash, QString()), Config::matchNothingQuery()); +} + void TestConfig::sentQueryComposesWithScopedQuery() { // A Sent view under one account must not show another account's sent mail. @@ -1432,7 +1482,7 @@ void TestConfig::everyBuiltinFilterIsAKnownGenerator() Config config; const QList filters = config.builtinFilters(); - QCOMPARE(filters.size(), 4); + QCOMPARE(filters.size(), 5); QStringList names; for (const SavedQuery &filter : filters) { @@ -1453,7 +1503,8 @@ void TestConfig::everyBuiltinFilterIsAKnownGenerator() QCOMPARE(names, (QStringList{ QStringLiteral("Unread"), QStringLiteral("Inbox"), QStringLiteral("Important"), - QStringLiteral("Sent") })); + QStringLiteral("Sent"), + QStringLiteral("Trash") })); } void TestConfig::aFilterAcrossAllAccountsIsTheUnscopedQuery() diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index b188ef4..7316689 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -7922,10 +7922,20 @@ void TestMainWindow::everyBuiltinFilterButtonCarriesAnIconAndItsText() // which is the same argument the Save button records. QTemporaryDir dir; QVERIFY(dir.isValid()); - Config config; - config.load(writeSentConfig(dir, { + const QString path = writeSentConfig(dir, { {QStringLiteral("work"), QStringLiteral("Sent")}, - })); + }); + // A trash key too, or the Trash filter finds nothing and is skipped from + // the row entirely (item 103), leaving no trashButton for this loop to + // find. + { + QSettings s(path, QSettings::IniFormat); + s.beginGroup(QStringLiteral("account.work")); + s.setValue(QStringLiteral("trash"), QStringLiteral("Trash")); + s.endGroup(); + } + Config config; + config.load(path); MainWindow window(config); diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index fd51547..ef88967 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -95,6 +95,10 @@ Sent Inviati + + Trash + Cestino + HtmlBuilder -- 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 'translations') 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 4583de009571aaa674e7d161d31ec640860787e1 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 18 Aug 2026 12:13:51 +0200 Subject: fix(delete): repair seven defects in the move-to-trash path Item 103's implementation was committed unreviewed and never hand-tested. Reviewing it, and then hand-testing it against real mail, found seven defects. Six of them lose or corrupt state and none was caught by the suite, which was green throughout. **Undo pushed a command instead of consuming one.** onMessagesMoved() pushed a MoveCommand for every confirmed move, including the move an undo had just made, so undoText went "Delete", "Undo Delete", "Undo Undo Delete". A second press of undo re-deleted the message the first had rescued. PendingMove carries a fromUndo flag, which has to survive the queued round trip and so cannot be a window-wide "am I undoing" flag. **Held moves were invisible to the quit guard.** pendingEditCount() summed the held tag edits and not the held moves, so a Delete pressed during a sync left the count at zero: the indicator stayed hidden and closeEvent()'s guard never fired, discarding the move on quit with no prompt. That is item 106's data loss with a worse shape, because a dropped move leaves the file in the folder the user asked it out of. **Two moves to one folder dropped the second's tags.** m_pendingMoves was keyed on the destination, so two Deletes in one account before the first confirmation both named `acct/Trash` and the second insert overwrote the first. That file reached the trash carrying neither `deleted` nor `deleted-from:`, unrestorable and invisible to a `tag:deleted` query. It is a FIFO now: the worker moves one batch at a time and emits in request order, so position alone matches a confirmation to its request. **Second Delete left the origin tag behind.** The restore passed the origin PLACEHOLDER in its removal list, and onMessagesMoved() resolves that from the folder the worker reports, which on a restore is the trash. It asked to remove `deleted-from:Trash`, a tag never written, while the real `deleted-from:inbox` was never named. A restore does not need the placeholder: it already read the origin to decide where to send the file. originTagFor() is now the one derivation both sides use. **Ctrl+Z left it behind too**, for a different reason: MoveCommand was constructed with the unresolved pending.add. The command carries the resolved tags now, and is pushed per origin group rather than once per batch, because the placeholder resolves to a different tag per origin. **A thread root re-deleted itself.** everySelectedRowHasTag() asked a thread row about its THREAD's tags, which notmuch gives as a union. Delete the root of a three-message thread and the replies are untouched, so the union carries no `deleted` and a second press ran Delete again: the message moved trash-to-trash and came out with `deleted`, `deleted-from:inbox` AND `deleted-from:Trash`, with no way back. The union was a documented approximation, called bounded because the worst case for a TAG toggle was re-applying a tag the message already had. A MOVE re-applies the move. Resolved through messageById(), NOT through ThreadSummary::firstMessageTags, which is the value the query delivered and is never refreshed by an optimistic update: after a delete the node reads `deleted` while the summary still reads `unread`. **Delete thread never moved anything.** It was left calling tagSelected() when Delete became a move, so a whole conversation sat in the inbox wearing a `deleted` chip. It moves every message now, each with its own origin, so a thread spanning folders reassembles on restore. A reply row resolves to its own thread through selectedThreadIds(): scopeFor() reports a reply under messageIds and leaves threadIds empty, which made a thread action on a reply row do nothing at all. **And the root card did not repaint** until it was clicked, while its replies did. sendMove() had no optimistic update at all, so nothing moved until the worker answered; and applyMessageTagChange() deliberately leaves a multi-message thread's SUMMARY alone, which is correct for a one-message edit and wrong for a thread-scoped one. The replies have nodes and repainted; the root card reads the summary. The thread paths repaint synchronously with applyTagChange() before the worker is asked, which also keeps the toggle's direction readable for the next press. Every fix carries a test and every test was mutation-checked. Three false greens were found while writing them and are recorded at their assertions: a disjunction that emptied on the wrong term, a QTRY_VERIFY(rowCount() == 0) satisfied by the interval before the worker answers, and a query issued before the confirming write had landed. Absence is asked of notmuch directly through a new notmuchCount() helper for that reason. Two bare-window tests moved off assertions about synchronous pending writes onto the model, since the thread actions now round-trip through the worker. Co-Authored-By: Claude Opus 5 --- src/mainwindow.cpp | 454 ++++++++++++++++++++--- src/mainwindow.h | 85 ++++- tests/test_mainwindow.cpp | 779 +++++++++++++++++++++++++++++++++++++++- translations/qtmaildir_it_IT.ts | 40 +-- 4 files changed, 1258 insertions(+), 100 deletions(-) (limited to 'translations') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index d3f2dc7..361c0d4 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -214,7 +214,7 @@ void MainWindow::closeEvent(QCloseEvent *event) // Degrade to a warning rather than offering a sync that cannot run. const auto answer = QMessageBox::warning( this, tr("Unsynced changes"), - tr("%n tag change(s) have not been synced, and no sync command " + tr("%n change(s) have not been synced, and no sync command " "is configured. Quit anyway?", "", pendingEditCount()), QMessageBox::Discard | QMessageBox::Cancel, QMessageBox::Cancel); @@ -228,7 +228,7 @@ void MainWindow::closeEvent(QCloseEvent *event) QMessageBox box(this); box.setIcon(QMessageBox::Question); box.setWindowTitle(tr("Unsynced changes")); - box.setText(tr("%n tag change(s) have not been synced.", "", + box.setText(tr("%n change(s) have not been synced.", "", pendingEditCount())); box.setInformativeText(tr("Sync before quitting?")); QPushButton *sync = @@ -933,12 +933,19 @@ void MainWindow::registerActions() }); addAction(QStringLiteral("delete_thread"), tr("&Delete thread"), tr("Add or remove the deleted tag on whole threads"), [this]() { + // A MOVE now, like its message-scoped twin. It tagged and moved + // nothing until item 103's follow-up, so "Delete thread" left a whole + // conversation sitting in the inbox wearing a `deleted` chip: exactly + // the half-deleted state Delete stopped producing. + // + // The direction is read per MESSAGE, not from the thread's tag union. + // A thread whose root was deleted on its own carries `deleted` in the + // union while its replies do not, and asking the union there ran + // Delete a second time on messages already in the trash. if (everySelectedRowHasTag(QStringLiteral("deleted"), TagScope::Thread)) { - tagSelected({}, { QStringLiteral("deleted") }, - tr("Undelete thread"), TagScope::Thread); + restoreSelectedThreads(); } else { - tagSelected({ QStringLiteral("deleted") }, {}, tr("Delete thread"), - TagScope::Thread); + trashSelectedThreads(); } }); addAction(QStringLiteral("spam_thread"), tr("Mark thread as &spam"), @@ -1613,6 +1620,9 @@ void MainWindow::wireWorker() connect(m_worker, &NotmuchWorker::messagesMovedFrom, this, &MainWindow::onMessagesMoved); + connect(m_worker, &NotmuchWorker::threadMessagesResolved, + this, &MainWindow::onThreadMessagesResolved); + m_workerThread.start(); // Queued behind the thread start, so the completer has real tags as soon @@ -2941,7 +2951,7 @@ void MainWindow::flushHeldEdits() m_heldMoves.clear(); for (const HeldMove &move : moves) { sendMove(move.messageIds, move.destFolder, move.add, move.remove, - move.description); + move.description, move.fromUndo); } updatePendingIndicator(); } @@ -3756,7 +3766,15 @@ int MainWindow::pendingEditCount() const // Each held edit counts as one whatever its size, since it carries thread // ids rather than message ids and cannot be netted against the map. const int held = int(m_heldEdits.size()); - return m_pendingTagEdits.size() + m_unnettablePendingEdits + held; + // Held MOVES count for exactly the same reason, and were missed. With no + // tag edit queued the count was 0, so the indicator stayed hidden and + // closeEvent()'s `pendingEditCount() > 0` guard never fired: a Delete + // pressed during a sync was discarded on quit with no prompt at all. That + // is item 106's data loss, and worse here, because a dropped move leaves + // the file in the folder the user asked it out of. + const int heldMoves = int(m_heldMoves.size()); + return m_pendingTagEdits.size() + m_unnettablePendingEdits + held + + heldMoves; } void MainWindow::updatePendingIndicator() @@ -3771,7 +3789,7 @@ void MainWindow::updatePendingIndicator() // they did, not the writes it became. m_pendingLabel->setText(tr("%n unsynced change(s)", "", pending)); m_pendingLabel->setToolTip( - tr("Tag changes made here that a sync has not yet carried to the mail " + tr("Changes made here that a sync has not yet carried to the mail " "store. An external notmuch run can clear them without this count " "noticing.")); m_pendingLabel->show(); @@ -3895,22 +3913,43 @@ bool MainWindow::everySelectedRowHasTag(const QString &tag, } else if (m_model->isMessageRow(index)) { tags = m_model->messageAt(index).tags; } else { - // The thread's summary, and this is a KNOWN approximation rather - // than an oversight. A thread row acts on the message its card - // displays, but that message's own tags are never in the model: - // setThreadMessages drops depth 0 because the root row stands for - // it, so there is no node to read and messageById() cannot find - // one. The summary is a union over the thread, so it answers - // "unread" while ANY message is. + // A thread row answers about the MESSAGE ITS CARD DISPLAYS, which + // is what it acts on. threadFor() already substitutes that + // message's own tags for the thread's union when they are known + // (item 110), so this reads the row's real state rather than a + // union over messages it does not stand for. // - // The consequence is bounded and only affects the DIRECTION a - // toggle picks, never what it writes: on a thread whose first - // message is read while a later one is not, Toggle unread reads - // the thread as unread and marks the first message read again, a - // no-op. Fixing it properly needs per-message state in - // ThreadSummary, which is the same thing item 87 needs; leave it - // for that item rather than guessing here. - tags = m_model->threadFor(index).tags; + // This used to read the union deliberately, with a comment + // calling the imprecision bounded because no per-message tags + // existed in the model. They do now: ThreadSummary carries + // firstMessageTags from the query, so an UNEXPANDED row already + // knows its own tags, and the comment outlived the fact. + // + // The cost of the union was not bounded once Delete became a + // MOVE. Deleting the root of a three-message thread left the two + // replies undeleted, so the union carried no `deleted`, so a + // second press read the row as not-deleted and deleted it AGAIN: + // the message was moved trash-to-trash and came out carrying + // `deleted`, `deleted-from:inbox` and `deleted-from:Trash` at + // once, with no way back. A tag toggle merely re-applied a tag it + // already had; a move re-applies the MOVE. + // + // Resolved through messageById() on the row's own message, which + // is the id messageScopeFor() will act on. Asking the same + // question the write asks is what keeps the direction and the + // write from disagreeing; the union answered a question about a + // conversation when the row stands for one message. + const ThreadSummary summary = m_model->threadFor(index); + const MessageNode own = + m_model->messageById(summary.firstMessageId); + // messageById() and NOT summary.firstMessageTags, which is the + // value the QUERY delivered and is not refreshed by an optimistic + // update: applyMessageTagChange() writes the row's node, so after + // a delete the node reads `deleted, deleted-from:inbox` while the + // summary still reads `inbox, unread`. Measured, and preferring + // the summary left this defect exactly as it was. + tags = own.messageId.isEmpty() ? summary.firstMessageTags + : own.tags; } if (!tags.contains(tag)) return false; @@ -4173,13 +4212,33 @@ void MainWindow::trashSelected() if (scope.messageIds.isEmpty()) return; + QHash pathById; + for (const QString &messageId : scope.messageIds) + pathById.insert(messageId, m_model->messageById(messageId).filePath); + + trashMessages(scope.messageIds, pathById, scope.messageCount); +} + +void MainWindow::trashMessages(const QStringList &messageIds, + const QHash &pathById, + int messageCount, + const QStringList &wholeThreadIds) +{ + if (messageIds.isEmpty()) + return; + // Grouped by destination, because moveMessages() takes one folder per call // and a selection can span accounts with different trash folders. + // + // Paths are passed IN rather than read from the model, because the thread + // path arrives with messages the model has never seen: a thread the user + // never expanded holds no node for its replies, so a lookup there returns + // nothing and every message resolves to no account. QHash byTrash; QStringList unconfigured; - for (const QString &messageId : scope.messageIds) { - const QString path = m_model->messageById(messageId).filePath; - const Account account = accountForMessagePath(path); + for (const QString &messageId : messageIds) { + const Account account = + accountForMessagePath(pathById.value(messageId)); if (account.trash.isEmpty()) { unconfigured.append(messageId); continue; @@ -4204,11 +4263,199 @@ void MainWindow::trashSelected() for (auto it = byTrash.cbegin(); it != byTrash.cend(); ++it) { sendMove(it.value(), it.key(), { QStringLiteral("deleted"), kOriginTagPlaceholder() }, {}, - tr("Delete")); + tr("Delete"), false, wholeThreadIds); } showTransientStatus( - tr("%1: %n message(s)", "", scope.messageCount).arg(tr("Delete"))); + tr("%1: %n message(s)", "", messageCount).arg(tr("Delete"))); +} + +QString MainWindow::originTagFor(const QString &dbRelativeFolder) const +{ + // `acct/inbox` becomes `deleted-from:inbox`. The tag stores the folder + // relative to the ACCOUNT, never to the database: the account prefix is + // recomposed from the message's own path when it is read back, so storing + // it would duplicate it and would go stale the day the user renames a + // maildir. + // + // Shared by the two sites that need the tag, rather than derived twice. + // They disagreed once already: onMessagesMoved() resolved a placeholder + // from the folder the worker reported, which on a RESTORE is the trash + // rather than the origin, so the restore stripped `deleted-from:Trash` + // and left the real tag in place. + const Account account = + accountForMessagePath(dbRelativeFolder + QLatin1Char('/')); + QString accountRelative = dbRelativeFolder; + if (!account.maildir.isEmpty() + && dbRelativeFolder.startsWith(account.maildir + QLatin1Char('/'))) { + accountRelative = dbRelativeFolder.mid(account.maildir.length() + 1); + } + if (accountRelative.isEmpty()) + return QString(); + return QStringLiteral("deleted-from:%1").arg(accountRelative); +} + +QStringList MainWindow::selectedThreadIds() const +{ + // A THREAD action on a reply row means that reply's conversation. + // + // scopeFor() reports a reply under messageIds and leaves threadIds empty, + // which is right for the mixed selections it was built for and wrong as + // the only input to a thread-scoped action: the early return on an empty + // threadIds made Delete thread do nothing at all when the selected row + // happened to be a reply. threadFor() resolves either kind of row. + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + QStringList threadIds; + for (const QModelIndex &index : rows) { + const QString threadId = m_model->threadFor(index).threadId; + if (!threadId.isEmpty() && !threadIds.contains(threadId)) + threadIds.append(threadId); + } + return threadIds; +} + +void MainWindow::trashSelectedThreads() +{ + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + if (rows.isEmpty()) + return; + + const QStringList threadIds = selectedThreadIds(); + if (threadIds.isEmpty()) + return; + + // Asked of the WORKER rather than resolved here. A thread the user never + // expanded has no nodes in the model for its replies, so the ids and the + // paths a move needs exist only in the database. applyTagsToThreads() + // solves the same problem the same way, for the same reason. + // + // Repainted HERE, synchronously, before the worker is asked. + // + // The move needs message ids and paths that only the database holds for an + // unexpanded thread, so the move itself is asynchronous. The DISPLAY must + // not wait for that round trip: the card is what the user watches, and + // holding it back is what made a deleted thread sit unchanged until it was + // clicked. It also keeps the toggle's direction readable immediately, so a + // second press restores rather than deleting again. + for (const QString &threadId : threadIds) + m_model->applyTagChange(threadId, { QStringLiteral("deleted") }, {}); + + m_pendingThreadScope = threadIds; + QMetaObject::invokeMethod(m_worker, "resolveThreadMessages", + Qt::QueuedConnection, + Q_ARG(QStringList, threadIds), + Q_ARG(QString, QStringLiteral("delete_thread"))); +} + +void MainWindow::onThreadMessagesResolved(const QStringList &messageIds, + const QStringList &paths, + const QStringList &tags, + const QString &requestTag) +{ + if (messageIds.size() != paths.size() || messageIds.size() != tags.size()) + return; + + QHash pathById; + for (int i = 0; i < messageIds.size(); ++i) + pathById.insert(messageIds.at(i), paths.at(i)); + + const QStringList threadScope = m_pendingThreadScope; + m_pendingThreadScope.clear(); + + if (requestTag == QStringLiteral("delete_thread")) { + trashMessages(messageIds, pathById, messageIds.size(), threadScope); + return; + } + + if (requestTag != QStringLiteral("undelete_thread")) + return; + + // Restore, resolved per message: each one goes back to the folder its own + // `deleted-from:` tag names, so a thread whose messages were deleted from + // different folders reassembles correctly rather than collapsing into one. + const QString prefix = QStringLiteral("deleted-from:"); + QHash byOrigin; + QStringList unknown; + for (int i = 0; i < messageIds.size(); ++i) { + // Split on TAB, matching resolveThreadMessages(). A space is not a + // safe separator: a folder name containing one produces a tag + // containing one, and splitting there silently truncates the origin + // to its first word. + 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; + } + } + // A message with no `deleted` tag is not in the trash and has nothing + // to come back from. A thread-scoped restore reaches every message, + // including ones the user never deleted, and moving those would drag + // untouched mail out of whatever folder it legitimately sits in. + if (!messageTags.contains(QStringLiteral("deleted"))) + continue; + const Account account = + accountForMessagePath(paths.at(i)); + if (origin.isEmpty() || account.maildir.isEmpty()) { + unknown.append(messageIds.at(i)); + continue; + } + byOrigin[account.maildir + QLatin1Char('/') + origin] + .append(messageIds.at(i)); + } + + if (!unknown.isEmpty()) { + // No origin recorded: 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, since guessing a folder would put the message somewhere + // the user never had it. + sendMessageTagChange(unknown, {}, { QStringLiteral("deleted") }, + tr("Undelete thread")); + m_undoStack.push(new MessageTagCommand(this, unknown, {}, + { QStringLiteral("deleted") }, + tr("Undelete thread"))); + } + + for (auto it = byOrigin.cbegin(); it != byOrigin.cend(); ++it) { + // The origin tag is named here, not left as the placeholder: on a + // restore the placeholder would resolve to the folder the message is + // coming FROM, which is the trash, and strip a tag never written. + const QString origin = originTagFor(it.key()); + QStringList remove{ QStringLiteral("deleted") }; + if (!origin.isEmpty()) + remove.append(origin); + sendMove(it.value(), it.key(), {}, remove, tr("Undelete thread"), + false, threadScope); + } + + showTransientStatus(tr("%1: %n message(s)", "", messageIds.size()) + .arg(tr("Undelete thread"))); +} + +void MainWindow::restoreSelectedThreads() +{ + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + if (rows.isEmpty()) + return; + + const QStringList threadIds = selectedThreadIds(); + if (threadIds.isEmpty()) + return; + + // Repainted synchronously, as the delete direction is. + for (const QString &threadId : threadIds) + m_model->applyTagChange(threadId, {}, { QStringLiteral("deleted") }); + + m_pendingThreadScope = threadIds; + QMetaObject::invokeMethod( + m_worker, "resolveThreadMessages", Qt::QueuedConnection, + Q_ARG(QStringList, threadIds), + Q_ARG(QString, QStringLiteral("undelete_thread"))); } void MainWindow::restoreSelected() @@ -4260,9 +4507,26 @@ void MainWindow::restoreSelected() } for (auto it = byOrigin.cbegin(); it != byOrigin.cend(); ++it) { - sendMove(it.value(), it.key(), {}, - { QStringLiteral("deleted"), kOriginTagPlaceholder() }, - tr("Undelete")); + // The origin tag is named HERE, not left as the placeholder. + // + // onMessagesMoved() resolves the placeholder from the origin the + // WORKER reports, which is where the message is coming FROM. On a + // delete that is the inbox and correct; on a restore it is the trash, + // so the placeholder resolved to `deleted-from:Trash` and asked to + // remove a tag that never existed, while the real `deleted-from:inbox` + // was never named. The message came home still claiming to have been + // deleted from somewhere, which then made Restore offer to move a + // message that was already back. + // + // A restore does not need the placeholder at all: the origin was just + // read off the message's own tag to decide where to send it, so the + // exact tag to strip is already known. Recomposed from the same + // account-relative form it was stored in. + const QString origin = originTagFor(it.key()); + QStringList remove{ QStringLiteral("deleted") }; + if (!origin.isEmpty()) + remove.append(origin); + sendMove(it.value(), it.key(), {}, remove, tr("Undelete")); } showTransientStatus( @@ -4272,7 +4536,8 @@ void MainWindow::restoreSelected() void MainWindow::sendMove(const QStringList &messageIds, const QString &destFolder, const QStringList &add, const QStringList &remove, - const QString &description) + const QString &description, bool fromUndo, + const QStringList &wholeThreadIds) { if (messageIds.isEmpty() || destFolder.isEmpty()) return; @@ -4286,8 +4551,8 @@ void MainWindow::sendMove(const QStringList &messageIds, // 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_heldMoves.append(HeldMove{ messageIds, destFolder, add, remove, + description, fromUndo }); m_statusLabel->setText( tr("A sync is running; your change will be applied when it " "finishes.")); @@ -4295,9 +4560,61 @@ void MainWindow::sendMove(const QStringList &messageIds, return; } + // Repainted NOW, before the worker is asked. + // + // The write itself waits for the move to be confirmed, and must: tagging + // the database first would leave a message marked deleted in a folder it + // never left if the rename failed. The DISPLAY has no such constraint, and + // holding it back until the round trip finished is what made a deleted row + // sit there unchanged until the user clicked it. The reply rows repainted + // and the root did not, because the replies were separately tagged while + // the root's card reads its thread's summary. + // + // Reverted by revertPendingTagChange() if the write is rejected, exactly + // as the tag path's optimistic update is. + // + // The placeholder is dropped rather than displayed: the real origin is not + // known until the worker answers, and a chip reading the placeholder's + // literal name would be worse than one chip arriving a moment late. + QStringList displayAdd; + for (const QString &tag : add) { + if (tag != kOriginTagPlaceholder()) + displayAdd.append(tag); + } + QStringList displayRemove; + for (const QString &tag : remove) { + if (tag != kOriginTagPlaceholder()) + displayRemove.append(tag); + } + // A thread-scoped move already repainted its rows in + // trashSelectedThreads() / restoreSelectedThreads(), synchronously, before + // the worker was asked to resolve the threads at all. Repeating it here + // would be harmless but redundant; more importantly the caller there needs + // the repaint to happen WITHOUT a worker round trip, which is the whole + // reason it is not done from this function. + // + // applyTagChange() is what those callers use, and applyMessageTagChange() + // is what this one uses, and the difference is not a style choice: the + // former moves the thread's SUMMARY, which a thread row's card draws from, + // while the latter deliberately leaves a multi-message thread's summary + // alone because one message's edit does not describe the conversation. + if (wholeThreadIds.isEmpty()) { + for (const QString &messageId : messageIds) + m_model->applyMessageTagChange(messageId, displayAdd, displayRemove); + } + // 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 }); + // + // A QUEUE, not a map keyed on the destination: two Deletes in the same + // account before the first confirmation arrives both name `acct/Trash`, + // so the second insert overwrote the first and the second confirmation + // took an empty PendingMove. That file landed in the trash carrying + // neither `deleted` nor `deleted-from:`, which makes it unrestorable and + // invisible to a `tag:deleted` query. The worker handles one move at a + // time on its own thread and emits in the order it was asked, so a plain + // FIFO matches confirmations to requests without needing a key at all. + m_pendingMoves.enqueue(PendingMove{ add, remove, description, fromUndo }); QMetaObject::invokeMethod(m_worker, "moveMessages", Qt::QueuedConnection, Q_ARG(QStringList, messageIds), @@ -4307,7 +4624,9 @@ void MainWindow::sendMove(const QStringList &messageIds, void MainWindow::onMessagesMoved(const QMap &originByMessageId, const QString &destFolder) { - const PendingMove pending = m_pendingMoves.take(destFolder); + if (m_pendingMoves.isEmpty()) + return; + const PendingMove pending = m_pendingMoves.dequeue(); if (originByMessageId.isEmpty()) return; @@ -4332,16 +4651,7 @@ void MainWindow::onMessagesMoved(const QMap &originByMessageId // `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); - } + const QString originTag = originTagFor(it.key()); auto resolve = [&](const QStringList &tags) { QStringList out; @@ -4350,24 +4660,50 @@ void MainWindow::onMessagesMoved(const QMap &originByMessageId out.append(tag); continue; } - if (!accountRelative.isEmpty()) { - out.append(QStringLiteral("deleted-from:%1") - .arg(accountRelative)); - } + if (!originTag.isEmpty()) + out.append(originTag); } return out; }; - sendMessageTagChange(it.value(), resolve(pending.add), - resolve(pending.remove), pending.description); + const QStringList resolvedAdd = resolve(pending.add); + const QStringList resolvedRemove = resolve(pending.remove); + sendMessageTagChange(it.value(), resolvedAdd, resolvedRemove, + pending.description); + + // The undo entry carries the RESOLVED tags, and is pushed per origin + // group rather than once for the batch. + // + // It used to be handed pending.add straight, which still holds the + // unresolved placeholder: undo then asked to remove a tag by that + // literal name, which no message carries, so the removal was a silent + // no-op and `deleted-from:inbox` survived the undo. The file came home + // still claiming to have been deleted from somewhere. Same defect as + // the one the second-Delete path had, reached through Ctrl+Z instead. + // + // Per group because the placeholder resolves to a DIFFERENT tag per + // origin: one command for a batch spanning two folders could only + // carry one of them, so the other would be the wrong tag rather than + // merely an unresolved one. + if (!pending.fromUndo) { + QMap groupOrigins; + for (const QString &messageId : it.value()) + groupOrigins.insert(messageId, originByMessageId.value(messageId)); + m_undoStack.push(new MoveCommand(this, groupOrigins, destFolder, + resolvedAdd, resolvedRemove, + 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)); + // The undo entries are pushed inside the loop above, one per origin + // group, because the placeholder resolves per origin. Nothing is pushed + // for a move the undo stack itself started: a MoveCommand is confirmed + // through this same slot, so pushing unconditionally left the undo of a + // Delete putting a fresh command on the stack instead of consuming the + // one it undid, and a second press of undo re-deleted the message. The + // flag rides on PendingMove because the answer has to survive the queued + // round trip; a window-wide "am I undoing" flag would long since have + // been cleared by the time the worker replies. } void MainWindow::sendThreadTagChange(const QStringList &threadIds, diff --git a/src/mainwindow.h b/src/mainwindow.h index ada4845..ae87868 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -754,18 +755,69 @@ private: /// 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. + /// `fromUndo` marks a move the undo stack itself started, which must NOT + /// push a command of its own when it is confirmed. See onMessagesMoved(). + /// `wholeThreadIds`, when non-empty, says this move covers every message + /// of those threads, so the optimistic repaint updates each thread's + /// SUMMARY rather than each message's node. A thread row's card reads the + /// summary, so a thread-scoped move that updated only nodes repainted the + /// replies and left the root card stale until the next query. void sendMove(const QStringList &messageIds, const QString &destFolder, const QStringList &add, const QStringList &remove, - const QString &description); + const QString &description, bool fromUndo = false, + const QStringList &wholeThreadIds = {}); /// Moves each selected row's message to its account's trash, tagging it /// `deleted` and recording where it came from. void trashSelected(); + /// The half of trashSelected() that does the work, given the messages and + /// their paths. + /// + /// Paths are passed in rather than looked up, because the thread-scoped + /// caller has messages the MODEL has never seen: an unexpanded thread + /// holds no node for its replies, so a model lookup resolves them to no + /// account and the move is silently dropped. The worker supplies them. + void trashMessages(const QStringList &messageIds, + const QHash &pathById, + int messageCount, + const QStringList &wholeThreadIds = {}); + + /// Moves every message of each selected THREAD to its account's trash. + /// + /// Asynchronous, unlike its message-scoped twin: the ids and paths of an + /// unexpanded thread's messages live only in the database, so this asks + /// the worker and finishes in onThreadMessagesResolved(). + void trashSelectedThreads(); + + /// The thread ids the selection covers, resolving a reply row to its own + /// thread. scopeFor() reports a reply under messageIds instead, which left + /// a thread action on a reply row doing nothing at all. + QStringList selectedThreadIds() const; + + /// The inverse of trashSelectedThreads(): moves every message of each + /// selected thread back where it came from. + void restoreSelectedThreads(); + + /// Runs the thread-scoped delete once the worker has resolved the + /// threads to messages. + void onThreadMessagesResolved(const QStringList &messageIds, + const QStringList &paths, + const QStringList &tags, + const QString &requestTag); + /// The inverse: moves each selected row's message back to the folder its /// `deleted-from:` tag names, stripping both tags. void restoreSelected(); + /// The `deleted-from:` tag naming `dbRelativeFolder`, or empty when no + /// account owns it. + /// + /// One rule for both sites that need the tag: the delete that writes it + /// and the restore that strips it. Deriving it twice let them disagree, + /// and a restore stripped a tag that had never been written. + QString originTagFor(const QString &dbRelativeFolder) const; + /// The account whose maildir contains `path`, or an invalid account when /// no configured maildir does. /// @@ -781,13 +833,27 @@ private: 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. + /// + /// A FIFO and not a map keyed on the destination: two Deletes in one + /// account before the first confirmation arrives name the same folder, so + /// a keyed map dropped the first entry and left the second confirmation + /// with nothing to apply. That file reached the trash carrying neither + /// `deleted` nor `deleted-from:`, unrestorable and invisible to a + /// `tag:deleted` query. The worker moves one batch at a time and emits in + /// request order, so position alone matches a confirmation to its request. struct PendingMove { QStringList add; QStringList remove; QString description; + /// Set for a move the undo stack started, which must not push again. + bool fromUndo = false; }; - QHash m_pendingMoves; + QQueue m_pendingMoves; + + /// The threads a resolveThreadMessages() request was made for, held until + /// the answer arrives so the optimistic repaint knows the move is + /// thread-scoped. + QStringList m_pendingThreadScope; /// Undoes the optimistic model update for a write the worker rejected. void revertPendingTagChange(); @@ -856,6 +922,9 @@ private: QStringList add; QStringList remove; QString description; + /// Carried through the hold, or a move undone during a sync would + /// push a command when it is finally flushed. + bool fromUndo = false; }; QVector m_heldMoves; @@ -1302,8 +1371,10 @@ public: m_firstRedo = false; return; } + // Also fromUndo: a redo replays a command that is ALREADY on the + // stack, so confirming it must not push a duplicate either. m_window->sendMove(m_origins.keys(), m_dest, m_add, m_remove, - m_description); + m_description, true); } void undo() override @@ -1317,8 +1388,12 @@ public: byOrigin[it.value()].append(it.key()); } for (auto it = byOrigin.cbegin(); it != byOrigin.cend(); ++it) { + // fromUndo: this move is the undo, so its confirmation must not + // push a command of its own. Without it the stack grew on every + // press and a second undo re-deleted the message. m_window->sendMove(it.value(), it.key(), m_remove, m_add, - QStringLiteral("Undo %1").arg(m_description)); + QStringLiteral("Undo %1").arg(m_description), + true); } } diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index f4ad6a8..a17eba1 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -16,6 +16,7 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include #include #include @@ -363,6 +364,14 @@ private slots: void undoMovesTheMessageBack(); void deleteOnAReplyMovesThatReplyOnly(); void deleteWithoutATrashFolderSaysSoRatherThanDoingNothing(); + void undoingADeleteConsumesItsCommandRatherThanPushingAnother(); + void aDeleteHeldDuringASyncCountsAsUnsyncedWork(); + void twoDeletesToOneTrashBothGetTheirTags(); + void deletingTwiceLeavesNoOriginTagBehind(); + void undoOfADeleteRemovesTheOriginTagToo(); + void deletingAThreadRootTwiceRestoresItRatherThanRedeleting(); + void deleteThreadMovesEveryMessageAndRepaintsTheRootCard(); + void aFolderNameWithASpaceSurvivesTheRoundTrip(); private: /// Owns the throwaway lock table init() points every test at. A pointer @@ -4782,15 +4791,27 @@ void TestMainWindow::deleteOnAReplyReadsItsOwnThreadNotTheFirstInTheList() "the fixture did not produce a reply row at row 0, so this test " "would assert nothing about item 88's trap"); + // t2 is the reply's thread and is NOT deleted, so the correct direction + // is Delete. Reading t1's state instead would choose Undelete. + QVERIFY2(!model->threadAt(1).isDeleted(), + "the fixture's second thread is already deleted, so both " + "directions would look alike and this test would assert nothing"); + action->trigger(); - QCOMPARE(window.undoDepthForTesting(), 1); - QVERIFY2(window.undoTextForTesting().contains(QStringLiteral("Delete")), - qPrintable(QStringLiteral( - "Delete on a reply of an undeleted thread chose " - "the wrong direction: %1. It read the FIRST " - "thread's state, which is deleted.") - .arg(window.undoTextForTesting()))); + // Asserted on the MODEL, not on the undo stack. Delete thread MOVES since + // item 103's follow-up, and the undo entry is pushed once the worker + // confirms the move, which this bare window has no database to perform. + // The DIRECTION is chosen synchronously and is what item 88's trap was + // about: the repaint below happens only on the delete direction. + QVERIFY2(model->threadAt(1).isDeleted(), + "Delete on a reply of an undeleted thread chose the wrong " + "direction: it read the FIRST thread's state, which is deleted"); + // And the OTHER thread is untouched: the action must act on the reply's + // own conversation, not on both. + QVERIFY2(model->threadAt(0).isDeleted(), + "the fixture's first thread stopped being deleted, which means " + "the action reached a thread it was never pointed at"); } void TestMainWindow::toggleUnreadOnAReplyReadsItsOwnThreadNotTheFirstInTheList() @@ -5356,18 +5377,24 @@ void TestMainWindow::anActionOnAThreadRowActsOnTheMessageItDisplays() // The thread action is how the conversation is reached, and it must still // work from the same selection. + // + // Asserted on the MODEL rather than on a pending write. Delete thread + // MOVES every message since item 103's follow-up, and a move needs ids and + // paths that only the database holds for a thread this bare window never + // expanded, so the write is issued after a worker round trip that never + // completes here. What is synchronous, and what this test is about, is the + // scope: the whole thread is marked, not the one message its card shows. auto *deleteThread = window.findChild(QStringLiteral("delete_thread")); QVERIFY(deleteThread); + QVERIFY2(!model->threadAt(0).isDeleted(), + "the thread already read as deleted, so the check below would " + "pass without the action doing anything"); deleteThread->trigger(); - QCOMPARE(window.pendingThreadIdsForTesting(), - QStringList{ QStringLiteral("t1") }); - - // Two commands, one per gesture, each recording the scope it used: a thread - // action that pushed the message command would undo a fraction of what it - // did. - QCOMPARE(window.undoDepthForTesting(), 2); + QVERIFY2(model->threadAt(0).isDeleted(), + "Delete thread did not mark the whole thread, so the card paints " + "undeleted until the row is clicked"); } void TestMainWindow::theThreadSubmenuIsReachableFromBothMenus() @@ -8753,6 +8780,29 @@ void TestMainWindow::aSingleMessageIdQuerysCardOpensInTheMessagePane() /// 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". +/// Counts messages matching `query` in the fixture's database, by running +/// notmuch itself. +/// +/// Asked directly rather than through the query bar because the UI's +/// rowCount() reads 0 for the whole interval before the worker answers, so an +/// assertion that a tag is ABSENT is satisfied by the gap before any answer +/// arrives and passes against a database that still carries the tag. +static int notmuchCount(const QString &configPath, const QString &query) +{ + QProcess process; + QProcessEnvironment env = QProcessEnvironment::systemEnvironment(); + env.insert(QStringLiteral("NOTMUCH_CONFIG"), configPath); + process.setProcessEnvironment(env); + process.start(QStringLiteral("notmuch"), + { QStringLiteral("count"), query }); + if (!process.waitForFinished(15000)) + return -1; + bool ok = false; + const int count = + QString::fromUtf8(process.readAllStandardOutput()).trimmed().toInt(&ok); + return ok ? count : -1; +} + static bool folderHasMessageFile(const QString &dir, const QString &stem) { QDir directory(dir); @@ -8882,6 +8932,531 @@ void TestMainWindow::deleteRecordsWhereTheMessageCameFrom() QLatin1Char(' '))))); } +void TestMainWindow::deletingTwiceLeavesNoOriginTagBehind() +{ + // Delete twice is the ordinary way back: the action toggles, so a second + // press on a deleted message restores it. That path is NOT the undo path + // and had its own defect. + // + // onMessagesMoved() resolved the origin placeholder from the folder the + // WORKER reported, which is where the message came FROM. On a delete that + // is the inbox and correct. On a restore it is the TRASH, so the restore + // asked to remove `deleted-from:Trash`, a tag that had never been written, + // while the real `deleted-from:inbox` was never named and stayed on the + // message. It came home still claiming to have been deleted from + // somewhere, which makes Restore offer to move a message already at home. + // + // Reported from a hand test. The undo test passed throughout, because undo + // carries its tags on the command and never resolves a placeholder. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("twice@example.org"), + QStringLiteral("Delete me twice"), 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("twice.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); + + // The origin tag really was written, so the assertion after the second + // delete is about it being REMOVED rather than never having existed. + queryEdit->setText(QStringLiteral( + "id:twice@example.org and tag:\"deleted-from:inbox\"")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + + // Second press on the same message, which restores it. + queryEdit->setText(QStringLiteral("id:twice@example.org")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete"))->trigger(); + + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"), stem) + || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), + stem), + 15000); + + // The file arriving is NOT the end of the restore. The tags are written + // only once the worker confirms the move, so the writes land after the + // rename the assertion above waits for. Querying in that gap reads the + // state before the restore finished tagging, which is how an earlier + // version of this test passed against the bug it exists to catch. + // + // Waited on the `deleted` tag, which the restore removes on every code + // path, rather than on a fixed sleep. + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(backed.fixture().configPath(), + QStringLiteral("id:twice@example.org and tag:deleted")) + == 0, + 15000); + + // BOTH tags gone, asked of the database. `deleted-from:` left behind is + // the defect this covers, and it survived a green suite before. + // The origin tag specifically, asserted on its OWN query. + // + // A combined `tag:deleted or tag:"deleted-from:inbox"` query is NOT + // equivalent and passed against the bug: `deleted` is removed correctly + // and promptly, so the disjunction went to zero on that term alone while + // the origin tag was still on the message. Split, so the assertion can + // only be satisfied by the tag it names. + // Asked of notmuch DIRECTLY, not through the query bar. + // + // A UI query cannot answer this reliably: rowCount() is 0 for the whole + // interval before the worker replies, so QTRY_VERIFY(rowCount() == 0) is + // satisfied instantly by the empty pre-result and passes against any + // state of the database. Measured while building this test: 0 right after + // returnPressed(), 1 once the answer actually landed. The database is the + // thing under test here, so it is asked directly. + const QString cfg = backed.fixture().configPath(); + + // The message still exists: an assertion that a tag is absent would be + // satisfied just as well by the message having vanished. + QCOMPARE(notmuchCount(cfg, QStringLiteral("id:twice@example.org")), 1); + + // The origin tag is gone. This is the defect: it used to survive the + // restore, because the placeholder resolved to `deleted-from:Trash`, the + // folder the message was coming FROM, and stripped a tag that had never + // been written. + QCOMPARE(notmuchCount(cfg, + QStringLiteral("id:twice@example.org and " + "tag:\"deleted-from:inbox\"")), + 0); + + // And no tag naming the trash was invented in its place. + QCOMPARE(notmuchCount(cfg, + QStringLiteral("id:twice@example.org and " + "tag:\"deleted-from:Trash\"")), + 0); + + // `deleted` itself, so a fix that dropped this one instead cannot hide. + QCOMPARE(notmuchCount( + cfg, QStringLiteral("id:twice@example.org and tag:deleted")), + 0); + +} + +void TestMainWindow::undoOfADeleteRemovesTheOriginTagToo() +{ + // Ctrl+Z is a THIRD way back, beside the second Delete, and it had the + // same defect for a different reason. + // + // MoveCommand was constructed with pending.add, which still holds the + // unresolved origin PLACEHOLDER: onMessagesMoved() resolved the + // placeholder for the tags it wrote to the database, but handed the undo + // command the raw list. Undo then asked to remove a tag by the + // placeholder's literal name, which no message carries, so the removal + // was a silent no-op and `deleted-from:inbox` survived. The message came + // home still claiming to have been deleted from somewhere, which makes + // Restore offer to move a message that is already at home. + // + // Reported from a hand test after the second-Delete path was fixed: that + // fix did not touch this one, and the existing undo test asserted on the + // file's location rather than on its tags. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("undotag@example.org"), + QStringLiteral("Undo my tags"), 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("undotag.example.org"); + const QString cfg = backed.fixture().configPath(); + + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete"))->trigger(); + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(root + QStringLiteral("/acct/Trash/cur"), stem), + 15000); + + // The origin tag really was written, so the assertion after the undo is + // about it being REMOVED rather than never having existed. + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, QStringLiteral("id:undotag@example.org and " + "tag:\"deleted-from:inbox\"")) == 1, + 15000); + + window.findChild(QStringLiteral("undo"))->trigger(); + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"), stem) + || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), + stem), + 15000); + + // The file arriving is not the end of the undo: the tags are written only + // once the worker confirms the move, so they land after the rename. + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, + QStringLiteral("id:undotag@example.org and tag:deleted")) + == 0, + 15000); + + // Asked of notmuch directly. A UI query cannot answer this: rowCount() is + // 0 for the whole interval before the worker replies, so an assertion + // that a tag is absent is satisfied by the gap before any answer arrives. + QCOMPARE(notmuchCount(cfg, QStringLiteral("id:undotag@example.org")), 1); + QCOMPARE(notmuchCount(cfg, + QStringLiteral("id:undotag@example.org and " + "tag:\"deleted-from:inbox\"")), + 0); + QCOMPARE(notmuchCount(cfg, + QStringLiteral("id:undotag@example.org and " + "tag:\"deleted-from:Trash\"")), + 0); +} + +void TestMainWindow::deletingAThreadRootTwiceRestoresItRatherThanRedeleting() +{ + // The toggle asked a THREAD ROW about its thread's tags, which notmuch + // gives as a UNION over the conversation. Delete the root of a + // three-message thread and the two replies are untouched, so the union + // carries no `deleted`, so a second press read the row as not-deleted and + // ran Delete AGAIN: the message was moved trash-to-trash and came out + // carrying `deleted`, `deleted-from:inbox` AND `deleted-from:Trash`, with + // no way back, since a later restore would send it to the trash it now + // claims to have come from. + // + // The union was a documented approximation, called bounded because the + // worst case for a TAG toggle was re-applying a tag the message already + // had, which is a no-op. A MOVE re-applies the move. The comment outlived + // the code it described. + // + // The row must be left ALONE between the two presses: a re-query rebuilds + // it from the database and hides the defect, which is why an earlier + // version of this probe passed. The user's gesture is two presses on the + // list as it stands. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("troot@example.org"), + QStringLiteral("Thread root"), QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Root body."))); + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("trep1@example.org"), + QStringLiteral("Re: Thread root"), QStringLiteral("other@example.org"), + QStringLiteral("Fri, 14 Aug 2026 11:00:00 +0200"), + QStringLiteral("Reply one."), true, + QStringLiteral("troot@example.org"))); + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("trep2@example.org"), + QStringLiteral("Re: Thread root"), QStringLiteral("third@example.org"), + QStringLiteral("Fri, 14 Aug 2026 12:00:00 +0200"), + QStringLiteral("Reply two."), true, + QStringLiteral("troot@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()) == 1, 15000); + + const QString root = backed.fixture().maildirPath(); + const QString cfg = backed.fixture().configPath(); + const QString stem = QStringLiteral("troot.example.org"); + const QString trash = root + QStringLiteral("/acct/Trash/cur"); + + // Three messages, so the union genuinely differs from the root's own + // tags. With one message the two are identical and the defect cannot + // appear at all. + QCOMPARE(notmuchCount(cfg, QStringLiteral("thread:{id:troot@example.org}")), + 3); + + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete"))->trigger(); + QTRY_VERIFY_WITH_TIMEOUT(folderHasMessageFile(trash, stem), 15000); + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, QStringLiteral("id:troot@example.org and " + "tag:\"deleted-from:inbox\"")) == 1, + 15000); + + // Only the root moved. The replies are what make the union disagree, so + // this is also the guard the rest of the test depends on. + QCOMPARE(notmuchCount(cfg, QStringLiteral("id:trep1@example.org and " + "tag:deleted")), + 0); + QCOMPARE(notmuchCount(cfg, QStringLiteral("id:trep2@example.org and " + "tag:deleted")), + 0); + + // Second press on the row as it stands, no re-query. + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete"))->trigger(); + + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"), stem) + || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), + stem), + 15000); + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, + QStringLiteral("id:troot@example.org and tag:deleted")) + == 0, + 15000); + + // Asked of notmuch directly: a UI query reads 0 rows for the whole + // interval before the worker answers, so an absence assertion through the + // query bar passes against any state of the database. + QCOMPARE(notmuchCount(cfg, QStringLiteral("id:troot@example.org")), 1); + QCOMPARE(notmuchCount(cfg, QStringLiteral("id:troot@example.org and " + "tag:\"deleted-from:inbox\"")), + 0); + // The tag the re-delete invented. Its presence is the signature of this + // defect rather than a variation on the origin-tag ones. + QCOMPARE(notmuchCount(cfg, QStringLiteral("id:troot@example.org and " + "tag:\"deleted-from:Trash\"")), + 0); + QVERIFY2(!folderHasMessageFile(trash, stem), + "the second press left the message in the trash"); +} + +void TestMainWindow::deleteThreadMovesEveryMessageAndRepaintsTheRootCard() +{ + // Two defects in one gesture, both reported from a hand test. + // + // Delete thread never moved anything: it was left calling tagSelected() + // when Delete became a move, so a whole conversation stayed in the inbox + // wearing a `deleted` chip, which is the half-deleted state item 103 + // existed to remove. It moves every message now, each carrying its own + // `deleted-from:` origin so a thread spanning folders reassembles. + // + // And the ROOT card did not repaint until it was clicked, while its + // replies did. A thread-scoped move updated each message's node; + // applyMessageTagChange() deliberately leaves a multi-message thread's + // SUMMARY alone, because one message's edit does not describe the + // conversation. The replies have nodes and repainted; the root card reads + // the summary and did not. A thread-scoped move DID change every message, + // so the summary genuinely moves and applyTagChange() is the right update. + // + // The stale summary was also why a second press did nothing: the toggle + // asks the summary for its direction and kept reading "not deleted". + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("dt0@example.org"), + QStringLiteral("DT root"), QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Root body."))); + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("dt1@example.org"), + QStringLiteral("Re: DT root"), QStringLiteral("other@example.org"), + QStringLiteral("Fri, 14 Aug 2026 11:00:00 +0200"), + QStringLiteral("Reply one."), true, QStringLiteral("dt0@example.org"))); + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("dt2@example.org"), + QStringLiteral("Re: DT root"), QStringLiteral("third@example.org"), + QStringLiteral("Fri, 14 Aug 2026 12:00:00 +0200"), + QStringLiteral("Reply two."), true, QStringLiteral("dt0@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()) == 1, 15000); + + const QString root = backed.fixture().maildirPath(); + const QString cfg = backed.fixture().configPath(); + const QString trash = root + QStringLiteral("/acct/Trash/cur"); + const QString thread = QStringLiteral("thread:{id:dt0@example.org}"); + + // Three messages, so a thread-scoped action is distinguishable from a + // message-scoped one at all. + QCOMPARE(notmuchCount(cfg, thread), 3); + + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + view->expand(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete_thread"))->trigger(); + + // Every message MOVED, not merely tagged. This is the half that was + // missing entirely: the action tagged and moved nothing. + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(trash, QStringLiteral("dt0.example.org")) + && folderHasMessageFile(trash, QStringLiteral("dt1.example.org")) + && folderHasMessageFile(trash, QStringLiteral("dt2.example.org")), + 15000); + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, thread + QStringLiteral(" and tag:deleted")) == 3, + 15000); + // Each with its own origin, which is what makes the move reversible. + QCOMPARE(notmuchCount(cfg, thread + + QStringLiteral(" and " + "tag:\"deleted-from:inbox\"")), + 3); + + // The ROOT CARD's own state, which is what the user watches. Read from the + // summary because that is what a thread row draws, and it is the value + // that stayed stale: the replies repainted and the root did not. + QVERIFY2(model->threadAt(0).tags.contains(QStringLiteral("deleted")), + "the root card still reads as not deleted, so it paints " + "undeleted until the row is clicked"); + + // Second press restores the whole thread, which only works if the toggle + // can see the state the first press produced. + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete_thread"))->trigger(); + + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, thread + QStringLiteral(" and tag:deleted")) == 0, + 15000); + + // Home, and nothing left behind in the trash. + QCOMPARE(notmuchCount(cfg, thread), 3); + QCOMPARE(notmuchCount(cfg, thread + + QStringLiteral(" and " + "tag:\"deleted-from:inbox\"")), + 0); + QVERIFY(!folderHasMessageFile(trash, QStringLiteral("dt0.example.org"))); + QVERIFY(!folderHasMessageFile(trash, QStringLiteral("dt1.example.org"))); + QVERIFY(!folderHasMessageFile(trash, QStringLiteral("dt2.example.org"))); +} + +void TestMainWindow::aFolderNameWithASpaceSurvivesTheRoundTrip() +{ + // A notmuch tag MAY contain a space, and a Maildir folder name may too. + // The worker reported each message's tags as one space-joined string, so + // `deleted-from:Inbox/SlackBuilds users` was split back into + // "deleted-from:Inbox/SlackBuilds" and "users", and Restore moved the + // messages to the truncated folder, CREATING it. On the user's real + // Maildir that put four messages into a directory mbsync does not sync, + // beside the real folder of 808, and they read as missing. + // + // The leftover origin tag was the visible half: the restore stripped the + // truncated name, which no message carried, so the real tag stayed on. + // + // Separator is a TAB now. A tag cannot contain one, since notmuch's own + // dump format is line-based and whitespace-delimited. + WorkerBackedWindow backed; + const QString folder = QStringLiteral("acct/Inbox/SlackBuilds users"); + QVERIFY(backed.fixture().addMessage( + folder, QStringLiteral("sp0@example.org"), QStringLiteral("SP root"), + QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Root body."))); + QVERIFY(backed.fixture().addMessage( + folder, QStringLiteral("sp1@example.org"), + QStringLiteral("Re: SP root"), QStringLiteral("other@example.org"), + QStringLiteral("Fri, 14 Aug 2026 11:00:00 +0200"), + QStringLiteral("Reply."), true, QStringLiteral("sp0@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()) == 1, 15000); + + const QString root = backed.fixture().maildirPath(); + const QString cfg = backed.fixture().configPath(); + const QString thread = QStringLiteral("thread:{id:sp0@example.org}"); + const QString home = root + QLatin1Char('/') + folder; + + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete_thread"))->trigger(); + + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, thread + QStringLiteral(" and tag:deleted")) == 2, + 15000); + + // The origin tag carries the WHOLE folder name, space included. + QCOMPARE(notmuchCount(cfg, + thread + + QStringLiteral(" and tag:\"deleted-from:" + "Inbox/SlackBuilds users\"")), + 2); + + // Back again. + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete_thread"))->trigger(); + + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, thread + QStringLiteral(" and tag:deleted")) == 0, + 15000); + + // No origin tag left behind. This is the half the user saw: a tag they + // could see, could not type, and could not remove. + QCOMPARE(notmuchCount(cfg, + thread + + QStringLiteral(" and tag:\"deleted-from:" + "Inbox/SlackBuilds users\"")), + 0); + // Nor a truncated one, which is what a space-split would have written. + QCOMPARE(notmuchCount(cfg, + thread + + QStringLiteral(" and tag:\"deleted-from:" + "Inbox/SlackBuilds\"")), + 0); + + // Home, in the folder with the space in its name. + QVERIFY2(folderHasMessageFile(home + QStringLiteral("/cur"), + QStringLiteral("sp0.example.org")), + "the root did not come back to the folder it was deleted from"); + QVERIFY2(folderHasMessageFile(home + QStringLiteral("/cur"), + QStringLiteral("sp1.example.org")), + "the reply did not come back to the folder it was deleted from"); + + // And the truncated folder was never created. Its existence is the defect + // that hid four real messages from the user and from mbsync. + QVERIFY2(!QDir(root + QStringLiteral("/acct/Inbox/SlackBuilds")).exists(), + "a folder named after the truncated origin was created, so the " + "messages are somewhere mbsync will never sync"); +} + void TestMainWindow::undoMovesTheMessageBack() { // Undo is this project's answer to the confirmation dialog it rules out, @@ -9024,6 +9599,182 @@ void TestMainWindow::deleteOnAReplyMovesThatReplyOnly() "deleting a reply moved its thread's root as well"); } +void TestMainWindow::undoingADeleteConsumesItsCommandRatherThanPushingAnother() +{ + // A move is confirmed through onMessagesMoved(), and so is the move an + // UNDO makes. Pushing a command there unconditionally meant undo left a + // fresh command on the stack instead of consuming the one it undid, so + // the stack grew on every press: "Delete", "Undo Delete", "Undo Undo + // Delete". A user pressing undo twice to be sure re-deleted the mail they + // had just rescued, which is the opposite of what undo is for here, undo + // being this project's stand-in for a confirmation dialog. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("undo2@example.org"), + QStringLiteral("Undo twice"), 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("undo2.example.org"); + const QString trash = root + QStringLiteral("/acct/Trash/cur"); + const auto inInbox = [&] { + return folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"), + stem) + || folderHasMessageFile( + root + QStringLiteral("/acct/inbox/new"), stem); + }; + + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete"))->trigger(); + QTRY_VERIFY_WITH_TIMEOUT(folderHasMessageFile(trash, stem), 15000); + + // The guard the assertions below need: one command, from the delete. + QTRY_VERIFY_WITH_TIMEOUT(window.undoDepthForTesting() == 1, 15000); + + window.findChild(QStringLiteral("undo"))->trigger(); + QTRY_VERIFY_WITH_TIMEOUT(inInbox(), 15000); + + // The stack is spent. Asserted on undoText rather than depth alone + // because a command that is merely marked done still reports its text, + // and it is the text the user reads off the Edit menu. + QTRY_VERIFY_WITH_TIMEOUT(window.undoTextForTesting().isEmpty(), 15000); + + // And the real point: pressing undo again must not move the message + // anywhere. Before the fix this put it straight back in the trash. + window.findChild(QStringLiteral("undo"))->trigger(); + QTest::qWait(1500); + QVERIFY2(!folderHasMessageFile(trash, stem), + "a second undo re-deleted the message the first one restored"); + QVERIFY2(inInbox(), "a second undo moved the message out of the inbox"); +} + +void TestMainWindow::aDeleteHeldDuringASyncCountsAsUnsyncedWork() +{ + // pendingEditCount() summed the held TAG edits and not the held MOVES, so + // a Delete pressed during a sync left the count at zero: the indicator + // stayed hidden and closeEvent()'s `pendingEditCount() > 0` guard never + // fired, discarding the move on quit with no prompt. That is item 106's + // data loss with a worse shape, since a dropped move leaves the file in + // the folder the user asked it out of. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("held1@example.org"), + QStringLiteral("Held by a sync"), 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")); + auto *label = window.findChild(QStringLiteral("pendingEdits")); + QVERIFY(model && view && queryEdit && label); + + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + + // Hidden before the gesture, so the assertion after it means something. + QVERIFY2(label->isHidden(), "the pending indicator was already showing"); + + // A sync now holds the write lock, which is what makes the move held + // rather than sent. + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Running)); + + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete"))->trigger(); + + QVERIFY2(!label->isHidden(), + "a Delete held by a sync did not count as unsynced work, so " + "quitting would have discarded it with no prompt"); + + // The file really is still where it was: this is a HELD move, not a + // failed one, and the indicator would be meaningless otherwise. + const QString root = backed.fixture().maildirPath(); + QVERIFY(!folderHasMessageFile(root + QStringLiteral("/acct/Trash/cur"), + QStringLiteral("held1.example.org"))); +} + +void TestMainWindow::twoDeletesToOneTrashBothGetTheirTags() +{ + // The pending-move table was keyed on the destination folder, so two + // Deletes in one account before the first confirmation arrived both named + // `acct/Trash`: the second insert overwrote the first and the second + // confirmation took an empty entry. That file reached the trash carrying + // neither `deleted` nor `deleted-from:`, which makes it unrestorable by + // Restore and invisible to a `tag:deleted` query. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("two1@example.org"), + QStringLiteral("First"), QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body text."))); + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("two2@example.org"), + QStringLiteral("Second"), QStringLiteral("other@example.org"), + QStringLiteral("Fri, 14 Aug 2026 11: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()) == 2, 15000); + + // Both Deletes issued back to back, WITHOUT waiting for the first to be + // confirmed. That is the whole point: waiting would serialise them and + // the keyed table would have coped. + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete"))->trigger(); + view->setCurrentIndex(model->index(1, 0, QModelIndex())); + window.findChild(QStringLiteral("delete"))->trigger(); + + const QString root = backed.fixture().maildirPath(); + const QString trash = root + QStringLiteral("/acct/Trash/cur"); + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(trash, QStringLiteral("two1.example.org")) + && folderHasMessageFile(trash, QStringLiteral("two2.example.org")), + 15000); + + // Both carry BOTH tags, asked of the database rather than of the model: + // the defect was a write that never happened, and the model would have + // shown the optimistic state either way. + queryEdit->setText(QStringLiteral( + "tag:deleted and tag:\"deleted-from:inbox\" and " + "(id:two1@example.org or id:two2@example.org)")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 2, 15000); +} + void TestMainWindow::deleteWithoutATrashFolderSaysSoRatherThanDoingNothing() { // Task 2 warns at config load. This is the second line of defence: a key diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index 89886d0..cc854b2 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -132,20 +132,6 @@ Unsynced changes Modifiche non sincronizzate - - %n tag change(s) have not been synced, and no sync command is configured. Quit anyway? - - %n modifica alle etichette non è stata sincronizzata e non è configurato alcun comando di sincronizzazione. Uscire comunque? - %n modifiche alle etichette non sono state sincronizzate e non è configurato alcun comando di sincronizzazione. Uscire comunque? - - - - %n tag change(s) have not been synced. - - %n modifica alle etichette non è stata sincronizzata. - %n modifiche alle etichette non sono state sincronizzate. - - Sync before quitting? Sincronizzare prima di uscire? @@ -262,6 +248,10 @@ Add or remove the deleted tag Aggiunge o rimuove l'etichetta deleted + + Changes made here that a sync has not yet carried to the mail store. An external notmuch run can clear them without this count noticing. + Modifiche fatte qui che nessuna sincronizzazione ha ancora trasferito all'archivio di posta. Un'esecuzione esterna di notmuch può azzerarle senza che questo conteggio se ne accorga. + %n message(s) could not be deleted: no trash folder is configured for their account. @@ -277,6 +267,20 @@ Delete Elimina + + %n change(s) have not been synced, and no sync command is configured. Quit anyway? + + %n modifica non è stata sincronizzata e non è configurato alcun comando di sincronizzazione. Uscire comunque? + %n modifiche non sono state sincronizzate e non è configurato alcun comando di sincronizzazione. Uscire comunque? + + + + %n change(s) have not been synced. + + %n modifica non è stata sincronizzata. + %n modifiche non sono state sincronizzate. + + Mark &spam Segna come &spam @@ -361,10 +365,6 @@ Undelete thread Ripristina conversazione - - Delete thread - Elimina conversazione - Mark thread as &spam Segna conversazione come &spam @@ -850,10 +850,6 @@ %n modifiche non sincronizzate - - Tag changes made here that a sync has not yet carried to the mail store. An external notmuch run can clear them without this count noticing. - Modifiche alle etichette fatte qui che nessuna sincronizzazione ha ancora trasferito all'archivio di posta. Un'esecuzione esterna di notmuch può azzerarle senza che questo conteggio se ne accorga. - &Whole thread &Intera conversazione -- cgit v1.2.3 From 601159309118cf65c73f5f50bb3cf216be9f1cbb Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 18 Aug 2026 12:35:05 +0200 Subject: feat(trash): restore mail from the trash view Task 6. Delete moved mail into the trash and the only ways back out were a second press of Delete or Ctrl+Z, both of which act on a row the user has to have deleted in this session. Browsing the trash and putting something back needed an action of its own. `restore` is enabled from the QUERY, not from the selection's tags. The trash view is path-based precisely so that mail trashed by another client appears in it, and such a message carries no tag of ours: deciding from `tag:deleted` would disable Restore on exactly the messages that most need it. isShowingTrash() compares the current query against the trash generator's own, for both the per-account and the all-accounts scope, so it follows the account dropdown like every other filter. A message with NO origin tag is the foreign-trashed case, and it is why this is not simply restoreSelected() under a new name. The two callers want opposite things from a missing origin, which `fallbackToInbox` selects. From the trash view the message is demonstrably in the trash and refusing to move it leaves the user looking at mail they cannot get out, so it goes to the inbox and the status bar says so. From a second press of Delete the message is not in the trash at all and merely wears a stale `deleted` tag from an older version or a hand-written notmuch command; moving that to the inbox would relocate mail the user never asked to move, so the tag comes off and the file stays put. The inbox FOLDER is a new optional per-account `inbox` key, defaulting to "Inbox". It is configurable rather than hardcoded because the name is not ours to assume: naming a folder that does not exist CREATES it, beside the real one, and under mbsync's `Create Both` that folder reaches the mail server. That is not hypothetical, it is what a truncated origin folder did to real mail while this branch was being tested. Unlike `trash` the key is optional, since the default is right for any ordinary Maildir and a wrong value here only affects the fallback. Ctrl+R, which was free. The action is only enabled in the trash view, so the key is inert elsewhere rather than doing something surprising. It sits in the Message menu beside Delete and in the thread context menu, greyed outside the trash rather than hidden: an action that vanishes teaches nothing, while a disabled entry with its shortcut beside it says both that it exists and where it applies. **Adding an action is FIVE places, not four.** knownActions(), defaultBindings() and the icon table are each enforced by a test that fails loudly, and being REACHABLE is a fifth that nothing checked: this shipped registered, bound, iconned, correctly enabled, and present in no menu at all, which a green suite reported as complete. Ctrl+R is not a shortcut anyone guesses, so it was effectively invisible. restoreIsReachableWithoutTheKeyboard() closes that, and deliberately excludes the context menu from its menu-bar assertion, since findChildren returns both and one check would otherwise satisfy the other. Four tests, each mutation-checked. Two worth keeping: the hardcoded "Inbox" mutation fails against the fixture's lowercase folders exactly as it would against a Maildir that spells its inbox differently, and the reachability mutation reproduces the keyboard-only state this shipped in. Co-Authored-By: Claude Opus 5 --- src/config.cpp | 19 ++++ src/config.h | 21 ++++ src/keymap.cpp | 4 + src/mainwindow.cpp | 135 +++++++++++++++++++++++-- src/mainwindow.h | 23 ++++- tests/test_mainwindow.cpp | 211 ++++++++++++++++++++++++++++++++++++++++ translations/qtmaildir_it_IT.ts | 26 +++++ 7 files changed, 429 insertions(+), 10 deletions(-) (limited to 'translations') diff --git a/src/config.cpp b/src/config.cpp index 1799ac6..a2d1cec 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -143,6 +143,18 @@ QString Account::trashQuery() const return folderQuery(maildir, trash); } +QString Account::inboxFolder() const +{ + // Never empty: Restore needs a folder to name, and "Inbox" is both the + // Maildir convention and what mbsync's own Inbox directive defaults to. + return inbox.isEmpty() ? QStringLiteral("Inbox") : inbox; +} + +QString Account::inboxQuery() const +{ + return folderQuery(maildir, inboxFolder()); +} + QString Config::allSentQuery() const { return joinAccountQueries(m_accounts, &Account::sentQuery); @@ -450,6 +462,13 @@ void Config::load(const QString &path) account.trash = settings.value(QStringLiteral("trash")).toString().trimmed(); + // Optional, unlike trash: inboxFolder() defaults it to "Inbox", which + // is right for any ordinary Maildir. Read so an account whose inbox is + // named otherwise can say so, rather than having Restore create a + // second folder under a name this program assumed. + account.inbox = + settings.value(QStringLiteral("inbox")).toString().trimmed(); + // Both optional, and both describe this account's chip in the thread // list. An account tag is a different taxonomy from a functional one, // saying which mailbox a thread arrived in rather than what state it diff --git a/src/config.h b/src/config.h index f60e7cc..ede5dea 100644 --- a/src/config.h +++ b/src/config.h @@ -67,6 +67,20 @@ struct Account /// reports a missing key through the warnings path. QString trash; + /// The account's inbox folder, relative to maildir. Optional. + /// + /// Only Restore reads it, as the destination for a message that carries no + /// `deleted-from:` origin, which is what mail trashed by another client + /// looks like. Defaults to "Inbox", the Maildir convention and mbsync's + /// own default. + /// + /// Configurable rather than hardcoded because the name is not ours to + /// assume: naming a folder that does not exist CREATES it, beside the real + /// one, and under mbsync's `Create Both` that folder reaches the server. + /// Unlike `trash` this is optional, since the default is right for every + /// ordinary Maildir and a wrong guess here only affects the fallback. + QString inbox; + /// Chip colour in the thread list. Invalid when unset, in which case one /// is generated from the account tag's name. QColor color; @@ -114,6 +128,13 @@ struct Account /// sentQuery(). The query helper still returns empty so callers compose /// uniformly; it is Config::load() that reports the problem. QString trashQuery() const; + + /// Matches this account's inbox folder, using inboxFolder(). + QString inboxQuery() const; + + /// The inbox folder name, which is `inbox` when set and "Inbox" + /// otherwise. Never empty, so a caller always has a folder to name. + QString inboxFolder() const; }; /// A named query, stored in queries.json. diff --git a/src/keymap.cpp b/src/keymap.cpp index c731bbb..319bc53 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -31,6 +31,7 @@ QStringList KeyMap::knownActions() QStringLiteral("open_thread"), QStringLiteral("archive"), QStringLiteral("delete"), + QStringLiteral("restore"), QStringLiteral("spam"), QStringLiteral("toggle_unread"), QStringLiteral("mark_all_read"), @@ -98,6 +99,9 @@ QList> KeyMap::defaultBindings() { QStringLiteral("Return"), QStringLiteral("open_thread") }, { QStringLiteral("Ctrl+E"), QStringLiteral("archive") }, { QStringLiteral("Ctrl+D"), QStringLiteral("delete") }, + // Restore is only enabled in the trash view, so its key is dead + // elsewhere rather than doing something surprising. + { QStringLiteral("Ctrl+R"), QStringLiteral("restore") }, { QStringLiteral("Ctrl+Shift+S"), QStringLiteral("spam") }, { QStringLiteral("Ctrl+U"), QStringLiteral("toggle_unread") }, // Shifted against Ctrl+U, which toggles unread on the selection: this diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 361c0d4..da7128f 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -849,6 +849,10 @@ void MainWindow::registerActions() else trashSelected(); }); + addAction(QStringLiteral("restore"), tr("&Restore from trash"), + tr("Move the selected messages out of the trash"), [this]() { + restoreSelected(true); + }); addAction(QStringLiteral("spam"), tr("Mark &spam"), tr("Add spam and remove inbox"), [this]() { tagSelected({ QStringLiteral("spam") }, { QStringLiteral("inbox") }, @@ -1136,6 +1140,11 @@ void MainWindow::buildMenus() auto *messageMenu = menuBar()->addMenu(tr("&Message")); messageMenu->addAction(m_actions.value(QStringLiteral("archive"))); messageMenu->addAction(m_actions.value(QStringLiteral("delete"))); + // Beside Delete, whose inverse it is. Greyed outside the trash view + // rather than hidden: an action that vanishes teaches nothing, while a + // disabled entry with its shortcut beside it says both that it exists and + // where it applies. + messageMenu->addAction(m_actions.value(QStringLiteral("restore"))); messageMenu->addAction(m_actions.value(QStringLiteral("spam"))); messageMenu->addSeparator(); messageMenu->addAction(m_actions.value(QStringLiteral("toggle_unread"))); @@ -1193,6 +1202,9 @@ void MainWindow::buildMenus() // control: two buttons with different consequences looked identical. { QStringLiteral("archive"), QStringLiteral("mail-archive") }, { QStringLiteral("delete"), QStringLiteral("edit-delete") }, + // The inverse of delete, and the theme's own name for it: the icon + // every desktop uses for taking something back out of the wastebasket. + { QStringLiteral("restore"), QStringLiteral("edit-undelete") }, { QStringLiteral("undo"), QStringLiteral("edit-undo") }, { QStringLiteral("spam"), QStringLiteral("mail-mark-junk") }, { QStringLiteral("flag"), QStringLiteral("mail-mark-important") }, @@ -1259,6 +1271,7 @@ void MainWindow::buildMenus() m_threadContextMenu->setObjectName(QStringLiteral("threadContextMenu")); m_threadContextMenu->addAction(m_actions.value(QStringLiteral("archive"))); m_threadContextMenu->addAction(m_actions.value(QStringLiteral("delete"))); + m_threadContextMenu->addAction(m_actions.value(QStringLiteral("restore"))); m_threadContextMenu->addAction(m_actions.value(QStringLiteral("spam"))); m_threadContextMenu->addSeparator(); m_threadContextMenu->addAction(m_actions.value(QStringLiteral("toggle_unread"))); @@ -2459,8 +2472,40 @@ void MainWindow::onQueryFinished(int total, quint64 generation) applyPendingRecovery(); } +bool MainWindow::isShowingTrash() const +{ + // Compared against the trash GENERATOR's query, not against the word + // "trash" or against a tag. The trash view is path-based so that mail + // trashed by another client shows up in it; deciding this from + // `tag:deleted` instead would disable Restore on exactly the messages + // that most need it, which is the case Restore's fallback exists for. + // + // Both scopes, because the view composes with the account dropdown like + // every other filter: one account's trash, or all of them. + const QString query = m_lastQuery.trimmed(); + if (query.isEmpty()) + return false; + + const QString all = m_config.allTrashQuery().trimmed(); + if (!all.isEmpty() && query == all) + return true; + + for (const Account &account : m_config.accounts()) { + const QString trash = account.trashQuery().trimmed(); + if (!trash.isEmpty() && query == trash) + return true; + } + return false; +} + void MainWindow::updateViewWideActions() { + // Only meaningful on mail that is actually in a trash folder. An enabled + // action that does nothing is worse than an absent one, and Restore + // outside the trash has nothing to restore from. + if (QAction *action = m_actions.value(QStringLiteral("restore"))) + action->setEnabled(isShowingTrash()); + // Threads arrive in batches of kBatchSize, so before the query reports its // total the model holds only what has landed. An action that says "all" // must not run against a partial set and silently skip the rest, and a @@ -4458,7 +4503,35 @@ void MainWindow::restoreSelectedThreads() Q_ARG(QString, QStringLiteral("undelete_thread"))); } -void MainWindow::restoreSelected() +QString MainWindow::inboxFolderFor(const Account &account) const +{ + // Discovered from the account's OWN inbox query, never hardcoded. + // + // The casing is not ours to assume: the real Maildir has `Inbox` and a + // test fixture has `inbox`, and picking either would create a SECOND + // folder beside the real one on whichever side disagreed. That is exactly + // the failure a truncated origin folder caused on real mail this morning, + // and under mbsync's `Create Both` such a folder can reach the server. + // + // The inbox query is a generated `path:"//**"`, so the + // folder name is the part between the account prefix and the glob. + const QString query = account.inboxQuery(); + const QString prefix = + QStringLiteral("path:\"") + account.maildir + QLatin1Char('/'); + const QString suffix = QStringLiteral("/**\""); + if (query.startsWith(prefix) && query.endsWith(suffix)) { + const int from = prefix.length(); + const int length = query.length() - from - suffix.length(); + if (length > 0) + return query.mid(from, length); + } + + // No inbox configured for this account. `Inbox` is the Maildir + // convention and is what mbsync's own `Inbox` directive defaults to. + return QStringLiteral("Inbox"); +} + +void MainWindow::restoreSelected(bool fallbackToInbox) { const QModelIndexList rows = m_threadView->selectionModel()->selectedRows(); @@ -4496,14 +4569,58 @@ void MainWindow::restoreSelected() } 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"))); + // No origin recorded. Two quite different situations reach here and + // they want opposite things, which is what `fallbackToInbox` selects. + // + // From the TRASH VIEW the message is demonstrably in the trash, put + // there by another client, and refusing to move it leaves the user + // looking at a message they cannot get out. Inbox is the documented + // fallback, and it is reported, because a guess the user is not told + // about is worse than the guess itself. + // + // From a second press of Delete the message is NOT in the trash: it is + // sitting wherever it always was, wearing a stale `deleted` tag from + // an older version or from a hand-written notmuch command. Moving it + // to the inbox there would relocate mail the user never asked to move. + // The tag comes off and the file stays put. + if (fallbackToInbox) { + QHash byInbox; + QStringList stranded; + for (const QString &messageId : unknown) { + const Account account = + accountForMessagePath(m_model->messageById(messageId).filePath); + if (account.maildir.isEmpty()) { + stranded.append(messageId); + continue; + } + byInbox[account.maildir + QLatin1Char('/') + + inboxFolderFor(account)] + .append(messageId); + } + + 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(unknown.size() - stranded.size()))); + } + if (!stranded.isEmpty()) { + m_statusLabel->setText( + tr("%n message(s) could not be restored: they belong to no " + "configured account.", "", int(stranded.size()))); + } + } else { + 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) { diff --git a/src/mainwindow.h b/src/mainwindow.h index ae87868..937d87c 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -808,7 +808,28 @@ private: /// The inverse: moves each selected row's message back to the folder its /// `deleted-from:` tag names, stripping both tags. - void restoreSelected(); + /// + /// `fallbackToInbox` decides what happens to a message with NO origin tag, + /// and the two callers want opposite things. From the trash view the + /// message is demonstrably in the trash, trashed by another client, and + /// must still come out: it goes to the inbox, reported. From a second + /// press of Delete it is not in the trash at all and merely wears a stale + /// tag, so the tag comes off and the file stays where it is. + void restoreSelected(bool fallbackToInbox = false); + + /// The account's inbox FOLDER name, discovered from its inbox query. + /// + /// Never hardcoded: the real Maildir has `Inbox` and a fixture has + /// `inbox`, and assuming either would create a second folder beside the + /// real one on the side that disagreed. + QString inboxFolderFor(const Account &account) const; + + /// Whether the current query IS a trash view, for either scope. + /// + /// Compared against the trash generator's own query rather than against a + /// tag: the view is path-based so mail trashed by another client appears + /// in it, and such a message carries no tag of ours. + bool isShowingTrash() const; /// The `deleted-from:` tag naming `dbRelativeFolder`, or empty when no /// account owns it. diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index a17eba1..cefd686 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -135,6 +135,11 @@ public: << "maildir=" << accountMaildir << "\n"; if (!accountTrash.isEmpty()) out << "trash=" << accountTrash << "\n"; + // The fixture's folders are lowercase, unlike the Maildir + // convention Account::inboxFolder() defaults to. Stated rather + // than assumed, which is the whole point of the key: naming a + // folder that does not exist would CREATE it. + out << "inbox=inbox\n"; } } file.close(); @@ -372,6 +377,10 @@ private slots: void deletingAThreadRootTwiceRestoresItRatherThanRedeleting(); void deleteThreadMovesEveryMessageAndRepaintsTheRootCard(); void aFolderNameWithASpaceSurvivesTheRoundTrip(); + void restoreIsReachableWithoutTheKeyboard(); + void restoreIsOnlyEnabledInTheTrashView(); + void restoreReturnsAMessageToItsOriginFolder(); + void restoreFallsBackToInboxWithoutAnOriginTag(); private: /// Owns the throwaway lock table init() points every test at. A pointer @@ -9457,6 +9466,208 @@ void TestMainWindow::aFolderNameWithASpaceSurvivesTheRoundTrip() "messages are somewhere mbsync will never sync"); } +void TestMainWindow::restoreIsReachableWithoutTheKeyboard() +{ + // Restore shipped as a keyboard shortcut and nothing else: registered, + // iconned, enabled correctly, and present in no menu at all. A user who + // does not read the changelog would never learn it exists, and Ctrl+R is + // not a guess anyone makes. + // + // The four places an action must touch are enforced by tests + // (knownActions, defaultBindings, the icon table); being REACHABLE is a + // fifth that nothing checked, which is why the gap survived a green suite. + const Config config; + MainWindow window(config); + + auto *restore = window.findChild(QStringLiteral("restore")); + QVERIFY(restore); + + const auto menuContains = [](const QMenu *menu, const QAction *action) { + return menu && menu->actions().contains(action); + }; + + // A menu on the MENU BAR, beside Delete whose inverse it is. The context + // menu is excluded here so this assertion cannot be satisfied by the one + // the next assertion checks: findChildren finds both. + auto *context = + window.findChild(QStringLiteral("threadContextMenu")); + QVERIFY(context); + + bool inAMenuBarMenu = false; + for (const QMenu *menu : window.findChildren()) { + if (menu != context && menuContains(menu, restore)) { + inAMenuBarMenu = true; + break; + } + } + QVERIFY2(inAMenuBarMenu, + "Restore is in no menu-bar menu, so a user browsing the menus " + "would never learn it exists"); + + // And the thread list's context menu, which is where the other + // message-scoped actions are reached by mouse. + QVERIFY2(menuContains(context, restore), + "Restore is missing from the thread context menu"); +} + +void TestMainWindow::restoreIsOnlyEnabledInTheTrashView() +{ + // Restore has no meaning outside the trash, and an enabled action that + // does nothing is worse than an absent one. + // + // Enabled from the QUERY rather than from the selection's tags: a message + // trashed by another client carries no tag of ours and must still be + // restorable, which is the whole reason the trash view is path-based. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("re1@example.org"), + QStringLiteral("In the inbox"), QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body text."))); + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/Trash"), QStringLiteral("re2@example.org"), + QStringLiteral("In the trash"), QStringLiteral("other@example.org"), + QStringLiteral("Fri, 14 Aug 2026 11: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 *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + auto *restore = window.findChild(QStringLiteral("restore")); + QVERIFY(model && queryEdit); + QVERIFY2(restore, "there is no restore action"); + + // An ordinary view. Both fixture messages carry `inbox`, since the + // fixture tags all new mail that way regardless of folder, so this is two + // rows rather than one; the count is not what is under test. + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 2, 15000); + QVERIFY2(!restore->isEnabled(), + "Restore is enabled in an ordinary view, where it means nothing"); + + // The trash view, which is the account's own generated trash query. + queryEdit->setText(QStringLiteral("path:\"acct/Trash/**\"")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + QVERIFY2(restore->isEnabled(), + "Restore is disabled in the trash view, where it is the point"); +} + +void TestMainWindow::restoreReturnsAMessageToItsOriginFolder() +{ + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("ro1@example.org"), + QStringLiteral("Send 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); + + const QString root = backed.fixture().maildirPath(); + const QString cfg = backed.fixture().configPath(); + const QString stem = QStringLiteral("ro1.example.org"); + + 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(); + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(root + QStringLiteral("/acct/Trash/cur"), stem), + 15000); + + // Now from the trash view, through Restore rather than through a second + // Delete: this is the action the user reaches for when browsing trash. + queryEdit->setText(QStringLiteral("path:\"acct/Trash/**\"")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("restore"))->trigger(); + + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"), stem) + || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), + stem), + 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)); +} + +void TestMainWindow::restoreFallsBackToInboxWithoutAnOriginTag() +{ + // A message trashed by ANOTHER client: it sits in the trash folder and + // carries no `deleted-from:` tag, because nothing here put it there. The + // real Maildir has such messages, which is why the trash view is path + // based rather than tag based. + // + // Inbox is the documented fallback. Refusing to move it would leave the + // user with a message they can see in the trash and cannot get out. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/Trash"), QStringLiteral("foreign@example.org"), + QStringLiteral("Trashed elsewhere"), 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); + + const QString root = backed.fixture().maildirPath(); + const QString cfg = backed.fixture().configPath(); + const QString stem = QStringLiteral("foreign.example.org"); + + // The guard this test needs: no origin tag, so the fallback is what is + // under test rather than an ordinary restore. + QCOMPARE(notmuchCount(cfg, QStringLiteral("id:foreign@example.org and " + "tag:\"deleted-from:inbox\"")), + 0); + + queryEdit->setText(QStringLiteral("path:\"acct/Trash/**\"")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("restore"))->trigger(); + + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"), stem) + || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), + stem), + 15000); + QVERIFY2(!folderHasMessageFile(root + QStringLiteral("/acct/Trash/cur"), + stem), + "the message was copied out of the trash rather than moved"); +} + void TestMainWindow::undoMovesTheMessageBack() { // Undo is this project's answer to the confirmation dialog it rules out, diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index cc854b2..1c5eedd 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -259,6 +259,24 @@ %n messaggi non sono stati eliminati: nessuna cartella cestino è configurata per il loro account. + + Restore + Ripristina + + + %n message(s) had no record of where they came from and were moved to the inbox. + + %n messaggio non aveva traccia della sua provenienza ed è stato spostato in arrivo. + %n messaggi non avevano traccia della loro provenienza e sono stati spostati in arrivo. + + + + %n message(s) could not be restored: they belong to no configured account. + + %n messaggio non è stato ripristinato: non appartiene ad alcun account configurato. + %n messaggi non sono stati ripristinati: non appartengono ad alcun account configurato. + + Undelete Ripristina @@ -369,6 +387,14 @@ Mark thread as &spam Segna conversazione come &spam + + &Restore from trash + &Ripristina dal cestino + + + Move the selected messages out of the trash + Sposta i messaggi selezionati fuori dal cestino + Add spam and remove inbox on whole threads Aggiunge spam e rimuove inbox su intere conversazioni -- cgit v1.2.3 From 830aa264f81af1a92dec9ee95bc24a9b25dee53d Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Wed, 19 Aug 2026 10:03:04 +0200 Subject: i18n: translate the trash strings, and document the trash key The three new strings from the cleanup action, translated into Italian. lrelease reports 383 finished and 0 unfinished; an unfinished string is silently dropped and ships as English inside an otherwise Italian UI. The changelog gains an Upgrading section for the mandatory `trash` key, the new optional `inbox` key and the `Del` binding, and states the consequence that cost real mail on this branch: a folder name that does not match the server is created rather than reported, mbsync adopts it, and under Create Both it propagates to the server where other clients see it. CLAUDE.md is corrected on two counts. Adding an action is five places, not four; the fifth is a menu, and nothing enforced it until this branch added everyActionIsReachableFromAMenu(). And the trash design is recorded: why the origin lives in a tag, why those tags are joined by a tab rather than a space, and why Restore resolves against the database rather than the model. Also repairs a race in deletingTwiceLeavesNoOriginTagBehind(). Its guard ran a query through the bar in the gap between the file rename and the tag writes, and a query bar run in that gap returns zero rows forever, since QTRY_VERIFY re-reads rowCount() and never re-runs the query. Measured 3 failures in 12 runs, each burning a full 15s timeout; 0 in 8 after asking the database directly, with the runtime down from 45s to 0.3s. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 49 ++++++++++++++++++++++++++++++++++++++ CLAUDE.md | 52 ++++++++++++++++++++++++++++++++++++----- tests/test_mainwindow.cpp | 17 ++++++++++---- translations/qtmaildir_it_IT.ts | 12 ++++++++++ 4 files changed, 120 insertions(+), 10 deletions(-) (limited to 'translations') diff --git a/CHANGELOG.md b/CHANGELOG.md index 75d2de1..e514fc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,55 @@ point at which they are stable. ## [Unreleased] +### Added + +- Delete now moves mail into the account's trash folder instead of only + tagging it. A **Trash** filter sits beside Unread, Inbox, Important and + Sent, and composes with the account selector like the others. +- **Restore from trash** (`Ctrl+R`), enabled while the trash view is showing. + A message this application deleted returns to the folder it came from; one + trashed by another client returns to the inbox. +- **Find stranded deleted mail** (`Ctrl+Alt+T`), in the Message menu. It lists + mail tagged `deleted` that never moved anywhere. Run it whenever you like; + it reports and moves nothing on its own. +- An optional per-account `inbox` key, naming the inbox folder a restore falls + back to when a message carries no record of where it came from. It defaults + to `Inbox`, so an account whose inbox is named that needs nothing. +- `Del` now deletes, alongside `Ctrl+D`. It still edits text in the query bar + and in any other text field, so nothing is lost where the key already had a + job. + +### Changed + +- Open thread, Clear message pane and Clear selection appear in the View menu. + All three existed and were reachable only by their shortcuts. + +### Upgrading + +**Every account now needs a `trash` key** in `qtmaildir.conf`, naming its +trash folder relative to `maildir`: + + [account.work] + maildir = work + trash = Trash + +The folder must be one your `mbsync` configuration actually syncs, or the move +will never reach the server. Accounts without the key still load and still +read mail, but Delete cannot work on them and a warning says so at startup. + +**Name the folder exactly as it exists on the server.** A trash or inbox name +that does not match creates that folder rather than reporting an error, and +under mbsync's `Create Both` the wrongly named folder then propagates to the +mail server, where other clients will see it. + +**Mail deleted by earlier versions is not migrated.** It carries the `deleted` +tag and sits wherever it always was. Use **Find stranded deleted mail** to +review it, and Delete on what should really go. + +Note that Delete's reversibility depends on your provider: a trash folder the +provider purges on a timer will eventually remove the mail for good. + + ## [0.25.0] - 2026-08-17 Acting on a row now means the message that row displays, not the whole diff --git a/CLAUDE.md b/CLAUDE.md index 50faf4b..2065a10 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -227,6 +227,34 @@ combined `thread:a or thread:b` query rather than one query per thread. The only escape hatch is `general/notmuch_config`, pointing at an alternate notmuch config. Per-account subdirectories *are* configured, since notmuch does not model accounts at all. +**Delete MOVES the file, and a wrong folder name reaches the mail server.** +Item 103. Every account carries a mandatory `trash` key and an optional +`inbox` one, both naming a folder relative to `maildir`. Naming a folder that +does not exist does not fail: the move CREATES it, mbsync adopts it and writes +state files for it, and under `Create Both` it then propagates to the server, +where every other client sees it. This is not theoretical. A folder name +containing a space was truncated by the origin tag, a bogus folder was created +beside the real one, and four messages of a thread were stranded in it on the +user's real mail. Treat any code that composes a folder name as reaching the +server, because it does. + +**A message records where it came from in a tag, because nothing else can.** +`deleted-from:` is written when Delete moves the file, and read back by +Restore. The file has moved, so neither the path nor anything in notmuch still +knows the original folder. A notmuch tag MAY contain a space, so tags crossing +the thread boundary are joined by a TAB rather than a space; joining on a space +truncated every folder name containing one. A message trashed by another client +carries no such tag at all, which is why the trash view is path-based and why +Restore falls back to the account's inbox rather than refusing. + +**Restore reads the DATABASE, never 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 in the trash, one run in +three. The origin tag is then not found, the message falls into the no-origin +branch, and it goes to the inbox instead of where it came from, silently and +irreversibly. A restore must be right about its destination or it is worse than +doing nothing. + **The sync script lives here, in `assets/mailsync.sh`.** It moved from the companion `mailctl` project, which documents that it never calls it: the script is `mbsync` plus `notmuch new` with a lock, and qtmaildir is the only thing that @@ -577,15 +605,27 @@ a union over the conversation, so it can arm for a thread whose displayed message is already read. The write is still scoped to that message, so the cost is a no-op rather than a wrong write. -**Adding an action is four places, and three of them are enforced by tests that +**Adding an action is FIVE places, and four of them are enforced by tests that fail in confusing ways.** `KeyMap::knownActions()` (a `Q_ASSERT` in the constructor fires otherwise, and it surfaces in whichever suite happens to build a `MainWindow` first — `test_tagrules` did), `defaultBindings()` (every action -must be keyboard-reachable), and the icon table (every action must carry one). -The no-duplicate-icons rule is narrowed to actions that can reach the toolbar, -by a named exception list; the five thread actions share their twins' icons -because a submenu entry always carries text, and the test asserts none of them -is on the toolbar so the exemption cannot be abused. +must be keyboard-reachable), the icon table (every action must carry one), and +a MENU. The no-duplicate-icons rule is narrowed to actions that can reach the +toolbar, by a named exception list; the five thread actions share their twins' +icons because a submenu entry always carries text, and the test asserts none of +them is on the toolbar so the exemption cannot be abused. + +**The menu was the fifth place, and this document said four until item 103.** +Nothing enforced it, so `restore` shipped on the trash branch reachable by +`Ctrl+R` and by nothing a user could see or discover. The three existing +coverage tests each assert a different property and all three pass against an +action that appears nowhere in the interface. +`everyActionIsReachableFromAMenu()` closes it, walking every menu and submenu +from the menu bar; it found three more of the same the moment it was written +(`open_thread`, `clear_pane`, `clear_selection`). The toolbar is deliberately +NOT the test's instrument: it is a small chosen subset and always will be. An +action owning a submenu is not itself counted as reachable, since Qt emits no +`triggered` for it. **A toggle must read the state of what the row STANDS FOR, not of its thread.** `MainWindow::everySelectedRowHasTag()` is the one question `delete` and diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 8a41c85..578606b 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -9079,10 +9079,19 @@ void TestMainWindow::deletingTwiceLeavesNoOriginTagBehind() // The origin tag really was written, so the assertion after the second // delete is about it being REMOVED rather than never having existed. - queryEdit->setText(QStringLiteral( - "id:twice@example.org and tag:\"deleted-from:inbox\"")); - queryEdit->returnPressed(); - QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + // + // Asked of the DATABASE, not through the query bar. The file arriving in + // the trash is not the end of the delete: the tag writes land after the + // rename this test waits for, and a query bar run inside that gap returns + // zero rows FOREVER, because QTRY_VERIFY re-reads rowCount() and never + // re-runs the query. Measured 1 failure in 3 runs, each burning the full + // 15s timeout on a guard that was correct about a database it had asked + // too early. + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(backed.fixture().configPath(), + QStringLiteral("id:twice@example.org and " + "tag:\"deleted-from:inbox\"")) == 1, + 15000); // Second press on the same message, which restores it. queryEdit->setText(QStringLiteral("id:twice@example.org")); diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index 1c5eedd..b9515b4 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -277,6 +277,10 @@ %n messaggi non sono stati ripristinati: non appartengono ad alcun account configurato. + + Mail tagged deleted but not in a trash folder. Select what should go and press Delete. + Posta etichettata come eliminata ma non in un cestino. Seleziona cosa deve essere rimosso e premi Elimina. + Undelete Ripristina @@ -395,6 +399,14 @@ Move the selected messages out of the trash Sposta i messaggi selezionati fuori dal cestino + + Find &stranded deleted mail + &Cerca posta eliminata non spostata + + + Show mail tagged deleted that is not in a trash folder + Mostra la posta etichettata come eliminata che non si trova in un cestino + Add spam and remove inbox on whole threads Aggiunge spam e rimuove inbox su intere conversazioni -- cgit v1.2.3