Improve springiness cpu usage

[erysaj]

This is to address an issue #281

[jessesquires]

Thanks @erysaj! Overall looks pretty good.

My first question is about JSQMessagesImageViewSource - I'm not opposed to this refactoring (or something similar) but I'm not sure I want a change this large at the moment.

I have some plans to refactor to use a separate JSQMessagesDataSource object (in order to achieve and promote Lighter View Controllers). It seems like the JSQMessagesImageViewSource change would be best suited for when that refactoring happens.

I've only glanced through your changes, but it looks like you simply return early if the imageView already has an image? Is that correct?

[erysaj]

I don't quite understand what early return you are talking about, and it doesn't sound correct. Could you point out in the code?
In fact that particular optimisation is pretty important. Adding a subview and pinning its edges causes
constraints to be re-evaluated, and if I'm not mistaken that ended up in computing text view's intrinsicSize which neglects caching computed text sizes. In fact the old API can be used, by taking
only image and highlightedImage from supplied image view, but I can't see how setting an avatar asynchronously can be achieved (sad truth is that chat apps do need asynchronous avatars in 95% of cases). So I'd like to keep the whole thing.
Your refactor plans sound promising. Beside giving a way more flexibility, that would also save a few % of CPU usage --- currently the time is wasted to populate custom layout attributes and then for applying them though I suppose all configuration (fonts, colors, geometry) could be made right after dequeueing cell for reuse or even at creation time (by using more reuse identifier to uniquely identify
cells with a particular colours/font theme). When do you plan to do that refactoring? I may assist to bring that to life earlier

[jessesquires]

@erysaj - I just looked through your code more and now understand what is happening.

The reason the API asked for UIImageView objects was to allow consumers to use, for example, SDWebImage for asynchronous loading of avatars.

For the BubbleImages, this was just a convenient (lazy? 😢) way to package an image and highlightedImage.

However, as you note, this isn't the best for performance in a collection view.

This PR has given me some even better ideas on how to handle this. But it needs to happen in distinct steps. Here's what I would like to do:

1. For this PR
   • Remove all of your changes regarding the JSQMessagesImageViewSource
   • Remove your changes to the cell xib files
   • Keep lazily updating cell constraints and properties
   • Keep JSQMessagesCollectionViewFlowLayoutInvalidationContext
   • Keep removal of concurrent enumeration
2. After merging that, I'll release v5.0.3
3. Then, I'll implement the following:
   • Make the bubble image view part of the cell prototype
   • Implement my JSQMessagesDataSource
   • This might include a separate JSQMessagesBubbleImageDataSource, not sure yet
   • Implement a JSQMessagesAvatarDataSource, similar to what you've done here
4. These API changes will require a release of v6.0.0 (to adhere to semantic versioning), because of this I may take the opportunity to make additional API changes.

---

More info on JSQMessagesAvatarDataSource:

Your JSQMessagesSimpleImageViewSource was a good start, but I think it can be improved and be more modular. It doesn't need an imageSize property, because the collectionViewLayout has that information. However, it should have a placeholder image. It would work the following way: if the avatarImage is nil, use the placeholderAvatar, otherwise use the avatarImage. I think this would remove the need for using something like SDWebImage as consumers of the API could use AFNetworking to retrieve their avatar in the background and once downloaded, set the avatarImage property on their JSQMessagesAvatarDataSource and then the collection view would use that instead of the placeholder. (This would all be encapsulated in the API, so you only have to worry about setting those 2 properties on your data source object.)

Let me know your thoughts!

/ cc: @robj, @cswelin, @hactar, @wdcurry, @kenn, @hohl
I would like your opinions here, too!
I think that's everyone who has been pretty involved in this project, please tag anyone I missed.

[kenn]

As to the topic on JSQMessagesAvatarDataSource:

... I think this would remove the need for using something like SDWebImage as consumers of the API could use AFNetworking to retrieve their avatar in the background and once downloaded, set the avatarImage property on their JSQMessagesAvatarDataSource and then the collection view would use that instead of the placeholder.

I think we still need some kind of cache layer, so the same avatar can be reused between multiple cells. And I don't think that's a responsibility of this library.

Personally I use UIImageView+AFNetworking, and with its bare-minimum simplicity, I don't feel the need for additional support for avatars. If that was what you're asking...

[jessesquires]

@kenn - nice! i didn't know that category on AFNetworking existed. 👍

with that, there is definitely no need to support custom UIImageView subclasses like SDWebImage.

back to my proposal -- it would allow caching. that would be the purpose of the avatarImage method. the protocol would look something like this:

@protocol JSQMessagesAvatarDataSource <NSObject>

@required
- (UIImage *)placeholderImage;
- (UIImage *)avatarImage;

@end

the library would ask the data source for avatarImage first, if it does not exist, it would use placeholderImage instead. exactly how the downloading, caching, etc. is accomplished would be up to you.

presumably your placeholder image would be on disk (or you could use my avatar factory to create an avatar with the user's initials), and you could encapsulate downloading in your JSQMessagesAvatarDataSource object. Once downloaded, you save (cache) the image in your data source object: self.avatarImage = [UIImage imageWithName:@"myImage"]; After that, reload the collection view (or only its visible index paths.

the goal here would be to make avatarImageView part of the cell prototype so that the library is not constantly adding subviews and new constraints to collection cells.

would an API like this affect your use of UIImageView+AFNetworking? presumably you could still encapsulate the use of this category if you wanted, or simply use the AFNetworking API that this category uses internally (without the NSCache of course, but this shouldn't be an issue with avatars).

[erysaj]

I don't like the idea of having to reload cells after image got fetched, just setting image on the UIImageView is a way simpler.
If API is to be changed, I'd propose design which does not need an awkward JSQMessagesAvatarProvider at all. Here is what I have in mind:

• reduce the JSQMessagesCollectionViewDataSource protocol: request only JSQMessageData
  and cell's reuse identifier for a given index path.
• library's user must manually register all required collection view cell classes; each class must
  conform to

@protocol JSQMessagesCollectionViewCell <NSObject>

@required
-(void)configureWithMessageData:(id<JSQMessegeData>)message;
@end

• sender in JSQMessageData is not just a sender's name: avatar image is basically its attribute,
  so sender's type must be more generic, I guess just id
• JSQMessagesCollectionViewCellIncoming and JSQMessagesCollectionViewCellOutgoing adopt
  that protocol, provide access to important internal details (e.g. avatarView, avatarSize, etc.) and
  allow easy customisation of layout (for example determine constraints' constants by invoking
  class methods). Thus the mentioned classes can be easily subclassed for default look, but
  library's user is not limited in any way by them.

The main thing happens in -[JSQMessagesCollectionViewCell configureWithMessageData:]
which is obviously called right after the cell has been dequeued from collection view.
Each cell type is designed for displaying a particular message kind, so user's code
"knows" how to get avatar for an opaque sender.
For instance:

• it could have avatarImage property, so cell just updates its avatarImageView with proper UIImage
• there could be avatarURL, so cell uses some conveniency UIImageView category (SDWebImage, AFNetworking or your own bicycle) to set avatar asynchronously.

Besides it might be convenient to have some prefixes in JSQMessageData protocol's methods:
that way you can use CoreData entities provided by NSFetchedResultsController directly by
implementing required method in a category (current common property names are likely to clash)

[erysaj]

@jessesquires - in case you haven't noticed, I have reverted breaking API changes and xib modifications. You can merge for the current release

[jessesquires]

@erysaj - thanks! i just ran your code and it looks good. 👍

i'm going to make a few changes after merging and then update the pod.

[kenn]

would an API like this affect your use of UIImageView+AFNetworking?

UIImageView+AFNetworking offers its own placeholder image, and I feel it's the right place to put the responsibility for a placeholder image, as it would be a natural extension for any async image loading library. So there will be an overlap.

I think the crux of this matter is how we provide a hook when image loading is finished. Right now, my message.speaker.picture.imageView getter method has something like this:

[_imageView setImageWithURLRequest:request
                  placeholderImage:[UIImage imageNamed:@"default-avatar"]
                           success:^(NSURLRequest *request, NSHTTPURLResponse *response, UIImage *image)
{
    weakImageView.image = image;
    dispatch_async(dispatch_get_main_queue(), ^{
        [[NSNotificationCenter defaultCenter] postNotificationName:@"avatarLoaded" object:nil];
    });
} failure:nil];

then, JSQMessagesViewController subclass has:

- (void)viewDidLoad
{
    ...
    [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(refreshAvatars) name:@"avatarLoaded" object:nil];
}

- (void)refreshAvatars
{
    for (NSIndexPath *indexPath in self.collectionView.indexPathsForVisibleItems) {
        Message *message = [self.messages objectAtIndex:indexPath.row];
        JSQMessagesCollectionViewCell *cell = (JSQMessagesCollectionViewCell *)[self.collectionView cellForItemAtIndexPath:indexPath];
        cell.avatarImageView.image = message.speaker.picture.imageView.image;
    }
}

I hope there is a better way... one quick idea is to expose an API to monitor a given object property change using KVO (in my case message.speaker.picture.imageView.image), then handle the refresh avatars in visible index paths automatically.

[jessesquires]

@kenn - thanks for all the info! very interesting.

one question, what are you returning from avatarImageViewForItemAtIndexPath: ?

i think this is essentially the pattern that i'm suggesting with the API change:

1. you'll provide my library with a JSQMessagesAvatarDataSource as described above
2. download your avatar in the background (during this, return nil from avatarImage)
3. in the meantime, the library displays the placeholder (from placeholderAvatar)
4. once you get the avatar, update your JSQMessagesAvatarDataSource to return that value
5. the library can provide a reloadVisibleCells helper method that you can call to refresh the view
6. the next time that a cell is dequeued, your JSQMessagesAvatarDataSource will already have and return the avatarImage
7. from here on, avatarImage will be displayed in the collectionview

so the library still knows absolutely nothing about downloading, caching, etc. it is simply checking if avatarImage is not nil. if it is nil, then it defaults to placeholderAvatar.

this seems like a really good way to do this. am i missing something?

[kenn]

one question, what are you returning from avatarImageViewForItemAtIndexPath: ?

I don't know if it's the right way, but here's my code:

- (UIImageView *)collectionView:(JSQMessagesCollectionView *)collectionView avatarImageViewForItemAtIndexPath:(NSIndexPath *)indexPath
{
    Message *message = self.messages[indexPath.row];
    UIImage *image = [JSQMessagesAvatarFactory avatarWithImage:message.speaker.picture.imageView.image diameter:kIconSize];
    UIImageView *imageView = [[UIImageView alloc] initWithImage:image];
    return imageView;
}

Your suggestion generally sounds good, but I don't have sufficient expertise to verify that (I'm not a full time iOS dev), and as always, the devil is in the detail, so. :)

[jessesquires]

ok, cool. thanks @kenn !

[erysaj]

@jessesquires great, thanks!

Please consider the solution I suggested earlier and give users the freedom to set/update avatars in any convenient way :-)

You propose to reload visible cells where you just need to update image a few image views. Trying
to do that for individual index paths is also not obvious: by the time an avatar image is downloaded
the underlying data source may change and the index is no more correct.

In the chat app I develop an approach similar (though it is used with table views) to the one I outlined is used and here is how it works:

• there's an intermediate layer on top of AFNetworking and image cache to coalesce the requests for the same image URL: when you enter the chat screen there may be several messages from the same sender and when corresponding cells are requested you now can just request sender's avatar by URL without worrying --- there'll be exactly one HTTP request.
• after the cell is dequeued it is configured with corresponding domain-model object. This includes
  asynchronous setting of an avatar image by URL on appropriate image view and cancelling any previous request to populate that particular view. Also sender's avatar "hash" value is observed
  via KVO to detect when sender changes avatar. When that happens, cell requests an updated avatar image for its avatarView.

[jessesquires]

new issue opened for this discussion, let's move the conversation there: #319

@erysaj, please repost your last comment on that thread :) i have more thoughts, but want to move to that thread.