-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ToolManager/Tools adding methods to set figure after initialization #6405
Conversation
Test fail is not related |
@OceanWolf can you take a look at this? |
I will do my best to find time to look at these two PRs soon |
54ddc90
to
7d7088d
Compare
@OceanWolf just in case you're going to rebase #4143 please take a look at this one first. Just to prevent you having to do two rebases |
@tacaswell I would like to have this to have reviewed by I really don't know who else to ask, maybe you or @efiring? |
"""Figure that holds the canvas""" | ||
return self._figure | ||
|
||
def set_figure(self, figure): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not a property setter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I put it as a setter, then when you want to override it you will have to redeclare the property and the setter with it's decorators, if not, the behavior is not the same between the base and the subclass (I can see the problems falling from the sky).
The other option (the one that I normally use) is the base to the declare the property and its setter with private methods that actually do the set and get. And if you need to override this methods, you override the _set_x
or _get_x
and the behavior is the same because you don't touch the property
@property
def x(self):
return self._get_x()
@x.setter(self, value):
self._set_x(value)
def _get_x(self):
....
def _set_x(self, value):
....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, that problem, I recall running into it before... I just dislike the asymmetry of a = foo
and set_foo(a)
, rather than foo = a
, I find asymmetry makes the API more complicated to learn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, and this is a problem that we are going to have everywhere. So we can decide to have set properties defined in the base that call the setters and getters and those are the ones that are specific to each subclass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we agree, it is matter of just adding
@figure.setter
def figure(self, figure):
self.set_figure(figure)
Is this an acceptable solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I do this as soon as I get home, I have to leave now.
Thanks for the feedback, keep it coming, I want this merged really bad
@OceanWolf i got rid of the manager reference for toolmanager and added the setters for for the figures. |
Looks much better. One idea I had, but again perhaps too complicated (and can easily get added later), making it ToolManager.set_figure(self, figure, update_tools=False):
# snip
if update_tools:
for tool in self.tools:
tool.figure = figure the most complicated part here comes from deciding whether the kw should default to |
With the second argument the property setter stops working. |
It doesn't stop working, because it gets passed as kwarg. |
What I mean is that it is impossible to change the value by calling the property setter |
Sure, but you have declared |
Done, I tried and it looks better than I thought. |
Yay, fabulous! I have no further comments. Anyone else? |
@@ -74,6 +73,43 @@ def __init__(self, canvas): | |||
self.keypresslock = widgets.LockDraw() | |||
self.messagelock = widgets.LockDraw() | |||
|
|||
if figure: | |||
self.figure = figure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be nit-picking, but I would put the initialization of self._figure
right above this, and instead of using the property I would use the setter. To me, eliminating that indirection would make the code more readable with no cost.
@efiring I updated the docstrings |
@QuLogic the interaction is bidirectional |
@efiring @OceanWolf can we merge? |
Fine by me! |
|
||
@property | ||
def canvas(self): | ||
return self.figure.canvas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than have the extra indirection via the property, this could be self._figure.canvas
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, too, it seems like we have a situation where accessing a property can lead to an obscure exception.
self.canvas.mpl_disconnect(self._key_press_handler_id) | ||
self._figure = figure | ||
self._key_press_handler_id = self.canvas.mpl_connect( | ||
'key_press_event', self._key_press) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think we should definitly allow the user to reset to None by explicitly passing in None, otherwise, we cannot return to the default original state. Thus we should match the if statement from the init here. Maybe a function is_bound
or something to make this test more changeable in the future.
@efiring that was a good catch, I modified the code so, it returns |
@@ -245,6 +283,8 @@ def add_tool(self, name, tool, *args, **kwargs): | |||
else: | |||
self._toggled.setdefault(tool_obj.radio_group, None) | |||
|
|||
if self.figure: | |||
tool_obj.set_figure(self.figure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these two lines? The tool inits with ToolBase._figure = None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the tools initialize with None, but when addding a tool, it is better to set the figure to the current figure of the Toolmanager, we could add a kwarg to add_tool
to bypass the tool.set_figure
but I find it a little bit overkill
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I said that awkwardly, I meant to say "do we really need the if?", i.e. keep the second line in there. If self.figure == None
, then the call to tool_obj.set_figure(self.figure) just does a no-op...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, Done
I just removed the last check for figure that I forgot in the |
3b4500f
to
5d73e9f
Compare
I think this is ready to merge |
Ahh, I was going to say squash the commits first, but okay. @fariza which other PR did you want me to look at? |
Wanting to cross control figures (tool from one FigureManger affects a figure from other FigureManager) the setting of the figure is separated from the initialization
The changes are :
ToolManager.__init__
andToolBase.__init__
set_figure
is called explicitly on the ToolManager and ToolBase objects