diff --git a/pylib/anki/collection.py b/pylib/anki/collection.py index 6cb230ee9..dc62566b8 100644 --- a/pylib/anki/collection.py +++ b/pylib/anki/collection.py @@ -896,6 +896,28 @@ table.review-log {{ {revlog_style} }} assert_exhaustive(self._undo) assert False + def add_custom_undo_entry(self, name: str) -> int: + """Add an empty undo entry with the given name. + The return value can be used to merge subsequent changes + with `merge_undo_entries()`. + + You should only use this with your own custom actions - when + extending default Anki behaviour, you should merge into an + existing undo entry instead, so the existing undo name is + preserved, and changes are processed correctly. + """ + return self._backend.add_custom_undo_entry(name) + + def merge_undo_entries(self, target: int) -> OpChanges: + """Combine multiple undoable operations into one. + + After a standard Anki action, you can use col.undo_status().last_step + to retrieve the target to merge into. When defining your own custom + actions, you can use `add_custom_undo_entry()` to define a custom + undo name. + """ + return self._backend.merge_undo_entries(target) + def clear_python_undo(self) -> None: """Clear the Python undo state. The backend will automatically clear backend undo state when diff --git a/rslib/backend.proto b/rslib/backend.proto index 1ae1b3092..32d72d873 100644 --- a/rslib/backend.proto +++ b/rslib/backend.proto @@ -289,6 +289,8 @@ service CollectionService { rpc GetUndoStatus(Empty) returns (UndoStatus); rpc Undo(Empty) returns (OpChangesAfterUndo); rpc Redo(Empty) returns (OpChangesAfterUndo); + rpc AddCustomUndoEntry(String) returns (UInt32); + rpc MergeUndoEntries(UInt32) returns (OpChanges); rpc LatestProgress(Empty) returns (Progress); rpc SetWantsAbort(Empty) returns (Empty); } @@ -1552,6 +1554,7 @@ message OpChanges { message UndoStatus { string undo = 1; string redo = 2; + uint32 last_step = 3; } message OpChangesAfterUndo { diff --git a/rslib/src/backend/collection.rs b/rslib/src/backend/collection.rs index dd23890c2..12cf8b125 100644 --- a/rslib/src/backend/collection.rs +++ b/rslib/src/backend/collection.rs @@ -96,4 +96,14 @@ impl CollectionService for Backend { fn redo(&self, _input: pb::Empty) -> Result { self.with_col(|col| col.redo().map(|out| out.into_protobuf(&col.tr))) } + + fn add_custom_undo_entry(&self, input: pb::String) -> Result { + self.with_col(|col| Ok(col.add_custom_undo_step(input.val).into())) + } + + fn merge_undo_entries(&self, input: pb::UInt32) -> Result { + let starting_from = input.val as usize; + self.with_col(|col| col.merge_undoable_ops(starting_from)) + .map(Into::into) + } } diff --git a/rslib/src/backend/ops.rs b/rslib/src/backend/ops.rs index c9c215f51..0f9c43636 100644 --- a/rslib/src/backend/ops.rs +++ b/rslib/src/backend/ops.rs @@ -31,6 +31,7 @@ impl UndoStatus { pb::UndoStatus { undo: self.undo.map(|op| op.describe(tr)).unwrap_or_default(), redo: self.redo.map(|op| op.describe(tr)).unwrap_or_default(), + last_step: self.last_step as u32, } } } diff --git a/rslib/src/collection/transact.rs b/rslib/src/collection/transact.rs index 8704e08a2..59fc5ccac 100644 --- a/rslib/src/collection/transact.rs +++ b/rslib/src/collection/transact.rs @@ -8,6 +8,8 @@ impl Collection { where F: FnOnce(&mut Collection) -> Result, { + let have_op = op.is_some(); + self.storage.begin_rust_trx()?; self.begin_undoable_operation(op); @@ -23,10 +25,10 @@ impl Collection { match res { Ok(output) => { - let changes = if op.is_some() { + let changes = if have_op { let changes = self.op_changes(); - self.maybe_clear_study_queues_after_op(changes); - self.maybe_coalesce_note_undo_entry(changes); + self.maybe_clear_study_queues_after_op(&changes); + self.maybe_coalesce_note_undo_entry(&changes); changes } else { self.clear_study_queues(); diff --git a/rslib/src/config/undo.rs b/rslib/src/config/undo.rs index 15288526f..0ddf90fff 100644 --- a/rslib/src/config/undo.rs +++ b/rslib/src/config/undo.rs @@ -85,13 +85,13 @@ mod test { assert_eq!(col.get_bool(key), true); // first set adds the key - col.transact(op, |col| col.set_bool(key, false))?; + col.transact(op.clone(), |col| col.set_bool(key, false))?; assert_eq!(col.get_bool(key), false); // mutate it twice - col.transact(op, |col| col.set_bool(key, true))?; + col.transact(op.clone(), |col| col.set_bool(key, true))?; assert_eq!(col.get_bool(key), true); - col.transact(op, |col| col.set_bool(key, false))?; + col.transact(op.clone(), |col| col.set_bool(key, false))?; assert_eq!(col.get_bool(key), false); // when we remove it, it goes back to its default diff --git a/rslib/src/notes/undo.rs b/rslib/src/notes/undo.rs index 46901a7a8..709778810 100644 --- a/rslib/src/notes/undo.rs +++ b/rslib/src/notes/undo.rs @@ -58,7 +58,7 @@ impl Collection { } /// If note is edited multiple times in quick succession, avoid creating extra undo entries. - pub(crate) fn maybe_coalesce_note_undo_entry(&mut self, changes: OpChanges) { + pub(crate) fn maybe_coalesce_note_undo_entry(&mut self, changes: &OpChanges) { if changes.op != Op::UpdateNote { return; } diff --git a/rslib/src/ops.rs b/rslib/src/ops.rs index 6344ddcfc..33709b87e 100644 --- a/rslib/src/ops.rs +++ b/rslib/src/ops.rs @@ -3,8 +3,9 @@ use crate::prelude::*; -#[derive(Debug, Clone, Copy, PartialEq)] +#[derive(Debug, Clone, PartialEq)] pub enum Op { + Custom(String), AddDeck, AddNote, AddNotetype, @@ -42,7 +43,7 @@ pub enum Op { } impl Op { - pub fn describe(self, tr: &I18n) -> String { + pub fn describe(&self, tr: &I18n) -> String { match self { Op::AddDeck => tr.undo_add_deck(), Op::AddNote => tr.undo_add_note(), @@ -78,6 +79,7 @@ impl Op { Op::AddNotetype => tr.undo_add_notetype(), Op::RemoveNotetype => tr.undo_remove_notetype(), Op::UpdateNotetype => tr.undo_update_notetype(), + Op::Custom(name) => name.into(), } .into() } @@ -94,7 +96,7 @@ pub struct StateChanges { pub deck_config: bool, } -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone)] pub struct OpChanges { pub op: Op, pub changes: StateChanges, diff --git a/rslib/src/scheduler/queue/mod.rs b/rslib/src/scheduler/queue/mod.rs index f9294700c..36415cc11 100644 --- a/rslib/src/scheduler/queue/mod.rs +++ b/rslib/src/scheduler/queue/mod.rs @@ -136,7 +136,7 @@ impl Collection { self.state.card_queues = None; } - pub(crate) fn maybe_clear_study_queues_after_op(&mut self, op: OpChanges) { + pub(crate) fn maybe_clear_study_queues_after_op(&mut self, op: &OpChanges) { if op.requires_study_queue_rebuild() { self.state.card_queues = None; } diff --git a/rslib/src/undo/mod.rs b/rslib/src/undo/mod.rs index f0af3dc0e..1cf1ab94d 100644 --- a/rslib/src/undo/mod.rs +++ b/rslib/src/undo/mod.rs @@ -21,17 +21,20 @@ pub(crate) struct UndoableOp { pub kind: Op, pub timestamp: TimestampSecs, pub changes: Vec, + pub counter: usize, } impl UndoableOp { /// True if changes empty, or only the collection mtime has changed. + /// Always true in the case of custom steps. fn has_changes(&self) -> bool { - !matches!( - &self.changes[..], - &[] | &[UndoableChange::Collection( - UndoableCollectionChange::Modified(_) - )] - ) + matches!(self.kind, Op::Custom(_)) + || !matches!( + &self.changes[..], + &[] | &[UndoableChange::Collection( + UndoableCollectionChange::Modified(_) + )] + ) } } @@ -51,6 +54,7 @@ impl Default for UndoMode { pub struct UndoStatus { pub undo: Option, pub redo: Option, + pub last_step: usize, } pub struct UndoOutput { @@ -68,6 +72,7 @@ pub(crate) struct UndoManager { redo_steps: Vec, mode: UndoMode, current_step: Option, + counter: usize, } impl UndoManager { @@ -90,6 +95,10 @@ impl UndoManager { kind: op, timestamp: TimestampSecs::now(), changes: vec![], + counter: { + self.counter += 1; + self.counter + }, }); } @@ -109,12 +118,12 @@ impl UndoManager { println!("ended, undo steps count now {}", self.undo_steps.len()); } - fn can_undo(&self) -> Option { - self.undo_steps.front().map(|s| s.kind) + fn can_undo(&self) -> Option<&Op> { + self.undo_steps.front().map(|s| &s.kind) } - fn can_redo(&self) -> Option { - self.redo_steps.last().map(|s| s.kind) + fn can_redo(&self) -> Option<&Op> { + self.redo_steps.last().map(|s| &s.kind) } fn previous_op(&self) -> Option<&UndoableOp> { @@ -131,35 +140,57 @@ impl UndoManager { .as_ref() .expect("current_changes() called when no op set"); - let mut changes = StateChanges::default(); - for change in ¤t_op.changes { - match change { - UndoableChange::Card(_) => changes.card = true, - UndoableChange::Note(_) => changes.note = true, - UndoableChange::Deck(_) => changes.deck = true, - UndoableChange::Tag(_) => changes.tag = true, - UndoableChange::Revlog(_) => {} - UndoableChange::Queue(_) => {} - UndoableChange::Config(_) => changes.config = true, - UndoableChange::DeckConfig(_) => changes.deck_config = true, - UndoableChange::Collection(_) => {} - UndoableChange::Notetype(_) => changes.notetype = true, - } - } - + let changes = StateChanges::from(¤t_op.changes[..]); OpChanges { - op: current_op.kind, + op: current_op.kind.clone(), changes, } } + + fn merge_undoable_ops(&mut self, starting_from: usize) -> Result { + let target_idx = self + .undo_steps + .iter() + .enumerate() + .filter_map(|(idx, op)| { + if op.counter == starting_from { + Some(idx) + } else { + None + } + }) + .next() + .ok_or_else(|| AnkiError::invalid_input("target undo op not found"))?; + let mut removed = vec![]; + for _ in 0..target_idx { + removed.push(self.undo_steps.pop_front().unwrap()); + } + let target = self.undo_steps.front_mut().unwrap(); + for step in removed.into_iter().rev() { + target.changes.extend(step.changes.into_iter()); + } + + Ok(OpChanges { + op: target.kind.clone(), + changes: StateChanges::from(&target.changes[..]), + }) + } + + /// Start a new step with a custom name, and return its associated + /// counter value, which can be used with `merge_undoable_ops`. + fn add_custom_step(&mut self, name: String) -> usize { + self.begin_step(Some(Op::Custom(name))); + self.end_step(); + self.counter + } } impl Collection { - pub fn can_undo(&self) -> Option { + pub fn can_undo(&self) -> Option<&Op> { self.state.undo.can_undo() } - pub fn can_redo(&self) -> Option { + pub fn can_redo(&self) -> Option<&Op> { self.state.undo.can_redo() } @@ -180,11 +211,25 @@ impl Collection { pub fn undo_status(&self) -> UndoStatus { UndoStatus { - undo: self.can_undo(), - redo: self.can_redo(), + undo: self.can_undo().cloned(), + redo: self.can_redo().cloned(), + last_step: self.state.undo.counter, } } + /// Merge multiple undoable operations into one, and return the union of + /// their changes. + pub fn merge_undoable_ops(&mut self, starting_from: usize) -> Result { + self.state.undo.merge_undoable_ops(starting_from) + } + + /// Add an empty custom undo step, which subsequent changes can be merged into. + pub fn add_custom_undo_step(&mut self, name: String) -> usize { + self.state.undo.add_custom_step(name) + } +} + +impl Collection { /// If op is None, clears the undo/redo queues. pub(crate) fn begin_undoable_operation(&mut self, op: Option) { self.state.undo.begin_step(op); @@ -235,15 +280,13 @@ impl Collection { pub(crate) fn op_changes(&self) -> OpChanges { self.state.undo.op_changes() } -} -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| { + let res = self.transact(undone_op.clone(), |col| { for change in changes.into_iter().rev() { change.undo(col)?; } @@ -258,8 +301,30 @@ impl Collection { } } +impl From<&[UndoableChange]> for StateChanges { + fn from(changes: &[UndoableChange]) -> Self { + let mut out = StateChanges::default(); + for change in changes { + match change { + UndoableChange::Card(_) => out.card = true, + UndoableChange::Note(_) => out.note = true, + UndoableChange::Deck(_) => out.deck = true, + UndoableChange::Tag(_) => out.tag = true, + UndoableChange::Revlog(_) => {} + UndoableChange::Queue(_) => {} + UndoableChange::Config(_) => out.config = true, + UndoableChange::DeckConfig(_) => out.deck_config = true, + UndoableChange::Collection(_) => {} + UndoableChange::Notetype(_) => out.notetype = true, + } + } + out + } +} + #[cfg(test)] mod test { + use super::UndoableChange; use crate::{card::Card, collection::open_test_collection, prelude::*}; #[test] @@ -301,38 +366,38 @@ mod test { } assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 4); - assert_eq!(col.can_undo(), Some(Op::UpdateCard)); + assert_eq!(col.can_undo(), Some(&Op::UpdateCard)); assert_eq!(col.can_redo(), None); // undo a step col.undo().unwrap(); assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 3); - assert_eq!(col.can_undo(), Some(Op::UpdateCard)); - assert_eq!(col.can_redo(), Some(Op::UpdateCard)); + assert_eq!(col.can_undo(), Some(&Op::UpdateCard)); + assert_eq!(col.can_redo(), Some(&Op::UpdateCard)); // and again col.undo().unwrap(); assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 2); assert_eq!(col.can_undo(), None); - assert_eq!(col.can_redo(), Some(Op::UpdateCard)); + assert_eq!(col.can_redo(), Some(&Op::UpdateCard)); // redo a step col.redo().unwrap(); assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 3); - assert_eq!(col.can_undo(), Some(Op::UpdateCard)); - assert_eq!(col.can_redo(), Some(Op::UpdateCard)); + assert_eq!(col.can_undo(), Some(&Op::UpdateCard)); + assert_eq!(col.can_redo(), Some(&Op::UpdateCard)); // and another col.redo().unwrap(); assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 4); - assert_eq!(col.can_undo(), Some(Op::UpdateCard)); + assert_eq!(col.can_undo(), Some(&Op::UpdateCard)); assert_eq!(col.can_redo(), None); // and undo the redo col.undo().unwrap(); assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 3); - assert_eq!(col.can_undo(), Some(Op::UpdateCard)); - assert_eq!(col.can_redo(), Some(Op::UpdateCard)); + assert_eq!(col.can_undo(), Some(&Op::UpdateCard)); + assert_eq!(col.can_redo(), Some(&Op::UpdateCard)); // if any action is performed, it should clear the redo queue col.transact(Op::UpdateCard, |col| { @@ -342,7 +407,7 @@ mod test { }) })?; assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 5); - assert_eq!(col.can_undo(), Some(Op::UpdateCard)); + assert_eq!(col.can_undo(), Some(&Op::UpdateCard)); assert_eq!(col.can_redo(), None); // and any action that doesn't support undoing will clear both queues @@ -369,4 +434,68 @@ mod test { Ok(()) } + + #[test] + fn custom() -> Result<()> { + let mut col = open_test_collection(); + + // perform some actions in separate steps + let nt = col.get_notetype_by_name("Basic")?.unwrap(); + let mut note = nt.new_note(); + col.add_note(&mut note, DeckId(1))?; + assert_eq!(col.undo_status().last_step, 1); + + let card = col.storage.all_cards_of_note(note.id)?.remove(0); + + col.transact(Op::UpdateCard, |col| { + col.get_and_update_card(card.id, |card| { + card.due = 10; + Ok(()) + }) + })?; + + let restore_point = col.add_custom_undo_step("hello".to_string()); + + col.transact(Op::UpdateCard, |col| { + col.get_and_update_card(card.id, |card| { + card.due = 20; + Ok(()) + }) + })?; + col.transact(Op::UpdateCard, |col| { + col.get_and_update_card(card.id, |card| { + card.due = 30; + Ok(()) + }) + })?; + // dummy op name + col.transact(Op::Bury, |col| col.set_current_notetype_id(NotetypeId(123)))?; + + // merge subsequent changes into our restore point + let op = col.merge_undoable_ops(restore_point)?; + assert_eq!(op.changes.card, true); + assert_eq!(op.changes.config, true); + + // the last undo action should be at the end of the step list, + // before the modtime bump + assert!(matches!( + col.state + .undo + .previous_op() + .unwrap() + .changes + .iter() + .rev() + .nth(1) + .unwrap(), + UndoableChange::Config(_) + )); + + // if we then undo, we'll be back to before step 3 + assert_eq!(col.storage.get_card(card.id)?.unwrap().due, 30); + col.undo()?; + assert_eq!(col.storage.get_card(card.id)?.unwrap().due, 10); + + Ok(()) + } }