Enabling tool positions #7
Conversation
| The name of the group. | ||
| position : int | ||
| The position within the group, if -1, it goes at the end. | ||
| """ |
There was a problem hiding this comment.
Note to self, need to add some more documentation here, i.e. say it returns the position that this should get inserted into the toolbar.
|
@OceanWolf I am a little bit confused, as I see, you are computing the per group position inside the base clases. But there is no actually "grouping" in the implementation (GTK3). What am I missing? The main idea of the groups, was to be able to "split" the buttons between different "toolbars" or "menus" when implementing a custom backend. |
|
Ahh, okay, I presumed that the groups were just to make it easier to add a tool to a specific position and to do mass delete of groups. As I understand it, you want a backend specific way to handle the grouping... do all backends support this? If not, then I guess I will leave these new methods in to get called by the specific backend if they need a fallback method... I will investigate it later this week. |
|
Until now, the only thing backend supported was the "spacer" and you added the tools in order. |
|
I am not sure on how to do this, it is just an idea |
…r a specific backend, while keeping default.
|
I don't know if one can do it on GTK, (I had a look but couldn't see any) but I have re-written to make it possible to override the default methods to do a backend specific implementation. |
|
There we go, I have just reimplemented the seperators to illustrate how the backend can now decide how it wants to implement groups. |
|
@OceanWolf Check my approach for this https://github.com/fariza/matplotlib/compare/fariza:navigation-by-events...tool-positions?expand=1 I am not saying mine is ok and yours is wrong. I just want to compare two different understandings of "grouping" In my view, group is completely up to the backend to implement it. |
|
I don't know enough GTK to see what goes on, but I assume it works and I want to try. Not exactly sure what I look at though, if I read it write it says it compares this (my) branch with P.S. Just doing my final checks on my latest branch before I PR :). |
|
My approach was pretty simple, I create a different toolbar for each group, and add the tools to the corresponding toolbar in it's given position (relative position). |
|
make sure the comparison is between my two branches, not yours, because your |
|
Yes, I agree with respect to the default backend, that's what I tried to do here, create a default implementation that allows the custom backends the flexability to override the default behaviour and do it in their own way. For example in ToolbarBase, I added the property As I say, I want to grab the code, run, and perhaps merge together into mine. I understand the comparison exists between our two branches, but I don't see any of your code here https://github.com/matplotlib/matplotlib/pull/3652/files I presume this contains the most up-to-date version, what do I miss? |
|
My code is not merged, is in a separate branch https://github.com/fariza/matplotlib/tree/tool-positions |
|
by the way you can not use an ordereddict it was added in python 2.7 and mpl is officially supported from 2.5 (if I remember correctly). |
|
Ahh yes, now I see, when you said So yes, I think our two approaches seem similar enough, practically identical. the only real difference lies in that your advance knowledge of GTK, i.e. knowing you can add multiple toolbars to a box, and my creation of a more uniform way to do it. But apart from that the code looks identical, the if statements, the variables used, they look exactly the same. If I understand the benefit(s) in GTK for using toolbars in a box, it means that each individual toolbar will get those down_arrows when the toolbar_group gets full. I had tried to search for that, but I only got ToolItemGroup for ToolPalette, not quite what we wanted... Shall I copy-paste your Gtk solution into mine, or attempt a merge? Variables different everywhere... so it may drive me crazy. |
|
And yes, now I see the dependencies on the install page, annoying, it doesn't mention 2.5, only 2.6, so only one version out. I wonder when 2.6 support will get get dropped, given that python officially retired it over a year ago... |
|
The two approaches are not really compatible, in my case, the position in Sometime ago, I asked about the 2.6 vs 2.7 support and came back empty, so I wouldn't count on that, also I don't want to pollute the code with custom implementations on ordereddict |
|
Ahh, I can see why you think that, and I did do it in my first commit, but since then I have deliberately made it more flexible then that after your first comment :). Trust me! I will go and make the changes and push it. Virtually the same :). Did you ask about 2.6 in relation to mpl1.5? Debian currently has 2.6.6 on oldstable, I guess mpl will wait for these distros which contain python 2.6 to disappear, for debian the Long Term Support continues until February 2016. |
9a6ae52 to
b15d600
Compare
|
Okay, if the mountain won't come to Muhammad... Also added in the multi-toolbar-area, and as you see, it required no changes to ToolbarBase :) (apart from an related minor change which I will get rid of in my next PR anyway, to come tomorrow after sleep), the ToolbarBase methods I made in this PR work flexibly enough to add the tool on the way into or on the way out of |
|
I don't know how much the mpl developer will appreciate a soon to be deprecated class in cbook. I completely understand, but still they may not like it. |
|
Personally I would rather start of doing it the right way, especially with something so easy to take out as this (I don't think it needs deprecation, as you just delete all references to it when once mpl no longer supports python2.6); then do it the wrong way with it embeded into a lot of code and thus will end up there for donkeys years. If the developers don't mind this then yes, I shall alter I don't mind adding more of OrderedDict's methods to this one. I have coded as I have gone, and tested on a server running python2.5 (I meant to upgrade it ages ago, but I haven't got around to it). Who should we ask about this? Where do we go from here? |
|
Okay, I have just finished implementing a few more OD methods, and raised NotImplementedError's on the remaining ones, the ones that I really don't think anyone will use before 2.6 gets ditched, well methods that I have never used anyhow. Shall I squash into the appropriate commit above and force push? |
b15d600 to
b6b3dfa
Compare
30681cd to
e0f153e
Compare
| tool, group, position = event.tool, event.data['group'], \ | ||
| event.data['position'] | ||
| if group is None: | ||
| return |
There was a problem hiding this comment.
This should be done by backend, maybe you want to create a separate menu with all the tools which group is None
There was a problem hiding this comment.
yes you are right, I did change the in_toolbar attribute for this and make sense. But again, the toolbar should be informed of all tools and it can decide what to include or not. The group=None is just a suggestion (implemented in the default backends)
There was a problem hiding this comment.
I agree with what you want to achieve, but I really disagree with implementing it like this. My next PR, the one that I have been talking about, will, in my opinion give a better implementation of this.
Perhaps I should push ahead and just release my next PR as a w-i-p so that you can see for yourself.
e0f153e to
043d32f
Compare
|
Okay, copied over the OrderedDict code. Anything else left to do here, bearing in mind I will remove the group == None bit in the other PR. |
|
I don't understand, I thought this one was dropped in favor of #11 |
|
I thought these PRs were doing two separate things... #11 allows for multiple instances of
|
|
I agree that the name is not really appropiate, I like I prefer if we go for refining #11 and for the moment we put the group/position calculations in the backend code. |
|
Okay, though I came back to this now because I really need some of the structure from here to finish #11, namely I really like how And yes, I agree, no need for subclasses at the moment, I believe that that goes beyond the objectives of this MEP, I think we just want to ensure the easy creation of user-defined-tools. Adding features not currently present to toolbars and menus which don't exist at all, I think goes out of scope, just so long as our code makes it easy for these features to get added later. |
|
I think, I am going to go for a simpler implementation in the backend, I don't think we need to add any more methods to the base classes for this. |
|
Feel free to go ahead with that, atm I have my head on the backend refactor so we can then get Just waiting/working on the last little bits to get Travis to pass (which has brought to light other bugs), and then I can turn my attention to refactoring each backend in separate branches, modifying matplotlib#4143 where needed. |
|
@OceanWolf I was checking your refactor. |
As far as I can see, I have it working...
I found it confusing seeing in some places, end of group meant passing
positionas -1 and other places as None, so I went for -1 for consistency (and it allows one to set it as -2 to place it in the penultimate position in the group).