Make frame with nulls clear items & array containers #1118

etpinard · 2016-11-07T19:52:35Z

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.

etpinard · 2016-11-07T19:52:57Z

test/jasmine/tests/plots_test.js

@@ -427,4 +427,89 @@ describe('Test Plots', function() {
            expect(gd._transitioning).toBeUndefined();
        });
    });
+
+    describe('extendLayout', function() {


@rreusser what do you think of ⏬

Checking now…

rreusser

I like this. I think it makes sense. I'd also be okay with setting empty items to null, but I think that would require a bit more work to make sure nothing crashes when it references a property on null. 💃

rreusser · 2016-11-07T19:58:52Z

src/plots/plots.js

-            containerProp.set(null);
-            Lib.nestedProperty(containerObj, containerPaths[i]).set(containerVal);
+
+            if(!containerVal) {


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?

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.

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.

rreusser · 2016-11-07T20:06:28Z

src/plots/plots.js

-                destContainer[j] = plots.extendObjectWithContainers(destContainer[j], srcContainer[j]);
+                var srcObj = srcContainer[j];
+
+                if(srcObj === null) destContainer[j] = {};


I like this. Could be null, but seems the most robust/backwards-compatible if it's at least an object.

Glad we agree here.

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).

cc @bpostlethwaite too ⬆️

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).

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.

What if non-plain-object item were coerced to { visible: false }

Sure, if we've guaranteed that every container supports visible

transforms are a container that use {enabled: false}, right?

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.

rreusser · 2016-11-07T20:06:47Z

test/jasmine/tests/plots_test.js

@@ -427,4 +427,89 @@ describe('Test Plots', function() {
            expect(gd._transitioning).toBeUndefined();
        });
    });
+
+    describe('extendLayout', function() {


rreusser · 2016-11-09T23:07:35Z

test/jasmine/tests/plots_test.js

+            Plots.extendLayout(dest, src);
+
+            expect(dest).toEqual({
+                annotations: [{


This looks like what I'd expect. 👍

rreusser · 2016-11-09T23:08:39Z

test/jasmine/tests/plots_test.js

+            });
+        });
+
+        it('clears container items when applying null src items', function() {


This also looks like what I'd expect. Am I correct in understanding that [undefined, undefined] would skip applying any changes?

rreusser · 2016-11-09T23:08:58Z

test/jasmine/tests/plots_test.js

+        });
+
+        it('clears container applying null src', function() {
+            var dest = {


Looks good. Ditto on behavior of undefined?

rreusser · 2016-11-09T23:11:47Z

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

etpinard · 2016-11-10T00:03:47Z

src/plot_api/helpers.js

@@ -102,13 +102,12 @@ exports.cleanLayout = function(layout) {
        }
    }

-    if(layout.annotations !== undefined && !Array.isArray(layout.annotations)) {


this commit was enough to fix bug discovered in http://codepen.io/etpinard/pen/xRGgwW

etpinard · 2016-11-10T00:05:03Z

src/plots/array_container_defaults.js

+ *    in handleItemDefaults relies on that fact.
+ *
+ */
+module.exports = function handleArrayContainerDefaults(parentObjIn, parentObjOut, opts) {


similar to plots/subplot_defaults.

The logic below is pretty trivial, but might as well 🔒 down the behavior for all array containers.

etpinard · 2016-11-10T00:05:44Z

src/components/annotations/defaults.js

-        containerOut = layoutOut.annotations = [];
+    var opts = {
+        name: 'annotations',
+        handleItemDefaults: handleAnnotationDefaults


etpinard · 2016-11-10T00:09:52Z

test/jasmine/tests/plots_test.js

+            };
+
+            var expected = {
+                container: [null, null]


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.

alexcjohnson · 2016-11-10T22:11:21Z

src/plot_api/helpers.js

-    if(layout.annotations !== undefined && !Array.isArray(layout.annotations)) {
-        Lib.warn('Annotations must be an array.');
-        delete layout.annotations;
-    }
    var annotationsLen = (layout.annotations || []).length;


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?

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.

yep, true.length doesn't break 😮

done in 9de2f77

alexcjohnson · 2016-11-10T22:12:40Z

src/plot_api/helpers.js

-    if(layout.shapes !== undefined && !Array.isArray(layout.shapes)) {
-        Lib.warn('Shapes must be an array.');
-        delete layout.shapes;
-    }
    var shapesLen = (layout.shapes || []).length;


Array.isArray again... I guess if you test this situation you may find even more of these...

done in 9de2f77

alexcjohnson · 2016-11-10T22:21:49Z

src/plots/array_container_defaults.js

+ *    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.


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?

yep, annotations and shapes.

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.

done in 5de6ff0

alexcjohnson

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.

alexcjohnson · 2016-11-10T23:38:28Z

src/plots/array_container_defaults.js


-        opts.handleItemDefaults(itemIn, itemOut, parentObjOut, opts);
+        opts.handleItemDefaults(itemIn, itemOut, parentObjOut, opts, itemOpts);


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.

alexcjohnson · 2016-11-10T23:38:52Z

💃

etpinard added feature something new status: in progress labels Nov 7, 2016

etpinard commented Nov 7, 2016

View reviewed changes

rreusser approved these changes Nov 7, 2016

View reviewed changes

etpinard added this to the v1.20.0 milestone Nov 7, 2016

rreusser reviewed Nov 9, 2016

View reviewed changes

etpinard added 8 commits November 9, 2016 18:59

test: add cases for Lib.expandObjectPaths w/ array containers

69bffe1

make clean data work for non-plain-object annotation / shape items

4c49755

add general array container defaults handler

d21d782

DRY layout array container defaults using handleArrayContainerDefaults

138ac35

test: add cases for array container defaults

d0953a0

make sure array containers set to null in frames are honored

174b640

make sure items inside array container set to null are honored in frames

36859f1

- items set to undefined produce the same outcome as nestedProperty makes no distinctions between null and undefined

test: add cases for extendObjectWithContainers

66f8bb7

etpinard force-pushed the frame-extend-with-nulls branch from 3312a45 to 66f8bb7 Compare November 9, 2016 23:59

etpinard added status: reviewable and removed status: in progress labels Nov 9, 2016

etpinard commented Nov 10, 2016

View reviewed changes

etpinard added the bug something broken label Nov 10, 2016

Merge branch 'master' into frame-extend-with-nulls

8ed1957

alexcjohnson reviewed Nov 10, 2016

View reviewed changes

alexcjohnson requested changes Nov 10, 2016

View reviewed changes

etpinard added 2 commits November 10, 2016 18:20

make annotation/shapes cleanLayout blocks more robust

9de2f77

- so that we *only* loop over arrays.

split container opts from item opts in handleArrayContainerDefaults

5de6ff0

alexcjohnson reviewed Nov 10, 2016

View reviewed changes

alexcjohnson approved these changes Nov 10, 2016

View reviewed changes

etpinard merged commit 2897167 into master Nov 11, 2016

etpinard deleted the frame-extend-with-nulls branch November 11, 2016 15:25


		opts.handleItemDefaults(itemIn, itemOut, parentObjOut, opts);
		opts.handleItemDefaults(itemIn, itemOut, parentObjOut, opts, itemOpts);

Make frame with nulls clear items & array containers #1118

Make frame with nulls clear items & array containers #1118

Conversation

etpinard commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser commented Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

etpinard Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Nov 10, 2016

rreusser left a comment •

edited

Loading

rreusser commented Nov 9, 2016 •

edited

Loading

etpinard Nov 10, 2016 •

edited

Loading