Skip to content

Array edits #1403

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

Merged
merged 18 commits into from
Feb 24, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ lib.isPlainObject = require('./is_plain_object');
lib.isArray = require('./is_array');
lib.mod = require('./mod');
lib.toLogRange = require('./to_log_range');
lib.relinkPrivateKeys = require('./relink_private');

var coerceModule = require('./coerce');
lib.valObjects = coerceModule.valObjects;
Expand Down
55 changes: 55 additions & 0 deletions src/lib/relink_private.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/**
* Copyright 2012-2017, 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 isArray = require('./is_array');
var isPlainObject = require('./is_plain_object');

/**
* Relink private _keys and keys with a function value from one container
* to the new container.
* Relink means copying if object is pass-by-value and adding a reference
* if object is pass-by-ref.
* This prevents deepCopying massive structures like a webgl context.
*/
module.exports = function relinkPrivateKeys(toContainer, fromContainer) {
var keys = Object.keys(fromContainer || {});

for(var i = 0; i < keys.length; i++) {
var k = keys[i],
fromVal = fromContainer[k],
toVal = toContainer[k];

if(k.charAt(0) === '_' || typeof fromVal === 'function') {

// if it already exists at this point, it's something
// that we recreate each time around, so ignore it
if(k in toContainer) continue;

toContainer[k] = fromVal;
}
else if(isArray(fromVal) && isArray(toVal) && isPlainObject(fromVal[0])) {

// recurse into arrays containers
for(var j = 0; j < fromVal.length; j++) {
if(isPlainObject(fromVal[j]) && isPlainObject(toVal[j])) {
relinkPrivateKeys(toVal[j], fromVal[j]);
}
}
}
else if(isPlainObject(fromVal) && isPlainObject(toVal)) {

// recurse into objects, but only if they still exist
relinkPrivateKeys(toVal, fromVal);

if(!Object.keys(toVal).length) delete toContainer[k];
}
}
};
3 changes: 0 additions & 3 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -2092,9 +2092,6 @@ function _relayout(gd, aobj) {
var oldWidth = gd._fullLayout.width,
oldHeight = gd._fullLayout.height;

// coerce the updated layout
Plots.supplyDefaults(gd);

// calculate autosizing
if(gd.layout.autosize) Plots.plotAutoSize(gd, gd.layout, gd._fullLayout);
Copy link
Contributor

@etpinard etpinard Feb 22, 2017

Choose a reason for hiding this comment

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

Really? This doesn't break the autosize teats? I thought I tried removing it at some point a few weeks back.

Oh well. One less supply defaults loop ia great news 🐎

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this is ridiculously complicated... actually tracing through it I found a very specific new bug, though not sure yet if it's exactly related. But I think we can sort it out without reinstating that redundant supplyDefaults:

All of the pathways after _relayout in the relayout, and update wrappers include full supplyDefaults - with the exception of relayout when the edits object aobj has had everything stripped out of it and executed directly within _relayout - and THAT can happen if all keys are handled by editContainerArray, which is OK because inside editContainerArray we call supplyDefaults for that particular component array. This won't work for the regex arrays, because the array name doesn't match a component, but then we don't find a draw method either so it reverts to a full replot.

So I think the only potential pitfall of this is if a change inside the array affects something in the layout outside the array. One case I thought might fit this bill is if you provide a new axis id in a component object (new or edited) - but actually out now as previously we don't generate new axes just from component references, so:

Plotly.relayout(gd,{'annotations[1]':{text:'boo',x:-1,y:4, xref:'x2', yref:'y2'}})

on a plot with only xaxis and yaxis will get drawn on the existing axes... but the bug is that in this case the axes don't get correctly autoranged until you redraw the plot. It's ONLY this case, autorange works if your axis refs are valid.

So, are there any cases I'm not thinking of where changes inside one of these arrays affect _fullLayout outside of the array?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, are there any cases I'm not thinking of where changes inside one of these arrays affect _fullLayout outside of the array?

I can't think of any other cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed that bad-reference bug in 9f89dd6 - it was unrelated to removing this supplyDefaults, which I still believe is OK to do.


Expand Down
18 changes: 15 additions & 3 deletions src/plots/array_container_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,13 @@ var Lib = require('../lib');
module.exports = function handleArrayContainerDefaults(parentObjIn, parentObjOut, opts) {
var name = opts.name;

var contIn = Array.isArray(parentObjIn[name]) ? parentObjIn[name] : [],
contOut = parentObjOut[name] = [];
var previousContOut = parentObjOut[name];

for(var i = 0; i < contIn.length; i++) {
var contIn = Lib.isArray(parentObjIn[name]) ? parentObjIn[name] : [],
contOut = parentObjOut[name] = [],
i;

for(i = 0; i < contIn.length; i++) {
var itemIn = contIn[i],
itemOut = {},
itemOpts = {};
Expand All @@ -64,4 +67,13 @@ module.exports = function handleArrayContainerDefaults(parentObjIn, parentObjOut

contOut.push(itemOut);
}

// in case this array gets its defaults rebuilt independent of the whole layout,
// relink the private keys just for this array.
if(Lib.isArray(previousContOut)) {
var len = Math.min(previousContOut.length, contOut.length);
for(i = 0; i < len; i++) {
Lib.relinkPrivateKeys(contOut[i], previousContOut[i]);
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rreusser here's the fix for the command test. However if you add or delete an object, the private key will get reattached to the wrong item, or lost. I'm not sure what to do about this... I could splice the fullLayout array in sync with the layout array, which would keep the private key with the right index, but I don't know what to do when I delete an element that has private keys. Most of the time it doesn't matter, but in the case of ._remove I guess it would.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Hmm… scrambling at the moment for a call this afternoon, but let me think about this and process a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rreusser can you confirm that is working as it should?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

70c882b keeps the private keys in sync to the degree that it can - which is unless you delete elements from the array. Then they get lost. @rreusser as this isn't new behavior are you OK with it as it stands now?

Copy link
Contributor

Choose a reason for hiding this comment

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

My benchmark for success is just whether it passes the component test that was failing due to the ._remove property getting lost. It was originally just called .remove which would still get lost, so I'll check and submit that fix as a separate PR as needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, no, I must be mistaken. It passes the test in its current form so the changes must have fixed it. Again, that was my only metric for this, so I'm satisfied although I wouldn't have expected fixing private keys to fix the problem. Hmmm.

};
47 changes: 2 additions & 45 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ var plots = module.exports = {};
var animationAttrs = require('./animation_attributes');
var frameAttrs = require('./frame_attributes');

var relinkPrivateKeys = Lib.relinkPrivateKeys;

// Expose registry methods on Plots for backward-compatibility
Lib.extendFlat(plots, Registry);

Expand Down Expand Up @@ -592,51 +594,6 @@ plots.cleanPlot = function(newFullData, newFullLayout, oldFullData, oldFullLayou
}
};

/**
* Relink private _keys and keys with a function value from one container
* to the new container.
* Relink means copying if object is pass-by-value and adding a reference
* if object is pass-by-ref.
* This prevents deepCopying massive structures like a webgl context.
*/
function relinkPrivateKeys(toContainer, fromContainer) {
var isPlainObject = Lib.isPlainObject,
isArray = Array.isArray;

var keys = Object.keys(fromContainer || {});

for(var i = 0; i < keys.length; i++) {
var k = keys[i],
fromVal = fromContainer[k],
toVal = toContainer[k];

if(k.charAt(0) === '_' || typeof fromVal === 'function') {

// if it already exists at this point, it's something
// that we recreate each time around, so ignore it
if(k in toContainer) continue;

toContainer[k] = fromVal;
}
else if(isArray(fromVal) && isArray(toVal) && isPlainObject(fromVal[0])) {

// recurse into arrays containers
for(var j = 0; j < fromVal.length; j++) {
if(isPlainObject(fromVal[j]) && isPlainObject(toVal[j])) {
relinkPrivateKeys(toVal[j], fromVal[j]);
}
}
}
else if(isPlainObject(fromVal) && isPlainObject(toVal)) {

// recurse into objects, but only if they still exist
relinkPrivateKeys(toVal, fromVal);

if(!Object.keys(toVal).length) delete toContainer[k];
}
}
}

plots.linkSubplots = function(newFullData, newFullLayout, oldFullData, oldFullLayout) {
var oldSubplots = oldFullLayout._plots || {},
newSubplots = newFullLayout._plots = {};
Expand Down