undo support for tag collapse; expand->collapse for consistency w/ decks

pylib/anki/tags.py

@@ -18,7 +18,7 @@ from typing import Collection, List, Match, Optional, Sequence
 import anki  # pylint: disable=unused-import
 import anki._backend.backend_pb2 as _pb
 import anki.collection
-from anki.collection import OpChangesWithCount
+from anki.collection import OpChanges, OpChangesWithCount
 from anki.decks import DeckId
 from anki.notes import NoteId
 from anki.utils import ids2str
@@ -63,9 +63,9 @@ class TagManager:
         res = self.col.db.list(query)
         return list(set(self.split(" ".join(res))))
 
-    def set_expanded(self, tag: str, expanded: bool) -> None:
+    def set_collapsed(self, tag: str, collapsed: bool) -> OpChanges:
         "Set browser expansion state for tag, registering the tag if missing."
-        self.col._backend.set_tag_expanded(name=tag, expanded=expanded)
+        return self.col._backend.set_tag_collapsed(name=tag, collapsed=collapsed)
 
     # Bulk addition/removal from specific notes
     #############################################################

qt/aqt/operations/tag.py

@@ -86,3 +86,7 @@ def reparent_tags(
             tr.browsing_notes_updated(count=out.count), parent=parent
         ),
     )
+
+
+def set_tag_collapsed(*, mw: AnkiQt, tag: str, collapsed: bool) -> None:
+    mw.perform_op(lambda: mw.col.tags.set_collapsed(tag=tag, collapsed=collapsed))

qt/aqt/sidebar.py

@@ -22,7 +22,12 @@ from aqt.operations.deck import (
     reparent_decks,
     set_deck_collapsed,
 )
-from aqt.operations.tag import remove_tags_from_all_notes, rename_tag, reparent_tags
+from aqt.operations.tag import (
+    remove_tags_from_all_notes,
+    rename_tag,
+    reparent_tags,
+    set_tag_collapsed,
+)
 from aqt.qt import *
 from aqt.theme import ColoredIcon, theme_manager
 from aqt.utils import KeyboardModifiersPressed, askUser, getOnlyText, showWarning, tr
@@ -922,20 +927,19 @@ class SidebarTreeView(QTreeView):
         def render(
             root: SidebarItem, nodes: Iterable[TagTreeNode], head: str = ""
         ) -> None:
+            def toggle_expand(node: TagTreeNode) -> Callable[[bool], None]:
+                full_name = head + node.name
+                return lambda expanded: set_tag_collapsed(
+                    mw=self.mw, tag=full_name, collapsed=not expanded
+                )
+
             for node in nodes:
-
-                def toggle_expand() -> Callable[[bool], None]:
-                    full_name = head + node.name  # pylint: disable=cell-var-from-loop
-                    return lambda expanded: self.mw.col.tags.set_expanded(
-                        full_name, expanded
-                    )
-
                 item = SidebarItem(
                     name=node.name,
                     icon=icon,
                     search_node=SearchNode(tag=head + node.name),
-                    on_expanded=toggle_expand(),
+                    on_expanded=toggle_expand(node),
-                    expanded=node.expanded,
+                    expanded=not node.collapsed,
                     item_type=SidebarItemType.TAG,
                     name_prefix=head,
                 )

rslib/backend.proto

@@ -232,7 +232,7 @@ service TagsService {
   rpc ClearUnusedTags(Empty) returns (OpChangesWithCount);
   rpc AllTags(Empty) returns (StringList);
   rpc RemoveTags(String) returns (OpChangesWithCount);
-  rpc SetTagExpanded(SetTagExpandedIn) returns (Empty);
+  rpc SetTagCollapsed(SetTagCollapsedIn) returns (OpChanges);
   rpc TagTree(Empty) returns (TagTreeNode);
   rpc ReparentTags(ReparentTagsIn) returns (OpChangesWithCount);
   rpc RenameTags(RenameTagsIn) returns (OpChangesWithCount);
@@ -913,9 +913,9 @@ message AddOrUpdateDeckConfigLegacyIn {
   bool preserve_usn_and_mtime = 2;
 }
 
-message SetTagExpandedIn {
+message SetTagCollapsedIn {
   string name = 1;
-  bool expanded = 2;
+  bool collapsed = 2;
 }
 
 message SetDeckCollapsedIn {
@@ -937,7 +937,7 @@ message TagTreeNode {
   string name = 1;
   repeated TagTreeNode children = 2;
   uint32 level = 3;
-  bool expanded = 4;
+  bool collapsed = 4;
 }
 
 message ReparentTagsIn {

rslib/src/backend/tags.rs

@@ -27,12 +27,10 @@ impl TagsService for Backend {
         self.with_col(|col| col.remove_tags(tags.val.as_str()).map(Into::into))
     }
 
-    fn set_tag_expanded(&self, input: pb::SetTagExpandedIn) -> Result<pb::Empty> {
+    fn set_tag_collapsed(&self, input: pb::SetTagCollapsedIn) -> Result<pb::OpChanges> {
         self.with_col(|col| {
-            col.transact_no_undo(|col| {
+            col.set_tag_collapsed(&input.name, input.collapsed)
-                col.set_tag_expanded(&input.name, input.expanded)?;
+                .map(Into::into)
-                Ok(().into())
-            })
         })
     }
 

rslib/src/dbcheck.rs

@@ -611,7 +611,7 @@ mod test {
         note.tags.push("two".into());
         col.add_note(&mut note, DeckId(1))?;
 
-        col.set_tag_expanded("one", true)?;
+        col.set_tag_collapsed("one", false)?;
 
         col.check_database(progress_fn)?;
 

rslib/src/storage/tag/mod.rs

@@ -93,11 +93,10 @@ impl SqliteStorage {
         Ok(())
     }
 
-    pub(crate) fn set_tag_collapsed(&self, tag: &str, collapsed: bool) -> Result<()> {
+    pub(crate) fn update_tag(&self, tag: &Tag) -> Result<()> {
         self.db
-            .prepare_cached("update tags set collapsed = ? where tag = ?")?
+            .prepare_cached(include_str!("update.sql"))?
-            .execute(params![collapsed, tag])?;
+            .execute(params![&tag.name, tag.usn, !tag.expanded])?;
-
         Ok(())
     }
 

rslib/src/storage/tag/update.sql

@@ -0,0 +1,5 @@
+UPDATE tags
+SET tag = ?1,
+  usn = ?,
+  collapsed = ?
+WHERE tag = ?1

rslib/src/tags/mod.rs

@@ -30,6 +30,10 @@ impl Tag {
             expanded: false,
         }
     }
+
+    pub(crate) fn set_modified(&mut self, usn: Usn) {
+        self.usn = usn;
+    }
 }
 
 pub(crate) fn split_tags(tags: &str) -> impl Iterator<Item = &str> {
@@ -55,17 +59,3 @@ fn immediate_parent_name_unicase(tag_name: UniCase<&str>) -> Option<UniCase<&str
 fn immediate_parent_name_str(tag_name: &str) -> Option<&str> {
     tag_name.rsplitn(2, "::").nth(1)
 }
-
-impl Collection {
-    pub(crate) fn set_tag_expanded(&self, name: &str, expanded: bool) -> Result<()> {
-        let mut name = name;
-        let tag;
-        if self.storage.get_tag(name)?.is_none() {
-            // tag is missing, register it
-            tag = Tag::new(name.to_string(), self.usn()?);
-            self.storage.register_tag(&tag)?;
-            name = &tag.name;
-        }
-        self.storage.set_tag_collapsed(name, !expanded)
-    }
-}

rslib/src/tags/remove.rs

@@ -108,7 +108,7 @@ mod test {
         note.tags.push("two".into());
         col.add_note(&mut note, DeckId(1))?;
 
-        col.set_tag_expanded("one", true)?;
+        col.set_tag_collapsed("one", false)?;
         col.clear_unused_tags()?;
         assert_eq!(col.storage.get_tag("one")?.unwrap().expanded, true);
         assert_eq!(col.storage.get_tag("two")?.unwrap().expanded, false);

rslib/src/tags/tree.rs

@@ -15,6 +15,29 @@ impl Collection {
 
         Ok(tree)
     }
+
+    pub fn set_tag_collapsed(&mut self, tag: &str, collapsed: bool) -> Result<OpOutput<()>> {
+        self.transact(Op::ExpandCollapse, |col| {
+            col.set_tag_collapsed_inner(tag, collapsed, col.usn()?)
+        })
+    }
+}
+
+impl Collection {
+    fn set_tag_collapsed_inner(&mut self, name: &str, collapsed: bool, usn: Usn) -> Result<()> {
+        self.register_tag_string(name.into(), usn)?;
+        if let Some(mut tag) = self.storage.get_tag(name)? {
+            let original = tag.clone();
+            tag.expanded = !collapsed;
+            self.update_tag_inner(&mut tag, original, usn)?;
+        }
+        Ok(())
+    }
+
+    fn update_tag_inner(&mut self, tag: &mut Tag, original: Tag, usn: Usn) -> Result<()> {
+        tag.set_modified(usn);
+        self.update_tag_undoable(&tag, original)
+    }
 }
 
 /// Append any missing parents. Caller must sort afterwards.
@@ -58,7 +81,7 @@ fn add_child_nodes(tags: &mut Peekable<impl Iterator<Item = Tag>>, parent: &mut
                     name: (*split_name.last().unwrap()).into(),
                     children: vec![],
                     level: parent.level + 1,
-                    expanded: tag.expanded,
+                    collapsed: !tag.expanded,
                 });
                 tags.next();
             }
@@ -105,8 +128,7 @@ mod test {
             name: name.into(),
             level,
             children,
-
+            collapsed: level != 0,
-            ..Default::default()
         }
     }
 

rslib/src/tags/undo.rs

@@ -8,6 +8,7 @@ use crate::prelude::*;
 pub(crate) enum UndoableTagChange {
     Added(Box<Tag>),
     Removed(Box<Tag>),
+    Updated(Box<Tag>),
 }
 
 impl Collection {
@@ -15,8 +16,22 @@ impl Collection {
         match change {
             UndoableTagChange::Added(tag) => self.remove_single_tag_undoable(*tag),
             UndoableTagChange::Removed(tag) => self.register_tag_undoable(&tag),
+            UndoableTagChange::Updated(tag) => {
+                let current = self
+                    .storage
+                    .get_tag(&tag.name)?
+                    .ok_or_else(|| AnkiError::invalid_input("tag disappeared"))?;
+                self.update_tag_undoable(&tag, current)
+            }
         }
     }
+
+    /// Updates an existing tag, saving an undo entry. Caller must update usn.
+    pub(super) fn update_tag_undoable(&mut self, tag: &Tag, original: Tag) -> Result<()> {
+        self.save_undo(UndoableTagChange::Updated(Box::new(original)));
+        self.storage.update_tag(tag)
+    }
+
     /// Adds an already-validated tag to the tag list, saving an undo entry.
     /// Caller is responsible for setting usn.
     pub(super) fn register_tag_undoable(&mut self, tag: &Tag) -> Result<()> {