square / cycler

Apache License 2.0
791 stars 27 forks source link

Recycler should defer update calls that are received while binding views #26

Open steve-the-edwards opened 4 years ago

steve-the-edwards commented 4 years ago

I am getting an IllegalStateException during the adapter notifications in the launched coroutine in the update block because I had injected the Dispatchers.main.immediate CoroutineDispatcher instead of the Dispatchers.main CoroutineDispatcher so this coroutine for layout wasn't posted back to the main thread but run synchronously leading to the exception during layout.

Now, using the immediate dispatcher may be 'user error' but we could protect against it with a yield() in update()'s launch.

steve-the-edwards commented 4 years ago

CC: @zach-klippenstein this was the reason for the problem I referred to.

zach-klippenstein commented 4 years ago

Sorry, not sure where you referred to this problem. Could you post a stack trace or a slice of one here? I thought RecyclerView supported synchronous use just fine, so I'm not sure what the actual problem is.

zach-klippenstein commented 4 years ago

This is the error:

java.lang.IllegalStateException: Cannot call this method while RecyclerView is computing a layout or scrolling
at androidx.recyclerview.widget.RecyclerView.assertNotInLayoutOrScroll(RecyclerView.java:3051)
at androidx.recyclerview.widget.RecyclerView$RecyclerViewDataObserver.onChanged(RecyclerView.java:5536)
at androidx.recyclerview.widget.RecyclerView$AdapterDataObservable.notifyChanged(RecyclerView.java:12253)
at androidx.recyclerview.widget.RecyclerView$Adapter.notifyDataSetChanged(RecyclerView.java:7354)
at com.squareup.cycler.Update$generateDataChangesLambdas$2.invoke(Update.kt:86)
at com.squareup.cycler.Update$generateDataChangesLambdas$2.invoke(Update.kt:29)
at com.squareup.cycler.Recycler$update$1.invokeSuspend(Recycler.kt:193)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
at kotlinx.coroutines.DispatchedContinuationKt.resumeCancellableWith(DispatchedContinuation.kt:314)
at kotlinx.coroutines.intrinsics.CancellableKt.startCoroutineCancellable(Cancellable.kt:26)
at kotlinx.coroutines.CoroutineStart.invoke(CoroutineStart.kt:109)
at kotlinx.coroutines.AbstractCoroutine.start(AbstractCoroutine.kt:158)
at kotlinx.coroutines.BuildersKt__Builders_commonKt.launch(Builders.common.kt:54)
at kotlinx.coroutines.BuildersKt.launch(Unknown Source:1)
at kotlinx.coroutines.BuildersKt__Builders_commonKt.launch$default(Builders.common.kt:47)
at kotlinx.coroutines.BuildersKt.launch$default(Unknown Source:1)
at com.squareup.cycler.Recycler.update(Recycler.kt:179)
// some internal code
at com.squareup.cycler.StandardRowSpec$StandardViewHolder.bind(StandardRowSpec.kt:83)
at com.squareup.cycler.Recycler$Adapter.onBindViewHolder(Recycler.kt:493)
at com.squareup.cycler.Recycler$Adapter.onBindViewHolder(Recycler.kt:405)
at androidx.recyclerview.widget.RecyclerView$Adapter.onBindViewHolder(RecyclerView.java:7065)

So it looks like a UI event is being sent by the row binding logic, which causes a rerender and another update to the recycler. If this update doesn't actually require any changes to the recycler, we could just not call update. But I am guessing that's already how DiffUtil works, so the fact that this is crashing means there was a significant update, so the problem is that there's actually an interesting change that we can't do just yet.

I think the best solution here is for Recycler to detect when update is called during a bind pass, and defer applying those updates until after the bind has finished. A simple unconditional yield() just adds unnecessary asynchronicity.