I am currently using Kingfisher library to load thumbnail images for collectionview cells and if it is already recorded in the cache, it would load from the cache. Seldomly some images are loaded for wrong cell which leads to duplicate images in the collectionview. When I try to scroll down and then dismiss/dequeue again, images are then correctly placed. This issue is not present if I do not use functionality of retrieving image from the cache also.
func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {
let cell = collectionView.dequeueReusableCell(withReuseIdentifier: "SimpleThumbnailView", for: indexPath) as! ThumbnailsCollectionViewCell
cell.thumbnailImage.kf.cancelDownloadTask()
cell.prepareForReuse()
if indexPath.row < thumbnails.count {
let urlString = thumbnails[indexPath.row].thumbnail ?? ""
ImageCache.default.retrieveImageInDiskCache(forKey: urlString) { result in
switch result {
case .success(let image):
if image == nil {
if let url = URL(string: urlString) {
DispatchQueue.main.async {
let resource = ImageResource(downloadURL: url, cacheKey: urlString)
cell.thumbnailImage.kf.setImage(with: resource, placeholder: UIImage(named: "DefaultImage"), options: [])
}
}
} else {
DispatchQueue.main.async {
cell.thumbnailImage.image = image
}
}
case .failure(let error):
print(error)
}
}
cell.titleLabel.text = thumbnails[indexPath.row].title
cell.authorLabel.text = StringBuilder.authorLikes(author: thumbnails[indexPath.row].writer, likes: thumbnails[indexPath.row].likeCount)
cell.articleId = thumbnails[indexPath.row].articleId
cell.authorId = thumbnails[indexPath.row].writer
if cell.articleId != -1 {
cell.removePlaceHolder()
}
} else {
thumbnailCollectionView.reloadData()
}
return cell
}
and this is my prepareForReuse()
override func prepareForReuse() {
super.prepareForReuse()
thumbnailImage.kf.cancelDownloadTask()
thumbnailImage.image = UIImage(named: "DefaultImage")
}
I also tried using cancelDownloadTask() function about anywhere I could think of but this didn’t worked either. Could someone have an idea how to solve this problem?
- this issue is also not present if all images are loaded from the cache disk
2
Answers
I’m not familiar with Kingfisher but would expect it does its own caching. However, your problem is a classic bug with asynchronous code in
cellForItemAt
.Collection (and table) view cells are reused when scrolling. If you perform any asynchronous work in
cellForItemAt
then there is a chance that by the time the completion handler is called the cell will have already been reused for a different index path. Your code is assuming that thecell
that is dequeued for a particular item is still being used for that item when the completion handler is called. Depending on timing and how fast you scroll the item you started the asynchronous code for may no longer be on screen or may now be used for a different item.In the
DispatchQueue.main.async
blocks where you assign the cached or fetched image you need to ask the collection view for the cell corresponding to the index path rather than assuming it is still the cell you first dequeued. This will be nil if that item is no longer on screen.Understanding the reuse system used in
UICollectionView
/UITableView
is important, and that’s why you dequeue cells.When a cell disappear, it goes on the "available cells pool" in memory.
When you need to show a cell, it’s check if there is an available cell in the previously mentioned pool, if not it creates one.
That’s for optimisation. Why? Let’s imagine that you have 10k cells to show, but only can show 5 at the same time on the screen, let’s add 1 more cell (when scrolling, you see the bottom only of the first one, and the top of the new one). Do you think it’d be efficient to create 10k cells? Not, let’s just create 6, and reuse them. That’s the whole optimization behind reusing cells (in Android SDK, it’s "Recycling", for Recycle View, the concept being the same).
But when the cells goes out of screen, it’s put in the pool as "such", with all the customization it had before (like label values, colors, subview, etc.). There it will call
prepareForReuse()
to put it in a "blank" state (do not call it yourself). It’s not needed to overrideprepareForReuse()
if you have for instance only oneUILabel
and incollectionView(_:cellForItemAt:)
you change its value each time (it’d belabel.text = nil
inprepareForReuse()
directly changed incollectionView(_:cellForItemAt:)
intolabel.text = "myValue"
, which is extra work not necessary.Once you understood this, the current issue with your code is this one:
You use two async call (but one is enough to create the issue), so when the closure is called and when you want to set the value, the cell might already have been reused for another
indexPath
. Hence the issue you get having images at wrong index paths cells when scrolling.Now, you can say that
setImage(with url:)
from KingFisher/Alamofire+Image/SDWebImage and other libs are using a "hidden" closure too and they don’t have the issue. It’s because they useassociated_object
, which let them to add an extra value to an object (theUIImageView
). That extra value is an id (the URL of the image), so when they want to set the image, they check if the imageView associated id is still the correct one, if not, the cell has already been reused.That’s a simplification of what’s done.
So in your case, you could add extra value to the cell, and check if it’s really the same inside the closure, but in fact it’s not needed, because you’re overthinking the issue with KingFisher.
Depending on how you use KingFisher (there are extra parameters : force refresh etc), but by default it will cache itself the images, and check in its cache if it’s there or not and avoid a web call.
Here, I’d suggest remove all of the cache check code (since it’s already done by the lib):
Now, if you want to check for yourself, you can put breakpoint inside the lib, of use the
setImage()
with the completion parameter. I didn’t test it, but I guess the answer from where is coming the image, cache or not might be in theRetrieveImageResult
value.Another issue you had was reloading of the whole collectionView.