What I think is wrong with the Spark Recording Circle code and why

Thoughts on removedOnCompletion in SparkRecordingCircle.md

There was a tweet a couple of days ago that resulted in a discussion/question about what it wrong with the usage of removedOnCompletion = NO in the SparkRecordingCircle code for that Subjective-C post.

We all kept saying that the explanation doesn't fit in a tweet, so here is my rough explanation about the issues.

But, let me first say that I think that the Subjective-C articles are reallt cool and useful to learn from. This is trying to reason about the general usage of removedOnCompletion = NO, using that code as an example, since that was what was discussed on twitter.

---

The root problem, as Nacho Soto pointed out, is that removedOnCompletion = NO in combination with fillMode = kCAFillModeForwards is almost always used when you are not updating the model layer. This means that after the animation has finished, what you see on screen is not reflected in the property of the layer. The biggest issue that this can cause is that all the mechanisms that rely on the model value (for example: hit-testing, KVO, etc.) won't work correctly if the model value doesn't change. That is not the case in this very example. But I see this mistake being made a lot by people who are new to Core Animation who just want the animation to persist.

In my opinion, it boils down to what Core Animation was built to do and how the animation part of it's meant to work (or at least according to my understanding of how it's meant to work). Animations in Core Animation are designed to be decoupled from the model value:

The data and state information of a layer object is decoupled from the visual presentation of that layer’s content onscreen.

They can however be triggered by a change in the model, which is the case with implicit animations. Animations can also be created and added explicitly to a layer without there being a property change. This is the very, very common case where we create our own CABasicAnimation and add it to the layer, just as it was done in this example. The important thing to note is that: since the animation is only changing how things appear on screen and since that is decoupled from the state information, adding an animation to a layer without changing the model is only doing half of the necessary work.

But, as R. Peres pointed out, in this example, the model value is updated in the removeAnimations method. And the animations are removed from the layer, so at the end of it all there is no inconsistency between the model and the presentation. That is completely true. In this example, everything works out in the end. The author writes:

You may have noticed in the updateAnimations method we set both the stokeEnd and strokeStart animations to not be removed on completion. Since we’re going to be doing some fancy footwork by copying the state of the layer when we stop the animations, we will remove the animations ourselves.

but I, personally, feel that if you are going to use a technique which is so often misused, it is important to say so and to explain why this may be a good use for that technique. This is a popular site and people look up to such sites and accept the coding practices that are used as best practices. This makes a lot of sense and the writers out there also generally write really good code. I just think that there is a responsibility to push yourself to a keep really high standard when you publish code on the web. But that is a completely separate topic, I won't say more about it.

OK, so you might say: "but the author updates the model and manually removes the animations. so what is all this fuss about?"

I, again personally, don't think that this was a good use of not removing the animation because it made the animation part of the logic. But, what not removing the animation upon completion really means in this case is that if the user holds their finger down until the the full circle is filled, the animations will persist until the user lifts their finger. This case could very easylly have been handeled by instead updating the model value before adding the animation. This means setting the toValue explicitly on the model.

Update:

Actually, as was pointed out in the comments, the animation is removed upon completion but was done so manually in the delgate callback. If that is all that the delegate callback was used for then it could be removed completely (along with setting the delegate). This would mean that the code that ecplicitly sets the model value can be made slighly clearer, since there is one less method involved to deal with the animation.

An example of this can be seen in my modified code below (I've commented out the lines that I removed and put a comment on the lines that I added or modified):

- (void)updateAnimations
{
    CGFloat duration = self.duration * (1.f - [[self.progressLayers firstObject] strokeEnd]);
    CGFloat strokeEndFinal = 1.f;
    
    for (CAShapeLayer *progressLayer in self.progressLayers)
    {
        CABasicAnimation *strokeEndAnimation = [CABasicAnimation animationWithKeyPath:@"strokeEnd"];
        strokeEndAnimation.duration = duration;
        strokeEndAnimation.fromValue = @(progressLayer.strokeEnd);
        strokeEndAnimation.toValue = @(strokeEndFinal);
        strokeEndAnimation.autoreverses = NO;
        strokeEndAnimation.repeatCount = 0.f;
//        strokeEndAnimation.fillMode = kCAFillModeForwards;            // REMOVED
//        strokeEndAnimation.removedOnCompletion = NO;                  // REMOVED
//        strokeEndAnimation.delegate = self;                           // REMOVED (optionally, after an update) 
        
        CGFloat oldStrokeEnd = progressLayer.strokeEnd;                 // ADDED
        progressLayer.strokeEnd = strokeEndFinal;                       // ADDED
        
        [progressLayer addAnimation:strokeEndAnimation forKey:@"strokeEndAnimation"];
        
        strokeEndFinal -= (oldStrokeEnd - progressLayer.strokeStart);   // MODIFIED
        
        if (progressLayer != self.currentProgressLayer)
        {
            CABasicAnimation *strokeStartAnimation = [CABasicAnimation animationWithKeyPath:@"strokeStart"];
            strokeStartAnimation.duration = duration;
            strokeStartAnimation.fromValue = @(progressLayer.strokeStart);
            strokeStartAnimation.toValue = @(strokeEndFinal);
            strokeStartAnimation.autoreverses = NO;
            strokeStartAnimation.repeatCount = 0.f;
//            strokeStartAnimation.fillMode = kCAFillModeForwards;      // REMOVED
//            strokeStartAnimation.removedOnCompletion = NO;            // REMOVED
            
            progressLayer.strokeStart = strokeEndFinal;                 // ADDED
            
            [progressLayer addAnimation:strokeStartAnimation forKey:@"strokeStartAnimation"];
        }
    }
    
    CABasicAnimation *backgroundLayerAnimation = [CABasicAnimation animationWithKeyPath:@"strokeStart"];
    backgroundLayerAnimation.duration = duration;
    backgroundLayerAnimation.fromValue = @(self.backgroundLayer.strokeStart);
    backgroundLayerAnimation.toValue = @(1.f);
    backgroundLayerAnimation.autoreverses = NO;
    backgroundLayerAnimation.repeatCount = 0.f;
//    backgroundLayerAnimation.fillMode = kCAFillModeForwards;          // REMOVED
//    backgroundLayerAnimation.removedOnCompletion = NO;                // REMOVED
//    backgroundLayerAnimation.delegate = self;                         // REMOVED (optionally, after an update) 
    
    self.backgroundLayer.strokeStart = 1.0;                             // ADDED
    
    [self.backgroundLayer addAnimation:backgroundLayerAnimation forKey:@"strokeStartAnimation"];
}

I don't think that is any more complex than the original code. And to the point about "copying the state of the layer": if the animation has already completed then it will be at it's correct state and we don't need to do any work at all. If that has not happened, the animation is still running which means that the current values can be read from the presentation layer, even though the animation wasn't configured to keep the animation around. Doing the animations like this, in my opinion, is closer to what regular usage of Core Animation looks like (add the explicit animation and update the model).

Thanks for the writeup. I tried to get this working earlier without using removedOnCompletion: and kCAFillModeForwards and failed (I was calculating the strokeEndFinal incorrectly in updateAnimations).

One thing I'm curious about is the overriding of the implicit animation (when setting the progressLayer's strokeEnd for example). In Nick Lockwood's book, he suggests wrapping the property setter using setDisableActions: YES, even though he notes that in practice the explicit animation always seems to override the implicit animation, though that behaviour is undocumented. In the WWDC 2010 presentation Core Animation In Practice Part 1 they say that an explicit animation can override an implicit animation by passing in the implicitly animated property's name in the forKey: argument of addAnimation:forKey: method. For example in this gist I put together to use the technique covered in the WWDC presentation to how Nick does it in his book. Where do you come down on this problem?

One thing David:

But what not removing the animation upon completion really means in this case is that if the user holds their finger down until the the full circle is filled, the animations will persist until the user lifts their finger.

This is not true. When the animation completes the delegate is called:

- (void)animationDidStop:(CAAnimation *)anim finished:(BOOL)flag
{
    if (self.hasFinishedAnimating == NO && flag)
    {
        [self removeAnimations];
        self.finishedAnimating = flag;
    }
}

So the animation is indeed removed when the circle completes, without lifting the finger.

You are right. If that is all the the delegate callback was used for, then it (the implementation) could be removed completely (along with assigning the delegate).

Really good explanation. It may also be relevant to explain that actually calling this:

progressLayer.strokeStart = strokeEndFinal;

is creating an (implicit) animation, which is overridden when we call:

[progressLayer addAnimation:strokeStartAnimation forKey:@"strokeStartAnimation"];

Because the animation has the same keypath for which the implicit animation was created:

CABasicAnimation *strokeStartAnimation = [CABasicAnimation animationWithKeyPath:@"strokeStart"];

And as a side note, stringly-typed programming in Objective-C is super fragile, and I strongly recommend using @keypath (https://github.com/jspahrsummers/libextobjc/blob/master/extobjc/EXTKeyPathCoding.h)

@JaviSoto actually, the implicit animation gets created with the same key as the keypath that you modified, so strokeStart, for this reason it's always good practice to do

[progressLayer setValue:strokeStartAnimation.toValue forKeyPath:strokeStartAnimation.keyPath];
[progressLayer addAnimation:strokeStartAnimation forKey:strokeStartAnimation.keyPath];

so that the animation overrides the implicit animation.

Oh wow, I completely missed out on the original Twitter conversation.

Thanks so much for taking the time to post this gist David, it's really clarified some of my misunderstandings of Core Animation. The absolute last thing I want to do is help spread bad patterns so I really appreciate you guys taking the time to discuss this.

I'm working on updating the original post and example code to correctly use the API's, will let you guys know when that's done!

I've updated the post and sample code to fix the issues discussed here.

Thanks again for the great explanation David and thanks to all for the super useful discussion. Part of what makes Cocoa so great is the communities willingness to discuss and share knowledge so we can all write better code and make cooler stuff.

If you ever notice any other issues with Subjective-C posts, I'm more than open to pull requests for the articles (https://github.com/subjc/posts) and/or any of the sample code.

Thx for this. Doing research into CADisplayLink. It will eventually be posted on www.stylekit.org