Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

ol.Collection versus array #274

Merged
merged 4 commits into from Mar 6, 2013

Conversation

Projects
None yet
5 participants
Owner

twpayne commented Mar 6, 2013

So why are layers specified as an ol.Collection and renderers as an array?

Won't this cause confusion for users?

Contributor

twpayne commented Mar 5, 2013

The layers are an ol.Collection so that events are generated as layers are added and removed.

Renderers is just used as a one-time configuration, no events are required.

Owner

bartvde commented Mar 5, 2013

Thanks for the explanation, I get the point now, but couldn't the constructor just take an array and create a collection from that instead? People will always go through getLayers() to add or remove layers or not?

Contributor

twpayne commented Mar 5, 2013

This could lead to some surprising results, for example:

var layers = [layer1, layer2];
var map1 = new ol.Map({
  layers: layers
});
var map2 = new ol.Map({
  layers: layers
});

In this case, the layers will not be synchronized between the maps as each map will create its own ol.Collection.

I think it's clearer to make it explicit by using ol.Collection.

Owner

bartvde commented Mar 5, 2013

Okay, but this is not the most common case.

Personally I feel we should not pass on this complexity to the user.

If people want your use case, they need to create the collection themselves.

In all other cases I think we could take an array to make it simpler.

Contributor

probins commented Mar 5, 2013

this is linked to the discussion on convenience functions for common use cases https://groups.google.com/forum/#!topic/ol3-dev/8Rp8YEEutgM

Owner

tschaub commented Mar 6, 2013

var layers = [layer1, layer2];
var map1 = new ol.Map({
  layers: layers
});
var map2 = new ol.Map({
  layers: layers
});

So far, nothing surprising.

map2.addLayer(layer3)

I'd be surprised if map1 had three layers at this point.

My opinions:

Everybody will understand if they have to type extra characters to get two (or more) synchronized maps. Everybody will complain if they have to type extra characters to get one map. Most people use one map.

Owner

elemoine commented Mar 6, 2013

I think I agree.

Adding addLayer and removeLayer methods to ol.Map makes sense to me. I see these as convenience methods to add and remove layers to and from the layers collection.

Contributor

twpayne commented Mar 6, 2013

This PR is now ready for review.

Owner

elemoine commented Mar 6, 2013

Feels so good to see issues and discussions resulting into code and commits! Please merge.

twpayne added a commit that referenced this pull request Mar 6, 2013

@twpayne twpayne merged commit 65e003d into openlayers:master Mar 6, 2013

1 check passed

default The Travis build passed
Details

@twpayne twpayne deleted the twpayne:map-layers-api branch Mar 6, 2013

Owner

bartvde commented Mar 6, 2013

agreed, great to see action taken on the api issues already!

Sent from my iPhone

On Mar 6, 2013, at 5:33 PM, Éric Lemoine notifications@github.com wrote:

Feels so good to see issues and discussions resulting into code and commits! Please merge.


Reply to this email directly or view it on GitHub.

This was referenced Mar 11, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment