8
0
Просмотр исходного кода

Ensured corret item removal in case of 2-w data binding.

Aleksander Nowodzinski 8 лет назад
Родитель
Сommit
94a102c655

+ 22 - 9
packages/ckeditor5-utils/src/collection.js

@@ -65,6 +65,15 @@ export default class Collection {
 		 */
 		this._bindToExternalToInternalMap = new Map();
 
+		/**
+		 * A helper mapping items of this collection to external items of a bound collection
+		 * ({@link #bindTo}).
+		 *
+		 * @protected
+		 * @member {Map}
+		 */
+		this._bindToInternalToExternalMap = new Map();
+
 		/**
 		 * A collection instance this collection is bound to as a result
 		 * of calling {@link #bindTo} method.
@@ -234,6 +243,10 @@ export default class Collection {
 		this._items.splice( index, 1 );
 		this._itemMap.delete( id );
 
+		const externalItem = this._bindToInternalToExternalMap.get( item );
+		this._bindToInternalToExternalMap.delete( item );
+		this._bindToExternalToInternalMap.delete( externalItem );
+
 		this.fire( 'remove', item );
 
 		return item;
@@ -419,21 +432,20 @@ export default class Collection {
 		// @private
 		const addItem = ( evt, externalItem, index ) => {
 			const isExternalBoundToThis = externalCollection._bindToCollection == this;
-			const isExternalItemBound = [
-					...externalCollection._bindToExternalToInternalMap.values()
-				].indexOf( externalItem ) > -1;
+			const externalItemBoundTo = externalCollection._bindToInternalToExternalMap.get( externalItem );
+			const item = externalItemBoundTo || factory( externalItem );
+
+			this._bindToExternalToInternalMap.set( externalItem, item );
+			this._bindToInternalToExternalMap.set( item, externalItem );
 
 			// If an external collection is bound to this collection, which makes it a 2–way binding,
 			// and the particular external collection item is already bound, don't add it here.
 			// The external item has been created **out of this collection's item** and (re)adding it will
 			// cause a loop.
-			if ( isExternalBoundToThis && isExternalItemBound ) {
+			if ( isExternalBoundToThis && externalItemBoundTo ) {
 				return;
 			}
 
-			const item = factory( externalItem );
-
-			this._bindToExternalToInternalMap.set( externalItem, item );
 			this.add( item, index );
 		};
 
@@ -449,8 +461,9 @@ export default class Collection {
 		this.listenTo( externalCollection, 'remove', ( evt, externalItem ) => {
 			const item = this._bindToExternalToInternalMap.get( externalItem );
 
-			this._bindToExternalToInternalMap.delete( externalItem );
-			this.remove( item );
+			if ( item ) {
+				this.remove( item );
+			}
 		} );
 	}
 

+ 54 - 0
packages/ckeditor5-utils/tests/collection.js

@@ -857,6 +857,60 @@ describe( 'Collection', () => {
 				sinon.assert.callCount( spyB, 3 );
 				sinon.assert.callCount( spyC, 4 );
 			} );
+
+			it( 'removes items correctly', () => {
+				const collectionA = new Collection();
+				const collectionB = new Collection();
+
+				const spyAddA = sinon.spy();
+				const spyAddB = sinon.spy();
+				const spyRemoveA = sinon.spy();
+				const spyRemoveB = sinon.spy();
+
+				collectionA.on( 'add', spyAddA );
+				collectionB.on( 'add', spyAddB );
+				collectionA.on( 'remove', spyRemoveA );
+				collectionB.on( 'remove', spyRemoveB );
+
+				// A<--->B
+				collectionA.bindTo( collectionB ).using( 'data' );
+				collectionB.bindTo( collectionA ).as( FactoryClass );
+
+				collectionA.add( { value: 'foo' } );
+				collectionA.add( { value: 'bar' } );
+
+				expect( collectionA.map( i => i.value ) ).to.deep.equal( [ 'foo', 'bar' ], 'CollectionA' );
+				expect( collectionB.map( i => i ).every( isFactoryClass ) ).to.be.true;
+				expect( collectionB.map( i => i.data.value ) ).to.deep.equal( [ 'foo', 'bar' ], 'CollectionB' );
+
+				collectionB.add( new FactoryClass( { value: 'baz' } ) );
+
+				expect( collectionA.map( i => i.value ) ).to.deep.equal( [ 'foo', 'bar', 'baz' ], 'CollectionA' );
+				expect( collectionB.map( i => i ).every( isFactoryClass ) ).to.be.true;
+				expect( collectionB.map( i => i.data.value ) ).to.deep.equal( [ 'foo', 'bar', 'baz' ], 'CollectionB' );
+
+				collectionB.remove( 0 );
+
+				expect( collectionA.map( i => i.value ) ).to.deep.equal( [ 'bar', 'baz' ], 'CollectionA' );
+				expect( collectionB.map( i => i ).every( isFactoryClass ) ).to.be.true;
+				expect( collectionB.map( i => i.data.value ) ).to.deep.equal( [ 'bar', 'baz' ], 'CollectionB' );
+
+				sinon.assert.callCount( spyAddA, 3 );
+				sinon.assert.callCount( spyAddB, 3 );
+				sinon.assert.callCount( spyRemoveA, 1 );
+				sinon.assert.callCount( spyRemoveB, 1 );
+
+				collectionA.remove( 1 );
+
+				expect( collectionA.map( i => i.value ) ).to.deep.equal( [ 'bar' ], 'CollectionA' );
+				expect( collectionB.map( i => i ).every( isFactoryClass ) ).to.be.true;
+				expect( collectionB.map( i => i.data.value ) ).to.deep.equal( [ 'bar' ], 'CollectionB' );
+
+				sinon.assert.callCount( spyAddA, 3 );
+				sinon.assert.callCount( spyAddB, 3 );
+				sinon.assert.callCount( spyRemoveA, 2 );
+				sinon.assert.callCount( spyRemoveB, 2 );
+			} );
 		} );
 	} );