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

Fixed: ControllerCollection binds improperly to non–Model data with a custom factory.

Aleksander Nowodzinski 9 лет назад
Родитель
Сommit
9d223b242a

+ 55 - 54
packages/ckeditor5-ui/src/controllercollection.js

@@ -140,8 +140,8 @@ export default class ControllerCollection extends Collection {
 	 *		data.remove( 0 );
 	 *		console.log( controllers.length == 1 );
 	 *
-	 * @param {utils.Collection.<ui.Model>} models Models to be synchronized with this controller collection.
-	 * @returns {Function} The `as` function in the `bind( models ).as( ... )` chain.
+	 * @param {utils.Collection.<ui.Model>} collection Models to be synchronized with this controller collection.
+	 * @returns {Function} The `as` function in the `bind( collection ).as( ... )` chain.
 	 * It activates factory using controller and view classes or uses a custom callback to produce
 	 * controller (view) instances.
 	 * @param {Function} return.ControllerClassOrFunction Specifies the constructor of the controller to be used or
@@ -149,76 +149,77 @@ export default class ControllerCollection extends Collection {
 	 * @param {Function} [return.ViewClass] Specifies constructor of the view to be used. If not specified,
 	 * `ControllerClassOrFunction` works as as custom callback function.
 	 */
-	bind( models ) {
-		const idProperty = models._idProperty;
+	bind( collection ) {
+		const controllerMap = new Map();
 
 		return {
 			as: ( ControllerClassOrFunction, ViewClass ) => {
-				const createController = ViewClass ?
-						defaultControllerFactory( ControllerClassOrFunction, ViewClass, idProperty )
+				const factory = ViewClass ?
+						defaultControllerFactory( ControllerClassOrFunction, ViewClass, controllerMap )
 					:
-						customControllerFactory( ControllerClassOrFunction, idProperty );
+						genericControllerFactory( ControllerClassOrFunction, controllerMap );
 
-				for ( let model of models ) {
-					this.add( createController( model, this.locale ) );
+				for ( let item of collection ) {
+					this.add( factory.create( item, this.locale ) );
 				}
 
-				// Updated controller collection when a new model is added.
-				models.on( 'add', ( evt, model, index ) => {
-					this.add( createController( model, this.locale ), index );
+				// Updated controller collection when a new item is added.
+				collection.on( 'add', ( evt, item, index ) => {
+					this.add( factory.create( item, this.locale ), index );
 				} );
 
-				// Update controller collection when a model is removed.
-				models.on( 'remove', ( evt, model ) => {
-					this.remove( model[ idProperty ] );
+				// Update controller collection when a item is removed.
+				collection.on( 'remove', ( evt, item ) => {
+					this.remove( factory.delete( item ) );
 				} );
 			}
 		};
 	}
 }
 
-// Initializes controller factory with controller and view classes.
-//
-// @param {Function} ControllerClass Specifies the constructor of the controller to be used.
-// @param {Function} ViewClass Specifies constructor of the view.
-// @param {String} idProperty A property used to associate the controller with its model {@link utils.Collection._idProperty}.
-// @returns {Function}
-function defaultControllerFactory( ControllerClass, ViewClass, idProperty ) {
-	// Returns a controller instance (and its view) for given model and class names.
-	//
-	// @param {ui.Model} model A model of the controller.
-	// @param {utils.Locale} [locale] The {@link ckeditor5.Editor#locale editor's locale} instance.
-	// @returns {ui.Controller}
-	return ( model, locale ) => {
-		return flagController( new ControllerClass( model, new ViewClass( locale ) ), idProperty );
-	};
-}
-
-// Initializes controller factory which is fed by a custom callback.
+// Initializes a generic controller factory.
 //
-// @param {Function} callback A callback which is to return an instance of {@link ui.Controller}.
-// @param {String} idProperty A property used to associate the controller with its model {@link utils.Collection._idProperty}.
-// @returns {Function}
-function customControllerFactory( callback, idProperty ) {
-	// Returns a controller instance (and its view) produced by the custom callback.
-	//
-	// @param {ui.Model} model A model of the controller.
-	// @param {utils.Locale} [locale] The {@link ckeditor5.Editor#locale editor's locale} instance.
-	// @returns {ui.Controller}
-	return ( ...args ) => {
-		return flagController( callback( ...args ), idProperty );
+// @param {Function} createController A function which returns controller instance if provided with data and locale.
+// @param {Map} controllerMap A Map used to associate data in the collection with corresponding controller instance.
+// @returns {Object}
+function genericControllerFactory( createController, controllerMap ) {
+	return {
+		// Returns a controller instance (and its view) for given data (i.e. Model) and locale.
+		//
+		// @param {Object} data A data to creates the controller, usually {@link ui.Model}.
+		// @param {utils.Locale} [locale] The {@link ckeditor5.Editor#locale editor's locale} instance.
+		// @returns {ui.Controller}
+		create( data, locale ) {
+			const controller = createController( data, locale );
+
+			controllerMap.set( data, controller );
+
+			return controller;
+		},
+
+		// Deletes controller instance in the factory by associated data and returns
+		// the controller instance.
+		//
+		// @param {Object} data A data associated with the controller, usually {@link ui.Model}.
+		// @returns {ui.Controller}
+		delete( data ) {
+			const controller = controllerMap.get( data );
+
+			controllerMap.delete( controller );
+
+			return controller;
+		}
 	};
 }
 
-// Gives the controller an id corresponding with {@link utils.Collection#_idProperty} of the model.
-// It allows retrieving this controller instance by the model in the future
-// and avoids a brute–force search in the entire controller collection.
+// Initializes controller factory with controller and view classes.
 //
-// @param {ui.Controller} controller An instance of controller.
-// @param {String} idProperty A property used to associate the controller with its model {@link utils.Collection._idProperty}.
-// @returns {ui.Controller}
-function flagController( controller, idProperty ) {
-	controller.id = controller.model[ idProperty ];
-
-	return controller;
+// @param {Function} ControllerClass Specifies the constructor of the controller to be used.
+// @param {Function} ViewClass Specifies constructor of the view.
+// @param {Map} controllerMap A Map used to associate data in the collection with corresponding controller instance.
+// @returns {Object}
+function defaultControllerFactory( ControllerClass, ViewClass, controllerMap ) {
+	return genericControllerFactory( ( model, locale ) => {
+		return new ControllerClass( model, new ViewClass( locale ) );
+	}, controllerMap );
 }

+ 26 - 2
packages/ckeditor5-ui/tests/controllercollection.js

@@ -166,7 +166,7 @@ describe( 'ControllerCollection', () => {
 					models.remove( 2 );
 					models.remove( 3 );
 
-					expect( controllers.map( c => c.id ) ).to.have.members( [ '0', '1', '3' ] );
+					expect( controllers.map( c => c.model.uid ) ).to.have.members( [ '0', '1', '3' ] );
 				} );
 
 				it( 'passes controller collection\'s locale to the views', () => {
@@ -228,7 +228,7 @@ describe( 'ControllerCollection', () => {
 					models.remove( 2 );
 					models.remove( 3 );
 
-					expect( controllers.map( c => c.id ) ).to.have.members( [ '0', '1', '3' ] );
+					expect( controllers.map( c => c.model.uid ) ).to.have.members( [ '0', '1', '3' ] );
 				} );
 
 				it( 'passes controller collection\'s locale to the views', () => {
@@ -242,6 +242,30 @@ describe( 'ControllerCollection', () => {
 					expect( controllers.get( 0 ).view.locale ).to.equal( locale );
 				} );
 			} );
+
+			describe( 'custom data format with custom factory', () => {
+				it( 'expands the initial collection of the models', () => {
+					const controllers = new ControllerCollection( 'synced' );
+					const data = new Collection();
+
+					data.add( { foo: 'a' } );
+					data.add( { foo: 'b' } );
+
+					controllers.bind( data ).as( ( item, locale ) => {
+						const model = new Model( {
+							custom: item.foo
+						} );
+
+						return new ItemController( model, new ItemView( locale ) );
+					} );
+
+					expect( controllers ).to.have.length( 2 );
+					expect( controllers.get( 0 ).model.custom ).to.equal( 'a' );
+					expect( controllers.get( 1 ).model.custom ).to.equal( 'b' );
+					expect( controllers.get( 0 ) ).to.be.instanceOf( ItemController );
+					expect( controllers.get( 0 ).view ).to.be.instanceOf( ItemView );
+				} );
+			} );
 		} );
 	} );
 } );