-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Firestore paging adapter #1178
Firestore paging adapter #1178
Conversation
Change-Id: I9890f5c629ce972624a5cf0fcd5276e83f48d1e5
Change-Id: I4c640d09a0c585f046df32b76b00bd48db94aacb
Change-Id: Id78e7ceee80b670424de4c93a5be2e819aac6fbb
Change-Id: I278fe22606f979011409b37938092763358b4dac
Change-Id: I3b4970fc36dfbbae6045e796577d67e10f015fde
Change-Id: I4102d360594aeb75183f0ac8f1d300826de3b47f
cc @puf |
Change-Id: I2264ada2d13cf9c667680f3a278f7b5e2edf0cb0
Change-Id: I75cd769dd3f9cd4c6d6cc1de13d411646827430c
Change-Id: I2be8141c859ba84994b4d9eeb624d2893aa02c3a
Change-Id: Ifc3ab274dcd2df184901452b26e1fe1cbe68f581
@morganchen12 at a high level, how does this compare to what you've tried on iOS? |
@samtstern Without looking at any code, my main concern is that the first issues people are going to submit will be something along the lines of "Paginated data not updating". Also, it'd make me so sad to see a Firebase app with the infamous refresh button. 😢😂 Have you looked at the Paging Library? When I first played with it, I couldn't get things to work out nicely, but that was way back in alpha 1. It should remove a bunch of boilerplate and support changes by passing in new lists and doing diffs on a background thread. The focus is more directed towards supporting Room, but I think we could make it work. Do you want to investigate, or should I? |
@SUPERCILEX I can definitely take a look there. I thought it was Room-only so hadn't really looked. I have a feeling that we're going to have so many issues if we try to maintain listeners on the paginated data ... but we can take another look at that. |
@SUPERCILEX ok the Paging library is f***ing awesome! I am at least not embarassed since my design was a lot like theirs, but theirs is way cleaner and best of all I don't have to maintain it. Before I go that route entirely:
|
@samtstern Haha, I thought you might like it! The Architecture Components team is doing an amazing job of perfecting and investing in all of their APIs so I'd expect nothing less from them. 😆 To answer your questions:
|
@SUPERCILEX thanks for all the info. Even if they don't get to As for |
Change-Id: I7b7a7da48530817dfad36afd428af7f553e12cd0
Change-Id: Ia8711bfb5b4f8dde11d4ef4630fabf70b945d45e
Change-Id: I5935b36ea30207d0e4b2a8091a258d108bad9d5a
Change-Id: Ic50dfc07ddd51bf318d655df6fae20ab0af28dcc
Change-Id: I863d67d604b1ef768a591d2456e71a7f94730a53
The |
Change-Id: I5da44eeb96a9acf4adf76e0ade70b9ae43a4308e
Change-Id: Idaa7d98c4c4712354ff6a4ed10883f8dc78fa898
Change-Id: Icaa6a6e24a65b87bc6d0e5c49059f72f99522d6c
Change-Id: I0d0d763c7e03c7d37390a2a61bd34195e12f2ca6
@SUPERCILEX what would you rather see as far as code splitting:
I lean towards (1) since we've done it before and it has the same size savings for people who don't care about paging. |
Yeah, I think you're right. My only concern is that people are going to be confused as to why we have 2 adapters. If I were just browsing the source code, I'd be like "why wouldn't I want a paging adapter?". At some point if the AC team adds an unload callback we might be able to provide realtime updates and deprecate the old adapter or something, but for now, if we go with #1, we need to have really clear docs. 📜😁 Yay, nay? |
Change-Id: I435f0713b3cfa4d1dc9c242defa2a4f0c2d216e1
@SUPERCILEX yep I think careful documentation is going to be the main answer here. As to "why wouldn't I want a paging adapter", that's a good question! I think most people will want paging unless they need to observe non-added/removed changes to individual items in real time. |
Huh, yeah, I guess it depends on the type of app you're building. I'm biased cuz my thing is cross-device collaboration, but I'd imagine most people are using Firestore for the database part primarily. Anyway, SGTM. 👍 I'll go through another round of review tomorrow or Friday. 👏 |
Change-Id: I6095b5df433c155c993100031b2633ee1670fb2d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samtstern This is looking really snazzy!
I'm putting myself into the shoes of future me trying to use this for my app, and here are the issues I've come up with (that I don't think should block the PR, but that we should keep in mind):
- Config changes. This is the huge beasty here: FirebaseUI 3: Adapter items reload, although there is no change in the data #947 is going to be made waaaaaay worse, especially since we don't have a way to go back to the page the user was on (AFAIK).
- Some sort of parser caching like we already have since—especially with the paging adapter—users will be scrolling through a ton of stuff and wastefully creating a ton of objects.
ButterKnife.bind(this); | ||
|
||
mFirestore = FirebaseFirestore.getInstance(); | ||
mProgressBar.setIndeterminate(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to the XML?
setContentView(R.layout.activity_firestore_paging); | ||
ButterKnife.bind(this); | ||
|
||
mFirestore = FirebaseFirestore.getInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering what the best practices should be... I think we should store the collection as a field, but use the full FirebaseFirestore.getInstance()
for other stuff like the batch. And make it final/inline the assignment? Yay, nay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna do all of that besides the inline assignment (not a fan of that in classes where you don't have a constructor, like Activity
or Fragment
)
@Override | ||
public boolean onCreateOptionsMenu(Menu menu) { | ||
getMenuInflater().inflate(R.menu.menu_firestore_paging, menu); | ||
return super.onCreateOptionsMenu(menu); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should return true
here to ensure the menu is displayed.
WriteBatch writeBatch = mFirestore.batch(); | ||
CollectionReference collRef = mFirestore.collection("items"); | ||
|
||
for (int i = 0; i < 500; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't 500 a bit excessive for a page size of 20? Maybe like 250 or something?
Also, while I've snagged your attention, the pricing for write batches really isn't clear in the docs (no mention, actually). You're still paying for 500 additions, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: pricing yeah they are priced per-write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: re: pricing thanks! 😁
app/src/main/res/values/strings.xml
Outdated
<string name="title_realtime_database_activity">Real-time database demo</string> | ||
<string name="title_storage_activity">Storage Image Demo</string> | ||
|
||
<string name="desc_auth">Demonstrates the Firebase Auth UI flow, with customization options.</string> | ||
<string name="desc_firestore">Demonstrates using a FirestoreRecyclerAdapter to load data from Cloud Firestore into a RecyclerView for a basic chat app.</string> | ||
<string name="desc_firestore_paging">Demonstrates using a FirestorePagingAdapter to load/infinite scroll paging data from Cloud Firestore.</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... paging ...
-> ... paged ...
?
onBindViewHolder(holder, position, mParser.parseSnapshot(snapshot)); | ||
} | ||
|
||
protected abstract void onBindViewHolder(@NonNull VH holder, int position, T model); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NonNull T model
|
||
@Override | ||
public String toString() { | ||
String startAfter = mStartAfter == null ? "null" : mStartAfter.getId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String.valueOf(...)
? 😁 Or TBH, I'm pretty sure the compiler does this for you if you just inline everything like so:
return "PageKey{" +
"StartAfter=" + mStartAfter.getId() +
", EndBefore=" + mEndBefore.getId() +
'}';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the ID I am worried about being null, it's mStartAfter
and mEndBefore
themselves which can be null.
In Kotlin I could use ?.
but in Java I have to do this junk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, yeah, I completely glossed over that one. What you're doing SGTM 👍
return mLoadingState; | ||
} | ||
|
||
public LiveData<FirestoreDataSource> getDataSource() { return mDataSource; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're not doing this for the other getters/setters, I think we should use line breaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was fooled by IDE trickery
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we all? 🤣
* All of the data the adapter needs. | ||
*/ | ||
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) | ||
public class PagingData { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this class is necessary at all... isn't it just casting stuff and forwarding calls along? I feel like it would be simpler to just give our adapter a PagedList
casted to the Firestore one and get the original live datas. Wha'd ya think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right. It was only necessary pre beta6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we can't get rid of the switchmap
thing due to the relationship between LivePagedListBuilder
and the Factory
. This is also something I stole from their samples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks for the explanation, I was just about to comment.
* Default diff callback implementation for Firestore snapshots. | ||
*/ | ||
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) | ||
public class SnapshotDiffCallback<T> extends DiffUtil.ItemCallback<DocumentSnapshot> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe DefaultSnapshotDiffCallback
?
Change-Id: Ic3964c31bbeb4a12c43960352d1c8b9304a8741b
Build failure:
|
@samtstern You've got to add the paging dependency to our sample 😉 |
Change-Id: I2bd2fa7812350940aff0ba8bc239c9e5b90df55d
Change-Id: I1d5cf1f6d49d5964b7cd5d702ccd7a24db3f79a8
Ok @SUPERCILEX no more TODOs on my end (besides the red build, which I know how to fix). I added tests and docs today. |
Change-Id: Ia4ccf0e08509c9720f7b91fdbcb02b6242252c11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So close!
firestore/README.md
Outdated
|
||
FirebaseUI offers two types of RecyclerView adapters for Cloud Firestore: | ||
|
||
* `FirestoreRecyclerAdapter` - bind a `Query` to a `RecyclerView` and respond to all real-time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fancy using —
instead of -
😁. Also, should we change the tense to binds [...] responds ...
?
} | ||
|
||
/** | ||
* Unsubscribe from paging / scrolling events, no more data will be populated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe [...] no more data will be populated, but the existing data will remain.
since that's different from the other adapter.
firestore/README.md
Outdated
Before using the adapter in your application, you must add a dependency on the support library: | ||
|
||
```groovy | ||
implementation 'android.arch.paging:runtime:1.0.0-beta1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use 1.x.x
instead of a specific version since we're going to forget to update it?
firestore/README.md
Outdated
}); | ||
``` | ||
|
||
Next create the `FirestorePagingAdapter` object. You should already have a `ViewHolder` subclass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next,
firestore/README.md
Outdated
@Override | ||
public ItemViewHolder onCreateViewHolder(@NonNull ViewGroup parent, int viewType) { | ||
// Create the ItemViewHolder | ||
// ,,, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks someone was a little too far left 😆, ...
.
firestore/README.md
Outdated
}; | ||
``` | ||
|
||
Finally attach the adapter to your `RecyclerView` with the `RecyclerView#setAdapter()`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... method.
firestore/README.md
Outdated
} | ||
``` | ||
|
||
Similarly, the `stopListening()` call removes the snapshot listener and all data in the adapter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removes all data in the adapter
Is this true? Pretty sure all the data is just going to stay there. Unless you meant to call submitList(Collections.emptyList())
in stopListening()
?
firestore/README.md
Outdated
|
||
#### Paging events | ||
|
||
When using the `FirestorePagingAdapter` you may want to perform some action every time data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... , you may ...
Whoops, I'm really late on this PR. This is fundamentally the same approach that we'll use on iOS, since we can't have both realtime and paging/loading new elements with reasonable scaling performance. We don't have a PagingAdapter thing though, so the way Firestore's data is connected to the UI will be a little bit different. I'm not worried about that though. This LGTM |
Change-Id: Iae9801e965664f987e892968df362186b7c9a8f2
firestore/README.md
Outdated
|
||
The `FirestorePagingAdapter` binds a `Query` to a `RecyclerView` by loading documents in pages. | ||
This results in a time and memory efficient binding, however it gives up the real-time events | ||
afforted by the `FirestoreRecyclerAdaoter`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor typo in FirestoreRecyclerAdapter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops thanks!
Change-Id: I7e3ca99bc9f682e7989a40155b7bbd3437b98559
@SUPERCILEX note that I just added a |
Change-Id: I3f1255426bafd74f3855bd05580b6cd2cecd00a6
Change-Id: I882ce0d573d48f87a85d37b821ea6c60c23e675a
Is there any way to delete or update item in FirestorePagingAdapter locally? I have a story app in which users post stories of different languages and in my apps home screen I list all of them sort by date using FirestorePagingAdapter. Now I have added language filtering option that is for example if user opted to English, Dutch and Arabic stories app should only show that in main screen. Since firestore doesn't support OR queries I am unable to do that with query what I am currently doing is just hiding items by setting View.Gone from OnBindViewHolder of the FirestorePagingAdapter. What I need is an option to remove those from PagingAdapter list itself |
See #17
TODO:
firestore
or creating a newfirestore-paging
retry()
method that the developer can call to make the error state non-terminalLoadingState
? What about thePageKey
?