ashatora/openlayers
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Simplifying and unhacking the ModifyFeature control

Browse Source 
With two nested controls (DragFeature, SelectFeature), which - among
others - activated two OpenLayers.Handler.Feature instances, the
ModifyFeature control was fragile and hard to debug.

The issue that @iwillig, @bartvde and myself tried to track down was
that the two Feature handlers interfered with each other, making it hard
to select features on mobile devices, and causing points to jump during
dragging because of not updated last pixel positions.

With this refactoring, there are no more nested controls. All that's left
is a Drag handler. All tests pass. I had to remove one test that checked
for dragging of an unselected point while another was selected, because
now (and partially even before this change, thanks to the ugly drag
handler hack that is now removed) dragging a point will also select it,
saving the user an extra click.
This commit is contained in:
ahocevar
2013-03-19 23:00:10 +01:00
parent 7002ec49e7
commit d2a32d5421
2 changed files with 125 additions and 210 deletions
Download Patch File Download Diff File Expand all files Collapse all files

229
lib/OpenLayers/Control/ModifyFeature.js
View File

@@ -4,8 +4,7 @@
* full text of the license. */
/**
* @requires OpenLayers/Control/DragFeature.js
* @requires OpenLayers/Control/SelectFeature.js
* @requires OpenLayers/Handler/Drag.js
* @requires OpenLayers/Handler/Keyboard.js
*/
@@ -88,18 +87,6 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
*/
virtualVertices: null,
/**
* Property: selectControl
* {<OpenLayers.Control.SelectFeature>}
*/
selectControl: null,
/**
* Property: dragControl
* {<OpenLayers.Control.DragFeature>}
*/
dragControl: null,
/**
* Property: handlers
* {Object}
@@ -232,64 +219,50 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
if(!(OpenLayers.Util.isArray(this.deleteCodes))) {
this.deleteCodes = [this.deleteCodes];
}
var control = this;
// configure the select control
var selectOptions = {
geometryTypes: this.geometryTypes,
clickout: this.clickout,
toggle: this.toggle,
onBeforeSelect: this.beforeSelectFeature,
onSelect: this.selectFeature,
onUnselect: this.unselectFeature,
scope: this
};
if(this.standalone === false) {
this.selectControl = new OpenLayers.Control.SelectFeature(
layer, selectOptions
);
}
// configure the drag control
var dragOptions = {
documentDrag: this.documentDrag,
geometryTypes: ["OpenLayers.Geometry.Point"],
onStart: function(feature, pixel) {
control.dragStart.apply(control, [feature, pixel]);
// configure the drag handler
var dragCallbacks = {
down: function(pixel) {
this.vertex = null;
var feature = this.layer.getFeatureFromEvent(
this.handlers.drag.evt);
if (feature) {
this.dragStart(feature);
} else if (this.feature && this.clickout) {
this.unselectFeature(this.feature);
}
},
onDrag: function(feature, pixel) {
control.dragVertex.apply(control, [feature, pixel]);
move: function(pixel) {
delete this._unselect;
if (this.vertex) {
this.dragVertex(this.vertex, pixel);
}
},
onComplete: function(feature) {
control.dragComplete.apply(control, [feature]);
up: function() {
this.handlers.drag.stopDown = false;
if (this._unselect) {
this.unselectFeature(this._unselect);
delete this._unselect;
}
},
featureCallbacks: {
over: function(feature) {
/**
* In normal mode, the feature handler is set up to allow
* dragging of all points. In standalone mode, we only
* want to allow dragging of sketch vertices and virtual
* vertices - or, in the case of a modifiable point, the
* point itself.
*/
if(control.standalone !== true || feature._sketch ||
control.feature === feature) {
control.dragControl.overFeature.apply(
control.dragControl, [feature]);
}
done: function(pixel) {
if (this.vertex) {
this.dragComplete(this.vertex);
}
}
};
this.dragControl = new OpenLayers.Control.DragFeature(
layer, dragOptions
);
var dragOptions = {
documentDrag: this.documentDrag,
stopDown: false
};
// configure the keyboard handler
var keyboardOptions = {
keydown: this.handleKeypress
};
this.handlers = {
keyboard: new OpenLayers.Handler.Keyboard(this, keyboardOptions)
keyboard: new OpenLayers.Handler.Keyboard(this, keyboardOptions),
drag: new OpenLayers.Handler.Drag(this, dragCallbacks, dragOptions)
};
},
@@ -299,8 +272,6 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
*/
destroy: function() {
this.layer = null;
this.standalone || this.selectControl.destroy();
this.dragControl.destroy();
OpenLayers.Control.prototype.destroy.apply(this, []);
},
@@ -312,8 +283,8 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
* {Boolean} Successfully activated the control.
*/
activate: function() {
return ((this.standalone || this.selectControl.activate()) &&
this.handlers.keyboard.activate() &&
return (this.handlers.keyboard.activate() &&
this.handlers.drag.activate() &&
OpenLayers.Control.prototype.activate.apply(this, arguments));
},
@@ -331,26 +302,17 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
this.layer.removeFeatures(this.vertices, {silent: true});
this.layer.removeFeatures(this.virtualVertices, {silent: true});
this.vertices = [];
this.dragControl.deactivate();
var feature = this.feature;
var valid = feature && feature.geometry && feature.layer;
if(this.standalone === false) {
if(valid) {
this.selectControl.unselect.apply(this.selectControl,
[feature]);
}
this.selectControl.deactivate();
} else {
if(valid) {
this.unselectFeature(feature);
}
}
this.handlers.drag.deactivate();
this.handlers.keyboard.deactivate();
var feature = this.feature;
if (feature && feature.geometry && feature.layer) {
this.unselectFeature(feature);
}
deactivated = true;
}
return deactivated;
},
/**
* Method: beforeSelectFeature
* Called before a feature is selected.
@@ -375,11 +337,19 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
* feature - {<OpenLayers.Feature.Vector>} the selected feature.
*/
selectFeature: function(feature) {
if (this.geometryTypes && OpenLayers.Util.indexOf(this.geometryTypes,
feature.geometry.CLASS_NAME) == -1) {
return;
}
if (!this.standalone || this.beforeSelectFeature(feature) !== false) {
if (this.feature) {
this.unselectFeature(this.feature);
}
this.feature = feature;
this.layer.selectedFeatures.push(feature);
this.layer.drawFeature(feature, 'select');
this.modified = false;
this.resetVertices();
this.dragControl.activate();
this.onModificationStart(this.feature);
}
// keep track of geometry modifications
@@ -409,8 +379,9 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
this.layer.destroyFeatures([this.radiusHandle], {silent: true});
delete this.radiusHandle;
}
this.layer.drawFeature(this.feature, 'default');
this.feature = null;
this.dragControl.deactivate();
OpenLayers.Util.removeItem(this.layer.selectedFeatures, feature);
this.onModificationEnd(feature);
this.layer.events.triggerEvent("afterfeaturemodified", {
feature: feature,
@@ -418,64 +389,48 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
});
this.modified = false;
},
/**
* Method: dragStart
* Called by the drag feature control with before a feature is dragged.
* This method is used to differentiate between points and vertices
* of higher order geometries. This respects the <geometryTypes>
* property and forces a select of points when the drag control is
* already active (and stops events from propagating to the select
* control).
* Called by the drag handler before a feature is dragged. This method is
* used to differentiate between points and vertices
* of higher order geometries.
*
* Parameters:
* feature - {<OpenLayers.Feature.Vector>} The point or vertex about to be
* dragged.
* pixel - {<OpenLayers.Pixel>} Pixel location of the mouse event.
*/
dragStart: function(feature, pixel) {
// only change behavior if the feature is not in the vertices array
if(feature != this.feature && !feature.geometry.parent &&
feature != this.dragHandle && feature != this.radiusHandle) {
if(this.standalone === false && this.feature) {
// unselect the currently selected feature
this.selectControl.clickFeature.apply(this.selectControl,
[this.feature]);
}
// check any constraints on the geometry type
if(this.geometryTypes == null ||
OpenLayers.Util.indexOf(this.geometryTypes,
feature.geometry.CLASS_NAME) != -1) {
// select the point
this.standalone || this.selectControl.clickFeature.apply(
this.selectControl, [feature]);
/**
* TBD: These lines improve workflow by letting the user
* immediately start dragging after the mouse down.
* However, it is very ugly to be messing with controls
* and their handlers in this way. I'd like a better
* solution if the workflow change is necessary.
*/
// prepare the point for dragging
this.dragControl.overFeature.apply(this.dragControl,
[feature]);
this.dragControl.lastPixel = pixel;
this.dragControl.handlers.drag.started = true;
this.dragControl.handlers.drag.start = pixel;
this.dragControl.handlers.drag.last = pixel;
dragStart: function(feature) {
var isPoint = feature.geometry.CLASS_NAME ==
'OpenLayers.Geometry.Point';
if (!this.standalone &&
((!feature._sketch && isPoint) || !feature._sketch)) {
if (this.toggle && this.feature === feature) {
// mark feature for unselection
this._unselect = feature;
}
this.selectFeature(feature);
}
if (feature._sketch || isPoint) {
// feature is a drag or virtual handle or point
this.vertex = feature;
this.handlers.drag.stopDown = true;
}
},
/**
* Method: dragVertex
* Called by the drag feature control with each drag move of a vertex.
* Called by the drag handler with each drag move of a vertex.
*
* Parameters:
* vertex - {<OpenLayers.Feature.Vector>} The vertex being dragged.
* pixel - {<OpenLayers.Pixel>} Pixel location of the mouse event.
*/
dragVertex: function(vertex, pixel) {
var pos = this.map.getLonLatFromViewPortPx(pixel);
var geom = vertex.geometry;
geom.move(pos.lon - geom.x, pos.lat - geom.y);
this.modified = true;
/**
* Five cases:
@@ -487,9 +442,6 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
*/
if(this.feature.geometry.CLASS_NAME == "OpenLayers.Geometry.Point") {
// dragging a simple point
if(this.feature != vertex) {
this.feature = vertex;
}
this.layer.events.triggerEvent("vertexmodified", {
vertex: vertex.geometry,
feature: this.feature,
@@ -526,7 +478,7 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
this.virtualVertices = [];
}
this.layer.drawFeature(this.feature, this.standalone ? undefined :
this.selectControl.renderIntent);
'select');
}
// keep the vertex on top so it gets the mouseout after dragging
// this should be removed in favor of an option to draw under or
@@ -536,7 +488,7 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
/**
* Method: dragComplete
* Called by the drag feature control when the feature dragging is complete.
* Called by the drag handler when the feature dragging is complete.
*
* Parameters:
* vertex - {<OpenLayers.Feature.Vector>} The vertex being dragged.
@@ -572,16 +524,6 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
* Method: resetVertices
*/
resetVertices: function() {
// if coming from a drag complete we're about to destroy the vertex
// that was just dragged. For that reason, the drag feature control
// will never detect a mouse-out on that vertex, meaning that the drag
// handler won't be deactivated. This can cause errors because the drag
// feature control still has a feature to drag but that feature is
// destroyed. To prevent this, we call outFeature on the drag feature
// control if the control actually has a feature to drag.
if(this.dragControl.feature) {
this.dragControl.outFeature(this.dragControl.feature);
}
if(this.vertices.length > 0) {
this.layer.removeFeatures(this.vertices, {silent: true});
this.vertices = [];
@@ -632,11 +574,10 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
// check for delete key
if(this.feature &&
OpenLayers.Util.indexOf(this.deleteCodes, code) != -1) {
var vertex = this.dragControl.feature;
if(vertex &&
OpenLayers.Util.indexOf(this.vertices, vertex) != -1 &&
!this.dragControl.handlers.drag.dragging &&
vertex.geometry.parent) {
var vertex = this.layer.getFeatureFromEvent(this.handlers.drag.evt);
if (vertex &&
OpenLayers.Util.indexOf(this.vertices, vertex) != -1 &&
!this.handlers.drag.dragging && vertex.geometry.parent) {
// remove the vertex
vertex.geometry.parent.removeComponent(vertex.geometry);
this.layer.events.triggerEvent("vertexremoved", {
@@ -645,8 +586,7 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
pixel: evt.xy
});
this.layer.drawFeature(this.feature, this.standalone ?
undefined :
this.selectControl.renderIntent);
undefined : 'select');
this.modified = true;
this.resetVertices();
this.setFeatureState();
@@ -800,8 +740,7 @@ OpenLayers.Control.ModifyFeature = OpenLayers.Class(OpenLayers.Control, {
* map - {<OpenLayers.Map>} The control's map.
*/
setMap: function(map) {
this.standalone || this.selectControl.setMap(map);
this.dragControl.setMap(map);
this.handlers.drag.setMap(map);
OpenLayers.Control.prototype.setMap.apply(this, arguments);
},

106
tests/Control/ModifyFeature.html
View File

@@ -13,34 +13,27 @@
}
};
var options = {
geometryTypes: "bar"
documentDrag: true
};
var control = new OpenLayers.Control.ModifyFeature(layer, options);
t.ok(control.layer == layer,
"constructor sets layer correctly");
t.eq(control.selectControl.geometryTypes, "bar",
"constructor sets options correctly on feature handler");
t.eq(control.handlers.drag.documentDrag, true,
"constructor sets options correctly on drag handler");
t.eq(control.mode, OpenLayers.Control.ModifyFeature.RESHAPE,
"constructor initializes modification mode correctly");
control.destroy();
}
function test_destroy(t) {
t.plan(2);
t.plan(1);
var map = new OpenLayers.Map("map");
var layer = new OpenLayers.Layer.Vector();
map.addLayer(layer);
var control = new OpenLayers.Control.ModifyFeature(layer);
control.selectControl.destroy = function() {
t.ok(true,
"control.destroy calls destroy on select control");
}
control.dragControl.destroy = function() {
t.ok(true,
"control.destroy calls destroy on feature handler");
}
control.destroy();
t.eq(control.layer, null, "Layer reference removed on destroy.");
map.destroy();
}
@@ -51,11 +44,11 @@
map.addLayer(layer);
var control = new OpenLayers.Control.ModifyFeature(layer);
map.addControl(control);
t.ok(!control.selectControl.active,
"select control is not active prior to activating control");
t.ok(!control.handlers.drag.active,
"drag handler is not active prior to activating control");
control.activate();
t.ok(control.selectControl.active,
"select control is active after activating control");
t.ok(control.handlers.drag.active,
"drag handler is active after activating control");
map.destroy();
}
@@ -99,7 +92,8 @@
);
// mock up vertex deletion
control.dragControl.feature = point;
var origGetFeatureFromEvent = layer.getFeatureFromEvent;
layer.getFeatureFromEvent = function() { return point; };
control.feature = poly;
// we cannot use selectFeature since the control is not part of a map
control._originalGeometry = poly.geometry.clone();
@@ -139,17 +133,18 @@
t.eq(control.feature.state, OpenLayers.State.UPDATE, "feature state set to update");
// now make sure nothing happens if the vertex is mid-drag
control.dragControl.handlers.drag.dragging = true;
control.handlers.drag.dragging = true;
control.handleKeypress({keyCode:delKey});
// clean up
layer.getFeatureFromEvent = origGetFeatureFromEvent;
control.destroy();
layer.destroy();
}
function test_onUnSelect(t) {
t.plan(6);
t.plan(5);
var layer = new OpenLayers.Layer.Vector();
var control = new OpenLayers.Control.ModifyFeature(layer);
var fakeFeature = {'id':'myid'};
@@ -159,7 +154,6 @@
layer.events.on({"afterfeaturemodified": function(event) {
t.eq(event.feature, fakeFeature, "afterfeaturemodified triggered");
}});
control.dragControl.deactivate = function() { t.ok(true, "Deactivate called on drag control"); }
control.onModificationEnd = function (feature) { t.eq(feature.id, fakeFeature.id, "onModificationEnd got feature.") }
layer.removeFeatures = function(verts) {
t.ok(verts == 'a', "Normal verts removed correctly");
@@ -190,10 +184,6 @@
// If a feature is to be modified, control.selectFeature gets called.
// We want this test to fail if selectFeature gets called.
var modified = false;
var _ = OpenLayers.Control.ModifyFeature.prototype.selectFeature;
OpenLayers.Control.ModifyFeature.prototype.selectFeature = function() {
modified = true;
}
var control = new OpenLayers.Control.ModifyFeature(layer);
map.addControl(control);
@@ -202,21 +192,19 @@
// register a listener that will stop feature modification
layer.events.on({"beforefeaturemodified": function() {return false}});
// we can initiate feature modification by selecting a feature with
// the control's select feature control
control.selectControl.select(feature);
// we can initiate feature modification by programmatically selecting
// a feature
control.selectFeature(feature);
if(modified) {
t.fail("selectFeature called, prepping feature for modification");
} else {
t.ok(true, "the beforefeaturemodified listener stopped feature modification");
}
OpenLayers.Control.ModifyFeature.prototype.selectFeature = _;
}
function test_selectFeature(t) {
t.plan(12);
t.plan(9);
var map = new OpenLayers.Map('map');
var layer = new OpenLayers.Layer.Vector("Vectors!", {isBaseLayer: true});
map.addLayer(layer);
@@ -228,7 +216,6 @@
t.ok(obj.feature == fakeFeature, "beforefeaturemodified triggered");
};
layer.events.on({"beforefeaturemodified": callback});
control.dragControl.activate = function() { t.ok(true, "drag Control activated"); }
control.onModificationStart = function(feature) { t.eq(feature.id, fakeFeature.id, "On Modification Start called with correct feature."); }
// Start of testing
@@ -262,7 +249,7 @@
control.selectFeature(fakeFeature);
control.vertices = ['a'];
control.virtualVertices = ['b'];
control.virtualVertices = [{destroy: function() {}}];
layer.addFeatures = function(features) {}
@@ -283,7 +270,7 @@
}
function test_resetVertices(t) {
t.plan(21);
t.plan(20);
var layer = new OpenLayers.Layer.Vector();
var control = new OpenLayers.Control.ModifyFeature(layer);
var point = new OpenLayers.Geometry.Point(5,6);
@@ -340,18 +327,6 @@
t.eq(control.vertices.length, 0, "No vertices when both resizing and reshaping (RESIZE|RESHAPE)");
t.eq(control.virtualVertices.length, 0, "No virtual vertices when both resizing and reshaping (RESIZE|RESHAPE)");
control.dragControl.feature = new OpenLayers.Feature.Vector(polygon);
control.dragControl.map = {};
control.dragControl.map.div = {};
control.dragControl.map.div.style = {};
control.dragControl.map.viewPortDiv = "foo";
control.dragControl.handlers.drag.deactivate = function() {
this.active = false;
}
control.resetVertices();
t.ok(!control.dragControl.handlers.drag.active, "resetVertices deactivates drag handler");
control.dragControl.map = null;
control.destroy();
layer.destroy();
}
@@ -512,17 +487,19 @@
var control = new OpenLayers.Control.ModifyFeature(layer);
map.addControl(control);
control.selectControl.deactivate = function() {
control.handlers.keyboard.deactivate = function() {
t.ok(true,
"control.deactivate calls deactivate on select control");
"control.deactivate calls deactivate on keyboard handler");
}
control.dragControl.deactivate = function() {
control.handlers.drag.deactivate = function() {
t.ok(true,
"control.deactivate calls deactivate on drag control");
"control.deactivate calls deactivate on drag handler");
}
control.active = true;
control.deactivate();
control.handlers.keyboard.deactivate = OpenLayers.Handler.Keyboard.prototype.deactivate;
control.handlers.drag.deactivate = OpenLayers.Handler.Drag.prototype.deactivate;
map.destroy();
}
@@ -609,14 +586,17 @@
layer.events.on({"featuremodified": function(event) {
t.eq(event.feature.id, poly.id, "featuremodified triggered");
}});
control.onModification = function(feature) {
t.eq(feature.id, poly.id,
"onModification called with the right feature on vertex delete");
};
point.geometry.parent = poly.geometry;
control.dragControl.feature = point;
origGetFeatureFromEvent = layer.getFeatureFromEvent;
layer.getFeatureFromEvent = function() { return point; };
control.handleKeypress({keyCode:46});
layer.drawFeature = oldDraw;
layer.getFeatureFromEvent = origGetFeatureFromEvent;
map.destroy();
}
@@ -694,7 +674,7 @@
function test_standalone(t) {
t.plan(18);
t.plan(17);
var map = new OpenLayers.Map("map");
var layer = new OpenLayers.Layer.Vector();
@@ -733,7 +713,6 @@
// activate control
control.activate();
t.eq(control.active, true, "[activate] control activated");
t.eq(control.selectControl, null, "[activate] no select control");
// manually select feature for editing
control.selectFeature(f1);
@@ -761,22 +740,19 @@
t.ok(log[0].feature === f2, "[deactivate] correct feature");
t.eq(log[0].modified, false, "[deactivate] feature not actually modified");
// reactivate control and select a point feature to see if we can drag
// another point feature;
control.activate();
control.selectFeature(f3);
control.dragControl.handlers.feature.triggerCallback("mousemove", "in", [f4]);
t.eq(control.dragControl.handlers.drag.active, false, "cannot drag unselected feature f4");
control.dragControl.handlers.feature.triggerCallback("mousemove", "in", [f3]);
t.eq(control.dragControl.handlers.drag.active, true, "can drag selected feature f3");
// select the polygon feature to make sure that we can drag vertices and
// virtual vertices
control.selectFeature(f2);
control.dragControl.handlers.feature.triggerCallback("mousemove", "in", [control.vertices[0]]);
t.eq(control.dragControl.handlers.drag.active, true, "can drag vertex of feature f2");
control.dragControl.handlers.feature.triggerCallback("mousemove", "in", [control.virtualVertices[0]]);
t.eq(control.dragControl.handlers.drag.active, true, "can drag virtual vertex of feature f2");
var origGetFeatureFromEvent = layer.getFeatureFromEvent;
layer.getFeatureFromEvent = function() { return control.vertices[0]; };
control.handlers.drag.callbacks.down.call(control, new OpenLayers.Pixel(0,0));
t.ok(control.vertex === control.vertices[0], "can drag vertex of feature f2");
t.ok(control.feature === f2, "dragging a vertex does not change the selected feature");
layer.getFeatureFromEvent = function() { return control.virtualVertices[0]; };
control.handlers.drag.callbacks.down.call(control, new OpenLayers.Pixel(0,0));
t.ok(control.vertex === control.virtualVertices[0], "can drag virtual vertex of feature f2");
t.ok(control.feature === f2, "dragging a vertex does not change the selected feature");
layer.getFeatureFromEvent = origGetFeatureFromEvent;
control.deactivate();
map.destroy();