rniemeyer / knockout-sortable

A Knockout.js binding to connect observableArrays with jQuery UI sortable functionality
MIT License
548 stars 128 forks source link

Bug when binding to an observable array containing _destroyed elements #75

Open nmancini411 opened 10 years ago

nmancini411 commented 10 years ago

I was implementing a UI with a numbered list of items. These items are stored in an observable array and bound with a knockout-sortable binding. I've got logic in my view models to ensure that the list items remain correctly numbered as items are added to the list or sorted to other positions in the list.

I noticed a bug in knockout-sortable when I added support for deleting items (using the _destroy property on my view model). Under certain conditions, when I dragged and reordered my list, I noticed that the numbering became incorrect.

After some research, I determined the bug was occurring in the updateIndexFromDestroyedItems function, but only when the index that's passed in happens to coincide with the index of a _destroyed element in the observable array. In that case, the index is not correctly incremented to the correct value and this appears to cause some subtle corruption in the observable array.

I was able to provide the following fix locally that worked for me:

var updateIndexFromDestroyedItems = function(index, items) { var unwrapped = unwrap(items);

    if (unwrapped) {
        for (var i = 0; i < index; i++) {
            //add one for every destroyed item we find before the targetIndex
            if (unwrapped[i] && unwrap(unwrapped[i]._destroy)) {
                index++;
            }
        }

        // begin bug fix

        // if index points to a destroyed item, find next non-destroyed item
        var i = index;
        while (i < unwrapped.length && 
                 unwrapped[i] && unwrap(unwrapped[i]._destroy)) {
          i++;
          index++;
        }

       // end bug fix
    }

    return index;
};
rniemeyer commented 10 years ago

@nmancini411 - thanks for this one. I will take a look when I get a chance, but your solution sounds reasonable!