From 69bffe1a7c143b773c9d1a0f4b50e0729256db44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Mon, 7 Nov 2016 17:28:41 -0500 Subject: [PATCH 01/10] test: add cases for Lib.expandObjectPaths w/ array containers --- test/jasmine/tests/lib_test.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/jasmine/tests/lib_test.js b/test/jasmine/tests/lib_test.js index 1cba4bd7745..efb37095478 100644 --- a/test/jasmine/tests/lib_test.js +++ b/test/jasmine/tests/lib_test.js @@ -590,6 +590,20 @@ describe('Test lib.js:', function() { expect(computed).toLooseDeepEqual(expected); }); + it('does not skip over array container set to null values', function() { + var input = {title: 'clear annotations', annotations: null}; + var expected = {title: 'clear annotations', annotations: null}; + var computed = Lib.expandObjectPaths(input); + expect(computed).toLooseDeepEqual(expected); + }); + + it('expands array containers', function() { + var input = {title: 'clear annotation 1', 'annotations[1]': { title: 'new' }}; + var expected = {title: 'clear annotation 1', annotations: [null, { title: 'new' }]}; + var computed = Lib.expandObjectPaths(input); + expect(computed).toLooseDeepEqual(expected); + }); + // TODO: This test is unimplemented since it's a currently-unused corner case. // Getting the test to pass requires some extension (pun?) to extendDeepNoArrays // that's intelligent enough to only selectively merge *some* arrays, in particular From 4c4975576b310b57fb5bd0c2684a3f5f018a2890 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 9 Nov 2016 18:17:15 -0500 Subject: [PATCH 02/10] make clean data work for non-plain-object annotation / shape items --- src/plot_api/helpers.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/plot_api/helpers.js b/src/plot_api/helpers.js index 1753335c49f..31a4010c536 100644 --- a/src/plot_api/helpers.js +++ b/src/plot_api/helpers.js @@ -102,13 +102,12 @@ exports.cleanLayout = function(layout) { } } - if(layout.annotations !== undefined && !Array.isArray(layout.annotations)) { - Lib.warn('Annotations must be an array.'); - delete layout.annotations; - } var annotationsLen = (layout.annotations || []).length; for(i = 0; i < annotationsLen; i++) { var ann = layout.annotations[i]; + + if(!Lib.isPlainObject(ann)) continue; + if(ann.ref) { if(ann.ref === 'paper') { ann.xref = 'paper'; @@ -120,17 +119,17 @@ exports.cleanLayout = function(layout) { } delete ann.ref; } + cleanAxRef(ann, 'xref'); cleanAxRef(ann, 'yref'); } - if(layout.shapes !== undefined && !Array.isArray(layout.shapes)) { - Lib.warn('Shapes must be an array.'); - delete layout.shapes; - } var shapesLen = (layout.shapes || []).length; for(i = 0; i < shapesLen; i++) { var shape = layout.shapes[i]; + + if(!Lib.isPlainObject(shape)) continue; + cleanAxRef(shape, 'xref'); cleanAxRef(shape, 'yref'); } From d21d7825b77b1972c32fa989aa8a0233503b39f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 9 Nov 2016 18:18:39 -0500 Subject: [PATCH 03/10] add general array container defaults handler --- src/plots/array_container_defaults.js | 65 +++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 src/plots/array_container_defaults.js diff --git a/src/plots/array_container_defaults.js b/src/plots/array_container_defaults.js new file mode 100644 index 00000000000..8e6476e4937 --- /dev/null +++ b/src/plots/array_container_defaults.js @@ -0,0 +1,65 @@ +/** +* Copyright 2012-2016, Plotly, Inc. +* All rights reserved. +* +* This source code is licensed under the MIT license found in the +* LICENSE file in the root directory of this source tree. +*/ + +'use strict'; + +var Lib = require('../lib'); + + +/** Convenience wrapper for making array container logic DRY and consistent + * + * @param {object} parentObjIn + * user input object where the container in question is linked + * (i.e. either a user trace object or the user layout object) + * + * @param {object} parentObjOut + * full object where the coerced container will be linked + * (i.e. either a full trace object or the full layout object) + * + * @param {object} opts + * options object: + * - name {string} + * name of the key linking the container in question + * - handleItemDefaults {function} + * defaults method to be called on each item in the array container in question, + * + * N.B. + * + * - opts is passed to handleItemDefaults so it can also store + * links to supplementary data (e.g. fullData for layout components) + * + * - opts.itemIsNotPlainObject is mutated on every pass in case so logic + * in handleItemDefaults relies on that fact. + * + */ +module.exports = function handleArrayContainerDefaults(parentObjIn, parentObjOut, opts) { + var name = opts.name; + + var contIn = Array.isArray(parentObjIn[name]) ? parentObjIn[name] : [], + contOut = parentObjOut[name] = []; + + for(var i = 0; i < contIn.length; i++) { + var itemIn = contIn[i], + itemOut = {}; + + if(!Lib.isPlainObject(itemIn)) { + opts.itemIsNotPlainObject = true; + itemIn = {}; + } + else { + opts.itemIsNotPlainObject = false; + } + + opts.handleItemDefaults(itemIn, itemOut, parentObjOut, opts); + + itemOut._input = itemIn; + itemOut._index = i; + + contOut.push(itemOut); + } +}; From 138ac356d8116368d5b9622127c0b2afc842b4e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 9 Nov 2016 18:19:56 -0500 Subject: [PATCH 04/10] DRY layout array container defaults using handleArrayContainerDefaults --- .../annotations/annotation_defaults.js | 6 ++--- src/components/annotations/defaults.js | 14 +++++------- src/components/annotations/draw.js | 3 ++- src/components/images/defaults.js | 20 +++++++---------- src/components/shapes/defaults.js | 14 +++++------- src/components/shapes/draw.js | 3 ++- src/components/shapes/shape_defaults.js | 7 +++--- src/components/sliders/defaults.js | 22 +++++-------------- src/components/updatemenus/defaults.js | 22 +++++-------------- 9 files changed, 43 insertions(+), 68 deletions(-) diff --git a/src/components/annotations/annotation_defaults.js b/src/components/annotations/annotation_defaults.js index 15aed2c04e1..ea77891e4da 100644 --- a/src/components/annotations/annotation_defaults.js +++ b/src/components/annotations/annotation_defaults.js @@ -16,14 +16,14 @@ var Axes = require('../../plots/cartesian/axes'); var attributes = require('./attributes'); -module.exports = function handleAnnotationDefaults(annIn, fullLayout) { - var annOut = {}; +module.exports = function handleAnnotationDefaults(annIn, annOut, fullLayout, opts) { + opts = opts || {}; function coerce(attr, dflt) { return Lib.coerce(annIn, annOut, attributes, attr, dflt); } - var visible = coerce('visible'); + var visible = coerce('visible', !opts.itemIsNotPlainObject); if(!visible) return annOut; diff --git a/src/components/annotations/defaults.js b/src/components/annotations/defaults.js index a8d56cd1d45..a82adabb815 100644 --- a/src/components/annotations/defaults.js +++ b/src/components/annotations/defaults.js @@ -9,17 +9,15 @@ 'use strict'; +var handleArrayContainerDefaults = require('../../plots/array_container_defaults'); var handleAnnotationDefaults = require('./annotation_defaults'); module.exports = function supplyLayoutDefaults(layoutIn, layoutOut) { - var containerIn = layoutIn.annotations || [], - containerOut = layoutOut.annotations = []; + var opts = { + name: 'annotations', + handleItemDefaults: handleAnnotationDefaults + }; - for(var i = 0; i < containerIn.length; i++) { - var annIn = containerIn[i] || {}, - annOut = handleAnnotationDefaults(annIn, layoutOut); - - containerOut.push(annOut); - } + handleArrayContainerDefaults(layoutIn, layoutOut, opts); }; diff --git a/src/components/annotations/draw.js b/src/components/annotations/draw.js index 92b2734ac73..0e1a51bdfab 100644 --- a/src/components/annotations/draw.js +++ b/src/components/annotations/draw.js @@ -243,7 +243,8 @@ function drawOne(gd, index, opt, value) { optionsIn[axLetter] = position; } - var options = handleAnnotationDefaults(optionsIn, fullLayout); + var options = {}; + handleAnnotationDefaults(optionsIn, options, fullLayout); fullLayout.annotations[index] = options; var xa = Axes.getFromId(gd, options.xref), diff --git a/src/components/images/defaults.js b/src/components/images/defaults.js index 3546d0f9d8a..208f75d5c12 100644 --- a/src/components/images/defaults.js +++ b/src/components/images/defaults.js @@ -8,24 +8,20 @@ 'use strict'; -var Axes = require('../../plots/cartesian/axes'); var Lib = require('../../lib'); -var attributes = require('./attributes'); +var Axes = require('../../plots/cartesian/axes'); +var handleArrayContainerDefaults = require('../../plots/array_container_defaults'); +var attributes = require('./attributes'); var name = 'images'; module.exports = function supplyLayoutDefaults(layoutIn, layoutOut) { - var contIn = Array.isArray(layoutIn[name]) ? layoutIn[name] : [], - contOut = layoutOut[name] = []; + var opts = { + name: name, + handleItemDefaults: imageDefaults + }; - for(var i = 0; i < contIn.length; i++) { - var itemIn = contIn[i] || {}, - itemOut = {}; - - imageDefaults(itemIn, itemOut, layoutOut); - - contOut.push(itemOut); - } + handleArrayContainerDefaults(layoutIn, layoutOut, opts); }; diff --git a/src/components/shapes/defaults.js b/src/components/shapes/defaults.js index 5479f37316f..706bf650a6b 100644 --- a/src/components/shapes/defaults.js +++ b/src/components/shapes/defaults.js @@ -9,17 +9,15 @@ 'use strict'; +var handleArrayContainerDefaults = require('../../plots/array_container_defaults'); var handleShapeDefaults = require('./shape_defaults'); module.exports = function supplyLayoutDefaults(layoutIn, layoutOut) { - var containerIn = layoutIn.shapes || [], - containerOut = layoutOut.shapes = []; + var opts = { + name: 'shapes', + handleItemDefaults: handleShapeDefaults + }; - for(var i = 0; i < containerIn.length; i++) { - var shapeIn = containerIn[i] || {}, - shapeOut = handleShapeDefaults(shapeIn, layoutOut); - - containerOut.push(shapeOut); - } + handleArrayContainerDefaults(layoutIn, layoutOut, opts); }; diff --git a/src/components/shapes/draw.js b/src/components/shapes/draw.js index 487a44c4472..de10e93acab 100644 --- a/src/components/shapes/draw.js +++ b/src/components/shapes/draw.js @@ -234,7 +234,8 @@ function updateShape(gd, index, opt, value) { optionsIn[posAttr] = position; } - var options = handleShapeDefaults(optionsIn, gd._fullLayout); + var options = {}; + handleShapeDefaults(optionsIn, options, gd._fullLayout); gd._fullLayout.shapes[index] = options; var clipAxes; diff --git a/src/components/shapes/shape_defaults.js b/src/components/shapes/shape_defaults.js index 04f70fb30b6..9799b7f1213 100644 --- a/src/components/shapes/shape_defaults.js +++ b/src/components/shapes/shape_defaults.js @@ -15,14 +15,15 @@ var Axes = require('../../plots/cartesian/axes'); var attributes = require('./attributes'); var helpers = require('./helpers'); -module.exports = function handleShapeDefaults(shapeIn, fullLayout) { - var shapeOut = {}; + +module.exports = function handleShapeDefaults(shapeIn, shapeOut, fullLayout, opts) { + opts = opts || {}; function coerce(attr, dflt) { return Lib.coerce(shapeIn, shapeOut, attributes, attr, dflt); } - var visible = coerce('visible'); + var visible = coerce('visible', !opts.itemIsNotPlainObject); if(!visible) return shapeOut; diff --git a/src/components/sliders/defaults.js b/src/components/sliders/defaults.js index b4b3bdce900..aedf22e69fb 100644 --- a/src/components/sliders/defaults.js +++ b/src/components/sliders/defaults.js @@ -9,6 +9,7 @@ 'use strict'; var Lib = require('../../lib'); +var handleArrayContainerDefaults = require('../../plots/array_container_defaults'); var attributes = require('./attributes'); var constants = require('./constants'); @@ -18,23 +19,12 @@ var stepAttrs = attributes.steps; module.exports = function slidersDefaults(layoutIn, layoutOut) { - var contIn = Array.isArray(layoutIn[name]) ? layoutIn[name] : [], - contOut = layoutOut[name] = []; + var opts = { + name: name, + handleItemDefaults: sliderDefaults + }; - for(var i = 0; i < contIn.length; i++) { - var sliderIn = contIn[i] || {}, - sliderOut = {}; - - sliderDefaults(sliderIn, sliderOut, layoutOut); - - // used on button click to update the 'active' field - sliderOut._input = sliderIn; - - // used to determine object constancy - sliderOut._index = i; - - contOut.push(sliderOut); - } + handleArrayContainerDefaults(layoutIn, layoutOut, opts); }; function sliderDefaults(sliderIn, sliderOut, layoutOut) { diff --git a/src/components/updatemenus/defaults.js b/src/components/updatemenus/defaults.js index d32a8c1892a..ea05569680a 100644 --- a/src/components/updatemenus/defaults.js +++ b/src/components/updatemenus/defaults.js @@ -9,6 +9,7 @@ 'use strict'; var Lib = require('../../lib'); +var handleArrayContainerDefaults = require('../../plots/array_container_defaults'); var attributes = require('./attributes'); var constants = require('./constants'); @@ -18,23 +19,12 @@ var buttonAttrs = attributes.buttons; module.exports = function updateMenusDefaults(layoutIn, layoutOut) { - var contIn = Array.isArray(layoutIn[name]) ? layoutIn[name] : [], - contOut = layoutOut[name] = []; + var opts = { + name: name, + handleItemDefaults: menuDefaults + }; - for(var i = 0; i < contIn.length; i++) { - var menuIn = contIn[i] || {}, - menuOut = {}; - - menuDefaults(menuIn, menuOut, layoutOut); - - // used on button click to update the 'active' field - menuOut._input = menuIn; - - // used to determine object constancy - menuOut._index = i; - - contOut.push(menuOut); - } + handleArrayContainerDefaults(layoutIn, layoutOut, opts); }; function menuDefaults(menuIn, menuOut, layoutOut) { From d0953a0e40c6dab84b79bf5658860b905427566d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 9 Nov 2016 18:54:39 -0500 Subject: [PATCH 05/10] test: add cases for array container defaults --- test/jasmine/tests/annotations_test.js | 70 ++++++++++++++++++++---- test/jasmine/tests/layout_images_test.js | 14 ++++- test/jasmine/tests/shapes_test.js | 44 +++++++++++++++ test/jasmine/tests/updatemenus_test.js | 32 +++++++++++ 4 files changed, 146 insertions(+), 14 deletions(-) diff --git a/test/jasmine/tests/annotations_test.js b/test/jasmine/tests/annotations_test.js index 4816097dea5..2cd7a81ca12 100644 --- a/test/jasmine/tests/annotations_test.js +++ b/test/jasmine/tests/annotations_test.js @@ -10,31 +10,79 @@ var customMatchers = require('../assets/custom_matchers'); var createGraphDiv = require('../assets/create_graph_div'); var destroyGraphDiv = require('../assets/destroy_graph_div'); + describe('Test annotations', function() { 'use strict'; describe('supplyLayoutDefaults', function() { + + function _supply(layoutIn, layoutOut) { + layoutOut = layoutOut || {}; + layoutOut._has = Plots._hasPlotType.bind(layoutOut); + + Annotations.supplyLayoutDefaults(layoutIn, layoutOut); + + return layoutOut.annotations; + } + + it('should skip non-array containers', function() { + [null, undefined, {}, 'str', 0, false, true].forEach(function(cont) { + var msg = '- ' + JSON.stringify(cont); + var layoutIn = { annotations: cont }; + var out = _supply(layoutIn); + + expect(layoutIn.annotations).toBe(cont, msg); + expect(out).toEqual([], msg); + }); + }); + + it('should make non-object item visible: false', function() { + var annotations = [null, undefined, [], 'str', 0, false, true]; + var layoutIn = { annotations: annotations }; + var out = _supply(layoutIn); + + expect(layoutIn.annotations).toEqual(annotations); + + out.forEach(function(item, i) { + expect(item).toEqual({ + visible: false, + _input: {}, + _index: i + }); + }); + }); + it('should default to pixel for axref/ayref', function() { - var annotationDefaults = {}; - annotationDefaults._has = Plots._hasPlotType.bind(annotationDefaults); + var layoutIn = { + annotations: [{ showarrow: true, arrowhead: 2}] + }; - Annotations.supplyLayoutDefaults({ annotations: [{ showarrow: true, arrowhead: 2}] }, annotationDefaults); + var out = _supply(layoutIn); - expect(annotationDefaults.annotations[0].axref).toEqual('pixel'); - expect(annotationDefaults.annotations[0].ayref).toEqual('pixel'); + expect(out[0].axref).toEqual('pixel'); + expect(out[0].ayref).toEqual('pixel'); }); it('should convert ax/ay date coordinates to milliseconds if tail is in axis terms and axis is a date', function() { - var annotationOut = { xaxis: { type: 'date', range: ['2000-01-01', '2016-01-01'] }}; - annotationOut._has = Plots._hasPlotType.bind(annotationOut); + var layoutIn = { + annotations: [{ + showarrow: true, + axref: 'x', + ayref: 'y', + x: '2008-07-01', + ax: '2004-07-01', + y: 0, + ay: 50 + }] + }; - var annotationIn = { - annotations: [{ showarrow: true, axref: 'x', ayref: 'y', x: '2008-07-01', ax: '2004-07-01', y: 0, ay: 50}] + var layoutOut = { + xaxis: { type: 'date', range: ['2000-01-01', '2016-01-01'] } }; - Annotations.supplyLayoutDefaults(annotationIn, annotationOut); + _supply(layoutIn, layoutOut); - expect(annotationIn.annotations[0].ax).toEqual(Dates.dateTime2ms('2004-07-01')); + expect(layoutIn.annotations[0].ax).toEqual(Dates.dateTime2ms('2004-07-01')); }); }); }); diff --git a/test/jasmine/tests/layout_images_test.js b/test/jasmine/tests/layout_images_test.js index 6aedc708425..71d7c2c8692 100644 --- a/test/jasmine/tests/layout_images_test.js +++ b/test/jasmine/tests/layout_images_test.js @@ -27,7 +27,11 @@ describe('Layout images', function() { Images.supplyLayoutDefaults(layoutIn, layoutOut); - expect(layoutOut.images).toEqual([{ visible: false }]); + expect(layoutOut.images).toEqual([{ + visible: false, + _index: 0, + _input: layoutIn.images[0] + }]); }); it('should reject when not an array', function() { @@ -44,7 +48,9 @@ describe('Layout images', function() { }); it('should coerce the correct defaults', function() { - layoutIn.images[0] = { source: jsLogo }; + var image = { source: jsLogo }; + + layoutIn.images[0] = image; var expected = { source: jsLogo, @@ -59,7 +65,9 @@ describe('Layout images', function() { sizing: 'contain', opacity: 1, xref: 'paper', - yref: 'paper' + yref: 'paper', + _input: image, + _index: 0 }; Images.supplyLayoutDefaults(layoutIn, layoutOut); diff --git a/test/jasmine/tests/shapes_test.js b/test/jasmine/tests/shapes_test.js index c00f104a90d..f6ac1131fbc 100644 --- a/test/jasmine/tests/shapes_test.js +++ b/test/jasmine/tests/shapes_test.js @@ -1,9 +1,12 @@ +var Shapes = require('@src/components/shapes'); var helpers = require('@src/components/shapes/helpers'); var constants = require('@src/components/shapes/constants'); var Plotly = require('@lib/index'); var PlotlyInternal = require('@src/plotly'); var Lib = require('@src/lib'); + +var Plots = PlotlyInternal.Plots; var Axes = PlotlyInternal.Axes; var d3 = require('d3'); @@ -12,6 +15,47 @@ var createGraphDiv = require('../assets/create_graph_div'); var destroyGraphDiv = require('../assets/destroy_graph_div'); +describe('Test shapes defaults:', function() { + 'use strict'; + + function _supply(layoutIn, layoutOut) { + layoutOut = layoutOut || {}; + layoutOut._has = Plots._hasPlotType.bind(layoutOut); + + Shapes.supplyLayoutDefaults(layoutIn, layoutOut); + + return layoutOut.shapes; + } + + it('should skip non-array containers', function() { + [null, undefined, {}, 'str', 0, false, true].forEach(function(cont) { + var msg = '- ' + JSON.stringify(cont); + var layoutIn = { shapes: cont }; + var out = _supply(layoutIn); + + expect(layoutIn.shapes).toBe(cont, msg); + expect(out).toEqual([], msg); + }); + }); + + it('should make non-object item visible: false', function() { + var shapes = [null, undefined, [], 'str', 0, false, true]; + var layoutIn = { shapes: shapes }; + var out = _supply(layoutIn); + + expect(layoutIn.shapes).toEqual(shapes); + + out.forEach(function(item, i) { + expect(item).toEqual({ + visible: false, + _input: {}, + _index: i + }); + }); + }); + +}); + describe('Test shapes:', function() { 'use strict'; diff --git a/test/jasmine/tests/updatemenus_test.js b/test/jasmine/tests/updatemenus_test.js index 77afe12a5e5..b34558d3491 100644 --- a/test/jasmine/tests/updatemenus_test.js +++ b/test/jasmine/tests/updatemenus_test.js @@ -21,6 +21,38 @@ describe('update menus defaults', function() { layoutOut = {}; }); + it('should skip non-array containers', function() { + [null, undefined, {}, 'str', 0, false, true].forEach(function(cont) { + var msg = '- ' + JSON.stringify(cont); + + layoutIn = { updatemenus: cont }; + layoutOut = {}; + supply(layoutIn, layoutOut); + + expect(layoutIn.updatemenus).toBe(cont, msg); + expect(layoutOut.updatemenus).toEqual([], msg); + }); + }); + + it('should make non-object item visible: false', function() { + var updatemenus = [null, undefined, [], 'str', 0, false, true]; + + layoutIn = { updatemenus: updatemenus }; + layoutOut = {}; + supply(layoutIn, layoutOut); + + expect(layoutIn.updatemenus).toEqual(updatemenus); + + layoutOut.updatemenus.forEach(function(item, i) { + expect(item).toEqual({ + visible: false, + buttons: [], + _input: {}, + _index: i + }); + }); + }); + it('should set \'visible\' to false when no buttons are present', function() { layoutIn.updatemenus = [{ buttons: [{ From 174b6404b4903fe95739021e5d05dbd0a68dd8b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 9 Nov 2016 18:56:13 -0500 Subject: [PATCH 06/10] make sure array containers set to null in frames are honored --- src/plots/plots.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index 157489c285d..1e4a7c46cbb 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1578,15 +1578,20 @@ plots.extendObjectWithContainers = function(dest, src, containerPaths) { if(!srcContainer) continue; destProp = Lib.nestedProperty(dest, containerPaths[i]); - destContainer = destProp.get(); + if(!Array.isArray(destContainer)) { destContainer = []; destProp.set(destContainer); } for(j = 0; j < srcContainer.length; j++) { - destContainer[j] = plots.extendObjectWithContainers(destContainer[j], srcContainer[j]); + var srcObj = srcContainer[j]; + + if(srcObj === null) destContainer[j] = null; + else { + destContainer[j] = plots.extendObjectWithContainers(destContainer[j], srcObj); + } } destProp.set(destContainer); From 36859f10900709d1beee28cb8c314f00d1c5b015 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 9 Nov 2016 18:57:50 -0500 Subject: [PATCH 07/10] make sure items inside array container set to null are honored in frames - items set to undefined produce the same outcome as nestedProperty makes no distinctions between null and undefined --- src/plots/plots.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index 1e4a7c46cbb..5a7209bfae5 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1563,8 +1563,14 @@ plots.extendObjectWithContainers = function(dest, src, containerPaths) { for(i = 0; i < containerPaths.length; i++) { containerProp = Lib.nestedProperty(expandedObj, containerPaths[i]); containerVal = containerProp.get(); - containerProp.set(null); - Lib.nestedProperty(containerObj, containerPaths[i]).set(containerVal); + + if(containerVal === undefined) { + Lib.nestedProperty(containerObj, containerPaths[i]).set(null); + } + else { + containerProp.set(null); + Lib.nestedProperty(containerObj, containerPaths[i]).set(containerVal); + } } } From 66f8bb79188a6151a48778ee9372c16954902af3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 9 Nov 2016 18:58:24 -0500 Subject: [PATCH 08/10] test: add cases for extendObjectWithContainers --- test/jasmine/tests/plots_test.js | 67 ++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/test/jasmine/tests/plots_test.js b/test/jasmine/tests/plots_test.js index 0ffbae73480..364e348b3a5 100644 --- a/test/jasmine/tests/plots_test.js +++ b/test/jasmine/tests/plots_test.js @@ -427,4 +427,71 @@ describe('Test Plots', function() { expect(gd._transitioning).toBeUndefined(); }); }); + + describe('extendObjectWithContainers', function() { + + function assert(dest, src, expected) { + Plots.extendObjectWithContainers(dest, src, ['container']); + expect(dest).toEqual(expected); + } + + it('extend each container items', function() { + var dest = { + container: [ + { text: '1', x: 1, y: 1 }, + { text: '2', x: 2, y: 2 } + ] + }; + + var src = { + container: [ + { text: '1-new' }, + { text: '2-new' } + ] + }; + + var expected = { + container: [ + { text: '1-new', x: 1, y: 1 }, + { text: '2-new', x: 2, y: 2 } + ] + }; + + assert(dest, src, expected); + }); + + it('clears container items when applying null src items', function() { + var dest = { + container: [ + { text: '1', x: 1, y: 1 }, + { text: '2', x: 2, y: 2 } + ] + }; + + var src = { + container: [null, null] + }; + + var expected = { + container: [null, null] + }; + + assert(dest, src, expected); + }); + + it('clears container applying null src', function() { + var dest = { + container: [ + { text: '1', x: 1, y: 1 }, + { text: '2', x: 2, y: 2 } + ] + }; + + var src = { container: null }; + + var expected = { container: null }; + + assert(dest, src, expected); + }); + }); }); From 9de2f772e537c7e4fe9cd87458369804ac88a25c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Thu, 10 Nov 2016 18:20:20 -0500 Subject: [PATCH 09/10] make annotation/shapes cleanLayout blocks more robust - so that we *only* loop over arrays. --- src/plot_api/helpers.js | 4 ++-- test/jasmine/tests/plot_api_test.js | 32 ++++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/src/plot_api/helpers.js b/src/plot_api/helpers.js index 31a4010c536..4d432cea9d6 100644 --- a/src/plot_api/helpers.js +++ b/src/plot_api/helpers.js @@ -102,7 +102,7 @@ exports.cleanLayout = function(layout) { } } - var annotationsLen = (layout.annotations || []).length; + var annotationsLen = Array.isArray(layout.annotations) ? layout.annotations.length : 0; for(i = 0; i < annotationsLen; i++) { var ann = layout.annotations[i]; @@ -124,7 +124,7 @@ exports.cleanLayout = function(layout) { cleanAxRef(ann, 'yref'); } - var shapesLen = (layout.shapes || []).length; + var shapesLen = Array.isArray(layout.shapes) ? layout.shapes.length : 0; for(i = 0; i < shapesLen; i++) { var shape = layout.shapes[i]; diff --git a/test/jasmine/tests/plot_api_test.js b/test/jasmine/tests/plot_api_test.js index c17d296ee02..c4d99ac8846 100644 --- a/test/jasmine/tests/plot_api_test.js +++ b/test/jasmine/tests/plot_api_test.js @@ -896,7 +896,7 @@ describe('Test plot api', function() { }); }); - describe('cleanData', function() { + describe('cleanData & cleanLayout', function() { var gd; beforeEach(function() { @@ -1039,6 +1039,36 @@ describe('Test plot api', function() { expect(trace1.transforms.length).toEqual(1); expect(trace1.transforms[0].target).toEqual('y'); }); + + it('should cleanup annotations / shapes refs', function() { + var data = [{}]; + + var layout = { + annotations: [ + { ref: 'paper' }, + null, + { xref: 'x02', yref: 'y1' } + ], + shapes: [ + { xref: 'y', yref: 'x' }, + null, + { xref: 'x03', yref: 'y1' } + ] + }; + + Plotly.plot(gd, data, layout); + + expect(gd.layout.annotations[0]).toEqual({ xref: 'paper', yref: 'paper' }); + expect(gd.layout.annotations[1]).toEqual(null); + expect(gd.layout.annotations[2]).toEqual({ xref: 'x2', yref: 'y' }); + + expect(gd.layout.shapes[0].xref).toBeUndefined(); + expect(gd.layout.shapes[0].yref).toBeUndefined(); + expect(gd.layout.shapes[1]).toEqual(null); + expect(gd.layout.shapes[2].xref).toEqual('x3'); + expect(gd.layout.shapes[2].yref).toEqual('y'); + + }); }); describe('Plotly.newPlot', function() { From 5de6ff02108d48c0676782aee70f05bf3684929c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Thu, 10 Nov 2016 18:28:07 -0500 Subject: [PATCH 10/10] split container opts from item opts in handleArrayContainerDefaults --- .../annotations/annotation_defaults.js | 5 +++-- src/components/shapes/shape_defaults.js | 5 +++-- src/plots/array_container_defaults.js | 22 ++++++++++--------- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/components/annotations/annotation_defaults.js b/src/components/annotations/annotation_defaults.js index 4260cd5bcd1..ecdb1edf547 100644 --- a/src/components/annotations/annotation_defaults.js +++ b/src/components/annotations/annotation_defaults.js @@ -16,14 +16,15 @@ var Axes = require('../../plots/cartesian/axes'); var attributes = require('./attributes'); -module.exports = function handleAnnotationDefaults(annIn, annOut, fullLayout, opts) { +module.exports = function handleAnnotationDefaults(annIn, annOut, fullLayout, opts, itemOpts) { opts = opts || {}; + itemOpts = itemOpts || {}; function coerce(attr, dflt) { return Lib.coerce(annIn, annOut, attributes, attr, dflt); } - var visible = coerce('visible', !opts.itemIsNotPlainObject); + var visible = coerce('visible', !itemOpts.itemIsNotPlainObject); if(!visible) return annOut; diff --git a/src/components/shapes/shape_defaults.js b/src/components/shapes/shape_defaults.js index 45cc7ffec4c..15942086e1a 100644 --- a/src/components/shapes/shape_defaults.js +++ b/src/components/shapes/shape_defaults.js @@ -16,14 +16,15 @@ var attributes = require('./attributes'); var helpers = require('./helpers'); -module.exports = function handleShapeDefaults(shapeIn, shapeOut, fullLayout, opts) { +module.exports = function handleShapeDefaults(shapeIn, shapeOut, fullLayout, opts, itemOpts) { opts = opts || {}; + itemOpts = itemOpts || {}; function coerce(attr, dflt) { return Lib.coerce(shapeIn, shapeOut, attributes, attr, dflt); } - var visible = coerce('visible', !opts.itemIsNotPlainObject); + var visible = coerce('visible', !itemOpts.itemIsNotPlainObject); if(!visible) return shapeOut; diff --git a/src/plots/array_container_defaults.js b/src/plots/array_container_defaults.js index 8e6476e4937..25263c45acf 100644 --- a/src/plots/array_container_defaults.js +++ b/src/plots/array_container_defaults.js @@ -26,16 +26,20 @@ var Lib = require('../lib'); * - name {string} * name of the key linking the container in question * - handleItemDefaults {function} - * defaults method to be called on each item in the array container in question, + * defaults method to be called on each item in the array container in question * + * Its arguments are: + * - itemIn {object} item in user layout + * - itemOut {object} item in full layout + * - parentObj {object} (as in closure) + * - opts {object} (as in closure) + * - itemOpts {object} + * - itemIsNotPlainObject {boolean} * N.B. * * - opts is passed to handleItemDefaults so it can also store * links to supplementary data (e.g. fullData for layout components) * - * - opts.itemIsNotPlainObject is mutated on every pass in case so logic - * in handleItemDefaults relies on that fact. - * */ module.exports = function handleArrayContainerDefaults(parentObjIn, parentObjOut, opts) { var name = opts.name; @@ -45,17 +49,15 @@ module.exports = function handleArrayContainerDefaults(parentObjIn, parentObjOut for(var i = 0; i < contIn.length; i++) { var itemIn = contIn[i], - itemOut = {}; + itemOut = {}, + itemOpts = {}; if(!Lib.isPlainObject(itemIn)) { - opts.itemIsNotPlainObject = true; + itemOpts.itemIsNotPlainObject = true; itemIn = {}; } - else { - opts.itemIsNotPlainObject = false; - } - opts.handleItemDefaults(itemIn, itemOut, parentObjOut, opts); + opts.handleItemDefaults(itemIn, itemOut, parentObjOut, opts, itemOpts); itemOut._input = itemIn; itemOut._index = i;