ToolManager/Tools adding methods to set figure after initialization by fariza · Pull Request #6405 · matplotlib/matplotlib

fariza · 2016-05-11T21:43:16Z

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 :

Removing the automatic setting of figure in ToolManager.__init__ and ToolBase.__init__
canvas becomes a property that is accessed strictly from figure
set_figure is called explicitly on the ToolManager and ToolBase objects

fariza · 2016-05-12T13:33:40Z

Test fail is not related

fariza · 2016-05-12T15:56:00Z

@OceanWolf can you take a look at this?

OceanWolf · 2016-05-15T23:32:08Z

I will do my best to find time to look at these two PRs soon

fariza · 2016-05-20T18:58:41Z

@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

fariza · 2016-05-20T19:05:05Z

@tacaswell I would like to have this to have reviewed by I really don't know who else to ask, maybe you or @efiring?

OceanWolf · 2016-05-20T19:21:26Z

lib/matplotlib/backend_managers.py

+        """Figure that holds the canvas"""
+        return self._figure
+
+    def set_figure(self, figure):


Why not a property setter?

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): ....

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.

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.

If we agree, it is matter of just adding

@figure.setter def figure(self, figure): self.set_figure(figure)

Is this an acceptable solution?

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

fariza · 2016-05-22T13:37:48Z

@OceanWolf i got rid of the manager reference for toolmanager and added the setters for for the figures.

OceanWolf · 2016-05-23T09:45:36Z

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 True, or to False, and thus how toolmgr.figure = my_figure would work.

fariza · 2016-05-24T12:07:36Z

With the second argument the property setter stops working.
Let's leave it for later

OceanWolf · 2016-05-24T12:10:00Z

It doesn't stop working, because it gets passed as kwarg.

fariza · 2016-05-24T12:19:59Z

What I mean is that it is impossible to change the value by calling the property setter

OceanWolf · 2016-05-24T12:22:00Z

Sure, but you have declared set_figure as public. We just need to decide the default behaviour. I guess default to True.

fariza · 2016-05-24T12:44:26Z

Done, I tried and it looks better than I thought.

OceanWolf · 2016-05-24T21:20:17Z

Yay, fabulous! I have no further comments. Anyone else?

efiring · 2016-05-24T21:55:03Z

lib/matplotlib/backend_managers.py

        self.messagelock = widgets.LockDraw()

+        if figure:
+            self.figure = figure


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.

fariza · 2016-05-24T23:17:08Z

@efiring I updated the docstrings

fariza · 2016-05-25T16:05:59Z

@QuLogic the interaction is bidirectional

fariza · 2016-06-02T14:17:53Z

@efiring @OceanWolf can we merge?

OceanWolf · 2016-06-02T14:23:46Z

Fine by me!

efiring · 2016-06-02T16:16:22Z

lib/matplotlib/backend_tools.py

+
+    @property
+    def canvas(self):
+        return self.figure.canvas


Rather than have the extra indirection via the property, this could be self._figure.canvas.

Here, too, it seems like we have a situation where accessing a property can lead to an obscure exception.

OceanWolf · 2016-06-02T17:01:10Z

lib/matplotlib/backend_managers.py

+            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)


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.

fariza · 2016-06-02T17:55:22Z

@efiring that was a good catch, I modified the code so, it returns None instead of rising an error.
Also, I do a check for existence before connecting again when figure is setted

OceanWolf · 2016-06-03T17:46:50Z

lib/matplotlib/backend_managers.py

                self._toggled.setdefault(tool_obj.radio_group, None)

+        if self.figure:
+            tool_obj.set_figure(self.figure)


Do we need these two lines? The tool inits with ToolBase._figure = None

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

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

you're right, Done

fariza · 2016-06-06T21:23:57Z

I just removed the last check for figure that I forgot in the __init__ method
Anything else?

fariza · 2016-06-08T21:40:12Z

I think this is ready to merge

OceanWolf · 2016-06-09T17:53:47Z

Ahh, I was going to say squash the commits first, but okay.

@fariza which other PR did you want me to look at?

mdboom added the status: needs review label May 11, 2016

fariza changed the title ~~adding methods to set figure after initialization~~ ToolManager/Tools adding methods to set figure after initialization May 12, 2016

fariza force-pushed the tools-set-figure-by-event branch from 54ddc90 to 7d7088d Compare May 17, 2016 21:16

OceanWolf reviewed May 20, 2016
View reviewed changes

tacaswell added this to the 2.1 (next point release) milestone May 21, 2016

efiring reviewed May 24, 2016
View reviewed changes

efiring reviewed Jun 2, 2016
View reviewed changes

OceanWolf reviewed Jun 2, 2016
View reviewed changes

OceanWolf reviewed Jun 3, 2016
View reviewed changes

ToolManager and Tools can be initialized without tools

5d73e9f

fariza force-pushed the tools-set-figure-by-event branch from 3b4500f to 5d73e9f Compare June 8, 2016 19:55

efiring merged commit e69e565 into matplotlib:master Jun 9, 2016

efiring removed the status: needs review label Jun 9, 2016

fariza deleted the tools-set-figure-by-event branch June 11, 2016 23:02

Uh oh!

Conversation

fariza commented May 11, 2016

Uh oh!

fariza commented May 12, 2016

Uh oh!

fariza commented May 12, 2016

Uh oh!

OceanWolf commented May 15, 2016

Uh oh!

fariza commented May 20, 2016

Uh oh!

fariza commented May 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fariza commented May 22, 2016

Uh oh!

OceanWolf commented May 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fariza commented May 24, 2016

Uh oh!

OceanWolf commented May 24, 2016

Uh oh!

fariza commented May 24, 2016

Uh oh!

OceanWolf commented May 24, 2016

Uh oh!

fariza commented May 24, 2016

Uh oh!

OceanWolf commented May 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fariza commented May 24, 2016

Uh oh!

fariza commented May 25, 2016

Uh oh!

fariza commented Jun 2, 2016

Uh oh!

OceanWolf commented Jun 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fariza commented Jun 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fariza commented Jun 6, 2016

Uh oh!

fariza commented Jun 8, 2016

Uh oh!

OceanWolf commented Jun 9, 2016

Uh oh!

Reviewers

Assignees

Labels

OceanWolf commented May 23, 2016 •

edited

Loading