Conversation
...ndation-layout/src/skikoMain/kotlin/androidx/compose/foundation/layout/WindowInsets.skiko.kt
Show resolved
Hide resolved
...ndation-layout/src/skikoMain/kotlin/androidx/compose/foundation/layout/WindowInsets.skiko.kt
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/RootNodeOwner.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsetsManager.kt
Outdated
Show resolved
Hide resolved
ca52921 to
8f9cf93
Compare
...layout/src/notMobileMain/kotlin/androidx/compose/foundation/layout/WindowInsets.notMobile.kt
Show resolved
Hide resolved
...ndation-layout/src/skikoMain/kotlin/androidx/compose/foundation/layout/WindowInsets.skiko.kt
Outdated
Show resolved
Hide resolved
...ndation-layout/src/skikoMain/kotlin/androidx/compose/foundation/layout/WindowInsets.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/layout/WindowInsetsRulers.skiko.kt
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/RootNodeOwner.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformInsets.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsetsConfig.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsetsConfig.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsetsConfig.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Show resolved
Hide resolved
MatkovIvan
left a comment
There was a problem hiding this comment.
Despite the number of comments it looks pretty good (didn't test it localy yet)
compose/ui/ui/src/notMobileMain/kotlin/androidx/compose/ui/platform/PlatformInsets.notMobile.kt
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/CompositionLocals.skiko.kt
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsets.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsets.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsets.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsets.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsets.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoTest/kotlin/androidx/compose/testutils/PlatformInsets.kt
Show resolved
Hide resolved
compose/ui/ui/src/skikoTest/kotlin/androidx/compose/testutils/PlatformInsets.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoTest/kotlin/androidx/compose/testutils/PlatformInsets.kt
Outdated
Show resolved
Hide resolved
| ) | ||
| } | ||
|
|
||
| // TODO: Consider LTR/RTL |
There was a problem hiding this comment.
Q: The original TODO about LTR/RTL for NSDirectionalEdgeInsets was removed in this change. Is RTL now handled elsewhere? If so, it might be helpful to clarify that in a comment or doc reference 🙂
There was a problem hiding this comment.
Thanks for the question!
RTL isn't being handled there. This was an internal method of converting UIKit NSDirectionalEdgeInsets to Compose PlatformInsets and it had been unused since its adding, so it was removed.
fbaddbb to
fdf226c
Compare
MatkovIvan
left a comment
There was a problem hiding this comment.
LGTM. But I still need to do some local tests to verify Dialog/Popup changes
Please make sure that you have at least some tests for new rulers, green CI and approval from iOS folks
...e/ui/ui/src/macosMain/kotlin/androidx/compose/ui/input/pointer/util/VelocityTracker.macos.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/layout/WindowInsetsRulers.skiko.kt
Show resolved
Hide resolved
compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/input/pointer/util/VelocityTracker.web.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsets.uikit.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.uikit.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsets.uikit.kt
Outdated
Show resolved
Hide resolved
2456f2c to
dd48390
Compare
.../ui/src/desktopMain/kotlin/androidx/compose/ui/input/pointer/util/VelocityTracker.desktop.kt
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsets.skiko.kt
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsets.uikit.kt
Outdated
Show resolved
Hide resolved
|
|
||
| assertNotNull(insetsRect) | ||
|
|
||
| if (insetsRect!!.top > 0) { |
There was a problem hiding this comment.
Why do you need this if/else? In general, that can make the test useless because it can skip checking desired behavior.
There was a problem hiding this comment.
This else is intended to account for devices that might not have a top inset for the given orientation (analogical to other test cases). If we're only running this instrumented test on current iPhone models, then I agree the conditional might be unnecessary. However, on a device without a top inset, the test would fail (correctly) unless this logic is in place.
There was a problem hiding this comment.
As far as I remember, top bar safe area present on every iPhone model, but the bottom safe area may vary.
However hostingViewController.updateInterfaceOrientation() won't rotate test device, only provide stabbed orientation, so the status bar safe area will be the same for every test. Because of the i'd remove if/else.
|
|
||
| val windowInsetsManager = UIKitWindowInsetsManager() | ||
|
|
||
| fun updateInterfaceOrientation(interfaceOrientation: InterfaceOrientation) { |
There was a problem hiding this comment.
- It would be better to pass the
val interfaceOrientation: State< InterfaceOrientation>fromComposeHostingViewControllertoComposeSceneMediatorand then to theUIKitWindowInsetsManager. - It looks like the
ComposeSceneMediatorfromUIKitComposeSceneLayeralso requires this argument.
There was a problem hiding this comment.
yes, that makes sense. I have updated ComposeHostingViewController to use mutable state for interfaceOrientation and safeAreaInsets
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/platform/PlatformWindowInsets.uikit.kt
Outdated
Show resolved
Hide resolved
dd48390 to
0c36a61
Compare
| compositionContext = compositionContext, | ||
| coroutineContext = composeCoroutineContext, | ||
| enableBackGesture = configuration.enableBackGesture, | ||
| safeAreaInsetsState = safeAreaInsetsState, |
There was a problem hiding this comment.
That's tricky because the frame of ComposeHostingViewController may differ from that of UIKitComposeSceneLayer. Unlike interfaceOrientationState, safeAreaInsetsState cannot be reused and must be calculated separately for the main mediator and every mediator of platform layers.
There was a problem hiding this comment.
I see the difference, thanks for pointing to this. I have removed the reused safeAreaInsetsState and it is tied to the mediator.
mazunin-v-jb
left a comment
There was a problem hiding this comment.
lgtm, just a few nitpicks from me
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/layout/WindowInsetsRulers.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Popup.skiko.kt
Show resolved
Hide resolved
.../ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/UIKitComposeSceneLayersHolder.uikit.kt
Outdated
Show resolved
Hide resolved
c2175bf to
3347218
Compare
…s and interfaceOrientation, removing redundant update methods.
3347218 to
2a2dbe1
Compare
…composition on direct state access.
In this PR:
WindowInsetsRulersWindowInsetsactuals implementation to skiko source setWindowInsetsandWindowInsetsRulersFixes
Testing
This should be tested by QA
Release Notes
Features - iOS
WindowInsetsRulersBreaking Changes - iOS
@Composableattribute inWindowInsets.Companion.captionBarto other platforms