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

Update Paging Library to alpha4 #360

Merged
merged 4 commits into from
Jan 25, 2018

Conversation

wkranich
Copy link
Contributor

In case you don't have time to get to it:

Update Android Paging Library to alpha4 and update the epoxy-pagingsample to fix breaking changes.

Also updated room while I was in there.

@elihart
Copy link
Contributor

elihart commented Dec 13, 2017

Thanks, I actually did exactly this, but the paging sample app doesn't seem to work at all once I made the changes. I spent a while trying to debug it and couldn't figure out what broke. I was hesitant to merge the update until I figured out why the paging wasn't working

@wkranich
Copy link
Contributor Author

Huh, strange, I tested the sample app and have it working now. Though initially, had some issues but figured out that I needed to use DataSource.Factory in the DAO and call create in order to pass it to the PagedList.Builder. A slight deviation from how it was done before where it would be created just by defining the type.

@elihart
Copy link
Contributor

elihart commented Dec 14, 2017

Yep, I did the same. The first page loads (80 items), but when I scroll down to the bottom no more items load. The callback for paged list updates is not called anymore like it used to be.

I also tried this approach with an observer

LivePagedListBuilder<Int, User>(
                    db.userDao().allUsers(),
                    PagedList.Config.Builder().run {
                        setEnablePlaceholders(false)
                        setPageSize(40)
                        setInitialLoadSizeHint(80)
                        setPrefetchDistance(200)
                        build()
                    }
            )
                    .build()
                    .observe(
                            this@PagingSampleActivity,
                            Observer<PagedList<User>?> { t -> pagingController.setList(t) }
                    )

but the observer is only called with the initial page, not when more items are requested.

I'm not sure if this is an issue with how I am setting up the paging library, or an issue with how the paging controller is requesting data updates.

Does the sample activity load more for you when you scroll down?

@wkranich
Copy link
Contributor Author

You're right, I'm not seeing page updates either. That being said, I've upgraded it locally in my project, and in my use case, which is a network based paging component, I'm not having any issues. So my thought would that it's something to do with how we are initializing the Room db or the PagedList from the db.

@wkranich
Copy link
Contributor Author

wkranich commented Dec 14, 2017

Found the problem. Looks like disabling placeholders breaks paging
https://issuetracker.google.com/issues/70573345

I've updated the PR to support placeholders, but understand if you don't want to merge this

@elihart
Copy link
Contributor

elihart commented Dec 14, 2017

@wkranich thanks for tracking that down! That's unfortunate...

I'm conflicted, this library doesn't work great with placeholders because it assumes that each item in the list represents an epoxymodel in order to track positioning within the global list. That doesn't work for null items where there is no epoxymodel added.

I would like to improve this to support placeholders - which would allow loading states, but probably won't have time for a while.

In the meantime I want to release this update to unblock people, but encouraging people to use placeholders might create subpar functionality.

@elihart
Copy link
Contributor

elihart commented Dec 14, 2017

btw, if you or anybody else would like to contribute to improving the epoxy paging support it would be much appreciated. I would like to support placeholders, programmatic scrolling, and restoring saved state scroll position. There is probably also a lot of room to tweak the logic and optimization for how the epoxy model window is rebuilt on scroll

@wkranich
Copy link
Contributor Author

Totally understood. I can temporarily host a local copy of PagingEpoxyController to unblock for now, until Google fixes the placeholder bug.

@elihart
Copy link
Contributor

elihart commented Jan 12, 2018

Fixed in an internal build, will be released in a new alpha version of Paging soon.

I'll update as soon as it is released

@elihart
Copy link
Contributor

elihart commented Jan 24, 2018

@wkranich new version is out. Mind bumping your libs and then I'll merge this? If you don't have time I can close this and put up my own

@wkranich
Copy link
Contributor Author

Sure thing!

Copy link
Contributor

@elihart elihart left a comment

Choose a reason for hiding this comment

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

Thanks! I am going to remove the placeholders because I'd like to recommend that pattern instead, but I can follow up with that.

Much appreciation for the help!

@elihart elihart merged commit ecc8a83 into airbnb:master Jan 25, 2018
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.

None yet

2 participants