airbnb / epoxy

Epoxy is an Android library for building complex screens in a RecyclerView
https://goo.gl/eIK82p
Apache License 2.0
8.52k stars 726 forks source link

Inheriting from EpoxyModelWithView with custom generic type generates wrong model #199

Closed davidbilik closed 7 years ago

davidbilik commented 7 years ago

Hi, I am a big fan of Epoxy 2.0 with EpoxyModelWithView, we are using Anko in our projects and this was the last missing thing to switch completely from xml to Anko. The problem is that we are using our special class for specifying layouts called ViewLayout. Its not child of View, its something like AnkoComponent but it has some additional features. And since EpoxyModelWithView requires generic type that inherits from View it does not really fit to our world. I've decided to write my own generic EpoxyModel like this

open abstract class EpoxyViewLayoutModel<T : ViewLayout> : EpoxyModelWithView<View>() {

    abstract fun createViewLayout(parent: ViewGroup): T

    override final fun buildView(parent: ViewGroup): View {
        val layout: T = createViewLayout(parent)
        val view = layout.createView()
        view.setTag(R.id.epoxy_view_layout, layout)
        return view
    }

    override final fun bind(view: View) {
        super.bind(view)
        bind(view.getTag(R.id.epoxy_view_layout) as T)
    }

    abstract fun bind(layout: T)
}

This works fine in theory, everything is syntactically OK but when I create concrete model class and use @EpoxyAttribute annotation, the generated model is wrong. The app crashes at runtime, because generated model is using my type instead of View and ClassCastException is thrown. I managed to workaround that by supply another generic type to my model like open abstract class EpoxyViewLayoutModel<V: View, T : ViewLayout> and not using that in any way but its not that nice. Am I doing something wrong? Or is it some kind of bug? Or this thing I'm trying to achieve is not possible? Thanks

elihart commented 7 years ago

@davidbilik Is the generated class creating an unbind method with type ViewLayout? I have a strong suspicion that is the case. Fortunately I happened to have fixed that issue yesterday :P I'll put up a PR, but I'm waiting on some last databinding improvements before I planned on pushing a new release.

If you need the fix urgently I can try to get a release out sooner for you, otherwise your work around should be fine for now.

elihart commented 7 years ago

This should fix it https://github.com/airbnb/epoxy/pull/200

By the way, I'm curious how your experience using Epoxy with Anko is. I haven't tried it in a production app so I'm not really aware of how it scales. Let me know if you see any opportunities for improving the integration.

Thanks again for reporting, and sorry about the bug!

elihart commented 7 years ago

By the way, the EpoxyViewLayoutModel class you have there to allow for the ViewLayout usage looks great. That's definitely the right way to approach it. Is that a common need in Anko that we should maybe build into Epoxy better?

davidbilik commented 7 years ago

Hi, thanks for a quick response. I forgot to mention in my workaround the problem with unbind method. It was the first problem, generated model had overriden method unbind with my ViewLayout type but this method was not in supertype. I've added it to the EpoxyViewLayoutModel class and compilation was ok. Now the app compiles and run but when binding viewholder, this exception is thrown

java.lang.ClassCastException: org.jetbrains.anko._LinearLayout cannot be cast to com.example.app.HeaderLayout
   at com.example.app.HeaderModel_.handlePreBind(HeaderModel_.java:18)
   at com.airbnb.epoxy.EpoxyViewHolder.bind(EpoxyViewHolder.java:32)
   at com.airbnb.epoxy.BaseEpoxyAdapter.onBindViewHolder(BaseEpoxyAdapter.java:105)
   at com.airbnb.epoxy.EpoxyControllerAdapter.onBindViewHolder(EpoxyControllerAdapter.java:9)
   at com.airbnb.epoxy.BaseEpoxyAdapter.onBindViewHolder(BaseEpoxyAdapter.java:14)
   at android.support.v7.widget.RecyclerView$Adapter.bindViewHolder(RecyclerView.java:6389)
   at android.support.v7.widget.RecyclerView$Recycler.tryBindViewHolderByDeadline(RecyclerView.java:5335)
   at android.support.v7.widget.RecyclerView$Recycler.tryGetViewHolderForPositionByDeadline(RecyclerView.java:5598)
   at android.support.v7.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:5440)
   at android.support.v7.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:5436)
   at android.support.v7.widget.LinearLayoutManager$LayoutState.next(LinearLayoutManager.java:2224)
   at android.support.v7.widget.GridLayoutManager.layoutChunk(GridLayoutManager.java:556)
   at android.support.v7.widget.LinearLayoutManager.fill(LinearLayoutManager.java:1511)
   at android.support.v7.widget.LinearLayoutManager.onLayoutChildren(LinearLayoutManager.java:595)
   at android.support.v7.widget.GridLayoutManager.onLayoutChildren(GridLayoutManager.java:170)
   at android.support.v7.widget.RecyclerView.dispatchLayoutStep2(RecyclerView.java:3583)
   at android.support.v7.widget.RecyclerView.dispatchLayout(RecyclerView.java:3312)
   at android.support.v7.widget.RecyclerView.onLayout(RecyclerView.java:3844)

I created a HeaderModel with HeaderLayout type. Mentioned line 18 is just the class definition line. In whole HeaderModel_ class HeaderLayout is used instead of view. I'm not sure if this problem is solvable, I do not have so much experience with annotation processing.

And for your questions - when we started with Anko it was a mess. Because with anko you are writing kotlin code, it doesnot have strict rules like xml so you can mix view definition/ data binding / business logic and when done bad, the code doesnt scale really well. But once we created strict rules and separated this responsibilities to different classes it's ok and I dont want to use xml layout definition anymore :) One of its rule was this ViewLayout class that serves a little as XML layout - we just define layout and view specific things there and bind them elsewhere. I can show some example of that if you want, I'm planning to write some article about that :) For question about supporting something like that in Epoxy - I don't know :) The community around Anko is not that big and if my solution will work I am fine with it :) I cannot imagine how the Epoxy Api would look like so I can take advantage of that

elihart commented 7 years ago

I believe you need to make the V: View type parameter of type org.jetbrains.anko._LinearLayout in your subclass.

davidbilik commented 7 years ago

Well its unfortunate because _LinearLayout is root view of my layout and this differs for different layouts

elihart commented 7 years ago

What does the subclass of your EpoxyViewLayoutModel look like?

I'm trying to push a snapshot release that includes the bug fix so you won't have to do this workaround

elihart commented 7 years ago

Ok, there is now a 2.1.0-SNAPSHOT release that you can try if you like to see if it fixes your problem.

davidbilik commented 7 years ago

ok, will try that and if it wont work i'll post my model class

davidbilik commented 7 years ago

It works 🎉

elihart commented 7 years ago

yay!

elihart commented 7 years ago

sorry for the trouble :(

Thanks for the background on your anko usage by the way. I was reading up on Anko and and saw the AnkoComponent (https://github.com/Kotlin/anko#ankocomponent). That seems like something we could standardize a model around - is there a reason you use your custom ViewLayout class instead of an AnkoComponent?

davidbilik commented 7 years ago

The usecase in example is very simple but we needed something more general also for Fragments and ViewHolders and AnkoComponent does not have so straightforward api so we've decided to create our custom implementation and it work just fine

davidbilik commented 7 years ago

Can I ask you a non-related question? Is there any way to get notified when models are rebuilt? I want to scroll to some position when my models are rebuilt but when model creation is asynchronous (I think) i'm not sure when I can search for model position to scroll to

elihart commented 7 years ago

Sure, ask away! The first time models are built it is synchronous, so scroll position in saved state can be restored. After that it is asynchronous. So if you only want to do the manual scroll after the first time models are built you could do it synchronously

Otherwise there is no callback for when models are built. Instead you could post a runnable call at the end of your buildModels method. Or, you could override the onModelBound and have that as a signal for when models are done building (that seems like an ugly/fragile approach though).

If you think Epoxy should provide an onModelsBuilt callback feel free to create a separate issue for it. I hadn't considered that use case, but we can see who else might need it and consider building support into the library