Modify

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#20913 closed defect (fixed)

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

Reported by: xeen Owned by: team
Priority: normal Milestone: 21.10
Component: Core Version: tested
Keywords: session gpx regression Cc: simon04

Description (last modified by skyper)

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]

Attachments (7)

20913-v2.1.patch (30.3 KB ) - added by Bjoeni 3 years ago.
josm-t20913-v2.2.jar (15.7 MB ) - added by Bjoeni 3 years ago.
20913-v2.6.patch (109.7 KB ) - added by Bjoeni 3 years ago.
20913-v3.patch (104.2 KB ) - added by Bjoeni 2 years ago.
gpx_markers.joz (1.6 KB ) - added by Bjoeni 2 years ago.
20913-v3.1.patch (103.9 KB ) - added by Bjoeni 2 years ago.
(fix checkstyle issue that slipped through)
20913-v3.1.2.patch (51.9 KB ) - added by GerdP 2 years ago.
converted patch

Change History (40)

comment:1 by skyper, 3 years ago

Description: modified (diff)
Keywords: session gpx added
Priority: trivialnormal

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.

comment:2 by xeen, 3 years ago

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.

comment:3 by skyper, 3 years ago

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

comment:4 by xeen, 3 years ago

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.

in reply to:  4 ; comment:5 by skyper, 3 years ago

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.

in reply to:  5 comment:6 by Bjoeni, 3 years ago

Keywords: regression added
Owner: changed from team to Bjoeni
Status: newassigned

Replying to skyper:

This could be another unrelated bug, though.

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

comment:7 by Bjoeni, 3 years ago

Milestone: 21.06
Owner: changed from Bjoeni to team
Status: assignednew
Summary: JOSM asks to save unmodified GPX files when opening them through a saved session[Patch] JOSM asks to save unmodified GPX files when opening them through a saved session
Version: tested

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

     
    4343import org.openstreetmap.josm.gui.layer.geoimage.GeoImageLayer;
    4444import org.openstreetmap.josm.gui.layer.markerlayer.MarkerLayer;
    4545import org.openstreetmap.josm.gui.preferences.projection.ProjectionPreference;
    46 import org.openstreetmap.josm.tools.ColorHelper;
    4746import org.openstreetmap.josm.tools.JosmRuntimeException;
    4847import org.openstreetmap.josm.tools.Logging;
    4948import org.openstreetmap.josm.tools.MultiMap;
     
    240239            if (!Utils.equalsEpsilon(layer.getOpacity(), 1.0)) {
    241240                el.setAttribute("opacity", Double.toString(layer.getOpacity()));
    242241            }
    243             if (layer.getColor() != null) {
    244                 el.setAttribute("color", ColorHelper.color2html(layer.getColor()));
    245             }
    246242            Set<Layer> deps = dependencies.get(layer);
    247243            final String depends = deps == null ? "" : deps.stream().map(depLayer -> {
    248244                int depIndex = layers.indexOf(depLayer);
  • src/org/openstreetmap/josm/io/session/SessionReader.java

     
    33
    44import static org.openstreetmap.josm.tools.I18n.tr;
    55
    6 import java.awt.Color;
    76import java.awt.GraphicsEnvironment;
    87import java.io.BufferedInputStream;
    98import java.io.File;
     
    4645import org.openstreetmap.josm.io.Compression;
    4746import org.openstreetmap.josm.io.IllegalDataException;
    4847import org.openstreetmap.josm.tools.CheckParameterUtil;
    49 import org.openstreetmap.josm.tools.ColorHelper;
    5048import org.openstreetmap.josm.tools.JosmRuntimeException;
    5149import org.openstreetmap.josm.tools.Logging;
    5250import org.openstreetmap.josm.tools.MultiMap;
     
    619617                    Logging.warn(ex);
    620618                }
    621619            }
    622             String colorString = el.getAttribute("color");
    623             if (colorString != null) {
    624                 try {
    625                     Color color = ColorHelper.html2color(colorString);
    626                     layer.setColor(color);
    627                 } catch (RuntimeException ex) {
    628                     Logging.warn("Cannot parse color " + colorString);
    629                 }
    630             }
    631620            layer.setName(names.get(entry.getKey()));
    632621            layers.add(layer);
    633622        }

comment:8 by Bjoeni, 3 years ago

Cc: simon04 added

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.

in reply to:  8 ; comment:9 by skyper, 3 years ago

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).

Last edited 3 years ago by skyper (previous) (diff)

comment:10 by Adrian, 3 years ago

Related report #20712

in reply to:  9 comment:11 by Bjoeni, 3 years ago

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.

comment:12 by Bjoeni, 3 years ago

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

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.

by Bjoeni, 3 years ago

Attachment: 20913-v2.1.patch added

comment:13 by Bjoeni, 3 years ago

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

in reply to:  12 ; comment:14 by skyper, 3 years ago

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.

by Bjoeni, 3 years ago

Attachment: josm-t20913-v2.2.jar added

in reply to:  14 ; comment:15 by Bjoeni, 3 years ago

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.

comment:16 by Bjoeni, 3 years ago

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
Last edited 3 years ago by Bjoeni (previous) (diff)

in reply to:  15 ; comment:17 by skyper, 3 years ago

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.

in reply to:  17 comment:18 by Bjoeni, 3 years ago

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!

comment:19 by Bjoeni, 3 years ago

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

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

by Bjoeni, 3 years ago

Attachment: 20913-v2.6.patch added

comment:20 by Don-vip, 3 years ago

Milestone: 21.0621.07

comment:21 by Don-vip, 3 years ago

Owner: changed from team to Bjoeni

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

comment:22 by Bjoeni, 3 years ago

Milestone: 21.0721.08

comment:23 by Don-vip, 3 years ago

Milestone: 21.0821.09

comment:24 by Bjoeni, 3 years ago

Milestone: 21.09

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)

comment:25 by Don-vip, 2 years ago

Milestone: 21.10

comment:26 by Don-vip, 2 years ago

Milestone: 21.1021.11

Milestone renamed

by Bjoeni, 2 years ago

Attachment: 20913-v3.patch added

by Bjoeni, 2 years ago

Attachment: gpx_markers.joz added

comment:27 by Bjoeni, 2 years ago

Owner: changed from Bjoeni to team

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

by Bjoeni, 2 years ago

Attachment: 20913-v3.1.patch added

(fix checkstyle issue that slipped through)

comment:28 by Don-vip, 2 years ago

Milestone: 21.1121.10

comment:29 by Don-vip, 2 years ago

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

comment:30 by GerdP, 2 years ago

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?

by GerdP, 2 years ago

Attachment: 20913-v3.1.2.patch added

converted patch

comment:31 by Don-vip, 2 years ago

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

comment:32 by Don-vip, 2 years ago

Resolution: fixed
Status: newclosed

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)

comment:33 by Don-vip, 2 years ago

Thanks a lot!

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.