From f666f15b63cbdd51f58727b4b733a1a1d0ef6695 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Sat, 3 Apr 2021 14:38:49 +1000 Subject: [PATCH] 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 | 49 ++++++++++----------- pylib/anki/errors.py | 10 ++++- pylib/tests/test_schedv1.py | 2 +- pylib/tests/test_schedv2.py | 5 ++- pylib/tests/test_undo.py | 10 ++--- qt/aqt/browser.py | 7 +-- qt/aqt/collection_ops.py | 73 +++++++++++++++++++++++++++++++ qt/aqt/main.py | 76 ++++++--------------------------- rslib/backend.proto | 12 +++++- rslib/src/backend/collection.rs | 14 ++---- rslib/src/backend/err.rs | 1 + rslib/src/backend/ops.rs | 18 +++++++- rslib/src/error/mod.rs | 1 + rslib/src/undo/mod.rs | 58 ++++++++++++++----------- 14 files changed, 195 insertions(+), 141 deletions(-) create mode 100644 qt/aqt/collection_ops.py diff --git a/pylib/anki/collection.py b/pylib/anki/collection.py index 2f3c1e6ce..9a772340e 100644 --- a/pylib/anki/collection.py +++ b/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 -class BackendUndo: - name: str - - -UndoResult = Union[None, BackendUndo, Checkpoint, ReviewUndo] +LegacyUndoResult = Union[None, LegacyCheckpoint, LegacyReviewUndo] 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: - """Returns ReviewUndo if undoing a v1/v2 scheduler review. - Returns None if the undo queue was empty.""" - # backend? - status = self._backend.get_undo_status() - if status.undo: - self._backend.undo() - self.clear_python_undo() - return BackendUndo(name=status.undo) + def undo(self) -> OpChangesAfterUndo: + """Returns result of backend undo operation, or throws UndoEmpty. + If UndoEmpty is received, caller should try undo_legacy().""" + out = self._backend.undo() + self.clear_python_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: - assert isinstance(self._undo, Checkpoint) + def _undo_checkpoint(self) -> LegacyCheckpoint: + 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( diff --git a/pylib/anki/errors.py b/pylib/anki/errors.py index 0ebcba870..5178d7643 100644 --- a/pylib/anki/errors.py +++ b/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) diff --git a/pylib/tests/test_schedv1.py b/pylib/tests/test_schedv1.py index 4a02066da..ac21d7fa7 100644 --- a/pylib/tests/test_schedv1.py +++ b/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) diff --git a/pylib/tests/test_schedv2.py b/pylib/tests/test_schedv2.py index 5f9a46aae..be7518f10 100644 --- a/pylib/tests/test_schedv2.py +++ b/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 - col.undo() + if is_2021(): + col.undo() + else: + col.undo_legacy() col.reset() c = col.sched.getCard() col.sched.answerCard(c, 3) diff --git a/pylib/tests/test_undo.py b/pylib/tests/test_undo.py index 7bfaa2381..094c198df 100644 --- a/pylib/tests/test_undo.py +++ b/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() diff --git a/qt/aqt/browser.py b/qt/aqt/browser.py index 67ebc244c..f5dec950d 100644 --- a/qt/aqt/browser.py +++ b/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 - # during the long-running op. mw.undo will take care of the progress - # dialog - self.setUpdatesEnabled(False) - self.mw.undo(lambda _: self.setUpdatesEnabled(True)) + undo(mw=self.mw, parent=self) def onUndoState(self, on: bool) -> None: self.form.actionUndo.setEnabled(on) diff --git a/qt/aqt/collection_ops.py b/qt/aqt/collection_ops.py new file mode 100644 index 000000000..95923c055 --- /dev/null +++ b/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() diff --git a/qt/aqt/main.py b/qt/aqt/main.py index a0f394c57..29d7321d6 100644 --- a/qt/aqt/main.py +++ b/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""" % ( # Undo & autosave ########################################################################## - def undo(self, on_done: Optional[Callable[[UndoResult], None]]) -> None: - def on_done_outer(fut: Future) -> None: - result = fut.result() - reviewing = self.state == "review" - just_refresh_reviewer = False + def undo(self) -> None: + "Call collection_ops.py:undo() directly instead." + undo(mw=self, parent=self) - if result is 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: + def update_undo_actions(self, status: Optional[UndoStatus] = None) -> 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 diff --git a/rslib/backend.proto b/rslib/backend.proto index e88c7f3e8..693549261 100644 --- a/rslib/backend.proto +++ b/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 Redo(Empty) returns (UndoStatus); + rpc Undo(Empty) returns (OpChangesAfterUndo); + 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; } diff --git a/rslib/src/backend/collection.rs b/rslib/src/backend/collection.rs index aa5104e70..577954d83 100644 --- a/rslib/src/backend/collection.rs +++ b/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 { - self.with_col(|col| { - col.undo()?; - Ok(col.undo_status().into_protobuf(&col.tr)) - }) + fn undo(&self, _input: pb::Empty) -> Result { + self.with_col(|col| col.undo().map(|out| out.into_protobuf(&col.tr))) } - fn redo(&self, _input: pb::Empty) -> Result { - self.with_col(|col| { - col.redo()?; - Ok(col.undo_status().into_protobuf(&col.tr)) - }) + fn redo(&self, _input: pb::Empty) -> Result { + self.with_col(|col| col.redo().map(|out| out.into_protobuf(&col.tr))) } } diff --git a/rslib/src/backend/err.rs b/rslib/src/backend/err.rs index 5edf8ed15..b80498ae7 100644 --- a/rslib/src/backend/err.rs +++ b/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 { diff --git a/rslib/src/backend/ops.rs b/rslib/src/backend/ops.rs index 4977ebeb8..6ddad03a6 100644 --- a/rslib/src/backend/ops.rs +++ b/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 for Kind { fn from(o: Op) -> Self { @@ -62,3 +67,14 @@ impl From> for pb::OpChangesWithId { } } } + +impl OpOutput { + 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)), + } + } +} diff --git a/rslib/src/error/mod.rs b/rslib/src/error/mod.rs index 088fb489c..215572afb 100644 --- a/rslib/src/error/mod.rs +++ b/rslib/src/error/mod.rs @@ -39,6 +39,7 @@ pub enum AnkiError { FilteredDeckError(FilteredDeckError), SearchError(SearchErrorKind), InvalidRegex(String), + UndoEmpty, } impl Display for AnkiError { diff --git a/rslib/src/undo/mod.rs b/rslib/src/undo/mod.rs index a5757b1f6..51c0ae210 100644 --- a/rslib/src/undo/mod.rs +++ b/rslib/src/undo/mod.rs @@ -39,6 +39,12 @@ pub struct UndoStatus { pub redo: Option, } +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> { + pub fn undo(&mut self) -> Result> { if let Some(step) = self.state.undo.undo_steps.pop_front() { - let changes = step.changes; - 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 + self.undo_inner(step, UndoMode::Undoing) } else { - Err(AnkiError::invalid_input("no undo available")) + Err(AnkiError::UndoEmpty) } } - - pub fn redo(&mut self) -> Result> { + pub fn redo(&mut self) -> Result> { if let Some(step) = self.state.undo.redo_steps.pop() { - let changes = step.changes; - 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 + self.undo_inner(step, UndoMode::Redoing) } 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> { + 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;