Fix values of slider label when range/value is set outside default #320
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
==========================================
- Coverage 86.40% 86.35% -0.05%
==========================================
Files 49 49
Lines 3861 3862 +1
==========================================
- Hits 3336 3335 -1
- Misses 525 527 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
pyside2 fails looks unrelated |
|
Hi @TimMonko If something has mysteriously changed at some point, and it's not clear where the fix should go, would you be able to run git bisect with superqt installed in editable mode and narrow down exactly what superqt commit caused the change? That might help us fill in the picture a bit and make sure we're fixing the right thing for you |
|
@tlambert03 you have no idea how much you've indirectly taught me over the years. Today was my first time using I found that 55b6639 was the first bad commit. It looks like there was a switch from using a spinbox to lineedit, which makes this more confusing to understand where the change should go than I expected. So, I connected now to rangeChanged in I'm not sure what to do about the test, maybe keep the one I have, or make a new direct test of |
|
I was worried that it was that commit... |
|
Yeah... I was too. That commit has definitely cropped up for me in other places as well (stuff where it's maybe not a direct bug, but it has made things more complicated). One possible solution that I'd like to consider (in parallel perhaps) is to have two versions, or a feature flag on a single version... one that reverts the changes back to the spinBox, and one that uses the line edit to support scientific notation. It's somewhat unfortunate to add the complexity and side effects for a feature that I (personally) find to be a minority use case? |
|
We need this in napari because QSpinBox do not support values above |
yes, @Czaki, if you read what I said, I wasn't proposing that we remove it altogether. but propose a way such that you can have what you need for your relatively edge use case, without imposing what has turned out to be a problematic design for the common use case. (i.e. have both) |
|
btw, I did just remember what the other issue with #226 was (it's in ndv, with pyapp-kit/ndv#213 ... where we've actively pinned superqt to |
|
@tlambert03 Could you show the command to reproduce segfaults from |
|
thanks @Czaki, I appreciate it! This works pretty consistently for me on macos: then, not sure if this is also useful (honestly I seem to remember the backtrace being more informative last time): $ source .venv/bin/activate
$ lldb python -- -m pytest -W ignore -qoutputand I think I also did install superqt locally, editable. and according to pyapp-kit/ndv#204 (comment) narrowed it down to a specific commit |
|
Ok. I'm able to reproduce failure on Linux. |
|
thanks! and yes I would also add that if we find that pyside6 has changed anything in the meantime, and simply updating pyside6 solves everything, then I'm more than happy to just bump our pinnings |
|
This segfaults for me on Windows:
|
3 participants
I noticed that in the camera popup widget in napari that the value of the label in slider labels no longer shows value outside the default range of the sliders (0,99). napari/napari#8534 (Should the decimal issue, which I proposed to fix in napari like this: https://github.com/napari/napari/pull/8535/files#diff-b7be858b0c42314c0f7fbaaa128a2e6a24822a61aa780d9374153c02a7a6b30f actually be fixed in superqt?)
I added a test here that all assertions fail on main, but pass with this PR.
I don't know if this code change is the correct fix because this is my first time looking inside superqt, but my thought process was that the label range needs to be connected to changing the slider range.
I'm only confused because this isn't how it used to work (see the screenshot in napari/napari#7626), but a quick scan of the file history isn't making anything stand out in this method as changed, so maybe this isn't the right place to fix this / the cause of the bug. It seems both the range and decimals somehow got disconnected from the label