undo support for deck adding/removing

Work in progress - still to do:
- renames appear as 'Update Deck' - easiest way to solve it would
be to have a separate backend method for renames
- drag&drop of decks not yet undoable
- since the undo status is updated after the backend method ends,
the older checkpoint() calls need to be replaced with an
update_undo_status() at the end of the call - if we just remove the
checkpoint, then the menu doesn't get updated

ftl/core/undo.ftl

@@ -11,7 +11,9 @@ undo-action-redone = { $action } redone
 
 undo-answer-card = Answer Card
 undo-unbury-unsuspend = Unbury/Unsuspend
+undo-add-deck = Add Deck
 undo-add-note = Add Note
 undo-update-tag = Update Tag
 undo-update-note = Update Note
 undo-update-card = Update Card
+undo-update-deck = Update Deck

qt/aqt/deckbrowser.py

@@ -258,7 +258,6 @@ class DeckBrowser:
         self.mw.onExport(did=did)
 
     def _rename(self, did: int) -> None:
-        self.mw.checkpoint(tr(TR.ACTIONS_RENAME_DECK))
         deck = self.mw.col.decks.get(did)
         oldName = deck["name"]
         newName = getOnlyText(tr(TR.DECKS_NEW_DECK_NAME), default=oldName)
@@ -271,6 +270,7 @@ class DeckBrowser:
         except DeckIsFilteredError as err:
             showWarning(str(err))
             return
+        self.mw.update_undo_actions()
         self.show()
 
     def _options(self, did: str) -> None:
@@ -318,10 +318,10 @@ class DeckBrowser:
                 return self.mw.col.decks.rem(did, True)
 
             def on_done(fut: Future) -> None:
+                self.mw.update_undo_actions()
                 self.show()
                 res = fut.result()  # Required to check for errors
 
-            self.mw.checkpoint(tr(TR.DECKS_DELETE_DECK))
             self.mw.taskman.with_progress(do_delete, on_done)
 
     # Top buttons

qt/aqt/sidebar.py

@@ -989,7 +989,6 @@ class SidebarTreeView(QTreeView):
         new_name = new_name.replace('"', "")
         if not new_name or new_name == old_name:
             return
-        self.mw.checkpoint(tr(TR.ACTIONS_RENAME_DECK))
         try:
             self.mw.col.decks.rename(deck, new_name)
         except DeckIsFilteredError as err:
@@ -997,6 +996,7 @@ class SidebarTreeView(QTreeView):
             return
         self.refresh()
         self.mw.deckBrowser.refresh()
+        self.mw.update_undo_actions()
 
     def remove_tag(self, item: SidebarItem) -> None:
         self.browser.editor.saveNow(lambda: self._remove_tag(item))
@@ -1063,7 +1063,6 @@ class SidebarTreeView(QTreeView):
                 self.refresh()
                 res = fut.result()  # Required to check for errors
 
-            self.mw.checkpoint(tr(TR.DECKS_DELETE_DECK))
             self.browser.model.beginReset()
             self.mw.taskman.run_in_background(do_delete, on_done)
 

rslib/src/decks/mod.rs

@@ -10,7 +10,10 @@ pub use crate::backend_proto::{
     deck_kind::Kind as DeckKind, filtered_search_term::FilteredSearchOrder, Deck as DeckProto,
     DeckCommon, DeckKind as DeckKindProto, FilteredDeck, FilteredSearchTerm, NormalDeck,
 };
-use crate::{backend_proto as pb, markdown::render_markdown, text::sanitize_html_no_images};
+use crate::{
+    backend_proto as pb, markdown::render_markdown, text::sanitize_html_no_images,
+    undo::UndoableOpKind,
+};
 use crate::{
     collection::Collection,
     deckconf::DeckConfID,
@@ -263,21 +266,38 @@ impl Collection {
     }
 
     /// Add or update an existing deck modified by the user. May add parents,
-    /// or rename children as required.
+    /// or rename children as required. Prefer add_deck() or update_deck() to
+    /// be explicit about your intentions; this function mainly exists so we
+    /// can integrate with older Python code that behaved this way.
     pub(crate) fn add_or_update_deck(&mut self, deck: &mut Deck) -> Result<()> {
-        self.state.deck_cache.clear();
+        if deck.id.0 == 0 {
+            self.add_deck(deck)
+        } else {
+            self.update_deck(deck)
+        }
+    }
 
-        self.transact(None, |col| {
+    /// Add a new deck. The id must be 0, as it will be automatically assigned.
+    pub fn add_deck(&mut self, deck: &mut Deck) -> Result<()> {
+        if deck.id.0 != 0 {
+            return Err(AnkiError::invalid_input("deck to add must have id 0"));
+        }
+
+        self.transact(Some(UndoableOpKind::AddDeck), |col| {
             let usn = col.usn()?;
-
             col.prepare_deck_for_update(deck, usn)?;
             deck.set_modified(usn);
+            col.match_or_create_parents(deck, usn)?;
+            col.add_deck_undoable(deck)
+        })
+    }
 
-            if deck.id.0 == 0 {
+    pub(crate) fn update_deck(&mut self, deck: &mut Deck) -> Result<()> {
-                // TODO: undo support
+        self.transact(Some(UndoableOpKind::UpdateDeck), |col| {
-                col.match_or_create_parents(deck, usn)?;
+            let usn = col.usn()?;
-                col.storage.add_deck(deck)
+            col.prepare_deck_for_update(deck, usn)?;
-            } else if let Some(existing_deck) = col.storage.get_deck(deck.id)? {
+            deck.set_modified(usn);
+            if let Some(existing_deck) = col.storage.get_deck(deck.id)? {
                 let name_changed = existing_deck.name != deck.name;
                 if name_changed {
                     // match closest parent name
@@ -301,15 +321,13 @@ impl Collection {
     /// Add/update a single deck when syncing/importing. Ensures name is unique
     /// & normalized, but does not check parents/children or update mtime
     /// (unless the name was changed). Caller must set up transaction.
-    /// TODO: undo support
     pub(crate) fn add_or_update_single_deck_with_existing_id(
         &mut self,
         deck: &mut Deck,
         usn: Usn,
     ) -> Result<()> {
-        self.state.deck_cache.clear();
         self.prepare_deck_for_update(deck, usn)?;
-        self.storage.add_or_update_deck_with_existing_id(deck)
+        self.add_or_update_deck_with_existing_id_undoable(deck)
     }
 
     pub(crate) fn ensure_deck_name_unique(&self, deck: &mut Deck, usn: Usn) -> Result<()> {
@@ -367,12 +385,11 @@ impl Collection {
 
     /// Add a single, normal deck with the provided name for a child deck.
     /// Caller must have done necessarily validation on name.
-    fn add_parent_deck(&self, machine_name: &str, usn: Usn) -> Result<()> {
+    fn add_parent_deck(&mut self, machine_name: &str, usn: Usn) -> Result<()> {
         let mut deck = Deck::new_normal();
         deck.name = machine_name.into();
         deck.set_modified(usn);
-        // fixme: undo
+        self.add_deck_undoable(&mut deck)
-        self.storage.add_deck(&mut deck)
     }
 
     /// If parent deck(s) exist, rewrite name to match their case.
@@ -404,7 +421,7 @@ impl Collection {
         }
     }
 
-    fn create_missing_parents(&self, mut machine_name: &str, usn: Usn) -> Result<()> {
+    fn create_missing_parents(&mut self, mut machine_name: &str, usn: Usn) -> Result<()> {
         while let Some(parent_name) = immediate_parent_name(machine_name) {
             if self.storage.get_deck_id(parent_name)?.is_none() {
                 self.add_parent_deck(parent_name, usn)?;
@@ -441,12 +458,8 @@ impl Collection {
     }
 
     pub fn remove_deck_and_child_decks(&mut self, did: DeckID) -> Result<()> {
-        // fixme: vet cache clearing
+        self.transact(Some(UndoableOpKind::RemoveDeck), |col| {
-        self.state.deck_cache.clear();
-
-        self.transact(None, |col| {
             let usn = col.usn()?;
-
             if let Some(deck) = col.storage.get_deck(did)? {
                 let child_decks = col.storage.child_decks(&deck)?;
 
@@ -463,23 +476,20 @@ impl Collection {
     }
 
     pub(crate) fn remove_single_deck(&mut self, deck: &Deck, usn: Usn) -> Result<()> {
-        // fixme: undo
         match deck.kind {
             DeckKind::Normal(_) => self.delete_all_cards_in_normal_deck(deck.id)?,
             DeckKind::Filtered(_) => self.return_all_cards_in_filtered_deck(deck.id)?,
         }
         self.clear_aux_config_for_deck(deck.id)?;
         if deck.id.0 == 1 {
+            // if deleting the default deck, ensure there's a new one, and avoid the grave
             let mut deck = deck.to_owned();
-            // fixme: separate key
             deck.name = self.i18n.tr(TR::DeckConfigDefaultName).into();
             deck.set_modified(usn);
-            self.add_or_update_single_deck_with_existing_id(&mut deck, usn)?;
+            self.add_or_update_single_deck_with_existing_id(&mut deck, usn)
         } else {
-            self.storage.remove_deck(deck.id)?;
+            self.remove_deck_and_add_grave_undoable(deck.clone(), usn)
-            self.storage.add_deck_grave(deck.id, usn)?;
         }
-        Ok(())
     }
 
     fn delete_all_cards_in_normal_deck(&mut self, did: DeckID) -> Result<()> {

rslib/src/decks/undo.rs

@@ -6,12 +6,17 @@ use crate::prelude::*;
 #[derive(Debug)]
 
 pub(crate) enum UndoableDeckChange {
+    Added(Box<Deck>),
     Updated(Box<Deck>),
+    Removed(Box<Deck>),
+    GraveAdded(Box<(DeckID, Usn)>),
+    GraveRemoved(Box<(DeckID, Usn)>),
 }
 
 impl Collection {
     pub(crate) fn undo_deck_change(&mut self, change: UndoableDeckChange) -> Result<()> {
         match change {
+            UndoableDeckChange::Added(deck) => self.remove_deck_undoable(*deck),
             UndoableDeckChange::Updated(mut deck) => {
                 let current = self
                     .storage
@@ -19,9 +24,28 @@ impl Collection {
                     .ok_or_else(|| AnkiError::invalid_input("deck disappeared"))?;
                 self.update_single_deck_undoable(&mut *deck, &current)
             }
+            UndoableDeckChange::Removed(deck) => self.restore_deleted_deck(*deck),
+            UndoableDeckChange::GraveAdded(e) => self.remove_deck_grave(e.0, e.1),
+            UndoableDeckChange::GraveRemoved(e) => self.add_deck_grave_undoable(e.0, e.1),
         }
     }
 
+    pub(super) fn add_deck_undoable(&mut self, deck: &mut Deck) -> Result<(), AnkiError> {
+        self.storage.add_deck(deck)?;
+        self.save_undo(UndoableDeckChange::Added(Box::new(deck.clone())));
+        Ok(())
+    }
+
+    pub(super) fn add_or_update_deck_with_existing_id_undoable(
+        &mut self,
+        deck: &mut Deck,
+    ) -> Result<(), AnkiError> {
+        self.state.deck_cache.clear();
+        self.storage.add_or_update_deck_with_existing_id(deck)?;
+        self.save_undo(UndoableDeckChange::Added(Box::new(deck.clone())));
+        Ok(())
+    }
+
     /// Update an individual, existing deck. Caller is responsible for ensuring deck
     /// is normalized, matches parents, is not a duplicate name, and bumping mtime.
     /// Clears deck cache.
@@ -34,4 +58,38 @@ impl Collection {
         self.save_undo(UndoableDeckChange::Updated(Box::new(original.clone())));
         self.storage.update_deck(deck)
     }
+
+    pub(crate) fn remove_deck_and_add_grave_undoable(
+        &mut self,
+        deck: Deck,
+        usn: Usn,
+    ) -> Result<()> {
+        self.state.deck_cache.clear();
+        self.add_deck_grave_undoable(deck.id, usn)?;
+        self.storage.remove_deck(deck.id)?;
+        self.save_undo(UndoableDeckChange::Removed(Box::new(deck)));
+        Ok(())
+    }
+
+    fn restore_deleted_deck(&mut self, deck: Deck) -> Result<()> {
+        self.storage.add_or_update_deck_with_existing_id(&deck)?;
+        self.save_undo(UndoableDeckChange::Added(Box::new(deck)));
+        Ok(())
+    }
+
+    fn remove_deck_undoable(&mut self, deck: Deck) -> Result<()> {
+        self.storage.remove_deck(deck.id)?;
+        self.save_undo(UndoableDeckChange::Removed(Box::new(deck)));
+        Ok(())
+    }
+
+    fn add_deck_grave_undoable(&mut self, did: DeckID, usn: Usn) -> Result<()> {
+        self.save_undo(UndoableDeckChange::GraveAdded(Box::new((did, usn))));
+        self.storage.add_deck_grave(did, usn)
+    }
+
+    fn remove_deck_grave(&mut self, did: DeckID, usn: Usn) -> Result<()> {
+        self.save_undo(UndoableDeckChange::GraveRemoved(Box::new((did, usn))));
+        self.storage.remove_deck_grave(did)
+    }
 }

rslib/src/storage/deck/mod.rs

@@ -138,8 +138,8 @@ impl SqliteStorage {
         }
     }
 
-    /// Used for syncing; will keep existing ID. Shouldn't be used to add new decks locally,
+    /// Used for syncing&undo; will keep existing ID. Shouldn't be used to add
-    /// since it does not allocate an id.
+    /// new decks locally, since it does not allocate an id.
     pub(crate) fn add_or_update_deck_with_existing_id(&self, deck: &Deck) -> Result<()> {
         if deck.id.0 == 0 {
             return Err(AnkiError::invalid_input("deck with id 0"));

rslib/src/storage/graves/mod.rs

@@ -48,6 +48,10 @@ impl SqliteStorage {
         self.remove_grave(nid.0, GraveKind::Note)
     }
 
+    pub(crate) fn remove_deck_grave(&self, did: DeckID) -> Result<()> {
+        self.remove_grave(did.0, GraveKind::Deck)
+    }
+
     pub(crate) fn pending_graves(&self, pending_usn: Usn) -> Result<Graves> {
         let mut stmt = self.db.prepare(&format!(
             "select oid, type from graves where {}",

rslib/src/undo/ops.rs

@@ -5,16 +5,19 @@ use crate::prelude::*;
 
 #[derive(Debug, Clone, Copy, PartialEq)]
 pub enum UndoableOpKind {
-    UpdateCard,
+    AddDeck,
+    AddNote,
     AnswerCard,
     Bury,
+    RemoveDeck,
+    RemoveNote,
     Suspend,
     UnburyUnsuspend,
-    AddNote,
+    UpdateCard,
-    RemoveNote,
+    UpdateDeck,
-    UpdateTag,
     UpdateNote,
     UpdatePreferences,
+    UpdateTag,
 }
 
 impl UndoableOpKind {
@@ -26,16 +29,19 @@ impl UndoableOpKind {
 impl Collection {
     pub fn describe_op_kind(&self, op: UndoableOpKind) -> String {
         let key = match op {
-            UndoableOpKind::UpdateCard => TR::UndoUpdateCard,
+            UndoableOpKind::AddDeck => TR::UndoAddDeck,
+            UndoableOpKind::AddNote => TR::UndoAddNote,
             UndoableOpKind::AnswerCard => TR::UndoAnswerCard,
             UndoableOpKind::Bury => TR::StudyingBury,
+            UndoableOpKind::RemoveDeck => TR::DecksDeleteDeck,
+            UndoableOpKind::RemoveNote => TR::StudyingDeleteNote,
             UndoableOpKind::Suspend => TR::StudyingSuspend,
             UndoableOpKind::UnburyUnsuspend => TR::UndoUnburyUnsuspend,
-            UndoableOpKind::AddNote => TR::UndoAddNote,
+            UndoableOpKind::UpdateCard => TR::UndoUpdateCard,
-            UndoableOpKind::RemoveNote => TR::StudyingDeleteNote,
+            UndoableOpKind::UpdateDeck => TR::UndoUpdateDeck,
-            UndoableOpKind::UpdateTag => TR::UndoUpdateTag,
             UndoableOpKind::UpdateNote => TR::UndoUpdateNote,
             UndoableOpKind::UpdatePreferences => TR::PreferencesPreferences,
+            UndoableOpKind::UpdateTag => TR::UndoUpdateTag,
         };
 
         self.i18n.tr(key).to_string()