From 6cf56e7861d2b9564b8ff61386cf27965ceb5cbd Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 19 Jun 2017 17:03:05 +0200 Subject: [PATCH 1/4] Made possible to call View#destroy() multiple times. --- src/view.js | 28 +++++++++++++++++++++++----- tests/view.js | 48 ++++++++++++++++++++++++++++++++++-------------- 2 files changed, 57 insertions(+), 19 deletions(-) diff --git a/src/view.js b/src/view.js index 1600133e..1f52f79b 100644 --- a/src/view.js +++ b/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 Collection from '@ckeditor/ckeditor5-utils/src/collection'; import mix from '@ckeditor/ckeditor5-utils/src/mix'; +import log from '@ckeditor/ckeditor5-utils/src/log'; import isIterable from '@ckeditor/ckeditor5-utils/src/isiterable'; /** @@ -101,6 +102,16 @@ export default class View { */ 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. this._viewCollections.on( 'add', ( evt, collection ) => { collection.locale = locale; @@ -307,15 +318,22 @@ export default class View { * @returns {Promise} A Promise resolved when the destruction process is finished. */ 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(); return Promise.all( this._viewCollections.map( c => c.destroy() ) ) .then( () => { - this._unboundChildren.clear(); - this._viewCollections.clear(); - - this.element = this.template = this.locale = this.t = - this._viewCollections = this._unboundChildren = null; + this._wasDestroyed = true; } ); } diff --git a/tests/view.js b/tests/view.js index 1f5536b2..b8a5cfae 100644 --- a/tests/view.js +++ b/tests/view.js @@ -262,41 +262,55 @@ describe( 'View', () => { beforeEach( createViewWithChildren ); 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( () => { - 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; return view.addChildren( [ new View(), new View() ] ) .then( () => { - expect( cached ).to.have.length.above( 2 ); + expect( cached ).to.have.length( 4 ); 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; expect( cached ).to.have.length( 1 ); 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', () => { + let promise; + view = new View(); expect( () => { - view.destroy(); + promise = view.destroy(); } ).to.not.throw(); + + return promise; } ); // https://github.com/ckeditor/ckeditor5-ui/issues/203 @@ -383,6 +401,8 @@ function setTestViewClass( templateDef ) { constructor() { super(); + this.locale = { t() {} }; + if ( templateDef ) { this.template = new Template( templateDef ); } From 95636ff3fc5539d0e784bf10b4d991bbea4afd22 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 19 Jun 2017 17:16:22 +0200 Subject: [PATCH 2/4] Tests: Added tests to various Views to make sure they are safe to destroy multiple times. --- tests/editableui/editableuiview.js | 10 ++++++++++ tests/editorui/editoruiview.js | 10 ++++++++++ tests/toolbar/contextual/contextualtoolbar.js | 7 +++++++ tests/toolbar/sticky/stickytoolbarview.js | 10 ++++++++++ 4 files changed, 37 insertions(+) diff --git a/tests/editableui/editableuiview.js b/tests/editableui/editableuiview.js index 49bee32e..2d43b590 100644 --- a/tests/editableui/editableuiview.js +++ b/tests/editableui/editableuiview.js @@ -85,6 +85,16 @@ describe( 'EditableUIView', () => { } ); } ); + it( 'can be called multiple times', done => { + expect( () => { + view.destroy().then( () => { + return view.destroy().then( () => { + done(); + } ); + } ); + } ).to.not.throw(); + } ); + describe( 'when #editableElement as an argument', () => { it( 'reverts contentEditable property of editableElement (was false)', () => { editableElement = document.createElement( 'div' ); diff --git a/tests/editorui/editoruiview.js b/tests/editorui/editoruiview.js index ab7a24c1..c85d70c1 100644 --- a/tests/editorui/editoruiview.js +++ b/tests/editorui/editoruiview.js @@ -51,5 +51,15 @@ describe( 'EditorUIView', () => { expect( el.parentNode ).to.be.null; } ); } ); + + it( 'can be called multiple times', done => { + expect( () => { + view.destroy().then( () => { + return view.destroy().then( () => { + done(); + } ); + } ); + } ).to.not.throw(); + } ); } ); } ); diff --git a/tests/toolbar/contextual/contextualtoolbar.js b/tests/toolbar/contextual/contextualtoolbar.js index 77ee9fba..3cde89e9 100644 --- a/tests/toolbar/contextual/contextualtoolbar.js +++ b/tests/toolbar/contextual/contextualtoolbar.js @@ -291,6 +291,13 @@ describe( 'ContextualToolbar', () => { } ); describe( 'destroy()', () => { + it( 'can be called multiple times', () => { + expect( () => { + contextualToolbar.destroy(); + contextualToolbar.destroy(); + } ).to.not.throw(); + } ); + it( 'should not fire `_selectionChangeDebounced` after plugin destroy', done => { const spy = sandbox.spy(); diff --git a/tests/toolbar/sticky/stickytoolbarview.js b/tests/toolbar/sticky/stickytoolbarview.js index 1ebe7fc7..221ca953 100644 --- a/tests/toolbar/sticky/stickytoolbarview.js +++ b/tests/toolbar/sticky/stickytoolbarview.js @@ -178,6 +178,16 @@ describe( 'StickyToolbarView', () => { expect( view.destroy() ).to.be.instanceof( Promise ); } ); + it( 'can be called multiple times', done => { + expect( () => { + view.destroy().then( () => { + return view.destroy().then( () => { + done(); + } ); + } ); + } ).to.not.throw(); + } ); + it( 'calls destroy on parent class', () => { const spy = testUtils.sinon.spy( ToolbarView.prototype, 'destroy' ); From 08314ac1a357c6c06d09c8748285c9cb080ab3d1 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 19 Jun 2017 17:17:19 +0200 Subject: [PATCH 3/4] Made ContextualBalloon#destroy() less intrusive and repeatable, leaving UI destruction to the View and ViewCollection tree. --- src/panel/balloon/contextualballoon.js | 10 ---------- tests/panel/balloon/contextualballoon.js | 12 +++++++++--- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/panel/balloon/contextualballoon.js b/src/panel/balloon/contextualballoon.js index aed4a67f..dc406761 100644 --- a/src/panel/balloon/contextualballoon.js +++ b/src/panel/balloon/contextualballoon.js @@ -205,14 +205,4 @@ export default class ContextualBalloon extends Plugin { _getBalloonPosition() { return this._stack.values().next().value.position; } - - /** - * @inheritDoc - */ - destroy() { - this.editor.ui.view.body.remove( this.view ); - this.view.destroy(); - this._stack.clear(); - super.destroy(); - } } diff --git a/tests/panel/balloon/contextualballoon.js b/tests/panel/balloon/contextualballoon.js index 991ef44a..fd5849d0 100644 --- a/tests/panel/balloon/contextualballoon.js +++ b/tests/panel/balloon/contextualballoon.js @@ -352,11 +352,17 @@ describe( 'ContextualBalloon', () => { } ); describe( 'destroy()', () => { - it( 'should remove balloon panel view from editor body collection and clear stack', () => { + it( 'can be called multiple times', () => { + expect( () => { + balloon.destroy(); + balloon.destroy(); + } ).to.not.throw(); + } ); + + it( 'should not touch the DOM', () => { balloon.destroy(); - expect( editor.ui.view.body.getIndex( balloon.view ) ).to.equal( -1 ); - expect( balloon.visibleView ).to.null; + expect( editor.ui.view.body.getIndex( balloon.view ) ).to.not.equal( -1 ); } ); } ); } ); From 38c00ab122edd2c745474a64ad47d3d3919b1664 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Tue, 20 Jun 2017 20:21:42 +0200 Subject: [PATCH 4/4] Removed unnecessary warning. --- src/view.js | 27 +-------------------------- 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/src/view.js b/src/view.js index 1f52f79b..9e3a4252 100644 --- a/src/view.js +++ b/src/view.js @@ -14,7 +14,6 @@ import DomEmmiterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin'; import Collection from '@ckeditor/ckeditor5-utils/src/collection'; import mix from '@ckeditor/ckeditor5-utils/src/mix'; -import log from '@ckeditor/ckeditor5-utils/src/log'; import isIterable from '@ckeditor/ckeditor5-utils/src/isiterable'; /** @@ -102,16 +101,6 @@ export default class View { */ 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. this._viewCollections.on( 'add', ( evt, collection ) => { collection.locale = locale; @@ -318,23 +307,9 @@ export default class View { * @returns {Promise} A Promise resolved when the destruction process is finished. */ 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(); - return Promise.all( this._viewCollections.map( c => c.destroy() ) ) - .then( () => { - this._wasDestroyed = true; - } ); + return Promise.all( this._viewCollections.map( c => c.destroy() ) ); } /**