-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Method to update Query in Firebase and Firestore Adapters. #1660
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
Method to update Query in Firebase and Firestore Adapters. #1660
Conversation
…aseUI-Android into sp-feature-1614
@samtstern There are some Auth lint errors exists that's why Trivis CI failed. |
@samtstern These are errors.
|
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.
@PatilShreyas thanks for this! Just a few comments.
* | ||
* @param newQuery is a new updated query. | ||
*/ | ||
public abstract void updateQuery(@NonNull Query newQuery); |
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 this be added in BaseObservableSnapshotArray
instead? It seems a bit strange that this would be the only method defined on ObservableSnapshotArray
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.
Because BaseObservableSnapshotArray
is inherited in both ObservableSnapshotArray
of Firestore as well as Firebase realtime database too. And most importantly, the Query
class of both databases are not the same.
- Firestore Query Class:
com.google.firebase.firestore.Query
- Firebase Query Class:
com.google.firebase.database.Query
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.
If we add this method in BaseObservableSnapshotArray
then Query
parameter will be different for them. What else can we do here?
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 would say let's just not make this part of the base class at all, it can just be something implemented in each of the Array
classes.
That will also allow us to give the indexed version a better name: updateKeyQuery()
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 have instantiated FirestoreArray
by its superclass references in Adapter
classes. So, Will it be fine them to be instantiated as Array
classes?
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.
Fine 😃. Okay then 👍 .
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 have some questions.
- If I instantiate mSnapshots as
FirebaseArray' as then how to take care of
FirebaseIndexArray`? - How could we know that when to call
updateQuery()
orupdateKeyQuery()
?
InFirebaseRecyclerAdapter
if I create an instance ofFirebaseArray
then we can't instantiateFirebaseIndexArray
. How to do it?
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.
Oh no that is a problem ... any ideas? I don't know why I am so opposed to adding updateQuery
to the ObservabeSnapshotArray
classes ... it just feels wrong. But maybe that is the best way.
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 also thought a lot about it before implementation in ObservableSnapshotArray
class. But I can't find anything helpful.
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 had one idea. But that idea was not much good to implement. Means, it's not good practice to implement. I had idea to use instanceof
to check whether it's instance of Firebase array or index array. We will check it in our method and by casting we will call array class method. That was the idea.
@@ -25,6 +26,7 @@ public boolean areItemsTheSame(@NonNull DataSnapshot oldItem, | |||
return oldItem.getKey().equals(newItem.getKey()); | |||
} | |||
|
|||
@SuppressLint("DiffUtilEquals") |
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.
Why is this lint suppression added in this PR when nothing else about this file changed?
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.
Because it caused an error when I run the command gradlew assembleDebug proguard-tests:build check
When I checked that class, it's showing me the red line under return
statement showing a suggestion to add this line.
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.
Ok I am fine with this, although I bet if we revert the unrelated changes in this PR, this will go away.
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.
Oh! Okay.
firestore/src/main/java/com/firebase/ui/firestore/ObservableSnapshotArray.java
Outdated
Show resolved
Hide resolved
buildSrc/build.gradle.kts
Outdated
repositories { | ||
jcenter() | ||
} | ||
|
||
plugins { | ||
`kotlin-dsl` | ||
kotlin("jvm") version "1.3.41" |
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 you explain why we need all of these kotlin changes in this PR? Are they related?
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.
No. I just updated it as it's on my system.
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.
Ok let's revert these changes since they are not related to the feature.
@@ -1,5 +1,5 @@ | |||
distributionBase=GRADLE_USER_HOME | |||
distributionPath=wrapper/dists | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-4.10.1-all.zip | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-5.1.1-all.zip |
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.
Let's also revert this, this could be a big change and I want to do it separately (when I have time to verify the release process with gradle 5)
Change-Id: I3d6465a15171b2cd5629d93e5745585552129d2d
@PatilShreyas ok I did some thinking and experimenting. First I found better words to explain myself :-) the reason I am not comfortable with Second I think it may be better to structure this as What do you think? |
Right @samtstern! |
[WIP] Update Query Experiments
@samtstern I have merged your PR and make changes as you suggested. Also implemented in Firestore adapter. Also, tested |
@PatilShreyas sorry I missed your last comment! Thanks for working with me on this, LGTM. |
Anytime! Thank you! |
This PR Implements a method to change/update
Query
in both Firebase as well as Firestore Adapters. (Issue #1614)