panel icon indicating copy to clipboard operation
panel copied to clipboard

Add hard bounds to editable sliders

Open hoxbro opened this issue 1 year ago • 13 comments

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.

hoxbro avatar Aug 03 '22 10:08 hoxbro

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%) :arrow_up:
unitexamples-tests 76.91% <48.75%> (-0.50%) :arrow_down:

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%) :arrow_down:
panel/param.py 86.73% <100.00%> (+0.08%) :arrow_up:
panel/tests/test_param.py 99.66% <100.00%> (+<0.01%) :arrow_up:
panel/tests/ui/widgets/test_sliders.py 100.00% <100.00%> (ø)
panel/widgets/slider.py 95.13% <100.00%> (+11.49%) :arrow_up:
panel/util.py 86.36% <0.00%> (-0.38%) :arrow_down:
panel/io/state.py 68.11% <0.00%> (-0.15%) :arrow_down:
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

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

codecov[bot] avatar Aug 03 '22 11:08 codecov[bot]

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.

philippjfr avatar Aug 08 '22 09:08 philippjfr

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

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

hoxbro avatar Aug 08 '22 10:08 hoxbro

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.

philippjfr avatar Aug 08 '22 10:08 philippjfr

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 avatar Aug 08 '22 13:08 jbednar

[Oops; hit close accidentally!]

jbednar avatar Aug 08 '22 13:08 jbednar

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.

philippjfr avatar Aug 08 '22 13:08 philippjfr

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)

maximlt avatar Aug 08 '22 14:08 maximlt

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?

jbednar avatar Aug 08 '22 21:08 jbednar

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!

philippjfr avatar Aug 09 '22 10:08 philippjfr

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.

https://user-images.githubusercontent.com/19758978/183668197-70299768-17b2-44d3-b610-c17da86fc95f.mp4


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

hoxbro avatar Aug 09 '22 14:08 hoxbro

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

philippjfr avatar Aug 10 '22 13:08 philippjfr

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 avatar Aug 11 '22 15:08 philippjfr

Thanks, merging (py 3.9 failure still unrelated).

philippjfr avatar Aug 11 '22 17:08 philippjfr