diff options
| author | Danilo M. <danix@danix.xyz> | 2026-09-14 14:02:21 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-09-14 14:02:21 +0200 |
| commit | 40328fb336ebcec716a39d1f78cdee95b89bc75b (patch) | |
| tree | 5564c0b731a8f0af4cf56791ffac04062c6f03dd | |
| parent | 9fdac493c9a6e35f36dc45ff0e13583ddf88605e (diff) | |
| download | qtmaildir-40328fb336ebcec716a39d1f78cdee95b89bc75b.tar.gz qtmaildir-40328fb336ebcec716a39d1f78cdee95b89bc75b.zip | |
fix: the Not spam review findings
Close the two Important test gaps and fold in the minor notes.
notSpamIsAbsentOnAReplyRow passed for the wrong reason: the reply node had
no filePath and the selected child was the first message, so the predicate
answered false on the empty path and hid the action with or without the reply
guard. Give the reply a real spam path and select the actual reply child;
mutation-checked that removing the guard now fails the test.
Add notSpamThreadMovesEveryMessageHome, the thread-scoped coverage
notSpamThreads()/m_pendingThreadScope/wholeThreadIds had none of, and assert
the folded thread-scoped trash Restore re-adds the inbox tag.
Rename the label Not junk -> Not spam (no free mnemonic in the Message menu)
to match the rest of the UI, with the Italian translation updated, and add a
changelog line for the thread-scoped Restore inbox-tag fix.
| -rw-r--r-- | CHANGELOG.md | 8 | ||||
| -rw-r--r-- | README.md | 4 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 16 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 124 | ||||
| -rw-r--r-- | translations/qtmaildir_it_IT.ts | 12 |
5 files changed, 145 insertions, 19 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index fd8d858..0765935 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,7 +47,7 @@ point at which they are stable. - **A spam button on the message pane's bar**, beside Star and Archive. It is drawn with a bug rather than the theme's junk glyph, and falls back to `mail-mark-junk` on a theme that ships no bug. -- **Not junk**, the way back out of the spam folder. Selecting a message in the +- **Not spam**, the way back out of the spam folder. Selecting a message in the Spam view offers it on both the Message menu and the message pane's bar, and it returns each message to the folder its `moved-from:` origin names, or to the account's inbox when a provider put it there and no origin was recorded, @@ -124,6 +124,12 @@ point at which they are stable. - **A skipped sync no longer looks like a successful one.** When a run exits because another sync already holds the lock, it carried nothing, and the pending count stays where it was. +- **A thread-scoped Restore from the trash re-adds the `inbox` tag when it + returns mail to an inbox.** The message-scoped restore already did this; the + thread-scoped one moved the file back to the inbox folder but left the tag + off, so the conversation sat in the folder invisible to the Inbox view until + the next hook run. Both routes now share one implementation and both restore + the tag. ### Upgrading @@ -596,7 +596,7 @@ From: account re-seeds it only until you pick one yourself. Archive, mark-important and toggle-unread write fixed tags. Delete and spam move the file instead: each adds a fixed tag (`deleted` or `spam`) and records the folder the file left in a `moved-from:<folder>` origin. Restore (in the -trash) and Not junk (in the spam folder) are their inverses: each moves the +trash) and Not spam (in the spam folder) are their inverses: each moves the message back to the folder its origin names, and mail caught by a provider with no origin at all goes to the account's inbox, which the status line reports. For anything else, **Ctrl+T** opens a dialog over the selected threads: type tags to @@ -783,7 +783,7 @@ Defaults, all rebindable through `[keys]`: Every action in this table carries a default binding, and every one appears in a menu. Seven actions carry none, because a chord for them would be arbitrary: -Check for stranded spam, Empty trash, Delete permanently, Empty spam, Not junk, +Check for stranded spam, Empty trash, Delete permanently, Empty spam, Not spam, Edit draft and Save message. They are reachable from the menus, and the shortcut reference prints them as unbound. **Help > Keyboard shortcuts** lists the current bindings, generated from the diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 383e257..68cdde7 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1813,11 +1813,13 @@ void MainWindow::registerActions() // back to the folder its `moved-from:` tag names, falling back to the // account's inbox for mail a provider caught, which carries no origin. // - // The mnemonic is on "&junk" rather than "spam": Alt+P is already Re&ply - // and Alt+S is a frozen collision, and no letter of "Not spam" is free in - // this menu. `junk` is the theme's own name for the folder and what its - // icon (`mail-mark-notjunk`) draws, so the wording stays honest. - addAction(QStringLiteral("not_spam"), tr("Not &junk"), + // No mnemonic, deliberately: every letter of "Not spam" is already used in + // this menu (N, o, t, s, p, a, m), Alt+P and Alt+S are taken by Re&ply and + // the frozen Mark &spam collision, and no free letter spells "spam". The + // mnemonic test skips an entry that carries none, and the terminology + // matches the rest of the UI ("spam folder", the `spam` key) rather than + // inventing "junk" for a menu that says spam everywhere else. + addAction(QStringLiteral("not_spam"), tr("Not spam"), tr("Move the selected messages out of the spam folder"), [this]() { notSpamSelected(); }); @@ -6773,7 +6775,7 @@ void MainWindow::onThreadMessagesResolved(const QStringList &messageIds, if (requestTag == QStringLiteral("not_spam_thread")) { restoreResolvedMessages(messageIds, paths, tags, - QStringLiteral("spam"), tr("Not junk"), + QStringLiteral("spam"), tr("Not spam"), true, false, threadScope); return; } @@ -6844,7 +6846,7 @@ void MainWindow::notSpamMessages(const QStringList &messageIds, const QStringList &tags) { restoreResolvedMessages(messageIds, paths, tags, QStringLiteral("spam"), - tr("Not junk"), true, false); + tr("Not spam"), true, false); } void MainWindow::notSpamThreads(const QStringList &threadIds) diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 66f11c0..0dfd834 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -547,6 +547,7 @@ private slots: void notSpamIsOfferedInTheSpamView(); void notSpamIsAbsentOnAReplyRow(); void notSpamIsHiddenOutsideTheSpamFolder(); + void notSpamThreadMovesEveryMessageHome(); void undoOfNotSpamReturnsTheFileToTheSpamFolder(); // ComposeWindow, item 123. These need a window but no worker: the composer @@ -12818,6 +12819,16 @@ void TestMainWindow::deleteThreadMovesEveryMessageAndRepaintsTheRootCard() QVERIFY(!folderHasMessageFile(trash, QStringLiteral("dt0.example.org"))); QVERIFY(!folderHasMessageFile(trash, QStringLiteral("dt1.example.org"))); QVERIFY(!folderHasMessageFile(trash, QStringLiteral("dt2.example.org"))); + + // And the `inbox` tag came back with each message, not only the file. The + // message-scoped restore always did this; the thread-scoped one did not, + // because it passed an empty add list. Item 201 folded the two routes into + // one implementation, and this is the property that was silently missing: + // the conversation sat in the inbox FOLDER with no `inbox` tag, so the + // Inbox view could not see it until the next hook run. + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, thread + QStringLiteral(" and tag:inbox")) == 3, + 15000); } void TestMainWindow::aFolderNameWithASpaceSurvivesTheRoundTrip() @@ -17468,15 +17479,24 @@ void TestMainWindow::notSpamIsAbsentOnAReplyRow() root.messageId = QStringLiteral("m1"); root.threadId = QStringLiteral("t1"); root.depth = 0; + root.filePath = QStringLiteral("acct/spam/cur/1:2,S"); MessageNode reply; reply.messageId = QStringLiteral("m2"); reply.threadId = QStringLiteral("t1"); reply.depth = 1; + // A REAL spam path, so the predicate answers true for this row and the + // ONLY thing that can hide Not spam is the reply guard. Without it the + // empty path made everySelectedRowIsInAFolder() return false and the test + // would pass against a missing guard. + reply.filePath = QStringLiteral("acct/spam/cur/2:2,S"); model->setThreadMessages(QStringLiteral("t1"), { root, reply }); const QModelIndex thread = model->index(0, 0, QModelIndex()); view->expand(thread); - const QModelIndex replyRow = model->index(0, 0, thread); + // Child 1, not child 0: since item 177 a conversation lists its FIRST + // message as a child too, so child 0 is the root message and child 1 is + // the reply whose row is under test. + const QModelIndex replyRow = model->index(1, 0, thread); view->selectionModel()->select( replyRow, QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); view->setCurrentIndex(replyRow); @@ -17609,4 +17629,106 @@ void TestMainWindow::undoOfNotSpamReturnsTheFileToTheSpamFolder() QCOMPARE(notmuchCount(cfg, QStringLiteral("id:nsundo@example.org")), 1); } +void TestMainWindow::notSpamThreadMovesEveryMessageHome() +{ + // The thread-scoped half of Not spam, mirroring + // deleteThreadMovesEveryMessageAndRepaintsTheRootCard: a conversation row + // moves its whole conversation, each message back to its OWN origin. The + // single-message test cannot see notSpamThreads(), the optimistic + // applyTagChange, m_pendingThreadScope or the wholeThreadIds repaint. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("nst0@example.org"), + QStringLiteral("NST 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("nst1@example.org"), + QStringLiteral("Re: NST root"), QStringLiteral("other@example.org"), + QStringLiteral("Fri, 14 Aug 2026 11:00:00 +0200"), + QStringLiteral("Reply one."), true, QStringLiteral("nst0@example.org"))); + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("nst2@example.org"), + QStringLiteral("Re: NST root"), QStringLiteral("third@example.org"), + QStringLiteral("Fri, 14 Aug 2026 12:00:00 +0200"), + QStringLiteral("Reply two."), true, QStringLiteral("nst0@example.org"))); + QVERIFY2(backed.build(QStringLiteral("acct"), QStringLiteral("acct"), + QStringLiteral("Trash"), QStringLiteral("Spam")), + qPrintable(backed.error())); + + MainWindow window(backed.config()); + auto *model = window.findChild<ThreadListModel *>(); + auto *view = window.findChild<ThreadListView *>(); + auto *queryEdit = + window.findChild<QLineEdit *>(QStringLiteral("queryEdit")); + QVERIFY(model && view && queryEdit); + + const QString root = backed.fixture().maildirPath(); + const QString cfg = backed.fixture().configPath(); + const QString spam = root + QStringLiteral("/acct/Spam/cur"); + const QString thread = QStringLiteral("thread:{id:nst0@example.org}"); + + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + QCOMPARE(notmuchCount(cfg, thread), 3); + + // A conversation row, so the move is thread-scoped. + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild<QAction *>(QStringLiteral("spam"))->trigger(); + + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(spam, QStringLiteral("nst0.example.org")) + && folderHasMessageFile(spam, QStringLiteral("nst1.example.org")) + && folderHasMessageFile(spam, QStringLiteral("nst2.example.org")), + 15000); + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, thread + QStringLiteral(" and tag:spam")) == 3, + 15000); + QCOMPARE(notmuchCount(cfg, thread + + QStringLiteral(" and " + "tag:\"moved-from:inbox\"")), + 3); + + // Now Not spam on the same conversation row, from the Spam view. + queryEdit->setText(QStringLiteral("path:\"acct/Spam/**\"")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild<QAction *>(QStringLiteral("not_spam"))->trigger(); + + // Every message back in the inbox folder, none left in spam, each carrying + // the `inbox` tag its origin move stripped. + QTRY_VERIFY_WITH_TIMEOUT( + (folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"), + QStringLiteral("nst0.example.org")) + || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), + QStringLiteral("nst0.example.org"))) + && (folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"), + QStringLiteral("nst1.example.org")) + || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), + QStringLiteral("nst1.example.org"))) + && (folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"), + QStringLiteral("nst2.example.org")) + || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), + QStringLiteral("nst2.example.org"))), + 15000); + QVERIFY(!folderHasMessageFile(spam, QStringLiteral("nst0.example.org"))); + QVERIFY(!folderHasMessageFile(spam, QStringLiteral("nst1.example.org"))); + QVERIFY(!folderHasMessageFile(spam, QStringLiteral("nst2.example.org"))); + + QCOMPARE(notmuchCount(cfg, thread), 3); + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, thread + QStringLiteral(" and tag:spam")) == 0, + 15000); + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, thread + + QStringLiteral(" and " + "tag:\"moved-from:inbox\"")) == 0, + 15000); + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, thread + QStringLiteral(" and tag:inbox")) == 3, + 15000); +} + #include "test_mainwindow.moc" diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index dfd2ff9..f4d7688 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -550,6 +550,10 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Aggiunge o rimuove l'etichetta deleted</translation> </message> <message> + <source>Not spam</source> + <translation>Non spam</translation> + </message> + <message> <source>Remove the unread tag from every message of the selected threads</source> <translation>Rimuove il tag unread da ogni messaggio delle conversazioni selezionate</translation> </message> @@ -770,10 +774,6 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Svuota spam</translation> </message> <message> - <source>Not junk</source> - <translation>Non spam</translation> - </message> - <message> <source>Undelete thread</source> <translation>Ripristina conversazione</translation> </message> @@ -928,10 +928,6 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Sposta nella cartella spam i messaggi selezionati</translation> </message> <message> - <source>Not &junk</source> - <translation>&Non spam</translation> - </message> - <message> <source>Move the selected messages out of the spam folder</source> <translation>Sposta fuori dalla cartella spam i messaggi selezionati</translation> </message> |
