Refactored to use sequences and added tests

Now testing with RobolectricTestRunner instead of mocking XmlPullParser

app/build.gradle.kts

@@ -116,6 +116,7 @@ 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")
 
     androidTestImplementation("androidx.test:runner:1.5.2")
     androidTestImplementation("androidx.test:rules:1.5.0")

app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImportParse.kt

@@ -8,48 +8,72 @@ import java.io.IOException
 import java.io.InputStream
 
 private const val TRACK_POINT = "trkpt"
-private const val TRACK = "trk"
+private const val SEGMENT = "trkseg"
+
+typealias TrackSegment = Sequence<LatLon>
 
 /**
- * Parses the track points of the first track from a GPX file.
+ * Parses all track points from a GPX file.
  *
- * As the standard doesn't require multiple tracks to be connected, and this lightweight parser
- * function is intended to provide a single consecutive track, further tracks are ignored.
- * Multiple track segments within the first track are parsed however, assuming any gaps are
- * negligible and can be interpolated meaningfully during subsequent processing.
+ * Yields consecutive segments. Track points within a TrackSegment can be interpolated meaningfully
+ * during subsequent processing, while track points from different TrackSegments cannot be assumed
+ * to be connected.
  *
  * @param inputStream valid XML according to http://www.topografix.com/GPX/1/1 schema
- */
-fun parseSingleGpxTrack(inputStream: InputStream): List<LatLon> {
-    // TODO sgr: if this should return a sequence, would need to pass e.g. a Sequence<String> from
-    // File.useLines to make sure the file is correctly closed; inputStream.use closes it
-    // immediately upon return, leading to an exception when requesting the next element of the sequence
-    inputStream.use {
-        val xpp = Xml.newPullParser()
-        xpp.setInput(inputStream, "UTF-8")
-        try {
-            return parseSingleGpxTrack(xpp).toList()
-        } catch (e: Exception) {
-            throw e;
-        }
-    }
-}
-
-/**
- * @param parser a pull parser with input already set, ready to call nextTag
+ * Note: the caller is responsible to close the inputStream as appropriate;
+ * calls to the resulting sequence after closing the inputStream may fail.
+ * @return a sequence of TrackSegments, i.e. sequences of track points logically connected.
+ *
+ * Note: The nested sequences returned work on a single input stream, thus make sure to exhaust
+ * any TrackSegment first before proceeding to the next one. A TrackSegment will not yield any
+ * more track points once you proceed to the next TrackSegment in the outer sequence, thus e.g.
+ * result.toList().first().toList() will not yield any element, while
+ * result.flatMap { it.toList() }.toList() will.
  */
 @Throws(XmlPullParserException::class, IOException::class)
-fun parseSingleGpxTrack(parser: XmlPullParser): Sequence<LatLon> = sequence {
+fun parseGpxFile(inputStream: InputStream): Sequence<TrackSegment> {
+    val parser = Xml.newPullParser()
+    parser.setInput(inputStream, "UTF-8")
+
     parser.setFeature(XmlPullParser.FEATURE_PROCESS_NAMESPACES, true)
 
     parser.nextTag()
     parser.require(XmlPullParser.START_TAG, "http://www.topografix.com/GPX/1/1", "gpx")
 
+    return sequence {
+        var depth = 1
+        while (depth != 0) {
+            when (parser.next()) {
+                XmlPullParser.END_TAG -> {
+                    depth--
+                }
+
+                XmlPullParser.START_TAG -> {
+                    if (parser.name == SEGMENT) {
+                        // segment is closed while parsing, thus depth remains the same
+                        yield(
+                            parseSegment(parser)
+                        )
+                    } else {
+                        depth++
+                    }
+                }
+
+                XmlPullParser.END_DOCUMENT -> {
+                    break
+                }
+            }
+        }
+    }
+}
+
+@Throws(XmlPullParserException::class, IOException::class)
+private fun parseSegment(parser: XmlPullParser): TrackSegment = sequence {
     var depth = 1
     while (depth != 0) {
         when (parser.next()) {
             XmlPullParser.END_TAG -> {
-                if (parser.name == TRACK) {
+                if (parser.name == SEGMENT) {
                     return@sequence
                 }
                 depth--

app/src/main/java/de/westnordost/streetcomplete/screens/main/MainFragment.kt

@@ -1221,7 +1221,8 @@ class MainFragment :
             }.getOrNull() ?: return@launch
 
             if (importData.displayTrack) {
-                fragment.replaceImportedTrack(importData.trackpoints)
+                // 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)
             }
             if (importData.downloadAlongTrack) {
                 if (importData.areaToDownloadInSqkm > ApplicationConstants.MAX_DOWNLOADABLE_AREA_IN_SQKM * 25) {
@@ -1234,7 +1235,6 @@ class MainFragment :
                             lengthUnit
                         ) { cont.resume(it) }.show()
                     }
-
                     for (bBox in importData.downloadBBoxes) {
                         downloadController.download(bBox)
                     }

app/src/main/java/de/westnordost/streetcomplete/screens/main/controls/GpxImportSettingsDialog.kt

@@ -114,7 +114,7 @@ class GpxImportSettingsDialog(
                 binding.displayTrackCheckBox.isChecked,
                 binding.downloadCheckBox.isChecked,
                 minDownloadDistanceInMeters()
-            ) { p -> withContext(Dispatchers.Main) { binding.importProgress.progress = p } }
+            )
         }
         val importData = worker!!.await()
 

app/src/main/java/de/westnordost/streetcomplete/screens/main/map/MainMapFragment.kt

@@ -316,7 +316,7 @@ class MainMapFragment : LocationAwareMapFragment(), ShowsGeometryMarkers {
     }
 
     /* -------------------------------- Show imported tracks ------------------------------------ */
-    fun replaceImportedTrack(trackpoints: List<LatLon>) {
+    fun replaceImportedTrack(trackpoints: List<List<LatLon>>) {
         importedTrackMapComponent?.replaceImportedTrack(trackpoints)
     }
 }

app/src/main/java/de/westnordost/streetcomplete/screens/main/map/components/ImportedTrackMapComponent.kt

@@ -12,16 +12,17 @@ class ImportedTrackMapComponent(ctrl: KtMapController) : KoinComponent {
     private val trackLayer = ctrl.addDataLayer(IMPORTED_TRACK_LAYER)
 
     fun replaceImportedTrack(
-        trackpoints: List<LatLon>,
+        trackpoints: List<List<LatLon>>,
     ) {
+
         trackLayer.clear()
         trackLayer.setFeatures(
-            listOf(
+            trackpoints.map { it ->
                 Polyline(
-                    trackpoints.map { it.toLngLat() }.toMutableList(),
+                    it.map { it.toLngLat() }.toMutableList(),
                     mutableMapOf("type" to "line")
                 )
-            )
+            }
         )
     }
 

app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportAddInterpolatedPointsTest.kt

@@ -0,0 +1,105 @@
+package de.westnordost.streetcomplete.data.import
+
+import de.westnordost.streetcomplete.data.osm.mapdata.LatLon
+import de.westnordost.streetcomplete.util.math.distanceTo
+import org.junit.Test
+import kotlin.test.assertContains
+import kotlin.test.assertEquals
+import kotlin.test.assertFails
+import kotlin.test.assertTrue
+
+class GpxImportAddInterpolatedPointsTest {
+    @Test
+    fun `fails with bad parameters`() {
+        assertFails {
+            emptySequence<LatLon>().addInterpolatedPoints(-15.0).count()
+        }
+    }
+
+    @Test
+    fun `gracefully handles small sequences`() {
+        assertEquals(
+            0,
+            emptySequence<LatLon>().addInterpolatedPoints(1.0).count(),
+            "empty sequence invents points"
+        )
+        assertEquals(
+            1,
+            sequenceOf(LatLon(89.9, 27.1)).addInterpolatedPoints(77.0).count(),
+            "size 1 sequence invents points"
+        )
+    }
+
+    @Test
+    fun `does not add unnecessary points`() {
+        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()
+        assertEquals(originalPoints.size, interpolatedPoints.size)
+    }
+
+    @Test
+    fun `ensures promised sampling distance on single segment`() {
+        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()
+        assertEquals(
+            numIntermediatePoints + 2,
+            interpolatedPoints.size,
+            "wrong number of points created"
+        )
+        assertCorrectSampling(originalPoints, interpolatedPoints, samplingDistance)
+    }
+
+    @Test
+    fun `ensures promised sampling distance on multiple segments`() {
+        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()
+        assertCorrectSampling(originalPoints, interpolatedPoints, samplingDistance)
+    }
+
+    private fun assertCorrectSampling(
+        originalPoints: List<LatLon>,
+        interpolatedPoints: List<LatLon>,
+        samplingDistance: Double,
+    ) {
+        // some tolerance is needed due to rounding errors
+        val maxToleratedDistance = samplingDistance * 1.001
+        val minToleratedDistance = samplingDistance * 0.999
+
+        val distances = interpolatedPoints.zipWithNext().map { it.first.distanceTo(it.second) }
+        distances.forEachIndexed { index, distance ->
+            assertTrue(
+                distance <= maxToleratedDistance,
+                "distance between consecutive points too big; $distance > $maxToleratedDistance"
+            )
+
+            // the only distance that may be smaller than samplingDistance is between the last
+            // interpolated point of one segment and the original point starting the next segment
+            if (distance < minToleratedDistance) {
+                assertContains(
+                    originalPoints, interpolatedPoints[index + 1],
+                    "distance between two interpolated points too small ($distance < $minToleratedDistance"
+                )
+            }
+
+        }
+
+        originalPoints.forEach {
+            assertContains(
+                interpolatedPoints, it,
+                "original point $it is missing in interpolated points"
+            )
+        }
+    }
+}

app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDetermineBBoxesTest.kt

@@ -0,0 +1,102 @@
+package de.westnordost.streetcomplete.data.import
+
+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 org.junit.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertTrue
+
+class GpxImportDetermineBBoxesTest {
+    @Test
+    fun `gracefully handles empty sequence`() {
+        assertEquals(
+            0,
+            emptySequence<BoundingBox>().determineBBoxes().count(),
+            "empty sequence not retained"
+        )
+    }
+    @Test
+    fun `drops duplicates`() {
+        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()
+        assertEquals(
+            1,
+            downloadBoxes.size,
+            "failed to merge all boxes into one"
+        )
+        val downloadBBox = downloadBoxes[0].boundingBox
+        inputBoxes.forEach {
+            assertTrue(
+                it.isCompletelyInside(downloadBBox),
+                "input bounding box $it is not contained in download box $downloadBBox"
+            )
+        }
+        assertEquals(
+            inputBoxes[0].area(),
+            downloadBBox.area(),
+            1.0,
+            "area to download is not the same as area of one input box"
+        )
+    }
+
+    @Test
+    fun `merges adjacent boxes forming a rectangle`() {
+        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()
+        assertEquals(
+            1,
+            downloadBoxes.size,
+            "failed to merge all boxes into one"
+        )
+        val downloadBBox = downloadBoxes[0].boundingBox
+        inputBoxes.forEach {
+            assertTrue(
+                it.isCompletelyInside(downloadBBox),
+                "input bounding box $it is not contained in download box $downloadBBox"
+            )
+        }
+        assertEquals(
+            inputBoxes.sumOf { it.area() },
+            downloadBBox.area(),
+            1.0,
+            "area to download is not the same as area of input boxes"
+        )
+    }
+
+    @Test
+    fun `partially merges boxes in an L-shape`() {
+        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()
+        assertEquals(
+            2,
+            downloadBoxes.size,
+            "failed to merge one side of the L-shape"
+        )
+        val downloadBBoxes = downloadBoxes.map { it.boundingBox }
+        inputBoxes.forEach {
+            assertTrue(
+                downloadBBoxes.any { downloadBBox -> it.isCompletelyInside(downloadBBox) },
+                "input bounding box $it is not contained in any download box $downloadBBoxes"
+            )
+        }
+        assertEquals(
+            inputBoxes.sumOf { it.area() },
+            downloadBBoxes.sumOf { it.area() },
+            1.0,
+            "area to download is not the same as area of input boxes"
+        )
+    }
+}