Add KML Styling support

Replying to rdewit:

What I won't be implementing at the moment: - <BalloonStyle> needs variable substitution from attribute info

Variable substitution can be done using OpenLayers.String.format(). IMO the BalloonStyle could be seen as a popup with dynamic information, so speaking in OpenLayers terms it is no style at all. One way to support BalloonStyle would probably be to add a popup property to the feature, and use the createPopup method of the feature to render the popup with the text parsed by OpenLayers.String.format().

I know that I haven't really documented these yet, but "Needs More work" is, in my mind, indicative of an existing, but insufficient, patch -- usually one which has failed review in the past.

example

Here is a first patch that implements KML styling support. I've added a patched proxy.cgi to make testing more pleasurable (adds support for extra domains and .kmz).

Please have a look and advise where I can improve the code. A few points of interest:

* This code is not integrated with Style.js

* It might be better to have a method that easily retrieves the nodeValue of a node (by name)

* I might have abused this.internalns

* The positioning with <hotspot> doesn't seem to work well

* The attached zip provides an example.

Removed 2 console.infos that slipped in. Btw: current tests pass. Will create new tests once patch is approved.

Roald, nice work! Here is a quick review:

• read() should probably have options instead of depth as second parameter, to conform with other format classes.

• Working with synchronous AJAX requests is dangerous. It tends to freeze the browser if anything goes wrong. But I understand the problem that led you to doing this: our Format classes return an instant result on the read() method, and this requires synchronous request. Maybe we could have an option to the read class that makes it call a callback function with the features as argument instead of returning them? Then the user can at least choose to have the safer asynchronous way.

• I might be missing something, but why is there a reverseFeatures option? Do we gain anything from reversing the order?

• By setting internalns to "*", you will get elements from any namespace. This might not be desired. I do not really have a better idea, except maybe to use an array of allowed internal namespaces with fallback to the next from the array if one does not return anything. But I

• It would not hurt to store styles as OL.Style objects instead of style hashes.

• The fact that you are using the parseProperty function from OL.Format.SLD leads me to the conclusion that this should go in OL.Format.XML (not Util, as you propose in your comment).

• I am not sure if KMZ parsing should be part of proxy.cgi. I think parsing of ZIP files in general would be worth it's own ticket

New version: cleaned up and uses OL.Style. Tests pass in FF2,IE6 and Opera 9.5b1

The latest patch (kml_styling_v2.diff) has the following changes:

• read() has an extra parameter: options (thanks Andreas)
• makes use of OpenLayers.Style (thanks Andreas)
• reverted the misuse of internalns (thanks Andreas)
• code has been cleaned up (thanks LISAsoft)
• removed the proxy.cgi bit (thanks Andreas)

There is something that i'd like to discuss:

• the use of this.parseProperty: as Andreas suggests, maybe we should move that method from Format.SLD to Format.XML. I'd like to use most of that, but get rid of the parseFloat bit in there, since it will break some of my functionality

Please have a look at the new patch and tell me what you think.

Roald:

first, there is something that I overlooked in my previous review: Properties that change during read() (features, styles, fetched) should be initialized in read(), otherwise you will get unexpected results from read() when you use it more than once on the same instance.

Also, I have to apologize for recommending to put depth in the options of read(). In recent discussions on IRC, I was taught when to use options passed to the constructor, and when to the read() method:

• Options that affect the return value of read() should be passed to the read() method, e.g. if you return [features, styles] instead of features.
• All other options should be passed to the constructor.

So in the new patch, the options should go in the constructor, not in the read() parameters. But this is my fault, sorry.

Now for the changes:

• Why do you call style.createStyle? This method is supposed to be called right before rendering a feature, when the rules can be evaluated against the current map scale and feature attributes. If you want to extend a style by additional style hashes, you should do so on style hashes and create the style object when your hash is ready, before adding it to the styles property of your class.

• On IRC, we were talking about using rules for the styles, which you do not do now. You could create a selectStyle that could be passed to Control.SelectFeature quite easily: create a style object, read the "highlight" styles, create a FidFilter rule for every feature (or groups of features with the same style), use the according "highlight" style as symbolizer for the rule, and add the rule to the style object. I'll be glad to help if you are not sure how to do that.

• The cleanup and the reverted internalns thing are ok.

• What exactly are your concerns with parseFloat in Format.SLD.parseProperty? If there is a number, it will be returned as number. Does that hurt? Also, I am not sure that parseParameter does what you need. This is probably too SLD specific to have it in Format.XML, as is most of parseProperty. So it would be helpful to find out what exactly the part is that you really need. Probably only <property>value</property?

• Did you think about making the AJAX requests asynchronous?

My overall impression is that there is still more work needed, sorry. But maybe we can clarify some things on IRC.

Roald, here is a summary of our discussion on IRC yesterday, plus some things that need to be in the patch that we forgot to talk about:

• No more use of OpenLayers.Style, because with the current implementation, there might be performance impact if using a layer style with FidFilters for every feature.

• No parsing of hover styles.

• No options for read(), but also no depth parameter at the level of read().

• Initialize features, styles and fetched in read() -- this is another strong argument for not using read() directly for your recursion.

• reverseFeatures: should be safe to set to true by default. Any objections from others?

• add an option (e.g. fetchExternalUrls) that will set maxDepth to 0 plus disable fetching of external styles if set to false. Set it to false by default.

Newer version: Changes based on summary ahocevar. Tests pass in FF2/3b2,IE6 and Opera 9.5b1

What has changed (kml_styling_v3.diff):

• removed reverseFeatures: is no styling issue and needs more work (and maybe discussion)
• removed OL.Style since there is no need for that at this point in time
• set maxDepth default = 0 (no external fetching)
• moved recursion to readData() instead of read()
• several attributes now initialized in read() instead of class creation

Please review.

kml_styling_v3.1.diff: result from discussion with crschmidt in IRC:

<crschmidt> I see But shouldn't, at the top level, the URL of the document being parsed be used, if a URL exists?

...

<crschmidt> or styleBaseUrl, even and I *think* that to implement that, all you have to do is have options as a second arg to 'read' and then pass that through to parseRead or parseData

Same as v3, but with extra parameter 'options' in read() to allow user to set styleBaseUrl

One more thing: the hotspots seem not to be working properly: the markers are sometimes not in place. If somebody can find out what is going wrong with positioning the markers... Worst case scenario, I take them out of the patch.

See ticket comments for changes

1259-r6161-A0.patch

This patch adds a few changes to Roald's kml_styling_v3.1.diff:

• features, styles and fetched will be cleared on every call of read()
• Styles will only be parsed if extractStyles is set to true
• Styles will no longer extend OpenLayers.Feature.Vector.Style["default"]
• Options from read() moved back to the instance, because they do not change the return type
• Shows style extraction in examples/KMLParser.html

I am not concerned about the hotspots. If it turns out that there are problems, another ticket can be created to track that.

Minor change: on the KML visual example, turn on style parsing.

This looks good to commit. Roald, thanks for the excellent work. Andreas, thanks for the excellent review and cleanup. Please commit when ready.

(In [6179]) Add KML Styling support. Thanks rdewit for this great contribution. r=crschmidt,me (closes #1259)