pedrovgs/Renderers

Making use of Collection instead of AdapteeCollection

Lukard opened this issue ยท 7 comments

Hi everyone!

I would like to know if we could change RVRendererAdapter and RVListRendererAdapter collection type to Collection<T> instead of AdapteeCollection<T>.

It causes me trouble in the following situation. I would like to update just one item from the list instead of performing a full diffUpdate. Since there is no set method in AdapteeCollection I can only do the following:

List<T> list = (List<T>) collection;
list.set(position, newElement);
notifyItemChanged(position);

The drawback of this approach is that it is performing a cast over collection assuming that the implementation inherits from List.

Another way of tackling this issue would be copying all elements into aList<T> and performing the set operation there. But that is even worse given the original collection is already a List<T> and we would be doing a useless copy.

Are there any arguments against using Collection instead of AdapteeCollection?

@pedrovgs If you agree with the solution I can open a PR to propose the changes so you can take a look!

@Lukard "instead" may not be the best solution if we want to avoid a breaking change. However, I guess if we would like to implement this change we could make the adapter to receive either an AdapteeCollection or just a Collection. However, why don't you use ListAdapteeCollection which already extends from ArrayList?

@pedrovgs I use ListAdapteeCollection but RvRendererAdapter and RVListRendererAdapter are holding an instance of AdapteeCollection (instead of ListAdapteeCollection). So if I want to add/extend those adapters with new functionality that manipulates the collection I need to cast it to List<T> or something like that, since AdapteeCollection provide much less functionality than Collection.

Cast it to List may be dangerous because someone may pass an instance to the adapters that is different than ArrayList or any other non-Collection types.

I know it may be a breaking change, but since the library hasn't been updated for a long time I would say it is pretty stable and a breaking change shouldn't be a big deal.

Would like to see a PR with requested changes so we can discuss about way to do it?

It's been a long time since I don't use this library, I'm using compose now...However, when I used it I always used RVRendererAdapter API and I wanted to change the class to extend from collection. I think we can can just avoid the breaking change by making the adapter extend from collection and receive a collection in construction too. If you don't mind, could you please change the RendererAdapter and RVRendererAdapter to implement Collection?. You can mark AdapteeCollection as deprecated as well ๐Ÿ˜ƒ

Thank you @pedrovgs I'll do that

Offtopic: BTW lucky you that could start using Jetpack compose. I want it to introduce it at New Work, but since we are working in a really big app we need it to be in an stable state. Unfortunately it is still in alpha. Hope it reaches to a stable release soon.

Hi @pedrovgs!
It has been a lot of time since my last message, but priorities changed at work and I couldn't tackle the issue until now.

I need to use List instead of Collection to make it work, all details are described in the PR. Let me know if you have any doubts.

Here you have the PR: #58

Thanks a lot ๐Ÿš€

Releasing a new version right now.