airbnb / lottie-ios

An iOS library to natively render After Effects vector animations
http://airbnb.io/lottie/
Apache License 2.0
25.76k stars 3.75k forks source link

LottieAnimationView remove animation, but LottieAnimationLayer.rootAnimationLayer not remove #2207

Closed skytoup closed 1 year ago

skytoup commented 1 year ago

Which Version of Lottie are you using?

Lottie 4.3.3

Expected Behavior

remove animation

let view = LottieAnimationView()
view.animation = nil

https://github.com/airbnb/lottie-ios/blob/45517c3cfec9469bbdd4f86e32393c28ae9df0bc/Sources/Public/Animation/LottieAnimationLayer.swift#L1149C3-L1149C3

Why not set the LottieAnimationLayer.rootAnimationLayer to nil? The rootAnimationLayer will continue reloadImages

calda commented 1 year ago

From reading the code, calling lottieAnimationView.animation = nil will then call lottieAnimationLayer.animation = nil.

Was there a bug or regression introduced by the architecture changes that added LottieAnimationLayer? If so please reopen this issue and share more details about the expected behavior and actual behavior

calda commented 1 year ago

I see, in Lottie 4.2.0 the implementation of LottieAnimationView.makeAnimationLayer had:

    if let oldAnimation = animationLayer {
      oldAnimation.removeFromSuperlayer()
      animationLayer = nil
    }

but the current implementation of LottieAnimationLayer.makeAnimationLayer instead has:

    if let oldAnimation = animationLayer {
      oldAnimation.removeFromSuperlayer()
    }

so we are no longer setting rootAnimationLayer = nil after removing that animation layer from the view hierarchy.

@eromanc made this change as a part of #2073. @eromanc -- was this intentional? Is this necessary for a specific reason?

I can't think of any reason why adding back the rootAnimationLayer = nil would be problematic.

skytoup commented 1 year ago
let view = LottieAnimationView()
view.animation = someAnim

view.animation = nil
view.imageProvider = aNewProvider

Setting the new imageProvider will trigger LottieAnimationLayer.reloadImages, because the rootAnimationLayeris not null, so it will reload the animation's image

eromanc commented 1 year ago

@calda perhaps because animationLayer is a get-only property now?

would this work instead?

if let oldAnimation = animationLayer {
       oldAnimation.removeFromSuperlayer()
      self.rootAnimationLayer = nil
     }
calda commented 1 year ago

Yeah, I think adding rootAnimationLayer = nil there would be correct and would match the previous behavior.

eromanc commented 1 year ago

I just tested that on Origami and nothing seems to break, so I think that change would be good on my end.