From df69584e893307e8e3ad351c3627fa2637357835 Mon Sep 17 00:00:00 2001 From: Nicolas Riesco Date: Fri, 1 Apr 2016 01:13:02 +0100 Subject: [PATCH 1/7] Split method shapes.draw into smaller functions --- src/components/shapes/index.js | 145 ++++++++++++++++++++------------- 1 file changed, 89 insertions(+), 56 deletions(-) diff --git a/src/components/shapes/index.js b/src/components/shapes/index.js index 916768e797d..d409fd5f4c1 100644 --- a/src/components/shapes/index.js +++ b/src/components/shapes/index.js @@ -115,86 +115,114 @@ 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); + return; +} + +function updateAllShapes(gd, opt, value) { + for(var i = 0; i < gd._fullLayout.shapes.length; i++) { + shapes.draw(gd, i, opt, value); } + return; +} + +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); + } + + return; +} + +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); + } + + return; +} + +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 +289,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 +301,20 @@ 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); + } + + return; +} function decodeDate(convertToPx) { return function(v) { return convertToPx(v.replace('_', ' ')); }; From e342a9e525ef00bec5e18e64ed832764a629b128 Mon Sep 17 00:00:00 2001 From: Nicolas Riesco Date: Mon, 4 Apr 2016 10:03:38 +0100 Subject: [PATCH 2/7] 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 d409fd5f4c1..65b9db34eb6 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 bfc8e0ab3eb9a0523b799c211fe711ce3adb309f Mon Sep 17 00:00:00 2001 From: Nicolas Riesco Date: Tue, 5 Apr 2016 11:39:24 +0100 Subject: [PATCH 3/7] Move select and zoombox elements into shapelayer Fixes #359 --- src/plots/cartesian/graph_interact.js | 11 ++++++++--- src/plots/cartesian/select.js | 6 +++++- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/plots/cartesian/graph_interact.js b/src/plots/cartesian/graph_interact.js index 19c428761fa..aa2ad73ce08 100644 --- a/src/plots/cartesian/graph_interact.js +++ b/src/plots/cartesian/graph_interact.js @@ -1433,6 +1433,9 @@ function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { var x0, y0, + xs = plotinfo.x()._offset, + ys = plotinfo.y()._offset, + zoomlayer = gd._fullLayout._shapelayer, box, lum, path0, @@ -1453,15 +1456,16 @@ function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { dimmed = false; zoomMode = 'xy'; - zb = plotinfo.plot.append('path') + zb = zoomlayer.append('path') .attr('class', 'zoombox') .style({ 'fill': lum>0.2 ? 'rgba(0,0,0,0)' : 'rgba(255,255,255,0)', 'stroke-width': 0 }) + .attr('transform','translate(' + xs + ' ' + ys + ')') .attr('d', path0 + 'Z'); - corners = plotinfo.plot.append('path') + corners = zoomlayer.append('path') .attr('class', 'zoombox-corners') .style({ fill: Plotly.Color.background, @@ -1469,6 +1473,7 @@ function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { 'stroke-width': 1, opacity: 0 }) + .attr('transform','translate(' + xs + ' ' + ys + ')') .attr('d','M0,0Z'); clearSelect(); @@ -1479,7 +1484,7 @@ function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { // until we get around to persistent selections, remove the outline // here. The selection itself will be removed when the plot redraws // at the end. - plotinfo.plot.selectAll('.select-outline').remove(); + zoomlayer.selectAll('.select-outline').remove(); } function zoomMove(dx0, dy0) { diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js index 502f0ea6923..17e863ce4a2 100644 --- a/src/plots/cartesian/select.js +++ b/src/plots/cartesian/select.js @@ -22,8 +22,10 @@ var MINSELECT = constants.MINSELECT; function getAxId(ax) { return ax._id; } module.exports = function prepSelect(e, startX, startY, dragOptions, mode) { - var plot = dragOptions.plotinfo.plot, + var plot = dragOptions.gd._fullLayout._shapelayer, dragBBox = dragOptions.element.getBoundingClientRect(), + xs = dragOptions.plotinfo.x()._offset, + ys = dragOptions.plotinfo.y()._offset, x0 = startX - dragBBox.left, y0 = startY - dragBBox.top, x1 = x0, @@ -45,6 +47,7 @@ module.exports = function prepSelect(e, startX, startY, dragOptions, mode) { outlines.enter() .append('path') .attr('class', function(d) { return 'select-outline select-outline-' + d; }) + .attr('transform','translate(' + xs + ' ' + ys + ')') .attr('d', path0 + 'Z'); var corners = plot.append('path') @@ -54,6 +57,7 @@ module.exports = function prepSelect(e, startX, startY, dragOptions, mode) { stroke: color.defaultLine, 'stroke-width': 1 }) + .attr('transform','translate(' + xs + ' ' + ys + ')') .attr('d','M0,0Z'); From 1d23673b4ea537d7343bb75d3c7d4938995a657b Mon Sep 17 00:00:00 2001 From: Nicolas Riesco Date: Tue, 5 Apr 2016 19:36:05 +0100 Subject: [PATCH 4/7] Shapes: Remove unnecessary call to String() --- src/components/shapes/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/shapes/index.js b/src/components/shapes/index.js index 65b9db34eb6..832916a3c9b 100644 --- a/src/components/shapes/index.js +++ b/src/components/shapes/index.js @@ -183,7 +183,7 @@ function deleteShape(gd, index) { // so they bind to the right events gd._fullLayout._shapelayer .selectAll('[data-index="' + (i+1) + '"]') - .attr('data-index', String(i)); + .attr('data-index', i); shapes.draw(gd, i); } @@ -206,7 +206,7 @@ function insertShape(gd, index, newShape) { for(var i = gd._fullLayout.shapes.length - 1; i > index; i--) { gd._fullLayout._shapelayer .selectAll('[data-index="' + (i - 1) + '"]') - .attr('data-index', String(i)); + .attr('data-index', i); shapes.draw(gd, i); } @@ -292,7 +292,7 @@ function updateShape(gd, index, opt, value) { gd._fullLayout.shapes[index] = options; var attrs = { - 'data-index': String(index), + 'data-index': index, 'fill-rule': 'evenodd', d: shapePath(gd, options) }, From e3bfea7a200a38723d402dfbb23b1bfa0018d8b7 Mon Sep 17 00:00:00 2001 From: Nicolas Riesco Date: Tue, 5 Apr 2016 19:43:39 +0100 Subject: [PATCH 5/7] graph_interact: Definitions before declarations --- src/plots/cartesian/graph_interact.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/plots/cartesian/graph_interact.js b/src/plots/cartesian/graph_interact.js index aa2ad73ce08..5b67792bdfe 100644 --- a/src/plots/cartesian/graph_interact.js +++ b/src/plots/cartesian/graph_interact.js @@ -1431,11 +1431,11 @@ function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { fx.dragElement(dragOptions); - var x0, - y0, + var zoomlayer = gd._fullLayout._shapelayer, xs = plotinfo.x()._offset, ys = plotinfo.y()._offset, - zoomlayer = gd._fullLayout._shapelayer, + x0, + y0, box, lum, path0, From 9b8d3c501a3c4c6149200b8d8057237b05704c0a Mon Sep 17 00:00:00 2001 From: Nicolas Riesco Date: Tue, 5 Apr 2016 20:58:04 +0100 Subject: [PATCH 6/7] Add more tests to select_test.js * Test whether select elements are appended to the shape layer. * Test whether lasso elements are appended to the shape layer. --- test/jasmine/tests/select_test.js | 76 +++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 6eb5b27231e..73550045126 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -1,3 +1,5 @@ +var d3 = require('d3'); + var Plotly = require('@lib/index'); var Lib = require('@src/lib'); var DBLCLICKDELAY = require('@src/plots/cartesian/constants').DBLCLICKDELAY; @@ -54,6 +56,80 @@ describe('select box and lasso', function() { expect(actual.y).toBeCloseToArray(expected.y, PRECISION); } + describe('select elements', function() { + var mockCopy = Lib.extendDeep({}, mock); + mockCopy.layout.dragmode = 'select'; + + var gd; + beforeEach(function(done) { + gd = createGraphDiv(); + + Plotly.plot(gd, mockCopy.data, mockCopy.layout) + .then(done); + }); + + it('should be appended to the the shape layer', function() { + var x0 = 100; + var y0 = 200; + var x1 = 150; + var y1 = 200; + + mouseEvent('mousemove', x0, y0); + expect(d3.selectAll('.shapelayer > .zoombox-corners').size()) + .toEqual(0); + + mouseEvent('mousedown', x0, y0); + mouseEvent('mousemove', x1, y1); + expect(d3.selectAll('.shapelayer > .zoombox-corners').size()) + .toEqual(1); + expect(d3.selectAll('.shapelayer > .select-outline').size()) + .toEqual(2); + + mouseEvent('mouseup', x1, y1); + expect(d3.selectAll('.shapelayer > .zoombox-corners').size()) + .toEqual(0); + expect(d3.selectAll('.shapelayer > .select-outline').size()) + .toEqual(2); + }); + }); + + describe('lasso elements', function() { + var mockCopy = Lib.extendDeep({}, mock); + mockCopy.layout.dragmode = 'lasso'; + + var gd; + beforeEach(function(done) { + gd = createGraphDiv(); + + Plotly.plot(gd, mockCopy.data, mockCopy.layout) + .then(done); + }); + + it('should be appended to the the shape layer', function() { + var x0 = 100; + var y0 = 200; + var x1 = 150; + var y1 = 200; + + mouseEvent('mousemove', x0, y0); + expect(d3.selectAll('.shapelayer > .zoombox-corners').size()) + .toEqual(0); + + mouseEvent('mousedown', x0, y0); + mouseEvent('mousemove', x1, y1); + expect(d3.selectAll('.shapelayer > .zoombox-corners').size()) + .toEqual(1); + expect(d3.selectAll('.shapelayer > .select-outline').size()) + .toEqual(2); + + mouseEvent('mouseup', x1, y1); + expect(d3.selectAll('.shapelayer > .zoombox-corners').size()) + .toEqual(0); + expect(d3.selectAll('.shapelayer > .select-outline').size()) + .toEqual(2); + }); + }); + describe('select events', function() { var mockCopy = Lib.extendDeep({}, mock); mockCopy.layout.dragmode = 'select'; From 4fbf087c7428a7ffc7a689d588530c63532af8ea Mon Sep 17 00:00:00 2001 From: Nicolas Riesco Date: Tue, 5 Apr 2016 21:28:14 +0100 Subject: [PATCH 7/7] Add zoom tests --- test/jasmine/tests/zoom_test.js | 51 +++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 test/jasmine/tests/zoom_test.js diff --git a/test/jasmine/tests/zoom_test.js b/test/jasmine/tests/zoom_test.js new file mode 100644 index 00000000000..b05f1a8c158 --- /dev/null +++ b/test/jasmine/tests/zoom_test.js @@ -0,0 +1,51 @@ +var d3 = require('d3'); + +var Plotly = require('@lib/index'); +var Lib = require('@src/lib'); + +var createGraphDiv = require('../assets/create_graph_div'); +var destroyGraphDiv = require('../assets/destroy_graph_div'); +var mouseEvent = require('../assets/mouse_event'); + + +describe('zoom box element', function() { + var mock = require('@mocks/14.json'); + + var gd; + beforeEach(function(done) { + gd = createGraphDiv(); + + var mockCopy = Lib.extendDeep({}, mock); + mockCopy.layout.dragmode = 'zoom'; + + Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done); + }); + + afterEach(destroyGraphDiv); + + it('should be appended to the the shape layer', function() { + var x0 = 100; + var y0 = 200; + var x1 = 150; + var y1 = 200; + + mouseEvent('mousemove', x0, y0); + expect(d3.selectAll('.shapelayer > .zoombox').size()) + .toEqual(0); + expect(d3.selectAll('.shapelayer > .zoombox-corners').size()) + .toEqual(0); + + mouseEvent('mousedown', x0, y0); + mouseEvent('mousemove', x1, y1); + expect(d3.selectAll('.shapelayer > .zoombox').size()) + .toEqual(1); + expect(d3.selectAll('.shapelayer > .zoombox-corners').size()) + .toEqual(1); + + mouseEvent('mouseup', x1, y1); + expect(d3.selectAll('.shapelayer > .zoombox').size()) + .toEqual(0); + expect(d3.selectAll('.shapelayer > .zoombox-corners').size()) + .toEqual(0); + }); +});