Modify

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#16995 closed defect (fixed)

time string in GPX export is wrong

Reported by: ikke1372@… Owned by: team
Priority: normal Milestone: 18.11
Component: Core Version:
Keywords: template_report gpx time Cc: cmuelle8

Description (last modified by Don-vip)

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.

Attachments (7)

20181113nmo.gpx (258.0 KB ) - added by jul2 5 years ago.
one of the exports (GPX)
josm-ticket-16995-corrupt-timestamps-when-using-ExportRelationToGpxAction-fix.patch (11.8 KB ) - added by cmuelle8 5 years ago.
josm-ticket-16995-corrupt-timestamps-when-using-ExportRelationToGpxAction-fix.patch
josm-ticket-16995-corrupt-timestamps-when-using-ExportRelationToGpxAction-fix__without-y2038-fix.patch (8.7 KB ) - added by cmuelle8 5 years ago.
josm-ticket-16995-corrupt-timestamps-when-using-ExportRelationToGpxAction-fixwithout-y2038-fix.patch
josm_WayPoint.java_to-use_Date_exclusively_for_PT_TIME_attribute.patch (7.9 KB ) - added by cmuelle8 5 years ago.
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/)
josm-findbugs-regressions_in_Marker.java_and_GpxReader.java.patch (2.2 KB ) - added by cmuelle8 5 years ago.
fix minor issues in response to https://josm.openstreetmap.de/jenkins/job/JOSM/4749/findbugsResult/new/
josm-cosmetic-fix-for-GPX-date-strings.patch (2.0 KB ) - added by cmuelle8 5 years ago.
proper fix to timestamp formatting in GPX files, does not append trailing subsecond zeroes
josm_refactor_WayPoint.java_deduplicate-multiple-storage-of-wpt-time.patch (34.3 KB ) - added by cmuelle8 5 years ago.
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)

Download all attachments as: .zip

Change History (50)

by jul2, 5 years ago

Attachment: 20181113nmo.gpx added

one of the exports (GPX)

comment:1 by Don-vip, 5 years ago

Description: modified (diff)

in reply to:  description comment:2 by Don-vip, 5 years ago

Replying to ikke1372@…:

  1. select a route in Relations

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

comment:3 by ikke1372@…, 5 years ago

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

Last edited 5 years ago by Don-vip (previous) (diff)

comment:4 by Don-vip, 5 years ago

Keywords: gpx time added
Milestone: 18.11

comment:5 by Don-vip, 5 years ago

Cc: cmuelle8 added

comment:6 by Don-vip, 5 years ago

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

comment:7 by cmuelle8, 5 years ago

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

Last edited 5 years ago by cmuelle8 (previous) (diff)

by cmuelle8, 5 years ago

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

comment:8 by cmuelle8, 5 years ago

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

by cmuelle8, 5 years ago

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

comment:9 by cmuelle8, 5 years ago

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

comment:10 by cmuelle8, 5 years ago

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

comment:11 by cmuelle8, 5 years ago

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 {} \;
Last edited 5 years ago by cmuelle8 (previous) (diff)

comment:12 by Don-vip, 5 years ago

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.

comment:13 by cmuelle8, 5 years ago

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.

Last edited 5 years ago by cmuelle8 (previous) (diff)

in reply to:  13 comment:14 by Don-vip, 5 years ago

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.

comment:15 by Don-vip, 5 years ago

Resolution: fixed
Status: newclosed

In 14434/josm:

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

comment:16 by Don-vip, 5 years ago

It seems OsmDataLayerTest.testToGpxData must be updated.

comment:17 by cmuelle8, 5 years ago

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

    old new  
    135135     */
    136136    public void setTime(Date time) {
    137137        this.time = time.getTime() / 1000.;
    138         this.attr.put(PT_TIME, DateUtils.fromDate(time));
     138        this.attr.put(PT_TIME, time);
    139139    }
    140140
    141141    /**

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

       
      11// License: GPL. For details, see LICENSE file.
      22package org.openstreetmap.josm.data.gpx;
      33
       4import static org.junit.Assert.assertTrue;
       5
      46import java.awt.Color;
      5 import java.time.DateTimeException;
      67import java.util.ArrayList;
      78import java.util.Date;
      89import java.util.List;
       
      1415import org.openstreetmap.josm.data.osm.search.SearchCompiler.Match;
      1516import org.openstreetmap.josm.data.projection.Projecting;
      1617import org.openstreetmap.josm.tools.Logging;
      17 import org.openstreetmap.josm.tools.UncheckedParseException;
      18 import org.openstreetmap.josm.tools.date.DateUtils;
      1918import org.openstreetmap.josm.tools.template_engine.TemplateEngineDataProvider;
      2019
      2120/**
       
      135134     */
      136135    public void setTime(Date time) {
      137136        this.time = time.getTime() / 1000.;
      138         this.attr.put(PT_TIME, DateUtils.fromDate(time));
       137        this.attr.put(PT_TIME, time);
      139138    }
      140139
      141140    /**
       
      155154     * @since 13210
      156155     */
      157156    public void setTime(long ts) {
      158         this.time = ts;
      159         this.attr.put(PT_TIME, DateUtils.fromTimestamp(ts));
       157        setTimeInMillis(ts*1000);
      160158    }
      161159
      162160    /**
       
      167165     */
      168166    public void setTimeInMillis(long ts) {
      169167        this.time = ts / 1000.;
      170         this.attr.put(PT_TIME, DateUtils.fromTimestampInMillis(ts));
       168        this.attr.put(PT_TIME, new Date(ts));
      171169    }
      172170
      173171    /**
       
      179177        if (attr.containsKey(PT_TIME)) {
      180178            try {
      181179                final Object obj = get(PT_TIME);
      182                 final Date date = obj instanceof Date ? (Date) obj : DateUtils.fromString(obj.toString());
       180                assertTrue(obj instanceof Date);
       181                final Date date = (Date) obj;
      183182                time = date.getTime() / 1000.;
      184183                return date;
      185             } catch (UncheckedParseException | DateTimeException e) {
       184            } catch (AssertionError e) {
      186185                Logging.warn(e);
      187186                time = 0;
      188187            }
  • 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

         
        135135     */
        136136    public void setTime(Date time) {
        137137        this.time = time.getTime() / 1000.;
        138         this.attr.put(PT_TIME, DateUtils.fromDate(time));
         138        this.attr.put(PT_TIME, DateUtils.getGpxFormat().format(time));
        139139    }
        140140
        141141    /**
         
        155155     * @since 13210
        156156     */
        157157    public void setTime(long ts) {
        158         this.time = ts;
        159         this.attr.put(PT_TIME, DateUtils.fromTimestamp(ts));
         158        setTimeInMillis(ts*1000);
        160159    }
        161160
        162161    /**
         
        167166     */
        168167    public void setTimeInMillis(long ts) {
        169168        this.time = ts / 1000.;
        170         this.attr.put(PT_TIME, DateUtils.fromTimestampInMillis(ts));
         169        this.attr.put(PT_TIME, DateUtils.getGpxFormat().format(new Date(ts)));
        171170    }
        172171
        173172    /**
    • 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

         
        220220                "    <tag k='ele' v='456' />\n" +
        221221                "    <tag k='time' v='2018-08-01T10:01:00Z' />\n" +
        222222                "  </node>\n" +
        223                 "  <way id='-546308'>\n" +
         223                "  <node id='-546308' timestamp='2018-08-01T10:02:00Z' lat='47.05' lon='9.05'>\n" +
         224                "    <tag k='ele' v='789' />\n" +
         225                "  </node>\n" +
         226                "  <way id='-546309'>\n" +
        224227                "    <nd ref='-546306' />\n" +
        225228                "    <nd ref='-546307' />\n" +
         229                "    <nd ref='-546308' />\n" +
        226230                "  </way>\r\n" +
        227231                "</osm>").getBytes(StandardCharsets.UTF_8)), null));
        228232        GpxData gpx = layer.toGpxData();
         
        237241        Collection<GpxTrackSegment> segments = track.getSegments();
        238242        assertEquals(1, segments.size());
        239243        Collection<WayPoint> trackpoints = segments.iterator().next().getWayPoints();
        240         assertEquals(2, trackpoints.size());
         244        assertEquals(3, trackpoints.size());
        241245        Iterator<WayPoint> it = trackpoints.iterator();
        242246        DateFormat gpxFormat = DateUtils.getGpxFormat();
        243247        WayPoint p1 = it.next();
        244248        assertEquals(new LatLon(47.0, 9.0), p1.getCoor());
        245249        assertEquals("123", p1.get(GpxConstants.PT_ELE));
        246         assertEquals("2018-08-01T10:00:00.000Z", gpxFormat.format(p1.get(GpxConstants.PT_TIME)));
         250        assertEquals("2018-08-01T10:00:00.000Z", gpxFormat.format(DateUtils.fromString((String) p1.get(GpxConstants.PT_TIME))));
        247251        WayPoint p2 = it.next();
        248252        assertEquals(new LatLon(47.1, 9.1), p2.getCoor());
        249253        assertEquals("456", p2.get(GpxConstants.PT_ELE));
        250         assertEquals("2018-08-01T10:01:00.000Z", gpxFormat.format(p2.get(GpxConstants.PT_TIME)));
         254        assertEquals("2018-08-01T10:01:00.000Z", gpxFormat.format(DateUtils.fromString((String) p2.get(GpxConstants.PT_TIME))));
         255        WayPoint p3 = it.next();
         256        assertEquals(new LatLon(47.05, 9.05), p3.getCoor());
         257        assertEquals("789", p3.get(GpxConstants.PT_ELE));
         258        assertEquals("2018-08-01T10:02:00.000Z", gpxFormat.format(DateUtils.fromString((String) p3.get(GpxConstants.PT_TIME))));
        251259    }
        252260
        253261    /**

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.

Last edited 5 years ago by cmuelle8 (previous) (diff)

comment:18 by cmuelle8, 5 years ago

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

Last edited 5 years ago by cmuelle8 (previous) (diff)

comment:19 by Don-vip, 5 years ago

Resolution: fixed
Status: closedreopened

comment:20 by Don-vip, 5 years ago

Resolution: fixed
Status: reopenedclosed

In 14445/josm:

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

comment:21 by Don-vip, 5 years ago

Thanks a lot for the analysis!

comment:22 by Don-vip, 5 years ago

Resolution: fixed
Status: closedreopened

comment:23 by cmuelle8, 5 years ago

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.

comment:24 by cmuelle8, 5 years ago

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.

comment:25 by cmuelle8, 5 years ago

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

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

     
    2626import org.openstreetmap.josm.data.gpx.ImmutableGpxTrack;
    2727import org.openstreetmap.josm.data.gpx.WayPoint;
    2828import org.openstreetmap.josm.tools.Logging;
     29import org.openstreetmap.josm.tools.UncheckedParseException;
    2930import org.openstreetmap.josm.tools.XmlUtils;
     31import org.openstreetmap.josm.tools.date.DateUtils;
    3032import org.xml.sax.Attributes;
    3133import org.xml.sax.InputSource;
    3234import org.xml.sax.SAXException;
     
    440442                        currentWayPoint.put(localName, 0f);
    441443                    }
    442444                    break;
    443                 case "time":
     445                case GpxConstants.PT_TIME:
     446                    try {
     447                        currentWayPoint.setTime(DateUtils.fromString(accumulator.toString()));
     448                    } catch (UncheckedParseException e) {
     449                        Logging.error(e);
     450                    }
     451                    break;
    444452                case "cmt":
    445453                case "desc":
    446454                    currentWayPoint.put(localName, accumulator.toString());
    447                     currentWayPoint.setTimeFromAttribute();
    448455                    break;
    449456                case "rtept":
    450457                    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.

by cmuelle8, 5 years ago

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

comment:26 by cmuelle8, 5 years ago

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

comment:27 by Don-vip, 5 years ago

In 14446/josm:

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

comment:28 by Don-vip, 5 years ago

Resolution: fixed
Status: reopenedclosed

In 14447/josm:

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

comment:29 by cmuelle8, 5 years ago

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

by cmuelle8, 5 years ago

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

comment:30 by cmuelle8, 5 years ago

Resolution: fixed
Status: closedreopened

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:

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

comment:31 by Don-vip, 5 years ago

In 14448/josm:

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

comment:32 by Don-vip, 5 years ago

Resolution: fixed
Status: reopenedclosed

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

comment:33 by Don-vip, 5 years ago

In 14453/josm:

see #16995 - update unit test

by cmuelle8, 5 years ago

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)

comment:34 by cmuelle8, 5 years ago

Resolution: fixed
Status: closedreopened

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

comment:35 by cmuelle8, 5 years ago

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.

Last edited 5 years ago by cmuelle8 (previous) (diff)

comment:36 by Don-vip, 5 years ago

Resolution: fixed
Status: reopenedclosed

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

comment:37 by Don-vip, 5 years ago

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.

comment:38 by Don-vip, 5 years ago

In 14459/josm:

see #16995 - remove unused imports

comment:39 by Don-vip, 5 years ago

In 14460/josm:

see #16995 - fix spotbugs/pmd/sonar warnings

in reply to:  36 ; comment:40 by GerdP, 5 years ago

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.

in reply to:  40 comment:41 by anonymous, 5 years ago

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

comment:42 by simon04, 3 years ago

In 17130/josm:

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

comment:43 by simon04, 3 years ago

In 17132/josm:

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

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

Modify Ticket

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

Add Comment


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