From 66645ed6d8bbf4ae643e692d2b458e8a134bd6f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 12 Apr 2019 10:21:56 -0400 Subject: [PATCH 1/6] set enabled:false when filter target array is empty - see https://github.com/plotly/plotly.js/issues/2908 for more info, - this fixes many potential problems downstream --- src/transforms/filter.js | 8 +++++- test/jasmine/tests/transform_filter_test.js | 27 +++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/transforms/filter.js b/src/transforms/filter.js index 4e96fbd30e9..e23556ee1d1 100644 --- a/src/transforms/filter.js +++ b/src/transforms/filter.js @@ -138,10 +138,16 @@ exports.supplyDefaults = function(transformIn) { var enabled = coerce('enabled'); if(enabled) { + var target = coerce('target'); + + if(Lib.isArrayOrTypedArray(target) && target.length === 0) { + transformOut.enabled = false; + return transformOut; + } + coerce('preservegaps'); coerce('operation'); coerce('value'); - coerce('target'); var handleCalendarDefaults = Registry.getComponentMethod('calendars', 'handleDefaults'); handleCalendarDefaults(transformIn, transformOut, 'valuecalendar', null); diff --git a/test/jasmine/tests/transform_filter_test.js b/test/jasmine/tests/transform_filter_test.js index 9679e415841..65cfa23e7cd 100644 --- a/test/jasmine/tests/transform_filter_test.js +++ b/test/jasmine/tests/transform_filter_test.js @@ -86,6 +86,33 @@ describe('filter transforms defaults:', function() { expect(traceOut.transforms[2].target).toEqual('x'); expect(traceOut.transforms[3].target).toEqual('marker.color'); }); + + it('supplyTraceDefaults should set *enabled:false* and return early when *target* is an empty array', function() { + // see https://github.com/plotly/plotly.js/issues/2908 + // this solves multiple problems downstream + + traceIn = { + x: [1, 2, 3], + transforms: [{ + type: 'filter', + target: [] + }] + }; + traceOut = Plots.supplyTraceDefaults(traceIn, {type: 'scatter'}, 0, fullLayout); + expect(traceOut.transforms[0].target).toEqual([]); + expect(traceOut.transforms[0].enabled).toBe(false, 'set to false!'); + + traceIn = { + x: new Float32Array([1, 2, 3]), + transforms: [{ + type: 'filter', + target: new Float32Array() + }] + }; + traceOut = Plots.supplyTraceDefaults(traceIn, {type: 'scatter'}, 0, fullLayout); + expect(traceOut.transforms[0].target).toEqual(new Float32Array()); + expect(traceOut.transforms[0].enabled).toBe(false, 'set to false!'); + }); }); describe('filter transforms calc:', function() { From 3a5a4c2b2761dd232838b6c59109650a308a3077 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 26 Apr 2019 17:09:30 -0400 Subject: [PATCH 2/6] treat trace w/ _length===0 as if they were visible:false ... after transform _module.calc loop - that way filter transforms that remove all data coordinates don't result in errors - of all the mocks listed in mock_lists.js, only 'scattercarpet' and 'world-cals' still error out (wip) --- src/components/fx/hover.js | 4 +- src/lib/filter_visible.js | 3 +- src/plot_api/subroutines.js | 2 +- src/plots/get_data.js | 6 +- src/plots/gl3d/scene.js | 7 ++- src/plots/plots.js | 2 +- src/plots/polar/set_convert.js | 3 + src/traces/scattermapbox/convert.js | 2 +- test/jasmine/tests/transform_filter_test.js | 64 +++++++++++++++++++++ 9 files changed, 83 insertions(+), 10 deletions(-) diff --git a/src/components/fx/hover.js b/src/components/fx/hover.js index e312690a176..73cac1a5821 100644 --- a/src/components/fx/hover.js +++ b/src/components/fx/hover.js @@ -370,10 +370,12 @@ function _hover(gd, evt, subplot, noHoverEvent) { cd = searchData[curvenum]; // filter out invisible or broken data - if(!cd || !cd[0] || !cd[0].trace || cd[0].trace.visible !== true) continue; + if(!cd || !cd[0] || !cd[0].trace) continue; trace = cd[0].trace; + if(trace.visible !== true || trace._length === 0) continue; + // Explicitly bail out for these two. I don't know how to otherwise prevent // the rest of this function from running and failing if(['carpet', 'contourcarpet'].indexOf(trace._module.name) !== -1) continue; diff --git a/src/lib/filter_visible.js b/src/lib/filter_visible.js index 39088029478..7748237bd2a 100644 --- a/src/lib/filter_visible.js +++ b/src/lib/filter_visible.js @@ -32,7 +32,8 @@ function baseFilter(item) { } function calcDataFilter(item) { - return item[0].trace.visible === true; + var trace = item[0].trace; + return trace.visible === true && trace._length !== 0; } function isCalcData(cont) { diff --git a/src/plot_api/subroutines.js b/src/plot_api/subroutines.js index 8607065f898..bbd58945d6f 100644 --- a/src/plot_api/subroutines.js +++ b/src/plot_api/subroutines.js @@ -674,7 +674,7 @@ exports.redrawReglTraces = function(gd) { for(i = 0; i < fullData.length; i++) { var trace = fullData[i]; - if(trace.visible === true) { + if(trace.visible === true && trace._length !== 0) { if(trace.type === 'splom') { fullLayout._splomScenes[trace.uid].draw(); } else if(trace.type === 'scattergl') { diff --git a/src/plots/get_data.js b/src/plots/get_data.js index b543c2f75bb..4523b5f4434 100644 --- a/src/plots/get_data.js +++ b/src/plots/get_data.js @@ -69,8 +69,10 @@ exports.getModuleCalcData = function(calcdata, arg1) { for(var i = 0; i < calcdata.length; i++) { var cd = calcdata[i]; var trace = cd[0].trace; - // N.B. 'legendonly' traces do not make it past here - if(trace.visible !== true) continue; + // N.B. + // - 'legendonly' traces do not make it past here + // - skip over 'visible' traces that got trimmed completely during calc transforms + if(trace.visible !== true || trace._length === 0) continue; // group calcdata trace not by 'module' (as the name of this function // would suggest), but by 'module plot method' so that if some traces diff --git a/src/plots/gl3d/scene.js b/src/plots/gl3d/scene.js index b15ef824713..57a317e72c8 100644 --- a/src/plots/gl3d/scene.js +++ b/src/plots/gl3d/scene.js @@ -502,7 +502,7 @@ proto.plot = function(sceneData, fullLayout, layout) { for(i = 0; i < sceneData.length; ++i) { data = sceneData[i]; - if(data.visible !== true) continue; + if(data.visible !== true || data._length === 0) continue; computeTraceBounds(this, data, dataBounds); } @@ -526,7 +526,7 @@ proto.plot = function(sceneData, fullLayout, layout) { // Update traces for(i = 0; i < sceneData.length; ++i) { data = sceneData[i]; - if(data.visible !== true) { + if(data.visible !== true || data._length === 0) { continue; } trace = this.traces[data.uid]; @@ -551,7 +551,8 @@ proto.plot = function(sceneData, fullLayout, layout) { traceIdLoop: for(i = 0; i < traceIds.length; ++i) { for(j = 0; j < sceneData.length; ++j) { - if(sceneData[j].uid === traceIds[i] && sceneData[j].visible === true) { + if(sceneData[j].uid === traceIds[i] && + (sceneData[j].visible === true && sceneData[j]._length !== 0)) { continue traceIdLoop; } } diff --git a/src/plots/plots.js b/src/plots/plots.js index 3ac177d31bc..2d1a28fe438 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -2778,7 +2778,7 @@ plots.doCalcdata = function(gd, traces) { var cd = []; - if(trace.visible === true) { + if(trace.visible === true && trace._length !== 0) { // clear existing ref in case it got relinked delete trace._indexToPoints; // keep ref of index-to-points map object of the *last* enabled transform, diff --git a/src/plots/polar/set_convert.js b/src/plots/polar/set_convert.js index bbcc341d1da..d3fc465b5db 100644 --- a/src/plots/polar/set_convert.js +++ b/src/plots/polar/set_convert.js @@ -174,6 +174,9 @@ function setConvertAngular(ax, polarLayout) { var catLen = ax._categories.length; var _period = ax.period ? Math.max(ax.period, catLen) : catLen; + // fallback in case all categories have been filtered out + if(_period === 0) _period = 1; + c2rad = t2rad = function(v) { return v * 2 * Math.PI / _period; }; rad2c = rad2t = function(v) { return v * _period / Math.PI / 2; }; diff --git a/src/traces/scattermapbox/convert.js b/src/traces/scattermapbox/convert.js index 5dbc94ced6a..d92228f34be 100644 --- a/src/traces/scattermapbox/convert.js +++ b/src/traces/scattermapbox/convert.js @@ -23,7 +23,7 @@ var convertTextOpts = require('../../plots/mapbox/convert_text_opts'); module.exports = function convert(calcTrace) { var trace = calcTrace[0].trace; - var isVisible = (trace.visible === true); + var isVisible = (trace.visible === true && trace._length !== 0); var hasFill = (trace.fill !== 'none'); var hasLines = subTypes.hasLines(trace); var hasMarkers = subTypes.hasMarkers(trace); diff --git a/test/jasmine/tests/transform_filter_test.js b/test/jasmine/tests/transform_filter_test.js index 65cfa23e7cd..611a506a67a 100644 --- a/test/jasmine/tests/transform_filter_test.js +++ b/test/jasmine/tests/transform_filter_test.js @@ -1319,3 +1319,67 @@ describe('filter transforms interactions', function() { .then(done); }); }); + +describe('filter resulting in empty coordinate arrays', function() { + afterEach(destroyGraphDiv); + + function filter2empty(mock) { + var fig = Lib.extendDeep({}, mock); + var data = fig.data || []; + + data.forEach(function(trace) { + trace.transforms = [{ + type: 'filter', + target: [null] + }]; + }); + + return fig; + } + + describe('svg mocks', function() { + var mockList = require('../assets/mock_lists').svg; + + mockList.forEach(function(d) { + if(d[0] === 'scattercarpet' || d[0] === 'world-cals') { + // scattercarpet don't work with transforms + // world-cals mock complains during a Lib.cleanDate() + return; + } + + it(d[0], function(done) { + var gd = createGraphDiv(); + var fig = filter2empty(d[1]); + Plotly.newPlot(gd, fig).catch(failTest).then(done); + }); + }); + }); + + describe('gl mocks', function() { + var mockList = require('../assets/mock_lists').gl; + + mockList.forEach(function(d) { + it('@gl ' + d[0], function(done) { + var gd = createGraphDiv(); + var fig = filter2empty(d[1]); + Plotly.newPlot(gd, fig).catch(failTest).then(done); + }); + }); + }); + + describe('mapbox mocks', function() { + var mockList = require('../assets/mock_lists').mapbox; + + Plotly.setPlotConfig({ + mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN + }); + + mockList.forEach(function(d) { + it('@noCI ' + d[0], function(done) { + var gd = createGraphDiv(); + var fig = filter2empty(d[1]); + Plotly.newPlot(gd, fig).catch(failTest).then(done); + }); + }); + }); +}); From afad530ed4ca5066200ad04a847645d75b547784 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 26 Apr 2019 17:22:20 -0400 Subject: [PATCH 3/6] fixup tests post _length===0 commit --- test/jasmine/tests/transform_filter_test.js | 16 ++++++++++++---- test/jasmine/tests/waterfall_test.js | 9 +++------ 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/test/jasmine/tests/transform_filter_test.js b/test/jasmine/tests/transform_filter_test.js index 611a506a67a..272d32046ee 100644 --- a/test/jasmine/tests/transform_filter_test.js +++ b/test/jasmine/tests/transform_filter_test.js @@ -1321,7 +1321,15 @@ describe('filter transforms interactions', function() { }); describe('filter resulting in empty coordinate arrays', function() { - afterEach(destroyGraphDiv); + var gd; + + afterEach(function(done) { + Plotly.purge(gd); + setTimeout(function() { + destroyGraphDiv(); + done(); + }, 200); + }); function filter2empty(mock) { var fig = Lib.extendDeep({}, mock); @@ -1348,7 +1356,7 @@ describe('filter resulting in empty coordinate arrays', function() { } it(d[0], function(done) { - var gd = createGraphDiv(); + gd = createGraphDiv(); var fig = filter2empty(d[1]); Plotly.newPlot(gd, fig).catch(failTest).then(done); }); @@ -1360,7 +1368,7 @@ describe('filter resulting in empty coordinate arrays', function() { mockList.forEach(function(d) { it('@gl ' + d[0], function(done) { - var gd = createGraphDiv(); + gd = createGraphDiv(); var fig = filter2empty(d[1]); Plotly.newPlot(gd, fig).catch(failTest).then(done); }); @@ -1376,7 +1384,7 @@ describe('filter resulting in empty coordinate arrays', function() { mockList.forEach(function(d) { it('@noCI ' + d[0], function(done) { - var gd = createGraphDiv(); + gd = createGraphDiv(); var fig = filter2empty(d[1]); Plotly.newPlot(gd, fig).catch(failTest).then(done); }); diff --git a/test/jasmine/tests/waterfall_test.js b/test/jasmine/tests/waterfall_test.js index 534e33c9143..faad2a862f3 100644 --- a/test/jasmine/tests/waterfall_test.js +++ b/test/jasmine/tests/waterfall_test.js @@ -957,7 +957,7 @@ describe('A waterfall plot', function() { .then(done); }); - it('should be able to deal with blank bars on transform', function(done) { + it('should be able to deal with transform that empty out the data coordinate arrays', function(done) { Plotly.plot(gd, { data: [{ type: 'waterfall', @@ -974,14 +974,11 @@ describe('A waterfall plot', function() { }) .then(function() { var traceNodes = getAllTraceNodes(gd); - var waterfallNodes = getAllWaterfallNodes(traceNodes[0]); - var pathNode = waterfallNodes[0].querySelector('path'); + expect(traceNodes.length).toBe(0); expect(gd.calcdata[0][0].x).toEqual(NaN); expect(gd.calcdata[0][0].y).toEqual(NaN); - expect(gd.calcdata[0][0].isBlank).toBe(true); - - expect(pathNode.outerHTML).toEqual(''); + expect(gd.calcdata[0][0].isBlank).toBe(undefined); }) .catch(failTest) .then(done); From b6432380de4b3474b0bec9b2f02d2bd49e416f15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 26 Apr 2019 17:50:48 -0400 Subject: [PATCH 4/6] get filter transform to work with scattercarpet traces --- src/plots/plots.js | 21 ++++++++++++--------- test/jasmine/tests/transform_filter_test.js | 3 +-- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index 2d1a28fe438..bd8ed88eea7 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -2730,17 +2730,13 @@ plots.doCalcdata = function(gd, traces) { ); } - setupAxisCategories(axList, fullData); - var hasCalcTransform = false; - // transform loop - for(i = 0; i < fullData.length; i++) { + function transformCalci(i) { trace = fullData[i]; + _module = trace._module; if(trace.visible === true && trace.transforms) { - _module = trace._module; - // we need one round of trace module calc before // the calc transform to 'fill in' the categories list // used for example in the data-to-coordinate method @@ -2767,9 +2763,6 @@ plots.doCalcdata = function(gd, traces) { } } - // clear stuff that should recomputed in 'regular' loop - if(hasCalcTransform) setupAxisCategories(axList, fullData); - function calci(i, isContainer) { trace = fullData[i]; _module = trace._module; @@ -2814,6 +2807,16 @@ plots.doCalcdata = function(gd, traces) { calcdata[i] = cd; } + setupAxisCategories(axList, fullData); + + // 'transform' loop - must calc container traces first + // so that if their dependent traces can get transform properly + for(i = 0; i < fullData.length; i++) calci(i, true); + for(i = 0; i < fullData.length; i++) transformCalci(i); + + // clear stuff that should recomputed in 'regular' loop + if(hasCalcTransform) setupAxisCategories(axList, fullData); + // 'regular' loop - make sure container traces (eg carpet) calc before // contained traces (eg contourcarpet) for(i = 0; i < fullData.length; i++) calci(i, true); diff --git a/test/jasmine/tests/transform_filter_test.js b/test/jasmine/tests/transform_filter_test.js index 272d32046ee..4be0139cdda 100644 --- a/test/jasmine/tests/transform_filter_test.js +++ b/test/jasmine/tests/transform_filter_test.js @@ -1349,8 +1349,7 @@ describe('filter resulting in empty coordinate arrays', function() { var mockList = require('../assets/mock_lists').svg; mockList.forEach(function(d) { - if(d[0] === 'scattercarpet' || d[0] === 'world-cals') { - // scattercarpet don't work with transforms + if(d[0] === 'world-cals') { // world-cals mock complains during a Lib.cleanDate() return; } From 40c8abc10cb029d1300c6b0c696acd71299bb3c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 26 Apr 2019 17:59:50 -0400 Subject: [PATCH 5/6] don't try to cleanDate coming from autorange --- src/plots/cartesian/set_convert.js | 2 +- test/jasmine/tests/transform_filter_test.js | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/src/plots/cartesian/set_convert.js b/src/plots/cartesian/set_convert.js index 55bb4bdcb98..bf5c397de93 100644 --- a/src/plots/cartesian/set_convert.js +++ b/src/plots/cartesian/set_convert.js @@ -403,7 +403,7 @@ module.exports = function setConvert(ax, fullLayout) { return; } - if(ax.type === 'date') { + if(ax.type === 'date' && !ax.autorange) { // check if milliseconds or js date objects are provided for range // and convert to date strings range[0] = Lib.cleanDate(range[0], BADNUM, ax.calendar); diff --git a/test/jasmine/tests/transform_filter_test.js b/test/jasmine/tests/transform_filter_test.js index 4be0139cdda..9287a1a0a98 100644 --- a/test/jasmine/tests/transform_filter_test.js +++ b/test/jasmine/tests/transform_filter_test.js @@ -1349,11 +1349,6 @@ describe('filter resulting in empty coordinate arrays', function() { var mockList = require('../assets/mock_lists').svg; mockList.forEach(function(d) { - if(d[0] === 'world-cals') { - // world-cals mock complains during a Lib.cleanDate() - return; - } - it(d[0], function(done) { gd = createGraphDiv(); var fig = filter2empty(d[1]); From 84c3606648629ea1ba9fa14b8f39dcc0f9804312 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 8 May 2019 15:33:51 -0400 Subject: [PATCH 6/6] fixup funnel test for new _length===0 logic --- test/jasmine/tests/funnel_test.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/test/jasmine/tests/funnel_test.js b/test/jasmine/tests/funnel_test.js index 7ed7898dc35..1368f46efb6 100644 --- a/test/jasmine/tests/funnel_test.js +++ b/test/jasmine/tests/funnel_test.js @@ -1018,7 +1018,7 @@ describe('A funnel plot', function() { .then(done); }); - it('should be able to deal with blank bars on transform', function(done) { + it('should be able to deal with transform that empty out the data coordinate arrays', function(done) { Plotly.plot(gd, { data: [{ type: 'funnel', @@ -1038,14 +1038,11 @@ describe('A funnel plot', function() { }) .then(function() { var traceNodes = getAllTraceNodes(gd); - var funnelNodes = getAllFunnelNodes(traceNodes[0]); - var pathNode = funnelNodes[0].querySelector('path'); + expect(traceNodes.length).toBe(0); expect(gd.calcdata[0][0].x).toEqual(NaN); expect(gd.calcdata[0][0].y).toEqual(NaN); - expect(gd.calcdata[0][0].isBlank).toBe(true); - - expect(pathNode.outerHTML).toEqual(''); + expect(gd.calcdata[0][0].isBlank).toBe(undefined); }) .catch(failTest) .then(done);