Skip to content

refactor non wx.ToolBar specific parts of NavigationToolbar2Wx/Agg into NavigationController2Wx/Agg #10145

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

refactor non wx.ToolBar specific parts of NavigationToolbar2Wx/Agg into NavigationController2Wx/Agg #10145

wants to merge 2 commits into from

Conversation

DietmarSchwertberger
Copy link
Contributor

plus bug fix: add missing 'import six'

PR Summary

When embedding a FigureCanvasWx or FigureCanvasWxAgg into a larger
application, the functionality of NavigationToolbar2Wx/Agg is quite useful,
but actually having to integrate the ToolBar is not too nice.

With this pull request you still can use the functionality (pan, zoom, save_figure etc.) now
as the non-ToolBar specific code is refactored into NavigationController2Wx/Agg.

I'm currently using this to prepare a more comprehensive example of a
matplotlib application to be supplied with wxGlade.

PR Checklist

I don't think that NavigationToolbar2Wx is documented anywhere except
in the examples. If anyone wants to use NavigationController2Wx/Agg, he/she
probably needs to look into the sources anyway.

If the pull request is accepted, I could prepare an example with
standard wx Buttons instead of the toolbar.

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

@DietmarSchwertberger
refactor non wx.ToolBar specific parts of NavigationToolbar2Wx and Wx…
d4f73c1 
…Agg into

separate classes NavigationController2Wx/Agg to be used separately

bug fix: add missing 'import six'
@anntzer
Copy link
Contributor

anntzer commented Jan 2, 2018

If your goal is to use the zoom/pan ability, you may be better served by the (poorly documented...) toolmanager API.

@tacaswell tacaswell added this to the v2.2 milestone Jan 2, 2018
@tacaswell
Copy link
Member

I don't think the new toolmanager API has been implemented for Wx yet.

This is a bit orthogonal to that work, but this is self-contained to the Wx backend and is backward compatible. I am 👍 on this going in as-is.

@tacaswell
Copy link
Member

@DietmarSchwertberger Thank you for your work on this.

@DietmarSchwertberger
Copy link
Contributor Author

I was not aware of that. A cleaner and more straightforward approach with separation of functionality and GUI elements would be nice.
I doubt that this exists anywhere yet for the wx backends.
E.g. a draw_rubberband method is only in the NavigationToolbar or now in the NavigationController.

The only documentation I found right now is stating that it's experimental and only in gtk3 and tk.
If one of these implementations is production-ready, then it might be possible to restructure the wx code in the same way. That seems doable, but my testing capabilities are limited...

For my wxGlade examples, I need to ship the restructured version anyway as I can't rely on anyone upgrading matplotlib immediately.

@anntzer
Copy link
Contributor

anntzer commented Jan 2, 2018

fwiw I would much prefer updating the toolmanager API for wx.

@DietmarSchwertberger
Copy link
Contributor Author

I had a look at the example and tried it with Tk instead of GTK as I'm using Windows.
The toolmanager API looks promising, but I think it is out of scope for me to implement this now.
Also, I don't want to take the ToolContainer approach. I just want to have the API for integration, not the toolbar or the status bar with their GUI elements.
It's not clear to me whether this would be possible at all. With the NavigationToolbar it was only possible after splitting as in the pull request. Even if I implement a ToolContainer without the GUI frontend, then I'm not better off than before.

tacaswell
tacaswell approved these changes Jan 3, 2018
View reviewed changes
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

👍 to change in theory.

Have not tested locally (and we have no CI on this). Strongly prefer second reviewer check this locally.

@DietmarSchwertberger DietmarSchwertberger mentioned this pull request Jan 3, 2018
Closed
6 tasks
@fariza
Copy link
Member

fariza commented Jan 4, 2018

@DietmarSchwertberger Using the toolmanager you can achieve what you want, In your lager application you won't use the toolbar, but just the toolmanager that gives you access to all the tools without the need for the gui part.

I agree that we are lacking documentation for the toolmanager, but have you checked https://matplotlib.org/api/backend_managers_api.html ?

I would really prefer this to go in the direction of the ToolManager, if we all push in the same direction we can move forward.
There is a PR open for QT implementation #9934 maybe it can help you to see what needs to be done

@DietmarSchwertberger
Copy link
Contributor Author

Would a PR be accepted that just implements these, i.e. without an actual tool or status bar implementation?

SaveFigureWx
SetCursorWx
RubberbandWx
...
backend_tools.ToolSaveFigure = SaveFigureWx
#backend_tools.ToolConfigureSubplots = ConfigureSubplotsQt
backend_tools.ToolSetCursor = SetCursorWx
backend_tools.ToolRubberband = RubberbandWx

@DietmarSchwertberger
Copy link
Contributor Author

Things seem to work fine with the tools copied from Qt and Tk implementations.
After some more testing, I will upload a pull request with the classes mentioned above.
So this PR can be closed.

@DietmarSchwertberger DietmarSchwertberger mentioned this pull request Jan 9, 2018
Merged
6 tasks
@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018
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

Assignees
No one assigned
Labels
GUI: wx
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DietmarSchwertberger @anntzer @tacaswell @fariza @QuLogic