time string in GPX export is wrong

[ikke1372@…]

What steps will reproduce the problem?

1. select a route in Relations
2. rightclick : export GPX file from first or last number
3. save : works normal.

What is the expected result? : time string in gpx-file

 <time>2018-10-19T12:55:03Z</time>

What happens instead? in gpx year = 50838

  <trk>
    <name>Platwijers Gele wandeling</name>
    <trkseg>
      <trkpt lat="50.9778408" lon="5.3039118">
        <time>+50838-07-11T15:39:56Z</time>
      </trkpt>

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

I tried this with more then five routes : all with the same error :date in the time string is out of range year 50838 and not 2018

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2018-10-28 22:27:31 +0100 (Sun, 28 Oct 2018)
Build-Date:2018-10-28 21:33:32
Revision:14382
Relative:URL: ^/trunk

Identification: JOSM/1.5 (14382 nl) Windows 10 64-Bit
OS Build number: Windows 10 Home 1803 (17134)
Memory Usage: 550 MB / 3618 MB (220 MB allocated, but free)
Java version: 1.8.0_191-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1080
Maximum Screen Size: 1920x1080

Plugins:
+ buildings_tools (34572)
+ reverter (34552)
+ utilsplugin2 (34506)

Map paint styles:
+ https://josm.openstreetmap.de/josmfile?page=Styles/Noname&zip=1

Last errors/warnings:
- W: Cannot lock cache directory. Will not use disk cache
- W: No configuration settings found.  Using hardcoded default values for all pools.

[jul2]

one of the exports (GPX)

[Don-vip]

Replying to ikke1372@…:

1. select a route in Relations

Can you please give us an example (relation id)?

[ikke1372@…]

You can try this id's :

Relation: Kruwers wandelroute (8722577)
Relation: Netepad (8187352)
Relation: nieuw Mol Om (7791375)
Relation: Spijkers met koppen (8197424)
Relation: Platwijers Gele wandeling (2696418)

with JOSM version 14422 today same result :

    <name>Platwijers Gele wandeling</name>
    <trkseg>
      <trkpt lat="50.9778408" lon="5.3039118">
        <time>+50841-08-11T18:16:42Z</time>

greatings
jul2

[Don-vip]

@cmuelle8: it seems the time calculation in ExportRelationToGpxAction added in #15606 is wrong.

[cmuelle8]

@Don-vip: I can confirm the problem and have a look at it currently.

The code depends on functions added 3 years ago in changeset:8565 in AbstractPrimitive.java. The GpxExport should add the timestamp of the last/current revision of the primitive. For the example given in comment:3 this should be the timestamp of field "Edited at:"

Node: 415817033
  Data Set: 2e98f6b3
  Edited at: 2014-02-14T23:51:34Z
  Edited by: Eebie (715684)
  Version: 4
  In changeset: 20568440
  Coordinates: 50.9778408, 5.3039118
  Coordinates (projected): 590428.760788435, 6617374.952449033
  UTM Zone: 31N
  Part of: 
    Way: 183491629
    Way: Goorstraat (97669590)

Aside from this conversion being incorrect atm, question is whether node timestamps exported to GPX are meaningful at all!? As the exported track is synthesized, there are three options to fill timestamp attribute of waypoints:

• synthesize timestamps (i.e. take start date from last-edit-timestamp of the relation itself take current date or offset, and artificially increment by a fixed or variable amount for each node)
• re-use last-edited-timestamp of osm nodes (should be current default, does not work, but is it meaningful!?)
• do not export timestamps

Another issue is that we DO NOT want these gpx track exports to be uploaded to OSM, which I believe blocks gpx uploads with invalid or no wpt timestamps (haven't tried). While synthesizing meaningful timestamps may be appealing to a user, we would have to think about measures that makes it, by default, easy to discriminate such exports from tracked device data.

Of course, using gpsbabel or a text editor, it will always be possible to further modify such exports, but the main point should be to protect the gpx upload feature of the osm website from thoughtless or accidental use when fed with synthesized tracks.

---

Because of these issues, it may be best to turn off waypoint timestamping for these exports all together, given that last-edit-timestamps of osm nodes is of little use (!?) even if it would work correctly. It may be nice as a debug feature to the GpxExportAction itself, but it should have little value to prod-users.

What are your oppinions on the issues raised?

EDIT: Stroke out some sections to reflect current state of the export code, last-edit-timestamp of the node has to date never been used in ExportRelationToGPXAction, it has always (attempted to) synthesize timestamps to be sequential from an offset close to the time the export function is used (read now() - 24 hours offset to be exact).

[cmuelle8]

josm-ticket-16995-corrupt-timestamps-when-using-ExportRelationToGpxAction-fix.patch

[cmuelle8]

@Don-vip: Please thoroughly test, before applying.

Esp. the data type change from int to long for member AbstractPrimitive.timestamp may trigger regressions and need more changes.

In theory this change can be postponed until the year 2038 (at which an int measuring seconds from the unix epoch will overflow), so if you want another patch without it and go the slow route this is ok with me.

Don't take this data type change light-hearted, quote from IPrimitive.java

    /**
     * Time of last modification to this object. This is not set by JOSM but
     * read from the server and delivered back to the server unmodified. It is
     * used to check against edit conflicts.
     *
     * @return last modification as timestamp
     * @see #setRawTimestamp
     */
    long getRawTimestamp();

I've not confirmed that this check continues to work with the data type changes I did.
Also, serialized objects derived from PrimitiveData.java will be incompatible between old versions and those after the change.

Then, each osm primitive will take +4 bytes memory. Given, that this datatype being an int is not an issue until 2038, it may make more sense to postpone that change to another ten or fifteen years from now, as to not shrink the dataset sizes currently loadable. I don't want to piss people off for a feature they do not perceive being immediate.

I may upload another version of the patch with those bits reverted and let you decide..

[cmuelle8]

josm-ticket-16995-corrupt-timestamps-when-using-ExportRelationToGpxAction-fixwithout-y2038-fix.patch

[cmuelle8]

@Don-vip: I guess you can apply the smaller patch first (with overseeable implications) and move the other issue to a new ticket.. (one that may raise in priority as 2038 nears..)

@ikke1372: If you're able to compile josm yourself, plz help test and report back if the issue is fixed for you. In any other case, you'll have to wait a tiny bit for Don-vip I suppose.

[cmuelle8]

​epochconverter.com is an online tool that may aid in debugging related issues and for readers new to this ticket, it should be noted that josm is not the only software that will need patching wrt y2038-issue (unless people find a way to rebase the epoch, with the added burden to differentiate different versions of epoch timestamps, of course).

​lwn.net e.g. also has an article about it, exhibiting filesystem timestamp data types of signed integer width.

Maybe josm already has a proper bug ticket for this issue? (I did not employ the search function, yet.)

[cmuelle8]

Oh, for starters we could ramp up AbstractPrimitive.timestamp from int to uint, I guess. Since it's the same width it won't break serialization objects and should work nicely with whatever is reported by the api servers now. That'd give room until 2106. But it's really another issue and does not belong into this ticket..

EDIT: ​Oracle says that from Java 8 int can be used like uint, but you have to be careful to use static methods like Integer.compareUnsigned(), Integer.divideUnsigned() to treat the variable with. I have not reviewed the JOSM code enough to tell whether these are already used / are usable to achieve "2038-safeness".

EDIT2: Doing a quick search in josm code yields no results, so I assume the current int timestamp at the core of AbstractPrimitive is not worked with employing the ​Unsigned Integer API available in Java8 and higher.

find workspace/JOSM/ -type f -name "*.java" -exec grep -H compareUnsigned {} \;
find workspace/JOSM/ -type f -name "*.java" -exec grep -H parseUnsigned {} \;
find workspace/JOSM/ -type f -name "*.java" -exec grep -H toUnsigned {} \;

[Don-vip]

With your patch, do we keep the waypoint date in GpxConstants.PT_TIME during OSM->GPX conversion (OsmDataLayer.nodeToWayPoint)?

I prefer keeping timestamp as int and using the Java 8 unsigned int API. Otherwise it will break many plugins I think.

[cmuelle8]

Yes, I've just extra-tested this:

• loaded an GPX track, exported that right away to 1.gpx
• converted the GPX to OSM data layer
  • dialog asking which gpx fields should be converted confirmed with 'all fields'
  • gpx-exported the osm layer to 2.gpx
• converted the OSM data layer back to GPX layer, exported that to 3.gpx

The result was identical data in 1.gpx, 2.gpx and 3.gpx - the nodes in the osm data layer had proper time=* (i.e. GpxConstants.PT_TIME) kv-pairs correlating to waypoint time data in the gpx files.

---

I do support using the unsigned int API for that other problem as an interim, but the long term fix is using a long data type.

time_t on x86_64 Linux is, by default, a datatype of 8 bytes length (long int) already, dictated by Linux headers.
The notable exception occurs when code is compiled as 32 bit binary on x86_64 using e.g. gcc -m32.

In the latter case headers will declare time_t to be 4 bytes wide. You can check this yourself, see e.g. the ​german wikipedia talk page on the problem that has c code to exploit the details.

But again, we should move this issue to another ticket. It has a much broader scope than the OP's issue and can be dealt with separately - and it's less urgent than the issue here.

[Don-vip]

Replying to cmuelle8:

Yes, I've just extra-tested this:

Great!

I do support using the unsigned int API for that other problem as an interim, but the long term fix is using a long data type.

Agreed.

But again, we should move this issue to another ticket. It has a much broader scope than the OP's issue and can be dealt with separately - and it's less urgent than the issue here.

Also agreed. Can you please create the ticket? We'll go with unsigned API for now.

[Don-vip]

In 14434/josm:

fix #16995 - fix timestamp in GPX exports (patch by cmuelle8) + use Java 8 unsigned int API

[Don-vip]

It seems OsmDataLayerTest.testToGpxData must be updated.

[cmuelle8]

It depends on wheter we want the time attribute of a WayPoint object to be a Date or a String. The test would have failed in r14433 already if it had been more comprehensive:

    @Test
    public void testToGpxData() throws IllegalDataException {
        ds.mergeFrom(OsmReader.parseDataSet(new ByteArrayInputStream((
                "<?xml version='1.0' encoding='UTF-8'?>\n" +
                "<osm version='0.6' upload='false' generator='JOSM'>\n" +
                "  <node id='-546306' timestamp='2018-08-01T10:00:00Z' lat='47.0' lon='9.0'>\n" +
                "    <tag k='ele' v='123' />\n" +
                "    <tag k='time' v='2018-08-01T10:00:00Z' />\n" +
                "  </node>\n" +
                "  <node id='-546307' timestamp='2018-08-01T10:01:00Z' lat='47.1' lon='9.1'>\n" +
                "    <tag k='ele' v='456' />\n" +
                "    <tag k='time' v='2018-08-01T10:01:00Z' />\n" +
                "  </node>\n" +
                "  <node id='-546308' timestamp='2018-08-01T10:02:00Z' lat='47.05' lon='9.05'>\n" +
                "    <tag k='ele' v='789' />\n" +
                "  </node>\n" +
                "  <way id='-546309'>\n" +
                "    <nd ref='-546306' />\n" +
                "    <nd ref='-546307' />\n" +
                "    <nd ref='-546308' />\n" +
                "  </way>\r\n" +
                "</osm>").getBytes(StandardCharsets.UTF_8)), null));
        GpxData gpx = layer.toGpxData();
        assertNotNull(gpx);
        // Check metadata
        assertEquals(new Bounds(47.0, 9.0, 47.1, 9.1), gpx.recalculateBounds());
        // Check there is no waypoint
        assertTrue(gpx.getWaypoints().isEmpty());
        // Check that track is correct
        assertEquals(1, gpx.getTrackCount());
        GpxTrack track = gpx.getTracks().iterator().next();
        Collection<GpxTrackSegment> segments = track.getSegments();
        assertEquals(1, segments.size());
        Collection<WayPoint> trackpoints = segments.iterator().next().getWayPoints();
        assertEquals(3, trackpoints.size());
        Iterator<WayPoint> it = trackpoints.iterator();
        DateFormat gpxFormat = DateUtils.getGpxFormat();
        WayPoint p1 = it.next();
        assertEquals(new LatLon(47.0, 9.0), p1.getCoor());
        assertEquals("123", p1.get(GpxConstants.PT_ELE));
        assertEquals("2018-08-01T10:00:00.000Z", gpxFormat.format(p1.get(GpxConstants.PT_TIME)));
        WayPoint p2 = it.next();
        assertEquals(new LatLon(47.1, 9.1), p2.getCoor());
        assertEquals("456", p2.get(GpxConstants.PT_ELE));
        assertEquals("2018-08-01T10:01:00.000Z", gpxFormat.format(p2.get(GpxConstants.PT_TIME)));
        WayPoint p3 = it.next();
        assertEquals(new LatLon(47.05, 9.05), p3.getCoor());
        assertEquals("789", p3.get(GpxConstants.PT_ELE));
        assertEquals("2018-08-01T10:02:00.000Z", gpxFormat.format(p3.get(GpxConstants.PT_TIME)));
    }

Note that the added third coordinate in the modified/supplemented test above does have a timestamp, but not a time tag.
The relevant nodeToWayPoint conversion function in r14433 was:

    public static WayPoint nodeToWayPoint(Node n, long time) {
        WayPoint wpt = new WayPoint(n.getCoor());

        // Position info

        addDoubleIfPresent(wpt, n, GpxConstants.PT_ELE);

        try {
            if (time > 0) {
                wpt.put(GpxConstants.PT_TIME, DateUtils.fromTimestamp(time));
                wpt.setTime(time);
            } else if (n.hasKey(GpxConstants.PT_TIME)) {
                wpt.put(GpxConstants.PT_TIME, DateUtils.fromString(n.get(GpxConstants.PT_TIME)));
                wpt.setTime();
            } else if (!n.isTimestampEmpty()) {
                wpt.put(GpxConstants.PT_TIME, DateUtils.fromTimestamp(n.getRawTimestamp()));
                wpt.setTime();
            }
        } catch (UncheckedParseException e) {
            Logging.error(e);
        }
        [...]

The problem is that

• DateUtils.fromString(x) associates a Date type object to the attribute with the key GpxConstants.PT_TIME, while
• DateUtils.fromTimestamp(x) associates a String type object.

While the unit test works/worked ok with Date type objects, it does not with String ones. The WayPoint class itself in r14433 was agnostic and dealt with both possibilities, as you can deduct from the occurence of the ternary operator in setTimeFromAttribute() function:

                final Object obj = get(PT_TIME);
                final Date date = obj instanceof Date ? (Date) obj : DateUtils.fromString(obj.toString());

---

In r14434, the revision with the fix for #16995, setting the GpxConstants.PT_TIME attribute was internalized into the WayPoint class and all setTime function variants associate String type objects exclusively with the attribute.

If you want to restore the previous behavior this may be easily done by

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

@@ -135 +135 @@
      */
     public void setTime(Date time) {
         this.time = time.getTime() / 1000.;
-        this.attr.put(PT_TIME, DateUtils.fromDate(time));
+        this.attr.put(PT_TIME, time);
     }
 
     /**

The (old) unit test will pass again, but this won't fix the extended one proposed with this comment. I'm pretty sure it is beneficial to settle on one specific java object type to be used for the GpxConstants.PT_TIME attribute in the WayPoint class, instead of restoring the previous mess that used multiple types.

However, I'm not quite sure, which of these types is best suited here. Given that ​a Date object occupies 32 bytes of memory it may be more memory efficient to use Date than to store String object to hold e.g. 2018-08-01T10:00:00.000Z that ​according to javamex.com's string_memory_usage article uses a minimum of 88 bytes in memory (per waypoint!).

EDIT: Below are three alternatives to properly fix the issue, but depending on if and how the WayPoint class is used in plugins, each may trigger work to do afterwards. Trying to avoid this is the _sole_ reason to rather depend on the patch with the quickfix given above.

---

• So if we are to use Date exclusively for the GpxConstants.PT_TIME attribute in Waypoint.java, we do not need to change the test assertions - neither in the old nor in the proposed test above. Patch to use Date exclusively:

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

  @@ -1 +1 @@
   // License: GPL. For details, see LICENSE file.
   package org.openstreetmap.josm.data.gpx;
   
  +import static org.junit.Assert.assertTrue;
  +
   import java.awt.Color;
  -import java.time.DateTimeException;
   import java.util.ArrayList;
   import java.util.Date;
   import java.util.List;
  @@ -14 +15 @@
   import org.openstreetmap.josm.data.osm.search.SearchCompiler.Match;
   import org.openstreetmap.josm.data.projection.Projecting;
   import org.openstreetmap.josm.tools.Logging;
  -import org.openstreetmap.josm.tools.UncheckedParseException;
  -import org.openstreetmap.josm.tools.date.DateUtils;
   import org.openstreetmap.josm.tools.template_engine.TemplateEngineDataProvider;
   
   /**
  @@ -135 +134 @@
        */
       public void setTime(Date time) {
           this.time = time.getTime() / 1000.;
  -        this.attr.put(PT_TIME, DateUtils.fromDate(time));
  +        this.attr.put(PT_TIME, time);
       }
   
       /**
  @@ -155 +154 @@
        * @since 13210
        */
       public void setTime(long ts) {
  -        this.time = ts;
  -        this.attr.put(PT_TIME, DateUtils.fromTimestamp(ts));
  +        setTimeInMillis(ts*1000);
       }
   
       /**
  @@ -167 +165 @@
        */
       public void setTimeInMillis(long ts) {
           this.time = ts / 1000.;
  -        this.attr.put(PT_TIME, DateUtils.fromTimestampInMillis(ts));
  +        this.attr.put(PT_TIME, new Date(ts));
       }
   
       /**
  @@ -179 +177 @@
           if (attr.containsKey(PT_TIME)) {
               try {
                   final Object obj = get(PT_TIME);
  -                final Date date = obj instanceof Date ? (Date) obj : DateUtils.fromString(obj.toString());
  +                assertTrue(obj instanceof Date);
  +                final Date date = (Date) obj;
                   time = date.getTime() / 1000.;
                   return date;
  -            } catch (UncheckedParseException | DateTimeException e) {
  +            } catch (AssertionError e) {
                   Logging.warn(e);
                   time = 0;
               }

• Should we use String objects exclusively the test assertions would need to change. In this case the strings could be
  • either compared directly, e.g. if the DateUtils.getGpxFormat() formatter was used to store the strings in Waypoint.java:

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

    @@ -135 +135 @@
          */
         public void setTime(Date time) {
             this.time = time.getTime() / 1000.;
    -        this.attr.put(PT_TIME, DateUtils.fromDate(time));
    +        this.attr.put(PT_TIME, DateUtils.getGpxFormat().format(time));
         }
     
         /**
    @@ -155 +155 @@
          * @since 13210
          */
         public void setTime(long ts) {
    -        this.time = ts;
    -        this.attr.put(PT_TIME, DateUtils.fromTimestamp(ts));
    +        setTimeInMillis(ts*1000);
         }
     
         /**
    @@ -167 +166 @@
          */
         public void setTimeInMillis(long ts) {
             this.time = ts / 1000.;
    -        this.attr.put(PT_TIME, DateUtils.fromTimestampInMillis(ts));
    +        this.attr.put(PT_TIME, DateUtils.getGpxFormat().format(new Date(ts)));
         }
     
         /**

  • or by converting String to Date first, i.e. gpxFormat.format(DateUtils.fromString(p1.get(GpxConstants.PT_TIME))) with no changes to current revision of Waypoint.java (i.e. PT_TIME attributes stored as String, but not having been normalized by the DateUtils.getGpxFormat() formatter). This means updates exclusively to fix the unit test in OsmDataLayerTest.java:

    test/unit/org/openstreetmap/josm/gui/layer/OsmDataLayerTest.java

    @@ -220 +220 @@
                     "    <tag k='ele' v='456' />\n" +
                     "    <tag k='time' v='2018-08-01T10:01:00Z' />\n" +
                     "  </node>\n" +
    -                "  <way id='-546308'>\n" +
    +                "  <node id='-546308' timestamp='2018-08-01T10:02:00Z' lat='47.05' lon='9.05'>\n" +
    +                "    <tag k='ele' v='789' />\n" +
    +                "  </node>\n" +
    +                "  <way id='-546309'>\n" +
                     "    <nd ref='-546306' />\n" +
                     "    <nd ref='-546307' />\n" +
    +                "    <nd ref='-546308' />\n" +
                     "  </way>\r\n" +
                     "</osm>").getBytes(StandardCharsets.UTF_8)), null));
             GpxData gpx = layer.toGpxData();
    @@ -237 +241 @@
             Collection<GpxTrackSegment> segments = track.getSegments();
             assertEquals(1, segments.size());
             Collection<WayPoint> trackpoints = segments.iterator().next().getWayPoints();
    -        assertEquals(2, trackpoints.size());
    +        assertEquals(3, trackpoints.size());
             Iterator<WayPoint> it = trackpoints.iterator();
             DateFormat gpxFormat = DateUtils.getGpxFormat();
             WayPoint p1 = it.next();
             assertEquals(new LatLon(47.0, 9.0), p1.getCoor());
             assertEquals("123", p1.get(GpxConstants.PT_ELE));
    -        assertEquals("2018-08-01T10:00:00.000Z", gpxFormat.format(p1.get(GpxConstants.PT_TIME)));
    +        assertEquals("2018-08-01T10:00:00.000Z", gpxFormat.format(DateUtils.fromString((String) p1.get(GpxConstants.PT_TIME))));
             WayPoint p2 = it.next();
             assertEquals(new LatLon(47.1, 9.1), p2.getCoor());
             assertEquals("456", p2.get(GpxConstants.PT_ELE));
    -        assertEquals("2018-08-01T10:01:00.000Z", gpxFormat.format(p2.get(GpxConstants.PT_TIME)));
    +        assertEquals("2018-08-01T10:01:00.000Z", gpxFormat.format(DateUtils.fromString((String) p2.get(GpxConstants.PT_TIME))));
    +        WayPoint p3 = it.next();
    +        assertEquals(new LatLon(47.05, 9.05), p3.getCoor());
    +        assertEquals("789", p3.get(GpxConstants.PT_ELE));
    +        assertEquals("2018-08-01T10:02:00.000Z", gpxFormat.format(DateUtils.fromString((String) p3.get(GpxConstants.PT_TIME))));
         }
     
         /**

---

The last of the three diffs is probably what you had in mind with your request in comment:16, but be aware that code may write differently formatted strings out to GPX files, if it spares the conversion that the unit test does. It could simply

• write(file, pX.get(GpxConstants.PT_TIME)) instead of doing
• write(file, gpxFormat.format(DateUtils.fromString((String) pX.get(GpxConstants.PT_TIME))))

which may lead to randomness in the date string format (e.g. one timestamp with .000 ms appendix, another without, yet another without timezone code and so on), of course depending on the source data.

My current favorite seems to be to rely on storing Date objects exclusively for the GpxConstants.PT_TIME attribute of class WayPoint. It seems to be the most memory efficient and should lead to uniform formatting of values in any case.

[cmuelle8]

@Don-vip: Yes, the preferred solution may need update of some plugins (if they write String object to GpxConstants.PT_TIME attribute of WayPoint). On the other hand, using a fixed data type for that attribute will lessen the maintenance burden in the future. It also makes behavior of WayPoint more predictable in terms of memory consumed.

Leaving that data type to be disambiguous may seem convenient when instantiating/writing WayPoint objects, but is less so when using/reading them: That ternary operator currently living in setTimeFromAttribute basically cries out to be copied to each and every place that the attribute is used/read (unless you have a unit test that only tests half of the possibilities).

EDIT: Code broken after this should be fixable by associating DateUtils.fromString(obj.toString()) instead of obj with GpxConstants.PT_TIME attributes. (It basically means moving the functionality of that ternary operator out of WayPoint to those clients that generate objects of it.)

[Don-vip]

In 14445/josm:

fix #16995 - use exclusively Date objects in waypoints (patch by cmuelle8, modified)

[Don-vip]

Thanks a lot for the analysis!

[Don-vip]

I've missed something: https://josm.openstreetmap.de/jenkins/job/JOSM/4748/testReport/

[cmuelle8]

Yes, this was expected. We need to fix the places where GpxConstants.PT_TIME attribute of WayPoint objects is set externally using lines like wpt.put(GpxConstants.PT_TIME, string_object). Those places in the code should now use wpt.setTime(x) instead.

If it is unacceptable that the josm build is broken until we've found and fixed all those places, then I suggest you revert to r14444, and apply the quickfix as an interim (i.e. the first diff in comment:17). After that, we can work on a more complete patch to have pure Date type in WayPoint.java, without fearing people flooding trac with bug reports.

[cmuelle8]

Btw, you did not seem to have looked at attachment:josm_WayPoint.java_to-use_Date_exclusively_for_PT_TIME_attribute.patch​ for your changeset. My proposed patch included changes to Marker.java and OsmDataLayerTest.java, but in r14445 only WayPoint.java changed.

[cmuelle8]

Ok, for starters I've adopted GpxReader.java:

src/org/openstreetmap/josm/io/GpxReader.java

@@ -26 +26 @@
 import org.openstreetmap.josm.data.gpx.ImmutableGpxTrack;
 import org.openstreetmap.josm.data.gpx.WayPoint;
 import org.openstreetmap.josm.tools.Logging;
+import org.openstreetmap.josm.tools.UncheckedParseException;
 import org.openstreetmap.josm.tools.XmlUtils;
+import org.openstreetmap.josm.tools.date.DateUtils;
 import org.xml.sax.Attributes;
 import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
@@ -440 +442 @@
                         currentWayPoint.put(localName, 0f);
                     }
                     break;
-                case "time":
+                case GpxConstants.PT_TIME:
+                    try {
+                        currentWayPoint.setTime(DateUtils.fromString(accumulator.toString()));
+                    } catch (UncheckedParseException e) {
+                        Logging.error(e);
+                    }
+                    break;
                 case "cmt":
                 case "desc":
                     currentWayPoint.put(localName, accumulator.toString());
-                    currentWayPoint.setTimeFromAttribute();
                     break;
                 case "rtept":
                     currentState = states.pop();

But please wait for me to attach a patch, instead of committing this right away. Please consider this a preview to discuss and share thoughts on. I will submit a new patch based on r14445 obsoleting the last attachment and retaining the Marker.java and OsmDataLayerTest.java bits you missed.

[cmuelle8]

fix regressions of r14445, fix GpxReader.java to put Date object into WayPoint HashMaps, fix unit tests (exactly those shown as failures in https://josm.openstreetmap.de/jenkins/job/JOSM/4748/testReport/)

[cmuelle8]

ConvertFromGpxLayerAction.java was one example of the added maintenance burden, resulting from ambiguity of the time data type: Just as the old setTimeFromAttribute() it accounted for String and Date objects with separate code branches to decode the PT_TIME attribute.

Since we can be sure now to strictly have Date objects, I have removed the branch that deals with time as String objects from ConvertFromGpxLayerAction.java, which is one part of the patch.

It is noteworthy that Date objects are timezone-less, converting/printing them with default jdk methods uses the default time zone of the calendar: This is why we have DateUtils.fromDate(Date date) to convert Date to UTC-timezone-coded String objects, regardless of current locale/calendar setting. When dealing with GPX, care must be taken to rely on this custom method. Otherwise GPX conversion process will differ depending on users locale. Afaik that function is currently employed, but it seems necessary to emphasize this here once more.

@Don-vip: Thanks for your patience and that second pair of eyes to spot potential issues. I have a feeling, that there are some more places needing change as we have not looked at plugin code yet..

[Don-vip]

In 14446/josm:

see #16995 - use_Date exclusively for PT_TIME attribute (patch by cmuelle8)

[cmuelle8]

fix minor issues in response to https://josm.openstreetmap.de/jenkins/job/JOSM/4749/findbugsResult/new/

[Don-vip]

In 14447/josm:

fix #16995 - fix spotbugs regressions (patch by cmuelle8)

[cmuelle8]

couple of issues I've noticed so far:

• using editgpx plugin triggers our new "Unsupported waypoint time value", so this plugin needs update
• conversion of Date objects to time strings should IMHO append milliseconds only if different from zero, i.e. we should convert to Z suffix only instead of .000Z - this is mostly cosmetic, but differs from previous behavior

[cmuelle8]

proper fix to timestamp formatting in GPX files, does not append trailing subsecond zeroes

[cmuelle8]

DateUtils.getGpxFormat() formatter may be useful in some respect when parsing, but when writing GPX DateUtils.fromDate() is sufficient as emphasized in comment:26. In particular the latter fulfills two conditions:

• it does not append trailing sub-second zeroes (in particular: if the timestamp resolution is seconds, it won't append useless .000Z but a mere Z character)
• it always outputs UTC timezoned date strings

Related read:

• ​Understanding sub-second values in a timestamp, quote: The decimal off of the second should be treated as, well, a decimal. 0.1 seconds == 100 milliseconds

@Don-vip: The patch will restore the previous gpx export timestamp writing behavior. Also, can you have a look at the editgpx plugin? I see ​you've last touched this yourself in 2016.

[Don-vip]

In 14448/josm:

see #16995 - proper fix to timestamp formatting in GPX files, does not append trailing subsecond zeroes (patch by cmuelle8)

[Don-vip]

Plugin editgpx should be fixed in [o34737:34738].

[Don-vip]

In 14453/josm:

see #16995 - update unit test

[cmuelle8]

de-duplicate storage of timestamp within WayPoint.java and refactor some methods, added documentation, added some robustness against legacy code (will also log a warning if detected)

[cmuelle8]

@Don-vip: I've done some further refactoring of class WayPoint, if you're interested. It deduplicates the duplicate storage of the time value in WayPoint.java and solely relies on the PT_TIME attribute. This further decreases memory usage, but adds some function calls (getter-methods-overhead compared to direct member access - should be neglectable).

• there's a fix to Marker.java ultimately needed, because we introduced a regression there - it should use setTimeInMillis(), not setTime()
• getTime() signature changed, it returns a Double - this is the drop-in-replacement for the deleted double time member variable - the previous behavior of getTime() moved to getDate()
  • it may once again affect editgpx plugin because of that
• unit tests passed, but I did not get to test editgpx plugin with the attached patch yet

Thx for reviewing.

[cmuelle8]

Ideas for further improvement:

---

Now that reading/writing PT_TIME is an internal job of class WayPoint, it seems easy to further reduce memory usage, by just associating a Long object which is 16 bytes in memory, compared to Date with 32 bytes. The necessary changes should be small and are limited to that class.

Given that the export feature is heavily used, we'd need an efficient equivalent to String DateUtils.fromDate(x), i.e. a method String DateUtils.fromLong(x) that does not simply defer its workload to fromDate, otherwise the mem gain will be shadowed by the increase in cpu cycles.

Should there be a demand for this, it may be feasible to work into that direction - just saying. For now, the gain we had moving from Strings to Dates should be fair enough.

[Don-vip]

In 14456/josm:

fix #16995 - de-duplicate storage of timestamp within WayPoint and refactor some methods, added documentation, added some robustness against legacy code (will also log a warning if detected). Patch by cmuelle8, modified

[Don-vip]

Thanks for the patch! I have modified some things to have a smaller change in the end. Please avoid moving methods while changing stuff, this makes the review harder ;) Can you please open a new ticket for the next proposed improvements? That will be all for the 18.11 release.

[Don-vip]

In 14459/josm:

see #16995 - remove unused imports

[Don-vip]

In 14460/josm:

see #16995 - fix spotbugs/pmd/sonar warnings

[GerdP]

Replying to Don-vip:

In 14456/josm:

fix #16995 - de-duplicate storage of timestamp within WayPoint and refactor some methods, added documentation, added some robustness against legacy code (will also log a warning if detected). Patch by cmuelle8, modified

This change introduced a lot of rather useless log messages when you handle a track without timestamps, e.g one created by brouter.

...
2019-04-12 10:49:37.595 INFO: Waypoint time value unset
2019-04-12 10:49:37.595 INFO: Waypoint time value unset
2019-04-12 10:49:37.595 INFO: Waypoint time value unset
2019-04-12 10:49:37.595 INFO: Waypoint time value unset
...

For a track with many points this can take many seconds and you get no response in the dialog when you try e.g. action "Choose visible tracks". Since the message doesn't tell me where the point is located or in which file it was found the message should only appear once.

[anonymous]

This change introduced a lot of rather useless log messages when you handle a track without timestamps, e.g one created by brouter.

...
2019-04-12 10:49:37.595 INFO: Waypoint time value unset
2019-04-12 10:49:37.595 INFO: Waypoint time value unset
2019-04-12 10:49:37.595 INFO: Waypoint time value unset
2019-04-12 10:49:37.595 INFO: Waypoint time value unset
...

For a track with many points this can take many seconds and you get no response in the dialog when you try e.g. action "Choose visible tracks". Since the message doesn't tell me where the point is located or in which file it was found the message should only appear once.

Do we have a method in JOSM that throttles output if a log message repeats? In dmesg output I've often seen messages like 'last message repeated n times' which seems to the appropiate solution to use here.

Just removing the log message should disturb cases in which just a couple of wpts with unset TS in an otherwise timestamped GPX.

Other options are

• print a message once per GPX file opened, e.g. "Waypoints with unset time values found."
• don't print any message if the number of waypoints without timestamp equals the number of waypoints

[simon04]

In 17130/josm:

see #16995 - Remove WayPoint.LegacyMap (to be removed mid 2019)

[simon04]

In 17132/josm:

see #16995 - WayPoint.LegacyMap silently ignored put(PT_TIME, null)

Fixes org.openstreetmap.josm.data.gpx.GpxDataTest.testMergeFromFiles