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

Add an Ember.SortedArrayProxy class (attempt 2) #803

Closed
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@oldfartdeveloper
Contributor

oldfartdeveloper commented May 10, 2012

This PR pulls the App.contactsController's 4 methods to keep the ArrayProxy sorted:

  • add
  • binarySearch
  • remove
  • contactSortValueDidChange

into a new wrapper class around ArrayProxy. The methods are generalized. If you indicate you will accept this pull request, I will be glad to update the App.contactsController to make the example easier to understand.

I've earnestly tried to put everything I can think of that's needed into automated regression tests and the API docs. I hope you like it.

Thanks.

OldFartDeveloper w/ help from Drew Purdy and James Gary.

Note: this is "attempt 2" after I inadvertently submitted the previous attempt as an empty pull request (d'oh)

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot May 10, 2012

This pull request passes (merged 29f0292 into 8d0b0b3).

travisbot commented May 10, 2012

This pull request passes (merged 29f0292 into 8d0b0b3).

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet May 10, 2012

Member

Is it possible for you to rebase on master and squash some of these commits down to something a bit more manageable?

Member

wagenet commented May 10, 2012

Is it possible for you to rebase on master and squash some of these commits down to something a bit more manageable?

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak May 10, 2012

Member

@oldfartdeveloper where the idea is good, there is some problems in my opinion :

  • it should be a mixin
  • we should not manipulate content, but rather expose sorted version via proxy api
  • we should stick to enumerable api
  • the sortValue property approach is to "simple". No way to specify the direction for example. The base should be orderBy method and recomputation should be done using * observers then they are implemented
  • There should be a beter way to recompute the position then remove + add

Anyway, thanks a lot for this attempt, I am using a modified version in my app for now :)
https://gist.github.com/2652287

Member

tchak commented May 10, 2012

@oldfartdeveloper where the idea is good, there is some problems in my opinion :

  • it should be a mixin
  • we should not manipulate content, but rather expose sorted version via proxy api
  • we should stick to enumerable api
  • the sortValue property approach is to "simple". No way to specify the direction for example. The base should be orderBy method and recomputation should be done using * observers then they are implemented
  • There should be a beter way to recompute the position then remove + add

Anyway, thanks a lot for this attempt, I am using a modified version in my app for now :)
https://gist.github.com/2652287

@oldfartdeveloper

This comment has been minimized.

Show comment
Hide comment
@oldfartdeveloper

oldfartdeveloper May 10, 2012

Contributor

@wagenet I'll squash and resubmit. I thought you would only look at the 3 added files and the one line added to the existing 4th. Thanks for the feedback.

Contributor

oldfartdeveloper commented May 10, 2012

@wagenet I'll squash and resubmit. I thought you would only look at the 3 added files and the one line added to the existing 4th. Thanks for the feedback.

@oldfartdeveloper

This comment has been minimized.

Show comment
Hide comment
@oldfartdeveloper

oldfartdeveloper May 10, 2012

Contributor

@tchak Thanks for your feedback. I agree with your points, especially that it needs to have bidirectionality.

I looked at your gist, and I'd like to incorporate it into a new submission w/ a test suite and the documentation. I'll go ahead and do so if you haven't already (or aren't planning to soon). Can you let me know?

Contributor

oldfartdeveloper commented May 10, 2012

@tchak Thanks for your feedback. I agree with your points, especially that it needs to have bidirectionality.

I looked at your gist, and I'd like to incorporate it into a new submission w/ a test suite and the documentation. I'll go ahead and do so if you haven't already (or aren't planning to soon). Can you let me know?

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak May 10, 2012

Member

@oldfartdeveloper I dont think it should be submitted to core in it current state.
I know that some of the points I mentioned are not trivial, but I think we need to sort it out before considering inclusion of this

Member

tchak commented May 10, 2012

@oldfartdeveloper I dont think it should be submitted to core in it current state.
I know that some of the points I mentioned are not trivial, but I think we need to sort it out before considering inclusion of this

@oldfartdeveloper

This comment has been minimized.

Show comment
Hide comment
@oldfartdeveloper

oldfartdeveloper May 10, 2012

Contributor

@tchak That's what I was suggesting. ;-) I'll address your points, resubmit, we can re-evaluate, etc. I simply want to keep this moving; it's an important feature.

Contributor

oldfartdeveloper commented May 10, 2012

@tchak That's what I was suggesting. ;-) I'll address your points, resubmit, we can re-evaluate, etc. I simply want to keep this moving; it's an important feature.

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak May 10, 2012

Member

@oldfartdeveloper ok
I think you should start by trying to move it to a mixin.

Can we have some input on how you see this feature from @tomdale and @wycats ?
Can we do it without * observer? The way I do it by keeping a list of concerned properties would not work with generic orderBy function...

Member

tchak commented May 10, 2012

@oldfartdeveloper ok
I think you should start by trying to move it to a mixin.

Can we have some input on how you see this feature from @tomdale and @wycats ?
Can we do it without * observer? The way I do it by keeping a list of concerned properties would not work with generic orderBy function...

@oldfartdeveloper

This comment has been minimized.

Show comment
Hide comment
@oldfartdeveloper

oldfartdeveloper May 10, 2012

Contributor

@tchak - Thanks, I'll give it a try as a mixin. I have some ideas w/ the orderBy function I'll pass by you; I'd appreciate your feedback.

Need advice:

  1. Should I close this PR now? The new submission, when it comes, will be substantially different. Do people comment on closed requests?
  2. Leave it open only for further comments on how to implement, then close when those are obtained?
  3. Leave it open as the design moves closer to what the team wants? Will rebasing/squashing work on an existing PR?
Contributor

oldfartdeveloper commented May 10, 2012

@tchak - Thanks, I'll give it a try as a mixin. I have some ideas w/ the orderBy function I'll pass by you; I'd appreciate your feedback.

Need advice:

  1. Should I close this PR now? The new submission, when it comes, will be substantially different. Do people comment on closed requests?
  2. Leave it open only for further comments on how to implement, then close when those are obtained?
  3. Leave it open as the design moves closer to what the team wants? Will rebasing/squashing work on an existing PR?
@tomdale

This comment has been minimized.

Show comment
Hide comment
@tomdale

tomdale May 14, 2012

Member

@oldfartdeveloper Thank you for your work on this! This feature is sorely needed.

Let's close this PR for now as it helps us keep a better handle on what actively needs integration. It is common for closed pull requests to still attract discussion.

I agree with all of @tchak's points above. This should definitely act on a sorted proxy copy of the data, and not modify the contents of the underlying array.

Rather than relying on a hypothetical star observer, it seems like it is necessary to explicitly enumerate which properties of the objects in the array are being used in the sorting method. (This is the same reason you must manually enumerate the dependencies of a computed property, and so I believe the same solution should apply for consistency. Also, the * observer implementation will probably be much slower than a specific property observer and may cause surprising performance problems if applied to many objects at once.)

Member

tomdale commented May 14, 2012

@oldfartdeveloper Thank you for your work on this! This feature is sorely needed.

Let's close this PR for now as it helps us keep a better handle on what actively needs integration. It is common for closed pull requests to still attract discussion.

I agree with all of @tchak's points above. This should definitely act on a sorted proxy copy of the data, and not modify the contents of the underlying array.

Rather than relying on a hypothetical star observer, it seems like it is necessary to explicitly enumerate which properties of the objects in the array are being used in the sorting method. (This is the same reason you must manually enumerate the dependencies of a computed property, and so I believe the same solution should apply for consistency. Also, the * observer implementation will probably be much slower than a specific property observer and may cause surprising performance problems if applied to many objects at once.)

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet May 14, 2012

Member

Closing, per @tomdale's direction.

Member

wagenet commented May 14, 2012

Closing, per @tomdale's direction.

@wagenet wagenet closed this May 14, 2012

@oldfartdeveloper

This comment has been minimized.

Show comment
Hide comment
@oldfartdeveloper

oldfartdeveloper May 14, 2012

Contributor

I'm cool with this. I'll try another submission with the points addressed as noted above. Thanks!

Contributor

oldfartdeveloper commented May 14, 2012

I'm cool with this. I'll try another submission with the points addressed as noted above. Thanks!

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