Skip to content

Commit

Permalink
fix(collectionRepeat): simplify item reusing process to fix rare reus…
Browse files Browse the repository at this point in the history 
…e error

Closes #1777.
  • Loading branch information
@ajoslin
ajoslin committed Aug 13, 2014
1 parent 2d2fd77 commit 8c6d5f2
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 114 deletions.
72 changes: 12 additions & 60 deletions js/angular/service/collectionRepeatDataSource.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,26 +25,14 @@ function($cacheFactory, $parse, $rootScope) {
this.dimensions = [];
this.data = [];

if (this.trackByExpr) {
var trackByGetter = $parse(this.trackByExpr);
var hashFnLocals = {$id: hashKey};
this.itemHashGetter = function(index, value) {
hashFnLocals[self.keyExpr] = value;
hashFnLocals.$index = index;
return trackByGetter(self.scope, hashFnLocals);
};
} else {
this.itemHashGetter = function(index, value) {
return hashKey(value);
};
}

this.attachedItems = {};
this.BACKUP_ITEMS_LENGTH = 10;
this.BACKUP_ITEMS_LENGTH = 20;
this.backupItemsArray = [];
}
CollectionRepeatDataSource.prototype = {
setup: function() {
if (this.isSetup) return;
this.isSetup = true;
for (var i = 0; i < this.BACKUP_ITEMS_LENGTH; i++) {
this.detachItem(this.createItem());
}
Expand All @@ -70,19 +58,18 @@ function($cacheFactory, $parse, $rootScope) {
},
createItem: function() {
var item = {};
item.scope = this.scope.$new();

item.scope = this.scope.$new();
this.transcludeFn(item.scope, function(clone) {
clone.css('position', 'absolute');
item.element = clone;
});

this.transcludeParent.append(item.element);

return item;
},
getItem: function(hash) {
if ( (item = this.attachedItems[hash]) ) {
getItem: function(index) {
if ( (item = this.attachedItems[index]) ) {
//do nothing, the item is good
} else if ( (item = this.backupItemsArray.pop()) ) {
reconnectScope(item.scope);
Expand All @@ -92,20 +79,18 @@ function($cacheFactory, $parse, $rootScope) {
return item;
},
attachItemAtIndex: function(index) {
var value = this.data[index];

if (index < this.dataStartIndex) {
return this.beforeSiblings[index];
} else if (index > this.data.length) {
return this.afterSiblings[index - this.data.length - this.dataStartIndex];
}

var hash = this.itemHashGetter(index, value);
var item = this.getItem(hash);
var item = this.getItem(index);
var value = this.data[index];

if (item.scope.$index !== index || item.scope[this.keyExpr] !== value) {
if (item.index !== index || item.scope[this.keyExpr] !== value) {
item.index = item.scope.$index = index;
item.scope[this.keyExpr] = value;
item.scope.$index = index;
item.scope.$first = (index === 0);
item.scope.$last = (index === (this.getLength() - 1));
item.scope.$middle = !(item.scope.$first || item.scope.$last);
Expand All @@ -116,9 +101,7 @@ function($cacheFactory, $parse, $rootScope) {
item.scope.$digest();
}
}

item.hash = hash;
this.attachedItems[hash] = item;
this.attachedItems[index] = item;

return item;
},
Expand All @@ -129,13 +112,12 @@ function($cacheFactory, $parse, $rootScope) {
item.element = null;
},
detachItem: function(item) {
delete this.attachedItems[item.hash];
delete this.attachedItems[item.index];

//If it's an outside item, only hide it. These items aren't part of collection
//repeat's list, only sit outside
if (item.isOutside) {
hideWithTransform(item.element);

// If we are at the limit of backup items, just get rid of the this element
} else if (this.backupItemsArray.length >= this.BACKUP_ITEMS_LENGTH) {
this.destroyItem(item);
Expand Down Expand Up @@ -167,36 +149,6 @@ function($cacheFactory, $parse, $rootScope) {
return CollectionRepeatDataSource;
}]);

/**
* Computes a hash of an 'obj'.
* Hash of a:
* string is string
* number is number as string
* object is either result of calling $$hashKey function on the object or uniquely generated id,
* that is also assigned to the $$hashKey property of the object.
*
* @param obj
* @returns {string} hash string such that the same input will have the same hash string.
* The resulting string key is in 'type:hashKey' format.
*/
function hashKey(obj) {
var objType = typeof obj,
key;

if (objType == 'object' && obj !== null) {
if (typeof (key = obj.$$hashKey) == 'function') {
// must invoke on object to keep the right this
key = obj.$$hashKey();
} else if (key === undefined) {
key = obj.$$hashKey = ionic.Utils.nextUid();
}
} else {
key = obj;
}

return objType + ':' + key;
}

function disconnectScope(scope) {
if (scope.$root === scope) {
return; // we can't disconnect the root node;
Expand Down
14 changes: 2 additions & 12 deletions js/angular/service/collectionRepeatManager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,7 @@ function($rootScope, $timeout) {
this.beforeSize = result.beforeSize;
this.setCurrentIndex(0);
this.render(true);
if (!this.dataSource.backupItemsArray.length) {
this.dataSource.setup();
}
this.dataSource.setup();
},
/*
* setCurrentIndex sets the index in the list that matches the scroller's position.
Expand Down Expand Up @@ -194,6 +192,7 @@ function($rootScope, $timeout) {
}
return this.scrollView.__$callback(transformLeft, transformTop, zoom, wasResize);
}),

renderIfNeeded: function(scrollPos) {
if ((this.hasNextIndex && scrollPos >= this.nextPos) ||
(this.hasPrevIndex && scrollPos < this.previousPos)) {
Expand Down Expand Up @@ -334,15 +333,6 @@ function($rootScope, $timeout) {
}
};

var exceptions = {'renderScroll':1, 'renderIfNeeded':1};
forEach(CollectionRepeatManager.prototype, function(method, key) {
if (exceptions[key]) return;
CollectionRepeatManager.prototype[key] = function() {
console.log(key + '(', arguments, ')');
return method.apply(this, arguments);
};
});

return CollectionRepeatManager;
}]);

4 changes: 2 additions & 2 deletions test/html/list-fit.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,11 @@ <h2>{{item.text}}</h2>

$scope.loadMore = function() {
$timeout(function() {
var n = 1 + Math.floor(4*Math.random());
var n = 100;//1 + Math.floor(4*Math.random());
for (var i = 0; i < n; i++) addImage();
$scope.$broadcast('scroll.infiniteScrollComplete');
$scope.$broadcast('scroll.refreshComplete');
}, 1500);
}, 1);
};
}
</script>
Expand Down
48 changes: 8 additions & 40 deletions test/unit/angular/service/collectionDataSource.unit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ describe('$collectionDataSource service', function() {
transcludeParent: 3,
keyExpr: 4,
listExpr: 5,
trackByExpr: 6,
heightGetter: 7,
widthGetter: 8
});
Expand All @@ -44,35 +43,10 @@ describe('$collectionDataSource service', function() {
expect(source.transcludeParent).toBe(3);
expect(source.keyExpr).toBe(4);
expect(source.listExpr).toBe(5);
expect(source.trackByExpr).toBe(6);
expect(source.heightGetter).toBe(7);
expect(source.widthGetter).toBe(8);
}));

describe('.itemHashGetter()', function() {

it('should default to hashKey of value', function() {
spyOn(window, 'hashKey').andReturn('hashed');

var source = setup();
expect(source.itemHashGetter(1, 2)).toBe('hashed');
expect(hashKey).toHaveBeenCalledWith(2);
});

it('should use trackbyExpr if provided, and provide locals', function() {
var source = setup({
keyExpr: 'item',
trackByExpr: 'item.idMaker($index, item)'
});
var idMakerSpy = jasmine.createSpy('idMaker').andReturn('superhash');
var item = {
idMaker: idMakerSpy
};
expect(source.itemHashGetter(1, item)).toEqual('superhash');
expect(idMakerSpy).toHaveBeenCalledWith(1, item);
});
});

it('.calculateDataDimensions()', function() {
function widthGetter(scope, locals) {
return locals.$index + locals.item + 'w';
Expand Down Expand Up @@ -119,7 +93,7 @@ describe('$collectionDataSource service', function() {
});

describe('.getItem()', function() {
it('should return attachedItems[hash] if available', function() {
it('should return attachedItems[index] if available', function() {
var source = setup();
var item = {};
source.attachedItems['123'] = item;
Expand Down Expand Up @@ -155,9 +129,6 @@ describe('$collectionDataSource service', function() {
spyOn(source, 'getItem').andCallFake(function() {
return { scope: $rootScope.$new() };
});
spyOn(source, 'itemHashGetter').andCallFake(function(index, value) {
return index + ':' + value;
});

var item1 = source.attachItemAtIndex(0);
expect(item1.scope.value).toEqual('a');
Expand All @@ -166,7 +137,6 @@ describe('$collectionDataSource service', function() {
expect(item1.scope.$last).toBe(false);
expect(item1.scope.$middle).toBe(false);
expect(item1.scope.$odd).toBe(false);
expect(item1.hash).toEqual('0:a');

var item2 = source.attachItemAtIndex(1);
expect(item2.scope.value).toEqual('b');
Expand All @@ -175,7 +145,6 @@ describe('$collectionDataSource service', function() {
expect(item2.scope.$last).toBe(false);
expect(item2.scope.$middle).toBe(true);
expect(item2.scope.$odd).toBe(true);
expect(item2.hash).toEqual('1:b');

var item3 = source.attachItemAtIndex(2);
expect(item3.scope.value).toEqual('c');
Expand All @@ -184,12 +153,11 @@ describe('$collectionDataSource service', function() {
expect(item3.scope.$last).toBe(true);
expect(item3.scope.$middle).toBe(false);
expect(item3.scope.$odd).toBe(false);
expect(item3.hash).toEqual('2:c');

expect(source.attachedItems).toEqual({
'0:a': item1,
'1:b': item2,
'2:c': item3
0: item1,
1: item2,
2: item3
});
}));
});
Expand All @@ -200,10 +168,10 @@ describe('$collectionDataSource service', function() {
var item = {
element: angular.element('<div>'),
scope: {},
hash: 'foo'
index: 1
};
source.backupItemsArray = [];
source.attachedItems[item.hash] = item;
source.attachedItems[1] = item;
spyOn(window, 'disconnectScope');
source.detachItem(item);
expect(source.attachedItems).toEqual({});
Expand All @@ -215,8 +183,8 @@ describe('$collectionDataSource service', function() {
spyOn(source, 'destroyItem');
source.BACKUP_ITEMS_LENGTH = 0;

var item = { hash: 'abc' };
source.attachedItems[item.hash] = item;
var item = { index: 0 };
source.attachedItems[item.index] = item;
source.detachItem(item);
expect(source.destroyItem).toHaveBeenCalledWith(item);
expect(source.attachedItems).toEqual({});
Expand Down

0 comments on commit 8c6d5f2

Please sign in to comment.