From 09076da937d25f779bc1ddec8a40402dabd14da0 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Thu, 18 Mar 2021 21:35:32 +1000 Subject: [PATCH] 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 | 32 ++++++++-------- qt/aqt/browser.py | 4 +- qt/aqt/note_ops.py | 13 ++++++- qt/aqt/reviewer.py | 6 ++- qt/aqt/sidebar.py | 23 ++++------- rslib/backend.proto | 3 +- rslib/src/backend/tags.rs | 13 +------ rslib/src/notes/mod.rs | 6 --- rslib/src/ops.rs | 2 + rslib/src/storage/tag/get.sql | 4 ++ rslib/src/storage/tag/mod.rs | 36 +++++++++-------- rslib/src/tags/mod.rs | 64 +++++++++++++++---------------- rslib/src/tags/prefix_replacer.rs | 10 +++++ 13 files changed, 113 insertions(+), 103 deletions(-) create mode 100644 rslib/src/storage/tag/get.sql diff --git a/pylib/anki/tags.py b/pylib/anki/tags.py index 71c9d14df..2726e6a7c 100644 --- a/pylib/anki/tags.py +++ b/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: - self.col._backend.clear_tag(tag) + def remove(self, space_separated_tags: str) -> OpChangesWithCount: + "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) diff --git a/qt/aqt/browser.py b/qt/aqt/browser.py index 6011ce381..16e3ac27d 100644 --- a/qt/aqt/browser.py +++ b/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 ) diff --git a/qt/aqt/note_ops.py b/qt/aqt/note_ops.py index b7a1af72a..49f962555 100644 --- a/qt/aqt/note_ops.py +++ b/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, diff --git a/qt/aqt/reviewer.py b/qt/aqt/reviewer.py index 088e6dbb5..4dc6c00f5 100644 --- a/qt/aqt/reviewer.py +++ b/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) diff --git a/qt/aqt/sidebar.py b/qt/aqt/sidebar.py index 86f61cb89..9e0637f4e 100644 --- a/qt/aqt/sidebar.py +++ b/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: - tags = self._selected_tags() + def remove_tags(self, item: SidebarItem) -> None: + tags = self.mw.col.tags.join(self._selected_tags()) + item.name = "..." - def do_remove() -> int: - return self.col._backend.expunge_tags(" ".join(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) + remove_tags_for_all_notes( + mw=self.mw, parent=self.browser, space_separated_tags=tags + ) def rename_tag(self, item: SidebarItem, new_name: str) -> None: if not new_name or new_name == item.name: diff --git a/rslib/backend.proto b/rslib/backend.proto index 4bde1021b..20dad19d7 100644 --- a/rslib/backend.proto +++ b/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); diff --git a/rslib/src/backend/tags.rs b/rslib/src/backend/tags.rs index 3ba67f116..f3e6e09a4 100644 --- a/rslib/src/backend/tags.rs +++ b/rslib/src/backend/tags.rs @@ -23,8 +23,8 @@ impl TagsService for Backend { }) } - fn expunge_tags(&self, tags: pb::String) -> Result { - self.with_col(|col| col.expunge_tags(tags.val.as_str()).map(Into::into)) + fn remove_tags(&self, tags: pb::String) -> Result { + self.with_col(|col| col.remove_tags(tags.val.as_str()).map(Into::into)) } fn set_tag_expanded(&self, input: pb::SetTagExpandedIn) -> Result { @@ -36,15 +36,6 @@ impl TagsService for Backend { }) } - fn clear_tag(&self, tag: pb::String) -> Result { - 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 { self.with_col(|col| col.tag_tree()) } diff --git a/rslib/src/notes/mod.rs b/rslib/src/notes/mod.rs index fe415f8c3..4feeec601 100644 --- a/rslib/src/notes/mod.rs +++ b/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(&mut self, re: &Regex, mut repl: T) -> bool { let mut changed = false; for tag in &mut self.tags { diff --git a/rslib/src/ops.rs b/rslib/src/ops.rs index d6466c3c8..dd5208d00 100644 --- a/rslib/src/ops.rs +++ b/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() diff --git a/rslib/src/storage/tag/get.sql b/rslib/src/storage/tag/get.sql new file mode 100644 index 000000000..3bc48697c --- /dev/null +++ b/rslib/src/storage/tag/get.sql @@ -0,0 +1,4 @@ +SELECT tag, + usn, + collapsed +FROM tags \ No newline at end of file diff --git a/rslib/src/storage/tag/mod.rs b/rslib/src/storage/tag/mod.rs index ce300c522..097da8f31 100644 --- a/rslib/src/storage/tag/mod.rs +++ b/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> { 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> { 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> { - self.db - .prepare_cached("select tag, usn, collapsed from tags where tag regexp ?")? - .query_and_then(&[format!("(?i)^{}($|::)", regex::escape(name))], row_to_tag)? - .collect() + pub(crate) fn get_tags_by_predicate(&self, want: F) -> Result> + where + F: Fn(&str) -> bool, + { + 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 = ?")? diff --git a/rslib/src/tags/mod.rs b/rslib/src/tags/mod.rs index 2a0fa4238..e4d87e576 100644 --- a/rslib/src/tags/mod.rs +++ b/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 { - 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> { - 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::>()?; - 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> { + self.transact(Op::RemoveTag, |col| col.remove_tags_inner(tags)) + } + + fn remove_tags_inner(&mut self, tags: &str) -> Result { + 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(¬e.tags); + note.set_modified(usn); + self.update_note_tags_undoable(¬e, original)?; + } + + Ok(match_count) + } } // fixme: merge with prefixmatcher diff --git a/rslib/src/tags/prefix_replacer.rs b/rslib/src/tags/prefix_replacer.rs index a14ab4a77..e2edb4369 100644 --- a/rslib/src/tags/prefix_replacer.rs +++ b/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 { self.seen_tags }