Skip to content
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.

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

SortableMixin does not work when whole array changes #3706

Closed
meurkens opened this issue Nov 7, 2013 · 12 comments
Closed

SortableMixin does not work when whole array changes #3706

meurkens opened this issue Nov 7, 2013 · 12 comments
Assignees
Labels

Comments

@meurkens
Copy link

meurkens commented Nov 7, 2013

I have an ArrayController of models, whose property which is the sortablePropertie is computed and can change at once, because it is dependent on a single property of another model they all belongTo.

When this happens, every object gets inserted again, but the binary search algorithm does not function properly anymore, because the whole Array became unsorted.

@trek
Copy link
Member

trek commented Nov 7, 2013

Can you create a a JSFiddle or JSBin that demonstrates this problem in isolation?

@meurkens
Copy link
Author

meurkens commented Nov 8, 2013

Something like this:
http://jsbin.com/ucanam/2024/edit?html,js,output

@wagenet
Copy link
Member

wagenet commented Nov 9, 2013

Hmm, that seems odd. @hjdivad, can you chime in here?

@ghost ghost assigned hjdivad Nov 9, 2013
@hjdivad
Copy link
Member

hjdivad commented Nov 11, 2013

both SortableMixin and Em.computed.sort fall down on this case, although for slightly different reasons.

SortableMixin

When an item's sort property changes, SortableMixin checks to see if the item is still sorted by comparing it to its left and right neighbours, and if the item is not sorted, reinserts it. This check fails when the entire array is unsorted due to batch updates, as the array will then have been sorted by prior values. In the case of this jsbin the sort is arbitrary because the initial values are all NaN.

Em.computed.sort

Em.computed.sort comes a little closer, but not much. In an attempt to handle multiple property changes (from eg setProperties) Em.computed.sort batches changes. Unfortunately, the cache is not invalidated until didChange.

Fixing SortableMixin

Fixing SortableMixin is not too much code, but it does require adding some complexity.

  • add a contentItemSortPropertyWillChange, decide there whether or not to remove the item, mark it
  • in contentItemSortPropertyDidChange, if the item is marked, reinsert it.

Fixing Em.computed.sort

sort needs to:

  • only flush one item at a time, ie batch changes to multiple properties, but not to multiple items and
  • only flush on the last property didChange, not the first.

@wagenet
Copy link
Member

wagenet commented Apr 15, 2014

@hjdivad anything actionable here?

@hjdivad
Copy link
Member

hjdivad commented Apr 15, 2014

@wagenet yeah I need to fix the two things I listed above for Em.computed.sort; thanks for the reminder

@mlb5000
Copy link

mlb5000 commented Jul 2, 2014

just ran into this issue in 1.6.0-beta.5. have you made any progress on this in a branch anywhere @hjdivad ?

@wagenet
Copy link
Member

wagenet commented Aug 6, 2014

@hjdivad ping.

@jwahdatehagh
Copy link

any news on this?

or workarounds?

@mlb5000
Copy link

mlb5000 commented Sep 10, 2014

@jwahdatehagh I can't confirm whether it's a fluke or actually a workaround, but I tried this again today and instead of pointing to 'model' in the Ember.computed.sort call I point at a computed property. In my case that computed property happens to filter the controller's content, but I don't know if that's strictly necessary. Maybe you'll have some luck that way?

@jwahdatehagh
Copy link

thanks @mlb5000 - i just opted out of this now - using the standard sortProperties option on the model itself with a weird workaround for sorting the properties in different directions now - but everything seems to work so far.

@wagenet
Copy link
Member

wagenet commented Nov 1, 2014

From what @hjdivad said, the core issue is around Ember.computed.sort. Merging into #9462.

@wagenet wagenet closed this as completed Nov 1, 2014
@wagenet wagenet mentioned this issue Nov 1, 2014
7 tasks
stefanpenner added a commit that referenced this issue Jun 23, 2015
#9492, #5319, #5268, #4831] Move away from AC/RC instead use the simpler naive enumerable methods, and rely on glimmers stable rendering for efficiency.

For more complex scenarios, custom solutions should be used.

@wagenet & @stefanpenner
stefanpenner added a commit to stefanpenner/ember.js that referenced this issue Jun 26, 2015
…rjs#9462, emberjs#4919, emberjs#4231, emberjs#3706, emberjs#5596, emberjs#9485, emberjs#9492, emberjs#5319, emberjs#5268, emberjs#4831, emberjs#5558] Move away from AC/RC instead use the simpler naive enumerable methods, and rely on glimmers stable rendering for efficiency.

For more complex scenarios, custom solutions should be used.

@wagenet & @stefanpenner
stefanpenner added a commit that referenced this issue Jun 28, 2015
#5596, #9485, #9492, #5319, #5268, #4831, #5558] Move away from AC/RC instead use the simpler naive enumerable methods, and rely on glimmers stable rendering for efficiency.

For more complex scenarios, custom solutions should be used.

@wagenet & @stefanpenner

(cherry picked from commit 0dc1a6c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants