From f3f2fadd822e63b72f464a2cd35115d406c2b2ca Mon Sep 17 00:00:00 2001 From: Alexandru2909 Date: Thu, 11 Aug 2022 16:22:39 +0300 Subject: [PATCH] [fenix] For https://github.com/mozilla-mobile/fenix/issues/26399 - Use a list of tabs for recent synced tabs success state --- .../RecentSyncedTabFeature.kt | 64 ++++++++++---- .../view/RecentSyncedTabViewHolder.kt | 2 +- .../mozilla/fenix/components/AppStoreTest.kt | 6 +- .../RecentSyncedTabFeatureTest.kt | 87 ++++++++++++------- 4 files changed, 104 insertions(+), 55 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeature.kt b/app/src/main/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeature.kt index 103d6a6d01..bbbfeebd05 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeature.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeature.kt @@ -9,6 +9,8 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach import mozilla.components.concept.storage.HistoryStorage +import mozilla.components.browser.storage.sync.Tab +import mozilla.components.concept.sync.Device import mozilla.components.concept.sync.DeviceType import mozilla.components.feature.syncedtabs.storage.SyncedTabsStorage import mozilla.components.lib.state.ext.flow @@ -22,10 +24,10 @@ import mozilla.components.service.fxa.sync.SyncReason import mozilla.components.support.base.feature.LifecycleAwareFeature import mozilla.components.support.ktx.kotlin.tryGetHostFromUrl import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifChanged -import org.mozilla.fenix.components.AppStore -import org.mozilla.fenix.components.appstate.AppAction import mozilla.telemetry.glean.GleanTimerId import org.mozilla.fenix.GleanMetrics.RecentSyncedTabs +import org.mozilla.fenix.components.AppStore +import org.mozilla.fenix.components.appstate.AppAction import java.util.concurrent.TimeUnit /** @@ -50,7 +52,7 @@ class RecentSyncedTabFeature( ) : LifecycleAwareFeature { private var syncStartId: GleanTimerId? = null - private var lastSyncedTab: RecentSyncedTab? = null + private var lastSyncedTabs: List? = null override fun start() { collectAccountUpdates() @@ -110,11 +112,20 @@ class RecentSyncedTabFeature( return } - val syncedTab = storage.getSyncedDeviceTabs() + val syncedTabs = storage.getSyncedDeviceTabs() .filterNot { it.device.isCurrentDevice || it.tabs.isEmpty() } - .maxByOrNull { it.device.lastAccessTime ?: 0 } - ?.let { - val tab = it.tabs.firstOrNull()?.active() ?: return + .flatMap { + it.tabs.map { tab -> + SyncedDeviceTab(it.device, tab) + } + } + .ifEmpty { return } + // We want to get the last device used based on the most recent accessed tab, + // as described here: https://github.com/mozilla-mobile/fenix/issues/26398 + .sortedByDescending { deviceTab -> deviceTab.tab.lastUsed } + .take(MAX_RECENT_SYNCED_TABS) + .map { deviceTab -> + val activeTabEntry = deviceTab.tab.active() val currentTime = System.currentTimeMillis() val maxAgeInMs = TimeUnit.DAYS.toMillis(DAYS_HISTORY_FOR_PREVIEW_IMAGE) @@ -126,28 +137,27 @@ class RecentSyncedTabFeature( // Searching history entries for any that share a top level domain and have a // preview image URL available casts a wider net for finding a suitable image. val previewImageUrl = history.find { entry -> - entry.url.contains(tab.url.tryGetHostFromUrl()) && entry.previewImageUrl != null + entry.url.contains(activeTabEntry.url.tryGetHostFromUrl()) && entry.previewImageUrl != null }?.previewImageUrl RecentSyncedTab( - deviceDisplayName = it.device.displayName, - deviceType = it.device.deviceType, - title = tab.title, - url = tab.url, + deviceDisplayName = deviceTab.device.displayName, + deviceType = deviceTab.device.deviceType, + title = activeTabEntry.title, + url = activeTabEntry.url, previewImageUrl = previewImageUrl ) } - - if (syncedTab == null) { + if (syncedTabs.isEmpty()) { appStore.dispatch( AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None) ) } else { - recordMetrics(syncedTab, lastSyncedTab, syncStartId) + recordMetrics(syncedTabs.first(), lastSyncedTabs?.first(), syncStartId) appStore.dispatch( - AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(syncedTab)) + AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(syncedTabs)) ) - lastSyncedTab = syncedTab + lastSyncedTabs = syncedTabs } } @@ -180,6 +190,11 @@ class RecentSyncedTabFeature( */ const val DAYS_HISTORY_FOR_PREVIEW_IMAGE = 3L + + /** + * Number of recent synced tabs we want to keep in the success state. + */ + const val MAX_RECENT_SYNCED_TABS = 8 } } @@ -198,9 +213,9 @@ sealed class RecentSyncedTabState { object Loading : RecentSyncedTabState() /** - * A user is authenticated and the most recent synced tab has been found. + * A user is authenticated and most recent synced tabs have been found. */ - data class Success(val tab: RecentSyncedTab) : RecentSyncedTabState() + data class Success(val tabs: List) : RecentSyncedTabState() } /** @@ -218,3 +233,14 @@ data class RecentSyncedTab( val url: String, val previewImageUrl: String?, ) + +/** + * Class representing a tab from a synced device. + * + * @param device The synced [Device]. + * @param tab The tab from the synced device. + */ +private data class SyncedDeviceTab( + val device: Device, + val tab: Tab +) diff --git a/app/src/main/java/org/mozilla/fenix/home/recentsyncedtabs/view/RecentSyncedTabViewHolder.kt b/app/src/main/java/org/mozilla/fenix/home/recentsyncedtabs/view/RecentSyncedTabViewHolder.kt index d62ac47e62..16b2cdc8ea 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recentsyncedtabs/view/RecentSyncedTabViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recentsyncedtabs/view/RecentSyncedTabViewHolder.kt @@ -48,7 +48,7 @@ class RecentSyncedTabViewHolder( val syncedTab = when (it) { RecentSyncedTabState.None, RecentSyncedTabState.Loading -> null - is RecentSyncedTabState.Success -> it.tab + is RecentSyncedTabState.Success -> it.tabs.firstOrNull() } RecentSyncedTab( tab = syncedTab, diff --git a/app/src/test/java/org/mozilla/fenix/components/AppStoreTest.kt b/app/src/test/java/org/mozilla/fenix/components/AppStoreTest.kt index bbf3ae3b5e..5eab339315 100644 --- a/app/src/test/java/org/mozilla/fenix/components/AppStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/AppStoreTest.kt @@ -165,11 +165,11 @@ class AppStoreTest { appStore.dispatch(AppAction.RecentSyncedTabStateChange(loading)).join() assertEquals(loading, appStore.state.recentSyncedTabState) - val recentSyncedTab = RecentSyncedTab("device name", DeviceType.DESKTOP, "title", "url", null) - val success = RecentSyncedTabState.Success(recentSyncedTab) + val recentSyncedTabs = listOf(RecentSyncedTab("device name", DeviceType.DESKTOP, "title", "url", null)) + val success = RecentSyncedTabState.Success(recentSyncedTabs) appStore.dispatch(AppAction.RecentSyncedTabStateChange(success)).join() assertEquals(success, appStore.state.recentSyncedTabState) - assertEquals(recentSyncedTab, (appStore.state.recentSyncedTabState as RecentSyncedTabState.Success).tab) + assertEquals(recentSyncedTabs, (appStore.state.recentSyncedTabState as RecentSyncedTabState.Success).tabs) } @Test diff --git a/app/src/test/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeatureTest.kt b/app/src/test/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeatureTest.kt index 3cddf06b4a..26583e5716 100644 --- a/app/src/test/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeatureTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeatureTest.kt @@ -178,7 +178,7 @@ class RecentSyncedTabFeatureTest { syncStore.setState(status = SyncStatus.Idle) runCurrent() - val expected = activeTab.toRecentSyncedTab(deviceAccessed1) + val expected = listOf(activeTab.toRecentSyncedTab(deviceAccessed1)) verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expected))) } } @@ -202,10 +202,10 @@ class RecentSyncedTabFeatureTest { syncStore.setState(status = SyncStatus.Idle) runCurrent() - val expectedTab = remoteTab.toRecentSyncedTab(deviceAccessed1) + val expectedTabs = listOf(remoteTab.toRecentSyncedTab(deviceAccessed1)) verify { appStore.dispatch( - AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab)) + AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTabs)) ) } } @@ -229,41 +229,63 @@ class RecentSyncedTabFeatureTest { syncStore.setState(status = SyncStatus.Idle) runCurrent() - val expectedTab = remoteTab.toRecentSyncedTab(deviceAccessed1) + val expectedTabs = listOf(remoteTab.toRecentSyncedTab(deviceAccessed1)) verify { appStore.dispatch( - AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab)) + AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTabs)) ) } } @Test - fun `GIVEN tabs from different remote devices WHEN dispatching recent synced tab THEN most recently accessed device is used`() = runTest { - val account = mockk() - syncStore.setState(account = account) - every { appStore.state } returns mockk { - every { recentSyncedTabState } returns RecentSyncedTabState.Loading - } - coEvery { historyStorage.getDetailedVisits(any(), any()) } returns listOf() - val firstTab = createActiveTab("first", "https://local.com", null) - val secondTab = createActiveTab("remote", "https://mozilla.org", null) - val syncedTabs = listOf( - SyncedDeviceTabs(deviceAccessed1, listOf(firstTab)), - SyncedDeviceTabs(deviceAccessed2, listOf(secondTab)) - ) - coEvery { syncedTabsStorage.getSyncedDeviceTabs() } returns syncedTabs - - feature.start() - syncStore.setState(status = SyncStatus.Idle) - runCurrent() - - val expectedTab = secondTab.toRecentSyncedTab(deviceAccessed2) - verify { - appStore.dispatch( - AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab)) + fun `GIVEN tabs from different remote devices WHEN dispatching recent synced tab THEN most recently accessed tabs are set in the Success state`() = + runTest { + val account = mockk() + syncStore.setState(account = account) + every { appStore.state } returns mockk { + every { recentSyncedTabState } returns RecentSyncedTabState.Loading + } + coEvery { historyStorage.getDetailedVisits(any(), any()) } returns listOf() + val firstDeviceTabs = listOf( + createActiveTab("first", "https://local.com", null), + createActiveTab("second", "https://github.com", null) ) + val secondDeviceTabs = listOf( + createActiveTab("first", "https://mozilla.org", null), + createActiveTab("second", "https://www.mozilla.org/en-US/firefox", null) + ) + val currentTime = System.currentTimeMillis() + // Delay used to change last used times of tabs + val usedDelay = 5 * 60 * 1000 + every { firstDeviceTabs[0].lastUsed } returns currentTime + every { firstDeviceTabs[1].lastUsed } returns currentTime - 2 * usedDelay + every { secondDeviceTabs[0].lastUsed } returns currentTime - usedDelay + every { secondDeviceTabs[1].lastUsed } returns currentTime - 3 * usedDelay + val syncedTabs = listOf( + SyncedDeviceTabs(deviceAccessed1, firstDeviceTabs), + SyncedDeviceTabs(deviceAccessed2, secondDeviceTabs) + ) + coEvery { syncedTabsStorage.getSyncedDeviceTabs() } returns syncedTabs + + feature.start() + syncStore.setState(status = SyncStatus.Idle) + runCurrent() + + // The order of the tabs should be given by the `lastUsed` property + val expectedTabs = + (firstDeviceTabs + secondDeviceTabs).sortedByDescending { it.lastUsed }.map { + if (it in firstDeviceTabs) { + it.toRecentSyncedTab(deviceAccessed1) + } else { + it.toRecentSyncedTab(deviceAccessed2) + } + } + verify { + appStore.dispatch( + AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTabs)) + ) + } } - } @Test fun `GIVEN sync tabs are disabled WHEN dispatching recent synced tab THEN dispatch none`() = runTest { @@ -421,7 +443,7 @@ class RecentSyncedTabFeatureTest { syncStore.setState(status = SyncStatus.LoggedOut) runCurrent() - val expected = tab.toRecentSyncedTab(deviceAccessed1) + val expected = listOf(tab.toRecentSyncedTab(deviceAccessed1)) verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expected))) } verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } } @@ -450,7 +472,7 @@ class RecentSyncedTabFeatureTest { syncStore.setState(status = SyncStatus.Idle) runCurrent() - val expected = activeTab.toRecentSyncedTab(deviceAccessed1, previewUrl) + val expected = listOf(activeTab.toRecentSyncedTab(deviceAccessed1, previewUrl)) verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expected))) } } @@ -477,7 +499,7 @@ class RecentSyncedTabFeatureTest { syncStore.setState(status = SyncStatus.Idle) runCurrent() - val expected = activeTab.toRecentSyncedTab(deviceAccessed1, null) + val expected = listOf(activeTab.toRecentSyncedTab(deviceAccessed1, null)) verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expected))) } } @@ -489,6 +511,7 @@ class RecentSyncedTabFeatureTest { val tab = mockk() val tabEntry = TabEntry(title, url, iconUrl) every { tab.active() } returns tabEntry + every { tab.lastUsed } returns System.currentTimeMillis() return tab }