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 reset_start and reset_end to RangeToolLink #5848

Merged
merged 2 commits into from
Aug 9, 2023
Merged

Conversation

hoxbro
Copy link
Member

@hoxbro hoxbro commented Aug 8, 2023

This change will return to the bounds set when clicking the reset button.

@hoxbro hoxbro changed the title Add reset start and end to RangeToolLink Add reset_start and reset_end to RangeToolLink Aug 8, 2023
@hoxbro hoxbro requested a review from droumis August 8, 2023 09:05
@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2023

Codecov Report

Merging #5848 (379ee99) into main (9e789e1) will decrease coverage by 0.01%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5848      +/-   ##
==========================================
- Coverage   88.24%   88.23%   -0.01%     
==========================================
  Files         309      309              
  Lines       63915    63921       +6     
==========================================
  Hits        56402    56402              
- Misses       7513     7519       +6     
Flag Coverage Δ
ui-tests 23.36% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
holoviews/plotting/bokeh/links.py 73.59% <100.00%> (+0.30%) ⬆️
holoviews/tests/plotting/bokeh/test_links.py 98.57% <100.00%> (+0.04%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hoxbro hoxbro added this to the 1.17.1 milestone Aug 8, 2023
@droumis
Copy link
Member

droumis commented Aug 8, 2023

The code looks good to me and behaves in the way I would expect now.. see the video below.

Screen.Recording.2023-08-08.at.7.10.49.PM.mov

Now when a user clicks the "reset" button in a Bokeh-backed plot, the axis range will revert to these specific values that were the range of the initial display, rather than the prior behavior which I think was ranging to the full data extents of the current data.

Although I think this is the correct default behavior, a potential side effect is that if an application relies on the existing default behavior of the "reset" action, this might introduce unexpected behavior. I can't really think of an instance where this would be an issue, except for the case where data in the plot is updated or changes dynamically. Then, the reset_start and reset_end values will not be updated to the new data extents and the reset action may not show the entirety of the new data. I doubt this will be an issue and I think this PR corrects what should be the default behavior, but I added a suggestion to include an option to revert to the previous behavior.

Otherwise, ready to merge

@hoxbro
Copy link
Member Author

hoxbro commented Aug 9, 2023

The previous behavior was "only" introduced in 1.17 (implemented in #5800), so I feel no one has had time to rely on that behavior. I can, like you, not think of a reason why it would be relied upon.

If you have set boundsx or boundsy, the reset button should return to the original state, which is what these changes do.

I would rather not add your suggested setting for this PR if you don't mind. If there is a demand for it, we can easily add it, but we cannot easily remove it if we add it to this PR.

@droumis
Copy link
Member

droumis commented Aug 9, 2023

sounds good!

@droumis droumis merged commit ee01948 into main Aug 9, 2023
14 checks passed
@droumis droumis deleted the add_reset_to_range_link branch August 9, 2023 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants