use perform_op() for undo()

Instead of manually updating the UI after undoing, we just rely
on the same change notification infrastructure regular operations
use.

pylib/anki/collection.py

@@ -18,6 +18,7 @@ UndoStatus = _pb.UndoStatus
 OpChanges = _pb.OpChanges
 OpChangesWithCount = _pb.OpChangesWithCount
 OpChangesWithId = _pb.OpChangesWithId
+OpChangesAfterUndo = _pb.OpChangesAfterUndo
 DefaultsForAdding = _pb.DeckAndNotetype
 BrowserRow = _pb.BrowserRow
 
@@ -66,22 +67,17 @@ SearchJoiner = Literal["AND", "OR"]
 
 
 @dataclass
-class ReviewUndo:
+class LegacyReviewUndo:
     card: Card
     was_leech: bool
 
 
 @dataclass
-class Checkpoint:
+class LegacyCheckpoint:
     name: str
 
 
-@dataclass
+LegacyUndoResult = Union[None, LegacyCheckpoint, LegacyReviewUndo]
-class BackendUndo:
-    name: str
-
-
-UndoResult = Union[None, BackendUndo, Checkpoint, ReviewUndo]
 
 
 class Collection:
@@ -835,7 +831,7 @@ table.review-log {{ {revlog_style} }}
 
         if isinstance(self._undo, _ReviewsUndo):
             return UndoStatus(undo=self.tr.scheduling_review())
-        elif isinstance(self._undo, Checkpoint):
+        elif isinstance(self._undo, LegacyCheckpoint):
             return UndoStatus(undo=self._undo.name)
         else:
             assert_exhaustive(self._undo)
@@ -848,19 +844,18 @@ table.review-log {{ {revlog_style} }}
         is run."""
         self._undo = None
 
-    def undo(self) -> UndoResult:
+    def undo(self) -> OpChangesAfterUndo:
-        """Returns ReviewUndo if undoing a v1/v2 scheduler review.
+        """Returns result of backend undo operation, or throws UndoEmpty.
-        Returns None if the undo queue was empty."""
+        If UndoEmpty is received, caller should try undo_legacy()."""
-        # backend?
+        out = self._backend.undo()
-        status = self._backend.get_undo_status()
-        if status.undo:
-            self._backend.undo()
         self.clear_python_undo()
-            return BackendUndo(name=status.undo)
+        return out
 
+    def undo_legacy(self) -> LegacyUndoResult:
+        "Returns None if the legacy undo queue is empty."
         if isinstance(self._undo, _ReviewsUndo):
             return self._undo_review()
-        elif isinstance(self._undo, Checkpoint):
+        elif isinstance(self._undo, LegacyCheckpoint):
             return self._undo_checkpoint()
         elif self._undo is None:
             return None
@@ -896,15 +891,15 @@ table.review-log {{ {revlog_style} }}
             self._undo = _ReviewsUndo()
 
         was_leech = card.note().has_tag("leech")
-        entry = ReviewUndo(card=copy.copy(card), was_leech=was_leech)
+        entry = LegacyReviewUndo(card=copy.copy(card), was_leech=was_leech)
         self._undo.entries.append(entry)
 
     def _have_outstanding_checkpoint(self) -> bool:
         self._check_backend_undo_status()
-        return isinstance(self._undo, Checkpoint)
+        return isinstance(self._undo, LegacyCheckpoint)
 
-    def _undo_checkpoint(self) -> Checkpoint:
+    def _undo_checkpoint(self) -> LegacyCheckpoint:
-        assert isinstance(self._undo, Checkpoint)
+        assert isinstance(self._undo, LegacyCheckpoint)
         self.rollback()
         undo = self._undo
         self.clear_python_undo()
@@ -914,13 +909,13 @@ table.review-log {{ {revlog_style} }}
         "Call via .save(). If name not provided, clear any existing checkpoint."
         self._last_checkpoint_at = time.time()
         if name:
-            self._undo = Checkpoint(name=name)
+            self._undo = LegacyCheckpoint(name=name)
         else:
             # saving disables old checkpoint, but not review undo
             if not isinstance(self._undo, _ReviewsUndo):
                 self.clear_python_undo()
 
-    def _undo_review(self) -> ReviewUndo:
+    def _undo_review(self) -> LegacyReviewUndo:
         "Undo a v1/v2 review."
         assert isinstance(self._undo, _ReviewsUndo)
         entry = self._undo.entries.pop()
@@ -1101,10 +1096,10 @@ _Collection = Collection
 
 @dataclass
 class _ReviewsUndo:
-    entries: List[ReviewUndo] = field(default_factory=list)
+    entries: List[LegacyReviewUndo] = field(default_factory=list)
 
 
-_UndoInfo = Union[_ReviewsUndo, Checkpoint, None]
+_UndoInfo = Union[_ReviewsUndo, LegacyCheckpoint, None]
 
 
 def _build_sort_mode(

pylib/anki/errors.py

@@ -7,7 +7,7 @@ from markdown import markdown
 
 import anki._backend.backend_pb2 as _pb
 
-# fixme: notfounderror etc need to be in rsbackend.py
+from anki.types import assert_exhaustive
 
 
 class StringError(Exception):
@@ -49,6 +49,10 @@ class ExistsError(Exception):
     pass
 
 
+class UndoEmpty(Exception):
+    pass
+
+
 class DeckRenameError(Exception):
     """Legacy error, use FilteredDeckError instead."""
 
@@ -97,8 +101,10 @@ def backend_exception_to_pylib(err: _pb.BackendError) -> Exception:
         return StringError(err.localized)
     elif val == "search_error":
         return SearchError(markdown(err.localized))
+    elif val == "undo_empty":
+        return UndoEmpty()
     else:
-        print("unhandled error type:", val)
+        assert_exhaustive(val)
         return StringError(err.localized)
 
 

pylib/tests/test_schedv1.py

@@ -270,7 +270,7 @@ def test_learn_day():
     # if we fail it, it should be back in the correct queue
     col.sched.answerCard(c, 1)
     assert c.queue == QUEUE_TYPE_LRN
-    col.undo()
+    col.undo_legacy()
     col.reset()
     c = col.sched.getCard()
     col.sched.answerCard(c, 2)

pylib/tests/test_schedv2.py

@@ -327,7 +327,10 @@ def test_learn_day():
     # if we fail it, it should be back in the correct queue
     col.sched.answerCard(c, 1)
     assert c.queue == QUEUE_TYPE_LRN
+    if is_2021():
         col.undo()
+    else:
+        col.undo_legacy()
     col.reset()
     c = col.sched.getCard()
     col.sched.answerCard(c, 3)

pylib/tests/test_undo.py

@@ -24,7 +24,7 @@ def test_op():
     # with about 5 minutes until it's clobbered
     assert time.time() - col._last_checkpoint_at < 1
     # undoing should restore the old value
-    col.undo()
+    col.undo_legacy()
     assert not col.undoName()
     assert "abc" not in col.conf
     # an (auto)save will clear the undo
@@ -64,7 +64,7 @@ def test_review():
     assert c.queue == QUEUE_TYPE_LRN
     # undo
     assert col.undoName()
-    col.undo()
+    col.undo_legacy()
     col.reset()
     assert col.sched.counts() == (2, 0, 0)
     c.load()
@@ -77,10 +77,10 @@ def test_review():
     c = col.sched.getCard()
     col.sched.answerCard(c, 3)
     assert col.sched.counts() == (0, 2, 0)
-    col.undo()
+    col.undo_legacy()
     col.reset()
     assert col.sched.counts() == (1, 1, 0)
-    col.undo()
+    col.undo_legacy()
     col.reset()
     assert col.sched.counts() == (2, 0, 0)
     # performing a normal op will clear the review queue
@@ -89,5 +89,5 @@ def test_review():
     assert col.undoName() == "Review"
     col.save("foo")
     assert col.undoName() == "foo"
-    col.undo()
+    col.undo_legacy()
     assert not col.undoName()

qt/aqt/browser.py

@@ -21,6 +21,7 @@ from anki.tags import MARKED_TAG
 from anki.utils import ids2str, isMac
 from aqt import AnkiQt, gui_hooks
 from aqt.card_ops import set_card_deck, set_card_flag
+from aqt.collection_ops import undo
 from aqt.editor import Editor
 from aqt.exporting import ExportDialog
 from aqt.find_and_replace import FindAndReplaceDialog
@@ -861,11 +862,7 @@ where id in %s"""
     ######################################################################
 
     def undo(self) -> None:
-        # need to make sure we don't hang the UI by redrawing the card list
+        undo(mw=self.mw, parent=self)
-        # during the long-running op. mw.undo will take care of the progress
-        # dialog
-        self.setUpdatesEnabled(False)
-        self.mw.undo(lambda _: self.setUpdatesEnabled(True))
 
     def onUndoState(self, on: bool) -> None:
         self.form.actionUndo.setEnabled(on)

qt/aqt/collection_ops.py

@@ -0,0 +1,73 @@
+# Copyright: Ankitects Pty Ltd and contributors
+# License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
+
+from __future__ import annotations
+
+import aqt
+from anki.collection import LegacyCheckpoint, LegacyReviewUndo, OpChangesAfterUndo
+from anki.errors import UndoEmpty
+from anki.types import assert_exhaustive
+from aqt import gui_hooks
+from aqt.qt import QWidget
+from aqt.utils import showInfo, showWarning, tooltip, tr
+
+
+def undo(*, mw: aqt.AnkiQt, parent: QWidget) -> None:
+    "Undo the last operation, and refresh the UI."
+
+    def on_success(out: OpChangesAfterUndo) -> None:
+        mw.update_undo_actions(out.new_status)
+        tooltip(tr.undo_action_undone(action=out.operation), parent=parent)
+
+    def on_failure(exc: Exception) -> None:
+        if isinstance(exc, UndoEmpty):
+            # backend has no undo, but there may be a checkpoint
+            # or v1/v2 review waiting
+            _legacy_undo(mw=mw, parent=parent)
+        else:
+            showWarning(str(exc), parent=parent)
+
+    mw.perform_op(mw.col.undo, success=on_success, failure=on_failure)
+
+
+def _legacy_undo(*, mw: aqt.AnkiQt, parent: QWidget) -> None:
+    reviewing = mw.state == "review"
+    just_refresh_reviewer = False
+
+    result = mw.col.undo_legacy()
+
+    if result is None:
+        # should not happen
+        showInfo("nothing to undo", parent=parent)
+        mw.update_undo_actions()
+        return
+
+    elif isinstance(result, LegacyReviewUndo):
+        name = tr.scheduling_review()
+
+        if reviewing:
+            # push the undone card to the top of the queue
+            cid = result.card.id
+            card = mw.col.getCard(cid)
+            mw.reviewer.cardQueue.append(card)
+
+            gui_hooks.review_did_undo(cid)
+
+            just_refresh_reviewer = True
+
+    elif isinstance(result, LegacyCheckpoint):
+        name = result.name
+
+    else:
+        assert_exhaustive(result)
+        assert False
+
+    if just_refresh_reviewer:
+        mw.reviewer.nextCard()
+    else:
+        # full queue+gui reset required
+        mw.reset()
+
+    tooltip(tr.undo_action_undone(action=name), parent=parent)
+    gui_hooks.state_did_revert(name)
+    mw.update_undo_actions()

qt/aqt/main.py

@@ -41,25 +41,22 @@ import aqt.webview
 from anki import hooks
 from anki._backend import RustBackend as _RustBackend
 from anki.collection import (
-    BackendUndo,
-    Checkpoint,
     Collection,
     Config,
     OpChanges,
+    OpChangesAfterUndo,
     OpChangesWithCount,
     OpChangesWithId,
-    ReviewUndo,
-    UndoResult,
     UndoStatus,
 )
 from anki.decks import DeckDict, DeckId
 from anki.hooks import runHook
 from anki.notes import NoteId
 from anki.sound import AVTag, SoundOrVideoTag
-from anki.types import assert_exhaustive
 from anki.utils import devMode, ids2str, intTime, isMac, isWin, splitFields
 from aqt import gui_hooks
 from aqt.addons import DownloadLogEntry, check_and_prompt_for_updates, show_log_to_user
+from aqt.collection_ops import undo
 from aqt.dbcheck import check_db
 from aqt.emptycards import show_empty_cards
 from aqt.legacy import install_pylib_legacy
@@ -104,7 +101,13 @@ class HasChangesProperty(Protocol):
 # either need to be added here, or cast at call time
 ResultWithChanges = TypeVar(
     "ResultWithChanges",
-    bound=Union[OpChanges, OpChangesWithCount, OpChangesWithId, HasChangesProperty],
+    bound=Union[
+        OpChanges,
+        OpChangesWithCount,
+        OpChangesWithId,
+        OpChangesAfterUndo,
+        HasChangesProperty,
+    ],
 )
 
 T = TypeVar("T")
@@ -1208,66 +1211,15 @@ title="%s" %s>%s</button>""" % (
     # Undo & autosave
     ##########################################################################
 
-    def undo(self, on_done: Optional[Callable[[UndoResult], None]]) -> None:
+    def undo(self) -> None:
-        def on_done_outer(fut: Future) -> None:
+        "Call collection_ops.py:undo() directly instead."
-            result = fut.result()
+        undo(mw=self, parent=self)
-            reviewing = self.state == "review"
-            just_refresh_reviewer = False
 
-            if result is None:
+    def update_undo_actions(self, status: Optional[UndoStatus] = None) -> None:
-                # should not happen
-                showInfo("nothing to undo")
-                self.update_undo_actions()
-                return
-
-            elif isinstance(result, ReviewUndo):
-                name = tr.scheduling_review()
-
-                if reviewing:
-                    # push the undone card to the top of the queue
-                    cid = result.card.id
-                    card = self.col.getCard(cid)
-                    self.reviewer.cardQueue.append(card)
-
-                    gui_hooks.review_did_undo(cid)
-
-                    just_refresh_reviewer = True
-
-            elif isinstance(result, BackendUndo):
-                name = result.name
-
-                if reviewing and self.col.sched.version == 3:
-                    # new scheduler will have taken care of updating queue
-                    just_refresh_reviewer = True
-
-            elif isinstance(result, Checkpoint):
-                name = result.name
-
-            else:
-                assert_exhaustive(result)
-                assert False
-
-            if just_refresh_reviewer:
-                self.reviewer.nextCard()
-            else:
-                # full queue+gui reset required
-                self.reset()
-
-            tooltip(tr.undo_action_undone(action=name))
-            gui_hooks.state_did_revert(name)
-            self.update_undo_actions()
-            if on_done:
-                on_done(result)
-
-        # fixme: perform_op? -> needs to save
-        # fixme: parent
-        self.taskman.with_progress(self.col.undo, on_done_outer)
-
-    def update_undo_actions(self) -> None:
         """Update menu text and enable/disable menu item as appropriate.
         Plural as this may handle redo in the future too."""
         if self.col:
-            status = self.col.undo_status()
+            status = status or self.col.undo_status()
             undo_action = status.undo or None
         else:
             undo_action = None

rslib/backend.proto

@@ -274,8 +274,8 @@ service CollectionService {
   rpc CloseCollection(CloseCollectionIn) returns (Empty);
   rpc CheckDatabase(Empty) returns (CheckDatabaseOut);
   rpc GetUndoStatus(Empty) returns (UndoStatus);
-  rpc Undo(Empty) returns (UndoStatus);
+  rpc Undo(Empty) returns (OpChangesAfterUndo);
-  rpc Redo(Empty) returns (UndoStatus);
+  rpc Redo(Empty) returns (OpChangesAfterUndo);
   rpc LatestProgress(Empty) returns (Progress);
   rpc SetWantsAbort(Empty) returns (Empty);
 }
@@ -571,6 +571,7 @@ message BackendError {
     Empty exists = 12;
     Empty filtered_deck_error = 13;
     Empty search_error = 14;
+    Empty undo_empty = 15;
   }
 }
 
@@ -1522,6 +1523,13 @@ message UndoStatus {
   string redo = 2;
 }
 
+message OpChangesAfterUndo {
+  OpChanges changes = 1;
+  string operation = 2;
+  int64 reverted_to_timestamp = 3;
+  UndoStatus new_status = 4;
+}
+
 message DefaultsForAddingIn {
   int64 home_deck_of_current_review_card = 1;
 }

rslib/src/backend/collection.rs

@@ -88,17 +88,11 @@ impl CollectionService for Backend {
         self.with_col(|col| Ok(col.undo_status().into_protobuf(&col.tr)))
     }
 
-    fn undo(&self, _input: pb::Empty) -> Result<pb::UndoStatus> {
+    fn undo(&self, _input: pb::Empty) -> Result<pb::OpChangesAfterUndo> {
-        self.with_col(|col| {
+        self.with_col(|col| col.undo().map(|out| out.into_protobuf(&col.tr)))
-            col.undo()?;
-            Ok(col.undo_status().into_protobuf(&col.tr))
-        })
     }
 
-    fn redo(&self, _input: pb::Empty) -> Result<pb::UndoStatus> {
+    fn redo(&self, _input: pb::Empty) -> Result<pb::OpChangesAfterUndo> {
-        self.with_col(|col| {
+        self.with_col(|col| col.redo().map(|out| out.into_protobuf(&col.tr)))
-            col.redo()?;
-            Ok(col.undo_status().into_protobuf(&col.tr))
-        })
     }
 }

rslib/src/backend/err.rs

@@ -34,6 +34,7 @@ pub(super) fn anki_error_to_proto_error(err: AnkiError, tr: &I18n) -> pb::Backen
         AnkiError::TemplateSaveError { .. } => V::TemplateParse(pb::Empty {}),
         AnkiError::ParseNumError => V::InvalidInput(pb::Empty {}),
         AnkiError::InvalidRegex(_) => V::InvalidInput(pb::Empty {}),
+        AnkiError::UndoEmpty => V::UndoEmpty(pb::Empty {}),
     };
 
     pb::BackendError {

rslib/src/backend/ops.rs

@@ -3,7 +3,12 @@
 
 use pb::op_changes::Kind;
 
-use crate::{backend_proto as pb, ops::OpChanges, prelude::*, undo::UndoStatus};
+use crate::{
+    backend_proto as pb,
+    ops::OpChanges,
+    prelude::*,
+    undo::{UndoOutput, UndoStatus},
+};
 
 impl From<Op> for Kind {
     fn from(o: Op) -> Self {
@@ -62,3 +67,14 @@ impl From<OpOutput<i64>> for pb::OpChangesWithId {
         }
     }
 }
+
+impl OpOutput<UndoOutput> {
+    pub(crate) fn into_protobuf(self, tr: &I18n) -> pb::OpChangesAfterUndo {
+        pb::OpChangesAfterUndo {
+            changes: Some(self.changes.into()),
+            operation: self.output.undone_op.describe(tr),
+            reverted_to_timestamp: self.output.reverted_to.0,
+            new_status: Some(self.output.new_undo_status.into_protobuf(tr)),
+        }
+    }
+}

rslib/src/error/mod.rs

@@ -39,6 +39,7 @@ pub enum AnkiError {
     FilteredDeckError(FilteredDeckError),
     SearchError(SearchErrorKind),
     InvalidRegex(String),
+    UndoEmpty,
 }
 
 impl Display for AnkiError {

rslib/src/undo/mod.rs

@@ -39,6 +39,12 @@ pub struct UndoStatus {
     pub redo: Option<Op>,
 }
 
+pub struct UndoOutput {
+    pub undone_op: Op,
+    pub reverted_to: TimestampSecs,
+    pub new_undo_status: UndoStatus,
+}
+
 #[derive(Debug, Default)]
 pub(crate) struct UndoManager {
     // undo steps are added to the front of a double-ended queue, so we can
@@ -140,37 +146,18 @@ impl Collection {
         self.state.undo.can_redo()
     }
 
-    pub fn undo(&mut self) -> Result<OpOutput<()>> {
+    pub fn undo(&mut self) -> Result<OpOutput<UndoOutput>> {
         if let Some(step) = self.state.undo.undo_steps.pop_front() {
-            let changes = step.changes;
+            self.undo_inner(step, UndoMode::Undoing)
-            self.state.undo.mode = UndoMode::Undoing;
-            let res = self.transact(step.kind, |col| {
-                for change in changes.into_iter().rev() {
-                    change.undo(col)?;
-                }
-                Ok(())
-            });
-            self.state.undo.mode = UndoMode::NormalOp;
-            res
         } else {
-            Err(AnkiError::invalid_input("no undo available"))
+            Err(AnkiError::UndoEmpty)
         }
     }
-
+    pub fn redo(&mut self) -> Result<OpOutput<UndoOutput>> {
-    pub fn redo(&mut self) -> Result<OpOutput<()>> {
         if let Some(step) = self.state.undo.redo_steps.pop() {
-            let changes = step.changes;
+            self.undo_inner(step, UndoMode::Redoing)
-            self.state.undo.mode = UndoMode::Redoing;
-            let res = self.transact(step.kind, |col| {
-                for change in changes.into_iter().rev() {
-                    change.undo(col)?;
-                }
-                Ok(())
-            });
-            self.state.undo.mode = UndoMode::NormalOp;
-            res
         } else {
-            Err(AnkiError::invalid_input("no redo available"))
+            Err(AnkiError::UndoEmpty)
         }
     }
 
@@ -233,6 +220,27 @@ impl Collection {
     }
 }
 
+impl Collection {
+    fn undo_inner(&mut self, step: UndoableOp, mode: UndoMode) -> Result<OpOutput<UndoOutput>> {
+        let undone_op = step.kind;
+        let reverted_to = step.timestamp;
+        let changes = step.changes;
+        self.state.undo.mode = mode;
+        let res = self.transact(step.kind, |col| {
+            for change in changes.into_iter().rev() {
+                change.undo(col)?;
+            }
+            Ok(UndoOutput {
+                undone_op,
+                reverted_to,
+                new_undo_status: col.undo_status(),
+            })
+        });
+        self.state.undo.mode = UndoMode::NormalOp;
+        res
+    }
+}
+
 #[cfg(test)]
 mod test {
     use crate::card::Card;