From f051dd657757bca3ac7e36454d7f2fc21f322f73 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 29 Aug 2026 09:49:29 +0200 Subject: fix: judge a conversation's trash state on all of its messages Item 178. everySelectedRowIsInATrashFolder() read ThreadSummary::firstMessagePath for any row that was not a message row. That was correct while a thread row MEANT that message (item 108) and stopped being correct when item 177 made it mean the conversation. A conversation is in the trash only when ALL of its messages are, so a partly trashed thread answered on whichever message the query returned first: Delete could be hidden on a conversation that still had mail outside the trash, and Restore offered on one that mostly did not. Not data-affecting. Both actions are no-ops in the wrong direction: Delete on already-trashed mail takes moveMessages()' already-there branch, and Restore on mail that was never trashed finds nothing to move. qtmaildir cannot produce such a thread itself, since Delete is absent on a reply row and Restore is thread-scoped. Two things outside it can: another client trashing a single message, and a reply arriving after the conversation was trashed. ThreadDigest already walks every message of the selected conversation for its sender counts, and a filename is served from the index like everything else in it, so the paths ride along on a request the selection already makes rather than costing a walk on every query. ThreadDigest::messagePaths is relative to the mail root, for the reason firstMessagePath records: an absolute path matches no account and silently resolves every row to none. MainWindow keeps them beside the dashboard's thread id and clears them when the dashboard is left, so a late digest cannot answer about another row. One limit, stated in the code rather than hidden. The digest is requested only for a single selected conversation row, so that is the only case with a real answer; any other selection falls back to the summary's one path. That fallback IS the pre-177 answer and is wrong in exactly the same partial case, which is the point: a multi-row selection is left no worse than it was, rather than given a second, differently wrong rule of its own. Making it exhaustive costs a per-query walk over every message, which is what this avoids. Two tests, both mutation-checked. The worker test puts its two messages in different folders, since two in one folder answer identically whichever way the code resolves them. The window test asserts both directions, so a fix that simply hid Delete everywhere would fail it, and sets totalCount explicitly: a summary left at the default is a message row, and the test would otherwise exercise the other branch and pass for the wrong reason. Suite: 42 of 43, with undoMovesTheMessageBack failing as it does on master (item 136). --- src/mainwindow.cpp | 75 +++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 20 deletions(-) (limited to 'src/mainwindow.cpp') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 321c608..eb8f4c6 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -3667,29 +3667,53 @@ bool MainWindow::everySelectedRowIsInATrashFolder() const return false; for (const QModelIndex &index : rows) { - // The row's own file: a reply row's message, a thread row's displayed - // message. Same rule as everySelectedRowHasTag(), and for the same - // reason: a thread row acts on the message its card shows. - const QString path = - m_model->isMessageRow(index) - ? m_model->messageAt(index).filePath - : m_model->threadFor(index).firstMessagePath; - if (path.isEmpty()) - return false; + // A MESSAGE row is its one file. A CONVERSATION is in the trash only + // when ALL of its messages are (item 178), so it answers on every path + // the digest reported rather than on the one the summary carries. + // + // That was the pre-177 rule and it is now wrong in a way that is + // invisible: a partly trashed thread answered on whichever message the + // query returned first, hiding Delete on a conversation with mail + // outside the trash and offering Restore on one that mostly is not. + QStringList paths; + if (m_model->isMessageRow(index)) { + paths = { m_model->messageAt(index).filePath }; + } else { + const ThreadSummary thread = m_model->threadFor(index); + // Known only for the single conversation row the dashboard is + // showing, since that is the one the digest was requested for. + // Anything else falls back to the summary's one path, which is the + // pre-177 answer: wrong in the same partial case, so a multi-row + // selection is left no worse than it was rather than being given a + // second, differently wrong rule of its own. + paths = (!m_conversationPaths.isEmpty() + && m_conversationPathsThreadId == thread.threadId) + ? m_conversationPaths + : QStringList{ thread.firstMessagePath }; + } - const Account account = accountForMessagePath(path); - if (account.maildir.isEmpty() || account.trash.isEmpty()) + if (paths.isEmpty()) return false; - // Compared as a path segment, never with startsWith(): `trash-old` - // starts with `trash` and is a different folder. The same trap the - // attachment-save check records. - const QString prefix = account.maildir + QLatin1Char('/') - + account.trash + QLatin1Char('/'); - // accountForMessagePath() accepts both shapes, so this must too: a - // thread row's path is database-relative and a reply row's absolute. - if (!path.contains(prefix)) - return false; + for (const QString &path : std::as_const(paths)) { + if (path.isEmpty()) + return false; + + const Account account = accountForMessagePath(path); + if (account.maildir.isEmpty() || account.trash.isEmpty()) + return false; + + // Compared as a path segment, never with startsWith(): `trash-old` + // starts with `trash` and is a different folder. The same trap the + // attachment-save check records. + const QString prefix = account.maildir + QLatin1Char('/') + + account.trash + QLatin1Char('/'); + // accountForMessagePath() accepts both shapes, so this must too: a + // thread row's path is database-relative and a reply row's + // absolute. + if (!path.contains(prefix)) + return false; + } } return true; } @@ -4149,6 +4173,10 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, } m_dashboardThreadId.clear(); + // Stale the moment the dashboard is not showing this thread: leaving them + // would answer a later conversation's question with an earlier one's paths. + m_conversationPathsThreadId.clear(); + m_conversationPaths.clear(); // The message the card displays, not the thread. The summary's `unread` is // a union over the conversation, so this can arm for a thread whose first @@ -4210,6 +4238,13 @@ void MainWindow::onThreadDigestLoaded(const ThreadDigest &digest, if (digest.threadId != m_dashboardThreadId) return; + // Item 178: what Delete and Restore need to judge the CONVERSATION rather + // than its first message. Kept beside the thread id so a late digest for + // another row cannot answer about this one. + m_conversationPathsThreadId = digest.threadId; + m_conversationPaths = digest.messagePaths; + refreshTrashActions(); + m_messageView->showDashboard(digest); } -- cgit v1.2.3