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

Jetpack Paging v3 support #194

Closed
wants to merge 11 commits into from
Closed

Jetpack Paging v3 support #194

wants to merge 11 commits into from

Conversation

kedzie
Copy link

@kedzie kedzie commented Aug 6, 2020

#193

Added support for Jetpack Paging v3 with LoadState/retry()/refresh() support

ezgif com-video-to-gif

@evant
Copy link
Owner

evant commented Aug 8, 2020

This is a great start thanks!. For the LoadState stuff, I think it might make sense to push it up a level into a MergeObservableList instead of using the ConcatAdapter/LoadStateAdapter. This should make it play better with the other parts of the lib. Working on a branch to do just that, will put it up when I've got something. Still need to think about how to connect the retry callback though.

@kedzie
Copy link
Author

kedzie commented Aug 9, 2020

I added a commit which adds a itemBinding based LoadStateAdapter. But I see you point, if we can merge all into the list then we can provide bindings all in the same adapter. instead of managing ConcatAdapter.

Either way the itemBinding based LoadState allows consumer to do this

val footerItemBinding = OnItemBindClass<LoadState>().apply {
        map<LoadState.Error>(BR.item, R.layout.network_state_item_error)
        map<LoadState.Loading>(BR.item, R.layout.network_state_item_progress)
    }.toItemBinding()

I suggest we provide a tatarka interface with retry() and refresh() methods. And we can bind an instance of that to the consumer's loadstate ItemBinding.

@kedzie
Copy link
Author

kedzie commented Aug 9, 2020

I just pushed a commit to tie in retry() and refresh().
I used the factory method

val loadStateItemBinding = LoadStateItemBindingFactory { callback: PagedListCallback ->
        OnItemBindClass<LoadState>().apply {
            map<LoadState.Error>(BR.item, R.layout.network_state_item_error)
            map<LoadState.Loading>(ItemBinding.VAR_NONE, R.layout.network_state_item_progress)
        }.toItemBinding().bindExtra(BR.callback, callback)
    }

it works. I'd like to circle back to the MergeObservableList and see if I can help out.

@kedzie
Copy link
Author

kedzie commented Aug 9, 2020

Challenges I see with the MergeObservableList is how the consumer will specify the bindings. If it is all a single itemBinding no way to distinguish between header and footer elements.

For example: (no way to know if the error/progress bindings are for the header or footer)

val allItems = LoadStateItemBindingFactory { callback: PagedListCallback ->
        OnItemBindClass<Any>().apply {
            map<ImmutableItem>(BR.item, R.layout.item_immutable)
            map<LoadState.Error>(BR.item, R.layout.network_state_item_error)
            map<LoadState.Loading>(ItemBinding.VAR_NONE, R.layout.network_state_item_progress)
        }.toItemBinding().bindExtra(BR.callback, callback)
    }

Be nice if consumer could specify separate bindings for items and the header/footer. Then we could make a way to combine those into a single ItemBinding. Then we could just maintain 3 ObservableLists, all merged together.

Only other issue is that our code would basically duplicate logic from LoadStateAdapter. Instead of notifying adapter it would update the underlying observableList.

@evant
Copy link
Owner

evant commented Aug 9, 2020

Alright, pushed up #195, thoughts?

@kedzie
Copy link
Author

kedzie commented Aug 9, 2020

I dig it. posted some comments related to retry/refresh. I like the custom OnItemBindLoadState; i was thinking same regarding the merged list approach. could we pass in a variable name for the retry callback in the OnItemBindLoadState constructor and then bind it the list? We could pass in the list reference when it is created in the binding adapter.

Only concern is that by not extending LoadStateAdapter we are forced to duplicate the logic it contained. it is pretty simple but it is something you will have to keep in sync if it changes.

@kedzie kedzie closed this Aug 11, 2020
@kedzie
Copy link
Author

kedzie commented Aug 11, 2020

Pushed new PR #196

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.

2 participants