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

Enforce CADisableMinimumFrameDurationOnPhone #1451

Merged
merged 14 commits into from
Aug 23, 2024
Merged

Conversation

elijah-semyonov
Copy link

@elijah-semyonov elijah-semyonov commented Jul 15, 2024

Proposed Changes

By default, crash applications which don't contain a proper CADisableMinimumFrameDurationOnPhone entry in the Info.plist, when running on an iPhone.

Implementation for https://youtrack.jetbrains.com/issue/CMP-5643/Make-CADisableMinimumFrameDurationOnPhone-a-requirement-to-run-Compose-Multiplatform-on-iOS

Release Notes

iOS - Breaking Changes

  • The app will crash by default, if CADisableMinimumFrameDurationOnPhone is not set to true in Info.plist. Use newly added ComposeUIViewControllerConfiguration.enforceStrictPlistSanityCheck to opt-out of this behavior.

@igordmn
Copy link
Collaborator

igordmn commented Jul 16, 2024

iOS - Features

Please, change it to Breaking Changes - iOS

@MatkovIvan MatkovIvan changed the title Es/enforce plist Enforce CADisableMinimumFrameDurationOnPhone Jul 16, 2024
configuration = ComposeUIViewControllerConfiguration().apply {
// TODO: setup proper plist for instrumented test
// https://youtrack.jetbrains.com/issue/CMP-5706/iOS-setup-proper-plist-in-instrumented-tests
enforceStrictPlistSanityCheck = false
Copy link
Collaborator

@igordmn igordmn Jul 19, 2024

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

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.

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

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.

@elijah-semyonov
Copy link
Author

elijah-semyonov commented Jul 22, 2024

Relaxed the check a change to crash only on iPhone. Apparently, iPad is not affected by this flag.

@igordmn
Copy link
Collaborator

igordmn commented Jul 22, 2024

Relaxed the check a change to crash only on iPhone. Apparently, iPad is not affected by this flag.

Better to reenable it:

  • devs who check on iPad can be surprised when run it on iPhone
  • if we checked only one device, we can't be sure that this always happen

@elijah-semyonov
Copy link
Author

Oh oh... still not merged

elijah-semyonov and others added 11 commits August 22, 2024 11:05
…listSanityCheck.uikit.kt

Co-authored-by: Igor Demin <igordmn@users.noreply.github.com>
…listSanityCheck.uikit.kt

Co-authored-by: Igor Demin <igordmn@users.noreply.github.com>
…omposeUIViewControllerConfiguration.uikit.kt

Co-authored-by: Igor Demin <igordmn@users.noreply.github.com>
…listSanityCheck.uikit.kt

Co-authored-by: Igor Demin <igordmn@users.noreply.github.com>
@elijah-semyonov
Copy link
Author

Removed TODO

@elijah-semyonov elijah-semyonov merged commit 2bcd84f into jb-main Aug 23, 2024
6 checks passed
@elijah-semyonov elijah-semyonov deleted the es/enforce-plist branch August 23, 2024 08:10
cheonjaeung added a commit to cheonjaeung/gridlayout-compose that referenced this pull request Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants