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

Async animation loading improvements #2127

Merged

Conversation

miguel-jimenez-0529
Copy link
Contributor

This PR adds the following changes:

  • Renames loadAnimationTrigger to reloadAnimationTrigger
  • Replaces reloadAnimationTrigger binding value from Hashable to Equatable
  • Fixes an issue where the animation is accidentally removed when calling reloadAnimationTrigger without a valid loadAnimation closure.
  • Add documentation to init(dotLottieFile:) about not using the initializer with SynchronouslyBlockingCurrentThread APIs
  • Add assert to SynchronouslyBlockingCurrentThread APIs when called from the main thread.

/// - showPlaceholder: When `true`, the current animation will be removed before invoking `loadAnimation`,
/// displaying the `Placeholder` until the new animation loads.
/// When `false`, the previous animation remains visible while the new one loads.
public func reloadAnimationTrigger<Value: Equatable>(_ binding: Binding<Value>, showPlaceholder: Bool = true) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be a Binding, or can it just take Value directly?

Suggested change
public func reloadAnimationTrigger<Value: Equatable>(_ binding: Binding<Value>, showPlaceholder: Bool = true) -> Self {
public func reloadAnimationTrigger<Value: Equatable>(_ value: Value, showPlaceholder: Bool = true) -> Self {

Copy link
Member

Choose a reason for hiding this comment

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

maybe if we use @State instead of Binding<any Equatable>?

Copy link
Member

Choose a reason for hiding this comment

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

We should be able to do the same onChange { loadAnimationIfNecessary() } logic if this used @State instead of a binding, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the recommendation would be to pass a Binding when your view doesn't own the @State. https://swiftuipropertywrappers.com/

Copy link

@denandreychuk denandreychuk Aug 3, 2023

Choose a reason for hiding this comment

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

Binding most of the time indicates that view can modify the value.

If we have a @Published property inside an ObservableObject, and this ObservableObject is part of a SwiftUI view, we will still receive an onChange notification even without using @State or @Binding. However, in such cases, I believe we can always use .constant(Value) or $viewModel.value to create a Binding if you want to force the caller to consider maintaining the source of truth.

final class TestViewModel: ObservableObject {
    
    private var timer: Timer?
    @Published var value: Bool = false
    
    func setup() {
        timer = Timer.scheduledTimer(withTimeInterval: 1, repeats: true) { [weak self] _ in
            self?.value.toggle()
        }
    }
}

struct TestView: View {
    @EnvironmentObject private var viewModel: TestViewModel
    
    var body: some View {
        LottieView { ... }
        .onAppear {
            viewModel.setup()
        }
        .onChange(of: viewModel.value) { value in
            print("value changed: \(value)")
        }
    }
}

Choose a reason for hiding this comment

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

I believe that your initial suggestion should work. It will trigger an onChange event as long as value is annotated with @State, @Binding, or @Published. However, this will not work if we have a regular property that is not marked with any of these property wrappers, which is fine.

public func reloadAnimationTrigger<Value: Equatable>(_ value: Value, showPlaceholder: Bool = true) -> Self

Copy link

@denandreychuk denandreychuk Aug 3, 2023

Choose a reason for hiding this comment

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

Specifically important is that the onChange has to be inside LottieView itself rather than the parent view.`

Example with ChildView usage

final class TestViewModel: ObservableObject { ... }

struct ChildViewAKALottieView: View {
    let value: Bool
    
    var body: some View {
        Text("Hello World")
            .onChange(of: value) { value in
                print("value changed: \(value)")
            }
    }
}

struct TestView: View {
    @EnvironmentObject private var viewModel: TestViewModel
    
    var body: some View {
        ChildViewAKALottieView(value: viewModel.value)
            .onAppear {
                viewModel.setup()
            }
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

We were able to get this to work without a binding in 1e777ea

Copy link
Member

Choose a reason for hiding this comment

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

Interestingly, if you take that commit and change private var reloadAnimationTrigger: AnyEquatable? to @State private var reloadAnimationTrigger: AnyEquatable? then it no longer works correctly. Adding @State specifically breaks it.

Copy link

@denandreychuk denandreychuk Aug 3, 2023

Choose a reason for hiding this comment

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

Thank you, @calda! Appreciate your time!

Regarding @State, it breaks everything cause it gets initialized only once. Then, even if you update the value in the parent, as long as LottieView is on-screen, it will maintain the same initialized value from the first call. However, you can override this behavior using State(initialValue:), but in practice, this doesn't always work as expected too.

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.

Thanks! All good improvements.

@calda calda merged commit 6126d6e into airbnb:master Aug 3, 2023
12 checks passed
iago849 pushed a commit to atteamapps/lottie-ios that referenced this pull request Feb 8, 2024
MoroziOS pushed a commit to MoroziOS/tmg-lottie-ios that referenced this pull request May 22, 2024
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.

3 participants