Modify

Opened 12 months ago

Closed 11 months ago

Last modified 8 months ago

#18389 closed defect (fixed)

[Patch] GPX Tracks not shown

Reported by: HGSandhagen@… Owned by: team
Priority: normal Milestone: 19.12
Component: Core Version:
Keywords: template_report Cc: Bjoeni

Description

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.

Attachments (16)

JOSM-15479.png (34.6 KB) - added by HGSandhagen@… 12 months ago.
Track shown in version 15479
Track_04-DEZ-19 130850.gpx (65.6 KB) - added by HGSandhagen@… 12 months ago.
GPX file
black-popup.png (9.5 KB) - added by Bjoeni 12 months ago.
trackextension-incorrect.png (21.1 KB) - added by Bjoeni 12 months ago.
trackextension-correct.png (20.6 KB) - added by Bjoeni 12 months ago.
18389.patch (6.7 KB) - added by Bjoeni 12 months ago.
JOSM-gpx-different-colors.png (14.8 KB) - added by HGSandhagen@… 12 months ago.
Two tracks in white and black.
layercolor.png (8.1 KB) - added by Bjoeni 12 months ago.
18389-2.patch (3.2 KB) - added by Bjoeni 12 months ago.
18389-3.patch (4.5 KB) - added by Bjoeni 12 months ago.
eye.png (236 bytes) - added by Bjoeni 12 months ago.
eye-translucent.png (221 bytes) - added by Bjoeni 12 months ago.
cde.png (12.6 KB) - added by Bjoeni 12 months ago.
gtk.png (18.2 KB) - added by Bjoeni 12 months ago.
metal.png (15.9 KB) - added by Bjoeni 12 months ago.
nimbus.png (21.3 KB) - added by Bjoeni 12 months ago.

Download all attachments as: .zip

Change History (56)

Changed 12 months ago by HGSandhagen@…

Attachment: JOSM-15479.png added

Track shown in version 15479

Changed 12 months ago by HGSandhagen@…

Attachment: Track_04-DEZ-19 130850.gpx added

GPX file

comment:1 Changed 12 months ago by 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 ;)

comment:2 Changed 12 months ago by GerdP

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

comment:3 in reply to:  1 Changed 12 months ago by 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?)

comment:4 Changed 12 months ago by Klumbumbus

Cc: Bjoeni added

comment:5 Changed 12 months ago by 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.

comment:6 Changed 12 months ago by 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

comment:7 in reply to:  6 Changed 12 months ago by 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".

comment:8 Changed 12 months ago by Bjoeni

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

     
    1212import org.openstreetmap.josm.actions.ExtensionFileFilter;
    1313import org.openstreetmap.josm.data.gpx.GpxData;
    1414import org.openstreetmap.josm.gui.MainApplication;
     15import org.openstreetmap.josm.gui.Notification;
    1516import org.openstreetmap.josm.gui.layer.GpxLayer;
     17import org.openstreetmap.josm.gui.layer.ImageryLayer;
     18import org.openstreetmap.josm.gui.layer.OsmDataLayer;
    1619import org.openstreetmap.josm.gui.layer.markerlayer.MarkerLayer;
    1720import org.openstreetmap.josm.gui.progress.ProgressMonitor;
    1821import org.openstreetmap.josm.gui.util.GuiHelper;
     
    153156                gpxLayer.setLinkedMarkerLayer(markerLayer);
    154157            }
    155158        }
     159
     160        final boolean isSameColor = MainApplication.getLayerManager()
     161                .getLayersOfType(ImageryLayer.class)
     162                .stream().noneMatch(ImageryLayer::isVisible)
     163                && OsmDataLayer.getBackgroundColor().equals(gpxLayer.getColor());
     164
    156165        Runnable postLayerTask = () -> {
    157166            if (!parsedProperly) {
    158167                String msg;
     
    165174                }
    166175                JOptionPane.showMessageDialog(null, msg);
    167176            }
     177            if (isSameColor) {
     178                new Notification(tr("The imported track \"{0}\" might not be visible because it has the same color as the background." +
     179                        "<br>You can change this in the context menu of the imported layer.", gpxLayerName))
     180                .setIcon(JOptionPane.WARNING_MESSAGE)
     181                .setDuration(Notification.TIME_LONG)
     182                .show();
     183            }
    168184        };
    169185        return new GpxImporterData(gpxLayer, markerLayer, postLayerTask);
    170186    }

This shows a popup only if

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


Changed 12 months ago by Bjoeni

Attachment: black-popup.png added

comment:9 Changed 12 months ago by Bjoeni

Summary: GPX Tracks not shown[Patch] GPX Tracks not shown

comment:10 Changed 12 months ago by GerdP

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

comment:11 Changed 12 months ago by 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

     
    1212import org.openstreetmap.josm.actions.ExtensionFileFilter;
    1313import org.openstreetmap.josm.data.gpx.GpxData;
    1414import org.openstreetmap.josm.gui.MainApplication;
     15import org.openstreetmap.josm.gui.Notification;
    1516import org.openstreetmap.josm.gui.layer.GpxLayer;
     17import org.openstreetmap.josm.gui.layer.ImageryLayer;
     18import org.openstreetmap.josm.gui.layer.OsmDataLayer;
    1619import org.openstreetmap.josm.gui.layer.markerlayer.MarkerLayer;
    1720import org.openstreetmap.josm.gui.progress.ProgressMonitor;
    1821import org.openstreetmap.josm.gui.util.GuiHelper;
     
    153156                gpxLayer.setLinkedMarkerLayer(markerLayer);
    154157            }
    155158        }
     159
     160        final boolean isSameColor = MainApplication.getLayerManager()
     161                .getLayersOfType(ImageryLayer.class)
     162                .stream().noneMatch(ImageryLayer::isVisible)
     163                && data.getTracks().stream().anyMatch(t -> OsmDataLayer.getBackgroundColor().equals(t.getColor()));
     164
    156165        Runnable postLayerTask = () -> {
    157166            if (!parsedProperly) {
    158167                String msg;
     
    165174                }
    166175                JOptionPane.showMessageDialog(null, msg);
    167176            }
     177            if (isSameColor) {
     178                new Notification(tr("The imported track \"{0}\" might not be visible because it has the same color as the background." +
     179                        "<br>You can change this in the context menu of the imported layer.", gpxLayerName))
     180                .setIcon(JOptionPane.WARNING_MESSAGE)
     181                .setDuration(Notification.TIME_LONG)
     182                .show();
     183            }
    168184        };
    169185        return new GpxImporterData(gpxLayer, markerLayer, postLayerTask);
    170186    }

comment:12 Changed 12 months ago by 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.

comment:13 Changed 12 months ago by 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.

Changed 12 months ago by Bjoeni

Changed 12 months ago by Bjoeni

Attachment: trackextension-correct.png added

Changed 12 months ago by Bjoeni

Attachment: 18389.patch added

comment:14 Changed 12 months ago by 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.

Changed 12 months ago by HGSandhagen@…

Two tracks in white and black.

comment:15 in reply to:  14 ; Changed 12 months ago by 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.

comment:16 in reply to:  15 Changed 12 months ago by 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.

comment:17 Changed 12 months ago by 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.

comment:18 in reply to:  17 Changed 12 months ago by 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.

comment:19 Changed 12 months ago by GerdP

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

comment:20 in reply to:  19 Changed 12 months ago by 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 :-)

comment:21 Changed 12 months ago by GerdP

Resolution: fixed
Status: newclosed

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

comment:22 Changed 12 months ago by Don-vip

Milestone: 19.12

comment:23 in reply to:  17 Changed 12 months ago by Bjoeni

Resolution: fixed
Status: closedreopened

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?

Changed 12 months ago by Bjoeni

Attachment: layercolor.png added

Changed 12 months ago by Bjoeni

Attachment: 18389-2.patch added

comment:24 Changed 12 months ago by 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.

Changed 12 months ago by Bjoeni

Attachment: 18389-3.patch added

Changed 12 months ago by Bjoeni

Attachment: eye.png added

Changed 12 months ago by Bjoeni

Attachment: eye-translucent.png added

Changed 12 months ago by Bjoeni

Attachment: cde.png added

Changed 12 months ago by Bjoeni

Attachment: gtk.png added

Changed 12 months ago by Bjoeni

Attachment: metal.png added

Changed 12 months ago by Bjoeni

Attachment: nimbus.png added

comment:25 Changed 12 months ago by 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).

comment:26 Changed 12 months ago by 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.

comment:27 Changed 12 months ago by GerdP

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

comment:28 Changed 12 months ago by GerdP

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

comment:29 in reply to:  28 Changed 12 months ago by 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?

comment:30 Changed 12 months ago by GerdP

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

comment:31 in reply to:  14 Changed 12 months ago by Bjoeni

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

comment:32 Changed 11 months ago by Don-vip

What's the status of this ticket?

comment:33 Changed 11 months ago by GerdP

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

comment:34 Changed 11 months ago by 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?

comment:35 Changed 11 months ago by Klumbumbus

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

comment:36 in reply to:  35 Changed 11 months ago by 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.

comment:37 in reply to:  26 Changed 11 months ago by 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.

comment:38 Changed 11 months ago by Don-vip

Resolution: fixed
Status: reopenedclosed

comment:39 Changed 11 months ago by Bjoeni

see #18509

comment:40 Changed 8 months ago by Klumbumbus

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

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.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.