[Patch] Various enhancements for GPX correlation

[Bjoeni]

I recently started tagging lots of my photos with existing GPX tracks and wanted to do this with JOSM. Since I combined my tracks from different sources, I wanted to interpolate between the tracks and had to modify the method a bit. Well... then I ran into some more trouble (mainly because of messed up, but according to the TopoGrafix standard perfectly correct GPX files) and decided to put all the modifications in a patch. Even though that is not the main purpose of JOSM (and I would be perfectly fine with the whole tagging stuff being in a plugin), I think that the GPX correlation should work properly (and be usable with all kinds of valid GPX files) if it's a part of JOSM core after all.

So here's what I ended up changing:

• Interpolation of timestamps within segments, if missing (mainly for manually modified GPX tracks)
• Options to interpolate between tracks/segments, depending on distance/time
• Options to determine whether images close to the end of a track should be tagged (instead of a hardcoded value only applying *before* segments)
• Sort tracks and segments in the file by timestamps (and therefore allow correct tagging and interpolation if that's messed up)
• Option to temporarily force tagging of all pictures (by applying the settings above)
• fixed bug that caused problems with empty segments or entire segments without waypoints
• Set elevation and speed only when photo is actually within a track segment or interpolation is activated
• fixed bug that didn't show a preview of the currently tagged photos when the dialog was first opened
• inverted the way timezones are applied (the set timezone must be substracted from the EXIF time in order to get ZULU time, not added and that's also conform with what the setOffset dialog returns)
• use regex to parse timezone and allow more than ±12h, e.g. DST in New Zealand is UTC+13, Samoa can have UTC+14
• add GPX files to the Combobox in the dialog if they are added (as a layer) while the dialog is opened

Ideas that could implemented in the future (by me or anyone else)...

• try to determine the timezone from the loaded pictures by the EXIF 2.31 OffsetTime tags, difference of UTC EXIF tags and timezone specific EXIF tags or the timezone of the modification date, if applicable (i.e. if modification date equals DateTimeOriginal except for the offset)
• allow existing timestamps to be overwritten without removing all existing ones that are not covered by the current track
• allow multiple tracks to be processed and prioritized (from different sources, e.g. GPS + phone + Google Timeline) [cut overlapping parts of less important tracks and replace by more important ones -> merge before handling]

These are quite a lot of changes and I tested everything as far as I could... but GPX files can be quite messed up and I'm open to feedback (regarding bugs, style, whether these changes make sense at all, whatever...).

Because of the JSpinners in the JCheckboxes, I had to split up the strings. Any idea how to handle that for translation? Use trc and copy the other half in the context? Or use a sign inside the string to split it up after translation?

And it might be a good idea to split up the CorrelateGpxWithImages.java, I left it in one file for the time being to keep the patch smaller.

This is the advanced options dialog with the default settings at the moment:

And these are the prefs:

[Don-vip]

Thanks for the patch! :)

Replying to Bjoeni:

Even though that is not the main purpose of JOSM (and I would be perfectly fine with the whole tagging stuff being in a plugin), I think that the GPX correlation should work properly (and be usable with all kinds of valid GPX files) if it's a part of JOSM core after all.

I think it's fine for JOSM core too.

So here's what I ended up changing:

I had a very quick look and have no objection with these change.

These are quite a lot of changes and I tested everything as far as I could... but GPX files can be quite messed up and I'm open to feedback (regarding bugs, style, whether these changes make sense at all, whatever...).

I trust you but given the number of changes, unit tests to ensure there is no regression would be more than welcome.

Also you may be aware we're working to implement a modular structure for JOSM core: #15229

CorrelateGpxWithImages is a perfect example of what we must not do: a GUI class containing a lot of complex logic.

Could you please see if you can extract the correlation logic outside of this class into a new one in the data.gpx package, and add unit tests for it (at least for the code you add)?

Because of the JSpinners in the JCheckboxes, I had to split up the strings. Any idea how to handle that for translation? Use trc and copy the other half in the context? Or use a sign inside the string to split it up after translation?

Good question. Dirk what do you think?

And it might be a good idea to split up the CorrelateGpxWithImages.java, I left it in one file for the time being to keep the patch smaller.

So we have the same feeling :) I'm OK with a bigger patch if it comes with tests ;)

This is the advanced options dialog with the default settings at the moment:

When exactly is it displayed?

[stoecker]

Because of the JSpinners in the JCheckboxes, I had to split up the strings. Any idea how to handle that for translation? Use trc and copy the other half in the context? Or use a sign inside the string to split it up after translation?

Good question. Dirk what do you think?

You simply can't do it this way. While this is fine for English it wont work for a number of other languages. Stick to single texts. Either replace the value by X in the strings or set units in brackets or similar solutions. This in-text-style is untranslatable.

text....: checkbox

[Bjoeni]

Replying to stoecker:

Because of the JSpinners in the JCheckboxes, I had to split up the strings. Any idea how to handle that for translation? Use trc and copy the other half in the context? Or use a sign inside the string to split it up after translation?

Good question. Dirk what do you think?

You simply can't do it this way. While this is fine for English it wont work for a number of other languages. Stick to single texts. Either replace the value by X in the strings or set units in brackets or similar solutions. This in-text-style is untranslatable.

text....: checkbox

That's why I suggested splitting the text on a sign. E.g. the translatable text would then be 'only when the tracks are less than # meters apart', the German translation 'nur wenn die Tracks weniger als # Meter auseinander liegen' and the Spinner will be where the # is (after translation). And it can obviously be at the end, if required by that language. Imo it looks better in any language I speak, but you're the boss ;)

Replying to Don-vip:

Could you please see if you can extract the correlation logic outside of this class into a new one in the data.gpx package, and add unit tests for it (at least for the code you add)?

Yes, I can do that. I'm just trying to figure out the handling of the configuration in the unit tests. Can I just put settings during the test without any side effects, and otherwise the defaults will be used?

This is the advanced options dialog with the default settings at the moment:

When exactly is it displayed?

When the 'Advanced settings' button in the correlation dialog is pressed:

(I can move that button somewhere else, I just put it in a free spot in the dialog. Oh, and I just noticed that the ... probably doesn't make sense there - not sure if you guys have design guidelines for that)

[stoecker]

Replying to Bjoeni:

You simply can't do it this way. While this is fine for English it wont work for a number of other languages. Stick to single texts. Either replace the value by X in the strings or set units in brackets or similar solutions. This in-text-style is untranslatable.

text....: checkbox

That's why I suggested splitting the text on a sign. E.g. the translatable text would then be 'only when the tracks are less than # meters apart', the German translation 'nur wenn die Tracks weniger als # Meter auseinander liegen' and the Spinner will be where the # is (after translation). And it can obviously be at the end, if required by that language. Imo it looks better in any language I speak, but you're the boss ;)

That's the typical answer of most programmers new to i18n. But there are languages where the sentence depends a lot on the value you have (even for English and German it does, but no so much). For them it simply never looks good, because you can't change the text based on the value for variable values. To do so would need to much work. And I'm pretty sure we can find a language where even that would not help (e.g. moving box positions depending on the value :-)

Perfect are language aware interfaces, where you can do things like proposed. As this is not manageable for a tool like JOSM we stick to the second best solution even if the result may not be as fancy: Translatable texts.

[Bjoeni]

I thought that these languages could just put the sign at the end, so that languages where it's more fancy can use it somewhere in the middle, and languages where it doesn't really work restore the usual bahaviour. So basically the closest you can get to language aware interfaces ;)

But tbh I don't really care too much about that... Could you maybe propose a sentence that I could use (in English)?

[Bjoeni]

Replying to Don-vip:

Could you please see if you can extract the correlation logic outside of this class into a new one in the data.gpx package, and add unit tests for it (at least for the code you add)?

Done. I didn't implement unit tests for segments specifically, because the logic is absolutely similar to the tracks. I can add them if required.

I can change the labels if somebody suggests a string that makes somewhat sense. For the time being it is still tr("only when the segments are less than # minutes apart").split("#"); and similar

And do I have to add the new unit test test/unit/org/openstreetmap/josm/data/gpx/GpxImageCorrelationTest.java to an index somewhere?

[Don-vip]

Replying to Bjoeni:

do I have to add the new unit test test/unit/org/openstreetmap/josm/data/gpx/GpxImageCorrelationTest.java to an index somewhere?

No. The tests only need to end with "Test.java" for unit tests, and "TestIT.java" for integration tests (those calling Taginfo and so on), see call-junit macro in build.xml:

<batchtest fork="yes" todir="${test.dir}/report">
    <fileset dir="${test.dir}/build/@{testfamily}" includes="**/*Test@{testITsuffix}.class"/>
</batchtest>

[Bjoeni]

Ah ok, that makes sense. I even had a quick look at the build.xml but didn't see it. Thanks.

Btw I just noticed that there were issues with the new timezone parser (one of which I would have noticed if I had run the according unit test...). Fixed that and added a test for parsing negative values (the one that wasn't caught by the TimezoneTest)

[Don-vip]

Awesome, thank you!

[Don-vip]

@Dirk, a suggestion for proper translatable strings?

[stoecker]

I'd leave the texts like they are: "only when the segments are less than # minutes apart", simply keep them as they are including the hash sign, maybe adding a ":" behind and then the value-box.

P.S. JOSM base is AE not BE, so use meters instead of metres.

[Don-vip]

In 14205/josm:

fix #16681 - Various enhancements for GPX correlation (patch by Bjoeni, modified)

[Don-vip]

In 14209/josm:

see #16681 - spotbugs / pmd

[Don-vip]

In 14210/josm:

see #16681 - sonarqube - squid:HiddenFieldCheck