Skip to content

Commit 104f98c

Browse files
author
Steve Orvell
committed
Merge pull request #2262 from Polymer/2261-kschaaf-collection-sort
Reduce keySplices to minimum change set before notifying. Fixes #2261
2 parents 7497729 + 337b54a commit 104f98c

File tree

4 files changed

+106
-62
lines changed

4 files changed

+106
-62
lines changed

‎src/lib/collection.html

+38-21
Original file line numberDiff line numberDiff line change
@@ -105,33 +105,50 @@
105105
return items;
106106
},
107107

108+
// Accepts an array of standard splice records (index, addedCount, removed
109+
// array), and performs two key actions:
110+
// 1. Applies the splice to the collection: adds newly added items to the
111+
// store which generates a unique key for it, and removes removed items
112+
// (and their key) from the store
113+
// 2. Generates a "keySplices" record (in contrast to the input
114+
// "indexSplices"), which contains an array of added and removed keys
115+
// corresponding to the added/removed items
108116
_applySplices: function(splices) {
109-
var keySplices = [];
110-
for (var i=0; i<splices.length; i++) {
111-
var j, o, key, s = splices[i];
112-
// Removed keys
113-
var removed = [];
114-
for (j=0; j<s.removed.length; j++) {
115-
o = s.removed[j];
116-
key = this.remove(o);
117+
// Dedupe added and removed keys to a final added/removed map
118+
var keyMap = {}, key, i;
119+
splices.forEach(function(s) {
120+
s.addedKeys = [];
121+
for (i=0; i<s.removed.length; i++) {
122+
key = this.getKey(s.removed[i]);
123+
keyMap[key] = keyMap[key] ? null : -1;
124+
}
125+
for (i=0; i<s.addedCount; i++) {
126+
var item = this.userArray[s.index + i];
127+
key = this.getKey(item);
128+
key = (key === undefined) ? this.add(item) : key;
129+
keyMap[key] = keyMap[key] ? null : 1;
130+
// Add an "addedKeys" array to indexSplices to capture keys associated
131+
// with added items, since references to added items can be lost by
132+
// further changes to the array by the time the splice is consumed
133+
s.addedKeys.push(key);
134+
}
135+
}, this);
136+
// Convert added/removed key map to added/removed arrays
137+
var removed = [];
138+
var added = [];
139+
for (var key in keyMap) {
140+
if (keyMap[key] < 0) {
141+
this.removeKey(key);
117142
removed.push(key);
118143
}
119-
// Added keys
120-
var added = [];
121-
for (j=0; j<s.addedCount; j++) {
122-
o = this.userArray[s.index + j];
123-
key = this.add(o);
144+
if (keyMap[key] > 0) {
124145
added.push(key);
125146
}
126-
// Record splice
127-
keySplices.push({
128-
index: s.index,
129-
removed: removed,
130-
removedItems: s.removed,
131-
added: added
132-
});
133147
}
134-
return keySplices;
148+
return [{
149+
removed: removed,
150+
added: added
151+
}];
135152
}
136153

137154
};

‎src/lib/template/dom-repeat.html

+14-11
Original file line numberDiff line numberDiff line change
@@ -268,11 +268,13 @@
268268
this._error(this._logf('dom-repeat', 'expected array for `items`,' +
269269
' found', this.items));
270270
}
271-
this._splices = [];
271+
this._keySplices = [];
272+
this._indexSplices = [];
272273
this._needFullRefresh = true;
273274
this._debounceTemplate(this._render);
274275
} else if (change.path == 'items.splices') {
275-
this._splices = this._splices.concat(change.value.keySplices);
276+
this._keySplices = this._keySplices.concat(change.value.keySplices);
277+
this._indexSplices = this._indexSplices.concat(change.value.indexSplices);
276278
this._debounceTemplate(this._render);
277279
} else { // items.*
278280
// slice off 'items.' ('items.'.length == 6)
@@ -303,10 +305,10 @@
303305
},
304306

305307
/**
306-
* Forces the element to render its content. Normally rendering is
307-
* asynchronous to a provoking change. This is done for efficiency so
308-
* that multiple changes trigger only a single render. The render method
309-
* should be called if, for example, template rendering is required to
308+
* Forces the element to render its content. Normally rendering is
309+
* asynchronous to a provoking change. This is done for efficiency so
310+
* that multiple changes trigger only a single render. The render method
311+
* should be called if, for example, template rendering is required to
310312
* validate application state.
311313
*/
312314
render: function() {
@@ -324,17 +326,18 @@
324326
this._needFullRefresh = false;
325327
} else {
326328
if (this._sortFn) {
327-
this._applySplicesUserSort(this._splices);
329+
this._applySplicesUserSort(this._keySplices);
328330
} else {
329331
if (this._filterFn) {
330332
// TODK(kschaaf): Filtering using array sort takes slow path
331333
this._applyFullRefresh();
332334
} else {
333-
this._applySplicesArrayOrder(this._splices);
335+
this._applySplicesArrayOrder(this._indexSplices);
334336
}
335337
}
336338
}
337-
this._splices = [];
339+
this._keySplices = [];
340+
this._indexSplices = [];
338341
// Update final _keyToInstIdx and instance indices
339342
var keyToIdx = this._keyToInstIdx = {};
340343
for (var i=0; i<this._instances.length; i++) {
@@ -511,10 +514,10 @@
511514
}
512515
this._instances.splice(s.index, s.removed.length);
513516
// Insert placeholders for new rows
514-
for (var i=0; i<s.added.length; i++) {
517+
for (var i=0; i<s.addedKeys.length; i++) {
515518
var inst = {
516519
isPlaceholder: true,
517-
key: s.added[i]
520+
key: s.addedKeys[i]
518521
};
519522
this._instances.splice(s.index + i, 0, inst);
520523
}

‎test/unit/dom-repeat.html

+33-2
Original file line numberDiff line numberDiff line change
@@ -3092,8 +3092,6 @@ <h4>inDocumentRepeater</h4>
30923092

30933093
});
30943094

3095-
3096-
30973095
suite('item changing on instance', function() {
30983096

30993097
test('primitive values - initial stamping', function() {
@@ -3191,6 +3189,39 @@ <h4>inDocumentRepeater</h4>
31913189

31923190
});
31933191

3192+
suite('external change & notification', function() {
3193+
3194+
test('in-place sort', function(done) {
3195+
var items = [
3196+
173, 166, 145, 755, 907,
3197+
836, 564, 721, 540, 372,
3198+
244, 145, 525, 958, 595,
3199+
207, 103, 602, 769, 190];
3200+
primitive.items = items;
3201+
setTimeout(function() {
3202+
var stamped1 = Polymer.dom(primitive.$.container1).querySelectorAll('*:not(template)');
3203+
for (var i=0; i<items.length; i++) {
3204+
assert.equal(stamped1[i].textContent, items[i]);
3205+
}
3206+
var prev = items.slice();
3207+
items.sort();
3208+
var splices = Polymer.ArraySplice.calculateSplices(items, prev);
3209+
var change = {
3210+
keySplices: Polymer.Collection.applySplices(items, splices),
3211+
indexSplices: splices
3212+
};
3213+
primitive.set('items.splices', change);
3214+
setTimeout(function() {
3215+
var stamped1 = Polymer.dom(primitive.$.container1).querySelectorAll('*:not(template)');
3216+
for (var i=0; i<items.length; i++) {
3217+
assert.equal(stamped1[i].textContent, items[i]);
3218+
}
3219+
done();
3220+
});
3221+
});
3222+
});
3223+
});
3224+
31943225
suite('repeater API', function() {
31953226

31963227
test('modelForElement', function() {

‎test/unit/notify-path.html

+21-28
Original file line numberDiff line numberDiff line change
@@ -728,7 +728,7 @@
728728
el.push('arrayNoColl', 1, 2, 3);
729729
assert.equal(el.observerCounts.arrayNoCollChanged, 2);
730730
assert.equal(Polymer.Collection.get(el.arrayNoColl).getKeys().length, 3);
731-
el.push('arrayNoColl', 1, 2, 3);
731+
el.push('arrayNoColl', 4, 5, 6);
732732
assert.equal(el.observerCounts.arrayNoCollChanged, 3);
733733
assert.equal(Polymer.Collection.get(el.arrayNoColl).getKeys().length, 6);
734734
});
@@ -741,7 +741,7 @@
741741
el.push('array', 1, 2, 3);
742742
assert.equal(el.observerCounts.arrayChanged, 2);
743743
assert.equal(Polymer.Collection.get(el.array).getKeys().length, 3);
744-
el.push('array', 1, 2, 3);
744+
el.push('array', 4, 5, 6);
745745
assert.equal(el.observerCounts.arrayChanged, 3);
746746
assert.equal(Polymer.Collection.get(el.array).getKeys().length, 6);
747747
});
@@ -753,7 +753,7 @@
753753
el.prop = 'first';
754754
el.push('array', 1, 2, 3);
755755
assert.equal(el.observerCounts.arrayOrPropChanged, 1);
756-
el.push('array', 1, 2, 3);
756+
el.push('array', 4, 5, 6);
757757
assert.equal(el.observerCounts.arrayOrPropChanged, 2);
758758
});
759759

@@ -774,10 +774,10 @@
774774
el.pop('data');
775775
el.pop('data');
776776
assert.equal(el.$.boundChild.arrayLength, el.data.length);
777-
el.unshift('data', 1, 2);
778-
el.unshift('data', 1, 2, 3);
777+
el.unshift('data', 4, 5);
778+
el.unshift('data', 6, 7, 8);
779779
assert.equal(el.$.boundChild.arrayLength, el.data.length);
780-
el.splice('data', 2, 2, 1, 2, 3);
780+
el.splice('data', 9, 10, 11, 12, 13);
781781
assert.equal(el.$.boundChild.arrayLength, el.data.length);
782782
el.shift('data');
783783
el.shift('data');
@@ -914,10 +914,9 @@
914914
assert.strictEqual(change.indexSplices[0].addedCount, 2);
915915
assert.strictEqual(change.indexSplices[0].removed.length, 0);
916916
assert.strictEqual(change.keySplices.length, 1);
917-
assert.strictEqual(change.keySplices[0].index, 3);
918917
assert.strictEqual(change.keySplices[0].added.length, 2);
919-
assert.strictEqual(change.keySplices[0].added[0], key);
920-
assert.strictEqual(change.keySplices[0].added[1], key+1);
918+
assert.equal(change.keySplices[0].added[0], key);
919+
assert.equal(change.keySplices[0].added[1], key+1);
921920
assert.strictEqual(change.keySplices[0].removed.length, 0);
922921
};
923922
var ret = el.push('array', 'new1', 'new2');
@@ -942,10 +941,9 @@
942941
assert.strictEqual(change.indexSplices[0].removed.length, 1);
943942
assert.strictEqual(change.indexSplices[0].removed[0], 'orig3');
944943
assert.strictEqual(change.keySplices.length, 1);
945-
assert.strictEqual(change.keySplices[0].index, 2);
946944
assert.strictEqual(change.keySplices[0].added.length, 0);
947945
assert.strictEqual(change.keySplices[0].removed.length, 1);
948-
assert.strictEqual(change.keySplices[0].removed[0], key);
946+
assert.equal(change.keySplices[0].removed[0], key);
949947
};
950948
var ret = el.pop('array');
951949
assert.strictEqual(ret, 'orig3');
@@ -965,10 +963,9 @@
965963
assert.strictEqual(change.indexSplices[0].addedCount, 2);
966964
assert.strictEqual(change.indexSplices[0].removed.length, 0);
967965
assert.strictEqual(change.keySplices.length, 1);
968-
assert.strictEqual(change.keySplices[0].index, 0);
969966
assert.strictEqual(change.keySplices[0].added.length, 2);
970-
assert.strictEqual(change.keySplices[0].added[0], key);
971-
assert.strictEqual(change.keySplices[0].added[1], key+1);
967+
assert.equal(change.keySplices[0].added[0], key);
968+
assert.equal(change.keySplices[0].added[1], key+1);
972969
assert.strictEqual(change.keySplices[0].removed.length, 0);
973970
};
974971
var ret = el.unshift('array', 'new1', 'new2');
@@ -991,11 +988,10 @@
991988
assert.strictEqual(change.indexSplices[0].addedCount, 0);
992989
assert.strictEqual(change.indexSplices[0].removed.length, 1);
993990
assert.strictEqual(change.indexSplices[0].removed[0], 'orig1');
994-
assert.strictEqual(change.keySplices[0].index, 0);
995991
assert.strictEqual(change.keySplices.length, 1);
996992
assert.strictEqual(change.keySplices[0].added.length, 0);
997993
assert.strictEqual(change.keySplices[0].removed.length, 1);
998-
assert.strictEqual(change.keySplices[0].removed[0], 0);
994+
assert.equal(change.keySplices[0].removed[0], 0);
999995
};
1000996
var ret = el.shift('array');
1001997
assert.strictEqual(ret, 'orig1');
@@ -1016,12 +1012,11 @@
10161012
assert.strictEqual(change.indexSplices[0].removed.length, 1);
10171013
assert.strictEqual(change.indexSplices[0].removed[0], 'orig2');
10181014
assert.strictEqual(change.keySplices.length, 1);
1019-
assert.strictEqual(change.keySplices[0].index, 1);
10201015
assert.strictEqual(change.keySplices[0].added.length, 2);
1021-
assert.strictEqual(change.keySplices[0].added[0], key);
1022-
assert.strictEqual(change.keySplices[0].added[1], key+1);
1016+
assert.equal(change.keySplices[0].added[0], key);
1017+
assert.equal(change.keySplices[0].added[1], key+1);
10231018
assert.strictEqual(change.keySplices[0].removed.length, 1);
1024-
assert.strictEqual(change.keySplices[0].removed[0], 1);
1019+
assert.equal(change.keySplices[0].removed[0], 1);
10251020
};
10261021
var ret = el.splice('array', 1, 1, 'new1', 'new2');
10271022
assert.deepEqual(ret, ['orig2']);
@@ -1116,12 +1111,11 @@
11161111
assert.strictEqual(change.indexSplices[0].removed.length, 1);
11171112
assert.strictEqual(change.indexSplices[0].removed[0], 'orig2');
11181113
assert.strictEqual(change.keySplices.length, 1);
1119-
assert.strictEqual(change.keySplices[0].index, 1);
11201114
assert.strictEqual(change.keySplices[0].added.length, 2);
1121-
assert.strictEqual(change.keySplices[0].added[0], key);
1122-
assert.strictEqual(change.keySplices[0].added[1], key+1);
1115+
assert.equal(change.keySplices[0].added[0], key);
1116+
assert.equal(change.keySplices[0].added[1], key+1);
11231117
assert.strictEqual(change.keySplices[0].removed.length, 1);
1124-
assert.strictEqual(change.keySplices[0].removed[0], 1);
1118+
assert.equal(change.keySplices[0].removed[0], 1);
11251119
};
11261120
var ret = el.splice('array', '1', '1', 'new1', 'new2');
11271121
assert.deepEqual(ret, ['orig2']);
@@ -1144,12 +1138,11 @@
11441138
assert.strictEqual(change.indexSplices[0].removed.length, 1);
11451139
assert.strictEqual(change.indexSplices[0].removed[0], 'orig2');
11461140
assert.strictEqual(change.keySplices.length, 1);
1147-
assert.strictEqual(change.keySplices[0].index, 1);
11481141
assert.strictEqual(change.keySplices[0].added.length, 2);
1149-
assert.strictEqual(change.keySplices[0].added[0], key);
1150-
assert.strictEqual(change.keySplices[0].added[1], key+1);
1142+
assert.equal(change.keySplices[0].added[0], key);
1143+
assert.equal(change.keySplices[0].added[1], key+1);
11511144
assert.strictEqual(change.keySplices[0].removed.length, 1);
1152-
assert.strictEqual(change.keySplices[0].removed[0], 1);
1145+
assert.equal(change.keySplices[0].removed[0], 1);
11531146
};
11541147
var ret = el.splice('array', '-2', '1', 'new1', 'new2');
11551148
assert.deepEqual(ret, ['orig2']);

0 commit comments

Comments
 (0)