-
-
Notifications
You must be signed in to change notification settings - Fork 7.8k
NavigationToolbar2 refactored into NavigationBase and Toolbar2Base #1849
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
Conversation
|
||
*event* | ||
a :class:`KeyEvent` instance | ||
*canvas* | ||
a :class:`FigureCanvasBase` instance | ||
*toolbar* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document the navigation argument.
I've looked over the In terms of implementing this on all backends, I'd be keen to create a feature branch on |
@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. |
@mdboom, what do you think about strategy for this substantial refactoring? |
@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 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? |
Yep, I can give it a go this weekend. Is there a particular issue/pr relating to the refactor of |
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 |
Threatened? Heh... Yes, my envisioned refactor work (which can be a topic for a BOF and/or |
@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...) |
@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! |
Postponing to 1.4 |
Or committing a first draft. There is a lot to be said for pair programming. 😉 |
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) |
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. |
Ah -- thanks for noticing that. It looks like this one got missed on the pass to change all deprecation warnings to |
I did not notice this PR before, it is too far in the list. If so, I should modify my PR #2465 to make use of this. (And maybe make my life easier or more complex?) |
At a minimum this will need to be re-based on master due to the overhaul of the signal/slots (PR #2382 ). |
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... |
I wasn't thinking about this, but lets brainstorm a little bit... 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'.... |
@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 ;) |
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? |
I would like to have a consensus here. Please correct me if I'm wrong
From my part, I don't mind redoing my work (toolbar manipulation stuff) based on this PR. |
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? |
@efiring do you mean, a feature-branch as @pelson proposed? As far as I know there are a couple of related ideas, or at least ideas that touch all the backends.
Sorry to be pushy(intense?) but I really need this to move forward. |
@warrd Working on #2624 we see that this split is the next step, but adding a couple of extra issues #2694 #2699 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 |
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. |
I agree this should not be targeted at 1.4 |
I agree about targetting for post-1.4. |
Has this been superseded by #3652? Can it be closed now? |
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. |
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..