diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt index 3457f0a4310..4e220e0adfa 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt +++ b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt @@ -24,7 +24,7 @@ import kotlin.coroutines.CoroutineContext * @param historyStorage An instance of [PlacesHistoryStorage], used for retrieving top frecent * sites from history. * @param topSitesProvider An optional instance of [TopSitesProvider], used for retrieving - * additional top sites from a provider. + * additional top sites from a provider. The returned top sites are added before pinned sites. * @param defaultTopSites A list containing a title to url pair of default top sites to be added * to the [PinnedSiteStorage]. */ @@ -83,28 +83,34 @@ class DefaultTopSitesStorage( } } - @Suppress("TooGenericExceptionCaught") + @Suppress("ComplexCondition", "TooGenericExceptionCaught") override suspend fun getTopSites( totalSites: Int, - fetchProvidedTopSites: Boolean, - frecencyConfig: FrecencyThresholdOption? + frecencyConfig: FrecencyThresholdOption?, + providerConfig: TopSitesProviderConfig? ): List { val topSites = ArrayList() val pinnedSites = pinnedSitesStorage.getPinnedSites().take(totalSites) var numSitesRequired = totalSites - pinnedSites.size - topSites.addAll(pinnedSites) - - if (fetchProvidedTopSites && topSitesProvider != null) { + if (topSitesProvider != null && + providerConfig != null && + providerConfig.showProviderTopSites && + pinnedSites.size < providerConfig.maxThreshold + ) { try { - val providerTopSites = topSitesProvider.getTopSites() - topSites.addAll(providerTopSites.take(numSitesRequired)) + val providerTopSites = topSitesProvider + .getTopSites() + .take(numSitesRequired) + topSites.addAll(providerTopSites) numSitesRequired -= providerTopSites.size } catch (e: Exception) { logger.error("Failed to fetch top sites from provider", e) } } + topSites.addAll(pinnedSites) + if (frecencyConfig != null && numSitesRequired > 0) { // Get 'totalSites' sites for duplicate entries with // existing pinned sites diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt index 278a4036ac3..bf79eaa8e67 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt +++ b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt @@ -11,13 +11,26 @@ import mozilla.components.concept.storage.FrecencyThresholdOption * whether or not to include top frecent sites in the top sites feature. * * @property totalSites A total number of sites that will be displayed. - * @property fetchProvidedTopSites Whether or not to fetch top sites from the [TopSitesProvider]. * @property frecencyConfig If [frecencyConfig] is specified, only visited sites with a frecency * score above the given threshold will be returned. Otherwise, frecent top site results are * not included. + * @property providerConfig An instance of [TopSitesProviderConfig] that specifies whether or + * not to fetch top sites from the [TopSitesProvider]. */ data class TopSitesConfig( val totalSites: Int, - val fetchProvidedTopSites: Boolean, - val frecencyConfig: FrecencyThresholdOption? + val frecencyConfig: FrecencyThresholdOption?, + val providerConfig: TopSitesProviderConfig? +) + +/** + * Top sites provider configuration to specify whether or not to fetch top sites from the provider. + * + * @property showProviderTopSites Whether or not to display the top sites from the provider. + * @property maxThreshold Only fetch the top sites from the provider if the number of top sites are + * below the maximum threshold. + */ +data class TopSitesProviderConfig( + val showProviderTopSites: Boolean, + val maxThreshold: Int = Int.MAX_VALUE ) diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt index 155fdc90463..3351bb8c4b1 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt +++ b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt @@ -43,15 +43,16 @@ interface TopSitesStorage : Observable { * If `frecencyConfig` is specified, fill in any missing top sites with frecent top site results. * * @param totalSites A total number of sites that will be retrieve if possible. - * @param fetchProvidedTopSites Whether or not to fetch top sites from the [TopSitesProvider]. * @param frecencyConfig If [frecencyConfig] is specified, only visited sites with a frecency * score above the given threshold will be returned. Otherwise, frecent top site results are * not included. + * @param providerConfig An instance of [TopSitesProviderConfig] that specifies whether or + * not to fetch top sites from the [TopSitesProvider]. */ suspend fun getTopSites( totalSites: Int, - fetchProvidedTopSites: Boolean, - frecencyConfig: FrecencyThresholdOption? + frecencyConfig: FrecencyThresholdOption? = null, + providerConfig: TopSitesProviderConfig? = null ): List /** diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt index da3ed0ea745..4ffb86e65cf 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt +++ b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt @@ -45,9 +45,9 @@ internal class DefaultTopSitesPresenter( scope.launch { val topSites = storage.getTopSites( - innerConfig.totalSites, - innerConfig.fetchProvidedTopSites, - innerConfig.frecencyConfig + totalSites = innerConfig.totalSites, + frecencyConfig = innerConfig.frecencyConfig, + providerConfig = innerConfig.providerConfig ) scope.launch(Dispatchers.Main) { diff --git a/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt b/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt index 8d61bde0d8a..2574fbfaf8a 100644 --- a/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt +++ b/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt @@ -154,7 +154,7 @@ class DefaultTopSitesStorageTest { } @Test - fun `getTopSites returns only default and pinned sites when frecencyConfig is null`() = runBlockingTest { + fun `GIVEN frecencyConfig and providerConfig are null WHEN getTopSites is called THEN only default and pinned sites are returned`() = runBlockingTest { val defaultTopSitesStorage = DefaultTopSitesStorage( pinnedSitesStorage = pinnedSitesStorage, historyStorage = historyStorage, @@ -183,41 +183,25 @@ class DefaultTopSitesStorageTest { ) whenever(pinnedSitesStorage.getPinnedSitesCount()).thenReturn(2) - var topSites = defaultTopSitesStorage.getTopSites( - totalSites = 0, - fetchProvidedTopSites = false, - frecencyConfig = null - ) + var topSites = defaultTopSitesStorage.getTopSites(totalSites = 0) assertTrue(topSites.isEmpty()) assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) - topSites = defaultTopSitesStorage.getTopSites( - totalSites = 1, - fetchProvidedTopSites = false, - frecencyConfig = null - ) + topSites = defaultTopSitesStorage.getTopSites(totalSites = 1) assertEquals(1, topSites.size) assertEquals(defaultSite, topSites[0]) assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) - topSites = defaultTopSitesStorage.getTopSites( - totalSites = 2, - fetchProvidedTopSites = false, - frecencyConfig = null - ) + topSites = defaultTopSitesStorage.getTopSites(totalSites = 2) assertEquals(2, topSites.size) assertEquals(defaultSite, topSites[0]) assertEquals(pinnedSite, topSites[1]) assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) - topSites = defaultTopSitesStorage.getTopSites( - totalSites = 5, - fetchProvidedTopSites = false, - frecencyConfig = null - ) + topSites = defaultTopSitesStorage.getTopSites(totalSites = 5) assertEquals(2, topSites.size) assertEquals(defaultSite, topSites[0]) @@ -226,7 +210,7 @@ class DefaultTopSitesStorageTest { } @Test - fun `GIVEN fetchProvidedTopSites is true WHEN getTopSites is called THEN default, pinned and provided top sites are returned`() = runBlockingTest { + fun `GIVEN providerConfig is specified WHEN getTopSites is called THEN default, pinned and provided top sites are returned`() = runBlockingTest { val defaultTopSitesStorage = DefaultTopSitesStorage( pinnedSitesStorage = pinnedSitesStorage, historyStorage = historyStorage, @@ -265,19 +249,16 @@ class DefaultTopSitesStorageTest { ) whenever(topSitesProvider.getTopSites()).thenReturn(listOf(providedSite)) - var topSites = defaultTopSitesStorage.getTopSites( - totalSites = 0, - fetchProvidedTopSites = false, - frecencyConfig = null - ) + var topSites = defaultTopSitesStorage.getTopSites(totalSites = 0) assertTrue(topSites.isEmpty()) assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) topSites = defaultTopSitesStorage.getTopSites( totalSites = 1, - fetchProvidedTopSites = true, - frecencyConfig = null + providerConfig = TopSitesProviderConfig( + showProviderTopSites = true + ) ) assertEquals(1, topSites.size) @@ -286,8 +267,34 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 2, - fetchProvidedTopSites = true, - frecencyConfig = null + providerConfig = TopSitesProviderConfig( + showProviderTopSites = true + ) + ) + + assertEquals(2, topSites.size) + assertEquals(defaultSite, topSites[0]) + assertEquals(pinnedSite, topSites[1]) + assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) + + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 5, + providerConfig = TopSitesProviderConfig( + showProviderTopSites = true + ) + ) + + assertEquals(3, topSites.size) + assertEquals(providedSite, topSites[0]) + assertEquals(defaultSite, topSites[1]) + assertEquals(pinnedSite, topSites[2]) + assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) + + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 5, + providerConfig = TopSitesProviderConfig( + showProviderTopSites = false + ) ) assertEquals(2, topSites.size) @@ -297,19 +304,35 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 5, - fetchProvidedTopSites = true, - frecencyConfig = null + providerConfig = TopSitesProviderConfig( + showProviderTopSites = true, + maxThreshold = 8 + ) ) assertEquals(3, topSites.size) + assertEquals(providedSite, topSites[0]) + assertEquals(defaultSite, topSites[1]) + assertEquals(pinnedSite, topSites[2]) + assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) + + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 5, + frecencyConfig = null, + providerConfig = TopSitesProviderConfig( + showProviderTopSites = true, + maxThreshold = 2 + ) + ) + + assertEquals(2, topSites.size) assertEquals(defaultSite, topSites[0]) assertEquals(pinnedSite, topSites[1]) - assertEquals(providedSite, topSites[2]) assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) } @Test - fun `GIVEN fetchProvidedTopSites is true and frecencyConfig is specified WHEN getTopSites is called THEN default, pinned, provided and frecent top sites are returned`() = runBlockingTest { + fun `GIVEN frecencyConfig and providerConfig are specified WHEN getTopSites is called THEN default, pinned, provided and frecent top sites are returned`() = runBlockingTest { val defaultTopSitesStorage = DefaultTopSitesStorage( pinnedSitesStorage = pinnedSitesStorage, historyStorage = historyStorage, @@ -353,16 +376,20 @@ class DefaultTopSitesStorageTest { var topSites = defaultTopSitesStorage.getTopSites( totalSites = 0, - fetchProvidedTopSites = true, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + providerConfig = TopSitesProviderConfig( + showProviderTopSites = true + ) ) assertTrue(topSites.isEmpty()) topSites = defaultTopSitesStorage.getTopSites( totalSites = 1, - fetchProvidedTopSites = true, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + providerConfig = TopSitesProviderConfig( + showProviderTopSites = true + ) ) assertEquals(1, topSites.size) @@ -371,8 +398,10 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 2, - fetchProvidedTopSites = true, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + providerConfig = TopSitesProviderConfig( + showProviderTopSites = true + ) ) assertEquals(2, topSites.size) @@ -382,26 +411,30 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 3, - fetchProvidedTopSites = true, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + providerConfig = TopSitesProviderConfig( + showProviderTopSites = true + ) ) assertEquals(3, topSites.size) - assertEquals(defaultSite, topSites[0]) - assertEquals(pinnedSite, topSites[1]) - assertEquals(providedSite, topSites[2]) + assertEquals(providedSite, topSites[0]) + assertEquals(defaultSite, topSites[1]) + assertEquals(pinnedSite, topSites[2]) assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) topSites = defaultTopSitesStorage.getTopSites( totalSites = 5, - fetchProvidedTopSites = true, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + providerConfig = TopSitesProviderConfig( + showProviderTopSites = true + ) ) assertEquals(4, topSites.size) - assertEquals(defaultSite, topSites[0]) - assertEquals(pinnedSite, topSites[1]) - assertEquals(providedSite, topSites[2]) + assertEquals(providedSite, topSites[0]) + assertEquals(defaultSite, topSites[1]) + assertEquals(pinnedSite, topSites[2]) assertEquals(frecentSite1.toTopSite(), topSites[3]) assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) } @@ -441,7 +474,6 @@ class DefaultTopSitesStorageTest { var topSites = defaultTopSitesStorage.getTopSites( totalSites = 0, - fetchProvidedTopSites = false, frecencyConfig = FrecencyThresholdOption.NONE ) @@ -449,7 +481,6 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 1, - fetchProvidedTopSites = false, frecencyConfig = FrecencyThresholdOption.NONE ) @@ -459,7 +490,6 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 2, - fetchProvidedTopSites = false, frecencyConfig = FrecencyThresholdOption.NONE ) @@ -470,7 +500,6 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 5, - fetchProvidedTopSites = false, frecencyConfig = FrecencyThresholdOption.NONE ) @@ -492,7 +521,6 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 5, - fetchProvidedTopSites = false, frecencyConfig = FrecencyThresholdOption.NONE ) @@ -516,7 +544,6 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 5, - fetchProvidedTopSites = false, frecencyConfig = FrecencyThresholdOption.NONE ) @@ -582,7 +609,6 @@ class DefaultTopSitesStorageTest { val topSites = defaultTopSitesStorage.getTopSites( totalSites = 5, - fetchProvidedTopSites = false, frecencyConfig = FrecencyThresholdOption.NONE ) diff --git a/docs/changelog.md b/docs/changelog.md index f62d9ea6efe..f8808bd6ab0 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -47,6 +47,7 @@ permalink: /changelog/ * ⚠️ **This is a breaking change**: The existing data class `TopSite` has been converted into a sealed class. [#11483](https://github.com/mozilla-mobile/android-components/issues/11483) * Extend `DefaultTopSitesStorage` to accept a `TopSitesProvider` for fetching top sites. [#11483](https://github.com/mozilla-mobile/android-components/issues/11483) * ⚠️ **This is a breaking change**: Added a new parameter `fetchProvidedTopSites` to `TopSitesConfig` to specify whether or not to fetch top sites from the `TopSitesProvider`. [#11605](https://github.com/mozilla-mobile/android-components/issues/11605) + * ⚠️ **This is a breaking change**: This changes `fetchProvidedTopSites` in `TopSitesConfig` into a data class `TopSitesProviderConfig` that specifies whether or not to display the top sites from the provider. [#11654](https://github.com/mozilla-mobile/android-components/issues/11654) * **concept-storage** * ⚠️ **This is a breaking change**: Adds a new `isRemote` property in `VisitInfo` which distinguishes visits made on the device and those that come from Sync.