Merge pull request #3766 from plotly/filter-enabled-false-when-empty-target-array

etpinard · web-flow · commit d6da48e69e67 · 2019-05-08T16:54:28.000-04:00
Set enabled:false when filter target array is empty
diff --git 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
@@ -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
@@ -642,7 +642,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/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/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
@@ -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
@@ -2749,17 +2749,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
@@ -2786,9 +2782,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;
@@ -2797,7 +2790,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,
@@ -2833,6 +2826,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/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
@@ -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/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/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('<path d="M0,0Z" style="vector-effect: non-scaling-stroke;"></path>');
+            expect(gd.calcdata[0][0].isBlank).toBe(undefined);
         })
         .catch(failTest)
         .then(done);
diff --git 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() {
@@ -1292,3 +1319,69 @@ describe('filter transforms interactions', function() {
         .then(done);
     });
 });
+
+describe('filter resulting in empty coordinate arrays', function() {
+    var gd;
+
+    afterEach(function(done) {
+        Plotly.purge(gd);
+        setTimeout(function() {
+            destroyGraphDiv();
+            done();
+        }, 200);
+    });
+
+    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) {
+            it(d[0], function(done) {
+                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) {
+                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) {
+                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
@@ -976,7 +976,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',
@@ -993,14 +993,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('<path d="M0,0Z" style="vector-effect: non-scaling-stroke;"></path>');
+            expect(gd.calcdata[0][0].isBlank).toBe(undefined);
         })
         .catch(failTest)
         .then(done);

Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,8 @@ function baseFilter(item) {`
`32`	`32`	`}`
`33`	`33`
`34`	`34`	`function calcDataFilter(item) {`
`35`		`- return item[0].trace.visible === true;`
	`35`	`+ var trace = item[0].trace;`
	`36`	`+ return trace.visible === true && trace._length !== 0;`
`36`	`37`	`}`
`37`	`38`
`38`	`39`	`function isCalcData(cont) {`
Original file line number	Diff line number	Diff line change
`@@ -403,7 +403,7 @@ module.exports = function setConvert(ax, fullLayout) {`
`403`	`403`	`return;`
`404`	`404`	`}`
`405`	`405`
`406`		`- if(ax.type === 'date') {`
	`406`	`+ if(ax.type === 'date' && !ax.autorange) {`
`407`	`407`	`// check if milliseconds or js date objects are provided for range`
`408`	`408`	`// and convert to date strings`
`409`	`409`	`range[0] = Lib.cleanDate(range[0], BADNUM, ax.calendar);`
Original file line number	Diff line number	Diff line change
`@@ -502,7 +502,7 @@ proto.plot = function(sceneData, fullLayout, layout) {`
`502`	`502`
`503`	`503`	`for(i = 0; i < sceneData.length; ++i) {`
`504`	`504`	`data = sceneData[i];`
`505`		`- if(data.visible !== true) continue;`
	`505`	`+ if(data.visible !== true \|\| data._length === 0) continue;`
`506`	`506`
`507`	`507`	`computeTraceBounds(this, data, dataBounds);`
`508`	`508`	`}`
`@@ -526,7 +526,7 @@ proto.plot = function(sceneData, fullLayout, layout) {`
`526`	`526`	`// Update traces`
`527`	`527`	`for(i = 0; i < sceneData.length; ++i) {`
`528`	`528`	`data = sceneData[i];`
`529`		`- if(data.visible !== true) {`
	`529`	`+ if(data.visible !== true \|\| data._length === 0) {`
`530`	`530`	`continue;`
`531`	`531`	`}`
`532`	`532`	`trace = this.traces[data.uid];`
`@@ -551,7 +551,8 @@ proto.plot = function(sceneData, fullLayout, layout) {`
`551`	`551`	`traceIdLoop:`
`552`	`552`	`for(i = 0; i < traceIds.length; ++i) {`
`553`	`553`	`for(j = 0; j < sceneData.length; ++j) {`
`554`		`- if(sceneData[j].uid === traceIds[i] && sceneData[j].visible === true) {`
	`554`	`+ if(sceneData[j].uid === traceIds[i] &&`
	`555`	`+ (sceneData[j].visible === true && sceneData[j]._length !== 0)) {`
`555`	`556`	`continue traceIdLoop;`
`556`	`557`	`}`
`557`	`558`	`}`