SwiftUI: Overwrite KFImage appear animation

[alexeichhorn]

Problem:
With pull request #1767 you fixed some animation issues, but also created a new one (at least for my use case). When you have a KFImage view with an animation modifier somewhere in the tree, this animation is also applied when the image was loaded. In ImageBinder a withAnimation-block is used to animate the appearance when needed, but a withAnimation-block doesn't overwrite any animation set in any parent of KFImage. This leads especially to weird animations when the image is loaded from cache and therefore no animation should happen, which happens anyway.
Also ImageBinder sets loadedImage directly which also often causes scaling animations when the resizable() modifier is applied to KFImage.

Failed solution: This seems like the trivial solution, but doesn't quite work: Set the animation modifier with animation(nil) like this:

List(objects) { object in
       HStack {
             KFImage(object.url)
                  .resizable()
                  .animation(nil)

             Text(object.title)
       }
}
.animation(.easeInOut)

This fixes the problem of having unwanted animations when the image inside KFImage is loaded, but also destroys the animation when e.g. two objects in the list get switched. In this example, the text would animate as expected in SwiftUI, but the image would just switch instantly. So this would completely disable the animation behaviour of these images (a wrapper with animation enabled doesn't help there).

Proposed Solution:
Instead of using a withAnimation-block in ImageBinder when the image was successfully loaded, I used a transaction block as follows:

var transaction = Transaction(animation: animation)
transaction.disablesAnimations = true
withTransaction(transaction) {
      self.loadedImage = value.image
      self.loaded = true
}

By using the disableAnimations property, you can actually overwrite the animation set on the view and have a guarantee to use the given one. The naming of disableAnimations is actually a bit confusing, because it only disables animations defined in the view hierarchy. I guess a better name would be overwriteAnimations. Therefore when using e.g. animation = nil, it is guaranteed there won't be any animation, while withAnimation doesn't do that. I also included setting the loadedImage inside the block, which prevents the weird scaling animations when the image is loaded.

Things to consider:
Developers may want to have custom animations when the image is loaded, which would only be possible via the internal ImageTransition, which currently only supports a linear fade, after this request and animation modifiers wouldn't affect any animations initiated by Kingfisher itself. So it would enable having different animations for image appearance and anything else in your app.
I think this way of handling animations is a better approach, but maybe you have a different taste or a better idea to solve this problem.

[onevcat]

Thanks for this. I didn't know the disableAnimations before and it looks promising! I will see how it can work with current setup!

[onevcat]

@alexeichhorn With the change here, I found the List Demo scene in the sample app works in a quite funny way:

Simulator.Screen.Recording.-.iPhone.12.-.2021-09-21.at.11.02.13.mp4

When I move it out from the transaction block, it works back to normal.

Is that necessary or is it trying to fix a certain problem?

[alexeichhorn]

Seems like I have only tested in the sample app with the first commit. Sorry for that.

Is that necessary or is it trying to fix a certain problem?

It should actually prevent these weird animations 😂, but in a different scenario. In your sample apps you don't really use the .animation(...) modifier (instead only withAnimation), but I do e.g. in certain parts of my apps. So this modifier causes problems with KFImage. Above I e.g. described:

Also ImageBinder sets loadedImage directly which also often causes scaling animations when the resizable() modifier is applied to KFImage.

So setting loadedImage inside the transaction block is to prevent that. If you move it out, I can give you few examples where it completely breaks animations:

1. In SingleViewDemo.swift change two lines of codes:

VStack {
    KFImage(url)
        .cacheOriginalImage()
        .setProcessor(blackWhite ? BlackWhiteProcessor() : DefaultImageProcessor())
        .onSuccess { r in
            print("suc: \(r)")
        }
        .onFailure { e in
            print("err: \(e)")
        }
        .placeholder { progress in
            ProgressView(progress)
        }
        .fade(duration: index == 1 ? 0 : 1)
        .forceTransition()
        .resizable()
        .scaledToFill()   // INSERTED THIS
        .frame(width: 300, height: 300)
        .cornerRadius(20)
        .shadow(radius: 5)
        .frame(width: 320, height: 320)

    Button(action: {
        self.index = (self.index % 10) + 1
    }) { Text("Next Image") }
    Button(action: {
        self.blackWhite.toggle()
    }) { Text("Black & White") }

}.navigationBarTitle(Text("Basic Image"), displayMode: .inline)
.animation(.linear(duration: 3.0))     // AND INSERTED THIS

(adding .scaledToFill() and .animation(.linear(duration: 3.0))).

And this is how it looks like:

RPReplay_Final1632440733.MP4

2. example in LazyVStackDemo.swift:
   Also insert .animation(.linear(duration: 1.0)) like this:

struct LazyVStackDemo: View {
    var body: some View {
        ScrollView {
            LazyVStack {
                ForEach(1..<700) { i in
                    ImageCell(index: i).frame(width: 300, height: 300)
                }
            }
            .animation(.linear(duration: 1.0))   // INSERT THIS
        }.navigationBarTitle(Text("Lazy Stack"), displayMode: .inline)
    }
}

And it will look like this:

RPReplay_Final1632440756.MP4

And in a similar way you can theoretically mess up most of these examples by just adding the animation() modifier.

[alexeichhorn]

I am working on a solution for this - it's a bit harder than anticipated. I currently almost have a solution, but the last Transition example in the Demo App is not 100% working. Everything else works.

[onevcat]

@alexeichhorn
Appreciate the work until now! ....At least now I can get the conclusion that SwiftUI animation is hard and fragile..

[alpha-khambir]

Any updates?

[alexeichhorn]

Still having problems to handle the animation correctly when also applying a transition animation. As @onevcat said, SwiftUI animations are fragile. Fixing one thing, breaks another. Fixing such little UI bugs in SwiftUI is very hard as you don't have full control. So I am sorry, I don't have the mental endurance to fix it currently 😂. Only solution that would fully work is basically just using the UIKit version and wrapping it for SwiftUI. Might however not be the best solution for other reasons.

[onevcat]

I will revisit this issue this weekend when I can find some more time!

[onevcat]

Hi,

I have a chance to check this issue again today, and here are some of my thoughts.

1. As we all know, from iOS 15, Apple deprecated the implicit animation animation(_:) and suggest to use animation(_:value:) instead. The non-value version of animation would add the animation to all the transaction applied to the whole view tree to any animatable properties. It is the evil cause of all the chaos of SwiftUI animation and finally now Apple noticed this. So, for animation purpose, adding a value to the implicit animation should solve most of the problems mentioned in this issue. For example, in the first code snippet, if you need to fade in the list, maybe it is a better idea to add a state and only apply animation to that state:

+ @State private var shown = false
List(objects) { object in
  HStack {
    KFImage(object.url)
    .resizable()
-   .animation(nil)

    Text(object.title)
  }
}
- .animation(.easeInOut)
+ .animation(.easeInOut, value: shown)
+ .opacity(shown ? 0.0 : 1.0)
+ .onAppear { shown.toggle() }

2. Again, adding .animation(nil) to KFImage is removing all the animation, and it is also an expected behavior. Yes, it also removes the possibility of animation from a parent view (such as list cell reordering), but it is what expected in the old implicit animation.
3. I am not quite a fan of creating a Transaction and customize the disablesAnimations more than necessary way. Behind the scene, withAnimation is a shorthand of Transaction and one can make further customization by adding a .transaction modifier to control it in a better way.
4. There is a recent change Move animation setting before image setting #1873 which fixes some certain case that the nil animation not applied. I am not sure if it is related to this issue (probably not), but withAnimation(nil) seems broken in old implementation.