diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-19 10:27:02 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-19 10:27:02 +0200 |
| commit | 2e0db925d5ca7100d8405ffc352ae435cdbdb73d (patch) | |
| tree | 4d10cc671d1ed994b23263e80bdbc1c77c8df330 | |
| parent | 6de81918dc7a9342fb532d6a5542c868ff65826c (diff) | |
| download | qtmaildir-delete-to-trash.tar.gz qtmaildir-delete-to-trash.zip | |
fix(trash): refresh the list when a restore empties a rowdelete-to-trash
Reported from a hand test: Restore moved the message correctly and the row it
came from sat in the trash list until the Trash filter was clicked again.
The trash view is path-based, so a restored message stops matching the query
the list was built from. That is a state no tag change can express, and nothing
in onMessagesMoved() removes a row, deliberately: in an ordinary view a deleted
message's card should stay put, since one deleted message does not doom the
conversation.
refreshCurrentQuery(), not runCurrentQuery(). The refresh runs immediately
after the undo entry is pushed, and re-running the query outright clears the
undo stack, which would make Restore the one mutation in the window with no way
back. Gated on isShowingTrash() rather than on the destination, because a
Delete is a move too and reaches the same slot.
Three tests, each catching a different mutation: the row leaves, undo survives
the refresh and still moves the file back, and a delete outside the trash view
leaves its row alone.
The third one was wrong on its first draft and passed against the mutation it
existed to catch. It used a `tag:inbox` view, which looks ordinary but which a
deleted message keeps matching, since Delete adds `deleted` and the origin tag
and removes nothing. A path query on the inbox folder is the honest instrument:
the file really leaves, so the row survives only because nothing refreshed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| -rw-r--r-- | CHANGELOG.md | 4 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 23 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 175 |
3 files changed, 202 insertions, 0 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index e514fc8..dbc06e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,10 @@ point at which they are stable. - Open thread, Clear message pane and Clear selection appear in the View menu. All three existed and were reachable only by their shortcuts. +- Restoring from the trash view refreshes the list, so the restored message + leaves it straight away instead of sitting there until the Trash filter is + clicked again. Other views are unaffected: a deleted message's card + deliberately stays where it is. ### Upgrading diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index a1c01c3..58c82ca 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -4982,6 +4982,29 @@ void MainWindow::onMessagesMoved(const QMap<QString, QString> &originByMessageId } } + // A restore out of the TRASH VIEW leaves the row it came from showing a + // message that is no longer there, and only a refresh can say so. + // + // Reported from a hand test: the move was correct and the row sat in the + // list until the Trash filter was clicked again. The trash view is PATH + // based, so a restored message stops matching the query the list was built + // from, which is a state no tag change can express. Nothing else here + // removes a row, deliberately: in an ordinary view a deleted message's + // card should stay put, since one deleted reply does not doom the + // conversation. + // + // refreshCurrentQuery() rather than runCurrentQuery(): it clears nothing, + // so the selection, the expanded threads, the undo stack and the message + // being read all survive. Re-running the query outright would destroy the + // undo entry this function just pushed, which is the one thing a restore + // must leave intact. + // + // Gated on isShowingTrash() and not on the destination: a Delete is a move + // too and reaches this same slot, and refreshing after every delete would + // make a row vanish from under the user in every other view. + if (isShowingTrash()) + refreshCurrentQuery(); + // 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 diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index e982e37..79e137a 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -388,6 +388,9 @@ private slots: void theCleanupQueryFindsStrandedMail(); void theCleanupQueryExcludesMailAlreadyInTrash(); void aMoveThatRelocatesNothingWritesNoTag(); + void restoringFromTheTrashViewRefreshesTheList(); + void theRefreshAfterARestoreLeavesUndoIntact(); + void deletingOutsideTheTrashViewLeavesTheRowInPlace(); private: /// Owns the throwaway lock table init() points every test at. A pointer @@ -10389,4 +10392,176 @@ void TestMainWindow::aMoveThatRelocatesNothingWritesNoTag() QStringLiteral("nomove.example.org"))); } +void TestMainWindow::restoringFromTheTrashViewRefreshesTheList() +{ + // Reported from a hand test: Restore moved the message correctly and the + // row it came from sat in the trash list until the Trash filter was + // clicked again. + // + // The trash view is PATH based, so a restored message no longer matches + // the query the list was built from. That is a state no tag change can + // express: onMessagesMoved() updates tags and the undo stack and never + // removes a row, which is right in an ordinary view (a deleted message's + // card should stay put) and wrong here, where the row is the one thing + // that is now false. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/Trash"), QStringLiteral("refr1@example.org"), + QStringLiteral("Restore me"), 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<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 stem = QStringLiteral("refr1.example.org"); + + // The account's own generated trash query, which is what the Trash filter + // puts in the bar. + 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<QAction *>(QStringLiteral("restore"))->trigger(); + + // The move really happened, waited on the FILE. Without this the row + // assertion below could pass against a restore that never ran. + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"), stem) + || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), + stem), + 15000); + + // And the list no longer shows it, without the user touching anything. + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 0, 15000); +} + +void TestMainWindow::theRefreshAfterARestoreLeavesUndoIntact() +{ + // The refresh that fixes the stale trash row runs immediately after the + // undo entry is pushed, so it must not be the thing that destroys it. + // runCurrentQuery() clears the undo stack outright, which would make + // Restore the one mutation in the window with no way back; this asserts + // the non-destructive refresh was used and stayed non-destructive. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/Trash"), QStringLiteral("undoref@example.org"), + QStringLiteral("Restore then undo"), + 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<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 stem = QStringLiteral("undoref.example.org"); + + 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<QAction *>(QStringLiteral("restore"))->trigger(); + + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"), stem) + || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), + stem), + 15000); + // The refresh has run by now, which is what the row count proves. + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 0, 15000); + + // And the undo entry is still there afterwards. + auto *undo = window.findChild<QAction *>(QStringLiteral("undo")); + QVERIFY(undo); + QVERIFY2(undo->isEnabled(), + "the refresh after a restore cleared the undo stack"); + + undo->trigger(); + + // Back in the trash, asserted on the FILE: the undo has to move it, not + // merely re-tag it. + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(root + QStringLiteral("/acct/Trash/cur"), stem), + 15000); +} + +void TestMainWindow::deletingOutsideTheTrashViewLeavesTheRowInPlace() +{ + // The other half of the trash-view refresh, and the reason it is gated. + // + // A Delete is a move too and reaches the same confirmation slot. Refreshing + // on every move would make the row vanish from under the user in every + // ordinary view, which this project has decided against twice: the card + // deliberately stays put, because one deleted message does not doom the + // conversation and a row disappearing mid-gesture loses the user's place. + // + // Nothing asserted this, so a mutation dropping the isShowingTrash() gate + // passed the whole suite. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("stay1@example.org"), + QStringLiteral("Stay on screen"), 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<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 stem = QStringLiteral("stay1.example.org"); + + // An ORDINARY view that the message STOPS MATCHING once the delete lands. + // Both halves matter and the first draft of this test had only one: a + // `tag:inbox` view looks ordinary but Delete adds `deleted` and the origin + // tag and removes nothing, so the message keeps `inbox` and keeps matching. + // A refresh there is a no-op, and the mutation dropping the + // isShowingTrash() gate passed against it. + // + // A path query on the inbox folder is the honest instrument: the file + // really leaves that folder, so the row survives only because nothing + // refreshed. + queryEdit->setText(QStringLiteral("path:\"acct/inbox/**\"")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild<QAction *>(QStringLiteral("delete"))->trigger(); + + // The delete really happened, waited on the file rather than on the list. + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(root + QStringLiteral("/acct/Trash/cur"), stem), + 15000); + + // A refresh is a queued round trip, so an immediate read would pass against + // one still in flight. Given time to arrive, then asserted not to have + // taken the row away. + QTest::qWait(1500); + QCOMPARE(model->rowCount(QModelIndex()), 1); +} + #include "test_mainwindow.moc" |
