dont relayout when node dissappears

client/app/scripts/charts/__tests__/node-layout-test.js

@@ -1,7 +1,7 @@
 jest.dontMock('../nodes-layout');
 jest.dontMock('../../constants/naming'); // edge naming: 'source-target'
 
-import { fromJS, is } from 'immutable';
+import { fromJS, List } from 'immutable';
 
 describe('NodesLayout', () => {
   const NodesLayout = require('../nodes-layout');
@@ -16,7 +16,7 @@ describe('NodesLayout', () => {
     left: 0,
     top: 0
   };
-  let history;
+  let history = List();
   let nodes;
 
   const nodeSets = {
@@ -44,9 +44,24 @@ describe('NodesLayout', () => {
         'n1-n3': {id: 'n1-n3', source: 'n1', target: 'n3'},
         'n1-n4': {id: 'n1-n4', source: 'n1', target: 'n4'}
       })
-    }
+    },
+    removeNode2: {
+      nodes: fromJS({
+        n1: {id: 'n1'},
+        n3: {id: 'n3'},
+        n4: {id: 'n4'}
+      }),
+      edges: fromJS({
+        'n1-n3': {id: 'n1-n3', source: 'n1', target: 'n3'},
+        'n1-n4': {id: 'n1-n4', source: 'n1', target: 'n4'}
+      })
+    },
   };
 
+  beforeEach(() => {
+    history = history.clear();
+  })
+
   it('lays out initial nodeset in a rectangle', () => {
     const result = NodesLayout.doLayout(
       nodeSets.initial4.nodes,
@@ -66,10 +81,10 @@ describe('NodesLayout', () => {
     let result = NodesLayout.doLayout(
       nodeSets.initial4.nodes,
       nodeSets.initial4.edges);
-    history = [{
+    history = history.unshift({
       nodes: result.nodes,
       edges: result.edges
-    }];
+    });
     result = NodesLayout.doLayout(
       nodeSets.removeEdge24.nodes,
       nodeSets.removeEdge24.edges,
@@ -91,20 +106,20 @@ describe('NodesLayout', () => {
       nodeSets.initial4.nodes,
       nodeSets.initial4.edges);
 
-    history = [{
+    history = history.unshift({
       nodes: result.nodes,
       edges: result.edges
-    }];
+    });
     result = NodesLayout.doLayout(
       nodeSets.removeEdge24.nodes,
       nodeSets.removeEdge24.edges,
       {history}
     );
 
-    history = [{
+    history = history.unshift({
       nodes: result.nodes,
       edges: result.edges
-    }];
+    });
     result = NodesLayout.doLayout(
       nodeSets.initial4.nodes,
       nodeSets.initial4.edges,
@@ -122,6 +137,27 @@ describe('NodesLayout', () => {
     expect(nodes.n3.y).toEqual(nodes.n4.y);
   });
 
+  it('keeps nodes in rectangle after node dissappears', () => {
+    let result = NodesLayout.doLayout(
+      nodeSets.initial4.nodes,
+      nodeSets.initial4.edges);
 
+    history = history.unshift({
+      nodes: result.nodes,
+      edges: result.edges
+    });
+    result = NodesLayout.doLayout(
+      nodeSets.removeNode2.nodes,
+      nodeSets.removeNode2.edges,
+      {history}
+    );
+
+    nodes = result.nodes.toJS();
+
+    expect(nodes.n1.x).toEqual(nodes.n3.x);
+    expect(nodes.n1.y).toBeLessThan(nodes.n3.y);
+    expect(nodes.n3.x).toBeLessThan(nodes.n4.x);
+    expect(nodes.n3.y).toEqual(nodes.n4.y);
+  });
 
 });

client/app/scripts/charts/nodes-chart.js

@@ -2,6 +2,7 @@ const _ = require('lodash');
 const d3 = require('d3');
 const debug = require('debug')('scope:nodes-chart');
 const React = require('react');
+const makeList = require('immutable').List;
 const makeMap = require('immutable').Map;
 const timely = require('timely');
 const Spring = require('react-motion').Spring;
@@ -21,6 +22,8 @@ const MARGINS = {
   bottom: 0
 };
 
+const MAX_HISTORY = 3;
+
 // make sure circular layouts a bit denser with 3-6 nodes
 const radiusDensity = d3.scale.threshold()
   .domain([3, 6]).range([2.5, 3.5, 3]);
@@ -31,7 +34,7 @@ const NodesChart = React.createClass({
     return {
       nodes: makeMap(),
       edges: makeMap(),
-      history: [],
+      history: makeList(),
       nodeScale: d3.scale.linear(),
       shiftTranslate: [0, 0],
       panTranslate: [0, 0],
@@ -493,8 +496,11 @@ const NodesChart = React.createClass({
       this.zoom.scale(zoomFactor);
     }
 
+    // throw away old layouts and save this layout result, first item is recent
+    const history = state.history.setSize(MAX_HISTORY - 1).unshift(graph);
+
     return {
-      history: [graph],
+      history: history,
       nodes: stateNodes,
       edges: stateEdges,
       nodeScale: nodeScale,

client/app/scripts/charts/nodes-layout.js

@@ -6,6 +6,16 @@ const Naming = require('../constants/naming');
 const MAX_NODES = 100;
 const topologyGraphs = {};
 
+/**
+ * Wrapper around layout engine
+ * After the layout engine run nodes and edges have x-y-coordinates. Creates and
+ * reuses one engine per topology. Engine is not run if the number of nodes is
+ * bigger than `MAX_NODES`.
+ * @param  {Map} imNodes new node set
+ * @param  {Map} imEdges new edge set
+ * @param  {Object} opts    dimensions, scales, etc.
+ * @return {Object}         Layout with nodes, edges, dimensions
+ */
 function runLayoutEngine(imNodes, imEdges, opts) {
   let nodes = imNodes;
   let edges = imEdges;
@@ -119,7 +129,14 @@ function runLayoutEngine(imNodes, imEdges, opts) {
   return layout;
 }
 
-function doLayoutEdges(nodes, edges, previousLayout) {
+/**
+ * Modifies add/remove edges to a previous layout based on what is present in
+ * the new edge set
+ * @param {Map} nodes          new node set
+ * @param {Map} edges          new edges
+ * @param {Object} previousLayout modified layout
+ */
+function addRemoveLayoutEdges(nodes, edges, previousLayout) {
   const previousEdges = previousLayout.edges;
 
   // remove old edges
@@ -147,12 +164,48 @@ function doLayoutEdges(nodes, edges, previousLayout) {
   return previousLayout;
 }
 
+/**
+ * Removes nodes from `previousLayout.nodes` that are not in `nodes` and returns
+ * the modified `previousLayout`.
+ * @param  {Map} nodes          new set of nodes
+ * @param  {Map} edges          new set of edges
+ * @param  {object} previousLayout old layout
+ * @return {Object}                Layout with nodes and and edges
+ */
+function removeOldLayoutNodes(nodes, edges, previousLayout) {
+  const previousNodes = previousLayout.nodes;
+  let layoutNodes = previousNodes.filter(node => {
+    return nodes.has(node.get('id'));
+  });
+  previousLayout.nodes = layoutNodes;
+  return previousLayout;
+}
+
+/**
+ * Determine if two node sets have the same nodes
+ * @param  {Map}  nodes     new node set
+ * @param  {Map}  prevNodes old node set
+ * @return {Boolean}           True if node ids of both sets are the same
+ */
 function hasSameNodes(nodes, prevNodes) {
   return ImmSet.fromKeys(nodes).equals(ImmSet.fromKeys(prevNodes));
 }
 
+/**
+ * Determine if nodes were removed between node sets
+ * @param  {Map} nodes     new Map of nodes
+ * @param  {Map} prevNodes old Map of nodes
+ * @return {Boolean}           True if nodes had no new node ids
+ */
+function wereNodesOnlyRemoved(nodes, prevNodes) {
+  return (nodes.size < prevNodes.size
+    && ImmSet.fromKeys(nodes).isSubset(ImmSet.fromKeys(prevNodes)));
+}
+
 /**
  * Layout of nodes and edges
+ * If a previous layout was given and not too much changed, the previous layout
+ * is changed and returned. Otherwise does a new layout engine run.
  * @param  {Map} nodes All nodes
  * @param  {Map} edges All edges
  * @param  {object} opts  width, height, margins, etc...
@@ -160,15 +213,17 @@ function hasSameNodes(nodes, prevNodes) {
  */
 export function doLayout(nodes, edges, opts) {
   const options = opts || {};
-  const history = options.history || [];
-  const previous = history.pop();
+  const previous = options.history && options.history.first();
   let layout;
 
   if (previous) {
-    // add/remove edges if nodes are the same
-    if (hasSameNodes(previous.nodes, nodes)) {
+    if (hasSameNodes(nodes, previous.nodes)) {
       debug('skip layout, only edges changed', edges.size, previous.edges.size);
-      layout = doLayoutEdges(nodes, edges, previous);
+      layout = addRemoveLayoutEdges(nodes, edges, previous);
+    } else if (wereNodesOnlyRemoved(nodes, previous.nodes)) {
+      debug('skip layout, only nodes removed', nodes.size, previous.nodes.size);
+      layout = removeOldLayoutNodes(nodes, edges, previous);
+      layout = addRemoveLayoutEdges(nodes, edges, layout);
     }
   }