Skip to content
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.

Sign up for GitHub

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

Merged
merged 35 commits into from
Oct 23, 2017
Merged

Fix bug where yellow icon is shown when icon tag is empty #5819 #5821

Changes from 1 commit
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
1fdc621
Fix bug where yellow icon is shown when icon tag is empty #5819
Sep 7, 2017
b59cba1
Revert "Fix bug where yellow icon is shown when icon tag is empty #5819"
Sep 7, 2017
13a6606
Fix bug with placemarks in imported KML, where placemarks with no spe…
Sep 7, 2017
ab50ab5
Add unit test and supporting KML file #5819
Sep 14, 2017
135d396
Revert "Add unit test and supporting KML file #5819"
Sep 14, 2017
8898faf
Add KML file to be used in test #5819
Sep 14, 2017
515de90
Add unit test for icon tag with no image #5819
Sep 14, 2017
35d690f
Merge pull request #1 from AnalyticalGraphicsInc/master
Sep 14, 2017
983ffa6
Resolve merge conflict #5819
Sep 14, 2017
b9dfceb
Merge branch 'master' into 5819_fix_for_empty_icon_tag
Sep 28, 2017
0bf2659
Add my name to contributors file #5819
Sep 29, 2017
f373ebe
Merge branch 'master' into 5819_fix_for_empty_icon_tag
Sep 29, 2017
5518ba9
Add test KML file with no style #5819
Sep 29, 2017
31efd9d
Include company name in CONTRIBUTORS.md #5819
Sep 29, 2017
00aefcc
Merge branch '5819_fix_for_empty_icon_tag' of github.com:josh-bernste…
Sep 29, 2017
848e6af
Fix URL for company #5819
Sep 29, 2017
b60dc73
Fix case with no style tag, and add test #5819
Sep 29, 2017
90dbfd5
Add additional test #5819
Sep 29, 2017
9ee14ae
Use === instead of == #5819
Sep 29, 2017
cd9c061
Remove extra space #5819
Sep 29, 2017
b5ffdff
Merge branch 'master' into 5819_fix_for_empty_icon_tag
Oct 2, 2017
6c40ecb
Add tag number to change in CHANGES.md #5819
Oct 2, 2017
b0a3834
Pull and resolve merge conflict #5819
Oct 2, 2017
be26845
Merge branch 'master' into 5819_fix_for_empty_icon_tag
Oct 2, 2017
470810d
Cover edge case where IconStyle tag is empty #5819
Oct 17, 2017
395dc5d
Fix unit tests to use toEqual #5819
Oct 17, 2017
5f5cb33
Update Changes.md to include change in next release #5819
Oct 17, 2017
650066a
Merge branch 'master' into 5819_fix_for_empty_icon_tag
Oct 17, 2017
df09819
Revert "Cover edge case where IconStyle tag is empty #5819"
Oct 18, 2017
f6154ca
Merge branch 'master' into 5819_fix_for_empty_icon_tag
Oct 20, 2017
cb773a6
Simplify code and cover all cases correctly #5819
Oct 20, 2017
fa1e73e
Merge branch 'master' into 5819_fix_for_empty_icon_tag
Oct 23, 2017
9758800
Merge remote-tracking branch 'upstream/master'
Oct 23, 2017
6b6f55c
Merge branch 'master' into 5819_fix_for_empty_icon_tag
Oct 23, 2017
4b0b03a
Avoid using == #5819
Oct 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion Source/DataSources/KmlDataSource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Contributor

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

var blankIcon = defined(iconNode) && !defined(icon);

...

if (!defined(billboard.image) && !blankIcon) {
              billboard.image = dataSource._pinBuilder.fromColor(Color.YELLOW, 64);
}

Copy link
Contributor Author

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?

icon = false;
}

var x = queryNumericValue(iconNode, 'x', namespaces.gx);
var y = queryNumericValue(iconNode, 'y', namespaces.gx);
var w = queryNumericValue(iconNode, 'w', namespaces.gx);
Expand Down Expand Up @@ -1138,8 +1144,10 @@ define([
entity.billboard = billboard;
}

if (!defined(styleEntity.billboard) && !defined(billboard.image)) {
if (!defined(billboard.image)) {
billboard.image = dataSource._pinBuilder.fromColor(Color.YELLOW, 64);
} else if (billboard.image == false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josh-bernstein this is why CI failed on eslint. == should be ===

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ===.
I could remove this 'else if' block altogether and it will still function correctly. I just think it is more intuitive to set the value back from false to undefined

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do } else if (!billboard.image) { instead

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or what is the value of billboard.image here? Is it 'false' instead of false?
In that case, do } else if (billboard.image === 'false') {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You explicitly set billboard.image to false. === should work in this case because the type and value will match.

Copy link
Contributor Author

@josh-bernstein josh-bernstein Oct 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hpinkos billboard.image instanceof ConstantProperty throws error saying ConstantProperty is undefined. However, everything seems to work if I do } else if (billboard.image && !billboard.image.getValue()) {

Copy link
Contributor

@hpinkos hpinkos Oct 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the top of the file, require in ConstantProperty. See how we require in CompositePositionProperty in this file for an example. We want to make sure it's a constant property because other property types require an argument passed into getValue

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 ConstantProperty. So we can safely just do } else if (!billboard.image.getValue()) {. I was just being cautious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
Expand Down