From 83d7a87d4382827aeb165bb68e92cc767202a9df Mon Sep 17 00:00:00 2001 From: David Kaltschmidt Date: Wed, 28 Oct 2015 17:02:09 +0000 Subject: [PATCH] dont relayout when node dissappears --- .../charts/__tests__/node-layout-test.js | 54 ++++++++++++--- client/app/scripts/charts/nodes-chart.js | 10 ++- client/app/scripts/charts/nodes-layout.js | 67 +++++++++++++++++-- 3 files changed, 114 insertions(+), 17 deletions(-) diff --git a/client/app/scripts/charts/__tests__/node-layout-test.js b/client/app/scripts/charts/__tests__/node-layout-test.js index f76f7ce00..0ad74e454 100644 --- a/client/app/scripts/charts/__tests__/node-layout-test.js +++ b/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); + }); }); diff --git a/client/app/scripts/charts/nodes-chart.js b/client/app/scripts/charts/nodes-chart.js index 7bc093227..aa33776b5 100644 --- a/client/app/scripts/charts/nodes-chart.js +++ b/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, diff --git a/client/app/scripts/charts/nodes-layout.js b/client/app/scripts/charts/nodes-layout.js index 686973b4d..5d5cfb93e 100644 --- a/client/app/scripts/charts/nodes-layout.js +++ b/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); } }