Skip to content

Commit

Permalink
Merge pull request #9356 from ef4/fix-9278
Browse files Browse the repository at this point in the history 
[BUGFIX beta] fixing broken computed.sort semantics
  • Loading branch information
@stefanpenner
stefanpenner committed Oct 21, 2014
2 parents 62e9c49 + ac184cf commit 7b270f8
Show file tree
Hide file tree
Showing 2 changed files with 184 additions and 48 deletions.
118 changes: 75 additions & 43 deletions packages/ember-runtime/lib/computed/reduce_computed_macros.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
*/

import Ember from 'ember-metal/core'; // Ember.assert
import merge from 'ember-metal/merge';
import { get } from 'ember-metal/property_get';
import {
isArray,
Expand All @@ -18,7 +17,6 @@ import run from 'ember-metal/run_loop';
import { addObserver } from 'ember-metal/observer';
import { arrayComputed } from 'ember-runtime/computed/array_computed';
import { reduceComputed } from 'ember-runtime/computed/reduce_computed';
import ObjectProxy from 'ember-runtime/system/object_proxy';
import SubArray from 'ember-runtime/system/subarray';
import keys from 'ember-metal/keys';
import compare from 'ember-runtime/compare';
Expand Down Expand Up @@ -600,8 +598,8 @@ function binarySearch(array, item, low, high) {
mid = low + Math.floor((high - low) / 2);
midItem = array.objectAt(mid);

guidMid = _guidFor(midItem);
guidItem = _guidFor(item);
guidMid = guidFor(midItem);
guidItem = guidFor(item);

if (guidMid === guidItem) {
return mid;
Expand All @@ -621,19 +619,9 @@ function binarySearch(array, item, low, high) {
}

return mid;

function _guidFor(item) {
if (SearchProxy.detectInstance(item)) {
return guidFor(get(item, 'content'));
}

return guidFor(item);
}
}


var SearchProxy = ObjectProxy.extend();

/**
A computed property which returns a new array with all the
properties from the first dependent array sorted based on a property
Expand Down Expand Up @@ -702,25 +690,58 @@ export function sort(itemsKey, sortDefinition) {
Ember.assert('Ember.computed.sort requires two arguments: an array key to sort and ' +
'either a sort properties key or sort function', arguments.length === 2);

var initFn, sortPropertiesKey;

if (typeof sortDefinition === 'function') {
initFn = function (array, changeMeta, instanceMeta) {
instanceMeta.order = sortDefinition;
instanceMeta.binarySearch = binarySearch;
};
return customSort(itemsKey, sortDefinition);
} else {
sortPropertiesKey = sortDefinition;
return propertySort(itemsKey, sortDefinition);
}
}

function customSort(itemsKey, comparator) {
return arrayComputed(itemsKey, {
initialize: function (array, changeMeta, instanceMeta) {
instanceMeta.order = comparator;
instanceMeta.binarySearch = binarySearch;
instanceMeta.waitingInsertions = [];
instanceMeta.insertWaiting = function() {
var index, item;
var waiting = instanceMeta.waitingInsertions;
instanceMeta.waitingInsertions = [];
for (var i=0; i<waiting.length; i++) {
item = waiting[i];
index = instanceMeta.binarySearch(array, item);
array.insertAt(index, item);
}
};
instanceMeta.insertLater = function(item) {
this.waitingInsertions.push(item);
run.once(this, 'insertWaiting');
};
},

addedItem: function (array, item, changeMeta, instanceMeta) {
instanceMeta.insertLater(item);
return array;
},

removedItem: function (array, item, changeMeta, instanceMeta) {
array.removeObject(item);
return array;
}
});
}

initFn = function (array, changeMeta, instanceMeta) {
function propertySort(itemsKey, sortPropertiesKey) {
return arrayComputed(itemsKey, {
initialize: function (array, changeMeta, instanceMeta) {
function setupSortProperties() {
var sortPropertyDefinitions = get(this, sortPropertiesKey);
var sortProperties = instanceMeta.sortProperties = [];
var sortPropertyAscending = instanceMeta.sortPropertyAscending = {};
var sortProperty, idx, asc;

Ember.assert('Cannot sort: \'' + sortPropertiesKey + '\' is not an array.',
isArray(sortPropertyDefinitions));
isArray(sortPropertyDefinitions));

changeMeta.property.clearItemPropertyKeys(itemsKey);

Expand Down Expand Up @@ -754,12 +775,14 @@ export function sort(itemsKey, sortDefinition) {
setupSortProperties.call(this);

instanceMeta.order = function (itemA, itemB) {
var isProxy = itemB instanceof SearchProxy;
var sortProperty, result, asc;
var keyA = this.keyFor(itemA);
var keyB = this.keyFor(itemB);

for (var i = 0; i < this.sortProperties.length; ++i) {
sortProperty = this.sortProperties[i];
result = compare(get(itemA, sortProperty), isProxy ? itemB[sortProperty] : get(itemB, sortProperty));

result = compare(keyA[sortProperty], keyB[sortProperty]);

if (result !== 0) {
asc = this.sortPropertyAscending[sortProperty];
Expand All @@ -771,34 +794,43 @@ export function sort(itemsKey, sortDefinition) {
};

instanceMeta.binarySearch = binarySearch;
};
}

return arrayComputed(itemsKey, {
initialize: initFn,
setupKeyCache(instanceMeta);
},

addedItem: function (array, item, changeMeta, instanceMeta) {
var index = instanceMeta.binarySearch(array, item);
array.insertAt(index, item);

return array;
},

removedItem: function (array, item, changeMeta, instanceMeta) {
var proxyProperties, index, searchItem;

if (changeMeta.previousValues) {
proxyProperties = merge({ content: item }, changeMeta.previousValues);

searchItem = SearchProxy.create(proxyProperties);
} else {
searchItem = item;
}

index = instanceMeta.binarySearch(array, searchItem);
var index = instanceMeta.binarySearch(array, item);
array.removeAt(index);

instanceMeta.dropKeyFor(item);
return array;
}
});
}

function setupKeyCache(instanceMeta) {
instanceMeta.keyFor = function(item) {
var guid = guidFor(item);
if (this.keyCache[guid]) {
return this.keyCache[guid];
}
var sortProperty;
var key = {};
for (var i = 0; i < this.sortProperties.length; ++i) {
sortProperty = this.sortProperties[i];
key[sortProperty] = get(item, sortProperty);
}
return this.keyCache[guid] = key;
};

instanceMeta.dropKeyFor = function(item) {
var guid = guidFor(item);
this.keyCache[guid] = null;
};

instanceMeta.keyCache = {};
}
114 changes: 109 additions & 5 deletions packages/ember-runtime/tests/computed/reduce_computed_macros_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,7 @@ function commonSortTests() {
deepEqual(sorted.mapBy('fname'), ['Cersei', 'Jaime', 'Bran', 'Robb'], "sorted array is updated");
});

test("guid sort-order fallback with a serach proxy is not confused by non-search ObjectProxys", function() {
test("guid sort-order fallback with a search proxy is not confused by non-search ObjectProxys", function() {
var tyrion = { fname: "Tyrion", lname: "Lannister" };
var tyrionInDisguise = ObjectProxy.create({
fname: "Yollo",
Expand All @@ -779,9 +779,9 @@ function commonSortTests() {
});

items = get(obj, 'items');
sorted = get(obj, 'sortedItems');

run(function() {
sorted = get(obj, 'sortedItems');
items.pushObject(tyrion);
});

Expand Down Expand Up @@ -1103,6 +1103,108 @@ test("changing item properties not specified via @each does not trigger a resort
deepEqual(sorted.mapBy('fname'), ['Cersei', 'Jaime', 'Bran', 'Robb'], "updating an unspecified property on an item does not resort it");
});

QUnit.module('computedSort - stability', {
setup: function() {
run(function() {
obj = EmberObject.createWithMixins({
items: Ember.A(Ember.A([{
name: "A", count: 1
}, {
name: "B", count: 1
}, {
name: "C", count: 1
}, {
name: "D", count: 1
}]).map(function(elt){return EmberObject.create(elt);})),

sortProps: Ember.A(['count', 'name']),
sortedItems: computedSort('items', 'sortProps')
});
});
},
teardown: function() {
run(function() {
obj.destroy();
});
}
});

test("sorts correctly as only one property changes", function(){
var sorted;
run(function() {
sorted = obj.get('sortedItems');
});
deepEqual(sorted.mapBy('name'), ['A', 'B', 'C', 'D'], "initial");
obj.get('items').objectAt(3).set('count', 2);
run(function() {
sorted = obj.get('sortedItems');
});
deepEqual(sorted.mapBy('name'), ['A', 'B', 'C', 'D'], "final");
});

QUnit.module('computedSort - concurrency', {
setup: function() {
run(function() {
obj = EmberObject.createWithMixins({
items: Ember.A(Ember.A([{
name: "A", count: 1
}, {
name: "B", count: 2
}, {
name: "C", count: 3
}, {
name: "D", count: 4
}]).map(function(elt){return EmberObject.create(elt);})),

sortProps: Ember.A(['count']),
sortedItems: computedSort('items', 'sortProps'),
customSortedItems: computedSort('[email protected]', function(a,b){
return get(a, 'count') - get(b, 'count');
})
});
});
},
teardown: function() {
run(function() {
obj.destroy();
});
}
});

test("sorts correctly when there are concurrent changes", function(){
var sorted;
run(function() {
sorted = obj.get('sortedItems');
});
deepEqual(sorted.mapBy('name'), ['A', 'B', 'C', 'D'], "initial");
Ember.changeProperties(function(){
obj.get('items').objectAt(1).set('count', 5);
obj.get('items').objectAt(2).set('count', 6);
});
run(function() {
sorted = obj.get('sortedItems');
});
deepEqual(sorted.mapBy('name'), ['A', 'D', 'B', 'C'], "final");
});

test("sorts correctly with a user-provided comparator when there are concurrent changes", function(){
var sorted;
run(function() {
sorted = obj.get('customSortedItems');
});
deepEqual(sorted.mapBy('name'), ['A', 'B', 'C', 'D'], "initial");
run(function() {
Ember.changeProperties(function(){
obj.get('items').objectAt(1).set('count', 5);
obj.get('items').objectAt(2).set('count', 6);
});
sorted = obj.get('customSortedItems');
});
deepEqual(sorted.mapBy('name'), ['A', 'D', 'B', 'C'], "final");
});



QUnit.module('computedMax', {
setup: function() {
run(function() {
Expand Down Expand Up @@ -1311,9 +1413,11 @@ QUnit.module('Ember.arrayComputed - chains', {
});

test("it can filter and sort when both depend on the same item property", function() {
filtered = get(obj, 'filtered');
sorted = get(obj, 'sorted');
todos = get(obj, 'todos');
run(function(){
filtered = get(obj, 'filtered');
sorted = get(obj, 'sorted');
todos = get(obj, 'todos');
});

deepEqual(todos.mapProperty('name'), ['E', 'D', 'C', 'B', 'A'], "precond - todos initially correct");
deepEqual(sorted.mapProperty('name'), ['A', 'B', 'C', 'D', 'E'], "precond - sorted initially correct");
Expand Down

0 comments on commit 7b270f8

Please sign in to comment.