Add GPX import functionality

[greuters]

To map along a predefined route, set your cache big enough and export a GPX track from some other routing app by sharing it with StreetComplete.

StreetComplete imports the track to show it on the map, and - if you accept the number of downloads and area - schedules tile downloading along the track for offline usage,

Fixes #473, fixes #1757.

[westnordost]

Continuing from #5345.

(#3898 is related and a use case for - persistently - showing the GPX track. I guess.)

Regarding the persistence: Ha, that's a trap to fall into with Android. Saving instance state is actually not persistent - it would be gone latest when you restart your phone or exit the app (i.e. remove from recents). The track would need to be kept in a SharedPreferences key.
I don't remember writing anything about max 5000 storable trackpoints.
On the question of UI, I think it makes sense to always replace the track when a new track is imported. But how to clear the track? Maybe for that, we actually need a new item in the setting.... at least no better solution comes to mind... or maybe only clear it when the cache is cleared?

Rate limits
I didn't care for (testing) it yet, because there is now way the current download behavior will ever trigger any rate limiting. Only (comparatively) tiny squares are downloaded.
Basically, we should make sure that whatever the OSM API returns as an error code in that case (if there is rate limiting at all?), it will be treated gracefully. (= cancel the download, do not attempt to try again, maybe show an error dialog)

Download distance
Hmm, I see. So I guess at least 1m does not make sense.
On reducing the number of downloads, an idea:
4 tiles of zoom X make up 1 tile of zoom X-1, right. So, if 3/4 tiles in zoom X that make up a tile of zoom X-1 are to be downloaded, maybe expand it to download all 4 tiles, i.e. as if to download a tile from zoom X-1? (Anyway, I still didn't look at your code yet)

UI
Hm wow, that's long. Then, a progress bar definitely makes sense. For the current feature set that you want to support, I think the better UI would be two radio buttons - 1. just display the track and 2. also download along the track. I think this is better than a checkbox because it makes clear what happens when you do not check that box.

Or, taking one step back from a concrete suggestion, it is just important to tell the user what is going to happen. In the current version, it is not clear what is going to happen if you uncheck the download-checkbox.
In any case, the download-distance-selection should only be visible when one wants to download along the track.

[greuters]

@Helium314
What exactly do you mean by "Did you check whether background map tiles are properly downloaded"?
We are testing this for a while now and can do quests along the way like usual, I didn't notice any difference to normal mapping around my location. However, you need to wait until all downloads are complete (progress in upper left corner not spinning any more) and I then usually check if the end of my track has been downloaded, as downloads sometimes fail if we have a bad WIFI connection. Is there any nice programmatic way of doing this check and informing the user?

[greuters]

@westnordost

persistence
So I would store the track e.g. as JSON via SharedPreferences editor upon import, and read it back in MainMapFragment.onMapReady? I think a separate setting to clear the track would make sense, at least if we support displaying the track as optional in the import dialog (see below)

UI
Reading the Android Dev Guide, I think checkboxes are most appropriate - switches should alter state immediately, which we don't do. Checkboxes should give a choice where you can select only one option - but I think track display and download should be independently selectable depending on usecase.

• Someone asked for a track to know where he wants to walk along to map a certain feature -> probably doesn't need to download data, might be better off with just downloading one BBox and displaying the track
• We need the downloaded tiles but overlaying the track is not necessary, as we navigate with a different app. Not showing it avoids additional clutter on the map.
  Would the following solution work?

1. by default, both checkboxes are enabled
   
2. deselecting "Download tiles along track" makes the selector disappear and collapse
   
3. deselecting both checkboxes disables the OK button - import makes no sense, as nothing would happen
   

[westnordost]

JSON is too verbose. You could just save it in a comma-separated string and then "parse" it again from that. E.g. the string could be "lat1,lon1;lat2,lon2;lat3,lon3;...".

You mention switches, but I mean radio button. Does the same apply there? There are plenty of quests where there are a couple of radio button and the answer (of course) only gets applied after one taps the ok button. In any case, is there a use case for wanting to download the route, but not display the route?

I fear I am getting into the nitpicking phase, but maybe it would look better than this huge spinner if the distance would be a number input within the label? I.e. (o) Download data [ ] meters around track

Oh, one thing came to my mind. US-Americans. You know, foot, inches, yards... hm.

[greuters]

Yes, radiobuttons would work as well. I think this boils down to the question if not displaying the track is a legitimate scenario - for my use case I'm not sure yet, as we didn't display it so far. I'll know in a few weeks if it is actually helpful or distracting.

About the nitpicking: all good, as long as it keeps being as constructive <3
Probably a slider would be a better fit (if the "250m" label above the slider should be showing can be configured):

Regarding the units: is there any builtin way to find out which ones are used? In the preferences the only localization option obvious to me is language choice..

[Helium314]

@Helium314 What exactly do you mean by "Did you check whether background map tiles are properly downloaded"? We are testing this for a while now and can do quests along the way like usual, I didn't notice any difference to normal mapping around my location. However, you need to wait until all downloads are complete (progress in upper left corner not spinning any more) and I then usually check if the end of my track has been downloaded, as downloads sometimes fail if we have a bad WIFI connection. Is there any nice programmatic way of doing this check and informing the user?

This is about the background map, it has nothing to do with quests.
Starting a download does 3 things: download OSM notes, download OSM data, download map tiles (vector tiles from jawg).
Notes and OSM data are a single download each, and the dowload will fail if one of them fails.
Map tiles are downloaded independently, i.e. one download per map tile. Failed individual downloads are logged, but don't cause the whole download process to fail.
Failed map tile downloads are usually not an issue, as download will be tried again when the area is in view. But the whole GPX download thing intends to download data for offline use, otherwise you could just rely on auto-download.

When doing many successive downloads to pre-load an area, which essentially is a manual version of what this PR does, I sometimes notice missing map tiles. Answering quests with a missing background map is still possible, but often it's quite hard to identify the element the quest is asking for.
So that's why I think it's relevant to ensure the map tiles are downloaded.

[westnordost]

@Helium314 But what should be done if the download fails for one tile? I mean, it could only fail because the server from which it is downloaded for one reason or another doesn't return it (in time). Certainly, the whole download should not be cancelled for that. So, I am not really sure what we can do in that case, other than ignore it.

@greuters You can do this (kind of pseudo-code):

val p = gpxTrack.boundingBox.center
val countryInfo = countryInfos.getByLocation(countryBoundaries, p.longitude, p.latitude)
val lengthUnit = countryInfo.lengthUnits.first()
when (lengthUnit) {
    METER: displayAsMeter()
    FOOT_AND_INCH: displayAsYards() // or some other unit??
}

So this does change the unit according to the location of the GPX track... maybe it would be better to change the unit according to the user's locale?
In that case, it should be possible to access a CountryInfo also by ISO country code extracted from the user'sLocale. Not sure if the user Locale always includes the country code, though. But anyway, I guess if it is not present, just default to metric system, anyway...

[matkoniecz]

But what should be done if the download fails for one tile?

Retry some limited number of times?

[greuters]

@westnordost Nice, that sounds like a good solution for the Locale, I'm gonna try that.

To ensure all tiles are downloaded like @Helium314 suggests, I think a combination of using a retryLimit and a flag ensureTilesDownloadSuccess in Downloader.download could be a sensible way. If mapTilesDownloader.download reports any failed tiles, it could be started over a number of times (e.g. to recover from intermittent connection problems). If it fails too often, a ConnectionException or similar could be thrown which triggers an error shown to the user. Maybe a toast is not appropriate though, some message might be better because these downloads can take quite a while and the user probably doesn't observe his phone all the time. At least this way the user would know that some downloads failed, and could also choose to restart the whole import manually.

[mnalis]

Oh, one thing came to my mind. US-Americans. You know, foot, inches, yards... hm.

That luckily can be kludged by translation alone in this particular case! 😃

yards and meters are close enough that can be used interchangeably in situation like this (where the value is approximate anyway, and we don't really care if that 100m radius actually turns out to be 108m)

[greuters]

Oh, one thing came to my mind. US-Americans. You know, foot, inches, yards... hm.

That luckily can be kludged by translation alone in this particular case! 😃

yards and meters are close enough that can be used interchangeably in situation like this (where the value is approximate anyway, and we don't really care if that 100m radius actually turns out to be 108m)

Hmm, I think this does not hold for the confirmation dialog though - probably the appropriate conversion would be to acres -> 1 km^2 = 247,105 acres

[mnalis]

Hmm, I think this does not hold for the confirmation dialog though

does the confirmation dialog need the total area, though? I would think that number of areas to download would be indicative enough to allow user to choose whether to proceed or not.

[greuters]

Hmm, I think this does not hold for the confirmation dialog though

does the confirmation dialog need the total area, though? I would think that number of areas to download would be indicative enough to allow user to choose whether to proceed or not.

Yes, I'd say some indication is needed. It is basically a tradeoff between number of downloads and download size - if you set the download distance to high values, you max out on the area you are downloading. One option might be to show the number of unique tiles instead, but that is less intuitive for users I guess.

Probably lifting to API level 24 is not around the corner? This would provide a nice MeasureUnit type.. I think I will stick to a simple proposal for the moment though.

[Helium314]

But what should be done if the download fails for one tile?

Retry some limited number of times?

That was my idea, and possibly inform the user if some tiles fail after the retry limit.
Further (and that's why I asked whether @greuters also noticed this) I had the impression that tiles were relatively often missing when mass-downloading, so there might be some kind of download rate limit that should be considered.

Actually, cache size and average tile size could be used to determine whether downloading all tiles makes sense at all, i.e. everything fits in cache.
Here maplibre would be really nice to have. I remember there was some possibility to lock map tiles so they would not get deleted even if cache size is exceeded.

Sorry for always pushing background map things, but I encountered missing map tiles too frequently, and they are really annoying (and will cause users to open issues).

[westnordost]

While the topic of retrying downloading tiles is certainly a useful issue to solve, I do not think it is integral to this PR. It could be a separate PR.

On MapLibre, my last information on it is that an experimental implementation has been done but you are concerned about performance and that you want to wait on some API change in an upcoming major version before continuing, while enocouraging that someone else could already port the StreetComplete style to MapLibre. (For the latter, I think there could be a ticket with "help wanted", but only if we know that the port will actually be done, otherwise whoever does that port might end up feeling cheated, i.e. invested time for nothing)
Anyway, off topic.

Another thing that was mentioned was an enhancement of the sync-notification on Android, adding a "cancel" button to stop a (too large) download. While this would be very nice, especially with the download times and sizes we are talking about here, I too think that this is not essential to this PR and could be a separate PR.

I'll have a more detailed look over the PR today and add some comments about the code.

[westnordost]

I've not had a detailed look at the actual GpxImporter / GpxTrackSampler yet, however, from looking at it generally, I see there are some issues with this. Rather than to tell you the issues, I will outline how it is supposed to be:

A Sequence in Kotlin is what a Stream is in Java, i.e. lazily evaluated iterable. This is very handy for when you have, say, something like a parser, which emits data classes from a continuous input stream. E.g. the difference between

fun parseGpx(): List<Trackpoint>

and

fun parseGpx(): Sequence<Trackpoint>

is that the former would load the entire list of trackpoints into memory first (consumes the input stream till the end), while the latter only consumes it when the sequence itself is consumed, i.e. turned into a list. Very handy when handling large data, but not important when handling small data. See https://kotlinlang.org/docs/sequences.html for a better explanation.

A Flow is basically a "coroutine-enabled" Sequence. I believe this is what you would like to have. Though I have to admit, I did not look properly into Flows yet and it is also not used anywhere in the app yet, so if you want to stay on terrain where I can actually help you, you could first do it with sequences and only later use flows instead. (The API should be similar). Docs: https://kotlinlang.org/docs/flow.html

So, the parsing should look like this:

1. Do not use a parser class that is part of a third party library. The parser is specific to its use case and also only parses GPX track as outputted by the OSM API. Other GPX can be (a little) more complex, more nested. Also, it's non-flow/sequence aware Java-code, to retrofit that into this kind of code is making things needlessly complex

2. The GPX parser emits a Flow of trackpoints from an input stream, nothing more. It can be tested in isolation. fun parseGpx(inputStream: InputStream): Flow<Trackpoint>. And since it's actually possibly Flow<Track> with Track itself consisting of a flow of TrackSegments, each of which also consists of a flow of TrackPoints according to the GPX schema definition
   , it's fair enough that the function does not do anything more, as it is more enough (to test)

3. The flow can then be further processed, functional style. E.g. in kinda pseudo code (assuming a sequence, because that's what I am used to):

val allTheSampledTrackpoints: Sequence<Sequence<Sequence<Trackpoint>>> = parseGpx(in)
  .map { track -> 

    track.map { segment ->

      val lastPoint = null

      segment.flatMap { point ->

        if (lastPoint == null) sequenceOf(point)
        else sampleAtMaxDistanceOf(maxDistance, lastPoint, point).toSequence()

        lastPoint = point
      }
    }
  }

where sampleAtMaxDistanceOf is a function that returns a list of trackpoints in between and including the two passed points where each point is maxDistance or less apart. This function can also be tested in isolation.

4. In the next step, the sequence (of sequence of sequence, as per the nested structure of a gpx) can be .flatMaped to tile positions. Alternatively, this could be done without this step in-between, I am just mirroring in this example how I understand is your data flow.

[westnordost]

I fear this will not work. Unfortunately. You cannot pass any parameters to any Fragment in the constructor. The reason is that the Android system is allowed to destroy fragments and activities at any point (e.g. when you tab out and in again) and re-create them (with empty constructor) later.

I think in general the whole setup of deferring is incorrect / "not the kotlin way", which just makes the whole thing more complex as it needs to be. The UI should be clearly separated from the data (flow). See my general comment for more details.

[westnordost]

Sorry to ask you to basically re-implement how you laid out the code, better now than later, though, since you didn't write tests yet. I suggest you leave the UI be for now and focus on the data first - parsing, processing and each unit testing these in isolation. That should give you a stable base to work with when integrating it into the UI.

[greuters]

Thanks for looking into my code and giving detailed feedback. I addressed the small issues in a commit and will have a go at restructuring the importer with sequences, just one question:

Do not use a parser class that is part of a third party library.

Should I just avoid using the GpxTrackParser, or do you mean to drop using an XML library at all? XmlPullParserFactory seems to be one of the recommended choices for Android according to https://developer.android.com/develop/connectivity/network-ops/xml

To be able to display the track we will anyway need to keep a full list of all original trackpoints in memory I think, so maybe it would make sense to just load this list with a custom GpxImportParser (extends XmlParser) and then continue from this list.asSequence() to do the whole resampling?

[westnordost]

I meant just the GpxTrackParser. The XmlPullParser is included in Android, if I remember correctly. So, no additional dependency needed.

so maybe it would make sense to just load this list with a custom GpxImportParser (extends XmlParser) and then continue from this list.asSequence() to do the whole resampling?

Mh, not sure if I understand you correctly. You have a point that the whole list of points needs to be kept in memory anyway. However, returning a sequence (and maybe a flow) is not actually harder to do than returning a list. May be even something like 1-2 lines less code, actually. And as you (and me) would like the loading to be cancelled if the UI for it is destroyed, at the end, a flow seems to be what we want.

Anyway, do not extend some generic parser. Note how also the code example in the link you posted does not extend a XmlParser, instead, it uses the parser. As Java devs, we are maybe used to inherit in order to inherit functionality (for code sharing), but it is in fact not such a good idea.

[westnordost]

Hm actually, does the import need to be cancelled if the dialog is closed? The whole thing - load gpx, sample gpx, determine to-be-downloaded tiles could actually be part of the downloading, i.e. is being done in the background, couldn't it?

Then the confirmation step would be omitted, but it could be replaced by showing an actual progress bar in the download notification and a button to cancel it.

But anyway, also the download that happens in the background should be cancellable, one way or another.

[greuters]

Ok, I think that is something I can work with. So I'll only use the XmlPullParser and work on sequences for the moment (changing for flow in a next step). I'm coming from Java, so yes, this kind of stuff needs a bit of rethinking and learning on my side ;)

[westnordost]

Great to hear! I am also coming from Java, and the first time I read the examples in

https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.sequences/sequence.html

... I was like... "whoa! What??" Well, I hope you share my enthusiasm for that!

[greuters]

I started changing my backend code to use sequences and get rid of parser dependencies, but didn't get so far yet. As our stationary time in Puerto Escondido is coming to an end, I don't know when I will continue with this work exactly. Latest by end of February, when we will take a while off from bicycle touring once again in Arizona. So I just wanted to share my intermediate state and keep you updated of my plans.

@westnordost can you have a glance at GpxImportKt#importGpx some time, is this more or less the approach you had in mind?

Of course the interface to the UI still has to be adapted as well, and I did not come up with a good solution to safely open / close the file and return a Sequence yet, hence parseSingleGpxTrack(inputStream: InputStream) still returns a list.
If it would work on a Sequence, the call would return immediately after calling yield for the first time, which closes the inputStream if opened with inputStream.use and then throws an exception when requesting the second element of the sequence. Is there a "Sequence-proof" version to open an InputStream?

[westnordost]

No problem, this PR can stay as a draft here for a while.

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.

Uf, I wouldn't do that. Because of this, my pseudo code earlier mentioned stuff like Sequence<Sequence<Sequence<LatLon>>>: One GPX file can contain several tracks which each can contain several track segments which in turn contain several positions. And it is not really a lot more complex to parse this.
It is totally imaginable that software with which you plan your future biking and hiking tour creates several tracks and also several segments within tracks.
Now, handling it onwards might be a bit more work, but ideally it should not be more than wrapping it in some .map and at the end (when collecting the tiles) flatMap or flatten them together again.

Otherwise, it looks about right. Regarding

// TODO sgr: if this should return a sequence, would need to pass e.g. a Sequence 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

Hm, right. Well, wouldn't the solution be to (not use .use but) close the input stream in the very last line within the sequence { ... } block? (Wrapped in try-catch of course). Because this and nothing more is what the .use { ... } block does.

The overall structure of it is exactly what I had in mind 👍 . The singular mapping functions should also be testable in isolation (this way).

Looking at the test, it seems to be unnecessarily complicated to me. Let's assume that the Android XML parser behaves correctly and instead of mocking it, just test the parser with actual XML strings. (If necessary, the test needs to be moved to androidTest.) Also easier to read.

[greuters]

I finally got around to put some more work into this. The current state is as
follows:

• The whole backend works with flows now - can you check if that looks ok to you @westnordost?

• I moved end-to-end / xml parsing tests to androidTest to make them work without mocking XmlPullParser
  With Robolectrics it would also be possible to run them as unit tests and get near 100% test coverage, but need a new dependency (see intermediate commit #86d3703aa9bcfcf74629ac0832c7d3a22191a140)

• My next step would be working on the UI. Currently it works the same as I showed initially

  1. Settings dialog
  2. Parsing the file once to compute the number of downloads / area to download and load the new track. Show this information in a confirmation dialog s.t. the user can cancel or continue
  3. Parse the file a second time to download in the background, without any interface to cancel at the moment

How would you like to have the UI? Should there be a 'Downloading' dialog showing progress, any error/success messages and have a Cancel button? Maybe if we do this we could skip the confirmation dialog, as the user can simply abort if it takes to long download.

[westnordost]

Cool, congratulations!

Unfortunately I don't have too much time right now and your PR is quite a mouthful so I can't review it soon in depth, maybe someone else would also review this?

A few remarks from reading this vertically:

• you should solve the conflicts with master

• soon-ish, several refactorings are in order or in progress on master

  1. we are migrating from Android views to ViewModels + Jetpack Compose, bit by bit: Migrate to Jetpack Compose (master) #5616. The code looks totally different from classical Android UI code (see for example LogsFiltersDialog.kt)
  2. we are migrating away from Java XmlPullParser + HttpUrlConnection to Ktor Client + xmlutil. It is actually done, and it includes among other things a gpx file writer, but I am not content with the performance yet, so this is on hold for now: Multiplatform OSM API client #5686
  3. The map renderer Tangram-ES is being replaced, too, with MapLibre. This is also already done, but also on hold due to a (for us) critical bug in MapLibre: Replace Tangram with Maplibre #5693.

  now, to be honest, not sure how this would affect you unless the PRs and issues above are merged to master very soon (which is unlikely), but it does affect me, because it means that I'll have to rewrite quite a bit of your code, e.g. the GPX parser would need to be based on xmlutil instead of XmlPullParser, any business logic added to any Fragment needs to be separated out into a ViewModel while only the view logic remains plus the view code needs to be rewritten in Compose. Finally, the ImportedTrackMapComponent needs to be rewritten, too. So, this looks like quite a bit of work (for me) :-/. Maybe to alleviate my workload later down the line, you could use xmlutil instead of the Android XmlPullParser to parse the GPX (see Multiplatform OSM API client #5686 for some other xmlutil-based parsers)? Then, the test for that could also go into the normal unit tests instead of the Android unit tests. The view stuff you could already (re)write in Compose now, but this is probably too much to ask as some learning is required to grok Compose.

[westnordost]

Regarding the UI, your last screenshot looks nice. I think I didn't criticize it, right?

[greuters]

Nice, thanks for your thoughts. Regarding the UI, I thought about these comments:

Hm actually, does the import need to be cancelled if the dialog is closed? The whole thing - load gpx, sample gpx, determine to-be-downloaded tiles could actually be part of the downloading, i.e. is being done in the background, couldn't it?

Then the confirmation step would be omitted, but it could be replaced by showing an actual progress bar in the download notification and a button to cancel it.

But anyway, also the download that happens in the background should be cancellable, one way or another.

but if you are good with the way it is right now, I would just keep it that way for the time being. Thinking about @Helium314 's valid comments regarding missing tiles, I guess that would be a good improvement to come (actually keeping a dialog open while downloading, showing progress and an error message if tile download was not entirely successful in the end even with retries).

So to summarize, I'll be working on the following for now:

1. get persistency of the imported track right
2. migrate the parser to xmlutil
3. keep merge conflicts at bay ;)

[Helium314]

Thinking about @Helium314 's valid comments regarding missing tiles

I think you don't need to bother any more, as this is handled differently when #5693 is merged.
MapLibre should more reliable here, as download is re-tried. And you don't have such direct control over downloading single tiles anyway as far as I know.

[westnordost]

• MapLibre has been merged to master now.
• usage of xmlutil has been merged to master now too.

[westnordost]

The main screen has been migrated to ViewModels + Compose now. This means that the UI, i.e. the dialog, would need to be redone.

IIRC the loading of a GPX track is done via an Android Intent, right? See MainActivity::onNewIntent. The MainViewModel will then parse the GPX and have a StateFlow of, well, I guess just the fact that a GPX is available for download/import along. The MainScreen, analogous to e.g. how it is done for urlConfig, will then display the "Import GPX track" dialog. On confirmation, a function on the view model will be called, which triggers the download.

[westnordost]

I noticed only now that you force-pushed this two months ago and that indeed the xml parsing is based on the xmlutil now, plus, the rendering of the track is based on maplibre now. Tell me if anything (any part) should be reviewed.

Edit: typo, I meant reviewed, not removed.