-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Array edits #1403
Changes from 1 commit
cc45189
13a87ce
61f250c
a9526bf
649a831
d55568c
2e6c030
f7e60fb
d7cdc0a
7ed9eb9
9f89dd6
46962af
1af058a
c6378b1
70c882b
5fb462f
d29cbae
3ef1c9b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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]; | ||
} | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = {}; | ||
|
@@ -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]); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rreusser can you confirm that is working as it should? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
}; |
There was a problem hiding this comment.
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 🐎
There was a problem hiding this comment.
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 therelayout
, andupdate
wrappers include fullsupplyDefaults
- with the exception ofrelayout
when the edits objectaobj
has had everything stripped out of it and executed directly within_relayout
- and THAT can happen if all keys are handled byeditContainerArray
, which is OK because insideeditContainerArray
we callsupplyDefaults
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 adraw
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
andyaxis
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of any other cases.
There was a problem hiding this comment.
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.