Array edits #1403

alexcjohnson · 2017-02-21T21:31:40Z

rreusser · 2017-02-21T21:32:56Z

etpinard · 2017-02-21T22:10:33Z

src/lib/nested_property.js

+ * ie containing objects like annotations, buttons, etc
+ */
+var DOMAIN_RANGE = /(^|.)(domain|range)$/;
+function isDeletable(val, propStr) {


Does this work for domain.x and domain.y ?

And more generally, shouldn't all info_array attributes (e.g. updemenues/sliders args, and geo.projection.parallels) be deletable?

Yes, good point. I was trying to match the previous logic and extend it gently but honestly I can't recall anymore why we had that logic - why does an empty data array need to stick around while other empty arrays can be removed entirely? Perhaps just to keep things simple for streaming? Does that point to a more robust condition on what should be deletable?

Alright, letting every array be deletable and running the full test suite was fairly enlightening - There are a lot of things that fail or error out, some of which could be addressed but fundamentally for data arrays there really is a difference between [] (which means there is data of that kind, it's just presently empty) and undefined (which means the data/attribute should get some default value filled in).

As far as I can tell, for info_array attributes there is no difference between [] and undefined though. So unless we want to try and build in here some way to hunt through the schema (which sounds super painful to me, and potentially not even well-defined given that nestedProperty can, on occasion, be called on a root object that's neither a complete trace or layout object but some portion of it) I would argue for some simple tests that might frequently describe an info array as falsely not deletable but will never describe a real data array as deletable.

As far as I can tell, for info_array attributes there is no difference between [] and undefined though

That's correct.

expanded the safe-to-delete list to include all info_array attributes in 1af058a

@rreusser this also required making args optional for sliders and updatemenus (fallback to [] in command.js) since an empty array will now get pruned. Not sure there will be a real use case for this (what method could one call with no args?) but this case pops up in the tests anyhow 🏆 and it does seem more plotly-esque to not require empty parameters.

The only use case I'm aware of is animating an empty list of frames [] which (serendipitously) functions as a pause button. I've had a bit of trouble with this behavior, so we documented pretty clearly in the web docs that you must use [null] instead of [] to prevent it from getting filtered and replaced with a bare null.

That is, in particular, it's when you create an updatemenus button with a command that pauses. It filters updatemenus[0].buttons[0].args = [[], {...}] and replaces it with [null, {...}] in that case.

etpinard · 2017-02-21T22:23:01Z

src/plot_api/plot_api.js

+    // even if we don't have anything left in aobj,
+    // something may have happened within relayout that we
+    // need to wait for
+    var seq = [Plots.previousPromises];


Good eyes 🔬

etpinard · 2017-02-21T22:31:24Z

src/components/updatemenus/attributes.js

@@ -49,6 +49,7 @@ var buttonsAttrs = {

 module.exports = {
    _isLinkedToArray: 'updatemenu',
+    _arrayAttrRegexps: [/^updatemenus\[(0|[1-9][0-9]+)\]\.buttons/],


I'm ok with adding _arrayAttrRegexps in this PR (there's enough new logic it in already 😫 ), but we could probably generate it at runtime down the road in the registry here?

Isn't _arrayAttrRegexps simply,

allIsLinkedToArrayAttributes.map((attr) => { return module.name + '\[[(0|[1-9][0-9]+)\]\.' + attr; });

that wouldn't work for the other instance of this, /^mapbox([2-9]|[1-9][0-9]+)?\.layers/

Just wondering why you chose to attach _arrayAttrRegexps to the attribute object?

Alternatively, we could set in the module object here - and not having to skip it in the plot schema logic.

Yes, we could... I just put it there because that's where _isLinkedToArray lives and I thought it would be good to keep them together. I don't have a strong opinion either way.

I just put it there because that's where _isLinkedToArray lives

Very good point. Let's keep it that way 👍

rreusser · 2017-02-21T22:50:21Z

alexcjohnson · 2017-02-22T06:35:25Z

src/plots/array_container_defaults.js

+        for(i = 0; i < len; i++) {
+            Lib.relinkPrivateKeys(contOut[i], previousContOut[i]);
+        }
+    }


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

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

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

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?

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.

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.

alexcjohnson · 2017-02-22T06:40:46Z

src/plots/plots.js

+            Registry.getComponentMethod(dataReferencedComponents[i], 'supplyLayoutDefaults')(
+                gd.layout, fullLayout, fullData);
+        }
+    }


I suppose I could list these components somehow in the registry... worth it or overkill?

BTW you'll see I added category-name coordinates to image and shape test images, annotations didn't need this because 16.json already has it, and that's the failing test that prompted this fix.

Let's leave it like that for now. We could always add that list to the registry down the road if it comes up somewhere else. 👍

alexcjohnson · 2017-02-22T06:46:43Z

src/lib/noop.js

+// Simple helper functions
+// none of these need any external deps
+
+module.exports = function noop() {};


I thought this was about the shortest possible module one could make... but I suppose you could do:

exports.a = 0;

Kind of silly... but needed to de-circularize, short of refactoring Lib (see #236 )

(resisting temptation suggest a noop factory approach)

Actually it's important that noop is a singleton so you can use === noop particularly on the results of Registry.getComponentMethod to see if it was found.

alexcjohnson · 2017-02-22T06:48:52Z

tasks/test_syntax.js


        if(circularDeps.length > MAX_ALLOWED_CIRCULAR_DEPS) {
+            console.log(circularDeps.join('\n'));


Just to see where I had made circular refs when this test was failing. So now if you want to see them all just set MAX_ALLOWED_CIRCULAR_DEPS = 0 temporarily and run this.

Nice touch 🎆

etpinard · 2017-02-22T15:37:42Z

src/plot_api/plot_api.js

@@ -2092,9 +2092,6 @@ function _relayout(gd, aobj) {
    var oldWidth = gd._fullLayout.width,
        oldHeight = gd._fullLayout.height;

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


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 🐎

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?

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.

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

etpinard · 2017-02-22T15:42:53Z

src/plot_api/plot_api.js

@@ -251,7 +251,7 @@ Plotly.plot = function(gd, data, layout, config) {
        // calc and autorange for errorbars
        ErrorBars.calc(gd);

-        // TODO: autosize extra for text markers
+        // TODO: autosize extra for text markers and images


There's actually an issue for that one #1111

ah thanks - comment-linked in 46962af

etpinard · 2017-02-23T00:31:24Z

src/lib/push_unique.js

+        var itemStr = item.toString(),
+            i;
+        for(i = 0; i < array.length; i++) {
+            if(array[i] instanceof RegExp && array[i].toString() === itemStr) {


Can you add a few test cases for regex items ->

https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/lib_test.js#L1246

https://github.com/plotly/plotly.js/pull/1403/files#diff-9edb82cf7ed7d529d7e77cce5714360aR1287

I need better 👓

etpinard · 2017-02-23T00:49:57Z

src/plot_api/manage_arrays.js

+ * - delete b and edit c:
+ *    {'annotations[1]': null, 'annotations[2].x': newX} (c is edited before b is removed)
+ *
+ * You CANNOT combine adding/deleting an item at index `i` with edits to the same index `i`


Can we add tests for these CANNOT cases?

Oops. Just saw https://github.com/plotly/plotly.js/pull/1403/files#diff-70e5a2fafe3d8a7e1ed2e632e9781df9R300

Ignore.

etpinard · 2017-02-23T02:04:08Z

src/plot_api/plot_api.js

+
+            if(i === '') {
+                // replacing the entire array: too much going on, force recalc
+                if(ai.indexOf('updatemenus') === -1) flags.docalc = true;


why does 'updatemenus' need special treatment here?

proximate reason: because this test fails after recalc because gd.calcdata is a new object. But what that shows is that we don't actually need a recalc in this case. We probably could exclude from recalc everything that doesn't contribute to autorange (so, I suspect only annotations, shapes, and eventually images need recalc) but without taking the time to investigate all the other cases in detail I erred on the side of caution.

Ok. That's fine. Maybe we should open (another) Registry list for components that have a calcAutorange method (like annotations and shapes) for this purpose here. But that's not necessary in this PR.

etpinard · 2017-02-23T16:50:27Z

src/lib/nested_property.js

-        isNotAContainer = containers.indexOf(key) === -1;
-
-    return isArray(val) && isNotAContainer;
+var INFO_PATTERNS = /(^|.)((domain|range)(\.[xy])?|args|parallels)$/;


Ok, well this is the only part I dislike about this PR. I'm ok with leaving as is for now, but should think about ways to 🔪 this down the road.

Solution 1: simply don't delete info_array attributes, like data_array

Solution 2: use the plot schema to look up all info_array attributes. This doesn't have to be done inside nested property where objects are set w/o a reference to their parent object (if any). Maybe this could be done only during restyle, relayout and update.

Solution 3: something else

added even more complexity c6378b1 after discussion with @rreusser but opened #1410 to discuss simplification later on.

etpinard · 2017-02-23T21:32:13Z

test/jasmine/tests/lib_test.js

@@ -375,6 +375,30 @@ describe('Test lib.js:', function() {
            expect(obj).toEqual({a: [undefined, {}]});
        });

+        it('does not prune inside `args` arrays', function() {


Nicely done. Thanks!

etpinard · 2017-02-23T21:36:54Z

rreusser · 2017-02-24T15:04:09Z

src/plot_api/helpers.js

@@ -484,3 +484,19 @@ exports.manageArrayContainers = function(np, newVal, undoit) {
        np.set(newVal);
    }
 };
+
+var ATTR_TAIL_RE = /(\.[^\[\]\.]+|\[[^\[\]\.]+\])$/;


Could you add maybe a single line or two about the sort of pattern this matches? I'm guessing… dot… more than one of… something… that's not brackets… or something that's not brackets or a dot? So

.something something

match while

.something[] something[]

do not?

(not a big deal; just difficult to confirm we're on the same page about what the end of an attr string means by mentally parsing the regexp)

good call - also added some tests for hasParent -> 5fb462f

rreusser

rreusser · 2017-02-24T15:33:29Z

src/plot_api/manage_arrays.js

+ * @returns {bool} `true` if it managed to complete drawing of the changes
+ *  `false` would mean the parent should replot.
+ */
+exports.editContainerArray = function editContainerArray(gd, np, edits, flags) {


Is editContainerArray the best name for this function? Grammar-wise, applyContainerArrayChanges might be more descriptive, and functionality-wise, I didn't assume by the name that it would also supply defaults and perform drawing updates.

rreusser · 2017-02-24T15:33:46Z

src/plot_api/manage_arrays.js

+ * edit an object -> {'annotations[2].text': 'boo'} -> {2: {'text': 'boo'}}
+ *
+ * You can combine many edits to different objects. Objects are added and edited
+ * in ascending order, then removed in descending order.


Thanks for enforcing that! As discussed elsewhere, the spec does not require any particular order when iterating over object keys, so I feel better knowing we're not just figuring people deserve to suffer the consequences if they combine operations ambiguously.

The other ambiguity-removing thing I did was to make both _restyle and _relayout error out if you try to edit both an object and one of its parents. That said, perhaps based on some other conversations of ours @etpinard in this case we should just log a warning and drop the child attribute, rather than throwing an error?

So {'marker.color': 'red', marker: {color: 'red}} errors out? I don't have strong feelings, though plotly's general solution seems to be to log an error and do the best it can.

To me, there are two types of plotly.js errors:

Invalid plot attribute combinations,

Invalid inputs to Plotly.___ methods.

I believe the former should never lead to errors as @rreusser said plotly.js should try to do the best it can. This is especially important for non-JS users that send "data" and "layout" and expect to get in return the best possible ™️ graph.

This case ⬆️ falls into the second category. Plotly.__ method are called by JS users only* and throwing errors should help them debug. For example in Plotly.plot(gd, fig) if gd isn't right we throw an error. I think the same should be expected for invalid relayout update objects.

*: Note that updatemenus and slide 10000 r now expose some Plotly.__ methods to non-JS users now. 😑

Good philosophical points, @etpinard!

rreusser · 2017-02-24T15:34:29Z

src/plot_api/manage_arrays.js

+ * (as prepared in _relayout):
+ *
+ * add an empty obj -> {'annotations[2]': 'add'} -> {2: {'': 'add'}}
+ * add a specific obj -> {'annotations[2]': {attrs}} -> {2: {'': {attrs}}}


I would have expected this to say {'annotations[2]': {attrs}} -> {2: {attrs}}, but I may have content confused with the action to insert content.

right - it's a bit of a weird structure I ended up with as input to editContainerArray. It's probably clear, but this is prepared in _relayout and users never see it. It's structured as edits[index][attribute] - so if you're setting an array element all at once, attribute will be '', and if you're setting the whole array at once, both attribute and index will be ''

Perfect. 👍

The reason it's not {'annotations[2]': {attrs}} -> {2: {attrs}} is that {2: {attrs}} and {2: {'': {attrs}} mean different things: the former would change just attrs, leaving intact anything else that's already present in annotations[2]. The latter inserts a new object containing just {attrs}.

rreusser · 2017-02-24T15:34:59Z

src/plot_api/manage_arrays.js

+                componentType);
+        }
+
+        var fullVal = edits[''][''];


Is it conceivable that edits[''] could ever not be array-indexable?

per #1403 (comment) - full-array changes will always have exactly this form. That was just the easiest construction for _relayout to do without edge cases, though I agree it looks a little odd in here...

All good! Assumed so. Just a PR-review reflex. 👍

rreusser · 2017-02-24T15:38:44Z

src/plot_api/plot_api.js

+        arrayEdits = {},
+        arrayStr,
+        i,
+        j;


FYI: I think by far the biggest manual change required to move from eslint to standard is that variable declarations like this must manually be split. The general rule is that initialized declarations must each have their own var and uninitialized declarations are fine in the same statement. So no particular feedback here, just that every time I see these, I worry about the big cleanup pass ahead 😬

Interesting... what's "standard"?

Was missing link to project page in that PR. Added. It's: https://github.com/feross/standard

Long story short, unconfigurable eslint.

rreusser · 2017-02-24T15:44:02Z

src/plot_api/plot_api.js

+                        r1 = ax.range[1];
+                    if(toLog) {
+                        // if both limits are negative, autorange
+                        if(r0 <= 0 && r1 <= 0) {


This is nothing new and probably not worth addressing, but lots of logic and log calculations deep inside a 350+ line function seem like they'd be difficult to test and maintain. Would be nice if there were a better place for the logic.

Man, the single biggest change I'm looking forward to in v2.0 is not using linearized coordinates for log axes. All of this crap will disappear then.

rreusser · 2017-02-24T15:48:19Z

src/plot_api/plot_api.js

+    // now we've collected component edits - execute them all together
+    for(arrayStr in arrayEdits) {
+        var finished = manageArrays.editContainerArray(gd,
+            Lib.nestedProperty(layout, arrayStr), arrayEdits[arrayStr], flags);


Same feedback. If this is the only place it's used, perhaps the supplydefaults and draw parts should be outside the function that manages arrays? You have a better sense than I do though, so feel free to disregard :)

For now I'll change the name per #1403 (comment) -> d29cbae but I suspect in implementing #1412 we will restructure it along the lines you're suggesting.

rreusser · 2017-02-24T15:50:32Z

src/plots/array_container_defaults.js

+        for(i = 0; i < len; i++) {
+            Lib.relinkPrivateKeys(contOut[i], previousContOut[i]);
+        }
+    }


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.

rreusser · 2017-02-24T15:52:32Z

src/registry.js

@@ -9,7 +9,9 @@

 'use strict';

-var Lib = require('./lib');
+var Loggers = require('./lib/loggers');
+var noop = require('./lib/noop');


echo "var noop = require('./lib/noop');" |wc -c 34 $ echo "function noop() {}" |wc -c 19

No. Stop. Don't bikeshed, Ricky. Don't bikeshed. LGTM. 👍

rreusser · 2017-02-24T15:57:32Z

test/jasmine/tests/command_test.js

@@ -594,7 +594,8 @@ describe('attaching component bindings', function() {
            expect(gd.layout.sliders[0].active).toBe(0);

            // Check that it still has one attached listener:
-            expect(typeof gd._internalEv._events.plotly_animatingframe).toBe('function');
+            expect(typeof gd._internalEv._events.plotly_animatingframe).toBe('function',
+                gd._internalEv._events.plotly_animatingframe);


The second argument to toBe adds a message? No worries, just checking to make sure there's no unintended difference in function.

yes - that's true for most of the jasmine matchers, and can be really useful in debugging failures.

alexcjohnson · 2017-02-24T21:58:24Z

test/jasmine/tests/plot_api_test.js

-            });
+            })
+            .catch(fail)
+            .then(done);


Hah, this test wasn't actually running. More precisely, it was running but not being used. Who wrote that sloppy thing? 🙈

Not me 😄

The biggest irony is my commit message there:

fix the tests I must have only thought I had run successfully

alexcjohnson added 4 commits February 21, 2017 15:28

fix basic annotation reference change bug

cc45189

support RegExp in Lib.pushUnique

13a87ce

test fixin

a9526bf

alexcjohnson added the status: in progress label Feb 21, 2017

etpinard reviewed Feb 21, 2017

View reviewed changes

alexcjohnson added 2 commits February 21, 2017 18:02

robustify click_test

649a831

🔪 obsolete getShapeLayer

d55568c

alexcjohnson mentioned this pull request Feb 22, 2017

De-circularize plotly.js require statements #236

Closed

alexcjohnson added 4 commits February 21, 2017 23:37

break up new circular deps

2e6c030

relinkPrivateKeys inside arrayContainerDefaults

f7e60fb

revert layout_image.json - keep #1390 separate for now

d7cdc0a

fix for components using category names as coordinates

7ed9eb9

alexcjohnson commented Feb 22, 2017

View reviewed changes

etpinard reviewed Feb 22, 2017

View reviewed changes

alexcjohnson added 2 commits February 22, 2017 15:54

fix for autorange with bad axis refs from a component

9f89dd6

point to open issue #1111

46962af

etpinard reviewed Feb 23, 2017

View reviewed changes

allow all info_arrays to be pruned and command args to be optional

1af058a

etpinard reviewed Feb 23, 2017

View reviewed changes

etpinard added this to the 1.24.0 milestone Feb 23, 2017

alexcjohnson added 2 commits February 23, 2017 13:49

exclude args from nestedProperty pruning

c6378b1

keep _fullLayout container arrays in sync with splices in layout

70c882b

alexcjohnson mentioned this pull request Feb 23, 2017

Stop pruning in nestedProperty? #1410

Closed

etpinard reviewed Feb 23, 2017

View reviewed changes

alexcjohnson mentioned this pull request Feb 24, 2017

Use editContainerArray for arrays in traces too #1412

Closed

rreusser reviewed Feb 24, 2017

View reviewed changes

rreusser approved these changes Feb 24, 2017

View reviewed changes

alexcjohnson added 2 commits February 24, 2017 11:23

comments and tests for plot_api/helpers.hasParent

5fb462f

editContainerArray => applyContainerArrayChanges

d29cbae

etpinard mentioned this pull request Feb 24, 2017

update annotations doc due to 'remove' deprecation plotly/documentation#578

Open

test errors from parent/child simultaneous edits

3ef1c9b

alexcjohnson commented Feb 24, 2017

View reviewed changes

alexcjohnson merged commit cab2b85 into master Feb 24, 2017

alexcjohnson deleted the array-edits branch February 24, 2017 22:15

etpinard mentioned this pull request Mar 17, 2017

Relayout edge case fixes #1494

Merged

etpinard mentioned this pull request May 29, 2017

shapes on category axis are redrawn incorrectly after zoom #1720

Closed

Uh oh!

Array edits #1403

Array edits #1403

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment