interactive differs fromspan_stays in that withspan_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(?) withinteractive. 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 whendrag_from_anywhere = False (the default))

interactive differs fromspan_stays in that withspan_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(?) withinteractive. 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 whendrag_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 theSpanSelector 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!

interactive differs fromspan_stays in that withspan_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(?) withinteractive. 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 whendrag_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 theSpanSelector 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 theSpanSelector (andRectangleSelector, 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?

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

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

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

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

(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 ofdrag_from_anywhere, changing the selection area is much easier and that it covers this case to a certain extent.

@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 bejust the right width).

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

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

Thanks@ericpre for keeping up!

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 someselector.set_* orselector.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)

theartists[0] is theselector.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...

@larsoner suggest you open an issue.