Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for range selection to RangeTool #13855

Open
wants to merge 3 commits into
base: branch-3.5
Choose a base branch
from

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Apr 30, 2024

This optionally allows users of RangeTool to make range selection gesture, typically with pan gesture. This PR additionally experiments with making this configurable, allowing either pan or tap (tap, move, tap) gestures, to avoid conflict with other tools.

Example of using pan gesture to make range selection:

Screencast.from.30.04.2024.13.09.49.webm

fixes #13646

@mattpap mattpap added this to the 3.5 milestone Apr 30, 2024
@mattpap mattpap added the grant: CZI R5 Funded by CZI Round 5 grant label Apr 30, 2024
@mattpap mattpap force-pushed the mattpap/13646_RangeTool_select branch from 557b028 to 2535273 Compare April 30, 2024 14:49
@@ -524,6 +524,13 @@ def __init__(self, *args, **kwargs) -> None:
A shaded annotation drawn to indicate the configured ranges.
""")

select_gesture = Enum("pan", "tap", "none", default="none", help="""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another word we can use here besides "select"? Concerned about overloading this term and confusion with data selections. start_gesture maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this question boils down to what RangeTool actually is, because when I see this name, I ask RangeWhat?Tool and the answer is always RangeSelectTool (or RangeSelectionTool), thus everything in this PR (branch name, PR title, new properties, docstrings, etc.) involve the word select. I thought about alternatives, but nothing comes close in meaning.

start_gesture maybe?

To me this is the same as above, start_of_what?_gesture. Maybe let's go generic and simply say interaction_gesture? Or perhaps we could be specific with range_selection_gesture and introduce "range selection" as a well defined term (in a glossary of terms) to make sure that we differentiate different types of selections. To me this is a situation similar to what we have with the term "layout", which on its own is really context dependent and may mean server things: CSS/DOM layout, (our) computed layout (LayoutDOM and canvas layouts) and perhaps other.

Comment on lines +201 to +210
if (!this.model.active) {
return
}

if (ev.key == "Escape") {
if (this._is_selecting) {
this._stop()
return
}
}
Copy link
Member

@bryevdv bryevdv May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it could be a bit more compact

Suggested change
if (!this.model.active) {
return
}
if (ev.key == "Escape") {
if (this._is_selecting) {
this._stop()
return
}
}
if (this.model.active) {
if (ev.key == "Escape" && this._is_selecting) {
this._stop()
}
}

Copy link
Member

@bryevdv bryevdv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattpap I would like to have select_gesture renamed, but apart from that 👍

@bryevdv
Copy link
Member

bryevdv commented May 17, 2024

For me "select" carries a strong notion of choosing "one or several things from a larger set of things". The range tool does not have any action that makes this kind of association for me. Instead I would say that the range tool...

  • ...defines a range
  • ...sets a range
  • ...configures a range
  • ...updates a range

But more broadly, I think that is is understood that interactive tools perform actions on the basis of a series of gestures input from the user. In this context (to me) "start gesture" means "what gesture starts the interaction for this tool". I'd provide the elaboration in the help string:

    start_gesture = Enum("pan", "tap", "none", default="none", help="""
    Which gesture will start a range update interaction in a new location. 

    When the value is "tap", a new range starts at the location where a single
    tap is made. The range is updated continuously while the pointer moves. 
    Tapping at another location sets the final value of the range. 

    When the value is "pan", a new range starts at the location where a pointer
    drag operation begins. The range is updated continuously while the drag 
    operation continues. Ending the drag operation sets the final value of the 
    range.  

    When the value is "none", only existing range definitions may be updated,
    by dragging their edges or interiors. 

    Configuring this property allows to make this tool simultaneously co-exist
    with another tool that would otherwise share a gesture.
    """)

That (to me) explains everything perfectly clearly, but it's entirely possible that what makes sense to me does not make sense to others. I'm not married to start_gesture specifically, but I'd like to avoid "select" confusion, and if possible not have a very long property name either. cc @tcmetzger for naming thoughts.

@tcmetzger
Copy link
Member

@bryevdv, your explanation and docstring make sense to me!

If I understand correctly, we are looking for a name for the action (gesture) that would initiate the process of defining the start point and eventually also the end point of a selection.

If we want to keep the name short, I can see how start_gesture makes a lot of sense. However, we could also consider a three-word name, if that is in line with our naming conventions. In that case, I'd suggest one of those:

  • range_setting_gesture
  • range_definition_gesture
  • range_select_gesture

Those (albeit somewhat long) names would include the notion that this gesture is not only for setting the start point but also the end point (i.e. the whole range).

Either way, the docstring that you propose helps a lot, and I'd strongly suggest we include that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grant: CZI R5 Funded by CZI Round 5 grant status: ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Support BoxSelectTool-like range-setting for the RangeTool
3 participants