[Patch] GPX Tracks not shown

[HGSandhagen@…]

What steps will reproduce the problem?

1. Menu File/Open/Selct a gpx file
2. The gpx is shown in the layer list, but not on the map.

What is the expected result?

Track is shown in the map (worked in older version, see screenshot with version 15479).

What happens instead?

Track is not displayed. Converting into a data layer works (context menu from layerlist). The converted layer is displayed. So it should not be a problem of the gpx file.

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

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2019-12-01 23:10:15 +0100 (Sun, 01 Dec 2019)
Build-Date:2019-12-02 02:30:57
Revision:15553
Relative:URL: ^/trunk

Identification: JOSM/1.5 (15553 de) Windows 10 64-Bit
OS Build number: Windows 10 Pro 1909 (18363)
Memory Usage: 431 MB / 910 MB (221 MB allocated, but free)
Java version: 1.8.0_222-b10, AdoptOpenJDK, OpenJDK 64-Bit Server VM
Screen: \Display0 1920x1080
Maximum Screen Size: 1920x1080
Dataset consistency test: No problems found

Plugins:
+ CustomizePublicTransportStop (35039)
+ RoadSigns (35234)
+ apache-commons (35092)
+ editgpx (35221)
+ ejml (35122)
+ geotools (35169)
+ imagery_offset_db (34908)
+ jaxb (35014)
+ jts (35122)
+ measurement (35221)
+ opendata (35179)
+ turnrestrictions (34977)
+ utilsplugin2 (35238)

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.

[HGSandhagen@…]

Track shown in version 15479

[HGSandhagen@…]

GPX file

[GerdP]

The gpx files sets color BLACK:

<gpxx:DisplayColor>Black</gpxx:DisplayColor>

I guess JOSM should ignore this as black on black is hard to see ;)

[GerdP]

On the other hand, if you have a background image like "OpenStreetMap Carto" the black color is visible.

[Klumbumbus]

Replying to GerdP:

I guess JOSM should ignore this as black on black is hard to see ;)

That would kind of revert the main intention of #16796. I think the behavior of JOSM is correct. If we make an exception for black then we get tickets like "I use <gpxx:DisplayColor>Black</gpxx:DisplayColor> but the track is not displayed black"...

The better solution would be that the users change the color of the track in case it is the same as their background color. (Or does garmin by default set black? Then maybe we could think of a more smart solution?)

[Bjoeni]

We could ask the user what to do if a color used in the GPX file equals the background color, but that color might change due to different layers in the background. So I too think that doesn't really make sense.
Another option might be to draw a border with a complementary color (or rather just black/white, the more distant one) around lines by default, but I don't really like that idea either.

That would kind of revert the main intention of #16796. I think the behavior of JOSM is correct. If we make an exception for black then we get tickets like "I use <gpxx:DisplayColor>Black</gpxx:DisplayColor> but the track is not displayed black"...

Agreed, the behavior has to be somewhat consistent.

[GerdP]

Ideas:

• draw it with an alternating color, e.g. black-white, if color matches background color
• show popup that track is not visible because color matches backgroud color

[Klumbumbus]

Replying to GerdP:

• show popup that track is not visible because color matches backgroud color

That might be the best solution. That type that you don't need to click away, like "No Notes found in this area".

[Bjoeni]

src/org/openstreetmap/josm/gui/io/importexport/GpxImporter.java

@@ -12 +12 @@
 import org.openstreetmap.josm.actions.ExtensionFileFilter;
 import org.openstreetmap.josm.data.gpx.GpxData;
 import org.openstreetmap.josm.gui.MainApplication;
+import org.openstreetmap.josm.gui.Notification;
 import org.openstreetmap.josm.gui.layer.GpxLayer;
+import org.openstreetmap.josm.gui.layer.ImageryLayer;
+import org.openstreetmap.josm.gui.layer.OsmDataLayer;
 import org.openstreetmap.josm.gui.layer.markerlayer.MarkerLayer;
 import org.openstreetmap.josm.gui.progress.ProgressMonitor;
 import org.openstreetmap.josm.gui.util.GuiHelper;
@@ -153 +156 @@
                 gpxLayer.setLinkedMarkerLayer(markerLayer);
             }
         }
+
+        final boolean isSameColor = MainApplication.getLayerManager()
+                .getLayersOfType(ImageryLayer.class)
+                .stream().noneMatch(ImageryLayer::isVisible)
+                && OsmDataLayer.getBackgroundColor().equals(gpxLayer.getColor());
+
         Runnable postLayerTask = () -> {
             if (!parsedProperly) {
                 String msg;
@@ -165 +174 @@
                 }
                 JOptionPane.showMessageDialog(null, msg);
             }
+            if (isSameColor) {
+                new Notification(tr("The imported track \"{0}\" might not be visible because it has the same color as the background." +
+                        "<br>You can change this in the context menu of the imported layer.", gpxLayerName))
+                .setIcon(JOptionPane.WARNING_MESSAGE)
+                .setDuration(Notification.TIME_LONG)
+                .show();
+            }
         };
         return new GpxImporterData(gpxLayer, markerLayer, postLayerTask);
     }

This shows a popup only if

• background color of all tracks in the file equals background color and
• no imagery layer is currently visible

[GerdP]

Wouldn't it be better to show the popup if any of the tracks is not visible?

[Bjoeni]

Then you might have a big file with different colors including black and get a warning for that.
But I guess you're right, it makes more sense:

src/org/openstreetmap/josm/gui/io/importexport/GpxImporter.java

@@ -12 +12 @@
 import org.openstreetmap.josm.actions.ExtensionFileFilter;
 import org.openstreetmap.josm.data.gpx.GpxData;
 import org.openstreetmap.josm.gui.MainApplication;
+import org.openstreetmap.josm.gui.Notification;
 import org.openstreetmap.josm.gui.layer.GpxLayer;
+import org.openstreetmap.josm.gui.layer.ImageryLayer;
+import org.openstreetmap.josm.gui.layer.OsmDataLayer;
 import org.openstreetmap.josm.gui.layer.markerlayer.MarkerLayer;
 import org.openstreetmap.josm.gui.progress.ProgressMonitor;
 import org.openstreetmap.josm.gui.util.GuiHelper;
@@ -153 +156 @@
                 gpxLayer.setLinkedMarkerLayer(markerLayer);
             }
         }
+
+        final boolean isSameColor = MainApplication.getLayerManager()
+                .getLayersOfType(ImageryLayer.class)
+                .stream().noneMatch(ImageryLayer::isVisible)
+                && data.getTracks().stream().anyMatch(t -> OsmDataLayer.getBackgroundColor().equals(t.getColor()));
+
         Runnable postLayerTask = () -> {
             if (!parsedProperly) {
                 String msg;
@@ -165 +174 @@
                 }
                 JOptionPane.showMessageDialog(null, msg);
             }
+            if (isSameColor) {
+                new Notification(tr("The imported track \"{0}\" might not be visible because it has the same color as the background." +
+                        "<br>You can change this in the context menu of the imported layer.", gpxLayerName))
+                .setIcon(JOptionPane.WARNING_MESSAGE)
+                .setDuration(Notification.TIME_LONG)
+                .show();
+            }
         };
         return new GpxImporterData(gpxLayer, markerLayer, postLayerTask);
     }

[GerdP]

I thought about the situation that you drag&drop a bunch of GPX files into JOSM. i think each is stored in a new layer.

[Bjoeni]

Well that was the case anyways, though you get one warning per layer. So multiple popups might open on top of each other, but I guess that's fine since you don't have to confirm them. I was just talking about how to handle multiple tracks per file with different colors.

A bit off-topic (I can open a separate ticket if you want me to):
When using OPs file I noticed that Garmin actually uses gpxx:TrackExtension instead of gpxx:TrackExtensions, so the abbreviations don't work and files are sometimes not written according to the standard.
Not abbreviated (the internal flattened extension "path" is displayed):

Abbreviated:

The patch attached fixes that, together with the note mentioned in this ticket.

[HGSandhagen@…]

I found one thing, which should be noticed. It seems, that the text in the layer list has always the same color a the track. So color white wouldn't be a good idea (see screenshot). I don't know, whether this behaviour is by intension.

[HGSandhagen@…]

Two tracks in white and black.

[Klumbumbus]

Replying to HGSandhagen@…:

the text in the layer list has always the same color a the track.

I think we should keep that as is.

[skyper]

Replying to Klumbumbus:

Replying to HGSandhagen@…:

the text in the layer list has always the same color a the track.

I think we should keep that as is.

How about adjusting the background if we do not have enough contrast.

[Bjoeni]

This behaviour is intended, but wasn't introduced by me. It was probably just noticed by barely anyone before since the color would always default to null.
I'll have a look at it over the weekend. I think adjusting the background color doesn't work too well but maybe adding a dot or something next to the layer name could work.

[stoecker]

Replying to Bjoeni:

This behaviour is intended, but wasn't introduced by me. It was probably just noticed by barely anyone before since the color would always default to null.
I'll have a look at it over the weekend. I think adjusting the background color doesn't work too well but maybe adding a dot or something next to the layer name could work.

I wouldn't say "intended". This code wasn't redesigned for a very long time. There surely is still a lot which can be improved.

[GerdP]

I think the patch looks good, should I commit it?

[stoecker]

Replying to GerdP:

I think the patch looks good, should I commit it?

Sure. Commit permission include the fact that you can use them :-)

[GerdP]

In 15560/josm:

fix #18389: GPX track with color black is invisible (Patch by Bjoeni)
Show popup that track is not visible because color matches backgroud color. This shows a popup only if

• background color of all tracks in the file equals background color and
• no imagery layer is currently visible

"I noticed that Garmin actually uses gpxx:TrackExtension instead of gpxx:TrackExtensions, so the abbreviations don't work and files are sometimes not written according to the standard."

[Bjoeni]

Replying to Bjoeni:

I think adjusting the background color doesn't work too well but maybe adding a dot or something next to the layer name could work.

My suggestion would be the following:

What do you guys think?

[Bjoeni]

Made it working with different styles, responding to visibility/transparency and way more subtle... I'm still not 100% happy but like this I could live with it:




Someone please check how it looks on Windows/Mac.

eye.png and eye-translucent.png should be updated in images/dialogs/layerlist/.
Also, still not sure if pmd checkstyle is working properly, so that might still complain.

[GerdP]

I am not sure what I should expect. I loaded the attached Track_04-DEZ-19 130850.gpx and I get the popup that it might not be visible. The layer list looks like before (Windows 10).

[Bjoeni]

Did you apply 18389-3.patch (below my last comment)?

It's another patch and doesn't really have much to do with the original issue anymore (except that it came up here), probably should've opened a separate ticket.

[GerdP]

Yes, I tried with and without the patch and found no difference.

[GerdP]

Ah, I see. The patch doesn't contain the binaries for eye.png

[Bjoeni]

Replying to GerdP:

Ah, I see. The patch doesn't contain the binaries for eye.png

That shouldn't make a difference though, except that the old eye.png is not exactly symmetrical and looks a bit off in front of the elliptic background.

Did you change it to different colors?

[GerdP]

Yes, did it now and the color of the eye symbol changes. What's the benefit?

[Bjoeni]

That you can read the layer name even when the track has a bright color (see comment:14 and following)

[Don-vip]

What's the status of this ticket?

[GerdP]

Seems to work as desired also on Windows. I didn't try to understand the code in 18389-3.patch.

[Bjoeni]

@HGSandhagen, @Klumbumbus, @skyper and @stoecker, what do you think?

Should we leave it as is, change it as mentioned in comment:24, or does one of you want to give adjusting the background a try?

[Klumbumbus]

I didn't test anything. IMHO the screenshots from comment:24 look good. Do we have other layer types which use colors?

[Bjoeni]

Replying to Klumbumbus:

Do we have other layer types which use colors?

I'm not aware of any at the moment. But it's in the interface and can easily be implemented by other layers / plugins.

[Don-vip]

Replying to Bjoeni:

It's another patch and doesn't really have much to do with the original issue anymore (except that it came up here), probably should've opened a separate ticket.

Can you please open a new ticket? I'm releasing the new version and your last patch hasn't been merged.

[Bjoeni]

see #18509

[Klumbumbus]

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