There is a fundamental problem with your code. Consider fetching of images in view controller. The problem that you can face with current code is if it would take a long time to load an image, there is possibility(quite hight in case user scrolls quickly) that cell can be dequeued for rendering of another row but image from previous row will be just download and displayed which will cause inconsistent UI.
There are 2 places in ELAppCell that cause the described bug:
- (void)configureCellWithAppEntry:(AppEntry *)appEntry{ that we already fixed- (UIImageView *)buildThumbnailImageView: (NSString *)imageUrl you should remove(or comment out) all dispatch calls. Calls to dispatch... are redundant and cause the bug; TableViewController in charge of loading images. Should you need to load small images then consider loading them in view controller as well.
There is also a serious bug in your ELAppListTableVC, your should remove appListTableView property and all places in the code where you use it. And in receiveEvent:notification method you should call [self.tableView reloadData]; instead of reloading appListTableView. Basically you had two table views laying one on another, where appListTableView was added manually. NOTE: your ELAppListTableVC already inherited from UITableViewController so it already has tableView that get automatically instantiated by the view controller.
Code that loads image can be moved to view controller or be a part of a model class. Details bellow:
Load image in view controller
After image gets loaded you need to update the cell with particular index path because the index path represent AppEntry instance for which image was downloaded. It means that you should ask table view to return cell that represents that index path. It may turn that cell currently invisible, it this case you will get nil. So in callback block that gets invoked on main queue once download completed you need to have a reference to that index path. Since code is better that hundred of worlds, here is how it should look:
- (UITableViewCell *)tableView:(UITableView *)tableView
cellForRowAtIndexPath:(NSIndexPath *)indexPath {
ELAppCell *elAppCell = (ELAppCell *)[tableView dequeueReusableCellWithIdentifier:CellIdentifier];
AppEntry *appEntry = [self.appEntries objectAtIndex:indexPath.row];
if (!elAppCell) {
elAppCell = [[ELAppCell alloc] initWithStyle:UITableViewCellStyleDefault reuseIdentifier:CellIdentifier appEntry:appEntry];
}
[elAppCell configureCellWithAppEntry:appEnty];
self.thumbnailImageView.image = nil;
dispatch_queue_t fetchQ = dispatch_queue_create("ConfigureCell", NULL);
dispatch_async(fetchQ, ^{
NSURL *address = [NSURL URLWithString:appEntry.smallPictureURl];
UIImage *image = [UIImage imageWithData:[NSData dataWithContentsOfURL:address]];
dispatch_async(dispatch_get_main_queue(), ^{
ELAppCell *updateCell = (ELAppCell *)[tableView cellForRowAtIndexPath:indexPath];
if (updateCell) { // if nil then cell is not visible hence no need to update
updateCell.thumbnailImageView.image = image;
}
});
});
return elAppCell;
}
after refactoring this configureCellWithAppEntry: code should look like this:
- (void)configureCellWithAppEntry:(AppEntry *)appEntry {
self.appNameLabel.text = appEntry.name;
self.appArtistLabel.text = [NSString stringWithFormat:@"By %@", appEntry.artist];
}
Much better code but still youэдд be re-downloading images that were already downloaded when user scrolls back.
Move image loading code to model class (further improvement of the code above)
You perform download each time cell gets displayed, i.e. image that was already downloaded will be downloaded again. To cache images you can add thumbnailImage property to AppEntry class and load this image lazily, only when user need is. To do that you can update your AppEntry class with the code bellow:
AppEntry.h
@interface AppEntry // if it is CoreData object here should be AppEntry: NSManagedObject
@property (nonatomic, readonly) UIImage *thumbnailImage;
- (void)loadThumbnailImage:(void (^)())completionBlock;
@end
AppEntry.m
@interface AppEntry ()
@property (nonatomic, readwrite) UIImage *thumbnailImage;
@end
@implementation AppEntry
- (void)loadThumbnailImage:(void (^)())completionBlock {
dispatch_queue_t fetchQ = dispatch_queue_create("ConfigureCell", NULL);
dispatch_async(fetchQ, ^{
NSURL *address = [NSURL URLWithString:self.smallPictureURl];
UIImage *image = [UIImage imageWithData:[NSData dataWithContentsOfURL:address]];
dispatch_async(dispatch_get_main_queue(), ^{
self.thumbnailImage = image;
completionBlock();
});
});
}
@end
of course view controller should be updated as well:
- (UITableViewCell *)tableView:(UITableView *)tableView
cellForRowAtIndexPath:(NSIndexPath *)indexPath {
ELAppCell *elAppCell = (ELAppCell *)[tableView dequeueReusableCellWithIdentifier:CellIdentifier];
AppEntry *appEntry = [self.appEntries objectAtIndex:indexPath.row];
if (!elAppCell) {
elAppCell = [[ELAppCell alloc] initWithStyle:UITableViewCellStyleDefault reuseIdentifier:CellIdentifier appEntry:appEntry];
}
[elAppCell configureCellWithAppEntry:appEnty];
if (appEntry.thumbnailImage) {
elAppCell.thumbnailImageView.image = appEntry.thumbnailImage;
} else {
[appEntry loadThumbnailImage: ^{
elAppCell.thumbnailImageView.image = appEntry.thumbnailImage;
}];
}
return elAppCell;
}
and as soon as we have thumbnail image in AppEntry model we can update cell code to use it in configureCellWithAppEntry: method:
- (void)configureCellWithAppEntry:(AppEntry *)appEntry{
self.appNameLabel.text = appEntry.name;
self.appArtistLabel.text = [NSString stringWithFormat:@"By %@", appEntry.artist];
self.thumbnailImageView.image = appEntry.thumbnailImage;
}
Even with image cache we can have re-downloading when user can scroll up and down so quickly that thumbnailImage can be nil for the AppEntry instance because image has not been downloaded yet. So we need to handle image downloading state some how, details bellow.
Optimization of image downloading and caching
To solve problem with sequential calls to loadThumbnailImage:, when we do perform a new download while we have one in the progress already we need to handle downloadInProgress state. AppEntry need to be tweaked to:
@implementation AppEntry {
dispatch_group_t _thumbnailDownloadGroup;
dispatch_queue_t _thumbnailFetchQueue;
dispatch_once_t _thumbnailQOnceToken;
BOOL _loadingThumbnail;
}
- (void)loadThumbnailImage:(void (^)())completionBlock {
if (self.thumbnailImage) { // return immediately since we have image
completionBlock();
} else if (_loadingThumbnail) { // return when previously requested download complete
dispatch_group_notify(_thumbnailDownloadGroup, _thumbnailFetchQueue, ^{
completionBlock();
});
} else { // download image
dispatch_once(&_thumbnailQOnceToken, ^{
_thumbnailDownloadGroup = dispatch_group_create();
_thumbnailFetchQueue = dispatch_queue_create("ThumbnailDownload", NULL);
});
__weak typeof(self) weakSelf = self;
dispatch_group_async(_thumbnailDownloadGroup, _thumbnailFetchQueue, ^{
NSURL *address = [NSURL URLWithString:weakSelf.smallPictureURl];
UIImage *image = [UIImage imageWithData:[NSData dataWithContentsOfURL:address]];
dispatch_async(dispatch_get_main_queue(), ^{
self.thumbnailImage = image;
completionBlock();
});
});
}
}
@end
I have not run the examples to test but I hope you can get the idea.
