Skip to content

Axes ref cleanup#1431

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.

Sign up for GitHub

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

Merged
etpinard merged 10 commits into from
Mar 2, 2017
Merged

Axes ref cleanup #1431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1700908
add extra karma config
etpinard Feb 28, 2017
11073f2
replace for-each by for-loop in cartesian layout defaults
etpinard Feb 28, 2017
2289657
rm _gd ref in fullLayout axis containers
etpinard Mar 1, 2017
6a271ed
call setConvert on 3d axis during Scene.plot
etpinard Mar 1, 2017
704596a
require axis_ids instead of Axes in cartesian index
etpinard Mar 1, 2017
045da99
improve comment for setScale in lsInner
etpinard Mar 1, 2017
50d1288
Merge branch 'master' into axes-ref-cleanup
etpinard Mar 2, 2017
5eb6202
replace .map with for-loops
etpinard Mar 2, 2017
6e25640
link fullLayout.separators to ax._separators
etpinard Mar 2, 2017
9612549
replace gd._replotting -> fullLayout._replotting
etpinard Mar 2, 2017
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
3 changes: 1 addition & 2 deletions src/components/colorbar/draw.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -185,11 +185,10 @@ module.exports = function draw(gd, id) {
}

// Prepare the Plotly axis object
handleAxisDefaults(cbAxisIn, cbAxisOut, coerce, axisOptions);
handleAxisDefaults(cbAxisIn, cbAxisOut, coerce, axisOptions, fullLayout);
handleAxisPositionDefaults(cbAxisIn, cbAxisOut, coerce, axisOptions);

cbAxisOut._id = 'y' + id;
cbAxisOut._gd = gd;

// position can't go in through supplyDefaults
// because that restricts it to [0,1]
Expand Down
9 changes: 5 additions & 4 deletions src/plot_api/plot_api.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -133,12 +133,14 @@ Plotly.plot = function(gd, data, layout, config) {

Plots.supplyDefaults(gd);

var fullLayout = gd._fullLayout;

// Polar plots
if(data && data[0] && data[0].r) return plotPolar(gd, data, layout);

// so we don't try to re-call Plotly.plot from inside
// legend and colorbar, if margins changed
gd._replotting = true;
fullLayout._replotting = true;

// make or remake the framework if we need to
if(graphWasEmpty) makePlotFramework(gd);
Expand All@@ -152,7 +154,6 @@ Plotly.plot = function(gd, data, layout, config) {
// save initial axis range once per graph
if(graphWasEmpty) Plotly.Axes.saveRangeInitial(gd);

var fullLayout = gd._fullLayout;

// prepare the data and find the autorange

Expand DownExpand Up@@ -321,13 +322,13 @@ Plotly.plot = function(gd, data, layout, config) {
Plots.addLinks(gd);

// Mark the first render as complete
gd._replotting = false;
fullLayout._replotting = false;

return Plots.previousPromises(gd);
}

// An initial paint must be completed before these components can be
// correctly sized and the whole plot re-margined. gd._replotting must
// correctly sized and the whole plot re-margined. fullLayout._replotting must
// be set to false before these will work properly.
function finalDraw() {
Registry.getComponentMethod('shapes', 'draw')(gd);
Expand Down
3 changes: 2 additions & 1 deletion src/plot_api/subroutines.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -49,7 +49,8 @@ exports.lsInner = function(gd) {
xa = Plotly.Axes.getFromId(gd, subplot, 'x'),
ya = Plotly.Axes.getFromId(gd, subplot, 'y');

xa.setScale(); // this may already be done... not sure
// reset scale in case the margins have changed
xa.setScale();
ya.setScale();

if(plotinfo.bg) {
Expand Down
14 changes: 5 additions & 9 deletions src/plots/cartesian/axes.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -328,14 +328,10 @@ axes.doAutoRange = function(ax) {

// doAutoRange will get called on fullLayout,
// but we want to report its results back to layout
var axIn = ax._gd.layout[ax._name];

if(!axIn) ax._gd.layout[ax._name] = axIn = {};

if(axIn !== ax) {
axIn.range = ax.range.slice();
axIn.autorange = ax.autorange;
}
var axIn = ax._input;
axIn.range = ax.range.slice();
axIn.autorange = ax.autorange;

@etpinardetpinardMar 1, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Now, doAutorange writes in ax._input instead of having dig in _gd.layout ... 🎉

}
};

Expand DownExpand Up@@ -1137,7 +1133,7 @@ axes.tickText = function(ax, x, hover) {
};

function tickTextObj(ax, x, text) {
var tf = ax.tickfont || ax._gd._fullLayout.font;
var tf = ax.tickfont || {};

@etpinardetpinardMar 1, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oh. I need to double-check this.

Removing the || {} should do the trick - as axis tickfont already default to layout font, but removing || {} makes the fonts.json mock fail. Not sure why.

@etpinardetpinardMar 2, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

After investigation, I'll keep the || {} fallback, if ok.

Whenever ax.showticklabels: false, ax.tickfont isn't coerced and isn't defined here.

We could make sure that tickTextObj isn't called when ax.showticklabels: false, but this requires adding a few early returns in axes.js, 3d and gl2d convert routines which I believe isn't worth it.

@alexcjohnsonalexcjohnsonMar 2, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍


return {
x: x,
Expand DownExpand Up@@ -1347,7 +1343,7 @@ function numFormat(v, ax, fmtoverride, hover) {
if(dp) v = v.substr(0, dp + tickRound).replace(/\.?0+$/, '');
}
// insert appropriate decimal point and thousands separator
v = Lib.numSeparate(v, ax._gd._fullLayout.separators, separatethousands);
v = Lib.numSeparate(v, ax._separators, separatethousands);
}

// add exponent
Expand Down
4 changes: 2 additions & 2 deletions src/plots/cartesian/axis_defaults.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -39,7 +39,7 @@ var autoType = require('./axis_autotype');
* data: the plot data to use in choosing auto type
* bgColor: the plot background color, to calculate default gridline colors
*/
module.exports = function handleAxisDefaults(containerIn, containerOut, coerce, options) {
module.exports = function handleAxisDefaults(containerIn, containerOut, coerce, options, layoutOut) {
var letter = options.letter,
font = options.font || {},
defaultTitle = 'Click to enter ' +
Expand DownExpand Up@@ -78,7 +78,7 @@ module.exports = function handleAxisDefaults(containerIn, containerOut, coerce,
handleCalendarDefaults(containerIn, containerOut, 'calendar', options.calendar);
}

setConvert(containerOut);
setConvert(containerOut, layoutOut);

var dfltColor = coerce('color');
// if axis.color was provided, use it for fonts too; otherwise,
Expand Down
9 changes: 5 additions & 4 deletions src/plots/cartesian/index.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -12,7 +12,8 @@
var d3 = require('d3');
var Lib = require('../../lib');
var Plots = require('../plots');
var Axes = require('./axes');

var axisIds = require('./axis_ids');
var constants = require('./constants');

exports.name = 'cartesian';
Expand DownExpand Up@@ -166,7 +167,7 @@ exports.clean = function(newFullData, newFullLayout, oldFullData, oldFullLayout)

if(hadCartesian && !hasCartesian) {
var subplotLayers = oldFullLayout._cartesianlayer.selectAll('.subplot');
var axIds = Axes.listIds({ _fullLayout: oldFullLayout });
var axIds = axisIds.listIds({ _fullLayout: oldFullLayout });

subplotLayers.call(purgeSubplotLayers, oldFullLayout);
oldFullLayout._defs.selectAll('.axesclip').remove();
Expand DownExpand Up@@ -241,13 +242,13 @@ function makeSubplotData(gd) {
// dimension that this one overlays to be an overlaid subplot,
// the main plot must exist make sure we're not trying to
// overlay on an axis that's already overlaying another
var xa2 = Axes.getFromId(gd, xa.overlaying) || xa;
var xa2 = axisIds.getFromId(gd, xa.overlaying) || xa;
if(xa2 !== xa && xa2.overlaying) {
xa2 = xa;
xa.overlaying = false;
}

var ya2 = Axes.getFromId(gd, ya.overlaying) || ya;
var ya2 = axisIds.getFromId(gd, ya.overlaying) || ya;
if(ya2 !== ya && ya2.overlaying) {
ya2 = ya;
ya.overlaying = false;
Expand Down
63 changes: 41 additions & 22 deletions src/plots/cartesian/layout_defaults.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -115,17 +115,43 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {

var bgColor = Color.combine(plot_bgcolor, layoutOut.paper_bgcolor);

var axLayoutIn, axLayoutOut;
var axName, axLayoutIn, axLayoutOut;

function coerce(attr, dflt) {
return Lib.coerce(axLayoutIn, axLayoutOut, layoutAttributes, attr, dflt);
}

axesList.forEach(function(axName) {
var axLetter = axName.charAt(0);
function getCounterAxes(axLetter) {

@etpinardetpinardMar 1, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

as promised in #1261 (comment)

@alexcjohnsonalexcjohnsonMar 2, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There are still twomaps and a filter 🐎

@etpinardetpinardMar 2, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done in 5eb6202

var list = {x: yaList, y: xaList}[axLetter];
return Lib.simpleMap(list, axisIds.name2id);
}

function getOverlayableAxes(axLetter, axName) {
var list = {x: xaList, y: yaList}[axLetter];
var out = [];

axLayoutIn = layoutIn[axName] || {};
axLayoutOut = {};
for(var j = 0; j < list.length; j++) {
var axName2 = list[j];

if(axName2 !== axName && !(layoutIn[axName2] || {}).overlaying) {
out.push(axisIds.name2id(axName2));
}
}

return out;
}

for(i = 0; i < axesList.length; i++) {
axName = axesList[i];

if(!Lib.isPlainObject(layoutIn[axName])) {
layoutIn[axName] = {};
}

axLayoutIn = layoutIn[axName];
axLayoutOut = layoutOut[axName] = {};

var axLetter = axName.charAt(0);

var defaultOptions = {
letter: axLetter,
Expand All@@ -142,29 +168,21 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {

var positioningOptions = {
letter: axLetter,
counterAxes: {x: yaList, y: xaList}[axLetter].map(axisIds.name2id),
overlayableAxes: {x: xaList, y: yaList}[axLetter].filter(function(axName2) {
return axName2 !== axName && !(layoutIn[axName2] || {}).overlaying;
}).map(axisIds.name2id)
counterAxes: getCounterAxes(axLetter),
overlayableAxes: getOverlayableAxes(axLetter, axName)
};

handlePositionDefaults(axLayoutIn, axLayoutOut, coerce, positioningOptions);

layoutOut[axName] = axLayoutOut;

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

});
axLayoutOut._input = axLayoutIn;
}

// quick second pass for range slider and selector defaults
var rangeSliderDefaults = Registry.getComponentMethod('rangeslider', 'handleDefaults'),
rangeSelectorDefaults = Registry.getComponentMethod('rangeselector', 'handleDefaults');

xaList.forEach(function(axName) {
for(i = 0; i < xaList.length; i++) {
axName = xaList[i];
axLayoutIn = layoutIn[axName];
axLayoutOut = layoutOut[axName];

Expand All@@ -181,9 +199,10 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
}

coerce('fixedrange');
});
}

yaList.forEach(function(axName) {
for(i = 0; i < yaList.length; i++) {
axName = yaList[i];
axLayoutIn = layoutIn[axName];
axLayoutOut = layoutOut[axName];

Expand All@@ -196,5 +215,5 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
);

coerce('fixedrange', fixedRangeDflt);
});
}
};
13 changes: 9 additions & 4 deletions src/plots/cartesian/set_convert.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -61,7 +61,8 @@ function num(v) {
* also clears the autorange bounds ._min and ._max
* and the autotick constraints ._minDtick, ._forceTick0
*/
module.exports = function setConvert(ax) {
module.exports = function setConvert(ax, fullLayout) {
fullLayout = fullLayout || {};

@etpinardetpinardMar 1, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Second, make setConvert take in fullLayout


// clipMult: how many axis lengths past the edge do we render?
// for panning, 1-2 would suffice, but for zooming more is nice.
Expand DownExpand Up@@ -319,7 +320,7 @@ module.exports = function setConvert(ax) {

// set scaling to pixels
ax.setScale = function(usePrivateRange) {
var gs = ax._gd._fullLayout._size,
var gs = fullLayout._size,

@etpinardetpinardMar 1, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

... so that setScale can look into _size.

axLetter = ax._id.charAt(0);

// TODO cleaner way to handle this case
Expand All@@ -328,7 +329,7 @@ module.exports = function setConvert(ax) {
// make sure we have a domain (pull it in from the axis
// this one is overlaying if necessary)
if(ax.overlaying) {
var ax2 = axisIds.getFromId(ax._gd, ax.overlaying);
var ax2 = axisIds.getFromId({ _fullLayout: fullLayout }, ax.overlaying);
ax.domain = ax2.domain;
}

Expand DownExpand Up@@ -360,7 +361,7 @@ module.exports = function setConvert(ax) {
Lib.notifier(
'Something went wrong with axis scaling',
'long');
ax._gd._replotting = false;
fullLayout._replotting = false;
throw new Error('axis scaling');
}
};
Expand DownExpand Up@@ -417,6 +418,10 @@ module.exports = function setConvert(ax) {
ax._min = [];
ax._max = [];

// copy ref to fullLayout.separators so that
// methods in Axes can use it w/o having to pass fullLayout
ax._separators = fullLayout.separators;

// and for bar charts and box plots: reset forced minimum tick spacing
delete ax._minDtick;
delete ax._forceTick0;
Expand Down
5 changes: 2 additions & 3 deletions src/plots/geo/geo.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -465,10 +465,9 @@ function createMockAxis(fullLayout) {
var mockAxis = {
type: 'linear',
showexponent: 'all',
exponentformat: Axes.layoutAttributes.exponentformat.dflt,
_gd: { _fullLayout: fullLayout }
exponentformat: Axes.layoutAttributes.exponentformat.dflt
};

Axes.setConvert(mockAxis);
Axes.setConvert(mockAxis, fullLayout);
return mockAxis;
}
15 changes: 0 additions & 15 deletions src/plots/gl3d/index.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -14,9 +14,6 @@ var Plots = require('../plots');
var Lib = require('../../lib');
var xmlnsNamespaces = require('../../constants/xmlns_namespaces');

var axesNames = ['xaxis', 'yaxis', 'zaxis'];


exports.name = 'gl3d';

exports.attr = 'scene';
Expand DownExpand Up@@ -45,8 +42,6 @@ exports.plot = function plotGl3d(gd) {
scene = sceneLayout._scene;

if(!scene) {
initAxes(gd, sceneLayout);

scene = new Scene({
id: sceneId,
graphDiv: gd,
Expand DownExpand Up@@ -118,13 +113,3 @@ exports.cleanId = function cleanId(id) {

return 'scene' + sceneNum;
};

exports.setConvert = require('./set_convert');

function initAxes(gd, sceneLayout) {
for(var j = 0; j < 3; ++j) {
var axisName = axesNames[j];

sceneLayout[axisName]._gd = gd;
}
}
14 changes: 9 additions & 5 deletions src/plots/gl3d/scene.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -22,7 +22,6 @@ var showNoWebGlMsg = require('../../lib/show_no_webgl_msg');

var createCamera = require('./camera');
var project = require('./project');
var setConvert = require('./set_convert');
var createAxesOptions = require('./layout/convert');
var createSpikeOptions = require('./layout/spikes');
var computeTickMarks = require('./layout/tick_marks');
Expand DownExpand Up@@ -339,6 +338,7 @@ proto.plot = function(sceneData, fullLayout, layout) {
this.glplot.snapToData = true;

// Update layout
this.fullLayout = fullLayout;
this.fullSceneLayout = fullSceneLayout;

this.glplotLayout = fullSceneLayout;
Expand All@@ -353,10 +353,7 @@ proto.plot = function(sceneData, fullLayout, layout) {
this.glplot.update({});

// Update axes functions BEFORE updating traces
for(i = 0; i < 3; ++i) {
axis = fullSceneLayout[axisProperties[i]];
setConvert(axis);
}
this.setConvert(axis);

// Convert scene data
if(!sceneData) sceneData = [];
Expand DownExpand Up@@ -708,4 +705,11 @@ proto.toImage = function(format) {
return dataURL;
};

proto.setConvert = function() {
for(var i = 0; i < 3; ++i) {
var ax = this.fullSceneLayout[axisProperties[i]];
Axes.setConvert(ax, this.fullLayout);
}
};

@alexcjohnsonalexcjohnsonMar 2, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice simplification! Why don't we need the containerOut.setScale = Lib.noop; line anymore?

@etpinardetpinardMar 2, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Not anymore. 🔪

@etpinardetpinardMar 2, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

To be more precise, 3d doesn't use any of the _m and friends fields that setScale computes at the moment. So setting setScale to a noop might be a minor 🐎 boost. But, for 3d annotations, I'm planning on using / mocking those scale fields. To be continued.


module.exports = Scene;
Loading