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

BUG: Ensure that the min/max values can be reached in ctkDoubleSlider and ctkDoubleRangeSlider #1071

Merged
merged 2 commits into from
Mar 11, 2023

Conversation

lassoan
Copy link
Member

@lassoan lassoan commented Mar 10, 2023

The minimum and maximum values were not always reachable in ctkDoubleSlider and ctkDoubleRangeSlider widgets when moved the slider to the minimum or maximum position.

For example:

lo = -1024
hi = 2000
step = (hi - lo) / 1000.

thresholdSlider = ctk.ctkRangeWidget()
thresholdSlider.spinBoxAlignment = qt.Qt.AlignTop
thresholdSlider.setRange(lo, hi)
thresholdSlider.singleStep = step
thresholdSlider.show()
# Impossible to reach 2000.0

slider = ctk.ctkDoubleSlider()
slider.minimum = lo
slider.maximum = hi
slider.singleStep = step
slider.show()
slider.connect("valueChanged(double)", lambda v: print(v))
# Impossible to reach 2000.0

The problem was that the integer slider range was determined by rounding, which could result in the integer slider range not covering the entire value range.
Fixed the problem by extending the integer slider range when it did not fully cover the value range. There are no regressions in tests.

@lassoan lassoan requested a review from pieper March 10, 2023 19:37
@lassoan lassoan self-assigned this Mar 10, 2023
Libs/Widgets/ctkDoubleRangeSlider.cpp Outdated Show resolved Hide resolved
Libs/Widgets/ctkDoubleRangeSlider.cpp Outdated Show resolved Hide resolved
lassoan and others added 2 commits March 10, 2023 17:26
Added a single clarification comment.
… and ctkDoubleRangeSlider

The minimum and maximum values were not always reachable in ctkDoubleSlider and ctkDoubleRangeSlider widgets when moved the slider to the minimum or maximum position.

For example:

```
lo = -1024
hi = 2000
step = (hi - lo) / 1000.

thresholdSlider = ctk.ctkRangeWidget()
thresholdSlider.spinBoxAlignment = qt.Qt.AlignTop
thresholdSlider.setRange(lo, hi)
thresholdSlider.singleStep = step
thresholdSlider.show()
# Impossible to reach 2000.0

slider = ctk.ctkDoubleSlider()
slider.minimum = lo
slider.maximum = hi
slider.singleStep = step
slider.show()
slider.connect("valueChanged(double)", lambda v: print(v))
# Impossible to reach 2000.0
```

The problem was that the integer slider range was determined by rounding, which could result in the integer slider range not covering the entire value range.
Fixed the problem by extending the integer slider range when it did not fully cover the value range.

Co-authored-by: Jean-Christophe Fillion-Robin <[email protected]>
@lassoan lassoan force-pushed the fix-double-slider-minmax-unreachable branch 2 times, most recently from c538926 to b9b5307 Compare March 10, 2023 22:30
@lassoan
Copy link
Member Author

lassoan commented Mar 10, 2023

Thank you @jcfr, I've merged the changes you suggested. Please check it out and approve if looks good.

Copy link
Member

@jcfr jcfr left a comment

Choose a reason for hiding this comment

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

$ LD_LIBRARY_PATH=/home/jcfr/Support/Qt/5.15.2/gcc_64/lib/ hctest -R "^ctkRange|ctkDoubleRange" -j5
Test project /home/jcfr/Projects/CTK-Qt5-build/CTK-build
      Start  63: ctkDoubleRangeSliderTest
      Start  64: ctkDoubleRangeSliderTest1
      Start  65: ctkDoubleRangeSliderTest2
      Start  66: ctkDoubleRangeSliderValueProxyTest
      Start 114: ctkRangeSliderTest
 1/12 Test  #66: ctkDoubleRangeSliderValueProxyTest ...............   Passed    0.26 sec
      Start 115: ctkRangeSliderTest1
 2/12 Test  #63: ctkDoubleRangeSliderTest .........................   Passed    0.46 sec
      Start 116: ctkRangeWidgetTest
 3/12 Test  #65: ctkDoubleRangeSliderTest2 ........................   Passed    0.55 sec
      Start 117: ctkRangeWidgetTest1
 4/12 Test  #64: ctkDoubleRangeSliderTest1 ........................   Passed    0.59 sec
      Start 118: ctkRangeWidgetValueProxyTest
 5/12 Test #115: ctkRangeSliderTest1 ..............................   Passed    0.52 sec
      Start 160: ctkDoubleRangeSliderEventTranslatorPlayerTest1
 6/12 Test #118: ctkRangeWidgetValueProxyTest .....................   Passed    0.33 sec
      Start 172: ctkRangeSliderEventTranslatorPlayerTest1
 7/12 Test #116: ctkRangeWidgetTest ...............................   Passed    0.51 sec
      Start 173: ctkRangeWidgetEventTranslatorPlayerTest1
 8/12 Test #117: ctkRangeWidgetTest1 ..............................   Passed    0.63 sec
 9/12 Test #114: ctkRangeSliderTest ...............................   Passed    1.88 sec
10/12 Test #173: ctkRangeWidgetEventTranslatorPlayerTest1 .........   Passed    8.90 sec
11/12 Test #172: ctkRangeSliderEventTranslatorPlayerTest1 .........   Passed   27.36 sec
12/12 Test #160: ctkDoubleRangeSliderEventTranslatorPlayerTest1 ...   Passed   28.26 sec

@lassoan lassoan merged commit ef3d86b into commontk:master Mar 11, 2023
@lassoan lassoan deleted the fix-double-slider-minmax-unreachable branch March 11, 2023 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants