diff --git a/pylib/anki/importing/noteimp.py b/pylib/anki/importing/noteimp.py index 7095a6408..8865c2674 100644 --- a/pylib/anki/importing/noteimp.py +++ b/pylib/anki/importing/noteimp.py @@ -250,7 +250,7 @@ class NoteImporter(Importer): self.col.tags.join(n.tags), n.fieldsStr, "", - "", + 0, 0, "", ] diff --git a/rslib/src/backend/mod.rs b/rslib/src/backend/mod.rs index 105aa3c7b..0d0a19e7d 100644 --- a/rslib/src/backend/mod.rs +++ b/rslib/src/backend/mod.rs @@ -1449,7 +1449,7 @@ impl BackendService for Backend { fn clear_tag(&self, tag: pb::String) -> BackendResult { self.with_col(|col| { col.transact(None, |col| { - col.storage.clear_tag(tag.val.as_str())?; + col.storage.clear_tag_and_children(tag.val.as_str())?; Ok(().into()) }) }) diff --git a/rslib/src/card.rs b/rslib/src/card.rs index 0386404a5..1cbf0d393 100644 --- a/rslib/src/card.rs +++ b/rslib/src/card.rs @@ -6,7 +6,7 @@ use crate::err::{AnkiError, Result}; use crate::notes::NoteID; use crate::{ collection::Collection, config::SchedulerVersion, timestamp::TimestampSecs, types::Usn, - undo::Undoable, + undo::Undo, }; use crate::{deckconf::DeckConf, decks::DeckID}; use num_enum::TryFromPrimitive; @@ -124,10 +124,10 @@ impl Card { } } #[derive(Debug)] -pub(crate) struct UpdateCardUndo(Card); +pub(crate) struct CardUpdated(Card); -impl Undoable for UpdateCardUndo { - fn apply(&self, col: &mut crate::collection::Collection, usn: Usn) -> Result<()> { +impl Undo for CardUpdated { + fn undo(self: Box, col: &mut crate::collection::Collection, usn: Usn) -> Result<()> { let current = col .storage .get_card(self.0.id)? @@ -168,9 +168,7 @@ impl Collection { if card.id.0 == 0 { return Err(AnkiError::invalid_input("card id not set")); } - self.state - .undo - .save_undoable(Box::new(UpdateCardUndo(original.clone()))); + self.save_undo(Box::new(CardUpdated(original.clone()))); card.set_modified(usn); self.storage.update_card(card) } @@ -247,99 +245,3 @@ impl Collection { Ok(DeckConf::default()) } } - -#[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.interval = 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.interval = 2; - Ok(()) - }) - .unwrap(); - assert_eq!(card.interval, 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.interval = i; - Ok(()) - }) - .unwrap(); - Ok(()) - }) - .unwrap(); - } - - assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 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().interval, 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().interval, 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().interval, 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().interval, 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().interval, 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.interval = 5; - Ok(()) - }) - .unwrap(); - Ok(()) - }) - .unwrap(); - assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 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 433ba2742..1ce2253e4 100644 --- a/rslib/src/collection.rs +++ b/rslib/src/collection.rs @@ -78,9 +78,10 @@ pub struct Collection { pub(crate) state: CollectionState, } -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, Copy, PartialEq)] pub enum CollectionOp { UpdateCard, + AnswerCard, } impl Collection { diff --git a/rslib/src/dbcheck.rs b/rslib/src/dbcheck.rs index 95b1bbe24..caa00a96c 100644 --- a/rslib/src/dbcheck.rs +++ b/rslib/src/dbcheck.rs @@ -235,7 +235,7 @@ impl Collection { let stamp = TimestampMillis::now(); let expanded_tags = self.storage.expanded_tags()?; - self.storage.clear_tags()?; + self.storage.clear_all_tags()?; let total_notes = self.storage.total_notes()?; let mut checked_notes = 0; @@ -247,7 +247,7 @@ impl Collection { None => { let first_note = self.storage.get_note(group.peek().unwrap().1)?.unwrap(); out.notetypes_recovered += 1; - self.recover_notetype(stamp, first_note.fields.len(), ntid)? + self.recover_notetype(stamp, first_note.fields().len(), ntid)? } Some(nt) => nt, }; @@ -264,6 +264,7 @@ impl Collection { checked_notes += 1; let mut note = self.get_note_fixing_invalid_utf8(nid, out)?; + let original = note.clone(); let cards = self.storage.existing_cards_for_note(nid)?; @@ -271,7 +272,7 @@ impl Collection { out.templates_missing += self.remove_cards_without_template(&nt, &cards)?; // fix fields - if note.fields.len() != nt.fields.len() { + if note.fields().len() != nt.fields.len() { note.fix_field_count(&nt); note.tags.push("db-check".into()); out.field_count_mismatch += 1; @@ -282,7 +283,7 @@ impl Collection { // write note, updating tags and generating missing cards let ctx = genctx.get_or_insert_with(|| CardGenContext::new(&nt, usn)); - self.update_note_inner_generating_cards(&ctx, &mut note, false, norm)?; + self.update_note_inner_generating_cards(&ctx, &mut note, &original, false, norm)?; } } @@ -575,7 +576,7 @@ mod test { } ); let note = col.storage.get_note(note.id)?.unwrap(); - assert_eq!(¬e.fields, &["a", "b; c; d"]); + assert_eq!(¬e.fields()[..], &["a", "b; c; d"]); // missing fields get filled with blanks col.storage @@ -590,7 +591,7 @@ mod test { } ); let note = col.storage.get_note(note.id)?.unwrap(); - assert_eq!(¬e.fields, &["a", ""]); + assert_eq!(¬e.fields()[..], &["a", ""]); Ok(()) } diff --git a/rslib/src/decks/tree.rs b/rslib/src/decks/tree.rs index 0c8065b40..0c5088c50 100644 --- a/rslib/src/decks/tree.rs +++ b/rslib/src/decks/tree.rs @@ -391,7 +391,7 @@ mod test { // add some new cards let nt = col.get_notetype_by_name("Cloze")?.unwrap(); let mut note = nt.new_note(); - note.fields[0] = "{{c1::}} {{c2::}} {{c3::}} {{c4::}}".into(); + note.set_field(0, "{{c1::}} {{c2::}} {{c3::}} {{c4::}}")?; col.add_note(&mut note, child_deck.id)?; let tree = col.deck_tree(Some(TimestampSecs::now()), None)?; diff --git a/rslib/src/findreplace.rs b/rslib/src/findreplace.rs index 43ee636d4..516acb0db 100644 --- a/rslib/src/findreplace.rs +++ b/rslib/src/findreplace.rs @@ -70,7 +70,7 @@ impl Collection { match field_ord { None => { // all fields - for txt in &mut note.fields { + for txt in note.fields_mut() { if let Cow::Owned(otxt) = ctx.replace_text(txt) { changed = true; *txt = otxt; @@ -79,7 +79,7 @@ impl Collection { } Some(ord) => { // single field - if let Some(txt) = note.fields.get_mut(ord) { + if let Some(txt) = note.fields_mut().get_mut(ord) { if let Cow::Owned(otxt) = ctx.replace_text(txt) { changed = true; *txt = otxt; @@ -108,13 +108,13 @@ mod test { let nt = col.get_notetype_by_name("Basic")?.unwrap(); let mut note = nt.new_note(); - note.fields[0] = "one aaa".into(); - note.fields[1] = "two aaa".into(); + note.set_field(0, "one aaa")?; + note.set_field(1, "two aaa")?; col.add_note(&mut note, DeckID(1))?; let nt = col.get_notetype_by_name("Cloze")?.unwrap(); let mut note2 = nt.new_note(); - note2.fields[0] = "three aaa".into(); + note2.set_field(0, "three aaa")?; col.add_note(&mut note2, DeckID(1))?; let nids = col.search_notes("")?; @@ -123,10 +123,10 @@ mod test { let note = col.storage.get_note(note.id)?.unwrap(); // but the update should be limited to the specified field when it was available - assert_eq!(¬e.fields, &["one BBB", "two BBB"]); + assert_eq!(¬e.fields()[..], &["one BBB", "two BBB"]); let note2 = col.storage.get_note(note2.id)?.unwrap(); - assert_eq!(¬e2.fields, &["three BBB", ""]); + assert_eq!(¬e2.fields()[..], &["three BBB", ""]); assert_eq!( col.storage.field_names_for_notes(&nids)?, @@ -144,7 +144,7 @@ mod test { let note = col.storage.get_note(note.id)?.unwrap(); // but the update should be limited to the specified field when it was available - assert_eq!(¬e.fields, &["one ccc", "two BBB"]); + assert_eq!(¬e.fields()[..], &["one ccc", "two BBB"]); Ok(()) } diff --git a/rslib/src/notes.rs b/rslib/src/notes.rs index 6be535d98..c82995c24 100644 --- a/rslib/src/notes.rs +++ b/rslib/src/notes.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::backend_proto::note_is_duplicate_or_empty_out::State as DuplicateState; use crate::{ backend_proto as pb, collection::Collection, @@ -14,6 +13,7 @@ use crate::{ timestamp::TimestampSecs, types::Usn, }; +use crate::{backend_proto::note_is_duplicate_or_empty_out::State as DuplicateState, undo::Undo}; use itertools::Itertools; use num_integer::Integer; use regex::{Regex, Replacer}; @@ -40,7 +40,7 @@ pub struct Note { pub mtime: TimestampSecs, pub usn: Usn, pub tags: Vec, - pub(crate) fields: Vec, + fields: Vec, pub(crate) sort_field: Option, pub(crate) checksum: Option, } @@ -60,10 +60,46 @@ impl Note { } } + #[allow(clippy::clippy::too_many_arguments)] + pub(crate) fn new_from_storage( + id: NoteID, + guid: String, + notetype_id: NoteTypeID, + mtime: TimestampSecs, + usn: Usn, + tags: Vec, + fields: Vec, + sort_field: Option, + checksum: Option, + ) -> Self { + Self { + id, + guid, + notetype_id, + mtime, + usn, + tags, + fields, + sort_field, + checksum, + } + } + pub fn fields(&self) -> &Vec { &self.fields } + pub(crate) fn fields_mut(&mut self) -> &mut Vec { + self.mark_dirty(); + &mut self.fields + } + + // Ensure we get an error if caller forgets to call prepare_for_update(). + fn mark_dirty(&mut self) { + self.sort_field = None; + self.checksum = None; + } + pub fn set_field(&mut self, idx: usize, text: impl Into) -> Result<()> { if idx >= self.fields.len() { return Err(AnkiError::invalid_input( @@ -72,6 +108,7 @@ impl Note { } self.fields[idx] = text.into(); + self.mark_dirty(); Ok(()) } @@ -252,7 +289,7 @@ fn invalid_char_for_field(c: char) -> bool { } impl Collection { - fn canonify_note_tags(&self, note: &mut Note, usn: Usn) -> Result<()> { + fn canonify_note_tags(&mut self, note: &mut Note, usn: Usn) -> Result<()> { if !note.tags.is_empty() { let tags = std::mem::replace(&mut note.tags, vec![]); note.tags = self.canonify_tags(tags, usn)?.0; @@ -286,13 +323,10 @@ impl Collection { } pub fn update_note(&mut self, note: &mut Note) -> Result<()> { - if let Some(existing_note) = self.storage.get_note(note.id)? { - if &existing_note == note { - // nothing to do - return Ok(()); - } - } else { - return Err(AnkiError::NotFound); + let existing_note = self.storage.get_note(note.id)?.ok_or(AnkiError::NotFound)?; + if &existing_note == note { + // nothing to do + return Ok(()); } self.transact(None, |col| { @@ -301,7 +335,7 @@ impl Collection { .ok_or_else(|| AnkiError::invalid_input("missing note type"))?; let ctx = CardGenContext::new(&nt, col.usn()?); let norm = col.normalize_note_text(); - col.update_note_inner_generating_cards(&ctx, note, true, norm) + col.update_note_inner_generating_cards(&ctx, note, &existing_note, true, norm) }) } @@ -309,11 +343,13 @@ impl Collection { &mut self, ctx: &CardGenContext, note: &mut Note, + original: &Note, mark_note_modified: bool, normalize_text: bool, ) -> Result<()> { self.update_note_inner_without_cards( note, + original, ctx.notetype, ctx.usn, mark_note_modified, @@ -325,6 +361,7 @@ impl Collection { pub(crate) fn update_note_inner_without_cards( &mut self, note: &mut Note, + original: &Note, nt: &NoteType, usn: Usn, mark_note_modified: bool, @@ -332,10 +369,28 @@ impl Collection { ) -> Result<()> { self.canonify_note_tags(note, usn)?; note.prepare_for_update(nt, normalize_text)?; - if mark_note_modified { + self.update_note_inner_undo_and_mtime_only( + note, + original, + if mark_note_modified { Some(usn) } else { None }, + ) + } + + /// Bumps modification time if usn provided, saves in the undo queue, and commits to DB. + /// No validation, card generation or normalization is done. + pub(crate) fn update_note_inner_undo_and_mtime_only( + &mut self, + note: &mut Note, + original: &Note, + update_usn: Option, + ) -> Result<()> { + if let Some(usn) = update_usn { note.set_modified(usn); } - self.storage.update_note(note) + self.save_undo(Box::new(NoteUpdated(original.clone()))); + self.storage.update_note(note)?; + + Ok(()) } /// Remove a note. Cards must already have been deleted. @@ -407,6 +462,7 @@ impl Collection { for (_, nid) in group { // grab the note and transform it let mut note = self.storage.get_note(nid)?.unwrap(); + let original = note.clone(); let out = transformer(&mut note, &nt)?; if !out.changed { continue; @@ -417,12 +473,14 @@ impl Collection { self.update_note_inner_generating_cards( &ctx, &mut note, + &original, out.mark_modified, norm, )?; } else { self.update_note_inner_without_cards( &mut note, + &original, &nt, usn, out.mark_modified, @@ -495,6 +553,19 @@ impl Collection { } } +#[derive(Debug)] +pub(crate) struct NoteUpdated(Note); + +impl Undo for NoteUpdated { + fn undo(self: Box, col: &mut crate::collection::Collection, usn: Usn) -> Result<()> { + let current = col + .storage + .get_note(self.0.id)? + .ok_or_else(|| AnkiError::invalid_input("note disappeared"))?; + col.update_note_inner_undo_and_mtime_only(&mut self.0.clone(), ¤t, Some(usn)) + } +} + #[cfg(test)] mod test { use super::{anki_base91, field_checksum}; diff --git a/rslib/src/notetype/render.rs b/rslib/src/notetype/render.rs index 689405a09..f4683f10f 100644 --- a/rslib/src/notetype/render.rs +++ b/rslib/src/notetype/render.rs @@ -169,7 +169,7 @@ fn fill_empty_fields(note: &mut Note, qfmt: &str, nt: &NoteType, i18n: &I18n) { if let Ok(tmpl) = ParsedTemplate::from_text(qfmt) { let cloze_fields = tmpl.cloze_fields(); - for (val, field) in note.fields.iter_mut().zip(nt.fields.iter()) { + for (val, field) in note.fields_mut().iter_mut().zip(nt.fields.iter()) { if field_is_empty(val) { if cloze_fields.contains(&field.name.as_str()) { *val = i18n.tr(TR::CardTemplatesSampleCloze).into(); diff --git a/rslib/src/notetype/schemachange.rs b/rslib/src/notetype/schemachange.rs index 4f87c280c..81567ccac 100644 --- a/rslib/src/notetype/schemachange.rs +++ b/rslib/src/notetype/schemachange.rs @@ -79,11 +79,11 @@ impl Collection { let usn = self.usn()?; for nid in nids { let mut note = self.storage.get_note(nid)?.unwrap(); - note.fields = ords + *note.fields_mut() = ords .iter() .map(|f| { if let Some(idx) = f { - note.fields + note.fields() .get(*idx as usize) .map(AsRef::as_ref) .unwrap_or("") @@ -201,24 +201,22 @@ mod test { .get_notetype(col.get_current_notetype_id().unwrap())? .unwrap(); let mut note = nt.new_note(); - assert_eq!(note.fields.len(), 2); - note.fields = vec!["one".into(), "two".into()]; + assert_eq!(note.fields().len(), 2); + note.set_field(0, "one")?; + note.set_field(1, "two")?; col.add_note(&mut note, DeckID(1))?; nt.add_field("three"); col.update_notetype(&mut nt, false)?; let note = col.storage.get_note(note.id)?.unwrap(); - assert_eq!( - note.fields, - vec!["one".to_string(), "two".into(), "".into()] - ); + assert_eq!(note.fields(), &["one".to_string(), "two".into(), "".into()]); nt.fields.remove(1); col.update_notetype(&mut nt, false)?; let note = col.storage.get_note(note.id)?.unwrap(); - assert_eq!(note.fields, vec!["one".to_string(), "".into()]); + assert_eq!(note.fields(), &["one".to_string(), "".into()]); Ok(()) } @@ -252,8 +250,9 @@ mod test { .get_notetype(col.get_current_notetype_id().unwrap())? .unwrap(); let mut note = nt.new_note(); - assert_eq!(note.fields.len(), 2); - note.fields = vec!["one".into(), "two".into()]; + assert_eq!(note.fields().len(), 2); + note.set_field(0, "one")?; + note.set_field(1, "two")?; col.add_note(&mut note, DeckID(1))?; assert_eq!( diff --git a/rslib/src/prelude.rs b/rslib/src/prelude.rs index 54a5ca204..6d7f932e6 100644 --- a/rslib/src/prelude.rs +++ b/rslib/src/prelude.rs @@ -3,7 +3,7 @@ pub use crate::{ card::{Card, CardID}, - collection::Collection, + collection::{Collection, CollectionOp}, deckconf::{DeckConf, DeckConfID}, decks::{Deck, DeckID, DeckKind}, err::{AnkiError, Result}, diff --git a/rslib/src/revlog.rs b/rslib/src/revlog.rs index 33fad146d..63af124b1 100644 --- a/rslib/src/revlog.rs +++ b/rslib/src/revlog.rs @@ -1,8 +1,11 @@ // Copyright: Ankitects Pty Ltd and contributors // License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html -use crate::serde::{default_on_invalid, deserialize_int_from_number}; use crate::{define_newtype, prelude::*}; +use crate::{ + serde::{default_on_invalid, deserialize_int_from_number}, + undo::Undo, +}; use num_enum::TryFromPrimitive; use serde::Deserialize; use serde_repr::{Deserialize_repr, Serialize_repr}; @@ -10,9 +13,25 @@ use serde_tuple::Serialize_tuple; define_newtype!(RevlogID, i64); +impl RevlogID { + pub fn new() -> Self { + RevlogID(TimestampMillis::now().0) + } + + pub fn as_secs(self) -> TimestampSecs { + TimestampSecs(self.0 / 1000) + } +} + +impl From for RevlogID { + fn from(m: TimestampMillis) -> Self { + RevlogID(m.0) + } +} + #[derive(Serialize_tuple, Deserialize, Debug, Default, PartialEq)] pub struct RevlogEntry { - pub id: TimestampMillis, + pub id: RevlogID, pub cid: CardID, pub usn: Usn, /// - In the V1 scheduler, 3 represents easy in the learning case. @@ -63,6 +82,14 @@ impl RevlogEntry { } impl Collection { + /// Add the provided revlog entry, modifying the ID if it is not unique. + pub(crate) fn add_revlog_entry(&mut self, mut entry: RevlogEntry) -> Result { + entry.id = self.storage.add_revlog_entry(&entry, true)?; + let id = entry.id; + self.save_undo(Box::new(RevlogAdded(entry))); + Ok(id) + } + pub(crate) fn log_manually_scheduled_review( &mut self, card: &Card, @@ -70,7 +97,7 @@ impl Collection { usn: Usn, ) -> Result<()> { let entry = RevlogEntry { - id: TimestampMillis::now(), + id: RevlogID::new(), cid: card.id, usn, button_chosen: 0, @@ -80,6 +107,28 @@ impl Collection { taken_millis: 0, review_kind: RevlogReviewKind::Manual, }; - self.storage.add_revlog_entry(&entry) + self.add_revlog_entry(entry)?; + Ok(()) + } +} + +#[derive(Debug)] +pub(crate) struct RevlogAdded(RevlogEntry); +#[derive(Debug)] +pub(crate) struct RevlogRemoved(RevlogEntry); + +impl Undo for RevlogAdded { + fn undo(self: Box, col: &mut crate::collection::Collection, _usn: Usn) -> Result<()> { + col.storage.remove_revlog_entry(self.0.id)?; + col.save_undo(Box::new(RevlogRemoved(self.0))); + Ok(()) + } +} + +impl Undo for RevlogRemoved { + fn undo(self: Box, col: &mut crate::collection::Collection, _usn: Usn) -> Result<()> { + col.storage.add_revlog_entry(&self.0, false)?; + col.save_undo(Box::new(RevlogAdded(self.0))); + Ok(()) } } diff --git a/rslib/src/scheduler/answering/mod.rs b/rslib/src/scheduler/answering/mod.rs index 3b4f24435..307ab2899 100644 --- a/rslib/src/scheduler/answering/mod.rs +++ b/rslib/src/scheduler/answering/mod.rs @@ -7,6 +7,7 @@ mod preview; mod relearning; mod review; mod revlog; +mod undo; use crate::{ backend_proto, @@ -44,7 +45,6 @@ pub struct CardAnswer { } // fixme: log preview review -// fixme: undo /// Holds the information required to determine a given card's /// current state, and to apply a state change to it. @@ -241,7 +241,9 @@ impl Collection { /// Answer card, writing its new state to the database. pub fn answer_card(&mut self, answer: &CardAnswer) -> Result<()> { - self.transact(None, |col| col.answer_card_inner(answer)) + self.transact(Some(CollectionOp::AnswerCard), |col| { + col.answer_card_inner(answer) + }) } fn answer_card_inner(&mut self, answer: &CardAnswer) -> Result<()> { @@ -267,6 +269,8 @@ impl Collection { self.update_deck_stats_from_answer(usn, &answer, &updater)?; let timing = updater.timing; + self.maybe_bury_siblings(&original, &updater.config)?; + let mut card = updater.into_card(); self.update_card(&mut card, &original, usn)?; if answer.new_state.leeched() { @@ -278,8 +282,21 @@ impl Collection { Ok(()) } + fn maybe_bury_siblings(&mut self, card: &Card, config: &DeckConf) -> Result<()> { + if config.inner.bury_new || config.inner.bury_reviews { + self.bury_siblings( + card.id, + card.note_id, + config.inner.bury_new, + config.inner.bury_reviews, + )?; + } + + Ok(()) + } + fn add_partial_revlog( - &self, + &mut self, partial: RevlogEntryPartial, usn: Usn, answer: &CardAnswer, @@ -291,7 +308,8 @@ impl Collection { answer.answered_at, answer.milliseconds_taken, ); - self.storage.add_revlog_entry(&revlog) + self.add_revlog_entry(revlog)?; + Ok(()) } fn update_deck_stats_from_answer( @@ -367,7 +385,7 @@ impl Collection { /// Return a consistent seed for a given card at a given number of reps. /// If in test environment, disable fuzzing. fn get_fuzz_seed(card: &Card) -> Option { - if *crate::timestamp::TESTING { + if *crate::timestamp::TESTING || cfg!(test) { None } else { Some((card.id.0 as u64).wrapping_add(card.reps as u64)) diff --git a/rslib/src/scheduler/answering/relearning.rs b/rslib/src/scheduler/answering/relearning.rs index 77e701adf..69378b804 100644 --- a/rslib/src/scheduler/answering/relearning.rs +++ b/rslib/src/scheduler/answering/relearning.rs @@ -18,6 +18,7 @@ impl CardStateUpdater { self.card.interval = next.review.scheduled_days; self.card.remaining_steps = next.learning.remaining_steps; self.card.ctype = CardType::Relearn; + self.card.lapses = next.review.lapses; let interval = next .interval_kind() diff --git a/rslib/src/scheduler/answering/revlog.rs b/rslib/src/scheduler/answering/revlog.rs index b5240cac2..a14f12b12 100644 --- a/rslib/src/scheduler/answering/revlog.rs +++ b/rslib/src/scheduler/answering/revlog.rs @@ -44,7 +44,7 @@ impl RevlogEntryPartial { taken_millis: u32, ) -> RevlogEntry { RevlogEntry { - id: answered_at, + id: answered_at.into(), cid, usn, button_chosen, diff --git a/rslib/src/scheduler/answering/undo.rs b/rslib/src/scheduler/answering/undo.rs new file mode 100644 index 000000000..28fd77eb8 --- /dev/null +++ b/rslib/src/scheduler/answering/undo.rs @@ -0,0 +1,136 @@ +// Copyright: Ankitects Pty Ltd and contributors +// License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html + +#[cfg(test)] +mod test { + use crate::{ + card::{CardQueue, CardType}, + collection::open_test_collection, + prelude::*, + scheduler::answering::{CardAnswer, Rating}, + }; + + #[test] + fn undo() -> Result<()> { + // add a note + let mut col = open_test_collection(); + let nt = col + .get_notetype_by_name("Basic (and reversed card)")? + .unwrap(); + let mut note = nt.new_note(); + note.set_field(0, "one")?; + note.set_field(1, "two")?; + col.add_note(&mut note, DeckID(1))?; + + // turn burying on + let mut conf = col.storage.get_deck_config(DeckConfID(1))?.unwrap(); + conf.inner.bury_new = true; + col.storage.update_deck_conf(&conf)?; + + // get the first card + let queued = col.next_card()?.unwrap(); + let nid = note.id; + let cid = queued.card.id; + let sibling_cid = col.storage.all_card_ids_of_note(nid)?[1]; + + let assert_initial_state = |col: &mut Collection| -> Result<()> { + let first = col.storage.get_card(cid)?.unwrap(); + assert_eq!(first.queue, CardQueue::New); + let sibling = col.storage.get_card(sibling_cid)?.unwrap(); + assert_eq!(sibling.queue, CardQueue::New); + Ok(()) + }; + + assert_initial_state(&mut col)?; + + // immediately graduate the first card + col.answer_card(&CardAnswer { + card_id: queued.card.id, + current_state: queued.next_states.current, + new_state: queued.next_states.easy, + rating: Rating::Easy, + answered_at: TimestampMillis::now(), + milliseconds_taken: 0, + })?; + + // the sibling will be buried + let sibling = col.storage.get_card(sibling_cid)?.unwrap(); + assert_eq!(sibling.queue, CardQueue::SchedBuried); + + // make it due now, with 7 lapses. we use the storage layer directly, + // bypassing undo + let mut card = col.storage.get_card(cid)?.unwrap(); + assert_eq!(card.ctype, CardType::Review); + card.lapses = 7; + card.due = 0; + col.storage.update_card(&card)?; + + // fail it, which should cause it to be marked as a leech + col.clear_queues(); + let queued = col.next_card()?.unwrap(); + dbg!(&queued); + col.answer_card(&CardAnswer { + card_id: queued.card.id, + current_state: queued.next_states.current, + new_state: queued.next_states.again, + rating: Rating::Again, + answered_at: TimestampMillis::now(), + milliseconds_taken: 0, + })?; + + let assert_post_review_state = |col: &mut Collection| -> Result<()> { + let card = col.storage.get_card(cid)?.unwrap(); + assert_eq!(card.interval, 1); + assert_eq!(card.lapses, 8); + + assert_eq!( + col.storage.get_all_revlog_entries(TimestampSecs(0))?.len(), + 2 + ); + + let note = col.storage.get_note(nid)?.unwrap(); + assert_eq!(note.tags, vec!["leech".to_string()]); + assert_eq!(col.storage.all_tags()?.is_empty(), false); + + Ok(()) + }; + + let assert_pre_review_state = |col: &mut Collection| -> Result<()> { + // the card should have its old state, but a new mtime (which we can't + // easily test without waiting) + let card = col.storage.get_card(cid)?.unwrap(); + assert_eq!(card.interval, 4); + assert_eq!(card.lapses, 7); + + // the revlog entry should have been removed + assert_eq!( + col.storage.get_all_revlog_entries(TimestampSecs(0))?.len(), + 1 + ); + + // the note should no longer be tagged as a leech + let note = col.storage.get_note(nid)?.unwrap(); + assert_eq!(note.tags.is_empty(), true); + assert_eq!(col.storage.all_tags()?.is_empty(), true); + + Ok(()) + }; + + // ensure everything is restored on undo/redo + assert_post_review_state(&mut col)?; + col.undo()?; + assert_pre_review_state(&mut col)?; + + col.redo()?; + assert_post_review_state(&mut col)?; + + col.undo()?; + assert_pre_review_state(&mut col)?; + col.undo()?; + assert_initial_state(&mut col)?; + + // fixme: make sure queue state updated, esp. on redo + + Ok(()) + } +} diff --git a/rslib/src/scheduler/bury_and_suspend.rs b/rslib/src/scheduler/bury_and_suspend.rs index 4fc93fa57..109176732 100644 --- a/rslib/src/scheduler/bury_and_suspend.rs +++ b/rslib/src/scheduler/bury_and_suspend.rs @@ -7,6 +7,7 @@ use crate::{ collection::Collection, config::SchedulerVersion, err::Result, + prelude::*, search::SortMode, }; @@ -130,6 +131,19 @@ impl Collection { col.bury_or_suspend_searched_cards(mode) }) } + + pub(crate) fn bury_siblings( + &mut self, + cid: CardID, + nid: NoteID, + include_new: bool, + include_reviews: bool, + ) -> Result<()> { + use pb::bury_or_suspend_cards_in::Mode; + self.storage + .search_siblings_for_bury(cid, nid, include_new, include_reviews)?; + self.bury_or_suspend_searched_cards(Mode::BurySched) + } } #[cfg(test)] diff --git a/rslib/src/scheduler/queue/mod.rs b/rslib/src/scheduler/queue/mod.rs index 8501b845d..8948808fe 100644 --- a/rslib/src/scheduler/queue/mod.rs +++ b/rslib/src/scheduler/queue/mod.rs @@ -247,8 +247,16 @@ impl Collection { })) } } + + #[cfg(test)] + pub(crate) fn next_card(&mut self) -> Result> { + Ok(self + .next_cards(1, false)? + .map(|mut resp| resp.cards.pop().unwrap())) + } } +#[derive(Debug)] pub(crate) struct QueuedCard { pub card: Card, pub kind: QueueEntryKind, diff --git a/rslib/src/storage/card/mod.rs b/rslib/src/storage/card/mod.rs index 932af4e90..0ea6de2b9 100644 --- a/rslib/src/storage/card/mod.rs +++ b/rslib/src/storage/card/mod.rs @@ -312,6 +312,28 @@ impl super::SqliteStorage { .collect() } + /// Place matching card ids into the search table. + pub(crate) fn search_siblings_for_bury( + &self, + cid: CardID, + nid: NoteID, + include_new: bool, + include_reviews: bool, + ) -> Result<()> { + self.setup_searched_cards_table()?; + self.db + .prepare_cached(include_str!("siblings_for_bury.sql"))? + .execute(params![ + cid, + nid, + include_new, + CardQueue::New as i8, + include_reviews, + CardQueue::Review as i8 + ])?; + Ok(()) + } + pub(crate) fn note_ids_of_cards(&self, cids: &[CardID]) -> Result> { let mut stmt = self .db diff --git a/rslib/src/storage/card/siblings_for_bury.sql b/rslib/src/storage/card/siblings_for_bury.sql new file mode 100644 index 000000000..4f8ad09e4 --- /dev/null +++ b/rslib/src/storage/card/siblings_for_bury.sql @@ -0,0 +1,15 @@ +INSERT INTO search_cids +SELECT id +FROM cards +WHERE id != ? + AND nid = ? + AND ( + ( + ? + AND queue = ? + ) + OR ( + ? + AND queue = ? + ) + ); \ No newline at end of file diff --git a/rslib/src/storage/note/get.sql b/rslib/src/storage/note/get.sql index fb1ba3619..53c8129ef 100644 --- a/rslib/src/storage/note/get.sql +++ b/rslib/src/storage/note/get.sql @@ -4,5 +4,7 @@ SELECT id, mod, usn, tags, - flds + flds, + cast(sfld AS text), + csum FROM notes \ No newline at end of file diff --git a/rslib/src/storage/note/mod.rs b/rslib/src/storage/note/mod.rs index c1a6e5aab..9ea67cf0d 100644 --- a/rslib/src/storage/note/mod.rs +++ b/rslib/src/storage/note/mod.rs @@ -21,19 +21,19 @@ pub(crate) fn join_fields(fields: &[String]) -> String { } fn row_to_note(row: &Row) -> Result { - Ok(Note { - id: row.get(0)?, - guid: row.get(1)?, - notetype_id: row.get(2)?, - mtime: row.get(3)?, - usn: row.get(4)?, - tags: split_tags(row.get_raw(5).as_str()?) + Ok(Note::new_from_storage( + row.get(0)?, + row.get(1)?, + row.get(2)?, + row.get(3)?, + row.get(4)?, + split_tags(row.get_raw(5).as_str()?) .map(Into::into) .collect(), - fields: split_fields(row.get_raw(6).as_str()?), - sort_field: None, - checksum: None, - }) + split_fields(row.get_raw(6).as_str()?), + Some(row.get(7)?), + Some(row.get(8).unwrap_or_default()), + )) } impl super::SqliteStorage { @@ -45,7 +45,7 @@ impl super::SqliteStorage { .transpose() } - /// Caller must call note.prepare_for_update() prior to calling this. + /// If fields have been modified, caller must call note.prepare_for_update() prior to calling this. pub(crate) fn update_note(&self, note: &Note) -> Result<()> { assert!(note.id.0 != 0); let mut stmt = self.db.prepare_cached(include_str!("update.sql"))?; diff --git a/rslib/src/storage/revlog/add.sql b/rslib/src/storage/revlog/add.sql index 13f12e073..fae7953b8 100644 --- a/rslib/src/storage/revlog/add.sql +++ b/rslib/src/storage/revlog/add.sql @@ -13,14 +13,15 @@ INSERT VALUES ( ( CASE - WHEN ?1 IN ( + WHEN ?1 + AND ?2 IN ( SELECT id FROM revlog ) THEN ( SELECT max(id) + 1 FROM revlog ) - ELSE ?1 + ELSE ?2 END ), ?, diff --git a/rslib/src/storage/revlog/mod.rs b/rslib/src/storage/revlog/mod.rs index 9d6ef3aaa..a93a5e299 100644 --- a/rslib/src/storage/revlog/mod.rs +++ b/rslib/src/storage/revlog/mod.rs @@ -59,10 +59,16 @@ impl SqliteStorage { Ok(()) } - pub(crate) fn add_revlog_entry(&self, entry: &RevlogEntry) -> Result<()> { + /// Returns the used id, which may differ if `ensure_unique` is true. + pub(crate) fn add_revlog_entry( + &self, + entry: &RevlogEntry, + ensure_unique: bool, + ) -> Result { self.db .prepare_cached(include_str!("add.sql"))? .execute(params![ + ensure_unique, entry.id, entry.cid, entry.usn, @@ -73,7 +79,7 @@ impl SqliteStorage { entry.taken_millis, entry.review_kind as u8 ])?; - Ok(()) + Ok(RevlogID(self.db.last_insert_rowid())) } pub(crate) fn get_revlog_entry(&self, id: RevlogID) -> Result> { @@ -84,6 +90,14 @@ impl SqliteStorage { .transpose() } + /// Only intended to be used by the undo code, as Anki can not sync revlog deletions. + pub(crate) fn remove_revlog_entry(&self, id: RevlogID) -> Result<()> { + self.db + .prepare_cached("delete from revlog where id = ?")? + .execute(&[id])?; + Ok(()) + } + pub(crate) fn get_revlog_entries_for_card(&self, cid: CardID) -> Result> { self.db .prepare_cached(concat!(include_str!("get.sql"), " where cid=?"))? diff --git a/rslib/src/storage/tag/mod.rs b/rslib/src/storage/tag/mod.rs index d3c56c2a2..f07c4f99d 100644 --- a/rslib/src/storage/tag/mod.rs +++ b/rslib/src/storage/tag/mod.rs @@ -65,7 +65,24 @@ impl SqliteStorage { .map_err(Into::into) } - pub(crate) fn clear_tag(&self, tag: &str) -> Result<()> { + // for undo in the future + #[allow(dead_code)] + pub(crate) fn get_tag_and_children(&self, name: &str) -> Result> { + self.db + .prepare_cached("select tag, usn, collapsed from tags where tag regexp ?")? + .query_and_then(&[format!("(?i)^{}($|::)", regex::escape(name))], row_to_tag)? + .collect() + } + + pub(crate) fn remove_single_tag(&self, tag: &str) -> Result<()> { + self.db + .prepare_cached("delete from tags where tag = ?")? + .execute(&[tag])?; + + Ok(()) + } + + pub(crate) fn clear_tag_and_children(&self, tag: &str) -> Result<()> { self.db .prepare_cached("delete from tags where tag regexp ?")? .execute(&[format!("(?i)^{}($|::)", regex::escape(tag))])?; @@ -81,7 +98,7 @@ impl SqliteStorage { Ok(()) } - pub(crate) fn clear_tags(&self) -> Result<()> { + pub(crate) fn clear_all_tags(&self) -> Result<()> { self.db.execute("delete from tags", NO_PARAMS)?; Ok(()) } diff --git a/rslib/src/sync/mod.rs b/rslib/src/sync/mod.rs index 49a4578fd..457687afb 100644 --- a/rslib/src/sync/mod.rs +++ b/rslib/src/sync/mod.rs @@ -904,7 +904,7 @@ impl Collection { Ok(()) } - fn merge_tags(&self, tags: Vec, latest_usn: Usn) -> Result<()> { + fn merge_tags(&mut self, tags: Vec, latest_usn: Usn) -> Result<()> { for tag in tags { self.register_tag(&mut Tag::new(tag, latest_usn))?; } @@ -925,7 +925,7 @@ impl Collection { fn merge_revlog(&self, entries: Vec) -> Result<()> { for entry in entries { - self.storage.add_revlog_entry(&entry)?; + self.storage.add_revlog_entry(&entry, false)?; } Ok(()) } @@ -1134,17 +1134,18 @@ impl From for CardEntry { impl From for Note { fn from(e: NoteEntry) -> Self { - Note { - id: e.id, - guid: e.guid, - notetype_id: e.ntid, - mtime: e.mtime, - usn: e.usn, - tags: split_tags(&e.tags).map(ToString::to_string).collect(), - fields: e.fields.split('\x1f').map(ToString::to_string).collect(), - sort_field: None, - checksum: None, - } + let fields = e.fields.split('\x1f').map(ToString::to_string).collect(); + Note::new_from_storage( + e.id, + e.guid, + e.ntid, + e.mtime, + e.usn, + split_tags(&e.tags).map(ToString::to_string).collect(), + fields, + None, + None, + ) } } @@ -1152,12 +1153,12 @@ impl From for NoteEntry { fn from(e: Note) -> Self { NoteEntry { id: e.id, + fields: e.fields().iter().join("\x1f"), guid: e.guid, ntid: e.notetype_id, mtime: e.mtime, usn: e.usn, tags: join_tags(&e.tags), - fields: e.fields.into_iter().join("\x1f"), sfld: String::new(), csum: String::new(), flags: 0, @@ -1324,7 +1325,7 @@ mod test { fn col1_setup(col: &mut Collection) { let nt = col.get_notetype_by_name("Basic").unwrap().unwrap(); let mut note = nt.new_note(); - note.fields[0] = "1".into(); + note.set_field(0, "1").unwrap(); col.add_note(&mut note, DeckID(1)).unwrap(); // // set our schema time back, so when initial server @@ -1392,18 +1393,21 @@ mod test { // add another note+card+tag let mut note = nt.new_note(); - note.fields[0] = "2".into(); + note.set_field(0, "2")?; note.tags.push("tag".into()); col1.add_note(&mut note, deck.id)?; // mock revlog entry - col1.storage.add_revlog_entry(&RevlogEntry { - id: TimestampMillis(123), - cid: CardID(456), - usn: Usn(-1), - interval: 10, - ..Default::default() - })?; + col1.storage.add_revlog_entry( + &RevlogEntry { + id: RevlogID(123), + cid: CardID(456), + usn: Usn(-1), + interval: 10, + ..Default::default() + }, + true, + )?; // config + creation col1.set_config("test", &"test1")?; @@ -1486,7 +1490,7 @@ mod test { // make some modifications let mut note = col2.storage.get_note(note.id)?.unwrap(); - note.fields[1] = "new".into(); + note.set_field(1, "new")?; note.tags.push("tag2".into()); col2.update_note(&mut note)?; diff --git a/rslib/src/tags.rs b/rslib/src/tags.rs index 7513fecac..3db2911e5 100644 --- a/rslib/src/tags.rs +++ b/rslib/src/tags.rs @@ -8,6 +8,7 @@ use crate::{ notes::{NoteID, TransformNoteOutput}, text::{normalize_to_nfc, to_re}, types::Usn, + undo::Undo, }; use regex::{NoExpand, Regex, Replacer}; @@ -195,7 +196,11 @@ impl Collection { /// Given a list of tags, fix case, ordering and duplicates. /// Returns true if any new tags were added. - pub(crate) fn canonify_tags(&self, tags: Vec, usn: Usn) -> Result<(Vec, bool)> { + pub(crate) fn canonify_tags( + &mut self, + tags: Vec, + usn: Usn, + ) -> Result<(Vec, bool)> { let mut seen = HashSet::new(); let mut added = false; @@ -223,7 +228,7 @@ impl Collection { /// in the tags list. True if the tag was added and not already in tag list. /// In the case the tag is already registered, tag will be mutated to match the existing /// name. - pub(crate) fn register_tag(&self, tag: &mut Tag) -> Result { + pub(crate) fn register_tag(&mut self, tag: &mut Tag) -> Result { let normalized_name = normalize_tag_name(&tag.name); if normalized_name.is_empty() { // this should not be possible @@ -238,11 +243,23 @@ impl Collection { } else if let Cow::Owned(new_name) = normalized_name { tag.name = new_name; } - self.storage.register_tag(&tag)?; + self.register_tag_inner(&tag)?; Ok(true) } } + /// Adds an already-validated tag to the DB and undo list. + /// Caller is responsible for setting usn. + pub(crate) fn register_tag_inner(&mut self, tag: &Tag) -> Result<()> { + self.save_undo(Box::new(AddedTag(tag.clone()))); + self.storage.register_tag(&tag) + } + + pub(crate) fn remove_single_tag(&mut self, tag: &Tag, _usn: Usn) -> Result<()> { + self.save_undo(Box::new(RemovedTag(tag.clone()))); + self.storage.remove_single_tag(&tag.name) + } + /// If parent tag(s) exist and differ in case, return a rewritten tag. fn adjusted_case_for_parents(&self, tag: &str) -> Result> { if let Some(parent_tag) = self.first_existing_parent_tag(&tag)? { @@ -271,7 +288,7 @@ impl Collection { pub fn clear_unused_tags(&self) -> Result<()> { let expanded: HashSet<_> = self.storage.expanded_tags()?.into_iter().collect(); - self.storage.clear_tags()?; + self.storage.clear_all_tags()?; let usn = self.usn()?; for name in self.storage.all_tags_in_notes()? { let name = normalize_tag_name(&name).into(); @@ -441,7 +458,7 @@ impl Collection { self.transact(None, |col| { // clear the existing original tags for (source_tag, _) in &source_tags_and_outputs { - col.storage.clear_tag(source_tag)?; + col.storage.clear_tag_and_children(source_tag)?; } col.transform_notes(&nids, |note, _nt| { @@ -464,6 +481,25 @@ impl Collection { } } +#[derive(Debug)] +struct AddedTag(Tag); + +#[derive(Debug)] +struct RemovedTag(Tag); + +impl Undo for AddedTag { + fn undo(self: Box, col: &mut Collection, usn: Usn) -> Result<()> { + col.remove_single_tag(&self.0, usn) + } +} + +impl Undo for RemovedTag { + fn undo(mut self: Box, col: &mut Collection, usn: Usn) -> Result<()> { + self.0.usn = usn; + col.register_tag_inner(&self.0) + } +} + #[cfg(test)] mod test { use super::*; @@ -575,11 +611,11 @@ mod test { assert_eq!(¬e.tags, &["barfoo", "foobar"]); // tag children are also cleared when clearing their parent - col.storage.clear_tags()?; + col.storage.clear_all_tags()?; for name in vec!["a", "a::b", "A::b::c"] { col.register_tag(&mut Tag::new(name.to_string(), Usn(0)))?; } - col.storage.clear_tag("a")?; + col.storage.clear_tag_and_children("a")?; assert_eq!(col.storage.all_tags()?, vec![]); Ok(()) @@ -624,7 +660,7 @@ mod test { // differing case should result in only one parent case being added - // the first one - col.storage.clear_tags()?; + col.storage.clear_all_tags()?; *(&mut note.tags[0]) = "foo::BAR::a".into(); *(&mut note.tags[1]) = "FOO::bar::b".into(); col.update_note(&mut note)?; @@ -642,7 +678,7 @@ mod test { ); // things should work even if the immediate parent is not missing - col.storage.clear_tags()?; + col.storage.clear_all_tags()?; *(&mut note.tags[0]) = "foo::bar::baz".into(); *(&mut note.tags[1]) = "foo::bar::baz::quux".into(); col.update_note(&mut note)?; @@ -661,7 +697,7 @@ mod test { // numbers have a smaller ascii number than ':', so a naive sort on // '::' would result in one::two being nested under one1. - col.storage.clear_tags()?; + col.storage.clear_all_tags()?; *(&mut note.tags[0]) = "one".into(); *(&mut note.tags[1]) = "one1".into(); note.tags.push("one::two".into()); @@ -676,7 +712,7 @@ mod test { ); // children should match the case of their parents - col.storage.clear_tags()?; + col.storage.clear_all_tags()?; *(&mut note.tags[0]) = "FOO".into(); *(&mut note.tags[1]) = "foo::BAR".into(); *(&mut note.tags[2]) = "foo::bar::baz".into(); diff --git a/rslib/src/undo.rs b/rslib/src/undo.rs index bc84add78..41a6608b1 100644 --- a/rslib/src/undo.rs +++ b/rslib/src/undo.rs @@ -6,17 +6,19 @@ use crate::{ err::Result, types::Usn, }; -use std::fmt; +use std::{collections::VecDeque, fmt}; -pub(crate) trait Undoable: fmt::Debug + Send { +const UNDO_LIMIT: usize = 30; + +pub(crate) trait Undo: fmt::Debug + Send { /// Undo the recorded action. - fn apply(&self, ctx: &mut Collection, usn: Usn) -> Result<()>; + fn undo(self: Box, col: &mut Collection, usn: Usn) -> Result<()>; } #[derive(Debug)] struct UndoStep { kind: CollectionOp, - changes: Vec>, + changes: Vec>, } #[derive(Debug, PartialEq)] @@ -34,14 +36,17 @@ impl Default for UndoMode { #[derive(Debug, Default)] pub(crate) struct UndoManager { - undo_steps: Vec, + // undo steps are added to the front of a double-ended queue, so we can + // efficiently cap the number of steps we retain in memory + undo_steps: VecDeque, + // redo steps are added to the end redo_steps: Vec, mode: UndoMode, current_step: Option, } impl UndoManager { - pub(crate) fn save_undoable(&mut self, item: Box) { + pub(crate) fn save(&mut self, item: Box) { if let Some(step) = self.current_step.as_mut() { step.changes.push(item) } @@ -67,7 +72,8 @@ impl UndoManager { if self.mode == UndoMode::Undoing { self.redo_steps.push(step); } else { - self.undo_steps.push(step); + self.undo_steps.truncate(UNDO_LIMIT - 1); + self.undo_steps.push_front(step); } } } @@ -77,11 +83,11 @@ impl UndoManager { } fn can_undo(&self) -> Option { - self.undo_steps.last().map(|s| s.kind.clone()) + self.undo_steps.front().map(|s| s.kind) } fn can_redo(&self) -> Option { - self.redo_steps.last().map(|s| s.kind.clone()) + self.redo_steps.last().map(|s| s.kind) } } @@ -95,13 +101,13 @@ impl Collection { } pub fn undo(&mut self) -> Result<()> { - if let Some(step) = self.state.undo.undo_steps.pop() { + 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(Some(step.kind), |col| { let usn = col.usn()?; - for change in changes.iter().rev() { - change.apply(col, usn)?; + for change in changes.into_iter().rev() { + change.undo(col, usn)?; } Ok(()) }); @@ -117,8 +123,8 @@ impl Collection { self.state.undo.mode = UndoMode::Redoing; let res = self.transact(Some(step.kind), |col| { let usn = col.usn()?; - for change in changes.iter().rev() { - change.apply(col, usn)?; + for change in changes.into_iter().rev() { + change.undo(col, usn)?; } Ok(()) }); @@ -127,4 +133,105 @@ impl Collection { } Ok(()) } + + #[inline] + pub(crate) fn save_undo(&mut self, item: Box) { + self.state.undo.save(item) + } +} + +#[cfg(test)] +mod test { + use crate::card::Card; + use crate::collection::{open_test_collection, CollectionOp}; + + #[test] + fn undo() { + let mut col = open_test_collection(); + + let mut card = Card::default(); + card.interval = 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.interval = 2; + Ok(()) + }) + .unwrap(); + assert_eq!(card.interval, 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.interval = i; + Ok(()) + }) + .unwrap(); + Ok(()) + }) + .unwrap(); + } + + assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 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().interval, 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().interval, 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().interval, 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().interval, 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().interval, 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.interval = 5; + Ok(()) + }) + .unwrap(); + Ok(()) + }) + .unwrap(); + assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 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); + } }