From 88e6e1f151eb6ea6629dce17a98453b730ddc58d Mon Sep 17 00:00:00 2001 From: Gabriel Luong Date: Wed, 2 Feb 2022 01:10:50 -0500 Subject: [PATCH] Issue #11654: Change fetchProvidedTopSites into a lambda function This changes `fetchProvidedTopSites` into a lambda function `shouldFetchTopSitesFromProvider` that takes the number of existing top sites as a parameter to determine whether or not to fetch top sites from the `TopSitesProvider`. --- .../top/sites/DefaultTopSitesStorage.kt | 10 +- .../feature/top/sites/TopSitesConfig.kt | 7 +- .../feature/top/sites/TopSitesStorage.kt | 7 +- .../presenter/DefaultTopSitesPresenter.kt | 6 +- .../top/sites/DefaultTopSitesStorageTest.kt | 121 +++++++++++------- 5 files changed, 88 insertions(+), 63 deletions(-) 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..836c770380c 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 @@ -86,16 +86,14 @@ class DefaultTopSitesStorage( @Suppress("TooGenericExceptionCaught") override suspend fun getTopSites( totalSites: Int, - fetchProvidedTopSites: Boolean, - frecencyConfig: FrecencyThresholdOption? + frecencyConfig: FrecencyThresholdOption?, + shouldFetchTopSitesFromProvider: (Int) -> Boolean ): 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 && shouldFetchTopSitesFromProvider(pinnedSites.size)) { try { val providerTopSites = topSitesProvider.getTopSites() topSites.addAll(providerTopSites.take(numSitesRequired)) @@ -105,6 +103,8 @@ class DefaultTopSitesStorage( } } + 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..3697935ee75 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,14 @@ 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 shouldFetchTopSitesFromProvider Callback that takes the number of existing top sites + * as a parameter to determine 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 shouldFetchTopSitesFromProvider: (Int) -> Boolean ) 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..451ac6c0f56 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 shouldFetchTopSitesFromProvider Callback that takes the number of existing top sites + * as a parameter to determine whether or not to fetch top sites from the [TopSitesProvider]. */ suspend fun getTopSites( totalSites: Int, - fetchProvidedTopSites: Boolean, - frecencyConfig: FrecencyThresholdOption? + frecencyConfig: FrecencyThresholdOption?, + shouldFetchTopSitesFromProvider: (Int) -> Boolean ): 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..0a31566bed2 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, + shouldFetchTopSitesFromProvider = innerConfig.shouldFetchTopSitesFromProvider ) 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..9d6cb33c9e9 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 @@ -185,8 +185,8 @@ class DefaultTopSitesStorageTest { var topSites = defaultTopSitesStorage.getTopSites( totalSites = 0, - fetchProvidedTopSites = false, - frecencyConfig = null + frecencyConfig = null, + shouldFetchTopSitesFromProvider = { false } ) assertTrue(topSites.isEmpty()) @@ -194,8 +194,8 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 1, - fetchProvidedTopSites = false, - frecencyConfig = null + frecencyConfig = null, + shouldFetchTopSitesFromProvider = { false } ) assertEquals(1, topSites.size) @@ -204,8 +204,8 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 2, - fetchProvidedTopSites = false, - frecencyConfig = null + frecencyConfig = null, + shouldFetchTopSitesFromProvider = { false } ) assertEquals(2, topSites.size) @@ -215,8 +215,8 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 5, - fetchProvidedTopSites = false, - frecencyConfig = null + frecencyConfig = null, + shouldFetchTopSitesFromProvider = { false } ) assertEquals(2, topSites.size) @@ -226,7 +226,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 shouldFetchTopSitesFromProvider is specified WHEN getTopSites is called THEN default, pinned and provided top sites are returned`() = runBlockingTest { val defaultTopSitesStorage = DefaultTopSitesStorage( pinnedSitesStorage = pinnedSitesStorage, historyStorage = historyStorage, @@ -267,8 +267,8 @@ class DefaultTopSitesStorageTest { var topSites = defaultTopSitesStorage.getTopSites( totalSites = 0, - fetchProvidedTopSites = false, - frecencyConfig = null + frecencyConfig = null, + shouldFetchTopSitesFromProvider = { false } ) assertTrue(topSites.isEmpty()) @@ -276,8 +276,8 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 1, - fetchProvidedTopSites = true, - frecencyConfig = null + frecencyConfig = null, + shouldFetchTopSitesFromProvider = { true } ) assertEquals(1, topSites.size) @@ -286,8 +286,8 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 2, - fetchProvidedTopSites = true, - frecencyConfig = null + frecencyConfig = null, + shouldFetchTopSitesFromProvider = { true } ) assertEquals(2, topSites.size) @@ -297,19 +297,42 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 5, - fetchProvidedTopSites = true, - frecencyConfig = null + frecencyConfig = null, + shouldFetchTopSitesFromProvider = { 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, + frecencyConfig = null, + shouldFetchTopSitesFromProvider = { size -> size < 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, + shouldFetchTopSitesFromProvider = { size -> size > 8 } + ) + + 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 shouldFetchTopSitesFromProvider is true and frecencyConfig is 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,16 @@ class DefaultTopSitesStorageTest { var topSites = defaultTopSitesStorage.getTopSites( totalSites = 0, - fetchProvidedTopSites = true, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + shouldFetchTopSitesFromProvider = { true } ) assertTrue(topSites.isEmpty()) topSites = defaultTopSitesStorage.getTopSites( totalSites = 1, - fetchProvidedTopSites = true, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + shouldFetchTopSitesFromProvider = { true } ) assertEquals(1, topSites.size) @@ -371,8 +394,8 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 2, - fetchProvidedTopSites = true, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + shouldFetchTopSitesFromProvider = { true } ) assertEquals(2, topSites.size) @@ -382,26 +405,26 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 3, - fetchProvidedTopSites = true, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + shouldFetchTopSitesFromProvider = { 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, + shouldFetchTopSitesFromProvider = { 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,16 +464,16 @@ class DefaultTopSitesStorageTest { var topSites = defaultTopSitesStorage.getTopSites( totalSites = 0, - fetchProvidedTopSites = false, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + shouldFetchTopSitesFromProvider = { false } ) assertTrue(topSites.isEmpty()) topSites = defaultTopSitesStorage.getTopSites( totalSites = 1, - fetchProvidedTopSites = false, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + shouldFetchTopSitesFromProvider = { false } ) assertEquals(1, topSites.size) @@ -459,8 +482,8 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 2, - fetchProvidedTopSites = false, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + shouldFetchTopSitesFromProvider = { false } ) assertEquals(2, topSites.size) @@ -470,8 +493,8 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 5, - fetchProvidedTopSites = false, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + shouldFetchTopSitesFromProvider = { false } ) assertEquals(3, topSites.size) @@ -492,8 +515,8 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 5, - fetchProvidedTopSites = false, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + shouldFetchTopSitesFromProvider = { false } ) assertEquals(5, topSites.size) @@ -516,8 +539,8 @@ class DefaultTopSitesStorageTest { topSites = defaultTopSitesStorage.getTopSites( totalSites = 5, - fetchProvidedTopSites = false, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + shouldFetchTopSitesFromProvider = { false } ) assertEquals(5, topSites.size) @@ -582,8 +605,8 @@ class DefaultTopSitesStorageTest { val topSites = defaultTopSitesStorage.getTopSites( totalSites = 5, - fetchProvidedTopSites = false, - frecencyConfig = FrecencyThresholdOption.NONE + frecencyConfig = FrecencyThresholdOption.NONE, + shouldFetchTopSitesFromProvider = { false } ) verify(historyStorage).getTopFrecentSites(5, frecencyThreshold = FrecencyThresholdOption.NONE)