[iOS] Factor container inset logic out into Window; remove use of deprecated APIs; detect window resize change on iPadOS; fix top offset calculations#4043
Conversation
|
Not ready for review again -- I just thought of 2 things:
|
|
I'm aware that the core team may not respond until up to 8 days. Sidenote: Bumped timeout for WinForms CI further as well, as those are more often and very costly to us. |
freakboy3742
left a comment
There was a problem hiding this comment.
The broad strokes of this make sense - moving to a less "hard coded" approach to offset handling makes sense conceptually.
However, when it's being reported as a "bug fix", it's important to provide an example of the bug it's fixing. There's no ticket associated with this; there's no reproduction examples; there's no new test case; there's not even a before/after screenshot of what is being changed. Therefore, it's impossible to understand what this is fixing, and under what conditions.
|
@freakboy3742 Apologies for examples not being provided. Example 1First example, which is probably trivial. With the main version of Toga:
In the layout debugger in Xcode for the build of the app -- notice how the UINavigationBar overlaps the top of the purple content:
With this branch of Toga:
Xcode layout debugger -- no longer overlaps:
Example 2Changing MainWindow to Window from the first example: Build it, and then go to the app in Xcode and remove "Requires Full Screen". Then run it on an iPad: Note that it automatically adjusts to the status bar correctly when moving the window, and the window is refreshed with resizes (as demonstrated by the small white margin at the bottom which is rounding error.) |
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
freakboy3742
left a comment
There was a problem hiding this comment.
I've pushed a couple of minor cleanups (details in the review suggestions); but otherwise, this looks like a good set of changes, removing some semi-hard-coded iOS behavior.
I'm guessing there's now a follow up to relax the "requires Full Screen" plist setting in iOS projects?
| f"{(self.container.width, self.container.height)}" | ||
| ) | ||
|
|
||
| def check_for_resize(self, container): |
There was a problem hiding this comment.
A minor thing, but check_for_resize() is a bit of an odd name; it says what is being checked, but not what will happen as a result. Something like notify_on_resize() might make more sense.
| super().__init__(content=content, on_refresh=on_refresh) | ||
| self.native = UIView.alloc().init() | ||
| super().__init__( | ||
| content=content, on_refresh=on_refresh, on_native_layout=on_native_layout |
There was a problem hiding this comment.
See notes on previous reviews about the "all args on one separate line" format.
|
@freakboy3742 Yes, there is such a followup, but later. Not quite yet, we'd need to integrate with the UIScene lifecycle and get UISceneSizeRestrictions usage in order to get minimum sizing enforced correctly, and only emit warnings about layout size exceeding window size if a UISceneSizeRestrictions object is unavailable. However when it comes to doing the plist key removal I am quite clueless about getting those versions in sync with Toga, because we don't want an older Toga without Requires Full Screen. Could you please provide me with some guidance on how to manage this part? Also -- this is only relevant on iPadOS. Should we advertise "initial iPadOS support' in release notes? Also FYI: #3956 tracks API deprecations for iOS 26+. I've posted a comment there saying that this part is resolved. |
I can think of two possibilities: The first option is to add the option to the template, defaulting to the historical The second option would be to add the option to the template, defaulting to the new The second option is the better long term; but requires some action on the part of maintainers. However, given the maturity of Toga on iOS, I think I can live with that.
Yes - but not until we've resolved the UIScene issues that would prevent non-full screen support from working as expected. |




This PR may look a lot, so I apologize if I overwhelm anybody.
The most important change in here is that I've gotten rid of top_offset as a computed property on Container, and I instead had 4 instance variables for top, left, bottom, and right insets respectively. The window, or whatever object starts the new containers, manages the calculations of these insets.
This is because there's lots of the different ways the insets can be calculated by, especially when the inset is introduced into widgets like OptionContainer (which is where we want to exclude tab bars, which can come in different forms and requires heuristics to detect which insets to exclude, on iPadOS.) or ScrollContainer. This may not be a lot right now, but it'll come into play significantly when we have more complex things like Sidebars as well -- it's just easier for whoever is utilizing the container to compute the insets (instead of hardcoding formulae in Container for specific cases). This is accomplished by adding a native relayout / safe area changed handler to Container, which covers all sitautions in which our computed insets may change.
I've also removed statusBarOrientationDidChange -- that's been deprecated since iOS 13 (I know... it's been 6 years now. I'm wondering if Rubicon-Objc should have an utiliity to parse headers and warn about deprecated APIs... but I'll save that discussion for later.). It's also not the appropriate phase of the lifecycle as demonstrated by the need to do asyncio.call_soon. So -- I've instead monitored resizing by using the size change for Container instead (another great reason for a native relayout handler so we can detcet changes in container size and call the on_resize / layout handler appropriately).
With all that being said -- for MainWindow, iOS seems to place navigation bars in mysterious ways, and our current statusBar + navigation bar handling does not work properly (content actually overlaps with the navigation bar!). So I've changed it to computing based on navigation bar's frame instead, and by experimentation, it seems that the change in the navigation bar's frame is always covered by the native relayout signals we listened to earlier (specifics in code).
But for a plain Window, on iPadOS the status bar height may not actually be included in the window! (when the window is not full-screen and not at the top -- UIRequiresFullscreen has to be removed from the app template for this to happen, but I figured since that key's getting deprecated in a year might as well make hte change right now) So we use a heuristic on whether to include it or not -- if we see that the top status bar is influencing the window's safe area, then the top status bar is probably in the window (since the window's rounded corners is smaller than the status bar height), and then we exclude it.
Draft for now to run CI.
PR Checklist: