-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix bug where yellow icon is shown when icon tag is empty #5819 #5821
Changes from 34 commits
1fdc621
b59cba1
13a6606
ab50ab5
135d396
8898faf
515de90
35d690f
983ffa6
b9dfceb
0bf2659
f373ebe
5518ba9
31efd9d
00aefcc
848e6af
b60dc73
90dbfd5
9ee14ae
cd9c061
b5ffdff
6c40ecb
b0a3834
be26845
470810d
395dc5d
5f5cb33
650066a
df09819
f6154ca
cb773a6
fa1e73e
9758800
6b6f55c
4b0b03a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -729,6 +729,12 @@ define([ | |
|
||
var iconNode = queryFirstNode(node, 'Icon', namespaces.kml); | ||
var icon = getIconHref(iconNode, dataSource, sourceUri, uriResolver, false, query); | ||
|
||
// If icon tags are present but blank, we do not want to show an icon | ||
if (defined(iconNode) && !defined(icon)) { | ||
icon = false; | ||
} | ||
|
||
var x = queryNumericValue(iconNode, 'x', namespaces.gx); | ||
var y = queryNumericValue(iconNode, 'y', namespaces.gx); | ||
var w = queryNumericValue(iconNode, 'w', namespaces.gx); | ||
|
@@ -1140,6 +1146,8 @@ define([ | |
|
||
if (!defined(billboard.image)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By doing this, it fixes your case, but breaks the case where there isn't an icon style at all. In that case Google Earth does show a yellow push pin. Changing the if to something like
will only set the default image if an icon style wasn't defined, which I believe is the correct behavior in both cases. |
||
billboard.image = dataSource._pinBuilder.fromColor(Color.YELLOW, 64); | ||
} else if (billboard.image == false) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @josh-bernstein this is why CI failed on eslint. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hpinkos This condition is only true in the desired cases if I use ==, not ===. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or what is the value of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You explicitly set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hpinkos There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the top of the file, require in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hpinkos In what other situation would billboard.image be defined but have a value of false? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. I had to double check throughout the code but it looks like if it is defined it will always be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hpinkos Ok sounds good. Thanks for checking it out! |
||
billboard.image = undefined; | ||
} | ||
|
||
var scale = 1.0; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<kml xmlns="http://www.opengis.net/kml/2.2"> | ||
<Document> | ||
<Placemark> | ||
<Style> | ||
<IconStyle> | ||
</IconStyle> | ||
</Style> | ||
<description><![CDATA[image.png <a href="./image.png">image.png</a><img src="image.png"/>]]></description> | ||
<Point> | ||
<coordinates>1,2,3</coordinates> | ||
</Point> | ||
</Placemark> | ||
</Document> | ||
</kml> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<kml xmlns="http://www.opengis.net/kml/2.2"> | ||
<Document> | ||
<Placemark> | ||
<Style> | ||
<IconStyle> | ||
<Icon> | ||
</Icon> | ||
</IconStyle> | ||
</Style> | ||
<description><![CDATA[image.png <a href="./image.png">image.png</a><img src="image.png"/>]]></description> | ||
<Point> | ||
<coordinates>1,2,3</coordinates> | ||
</Point> | ||
</Placemark> | ||
</Document> | ||
</kml> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<kml xmlns="http://www.opengis.net/kml/2.2"> | ||
<Document> | ||
<Placemark> | ||
<description><![CDATA[image.png <a href="./image.png">image.png</a><img src="image.png"/>]]></description> | ||
<Point> | ||
<coordinates>1,2,3</coordinates> | ||
</Point> | ||
</Placemark> | ||
</Document> | ||
</kml> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reusing the
icon
variable as a temporary with a different type, even though fine in Javascript, isn't best practice. I would just use a separate variable.Then you can simplify everything like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blankIcon would be set in the processBillboardIcon method, while the check
if (!defined(billboard.image) && !blankIcon)
would need to happen in processPositionGraphics method.Therefore, I believe the only ways to incorporate your suggestions would be to make blankIcon a global variable or a property of billboard. Is either of these ways better than the current way?