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

Adds function that inserts missing namespace into KML #5860

Merged
merged 27 commits into from
Oct 18, 2017
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
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 @@ -18,6 +18,7 @@ Change Log
* Fixed a 3D Tiles point cloud bug causing a stray point to appear at the center of the screen on certain hardware. [#5599](https://github.com/AnalyticalGraphicsInc/cesium/issues/5599)
* 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.
* Added function that inserts missing namespace declarations into KML files. [#5860](https://github.com/AnalyticalGraphicsInc/cesium/pull/5860)
Copy link
Contributor

Choose a reason for hiding this comment

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

1.38 Came out already. Needs to be updated for 1.39 which will be released Nov 1.

* Fixed a tileset traversal bug when the `skipLevelOfDetail` optimization is off. [#5869](https://github.com/AnalyticalGraphicsInc/cesium/issues/5869)

### 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/)
* [Natanael Rivera](https://github.com/nrivera-Novetta/)

## [Individual CLA](Documentation/Contributors/CLAs/individual-cla-agi-v1.0.txt)
* [Victor Berchet](https://github.com/vicb)
Expand Down
30 changes: 30 additions & 0 deletions Source/DataSources/KmlDataSource.js
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,33 @@ define([
return deferred.promise;
}

function insertNamespaces(text) {
var namespaceMap = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be cleaner to have the following then build the search strings in the loop.

var namespaceMap = {
  xsi : 'http://www.w3.org/2001/XMLSchema-instance'
};

xsi : 'http://www.w3.org/2001/XMLSchema-instance'
};
var firstPart, lastPart, reg, declaration;

for (var key in namespaceMap) {
if (namespaceMap.hasOwnProperty(key)) {
reg = RegExp('[<| ]' + key + ':');
Copy link
Contributor

Choose a reason for hiding this comment

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

[ ] specifies a character group where it represents any of the characters in it. This one will match <xsi:, xsi: or |xsi:. I think you were trying to use the OR syntax that you need in capture group (represented with ( )). So it can either be [< ] or (<| ). I think just removing the | from your code and going with the character group is the way to go as it doesn't have the overhead of capturing.

declaration = 'xmlns:' + key + '=';
if (reg.test(text) && text.indexOf(declaration) === -1) {
if (!firstPart) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be

if (!defined(firstPart)) {

Copy link
Contributor Author

@njrivera njrivera Oct 18, 2017

Choose a reason for hiding this comment

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

@tfili do you mean this line should actually use 'defined'? or just check if it's defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

defined is a function in Cesium. It does some extra checking to make sure a value is actually undefined or null. This should be used in cases like this.

The magic of javascript's implicit type conversion would cause !firstpart to be true in other cases.

firstPart = text.substr(0, text.indexOf('<kml') + 4);
lastPart = text.substr(firstPart.length);
}
firstPart += ' ' + declaration + '"' + namespaceMap[key] + '"';
}
}
}

if (firstPart) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use defined here as well.

text = firstPart + lastPart;
}

return text;
}

function loadXmlFromZip(reader, entry, uriResolver, deferred) {
entry.getData(new zip.TextWriter(), function(text) {
uriResolver.kml = parser.parseFromString(text, 'application/xml');
Expand Down Expand Up @@ -2322,6 +2349,9 @@ define([
//There's no official way to validate if a parse was successful.
//The following check detects the error on various browsers.

//Insert missing namespaces
text = insertNamespaces(text);

//IE raises an exception
var kml;
var error;
Expand Down
18 changes: 18 additions & 0 deletions Specs/Data/KML/undeclaredNamespaces.kml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?xml version="1.0" encoding="UTF-8"?>
<kml xmlns="http://www.opengis.net/kml/2.2">
<Document xsi:schemaLocation="http://www.opengis.net/kml/2.2 http://schemas.opengis.net/kml/2.2.0/ogckml22.xsd http://www.google.com/kml/ext/2.2 http://code.google.com/apis/kml/schema/kml22gx.xsd">
<Placemark>
<Style>
<IconStyle>
<Icon>
<href>image.png</href>
</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>
8 changes: 8 additions & 0 deletions Specs/DataSources/KmlDataSourceSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,14 @@ defineSuite([
});
});

it('load inserts missing namespace declaration', function() {
var dataSource = new KmlDataSource(options);
return dataSource.load('Data/KML/undeclaredNamespaces.kml').then(function(source) {
expect(source).toBe(dataSource);
expect(source.entities.values.length).toEqual(1);
});
});

it('load rejects nonexistent URL', function() {
return KmlDataSource.load('test.invalid', options).otherwise(function(e) {
expect(e).toBeInstanceOf(RequestErrorEvent);
Expand Down