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

Breaking Refactor: Make GenericSliders inherit QWidget rather than QSlider or QAbstractSlider #249

Open
tlambert03 opened this issue Jun 3, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@tlambert03
Copy link
Member

The original design of all the Range sliders here (which all subclass sliders._generic_slider._GenericSlider) was to make them behave as much like drop-in replacements for QSlider as possible. I wanted them all to pass isinstance(obj, QSlider) checks, and inasmuch as possible, inherit all of the behaviors of QSlider.

However, that has proven difficult to maintain, and the biggest issue relates to signals (particularly with PySide). In most of the slider variants the value() (and therefore valueChanged signal) is of a different type than the superclass. QDoubleSlider, of course, has a value() of type float, while QRangeSlider has a value type of tuple[int, ...].

We do some "unsavory" patching of the signal instances, on the object instances themselves, like here and here. And, for the most part it works, but requires very ugly hacks (#51). But really, it doesn't work well... and it's just a matter of time before it breaks again. I already get misc bus errors and segfaults during tests on pyside6 with the current strategy (thought pyqt6 is fine).

So, the two main options that I can see are:

  1. Don't try to inherit from QAbstractSlider, since it already has the signal names we want to use, and changing the value type of signals has proven to be a bad idea. A refactor like this would mean either using composition, or re-implementing basically all of the methods in the C implementation of QAbstractSlider to make these objects essentially duck-QSliders (many of those are reimplemented anyway as is). They would no longer pass isinstance(), but they would still be mostly drop-in replacements.
  2. Use new (different) names signals that change types. For example QDoubleSlider.valueChanged would be QDoubleSlider.floatValueChanged or something like that, rangeChanged would have to be floatRangeChanged... and QRangeSlider.valueChanged might be QRangeSlider.valuesChanged, etc... This breaks a lot of existing code, but might be cleaner to implement.

this doesn't have to happen soon, but it's a bit of a ticking time bomb. @Czaki and @jni, just a heads up if you have any opinions.

@tlambert03 tlambert03 added the bug Something isn't working label Jun 3, 2024
@jni
Copy link

jni commented Jun 4, 2024

I don't have a strong opinion, but generally favour more elegant implementations even if it breaks existing code. This is especially true while the number of users is (relatively) small. So that sounds like option 2 might be better. I'd be happy to simul-change things on the napari side when the time comes.

@Czaki
Copy link
Contributor

Czaki commented Jun 4, 2024

Hm. I think that @jni underestimate how it could break napari. I prefer option one. In addition, superqt could export the own abstract type that could be used in isinstance calls.

@tlambert03
Copy link
Member Author

thanks both. yeah I started looking into it. Option number one is going to be a lot of work (basically a complete reimplementation of QSlider from scratch), but it's probably the only way to preserve the same signal names. this will likely go on the backburner, but it's good to have an open issue for it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

3 participants