fix renaming corner cases and decks.update()

- .update() should update a single deck and preserve usn by default,
as that's what existing code expects
- decks are automatically renamed when they conflict with an existing
name

pylib/anki/decks.py

@@ -75,7 +75,7 @@ class DeckManager:
             self.update_config(g)
             return
         else:
-            self.update(g)
+            self.update(g, preserve_usn=False)
 
     # legacy
     def flush(self):
@@ -97,7 +97,7 @@ class DeckManager:
 
         deck = self.new_deck_legacy(bool(type))
         deck["name"] = name
-        self.update(deck)
+        self.update(deck, preserve_usn=False)
 
         # fixme
         hooks.deck_added(deck)
@@ -205,7 +205,7 @@ class DeckManager:
             return self.get_legacy(id)
         return None
 
-    def update(self, g: Dict[str, Any], preserve_usn=False) -> None:
+    def update(self, g: Dict[str, Any], preserve_usn=True) -> None:
         "Add or update an existing deck. Used for syncing and merging."
         try:
             self.col.backend.add_or_update_deck_legacy(g, preserve_usn)
@@ -217,12 +217,9 @@ class DeckManager:
     def rename(self, g: Dict[str, Any], newName: str) -> None:
         "Rename deck prefix to NAME if not exists. Updates children."
         g["name"] = newName
-        self.update(g)
+        self.update(g, preserve_usn=False)
         return
 
-        # fixme: ensure rename of b in a::b::c generates new b
-        # fixme: renaming may have altered active did order
-
     def renameForDragAndDrop(self, draggedDeckDid: int, ontoDeckDid: Any) -> None:
         draggedDeck = self.get(draggedDeckDid)
         draggedDeckName = draggedDeck["name"]

pylib/tests/test_decks.py

@@ -72,8 +72,10 @@ def test_rename():
     assert "hello::world" not in names
     # create another deck
     id = d.decks.id("tmp")
-    # we can't rename it if it conflicts
-    assertException(Exception, lambda: d.decks.rename(d.decks.get(id), "foo"))
+    # automatically adjusted if a duplicate name
+    d.decks.rename(d.decks.get(id), "FOO")
+    names = [n.name for n in d.decks.all_names_and_ids()]
+    assert "FOO+" in names
     # when renaming, the children should be renamed too
     d.decks.id("one::two::three")
     id = d.decks.id("one")
@@ -88,11 +90,6 @@ def test_rename():
     child = d.decks.get(childId)
     assertException(DeckRenameError, lambda: d.decks.rename(child, "filtered::child"))
     assertException(DeckRenameError, lambda: d.decks.rename(child, "FILTERED::child"))
-    # changing case
-    parentId = d.decks.id("PARENT")
-    d.decks.id("PARENT::CHILD")
-    assertException(DeckRenameError, lambda: d.decks.rename(child, "PARENT::CHILD"))
-    assertException(DeckRenameError, lambda: d.decks.rename(child, "PARENT::child"))
 
 
 def test_renameForDragAndDrop():
@@ -137,18 +134,10 @@ def test_renameForDragAndDrop():
     d.decks.renameForDragAndDrop(chinese_did, None)
     assert deckNames() == ["Chinese", "Chinese::HSK", "Languages"]
 
-    # can't drack a deck where sibling have same name
-    new_hsk_did = d.decks.id("HSK")
-    assertException(
-        DeckRenameError, lambda: d.decks.renameForDragAndDrop(new_hsk_did, chinese_did)
-    )
-    d.decks.rem(new_hsk_did)
-
-    # can't drack a deck where sibling have same name different case
+    # decks are renamed if necessary
     new_hsk_did = d.decks.id("hsk")
-    assertException(
-        DeckRenameError, lambda: d.decks.renameForDragAndDrop(new_hsk_did, chinese_did)
-    )
+    d.decks.renameForDragAndDrop(new_hsk_did, chinese_did)
+    assert deckNames() == ["Chinese", "Chinese::HSK", "Chinese::hsk+", "Languages"]
     d.decks.rem(new_hsk_did)
 
     # '' is a convenient alias for the top level DID

rslib/src/backend/mod.rs

@@ -1046,7 +1046,14 @@ impl Backend {
         self.with_col(|col| {
             let schema11: DeckSchema11 = serde_json::from_slice(&input.deck)?;
             let mut deck: Deck = schema11.into();
-            col.add_or_update_deck(&mut deck, input.preserve_usn_and_mtime)?;
+            if input.preserve_usn_and_mtime {
+                col.transact(None, |col| {
+                    let usn = col.usn()?;
+                    col.add_or_update_single_deck(&mut deck, usn)
+                })?;
+            } else {
+                col.add_or_update_deck(&mut deck)?;
+            }
             Ok(deck.id.0)
         })
     }

rslib/src/dbcheck.rs

@@ -154,8 +154,9 @@ impl Collection {
     }
 
     fn check_missing_deck_ids(&mut self, out: &mut CheckDatabaseOutput) -> Result<()> {
+        let usn = self.usn()?;
         for did in self.storage.missing_decks()? {
-            self.recover_missing_deck(did)?;
+            self.recover_missing_deck(did, usn)?;
             out.decks_missing += 1;
         }
         Ok(())

rslib/src/decks/mod.rs

@@ -89,16 +89,6 @@ impl Deck {
         self.name.replace("\x1f", "::")
     }
 
-    pub(crate) fn prepare_for_update(&mut self) {
-        // fixme - we currently only do this when converting from human; should be done in pub methods instead
-
-        // if self.name.contains(invalid_char_for_deck_component) {
-        //     self.name = self.name.replace(invalid_char_for_deck_component, "");
-        // }
-        // ensure_string_in_nfc(&mut self.name);
-    }
-
-    // fixme: unify with prepare for update
     pub(crate) fn set_modified(&mut self, usn: Usn) {
         self.mtime_secs = TimestampSecs::now();
         self.usn = usn;
@@ -115,11 +105,11 @@ impl Deck {
     }
 }
 
-// fixme: need to bump usn on upgrade if we rename
 fn invalid_char_for_deck_component(c: char) -> bool {
     c.is_ascii_control() || c == '"'
 }
 
+// fixme: need to bump usn on upgrade if we rename
 fn normalized_deck_name_component(comp: &str) -> Cow<str> {
     let mut out = normalize_to_nfc(comp);
     if out.contains(invalid_char_for_deck_component) {
@@ -127,15 +117,29 @@ fn normalized_deck_name_component(comp: &str) -> Cow<str> {
     }
     let trimmed = out.trim();
     if trimmed.is_empty() {
-        "blank".into()
+        "blank".to_string().into()
     } else if trimmed.len() != out.len() {
-        // fixme: trimming leading/trailing spaces may break old clients if we don't bump mod
         trimmed.to_string().into()
     } else {
         out
     }
 }
 
+fn normalize_native_name(name: &str) -> Cow<str> {
+    if name
+        .split('\x1f')
+        .any(|comp| matches!(normalized_deck_name_component(comp), Cow::Owned(_)))
+    {
+        name.split('\x1f')
+            .map(normalized_deck_name_component)
+            .collect::<String>()
+            .into()
+    } else {
+        // no changes required
+        name.into()
+    }
+}
+
 pub(crate) fn human_deck_name_to_native(name: &str) -> String {
     let mut out = String::with_capacity(name.len());
     for comp in name.split("::") {
@@ -146,7 +150,6 @@ pub(crate) fn human_deck_name_to_native(name: &str) -> String {
 }
 
 impl Collection {
-    // fixme: this cache may belong in CardGenContext?
     pub(crate) fn get_deck(&mut self, did: DeckID) -> Result<Option<Arc<Deck>>> {
         if let Some(deck) = self.state.deck_cache.get(&did) {
             return Ok(Some(deck.clone()));
@@ -192,43 +195,52 @@ impl Collection {
         self.storage.deck_is_empty(DeckID(1))
     }
 
-    pub(crate) fn add_or_update_deck(&mut self, deck: &mut Deck, preserve_usn: bool) -> Result<()> {
-        // fixme: vet cache clearing
+    /// Normalize deck name and rename if not unique. Bumps mtime and usn if
+    /// deck was modified.
+    fn prepare_deck_for_update(&mut self, deck: &mut Deck, usn: Usn) -> Result<()> {
+        if let Cow::Owned(name) = normalize_native_name(&deck.name) {
+            deck.name = name;
+            deck.set_modified(usn);
+        }
+        self.ensure_deck_name_unique(deck, usn)
+    }
+
+    /// Add or update an existing deck modified by the user. May add parents,
+    /// or rename children as required.
+    pub(crate) fn add_or_update_deck(&mut self, deck: &mut Deck) -> Result<()> {
         self.state.deck_cache.clear();
 
         self.transact(None, |col| {
             let usn = col.usn()?;
 
-            deck.prepare_for_update();
-
-            // fixme: bail
-            assert!(!deck.name.contains("::"));
-
-            // fixme: check deck name is not duplicate
-            // handle blank deck name, etc
-
-            if !preserve_usn {
-                deck.set_modified(usn);
-            }
+            deck.set_modified(usn);
 
             if deck.id.0 == 0 {
+                col.prepare_deck_for_update(deck, usn)?;
                 col.match_or_create_parents(deck)?;
                 col.storage.add_deck(deck)
-            } else {
-                if let Some(existing_deck) = col.storage.get_deck(deck.id)? {
-                    if existing_deck.name != deck.name {
-                        return col.update_renamed_deck(existing_deck, deck, usn);
-                    }
+            } else if let Some(existing_deck) = col.storage.get_deck(deck.id)? {
+                if existing_deck.name != deck.name {
+                    col.update_renamed_deck(existing_deck, deck, usn)
                 } else {
-                    // fixme: this should only happen in the syncing case, and we should
-                    // ensure there are no missing parents at the end of the sync
+                    col.add_or_update_single_deck(deck, usn)
                 }
-                col.storage.update_deck(deck)
+            } else {
+                Err(AnkiError::invalid_input("updating non-existent deck"))
             }
         })
     }
 
-    fn ensure_deck_name_unique(&self, deck: &mut Deck) -> Result<()> {
+    /// 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.
+    pub(crate) fn add_or_update_single_deck(&mut self, deck: &mut Deck, usn: Usn) -> Result<()> {
+        self.state.deck_cache.clear();
+        self.prepare_deck_for_update(deck, usn)?;
+        self.storage.update_deck(deck)
+    }
+
+    fn ensure_deck_name_unique(&self, deck: &mut Deck, usn: Usn) -> Result<()> {
         loop {
             match self.storage.get_deck_id(&deck.name)? {
                 Some(did) if did == deck.id => {
@@ -238,20 +250,18 @@ impl Collection {
                 _ => (),
             }
             deck.name += "+";
+            deck.set_modified(usn);
         }
 
         Ok(())
     }
 
-    pub(crate) fn recover_missing_deck(&mut self, did: DeckID) -> Result<()> {
+    pub(crate) fn recover_missing_deck(&mut self, did: DeckID, usn: Usn) -> Result<()> {
         let mut deck = Deck::new_normal();
         deck.id = did;
         deck.name = format!("recovered{}", did);
-        self.ensure_deck_name_unique(&mut deck)?;
-        deck.prepare_for_update();
-        self.storage.update_deck(&deck)?;
-
-        Ok(())
+        deck.set_modified(usn);
+        self.add_or_update_single_deck(&mut deck, usn)
     }
 
     pub fn get_or_create_normal_deck(&mut self, human_name: &str) -> Result<Deck> {
@@ -261,26 +271,23 @@ impl Collection {
         } else {
             let mut deck = Deck::new_normal();
             deck.name = native_name;
-            self.add_or_update_deck(&mut deck, false)?;
+            self.add_or_update_deck(&mut deck)?;
             Ok(deck)
         }
     }
 
     fn update_renamed_deck(&mut self, existing: Deck, updated: &mut Deck, usn: Usn) -> Result<()> {
-        // new name should not conflict with a different deck
-        if let Some(other_did) = self.storage.get_deck_id(&updated.name)? {
-            if other_did != updated.id {
-                // fixme: this could break when syncing
-                return Err(AnkiError::Existing);
-            }
-        }
-
+        // match closest parent name
         self.match_or_create_parents(updated)?;
-        self.storage.update_deck(updated)?;
-        self.rename_child_decks(&existing, &updated.name, usn)
+        // rename children
+        self.rename_child_decks(&existing, &updated.name, usn)?;
+        // update deck
+        self.add_or_update_single_deck(updated, usn)?;
+        // after updating, we need to ensure all grandparents exist, which may not be the case
+        // in the parent->child case
+        self.create_missing_parents(&updated.name)
     }
 
-    // fixme: make sure this handles foo::bar and FOO::baz
     fn rename_child_decks(&mut self, old: &Deck, new_name: &str, usn: Usn) -> Result<()> {
         let children = self.storage.child_decks(old)?;
         let old_component_count = old.name.matches('\x1f').count() + 1;
@@ -372,6 +379,9 @@ impl Collection {
     }
 
     pub fn remove_deck_and_child_decks(&mut self, did: DeckID) -> Result<()> {
+        // fixme: vet cache clearing
+        self.state.deck_cache.clear();
+
         self.transact(None, |col| {
             let usn = col.usn()?;
 
@@ -400,9 +410,8 @@ impl Collection {
             let mut deck = deck.to_owned();
             // fixme: separate key
             deck.name = self.i18n.tr(TR::DeckConfigDefaultName).into();
-            self.ensure_deck_name_unique(&mut deck)?;
             deck.set_modified(usn);
-            self.storage.update_deck(&deck)?;
+            self.add_or_update_single_deck(&mut deck, usn)?;
         } else {
             self.storage.remove_deck(deck.id)?;
             self.storage.add_deck_grave(deck.id, usn)?;
@@ -525,12 +534,48 @@ mod test {
         let _ = col.get_or_create_normal_deck("foo::bar::baz")?;
         let mut top_deck = col.get_or_create_normal_deck("foo")?;
         top_deck.name = "other".into();
-        col.add_or_update_deck(&mut top_deck, false)?;
+        col.add_or_update_deck(&mut top_deck)?;
         assert_eq!(
             sorted_names(&col),
             vec!["Default", "other", "other::bar", "other::bar::baz"]
         );
 
+        // should do the right thing in the middle of the tree as well
+        let mut middle = col.get_or_create_normal_deck("other::bar")?;
+        middle.name = "quux\x1ffoo".into();
+        col.add_or_update_deck(&mut middle)?;
+        assert_eq!(
+            sorted_names(&col),
+            vec!["Default", "other", "quux", "quux::foo", "quux::foo::baz"]
+        );
+
+        // add another child
+        let _ = col.get_or_create_normal_deck("quux::foo::baz2");
+
+        // quux::foo -> quux::foo::baz::four
+        // means quux::foo::baz2 should be quux::foo::baz::four::baz2
+        // and a new quux::foo should have been created
+        middle.name = "quux\x1ffoo\x1fbaz\x1ffour".into();
+        col.add_or_update_deck(&mut middle)?;
+        assert_eq!(
+            sorted_names(&col),
+            vec![
+                "Default",
+                "other",
+                "quux",
+                "quux::foo",
+                "quux::foo::baz",
+                "quux::foo::baz::four",
+                "quux::foo::baz::four::baz",
+                "quux::foo::baz::four::baz2"
+            ]
+        );
+
+        // should handle name conflicts
+        middle.name = "other".into();
+        col.add_or_update_deck(&mut middle)?;
+        assert_eq!(middle.name, "other+");
+
         Ok(())
     }
 
@@ -542,7 +587,7 @@ mod test {
 
         let mut default = col.get_or_create_normal_deck("default")?;
         default.name = "one\x1ftwo".into();
-        col.add_or_update_deck(&mut default, false)?;
+        col.add_or_update_deck(&mut default)?;
 
         // create a non-default deck confusingly named "default"
         let _fake_default = col.get_or_create_normal_deck("default")?;

rslib/src/decks/tree.rs

@@ -335,9 +335,9 @@ mod test {
 
         // simulate answering a card
         child_deck.common.new_studied = 1;
-        col.add_or_update_deck(&mut child_deck, false)?;
+        col.add_or_update_deck(&mut child_deck)?;
         parent_deck.common.new_studied = 1;
-        col.add_or_update_deck(&mut parent_deck, false)?;
+        col.add_or_update_deck(&mut parent_deck)?;
 
         // with the default limit of 20, there should still be 4 due
         let tree = col.deck_tree(true, None)?;