Skip to content

Comments

Adopt window insets rulers#2258

Merged
svastven merged 60 commits intojb-mainfrom
svastven/window-inset-rulers
Aug 11, 2025
Merged

Adopt window insets rulers#2258
svastven merged 60 commits intojb-mainfrom
svastven/window-inset-rulers

Conversation

@svastven
Copy link

@svastven svastven commented Jul 21, 2025

In this PR:

  • Add support for WindowInsetsRulers
  • Move WindowInsets actuals implementation to skiko source set
  • Use single source for retrieving platform window insets for WindowInsets and WindowInsetsRulers

Fixes

  • CMP-8299 [iOS] Support window insets rulers
  • CMP-6878 Use PlatformInsets as a common abstraction instead of WindowInsets
  • CMP-4434 Compose 1.6.0 full recomposition after keyboard appearing (iOS)

Testing

This should be tested by QA

Release Notes

Features - iOS

  • Add support for WindowInsetsRulers

Breaking Changes - iOS

  • Align @Composable attribute in WindowInsets.Companion.captionBar to other platforms

@svastven svastven changed the title Draft: Svastven/window inset rulers Adopt window inset rulers Jul 21, 2025
@svastven svastven force-pushed the svastven/window-inset-rulers branch from ca52921 to 8f9cf93 Compare July 23, 2025 20:46
@svastven svastven changed the title Adopt window inset rulers Adopt window insets rulers Jul 23, 2025
Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

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

Despite the number of comments it looks pretty good (didn't test it localy yet)

@svastven svastven self-assigned this Jul 24, 2025
)
}

// TODO: Consider LTR/RTL

Choose a reason for hiding this comment

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

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 🙂

Choose a reason for hiding this comment

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

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.

@svastven svastven force-pushed the svastven/window-inset-rulers branch from fbaddbb to fdf226c Compare July 24, 2025 22:47
@svastven svastven marked this pull request as ready for review July 24, 2025 22:55
Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

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

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

@svastven svastven requested a review from ASalavei July 25, 2025 09:57
@svastven svastven force-pushed the svastven/window-inset-rulers branch from 2456f2c to dd48390 Compare July 31, 2025 21:45
@svastven svastven requested a review from MatkovIvan July 31, 2025 21:50

assertNotNull(insetsRect)

if (insetsRect!!.top > 0) {
Copy link

@ASalavei ASalavei Aug 1, 2025

Choose a reason for hiding this comment

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

Why do you need this if/else? In general, that can make the test useless because it can skip checking desired behavior.

Copy link
Author

@svastven svastven Aug 1, 2025

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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) {
Copy link

Choose a reason for hiding this comment

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

  1. It would be better to pass the val interfaceOrientation: State< InterfaceOrientation> from ComposeHostingViewController to ComposeSceneMediator and then to the UIKitWindowInsetsManager.
  2. It looks like the ComposeSceneMediator from UIKitComposeSceneLayer also requires this argument.

Copy link
Author

Choose a reason for hiding this comment

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

yes, that makes sense. I have updated ComposeHostingViewController to use mutable state for interfaceOrientation and safeAreaInsets

@svastven svastven force-pushed the svastven/window-inset-rulers branch from dd48390 to 0c36a61 Compare August 4, 2025 11:20
@svastven svastven requested a review from ASalavei August 4, 2025 12:43
compositionContext = compositionContext,
coroutineContext = composeCoroutineContext,
enableBackGesture = configuration.enableBackGesture,
safeAreaInsetsState = safeAreaInsetsState,
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I see the difference, thanks for pointing to this. I have removed the reused safeAreaInsetsState and it is tied to the mediator.

Copy link

@mazunin-v-jb mazunin-v-jb left a comment

Choose a reason for hiding this comment

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

lgtm, just a few nitpicks from me

@svastven svastven force-pushed the svastven/window-inset-rulers branch from c2175bf to 3347218 Compare August 5, 2025 19:07
@svastven svastven force-pushed the svastven/window-inset-rulers branch from 3347218 to 2a2dbe1 Compare August 6, 2025 08:07
@svastven svastven requested a review from ASalavei August 6, 2025 08:55
@svastven svastven requested a review from ASalavei August 7, 2025 14:27
@svastven svastven merged commit f4fb6a8 into jb-main Aug 11, 2025
15 checks passed
@svastven svastven deleted the svastven/window-inset-rulers branch August 11, 2025 11:00
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.

5 participants