From f87d026616651dfd161366d8fb65164f8683b679 Mon Sep 17 00:00:00 2001 From: Benoit Vermont Date: Tue, 13 Jul 2021 18:14:49 +0200 Subject: [PATCH 01/10] Put together an checker to check validity of onChange calls --- .../android/bento/core/ComponentGroup.java | 12 +- .../yelp/android/bento/core/NotifyChecker.kt | 220 ++++++++++++++++++ .../yelp/gradle/bento/GlobalDependencies.kt | 2 +- 3 files changed, 232 insertions(+), 2 deletions(-) create mode 100644 bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt diff --git a/bento/src/main/java/com/yelp/android/bento/core/ComponentGroup.java b/bento/src/main/java/com/yelp/android/bento/core/ComponentGroup.java index d7b248a..764d4b6 100644 --- a/bento/src/main/java/com/yelp/android/bento/core/ComponentGroup.java +++ b/bento/src/main/java/com/yelp/android/bento/core/ComponentGroup.java @@ -35,6 +35,7 @@ public class ComponentGroup extends Component { private final Map mComponentDataObserverMap = new HashMap<>(); private final ComponentGroupObservable mObservable = new ComponentGroupObservable(); + private final NotifyChecker mNotifyChecker = new NotifyChecker(); public ComponentGroup() { mSpanSizeLookup = @@ -346,7 +347,10 @@ public void unregisterComponentGroupObserver(@NonNull ComponentGroupDataObserver public Object getItem(int position) { RangedValue 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 +595,7 @@ private void cleanupComponent(@NonNull Component component) { entry.setValue(entry.getValue() - 1); } } + mNotifyChecker.remove(component); mObservable.notifyOnComponentRemoved(component); } @@ -610,6 +615,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 +627,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 +637,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 +651,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 +665,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; diff --git a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt new file mode 100644 index 0000000..55fb946 --- /dev/null +++ b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt @@ -0,0 +1,220 @@ +package com.yelp.android.bento.core + +import android.util.Log +import java.lang.ref.WeakReference + +class NotifyChecker { + private val componentsToItem: MutableMap = mutableMapOf() + + fun save(component: Component, position: Int, item: Any?) { + val items = componentsToItem.getOrPut(component) { + ItemStorage(component.countInternal) + } + + items[position] = WeakReference(component) + } + + fun remove(component: Component) { + componentsToItem.remove(component) + } + + // 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.isFull) { + return NotifyCheckResult.NotEnoughData + } + + val updatedItems = ItemStorage(component.count) + + val unchanged = mutableListOf() + val undecided = mutableListOf() + + when { + itemStorage.items.size == component.count -> { + // 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 == weakReference?.get()) { + unchanged.add(index) + } + } + componentsToItem[component] = updatedItems + return if (unchanged.size > 0) { + NotifyCheckResult.IncorrectChange(unchanged) + } else { + NotifyCheckResult.CorrectChange + } + } + itemStorage.items.size < component.count -> { + // 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 == weakReference?.get()) { + unchanged.add(index) + } + } + for (index in itemStorage.items.size until component.count) { + val item: Any? = component.getItem(index) + updatedItems[index] = item + } + + componentsToItem[component] = updatedItems + + return if (unchanged.size > 0) { + NotifyCheckResult.IncorrectChange(unchanged) + } else { + NotifyCheckResult.CorrectChange + } + } + 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.count) { + val weakReference: WeakReference<*>? = itemStorage[index] + val item: Any? = component.getItem(index) + updatedItems[index] = item + if (item == null) { + undecided.add(index) + } else if (item == weakReference?.get()) { + unchanged.add(index) + } + } + + componentsToItem[component] = updatedItems + + return if (unchanged.size > 0) { + NotifyCheckResult.IncorrectChange(unchanged) + } else { + NotifyCheckResult.CorrectChange + } + } + } + } + + fun onChanged(component: Component) { + val verifyChange = verifyOnChanged(component) + Log.d("ComponentGroup", "onChanged for $component: $verifyChange") + } + + fun onItemRangeChanged(component: Component, positionStart: Int, itemCount: Int) { + val verifyChange = verifyItemRangeChanged(component, positionStart, itemCount) + Log.d("ComponentGroup", "onItemRangeChanged for $component: $verifyChange") + } + + fun verifyItemRangeChanged(component: Component, positionStart: Int, itemCount: Int): NotifyCheckResult { + val itemStorage = componentsToItem[component] + if (itemStorage == null) { + return NotifyCheckResult.NotEnoughData + } else if (!itemStorage.isFull) { + return NotifyCheckResult.NotEnoughData + } + + val unchanged = mutableListOf() + val undecided = mutableListOf() + val updatedItems: Array?> = 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 == weakReference?.get()) { + unchanged.add(index) + } + } + itemStorage.items = updatedItems + return if (unchanged.size > 0) { + NotifyCheckResult.IncorrectChange(unchanged) + } else { + NotifyCheckResult.CorrectChange + } + } + + 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) + } + + class ItemStorage(capacity: Int) { + var items: Array?> = Array(capacity) { null } + + val isFull: 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?> = 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?> = 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 IncorrectChange(val unchanged: List) : NotifyCheckResult() { + override fun toString(): String { + return "NotifyCheckResult: You should not call onChange, indexes $unchanged stayed the same." + } + } + + object CorrectChange : NotifyCheckResult() { + override fun toString(): String { + return "NotifyCheckResult: The onChange call was justified" + } + } +} diff --git a/buildSrc/src/main/java/com/yelp/gradle/bento/GlobalDependencies.kt b/buildSrc/src/main/java/com/yelp/gradle/bento/GlobalDependencies.kt index eb0f6ea..b4ab4e7 100644 --- a/buildSrc/src/main/java/com/yelp/gradle/bento/GlobalDependencies.kt +++ b/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 { From 80163f51c88595404928c90897d88725b8fa725e Mon Sep 17 00:00:00 2001 From: Benoit Vermont Date: Wed, 14 Jul 2021 11:22:20 +0200 Subject: [PATCH 02/10] Minor change --- .../com/yelp/android/bento/core/NotifyChecker.kt | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt index 55fb946..4efa7d0 100644 --- a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt +++ b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt @@ -23,13 +23,15 @@ class NotifyChecker { val itemStorage = componentsToItem[component] if (itemStorage == null) { return NotifyCheckResult.NotEnoughData - } else if (!itemStorage.isFull) { + } else if (!itemStorage.isFullyPrefetched) { return NotifyCheckResult.NotEnoughData } val updatedItems = ItemStorage(component.count) val unchanged = mutableListOf() + // 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() when { @@ -113,11 +115,11 @@ class NotifyChecker { Log.d("ComponentGroup", "onItemRangeChanged for $component: $verifyChange") } - fun verifyItemRangeChanged(component: Component, positionStart: Int, itemCount: Int): NotifyCheckResult { + private fun verifyItemRangeChanged(component: Component, positionStart: Int, itemCount: Int): NotifyCheckResult { val itemStorage = componentsToItem[component] if (itemStorage == null) { return NotifyCheckResult.NotEnoughData - } else if (!itemStorage.isFull) { + } else if (!itemStorage.isFullyPrefetched) { return NotifyCheckResult.NotEnoughData } @@ -165,7 +167,10 @@ class NotifyChecker { class ItemStorage(capacity: Int) { var items: Array?> = Array(capacity) { null } - val isFull: Boolean get() = items.none { it == 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) From ca799a6bf457299a0452e3f5716abbd6a1f0a4d6 Mon Sep 17 00:00:00 2001 From: Benoit Vermont Date: Wed, 14 Jul 2021 11:31:34 +0200 Subject: [PATCH 03/10] Did that ever worked? --- .../src/main/java/com/yelp/android/bento/core/NotifyChecker.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt index 4efa7d0..305db70 100644 --- a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt +++ b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt @@ -11,7 +11,7 @@ class NotifyChecker { ItemStorage(component.countInternal) } - items[position] = WeakReference(component) + items[position] = WeakReference(item) } fun remove(component: Component) { From 7a24a0ecc33b2ef5a5c4fbd604e0ae4290de467a Mon Sep 17 00:00:00 2001 From: Benoit Vermont Date: Wed, 14 Jul 2021 11:43:20 +0200 Subject: [PATCH 04/10] Prefetch item, so that the checker is more useful --- .../android/bento/core/ComponentGroup.java | 1 + .../yelp/android/bento/core/NotifyChecker.kt | 66 +++++++++++-------- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/bento/src/main/java/com/yelp/android/bento/core/ComponentGroup.java b/bento/src/main/java/com/yelp/android/bento/core/ComponentGroup.java index 764d4b6..0674938 100644 --- a/bento/src/main/java/com/yelp/android/bento/core/ComponentGroup.java +++ b/bento/src/main/java/com/yelp/android/bento/core/ComponentGroup.java @@ -167,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(); diff --git a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt index 305db70..2e67e03 100644 --- a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt +++ b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt @@ -6,6 +6,16 @@ import java.lang.ref.WeakReference class NotifyChecker { private val componentsToItem: MutableMap = mutableMapOf() + fun prefetch(component: Component) { + val items = componentsToItem.getOrPut(component) { + ItemStorage(component.countInternal) + } + + for (position in 0 until component.countInternal) { + items[position] = component.getItemInternal(position) + } + } + fun save(component: Component, position: Int, item: Any?) { val items = componentsToItem.getOrPut(component) { ItemStorage(component.countInternal) @@ -18,6 +28,34 @@ class NotifyChecker { componentsToItem.remove(component) } + fun onChanged(component: Component) { + val verifyChange = verifyOnChanged(component) + Log.d("ComponentGroup", "onChanged for $component: $verifyChange") + } + + fun onItemRangeChanged(component: Component, positionStart: Int, itemCount: Int) { + val verifyChange = verifyItemRangeChanged(component, positionStart, itemCount) + Log.d("ComponentGroup", "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] @@ -105,16 +143,6 @@ class NotifyChecker { } } - fun onChanged(component: Component) { - val verifyChange = verifyOnChanged(component) - Log.d("ComponentGroup", "onChanged for $component: $verifyChange") - } - - fun onItemRangeChanged(component: Component, positionStart: Int, itemCount: Int) { - val verifyChange = verifyItemRangeChanged(component, positionStart, itemCount) - Log.d("ComponentGroup", "onItemRangeChanged for $component: $verifyChange") - } - private fun verifyItemRangeChanged(component: Component, positionStart: Int, itemCount: Int): NotifyCheckResult { val itemStorage = componentsToItem[component] if (itemStorage == null) { @@ -146,24 +174,6 @@ class NotifyChecker { } } - 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) - } - class ItemStorage(capacity: Int) { var items: Array?> = Array(capacity) { null } From 036aee1a65b5080503d1189ab69987dc020053fe Mon Sep 17 00:00:00 2001 From: Benoit Vermont Date: Wed, 14 Jul 2021 12:06:54 +0200 Subject: [PATCH 05/10] Try catch prefetch, because of some issues on the bizpage --- .../java/com/yelp/android/bento/core/NotifyChecker.kt | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt index 2e67e03..7476911 100644 --- a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt +++ b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt @@ -11,8 +11,13 @@ class NotifyChecker { ItemStorage(component.countInternal) } - for (position in 0 until component.countInternal) { - items[position] = component.getItemInternal(position) + val result = runCatching { + for (position in 0 until component.countInternal) { + items[position] = component.getItemInternal(position) + } + } + if (result.isFailure) { + Log.d("ComponentGroup", "Could not prefetch $component", result.exceptionOrNull()) } } From b4a69baf8c3d293966930d73f3ffdb9cdef450fa Mon Sep 17 00:00:00 2001 From: Benoit Vermont Date: Wed, 14 Jul 2021 12:18:29 +0200 Subject: [PATCH 06/10] Add granularity for the results --- .../com/yelp/android/bento/core/NotifyChecker.kt | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt index 7476911..a3131da 100644 --- a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt +++ b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt @@ -92,7 +92,7 @@ class NotifyChecker { } componentsToItem[component] = updatedItems return if (unchanged.size > 0) { - NotifyCheckResult.IncorrectChange(unchanged) + NotifyCheckResult.IncorrectOnChange(unchanged) } else { NotifyCheckResult.CorrectChange } @@ -118,7 +118,7 @@ class NotifyChecker { componentsToItem[component] = updatedItems return if (unchanged.size > 0) { - NotifyCheckResult.IncorrectChange(unchanged) + NotifyCheckResult.IncorrectOnChange(unchanged) } else { NotifyCheckResult.CorrectChange } @@ -140,7 +140,7 @@ class NotifyChecker { componentsToItem[component] = updatedItems return if (unchanged.size > 0) { - NotifyCheckResult.IncorrectChange(unchanged) + NotifyCheckResult.IncorrectOnChange(unchanged) } else { NotifyCheckResult.CorrectChange } @@ -173,7 +173,7 @@ class NotifyChecker { } itemStorage.items = updatedItems return if (unchanged.size > 0) { - NotifyCheckResult.IncorrectChange(unchanged) + NotifyCheckResult.IncorrectOnItemRangeChange(unchanged) } else { NotifyCheckResult.CorrectChange } @@ -226,12 +226,18 @@ sealed class NotifyCheckResult { } } - class IncorrectChange(val unchanged: List) : NotifyCheckResult() { + class IncorrectOnChange(private val unchanged: List) : NotifyCheckResult() { override fun toString(): String { return "NotifyCheckResult: You should not call onChange, indexes $unchanged stayed the same." } } + class IncorrectOnItemRangeChange(private val unchanged: List) : 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" From e9494f9bcaf3a2cbf3c1d25c4ee4ff049f4adc80 Mon Sep 17 00:00:00 2001 From: Benoit Vermont Date: Wed, 14 Jul 2021 14:42:26 +0200 Subject: [PATCH 07/10] Implement a deep equals method with reflection --- bento/build.gradle | 1 + .../yelp/android/bento/core/NotifyChecker.kt | 21 +++++------ .../yelp/android/bento/utils/EqualsHelper.kt | 36 +++++++++++++++++++ 3 files changed, 48 insertions(+), 10 deletions(-) create mode 100644 bento/src/main/java/com/yelp/android/bento/utils/EqualsHelper.kt diff --git a/bento/build.gradle b/bento/build.gradle index 65e9de8..244e81b 100644 --- a/bento/build.gradle +++ b/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 diff --git a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt index a3131da..6ebbc76 100644 --- a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt +++ b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt @@ -1,6 +1,7 @@ package com.yelp.android.bento.core import android.util.Log +import com.yelp.android.bento.utils.EqualsHelper.deepEquals import java.lang.ref.WeakReference class NotifyChecker { @@ -26,7 +27,7 @@ class NotifyChecker { ItemStorage(component.countInternal) } - items[position] = WeakReference(item) + items[position] = item } fun remove(component: Component) { @@ -70,7 +71,7 @@ class NotifyChecker { return NotifyCheckResult.NotEnoughData } - val updatedItems = ItemStorage(component.count) + val updatedItems = ItemStorage(component.countInternal) val unchanged = mutableListOf() // We can also keep track of the "null"? A bunch of components use null as items, @@ -78,7 +79,7 @@ class NotifyChecker { val undecided = mutableListOf() when { - itemStorage.items.size == component.count -> { + 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) @@ -86,7 +87,7 @@ class NotifyChecker { if (item == null) { undecided.add(index) - } else if (item == weakReference?.get()) { + } else if (item.deepEquals(weakReference?.get())) { unchanged.add(index) } } @@ -97,7 +98,7 @@ class NotifyChecker { NotifyCheckResult.CorrectChange } } - itemStorage.items.size < component.count -> { + 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 -> @@ -106,11 +107,11 @@ class NotifyChecker { if (item == null) { undecided.add(index) - } else if (item == weakReference?.get()) { + } else if (item.deepEquals(weakReference?.get())) { unchanged.add(index) } } - for (index in itemStorage.items.size until component.count) { + for (index in itemStorage.items.size until component.countInternal) { val item: Any? = component.getItem(index) updatedItems[index] = item } @@ -126,13 +127,13 @@ class NotifyChecker { 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.count) { + 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 == weakReference?.get()) { + } else if (item.deepEquals(weakReference?.get())) { unchanged.add(index) } } @@ -167,7 +168,7 @@ class NotifyChecker { if (item == null) { undecided.add(index) - } else if (item == weakReference?.get()) { + } else if (item.deepEquals(weakReference?.get())) { unchanged.add(index) } } diff --git a/bento/src/main/java/com/yelp/android/bento/utils/EqualsHelper.kt b/bento/src/main/java/com/yelp/android/bento/utils/EqualsHelper.kt new file mode 100644 index 0000000..2f958c1 --- /dev/null +++ b/bento/src/main/java/com/yelp/android/bento/utils/EqualsHelper.kt @@ -0,0 +1,36 @@ +package com.yelp.android.bento.utils + +import kotlin.reflect.KClass +import kotlin.reflect.KProperty1 +import kotlin.reflect.full.memberProperties +import kotlin.reflect.jvm.isAccessible + +object EqualsHelper { + fun Any?.deepEquals(other: Any?): Boolean { + if (this == null && other == null) { + return true + } + if (this == null || other == null) { + return false + } + + if (this::class != other::class) return false + + val kClass: KClass = this::class + if (kClass.java.isPrimitive || kClass.isData) { + return this == other + } + + return kClass.memberProperties.all { member -> + @Suppress("UNCHECKED_CAST") + val castedMember = member as KProperty1 + val wasAccessible = castedMember.isAccessible + castedMember.isAccessible = true + val memberA = castedMember.get(this) + val memberB = castedMember.get(other) + castedMember.isAccessible = wasAccessible + + memberA.deepEquals(memberB) + } + } +} From 240c45121260450484aac7bd20b17352576d611c Mon Sep 17 00:00:00 2001 From: Benoit Vermont Date: Wed, 14 Jul 2021 14:45:38 +0200 Subject: [PATCH 08/10] Actually use a proper TAG --- .../java/com/yelp/android/bento/core/NotifyChecker.kt | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt index 6ebbc76..60c146c 100644 --- a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt +++ b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt @@ -4,6 +4,8 @@ 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 = mutableMapOf() @@ -18,7 +20,7 @@ class NotifyChecker { } } if (result.isFailure) { - Log.d("ComponentGroup", "Could not prefetch $component", result.exceptionOrNull()) + Log.d(TAG, "Could not prefetch $component", result.exceptionOrNull()) } } @@ -36,12 +38,12 @@ class NotifyChecker { fun onChanged(component: Component) { val verifyChange = verifyOnChanged(component) - Log.d("ComponentGroup", "onChanged for $component: $verifyChange") + Log.d(TAG, "onChanged for $component: $verifyChange") } fun onItemRangeChanged(component: Component, positionStart: Int, itemCount: Int) { val verifyChange = verifyItemRangeChanged(component, positionStart, itemCount) - Log.d("ComponentGroup", "onItemRangeChanged for $component: $verifyChange") + Log.d(TAG, "onItemRangeChanged for $component: $verifyChange") } fun onItemRangeInserted(component: Component, positionStart: Int, itemCount: Int) { From 974113f58f64e9e0c8844157628717cc6537d6a0 Mon Sep 17 00:00:00 2001 From: Benoit Vermont Date: Wed, 14 Jul 2021 15:44:53 +0200 Subject: [PATCH 09/10] Let's use the existing guava method --- .../yelp/android/bento/utils/EqualsHelper.kt | 36 +++++-------------- 1 file changed, 8 insertions(+), 28 deletions(-) diff --git a/bento/src/main/java/com/yelp/android/bento/utils/EqualsHelper.kt b/bento/src/main/java/com/yelp/android/bento/utils/EqualsHelper.kt index 2f958c1..2b63fdf 100644 --- a/bento/src/main/java/com/yelp/android/bento/utils/EqualsHelper.kt +++ b/bento/src/main/java/com/yelp/android/bento/utils/EqualsHelper.kt @@ -1,36 +1,16 @@ package com.yelp.android.bento.utils -import kotlin.reflect.KClass -import kotlin.reflect.KProperty1 -import kotlin.reflect.full.memberProperties -import kotlin.reflect.jvm.isAccessible +import org.apache.commons.lang3.builder.EqualsBuilder object EqualsHelper { + // Works well enough, but not for collections. fun Any?.deepEquals(other: Any?): Boolean { - if (this == null && other == null) { - return true - } - if (this == null || other == null) { - return false - } - - if (this::class != other::class) return false - - val kClass: KClass = this::class - if (kClass.java.isPrimitive || kClass.isData) { - return this == other - } - - return kClass.memberProperties.all { member -> - @Suppress("UNCHECKED_CAST") - val castedMember = member as KProperty1 - val wasAccessible = castedMember.isAccessible - castedMember.isAccessible = true - val memberA = castedMember.get(this) - val memberB = castedMember.get(other) - castedMember.isAccessible = wasAccessible - - memberA.deepEquals(memberB) + 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) } } } From 98eca64b3f63834e3876ecd8dfa264ecb1d8f349 Mon Sep 17 00:00:00 2001 From: Benoit Vermont Date: Wed, 14 Jul 2021 16:21:01 +0200 Subject: [PATCH 10/10] Cleanup, less verbose --- .../yelp/android/bento/core/NotifyChecker.kt | 38 +++++++------------ 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt index 60c146c..a82bb4e 100644 --- a/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt +++ b/bento/src/main/java/com/yelp/android/bento/core/NotifyChecker.kt @@ -20,7 +20,7 @@ class NotifyChecker { } } if (result.isFailure) { - Log.d(TAG, "Could not prefetch $component", result.exceptionOrNull()) + Log.v(TAG, "Could not prefetch $component", result.exceptionOrNull()) } } @@ -38,12 +38,16 @@ class NotifyChecker { fun onChanged(component: Component) { val verifyChange = verifyOnChanged(component) - Log.d(TAG, "onChanged for $component: $verifyChange") + 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) - Log.d(TAG, "onItemRangeChanged for $component: $verifyChange") + if (verifyChange is NotifyCheckResult.IncorrectOnItemRangeChange) { + Log.d(TAG, "onItemRangeChanged for $component: $verifyChange") + } } fun onItemRangeInserted(component: Component, positionStart: Int, itemCount: Int) { @@ -93,12 +97,6 @@ class NotifyChecker { unchanged.add(index) } } - componentsToItem[component] = updatedItems - return if (unchanged.size > 0) { - NotifyCheckResult.IncorrectOnChange(unchanged) - } else { - NotifyCheckResult.CorrectChange - } } itemStorage.items.size < component.countInternal -> { // Let's compare the stored items, to see if it's okay to call onChanged, @@ -117,14 +115,6 @@ class NotifyChecker { val item: Any? = component.getItem(index) updatedItems[index] = item } - - componentsToItem[component] = updatedItems - - return if (unchanged.size > 0) { - NotifyCheckResult.IncorrectOnChange(unchanged) - } else { - NotifyCheckResult.CorrectChange - } } else -> { // Well, the count shrunk, let's make sure that the items in the component are different @@ -139,15 +129,15 @@ class NotifyChecker { unchanged.add(index) } } + } + } - componentsToItem[component] = updatedItems + componentsToItem[component] = updatedItems - return if (unchanged.size > 0) { - NotifyCheckResult.IncorrectOnChange(unchanged) - } else { - NotifyCheckResult.CorrectChange - } - } + return if (unchanged.size > 0) { + NotifyCheckResult.IncorrectOnChange(unchanged) + } else { + NotifyCheckResult.CorrectChange } }