6

I'm using Kingfisher to download and cache my images. I'm using the custom class below CustomImageView that extends ImageView. I also am using my custom UITableViewCell to call the loadImage method within the didSet property in my CustomCell class file.

The below method loadImage is where all the magic happens.

class CustomImageView : UIImageView {
var lastUrlLoaded: String?

func loadImage(url:String) {
    lastUrlLoaded = url
    ImageCache.default.retrieveImage(forKey: url, options: nil) { (image, cacheType) in
        if let image = image {
            self.image = image
            return
        }
        else {
            guard let url = URL(string: url) else { return }
            self.kf.setImage(with: url, placeholder: nil, options: nil, progressBlock: nil) {
                (image, error, cacheTye, _) in
                if let err = error {
                    self.kf.indicatorType = .none
                    DispatchQueue.main.async {
                      self.image = #imageLiteral(resourceName: "no_image_available") 
                    }
                    print ("Error loading Image:", err)
                    return
                }

                if url.absoluteString != self.lastUrlLoaded { return }
                if let image = image {
                    ImageCache.default.store(image, forKey: url.absoluteString)
                    DispatchQueue.main.async {
                        self.image = image
                    }
                }
            }
        }
    }
}

// Custom TableViewCell -- NewsCell.file
 class NewsCell: UITableViewCell {
var article: Article? {
    didSet{
        guard let image_url = article?.image_url else { return }
        _image.kf.indicatorType = .activity
        _image.loadImage(url: image_url, type: "news")
      }
   }
}

 // METHOD in my NewsController
 override func tableView(_ tableView: UITableView, cellForRowAt indexPath: 
  IndexPath) -> UITableViewCell {        
   let cell = tableView.dequeueReusableCell(withIdentifier: cellId, for: indexPath) as! NewsCell
      cell.article = articles[indexPath.item]
return cell
}

I'm having an issue when scrolling fast through my cells, some images that have loaded are being used for the wrong cells. I've researched and read other post but no answers helped me. I understanding it's dealing with the background task but just not sure how to fix it. If someone can help or point me in the right direction it would be much appreciated.

Solutions I've seen on stackoverflow and tried:

  1. prepareForReuse in custom cell setting the image to nil.
  2. setting the image to nil at cellforrowatindexpath.
antdwash
  • 516
  • 4
  • 10
  • Can you clarify this: "when I'm scrolling fast .. images that have loaded are being used for the wrong cells". So, if you don't scroll and just wait for everything to load, it all looks perfect? And if you scroll slowly, it all looks perfect? The images appear misplaced only when you scroll fast WHILE asynchronous image loads are happening. Is that the case? Are your rows constant height? Are all images the same height? If the image substitution is such that the row height should change, you might need to use begin/end updates. – ozzieozumo Nov 09 '17 at 05:36
  • So if I scroll slowly or don't scroll through the view until the first set of cells load their images everything loads as expected -- when I say scrolling fast, I'm referring to scrolling up and down through the view will the asynchronous images are loading in the background. All my images are the same height and don't affect the height of the cells. – antdwash Nov 09 '17 at 06:20
  • Ok, show your cellforrowatindexpath and didset from your cell class. – ozzieozumo Nov 09 '17 at 13:22
  • @ozzieozumo I've updated the snippet to show the methods you asked – antdwash Nov 09 '17 at 15:07
  • Ok, looks like you are on the right track. Now, you said that you have tried "setting the image to nil at cellforrowatindexpath" but it didn't work. It should work. Make sure that you use this order: a) dequeue b) nil image c) set article d) return cell. Maybe update your question with the code that you tried for this technique. – ozzieozumo Nov 09 '17 at 15:21

3 Answers3

9
override func prepareForReuse() {
    super.prepareForReuse()
    imageView.kf.cancelDownloadTask() // first, cancel currenct download task
    imageView.kf.setImage(with: URL(string: "")) // second, prevent kingfisher from setting previous image
    imageView.image = nil
}
Howard Sun
  • 91
  • 1
  • 3
8

Problem

When you scroll fast, you are reusing cells. The reused cells contain KF images which may have a running download task (initiated from the call to kf.setImage), for the image in the recycled article. If that task is allowed to finish, it will update your new cell with the old image. I will suggest two ways to fix it.

Answer 1 - Kill Recycled Download Tasks

Refer to the Kingfisher cheat sheet, Canceling a downloading task.

The cheat sheet shows how to stop those tasks in didEndDisplaying of your table view. I quote it here:

// In table view delegate
func tableView(_ tableView: UITableView, didEndDisplaying cell: 
UITableViewCell, forRowAt indexPath: IndexPath) {
    //...
    cell.imageView.kf.cancelDownloadTask()
}

You could do it there or in the didSet observer in your cell class. You get the idea.

Answer 2 - Don't kill them, just ignore the result

The other approach would be to allow the download task to finish but only update the cell image if the downloaded URL matches the currently desired URL. Perhaps you were trying to do something like that with your lastUrlLoaded instance variable.

You have this in the completion handler of kf.setImage:

if url.absoluteString != self.lastUrlLoaded { return }

This is kind of what I mean, except you are not setting that variable properly to make it work. Let's imagine it was called "desiredURL" instead, for sake of explanation. Then you would set it in the didSet observer:

_image.desiredURL = image_url

And test it in the completion handler of kf.setImage:

// phew, finished downloading, is this still the image that I want?
if url != self.desiredURL { return }

Notes

If it were me, I would definitely go with the second approach (it has the positive side effect of caching the downloaded image, but also I don't trust canceling tasks).

Also, it seems that you are trying to do the hard way what kf.setImage does for you, i.e. loading the cache etc. I'm sure that you have your reasons for that, and either way you would still need to deal with this issue.

ozzieozumo
  • 426
  • 2
  • 8
  • thanks for the help — I gave it a shot but still the same issue :( - not sure why because in theory it should work. Every dev knows coding doesn’t work like it’s suppose to the first time lol – antdwash Nov 10 '17 at 02:51
  • I'd be happy to take a look at your code if you post an update. Sort of interesting. – ozzieozumo Nov 10 '17 at 03:09
  • @ozzieozumo Thanks for your answer. I used your first approach, canceling previous downloads. The only thing i did different is call `cell.imageView.kf.cancelDownloadTask()` in `cellForRow` instead of `didEndDisplaying`. – Arnold Plakolli Mar 21 '18 at 13:54
  • When I try using cancelDownloadTask, the console is spammed with logs along the lines of: `Task <08624AEE-9A76-47EE-AFBC-8836CCDD5213>.<10> load failed with error Error Domain=NSURLErrorDomain Code=-999 "cancelled" UserInfo={NSErrorFailingURLStringKey=https://image.tmdb.org/t/p/w45/2vrnA7cg6D7EmOlwivGsuHnk2f9.jpg, NSErrorFailingURLKey=https://image.tmdb.org/t/p/w45/2vrnA7cg6D7EmOlwivGsuHnk2f9.jpg, _NSURLErrorRelatedURLSessionTaskErrorKey=( "LocalDataTask <08624AEE-9A76-47EE-AFBC-8836CCDD5213>.<10>".` I'm not sure how to use cancelDownloadTask without this error being printed. – elveatles Dec 11 '18 at 23:04
0

in your model check if the url not exist set it as nil, then in cell check your url if it is exist set it's image, else set a placeholder for that.

if article?.image_url != nil{
  _image.kf.indicatorType = .activity
  _image.loadImage(url: image_url, type: "news")
}else{
  _image = UIImage(named: "placeholder")
}
Negar
  • 91
  • 6