Closed ahaverty closed 8 years ago
@ahaverty Yeah, I got a little bit confused about that too.
It would help if @nmoskalenko could explain the use cases he was going for. That way we could at least discuss better naming for those methods.
I'd suggest altering observeValuesMap(), observeValuesList(), observeValues() to do what @renanferrari 's observeValue() is doing, which is essentially just never onComplete()'ing, similarly to how Firebase's doesn't either.
I'm not familiar enough with Rx to understand what this is doing though:
subscriber.add(Subscriptions.create(() -> query.removeEventListener(valueEventListener)));
I've taken your code and adapted it to work for Firebase Server. However, I've since applied @nmoskalenko 's changes to those methods: rxfirebaseserver/RxFirebaseDatabase.java
Let me know if you'd like a PR to see what I mean.
@ahaverty If I understand correctly, observeValuesMap(), observeValuesList() and observeValues() observes an array-like value (with single-typed children) and then emits its children as a map, as a list or one by one, respectively. The issue is that it only listens to changes at the parent level, not at the children level (for that, we'd have to use observeChildrenEvents()). With that in mind, I guess these methods would not be very useful as hot observables. But I might be wrong on this, as I said before, it would be nice to know some cases where it should be useful. (edit: I was wrong, as explained by @ahaverty below)
About the piece of code you said you didn't understand, I believe it removes the Firebase's event listener when the subscriber unsubscribes.
Just to ensure we're on the same page, Firebase's ValueEventListener listens to changes including child changes. (See 'Value events' section: https://firebase.google.com/docs/database/android/retrieve-data)
Important: The onDataChange() method is called every time data is changed at the specified database reference, including changes to children. To limit the size of your snapshots, attach only at the highest level needed for watching changes. For example, attaching a listener to the root of your database is not recommended.
Use case:
Books ->
Book1Uid ->
AuthorName -> Michael
BookName -> Jurassic
Genres ->
jurassic -> true
Book2Uid ->
AuthorName -> Stephen
BookName -> IT
Genres ->
horror -> true
coming of age -> true
I personally use Value Event listeners to listen to a reference like Books/Book1Uid. What I've been doing with your RxFirebase is passing in that reference and the class Book.class . Now that I've modified observeValuesMap(), observeValuesList() and observeValues() with @nmoskalenko 's changes, I can continuously/hot listen to any changes below Book1Uid. (The genre's, where the key is the value and boolean is a holder. I originally took that from Firebase's documentation recommendations, hence why I asked for the key's to be added in the other issue)
Does this make sense or are we on the wrong page? Thanks @renanferrari !
Hello, guys! Thank you for providing such an interesting feedback. Firebase allows us to retrieve data in 2 ways:
This library was about the first way, as only it was needed on the project I was working on. But, of course, I do see the importance of second way too. As I can see @renanferrari is going into the right way in terms of functionality, the only problem I see is API naming is getting more and more complicated :) Now even for me it's hard to understand the differences between all these observeValue, observeValues, observeSingleValue and so on.
How about split API into 2 parts:
?
Also, seems like observeValuesList is redundant as doesn't produce any real value to caller.
I'd be happy either way. If you were to split it in two, i'd imagine new comers would have a hard time distinguishing the differences between RxFirebaseDatabaseSnapshot and RxFirebaseDatabase since both would deal in Datasnapshots?
What about:
Either way, I'm personally happy as long as the functionality is there. Thanks for the comments
@ahaverty You're completely right. I was wrong about how the listeners handles children changes.
@nmoskalenko We should aim to have a thinner API, so I don't think having the API split in two different classes is completely necessary. For the behavior we want regarding hot/cold observing, I think Firebase's original API already handles this pretty well with its listeners, so it makes sense to me that this library's core functionality would be just to replicate these behaviors using RxJava's API, providing something equivalent in name and functionality, like so:
// Equivalent to addValueEventListener()
public static Observable<DataSnapshot> observeValueEvent(final Query query) {...}
// Equivalent to addListenerForSingleValueEvent()
public static Observable<DataSnapshot> observeSingleValueEvent(final Query query) {...}
// Equivalent to addChildEventListener()
public static Observable<RxFirebaseChildEvent<DataSnapshot>> observeChildEvent(final Query query) {...}
This is the core, everything else is additional.
In fact, if I'm thinking correctly this time, the observeSingleValueEvent method is actually optional, as it should have the same behavior as calling ".take(1).single()" after calling observeValueEvent.
From there, we could think about evolving the API adding functionalities that we think are really useful, but are not core. The best example is the automatic DataSnapshot mapping we already have.
I believe we can achieve a more powerful, extensible and flexible API, if we have only three additional methods, with the same name as the ones I cited before, but with an additional parameter that would take something like a DataSnapshotMapper, a class that would be responsible to map the DataSnapshot to a custom object.
public static <T> Observable<T> observeValueEvent(final Query query, final DataSnapshotMapper<T> mapper) {...}
public static <T> Observable<T> observeSingleValueEvent(final Query query, final DataSnapshotMapper<T> mapper) {...}
public static <T> Observable<RxFirebaseChildEvent<T>> observeChildEvent(final Query query, final DataSnapshotMapper<T> mapper) {...}
That would decouple the core API from this library from its additional features, like mapping to a custom object, list, maps, etc, and would protect the API from any changes in Firebase's API regarding DataSnapshot.
Essentially, DataSnapshotMapper
We would have to discuss further what these default implementations could be, but I think the very basic one would just use DataSnapshot.getValue(Class) method under the hood, so the calls would be very similar to what we do today already:
// The static method DataSnapshotMapper.from() would be a default implementation
// that would user use Firebase's DataSnapshot.getValue(Class) method under the hood
RxFirebaseDatabase.observeValueEvent(commentsRef.child(key), DataSnapshotMapper.from(Comment.class))
.subscribe(comment -> Log.d("RxFirebase", "Comment: " + comment));
// Mapping at any stage of our chain would be possible too, since DataSnapshotMapper would be just a Func1
RxFirebaseDatabase.observeValueEvent(commentsRef.child(key))
// do whatever we want with our DataSnapshot
.doOnNext(dataSnapshot -> Log.d("RxFirebase", "DataSnapshot: " + dataSnapshot))
// and then we map it at any time we want to
.map(DataSnapshotMapper.from(Comment.class))
.subscribe(comment -> Log.d("RxFirebase", "Comment: " + comment));
In the end, the API would even be able to handle more complex cases, where you may want to map the DataSnapshot differently from what Firebase's does automatically.
Using @ahaverty example above, that would give him one more option to map its Book object in any way he likes.
// Custom mapping (not the best example, but it shows the potential for the API to handle more complex mapping)
DataSnapshotMapper<Book> bookDataSnapshotMapper = new DataSnapshotMapper<Book>() {
@Override public Book call(final DataSnapshot dataSnapshot) {
String authorName = dataSnapshot.child("author_name").getValue(String.class);
String bookName = dataSnapshot.child("book_name").getValue(String.class);
Map<String, Boolean> genreMap = dataSnapshot.child("genres").getValue(new GenericTypeIndicator<Map<String, Boolean>>() {});
List<String> genreList = new ArrayList<>(genreMap.keySet());
// In this case, the Book model wouldn't have to be limited by Firebase's annotations
return new Book(authorName, bookName, genreList);
}
};
// The call
RxFirebaseDatabase.observeValueEvent(booksRef.child(key), bookDataSnapshotMapper).subscribe(book -> {
Log.d("RxFirebaseTest", "Book with custom mapper: " + book);
textView.setText("Book with custom mapper: " + book);
});
Anyway, this is just a bunch of ideas that I think would help with #5 and #6 and keep the library both powerful and flexible.
What do you think?
I agree @renanferrari , I like the idea of making this library as simple as possible, letting RxJava and their documentation do the hard work. Otherwise it'll be an endless stream of people like me asking for specific features and specific mapping pull requests! Let me know how/where/when I can code/help/test.
@renanferrari , I love the idea of DataSnapshotMapper's. This technique can indeed make the library both powerful and flexible! I'm not really sure that ".take(1).single()" is 100% equivalent for observeSingleValueEvent, would vote for keeping it at least for now, but what about all other options you raised, I fully agree with you. Please let me know if you like to implement it by yourself or you need any help on this. And thank you for such a great coopiration!
@nmoskalenko Excellent. I'm going to try to code a draft implementation during this week. I'll let you know.
@nmoskalenko @ahaverty I've just pushed the API changes here.
Some considerations are needed before I can submit a pull request:
public static Observable<DataSnapshot> observeValueEvent(Query query) {...}
public static Observable<DataSnapshot> observeSingleValueEvent(Query query) {...}
public static Observable<RxFirebaseChildEvent<DataSnapshot>> observeChildEvent(Query query) {...}
public static <T> Observable<T> observeValueEvent(Query query, Func1<DataSnapshot, T> mapper) {...}
public static <T> Observable<T> observeSingleValueEvent(Query query, Func1<DataSnapshot, T> mapper) {...}
public static <T> Observable<RxFirebaseChildEvent<T>> observeChildEvent(Query query, Func1<RxFirebaseChildEvent<DataSnapshot>, RxFirebaseChildEvent<T>> mapper) {...}
public static <T> Observable<T> observeValueEvent(Query query, Class<T> clazz) {...}
public static <T> Observable<T> observeSingleValueEvent(Query query, Class<T> clazz) {...}
public static <T> Observable<RxFirebaseChildEvent<T>> observeChildEvent(Query query, Class<T> clazz) {...}
public static <U> DataSnapshotMapper<DataSnapshot, U> of(Class<U> clazz) {
return new TypedDataSnapshotMapper<U>(clazz);
}
public static <U> DataSnapshotMapper<RxFirebaseChildEvent<DataSnapshot>, RxFirebaseChildEvent<U>> ofChild(Class<U> clazz) {
return new ChildEventDataSnapshotMapper<U>(clazz);
}
The utility methods that mapped Firebase responses to List and Map objects were removed in favor of RxJava's own methods toList() and toMap(). We may have to discuss the possibility to have a DataSnapshotMapper implementation to handle Firebase's GenericTypeIndicator class.
I've adapted the current tests and sample project to reflect the API changes, but we probably need to write some tests specifically for the mapping behavior and update the sample project along with the documentation (maybe a Wiki page would be a nice addition too?).
That's it. Since this would present several breaking changes, I think we should discuss the best way to proceed from here.
Also, let me know if I've missed something in any way.
Great work @renanferrari ! We should pull the Firebase guys in here to see if they have any thoughts maybe. I'm admittedly uneducated in Rx and probably unlikely to influence from here on, but I'll try to take it for a spin over the next few days. Let me know if you need a hand with anything specifically 👌
@renanferrari I'm finding it difficult to use .toMap() with your new observeSingleValueEvent(). Do you have a quick example?
Hello Renan! Amazing job! The API is very clean as well as its implementation.
I like the decision of keeping "class type" as if would provide lots of simplicity for trivial use-cases.
The only thing I didn't quite understand is why you introduced empty private constructor for DataSnapshotMapper. It's already abstract. I guess it show also be not public.
Also, you updated grade, compileSdkVersion, buildToolsVersion, Firebase versions. And changed "provided" dependency of Firebase libs to "compile". Was it really needed? It wanted to keep Firebase dependency compile-time only.
@ahaverty Thanks! I like the idea of receiving some feedbacks from the Firebase people. About the .toMap(), I could write an example. Do you have any specific use case?
@nmoskalenko Thank you! The private constructor is an attempt to avoid any calls like observeValueEvent(query, new DataSnapshotMapper(){...}). Its static factory methods should be used instead of that. About the dependencies updates, they are probably not required. I don't remember exactly the reason I changed it, but most of those changes were made automatically by Android Studio (or maybe just a bad copy and paste). I'll review everything before making a PR. Let me know if you need anything else.
nice one guys :)
Appreciate it @renanferrari ! I'm just looking to return a basic map using .toMap() but I'm guessing I need a class/type like the DataSnapshotMapper that you were talking about in order to map firebase's keys and values. I tried a few combinations of the GenericTypeIndicator out but couldn't get anywhere. Perhaps if I show you my workaround, it might give a better understanding?
Firebase example content:
{
"merchants" : {
"uniqueUid1" : {
"name" : "Pizza Shop",
"type" : "restaurant"
},
"uniqueUid2" : {
"name" : "Red Cafe",
"type" : "coffee shop"
}
}
}
Current Workaround:
RxFirebaseDatabase.observeSingleValueEvent(rootRef.child("merchants"))
.subscribe(dataSnapshot -> {
HashMap<String, Merchant> merchantsMap = new HashMap<>();
for(DataSnapshot childSnapshot : dataSnapshot.getChildren()) {
merchantsMap.put(childSnapshot.getKey(), childSnapshot.getValue(Merchant.class));
}
//now I can do stuff with merchantsMap (Keys and values together)
As you can see, I'm not looking for anything too custom as I use models for everything that I'm getting from Firebase anyway. If I could use toMap() to save on the boilerplate stuff to allow me to use the key and value, that would be great.
Also, sidenote thought: Am I wrong in saying observeValueEvent() (Hot observing) can't be used with toMap() or toList() as they never call .onComplete()? Not a problem for me but might be worth noting in the readme if so.
@ahaverty I think that could be achieved with something like the following:
RxFirebaseDatabase.observeSingleValueEvent(reference.child("merchants"))
.map(DataSnapshot::getChildren)
.flatMap(Observable::from)
.toMap(DataSnapshot::getKey, DataSnapshotMapper.of(Merchant.class))
.subscribe(merchantsMap -> {
// Do what you need
});
Am I wrong in saying observeValueEvent() (Hot observing) can't be used with toMap() or toList() as they never call .onComplete()?
You're right, it couldn't be used directly, but it should be fine to use inside a flatMap()
, like so:
RxFirebaseDatabase.observeValueEvent(reference.child("merchants"))
.map(DataSnapshot::getChildren)
.flatMap(dataSnapshots -> Observable.from(dataSnapshots)
.toMap(DataSnapshot::getKey, DataSnapshotMapper.of(Merchant.class)))
.subscribe(merchantsMap -> {
// Do what you need
});
I haven't tested the code above, so let me know if it works for you.
Maybe we should write a Wiki later, with these common use cases.
Thanks for your awesome work, Renan! Really looking forward to your PR!
Your first example worked well for me, we should definitely add a few examples for toMap and toList to the readme. Thanks @renanferrari !
@renanferrari , how the things are going for you? Seems like your work is already done.
@nmoskalenko Sorry for the delay. Just made the PR.
Brilliant job, Renan, thank you very much! I resolved the conflict manually, fixed unit tests and bumped the version number. And also squashed all commits into one to make things more clear.
Will update readme, sample and wiki next few days.
Am I wrong in asking why all of the other listeners aren't like @renanferrari 's pull request #3, allowing continuous listening like Firebase intended? Or am I using these incorrectly?