[Patch] Wrong correlate to gpx behaviour (use an old gpx file version)

[StephaneP]

What steps will reproduce the problem?

1. Load the images and the track.gpx file you can found inside the attached samples archive
2. correlate the images with the gpx (offset is 00:00, enable override position for image with geo location in exif data) You should see something like this:

3. Convert the gpx to a data layer
4. add a point to make a < instead of the straight line

5. export this layer to a gpx file, and overwrite the existing track.gpx
6. Start a new image correlation, click on open another gpx, select track.gpx and correlate (enable override position for image with geo location in exif data)
7. it should be ok.

8. drag and drop the track.gpx on Josm
9. convert the gpx to a data layer and edit the center node to transform the < to a >

10. export this layer to a gpx file, and overwrite the existing track.gpx
11. Start another image correlation, click on open another gpx, select track.gpx (enable override position for image with geo location in exif data)

What is the expected result?

The images should be correlated on the > gpx track.

What happens instead?

The images seem correlated with the previous gpx (with the < instead of the >)

Please provide any additional information below. Attach a screenshot if possible.

I had this bug from time to time when I was editing my gpx files to fix some positions, but it was difficult to create the correct steps to reproduce it.
It seems Josm keep the previous versions in memory, and don't use the new version.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2021-04-23 21:32:22 +0200 (Fri, 23 Apr 2021)
Build-Date:2021-04-24 01:30:58
Revision:17814
Relative:URL: ^/trunk

Identification: JOSM/1.5 (17814 en) Windows 10 64-Bit
OS Build number: Windows 10 Pro 2009 (19042)
Memory Usage: 3727 MB / 7218 MB (1781 MB allocated, but free)
Java version: 1.8.0_191-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920×1080 (scaling 1.00×1.00)
Maximum Screen Size: 1920×1080
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: Cp1252
System property sun.jnu.encoding: Cp1252

Plugins:
+ Mapillary (1.5.37.6)
+ OpeningHoursEditor (35640)
+ PicLayer (2a9aa7a)
+ apache-commons (35524)
+ apache-http (35589)
+ areaselector (368)
+ austriaaddresshelper (1597341117)
+ cadastre-fr (35727)
+ continuosDownload (91)
+ contourmerge (v0.1.6)
+ ejml (35458)
+ geotools (35458)
+ javafx-windows (35655)
+ jaxb (35543)
+ jna (35662)
+ jts (35458)
+ log4j (35458)
+ opendata (35640)
+ photo_geotagging (35715)
+ photoadjust (35640)
+ reverter (35732)
+ tageditor (35640)
+ todo (30306)
+ turnlanes-tagging (288)
+ turnrestrictions (35640)
+ undelete (35640)
+ utilsplugin2 (35691)

Tagging presets:
+ %UserProfile%\Documents\Gitlab\preset-josm-collectivites\Presets-collectivités.xml

Map paint styles:
- https://josm.openstreetmap.de/josmfile?page=Styles/Enhanced_Lane_and_Road_Attributes&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1
- https://raw.githubusercontent.com/species/josm-preset-traffic_sign_direction/master/direction.mapcss
- %UserProfile%\Documents\GitHub\MapCSS-JOSM-Bicycle\cycleway.mapcss

Last errors/warnings:
- 00041.368 E: Failed to locate image 'regulatory--dual-lanes-cyclists-and-pedestrians--g1'
- 00041.852 E: Failed to locate image 'regulatory--texts--g1'
- 00041.853 E: Failed to locate image 'regulatory--texts--g2'
- 00041.942 E: Failed to locate image 'void--car-mount'
- 00041.943 E: Failed to locate image 'void--dynamic'
- 00041.944 E: Failed to locate image 'void--ego-vehicle'
- 00041.944 E: Failed to locate image 'void--ground'
- 00041.945 E: Failed to locate image 'void--static'
- 00042.066 E: Failed to locate image 'warning--kangaroo-crossing--g1'
- 00740.978 W: Unable to delete file H:\bug_gpx_josm_1\2021-04-15_11H59mn42s-Cam_avant-G0052614.JPG

[StephaneP]

some images and track.gpx

[skyper]

Regression?
Does tested version (r17702) work?

[StephaneP]

Replying to skyper:

Regression?
Does tested version (r17702) work?

I don't think so. I noticed this bug a long time ago (several years) but it's the first time i took some time to investigate a little more and create a way to reproduce it.

[Bjoeni]

Ticket #20797 has been marked as a duplicate of this ticket.

[Bjoeni]

patch fixes #20795 and #20797

[simon04]

• The (unrelated) refactoring of the code makes the patch very hard to review
• CorrelateGpxWithImages instances are never removed from org.openstreetmap.josm.gui.layer.geoimage.CorrelateGpxWithImages#instances, and thus will never be garbage collected. Why is this needed in the first place?
• I'd remove the angular brackets from tr("<No GPX track loaded yet>") (those are very rarely used for i18n strings in JOSM)
• Is it feasible to write a unit test for this bugfix?

[Bjoeni]

Replying to simon04:

• The (unrelated) refactoring of the code makes the patch very hard to review

Not sure what exactly you're referring to, the only thing actually unrelated to the patch was renaming of updataStatusBar() to matchAndUpdateStatusbar() and moving boolean forceTags. Everything else was directly related to these two issues.

• CorrelateGpxWithImages instances are never removed from org.openstreetmap.josm.gui.layer.geoimage.CorrelateGpxWithImages#instances, and thus will never be garbage collected. Why is this needed in the first place?

I'm storing the instances in order to close other dialogs when a new one is opened and repaint the Combobox when required. I could have implemented it using event listeners, but it seemed easier this way.

I'm aware that they aren't removed. Originally I only kept the dialogs which didn't sum up to much (so I was planning on implementing it sometime in the future), but then I changed it so that the full instances are kept which leads to the layer not being properly garbage collected either. I'll change that.

• I'd remove the angular brackets from tr("<No GPX track loaded yet>") (those are very rarely used for i18n strings in JOSM)

I just kept the existing translatable String so that it doesn't have to be retranslated in every language.

• Is it feasible to write a unit test for this bugfix?

One of the main issues was the dialog not handling newly loaded GPX tracks properly. So testing that requires mocking which I'm not a big fan of. Someone can implement that, I just don't think it's worth it in this case.

[Bjoeni]

I just had a brief look at it, even in the current stable the GeoImageLayers are never garbage collected because of the non-static layerChangeListener that is never removed. This is actually (by accident) fixed by this patch, since it is static now.

Also I got rid of the static reference to the instances and rely on MainLayerManager instead. I'll upload a new patch for that later.

---

There is however one more issue: The non-static JCombobox registers an eventListener to the static gpxModel which usually never unregisters:

The only way I could get the JCombobox to remove that listener was by setting a new anonymous model (which doesn't really matter in terms of heap allocations but seems quite unneccessary):

    @Override
    public void destroy() {
        if (cbGpx != null) {
            cbGpx.setModel(new DefaultComboBoxModel<GpxDataWrapper>());
            cbGpx = null;
        }
        closeDialog();
    }

Do you have a better idea for that?

---

Also (probably completely unrelated to this patch) GeoImageLayers are still not garbage collected because of a MouseMotionListener added to the MapView:

Even though that should be removed in GeoImageLayer#destroy: https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/gui/layer/geoimage/GeoImageLayer.java?annotate=blame&rev=17548#L851

Do you have any idea what could be causing that? Or should I open a new ticket?

[Bjoeni]

• memory leaks caused by instances and the JComboBox should be fixed

Couldn't really reproduce the issue with the mouseMotionListener. But that's anyways unrelated to this patch.

[Don-vip]

In 18034/josm:

fix #20795 - fix #20797 - fix wrong correlate to gpx behaviour (patch by Bjoeni)

[Don-vip]

In 18124/josm:

see #20795 - see #20797 - make sure initial correlation attempt is performed only after dialog is displayed, otherwise it is simply ignored