BeksOmega / looping-layout

A looping LayoutManager for the Android RecyclerView.
Apache License 2.0
271 stars 16 forks source link

Fix possible crash of IndexOutOfBoundsException when index is -1 #64

Closed Kevinrob closed 2 years ago

Kevinrob commented 2 years ago

:clap: Resolves

[no issue was created] ### :star2: Description

We have some crash about trying to get item -1.
The adapter goes from a lot of items to 0 and the crash popup sometimes.

I think that this guard clause should protect us from this crash.

:bug: Testing

It's really hard to reproduce this crash. I don't know how we can test it.

:thought_balloon: Other info

full stacktrace:

Fatal Exception: java.lang.IndexOutOfBoundsException: Invalid item position -1(-1). Item count:0 [...]{e91fd7 VFED..... ......I. 0,407-1080,729 #7f0a0869 app:id/sushi_train_sushi_container}, adapter:[...]@6b7353f, layout:com.bekawestberg.loopinglayout.library.LoopingLayoutManager@bf50e0c, context:[...]@2b8f134
       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.bekawestberg.loopinglayout.library.LoopingLayoutManager.createViewForIndex(LoopingLayoutManager.java:356)
       at com.bekawestberg.loopinglayout.library.LoopingLayoutManager.onLayoutChildren(LoopingLayoutManager.java:207)
       at androidx.recyclerview.widget.RecyclerView.dispatchLayoutStep2(RecyclerView.java:4309)
       at androidx.recyclerview.widget.RecyclerView.onMeasure(RecyclerView.java:3686)
       at android.view.View.measure(View.java:24872)
       at androidx.constraintlayout.widget.ConstraintLayout$Measurer.measure(ConstraintLayout.java:811)
       at androidx.constraintlayout.core.widgets.analyzer.BasicMeasure.measure(BasicMeasure.java:466)
       at androidx.constraintlayout.core.widgets.analyzer.BasicMeasure.measureChildren(BasicMeasure.java:134)
       at androidx.constraintlayout.core.widgets.analyzer.BasicMeasure.solverMeasure(BasicMeasure.java:278)
       at androidx.constraintlayout.core.widgets.ConstraintWidgetContainer.measure(ConstraintWidgetContainer.java:120)
       at androidx.constraintlayout.widget.ConstraintLayout.resolveSystem(ConstraintLayout.java:1594)
       at androidx.constraintlayout.widget.ConstraintLayout.onMeasure(ConstraintLayout.java:1708)
       at android.view.View.measure(View.java:24872)
       at androidx.recyclerview.widget.RecyclerView$LayoutManager.measureChildWithMargins(RecyclerView.java:9681)
       at androidx.recyclerview.widget.LinearLayoutManager.layoutChunk(LinearLayoutManager.java:1657)
       at androidx.recyclerview.widget.LinearLayoutManager.fill(LinearLayoutManager.java:1591)
       at androidx.recyclerview.widget.LinearLayoutManager.onLayoutChildren(LinearLayoutManager.java:678)
       at androidx.recyclerview.widget.RecyclerView.dispatchLayoutStep1(RecyclerView.java:4255)
       at androidx.recyclerview.widget.RecyclerView.dispatchLayout(RecyclerView.java:4010)
       at androidx.recyclerview.widget.RecyclerView.onLayout(RecyclerView.java:4578)
       at android.view.View.layout(View.java:22244)
       at android.view.ViewGroup.layout(ViewGroup.java:6494)
       at androidx.swiperefreshlayout.widget.SwipeRefreshLayout.onLayout(SwipeRefreshLayout.java:625)
       at android.view.View.layout(View.java:22244)
       at android.view.ViewGroup.layout(ViewGroup.java:6494)
       at android.widget.RelativeLayout.onLayout(RelativeLayout.java:1103)
       at android.view.View.layout(View.java:22244)
       at android.view.ViewGroup.layout(ViewGroup.java:6494)
       at android.widget.FrameLayout.layoutChildren(FrameLayout.java:334)
       at android.widget.FrameLayout.onLayout(FrameLayout.java:270)
       at android.view.View.layout(View.java:22244)
       at android.view.ViewGroup.layout(ViewGroup.java:6494)
       at androidx.constraintlayout.widget.ConstraintLayout.onLayout(ConstraintLayout.java:1873)
       at android.view.View.layout(View.java:22244)
       at android.view.ViewGroup.layout(ViewGroup.java:6494)
       at android.widget.FrameLayout.layoutChildren(FrameLayout.java:334)
       at android.widget.FrameLayout.onLayout(FrameLayout.java:270)
       at android.view.View.layout(View.java:22244)
       at android.view.ViewGroup.layout(ViewGroup.java:6494)
       at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1857)
       at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1701)
       at android.widget.LinearLayout.onLayout(LinearLayout.java:1610)
       at android.view.View.layout(View.java:22244)
       at android.view.ViewGroup.layout(ViewGroup.java:6494)
       at android.widget.FrameLayout.layoutChildren(FrameLayout.java:334)
       at android.widget.FrameLayout.onLayout(FrameLayout.java:270)
       at android.view.View.layout(View.java:22244)
       at android.view.ViewGroup.layout(ViewGroup.java:6494)
       at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1857)
       at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1701)
       at android.widget.LinearLayout.onLayout(LinearLayout.java:1610)
       at android.view.View.layout(View.java:22244)
       at android.view.ViewGroup.layout(ViewGroup.java:6494)
       at android.widget.FrameLayout.layoutChildren(FrameLayout.java:334)
       at android.widget.FrameLayout.onLayout(FrameLayout.java:270)
       at com.android.internal.policy.DecorView.onLayout(DecorView.java:1084)
       at android.view.View.layout(View.java:22244)
       at android.view.ViewGroup.layout(ViewGroup.java:6494)
       at android.view.ViewRootImpl.performLayout(ViewRootImpl.java:3631)
       at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:3111)
       at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:2109)
       at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:8575)
       at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1256)
       at android.view.Choreographer.doCallbacks(Choreographer.java:995)
       at android.view.Choreographer.doFrame(Choreographer.java:887)
       at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1224)
       at android.os.Handler.handleCallback(Handler.java:900)
       at android.os.Handler.dispatchMessage(Handler.java:103)
       at android.os.Looper.loop(Looper.java:219)
       at android.app.ActivityThread.main(ActivityThread.java:8393)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:513)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1055)
BeksOmega commented 2 years ago

HI! Thank you for the pull request :D Sorry it's taken me a bit to get back to you, there's been a lot going on IRL hehe.

I really appreciate you putting this up, but I'd rather not add a guard condition if we don't know how to reproduce the crash. It seems like this is just band-aid-ing and underlying problem, which I'm not comfortable with :/

However, crashes are definitely bad. So if this is giving you a lot of trouble, I'd recommend composing a new LayoutManager that uses the LoopingLayoutManager internally. It would look something like this:

class MyLayoutManager : LayoutManager, RecyclerView.SmoothScroller.ScrollVectorProvider {
  private lateinit var loopingLayoutManager : LoopingLayoutManager

    public constructor(context: Context, orientation: Int = VERTICAL, reverseLayout: Boolean = false) {
        loopingLayoutManager = LoopingLayoutManager(context, orientation, reverseLayout)
    }

    public constructor(context: Context, attrs: AttributeSet, defStyleAttr: Int, defStyleRes: Int) {
        loopingLayoutManager = LoopingLayoutManager(context, attrs, defStyleAttr, defStyleRes)
    }

    public override fun onLayoutChildren(recycler: RecyclerView.Recycler, state: RecyclerView.State) {
        if (state.itemCount == 0) return  // Guard condition for crash bug
        loopingLayoutManager.onLayoutChildren(recycler, state)
    }

    // Wrap all other public functions you choose to, etc
}

Or if there's a way to reproduce the issue so we can figure out the root cause, then we can investigate to figure out if the guard condition is the right solution, or if there's a bug somewhere else.

But that's just my opinion! I'm happy to discuss more with you. Thank you again so much for putting up this PR, I really appreciate it =)

Kevinrob commented 2 years ago

We will not use this library anymore. So we don't plan to take time for this bug anymore. Thank you for your answer!