From 12b478766536b4f7428e05c169b635653ac1ae7e Mon Sep 17 00:00:00 2001 From: Simon Greuter <6537188+greuters@users.noreply.github.com> Date: Sat, 29 Jun 2024 03:32:54 -0700 Subject: [PATCH] Complete refactoring of GpxImport backend code Refactored to use Flows and moving tests that need XmlPullParser to androidTest to get rid of robolectric --- app/build.gradle.kts | 4 +- .../data/import/GpxImportParseTest.kt | 102 +++-- .../data/import/GpxImportTest.kt | 199 +++++----- .../streetcomplete/data/import/TestHelpers.kt | 16 +- .../streetcomplete/data/import/GpxImport.kt | 361 ++++++++---------- .../data/import/GpxImportParse.kt | 16 +- .../screens/main/MainFragment.kt | 92 ++++- .../controls/GpxImportConfirmationDialog.kt | 16 +- .../main/controls/GpxImportSettingsDialog.kt | 47 +-- .../GpxImportAddInterpolatedPointsTest.kt | 32 +- .../import/GpxImportDetermineBBoxesTest.kt | 24 +- .../GpxImportDiscardRedundantPointsTest.kt | 40 +- .../GpxImportMapToCenteredSquaresTest.kt | 16 +- .../data/import/GpxImportMergeBBoxesTest.kt | 20 +- app/test-proguard-rules.pro | 9 + 15 files changed, 541 insertions(+), 453 deletions(-) rename app/src/{test => androidTest}/java/de/westnordost/streetcomplete/data/import/GpxImportParseTest.kt (64%) rename app/src/{test => androidTest}/java/de/westnordost/streetcomplete/data/import/GpxImportTest.kt (68%) rename app/src/{test => androidTest}/java/de/westnordost/streetcomplete/data/import/TestHelpers.kt (62%) create mode 100644 app/test-proguard-rules.pro diff --git a/app/build.gradle.kts b/app/build.gradle.kts index bb4617e2262..2ad5aa452dd 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -45,6 +45,7 @@ android { isShrinkResources = false // don't use proguard-android-optimize.txt, it is too aggressive, it is more trouble than it is worth proguardFiles(getDefaultProguardFile("proguard-android.txt"), "proguard-rules.pro") + testProguardFile("test-proguard-rules.pro") } getByName("release") { signingConfig = signingConfigs.getByName("release") @@ -116,12 +117,13 @@ dependencies { testImplementation("org.mockito:mockito-inline:$mockitoVersion") testImplementation("org.assertj:assertj-core:3.23.1") testImplementation(kotlin("test")) - testImplementation("org.robolectric:robolectric:4.12.2") + testImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-test:1.7.1") androidTestImplementation("androidx.test:runner:1.5.2") androidTestImplementation("androidx.test:rules:1.5.0") androidTestImplementation("org.mockito:mockito-android:$mockitoVersion") androidTestImplementation("org.assertj:assertj-core:3.23.1") + androidTestImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-test:1.7.1") androidTestImplementation(kotlin("test")) // dependency injection diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportParseTest.kt b/app/src/androidTest/java/de/westnordost/streetcomplete/data/import/GpxImportParseTest.kt similarity index 64% rename from app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportParseTest.kt rename to app/src/androidTest/java/de/westnordost/streetcomplete/data/import/GpxImportParseTest.kt index d5e9f392403..36875a36116 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportParseTest.kt +++ b/app/src/androidTest/java/de/westnordost/streetcomplete/data/import/GpxImportParseTest.kt @@ -1,16 +1,15 @@ package de.westnordost.streetcomplete.data.import import de.westnordost.streetcomplete.data.osm.mapdata.LatLon -import org.junit.runner.RunWith -import org.robolectric.RobolectricTestRunner +import kotlinx.coroutines.flow.toList +import kotlinx.coroutines.runBlocking import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFails -@RunWith(RobolectricTestRunner::class) class GpxImportParseTest { @Test - fun `successfully parses minimal sample track`() { + fun successfullyParsesMinimalSampleTrack() = runBlocking { val originalTrackPoints = arrayListOf( TrackPoint("22.22", "172.3"), TrackPoint("39.11111", "-179.999"), @@ -20,13 +19,17 @@ class GpxImportParseTest { TrackPoint("-72.0", "0.3"), ) - val inputGpx = minimalGpxBuilder(originalTrackPoints) + val inputGpx = + minimalGpxBuilder(originalTrackPoints) - assertSuccess(originalTrackPoints, parseGpx(inputGpx)) + assertSuccess( + originalTrackPoints, + parseGpx(inputGpx) + ) } @Test - fun `concatenates multiple track segments`() { + fun concatenatesMultipleTrackSegments() = runBlocking { val trackPointsSegment1 = arrayListOf( TrackPoint("-56.0", "0.0"), TrackPoint("57.57", "172.3") @@ -53,11 +56,14 @@ class GpxImportParseTest { append("") } - assertSuccess(trackPointsSegment1 + trackPointsSegment2, parseGpx(inputGpx)) + assertSuccess( + trackPointsSegment1 + trackPointsSegment2, + parseGpx(inputGpx) + ) } @Test - fun `process multiple tracks and segments`() { + fun processesMultipleTracksAndSegments() = runBlocking { val trackPoints1 = arrayListOf( TrackPoint("-12.33", "0.0"), TrackPoint("74.1", "-122.34") @@ -95,21 +101,32 @@ class GpxImportParseTest { append("") } - assertSuccess(trackPoints1 + trackPoints2 + trackPoints3, parseGpx(inputGpx)) + assertSuccess( + trackPoints1 + trackPoints2 + trackPoints3, + parseGpx(inputGpx) + ) } @Test - fun `throws on invalid trackPoints`() { + fun throwsOnInvalidTrackPoints(): Unit = runBlocking { assertFails { - parseGpx(minimalGpxBuilder(listOf(TrackPoint("99.0", "-12.1")))) + parseGpx( + minimalGpxBuilder( + listOf(TrackPoint("99.0", "-12.1")) + ) + ) } assertFails { - parseGpx(minimalGpxBuilder(listOf(TrackPoint("-11.5", "-181.0")))) + parseGpx( + minimalGpxBuilder( + listOf(TrackPoint("-11.5", "-181.0")) + ) + ) } } @Test - fun `throws on non-gpx files`() { + fun throwsOnNonGpxFiles(): Unit = runBlocking { val nonGpxXml = """ @@ -120,7 +137,7 @@ class GpxImportParseTest { } @Test - fun `Exhausting outer before inner sequence yields no elements`() { + fun exhaustingOuterBeforeInnerFlowYieldsNoElements() = runBlocking { val inputGpx = minimalGpxBuilder( arrayListOf( TrackPoint("-39.654", "180"), @@ -129,28 +146,28 @@ class GpxImportParseTest { ) // exhausting outer first - val incorrectlyRetrievedSegments = parseGpxFile(inputGpx.byteInputStream()).toList(); + val incorrectlyRetrievedSegments = parseGpxFile(inputGpx.byteInputStream()).toList() assertEquals( 1, incorrectlyRetrievedSegments.size, "Exhausting outer first fails to retrieve the track segment" ) assertEquals( - 0, incorrectlyRetrievedSegments.first().count(), + emptyList(), incorrectlyRetrievedSegments.first().toList(), "Exhausting outer first unexpectedly yields track points" ) // exhausting inner first - val correctlyRetrievedSegments = - parseGpxFile(inputGpx.byteInputStream()).flatMap { it.toList() } + val correctlyRetrievedSegments = parseGpx(inputGpx) assertEquals( - 2, correctlyRetrievedSegments.count(), + 2, correctlyRetrievedSegments.size, "Exhausting inner first fails to retrieve track points" ) } @Test - fun `handles additional data gracefully`() { - val originalTrackPoints = arrayListOf(TrackPoint("88", "-19")) + fun handlesAdditionalDataGracefully() = runBlocking { + val originalTrackPoints = + arrayListOf(TrackPoint("88", "-19")) val inputGpx = buildString { append("") @@ -167,28 +184,31 @@ class GpxImportParseTest { append("") } - assertSuccess(originalTrackPoints, parseGpx(inputGpx)) + assertSuccess( + originalTrackPoints, + parseGpx(inputGpx) + ) } -} -private fun assertSuccess( - originalTrackPoints: List, - parseResult: List, -) { - assertEquals( - originalTrackPoints.size, parseResult.size, - "Not all trackPoints are retrieved" - ) - originalTrackPoints.map{it.toLatLon()}.zip(parseResult).forEach { pair -> - assertEquals( - expected = pair.component1().latitude, - actual = pair.component2().latitude, - "Latitudes don't match" - ) + private fun assertSuccess( + originalTrackPoints: List, + parseResult: List, + ) { assertEquals( - expected = pair.component1().longitude, - actual = pair.component2().longitude, - "Longitudes don't match" + originalTrackPoints.size, parseResult.size, + "Not all trackPoints are retrieved" ) + originalTrackPoints.map { it.toLatLon() }.zip(parseResult).forEach { pair -> + assertEquals( + expected = pair.component1().latitude, + actual = pair.component2().latitude, + "Latitudes don't match" + ) + assertEquals( + expected = pair.component1().longitude, + actual = pair.component2().longitude, + "Longitudes don't match" + ) + } } } diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportTest.kt b/app/src/androidTest/java/de/westnordost/streetcomplete/data/import/GpxImportTest.kt similarity index 68% rename from app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportTest.kt rename to app/src/androidTest/java/de/westnordost/streetcomplete/data/import/GpxImportTest.kt index cc32914d37a..77ec16b7e09 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportTest.kt +++ b/app/src/androidTest/java/de/westnordost/streetcomplete/data/import/GpxImportTest.kt @@ -3,21 +3,21 @@ package de.westnordost.streetcomplete.data.import import de.westnordost.streetcomplete.ApplicationConstants import de.westnordost.streetcomplete.data.download.tiles.enclosingTilePos import de.westnordost.streetcomplete.data.download.tiles.enclosingTilesRect +import de.westnordost.streetcomplete.data.osm.mapdata.BoundingBox import de.westnordost.streetcomplete.data.osm.mapdata.LatLon import de.westnordost.streetcomplete.util.math.contains -import de.westnordost.streetcomplete.util.math.initialBearingTo import de.westnordost.streetcomplete.util.math.translate +import kotlinx.coroutines.async +import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.flow.toList import kotlinx.coroutines.runBlocking import org.junit.Test -import org.junit.runner.RunWith -import org.robolectric.RobolectricTestRunner import kotlin.test.assertEquals import kotlin.test.assertTrue -@RunWith(RobolectricTestRunner::class) class GpxImportTest { @Test - fun `download works on single-segment track`() = runBlocking { + fun downloadWorksOnSingleSegmentTrack() = runBlocking { val originalTrackPoints = arrayListOf( TrackPoint("0.0", "0.0"), TrackPoint("1.3", "-0.3"), @@ -26,20 +26,19 @@ class GpxImportTest { TrackPoint("2.4", "-2.2"), TrackPoint("2.6", "-3"), ) - - val minDownloadDistance = 100.0 val inputGpx = minimalGpxBuilder(originalTrackPoints) - val data = importGpx( - inputGpx.byteInputStream(), - displayTrack = true, - findDownloadBBoxes = true, + val minDownloadDistance = 100.0 + val (segments, downloadBBoxes) = import(minDownloadDistance, inputGpx) + assertInvariants( + originalTrackPoints, + segments, + downloadBBoxes, minDownloadDistance ) - assertInvariants(originalTrackPoints, data.getOrThrow(), minDownloadDistance) } @Test - fun `display-only import works on single-segment track`() = runBlocking { + fun displayOnlyImportWorksOnSingleSegmentTrack() = runBlocking { val originalTrackPoints = arrayListOf( TrackPoint("-36.1", "-143.0"), TrackPoint("-40.2", "-179.999"), @@ -48,23 +47,18 @@ class GpxImportTest { ) val inputGpx = minimalGpxBuilder(originalTrackPoints) - val data = importGpx( - inputGpx.byteInputStream(), - displayTrack = true, - findDownloadBBoxes = false, - 250.0 - ) + val (segments, _) = import(250.0, inputGpx) originalTrackPoints.forEach { trackPoint -> assertTrue( - data.getOrThrow().segments.any { it.contains(trackPoint.toLatLon()) }, + segments.any { it.contains(trackPoint.toLatLon()) }, "originalTrackPoint $trackPoint not contained in displayed segments" ) } } @Test - fun `east-west line downloads necessary area`() = runBlocking { + fun eastWestLineDownloadsNecessaryArea() = runBlocking { // at zoom level 16, one tile is 0.005° latitude high, or ~500m high on equator // the equator itself lies directly on a boundary between two tile rows // LatLon(0.0025, ..) is in the middle of the row above the equator @@ -80,14 +74,14 @@ class GpxImportTest { // setting the minDownloadDistance well below half tile height should download only one row val smallMinDownloadDistance = 100.0 - val smallData = importGpx( - inputGpx.byteInputStream(), - displayTrack = true, - findDownloadBBoxes = true, + val (smallSegments, smallDownloadBBoxes) = import(smallMinDownloadDistance, inputGpx) + assertInvariants( + originalTrackPoints, + smallSegments, + smallDownloadBBoxes, smallMinDownloadDistance - ).getOrThrow() - assertInvariants(originalTrackPoints, smallData, smallMinDownloadDistance) - smallData.downloadBBoxes + ) + smallDownloadBBoxes .map { it.enclosingTilesRect(ApplicationConstants.DOWNLOAD_TILE_ZOOM) } .forEach { assertEquals( @@ -103,15 +97,15 @@ class GpxImportTest { } // setting the minDownloadDistance at half tile width should download three rows of tiles - val bigDownloadDistance = 250.0 - val bigData = importGpx( - inputGpx.byteInputStream(), - displayTrack = true, - findDownloadBBoxes = true, - bigDownloadDistance - ).getOrThrow() - assertInvariants(originalTrackPoints, bigData, bigDownloadDistance) - bigData.downloadBBoxes + val bigMinDownloadDistance = 250.0 + val (bigSegments, bigDownloadBBoxes) = import(bigMinDownloadDistance, inputGpx) + assertInvariants( + originalTrackPoints, + bigSegments, + bigDownloadBBoxes, + bigMinDownloadDistance + ) + bigDownloadBBoxes .map { it.enclosingTilesRect(ApplicationConstants.DOWNLOAD_TILE_ZOOM) } .forEach { assertEquals( @@ -128,7 +122,7 @@ class GpxImportTest { } @Test - fun `north-south line downloads necessary area`() = runBlocking { + fun northSouthLineDownloadsNecessaryArea() = runBlocking { // at zoom level 16, one tile is 0.005° longitude wide, or ~250m at 60° latitude // LatLon(.., 0.0025) is in the middle of the column to the east of the 0 meridian require(ApplicationConstants.DOWNLOAD_TILE_ZOOM == 16) @@ -143,14 +137,14 @@ class GpxImportTest { // setting the minDownloadDistance well below half tile width should download only one column val smallMinDownloadDistance = 50.0 - val smallData = importGpx( - inputGpx.byteInputStream(), - displayTrack = true, - findDownloadBBoxes = true, + val (smallSegments, smallDownloadBBoxes) = import(smallMinDownloadDistance, inputGpx) + assertInvariants( + originalTrackPoints, + smallSegments, + smallDownloadBBoxes, smallMinDownloadDistance - ).getOrThrow() - assertInvariants(originalTrackPoints, smallData, smallMinDownloadDistance) - smallData.downloadBBoxes + ) + smallDownloadBBoxes .map { it.enclosingTilesRect(ApplicationConstants.DOWNLOAD_TILE_ZOOM) } .forEach { assertEquals( @@ -166,15 +160,15 @@ class GpxImportTest { } // setting the minDownloadDistance at 1.5 times tile width should download five columns - val bigDownloadDistance = 375.0 - val bigData = importGpx( - inputGpx.byteInputStream(), - displayTrack = true, - findDownloadBBoxes = true, - bigDownloadDistance - ).getOrThrow() - assertInvariants(originalTrackPoints, bigData, bigDownloadDistance) - bigData.downloadBBoxes + val bigMinDownloadDistance = 375.0 + val (bigSegments, bigDownloadBBoxes) = import(bigMinDownloadDistance, inputGpx) + assertInvariants( + originalTrackPoints, + bigSegments, + bigDownloadBBoxes, + bigMinDownloadDistance + ) + bigDownloadBBoxes .map { it.enclosingTilesRect(ApplicationConstants.DOWNLOAD_TILE_ZOOM) } .forEach { assertEquals( @@ -191,7 +185,7 @@ class GpxImportTest { } @Test - fun `line close to corner downloads adjacent tile`() = runBlocking { + fun lineCloseToCornerDownloadsAdjacentTile() = runBlocking { /* a line barely not touching the corner of a tile should download all four tiles around the corner - even if one of them is not touched by the line itself @@ -221,35 +215,38 @@ class GpxImportTest { // area around line should touch NW val touchingMinDownloadDistance = lineOriginDistance + 0.001 - val touchingData = importGpx( - inputGpx.byteInputStream(), - displayTrack = true, - findDownloadBBoxes = true, + val (touchingSegments, touchingDownloadBBoxes) = import( + touchingMinDownloadDistance, + inputGpx + ) + assertInvariants( + originalTrackPoints, + touchingSegments, + touchingDownloadBBoxes, touchingMinDownloadDistance - ).getOrThrow() - assertInvariants(originalTrackPoints, touchingData, touchingMinDownloadDistance) + ) assertTrue( - touchingData.downloadBBoxes.any { it.contains(nWCenterPoint) }, + touchingDownloadBBoxes.any { it.contains(nWCenterPoint) }, "north west center point not contained" ) // area around line should not touch NW val noTouchMinDownloadDistance = lineOriginDistance / 2 - val noTouchData = importGpx( - inputGpx.byteInputStream(), - displayTrack = true, - findDownloadBBoxes = true, + val (noTouchSegments, noTouchDownloadBBoxes) = import(noTouchMinDownloadDistance, inputGpx) + assertInvariants( + originalTrackPoints, + noTouchSegments, + noTouchDownloadBBoxes, noTouchMinDownloadDistance - ).getOrThrow() - assertInvariants(originalTrackPoints, noTouchData, noTouchMinDownloadDistance) + ) assertTrue( - !noTouchData.downloadBBoxes.any { it.contains(nWCenterPoint) }, + !noTouchDownloadBBoxes.any { it.contains(nWCenterPoint) }, "north west center point contained even if it should not" ) } @Test - fun `works around equator`() = runBlocking { + fun worksAroundEquator() = runBlocking { val originalTrackPoints = arrayListOf( TrackPoint("0.0", "73.1"), TrackPoint("-0.3", "74.2"), @@ -257,17 +254,17 @@ class GpxImportTest { val minDownloadDistance = 100.0 val inputGpx = minimalGpxBuilder(originalTrackPoints) - val data = importGpx( - inputGpx.byteInputStream(), - displayTrack = true, - findDownloadBBoxes = true, + val (segments, downloadBBoxes) = import(minDownloadDistance, inputGpx) + assertInvariants( + originalTrackPoints, + segments, + downloadBBoxes, minDownloadDistance ) - assertInvariants(originalTrackPoints, data.getOrThrow(), minDownloadDistance) } @Test - fun `works with wrap-around longitude`() = runBlocking { + fun worksWithWrapAroundLongitude() = runBlocking { val originalTrackPoints = arrayListOf( TrackPoint("-33.0", "164.9"), TrackPoint("-35.1", "-170.3"), @@ -275,17 +272,17 @@ class GpxImportTest { val minDownloadDistance = 100.0 val inputGpx = minimalGpxBuilder(originalTrackPoints) - val data = importGpx( - inputGpx.byteInputStream(), - displayTrack = true, - findDownloadBBoxes = true, + val (segments, downloadBBoxes) = import(minDownloadDistance, inputGpx) + assertInvariants( + originalTrackPoints, + segments, + downloadBBoxes, minDownloadDistance ) - assertInvariants(originalTrackPoints, data.getOrThrow(), minDownloadDistance) } @Test - fun `works around north pole`() = runBlocking { + fun worksAroundNorthPole() = runBlocking { // TODO sgr: would actually like to run test with lat: 90.0, but this fails // latitude should be between -85.0511 and 85.0511 to be within OSM // tiles apparently, see https://wiki.openstreetmap.org/wiki/Slippy_map_tilenames @@ -298,34 +295,54 @@ class GpxImportTest { val minDownloadDistance = 500.0 val inputGpx = minimalGpxBuilder(originalTrackPoints) - val data = importGpx( - inputGpx.byteInputStream(), - displayTrack = false, - findDownloadBBoxes = true, + val (segments, downloadBBoxes) = import(minDownloadDistance, inputGpx) + assertInvariants( + originalTrackPoints, + segments, + downloadBBoxes, minDownloadDistance ) - assertInvariants(originalTrackPoints, data.getOrThrow(), minDownloadDistance) + } + + private suspend fun import( + minDownloadDistance: Double, + inputGpx: String, + ): Pair>, List> { + return coroutineScope { + val importer = GpxImporter(minDownloadDistance) + val segments = async { + val segments = arrayListOf>() + importer.segments.collect { segments.add(it.toList()) } + segments + } + val bBoxes = async { + importer.downloadBBoxes.toList() + } + importer.readFile(this, inputGpx.byteInputStream()) + return@coroutineScope Pair(segments.await(), bBoxes.await()) + } } private fun assertInvariants( originalTrackPoints: List, - importData: GpxImportData, + segments: List>, + downloadBBoxes: List, minDownloadDistance: Double, ) { originalTrackPoints.forEach { trackPoint -> assertTrue( - importData.segments.any { it.contains(trackPoint.toLatLon()) }, + segments.any { it.contains(trackPoint.toLatLon()) }, "originalTrackPoint $trackPoint not contained in displayed segments" ) assertTrue( - importData.downloadBBoxes.any { it.contains(trackPoint.toLatLon()) }, + downloadBBoxes.any { it.contains(trackPoint.toLatLon()) }, "originalTrackPoint $trackPoint not contained in area to download" ) for (testAngle in 0..360 step 10) { val testPoint = trackPoint.toLatLon().translate(minDownloadDistance, testAngle.toDouble()) assertTrue( - importData.downloadBBoxes.any { it.contains(testPoint) }, + downloadBBoxes.any { it.contains(testPoint) }, "$testPoint <= $minDownloadDistance away from $trackPoint not included in area to download" ) } diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/TestHelpers.kt b/app/src/androidTest/java/de/westnordost/streetcomplete/data/import/TestHelpers.kt similarity index 62% rename from app/src/test/java/de/westnordost/streetcomplete/data/import/TestHelpers.kt rename to app/src/androidTest/java/de/westnordost/streetcomplete/data/import/TestHelpers.kt index 1e0cdecd4cf..a04458e7243 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/import/TestHelpers.kt +++ b/app/src/androidTest/java/de/westnordost/streetcomplete/data/import/TestHelpers.kt @@ -1,15 +1,20 @@ package de.westnordost.streetcomplete.data.import import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.flatMapConcat +import kotlinx.coroutines.flow.toList data class TrackPoint( val lat: String, val lon: String, ) -internal fun LatLon.toTrackPoint() : TrackPoint { + +internal fun LatLon.toTrackPoint(): TrackPoint { return TrackPoint(this.latitude.toString(), this.longitude.toString()) } -internal fun TrackPoint.toLatLon() : LatLon { + +internal fun TrackPoint.toLatLon(): LatLon { return LatLon(this.lat.toDouble(), this.lon.toDouble()) } @@ -26,8 +31,9 @@ internal fun minimalGpxBuilder(trackPoints: List): String = buildStr append("") } -internal fun parseGpx(input: String): List { - // make sure sequences are exhausted in correct order - return parseGpxFile(input.byteInputStream()).flatMap { it.toList() }.toList() +@OptIn(ExperimentalCoroutinesApi::class) +internal suspend fun parseGpx(input: String): List { + // make sure flows are exhausted in correct order + return parseGpxFile(input.byteInputStream()).flatMapConcat { it }.toList() } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImport.kt b/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImport.kt index 7c6bac52fa3..3fbf98e02f3 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImport.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImport.kt @@ -5,29 +5,31 @@ import de.westnordost.streetcomplete.ApplicationConstants import de.westnordost.streetcomplete.data.download.tiles.TilePos import de.westnordost.streetcomplete.data.download.tiles.enclosingTilesRect import de.westnordost.streetcomplete.data.osm.mapdata.BoundingBox - import de.westnordost.streetcomplete.data.osm.mapdata.LatLon import de.westnordost.streetcomplete.data.osm.mapdata.toPolygon -import de.westnordost.streetcomplete.util.math.area import de.westnordost.streetcomplete.util.math.distanceTo import de.westnordost.streetcomplete.util.math.enclosingBoundingBox import de.westnordost.streetcomplete.util.math.initialBearingTo import de.westnordost.streetcomplete.util.math.translate -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.withContext +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.flow.SharedFlow +import kotlinx.coroutines.flow.asSharedFlow +import kotlinx.coroutines.flow.emitAll +import kotlinx.coroutines.flow.filterNotNull +import kotlinx.coroutines.flow.flattenConcat +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.takeWhile +import kotlinx.coroutines.flow.withIndex +import kotlinx.coroutines.launch import java.io.InputStream import kotlin.math.sqrt private const val TAG = "GpxImport" -data class GpxImportData( - val displayTrack: Boolean, - val downloadAlongTrack: Boolean, - val segments: List>, - val downloadBBoxes: List, - val areaToDownloadInSqkm: Double, -) - internal class DecoratedBoundingBox( val polygon: Iterable, requestedTiles: Set? = null, @@ -47,53 +49,9 @@ internal class DecoratedBoundingBox( } } -/** - * @param inputStream valid XML according to http://www.topografix.com/GPX/1/1 schema - * Note: the caller is responsible to close the inputStream as appropriate - * @param displayTrack display the track on the map after import - * @param findDownloadBBoxes if true, compute bounding boxes which need to be downloaded to cover the track - * @param minDownloadDistance in meters; all points within minDownloadDistance along the track should be downloaded - */ -suspend fun importGpx( - inputStream: InputStream, - displayTrack: Boolean, - findDownloadBBoxes: Boolean, +class GpxImporter( minDownloadDistance: Double, -): Result = withContext(Dispatchers.Default) { - require(minDownloadDistance in 10.0..500.0) { - "minDownloadDistance needs to be of reasonable size" - } - val trackSegments = parseGpxFile(inputStream) - - if (findDownloadBBoxes) { - return@withContext importWithBBoxes(trackSegments, displayTrack, minDownloadDistance) - } else { - return@withContext importTrackOnly(trackSegments, displayTrack) - } -} - -private fun importTrackOnly( - trackSegments: Sequence, - displayTrack: Boolean, -): Result { - return Result.success( - GpxImportData( - displayTrack, - false, - trackSegments - .map { it.toList() } - .toList(), - arrayListOf(), - 0.0 - ) - ) -} - -private fun importWithBBoxes( - trackSegments: Sequence, - displayTrack: Boolean, - minDownloadDistance: Double, -): Result { +) { /* Algorithm overview: * Given that two resampled points A and B are at most 2 * minDownloadDistance away from each * other and any track point between them is at most minDownloadDistance away from either A or B, @@ -108,51 +66,42 @@ private fun importWithBBoxes( * contain S_min if their side length is at least 4 * minDownloadDistance / sqrt(2) - the worst * case being were S_min is rotated 45 degrees with respect to the aligned squares. */ - val maxSampleDistance = 2 * minDownloadDistance - val coveringSquareHalfLength = 2 * minDownloadDistance / sqrt(2.0) + private val maxSampleDistance = 2 * minDownloadDistance + private val coveringSquareHalfLength = 2 * minDownloadDistance / sqrt(2.0) - // TODO sgr: find a good way of retrieving original track points and area to download. - // Up to this point, working on sequences seems clean to me and would be easy if we only need - // the bounding boxes. Retrieving additional info seems to be awkward though. - // - - // One workaround might be to parse the file twice (once only to retrieve the original track - // points) and omit estimating any download size. - val originalTrackPoints = arrayListOf>() - val mergedBBoxes = arrayListOf() - val areaToDownloadInSqkm = trackSegments - // TODO sgr: this seems to be an awkward way of smuggling out original track points - .map { - originalTrackPoints.add(arrayListOf()) - it.onEach { trackPoint -> - originalTrackPoints.last().add(trackPoint) - } - } - .map { trackSegment -> - trackSegment + private val _mutableSegments = MutableSharedFlow?>() + private val _sharedSegments = _mutableSegments.asSharedFlow() + + val segments = _sharedSegments.takeWhile { it != null }.filterNotNull() + .map { flow -> flow.takeWhile { it != null }.filterNotNull() } + + @OptIn(ExperimentalCoroutinesApi::class) + val downloadBBoxes = _sharedSegments.takeWhile { it != null }.filterNotNull() + .map { flow -> + flow.takeWhile { it != null }.filterNotNull() .addInterpolatedPoints(maxSampleDistance) .discardRedundantPoints(maxSampleDistance) .mapToCenteredSquares(coveringSquareHalfLength) .determineBBoxes() } - .flatten() + .flattenConcat() .mergeBBoxes() - // TODO sgr: this seems to be an awkward way of smuggling out bounding boxes - .onEach { - mergedBBoxes.add(it.boundingBox) - } - .flatMap { it.tiles } - .distinct() - .sumOf { it.asBoundingBox(ApplicationConstants.DOWNLOAD_TILE_ZOOM).area() } / 1000000 + .map { it.boundingBox } - return Result.success( - GpxImportData( - displayTrack, - true, - originalTrackPoints, - mergedBBoxes, - areaToDownloadInSqkm - ) - ) + fun readFile(scope: CoroutineScope, inputStream: InputStream) { + scope.launch { + parseGpxFile(inputStream).collect { segment -> + val m = MutableSharedFlow() + _mutableSegments.emit(m.asSharedFlow()) + segment.collect { + m.emit(it) + } + m.emit(null) + } + + _mutableSegments.emit(null) + } + } } /** @@ -162,84 +111,89 @@ private fun importWithBBoxes( * The algorithm merges adjacent boxes if the merged box still has a good enough ratio * of actually requested vs total number of tiles downloaded. */ -internal fun Sequence.mergeBBoxes(): Sequence { - val inputIterator = this.iterator() - if (!inputIterator.hasNext()) - return emptySequence() - return sequence { - var mergedBBox = inputIterator.next() - while(inputIterator.hasNext()) { - val bBox = inputIterator.next() - val candidateBBox = DecoratedBoundingBox( - mergedBBox.polygon + bBox.polygon, - mergedBBox.requestedTiles.plus(bBox.tiles) - ) - val requestedRatio = - candidateBBox.requestedTiles.size.toDouble() / candidateBBox.numberOfTiles - Log.d(TAG, "requestedRatio = $requestedRatio)") - // requestedRatio >= 0.75 is a good compromise, as this allows downloading three - // neighbouring tiles at zoom level x in a single call at level x-1 - mergedBBox = if (requestedRatio >= 0.75) { - candidateBBox +internal fun Flow.mergeBBoxes(): Flow { + return flow { + lateinit var mergedBBox: DecoratedBoundingBox + var initialized = false + collect { + if (initialized) { + val candidateBBox = DecoratedBoundingBox( + mergedBBox.polygon + it.polygon, + mergedBBox.requestedTiles.plus(it.tiles) + ) + val requestedRatio = + candidateBBox.requestedTiles.size.toDouble() / candidateBBox.numberOfTiles + Log.d(TAG, "requestedRatio = $requestedRatio") + // requestedRatio >= 0.75 is a good compromise, as this allows downloading three + // neighbouring tiles at zoom level x in a single call at level x-1 + mergedBBox = if (requestedRatio >= 0.75) { + candidateBBox + } else { + emit(mergedBBox) + it + } } else { - yield(mergedBBox) - bBox + mergedBBox = it + initialized = true } - } - yield(mergedBBox) + if (initialized) { + emit(mergedBBox) + } } } /** - * Reduce a sequence of bounding boxes by + * Reduce a flow of bounding boxes by * - dropping boxes which don't contribute additional tiles to download * - merging adjacent boxes if no additional tiles are contained in the merged box * * the mapped boxes are also decorated with some cached data for future processing. */ -internal fun Sequence.determineBBoxes(): Sequence { - val inputIterator = this.map { DecoratedBoundingBox(it.toPolygon()) }.withIndex().iterator() - if (!inputIterator.hasNext()) { - return emptySequence() - } - +internal fun Flow.determineBBoxes(): Flow { + val inputFlow = this.map { DecoratedBoundingBox(it.toPolygon()) }.withIndex() val uniqueTilesToDownload = HashSet() - return sequence { - var currentBBox = inputIterator.next().value - uniqueTilesToDownload.addAll(currentBBox.tiles) - - for ((index, newBBox) in inputIterator) { - if (newBBox.tiles.all { it in uniqueTilesToDownload }) { - Log.d(TAG, "omit bounding box #$index, all tiles already scheduled for download") - continue - } - val extendedBBox = DecoratedBoundingBox(currentBBox.polygon + newBBox.polygon) - currentBBox = if ( - extendedBBox.numberOfTiles <= (currentBBox.tiles + newBBox.tiles).toHashSet().size - ) { - // no additional tile needed to extend the polygon and download newBBox together with currentBBox - Log.d(TAG, "extend currentBBox with bounding box #$index") - extendedBBox - } else { - Log.d(TAG, "retain currentBBox, start new with bounding box #$index") - yield(currentBBox) + return flow { + lateinit var currentBBox: DecoratedBoundingBox + var initialized = false + inputFlow.collect { + val newBBox = it.value + if (!initialized) { + currentBBox = newBBox uniqueTilesToDownload.addAll(currentBBox.tiles) - newBBox + initialized = true + } else if (newBBox.tiles.all { bBox -> bBox in uniqueTilesToDownload }) { + Log.d(TAG, "omit bounding box #$it.index, all tiles already scheduled for download") + } else { + val extendedBBox = DecoratedBoundingBox(currentBBox.polygon + newBBox.polygon) + currentBBox = if ( + extendedBBox.numberOfTiles <= (currentBBox.tiles + newBBox.tiles).toHashSet().size + ) { + // no additional tile needed to extend the polygon and download newBBox together with currentBBox + Log.d(TAG, "extend currentBBox with bounding box #$it.index") + extendedBBox + } else { + Log.d(TAG, "retain currentBBox, start new with bounding box #$it.index") + emit(currentBBox) + uniqueTilesToDownload.addAll(currentBBox.tiles) + newBBox + } } } - yield(currentBBox) + if (initialized) { + emit(currentBBox) + } } } /** - * Transform a sequence of points to a sequence of north-south aligned bounding boxes centered on + * Transform a flow of points to a flow of north-south aligned bounding boxes centered on * these points. * * @param halfSideLength > 0.0, in meters */ -internal fun Sequence.mapToCenteredSquares(halfSideLength: Double): Sequence { +internal fun Flow.mapToCenteredSquares(halfSideLength: Double): Flow { require(halfSideLength > 0.0) { "halfSideLength has to be positive" } @@ -263,25 +217,26 @@ internal fun Sequence.mapToCenteredSquares(halfSideLength: Double): Sequ * * @param samplingDistance > 0.0, in meters */ -internal fun Sequence.addInterpolatedPoints(samplingDistance: Double): Sequence { - require(samplingDistance > 0.0) { - "samplingDistance has to be positive" - } +internal fun Flow.addInterpolatedPoints(samplingDistance: Double): Flow = + flow { + require(samplingDistance > 0.0) { + "samplingDistance has to be positive" + } - val inputIterator = this.iterator() - if (!inputIterator.hasNext()) { - return emptySequence() - } - var lastPoint = inputIterator.next() - return sequence { - while (inputIterator.hasNext()) { - val currentPoint = inputIterator.next() - this.yieldAll(interpolate(lastPoint, currentPoint, samplingDistance)) - lastPoint = currentPoint + lateinit var lastPoint: LatLon + var initialized = false + collect { + if (initialized) { + this.emitAll(interpolate(lastPoint, it, samplingDistance)) + } else { + initialized = true + } + lastPoint = it + } + if (initialized) { + emit(lastPoint) } - yield(lastPoint) } -} /** * Interpolate points between start (included) and end (not included) @@ -291,24 +246,23 @@ internal fun Sequence.addInterpolatedPoints(samplingDistance: Double): S * * @param samplingDistance > 0.0, in meters */ -private fun interpolate(start: LatLon, end: LatLon, samplingDistance: Double): Sequence = - sequence { - require(samplingDistance > 0.0) { - "samplingDistance has to be positive" - } +private fun interpolate(start: LatLon, end: LatLon, samplingDistance: Double): Flow = flow { + require(samplingDistance > 0.0) { + "samplingDistance has to be positive" + } - var intermediatePoint = start - while (true) { - yield(intermediatePoint) - if (intermediatePoint.distanceTo(end) <= samplingDistance) { - break - } - intermediatePoint = intermediatePoint.translate( - samplingDistance, - intermediatePoint.initialBearingTo(end) - ) + var intermediatePoint = start + while (true) { + emit(intermediatePoint) + if (intermediatePoint.distanceTo(end) <= samplingDistance) { + break } + intermediatePoint = intermediatePoint.translate( + samplingDistance, + intermediatePoint.initialBearingTo(end) + ) } +} /** * Discard redundant points, such that no three adjacent points A, B, C remain where B is less than @@ -316,34 +270,37 @@ private fun interpolate(start: LatLon, end: LatLon, samplingDistance: Double): S * * @param samplingDistance > 0.0, in meters */ -internal fun Sequence.discardRedundantPoints(samplingDistance: Double): Sequence { +internal fun Flow.discardRedundantPoints(samplingDistance: Double): Flow = flow { require(samplingDistance > 0.0) { "samplingDistance has to be positive" } - val inputIterator = this.iterator() - if (!inputIterator.hasNext()) { - return emptySequence() - } - return sequence { - var lastRetainedPoint = inputIterator.next() - yield(lastRetainedPoint) - - if (inputIterator.hasNext()) { - var candidatePoint = inputIterator.next() - while (inputIterator.hasNext()) { - val currentPoint = inputIterator.next() - if (lastRetainedPoint.distanceTo(candidatePoint) < samplingDistance - && candidatePoint.distanceTo(currentPoint) < samplingDistance - ) { - // discard candidatePoint - } else { - lastRetainedPoint = candidatePoint - yield(lastRetainedPoint) - } - candidatePoint = currentPoint + lateinit var lastRetainedPoint: LatLon + lateinit var candidatePoint: LatLon + var initializedLastRetainedPoint = false + var initializedCandidatePoint = false + collect { + if (!initializedLastRetainedPoint) { + lastRetainedPoint = it + initializedLastRetainedPoint = true + emit(it) + } else if (!initializedCandidatePoint) { + candidatePoint = it + initializedCandidatePoint = true + } else { + val currentPoint = it + if (lastRetainedPoint.distanceTo(candidatePoint) < samplingDistance + && candidatePoint.distanceTo(currentPoint) < samplingDistance + ) { + // discard candidatePoint + } else { + lastRetainedPoint = candidatePoint + emit(lastRetainedPoint) } - yield(candidatePoint) + candidatePoint = currentPoint } } + if (initializedCandidatePoint) { + emit(candidatePoint) + } } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImportParse.kt b/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImportParse.kt index a6741dab7e5..130ecb6a2e4 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImportParse.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImportParse.kt @@ -2,6 +2,8 @@ package de.westnordost.streetcomplete.data.import import android.util.Xml import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.flow import org.xmlpull.v1.XmlPullParser import org.xmlpull.v1.XmlPullParserException import java.io.IOException @@ -10,7 +12,7 @@ import java.io.InputStream private const val TRACK_POINT = "trkpt" private const val SEGMENT = "trkseg" -typealias TrackSegment = Sequence +typealias TrackSegment = Flow /** * Parses all track points from a GPX file. @@ -31,7 +33,7 @@ typealias TrackSegment = Sequence * result.flatMap { it.toList() }.toList() will. */ @Throws(XmlPullParserException::class, IOException::class) -fun parseGpxFile(inputStream: InputStream): Sequence { +fun parseGpxFile(inputStream: InputStream): Flow { val parser = Xml.newPullParser() parser.setInput(inputStream, "UTF-8") @@ -40,7 +42,7 @@ fun parseGpxFile(inputStream: InputStream): Sequence { parser.nextTag() parser.require(XmlPullParser.START_TAG, "http://www.topografix.com/GPX/1/1", "gpx") - return sequence { + return flow { var depth = 1 while (depth != 0) { when (parser.next()) { @@ -51,7 +53,7 @@ fun parseGpxFile(inputStream: InputStream): Sequence { XmlPullParser.START_TAG -> { if (parser.name == SEGMENT) { // segment is closed while parsing, thus depth remains the same - yield( + emit( parseSegment(parser) ) } else { @@ -68,20 +70,20 @@ fun parseGpxFile(inputStream: InputStream): Sequence { } @Throws(XmlPullParserException::class, IOException::class) -private fun parseSegment(parser: XmlPullParser): TrackSegment = sequence { +private fun parseSegment(parser: XmlPullParser): TrackSegment = flow { var depth = 1 while (depth != 0) { when (parser.next()) { XmlPullParser.END_TAG -> { if (parser.name == SEGMENT) { - return@sequence + return@flow } depth-- } XmlPullParser.START_TAG -> { if (parser.name == TRACK_POINT) { - yield( + emit( LatLon( parser.getAttributeValue(null, "lat").toDouble(), parser.getAttributeValue(null, "lon").toDouble() diff --git a/app/src/main/java/de/westnordost/streetcomplete/screens/main/MainFragment.kt b/app/src/main/java/de/westnordost/streetcomplete/screens/main/MainFragment.kt index 0322e5a04dc..b30fae24cf3 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/screens/main/MainFragment.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/screens/main/MainFragment.kt @@ -39,9 +39,11 @@ import de.westnordost.streetcomplete.Prefs import de.westnordost.streetcomplete.R import de.westnordost.streetcomplete.data.download.DownloadController import de.westnordost.streetcomplete.data.download.tiles.asBoundingBoxOfEnclosingTiles +import de.westnordost.streetcomplete.data.download.tiles.enclosingTilesRect import de.westnordost.streetcomplete.data.edithistory.Edit import de.westnordost.streetcomplete.data.edithistory.EditKey import de.westnordost.streetcomplete.data.edithistory.icon +import de.westnordost.streetcomplete.data.import.GpxImporter import de.westnordost.streetcomplete.data.meta.CountryInfos import de.westnordost.streetcomplete.data.meta.LengthUnit import de.westnordost.streetcomplete.data.osm.edits.ElementEdit @@ -121,7 +123,16 @@ import de.westnordost.streetcomplete.util.math.enlargedBy import de.westnordost.streetcomplete.util.math.initialBearingTo import de.westnordost.streetcomplete.util.viewBinding import de.westnordost.streetcomplete.view.insets_animation.respectSystemInsets +import kotlinx.coroutines.Deferred import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.async +import kotlinx.coroutines.flow.asFlow +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.flatMapConcat +import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.reduce +import kotlinx.coroutines.flow.toList import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine import kotlinx.coroutines.withContext @@ -1203,41 +1214,86 @@ class MainFragment : setIsFollowingPosition(false) } + private data class DownloadStats ( + val numberOfDownloads: Int, + val areaInSqkm: Double, + ) + + @OptIn(ExperimentalCoroutinesApi::class) fun importTrack(uri: Uri) { val ctx = context ?: return val fragment = mapFragment ?: return + val lengthUnit = Locale.getDefault().country.takeIf { it.isNotEmpty() } + ?.let { countryInfos.get(listOf(it)) }?.lengthUnits?.first() ?: LengthUnit.METER + viewLifecycleScope.launch { - val lengthUnit = Locale.getDefault().country.takeIf { it.isNotEmpty() } - ?.let { countryInfos.get(listOf(it)) }?.lengthUnits?.first() ?: LengthUnit.METER - - val importData = suspendCancellableCoroutine { cont -> - ctx.contentResolver?.openInputStream(uri)?.let { inputStream -> - val dialog = GpxImportSettingsDialog( - inputStream, - lengthUnit - ) { cont.resume(it) } + val importSettings = suspendCancellableCoroutine { cont -> + val dialog = GpxImportSettingsDialog(lengthUnit) { cont.resume(it) } dialog.show(parentFragmentManager, null) } - }.getOrNull() ?: return@launch - if (importData.displayTrack) { - // TODO sgr: maybe want to get rid of showing the track, as it would mean storing all points which we are avoiding with sequences.. - fragment.replaceImportedTrack(importData.segments) + val importer = GpxImporter(importSettings.minDownloadDistance) + + // read file once to find number and size of downloads, as well as original imported points + var downloadStatsResult: Deferred? = null + if(importSettings.downloadAlongTrack) { + downloadStatsResult = async { + var numberOfDownloads = 0 + val areaInSqkm = importer.downloadBBoxes + .map { + numberOfDownloads += 1 + it + } + .flatMapConcat { it.enclosingTilesRect(ApplicationConstants.DOWNLOAD_TILE_ZOOM).asTilePosSequence().asFlow() } + .distinctUntilChanged() + .map { it.asBoundingBox(ApplicationConstants.DOWNLOAD_TILE_ZOOM).area() } + .reduce { accumulator, value -> accumulator + value } / 1000000.0 + DownloadStats(numberOfDownloads, areaInSqkm) + } + } + + val newSegmentsResult = async { + val segments = arrayListOf>() + importer.segments.collect {segments.add(it.toList())} + segments + } + + ctx.contentResolver?.openInputStream(uri)?.let { inputStream -> + importer.readFile(this, inputStream) } - if (importData.downloadAlongTrack) { - if (importData.areaToDownloadInSqkm > ApplicationConstants.MAX_DOWNLOADABLE_AREA_IN_SQKM * 25) { + + val downloadStats = downloadStatsResult?.await() + val newSegments = newSegmentsResult.await() + + if (importSettings.displayTrack) { + // TODO sgr: maybe want to get rid of showing the track, as it means storing all points which we are avoiding with sequences.. + fragment.replaceImportedTrack(newSegments) + } + + if(importSettings.downloadAlongTrack) { + assert(downloadStats != null) + if (downloadStats!!.areaInSqkm > ApplicationConstants.MAX_DOWNLOADABLE_AREA_IN_SQKM * 25) { context?.toast(R.string.gpx_import_download_area_too_big, Toast.LENGTH_LONG) } else { suspendCancellableCoroutine { cont -> GpxImportConfirmationDialog( ctx, - importData, + downloadStats!!.areaInSqkm, + downloadStats!!.numberOfDownloads, lengthUnit ) { cont.resume(it) }.show() } - for (bBox in importData.downloadBBoxes) { - downloadController.download(bBox) + + // read file a second time to actually download the boxes + val downloadJob = launch { + importer.downloadBBoxes + .collect {downloadController.download(it)} + } + + ctx.contentResolver?.openInputStream(uri)?.let { inputStream -> + importer.readFile(this, inputStream) } + downloadJob.join() } } } diff --git a/app/src/main/java/de/westnordost/streetcomplete/screens/main/controls/GpxImportConfirmationDialog.kt b/app/src/main/java/de/westnordost/streetcomplete/screens/main/controls/GpxImportConfirmationDialog.kt index d9f6fbda88e..d4d74421aaf 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/screens/main/controls/GpxImportConfirmationDialog.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/screens/main/controls/GpxImportConfirmationDialog.kt @@ -6,34 +6,36 @@ import android.view.LayoutInflater import android.widget.TextView import androidx.appcompat.app.AlertDialog import de.westnordost.streetcomplete.R -import de.westnordost.streetcomplete.data.import.GpxImportData import de.westnordost.streetcomplete.data.meta.LengthUnit /** A dialog to confirm download of (potentially massive) data along imported GPX track */ class GpxImportConfirmationDialog( context: Context, - importData: GpxImportData, + areaToDownloadInSqkm: Double, + numberOfDownloads: Int, lengthUnit: LengthUnit, private val callback: (confirm: Boolean) -> Unit, ) : AlertDialog(context) { init { - val view = LayoutInflater.from(context).inflate(R.layout.dialog_gpx_import_confirmation, null) + val view = + LayoutInflater.from(context).inflate(R.layout.dialog_gpx_import_confirmation, null) setView(view) val formattedArea = when (lengthUnit) { - LengthUnit.FOOT_AND_INCH -> "%.0f acres".format(importData.areaToDownloadInSqkm * ACRES_IN_SQUARE_KILOMETER) - else -> "%.1f km²".format(importData.areaToDownloadInSqkm) + LengthUnit.FOOT_AND_INCH -> "%.0f acres".format(areaToDownloadInSqkm * ACRES_IN_SQUARE_KILOMETER) + else -> "%.1f km²".format(areaToDownloadInSqkm) } val downloadsToScheduleTextView = view.findViewById(R.id.downloadsToScheduleText) downloadsToScheduleTextView.text = context.getString( R.string.gpx_import_number_of_downloads_to_schedule, - importData.downloadBBoxes.size + numberOfDownloads ) val areaToDownloadTextView = view.findViewById(R.id.areaToDownloadText) - areaToDownloadTextView.text = context.getString(R.string.gpx_import_area_to_download, formattedArea) + areaToDownloadTextView.text = + context.getString(R.string.gpx_import_area_to_download, formattedArea) setButton(DialogInterface.BUTTON_POSITIVE, context.getString(android.R.string.ok)) { _, _ -> callback(true) diff --git a/app/src/main/java/de/westnordost/streetcomplete/screens/main/controls/GpxImportSettingsDialog.kt b/app/src/main/java/de/westnordost/streetcomplete/screens/main/controls/GpxImportSettingsDialog.kt index 27189b62fbf..d17fd1c4e7b 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/screens/main/controls/GpxImportSettingsDialog.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/screens/main/controls/GpxImportSettingsDialog.kt @@ -5,27 +5,26 @@ import android.view.View import androidx.fragment.app.DialogFragment import com.google.android.material.slider.LabelFormatter import de.westnordost.streetcomplete.R -import de.westnordost.streetcomplete.data.import.GpxImportData -import de.westnordost.streetcomplete.data.import.importGpx import de.westnordost.streetcomplete.data.meta.LengthUnit import de.westnordost.streetcomplete.databinding.DialogGpxImportSettingsBinding import de.westnordost.streetcomplete.util.ktx.viewLifecycleScope import de.westnordost.streetcomplete.util.viewBinding import kotlinx.coroutines.Deferred -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.async import kotlinx.coroutines.launch -import kotlinx.coroutines.withContext -import java.io.InputStream + +data class GpxImportSettings( + val displayTrack: Boolean, + val downloadAlongTrack: Boolean, + val minDownloadDistance: Double, +) /** A dialog to specify GPX import settings */ class GpxImportSettingsDialog( - private val inputStream: InputStream, private val lengthUnit: LengthUnit, - private val callback: (result: Result) -> Unit, + private val callback: (GpxImportSettings) -> Unit, ) : DialogFragment(R.layout.dialog_gpx_import_settings) { private val binding by viewBinding(DialogGpxImportSettingsBinding::bind) - private var worker: Deferred>? = null + private var worker: Deferred>? = null private val minDownloadDistanceOptions: List = listOf(10.0, 100.0, 250.0, 500.0) @@ -62,7 +61,13 @@ class GpxImportSettingsDialog( } binding.okButton.setOnClickListener { viewLifecycleScope.launch { - callback(processGpxFile()) + callback( + GpxImportSettings( + binding.displayTrackCheckBox.isChecked, + binding.downloadCheckBox.isChecked, + minDownloadDistanceInMeters() + ) + ) } } binding.cancelButton.setOnClickListener { @@ -98,32 +103,14 @@ class GpxImportSettingsDialog( } private fun minDownloadDistanceInMeters(): Double { - val minDownloadDistance = minDownloadDistanceOptions[binding.minDownloadDistanceSlider.value.toInt()] + val minDownloadDistance = + minDownloadDistanceOptions[binding.minDownloadDistanceSlider.value.toInt()] return when (lengthUnit) { LengthUnit.FOOT_AND_INCH -> minDownloadDistance * YARDS_IN_METER else -> minDownloadDistance } } - private suspend fun processGpxFile(): Result { - binding.okButton.isEnabled = false - - worker = viewLifecycleScope.async { - return@async importGpx( - inputStream, - binding.displayTrackCheckBox.isChecked, - binding.downloadCheckBox.isChecked, - minDownloadDistanceInMeters() - ) - } - val importData = worker!!.await() - - worker = null - - dismiss() - return importData - } - companion object { private const val INITIAL_MIN_DOWNLOAD_DISTANCE_INDEX = 1 private const val YARDS_IN_METER = 0.9144 diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportAddInterpolatedPointsTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportAddInterpolatedPointsTest.kt index 5574c85882e..49032237c84 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportAddInterpolatedPointsTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportAddInterpolatedPointsTest.kt @@ -2,6 +2,12 @@ package de.westnordost.streetcomplete.data.import import de.westnordost.streetcomplete.data.osm.mapdata.LatLon import de.westnordost.streetcomplete.util.math.distanceTo +import kotlinx.coroutines.flow.asFlow +import kotlinx.coroutines.flow.count +import kotlinx.coroutines.flow.emptyFlow +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.flow.toList +import kotlinx.coroutines.test.runTest import org.junit.Test import kotlin.test.assertContains import kotlin.test.assertEquals @@ -10,45 +16,45 @@ import kotlin.test.assertTrue class GpxImportAddInterpolatedPointsTest { @Test - fun `fails with bad parameters`() { + fun `fails with bad parameters`() = runTest { assertFails { - emptySequence().addInterpolatedPoints(-15.0).count() + emptyFlow().addInterpolatedPoints(-15.0).count() } } @Test - fun `gracefully handles small sequences`() { + fun `gracefully handles small flows`() = runTest { assertEquals( 0, - emptySequence().addInterpolatedPoints(1.0).count(), - "empty sequence invents points" + emptyFlow().addInterpolatedPoints(1.0).count(), + "empty flow invents points" ) assertEquals( 1, - sequenceOf(LatLon(89.9, 27.1)).addInterpolatedPoints(77.0).count(), - "size 1 sequence invents points" + flowOf(LatLon(89.9, 27.1)).addInterpolatedPoints(77.0).count(), + "size 1 flow invents points" ) } @Test - fun `does not add unnecessary points`() { + fun `does not add unnecessary points`() = runTest { val originalPoints = listOf(LatLon(0.0, 0.0), LatLon(0.1, -0.5), LatLon(1.0, -1.0)) val samplingDistance = originalPoints.zipWithNext().maxOf { it.first.distanceTo(it.second) } val interpolatedPoints = - originalPoints.asSequence().addInterpolatedPoints(samplingDistance).toList() + originalPoints.asFlow().addInterpolatedPoints(samplingDistance).toList() assertEquals(originalPoints.size, interpolatedPoints.size) } @Test - fun `ensures promised sampling distance on single segment`() { + fun `ensures promised sampling distance on single segment`() = runTest { val p1 = LatLon(-11.1, 36.7) val p2 = LatLon(-89.0, 61.0) val numIntermediatePoints = 100 val samplingDistance = p1.distanceTo(p2) / (numIntermediatePoints + 1) val originalPoints = listOf(p1, p2) val interpolatedPoints = - originalPoints.asSequence().addInterpolatedPoints(samplingDistance).toList() + originalPoints.asFlow().addInterpolatedPoints(samplingDistance).toList() assertEquals( numIntermediatePoints + 2, interpolatedPoints.size, @@ -58,13 +64,13 @@ class GpxImportAddInterpolatedPointsTest { } @Test - fun `ensures promised sampling distance on multiple segments`() { + fun `ensures promised sampling distance on multiple segments`() = runTest { val originalPoints = listOf(LatLon(0.0, 0.0), LatLon(1.0, 1.0), LatLon(2.1, 1.3), LatLon(0.0, 0.0)) val samplingDistance = originalPoints.zipWithNext().minOf { it.first.distanceTo(it.second) } / 100 val interpolatedPoints = - originalPoints.asSequence().addInterpolatedPoints(samplingDistance).toList() + originalPoints.asFlow().addInterpolatedPoints(samplingDistance).toList() assertCorrectSampling(originalPoints, interpolatedPoints, samplingDistance) } diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDetermineBBoxesTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDetermineBBoxesTest.kt index 6a35b2181be..c854ef65106 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDetermineBBoxesTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDetermineBBoxesTest.kt @@ -4,26 +4,32 @@ import de.westnordost.streetcomplete.data.osm.mapdata.BoundingBox import de.westnordost.streetcomplete.data.osm.mapdata.LatLon import de.westnordost.streetcomplete.util.math.area import de.westnordost.streetcomplete.util.math.isCompletelyInside +import kotlinx.coroutines.flow.asFlow +import kotlinx.coroutines.flow.count +import kotlinx.coroutines.flow.emptyFlow +import kotlinx.coroutines.flow.toList +import kotlinx.coroutines.test.runTest import org.junit.Test import kotlin.test.assertEquals import kotlin.test.assertTrue class GpxImportDetermineBBoxesTest { @Test - fun `gracefully handles empty sequence`() { + fun `gracefully handles empty flow`() = runTest { assertEquals( 0, - emptySequence().determineBBoxes().count(), - "empty sequence not retained" + emptyFlow().determineBBoxes().count(), + "empty flow not retained" ) } + @Test - fun `drops duplicates`() { + fun `drops duplicates`() = runTest { val inputBoxes = listOf( BoundingBox(LatLon(-0.01, -0.01), LatLon(0.0, 0.0)), BoundingBox(LatLon(-0.01, -0.01), LatLon(0.0, 0.0)), ) - val downloadBoxes = inputBoxes.asSequence().determineBBoxes().toList() + val downloadBoxes = inputBoxes.asFlow().determineBBoxes().toList() assertEquals( 1, downloadBoxes.size, @@ -45,13 +51,13 @@ class GpxImportDetermineBBoxesTest { } @Test - fun `merges adjacent boxes forming a rectangle`() { + fun `merges adjacent boxes forming a rectangle`() = runTest { val inputBoxes = listOf( BoundingBox(LatLon(0.00, 0.0), LatLon(0.01, 0.01)), BoundingBox(LatLon(0.01, 0.0), LatLon(0.02, 0.01)), BoundingBox(LatLon(0.02, 0.0), LatLon(0.03, 0.01)), ) - val downloadBoxes = inputBoxes.asSequence().determineBBoxes().toList() + val downloadBoxes = inputBoxes.asFlow().determineBBoxes().toList() assertEquals( 1, downloadBoxes.size, @@ -73,13 +79,13 @@ class GpxImportDetermineBBoxesTest { } @Test - fun `partially merges boxes in an L-shape`() { + fun `partially merges boxes in an L-shape`() = runTest { val inputBoxes = listOf( BoundingBox(LatLon(0.00, 0.00), LatLon(0.01, 0.01)), BoundingBox(LatLon(0.01, 0.00), LatLon(0.02, 0.01)), BoundingBox(LatLon(0.00, 0.01), LatLon(0.01, 0.06)), ) - val downloadBoxes = inputBoxes.asSequence().determineBBoxes().toList() + val downloadBoxes = inputBoxes.asFlow().determineBBoxes().toList() assertEquals( 2, downloadBoxes.size, diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDiscardRedundantPointsTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDiscardRedundantPointsTest.kt index 18fea84c02c..dec21c775f1 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDiscardRedundantPointsTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDiscardRedundantPointsTest.kt @@ -2,44 +2,50 @@ package de.westnordost.streetcomplete.data.import import de.westnordost.streetcomplete.data.osm.mapdata.LatLon import de.westnordost.streetcomplete.util.math.distanceTo +import kotlinx.coroutines.flow.asFlow +import kotlinx.coroutines.flow.count +import kotlinx.coroutines.flow.emptyFlow +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.flow.toList +import kotlinx.coroutines.test.runTest import org.junit.Test import kotlin.test.assertEquals import kotlin.test.assertFails class GpxImportDiscardRedundantPointsTest { @Test - fun `fails with bad parameters`() { + fun `fails with bad parameters`() = runTest { assertFails { - emptySequence().discardRedundantPoints(-15.0).count() + emptyFlow().discardRedundantPoints(-15.0).count() } } @Test - fun `gracefully handles small sequences`() { + fun `gracefully handles small flows`() = runTest { assertEquals( 0, - emptySequence().discardRedundantPoints(1.0).count(), - "empty sequence not retained" + emptyFlow().discardRedundantPoints(1.0).count(), + "empty flow not retained" ) assertEquals( 1, - sequenceOf(LatLon(89.9, 27.1)).discardRedundantPoints(77.0).count(), - "size 1 sequence not retained" + flowOf(LatLon(89.9, 27.1)).discardRedundantPoints(77.0).count(), + "size 1 flow not retained" ) assertEquals( 2, - sequenceOf(LatLon(-41.7, -39.8), LatLon(33.1, 78.8)).discardRedundantPoints(20.0) + flowOf(LatLon(-41.7, -39.8), LatLon(33.1, 78.8)).discardRedundantPoints(20.0) .count(), - "size 2 sequence not retained" + "size 2 flow not retained" ) } @Test - fun `keeps non-redundant points`() { + fun `keeps non-redundant points`() = runTest { val originalPoints = listOf(LatLon(10.10, -2.0), LatLon(19.0, -54.4), LatLon(51.04, -71.30)) val samplingDistance = originalPoints.zipWithNext().minOf { it.first.distanceTo(it.second) } val retainedPoints = - originalPoints.asSequence().discardRedundantPoints(samplingDistance).toList() + originalPoints.asFlow().discardRedundantPoints(samplingDistance).toList() assertEquals( originalPoints.size, retainedPoints.size, @@ -48,20 +54,20 @@ class GpxImportDiscardRedundantPointsTest { } @Test - fun `discards single redundant point`() { + fun `discards single redundant point`() = runTest { val originalPoints = listOf(LatLon(10.10, -2.0), LatLon(19.0, -54.4), LatLon(51.04, -71.30)) val epsilon = 0.00001 val samplingDistance = originalPoints.zipWithNext().maxOf { it.first.distanceTo(it.second) } + epsilon assertEquals( originalPoints.size - 1, - originalPoints.asSequence().discardRedundantPoints(samplingDistance).count(), + originalPoints.asFlow().discardRedundantPoints(samplingDistance).count(), "failed to drop redundant point" ) } @Test - fun `discards multiple adjacent redundant points`() { + fun `discards multiple adjacent redundant points`() = runTest { val originalPoints = listOf( LatLon(1.0, 0.0), LatLon(2.0, 0.0), @@ -71,13 +77,13 @@ class GpxImportDiscardRedundantPointsTest { val samplingDistance = originalPoints.first().distanceTo(originalPoints.last()) assertEquals( 2, - originalPoints.asSequence().discardRedundantPoints(samplingDistance).count(), + originalPoints.asFlow().discardRedundantPoints(samplingDistance).count(), "failed to drop redundant point" ) } @Test - fun `discards multiple non-adjacent redundant points`() { + fun `discards multiple non-adjacent redundant points`() = runTest { val originalPoints = listOf( LatLon(1.0, 0.0), LatLon(2.0, 0.0), @@ -89,7 +95,7 @@ class GpxImportDiscardRedundantPointsTest { val samplingDistance = originalPoints[0].distanceTo(originalPoints[2]) assertEquals( 4, - originalPoints.asSequence().discardRedundantPoints(samplingDistance).count(), + originalPoints.asFlow().discardRedundantPoints(samplingDistance).count(), "failed to drop redundant point" ) } diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMapToCenteredSquaresTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMapToCenteredSquaresTest.kt index d44ed346180..0795aa9a1f2 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMapToCenteredSquaresTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMapToCenteredSquaresTest.kt @@ -3,6 +3,11 @@ package de.westnordost.streetcomplete.data.import import de.westnordost.streetcomplete.data.osm.mapdata.LatLon import de.westnordost.streetcomplete.util.math.area import de.westnordost.streetcomplete.util.math.contains +import kotlinx.coroutines.flow.count +import kotlinx.coroutines.flow.emptyFlow +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.flow.toList +import kotlinx.coroutines.test.runTest import org.junit.Test import kotlin.math.pow import kotlin.test.assertEquals @@ -11,17 +16,17 @@ import kotlin.test.assertTrue class GpxImportMapToCenteredSquaresTest { @Test - fun `fails with bad parameters`() { + fun `fails with bad parameters`() = runTest { assertFails { - emptySequence().mapToCenteredSquares(-1.0).count() + emptyFlow().mapToCenteredSquares(-1.0).count() } } @Test - fun `maps correctly`() { + fun `maps correctly`() = runTest { val point = LatLon(12.0, 37.1) val halfSideLength = 100.0 - val squares = sequenceOf(point).mapToCenteredSquares(halfSideLength).toList() + val squares = flowOf(point).mapToCenteredSquares(halfSideLength).toList() assertEquals( 1, squares.size, @@ -34,4 +39,5 @@ class GpxImportMapToCenteredSquaresTest { 10.0, "area not matching" ) - }} + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMergeBBoxesTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMergeBBoxesTest.kt index 32aacd7856d..6c75bdcd7a5 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMergeBBoxesTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMergeBBoxesTest.kt @@ -5,27 +5,33 @@ import de.westnordost.streetcomplete.data.osm.mapdata.LatLon import de.westnordost.streetcomplete.data.osm.mapdata.toPolygon import de.westnordost.streetcomplete.util.math.area import de.westnordost.streetcomplete.util.math.isCompletelyInside +import kotlinx.coroutines.flow.asFlow +import kotlinx.coroutines.flow.count +import kotlinx.coroutines.flow.emptyFlow +import kotlinx.coroutines.flow.toList +import kotlinx.coroutines.test.runTest import org.junit.Test import kotlin.test.assertEquals import kotlin.test.assertTrue class GpxImportMergeBBoxesTest { @Test - fun `gracefully handles empty sequence`() { + fun `gracefully handles empty flow`() = runTest { assertEquals( 0, - emptySequence().mergeBBoxes().count(), - "empty sequence not retained" + emptyFlow().mergeBBoxes().count(), + "empty flow not retained" ) } + @Test - fun `merges same size squares in an L-shape`() { + fun `merges same size squares in an L-shape`() = runTest { val inputBoxes = listOf( BoundingBox(LatLon(0.00, 0.00), LatLon(0.01, 0.01)), BoundingBox(LatLon(0.01, 0.00), LatLon(0.02, 0.01)), BoundingBox(LatLon(0.00, 0.01), LatLon(0.01, 0.02)), ).map { DecoratedBoundingBox(it.toPolygon()) } - val downloadBoxes = inputBoxes.asSequence().mergeBBoxes().toList() + val downloadBoxes = inputBoxes.asFlow().mergeBBoxes().toList() assertEquals( 1, downloadBoxes.size, @@ -41,13 +47,13 @@ class GpxImportMergeBBoxesTest { } @Test - fun `partially merges L-shape with one long leg`() { + fun `partially merges L-shape with one long leg`() = runTest { val inputBoxes = listOf( BoundingBox(LatLon(0.00, 0.00), LatLon(0.01, 0.01)), BoundingBox(LatLon(0.01, 0.00), LatLon(0.02, 0.01)), BoundingBox(LatLon(0.00, 0.01), LatLon(0.01, 0.06)), ).map { DecoratedBoundingBox(it.toPolygon()) } - val downloadBoxes = inputBoxes.asSequence().mergeBBoxes().toList() + val downloadBoxes = inputBoxes.asFlow().mergeBBoxes().toList() assertEquals( 2, downloadBoxes.size, diff --git a/app/test-proguard-rules.pro b/app/test-proguard-rules.pro new file mode 100644 index 00000000000..9bf7f432ddf --- /dev/null +++ b/app/test-proguard-rules.pro @@ -0,0 +1,9 @@ +-dontwarn com.sun.jna.** +-dontwarn java.lang.** +-dontwarn org.junit.** +-dontwarn com.google.errorprone.annotations.MustBeClosed +-dontwarn edu.umd.cs.findbugs.annotations.SuppressFBWarnings +-dontwarn kotlin.reflect.full.* +-dontwarn org.mockito.internal.creation.bytebuddy.inject.MockMethodDispatcher +-dontwarn org.opentest4j.* +-dontwarn org.w3c.dom.bootstrap.DOMImplementationRegistry