Fixed
Status Update
Comments
fi...@gmail.com <fi...@gmail.com> #2
cy...@zen.ly <cy...@zen.ly> #3
Project: platform/frameworks/support
Branch: androidx-main
commit 8aea0fa4e7bed4d9dee1f03482328014e4dd86c2
Author: John Nichol <jnichol@google.com>
Date: Fri Oct 21 17:21:35 2022
Correct ScalingLazyListState.centerItemIndex calculation
Change the calculation of centerItemIndex/centerItemOffset to ensure that the closest item to the center of the viewport is returned.
The previous (incorrect) behaviour found the first items that had its itemEnd below the center line.
As a result it was possible in some edge conditions for an items which sat just above the center line to be ignored and the first items below the center line to be selected - even it was further away from the center line.
Bug: 254257769
Test: ./gradlew :wear:compose:compose-material:connectedCheck --info --daemon
Relnote: "We have corrected the calculation of `ScalingLazyListState.centerItemIndex/centerItemOffset` so that if two items sit either side of the viewport center line the one that is closest will be considered as the centerItem."
Change-Id: I307091167e04914d1ae29d5324f84ec18ed7b8a8
M wear/compose/compose-material/src/androidAndroidTest/kotlin/androidx/wear/compose/material/ScalingLazyListLayoutInfoTest.kt
M wear/compose/compose-material/src/commonMain/kotlin/androidx/wear/compose/material/ScalingLazyListItemInfo.kt
M wear/compose/compose-material/src/commonMain/kotlin/androidx/wear/compose/material/ScalingLazyListState.kt
M wear/compose/integration-tests/demos/src/main/java/androidx/wear/compose/integration/demos/ScalingLazyColumnDemo.kt
https://android-review.googlesource.com/2263181
Branch: androidx-main
commit 8aea0fa4e7bed4d9dee1f03482328014e4dd86c2
Author: John Nichol <jnichol@google.com>
Date: Fri Oct 21 17:21:35 2022
Correct ScalingLazyListState.centerItemIndex calculation
Change the calculation of centerItemIndex/centerItemOffset to ensure that the closest item to the center of the viewport is returned.
The previous (incorrect) behaviour found the first items that had its itemEnd below the center line.
As a result it was possible in some edge conditions for an items which sat just above the center line to be ignored and the first items below the center line to be selected - even it was further away from the center line.
Bug: 254257769
Test: ./gradlew :wear:compose:compose-material:connectedCheck --info --daemon
Relnote: "We have corrected the calculation of `ScalingLazyListState.centerItemIndex/centerItemOffset` so that if two items sit either side of the viewport center line the one that is closest will be considered as the centerItem."
Change-Id: I307091167e04914d1ae29d5324f84ec18ed7b8a8
M wear/compose/compose-material/src/androidAndroidTest/kotlin/androidx/wear/compose/material/ScalingLazyListLayoutInfoTest.kt
M wear/compose/compose-material/src/commonMain/kotlin/androidx/wear/compose/material/ScalingLazyListItemInfo.kt
M wear/compose/compose-material/src/commonMain/kotlin/androidx/wear/compose/material/ScalingLazyListState.kt
M wear/compose/integration-tests/demos/src/main/java/androidx/wear/compose/integration/demos/ScalingLazyColumnDemo.kt
fi...@gmail.com <fi...@gmail.com> #4
se...@gmail.com <se...@gmail.com> #6
yb...@google.com <yb...@google.com>
yb...@google.com <yb...@google.com> #7
ap...@google.com <ap...@google.com> #8
fi...@gmail.com <fi...@gmail.com> #9
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.wear.compose:compose-material:1.1.0-rc01
yb...@google.com <yb...@google.com> #10
sorry it missed alpha 2 but should be out in alpha 3 of recyclerview.
here is the CL to follow:
once it is merged you can use a build from CI but keep in mind that it has latest changes in RV.
ap...@google.com <ap...@google.com> #11
Project: platform/frameworks/support
Branch: androidx-master-dev
commit adebffff1a528c659f635280902993d5a30ecfc5
Author: Yigit Boyar <yboyar@google.com>
Date: Mon Mar 09 16:24:52 2020
Fix duplicate items handling in diff util
This CL fixes a bug in DiffUtil where it would get confused when
and item in the original list is duplicated, by shortcircuting the
path between the duplicate instances.
This CL fixes that bug by "unfortunately" re-implementing the
snake finding and path finding logic. I just couldn't figure out
what was wrong in the previous implementation and it was just too
hard to read with a billion offsets etc everywhere.
This implementation is easier to read, more true to the algorithm
in the paper at the cost of some more function calls.
On a pixel 3 (where allocations matter less), it is consistently
faster ~10-20% except when large item chunks move where it is
5% slower.
see measurements:https://paste.googleplex.com/6430892597706752
Bug: 123376278
Test: DiffUtilTest with duplicate cases
Change-Id: I94f64db7162c0107f9af512cdd55c0d39b44cabf
M leanback/leanback/src/androidTest/java/androidx/leanback/widget/ObjectAdapterTest.java
M paging/runtime/src/androidTest/java/androidx/paging/NullPaddedListDiffHelperTest.kt
M recyclerview/recyclerview/src/main/java/androidx/recyclerview/widget/DiffUtil.java
M recyclerview/recyclerview/src/test/java/androidx/recyclerview/widget/DiffUtilTest.kt
https://android-review.googlesource.com/1253271
Branch: androidx-master-dev
commit adebffff1a528c659f635280902993d5a30ecfc5
Author: Yigit Boyar <yboyar@google.com>
Date: Mon Mar 09 16:24:52 2020
Fix duplicate items handling in diff util
This CL fixes a bug in DiffUtil where it would get confused when
and item in the original list is duplicated, by shortcircuting the
path between the duplicate instances.
This CL fixes that bug by "unfortunately" re-implementing the
snake finding and path finding logic. I just couldn't figure out
what was wrong in the previous implementation and it was just too
hard to read with a billion offsets etc everywhere.
This implementation is easier to read, more true to the algorithm
in the paper at the cost of some more function calls.
On a pixel 3 (where allocations matter less), it is consistently
faster ~10-20% except when large item chunks move where it is
5% slower.
see measurements:
Bug: 123376278
Test: DiffUtilTest with duplicate cases
Change-Id: I94f64db7162c0107f9af512cdd55c0d39b44cabf
M leanback/leanback/src/androidTest/java/androidx/leanback/widget/ObjectAdapterTest.java
M paging/runtime/src/androidTest/java/androidx/paging/NullPaddedListDiffHelperTest.kt
M recyclerview/recyclerview/src/main/java/androidx/recyclerview/widget/DiffUtil.java
M recyclerview/recyclerview/src/test/java/androidx/recyclerview/widget/DiffUtilTest.kt
yb...@google.com <yb...@google.com>
fi...@gmail.com <fi...@gmail.com> #12
thanks!
sa...@gmail.com <sa...@gmail.com> #13
Was this released? If yes in which version? Thank you
yb...@google.com <yb...@google.com> #14
unfortunately it is still in 1.2 alpha due to some dependencies but you can use the alpha of RecyclerView, it is fairly stable.
Description
I'm testing it on String items so it shouldn't be related to my own recycler implementation.
I've created simple Instrumented Test that is failing on mentioned case:
#####################################
package com.example.diffutilbug
import android.util.Log
import android.view.View
import android.view.ViewGroup
import androidx.recyclerview.widget.DiffUtil
import androidx.recyclerview.widget.ListAdapter
import androidx.recyclerview.widget.RecyclerView
import junit.framework.Assert.assertTrue
import kotlinx.coroutines.*
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.BlockJUnit4ClassRunner
@RunWith(BlockJUnit4ClassRunner::class)
internal class ExampleUnitTest {
@Test
fun testDiffUtil4() {
val handler = CoroutineExceptionHandler { _, exception ->
throw exception
}
// adapter compare items :
// areItemsTheSame -> compare length of String
// areContentsTheSame -> compare content with ==
val adapter = StringAdapterJunit(handler)
runBlocking {
adapter.submitList(
mutableListOf<String>(
"1",//1,
"22",//2,
"333",//3,
"4444",//4,
"55555",//5,
"666666",//6,
"7777777",//7,
"88888888",//8,
"999999999",//9,
"55555",//5,
"1010101010",//10,
"1010109999",//10,
"55555",//5,
"1313131313",//10,
"1414141414",//10,
"55555",//5,
"1313131313",//10,
"1414141414",//10,
"55555"//5
)
)
delay(40)
adapter.submitList(
mutableListOf<String>(
"55555",//5,
"1010101010",//10,
"1010109999",//10,
"55555",//11,
"1313131313",//10,
"1414141414",//10,
"11111111111"//11
)
)
delay(500)
}
}
}
// Stub Adapter for Strings that uses DiffUtil underneath.
// logs all callbacks to logcat
class StringAdapterJunit(val handler: CoroutineExceptionHandler) : ListAdapter<String, RecyclerView.ViewHolder>(object : DiffUtil.ItemCallback<String>() {
override fun areItemsTheSame(oldItem: String, newItem: String): Boolean {
Log.e("DiffUtilTest", "areItemsTheSame comparing $oldItem with $newItem = ${oldItem.length == newItem.length}")
return oldItem.length == newItem.length
}
override fun areContentsTheSame(oldItem: String, newItem: String): Boolean {
//should be called only if areContentsTheSame == true
Log.e(
"DiffUtilTest",
"areContentsTheSame error = ${oldItem.length != newItem.length} comparing $oldItem with $newItem"
)
runBlocking {
GlobalScope.launch(handler + Dispatchers.Main) {
assertTrue("areContentsTheSame can be called only if areItemsTheSame return true" , areItemsTheSame(oldItem, newItem))
}.join()
}
return oldItem == newItem
}
override fun getChangePayload(oldItem: String, newItem: String): Any? {
//should be called only if areItemsTheSame = true and areContentsTheSame = false
Log.e(
"DiffUtilTest",
"getChangePayload error = ${oldItem.length == newItem.length && oldItem == newItem} $oldItem with $newItem"
)
return null
}
}) {
// stub implementation on adapter - never used
override fun onCreateViewHolder(parent: ViewGroup, viewType: Int) = object : RecyclerView.ViewHolder(View(null)) {}
override fun onBindViewHolder(holder: RecyclerView.ViewHolder, position: Int) {}
override fun getItemViewType(position: Int): Int = getItem(position).length
}
#####################################
and gradle dependencies needed for it:
dependencies {
implementation"org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
testImplementation 'junit:junit:4.12'
androidTestImplementation 'androidx.test:runner:1.1.1'
androidTestImplementation 'androidx.test.espresso:espresso-core:3.1.1'
androidTestImplementation 'androidx.test.ext:junit:1.1.0'
//coroutines
implementation 'org.jetbrains.kotlinx:kotlinx-coroutines-core:1.0.1'
implementation 'org.jetbrains.kotlinx:kotlinx-coroutines-android:1.0.1'
implementation 'androidx.recyclerview:recyclerview:1.0.0'
}
#####################################
please note that you need to add
android.useAndroidX=true
android.enableJetifier=true
in your gradle.properties
coroutines and handler for exceptions added because DiffUtil computes diff on background thread and JUnit handles assertion only on main thread