Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add browser hook setup and teardown hooks #2201

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

glutanimate
Copy link
Contributor

@glutanimate glutanimate commented Nov 21, 2022

Makes it easier for add-ons to subscribe to events such as browser_did_change_row with closures that refer to the current browser instance or child objects thereof, e.g. addon-provided UI elements.

The concrete use case that spawned this: As part of a new feature, the AMBOSS add-on needs to add a button to the card browser that responds to the number of cards currently selected. As gui_hooks.browser_did_change_row is a global, but both the button and browser instance will likely be deleted during the session lifetime, adding a hook like browser_did_teardown_hooks allows the add-on to sever the connection at the appropriate time.

Debug console sanity check snippet:

from aqt import gui_hooks
gui_hooks.browser_did_setup_hooks.append(lambda *args: print(f"did_setup: {args}"))
gui_hooks.browser_did_teardown_hooks.append(lambda *args: print(f"did_teardown: {args}"))

A quick thought before merging: I'm wondering if instead you'd also be open to adding a Qt signal to Browser, e.g. selection_changed. That would make it even easier for add-ons to listen to selection state changes without having to clean up after themselves (as Qt takes care of severing the signal/slot connections). But, it throws more Qt-specificity into the mix and from what I can tell, Anki only uses a handful of custom Qt signals and I don't think they were ever explicitly regarded as add-on APIs.


Copyright disclosure: This patch was written as part of my work for AMBOSS. Its rights of use lie with AMBOSS MD Inc and it is being submitted in agreement with Anki's contributor license agreement, as signed by AMBOSS MD Inc. (see the CONTRIBUTORS file).

Allows add-ons to more easily update UI elements without complex browser instance management.
@glutanimate glutanimate force-pushed the browser-hook-setup-and-teardown-hooks branch from 809718e to 7724622 Compare November 21, 2022 01:06
@dae
Copy link
Member

dae commented Nov 21, 2022

I'd lean towards a hook-based approach, as they're more discoverable, and as you pointed out, Anki doesn't make use of many custom signals.

Happy to merge this, but just a question first - could you perhaps accomplish the same thing by using a weakref in your closure?

@dae
Copy link
Member

dae commented Mar 20, 2024

Is this something you still want merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants