From 37a0edceb698dad8f75cfff71e612dad7aaf3459 Mon Sep 17 00:00:00 2001 From: Gabriel Luong Date: Mon, 7 Feb 2022 17:11:37 -0500 Subject: [PATCH] For #23431 - Display the order of Contile Top Sites correctly --- .../org/mozilla/fenix/FenixApplication.kt | 9 +- .../java/org/mozilla/fenix/ext/TopSite.kt | 20 +++ .../org/mozilla/fenix/home/HomeFragment.kt | 19 ++- .../home/topsites/DefaultTopSitesView.kt | 15 ++- .../java/org/mozilla/fenix/utils/Settings.kt | 12 +- .../java/org/mozilla/fenix/ext/TopSiteTest.kt | 117 ++++++++++++++++++ buildSrc/src/main/java/AndroidComponents.kt | 2 +- 7 files changed, 182 insertions(+), 12 deletions(-) create mode 100644 app/src/test/java/org/mozilla/fenix/ext/TopSiteTest.kt diff --git a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt index bf24bb65e..205a57085 100644 --- a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt +++ b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt @@ -36,6 +36,7 @@ import mozilla.components.feature.addons.update.GlobalAddonDependencyProvider import mozilla.components.feature.autofill.AutofillUseCases import mozilla.components.feature.search.ext.buildSearchUrl import mozilla.components.feature.search.ext.waitForSelectedOrDefaultSearchEngine +import mozilla.components.feature.top.sites.TopSitesProviderConfig import mozilla.components.lib.crash.CrashReporter import mozilla.components.service.fxa.manager.SyncEnginesStorage import mozilla.components.service.glean.Glean @@ -82,6 +83,7 @@ import org.mozilla.fenix.session.VisibilityLifecycleCallback import org.mozilla.fenix.telemetry.TelemetryLifecycleObserver import org.mozilla.fenix.utils.BrowsersCache import org.mozilla.fenix.utils.Settings +import org.mozilla.fenix.utils.Settings.Companion.TOP_SITES_PROVIDER_MAX_THRESHOLD import java.util.concurrent.TimeUnit /** @@ -258,11 +260,14 @@ open class FenixApplication : LocaleAwareApplication(), Provider { // we can prevent with this. components.core.topSitesStorage.getTopSites( totalSites = components.settings.topSitesMaxLimit, - fetchProvidedTopSites = components.settings.showContileFeature, frecencyConfig = if (components.settings.showTopFrecentSites) FrecencyThresholdOption.SKIP_ONE_TIME_PAGES else - null + null, + providerConfig = TopSitesProviderConfig( + showProviderTopSites = components.settings.showContileFeature, + maxThreshold = TOP_SITES_PROVIDER_MAX_THRESHOLD + ) ) // This service uses `historyStorage`, and so we can only touch it when we know diff --git a/app/src/main/java/org/mozilla/fenix/ext/TopSite.kt b/app/src/main/java/org/mozilla/fenix/ext/TopSite.kt index 3bda9a589..3af6a701f 100644 --- a/app/src/main/java/org/mozilla/fenix/ext/TopSite.kt +++ b/app/src/main/java/org/mozilla/fenix/ext/TopSite.kt @@ -5,6 +5,7 @@ package org.mozilla.fenix.ext import mozilla.components.feature.top.sites.TopSite +import org.mozilla.fenix.settings.SupportUtils /** * Returns the type name of the [TopSite]. @@ -15,3 +16,22 @@ fun TopSite.name(): String = when (this) { is TopSite.Pinned -> "PINNED" is TopSite.Provided -> "PROVIDED" } + +/** + * Returns a sorted list of [TopSite] with the default Google top site always appearing + * as the first item. + */ +fun List.sort(): List { + val defaultGoogleTopSiteIndex = this.indexOfFirst { + it is TopSite.Default && it.url == SupportUtils.GOOGLE_URL + } + + return if (defaultGoogleTopSiteIndex == -1) { + this + } else { + val result = this.toMutableList() + result.removeAt(defaultGoogleTopSiteIndex) + result.add(0, this[defaultGoogleTopSiteIndex]) + result + } +} diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt index d609cc265..02fbcad88 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -66,6 +66,7 @@ import mozilla.components.concept.sync.OAuthAccount import mozilla.components.feature.tab.collections.TabCollection import mozilla.components.feature.top.sites.TopSitesConfig import mozilla.components.feature.top.sites.TopSitesFeature +import mozilla.components.feature.top.sites.TopSitesProviderConfig import mozilla.components.lib.state.ext.consumeFlow import mozilla.components.lib.state.ext.consumeFrom import mozilla.components.support.base.feature.ViewBoundFeatureWrapper @@ -102,6 +103,7 @@ import org.mozilla.fenix.ext.nav import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.ext.runIfFragmentIsAttached import org.mozilla.fenix.ext.settings +import org.mozilla.fenix.ext.sort import org.mozilla.fenix.home.mozonline.showPrivacyPopWindow import org.mozilla.fenix.home.pocket.DefaultPocketStoriesController import org.mozilla.fenix.home.pocket.PocketRecommendedStoriesCategory @@ -124,6 +126,7 @@ import org.mozilla.fenix.settings.SupportUtils import org.mozilla.fenix.settings.SupportUtils.SumoTopic.HELP import org.mozilla.fenix.settings.deletebrowsingdata.deleteAndQuit import org.mozilla.fenix.theme.ThemeManager +import org.mozilla.fenix.utils.Settings.Companion.TOP_SITES_PROVIDER_MAX_THRESHOLD import org.mozilla.fenix.utils.ToolbarPopupWindow import org.mozilla.fenix.utils.allowUndo import org.mozilla.fenix.wallpapers.WallpaperManager @@ -237,7 +240,7 @@ class HomeFragment : Fragment() { collections = components.core.tabCollectionStorage.cachedTabCollections, expandedCollections = emptySet(), mode = currentMode.getCurrentMode(), - topSites = components.core.topSitesStorage.cachedTopSites, + topSites = components.core.topSitesStorage.cachedTopSites.sort(), tip = components.strictMode.resetAfter(StrictMode.allowThreadDiskReads()) { FenixTipManager( listOf( @@ -282,7 +285,10 @@ class HomeFragment : Fragment() { topSitesFeature.set( feature = TopSitesFeature( - view = DefaultTopSitesView(homeFragmentStore), + view = DefaultTopSitesView( + store = homeFragmentStore, + settings = components.settings + ), storage = components.core.topSitesStorage, config = ::getTopSitesConfig ), @@ -421,8 +427,11 @@ class HomeFragment : Fragment() { val settings = requireContext().settings() return TopSitesConfig( totalSites = settings.topSitesMaxLimit, - fetchProvidedTopSites = settings.showContileFeature, - frecencyConfig = if (settings.showTopFrecentSites) FrecencyThresholdOption.SKIP_ONE_TIME_PAGES else null + frecencyConfig = if (settings.showTopFrecentSites) FrecencyThresholdOption.SKIP_ONE_TIME_PAGES else null, + providerConfig = TopSitesProviderConfig( + showProviderTopSites = settings.showContileFeature, + maxThreshold = TOP_SITES_PROVIDER_MAX_THRESHOLD + ) ) } @@ -690,7 +699,7 @@ class HomeFragment : Fragment() { HomeFragmentAction.Change( collections = components.core.tabCollectionStorage.cachedTabCollections, mode = currentMode.getCurrentMode(), - topSites = components.core.topSitesStorage.cachedTopSites, + topSites = components.core.topSitesStorage.cachedTopSites.sort(), tip = components.strictMode.resetAfter(StrictMode.allowThreadDiskReads()) { FenixTipManager( listOf( diff --git a/app/src/main/java/org/mozilla/fenix/home/topsites/DefaultTopSitesView.kt b/app/src/main/java/org/mozilla/fenix/home/topsites/DefaultTopSitesView.kt index d850cce8f..0db7557c9 100644 --- a/app/src/main/java/org/mozilla/fenix/home/topsites/DefaultTopSitesView.kt +++ b/app/src/main/java/org/mozilla/fenix/home/topsites/DefaultTopSitesView.kt @@ -6,14 +6,25 @@ package org.mozilla.fenix.home.topsites import mozilla.components.feature.top.sites.TopSite import mozilla.components.feature.top.sites.view.TopSitesView +import org.mozilla.fenix.ext.sort import org.mozilla.fenix.home.HomeFragmentAction import org.mozilla.fenix.home.HomeFragmentStore +import org.mozilla.fenix.utils.Settings class DefaultTopSitesView( - val store: HomeFragmentStore + val store: HomeFragmentStore, + val settings: Settings ) : TopSitesView { override fun displayTopSites(topSites: List) { - store.dispatch(HomeFragmentAction.TopSitesChange(topSites)) + store.dispatch( + HomeFragmentAction.TopSitesChange( + if (!settings.showContileFeature) { + topSites + } else { + topSites.sort() + } + ) + ) } } diff --git a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt index 048dbf674..20f830f76 100644 --- a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt +++ b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt @@ -18,6 +18,7 @@ import androidx.lifecycle.LifecycleOwner import mozilla.components.feature.sitepermissions.SitePermissionsRules import mozilla.components.feature.sitepermissions.SitePermissionsRules.Action import mozilla.components.feature.sitepermissions.SitePermissionsRules.AutoplayAction +import mozilla.components.service.contile.ContileTopSitesProvider import mozilla.components.support.ktx.android.content.PreferencesHolder import mozilla.components.support.ktx.android.content.booleanPreference import mozilla.components.support.ktx.android.content.floatPreference @@ -61,7 +62,6 @@ private const val AUTOPLAY_USER_SETTING = "AUTOPLAY_USER_SETTING" class Settings(private val appContext: Context) : PreferencesHolder { companion object { - const val topSitesMaxCount = 16 const val FENIX_PREFERENCES = "fenix_preferences" private const val BLOCKED_INT = 0 @@ -84,6 +84,14 @@ class Settings(private val appContext: Context) : PreferencesHolder { */ const val SEARCH_GROUP_MINIMUM_SITES: Int = 2 + // The maximum number of top sites to display. + const val TOP_SITES_MAX_COUNT = 16 + /** + * Only fetch top sites from the [ContileTopSitesProvider] when the number of default and + * pinned sites are below this maximum threshold. + */ + const val TOP_SITES_PROVIDER_MAX_THRESHOLD = 8 + private fun Action.toInt() = when (this) { Action.BLOCKED -> BLOCKED_INT Action.ASK_TO_ALLOW -> ASK_TO_ALLOW_INT @@ -1122,7 +1130,7 @@ class Settings(private val appContext: Context) : PreferencesHolder { val topSitesMaxLimit by intPreference( appContext.getPreferenceKey(R.string.pref_key_top_sites_max_limit), - default = topSitesMaxCount + default = TOP_SITES_MAX_COUNT ) var openTabsCount by intPreference( diff --git a/app/src/test/java/org/mozilla/fenix/ext/TopSiteTest.kt b/app/src/test/java/org/mozilla/fenix/ext/TopSiteTest.kt new file mode 100644 index 000000000..41bc34fa4 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/ext/TopSiteTest.kt @@ -0,0 +1,117 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.ext + +import mozilla.components.feature.top.sites.TopSite +import org.junit.Assert.assertEquals +import org.junit.Test +import org.junit.runner.RunWith +import org.mozilla.fenix.helpers.FenixRobolectricTestRunner +import org.mozilla.fenix.settings.SupportUtils + +@RunWith(FenixRobolectricTestRunner::class) +class TopSiteTest { + + val defaultGoogleTopSite = TopSite.Default( + id = 1L, + title = "Google", + url = SupportUtils.GOOGLE_URL, + createdAt = 0 + ) + val providedSite1 = TopSite.Provided( + id = 3, + title = "Mozilla", + url = "https://mozilla.com", + clickUrl = "https://mozilla.com/click", + imageUrl = "https://test.com/image2.jpg", + impressionUrl = "https://example.com", + createdAt = 3 + ) + val providedSite2 = TopSite.Provided( + id = 3, + title = "Firefox", + url = "https://firefox.com", + clickUrl = "https://firefox.com/click", + imageUrl = "https://test.com/image2.jpg", + impressionUrl = "https://example.com", + createdAt = 3 + ) + val pinnedSite1 = TopSite.Pinned( + id = 1L, + title = "DuckDuckGo", + url = "https://duckduckgo.com", + createdAt = 0 + ) + val pinnedSite2 = TopSite.Pinned( + id = 1L, + title = "Mozilla", + url = "mozilla.org", + createdAt = 0 + ) + val frecentSite = TopSite.Frecent( + id = 1L, + title = "Mozilla", + url = "mozilla.org", + createdAt = 0 + ) + + @Test + fun `GIVEN the default Google top site is the first item WHEN the list of top sites is sorted THEN the order doesn't change`() { + val topSites = listOf( + defaultGoogleTopSite, + providedSite1, + providedSite2, + pinnedSite1, + pinnedSite2, + frecentSite + ) + + assertEquals(topSites.sort(), topSites) + } + + @Test + fun `GIVEN the default Google top site is after the provided top sites WHEN the list of top sites is sorted THEN the default Google top site should be first`() { + val topSites = listOf( + providedSite1, + providedSite2, + defaultGoogleTopSite, + pinnedSite1, + pinnedSite2, + frecentSite + ) + val expected = listOf( + defaultGoogleTopSite, + providedSite1, + providedSite2, + pinnedSite1, + pinnedSite2, + frecentSite + ) + + assertEquals(topSites.sort(), expected) + } + + @Test + fun `GIVEN the default Google top site is the last item WHEN the list of top sites is sorted THEN the default Google top site should be first`() { + val topSites = listOf( + providedSite1, + providedSite2, + pinnedSite1, + pinnedSite2, + frecentSite, + defaultGoogleTopSite + ) + val expected = listOf( + defaultGoogleTopSite, + providedSite1, + providedSite2, + pinnedSite1, + pinnedSite2, + frecentSite + ) + + assertEquals(topSites.sort(), expected) + } +} diff --git a/buildSrc/src/main/java/AndroidComponents.kt b/buildSrc/src/main/java/AndroidComponents.kt index 5c728fc15..d6749f024 100644 --- a/buildSrc/src/main/java/AndroidComponents.kt +++ b/buildSrc/src/main/java/AndroidComponents.kt @@ -3,5 +3,5 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ object AndroidComponents { - const val VERSION = "99.0.20220209143350" + const val VERSION = "99.0.20220209170927" }