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

Create a pluggable JSON parsing module #3

Closed
gpeal opened this issue Oct 11, 2016 · 23 comments
Closed

Create a pluggable JSON parsing module #3

gpeal opened this issue Oct 11, 2016 · 23 comments
Assignees

Comments

@gpeal
Copy link
Collaborator

gpeal commented Oct 11, 2016

Ideally there would be 2 (or n) optional json parsing modules.
1 would use raw json objects so there would be no transitive dependencies
The other would use jackson or another more performant json parsing library.

@gpeal
Copy link
Collaborator Author

gpeal commented Oct 11, 2016

@felipecsl

@felipecsl felipecsl self-assigned this Oct 11, 2016
@felipecsl
Copy link
Collaborator

This is gonna require a fair amount of work to decouple JSONObject from LottieComposition.
I think first we should flatten the package structure into just com.airbnb.lottie so you can keep everything package private and prevent from leaking classes to users (and avoid the @RestrictTo annotation).
@gpeal if you agree with that, I could go ahead and break it down into multiple artifacts (java libs) for model, layers, animation, etc. That way we can keep each module small while maintaining everything under a single package.
That's sort of a pre-requisite change in order for us to be able to have pluggable json parsing implementations. What do you think?

@gpeal
Copy link
Collaborator Author

gpeal commented Feb 2, 2017

@subtleGradient suggested using FlatBuffer. Tracking progress here.

@gpeal gpeal changed the title Create a forked JSON parsing module Create a pluggable JSON parsing module Feb 2, 2017
@gpeal
Copy link
Collaborator Author

gpeal commented Feb 2, 2017

@felipecsl That sounds like a good plan. Having better scope protection is also definitely good.

@felipecsl
Copy link
Collaborator

Alright, I'll start going down this path then :)

@pdenise708
Copy link

K p

@ZacSweers
Copy link
Contributor

@felipecsl did you start this? If not I'd like to contribute. I think the core of this is that a composition should be constructable via some conventional way (maybe a builder?). The default could basically be the current JSONObject implementations, with maybe some batteries-included optional artifacts that do it with gson/moshi/etc.

@felipecsl
Copy link
Collaborator

@hzsweers nope, I was waiting for @gpeal to finish some major JSON parsing refactoring that he had underway. AFAIK the plan was to move the current implementation to a streaming JsonReader which should be more memory efficient. Not sure what's the status of that change though, @gpeal can help clarify

@gpeal
Copy link
Collaborator Author

gpeal commented Feb 18, 2017

@felipecsl @hzsweers I spent quite a long time working on the JsonReader refactor which would have been a good end-all solution to this since it's strictly n time. However, there were some issues with the way the json was formatted that made it almost impossible to do proper parsing when only reading the json beginning to end linearly. Here is my WIP if you're interested: d7227b3

I synced up with Hernan on bodymovin about it and he made a few changes to the json that would make it more feasible:
airbnb/lottie-web@6ed7b0f
airbnb/lottie-web@18201cb

I want to give it another try but it would break compatibility with older versions so maybe splitting json parsing into a module and using the new one if you have a new animation would be a good idea and then we can drop support for older versions eventually.

@felipecsl Do you want to take another stab at this?

@felipecsl
Copy link
Collaborator

Sounds good to me, thanks for clarifying @gpeal.
@hzsweers I think we're on the same page about how it should work. I can probably work on it this week, we can sync offline on the details. How does that sound?

@gpeal
Copy link
Collaborator Author

gpeal commented Feb 20, 2017

For reference, the longest and most complex json file, LottieLogo2.json (90kb) takes about 30ms to parse.

@ZacSweers
Copy link
Contributor

sounds good

gpeal pushed a commit that referenced this issue Feb 21, 2017
This is part of issue #3. Pulled static factory methods out of LottieComposition into a static inner class. Made all fields final and initialized them all in the constructor.
This will make it easier for follow up refactors where we'll modularize the parsing logic and make it pluggable.
Also upgraded the Espresso tests to JUnit 4 so it no longer uses the deprecated ActivityInstrumentationTestCase2
gpeal pushed a commit that referenced this issue Feb 21, 2017
Another change for #3

Pretty mechanical and straightforward change. Extracted a bunch of constructors into more static inner factory classes with a newInstance method by convention.
Later on we can pull out all these factories into a separate module that provides the default json parsing implementation.
gpeal pushed a commit that referenced this issue Feb 22, 2017
This was a bit more involved as the JSON parsing in Keyframe and AnimatableValue classes is a lot more involved.

Part of #3
@0legg
Copy link
Contributor

0legg commented Feb 22, 2017

Few days ago there was JSON spec created for bodymovin, and I'm working on bringing it to JSON Schema — so it'll be possible in the future autogenerate parser source code.

gpeal pushed a commit that referenced this issue Feb 23, 2017
In this pass I've converted BaseAnimatableValue and its subclasses.
Created a AnimatableValueParser with the common logic to all subclasses.
This change caused a "chicken and egg" problem where the AnimatableValueParser calls into Keyframe.Factory.parseKeyframes, which needs the respective instance of BaseAnimatableValue. This caused a circular dependency issue. I've resolved by moving AnimatableValue#valueFromObject() into a separate interface called AnimatableValue.Factory which contains only that method.
I think almost all the relevant classes have been refactored by now.
Part of #3
@felipecsl
Copy link
Collaborator

felipecsl commented Feb 28, 2017

Alright let me explain the rest of the changes I have in mind for this issue so I can get input from you guys before sending the PR:
We'd have a new class called Lottie responsible for initializing the library state. It would have a builder where we could specific the desired Converter implementation to use (similar to how Retrofit works). We could also use this in the future to configure other settings for the library:

Lottie lottie = new Lotite.Builder()
  .converter(new DefaultJsonConverter())
  .build()

Then, we'd need to use this lottie object when loading animations so it knows which Converter implementation to use:

animationView.setAnimation(lottie, "hello-world.json");

We'd also move the current LottieComposition.Factory#fromJson(), etc. APIs to Lottie#fromJson(). That'd be the entry point for most operations.

LottieAnimationView would use a default implementation of Lottie when app:lottie_fileName is used via XML since we wouldn't have the chance to initialize the Lottie object properly. We'll have to find a way to cover that scenario by maybe initializing it via a singleton.

Finally, the different Converter implementations would live in separate artifacts (also Android libraries). In order to use Lottie, you'd have to depend on the correct artifact, for example:

  compile 'com.airbnb.android:lottie-converter-default:version'

The lottie-converter-default artifact depends on lottie so it would get the dependency transitively.

What do you guys think?

@0legg
Copy link
Contributor

0legg commented Feb 28, 2017

Looks good to me; but it's still unclear how will you manage model and logic separation. Also, it's interesting, whether it's possible to parse JSON with databinding; and to provide model automatically from JSON Schema. I wish I had enough time to contribute lot of stuff here

@felipecsl
Copy link
Collaborator

our model classes are still somewhat coupled to the JSON structure, but over time we should decouple them as much as possible in a way that makes us input format agnostic. All you need is a converter that takes an InputStream and outputs a LottieComposition

@fabionuno
Copy link
Contributor

There is any advantage to use this new Lottie class as a parameter of setAnimation method and not as a parameter of LottieAnimationView's constructor?
Using as a method parameter is useful if we need to use different JSON loaders with same LottieAnimationView instance, but i don't see a situation where this will occur, and I need to define Lottie as a member variable of my class to use it every time I call setAnimation.

A setSingletonInstance method could be created, like Picasso, to configure the default behavior of Lottie and use this singleton when a custom one is not specified in LottieAnimationView constructor.

This will help when instantiating LottieAnimationView from XML as you suggested.

@gpeal
Copy link
Collaborator Author

gpeal commented Feb 28, 2017

@felipecsl I like the overall approach. What do you think about Lottie including the default parser as a transitive dependency so that a) LottieAnimationView can create the default one and b) users don't have to add a second dependency especially when, as of now, there is only one option

@felipecsl
Copy link
Collaborator

felipecsl commented Mar 1, 2017

@gpeal that's not possible because it would cause a circular dependency.
@fabionuno we can't add Lottie as a constructor paramterer to LottieAnimationView because it needs to be instantiated from the XML

@fabionuno
Copy link
Contributor

@felipecsl The idea was to have a new constructor with Lottie as a parameter and when missing get the singleton instance, however in a situation where I declare the LottieAnimationView in XML but load the JSON animation programmatically, have it as parameter of the method setAnimation is a better option.

@gpeal
Copy link
Collaborator Author

gpeal commented Mar 1, 2017

@fabionuno Adding View constructors is pretty unconventional. We could also add something like what RV does with LayoutManager: https://developer.android.com/reference/android/support/v7/widget/RecyclerView.html#attr_android.support.v7.recyclerview:layoutManager

@felipecsl
Copy link
Collaborator

Should we close this issue? Not sure we're still planning to do anything else for this.

@gpeal
Copy link
Collaborator Author

gpeal commented Mar 14, 2017

@felipecsl Let's close this for now.

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

No branches or pull requests

8 participants
@felipecsl @gpeal @ZacSweers @0legg @fabionuno @pdenise708 and others