Make frame with nulls clear items & array containers #1118
Conversation
| @@ -427,4 +427,89 @@ describe('Test Plots', function() { | |||
| expect(gd._transitioning).toBeUndefined(); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('extendLayout', function() { | |||
There was a problem hiding this comment.
@rreusser what do you think of ⏬
| containerProp.set(null); | ||
| Lib.nestedProperty(containerObj, containerPaths[i]).set(containerVal); | ||
|
|
||
| if(!containerVal) { |
There was a problem hiding this comment.
Working through the logic. Just a note that false, undefined, null, and 0 will all trigger this condition. This is fine because this could only reasonably be an object or not an object, right?
There was a problem hiding this comment.
This is fine because this could only reasonably be an object or not an object, right?
Correct. That's what I'm thinking.
Maybe we could be slightly more strict and make this condition if(containerVal === undefined) which would be fulfilled when { annotations: undefined } and { annotations: null } but not for other falsy values.
There was a problem hiding this comment.
I like the idea in general that null is equivalent to "deliberately unset" as opposed to undefined which means "happens to not be specified," but I'm not fully aware of the implications here. I think most people would expect to be able to use them more or less equivalently, but I can see the argument for requiring explicit null in order to unset.
| destContainer[j] = plots.extendObjectWithContainers(destContainer[j], srcContainer[j]); | ||
| var srcObj = srcContainer[j]; | ||
|
|
||
| if(srcObj === null) destContainer[j] = {}; |
There was a problem hiding this comment.
I like this. Could be null, but seems the most robust/backwards-compatible if it's at least an object.
There was a problem hiding this comment.
Glad we agree here.
There was a problem hiding this comment.
More generally, what should non-plain-object items in layout array containers be coerced to?
See current behavior here: http://codepen.io/etpinard/pen/xRGgwW?
I'd vote for completely skipping over non-plain-object items (i.e. non-plain-object items won't show up in fullLayout). Any objections? Some may interpret this change as backward incompatible. @alexcjohnson thoughts?
Currently, updatemenus and sliders skip over non-plain-object buttons and step items (see here and here).
There was a problem hiding this comment.
cc @bpostlethwaite too ⬆️
There was a problem hiding this comment.
I'd vote for completely skipping over non-plain-object items
It's always a mistake / error when we get values like this, right? rather than something with a real use case? Seems like the priorities are first don't break anything else so the rest of the plot still works, and second try to help the user figure out what they did wrong.
So erroring out is the wrong thing to do (don't break anything else - good catch!) but then we can either skip non-objects (as updatemenus.buttons does) or pretend they're empty objects (as updatemenus does). I kind of feel like pretending they're empty objects is better, because then the user can look at the array item by item and see what happened to their input. Also if we have anything that references these items by index it would still match up (though I don't know of anywhere we do that).
There was a problem hiding this comment.
because then the user can look at the array item by item and see what happened to their input
good point here.
What if non-plain-object item were coerced to { visible: false } instead? Unless you can think of a situation were http://codepen.io/etpinard/pen/ENjdBX may be useful.
There was a problem hiding this comment.
What if non-plain-object item were coerced to { visible: false }
Sure, if we've guaranteed that every container supports visible
There was a problem hiding this comment.
transforms are a container that use {enabled: false}, right?
There was a problem hiding this comment.
can we just say if(!Lib.isPlainObject(containerIn)) containerIn = {}; and let the ensuing coerce logic sort it out? Are there any containers that would end up visible/enabled if given an empty input? I guess maybe annotations and shapes, but this isn't useful for anyone, it was just a shortcut for creating them in the workspace... we could unwind that without causing any problems. But the point is, it shouldn't be a coerce (setting attributes of containerOut), it should just be replacing containerIn for the purpose of the logic that follows.
| @@ -427,4 +427,89 @@ describe('Test Plots', function() { | |||
| expect(gd._transitioning).toBeUndefined(); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('extendLayout', function() { | |||
| Plots.extendLayout(dest, src); | ||
|
|
||
| expect(dest).toEqual({ | ||
| annotations: [{ |
There was a problem hiding this comment.
This looks like what I'd expect. 👍
| }); | ||
| }); | ||
|
|
||
| it('clears container items when applying null src items', function() { |
There was a problem hiding this comment.
This also looks like what I'd expect. Am I correct in understanding that [undefined, undefined] would skip applying any changes?
| }); | ||
|
|
||
| it('clears container applying null src', function() { | ||
| var dest = { |
There was a problem hiding this comment.
Looks good. Ditto on behavior of undefined?
|
On my first pass through this I was focusing on the code but should have thought more about the tests. The behavior looks good to me, but some of the details here are still a little tricky. And if they're tricky for us, they're definitely tricky for users. Of course it's rather obscure use-cases so I think the key is that it's at least consistent/intuitive. @etpinard did you mention you were drying this up? Are there other thoughts or details I can help with? Other use-cases blocking this? |
- items set to undefined produce the same outcome as nestedProperty makes no distinctions between null and undefined
3312a45 to
66f8bb7
Compare
| @@ -102,13 +102,12 @@ exports.cleanLayout = function(layout) { | |||
| } | |||
| } | |||
|
|
|||
| if(layout.annotations !== undefined && !Array.isArray(layout.annotations)) { | |||
There was a problem hiding this comment.
this commit was enough to fix bug discovered in http://codepen.io/etpinard/pen/xRGgwW
| * in handleItemDefaults relies on that fact. | ||
| * | ||
| */ | ||
| module.exports = function handleArrayContainerDefaults(parentObjIn, parentObjOut, opts) { |
There was a problem hiding this comment.
similar to plots/subplot_defaults.
The logic below is pretty trivial, but might as well 🔒 down the behavior for all array containers.
| containerOut = layoutOut.annotations = []; | ||
| var opts = { | ||
| name: 'annotations', | ||
| handleItemDefaults: handleAnnotationDefaults |
| }; | ||
|
|
||
| var expected = { | ||
| container: [null, null] |
There was a problem hiding this comment.
N.B. I changed the behavior here as discussed in #1118 (comment) - see 36859f1
In brief, frames with array containers set to null extend the state with null. It is now up to the subsequent supplyDefaults calls to coerced those null items into {}, as in regular Plotly.plot calls.
| if(layout.annotations !== undefined && !Array.isArray(layout.annotations)) { | ||
| Lib.warn('Annotations must be an array.'); | ||
| delete layout.annotations; | ||
| } | ||
| var annotationsLen = (layout.annotations || []).length; |
There was a problem hiding this comment.
in handleArrayContainerDefaults you do Array.isArray(parentObjIn[name]) ? parentObjIn[name] : [] - you want to do that here too so we still keep going if annotations isn't even an array?
There was a problem hiding this comment.
Good catch here. Thanks!
The only (I think) case where cont || [] vs Array.isArray(cont) ? ... matters is when someone inputs a string instead of an array.
There was a problem hiding this comment.
yep, true.length doesn't break 😮
| if(layout.shapes !== undefined && !Array.isArray(layout.shapes)) { | ||
| Lib.warn('Shapes must be an array.'); | ||
| delete layout.shapes; | ||
| } | ||
| var shapesLen = (layout.shapes || []).length; |
There was a problem hiding this comment.
Array.isArray again... I guess if you test this situation you may find even more of these...
| * links to supplementary data (e.g. fullData for layout components) | ||
| * | ||
| * - opts.itemIsNotPlainObject is mutated on every pass in case so logic | ||
| * in handleItemDefaults relies on that fact. |
There was a problem hiding this comment.
this feels a little odd to me. Do you ever see a case where other parts of opts would need to be accessible to the item handler (or is there one already that I didn't notice?), or could we just pass itemIsNotPlainObject by itself as the last arg to the handler?
There was a problem hiding this comment.
yep, annotations and shapes.
There was a problem hiding this comment.
Sure. I'll make this the last arg.
I've ran into some problem with our multi-argument internal function lately, but yeah you're right, mutating opts is stupid.
There was a problem hiding this comment.
Very nice unification. My usage comment (passing opts vs just itemIsNotPlainObject) is nonblocking, but I'm thinking we should poke even a little more into making sure we keep going no matter what garbage folks throw in (ie the comment about the container itself not being an array).
- so that we *only* loop over arrays.
|
|
||
| opts.handleItemDefaults(itemIn, itemOut, parentObjOut, opts); | ||
| opts.handleItemDefaults(itemIn, itemOut, parentObjOut, opts, itemOpts); |
There was a problem hiding this comment.
Sure, that's fine. I still don't quite see where the handler will use the original opts but I guess it doesn't hurt to leave it in.
|
💃 |
Proof of concept PR, attempting to address #1081 (comment) demonstrated in http://codepen.io/etpinard/pen/WoNryW and extending what #1041 put forward.
I'm looking for @rreusser's opinion.