Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't attempt to constrain values in setters #798

Merged
merged 3 commits into from Jun 20, 2013

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented Jun 19, 2013

A number of setters in ol.layer.Layer attempted to constrain the values that could be set which breaks property binding. This PR fixes that by deferring the application of the constraints until the creation of the layer state.

Here are two examples of how the old code could break. Consider brightness (which eventually needs to be in the range -1..1) and opacity (range 0..1).

Case one:

// Bind brightness to opacity, strange but legal
layer.bindTo('brightness', layer, 'opacity');

// What should happen here? -1 is not a valid opacity
layer.setBrightness(-1);

// What should the following output?
console.log(layer.getBrightness());
console.log(layer.getOpacity());

// Is it possible for opacity and brightness to be bound, and 
// for brightness to be equal to the set value (-1)?

Case two:

// Set up the binding chain:
// layer1.brightness -> layer2.brightness -> layer2.opacity -> layer3.brightness
layer1.bindTo('brightness', layer2);
layer2.bindTo('opacity', layer2, 'brightness');
layer3.bindTo('brightness', layer2, 'opacity');

layer3.setBrightness(-1); // A valid brightness value, but not a valid opacity value

// What should the following output?
console.log(layer1.getBrightness());

// How did the opacity constraint of 0..1 affect the the brightness value?

In general, due to binding, it is not possible to apply constraints in setters. Consider the case where two properties with incompatible constraints are bound to each other (e.g. constraint in the first object is x > 0 and constraint in the second object is x < 0; what values can x take?).

The PR fixes these problems by removing the constraints from the properties, and instead sanitising the values when they are used.

@twpayne twpayne mentioned this pull request Jun 19, 2013
@elemoine
Copy link
Member

Sounds sane to me. Thanks.

@tschaub
Copy link
Member

tschaub commented Jun 19, 2013

How will this work if we allow something like +/- buttons to modify opacity? If I get to opacity 1 and click + ten times, do I have to click - 11 times just to reduce the opacity?

@twpayne
Copy link
Contributor Author

twpayne commented Jun 20, 2013

increaseOpacityButton.addEventListener('click', function (e) {
  layer.setOpacity(Math.min(1, layer.getOpacity() + 0.1));
});

@tschaub
Copy link
Member

tschaub commented Jun 20, 2013

I don't think this is going in the right direction to have every control know about the constraint (all in the name of binding to another property without a constraint). I'd like to have more discussion about this before you merge.

@twpayne
Copy link
Contributor Author

twpayne commented Jun 20, 2013

I don't mind holding off merging, but:

  • Controls need to know about valid values anyway, for example an opacity slider has to be configured to generate values in the range 0..1, I don't see how this is any different.
  • What are the alternatives? Specifically, what do you think should happen if two properties are bound that have mutually incompatible constraints?

@twpayne
Copy link
Contributor Author

twpayne commented Jun 20, 2013

Examples of controls needing to know about the valid values:

https://github.com/openlayers/ol3/blob/master/examples/bind-input.html#L54-63

@tschaub
Copy link
Member

tschaub commented Jun 20, 2013

As discussed on the call, at some point it could be nice to configure an ol.Object with some constraints (on type, range, enumerated values, etc.). This would be a nice way to support validation for feature editing, and would mean application code wouldn't need to duplicate these constraints. If this is ever done, it would mean binding would only work between object properties with matching constraints.

For now, we agree that its ok to have controls etc. needing to know about these constraints (without asking the object for them).

So merging is ok by me.

@twpayne
Copy link
Contributor Author

twpayne commented Jun 20, 2013

Thanks for the reviews @elemoine and @tschaub.

twpayne added a commit that referenced this pull request Jun 20, 2013
Don't attempt to constrain values in setters
@twpayne twpayne merged commit b7a1ada into openlayers:master Jun 20, 2013
@twpayne
Copy link
Contributor Author

twpayne commented Jun 20, 2013

P.S. I agree that constraints on ol.Object are desirable.

@twpayne twpayne deleted the fix-layer-setters branch June 20, 2013 19:44
afabiani pushed a commit to geosolutions-it/openlayers that referenced this pull request May 22, 2017
Fix click on PanZoomBar when page is scrolled (fix openlayers#798).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants