From d52c36e920ae731f55b7966d4fd7465199713362 Mon Sep 17 00:00:00 2001
From: Damien Elmes <gpg@ankiweb.net>
Date: Fri, 25 Feb 2022 13:39:23 +1000
Subject: [PATCH] Make flag changes undoable again

The previous change in 1871b5766366e4faa3f828c038aab94c42d7863d failed
to consider the browser refreshing case, as reported here:
https://forums.ankiweb.net/t/anki-2-1-50-beta-3-4/17501/30

I previously attempted to solve this by having SetFlag skip the queue
rebuild, then mutating the captured mtimes in the queues. That didn't
work correctly when undoing, as the queue mutations weren't recorded.
This approach combines that attempt and the previous change: flag
setting is an undoable operation again, but does not change the card's
modification time, so it can be applied/undone without a queue build
being required. Instead of special-casing flag changes in the review
screen, we now just redraw the flag on changes.card, as any other card
op will have triggered a queue rebuild.
---
 qt/aqt/reviewer.py    | 20 ++++++++++++++------
 rslib/src/card/mod.rs |  7 +++++--
 rslib/src/ops.rs      |  4 +++-
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/qt/aqt/reviewer.py b/qt/aqt/reviewer.py
index 173545cd2..8910913eb 100644
--- a/qt/aqt/reviewer.py
+++ b/qt/aqt/reviewer.py
@@ -22,6 +22,7 @@ from anki.collection import Config, OpChanges, OpChangesWithCount
 from anki.scheduler.v3 import CardAnswer, NextStates, QueuedCards
 from anki.scheduler.v3 import Scheduler as V3Scheduler
 from anki.tags import MARKED_TAG
+from anki.types import assert_exhaustive
 from anki.utils import strip_html
 from aqt import AnkiQt, gui_hooks
 from aqt.browser.card_info import PreviousReviewerCardInfo, ReviewerCardInfo
@@ -49,6 +50,7 @@ from aqt.utils import askUserDialog, downArrow, qtMenuShortcutWorkaround, toolti
 class RefreshNeeded(Enum):
     NOTE_TEXT = auto()
     QUEUES = auto()
+    FLAG = auto()
 
 
 class ReviewerBottomBar:
@@ -171,6 +173,14 @@ class Reviewer:
             self._redraw_current_card()
             self.mw.fade_in_webview()
             self._refresh_needed = None
+        elif self._refresh_needed is RefreshNeeded.FLAG:
+            self.card.load()
+            self._update_flag_icon()
+            # for when modified in browser
+            self.mw.fade_in_webview()
+            self._refresh_needed = None
+        elif self._refresh_needed:
+            assert_exhaustive(self._refresh_needed)
 
     def op_executed(
         self, changes: OpChanges, handler: object | None, focused: bool
@@ -180,6 +190,8 @@ class Reviewer:
                 self._refresh_needed = RefreshNeeded.QUEUES
             elif changes.note_text:
                 self._refresh_needed = RefreshNeeded.NOTE_TEXT
+            elif changes.card:
+                self._refresh_needed = RefreshNeeded.FLAG
 
         if focused and self._refresh_needed:
             self.refresh_if_needed()
@@ -991,10 +1003,6 @@ time = %(time)d;
         self._card_info.toggle()
 
     def set_flag_on_current_card(self, desired_flag: int) -> None:
-        def redraw_flag(out: OpChangesWithCount) -> None:
-            self.card.load()
-            self._update_flag_icon()
-
         # need to toggle off?
         if self.card.user_flag() == desired_flag:
             flag = 0
@@ -1002,8 +1010,8 @@ time = %(time)d;
             flag = desired_flag
 
         set_card_flag(parent=self.mw, card_ids=[self.card.id], flag=flag).success(
-            redraw_flag
-        ).run_in_background(initiator=self)
+            lambda _: None
+        ).run_in_background()
 
     def set_flag_func(self, desired_flag: int) -> Callable:
         return lambda: self.set_flag_on_current_card(desired_flag)
diff --git a/rslib/src/card/mod.rs b/rslib/src/card/mod.rs
index 7fc769165..258ea8285 100644
--- a/rslib/src/card/mod.rs
+++ b/rslib/src/card/mod.rs
@@ -279,12 +279,15 @@ impl Collection {
 
         self.storage.set_search_table_to_card_ids(cards, false)?;
         let usn = self.usn()?;
-        self.transact(Op::SkipUndo, |col| {
+        self.transact(Op::SetFlag, |col| {
             let mut count = 0;
             for mut card in col.storage.all_searched_cards()? {
+                let original = card.clone();
                 if card.set_flag(flag) {
+                    // To avoid having to rebuild the study queues, we mark the card as requiring
+                    // a sync, but do not change its modification time.
                     card.usn = usn;
-                    col.storage.update_card(&card)?;
+                    col.update_card_undoable(&mut card, original)?;
                     count += 1;
                 }
             }
diff --git a/rslib/src/ops.rs b/rslib/src/ops.rs
index 734df8412..2a2ecaf66 100644
--- a/rslib/src/ops.rs
+++ b/rslib/src/ops.rs
@@ -29,6 +29,7 @@ pub enum Op {
     ScheduleAsNew,
     SetCardDeck,
     SetDueDate,
+    SetFlag,
     SortCards,
     Suspend,
     UnburyUnsuspend,
@@ -67,6 +68,7 @@ impl Op {
             Op::UpdatePreferences => tr.preferences_preferences(),
             Op::UpdateTag => tr.actions_update_tag(),
             Op::SetCardDeck => tr.browsing_change_deck(),
+            Op::SetFlag => tr.actions_set_flag(),
             Op::FindAndReplace => tr.browsing_find_and_replace(),
             Op::ClearUnusedTags => tr.browsing_clear_unused_tags(),
             Op::SortCards => tr.browsing_reschedule(),
@@ -154,7 +156,7 @@ impl OpChanges {
 
     pub fn requires_study_queue_rebuild(&self) -> bool {
         let c = &self.changes;
-        c.card
+        (c.card && self.op != Op::SetFlag)
             || c.deck
             || (c.config && matches!(self.op, Op::SetCurrentDeck | Op::UpdatePreferences))
             || c.deck_config