Skip to content
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

SnapshotParser Documentation and Performance #1037

Closed
ahaverty opened this issue Dec 4, 2017 · 13 comments
Closed

SnapshotParser Documentation and Performance #1037

ahaverty opened this issue Dec 4, 2017 · 13 comments
Labels

Comments

@ahaverty
Copy link

ahaverty commented Dec 4, 2017

Hi Firebase,
I'm using the SnapshotParser to allow my onBindViewHolder to decide what model to map data to.
It took me awhile to figure out how to do this, as I wanted to use generics with various different models in a single recycler view (Harnessing the power of firebaseUI 🤘).
I found this answer from last year from @puf . But it looks like SnapshotParser is the new way to do this?

SnapshotParser<DataSnapshot> dataSnapshotSnapshotParser = new SnapshotParser<DataSnapshot>() {
    @Override
    public DataSnapshot parseSnapshot(DataSnapshot snapshot) {
        return snapshot;
    }
};

FirebaseRecyclerOptions<DataSnapshot> options =
        new FirebaseRecyclerOptions.Builder<DataSnapshot>()
                .setQuery(myReference, dataSnapshotSnapshotParser)
                .build();

mAdapter = new FirebaseRecyclerAdapter<DataSnapshot, MyHolder>(options) {
 //etc....

Two things:

  1. It would be nice to see some documentation on this, and any recommendations/guidelines on working with multiple class parsing with firebase ui. (We've gotten it working, but I'm sure we could do something fancier/simpler)
  2. Does individually parsing snapshots like this cause any performance issues versus using .setQuery(myReference, MyModel.class)? Are we losing out on any caching etc by using the SnapshotParser?

Thank you!

@samtstern samtstern added the docs label Dec 4, 2017
@samtstern
Copy link
Contributor

@ahaverty thanks for the suggestion, we should improve the docs on SnapshotParser to make this clearer.

The caching behavior is in BaseCachingSnapshotParser:
https://github.com/firebase/FirebaseUI-Android/blob/master/common/src/main/java/com/firebase/ui/common/BaseCachingSnapshotParser.java

It caches the result of the operation that turns a DataSnapshot into your custom model class. However it has a single generic type parameter for the model class, <T>, so you have to be careful.

I think if you extended BaseCachingSnapshotParser<Object> with custom parsing code you would be able to get the behavior you want without much of a performance hit.

@ahaverty
Copy link
Author

ahaverty commented Dec 4, 2017

Thanks for the suggestion @samtstern

Am I wrong in saying I'm restricted by the BaseSnapshotParser interface requiring a single type?

Am I digging a hole if I try go further than that?
It turns out the performance without the caching snapshot parser is poor.

We're now thinking of something like a big Everything.class, passing that to the snapshot parser and mapping to our subclasses from our bind view. It sounds like it's going to get messy, so I'd appreciate any more ideas 🙏

@samtstern
Copy link
Contributor

I think you could use Object as your type parameter and then use a bunch of casting but yes you're right it will get pretty messy.

@ahaverty
Copy link
Author

ahaverty commented Dec 4, 2017

That makes way more sense.. Thanks @samtstern I'll give it a shot.

@SUPERCILEX
Copy link
Collaborator

@ahaverty are all your models representing completely different data? If not, you can use a base class that contains the common fields and pass that around. Then use some sort of ModelType enum so you can figure out what type of model it is and also store it in the database.

@ahaverty
Copy link
Author

ahaverty commented Dec 4, 2017

Thanks @SUPERCILEX We've structured it like that alright:

[ {
  "type" : "merchant",
  "value" : {
    "availability" : {
      "title" : "Open",
      "type" : "open"
    },
    "chips" : [ {
      "colour" : "#68A225",
      "title" : "Favourite"
    } ],
    "imageUrl" : "testimage",
    "name" : "test store"
  }
}, {
  "type" : "advertisement",
  "value" : {
    "advertSpecificProperty1" : "test",
    "advertSpecificProperty2" : "test2"
  }
}

I was hoping there'd be no problem just checking the type and casting using generics. It worked, but it seems to have slowed down the recyclerview massively, especially with glide/images involved.
I tried parsing the object, but i'm having issues parsing from a hashmap to my object.

@SUPERCILEX I'm not familiar with kotlin, is the same possible with Java? (If not, is it possible to mix in that kotlin with my existing stuff?) Thanks for the fast help 😃

@SUPERCILEX
Copy link
Collaborator

SUPERCILEX commented Dec 4, 2017

It should totally work in Java, but it might be a little longer and uglier. 😄 Looking at your data, I would create a base model with a type and value field of type T. The advertisement should be easy and just require a subclass to clarify its type (it would extend the base model with a type of Map<String, String>). The merchant one looks more complicated so I think your subclass would include another class like MerchantData or something. It would have fields imageUrl: String, name: String, availability: Availability, and chips: Map<String, String>. Then it would be up to your parser function to build those model objects from the DataSnapshot. Anything else I can clarify? 😄

@ahaverty
Copy link
Author

ahaverty commented Dec 4, 2017

Thanks @SUPERCILEX I actually have my generics working nicely, using the SnapshotParser<DataSnapshot> trick in my original question. We're looking to move a lot of our logic out of the client, and onto functions + firebaseUI lists in the clients. So out actual production models are more complex than my example there, and more will be needed as we go . I think my only remaining issue is the performance of scrolling in a recycler view with images.

I can't prove 100% yet, but scrolling appears to be silky smooth when using FirebaseUI options with a single class + your nice caching SnapshotParser's versus using a custom SnapshotParser without the caching. Making an educated guess, I'm thinking the two Datasnapshot.getValue(myGeneric) being called in every onBindViewHolder is causing the lag. (It looks like your example doesn't cache, right?)

The single model generic seems to go all the way to the core of FirebaseUI, so I'm thinking of possibly taking @samtstern 's advice about implementing caching for multiple classes, but maybe doing it in my activity/onBindViewHolder for now. I've got datasnapshot.getKey(), so I can probably just create a new LruCache for each new model class.

Would love to hear any ideas, thanks for the help!

@SUPERCILEX
Copy link
Collaborator

@ahaverty as long as you do the intensive processing in a SnapshotParser, it will be cached. Otherwise, using your own cache would work just fine too.

@ahaverty
Copy link
Author

ahaverty commented Dec 4, 2017

@SUPERCILEX & @samtstern Got it working beautifully with your simple SnapshotParser !
I wasn't thinking properly about my Base Models and inheritance, and kept rereading your comments until I got it Alex & Sam!
Thanks for sticking with me ❤️

(Medium post to follow once we get FirebaseFunctions + FirebaseUI working in production 😉)

SnapshotParser<HomeFeedCell> snapshotParser = snapshot -> {
            GenericTypeIndicator<HashMap<String, Object>> rawIndicator =
                    new GenericTypeIndicator<HashMap<String, Object>>() {
                    };

            HashMap<String, Object> rawHashMap = snapshot.getValue(rawIndicator);
            String type = (String) rawHashMap.get("type");
            HomeFeedCell homeFeedCell = null;

            switch (type) {
                case "merchantOrdinary": {
                    GenericTypeIndicator<HomeFeedCell<MerchantCellValue, MerchantCellInteraction>> genericTypeIndicator =
                            new GenericTypeIndicator<HomeFeedCell<MerchantCellValue, MerchantCellInteraction>>() {
                            };
                    homeFeedCell = snapshot.getValue(genericTypeIndicator);
                    break;
                }
                case "advert": {
                    GenericTypeIndicator<HomeFeedCell<AdvertCellValue, AdvertCellInteraction>> genericTypeIndicator =
                            new GenericTypeIndicator<HomeFeedCell<AdvertCellValue, AdvertCellInteraction>>() {
                            };
                    homeFeedCell = snapshot.getValue(genericTypeIndicator);
                    break;
                }
            }

            return homeFeedCell;
        };


        FirebaseRecyclerOptions<HomeFeedCell> options =
                new FirebaseRecyclerOptions.Builder<HomeFeedCell>()
                        .setQuery(homeViewCellsReference, snapshotParser)
                        .build();

@ahaverty ahaverty closed this as completed Dec 4, 2017
@ahaverty
Copy link
Author

ahaverty commented Dec 4, 2017

Sorry @samtstern , I prematurely closed this out of happiness. I forgot about the docs, I'll reopen, but consider my part ii) solved. Thanks again!

@SUPERCILEX
Copy link
Collaborator

@samtstern This issue can be closed since we pushed the changes to master 👍.

@samtstern
Copy link
Contributor

Thanks @SUPERCILEX

@ahaverty be sure to let us know when that medium post comes out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants