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

[widget Audit] toga.SplitContainer #1984

Merged
merged 28 commits into from
Jul 17, 2023

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Jun 14, 2023

Audit of SplitContainer.

Builds on #1969 because of the changes to root containers on Cocoa.

Removes support for more than 2 panels of splitpanel content; Cocoa was the only platform that had any support for this.

Removes support for a configuration option that allowed a panel in a layout to have fixed size. This seemed exceedingly fragile in implementation, had some interesting interactions with Pack, and (AFAICT) wasn't implemented as documented on any platform.

There's one piece of platform behavior that is undefined: if you have a split container and change the direction of the split, the position of the split after the orientation change is undefined. I was borderline on whether this API should be removed entirely - changing split direction seemed like a niche use case.

Explicitly not implemented on Android and iOS because a split panel isn't a native concept on those platforms.

Audit checklist

  • Core tests ported to pytest
  • Core tests at 100% coverage
  • Core docstrings complete, and in Sphinx format
  • Documentation complete and consistently formatted
  • cocoa backend at 100% coverage
  • winforms backend at 100% coverage
  • gtk backend at 100% coverage
  • iOS backend at 100% coverage
  • android backend at 100% coverage
  • Widget support table updated
  • Widget Issue backlog reviewed

@freakboy3742 freakboy3742 force-pushed the audit-splitcontainer branch from fd8ae38 to 2a20fea Compare June 14, 2023 03:26
@freakboy3742 freakboy3742 force-pushed the audit-splitcontainer branch from 2a20fea to 305a3f2 Compare June 14, 2023 03:59
@mhsmith mhsmith changed the title [widget Audit] toga.SplitCcontainer [widget Audit] toga.SplitContainer Jun 14, 2023
@freakboy3742 freakboy3742 force-pushed the audit-splitcontainer branch from 10c2b55 to dbc6887 Compare June 19, 2023 05:07
@freakboy3742 freakboy3742 mentioned this pull request Jul 7, 2023
42 tasks
docs/reference/api/containers/splitcontainer.rst Outdated Show resolved Hide resolved
core/src/toga/widgets/splitcontainer.py Outdated Show resolved Hide resolved
docs/reference/api/containers/splitcontainer.rst Outdated Show resolved Hide resolved
core/src/toga/widgets/splitcontainer.py Outdated Show resolved Hide resolved
core/src/toga/widgets/splitcontainer.py Outdated Show resolved Hide resolved
core/src/toga/widgets/splitcontainer.py Outdated Show resolved Hide resolved
docs/reference/data/widgets_by_platform.csv Outdated Show resolved Hide resolved
docs/reference/api/containers/splitcontainer.rst Outdated Show resolved Hide resolved
docs/reference/api/containers/splitcontainer.rst Outdated Show resolved Hide resolved
docs/reference/api/index.rst Outdated Show resolved Hide resolved
@freakboy3742 freakboy3742 requested a review from mhsmith July 12, 2023 05:18
@freakboy3742 freakboy3742 marked this pull request as ready for review July 12, 2023 05:20
core/tests/widgets/test_splitcontainer.py Outdated Show resolved Hide resolved
Comment on lines +58 to +61
# The inner tuple's full type is tuple[Widget | None, float], but that would make
# the documentation unreadable.
@property
def content(self) -> tuple[Widget | None | tuple, Widget | None | tuple]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using a type alias along with autodoc_type_aliases, but that resulted in types becoming unlinked or even disappearing from the docs completely.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - I've haven't been 100% happy with how these non-trivial type annotations are rendered, but type aliases just seems to make it worse in every variation I've tried.

Comment on lines 101 to +104
def rehint(self):
self.interface.intrinsic.width = at_least(self.interface._MIN_WIDTH)
self.interface.intrinsic.height = at_least(self.interface._MIN_HEIGHT)
# This is a SWAG (scientific wild-ass guess). There doesn't appear to be
# an actual API to get the true size of the splitter. 10px seems enough.
SPLITTER_WIDTH = 10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether this code is properly verified by the testbed, and it isn't easy to test manually until minimum window size is working. So maybe any further work in this area should be deferred until #2020. However, I have a couple of comments:

  • This code is very similar between Cocoa and GTK, so it should be factored out into a function in the interface layer.

  • Also, I see that the GTK version calls recompute to update min_width and min_height, but I don't see any equivalent in Cocoa. Doesn't that mean that it'll always use the minimum size stored in set_content, which won't reflect any later internal changes to the content?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed there's a lot of similarity here; AFAIR, the discrepancy is because of GTK's "deferred" layout scheme. In Cocoa, by the time rehint() has been called, we know content has been added and Pack has been called to update minimum sizes; on GTK, Pack isn't invoked until the layout is actually underway, and it's done "top down" in the layout tree, so the layout of the children of the splitter haven't necessarily been computed yet.

I'm fairly sure you're also right that this will fall apart in the cases that #2020 fixes - i.e., the case where the root content of the split containers doesn't change, but there is a layout altering change in that content that changes the minimum.

There is a test for enforcing minimum sizes on containers, but it's probably not as thorough as it could be - it needs to:

  • explicitly check the case where the sub-container is > and < MIN in the given axis - the current test only validates "can't make the splitter smaller than the minimum"; and
  • check the case where the layout of the sub-container is altered without changing the root widget that is associated with the split container.

The second case can't really be checked until #2020; so I agree it makes sense to defer until then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've added a link to this discussion to #2020.

@@ -55,7 +56,7 @@ async def content3_probe(content3):

@pytest.fixture
async def widget(content1, content2):
xfail_on_platforms("android", "iOS")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I marked this as xfail because I can't think of any meaningful analog of a SplitContainer view on mobile, so SplitContainer is "not implemented on purpose", rather than "not implemented yet". Have you got something in mind for SplitContainer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that makes sense. I've changed it back to xfail and added some comments to explain the difference.

@freakboy3742 freakboy3742 merged commit ab06dc9 into beeware:main Jul 17, 2023
@freakboy3742 freakboy3742 deleted the audit-splitcontainer branch July 17, 2023 06:29
@mhsmith mhsmith mentioned this pull request Dec 2, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants