SelectFeature and OpenLayers.Feature.Vector.style["select"]

[rdewit]

Currently OL replaces the current style of a feature completely with the default "select" style. This gives problems if your own style has an externalGraphic or a thick line for example.

It is more intuitive to let the "select" style only be the a small list of style elements that will be changed and have the FeatureSelect control extend the current style with the "select" style. It's more in line with what CSS does.

Please look at the patch for more clarity.

[ahocevar]

Roald, I agree with you that the selectStyle should extend the current feature style instead of replacing it. But since selectStyle is an API property of Control.SelectFeature, there is no need to remove properties from the default select style. You can set your custom selectStyle any time, e.g. when you create the control.

Also, it is not a good idea to store the original style as a clone, because applications might rely on the reference to the style that was originally applied to a feature. This reference will be lost when you restore the cloned originalStyle. So lines 224 and 243 should be left unchanged.

[elemoine]

Replying to ahocevar:

Roald, I agree with you that the selectStyle should extend the current feature style instead of replacing it. But since selectStyle is an API property of Control.SelectFeature, there is no need to remove properties from the default select style. You can set your custom selectStyle any time, e.g. when you create the control. Also, it is not a good idea to store the original style as a clone, because applications might rely on the reference to the style that was originally applied to a feature. This reference will be lost when you restore the cloned originalStyle. So lines 224 and 243 should be left unchanged.

---

I agree with Andreas. If one wants the select style to be an extension of the default style, then one can do:

var mySelectStyle = OpenLayers.Util.extend({}, OpenLayers.Feature.Vector.style["default"]);
OpenLayers.Util.extend(mySelectStyle, {fillColor: blue});
var ctrl = new OpenLayers.Control.SelectFeature(layer, {selectStyle: mySelectStyle});

Can this ticket be closed?

[crschmidt]

Eric:

I don't think so. I think Roald has a point here -- your suggestion works if you want only one select style, but if you imagine that I have 17 different features, all different colors (from green to blue to etc.) and then when I select, I want to turn the strokeStyle of the features to hot pink (and only the strokeStyle).

I think that the part of this patch that is worthwhile is the first part of the patchc to selectFeature -- everything else can be dropped. This would allow me to set a selectStyle: {'strokeColor': "hotpink"}.

One question I have, which Andreas can probably answer better than I: If a feature has an OL.Style set as its style, does this work? I think the answer is probably 'no' -- if not, then I think this is the first case we have where something would interfere with that, and at the very least, we would need to document it... But I could be wrong.

[ahocevar]

Chris,

I agree with you: the value that this patch adds is the first part. And you are right that this will not work with OL.Style feature styles. To make it work, the snippet would have to look like this:

var baseStyle = feature.style.CLASS_NAME == "OpenLayers.Style" ?
    feature.style.createStyle(feature) :
    OpenLayers.Util.extend({}, feature.style);
feature.style = OpenLayers.Util.extend(baseStyle, this.selectStyle);

instead of just:

feature.style = OpenLayers.Util.extend(feature.style, this.selectStyle);

[crschmidt]

Okay: If we can go ahead and make that change, and upload a new patch, Ill take a look at it. I'm not sure if we have any tests on this or not, but it might be valuable to have them if we can do it without too much work.

[elemoine]

Ok, I was too fast. But do we want to change the behavior of an API property in 2.6?

[crschmidt]

Can you describe a situation where the behavior will change?

Thus far, we have treated the select style as a 'you must describe all properties you care about'. If the problem is that a property that you don't care about might leak through form the base class due to a narrowly defined select style, I personally consider the current behavior a 'bug' rather than a feature. Example: point with a pointRadius of 10. I use a 'selectStyle' of {'strokeColor': 'red'}... and all of a sudden my point changes size! That wasn't what I was expecting, and I go change my select style.

Are you using things in such a way that this will cause a problem? My personal opinion has always been that this was a bug, but one I didn't care about: maybe I'm wrong, and this is expected behavior that we'd be changing. The difference would mean the differencec between putting this in as is, and doing something a bit smarter about it.

[rdewit]

Before I create a new patch, first some explanation about the current one: I line 224 I clone feature.style. If I don't do that and alter feature.style later (line 227), feature.originalStyle changes too. I did something similar in line 243.

Should I leave Feature.Vector.style in it's original state, therefore changing the selectStyle in my app? If so, please consider that the consequence will be that every app that uses KML (or other fancy formats) will have to create a custom selectStyle. I hoped that my patch wouldn't break any current implementations.

I will take Andreas' test for OL.Style into account. Andreas, I really could use your help with using your Style in my KML code (especially StyleMap).

[ahocevar]

Replying to rdewit:

Before I create a new patch, first some explanation about the current one: I line 224 I clone feature.style. If I don't do that and alter feature.style later (line 227), feature.originalStyle changes too. I did something similar in line 243.

I understand that this is the reason why you did it. But if you create a baseStyle first (like in the code snippet I proposed in my previous comment, you will not alter the original feature.style at all.

Should I leave Feature.Vector.style in it's original state, therefore changing the selectStyle in my app? If so, please consider that the consequence will be that every app that uses KML (or other fancy formats) will have to create a custom selectStyle. I hoped that my patch wouldn't break any current implementations.

This would be the consequence, yes. But once we have finished advanced styling, KML StyleMaps can be directly used for default and select style (see below).

I will take Andreas' test for OL.Style into account. Andreas, I really could use your help with using your Style in my KML code (especially StyleMap).

Have a look at the "Ideas For The Integration of OpenLayers.Style" I proposed in  http://trac.openlayers.org/wiki/Styles. It is about using a StyleMap and make rendering dependent of the rendering intent. If you need more help, don't hesitate to ask. You would, however, have to extract the styles from the KML into OL.Style objects.

[elemoine]

Replying to crschmidt:

Can you describe a situation where the behavior will change? Thus far, we have treated the select style as a 'you must describe all properties you care about'. If the problem is that a property that you don't care about might leak through form the base class due to a narrowly defined select style,

---

Yes that's what I'm referring to.

For example: with the current behavior, if your default style defines externalGraphic and your select style does not, then selected features won't be rendered as external graphics. With the proposed change, selected features will be rendered as external graphics.

---

I personally consider the current behavior a 'bug' rather than a feature. Example: point with a pointRadius of 10. I use a 'selectStyle' of {'strokeColor': 'red'}... and all of a sudden my point changes size! That wasn't what I was expecting, and I go change my select style.

---

Is it going to change size or are you going to get an error because your haven't defined pointRadius in your select style?

---

Are you using things in such a way that this will cause a problem? My personal opinion has always been that this was a bug, but one I didn't care about: maybe I'm wrong, and this is expected behavior that we'd be changing. The difference would mean the differencec between putting this in as is, and doing something a bit smarter about it.

---

I agree that the current behavior isn't very "intuitive". I'm just saying that the proposed change will modify the current behavior and "break" existing apps.

All in all, I think I'm in favor for this change. (If "breaking" existing apps is acceptable.)

[crschmidt]

I think that if this breaks anything, it's because peopel are depending on the behavior of a bug :) So I'd say we should go ahead with this change, and document the change in behavior in the release notes.

(Still needs an updated patch, of course, but bumping to needs more work instead of needs discussion.)

[tschaub]

I think the issues discussed here are captured in #1120. If it turns out that the fix there doesn't address an issue here, please reopen.

[ahocevar]

Reopening, because I think it is better to have separate patches for both issues.

[ahocevar]

New patch that includes the suggestions from the discussion, plus works with all combinations of style hashes and OL.Style objects

[ahocevar]

Just for the record: tests added to test_SelectFeature.html, and all tests pass in FF2 and IE6

[tschaub]

Looks good. Confirmed tests pass in FF. Please commit.

[ahocevar]

(In [5896]) SelectFeature and OpenLayers.Feature.Vector.styleselect?: changed Control.SelectFeature to inherit properties that are not set in selectStyle from feature.style. r=tschaub (closes #1260)

[elemoine]

One comment on the patch commited in r5896: the OpenLayers.Feature.Vector class doesn't define the property originalStyle and I don't like the idea of creating object properties like that. That may be just me, but it looks we tend not to do that in OpenLayers, don't we? Sorry for jumping a bit late in the game!

[elemoine]

Just figured that the originalStyle stuff was there before Andreas' commit. Sorry.

[ahocevar]

Replying to elemoine:

Just figured that the originalStyle stuff was there before Andreas' commit. Sorry.

I am also not happy with this. Have a look at #1120 for ideas on how to handle selectStyle and other style related things in the future. Contributions to that discussion are always welcome.

[elemoine]

Replying to ahocevar:

Replying to elemoine:

Just figured that the originalStyle stuff was there before Andreas' commit. Sorry.

I am also not happy with this. Have a look at #1120 for ideas on how to handle selectStyle and other style related things in the future. Contributions to that discussion are always welcome.

I've started looking at it, but I'm not comfortable enough with the style code to comment on it. I'll keep trying...