From bae2a066931d4ed6283eca1a1332deba1f89740e Mon Sep 17 00:00:00 2001 From: Thore Goebel Date: Sun, 14 Apr 2024 15:05:25 +0200 Subject: [PATCH] Refactor: Move fetchUrl up from FetchResult into Fetching The fetchUrl is independent of the result, it better belongs into Fetching. --- .../fdroid/views/repos/RepoPreviewScreen.kt | 18 ++++++--- .../main/java/org/fdroid/repo/RepoAdder.kt | 40 ++++++++++--------- .../java/org/fdroid/repo/RepoAdderTest.kt | 6 +-- 3 files changed, 36 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/views/repos/RepoPreviewScreen.kt b/app/src/main/java/org/fdroid/fdroid/views/repos/RepoPreviewScreen.kt index f5a9026e4..6ed72e85e 100644 --- a/app/src/main/java/org/fdroid/fdroid/views/repos/RepoPreviewScreen.kt +++ b/app/src/main/java/org/fdroid/fdroid/views/repos/RepoPreviewScreen.kt @@ -197,7 +197,8 @@ fun LazyItemScope.RepoPreviewApp( @Preview @Composable fun RepoPreviewScreenFetchingPreview() { - val repo = FDroidApp.createSwapRepo("https://example.org", "foo bar") + val address = "https://example.org" + val repo = FDroidApp.createSwapRepo(address, "foo bar") val app1 = object : MinimalApp { override val repoId = 0L override val packageName = "org.example" @@ -225,7 +226,7 @@ fun RepoPreviewScreenFetchingPreview() { FDroidContent { RepoPreviewScreen( PaddingValues(0.dp), - Fetching(repo, listOf(app1, app2, app3), IsNewRepository("foo")) + Fetching(address, repo, listOf(app1, app2, app3), IsNewRepository) ) {} } } @@ -233,11 +234,12 @@ fun RepoPreviewScreenFetchingPreview() { @Composable @Preview(uiMode = Configuration.UI_MODE_NIGHT_YES, widthDp = 720, heightDp = 360) fun RepoPreviewScreenNewMirrorPreview() { - val repo = FDroidApp.createSwapRepo("https://example.org", "foo bar") + val address = "https://example.org" + val repo = FDroidApp.createSwapRepo(address, "foo bar") FDroidContent { RepoPreviewScreen( PaddingValues(0.dp), - Fetching(repo, emptyList(), IsNewMirror(0L, "foo")) + Fetching(address, repo, emptyList(), IsNewMirror(0L)) ) {} } } @@ -245,8 +247,12 @@ fun RepoPreviewScreenNewMirrorPreview() { @Preview @Composable fun RepoPreviewScreenExistingRepoPreview() { - val repo = FDroidApp.createSwapRepo("https://example.org", "foo bar") + val address = "https://example.org" + val repo = FDroidApp.createSwapRepo(address, "foo bar") FDroidContent { - RepoPreviewScreen(PaddingValues(0.dp), Fetching(repo, emptyList(), IsExistingRepository)) {} + RepoPreviewScreen( + PaddingValues(0.dp), + Fetching(address, repo, emptyList(), IsExistingRepository) + ) {} } } diff --git a/libs/database/src/main/java/org/fdroid/repo/RepoAdder.kt b/libs/database/src/main/java/org/fdroid/repo/RepoAdder.kt index 812ea0b56..5a0b000ed 100644 --- a/libs/database/src/main/java/org/fdroid/repo/RepoAdder.kt +++ b/libs/database/src/main/java/org/fdroid/repo/RepoAdder.kt @@ -50,6 +50,7 @@ public sealed class AddRepoState public object None : AddRepoState() public class Fetching( + public val fetchUrl: String, public val repo: Repository?, public val apps: List, public val fetchResult: FetchResult?, @@ -65,8 +66,8 @@ public class Fetching( (fetchResult != null && fetchResult !is FetchResult.IsExistingRepository) override fun toString(): String { - return "Fetching(repo=${repo?.address}, apps=${apps.size}, fetchResult=$fetchResult, " + - "done=$done, canAdd=$canAdd)" + return "Fetching(fetchUrl=$fetchUrl, repo=${repo?.address}, apps=${apps.size}, " + + "fetchResult=$fetchResult, done=$done, canAdd=$canAdd)" } } @@ -90,13 +91,10 @@ public data class AddRepoError( } public sealed class FetchResult { - public data class IsNewRepository(internal val addUrl: String) : FetchResult() - public data class IsNewMirror( - internal val existingRepoId: Long, - internal val newMirrorUrl: String, - ) : FetchResult() + public data object IsNewRepository : FetchResult() + public data class IsNewMirror(internal val existingRepoId: Long) : FetchResult() - public object IsExistingRepository : FetchResult() + public data object IsExistingRepository : FetchResult() } @OptIn(DelicateCoroutinesApi::class) @@ -147,11 +145,13 @@ internal class RepoAdder( addRepoState.value = AddRepoError(IS_ARCHIVE_REPO) return } + val fetchUrl = nUri.uri.toString() // some plumping to receive the repo preview var receivedRepo: Repository? = null val apps = ArrayList() var fetchResult: FetchResult? = null + val receiver = object : RepoPreviewReceiver { override fun onRepoReceived(repo: Repository) { receivedRepo = repo @@ -161,17 +161,17 @@ internal class RepoAdder( "Known fingerprint different from given one: ${repo.fingerprint}" ) } - fetchResult = getFetchResult(nUri.uri.toString(), repo) - addRepoState.value = Fetching(receivedRepo, apps.toList(), fetchResult) + fetchResult = getFetchResult(fetchUrl, repo) + addRepoState.value = Fetching(fetchUrl, receivedRepo, apps.toList(), fetchResult) } override fun onAppReceived(app: AppOverviewItem) { apps.add(app) - addRepoState.value = Fetching(receivedRepo, apps.toList(), fetchResult) + addRepoState.value = Fetching(fetchUrl, receivedRepo, apps.toList(), fetchResult) } } // set a state early, so the ui can show progress animation - addRepoState.value = Fetching(receivedRepo, apps, fetchResult) + addRepoState.value = Fetching(fetchUrl, receivedRepo, apps, fetchResult) // try fetching repo with v2 format first and fallback to v1 try { @@ -198,7 +198,7 @@ internal class RepoAdder( if (finalRepo == null) { addRepoState.value = AddRepoError(INVALID_INDEX) } else { - addRepoState.value = Fetching(finalRepo, apps, fetchResult, done = true) + addRepoState.value = Fetching(fetchUrl, finalRepo, apps, fetchResult, done = true) } } @@ -230,8 +230,9 @@ internal class RepoAdder( private fun getFetchResult(url: String, repo: Repository): FetchResult { val cert = repo.certificate ?: error("Certificate was null") val existingRepo = repositoryDao.getRepository(cert) + return if (existingRepo == null) { - FetchResult.IsNewRepository(url) + FetchResult.IsNewRepository } else { val existingMirror = if (existingRepo.address.trimEnd('/') == url) { url @@ -240,7 +241,7 @@ internal class RepoAdder( ?: existingRepo.userMirrors.find { it.trimEnd('/') == url } } if (existingMirror == null) { - FetchResult.IsNewMirror(existingRepo.repoId, url) + FetchResult.IsNewMirror(existingRepo.repoId) } else { FetchResult.IsExistingRepository } @@ -281,10 +282,10 @@ internal class RepoAdder( db.runInTransaction { val repoId = repositoryDao.insert(newRepo) // add user mirror, if URL is not the repo address and not a known mirror - if (fetchResult.addUrl != repo.address.trimEnd('/') && - repo.mirrors.find { fetchResult.addUrl == it.url.trimEnd('/') } == null + if (state.fetchUrl != repo.address.trimEnd('/') && + repo.mirrors.find {state.fetchUrl == it.url.trimEnd('/') } == null ) { - val userMirrors = listOf(fetchResult.addUrl) + val userMirrors = listOf(state.fetchUrl) repositoryDao.updateUserMirrors(repoId, userMirrors) } repositoryDao.getRepository(repoId) ?: error("New repository not found in DB") @@ -297,7 +298,7 @@ internal class RepoAdder( val existingRepo = repositoryDao.getRepository(repoId) ?: error("No repo with $repoId") val userMirrors = existingRepo.userMirrors.toMutableList().apply { - add(fetchResult.newMirrorUrl) + add(state.fetchUrl) } repositoryDao.updateUserMirrors(repoId, userMirrors) existingRepo @@ -319,6 +320,7 @@ internal class RepoAdder( if (repo.isArchiveRepo) error { "Repo ${repo.address} is already an archive repo." } val address = repo.address.replace(Regex("repo/?$"), "archive") + @Suppress("PARAMETER_NAME_CHANGED_ON_OVERRIDE") val receiver = object : RepoPreviewReceiver { override fun onRepoReceived(archiveRepo: Repository) { diff --git a/libs/database/src/test/java/org/fdroid/repo/RepoAdderTest.kt b/libs/database/src/test/java/org/fdroid/repo/RepoAdderTest.kt index eb58d8864..ae1c61fff 100644 --- a/libs/database/src/test/java/org/fdroid/repo/RepoAdderTest.kt +++ b/libs/database/src/test/java/org/fdroid/repo/RepoAdderTest.kt @@ -144,7 +144,7 @@ internal class RepoAdderTest { // repo not in DB every { repoDao.getRepository(any()) } returns null - expectMinRepoPreview(repoName, FetchResult.IsNewRepository(urlTrimmed)) { + expectMinRepoPreview(repoName, FetchResult.IsNewRepository) { repoAdder.fetchRepository(url = url, proxy = null) } @@ -206,7 +206,7 @@ internal class RepoAdderTest { ) every { repoDao.getRepository(any()) } returns existingRepo - expectMinRepoPreview(repoName, FetchResult.IsNewMirror(42L, url.trimEnd('/'))) { + expectMinRepoPreview(repoName, FetchResult.IsNewMirror(42L)) { repoAdder.fetchRepository(url = url, proxy = null) } @@ -783,7 +783,7 @@ internal class RepoAdderTest { // repo not in DB every { repoDao.getRepository(any()) } returns null - expectMinRepoPreview(repoName, FetchResult.IsNewRepository(urlTrimmed)) { + expectMinRepoPreview(repoName, FetchResult.IsNewRepository) { repoAdder.fetchRepository(url = url, proxy = null) }