-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Use CallbackRegistry in Widgets and some related cleanup #18226
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
Conversation
It also needs to be mentioned in the API change note as if you register a method into it currently we will keep a hard-ref that keeps that object alive, the CallbackRegistry will only keep a weakref and if you otherwise let the object get gc'd your callback will go away. I don't think we need a deprecation path on that.
On the other hand, to channel @anntzer he will say we should always keep a hard-ref and keep objects alive (I am skeptical of this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note subtle behavior in keeping the objects of registered methods alive.
Anyone can dismiss this review and if there is one other 👍 @QuLogic can self-merge.
53580ba to
4adba7a
Compare
I added an API change note.
(I'm not going to fight over this, at least being consistent within the library is good too.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only applies to methods, we keep hard-refs to everything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we think that there was anyone directly adding things to the observers dictionary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then they should have updated cnt so that followup additions would have the right number. I would expect them to get an error from that, and hopefully figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am 👍 on this, would like @timhoffm to review this before merged.
The wording on the deprecation can still be clarified a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Widget.cnt is roughly the next cid to be returned, but this is just the count of the callbacks added. I can't get the next cid without reaching into CallbackRegistry internals (and maybe not even then since it uses itertools.count().)
This simplifies processing, fixes memory leaks with weak references, and fixes bugs like being unable to disconnect a callback in its own code.
4adba7a to
ae888ca
Compare
Actually I missed two points here:
- We could have kept the old strong refs behavior by using the wrap-in-lambda trick (Prevent GC of animations and widgets still in use. #16221 /Use CallbackRegistry for {Artist,Collection}.add_callback. #18912 ).
- If you connect the same function twice to a CallbackRegistry, the second (and later) connections are silently dropped per which is perhaps even more likely to be a source of API breakage than the strong/weakref difference. Again wrapping in lambdas solves the issue (because each lambda is effectively unique, and equality of lambdas doesn't look into their implementation), but at least this should be noted in the API changes note. (This can easily be checked by e.g. comparing
matplotlib/lib/matplotlib/cbook/__init__.py
Lines 180 to 181 in ce29648
if proxy in self._func_cid_map[s]:return self._func_cid_map[s][proxy]for _ in range(2): gcf().canvas.mpl_connect("key_press_event", print)andfor _ in range(2): gcf().canvas.mpl_connect("key_press_event", lambda e: print(e)).)
May be nice to decide on this before 3.4 gets released @QuLogic @tacaswell @timhoffm
PR Summary
This came about from this Discourse post: https://discourse.matplotlib.org/t/have-a-button-disconnect-its-own-callback/21465
Using a
CallbackRegistrysimplifies processing, fixes memory leaks with weak references, and fixes bugs like being unable to disconnect a callback in its own code, as in the above post.I also cleaned up some conditions and deprecated the old callback-related attributes.
PR Checklist