diff options
| -rw-r--r-- | src/keymap.cpp | 48 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 532 | ||||
| -rw-r--r-- | src/mainwindow.h | 117 | ||||
| -rw-r--r-- | src/notmuchworker.cpp | 7 | ||||
| -rw-r--r-- | src/threadlistmodel.cpp | 67 | ||||
| -rw-r--r-- | src/threadlistmodel.h | 37 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 866 | ||||
| -rw-r--r-- | tests/test_notmuchworker.cpp | 32 | ||||
| -rw-r--r-- | tests/test_threadlistmodel.cpp | 192 | ||||
| -rw-r--r-- | translations/qtmaildir_it_IT.ts | 92 |
10 files changed, 1062 insertions, 928 deletions
diff --git a/src/keymap.cpp b/src/keymap.cpp index 6605882..17f8241 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -43,26 +43,6 @@ QStringList KeyMap::knownActions() QStringLiteral("edit_tags"), QStringLiteral("tag_rules"), QStringLiteral("flag"), - // The whole-thread counterparts (item 108). The names above act on the - // message a row displays; these act on its entire thread. Separate - // names rather than a scope flag, because a name is what a user writes - // in [keys]: giving `delete` new semantics would silently change an - // existing config, and renaming it would break one that mentions it. - // - // Unbound by default. They are reached through the "Whole thread" - // submenu, and inventing five more default chords for actions most - // users will rarely want is worse than leaving them to bind what they - // use. - QStringLiteral("archive_thread"), - QStringLiteral("delete_thread"), - QStringLiteral("spam_thread"), - // Item 112 split the thread toggle in two. Neither carries a default - // chord, at the user's choice: since item 132 a shortcut is a chosen - // subset rather than a requirement, and Ctrl+Alt+U meant whichever - // direction the union happened to pick, which is what made it wrong. - QStringLiteral("mark_thread_read"), - QStringLiteral("mark_thread_unread"), - QStringLiteral("flag_thread"), // Compose and send (item 123). save_message deliberately carries no // default chord: since item 132 a shortcut is a chosen subset rather // than a requirement, and writing the raw message to a file is the @@ -121,17 +101,14 @@ QList<QPair<QString, QString>> KeyMap::defaultBindings() // Compose and send (item 123), listed where the Message menu presents // them: composing sits above organising. // - // PROVISIONAL. The user intends to rework the bindings, and - // Ctrl+Alt+R for reply_no_quote is an imperfect fit: the Ctrl+Alt tier - // elsewhere means a WIDER SCOPE (the five whole-thread actions), not a - // variant of the same scope. + // PROVISIONAL. The user intends to rework the bindings. // // Each was checked against every sequence in this table, not merely // against the lines above it: these sit near the top, so most of the // table is BELOW them, Ctrl+Shift+U and Ctrl+Shift+S among it. - // Checking only upwards would miss exactly those. The near misses: - // Ctrl+R is restore, Ctrl+A is select_all and Ctrl+Alt+S is - // spam_thread, so none of these five is a reuse. + // Checking only upwards would miss exactly those. The near misses are + // Ctrl+R for restore and Ctrl+A for select_all, so none of these five + // is a reuse. // // save_message gets none. Item 132 made a chord a chosen subset rather // than a requirement, and this is the escape hatch nobody presses a @@ -164,8 +141,8 @@ QList<QPair<QString, QString>> KeyMap::defaultBindings() { 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". + // the keyboard. Ctrl+Shift+D is message_details, 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") }, @@ -174,19 +151,6 @@ QList<QPair<QString, QString>> KeyMap::defaultBindings() // action takes the harder chord rather than the easier one. { QStringLiteral("Ctrl+Shift+U"), QStringLiteral("mark_all_read") }, { QStringLiteral("Ctrl+I"), QStringLiteral("flag") }, - // The whole-thread tier (item 108), one modifier out from each - // message-scoped twin: Ctrl+D deletes the message a row displays, - // Ctrl+Alt+D deletes its conversation. - // - // Ctrl+ALT, not Ctrl+Shift. The obvious pairing is taken twice over: - // Ctrl+Shift+S is `spam` and Ctrl+Shift+U is `mark_all_read`, both - // shipped and both in users' fingers. Reusing either would silently - // change what an existing key does, which is the same objection that - // made these separate action names rather than a flag on the old ones. - { QStringLiteral("Ctrl+Alt+E"), QStringLiteral("archive_thread") }, - { QStringLiteral("Ctrl+Alt+D"), QStringLiteral("delete_thread") }, - { QStringLiteral("Ctrl+Alt+S"), QStringLiteral("spam_thread") }, - { QStringLiteral("Ctrl+Alt+I"), QStringLiteral("flag_thread") }, { QStringLiteral("Ctrl+T"), QStringLiteral("edit_tags") }, // Shifted against Ctrl+T for the same reason Ctrl+Shift+U is shifted // against Ctrl+U: this is the standing version of tagging, applied to diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index b64e20b..7d9cfbc 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1011,14 +1011,24 @@ void MainWindow::composeNew() void MainWindow::composeReply(ComposeContext::Kind kind, bool quote) { - // messageScopeFor() semantics, NOT threadFor(): a thread row means the one - // message its card shows, a reply row means itself. Replying to a thread - // is meaningless; a reply answers a message. - // + const QModelIndex current = m_threadView->currentIndex(); + + // A CONVERSATION row answers the thread, not a message (item 177). The + // old comment here said "replying to a thread is meaningless; a reply + // answers a message", and half of that survives: this still resolves to + // one message. What changed is WHICH one. A conversation row's card + // shows the thread's FIRST message, and answering that would thread the + // reply off the opening post of a discussion that has moved on, so the + // newest message is asked for instead. Only `reply` reaches here on such + // a row; refreshScopedActionLabels() hides the other four. + if (m_model->isConversationRow(current)) { + replyToThread(m_model->threadFor(current).threadId); + return; + } + // It takes a QModelIndexList, not a single index, so the current index is // wrapped rather than passed bare. - const ActionScope scope = - m_model->messageScopeFor({ m_threadView->currentIndex() }); + const ActionScope scope = m_model->scopeForSelection({ current }); if (scope.messageIds.isEmpty()) { showTransientStatus(tr("No message is selected")); return; @@ -1031,6 +1041,27 @@ void MainWindow::composeReply(ComposeContext::Kind kind, bool quote) requestMessageForCompose(scope.messageIds.first(), kind, quote); } +void MainWindow::replyToThread(const QString &threadId) +{ + if (threadId.isEmpty()) { + showTransientStatus(tr("No thread is selected")); + return; + } + + // A round trip, because the model cannot answer this. The summary carries + // only the thread's FIRST message, and an unexpanded conversation holds no + // nodes for its replies at all, so the newest message's id exists only in + // the database. resolveQuery() sorts newest-first explicitly for this. + // + // m_pendingThreadScope is deliberately NOT set. It carries the ids a MOVE + // is about to apply to, and a reply moves nothing; setting it would leave + // a delete arriving next reading a scope this gesture left behind. + QMetaObject::invokeMethod( + m_worker, "resolveThreadMessages", Qt::QueuedConnection, + Q_ARG(QStringList, QStringList{ threadId }), + Q_ARG(QString, QStringLiteral("reply_thread"))); +} + void MainWindow::editDraft() { editDraftAt(m_threadView->currentIndex()); @@ -1038,9 +1069,9 @@ void MainWindow::editDraft() void MainWindow::editDraftAt(const QModelIndex &index) { - // messageScopeFor(), like composeReply(): a thread row means the one - // message its card shows. - const ActionScope scope = m_model->messageScopeFor({ index }); + // A draft is one message by construction, so a drafts row is never a + // conversation and this needs no thread branch of its own. + const ActionScope scope = m_model->scopeForSelection({ index }); if (scope.messageIds.isEmpty()) { showTransientStatus(tr("No message is selected")); return; @@ -1445,7 +1476,16 @@ void MainWindow::updateComposeActions() void MainWindow::saveDisplayedMessage(const QString &chosenDirectory) { const QModelIndex current = m_threadView->currentIndex(); - const ActionScope scope = m_model->messageScopeFor({ current }); + + // Never reached on a conversation row: refreshScopedActionLabels() hides + // Save there, because a conversation names no single file. Guarded anyway, + // since a hidden QAction still fires from a shortcut. + if (m_model->isConversationRow(current)) { + showTransientStatus(tr("Select one message to save")); + return; + } + + const ActionScope scope = m_model->scopeForSelection({ current }); if (scope.messageIds.isEmpty()) { showTransientStatus(tr("No message is selected")); return; @@ -1746,77 +1786,6 @@ void MainWindow::registerActions() editTagsOnSelection(); }); - // The whole-thread counterparts (item 108). Separate action NAMES, because - // a name is what a user writes in [keys]: reusing `delete` with new - // semantics would silently change what an existing config does, and - // renaming it would break one that mentions it. These are unbound by - // default; the submenu is how they are reached. - // - // Each one is its message-scoped twin with TagScope::Thread, so the two - // cannot drift in what they write, only in what they write it to. - addAction(QStringLiteral("archive_thread"), tr("&Archive thread"), - tr("Remove inbox from every message of the selected threads"), - [this]() { - tagSelected({}, { QStringLiteral("inbox") }, tr("Archive thread"), - TagScope::Thread); - }); - 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)) { - restoreSelectedThreads(); - } else { - trashSelectedThreads(); - } - }); - addAction(QStringLiteral("spam_thread"), tr("Mark thread as &spam"), - tr("Add spam and remove inbox on whole threads"), [this]() { - tagSelected({ QStringLiteral("spam") }, { QStringLiteral("inbox") }, - tr("Mark thread spam"), TagScope::Thread); - }); - // Two fixed directions rather than one toggle, and the asymmetry with the - // message-scoped twin is the point (item 112). `ThreadSummary::tags` is - // notmuch's UNION over the conversation, so a thread holding even one - // unread message answers "unread" and a toggle reading that predicate - // always chose "mark read": there was no input that reached "mark thread - // unread" on a mixed thread, which is exactly the thread a user wants it - // for. A union is not a state, and a toggle needs a state. - // - // The message-scoped `toggle_unread` stays a toggle, because one message - // has a real two-valued state. Do not unify them. - addAction(QStringLiteral("mark_thread_read"), tr("Mark thread &read"), - tr("Remove the unread tag from every message of the selected " - "threads"), [this]() { - m_markReadTimer->stop(); - m_markReadMessageId.clear(); - tagSelected({}, { QStringLiteral("unread") }, - tr("Mark thread read"), TagScope::Thread); - }); - addAction(QStringLiteral("mark_thread_unread"), tr("Mark thread &unread"), - tr("Add the unread tag to every message of the selected threads"), - [this]() { - // Cancels the automatic mark-read for the same reason its - // message-scoped twin does: a thread marked unread by hand must not be - // undone a moment later by a timer armed when it was opened. - m_markReadTimer->stop(); - m_markReadMessageId.clear(); - tagSelected({ QStringLiteral("unread") }, {}, - tr("Mark thread unread"), TagScope::Thread); - }); - addAction(QStringLiteral("flag_thread"), tr("&Important"), - tr("Mark every message of the selected threads as important"), - [this]() { - tagSelected({ QStringLiteral("flagged") }, {}, - tr("Mark thread important"), TagScope::Thread); - }); addAction(QStringLiteral("tag_rules"), tr("Tagging &rules..."), tr("Edit the rules that tag mail as it arrives"), [this]() { showTagRulesDialog(); @@ -2037,8 +2006,6 @@ void MainWindow::buildMenus() messageMenu->addAction(m_actions.value(QStringLiteral("mark_all_read"))); messageMenu->addAction(m_actions.value(QStringLiteral("edit_tags"))); messageMenu->addAction(m_actions.value(QStringLiteral("flag"))); - messageMenu->addSeparator(); - messageMenu->addMenu(buildThreadActionsMenu(messageMenu)); // 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(); @@ -2142,25 +2109,12 @@ void MainWindow::buildMenus() { QStringLiteral("zoom_out"), QStringLiteral("zoom-out") }, { QStringLiteral("zoom_reset"), QStringLiteral("zoom-original") }, - // The whole-thread tier (item 108) deliberately SHARES each icon with - // its message-scoped twin. The no-duplicates rule exists because the - // toolbar can be icon-only, where the icon is the entire control; - // these five never reach the toolbar. They live in a submenu whose - // entries always carry text, and "Delete thread" beside the delete - // icon is the honest pairing: the same operation, a wider scope, with - // the words saying which. Inventing five different shapes for the same - // five operations would be less clear, not more. - { QStringLiteral("archive_thread"), QStringLiteral("mail-archive") }, - { QStringLiteral("delete_thread"), QStringLiteral("edit-delete") }, - { QStringLiteral("spam_thread"), QStringLiteral("mail-mark-junk") }, - { QStringLiteral("mark_thread_read"), QStringLiteral("mail-mark-read") }, - { QStringLiteral("mark_thread_unread"), QStringLiteral("mail-mark-unread") }, - { QStringLiteral("flag_thread"), QStringLiteral("mail-mark-important") }, - - // Compose and send (item 123). reply_no_quote SHARES reply's icon for - // the same reason the five above share theirs: it never reaches the - // toolbar, it is a menu entry that always carries its text, and - // "Reply without quoting" beside the reply icon is the honest pairing. + // Compose and send (item 123). reply_no_quote SHARES reply's icon, + // which the no-duplicates rule allows because that rule exists for the + // icon-only TOOLBAR, where the icon is the entire control: it never + // reaches the toolbar, it is a menu entry that always carries its + // text, and "Reply without quoting" beside the reply icon is the + // honest pairing. // It is named in the exception list in noTwoActionsShareAnIcon(), so // putting it on the toolbar fails that test rather than passing // silently. @@ -2200,8 +2154,6 @@ void MainWindow::buildMenus() m_threadContextMenu->addAction(m_actions.value(QStringLiteral("flag"))); m_threadContextMenu->addAction(m_actions.value(QStringLiteral("edit_tags"))); m_threadContextMenu->addSeparator(); - m_threadContextMenu->addMenu(buildThreadActionsMenu(m_threadContextMenu)); - m_threadContextMenu->addSeparator(); m_threadContextMenu->addAction(m_actions.value(QStringLiteral("select_all"))); m_threadView->setContextMenuPolicy(Qt::CustomContextMenu); @@ -3698,35 +3650,66 @@ void MainWindow::refreshTrashActions() // moveMessages() finds the file already in the destination and takes its // early-return branch, which counts an unsynced change for a move that // never happened (item 168). - if (auto *del = m_actions.value(QStringLiteral("delete"))) - del->setVisible(!haveSelection || !inTrash); + // ...and hidden on a reply row as well (item 177): deleting is a + // conversation-level act, so a reply offers no Delete at all. The two + // hides are ORed rather than fought over, which is why this reads a flag + // refreshScopedActionLabels() sets instead of walking the selection twice. + if (auto *del = m_actions.value(QStringLiteral("delete"))) { + del->setVisible((!haveSelection || !inTrash) + && !m_replySelectionHidesDelete); + } // The mirror, which shipped beside it: Restore was added unconditionally // to both menus and so was offered on mail that was never deleted. - if (auto *restore = m_actions.value(QStringLiteral("restore"))) - restore->setVisible(!haveSelection || inTrash); + if (auto *restore = m_actions.value(QStringLiteral("restore"))) { + restore->setVisible((!haveSelection || inTrash) + && !m_replySelectionHidesDelete); + } } void MainWindow::refreshUnreadAction() { // The user's design (item 112 and its duplicates 99/147): the label says // which way the action will go, and on a selection with no single state - // the entry is HIDDEN rather than labelled wrongly. The thread submenu is - // then the route, whose entries are absolute and work whatever the mix. + // the entry is HIDDEN rather than labelled wrongly. + // + // The "Whole thread" submenu used to be the route out of the hidden case, + // with two absolute entries that worked whatever the mix. It is gone + // (item 177), and nothing replaces it: on a conversation row the label + // now names the THREAD, and the thread's union is a single state for the + // same reason it was not a message's. A genuinely mixed MULTI-row + // selection still hides the entry, and Edit tags beside it is the route. auto *action = m_actions.value(QStringLiteral("toggle_unread")); if (!action) return; + // Mixed as well as Conversations: the write really will take whole threads + // for the rows that are conversations, so the wider claim is the honest + // one. Same rule as refreshScopedActionLabels(), which must not disagree. + const SelectionKind kind = selectionKind(); + const bool namesTheThread = kind == SelectionKind::Conversations + || kind == SelectionKind::Mixed; + switch (selectionTagPresence(QStringLiteral("unread"))) { case TagPresence::Every: action->setVisible(true); - action->setText(tr("Mark as &read")); - action->setStatusTip(tr("Remove the unread tag from the selection")); + action->setText(namesTheThread ? tr("Mark thread as &read") + : tr("Mark as &read")); + action->setStatusTip( + namesTheThread + ? tr("Remove the unread tag from every message of the " + "selected threads") + : tr("Remove the unread tag from the selection")); break; case TagPresence::None: action->setVisible(true); - action->setText(tr("Mark as &unread")); - action->setStatusTip(tr("Add the unread tag to the selection")); + action->setText(namesTheThread ? tr("Mark thread as &unread") + : tr("Mark as &unread")); + action->setStatusTip( + namesTheThread + ? tr("Add the unread tag to every message of the selected " + "threads") + : tr("Add the unread tag to the selection")); break; case TagPresence::Mixed: // No honest label exists, so there is no label to show. Hidden rather @@ -3736,6 +3719,118 @@ void MainWindow::refreshUnreadAction() } } +MainWindow::SelectionKind MainWindow::selectionKind() const +{ + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + if (rows.isEmpty()) + return SelectionKind::Empty; + + int conversations = 0; + for (const QModelIndex &index : rows) { + if (m_model->isConversationRow(index)) + ++conversations; + } + if (conversations == 0) + return SelectionKind::Messages; + return conversations == rows.size() ? SelectionKind::Conversations + : SelectionKind::Mixed; +} + +void MainWindow::refreshScopedActionLabels() +{ + // One pass, one selection, both jobs. Splitting the label from the + // visibility would let the two answer from different reads of the + // selection, and a hidden action wearing the wrong label is worse than + // either fault alone. + const SelectionKind kind = selectionKind(); + const bool conversation = kind == SelectionKind::Conversations; + + // Mixed counts as a conversation for the LABEL, because the write really + // will take whole threads for the rows that are conversations, and the + // wider claim is the honest one when the selection holds both. + const bool namesTheThread = conversation || kind == SelectionKind::Mixed; + + const auto relabel = [this](const QString &name, const QString &text, + const QString &tip) { + if (QAction *action = m_actions.value(name)) { + action->setText(text); + action->setStatusTip(tip); + } + }; + + if (namesTheThread) { + relabel(QStringLiteral("archive"), tr("&Archive thread"), + tr("Remove inbox from every message of the selected threads")); + relabel(QStringLiteral("delete"), tr("&Delete thread"), + tr("Move every message of the selected threads to the trash")); + relabel(QStringLiteral("restore"), tr("&Restore thread from trash"), + tr("Move every message of the selected threads out of the " + "trash")); + relabel(QStringLiteral("spam"), tr("Mark thread as &spam"), + tr("Add spam and remove inbox on the selected threads")); + relabel(QStringLiteral("flag"), tr("&Important thread"), + tr("Mark every message of the selected threads as important")); + } else { + relabel(QStringLiteral("archive"), tr("&Archive"), + tr("Remove the inbox tag")); + relabel(QStringLiteral("delete"), tr("&Delete"), + tr("Add or remove the deleted tag")); + relabel(QStringLiteral("restore"), tr("&Restore from trash"), + tr("Move the selected messages out of the trash")); + relabel(QStringLiteral("spam"), tr("Mark &spam"), + tr("Add spam and remove inbox")); + relabel(QStringLiteral("flag"), tr("&Important"), + tr("Add or remove the important tag")); + } + + // Delete and Archive are ABSENT on a reply, not disabled, at the user's + // decision: "I don't think I'd want to be able to remove a single reply + // from a thread". A thread of ONE is still a message row and keeps them, + // because there deleting the message and deleting the conversation are + // the same act. So the test is not "is this a message row" but "is this a + // reply", which only a message row can be. + bool anyReply = false; + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + for (const QModelIndex &index : rows) { + if (m_model->isMessageRow(index)) { + anyReply = true; + break; + } + } + + // Delete's and Restore's visibility is also refreshTrashActions()' job, + // which runs after this and would overwrite a hide made here. It is told + // about the reply case rather than asked to repeat the walk. + m_replySelectionHidesDelete = anyReply; + + if (QAction *archive = m_actions.value(QStringLiteral("archive"))) + archive->setVisible(!anyReply); + + // The mirror, on a conversation row: it shows no message, so the actions + // that need one cannot mean what they usually do. Reply survives as the + // thread's own, relabelled above; the other four go. + for (const QString &name : { QStringLiteral("reply_all"), + QStringLiteral("reply_no_quote"), + QStringLiteral("forward"), + QStringLiteral("save_message") }) { + if (QAction *action = m_actions.value(name)) + action->setVisible(!conversation); + } + + if (QAction *reply = m_actions.value(QStringLiteral("reply"))) { + if (conversation) { + reply->setText(tr("Reply to this &thread")); + reply->setStatusTip( + tr("Add an answer to the end of this conversation")); + } else { + reply->setText(tr("Re&ply")); + reply->setStatusTip(tr("Reply to the displayed message")); + } + } +} + void MainWindow::onSelectionChanged() { // Here rather than in the currentRowChanged handler: that signal is @@ -3743,6 +3838,7 @@ void MainWindow::onSelectionChanged() // selectedRows() there sees the PREVIOUS selection and would label the // action for the rows the user just left (CLAUDE.md, verified Qt 6.11). refreshUnreadAction(); + refreshScopedActionLabels(); refreshTrashActions(); const QModelIndexList rows = m_threadView->selectionModel()->selectedRows(); @@ -3753,7 +3849,7 @@ void MainWindow::onSelectionChanged() // a message row for one, and the keypress looks identical. Naming it // here is what this project does instead of a confirmation dialog, // which CLAUDE.md rules out for tag mutations. - const ActionScope scope = m_model->scopeFor(rows); + const ActionScope scope = m_model->scopeForSelection(rows); if (scope.wholeThread) { m_selectionMessage = @@ -3814,7 +3910,7 @@ void MainWindow::onSelectionChanged() // Reported per row kind rather than as a bare row count, so a mixed // selection says what it will really touch instead of calling three replies // "3 threads". - const ActionScope scope = m_model->scopeFor(rows); + const ActionScope scope = m_model->scopeForSelection(rows); if (!scope.threadIds.isEmpty() && scope.messageIds.isEmpty()) { m_selectionMessage = tr("%n thread(s) selected (%1 messages)", "", scope.threadIds.size()) @@ -5086,7 +5182,7 @@ QVector<PendingChange> MainWindow::pendingChangeSnapshot() const rows.append(PendingChange{ id, false, it->action, QString(), -1 }); } - // Held THREAD edits, which stay thread-scoped: a `*_thread` action is what + // Held THREAD edits, which stay thread-scoped: a CONVERSATION row is what // made them, and reporting the messages instead would claim the user acted // on each one. One row per thread the edit named, since a single edit can // cover a multi-row selection. @@ -5204,7 +5300,8 @@ void MainWindow::scheduleMarkRead(const QString &messageId, bool unread) return; // A row the model cannot name a message for. Marking its thread instead - // would be the escalation item 108 removed. + // would be a silent escalation: the automatic mark-read is about the + // message on display, never about the conversation around it. if (messageId.isEmpty()) return; @@ -5278,29 +5375,32 @@ QString MainWindow::currentThreadFirstMessageId() const return {}; } -bool MainWindow::everySelectedRowHasTag(const QString &tag, - TagScope scope) const +bool MainWindow::everySelectedRowHasTag(const QString &tag) const { // Kept as the direction question, which only has two answers to give: a // mixed selection has to go one way, and this says which. The LABEL asks // selectionTagPresence() instead, because a label can say "these disagree" // and a direction cannot. - return selectionTagPresence(tag, scope) == TagPresence::Every; + return selectionTagPresence(tag) == TagPresence::Every; } -MainWindow::TagPresence MainWindow::selectionTagPresence(const QString &tag, - TagScope scope) const +MainWindow::TagPresence MainWindow::selectionTagPresence( + const QString &tag) const { // What a toggle asks before choosing its direction, for both Delete and // Toggle unread. // - // Per ROW, and each row is asked about what it stands for: a reply row - // reports the message's tags, a thread row the thread's. Asking a reply's - // THREAD is the trap both toggles fell into. The write is message-scoped, - // so it never changes the thread's tags; the thread's answer therefore - // never moves however many times the key is pressed, and the toggle - // becomes one-way. On the second press it re-sends a tag the message - // already has, which is a no-op, and a no-op repaints nothing. + // Per ROW, and each row is asked about what it stands for. The same + // question ThreadListModel::scopeForSelection() answers for the WRITE, and + // it has to be the same question: a direction taken from one object while + // the write lands on another is how a toggle goes one-way. + // + // A reply row reports the message's tags. Asking a reply's THREAD is the + // trap both toggles fell into: the write is message-scoped, so it never + // changes the thread's tags; the thread's answer therefore never moves + // however many times the key is pressed. On the second press it re-sends a + // tag the message already has, which is a no-op, and a no-op repaints + // nothing. // // One direction for the WHOLE selection, which is the rule Delete // established: toggling each row independently would leave one keystroke @@ -5313,37 +5413,30 @@ MainWindow::TagPresence MainWindow::selectionTagPresence(const QString &tag, int withTag = 0; for (const QModelIndex &index : rows) { QStringList tags; - if (scope == TagScope::Thread) { + if (m_model->isConversationRow(index)) { + // The conversation's own union, which is what a conversation-scoped + // write is about to change. Reading one message here would make the + // toggle disagree with itself the moment the thread is mixed. tags = m_model->threadFor(index).tags; } else if (m_model->isMessageRow(index)) { tags = m_model->messageAt(index).tags; } else { - // 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. - // - // 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. + // A thread row that is NOT a conversation: a thread of one, whose + // union IS its message. Asked about that message anyway rather + // than about the summary, because the two can diverge in one + // direction that matters, described below. // - // 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. + // The union used to be read for EVERY thread row, with a comment + // calling the imprecision bounded because a tag toggle at worst + // re-applied a tag the message already had. That stopped being + // bounded when Delete became a MOVE: deleting the root of a + // three-message thread left the replies undeleted, so the union + // carried no `deleted`, so a second press read the row as + // not-deleted and deleted it AGAIN, trash-to-trash, ending with + // `deleted-from:inbox` and `deleted-from:Trash` at once and no way + // back. Item 177 removes the case rather than the symptom: a + // three-message row is a conversation now and is asked about its + // conversation, above. const ThreadSummary summary = m_model->threadFor(index); const MessageNode own = m_model->messageById(summary.firstMessageId); @@ -5370,25 +5463,6 @@ ThreadSummary MainWindow::threadForCurrentRowForTesting() const return m_model->threadFor(m_threadView->currentIndex()); } -QMenu *MainWindow::buildThreadActionsMenu(QWidget *parent) -{ - // Built per call rather than shared. A QMenu belongs to one place in one - // menu tree, and adding the same instance to both the menu bar and the - // context menu gives whichever added it last the object. The ACTIONS are - // shared, which is what has to stay consistent; the menu holding them is - // just a container. - auto *menu = new QMenu(tr("&Whole thread"), parent); - menu->setObjectName(QStringLiteral("threadActionsMenu")); - menu->addAction(m_actions.value(QStringLiteral("archive_thread"))); - menu->addAction(m_actions.value(QStringLiteral("delete_thread"))); - menu->addAction(m_actions.value(QStringLiteral("spam_thread"))); - menu->addSeparator(); - menu->addAction(m_actions.value(QStringLiteral("mark_thread_read"))); - menu->addAction(m_actions.value(QStringLiteral("mark_thread_unread"))); - menu->addAction(m_actions.value(QStringLiteral("flag_thread"))); - return menu; -} - QHash<QString, int> MainWindow::selectionTagCounts() const { // How many of the selected rows carry each tag, which is what tells a tag @@ -5439,7 +5513,7 @@ void MainWindow::editTagsOnSelection() } void MainWindow::tagSelected(const QStringList &add, const QStringList &remove, - const QString &description, TagScope tagScope) + const QString &description) { const QModelIndexList rows = m_threadView->selectionModel()->selectedRows(); @@ -5451,12 +5525,10 @@ void MainWindow::tagSelected(const QStringList &add, const QStringList &remove, // threadAt(index.row()) mapping silently acted on whichever thread sat at // that position in the list. // - // Message scope by default since item 108: a thread row displays one - // message, so acting on it acts on that message. Thread scope is what the - // "Whole thread" actions ask for explicitly. - const ActionScope scope = tagScope == TagScope::Thread - ? m_model->scopeFor(rows) - : m_model->messageScopeFor(rows); + // One resolution, per row, from what the row IS (item 177). There is no + // scope argument any more: a caller that could choose is what let one + // gesture mean two things and forced a second set of actions to exist. + const ActionScope scope = m_model->scopeForSelection(rows); if (scope.isEmpty()) return; @@ -5613,11 +5685,19 @@ void MainWindow::trashSelected() if (rows.isEmpty()) return; - // Message scope, exactly as tagSelected() uses by default: a thread row - // stands for the ONE message its card displays. Escalating to the thread - // would move a whole conversation into the trash because the user deleted - // one reply. - const ActionScope scope = m_model->messageScopeFor(rows); + // Resolved per row, like every other action since item 177. A conversation + // row deletes its conversation; a thread of one deletes its message. A + // REPLY row never reaches here at all, because Delete is hidden on one: + // the user's rule is that a single reply cannot be removed from a thread. + const ActionScope scope = m_model->scopeForSelection(rows); + + // Both halves are run, because a selection really can hold one of each and + // dropping either would silently delete less than the user asked for. They + // travel different routes: an unexpanded conversation's message ids and + // paths live only in the database, so the thread half is asynchronous. + if (!scope.threadIds.isEmpty()) + trashThreads(scope.threadIds); + if (scope.messageIds.isEmpty()) return; @@ -5625,7 +5705,7 @@ void MainWindow::trashSelected() for (const QString &messageId : scope.messageIds) pathById.insert(messageId, m_model->messageById(messageId).filePath); - trashMessages(scope.messageIds, pathById, scope.messageCount); + trashMessages(scope.messageIds, pathById, scope.messageIds.size()); } void MainWindow::trashMessages(const QStringList &messageIds, @@ -5725,34 +5805,8 @@ QString MainWindow::originTagFor(const QString &dbRelativeFolder) const return QStringLiteral("deleted-from:%1").arg(accountRelative); } -QStringList MainWindow::selectedThreadIds() const +void MainWindow::trashThreads(const QStringList &threadIds) { - // 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; @@ -5794,6 +5848,25 @@ void MainWindow::onThreadMessagesResolved(const QStringList &messageIds, const QStringList threadScope = m_pendingThreadScope; m_pendingThreadScope.clear(); + if (requestTag == QStringLiteral("reply_thread")) { + if (messageIds.isEmpty()) { + showTransientStatus(tr("That thread holds no message to answer")); + return; + } + // The NEWEST message, which resolveQuery() puts first: In-Reply-To and + // References then land the answer at the END of the conversation, and + // the recipients are the ones currently in it rather than whoever + // started it. + // + // ReplyAll and no quoting, at the user's decision. A conversation is + // multi-party by definition, so answering one participant of it is the + // unusual case and stays available on an individual message; and "we + // just add an answer to the thread", so there is nothing to quote. + requestMessageForCompose(messageIds.first(), + ComposeContext::Kind::ReplyAll, false); + return; + } + if (requestTag == QStringLiteral("empty_trash")) { confirmAndPurge(messageIds); return; @@ -5876,14 +5949,8 @@ void MainWindow::onThreadMessagesResolved(const QStringList &messageIds, .arg(tr("Undelete thread"))); } -void MainWindow::restoreSelectedThreads() +void MainWindow::untrashThreads(const QStringList &threadIds) { - const QModelIndexList rows = - m_threadView->selectionModel()->selectedRows(); - if (rows.isEmpty()) - return; - - const QStringList threadIds = selectedThreadIds(); if (threadIds.isEmpty()) return; @@ -6042,7 +6109,14 @@ void MainWindow::restoreSelectedFromTrash() if (rows.isEmpty()) return; - const ActionScope scope = m_model->messageScopeFor(rows); + const ActionScope scope = m_model->scopeForSelection(rows); + + // A conversation row restores its whole conversation, by the same rule + // that makes Delete conversation-scoped there: the two are inverses and + // must agree about what they act on. + if (!scope.threadIds.isEmpty()) + untrashThreads(scope.threadIds); + if (scope.messageIds.isEmpty()) return; @@ -6057,7 +6131,7 @@ void MainWindow::restoreSelectedFromTrash() // // 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 + // may be a moment behind. untrashThreads() already worked this // way; this is the same reasoning applied to the message-scoped path. m_pendingRestoreIds = scope.messageIds; QMetaObject::invokeMethod( @@ -6180,7 +6254,13 @@ void MainWindow::restoreSelected(bool fallbackToInbox) if (rows.isEmpty()) return; - const ActionScope scope = m_model->messageScopeFor(rows); + const ActionScope scope = m_model->scopeForSelection(rows); + + // The conversation half, for the same reason trashSelected() has one: this + // is the undelete direction of the same toggle. + if (!scope.threadIds.isEmpty()) + untrashThreads(scope.threadIds); + if (scope.messageIds.isEmpty()) return; @@ -6346,7 +6426,7 @@ void MainWindow::sendMove(const QStringList &messageIds, displayRemove.append(tag); } // A thread-scoped move already repainted its rows in - // trashSelectedThreads() / restoreSelectedThreads(), synchronously, before + // trashThreads() / untrashThreads(), 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 diff --git a/src/mainwindow.h b/src/mainwindow.h index 6321573..fa413fe 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -112,7 +112,7 @@ public: /// this is testable with no worker and no database. /// /// Scope follows the ACTION. The three queues already encode it: a held - /// thread edit carries thread ids because a `*_thread` action made it, + /// thread edit carries thread ids because a CONVERSATION row made it, /// while a netted tag edit and a held move both carry message ids. Nothing /// is expanded, and nothing is escalated. /// Net changes the index holds that a sync has not carried over. @@ -768,11 +768,23 @@ private: /// reply from reply-without-quoting, which are the same kind with and /// without a seeded body. /// - /// Resolves through ThreadListModel::messageScopeFor(), NOT threadFor(): a - /// thread row means the one message its card shows. Replying to a thread - /// is meaningless, a reply answers a message. + /// Resolved per ROW since item 177. A message row, whether a reply or a + /// thread of one, seeds the composer from that message exactly as before. + /// A CONVERSATION row has no one message to answer, so `reply` there means + /// "reply to this thread": reply-all, quoting nothing, threaded off the + /// conversation's NEWEST message so the answer lands at its end. The other + /// four compose actions are hidden on such a row rather than redefined. void composeReply(ComposeContext::Kind kind, bool quote); + /// Starts the conversation reply: asks the worker for the thread's + /// messages, and finishes in onThreadMessagesResolved(). + /// + /// A round trip because the newest message's id is not in the model. A + /// thread the user never expanded holds no nodes for its replies at all, + /// and the summary carries only the FIRST message, which is the one a + /// reply must not answer. + void replyToThread(const QString &threadId); + /// Asks the worker for \p messageId's current file, then opens a composer. /// /// The round trip is the point. The context is built from the DATABASE and @@ -835,20 +847,16 @@ private: const QString &description, const std::function<void()> &handler); - /// What a tag action acts on. + /// Applies a tag change to whatever the selection stands for. /// - /// Since item 108 a thread ROW means the one message its card displays, so - /// Message is the default and Thread is the explicit choice the user makes - /// through the "Whole thread" submenu. Before that there was no choice: - /// a thread row always meant the conversation. - enum class TagScope { - Message, ///< The message each selected row displays. - Thread, ///< Every message of each selected row's thread. - }; - + /// There is no scope parameter, and that is item 177: the ROW decides. + /// A conversation row means its conversation, any other row means its one + /// message, and ThreadListModel::scopeForSelection() is the only place + /// that answer is worked out. A caller that could choose is how the same + /// gesture came to mean two things, which is what the deleted "Whole + /// thread" submenu existed to disambiguate. void tagSelected(const QStringList &add, const QStringList &remove, - const QString &description, - TagScope scope = TagScope::Message); + const QString &description); /// Starts, restarts or cancels the mark-read timer for a newly opened /// MESSAGE. Cancels outright for one that is not unread, so an already read @@ -921,32 +929,18 @@ private: void updateSyncControls(); - /// Opens the tag dialog on the current selection and applies its result. - /// - /// The only route to an arbitrary tag: every other tag action writes a - /// hardcoded name. - /// The "Whole thread" submenu, built fresh for each parent that needs one. - /// - /// A QMenu lives in one menu tree, so the menu bar and the context menu get - /// their own instance. The actions inside are shared, which is what has to - /// stay consistent between them. - QMenu *buildThreadActionsMenu(QWidget *parent); - /// Per-tag counts across the selected rows, for the tag dialog. QHash<QString, int> selectionTagCounts() const; /// True when every selected row already carries \p tag, which is what a /// toggle asks before choosing its direction. /// - /// Under Message scope each row answers about what it STANDS FOR: a reply - /// row about its message, a thread row about the message its card - /// displays. Asking a reply's thread makes a toggle one-way, since the - /// message-scoped write never changes the thread's tags. - /// - /// Under Thread scope a row answers about its whole thread, so the - /// question matches the write the thread actions are about to make. - bool everySelectedRowHasTag(const QString &tag, - TagScope scope = TagScope::Message) const; + /// Each row answers about what it STANDS FOR, which is the same question + /// the write asks: a conversation row about its whole thread, any other + /// row about its one message. Asking a reply's thread makes a toggle + /// one-way, since the message-scoped write never changes the thread's + /// tags and the answer therefore never moves. + bool everySelectedRowHasTag(const QString &tag) const; /// The three-valued version of the question above, which is what a LABEL /// needs and a toggle's direction does not. @@ -958,13 +952,37 @@ private: /// is what happens when a two-valued predicate is asked a three-valued /// question. enum class TagPresence { None, Every, Mixed }; - TagPresence selectionTagPresence( - const QString &tag, TagScope scope = TagScope::Message) const; + TagPresence selectionTagPresence(const QString &tag) const; /// Relabels the unread action, and hides it when the selection has no /// single state. Called whenever the selection changes. void refreshUnreadAction(); + /// Makes every scoped action say what it will act on, and hides the ones + /// that cannot mean anything on the selected row (item 177). + /// + /// Two separate jobs, deliberately in one pass over one selection so the + /// label and the visibility cannot disagree. Delete and Archive name the + /// thread on a conversation row and are ABSENT on a reply, per the user: + /// a single reply cannot be removed from a conversation. Forward and Save + /// are the mirror, absent on a conversation row, which shows no message + /// to forward and names no file to write. + void refreshScopedActionLabels(); + + /// Whether every selected row is a conversation, a message, or neither + /// because the selection mixes them or is empty. + enum class SelectionKind { Empty, Conversations, Messages, Mixed }; + SelectionKind selectionKind() const; + + /// Whether the selection holds a reply row, which is what hides Delete, + /// Restore and Archive (item 177). + /// + /// Written by refreshScopedActionLabels() and read by + /// refreshTrashActions(), which runs after it and owns the same two + /// actions' visibility. A flag rather than a second walk over the + /// selection, so the two cannot answer differently. + bool m_replySelectionHidesDelete = false; + /// Hides Delete on mail already in the trash, and Restore on mail that /// was never there (item 168). Each is offered only where it means /// something, the same rule refreshUnreadAction() applies to the label. @@ -979,6 +997,10 @@ private: /// Delete on exactly the mail a trash view is full of. bool everySelectedRowIsInATrashFolder() const; + /// Opens the tag dialog on the current selection and applies its result. + /// + /// The only route to an arbitrary tag: every other tag action writes a + /// hardcoded name. void editTagsOnSelection(); /// Set once the user has answered the exit prompt, or once a sync started @@ -1052,21 +1074,20 @@ private: int messageCount, const QStringList &wholeThreadIds = {}); - /// Moves every message of each selected THREAD to its account's trash. + /// Moves every message of the named THREADS to their accounts' 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; + /// + /// Takes ids rather than reading the selection, because since item 177 the + /// conversation half of a delete is one branch of trashSelected() rather + /// than a separate action the user could pick. + void trashThreads(const QStringList &threadIds); - /// The inverse of trashSelectedThreads(): moves every message of each - /// selected thread back where it came from. - void restoreSelectedThreads(); + /// The inverse of trashThreads(): moves every message of the named threads + /// back where it came from. + void untrashThreads(const QStringList &threadIds); /// Runs the thread-scoped delete once the worker has resolved the /// threads to messages. diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 1f28973..9fb3e4d 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -1326,6 +1326,13 @@ void NotmuchWorker::resolveQuery(const QString &query, return; } + // Stated rather than inherited. notmuch's default already is newest-first, + // and the move and restore callers do not care about the order at all, but + // "reply to this thread" reads the FIRST id as the conversation's newest + // message: an unstated default is not something a reply's threading + // headers should rest on. + notmuch_query_set_sort(nmQuery.get(), NOTMUCH_SORT_NEWEST_FIRST); + notmuch_messages_t *raw = nullptr; if (notmuch_query_search_messages(nmQuery.get(), &raw) != NOTMUCH_STATUS_SUCCESS) { diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index ddddf0b..fd4899b 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -1050,73 +1050,6 @@ MessageNode ThreadListModel::messageById(const QString &messageId) const return {}; } -ActionScope ThreadListModel::messageScopeFor( - const QModelIndexList &selection) const -{ - ActionScope scope; - - for (const QModelIndex &index : selection) { - QString messageId; - if (isMessageRow(index)) { - messageId = messageAt(index).messageId; - } else { - if (index.row() < 0 || index.row() >= m_threads.size()) - continue; - // The message the CARD displays, which the query already named. - // Not the loaded children: a thread the user never expanded still - // shows its first message, and this must work without one. - messageId = m_threads.at(index.row()).summary.firstMessageId; - } - - // Skipped rather than widened. Falling back to the thread here would - // silently act on messages the row does not display, which is the - // behaviour item 108 removed. - if (messageId.isEmpty() || scope.messageIds.contains(messageId)) - continue; - - scope.messageIds.append(messageId); - scope.messageCount += 1; - } - - return scope; -} - -ActionScope ThreadListModel::scopeFor(const QModelIndexList &selection) const -{ - ActionScope scope; - - for (const QModelIndex &index : selection) { - if (isMessageRow(index)) { - const MessageNode node = messageAt(index); - if (node.messageId.isEmpty() - || scope.messageIds.contains(node.messageId)) - continue; - scope.messageIds.append(node.messageId); - scope.messageCount += 1; - continue; - } - - if (index.row() < 0 || index.row() >= m_threads.size()) - continue; - - const ThreadSummary &summary = m_threads.at(index.row()).summary; - if (scope.threadIds.contains(summary.threadId)) - continue; - - scope.threadIds.append(summary.threadId); - - // totalCount, not the loaded children: a thread that was never expanded - // still has all of its messages, and counting only what happens to be - // on screen would understate what the action does. Floored at 1, since - // a summary with no count still stands for at least the message that - // produced it. - scope.messageCount += qMax(1, summary.totalCount); - scope.wholeThread = true; - } - - return scope; -} - ActionScope ThreadListModel::scopeForSelection( const QModelIndexList &selection) const { diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index fcbb8f5..a528c7e 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -333,40 +333,13 @@ public: /// expanded thread holds it. MessageNode messageById(const QString &messageId) const; - /// Resolves a selection into whole THREADS, for the thread-scoped actions. - /// - /// A thread row contributes its thread; a message row still contributes - /// only itself, since a reply's own row cannot be widened into its - /// conversation without escalating silently. Mixed selections are honoured - /// as given: a thread root and an unrelated reply act on that whole thread - /// and that one message. - /// - /// **Not the default any more.** Since item 108 the ordinary actions use - /// messageScopeFor(); this is what the explicit "whole thread" submenu - /// resolves through. - ActionScope scopeFor(const QModelIndexList &selection) const; - - /// Resolves a selection into individual MESSAGES, which is what the - /// ordinary tag actions act on since item 108. - /// - /// A thread row contributes the ONE message its card displays, not its - /// whole conversation. That is `ThreadSummary::firstMessageId`, carried - /// from the query, so this needs no expansion and no worker round trip. - /// In the Sent view that field is the first MATCHED message rather than - /// the thread's opening one, which is right here for the same reason it is - /// right on the card: both answer "the message this row shows". - /// - /// A thread row whose `firstMessageId` is empty contributes nothing. That - /// is a row the model cannot name a message for, and acting on the whole - /// thread instead would be the silent escalation this exists to remove. - ActionScope messageScopeFor(const QModelIndexList &selection) const; - /// What a selection means, resolved per row from what that row IS. /// - /// Replaces the scopeFor()/messageScopeFor() pair, which made the caller - /// choose the scope and so let one gesture mean two things (item 177). A - /// conversation row contributes its thread, any other row its message, and - /// a mixed selection carries both. + /// The one answer to the question, and the only one (item 177). It + /// replaced a scopeFor()/messageScopeFor() pair that made the CALLER + /// choose, which let one gesture mean two things and needed a second set + /// of actions to disambiguate. A conversation row contributes its thread, + /// any other row its message, and a mixed selection carries both. ActionScope scopeForSelection(const QModelIndexList &selection) const; /// The account keys behind a thread's account tags, for item 49's diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 4ed62f9..8fe1f87 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -383,7 +383,6 @@ private slots: void escapeBlanksTheMessagePane(); void deleteTogglesOnAnAlreadyDeletedThread(); void deleteOnAMixedSelectionDeletesRatherThanSplittingIt(); - void deleteOnAReplyReadsItsOwnThreadNotTheFirstInTheList(); void toggleUnreadOnAReplyReadsItsOwnThreadNotTheFirstInTheList(); void importantOnAnAlreadyImportantThreadRemovesTheTag(); void importantOnAPlainThreadStillAddsTheTag(); @@ -398,15 +397,13 @@ private slots: void theUnreadLabelSaysWhichDirectionItWillGo(); void theUnreadLabelFollowsAWriteWithoutReselecting(); void theUnreadActionIsHiddenOnAMixedSelection(); - void markThreadUnreadReachesAMixedThread(); - void markThreadReadAndUnreadAreSeparateActions(); + void aMixedThreadIsMarkedReadAndEditTagsIsTheWayBack(); void toggleUnreadOnAReplyReadsTheReplysOwnState(); void toggleUnreadOnAReplyRepaintsItInBothDirections(); void taggingTheOpenReplyUpdatesTheMessagePaneStrip(); void taggingAnUnrelatedReplyLeavesTheStripAlone(); void aHeldMessageEditIsSentWhenTheSyncEnds(); - void anActionOnAThreadRowActsOnTheMessageItDisplays(); - void theThreadSubmenuIsReachableFromBothMenus(); + void anActionOnAConversationRowTakesTheConversation(); void autoMarkReadTouchesOnlyTheMessageOnDisplay(); void autoMarkReadArmsForAReplyToo(); void taggingTheOpenRootMessageKeepsTheStripPopulated(); @@ -481,7 +478,7 @@ private slots: void twoDeletesToOneTrashBothGetTheirTags(); void deletingTwiceLeavesNoOriginTagBehind(); void undoOfADeleteRemovesTheOriginTagToo(); - void deletingAThreadRootRemovesItFromTheInboxAndUndoReturnsIt(); + void deletingALoneMessageRemovesItFromTheInboxAndUndoReturnsIt(); void deleteThreadMovesEveryMessageAndRepaintsTheRootCard(); void aFolderNameWithASpaceSurvivesTheRoundTrip(); void deleteIsBoundToTheDeleteKey(); @@ -548,6 +545,15 @@ private slots: void aSmallSizeLimitIsNotDescribedAsZeroMegabytes(); void theBusinessSenderListIsLoadedAtStartup(); + // Item 177, task 5: the scope comes from the row, and the labels say so. + void theUnreadActionNamesTheThreadOnAConversationRow(); + void deleteIsAbsentOnAReplyRow(); + void theWholeThreadSubmenuIsGone(); + void forwardAndSaveAreAbsentOnAConversationRow(); + void replyOnAConversationRowNamesTheThread(); + void replyToAConversationAnswersItsNewestMessage(); + void replyIsUntouchedOnAMessageRow(); + private: /// Owns the throwaway lock table init() points every test at. A pointer /// rather than a value because it is rebuilt per test, and QTemporaryDir @@ -1585,11 +1591,12 @@ void TestMainWindow::anActionOnAThreadRowSaysItHitTheWholeThread() selectThreadRow(view, 0); QApplication::processEvents(); - // The THREAD action since item 108. The plain `archive` now acts on the - // one message a card displays, and would rightly not claim to have taken - // the whole thread; this suffix belongs to the action that really does. - auto *archive = window.findChild<QAction *>(QStringLiteral("archive_thread")); - QVERIFY2(archive, "no archive_thread action to trigger"); + // The plain `archive`, on a CONVERSATION row. Item 108's separate + // `archive_thread` is gone: since item 177 the row's identity is what + // makes this thread-scoped, and the fixture's totalCount of 7 is what + // makes the row a conversation. + auto *archive = window.findChild<QAction *>(QStringLiteral("archive")); + QVERIFY2(archive, "no archive action to trigger"); archive->trigger(); // Read BEFORE processEvents, deliberately. This binary has no worker @@ -4822,14 +4829,16 @@ void TestMainWindow::deleteTogglesOnAnAlreadyDeletedThread() QVERIFY(model); auto *view = window.findChild<QTreeView *>(); QVERIFY(view); - // The THREAD action, since this is about a THREAD's state. Item 108 made - // the plain `delete` act on the one message a card displays, and a thread - // summary carrying `deleted` says nothing about that message's own tags. - auto *action = window.findChild<QAction *>(QStringLiteral("delete_thread")); + // `delete` on a CONVERSATION row, which is the thread-scoped delete since + // item 177: the row's identity is what decides the scope, so a summary + // carrying `deleted` is the right thing to read here. + auto *action = window.findChild<QAction *>(QStringLiteral("delete")); QVERIFY(action); - model->appendBatch({ makeThread(QStringLiteral("t1"), - { QStringLiteral("deleted") }) }); + ThreadSummary deleted = makeThread(QStringLiteral("t1"), + { QStringLiteral("deleted") }); + deleted.totalCount = 3; + model->appendBatch({ deleted }); selectThreadRow(view, 0); action->trigger(); @@ -4853,12 +4862,17 @@ void TestMainWindow::deleteOnAMixedSelectionDeletesRatherThanSplittingIt() QVERIFY(model); auto *view = window.findChild<QTreeView *>(); QVERIFY(view); - auto *action = window.findChild<QAction *>(QStringLiteral("delete_thread")); + auto *action = window.findChild<QAction *>(QStringLiteral("delete")); QVERIFY(action); - model->appendBatch({ makeThread(QStringLiteral("t1"), - { QStringLiteral("deleted") }), - makeThread(QStringLiteral("t2"), {}) }); + // Conversations, so `delete` is thread-scoped on both and the two rows + // really are in opposite THREAD states. + ThreadSummary first = makeThread(QStringLiteral("t1"), + { QStringLiteral("deleted") }); + first.totalCount = 2; + ThreadSummary second = makeThread(QStringLiteral("t2"), {}); + second.totalCount = 2; + model->appendBatch({ first, second }); view->selectAll(); QCOMPARE(view->selectionModel()->selectedRows().size(), 2); @@ -4927,59 +4941,6 @@ static QModelIndex expandSecondThreadAndSelectItsReply( return replyRow; } -void TestMainWindow::deleteOnAReplyReadsItsOwnThreadNotTheFirstInTheList() -{ - // Item 88's trap, still live: a toggle must read the state of the row it - // is on, not of whichever thread sits at that row NUMBER in the list. - // - // Through `delete_thread` rather than `delete`. Since item 103 Delete - // MOVES the file, so it is no longer a pure toggle over a tag and needs a - // configured trash folder and a worker; `delete_thread` is the variant - // that stayed tag-only, and it is a toggle over `deleted` exactly as - // Delete used to be. The message-scoped Delete's own direction choice is - // covered by the worker-backed cases at the bottom of this file, which is - // where a move can actually be observed. - const Config config; - MainWindow window(config); - - auto *model = window.findChild<ThreadListModel *>(); - QVERIFY(model); - auto *view = window.findChild<QTreeView *>(); - QVERIFY(view); - auto *action = window.findChild<QAction *>(QStringLiteral("delete_thread")); - QVERIFY(action); - - // t1 deleted, t2 not. Reading t1's state for a reply of t2 makes the - // toggle choose UNDELETE for a thread that was never deleted. - const QModelIndex reply = expandSecondThreadAndSelectItsReply( - view, model, { QStringLiteral("deleted") }, {}); - QVERIFY2(reply.isValid(), - "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(); - - // 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() { const Config config; @@ -5557,20 +5518,25 @@ void TestMainWindow::theUnreadActionIsHiddenOnAMixedSelection() "the action did not return when the selection agreed again"); } -void TestMainWindow::markThreadUnreadReachesAMixedThread() +void TestMainWindow::aMixedThreadIsMarkedReadAndEditTagsIsTheWayBack() { - // Item 112. The user's report: on a thread with two unread replies, asking - // to mark the whole thread unread marked it READ instead. + // What item 112 became under item 177. That item's report was real: on a + // thread with two unread replies, asking to mark the whole thread unread + // marked it READ, because ThreadSummary::tags is notmuch's UNION and a + // thread holding even one unread message answers "unread". A union is not + // a state, and a toggle needs a state. // - // ThreadSummary::tags is notmuch's UNION over the conversation, so a - // thread containing even one unread message answers "unread" and a toggle - // reading that predicate always picks "mark read". There was no input that - // could reach "mark thread unread" on a mixed thread: the only threads - // taking that branch were the ones already entirely read. + // Its fix was two fixed-direction thread actions in a submenu. Item 177 + // deleted that submenu: the ROW decides the scope, so a second set of + // actions was a second answer to a settled question. The cost is recorded + // here rather than hidden. On a mixed conversation the toggle still goes + // ONE way, and that way is "mark read", which is the safe direction: it + // takes the thread to a state it can then be toggled out of, where the + // reverse would have left it mixed and the key still dead. // - // A union is not a state. The fix is two fixed-direction actions, so this - // asserts the direction rather than the resulting tags: on a mixed thread - // BOTH directions are reachable, which is the property that was missing. + // The way back is Edit tags, which is absolute rather than a toggle and + // works whatever the mix. That is what the deleted submenu was really + // providing, and it did not need six actions to provide it. const Config config; MainWindow window(config); @@ -5584,61 +5550,62 @@ void TestMainWindow::markThreadUnreadReachesAMixedThread() // identically whichever way the direction is computed, so a uniform // fixture passes against the bug (CLAUDE.md, item 88's opposite-states // requirement). - model->appendBatch({ makeThread(QStringLiteral("T1"), - { QStringLiteral("unread") }) }); + ThreadSummary mixed = makeThread(QStringLiteral("T1"), + { QStringLiteral("unread") }); + mixed.totalCount = 3; + model->appendBatch({ mixed }); + const QModelIndex thread = model->index(0, 0, {}); QVERIFY(thread.isValid()); + QVERIFY2(model->isConversationRow(thread), + "the fixture's row is not a conversation, so the toggle would " + "read one message and this test would assert nothing about the " + "union"); QVERIFY2(model->threadFor(thread).isUnread(), "the fixture's union does not carry unread, so this test cannot " "reach the branch the defect lives in"); - view->setCurrentIndex(thread); - auto *markUnread = - window.findChild<QAction *>(QStringLiteral("mark_thread_unread")); - QVERIFY2(markUnread, "mark_thread_unread does not exist: the thread toggle " - "was not split, so a mixed thread still has no way to " - "be marked unread"); - markUnread->trigger(); - - QVERIFY2(window.undoTextForTesting().contains(QStringLiteral("unread")), - qPrintable(QStringLiteral("wrong direction on a mixed thread: %1") - .arg(window.undoTextForTesting()))); - QVERIFY2(!window.undoTextForTesting().contains(QStringLiteral("Mark thread read")), - qPrintable(QStringLiteral("marked the thread READ when asked to " - "mark it unread: %1") - .arg(window.undoTextForTesting()))); -} + view->setCurrentIndex(thread); + view->selectionModel()->select( + thread, QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + QApplication::processEvents(); -void TestMainWindow::markThreadReadAndUnreadAreSeparateActions() -{ - // The other half: the read direction must still be reachable, and must be - // its own action rather than the same one answering differently. Both are - // asserted on the SAME mixed thread, which a toggle cannot do: whichever - // direction it picks, the other is unreachable there. - const Config config; - MainWindow window(config); + auto *toggle = window.findChild<QAction *>(QStringLiteral("toggle_unread")); + QVERIFY(toggle); - auto *model = window.findChild<ThreadListModel *>(); - QVERIFY(model); - auto *view = window.findChild<QTreeView *>(); - QVERIFY(view); + // The LABEL is the promise, and it must name the thread and the direction + // before the key is pressed. A label saying only "Mark as read" on a row + // that is about to touch three messages is the ambiguity item 177 exists + // to remove. + QVERIFY2(toggle->text().contains(QStringLiteral("thread"), + Qt::CaseInsensitive), + qPrintable(QStringLiteral("the label does not name the thread: %1") + .arg(toggle->text()))); + QVERIFY2(toggle->text().contains(QStringLiteral("read"), Qt::CaseInsensitive) + && !toggle->text().contains(QStringLiteral("unread"), + Qt::CaseInsensitive), + qPrintable(QStringLiteral("the label does not promise the read " + "direction: %1").arg(toggle->text()))); + + toggle->trigger(); - model->appendBatch({ makeThread(QStringLiteral("T1"), - { QStringLiteral("unread") }) }); - const QModelIndex thread = model->index(0, 0, {}); - view->setCurrentIndex(thread); + QVERIFY2(window.undoTextForTesting().contains(QStringLiteral("Mark read")), + qPrintable(QStringLiteral("the toggle went the other way on a " + "mixed thread: %1") + .arg(window.undoTextForTesting()))); - auto *markRead = - window.findChild<QAction *>(QStringLiteral("mark_thread_read")); - QVERIFY(markRead); - markRead->trigger(); - QVERIFY2(window.undoTextForTesting().contains(QStringLiteral("Mark thread read")), - qPrintable(window.undoTextForTesting())); + // And it was THREAD-scoped, which is the whole of item 177: one keystroke + // on a conversation row took the conversation, not the one message its + // card shows. + QVERIFY2(!model->threadAt(0).isUnread(), + "the thread's own tags did not move, so the write was scoped to " + "one message and the other two are still unread"); - // The old toggle must be gone rather than left beside its replacements, - // which would leave the defect reachable from the menu it still sat in. - QVERIFY2(!window.findChild<QAction *>(QStringLiteral("toggle_unread_thread")), - "toggle_unread_thread still exists beside the split actions"); + // The route back exists and is not the toggle. + auto *editTags = window.findChild<QAction *>(QStringLiteral("edit_tags")); + QVERIFY2(editTags && editTags->isEnabled(), + "Edit tags is the absolute route the deleted submenu used to " + "provide, and it is not available"); } void TestMainWindow::toggleUnreadOnAReplyReadsTheReplysOwnState() @@ -5910,16 +5877,19 @@ void TestMainWindow::aHeldMessageEditIsSentWhenTheSyncEnds() "sending the held edit lost the tag from the reply's row"); } -void TestMainWindow::anActionOnAThreadRowActsOnTheMessageItDisplays() +void TestMainWindow::anActionOnAConversationRowTakesTheConversation() { + // The inversion item 177 is. Item 108 made a thread row act on the ONE + // message its card displays, and this test asserted exactly that; the user + // then reported it as the defect, because a card that stands above a + // conversation and acts on one message of it is two things at once. A row + // with replies is now the conversation, and a row without them is still + // its message. + // // `spam`, not `delete`. Since item 103 Delete MOVES the file, so it needs // an account with a configured trash folder and a worker to do the move; - // this bare window has neither, and Delete correctly refuses. What is - // under test here is unchanged by that: `spam` is the other message-scoped - // tag-only action, and it paints the same doomed state. - // Item 108, the whole point of it. A root card renders ONE message since - // item 66, so acting on it acts on that message; the conversation is - // reached through the explicit thread actions. + // this bare window has neither. `spam` is the other tag-only action and + // resolves its scope through the same tagSelected(). const Config config; MainWindow window(config); @@ -5928,99 +5898,35 @@ void TestMainWindow::anActionOnAThreadRowActsOnTheMessageItDisplays() auto *view = window.findChild<QTreeView *>(); QVERIFY(view); - ThreadSummary t = makeThread(QStringLiteral("t1"), {}); - t.totalCount = 7; - model->appendBatch({ t }); - selectThreadRow(view, 0); - - auto *deleteAction = window.findChild<QAction *>(QStringLiteral("spam")); - QVERIFY(deleteAction); - deleteAction->trigger(); - - QCOMPARE(window.pendingMessageIdsForTesting(), - QStringList{ QStringLiteral("t1-first@example.org") }); - QVERIFY2(window.pendingThreadIdsForTesting().isEmpty(), - "the ordinary Delete still acted on the whole thread, so it " - "touched six messages the card does not display"); - - // 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<QAction *>(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(); - - QVERIFY2(model->threadAt(0).isDeleted(), - "Delete thread did not mark the whole thread, so the card paints " - "undeleted until the row is clicked"); -} + // A conversation FIRST and a thread of one SECOND, so a wrong answer is + // visible in both directions rather than accidentally right in one. + ThreadSummary many = makeThread(QStringLiteral("t1"), {}); + many.totalCount = 7; + ThreadSummary one = makeThread(QStringLiteral("t2"), {}); + one.totalCount = 1; + model->appendBatch({ many, one }); -void TestMainWindow::theThreadSubmenuIsReachableFromBothMenus() -{ - // The user asked for "a submenu when right clicking and the same submenu - // under Message in the top menu". Both, not one: the context menu is where - // the gesture starts and the menu bar is where a shortcut is discovered. - // - // A QMenu belongs to ONE menu tree, so these are two instances holding the - // same actions. Adding a single instance to both silently gives it to - // whichever added it last, which is the failure this pins. - const Config config; - MainWindow window(config); + auto *spam = window.findChild<QAction *>(QStringLiteral("spam")); + QVERIFY(spam); - auto *context = - window.findChild<QMenu *>(QStringLiteral("threadContextMenu")); - QVERIFY(context); + selectThreadRow(view, 0); + QApplication::processEvents(); + spam->trigger(); - const QStringList expected = { - QStringLiteral("archive_thread"), - QStringLiteral("delete_thread"), - QStringLiteral("spam_thread"), - QStringLiteral("mark_thread_read"), - QStringLiteral("mark_thread_unread"), - QStringLiteral("flag_thread"), - }; + QCOMPARE(window.pendingThreadIdsForTesting(), + QStringList{ QStringLiteral("t1") }); + QVERIFY2(window.pendingMessageIdsForTesting().isEmpty(), + "a conversation row acted on one message, so six of the seven " + "messages the card stands above were left untouched"); - // Every submenu instance in the window, wherever it was added. - const QList<QMenu *> submenus = - window.findChildren<QMenu *>(QStringLiteral("threadActionsMenu")); - QVERIFY2(submenus.size() >= 2, - qPrintable(QStringLiteral("expected the thread submenu in both " - "the context menu and the menu bar, " - "found %1 instance(s)") - .arg(submenus.size()))); - - for (QMenu *menu : submenus) { - QStringList names; - for (QAction *action : menu->actions()) { - if (!action->isSeparator()) - names.append(action->objectName()); - } - QCOMPARE(names, expected); - } + // And the other half of the rule, which is what makes it a rule rather + // than a blanket escalation: a thread of one is still its message. + selectThreadRow(view, 1); + QApplication::processEvents(); + spam->trigger(); - // One of them is the context menu's own, reached as a submenu rather than - // as a loose action. - bool inContextMenu = false; - for (QAction *action : context->actions()) { - if (action->menu() - && action->menu()->objectName() - == QStringLiteral("threadActionsMenu")) { - inContextMenu = true; - break; - } - } - QVERIFY2(inContextMenu, - "right-clicking a thread offers no whole-thread submenu"); + QCOMPARE(window.pendingMessageIdsForTesting(), + QStringList{ QStringLiteral("t2-first@example.org") }); } void TestMainWindow::autoMarkReadTouchesOnlyTheMessageOnDisplay() @@ -6483,8 +6389,14 @@ void TestMainWindow::theSnapshotKeepsAThreadActionThreadScoped() auto *model = window.findChild<ThreadListModel *>(); auto *view = window.findChild<QTreeView *>(); QVERIFY(model && view); - model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); + // A CONVERSATION row, which is what makes `flag` thread-scoped since item + // 177. A thread of one would produce a message edit and this test would + // assert nothing about thread ids. + ThreadSummary many = makeThread(QStringLiteral("t1"), {}); + many.totalCount = 3; + model->appendBatch({ many }); selectThreadRow(view, 0); + QApplication::processEvents(); // A cron sync takes the lock, which is what makes the edit HELD rather // than sent, and a held edit is the only thing that carries thread ids. @@ -6492,7 +6404,7 @@ void TestMainWindow::theSnapshotKeepsAThreadActionThreadScoped() Q_ARG(SyncMonitor::State, SyncMonitor::State::Running)); - auto *action = window.findChild<QAction *>(QStringLiteral("flag_thread")); + auto *action = window.findChild<QAction *>(QStringLiteral("flag")); QVERIFY(action); action->trigger(); QVERIFY(window.hasEditAwaitingSend()); @@ -6572,14 +6484,18 @@ void TestMainWindow::anEditDuringABackgroundSyncIsNotSentYet() QVERIFY(model); auto *view = window.findChild<QTreeView *>(); QVERIFY(view); - // The THREAD action: this test asserts on the thread ROW, which a - // message-scoped write deliberately leaves alone since item 108. What - // is under test is the HOLD, which is identical either way. - auto *action = window.findChild<QAction *>(QStringLiteral("flag_thread")); + // A CONVERSATION row, which is what makes `flag` thread-scoped since item + // 177. This test asserts on the thread ROW, which a message-scoped write + // deliberately leaves alone. What is under test is the HOLD, which is + // identical either way. + auto *action = window.findChild<QAction *>(QStringLiteral("flag")); QVERIFY2(action, "no flag action registered"); - model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); + ThreadSummary many = makeThread(QStringLiteral("t1"), {}); + many.totalCount = 3; + model->appendBatch({ many }); selectThreadRow(view, 0); + QApplication::processEvents(); // A cron sync takes the lock. QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", @@ -6606,14 +6522,18 @@ void TestMainWindow::aHeldEditIsSentWhenTheBackgroundSyncEnds() QVERIFY(model); auto *view = window.findChild<QTreeView *>(); QVERIFY(view); - // The THREAD action: this test asserts on the thread ROW, which a - // message-scoped write deliberately leaves alone since item 108. What - // is under test is the HOLD, which is identical either way. - auto *action = window.findChild<QAction *>(QStringLiteral("flag_thread")); + // A CONVERSATION row, which is what makes `flag` thread-scoped since item + // 177. This test asserts on the thread ROW, which a message-scoped write + // deliberately leaves alone. What is under test is the HOLD, which is + // identical either way. + auto *action = window.findChild<QAction *>(QStringLiteral("flag")); QVERIFY(action); - model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); + ThreadSummary many = makeThread(QStringLiteral("t1"), {}); + many.totalCount = 3; + model->appendBatch({ many }); selectThreadRow(view, 0); + QApplication::processEvents(); QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, @@ -7981,28 +7901,19 @@ void TestMainWindow::noTwoActionsShareAnIcon() // theme are just as ambiguous on screen, and that is what the user sees. // Narrowed by item 108 to the actions that can reach the TOOLBAR, which is // where the rule comes from: an icon-only toolbar makes the icon the whole - // control. The five whole-thread actions live only in the "Whole thread" - // submenu, whose entries always carry text, and each deliberately shares - // the icon of its message-scoped twin: same operation, wider scope, with - // the words saying which. Giving them five invented shapes would be less - // clear than the pairing. - // - // reply_no_quote joined them in item 123 for exactly the same reason: it - // shares reply's icon, it is a menu entry that always carries its text, - // and it is not on the toolbar. The list is therefore no longer only the - // thread tier, which is why it is named for the PROPERTY that earns the - // exemption rather than for the tier that first needed it. + // control. The exemption was six whole-thread actions sharing their + // message-scoped twins' icons; item 177 deleted those six, so the list is + // down to the one entry that earns it on its own terms. + // + // reply_no_quote shares reply's icon, is a menu entry that always carries + // its text, and is not on the toolbar. The list is named for the PROPERTY + // that earns the exemption rather than for the tier that first needed it, + // which is why it survives that tier's deletion unchanged. // // Named as an exception list rather than by asking the toolbar what it // holds, so that PUTTING one of these on the toolbar fails this test // rather than silently passing it. static const QStringList menuOnlySharedIconActions = { - QStringLiteral("archive_thread"), - QStringLiteral("delete_thread"), - QStringLiteral("spam_thread"), - QStringLiteral("mark_thread_read"), - QStringLiteral("mark_thread_unread"), - QStringLiteral("flag_thread"), QStringLiteral("reply_no_quote"), }; @@ -11393,44 +11304,33 @@ void TestMainWindow::undoOfADeleteRemovesTheOriginTagToo() 0); } -void TestMainWindow::deletingAThreadRootRemovesItFromTheInboxAndUndoReturnsIt() +void TestMainWindow::deletingALoneMessageRemovesItFromTheInboxAndUndoReturnsIt() { - // 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. + // Delete's message-scoped half, end to end, on the row where it still + // lives: a thread of ONE. Since item 177 a row with replies is the + // conversation and Delete there takes every message, so the only Delete + // that writes one message is this one. // - // 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. + // This test used to run on the ROOT of a three-message thread, because + // that was the message-scoped case then, and it pinned a defect that came + // from the mismatch: the toggle asked a thread ROW about its thread's + // tags, which notmuch gives as a UNION, so deleting the root left the + // union carrying no `deleted` and a second press ran Delete AGAIN, + // trash-to-trash, producing `deleted-from:inbox` and + // `deleted-from:Trash` at once with no way back. Item 177 dissolves the + // mismatch rather than patching it: the row and the write now agree about + // what they are for. The trash-to-trash assertions stay, because they are + // what proves a delete cannot run twice on one message. // - // 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. + // The row must be left ALONE between the presses: a re-query rebuilds it + // from the database and hides that class of defect, which is why an + // earlier version of this probe passed. WorkerBackedWindow backed; QVERIFY(backed.fixture().addMessage( - QStringLiteral("acct/inbox"), QStringLiteral("troot@example.org"), - QStringLiteral("Thread root"), QStringLiteral("sender@example.org"), + QStringLiteral("acct/inbox"), QStringLiteral("tlone@example.org"), + QStringLiteral("On its own"), 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"))); + QStringLiteral("Body."))); QVERIFY2(backed.build(QStringLiteral("acct"), QStringLiteral("acct"), QStringLiteral("Trash")), qPrintable(backed.error())); @@ -11448,32 +11348,26 @@ void TestMainWindow::deletingAThreadRootRemovesItFromTheInboxAndUndoReturnsIt() const QString root = backed.fixture().maildirPath(); const QString cfg = backed.fixture().configPath(); - const QString stem = QStringLiteral("troot.example.org"); + const QString stem = QStringLiteral("tlone.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); + // The guard that says this is the message-scoped path at all. With a + // second message the row would be a conversation and Delete would take + // the thread, which is a different test. + const QModelIndex row = model->index(0, 0, QModelIndex()); + QVERIFY2(!model->isConversationRow(row), + "the fixture's row is a conversation, so Delete is thread-scoped " + "here and this test asserts nothing about a lone message"); - view->setCurrentIndex(model->index(0, 0, QModelIndex())); + view->setCurrentIndex(row); + QApplication::processEvents(); window.findChild<QAction *>(QStringLiteral("delete"))->trigger(); QTRY_VERIFY_WITH_TIMEOUT(folderHasMessageFile(trash, stem), 15000); QTRY_VERIFY_WITH_TIMEOUT( - notmuchCount(cfg, QStringLiteral("id:troot@example.org and " + notmuchCount(cfg, QStringLiteral("id:tlone@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); - // There is no second press to make any more, and that is the point. // // Item 16's double-press-to-undelete existed because the deleted row @@ -11496,7 +11390,7 @@ void TestMainWindow::deletingAThreadRootRemovesItFromTheInboxAndUndoReturnsIt() 15000); QTRY_VERIFY_WITH_TIMEOUT( notmuchCount(cfg, - QStringLiteral("id:troot@example.org and tag:deleted")) + QStringLiteral("id:tlone@example.org and tag:deleted")) == 0, 15000); @@ -11506,24 +11400,24 @@ void TestMainWindow::deletingAThreadRootRemovesItFromTheInboxAndUndoReturnsIt() // it was returned to. QTRY_VERIFY_WITH_TIMEOUT( notmuchCount(cfg, - QStringLiteral("id:troot@example.org and tag:inbox")) + QStringLiteral("id:tlone@example.org and tag:inbox")) == 1, 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 " + QCOMPARE(notmuchCount(cfg, QStringLiteral("id:tlone@example.org")), 1); + QCOMPARE(notmuchCount(cfg, QStringLiteral("id:tlone@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 " + // The tag a re-delete would invent. Its presence is the signature of a + // trash-to-trash move rather than a variation on the origin-tag defects. + QCOMPARE(notmuchCount(cfg, QStringLiteral("id:tlone@example.org and " "tag:\"deleted-from:Trash\"")), 0); QVERIFY2(!folderHasMessageFile(trash, stem), - "the second press left the message in the trash"); + "the message was left in the trash"); } void TestMainWindow::deleteThreadMovesEveryMessageAndRepaintsTheRootCard() @@ -11588,7 +11482,7 @@ void TestMainWindow::deleteThreadMovesEveryMessageAndRepaintsTheRootCard() view->setCurrentIndex(model->index(0, 0, QModelIndex())); view->expand(model->index(0, 0, QModelIndex())); - window.findChild<QAction *>(QStringLiteral("delete_thread"))->trigger(); + window.findChild<QAction *>(QStringLiteral("delete"))->trigger(); // Every message MOVED, not merely tagged. This is the half that was // missing entirely: the action tagged and moved nothing. @@ -11616,7 +11510,7 @@ void TestMainWindow::deleteThreadMovesEveryMessageAndRepaintsTheRootCard() // 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<QAction *>(QStringLiteral("delete_thread"))->trigger(); + window.findChild<QAction *>(QStringLiteral("delete"))->trigger(); QTRY_VERIFY_WITH_TIMEOUT( notmuchCount(cfg, thread + QStringLiteral(" and tag:deleted")) == 0, @@ -11681,7 +11575,7 @@ void TestMainWindow::aFolderNameWithASpaceSurvivesTheRoundTrip() const QString home = root + QLatin1Char('/') + folder; view->setCurrentIndex(model->index(0, 0, QModelIndex())); - window.findChild<QAction *>(QStringLiteral("delete_thread"))->trigger(); + window.findChild<QAction *>(QStringLiteral("delete"))->trigger(); QTRY_VERIFY_WITH_TIMEOUT( notmuchCount(cfg, thread + QStringLiteral(" and tag:deleted")) == 2, @@ -11696,7 +11590,7 @@ void TestMainWindow::aFolderNameWithASpaceSurvivesTheRoundTrip() // Back again. view->setCurrentIndex(model->index(0, 0, QModelIndex())); - window.findChild<QAction *>(QStringLiteral("delete_thread"))->trigger(); + window.findChild<QAction *>(QStringLiteral("delete"))->trigger(); QTRY_VERIFY_WITH_TIMEOUT( notmuchCount(cfg, thread + QStringLiteral(" and tag:deleted")) == 0, @@ -15026,4 +14920,348 @@ void TestMainWindow::theBusinessSenderListIsLoadedAtStartup() QStringLiteral("cofidis.it"))); } +void TestMainWindow::theUnreadActionNamesTheThreadOnAConversationRow() +{ + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(model && view); + + ThreadSummary one = makeThread(QStringLiteral("t1"), + QStringList{ QStringLiteral("unread") }); + one.totalCount = 1; + ThreadSummary many = makeThread(QStringLiteral("t2"), + QStringList{ QStringLiteral("unread") }); + many.totalCount = 4; + model->appendBatch({ one, many }); + + auto *action = window.findChild<QAction *>(QStringLiteral("toggle_unread")); + QVERIFY(action); + + selectThreadRow(view, 0); + QApplication::processEvents(); + const QString onMessage = action->text(); + + selectThreadRow(view, 1); + QApplication::processEvents(); + const QString onThread = action->text(); + + QVERIFY2(onMessage != onThread, + "the label reads the same on a message and on a conversation, so " + "nothing tells the user which one the key will act on"); + QVERIFY2(onThread.contains(QStringLiteral("thread"), Qt::CaseInsensitive), + qPrintable(QStringLiteral("a conversation row's label does not " + "name the thread: %1").arg(onThread))); +} + +void TestMainWindow::deleteIsAbsentOnAReplyRow() +{ + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(model && view); + + ThreadSummary first = makeThread(QStringLiteral("t1"), {}); + first.totalCount = 1; + ThreadSummary many = makeThread(QStringLiteral("t2"), {}); + many.totalCount = 2; + model->appendBatch({ first, many }); + + MessageNode root; + root.messageId = QStringLiteral("m1"); + root.threadId = QStringLiteral("t2"); + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m2"); + reply.threadId = QStringLiteral("t2"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t2"), { root, reply }); + + const QModelIndex thread = model->index(1, 0, QModelIndex()); + view->expand(thread); + const QModelIndex replyRow = model->index(0, 0, thread); + view->selectionModel()->select( + replyRow, QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(replyRow); + QApplication::processEvents(); + + auto *del = window.findChild<QAction *>(QStringLiteral("delete")); + QVERIFY(del); + QVERIFY2(!del->isVisible() || !del->isEnabled(), + "Delete is offered on a reply: deleting is a conversation-level " + "action and a single reply cannot be removed from a thread"); + + auto *archive = window.findChild<QAction *>(QStringLiteral("archive")); + QVERIFY(archive); + QVERIFY2(!archive->isVisible() || !archive->isEnabled(), + "Archive is offered on a reply: it is conversation-level for the " + "same reason Delete is"); + + // And the mirror: on the conversation row itself both are back, so the + // hide is about what the row IS and not a stuck flag. + selectThreadRow(view, 1); + QApplication::processEvents(); + QVERIFY2(del->isVisible() && del->isEnabled(), + "Delete stayed hidden on a conversation row"); + QVERIFY2(archive->isVisible() && archive->isEnabled(), + "Archive stayed hidden on a conversation row"); +} + +void TestMainWindow::theWholeThreadSubmenuIsGone() +{ + const Config config; + MainWindow window(config); + + for (const QString &name : { QStringLiteral("archive_thread"), + QStringLiteral("delete_thread"), + QStringLiteral("spam_thread"), + QStringLiteral("flag_thread"), + QStringLiteral("mark_thread_read"), + QStringLiteral("mark_thread_unread") }) { + QVERIFY2(!window.findChild<QAction *>(name), + qPrintable(QStringLiteral("%1 still exists; the scope now " + "comes from the row, so a separate " + "action is a second answer to a " + "settled question").arg(name))); + } + + QVERIFY2(window.findChildren<QMenu *>( + QStringLiteral("threadActionsMenu")).isEmpty(), + "the Whole thread submenu is still built"); +} + +void TestMainWindow::forwardAndSaveAreAbsentOnAConversationRow() +{ + // A conversation row shows no message, so the three actions that need one + // cannot mean what they usually do. Forward and Save simply go; Reply + // becomes "reply to the thread" and is covered by the next test. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(model && view); + + ThreadSummary one = makeThread(QStringLiteral("t1"), {}); + one.totalCount = 1; + ThreadSummary many = makeThread(QStringLiteral("t2"), {}); + many.totalCount = 3; + model->appendBatch({ one, many }); + + auto *forward = window.findChild<QAction *>(QStringLiteral("forward")); + auto *save = window.findChild<QAction *>(QStringLiteral("save_message")); + auto *replyAll = window.findChild<QAction *>(QStringLiteral("reply_all")); + auto *noQuote = + window.findChild<QAction *>(QStringLiteral("reply_no_quote")); + QVERIFY(forward && save && replyAll && noQuote); + + selectThreadRow(view, 0); + QApplication::processEvents(); + QVERIFY2(forward->isVisible() && save->isVisible(), + "Forward and Save are hidden on a one-message row, where they " + "mean exactly what they always did"); + + selectThreadRow(view, 1); + QApplication::processEvents(); + QVERIFY2(!forward->isVisible(), + "Forward is offered on a conversation row, which shows no " + "message to forward"); + QVERIFY2(!save->isVisible(), + "Save is offered on a conversation row, which names no file"); + QVERIFY2(!replyAll->isVisible() && !noQuote->isVisible(), + "the reply variants are offered on a conversation row, where " + "there is one reply action and it is the thread's"); +} + +void TestMainWindow::replyOnAConversationRowNamesTheThread() +{ + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(model && view); + + ThreadSummary one = makeThread(QStringLiteral("t1"), {}); + one.totalCount = 1; + ThreadSummary many = makeThread(QStringLiteral("t2"), {}); + many.totalCount = 3; + model->appendBatch({ one, many }); + + auto *reply = window.findChild<QAction *>(QStringLiteral("reply")); + QVERIFY(reply); + + selectThreadRow(view, 0); + QApplication::processEvents(); + const QString onMessage = reply->text(); + + selectThreadRow(view, 1); + QApplication::processEvents(); + const QString onThread = reply->text(); + + QVERIFY2(reply->isVisible(), + "Reply disappeared on a conversation row; one reply action stays, " + "and it answers the thread"); + QVERIFY2(onMessage != onThread, + "Reply reads the same on a message and on a conversation, so " + "nothing says the answer goes to the whole thread"); + QVERIFY2(onThread.contains(QStringLiteral("thread"), Qt::CaseInsensitive), + qPrintable(QStringLiteral("a conversation row's Reply does not " + "name the thread: %1").arg(onThread))); +} + +void TestMainWindow::replyToAConversationAnswersItsNewestMessage() +{ + // The routing, which the label test does NOT cover: a probe on the action's + // TEXT passes with the conversation branch of composeReply() deleted + // outright, measured. This asserts through the composer that opens. + // + // The newest message rather than the first is the whole decision. A card + // stands above a conversation and shows its OPENING post, so answering + // what the card displays would thread the reply off a message the + // discussion has moved on from: In-Reply-To and References would fork the + // thread, and the recipients would be whoever was in it at the start. + // + // Three senders, one per message, so "answered the newest" is + // distinguishable from "answered the first" AND from "answered any of + // them". Reply-all, so the To and Cc together carry the whole cast and the + // assertion is about which message supplied the headers, not about which + // fold the addresses landed in. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("work/inbox"), QStringLiteral("rt0@example.org"), + QStringLiteral("RT root"), QStringLiteral("first@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Root body."), false)); + QVERIFY(backed.fixture().addMessage( + QStringLiteral("work/inbox"), QStringLiteral("rt1@example.org"), + QStringLiteral("Re: RT root"), QStringLiteral("middle@example.org"), + QStringLiteral("Fri, 14 Aug 2026 11:00:00 +0200"), + QStringLiteral("Reply one."), false, QStringLiteral("rt0@example.org"))); + QVERIFY(backed.fixture().addMessage( + QStringLiteral("work/inbox"), QStringLiteral("rt2@example.org"), + QStringLiteral("Re: RT root"), QStringLiteral("newest@example.org"), + QStringLiteral("Fri, 14 Aug 2026 12:00:00 +0200"), + QStringLiteral("Reply two."), false, QStringLiteral("rt0@example.org"))); + QVERIFY2(backed.buildWithAccounts( + { { QStringLiteral("work"), QStringLiteral("work"), + QString(), QStringLiteral("/bin/true"), + QStringLiteral("you@example.org") } }), + qPrintable(backed.error())); + + MainWindow window(backed.config()); + auto *model = window.findChild<ThreadListModel *>(); + auto *view = window.findChild<ThreadListView *>(); + auto *queryEdit = + window.findChild<QLineEdit *>(QStringLiteral("queryEdit")); + QVERIFY(model && view && queryEdit); + + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1 + && !window.mailRootForTesting().isEmpty(), + 15000); + + const QModelIndex row = model->index(0, 0, QModelIndex()); + QVERIFY2(model->isConversationRow(row), + "the fixture's row is not a conversation, so Reply would take the " + "ordinary message path and this test would assert nothing"); + + view->setCurrentIndex(row); + view->selectionModel()->select( + row, QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + QApplication::processEvents(); + + auto *reply = window.findChild<QAction *>(QStringLiteral("reply")); + QVERIFY(reply); + QVERIFY2(reply->isEnabled(), + "the account cannot send, so Reply is disabled and the gesture " + "never reaches the code under test"); + reply->trigger(); + + QTRY_VERIFY_WITH_TIMEOUT(window.openComposerCount() == 1, 15000); + ComposeWindow *composer = window.openComposersForTest().value(0); + QVERIFY(composer); + + auto *to = composer->findChild<QLineEdit *>(QStringLiteral("to")); + auto *cc = composer->findChild<QLineEdit *>(QStringLiteral("cc")); + QVERIFY(to && cc); + const QString recipients = to->text() + QLatin1Char(' ') + cc->text(); + + QVERIFY2(recipients.contains(QStringLiteral("newest@example.org")), + qPrintable(QStringLiteral("the reply does not answer the " + "conversation's newest message: %1") + .arg(recipients))); + + // And it QUOTES NOTHING, per the user: "we just add an answer to the + // thread". A quoted body would be the newest message's text, which is a + // second, separate way for this to be wrong. + auto *body = composer->findChild<QPlainTextEdit *>(QStringLiteral("body")); + QVERIFY(body); + QVERIFY2(!body->toPlainText().contains(QStringLiteral("Reply two.")), + qPrintable(QStringLiteral("the reply quoted the message it " + "answers: %1").arg(body->toPlainText()))); + + composer->show(); + composer->close(); +} + +void TestMainWindow::replyIsUntouchedOnAMessageRow() +{ + // The other half of the rule: a reply row is a message like any other, so + // every compose action behaves exactly as it did before item 177. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(model && view); + + ThreadSummary many = makeThread(QStringLiteral("t1"), {}); + many.totalCount = 2; + model->appendBatch({ many }); + + MessageNode root; + root.messageId = QStringLiteral("m1"); + root.threadId = QStringLiteral("t1"); + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m2"); + reply.threadId = QStringLiteral("t1"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t1"), { root, reply }); + + const QModelIndex thread = model->index(0, 0, QModelIndex()); + view->expand(thread); + const QModelIndex replyRow = model->index(0, 0, thread); + view->selectionModel()->select( + replyRow, + QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(replyRow); + QApplication::processEvents(); + + for (const QString &name : { QStringLiteral("reply"), + QStringLiteral("reply_all"), + QStringLiteral("reply_no_quote"), + QStringLiteral("forward"), + QStringLiteral("save_message") }) { + auto *action = window.findChild<QAction *>(name); + QVERIFY(action); + QVERIFY2(action->isVisible(), + qPrintable(QStringLiteral("%1 is hidden on a reply row, which " + "is an ordinary message").arg(name))); + } + + auto *replyAction = window.findChild<QAction *>(QStringLiteral("reply")); + QVERIFY2(!replyAction->text().contains(QStringLiteral("thread"), + Qt::CaseInsensitive), + qPrintable(QStringLiteral("a reply row's Reply claims to answer " + "the thread: %1") + .arg(replyAction->text()))); +} + #include "test_mainwindow.moc" diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index 2e960be..45dc9a0 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -101,6 +101,7 @@ private slots: void purgeMessagesReportsWhatItDestroyed(); void purgeMessagesLeavesOtherMessagesAlone(); void purgeMessagesDoesNotClaimAnIdItCouldNotDelete(); + void resolveThreadMessagesReportsTheNewestFirst(); void resolveQueryMessagesRefusesAnEmptyQuery(); void moveMessagesKeepsTheMaildirFlags(); void moveMessagesRecoversWhenASyncRenamedTheFile(); @@ -1530,6 +1531,37 @@ void TestNotmuchWorker::purgeMessagesDoesNotClaimAnIdItCouldNotDelete() "claimed to have destroyed a message whose file was already gone"); } +void TestNotmuchWorker::resolveThreadMessagesReportsTheNewestFirst() +{ + // Item 177's "Reply to this thread" reads the FIRST id this reports as the + // conversation's newest message, so its In-Reply-To and References land + // the answer at the end of the conversation and its recipients are the + // ones currently in it. Answering the thread's opening post instead would + // fork the discussion, silently, in mail that has already gone out. + // + // Asserted here rather than in the UI because the ORDER is the contract. + // notmuch's own default already is newest-first, which is exactly why this + // needs a test: the code now states the sort explicitly, and nothing else + // would notice if that statement were dropped and the default later moved. + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy resolved(&worker, &NotmuchWorker::threadMessagesResolved); + + // Thread A, whose two messages are a day apart. A one-message thread + // answers identically whatever the sort and would assert nothing. + const QString threadId = threadIdOf(QStringLiteral("Release notes")); + QVERIFY(!threadId.isEmpty()); + + worker.resolveThreadMessages({ threadId }, QStringLiteral("reply_thread")); + + QCOMPARE(resolved.size(), 1); + const QStringList ids = resolved.first().at(0).toStringList(); + QCOMPARE(ids.size(), 2); + QCOMPARE(ids.first(), QStringLiteral("a2@example.org")); + QVERIFY2(ids.last() == QStringLiteral("a1@example.org"), + qPrintable(QStringLiteral("wrong order: %1") + .arg(ids.join(QLatin1Char(','))))); +} + void TestNotmuchWorker::resolveQueryMessagesRefusesAnEmptyQuery() { // An EMPTY query means "match everything" to notmuch, and this walk is diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index edc84ee..dbff6b7 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -41,9 +41,6 @@ private slots: void replySharingEveryThreadTagShowsNone(); void reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem(); void anUnexpandedMultiMessageThreadOffersAnExpander(); - void scopeFollowsTheSelectedRowKind(); - void scopeCountsEveryMessageOfAnUnexpandedThread(); - void scopeHonoursAMixedSelectionWithoutEscalating(); void startsEmpty(); void accountKeysComeFromTheAccountTags(); void accountKeysCoverAThreadSpanningTwoAccounts(); @@ -78,8 +75,6 @@ private slots: void markingAReplyReadChangesItsForeground(); void anUnreadReplyIsBoldAndStillSmallerThanItsThread(); void aThreadTagChangeReachesItsLoadedReplies(); - void messageScopeResolvesAThreadRowToTheMessageItDisplays(); - void messageScopeSkipsAThreadRowItCannotNameAMessageFor(); void aMessageTagChangeReachesTheRootCardsOwnMessage(); void aMessageTagChangeOnOneOfManyLeavesTheThreadSummaryAlone(); void aConversationRowDrawsTheThreadsTags(); @@ -108,6 +103,7 @@ private slots: void aLoadedThreadTrustsItsChildrenOverItsCount(); void aMessageRowIsNeverAConversationRow(); void aConversationRowResolvesToItsThread(); + void aRowNamingNoMessageIsSkippedNotEscalated(); void aLoneMessageRowResolvesToItsMessage(); void aReplyRowResolvesToItsMessage(); void aMixedSelectionCarriesBothScopes(); @@ -278,86 +274,6 @@ void TestThreadListModel::anUnexpandedMultiMessageThreadOffersAnExpander() QVERIFY(!model.hasChildren(model.index(0, 0, withReplies))); } -void TestThreadListModel::scopeFollowsTheSelectedRowKind() -{ - ThreadListModel model; - ThreadSummary t = makeThread(QStringLiteral("t1"), - QStringLiteral("A subject")); - t.totalCount = 3; - model.appendBatch({ t }); - model.setThreadMessages(QStringLiteral("t1"), - { makeNode(QStringLiteral("m0@example.org"), 0), - makeNode(QStringLiteral("m1@example.org"), 1) }); - - const QModelIndex root = model.index(0, 0, QModelIndex()); - const QModelIndex child = model.index(0, 0, root); - - // A thread root acts on the whole thread, and reports every message it - // stands for so the status bar can say so. - const ActionScope threadScope = model.scopeFor({ root }); - QCOMPARE(threadScope.threadIds, QStringList{ QStringLiteral("t1") }); - QVERIFY(threadScope.messageIds.isEmpty()); - QCOMPARE(threadScope.messageCount, 3); - QVERIFY(threadScope.wholeThread); - - // A message row acts on that message alone. - const ActionScope messageScope = model.scopeFor({ child }); - QVERIFY(messageScope.threadIds.isEmpty()); - QCOMPARE(messageScope.messageIds, - QStringList{ QStringLiteral("m1@example.org") }); - QCOMPARE(messageScope.messageCount, 1); - QVERIFY(!messageScope.wholeThread); -} - -void TestThreadListModel::scopeCountsEveryMessageOfAnUnexpandedThread() -{ - // totalCount, not the loaded children. A thread that was never expanded - // still has all of its messages, and counting only what happens to be on - // screen would understate what the action is about to do. - ThreadListModel model; - ThreadSummary t = makeThread(QStringLiteral("t1"), - QStringLiteral("A subject")); - t.totalCount = 7; - model.appendBatch({ t }); - - const QModelIndex root = model.index(0, 0, QModelIndex()); - QCOMPARE(model.rowCount(root), 0); // guard: nothing expanded - - const ActionScope scope = model.scopeFor({ root }); - QCOMPARE(scope.messageCount, 7); -} - -void TestThreadListModel::scopeHonoursAMixedSelectionWithoutEscalating() -{ - // Selecting a thread root and an unrelated reply acts on that whole thread - // AND that one message. Nothing is escalated to thread scope or narrowed to - // message scope silently, which is the point of the scope being visible. - ThreadListModel model; - ThreadSummary t1 = makeThread(QStringLiteral("t1"), QStringLiteral("One")); - t1.totalCount = 2; - ThreadSummary t2 = makeThread(QStringLiteral("t2"), QStringLiteral("Two")); - t2.totalCount = 5; - model.appendBatch({ t1, t2 }); - - MessageNode reply = makeNode(QStringLiteral("m1@example.org"), 1); - reply.threadId = QStringLiteral("t2"); - model.setThreadMessages(QStringLiteral("t2"), - { makeNode(QStringLiteral("m0@example.org"), 0), - reply }); - - const QModelIndex firstRoot = model.index(0, 0, QModelIndex()); - const QModelIndex secondRoot = model.index(1, 0, QModelIndex()); - const QModelIndex reply1 = model.index(0, 0, secondRoot); - - const ActionScope scope = model.scopeFor({ firstRoot, reply1 }); - QCOMPARE(scope.threadIds, QStringList{ QStringLiteral("t1") }); - QCOMPARE(scope.messageIds, QStringList{ QStringLiteral("m1@example.org") }); - - // 2 from the whole thread plus 1 for the lone message. - QCOMPARE(scope.messageCount, 3); - QVERIFY(scope.wholeThread); -} - void TestThreadListModel::accountKeysComeFromTheAccountTags() { // Item 49 reads this to decide which mbsync channels a sync needs. Only @@ -1268,80 +1184,6 @@ void TestThreadListModel::aThreadTagChangeReachesItsLoadedReplies() QVERIFY(model.messageAt(replyIndex).isUnread()); } -void TestThreadListModel::messageScopeResolvesAThreadRowToTheMessageItDisplays() -{ - // Item 108. A thread root RENDERS one message since item 66, so acting on - // it acts on that message. The thread's other messages are reached through - // the explicit thread actions, which still resolve through scopeFor(). - ThreadListModel model; - ThreadSummary t = makeThread(QStringLiteral("t1"), - QStringLiteral("A subject")); - t.totalCount = 7; - t.firstMessageId = QStringLiteral("m0@example.org"); - model.appendBatch({ t }); - - const QModelIndex root = model.index(0, 0, QModelIndex()); - - // Unexpanded, which is the case that matters: the id comes from the query, - // so this needs no children loaded. - QCOMPARE(model.rowCount(root), 0); - - const ActionScope scope = model.messageScopeFor({ root }); - QCOMPARE(scope.messageIds, QStringList{ QStringLiteral("m0@example.org") }); - QVERIFY2(scope.threadIds.isEmpty(), - "a thread row still resolved to its whole thread, so every action " - "on a root card would touch messages it does not display"); - QCOMPARE(scope.messageCount, 1); - QVERIFY2(!scope.wholeThread, - "the status bar would claim '(whole thread)' for a one-message " - "action"); - - // The old resolver is unchanged and is what the thread actions use. - const ActionScope threadScope = model.scopeFor({ root }); - QCOMPARE(threadScope.threadIds, QStringList{ QStringLiteral("t1") }); - QCOMPARE(threadScope.messageCount, 7); - QVERIFY(threadScope.wholeThread); - - // A reply row is unchanged in both: it always stood for one message. - model.setThreadMessages(QStringLiteral("t1"), - { makeNode(QStringLiteral("m0@example.org"), 0), - makeNode(QStringLiteral("m1@example.org"), 1) }); - const QModelIndex reply = model.index(0, 0, root); - QCOMPARE(model.messageScopeFor({ reply }).messageIds, - QStringList{ QStringLiteral("m1@example.org") }); - - // A root and one of its own replies is two DISTINCT messages, not one - // deduplicated to the thread. - const ActionScope both = model.messageScopeFor({ root, reply }); - QCOMPARE(both.messageIds, - (QStringList{ QStringLiteral("m0@example.org"), - QStringLiteral("m1@example.org") })); - QCOMPARE(both.messageCount, 2); -} - -void TestThreadListModel::messageScopeSkipsAThreadRowItCannotNameAMessageFor() -{ - // firstMessageId is populated by the worker from the query. A summary that - // arrived without one names no message, and the tempting fallback is to - // act on the whole thread instead. That is exactly the silent escalation - // item 108 exists to remove: the user would ask to act on one message and - // hit the conversation. - ThreadListModel model; - ThreadSummary t = makeThread(QStringLiteral("t1"), - QStringLiteral("A subject")); - t.totalCount = 4; - t.firstMessageId.clear(); - model.appendBatch({ t }); - - const QModelIndex root = model.index(0, 0, QModelIndex()); - const ActionScope scope = model.messageScopeFor({ root }); - - QVERIFY2(scope.isEmpty(), - "a thread row with no message id was escalated to its whole " - "thread rather than skipped"); - QCOMPARE(scope.messageCount, 0); -} - void TestThreadListModel::aMessageTagChangeReachesTheRootCardsOwnMessage() { // The user, 2026-08-16: "delete single message on the root message of a @@ -2303,6 +2145,38 @@ void TestThreadListModel::aConversationRowResolvesToItsThread() "a conversation row named a message, so an action on it would " "touch one message of the thread it claims to act on"); QVERIFY(scope.wholeThread); + + // totalCount, not the loaded children: nothing is expanded here, and a + // status bar counting only what happens to be on screen would understate + // what the action is about to do. + QCOMPARE(model.rowCount(model.index(1, 0, QModelIndex())), 0); + QCOMPARE(scope.messageCount, 4); +} + +void TestThreadListModel::aRowNamingNoMessageIsSkippedNotEscalated() +{ + // firstMessageId comes from the query. A summary that arrived without one + // names no message, and the tempting fallback is to act on the whole + // thread instead. That is a silent escalation: the user asked to act on + // one message and would hit the conversation. + // + // A thread of ONE, deliberately. A conversation row resolves to its thread + // and needs no message id at all, so the escalation can only be reached + // where the row really does stand for one message. + ThreadListModel model; + ThreadSummary t = makeThread(QStringLiteral("t1"), QStringLiteral("Alone")); + t.totalCount = 1; + t.firstMessageId.clear(); + model.appendBatch({ t }); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + QVERIFY(!model.isConversationRow(root)); + + const ActionScope scope = model.scopeForSelection({ root }); + QVERIFY2(scope.isEmpty(), + "a row with no message id was escalated to its whole thread " + "rather than skipped"); + QCOMPARE(scope.messageCount, 0); } void TestThreadListModel::aLoneMessageRowResolvesToItsMessage() diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index 040af5c..621668f 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -471,6 +471,14 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Nascondi l'output della sincronizzazione fino al prossimo errore</translation> </message> <message> + <source>No thread is selected</source> + <translation>Nessuna conversazione selezionata</translation> + </message> + <message> + <source>Select one message to save</source> + <translation>Seleziona un singolo messaggio da salvare</translation> + </message> + <message> <source>&Find</source> <translation>&Cerca</translation> </message> @@ -523,10 +531,6 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Aggiunge o rimuove l'etichetta deleted</translation> </message> <message> - <source>Mark thread &read</source> - <translation>Segna conversazione come &letta</translation> - </message> - <message> <source>Remove the unread tag from every message of the selected threads</source> <translation>Rimuove il tag unread da ogni messaggio delle conversazioni selezionate</translation> </message> @@ -558,6 +562,10 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> </translation> </message> <message> + <source>That thread holds no message to answer</source> + <translation>Quella conversazione non contiene messaggi a cui rispondere</translation> + </message> + <message> <source>Restore</source> <translation>Ripristina</translation> </message> @@ -705,18 +713,10 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Rimuove inbox da ogni messaggio delle conversazioni selezionate</translation> </message> <message> - <source>Archive thread</source> - <translation>Archivia conversazione</translation> - </message> - <message> <source>&Delete thread</source> <translation>&Elimina conversazione</translation> </message> <message> - <source>Add or remove the deleted tag on whole threads</source> - <translation>Aggiunge o rimuove l'etichetta eliminato su intere conversazioni</translation> - </message> - <message> <source>Undelete thread</source> <translation>Ripristina conversazione</translation> </message> @@ -827,38 +827,14 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Elimina definitivamente ogni messaggio nel cestino</translation> </message> <message> - <source>Add spam and remove inbox on whole threads</source> - <translation>Aggiunge spam e rimuove inbox su intere conversazioni</translation> - </message> - <message> - <source>Mark thread spam</source> - <translation>Segna conversazione come spam</translation> - </message> - <message> - <source>Mark thread read</source> - <translation>Segna conversazione come letta</translation> - </message> - <message> - <source>Mark thread &unread</source> - <translation>Segna conversazione come &non letta</translation> - </message> - <message> <source>Add the unread tag to every message of the selected threads</source> <translation>Aggiunge il tag unread a ogni messaggio delle conversazioni selezionate</translation> </message> <message> - <source>Mark thread unread</source> - <translation>Segna conversazione come non letta</translation> - </message> - <message> <source>Mark every message of the selected threads as important</source> <translation>Segna come importante ogni messaggio delle conversazioni selezionate</translation> </message> <message> - <source>Mark thread important</source> - <translation>Segna conversazione come importante</translation> - </message> - <message> <source>Tagging &rules...</source> <translation>&Regole di etichettatura...</translation> </message> @@ -1259,6 +1235,10 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> </translation> </message> <message> + <source>Mark thread as &read</source> + <translation>Segna conversazione come &letta</translation> + </message> + <message> <source>Mark as &read</source> <translation>Segna come &letto</translation> </message> @@ -1267,6 +1247,10 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Rimuove il tag unread dalla selezione</translation> </message> <message> + <source>Mark thread as &unread</source> + <translation>Segna conversazione come &non letta</translation> + </message> + <message> <source>Mark as &unread</source> <translation>Segna come &non letto</translation> </message> @@ -1274,6 +1258,38 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <source>Add the unread tag to the selection</source> <translation>Aggiunge il tag unread alla selezione</translation> </message> + <message> + <source>Move every message of the selected threads to the trash</source> + <translation>Sposta nel cestino ogni messaggio delle conversazioni selezionate</translation> + </message> + <message> + <source>&Restore thread from trash</source> + <translation>&Ripristina conversazione dal cestino</translation> + </message> + <message> + <source>Move every message of the selected threads out of the trash</source> + <translation>Sposta fuori dal cestino ogni messaggio delle conversazioni selezionate</translation> + </message> + <message> + <source>Add spam and remove inbox on the selected threads</source> + <translation>Aggiunge spam e rimuove inbox sulle conversazioni selezionate</translation> + </message> + <message> + <source>&Important thread</source> + <translation>Conversazione &importante</translation> + </message> + <message> + <source>Remove the inbox tag</source> + <translation>Rimuove il tag inbox</translation> + </message> + <message> + <source>Reply to this &thread</source> + <translation>Rispondi a questa &conversazione</translation> + </message> + <message> + <source>Add an answer to the end of this conversation</source> + <translation>Aggiunge una risposta in fondo a questa conversazione</translation> + </message> <message numerus="yes"> <source>1 thread selected (%n message(s))</source> <translation> @@ -1371,10 +1387,6 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> </translation> </message> <message> - <source>&Whole thread</source> - <translation>&Intera conversazione</translation> - </message> - <message> <source>Select a thread first</source> <translation>Seleziona prima una conversazione</translation> </message> |