Skip to content

Use CallbackRegistry in Widgets and some related cleanup - #18226

Merged
timhoffm merged 4 commits into
matplotlib:masterfrom
QuLogic:widget-cbs
Aug 18, 2020
Merged

Use CallbackRegistry in Widgets and some related cleanup#18226
timhoffm merged 4 commits into
matplotlib:masterfrom
QuLogic:widget-cbs

Conversation

@QuLogic

@QuLogic QuLogic commented Aug 12, 2020

Copy link
Copy Markdown
Member

PR Summary

This came about from this Discourse post: https://discourse.matplotlib.org/t/have-a-button-disconnect-its-own-callback/21465
Using a CallbackRegistry simplifies 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

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • [n/a] New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • [n/a] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/next_api_changes/* if API changed in a backward-incompatible way

@QuLogic QuLogic added this to the v3.4.0 milestone Aug 12, 2020
@tacaswell
tacaswell requested a review from timhoffm August 12, 2020 02:20
@tacaswell

tacaswell commented Aug 12, 2020

Copy link
Copy Markdown
Member

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).

tacaswell
tacaswell requested changes Aug 12, 2020
View reviewed changes

@tacaswell tacaswell left a comment

Copy link
Copy Markdown
Member

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.

@QuLogic

QuLogic commented Aug 12, 2020

Copy link
Copy Markdown
Member Author

I added an API change note.

@anntzer

anntzer commented Aug 12, 2020

Copy link
Copy Markdown
Contributor

(I'm not going to fight over this, at least being consistent within the library is good too.)

tacaswell
tacaswell reviewed Aug 12, 2020
View reviewed changes
Comment thread doc/api/next_api_changes/behavior/18226-ES.rst Outdated
The `.widgets.Widget` classes that allow event observers now use a
`.CallbackRegistry` to save callbacks. This provides consistency with canvas
event callbacks, and fixes some bugs that are already handled there. It also
means that callbacks are now stored as weak references, which means you must

@tacaswell tacaswell Aug 12, 2020

Copy link
Copy Markdown
Member

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.

tacaswell
tacaswell reviewed Aug 12, 2020
View reviewed changes
Comment thread doc/api/next_api_changes/deprecations/18226-ES.rst
Widget class internals
~~~~~~~~~~~~~~~~~~~~~~

Several `.widgets.Widget` class internals have been privatized and deprecated:

@tacaswell tacaswell Aug 12, 2020

Copy link
Copy Markdown
Member

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?

@QuLogic QuLogic Aug 14, 2020

Copy link
Copy Markdown
Member Author

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.

tacaswell
tacaswell approved these changes Aug 12, 2020
View reviewed changes

@tacaswell tacaswell left a comment

Copy link
Copy Markdown
Member

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.

timhoffm
timhoffm reviewed Aug 13, 2020
View reviewed changes
Comment thread lib/matplotlib/widgets.py Resolved
Comment thread lib/matplotlib/widgets.py
@cbook.deprecated("3.4")
@property
def cnt(self):
# Not real, but close enough.

@timhoffm timhoffm Aug 13, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean?

@QuLogic QuLogic Aug 14, 2020

Copy link
Copy Markdown
Member Author

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().)

Comment thread lib/matplotlib/widgets.py Resolved
Comment thread lib/matplotlib/widgets.py Resolved
Comment thread lib/matplotlib/widgets.py Resolved
Comment thread lib/matplotlib/widgets.py Resolved
Comment thread lib/matplotlib/widgets.py Resolved
Comment thread lib/matplotlib/widgets.py Resolved
Comment thread doc/api/next_api_changes/behavior/18226-ES.rst Resolved
QuLogic added 2 commits August 14, 2020 01:09
@QuLogic
Use CallbackRegistry for all Widget event observers.
b9d9049 
This simplifies processing, fixes memory leaks with weak references, and
fixes bugs like being unable to disconnect a callback in its own code.
@QuLogic
Shorten some conditions in widgets.
ae888ca
@timhoffm
timhoffm merged commit a1159a4 into matplotlib:master Aug 18, 2020
@QuLogic
QuLogic deleted the widget-cbs branch August 18, 2020 20:17
@anntzer anntzer mentioned this pull request Nov 7, 2020
Merged
7 tasks
@anntzer

anntzer commented Nov 7, 2020

Copy link
Copy Markdown
Contributor

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

    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]
    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 for _ in range(2): gcf().canvas.mpl_connect("key_press_event", print) and for _ 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

@anntzer anntzer mentioned this pull request Nov 10, 2020
Merged
7 tasks
@richardsheridan richardsheridan mentioned this pull request May 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell approved these changes
@timhoffm timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

topic: widgets/UI

Projects

None yet

Milestone

v3.4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@QuLogic @tacaswell @anntzer @timhoffm