make tag renaming undoable, and speed it up

~3x speedup when renaming a tag that's on 25k notes

pylib/anki/tags.py

@@ -85,12 +85,9 @@ class TagManager:
         return self.bulk_update(nids, tags, "", False)
 
     def rename(self, old: str, new: str) -> OpChangesWithCount:
-        "Rename provided tag, returning number of changed notes."
-        nids = self.col.find_notes(anki.collection.SearchNode(tag=old))
-        if not nids:
-            return OpChangesWithCount()
-        escaped_name = re.sub(r"[*_\\]", r"\\\g<0>", old)
-        return self.bulk_update(nids, escaped_name, new, False)
+        "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:
         self.col._backend.clear_tag(tag)

qt/aqt/main.py

@@ -789,7 +789,7 @@ class AnkiQt(QMainWindow):
 
         after_hooks() will be called after hooks are fired, if it is provided.
         Components can use this to ignore change notices generated by operations
-        they invoke themselves.
+        they invoke themselves, or perform some subsequent action.
         """
 
         self._increase_background_ops()

qt/aqt/note_ops.py

@@ -5,6 +5,7 @@ from __future__ import annotations
 
 from typing import Callable, Optional, Sequence
 
+from anki.collection import OpChangesWithCount
 from anki.lang import TR
 from anki.notes import Note
 from aqt import AnkiQt, QWidget
@@ -57,6 +58,27 @@ def clear_unused_tags(*, mw: AnkiQt, parent: QWidget) -> None:
     )
 
 
+def rename_tag(
+    *,
+    mw: AnkiQt,
+    parent: QWidget,
+    current_name: str,
+    new_name: str,
+    after_rename: Callable[[], None],
+) -> None:
+    def success(out: OpChangesWithCount) -> None:
+        if out.count:
+            tooltip(tr(TR.BROWSING_NOTES_UPDATED, count=out.count), parent=parent)
+        else:
+            showInfo(tr(TR.BROWSING_TAG_RENAME_WARNING_EMPTY), parent=parent)
+
+    mw.perform_op(
+        lambda: mw.col.tags.rename(old=current_name, new=new_name),
+        success=success,
+        after_hooks=after_rename,
+    )
+
+
 def find_and_replace(
     *,
     mw: AnkiQt,

qt/aqt/sidebar.py

@@ -19,6 +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.qt import *
 from aqt.theme import ColoredIcon, theme_manager
 from aqt.utils import (
@@ -27,7 +28,6 @@ from aqt.utils import (
     askUser,
     getOnlyText,
     show_invalid_search_error,
-    showInfo,
     showWarning,
     tooltip,
     tr,
@@ -1217,40 +1217,26 @@ class SidebarTreeView(QTreeView):
         self.mw.taskman.with_progress(do_remove, on_done)
 
     def rename_tag(self, item: SidebarItem, new_name: str) -> None:
-        new_name = new_name.replace(" ", "")
-        if new_name and new_name != item.name:
-            # block repainting until collection is updated
-            self.setUpdatesEnabled(False)
-            self.browser.editor.call_after_note_saved(
-                lambda: self._rename_tag(item, new_name)
-            )
+        if not new_name or new_name == item.name:
+            return
+
+        new_name_base = new_name
 
-    def _rename_tag(self, item: SidebarItem, new_name: str) -> None:
         old_name = item.full_name
         new_name = item.name_prefix + new_name
 
-        def do_rename() -> int:
-            self.mw.col.tags.remove(old_name)
-            return self.col.tags.rename(old_name, new_name).count
+        item.name = new_name_base
 
-        def on_done(fut: Future) -> None:
-            self.setUpdatesEnabled(True)
-            self.mw.requireReset(reason=ResetReason.BrowserAddTags, context=self)
-            self.browser.model.endReset()
-
-            count = fut.result()
-            if not count:
-                showInfo(tr(TR.BROWSING_TAG_RENAME_WARNING_EMPTY))
-            else:
-                tooltip(tr(TR.BROWSING_NOTES_UPDATED, count=count), parent=self)
-                self.refresh(
-                    lambda item: item.item_type == SidebarItemType.TAG
-                    and item.full_name == new_name
-                )
-
-        self.mw.checkpoint(tr(TR.ACTIONS_RENAME_TAG))
-        self.browser.model.beginReset()
-        self.mw.taskman.with_progress(do_rename, on_done)
+        rename_tag(
+            mw=self.mw,
+            parent=self.browser,
+            current_name=old_name,
+            new_name=new_name,
+            after_rename=lambda: self.refresh(
+                lambda item: item.item_type == SidebarItemType.TAG
+                and item.full_name == new_name
+            ),
+        )
 
     # Saved searches
     ####################################

rslib/backend.proto

@@ -224,6 +224,7 @@ service TagsService {
   rpc ClearTag(String) returns (Empty);
   rpc TagTree(Empty) returns (TagTreeNode);
   rpc DragDropTags(DragDropTagsIn) returns (Empty);
+  rpc RenameTags(RenameTagsIn) returns (OpChangesWithCount);
 }
 
 service SearchService {
@@ -930,6 +931,11 @@ message DragDropTagsIn {
   string target_tag = 2;
 }
 
+message RenameTagsIn {
+  string current_prefix = 1;
+  string new_prefix = 2;
+}
+
 message SetConfigJsonIn {
   string key = 1;
   bytes value_json = 2;

rslib/src/backend/tags.rs

@@ -59,4 +59,9 @@ impl TagsService for Backend {
         self.with_col(|col| col.drag_drop_tags(&source_tags, target_tag))
             .map(Into::into)
     }
+
+    fn rename_tags(&self, input: pb::RenameTagsIn) -> Result<pb::OpChangesWithCount> {
+        self.with_col(|col| col.rename_tag(&input.current_prefix, &input.new_prefix))
+            .map(Into::into)
+    }
 }

rslib/src/notes/mod.rs

@@ -49,6 +49,23 @@ pub struct Note {
     pub(crate) checksum: Option<u32>,
 }
 
+/// Information required for updating tags while leaving note content alone.
+/// Tags are stored in their DB form, separated by spaces.
+#[derive(Debug, PartialEq, Clone)]
+pub(crate) struct NoteTags {
+    pub id: NoteID,
+    pub mtime: TimestampSecs,
+    pub usn: Usn,
+    pub tags: String,
+}
+
+impl NoteTags {
+    pub(crate) fn set_modified(&mut self, usn: Usn) {
+        self.mtime = TimestampSecs::now();
+        self.usn = usn;
+    }
+}
+
 impl Note {
     pub(crate) fn new(notetype: &NoteType) -> Self {
         Note {

rslib/src/notes/undo.rs

@@ -3,6 +3,8 @@
 
 use crate::{prelude::*, undo::UndoableChange};
 
+use super::NoteTags;
+
 #[derive(Debug)]
 pub(crate) enum UndoableNoteChange {
     Added(Box<Note>),
@@ -10,6 +12,7 @@ pub(crate) enum UndoableNoteChange {
     Removed(Box<Note>),
     GraveAdded(Box<(NoteID, Usn)>),
     GraveRemoved(Box<(NoteID, Usn)>),
+    TagsUpdated(Box<NoteTags>),
 }
 
 impl Collection {
@@ -26,6 +29,13 @@ impl Collection {
             UndoableNoteChange::Removed(note) => self.restore_deleted_note(*note),
             UndoableNoteChange::GraveAdded(e) => self.remove_note_grave(e.0, e.1),
             UndoableNoteChange::GraveRemoved(e) => self.add_note_grave(e.0, e.1),
+            UndoableNoteChange::TagsUpdated(note_tags) => {
+                let current = self
+                    .storage
+                    .get_note_tags_by_id(note_tags.id)?
+                    .ok_or_else(|| AnkiError::invalid_input("note disappeared"))?;
+                self.update_note_tags_undoable(&note_tags, current)
+            }
         }
     }
 
@@ -81,6 +91,15 @@ impl Collection {
         Ok(())
     }
 
+    pub(crate) fn update_note_tags_undoable(
+        &mut self,
+        tags: &NoteTags,
+        original: NoteTags,
+    ) -> Result<()> {
+        self.save_undo(UndoableNoteChange::TagsUpdated(Box::new(original)));
+        self.storage.update_note_tags(tags)
+    }
+
     fn remove_note_without_grave(&mut self, note: Note) -> Result<()> {
         self.storage.remove_note(note.id)?;
         self.save_undo(UndoableNoteChange::Removed(Box::new(note)));

rslib/src/ops.rs

@@ -14,6 +14,7 @@ pub enum Op {
     RemoveDeck,
     RemoveNote,
     RenameDeck,
+    RenameTag,
     ScheduleAsNew,
     SetDeck,
     SetDueDate,
@@ -52,6 +53,7 @@ impl Op {
             Op::FindAndReplace => TR::BrowsingFindAndReplace,
             Op::ClearUnusedTags => TR::BrowsingClearUnusedTags,
             Op::SortCards => TR::BrowsingReschedule,
+            Op::RenameTag => TR::ActionsRenameTag,
         };
 
         i18n.tr(key).to_string()

rslib/src/storage/note/get_tags.sql

@@ -0,0 +1,5 @@
+SELECT id,
+  mod,
+  usn,
+  tags
+FROM notes

rslib/src/storage/note/mod.rs

@@ -5,7 +5,7 @@ use std::collections::HashSet;
 
 use crate::{
     err::Result,
-    notes::{Note, NoteID},
+    notes::{Note, NoteID, NoteTags},
     notetype::NoteTypeID,
     tags::{join_tags, split_tags},
     timestamp::TimestampMillis,
@@ -189,4 +189,49 @@ impl super::SqliteStorage {
 
         Ok(())
     }
+
+    pub(crate) fn get_note_tags_by_id(&mut self, note_id: NoteID) -> Result<Option<NoteTags>> {
+        self.db
+            .prepare_cached(&format!("{} where id = ?", include_str!("get_tags.sql")))?
+            .query_and_then(&[note_id], |row| -> Result<_> {
+                {
+                    Ok(NoteTags {
+                        id: row.get(0)?,
+                        mtime: row.get(1)?,
+                        usn: row.get(2)?,
+                        tags: row.get(3)?,
+                    })
+                }
+            })?
+            .next()
+            .transpose()
+    }
+
+    pub(crate) fn get_note_tags_by_predicate<F>(&mut self, want: F) -> Result<Vec<NoteTags>>
+    where
+        F: Fn(&str) -> bool,
+    {
+        let mut query_stmt = self.db.prepare_cached(include_str!("get_tags.sql"))?;
+        let mut rows = query_stmt.query(NO_PARAMS)?;
+        let mut output = vec![];
+        while let Some(row) = rows.next()? {
+            let tags = row.get_raw(3).as_str()?;
+            if want(tags) {
+                output.push(NoteTags {
+                    id: row.get(0)?,
+                    mtime: row.get(1)?,
+                    usn: row.get(2)?,
+                    tags: tags.to_owned(),
+                })
+            }
+        }
+        Ok(output)
+    }
+
+    pub(crate) fn update_note_tags(&mut self, note: &NoteTags) -> Result<()> {
+        self.db
+            .prepare_cached(include_str!("update_tags.sql"))?
+            .execute(params![note.mtime, note.usn, note.tags, note.id])?;
+        Ok(())
+    }
 }

rslib/src/storage/note/update_tags.sql

@@ -0,0 +1,5 @@
+UPDATE notes
+SET mod = ?,
+  usn = ?,
+  tags = ?
+WHERE id = ?

rslib/src/storage/tag/mod.rs

@@ -65,8 +65,6 @@ impl SqliteStorage {
             .map_err(Into::into)
     }
 
-    // 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 ?")?

rslib/src/tags/mod.rs

@@ -1,6 +1,7 @@
 // Copyright: Ankitects Pty Ltd and contributors
 // License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
 
+mod prefix_replacer;
 pub(crate) mod undo;
 
 use crate::{
@@ -13,6 +14,7 @@ use crate::{
     types::Usn,
 };
 
+use prefix_replacer::PrefixReplacer;
 use regex::{NoExpand, Regex, Replacer};
 use std::{borrow::Cow, collections::HashSet, iter::Peekable};
 use unicase::UniCase;
@@ -231,6 +233,21 @@ impl Collection {
     /// In the case the tag is already registered, tag will be mutated to match the existing
     /// name.
     pub(crate) fn register_tag(&mut self, tag: &mut Tag) -> Result<bool> {
+        let is_new = self.prepare_tag_for_registering(tag)?;
+        if is_new {
+            self.register_tag_undoable(&tag)?;
+        }
+        Ok(is_new)
+    }
+
+    fn register_tag_string(&mut self, tag: String, usn: Usn) -> Result<bool> {
+        let mut tag = Tag::new(tag, usn);
+        self.register_tag(&mut tag)
+    }
+
+    /// Create a tag object, normalize text, and match parents/existing case if available.
+    /// True if tag is new.
+    fn prepare_tag_for_registering(&self, tag: &mut Tag) -> Result<bool> {
         let normalized_name = normalize_tag_name(&tag.name);
         if normalized_name.is_empty() {
             // this should not be possible
@@ -245,7 +262,6 @@ impl Collection {
             } else if let Cow::Owned(new_name) = normalized_name {
                 tag.name = new_name;
             }
-            self.register_tag_undoable(&tag)?;
             Ok(true)
         }
     }
@@ -446,18 +462,7 @@ impl Collection {
             .iter()
             // convert the names into regexps/replacements
             .map(|(tag, output)| {
-                Regex::new(&format!(
-                    r#"(?ix)
-                    ^
-                    {}
-                    # optional children
-                    (::.+)?
-                    $
-                    "#,
-                    regex::escape(tag)
-                ))
-                .map_err(|_| AnkiError::invalid_input("invalid regex"))
-                .map(|regex| (regex, output))
+                regex_matching_tag_and_children_in_single_tag(tag).map(|regex| (regex, output))
             })
             .collect::<Result<Vec<_>>>()?;
 
@@ -505,6 +510,82 @@ impl Collection {
 
         Ok(())
     }
+
+    /// Rename a given tag and its children on all notes that reference it, returning changed
+    /// note count.
+    pub fn rename_tag(&mut self, old_prefix: &str, new_prefix: &str) -> Result<OpOutput<usize>> {
+        self.transact(Op::RenameTag, |col| {
+            col.rename_tag_inner(old_prefix, new_prefix)
+        })
+    }
+
+    fn rename_tag_inner(&mut self, old_prefix: &str, new_prefix: &str) -> Result<usize> {
+        if new_prefix.contains(' ') {
+            return Err(AnkiError::invalid_input(
+                "replacement name can not contain a space",
+            ));
+        }
+        if new_prefix.trim().is_empty() {
+            return Err(AnkiError::invalid_input(
+                "replacement name must not be empty",
+            ));
+        }
+
+        let usn = self.usn()?;
+
+        // match existing case if available, and ensure normalized.
+        let mut tag = Tag::new(new_prefix.to_string(), usn);
+        self.prepare_tag_for_registering(&mut tag)?;
+        let new_prefix = &tag.name;
+
+        // gather tags that need replacing
+        let mut re = PrefixReplacer::new(old_prefix)?;
+        let matched_notes = self
+            .storage
+            .get_note_tags_by_predicate(|tags| re.is_match(tags))?;
+        let match_count = matched_notes.len();
+        if match_count == 0 {
+            // no matches; exit early so we don't clobber the empty tag entries
+            return Ok(0);
+        }
+
+        // remove old prefix from the tag list
+        for tag in self.storage.get_tag_and_children(old_prefix)? {
+            self.remove_single_tag_undoable(tag)?;
+        }
+
+        // replace tags
+        for mut note in matched_notes {
+            let original = note.clone();
+            note.tags = re.replace(&note.tags, new_prefix);
+            note.set_modified(usn);
+            self.update_note_tags_undoable(&note, original)?;
+        }
+
+        // update tag list
+        for tag in re.into_seen_tags() {
+            self.register_tag_string(tag, usn)?;
+        }
+
+        Ok(match_count)
+    }
+}
+
+// fixme: merge with prefixmatcher
+
+/// A regex that will match a string tag that has been split from a list.
+fn regex_matching_tag_and_children_in_single_tag(tag: &str) -> Result<Regex> {
+    Regex::new(&format!(
+        r#"(?ix)
+        ^
+        {}
+        # optional children
+        (::.+)?
+        $
+        "#,
+        regex::escape(tag)
+    ))
+    .map_err(Into::into)
 }
 
 #[cfg(test)]

rslib/src/tags/prefix_replacer.rs

@@ -0,0 +1,119 @@
+// Copyright: Ankitects Pty Ltd and contributors
+// License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
+
+use regex::{Captures, Regex};
+use std::{borrow::Cow, collections::HashSet};
+
+use super::{join_tags, split_tags};
+use crate::prelude::*;
+pub(crate) struct PrefixReplacer {
+    regex: Regex,
+    seen_tags: HashSet<String>,
+}
+
+/// Helper to match any of the provided space-separated tags in a space-
+/// separated list of tags, and replace the prefix.
+///
+/// Tracks seen tags during replacement, so the tag list can be updated as well.
+impl PrefixReplacer {
+    pub fn new(space_separated_tags: &str) -> Result<Self> {
+        // convert "fo*o bar" into "fo\*o|bar"
+        let tags: Vec<_> = split_tags(space_separated_tags)
+            .map(regex::escape)
+            .collect();
+        let tags = tags.join("|");
+
+        let regex = Regex::new(&format!(
+            r#"(?ix)
+            # start of string, or a space
+            (?:^|\ )
+            # 1: the tag prefix
+            (
+                {}
+            )
+            (?:
+                # 2: an optional child separator
+                (::)
+                # or a space/end of string the end of the string
+                |\ |$
+            )
+        "#,
+            tags
+        ))?;
+
+        Ok(Self {
+            regex,
+            seen_tags: HashSet::new(),
+        })
+    }
+
+    pub fn is_match(&self, space_separated_tags: &str) -> bool {
+        self.regex.is_match(space_separated_tags)
+    }
+
+    pub fn replace(&mut self, space_separated_tags: &str, replacement: &str) -> String {
+        let tags: Vec<_> = split_tags(space_separated_tags)
+            .map(|tag| {
+                self.regex
+                    .replace(tag, |caps: &Captures| {
+                        // if we captured the child separator, add it to the replacement
+                        if caps.get(2).is_some() {
+                            Cow::Owned(format!("{}::", replacement))
+                        } else {
+                            Cow::Borrowed(replacement)
+                        }
+                    })
+                    .to_string()
+            })
+            .collect();
+
+        for tag in &tags {
+            // sadly HashSet doesn't have an entry API at the moment
+            if !self.seen_tags.contains(tag) {
+                self.seen_tags.insert(tag.clone());
+            }
+        }
+
+        join_tags(tags.as_slice())
+    }
+
+    pub fn into_seen_tags(self) -> HashSet<String> {
+        self.seen_tags
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn regex() -> Result<()> {
+        let re = PrefixReplacer::new("one two")?;
+        assert_eq!(re.is_match(" foo "), false);
+        assert_eq!(re.is_match(" foo one "), true);
+        assert_eq!(re.is_match(" two foo "), true);
+
+        let mut re = PrefixReplacer::new("foo")?;
+        assert_eq!(re.is_match("foo"), true);
+        assert_eq!(re.is_match(" foo "), true);
+        assert_eq!(re.is_match(" bar foo baz "), true);
+        assert_eq!(re.is_match(" bar FOO baz "), true);
+        assert_eq!(re.is_match(" bar foof baz "), false);
+        assert_eq!(re.is_match(" barfoo "), false);
+
+        let mut as_xxx = |text| re.replace(text, "xxx");
+
+        assert_eq!(&as_xxx(" baz FOO "), " baz xxx ");
+        assert_eq!(&as_xxx(" x foo::bar x "), " x xxx::bar x ");
+        assert_eq!(
+            &as_xxx(" x foo::bar bar::foo x "),
+            " x xxx::bar bar::foo x "
+        );
+        assert_eq!(
+            &as_xxx(" x foo::bar foo::bar::baz x "),
+            " x xxx::bar xxx::bar::baz x "
+        );
+
+        Ok(())
+    }
+}

rslib/src/tags/undo.rs

@@ -17,13 +17,15 @@ impl Collection {
             UndoableTagChange::Removed(tag) => self.register_tag_undoable(&tag),
         }
     }
-    /// Adds an already-validated tag to the DB and undo list.
+    /// 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<()> {
         self.save_undo(UndoableTagChange::Added(Box::new(tag.clone())));
         self.storage.register_tag(&tag)
     }
 
+    /// Remove a single tag from the tag list, saving an undo entry. Does not alter notes.
+    /// FIXME: caller will need to update usn when we make tags incrementally syncable.
     pub(super) fn remove_single_tag_undoable(&mut self, tag: Tag) -> Result<()> {
         self.storage.remove_single_tag(&tag.name)?;
         self.save_undo(UndoableTagChange::Removed(Box::new(tag)));