search

One-com / knockout-projections

Knockout.js observable arrays get smarter
5 stars 2 forks source link

SortBy projection - changing the sort direction or sort key #1

Open mwoc opened 9 years ago

mwoc commented 9 years ago

I've got a use case for changing the sort direction or the sort key after the data already has been sorted, however that results in an invalid output with some items missing and some duplicated.

The second test in this commit fails as just described: https://github.com/One-com/knockout-projections/commit/463fd04042b545139a5e29bc9e2355aee15c2be6

mwoc commented 9 years ago

The problem is that the SortByProjection keeps using the outputObservable, splicing items in and out of it based on where they are according to the new comparefn, while it should find them with the old comparefn and insert them with the new.

Alternatively an onStructuralChange should be triggered.

mwoc commented 9 years ago

Or after some more testing, the binary search finds the right item for the first 14 changes, then fails 4 times (doesn't find the item at all, returns index -1), and the last 2 are found correctly again.

As we are able to detect failing to find the right item index, we could in that case implicitly trigger a full resort somehow.

mwoc commented 9 years ago

https://github.com/One-com/knockout-projections/tree/feature/change-sort-order now implements automatic re-sorting when the above case is detected.

mwoc commented 9 years ago

Seeing some slowness when testing this on my end, not sure why.

sunesimonsen commented 9 years ago

Let's look at it tomorrow.

AdamWillden commented 9 years ago

I'm experiencing the same issue.

I've just looked and tried the feature branch and it seems to have solved all my issues! Seems quite snappy for changing the sort order on ~300 items!

sunesimonsen commented 9 years ago

Hi @mwoc, is this still an issue after I merged feature/change-sort-order to master?

AdamWillden commented 9 years ago

@sunesimonsen were you notified of my note on this commit? https://github.com/One-com/knockout-projections/commit/7711a5a5d095915fbbbc456e1d7e3b0e876637b1

sunesimonsen commented 9 years ago

@AdamWillden maybe - I'll take a look at it.

sunesimonsen commented 9 years ago

I merged feature/change-sort-order and released a new version, so if you found a problem I'm of cause interested :-)

sunesimonsen commented 9 years ago

I fixed the problem you highlighted in 7711a5a. Thank for finding that.

mwoc commented 9 years ago

Sounds good, I'll re-test with 1.3.1 soon and will close if it's working now.

AdamWillden commented 9 years ago

@sunesimonsen the issue is worse than I first noticed. Look at the new comments I've added in the same commit: https://github.com/One-com/knockout-projections/commit/7711a5a5d095915fbbbc456e1d7e3b0e876637b1

Be worth checking I've not missed any more

sunesimonsen commented 9 years ago

@AdamWillden I responded to the comment inline.