Merge pull request #4036 from alvinlindstam/fix-modify-events

Fix modify interaction event order

src/ol/interaction/modifyinteraction.js

@@ -12,6 +12,7 @@ goog.require('ol.Collection');
 goog.require('ol.CollectionEventType');
 goog.require('ol.Feature');
 goog.require('ol.MapBrowserEvent.EventType');
+goog.require('ol.MapBrowserPointerEvent');
 goog.require('ol.ViewHint');
 goog.require('ol.coordinate');
 goog.require('ol.events.condition');
@@ -151,6 +152,12 @@ ol.interaction.Modify = function(options) {
    */
   this.ignoreNextSingleClick_ = false;
 
+  /**
+   * @type {boolean}
+   * @private
+   */
+  this.modified_ = false;
+
   /**
    * Segment RTree for each layer
    * @type {ol.structs.RBush.<ol.interaction.SegmentDataType>}
@@ -251,6 +258,19 @@ ol.interaction.Modify.prototype.addFeature_ = function(feature) {
 };
 
 
+/**
+ * @param {ol.MapBrowserPointerEvent} evt Map browser event
+ * @private
+ */
+ol.interaction.Modify.prototype.willModifyFeatures_ = function(evt) {
+  if (!this.modified_) {
+    this.modified_ = true;
+    this.dispatchEvent(new ol.ModifyEvent(ol.ModifyEventType.MODIFYSTART,
+        this.features_, evt));
+  }
+};
+
+
 /**
  * @param {ol.Feature} feature Feature.
  * @private
@@ -532,6 +552,7 @@ ol.interaction.Modify.compareIndexes_ = function(a, b) {
 ol.interaction.Modify.handleDownEvent_ = function(evt) {
   this.handlePointerAtPixel_(evt.pixel, evt.map);
   this.dragSegments_ = [];
+  this.modified_ = false;
   var vertexFeature = this.vertexFeature_;
   if (vertexFeature) {
     var insertVertices = [];
@@ -576,12 +597,12 @@ ol.interaction.Modify.handleDownEvent_ = function(evt) {
         insertVertices.push([segmentDataMatch, vertex]);
       }
     }
+    if (insertVertices.length) {
+      this.willModifyFeatures_(evt);
+    }
     for (i = insertVertices.length - 1; i >= 0; --i) {
       this.insertVertex_.apply(this, insertVertices[i]);
     }
-    this.dispatchEvent(
-        new ol.interaction.ModifyEvent(ol.ModifyEventType.MODIFYSTART,
-        this.features_, evt));
   }
   return !!this.vertexFeature_;
 };
@@ -594,6 +615,7 @@ ol.interaction.Modify.handleDownEvent_ = function(evt) {
  */
 ol.interaction.Modify.handleDragEvent_ = function(evt) {
   this.ignoreNextSingleClick_ = false;
+  this.willModifyFeatures_(evt);
 
   var vertex = evt.coordinate;
   for (var i = 0, ii = this.dragSegments_.length; i < ii; ++i) {
@@ -655,9 +677,11 @@ ol.interaction.Modify.handleUpEvent_ = function(evt) {
     this.rBush_.update(ol.extent.boundingExtent(segmentData.segment),
         segmentData);
   }
-  this.dispatchEvent(
+  if (this.modified_) {
-      new ol.interaction.ModifyEvent(ol.ModifyEventType.MODIFYEND,
+    this.dispatchEvent(new ol.ModifyEvent(ol.ModifyEventType.MODIFYEND,
         this.features_, evt));
+    this.modified_ = false;
+  }
   return false;
 };
 
@@ -671,6 +695,10 @@ ol.interaction.Modify.handleUpEvent_ = function(evt) {
  * @api
  */
 ol.interaction.Modify.handleEvent = function(mapBrowserEvent) {
+  if (!(mapBrowserEvent instanceof ol.MapBrowserPointerEvent)) {
+    return true;
+  }
+
   var handled;
   if (!mapBrowserEvent.map.getView().getHints()[ol.ViewHint.INTERACTING] &&
       mapBrowserEvent.type == ol.MapBrowserEvent.EventType.POINTERMOVE &&
@@ -683,7 +711,11 @@ ol.interaction.Modify.handleEvent = function(mapBrowserEvent) {
       var geometry = this.vertexFeature_.getGeometry();
       goog.asserts.assertInstanceof(geometry, ol.geom.Point,
           'geometry should be an ol.geom.Point');
+      this.willModifyFeatures_(mapBrowserEvent);
       handled = this.removeVertex_();
+      this.dispatchEvent(new ol.ModifyEvent(ol.ModifyEventType.MODIFYEND,
+          this.features_, mapBrowserEvent));
+      this.modified_ = false;
     } else {
       handled = true;
     }

test/spec/ol/interaction/modifyinteraction.test.js

@@ -78,6 +78,59 @@ describe('ol.interaction.Modify', function() {
     map.handleMapBrowserEvent(event);
   }
 
+  /**
+   * Tracks events triggered by the interaction as well as feature
+   * modifications. Helper function to
+   * @param {ol.Feature} feature Modified feature.
+   * @param {ol.interaction.Modify} interaction
+   * @return {Array<ol.ModifyEvent|string>} events
+   */
+  function trackEvents(feature, interaction) {
+    var events = [];
+    feature.on('change', function(event) {
+      events.push('change');
+    });
+    interaction.on('modifystart', function(event) {
+      events.push(event);
+    });
+    interaction.on('modifyend', function(event) {
+      events.push(event);
+    });
+    return events;
+  }
+
+  /**
+  * Validates the event array to verify proper event sequence. Checks
+  * that first and last event are correct ModifyEvents and that feature
+  * modifications event are in between.
+  * @param {Array<ol.ModifyEvent|string>} event
+  * @param {Array<ol.Feature>} features
+  */
+  function validateEvents(events, features) {
+
+    var startevent = events[0];
+    var endevent = events[events.length - 1];
+
+    // first event should be modifystary
+    expect(startevent).to.be.an(ol.ModifyEvent);
+    expect(startevent.type).to.eql('modifystart');
+
+    // last event should be modifyend
+    expect(endevent).to.be.an(ol.ModifyEvent);
+    expect(endevent.type).to.eql('modifyend');
+
+    // make sure we get change events to events array
+    expect(events.length > 2).to.be(true);
+    // middle events should be feature modification events
+    for (var i = 1; i < events.length - 2; i++) {
+      expect(events[i]).to.equal('change');
+    }
+
+    // ModifyEvents should include the expected features
+    expect(startevent.features.getArray()).to.eql(features);
+    expect(endevent.features.getArray()).to.eql(features);
+  }
+
   describe('constructor', function() {
     it('adds features to the RTree', function() {
       var feature = new ol.Feature(
@@ -103,11 +156,13 @@ describe('ol.interaction.Modify', function() {
       map.addInteraction(modify);
 
       var first = features[0];
-      var second = features[0];
+      var second = features[1];
+
+      events = trackEvents(first, modify);
 
       expect(first.getGeometry().getRevision()).to.equal(1);
       expect(first.getGeometry().getCoordinates()[0]).to.have.length(5);
-      expect(second.getGeometry().getRevision()).to.equal(1);
+      expect(second.getGeometry().getRevision()).to.equal(2);
       expect(second.getGeometry().getCoordinates()[0]).to.have.length(5);
 
       simulateEvent('pointerdown', 10, -20, false, 0);
@@ -117,22 +172,29 @@ describe('ol.interaction.Modify', function() {
 
       expect(first.getGeometry().getRevision()).to.equal(2);
       expect(first.getGeometry().getCoordinates()[0]).to.have.length(4);
-      expect(second.getGeometry().getRevision()).to.equal(2);
+      expect(second.getGeometry().getRevision()).to.equal(3);
       expect(second.getGeometry().getCoordinates()[0]).to.have.length(4);
+
+      validateEvents(events, features);
     });
 
   });
 
   describe('boundary modification', function() {
+    var modify, feature, events;
 
-    it('clicking vertex should delete it and +r1', function() {
+    beforeEach(function() {
-      var modify = new ol.interaction.Modify({
+      modify = new ol.interaction.Modify({
         features: new ol.Collection(features)
       });
       map.addInteraction(modify);
 
-      var feature = features[0];
+      feature = features[0];
 
+      events = trackEvents(feature, modify);
+    });
+
+    it('clicking vertex should delete it and +r1', function() {
       expect(feature.getGeometry().getRevision()).to.equal(1);
       expect(feature.getGeometry().getCoordinates()[0]).to.have.length(5);
 
@@ -143,16 +205,11 @@ describe('ol.interaction.Modify', function() {
 
       expect(feature.getGeometry().getRevision()).to.equal(2);
       expect(feature.getGeometry().getCoordinates()[0]).to.have.length(4);
+
+      validateEvents(events, [feature]);
     });
 
     it('single clicking boundary should add vertex and +r1', function() {
-      var modify = new ol.interaction.Modify({
-        features: new ol.Collection(features)
-      });
-      map.addInteraction(modify);
-
-      var feature = features[0];
-
       expect(feature.getGeometry().getRevision()).to.equal(1);
       expect(feature.getGeometry().getCoordinates()[0]).to.have.length(5);
 
@@ -163,16 +220,11 @@ describe('ol.interaction.Modify', function() {
 
       expect(feature.getGeometry().getRevision()).to.equal(2);
       expect(feature.getGeometry().getCoordinates()[0]).to.have.length(6);
+
+      validateEvents(events, [feature]);
     });
 
     it('single clicking on created vertex should delete it again', function() {
-      var modify = new ol.interaction.Modify({
-        features: new ol.Collection(features)
-      });
-      map.addInteraction(modify);
-
-      var feature = features[0];
-
       expect(feature.getGeometry().getRevision()).to.equal(1);
       expect(feature.getGeometry().getCoordinates()[0]).to.have.length(5);
 
@@ -184,6 +236,9 @@ describe('ol.interaction.Modify', function() {
       expect(feature.getGeometry().getRevision()).to.equal(2);
       expect(feature.getGeometry().getCoordinates()[0]).to.have.length(6);
 
+      validateEvents(events, [feature]);
+      events.length = 0;
+
       simulateEvent('pointerdown', 40, -20, false, 0);
       simulateEvent('pointerup', 40, -20, false, 0);
       simulateEvent('click', 40, -20, false, 0);
@@ -191,16 +246,11 @@ describe('ol.interaction.Modify', function() {
 
       expect(feature.getGeometry().getRevision()).to.equal(3);
       expect(feature.getGeometry().getCoordinates()[0]).to.have.length(5);
+
+      validateEvents(events, [feature]);
     });
 
     it('clicking with drag should add vertex and +r3', function() {
-      var modify = new ol.interaction.Modify({
-        features: new ol.Collection(features)
-      });
-      map.addInteraction(modify);
-
-      var feature = features[0];
-
       expect(feature.getGeometry().getRevision()).to.equal(1);
       expect(feature.getGeometry().getCoordinates()[0]).to.have.length(5);
 
@@ -212,19 +262,28 @@ describe('ol.interaction.Modify', function() {
 
       expect(feature.getGeometry().getRevision()).to.equal(4);
       expect(feature.getGeometry().getCoordinates()[0]).to.have.length(6);
+
+      validateEvents(events, [feature]);
     });
   });
 
   describe('double click deleteCondition', function() {
 
-    it('should delete vertex on double click', function() {
+    var modify, feature, events;
-      var modify = new ol.interaction.Modify({
+
+    beforeEach(function() {
+      modify = new ol.interaction.Modify({
         features: new ol.Collection(features),
         deleteCondition: ol.events.condition.doubleClick
       });
       map.addInteraction(modify);
 
-      var feature = features[0];
+      feature = features[0];
+
+      events = trackEvents(feature, modify);
+    });
+
+    it('should delete vertex on double click', function() {
 
       expect(feature.getGeometry().getRevision()).to.equal(1);
       expect(feature.getGeometry().getCoordinates()[0]).to.have.length(5);
@@ -239,16 +298,11 @@ describe('ol.interaction.Modify', function() {
 
       expect(feature.getGeometry().getRevision()).to.equal(2);
       expect(feature.getGeometry().getCoordinates()[0]).to.have.length(4);
+
+      validateEvents(events, features);
     });
 
     it('should do nothing on single click', function() {
-      var modify = new ol.interaction.Modify({
-        features: new ol.Collection(features),
-        deleteCondition: ol.events.condition.doubleClick
-      });
-      map.addInteraction(modify);
-
-      var feature = features[0];
 
       expect(feature.getGeometry().getRevision()).to.equal(1);
       expect(feature.getGeometry().getCoordinates()[0]).to.have.length(5);
@@ -260,6 +314,8 @@ describe('ol.interaction.Modify', function() {
 
       expect(feature.getGeometry().getRevision()).to.equal(1);
       expect(feature.getGeometry().getCoordinates()[0]).to.have.length(5);
+
+      expect(events.length).to.eql(0);
     });
   });