From 112cbe8b59d73fb10e2b1f7a7215f4c28f13635a Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Sat, 13 Mar 2021 23:59:32 +1000 Subject: [PATCH] experiment with finer-scoped reset in perform_op() Basic proof of concept, where the 'delete note' operation in the reviewer has been updated to use mw.perform_op(). Instead of manually calling .reset() afterwards, a summary of the changes is returned as part of the undo status query, and various parts of the GUI can listen to gui_hooks.operation_did_execute and decide whether they want to redraw based on the scope of the changes. This should allow the sidebar to selectively redraw just the tags area in the future for example. Currently we're just listing out all possible areas that might be changed; in the future we could theoretically inspect the specific changes in the undo log to provide a more accurate report (avoiding refreshing the tags list when no tags were added for example). You can test it out by opening the browse screen while studying, and then deleting the current card - the browser should update to show (deleted) on the cards due the earlier change. If going ahead with this, aside from updating all the screens that currently listen for resets, some thought will be required on how we can integrate it with legacy code that expects to called when resets are made, and expects to call .reset() when it makes changes. Thoughts? --- pylib/anki/collection.py | 1 + qt/aqt/browser.py | 60 ++++++++++++---------------------- qt/aqt/main.py | 69 +++++++++++++++++++++++++++++++++++++++- qt/aqt/reviewer.py | 23 +++++++++++--- qt/tools/genhooks_gui.py | 19 ++++++++++- rslib/src/ops.rs | 3 ++ 6 files changed, 130 insertions(+), 45 deletions(-) diff --git a/pylib/anki/collection.py b/pylib/anki/collection.py index c3113fa9b..dcd4d2fd6 100644 --- a/pylib/anki/collection.py +++ b/pylib/anki/collection.py @@ -54,6 +54,7 @@ GraphPreferences = _pb.GraphPreferences BuiltinSort = _pb.SortOrder.Builtin Preferences = _pb.Preferences UndoStatus = _pb.UndoStatus +StateChanges = _pb.StateChanges DefaultsForAdding = _pb.DeckAndNotetype diff --git a/qt/aqt/browser.py b/qt/aqt/browser.py index 4711e4732..9c7332474 100644 --- a/qt/aqt/browser.py +++ b/qt/aqt/browser.py @@ -12,7 +12,7 @@ from typing import Any, Callable, Dict, List, Optional, Sequence, Tuple, Union, import aqt import aqt.forms from anki.cards import Card -from anki.collection import Collection, Config, SearchNode +from anki.collection import Collection, Config, SearchNode, StateChanges from anki.consts import * from anki.errors import InvalidInput, NotFoundError from anki.lang import without_unicode_isolation @@ -281,6 +281,15 @@ class DataModel(QAbstractTableModel): else: tv.selectRow(0) + def maybe_redraw_after_operation(self, changes: StateChanges) -> None: + if ( + changes.card_modified + or changes.note_modified + or changes.deck_modified + or changes.notetype_modified + ): + self.reset() + # Column data ###################################################################### @@ -434,8 +443,6 @@ class StatusDelegate(QItemDelegate): # Browser window ###################################################################### -# fixme: respond to reset+edit hooks - class Browser(QMainWindow): model: DataModel @@ -481,43 +488,14 @@ class Browser(QMainWindow): gui_hooks.browser_will_show(self) self.show() - def perform_op( - self, - op: Callable, - on_done: Callable[[Future], None], - *, - reset_model: bool = True, - ) -> None: - """Run the provided operation on a background thread. - - Ensures any changes in the editor have been saved. - - Shows progress popup for the duration of the op. - - Ensures the browser doesn't try to redraw during the operation, which can lead - to a frozen UI - - Updates undo state at the end of the operation - - If `reset_model` is true, calls beginReset()/endReset(), which will - refresh the displayed data, and update the editor's note. If the current search - has changed results, you will need to call .search() yourself in `on_done`. + def on_operation_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) - Caller must run fut.result() in the on_done() callback to check for errors; - if the operation returned a value, it will be returned by .result() - """ - - def wrapped_op() -> None: - if reset_model: - self.model.beginReset() - self.setUpdatesEnabled(False) - op() - - def wrapped_done(fut: Future) -> None: - self.setUpdatesEnabled(True) - on_done(fut) - if reset_model: - self.model.endReset() - self.mw.update_undo_actions() - - self.editor.saveNow( - lambda: self.mw.taskman.with_progress(wrapped_op, wrapped_done) - ) + def on_operation_did_execute(self, changes: StateChanges) -> None: + self.setUpdatesEnabled(True) + self.model.maybe_redraw_after_operation(changes) def setupMenus(self) -> None: # pylint: disable=unnecessary-lambda @@ -1466,6 +1444,8 @@ where id in %s""" gui_hooks.editor_did_unfocus_field.append(self.on_unfocus_field) 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.operation_did_execute.append(self.on_operation_did_execute) def teardownHooks(self) -> None: gui_hooks.undo_state_did_change.remove(self.onUndoState) @@ -1475,6 +1455,8 @@ where id in %s""" gui_hooks.editor_did_unfocus_field.remove(self.on_unfocus_field) 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.operation_did_execute.remove(self.on_operation_did_execute) def on_unfocus_field(self, changed: bool, note: Note, field_idx: int) -> None: self.refreshCurrentCard(note) diff --git a/qt/aqt/main.py b/qt/aqt/main.py index eef4e99d0..392802407 100644 --- a/qt/aqt/main.py +++ b/qt/aqt/main.py @@ -14,7 +14,18 @@ import zipfile from argparse import Namespace from concurrent.futures import Future from threading import Thread -from typing import Any, Callable, Dict, List, Optional, Sequence, TextIO, Tuple, cast +from typing import ( + Any, + Callable, + Dict, + List, + Optional, + Sequence, + TextIO, + Tuple, + TypeVar, + cast, +) import anki import aqt @@ -34,6 +45,7 @@ from anki.collection import ( Config, ReviewUndo, UndoResult, + UndoStatus, ) from anki.decks import Deck from anki.hooks import runHook @@ -74,6 +86,8 @@ from aqt.utils import ( tr, ) +T = TypeVar("T") + install_pylib_legacy() @@ -694,6 +708,44 @@ class AnkiQt(QMainWindow): # Resetting state ########################################################################## + def perform_op( + self, + op: Callable[[], T], + on_success: Optional[Callable[[T], None]] = None, + on_exception: Optional[Callable[[BaseException], None]] = None, + ) -> None: + """Run the provided operation on a background thread. + - Ensures any changes in the editor have been saved. + - Shows progress popup for the duration of the op. + - Ensures the browser doesn't try to redraw during the operation, which can lead + to a frozen UI + - Updates undo state at the end of the operation + + on_success() will be called with the return value of op() + if op() threw an exception, on_exception() will be called with it, + if it was provided + + """ + + gui_hooks.operation_will_execute() + + def wrapped_done(future: Future) -> None: + try: + if exception := future.exception(): + if on_exception: + on_exception(exception) + else: + showWarning(str(exception)) + else: + if on_success: + on_success(future.result()) + finally: + status = self.col.undo_status() + self._update_undo_actions_for_status(status) + gui_hooks.operation_did_execute(status.changes) + + self.taskman.with_progress(op, wrapped_done) + def reset(self, guiOnly: bool = False) -> None: "Called for non-trivial edits. Rebuilds queue and updates UI." if self.col: @@ -1108,6 +1160,21 @@ title="%s" %s>%s""" % ( self.form.actionUndo.setEnabled(False) gui_hooks.undo_state_did_change(False) + def _update_undo_actions_for_status(self, status: UndoStatus) -> None: + """Update menu text and enable/disable menu item as appropriate. + Plural as this may handle redo in the future too.""" + undo_action = status.undo + + if undo_action: + undo_action = tr(TR.UNDO_UNDO_ACTION, val=undo_action) + self.form.actionUndo.setText(undo_action) + self.form.actionUndo.setEnabled(True) + gui_hooks.undo_state_did_change(True) + else: + self.form.actionUndo.setText(tr(TR.UNDO_UNDO)) + self.form.actionUndo.setEnabled(False) + gui_hooks.undo_state_did_change(False) + def checkpoint(self, name: str) -> None: self.col.save(name) self.update_undo_actions() diff --git a/qt/aqt/reviewer.py b/qt/aqt/reviewer.py index 420293ea3..21f009de1 100644 --- a/qt/aqt/reviewer.py +++ b/qt/aqt/reviewer.py @@ -13,7 +13,7 @@ from PyQt5.QtCore import Qt from anki import hooks from anki.cards import Card -from anki.collection import Config +from anki.collection import Config, StateChanges from anki.utils import stripHTML from aqt import AnkiQt, gui_hooks from aqt.profiles import VideoDriver @@ -63,6 +63,7 @@ class Reviewer: self.state: Optional[str] = None self.bottom = BottomBar(mw, mw.bottomWeb) hooks.card_did_leech.append(self.onLeech) + gui_hooks.operation_did_execute.append(self.on_operation_did_execute) def show(self) -> None: self.mw.col.reset() @@ -86,6 +87,18 @@ class Reviewer: gui_hooks.reviewer_will_end() self.card = None + def on_operation_did_execute(self, changes: StateChanges) -> None: + need_queue_rebuild = ( + changes.card_added + or changes.card_modified + or changes.deck_modified + or changes.preference_modified + ) + + if need_queue_rebuild: + self.mw.col.reset() + self.nextCard() + # Fetching a card ########################################################################## @@ -839,9 +852,11 @@ time = %(time)d; if self.mw.state != "review" or not self.card: return cnt = len(self.card.note().cards()) - self.mw.col.remove_notes([self.card.note().id]) - self.mw.reset() - tooltip(tr(TR.STUDYING_NOTE_AND_ITS_CARD_DELETED, count=cnt)) + + self.mw.perform_op( + lambda: self.mw.col.remove_notes([self.card.note().id]), + lambda _: tooltip(tr(TR.STUDYING_NOTE_AND_ITS_CARD_DELETED, count=cnt)), + ) def onRecordVoice(self) -> None: def after_record(path: str) -> None: diff --git a/qt/tools/genhooks_gui.py b/qt/tools/genhooks_gui.py index 4512dbbc9..d527e674c 100644 --- a/qt/tools/genhooks_gui.py +++ b/qt/tools/genhooks_gui.py @@ -365,8 +365,9 @@ hooks = [ args=["context: aqt.browser.SearchContext"], doc="""Allows you to modify the list of returned card ids from a search.""", ), - # States + # Main window states ################### + # these refer to things like deckbrowser, overview and reviewer state, Hook( name="state_will_change", args=["new_state: str", "old_state: str"], @@ -382,6 +383,8 @@ hooks = [ name="state_shortcuts_will_change", args=["state: str", "shortcuts: List[Tuple[str, Callable]]"], ), + # UI state/refreshing + ################### Hook( name="state_did_revert", args=["action: str"], @@ -393,6 +396,20 @@ hooks = [ legacy_hook="reset", doc="Called when the interface needs to be redisplayed after non-trivial changes have been made.", ), + 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.""", + ), + Hook( + name="operation_did_execute", + args=[ + "changes: anki.collection.StateChanges", + ], + doc="""Called after an operation completes. + Changes can be inspected to determine whether the UI needs updating.""", + ), # Webview ################### Hook( diff --git a/rslib/src/ops.rs b/rslib/src/ops.rs index 5d5b50416..3326f402e 100644 --- a/rslib/src/ops.rs +++ b/rslib/src/ops.rs @@ -61,6 +61,7 @@ impl Op { Op::AddNote => StateChanges { card_added: true, note_added: true, + tag_modified: true, ..default() }, Op::RemoveDeck => StateChanges { @@ -86,6 +87,8 @@ impl Op { note_modified: true, // edits may result in new cards being generated card_added: true, + // and may result in new tags being added + tag_modified: true, ..default() }, Op::UpdatePreferences => StateChanges {