Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#88 Jetpack Paging 3 support #91

Closed
wants to merge 1 commit into from

Conversation

krossovochkin
Copy link

My vision on how to implement support for new paging 3 jetpack library.
Relates to #88

Main obstacle is that AdapterDelegates are heavily based on "List" in their implementations.
And new Paging library doesn't have final List underneath.
But seems there is no need in that, because the only features that could be needed from List - are getting item by position and maybe size of a list.
All of this is a public API of adapter and can be exposed under some interface, which I called AdapterItemProvider.
But it creates a new separate parallel branch of implementations.

What's inside this PR:

  • some updates on build.gradle (as e.g. Java 1.8 compatibility is required)
  • base interface for AdapterItemProvider and Abs...Delegate for it
  • ViewBinding DSL for new Abs...Delegate
  • LayoutContainer DSL for new Abs...Delegate
  • Delegation adapter implementation for Paging 3 Jetpack library along with support of new AdapterItemProvider interface
  • Sample

What's missing:

  • naming most likely not final and could be updated
  • docs (javadoc, readme, ...)
  • something else?

Current PR I think still is a WIP. Most part is done, but some polishing might be needed.

@sockeqwe, hope you'll be able to review it (or find someone to review)
Thank you

@krossovochkin
Copy link
Author

Hi @sockeqwe

Did you have a chance to look at this PR?

Thank you

@sockeqwe
Copy link
Owner

Hi @krossovochkin
Sorry for the delay, I am on vacation and didn't check my mails frequently.
Will take a look at it now, but I think it could end up in a major rework of this library as I also wanted to address other issues like #89 and #83.

Although I didn't look at your code changes yet, I think this is something that could lead to bigger changes and longer time until it's released (just to manage expectations)


import androidx.annotation.Nullable;

public interface AdapterItemProvider<T> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I started with this library I was thinking about introducing something like this but then I decided to not do that as it felt like reinventing the Adapter class (which has that API already but Adapter class in general has no clear single responsibility this is where my motivation for introducing something like this came from).

I then thought it would be easier to let people subclass adapter and override the corresponding methods. What do you think? Do I miss something?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is that AdapterDelegate has access to "datasource", which is by default is List in the library.
I don't know whether accessing datasource from delegate is really something that is needed in API. Seems I haven't used it.
But if it is needed, then for me it seems that List is not the best default option. Default option should be some "random-access" interface.
List provides such access. Adapter provides such access.
But not all adapters have List as a "datasource".

That is why I think some general interface most likely should be added. Because List is too specific and limits for example usage with Paginated adapters, which don't have List under the hood.

We could pass Adapter into Delegate directly. But yes, Adapter has too many responsibilities, so it feels it should be hidden under some interface.

Naming is the question though.
Maybe "AdapterDatasource", "DelegationDatasource" or so could be better than AdapterItemProvider

@krossovochkin
Copy link
Author

Hi @sockeqwe
No problem. Was also on a vacation recently)

I would be happy if lib had 5 release with covering more pain points.
Personally, I don't expect this PR to be merged. I've just wanted to accelerate discussion and provide my vision. As code shows more than textual description of the problem.

@krossovochkin
Copy link
Author

@ibrahimyilmaz thanks for review
But some things were implemented in a way they are in other places in the lib. Sample part of the lib is not a perfect piece of code by itself. Tried not to introduce some new approaches in this implementation.
Instead of writing probably same comments I'll add few reactions )

Thanks again for your time

) : AbsItemAdapterDelegate<I, T, AdapterDelegateLayoutContainerViewHolder<I>>() {

override fun isForViewType(item: T, items: AdapterItemProvider<T>, position: Int): Boolean = on(
item, items, position

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here should be !=null check.
override fun isForViewType(item: T, items: AdapterItemProvider<T>, position: Int): Boolean { return item != null && on(item, items, position) }

holder: AdapterDelegateLayoutContainerViewHolder<I>,
payloads: List<Any>
) {
holder._item = item as Any
Copy link

@PavelSidyakin PavelSidyakin Sep 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

item as Any
Sometimes it causes a crash that null cannot be cast to non-null type.
Consider adding the non-null check:
if (item != null) { holder._item = item holder._bind?.invoke(payloads) }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When placeholders are using, item can be null. So maybe it's better to make _item nullable.

@vanniktech
Copy link
Contributor

I've also gotten interested in paging 3 support. For the moment I'm forced to use Paging 2. @sockeqwe can the community somehow help with the refactoring (adapterdelegates5) which would enable this as well as #89 & #83?

@sockeqwe
Copy link
Owner

Hey,
to be honest I dont think adapterdelegates5 will ever be created. I think with Jetpack Compose this library will become more and more obsolete.

I personally done use pagination library, thus don't know much about it. I'm happy to add any artifact to adapter delegates to support pagination 3, I just don't have (and want) invest much of my time in pagination 3. I hope you understand. So what I can do is the following: I can take until mid of February a look into Pagination 3 library APIs then get back again to this PR to check what is missing / required to add support.

@vanniktech
Copy link
Contributor

Totally understandable & sounds good.

I don't know much about pagination either. I've had to use it for my RSS Reader though as some users have 40+ sources and loading hundreds of entries from the database isn't the best way (either OOM or result from sqlite database was too large). I'm creating my paging source from SQLDelight using their pagination support and wanted to keep my adapter delegates so that I didn't need to change that part too.

@ahulyk
Copy link

ahulyk commented Jun 6, 2023

Any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants