Skip to content

Conversation

thatfiredev
Copy link
Member

@thatfiredev thatfiredev commented Jul 29, 2021

This PR should:

  • update the database module to use Android Jetpack's Paging 3
  • update the sample app to use Paging 3
  • update the docs

See #1982

@thatfiredev thatfiredev requested a review from samtstern as a code owner July 29, 2021 16:11
@google-cla google-cla bot added the cla: yes label Jul 29, 2021
@thatfiredev thatfiredev mentioned this pull request Jul 29, 2021
7 tasks
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.

Approve, just one comment you may want to address before I merge. Thank you for this!

}

return Single.fromCallable(() -> {
Tasks.await(task);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will throw if the Tasks fails:
https://developers.google.com/android/reference/com/google/android/gms/tasks/Tasks#await(com.google.android.gms.tasks.Task%3CTResult%3E)

So we can't rely on getting to the next line. Although I think it's fine since you re-throw at the bottom, but I just don't think you'll ever get that far.

Also we can probably remove the task.isSuccessful nesting since we know if we even get to the next line, the task is successful.

Copy link
Member Author

Choose a reason for hiding this comment

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

@samtstern Interesting, I didn't know that.

I have wrapped it around a try/catch so that developers using FirebaseUI can get the original cause instead of await's ExecutionException.

@samtstern samtstern merged commit da7beaf into firebase:version-8.0.0-dev Jul 30, 2021
@samtstern samtstern added this to the 8.0.0 milestone Jul 30, 2021
@thatfiredev thatfiredev deleted the rpf-rtdb-paging-3 branch July 30, 2021 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants