ol.layer.Layer: default values

[fredj]

Move the default values into the getters (same as ol.View2D.prototype.getRotation), cleanup constructor

[twpayne]

@fredj, let's chat about this tomorrow. There are many good ideas here, but I'd like to talk through the implications first.

[tschaub]

I'll take the blame for adding the getRotation default and am interested to hear what comes of this discussion.

[elemoine]

@tschaub, did you really mean to close this PR?

Also, the commit link you provided gives a 404.

[tschaub]

@tschaub, did you really mean to close this PR?

No, that's what I get for trying to make comments while driving. I meant to delete my comment instead of closing the pull request :) Still interested in hearing what was discussed here.

[twpayne]

The reason for initialising the properties in a specific order was to ensure that visible was set last, to avoid unnecessary state changes. However, since this occurs in the constructor - and therefore when nothing is listening for events on the object, I think it is OK to use a goog.base(this, options) to initialise properties in an unspecified order and allow arbitrary properties.

I think moving the defaults into the getters is likely to cause strange problems. In fact, I think the defaults can be removed because they are handled already in getLayerState.

[elemoine]

So if we do as @twpayne suggests it we should also change View2D:getRotation and View2D:getView2DState. View2D:getRotation woulld return undefined if no specific rotation has been set. And View2D:getView2DState would return 0 for the rotation value if getRotation returns undefined.

[twpayne]

We've just chatted about this, @fredj can you rebase this on master for a re-review?

[fredj]

Ready for review, thanks.

[twpayne]

Perfect. Please merge.

[twpayne]

So if we do as @twpayne suggests it we should also change View2D:getRotation and View2D:getView2DState. View2D:getRotation woulld return undefined if no specific rotation has been set. And View2D:getView2DState would return 0 for the rotation value if getRotation returns undefined.

For info, I'm creating a PR for this.

[fredj]

Ready for review (again)

[twpayne]

This modifies the options object passed to the constructor, which might not be desirable. Maybe modify a clone instead?

[bbinet]

This seems an expected new feature of ol.layer.Layer: I was just curious to know what use case do you have in mind for adding support for custom properties to a layer?

[elemoine]

E.g. give names to layers. And then search layers by names.

var layer = new ol.layer.TileLayer({
  name: 'my layer'
  ...
});
map.addLayer(layer);

and then:

var layers = map.getLayers().getArray();
var myLayer = jQuery.grep(layers, function(layer, i) { return layer.get('name') === 'my layer'; })[0];

[twpayne]

The use case is to allow users to set properties like name and id to simplify their application logic. Without it, you'd have to maintain these properties elsewhere, outside ol.layer.Layer. With it, you can set them directly on the layer.

[bbinet]

I see, thanks @elemoine and @twpayne for the explanation.

[fredj]

FYI, it works for ol.layer.Layer but not for (at least) ol.layer.TileLayer.
See build/src/external/src/exports.js: the option object passed to the constructor is limited to ol.layer.TileLayerOptions:

arg = {
      brightness: options.brightness,
      contrast: options.contrast,
      hue: options.hue,
      opacity: options.opacity,
      preload: options.preload,
      saturation: options.saturation,
      source: options.source,
      visible: options.visible
};
// ...
goog.base(this, arg);

[elemoine]

That patch:

diff --git a/bin/generate-exports.py b/bin/generate-exports.py
index 2ae2cd8..d098e0c 100755
--- a/bin/generate-exports.py
+++ b/bin/generate-exports.py
@@ -92,7 +92,7 @@ class Class(Exportable):
             lines.append(' */\n')
             lines.append('%s = function(options) {\n' % (self.export_name(),))
             lines.append('  /** @type {%s} */\n' % (self.object_literal.name,))
-            lines.append('  var arg;\n');
+            lines.append('  var arg = /** @type {%s} */ (options);\n' % (self.object_literal.name,));
             lines.append('  if (goog.isDefAndNotNull(options)) {\n')
             # FIXME: we modify the user's options object
             lines.append(''.join(
@@ -103,12 +103,8 @@ class Class(Exportable):
                              {'o': o, 'base': b.name, 'ctor': k.export_name(),
                               'extern': ol.extern_name()} \
                                      for o, ol, k, b in self.nested_options()))
-            lines.append('    arg = {')
-            lines.extend(','.join('\n      %s: options.%s' % (key, key) for key in sorted(self.object_literal.prop_types.keys())))
-            lines.append('\n    };\n')
-            lines.append('  } else {\n')
-            lines.append('    arg = /** @type {%s} */ (options);\n' % (self.object_literal.name,))
-            lines.append('  }\n')
+            lines.extend('\n'.join('    arg.%s = options.%s;' % (key, key) for key in sorted(self.object_literal.prop_types.keys())))
+            lines.append('\n  }\n')
             lines.append('  goog.base(this, arg);\n')
             lines.append('};\n')
             lines.append('goog.inherits(\n')

Seems to work for me. What do you think?

[fredj]

@elemoine I suppose that it works for ol.layer.Layer because of the unit test ...
Thanks for the patch, I'll give a try

[elemoine]

Yes, the unit tests run against ol in debug mode. So TileLayer would also work in the tests.

[fredj]

Your patch works like a charm, see https://github.com/fredj/ol3/compare/layer-options

[elemoine]

Cool. Note that the tests prove nothing here.