toolmanager and tools dont hold figure

[fariza]

To avoid possible problems with somebody replacing the figure inside the tools or the toolmanager.
I replaced the attributes for properties in the tools

[fariza]

@OceanWolf I think this belongs here, just to make MEP27 consistent

[OceanWolf]

@fariza, if I remember correctly we had a discussion about this already on the main branch of MEP22. I think you originally passed the figmgr to toolmgr but changed it to canvas because people can run a toolmgr without a figmgr when it comes to embedding...

The lines you change here I was just about to change myself. I just need to split my code up into commits and then I will upload it and you can tell me what you think.

[fariza]

If I remember correctly I was passing both canvas and FigureManager.
But the more I see it and use it, it makes more sense just to pass
figmanager.

Later when we make the figure changeable figmanager will emit an event and
the tools that need it will catch it.
On 26 Jun 2015 8:20 pm, "OceanWolf" [email protected] wrote:

@fariza https://github.com/fariza, if I remember correctly we had a
discussion about this already on the main branch of MEP22. I think you
originally passed the figmgr to toolmgr but changed it to canvas because
people can run a toolmgr without a figmgr when it comes to embedding...

The lines you change here I was just about to change myself. I just need
to split my code up into commits and then I will upload it and you can tell
me what you think.

—
Reply to this email directly or view it on GitHub
#8 (comment).

[OceanWolf]

What about multifigure manager? It would make sense to me to change the toolmanager to relate to a different figure. Especially if they use a separate window for a toolbar like in your gui_elements example.

[fariza]

What I was thinking, and I really think is the best way to do it, is to emit an event from FigureManager when the figure gets setted.
The tools that need to refresh information based on the figure will hook into that event, and store the figures in a local array or dictionary.
Having a reference to figure here makes me nervous.

[fariza]

Also, right now is not used anywhere, and leaving it there might just give ideas and then it would be impossible to change

I know it was me, but again, the more I see it the more I like the new way of not having local references to figure

[OceanWolf]

I still don't feel convinced about this, I see FigureManager, while a lot more flexible then before, it still functions very much as a neat and easy wrapper, as soon as you try to do anything complicated, like embedding, it falls apart and thus we shouldn't build MEP22 around it.

I would much rather emit an event from ToolManager when the figure changes and/or don't let the tool keep the figure... ooh, actually, scrap that last bit, we can build in a figure updater callback into ToolBase and descendent classes can override that updater method if they need to do anything extra.

If we go with that then I don't think we need to implement that now as we purely add a new feature, we don't change any existing structure, so it will remain fully BC.

[fariza]

That last bit of having a callback in the tools, is more or less what I was thinking. But I really prefer not to have a permanent reference to it and even less a setter.

[OceanWolf]

Ahh yes, I understand, sorry, getting confused as to where bits of code lie. Yes, I agree we shouldn't have this setter. For some reason I thought we had this bit of code in ToolManager.

Yes, I completely agree we want to convert this setter in ToolBase into just a callback that sets the figure.

Going back, I think I got confused because I saw it completely logical to have a figure setter in ToolManager and instead of passing in a Canvas or a FigureManager into ToolManager we just pass in a figure.

[OceanWolf]

Not sure how much MEP22 modifications I want to go into the MEP27 branch, I think we should keep it to a bare minimum of changes where:
a) overlap exists (i.e. the method calls between the two), and
b) API changes for interaction with the backends.

I.e. Just the things we consider we need to get right before we can start converting the other backends, because I see this main PR as the slowest link in the chain.

[OceanWolf]

To make it clear I don't mind making the change from ToolManager(self.figure.canvas) to ToolManager(self.figure) here. I think that makes more sense and exists in the overlap of MEP27 and MEP22. Then again, perhaps easier to do it all in one go in a future PR, especially as this won't affect the backends... but your call on that.

[fariza]

Ok no problem

[fariza]

I really don't think ToolManager(figure) is the way to go. I understand what you say about ToolManager(figuremanager), but still, we don't use too many things from figuremanager and what we really want from ToolManager is to be able to interact with both the figure and its holder (figuremanager).

In the way I see it, ToolManager(figuremanager) is easily replaceable by ToolManager(my_own_stuff) as long as my_own_stuff provides a my_own_stuff.figure and (in the near future) emit a figure-setted-event or something like that.

If we let ToolManager(figure) it is more restrictive and very counter-intuitive that figure could change in the lifespan of toolmanager. On the contrary figuremanager or my_own_stuff can not change in the lifespan of toolmanager

[OceanWolf]

Hmm, how do you see ToolManager interacting with figuremanager and can't we do that anyway? I.e. figure.canvas.manager?

I feel convinced that like FigureManager we should make ToolManager figure focused, I also feel convinced that in theory we could pass a FigureManager to ToolManager, and perhaps make it so it can accept both like:

try:
  self.figure = figure_like.figure
  self.figure_manager = figure_like
except:
  self.figure = figure_like
  self.figure_manager = None

I just don't see the need right now and not sure if it will just lead to more confusion, with people falsely assume they have access to properties and methods that they don't have.

[fariza]

I think ToolManager will be instantiated by one bigger object that holds also the figure, it won't be living by itself, and won't be instantiated alone.
It is exactly that bigger object that will be passed as argument to ToolManager. In this moment that object is figuremanager, but when embedding can be your own thing.

Of course toolmanager can access figuremanager going back and forth self.canvas.manager but I think of canvas as parallel to toolmanager not above.

[fariza]

Again, if you think this doesn't belong here, I can make a PR to master.

[OceanWolf]

Hmm, I see it the other way around, I see canvas and figure as parallel, given by the ease we changed FigureManager, actually you say it yourself when you refer to FigureManager as "bigger". They have a one-one relationship, they can't exist without the other. On the other hand can have a canvas/figure, but it doesn't work the other way around., whether one considers embedding, or printing, or whatever.

You have explained "how", I just don't understand "why"... anyway, lets leave this for a future discussion...