SortedArray.prototype.swap dispatches "after" changes out of order, reports duplicate "plus" elements, and invalid "index"

[fredkilbourn]

The changes to the model are correct, it is just the dispatching that is not reporting those changes correctly.

Using this test code based on the api doc example:

require( "collections/shim-object" );
var SortedArraySet = require( "collections/sorted-array-set" );

var array = new SortedArraySet( [ 1, 2, 3 ] );
array.addRangeChangeListener( function( plus, minus, index )
{
    console.log( "added:", plus, "removed:", minus, "at", index );
    console.log( array.toJSON() );
} );

console.log( array.toJSON() );
array.push( 4 );
array.splice( 1, 2, 5 );

Is currently outputting:

[1, 2, 3]
added: [4] removed: [] at 3
[1, 2, 3, 4]
added: [5] removed: [] at 2
[1, 4, 5]
added: [5] removed: [2, 3] at 1
[1, 4, 5]

Note that 5 is reported to be added twice, and at the wrong index the second time and that 2,3 were removed from the model one dispatch before they were reported to be removed.

I applied this patch:

--- sorted-array.old.js Thu Jan 22 12:47:17 2015
+++ sorted-array.js Thu Jan 22 13:25:26 2015
@@ -263,10 +263,10 @@
         this.dispatchBeforeRangeChange(plus, minus, index);
     }
     this.array.splice(index, length);
-    this.addEach(plus);
     if (this.dispatchesRangeChanges) {
-        this.dispatchRangeChange(plus, minus, index);
+        this.dispatchRangeChange([], minus, index);
     }
+    this.addEach(plus);
     return minus;
 };

Which changes the output to:

[1, 2, 3]
added: [4] removed: [] at 3
[1, 2, 3, 4]
added: [] removed: [2, 3] at 1
[1, 4]
added: [5] removed: [] at 2
[1, 4, 5]

[marchant]

Thanks Fred, could you do a pull request? I'm not sure it makes sense to move the "this.addEach(plus)" down to after the this.dispatchesRangeChanges(), because when observers get it, the collection wouldn't have changed yet

[fredkilbourn]

I didn't dig into it to actually see the code, but this.addEach(plus) seems to fire this.dispatchesRangeChanges() internally for each element it adds.

I don't actually know how to submit a pull request :confused: if you could handle integrating it that'd be great, if not let me know and I'll try to sort it out.

[marchant]

You should learn how to do a PR, very handy, but I'm taking care of it

[fredkilbourn]

Figured it out, submitted https://github.com/montagejs/collections/pull/106 which also addresses one additional issue with the length not being maintained for the collection.

[fredkilbourn]

Pull request was merged, closed.