From a0d0f2f8fd7cca6f3426c9f28ad8ffdfdbcc648c Mon Sep 17 00:00:00 2001 From: RumovZ Date: Thu, 24 Feb 2022 12:15:56 +0100 Subject: [PATCH] Add progress.single_shot() (#1683) * Add progress.single_shot() * Fix periodic garbage collection * Properly cleanup mediasync timers * Revert some replacements with `single_shot()` These timers shouldn't fire if their widget is destroyed. * Add timer docs explaining issues and alternatives * Apply suggestions from code review * Tweak docstrings --- qt/aqt/addons.py | 8 ++--- qt/aqt/browser/previewer.py | 2 +- qt/aqt/editor.py | 2 +- qt/aqt/main.py | 20 ++++------- qt/aqt/mediasync.py | 4 +-- qt/aqt/progress.py | 66 ++++++++++++++++++++++++++++++------- qt/aqt/webview.py | 2 +- 7 files changed, 68 insertions(+), 36 deletions(-) diff --git a/qt/aqt/addons.py b/qt/aqt/addons.py index 45d4e3325..351706778 100644 --- a/qt/aqt/addons.py +++ b/qt/aqt/addons.py @@ -1150,9 +1150,7 @@ class DownloaderInstaller(QObject): self.mgr.mw.progress.finish() # qt gets confused if on_done() opens new windows while the progress # modal is still cleaning up - self.mgr.mw.progress.timer( - 50, lambda: self.on_done(self.log), False, parent=self - ) + self.mgr.mw.progress.single_shot(50, lambda: self.on_done(self.log)) def show_log_to_user(parent: QWidget, log: list[DownloadLogEntry]) -> None: @@ -1401,12 +1399,10 @@ def check_for_updates( def update_info_received(future: Future) -> None: # if syncing/in profile screen, defer message delivery if not mgr.mw.col: - mgr.mw.progress.timer( + mgr.mw.progress.single_shot( 1000, lambda: update_info_received(future), False, - requiresCollection=False, - parent=mgr.mw, ) return diff --git a/qt/aqt/browser/previewer.py b/qt/aqt/browser/previewer.py index c74a9b21d..fb36c4def 100644 --- a/qt/aqt/browser/previewer.py +++ b/qt/aqt/browser/previewer.py @@ -106,7 +106,7 @@ class Previewer(QDialog): def _on_finished(self, ok: int) -> None: saveGeom(self, "preview") - self.mw.progress.timer(100, self._on_close, False, parent=self) + self.mw.progress.single_shot(100, self._on_close) def _on_replay_audio(self) -> None: if self._state == "question": diff --git a/qt/aqt/editor.py b/qt/aqt/editor.py index 36b41820e..493c4d8f7 100644 --- a/qt/aqt/editor.py +++ b/qt/aqt/editor.py @@ -552,7 +552,7 @@ require("anki/ui").loaded.then(() => require("anki/NoteEditor").instances[0].too "Save unsaved edits then call callback()." if not self.note: # calling code may not expect the callback to fire immediately - self.mw.progress.timer(10, callback, False, parent=self.widget) + self.mw.progress.single_shot(10, callback) return self.web.evalWithCallback("saveNow(%d)" % keepFocus, lambda res: callback()) diff --git a/qt/aqt/main.py b/qt/aqt/main.py index 3c0786643..619506fa6 100644 --- a/qt/aqt/main.py +++ b/qt/aqt/main.py @@ -187,9 +187,7 @@ class AnkiQt(QMainWindow): fn() gui_hooks.main_window_did_init() - self.progress.timer( - 10, on_window_init, False, requiresCollection=False, parent=self - ) + self.progress.single_shot(10, on_window_init, False) def setupUI(self) -> None: self.col = None @@ -223,12 +221,10 @@ class AnkiQt(QMainWindow): def setupProfileAfterWebviewsLoaded(self) -> None: for w in (self.web, self.bottomWeb): if not w._domDone: - self.progress.timer( + self.progress.single_shot( 10, self.setupProfileAfterWebviewsLoaded, False, - requiresCollection=False, - parent=self, ) return else: @@ -914,7 +910,7 @@ title="{}" {}>{}""".format( self.col.db.rollback() self.close() - self.progress.timer(100, quit, False, parent=self) + self.progress.single_shot(100, quit) def setupProgress(self) -> None: self.progress = aqt.progress.ProgressManager(self) @@ -1361,7 +1357,7 @@ title="{}" {}>{}""".format( self.progress.timer(5 * 60 * 1000, self.on_autosync_timer, True, parent=self) # periodic garbage collection self.progress.timer( - 15 * 60 * 1000, self.garbage_collect_now, False, parent=self + 15 * 60 * 1000, self.garbage_collect_now, True, False, parent=self ) # ensure Python interpreter runs at least once per second, so that # SIGINT/SIGTERM is processed without a long delay @@ -1693,12 +1689,10 @@ title="{}" {}>{}""".format( if self.state == "startup": # try again in a second - self.progress.timer( + self.progress.single_shot( 1000, lambda: self.onAppMsg(buf), False, - requiresCollection=False, - parent=self, ) return elif self.state == "profileManager": @@ -1764,9 +1758,7 @@ title="{}" {}>{}""".format( def deferred_delete_and_garbage_collect(self, obj: QObject) -> None: obj.deleteLater() - self.progress.timer( - 1000, self.garbage_collect_now, False, requiresCollection=False, parent=self - ) + self.progress.single_shot(1000, self.garbage_collect_now, False) def disable_automatic_garbage_collection(self) -> None: gc.collect() diff --git a/qt/aqt/mediasync.py b/qt/aqt/mediasync.py index 4326ccc94..04fcf1a7b 100644 --- a/qt/aqt/mediasync.py +++ b/qt/aqt/mediasync.py @@ -78,7 +78,7 @@ class MediaSyncer: def _on_finished(self, future: Future) -> None: self._syncing = False if self._progress_timer: - self._progress_timer.stop() + self._progress_timer.deleteLater() self._progress_timer = None gui_hooks.media_sync_did_start_or_stop(False) @@ -131,7 +131,7 @@ class MediaSyncer: def check_finished() -> None: if not self.is_syncing(): - timer.stop() + timer.deleteLater() on_finished() timer = self.mw.progress.timer(150, check_finished, True, False, parent=self.mw) diff --git a/qt/aqt/progress.py b/qt/aqt/progress.py index 633928b97..3aea4f652 100644 --- a/qt/aqt/progress.py +++ b/qt/aqt/progress.py @@ -26,7 +26,7 @@ class ProgressManager: # Safer timers ########################################################################## - # A custom timer which avoids firing while a progress dialog is active + # Custom timers which avoid firing while a progress dialog is active # (likely due to some long-running DB operation) def timer( @@ -38,7 +38,7 @@ class ProgressManager: *, parent: QObject = None, ) -> QTimer: - """Create and start a standard Anki timer. + """Create and start a standard Anki timer. For an alternative see `single_shot()`. If the timer fires while a progress window is shown: - if it is a repeating timer, it will wait the same delay again @@ -47,16 +47,65 @@ class ProgressManager: If requiresCollection is True, the timer will not fire if the collection has been unloaded. Setting it to False will allow the timer to fire even when there is no collection, but will still - only fire when there is no current progress dialog.""" + only fire when there is no current progress dialog. + + + Issues and alternative + --- + The created timer will only be destroyed when `parent` is destroyed. + This can cause memory leaks, because anything captured by `func` isn't freed either. + If there is no QObject that will get destroyed reasonably soon, and you have to + pass `mw`, you should call `deleteLater()` on the returned QTimer as soon as + it's served its purpose, or use `single_shot()`. + + Also note that you may not be able to pass an adequate parent, if you want to + make a callback after a widget closes. If you passed that widget, the timer + would get destroyed before it could fire. + """ if parent is None: print_deprecation_warning( "to avoid memory leaks, pass an appropriate parent to progress.timer()" + " or use progress.single_shot()" ) parent = self.mw + qtimer = QTimer(parent) + if not repeat: + qtimer.setSingleShot(True) + qconnect(qtimer.timeout, self._get_handler(func, repeat, requiresCollection)) + qtimer.start(ms) + return qtimer + + def single_shot( + self, + ms: int, + func: Callable[[], None], + requires_collection: bool = True, + ) -> None: + """Create and start a one-off Anki timer. For an alternative and more + documentation, see `timer()`. + + + Issues and alternative + --- + `single_shot()` cleans itself up, so a passed closure won't leak any memory. + However, if `func` references a QObject other than `mw`, which gets deleted before the + timer fires, an Exception is raised. To avoid this, either use `timer()` passing + that object as the parent, or check in `func` with `sip.isdeleted(object)` if + it still exists. + + On the other hand, if a widget is supposed to make an external callback after it closes, + you likely want to use `single_shot()`, which will fire even if the calling + widget is already destroyed. + """ + QTimer.singleShot(ms, self._get_handler(func, False, requires_collection)) + + def _get_handler( + self, func: Callable[[], None], repeat: bool, requires_collection: bool + ) -> Callable[[], None]: def handler() -> None: - if requiresCollection and not self.mw.col: + if requires_collection and not self.mw.col: # no current collection; timer is no longer valid print(f"Ignored progress func as collection unloaded: {repr(func)}") return @@ -70,14 +119,9 @@ class ProgressManager: pass else: # retry in 100ms - self.timer(100, func, False, requiresCollection, parent=parent) + self.single_shot(100, func, requires_collection) - t = QTimer(parent) - if not repeat: - t.setSingleShot(True) - qconnect(t.timeout, handler) - t.start(ms) - return t + return handler # Creating progress dialogs ########################################################################## diff --git a/qt/aqt/webview.py b/qt/aqt/webview.py index 1d3557596..afa1c3617 100644 --- a/qt/aqt/webview.py +++ b/qt/aqt/webview.py @@ -628,7 +628,7 @@ html {{ {font} }} if qvar is None: - mw.progress.timer(1000, mw.reset, False, parent=self) + mw.progress.single_shot(1000, mw.reset) return self.setFixedHeight(int(qvar))