Fix bug when setting both sortProperties and sortAscending in SortableMixin #3422

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@Mitek99

Mitek99 commented Sep 15, 2013

I'm setting both sortProperties and sortAscending using one call

sortedArrayController.setProperties({ sortProperties: ['id'], sortAscending: true});

The arrangedContent gets sorted with new value for sortAscending, and then gets reversed, when the sortAscendingDidChange observer kicks in. To prevent this, _lastSortAscending needs to be set when the array is sorted

@alexspeller

This comment has been minimized.

Show comment Hide comment
@alexspeller

alexspeller Sep 16, 2013

Contributor

Actually I think the correct fix for this is to wrap this observer in an Em.run.once as it's firing synchronously and we want it to fire after the properties are finished updating

Contributor

alexspeller commented Sep 16, 2013

Actually I think the correct fix for this is to wrap this observer in an Em.run.once as it's firing synchronously and we want it to fire after the properties are finished updating

@stefanpenner

This comment has been minimized.

Show comment Hide comment
@stefanpenner

stefanpenner Sep 26, 2013

Owner

@Mitek99 thought on @alexspeller suggestion?

(Cc @hjdivad)

Owner

stefanpenner commented Sep 26, 2013

@Mitek99 thought on @alexspeller suggestion?

(Cc @hjdivad)

@wagenet

This comment has been minimized.

Show comment Hide comment
@wagenet

wagenet Sep 26, 2013

Owner

@alexspeller it looks like @Mitek99 may have let this one go. Is this something you'd be interested in submitting a PR for?

Owner

wagenet commented Sep 26, 2013

@alexspeller it looks like @Mitek99 may have let this one go. Is this something you'd be interested in submitting a PR for?

@Mitek99

This comment has been minimized.

Show comment Hide comment
@Mitek99

Mitek99 Sep 26, 2013

I'm new to Ember and it takes some time to digest what you guys are suggesting. If somebody can fix this faster - go on. And I heard that SortableMixin is going to be deprecated anyway...

Mitek99 commented Sep 26, 2013

I'm new to Ember and it takes some time to digest what you guys are suggesting. If somebody can fix this faster - go on. And I heard that SortableMixin is going to be deprecated anyway...

@hjdivad

This comment has been minimized.

Show comment Hide comment
@hjdivad

hjdivad Sep 26, 2013

Member

It would be nice to either deprecate SortableMixin or change the implementation to just use Em.computed.sort. I looked briefly into the latter and it would require a couple of additions to Em.computed.sort like adding support for indirect user-specified sort functions (ie retrieving the sort function from a property).

Member

hjdivad commented Sep 26, 2013

It would be nice to either deprecate SortableMixin or change the implementation to just use Em.computed.sort. I looked briefly into the latter and it would require a couple of additions to Em.computed.sort like adding support for indirect user-specified sort functions (ie retrieving the sort function from a property).

@stefanpenner

This comment has been minimized.

Show comment Hide comment
@stefanpenner

stefanpenner Jan 14, 2014

Owner

@alexspeller or @hjdivad either of your got a chance to help out on this one?

Owner

stefanpenner commented Jan 14, 2014

@alexspeller or @hjdivad either of your got a chance to help out on this one?

@hjdivad

This comment has been minimized.

Show comment Hide comment
@hjdivad

hjdivad Jan 17, 2014

Member

@stefanpenner I'm going to try and replace SortableMixin once I finish computed.literal and it makes it in

Member

hjdivad commented Jan 17, 2014

@stefanpenner I'm going to try and replace SortableMixin once I finish computed.literal and it makes it in

@stefanpenner

This comment has been minimized.

Show comment Hide comment
@stefanpenner

stefanpenner Jan 17, 2014

Owner

@hjdivad 👍

Owner

stefanpenner commented Jan 17, 2014

@hjdivad 👍

@nathanhammond

This comment has been minimized.

Show comment Hide comment
@nathanhammond

nathanhammond Jan 30, 2014

Member

Hey, so I guess it doesn't make sense for me to continue my work on extending SortableMixin if we're going to deprecate it in favor of Em.computed.sort ... I started a discussion earlier on this, but most valuable in that thread is the list of sorting problems that I was trying to solve for:
http://discuss.emberjs.com/t/extend-ember-sortablemixin-to-allow-sorting-multiple-properties-in-varying-orders/3753/8?u=nathanhammond

@hjdivad Let me know if you want any of my code. :)

Member

nathanhammond commented Jan 30, 2014

Hey, so I guess it doesn't make sense for me to continue my work on extending SortableMixin if we're going to deprecate it in favor of Em.computed.sort ... I started a discussion earlier on this, but most valuable in that thread is the list of sorting problems that I was trying to solve for:
http://discuss.emberjs.com/t/extend-ember-sortablemixin-to-allow-sorting-multiple-properties-in-varying-orders/3753/8?u=nathanhammond

@hjdivad Let me know if you want any of my code. :)

@hjdivad

This comment has been minimized.

Show comment Hide comment
@hjdivad

hjdivad Feb 8, 2014

Member

replacing SortableMixin is essentially blocked on #4185.

Member

hjdivad commented Feb 8, 2014

replacing SortableMixin is essentially blocked on #4185.

@hjdivad

This comment has been minimized.

Show comment Hide comment
@hjdivad

hjdivad Feb 8, 2014

Member

@nathanhammond tests will certainly be useful. Once #4185 is in I should be able to replace SortableMixin (or rather, deprecate it and replace its internals). It would definitely be useful for you to review that future PR and point out remaining deficiencies, supply tests &c. And of course, to fix all the bugs I'll include. ^_^

Alternatively, once #4185 is in, I can give you some guidance on doing the SortableMixin replacement if you'd like to make the contribution.

Member

hjdivad commented Feb 8, 2014

@nathanhammond tests will certainly be useful. Once #4185 is in I should be able to replace SortableMixin (or rather, deprecate it and replace its internals). It would definitely be useful for you to review that future PR and point out remaining deficiencies, supply tests &c. And of course, to fix all the bugs I'll include. ^_^

Alternatively, once #4185 is in, I can give you some guidance on doing the SortableMixin replacement if you'd like to make the contribution.

@nathanhammond

This comment has been minimized.

Show comment Hide comment
@nathanhammond

nathanhammond Feb 8, 2014

Member

@hjdivad I'll be involved either way–I have one hell of a use case for it and I'm tired of my controllers looking like Excel. I'm saving off so many copies of properties in order to marshal them into the sort functionality I need...

I've subscribed to #4185 and once that lands I'll dive in (intentionally using that as a delay so that I can get things in order beforehand). We can chat as to how we want to divide the effort, I've got a number of good examples already written for test cases and documentation.

Member

nathanhammond commented Feb 8, 2014

@hjdivad I'll be involved either way–I have one hell of a use case for it and I'm tired of my controllers looking like Excel. I'm saving off so many copies of properties in order to marshal them into the sort functionality I need...

I've subscribed to #4185 and once that lands I'll dive in (intentionally using that as a delay so that I can get things in order beforehand). We can chat as to how we want to divide the effort, I've got a number of good examples already written for test cases and documentation.

@wagenet

This comment has been minimized.

Show comment Hide comment
@wagenet

wagenet Apr 12, 2014

Owner

@nathanhammond: looks like #4185 won't be landing. Thoughts?

Owner

wagenet commented Apr 12, 2014

@nathanhammond: looks like #4185 won't be landing. Thoughts?

@nathanhammond

This comment has been minimized.

Show comment Hide comment
@nathanhammond

nathanhammond Apr 12, 2014

Member

@wagenet I love that you take the time to dig through the old bugs. :) Thank you for that effort.

I'll take this one and make sure we've got the right solution in place. Updated PR within the week.

Member

nathanhammond commented Apr 12, 2014

@wagenet I love that you take the time to dig through the old bugs. :) Thank you for that effort.

I'll take this one and make sure we've got the right solution in place. Updated PR within the week.

@nathanhammond

This comment has been minimized.

Show comment Hide comment
@nathanhammond

nathanhammond Apr 12, 2014

Member

I believe that we should deprecate SortableMixin so I don't want to do much work on it as is. @hjdivad and I are in a holding pattern until @tomdale pushes updates to canal.js. In the interim I want to patch this bug non-invasively, my PR will probably look like what @alexspeller describes.

Notes for me:
Test Case

Member

nathanhammond commented Apr 12, 2014

I believe that we should deprecate SortableMixin so I don't want to do much work on it as is. @hjdivad and I are in a holding pattern until @tomdale pushes updates to canal.js. In the interim I want to patch this bug non-invasively, my PR will probably look like what @alexspeller describes.

Notes for me:
Test Case

@wagenet

This comment has been minimized.

Show comment Hide comment
@wagenet

wagenet Apr 12, 2014

Owner

@tomdale what's the status on your end?

Owner

wagenet commented Apr 12, 2014

@tomdale what's the status on your end?

@nathanhammond

This comment has been minimized.

Show comment Hide comment
@nathanhammond

nathanhammond Apr 14, 2014

Member

Notes:

  • Test coverage from @Mitek99's test cases is incomplete, assumes toggling of sortAscending every time instead of arbitrarily setting it.
  • Patch proposed by @Mitek99 makes property order inside the setProperties object important.

(@Mitek99, just notes, I'm grateful for the effort you've gone through!)

Member

nathanhammond commented Apr 14, 2014

Notes:

  • Test coverage from @Mitek99's test cases is incomplete, assumes toggling of sortAscending every time instead of arbitrarily setting it.
  • Patch proposed by @Mitek99 makes property order inside the setProperties object important.

(@Mitek99, just notes, I'm grateful for the effort you've gone through!)

@wagenet wagenet closed this in #4722 Apr 15, 2014

rwjblue added a commit that referenced this pull request Apr 23, 2014

Complete set of failing tests.
SortableMixin doesn't currently work if sortAscending and sortProperties are set at the same time.
Failing tests either copied from or based off of #3422
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment