Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

ol.Collection versus array #274

Merged
merged 4 commits into from

5 participants

@bartvde
Owner

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

Won't this cause confusion for users?

@twpayne

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.

@bartvde
Owner

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?

@twpayne

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.

@bartvde
Owner

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.

@probins

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

@tschaub
Owner
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.

@elemoine
Owner

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.

@twpayne

This PR is now ready for review.

@elemoine
Owner

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

@twpayne twpayne merged commit 65e003d into from
@twpayne twpayne deleted the branch
@bartvde
Owner
This was referenced
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
3  examples/anchored-elements.js
@@ -1,5 +1,4 @@
goog.require('ol.AnchoredElement');
-goog.require('ol.Collection');
goog.require('ol.Coordinate');
goog.require('ol.Map');
goog.require('ol.RendererHints');
@@ -14,7 +13,7 @@ var layer = new ol.layer.TileLayer({
});
var map = new ol.Map({
- layers: new ol.Collection([layer]),
+ layers: [layer],
renderers: ol.RendererHints.createFromQueryData(),
target: 'map',
view: new ol.View2D({
View
5 examples/canvas-tiles.js
@@ -1,4 +1,3 @@
-goog.require('ol.Collection');
goog.require('ol.Coordinate');
goog.require('ol.Map');
goog.require('ol.RendererHint');
@@ -10,7 +9,7 @@ goog.require('ol.source.OpenStreetMap');
goog.require('ol.tilegrid.XYZ');
-var layers = new ol.Collection([
+var layers = [
new ol.layer.TileLayer({
source: new ol.source.OpenStreetMap()
}),
@@ -22,7 +21,7 @@ var layers = new ol.Collection([
})
})
})
-]);
+];
var webglMap = new ol.Map({
view: new ol.View2D({
View
5 examples/epsg-4326.js
@@ -1,4 +1,3 @@
-goog.require('ol.Collection');
goog.require('ol.Coordinate');
goog.require('ol.Map');
goog.require('ol.RendererHint');
@@ -10,7 +9,7 @@ goog.require('ol.projection');
goog.require('ol.source.TiledWMS');
-var layers = new ol.Collection([
+var layers = [
new ol.layer.TileLayer({
source: new ol.source.TiledWMS({
url: 'http://vmap0.tiles.osgeo.org/wms/vmap0',
@@ -22,7 +21,7 @@ var layers = new ol.Collection([
}
})
})
-]);
+];
var map = new ol.Map({
controls: ol.control.defaults({
View
3  examples/full-screen.js
@@ -1,4 +1,3 @@
-goog.require('ol.Collection');
goog.require('ol.Coordinate');
goog.require('ol.Map');
goog.require('ol.RendererHints');
@@ -15,7 +14,7 @@ var map = new ol.Map({
controls: ol.control.defaults({
scaleLine: true
}),
- layers: new ol.Collection([layer]),
+ layers: [layer],
renderers: ol.RendererHints.createFromQueryData(),
target: 'map',
view: new ol.View2D({
View
5 examples/stamen.js
@@ -1,4 +1,3 @@
-goog.require('ol.Collection');
goog.require('ol.Coordinate');
goog.require('ol.Map');
goog.require('ol.RendererHints');
@@ -7,7 +6,7 @@ goog.require('ol.layer.TileLayer');
goog.require('ol.source.Stamen');
-var layers = new ol.Collection([
+var layers = [
new ol.layer.TileLayer({
source: new ol.source.Stamen({
layer: 'watercolor'
@@ -18,7 +17,7 @@ var layers = new ol.Collection([
layer: 'terrain-labels'
})
})
-]);
+];
var map = new ol.Map({
layers: layers,
renderers: ol.RendererHints.createFromQueryData(),
View
5 examples/two-layers.js
@@ -1,4 +1,3 @@
-goog.require('ol.Collection');
goog.require('ol.Coordinate');
goog.require('ol.Map');
goog.require('ol.RendererHint');
@@ -9,7 +8,7 @@ goog.require('ol.source.BingMaps');
goog.require('ol.source.TileJSON');
-var layers = new ol.Collection([
+var layers = [
new ol.layer.TileLayer({
source: new ol.source.BingMaps({
key: 'AgtFlPYDnymLEe9zJ5PCkghbNiFZE9aAtTy3mPaEnEBXqLHtFuTcKoZ-miMC3w7R',
@@ -21,7 +20,7 @@ var layers = new ol.Collection([
uri: 'http://api.tiles.mapbox.com/v3/mapbox.va-quake-aug.jsonp'
})
})
-]);
+];
var webglMap = new ol.Map({
layers: layers,
View
5 examples/wms-custom-proj.js
@@ -1,5 +1,4 @@
goog.require('ol.Attribution');
-goog.require('ol.Collection');
goog.require('ol.Coordinate');
goog.require('ol.Extent');
goog.require('ol.Map');
@@ -20,7 +19,7 @@ var epsg21781 = new ol.Projection('EPSG:21781', ol.ProjectionUnits.METERS,
ol.projection.addProjection(epsg21781);
var extent = new ol.Extent(420000, 30000, 900000, 350000);
-var layers = new ol.Collection([
+var layers = [
new ol.layer.TileLayer({
source: new ol.source.TiledWMS({
url: 'http://wms.geo.admin.ch/',
@@ -45,7 +44,7 @@ var layers = new ol.Collection([
params: {'LAYERS': 'ch.bafu.schutzgebiete-paerke_nationaler_bedeutung'}
})
})
-]);
+];
var map = new ol.Map({
layers: layers,
View
5 examples/wms.js
@@ -1,4 +1,3 @@
-goog.require('ol.Collection');
goog.require('ol.Coordinate');
goog.require('ol.Extent');
goog.require('ol.Map');
@@ -9,7 +8,7 @@ goog.require('ol.source.MapQuestOpenAerial');
goog.require('ol.source.TiledWMS');
-var layers = new ol.Collection([
+var layers = [
new ol.layer.TileLayer({
source: new ol.source.MapQuestOpenAerial()
}),
@@ -21,7 +20,7 @@ var layers = new ol.Collection([
extent: new ol.Extent(-13884991, 2870341, -7455066, 6338219)
})
})
-]);
+];
var map = new ol.Map({
renderer: ol.RendererHint.CANVAS,
layers: layers,
View
2  src/objectliterals.exports
@@ -1,7 +1,7 @@
@exportObjectLiteral ol.MapOptions
@exportObjectLiteralProperty ol.MapOptions.controls Array.<ol.control.Control>|undefined
@exportObjectLiteralProperty ol.MapOptions.interactions ol.Collection|undefined
-@exportObjectLiteralProperty ol.MapOptions.layers ol.Collection|undefined
+@exportObjectLiteralProperty ol.MapOptions.layers Array.<ol.layer.Layer>|ol.Collection|undefined
@exportObjectLiteralProperty ol.MapOptions.renderer ol.RendererHint|undefined
@exportObjectLiteralProperty ol.MapOptions.renderers Array.<ol.RendererHint>|undefined
@exportObjectLiteralProperty ol.MapOptions.target Element|string
View
2  src/ol/map.exports
@@ -1,8 +1,10 @@
@exportClass ol.Map ol.MapOptions
+@exportProperty ol.Map.prototype.addLayer
@exportProperty ol.Map.prototype.addPreRenderFunction
@exportProperty ol.Map.prototype.addPreRenderFunctions
@exportProperty ol.Map.prototype.getInteractions
@exportProperty ol.Map.prototype.getRenderer
+@exportProperty ol.Map.prototype.removeLayer
@exportSymbol ol.RendererHint
@exportProperty ol.RendererHint.CANVAS
View
36 src/ol/map.js
@@ -293,6 +293,16 @@ goog.inherits(ol.Map, ol.Object);
/**
+ * @param {ol.layer.Layer} layer Layer.
+ */
+ol.Map.prototype.addLayer = function(layer) {
+ var layers = this.getLayers();
+ goog.asserts.assert(goog.isDef(layers));
+ layers.push(layer);
+};
+
+
+/**
* @param {ol.PreRenderFunction} preRenderFunction Pre-render function.
*/
ol.Map.prototype.addPreRenderFunction = function(preRenderFunction) {
@@ -620,6 +630,18 @@ ol.Map.prototype.requestRenderFrame = function() {
/**
+ * @param {ol.layer.Layer} layer Layer.
+ * @return {ol.layer.Layer|undefined} The removed layer or undefined if the
+ * layer was not found.
+ */
+ol.Map.prototype.removeLayer = function(layer) {
+ var layers = this.getLayers();
+ goog.asserts.assert(goog.isDef(layers));
+ return /** @type {ol.layer.Layer|undefined} */ (layers.remove(layer));
+};
+
+
+/**
* @param {number} time Time.
* @private
*/
@@ -823,8 +845,18 @@ ol.Map.createOptionsInternal = function(mapOptions) {
*/
var values = {};
- values[ol.MapProperty.LAYERS] = goog.isDef(mapOptions.layers) ?
- mapOptions.layers : new ol.Collection();
+ var layers;
+ if (goog.isDef(mapOptions.layers)) {
+ if (goog.isArray(mapOptions.layers)) {
+ layers = new ol.Collection(goog.array.clone(mapOptions.layers));
+ } else {
+ goog.asserts.assert(mapOptions.layers instanceof ol.Collection);
+ layers = mapOptions.layers;
+ }
+ } else {
+ layers = new ol.Collection();
+ }
+ values[ol.MapProperty.LAYERS] = layers;
values[ol.MapProperty.VIEW] = goog.isDef(mapOptions.view) ?
mapOptions.view : new ol.View2D();
Something went wrong with that request. Please try again.