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 AxesWidget base class to handle event activation and clean up. #724

Merged
merged 8 commits into from
Mar 17, 2012

Conversation

tonysyu
Copy link
Contributor

@tonysyu tonysyu commented Feb 28, 2012

  • Adds connect_event method to connect event and store a reference. Additional disconnect_events method allows user to clean up widget. Some widgets did their own cleanup, but this makes behavior more consistent.
  • Adds default ignore method and call it in event callbacks. Some widgets had their own ignore methods, but this makes behavior more consistent.

Todo:

  • I think SpanSelector.new_axes should be removed, but may be some user's code depends on it. Right now I duplicate most of the new_axes code in __init__: If new_axis isn't removed, then duplication should be removed.
  • update_background in SpanSelector and RectangleSelector don't ignore events when deactivated.

* This change is in preparation for generalizing the base Widget.
* It's not clear to me why `new_axes` is necessary, but I left it in (for now).
* Renamed `Lasso.axes` to `Lasso.ax` for consistency.
* Remove `Lasso.figure`, which was unused.
* Attribute name "active" can be confused with parameter to `RadioButtons`, but `RadioButtons` doesn't save that parameter as an attribute, so there's no name clash. BUT this is still really confusing to users. Consider renaming `AxesWidget.active`.
*
* * This could have been implemented as a decorator, but some callbacks wouldn't be compatible. For example, `SpanSelector.release` should not be ignored if widget was deactivated during mouse press.
*
* * `SpanSelector` and `RectangleSelector` already had `ignore` methods, but they do not call it in `update_background`. I don't change the current behavior, although it seems desirable.

def connect_event(self, event, callback):
self.canvas.mpl_connect(event, callback)
self.cids.append(callback)
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong. The cids are not the callback functions themselves, they are the integers returned by the call to mpl_connect().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I had this correct at some point, but somehow, I broke it along the way.

@WeatherGod
Copy link
Member

Overall, I like the idea, and I do see its value in bringing uniformity to the widgets. This does open the doors to subsequent updates.

@tonysyu tonysyu mentioned this pull request Mar 1, 2012
Class links were incorrect.
@efiring efiring merged commit f169460 into matplotlib:master Mar 17, 2012
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.

3 participants