facebookarchive / pop

An extensible iOS and OS X animation library, useful for physics-based interactions.
Other
19.66k stars 2.88k forks source link

If toValue equals the target's current value then completionBlock will fire immediately #414

Open warpling opened 6 years ago

warpling commented 6 years ago

TL;DR: If the toValue of an animation equals the target's current value (even if the fromValue does not) then the completionBlock will be called immediately with a truthy completed value.

Imagine we have a red square we've hidden by setting it's alpha to 0.0. Now we want to flash it in to alpha 1.0 and slowly fade it out. We might add a basic animation like this:

let view = UIView(frame: CGRect(x: 0, y: 0, width: 100, height: 100))
view.backgroundColor = .magenta
view.alpha = 0.0
addSubview(view)

let flashAndFadeAnimation = POPBasicAnimation(propertyNamed: kPOPViewAlpha)!
flashAndFadeAnimation.duration = 10.0
flashAndFadeAnimation.timingFunction = CAMediaTimingFunction.cubicOut()
flashAndFadeAnimation.fromValue = 1.0
flashAndFadeAnimation.toValue   = 0.0
flashAndFadeAnimation.completionBlock = { [weak self] (animation, completed) in
   print("completed at: \(Date())")
}

print("animation added at: \(Date())")
view.pop_add(flashAndFadeAnimation, forKey: "flashAndFade")

The result is that the completionBlock will be called immediately despite the animation itself taking 10 seconds and properly animating.

This elusive bug has bitten me for years and I've only now connected the dots. 😆 I'd love to create a PR if anyone with intimate knowledge of the library could help set me off in the right direction to patch it and add the proper tests.

warpling commented 6 years ago

Two potential workarounds using the above code as an example:

A (best workaround)

Use animationDidStartBlock to set the target's value to match the fromValue. This will work even if the animation has a delayed beginTime. (If referencing self you may want to consider including [weak self] at the beginning of the block.)

flashAndFadeAnimation.animationDidStartBlock = { (animation) in
    view.alpha = flashAndFadeAnimation.fromValue as! CGFloat
}

B (simplest workaround if you aren't relying on beginTime)

Since POP actually changes the target's properties (unlike CAAnimation), you can set the target's property to the animation's fromValue before the animation is added:

view.alpha = flashAndFadeAnimation.fromValue as! CGFloat
view.pop_add(flashAndFadeAnimation, forKey: "flashAndFade")

C (quick, dirty, and error prone)

Set the toValue to something like 0.00001. But now if you then try to run this flash animation a second time the current value will now be the same as the toValue and the same problem can occur! To avoid this you might be able to set the target's value to 0.0 in the completionBlock.