diff --git a/ftl/core/browsing.ftl b/ftl/core/browsing.ftl index 2a2e14b85..46b317627 100644 --- a/ftl/core/browsing.ftl +++ b/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. + } diff --git a/pylib/anki/tags.py b/pylib/anki/tags.py index ffd9da9fc..253383ed3 100644 --- a/pylib/anki/tags.py +++ b/pylib/anki/tags.py @@ -44,17 +44,8 @@ class TagManager: # Registering and fetching tags ############################################################# - def register( - self, tags: Collection[str], usn: Optional[int] = None, clear: bool = False - ) -> 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 clear_unused_tags(self) -> OpChangesWithCount: + return 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") diff --git a/qt/aqt/browser.py b/qt/aqt/browser.py index d6e8ec212..437a83377 100644 --- a/qt/aqt/browser.py +++ b/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, tr(TR.BROWSING_ENTER_TAGS_TO_DELETE) - ) + tags := tags or self._prompt_for_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]: - if tags is not None: - return tags - + def _prompt_for_tags(self, prompt: str) -> Optional[str]: (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 on_done(fut: Future) -> None: - fut.result() - self.on_tag_list_update() - - self.mw.taskman.run_in_background(self.col.tags.registerNotes, on_done) + def clear_unused_tags(self) -> None: + clear_unused_tags(mw=self.mw, parent=self) 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) diff --git a/qt/aqt/editor.py b/qt/aqt/editor.py index 130cb96bd..b18e99537 100644 --- a/qt/aqt/editor.py +++ b/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 err == 2: + if result == DuplicateOrEmptyResult.DUPLICATE: 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: - if not self.note: - return + def on_tag_focus_lost(self) -> None: 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 ###################################################################### diff --git a/qt/aqt/main.py b/qt/aqt/main.py index d1c0e8bdb..6667e6048 100644 --- a/qt/aqt/main.py +++ b/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() + + result = future.result() try: - result = future.result() 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""" % ( def setupThreads(self) -> None: self._mainThread = QThread.currentThread() + self._background_op_count = 0 def inMainThread(self) -> bool: return self._mainThread == QThread.currentThread() diff --git a/qt/aqt/note_ops.py b/qt/aqt/note_ops.py index 36592537c..091b93d55 100644 --- a/qt/aqt/note_ops.py +++ b/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, diff --git a/qt/aqt/progress.py b/qt/aqt/progress.py index 6f3eeab70..9f52e26b2 100644 --- a/qt/aqt/progress.py +++ b/qt/aqt/progress.py @@ -179,7 +179,10 @@ class ProgressManager: if elap >= 0.5: break self.app.processEvents(QEventLoop.ExcludeUserInputEvents) # type: ignore #possibly related to https://github.com/python/mypy/issues/6910 - self._win.cancel() + # 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 diff --git a/qt/aqt/sidebar.py b/qt/aqt/sidebar.py index 7aa53479b..b3b70447c 100644 --- a/qt/aqt/sidebar.py +++ b/qt/aqt/sidebar.py @@ -440,16 +440,17 @@ class SidebarTreeView(QTreeView): if not self.isVisible(): return - def on_done(fut: Future) -> None: - self.setUpdatesEnabled(True) - root = fut.result() + def on_done(root: SidebarItem) -> None: + # user may have closed browser + 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(False) - self.mw.taskman.run_in_background(self._root_tree, on_done) + self.setUpdatesEnabled(True) + + # 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): diff --git a/qt/aqt/taskman.py b/qt/aqt/taskman.py index 0a059e910..3189024f3 100644 --- a/qt/aqt/taskman.py +++ b/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 = {} diff --git a/qt/tools/genhooks_gui.py b/qt/tools/genhooks_gui.py index 49dd359d4..692caa9ec 100644 --- a/qt/tools/genhooks_gui.py +++ b/qt/tools/genhooks_gui.py @@ -400,10 +400,19 @@ hooks = [ """, ), Hook( - name="operation_will_execute", - doc="""Called before an operation is 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.""", + name="operations_will_execute", + doc="""Called before one or more operations are executed with mw.perform_op(). + + 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", diff --git a/rslib/backend.proto b/rslib/backend.proto index 1fa44011e..1bc90634e 100644 --- a/rslib/backend.proto +++ b/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); diff --git a/rslib/src/backend/tags.rs b/rslib/src/backend/tags.rs index 26a15d72f..4c24d38e3 100644 --- a/rslib/src/backend/tags.rs +++ b/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 { + fn clear_unused_tags(&self, _input: pb::Empty) -> Result { self.with_col(|col| col.transact_no_undo(|col| col.clear_unused_tags().map(Into::into))) } diff --git a/rslib/src/ops.rs b/rslib/src/ops.rs index 4807719f4..f80bfdec1 100644 --- a/rslib/src/ops.rs +++ b/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() diff --git a/rslib/src/tags/mod.rs b/rslib/src/tags/mod.rs index ff2ed3cb4..553069db0 100644 --- a/rslib/src/tags/mod.rs +++ b/rslib/src/tags/mod.rs @@ -276,20 +276,25 @@ impl Collection { Ok(None) } - pub fn clear_unused_tags(&self) -> Result<()> { - let expanded: HashSet<_> = self.storage.expanded_tags()?.into_iter().collect(); - self.storage.clear_all_tags()?; - 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, - })?; + /// Remove tags not referenced by notes, returning removed count. + pub fn clear_unused_tags(&mut self) -> Result> { + self.transact(Op::ClearUnusedTags, |col| col.clear_unused_tags_inner()) + } + + fn clear_unused_tags_inner(&mut self) -> Result { + 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(()) + Ok(count) } /// Take tags as a whitespace-separated string and remove them from all notes and the storage. diff --git a/rslib/src/tags/undo.rs b/rslib/src/tags/undo.rs index 907a7ea86..3775dcfb2 100644 --- a/rslib/src/tags/undo.rs +++ b/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<()> { - self.save_undo(UndoableTagChange::Removed(Box::new(tag.clone()))); - self.storage.remove_single_tag(&tag.name) + 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))); + Ok(()) } }