RangeSlider

[brisvag]

Attempt at adding RangeSlider to magicgui through superqt.

See also #192.

Currently I got the backed connection to work with the magic widget. What's missing is everything magic :)

I'm opening the PR early to ask a question: currently moving the slider fires a million errors such as:

~/git/magicgui/magicgui/backends/_qtpy/widgets.py in _on_slider_change(self, value)
    548
    549     def _on_slider_change(self, value):
--> 550         self._readout_widget.setValue(self._post_get_hook(value))
    551
    552     def _on_readout_change(self):

TypeError: setValue(self, int): argument 1 has unexpected type 'tuple'

I understand the problem, but I can't see how to solve it from the surrounding code. I guess we need two readouts somehow?

[tlambert03]

awesome!! thank for taking the initiative on this! lemme take a look and give you next steps

[tlambert03]

Ok, so, the ultimate limitation here is that we don't have a protocol for something like this (a multi-numeric value), however, there's an omission from the typing in magicgui that should have prevented this (at least in type checking)

magicgui works with a pretty strictly typed protocol between the magicgui widget, and the backend widget. Backend widgets all have to implement a specific magicgui protocol. For example, backends._qtpy.widgets.SpinBox and backends._qtpy.widgets.Slider both implement the widgets._protocols.RangedWidgetProtocol protocol... which, among other things, means that they implement the following methods (any types!)

class RangedWidgetProtocol(ValueWidgetProtocol, Protocol):
    """Value widget that supports numbers within a provided min/max range."""
    def _mgui_get_min(self) -> float: ...
    def _mgui_set_min(self, value: ... float) -> None: ...
    def _mgui_get_max(self) -> float: ...
    def _mgui_set_max(self, value: ... float) -> None: ...
    def _mgui_get_step(self) -> float: ...
    def _mgui_set_step(self, value: ... float) -> None: ...
    # from ValueWidgetProtocol
    def _mgui_get_value(self) -> Any: ...
    def _mgui_set_value(self, value: Any) -> None: ...

note there that the ValueWidgetProtocol doesn't make any assumptions about the type of the value, but the RangedWidgetProtocol assumes that value is going to be numeric

one big problem comes in these lines, where I omitted the value annotation:

https://github.com/napari/magicgui/blob/9e3833a55546940f1fc98926c74e84c6ca45728f/magicgui/widgets/_bases/ranged_widget.py#L54-L57

So, i think ideally we'd create intermediate protocols there, that type value as either float or Tuple[float, ...] (where, from a typing perspective, float also works for int)

That said, if you'd like to just get it "working" for now and leave the rigorous typing for later, we need to:

1. catch the case of value being a sequence of floats in the RangedWidget.value.setter, and deal with that
2. deal with the fact that magicgui is not yet using superqt in any way (not that I don't want to, it just isn't yet)... so it has also implemented Slider as a compound widget with a slider (Slider._qwidget) and readout spinbox (Slider._readout_widget). However, superqt comes with that already, so, for better or worse, you don't really want to use _qtpy.widgets.Slider as your base anymore
3. Even if you did use (Slider), there's an issue with your RangeSlider implementation. note that:

class RangeSlider(Slider):
    _qwidget: QRangeSlider
    _readout: None

is not the same as this (note equals vs colon)

class RangeSlider(Slider):
    _qwidget = QRangeSlider
    _readout = None

your colons are just talking to mypy, by the superclass actually uses those values to instantiate that type of widget. and if you did _readout = None, you'd get an error when it tries to instantiate it. This all just re-emphasizes that we need a different backend widget that isn't just a subclass of slider.

[tlambert03]

if you are enjoying digging into this, I'll leave you to it and help out whenever you want. If you find that extra legwork annoying, let me know and I can stub out some of the framework

[tlambert03]

btw, let's add this to examples and aim to get it to work:

from typing import Tuple
from magicgui import magicgui

@magicgui(range_value=dict(widget_type="RangeSlider"))
def func(range_value: Tuple[int, int] = (20, 80)): ...

func.show(run=True)

[brisvag]

Thanks for the explanations! Played around with it a bit, and I noticed that superqt's labeled sliders don't have an editable readout. What do you think about that? It seems to me that maybe it's better to simply reimplement a rangeslider here instead of using superqt, if we want that?

[tlambert03]

I noticed that superqt's labeled sliders don't have an editable readout.

They do also have an editable spin box. What made you say that?

maybe it's better to simply reimplement a rangeslider here instead of using superqt

Nooo, I'm not sure if you looked at the implementation over at superqt, but is a lot. We definitely don't want to reimplement that.

[brisvag]

They do also have an editable spin box. What made you say that?

Woops, my bad. It just looked like a label, since it had no "box" around it and no arrows to click up and down. I didn't even try to edit it :P Then it's a lot less work than I thought!

[brisvag]

The example acutally spawns the right slider! Errors happen, and things don't work, but it's something :D

(Actually, to work it's using a small change I did to superqt, where I added setLabelVisible to LabeledSliders. Not sure if that's a reasonable approach, but I wanted to get it working)

[brisvag]

Updated to use napari/superqt#59 (needed for this to work). The example currenlty segfaults due to events firing with the wrong types:

TypeError: rangeChanged(self, int, int).emit(): argument 1 has unexpected type 'float'

Searching the repo for rangeChanged yields no results, I'm not sure where to find the code responsible for this. Pointers?

[tlambert03]

is there more to that stack trace? rangeChanged is from the QSlider itself (https://doc.qt.io/qt-5/qabstractslider.html) ... we can override the type if we need to, but we need to know exactly where the emitting is occurring

[brisvag]

Only with ipython I get a bit more, but it's not relevant:

Error in sys.excepthook:
Traceback (most recent call last):
  File "/usr/lib/python3.10/site-packages/IPython/core/interactiveshell.py", line 1934, in showtraceback
    stb = value._render_traceback_()
AttributeError: 'TypeError' object has no attribute '_render_traceback_'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.10/site-packages/IPython/core/interactiveshell.py", line 1936, in showtraceback
    stb = self.InteractiveTB.structured_traceback(etype,
  File "/usr/lib/python3.10/site-packages/IPython/core/ultratb.py", line 1105, in structured_traceback
    return FormattedTB.structured_traceback(
  File "/usr/lib/python3.10/site-packages/IPython/core/ultratb.py", line 999, in structured_traceback
    return VerboseTB.structured_traceback(
  File "/usr/lib/python3.10/site-packages/IPython/core/ultratb.py", line 851, in structured_traceback
    assert etb is not None
AssertionError

Original exception was:
TypeError: rangeChanged(self, int, int).emit(): argument 1 has unexpected type 'float'

[tlambert03]

k, lemme pull

[tlambert03]

hmm... frustratingly, it seems to be only pyqt5. pyside2 is working (which is a good sign! :) This is might be related to some changes I needed to make to fix https://github.com/napari/superqt/issues/48 ... a bug in PySide6 that broke a lot of stuff for us. I merged https://github.com/napari/superqt/pull/51 which fixed that issue for pyside2 (but may have planted the seeds for this issue here on pyqt5?) ... it's the first place my brain goes here. need to keep digging

[brisvag]

Well, pyside2 working at least means we're on the right track! Cool, let me know what you unearth :)

[imagesc-bot]

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/using-multiple-buttons-in-napari-magic-gui/65241/2

[brisvag]

I fixed some tests, what's left is a bit incomprehensible to me (and some of the failures are still related to the pyqt problem mentioned above).