[widget Audit] toga.ScrollContainer #1969

freakboy3742 · 2023-06-08T08:11:20Z

Audit of ScrollContainer.

Includes a Cocoa and iOS version of #1794. This allows us to remove the special case handling of the root content object in any given layout - constraints are used for all Toga widgets, rather than "all widgets except the root widget", and we now know that all widgets that are in a layout have a container.

It also fixes some issues with ScrollContainer specifically, as we need to decouple the size of the document being scrolled from the layout:

The size of the document being scrolled should not alter the layout of the scroll container. For Cocoa/iOS, this means the document must be at least the size of the content window it is being displayed in
If the document is larger than the scroll container in an axis that doesn't allow scrolling, Cocoa and iOS will turn on scrolling in that axis.

Some changes that follow on from this:

The dummy backend has been modified to be more "Container"-like than "viewport"-like.
When adding/removing content from a layout, the widget's refresh needs to be updated, not the window. If the content is inside a scroll container, it should only impact the layout inside the scroll container, not the overall window.
refresh_sublayouts isn't needed for scroll container; and I suspect it won't be needed for OptionContainer or SplitContainer either.
iOS ViewController handling has been moved to be a container property. This lays the groundwork for introducing other controllers, such as the UITabController for OptionView, or a UINavigationController for navigating stacks of content.

Issues fixed:

Fixes Winforms ScrollContainer not displaying all content, or responding to new content #1983.
Fixes Windows ScrollContainer not responding to changes in inner content #1915.
- These two are the same problem, which is Windows specific and should validated by the current testbed.
Fixes Dynamic changes to ScrollContainer content not applying #1655.
- Windows specific, but should be tested by the current testbed
Fixes Contents are laid out before container size is known #1624.
- Not sure if the testbed will fully exercise this, due to the interaction with window resizing; AFAICT it doesn't manifest on iOS, Cocoa or GTK.
Fixes Windows ScrollView behavior isn't linked to scrollbar visibility #1209.
- Windows specific; I presume the testbed will verify this.
Fixes Scroll Container height on iOS #768.
- iOS specific as reported. I suspect it was actually an issue with Pack (and/or root container handling) that has been resolved along the way, but it's definitely fixed now.
Fixes Canvas does not render in ScrollContainer #579.
- This raises an interesting question for ScrollContainer: If I put a widget with an intrinsic size of 0x0 into a scroll container that can scroll in both axes, how big is the widget? For that matter - if I put a layout that is flexible in a given axis into a scrollable container, what's the size of the layout? The effect of the changes in this PR are that the widget will have a layout size that is at least the size of the visible area of the scroll container, which seems like a reasonable interpretation; however, we should add some testbed validation that flexible layouts will stretch to at least the available width/height.

Audit checklist

…croll offset.

freakboy3742 · 2023-07-06T03:26:42Z

I've just (re)audited the ScrollContainer issues; I've presumptively put them all on the ticket as "fixed"; the current status of each AFAICT is:

[Merged into the Fixes list above, except for the following. -- @mhsmith]

Error AttributeError: 'NoneType' object has no attribute '_impl' #1473
- Mentions ScrollContainer along the way, but the ScrollContainer issue is (a) unrelated to the originally reported issue and (b) was fixed in Refs #1473 - Check for content before updating content size. #1557. The remaining issue is acutally with DetailedList on iOS, and I've addressed it in [widget Audit] toga.DetailedList #2025.
Windows: ScrollContainer loses position when window is resized #1606.
- Windows specific; I've verified it doesn't happen on macOS and GTK, but there's no testbed verification
- Only partly fixed by this PR: see comments in the issue. -- @mhsmith

mhsmith · 2023-07-10T11:47:46Z

Includes a Cocoa and iOS version of #1794. This allows us to remove the special case handling of the root content object in any given layout - constraints are used for all Toga widgets, rather than "all widgets except the root widget", and we now know that all widgets that are in a layout have a container.

The same has now been done for Winforms and Android.

mhsmith · 2023-07-10T18:30:12Z

core/src/toga/widgets/scrollcontainer.py

        self._content = widget
-        self.refresh()
+        if widget:
+            widget.refresh()


My reasoning for this change is that setting the content should not affect the size of self (the container), but it will affect the size of widget (the content).

Is this correct? And if so, is there anything which might incorrectly be depending on the previous behavior?

Agreed that this seems correct. I can't think of any behavior that would depending on this - it's just an easy optimisation, AFAICT.

On Cocoa/iOS, calling refresh on the widget will cause set_bounds() to be invoked on the widget, which will trigger a refresh on the content anyway; this optimisation shortcuts the unnecessary changes to the container, and goes directly to the content. When the content layout completes (or any other content change is applied), the container scroll bounds are updated by the on_refresh callback on the container.

On GTK, all you're changing is which widget is marked as dirty; once the widget is marked as dirty, it will get a layout in the next render pass. Again, all this change does is optimise how much is marked as dirty.

mhsmith · 2023-07-10T18:36:23Z

Winforms is now passing the testbed, but I still need to verify that everything in the Fixes list above is actually fixed.

freakboy3742

A couple of questions inline; the only big issue I see is the composition vs inheritance thing for Window and ScrollContainer. Inheritance clearly works for these two cases - although the approach won't work for OptionContainer and SplitContainer (because there's multiple sub-containers).

freakboy3742 · 2023-07-10T22:56:48Z

android/src/toga_android/widgets/base.py

+    def viewport(self):
+        return self._container
+
+    def scale_in(self, value):


A quick note for future explorers on which direction is "in" and "out" would be helpful here.

freakboy3742 · 2023-07-10T23:03:37Z

android/src/toga_android/window.py


-
-class Window:
+class Window(Container):


Is there a particular reason for using "is-A" rather than "has-A" as the composition for Window and Container? It's an interesting discrepancy between the iOS/Cocoa/GTK world and the Android one; I'm wondering what the source of that difference is.

I agree the benefit of inheritance turned out to be fairly small. It's mainly just providing a single implementation of the set_content method, reducing the number of distinct objects you need to keep track of, and unifying the meaning of the word "container".

As discussed, my preference is to keep it the way it is for this PR, and revisit it in the SplitContainer PR where the container/content relationship will no longer be 1-1.

freakboy3742 · 2023-07-10T23:13:47Z

winforms/src/toga_winforms/widgets/base.py

-        return self._viewport
+        return self._container
+
+    def scale_in(self, value):


As with android - some documentation for future explorers would be helpful

freakboy3742 · 2023-07-10T23:19:00Z

android/src/toga_android/widgets/scrollcontainer.py


+class ScrollContainer(Widget, Container):


Similar thing here with composition vs inheritance for the ScrollContainer.

freakboy3742 · 2023-07-10T23:21:58Z

winforms/src/toga_winforms/widgets/scrollcontainer.py

+        # at the top of this file).
+        def apply_insets():
+            need_scrollbar = False
+            if self.vertical and layout.height > self.height:


Should this comparison be with self.height, or inset_height?

self.height is the correct value in both cases:

In the first call to apply_insets, self.height == full_height, because you're checking whether the initial layout needs a vertical scroll bar.

In the second call, self.height may have been set to inset_height if a horizontal scroll bar was added by the first call, in which case we now need to check whether the second layout fits in that smaller space.

freakboy3742 · 2023-07-10T23:25:02Z

winforms/src/toga_winforms/window.py

+            vertical_shift += self.native.MainMenuStrip.Height
+
+        self.native_content.Location = Point(0, vertical_shift)
+        super().resize_content(


This is the essentially the reason I'm questioning the composition vs inheritance approach - composition gives a clear distinction between the process of resizing the window, and the fact this requires resizing the container.

mhsmith · 2023-07-11T18:09:18Z

Confirmed that all the Windows issues in the Fixes list are actually fixed, except for #1606, which I've added some notes to.

… layer

mhsmith · 2023-07-11T18:21:56Z

Everything looks fine now, except for a couple of uncovered branches in Cocoa and iOS which I'm not sure how to fix:

Users/runner/work/toga/toga/testbed/build/testbed/macos/app/Toga Testbed.app/Contents/Resources/app_packages/toga_cocoa/widgets/scrollcontainer.py 77 0 18 1 98.9% 31->exit

Users/runner/Library/Developer/CoreSimulator/Devices/72A177E7-6329-4DF7-ACCA-085F1FFB5AD6/data/Containers/Bundle/Application/ED74A15B-E796-4174-8E6E-16508E070879/Toga Testbed.app/app_packages/toga_iOS/widgets/scrollcontainer.py 76 0 20 1 99.0% 19->exit

freakboy3742 · 2023-07-12T03:10:51Z

Testbed test case added for the 2 missing lines. It's the edge case of a ScrollContainer with no content having it's layout updated. Looks like GTK, Windows, and Android all pass that test case without any additional changes.

freakboy3742 added 2 commits June 9, 2023 08:49

Update docs for ScrollContainer

40a7dd0

Update core API for ScrollContainer.

e256720

freakboy3742 force-pushed the audit-scrollcontainer branch from a860811 to e256720 Compare June 9, 2023 01:26

freakboy3742 added 4 commits June 9, 2023 09:30

Add changenote.

6705fe9

Initial fixes for Cocoa, plus cocoa test probe for scrollcontainer.

e06281d

Tweaked scrollcontainer example.

d72cfe5

Misc debug cleanups.

04619a9

freakboy3742 force-pushed the audit-scrollcontainer branch 3 times, most recently from 5b5b21d to 8f56f27 Compare June 12, 2023 04:48

Cocoa ScrollContainer at 100%, including container rework.

b89f14a

freakboy3742 force-pushed the audit-scrollcontainer branch 2 times, most recently from afb0600 to ba3a44f Compare June 12, 2023 06:29

Correct CI failures by using frame, rather than contentView for the s…

73edde4

…croll offset.

freakboy3742 force-pushed the audit-scrollcontainer branch from ba3a44f to 73edde4 Compare June 12, 2023 06:42

freakboy3742 added 7 commits June 13, 2023 08:32

GTK ScrollContainer to 100%.

73e1d30

Add a content handler to the cocoa container impl, matching GTK.

bea23c1

Refactor iOS to remove viewports.

ae3aa13

Initial working implementation of iOS ScrollContainer.

3f34595

Normalize some implementation details between iOS and Cocoa.

e317601

iOS ScrollContainer to 100% coverage.

883f8fb

Updated core tests, cocoa and GTK to match iOS implementation.

ceb26be

freakboy3742 assigned mhsmith Jun 13, 2023

freakboy3742 marked this pull request as ready for review June 13, 2023 07:30

Remove unneeded dummy class.

dac11fb

freakboy3742 mentioned this pull request Jun 14, 2023

Winforms ScrollContainer not displaying all content, or responding to new content #1983

Closed

freakboy3742 added 3 commits June 14, 2023 10:57

Minor docs cleanups.

20ca03e

Corrections to the handling of containers in the Dummy backend.

dee3253

Merge branch 'main' into audit-scrollcontainer

78d521a

freakboy3742 mentioned this pull request Jun 18, 2023

[widget Audit] toga.SplitContainer #1984

Merged

11 tasks

freakboy3742 mentioned this pull request Jul 6, 2023

Windows ScrollContainer not responding to changes in inner content #1915

Closed

Refactor Winforms container/viewport mechanism

ee52a10

freakboy3742 mentioned this pull request Jul 7, 2023

Gtk4 migration #1978

Draft

42 tasks

mhsmith added 2 commits July 8, 2023 13:53

Fix "backend" tests on Winforms

b6a470b

Fix ImageView tests on Winforms with positive scale factor

07c4e37

mhsmith added 3 commits July 10, 2023 12:51

Miscellaneous cleanups

0d21010

Initial Winforms implementation (not working)

7bb9766

Winforms to 100%

2004cb8

mhsmith reviewed Jul 10, 2023

View reviewed changes

freakboy3742 commented Jul 10, 2023

View reviewed changes

mhsmith mentioned this pull request Jul 11, 2023

Windows: ScrollContainer loses position when window is resized #1606

Open

Add comments explaining scale_in and scale_out

6f6516e

Remove redundant checks in GTK which are now covered by the interface…

1bf22d1

… layer

freakboy3742 added 2 commits July 12, 2023 10:48

Cleaned up 2 stray uncovered lines.

096ac1f

Add a test for performing a layout with no content.

0e1302a

freakboy3742 requested a review from mhsmith July 12, 2023 03:56

mhsmith approved these changes Jul 12, 2023

View reviewed changes

mhsmith merged commit f7dc1c8 into beeware:main Jul 12, 2023
40 checks passed

freakboy3742 deleted the audit-scrollcontainer branch July 12, 2023 23:12

freakboy3742 mentioned this pull request Jul 19, 2023

Crash when displaying InfoDialog on iOS #2039

Closed

Cheukting mentioned this pull request Jul 22, 2023

Fixing dialog clashing on iOS bug (#2039) #2041

Merged

4 tasks

freakboy3742 mentioned this pull request Jul 23, 2023

Scroll container not working at all #2048

Closed

freakboy3742 mentioned this pull request Aug 4, 2023

Make upstream window and app references weak to avoid reference cycles #2061

Closed

4 tasks

samschott mentioned this pull request Aug 4, 2023

[widget Audit] toga.Window #2058

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[widget Audit] toga.ScrollContainer #1969

[widget Audit] toga.ScrollContainer #1969

freakboy3742 commented Jun 8, 2023 •

edited by mhsmith

freakboy3742 commented Jul 6, 2023 •

edited by mhsmith

mhsmith commented Jul 10, 2023

mhsmith Jul 10, 2023

freakboy3742 Jul 10, 2023

mhsmith commented Jul 10, 2023

freakboy3742 left a comment

freakboy3742 Jul 10, 2023

mhsmith Jul 11, 2023

freakboy3742 Jul 10, 2023

mhsmith Jul 11, 2023

freakboy3742 Jul 10, 2023

freakboy3742 Jul 10, 2023

freakboy3742 Jul 10, 2023

mhsmith Jul 11, 2023

freakboy3742 Jul 10, 2023

mhsmith commented Jul 11, 2023

mhsmith commented Jul 11, 2023

freakboy3742 commented Jul 12, 2023



		class Window:
		class Window(Container):

[widget Audit] toga.ScrollContainer #1969

[widget Audit] toga.ScrollContainer #1969

Conversation

freakboy3742 commented Jun 8, 2023 • edited by mhsmith

Audit checklist

freakboy3742 commented Jul 6, 2023 • edited by mhsmith

mhsmith commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsmith commented Jul 10, 2023

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsmith commented Jul 11, 2023

mhsmith commented Jul 11, 2023

freakboy3742 commented Jul 12, 2023

freakboy3742 commented Jun 8, 2023 •

edited by mhsmith

freakboy3742 commented Jul 6, 2023 •

edited by mhsmith