Wrong CollectionView cell image while downloading and saving file async with completionBlock

Question

I realize there is a lot of these questions already, but none of them seem to address this case.

The CollectionView reads file names from files array and either loads images from Documents directory or downloads them, displays and saves to Documents. Works perfectly smooth and without any additional libraries, HOWEVER when scrolling fast several cells receive wrong images. Calling [collectionView reloadData] with any action or scrolling back reloads these wrong images to good ones.

I imagine that it's related to cell reusing with async image assigning, but how does one solve this issue? CollectionView and custom cell defined in Storyboard. Images are stored on server behind Basic Authentication, so most reuse solutions don't apply.

- (UICollectionViewCell *)collectionView:(UICollectionView *)collectionView cellForItemAtIndexPath:(NSIndexPath *)indexPath
{
    BrowseCollectionViewCell *cell = [self.collectionView dequeueReusableCellWithReuseIdentifier:@"MyCell" forIndexPath:indexPath];

    NSString *fileName = [files objectAtIndex:indexPath.item];
    NSString *filePath = [thumbDir stringByAppendingPathComponent:fileName];

    if ([[NSFileManager defaultManager] fileExistsAtPath:filePath])
    {
        cell.imageView.image = [UIImage imageWithContentsOfFile:filePath];
    }
    else //DOWNLOAD
    {
        NSString *strURL = [NSString stringWithFormat:@"%@%@", THUMBURL, fileName];
        NSURL *fileURL = [NSURL URLWithString:strURL];
        cell.imageView.image = [UIImage imageNamed:@"placeholder.jpg"];

        [self downloadFromURL:fileURL to:filePath
              completionBlock:^(BOOL succeeded, UIImage *image) {
                  if (succeeded)
                  {                          
                      cell.imageView.image = image;
                  }
              }];
        }
    }
    cell.imageName = fileName;
    return cell;
}

- (void)downloadFromURL:(NSURL*)url to:(NSString *)filePath completionBlock:(void (^)(BOOL succeeded, UIImage *image))completionBlock
{
    NSString *authStr = [NSString stringWithFormat:@"%@:%@", LOGIN, PASS];
    NSData *authData = [authStr dataUsingEncoding:NSASCIIStringEncoding];
    NSString *authValue = [authData base64EncodedStringWithOptions:NSDataBase64Encoding64CharacterLineLength];

    NSMutableURLRequest *request = [NSMutableURLRequest requestWithURL:url cachePolicy:NSURLRequestReloadRevalidatingCacheData timeoutInterval:30];

    [request setValue:[NSString stringWithFormat:@"Basic %@", authValue] forHTTPHeaderField:@"Authorization"];

    [NSURLConnection sendAsynchronousRequest:request queue:[NSOperationQueue mainQueue]
        completionHandler:^(NSURLResponse *response, NSData *data, NSError *error) {
         if (!error)
         {
             UIImage *image = [[UIImage alloc] initWithData:data];
             [data writeToFile:filePath atomically:NO];
             completionBlock(YES, image);
         } else {
             completionBlock(NO, nil);
         }
     }];
}

I've tried many modifications, but none of them seem to solve the problem. Currently, the effect is that new cells appearing during fast scroll are being changed more than once and some of them always end up with wrong image, pointing at some reuse issue.

Thanks in advance for any help and suggestions.

Answer 1

The issue is that when scrolling fast, the cell might be dequeued and reused for another cell by the time the asynchronous request finishes, thereby updating the wrong cell. So, in your completion block, you should make sure the cell is still visible:

[self downloadFromURL:fileURL to:filePath completionBlock:^(BOOL succeeded, UIImage *image) {
    if (succeeded) {                          
        BrowseCollectionViewCell *updateCell = (id)[collectionView cellForItemAtIndexPath:indexPath];
        if (updateCell) {                       // if the cell is still visible ...
            updateCell.imageView.image = image; // ... then update its image
        }
    }
}];

Note, this UICollectionView method cellForItemAtIndexPath should not be confused with the similarly named UICollectionViewDataSource method collectionView:cellForItemAtIndexPath:. The UICollectionView method cellForItemAtIndexPath returns a UICollectionViewCell if the cell is still visible, and returns nil if it is not.

By the way, the above assumes that the NSIndexPath for this cell cannot change (i.e. that it's not possible that additional rows could have been inserted above this row while the image was being asynchronously retrieved). That sometimes is not a valid assumption. So, if you wanted to be careful, what you really should do is go back to the model and recalculate what the NSIndexPath for this model object, and use that when determining the appropriate updateCell reference.

---

Ideally, once you address the above, there are a few optimizations you could make to this process:

1. If the cell is reused while the previous request is still running, you might want to cancel that prior request. If you don't and you scroll quickly past, say, 200 cells, now showing, say, cells 201 through 221, the requests for those 20 currently visible cells will be queued up and won't display until the prior 200 requests finish.

   To be able to cancel prior requests, though, you can't use sendAsynchronousRequest. You'd have to use a delegate-based NSURLConnection or NSURLSessionTask, which are cancelable.

2. You probably should be caching your images. If your currently visible cells all have their images and you then scroll them off and back on, you appear to be re-requesting them. You should first see if you've already retrieved the image, and if so, use that, and only if you don't have one in your cache, re-issue the request.

   Yes, I know you're using the version of the file in persistent storage to cache, but you might want to cache to RAM, too. Typically people use NSCache for this purpose.

This is a lot to change. If you want help doing this, let us know. But far easier is to use the UIImageView category in SDWebImage or AFNetworking, which does all of this for you.

Answer 2

You need to keep some contextual information somewhere. In other words, in your completion block, check if the cell should really contain downloaded image or not. If not, do not assign it. For example image URL. You can do this ...

• add property NSURL *imageURL to your cell object
• before your downloadFromURL:... store fileURL to imageURL property of the cell
• in completion block, compare if fileURL matches cell.imageURL and if it matches, set the image otherwise do not set it
• and nil your cell imageURL property in prepareForReuse

... or you can move your loading code into your cell class and assign there just URL of the image, etc. Many ways to do this.

Answer 3

You need to get reference of cell in block replace:

[self downloadFromURL:fileURL to:filePath
              completionBlock:^(BOOL succeeded, UIImage *image) {
                  if (succeeded)
                  {                          
                      cell.imageView.image = image;
                  }
              }];

with

 [self downloadFromURL:fileURL to:filePath
              completionBlock:^(BOOL succeeded, UIImage *image) {
BrowseCollectionViewCell *innerCell = (BrowseCollectionViewCell *)[collectionView cellForItemAtIndexPath:indexPath];
                  if (succeeded)
                  {                          
                      innerCell.imageView.image = image;
                  }
              }];