[fenix] For https://github.com/mozilla-mobile/fenix/issues/24744 - Observe and update selector icon while the View is attached

This avoids having to pass another LifecycleOwner from outside and will ensure
the View is update only in the bounds of it being attached.

The observe-update code is moved to bind(..) as that seems like a more
idiomatic callback for updating an already constructed View rather than
createView() which should only create and return a View.

app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt

@@ -713,7 +713,6 @@ class SearchDialogFragment : AppCompatDialogFragment(), UserInteractionHandler {
             SearchSelectorToolbarAction(
                 store = store,
                 menu = searchSelectorMenu,
-                viewLifecycleOwner = viewLifecycleOwner
             )
         )
 

app/src/main/java/org/mozilla/fenix/search/toolbar/SearchSelectorToolbarAction.kt

@@ -9,21 +9,23 @@ import android.graphics.Bitmap
 import android.graphics.drawable.BitmapDrawable
 import android.view.View
 import android.view.ViewGroup
-import androidx.lifecycle.LifecycleOwner
-import kotlinx.coroutines.flow.collect
+import androidx.annotation.VisibleForTesting
+import kotlinx.coroutines.Job
+import kotlinx.coroutines.flow.filterNotNull
 import kotlinx.coroutines.flow.map
+import kotlinx.coroutines.launch
 import mozilla.components.browser.state.search.SearchEngine
 import mozilla.components.concept.menu.Orientation
 import mozilla.components.concept.toolbar.Toolbar
-import mozilla.components.lib.state.ext.flowScoped
+import mozilla.components.lib.state.ext.flow
 import mozilla.components.support.ktx.android.content.res.resolveAttribute
+import mozilla.components.support.ktx.android.view.toScope
 import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifChanged
 import mozilla.telemetry.glean.private.NoExtras
 import org.mozilla.fenix.GleanMetrics.UnifiedSearch
 import org.mozilla.fenix.R
 import org.mozilla.fenix.ext.settings
 import org.mozilla.fenix.search.SearchDialogFragmentStore
-import java.lang.ref.WeakReference
 
 /**
  * A [Toolbar.Action] implementation that shows a [SearchSelector].
@@ -31,32 +33,17 @@ import java.lang.ref.WeakReference
  * @property store [SearchDialogFragmentStore] containing the complete state of the search dialog.
  * @property menu An instance of [SearchSelectorMenu] to display a popup menu for the search
  * selections.
- * @property viewLifecycleOwner [LifecycleOwner] life cycle owner for the view.
  */
 class SearchSelectorToolbarAction(
     private val store: SearchDialogFragmentStore,
     private val menu: SearchSelectorMenu,
-    private val viewLifecycleOwner: LifecycleOwner
 ) : Toolbar.Action {
-
-    private var reference = WeakReference<SearchSelector>(null)
+    private var updateIconJob: Job? = null
 
     override fun createView(parent: ViewGroup): View {
         val context = parent.context
 
-        store.flowScoped(viewLifecycleOwner) { flow ->
-            flow.map { state -> state.searchEngineSource.searchEngine }
-                .ifChanged()
-                .collect { searchEngine ->
-                    searchEngine?.let {
-                        updateIcon(context, it)
-                    }
-                }
-        }
-
         return SearchSelector(context).apply {
-            reference = WeakReference(this)
-
             setOnClickListener {
                 val orientation = if (context.settings().shouldUseBottomToolbar) {
                     Orientation.UP
@@ -74,19 +61,41 @@ class SearchSelectorToolbarAction(
         }
     }
 
-    override fun bind(view: View) = Unit
+    override fun bind(view: View) {
+        // It may happen that this View is binded multiple times.
+        // Prevent launching new coroutines for every time this is binded and only update the icon once.
+        if (updateIconJob?.isActive != true) {
+            updateIconJob = (view as? SearchSelector)?.toScope()?.launch {
+                store.flow()
+                    .map { state -> state.searchEngineSource.searchEngine }
+                    .filterNotNull()
+                    .ifChanged()
+                    .collect { searchEngine ->
+                        view.setIcon(
+                            icon = searchEngine.getScaledIcon(view.context),
+                            contentDescription = searchEngine.name
+                        )
+                    }
+            }.also {
+                it?.start()
+            }
+        }
+    }
+}
 
-    private fun updateIcon(context: Context, searchEngine: SearchEngine) {
-        val iconSize =
-            context.resources.getDimensionPixelSize(R.dimen.preference_icon_drawable_size)
-        val scaledIcon = Bitmap.createScaledBitmap(
-            searchEngine.icon,
-            iconSize,
-            iconSize,
-            true
-        )
-        val icon = BitmapDrawable(context.resources, scaledIcon)
+/**
+ * Get the search engine icon appropriately scaled to be shown in the selector.
+ */
+@VisibleForTesting
+internal fun SearchEngine.getScaledIcon(context: Context): BitmapDrawable {
+    val iconSize =
+        context.resources.getDimensionPixelSize(R.dimen.preference_icon_drawable_size)
+    val scaledIcon = Bitmap.createScaledBitmap(
+        icon,
+        iconSize,
+        iconSize,
+        true
+    )
 
-        reference.get()?.setIcon(icon, searchEngine.name)
-    }
+    return BitmapDrawable(context.resources, scaledIcon)
 }

app/src/test/java/org/mozilla/fenix/search/toolbar/SearchSelectorToolbarActionTest.kt

@@ -4,18 +4,25 @@
 
 package org.mozilla.fenix.search.toolbar
 
+import android.graphics.Bitmap
+import android.graphics.Bitmap.Config.ARGB_8888
+import android.graphics.Color
+import android.graphics.drawable.BitmapDrawable
 import android.view.ViewGroup
 import android.widget.LinearLayout
-import androidx.lifecycle.Lifecycle
-import androidx.lifecycle.LifecycleOwner
-import androidx.lifecycle.LifecycleRegistry
+import androidx.core.graphics.applyCanvas
 import io.mockk.MockKAnnotations
 import io.mockk.every
 import io.mockk.impl.annotations.MockK
+import io.mockk.mockk
+import io.mockk.mockkStatic
 import io.mockk.spyk
 import io.mockk.verify
+import mozilla.components.browser.state.search.SearchEngine
+import mozilla.components.browser.state.search.SearchEngine.Type.BUNDLED
 import mozilla.components.concept.menu.Orientation
 import mozilla.components.service.glean.testing.GleanTestRule
+import mozilla.components.support.test.libstate.ext.waitUntilIdle
 import mozilla.components.support.test.robolectric.testContext
 import mozilla.components.support.test.rule.MainCoroutineRule
 import org.junit.Assert.assertFalse
@@ -25,10 +32,17 @@ import org.junit.Rule
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.mozilla.fenix.GleanMetrics.UnifiedSearch
+import org.mozilla.fenix.R
+import org.mozilla.fenix.components.metrics.MetricsUtils
 import org.mozilla.fenix.ext.settings
 import org.mozilla.fenix.helpers.FenixRobolectricTestRunner
 import org.mozilla.fenix.search.SearchDialogFragmentStore
+import org.mozilla.fenix.search.SearchEngineSource
+import org.mozilla.fenix.search.SearchFragmentAction.SearchDefaultEngineSelected
+import org.mozilla.fenix.search.SearchFragmentAction.SearchHistoryEngineSelected
+import org.mozilla.fenix.search.SearchFragmentState
 import org.mozilla.fenix.utils.Settings
+import java.util.UUID
 
 @RunWith(FenixRobolectricTestRunner::class)
 class SearchSelectorToolbarActionTest {
@@ -42,27 +56,16 @@ class SearchSelectorToolbarActionTest {
     @MockK(relaxed = true)
     private lateinit var settings: Settings
 
-    private lateinit var lifecycleOwner: MockedLifecycleOwner
-
     @get:Rule
     val coroutinesTestRule = MainCoroutineRule()
 
     @get:Rule
     val gleanTestRule = GleanTestRule(testContext)
 
-    internal class MockedLifecycleOwner(initialState: Lifecycle.State) : LifecycleOwner {
-        val lifecycleRegistry = LifecycleRegistry(this).apply {
-            currentState = initialState
-        }
-
-        override fun getLifecycle(): Lifecycle = lifecycleRegistry
-    }
-
     @Before
     fun setup() {
         MockKAnnotations.init(this)
 
-        lifecycleOwner = MockedLifecycleOwner(Lifecycle.State.STARTED)
         every { testContext.settings() } returns settings
     }
 
@@ -72,7 +75,6 @@ class SearchSelectorToolbarActionTest {
             SearchSelectorToolbarAction(
                 store = store,
                 menu = menu,
-                viewLifecycleOwner = lifecycleOwner
             )
         )
         val view = action.createView(LinearLayout(testContext) as ViewGroup) as SearchSelector
@@ -94,4 +96,185 @@ class SearchSelectorToolbarActionTest {
             menu.menuController.show(view, Orientation.UP)
         }
     }
+
+    @Test
+    fun `GIVEN a binded search selector View WHEN a search engine is selected THEN update the icon`() {
+        mockkStatic("org.mozilla.fenix.search.toolbar.SearchSelectorToolbarActionKt") {
+            val searchEngineIcon: BitmapDrawable = mockk(relaxed = true)
+            every { any<SearchEngine>().getScaledIcon(any()) } returns searchEngineIcon
+            val store = SearchDialogFragmentStore(testSearchFragmentState)
+            val selector = SearchSelectorToolbarAction(store, mockk())
+            val view = spyk(SearchSelector(testContext))
+
+            selector.bind(view)
+            store.dispatch(
+                SearchDefaultEngineSelected(
+                    engine = testSearchEngine,
+                    settings = mockk(relaxed = true)
+                )
+            )
+            store.waitUntilIdle()
+
+            verify { testSearchEngine.getScaledIcon(any()) }
+            verify {
+                view.setIcon(
+                    icon = searchEngineIcon,
+                    contentDescription = testSearchEngine.name
+                )
+            }
+        }
+    }
+
+    @Test
+    fun `GIVEN the same view is binded multiple times WHEN the search engine changes THEN update the icon only once`() {
+        // This scenario with the same View binded multiple times can happen after a "invalidateActions" call.
+        mockkStatic("org.mozilla.fenix.search.toolbar.SearchSelectorToolbarActionKt") {
+            val searchEngineIcon: BitmapDrawable = mockk(relaxed = true)
+            every { any<SearchEngine>().getScaledIcon(any()) } returns searchEngineIcon
+            val store = SearchDialogFragmentStore(testSearchFragmentState)
+            val selector = SearchSelectorToolbarAction(store, mockk())
+            val view = spyk(SearchSelector(testContext))
+
+            selector.bind(view)
+            selector.bind(view)
+            selector.bind(view)
+            store.dispatch(
+                SearchDefaultEngineSelected(
+                    engine = testSearchEngine,
+                    settings = mockk(relaxed = true)
+                )
+            )
+            store.waitUntilIdle()
+
+            verify { testSearchEngine.getScaledIcon(any()) }
+            verify(exactly = 1) {
+                view.setIcon(
+                    icon = searchEngineIcon,
+                    contentDescription = testSearchEngine.name
+                )
+            }
+        }
+    }
+
+    @Test
+    fun `GIVEN a binded search selector View WHEN a search engine is selected THEN update the icon only if a different search engine is selected`() {
+        mockkStatic("org.mozilla.fenix.search.toolbar.SearchSelectorToolbarActionKt") {
+            val searchEngineIcon: BitmapDrawable = mockk(relaxed = true)
+            every { any<SearchEngine>().getScaledIcon(any()) } returns searchEngineIcon
+            val store = SearchDialogFragmentStore(testSearchFragmentState)
+            val selector = SearchSelectorToolbarAction(store, mockk())
+            val view = spyk(SearchSelector(testContext))
+
+            // Test an initial change
+            selector.bind(view)
+            store.dispatch(
+                SearchDefaultEngineSelected(
+                    engine = testSearchEngine,
+                    settings = mockk(relaxed = true)
+                )
+            )
+            store.waitUntilIdle()
+            verify(exactly = 1) { testSearchEngine.getScaledIcon(any()) }
+            verify(exactly = 1) {
+                view.setIcon(
+                    icon = searchEngineIcon,
+                    contentDescription = testSearchEngine.name
+                )
+            }
+
+            // Test the same search engine being selected
+            store.dispatch(
+                SearchDefaultEngineSelected(
+                    engine = testSearchEngine,
+                    settings = mockk(relaxed = true)
+                )
+            )
+            store.waitUntilIdle()
+            verify(exactly = 1) { testSearchEngine.getScaledIcon(any()) }
+            verify(exactly = 1) {
+                view.setIcon(
+                    icon = searchEngineIcon,
+                    contentDescription = testSearchEngine.name
+                )
+            }
+
+            // Test another search engine being selected
+            val newSearchEngine = testSearchEngine.copy(
+                name = "NewSearchEngine"
+            )
+            store.dispatch(
+                SearchHistoryEngineSelected(
+                    engine = newSearchEngine
+                )
+            )
+            store.waitUntilIdle()
+            verify(exactly = 1) { testSearchEngine.getScaledIcon(any()) }
+            verify(exactly = 1) { newSearchEngine.getScaledIcon(any()) }
+            verify(exactly = 1) {
+                view.setIcon(
+                    icon = searchEngineIcon,
+                    contentDescription = testSearchEngine.name
+                )
+            }
+            verify(exactly = 1) {
+                view.setIcon(
+                    icon = searchEngineIcon,
+                    contentDescription = newSearchEngine.name
+                )
+            }
+        }
+    }
+
+    @Test
+    fun `GIVEN a search engine WHEN asking for a scaled icon THEN return a drawable with a fixed size`() {
+        val originalIcon = Bitmap.createBitmap(100, 100, ARGB_8888).applyCanvas {
+            drawColor(Color.RED)
+        }
+        val expectedScaledIcon = Bitmap.createBitmap(
+            testContext.resources.getDimensionPixelSize(R.dimen.preference_icon_drawable_size),
+            testContext.resources.getDimensionPixelSize(R.dimen.preference_icon_drawable_size),
+            ARGB_8888
+        ).applyCanvas {
+            drawColor(Color.RED)
+        }
+        val searchEngine = testSearchEngine.copy(
+            icon = originalIcon
+        )
+
+        val result = searchEngine.getScaledIcon(testContext)
+
+        // Check dimensions, config and pixel data
+        assertTrue(expectedScaledIcon.sameAs(result.bitmap))
+    }
 }
+
+private val testSearchFragmentState = SearchFragmentState(
+    query = "https://example.com",
+    url = "https://example.com",
+    searchTerms = "search terms",
+    searchEngineSource = SearchEngineSource.None,
+    defaultEngine = null,
+    showSearchSuggestions = false,
+    showSearchShortcutsSetting = false,
+    showSearchSuggestionsHint = false,
+    showSearchShortcuts = false,
+    areShortcutsAvailable = false,
+    showClipboardSuggestions = false,
+    showHistorySuggestions = false,
+    showBookmarkSuggestions = false,
+    showSyncedTabsSuggestions = false,
+    showSessionSuggestions = true,
+    tabId = "tabId",
+    pastedText = "",
+    searchAccessPoint = MetricsUtils.Source.SHORTCUT
+)
+
+private val testSearchEngine = SearchEngine(
+    id = UUID.randomUUID().toString(),
+    name = "testSearchEngine",
+    icon = mockk(),
+    type = BUNDLED,
+    resultUrls = listOf(
+        "https://www.startpage.com/sp/search?q={searchTerms}"
+    )
+)