pyapp-kit / superqt

Missing widgets and components for Qt-python
https://pyapp-kit.github.io/superqt/
BSD 3-Clause "New" or "Revised" License
210 stars 37 forks source link

fix: Check min max versus current value #221

Closed psobolewskiPhD closed 12 months ago

psobolewskiPhD commented 1 year ago

Closes https://github.com/pyapp-kit/superqt/issues/220

As suggested by Grzegorz here https://github.com/pyapp-kit/superqt/issues/220#issuecomment-1824785806 in this PR I have setMin and setMax check versus the current value and if that is smaller/larger it's reset to min/max.

I also added a test for the issue that fails on current main, but passes with this fix.

psobolewskiPhD commented 1 year ago

@Czaki also suggested:

it could be nice to add check if minimum is lower than maximum in setRange

So this is the case where someone does setRange(100, 0) or similar. In this case is it better to raise a ValueError or just swap min and max so they are correct?

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (b927159) 87.45% compared to head (7d9223e) 87.44%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #221 +/- ## ========================================== - Coverage 87.45% 87.44% -0.01% ========================================== Files 46 46 Lines 3355 3361 +6 ========================================== + Hits 2934 2939 +5 - Misses 421 422 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

goanpeca commented 1 year ago

Let's do what Qt does for it

https://doc.qt.io/qt-6/qabstractslider.html#setRange

If max is smaller than min, min becomes the only legal value.


update

Justed tested with QSlider and there seems to be no error, but the widget becomes invisible, I guess because min. and max become the same.

Swapping would be unexpected behavior so I suggest we either follow what Qt does, (puts min and max equal to min?) or raise a value error.

I would prefer the later, not sure what others think :)

psobolewskiPhD commented 1 year ago

I was also leaning towards ValueError. Can then use with pytest.raises(ValueError): to test.

Edit: but there is some value in mimicking Qt behavior, so totally open to implementing that too!

tlambert03 commented 1 year ago

Yeah, I agree with mimicking qt behavior. It should just do the same thing that the regular QSpinBox does.