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.

Sign up for GitHub

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

Enforce CADisableMinimumFrameDurationOnPhone #1451

Merged
merged 14 commits into from
Aug 23, 2024
+31 −5
Merged

Enforce CADisableMinimumFrameDurationOnPhone #1451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3d6ad2a
Implement strict checking with opt-out
elijah-semyonov Jul 15, 2024
d696298
Fix
elijah-semyonov Jul 15, 2024
820bdb1
Modify comment
elijah-semyonov Jul 15, 2024
ba5b583
Update compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/P…
elijah-semyonov Jul 19, 2024
4921040
Update compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/P…
elijah-semyonov Jul 19, 2024
8c393f1
Update compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/C…
elijah-semyonov Jul 19, 2024
ee8737b
Set enforceStrictPlistSanityCheck to false on instrumented tests
elijah-semyonov Jul 19, 2024
96659ea
Set enforceStrictPlistSanityCheck to false on instrumented tests
elijah-semyonov Jul 19, 2024
df96cdf
Modify code a bit
elijah-semyonov Jul 22, 2024
2a8c759
Roll back iPhone check
elijah-semyonov Jul 30, 2024
3e0538e
Update compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/P…
elijah-semyonov Jul 30, 2024
def252d
Bump CI
elijah-semyonov Jul 31, 2024
140902c
Bump CI
elijah-semyonov Jul 31, 2024
8644cab
Remove TODO
elijah-semyonov Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ...e/ui/ui/src/uikitInstrumentedTest/kotlin/android/compose/ui/test/UIKitInstrumentedTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,11 @@ internal class UIKitInstrumentedTest {
) {
var onDraw = false
igordmn marked this conversation as resolved.
Show resolved Hide resolved
composeContainer = ComposeContainer(
configuration = ComposeUIViewControllerConfiguration().apply(configure),
configuration = ComposeUIViewControllerConfiguration().apply {
// Current instrumented test environment doesn't allow providing a plist.
enforceStrictPlistSanityCheck = false
Copy link
Collaborator

@igordmn igordmn Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Will it mean that user UI tests also fail? Could you check?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's a breaking change.

Copy link
Collaborator

@igordmn igordmn Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What I wonder, if it fail in these cases:

  1. when user uses our ui-test API. Does it use ComposeUIViewController inside?
  2. when user just uses ComposeUIViewController in the test. Does it uses the same plist as for the main application?

If we don't force users to specify enforceStrictPlistSanityCheck = false in their tests - it is good. If we force - it isn't, and we should weight all pros/cons.

Copy link
Author

Choose a reason for hiding this comment

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

Oh... I've just realised, that our test api on iOS is internal, so no blocker here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

UIKitInstrumentedTest is internal, yes, no blocker with it. But what is with the others?

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://www.jetbrains.com/help/kotlin-multiplatform-dev/compose-test.html#writing-and-running-tests-with-compose-multiplatform

Copy link
Author

Choose a reason for hiding this comment

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

We don't have other usages. If you mean other people rolling their own testing and getting a crash if plist is wrong - then it's by design. They can specifically opt out of this behavior if they don't need it for test. That's the whole rationale behind this PR.

Copy link
Author

Choose a reason for hiding this comment

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

It's completely normal that manifest inconsistencies cause a crash in runtime on iOS - a good example of it is lack of privacy description in plist, whenever a user attempts to access privacy-protected API.

Copy link
Collaborator

@ASalavei ASalavei Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like for now instrumented tests aren't use plist file, and considering the fact we're not running tests on a real device, I would recommend to keep as it is and remove TODO part.

configure()
},
content = {
Layout(
content = content,
Expand Down
7 changes: 7 additions & 0 deletions .../uikitMain/kotlin/androidx/compose/ui/uikit/ComposeUIViewControllerConfiguration.uikit.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,13 @@ class ComposeUIViewControllerConfiguration {
*/
@ExperimentalComposeApi
var opaque: Boolean = true

/**
* A boolean flag to enable or disable the strict sanity check for the `Info.plist` file.
* If the flag is set to true, and keys are missing, the app will crash with an
* explanation on how to fix the issue.
*/
var enforceStrictPlistSanityCheck: Boolean = true
}

/**
Expand Down
15 changes: 12 additions & 3 deletions compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/PlistSanityCheck.uikit.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,21 @@ internal object PlistSanityCheck {
DISPATCH_QUEUE_PRIORITY_LOW.toLong(),
0u
)) {
val entry = NSBundle
val bundle = NSBundle
.mainBundle

val displayLinkEntry = bundle
.objectForInfoDictionaryKey("CADisableMinimumFrameDurationOnPhone") as? NSNumber

if (entry?.boolValue != true) {
println("WARNING: `Info.plist` doesn't have a valid `CADisableMinimumFrameDurationOnPhone` entry. Framerate will be restricted to 60hz on iPhones. To support high frequency rendering on iPhones, add `<key>CADisableMinimumFrameDurationOnPhone</key><true/>` entry to `Info.plist`.")
if (displayLinkEntry?.boolValue != true) {
val message = """
Error: `Info.plist` doesn't have a valid `CADisableMinimumFrameDurationOnPhone` entry.
This will result in an inadequate performance on iPhones with high refresh rate.
Add `<key>CADisableMinimumFrameDurationOnPhone</key><true/>` entry to `Info.plist` to fix this error.
If you don't have a separate plist file, add the entry to the target from within Xcode: Project -> Targets -> Info -> Custom iOS Target Properties.
Or set `ComposeUIViewController(configure = { enforceStrictPlistSanityCheck = false }) { .. }`, if it's intended.
""".trimIndent()
error(message)
}
}
}
Expand Down
8 changes: 7 additions & 1 deletion compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/window/ComposeContainer.uikit.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,11 @@ import platform.UIKit.UIContentSizeCategoryExtraSmall
import platform.UIKit.UIContentSizeCategoryLarge
import platform.UIKit.UIContentSizeCategoryMedium
import platform.UIKit.UIContentSizeCategorySmall
import platform.UIKit.UIDevice
import platform.UIKit.UIStatusBarAnimation
import platform.UIKit.UIStatusBarStyle
import platform.UIKit.UITraitCollection
import platform.UIKit.UIUserInterfaceIdiomPhone
import platform.UIKit.UIUserInterfaceLayoutDirection
import platform.UIKit.UIUserInterfaceStyle
import platform.UIKit.UIView
Expand Down Expand Up @@ -185,7 +187,11 @@ internal class ComposeContainer(

override fun viewDidLoad() {
super.viewDidLoad()
PlistSanityCheck.performIfNeeded()

if (configuration.enforceStrictPlistSanityCheck) {
PlistSanityCheck.performIfNeeded()
}

configuration.delegate.viewDidLoad()
systemThemeState.value = traitCollection.userInterfaceStyle.asComposeSystemTheme()
}
Expand Down