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.h | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) (limited to 'src/mainwindow.h') 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; +}; -- 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 'src/mainwindow.h') 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 'src/mainwindow.h') 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 b7d8ca35d74a9531ad292d9e875803964f6e0043 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 18 Aug 2026 13:01:51 +0200 Subject: feat(delete): bind Del, and resolve a restore against the database Del is the key a user reaches for and Ctrl+D is not a guess anyone makes. Both are bound; Del is listed FIRST because that is the one the menus advertise. Bare, which is safe here for a reason that does not generalise to other bare keys. A QAction shortcut is dispatched before the focused widget sees the key, and Qt withholds only plain LETTERS from editable widgets, so by the argument that made bare Return break the query bar this should delete mail while the user edits a query. It does not: QLineEdit accepts the ShortcutOverride for Delete itself, because Delete is one of its own editing keys, which Return is not. Measured with and without an explicit filter, the action fires 0 times either way, so no filter is added. theDeleteKeyEditsTextInTheQueryBar() pins that Qt behaviour, since the binding rests on it. **Two defects surfaced from the second binding, both real.** An action can now have more than one default, and KeyMap did not allow for it. sequenceFor() decided "is this a built-in?" by comparing against defaultSequenceFor(), which returns only the FIRST default, so the second looked like a user override and won the "a user binding beats the default" rule. The menus advertised Ctrl+D to a user who had configured nothing, and sequenceFor() and defaultSequenceFor() disagreed about an untouched action. isDefaultBinding() asks whether a sequence is ANY of the action's defaults; when two defaults tie, the one defaultBindings() lists first wins, which is the author's stated preference rather than an alphabetical accident. And Restore read each message's origin tag FROM 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 sitting in the trash, one run in three. No origin tag was found, the message took the no-origin branch, and Restore moved it to the INBOX instead of the folder it came from, silently, with the origin tag left behind as the only evidence. A restore has to be right about the destination or it is worse than doing nothing. The trash-view Restore now resolves its messages against the DATABASE first, through a new NotmuchWorker::resolveMessages(). That and resolveThreadMessages() share one walk, resolveQuery(), rather than growing a near-duplicate: they differ only in whether the terms are `id:` or `thread:`. restoreSelectedThreads() already worked this way; this is the same reasoning applied to the message-scoped path. The flake was found by running one test five times rather than trusting a single green, and the fix verified the same way: 5 of 5, then the full suite three times over. Co-Authored-By: Claude Opus 5 --- src/keymap.cpp | 46 +++++++++++++++++- src/keymap.h | 8 ++++ src/mainwindow.cpp | 120 +++++++++++++++++++++++++++++++++++++++++++++- src/mainwindow.h | 19 ++++++++ src/notmuchworker.cpp | 27 +++++++++-- src/notmuchworker.h | 18 +++++++ tests/test_mainwindow.cpp | 78 ++++++++++++++++++++++++++++-- 7 files changed, 307 insertions(+), 9 deletions(-) (limited to 'src/mainwindow.h') diff --git a/src/keymap.cpp b/src/keymap.cpp index 319bc53..7a08a58 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -98,6 +98,22 @@ QList> KeyMap::defaultBindings() { QStringLiteral("Alt+Up"), QStringLiteral("prev_thread") }, { QStringLiteral("Return"), QStringLiteral("open_thread") }, { QStringLiteral("Ctrl+E"), QStringLiteral("archive") }, + // Del FIRST, and the order matters twice over. defaultSequenceFor() + // returns the first match, and sequenceFor() prefers any binding that + // is not that default, treating it as a user override; listing Del + // second therefore made it the "override" of Ctrl+D and left the two + // functions disagreeing about which key the menus should advertise. + // First also makes it the ADVERTISED one, which is the point: it is + // the key a user reaches for, and Ctrl+D is not a guess anyone makes. + // + // Bare, which is safe for a reason that does NOT generalise to other + // bare keys. Delete is not a letter, so Qt's protection for editable + // widgets does not cover it, but QLineEdit accepts the + // ShortcutOverride for Delete itself, because it is one of its own + // editing keys. Return is not, which is why that one needed an + // explicit filter in MainWindow::eventFilter() and this one does not. + // Measured both ways; see theDeleteKeyEditsTextInTheQueryBar(). + { QStringLiteral("Del"), QStringLiteral("delete") }, { QStringLiteral("Ctrl+D"), QStringLiteral("delete") }, // Restore is only enabled in the trash view, so its key is dead // elsewhere rather than doing something surprising. @@ -250,7 +266,7 @@ QKeySequence KeyMap::sequenceFor(const QString &action) const if (it.value() != action) continue; - const bool isBuiltIn = !builtIn.isEmpty() && it.key() == builtIn; + const bool isBuiltIn = isDefaultBinding(it.key(), action); if (best.isEmpty()) { best = it.key(); bestIsBuiltIn = isBuiltIn; @@ -260,6 +276,13 @@ QKeySequence KeyMap::sequenceFor(const QString &action) const if (bestIsBuiltIn && !isBuiltIn) { best = it.key(); bestIsBuiltIn = false; + } else if (bestIsBuiltIn && isBuiltIn) { + // Both are defaults, so the ADVERTISED one is whichever + // defaultBindings() lists first: that order is the author's + // preference and is why Del is listed before Ctrl+D. Falling back + // to alphabetical here would advertise Ctrl+D instead. + if (it.key() == builtIn) + best = it.key(); } else if (bestIsBuiltIn == isBuiltIn && it.key().toString() < best.toString()) { best = it.key(); @@ -268,6 +291,27 @@ QKeySequence KeyMap::sequenceFor(const QString &action) const return best; } +bool KeyMap::isDefaultBinding(const QKeySequence &sequence, + const QString &action) +{ + // ANY of the action's defaults, not just the first. + // + // An action can ship with more than one binding: `delete` has Del and + // Ctrl+D. sequenceFor() compares against defaultSequenceFor(), which + // returns only the first, so the second looked like a USER binding and + // won the "a user binding always beats the default" rule. The menus then + // advertised Ctrl+D for a user who had configured nothing, and + // sequenceFor() and defaultSequenceFor() disagreed about an untouched + // action. + for (const auto &binding : defaultBindings()) { + if (binding.second == action + && normalizeSequence(binding.first) == sequence) { + return true; + } + } + return false; +} + QKeySequence KeyMap::defaultSequenceFor(const QString &action) { for (const auto &binding : defaultBindings()) { diff --git a/src/keymap.h b/src/keymap.h index 81e1813..8774209 100644 --- a/src/keymap.h +++ b/src/keymap.h @@ -73,6 +73,14 @@ public: /// The built-in sequence for an action, ignoring any user override. static QKeySequence defaultSequenceFor(const QString &action); + /// Whether `sequence` is ANY of `action`'s default bindings. + /// + /// Not the same question as `sequence == defaultSequenceFor(action)`: an + /// action can ship several, and comparing against only the first makes the + /// others look like user overrides. + static bool isDefaultBinding(const QKeySequence &sequence, + const QString &action); + /// Every action name carrying a built-in binding. static QStringList defaultActions(); diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index da7128f..055e783 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -309,6 +309,18 @@ bool MainWindow::eventFilter(QObject *watched, QEvent *event) keyEvent->accept(); return true; } + // Delete needs NO entry here, and that is worth stating because the + // reasoning that says it does is nearly right. It is bound bare to + // `delete`, and Qt's protection for editable widgets covers plain + // LETTERS only, so by the same argument that made Return a problem it + // should trigger the action while the user edits a query. + // + // It does not, because QLineEdit accepts the ShortcutOverride for + // Delete itself: Delete is one of its own editing keys, which Return + // is not. Measured both ways, with this branch present and absent: + // the action fires 0 times either way and the text is edited either + // way. Adding a guard here would be dead code carrying a test that + // cannot fail. } return QMainWindow::eventFilter(watched, event); @@ -851,7 +863,7 @@ void MainWindow::registerActions() }); addAction(QStringLiteral("restore"), tr("&Restore from trash"), tr("Move the selected messages out of the trash"), [this]() { - restoreSelected(true); + restoreSelectedFromTrash(); }); addAction(QStringLiteral("spam"), tr("Mark &spam"), tr("Add spam and remove inbox"), [this]() { @@ -4414,6 +4426,11 @@ void MainWindow::onThreadMessagesResolved(const QStringList &messageIds, return; } + if (requestTag == QStringLiteral("restore_messages")) { + restoreResolvedMessages(messageIds, paths, tags); + return; + } + if (requestTag != QStringLiteral("undelete_thread")) return; @@ -4531,6 +4548,107 @@ QString MainWindow::inboxFolderFor(const Account &account) const return QStringLiteral("Inbox"); } +void MainWindow::restoreResolvedMessages(const QStringList &messageIds, + const QStringList &paths, + const QStringList &tags) +{ + if (messageIds.size() != paths.size() || messageIds.size() != tags.size()) + return; + + const QString prefix = QStringLiteral("deleted-from:"); + QHash byOrigin; + QHash byInbox; + QStringList stranded; + + for (int i = 0; i < messageIds.size(); ++i) { + 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; + } + } + + const Account account = accountForMessagePath(paths.at(i)); + if (account.maildir.isEmpty()) { + stranded.append(messageIds.at(i)); + continue; + } + + if (origin.isEmpty()) { + // Trashed by another client, so there is no record of where it + // belongs. Inbox is the documented fallback, and it is reported: + // a guess the user is not told about is worse than the guess. + byInbox[account.maildir + QLatin1Char('/') + + account.inboxFolder()] + .append(messageIds.at(i)); + continue; + } + byOrigin[account.maildir + QLatin1Char('/') + origin] + .append(messageIds.at(i)); + } + + for (auto it = byOrigin.cbegin(); it != byOrigin.cend(); ++it) { + // The origin tag is named here rather than left as the placeholder, + // which onMessagesMoved() would resolve to the folder the message is + // coming FROM, namely the trash. + const QString origin = originTagFor(it.key()); + QStringList remove{ QStringLiteral("deleted") }; + if (!origin.isEmpty()) + remove.append(origin); + sendMove(it.value(), it.key(), {}, remove, tr("Restore")); + } + + 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(byInbox.size()))); + } + if (!stranded.isEmpty()) { + m_statusLabel->setText( + tr("%n message(s) could not be restored: they belong to no " + "configured account.", "", int(stranded.size()))); + } +} + +void MainWindow::restoreSelectedFromTrash() +{ + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + if (rows.isEmpty()) + return; + + const ActionScope scope = m_model->messageScopeFor(rows); + if (scope.messageIds.isEmpty()) + return; + + // Resolved by the WORKER, not read from the model. + // + // The model's tags come from the QUERY, and a row whose delete has not yet + // 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 Restore sends it to the INBOX instead of the + // folder it came from, silently and irreversibly. + // + // A restore has to be right about the destination or it is worse than + // doing nothing, so it asks the database rather than trusting a view that + // may be a moment behind. restoreSelectedThreads() already worked this + // way; this is the same reasoning applied to the message-scoped path. + m_pendingRestoreIds = scope.messageIds; + QMetaObject::invokeMethod( + m_worker, "resolveMessages", Qt::QueuedConnection, + Q_ARG(QStringList, scope.messageIds), + Q_ARG(QString, QStringLiteral("restore_messages"))); +} + void MainWindow::restoreSelected(bool fallbackToInbox) { const QModelIndexList rows = diff --git a/src/mainwindow.h b/src/mainwindow.h index 937d87c..f9a075d 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -817,6 +817,25 @@ private: /// tag, so the tag comes off and the file stays where it is. void restoreSelected(bool fallbackToInbox = false); + /// Restore as reached from the TRASH VIEW: resolves each selected + /// message against the database first, then moves it. + /// + /// Asynchronous, unlike restoreSelected(), and that is the point. The + /// model's tags come from the query, so a row whose delete has not been + /// re-queried still carries its pre-delete tags; reading the origin from + /// there found none and sent the message to the INBOX instead of the + /// folder it came from, one run in three. + void restoreSelectedFromTrash(); + + /// Moves each resolved message home, using the tags and paths the WORKER + /// reported rather than anything the model holds. + void restoreResolvedMessages(const QStringList &messageIds, + const QStringList &paths, + const QStringList &tags); + + /// The messages a resolveMessages() request was made for. + QStringList m_pendingRestoreIds; + /// The account's inbox FOLDER name, discovered from its inbox query. /// /// Never hardcoded: the real Maildir has `Inbox` and a fixture has diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 62174c9..2c03226 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -776,15 +776,26 @@ void NotmuchWorker::moveMessages(const QStringList &messageIds, emit messagesMovedFrom(origins, destFolder); } +void NotmuchWorker::resolveMessages(const QStringList &messageIds, + const QString &requestTag) +{ + if (messageIds.isEmpty()) + return; + + QStringList terms; + terms.reserve(messageIds.size()); + for (const QString &id : messageIds) + terms.append(QStringLiteral("id:%1").arg(id)); + + resolveQuery(terms.join(QStringLiteral(" or ")), requestTag); +} + void NotmuchWorker::resolveThreadMessages(const QStringList &threadIds, const QString &requestTag) { if (threadIds.isEmpty()) return; - if (!openReadOnly()) - return; - // One combined query, for the reason applyTagsToThreads() gives: a query // per thread reopens the same Xapian cursor once per selected row. QStringList terms; @@ -792,7 +803,15 @@ void NotmuchWorker::resolveThreadMessages(const QStringList &threadIds, for (const QString &id : threadIds) terms.append(QStringLiteral("thread:%1").arg(id)); - const QString query = terms.join(QStringLiteral(" or ")); + resolveQuery(terms.join(QStringLiteral(" or ")), requestTag); +} + +void NotmuchWorker::resolveQuery(const QString &query, + const QString &requestTag) +{ + if (!openReadOnly()) + return; + NmQuery nmQuery(notmuch_query_create(m_db, query.toUtf8().constData())); if (!nmQuery) { emit errorOccurred(QStringLiteral("Cannot resolve selected threads")); diff --git a/src/notmuchworker.h b/src/notmuchworker.h index dfd4303..9932e59 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -158,6 +158,24 @@ public slots: void resolveThreadMessages(const QStringList &threadIds, const QString &requestTag); + /// The same walk for a set of MESSAGE ids rather than thread ids. + /// + /// Restore needs each message's tags and path to decide where to send it, + /// and must not read them from 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 and the origin tag is missing. A restore that guesses + /// the destination is worse than one that does nothing. + void resolveMessages(const QStringList &messageIds, + const QString &requestTag); + +private: + /// The shared walk behind resolveMessages() and resolveThreadMessages(): + /// runs `query` and emits threadMessagesResolved() with each match's id, + /// database-relative path and tab-joined tags. + void resolveQuery(const QString &query, const QString &requestTag); + +public slots: + /// Every tag in the database, sorted. Feeds query bar completion, which /// cannot offer tag names it has no way to enumerate. Called at startup, /// after a sync, and after a tag mutation introduces an unknown tag. diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index cefd686..85418a4 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -377,6 +377,8 @@ private slots: void deletingAThreadRootTwiceRestoresItRatherThanRedeleting(); void deleteThreadMovesEveryMessageAndRepaintsTheRootCard(); void aFolderNameWithASpaceSurvivesTheRoundTrip(); + void deleteIsBoundToTheDeleteKey(); + void theDeleteKeyEditsTextInTheQueryBar(); void restoreIsReachableWithoutTheKeyboard(); void restoreIsOnlyEnabledInTheTrashView(); void restoreReturnsAMessageToItsOriginFolder(); @@ -9466,6 +9468,68 @@ void TestMainWindow::aFolderNameWithASpaceSurvivesTheRoundTrip() "messages are somewhere mbsync will never sync"); } +void TestMainWindow::deleteIsBoundToTheDeleteKey() +{ + // Del is the key a user reaches for, and Ctrl+D is not a guess anyone + // makes. Both are bound; this asserts the bare one is really there, + // since setShortcut() keeps only the LAST of several and silently drops + // the rest, which would leave the documented binding absent. + const Config config; + MainWindow window(config); + + auto *action = window.findChild(QStringLiteral("delete")); + QVERIFY(action); + + QVERIFY2(action->shortcuts().contains(QKeySequence(Qt::Key_Delete)), + qPrintable(QStringLiteral("delete is not on the Del key; it has: %1") + .arg(QKeySequence::listToString(action->shortcuts())))); +} + +void TestMainWindow::theDeleteKeyEditsTextInTheQueryBar() +{ + // `delete` is bound to bare Del, and a QAction shortcut is dispatched + // BEFORE the focused widget sees the key. Qt withholds only plain LETTERS + // from editable widgets, so by the argument that made bare Return break + // the query bar, Delete should move mail to the trash while the user is + // editing a query. + // + // It does not: QLineEdit accepts the ShortcutOverride for Delete itself, + // because Delete is one of its own editing keys, which Return is not. That + // is a property of Qt rather than of this code, which is exactly why it is + // pinned here: it is the assumption the bare binding rests on, and if a + // future Qt or a future focus proxy changes it, mail gets deleted while + // someone types. + // + // Asserted on the ACTION not firing, not on the ShortcutOverride phase. A + // probe on the override reports notify=1 accepted=1 whether or not this + // window filters the key, since QLineEdit accepts it either way, so it + // cannot distinguish the two and passes against any implementation. + // Measured, while trying to write this test the obvious way. + const Config config; + MainWindow window(config); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + auto *deleteAction = window.findChild(QStringLiteral("delete")); + QVERIFY(queryEdit && deleteAction); + + int fired = 0; + QObject::connect(deleteAction, &QAction::triggered, + [&fired]() { ++fired; }); + + queryEdit->setFocus(); + QTRY_VERIFY(queryEdit->hasFocus()); + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->setCursorPosition(0); + + QTest::keyClick(queryEdit, Qt::Key_Delete); + + QCOMPARE(fired, 0); + QCOMPARE(queryEdit->text(), QStringLiteral("ag:inbox")); +} + void TestMainWindow::restoreIsReachableWithoutTheKeyboard() { // Restore shipped as a keyboard shortcut and nothing else: registered, @@ -9603,15 +9667,23 @@ void TestMainWindow::restoreReturnsAMessageToItsOriginFolder() || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), stem), 15000); + // Waited on the ORIGIN tag, not on `deleted`. + // + // Both come off in one write, but the file rename and the tag write are + // separate operations and the assertions below raced the second one: + // measured 1 failure in 3 runs waiting on `deleted` alone, reporting the + // origin tag still present. Waiting on the tag this test is actually about + // removes the race rather than papering over it with a longer timeout. + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, QStringLiteral("id:ro1@example.org and " + "tag:\"deleted-from:inbox\"")) == 0, + 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)); } -- cgit v1.2.3 From 44b341f774a706d70509751b0251793e1c5a34f5 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Wed, 19 Aug 2026 09:54:53 +0200 Subject: feat: find mail tagged deleted but never moved to trash Every version before item 103 tagged a message `deleted` and left its file exactly where it was, so deleted mail accumulated in the inboxes with only a chip to say otherwise. `Find stranded deleted mail` runs the query that finds it: tagged `deleted`, and not inside any configured trash folder. It reports and moves nothing. Acting on its own would be a bulk delete with no selection behind it, and the user asked for something they could come back to and review. Repeatable rather than a startup migration, for the same reason: mail reaches this state again whenever another client tags without moving. A menu entry only, at the user's request, so it cannot be confused with the Trash filter beside the other four. Also adds everyActionIsReachableFromAMenu(), which asserts the fifth registration site nothing enforced. CLAUDE.md documents four places; a menu is the fifth, and `restore` shipped on this branch reachable by a chord and by nothing a user could see. The new test found three more of the same: open_thread, clear_pane and clear_selection were all keyboard-only. All three now sit in the View menu. Co-Authored-By: Claude Opus 5 --- src/keymap.cpp | 6 ++ src/mainwindow.cpp | 52 ++++++++++++ src/mainwindow.h | 11 +++ tests/test_mainwindow.cpp | 206 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 275 insertions(+) (limited to 'src/mainwindow.h') diff --git a/src/keymap.cpp b/src/keymap.cpp index 7a08a58..76c6b60 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -32,6 +32,7 @@ QStringList KeyMap::knownActions() QStringLiteral("archive"), QStringLiteral("delete"), QStringLiteral("restore"), + QStringLiteral("cleanup_stranded"), QStringLiteral("spam"), QStringLiteral("toggle_unread"), QStringLiteral("mark_all_read"), @@ -118,6 +119,11 @@ QList> KeyMap::defaultBindings() // Restore is only enabled in the trash view, so its key is dead // elsewhere rather than doing something surprising. { QStringLiteral("Ctrl+R"), QStringLiteral("restore") }, + // Item 103's cleanup. A chord rather than a plain key: it replaces the + // whole view, and it is reached from a menu far more often than from + // the keyboard. Ctrl+Shift+D is message_details and Ctrl+Alt+D is + // delete_thread, so this takes the T of "trash". + { QStringLiteral("Ctrl+Alt+T"), QStringLiteral("cleanup_stranded") }, { 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 055e783..a1c01c3 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -865,6 +865,12 @@ void MainWindow::registerActions() tr("Move the selected messages out of the trash"), [this]() { restoreSelectedFromTrash(); }); + addAction(QStringLiteral("cleanup_stranded"), + tr("Find &stranded deleted mail"), + tr("Show mail tagged deleted that is not in a trash folder"), + [this]() { + showStrandedDeletedMail(); + }); addAction(QStringLiteral("spam"), tr("Mark &spam"), tr("Add spam and remove inbox"), [this]() { tagSelected({ QStringLiteral("spam") }, { QStringLiteral("inbox") }, @@ -1168,11 +1174,23 @@ void MainWindow::buildMenus() // Separated from the entries above: those act on the selection, this edits // a rule store shared with mailctl and changes nothing that is on screen. messageMenu->addSeparator(); + // A MENU entry and nothing else, at the user's request: "the cleanup + // should be a menu entry only, not to be confused with the filter Trash". + // It replaces the whole view like a filter does, so a sixth button beside + // the five filters would read as one of them. + messageMenu->addAction(m_actions.value(QStringLiteral("cleanup_stranded"))); messageMenu->addAction(m_actions.value(QStringLiteral("tag_rules"))); auto *viewMenu = menuBar()->addMenu(tr("&View")); viewMenu->addAction(m_actions.value(QStringLiteral("prev_thread"))); viewMenu->addAction(m_actions.value(QStringLiteral("next_thread"))); + viewMenu->addAction(m_actions.value(QStringLiteral("open_thread"))); + viewMenu->addSeparator(); + // The two clears. Both shipped keyboard-only, which is what + // everyActionIsReachableFromAMenu() exists to stop: an action reachable + // only by a chord is an action nobody discovers. + viewMenu->addAction(m_actions.value(QStringLiteral("clear_pane"))); + viewMenu->addAction(m_actions.value(QStringLiteral("clear_selection"))); viewMenu->addSeparator(); viewMenu->addAction(m_actions.value(QStringLiteral("toggle_html"))); viewMenu->addAction(m_actions.value(QStringLiteral("load_remote"))); @@ -1217,6 +1235,10 @@ void MainWindow::buildMenus() // 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") }, + // A SEARCH, not a delete. The action reports what it finds and moves + // nothing, so an icon from the delete family would promise the one + // thing it deliberately does not do. + { QStringLiteral("cleanup_stranded"), QStringLiteral("system-search") }, { QStringLiteral("undo"), QStringLiteral("edit-undo") }, { QStringLiteral("spam"), QStringLiteral("mail-mark-junk") }, { QStringLiteral("flag"), QStringLiteral("mail-mark-important") }, @@ -4649,6 +4671,36 @@ void MainWindow::restoreSelectedFromTrash() Q_ARG(QString, QStringLiteral("restore_messages"))); } +void MainWindow::showStrandedDeletedMail() +{ + // Not scoped to the selected account, deliberately. The stranded mail is + // an artefact of an old version rather than a view of anything, and the + // user wants to see all of it at once; the account dropdown is still there + // to narrow it by hand afterwards. + const QString trash = m_config.allTrashQuery(); + + // No account configures a trash folder: everything tagged `deleted` is by + // definition stranded, since there is nowhere for it to have gone. An + // empty exclusion must never be written as `not ()`, which notmuch parses + // without complaint and matches nothing, reporting a clean database. + const QString query = + trash.isEmpty() + ? QStringLiteral("tag:deleted") + : QStringLiteral("tag:deleted and not (%1)").arg(trash); + + // Into the bar, like a filter: what ran is visible and editable, and + // AlreadyScoped stops runQuery() wrapping it in the selected account's + // path, which would hide every other account's stranded mail. + m_queryEdit->setText(query); + runQuery(FlatResult::No, AccountScope::AlreadyScoped); + + // After runQuery(), which sets "Searching...": set before it, this would + // be overwritten and the user would be told nothing about what they are + // looking at. + m_statusLabel->setText(tr("Mail tagged deleted but not in a trash folder. " + "Select what should go and press Delete.")); +} + void MainWindow::restoreSelected(bool fallbackToInbox) { const QModelIndexList rows = diff --git a/src/mainwindow.h b/src/mainwindow.h index f9a075d..5b1621f 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -827,6 +827,17 @@ private: /// folder it came from, one run in three. void restoreSelectedFromTrash(); + /// Runs the query that finds mail tagged `deleted` whose file never left + /// its original folder, which is what every version before item 103 left + /// behind. It REPORTS and moves nothing: acting on its own would be a bulk + /// delete with no selection behind it, and the user asked for something + /// they could come back to and review. + /// + /// Repeatable rather than a one-time startup migration, for the same + /// reason: mail reaches this state again whenever another client tags + /// without moving. + void showStrandedDeletedMail(); + /// Moves each resolved message home, using the tags and paths the WORKER /// reported rather than anything the model holds. void restoreResolvedMessages(const QStringList &messageIds, diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 85418a4..8a41c85 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -330,6 +331,7 @@ private slots: void aCronSyncDoesNotClearAnEditMadeWhileItRan(); void everyActionCarriesAnIcon(); + void everyActionIsReachableFromAMenu(); void theToolbarDoesNotOverrideTheDesktopButtonStyle(); void theImportantActionIsLabelledImportant(); void theImportantActionStillWritesTheFlaggedTag(); @@ -383,6 +385,8 @@ private slots: void restoreIsOnlyEnabledInTheTrashView(); void restoreReturnsAMessageToItsOriginFolder(); void restoreFallsBackToInboxWithoutAnOriginTag(); + void theCleanupQueryFindsStrandedMail(); + void theCleanupQueryExcludesMailAlreadyInTrash(); private: /// Owns the throwaway lock table init() points every test at. A pointer @@ -6384,6 +6388,75 @@ void TestMainWindow::aCronSyncDoesNotClearAnEditMadeWhileItRan() // Items 56 and 57. +void TestMainWindow::everyActionIsReachableFromAMenu() +{ + // The fourth registration site nothing enforced. CLAUDE.md says adding an + // action is four places: knownActions(), defaultBindings(), the icon table + // and the action itself. It is FIVE, and the fifth is a menu. + // + // Found the hard way on the trash branch: `restore` shipped keyboard-only, + // reachable by a chord and by nothing a user could see or discover, and no + // test noticed. The three existing coverage tests each assert a different + // property and all three pass against an action that appears nowhere in + // the interface. + // + // The MENU rather than the toolbar, since the toolbar is a small + // deliberate subset and always will be. Every menu is walked, submenus + // included, because the five whole-thread actions live only in the "Whole + // thread" submenu. + const Config config; + MainWindow window(config); + + auto *bar = window.menuBar(); + QVERIFY(bar); + + QSet reachable; + QList pending; + const auto topLevel = bar->actions(); + for (QAction *action : topLevel) { + if (action->menu()) + pending.append(action->menu()); + } + QVERIFY2(!pending.isEmpty(), "the menu bar holds no menus"); + + while (!pending.isEmpty()) { + QMenu *menu = pending.takeFirst(); + const auto entries = menu->actions(); + for (QAction *entry : entries) { + if (QMenu *sub = entry->menu()) { + pending.append(sub); + // An action owning a menu emits no `triggered`, so it is the + // submenu that makes its children reachable and never the + // parent entry itself. Not counted as reachable. + continue; + } + reachable.insert(entry); + } + } + + // The guard, before anything is asserted about what is missing: a walk + // that found nothing would report every action as unreachable and read as + // a catastrophic regression rather than as a broken probe. + QVERIFY2(reachable.size() > 10, + qPrintable(QStringLiteral("the menu walk found only %1 entries") + .arg(reachable.size()))); + + QStringList unreachable; + for (const QString &name : KeyMap::knownActions()) { + auto *action = window.findChild(name); + QVERIFY2(action, qPrintable(QStringLiteral("no action named %1").arg(name))); + if (!reachable.contains(action)) + unreachable.append(name); + } + + QVERIFY2(unreachable.isEmpty(), + qPrintable(QStringLiteral("%1 action(s) reach no menu, so they " + "exist only for whoever already knows " + "the chord: %2") + .arg(unreachable.size()) + .arg(unreachable.join(QStringLiteral(", "))))); +} + void TestMainWindow::everyActionCarriesAnIcon() { // Item 56. The complaint was inconsistency, not absence: eight actions had @@ -8814,6 +8887,22 @@ static int notmuchCount(const QString &configPath, const QString &query) return ok ? count : -1; } +/// Applies a tag change with the notmuch binary, for the one thing the UI +/// cannot produce any more: a message tagged `deleted` while its file is still +/// in the inbox. That is the state the OLD Delete left mail in, and the state +/// the cleanup action exists to find, so a test for it has to write it +/// directly rather than through an action that now moves the file too. +static bool notmuchTag(const QString &configPath, const QStringList &args) +{ + QProcess process; + QProcessEnvironment env = QProcessEnvironment::systemEnvironment(); + env.insert(QStringLiteral("NOTMUCH_CONFIG"), configPath); + process.setProcessEnvironment(env); + process.start(QStringLiteral("notmuch"), + QStringList{ QStringLiteral("tag") } + args); + return process.waitForFinished(15000) && process.exitCode() == 0; +} + static bool folderHasMessageFile(const QString &dir, const QString &stem) { QDir directory(dir); @@ -10107,4 +10196,121 @@ void TestMainWindow::deleteWithoutATrashFolderSaysSoRatherThanDoingNothing() QStringLiteral("notrash.example.org"))); } +void TestMainWindow::theCleanupQueryFindsStrandedMail() +{ + // The state 848 real messages are in today: tagged `deleted` by a version + // of Delete that only ever tagged, with the file still sitting in the + // inbox. Nothing moves them on their own, so the action reports them and + // the user decides. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("strand@example.org"), + QStringLiteral("Tagged but never moved"), + QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body text."))); + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("keep@example.org"), + QStringLiteral("Perfectly ordinary mail"), + 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())); + + const QString cfg = backed.fixture().configPath(); + QVERIFY(notmuchTag(cfg, { QStringLiteral("+deleted"), + QStringLiteral("--"), + QStringLiteral("id:strand@example.org") })); + // The guard, before anything is asserted about what the action finds: one + // message is stranded and one is not, so a query that simply returns + // everything cannot pass. + QCOMPARE(notmuchCount(cfg, QStringLiteral("tag:deleted")), 1); + + MainWindow window(backed.config()); + auto *model = window.findChild(); + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + auto *cleanup = + window.findChild(QStringLiteral("cleanup_stranded")); + QVERIFY(model && queryEdit); + QVERIFY2(cleanup, "there is no cleanup_stranded action"); + + cleanup->trigger(); + + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + // The query lands in the bar, like every other generated query, so what + // ran is visible and the user can edit it. + QVERIFY2(queryEdit->text().contains(QStringLiteral("tag:deleted")), + qPrintable(QStringLiteral("the bar holds '%1'") + .arg(queryEdit->text()))); + QVERIFY2(queryEdit->text().contains(QStringLiteral("not ")), + qPrintable(QStringLiteral("the bar holds '%1'") + .arg(queryEdit->text()))); + + // It reports and moves NOTHING. A cleanup that acted on its own would be a + // bulk delete with no selection behind it, which is the opposite of what + // the user asked for. + const QString mail = backed.fixture().maildirPath(); + QVERIFY(folderHasMessageFile(mail + QStringLiteral("/acct/inbox/new"), + QStringLiteral("strand.example.org")) + || folderHasMessageFile(mail + QStringLiteral("/acct/inbox/cur"), + QStringLiteral("strand.example.org"))); + QCOMPARE(notmuchCount(cfg, QStringLiteral("path:\"acct/Trash/**\"")), 0); +} + +void TestMainWindow::theCleanupQueryExcludesMailAlreadyInTrash() +{ + // Properly trashed mail carries the tag AND sits in the folder. Without + // the exclusion this reports every deleted message ever, which makes the + // action useless the moment Delete starts working. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("cln1@example.org"), + QStringLiteral("Going to the trash"), + 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 cfg = backed.fixture().configPath(); + const QString mail = backed.fixture().maildirPath(); + + 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, never of the list: rowCount() reads 0 for the + // whole interval before the worker answers, so "the cleanup found + // nothing" would pass against a delete that never happened. + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(mail + QStringLiteral("/acct/Trash/cur"), + QStringLiteral("cln1.example.org")), + 15000); + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, QStringLiteral("tag:deleted")) == 1, 15000); + + auto *cleanup = + window.findChild(QStringLiteral("cleanup_stranded")); + QVERIFY2(cleanup, "there is no cleanup_stranded action"); + cleanup->trigger(); + + // The query the action ran, asked of notmuch directly. The list is the + // wrong instrument for an emptiness claim, for the reason above. + QTRY_VERIFY_WITH_TIMEOUT(!queryEdit->text().isEmpty(), 15000); + QCOMPARE(notmuchCount(cfg, queryEdit->text()), 0); +} + #include "test_mainwindow.moc" -- cgit v1.2.3