initial work on undoing reviews+burying siblings

- fetch sfld and csum when fetching notes, to make it cheaper
to write them back out unmodified
- make `fields` private, and access it via accessors, so we can
still catch when fields have been mutated without calling
prepare_for_update()
- fix python importing code passing a string in as the checksum

pylib/anki/importing/noteimp.py

@@ -250,7 +250,7 @@ class NoteImporter(Importer):
             self.col.tags.join(n.tags),
             n.fieldsStr,
             "",
-            "",
+            0,
             0,
             "",
         ]

rslib/src/backend/mod.rs

@@ -1449,7 +1449,7 @@ impl BackendService for Backend {
     fn clear_tag(&self, tag: pb::String) -> BackendResult<pb::Empty> {
         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())
             })
         })

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 {
+impl Undo for CardUpdated {
-    fn apply(&self, col: &mut crate::collection::Collection, usn: Usn) -> Result<()> {
+    fn undo(self: Box<Self>, 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
+        self.save_undo(Box::new(CardUpdated(original.clone())));
-            .undo
-            .save_undoable(Box::new(UpdateCardUndo(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);
-    }
-}

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 {

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!(&note.fields, &["a", "b; c; d"]);
+        assert_eq!(&note.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!(&note.fields, &["a", ""]);
+        assert_eq!(&note.fields()[..], &["a", ""]);
 
         Ok(())
     }

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)?;

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.set_field(0, "one aaa")?;
-        note.fields[1] = "two aaa".into();
+        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!(&note.fields, &["one BBB", "two BBB"]);
+        assert_eq!(&note.fields()[..], &["one BBB", "two BBB"]);
 
         let note2 = col.storage.get_note(note2.id)?.unwrap();
-        assert_eq!(&note2.fields, &["three BBB", ""]);
+        assert_eq!(&note2.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!(&note.fields, &["one ccc", "two BBB"]);
+        assert_eq!(&note.fields()[..], &["one ccc", "two BBB"]);
 
         Ok(())
     }

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<String>,
-    pub(crate) fields: Vec<String>,
+    fields: Vec<String>,
     pub(crate) sort_field: Option<String>,
     pub(crate) checksum: Option<u32>,
 }
@@ -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<String>,
+        fields: Vec<String>,
+        sort_field: Option<String>,
+        checksum: Option<u32>,
+    ) -> Self {
+        Self {
+            id,
+            guid,
+            notetype_id,
+            mtime,
+            usn,
+            tags,
+            fields,
+            sort_field,
+            checksum,
+        }
+    }
+
     pub fn fields(&self) -> &Vec<String> {
         &self.fields
     }
 
+    pub(crate) fn fields_mut(&mut self) -> &mut Vec<String> {
+        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<String>) -> 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,14 +323,11 @@ impl Collection {
     }
 
     pub fn update_note(&mut self, note: &mut Note) -> Result<()> {
-        if let Some(existing_note) = self.storage.get_note(note.id)? {
+        let existing_note = self.storage.get_note(note.id)?.ok_or(AnkiError::NotFound)?;
         if &existing_note == note {
             // nothing to do
             return Ok(());
         }
-        } else {
-            return Err(AnkiError::NotFound);
-        }
 
         self.transact(None, |col| {
             let nt = 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<Usn>,
+    ) -> 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<Self>, 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(), &current, Some(usn))
+    }
+}
+
 #[cfg(test)]
 mod test {
     use super::{anki_base91, field_checksum};

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();

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);
+        assert_eq!(note.fields().len(), 2);
-        note.fields = vec!["one".into(), "two".into()];
+        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!(
+        assert_eq!(note.fields(), &["one".to_string(), "two".into(), "".into()]);
-            note.fields,
-            vec!["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);
+        assert_eq!(note.fields().len(), 2);
-        note.fields = vec!["one".into(), "two".into()];
+        note.set_field(0, "one")?;
+        note.set_field(1, "two")?;
         col.add_note(&mut note, DeckID(1))?;
 
         assert_eq!(

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

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<TimestampMillis> 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<RevlogID> {
+        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<Self>, 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<Self>, 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(())
     }
 }

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<u64> {
-    if *crate::timestamp::TESTING {
+    if *crate::timestamp::TESTING || cfg!(test) {
         None
     } else {
         Some((card.id.0 as u64).wrapping_add(card.reps as u64))

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()

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,

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(())
+    }
+}

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)]

rslib/src/scheduler/queue/mod.rs

@@ -247,8 +247,16 @@ impl Collection {
             }))
         }
     }
+
+    #[cfg(test)]
+    pub(crate) fn next_card(&mut self) -> Result<Option<QueuedCard>> {
+        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,

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<HashSet<NoteID>> {
         let mut stmt = self
             .db

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 = ?
+    )
+  );

rslib/src/storage/note/get.sql

@@ -4,5 +4,7 @@ SELECT id,
   mod,
   usn,
   tags,
-  flds
+  flds,
+  cast(sfld AS text),
+  csum
 FROM notes

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<Note> {
-    Ok(Note {
+    Ok(Note::new_from_storage(
-        id: row.get(0)?,
+        row.get(0)?,
-        guid: row.get(1)?,
+        row.get(1)?,
-        notetype_id: row.get(2)?,
+        row.get(2)?,
-        mtime: row.get(3)?,
+        row.get(3)?,
-        usn: row.get(4)?,
+        row.get(4)?,
-        tags: split_tags(row.get_raw(5).as_str()?)
+        split_tags(row.get_raw(5).as_str()?)
             .map(Into::into)
             .collect(),
-        fields: split_fields(row.get_raw(6).as_str()?),
+        split_fields(row.get_raw(6).as_str()?),
-        sort_field: None,
+        Some(row.get(7)?),
-        checksum: None,
+        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"))?;

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
     ),
     ?,

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<RevlogID> {
         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<Option<RevlogEntry>> {
@@ -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<Vec<RevlogEntry>> {
         self.db
             .prepare_cached(concat!(include_str!("get.sql"), " where cid=?"))?

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<Vec<Tag>> {
+        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(())
     }

rslib/src/sync/mod.rs

@@ -904,7 +904,7 @@ impl Collection {
         Ok(())
     }
 
-    fn merge_tags(&self, tags: Vec<String>, latest_usn: Usn) -> Result<()> {
+    fn merge_tags(&mut self, tags: Vec<String>, 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<RevlogEntry>) -> 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<Card> for CardEntry {
 
 impl From<NoteEntry> for Note {
     fn from(e: NoteEntry) -> Self {
-        Note {
+        let fields = e.fields.split('\x1f').map(ToString::to_string).collect();
-            id: e.id,
+        Note::new_from_storage(
-            guid: e.guid,
+            e.id,
-            notetype_id: e.ntid,
+            e.guid,
-            mtime: e.mtime,
+            e.ntid,
-            usn: e.usn,
+            e.mtime,
-            tags: split_tags(&e.tags).map(ToString::to_string).collect(),
+            e.usn,
-            fields: e.fields.split('\x1f').map(ToString::to_string).collect(),
+            split_tags(&e.tags).map(ToString::to_string).collect(),
-            sort_field: None,
+            fields,
-            checksum: None,
+            None,
-        }
+            None,
+        )
     }
 }
 
@@ -1152,12 +1153,12 @@ impl From<Note> 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 {
+        col1.storage.add_revlog_entry(
-            id: TimestampMillis(123),
+            &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)?;
 

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<String>, usn: Usn) -> Result<(Vec<String>, bool)> {
+    pub(crate) fn canonify_tags(
+        &mut self,
+        tags: Vec<String>,
+        usn: Usn,
+    ) -> Result<(Vec<String>, 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<bool> {
+    pub(crate) fn register_tag(&mut self, tag: &mut Tag) -> Result<bool> {
         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<Option<String>> {
         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<Self>, col: &mut Collection, usn: Usn) -> Result<()> {
+        col.remove_single_tag(&self.0, usn)
+    }
+}
+
+impl Undo for RemovedTag {
+    fn undo(mut self: Box<Self>, 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!(&note.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();

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<Self>, col: &mut Collection, usn: Usn) -> Result<()>;
 }
 
 #[derive(Debug)]
 struct UndoStep {
     kind: CollectionOp,
-    changes: Vec<Box<dyn Undoable>>,
+    changes: Vec<Box<dyn Undo>>,
 }
 
 #[derive(Debug, PartialEq)]
@@ -34,14 +36,17 @@ impl Default for UndoMode {
 
 #[derive(Debug, Default)]
 pub(crate) struct UndoManager {
-    undo_steps: Vec<UndoStep>,
+    // 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<UndoStep>,
+    // redo steps are added to the end
     redo_steps: Vec<UndoStep>,
     mode: UndoMode,
     current_step: Option<UndoStep>,
 }
 
 impl UndoManager {
-    pub(crate) fn save_undoable(&mut self, item: Box<dyn Undoable>) {
+    pub(crate) fn save(&mut self, item: Box<dyn Undo>) {
         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<CollectionOp> {
-        self.undo_steps.last().map(|s| s.kind.clone())
+        self.undo_steps.front().map(|s| s.kind)
     }
 
     fn can_redo(&self) -> Option<CollectionOp> {
-        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() {
+                for change in changes.into_iter().rev() {
-                    change.apply(col, usn)?;
+                    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() {
+                for change in changes.into_iter().rev() {
-                    change.apply(col, usn)?;
+                    change.undo(col, usn)?;
                 }
                 Ok(())
             });
@@ -127,4 +133,105 @@ impl Collection {
         }
         Ok(())
     }
+
+    #[inline]
+    pub(crate) fn save_undo(&mut self, item: Box<dyn Undo>) {
+        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);
+    }
 }