From c53303b4ac6b3f3bb38482a4e002863c3afe912e Mon Sep 17 00:00:00 2001 From: Nicolas Riesco Date: Fri, 1 Apr 2016 01:13:02 +0100 Subject: [PATCH 1/5] Split method shapes.draw into smaller functions --- src/components/shapes/index.js | 135 +++++++++++++++++++-------------- 1 file changed, 79 insertions(+), 56 deletions(-) diff --git a/src/components/shapes/index.js b/src/components/shapes/index.js index 916768e797d..a52ad658487 100644 --- a/src/components/shapes/index.js +++ b/src/components/shapes/index.js @@ -115,86 +115,106 @@ shapes.add = function(gd) { // if opt is blank, val can be 'add' or a full options object to add a new // annotation at that point in the array, or 'remove' to delete this one shapes.draw = function(gd, index, opt, value) { - var layout = gd.layout, - fullLayout = gd._fullLayout, - i; - - // TODO: abstract out these drawAll, add, and remove blocks for shapes and annotations if(!isNumeric(index) || index===-1) { // no index provided - we're operating on ALL shapes if(!index && Array.isArray(value)) { - // a whole annotation array is passed in - // (as in, redo of delete all) - layout.shapes = value; - shapes.supplyLayoutDefaults(layout, fullLayout); - shapes.drawAll(gd); + replaceAllShapes(gd, value); return; } else if(value==='remove') { - // delete all - delete layout.shapes; - fullLayout.shapes = []; - shapes.drawAll(gd); + deleteAllShapes(gd); return; } else if(opt && value!=='add') { - // make the same change to all shapes - for(i = 0; i < fullLayout.shapes.length; i++) { - shapes.draw(gd, i, opt, value); - } + updateAllShapes(gd, opt, value); return; } else { // add a new empty annotation - index = fullLayout.shapes.length; - fullLayout.shapes.push({}); + index = gd._fullLayout.shapes.length; + gd._fullLayout.shapes.push({}); } } if(!opt && value) { if(value==='remove') { - fullLayout._shapelayer.selectAll('[data-index="'+index+'"]') - .remove(); - fullLayout.shapes.splice(index,1); - layout.shapes.splice(index,1); - for(i=index; iindex; i--) { - fullLayout._shapelayer - .selectAll('[data-index="'+(i-1)+'"]') - .attr('data-index',String(i)); - shapes.draw(gd,i); - } - } +function deleteAllShapes(gd) { + delete gd.layout.shapes; + gd._fullLayout.shapes = []; + shapes.drawAll(gd); +} + +function updateAllShapes(gd, opt, value) { + for(var i = 0; i < gd._fullLayout.shapes.length; i++) { + shapes.draw(gd, i, opt, value); + } +} + +function deleteShape(gd, index) { + gd._fullLayout._shapelayer.selectAll('[data-index="' + index + '"]') + .remove(); + + gd._fullLayout.shapes.splice(index, 1); + + gd.layout.shapes.splice(index, 1); + + for(var i = index; i < gd._fullLayout.shapes.length; i++) { + // redraw all shapes past the removed one, + // so they bind to the right events + gd._fullLayout._shapelayer + .selectAll('[data-index="' + (i+1) + '"]') + .attr('data-index', String(i)); + shapes.draw(gd, i); + } +} + +function insertShape(gd, index, newShape) { + gd._fullLayout.shapes.splice(index, 0, {}); + + var rule = Plotly.Lib.isPlainObject(newShape) ? + Plotly.Lib.extendFlat({}, newShape) : + {text: 'New text'}; + + if(gd.layout.shapes) { + gd.layout.shapes.splice(index, 0, rule); + } else { + gd.layout.shapes = [rule]; } + for(var i = gd._fullLayout.shapes.length - 1; i > index; i--) { + gd._fullLayout._shapelayer + .selectAll('[data-index="' + (i - 1) + '"]') + .attr('data-index', String(i)); + shapes.draw(gd, i); + } +} + +function updateShape(gd, index, opt, value) { + var i; + // remove the existing shape if there is one - fullLayout._shapelayer.selectAll('[data-index="'+index+'"]').remove(); + gd._fullLayout._shapelayer.selectAll('[data-index="' + index + '"]') + .remove(); // remember a few things about what was already there, - var optionsIn = layout.shapes[index]; + var optionsIn = gd.layout.shapes[index]; // (from annos...) not sure how we're getting here... but C12 is seeing a bug // where we fail here when they add/remove annotations @@ -261,8 +281,8 @@ shapes.draw = function(gd, index, opt, value) { optionsIn[posAttr] = position; } - var options = handleShapeDefaults(optionsIn, fullLayout); - fullLayout.shapes[index] = options; + var options = handleShapeDefaults(optionsIn, gd._fullLayout); + gd._fullLayout.shapes[index] = options; var attrs = { 'data-index': String(index), @@ -273,15 +293,18 @@ shapes.draw = function(gd, index, opt, value) { var lineColor = options.line.width ? options.line.color : 'rgba(0,0,0,0)'; - var path = fullLayout._shapelayer.append('path') + var path = gd._fullLayout._shapelayer.append('path') .attr(attrs) .style('opacity', options.opacity) .call(Plotly.Color.stroke, lineColor) .call(Plotly.Color.fill, options.fillcolor) .call(Plotly.Drawing.dashLine, options.line.dash, options.line.width); - if(clipAxes) path.call(Plotly.Drawing.setClipUrl, 'clip' + fullLayout._uid + clipAxes); -}; + if(clipAxes) { + path.call(Plotly.Drawing.setClipUrl, + 'clip' + gd._fullLayout._uid + clipAxes); + } +} function decodeDate(convertToPx) { return function(v) { return convertToPx(v.replace('_', ' ')); }; From f67ff4d1dcbd4a0a9e5d460ed10020861a8abcae Mon Sep 17 00:00:00 2001 From: Nicolas Riesco Date: Mon, 4 Apr 2016 10:03:38 +0100 Subject: [PATCH 2/5] Remove unnecessary step from shapes.drawAll() * Removed call to `.remove()` in `shapes.drawAll()`, since `.remove()` is also call in `shapes.draw()`. * No new test failed after this change. --- src/components/shapes/index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/shapes/index.js b/src/components/shapes/index.js index a52ad658487..fc6fe8a522a 100644 --- a/src/components/shapes/index.js +++ b/src/components/shapes/index.js @@ -89,7 +89,6 @@ function linearToData(ax) { return ax.type === 'category' ? ax.l2c : ax.l2d; } shapes.drawAll = function(gd) { var fullLayout = gd._fullLayout; - fullLayout._shapelayer.selectAll('path').remove(); for(var i = 0; i < fullLayout.shapes.length; i++) { shapes.draw(gd, i); } From 22d7248628d2e267d9e05e53b4a0bd7598efb488 Mon Sep 17 00:00:00 2001 From: Nicolas Riesco Date: Wed, 6 Apr 2016 23:42:42 +0100 Subject: [PATCH 3/5] Add more shapes tests * Test whether Plotly.relayout() can add and remove shapes. --- test/jasmine/tests/shapes_test.js | 76 +++++++++++++++++++++++++------ 1 file changed, 62 insertions(+), 14 deletions(-) diff --git a/test/jasmine/tests/shapes_test.js b/test/jasmine/tests/shapes_test.js index b9562a88e56..da69057b8f6 100644 --- a/test/jasmine/tests/shapes_test.js +++ b/test/jasmine/tests/shapes_test.js @@ -7,7 +7,7 @@ var createGraphDiv = require('../assets/create_graph_div'); var destroyGraphDiv = require('../assets/destroy_graph_div'); -describe('Test shapes nodes', function() { +describe('Test shapes:', function() { 'use strict'; var mock = require('@mocks/shapes.json'); @@ -28,26 +28,74 @@ describe('Test shapes nodes', function() { return d3.selectAll('.shapelayer').size(); } - function countPaths() { + function countShapePaths() { return d3.selectAll('.shapelayer > path').size(); } - it('has one *shapelayer* node', function() { - expect(countShapeLayers()).toEqual(1); - }); + describe('DOM', function() { + it('has one *shapelayer* node', function() { + expect(countShapeLayers()).toEqual(1); + }); + + it('has as many *path* nodes as there are shapes', function() { + expect(countShapePaths()).toEqual(mock.layout.shapes.length); + }); - it('has as many *path* nodes as there are shapes', function() { - expect(countPaths()).toEqual(mock.layout.shapes.length); + it('should be able to get relayout', function(done) { + expect(countShapeLayers()).toEqual(1); + expect(countShapePaths()).toEqual(mock.layout.shapes.length); + + Plotly.relayout(gd, {height: 200, width: 400}).then(function() { + expect(countShapeLayers()).toEqual(1); + expect(countShapePaths()).toEqual(mock.layout.shapes.length); + }).then(done); + }); }); - it('should be able to get relayout', function(done) { - expect(countShapeLayers()).toEqual(1); - expect(countPaths()).toEqual(mock.layout.shapes.length); + function countShapes(gd) { + return gd.layout.shapes ? + gd.layout.shapes.length : + 0; + } + + function getLastShape(gd) { + return gd.layout.shapes ? + gd.layout.shapes[gd.layout.shapes.length - 1] : + null; + } - Plotly.relayout(gd, {height: 200, width: 400}).then(function() { - expect(countShapeLayers()).toEqual(1); - expect(countPaths()).toEqual(mock.layout.shapes.length); - done(); + function getRandomShape() { + return { + x0: Math.random(), + y0: Math.random(), + x1: Math.random(), + y1: Math.random() + }; + } + + describe('Plotly.relayout', function() { + it('should be able to add a shape', function(done) { + var index = countShapes(gd); + var shape = getRandomShape(); + + Plotly.relayout(gd, 'shapes[' + index + ']', shape).then(function() { + expect(getLastShape(gd)).toEqual(shape); + expect(countShapes(gd)).toEqual(index + 1); + }).then(done); + }); + + it('should be able to remove a shape', function(done) { + var index = countShapes(gd); + var shape = getRandomShape(); + + Plotly.relayout(gd, 'shapes[' + index + ']', shape).then(function() { + expect(getLastShape(gd)).toEqual(shape); + expect(countShapes(gd)).toEqual(index + 1); + }).then(function() { + Plotly.relayout(gd, 'shapes[' + index + ']', 'remove'); + }).then(function() { + expect(countShapes(gd)).toEqual(index); + }).then(done); }); }); }); From f8dbdd18583a66fe6e887804d021f9c7e1e8b920 Mon Sep 17 00:00:00 2001 From: Nicolas Riesco Date: Thu, 7 Apr 2016 16:01:56 +0100 Subject: [PATCH 4/5] Update shapes test * Count number of shape layers after relayout. --- test/jasmine/tests/shapes_test.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/jasmine/tests/shapes_test.js b/test/jasmine/tests/shapes_test.js index da69057b8f6..0f15854dd95 100644 --- a/test/jasmine/tests/shapes_test.js +++ b/test/jasmine/tests/shapes_test.js @@ -79,6 +79,7 @@ describe('Test shapes:', function() { var shape = getRandomShape(); Plotly.relayout(gd, 'shapes[' + index + ']', shape).then(function() { + expect(countShapeLayers()).toEqual(1); expect(getLastShape(gd)).toEqual(shape); expect(countShapes(gd)).toEqual(index + 1); }).then(done); @@ -89,11 +90,13 @@ describe('Test shapes:', function() { var shape = getRandomShape(); Plotly.relayout(gd, 'shapes[' + index + ']', shape).then(function() { + expect(countShapeLayers()).toEqual(1); expect(getLastShape(gd)).toEqual(shape); expect(countShapes(gd)).toEqual(index + 1); }).then(function() { Plotly.relayout(gd, 'shapes[' + index + ']', 'remove'); }).then(function() { + expect(countShapeLayers()).toEqual(1); expect(countShapes(gd)).toEqual(index); }).then(done); }); From 92e46e3c6e40c96d82452a0c878bf1471e7ee260 Mon Sep 17 00:00:00 2001 From: Nicolas Riesco Date: Thu, 7 Apr 2016 16:50:45 +0100 Subject: [PATCH 5/5] Update shapes test * Count the number of paths in the shape layer after relayout. --- test/jasmine/tests/shapes_test.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/jasmine/tests/shapes_test.js b/test/jasmine/tests/shapes_test.js index 0f15854dd95..05ab310d9bb 100644 --- a/test/jasmine/tests/shapes_test.js +++ b/test/jasmine/tests/shapes_test.js @@ -75,28 +75,33 @@ describe('Test shapes:', function() { describe('Plotly.relayout', function() { it('should be able to add a shape', function(done) { + var pathCount = countShapePaths(); var index = countShapes(gd); var shape = getRandomShape(); Plotly.relayout(gd, 'shapes[' + index + ']', shape).then(function() { expect(countShapeLayers()).toEqual(1); + expect(countShapePaths()).toEqual(pathCount + 1); expect(getLastShape(gd)).toEqual(shape); expect(countShapes(gd)).toEqual(index + 1); }).then(done); }); it('should be able to remove a shape', function(done) { + var pathCount = countShapePaths(); var index = countShapes(gd); var shape = getRandomShape(); Plotly.relayout(gd, 'shapes[' + index + ']', shape).then(function() { expect(countShapeLayers()).toEqual(1); + expect(countShapePaths()).toEqual(pathCount + 1); expect(getLastShape(gd)).toEqual(shape); expect(countShapes(gd)).toEqual(index + 1); }).then(function() { Plotly.relayout(gd, 'shapes[' + index + ']', 'remove'); }).then(function() { expect(countShapeLayers()).toEqual(1); + expect(countShapePaths()).toEqual(pathCount); expect(countShapes(gd)).toEqual(index); }).then(done); });