For #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.
This commit is contained in:
parent
070d6288e1
commit
1a2be2065a
|
@ -713,7 +713,6 @@ class SearchDialogFragment : AppCompatDialogFragment(), UserInteractionHandler {
|
|||
SearchSelectorToolbarAction(
|
||||
store = store,
|
||||
menu = searchSelectorMenu,
|
||||
viewLifecycleOwner = viewLifecycleOwner
|
||||
)
|
||||
)
|
||||
|
||||
|
|
|
@ -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
|
||||
|
||||
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)
|
||||
|
||||
reference.get()?.setIcon(icon, searchEngine.name)
|
||||
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()
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* 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
|
||||
)
|
||||
|
||||
return BitmapDrawable(context.resources, scaledIcon)
|
||||
}
|
||||
|
|
|
@ -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}"
|
||||
)
|
||||
)
|
||||
|
|
Loading…
Reference in New Issue
Block a user