Merge pull request #378 from n-riesco/issue-148-and-359

Shape clean up

Author: etpinard

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);
     }
@@ -115,86 +114,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; i<fullLayout.shapes.length; i++) {
-                fullLayout._shapelayer
-                    .selectAll('[data-index="'+(i+1)+'"]')
-                    .attr('data-index',String(i));
-
-                // redraw all shapes past the removed one,
-                // so they bind to the right events
-                shapes.draw(gd,i);
-            }
+            deleteShape(gd, index);
             return;
         }
         else if(value==='add' || Plotly.Lib.isPlainObject(value)) {
-            fullLayout.shapes.splice(index,0,{});
+            insertShape(gd, index, value);
+        }
+    }
 
-            var rule = Plotly.Lib.isPlainObject(value) ?
-                    Plotly.Lib.extendFlat({}, value) :
-                    {text: 'New text'};
+    updateShape(gd, index, opt, value);
+};
 
-            if(layout.shapes) {
-                layout.shapes.splice(index, 0, rule);
-            } else {
-                layout.shapes = [rule];
-            }
+function replaceAllShapes(gd, newShapes) {
+    gd.layout.shapes = newShapes;
+    shapes.supplyLayoutDefaults(gd.layout, gd._fullLayout);
+    shapes.drawAll(gd);
+}
 
-            for(i=fullLayout.shapes.length-1; i>index; 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 +280,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 +292,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('_', ' ')); };

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,82 @@ 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 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);
         });
     });
 });