I only done a cursory look-through, but it is interesting work. One particular issue I have always had is that mplot3d's semantics for zooming and panning is completely hacked (and also for polar graphs, too). This navigation stuff seems to further ingrain some of the same set of assumptions for 2d rectilinear plots that has hurt mplot3d and polar plots. Do you see any sort of way to address that? Could it be that when an axes object is created, that it can supply some buttons that are relevant to it? Or maybe have a "focused axes" metaphor and the navigation toolbar displays a toolbar relevant to the focused axes?

Other notes: PEP8 issue (the triple quotes in docstrings go on a line by themselves, and need a blank line at the end of the docustring). I am sure there are plenty of other things, but this is just a cursory overview.

@WeatherGod If I understood correctly byassumptions for 2d reclilinear plots you mean the stuff related with history buttons. Or is anything else that you consider2d

If you check#2511 there was the idea of moving theviews andpositions stacks out ofNavigation and into theaxes. This could be done.

Essentially, there is a lot of existing code that assume two things: 1)
that there are only two axes (x and y). and 2) that zooming and panning for
one axes means the same for another arbitrary axes. If you haven't tried
doing so, try doing zoom/pan for 3d plots, or even for a simple polar plot,
or maybe one of the map projection plots. It isn't the same for these plots
as it is for a regular plot (and nor should it be). Unfortunately, though,
with the existing design, we have to hammer a square peg (pan/zoom sematics
for a non-typical plot type) into a round hole (pan/zoom mechanisms for
regular 2D plots). I was hoping to see some sort of elegant solution to
this problem, or at the least, to not compound the existing problem by
furthering these assumptions (which we do see multiple times with explicit
calls to x and y axis and assuming 2D view limits, etc).

Another PEP8 issue, have a space between "#" and the rest of your comment.

@fariza Can you remind me again why there needs to be a sub-class for every tool and not a singleTool class which gets initialized via a dictionary?

I am not a fan of havinganother singleton keeping track of all of the figures, it would probably be good to merge that in withGcf's registry of figures.

@tacaswell do you mean all the classes that are subclasses ofToolBase andToolToggleBase?

Yes it seems overkill to me.

What do you mean via dictionary?
Where would be the actual implementation of the tools?
How a user would add a tool?

My main goal (in the structure of things), was to make it as easy as possible for the user to create his own tools and add them at runtime.

For example, a very simple tool that the user can implement

class HiTool(ToolBase):    # keyboard shortcut    keymap = 'h'    description = 'Say Hi'    def trigger(self, event):        print('Hi')fig.canvas.manager.navigation.add_tool('Hi', HiTool)

tool_db= {'Quit': {'description':'Quit the Figure','call_back':some_function,keymap=rcParams['kemap.quit']}              ,'Home':...}classToolBase(object):def__init__(self,name,description,call_back,keymap):self.name=nameself._call_back=call_backself.description=descriptionself.set_up_keymap_callbacks(keymap)deftrigger(self,event):self._call_back(event)# the rest of this classquit_tool=ToolBase('Quit',tool_db['Quit'])

Theintoolbar should be kept some place else, such as a list of names of tools that are in the toolbar.

[edited to clean up code a bit]

@tacaswell
Who is the singleton that you made reference to?
Tool objects belongs toNavigation object that in turn belongs to theFigureManager object.
So every time a newFigureManager is created,Navigation and itsTools are created as well, not reused

I agree with theintoolbar and I am trying to correct that.

Respect to thedictionnary, I just find it prone to confusion. And I don't see clearly how to use that approach withToolToggleBase sub-classes

Who do you propose to run the initialization (matching between dictionnary and base class)ToolBase('Quit', tool_db['Quit'])?Navigation?

Sorry, the singleton is inclass ViewsPositions(object): where there is a class-level dictionary keyed on figures.

For the toggle tools I think the same pattern should work but I think a class needs to be injected instead of a function (to deal with the fact that many of the toggleable tools have buckets of state that go with them).

It is also very confusing that you have tools namedToolToggle* which are not sub-classes ofToolToggleBase.

Theclass ViewsPositions(object) is to store thehistory (views andpositions) of the canvas/axes.
Thishistory needs to be shared by several tools (home, back, forward, zoom, pan).

The simplest of things, would be to move thishistory to the correspondingFigure or even betterAxes, but in the pass you mentioned that it wasn't a good idea.

Regarding theToolToggle name I agree, it was just a... lapsus? I will correct that

@tacaswell
I am working on an alternative, where theNavigation emits events, to communicate changes in the tools (add, remove) and inform of trigger events.

In this "alternative" the toolbar just listens to the navigation events to set itself, so navigation doesn't have to be aware of the toolbar existence.

What do you think?
I don't want to delay this PR more than necessary...

Hey guys, sorry for going AWOL for so long, now I finally have a fully working computer again, but still very busy.

@fariza What you suggest re events sounds quite similar to what I did to remove from_toolbar, apologies that I never got to rebase the large commit into the three smaller ones. I may well find some time at the weekend if you would like me to copy the code over for this.

Again with getting rid of the intoolbar, I have it planned out and would have implemented it following the other commit (stupid computer dying on me). I just need to get two items with a deadline of tomorrow, then I should find some time.

Oh, and I say definitly stick with classes, I find them much easier to read, comment, debug and extend compared with dictionary objects using callbacks, which often leads to nasty hacking later.

@tacaswell@OceanWolf The version with the events is in PR#3652

If we can decide for a route to take, I will discard the other PR, and make the changes that we talked about before (except for the dictionary thing that still in discussion)

One thing that really bothers me, and I would appreciate some ideas, is how to organize the tools, so far, in the new PR, I added the notion of "group", but due to the lack of OrderedDict in python2.6 (and I don't want to carry more stuff likehttp://code.activestate.com/recipes/576693/) I used a messy list of list of list of......

@fariza Ahh, okay, then I shall compare your implementation to mine over the weekend and give comments.

With groups, I gave one method of implementation above, see#2759 (comment). This solves the OrderedDict issue by using a tuple, though one could also use a list, but this sounds rather like your list of list of list implementation. I don't like the idea of using a Tool_Group class as this seems overkill, and toolbar specfic.

I have a third idea for groups that I think goes in the direction we want to head in, but I shall hold of until I look at the code, otherwise it may make no sense.

@OceanWolf the comment that you refer to, translates into a list of list of lists, just spelling each internal list appart, and then gluing them together into a single list.

But this is not a big problem, this list or dict or tuple or magical arrangement is only used byadd_tools, so pretty easy to modify.

For the moment I am using the group just to know if it goes in the toolbar or not.
But I like the idea of having a way to address specific place in the toolbar.
By the way, the only backend that works is GTK3

@OceanWolf@tacaswell@WeatherGod
I hate to do this, but just to add more to the confusion, I just added another PR#3663

I will be available for a hangout, chat if you want to review things together (faster)

Why 3 different PR's? I don't like the waytoolbar is forced to communicate tonavigation, and vice-versa.

In this PR#2759

• Toolbar is strongly tied toNavigation
• The tools are only triggered throughNavigation

In the second PR#3652

• Navigation implements a set of events (similar toCanvas) and uses those events to communicate with theToolbar
• Thetools are only triggered throughNavigation

In the third PR#3663

• Navigation,Toolbar andTools use the central dispatcher to communicate between them
• Toolbar or any other client can trigger thetools without passing throughnavigation (although it will be informed by apre-trigger signal)
• With this approach, is simpler to move some annoying things out ofnavigation, these includeset_cursor,set_message,cursor position and possibly (I didn't have time yet) therubberband stuff. These things, don't belong to navigation.
• This PR uses a central signal handler (pydispatch) but can be replaced for something else.

@mdboom If you can comment on your preference between the three approaches would be greatly appreciated#2759 (comment)

Closed this PR, but it should stay around so we don't lose the conversation.