unshiftObjects should maintain the order of the inserted array #764

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@rlivsey
Contributor

rlivsey commented Apr 30, 2012

According to the documentation, unshiftObjects should maintain the order of the inserted array, as such:

var colors = ["red", "green", "blue"];
colors.unshiftObjects(["black", "white"]); => ["black", "white", "red", "green", "blue"]

However, currently the inserted array is added in reverse order because it unshifts one by one from the beginning:

var colors = ["red", "green", "blue"];
colors.unshiftObjects(["black", "white"]); => ["white", "black", "red", "green", "blue"]

I've added failing tests for unshiftObjects by duplicating the unshiftObject ones, then fixed the issue by unshifting the array in reverse order.

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Apr 30, 2012

This pull request fails (merged 1200865 into 8364ed8).

This pull request fails (merged 1200865 into 8364ed8).

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Apr 30, 2012

This pull request passes (merged 0758683 into 8364ed8).

This pull request passes (merged 0758683 into 8364ed8).

@@ -219,7 +219,7 @@ Ember.MutableArray = Ember.Mixin.create(Ember.Array, Ember.MutableEnumerable,
*/
unshiftObjects: function(objects) {
this.beginPropertyChanges();
- forEach(objects, function(obj) { this.unshiftObject(obj); }, this);
+ forEach(objects.slice(0).reverse(), function(obj) { this.unshiftObject(obj); }, this);

This comment has been minimized.

Show comment Hide comment
@wagenet

wagenet Apr 30, 2012

Owner

It seems like this change will make it no longer work with other Ember.Enumerables that don't provide support for slice or reverse.

@wagenet

wagenet Apr 30, 2012

Owner

It seems like this change will make it no longer work with other Ember.Enumerables that don't provide support for slice or reverse.

This comment has been minimized.

Show comment Hide comment
@rlivsey

rlivsey Apr 30, 2012

Contributor

Yeah I wasn't sure of the best approach.

Should it use Array.prototype.slice/reverse instead of calling it on the object directly?

@rlivsey

rlivsey Apr 30, 2012

Contributor

Yeah I wasn't sure of the best approach.

Should it use Array.prototype.slice/reverse instead of calling it on the object directly?

This comment has been minimized.

Show comment Hide comment
@wagenet

wagenet Apr 30, 2012

Owner

The issue is that enumerables aren't necessarily arrays. However, they do implement toArray so it may be sufficient to do objects.toArray().reverse().

@wagenet

wagenet Apr 30, 2012

Owner

The issue is that enumerables aren't necessarily arrays. However, they do implement toArray so it may be sufficient to do objects.toArray().reverse().

This comment has been minimized.

Show comment Hide comment
@rlivsey

rlivsey Apr 30, 2012

Contributor

I've just tried using toArray but that breaks the existing tests with:

Object ember344 has no method 'toArray' - {} 
@rlivsey

rlivsey Apr 30, 2012

Contributor

I've just tried using toArray but that breaks the existing tests with:

Object ember344 has no method 'toArray' - {} 
@wagenet

This comment has been minimized.

Show comment Hide comment
@wagenet

wagenet Apr 30, 2012

Owner

We should write a test that ensures that this continues to work when the "objects" is a basic non-array Ember.Enumerable.

Owner

wagenet commented Apr 30, 2012

We should write a test that ensures that this continues to work when the "objects" is a basic non-array Ember.Enumerable.

@tchak

This comment has been minimized.

Show comment Hide comment
@tchak

tchak Apr 30, 2012

Member

We should add reverse to enumerables

Member

tchak commented Apr 30, 2012

We should add reverse to enumerables

@rlivsey

This comment has been minimized.

Show comment Hide comment
@rlivsey

rlivsey Apr 30, 2012

Contributor

It looks like each unshiftObject call in unshiftObjects causes arrayDidChange for CollectionView to be triggered. So if you unshift 50 objects, arrayDidChange gets triggered 50 times.

Another oddity I've noticed is that the contentIndex of the views prepended in CollectionView#arrayDidChange all get set to 0, and subsequent ones don't have their contentIndex changed.

Just working on extracting an example from my app as a fiddle to illustrate this problem.

Contributor

rlivsey commented Apr 30, 2012

It looks like each unshiftObject call in unshiftObjects causes arrayDidChange for CollectionView to be triggered. So if you unshift 50 objects, arrayDidChange gets triggered 50 times.

Another oddity I've noticed is that the contentIndex of the views prepended in CollectionView#arrayDidChange all get set to 0, and subsequent ones don't have their contentIndex changed.

Just working on extracting an example from my app as a fiddle to illustrate this problem.

@rlivsey

This comment has been minimized.

Show comment Hide comment
@rlivsey

rlivsey Apr 30, 2012

Contributor

Here's a fiddle outlining both the original problem (you can see the objects get prepended in reverse order) and also the issue where contentIndex isn't correctly updated when items are prepended.

http://jsfiddle.net/rlivsey/xyKkb/

Contributor

rlivsey commented Apr 30, 2012

Here's a fiddle outlining both the original problem (you can see the objects get prepended in reverse order) and also the issue where contentIndex isn't correctly updated when items are prepended.

http://jsfiddle.net/rlivsey/xyKkb/

@wagenet

This comment has been minimized.

Show comment Hide comment
@wagenet

wagenet May 3, 2012

Owner

@rlivsey Thanks for looking into this. I ended up diving into this myself and went with a different fix. Let me know if it all seems good to you.

Owner

wagenet commented May 3, 2012

@rlivsey Thanks for looking into this. I ended up diving into this myself and went with a different fix. Let me know if it all seems good to you.

@wagenet wagenet closed this May 3, 2012

@rlivsey

This comment has been minimized.

Show comment Hide comment
@rlivsey

rlivsey May 3, 2012

Contributor

That looks much better and should solve the issue where it was firing an event for each object instead of for the whole set.

Contributor

rlivsey commented May 3, 2012

That looks much better and should solve the issue where it was firing an event for each object instead of for the whole set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment