From 39e82861b6e1194661134e710c2e16f26624e0c3 Mon Sep 17 00:00:00 2001 From: Gabriel Luong Date: Tue, 11 Jan 2022 17:59:33 -0500 Subject: [PATCH] For #11483 - Part 2: Convert TopSites into a sealed class --- ...orageTest.kt => PinnedSitesStorageTest.kt} | 21 +++--- .../top/sites/DefaultTopSitesStorage.kt | 5 +- .../components/feature/top/sites/TopSite.kt | 74 ++++++++++++------- .../feature/top/sites/db/PinnedSiteEntity.kt | 30 ++++---- .../top/sites/ext/TopFrecentSiteInfo.kt | 6 +- .../top/sites/DefaultTopSitesStorageTest.kt | 65 +++++++--------- .../top/sites/PinnedSitesStorageTest.kt | 10 +-- .../ext/{TopSiteKtTest.kt => TopSiteTest.kt} | 7 +- docs/changelog.md | 1 + 9 files changed, 110 insertions(+), 109 deletions(-) rename components/feature/top-sites/src/androidTest/java/mozilla/components/feature/top/sites/{OnDevicePinnedSitesStorageTest.kt => PinnedSitesStorageTest.kt} (95%) rename components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/ext/{TopSiteKtTest.kt => TopSiteTest.kt} (91%) diff --git a/components/feature/top-sites/src/androidTest/java/mozilla/components/feature/top/sites/OnDevicePinnedSitesStorageTest.kt b/components/feature/top-sites/src/androidTest/java/mozilla/components/feature/top/sites/PinnedSitesStorageTest.kt similarity index 95% rename from components/feature/top-sites/src/androidTest/java/mozilla/components/feature/top/sites/OnDevicePinnedSitesStorageTest.kt rename to components/feature/top-sites/src/androidTest/java/mozilla/components/feature/top/sites/PinnedSitesStorageTest.kt index 053e44169ce..4ad6bef487e 100644 --- a/components/feature/top-sites/src/androidTest/java/mozilla/components/feature/top/sites/OnDevicePinnedSitesStorageTest.kt +++ b/components/feature/top-sites/src/androidTest/java/mozilla/components/feature/top/sites/PinnedSitesStorageTest.kt @@ -12,13 +12,12 @@ import androidx.sqlite.db.framework.FrameworkSQLiteOpenHelperFactory import androidx.test.core.app.ApplicationProvider import androidx.test.platform.app.InstrumentationRegistry import kotlinx.coroutines.runBlocking -import mozilla.components.feature.top.sites.TopSite.Type.DEFAULT -import mozilla.components.feature.top.sites.TopSite.Type.PINNED import mozilla.components.feature.top.sites.db.Migrations import mozilla.components.feature.top.sites.db.TopSiteDatabase import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Rule import org.junit.Test @@ -28,7 +27,7 @@ import java.util.concurrent.Executors private const val MIGRATION_TEST_DB = "migration-test" @Suppress("LargeClass") -class OnDevicePinnedSitesStorageTest { +class PinnedSitesStorageTest { private lateinit var context: Context private lateinit var storage: PinnedSiteStorage private lateinit var executor: ExecutorService @@ -77,16 +76,16 @@ class OnDevicePinnedSitesStorageTest { assertEquals("Mozilla", topSites[0].title) assertEquals("https://www.mozilla.org", topSites[0].url) - assertEquals(DEFAULT, topSites[0].type) + assertTrue(topSites[0] is TopSite.Default) assertEquals("Firefox", topSites[1].title) assertEquals("https://www.firefox.com", topSites[1].url) - assertEquals(DEFAULT, topSites[2].type) + assertTrue(topSites[1] is TopSite.Default) assertEquals("Wikipedia", topSites[2].title) assertEquals("https://www.wikipedia.com", topSites[2].url) - assertEquals(DEFAULT, topSites[2].type) + assertTrue(topSites[2] is TopSite.Default) assertEquals("Pocket", topSites[3].title) assertEquals("https://www.getpocket.com", topSites[3].url) - assertEquals(DEFAULT, topSites[3].type) + assertTrue(topSites[3] is TopSite.Default) } @Test @@ -101,10 +100,10 @@ class OnDevicePinnedSitesStorageTest { assertEquals("Mozilla", topSites[0].title) assertEquals("https://www.mozilla.org", topSites[0].url) - assertEquals(PINNED, topSites[0].type) + assertTrue(topSites[0] is TopSite.Pinned) assertEquals("Firefox", topSites[1].title) assertEquals("https://www.firefox.com", topSites[1].url) - assertEquals(DEFAULT, topSites[1].type) + assertTrue(topSites[1] is TopSite.Pinned) } @Test @@ -142,13 +141,13 @@ class OnDevicePinnedSitesStorageTest { with(topSites[0]) { assertEquals("Mozilla", title) assertEquals("https://www.mozilla.org", url) - assertEquals(PINNED, type) + assertTrue(this is TopSite.Pinned) } with(topSites[1]) { assertEquals("Firefox", title) assertEquals("https://www.firefox.com", url) - assertEquals(DEFAULT, type) + assertTrue(this is TopSite.Default) } } 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 7c1f7ea2650..923660f5d89 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 @@ -9,7 +9,6 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch import mozilla.components.browser.storage.sync.PlacesHistoryStorage import mozilla.components.concept.storage.FrecencyThresholdOption -import mozilla.components.feature.top.sites.TopSite.Type.FRECENT import mozilla.components.feature.top.sites.ext.hasUrl import mozilla.components.feature.top.sites.ext.toTopSite import mozilla.components.feature.top.sites.facts.emitTopSitesCountFact @@ -58,7 +57,7 @@ class DefaultTopSitesStorage( override fun removeTopSite(topSite: TopSite) { scope.launch { - if (topSite.type != FRECENT) { + if (topSite is TopSite.Default || topSite is TopSite.Pinned) { pinnedSitesStorage.removePinnedSite(topSite) } @@ -72,7 +71,7 @@ class DefaultTopSitesStorage( override fun updateTopSite(topSite: TopSite, title: String, url: String) { scope.launch { - if (topSite.type != FRECENT) { + if (topSite is TopSite.Default || topSite is TopSite.Pinned) { pinnedSitesStorage.updatePinnedSite(topSite, title, url) } diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSite.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSite.kt index fc40c88de45..f04a9267b71 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSite.kt +++ b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSite.kt @@ -6,37 +6,55 @@ package mozilla.components.feature.top.sites /** * A top site. - * - * @property id Unique ID of this top site. - * @property title The title of the top site. - * @property url The URL of the top site. - * @property createdAt The optional date the top site was added. - * @property type The type of a top site. */ -data class TopSite( - val id: Long?, - val title: String?, - val url: String, - val createdAt: Long?, - val type: Type -) { +sealed class TopSite { + abstract val id: Long? + abstract val title: String? + abstract val url: String + abstract val createdAt: Long? + /** - * The type of a [TopSite]. + * This top site was added as a default by the application. + * + * @property id Unique ID of this top site. + * @property title The title of the top site. + * @property url The URL of the top site. + * @property createdAt The optional date the top site was added. */ - enum class Type { - /** - * This top site was added as a default by the application. - */ - DEFAULT, + data class Default( + override val id: Long?, + override val title: String?, + override val url: String, + override val createdAt: Long?, + ) : TopSite() - /** - * This top site was pinned by an user. - */ - PINNED, + /** + * This top site was pinned by an user. + * + * @property id Unique ID of this top site. + * @property title The title of the top site. + * @property url The URL of the top site. + * @property createdAt The optional date the top site was added. + */ + data class Pinned( + override val id: Long?, + override val title: String?, + override val url: String, + override val createdAt: Long?, + ) : TopSite() - /** - * This top site is auto-generated from the history storage based on the most frecent site. - */ - FRECENT - } + /** + * This top site is auto-generated from the history storage based on the most frecent site. + * + * @property id Unique ID of this top site. + * @property title The title of the top site. + * @property url The URL of the top site. + * @property createdAt The optional date the top site was added. + */ + data class Frecent( + override val id: Long?, + override val title: String?, + override val url: String, + override val createdAt: Long?, + ) : TopSite() } diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/db/PinnedSiteEntity.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/db/PinnedSiteEntity.kt index e1e70b662d3..025704e2539 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/db/PinnedSiteEntity.kt +++ b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/db/PinnedSiteEntity.kt @@ -8,8 +8,6 @@ import androidx.room.ColumnInfo import androidx.room.Entity import androidx.room.PrimaryKey import mozilla.components.feature.top.sites.TopSite -import mozilla.components.feature.top.sites.TopSite.Type.DEFAULT -import mozilla.components.feature.top.sites.TopSite.Type.PINNED /** * Internal entity representing a pinned site. @@ -32,16 +30,22 @@ internal data class PinnedSiteEntity( @ColumnInfo(name = "created_at") var createdAt: Long = System.currentTimeMillis() ) { - internal fun toTopSite(): TopSite { - val type = if (isDefault) DEFAULT else PINNED - return TopSite( - id, - title, - url, - createdAt, - type - ) - } + internal fun toTopSite(): TopSite = + if (isDefault) { + TopSite.Default( + id = id, + title = title, + url = url, + createdAt = createdAt + ) + } else { + TopSite.Pinned( + id = id, + title = title, + url = url, + createdAt = createdAt + ) + } } internal fun TopSite.toPinnedSite(): PinnedSiteEntity { @@ -49,7 +53,7 @@ internal fun TopSite.toPinnedSite(): PinnedSiteEntity { id = id, title = title ?: "", url = url, - isDefault = type === DEFAULT, + isDefault = this is TopSite.Default, createdAt = createdAt ?: 0 ) } diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/ext/TopFrecentSiteInfo.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/ext/TopFrecentSiteInfo.kt index cea395ae614..f07d73fd083 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/ext/TopFrecentSiteInfo.kt +++ b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/ext/TopFrecentSiteInfo.kt @@ -6,18 +6,16 @@ package mozilla.components.feature.top.sites.ext import mozilla.components.concept.storage.TopFrecentSiteInfo import mozilla.components.feature.top.sites.TopSite -import mozilla.components.feature.top.sites.TopSite.Type.FRECENT import mozilla.components.support.ktx.kotlin.tryGetHostFromUrl /** * Returns a [TopSite] for the given [TopFrecentSiteInfo]. */ fun TopFrecentSiteInfo.toTopSite(): TopSite { - return TopSite( + return TopSite.Frecent( id = null, title = this.title?.takeIf(String::isNotBlank) ?: this.url.tryGetHostFromUrl(), url = this.url, - createdAt = null, - type = FRECENT + createdAt = null ) } 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 9383e2d9231..40cba459d44 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 @@ -72,35 +72,32 @@ class DefaultTopSitesStorageTest { coroutineContext = coroutineContext ) - val frecentSite = TopSite( + val frecentSite = TopSite.Frecent( id = 1, title = "Mozilla", url = "https://mozilla.com", - createdAt = 1, - type = TopSite.Type.FRECENT + createdAt = 1 ) defaultTopSitesStorage.removeTopSite(frecentSite) verify(historyStorage).deleteVisitsFor(frecentSite.url) - val pinnedSite = TopSite( + val pinnedSite = TopSite.Pinned( id = 2, title = "Firefox", url = "https://firefox.com", - createdAt = 2, - type = TopSite.Type.PINNED + createdAt = 2 ) defaultTopSitesStorage.removeTopSite(pinnedSite) verify(pinnedSitesStorage).removePinnedSite(pinnedSite) verify(historyStorage).deleteVisitsFor(pinnedSite.url) - val defaultSite = TopSite( + val defaultSite = TopSite.Default( id = 3, title = "Wikipedia", url = "https://wikipedia.com", - createdAt = 3, - type = TopSite.Type.DEFAULT + createdAt = 3 ) defaultTopSitesStorage.removeTopSite(defaultSite) @@ -117,34 +114,31 @@ class DefaultTopSitesStorageTest { coroutineContext = coroutineContext ) - val defaultSite = TopSite( + val defaultSite = TopSite.Default( id = 1, title = "Firefox", url = "https://firefox.com", - createdAt = 1, - type = TopSite.Type.DEFAULT + createdAt = 1 ) defaultTopSitesStorage.updateTopSite(defaultSite, "Mozilla Firefox", "https://mozilla.com") verify(pinnedSitesStorage).updatePinnedSite(defaultSite, "Mozilla Firefox", "https://mozilla.com") - val pinnedSite = TopSite( + val pinnedSite = TopSite.Pinned( id = 2, title = "Wikipedia", url = "https://wikipedia.com", - createdAt = 2, - type = TopSite.Type.PINNED + createdAt = 2 ) defaultTopSitesStorage.updateTopSite(pinnedSite, "Wiki", "https://en.wikipedia.org/wiki/Wiki") verify(pinnedSitesStorage).updatePinnedSite(pinnedSite, "Wiki", "https://en.wikipedia.org/wiki/Wiki") - val frecentSite = TopSite( + val frecentSite = TopSite.Frecent( id = 1, title = "Mozilla", url = "https://mozilla.com", - createdAt = 1, - type = TopSite.Type.FRECENT + createdAt = 1 ) defaultTopSitesStorage.updateTopSite(frecentSite, "Moz", "") @@ -160,19 +154,17 @@ class DefaultTopSitesStorageTest { coroutineContext = coroutineContext ) - val defaultSite = TopSite( + val defaultSite = TopSite.Default( id = 1, title = "Firefox", url = "https://firefox.com", - createdAt = 1, - type = TopSite.Type.DEFAULT + createdAt = 1 ) - val pinnedSite = TopSite( + val pinnedSite = TopSite.Pinned( id = 2, title = "Wikipedia", url = "https://wikipedia.com", - createdAt = 2, - type = TopSite.Type.PINNED + createdAt = 2 ) whenever(pinnedSitesStorage.getPinnedSites()).thenReturn( listOf( @@ -213,19 +205,17 @@ class DefaultTopSitesStorageTest { coroutineContext = coroutineContext ) - val defaultSite = TopSite( + val defaultSite = TopSite.Default( id = 1, title = "Firefox", url = "https://firefox.com", - createdAt = 1, - type = TopSite.Type.DEFAULT + createdAt = 1 ) - val pinnedSite = TopSite( + val pinnedSite = TopSite.Pinned( id = 2, title = "Wikipedia", url = "https://wikipedia.com", - createdAt = 2, - type = TopSite.Type.PINNED + createdAt = 2 ) whenever(pinnedSitesStorage.getPinnedSites()).thenReturn( listOf( @@ -307,26 +297,23 @@ class DefaultTopSitesStorageTest { coroutineContext = coroutineContext ) - val defaultSiteFirefox = TopSite( + val defaultSiteFirefox = TopSite.Default( id = 1, title = "Firefox", url = "https://firefox.com", - createdAt = 1, - type = TopSite.Type.DEFAULT + createdAt = 1 ) - val pinnedSite1 = TopSite( + val pinnedSite1 = TopSite.Pinned( id = 2, title = "Wikipedia", url = "https://wikipedia.com", - createdAt = 2, - type = TopSite.Type.PINNED + createdAt = 2 ) - val pinnedSite2 = TopSite( + val pinnedSite2 = TopSite.Pinned( id = 3, title = "Example", url = "https://example.com", - createdAt = 3, - type = TopSite.Type.PINNED + createdAt = 3 ) whenever(pinnedSitesStorage.getPinnedSites()).thenReturn( listOf( diff --git a/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/PinnedSitesStorageTest.kt b/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/PinnedSitesStorageTest.kt index ad1b6527995..d0bdd52faf5 100644 --- a/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/PinnedSitesStorageTest.kt +++ b/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/PinnedSitesStorageTest.kt @@ -5,8 +5,6 @@ package mozilla.components.feature.top.sites import kotlinx.coroutines.runBlocking -import mozilla.components.feature.top.sites.TopSite.Type.DEFAULT -import mozilla.components.feature.top.sites.TopSite.Type.PINNED import mozilla.components.feature.top.sites.db.PinnedSiteDao import mozilla.components.feature.top.sites.db.PinnedSiteEntity import mozilla.components.feature.top.sites.db.TopSiteDatabase @@ -72,8 +70,8 @@ class PinnedSitesStorageTest { val storage = PinnedSiteStorage(mock()) val dao = mockDao(storage) - storage.removePinnedSite(TopSite(1, "Mozilla", "https://www.mozilla.org", 1, PINNED)) - storage.removePinnedSite(TopSite(2, "Firefox", "https://www.firefox.com", 1, DEFAULT)) + storage.removePinnedSite(TopSite.Pinned(1, "Mozilla", "https://www.mozilla.org", 1)) + storage.removePinnedSite(TopSite.Default(2, "Firefox", "https://www.firefox.com", 1)) verify(dao).deletePinnedSite(PinnedSiteEntity(1, "Mozilla", "https://www.mozilla.org", false, 1)) verify(dao).deletePinnedSite(PinnedSiteEntity(2, "Firefox", "https://www.firefox.com", true, 1)) @@ -103,7 +101,6 @@ class PinnedSitesStorageTest { assertEquals(1L, id) assertEquals("Mozilla", title) assertEquals("https://www.mozilla.org", url) - assertEquals(PINNED, type) assertEquals(10L, createdAt) } @@ -111,7 +108,6 @@ class PinnedSitesStorageTest { assertEquals(2L, id) assertEquals("Firefox", title) assertEquals("https://www.firefox.com", url) - assertEquals(DEFAULT, type) assertEquals(10L, createdAt) } } @@ -121,7 +117,7 @@ class PinnedSitesStorageTest { val storage = PinnedSiteStorage(mock()) val dao = mockDao(storage) - val site = TopSite(1, "Mozilla", "https://www.mozilla.org", 1, PINNED) + val site = TopSite.Pinned(1, "Mozilla", "https://www.mozilla.org", 1) storage.updatePinnedSite(site, "Mozilla (IT)", "https://www.mozilla.org/it") verify(dao).updatePinnedSite(PinnedSiteEntity(1, "Mozilla (IT)", "https://www.mozilla.org/it", false, 1)) diff --git a/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/ext/TopSiteKtTest.kt b/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/ext/TopSiteTest.kt similarity index 91% rename from components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/ext/TopSiteKtTest.kt rename to components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/ext/TopSiteTest.kt index 35a0ad11794..7ce410db444 100644 --- a/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/ext/TopSiteKtTest.kt +++ b/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/ext/TopSiteTest.kt @@ -9,17 +9,16 @@ import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Test -class TopSiteKtTest { +class TopSiteTest { @Test fun hasUrl() { val topSites = listOf( - TopSite( + TopSite.Frecent( id = 1, title = "Mozilla", url = "https://mozilla.com", - createdAt = 1, - type = TopSite.Type.FRECENT + createdAt = 1 ) ) diff --git a/docs/changelog.md b/docs/changelog.md index a000ca419bd..c7a489b767d 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -15,6 +15,7 @@ permalink: /changelog/ * ⚠️ **This is a breaking change**: Removed `String.urlToTrimmedHost` extension method. * **feature-top-sites** + * ⚠️ **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) # 97.0.0