[Patch] JOSM asks to save unmodified GPX files when opening them through a saved session

[xeen]

steps to reproduce:

1. Start JOSM
2. open any GPX file
3. enable expert mode
4. save session as
5. close JOSM. Observe: no dialog to save GPX
6. Start JOSM again
7. open previously saved session
8. close JOSM. Observe: it asks to save/upload the supposedly modified GPX file

Note that the STR above are just one way to trigger this bug. As far as I've tested, it is triggered for any GPX file opened through a saved session, and how you arrive there is not important (i.e. the .jos file doesn't have a wrong attribute on the <layer> tag for the GPX file).

Status report below is with a fresh ~/.josm directory.

Revision:17702
Is-Local-Build:false
Build-Date:1969-12-10 07:20:05
Debian-Release:0.0.svn17702+dfsg-1~exp1
Build-Name:Debian

Identification: JOSM/1.5 (17702 Debian en) Linux Debian GNU/Linux bullseye/sid
Memory Usage: 350 MB / 4096 MB (255 MB allocated, but free)
Java version: 13.0.5.1+1-Debian-1, Debian, OpenJDK 64-Bit Server VM
Look and Feel: javax.swing.plaf.metal.MetalLookAndFeel
Screen: :0.0 3840×2160 (scaling 2.00×2.00)
Maximum Screen Size: 3840×2160
Best cursor sizes: 16×16→16×16, 32×32→32×32
Environment variable LANG: en_US.UTF-8
System property file.encoding: UTF-8
System property sun.jnu.encoding: UTF-8
Java ATK Wrapper package: libatk-wrapper-java:all-0.38.0-2
libcommons-compress-java: libcommons-compress-java:all-1.20-1
libcommons-logging-java: libcommons-logging-java:all-1.2-2
fonts-noto: fonts-noto:all-20201225-1
liboauth-signpost-java: liboauth-signpost-java:all-1.2.1.2-3
VM arguments: [-Djosm.restart=true, -Djava.net.useSystemProxies=true]

[skyper]

Note: Debian is in freeze, atm, which means even backport packages are not upgraded.

Does this also happen with our own josm-latest (ubuntu) deb-package, which can be installed along side the official tested josm package from Debian, see Download.

[xeen]

It is reproducible with the latest devel version started through java -jar josm-latest.jar:

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2021-05-17 21:27:21 +0200 (Mon, 17 May 2021)
Revision:17903
Build-Date:2021-05-18 01:31:02

Java version: 13.0.5.1+1-Debian-1, Debian, OpenJDK 64-Bit Server VM

I didn't use the deb but downloaded the jar directly. If you suspect it's something related to my system which is potentially fixed by the deb please let me know and I will check that.

[skyper]

Faced this myself.
Is the color setting for the gpx the problem or any setting for the session which is not properly store within the session file?

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2021-06-02 23:14:11 +0200 (Wed, 02 Jun 2021)
Revision:17921
Build-Date:2021-06-03 01:31:07
URL:https://josm.openstreetmap.de/svn/trunk

[xeen]

If this is a question to me, I am not quite sure how to answer. I checked with 17919 using a fresh profile (i.e. global settings = single color GPX points). The tracks are the default pink.

Checking the individual track drawing, it was also set to "single color" and not "use global settings". However, either of them still cause the dialog. I tested a bunch of other combinations, and they also trigger the dialog. So I guess it's not related to any specific setting of that customized drawing dialog? I didn't try to customize the color.

[skyper]

Replying to xeen:

If this is a question to me,

It was a question to everybody including myself, as I noticed the track color warning.

I am not quite sure how to answer. I checked with 17919 using a fresh profile (i.e. global settings = single color GPX points). The tracks are the default pink.

Checking the individual track drawing, it was also set to "single color" and not "use global settings". However, either of them still cause the dialog. I tested a bunch of other combinations, and they also trigger the dialog. So I guess it's not related to any specific setting of that customized drawing dialog? I didn't try to customize the color.

Thanks, I have a general setting for coloring per velocity and I notice that the session uses "black" instead, so something is wrong with the coloring. This could be another unrelated bug, though.

[Bjoeni]

Replying to skyper:

This could be another unrelated bug, though.

Appears to be both related to #16796. I'll have a look.

[Bjoeni]

That was caused by some legacy code in the session handling from before colors and layer preferences could be saved in the file.
Simply removing that should fix both issues mentioned above.

src/org/openstreetmap/josm/io/session/SessionWriter.java

@@ -43 +43 @@
 import org.openstreetmap.josm.gui.layer.geoimage.GeoImageLayer;
 import org.openstreetmap.josm.gui.layer.markerlayer.MarkerLayer;
 import org.openstreetmap.josm.gui.preferences.projection.ProjectionPreference;
-import org.openstreetmap.josm.tools.ColorHelper;
 import org.openstreetmap.josm.tools.JosmRuntimeException;
 import org.openstreetmap.josm.tools.Logging;
 import org.openstreetmap.josm.tools.MultiMap;
@@ -240 +239 @@
             if (!Utils.equalsEpsilon(layer.getOpacity(), 1.0)) {
                 el.setAttribute("opacity", Double.toString(layer.getOpacity()));
             }
-            if (layer.getColor() != null) {
-                el.setAttribute("color", ColorHelper.color2html(layer.getColor()));
-            }
             Set<Layer> deps = dependencies.get(layer);
             final String depends = deps == null ? "" : deps.stream().map(depLayer -> {
                 int depIndex = layers.indexOf(depLayer);

src/org/openstreetmap/josm/io/session/SessionReader.java

@@ -3 +3 @@
 
 import static org.openstreetmap.josm.tools.I18n.tr;
 
-import java.awt.Color;
 import java.awt.GraphicsEnvironment;
 import java.io.BufferedInputStream;
 import java.io.File;
@@ -46 +45 @@
 import org.openstreetmap.josm.io.Compression;
 import org.openstreetmap.josm.io.IllegalDataException;
 import org.openstreetmap.josm.tools.CheckParameterUtil;
-import org.openstreetmap.josm.tools.ColorHelper;
 import org.openstreetmap.josm.tools.JosmRuntimeException;
 import org.openstreetmap.josm.tools.Logging;
 import org.openstreetmap.josm.tools.MultiMap;
@@ -619 +617 @@
                     Logging.warn(ex);
                 }
             }
-            String colorString = el.getAttribute("color");
-            if (colorString != null) {
-                try {
-                    Color color = ColorHelper.html2color(colorString);
-                    layer.setColor(color);
-                } catch (RuntimeException ex) {
-                    Logging.warn("Cannot parse color " + colorString);
-                }
-            }
             layer.setName(names.get(entry.getKey()));
             layers.add(layer);
         }

[Bjoeni]

Actually, I was mistaken here. That's not legacy code but was introduced in #20233.

That however completely reverts the intention of #16796 as track colors (including marker layer colors!) should be saved in the GPX file now.

@simon04 and @skyper, am I missing the point here or what was the intention of that ticket? I think the answer to that ticket should have been "save the GPX file before closing". The only thing that should probably be changed is that the GPX layer asks to be saved before removal when the layer preferences were modified.

[skyper]

Replying to Bjoeni:

@simon04 and @skyper, am I missing the point here or what was the intention of that ticket? I think the answer to that ticket should have been "save the GPX file before closing". The only thing that should probably be changed is that the GPX layer asks to be saved before removal when the layer preferences were modified.

There should not be a question about saving as, at least in my case, I did not change anything regarding the individual GPX file.
Additionally, the general setting (per velocity) should be used loading a session file but in my case there is an individual color setting (black) for the track used which I have never set within JOSM. (Might be the color in the gpx file as my Garmin device sets one).

[Adrian]

Related report #20712

[Bjoeni]

Replying to skyper:

There should not be a question about saving as, at least in my case, I did not change anything regarding the individual GPX file.
Additionally, the general setting (per velocity) should be used loading a session file but in my case there is an individual color setting (black) for the track used which I have never set within JOSM. (Might be the color in the gpx file as my Garmin device sets one).

That's exactly what I meant. I'll try to explain it differently:

• This ticket is completely valid and mentions two bugs: The save state and not saving color settings in sessions. Both are regressions, caused by r17659
• Ticket #20233 / r17659 on the other hand was invalid as it attempted to implement color handling for sessions, which at that point already existed for files. That's why that conflicts, and that's what caused this whole mess.

So my last paragraph was referring to the other ticket. Of course, when you don't change anything, you shouldn't be asked about it.

So OP from ticket #20233 (ra@...) should have been told that colors of GPS markers can already be saved, you just have to save the corresponding GPX file. They probably just didn't know that, and that's why I stated that in their scenario (after changing the colors of markers) THEY should be asked to save the changes to the file (only problem is that that doesn't work if you remove the corresponding track before the markers. But that's a separate issue).

That's why I think that r17659 needs to be reverted and everything should work (which is basically the patch from comment:7 that I posted before I was aware of the other ticket).

Sorry for the confusion, let me know if it's still unclear.

[Bjoeni]

I added a new WIP patch that hopefully resolves the issues regarding colors, sessions and marker layers (#20233, #20712, #20913 and #20962).

• revert r17659 (except for unit tests)
  • don't save GPX track and marker colors in session file anymore, but in the GPX files as extensions (like before)
  • don't ask to save unmodified GPX file
  • don't override global color settings when individual track doesn't have a color
• ask user to save changes to GPX file when any drawing settings or marker colors have changed (currently only happens for track colors)
• save marker color values to session even when corresponding GPX layer has already been deleted
• save alpha values for GPX marker colors
• added explanation to the "overwrite GPX file" dialog
• inform user if not all files referenced by the session file are saved yet
• allow user to save all files that are not included in the *.jos/*.joz but are only referenced in the session file

The last points hopefully avoid similar tickets à la "I already saved the session file, now the GPX layer asks to be saved too".
Also I think it's usually quite convenient to save all local files when saving a session, so I enabled that checkbox by default. But that can obviously be changed if you don't agree.

Marker colors will be saved in the GPX file like this:

    <extensions>
      <josm:layerPreferences>
        <josm:entry key="markers.color" value="#34567812"/>
      </josm:layerPreferences>
    </extensions>

(that already exists since #16796, but only happened when manually saved by the user).

As always, feedback is very welcome. I haven't really tested it much, that's why it's WIP.

@skyper, could you please test if the issue you mentioned in comment:5 is resolved with this patch? I think I know what went wrong there and it should be fixed, but I'm not 100% sure.

[Bjoeni]

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

[skyper]

Replying to Bjoeni:

Also I think it's usually quite convenient to save all local files when saving a session, so I enabled that checkbox by default. But that can obviously be changed if you don't agree.

I am fine with the default setting but how about an option in preferences to enable/disable this behavior?

@skyper, could you please test if the issue you mentioned in comment:5 is resolved with this patch? I think I know what went wrong there and it should be fixed, but I'm not 100% sure.

Sorry, I don't have a compile system set up, atm.

But I wonder if we forgot to look at the two different options: internally within the session file vs. own external file
My example is with the gpx file saved externally where no settings need to be saved within the session file but saving the files within the session needs to save the coloring settings within the session file, too, and leave the original file untouched. This might get tricky as I did not check if the "unsaved changes" dialog, when exiting JOSM or deleting the layer, catches the differences.

[Bjoeni]

Replying to skyper:

I am fine with the default setting but how about an option in preferences to enable/disable this behavior?

It's a checkbox in the session exporter dialog, that keeps the value that was last set. So it's only about the first time you encounter it, if you disable it it'll stay disabled.

saving the files within the session needs to save the coloring settings within the session file, too, and leave the original file untouched

That's not really something I was planning on implementing. That's what we tried to get rid of in the first place, since it's not feasible to keep all that information in JOSM (or in this case in the session file).
Opacity is handled differently because it affects the whole layer (and any kind of layer), but individual tracks may have different colors, and e.g. naming patterns for waypoints are saved as well.

Also, since GPX files can handle even unknown extensions (as long as they follow the GPX standards), I don't really see a reason not to modify the original file. You're changing the color, and that can be reflected in the GPX file imo (when saving it in a session you even have three options: Overwrite existing file, save file under new name or include it in the session. 2/3 leave your original file untouched).

I guess it's a bit like when you ZIP a number of word documents and images: You're going to save the text color in each word document, and not in the ZIP file.

Sorry, I don't have a compile system set up, atm.

I attached a compiled jar.

[Bjoeni]

TODO for this patch

• fix bug that causes non-modifiable layers (e.g. imagery layers) to be saved
• check plugin compatibility for savable layers
• do not ask if GPX files generated by JOSM should be overwritten
• display * next to GPX layers that need to be saved (move isDirty() logic from OsmDataLayer to AbstractModifiableLayer)
• fix bug displaying markers in white instead of neutral color in some cases
• add tests
• pmd/checkstyle

[skyper]

Replying to Bjoeni:

Replying to skyper:

I am fine with the default setting but how about an option in preferences to enable/disable this behavior?

It's a checkbox in the session exporter dialog, that keeps the value that was last set. So it's only about the first time you encounter it, if you disable it it'll stay disabled.

Nice, that is even better. session.savelocal is the advanced preference setting, am I right.

saving the files within the session needs to save the coloring settings within the session file, too, and leave the original file untouched

That's not really something I was planning on implementing. That's what we tried to get rid of in the first place, since it's not feasible to keep all that information in JOSM (or in this case in the session file).
Opacity is handled differently because it affects the whole layer (and any kind of layer), but individual tracks may have different colors, and e.g. naming patterns for waypoints are saved as well.

Also, since GPX files can handle even unknown extensions (as long as they follow the GPX standards), I don't really see a reason not to modify the original file. You're changing the color, and that can be reflected in the GPX file imo (when saving it in a session you even have three options: Overwrite existing file, save file under new name or include it in the session. 2/3 leave your original file untouched).

I guess it's a bit like when you ZIP a number of word documents and images: You're going to save the text color in each word document, and not in the ZIP file.

Ok, I was on the wrong train. You are right, that in both situations the information can be stored within the gpx file, either as local individual file or within the gpx file included in the .joz file.

Sorry, I don't have a compile system set up, atm.

I attached a compiled jar.

Thanks. In a short test it works nicely and both issues seem to be fixed. Good job.

[Bjoeni]

Replying to skyper:

Replying to Bjoeni:

Replying to skyper:

I am fine with the default setting but how about an option in preferences to enable/disable this behavior?

It's a checkbox in the session exporter dialog, that keeps the value that was last set. So it's only about the first time you encounter it, if you disable it it'll stay disabled.

Nice, that is even better. session.savelocal is the advanced preference setting, am I right.

Yes, exactly.

Sorry, I don't have a compile system set up, atm.

I attached a compiled jar.

Thanks. In a short test it works nicely and both issues seem to be fixed. Good job.

Thanks!

[Bjoeni]

changed since comment:12

• do not ask if GPX files generated by JOSM should be overwritten
• display * next to GPX layers that need to be saved (move isDirty() logic from OsmDataLayer to AbstractModifiableLayer)
• added SessionWriter tests

[Don-vip]

@Bjoeni I have trouble to apply this patch, gives an error with both Eclipse and Tortoise. Could you please update it?

[Bjoeni]

Sorry I won't be able to get to this / won't be home till at least mid October

(that milestone doesn't exist yet)

[Don-vip]

Milestone renamed

[Bjoeni]

gpx_markers.joz (binary) must be put in test/data/sessions

[Bjoeni]

(fix checkstyle issue that slipped through)

[Don-vip]

@Bjoeni still the same problem, I can't apply your patch, both Eclipse and Tortoise say it's not valid.

[GerdP]

My editor says the patch is in Utf-16 little endian. Quite unusual. When I convert it to UTf-8 no Bom svn applies it. Should I upload my version?

[GerdP]

converted patch

[Don-vip]

Ah, thank you, I didn't think to check encoding as I didn't have a useful error message.

[Don-vip]

In 18287/josm:

fix #20913 - fix handling of GPX files in sessions (patch by Bjoeni)

• revert r17659 (except for unit tests) - see #20233
  • don't save GPX track and marker colors in session file anymore, but in the GPX files as extensions (like before)
  • don't ask to save unmodified GPX file
  • don't override global color settings when individual track doesn't have a color
• ask user to save changes to GPX file when any drawing settings or marker colors have changed (currently only happens for track colors)
• save marker color values to session even when corresponding GPX layer has already been deleted
• save alpha values for GPX marker colors
• added explanation to the "overwrite GPX file" dialog
• inform user if not all files referenced by the session file are saved yet
• allow user to save all files that are not included in the *.jos/*.joz but are only referenced in the session file
• display * next to GPX layers that need to be saved (move isDirty() logic from OsmDataLayer to AbstractModifiableLayer)

[Don-vip]

Thanks a lot!