Improve performance: new bubbleImage and avatarImage dataSource

[jessesquires]

UPDATED: 1 Sept. 2014

Bubble images

• make bubbleImageView part of the cell prototype
• add new JSQMessageBubbleImageDataSource (instead of using UIImageView)
• add concrete JSQMessagesBubbleImage class that conforms to new protocol
• add unit tests for this new object
• documentation for new protocol and object
• refactor JSQMessagesBubbleImageFactory to return JSQMessagesBubbleImage objects
• refactor view controller to use this new model
• update unit tests

Avatar images

• make avatarImageView part of the cell prototype
• add new JSQMessageAvatarImageDataSource (instead of using UIImageView)
• add concrete JSQMessagesAvatarImage class that conforms to new protocol
• add unit tests for this new object
• documentation for new protocol and object
• refactor JSQMessagesAvatarFactory to return JSQMessagesAvatarImage objects
• refactor view controller to use this new model
• update unit-tests

---

Per discussion in #313

This will prevent the need to continually add/remove subviews and update constraints in cells during configuration.

This will also provide a better solution for those wishing to do async avatar loading.

Other relevant issues: #306, #281

[erysaj]

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.

The other issue I'm concerned with is extensibility. We are doing a redesign of the app (that's the reason for switching to this library) and among other things we need different kind of messages:
media (images or video with displayed thumbnail) and "system" ones ("%username% left chat", etc).
I'm not 100% that is not possible without modifying the library code, but deadline waits for nobody, so other developers are hacking this library to meet requirements (including appearance).
I'm afraid this will end up with situation where it is extremely difficult to pull in fixes or give something
back so having an easily extendable library would benefit everybody.

[jessesquires]

@erysaj - i'm still not 100% clear on what you are suggesting. reloading only visible cells should not be a huge issue, and this will only happen once for each avatar once it is downloaded. (unless you are clever about this and reload only when all avatars have finished downloading)

Firstly, this library is not concerned with anything regarding networking. It does not (and will not) do anything with downloading, caching, async loading, etc. It will only provide the hooks you need to display your data.

by the time an avatar image is downloaded the underlying data source may change and the index is no more correct.

I don't understand what you mean. Maybe my proposal wasn't clear, but the data source will not change. To reiterate the flow:

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 as it dequeues new cells and configures them

This would basically function like my demo project -- which keeps all the avatars in a dictionary. It is similar to @kenn's code sample but would be wrapped in a better, more robust API.

Does that make sense? What are the potential issues you see with this in your use-case?

Also, can you provide some more concrete examples/code explaining what you are suggesting?

[erysaj]

I don't understand what you mean. Maybe my proposal wasn't clear, but the data source will not change.

I meant the underlying 'messages data source'. I can receive new messages from server, so if I
memorized item's index path when requesting avatar (to update only that cell upon completion)
this update will cause that index path to refer to another cell

Firstly, this library is not concerned with anything regarding networking. It does not (and will not) do anything with downloading, caching, async loading, etc. It will only provide the hooks you need to display your data.

I completely agree to that point and suggest to move it further: do not make any unnecessary decisions for the library's user. For example, currently you have a restriction on how does message
cell should look. It is pretty customisable (you can have bubble or coloured background, show avatars or not, add labels, change fonts/paddings etc.), but still you assume for the user a particular cell's appearance and ask collection view data source for parameters of that appearance.
I want additionally display an image instead of text message (I believe there's a feature request for something similar) --- I'm stuck; want some center-aligned "system" message --- the same.
I propose to shift responsibility for cell's appearance to the library's user, but provide ready-to-use
implementations (like JSQMessagesCollectionViewCell) for those who is content with default look
or just wants to tweak some parameters. Make simple easy and complex possible.

I can provide some code snippets if needed, but that'll be later --- it's pretty late in my timezone

[jessesquires]

@erysaj - ah! i got it. my proposal does not keep track the indexPath. it doesn't care. maybe i should begin this on a new branch to give a better example.

---

For example, currently you have a restriction on how does message
cell should look.

I think it is very customizable right now. You can adjust all the things you mentioned. Also, you can provide your own bubble images, or not use images at all.

I want to display an image instead of text message

This is not supported, but will be eventually. See #223

want some center-aligned "system" message

Can't you use one of the cell labels for this?

---

Overall, it sounds like you have some really specific use-cases. I think the way this library is built works perfectly for 80% of the people using it. :) which is the goal.

[erysaj]

i should begin this on a new branch to give a better example.

No need for that, it is clear. I developed the index path theme after pointing out that re-creating all visible cell instead updating a few image views seem to much

You can adjust all the things you mentioned. Also, you can provide your own bubble images, or not use images at all.

I know, I listed the things that can be customised (maybe not all) just to point out that despite being very customisable cell is still restricting --- you can't display an "image" message for instance :)

Can't you use one of the cell labels for this?

That's a dirty hack. It breaks a data model --- 'system' message is no worse than any other and besides there could be a few of them in a row.

it sounds like you have some really specific use-cases.

I wouldn't say so. Exchanging photos/video seems to be in trend of modern chat apps (hence 'image' messages) and having group conversations is a common feature --- skype has it for years (hence 'system')

works perfectly for 80%

why not raise to 85%? especially considering the fact that newcomers will be forced to fix existing bugs :)

[jessesquires]

Like I said, media messages is on the ToDo list, and high priority. See #172 and #76 for some background on this control, v4.0 had an even more restricting implementation. v5.0 was a complete re-write that solved all of the issues with v4.0.

having group conversations is a common feature

This is 100% supported.

RE: system messages

That's a dirty hack. It breaks a data model --- 'system' message is no worse than any other and besides there could be a few of them in a row.

Fair enough and makes sense. Sounds like a feature request! #321 opened. This should actually be a simple addition, the framework is modular enough to easily adapt to something like this.

When I mentioned that you had specific use-cases, I was referring to the system messages. I don't think most people want/need this feature. Of course, everyone wants media messages. I am very aware of this. But this is non-trivial to design well.

why not raise to 85%? especially considering the fact that newcomers will be forced to fix existing bugs :)

Let's not forget that I have spent enormous amounts of my time building this library for free and making it open-source which gives you the opportunity to fix bugs.

[erysaj]

Let's not forget that I have spent enormous amounts of my time building this library for free and making it open-source which gives you the opportunity to fix bugs.

I appreciate that and you have my thanks for the great library.
I'm just trying to make it even better. In the created feature request you propose to provide a special
treatment for a particular sender name; my goal is that such trivial feature won't require any alteration of the framework. I suppose it would be more constructive if I re-work demo application in some branch to illustrate my vision. Hope you are not in a rush for developing v6

BTW, earlier you mentioned plans to make some refactoring according to "lighter view controllers".
Do you happen to have written summary of what exactly you were planning to do?

[jessesquires]

@erysaj 👍 see the objc.io issue on lighter view controllers. what i want to do is refactor out the data source object like you see there.

[jessesquires]

Completed and set for 6.0 release. See branch release-6.0.

See release 6.0 milestone details here

[marcoscurvello]

hey @jessesquires why did you remove the stroked bubbles from the 6.0 beta1 release mate? ;(

[jessesquires]

@marcoscurvello - I didn't think they were being used! Sorry, I'll add them back.

[marcoscurvello]

@jessesquires thanks so much dude, I've just tested the beta2 loving these changes ;)

[jessesquires]

@marcoscurvello - #534