make tag deletion undoable, and speed it up

- ~4x faster than before on tag tree with 30k notes
- remove the separate clear_tag() backend method

pylib/anki/tags.py

@@ -65,7 +65,7 @@ class TagManager:
         "Set browser expansion state for tag, registering the tag if missing."
         self.col._backend.set_tag_expanded(name=tag, expanded=expanded)
 
-    # Bulk addition/removal from notes
+    # Bulk addition/removal from specific notes
     #############################################################
 
     def bulk_add(self, nids: Sequence[int], tags: str) -> OpChangesWithCount:
@@ -84,31 +84,23 @@ class TagManager:
     def bulk_remove(self, nids: Sequence[int], tags: str) -> OpChangesWithCount:
         return self.bulk_update(nids, tags, "", False)
 
+    # Bulk addition/removal based on tag
+    #############################################################
+
     def rename(self, old: str, new: str) -> OpChangesWithCount:
         "Rename provided tag and its children, returning number of changed notes."
         x = self.col._backend.rename_tags(current_prefix=old, new_prefix=new)
         return x
 
-    def remove(self, tag: str) -> None:
+    def remove(self, space_separated_tags: str) -> OpChangesWithCount:
-        self.col._backend.clear_tag(tag)
+        "Remove the provided tag(s) and their children from notes and the tag list."
+        return self.col._backend.remove_tags(val=space_separated_tags)
 
     def drag_drop(self, source_tags: List[str], target_tag: str) -> None:
         """Rename one or more source tags that were dropped on `target_tag`.
         If target_tag is "", tags will be placed at the top level."""
         self.col._backend.drag_drop_tags(source_tags=source_tags, target_tag=target_tag)
 
-    # legacy routines
-
-    def bulkAdd(self, ids: List[int], tags: str, add: bool = True) -> None:
-        "Add tags in bulk. TAGS is space-separated."
-        if add:
-            self.bulk_add(ids, tags)
-        else:
-            self.bulk_update(ids, tags, "", False)
-
-    def bulkRem(self, ids: List[int], tags: str) -> None:
-        self.bulkAdd(ids, tags, False)
-
     # String-based utilities
     ##########################################################################
 
@@ -170,3 +162,13 @@ class TagManager:
         self, tags: Collection[str], usn: Optional[int] = None, clear: bool = False
     ) -> None:
         print("tags.register() is deprecated and no longer works")
+
+    def bulkAdd(self, ids: List[int], tags: str, add: bool = True) -> None:
+        "Add tags in bulk. TAGS is space-separated."
+        if add:
+            self.bulk_add(ids, tags)
+        else:
+            self.bulk_update(ids, tags, "", False)
+
+    def bulkRem(self, ids: List[int], tags: str) -> None:
+        self.bulkAdd(ids, tags, False)

qt/aqt/browser.py

@@ -31,7 +31,7 @@ from aqt.note_ops import (
     clear_unused_tags,
     find_and_replace,
     remove_notes,
-    remove_tags,
+    remove_tags_for_notes,
 )
 from aqt.previewer import BrowserPreviewer as PreviewDialog
 from aqt.previewer import Previewer
@@ -1259,7 +1259,7 @@ where id in %s"""
             tags := tags or self._prompt_for_tags(tr(TR.BROWSING_ENTER_TAGS_TO_DELETE))
         ):
             return
-        remove_tags(
+        remove_tags_for_notes(
             mw=self.mw, note_ids=self.selected_notes(), space_separated_tags=tags
         )
 

qt/aqt/note_ops.py

@@ -43,7 +43,7 @@ def add_tags(*, mw: AnkiQt, note_ids: Sequence[int], space_separated_tags: str)
     mw.perform_op(lambda: mw.col.tags.bulk_add(note_ids, space_separated_tags))
 
 
-def remove_tags(
+def remove_tags_for_notes(
     *, mw: AnkiQt, note_ids: Sequence[int], space_separated_tags: str
 ) -> None:
     mw.perform_op(lambda: mw.col.tags.bulk_remove(note_ids, space_separated_tags))
@@ -79,6 +79,17 @@ def rename_tag(
     )
 
 
+def remove_tags_for_all_notes(
+    *, mw: AnkiQt, parent: QWidget, space_separated_tags: str
+) -> None:
+    mw.perform_op(
+        lambda: mw.col.tags.remove(space_separated_tags=space_separated_tags),
+        success=lambda out: tooltip(
+            tr(TR.BROWSING_NOTES_UPDATED, count=out.count), parent=parent
+        ),
+    )
+
+
 def find_and_replace(
     *,
     mw: AnkiQt,

qt/aqt/reviewer.py

@@ -19,7 +19,7 @@ from anki.tags import MARKED_TAG
 from anki.utils import stripHTML
 from aqt import AnkiQt, gui_hooks
 from aqt.card_ops import set_card_flag
-from aqt.note_ops import add_tags, remove_notes, remove_tags
+from aqt.note_ops import add_tags, remove_notes, remove_tags_for_notes
 from aqt.profiles import VideoDriver
 from aqt.qt import *
 from aqt.scheduling_ops import (
@@ -847,7 +847,9 @@ time = %(time)d;
     def toggle_mark_on_current_note(self) -> None:
         note = self.card.note()
         if note.has_tag(MARKED_TAG):
-            remove_tags(mw=self.mw, note_ids=[note.id], space_separated_tags=MARKED_TAG)
+            remove_tags_for_notes(
+                mw=self.mw, note_ids=[note.id], space_separated_tags=MARKED_TAG
+            )
         else:
             add_tags(mw=self.mw, note_ids=[note.id], space_separated_tags=MARKED_TAG)
 

qt/aqt/sidebar.py

@@ -19,7 +19,7 @@ from aqt.clayout import CardLayout
 from aqt.deck_ops import remove_decks
 from aqt.main import ResetReason
 from aqt.models import Models
-from aqt.note_ops import rename_tag
+from aqt.note_ops import remove_tags_for_all_notes, rename_tag
 from aqt.qt import *
 from aqt.theme import ColoredIcon, theme_manager
 from aqt.utils import (
@@ -29,7 +29,6 @@ from aqt.utils import (
     getOnlyText,
     show_invalid_search_error,
     showWarning,
-    tooltip,
     tr,
 )
 
@@ -1200,21 +1199,13 @@ class SidebarTreeView(QTreeView):
     # Tags
     ###########################
 
-    def remove_tags(self, _item: SidebarItem) -> None:
+    def remove_tags(self, item: SidebarItem) -> None:
-        tags = self._selected_tags()
+        tags = self.mw.col.tags.join(self._selected_tags())
+        item.name = "..."
 
-        def do_remove() -> int:
+        remove_tags_for_all_notes(
-            return self.col._backend.expunge_tags(" ".join(tags))
+            mw=self.mw, parent=self.browser, space_separated_tags=tags
-
+        )
-        def on_done(fut: Future) -> None:
-            self.mw.requireReset(reason=ResetReason.BrowserRemoveTags, context=self)
-            self.browser.model.endReset()
-            tooltip(tr(TR.BROWSING_NOTES_UPDATED, count=fut.result()), parent=self)
-            self.refresh()
-
-        self.mw.checkpoint(tr(TR.ACTIONS_REMOVE_TAG))
-        self.browser.model.beginReset()
-        self.mw.taskman.with_progress(do_remove, on_done)
 
     def rename_tag(self, item: SidebarItem, new_name: str) -> None:
         if not new_name or new_name == item.name:

rslib/backend.proto

@@ -219,9 +219,8 @@ service DeckConfigService {
 service TagsService {
   rpc ClearUnusedTags(Empty) returns (OpChangesWithCount);
   rpc AllTags(Empty) returns (StringList);
-  rpc ExpungeTags(String) returns (UInt32);
+  rpc RemoveTags(String) returns (OpChangesWithCount);
   rpc SetTagExpanded(SetTagExpandedIn) returns (Empty);
-  rpc ClearTag(String) returns (Empty);
   rpc TagTree(Empty) returns (TagTreeNode);
   rpc DragDropTags(DragDropTagsIn) returns (Empty);
   rpc RenameTags(RenameTagsIn) returns (OpChangesWithCount);

rslib/src/backend/tags.rs

@@ -23,8 +23,8 @@ impl TagsService for Backend {
         })
     }
 
-    fn expunge_tags(&self, tags: pb::String) -> Result<pb::UInt32> {
+    fn remove_tags(&self, tags: pb::String) -> Result<pb::OpChangesWithCount> {
-        self.with_col(|col| col.expunge_tags(tags.val.as_str()).map(Into::into))
+        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> {
@@ -36,15 +36,6 @@ impl TagsService for Backend {
         })
     }
 
-    fn clear_tag(&self, tag: pb::String) -> Result<pb::Empty> {
-        self.with_col(|col| {
-            col.transact_no_undo(|col| {
-                col.storage.clear_tag_and_children(tag.val.as_str())?;
-                Ok(().into())
-            })
-        })
-    }
-
     fn tag_tree(&self, _input: pb::Empty) -> Result<pb::TagTreeNode> {
         self.with_col(|col| col.tag_tree())
     }

rslib/src/notes/mod.rs

@@ -210,12 +210,6 @@ impl Note {
             .collect()
     }
 
-    pub(crate) fn remove_tags(&mut self, re: &Regex) -> bool {
-        let old_len = self.tags.len();
-        self.tags.retain(|tag| !re.is_match(tag));
-        old_len > self.tags.len()
-    }
-
     pub(crate) fn replace_tags<T: Replacer>(&mut self, re: &Regex, mut repl: T) -> bool {
         let mut changed = false;
         for tag in &mut self.tags {

rslib/src/ops.rs

@@ -15,6 +15,7 @@ pub enum Op {
     RemoveNote,
     RenameDeck,
     RenameTag,
+    RemoveTag,
     ScheduleAsNew,
     SetDeck,
     SetDueDate,
@@ -54,6 +55,7 @@ impl Op {
             Op::ClearUnusedTags => TR::BrowsingClearUnusedTags,
             Op::SortCards => TR::BrowsingReschedule,
             Op::RenameTag => TR::ActionsRenameTag,
+            Op::RemoveTag => TR::ActionsRemoveTag,
         };
 
         i18n.tr(key).to_string()

rslib/src/storage/tag/get.sql

@@ -0,0 +1,4 @@
+SELECT tag,
+  usn,
+  collapsed
+FROM tags

rslib/src/storage/tag/mod.rs

@@ -19,7 +19,7 @@ impl SqliteStorage {
     /// All tags in the collection, in alphabetical order.
     pub(crate) fn all_tags(&self) -> Result<Vec<Tag>> {
         self.db
-            .prepare_cached("select tag, usn, collapsed from tags")?
+            .prepare_cached(include_str!("get.sql"))?
             .query_and_then(NO_PARAMS, row_to_tag)?
             .collect()
     }
@@ -43,7 +43,7 @@ impl SqliteStorage {
 
     pub(crate) fn get_tag(&self, name: &str) -> Result<Option<Tag>> {
         self.db
-            .prepare_cached("select tag, usn, collapsed from tags where tag = ?")?
+            .prepare_cached(&format!("{} where tag = ?", include_str!("get.sql")))?
             .query_and_then(&[name], row_to_tag)?
             .next()
             .transpose()
@@ -65,11 +65,24 @@ impl SqliteStorage {
             .map_err(Into::into)
     }
 
-    pub(crate) fn get_tag_and_children(&self, name: &str) -> Result<Vec<Tag>> {
+    pub(crate) fn get_tags_by_predicate<F>(&self, want: F) -> Result<Vec<Tag>>
-        self.db
+    where
-            .prepare_cached("select tag, usn, collapsed from tags where tag regexp ?")?
+        F: Fn(&str) -> bool,
-            .query_and_then(&[format!("(?i)^{}($|::)", regex::escape(name))], row_to_tag)?
+    {
-            .collect()
+        let mut query_stmt = self.db.prepare_cached(include_str!("get.sql"))?;
+        let mut rows = query_stmt.query(NO_PARAMS)?;
+        let mut output = vec![];
+        while let Some(row) = rows.next()? {
+            let tag = row.get_raw(0).as_str()?;
+            if want(tag) {
+                output.push(Tag {
+                    name: tag.to_owned(),
+                    usn: row.get(1)?,
+                    expanded: !row.get(2)?,
+                })
+            }
+        }
+        Ok(output)
     }
 
     pub(crate) fn remove_single_tag(&self, tag: &str) -> Result<()> {
@@ -88,15 +101,6 @@ impl SqliteStorage {
         Ok(())
     }
 
-    /// Clear all matching tags where tag_group is a regexp group that should not match whitespace.
-    pub(crate) fn clear_tag_group(&self, tag_group: &str) -> Result<()> {
-        self.db
-            .prepare_cached("delete from tags where tag regexp ?")?
-            .execute(&[format!("(?i)^{}($|::)", tag_group)])?;
-
-        Ok(())
-    }
-
     pub(crate) fn set_tag_collapsed(&self, tag: &str, collapsed: bool) -> Result<()> {
         self.db
             .prepare_cached("update tags set collapsed = ? where tag = ?")?

rslib/src/tags/mod.rs

@@ -313,37 +313,6 @@ impl Collection {
         Ok(count)
     }
 
-    /// Take tags as a whitespace-separated string and remove them from all notes and the storage.
-    pub fn expunge_tags(&mut self, tags: &str) -> Result<usize> {
-        let tag_group = format!("({})", regex::escape(tags.trim()).replace(' ', "|"));
-        let nids = self.nids_for_tags(&tag_group)?;
-        let re = Regex::new(&format!("(?i)^{}(::.*)?$", tag_group))?;
-        self.transact_no_undo(|col| {
-            col.storage.clear_tag_group(&tag_group)?;
-            col.transform_notes(&nids, |note, _nt| {
-                Ok(TransformNoteOutput {
-                    changed: note.remove_tags(&re),
-                    generate_cards: false,
-                    mark_modified: true,
-                })
-            })
-        })
-    }
-
-    /// Take tags as a regexp group, i.e. separated with pipes and wrapped in brackets, and return
-    /// the ids of all notes with one of them.
-    fn nids_for_tags(&mut self, tag_group: &str) -> Result<Vec<NoteID>> {
-        let mut stmt = self
-            .storage
-            .db
-            .prepare("select id from notes where tags regexp ?")?;
-        let args = format!("(?i).* {}(::| ).*", tag_group);
-        let nids = stmt
-            .query_map(&[args], |row| row.get(0))?
-            .collect::<std::result::Result<_, _>>()?;
-        Ok(nids)
-    }
-
     pub(crate) fn set_tag_expanded(&self, name: &str, expanded: bool) -> Result<()> {
         let mut name = name;
         let tag;
@@ -550,7 +519,7 @@ impl Collection {
         }
 
         // remove old prefix from the tag list
-        for tag in self.storage.get_tag_and_children(old_prefix)? {
+        for tag in self.storage.get_tags_by_predicate(|tag| re.is_match(tag))? {
             self.remove_single_tag_undoable(tag)?;
         }
 
@@ -569,6 +538,37 @@ impl Collection {
 
         Ok(match_count)
     }
+
+    /// Take tags as a whitespace-separated string and remove them from all notes and the tag list.
+    pub fn remove_tags(&mut self, tags: &str) -> Result<OpOutput<usize>> {
+        self.transact(Op::RemoveTag, |col| col.remove_tags_inner(tags))
+    }
+
+    fn remove_tags_inner(&mut self, tags: &str) -> Result<usize> {
+        let usn = self.usn()?;
+
+        // gather tags that need removing
+        let mut re = PrefixReplacer::new(tags)?;
+        let matched_notes = self
+            .storage
+            .get_note_tags_by_predicate(|tags| re.is_match(tags))?;
+        let match_count = matched_notes.len();
+
+        // remove from the tag list
+        for tag in self.storage.get_tags_by_predicate(|tag| re.is_match(tag))? {
+            self.remove_single_tag_undoable(tag)?;
+        }
+
+        // replace tags
+        for mut note in matched_notes {
+            let original = note.clone();
+            note.tags = re.remove(&note.tags);
+            note.set_modified(usn);
+            self.update_note_tags_undoable(&note, original)?;
+        }
+
+        Ok(match_count)
+    }
 }
 
 // fixme: merge with prefixmatcher

rslib/src/tags/prefix_replacer.rs

@@ -77,6 +77,16 @@ impl PrefixReplacer {
         join_tags(tags.as_slice())
     }
 
+    /// Remove any matching tags. Does not update seen_tags.
+    pub fn remove(&mut self, space_separated_tags: &str) -> String {
+        let tags: Vec<_> = split_tags(space_separated_tags)
+            .filter(|&tag| !self.is_match(tag))
+            .map(ToString::to_string)
+            .collect();
+
+        join_tags(tags.as_slice())
+    }
+
     pub fn into_seen_tags(self) -> HashSet<String> {
         self.seen_tags
     }