CCK does not limit features

Indeed, currently none of the what's-allowed things about the cck module are working. This is mostly because I don't want the OpenLayers module to do any real parsing of WKT. We need to recreate these things on the Javascript end if they're deemed necessary. The CCK editor could also be built out (currently you can't undo or select/delete a feature).

What do you mean by replace the features?

I think that there are practical reasons to limit the number of features one is able to input, but if we are to do it right, then the field should actually be multiple fields for each feature, or something of that sort - 'proper' multiple entities. 'Faking it' by counting occurrences of text in a text field isn't a good idea.

Also, deleting is very important.

I don't really follow the second part - what is deleting in this context and why is it important?

So, I think that the best option here is to prevent users from adding more than n features if the field is limited to that number, and then they either need to undo and redraw a feature (should we have an undo button) or use the draw feature control (which will be in the editing toolbar) to delete other features to fill space.

This will likely need some kind of notification in the UI, like a message under the map. This shouldn't be invisible. I think that this method is more predictable and possibly less destructive than simply removing the previous additions when you add something over the limit.

"Well, the 1.x version limits the number of features depending on the field setting, and if you reach the max, it simply replaces the last feature with the new one you add. It's hard to say what is the best usability case for this, but I rather think the replacement method is nice."

This is definitely the best solution if the maximum number of features is one. The aproach described in #5 should only be taken into consideration if the maximum number of features is more then one.

Good to see progress here. I've noticed that we have kind of different commenting styles, which is fine, but please keep the comment transform() modifies geometry in the final patch - it's rather important, since transform is somewhat unexpected in that behavior and it goes a long way toward explaining why that code exists. Saying that it's reprojecting features is also kind of wrong, since it's reprojecting a copy of the features which are never used on the map. I'm more into comments that explain the why than the what of code - like specifying unusual turns in code like transform.

any progression at this

+1

Okay, this is, after all, fairly simple to patch. Here's a patch that rewrites a bunch of the code in drawfeatures, eliminating a bunch of variables and making it a bit more robust. It also removes the function that was put into the window. object by that behavior, and enforces the feature limit.

Please test, thanks.

Further reviewed, committed: http://drupal.org/cvs?commit=404072

function openlayers_behavior_drawfeatures_update(features) {
WktWriter = new OpenLayers.Format.WKT();
if (this.feature_limit < features.object.features.length) {
features.feature.layer.removeFeatures(features.object.features.shift());
}

The code above fails to respect 'unlimited', which I belive would be encoded as this.feature_limit being 0.
Haven't tried, but had problems having unlimited features.

Also, when updating an existing system where many features are added bug having
a limit of 1 each click only drops a single old feature, not all except the new one.

Indeed - try this patch that fixes both of those things.

Ok for the first issue (respecting unlimited) as for the second, there seems to be a bigger problem:

features.feature.layer is null

I'll inspect and provide a fix myself

Ok, the thing is that the same 'update' function is invoked on both 'featureadded' and 'featureremoved', with the latter being triggered by the former on removeFeature. The call on 'featureremoved' seems to lack the 'layer' attribute for some reason (a bug in OL?).

Anyway the double invocation doesn't seem to be nice to have. And in any case we only want to perform
the removal on 'featureadded'. Committed:

http://drupal.org/cvs?commit=404270

tmwc: what's the point of performing these check on each iteration?
features.type == 'featureadded' && this.feature_limit

They only need to be performed once, which my patch did...
I'm referring to
http://drupalcode.org/viewvc/drupal/contributions/modules/openlayers/inc...

Didn't seem to make sense as a performance optimization, given that the loop runs, in most cases, once per call. The only case in which it actually iterates is when somebody has created a lot of points and now wants them deleted, and even in that case, it only iterates through the points once. So, yeah, the performance hit - which is likely microscopic, given that there's no scoping problem and it's just testing equality - doesn't seem worth the extra control statement.

Okay? Remarking as fixed..