clear_unused_tags and browser redraw improvements

- clear_unused_tags() is now undoable, and returns the number of removed
notes
- add a new mw.query_op() helper for immutable queries
- decouple "freeze/unfreeze ui state" hooks from the "interface update
required" hook, so that the former is fired even on error, and can be
made re-entrant
- use a 'block_updates' flag in Python, instead of setUpdatesEnabled(),
as the latter has the side-effect of preventing child windows like
tooltips from appearing, and forces a full redrawn when updates are
enabled again. The new behaviour leads to the card list blanking out
when a long-running op is running, but in the future if we cache the
cell values we can just display them from the cache instead.
- we were indiscriminately saving the note with saveNow(), due to the
call to saveTags(). Changed so that it only saves when the tags field
is focused.
- drain the "on_done" queue on main before launching a new background
task, to lower the chances of something in on_done making a small query
to the DB and hanging until a long op finishes
- the duplicate check in the editor was executed after the webview loads,
leading to it hanging until the sidebar finishes loading. Run it at
set_note() time instead, so that the editor loads first.
- don't throw an error when a long-running op started with with_progress()
finishes after the window it was launched from has closed
- don't throw an error when the browser is closed before the sidebar
has finished loading

ftl/core/browsing.ftl

@@ -141,3 +141,8 @@ browsing-sidebar-due-today = Due
 browsing-sidebar-untagged = Untagged
 browsing-sidebar-overdue = Overdue
 browsing-row-deleted = (deleted)
+browsing-removed-unused-tags-count =
+    { $count ->
+        [one] Removed { $count } unused tag.
+       *[other] Removed { $count } unused tags.
+    }

pylib/anki/tags.py

@@ -44,17 +44,8 @@ class TagManager:
     # Registering and fetching tags
     #############################################################
 
-    def register(
+    def clear_unused_tags(self) -> OpChangesWithCount:
-        self, tags: Collection[str], usn: Optional[int] = None, clear: bool = False
+        return self.col._backend.clear_unused_tags()
-    ) -> None:
-        print("tags.register() is deprecated and no longer works")
-
-    def registerNotes(self, nids: Optional[List[int]] = None) -> None:
-        "Clear unused tags and add any missing tags from notes to the tag list."
-        self.clear_unused_tags()
-
-    def clear_unused_tags(self) -> None:
-        self.col._backend.clear_unused_tags()
 
     def byDeck(self, did: int, children: bool = False) -> List[str]:
         basequery = "select n.tags from cards c, notes n WHERE c.nid = n.id"
@@ -170,3 +161,14 @@ class TagManager:
     def inList(self, tag: str, tags: List[str]) -> bool:
         "True if TAG is in TAGS. Ignore case."
         return tag.lower() in [t.lower() for t in tags]
+
+    # legacy
+    ##########################################################################
+
+    def registerNotes(self, nids: Optional[List[int]] = None) -> None:
+        self.clear_unused_tags()
+
+    def register(
+        self, tags: Collection[str], usn: Optional[int] = None, clear: bool = False
+    ) -> None:
+        print("tags.register() is deprecated and no longer works")

qt/aqt/browser.py

@@ -25,7 +25,13 @@ from aqt.card_ops import set_card_deck, set_card_flag
 from aqt.editor import Editor
 from aqt.exporting import ExportDialog
 from aqt.main import ResetReason
-from aqt.note_ops import add_tags, find_and_replace, remove_notes, remove_tags
+from aqt.note_ops import (
+    add_tags,
+    clear_unused_tags,
+    find_and_replace,
+    remove_notes,
+    remove_tags,
+)
 from aqt.previewer import BrowserPreviewer as PreviewDialog
 from aqt.previewer import Previewer
 from aqt.qt import *
@@ -103,6 +109,7 @@ class DataModel(QAbstractTableModel):
         self.cards: Sequence[int] = []
         self.cardObjs: Dict[int, Card] = {}
         self._refresh_needed = False
+        self.block_updates = False
 
     def getCard(self, index: QModelIndex) -> Optional[Card]:
         id = self.cards[index.row()]
@@ -129,6 +136,8 @@ class DataModel(QAbstractTableModel):
         return len(self.activeCols)
 
     def data(self, index: QModelIndex = QModelIndex(), role: int = 0) -> Any:
+        if self.block_updates:
+            return
         if not index.isValid():
             return
         if role == Qt.FontRole:
@@ -431,6 +440,9 @@ class StatusDelegate(QItemDelegate):
     def paint(
         self, painter: QPainter, option: QStyleOptionViewItem, index: QModelIndex
     ) -> None:
+        if self.model.block_updates:
+            return QItemDelegate.paint(self, painter, option, index)
+
         c = self.model.getCard(index)
         if not c:
             return QItemDelegate.paint(self, painter, option, index)
@@ -502,15 +514,16 @@ class Browser(QMainWindow):
         gui_hooks.browser_will_show(self)
         self.show()
 
-    def on_operation_will_execute(self) -> None:
+    def on_operations_will_execute(self) -> None:
         # make sure the card list doesn't try to refresh itself during the operation,
         # as that will block the UI
-        self.setUpdatesEnabled(False)
+        self.model.block_updates = True
+
+    def on_operations_did_execute(self) -> None:
+        self.model.block_updates = False
 
     def on_operation_did_execute(self, changes: OpChanges) -> None:
         focused = current_top_level_widget() == self
-        if focused:
-            self.setUpdatesEnabled(True)
         self.model.op_executed(changes, focused)
         self.sidebar.op_executed(changes, focused)
         if changes.note or changes.notetype:
@@ -547,7 +560,7 @@ class Browser(QMainWindow):
             f.actionRemove_Tags.triggered,
             lambda: self.remove_tags_from_selected_notes(),
         )
-        qconnect(f.actionClear_Unused_Tags.triggered, self.clearUnusedTags)
+        qconnect(f.actionClear_Unused_Tags.triggered, self.clear_unused_tags)
         qconnect(f.actionToggle_Mark.triggered, lambda: self.onMark())
         qconnect(f.actionChangeModel.triggered, self.onChangeModel)
         qconnect(f.actionFindDuplicates.triggered, self.onFindDupes)
@@ -1219,7 +1232,7 @@ where id in %s"""
     ) -> None:
         "Shows prompt if tags not provided."
         if not (
-            tags := self._maybe_prompt_for_tags(tags, tr(TR.BROWSING_ENTER_TAGS_TO_ADD))
+            tags := tags or self._prompt_for_tags(tr(TR.BROWSING_ENTER_TAGS_TO_ADD))
         ):
             return
         add_tags(mw=self.mw, note_ids=self.selectedNotes(), space_separated_tags=tags)
@@ -1228,19 +1241,14 @@ where id in %s"""
     def remove_tags_from_selected_notes(self, tags: Optional[str] = None) -> None:
         "Shows prompt if tags not provided."
         if not (
-            tags := self._maybe_prompt_for_tags(
+            tags := tags or self._prompt_for_tags(tr(TR.BROWSING_ENTER_TAGS_TO_DELETE))
-                tags, tr(TR.BROWSING_ENTER_TAGS_TO_DELETE)
-            )
         ):
             return
         remove_tags(
             mw=self.mw, note_ids=self.selectedNotes(), space_separated_tags=tags
         )
 
-    def _maybe_prompt_for_tags(self, tags: Optional[str], prompt: str) -> Optional[str]:
+    def _prompt_for_tags(self, prompt: str) -> Optional[str]:
-        if tags is not None:
-            return tags
-
         (tags, ok) = getTag(self, self.col, prompt)
         if not ok:
             return None
@@ -1248,15 +1256,12 @@ where id in %s"""
             return tags
 
     @ensure_editor_saved_on_trigger
-    def clearUnusedTags(self) -> None:
+    def clear_unused_tags(self) -> None:
-        def on_done(fut: Future) -> None:
+        clear_unused_tags(mw=self.mw, parent=self)
-            fut.result()
-            self.on_tag_list_update()
-
-        self.mw.taskman.run_in_background(self.col.tags.registerNotes, on_done)
 
     addTags = add_tags_to_selected_notes
     deleteTags = remove_tags_from_selected_notes
+    clearUnusedTags = clear_unused_tags
 
     # Suspending
     ######################################################################
@@ -1419,7 +1424,8 @@ where id in %s"""
         # fixme: remove these once all items are using `operation_did_execute`
         gui_hooks.sidebar_should_refresh_decks.append(self.on_item_added)
         gui_hooks.sidebar_should_refresh_notetypes.append(self.on_item_added)
-        gui_hooks.operation_will_execute.append(self.on_operation_will_execute)
+        gui_hooks.operations_will_execute.append(self.on_operations_will_execute)
+        gui_hooks.operations_did_execute.append(self.on_operations_did_execute)
         gui_hooks.operation_did_execute.append(self.on_operation_did_execute)
         gui_hooks.focus_did_change.append(self.on_focus_change)
 
@@ -1427,7 +1433,8 @@ where id in %s"""
         gui_hooks.undo_state_did_change.remove(self.onUndoState)
         gui_hooks.sidebar_should_refresh_decks.remove(self.on_item_added)
         gui_hooks.sidebar_should_refresh_notetypes.remove(self.on_item_added)
-        gui_hooks.operation_will_execute.remove(self.on_operation_will_execute)
+        gui_hooks.operations_will_execute.remove(self.on_operations_will_execute)
+        gui_hooks.operations_did_execute.remove(self.on_operations_will_execute)
         gui_hooks.operation_did_execute.remove(self.on_operation_did_execute)
         gui_hooks.focus_did_change.remove(self.on_focus_change)
 

qt/aqt/editor.py

@@ -1,5 +1,8 @@
 # Copyright: Ankitects Pty Ltd and contributors
 # License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
+
+from __future__ import annotations
+
 import base64
 import html
 import itertools
@@ -24,7 +27,7 @@ from anki.collection import Config, SearchNode
 from anki.consts import MODEL_CLOZE
 from anki.hooks import runFilter
 from anki.httpclient import HttpClient
-from anki.notes import Note
+from anki.notes import DuplicateOrEmptyResult, Note
 from anki.utils import checksum, isLin, isWin, namedtmp
 from aqt import AnkiQt, colors, gui_hooks
 from aqt.note_ops import update_note
@@ -469,10 +472,10 @@ class Editor:
                     # event has had time to fire
                     self.mw.progress.timer(100, self.loadNoteKeepingFocus, False)
                 else:
-                    self.checkValid()
+                    self._check_and_update_duplicate_display_async()
             else:
                 gui_hooks.editor_did_fire_typing_timer(self.note)
-                self.checkValid()
+                self._check_and_update_duplicate_display_async()
 
         # focused into field?
         elif cmd.startswith("focus"):
@@ -529,11 +532,15 @@ class Editor:
         self.widget.show()
         self.updateTags()
 
+        dupe_status = self.note.duplicate_or_empty()
+
         def oncallback(arg: Any) -> None:
             if not self.note:
                 return
             self.setupForegroundButton()
-            self.checkValid()
+            # we currently do this synchronously to ensure we load before the
+            # sidebar on browser startup
+            self._update_duplicate_display(dupe_status)
             if focusTo is not None:
                 self.web.setFocus()
             gui_hooks.editor_did_load_note(self)
@@ -577,15 +584,26 @@ class Editor:
             # calling code may not expect the callback to fire immediately
             self.mw.progress.timer(10, callback, False)
             return
-        self.saveTags()
+        self.blur_tags_if_focused()
         self.web.evalWithCallback("saveNow(%d)" % keepFocus, lambda res: callback())
 
     saveNow = call_after_note_saved
 
-    def checkValid(self) -> None:
+    def _check_and_update_duplicate_display_async(self) -> None:
+        note = self.note
+
+        def on_done(result: DuplicateOrEmptyResult.V) -> None:
+            if self.note != note:
+                return
+            self._update_duplicate_display(result)
+
+        self.mw.query_op(self.note.duplicate_or_empty, success=on_done)
+
+    checkValid = _check_and_update_duplicate_display_async
+
+    def _update_duplicate_display(self, result: DuplicateOrEmptyResult.V) -> None:
         cols = [""] * len(self.note.fields)
-        err = self.note.duplicate_or_empty()
+        if result == DuplicateOrEmptyResult.DUPLICATE:
-        if err == 2:
             cols[0] = "dupe"
 
         self.web.eval(f"setBackgrounds({json.dumps(cols)});")
@@ -681,7 +699,7 @@ class Editor:
         l = QLabel(tr(TR.EDITING_TAGS))
         tb.addWidget(l, 1, 0)
         self.tags = aqt.tagedit.TagEdit(self.widget)
-        qconnect(self.tags.lostFocus, self.saveTags)
+        qconnect(self.tags.lostFocus, self.on_tag_focus_lost)
         self.tags.setToolTip(
             shortcut(tr(TR.EDITING_JUMP_TO_TAGS_WITH_CTRLANDSHIFTANDT))
         )
@@ -697,13 +715,17 @@ class Editor:
         if not self.tags.text() or not self.addMode:
             self.tags.setText(self.note.stringTags().strip())
 
-    def saveTags(self) -> None:
+    def on_tag_focus_lost(self) -> None:
-        if not self.note:
-            return
         self.note.tags = self.mw.col.tags.split(self.tags.text())
+        gui_hooks.editor_did_update_tags(self.note)
         if not self.addMode:
             self._save_current_note()
-        gui_hooks.editor_did_update_tags(self.note)
+
+    def blur_tags_if_focused(self) -> None:
+        if not self.note:
+            return
+        if self.tags.hasFocus():
+            self.widget.setFocus()
 
     def hideCompleters(self) -> None:
         self.tags.hideCompleter()
@@ -712,9 +734,12 @@ class Editor:
         self.tags.setFocus()
 
     # legacy
+
     def saveAddModeVars(self) -> None:
         pass
 
+    saveTags = blur_tags_if_focused
+
     # Format buttons
     ######################################################################
 

qt/aqt/main.py

@@ -715,6 +715,48 @@ class AnkiQt(QMainWindow):
     # Resetting state
     ##########################################################################
 
+    def query_op(
+        self,
+        op: Callable[[], Any],
+        *,
+        success: Callable[[Any], Any] = None,
+        failure: Optional[Callable[[Exception], Any]] = None,
+    ) -> None:
+        """Run an operation that queries the DB on a background thread.
+
+        Similar interface to perform_op(), but intended to be used for operations
+        that do not change collection state. Undo status will not be changed,
+        and `operation_did_execute` will not fire. No progress window will
+        be shown either.
+
+        `operations_will|did_execute` will still fire, so the UI can defer
+        updates during a background task.
+        """
+
+        def wrapped_done(future: Future) -> None:
+            self._decrease_background_ops()
+            # did something go wrong?
+            if exception := future.exception():
+                if isinstance(exception, Exception):
+                    if failure:
+                        failure(exception)
+                    else:
+                        showWarning(str(exception))
+                    return
+                else:
+                    # BaseException like SystemExit; rethrow it
+                    future.result()
+
+            result = future.result()
+            if success:
+                success(result)
+
+        self._increase_background_ops()
+        self.taskman.run_in_background(op, wrapped_done)
+
+    # Resetting state
+    ##########################################################################
+
     def perform_op(
         self,
         op: Callable[[], ResultWithChanges],
@@ -750,9 +792,10 @@ class AnkiQt(QMainWindow):
         they invoke themselves.
         """
 
-        gui_hooks.operation_will_execute()
+        self._increase_background_ops()
 
         def wrapped_done(future: Future) -> None:
+            self._decrease_background_ops()
             # did something go wrong?
             if exception := future.exception():
                 if isinstance(exception, Exception):
@@ -764,8 +807,9 @@ class AnkiQt(QMainWindow):
                 else:
                     # BaseException like SystemExit; rethrow it
                     future.result()
-            try:
+
             result = future.result()
+            try:
                 if success:
                     success(result)
             finally:
@@ -777,6 +821,17 @@ class AnkiQt(QMainWindow):
 
         self.taskman.with_progress(op, wrapped_done)
 
+    def _increase_background_ops(self) -> None:
+        if not self._background_op_count:
+            gui_hooks.operations_will_execute()
+        self._background_op_count += 1
+
+    def _decrease_background_ops(self) -> None:
+        self._background_op_count -= 1
+        if not self._background_op_count:
+            gui_hooks.operations_did_execute()
+        assert self._background_op_count >= 0
+
     def _fire_change_hooks_after_op_performed(
         self, result: ResultWithChanges, after_hooks: Optional[Callable[[], None]]
     ) -> None:
@@ -991,6 +1046,7 @@ title="%s" %s>%s</button>""" % (
 
     def setupThreads(self) -> None:
         self._mainThread = QThread.currentThread()
+        self._background_op_count = 0
 
     def inMainThread(self) -> bool:
         return self._mainThread == QThread.currentThread()

qt/aqt/note_ops.py

@@ -9,7 +9,7 @@ from anki.lang import TR
 from anki.notes import Note
 from aqt import AnkiQt, QWidget
 from aqt.main import PerformOpOptionalSuccessCallback
-from aqt.utils import show_invalid_search_error, showInfo, tr
+from aqt.utils import show_invalid_search_error, showInfo, tooltip, tr
 
 
 def add_note(
@@ -48,6 +48,15 @@ def remove_tags(
     mw.perform_op(lambda: mw.col.tags.bulk_remove(note_ids, space_separated_tags))
 
 
+def clear_unused_tags(*, mw: AnkiQt, parent: QWidget) -> None:
+    mw.perform_op(
+        mw.col.tags.clear_unused_tags,
+        success=lambda out: tooltip(
+            tr(TR.BROWSING_REMOVED_UNUSED_TAGS_COUNT, count=out.count), parent=parent
+        ),
+    )
+
+
 def find_and_replace(
     *,
     mw: AnkiQt,

qt/aqt/progress.py

@@ -179,6 +179,9 @@ class ProgressManager:
                 if elap >= 0.5:
                     break
                 self.app.processEvents(QEventLoop.ExcludeUserInputEvents)  # type: ignore #possibly related to https://github.com/python/mypy/issues/6910
+        # if the parent window has been deleted, the progress dialog may have
+        # already been dropped; delete it if it hasn't been
+        if not sip.isdeleted(self._win):
             self._win.cancel()
         self._win = None
         self._shown = 0

qt/aqt/sidebar.py

@@ -440,16 +440,17 @@ class SidebarTreeView(QTreeView):
         if not self.isVisible():
             return
 
-        def on_done(fut: Future) -> None:
+        def on_done(root: SidebarItem) -> None:
-            self.setUpdatesEnabled(True)
+            # user may have closed browser
-            root = fut.result()
+            if sip.isdeleted(self):
+                return
+
+            # block repainting during refreshing to avoid flickering
+            self.setUpdatesEnabled(False)
+
             model = SidebarModel(self, root)
-
-            # from PyQt5.QtTest import QAbstractItemModelTester
-            # tester = QAbstractItemModelTester(model)
-
             self.setModel(model)
-            qconnect(self.selectionModel().selectionChanged, self._on_selection_changed)
+
             if self.current_search:
                 self.search_for(self.current_search)
             else:
@@ -457,9 +458,12 @@ class SidebarTreeView(QTreeView):
             if is_current:
                 self.restore_current(is_current)
 
-        # block repainting during refreshing to avoid flickering
+            self.setUpdatesEnabled(True)
-        self.setUpdatesEnabled(False)
+
-        self.mw.taskman.run_in_background(self._root_tree, on_done)
+            # needs to be set after changing model
+            qconnect(self.selectionModel().selectionChanged, self._on_selection_changed)
+
+        self.mw.query_op(self._root_tree, success=on_done)
 
     def restore_current(self, is_current: Callable[[SidebarItem], bool]) -> None:
         if current := self.find_item(is_current):

qt/aqt/taskman.py

@@ -3,6 +3,8 @@
 
 """
 Helper for running tasks on background threads.
+
+See mw.query_op() and mw.perform_op() for slightly higher-level routines.
 """
 
 from __future__ import annotations
@@ -49,6 +51,14 @@ class TaskManager(QObject):
         the completed future.
 
         Args if provided will be passed on as keyword arguments to the task callable."""
+        # Before we launch a background task, ensure any pending on_done closure are run on
+        # main. Qt's signal/slot system will have posted a notification, but it may
+        # not have been processed yet. The on_done() closures may make small queries
+        # to the database that we want to run first - if we delay them until after the
+        # background task starts, and it takes out a long-running lock on the database,
+        # the UI thread will hang until the end of the op.
+        self._on_closures_pending()
+
         if args is None:
             args = {}
 

qt/tools/genhooks_gui.py

@@ -400,10 +400,19 @@ hooks = [
         """,
     ),
     Hook(
-        name="operation_will_execute",
+        name="operations_will_execute",
-        doc="""Called before an operation is executed with mw.perform_op().
+        doc="""Called before one or more operations are executed with mw.perform_op().
-        Subscribers can use this to ensure they don't try to access the collection until the operation completes,
+        
-        as doing so on the main thread will temporarily freeze the UI.""",
+        Subscribers can use this to set a flag to avoid DB updates until the operation
+        completes, as doing so will freeze the UI.
+        """,
+    ),
+    Hook(
+        name="operations_did_execute",
+        doc="""Called after one or more operations are executed with mw.perform_op().
+        Called regardless of the success of individual operations, and only called when
+        there are no outstanding ops.
+        """,
     ),
     Hook(
         name="operation_did_execute",

rslib/backend.proto

@@ -217,7 +217,7 @@ service DeckConfigService {
 }
 
 service TagsService {
-  rpc ClearUnusedTags(Empty) returns (Empty);
+  rpc ClearUnusedTags(Empty) returns (OpChangesWithCount);
   rpc AllTags(Empty) returns (StringList);
   rpc ExpungeTags(String) returns (UInt32);
   rpc SetTagExpanded(SetTagExpandedIn) returns (Empty);

rslib/src/backend/tags.rs

@@ -6,7 +6,7 @@ use crate::{backend_proto as pb, prelude::*};
 pub(super) use pb::tags_service::Service as TagsService;
 
 impl TagsService for Backend {
-    fn clear_unused_tags(&self, _input: pb::Empty) -> Result<pb::Empty> {
+    fn clear_unused_tags(&self, _input: pb::Empty) -> Result<pb::OpChangesWithCount> {
         self.with_col(|col| col.transact_no_undo(|col| col.clear_unused_tags().map(Into::into)))
     }
 

rslib/src/ops.rs

@@ -9,6 +9,7 @@ pub enum Op {
     AddNote,
     AnswerCard,
     Bury,
+    ClearUnusedTags,
     FindAndReplace,
     RemoveDeck,
     RemoveNote,
@@ -48,6 +49,7 @@ impl Op {
             Op::SetDeck => TR::BrowsingChangeDeck,
             Op::SetFlag => TR::UndoSetFlag,
             Op::FindAndReplace => TR::BrowsingFindAndReplace,
+            Op::ClearUnusedTags => TR::BrowsingClearUnusedTags,
         };
 
         i18n.tr(key).to_string()

rslib/src/tags/mod.rs

@@ -276,20 +276,25 @@ impl Collection {
         Ok(None)
     }
 
-    pub fn clear_unused_tags(&self) -> Result<()> {
+    /// Remove tags not referenced by notes, returning removed count.
-        let expanded: HashSet<_> = self.storage.expanded_tags()?.into_iter().collect();
+    pub fn clear_unused_tags(&mut self) -> Result<OpOutput<usize>> {
-        self.storage.clear_all_tags()?;
+        self.transact(Op::ClearUnusedTags, |col| col.clear_unused_tags_inner())
-        let usn = self.usn()?;
-        for name in self.storage.all_tags_in_notes()? {
-            let name = normalize_tag_name(&name).into();
-            self.storage.register_tag(&Tag {
-                expanded: expanded.contains(&name),
-                name,
-                usn,
-            })?;
     }
 
-        Ok(())
+    fn clear_unused_tags_inner(&mut self) -> Result<usize> {
+        let mut count = 0;
+        let in_notes = self.storage.all_tags_in_notes()?;
+        let need_remove = self
+            .storage
+            .all_tags()?
+            .into_iter()
+            .filter(|tag| !in_notes.contains(&tag.name));
+        for tag in need_remove {
+            self.remove_single_tag_undoable(tag)?;
+            count += 1;
+        }
+
+        Ok(count)
     }
 
     /// Take tags as a whitespace-separated string and remove them from all notes and the storage.

rslib/src/tags/undo.rs

@@ -13,7 +13,7 @@ pub(crate) enum UndoableTagChange {
 impl Collection {
     pub(crate) fn undo_tag_change(&mut self, change: UndoableTagChange) -> Result<()> {
         match change {
-            UndoableTagChange::Added(tag) => self.remove_single_tag_undoable(&tag),
+            UndoableTagChange::Added(tag) => self.remove_single_tag_undoable(*tag),
             UndoableTagChange::Removed(tag) => self.register_tag_undoable(&tag),
         }
     }
@@ -24,8 +24,9 @@ impl Collection {
         self.storage.register_tag(&tag)
     }
 
-    fn remove_single_tag_undoable(&mut self, tag: &Tag) -> Result<()> {
+    pub(super) fn remove_single_tag_undoable(&mut self, tag: Tag) -> Result<()> {
-        self.save_undo(UndoableTagChange::Removed(Box::new(tag.clone())));
+        self.storage.remove_single_tag(&tag.name)?;
-        self.storage.remove_single_tag(&tag.name)
+        self.save_undo(UndoableTagChange::Removed(Box::new(tag)));
+        Ok(())
     }
 }