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

Replace UIScreen.main to get display scale on iOS 13.0 and later (#2215) #2216

Merged
merged 14 commits into from Nov 2, 2023

Conversation

hyun99999
Copy link
Contributor

@hyun99999 hyun99999 commented Oct 18, 2023

UIScree.main will be deprecated in a future version of iOS.
I used window.windowScene.screen to get display's scale.

developer doc > mian already deprecated. It causes serious errors in the near future.

So have to change another way. And window.windowScene.screen is the one way of getting display's scale. That is available from iOS 13.0. This is why control flow has iOS 13.0 condition.

Resolved: #2215

@hyun99999 hyun99999 changed the title Use UIApplication instead of UIScreen.main on iOS 13.0 and later Use UIApplication instead of UIScreen.main on iOS 13.0 and later (#2215) Oct 18, 2023
@hyun99999 hyun99999 changed the title Use UIApplication instead of UIScreen.main on iOS 13.0 and later (#2215) Use window.windowScene.screen instead of UIScreen.main on iOS 13.0 and later (#2215) Oct 18, 2023
@@ -39,7 +39,11 @@ open class LottieAnimationViewBase: UIView {

var screenScale: CGFloat {
#if os(iOS) || os(tvOS)
UIScreen.main.scale
if #available(iOS 13.0, *) {
return window?.windowScene?.screen.scale ?? .zero
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to provide a reasonable fallback, similar to UIScreen.main.scale, rather than just .zero? Returning .zero for some reason would result in a very broken experience.

Copy link
Contributor Author

@hyun99999 hyun99999 Oct 18, 2023

Choose a reason for hiding this comment

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

UIScreen.main.scale has several values taht each device display.

For Retina displays, the scale factor may be 3.0 or 2.0 and one point can represented by nine or four pixels, respectively. For standard-resolution displays, the scale factor is 1.0 and one point equals one pixel
https://developer.apple.com/documentation/uikit/uiscreen/1617836-scale

Not all devices had a Retina display for the iPhone. This is expected complex control flow.

If there noting special happens, a view will have window.windowScene. However, it seems reasonable to set it to 1.0 to ensure minimal operation through the scale factor.

hyun99999 and others added 2 commits October 18, 2023 22:54
- add tvOS 13.0

Co-authored-by: Cal Stephens <cal@calstephens.tech>
@@ -39,7 +39,11 @@ open class LottieAnimationViewBase: UIView {

var screenScale: CGFloat {
#if os(iOS) || os(tvOS)
UIScreen.main.scale
if #available(iOS 13.0, tvOS 13.0, *) {
return window?.windowScene?.screen.scale ?? 1.0
Copy link
Member

Choose a reason for hiding this comment

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

This will still be a behavior change if window == nil, but I'm not sure if there's a case where that matters (since if window == nil then the view isn't on-screen anyway).

Copy link
Contributor Author

@hyun99999 hyun99999 Oct 18, 2023

Choose a reason for hiding this comment

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

I'm on the same page.

@calda calda enabled auto-merge (squash) October 18, 2023 15:55
Copy link
Contributor Author

@hyun99999 hyun99999 left a comment

Choose a reason for hiding this comment

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

Thank you for your review so far.

@calda calda self-requested a review October 18, 2023 18:56
@calda
Copy link
Member

calda commented Oct 18, 2023

This change has reduced the render quality of text in the snapshot tests regression suite. Here's an example:

Before After
image image

That will need to be fixed before we can merge this, since there could be other consumers using a render configuration similar to our snapshot test suite.

Copy link
Member

@calda calda left a comment

Choose a reason for hiding this comment

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

Currently screenScale is only accessed in commonInit(), which is called during init(). At this point view.window is always nil, so screenScale will always be 1.0.

If we're going to access view.window, then we need to handle the case where the window changes, e.g. by rerunning any logic that depends on the window or screenScale in didMoveToWindow.

auto-merge was automatically disabled October 19, 2023 03:09

Head branch was pushed to by a user without write access

@hyun99999
Copy link
Contributor Author

Currently screenScale is only accessed in commonInit(), which is called during init(). At this point view.window is always nil, so screenScale will always be 1.0.

If we're going to access view.window, then we need to handle the case where the window changes, e.g. by rerunning any logic that depends on the window or screenScale in didMoveToWindow.

Implements screenScale setter to re-run action that depend on the screenScale in didMoveToWindow
Related comiit is 5cf3555.

@hyun99999
Copy link
Contributor Author

hmm.. window property has nil, until a view is added to window.
Would it be difficult to change Build Setting > Require Only App-Extension-Safe APIvalue to NO?
That can use UIApplication.shared. And this is can access to scale property.
@calda

@calda
Copy link
Member

calda commented Oct 19, 2023

I don't think the change you made fixed the problem, because LottieAnimationView itself is accessing screenScale in init (before the view has a window) and then never accesses it again. We'll need to make it so LottieAnimationView itself reacts to screenScale updates. Hopefully this doesn't require completely reinitializing the animation layer, since that would be pretty expensive.

@calda
Copy link
Member

calda commented Oct 19, 2023

Require Only App-Extension-Safe APIvalue to NO?

Hmmmmm, this probably would be a breaking change for some number of consumers. Is there a way for us to #if def the check, so we use the simpler code unless in an app extension?

@calda
Copy link
Member

calda commented Oct 19, 2023

What would the code look like using UIApplication.shared?

@hyun99999
Copy link
Contributor Author

What would the code look like using UIApplication.shared?

Like this!

let windowScene = UIApplication.shared.connectedScenes.first as? UIWindowScene
windowScene?.screen.scale

@hyun99999
Copy link
Contributor Author

Require Only App-Extension-Safe APIvalue to NO?

Hmmmmm, this probably would be a breaking change for some number of consumers. Is there a way for us to #if def the check, so we use the simpler code unless in an app extension?

I don't understand '#if def the check'. Perdon?

@hyun99999
Copy link
Contributor Author

CI/Test Package are continuously failed.
Let's commit and see if the test passes or not.

- to use `UIApplication.shared`
@calda
Copy link
Member

calda commented Oct 19, 2023

I was thinking something like this: https://stackoverflow.com/questions/25048026/how-to-detect-if-code-is-running-in-main-app-or-app-extension-target but it sounds like this won't be possible for us since (1) we don't control the custom build flags, and (2) we distribute pre-compiled binaries. So I don't think we can use UIApplication.shared unfortunately.

@hyun99999
Copy link
Contributor Author

I was thinking something like this: https://stackoverflow.com/questions/25048026/how-to-detect-if-code-is-running-in-main-app-or-app-extension-target but it sounds like this won't be possible for us since (1) we don't control the custom build flags, and (2) we distribute pre-compiled binaries. So I don't think we can use UIApplication.shared unfortunately.

Recently i access screenScale in didMoveToWindow() but it is not apply layer screen scale in commonInit(). I agree your opinion.
Then i'll have to look for it that LottieAnimationView updates screenScale itself. After view is added that it has window.

@hyun99999 hyun99999 changed the title Use window.windowScene.screen instead of UIScreen.main on iOS 13.0 and later (#2215) Replace UIScreen.main to get display scale on iOS 13.0 and later (#2215) Nov 2, 2023
Co-authored-by: Cal Stephens <cal@calstephens.tech>
@calda
Copy link
Member

calda commented Nov 2, 2023

Thanks!

@calda calda merged commit f081afa into airbnb:master Nov 2, 2023
15 checks passed
@hyun99999
Copy link
Contributor Author

Thank you for your sincere review! 🙌

cgrindel-self-hosted-renovate bot added a commit to cgrindel/rules_swift_package_manager that referenced this pull request Dec 4, 2023
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [airbnb/lottie-spm](https://togithub.com/airbnb/lottie-spm) | patch |
`from: "4.3.3"` -> `from: "4.3.4"` |

---

### Release Notes

<details>
<summary>airbnb/lottie-spm (airbnb/lottie-spm)</summary>

###
[`v4.3.4`](https://togithub.com/airbnb/lottie-spm/releases/tag/4.3.4)

[Compare
Source](https://togithub.com/airbnb/lottie-spm/compare/4.3.3...4.3.4)

#### What's Changed

- Fix parsing regression in 4.3.0 from addition of parsing layer effects
by [@&#8203;calda](https://togithub.com/calda) in
[airbnb/lottie-ios#2208
- Remove old animation layer when creating a new animation layer by
[@&#8203;junjielu](https://togithub.com/junjielu) in
[airbnb/lottie-ios#2214
- Add configuration setting to remove animated bounds changes by
[@&#8203;thedrick](https://togithub.com/thedrick) in
[airbnb/lottie-ios#2218
- Change pod dependencies `SwiftUI` and `Combine` to `weak` to ensure
compatibility with iOS 12 by
[@&#8203;florianrhein](https://togithub.com/florianrhein) in
[airbnb/lottie-ios#2219
- Fix issue where Repeater would be ignored if not at top level by
[@&#8203;calda](https://togithub.com/calda) in
[airbnb/lottie-ios#2221
- Replace `UIScreen.main` to get display scale on iOS 13.0 and later
([#&#8203;2215](https://togithub.com/airbnb/lottie-spm/issues/2215)) by
[@&#8203;hyun99999](https://togithub.com/hyun99999) in
[airbnb/lottie-ios#2216
- Dispatch dot lottie file loading onto a single serial queue by
[@&#8203;erichoracek](https://togithub.com/erichoracek) in
[airbnb/lottie-ios#2229
- Clean up unused property in InvertedMatteLayer by
[@&#8203;hanton](https://togithub.com/hanton) in
[airbnb/lottie-ios#2241
- Fix issue where LottieView animation would restart from beginning
after backgrounding app by [@&#8203;calda](https://togithub.com/calda)
in
[airbnb/lottie-ios#2237

**Full Changelog**:
airbnb/lottie-ios@4.3.3...4.3.4

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDAuMCIsInVwZGF0ZWRJblZlciI6IjM2LjEwMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>
iago849 pushed a commit to atteamapps/lottie-ios that referenced this pull request Feb 8, 2024
…IScreen.main.scale` (airbnb#2216)

Co-authored-by: Cal Stephens <cal@calstephens.tech>
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.

UIScreen.main will be deprecated in a future version of iOS
2 participants