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

[anntzer]

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

[tacaswell]

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

[tacaswell]

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.

[tacaswell]

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

[tacaswell]

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]

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]

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]

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]

This should over-ride the property as well.

[blink1073]

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

[tacaswell]

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

[tacaswell]

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.

[anntzer]

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]

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

[blink1073]

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

[tacaswell]

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