NavigationToolbar2 refactored into NavigationBase and Toolbar2Base

[warrd]

In response to #1829 and #1830, I have replaced the functionality of NavigationToolbar2 into two separate classes, NavigationBase and Toolbar2Base.

All figure interactions previously handled by the toolbar class are now solely handled by NavigationBase, except for anything directly related to pressing buttons on the toolbar. This now allows, for example, keyboard shortcuts to work as expected when no toolbar is present (#1829).

All backends should sub-class both of these objects. At the moment, I have only done the QT4 backend as a proof of concept, so any architectural issues can be dealt with before making changes to the other backends.

In the QT4 backend there is a potential issue in that the NavigationQT class is not an instance of QObject, so I had to emit/listen to status bar events on the canvas object rather than itself (as had been done with NavigationToolbar2QT). Is there a more appropriate object for these events?

If the developers are happy with the implementation, I can start implementing some of the other backends in preparation for the merge..

[pelson]

Please document the navigation argument.

[pelson]

I've looked over the backend_base changes and in general, I think they improve the situation. I'm interested to hear from @efiring & @mdboom about the history of the Toolbar classes - when a replacement Toolbar was last introduced a decision was made to call it Toolbar2 to avoid any confusing migration problems (I assume). Would a similar approach be desirable here (i.e. do we need a Toolbar3)?

In terms of implementing this on all backends, I'd be keen to create a feature branch on matplotlib/matplotlib which @warrd & other contributors can submit PRs against - this would avoid a monolithic PR which this could easily turn into. Once all the work is done, we can then merge the feature branch and then delete it. Does anybody have any objections to that approach?

[efiring]

@pelson, I think your idea of a feature branch is good, so long as it doesn't take too long to flesh it out and merge it back. Overall, I like the idea of this refactoring--separating the specific toolbar interface from the underlying interactive navigation functionality.

I don't know what to say about the Toolbar2, Toolbar3, etc. business. I would rather not see the proliferation of names, and of implementations requiring maintenance, if it can be avoided. This refactoring is not changing the actual toolbar from the user's standpoint. Ideally, the original Toolbar, which is ancient and probably hasn't been used by anyone for years, would be deprecated and go away. I think we might even have agreed to do this some time ago, but I don't remember for sure.

Maybe cleaning all this up is enough of a change to warrant a major version number bump, allowing us to break backwards compatibility for user code that modifies the Toolbar2? Maybe there are some other refactorings and cleanups that could be done at the same time? Such as the major refactoring of axes.py? This is just speculation to raise the question. It is based on the general concern that our development model has led to a sprawling codebase, with features and bugfixes continually being tacked on, but with less attention paid to consolidation, design, and long-term maintainability. As mpl has matured the emphasis has shifted somewhat, and good cleanups have been happening; but it might be time for a more concerted effort in that direction.

[efiring]

@mdboom, what do you think about strategy for this substantial refactoring?

[mdboom]

@efiring: I agree -- I think this PR has grown enough that it probably requires a little more up front design. It seems pretty orthogonal to @WeatherGod's plans for the axes.py refactor, though.

Maybe try a MEP for this? @warrd: I don't want to scare off new developers who have obviously done good work -- would you be interested in drafting a MEP on how to move toolbars forward so we can do this right?

[warrd]

Yep, I can give it a go this weekend.

Is there a particular issue/pr relating to the refactor of axes.py you mention, so I can see how this might be affected?

[mdboom]

I think @WeatherGod has just threatened to do it -- but I can't find the reference. Maybe he can be of some help.

The gist is that axes.py is full of many many plotting methods and it would probably make sense to separate them in to separate files so things were more modular. But as I said above, I think that's somewhat orthogonal to the issue with toolbars and panning/zooming raised here.

[WeatherGod]

Threatened? Heh...

Yes, my envisioned refactor work (which can be a topic for a BOF and/or
Sprint) is completely orthogonal to this work. Don't hold up for my sake.

[mdboom]

@WeatherGod: In case my tone of voice didn't come across in writing, I meant "threatened" in the best sense. I think it's a great plan and would love to see it come to fruition. (I'd do it myself given infinite time...)

[efiring]

@NelleV has started an axes refactor in #1931.

[mdboom]

@NelleV: apologies for forgetting about #1931. I think it looks like a good start. It would be nice to get @WeatherGod's comments on it -- and if you're at SciPy that might be a good time to discuss any barriers to completion on that (if any). Thanks!

[mdboom]

Postponing to 1.4

[pelson]

and if you're at SciPy that might be a good time to discuss any barriers to completion on that (if any).

Or committing a first draft. There is a lot to be said for pair programming. 😉

[NelleV]

I'd love to do some pair programming and get feedback on this during the Scipy sprints as I have no time right now to finish the work.

[pelson]

I'd love to do some pair programming and get feedback on this during the Scipy sprints as I have no time right now to finish the work.

I meant at SciPy. The problem with a big re-factor like the one proposed in #1931 is that it is really hard to review. If there are two "trusted" developers sat together working on the issue, then it might just be easier to allow them to commit without a full review (but with the necessary discussions taking place before hand)

[efiring]

I just noticed that I deprecated the original NavigationToolbar in release 1.2, #1388, so it can go away in 1.4. The warning actually says it will go away in 1.3, but it is not urgent. Or, we could still yank it out in 1.3. The warning is only in the rcsetup.py validation, so none of us noticed it when converting to the new deprecation scheme.

[mdboom]

Ah -- thanks for noticing that. It looks like this one got missed on the pass to change all deprecation warnings to mplDeprecationWarning category, so that deprecation warning hasn't been displayed by default (and it's also why I missed it on my pass through to remove deprecated stuff for 1.3.) My gut tells me there's probably very few people using classic toolbars (the new ones have been around at least as long as I've worked on matplotlib), so they are probably safe to remove, but if we wanted to be cautious, I'd say let's update the deprecation warning now to be a mplDeprectationWarning, and remove it for 1.4.

[fariza]

I did not notice this PR before, it is too far in the list.
Is this PR going forward?

If so, I should modify my PR #2465 to make use of this. (And maybe make my life easier or more complex?)

[tacaswell]

At a minimum this will need to be re-based on master due to the overhaul of the signal/slots (PR #2382 ).

[warrd]

As the original author of the pull request, my advice is that this isn't really ready for merge yet. At the moment the only backend I got working was Qt, and I had planned to start work on some other backends, but life and what have you got in the way...

[fariza]

I wasn't thinking about this, but lets brainstorm a little bit...
I created the class ChildNavigationToolbar, that actually just holds the instance of NavigationToolbar2 and is the toolbar that the manager sees,
This class adds itself as a child to the real toolbar MultiFigureToolbar.
In this moment I instantiate this toolbar only if the FigureManager instantiate the MultiFigureToolbar, but I do not see a reason why not just always instantiate this child toolbar, it could live without a real toolbar, and sill handle the keyboard requests for the events.

And because I am not modifiying any existing base class it may be less messier to integrate than to modify all the backends.

I think this ChildNavigationToolbar needs a rename to something like NavigationStateHolder, or something like that, and get rid of too many 'toolbars'....

[mdboom]

@warrd: Do you fancy doing the rebase and bringing this inline with current master? I'd love to see this included.

EDIT: Just saw your comment above about lack of time. Didn't mean to come across as pushy. I do have enthusiasm for this change, however ;)

[warrd]

Hi. I was just working on rebasing this to get up to speed with latest master branch, but have just noticed #2557, which seems to have a slightly updated API, so should I not bother and wait until the other PR is merged?

[fariza]

I would like to have a consensus here.
Bot PRs overlap. As I see it, it is one or the other but not both.
Personally I have no preference for one way or another. I just need to advance.

Please correct me if I'm wrong

• With this PR
  • There is need to upgrade all the backends at the same time.
  • It has the advantage that is cleaner.
• With my PR Splitting navigation and toolbar #2557
  • There is no rush to upgrade all the backends and we can go as fast as we want.
  • Is not as straight forward
  • It will be possible to deprecate NavigationToolbar2 in favor of Navigation when the backends migration is complete.
  • Includes other things related to toolbar manipulation

From my part, I don't mind redoing my work (toolbar manipulation stuff) based on this PR.

[efiring]

It would be good to get this resolved, and move forward. I could be convinced otherwise, but I lean towards proceeding with this more straightforward approach to the initial refactoring, and getting all the backends done at once. It seems less confusing in the long run. I don't like to see divergence among the backends.

Is there any reason why all distinctions between Toolbar and Toolbar2 can't be obliterated as part of this? Leave the "2" version for a while as an alias?

[fariza]

@efiring do you mean, a feature-branch as @pelson proposed?
Or just one quick shot?

As far as I know there are a couple of related ideas, or at least ideas that touch all the backends.
My point being.
If we are going to touch all of them wouldn't it be easier to include/implement all of those ideas.
These ideas are:

• Separate navigation from toolbar (this PR)
• Reconfigurable toolbar (part in Splitting navigation and toolbar #2557)
• Multi-figure-backends (my PR Tabbed gtk3 figuremanager #2465)
• Pyplot decoupling as @tacaswell was proposing

Sorry to be pushy(intense?) but I really need this to move forward.

[fariza]

@warrd Working on #2624 we see that this split is the next step, but adding a couple of extra issues #2694 #2699
To have a better consensus on how this should be done. I created a MEP.
Please take a look, this will address the split and also add configuration options.
https://github.com/matplotlib/matplotlib/wiki/Mep22

Sorry if I sound impositive, this is just a suggestion, it may be that the best way to do it, is to merge this PR and then move to other things

[pelson]

Personally, I don't think we should be targeting v1.4.x with this refactor - it is sufficiently large that I'd like to get it onto master once we've cut the v1.4.x branch to get testing from as many people as possible before releasing it.

@mdboom?

[tacaswell]

I agree this should not be targeted at 1.4

[mdboom]

I agree about targetting for post-1.4.

[efiring]

Has this been superseded by #3652? Can it be closed now?

[tacaswell]

Closing this as I think it is fully replaced by #3652, it has been dead for over a year, and needs a rebase. Ping to have it re-opened if you disagree.