From bf83715ee050fc417f74ca0c16540c9ae27b02e4 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Sun, 29 Mar 2020 17:52:16 +1000 Subject: [PATCH] initial work on undo support --- pylib/tests/test_schedv1.py | 2 +- rslib/src/backend/mod.rs | 10 ++- rslib/src/card.rs | 132 +++++++++++++++++++++++++++++++++++- rslib/src/collection.rs | 23 +++++-- rslib/src/lib.rs | 1 + rslib/src/storage/sqlite.rs | 5 -- rslib/src/undo.rs | 126 ++++++++++++++++++++++++++++++++++ 7 files changed, 286 insertions(+), 13 deletions(-) create mode 100644 rslib/src/undo.rs diff --git a/pylib/tests/test_schedv1.py b/pylib/tests/test_schedv1.py index d59264997..822ab9a12 100644 --- a/pylib/tests/test_schedv1.py +++ b/pylib/tests/test_schedv1.py @@ -681,7 +681,7 @@ def test_cram(): c.col = None c2 = copy.deepcopy(c) c2.col = c.col = d - c2.id = 123 + c2.id = 0 c2.ord = 1 c2.due = 325 c2.col = c.col diff --git a/rslib/src/backend/mod.rs b/rslib/src/backend/mod.rs index cab32fd88..0093a062c 100644 --- a/rslib/src/backend/mod.rs +++ b/rslib/src/backend/mod.rs @@ -635,7 +635,15 @@ impl Backend { fn update_card(&self, pbcard: pb::Card) -> Result<()> { let mut card = pbcard_to_native(pbcard)?; - self.with_col(|col| col.transact(None, |ctx| ctx.update_card(&mut card))) + self.with_col(|col| { + col.transact(None, |ctx| { + let orig = ctx + .storage + .get_card(card.id)? + .ok_or_else(|| AnkiError::invalid_input("missing card"))?; + ctx.update_card(&mut card, &orig) + }) + }) } fn add_card(&self, pbcard: pb::Card) -> Result { diff --git a/rslib/src/card.rs b/rslib/src/card.rs index d23f2532b..6a1533e2e 100644 --- a/rslib/src/card.rs +++ b/rslib/src/card.rs @@ -5,7 +5,7 @@ use crate::decks::DeckID; use crate::define_newtype; use crate::err::{AnkiError, Result}; use crate::notes::NoteID; -use crate::{collection::Collection, timestamp::TimestampSecs, types::Usn}; +use crate::{collection::Collection, timestamp::TimestampSecs, types::Usn, undo::Undoable}; use num_enum::TryFromPrimitive; use serde_repr::{Deserialize_repr, Serialize_repr}; @@ -86,11 +86,43 @@ impl Default for Card { } } +#[derive(Debug)] +pub(crate) struct UpdateCardUndo(Card); + +impl Undoable for UpdateCardUndo { + fn apply(&self, col: &mut crate::collection::Collection) -> Result<()> { + let current = col + .storage + .get_card(self.0.id)? + .ok_or_else(|| AnkiError::invalid_input("card disappeared"))?; + // when called here, update_card should be placing the original content into the redo queue + col.update_card(&mut self.0.clone(), ¤t) + } +} + impl Collection { - pub(crate) fn update_card(&mut self, card: &mut Card) -> Result<()> { + #[cfg(test)] + pub(crate) fn get_and_update_card(&mut self, cid: CardID, func: F) -> Result + where + F: FnOnce(&mut Card) -> Result, + { + let orig = self + .storage + .get_card(cid)? + .ok_or_else(|| AnkiError::invalid_input("no such card"))?; + let mut card = orig.clone(); + func(&mut card)?; + self.update_card(&mut card, &orig)?; + Ok(card) + } + + pub(crate) fn update_card(&mut self, card: &mut Card, original: &Card) -> Result<()> { if card.id.0 == 0 { return Err(AnkiError::invalid_input("card id not set")); } + self.state + .undo + .save_undoable(Box::new(UpdateCardUndo(original.clone()))); card.mtime = TimestampSecs::now(); card.usn = self.usn()?; self.storage.update_card(card) @@ -106,3 +138,99 @@ impl Collection { self.storage.add_card(card) } } + +#[cfg(test)] +mod test { + use super::Card; + use crate::collection::{open_test_collection, CollectionOp}; + + #[test] + fn undo() { + let mut col = open_test_collection(); + + let mut card = Card::default(); + card.ivl = 1; + col.add_card(&mut card).unwrap(); + let cid = card.id; + + assert_eq!(col.can_undo(), None); + assert_eq!(col.can_redo(), None); + + // outside of a transaction, no undo info recorded + let card = col + .get_and_update_card(cid, |card| { + card.ivl = 2; + Ok(()) + }) + .unwrap(); + assert_eq!(card.ivl, 2); + assert_eq!(col.can_undo(), None); + assert_eq!(col.can_redo(), None); + + // record a few undo steps + for i in 3..=4 { + col.transact(Some(CollectionOp::UpdateCard), |col| { + col.get_and_update_card(cid, |card| { + card.ivl = i; + Ok(()) + }) + .unwrap(); + Ok(()) + }) + .unwrap(); + } + + assert_eq!(col.storage.get_card(cid).unwrap().unwrap().ivl, 4); + assert_eq!(col.can_undo(), Some(CollectionOp::UpdateCard)); + assert_eq!(col.can_redo(), None); + + // undo a step + col.undo().unwrap(); + assert_eq!(col.storage.get_card(cid).unwrap().unwrap().ivl, 3); + assert_eq!(col.can_undo(), Some(CollectionOp::UpdateCard)); + assert_eq!(col.can_redo(), Some(CollectionOp::UpdateCard)); + + // and again + col.undo().unwrap(); + assert_eq!(col.storage.get_card(cid).unwrap().unwrap().ivl, 2); + assert_eq!(col.can_undo(), None); + assert_eq!(col.can_redo(), Some(CollectionOp::UpdateCard)); + + // redo a step + col.redo().unwrap(); + assert_eq!(col.storage.get_card(cid).unwrap().unwrap().ivl, 3); + assert_eq!(col.can_undo(), Some(CollectionOp::UpdateCard)); + assert_eq!(col.can_redo(), Some(CollectionOp::UpdateCard)); + + // and another + col.redo().unwrap(); + assert_eq!(col.storage.get_card(cid).unwrap().unwrap().ivl, 4); + assert_eq!(col.can_undo(), Some(CollectionOp::UpdateCard)); + assert_eq!(col.can_redo(), None); + + // and undo the redo + col.undo().unwrap(); + assert_eq!(col.storage.get_card(cid).unwrap().unwrap().ivl, 3); + assert_eq!(col.can_undo(), Some(CollectionOp::UpdateCard)); + assert_eq!(col.can_redo(), Some(CollectionOp::UpdateCard)); + + // if any action is performed, it should clear the redo queue + col.transact(Some(CollectionOp::UpdateCard), |col| { + col.get_and_update_card(cid, |card| { + card.ivl = 5; + Ok(()) + }) + .unwrap(); + Ok(()) + }) + .unwrap(); + assert_eq!(col.storage.get_card(cid).unwrap().unwrap().ivl, 5); + assert_eq!(col.can_undo(), Some(CollectionOp::UpdateCard)); + assert_eq!(col.can_redo(), None); + + // and any action that doesn't support undoing will clear both queues + col.transact(None, |_col| Ok(())).unwrap(); + assert_eq!(col.can_undo(), None); + assert_eq!(col.can_redo(), None); + } +} diff --git a/rslib/src/collection.rs b/rslib/src/collection.rs index 4f5c621e5..66c279dd9 100644 --- a/rslib/src/collection.rs +++ b/rslib/src/collection.rs @@ -6,7 +6,7 @@ use crate::i18n::I18n; use crate::log::Logger; use crate::timestamp::TimestampSecs; use crate::types::Usn; -use crate::{sched::cutoff::SchedTimingToday, storage::SqliteStorage}; +use crate::{sched::cutoff::SchedTimingToday, storage::SqliteStorage, undo::UndoManager}; use std::path::PathBuf; pub fn open_collection>( @@ -34,9 +34,17 @@ pub fn open_collection>( Ok(col) } +#[cfg(test)] +pub fn open_test_collection() -> Collection { + use crate::log; + let i18n = I18n::new(&[""], "", log::terminal()); + open_collection(":memory:", "", "", false, i18n, log::terminal()).unwrap() +} + #[derive(Debug, Default)] pub struct CollectionState { task_state: CollectionTaskState, + pub(crate) undo: UndoManager, timing_today: Option, } @@ -62,10 +70,13 @@ pub struct Collection { pub(crate) i18n: I18n, pub(crate) log: Logger, pub(crate) server: bool, - state: CollectionState, + pub(crate) state: CollectionState, } -pub(crate) enum CollectionOp {} +#[derive(Debug, Clone, PartialEq)] +pub enum CollectionOp { + UpdateCard, +} impl Collection { /// Execute the provided closure in a transaction, rolling back if @@ -75,19 +86,23 @@ impl Collection { F: FnOnce(&mut Collection) -> Result, { self.storage.begin_rust_trx()?; + self.state.undo.begin_step(op); let mut res = func(self); if res.is_ok() { if let Err(e) = self.storage.mark_modified() { res = Err(e); - } else if let Err(e) = self.storage.commit_rust_op(op) { + } else if let Err(e) = self.storage.commit_rust_trx() { res = Err(e); } } if res.is_err() { + self.state.undo.discard_step(); self.storage.rollback_rust_trx()?; + } else { + self.state.undo.end_step(); } res diff --git a/rslib/src/lib.rs b/rslib/src/lib.rs index 87253f998..aa2de288b 100644 --- a/rslib/src/lib.rs +++ b/rslib/src/lib.rs @@ -30,3 +30,4 @@ pub mod template_filters; pub mod text; pub mod timestamp; pub mod types; +pub mod undo; diff --git a/rslib/src/storage/sqlite.rs b/rslib/src/storage/sqlite.rs index 0a3aad607..19ed14262 100644 --- a/rslib/src/storage/sqlite.rs +++ b/rslib/src/storage/sqlite.rs @@ -1,7 +1,6 @@ // Copyright: Ankitects Pty Ltd and contributors // License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html -use crate::collection::CollectionOp; use crate::config::Config; use crate::decks::DeckID; use crate::err::Result; @@ -232,10 +231,6 @@ impl SqliteStorage { Ok(()) } - pub(crate) fn commit_rust_op(&self, _op: Option) -> Result<()> { - self.commit_rust_trx() - } - pub(crate) fn rollback_rust_trx(&self) -> Result<()> { self.db .prepare_cached("rollback to rust")? diff --git a/rslib/src/undo.rs b/rslib/src/undo.rs new file mode 100644 index 000000000..87c1f3818 --- /dev/null +++ b/rslib/src/undo.rs @@ -0,0 +1,126 @@ +// Copyright: Ankitects Pty Ltd and contributors +// License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html + +use crate::{ + collection::{Collection, CollectionOp}, + err::Result, +}; +use std::fmt; + +pub(crate) trait Undoable: fmt::Debug + Send { + fn apply(&self, ctx: &mut Collection) -> Result<()>; +} + +#[derive(Debug)] +struct UndoStep { + kind: CollectionOp, + changes: Vec>, +} + +#[derive(Debug, PartialEq)] +enum UndoMode { + NormalOp, + Undoing, + Redoing, +} + +impl Default for UndoMode { + fn default() -> Self { + Self::NormalOp + } +} + +#[derive(Debug, Default)] +pub(crate) struct UndoManager { + undo_steps: Vec, + redo_steps: Vec, + mode: UndoMode, + current_step: Option, +} + +impl UndoManager { + pub(crate) fn save_undoable(&mut self, item: Box) { + if let Some(step) = self.current_step.as_mut() { + step.changes.push(item) + } + } + + pub(crate) fn begin_step(&mut self, op: Option) { + if op.is_none() { + // action doesn't support undoing; clear the queue + self.undo_steps.clear(); + self.redo_steps.clear(); + } else if self.mode == UndoMode::NormalOp { + // a normal op clears the redo queue + self.redo_steps.clear(); + } + self.current_step = op.map(|op| UndoStep { + kind: op, + changes: vec![], + }); + } + + pub(crate) fn end_step(&mut self) { + if let Some(step) = self.current_step.take() { + if self.mode == UndoMode::Undoing { + self.redo_steps.push(step); + } else { + self.undo_steps.push(step); + } + } + } + + pub(crate) fn discard_step(&mut self) { + self.begin_step(None) + } + + fn can_undo(&self) -> Option { + self.undo_steps.last().map(|s| s.kind.clone()) + } + + fn can_redo(&self) -> Option { + self.redo_steps.last().map(|s| s.kind.clone()) + } +} + +impl Collection { + pub fn can_undo(&self) -> Option { + self.state.undo.can_undo() + } + + pub fn can_redo(&self) -> Option { + self.state.undo.can_redo() + } + + pub fn undo(&mut self) -> Result<()> { + if let Some(step) = self.state.undo.undo_steps.pop() { + let changes = step.changes; + self.state.undo.mode = UndoMode::Undoing; + let res = self.transact(Some(step.kind), |col| { + for change in changes.iter().rev() { + change.apply(col)?; + } + Ok(()) + }); + self.state.undo.mode = UndoMode::NormalOp; + res?; + } + Ok(()) + } + + 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(Some(step.kind), |col| { + for change in changes.iter().rev() { + change.apply(col)?; + } + Ok(()) + }); + self.state.undo.mode = UndoMode::NormalOp; + res?; + } + Ok(()) + } +}