-
-
Notifications
You must be signed in to change notification settings - Fork 791
[Qt] ScrollContainer widget #3971
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
base: main
Are you sure you want to change the base?
[Qt] ScrollContainer widget #3971
Conversation
Tests use measurements for running on Mac, may fail on linux.
johnzhou721
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DISCLAIMER: I am not on the official core team and am permissionless. The things flagged represents my own opinion and not that of BeeWare or any of its other contributors.
From a quick look at the PR, I noticed some things about layout that may be a bit off. I've noted them and provided hints (pun intended) on fixes. Thanks for the contributions by the way, it's definitely exciting to see something I started grow this fast.
| self.native.setHorizontalScrollBarPolicy( | ||
| Qt.ScrollBarPolicy.ScrollBarAsNeeded | ||
| if value | ||
| else Qt.ScrollBarPolicy.ScrollBarAlwaysOff, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to check: Does ScrollBarAlwaysOff disable trackpad scrolling as well? Because the current thing looks like it's doing something with ScrollBars, not scorlling in general (from a quick look at the code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See bottom content_refrehsed comment for a hint on this.
| self._default_background_color = TRANSPARENT | ||
|
|
||
| self.document_container = Container( | ||
| on_refresh=self.content_refreshed, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK so ScrollContainer is kind of an awkward widget; if users put things that has things in the ScrollContainer that would expand to fill a parent, then the widget should fill the viewport of the scroll container. In other words, a ScrollContainer is just like a regular container, but is scrollable when it overflows.
We'd have to use https://doc.qt.io/archives/qt-5.15/qabstractscrollarea.html#viewport here to get the viewport widget to use for sizing things, and pass it as the layout_native parameter -- that way the Container performs size meassurement with the viewport.
| on_refresh=self.content_refreshed, | ||
| ) | ||
| self.native.setWidget(self.document_container.native) | ||
| self.native.setWidgetResizable(True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 things here -- 1 you have a setWidgetResizable(False) on top... so one of these is extra.
Another is that widgetResizable might not be good to turn on... I don't get an immediate impression on how it works from the docs. If it's going to result in slightly weird behavior, might as well turn this off.
Sorry if I missed anything.
| def content_refreshed(self, container): | ||
| min_width = self.interface.content.layout.min_width | ||
| min_height = self.interface.content.layout.min_height | ||
| self.document_container.native.setMinimumSize(min_width, min_height) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is the right thing to enforce here... I mean if you have widgets that relies on filling the ScrollContainer's size (e.g. if you have flex inside a ScrollContainer ocntents), then using the minimum width and height would make it too small and it would not match the actual layout that is performed.
I believe the correct alternative here is to set the size of the document container explicitly to the maximum size that is laid out in the child container and the size of the viewport. If we're not scrollable, though, we should clamp the size of the document container to the size of the viewport itself, so it's not just scrollbar hidden but actually not scrollable by trackpad either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically, using self.interface.content.layout.width for laid out width, and self.interface.content.layout.height, and using the Qt APIs for the native viewport width and height. Start with the native viewport width and height; if horizontally scrollable, max the width with the laid out width; if vertically scrolalble, max the height with the laid out height; then use the width+height for document container.
| size = self.native.viewportSizeHint() | ||
| self.interface.intrinsic.width = at_least( | ||
| max(size.width(), self.interface._MIN_WIDTH) | ||
| ) | ||
| self.interface.intrinsic.height = at_least( | ||
| max(size.height(), self.interface._MIN_HEIGHT) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
viewportSizeHint doesn't seem to include scrollbars... but on the other hand, rehinting is only based on MIN_WIDTH and MIN_HEIGHT on most other backends anyways, and since scrollbars shouldn't be ridiculously thick, I suggest only hinting based on _MIN_WIDTH and _MIN_HEIGHT by interface.
I ran into issues around layout when trying to implement the SplitWidget. I assume that Travertino is basically handling all the layout and we aren't relying on Qt's layout at all; is there documentation (or a pointer to relevant code) for how layout works under the covers. I think I need to wrap my head around it. |
|
@corranwebster Yes, your instincts are correct; however, Toga's Pack layout system handles layout, but, yes, it's based on Travertino's ways of specifying abstract layout. The relevant entry points are that [INTERFACE WIDGET].refresh() will redo the layout of [INTERFACE WIDGET]; however, it will actually start from the root container and refresh downwards. The Container is defined in toga_qt/container.py; the layouts are performed by way of set_bounds() and refresh() in Widget of toga_qt/widgets/base.py. The set_bounds method will set the geometry of the widget; it is called as part of the layout. [IMPL LAYER WIDGET].refresh() will rehint the widget, which provides Toga with the necessary information to perform the layout (such as intrinsic size). In this case -- you'd need to call sub_container.content.interface.refresh() (the .interface part is important; else it'll only rehint the current widget, not call a refresh) for each sub_container you instantiate when the size of a split subcontainer changes. Then, about applying split proportion -- do something similar to the other backends; the GTK one is a good reference -- use the width and height, and do the math and set it whenever the width/height of the widget changes (hint: set_bounds provides that, since that's when the layout for the root container already ran to this spot and you can know the actual pixel value of the split.) Does this help? If not, feel free to ask for clarifications. EDIT -- though, I think it's better to discuss this at the main backfilling ticket. |
Adds the ScrollContainer widget to the Qt backend.
For testing we add a
frame-insetfudge factor which is intended to represent the difference between the internal area of the QScrollArea widget and the internal area of the QWidget that is being scrolled (appears to be ~1 pixel on all sides). An alternative would have been to increase the approximate match factors in the width/height from ±1 to ±2 or ±3.This is split out from #3966 for ease of review.
Ref #3914.
To Do:
Visual comparison of layout with other backends.Contents are not expanding when container expands.PR Checklist: