airbnb / epoxy

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

position in `fun isStickyHead(position: Int)` larger than models size. #1148

Open Sean-Y-X opened 3 years ago

Sean-Y-X commented 3 years ago

I want a summary model to be the sticky header so I added the following check.

override fun isStickyHeader(position: Int): Boolean {
    return adapter.getModelAtPosition(position).let { it is SummaryModel }
}

I noticed that after the state of this view changed, under some circumstances the view would crash. Through debugging I found that the position that passed into this function was larger than the count of the models. I have to do a check like this:

override fun isStickyHeader(position: Int): Boolean {
    return position < adapter.copyOfModels.size  && adapter.getModelAtPosition(position).let { it is SummaryModel }
}

However, I feel pretty weird about it. I wonder if it is a bug in this library?

Sean-Y-X commented 3 years ago

after a brief digging, I think positionStart (Position of the first item that was inserted) is wrong.

elihart commented 3 years ago

Thanks for reporting, it could very well be a bug.

The sticky support was added by @AkshayChordiya so maybe he can help. If he isn't available I'm not in a great position to review the change and know if it is right, but if you can write unit tests that show the issue and correct it then I'm willing to accept a PR with the fix 👍

AkshayChordiya commented 3 years ago

Thanks for reporting. Umm yeah seems like a bug :thinking: I'll take a look at during the weekend hopefully

nomanr commented 3 years ago

Reported #1084 few months back. Seems like they are related.

Sean-Y-X commented 3 years ago

Reported #1084 few months back. Seems like they are related.

Yes, very likely they are due to the same reason.

samuelchou commented 2 years ago

I encountered the similar problem, except when isStickyHeader is called, the position value it passed is negative.

I'm using Epoxy 4.6.4.

Fatal Exception: java.lang.IndexOutOfBoundsException: Invalid item position -88(-88). Item count:94 com.airbnb.epoxy.EpoxyRecyclerView{c9f6fc8 VFED..... ......ID 0,816-1080,2631 #7f080199 app:id/itemContainer}, adapter:com.airbnb.epoxy.EpoxyControllerAdapter@e3de88d, layout:com.airbnb.epoxy.stickyheader.StickyHeaderLinearLayoutManager@7452a42, context:dagger.hilt.android.internal.managers.ViewComponentManager$FragmentContextWrapper@7100797
       at androidx.recyclerview.widget.RecyclerView$Recycler.tryGetViewHolderForPositionByDeadline(RecyclerView.java:6326)
       at androidx.recyclerview.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:6300)
       at androidx.recyclerview.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:6296)
       at com.airbnb.epoxy.stickyheader.StickyHeaderLinearLayoutManager.createStickyHeader(StickyHeaderLinearLayoutManager.kt:283)
       at com.airbnb.epoxy.stickyheader.StickyHeaderLinearLayoutManager.updateStickyHeader(StickyHeaderLinearLayoutManager.kt:253)
       at com.airbnb.epoxy.stickyheader.StickyHeaderLinearLayoutManager.onLayoutChildren(StickyHeaderLinearLayoutManager.kt:111)
       at androidx.recyclerview.widget.RecyclerView.dispatchLayoutStep2(RecyclerView.java:4309)
       at androidx.recyclerview.widget.RecyclerView.dispatchLayout(RecyclerView.java:4012)
       // ...

So I decided to use more safe-condition in isStickyHeader:

override fun isStickyHeader(position: Int): Boolean {
    return if (adapter.itemCount <= position || position < 0) {
        val exception = IndexOutOfBoundsException("Size: ${adapter.itemCount}, called: $position")
        Log.w(
            TAG, "isStickyHeader: called at illegal position. IGNORED and treat as viewed as not-header.",
            exception
        )
        FirebaseCrashlytics.getInstance().recordException(exception)
        false
    } else {
        adapter.getModelAtPosition(position) is MyStickyHeaderBindingModel_
    }
}

I'll just leave it here to warn future users. (And to wait for fix, of course.)

WessimBetclic commented 2 years ago

Hello ! Any news on this particular bug ?