Interactive span selector improvement #20113

ericpre · 2021-04-29T18:13:37Z

PR Summary

This PR updates the SpanSelector to make it consistent with RectangleSelector:

Deprecate span_stays in favour of interactive
Add edge handle when interactive=True
Add drag_from_anywhere option, in line with Allow Selectors to be dragged from anywhere within their patch #19657
Fix Update background in SpanSelector._press whenever useblit=True. #9660
Improve docstring
Deprecate pressv
When reviewing the docstrings, I realised that some attributes are not expected to be used by users and should be private - I am happy to revert it back if necessary
- Privatize rect, rectprops, for the SpanSelector
- Privatize to_draw, drawtype for the `RectangleSelector
- Privatize active_handle for SpanSelector and RectangleSelector
Make direction a property

If you are happy with the changes, I will do the remaining doc, examples, etc updates.

PR Checklist

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (run flake8 on changed files to check).
New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs should build without error).
Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

lib/matplotlib/widgets.py

anntzer · 2021-05-10T23:39:00Z

interactive differs from span_stays in that with span_stays it was possible to completely unset a previously set SpanSelector by clicking anywhere (effectively drawing a zero-sized selector), whereas this is now impossible(?) with interactive. Not vetoing the change, but I would be slightly annoyed, being used to the old semantics (well, I don't know how intuitive they were either)...
(I guess the old semantics are conflicting with drag_from_anywhere :/ but at least they should beep working when drag_from_anywhere = False (the default))

ericpre · 2021-05-12T18:42:53Z

interactive differs from span_stays in that with span_stays it was possible to completely unset a previously set SpanSelector by clicking anywhere (effectively drawing a zero-sized selector), whereas this is now impossible(?) with interactive. Not vetoing the change, but I would be slightly annoyed, being used to the old semantics (well, I don't know how intuitive they were either)...
(I guess the old semantics are conflicting with drag_from_anywhere :/ but at least they should beep working when drag_from_anywhere = False (the default))

Thanks for the review! Indeed, this is a change in behaviour and the motivation for this really is to have the SpanSelector behaving consistently with other widgets where possible. The current inconsistency could be considered as a bug! ;)
My view on this is that consistency and improved code readability/easier maintenance are worth a (small?) change in behaviour!

ericpre · 2021-05-13T16:10:54Z

interactive differs from span_stays in that with span_stays it was possible to completely unset a previously set SpanSelector by clicking anywhere (effectively drawing a zero-sized selector), whereas this is now impossible(?) with interactive. Not vetoing the change, but I would be slightly annoyed, being used to the old semantics (well, I don't know how intuitive they were either)...
(I guess the old semantics are conflicting with drag_from_anywhere :/ but at least they should beep working when drag_from_anywhere = False (the default))

Thanks for the review! Indeed, this is a change in behaviour and the motivation for this really is to have the SpanSelector behaving consistently with other widgets where possible. The current inconsistency could be considered as a bug! ;)
My view on this is that consistency and improved code readability/easier maintenance are worth a (small?) change in behaviour!

Actually, thinking a bit more about it, I would like to add a separate optional parameter to the SpanSelector (and RectangleSelector, etc) to ignore events outside the widgets and I would prefer to do this in a follow up PR, because there are already a lot of changes in this PR. What do you think?

lib/matplotlib/widgets.py

…er release. Deprecate span_stays

…e its interactivity.

…e `span_stays` to `interactive`.

…seSelector have been drawn.

…elector.

ericpre · 2021-05-20T19:32:22Z

Rebase on master to check that #9660 is fixed with this PR and #20264 and it seems to be the case!

examples/widgets/span_selector.py

lib/matplotlib/tests/test_widgets.py

lib/matplotlib/widgets.py

… selector consistently with SpanSelector

ericpre · 2021-06-19T12:32:16Z

@QuLogic, thanks for the review, I have addressed all points!

ericpre · 2021-06-28T19:23:26Z

@anntzer, @ianhi, @tacaswell, @QuLogic, is there anything left to do on this PR?

lib/matplotlib/widgets.py

…code

lib/matplotlib/widgets.py

anntzer · 2021-06-30T08:34:38Z

(Not that it adds much to the discussion as @tacaswell mostly summarized my points above, but I think that I finally realized why I am so attached to the old UI model: it mimicks the UI of text selection, where you can restart another selection by just clicking anywhere and selecting from there.)

… and add test.

ericpre · 2021-06-30T08:46:51Z

(Not that it adds much to the discussion as @tacaswell mostly summarized my points above, but I think that I finally realized why I am so attached to the old UI model: it mimicks the UI of text selection, where you can restart another selection by just clicking anywhere and selecting from there.)

I see what you mean, however I would imagine that with the recent addition of drag_from_anywhere, changing the selection area is much easier and that it covers this case to a certain extent.

tacaswell · 2021-06-30T19:35:28Z

@anntzer I think the current state captures the best of the old behavior (click outside of the selection region and make a new span) and the new (you can drag either edge and if you click on the middle you drag the whole span). Both of the new interaction modes are things that I personally wished for in my past-life (to either scan a fixed-width window across some line or to tweak it to be just the right width).

timhoffm · 2021-06-30T21:21:30Z

lib/matplotlib/widgets.py

+    interactive : bool, default: False
+        Whether to draw a set of handles that allow interaction with the
+        widget after it is drawn.


To me the primary difference is whether the selector is still visible after the first selection.

When interactive=True I get a selector that persists, but I can still modify later. Coming from that context I would expect interactive=False to still show my selected region, but I cannot modify it anymore. This indicates that interactive is a misleading name.

When starting from scratch, I'd call this persistent, but given that we have span_stays keeping that is ok as well.

timhoffm · 2021-06-30T21:29:12Z

lib/matplotlib/widgets.py

    button : `.MouseButton` or list of `.MouseButton`
        The mouse buttons which activate the span selector.

+    line_props : dict, default: None


Since there's no canonical line in a span. the name does not make it clear what this controls. I'd call these handle_props or handle_line_props if you want to be very explicit.

timhoffm · 2021-06-30T21:30:36Z

lib/matplotlib/widgets.py

        The mouse buttons which activate the span selector.

+    line_props : dict, default: None
+        Line properties with which the interactive line are drawn. Only used


Suggested change

Line properties with which the interactive line are drawn. Only used

Properties of the handler lines at the edges of the span. Only used

timhoffm · 2021-06-30T21:33:50Z

🙄 8 minutes too late. Will open a follow-up PR.

ericpre · 2021-07-01T11:38:08Z

Thanks everyone for the reviews, it has been very useful!

timhoffm · 2021-07-01T20:17:52Z

Thanks @ericpre for keeping up!

larsoner · 2021-07-09T17:32:50Z

In MNE we allow users to update the selector colors, and have:

        selector.rect.set_color(color)
        selector.rectprops.update(dict(facecolor=color))

This now emits a deprecation warning from this PR, but it doesn't indicate how code should be migrated. I looked at this and #20558 and hoped / thought maybe I could find some selector.set_* or selector.handle_props or something to modify but I don't see anything. How should we update our code? (And it might be worth improving the deprecation warning to give some hints to anyone else who hits this issue.) Maybe with something like this?

selector.artists[0].set_color(color)

the artists[0] is the selector.rect. But this just seems like a hack workaround, and if there are properties held internally it will not "stick" so I'm guessing it's not the right idea...

jklymak · 2021-07-09T17:55:34Z

@larsoner suggest you open an issue.

QuLogic added New feature topic: widgets/UI labels Apr 30, 2021

jklymak requested a review from ianhi May 10, 2021 20:53

anntzer reviewed May 10, 2021

View reviewed changes

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved

anntzer reviewed May 10, 2021

View reviewed changes

lib/matplotlib/widgets.py Show resolved Hide resolved

anntzer reviewed May 10, 2021

View reviewed changes

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved

ericpre requested a review from anntzer May 12, 2021 19:27

ericpre mentioned this pull request May 19, 2021

Fix blitting selector #20264

Merged

7 tasks

ianhi reviewed May 19, 2021

View reviewed changes

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved

ericpre added 14 commits May 20, 2021 20:05

Add interactive option to SpanSelector to change it interactively aft…

afa875f

…er release. Deprecate span_stays

Add drag_from_anywhere functionality.

6ebf76a

Add test for dragging span selector.

01319f1

Use line handlers for span selector

cc1d69a

Set handle properties.

7d37461

Update docstring.

db23069

Tidy up.

db8b587

Add API deprecation.

369ff9d

Make SpanSelector consistent with RectangleSelector to further improv…

a3c321c

…e its interactivity.

Test move outside of axis.

349c134

flake8 fixes.

52c3666

Use @_api.rename_parameter instead of @_api.delete_parameter to renam…

bc3808b

…e `span_stays` to `interactive`.

Allow setting extents before the SpanSelector/RectangleSelector/Ellip…

1e774e0

…seSelector have been drawn.

Privatize draw_shape for SpanSelector, RectangleSelector and EllipseS…

4bf2d09

…elector.

ericpre force-pushed the interactive_span_selector branch from 7137b09 to c9efdfa Compare May 20, 2021 19:19

jklymak requested a review from ianhi May 20, 2021 23:28

Deprecate SpanSelector.pressv

bb3c1ae

QuLogic reviewed Jun 19, 2021

View reviewed changes

ericpre added 7 commits June 19, 2021 09:57

Fix typo and docstring

3f72e17

Privatize interactive in RectangleSelector and EllipseSelector

8eeea02

Fix clearing span without blitting.

6266fcf

Clear RectangleSelector and EllipseSelector when clicking outside the…

ff07b6e

… selector consistently with SpanSelector

Deprecate SpanSelector.span_stays attributes

9b98eec

Deprecate SpanSelector.prev attribute

d123ebf

Allow changing direction of span selector

f64e98b

QuLogic reviewed Jun 29, 2021

View reviewed changes

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved

ericpre added 2 commits June 29, 2021 10:44

Fix typos

7abf57c

Use deprecate_privatize_attribute in favour of deprecate to simplify …

e280ed9

…code

QuLogic reviewed Jun 30, 2021

View reviewed changes

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved

Use the property setter of direction when initialising a SpanSelector…

92d1705

… and add test.

tacaswell approved these changes Jun 30, 2021

View reviewed changes

QuLogic approved these changes Jun 30, 2021

View reviewed changes

QuLogic merged commit 4013172 into matplotlib:master Jun 30, 2021

timhoffm requested changes Jun 30, 2021

View reviewed changes

timhoffm mentioned this pull request Jun 30, 2021

Rename recently introduced parameters in SpanSelector #20558

Merged

larsoner mentioned this pull request Jul 9, 2021

BUG: Lost functionality of interactive selector update #20618

Closed

ericpre removed the status: needs review label Oct 17, 2021

	Line properties with which the interactive line are drawn. Only used
	Properties of the handler lines at the edges of the span. Only used

Uh oh!

Interactive span selector improvement #20113

Interactive span selector improvement #20113

Uh oh!

Conversation

ericpre commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anntzer commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericpre commented May 12, 2021

Uh oh!

ericpre commented May 13, 2021

Uh oh!

Uh oh!

ericpre commented May 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericpre commented Jun 19, 2021

Uh oh!

ericpre commented Jun 28, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anntzer commented Jun 30, 2021

Uh oh!

ericpre commented Jun 30, 2021

Uh oh!

tacaswell commented Jun 30, 2021

Uh oh!

timhoffm Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

timhoffm Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

timhoffm Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

timhoffm commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericpre commented Jul 1, 2021

Uh oh!

timhoffm commented Jul 1, 2021

Uh oh!

larsoner commented Jul 9, 2021

Uh oh!

jklymak commented Jul 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ericpre commented Apr 29, 2021 •

edited

Loading

anntzer commented May 10, 2021 •

edited

Loading

timhoffm commented Jun 30, 2021 •

edited

Loading