From acec616bee1c7a336a9dcee8338849b970c3eab1 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 17 Aug 2026 19:46:08 +0200 Subject: feat(config): read a per-account trash folder --- src/config.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'src/config.cpp') diff --git a/src/config.cpp b/src/config.cpp index 600c558..d56099a 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -137,6 +137,11 @@ QString Account::draftsQuery() const return folderQuery(maildir, drafts); } +QString Account::trashQuery() const +{ + return folderQuery(maildir, trash); +} + QString Config::allSentQuery() const { return joinAccountQueries(m_accounts, &Account::sentQuery); @@ -433,6 +438,12 @@ void Config::load(const QString &path) account.sent = settings.value(QStringLiteral("sent")).toString().trimmed(); + // Mandatory, unlike sent: Delete moves a file into this folder, so an + // account without one cannot delete at all. Trimmed for the same + // reason as sent, above. + account.trash = + settings.value(QStringLiteral("trash")).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 -- cgit v1.2.3 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. --- src/config.cpp | 15 +++++++++++ tests/test_config.cpp | 56 +++++++++++++++++++++++++++++++++-------- translations/qtmaildir_it_IT.ts | 10 +++++--- 3 files changed, 67 insertions(+), 14 deletions(-) (limited to 'src/config.cpp') diff --git a/src/config.cpp b/src/config.cpp index d56099a..8294e4b 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -473,6 +473,21 @@ void Config::load(const QString &path) .arg(account.key)); continue; } + + // Mandatory, unlike sent: Delete moves a file into this folder, so an + // account without one cannot delete at all. Reported rather than + // silently disabled, so the user finds out from a warning rather than + // from a Delete that quietly does nothing. The account still loads; + // only Delete is unusable, which does not warrant losing the rest of + // the account's mail. + if (account.trash.isEmpty()) { + addProblem( + tr("Account '%1' has no trash folder configured; add a " + "'trash' key to its section. Delete will not work for " + "this account until it does.") + .arg(account.key)); + } + m_accounts.append(account); } diff --git a/tests/test_config.cpp b/tests/test_config.cpp index 98743f5..10dcf68 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -118,6 +118,7 @@ private slots: void allDraftsQueryIsIndependentOfSent(); void anAccountCarriesItsTrashFolder(); void aBracketedTrashFolderIsQuoted(); + void anAccountWithoutATrashFolderWarns(); }; static QString writeIni(const QTemporaryDir &dir, const QString &body) @@ -430,7 +431,8 @@ void TestConfig::absentSyncCommandIsNoticeNotProblem() QTemporaryDir dir; const QString path = writeIni(dir, QStringLiteral( "[account.work]\n" - "maildir=work-mail\n")); + "maildir=work-mail\n" + "trash=Trash\n")); Config config; config.load(path); @@ -450,7 +452,8 @@ void TestConfig::brokenSyncCommandIsAProblem() "command=/nonexistent/qtmaildir-test/mailsync.sh\n" "\n" "[account.work]\n" - "maildir=work-mail\n")); + "maildir=work-mail\n" + "trash=Trash\n")); Config config; config.load(path); @@ -485,7 +488,8 @@ void TestConfig::validConfigHasNoProblems() "\n" "[account.work]\n" "maildir=work-mail\n" - "address=user@example.org\n")); + "address=user@example.org\n" + "trash=Trash\n")); Config config; config.load(path); @@ -905,7 +909,8 @@ void TestConfig::sentQueryIsEmptyWithoutTheKey() Config config; config.load(writeIni(dir, QStringLiteral( "[account.provider-c]\n" - "maildir = provider-c\n"))); + "maildir = provider-c\n" + "trash = Trash\n"))); QCOMPARE(config.accounts().size(), 1); QVERIFY(config.accounts().at(0).sentQuery().isEmpty()); @@ -983,6 +988,27 @@ void TestConfig::aBracketedTrashFolderIsQuoted() QStringLiteral("path:\"provider-a/[Provider]/Cestino/**\"")); } +void TestConfig::anAccountWithoutATrashFolderWarns() +{ + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[account.work]\n" + "maildir=work\n"))); + + // The account still loads. A missing trash folder disables Delete, it does + // not invalidate the account: the user can still read mail. + QVERIFY(config.account(QStringLiteral("work")).isValid()); + + // Names the account and the key, so the warning is actionable. A warning + // the user cannot act on teaches them to ignore warnings, which item 83 + // recorded the hard way. + QVERIFY(!config.problems().isEmpty()); + const QString joined = config.warnings().join(QLatin1Char('\n')); + QVERIFY(joined.contains(QStringLiteral("work"))); + QVERIFY(joined.contains(QStringLiteral("trash"))); +} + void TestConfig::sentQueryComposesWithScopedQuery() { // A Sent view under one account must not show another account's sent mail. @@ -1103,9 +1129,11 @@ void TestConfig::theStartupAccountIsReadAndValidated() "\n" "[account.work]\n" "maildir=work\n" + "trash=Trash\n" "\n" "[account.personal]\n" - "maildir=personal\n"))); + "maildir=personal\n" + "trash=Trash\n"))); QCOMPARE(config.startupAccount(), QStringLiteral("work")); QVERIFY(config.problems().isEmpty()); @@ -1130,7 +1158,8 @@ void TestConfig::theStartupAccountIsReadAndValidated() "startup_account=nosuchaccount\n" "\n" "[account.work]\n" - "maildir=work\n"))); + "maildir=work\n" + "trash=Trash\n"))); QVERIFY2(wrong.startupAccount().isEmpty(), "an unknown startup account was passed through rather than " "falling back to All accounts"); @@ -1155,7 +1184,8 @@ void TestConfig::theStartupAccountTakesTheKeyNotTheSyncChannel() "\n" "[account.provider-work.mailbox]\n" "maildir=provider-work.mailbox\n" - "channel=provider-workmailbox\n"))); + "channel=provider-workmailbox\n" + "trash=Trash\n"))); QCOMPARE(config.accounts().size(), 1); QCOMPARE(config.accounts().constFirst().key, @@ -1177,7 +1207,8 @@ void TestConfig::theStartupAccountTakesTheKeyNotTheSyncChannel() "\n" "[account.provider-work.mailbox]\n" "maildir=provider-work.mailbox\n" - "channel=provider-workmailbox\n"))); + "channel=provider-workmailbox\n" + "trash=Trash\n"))); QVERIFY2(byChannel.startupAccount().isEmpty(), "the sync channel was accepted as an account key"); @@ -1261,7 +1292,8 @@ void TestConfig::theStartupQuerySurvivesATranslatedFilterName() "\n" "[account.work]\n" "maildir=work\n" - "sent=Sent\n"))); + "sent=Sent\n" + "trash=Trash\n"))); QVERIFY2(!config.savedQueries().isEmpty(), "queries.json did not load, so the warning path is unreachable"); @@ -1286,7 +1318,8 @@ void TestConfig::theStartupQuerySurvivesATranslatedFilterName() "\n" "[account.work]\n" "maildir=work\n" - "sent=Sent\n"))); + "sent=Sent\n" + "trash=Trash\n"))); QVERIFY(!byLabel.savedQueries().isEmpty()); QCOMPARE(byLabel.startupSavedQuery().generated, QStringLiteral("inbox")); QVERIFY(byLabel.problems().isEmpty()); @@ -1513,7 +1546,8 @@ void TestConfig::draftsQueryIsEmptyWithoutTheKey() Config config; config.load(writeIni(dir, QStringLiteral( "[account.provider-c]\n" - "maildir = provider-c\n"))); + "maildir = provider-c\n" + "trash = Trash\n"))); QCOMPARE(config.accounts().size(), 1); QVERIFY(config.accounts().at(0).draftsQuery().isEmpty()); 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 'src/config.cpp') 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 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 'src/config.cpp') 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