Navigation indepenent of toolbar. #11
Conversation
a78cfe5 to
2605f99
Compare
|
The one thing that really bothers me, is that we have to manually add tools to navigation and toolbar independently. |
|
I know, I may have a solution for that, but I just wanted to get this off the ground for now. The way it stands at the moment though feels too heavy handed and will get very messy when you want to do very little more. I would much prefer the flexibility of this, then the simplicity of one less line of code... but perhaps, just perhaps we can have both. |
|
The simplicity is not only for the static code that goes into MPL, it is for the user that add tools on the fly. What you are proposing is that the user has to do an additional (not intuitive) And, if we go that way, why do we need a |
|
I know you were speaking about the user code, MPL code I don't think matters so long as it runs smoothly and one can debug and add features to it easily enough. Speaking personally, it took me ages to figure out I had to specify a group. It makes sense to me that if I want to use a tool, I add it to, (hopefully) It took me ages to trace where the decision gets made about whether it goes in the toolbar, and I eventually found it in the backend specific code, the last place I would have thought of looking for it. Then comes the problem of why having more then one GUI tool interface (I count subtoolbars as one toolbar), what if I want tools A,B and C in the toolbar, but E,F,A and B in a right click menu, in that order, it becomes impossible. Also what if I want one tool for whatever reason to appear in the toolbar twice but under different groups, again impossible. Here I can do: toolbar_left.add_tools(toolbar1_tools)
toolbar_right.add_tools(toolbar2_tools)
...
context_menu.add_tools(toolbar1_tools + toolbar2_tools)Where toolbar1_tools and toolbar2_tools may well have tools common to both. But I cannot do this under the current setup, primarily because it does not follow OOP design. It does an all or nothing approach, either a tool exists in all GUI containers or None at all. |
|
OK, I'm almost convinced. |
|
Hehe and yay! I kind of just did that, and a bit more... Confused about the position argument though, no changes in group code, just straining out the |
|
Did I add too much extra code just now? Oh, and I think I have a couple of small fixes to make sure the full flexibility works. |
| for position, tool in enumerate(grouptools): 8000 | ||
| self.add_tool(self.navigation.get_tool(tool), group, position) | ||
|
|
||
| def add_tool(self, tool, group, position): |
There was a problem hiding this comment.
Here the tool can be a string, what if we just one to add one single tool on the fly?
2186529 to
7683b0b
Compare
| for position, tool in enumerate(grouptools): | ||
| self.add_tool(tool, group, position) | ||
|
|
||
| def add_tool(self, tool, group, position): |
There was a problem hiding this comment.
I like that it can be auto-added to navigation, but in that case, put the name as kwarg, when do you see it getting a tool object?
There was a problem hiding this comment.
The only thing with using a kwarg means that we can't auto-add from ToolbarBase.add_tools(). I.e. sometimes pass a name, sometimes not... but I don't mind that if you don't, only a helper method.
There was a problem hiding this comment.
the test inside can make mandatory the kwarg name if the tool is a class
There was a problem hiding this comment.
Hang on, now that gets weird:
add_tool(self, name, group, position=-1, tool_class=None)
There was a problem hiding this comment.
add_tool(self, tool, group, position=-1, name=None)
There was a problem hiding this comment.
tool can be either a string or a class
There was a problem hiding this comment.
Because we pass the name to Navigation.get_tool, so we don't want to give that as the kwarg...
Sometimes, as I did here, I like to use tool, as an abstract word, it could mean name, or it could mean the object, or the class. So now we say no object, we definitely need a name, but we don't necessarily need a tool_class
There was a problem hiding this comment.
A couple of try/except blocks to let navigation give whatever it has with that reference, if not, then it means is a class, and we try again forcing the name kwarg
|
I prefer the signature something like |
|
Okay, I think I have sorted it all out, just one last pesky bug to nail down tomorrow that I have just found (I think in master) that I doubt has ever been exposed, until now... |
|
Phew, nasty bug found and fixed! I think this bug has existed since the creation of the mpl CallbackRegistry... (and I also improved the efficiency of the Garbage Collector at the same time). Now back to tools and this PR. P.S. Do you get messages every time I push commits to this branch? Or only when I write a message? |
|
I get messages when you write a message and when you submit a PR but I don't remember getting anything with the commits |
|
Don't do the bugfix here, make another PR for mpl, and when merged there I will rebase. |
|
Okay, but I think I have to enact some kind of magic to do this as I forked from your branch... |
0040d25 to
1a4ac30
Compare
|
Okay, all fixed up I think. and it even works even without the CallbackRegistry fix (though it doesn't hurt to have it, it just means slightly more work for the CPU and the memory in this case). Now, shall I copy in the OrderedDict recipe into this branch/PR? I thought plain Also do you want me to squash some/all of these commits? |
|
P.S. Can't remove |
1a4ac30 to
5dd6099
Compare
| type given by tool, and use this as the name of the tool. | ||
| """ | ||
| t = self.navigation.get_tool(tool) | ||
| if t is None: |
There was a problem hiding this comment.
Navigation will raise a warning, maybe use a try: block here to prevent that warning
There was a problem hiding this comment.
that doesn't work, though just commited a fix that does work. Also I just learnt that warnings.warn only gives a specific warning once...
|
Almost there, just I want your confirmation that this seems right to you. Isn't it too many different configurations for the same method name (two different methods, but same name) To correct my resume!!
So the name comes first then the tool (string or cls) in toolbar, when the tool object already exist in navigation
When the tool doesn't exist in navigation
|
|
You forgot to add
But before we talk about too many configurations, I think we should look at a resume not built on types, because python doesn't work as a typed language... it uses duck-typing, therefore as a resume, I would write: In Navigation:
In ToolCollection:
This raises the question whether class_like should include an object, with Also, if we try to register and add the tool and name already exists, should we just add that name? Using the same logic as the Finally we can now ask the question if it looks too complicated, i.e., whether we should remove the auto-register logic from |
|
I am just afraid that it is prone to confusion. In conclusion, what do you think of limiting the methods as follow
in toolbar
|
|
Fine with me. For the other question, should a tool instance count as class_like? Or should we leave that up to the user to type As I have already made Then comes the OrderedDict, as I said above, I think On an aside, does something exist for documenting stuff like x_like? Either point to the function that deals with it, i.e. |
|
I prefer if tool instance doesn't count as class_like, so no modification needed for in
For For the docs, are you following https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt I am not sure the correct way to do it, so far, I just use a cls or int in the argument definition |
|
Nice, that answers one of my questions, everywhere I see I had been partially working to that, I mainly have used http://matplotlib.org/devel/coding_guide.html, falling back to http://matplotlib.org/devel/documenting_mpl.html#documenting-matplotlib as it seems out of date... |
|
There we go, I think that looks good now, I especially like the neatness of the dict :). |
|
Did you check that the documentation builds correctly with those strings? |
|
No, I think I followed numpy spec though. How should I check that? |
|
Yep, travis is giving problems, difficult to track if documentation fails |
|
Also, is always a good idea, to see how it does look. |
|
Yes, but how do I do it? Does one command do it all? |
|
yes, in doc |
| toolbar_tools = [['navigation', ['home', 'back', 'forward']], | ||
| ['zoompan', ['zoom', 'pan']], | ||
| ['layout', ['subplots']], | ||
| ['io', ['save']]] | ||
| """Default tools""" |
There was a problem hiding this comment.
This docstring belongs to tools add one for this variable
|
Deleted those comments, realised what went wrong. |
|
Okay, running How do I see if I get warnings/errors from here, i.e. once I have fixed them all? |
|
I don't have much experience with that appart from Travis fails on my last commit. I mean, errors related to your changes |
|
But you just Travis gave problems on this... |
|
The only ugly doc I see comes under |
…dependence Navigation indepenent of toolbar.
|
I agree that the dosctring for variables like that is not pretty. I have been looking for a way to fix it, but so far nothing. |
|
Yay, merged! 👯 But I thought you were going to let me squash those commits first ;). |
|
Ups... I forgot, anyway, I am pretty sure, the PR is going to be squashed before merged in master |
This PR finishes the job of making
NavigationBaseindependent fromToolbarBaseas indicated by matplotlib#3652 (comment)I.e. Navigation should not deal with Toolbar specific data, such as group and position, but only act as a manager of tools. This will only become a further nightmare when context menu's etc arise. Each Toolbar/Menu/other UI should encapsulate this data and allow for custom UI representations of these tools, especially when one wants to create menus on the fly depending on context.