Moved tooltriggerevents into the tool

[OceanWolf]

With this PR, we let the tool tell us about its state changes, i.e. it's triggering. This gets rid of a unique event type dedicated to each tool, as we now register directly with the tool.

This PR also fixes a bug in the remove_tool function, when a currently toggled tool gets removed from Navigation.

[fariza]

I like the idea but I am not sure in practice will be that fun to use.

If I am a user (not developping the backend), and I want to listen to a specific tool, I have to get the object (tool instance) from navigation and then I will be able to connect to it.
It is not as direct as connecting to navigation with the name of the tool.

[OceanWolf]

Obviously I can't say how users will use it, but I would have thought that at least in the vast majority of cases, the user would already have the tool class to hand, as we do in both the cases where we use it here.

[fariza]

That is if you are adding the tool, in that case it is handled automatically and in practice it doesn't matter if the event comes from navigation or the tool.
My point is when you want to listen to an existing tool for something else.
But again I'm not sure, I'm brainstorming to see the pros/cons of each approach

[OceanWolf]

Well yes, the other case where it gets used SetCursorBase, where we have to type lots extra, self.navigation. ... _%s' % tool.name.

From my own perspective as a user, I would fear typing the tool name incorrectly... and slow me down in the debugging time as I search for why something doesn't happen.

I could add a helper method to navigation, that gets the tool and sets up the callback... that should eliminate all possible negatives, and leave all the positives, right?

[fariza]

why is the event fired here and not from inside the tool trigger method?

[OceanWolf]

I would love to, as that will bring it directly to the point it gets fired, however it makes the code more cumbersome to write, with every tool needing a call to super...

def trigger(self, sender, event, data=None):
    ToolBase.trigger(self. sender, event, data)

In my old code base (from the summer before my computer died), I did it like this:

def _trigger(*args, **kwargs):
    self.trigger(*args, **kwargs)
    tool.state_changed_event(...)

But I didn't like that because it makes trigger a private method...

I guess I could do it the other way around, reversing those methods, make the developer override _trigger which executes the tool specific code, and have trigger() as the entry to the tool which ensures that the event gets fired.

[fariza]

@OceanWolf the more I think about this...
Again, as in one of my comments, it is odd, that you listen to tools for events, but to fire them you use navigation.

[OceanWolf]

Okay, I have rebased, and it looks good to me :).

I think that oddness has disappeared with the function renaming and simplification of functions. In terms of adding a convenience method to NavigationBase, I don't think it really saves much in typing, just:

self.get_tool(tool).tool_connect(s, func)
self.add_trigger_cbk(tool, s, func)

where self would obviously get replaced by fig.canvas.navigation or whatever, and s replaced by tool_triggered, but of course it makes no difference in comparing code length, but same in both cases, unless I put that in the convenience method. What do you think?

[fariza]

@OceanWolf for the rebase I was talking about #4

This one, I'm not convinced yet, it makes the tools more complex

[OceanWolf]

I know you meant #4, but I wanted to get this one updated, I didn't like having my thoughts spread across PRs (and it gave me a lot of practice with rebase).

While I agree it makes the tools slightly more complex, I think it just represents the complexity of the tool, it encapsulates what a tool can do by itself, and thus IMO reduces the complexity of the entire system.

[fariza]

I am not convinced yet.
Maybe I'm wrong but I see it as a kind of MVC where Model=Tools, View=Toolbar, Controller=Navigation of course it is not exactly that but you get my point.
In that sense if I understand correctly there is no reason to make the model inform the rest of the world of changes, it is the controller responsibility.

[OceanWolf]

Hmm, I sort of see it like that, though I prefer the thin controller paradigm for MVC where the controller just acts as an interface between view and model (if needed) and all the action happens in the view and the model. By moving stuff to where it actually gets done, it allows for more reusable code, cleaner code, and thus less chance for mistakes.

And when I said "sort of" above, I meant that the Navigation behaves like quite the fat controller, doing a lot of model work, in storing the tools and micro-managing the way tools should interact with each other. As for Toolbar, well, I shall leave that for my next PR ;) (my last structural PR, I think). Anyway, I shall leave it as this and come back after that next PR.