Skip to content

Sanitize orphan subplot logic and 'last trace' deletion #268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,7 @@ function cleanLayout(layout) {
if(!layout.xaxis) layout.xaxis = layout.xaxis1;
delete layout.xaxis1;
}

if(layout.yaxis1) {
if(!layout.yaxis) layout.yaxis = layout.yaxis1;
delete layout.yaxis1;
Expand Down
6 changes: 2 additions & 4 deletions src/plots/cartesian/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,8 @@ exports.plot = function(gd) {
}

// finally do all error bars at once
if(fullLayout._hasCartesian) {
ErrorBars.plot(gd, subplotInfo, cdError);
Lib.markTime('done ErrorBars');
}
ErrorBars.plot(gd, subplotInfo, cdError);
Lib.markTime('done ErrorBars');
}

// now draw stuff not on subplots (ie, only pies at the moment)
Expand Down
83 changes: 58 additions & 25 deletions src/plots/cartesian/layout_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,28 +20,36 @@ var axisIds = require('./axis_ids');


module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
// get the full list of axes already defined
var layoutKeys = Object.keys(layoutIn),
xaList = [],
yaList = [],
xaListCartesian = [],
yaListCartesian = [],
xaListGl2d = [],
yaListGl2d = [],
outerTicks = {},
noGrids = {},
i;

for(i = 0; i < layoutKeys.length; i++) {
var key = layoutKeys[i];
if(constants.xAxisMatch.test(key)) xaList.push(key);
else if(constants.yAxisMatch.test(key)) yaList.push(key);
}

// look for axes in the data
for(i = 0; i < fullData.length; i++) {
var trace = fullData[i],
xaName = axisIds.id2name(trace.xaxis),
var trace = fullData[i];
var listX, listY;

if(Plots.traceIs(trace, 'cartesian')) {
listX = xaListCartesian;
listY = yaListCartesian;
}
else if(Plots.traceIs(trace, 'gl2d')) {
listX = xaListGl2d;
listY = yaListGl2d;
}
else continue;

var xaName = axisIds.id2name(trace.xaxis),
yaName = axisIds.id2name(trace.yaxis);

// add axes implied by traces
if(xaName && xaList.indexOf(xaName) === -1) xaList.push(xaName);
if(yaName && yaList.indexOf(yaName) === -1) yaList.push(yaName);
if(xaName && listX.indexOf(xaName) === -1) listX.push(xaName);
if(yaName && listY.indexOf(yaName) === -1) listY.push(yaName);

// check for default formatting tweaks
if(Plots.traceIs(trace, '2dMap')) {
Expand All @@ -55,22 +63,47 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
}
}

function axSort(a,b) {
var aNum = Number(a.substr(5)||1),
bNum = Number(b.substr(5)||1);
return aNum - bNum;
// N.B. Ignore orphan axes (i.e. axes that have no data attached to them)
// if gl3d or geo is present on graph. This is retain backward compatible.
//
// TODO drop this in version 2.0
var ignoreOrphan = (layoutOut._hasGL3D || layoutOut._hasGeo);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very important point, to preserve backward compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... because some old gl3d and geo plots have layout.xaxis and/or layout.yaxis in them, which would generate blank cartesian axes without the line above.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is presumably just an issue just for plots made and stored in the workspace right? Can we confirm that we're not generating this situation anymore? (Or perhaps we are until we properly handle type changes in restyle) And then how hard would it be to write a command to find and clear these orphans out of stored plots, so we never have to put this logic in? Otherwise I don't see how we're going to be able to take it out later.

I suppose we can put this logic in now, deploy it, then at some later time make and run a cleaning command, then come in after THAT and delete this logic... I just worry that if we don't do it now it will become harder later, and therefore will never happen, and this quirk will live forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an we confirm that we're not generating this situation anymore?

scatter3d traces are still generated with a layout.xaxis and layout.yaxis in the workspace.


if(!ignoreOrphan) {
for(i = 0; i < layoutKeys.length; i++) {
var key = layoutKeys[i];

// orphan layout axes are considered cartesian subplots

if(xaListGl2d.indexOf(key) === -1 &&
xaListCartesian.indexOf(key) === -1 &&
constants.xAxisMatch.test(key)) {
xaListCartesian.push(key);
}
else if(yaListGl2d.indexOf(key) === -1 &&
yaListCartesian.indexOf(key) === -1 &&
constants.yAxisMatch.test(key)) {
yaListCartesian.push(key);
}
}
}

if(layoutOut._hasCartesian || layoutOut._hasGL2D || !fullData.length) {
// make sure there's at least one of each and lists are sorted
if(!xaList.length) xaList = ['xaxis'];
else xaList.sort(axSort);
// make sure that plots with orphan cartesian axes
// are considered 'cartesian'
if(xaListCartesian.length && yaListCartesian.length) {
layoutOut._hasCartesian = true;
}

if(!yaList.length) yaList = ['yaxis'];
else yaList.sort(axSort);
function axSort(a, b) {
var aNum = Number(a.substr(5) || 1),
bNum = Number(b.substr(5) || 1);
return aNum - bNum;
}

xaList.concat(yaList).forEach(function(axName){
var xaList = xaListCartesian.concat(xaListGl2d).sort(axSort),
yaList = yaListCartesian.concat(yaListGl2d).sort(axSort);

xaList.concat(yaList).forEach(function(axName) {
var axLetter = axName.charAt(0),
axLayoutIn = layoutIn[axName] || {},
axLayoutOut = {},
Expand Down Expand Up @@ -100,7 +133,7 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {

// so we don't have to repeat autotype unnecessarily,
// copy an autotype back to layoutIn
if(!layoutIn[axName] && axLayoutIn.type!=='-') {
if(!layoutIn[axName] && axLayoutIn.type !== '-') {
layoutIn[axName] = {type: axLayoutIn.type};
}

Expand Down
38 changes: 31 additions & 7 deletions src/plots/geo/geo.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ function Geo(options, fullLayout) {

this.makeFramework();
this.updateFx(fullLayout.hovermode);

this.traceHash = {};
}

module.exports = Geo;
Expand Down Expand Up @@ -152,25 +154,47 @@ function filterData(dataIn) {
}

proto.onceTopojsonIsLoaded = function(geoData, geoLayout) {
var traceData = {};
var i;

this.drawLayout(geoLayout);

for(var i = 0; i < geoData.length; i++) {
var traceHashOld = this.traceHash;
var traceHash = {};

for(i = 0; i < geoData.length; i++) {
var trace = geoData[i];

traceData[trace.type] = traceData[trace.type] || [];
traceData[trace.type].push(trace);
traceHash[trace.type] = traceHash[trace.type] || [];
traceHash[trace.type].push(trace);
}

var traceKeys = Object.keys(traceData);
for(var j = 0; j < traceKeys.length; j++){
var moduleData = traceData[traceKeys[j]];
var moduleNamesOld = Object.keys(traceHashOld);
var moduleNames = Object.keys(traceHash);

// when a trace gets deleted, make sure that its module's
// plot method is called so that it is properly
// removed from the DOM.
for(i = 0; i < moduleNamesOld.length; i++) {
var moduleName = moduleNamesOld[i];

if(moduleNames.indexOf(moduleName) === -1) {
var fakeModule = traceHashOld[moduleName][0];
fakeModule.visible = false;
traceHash[moduleName] = [fakeModule];
}
}

moduleNames = Object.keys(traceHash);

for(i = 0; i < moduleNames.length; i++) {
var moduleData = traceHash[moduleNames[i]];
var _module = moduleData[0]._module;

_module.plot(this, filterData(moduleData), geoLayout);
}

this.traceHash = traceHash;

this.render();
};

Expand Down
13 changes: 13 additions & 0 deletions src/plots/geo/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,16 @@ exports.plot = function plotGeo(gd) {
geo.plot(fullGeoData, fullLayout, gd._promises);
}
};

exports.clean = function(newFullData, newFullLayout, oldFullData, oldFullLayout) {
var oldGeoKeys = Plots.getSubplotIds(oldFullLayout, 'geo');

for(var i = 0; i < oldGeoKeys.length; i++) {
var oldGeoKey = oldGeoKeys[i];
var oldGeo = oldFullLayout[oldGeoKey]._geo;

if(!newFullLayout[oldGeoKey] && !!oldGeo) {
oldGeo.geoDiv.remove();
}
}
};
12 changes: 10 additions & 2 deletions src/plots/geo/layout/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,12 @@ var supplyGeoAxisLayoutDefaults = require('./axis_defaults');


module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
var geos = Plots.getSubplotIdsInData(fullData, 'geo'),
var geos = Plots.findSubplotIds(fullData, layoutIn, 'geo'),
geosLength = geos.length;

if(geos.length) layoutOut._hasGeo = true;
else return;

var geoLayoutIn, geoLayoutOut;

function coerce(attr, dflt) {
Expand All @@ -28,7 +31,12 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {

for(var i = 0; i < geosLength; i++) {
var geo = geos[i];
geoLayoutIn = layoutIn[geo] || {};

// geo traces get a layout geo for free!
if(layoutIn[geo]) geoLayoutIn = layoutIn[geo];
else geoLayoutIn = layoutIn[geo] = {};

geoLayoutIn = layoutIn[geo];
geoLayoutOut = {};

coerce('domain.x');
Expand Down
12 changes: 12 additions & 0 deletions src/plots/gl3d/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,18 @@ exports.plot = function plotGl3d(gd) {
}
};

exports.clean = function(newFullData, newFullLayout, oldFullData, oldFullLayout) {
var oldSceneKeys = Plots.getSubplotIds(oldFullLayout, 'gl3d');

for(var i = 0; i < oldSceneKeys.length; i++) {
var oldSceneKey = oldSceneKeys[i];

if(!newFullLayout[oldSceneKey] && !!oldFullLayout[oldSceneKey]._scene) {
oldFullLayout[oldSceneKey]._scene.destroy();
}
}
};

// clean scene ids, 'scene1' -> 'scene'
exports.cleanId = function cleanId(id) {
if (!id.match(/^scene[0-9]*$/)) return;
Expand Down
10 changes: 5 additions & 5 deletions src/plots/gl3d/layout/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,11 @@ var supplyGl3dAxisLayoutDefaults = require('./axis_defaults');


module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
if(!layoutOut._hasGL3D) return;
var scenes = Plots.findSubplotIds(fullData, layoutIn, 'gl3d'),
scenesLength = scenes.length;

var scenes = Plots.getSubplotIdsInData(fullData, 'gl3d');

// Get number of scenes to compute default scene domain
var scenesLength = scenes.length;
if(scenes.length) layoutOut._hasGL3D = true;
else return;

var sceneLayoutIn, sceneLayoutOut;

Expand Down Expand Up @@ -59,6 +58,7 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
* attributes like aspectratio can be written back dynamically.
*/

// gl3d traces get a layout scene for free!
if(layoutIn[scene] !== undefined) sceneLayoutIn = layoutIn[scene];
else layoutIn[scene] = sceneLayoutIn = {};

Expand Down
Loading