Add scrollable widgets

[dstansby]

Fixes https://github.com/napari/magicgui/issues/380. With

from magicgui import magicgui

@magicgui
def add(my_number: int, some_word: str = 'hello', maybe=True):
    pass

add.scrollable = True
add.show(run=True)

[codecov]

Codecov Report

Merging #388 (ebe0cee) into main (b17aeb0) will decrease coverage by 0.02%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main     #388      +/-   ##
==========================================
- Coverage   88.74%   88.71%   -0.03%     
==========================================
  Files          30       30              
  Lines        3838     3864      +26     
==========================================
+ Hits         3406     3428      +22     
- Misses        432      436       +4     

Impacted Files	Coverage Δ
magicgui/_magicgui.py	96.29% <ø> (ø)
magicgui/widgets/_function_gui.py	94.06% <ø> (ø)
magicgui/backends/_qtpy/widgets.py	86.67% <87.09%> (-0.07%)	⬇️
magicgui/widgets/_bases/container_widget.py	92.85% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b17aeb0...ebe0cee. Read the comment docs.

[tlambert03]

thanks for taking this on @dstansby! This is a great start.

couple first impressions:

1. I like the external API of widget.scrollable = True/False ... but I think it's going to be more difficult to actually implement that on the Qt Side. For example, in your example try adding add.scrollable = False right after setting it to True, and you'll get an error. Everything needs to be reversible.
2. I think it would be nice to be a little more aware of the size and stretch of the underlying widget. In an "ideal" world, the layout would continue to stretch with the widget as it gets bigger, and only once it reaches a minimum threshold would a scrollbar appear. The sizehints and layouts bit here might be of help there... but need to look a little more
3. would be nice if one could make just one of horizontal vertical scrollable (I suspect that main use case here will be wanting a vertical scroll bar for too-long widgets, while maintaining the nice horizontal layout management, right?)

[dstansby]

1.  Everything needs to be reversible.

Fixed, you can now set/unset scrollable and it works as intended.

2. IIn an "ideal" world, the layout would continue to stretch with the widget as it gets bigger

Fixed - this was just setting setWidgetResizable(True). Now the widget expands as before (or without scrollable on), but when the window is smaller than the minimum size scroll bars appear.

3. would be nice if one could make just one of horizontal vertical scrollable

I'll take a look into this.

Additional question I now have: I've tagged this on to the ContainerProtocol, but should I create a new ScrollableProtocol instead?

[tlambert03]

Additional question I now have: I've tagged this on to the ContainerProtocol, but should I create a new ScrollableProtocol instead?

I like it on the container protocol. happy to leave it there

[dstansby]

I've updated this so that:

• Scrolling is only enabled in the direction of the layout (e.g. horizontally for horizontal layout)
• Scrolling is disabled by deafult on Container, but enabled by default when creating a FunctionGui
• Aside from a scrollbar appearing when the layout is too big to fit in the window, resizing etc. behaves as before this PR
  There's a new .gif on the original post above.

I think that's a good place to be from an API and functionality point of view. I've added a sentence to the docs, is there anywhere else I should add docs? I've also added a really basic smoke test. Is there any other tests I should add?

[tlambert03]

test fail in mpl_widget does appear to be real... for better or worse, accessing widget.native.layout() has generally been a supported use case, and the native widget for Container has so far always had a layout(). I wonder whether it would be possible here, to swap the layout from self._qwidget to self._scroll, rather than transferring _qwidget itself to self._scroll? One way or another, native.layout() needs to point to the layout of the Container, rather than sometimes a layout, and sometimes None. (technically, one could use native.centralWidget().layout() here ... but we don't want them to have to swap depending on scrollability)

[dstansby]

Naïvely copying the layout onto the ScrollArea doesn't work, but overriding _mgui_get_native_widget() to always return the widget inside the ScrollArea does work. There's one more test that's failing, and after a bit of time trying to work out why I'm stumped, does anyone else have any insight?

    def test_reset_choice_recursion():
        """Test that reset_choices recursion works for multiple types of widgets."""
        x = 0
    
        def get_choices(widget):
            nonlocal x
            x += 1
            return list(range(x))
    
        @magicgui(c={"choices": get_choices})
        def f(c):
            pass
    
>       assert f.c.choices == (0,)
E       assert (0, 1) == (0,)
E         Left contains one more item: 1
E         Full diff:
E         - (0,)
E         + (0, 1)
E         ?    ++

tests/test_widgets.py:420: AssertionError

[dstansby]

I took a look into the failing test (test_reset_choice_recursion), and it was failing because the new code causes a call of reset_choices() when the gui is created:

/Users/dstansby/projects/napari/magicgui/magicgui/widgets/_bases/container_widget.py(226)scrollable()
-> self._widget._mgui_set_scrollable(value)
/Users/dstansby/projects/napari/magicgui/magicgui/backends/_qtpy/widgets.py(408)_mgui_set_scrollable()
-> self._scroll.setWidget(self._qwidget)
/Users/dstansby/projects/napari/magicgui/magicgui/backends/_qtpy/widgets.py(36)eventFilter()
-> self.parentChanged.emit()
/Users/dstansby/projects/napari/magicgui/magicgui/widgets/_bases/widget.py(348)_emit_parent()
-> self.parent_changed.emit(self.parent)
/Users/dstansby/projects/napari/magicgui/magicgui/widgets/_bases/container_widget.py(237)reset_choices()
-> widget.reset_choices()  # type: ignore
/Users/dstansby/projects/napari/magicgui/magicgui/widgets/_bases/categorical_widget.py(66)reset_choices()
-> self.choices = self._default_choices
/Users/dstansby/projects/napari/magicgui/magicgui/widgets/_bases/categorical_widget.py(114)choices()
-> _choices = choices(self)
> /Users/dstansby/projects/napari/magicgui/tests/test_widgets.py(414)get_choices()
-> x += 1

I think this is fine, so I've modified the test accordingly, and added a new explanatory comment. @tlambert03 I think this is now ready for review.

[tlambert03]

hmm, something weird is still happening. If I run python examples/widget_demo.py i get an empty widget on this branch.

[dstansby]

I don't have time to debug in the very near future, but it seems to be an issue when main_window=True.

[dstansby]

👍 , I fixed the MainWindow issue.

[tlambert03]

thanks @dstansby!

there are two things I'd like to do before merge, if it's ok with you:

1. like the layout parameter, scrollable should probably be passed through all the way from the @magicgui decorator
2. I think I'd prefer the default to be False for now, at least for this first release... so as not to suddenly change anyone's layout. That ok with you?

I can make/push these changes if you want. just want to get your opinion

[dstansby]

👍 makes sense - I think I've made those changes, and I checked it works fine by locally adding scrollable to a few of the examples.

[tlambert03]

thanks!! will merge and release this weekend

[tlambert03]

arg... more issues. I removed this line (which was overriding the False default for scrollable), and it exposed another issue. Looks like when scrollable is true, the parent_changed signal gets fired quite a few times, which is problematic for the (currently poorly implemented) choices behavior (see #306). I know it's now just an opt-in behavior, but I fear the actual swapping of self._qwidget in the underlying widget might cause unexpected/untested errors. I'm going to play with it for a bit. possibly making it settable only during construction.

[tlambert03]

ok, for now, I've removed scrollable mutability. It is what it is when you create the widget for the first time, and it can't be changed. This simplifies the swapping out of the underlying qwidget after construction, and should be a little easier to reason about. Is that ok with you?

If someone comes along and want to toggle scrollability after construction we can revisit it at that time. but this at least gets something in. sorry for the delay

[dstansby]

Thanks a lot for improving this! I'm 👍 to making the scrollable state immutable and set on creation.

[tlambert03]

awesome. Then LGTM! Thanks a lot for your patience and effort on this!