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

Jump to bottom

KML: Parsing of NetworkLink tag #3171

Merged
merged 2 commits into from
Feb 5, 2015
Merged

KML: Parsing of NetworkLink tag #3171

merged 2 commits into from
Feb 5, 2015

Conversation

oterral
Copy link
Contributor

@oterral oterral commented Jan 26, 2015

This PR add the pasing of NetworkLink tag: https://developers.google.com/kml/documentation/kmlreference#networklink

As in OL2, that made a synchronous http request to load the kml file.

@oterral oterral mentioned this pull request Jan 26, 2015
Closed
@oterral oterral force-pushed the kml_link branch 3 times, most recently from f5ad9dc to 0981970 Compare January 27, 2015 09:58
@oterral
Copy link
Contributor Author

oterral commented Jan 27, 2015

Thx @tsauerwein, I've adapted according to your comments (oterral@90521ca#commitcomment-9454630)

@fredj
Copy link
Member

fredj commented Jan 28, 2015

Is a synchronous request really needed?
They are deprecated in Firefox:
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/Synchronous_and_Asynchronous_Requests#Synchronous_request

@oterral
Copy link
Contributor Author

oterral commented Jan 28, 2015

Damn Firefox !!! Ï didn't know that. May be the other way is to make a readNetworkLinks method (like readName) and let the user handle the links as he wants (using promise or callbacks).

What do you think?

@oterral
Copy link
Contributor Author

oterral commented Jan 29, 2015

@fredj?

@elemoine
Copy link
Member

@oterral That sounds good to me.

@fredj
Copy link
Member

fredj commented Jan 29, 2015

LGTM too

@oterral
Copy link
Contributor Author

oterral commented Feb 3, 2015

@fredj @elemoine could you merge this one ?

@ponceta ponceta mentioned this pull request Feb 3, 2015
Merged
@elemoine
Copy link
Member

elemoine commented Feb 4, 2015

@oterral I still see the synchronous. I thought your plan was to change this and use a callback (or a promise) for that. Did I misunderstand?

@fredj
Copy link
Member

fredj commented Feb 4, 2015

@oterral I still see the synchronous. I thought your plan was to change this and use a callback (or a promise) for that. Did I misunderstand?

I thought the same

@oterral
Copy link
Contributor Author

oterral commented Feb 4, 2015

olalala total misunderstanding sorry. I change that.

@oterral oterral force-pushed the kml_link branch 6 times, most recently from c3a14ef to 137472c Compare February 5, 2015 15:01
@oterral
Copy link
Contributor Author

oterral commented Feb 5, 2015

method readNetworkLinks added

elemoine
elemoine reviewed Feb 5, 2015
View reviewed changes
src/ol/format/kmlformat.js
@@ -1712,6 +1753,71 @@ ol.format.KML.prototype.readNameFromNode = function(node) {


/**
* @param {Document|Node|string} source Souce.
* @return {Array.<Object>} Network links.
* @api stable
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need the stability marker at this point. So please leave @api but remove stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I remove

Copy link
Member

Choose a reason for hiding this comment

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

@api should be sufficient for now

elemoine pushed a commit that referenced this pull request Feb 5, 2015
Merge pull request #3171 from oterral/kml_link
480f352 
KML: Parsing of NetworkLink tag
@elemoine elemoine merged commit 480f352 into openlayers:master Feb 5, 2015
@oterral
Copy link
Contributor Author

oterral commented Feb 5, 2015

Thank you very much guys!!!

@oterral oterral deleted the kml_link branch February 5, 2015 15:47
@bartvde
Copy link
Member

bartvde commented Mar 26, 2015

did tests pass for you in Firefox?
I am getting: expected 'http://localhost:3000/test/bar/bar.kml' to equal 'bar/bar.kml'

@bartvde bartvde mentioned this pull request Apr 1, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oterral @fredj @elemoine @bartvde