浏览代码

Made possible to call View#destroy() multiple times.

Aleksander Nowodzinski 8 年之前
父节点
当前提交
2cc47077ad
共有 2 个文件被更改,包括 57 次插入19 次删除
  1. 23 5
      packages/ckeditor5-ui/src/view.js
  2. 34 14
      packages/ckeditor5-ui/tests/view.js

+ 23 - 5
packages/ckeditor5-ui/src/view.js

@@ -14,6 +14,7 @@ import DomEmmiterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin';
 import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
 import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
 import Collection from '@ckeditor/ckeditor5-utils/src/collection';
 import Collection from '@ckeditor/ckeditor5-utils/src/collection';
 import mix from '@ckeditor/ckeditor5-utils/src/mix';
 import mix from '@ckeditor/ckeditor5-utils/src/mix';
+import log from '@ckeditor/ckeditor5-utils/src/log';
 import isIterable from '@ckeditor/ckeditor5-utils/src/isiterable';
 import isIterable from '@ckeditor/ckeditor5-utils/src/isiterable';
 
 
 /**
 /**
@@ -101,6 +102,16 @@ export default class View {
 		 */
 		 */
 		this._unboundChildren = this.createCollection();
 		this._unboundChildren = this.createCollection();
 
 
+		/**
+		 * Specifies whether the instance was destroyed using {@link #destroy} method
+		 * in the past.
+		 *
+		 * @private
+		 * @readonly
+		 * @member {Boolean}
+		 */
+		this._wasDestroyed = false;
+
 		// Pass parent locale to its children.
 		// Pass parent locale to its children.
 		this._viewCollections.on( 'add', ( evt, collection ) => {
 		this._viewCollections.on( 'add', ( evt, collection ) => {
 			collection.locale = locale;
 			collection.locale = locale;
@@ -307,15 +318,22 @@ export default class View {
 	 * @returns {Promise} A Promise resolved when the destruction process is finished.
 	 * @returns {Promise} A Promise resolved when the destruction process is finished.
 	 */
 	 */
 	destroy() {
 	destroy() {
+		/**
+		 * The view has already been destroyed. If you see this warning, it means that some piece
+		 * of code attempted to destroy it again, which usually may (but not must) be a symptom of
+		 * a broken destruction logic in a code that uses this view instance.
+		 *
+		 * @error ui-view-destroy-again
+		 */
+		if ( this._wasDestroyed ) {
+			log.warn( 'ui-view-destroy-again: The view has already been destroyed.', { view: this } );
+		}
+
 		this.stopListening();
 		this.stopListening();
 
 
 		return Promise.all( this._viewCollections.map( c => c.destroy() ) )
 		return Promise.all( this._viewCollections.map( c => c.destroy() ) )
 			.then( () => {
 			.then( () => {
-				this._unboundChildren.clear();
-				this._viewCollections.clear();
-
-				this.element = this.template = this.locale = this.t =
-					this._viewCollections = this._unboundChildren = null;
+				this._wasDestroyed = true;
 			} );
 			} );
 	}
 	}
 
 

+ 34 - 14
packages/ckeditor5-ui/tests/view.js

@@ -262,41 +262,55 @@ describe( 'View', () => {
 		beforeEach( createViewWithChildren );
 		beforeEach( createViewWithChildren );
 
 
 		it( 'should return a promise', () => {
 		it( 'should return a promise', () => {
-			expect( view.destroy() ).to.be.instanceof( Promise );
+			const promise = view.destroy();
+
+			expect( promise ).to.be.instanceof( Promise );
+
+			return promise;
+		} );
+
+		it( 'can be called multiple times', done => {
+			expect( () => {
+				view.destroy().then( () => {
+					return view.destroy().then( () => {
+						done();
+					} );
+				} );
+			} ).to.not.throw();
 		} );
 		} );
 
 
-		it( 'should set basic properties null', () => {
+		it( 'should not touch the basic properties', () => {
 			return view.destroy().then( () => {
 			return view.destroy().then( () => {
-				expect( view.element ).to.be.null;
-				expect( view.template ).to.be.null;
-				expect( view.locale ).to.be.null;
-				expect( view.t ).to.be.null;
+				expect( view.element ).to.be.an.instanceof( HTMLElement );
+				expect( view.template ).to.be.an.instanceof( Template );
+				expect( view.locale ).to.be.an( 'object' );
+				expect( view.locale.t ).to.be.a( 'function' );
 
 
-				expect( view._unboundChildren ).to.be.null;
-				expect( view._viewCollections ).to.be.null;
+				expect( view._viewCollections ).to.be.instanceOf( Collection );
+				expect( view._unboundChildren ).to.be.instanceOf( ViewCollection );
 			} );
 			} );
 		} );
 		} );
 
 
-		it( 'clears #_unboundChildren', () => {
+		it( 'should not clear the #_unboundChildren', () => {
 			const cached = view._unboundChildren;
 			const cached = view._unboundChildren;
 
 
 			return view.addChildren( [ new View(), new View() ] )
 			return view.addChildren( [ new View(), new View() ] )
 				.then( () => {
 				.then( () => {
-					expect( cached ).to.have.length.above( 2 );
+					expect( cached ).to.have.length( 4 );
 
 
 					return view.destroy().then( () => {
 					return view.destroy().then( () => {
-						expect( cached ).to.have.length( 0 );
+						expect( cached ).to.have.length( 4 );
 					} );
 					} );
 				} );
 				} );
 		} );
 		} );
 
 
-		it( 'clears #_viewCollections', () => {
+		it( 'should not clear the #_viewCollections', () => {
 			const cached = view._viewCollections;
 			const cached = view._viewCollections;
 
 
 			expect( cached ).to.have.length( 1 );
 			expect( cached ).to.have.length( 1 );
 
 
 			return view.destroy().then( () => {
 			return view.destroy().then( () => {
-				expect( cached ).to.have.length( 0 );
+				expect( cached ).to.have.length( 1 );
 			} );
 			} );
 		} );
 		} );
 
 
@@ -326,11 +340,15 @@ describe( 'View', () => {
 		} );
 		} );
 
 
 		it( 'destroy a template–less view', () => {
 		it( 'destroy a template–less view', () => {
+			let promise;
+
 			view = new View();
 			view = new View();
 
 
 			expect( () => {
 			expect( () => {
-				view.destroy();
+				promise = view.destroy();
 			} ).to.not.throw();
 			} ).to.not.throw();
+
+			return promise;
 		} );
 		} );
 
 
 		// https://github.com/ckeditor/ckeditor5-ui/issues/203
 		// https://github.com/ckeditor/ckeditor5-ui/issues/203
@@ -383,6 +401,8 @@ function setTestViewClass( templateDef ) {
 		constructor() {
 		constructor() {
 			super();
 			super();
 
 
+			this.locale = { t() {} };
+
 			if ( templateDef ) {
 			if ( templateDef ) {
 				this.template = new Template( templateDef );
 				this.template = new Template( templateDef );
 			}
 			}