Mutable symbolizer properties for styles

[ahocevar]

This change adds setters for symbolizer properties. In addition, it introduces a mutable flag on all styles. By default, this is set to true. ol.style.createStyleFunction() sets it to false for all static styles.

The new setters assert that the mutable flag is true, so whenever an application tries to set a symbolizer property on a style that was assigned to a vector layer or feature overlay, the assertion will fail.

[tschaub]

Curious to hear about the use case. If the is to be used in a style editing application, could regular old objects (like those passes to the symbolized constructors) be used instead?

I fear that people will see the setter methods and assume that is the way to modify existing styles - only to be frustrated when nothing happens (no asserts in compiled code).

[ahocevar]

Good point @tschaub. Yes, the use case is interactive style editing. My first solution would also have been plain object literals, but then I saw that ol.style.Image provides more than just storage for the symbolizer properties.

[elemoine]

It may be obvious but I'm failing to understand the purpose of the mutable flag. When does setting it to false make sense? Why does the createStyleFunction set it to false?

And what if you do

layer.setStyle(styles);
layer.setStyle(function(feature, resolution) {
  return styles;
});

You will have your own style function but with immutable styles. Not sure that's expected.

[elemoine]

And in the case where you have your own style function the layer will not be redrawn if you change a style outside the style function. You have to call layer.dispatchChangeEvent for the layer to be redrawn. So I don't see why the styles are mutable in this case while they are immutable when the style function is created by the layer.

So, if we want mutable styles, I'd just make it clear (in the docs) that the user needs to call dispatchChangeEvent on the layer when changing a style outside the style function. The user needs to understand that the layer does not know about styles and that he is the one managing them.

[ahocevar]

mutable flag will be removed, and documentation improved to educate users to call setStyle for changes to take effect.

[ahocevar]

Removed the mutable_ flag, as discussed in the hangout.

[elemoine]

Code looks good to me. But I think the docs need clarifications. I can try to add a commit to your branch tomorrow if you agree.

[ahocevar]

Sure @elemoine, thanks.

[elemoine]

For the record I know you don't like the idea of calling dispatchChangeEvent on the layer, but I continue to think that it makes more sense to call dispatchChangeEvent than to call setStyle.

The following would look ridiculous to me:

style.getFill().setColor(color);
layer.setStyle(layer.getStyle());

Hard to understand for someone reading that code I think.

This makes more sense to me:

style.getFill().setColor(color);
// notify the layer that we've changed styles (by
// dispatching a change event to it)
layer.dispatchChangeEvent();

We may be able to find better ways in the future. But calling dispatchChangeEvent is the best way currently I think.

[bartvde]

I agree with @elemoine that the second way makes more sense

As an aside, I don't really like the name dispatchChangeEvent, something like update would make more sense to me.

[ahocevar]

Both works, and @elemoine's code does not have to look that ugly:

style.getFill().setColor(color);
layer.setStyle(style);

If the docs are changed from "Use setStyle()" to "Use setStyle() or dispatchChangeEvent()", then everyone would be happy, and we don't need to spend precious time on this rather academic discussion. Update in the works.

[bartvde]

Agreed

[elemoine]

@ahocevar your code won't work if what you passed to the layer initially was a style function. Calling dispatchChangeEvent is a systematic way.

then everyone would be happy, and we don't need to spend precious time on this rather academic discussion.

Too bad (and worrying) that you're taking it that way.

[ahocevar]

Whatever. Docs updated.

[elemoine]

At the risk of being "academic" I don't like "update". Update what? This sounds too abstract to me. But I'll think more about it...

[elemoine]

Calling setStyle or dispatchChangeEvent on what object? What if the style you're modifying is set on a feature? This is what I mean by saying "needs clarification".

[ahocevar]

ol.Feature, ol.layer.Vector and ol.FeatureOverlay all have setStyle() and dispatchChangeEvent(). Suggestions for further clarification welcome.

And @elemoine, don't worry. You know I am a friend of pragmatic, so excuse me that I find this discussion academic.

[ahocevar]

Actually I was wrong. ol.FeatureOverlay does not inherit from ol.Observable, so calling dispatchChangeEvent() does not work there (but setStyle() does, and map.render() does as well).

[elemoine]

I think we should update this patch to replace changed() by dispatchChangeEvent() in the docs and then merge it.

I'm not a big fan of repeating the docs for each style property as done in this patch. But I'll create a new PR if I have ideas on improving this.

[ahocevar]

Please consider #2684. The DispatchChangeEvent method really should be renamed because it does more than just dispatch a change event.

[ahocevar]

@elemoine I moved the documentation about re-rendering to the ol.style.Style classdesc, so you can change it easily to your liking. Merging now.

[elemoine]

Thanks. It makes to get this change in and exercise it a bit. Looking forward to knowing if that makes less the styling framework less "awkward" for people.