[PATCH] GPX track colors / layer preferences

[Bjoeni]

Hi,

just some thoughts on colors in GPX files / tracks...

Currently they can only be defined for the whole layer and are not saved, but the filename / name of the layer is put in the preferences and associated with a color... which doesn't really seem like a good idea to me. I have like tons of dead GPX file settings in my preferences (all of those clr.layer.* settings), and files with similar filenames can lead to problems. They should actually be saved in the file or that should imo at least be an option when saving.

The ​default GPX schema doesn't allow that, but it is extensible, e.g. the ​Garmin GPX extensions supports (very few) DisplayColors:

<xsd:element name="DisplayColor" type="DisplayColor_t" minOccurs="0"/>

  <xsd:simpleType name="DisplayColor_t">
    <xsd:restriction base="xsd:token">
      <xsd:enumeration value="Black"/>
      <xsd:enumeration value="DarkRed"/>
      <xsd:enumeration value="DarkGreen"/>
      <xsd:enumeration value="DarkYellow"/>
      <xsd:enumeration value="DarkBlue"/>
      <xsd:enumeration value="DarkMagenta"/>
      <xsd:enumeration value="DarkCyan"/>
      <xsd:enumeration value="LightGray"/>
      <xsd:enumeration value="DarkGray"/>
      <xsd:enumeration value="Red"/>
      <xsd:enumeration value="Green"/>
      <xsd:enumeration value="Yellow"/>
      <xsd:enumeration value="Blue"/>
      <xsd:enumeration value="Magenta"/>
      <xsd:enumeration value="Cyan"/>
      <xsd:enumeration value="White"/>
      <xsd:enumeration value="Transparent"/>
    </xsd:restriction>
  </xsd:simpleType>

Which will look like this:

    <extensions>
      <gpxx:TrackExtension>
        <gpxx:DisplayColor>Magenta</gpxx:DisplayColor>
      </gpxx:TrackExtension>
    </extensions>

OsmAnd just uses a ​non-standard extension like

<extensions>
  <color>#aafaf700</color>
</extensions>

That however doesn't seem to be documented anywhere else and won't pass validation as ​pointed out on GitHub by baerrus.

How do you see that?

• Where do colors and other settings for specific GPX layers belong? Preferences or layer, i.e. GPX file.
• Should colors only be defined for segments/tracks or for entire files/layers? This makes a difference e.g. when merging.
• In case we agree that they mainly belong in the file or that it should at least be possible to save them, how could they be saved? Possibly creating an own open GPX extension, maybe together with the guys from OsmAnd in order to produce valid GPX files? (not just limited to the color, but also (drawing) features other programs like OsmAnd could use or that could be implemented in the future [width, opacity, dotted lines, ...])

I haven't really thought all of that through, those are just ideas to be discussed. In my opinion the current method of saving layer specific settings in the preferences related to the name just doesn't seem right (settings/colors are even lost when the layer is renamed within JOSM).

[Don-vip]

Creating our own GPX extension sounds like the more elegant solution, and can be reused by OsmAnd. I would welcome a patch for this :)

[Bjoeni]

I'll do that once I'm back in Europe, so probably around February / March.

Plan would be to

• Implement a 'drawing' extension (gpxd:*) / a schema for that. Possible values could be color, opacity (or just color with alpha value?), width, dotting. Not all of that has to be used/implemented by JOSM.
• Rewrite handling of extensions to be more flexible with multiple extensions as required, not just support josm:*
• Correct the way schema locations are written in the file (technically, they are not at all specified at the moment - just the name of the extension happens to be a URL pointing to the XSD schema)
• Allow GPX segments to have colors, not the layers. Needs some adoptions in rendering.
• And obviously read and write those colors to/from the file.
• Add compatibility to the Garmin extension (gpxx:). When saving this means a loss of colors, see supported values above.
• Maybe extend the current 'choose track visibility' dialog to allow manual choosing of colors for tracks / segments.

Any objections?

---

One more thing:
Currently the abstract layer class supports colors (public AbstractProperty<Color> getColorProperty()), it is however from what I can tell just implemented by the MarkerLayer and GpxLayer. And since we want to avoid saving exactly that in preferences, but rather in the file - do we need this property at all? I think it doesn't make any sense anymore, I just don't know if any plugins rely on that (but even if some do - IMO it is by design broken to save any settings regarding a specific layer in the preferences - that should / must necessarily be saved with the contents of a layer). So would it be ok to remove that?

[Bjoeni]

One issue I came across is that when the colors are saved in the tracks (which are ImmutableGpxTracks) the according attributes should be changed when the user changes the color - so not-so-immutable anymore.
I don't think there would be any issues with changing the attributes but it just wouldn't really be an immutable implementation anymore. Is there a deeper meaning to that / why you chose to make it immutable or can I just change the attr field to a modifiable HashList?

And again regarding the ColorProperties (see above), any objections to get rid of them?

[Bjoeni]

I'm not done yet but would like to get some feedback on the patch before I continue - those are quite a few changes after all.

What I did so far:

• Rewrote handling of extensions (and importer/exporter) to be more flexible with multiple extensions as required, not just support josm:*
• Implemented a 'drawing' extension (gpxd:*) / a schema for that (this should be made available at https://josm.openstreetmap.de/gpx-drawing-extensions-1.0.xsd). Possible values are color, opacity, width and dash patterns (dotted lines). So far only colors are written and read by JOSM.
• Corrected the way schema locations are written in the file (technically, they weren't specified at all - just the name of the extension happened to be a URL pointing to the XSD schema)
• Allow GPX tracks to have colors, not the layers. It is still possible to use the "Customize Color" action for multiple layers together (color is then applied to all tracks in these layers) or alternatively set a color for individual tracks in the ChooseTrackVisibilityDialog
• Track name and description can be edited directly in said dialog (in the table)
• Accordingly the colors are not anymore saved in the preferences, but in the files. The ColorProperty is now only used for the default/neutral color (magenta), which should drastically reduce the amount of properties. This means the layer specific getColorProperty() stuff etc. is not supported anymore, the layers now just have the methods hasColor(), getColor() and setColor(), which can, but do not have to be overridden.
• Added events handling the now possible change of GpxTracks during runtime
• Added compatibility to the Garmin extension (gpxx:). When saving this means a loss of colors/information. Users are warned
• Added a couple of tests

What needs to be done (at least) before applying this patch:

• Gpx exporter doesn't work when asking if changes should be saved (upon closing/removal of a layer) due to threading problems
• Marker layers don't support colors anymore/yet
• Add more tests for importer/exporter
• Check Plugin compatibility (Should probably be checked again before committing)
• No lines are drawn for unnamed new gpx tracks converted from data layers for some reason. This doesn't seem to be an issue with my patch though, same goes for the latest stable. Did someone change something there within the last few months? (haven't had a look at that yet, just noticed when creating the screenshots)

Notes:

• The determination of the needed namespaces for exporting and conversion between GPXX and GPXD isn't exactly beautiful.
• the opacity from the layer should also be saved in the files
• I'd suggest linking MarkerLayer and GpxLayer upon creation in order to allow the whole layer to be saved together again. Otherwise this might lead to a loss of markers if someone overrides the old file.
• Not sure about the (visual) design in the ChooseTrackVisibilityDialog...
• In the long term I'd like to get rid of all layer specific settings in the preferences. Imo this is just poor design. The clr.layer.* ones are mostly removed, but the gps.raw.* ones still remain. Suggestion: Either save most of them as further JOSM extensions or just don't save these layer specific settings at all. They are saved for Gpx layers now, should probably be implemented for other layers as well
• The ImmutableGpxTrack isn't immutable anymore. I'd rename it and handle it like the MarkerLayer: The Interface GpxTrack.java becomes IGpxTrack.java and the Class ImmutableGpxTrack.java becomes GpxTrack.java (described as default implementation). This will inevitably lead to some issues since it's referenced by five Plugins, but I think that's worth it.
• Thank you for taking the time to read all of this.
• The MarkerLayer is (unlike all the other layers) in layer/markerlayer/MarkerLayer.java and two MarkerLayerTests existed in layer/ and layer/markerlayer. I merged them to layer/markerlayer to match the source file structure but would actually put them both in layer/.

[Bjoeni]

Has anyone had a look at the patch above?
So far I implemented that mainly for myself anyways (to allow different colors in one file), but I'm not gonna continue with the points mentioned above if you aren't interested in this patch (which is fine, I just need to know).
(oh and yes the way the table is colored on the screenshots is horrible, just didn't really have a better idea so far).

[Bjoeni]

Replacing screenshots above

[Bjoeni]

Replying to Bjoeni:

Currently the abstract layer class supports colors (public AbstractProperty<Color> getColorProperty()), it is however from what I can tell just implemented by the MarkerLayer and GpxLayer. And since we want to avoid saving exactly that in preferences, but rather in the file - do we need this property at all? I think it doesn't make any sense anymore, I just don't know if any plugins rely on that (but even if some do - IMO it is by design broken to save any settings regarding a specific layer in the preferences - that should / must necessarily be saved with the contents of a layer). So would it be ok to remove that?

Following ticket:2760#comment:18, I checked again if any of the public signatures I changed in this patch (that I didn't provide overloaded methods for to restore the old behavior) affected any plugins - they didn't. Especially the whole ColorProperty-stuff isn't used by a single layer (plugin and core) apart from MarkerLayer and GpxLayer.

A list of the public methods/fields that are changed:

• data.gpx.Extensions
  • constructor - used by extensions handling in core
• data.gpx.GpxConstants
  • META_EXTENSIONS, JOSM_EXTENSIONS_NAMESPACE_URI - used by extensions handling in core
• data.gpx.IWithAttributes
  • addExtension() - used by Marker and AudioMarker in MarkerLayer
• data.gpx.Line
  • constructors - only used within GpxData
• gui.layer.Layer
  • getColorProperty(), getBaseColorProperty(), addColorPropertyListener(), removeColorPropertyListener() - used by GpxLayer and MarkerLayer. Replaced by hasColor(), getColor(), setColor()
• gui.layer.gpx.GpxDrawHelper
  • getColor(), getGenericColor() - public methods only used internally
  • DEFAULT_COLOR - used by GpxDrawHelper, GpxLayer, GPXSettingsPanel. That change wasn't absolutely necessary but is less confusing now. MarkerLayer had Color DEFAULT_COLOR and NamedColorProperty COLOR_PROPERTY, while GpxDrawHelper had NamedColorProperty DEFAULT_COLOR. Now both have NamedColorProperty DEFAULT_COLOR_PROPERTY.
• gui.layer.marker.Markerlayer
  • getGenericColor() - only used by gui.preferences.display.ColorPreference

[Don-vip]

Sorry I missed this patch. I'll take a look soon.

[Don-vip]

Milestone renamed

[Bjoeni]

Resolved conflicts, especially with #2760. Of course still WIP.

[Bjoeni]

Update 0.16:

• Gpx layer specific settings are saved in layer (e.g. from the "customize track drawing" dialog)
• marker colors implemented again and saved in layer
• fixed (existing) encoding bugs in GpxWriter

So a Gpx file with layer specific settings, colors set for the tracks and a color set for the markers could look like this now:

<?xml version='1.0' encoding='UTF-8'?>
<gpx version="1.1" creator="JOSM GPX export" xmlns="http://www.topografix.com/GPX/1/1"
    xmlns:josm="http://josm.openstreetmap.de/gpx-extensions-1.1"
    xmlns:gpxd="http://josm.openstreetmap.de/gpx-drawing-extensions-1.0"
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xsi:schemaLocation="http://www.topografix.com/GPX/1/1 http://www.topografix.com/GPX/1/1/gpx.xsd http://josm.openstreetmap.de/gpx-extensions-1.1 http://josm.openstreetmap.de/gpx-extensions-1.1.xsd http://josm.openstreetmap.de/gpx-drawing-extensions-1.0 http://josm.openstreetmap.de/gpx-drawing-extensions-1.0.xsd">
  <metadata>
    <desc>Test</desc>
    <bounds minlat="46.001" minlon="10.001" maxlat="46.004" maxlon="10.004"/>
    <extensions>
      <josm:layerPreferences>
        <josm:entry key="colormode" value="1"/>
        <josm:entry key="lines" value="1"/>
        <josm:entry key="lines.arrows" value="true"/>
        <josm:entry key="lines.max-length.local" value="150"/>
        <josm:entry key="lines.width" value="3"/>
        <josm:entry key="markers.color" value="#00CCCC"/>
        <josm:entry key="markers.show-text" value="false"/>
        <josm:entry key="points.large" value="true"/>
      </josm:layerPreferences>
    </extensions>
  </metadata>
  <wpt lat="46.001" lon="10.001">
    <ele>411.326660</ele>
    <time>2017-04-01T22:21:32Z</time>
    <name>001</name>
    <cmt>01-APR-17 22:21:32</cmt>
    <desc>01-APR-17 22:21:32</desc>
    <sym>Flag, Blue</sym>
  </wpt>
  <trk>
    <name>ACTIVE LOG080603</name>
    <extensions>
      <gpxd:color>#CCCC00</gpxd:color>
    </extensions>
    <trkseg>
      <trkpt lat="46.002" lon="10.002">
        <ele>2307.992</ele>
        <time>2018-08-22T06:06:03Z</time>
      </trkpt>
      <trkpt lat="46.003" lon="10.003">
        <ele>2312.783</ele>
        <time>2018-08-22T06:06:28Z</time>
      </trkpt>
      <trkpt lat="46.004" lon="10.004">
        <ele>2316.533</ele>
        <time>2018-08-22T06:06:30Z</time>
      </trkpt>
    </trkseg>
  </trk>
</gpx>

I updated the todo list above.

[Bjoeni]

@Don-vip, when do you think you'll have a chance to have a look at the patch? Just because it regularly conflicts with updates atm due to its size.

[Don-vip]

Hi Bjoeni,
Sorry for the long delay, it's not a lack of interest but a lack of time due to the size of this patch. It's huge work, thanks a lot!

• Questions:
  • get rid of ColorProperties: no objection
  • make the gpx track no longer immutable: no objection
• Extensions.java:
  • InvalidExtensionException: please add a parameter to the exception constructor (invalid value)
  • Check for uses of single-character strings and replace them by characters when possible, for example here type.substring(type.lastIndexOf(".") + 1) + ":";
• GpxData.java
  • Use diamond operator when possible, for example here public Map<String, String> layerPrefs = new HashMap<String, String>() {};
  • Document all parameters in javadoc, for example here: @param value
• Add @since xxx in the javadoc of new classes/interfaces and new public methods of existing classes
• Make sure that ant pmd checkstyle findbugs do not raise ney issues
• Update Preferences.removeObsolete to remove color preferences entries made obsolete by your patch

[Bjoeni]

Thanks for the feedback!

Replying to Don-vip:

it's not a lack of interest but a lack of time due to the size of this patch

I know, we all do it just for fun :) Was really just about constantly getting conflicts.

• make the gpx track no longer immutable: no objection

Do you want me to rename (so probably duplicate and deprecate) it as suggested above?:

The ImmutableGpxTrack isn't immutable anymore. I'd rename it and handle it like the MarkerLayer: The Interface GpxTrack.java becomes IGpxTrack.java and the Class ImmutableGpxTrack.java becomes GpxTrack.java (described as default implementation).

It is currently referenced by five Plugins, but I think that's worth it (better than having a mutable ImmutableGpxTrack)

[Bjoeni]

And what should I use for a constant map like this:

    /**
     * Map with all supported Garmin colors
     */
    Map<String, Color> GARMIN_COLORS = Collections.unmodifiableMap(
            new TreeMap<String, Color>(String.CASE_INSENSITIVE_ORDER) {{
                put("Black", Color.BLACK);
                put("DarkRed", new Color(139, 0, 0));
                put("DarkGreen", new Color(0, 100, 0));
                put("DarkYellow", new Color(255, 170, 0));
                put("DarkBlue", new Color(0, 0, 139));
                put("DarkMagenta", new Color(139, 0, 139));
                put("DarkCyan", new Color(0, 139, 139));
                put("LightGray", Color.LIGHT_GRAY);
                put("DarkGray", Color.DARK_GRAY);
                put("Red", Color.RED);
                put("Green", Color.GREEN);
                put("Yellow", Color.YELLOW);
                put("Blue", Color.BLUE);
                put("Magenta", Color.MAGENTA);
                put("Cyan", Color.CYAN);
                put("White", Color.WHITE);
                put("Transparent", new Color(0, 0, 0, 255));
            }});

For that I'd get a warning for ​https://errorprone.info/bugpattern/DoubleBraceInitialization (obsly I can't use Map.of() since it's more than 10 items).

[Don-vip]

Yes, it's worth renaming/deprecating the class.
For the static Map initialization we have different ways of doing it:

static block:

    private static final Map<String, Ellipsoid> ellipsoids = new HashMap<>();

    static {
        ellipsoids.put("airy", Ellipsoid.Airy);
        ellipsoids.put("mod_airy", Ellipsoid.AiryMod);
        ellipsoids.put("aust_SA", Ellipsoid.AustSA);
        ellipsoids.put("bessel", Ellipsoid.Bessel1841);
        ellipsoids.put("bess_nam", Ellipsoid.BesselNamibia);
        ellipsoids.put("clrk66", Ellipsoid.Clarke1866);
        ellipsoids.put("clrk80", Ellipsoid.Clarke1880);
        ellipsoids.put("clrk80ign", Ellipsoid.ClarkeIGN);
        ellipsoids.put("evrst30", Ellipsoid.Everest);
        ellipsoids.put("evrst48", Ellipsoid.Everest1948);
        ellipsoids.put("evrst56", Ellipsoid.Everest1956);
        ellipsoids.put("evrst69", Ellipsoid.Everest1969);
        ellipsoids.put("evrstSS", Ellipsoid.EverestSabahSarawak);
        ellipsoids.put("fschr60", Ellipsoid.Fischer);
        ellipsoids.put("fschr60m", Ellipsoid.FischerMod);
        ellipsoids.put("fschr68", Ellipsoid.Fischer1968);
        ellipsoids.put("intl", Ellipsoid.Hayford);
        ellipsoids.put("helmert", Ellipsoid.Helmert);
        ellipsoids.put("hough", Ellipsoid.Hough);
        ellipsoids.put("krass", Ellipsoid.Krassowsky);
        ellipsoids.put("sphere", Ellipsoid.Sphere);
        ellipsoids.put("walbeck", Ellipsoid.Walbeck);
        ellipsoids.put("GRS67", Ellipsoid.GRS67);
        ellipsoids.put("GRS80", Ellipsoid.GRS80);
        ellipsoids.put("WGS66", Ellipsoid.WGS66);
        ellipsoids.put("WGS72", Ellipsoid.WGS72);
        ellipsoids.put("WGS84", Ellipsoid.WGS84);
    }

method:

    private static final Map<String, Double> PRIME_MERIDANS = getPrimeMeridians();

    private static Map<String, Double> getPrimeMeridians() {
        Map<String, Double> ret = new ConcurrentHashMap<>();
        try {
            ret.put("greenwich", 0.0);
            ret.put("lisbon", parseAngle("9d07'54.862\"W", null));
            ret.put("paris", parseAngle("2d20'14.025\"E", null));
            ret.put("bogota", parseAngle("74d04'51.3\"W", null));
            ret.put("madrid", parseAngle("3d41'16.58\"W", null));
            ret.put("rome", parseAngle("12d27'8.4\"E", null));
            ret.put("bern", parseAngle("7d26'22.5\"E", null));
            ret.put("jakarta", parseAngle("106d48'27.79\"E", null));
            ret.put("ferro", parseAngle("17d40'W", null));
            ret.put("brussels", parseAngle("4d22'4.71\"E", null));
            ret.put("stockholm", parseAngle("18d3'29.8\"E", null));
            ret.put("athens", parseAngle("23d42'58.815\"E", null));
            ret.put("oslo", parseAngle("10d43'22.5\"E", null));
        } catch (ProjectionConfigurationException ex) {
            throw new IllegalStateException(ex);
        }
        return ret;
    }

stream:

    public static final Map<String, SystemOfMeasurement> ALL_SYSTEMS = Collections.unmodifiableMap(
            Stream.of(METRIC, CHINESE, IMPERIAL, NAUTICAL_MILE)
            .collect(Collectors.toMap(SystemOfMeasurement::getName, Function.identity())));

Don't forget the code must still compile with Java 8.

[Bjoeni]

Isn't the first one the exact same thing as initializing it with a double brace, except being more complicated? A static anonymous method used to put the values?

[michael2402-]

@Bjoeni

You way creates an anonymous class and fills the hash map in the constructor. This adds one more class to the classpath. And it makes method calls slower (since hotspot does an exact class test when inlining, not a subclass test). But all in all, the overhead is not much. It is more a matter of programming style (use subclasses only if you specialize functionality).

Wrapping it in an unmodifyable map is good to avoid further errors (concurrency, ...)

[Bjoeni]

Thanks for the explanation! But I just tried it, can't use a static block in an interface (GpxConstants). So that means I would have to create a helper class/method inside just for initializing or change the whole thing to a class? Doesn't seem ideal either.

[Bjoeni]

For now I did it like this:

    /**
     * Map with all supported Garmin colors
     */
    Map<String, Color> GARMIN_COLORS = getGarminColors();

    /**
     * Helper method for {@link #GARMIN_COLORS}
     * @return the map with supported Garmin colors
     */
    static Map<String, Color> getGarminColors() {
        TreeMap<String, Color> m = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
        m.put("Black", Color.BLACK);
        m.put("DarkRed", new Color(139, 0, 0));
        m.put("DarkGreen", new Color(0, 100, 0));
        m.put("DarkYellow", new Color(255, 170, 0));
        m.put("DarkBlue", new Color(0, 0, 139));
        m.put("DarkMagenta", new Color(139, 0, 139));
        m.put("DarkCyan", new Color(0, 139, 139));
        m.put("LightGray", Color.LIGHT_GRAY);
        m.put("DarkGray", Color.DARK_GRAY);
        m.put("Red", Color.RED);
        m.put("Green", Color.GREEN);
        m.put("Yellow", Color.YELLOW);
        m.put("Blue", Color.BLUE);
        m.put("Magenta", Color.MAGENTA);
        m.put("Cyan", Color.CYAN);
        m.put("White", Color.WHITE);
        m.put("Transparent", new Color(0, 0, 0, 255));
        return Collections.unmodifiableMap(m);
    }

Something else, regarding the export: Assuming the user modifies the layer specific options or the color/name of a track, he will be asked if he wants to save the file upon deletion of the layer. At least hypothetically there is the option that the file had some extensions not supported by JOSM which would get lost (like for example the non-standard color "extension" of OSMAnd mentioned in the first post). How would you handle that?

[michael2402]

What you can do:

• You can move those constants to an utility class (only static fields, no constructor), then use a private factory method:

  public final class GpxConstants {
    public static final Map<...> GARMIN_COLORS = generateGarminColors();
  
    private static Map<...> generateGarminColors() {
      return ...
    }
  }
  

• You can have a static method in your interface

  public interface GpxConstants {
    static final Map<...> GARMIN_COLORS = generateGarminColors();
  
    static Map<...> generateGarminColors() {
      return ...
    }
  }
  

[Don-vip]

Replying to Bjoeni:

At least hypothetically there is the option that the file had some extensions not supported by JOSM which would get lost (like for example the non-standard color "extension" of OSMAnd mentioned in the first post). How would you handle that?

I would make sure we keep these extensions in memory to not lose them when saving the file back.

[Bjoeni]

Replying to Don-vip:

Replying to Bjoeni:

At least hypothetically there is the option that the file had some extensions not supported by JOSM which would get lost (like for example the non-standard color "extension" of OSMAnd mentioned in the first post). How would you handle that?

I would make sure we keep these extensions in memory to not lose them when saving the file back.

I started implementing that and noticed that it's getting a bit bigger than expected.
Originally I just wanted to write a hack to save the Strings within <extensions> elements, but
a) SAX doesn't return it that way and
b) it's not exactly beautiful
c) it would have to be filtered manually (for example the gpxx:TrackColor element is a child of gpxx:TrackExtensions, so other elements with the same parent would have to be kept etc.)

I ended up re-implementing the extensions handling from scratch, because the old model was only designed for JOSM extensions, and I the one I chose was just the old one modified for my needs, without being flexible at all (e.g. no child extensions supported). So changing that makes sense anyways I guess.
Also the namespace handling in GpxReader and GpxWriter needs to be rewritten then, since it actually needs to deal with all kinds of namespaces and associated schemaLocations (unlike just printing the hard-coded headers).
What I can't guarantee though is that schema locations of unknown extensions will be written correctly after converting between GPX and OSM, because there is no way to save this in OSM. But I'd say you can't really expect any software to keep unsupported elements in a file after converting it back and forth.

[Don-vip]

Replying to Bjoeni:

What I can't guarantee though is that schema locations of unknown extensions will be written correctly after converting between GPX and OSM, because there is no way to save this in OSM. But I'd say you can't really expect any software to keep unsupported elements in a file after converting it back and forth.

I think it's ok :)

[Bjoeni]

I think I'm pretty much done here:

• reimplemented Extension and namespace handling from scratch (allow child extensions)
• write back all extensions to file
• try to convert extensions to OSM layer and back
• delete obsolete / rename updated preferences
• added and fixed a lot of Tests
• renamed ImmutableGpxTrack and ImmutableGpxTrackSegment
• GpxTrackSegment extends WithAttributes now
• save template pattern values in layer specific prefs
• fixed pmd/checkstyle/findbugs issues, including chars/diamond operators
• fixed java warnings, including DoubleBraceIntitializer
• added further javadoc comments
• added @since tags
• added parameter to InvalidExtensionException
• fixed bug that drew first point of a line in neutral color
• increase performance for drawing points
• fixed equals and hashCode of GpxData
• replaced some Java 9 calls I accidentally used
• fixed bug that prevented overriding of non-default global prefs by layer prefs
• don't save unnecessary values when lines value is set to "none"
• moved further values used by GpxDrawHelper to layer specific prefs
• removed COLOR_CATEGORY_LAYER from NamedColorProperty
• probably some other stuff I forgot to write down

This means that now all crl.layer and all layer specific draw.rawgps prefs are obsolete and removed. From what I can tell that means that there are no layer specific preferences left at all (in core).

About the renaming/deprecating of GpxTracks though:
I could only deprecate the class, not the interface, because

• Class ImmutableGpxTrack becomes Class GpxTrack and
• Interface GpxTrack becomes Interface IGpxTrack

that means that GpxTrack.java basically changes from Interface to Class (same goes for GpxTrackSegment.java).
This breaks the following plugins:

• public_transport, LiveGPS, Elevationprofile, JOSM-gpsblam, JOSM-editgpx, JOSM-commandline, JOSM-NanoLog, JOSM-InfoMode, JOSM-Imagery-XML-Bounds

All easy to fix, so is it possible to do drop the Interface without deprecating it and update all affected plugins before the next stable? Because I don't really see a way around it that makes sense.

Also, I'd suggest pushing the milestone since I guess it makes a lot of sense to commit a patch of this size just after releasing a stable.

[Don-vip]

Thanks! Can you please just make sure that new fields are added as private fields with getters/setters instead of public fields? I'll commit it right after we release 19.10.

[Bjoeni]

I'll do that, shouldn't be much work.

About the broken plugins, how are we gonna handle that?

I could only deprecate the class, not the interface, because

• Class ImmutableGpxTrack becomes Class GpxTrack and
• Interface GpxTrack becomes Interface IGpxTrack

that means that GpxTrack.java basically changes from Interface to Class (same goes for GpxTrackSegment.java).
This breaks the following plugins:

• public_transport, LiveGPS, Elevationprofile, JOSM-gpsblam, JOSM-editgpx, JOSM-commandline, JOSM-NanoLog, JOSM-InfoMode, JOSM-Imagery-XML-Bounds

All easy to fix, so is it possible to do drop the Interface without deprecating it and update all affected plugins before the next stable? Because I don't really see a way around it that makes sense.

[Bjoeni]

And should the getters for collections always return unmodifiable collections in JOSM and possibly provide wrappers for all modifying methods of the subclass, or is it fine to only have a getter and expect the caller to modify the Object returned by the getter? I'm not too aware of how these things are usually dealt with in Java.

Because in this case I added a getExtensions() method to WithAttributes which returns a GpxExtensionCollection that has these setters / modifying methods:

add(GpxExtension)
add(String, String)
add(String, String, String)
addAll(Collection<? extends GpxExtension>)
addFlat(String[], String)
addIfNotPresent(String, String)
addOrUpdate(String, String, String)
closeChild(String, String)
openChild(String, String, Attributes)
remove(String, String)
removeAllWithPrefix(String)

(plus all the extended methods)

which I think is much better than blowing up WithAttributes to the point where you don't get what does what anymore.
But a call could look like this: GpxTrack.getExtensions().add("gpxd", "color", "#FF0000"). Is that fine?

[Don-vip]

Replying to Bjoeni:

About the broken plugins, how are we gonna handle that?

I'll update them at the same time, don't worry.

For the getters/setters, keep it simple. The patch is already big enough :) The syntax you propose is fine.

[Bjoeni]

• Use getters and setters instead of public fields
• some small changes / tests added

[Bjoeni]

Fixed a small bug in GpxExtension.remove(), GpxExtension.hide() and GpxTrack.convertColor(ColorFormat) that didn't cause any issue at the moment (with this patch) but could potentially have been a problem if used by a plugin / a future version of JOSM.

[Don-vip]

In 15496/josm:

fix #16796 - Rework of GPX track colors / layer preferences (patch by Bjoeni)

[Don-vip]

@Bjoeni can you please provide a patch for livegps plugin? I don't know how to fix this one.

[Don-vip]

Needs core changes as well, see https://josm.openstreetmap.de/jenkins/job/JOSM/5723/:

	org.openstreetmap.josm.gui.layer.gpx.ChooseTrackVisibilityActionTest.testAction fails
	FindBugs: 6 new warnings
	Checkstyle: 31 warnings
	PMD: 2 warnings

[Don-vip]

Plugins (except livegps) updated in [o35209:35220].

[Don-vip]

In 15497/josm:

see #16796 - fix most of checkstyle/pmd/findbugs violations

[Don-vip]

@Bjoeni I've fixed the obvious violations, please run ant pmd checkstyle to fix the remaining ones.

[Bjoeni]

Weird... pmd checkstyle didn't return anything and the ChooseTrackVisibilityActionTest passed as well (I guess JMockit behaves differently on different systems). I'll have a look at it when I'm home.

[Bjoeni]

Any idea why I'm not getting those warnings / the test works for me?
And yes I did get some warnings, e.g. DoubleBraceIntitializer and chars/diamond operators, but I fixed them before submitting the patch.

niklas@niklas-Akoya-P7818:~/eclipse/JOSM$ svn info
Path: .
Working Copy Root Path: /home/niklas/eclipse/JOSM
URL: https://josm.openstreetmap.de/svn/trunk
Relative URL: ^/trunk
Repository Root: https://josm.openstreetmap.de/svn
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Revision: 15498
Node Kind: directory
Schedule: normal
Last Changed Author: Don-vip
Last Changed Rev: 15498
Last Changed Date: 2019-11-02 16:40:58 +0100 (Sa, 02 Nov 2019)

niklas@niklas-Akoya-P7818:~/eclipse/JOSM$ svn diff
niklas@niklas-Akoya-P7818:~/eclipse/JOSM$ ant clean dist
Buildfile: /home/niklas/eclipse/JOSM/build.xml

init-properties:

clean:
   [delete] Deleting directory /home/niklas/eclipse/JOSM/build2
   [delete] Deleting: /home/niklas/eclipse/JOSM/tools/pmd/cache

init-properties:

init:
    [mkdir] Created dir: /home/niklas/eclipse/JOSM/build
    [mkdir] Created dir: /home/niklas/eclipse/JOSM/dist

javacc:
    [mkdir] Created dir: /home/niklas/eclipse/JOSM/src/org/openstreetmap/josm/gui/mappaint/mapcss/parsergen
     [java] Warning: Line 4, Column 3: Command line setting of "OUTPUT_DIRECTORY" modifies option value in file.
     [java] Java Compiler Compiler Version 7.0.2 (Parser Generator)
     [java] (type "javacc" with no arguments for help)
     [java] Reading from file /home/niklas/eclipse/JOSM/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSParser.jj . . .
     [java] Warning: Line 171, Column 5: Non-ASCII characters used in regular expression.
     [java] Please make sure you use the correct Reader when you create the parser, one that can handle your character set.
     [java] File "TokenMgrError.java" does not exist.  Will create one.
     [java] File "ParseException.java" does not exist.  Will create one.
     [java] File "Token.java" does not exist.  Will create one.
     [java] File "SimpleCharStream.java" does not exist.  Will create one.
     [java] Parser generated with 0 errors and 2 warnings.

compile-cots:
    [javac] Compiling 580 source files to /home/niklas/eclipse/JOSM/build
    [javac] Note: Some input files use or override a deprecated API.
    [javac] Note: Recompile with -Xlint:deprecation for details.
    [javac] Note: Some input files use unchecked or unsafe operations.
    [javac] Note: Recompile with -Xlint:unchecked for details.

compile-jmapviewer:
    [javac] Compiling 50 source files to /home/niklas/eclipse/JOSM/build
    [javac] warning: [options] bootstrap class path not set in conjunction with -source 8
    [javac] 1 warning

compile:
    [javac] Compiling 1701 source files to /home/niklas/eclipse/JOSM/build
    [javac] warning: [options] bootstrap class path not set in conjunction with -source 8
    [javac] 1 warning
     [copy] Copying 1 file to /home/niklas/eclipse/JOSM/build

init-svn-revision-xml:

init-git-revision-xml:

create-revision:
   [delete] Deleting: /home/niklas/eclipse/JOSM/REVISION.XML

check-schemas:

epsg-compile:
    [mkdir] Created dir: /home/niklas/eclipse/JOSM/build2
    [javac] Compiling 1 source file to /home/niklas/eclipse/JOSM/build2
    [javac] warning: [options] bootstrap class path not set in conjunction with -source 8
    [javac] 1 warning

epsg:
    [touch] Creating /home/niklas/eclipse/JOSM/data/projection/custom-epsg

dist:
     [echo] Revision 15498
     [copy] Copying 1 file to /home/niklas/eclipse/JOSM/build
     [copy] Copying 1 file to /home/niklas/eclipse/JOSM/build
     [copy] Copying 1 file to /home/niklas/eclipse/JOSM/build
      [jar] Building jar: /home/niklas/eclipse/JOSM/dist/josm-custom.jar

BUILD SUCCESSFUL
Total time: 1 minute 54 seconds
niklas@niklas-Akoya-P7818:~/eclipse/JOSM$ ant pmd checkstyle
Buildfile: /home/niklas/eclipse/JOSM/build.xml

init-properties:

pmd:
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by net.sourceforge.pmd.ant.Formatter (file:/home/niklas/eclipse/JOSM/tools/pmd/pmd-core-6.10.0.jar) to field java.io.Console.cs
WARNING: Please consider reporting this to the maintainers of net.sourceforge.pmd.ant.Formatter
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
Analysis cache created
Analysis cache updated

init-properties:

checkstyle-compile:
    [javac] Compiling 1 source file to /home/niklas/eclipse/JOSM/build2
    [javac] warning: [options] bootstrap class path not set in conjunction with -source 8
    [javac] 1 warning

checkstyle:
[checkstyle] Running Checkstyle 8.15 on 2290 files

BUILD SUCCESSFUL
Total time: 2 minutes 33 seconds
niklas@niklas-Akoya-P7818:~/eclipse/JOSM$ javac -cp $TESTCP test/unit/org/openstreetmap/josm/gui/layer/gpx/ChooseTrackVisibilityActionTest.java
niklas@niklas-Akoya-P7818:~/eclipse/JOSM$ java -ea -javaagent:test/lib/jmockit.jar -cp $TESTCP org.junit.runner.JUnitCore org.openstreetmap.josm.gui.layer.gpx.ChooseTrackVisibilityActionTest
JUnit version 4.12
.System property josm.test.data is not set, using 'test/data'
2019-11-02 23:11:59.942 INFO: org.openstreetmap.josm.gui.layer.gpx.ChooseTrackVisibilityActionTest$1 answering 2 to ExtendedDialog with content <html>Select all tracks that you want to be displayed. You can drag select a range of tracks or use CTRL+Click to select specific ones. The map is updated live in the background. Open the URLs by double clicking them, edit name and description by double clicking the cell.</html>
2019-11-02 23:12:00.186 INFO: Attempting to close 1 windows left open by tests: [org.openstreetmap.josm.gui.layer.gpx.ChooseTrackVisibilityAction$4[dialog0,302,183,1066x560,invalid,hidden,layout=java.awt.BorderLayout,DOCUMENT_MODAL,title=Set track visibility for Bananas,defaultCloseOperation=DISPOSE_ON_CLOSE,rootPane=javax.swing.JRootPane[,5,25,1056x530,invalid,layout=javax.swing.JRootPane$RootLayout,alignmentX=0.0,alignmentY=0.0,border=,flags=16777673,maximumSize=,minimumSize=,preferredSize=],rootPaneCheckingEnabled=true]]

Time: 2.255

OK (1 test)

[Don-vip]

Checkstyle issue is strange, this is my output:

Buildfile: C:\SVN\josm\core\build.xml
init-properties:
checkstyle-compile:
    [javac] Compiling 1 source file to C:\SVN\josm\core\build2
checkstyle:
[checkstyle] Running Checkstyle 8.15 on 2291 files
[checkstyle] [WARN] C:\SVN\josm\core\src\org\openstreetmap\josm\gui\layer\geoimage\CorrelateGpxWithImages.java:819: La ligne excède 145 caractères (trouvé 164). [LineLength]
[checkstyle] [WARN] C:\SVN\josm\core\src\org\openstreetmap\josm\gui\layer\gpx\ChooseTrackVisibilityAction.java:173:26: La classe interne anonyme contient 71 lignes alors que le maximum autorisé est de 50. [AnonInnerLength]
[checkstyle] [WARN] C:\SVN\josm\core\test\unit\org\openstreetmap\josm\io\GpxWriterTest.java:100: La balise Javadoc doit avoir une description non-vide. [NonEmptyAtclauseDescription]
BUILD SUCCESSFUL
Total time: 27 seconds

Can you please refactor ChooseTrackVisibilityAction inner annonymous class?

For the unit test I guess it fails only when run along all the other ones. @ris: any idea?

[ris]

I think I'd need to see the test/report/ xml file of the failing test to say any more...

They way I used to used to pin down tests dependent on test order was to replace the <fileset ... inside the call-junit of build.xml with a statically generated <FileList ... of the files in question (probably hacked together with some find ... | sed ... concoction) which would allow me to move the problematic test up & down the order of execution to dial in on which is exactly the first other test to cause problems.

[Bjoeni]

Replying to Don-vip:

@Bjoeni can you please provide a patch for livegps plugin? I don't know how to fix this one.

It looks like something went wrong with the automatic refactoring in Eclipse, so GpxData still uses/returns GpxTrack instead of IGpxTrack, which is not a problem unless a plugin uses an own implementation - like liveGPS.

I attached a patch for both core and the plugins that should be modified, some of which you already updated. Sorry about that.
Note that plugin.main.version still has to be set.

Replying to Don-vip:

Can you please refactor ChooseTrackVisibilityAction inner annonymous class?

Done.

[Bjoeni]

@ris / Don-vip: Maybe we should try the ChooseTrackVisibilityActionTest with this.getContent(instance) instead of instance.getContentPane().getComponent(0) again?

See [15496#file72] and https://josm.openstreetmap.de/jenkins/job/JOSM/5723/jdk=JDK8/testReport/junit/org.openstreetmap.josm.gui.layer.gpx/ChooseTrackVisibilityActionTest/testAction/

I had to modify it to get it running for me (which worked) and assumed it was because I changed the dialog, but that might not have been the case after all..?

[ris]

There's a possibility the mocker isn't catching the dialog you think it is. Sometimes you have to be a bit conservative in things like the getString method and have fallbacks to rescue some sort of string if the dialog passed to you doesn't have the structure you expect it to. At this point I'd probably revert to printLn debugging to investigate what exactly what instance that is being passed to you.

[Don-vip]

In 15502/josm:

see #16796 - use IGpxTrack in GpxData (patch by Bjoeni)

[Don-vip]

Plugins patch applied in [o35221:35222].

[Don-vip]

In 15503/josm:

see #16796 - fix 3 forgotten Checkstyle violations

[Don-vip]

In 15504/josm:

see #16796 - fix 1 forgotten FindBugs violation

[Bjoeni]

Thank you!

Just saw the checkstyle warnings in Jenkins, I really have to get my pmd/checkstyle working somehow...

[Don-vip]

In 15516/josm:

see #16796 - ignore test until a fix is provided

[ris]

Sigh.

[Bjoeni]

Sigh too. I think the test can just be removed, it doesn't test any of the critical functionality of the dialog anyways.

And at least I'm not gonna invest any (more) time into trying to fix this small test. Just not worth it. It appears to get the right instance, but crippled without content, no idea why. It has nothing to do with previously executed tests, e.g. if I set <property name="default-junit-includes" value="**/gui/layer/gpx/**/*Test.class"/> so that it is the first one to be executed by ant, it still fails. Compiling it separately works just fine though, see comment:46.

I hate mocking.

[ris]

If you set it to run after all the other tests, does it pass? It's equally likely that a previous test sets up a resource that the test in question needs but doesn't declare.

[Bjoeni]

Replying to ris:

It's equally likely that a previous test sets up a resource that the test in question needs but doesn't declare.

When compiled separately it works just fine

niklas@niklas-Akoya-P7818:~/eclipse/JOSM$ javac -cp $TESTCP test/unit/org/openstreetmap/josm/gui/layer/gpx/ChooseTrackVisibilityActionTest.java
niklas@niklas-Akoya-P7818:~/eclipse/JOSM$ java -ea -javaagent:test/lib/jmockit.jar -cp $TESTCP org.junit.runner.JUnitCore org.openstreetmap.josm.gui.layer.gpx.ChooseTrackVisibilityActionTest
JUnit version 4.12
.System property josm.test.data is not set, using 'test/data'
2019-11-02 23:11:59.942 INFO: org.openstreetmap.josm.gui.layer.gpx.ChooseTrackVisibilityActionTest$1 answering 2 to ExtendedDialog with content <html>Select all tracks that you want to be displayed. You can drag select a range of tracks or use CTRL+Click to select specific ones. The map is updated live in the background. Open the URLs by double clicking them, edit name and description by double clicking the cell.</html>
2019-11-02 23:12:00.186 INFO: Attempting to close 1 windows left open by tests: [org.openstreetmap.josm.gui.layer.gpx.ChooseTrackVisibilityAction$4[dialog0,302,183,1066x560,invalid,hidden,layout=java.awt.BorderLayout,DOCUMENT_MODAL,title=Set track visibility for Bananas,defaultCloseOperation=DISPOSE_ON_CLOSE,rootPane=javax.swing.JRootPane[,5,25,1056x530,invalid,layout=javax.swing.JRootPane$RootLayout,alignmentX=0.0,alignmentY=0.0,border=,flags=16777673,maximumSize=,minimumSize=,preferredSize=],rootPaneCheckingEnabled=true]]

Time: 2.255

OK (1 test)

[Bjoeni]

@Don-vip, the XSDs should be made available on the server (like currently https://josm.openstreetmap.de/gpx-extensions-1.0.xsd)

I'd suggest the following redirects (namespace URI without extension = nice preview):
https://josm.openstreetmap.de/gpx-extensions-1.1 -> https://josm.openstreetmap.de/browser/josm/trunk/data/gpx-extensions-1.1.xsd
https://josm.openstreetmap.de/gpx-extensions-1.1.xsd -> https://josm.openstreetmap.de/export/latest/josm/trunk/data/gpx-extensions-1.1.xsd

https://josm.openstreetmap.de/gpx-drawing-extensions-1.0 -> https://josm.openstreetmap.de/browser/josm/trunk/data/gpx-drawing-extensions-1.0.xsd
https://josm.openstreetmap.de/gpx-drawing-extensions-1.0.xsd -> https://josm.openstreetmap.de/export/latest/josm/trunk/data/gpx-drawing-extensions-1.0.xsd

Other than that I guess this ticket can be closed?

[stoecker]

I added the redirects for these two. That "*.xsd" is forwarded is a side effect which may change. Only the variant as specified in the XML is by design. The other one may vanish without notice. Note also, that the base link still uses "http:", as we can't change the XML so simply.

[Bjoeni]

Thanks!

But I'm pretty sure it is (or should) actually be exactly the other way round:

Replying to Bjoeni:

• Corrected the way schema locations are written in the file (technically, they weren't specified at all - just the name of the extension happened to be a URL pointing to the XSD schema)

The header looks like this, see comment:17:

<?xml version='1.0' encoding='UTF-8'?>
<gpx version="1.1" creator="JOSM GPX export" xmlns="http://www.topografix.com/GPX/1/1"
    xmlns:josm="http://josm.openstreetmap.de/gpx-extensions-1.1"
    xmlns:gpxd="http://josm.openstreetmap.de/gpx-drawing-extensions-1.0"
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xsi:schemaLocation="http://www.topografix.com/GPX/1/1 http://www.topografix.com/GPX/1/1/gpx.xsd http://josm.openstreetmap.de/gpx-extensions-1.1 http://josm.openstreetmap.de/gpx-extensions-1.1.xsd http://josm.openstreetmap.de/gpx-drawing-extensions-1.0 http://josm.openstreetmap.de/gpx-drawing-extensions-1.0.xsd">

According to the ​specs that means that:

namespace prefix	namespace URI (xmlns)	schema location (space separated pairs in xsi:schemaLocation)
(none)	​http://www.topografix.com/GPX/1/1	​http://www.topografix.com/GPX/1/1/gpx.xsd
josm	​http://josm.openstreetmap.de/gpx-extensions-1.1	​http://josm.openstreetmap.de/gpx-extensions-1.1.xsd
gpxd	​http://josm.openstreetmap.de/gpx-drawing-extensions-1.0	​http://josm.openstreetmap.de/gpx-drawing-extensions-1.0.xsd

The URI ​does not have to be retrievable (it just has to be a unique identifier of the namespace) but usually points to a human-readable schema documentation, for example the topografix namespace ​http://www.topografix.com/GPX/1/1. See also ​this thread on stackoverflow for a good comparision of the three.

Since we don't really have a human-readable documentation except for the comments in the XSD I suggested pointing to the preview of said document.

So I think the non-*.xsd namespace URI is a side effect that may change at any time.

[stoecker]

Well, that's something you introduced (or another change to GPX before?), which was not there before. None of the other formats have a schemaLocation entry. I added a note to the server config that it should remain a feature in the future as well.

Years ago I made links to the namespaces only because I found it strange that these don't point to something useful. But these are not pointing to a preview, but to the XSD and that wont change.

[Bjoeni]

Replying to stoecker:

Well, that's something you introduced (or another change to GPX before?), which was not there before.

It was introduced in 11 years ago in r1574, so it's been there quite a while ;) But you're right, it wasn't printed for the extension. I should've been more clear about that.

None of the other formats have a schemaLocation entry.

An example of GPX file headers produced by Garmin:
<?xml version="1.0" encoding="utf-8"?><gpx creator="Garmin Desktop App" version="1.1" xsi:schemaLocation="http://www.topografix.com/GPX/1/1 http://www.topografix.com/GPX/1/1/gpx.xsd http://www.garmin.com/xmlschemas/WaypointExtension/v1 http://www8.garmin.com/xmlschemas/WaypointExtensionv1.xsd http://www.garmin.com/xmlschemas/TrackPointExtension/v1 http://www.garmin.com/xmlschemas/TrackPointExtensionv1.xsd http://www.garmin.com/xmlschemas/GpxExtensions/v3 http://www8.garmin.com/xmlschemas/GpxExtensionsv3.xsd http://www.garmin.com/xmlschemas/ActivityExtension/v1 http://www8.garmin.com/xmlschemas/ActivityExtensionv1.xsd http://www.garmin.com/xmlschemas/AdventuresExtensions/v1 http://www8.garmin.com/xmlschemas/AdventuresExtensionv1.xsd http://www.garmin.com/xmlschemas/PressureExtension/v1 http://www.garmin.com/xmlschemas/PressureExtensionv1.xsd http://www.garmin.com/xmlschemas/TripExtensions/v1 http://www.garmin.com/xmlschemas/TripExtensionsv1.xsd http://www.garmin.com/xmlschemas/TripMetaDataExtensions/v1 http://www.garmin.com/xmlschemas/TripMetaDataExtensionsv1.xsd http://www.garmin.com/xmlschemas/ViaPointTransportationModeExtensions/v1 http://www.garmin.com/xmlschemas/ViaPointTransportationModeExtensionsv1.xsd http://www.garmin.com/xmlschemas/CreationTimeExtension/v1 http://www.garmin.com/xmlschemas/CreationTimeExtensionsv1.xsd http://www.garmin.com/xmlschemas/AccelerationExtension/v1 http://www.garmin.com/xmlschemas/AccelerationExtensionv1.xsd http://www.garmin.com/xmlschemas/PowerExtension/v1 http://www.garmin.com/xmlschemas/PowerExtensionv1.xsd http://www.garmin.com/xmlschemas/VideoExtension/v1 http://www.garmin.com/xmlschemas/VideoExtensionv1.xsd" xmlns="http://www.topografix.com/GPX/1/1" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:wptx1="http://www.garmin.com/xmlschemas/WaypointExtension/v1" xmlns:gpxtrx="http://www.garmin.com/xmlschemas/GpxExtensions/v3" xmlns:gpxtpx="http://www.garmin.com/xmlschemas/TrackPointExtension/v1" xmlns:gpxx="http://www.garmin.com/xmlschemas/GpxExtensions/v3" xmlns:trp="http://www.garmin.com/xmlschemas/TripExtensions/v1" xmlns:adv="http://www.garmin.com/xmlschemas/AdventuresExtensions/v1" xmlns:prs="http://www.garmin.com/xmlschemas/PressureExtension/v1" xmlns:tmd="http://www.garmin.com/xmlschemas/TripMetaDataExtensions/v1" xmlns:vptm="http://www.garmin.com/xmlschemas/ViaPointTransportationModeExtensions/v1" xmlns:ctx="http://www.garmin.com/xmlschemas/CreationTimeExtension/v1" xmlns:gpxacc="http://www.garmin.com/xmlschemas/AccelerationExtension/v1" xmlns:gpxpx="http://www.garmin.com/xmlschemas/PowerExtension/v1" xmlns:vidx1="http://www.garmin.com/xmlschemas/VideoExtension/v1">

Or do you mean other XML based JOSM formats?

Years ago I made links to the namespaces only because I found it strange that these don't point to something useful. But these are not pointing to a preview, but to the XSD and that wont change.

That's totally fine with me, just wanted us to be on the same page :) Only reason I personally preferred the preview is because it indirectly links the according changeset and ticket for reference.

[stoecker]

Replying to Bjoeni:

Replying to stoecker:

Well, that's something you introduced (or another change to GPX before?), which was not there before.

It was introduced in 11 years ago in r1574, so it's been there quite a while ;) But you're right, it wasn't printed for the extension. I should've been more clear about that.

I'm talking about the XSD-URL.

Or do you mean other XML based JOSM formats?

Yes.

[Bjoeni]

Replying to stoecker:

Replying to Bjoeni:

Replying to stoecker:

Well, that's something you introduced (or another change to GPX before?), which was not there before.

It was introduced in 11 years ago in r1574, so it's been there quite a while ;) But you're right, it wasn't printed for the extension. I should've been more clear about that.

I'm talking about the XSD-URL.

So am I. It was hardcoded for the GPX schema but then never added for the josm:* extension. From [1574#file3]:

trunk/src/org/openstreetmap/josm/io/GpxWriter.java

@@ -45 +45 @@
         this.data = data;
         out.println("<?xml version='1.0' encoding='UTF-8'?>");
-        out.println("<gpx version=\"1.1\" creator=\"JOSM GPX export\" xmlns=\"http://www.topografix.com/GPX/1/1\">");
+        out.println("<gpx version=\"1.1\" creator=\"JOSM GPX export\" xmlns=\"http://www.topografix.com/GPX/1/1\"\n" +
+                        "    xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\" \n" + 
+                        "    xsi:schemaLocation=\"http://www.topografix.com/GPX/1/1 http://www.topografix.com/GPX/1/1/gpx.xsd\">");
         indent = "  ";
         writeMetaData();

But actually, it really doesn't matter. It's there now and I hope everyone can live with that :)

[Klumbumbus]

Bjoeni, could you please summarize what has changed for the end user at the end of the day for Changelog?

[Bjoeni]

• Reading and writing of colors in *.gpx files
• Multiple colors per GPX layer supported (each track can have an individual color)
• Support of Garmin track extensions
• Saving of layer specific preferences in the GPX file instead of global preferences
• Conversion of track and segment related fields/tags between GPX <-> OSM
• Editing of GPX track color, name and description possible without prior conversion to OSM

I guess these are the most notable changes. Feel free to shorten or rephrase it as you see fit.

[Klumbumbus]

Thanks!

[simon04]

Over the past months I've been trying to reduce the memory consumption of JOSM. After loading a 55k wpt gpx file in JOSM, ​https://www.outdooractive.com/download.tour.gpx?i=17291189, I've noticed that 7.147 MiB out of 19.5 MiB are due to the gpx extensions, in particular every single org.openstreetmap.josm.data.gpx.WayPoint instance requires 344 bytes of memory. Since the corresponding gpx files does not use extensions at all, this is somewhat wasteful...

[Bjoeni]

I don't think that that has much to do with the Extensions.

In your example exactly one instance of
org.openstreetmap.josm.data.gpx.GpxExtension 48 B (0 %) 1 (0 %)
is created because the file has one extension:

    <extensions>
      <oa:oaCategory>longDistanceHikingTrail</oa:oaCategory>
    </extensions>

When the color is changed and the track saved three more instances are created:
org.openstreetmap.josm.data.gpx.GpxExtension 192 B (0 %) 4 (0 %)
because it's added to the track:

    <extensions>
      <oa:oaCategory>longDistanceHikingTrail</oa:oaCategory>
      <josm:layerPreferences>
        <josm:entry key="colormode" value="0"/>
      </josm:layerPreferences>
    </extensions>
...
    <extensions>
      <gpxd:color>#FF0033</gpxd:color>
    </extensions>

So that behaves as expected.

The only issue is that each IWithAttributes has a GpxExtensionCollection that is usually empty, with 32b each:
org.openstreetmap.josm.data.gpx.GpxExtensionCollection 1.786.976 B (0,5 %) 55.843 (0,9 %)
I guess that could be changed so that the GpxExtensionCollection is only created when it's actually needed.

---

in particular every single org.openstreetmap.josm.data.gpx.WayPoint instance requires 344 bytes of memory

I also could not reproduce that:
org.openstreetmap.josm.data.gpx.WayPoint 4.020.336 B (0,5 %) 55.838 (0,4 %)
That's 72b each for me.

[simon04]

Replying to simon04:

I've noticed that 7.147 MiB out of 19.5 MiB are due to the gpx extensions, in particular every single org.openstreetmap.josm.data.gpx.WayPoint instance requires 344 bytes of memory.

I've loaded the mentioned GPX file with and without the following patch. Then, I've computed the retained size of the org.openstreetmap.josm.data.gpx.GpxTrack

src/org/openstreetmap/josm/data/gpx/WithAttributes.java

@@ -21 +21 @@
      */
     public Map<String, Object> attr = new HashMap<>(0);
 
-    /**
-     * The "exts" collection contains all extensions.
-     */
-    private final GpxExtensionCollection exts = new GpxExtensionCollection(this);
-
     /**
      * Returns the Object value to which the specified key is mapped,
      * or {@code null} if this map contains no mapping for the key.
@@ -87 +82 @@
 
     @Override
     public GpxExtensionCollection getExtensions() {
-        return exts;
+        return new GpxExtensionCollection(this);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(attr, exts);
+        return Objects.hash(attr);
     }
 
     @Override
@@ -108 +103 @@
             if (other.attr != null)
                 return false;
         } else if (!attr.equals(other.attr))
-            return false;
-        if (exts == null) {
-            if (other.exts != null)
-                return false;
-        } else if (!exts.equals(other.exts))
             return false;
         return true;
     }

Replying to Bjoeni:

in particular every single org.openstreetmap.josm.data.gpx.WayPoint instance requires 344 bytes of memory

I also could not reproduce that:
org.openstreetmap.josm.data.gpx.WayPoint 4.020.336 B (0,5 %) 55.838 (0,4 %)
That's 72b each for me.

72 bytes is the shallow size, 344 bytes is the retained size (including the attr and exts objects).

[GerdP]

It's probably obvious that this patch would work as is?
See e.g. GpxWriter

        data.getExtensions().removeAllWithPrefix("josm");
        if (data.fromServer) {
            data.getExtensions().add("josm", "from-server", "true");
        }

My understanding is that this will fail.

[Bjoeni]

Replying to simon04:

Replying to Bjoeni:

in particular every single org.openstreetmap.josm.data.gpx.WayPoint instance requires 344 bytes of memory

I also could not reproduce that:
org.openstreetmap.josm.data.gpx.WayPoint 4.020.336 B (0,5 %) 55.838 (0,4 %)
That's 72b each for me.

72 bytes is the shallow size, 344 bytes is the retained size (including the attr and exts objects).

You're right, that was my mistake. I actually never really had to deal with memory management in Java before.

The main issue appears to be the stack, which - for whatever reason - takes up 140b despite being empty:

[Bjoeni]

Replying to GerdP:

It's probably obvious that this patch would work as is?

I assume you meant wouldn't work?
If so, yes I guess that was just for debugging.

@Simon, I'll have a look and provide a patch.

[Bjoeni]

Replying to Bjoeni:

@Simon, I'll have a look and provide a patch.

see #20793