8000 Array edits by alexcjohnson · Pull Request #1403 · plotly/plotly.js · GitHub
[go: up one dir, main page]
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.

Sign up for GitHub

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

Array edits #1403

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
cc45189
fix basic annotation reference change bug
alexcjohnson Jan 6, 2017
13a87ce
support RegExp in Lib.pushUnique
alexcjohnson Feb 18, 2017
61f250c
editContainerArray: consistent handling of container array edits
alexcjohnson Jan 10, 2017
a9526bf
test fixin
alexcjohnson Feb 21, 2017
649a831
robustify click_test
alexcjohnson Feb 21, 2017
d55568c
:hocho: obsolete getShapeLayer
alexcjohnson Feb 21, 2017
2e6c030
break up new circular deps
alexcjohnson Feb 22, 2017
f7e60fb
relinkPrivateKeys inside arrayContainerDefaults
alexcjohnson Feb 22, 2017
d7cdc0a
revert layout_image.json - keep #1390 separate for now
alexcjohnson Feb 22, 2017
7ed9eb9
fix for components using category names as coordinates
alexcjohnson Feb 22, 2017
9f89dd6
fix for autorange with bad axis refs from a component
alexcjohnson Feb 22, 2017
46962af
point to open issue #1111
alexcjohnson Feb 22, 2017
1af058a
allow all `info_array`s to be pruned and command args to be optional
alexcjohnson Feb 23, 2017
c6378b1
exclude `args` from `nestedProperty` pruning
alexcjohnson Feb 23, 2017
70c882b
keep _fullLayout container arrays in sync with splices in layout
alexcjohnson Feb 23, 2017
5fb462f
comments and tests for plot_api/helpers.hasParent
alexcjohnson Feb 24, 2017
d29cbae
editContainerArray => applyContainerArrayChanges
alexcjohnson Feb 24, 2017
3ef1c9b
test errors from parent/child simultaneous edits
alexcjohnson Feb 24, 2017
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
Prev Previous commit
Next Next commit
test fixin
  • Loading branch information
@alexcjohnson
alexcjohnson committed Feb 21, 2017
commit a9526bf1d825a3f900f1f02dd90c8433dbbaf7a2
9 changes: 3 additions & 6 deletions src/plot_api/manage_arrays.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,12 +141,8 @@ exports.editContainerArray = function editContainerArray(gd, np, edits, flags) {
}

var componentNums = Object.keys(edits).map(Number).sort(),
componentArray = np.get();

if(!componentArray) {
componentArray = [];
np.set(componentArray);
}
componentArrayIn = np.get(),
componentArray = componentArrayIn || [];

var deletes = [],
firstIndexChange = -1,
Expand Down Expand Up @@ -206,6 +202,7 @@ exports.editContainerArray = function editContainerArray(gd, np, edits, flags) {
}

if(!componentArray.length) np.set(null);
else if(!componentArrayIn) np.set(componentArray);

if(replotLater) return false;

Expand Down
19 changes: 16 additions & 3 deletions src/plot_api/plot_api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ var svgTextUtils = require('../lib/svg_text_utils');
var manageArrays = require('./manage_arrays');
var helpers = require('./helpers');
var subroutines = require('./subroutines');
var cartesianConstants = require('../plots/cartesian/constants');


/**
Expand Down Expand Up @@ -1950,6 +1951,17 @@ function _relayout(gd, aobj) {
doextra(ptrunk + '.autorange', true);
}
}
else if(pleaf.match(cartesianConstants.AX_NAME_PATTERN)) {
var fullProp = Lib.nestedProperty(fullLayout, ai).get(),
newType = (vi || {}).type;

// This can potentially cause strange behavior if the autotype is not
// numeric (linear, because we don't auto-log) but the previous type
// was log. That's a very strange edge case though
if(!newType || newType === '-') newType = 'linear';
Registry.getComponentMethod('annotations', 'convertCoords')(gd, fullProp, newType, doextra);
Registry.getComponentMethod('images', 'convertCoords')(gd, fullProp, newType, doextra);
}

// alter gd.layout

Expand All @@ -1967,7 +1979,7 @@ function _relayout(gd, aobj) {

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

Choose a reason for hiding this comment

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

why does 'updatemenus' need special treatment here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
else if(propStr === '') {
// special handling of undoit if we're adding or removing an element
Expand All @@ -1982,12 +1994,13 @@ function _relayout(gd, aobj) {
}
else Lib.warn('unrecognized full object value', aobj);

if(refAutorange(toggledObj, 'x') || refAutorange(toggledObj, 'y')) {
if(refAutorange(toggledObj, 'x') || refAutorange(toggledObj, 'y') &&
ai.indexOf('updatemenus') === -1) {
flags.docalc = true;
}
}
else if((refAutorange(obji, 'x') || refAutorange(obji, 'y')) &&
!Lib.containsAny(ai, ['color', 'opacity', 'align', 'dash'])) {
!Lib.containsAny(ai, ['color', 'opacity', 'align', 'dash', 'updatemenus'])) {
flags.docalc = true;
}

Expand Down
3 changes: 2 additions & 1 deletion test/jasmine/tests/command_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


// Change this to a non-simple binding:
return Plotly.relayout(gd, {'sliders[0].steps[0].args[0]': 'line.color'});
Expand Down 57AE
3 changes: 2 additions & 1 deletion test/jasmine/tests/layout_images_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@ describe('Layout images', function() {

Plotly.plot(gd, data, layout).then(function() {
assertImages(0);
expect(gd.layout.images).toBeUndefined();

return Plotly.relayout(gd, 'images[0]', makeImage(jsLogo, 0.1, 0.1));
})
Expand Down Expand Up @@ -375,7 +376,7 @@ describe('Layout images', function() {
})
.then(function() {
assertImages(0);
expect(gd.layout.images).toEqual([]);
expect(gd.layout.images).toBeUndefined();

done();
});
Expand Down
27 changes: 16 additions & 11 deletions test/jasmine/tests/mapbox_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,17 +545,22 @@ describe('mapbox plots', function() {
return layerLen;
}

function getLayerLength(gd) {
return (gd.layout.mapbox.layers || []).length;
}

function assertLayerStyle(gd, expectations, index) {
var mapInfo = getMapInfo(gd),
layers = mapInfo.layers,
layerNames = mapInfo.layoutLayers;

var layer = layers[layerNames[index]];
expect(layer).toBeDefined(layerNames[index]);

return new Promise(function(resolve) {
setTimeout(function() {
Object.keys(expectations).forEach(function(k) {
expect(layer.paint[k]).toEqual(expectations[k]);
expect(((layer || {}).paint || {})[k]).toEqual(expectations[k]);
});
resolve();
}, TRANSITION_DELAY);
Expand All @@ -565,26 +570,26 @@ describe('mapbox plots', function() {
expect(countVisibleLayers(gd)).toEqual(0);

Plotly.relayout(gd, 'mapbox.layers[0]', layer0).then(function() {
expect(gd.layout.mapbox.layers.length).toEqual(1);
expect(getLayerLength(gd)).toEqual(1);
expect(countVisibleLayers(gd)).toEqual(1);

// add a new layer at the beginning
return Plotly.relayout(gd, 'mapbox.layers[1]', layer1);
})
.then(function() {
expect(gd.layout.mapbox.layers.length).toEqual(2);
expect(getLayerLength(gd)).toEqual(2);
expect(countVisibleLayers(gd)).toEqual(2);

return Plotly.relayout(gd, mapUpdate);
})
.then(function() {
expect(gd.layout.mapbox.layers.length).toEqual(2);
expect(getLayerLength(gd)).toEqual(2);
expect(countVisibleLayers(gd)).toEqual(2);

return Plotly.relayout(gd, styleUpdate0);
})
.then(function() {
expect(gd.layout.mapbox.layers.length).toEqual(2);
expect(getLayerLength(gd)).toEqual(2);
expect(countVisibleLayers(gd)).toEqual(2);

return assertLayerStyle(gd, {
Expand All @@ -594,13 +599,13 @@ describe('mapbox plots', function() {
}, 0);
})
.then(function() {
expect(gd.layout.mapbox.layers.length).toEqual(2);
expect(getLayerLength(gd)).toEqual(2);
expect(countVisibleLayers(gd)).toEqual(2);

return Plotly.relayout(gd, styleUpdate1);
})
.then(function() {
expect(gd.layout.mapbox.layers.length).toEqual(2);
expect(getLayerLength(gd)).toEqual(2);
expect(countVisibleLayers(gd)).toEqual(2);

return assertLayerStyle(gd, {
Expand All @@ -610,20 +615,20 @@ describe('mapbox plots', function() {
}, 1);
})
.then(function() {
expect(gd.layout.mapbox.layers.length).toEqual(2);
expect(getLayerLength(gd)).toEqual(2);
expect(countVisibleLayers(gd)).toEqual(2);

// delete the first layer
return Plotly.relayout(gd, 'mapbox.layers[0]', null);
})
.then(function() {
expect(gd.layout.mapbox.layers.length).toEqual(1);
expect(getLayerLength(gd)).toEqual(1);
expect(countVisibleLayers(gd)).toEqual(1);

return Plotly.relayout(gd, 'mapbox.layers[0]', null);
})
.then(function() {
expect(gd.layout.mapbox.layers).toBeUndefined();
expect(getLayerLength(gd)).toEqual(0);
expect(countVisibleLayers(gd)).toEqual(0);

return Plotly.relayout(gd, 'mapbox.layers[0]', {});
Expand All @@ -637,7 +642,7 @@ describe('mapbox plots', function() {
return Plotly.relayout(gd, 'mapbox.layers[0].source', layer0.source);
})
.then(function() {
expect(gd.layout.mapbox.layers.length).toEqual(1);
expect(getLayerLength(gd)).toEqual(1);
expect(countVisibleLayers(gd)).toEqual(1);
})
.catch(failTest)
Expand Down
5 changes: 4 additions & 1 deletion test/jasmine/tests/plotschema_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,10 @@ describe('plot schema', function() {
expect(plotSchema.defs.valObjects).toBeDefined();

expect(plotSchema.defs.metaKeys)
.toEqual(['_isSubplotObj', '_isLinkedToArray', '_deprecated', 'description', 'role']);
.toEqual([
'_isSubplotObj', '_isLinkedToArray', '_arrayAttrRegexps',
'_deprecated', 'description', 'role'
]);
});

it('should list the correct frame attributes', function() {
Expand Down
5 changes: 3 additions & 2 deletions test/jasmine/tests/updatemenus_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ describe('update menus interactions', function() {
});

it('should draw only visible menus', function(done) {
var initialUM1 = Lib.extendDeep({}, gd.layout.updatemenus[1]);
assertMenus([0, 0]);
expect(gd._fullLayout._pushmargin['updatemenu-0']).toBeDefined();
expect(gd._fullLayout._pushmargin['updatemenu-1']).toBeDefined();
Expand All @@ -333,7 +334,7 @@ describe('update menus interactions', function() {

return Plotly.relayout(gd, {
'updatemenus[0].visible': true,
'updatemenus[1].visible': true
'updatemenus[1]': initialUM1
});
})
.then(function() {
Expand Down Expand Up @@ -655,7 +656,7 @@ describe('update menus interactions', function() {
});

function assertNodeCount(query, cnt) {
expect(d3.selectAll(query).size()).toEqual(cnt);
expect(d3.selectAll(query).size()).toEqual(cnt, query);
}

// call assertMenus([0, 3]); to check that the 2nd update menu is dropped
Expand Down
0