langfingaz/anki
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

add support for custom undo steps, and merging multiple actions

Browse Source 
Allows add-on authors to define their own label for a group of undoable
operations. For example:

def mark_and_bury(
    *,
    parent: QWidget,
    card_id: CardId,
) -> CollectionOp[OpChanges]:
    def op(col: Collection) -> OpChanges:
        target = col.add_custom_undo_entry("Mark and Bury")
        col.sched.bury_cards([card_id])
        card = col.get_card(card_id)
        col.tags.bulk_add(note_ids=[card.nid], tags="marked")
        return col.merge_undo_entries(target)

    return CollectionOp(parent, op)

The .add_custom_undo_entry() is for adding your own custom actions.
When extending a standard Anki action, instead store `target = 
col.undo_status().last_step` after executing the standard operation.

This started out as a bigger refactor that required a separate
.commit_undoable() call to be run after each operation, instead of
having each operation return changes directly. But that proved to be
somewhat cumbersome in unit tests, and ran the risk of unexpected
behaviour if the caller invoked an operation without remembering to
finalize it.
This commit is contained in:
Damien Elmes 2021-05-06 15:54:04 +10:00
parent 2663c891eb
commit be994f4102
10 changed files with 224 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
pylib/anki/collection.py
View File

@ -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

3
rslib/backend.proto
View File

@ -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 {

10
rslib/src/backend/collection.rs
View File

@ -96,4 +96,14 @@ impl CollectionService for Backend {
fn redo(&self, _input: pb::Empty) -> Result<pb::OpChangesAfterUndo> {
self.with_col(|col| col.redo().map(|out| out.into_protobuf(&col.tr)))
}
fn add_custom_undo_entry(&self, input: pb::String) -> Result<pb::UInt32> {
self.with_col(|col| Ok(col.add_custom_undo_step(input.val).into()))
}
fn merge_undo_entries(&self, input: pb::UInt32) -> Result<pb::OpChanges> {
let starting_from = input.val as usize;
self.with_col(|col| col.merge_undoable_ops(starting_from))
.map(Into::into)
}
}

1
rslib/src/backend/ops.rs
View File

@ -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,
}
}
}

8
rslib/src/collection/transact.rs
View File

@ -8,6 +8,8 @@ impl Collection {
where
F: FnOnce(&mut Collection) -> Result<R>,
{
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();

6
rslib/src/config/undo.rs
View File

@ -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

2
rslib/src/notes/undo.rs
View File

@ -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;
}

8
rslib/src/ops.rs
View File

@ -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,

2
rslib/src/scheduler/queue/mod.rs
View File

@ -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;
}

217
rslib/src/undo/mod.rs
View File

@ -21,17 +21,20 @@ pub(crate) struct UndoableOp {
pub kind: Op,
pub timestamp: TimestampSecs,
pub changes: Vec<UndoableChange>,
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<Op>,
pub redo: Option<Op>,
pub last_step: usize,
}
pub struct UndoOutput {
@ -68,6 +72,7 @@ pub(crate) struct UndoManager {
redo_steps: Vec<UndoableOp>,
mode: UndoMode,
current_step: Option<UndoableOp>,
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<Op> {
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<Op> {
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 &current_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(&current_op.changes[..]);
OpChanges {
op: current_op.kind,
op: current_op.kind.clone(),
changes,
}
}
fn merge_undoable_ops(&mut self, starting_from: usize) -> Result<OpChanges> {
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<Op> {
pub fn can_undo(&self) -> Option<&Op> {
self.state.undo.can_undo()
}
pub fn can_redo(&self) -> Option<Op> {
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<OpChanges> {
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<Op>) {
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<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| {
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(())
}
}