Cancelling download when manually setting an image in the meantime

[solidcell]

I ran into this issue while fixing a bug related to cell reuse. It would seem like an active download should be cancelled if an image is set manually before the download is finished.

// start a potentially large download
imageView.kf.setImage(with: URL(string: "https://raw.githubusercontent.com/onevcat/Kingfisher/master/images/logo.png")!)

// cell reused (and therefore UIImageView) before download is finished and instead we want to set a `UIImage`
imageView.image = UIImage(systemName: "pencil")

// later, the download finishes and replaces the image we've more recently set. 💥

What we need to do instead is:

// start a potentially large download
imageView.kf.setImage(with: URL(string: "https://raw.githubusercontent.com/onevcat/Kingfisher/master/images/logo.png")!)

// cell reused (and therefore UIImageView) before download is finished and instead we want to set a `UIImage`
// however, first we must cancel the current download
imageView.kf.cancelDownloadTask()
imageView.image = UIImage(systemName: "pencil")

// the image stays as it should ✅

This might be intended behavior, but I feel like this is very strange. self.image= and self.kf.setImage() are two sides of the same coin. If you were to call setImage twice, the first call would automatically be cancelled, yet setting image directly is not safe.

[onevcat]

self.image= and self.kf.setImage() are two sides of the same coin

It is intended behavior, since it is hardly for Kingfisher to detect or determine if an plain image view set (imageView.image = ...) should cancel the download or not.

I'd say they are two different things instead of the "same coin". The kf extension method involves download task management, while the plain image setting is a pure UIKit thing. For your case, I would suggest an image set method with nil source, so the previous image setting won't work and you can get the correct result (as well as some uniform code):

imageView.kf.setImage(with: largeImageURL)

// cell reuse
imageView.kf.setImage(with: nil, placeholder: UIImage(systemName: "pencil"))

Or say, even simpler, if you are going to use the "reused cell" immediately to load a new URL, you can even just skip the "pencil" image setting. Just set the new URL and Kingfisher did the trick behind scene to prevent the previous image from showing:

// Cell delegate,  eg `collectionView(_:cellForItemAt:)`:
imageView.kf.setImage(with: imageURL1, placeholder: UIImage(systemName: "pencil"))

// Later, another cell delegate method (reused)
imageView.kf.setImage(with: imageURL2, placeholder: UIImage(systemName: "pencil"))

[solidcell]

In my use case, the "pencil" image is just a default avatar which might be used if there's no remote image to download at all. So less of a placeholder and more of a legitimate case in and of itself. So using your first suggestion (with nil and a placeholder) would do the job, but is a bit semantically misleading IMO.

In either case (using cancelDownloadTask/image= or setImage(with: nil), the onus is still on the user to keep things in sync or to be sure to only use Kingfisher for a given UIImageView if ever used once, respectively..

it is hardly for Kingfisher to detect or determine if an plain image view set

I'm not so sure I agree here. Kingfisher is using objc_setAssociatedObject, so it's already asserting itself into the UIKit object. I don't think swizzling image= to add an implicit cancelDownloadTask would be much different.