Skip to content

Feature - Realtime Database Pagination #1603

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Mar 26, 2019
Merged

Feature - Realtime Database Pagination #1603

merged 5 commits into from
Mar 26, 2019

Conversation

PatilShreyas
Copy link
Contributor

Here, I have implemented FirebaseRecyclerPagingAdapter that supports Pagination of Firebase Database with RecyclerView as issue #1601

@PatilShreyas
Added support for Firebase Database Pagination as firebase#1601
f479b6f 
Implementation of Paging for Firebase Database
@PatilShreyas PatilShreyas requested a review from samtstern as a code owner March 20, 2019 18:41
@PatilShreyas PatilShreyas changed the title Added support for Firebase Database Pagination as #1601 Feature - Support for Firebase Database Paging as #1601 Mar 20, 2019
@samtstern samtstern changed the title Feature - Support for Firebase Database Paging as #1601 Feature - Realtime Database Pagination Mar 21, 2019
samtstern
samtstern reviewed Mar 21, 2019
View reviewed changes
Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

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

I did a first round of comments. In general this looks pretty good but I think we should make the logic almost exactly identical to the Firestore version, rather than having smaller differences througout.

See if you can go through each file, compare it to the Firestore version, and make them as similar as posisble.

database/src/main/java/com/firebase/ui/database/paging/FirebaseDataSource.java Outdated
* managing our own thread pool or requiring the user to pass us an executor.
*/
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public class FirebaseDataSource<T> extends PageKeyedDataSource<String, T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at the Firestore implementation, we have the DataSource deal only in snapshots rather than putting in Class<T>. I think it's simpler that way and also I would like for this code to be as similar to the Firestore code as possible to enable future maintenance.

@PatilShreyas
Copy link
Contributor Author

Thank you @samtstern
I will make changes soon as you suggested.

@PatilShreyas
Added features as suggested in firebase#1603 and issue 1601
460b726 
Implemented exactly as Firestore version
@PatilShreyas PatilShreyas mentioned this pull request Mar 22, 2019
Closed
@PatilShreyas
Copy link
Contributor Author

@samtstern I have updated code as you suggested.
Issue #1601

@samtstern
Copy link
Contributor

@PatilShreyas on first glance this looks great! Thank you for such a well-formed PR. I will hopefully have time to do a final thorough review later today.

@PatilShreyas
Copy link
Contributor Author

Thank You!

@PatilShreyas
Minor changes in code firebase#1603 issue firebase#1601
ba4fdae 
Fixed some code and minor changes.
@PatilShreyas
Added 'retry()' and 'refresh()' methods PR firebase#1603 issue fireba…
c090b91 
…se#1601

Added functionalities :
1. retry() : Retry the loading of data in RecyclerView after failure.

2. refresh() : Reload data in RecyclerView by clearing recent loaded data.
samtstern
samtstern reviewed Mar 25, 2019
View reviewed changes
Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

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

This looks 99% good! Some really small comments and then I am happy to merge this.

@PatilShreyas
Copy link
Contributor Author

@samtstern I have fixed the code as your suggestions.

@samtstern
Copy link
Contributor

@PatilShreyas thanks for this! Here's my plan:

  • I am going to release version 4.3.2 today since it contains important bug fixes
  • Then I will create a new branch for version-4.4.0-dev
  • I'll merge your PR into that branch, and then we can put a few more things together and release 4.4.0

@PatilShreyas
Copy link
Contributor Author

@samtstern Okay!
Thank You!

@samtstern samtstern changed the base branch from version-4.3.2-dev to version-4.4.0-dev March 26, 2019 16:26
@samtstern samtstern added this to the 4.4.0 milestone Mar 26, 2019
@samtstern samtstern merged commit d170531 into firebase:version-4.4.0-dev Mar 26, 2019
@samtstern samtstern mentioned this pull request Mar 26, 2019
Merged
@PatilShreyas PatilShreyas deleted the version-4.3.2-dev branch March 26, 2019 17:56
@PatilShreyas PatilShreyas restored the version-4.3.2-dev branch March 26, 2019 17:56
@PatilShreyas PatilShreyas deleted the version-4.3.2-dev branch March 26, 2019 18:13
@PatilShreyas PatilShreyas restored the version-4.3.2-dev branch March 26, 2019 18:25
@PatilShreyas PatilShreyas mentioned this pull request Mar 30, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samtstern samtstern samtstern approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@PatilShreyas @samtstern