ReactiveCocoa / ReactiveViewModel

Model-View-ViewModel, using ReactiveCocoa
MIT License
1.96k stars 259 forks source link

Binding asynchronously loaded images from a view-model #16

Closed andrewsardone closed 10 years ago

andrewsardone commented 10 years ago

I'm tinkering with an MVVM implementation, and I'm curious to hear others' input.

We need to present a table view UI of people, showing a single label in each cell for the person's name, and a single UIImageView for the person's avatar. We need to fetch each person's avatar from some web service.

The standard Cocoa Touch MVC solution usually involves queueing up some background requests and coordinating their returned images with the appropriate UITableViewCell subviews that are on screen. This is done asynchronously and kicked off via delegate or data source methods, usually handled within the view controller.

It seems like an MVVM solution would push the avatar image retrieval into a view-model.

So, we have a very simple model:

@interface Person : NSObject

@property (nonatomic, copy) NSString *firstName;
@property (nonatomic, copy) NSString *lastName;
@property (nonatomic, strong) NSURL *avatarURL;

@end

And I suppose we have the following view-model:

@interface PersonEntryViewModel : NSObject

/// A UI-ready combination of a `Person` `firstName` and `lastName`
@property (nonatomic, copy) NSString *name;

@property (nonatomic, strong) UIImage *avatar;

- (instancetype)initWithPerson:(Person *)person;

@end

Then we could have, say, a UITableViewCell implementation:

@interface PersonTableCell : UITableViewCell
@property (nonatomic, strong) PersonEntryViewModel *viewModel;
@end
@implementation PersonTableCell

- (id)initWithStyle:(UITableViewCellStyle)style reuseIdentifier:(NSString *)reuseIdentifier {
    self = [super initWithStyle:style reuseIdentifier:reuseIdentifier];
    if (self == nil) return nil;

    RAC(self.textLabel, attributedText) = RACObserve(self, viewModel.name);
    RAC(self.imageView, image) = RACObserve(self, viewModel.avatar);

    return self;
}

@end

This seems like a good strategy, but I'm wrestling with how the PersonEntryViewModel provides a UIImage for the avatar. It seems like +[NSURLConnection rac_sendAsynchronousRequest:] could be the answer:

@implementation PersonEntryViewModel

- (instancetype)initWithPerson:(Person *)person
{
    self = [super init];
    if (!self) return nil;

    // omitted: binding self.name based on person.firstName & person.lastName

    NSURLRequest *request = [NSURLRequest requestWithURL:person.avatarURL];
    RAC(self, avatar) = [[[NSURLConnection rac_sendAsynchronousRequest:request]
        reduceEach:^id(NSURLResponse *response, NSData *data){
            return [[UIImage alloc] initWithData:data];
        }]
        deliverOn:[RACScheduler mainThreadScheduler]];

    return self;
}

@end

…but this will kick off the request on initialization of the view-model as opposed to, say, when the view-model is set on the UITableViewCell within -tableView:cellForRowAtIndexPath:. There also isn't any cancellation of the request when the cell scrolls out of view.

Should the view-model actually expose a RACSignal as opposed to a simple image property? Should the avatar binding between the cell and the view-model occur somewhere else other than the cell's initializer?

Thanks in advance for any input anyone has! If this is too generic of a problem, or poorly outlined, please close it out and I'll try to reevaluate.

ashfurrow commented 10 years ago

The solution to the problem of the avatar download being kicked off as soon as the view model is initialized is solved by using the view model's active property and setting it to YES when the table view is about to display that cell (there's a UITableViewDelegate method for that). You can then set it to NO when the cell disappears, which could cancel the request. However, you'll be unable to use the rac_sendAsynchronousRequest: method because it's not cancellable; abstract out the network logic into its own class using the NSURLConnectionDelegate protocol. There are a few other opportunities to make your approach a little better, though.

You're not storing the image data anywhere, so it'll have to be re-downloaded every time a cell is displayed, which can get costly for the user's data plan. You should at least be caching them by url in a static NSCache instance, but more ideally storing them in the Person model. Second, UIImage's initWithData: method is expensive. I'd recommend this approach for decompressing on a background thread (note: simply using initWithData: on a background thread will not actually decompress the JPEG). As it is now, you'll likely see dropped frames while scrolling the table view.

Best of luck, and let us know how it goes!

andrewsardone commented 10 years ago

All good feedback, @AshFurrow – thanks! Using the active property makes sense. I'll take a look at that and return if I have any other questions.

Thanks again!

andrask commented 10 years ago

FYI the same upgrade has beend done on FRP repo as I noticed the performance hit too (https://github.com/AshFurrow/FunctionalReactivePixels/issues/27 & https://github.com/AshFurrow/FunctionalReactivePixels/pull/30) Note that the performance hit may still be present as long as the code is compiled in debug mode.