Interactive span selector improvement #20113
Conversation
|
|
Thanks for the review! Indeed, this is a change in behaviour and the motivation for this really is to have the |
Actually, thinking a bit more about it, I would like to add a separate optional parameter to the |
…er release. Deprecate span_stays
…e its interactivity.
…e `span_stays` to `interactive`.
…seSelector have been drawn.
7137b09 to
c9efdfa
Compare
… selector consistently with SpanSelector
|
@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.) |
I see what you mean, however I would imagine that with the recent addition of |
|
@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). |
| interactive : bool, default: False | ||
| Whether to draw a set of handles that allow interaction with the | ||
| widget after it is drawn. |
There was a problem hiding this comment.
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.
|
|
||
| button : `.MouseButton` or list of `.MouseButton` | ||
| The mouse buttons which activate the span selector. | ||
|
|
||
| line_props : dict, default: None |
There was a problem hiding this comment.
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.
|
|
||
| button : `.MouseButton` or list of `.MouseButton` | ||
| The mouse buttons which activate the span selector. | ||
|
|
||
| line_props : dict, default: None | ||
| Line properties with which the interactive line are drawn. Only used |
There was a problem hiding this comment.
| Line properties with which the interactive line are drawn. Only used | |
| Properties of the handler lines at the edges of the span. Only used |
|
🙄 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: 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 the |
|
@larsoner suggest you open an issue. |
PR Summary
This PR updates the
SpanSelectorto make it consistent withRectangleSelector:span_staysin favour ofinteractiveinteractive=Truedrag_from_anywhereoption, in line with Allow Selectors to be dragged from anywhere within their patch #19657pressvrect,rectprops, for theSpanSelectorto_draw,drawtypefor the `RectangleSelectoractive_handlefor SpanSelector and RectangleSelectordirectiona propertyIf you are happy with the changes, I will do the remaining doc, examples, etc updates.
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).