fix(grid-snapping): correctly update snapped waypoints

* make side-effect free
* use Modeling#updateWaypoints for actual update

lib/features/grid-snapping/behavior/LayoutConnectionBehavior.js

@@ -4,6 +4,10 @@ import CommandInterceptor from 'diagram-js/lib/command/CommandInterceptor';
 
 import { pointsAligned } from 'diagram-js/lib/util/Geometry';
 
+import {
+  assign
+} from 'min-dash';
+
 var HIGH_PRIORITY = 3000;
 
 
@@ -30,11 +34,11 @@ export default function LayoutConnectionBehavior(eventBus, gridSnapping, modelin
       return;
     }
 
-    if (hasMiddleSegments(waypoints)) {
+    if (!hasMiddleSegments(waypoints)) {
-      modeling.updateProperties(connection, {
+      return;
-        waypoints: self.snapMiddleSegments(waypoints)
-      });
     }
+
+    modeling.updateWaypoints(connection, self.snapMiddleSegments(waypoints));
   });
 }
 
@@ -54,34 +58,23 @@ inherits(LayoutConnectionBehavior, CommandInterceptor);
  * @returns {Array<Point>}
  */
 LayoutConnectionBehavior.prototype.snapMiddleSegments = function(waypoints) {
-  var gridSnapping = this._gridSnapping;
+  var gridSnapping = this._gridSnapping,
+      snapped;
 
-  var middleSegments = getMiddleSegments(waypoints);
+  waypoints = waypoints.slice();
 
-  middleSegments.forEach(function(middleSegment) {
+  for (var i = 1; i < waypoints.length - 2; i++) {
-    var segmentStart = middleSegment.start,
-        segmentEnd = middleSegment.end;
 
-    var aligned = pointsAligned(segmentStart, segmentEnd);
+    snapped = snapSegment(gridSnapping, waypoints[i], waypoints[i + 1]);
 
-    if (horizontallyAligned(aligned)) {
+    waypoints[i] = snapped[0];
-
+    waypoints[i + 1] = snapped[1];
-      // snap horizontally
+  }
-      segmentStart.y = segmentEnd.y = gridSnapping.snapValue(segmentStart.y);
-    }
-
-    if (verticallyAligned(aligned)) {
-
-      // snap vertically
-      segmentStart.x = segmentEnd.x = gridSnapping.snapValue(segmentStart.x);
-    }
-  });
 
   return waypoints;
 };
 
 
-
 // helpers //////////
 
 /**
@@ -124,15 +117,28 @@ function verticallyAligned(aligned) {
  *
  * @returns {Array}
  */
-function getMiddleSegments(waypoints) {
+function snapSegment(gridSnapping, segmentStart, segmentEnd) {
-  var middleSegments = [];
 
-  for (var i = 1; i < waypoints.length - 2; i++) {
+  var aligned = pointsAligned(segmentStart, segmentEnd);
-    middleSegments.push({
+
-      start: waypoints[i],
+  var snapped = {};
-      end: waypoints[i + 1]
+
-    });
+  if (horizontallyAligned(aligned)) {
+
+    // snap horizontally
+    snapped.y = gridSnapping.snapValue(segmentStart.y);
   }
 
-  return middleSegments;
+  if (verticallyAligned(aligned)) {
+
+    // snap vertically
+    snapped.x = gridSnapping.snapValue(segmentStart.x);
+  }
+
+  if ('x' in snapped || 'y' in snapped) {
+    segmentStart = assign({}, segmentStart, snapped);
+    segmentEnd = assign({}, segmentEnd, snapped);
+  }
+
+  return [ segmentStart, segmentEnd ];
 }

test/spec/features/grid-snapping/behavior/LayoutConnectionBehavior.bpmn

@@ -1,11 +1,17 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:dc="http://www.omg.org/spec/DD/20100524/DC" id="Definitions_1" targetNamespace="http://bpmn.io/schema/bpmn" exporter="Camunda Modeler" exporterVersion="3.0.0">
+<bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:dc="http://www.omg.org/spec/DD/20100524/DC" xmlns:di="http://www.omg.org/spec/DD/20100524/DI" id="Definitions_1" targetNamespace="http://bpmn.io/schema/bpmn" exporter="bpmn-js (https://demo.bpmn.io)" exporterVersion="4.0.0-beta.1">
   <bpmn:process id="Process_1" isExecutable="true">
     <bpmn:task id="Task_1" />
-    <bpmn:task id="Task_2" />
+    <bpmn:task id="Task_2">
+      <bpmn:outgoing>SequenceFlow_1</bpmn:outgoing>
+    </bpmn:task>
     <bpmn:task id="Task_3" />
     <bpmn:task id="Task_4" />
     <bpmn:boundaryEvent id="BoundaryEvent_1" attachedToRef="Task_3" />
+    <bpmn:task id="Task_5">
+      <bpmn:incoming>SequenceFlow_1</bpmn:incoming>
+    </bpmn:task>
+    <bpmn:sequenceFlow id="SequenceFlow_1" sourceRef="Task_2" targetRef="Task_5" />
   </bpmn:process>
   <bpmndi:BPMNDiagram id="BPMNDiagram_1">
     <bpmndi:BPMNPlane id="BPMNPlane_1" bpmnElement="Process_1">
@@ -24,6 +30,15 @@
       <bpmndi:BPMNShape id="BoundaryEvent_1_di" bpmnElement="BoundaryEvent_1">
         <dc:Bounds x="132" y="462" width="36" height="36" />
       </bpmndi:BPMNShape>
+      <bpmndi:BPMNShape id="Task_5_di" bpmnElement="Task_5">
+        <dc:Bounds x="546" y="270" width="100" height="80" />
+      </bpmndi:BPMNShape>
+      <bpmndi:BPMNEdge id="SequenceFlow_1_di" bpmnElement="SequenceFlow_1">
+        <di:waypoint x="456" y="242" />
+        <di:waypoint x="526" y="242" />
+        <di:waypoint x="526" y="310" />
+        <di:waypoint x="546" y="310" />
+      </bpmndi:BPMNEdge>
     </bpmndi:BPMNPlane>
   </bpmndi:BPMNDiagram>
 </bpmn:definitions>

test/spec/features/grid-snapping/behavior/LayoutConnectionBehaviorSpec.js

@@ -76,7 +76,7 @@ describe('features/grid-snapping - layout connection', function() {
     }));
 
 
-    it('should NOT sap on reconnect start', inject(function(modeling) {
+    it('should NOT snap on reconnect start', inject(function(modeling) {
 
       // when
       modeling.moveElements([ task1 ], { x: 50, y: 50 });
@@ -87,7 +87,7 @@ describe('features/grid-snapping - layout connection', function() {
     }));
 
 
-    it('should NOT sap on reconnect end', inject(function(modeling) {
+    it('should NOT snap on reconnect end', inject(function(modeling) {
 
       // when
       modeling.moveElements([ task2 ], { x: -50, y: -50 });
@@ -97,6 +97,36 @@ describe('features/grid-snapping - layout connection', function() {
       expect(connection.waypoints[2]).to.eql({ x: 225, y: 190 });
     }));
 
+
+    it('should snap', inject(function(modeling, elementRegistry) {
+
+      // given
+      var flow = elementRegistry.get('SequenceFlow_1');
+
+      // when
+      modeling.layoutConnection(flow);
+
+      // then
+      expect(flow.waypoints[1]).to.eql({ x: 530, y: 242 });
+      expect(flow.waypoints[2]).to.eql({ x: 530, y: 310 });
+    }));
+
+
+    it('should UNDO snap', inject(function(modeling, commandStack, elementRegistry) {
+
+      // given
+      var flow = elementRegistry.get('SequenceFlow_1');
+
+      modeling.layoutConnection(flow);
+
+      // when
+      commandStack.undo();
+
+      // then
+      expect(flow.waypoints[1]).to.eql({ x: 526, y: 242 });
+      expect(flow.waypoints[2]).to.eql({ x: 526, y: 310 });
+    }));
+
   });
 
 });