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

Remove Duplicate Namespace Declarations #5972

Merged
merged 18 commits into from
Nov 30, 2017
Merged

Conversation

JMarzano-Novetta
Copy link
Contributor

@JMarzano-Novetta JMarzano-Novetta commented Nov 9, 2017

Description

  • Some of our users have reported issues opening KML's that are generated with duplicate namespace declarations.
  • This PR checks for a duplicate namespace declaration and deletes it.
  • Duplicate namespaces can include xsi, gmd, etc.
  • The PR adds a function that searches for namespace declarations starting with "xmlns:". It then checks for all duplicate namespace declarations until the end of the block ">". Any duplicate declarations are removed by setting the KML text to the slices before and after that namespace.
  • Note: I work for Novetta, and have written this code for them.

@cesium-concierge
Copy link

Please sign the CLA before we review this PR.

Welcome to the Cesium community @JeremyMarzano-ISPA!

Can you please send in a Contributor License Agreement (CLA) so that we can review and merge this pull request?


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@pjcozzi pjcozzi requested a review from tfili November 20, 2017 21:26
@ggetz
Copy link
Contributor

ggetz commented Nov 20, 2017

Hi @JeremyMarzano-ISPA, thanks for the PR. @tfili will give this a review shortly!

It looks like you have a corporate CLA, but could you please update CONTRIBUTORS.md with the proper information? Thanks!

@tfili
Copy link
Contributor

tfili commented Nov 20, 2017

@JeremyMarzano-ISPA Thanks for your PR.

This will only detect duplicates if it is defined twice, but not 3 times. It will find occurrence 1, then search for occurrence 2 and remove it. Then it will start its search after occurrence 1, so when it find occurrence 3 it doesn't realize it already exists.

@JMarzano-Novetta
Copy link
Contributor Author

Thank you @ggetz. I have updated CONTRIBUTORS.md with my information.

@tfili Thank you for the review. I have updated the function to continue to search for duplicates of the same namespace, until no duplicates are found. I have also updated the test case to include this. Please let me know if you find any other issues, or have any additional comments. Thank you.

@hpinkos
Copy link
Contributor

hpinkos commented Nov 28, 2017

@JeremyMarzano-ISPA can you merge in master when you have a chance?

@JMarzano-Novetta
Copy link
Contributor Author

@hpinkos Thank you for letting me know. I have merged with master.

@@ -2369,6 +2391,9 @@ define([
//Insert missing namespaces
text = insertNamespaces(text);

//Remove Duplicate Namespaces
text = removeDuplicateNamespaces(text);
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't get called for kmz files. Look for all the occurrences of insertNamespaces(...). This should be called in the same places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for finding that. I have updated the PR with a call for KMZ's as well. Please let me know if you find any other issues. Thank you.

endIndex = text.indexOf('\"', (text.indexOf('\"', index) + 1));
text = text.slice(0, index -1) + text.slice(endIndex + 1, text.length);
index = text.indexOf('xmlns:', startIndex - 1);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Else should be on same line as the previous curly brace.

@tfili
Copy link
Contributor

tfili commented Nov 30, 2017

Thanks @JeremyMarzano-ISPA.

@tfili tfili merged commit 8fedf99 into CesiumGS:master Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants