From 2c16a785bde2545cdbb47e289dd7f214124a3c7d Mon Sep 17 00:00:00 2001
From: t-p-white <towhite@mozilla.com>
Date: Fri, 25 Aug 2023 11:51:41 +0100
Subject: [PATCH] Bug 1849966 - Compose Top Sites has unwanted recompositions
 on scrolling

---
 .../sessioncontrol/SessionControlAdapter.kt   |  6 +---
 .../mozilla/fenix/home/topsites/TopSites.kt   |  7 +++-
 .../fenix/home/topsites/TopSitesViewHolder.kt | 35 ++++++++++---------
 3 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt
index cc7add9830..258ded268a 100644
--- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt
+++ b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt
@@ -324,6 +324,7 @@ class SessionControlAdapter(
             is PocketCategoriesViewHolder,
             is PocketRecommendationsHeaderViewHolder,
             is PocketStoriesViewHolder,
+            is TopSitesViewHolder,
             -> {
                 // no op
                 // This previously called "composeView.disposeComposition" which would have the
@@ -331,11 +332,6 @@ class SessionControlAdapter(
                 // This View already listens and maps store updates. Avoid creating and binding new Views.
                 // The composition will live until the ViewTreeLifecycleOwner to which it's attached to is destroyed.
             }
-            is TopSitesViewHolder -> {
-                // Dispose the underlying composition immediately.
-                // This ViewHolder can be removed / re-added and we need it to show a fresh new composition.
-                holder.composeView.disposeComposition()
-            }
             is CollectionViewHolder -> {
                 // Dispose the underlying composition immediately.
                 // This ViewHolder can be removed / re-added and we need it to show a fresh new composition.
diff --git a/app/src/main/java/org/mozilla/fenix/home/topsites/TopSites.kt b/app/src/main/java/org/mozilla/fenix/home/topsites/TopSites.kt
index 197df834a6..318dc1298f 100644
--- a/app/src/main/java/org/mozilla/fenix/home/topsites/TopSites.kt
+++ b/app/src/main/java/org/mozilla/fenix/home/topsites/TopSites.kt
@@ -384,8 +384,13 @@ private fun FaviconBitmap(topSite: TopSite.Provided) {
     }
 
     when (val uiState = faviconBitmapUiState) {
-        is FaviconBitmapUiState.Loading, FaviconBitmapUiState.Error -> FaviconDefault(topSite.url)
         is FaviconBitmapUiState.Data -> FaviconImage(BitmapPainter(uiState.imageBitmap))
+        is FaviconBitmapUiState.Error -> FaviconDefault(topSite.url)
+        is FaviconBitmapUiState.Loading -> {
+            // no-op
+            // Don't update the icon while loading else the top site icon could have a 'flashing' effect
+            // caused by the 'place holder letter' icon being immediately updated with the desired bitmap.
+        }
     }
 }
 
diff --git a/app/src/main/java/org/mozilla/fenix/home/topsites/TopSitesViewHolder.kt b/app/src/main/java/org/mozilla/fenix/home/topsites/TopSitesViewHolder.kt
index ee8322643b..74e67ef26b 100644
--- a/app/src/main/java/org/mozilla/fenix/home/topsites/TopSitesViewHolder.kt
+++ b/app/src/main/java/org/mozilla/fenix/home/topsites/TopSitesViewHolder.kt
@@ -8,7 +8,7 @@ import android.view.View
 import androidx.compose.runtime.Composable
 import androidx.compose.ui.platform.ComposeView
 import androidx.lifecycle.LifecycleOwner
-import mozilla.components.lib.state.ext.observeAsState
+import mozilla.components.lib.state.ext.observeAsComposableState
 import org.mozilla.fenix.components.components
 import org.mozilla.fenix.compose.ComposeViewHolder
 import org.mozilla.fenix.home.sessioncontrol.TopSiteInteractor
@@ -31,23 +31,26 @@ class TopSitesViewHolder(
     @Composable
     override fun Content() {
         val topSites =
-            components.appStore.observeAsState(emptyList()) { state -> state.topSites }.value
+            components.appStore.observeAsComposableState { state -> state.topSites }.value
         val wallpaperState = components.appStore
-            .observeAsState(WallpaperState.default) { state -> state.wallpaperState }.value
+            .observeAsComposableState { state -> state.wallpaperState }.value
+            ?: WallpaperState.default
 
-        TopSites(
-            topSites = topSites,
-            topSiteColors = TopSiteColors.colors(wallpaperState = wallpaperState),
-            onTopSiteClick = { topSite ->
-                interactor.onSelectTopSite(topSite, topSites.indexOf(topSite))
-            },
-            onTopSiteLongClick = interactor::onTopSiteLongClicked,
-            onOpenInPrivateTabClicked = interactor::onOpenInPrivateTabClicked,
-            onRenameTopSiteClicked = interactor::onRenameTopSiteClicked,
-            onRemoveTopSiteClicked = interactor::onRemoveTopSiteClicked,
-            onSettingsClicked = interactor::onSettingsClicked,
-            onSponsorPrivacyClicked = interactor::onSponsorPrivacyClicked,
-        )
+        topSites?.let {
+            TopSites(
+                topSites = it,
+                topSiteColors = TopSiteColors.colors(wallpaperState = wallpaperState),
+                onTopSiteClick = { topSite ->
+                    interactor.onSelectTopSite(topSite, it.indexOf(topSite))
+                },
+                onTopSiteLongClick = interactor::onTopSiteLongClicked,
+                onOpenInPrivateTabClicked = interactor::onOpenInPrivateTabClicked,
+                onRenameTopSiteClicked = interactor::onRenameTopSiteClicked,
+                onRemoveTopSiteClicked = interactor::onRemoveTopSiteClicked,
+                onSettingsClicked = interactor::onSettingsClicked,
+                onSponsorPrivacyClicked = interactor::onSponsorPrivacyClicked,
+            )
+        }
     }
 
     companion object {