From 3e2f501795ed6795c2708a407dfef2b57691ce4f Mon Sep 17 00:00:00 2001 From: Jonathan Almeida Date: Wed, 16 Jun 2021 15:57:07 -0400 Subject: [PATCH] [fenix] Issue https://github.com/mozilla-mobile/fenix/issues/20054: Update recent tab on title or icon changes --- .../mozilla/fenix/home/HomeFragmentStore.kt | 6 +- .../home/recenttabs/RecentTabsListFeature.kt | 17 ++-- .../recenttabs/view/RecentTabViewHolder.kt | 12 ++- .../sessioncontrol/SessionControlAdapter.kt | 8 ++ .../fenix/home/RecentTabsListFeatureTest.kt | 77 ++++++++++++++++++- .../view/RecentTabViewHolderTest.kt | 26 +++++++ 6 files changed, 129 insertions(+), 17 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt b/app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt index c10a2f0b1..45c6933c9 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt @@ -9,6 +9,7 @@ import mozilla.components.browser.state.state.TabSessionState import mozilla.components.feature.tab.collections.TabCollection import mozilla.components.feature.top.sites.TopSite import mozilla.components.lib.state.Action +import mozilla.components.lib.state.Middleware import mozilla.components.lib.state.State import mozilla.components.lib.state.Store import org.mozilla.fenix.components.tips.Tip @@ -17,9 +18,10 @@ import org.mozilla.fenix.components.tips.Tip * The [Store] for holding the [HomeFragmentState] and applying [HomeFragmentAction]s. */ class HomeFragmentStore( - initialState: HomeFragmentState = HomeFragmentState() + initialState: HomeFragmentState = HomeFragmentState(), + middlewares: List> = emptyList() ) : Store( - initialState, ::homeFragmentStateReducer + initialState, ::homeFragmentStateReducer, middlewares ) data class Tab( diff --git a/app/src/main/java/org/mozilla/fenix/home/recenttabs/RecentTabsListFeature.kt b/app/src/main/java/org/mozilla/fenix/home/recenttabs/RecentTabsListFeature.kt index 52e774ded..1a8701098 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recenttabs/RecentTabsListFeature.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recenttabs/RecentTabsListFeature.kt @@ -8,11 +8,11 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.collect import kotlinx.coroutines.flow.map -import mozilla.components.browser.state.selector.normalTabs +import mozilla.components.browser.state.selector.selectedTab import mozilla.components.browser.state.state.BrowserState import mozilla.components.browser.state.store.BrowserStore import mozilla.components.lib.state.helpers.AbstractBinding -import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifChanged +import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifAnyChanged import org.mozilla.fenix.home.HomeFragmentAction import org.mozilla.fenix.home.HomeFragmentStore @@ -22,20 +22,17 @@ import org.mozilla.fenix.home.HomeFragmentStore */ @OptIn(ExperimentalCoroutinesApi::class) class RecentTabsListFeature( - private val browserStore: BrowserStore, + browserStore: BrowserStore, private val homeStore: HomeFragmentStore ) : AbstractBinding(browserStore) { override suspend fun onState(flow: Flow) { - flow.map { it.selectedTabId } - .ifChanged() - .collect { selectedTabId -> + flow.map { it.selectedTab } + .ifAnyChanged { arrayOf(it?.id, it?.content?.title, it?.content?.icon) } + .collect { selectedTab -> // Attempt to get the selected normal tab since here may not be a selected tab or // the selected tab may be a private tab. - val selectedTab = browserStore.state.normalTabs.firstOrNull { - it.id == selectedTabId - } - val recentTabsList = if (selectedTab != null) { + val recentTabsList = if (selectedTab != null && selectedTab.content.private.not()) { listOf(selectedTab) } else { emptyList() diff --git a/app/src/main/java/org/mozilla/fenix/home/recenttabs/view/RecentTabViewHolder.kt b/app/src/main/java/org/mozilla/fenix/home/recenttabs/view/RecentTabViewHolder.kt index 5ac12cd58..87fea0fe9 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recenttabs/view/RecentTabViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recenttabs/view/RecentTabViewHolder.kt @@ -7,6 +7,7 @@ package org.mozilla.fenix.home.recenttabs.view import android.view.View import kotlinx.android.synthetic.main.recent_tabs_list_row.* import mozilla.components.browser.icons.BrowserIcons +import mozilla.components.browser.state.state.ContentState import mozilla.components.browser.state.state.TabSessionState import org.mozilla.fenix.R import org.mozilla.fenix.ext.components @@ -18,7 +19,8 @@ import org.mozilla.fenix.utils.view.ViewHolder * View holder for a recent tab item. * * @param interactor [RecentTabInteractor] which will have delegated to all user interactions. - * @param icons + * @param icons an instance of [BrowserIcons] for rendering the sites icon if one isn't found + * in [ContentState.icon]. */ class RecentTabViewHolder( view: View, @@ -27,13 +29,19 @@ class RecentTabViewHolder( ) : ViewHolder(view) { fun bindTab(tab: TabSessionState) { - recent_tab_title.text = tab.content.title + // A page may take a while to retrieve a title, so let's show the url until we get one. + recent_tab_title.text = if (tab.content.title.isNotEmpty()) { + tab.content.title + } else { + tab.content.url + } if (tab.content.icon != null) { recent_tab_icon.setImageBitmap(tab.content.icon) } else { icons.loadIntoView(recent_tab_icon, tab.content.url) } + recent_tab_icon.setImageBitmap(tab.content.icon) itemView.setOnClickListener { interactor.onRecentTabClicked(tab.id) 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 26a7c4da9..44bbaf730 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 @@ -137,6 +137,14 @@ sealed class AdapterItem(@LayoutRes val viewType: Int) { object RecentTabsHeader : AdapterItem(RecentTabsHeaderViewHolder.LAYOUT_ID) data class RecentTabItem(val tab: TabSessionState) : AdapterItem(RecentTabViewHolder.LAYOUT_ID) { override fun sameAs(other: AdapterItem) = other is RecentTabItem && tab.id == other.tab.id + + override fun contentsSameAs(other: AdapterItem): Boolean { + val otherItem = other as RecentTabItem + // We only care about updating if the title and icon have changed because that is + // all we show today. This should be updated if we want to show updates for more. + return tab.content.title == otherItem.tab.content.title && + tab.content.icon == otherItem.tab.content.icon + } } /** diff --git a/app/src/test/java/org/mozilla/fenix/home/RecentTabsListFeatureTest.kt b/app/src/test/java/org/mozilla/fenix/home/RecentTabsListFeatureTest.kt index e7805085e..bf00d3f2d 100644 --- a/app/src/test/java/org/mozilla/fenix/home/RecentTabsListFeatureTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/RecentTabsListFeatureTest.kt @@ -4,26 +4,50 @@ package org.mozilla.fenix.home +import io.mockk.mockk import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.TestCoroutineDispatcher +import mozilla.components.browser.state.action.ContentAction.UpdateIconAction +import mozilla.components.browser.state.action.ContentAction.UpdateTitleAction import mozilla.components.browser.state.action.TabListAction import mozilla.components.browser.state.state.BrowserState import mozilla.components.browser.state.state.createTab import mozilla.components.browser.state.store.BrowserStore import mozilla.components.support.test.ext.joinBlocking import mozilla.components.support.test.libstate.ext.waitUntilIdle +import mozilla.components.support.test.middleware.CaptureActionsMiddleware import mozilla.components.support.test.rule.MainCoroutineRule +import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before import org.junit.Rule import org.junit.Test +import org.mozilla.fenix.home.HomeFragmentAction.RecentTabsChange import org.mozilla.fenix.home.recenttabs.RecentTabsListFeature class RecentTabsListFeatureTest { + private lateinit var homeStore: HomeFragmentStore + private lateinit var middleware: CaptureActionsMiddleware + @OptIn(ExperimentalCoroutinesApi::class) @get:Rule val coroutinesTestRule = MainCoroutineRule(TestCoroutineDispatcher()) + @Before + fun setup() { + middleware = CaptureActionsMiddleware() + homeStore = HomeFragmentStore(middlewares = listOf(middleware)) + } + + @After + fun teardown() { + middleware.reset() + } + @Test fun `GIVEN no selected tab WHEN the feature starts THEN dispatch an empty list`() { val browserStore = BrowserStore() @@ -53,7 +77,6 @@ class RecentTabsListFeatureTest { selectedTabId = "1" ) ) - val homeStore = HomeFragmentStore() val feature = RecentTabsListFeature( browserStore = browserStore, homeStore = homeStore @@ -83,7 +106,6 @@ class RecentTabsListFeatureTest { selectedTabId = "1" ) ) - val homeStore = HomeFragmentStore() val feature = RecentTabsListFeature( browserStore = browserStore, homeStore = homeStore @@ -122,7 +144,6 @@ class RecentTabsListFeatureTest { selectedTabId = "1" ) ) - val homeStore = HomeFragmentStore() val feature = RecentTabsListFeature( browserStore = browserStore, homeStore = homeStore @@ -141,4 +162,54 @@ class RecentTabsListFeatureTest { assertEquals(0, homeStore.state.recentTabs.size) } + + @Test + fun `WHEN the selected tabs title or icon update THEN update the home store`() { + val browserStore = BrowserStore( + BrowserState( + tabs = listOf( + createTab( + url = "https://www.mozilla.org", + id = "1" + ) + ), + selectedTabId = "1" + ) + ) + val feature = RecentTabsListFeature( + browserStore = browserStore, + homeStore = homeStore + ) + + feature.start() + + homeStore.waitUntilIdle() + + middleware.assertLastAction(RecentTabsChange::class) { + val tab = it.recentTabs.first() + assertTrue(tab.content.title.isEmpty()) + assertNull(tab.content.icon) + } + + browserStore.dispatch(UpdateTitleAction("1", "test")).joinBlocking() + + homeStore.waitUntilIdle() + + middleware.assertLastAction(RecentTabsChange::class) { + val tab = it.recentTabs.first() + assertEquals("test", tab.content.title) + assertNull(tab.content.icon) + } + + browserStore.dispatch(UpdateIconAction("1", "https://www.mozilla.org", mockk())) + .joinBlocking() + + homeStore.waitUntilIdle() + + middleware.assertLastAction(RecentTabsChange::class) { + val tab = it.recentTabs.first() + assertEquals("test", tab.content.title) + assertNotNull(tab.content.icon) + } + } } diff --git a/app/src/test/java/org/mozilla/fenix/home/recenttabs/view/RecentTabViewHolderTest.kt b/app/src/test/java/org/mozilla/fenix/home/recenttabs/view/RecentTabViewHolderTest.kt index 589d16463..33ec59fab 100644 --- a/app/src/test/java/org/mozilla/fenix/home/recenttabs/view/RecentTabViewHolderTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/recenttabs/view/RecentTabViewHolderTest.kt @@ -6,6 +6,7 @@ package org.mozilla.fenix.home.recenttabs.view import android.view.LayoutInflater import android.view.View +import androidx.core.graphics.drawable.toBitmap import io.mockk.every import io.mockk.mockk import io.mockk.verify @@ -16,9 +17,12 @@ import mozilla.components.browser.icons.IconRequest import mozilla.components.browser.state.state.createTab import mozilla.components.support.test.robolectric.testContext import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import org.mozilla.fenix.R import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.home.sessioncontrol.SessionControlInteractor @@ -60,4 +64,26 @@ class RecentTabViewHolderTest { verify { interactor.onRecentTabClicked(tab.id) } } + + @Test + fun `WHEN a recent tab icon exists THEN load it`() { + val bitmap = testContext.getDrawable(R.drawable.ic_search)!!.toBitmap() + val tabWithIcon = tab.copy(content = tab.content.copy(icon = bitmap)) + val viewHolder = RecentTabViewHolder(view, interactor, icons) + + assertNull(view.recent_tab_icon.drawable) + + viewHolder.bindTab(tabWithIcon) + + assertNotNull(view.recent_tab_icon.drawable) + } + + @Test + fun `WHEN a recent tab does not have a title THEN show the url`() { + val tabWithoutTitle = createTab(url = "https://mozilla.org") + + RecentTabViewHolder(view, interactor, icons).bindTab(tabWithoutTitle) + + assertEquals(tabWithoutTitle.content.url, view.recent_tab_title.text) + } }