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

Merged
merged 6 commits into from
May 30, 2024

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
@mattpap mattpap force-pushed the mattpap/13646_RangeTool_select branch from 2535273 to 5c8f4b6 Compare May 15, 2024 11:04
src/bokeh/models/tools.py Outdated Show resolved Hide resolved
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!

@mattpap mattpap force-pushed the mattpap/13646_RangeTool_select branch from 5c8f4b6 to 6ceab9d Compare May 28, 2024 08:42
@mattpap mattpap requested a review from tcmetzger May 28, 2024 09:12
@mattpap
Copy link
Contributor Author

mattpap commented May 28, 2024

I renamed the property to start_gesture to move this PR forward. However, when writing release notes, I had to find an explanation for this feature and the best I could come up with is "range setting gesture", so perhaps let's use setting_gesture instead? @tcmetzger, I requested review from you to finalize the matter. Pinging @philippjfr, @hoxbro if you have any further suggestions or are fine with the existing naming convention.

@jbednar
Copy link
Contributor

jbednar commented May 28, 2024

And just gesture isn't sufficient? (It sounds like there may be a different gesture at some point for the end, but I can't quite see how that would work, and so I'd vote to keep it as simple as practical otherwise!)

@mattpap
Copy link
Contributor Author

mattpap commented May 28, 2024

And just gesture isn't sufficient? (It sounds like there may be a different gesture at some point for the end, but I can't quite see how that would work, and so I'd vote to keep it as simple as practical otherwise!)

That's my takeaway from today's CZI meeting, that simple gesture had the most support. I don't mind that, in fact that was my initial choice before I switched to select_gesture. I don't expect other kinds of gestures for this tool. Though even if there was one in the future, it can use a more specific name.

@mattpap
Copy link
Contributor Author

mattpap commented May 30, 2024

I will go ahead and merge this given the existing approval. We can always change the naming convention later.

@mattpap mattpap merged commit 1ff0f75 into branch-3.5 May 30, 2024
25 checks passed
@mattpap mattpap deleted the mattpap/13646_RangeTool_select branch May 30, 2024 06:54
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: accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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