Skip to content

move NavigationToolbar2 canvas assignment to __new__ #5121

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

Closed
wants to merge 1 commit into from
Closed

move NavigationToolbar2 canvas assignment to __new__ #5121

wants to merge 1 commit into from

Conversation

blbradley
Copy link

Should prevent copy-paste of assignment of canvas and canvas.toolbar from __init__ when sublcassing NavigationToolbar2.

@blbradley
Copy link
Author

Tests passed. Still needs review.

@tacaswell
Copy link
Member

What is wrong with just calling super().__init__()? __new__ seems like a bit of overkill for this purpose.

@blbradley
Copy link
Author

There is nothing wrong with that.

In my case, I'm overwriting most of the functionality of the toolbar. Less attributes to look at would be nice if I choose to override __init__. So, I think this is better for people making custom toolbars.

Thanks for your speedy responses!

@blbradley
Copy link
Author

For example, I'm not using the zoom/pan functionality nor the original home/back/forward functionality. Deciding if I should use NavigationToolbar2._views and NavigationToolbar2._positions vs another abstraction was confusing for me. Indeed, I need another abstraction (but will still be using cbook.Stack). So, it looked like a way to not initialize them along with other attributes of NavigationToolbar2 was ideal.

@tacaswell
Copy link
Member

If you are re-implementing that much of the internal details, I would not bother sub-classing. Just implement enough of the public API that it duck-types correctly.

You should be aware of the work that @fariza and @OceanWolf have been doing to re-work the toolbar.

@fariza
Copy link
Member

fariza commented Sep 22, 2015

Check the basic example https://github.com/matplotlib/matplotlib/blob/master/examples/user_interfaces/toolmanager.py Is this the kind of control that you want with the Toolbar?

@blbradley
Copy link
Author

@fariza Maybe. Is this GTK only or does it work with other backends?

@fariza
Copy link
Member

fariza commented Sep 22, 2015

For the moment it works only with GTK3 and TK.
We are waiting for this release to continue working on MEP27 that will simplify things porting the other backends.

@OceanWolf
Copy link
Member

Well, "this" release has already gone out... as a release candidate. The next feature release (probably in April) we will finalise the API and have it working on all of the other backends.

Longer term we will depricate NavigationToolbar2.

@blbradley
Copy link
Author

@tacaswell Very good point. I still believe not having to worry about the canvas and canvas.toolbar assignment if someone chooses to sublcass. However, I won't be hurt if this doesn't get merged.

@fariza I'm using the Notebook backend. That does look nice for controlling toolbar items.

@OceanWolf Thank for the info!

@tacaswell
Copy link
Member

There is also some ongoing work (see #4582) to convert the notebook backend to be a proper IPython widget. This will let you hook into with the rest of the IPython widget tools which may provide another path to getting the (I assume) richer toolbar you want.

@tacaswell
Copy link
Member

I am going to close this as bringing in __new__ is a very big hammer for something that has simpler solutions on a class that we are going to deprecate 'soon'.

@blbradley Thank you for your input and work, please be in touch about the toolbar refactor (the start of which will be in 1.5).

Closing PRs without merging is one of the worst parts about being a maintainer 😞 .

@blbradley
Copy link
Author

@tacaswell I'm sure I'll have to consider the refactor when I update my custom toolbar code. Thanks for all the info!

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.

4 participants