Move widget.{get,set}_active to AxisWidget. #3376

anntzer · 2014-08-17T20:07:04Z

The method was previously defined only for RectangleSelectors (#3375).

Also, an redundant line was removed (RectangleSelectors are already set
active on init by the superclass).

The method was previously defined only for RectangleSelectors (matplotlib#3375). Also, an redundant line was removed (RectangleSelectors are already set active on __init__ by the superclass).

tacaswell · 2014-08-17T20:09:44Z

Could you also property-ify it while you are at it?

tacaswell · 2014-08-17T20:10:23Z

And this will need to be documented in whats_new, see #3349 for a draft of how we are going to do this going forward.

Because selectors must update their background on set_active(True), widget.active was made a property for backwards compatibility.

Because _Selector is a private class there are no references to the actual getter and setter in the rst file.

tacaswell · 2014-08-17T20:37:38Z

You added it to the 1.4.0 whats_new section, this should be under the 1.5.0 section.

tacaswell · 2014-08-17T20:40:00Z

lib/matplotlib/widgets.py

-class SpanSelector(AxesWidget):
+class _SelectorWidget(AxesWidget):
+    def set_active(self, active):
+        super(_SelectorWidget, self).set_active(active)


As a good of an idea is it seems, because we aren't using super every where can you hard-code the over loading?

The reason I am worried is that if this ends up in an MRO where not everything is cooperating, you can get strange bugs. If we are going to go down this route, everything in the AxesWidget class strucutre also needs to use super.

anntzer · 2014-08-17T21:53:35Z

If this doesn't go in a bugfix release of 1.4, there should at least be an indication in the docstrings of the Selector classes that changing the underlying background when the selector is not active is (was) not supported (if useblit=True, which is the default).

tacaswell · 2014-08-17T22:01:08Z

This seems to me to be too big of a change to go into a bug-fix release.

I don't fully understand your comment.

anntzer · 2014-08-18T03:49:30Z

My point is that this is not just an API change; the previous behaviour is actually buggy. Consider the embedding_in_qt4.py from the official docs and apply the following patch:

***************
*** 61,62 ****
--- 61,64 ----
          timer.start(1000)
+         from matplotlib.widgets import LassoSelector
+         self._selector = LassoSelector(self.axes, print)

***************
*** 69,72 ****
--- 71,76 ----

+         self._selector.active = False
          self.axes.plot([0, 1, 2, 3], l, 'r')
          self.draw()
+         self._selector.active = True

Now, if you try to use the lasso selector after an update of the dynamic canvas, the canvas will go back to the original plot. The reason is that update_background has not been called (because the selector ignored the draw_event), so the selector believes that it can still use the initial background. (This is also why in my patch update_background is called when setting active back to True).

tacaswell · 2014-08-18T14:22:52Z

lib/matplotlib/widgets.py

@@ -1081,7 +1093,21 @@ def _update(self):
            self.canvas.draw_idle()


-class SpanSelector(AxesWidget):
+class _SelectorWidget(AxesWidget):
+    def set_active(self, active):


This should over-ride the property as well.

@tacaswell, is this the only thing holding this PR up?

Actually, looking at this again, it does update, the property method is tricksy.

tacaswell · 2014-08-18T14:28:26Z

Ah, it was not clear that this was also fixing a bug and I missed the behavior change reading through.

@mdboom @efiring I am inclined to leave this just on 1.5, if it is broken in 1.4 it has been broken for a long time and widgets are not as widely used as they should/could be. It is not worth picking this apart to back-port just the bug fix. I can also be convinced that this whole thing should be back-ported.

... so it should dynamically retrieve the set_active method.

anntzer · 2014-08-18T17:06:13Z

I agree that breaking the patch apart for the sake of it is a bit overkill, although there are nearly no API changes and one could even remove the {set,get}_active methods themselves to keep the exact same API.

tacaswell · 2014-09-11T03:53:54Z

@blink1073 I will try to take a look at this tomorrow.

blink1073 · 2014-09-11T03:56:20Z

Okay, my suggestion would be for me to handle the change in my PR in lieu of waiting.

ENH : Move widget.{get,set}_active to AxisWidget.

tacaswell · 2014-09-11T04:16:08Z

@blink1073 I re-read this now and my concern was addressed with out me realizing it.

I am against back-porting this change to 1.4.x.

Blitting is not used in matplotlib GTK3Cairo backend, disabled here as well. SpanSelector and RectangleSelector now have same properties "active" and "visible" (see matplotlib/matplotlib#3376 ) FIXME: On layout change, more than one SpanSelector appears on Graph

Move widget.{get,set}_active to AxisWidget.

13ce609

The method was previously defined only for RectangleSelectors (matplotlib#3375). Also, an redundant line was removed (RectangleSelectors are already set active on __init__ by the superclass).

tacaswell added the needs_review label Aug 17, 2014

tacaswell added this to the v1.5.x milestone Aug 17, 2014

anntzer added 2 commits August 17, 2014 13:26

Update background on widget.set_active(True).

8a6845c

Because selectors must update their background on set_active(True), widget.active was made a property for backwards compatibility.

Updated whats_new.rst.

51f6a6e

Because _Selector is a private class there are no references to the actual getter and setter in the rst file.

tacaswell reviewed Aug 17, 2014
View reviewed changes

Minor fixes according to Github discussion.

f1161df

tacaswell reviewed Aug 18, 2014
View reviewed changes

_SelectorWidget overrides the active property.

1b39b24

... so it should dynamically retrieve the set_active method.

fariza mentioned this pull request Sep 10, 2014

Update Selection Widgets #3486

Closed

tacaswell added a commit that referenced this pull request Sep 11, 2014

Merge pull request #3376 from anntzer/widget-active-setter-getter

c74dfa5

ENH : Move widget.{get,set}_active to AxisWidget.

tacaswell merged commit c74dfa5 into matplotlib:master Sep 11, 2014

anntzer deleted the widget-active-setter-getter branch September 11, 2014 06:48

blink1073 mentioned this pull request Sep 12, 2014

Improved selection widget #3502

Merged

tacaswell mentioned this pull request Nov 26, 2014

LassoSelector could implement {set,get}_active #3375

Closed

QuLogic removed the status: needs review label Nov 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move widget.{get,set}_active to AxisWidget. #3376

Move widget.{get,set}_active to AxisWidget. #3376

anntzer commented Aug 17, 2014

tacaswell commented Aug 17, 2014

tacaswell commented Aug 17, 2014

tacaswell commented Aug 17, 2014

tacaswell Aug 17, 2014

anntzer commented Aug 17, 2014

tacaswell commented Aug 17, 2014

anntzer commented Aug 18, 2014

tacaswell Aug 18, 2014

blink1073 Sep 11, 2014

tacaswell Sep 11, 2014

tacaswell commented Aug 18, 2014

anntzer commented Aug 18, 2014

tacaswell commented Sep 11, 2014

blink1073 commented Sep 11, 2014

tacaswell commented Sep 11, 2014

Move widget.{get,set}_active to AxisWidget. #3376

Move widget.{get,set}_active to AxisWidget. #3376

Conversation

anntzer commented Aug 17, 2014

tacaswell commented Aug 17, 2014

tacaswell commented Aug 17, 2014

tacaswell commented Aug 17, 2014

tacaswell Aug 17, 2014

Choose a reason for hiding this comment

anntzer commented Aug 17, 2014

tacaswell commented Aug 17, 2014

anntzer commented Aug 18, 2014

tacaswell Aug 18, 2014

Choose a reason for hiding this comment

blink1073 Sep 11, 2014

Choose a reason for hiding this comment

tacaswell Sep 11, 2014

Choose a reason for hiding this comment

tacaswell commented Aug 18, 2014

anntzer commented Aug 18, 2014

tacaswell commented Sep 11, 2014

blink1073 commented Sep 11, 2014

tacaswell commented Sep 11, 2014