skip to Main Content

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


  1. 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 the cell 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.

    Login or Signup to reply.
  2. 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 override prepareForReuse() if you have for instance only one UILabel and in collectionView(_:cellForItemAt:) you change its value each time (it’d be label.text = nil in prepareForReuse() directly changed in collectionView(_:cellForItemAt:) into label.text = "myValue", which is extra work not necessary.

    Once you understood this, the current issue with your code is this one:

    func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {
        let cell = collectionView.dequeueReusableCell(withReuseIdentifier: "SimpleThumbnailView", for: indexPath) as! ThumbnailsCollectionViewCell
        //First Async call (I guess it's async)
        ImageCache.default.retrieveImageInDiskCache(forKey: urlString) { result in
            //Second async call
            DispatchQueue.main.async {
                cell.thumbnailImage.image = image
            }
        }
    }
    

    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 use associated_object, which let them to add an extra value to an object (the UIImageView). 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):

    func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {
        let cell = collectionView.dequeueReusableCell(withReuseIdentifier: "SimpleThumbnailView", for: indexPath) as! ThumbnailsCollectionViewCell
        if indexPath.row < thumbnails.count {
            let urlString = thumbnails[indexPath.row].thumbnail ?? ""
            cell.thumbnailImage.kf.setImage(with: urlString, placeholder: UIImage(named: "DefaultImage"), options: []) //Might need a if let url here
            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()
            }
        return cell
    }
    

    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 the RetrieveImageResult value.

    setImage(with: urlString, placeholder: UIImage(named: "DefaultImage"), options: [], completionHandler: { result in 
        switch result {
            case .failure(let error):
                print(error)
            case success(let imageResult):
                //check imageResult, maybe cacheType, source, etc.
        }
    }
    

    Another issue you had was reloading of the whole collectionView.

    Login or Signup to reply.
Please signup or login to give your own answer.
Back To Top
Search