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 hard bounds to editable sliders #3739

Merged
merged 15 commits into from
Aug 11, 2022
Merged

Add hard bounds to editable sliders #3739

merged 15 commits into from
Aug 11, 2022

Conversation

hoxbro
Copy link
Member

@hoxbro hoxbro commented Aug 3, 2022

Fixes #3637

The way to set the hard bounds is by using fixed_start and fixed_end, which are then set on the value parameter as bounds and start and end of the text input widgets. The reason for using two parameters and not one is to mimic how the widget start and end work.

I tried to set the bounds of the value parameter of the non-editable sliders to fix the #2735, but it broke a lot of the tests, so I removed it again (for now).

The removal of self._slider.jscallback is because it didn't seem to do anything, but maybe it is to avoid multiple synchronizations between the bokeh model and the param class. I will add it back again with a comment if needed.

The last thing is on the EditableRangeSlider, I set the start value of the text input always to be smaller than the end value and vice versa.

@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #3739 (2e90121) into master (fe814cb) will increase coverage by 0.42%.
The diff coverage is 98.75%.

@@            Coverage Diff             @@
##           master    #3739      +/-   ##
==========================================
+ Coverage   83.69%   84.11%   +0.42%     
==========================================
  Files         209      211       +2     
  Lines       30053    30540     +487     
==========================================
+ Hits        25152    25690     +538     
+ Misses       4901     4850      -51     
Flag Coverage Δ
ui-tests 34.14% <74.37%> (+1.47%) ⬆️
unitexamples-tests 76.91% <48.75%> (-0.50%) ⬇️

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

Impacted Files Coverage Δ
panel/tests/widgets/test_slider.py 98.89% <94.11%> (-1.11%) ⬇️
panel/param.py 86.73% <100.00%> (+0.08%) ⬆️
panel/tests/test_param.py 99.66% <100.00%> (+<0.01%) ⬆️
panel/tests/ui/widgets/test_sliders.py 100.00% <100.00%> (ø)
panel/widgets/slider.py 95.13% <100.00%> (+11.49%) ⬆️
panel/util.py 86.36% <0.00%> (-0.38%) ⬇️
panel/io/state.py 68.11% <0.00%> (-0.15%) ⬇️
panel/tests/ui/widgets/test_card.py
panel/tests/ui/layout/test_accordion.py 100.00% <0.00%> (ø)
panel/tests/ui/layout/test_card.py 100.00% <0.00%> (ø)
... and 4 more

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

@philippjfr
Copy link
Member

Looking good to me! Not entirely sure fixed_start and fixed_end convey the concept of a hard bound but don't love hard_start and hard_end either. Any other suggestions?

Separately I think this PR should also update the Param pane to map the softbounds and bounds to these new parameters on the editable sliders.

@hoxbro
Copy link
Member Author

hoxbro commented Aug 8, 2022

I don't favor fixed_* either but also liked it better than hard_*.

Some suggestions for another name: pinned, immovable, locked, strict, firm.

@philippjfr
Copy link
Member

Maybe let's move away from the "hard" connotation and instead describe what it actually bounds, e.g. editor_start and editor_end? Ping @maximlt and @jbednar for votes/suggestions.

@jbednar
Copy link
Member

jbednar commented Aug 8, 2022

I think "hard bounds" is self-explanatory; that's something people say in the real world. But "hard_end" doesn't really work. It can't be a tuple to avoid that problem?

I don't think "editor" is the right concept, certainly not at the Param level but it also seems odd to me at the Panel level. "Soft bounds" are really "suggested bounds", i.e., they are a weak suggestion of a useful range, a hint that doesn't specifically have to do with editors or widgets, but a declaration that the author of this app thought that this range was useful or the best starting point. Such a suggestion maps nicely to the initial bounds of a widget, but that's not its only interpretation.

@jbednar jbednar closed this Aug 8, 2022
@jbednar jbednar reopened this Aug 8, 2022
@jbednar
Copy link
Member

jbednar commented Aug 8, 2022

[Oops; hit close accidentally!]

@philippjfr
Copy link
Member

For compatibility with other sliders the start and end values will continue to determine the slider bounds. Adding additional parameters to control the minimum and maximum values that can be entered in the editor/spinner box is what we are discussing here.

How that maps onto the bounds and softbounds is entirely up to us but also a separate issue from naming the parameters that determine the hard bounds on the widget.

@maximlt
Copy link
Member

maximlt commented Aug 8, 2022

I like better hard_* but that's maybe just because I got used to that working with Param.

One note, the docs will have to be updated, this is e.g. the current definition of the EditableFloatSlider that doesn't say that the editor can be used to set a value outside of the soft-bounds offered by the slider.

The EditableFloatSlider widget allows selecting selecting a numeric floating-point value within a set bounds using a slider and for more precise control offers an editable number input box.

By the way are we okay with the fact that if hard/fixed/..._* isn't set then that does mean that by default there's no hard bound? Not entirely sure but I would guess that in most cases people actually want to have hard bounds when they use sliders in their app. They would have to write pn.widgets.Editable...(start=1, hard_start=1, end=2, hard_end=2) to get that. I see the editable slider as a way to provide users an easy way to enter a specific value, more than a way to enter a value outside of the slider bounds.

(That's another issue but the Input number widgets don't seem to have a way to indicate their bounds)

@jbednar
Copy link
Member

jbednar commented Aug 8, 2022

By the way are we okay with the fact that if hard/fixed/..._* isn't set then that does mean that by default there's no hard bound? Not entirely sure but I would guess that in most cases people actually want to have hard bounds when they use sliders in their app.

I would have assumed that any previously supported single bounds would have to be hard bounds, and that any new set of bounds are optional soft bounds that override the previous bounds if present. Isn't that the only way the system can behave reasonably?

@philippjfr
Copy link
Member

I'm actually okay with fixed_start and fixed_end now since the semantics are that start/end can be overridden by manually entering a value but the fixed_ bounds cannot. Just want to see updates to the docs for the various Editable widgets and then I'm very happy with this PR. Thanks @hoxbro!

@hoxbro
Copy link
Member Author

hoxbro commented Aug 9, 2022

When updating the documentation, I updated a small bug, which meant that the start/end of the _slider could exceed the fixed_start and fixed_end, as the following video show. This should now be fixed.

screenrecord-2022-08-09_15.37.53.mp4

The fixed_start and fixed_end of EditableRangeSlider were set to param.Integer which was a copy/paste error...

@philippjfr
Copy link
Member

A couple of UI tests would be super nice, otherwise this looks ready to merge.

@philippjfr
Copy link
Member

Looks like one of the tests is flakey. Can you see if you can fix it? Otherwise use the flaky marker with max_runs=3.

There also seems to be an unrelated VTK issue on Windows.

@philippjfr
Copy link
Member

Thanks, merging (py 3.9 failure still unrelated).

@philippjfr philippjfr merged commit e4e0e4d into holoviz:master Aug 11, 2022
@hoxbro hoxbro deleted the hard_bounds_editable branch August 11, 2022 19:00
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.

Editable slider widgets should have hard bounds
4 participants