mbest / knockout-deferred-updates

Deferred Updates plugin for Knockout <=3.3.0
http://mbest.github.io/knockout-deferred-updates/
134 stars 23 forks source link

Sortable with jQueryUI is broken #2

Closed scottmessinger closed 12 years ago

scottmessinger commented 12 years ago

Sorting is broken when using the deferred-updates.

To duplicate, drag the bottom item to the top: http://jsfiddle.net/scottmessinger/AZqJQ/15/

Screencast duplicating problem http://screencast.com/t/XjjdnF5kh

mbest commented 12 years ago

Interesting.

The sortableList binding first deletes and and then re-inserts the moved item in the array. Normally this invokes two updates to the foreach binding, first deleting and then inserting an item. When using deferred updates, those two updates are combined, and although foreach still does a delete and insert, it may not be of the same item. For example, if we start with 1, 2, 3 and change it to 2, 1, 3, here's what happens normally:

  1. 1, 3 (delete 2)
  2. 2, 1, 3 (insert 2)

Here's what happens when you combine the updates:

  1. 2, 3 (delete 1)
  2. 2, 1, 3 (insert 1)

The end result should be the same, but because the sortableList binding has deleted the element for item 2, the new list just has 1 and 3.

There are two ways to fix this problem. The first is not have sortableList remove the element and let foreach do all the updates. In order for this to work, we have to make sure that foreach only sees the LI element without any text around it: http://jsfiddle.net/mbest/AZqJQ/17/

The second is the make sure that foreach gets two updates. For that we can use ko.processAllDeferredBindingUpdates(): http://jsfiddle.net/mbest/AZqJQ/18/

mbest commented 12 years ago

Obviously a lesson here is that sometimes you really don't want updates to be combined. Unfortunately, there's no way to know automatically when that's the case.

scottmessinger commented 12 years ago

Your second solution worked best (using ko.processAllDeferredBindingUpdates() ). The first solution still caused the problem at times.

Thanks so much!!!!

On Friday, March 2, 2012 at 6:24 PM, Michael Best wrote:

Obviously a lesson here is that sometimes you really don't want updates to be combined. Unfortunately, there's no way to know automatically when that's the case.


Reply to this email directly or view it on GitHub: https://github.com/mbest/knockout-deferred-updates/issues/2#issuecomment-4296120

mbest commented 12 years ago

Your second solution worked best (using ko.processAllDeferredBindingUpdates() ). The first solution still caused the problem at times.

I think the problem might be with the original implementation, because I found problems even with using processAllDeferredBindingUpdates. Here's the problem I found: Drag item one below item two (but don't drop it) and then put it back; drag item three before two (and don't drop it) and put it back; drag item one and put it after item two. Now the list are out of sync. This specific problem doesn't happen with my first solution, rev 17., but does happen with your original fiddle when I remove the deferred updates plugin.

Here's a version that uses processAllDeferredBindingUpdates and also changes the logic of the binding to one that should always work: http://jsfiddle.net/mbest/AZqJQ/20/

scottmessinger commented 12 years ago

Thanks! I just implemented the new fix. It’s working great.

-Scott

On Sunday, March 4, 2012 at 7:20 PM, Michael Best wrote:

Your second solution worked best (using ko.processAllDeferredBindingUpdates() ). The first solution still caused the problem at times.

I think the problem might be with the original implementation, because I found problems even with using processAllDeferredBindingUpdates. Here's the problem I found: Drag item one below item two (but don't drop it) and then put it back; drag item three before two (and don't drop it) and put it back; drag item one and put it after item two. Now the list are out of sync. This specific problem doesn't happen with my first solution, rev 17., but does happen with your original fiddle when I remove the deferred updates plugin.

Here's a version that uses processAllDeferredBindingUpdates and also changes the logic of the binding to one that should always work: http://jsfiddle.net/mbest/AZqJQ/20/


Reply to this email directly or view it on GitHub: https://github.com/mbest/knockout-deferred-updates/issues/2#issuecomment-4314941