refactor(modeling): move labels + attachers with closure

Prior to this commit we adopted a complicated mechanism
to move attachers and labels after the actual move
closure.

This commit simplifies the matter: We now add attachers
and labels to the move closure and let our existing
move mechanisms handle their movements, too.

As a result, users can rely on the fact that all
elements moved once

* connection layouting is triggered
* the elements.move postExecute phase is triggered

:tada:

lib/features/attach-support/AttachSupport.js

@@ -74,26 +74,15 @@ export default function AttachSupport(injector, eventBus, rules, modeling) {
   });
 
 
-  // move all attachments after the other shapes are done moving
-  this.postExecuted('elements.move', function(event) {
-
-    var context = event.context,
-        delta = context.delta,
-        newParent = context.newParent,
+  // add all attachers to move closure
+  this.preExecuted('elements.move', HIGH_PRIORITY, function(e) {
+    var context = e.context,
         closure = context.closure,
-        enclosedElements = closure.enclosedElements,
-        attachers = getAttachers(enclosedElements);
+        shapes = context.shapes,
+        attachers = getAttachers(shapes);
 
-    // ensure we move all attachers with their hosts
-    // if they have not been moved already
     forEach(attachers, function(attacher) {
-      if (!enclosedElements[attacher.id]) {
-        modeling.moveShape(attacher, delta, newParent);
-
-        forEach(attacher.labels, function(label) {
-          modeling.moveShape(label, delta, newParent);
-        });
-      }
+      closure.add(attacher, closure.topLevel[attacher.host.id]);
     });
   });
 

lib/features/label-support/LabelSupport.js

@@ -1,16 +1,13 @@
 import {
   forEach,
-  filter,
-  reduce
+  filter
 } from 'min-dash';
 
 import inherits from 'inherits';
 
 var LOW_PRIORITY = 250,
     HIGH_PRIORITY = 1400;
 
-import { getMid } from '../../layout/LayoutUtil';
-
 import {
   add as collectionAdd,
   indexOf as collectionIdx
@@ -75,45 +72,28 @@ export default function LabelSupport(injector, eventBus, modeling) {
 
   });
 
-  // fetch all labels to be moved together with their
-  // pre-move mid; we'll use this to determine, if a label
-  // needs move afterwards
-  this.postExecute('elements.move', HIGH_PRIORITY, function(e) {
+  // add all labels to move closure
+  this.preExecuted('elements.move', HIGH_PRIORITY, function(e) {
     var context = e.context,
         closure = context.closure,
         enclosedElements = closure.enclosedElements;
 
-    context.enclosedLabels = reduce(enclosedElements, function(enclosedLabels, element) {
+    var enclosedLabels = [];
+
+    // find labels that are not part of
+    // move closure yet and add them
+    forEach(enclosedElements, function(element) {
       forEach(element.labels, function(label) {
 
         if (!enclosedElements[label.id]) {
-          enclosedLabels.push([ label, getMid(label) ]);
+          enclosedLabels.push(label);
         }
       });
+    });
 
-      return enclosedLabels;
-    }, []);
+    closure.addAll(enclosedLabels);
   });
 
-  // move previously fetched labels, if the have not been moved already
-  this.postExecuted('elements.move', function(e) {
-
-    var context = e.context,
-        labels = context.enclosedLabels,
-        delta = context.delta;
-
-    forEach(labels, function(entry) {
-      var label = entry[0];
-      var mid = entry[1];
-
-      var currentMid = getMid(label);
-
-      // has label not been moved yet?
-      if (currentMid.x === mid.x && currentMid.y === mid.y) {
-        modeling.moveShape(label, delta, label.labelTarget.parent);
-      }
-    });
-  });
 
   this.preExecute([
     'connection.delete',

lib/features/modeling/cmd/helper/MoveClosure.js

@@ -0,0 +1,34 @@
+import {
+  assign
+} from 'min-dash';
+
+import {
+  getClosure
+} from '../../../../util/Elements';
+
+
+export default function MoveClosure() {
+
+  this.allShapes = {};
+  this.allConnections = {};
+
+  this.enclosedElements = {};
+  this.enclosedConnections = {};
+
+  this.topLevel = {};
+}
+
+
+MoveClosure.prototype.add = function(element, isTopLevel) {
+  return this.addAll([ element ], isTopLevel);
+};
+
+
+MoveClosure.prototype.addAll = function(elements, isTopLevel) {
+
+  var newClosure = getClosure(elements, !!isTopLevel, this);
+
+  assign(this, newClosure);
+
+  return this;
+};

lib/features/modeling/cmd/helper/MoveHelper.js

@@ -2,15 +2,14 @@ import {
   forEach
 } from 'min-dash';
 
-import {
-  getClosure
-} from '../../../../util/Elements';
-
 import {
   getMovedSourceAnchor,
   getMovedTargetAnchor
 } from './AnchorsHelper';
 
+import MoveClosure from './MoveClosure';
+
+
 /**
  * A helper that is able to carry out serialized move
  * operations on multiple elements.
@@ -93,6 +92,8 @@ MoveHelper.prototype.moveClosure = function(closure, delta, newParent, newHost,
  * Returns the closure for the selected elements
  *
  * @param  {Array<djs.model.Base>} elements
- * @return {Object} closure
+ * @return {MoveClosure} closure
  */
-MoveHelper.prototype.getClosure = getClosure;
+MoveHelper.prototype.getClosure = function(elements) {
+  return new MoveClosure().addAll(elements, true);
+};

lib/util/Elements.js

@@ -1,6 +1,9 @@
 import {
+  assign,
   isArray,
   isNumber,
+  isObject,
+  isUndefined,
   groupBy,
   forEach
 } from 'min-dash';
@@ -112,20 +115,38 @@ export function selfAndAllChildren(elements, allowDuplicates) {
 
 /**
  * Gets the the closure for all selected elements,
- * their connections and their attachment's connections
+ * their enclosed children and connections.
  *
  * @param {Array<djs.model.Base>} elements
- * @return {Object} enclosure
+ * @param {Boolean} [isTopLevel=true]
+ * @param {Object} [existingClosure]
+ *
+ * @return {Object} newClosure
  */
-export function getClosure(elements) {
+export function getClosure(elements, isTopLevel, closure) {
+
+  if (isUndefined(isTopLevel)) {
+    isTopLevel = true;
+  }
+
+  if (isObject(isTopLevel)) {
+    closure = isTopLevel;
+    isTopLevel = true;
+  }
+
 
-  // original elements passed to this function
-  var topLevel = groupBy(elements, function(e) { return e.id; });
+  closure = closure || {};
+
+  var allShapes = copyObject(closure.allShapes),
+      allConnections = copyObject(closure.allConnections),
+      enclosedElements = copyObject(closure.enclosedElements),
+      enclosedConnections = copyObject(closure.enclosedConnections);
+
+  var topLevel = copyObject(
+    closure.topLevel,
+    isTopLevel && groupBy(elements, function(e) { return e.id; })
+  );
 
-  var allShapes = {},
-      allConnections = {},
-      enclosedElements = {},
-      enclosedConnections = {};
 
   function handleConnection(c) {
     if (topLevel[c.source.id] && topLevel[c.target.id]) {
@@ -287,4 +308,11 @@ export function getType(element) {
   }
 
   return 'root';
+}
+
+
+// helpers ///////////////////////////////
+
+function copyObject(src1, src2) {
+  return assign({}, src1 || {}, src2 || {});
 }

test/spec/features/attach-support/AttachSupportSpec.js

@@ -29,6 +29,9 @@ import { classes as svgClasses } from 'tiny-svg';
 var ATTACH = { attach: true };
 var NO_ATTACH = { attach: false };
 
+/* global sinon */
+var { spy } = sinon;
+
 
 describe('features/attach-support', function() {
 
@@ -81,7 +84,7 @@ describe('features/attach-support', function() {
     beforeEach(inject(function(canvas, elementFactory, modeling) {
 
       host = elementFactory.createShape({
-        id:'host',
+        id: 'host',
         x: 700, y: 100,
         width: 100, height: 100
       });
@@ -193,6 +196,33 @@ describe('features/attach-support', function() {
       expect(parentShape.attachers).not.to.include(attacher);
     }));
 
+
+    it('should move with closure', inject(function(modeling, eventBus) {
+
+      // given
+      var listener = spy(function(event) {
+
+        var closure = event.context.closure;
+
+        // attacher is part of closure
+        expect(closure.allShapes).to.contain.key(attacher.id);
+
+        // attacher did move with closure
+        expect(attacher).to.have.position({
+          x: 400 - 25 - 50,
+          y: 110 - 25
+        });
+      });
+
+      eventBus.once('commandStack.elements.move.postExecuted', 5000, listener);
+
+      // when
+      modeling.moveElements([ parentShape ], { x: -50, y: 0 });
+
+      // then
+      expect(listener).to.have.been.called;
+    }));
+
   });
 
 
@@ -1656,7 +1686,6 @@ describe('features/attach-support', function() {
   });
 
 
-
   describe('append', function() {
 
     var hostShape,

test/spec/features/label-support/LabelSupportSpec.js

@@ -17,6 +17,9 @@ import {
   classes as svgClasses
 } from 'tiny-svg';
 
+/* global sinon */
+var { spy } = sinon;
+
 
 describe('features/label-support', function() {
 
@@ -280,6 +283,34 @@ describe('features/label-support', function() {
 
     });
 
+
+    it('should move with closure', inject(function(modeling, eventBus) {
+
+      // given
+      var listener = spy(function(event) {
+
+        var closure = event.context.closure;
+
+        // labels are part of closure
+        expect(closure.allShapes).to.contain.keys(
+          label.id,
+          otherLabel.id
+        );
+
+        // labels did move with closure
+        expect(label).to.have.position({ x: 235, y: 240 });
+        expect(otherLabel).to.have.position({ x: 235, y: 290 });
+      });
+
+      eventBus.once('commandStack.elements.move.postExecuted', 5000, listener);
+
+      // when
+      modeling.moveElements([ childShape ], { x: 75, y: 10 });
+
+      // then
+      expect(listener).to.have.been.called;
+    }));
+
   });
 
 
@@ -305,33 +336,6 @@ describe('features/label-support', function() {
     }));
 
 
-    it('should not move, if already moved', inject(function(eventBus, modeling) {
-
-      // given
-      var labelPosition = {
-        x: label.x,
-        y: label.y
-      };
-
-      eventBus.once('commandStack.shape.move.postExecute', function(e) {
-
-        var shape = e.context.shape;
-
-        var label = shape.label;
-
-        modeling.moveShape(label, { x: 30, y: 0 });
-      });
-
-      // when
-      modeling.moveElements([ childShape ], { x: 75, y: 0 }, parentShape);
-
-      // then
-      // label was not moved by LabelSupport
-      expect(label.x).to.eql(labelPosition.x + 30);
-      expect(label.y).to.eql(labelPosition.y);
-    }));
-
-
     describe('should drag move with labelTarget', function() {
 
       it('execute', inject(function(move, dragging) {