fix: undo only what the write actually changed

Closes item 176. applyTags reports the messages whose tags really moved, and
a command stores that rather than what it asked for, so undoing a mark-read
no longer marks the whole conversation unread.

3 files changed, 220 insertions, 33 deletions

diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
index c781d77..4cce042 100644
--- a/src/mainwindow.cpp
+++ b/src/mainwindow.cpp
@@ -3415,6 +3415,9 @@ void MainWindow::runQuery(FlatResult flat, AccountScope scope)
     // update they invert would be a no-op against the new result set, leaving
     // undo half-applied: the database would change and the list would not.
     m_undoStack.clear();
+
+    // The queue holds pointers the stack owned. See awaitTagConfirmation().
+    m_awaitingTagConfirmation.clear();
     m_pendingChange = {};
     m_pendingThreadIds.clear();
 
@@ -3582,14 +3585,17 @@ void MainWindow::markAllRead()
     m_markReadMessageId.clear();
 
     const QString description = tr("Mark all read");
+    // ONE command for the batch, exactly as tagSelected does: a user who marks
+    // 400 threads read expects a single Ctrl+Z to put them back. Built and
+    // armed before the send so the worker's confirmation can tell it which
+    // messages actually moved (item 176).
+    auto *command = new ThreadTagCommand(this, threadIds, {},
+                                         { QStringLiteral("unread") },
+                                         description);
+    awaitTagConfirmation(command);
     sendThreadTagChange(threadIds, {}, { QStringLiteral("unread") },
                         description);
-
-    // ONE command for the batch, exactly as tagSelected does: a user who marks
-    // 400 threads read expects a single Ctrl+Z to put them back.
-    m_undoStack.push(new ThreadTagCommand(this, threadIds, {},
-                                          { QStringLiteral("unread") },
-                                          description));
+    m_undoStack.push(command);
 
     showTransientStatus(
         tr("%1: %n thread(s)", "", threadIds.size()).arg(description));
@@ -4303,6 +4309,10 @@ void MainWindow::revertPendingTagChange()
     if (m_undoStack.canUndo())
         m_undoStack.undo();
 
+    // The write never landed, so no confirmation is coming for it. Left in
+    // place it would be credited the NEXT write's ids.
+    m_awaitingTagConfirmation.clear();
+
     m_pendingChange = {};
     m_pendingThreadIds.clear();
 }
@@ -4549,6 +4559,16 @@ void MainWindow::onTagsApplied(const TagChange &change)
     m_pendingChange = {};
     m_pendingThreadIds.clear();
 
+    // Item 176. The ids the worker reports are the ones whose tags really
+    // moved, which is what the undo entry has to invert. Inverting the REQUEST
+    // instead turned the undo of "mark 44 read" into "mark 44 unread": 2
+    // unread became 43, on real mail, and maildir.synchronize_flags carried it
+    // into the filenames.
+    if (!m_awaitingTagConfirmation.isEmpty()) {
+        TagCommand *command = m_awaitingTagConfirmation.takeFirst();
+        command->addChangedMessageIds(change.messageIds);
+    }
+
     // Recorded here, where a write is CONFIRMED, rather than where one is sent:
     // an optimistic update the worker later rejects must not leave the
     // indicator claiming an edit that never landed.
@@ -5555,18 +5575,28 @@ void MainWindow::tagSelected(const QStringList &add, const QStringList &remove,
         return;
 
     if (!scope.threadIds.isEmpty()) {
+        // Constructed and armed BEFORE the send, not after: the confirmation
+        // names the messages the write moved, and the command has to be
+        // reachable when it arrives (item 176).
+        auto *command = new ThreadTagCommand(this, scope.threadIds, add,
+                                             remove, description);
+        awaitTagConfirmation(command);
         sendThreadTagChange(scope.threadIds, add, remove, description);
 
-        // Pushed for undo. The inverse re-resolves the same threads, so it
-        // works whether or not those rows are still selected.
-        m_undoStack.push(new ThreadTagCommand(this, scope.threadIds, add,
-                                              remove, description));
+        // Pushed for undo. A redo re-resolves the same threads, so it works
+        // whether or not those rows are still selected.
+        m_undoStack.push(command);
     }
 
     if (!scope.messageIds.isEmpty()) {
+        auto *command = new MessageTagCommand(this, scope.messageIds, add,
+                                              remove, description);
+        // Not the first when the thread branch above already armed one: both
+        // halves of one gesture are in flight together and their
+        // confirmations come back in send order.
+        awaitTagConfirmation(command, scope.threadIds.isEmpty());
         sendMessageTagChange(scope.messageIds, add, remove, description);
-        m_undoStack.push(new MessageTagCommand(this, scope.messageIds, add,
-                                               remove, description));
+        m_undoStack.push(command);
     }
 
     // The scope named after the fact, since the selection may well be gone by
@@ -5982,11 +6012,13 @@ void MainWindow::onThreadMessagesResolved(const QStringList &messageIds,
         // The tag comes off so the row stops claiming to be deleted, but no
         // file moves, since guessing a folder would put the message somewhere
         // the user never had it.
+        auto *command = new MessageTagCommand(this, unknown, {},
+                                              { QStringLiteral("deleted") },
+                                              tr("Undelete thread"));
+        awaitTagConfirmation(command);
         sendMessageTagChange(unknown, {}, { QStringLiteral("deleted") },
                              tr("Undelete thread"));
-        m_undoStack.push(new MessageTagCommand(this, unknown, {},
-                                               { QStringLiteral("deleted") },
-                                               tr("Undelete thread")));
+        m_undoStack.push(command);
     }
 
     for (auto it = byOrigin.cbegin(); it != byOrigin.cend(); ++it) {
@@ -6393,11 +6425,13 @@ void MainWindow::restoreSelected(bool fallbackToInbox)
                        "configured account.", "", int(stranded.size())));
             }
         } else {
+            auto *command = new MessageTagCommand(
+                this, unknown, {}, { QStringLiteral("deleted") },
+                tr("Undelete"));
+            awaitTagConfirmation(command);
             sendMessageTagChange(unknown, {}, { QStringLiteral("deleted") },
                                  tr("Undelete"));
-            m_undoStack.push(new MessageTagCommand(
-                this, unknown, {}, { QStringLiteral("deleted") },
-                tr("Undelete")));
+            m_undoStack.push(command);
         }
     }
 
@@ -6801,7 +6835,8 @@ void MainWindow::onMessagesMoved(const QMap<QString, QString> &originByMessageId
 void MainWindow::sendThreadTagChange(const QStringList &threadIds,
                                      const QStringList &add,
                                      const QStringList &remove,
-                                     const QString &description)
+                                     const QString &description,
+                                     const QStringList &onlyMessageIds)
 {
     // Optimistic: the rows change now, so a bulk archive of hundreds of threads
     // feels instant. Recorded so onWorkerError() can put them back.
@@ -6847,7 +6882,8 @@ void MainWindow::sendThreadTagChange(const QStringList &threadIds,
     // what the user asked for and it is going to be applied.
     if (aSyncHoldsTheWriteLock()) {
         m_heldEdits.append(HeldEdit{
-            threadIds, TagChange{ {}, add, remove, description } });
+            onlyMessageIds.isEmpty() ? threadIds : QStringList{},
+            TagChange{ onlyMessageIds, add, remove, description } });
 
         // NOT transient. This describes state that lasts until the sync ends,
         // and a message that expired would leave the user with rows showing a
@@ -6862,7 +6898,18 @@ void MainWindow::sendThreadTagChange(const QStringList &threadIds,
     }
 
     m_pendingThreadIds = threadIds;
-    m_pendingChange = TagChange{ {}, add, remove, description };
+    m_pendingChange = TagChange{ onlyMessageIds, add, remove, description };
+
+    // Item 176. An UNDO names the messages the original write actually moved,
+    // so it writes those and no others: inverting over the whole thread is
+    // what turned "mark 44 read" into "mark 44 unread". The repaint above is
+    // still thread-scoped, and correctly so, because the rows being put back
+    // are the conversation rows the write changed.
+    if (!onlyMessageIds.isEmpty()) {
+        QMetaObject::invokeMethod(m_worker, "applyTags", Qt::QueuedConnection,
+                                  Q_ARG(TagChange, m_pendingChange));
+        return;
+    }
 
     // The worker resolves thread ids to message ids: the UI does not hold
     // message ids for rows it never opened.
diff --git a/src/mainwindow.h b/src/mainwindow.h
index 0999ef3..11f35e7 100644
--- a/src/mainwindow.h
+++ b/src/mainwindow.h
@@ -60,6 +60,8 @@ class QTimer;
 class QToolButton;
 class QVBoxLayout;
 
+class TagCommand;
+
 class BusyIndicator;
 class CardDelegate;
 class ThreadListModel;
@@ -1029,10 +1031,41 @@ private:
     /// Invokable so a test can record an edit against a known account without a
     /// worker: this is where m_editedAccounts is populated, and item 54's
     /// draining of it cannot be observed otherwise.
+    ///
+    /// `onlyMessageIds`, when non-empty, is the exact set of messages to
+    /// WRITE, while the repaint still covers `threadIds`. Item 176: an undo
+    /// covers what the write actually moved, and only the worker knows which
+    /// messages that was, so the command carries them back here. The two
+    /// scopes differ on purpose: the rows being put back are the conversation
+    /// rows the original write changed, so the card is repainted as a whole.
     Q_INVOKABLE void sendThreadTagChange(const QStringList &threadIds,
                              const QStringList &add,
                              const QStringList &remove,
-                             const QString &description);
+                             const QString &description,
+                             const QStringList &onlyMessageIds = {});
+
+    /// Names the command the next confirmed write belongs to.
+    ///
+    /// Item 176. A command must undo what the write CHANGED, not what it
+    /// asked for, and only the worker knows which. Armed immediately before
+    /// the send and read in onTagsApplied().
+    ///
+    /// A QUEUE, not one slot. One gesture over a mixed selection sends a
+    /// thread write and a message write, both queued to the worker, so two
+    /// confirmations come back in the order they were sent; a single slot
+    /// would credit the first arrival to the second command. A confirmation
+    /// arriving with the queue empty has no command to inform, which is the
+    /// case for every write that never goes on the stack (the automatic
+    /// mark-read, a move's tags).
+    ///
+    /// `first` says this command opens a new gesture, which discards anything
+    /// a previous one stranded.
+    void awaitTagConfirmation(TagCommand *command, bool first = true)
+    {
+        if (first)
+            m_awaitingTagConfirmation.clear();
+        m_awaitingTagConfirmation.append(command);
+    }
 
     /// The same for individual MESSAGES, without touching the undo stack.
     /// Both tagSelected() and MessageTagCommand route through this.
@@ -1680,6 +1713,18 @@ private:
     TagChange m_pendingChange;
     QStringList m_pendingThreadIds;
 
+    /// The undo entries the confirmations still in flight belong to, oldest
+    /// first. See awaitTagConfirmation().
+    ///
+    /// Raw pointers, and the undo stack owns them. Cleared whenever the stack
+    /// is, and drained in order by onTagsApplied(). An entry can be left
+    /// stranded by a write that never confirms (one held for a sync, or one
+    /// the worker found nothing to change), so a gesture clears whatever is
+    /// left before arming its own: crediting a fresh confirmation to a dead
+    /// command would put the wrong ids on the wrong undo entry, which is the
+    /// class of defect item 176 is about.
+    QVector<TagCommand *> m_awaitingTagConfirmation;
+
     /// Account keys whose mail store has edits a sync has not yet carried,
     /// for item 49's per-account sync.
     ///
@@ -1703,17 +1748,58 @@ private:
     Q_INVOKABLE QStringList pendingSyncChannels() const;
 };
 
+/// What a tag command needs from the worker's confirmation.
+///
+/// Item 176. A command is pushed with what the user ASKED for, and the worker
+/// is the only thing that knows what actually moved: `applyTags()` holds each
+/// message open and compares. Undoing the request rather than the effect is
+/// what turned "mark 44 read" into "mark 44 unread", measured on real mail as
+/// 2 unread becoming 43, and `maildir.synchronize_flags` carried it to the
+/// files. `MainWindow::onTagsApplied()` hands the effective ids to whichever
+/// command is waiting for them.
+class TagCommand : public QUndoCommand
+{
+public:
+    using QUndoCommand::QUndoCommand;
+
+    /// Records the messages whose tags the write really moved.
+    ///
+    /// Accumulated rather than assigned: a thread-scoped write and a
+    /// message-scoped one can be in flight together from one gesture, and a
+    /// redo of a batch arrives as more than one confirmation.
+    void addChangedMessageIds(const QStringList &ids)
+    {
+        for (const QString &id : ids) {
+            if (!m_changedMessageIds.contains(id))
+                m_changedMessageIds.append(id);
+        }
+    }
+
+    QStringList changedMessageIds() const { return m_changedMessageIds; }
+
+protected:
+    /// Cleared before each re-send, so a redo learns what IT moved rather than
+    /// undoing the effect of the run before it.
+    void forgetChangedMessageIds() { m_changedMessageIds.clear(); }
+
+private:
+    QStringList m_changedMessageIds;
+};
+
 /// Undo entry for a tag change over a set of threads.
 ///
-/// Stores thread ids rather than message ids, so undo re-resolves them on the
-/// worker and stays correct even if the selection has moved on.
-class ThreadTagCommand : public QUndoCommand
+/// Stores thread ids rather than message ids, so a REDO re-resolves them on
+/// the worker and stays correct even if the selection has moved on. The UNDO
+/// does not: it covers the messages the write actually changed, which the
+/// worker reports, because inverting over the whole thread rewrites messages
+/// the user never touched (item 176).
+class ThreadTagCommand : public TagCommand
 {
 public:
     ThreadTagCommand(MainWindow *window, const QStringList &threadIds,
                      const QStringList &add, const QStringList &remove,
                      const QString &description)
-        : QUndoCommand(description), m_window(window), m_threadIds(threadIds),
+        : TagCommand(description), m_window(window), m_threadIds(threadIds),
           m_add(add), m_remove(remove), m_description(description) {}
 
     /// The stack calls redo() when the command is pushed. The change has
@@ -1724,15 +1810,28 @@ public:
             m_firstRedo = false;
             return;
         }
+        forgetChangedMessageIds();
+        m_window->awaitTagConfirmation(this);
         m_window->sendThreadTagChange(m_threadIds, m_add, m_remove,
                                       m_description);
     }
 
     void undo() override
     {
-        // Inverted: what was added is removed and vice versa.
-        m_window->sendThreadTagChange(m_threadIds, m_remove, m_add,
-                                      QStringLiteral("Undo %1").arg(m_description));
+        // Inverted: what was added is removed and vice versa. The WRITE is
+        // scoped to the messages that moved rather than to the threads, which
+        // is item 176; the repaint stays thread-wide, since a conversation row
+        // is what is being put back.
+        //
+        // Nothing confirmed means nothing reached the database: the write was
+        // held for a sync, or the worker found nothing to change. Undoing it
+        // would write tags no message ever had, which is the defect itself.
+        const QStringList ids = changedMessageIds();
+        if (ids.isEmpty())
+            return;
+        m_window->sendThreadTagChange(
+            m_threadIds, m_remove, m_add,
+            QStringLiteral("Undo %1").arg(m_description), ids);
     }
 
 private:
@@ -1750,13 +1849,13 @@ private:
 /// point rather than an inconsistency: a message row acts on one message, so
 /// re-resolving its thread on undo would restore tags across every sibling the
 /// action never touched.
-class MessageTagCommand : public QUndoCommand
+class MessageTagCommand : public TagCommand
 {
 public:
     MessageTagCommand(MainWindow *window, const QStringList &messageIds,
                       const QStringList &add, const QStringList &remove,
                       const QString &description)
-        : QUndoCommand(description), m_window(window),
+        : TagCommand(description), m_window(window),
           m_messageIds(messageIds), m_add(add), m_remove(remove),
           m_description(description) {}
 
@@ -1768,14 +1867,22 @@ public:
             m_firstRedo = false;
             return;
         }
+        forgetChangedMessageIds();
+        m_window->awaitTagConfirmation(this);
         m_window->sendMessageTagChange(m_messageIds, m_add, m_remove,
                                        m_description);
     }
 
+    /// Scoped to what the write MOVED, for the reason TagCommand gives.
+    /// Harmless on one message, where asked and changed agree, and the same
+    /// defect as the thread case on a multi-row selection (item 176).
     void undo() override
     {
+        const QStringList ids = changedMessageIds();
+        if (ids.isEmpty())
+            return;
         m_window->sendMessageTagChange(
-            m_messageIds, m_remove, m_add,
+            ids, m_remove, m_add,
             QStringLiteral("Undo %1").arg(m_description));
     }
 
diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp
index 9fb3e4d..75a4483 100644
--- a/src/notmuchworker.cpp
+++ b/src/notmuchworker.cpp
@@ -811,6 +811,16 @@ void NotmuchWorker::applyTags(const TagChange &change)
         return;
     }
 
+    // Item 176. What the caller ASKED for is not what the write does: a
+    // "remove unread" over a whole thread touches only the messages that
+    // carried it. The undo entry is built from this list, so reporting the
+    // request instead made the inverse of "mark 44 read" into "mark 44
+    // unread", measured on real mail as 2 unread becoming 43. With
+    // maildir.synchronize_flags on, that rewrites filenames and reaches the
+    // server.
+    QStringList changedIds;
+    changedIds.reserve(change.messageIds.size());
+
     for (const QString &id : change.messageIds) {
         notmuch_message_t *raw = nullptr;
         // find_message reports SUCCESS with a null message when the id is not
@@ -822,6 +832,20 @@ void NotmuchWorker::applyTags(const TagChange &change)
         }
         NmMessage message(raw);
 
+        // Read BEFORE the freeze, so the comparison is against the message as
+        // it stands. A tag already present that is being added, or absent and
+        // being removed, is a no-op notmuch reports no differently from a real
+        // write, so the only way to know is to look first.
+        const QStringList before = tagsOf(message.get());
+
+        bool moves = false;
+        for (const QString &tag : change.removed)
+            moves = moves || before.contains(tag);
+        for (const QString &tag : change.added)
+            moves = moves || !before.contains(tag);
+        if (moves)
+            changedIds.append(id);
+
         notmuch_message_freeze(message.get());
         for (const QString &tag : change.removed)
             notmuch_message_remove_tag(message.get(), tag.toUtf8().constData());
@@ -837,7 +861,16 @@ void NotmuchWorker::applyTags(const TagChange &change)
     notmuch_database_close(db);
     notmuch_database_destroy(db);
 
-    emit tagsApplied(change);
+    // Nothing moved, so there is nothing to record as pending, nothing to sync
+    // and nothing to undo. Emitting an empty change would push an undo entry
+    // whose inverse adds a tag no message ever carried, which is item 176 one
+    // step further along. The same silence as the empty-id early return above,
+    // for the same reason.
+    if (changedIds.isEmpty())
+        return;
+
+    emit tagsApplied(TagChange{ changedIds, change.added, change.removed,
+                                change.description });
 }
 
 void NotmuchWorker::moveMessages(const QStringList &messageIds,