airbnb / epoxy

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

Epoxy ModelView binding issues with BaseMvRxFragment #612

Open laszlo-galosi opened 5 years ago

laszlo-galosi commented 5 years ago

Hi @elihart,

I don't know this is the proper repo to submit this issue or the MvRx would be.

I use ModelView annotated models, with simpleController in EpoxyRecyclerView like in the code sample. I use the MvRx, Epoxy ModelView pattern in multiple BaseMvRxFragment. I use different layout for CompoundLabelRow models by setting layoutRes property, simpy inflating it and added the inflated view to the base FrameLayout. The views appears messed up as they were mixed between different fragments, in some cases, the title view shows no text, sometimes completely wrong layout. I have unique id-s for all my models. What's wrong with this approach? I suspect something with the layoutRes property, or am I missing some important step, or something to override? The state updates are correct, I check the model properties in onBind {} by logging it, and they are ok. But the view appears messed up, between different fragments, inconsistently,
sometimes the first item displays wrong layout file, than the rest, used in another fragment's, simpleController.

My model is something like this:

@ModelView(
    autoLayout = ModelView.Size.MATCH_WIDTH_WRAP_HEIGHT
)
open class CompoundLabelRow @JvmOverloads constructor(
    context: Context,
    attrs: AttributeSet? = null,
    defStyleAttr: Int = 0
) : FrameLayout(context, attrs, defStyleAttr) {

    open val titleView: TextView? by lazy {
        findViewById<TextView>(R.id.itemTitle)
    }

    open val iconView: ImageView? by lazy {
        findViewById<ImageView>(R.id.itemIcon)
    }

    var clickListener: View.OnClickListener? = null
        @CallbackProp set

    var icon: Drawable? = null
        @CallbackProp set

    @TextProp
    lateinit var title: CharSequence

    init {
        inflate(context, R.layout.row_compound, this)
    }

    @ModelProp
    fun setLayoutRes(@LayoutRes layoutRes: Int = R.layout.row_compound) {
        this.takeIf { layoutRes > 0 }?.apply {
            removeAllViews()
            inflate(context, layoutRes, this)
        }
    }

    @AfterPropsSet
    fun useProps() {
        titleView?.text = title
        titleView?.takeIf { iconView == null }?.let {
            titleView?.setCompoundDrawablesWithIntrinsicBounds(icon, null, null, null)
        } ?: iconView?.setImageDrawable(icon)

        this.setOnClickListener(clickListener)
    }
}

I bind the models in simpleController like this:

simpleController(viewModel) {state ->
state.partners.forEach { partner ->
 compoundItem{
                id(partner.id)
                layoutRes(R.layout.row_conpount__w_swipe_delete)
                title(partner.name)
                icon(context?.toDrawable(R.drawable.ic_nav_profile))
                onBind { model, holder, _ ->
                    Log.d("onBind: $model")        
                }
                clickListener(OnCliclListener{
                   ...
                })
            }
}
elihart commented 5 years ago

You're doing a very strange, inefficient thing by passing a layout and inflating it again every time the view is bound - this completely get's rid of the benefits of recycling views.

Also I don't think lazy is going to work as you have it - lazy caches the first usage of a value. What you're doing is holding on to the first "title" view, and then you're creating a new one by inflating another view, so your reference is going to be wrong. I'm guessing that's why the text is blank

laszlo-galosi commented 5 years ago

Thanks for the quick response. So instead of using layoutRes property, maybe I should use Different ModelView classes, for different kind of views? Maybe use layout overrides? Yeah, the lazy is not too smart way, thank you for pointing out.

elihart commented 5 years ago

If the layout variations are similar enough that they have the same items, just arranged/styled differently, then I would recommend creating subclasses of your view that simply provide the layout that should be inflated, and possibly custom logic for handling the props

laszlo-galosi commented 5 years ago

Oh I see thank you, I ended up with subclassing like this: The setLayoutRes() function is the base class function which initializes, the common views, title, icon, and border.

@ModelView(autoLayout = ModelView.Size.MATCH_WIDTH_WRAP_HEIGHT)
class PartnerRow @JvmOverloads constructor(
    context: Context,
    attrs: AttributeSet? = null,
    defStyleAttr: Int = 0
) : CompoundLabelRow(context, attrs, defStyleAttr) {

    var dateView: TextView? = null
    var foregroundView: View? = null

    init {
        setLayoutRes(R.layout.row_partner_w_swipe_delete)
        dateView = findViewById(R.id.itemDate)
        foregroundView = findViewById(R.id.foreground)
    }

    var birthDate: CharSequence? = null
        @TextProp set

    @AfterPropsSet
    override fun useProps() {
        super.useProps()
        birthDate?.let { dateView?.text = birthDate }
        foregroundView?.setOnClickListener {
            clickListener.invoke()
        }
    }
}