image loading on the wrong UITableViewCell when scrolling

Question

I am loading set of images for a particular cell and it works fine only if I wait until all images is loaded, I am using thread to fetch the images from the web the problem is when I scroll the table view while the loading process is still on going it will generate the image in the wrong cell position. I know this is the wrong position because I also set a label for each image sets. Any help would be appreciated thanks in advance!!!!

here are the sample code.

- (void)viewDidLoad{
[super viewDidLoad];
/* loads xml from the web and parses the images and store it in core data(sqlite)
   it uses NSFetchedController protocol for inserting cell after saving into core data
*/
[self loadFromXmlSource];
}

I have 3 sections that uses 3 different prototype cell in storyboard the one I am having problem is the photo section.

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath         *)indexPath
{
Media *mediaModel = [_fetchedResultsController objectAtIndexPath:indexPath];
NSString *CellIdentifier = @"MediaCellNoImage";
if ([[mediaModel.category lowercaseString] isEqualToString:@"photos"]) {
    CellIdentifier = @"MediaGalleryCell";
} else if ([[mediaModel.category lowercaseString] isEqualToString:@"videos"]) {
    CellIdentifier = @"MediaCellNoImage";
} else if ([[mediaModel.category lowercaseString] isEqualToString:@"podcasts"]) {
    CellIdentifier = @"MediaCell";
}

MediaGalleryCell *cell = [tableView dequeueReusableCellWithIdentifier:CellIdentifier];
if (cell == nil) {
    cell = [[MediaGalleryCell alloc] initWithStyle:UITableViewCellStyleDefault reuseIdentifier:CellIdentifier];
}
// I am also having a problem with image duplication so I always delete all images
if([cell.reuseIdentifier isEqualToString:@"MediaGalleryCell"]) {
    for(id subview in cell.scrollView.subviews){
        [subview removeFromSuperview];
    }
}

[self configureCell:cell atIndexPath:indexPath];
return cell;
}

here is the configure cell where I spawn a thread for image download

-(void)configureCell:(MediaGalleryCell *)cell atIndexPath:(NSIndexPath *)indexPath
{

Media *mediaModel = [_fetchedResultsController objectAtIndexPath:indexPath];
if ([[mediaModel.category lowercaseString] isEqualToString:@"photos"]) {
        NSEnumerator *e = [mediaModel.attachments objectEnumerator];
        Attachment *attachment;
        // iterate all images in a particular cell
        while ((attachment = [e nextObject])) {
                NSDictionary *objects = [[NSMutableDictionary alloc] initWithObjectsAndKeys:

                                         attachment.identifier, @"identifier",
                                         attachment.thumb, @"thumb",
                                         attachment.filename, @"filename",
                                         cell, @"cell", 
                                         nil];

                // for each image spawn a thread i am also passing the cell in order to set it there.
                [NSThread detachNewThreadSelector:@selector(loadImageInScrollView:)
                                         toTarget:self withObject:objects];
        }   
} else {
    // code for other section
}


cell.titleLabel.text = mediaModel.title;
cell.contentLabel.text = mediaModel.content;
}

here is the method that is in thread

-(void)loadImageInScrollView:(NSDictionary *)objects
{
MediaGalleryCell *cell = [objects valueForKey:@"cell"];
SBCacheFile *cacheFile = [self getCache];
NSString *finalIdentifier = [NSString stringWithFormat:@"%@_s", [objects valueForKey:@"identifier"]];

// here is where I fetched the image and cache it locally
UIImage *image = [cacheFile getCachedFileWithUrl:[objects valueForKey:@"thumb"]
                                        withName:[objects valueForKey:@"filename"]
                                        andStringIdentifier:finalIdentifier];

NSDictionary *obj = [[NSDictionary alloc] initWithObjectsAndKeys:
                     cell, @"cell",
                     image, @"image",
                     nil];
//after fetching the image I need to send it back to the main thread in order to do some UI operation
[self performSelectorOnMainThread:@selector(setImageViewForGallery:) withObject:obj waitUntilDone:YES];

}

here is the last piece of code where I add the image as a subview of a property of a cell which is a scrollView think of it like an image carousel in a cell.

-(void)setImageViewForGallery:(NSDictionary *)dictionary
{
MediaGalleryCell *cell = [dictionary valueForKey:@"cell"];
UIImage *image = [dictionary valueForKey:@"image"];

UIImageView *imageView = [[UIImageView alloc] initWithImage:image];

UIImageView *lastImageView =[[cell.scrollView subviews] lastObject];
if (lastImageView != nil) {
    [imageView setFrame:CGRectMake((lastImageView.frame.origin.x + lastImageView.frame.size.width + 10.0f), 
                                   5.0f, 70.0f, 70.0f)];
} else {
    [imageView setFrame:CGRectMake(5.0f, 5.0f, 70.0f, 70.0f)];
}

 // This is where I add the imageView
 [cell.scrollView addSubview:imageView];

 UIImageView *lastViewForSize = [[cell.scrollView subviews] lastObject];
    [cell.scrollView setContentSize:CGSizeMake(lastViewForSize.frame.origin.x + lastViewForSize.frame.size.width + 0.5,cell.scrollView.frame.size.height)]; 
}

Answer 1

This is the classical pitfall of UITableView. When you scroll a cell out of view, the cell will be reused to display different data. It basically pops out at the top, will be fill with new data and reappears at the bottom.

However, your background processes are not notified about this. So when the image finally loads, the attach it to a table cell that has been reused for something different.

So to fix it, you need to either cancel the image loading if a table cell is no longer visible or at least notify the thread that the table cell has been reused and that it is no longer possible to assign the image once it has loaded. This can happen either when the cell is scrolled out of view or when the cell is reused to display different data.

In my app, I'm using a central image cache. It uses up to four threads for loading images. And it keeps a list of all table cells waiting for an image to be loaded. If one of these table cells appears in a different image request, the earlier request is modified so it doesn't update the table cell but just puts the image into the central cache.

Answer 2

First, this is incredibly dangerous:

// for each image spawn a thread i am also passing the cell in order to set it there.
[NSThread detachNewThreadSelector:@selector(loadImageInScrollView:)
                         toTarget:self withObject:objects];

This can fork a huge (and unbounded) number of threads. What is likely happening is that as you scroll around, threads complete for cells that have already been reused, and so stomp on their new data.

There are several things you should be doing here to improve things. First, you seldom want to use detachNewThreadSelector:toTarget:withObject:. Instead, use an NSOperationQueue, a GCD queue, or just a single worker NSThread and performSelectorOnThread:...

Next, you've mixed your model and view logic. Your current way is storing model data (the list of images) inside the views (cells). This doesn't work when cells are reused. Views are not for storing data.

You should have a model that keeps track of all the images and where they go. In your case, that's probably just an array of arrays (rows/images). The table view datasource will always return a cell with the current contents of the appropriate rows of the array.

Whenever the model changes (because a new image was downloaded), your table view delegate should call reloadRowsAtIndexPaths:withRowAnimation: to let the table view know that row has changed. If that row happens to be needed, then (and only then) the table view will ask the datasource for a new cell, which you'll update with the new images. This approach is cleaner, more stable, faster, and uses less system resources.