Skip to content

Fix spurious event on rangeslider.setRange with non-list value - #363

Merged
tlambert03 merged 2 commits into
pyapp-kit:mainfrom
brisvag:fix/range-set-value
Oct 30, 2026
Merged

tlambert03 merged 2 commits into
pyapp-kit:mainfrom
brisvag:fix/range-set-value

Conversation

@brisvag

@brisvag brisvag commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

Spotted in napari/napari#9773.

Internally, _GenericSlider does not convert the given value to tuple/list, but converts it to list when setting self._position and then compares directly self._value == self._position in setValue:

if self._value == value and self._position == value:
return

This results on an extra event fired when setting range, because it thinks that the "new" value is different from the old one.

I added a test that fails on main. I'm not sure this is the right fix, but it seems ok to me since anyways we always return tuple(self._value) from self.value(), so coercing shoildn't matter for the user.

with qtbot.waitSignal(sld.rangeChanged):
sld.setRange(0, 255)

mock.assert_not_called()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the line that fails on main (it says it's called once).

@codecov

codecov Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.92%. Comparing base (4668c07) to head (980c07a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #363   +/-   ##
=======================================
  Coverage   84.92%   84.92%           
=======================================
  Files          49       49           
  Lines        3940     3940           
=======================================
  Hits         3346     3346           
  Misses        594      594           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tlambert03

Copy link
Copy Markdown
Member

thanks @brisvag
diagnosis is good and fix is in the right direction... but it can't be where it currently is because this test now fails for PySide6. PySide6 applies constructor kwargs by calling the Python methods. So QRangeSlider(maximum=10) calls setMaximum, which calls setRange and then setValue(self._value) while _value is still the base class's 0.0, (because the range slider only assigns its list after super().__init__), so list(0.0) raises TypeError.

def test_range_kwarg_in_constructor(qtbot):
    # PySide6 calls the python setMaximum during __init__, before `_value` is a list
    qtbot.addWidget(QRangeSlider(maximum=10))

can you try to normalize in _GenericRangeSlider._bound and drop the setValue override:

if isinstance(value, (list, tuple)):
    return [self._bound(v) for v in value]

@brisvag

brisvag commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Like this? Why were tests not failing though? 🤔

@tlambert03

Copy link
Copy Markdown
Member

Like this?

yes, thanks! merging now

Why were tests not failing though? 🤔

Because, this ultimately isn't our feature... it's an upstream feature of PySide. However, we should be testing it and we aren't. And now that I look into it more, it's actually already failing on main (relevant #300). So I shouldn't have made it sound like this PR would be the first to break it... but your last commit will prevent it from become a bug if/when we do test and support it.

thanks!

@tlambert03
tlambert03 merged commit a81c9c7 into pyapp-kit:main Oct 30, 2026
27 checks passed
@tlambert03

Copy link
Copy Markdown
Member

releasing as v0.9.1 so you can return to the upstream issue 👍

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