WMSGetFeatureInfo control should use isEquivalentUrl

[tschaub]

Instead of comparing for strict equality, OpenLayers.Util.isEquivalentUrl should be used in findLayers. Also, if since the layers option is optional, url should be as well (first layer url would be assumed).

[tschaub]

And, if findLayers returns no layers, the request should not be issued.

[tschaub]

And, the request parameter value should be "GetFeatureInfo" instead of "getfeatureinfo".

From the WMS spec (WMS 1.1.1 section 6.4.1):

Parameter names shall not be case sensitive, but parameter values shall be case sensitive.

[bartvde]

What I was wondering: should we not couple the text/xml mimetype as well to WMSGetFeatureInfo format by default? I override the OGC mimetypes to text/xml in the proxy for IE support, I thought that was common practice?

this.formats = OpenLayers.Util.extend({
  'application/vnd.ogc.gml': new OpenLayers.Format.WMSGetFeatureInfo()
  }, options.formats);

Do I get it correct that the current WMSGetFeatureInfo Control has the limitation that it cannot work over multiple layers which come from a different WMS?

[tschaub]

Replying to bartvde:

Do I get it correct that the current WMSGetFeatureInfo Control has the limitation that it cannot work over multiple layers which come from a different WMS?

Bart, are you saying that you would like one request to go out to multiple servers? Not sure I follow.

[tschaub]

And, since I think it would be a pity for this change to have to wait for another release, I'm going to see what happens if I set this to 2.8. I know I just voted for a RC. Feel free to slap this back - and I'll just hope there is some other regression.

[bartvde]

Replying to tschaub:

Replying to bartvde:

Do I get it correct that the current WMSGetFeatureInfo Control has the limitation that it cannot work over multiple layers which come from a different WMS?

Bart, are you saying that you would like one request to go out to multiple servers? Not sure I follow.

Hi Tim, no I would like 1 control to go through all WMS layers in map. The WMS layers can come from different urls ofcourse. So the control will add requests to a queue and combine the result.

[tschaub]

Replying to bartvde:

Replying to tschaub:

Replying to bartvde:

Do I get it correct that the current WMSGetFeatureInfo Control has the limitation that it cannot work over multiple layers which come from a different WMS?

Bart, are you saying that you would like one request to go out to multiple servers? Not sure I follow.

Hi Tim, no I would like 1 control to go through all WMS layers in map. The WMS layers can come from different urls ofcourse. So the control will add requests to a queue and combine the result.

Ok, understood. Since dispatching an arbitrary number of asynchronous requests, waiting for them to all return, and concatenating the results (or somehow bundling them) for the callback is a bit beyond what I am willing to take on today, I've opted for some simpler changes. I think the changes I'm proposing would still allow you to make the multiple server enhancement you're talking about in the future.

[tschaub]

I've added another patch with more tests. If anyone is awake, I'd really appreciate a review on this. Though the flaws are minor, I think it would be a pity to have this go out without a bit more work.

Here's what the attached patch does:

WMSGetFeatureInfo.js

• Correct some docs that were copied with erroneous or inappropriate verbiage from elsewhere.

• Add the layerUrls property. This is an optional array of urls to look through for matches. If a layer url matches either the control url or one of the layerUrls, then it can be queried. The justification for this is cases where people have a layer on their map that requests cached tiles. If the layer has the same name as a WMS layer, a GetFeatureInfo request can be created for that layer (and the request should go to the control url, not the layer url).

• Removed the url argument from the control constructor. Without a url provided, the control assumes the url of the first eligible layer. Where a map is created with layers dynamically (from a capabilities doc for example), it simplifies the control configuration to not have to provide a url.

• Modified the findLayers function to consider candidates either from the control's layers array or from the map layers array. In either case, the queryVisible option on the control is respected. Also, since layers that inherit from HTTPRequest can have a url array, the method checks all. In addition, instead of checking for strict equality in url, the method now calls urlMatches.

• Added a urlMatches method that determines if a given url matches the control's url or one of the control's layerUrls. A match is determined based on url equivalence, not strict equality.

• The request only issues the request if findLayers returns one or more layers. (This change is smaller than it looks in the trac preview.) Also, instead of extending a new object with everything from the control, the callback is called with the pixel position as its first argument (the scope of the GET request is now this).

• The handleResponse method takes the pixel position as its first argument. This saves us from extending a new object with all properties of the control with every request.

WMSGetFeatureInfo.html

• Updated tests to reflect the new constructor signature. The url is now optional.

• Added one test in function test_Control_WMSGetFeatureInfo_MixedParams to demonstrate that a control without a url will assign itself one based on the first eligible candidate layer url.

• Added tests for new urlMatches method.

• Added tests for new layerUrls property.

getfeatureinfo-control.html

• Updated example to reflect new constructor signature.

• Put the word GetFeatureInfo in the part of the example that is searchable.

• Gave some labels 'for' attributes.

[tschaub]

Tests pass in FF3, IE6, and IE8. Bart, if you (or anybody else) are available for reviewing this, I'd be very grateful.

[bartvde]

Hey Tim, this looks good to me. Please commit.

[tschaub]

Bart!!!! Your my savior. I'm interested to understand your comment about overriding mimetype. Seems like irc might be an easier place for it.

Thank you for reviewing this.

[tschaub]

s/Your/You're/

[tschaub]

A couple other things:

• Response parsing used to have the format read the request responseXml property. This should always be undefined. The responseXML property may contain a document. Also, as we do elsewhere, we need to check the documentElement before sending to format.read. IE will often have a truthy responseXML with no documentElement (see r6573 and others).

• Since there is only one infoFormat property value (all requests go out with the same info_format parameter value), I don't see how it makes sense to allow for multiple formats for parsing the responses based on different content types. Bart and I talked about this on IRC (based on his comment above). With the current formats object, parsing will fail (or not happen) if the content type header is improperly set (or intentionally changed). I think we want to be more forgiving than that. Having a single format property lets people configure the control to use a different format if the WMSGetFeatureInfo format is not what they want - but I think it sets us up for too many failures if we limit parsing based on the content type header.

Bart voiced his support for this change and I think it makes sense as well. When we get to the point of having multiple requests issued (to different servers) by a single control, we might want to reconsider the single format - but we'd need to change the infoFormat property as well. Also, the point of the WMSGetFeatureInfo format was to handle multiple response types.

[tschaub]

clean up WMSGetFeatureInfo

[tschaub]

in w/ r9300 r=bartvde