Put together an checker to check validity of onChange calls

bento/build.gradle

@@ -40,6 +40,7 @@ task androidSourcesJar(type: Jar) {
 dependencies {
     // Kotlin
     implementation Libs.KOTLIN
+    implementation Libs.KOTLIN_REFLECT
 
     // Apache Commons
     implementation Libs.APACHE_COMMONS

bento/src/main/java/com/yelp/android/bento/core/ComponentGroup.java

@@ -35,6 +35,7 @@ public class ComponentGroup extends Component {
     private final Map<Component, ComponentDataObserver> mComponentDataObserverMap = new HashMap<>();
 
     private final ComponentGroupObservable mObservable = new ComponentGroupObservable();
+    private final NotifyChecker mNotifyChecker = new NotifyChecker();
 
     public ComponentGroup() {
         mSpanSizeLookup =
@@ -166,6 +167,7 @@ public ComponentGroup addComponent(int index, @NonNull final Component component
         ComponentDataObserver componentDataObserver = new ChildComponentDataObserver(component);
         component.registerComponentDataObserver(componentDataObserver);
         mComponentDataObserverMap.put(component, componentDataObserver);
+        mNotifyChecker.prefetch(component);
 
         notifyItemRangeInserted(insertionStartIndex, component.getCountInternal());
         mObservable.notifyOnChanged();
@@ -346,7 +348,10 @@ public void unregisterComponentGroupObserver(@NonNull ComponentGroupDataObserver
     public Object getItem(int position) {
         RangedValue<Component> compPair = mComponentAccordionList.rangedValueAt(position);
         Component component = mComponentAccordionList.valueAt(position);
-        return component.getItemInternal(position - compPair.mRange.mLower);
+        int positionInComponent = position - compPair.mRange.mLower;
+        Object itemInternal = component.getItemInternal(positionInComponent);
+        mNotifyChecker.save(component, positionInComponent, itemInternal);
+        return itemInternal;
     }
 
     /**
@@ -591,6 +596,7 @@ private void cleanupComponent(@NonNull Component component) {
                 entry.setValue(entry.getValue() - 1);
             }
         }
+        mNotifyChecker.remove(component);
 
         mObservable.notifyOnComponentRemoved(component);
     }
@@ -610,6 +616,7 @@ private ChildComponentDataObserver(@NonNull Component component) {
 
         @Override
         public void onChanged() {
+            mNotifyChecker.onChanged(mComponent);
             int listPosition = mComponentIndexMap.get(mComponent);
             Range originalRange = mComponentAccordionList.get(listPosition).mRange;
             int newSize = mComponent.getCountInternal();
@@ -621,6 +628,7 @@ public void onChanged() {
 
         @Override
         public void onItemRangeChanged(int positionStart, int itemCount) {
+            mNotifyChecker.onItemRangeChanged(mComponent, positionStart, itemCount);
             int listPosition = mComponentIndexMap.get(mComponent);
             Range originalRange = mComponentAccordionList.get(listPosition).mRange;
 
@@ -630,6 +638,7 @@ public void onItemRangeChanged(int positionStart, int itemCount) {
 
         @Override
         public void onItemRangeInserted(int positionStart, int itemCount) {
+            mNotifyChecker.onItemRangeInserted(mComponent, positionStart, itemCount);
             int listPosition = mComponentIndexMap.get(mComponent);
             Range originalRange = mComponentAccordionList.get(listPosition).mRange;
             mComponentAccordionList.set(
@@ -643,6 +652,7 @@ public void onItemRangeInserted(int positionStart, int itemCount) {
 
         @Override
         public void onItemRangeRemoved(int positionStart, int itemCount) {
+            mNotifyChecker.onItemRangeRemoved(mComponent, positionStart, itemCount);
             int listPosition = mComponentIndexMap.get(mComponent);
             Range originalRange = mComponentAccordionList.get(listPosition).mRange;
             mComponentAccordionList.set(
@@ -656,6 +666,7 @@ public void onItemRangeRemoved(int positionStart, int itemCount) {
 
         @Override
         public void onItemMoved(int fromPosition, int toPosition) {
+            mNotifyChecker.onItemMoved(mComponent, fromPosition, toPosition);
             int listPosition = mComponentIndexMap.get(mComponent);
             Range originalRange = mComponentAccordionList.get(listPosition).mRange;
 

bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt

@@ -0,0 +1,239 @@
+package com.yelp.android.bento.core
+
+import android.util.Log
+import com.yelp.android.bento.utils.EqualsHelper.deepEquals
+import java.lang.ref.WeakReference
+
+private const val TAG = "NotifyChecker"
+
+class NotifyChecker {
+    private val componentsToItem: MutableMap<Component, ItemStorage> = mutableMapOf()
+
+    fun prefetch(component: Component) {
+        val items = componentsToItem.getOrPut(component) {
+            ItemStorage(component.countInternal)
+        }
+
+        val result = runCatching {
+            for (position in 0 until component.countInternal) {
+                items[position] = component.getItemInternal(position)
+            }
+        }
+        if (result.isFailure) {
+            Log.v(TAG, "Could not prefetch $component", result.exceptionOrNull())
+        }
+    }
+
+    fun save(component: Component, position: Int, item: Any?) {
+        val items = componentsToItem.getOrPut(component) {
+            ItemStorage(component.countInternal)
+        }
+
+        items[position] = item
+    }
+
+    fun remove(component: Component) {
+        componentsToItem.remove(component)
+    }
+
+    fun onChanged(component: Component) {
+        val verifyChange = verifyOnChanged(component)
+        if (verifyChange is NotifyCheckResult.IncorrectOnChange) {
+            Log.d(TAG, "onChanged for $component: $verifyChange")
+        }
+    }
+
+    fun onItemRangeChanged(component: Component, positionStart: Int, itemCount: Int) {
+        val verifyChange = verifyItemRangeChanged(component, positionStart, itemCount)
+        if (verifyChange is NotifyCheckResult.IncorrectOnItemRangeChange) {
+            Log.d(TAG, "onItemRangeChanged for $component: $verifyChange")
+        }
+    }
+
+    fun onItemRangeInserted(component: Component, positionStart: Int, itemCount: Int) {
+        val itemStorage = componentsToItem[component] ?: return
+
+        itemStorage.onItemRangeInserted(positionStart, itemCount)
+    }
+
+    fun onItemRangeRemoved(component: Component, positionStart: Int, itemCount: Int) {
+        val itemStorage = componentsToItem[component] ?: return
+
+        itemStorage.onItemRangeRemoved(positionStart, itemCount)
+    }
+
+    fun onItemMoved(component: Component, fromPosition: Int, toPosition: Int) {
+        val itemStorage = componentsToItem[component] ?: return
+
+        itemStorage.onItemMoved(fromPosition, toPosition)
+    }
+
+    // Check to confirm that the whole component indeed changed.
+    private fun verifyOnChanged(component: Component): NotifyCheckResult {
+        val itemStorage = componentsToItem[component]
+        if (itemStorage == null) {
+            return NotifyCheckResult.NotEnoughData
+        } else if (!itemStorage.isFullyPrefetched) {
+            return NotifyCheckResult.NotEnoughData
+        }
+
+        val updatedItems = ItemStorage(component.countInternal)
+
+        val unchanged = mutableListOf<Int>()
+        // We can also keep track of the "null"? A bunch of components use null as items,
+        // like dividers, simple components, and so on.
+        val undecided = mutableListOf<Int>()
+
+        when {
+            itemStorage.items.size == component.countInternal -> {
+                // Same size, let's compare items one by one.
+                itemStorage.items.forEachIndexed { index, weakReference ->
+                    val item: Any? = component.getItem(index)
+                    updatedItems[index] = item
+
+                    if (item == null) {
+                        undecided.add(index)
+                    } else if (item.deepEquals(weakReference?.get())) {
+                        unchanged.add(index)
+                    }
+                }
+            }
+            itemStorage.items.size < component.countInternal -> {
+                // Let's compare the stored items, to see if it's okay to call onChanged,
+                // or if we should call onItemRangeInsertedInstead
+                itemStorage.items.forEachIndexed { index, weakReference ->
+                    val item: Any? = component.getItem(index)
+                    updatedItems[index] = item
+
+                    if (item == null) {
+                        undecided.add(index)
+                    } else if (item.deepEquals(weakReference?.get())) {
+                        unchanged.add(index)
+                    }
+                }
+                for (index in itemStorage.items.size until component.countInternal) {
+                    val item: Any? = component.getItem(index)
+                    updatedItems[index] = item
+                }
+            }
+            else -> {
+                // Well, the count shrunk, let's make sure that the items in the component are different
+                // than the one stored. If not, it would be better to call OnItemRangeChange instead.
+                for (index in 0 until component.countInternal) {
+                    val weakReference: WeakReference<*>? = itemStorage[index]
+                    val item: Any? = component.getItem(index)
+                    updatedItems[index] = item
+                    if (item == null) {
+                        undecided.add(index)
+                    } else if (item.deepEquals(weakReference?.get())) {
+                        unchanged.add(index)
+                    }
+                }
+            }
+        }
+
+        componentsToItem[component] = updatedItems
+
+        return if (unchanged.size > 0) {
+            NotifyCheckResult.IncorrectOnChange(unchanged)
+        } else {
+            NotifyCheckResult.CorrectChange
+        }
+    }
+
+    private fun verifyItemRangeChanged(component: Component, positionStart: Int, itemCount: Int): NotifyCheckResult {
+        val itemStorage = componentsToItem[component]
+        if (itemStorage == null) {
+            return NotifyCheckResult.NotEnoughData
+        } else if (!itemStorage.isFullyPrefetched) {
+            return NotifyCheckResult.NotEnoughData
+        }
+
+        val unchanged = mutableListOf<Int>()
+        val undecided = mutableListOf<Int>()
+        val updatedItems: Array<WeakReference<*>?> = arrayOf(*itemStorage.items)
+
+        for (index in positionStart until positionStart + itemCount) {
+            val weakReference: WeakReference<*>? = itemStorage[index]
+            val item: Any? = component.getItem(index)
+            updatedItems[index] = WeakReference(item)
+
+            if (item == null) {
+                undecided.add(index)
+            } else if (item.deepEquals(weakReference?.get())) {
+                unchanged.add(index)
+            }
+        }
+        itemStorage.items = updatedItems
+        return if (unchanged.size > 0) {
+            NotifyCheckResult.IncorrectOnItemRangeChange(unchanged)
+        } else {
+            NotifyCheckResult.CorrectChange
+        }
+    }
+
+    class ItemStorage(capacity: Int) {
+        var items: Array<WeakReference<*>?> = Array(capacity) { null }
+
+        /**
+         * Are all the item pre-fetched? Check that the items array is full of weak references.
+         */
+        val isFullyPrefetched: Boolean get() = items.none { it == null }
+
+        operator fun set(index: Int, item: Any?) {
+            items[index] = WeakReference(item)
+        }
+
+        operator fun get(index: Int) = items[index]
+
+        fun onItemRangeInserted(positionStart: Int, itemCount: Int) {
+            val newItems: Array<WeakReference<*>?> = Array(items.size + itemCount) { null }
+            items.copyInto(newItems, 0, 0, positionStart)
+            items.copyInto(newItems, positionStart + itemCount, positionStart, items.size)
+
+            items = newItems
+        }
+
+        fun onItemRangeRemoved(positionStart: Int, itemCount: Int) {
+            val newItems: Array<WeakReference<*>?> = Array(items.size - itemCount) { null }
+            items.copyInto(newItems, 0, 0, positionStart)
+            items.copyInto(newItems, positionStart, positionStart + itemCount, items.size)
+
+            items = newItems
+        }
+
+        fun onItemMoved(fromPosition: Int, toPosition: Int) {
+            val itemsAsList = items.toMutableList()
+            val item: WeakReference<*>? = itemsAsList.removeAt(fromPosition)
+            itemsAsList.add(toPosition, item)
+
+            items = itemsAsList.toTypedArray()
+        }
+    }
+}
+
+sealed class NotifyCheckResult {
+    object NotEnoughData : NotifyCheckResult() {
+        override fun toString(): String {
+            return "NotifyCheckResult: Not enough data to establish validity of data change"
+        }
+    }
+
+    class IncorrectOnChange(private val unchanged: List<Int>) : NotifyCheckResult() {
+        override fun toString(): String {
+            return "NotifyCheckResult: You should not call onChange, indexes $unchanged stayed the same."
+        }
+    }
+
+    class IncorrectOnItemRangeChange(private val unchanged: List<Int>) : NotifyCheckResult() {
+        override fun toString(): String {
+            return "NotifyCheckResult: You should not call onItemRangeChange, indexes $unchanged stayed the same."
+        }
+    }
+
+    object CorrectChange : NotifyCheckResult() {
+        override fun toString(): String {
+            return "NotifyCheckResult: The onChange call was justified"
+        }
+    }
+}

bento/src/main/java/com/yelp/android/bento/utils/EqualsHelper.kt

@@ -0,0 +1,16 @@
+package com.yelp.android.bento.utils
+
+import org.apache.commons.lang3.builder.EqualsBuilder
+
+object EqualsHelper {
+    // Works well enough, but not for collections.
+    fun Any?.deepEquals(other: Any?): Boolean {
+        return when {
+            this == null && other == null -> true
+            this == null || other == null -> false
+            this::class != other::class -> false
+            this::class.isData -> this == other
+            else -> EqualsBuilder.reflectionEquals(this, other)
+        }
+    }
+}

buildSrc/src/main/java/com/yelp/gradle/bento/GlobalDependencies.kt

@@ -5,7 +5,7 @@ import org.gradle.api.artifacts.repositories.MavenArtifactRepository
 
 object Publishing {
     const val GROUP = "com.yelp.android"
-    const val VERSION = "18.0.2"
+    const val VERSION = "18.0.2-bvermont-SNAPSHOT"
 }
 
 object Versions {