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.

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
Show file tree
Hide file tree
Changes from 24 commits
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
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ Change Log
* Fixed removing multiple event listeners within event callbacks. [#5827](https://github.com/AnalyticalGraphicsInc/cesium/issues/5827)
* Running `buildApps` now creates a built version of Sandcastle which uses the built version of Cesium for better performance.
* Fixed a tileset traversal bug when the `skipLevelOfDetail` optimization is off. [#5869](https://github.com/AnalyticalGraphicsInc/cesium/issues/5869)
* Fixed bug with placemarks in imported KML: placemarks with no specified icon would be displayed with default icon. [#5819](https://github.com/AnalyticalGraphicsInc/cesium/issues/5819)

### 1.37 - 2017-09-01

Expand Down
2 changes: 2 additions & 0 deletions CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu
* [Jannes Bolling](https://github.com/jbo023)
* [Logilab](https://www.logilab.fr/)
* [Florent Cayré](https://github.com/fcayre/)
* [Novetta](http://www.novetta.com/)
* [Joshua Bernstein](https://github.com/jbernstein/)

## [Individual CLA](Documentation/Contributors/CLAs/individual-cla-agi-v1.0.txt)
* [Victor Berchet](https://github.com/vicb)
Expand Down
2 changes: 1 addition & 1 deletion Source/DataSources/KmlDataSource.js
Original file line number Diff line number Diff line change
Expand Up @@ -1111,7 +1111,7 @@ define([
entity.billboard = billboard;
}

if (!defined(billboard.image)) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

if (!defined(styleEntity.billboard) && !defined(billboard.image)) {

will only set the default image if an icon style wasn't defined, which I believe is the correct behavior in both cases.

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

Expand Down
15 changes: 15 additions & 0 deletions Specs/Data/KML/simpleEmptyIconStyle.kml
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>
17 changes: 17 additions & 0 deletions Specs/Data/KML/simpleNoIcon.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>
11 changes: 11 additions & 0 deletions Specs/Data/KML/simpleNoStyle.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>
36 changes: 36 additions & 0 deletions Specs/DataSources/KmlDataSourceSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,42 @@ defineSuite([
});
});

it('if load contains <icon> tag with no image included, no image is added', function() {
var dataSource = new KmlDataSource(options);
return loadBlob('Data/KML/simpleNoIcon.kml').then(function(blob) {
return dataSource.load(blob);
}).then(function(source) {
expect(source.entities);
expect(source.entities.values.length).toEqual(1);
expect(source.entities._entities._array.length).toEqual(1);
expect(source.entities._entities._array[0]._billboard._image).toBeUndefined();
});
});

it('if load does not contain icon <style> tag for placemark, default yellow pin does show', function() {
var dataSource = new KmlDataSource(options);
return loadBlob('Data/KML/simpleNoStyle.kml').then(function(blob) {
return dataSource.load(blob);
}).then(function(source) {
expect(source.entities);
expect(source.entities.values.length).toEqual(1);
expect(source.entities._entities._array.length).toEqual(1);
expect(source.entities._entities._array[0]._billboard._image === dataSource._pinBuilder.fromColor(Color.YELLOW, 64));
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use .toEqual here as well.

});
});

it('if load contains empty <IconStyle> tag for placemark, default yellow pin does show', function() {
var dataSource = new KmlDataSource(options);
return loadBlob('Data/KML/simpleEmptyIconStyle.kml').then(function(blob) {
return dataSource.load(blob);
}).then(function(source) {
expect(source.entities);
expect(source.entities.values.length).toEqual(1);
expect(source.entities._entities._array.length).toEqual(1);
expect(source.entities._entities._array[0]._billboard._image === dataSource._pinBuilder.fromColor(Color.YELLOW, 64));
Copy link
Contributor

Choose a reason for hiding this comment

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

Use .toEqual

});
});

it('sets DataSource name from Document', function() {
var kml = '<?xml version="1.0" encoding="UTF-8"?>\
<Document>\
Expand Down