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

Make container automatically resize on orientation change #2161

Merged
merged 5 commits into from
Oct 23, 2023

Conversation

landrybr
Copy link
Contributor

This sets the autoResizingMask property of the container.native UIView so that all possible resizing options (i.e. margin, height, and width) are set.

On iOS14 and iOS15 the container.native UIView doesn't always resize properly on orientation change. This is not a problem on iOS16 and iOS17 for some reason. You can see this behavior on the positron-django example.

PR Checklist:

  • [x ] All new features have been tested
  • [x ] All new features have been documented
  • [x ] I have read the CONTRIBUTING.md file
  • [x ] I will abide by the code of conduct

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Well that's a doozy - nice work finding the fix. One suggested improvement inline relating to how the mask is specified.

@@ -65,6 +65,7 @@ def __init__(self, content=None, layout_native=None, on_refresh=None):
super().__init__(content=content, on_refresh=on_refresh)
self.native = UIView.alloc().init()
self.native.translatesAutoresizingMaskIntoConstraints = True
self.native.autoresizingMask = 63
Copy link
Member

Choose a reason for hiding this comment

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

What's the significance of this magic number? I presume it's the manifestation of a bunch of internal constants? If so, preference would be to preserve those constants in toga_iOS.libs and reference them here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, makes sense. Made the changes you suggested.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

One request for clarification, and a code style tweak inline.

UIViewAutoresizingFlexibleRightMargin = 1 << 2
UIViewAutoresizingFlexibleTopMargin = 1 << 3
UIViewAutoresizingFlexibleHeight = 1 << 4
UIViewAutoresizingFlexibleBottomMargin = 1 << 5
Copy link
Member

Choose a reason for hiding this comment

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

We've been leaning towards using Enums (e.g., UIViewContentMode above), rather than constants. There are some historical examples that use constants; these generally predate the introduction of Enum support in Rubicon.

Copy link
Contributor Author

@landrybr landrybr Oct 23, 2023

Choose a reason for hiding this comment

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

I used intEnum rather than Enum so that the | operator would work without calling .value. Also, note that I renamed UIViewAutoresizingNone to UIViewAutoresizing.NoAutoResizing since I couldn't assign to None. If you think we should handle that issue in another way, let me know.

| UIViewAutoresizingFlexibleRightMargin
| UIViewAutoresizingFlexibleTopMargin
| UIViewAutoresizingFlexibleHeight
| UIViewAutoresizingFlexibleBottomMargin
Copy link
Member

Choose a reason for hiding this comment

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

Have you got an example that demonstrates why Flexible(Top/Bottom/Left/Right)Margin is needed? Conceptually, it doesn't make sense to me that those should be required - the margins should always be 0 an fixed; and from my own testing, masking FlexibleWidth and FlexibleHeight seems to be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I didn't notice any issues with just FlexibleWidth and FlexibleHeight so I removed the other ones.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Awesome - thanks for putting up with all the nitpicks!

@freakboy3742 freakboy3742 merged commit 226f4d9 into beeware:main Oct 23, 2023
41 checks passed
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