Ticket #1731 (closed feature: fixed)

Opened 5 years ago

Last modified 5 years ago

Add BBOX strategy.

Reported by: tschaub Owned by: elemoine
Priority: minor Milestone: 2.7 Release
Component: Layer.Vector Version: 2.7 RC1
Keywords: Cc:
State: Complete

Description

Add a bbox strategy that triggers requests for new data when previous data bounds are invalidated.

This was originally part of #1648, but can make it in independently.

Attachments

bbox.patch Download (17.5 KB) - added by tschaub 5 years ago.
add a bbox strategy and teach the http protocol to serialize bbox

Change History

Changed 5 years ago by tschaub

add a bbox strategy and teach the http protocol to serialize bbox

  Changed 5 years ago by tschaub

  • owner changed from crschmidt to elemoine
  • state set to Review

Part of my intention in the original was to have the http protocol be able to serialize bbox in the query string. I've added that capability in, created a test for it, and put together an example that shows the bbox strategy in conjunction with the http protocol (basically mimicking wfs).

Eric, interested to hear any thoughts. Barring objections, I think this is good to go in. (Count that as a "commit" review from me if you agree.)

follow-up: ↓ 4   Changed 5 years ago by crschmidt

tschaub: If you pass a filter with an AND of Spatial and Comparison filter, it doesn't work here, right? is that intentional? I had kind of assumed we would somehow trawl through the filter looking for a spatial filter anded in or something, but maybe that's something for further subclasses to implement (since an HTTP protocol wouldn't actually support other things) -- just makes me wonder if we should inform the user "your filter is being ignored" if it's not supported...

follow-up: ↓ 6   Changed 5 years ago by elemoine

  • state changed from Review to Commit

Tim, this is looking good, except one thing that looks suspicious to me. In HTTP.js you do

bbox: options.filter.value.toArray()

to get the bbox param. Bounds.toArray() returns an array, don't we want a string here? Isn't toBBOX() more appropriate?

Please commit once you have either an explanation or a correction for this.

in reply to: ↑ 2   Changed 5 years ago by tschaub

Replying to crschmidt:

tschaub: If you pass a filter with an AND of Spatial and Comparison filter, it doesn't work here, right? is that intentional? I had kind of assumed we would somehow trawl through the filter looking for a spatial filter anded in or something, but maybe that's something for further subclasses to implement (since an HTTP protocol wouldn't actually support other things) -- just makes me wonder if we should inform the user "your filter is being ignored" if it's not supported...

Yeah, this is intentional. As a convenience, we allow the bbox strategy to be used with the http protocol - because there is a "standard" of sorts for serializing bbox in geo queries (OpenSearch Geo Extension proposal - and very common practice).

In this case, the user is never constructing a filter - the strategy does that for them. If they are using the HTTP protocol and they add a filter to their layer, they should *not* expect things to work - regardless of what strategy they use. If the base protocol evaluated filters, or if we had a filter strategy that tried to filter client side, then this would be different.

The explanation is in the natural docs:

     * Valid options:
     * url - {String} Url for the request.
     * params - {Object} Parameters to get serialized as a query string.
     * headers - {Object} Headers to be set on the request.
     * filter - {<OpenLayers.Filter.BBOX>} If a bbox filter is sent, it will be
     *     serialized according to the OpenSearch Geo extension
     *     (bbox=minx,miny,maxx,maxy).  Note that a BBOX filter as the child
     *     of a logical filter will not be serialized.
     *

Please augment these docs if you think more detail is needed. Please do not add an error/alert. The filter property is *not* a documented property of the vector layer. It does not make sense to me to be throwing up alerts if people are mucking with undocumented properties.

  Changed 5 years ago by crschmidt

Thanks for the feedback, I have no complaints.

in reply to: ↑ 3 ; follow-up: ↓ 7   Changed 5 years ago by tschaub

Replying to elemoine:

Tim, this is looking good, except one thing that looks suspicious to me. In HTTP.js you do {{{ bbox: options.filter.value.toArray() }}} to get the bbox param. Bounds.toArray() returns an array, don't we want a string here? Isn't toBBOX() more appropriate?

Thanks for the review. The params sent to Request.issue get serialized as a query string with OpenLayers.Util.getParameterString. As mentioned in the  request docs arrays are serialized as comma delimited strings. As I say there, this goes against form-encoding - but since it is the convention elsewhere in the lib, I followed it here.

There's no reason this wouldn't be better as a string. And it does future proof the code in case we do something different with getParameterString (or equivalent) in the future. I'll do the string serialization with toBBOX as you suggest.

in reply to: ↑ 6   Changed 5 years ago by tschaub

Replying to tschaub:

Replying to elemoine:

Tim, this is looking good, except one thing that looks suspicious to me. In HTTP.js you do {{{ bbox: options.filter.value.toArray() }}} to get the bbox param. Bounds.toArray() returns an array, don't we want a string here? Isn't toBBOX() more appropriate?

Thanks for the review. The params sent to Request.issue get serialized as a query string with OpenLayers.Util.getParameterString. As mentioned in the  request docs arrays are serialized as comma delimited strings. As I say there, this goes against form-encoding - but since it is the convention elsewhere in the lib, I followed it here. There's no reason this wouldn't be better as a string. And it does future proof the code in case we do something different with getParameterString (or equivalent) in the future. I'll do the string serialization with toBBOX as you suggest.

Ah, I'm sorry. I retract that.

The reason to send an array instead of a string is so the commas aren't encoded with encodeURIComponent. If this is not clear, let me know and I'll give more detail.

  Changed 5 years ago by tschaub

  • state changed from Commit to Pullup

In with r8000!

  Changed 5 years ago by euzuro

  • status changed from new to closed
  • state changed from Pullup to Complete
  • resolution set to fixed

(In [8012]) Batch merge for rc2 of 2.7. 'svn merge -r7967:HEAD from trunk

Note: See TracTickets for help on using tickets.