I think I get what you mean, you see a problem with wanting the possibility of different default_tools for different backends.

I agree that playing around with the default_tools dictionary seems like a bad idea, but as far as I know, other options exist for going around this. For example the backend can remove a tool and then add a different one, and also in MEP27 I effectively redesigned the quit tool to fire an MPL closing event. The whole point of MEP22 and MEP27 were to standardise the backends making things simple. I think as long as we keep to that, we give the individual backends the maximum freedom to do what they want.

Also pushed removal of sender and data kwargs.

@OceanWolf how do you see this?
I tested it and seems to work fine.

Pros:

• I like how it simplifies things.

Cons:

• I don't like the removal of the rubberband as a tool, but I agree that it is not used anywhere else.
• The end user now has to do one extra action and is to register the tool, even if he is adding it manually to the toolmanager.

@anntzer I know it is an anti-pattern but, can we do something to eliminate the need to register the tool when adding it by hand (like in the example)?

I agree always having to register is not super elegant; I'm totally fine with allowing passing a Tool instance to add_tool directly (and store the name of the tool as an attribute of the class, so this way thusly added tools also have a proper signal name, and there's no need to mention the name explicitly when registering the builtin tools).

I'm keeping the inheritance mechanism private for now because I hope to make it work together with the_Backend class inheritance mechanism.

I still think that if we're worried about rubberbands they should become freestanding functions, to me it's very clear they were previously shoehorned into the wrong API.

Pushed the changes suggested above.

why change to canvas instead of backend for tracking?

Now there is another problem, we can not add twice the same tool, this is sometimes useful when passing arguments at initialization (*args, **kwargs of add_tool)

Switching to registering on the canvas class allows one to rely on canvas inheritance rather than having to manually declare the inherited tools.

I think the issue with having the same tool twice was already present before (in the sense that thetool_trigger_$foo event is overloaded). I agree that this should be fixed somehow, but I don't think this PR changes anything?

The name was defined when callingadd_tool so we could give different names to the same tool.

A combination of both could work fine.
What aboutadd_tool(self, name, cls, *args, **args) where name is striclty string, and cls is strictly a class derived from ToolBase.
We could use thetools.register just to fill the dict of available tools for default initialization as you were doing (I like that approach)

And for the user in "manual" mode, we force him to use the name and the cls

I guess a possibility would be to change the signature of ToolBase toTool(toolmanager, name=None, ...) (with name effectively defaulting to the class name), and use tool.name as the key.

What is the advantage of this vs forcing to pass the name?

After a little thought, maybe is not a bad idea.
As long as the default tools, we provide a name that is not the classname I'm fine with that solution

This seems rather ugly to me. I haven't had a look at the code yet, but "name" of a tool exists as a label for identification by theToolManager. By passing a name to the tool, you then give the tool the job to check with the tool manager whether if it can take that name or not, and if not, what then? We now have an object that has failed to set up properly because the given attributes,name andtoolmanager conflict with one another.

I would keepname out ofTool, as it does not belong to theTool, but the relationship between theToolManager and theTool.

Haven't thoughttoo much about this but I think it's fine to use@fariza's proposed signature too.

@OceanWolf right now the tool name is given at the add_tool call.
Before the tool is actually instantiated, it is the toolmanager that decides if the name is valid or not.
The tool keeps a reference to it's name

Just to say, that sometimes tools need to know their identifier

Why? I don't see why a tool should ever need to know what we call it. It makes no difference to the function of a hammer whether it gets called a hammer, or a hitty thing, so long as we know what we call it. The purpose of a tool lies in doing something. That said, if we already have it that way, I don't mind changing it.

Also, I think I come around now to the idea of using the canvas class to store the tools used, but I think the PR here looks way too complicated for such a simple change. Why don't you just movedefault_tools from global scope, into theFigureCanvasBase and then have theinit function of inherited classes operate on the dict? When creating an instance of the FigureCanvas the MLO will happen automatically without you having to explicitly search through the MLO, plus you get way more custom functionality because you essentially implicitly move yourregister_tool method out of global scope and into the__init__ method of canvas.

@OceanWolf right now the name it is only used by the cursor tool, it needs to know the name of the tools to connect to their trigger event, and this is done using their name. But I agree with you (the hammer convinced me)
One solution for this, and remove the name completely from the tool, would be to modify the add_tool_event and pass the name of the added tool (makes sense).
@anntzer do you want to include this change in this PR? if you prefer I can make a smaller one (easier to approve) with just that change

Regarding the filling ofdefault_tools
I prefer the use of the backend for tool identification (seems more natural to me), but I don't see anything against using the canvas.
As I understand@OceanWolf goes towards a solution that implies filling the dict on canvas init method instead of a separated "Register" call near the tool declaration.
This seems simpler but less explicit.
If this is a viable solution, why not the figuremanager instead of the canvas? All the toolmanager/toolbar/statusbar initialization stuff happens in the figuremanager, so the filling of that dict could happen here aswell

May be easier to do all the refactoring in a single PR I think.

I would rather have smaller PRs, it becomes easier to see the logic, to see what gets changed and why. I have been toying with the idea of getting rid of breaking of chunks of the main MEP27 PR, but the pain of the rebasing has put me off so far given how bits of the PR have changed as time has progressed. Maybe I will just bite the bullet over the holidays or something.

And yes, I see why you do it, for tool admin purposes, but that shouldn't require too much fiddling to change. Also as a tool admin thing, I see that as more the responsibility of theToolManager than the tool, so either make SetCursor as the exception and have it better interface withToolManager with name getting passed as an event; and/or move this event connection logic completely to theToolManager.

As for thedefault_tools location, yes, you understand me correctly. The problem with putting it inFigureManager lies with theFigureManager classes getting replaced quite soon (hopefully) with MEP27 with no inheritance.

I dislike having all of these methods and variables around to determine something. It spreads out the code too much to figure out what what has happening, keep code simple to read, if something happens with the Canvas class, I expect to find it in the canvas class in the code. Having tool registration inbackend_tools.py seems wrong to me. This file exists as a compilation of tool classes, not managing them. Any logic outside of what an individual can do tool I think should go elsewhere.

I go with KISS, easier to debug and spot mistakes. Also going back to the logic before about a tool not knowing its name, this will then mess up the register tools function. Going back to a dict of{name: class} will correct this.

I am not convinced thatdefault_tools dictionary has to be moved somewhere else. Insidebackend_tools.py seems just logic to me. But if the implementation makes it easier to read and to debug I'm in!

Regarding the name argument in tool.
We just remove it and modify the tool_added_event, it just makes sense for this event to contain the name of the tool being added, actually for all events, it just makes sense to contain the name of the tool

I don't see any other way for this PR to accomplish want it wants to accomplish once we have moved name, but yes, lets do the name thing in a separate PR and then see what we have left with here.

@anntzer in case you didn't have time, here is a small PR removing the name#9966
If you feel, rebasing is going to give you more problems than reimplementing it over your PR, please feel free to reject it.

No worries, please go ahead.

Although I guess the other question is, do tools really need to have names to start with?
As you mentioned, this is only used to that the cursor tool can register onto trigger events for other tools. But this can be implemented in at least two different ways:

• the individual tools can call themselves canvas.set_cursor or toolbar.set_cursor (or wherever we want to store this method)
• or, there could be a single tool_trigger_event that passes the Tool object as one of its arguments.

We need name to identify the tool, we can have multiple versions of the same tool at the moment, that makes it easy to create several tools from the same class initialised with different parameters.

I am +1 with registering the tool inside the canvas,@OceanWolf can we move ahead with this? No matter what we do, it will never be perfect, put it is pretty good as it is.

Superseded by#20990.