MEP22 Navigation toolbar coexistence TODELETE

[fariza]

Edit
This PR has been dropped in favor of #3652

This version of the MEP22 implementation https://github.com/matplotlib/matplotlib/wiki/Mep22#implementation

The proposed solution is to take the actions out of the Toolbar and the shortcuts out of the Canvas. This actions and shortcuts will be in the form of Tools.

A new class Navigation is the bridge between the events from the Canvas and Toolbar and redirect them to the appropriate Tool.

[tacaswell]

I think it would be better to split this into two functions, one which returns a a list of what is installed, and one function that returns self._toggled. It might also be better to do the later as a property.

The use case for this check is to be able to easily check if one of the tools is toggled to disable functions in user written functions so it should be as streamlined as possible, something like

@property
def active_toggle(self):
    return self._toggled

if tool_bar.active_toggle is None:
    # do stuff

[fariza]

I agree, it is the intended use.
At the same time, I hope people will use the Tools clases, so they create their own tools, that takes care of the currently toggled conflict.

[WeatherGod]

let's leave the jokes to comedians

[WeatherGod]

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.

[fariza]

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

If you check #2511 there was the idea of moving the views and positions stacks out of Navigation and into the axes. This could be done.

[WeatherGod]

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.

[tacaswell]

white space issue?

[tacaswell]

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

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

[fariza]

@tacaswell do you mean all the classes that are subclasses of ToolBase and ToolToggleBase?

[tacaswell]

Yes it seems overkill to me.

[fariza]

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

[fariza]

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)

[tacaswell]

tool_db = {'Quit': {'description': 'Quit the Figure', 'call_back': some_function, 
                      keymap=rcParams['kemap.quit']}
              , 'Home':...}

class ToolBase(object):
    def __init__(self, name, description, call_back, keymap):
        self.name = name
        self._call_back = call_back
        self.description = description
        self.set_up_keymap_callbacks(keymap)

    def trigger(self, event):
        self._call_back(event)

    # the rest of this class

quit_tool = ToolBase('Quit', tool_db['Quit'])

The intoolbar 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]

[fariza]

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

I agree with the intoolbar and I am trying to correct that.

Respect to the dictionnary, I just find it prone to confusion. And I don't see clearly how to use that approach with ToolToggleBase sub-classes

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

[tacaswell]

Why is this here as a class-level attribute?

[fariza]

Yes, you are right, I forgot that one. _toggled belongs to the object

[tacaswell]

Sorry, the singleton is in class 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 named ToolToggle* which are not sub-classes of ToolToggleBase.

[fariza]

The class ViewsPositions(object) is to store the history (views and positions) of the canvas/axes.
This history needs to be shared by several tools (home, back, forward, zoom, pan).

The simplest of things, would be to move this history to the corresponding Figure or even better Axes, but in the pass you mentioned that it wasn't a good idea.

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

[fariza]

@tacaswell
I am working on an alternative, where the Navigation 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...

[OceanWolf]

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.

[fariza]

@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 like http://code.activestate.com/recipes/576693/) I used a messy list of list of list of......

[OceanWolf]

@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.

[fariza]

@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 by add_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

[fariza]

@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 way toolbar is forced to communicate to navigation, and vice-versa.

In this PR #2759

• Toolbar is strongly tied to Navigation
• The tools are only triggered through Navigation

In the second PR #3652

• Navigation implements a set of events (similar to Canvas) and uses those events to communicate with the Toolbar
• The tools are only triggered through Navigation

In the third PR #3663

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

[fariza]

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

[tacaswell]

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