From 2933e1c9e8acfaf92748bc86dcaa403137d55d66 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Jul 2017 16:01:54 +0200 Subject: [PATCH 01/14] Made View and ViewCollection classes synchronous. --- src/view.js | 55 +++++----- src/viewcollection.js | 45 +------- tests/view.js | 223 +++++++++------------------------------- tests/viewcollection.js | 185 +++++++-------------------------- 4 files changed, 114 insertions(+), 394 deletions(-) diff --git a/src/view.js b/src/view.js index 9e3a4252..cb8bf86f 100644 --- a/src/view.js +++ b/src/view.js @@ -43,10 +43,10 @@ import isIterable from '@ckeditor/ckeditor5-utils/src/isiterable'; * * const view = new SampleView( locale ); * - * view.init().then( () => { - * // Will append

Helloworld

- * document.body.appendChild( view.element ); - * } ); + * view.init(); + * + * // Will append

Helloworld

+ * document.body.appendChild( view.element ); * * @mixes module:utils/observablemixin~ObservableMixin */ @@ -190,14 +190,14 @@ export default class View { * const view = new SampleView( locale ); * const anotherView = new AnotherSampleView( locale ); * - * view.init().then( () => { - * // Will append

- * document.body.appendChild( view.element ); + * view.init(); + * + * // Will append

+ * document.body.appendChild( view.element ); * - * // `anotherView` becomes a child of the view, which is reflected in DOM - * //

- * view.items.add( anotherView ); - * } ); + * // `anotherView` becomes a child of the view, which is reflected in DOM + * //

+ * view.items.add( anotherView ); * * @returns {module:ui/viewcollection~ViewCollection} A new collection of view instances. */ @@ -237,10 +237,10 @@ export default class View { * * const view = new SampleView( locale ); * - * view.init().then( () => { - * // Will append

- * document.body.appendChild( view.element ); - * } ); + * view.init(); + * + * // Will append

+ * document.body.appendChild( view.element ); * * **Note**: There's no need to add child views if they're used in the * {@link #template} explicitly: @@ -265,20 +265,17 @@ export default class View { * } * * @param {module:ui/view~View|Iterable.} children Children views to be registered. - * @returns {Promise} */ addChildren( children ) { if ( !isIterable( children ) ) { children = [ children ]; } - return Promise.all( children.map( c => this._unboundChildren.add( c ) ) ); + children.map( c => this._unboundChildren.add( c ) ); } /** * Initializes the view and child views located in {@link #_viewCollections}. - * - * @returns {Promise} A Promise resolved when the initialization process is finished. */ init() { if ( this.ready ) { @@ -290,26 +287,20 @@ export default class View { throw new CKEditorError( 'ui-view-init-reinit: This View has already been initialized.' ); } - return Promise.resolve() - // Initialize collections in #_viewCollections. - .then( () => { - return Promise.all( this._viewCollections.map( c => c.init() ) ); - } ) - // Spread the word that this view is ready! - .then( () => { - this.ready = true; - } ); + // Initialize collections in #_viewCollections. + this._viewCollections.map( c => c.init() ); + + // Spread the word that this view is ready! + this.ready = true; } /** - * Destroys the view instance and child views located in {@link #_viewCollections}. - * - * @returns {Promise} A Promise resolved when the destruction process is finished. +f * Destroys the view instance and child views located in {@link #_viewCollections}. */ destroy() { this.stopListening(); - return Promise.all( this._viewCollections.map( c => c.destroy() ) ); + this._viewCollections.map( c => c.destroy() ); } /** diff --git a/src/viewcollection.js b/src/viewcollection.js index 3e5e042a..f0b24c03 100644 --- a/src/viewcollection.js +++ b/src/viewcollection.js @@ -69,23 +69,11 @@ export default class ViewCollection extends Collection { * @member {HTMLElement} */ this._parentElement = null; - - /** - * A set containing promises created by {@link #add}. If the {@link #destroy} is called - * before the child views' {@link module:ui/view~View#init} are completed, - * {@link #destroy} will wait until all the promises are resolved. - * - * @private - * @member {Set} - */ - this._addPromises = new Set(); } /** * Initializes child views by injecting {@link module:ui/view~View#element} into DOM * and calling {@link module:ui/view~View#init}. - * - * @returns {Promise} A Promise resolved when the initialization process is finished. */ init() { if ( this.ready ) { @@ -97,25 +85,16 @@ export default class ViewCollection extends Collection { throw new CKEditorError( 'ui-viewcollection-init-reinit: This ViewCollection has already been initialized.' ); } - return Promise.all( this.map( v => v.init() ) ) - .then( () => { - this.ready = true; - } ); + this.map( v => v.init() ); + + this.ready = true; } /** * Destroys the view collection along with child views. - * - * @returns {Promise} A Promise resolved when the destruction process is finished. */ destroy() { - // Wait for all #add() promises to resolve before destroying the children. - // https://github.com/ckeditor/ckeditor5-ui/issues/203 - return Promise.all( this._addPromises ) - // Then begin the process of destroying the children. - .then( () => { - return Promise.all( this.map( v => v.destroy() ) ); - } ); + this.map( v => v.destroy() ); } /** @@ -124,27 +103,13 @@ export default class ViewCollection extends Collection { * * @param {module:ui/view~View} view A child view. * @param {Number} [index] Index at which the child will be added to the collection. - * @returns {Promise} A Promise resolved when the child {@link module:ui/view~View#init} is done. */ add( view, index ) { super.add( view, index ); - // {@link module:ui/view~View#init} returns `Promise`. - let promise = Promise.resolve(); - if ( this.ready && !view.ready ) { - promise = promise - .then( () => view.init() ) - // The view is ready. There's no point in storing the promise any longer. - .then( () => this._addPromises.delete( promise ) ); - - // Store the promise so it can be respected (and resolved) before #destroy() - // starts destroying the child view. - // https://github.com/ckeditor/ckeditor5-ui/issues/203 - this._addPromises.add( promise ); + view.init(); } - - return promise; } /** diff --git a/tests/view.js b/tests/view.js index b8a5cfae..0377a726 100644 --- a/tests/view.js +++ b/tests/view.js @@ -3,7 +3,7 @@ * For licensing, see LICENSE.md. */ -/* global document, setTimeout, HTMLElement */ +/* global document, HTMLElement */ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import View from '../src/view'; @@ -87,47 +87,21 @@ describe( 'View', () => { setTestViewInstance(); } ); - it( 'should return a promise', () => { - const spy = sinon.spy(); - const child = { - init: () => { - return new Promise( resolve => { - setTimeout( () => resolve(), 100 ); - } ) - .then( () => spy() ); - } - }; - - return view.init() - .then( () => { - const returned = view.addChildren( child ); - expect( returned ).to.be.instanceof( Promise ); - - return returned.then( () => { - sinon.assert.calledOnce( spy ); - } ); - } ); - } ); - it( 'should add a single view to #_unboundChildren', () => { expect( view._unboundChildren ).to.have.length( 0 ); const child = {}; - return view.addChildren( child ) - .then( () => { - expect( view._unboundChildren ).to.have.length( 1 ); - expect( view._unboundChildren.get( 0 ) ).to.equal( child ); - } ); + view.addChildren( child ); + expect( view._unboundChildren ).to.have.length( 1 ); + expect( view._unboundChildren.get( 0 ) ).to.equal( child ); } ); it( 'should support iterables', () => { expect( view._unboundChildren ).to.have.length( 0 ); - return view.addChildren( [ {}, {}, {} ] ) - .then( () => { - expect( view._unboundChildren ).to.have.length( 3 ); - } ); + view.addChildren( [ {}, {}, {} ] ); + expect( view._unboundChildren ).to.have.length( 3 ); } ); } ); @@ -135,28 +109,22 @@ describe( 'View', () => { beforeEach( createViewWithChildren ); it( 'should throw if already initialized', () => { - return view.init() - .then( () => { - view.init(); - - throw new Error( 'This should not be executed.' ); - } ) - .catch( err => { - expect( err ).to.be.instanceof( CKEditorError ); - expect( err.message ).to.match( /ui-view-init-re/ ); - } ); - } ); - - it( 'returns a promise', () => { - expect( view.init() ).to.be.instanceof( Promise ); + view.init(); + + try { + view.init(); + throw new Error( 'This should not be executed.' ); + } catch ( err ) { + expect( err ).to.be.instanceof( CKEditorError ); + expect( err.message ).to.match( /ui-view-init-re/ ); + } } ); it( 'should set view#ready', () => { expect( view.ready ).to.be.false; - return view.init().then( () => { - expect( view.ready ).to.be.true; - } ); + view.init(); + expect( view.ready ).to.be.true; } ); it( 'calls init() on all view#_viewCollections', () => { @@ -166,11 +134,10 @@ describe( 'View', () => { const spyA = testUtils.sinon.spy( collectionA, 'init' ); const spyB = testUtils.sinon.spy( collectionB, 'init' ); - return view.init().then( () => { - sinon.assert.calledOnce( spyA ); - sinon.assert.calledOnce( spyB ); - sinon.assert.callOrder( spyA, spyB ); - } ); + view.init(); + sinon.assert.calledOnce( spyA ); + sinon.assert.calledOnce( spyB ); + sinon.assert.callOrder( spyA, spyB ); } ); } ); @@ -261,47 +228,32 @@ describe( 'View', () => { describe( 'destroy()', () => { beforeEach( createViewWithChildren ); - it( 'should return a promise', () => { - const promise = view.destroy(); - - expect( promise ).to.be.instanceof( Promise ); - - return promise; - } ); - - it( 'can be called multiple times', done => { + it( 'can be called multiple times', () => { expect( () => { - view.destroy().then( () => { - return view.destroy().then( () => { - done(); - } ); - } ); + view.destroy(); } ).to.not.throw(); } ); it( 'should not touch the basic properties', () => { - return view.destroy().then( () => { - 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._viewCollections ).to.be.instanceOf( Collection ); - expect( view._unboundChildren ).to.be.instanceOf( ViewCollection ); - } ); + view.destroy(); + + 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._viewCollections ).to.be.instanceOf( Collection ); + expect( view._unboundChildren ).to.be.instanceOf( ViewCollection ); } ); it( 'should not clear the #_unboundChildren', () => { const cached = view._unboundChildren; - return view.addChildren( [ new View(), new View() ] ) - .then( () => { - expect( cached ).to.have.length( 4 ); + view.addChildren( [ new View(), new View() ] ); + expect( cached ).to.have.length( 4 ); - return view.destroy().then( () => { - expect( cached ).to.have.length( 4 ); - } ); - } ); + view.destroy(); + expect( cached ).to.have.length( 4 ); } ); it( 'should not clear the #_viewCollections', () => { @@ -309,9 +261,8 @@ describe( 'View', () => { expect( cached ).to.have.length( 1 ); - return view.destroy().then( () => { - expect( cached ).to.have.length( 1 ); - } ); + view.destroy(); + expect( cached ).to.have.length( 1 ); } ); it( 'leaves the #element in DOM', () => { @@ -320,9 +271,8 @@ describe( 'View', () => { parentEl.appendChild( view.element ); - return view.destroy().then( () => { - expect( elRef.parentNode ).to.equal( parentEl ); - } ); + view.destroy(); + expect( elRef.parentNode ).to.equal( parentEl ); } ); it( 'calls destroy() on all view#_viewCollections', () => { @@ -332,61 +282,18 @@ describe( 'View', () => { const spyA = testUtils.sinon.spy( collectionA, 'destroy' ); const spyB = testUtils.sinon.spy( collectionB, 'destroy' ); - return view.destroy().then( () => { - sinon.assert.calledOnce( spyA ); - sinon.assert.calledOnce( spyB ); - sinon.assert.callOrder( spyA, spyB ); - } ); + view.destroy(); + sinon.assert.calledOnce( spyA ); + sinon.assert.calledOnce( spyB ); + sinon.assert.callOrder( spyA, spyB ); } ); it( 'destroy a template–less view', () => { - let promise; - view = new View(); expect( () => { - promise = view.destroy(); + view.destroy(); } ).to.not.throw(); - - return promise; - } ); - - // https://github.com/ckeditor/ckeditor5-ui/issues/203 - it( 'waits for all #addChildren promises to resolve', () => { - const spyA = sinon.spy(); - const spyB = sinon.spy(); - - class DelayedInitView extends View { - constructor( delay, spy ) { - super(); - - this.delay = delay; - this.spy = spy; - } - - init() { - return new Promise( resolve => { - setTimeout( () => resolve(), this.delay ); - } ) - .then( () => super.init() ) - .then( () => { - this.spy(); - } ); - } - } - - const viewA = new DelayedInitView( 200, spyA ); - const viewB = new DelayedInitView( 100, spyB ); - - return view.init().then( () => { - view.addChildren( [ viewA, viewB ] ); - - return view.destroy().then( () => { - expect( viewA.ready ).to.be.true; - expect( viewB.ready ).to.be.true; - sinon.assert.callOrder( spyB, spyA ); - } ); - } ); } ); } ); } ); @@ -429,44 +336,8 @@ function createViewWithChildren() { } } - class ChildViewA extends ChildView { - init() { - const promise = new Promise( resolve => { - setTimeout( resolve, 50 ); - } ); - - return super.init().then( promise ); - } - - destroy() { - const promise = new Promise( resolve => { - setTimeout( resolve, 10 ); - } ); - - return super.destroy().then( promise ); - } - } - - class ChildViewB extends ChildView { - init() { - const promise = new Promise( resolve => { - setTimeout( resolve, 10 ); - } ); - - return super.init().then( promise ); - } - - destroy() { - const promise = new Promise( resolve => { - setTimeout( resolve, 50 ); - } ); - - return super.destroy().then( promise ); - } - } - - childA = new ChildViewA(); - childB = new ChildViewB(); + childA = new ChildView(); + childB = new ChildView(); setTestViewClass( { tag: 'p', diff --git a/tests/viewcollection.js b/tests/viewcollection.js index 977adc15..e5a03f33 100644 --- a/tests/viewcollection.js +++ b/tests/viewcollection.js @@ -3,7 +3,7 @@ * For licensing, see LICENSE.md. */ -/* global document, setTimeout */ +/* global document */ import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; @@ -111,79 +111,26 @@ describe( 'ViewCollection', () => { collection.remove( 1 ); // Finally init the view. Check what's in there. - return view.init().then( () => { - expect( view.element.childNodes ).to.have.length( 2 ); - expect( view.element.childNodes[ 0 ] ).to.equal( viewA.element ); - expect( view.element.childNodes[ 1 ] ).to.equal( viewC.element ); - } ); + view.init(); + + expect( view.element.childNodes ).to.have.length( 2 ); + expect( view.element.childNodes[ 0 ] ).to.equal( viewA.element ); + expect( view.element.childNodes[ 1 ] ).to.equal( viewC.element ); } ); } ); } ); describe( 'init()', () => { - it( 'should return a promise', () => { - expect( collection.init() ).to.be.instanceof( Promise ); - } ); - - it( 'should return a composition of child init() promises', () => { - const spyA = sinon.spy().named( 'A' ); - const spyB = sinon.spy().named( 'B' ); - const spyC = sinon.spy().named( 'C' ); - - const childA = { - init: () => { - return new Promise( resolve => { - setTimeout( () => { - spyA(); - resolve(); - }, 20 ); - } ); - } - }; - - const childB = { - init: () => { - return new Promise( resolve => { - setTimeout( () => { - spyB(); - resolve(); - }, 10 ); - } ); - } - }; - - const childC = { - init: () => { - return new Promise( resolve => { - setTimeout( () => { - spyC(); - resolve(); - }, 0 ); - } ); - } - }; - - collection.add( childA ); - collection.add( childB ); - collection.add( childC ); - - return collection.init() - .then( () => { - sinon.assert.callOrder( spyC, spyB, spyA ); - } ); - } ); - it( 'should throw if already initialized', () => { - return collection.init() - .then( () => { - collection.init(); - - throw new Error( 'This should not be executed.' ); - } ) - .catch( err => { - expect( err ).to.be.instanceof( CKEditorError ); - expect( err.message ).to.match( /ui-viewcollection-init-reinit/ ); - } ); + collection.init(); + + try { + collection.init(); + throw new Error( 'This should not be executed.' ); + } catch ( err ) { + expect( err ).to.be.instanceof( CKEditorError ); + expect( err.message ).to.match( /ui-viewcollection-init-reinit/ ); + } } ); it( 'calls #init on all views in the collection', () => { @@ -201,23 +148,18 @@ describe( 'ViewCollection', () => { collection.add( viewA ); collection.add( viewB ); - return collection.init().then( () => { - sinon.assert.calledOnce( spyA ); - sinon.assert.calledOnce( spyB ); - sinon.assert.callOrder( spyA, spyB ); + collection.init(); + sinon.assert.calledOnce( spyA ); + sinon.assert.calledOnce( spyB ); + sinon.assert.callOrder( spyA, spyB ); - expect( viewA.element.parentNode ).to.equal( collection._parentElement ); - expect( viewA.element.nextSibling ).to.equal( viewB.element ); - expect( collection.ready ).to.be.true; - } ); + expect( viewA.element.parentNode ).to.equal( collection._parentElement ); + expect( viewA.element.nextSibling ).to.equal( viewB.element ); + expect( collection.ready ).to.be.true; } ); } ); describe( 'destroy()', () => { - it( 'should return a promise', () => { - expect( collection.destroy() ).to.be.instanceof( Promise ); - } ); - it( 'calls #destroy on all views in the collection', () => { const viewA = new View(); const viewB = new View(); @@ -228,59 +170,14 @@ describe( 'ViewCollection', () => { collection.add( viewA ); collection.add( viewB ); - return collection.destroy().then( () => { - sinon.assert.calledOnce( spyA ); - sinon.assert.calledOnce( spyB ); - sinon.assert.callOrder( spyA, spyB ); - } ); - } ); - - // https://github.com/ckeditor/ckeditor5-ui/issues/203 - it( 'waits for all #add promises to resolve', () => { - const spyA = sinon.spy(); - const spyB = sinon.spy(); - - class DelayedInitView extends View { - constructor( delay, spy ) { - super(); - - this.delay = delay; - this.spy = spy; - } - - init() { - return new Promise( resolve => { - setTimeout( () => resolve(), this.delay ); - } ) - .then( () => super.init() ) - .then( () => { - this.spy(); - } ); - } - } - - const viewA = new DelayedInitView( 200, spyA ); - const viewB = new DelayedInitView( 100, spyB ); - - return collection.init().then( () => { - collection.add( viewA ); - collection.add( viewB ); - } ) - .then( () => { - return collection.destroy().then( () => { - expect( viewA.ready ).to.be.true; - expect( viewB.ready ).to.be.true; - sinon.assert.callOrder( spyB, spyA ); - } ); - } ); + collection.destroy(); + sinon.assert.calledOnce( spyA ); + sinon.assert.calledOnce( spyB ); + sinon.assert.callOrder( spyA, spyB ); } ); } ); describe( 'add()', () => { - it( 'returns a promise', () => { - expect( collection.add( {} ) ).to.be.instanceof( Promise ); - } ); - it( 'initializes the new view in the collection', () => { let view = new View(); let spy = testUtils.sinon.spy( view, 'init' ); @@ -288,23 +185,20 @@ describe( 'ViewCollection', () => { expect( collection.ready ).to.be.false; expect( view.ready ).to.be.false; - return collection.add( view ).then( () => { - expect( collection.ready ).to.be.false; - expect( view.ready ).to.be.false; - - sinon.assert.notCalled( spy ); + collection.add( view ); + expect( collection.ready ).to.be.false; + expect( view.ready ).to.be.false; - view = new View(); - spy = testUtils.sinon.spy( view, 'init' ); + sinon.assert.notCalled( spy ); - collection.ready = true; + view = new View(); + spy = testUtils.sinon.spy( view, 'init' ); - return collection.add( view ).then( () => { - expect( view.ready ).to.be.true; + collection.ready = true; - sinon.assert.calledOnce( spy ); - } ); - } ); + collection.add( view ); + expect( view.ready ).to.be.true; + sinon.assert.calledOnce( spy ); } ); it( 'works for a view with a Number view#id attribute', () => { @@ -312,10 +206,9 @@ describe( 'ViewCollection', () => { view.set( 'id', 1 ); - return collection.add( view ).then( () => { - expect( view.id ).to.equal( 1 ); - expect( view.viewUid ).to.be.a( 'string' ); - } ); + collection.add( view ); + expect( view.id ).to.equal( 1 ); + expect( view.viewUid ).to.be.a( 'string' ); } ); } ); From 0f23ea7b849e21292ff986c1b739222cf9748377 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Jul 2017 18:19:32 +0200 Subject: [PATCH 02/14] Made ButtonView synchronous. --- src/button/buttonview.js | 6 ++---- tests/button/buttonview.js | 26 ++++++++++++-------------- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/src/button/buttonview.js b/src/button/buttonview.js index afbe4670..d026d28c 100644 --- a/src/button/buttonview.js +++ b/src/button/buttonview.js @@ -207,8 +207,6 @@ export default class ButtonView extends View { * @inheritDoc */ init() { - let promise = Promise.resolve(); - if ( this.icon && !this.iconView ) { const iconView = this.iconView = new IconView(); @@ -217,10 +215,10 @@ export default class ButtonView extends View { this.element.insertBefore( iconView.element, this.element.firstChild ); // Make sure the icon view will be destroyed along with button. - promise = promise.then( () => this.addChildren( iconView ) ); + this.addChildren( iconView ); } - return promise.then( () => super.init() ); + super.init(); } /** diff --git a/tests/button/buttonview.js b/tests/button/buttonview.js index baba5e54..ce09c11d 100644 --- a/tests/button/buttonview.js +++ b/tests/button/buttonview.js @@ -225,29 +225,27 @@ describe( 'ButtonView', () => { view = new ButtonView( locale ); view.icon = 'foo'; - return view.init().then( () => { - expect( view.element.childNodes ).to.have.length( 2 ); - expect( view.element.childNodes[ 0 ] ).to.equal( view.iconView.element ); + view.init(); + expect( view.element.childNodes ).to.have.length( 2 ); + expect( view.element.childNodes[ 0 ] ).to.equal( view.iconView.element ); - expect( view.iconView ).to.instanceOf( IconView ); - expect( view.iconView.content ).to.equal( 'foo' ); + expect( view.iconView ).to.instanceOf( IconView ); + expect( view.iconView.content ).to.equal( 'foo' ); - view.icon = 'bar'; - expect( view.iconView.content ).to.equal( 'bar' ); - } ); + view.icon = 'bar'; + expect( view.iconView.content ).to.equal( 'bar' ); } ); it( 'is destroyed with the view', () => { view = new ButtonView( locale ); view.icon = 'foo'; - return view.init().then( () => { - const spy = sinon.spy( view.iconView, 'destroy' ); + view.init(); - return view.destroy().then( () => { - sinon.assert.calledOnce( spy ); - } ); - } ); + const spy = sinon.spy( view.iconView, 'destroy' ); + + view.destroy(); + sinon.assert.calledOnce( spy ); } ); } ); From 0a23e5e5e9fc288936f9f2b8ba653fe13b5a054a Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Jul 2017 18:19:52 +0200 Subject: [PATCH 03/14] Made DropdownView synchronous. --- src/dropdown/dropdownview.js | 2 +- tests/dropdown/dropdownview.js | 14 ++++++-------- tests/dropdown/list/createlistdropdown.js | 6 +++--- tests/dropdown/manual/dropdown.js | 5 ++--- 4 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/dropdown/dropdownview.js b/src/dropdown/dropdownview.js index 0e441b2b..7a05c30c 100644 --- a/src/dropdown/dropdownview.js +++ b/src/dropdown/dropdownview.js @@ -138,7 +138,7 @@ export default class DropdownView extends View { this.keystrokes.set( 'arrowleft', closeDropdown ); this.keystrokes.set( 'esc', closeDropdown ); - return super.init(); + super.init(); } /** diff --git a/tests/dropdown/dropdownview.js b/tests/dropdown/dropdownview.js index 0c3dd692..d9ea37c1 100644 --- a/tests/dropdown/dropdownview.js +++ b/tests/dropdown/dropdownview.js @@ -100,10 +100,9 @@ describe( 'DropdownView', () => { const spy = sinon.spy( view.keystrokes, 'listenTo' ); - return view.init().then( () => { - sinon.assert.calledOnce( spy ); - sinon.assert.calledWithExactly( spy, view.element ); - } ); + view.init(); + sinon.assert.calledOnce( spy ); + sinon.assert.calledWithExactly( spy, view.element ); } ); it( 'adds #element to #focusTracker', () => { @@ -113,10 +112,9 @@ describe( 'DropdownView', () => { const spy = sinon.spy( view.focusTracker, 'add' ); - return view.init().then( () => { - sinon.assert.calledOnce( spy ); - sinon.assert.calledWithExactly( spy, view.element ); - } ); + view.init(); + sinon.assert.calledOnce( spy ); + sinon.assert.calledWithExactly( spy, view.element ); } ); describe( 'activates keyboard navigation for the dropdown', () => { diff --git a/tests/dropdown/list/createlistdropdown.js b/tests/dropdown/list/createlistdropdown.js index 3f4ded11..056c2075 100644 --- a/tests/dropdown/list/createlistdropdown.js +++ b/tests/dropdown/list/createlistdropdown.js @@ -25,9 +25,9 @@ describe( 'createListDropdown', () => { label: 'foo' } ); - return ( view = createListDropdown( model, locale ) ).init().then( () => { - document.body.appendChild( view.element ); - } ); + view = createListDropdown( model, locale ); + view.init(); + document.body.appendChild( view.element ); } ); describe( 'constructor()', () => { diff --git a/tests/dropdown/manual/dropdown.js b/tests/dropdown/manual/dropdown.js index effa1d3b..a7a956ee 100644 --- a/tests/dropdown/manual/dropdown.js +++ b/tests/dropdown/manual/dropdown.js @@ -14,9 +14,8 @@ import createListDropdown from '../../../src/dropdown/list/createlistdropdown'; import '@ckeditor/ckeditor5-theme-lark/theme/theme.scss'; function renderInto( selector, view ) { - view.init().then( () => { - document.querySelector( selector ).appendChild( view.element ); - } ); + view.init(); + document.querySelector( selector ).appendChild( view.element ); } function testEmpty() { From 3473488889bf916f5c88abc4ce43ec466be5c621 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Jul 2017 18:20:32 +0200 Subject: [PATCH 04/14] Made ToolbarView synchronous. --- src/toolbar/sticky/stickytoolbarview.js | 5 ++- src/toolbar/toolbarview.js | 11 +++--- .../stickytoolbarview/stickytoolbarview.js | 17 +++------ tests/toolbar/sticky/stickytoolbarview.js | 23 ++++-------- tests/toolbar/toolbarview.js | 35 +++++++++---------- 5 files changed, 36 insertions(+), 55 deletions(-) diff --git a/src/toolbar/sticky/stickytoolbarview.js b/src/toolbar/sticky/stickytoolbarview.js index f710a5c7..6fc91641 100644 --- a/src/toolbar/sticky/stickytoolbarview.js +++ b/src/toolbar/sticky/stickytoolbarview.js @@ -175,9 +175,8 @@ export default class StickyToolbarView extends ToolbarView { * Destroys the toolbar and removes the {@link #_elementPlaceholder}. */ destroy() { - return super.destroy().then( () => { - this._elementPlaceholder.remove(); - } ); + super.destroy(); + this._elementPlaceholder.remove(); } /** diff --git a/src/toolbar/toolbarview.js b/src/toolbar/toolbarview.js index e08c1484..c3ddc077 100644 --- a/src/toolbar/toolbarview.js +++ b/src/toolbar/toolbarview.js @@ -103,7 +103,7 @@ export default class ToolbarView extends View { // Start listening for the keystrokes coming from #element. this.keystrokes.listenTo( this.element ); - return super.init(); + super.init(); } /** @@ -119,18 +119,17 @@ export default class ToolbarView extends View { * * @param {Array} config The toolbar config. * @param {module:ui/componentfactory~ComponentFactory} factory A factory producing toolbar items. - * @returns {Promise} A promise resolved when created toolbar items are initialized. */ fillFromConfig( config, factory ) { if ( !config ) { - return Promise.resolve(); + return; } - return Promise.all( config.map( name => { + config.map( name => { const component = name == '|' ? new ToolbarSeparatorView() : factory.create( name ); - return this.items.add( component ); - } ) ); + this.items.add( component ); + } ); } } diff --git a/tests/manual/stickytoolbarview/stickytoolbarview.js b/tests/manual/stickytoolbarview/stickytoolbarview.js index 8dbb7dbe..6655a691 100644 --- a/tests/manual/stickytoolbarview/stickytoolbarview.js +++ b/tests/manual/stickytoolbarview/stickytoolbarview.js @@ -3,29 +3,22 @@ * For licensing, see LICENSE.md. */ -/* globals console:false */ - import testUtils from '../../../tests/_utils/utils'; import StickyToolbarView from '../../../src/toolbar/sticky/stickytoolbarview'; import '@ckeditor/ckeditor5-theme-lark/theme/theme.scss'; -testUtils.createTestUIView( { +const ui = testUtils.createTestUIView( { top: '.ck-editor__top' -} ) -.then( ui => { - createToolbar( ui.top ); -} ) -.catch( err => { - console.error( err.stack ); } ); +createToolbar( ui.top ); + function createToolbar( collection ) { const toolbar = new StickyToolbarView(); toolbar.limiterElement = collection._parentElement.parentNode; - collection.add( toolbar ).then( () => { - toolbar.isActive = true; - } ); + collection.add( toolbar ); + toolbar.isActive = true; } diff --git a/tests/toolbar/sticky/stickytoolbarview.js b/tests/toolbar/sticky/stickytoolbarview.js index 221ca953..449b2cf3 100644 --- a/tests/toolbar/sticky/stickytoolbarview.js +++ b/tests/toolbar/sticky/stickytoolbarview.js @@ -174,32 +174,23 @@ describe( 'StickyToolbarView', () => { } ); describe( 'destroy()', () => { - it( 'should return a promise', () => { - expect( view.destroy() ).to.be.instanceof( Promise ); - } ); - - it( 'can be called multiple times', done => { + it( 'can be called multiple times', () => { expect( () => { - view.destroy().then( () => { - return view.destroy().then( () => { - done(); - } ); - } ); + view.destroy(); + view.destroy(); } ).to.not.throw(); } ); it( 'calls destroy on parent class', () => { const spy = testUtils.sinon.spy( ToolbarView.prototype, 'destroy' ); - return view.destroy().then( () => { - expect( spy.calledOnce ).to.be.true; - } ); + view.destroy(); + expect( spy.calledOnce ).to.be.true; } ); it( 'removes view._elementPlaceholder from DOM', () => { - return view.destroy().then( () => { - expect( view._elementPlaceholder.parentNode ).to.be.null; - } ); + view.destroy(); + expect( view._elementPlaceholder.parentNode ).to.be.null; } ); } ); diff --git a/tests/toolbar/toolbarview.js b/tests/toolbar/toolbarview.js index b9d80645..0a12c5b7 100644 --- a/tests/toolbar/toolbarview.js +++ b/tests/toolbar/toolbarview.js @@ -21,8 +21,7 @@ describe( 'ToolbarView', () => { beforeEach( () => { locale = {}; view = new ToolbarView( locale ); - - return view.init(); + view.init(); } ); describe( 'constructor()', () => { @@ -82,10 +81,9 @@ describe( 'ToolbarView', () => { const spy = sinon.spy( view.keystrokes, 'listenTo' ); - return view.init().then( () => { - sinon.assert.calledOnce( spy ); - sinon.assert.calledWithExactly( spy, view.element ); - } ); + view.init(); + sinon.assert.calledOnce( spy ); + sinon.assert.calledWithExactly( spy, view.element ); } ); describe( 'activates keyboard navigation for the toolbar', () => { @@ -232,21 +230,22 @@ describe( 'ToolbarView', () => { factory.add( 'bar', namedFactory( 'bar' ) ); } ); - it( 'returns a promise', () => { - expect( view.fillFromConfig() ).to.be.instanceOf( Promise ); + it( 'does not throw when no config is provided', () => { + expect( () => { + view.fillFromConfig(); + } ).to.not.throw(); } ); it( 'expands the config into collection', () => { - return view.fillFromConfig( [ 'foo', 'bar', '|', 'foo' ], factory ) - .then( () => { - const items = view.items; - - expect( items ).to.have.length( 4 ); - expect( items.get( 0 ).name ).to.equal( 'foo' ); - expect( items.get( 1 ).name ).to.equal( 'bar' ); - expect( items.get( 2 ) ).to.be.instanceOf( ToolbarSeparatorView ); - expect( items.get( 3 ).name ).to.equal( 'foo' ); - } ); + view.fillFromConfig( [ 'foo', 'bar', '|', 'foo' ], factory ); + + const items = view.items; + + expect( items ).to.have.length( 4 ); + expect( items.get( 0 ).name ).to.equal( 'foo' ); + expect( items.get( 1 ).name ).to.equal( 'bar' ); + expect( items.get( 2 ) ).to.be.instanceOf( ToolbarSeparatorView ); + expect( items.get( 3 ).name ).to.equal( 'foo' ); } ); } ); } ); From af3a12b38829672e284168d5baa0b3463b9c672b Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Jul 2017 18:21:00 +0200 Subject: [PATCH 05/14] Made ListView synchronous. --- src/list/listview.js | 2 +- tests/list/listview.js | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/list/listview.js b/src/list/listview.js index 2f0f081e..eb544922 100644 --- a/src/list/listview.js +++ b/src/list/listview.js @@ -98,7 +98,7 @@ export default class ListView extends View { // Start listening for the keystrokes coming from #element. this.keystrokes.listenTo( this.element ); - return super.init(); + super.init(); } /** diff --git a/tests/list/listview.js b/tests/list/listview.js index 5a66c2db..2b1efaca 100644 --- a/tests/list/listview.js +++ b/tests/list/listview.js @@ -64,10 +64,9 @@ describe( 'ListView', () => { const spy = sinon.spy( view.keystrokes, 'listenTo' ); - return view.init().then( () => { - sinon.assert.calledOnce( spy ); - sinon.assert.calledWithExactly( spy, view.element ); - } ); + view.init(); + sinon.assert.calledOnce( spy ); + sinon.assert.calledWithExactly( spy, view.element ); } ); describe( 'activates keyboard navigation for the list', () => { From 5cbb8ab277aa9a31b2d8ac047014aa211ffc7bdf Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Jul 2017 18:21:59 +0200 Subject: [PATCH 06/14] Made EditableUIView synchronous. --- src/editableui/editableuiview.js | 6 ++-- tests/editableui/editableuiview.js | 58 ++++++++++++------------------ 2 files changed, 24 insertions(+), 40 deletions(-) diff --git a/src/editableui/editableuiview.js b/src/editableui/editableuiview.js index a207ceae..2c2f2ffe 100644 --- a/src/editableui/editableuiview.js +++ b/src/editableui/editableuiview.js @@ -78,8 +78,6 @@ export default class EditableUIView extends View { /** * Initializes the view by either applying the {@link #template} to the existing * {@link #editableElement} or assigning {@link #element} as {@link #editableElement}. - * - * @returns {Promise} */ init() { if ( this.externalElement ) { @@ -88,7 +86,7 @@ export default class EditableUIView extends View { this.editableElement = this.element; } - return super.init(); + super.init(); } /** @@ -99,6 +97,6 @@ export default class EditableUIView extends View { this.template.revert( this.externalElement ); } - return super.destroy(); + super.destroy(); } } diff --git a/tests/editableui/editableuiview.js b/tests/editableui/editableuiview.js index 2d43b590..8a1c361b 100644 --- a/tests/editableui/editableuiview.js +++ b/tests/editableui/editableuiview.js @@ -32,22 +32,20 @@ describe( 'EditableUIView', () => { it( 'renders element from template when no editableElement', () => { view = new EditableUIView( locale ); - return view.init().then( () => { - expect( view.element ).to.equal( view.editableElement ); - expect( view.element.classList.contains( 'ck-editor__editable' ) ).to.be.true; - expect( view.externalElement ).to.be.undefined; - } ); + view.init(); + expect( view.element ).to.equal( view.editableElement ); + expect( view.element.classList.contains( 'ck-editor__editable' ) ).to.be.true; + expect( view.externalElement ).to.be.undefined; } ); it( 'accepts editableElement as an argument', () => { view = new EditableUIView( locale, editableElement ); - return view.init().then( () => { - expect( view.element ).to.equal( editableElement ); - expect( view.element ).to.equal( view.editableElement ); - expect( view.element.classList.contains( 'ck-editor__editable' ) ).to.be.true; - expect( view.externalElement ).to.equal( editableElement ); - } ); + view.init(); + expect( view.element ).to.equal( editableElement ); + expect( view.element ).to.equal( view.editableElement ); + expect( view.element.classList.contains( 'ck-editor__editable' ) ).to.be.true; + expect( view.externalElement ).to.equal( editableElement ); } ); } ); @@ -80,18 +78,14 @@ describe( 'EditableUIView', () => { it( 'calls super#destroy()', () => { const spy = testUtils.sinon.spy( View.prototype, 'destroy' ); - return view.destroy().then( () => { - sinon.assert.calledOnce( spy ); - } ); + view.destroy(); + sinon.assert.calledOnce( spy ); } ); - it( 'can be called multiple times', done => { + it( 'can be called multiple times', () => { expect( () => { - view.destroy().then( () => { - return view.destroy().then( () => { - done(); - } ); - } ); + view.destroy(); + view.destroy(); } ).to.not.throw(); } ); @@ -102,14 +96,10 @@ describe( 'EditableUIView', () => { view = new EditableUIView( locale, editableElement ); - return view.init() - .then( () => { - expect( editableElement.contentEditable ).to.equal( 'true' ); - } ) - .then( () => view.destroy() ) - .then( () => { - expect( editableElement.contentEditable ).to.equal( 'false' ); - } ); + view.init(); + expect( editableElement.contentEditable ).to.equal( 'true' ); + view.destroy(); + expect( editableElement.contentEditable ).to.equal( 'false' ); } ); it( 'reverts contentEditable property of editableElement (was true)', () => { @@ -118,14 +108,10 @@ describe( 'EditableUIView', () => { view = new EditableUIView( locale, editableElement ); - return view.init() - .then( () => { - expect( editableElement.contentEditable ).to.equal( 'true' ); - } ) - .then( () => view.destroy() ) - .then( () => { - expect( editableElement.contentEditable ).to.equal( 'true' ); - } ); + view.init(); + expect( editableElement.contentEditable ).to.equal( 'true' ); + view.destroy(); + expect( editableElement.contentEditable ).to.equal( 'true' ); } ); } ); } ); From e1b6562a05ae413f5369d79adc11f5f64296db79 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Jul 2017 18:22:13 +0200 Subject: [PATCH 07/14] Made EditorUIView synchronous. --- src/editorui/editoruiview.js | 5 ++--- tests/editorui/editoruiview.js | 14 +++++--------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/editorui/editoruiview.js b/src/editorui/editoruiview.js index d7a11149..cdbe618f 100644 --- a/src/editorui/editoruiview.js +++ b/src/editorui/editoruiview.js @@ -47,9 +47,8 @@ export default class EditorUIView extends View { * @inheritDoc */ init() { - return Promise.resolve() - .then( () => this._renderBodyCollection() ) - .then( () => super.init() ); + this._renderBodyCollection(); + super.init(); } /** diff --git a/tests/editorui/editoruiview.js b/tests/editorui/editoruiview.js index c85d70c1..2a5c8def 100644 --- a/tests/editorui/editoruiview.js +++ b/tests/editorui/editoruiview.js @@ -47,18 +47,14 @@ describe( 'EditorUIView', () => { it( 'removes the body region container', () => { const el = view._bodyCollectionContainer; - return view.destroy().then( () => { - expect( el.parentNode ).to.be.null; - } ); + view.destroy(); + expect( el.parentNode ).to.be.null; } ); - it( 'can be called multiple times', done => { + it( 'can be called multiple times', () => { expect( () => { - view.destroy().then( () => { - return view.destroy().then( () => { - done(); - } ); - } ); + view.destroy(); + view.destroy(); } ).to.not.throw(); } ); } ); From 291899bcfea3441e25fbe472bfd5911d8647d4ef Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Jul 2017 18:22:48 +0200 Subject: [PATCH 08/14] Made createTestUIView helper synchronous. --- tests/_utils-tests/utils.js | 24 +++++++----------------- tests/_utils/utils.js | 6 +++--- 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/tests/_utils-tests/utils.js b/tests/_utils-tests/utils.js index 9b47b7c9..74cf3fdc 100644 --- a/tests/_utils-tests/utils.js +++ b/tests/_utils-tests/utils.js @@ -9,40 +9,30 @@ import testUtils from '../../tests/_utils/utils'; describe( 'utils', () => { describe( 'createTestUIView', () => { - it( 'returns a promise', () => { - expect( testUtils.createTestUIView() ).to.be.instanceof( Promise ); - } ); - describe( 'view instance', () => { it( 'comes with a view', () => { - const promise = testUtils.createTestUIView(); + const view = testUtils.createTestUIView(); - return promise.then( view => { - expect( view.element ).to.equal( document.body ); - } ); + expect( view.element ).to.equal( document.body ); } ); it( 'creates collections and regions', () => { - const promise = testUtils.createTestUIView( { + const view = testUtils.createTestUIView( { foo: el => el.firstChild, bar: el => el.lastChild, } ); - return promise.then( view => { - expect( view.foo._parentElement ).to.equal( document.body.firstChild ); - expect( view.bar._parentElement ).to.equal( document.body.lastChild ); - } ); + expect( view.foo._parentElement ).to.equal( document.body.firstChild ); + expect( view.bar._parentElement ).to.equal( document.body.lastChild ); } ); it( 'is ready', () => { - const promise = testUtils.createTestUIView( { + const view = testUtils.createTestUIView( { foo: el => el.firstChild, bar: el => el.lastChild, } ); - return promise.then( view => { - expect( view.ready ).to.be.true; - } ); + expect( view.ready ).to.be.true; } ); } ); } ); diff --git a/tests/_utils/utils.js b/tests/_utils/utils.js index 5f180573..4e2087a2 100644 --- a/tests/_utils/utils.js +++ b/tests/_utils/utils.js @@ -50,9 +50,9 @@ const utils = { const view = new TestUIView(); - return view.init().then( () => { - return view; - } ); + view.init(); + + return view; } }; From b7358107d87566475afb7aca321f0ad99f1131b8 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Jul 2017 18:23:20 +0200 Subject: [PATCH 09/14] Made ContextualBalloon plugin synchronous. --- src/panel/balloon/contextualballoon.js | 19 ++-- .../contextualballoon/contextualballoon.js | 2 - tests/panel/balloon/contextualballoon.js | 95 ++++--------------- 3 files changed, 22 insertions(+), 94 deletions(-) diff --git a/src/panel/balloon/contextualballoon.js b/src/panel/balloon/contextualballoon.js index dc406761..3c243079 100644 --- a/src/panel/balloon/contextualballoon.js +++ b/src/panel/balloon/contextualballoon.js @@ -60,7 +60,7 @@ export default class ContextualBalloon extends Plugin { this.editor.ui.focusTracker.add( this.view.element ); // Add balloon panel view to editor `body` collection and wait until view will be ready. - return this.editor.ui.view.body.add( this.view ); + this.editor.ui.view.body.add( this.view ); } /** @@ -92,7 +92,6 @@ export default class ContextualBalloon extends Plugin { * @param {module:ui/view~View} [data.view] Content of the balloon. * @param {module:utils/dom/position~Options} [data.position] Positioning options. * @param {String} [data.balloonClassName] Additional css class for {@link #view} added when given view is visible. - * @returns {Promise} A Promise resolved when the child {@link module:ui/view~View#init} is done. */ add( data ) { if ( this.hasView( data.view ) ) { @@ -112,8 +111,9 @@ export default class ContextualBalloon extends Plugin { // Add new view to the stack. this._stack.set( data.view, data ); + // And display it. - return this._show( data ); + this._show( data ); } /** @@ -122,7 +122,6 @@ export default class ContextualBalloon extends Plugin { * When there is no view in the stack then balloon will hide. * * @param {module:ui/view~View} view A view to be removed from the balloon. - * @returns {Promise} A Promise resolved when the preceding view is ready. */ remove( view ) { if ( !this.hasView( view ) ) { @@ -134,9 +133,6 @@ export default class ContextualBalloon extends Plugin { throw new CKEditorError( 'contextualballoon-remove-view-not-exist: Cannot remove configuration of not existing view.' ); } - // A Promise resolved when the preceding view is ready. - let promise = Promise.resolve(); - // When visible view is being removed. if ( this.visibleView === view ) { // We need to remove it from the view content. @@ -151,7 +147,7 @@ export default class ContextualBalloon extends Plugin { // If it is some other view. if ( last ) { // Just show it. - promise = this._show( last ); + this._show( last ); } else { // Hide the balloon panel. this.view.hide(); @@ -160,8 +156,6 @@ export default class ContextualBalloon extends Plugin { // Just remove given view from the stack. this._stack.delete( view ); } - - return promise; } /** @@ -190,9 +184,8 @@ export default class ContextualBalloon extends Plugin { _show( { view, balloonClassName = '' } ) { this.view.className = balloonClassName; - return this.view.content.add( view ).then( () => { - this.view.pin( this._getBalloonPosition() ); - } ); + this.view.content.add( view ); + this.view.pin( this._getBalloonPosition() ); } /** diff --git a/tests/manual/contextualballoon/contextualballoon.js b/tests/manual/contextualballoon/contextualballoon.js index c95f7dce..43786015 100644 --- a/tests/manual/contextualballoon/contextualballoon.js +++ b/tests/manual/contextualballoon/contextualballoon.js @@ -153,8 +153,6 @@ class PluginGeneric extends Plugin { return toolbar.fillFromConfig( this.buttons, this.editor.ui.componentFactory ); } - - return Promise.resolve(); } _showPanel() { diff --git a/tests/panel/balloon/contextualballoon.js b/tests/panel/balloon/contextualballoon.js index fd5849d0..5f8f4522 100644 --- a/tests/panel/balloon/contextualballoon.js +++ b/tests/panel/balloon/contextualballoon.js @@ -10,7 +10,7 @@ import View from '../../../src/view'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; -/* global document, Event, setTimeout */ +/* global document, Event */ describe( 'ContextualBalloon', () => { let editor, editorElement, balloon, viewA, viewB; @@ -35,13 +35,12 @@ describe( 'ContextualBalloon', () => { viewB = new View(); // Add viewA to the pane and init viewB. - return Promise.all( [ - balloon.add( { - view: viewA, - position: { target: 'fake' } - } ), - viewB.init(), - ] ); + balloon.add( { + view: viewA, + position: { target: 'fake' } + } ); + + viewB.init(); } ); } ); @@ -104,33 +103,6 @@ describe( 'ContextualBalloon', () => { } ); describe( 'add()', () => { - it( 'should return promise resolved when view is ready', done => { - const clock = sinon.useFakeTimers(); - - const view = { - init: () => { - return new Promise( resolve => { - setTimeout( () => { - resolve(); - }, 10 ); - } ); - }, - destroy: () => {} - }; - - const result = balloon.add( { - view, - position: { target: 'fake' } - } ); - - expect( result ).to.instanceof( Promise ); - - result.then( done ); - - clock.tick( 11 ); - clock.restore(); - } ); - it( 'should add view to the stack and display in balloon attached using given position options', () => { expect( balloon.view.content.length ).to.equal( 1 ); expect( balloon.view.content.get( 0 ) ).to.deep.equal( viewA ); @@ -162,20 +134,19 @@ describe( 'ContextualBalloon', () => { } ); it( 'should keep balloon at the same position after adding next view', () => { - return balloon.add( { + balloon.add( { view: viewB, position: { target: 'other' } - } ) - .then( () => { - expect( balloon.view.pin.calledTwice ).to.true; + } ); - expect( balloon.view.pin.firstCall.args[ 0 ] ).to.deep.equal( { - target: 'fake' - } ); + expect( balloon.view.pin.calledTwice ).to.true; - expect( balloon.view.pin.secondCall.args[ 0 ] ).to.deep.equal( { - target: 'fake' - } ); + expect( balloon.view.pin.firstCall.args[ 0 ] ).to.deep.equal( { + target: 'fake' + } ); + + expect( balloon.view.pin.secondCall.args[ 0 ] ).to.deep.equal( { + target: 'fake' } ); } ); @@ -221,10 +192,6 @@ describe( 'ContextualBalloon', () => { } ); describe( 'remove()', () => { - it( 'should return promise', () => { - expect( balloon.remove( viewA ) ).to.instanceof( Promise ); - } ); - it( 'should remove given view and hide balloon when there is no other view to display', () => { balloon.view.isVisible = true; @@ -245,36 +212,6 @@ describe( 'ContextualBalloon', () => { expect( balloon.visibleView ).to.equal( viewA ); } ); - it( 'should wait for init of preceding view when was is not ready', done => { - const clock = sinon.useFakeTimers(); - - const view = { - init: () => { - return new Promise( resolve => { - setTimeout( () => { - resolve(); - }, 10 ); - } ); - }, - destroy: () => {} - }; - - balloon.add( { - view, - position: { target: 'fake' } - } ); - - balloon.add( { - view: viewB, - position: { target: 'fake' } - } ); - - balloon.remove( viewB ).then( done ); - - clock.tick( 11 ); - clock.restore(); - } ); - it( 'should remove given view from the stack when view is not visible', () => { balloon.add( { view: viewB, From a6500fa991e38c5b7864a29eef518b9cd43b4815 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Jul 2017 18:23:33 +0200 Subject: [PATCH 10/14] Made ContextualToolbar plugin synchronous. --- src/toolbar/contextual/contextualtoolbar.js | 51 +++---- tests/toolbar/contextual/contextualtoolbar.js | 144 +++++++----------- 2 files changed, 78 insertions(+), 117 deletions(-) diff --git a/src/toolbar/contextual/contextualtoolbar.js b/src/toolbar/contextual/contextualtoolbar.js index 60d72d7e..b2319b54 100644 --- a/src/toolbar/contextual/contextualtoolbar.js +++ b/src/toolbar/contextual/contextualtoolbar.js @@ -90,7 +90,7 @@ export default class ContextualToolbar extends Plugin { const config = this.editor.config.get( 'contextualToolbar' ); const factory = this.editor.ui.componentFactory; - return this.toolbarView.fillFromConfig( config, factory ); + this.toolbarView.fillFromConfig( config, factory ); } /** @@ -142,7 +142,6 @@ export default class ContextualToolbar extends Plugin { * Fires {@link #event:beforeShow} event just before displaying the panel. * * @protected - * @return {Promise} A promise resolved when the {@link #toolbarView} {@link module:ui/view~View#init} is done. */ _showPanel() { const editingView = this.editor.editing.view; @@ -150,47 +149,39 @@ export default class ContextualToolbar extends Plugin { // Do not add toolbar to the balloon stack twice. if ( this._balloon.hasView( this.toolbarView ) ) { - return Promise.resolve(); + return; } // This implementation assumes that only non–collapsed selections gets the contextual toolbar. if ( !editingView.isFocused || editingView.selection.isCollapsed ) { - return Promise.resolve(); + return; } - const showPromise = new Promise( resolve => { - // If `beforeShow` event is not stopped by any external code then panel will be displayed. - this.once( 'beforeShow', () => { - if ( isStopped ) { - resolve(); - - return; - } - - // Update panel position when selection changes while balloon will be opened - // (by an external document changes). - this.listenTo( editingView, 'render', () => { - this._balloon.updatePosition( this._getBalloonPositionData() ); - } ); - - resolve( - // Add panel to the common editor contextual balloon. - this._balloon.add( { - view: this.toolbarView, - position: this._getBalloonPositionData(), - balloonClassName: 'ck-toolbar-container ck-editor-toolbar-container' - } ) - ); + // If `beforeShow` event is not stopped by any external code then panel will be displayed. + this.once( 'beforeShow', () => { + if ( isStopped ) { + return; + } + + // Update panel position when selection changes while balloon will be opened + // (by an external document changes). + this.listenTo( editingView, 'render', () => { + this._balloon.updatePosition( this._getBalloonPositionData() ); + } ); + + // Add panel to the common editor contextual balloon. + this._balloon.add( { + view: this.toolbarView, + position: this._getBalloonPositionData(), + balloonClassName: 'ck-toolbar-container ck-editor-toolbar-container' } ); - }, { priority: 'lowest' } ); + } ); // Fire this event to inform that `ContextualToolbar` is going to be shown. // Helper function for preventing the panel from being displayed is passed along with the event. this.fire( 'beforeShow', () => { isStopped = true; } ); - - return showPromise; } /** diff --git a/tests/toolbar/contextual/contextualtoolbar.js b/tests/toolbar/contextual/contextualtoolbar.js index 3cde89e9..2eae0867 100644 --- a/tests/toolbar/contextual/contextualtoolbar.js +++ b/tests/toolbar/contextual/contextualtoolbar.js @@ -43,7 +43,7 @@ describe( 'ContextualToolbar', () => { // Focus the engine. editor.editing.view.isFocused = true; - return contextualToolbar.toolbarView.init(); + contextualToolbar.toolbarView.init(); } ); } ); @@ -141,38 +141,28 @@ describe( 'ContextualToolbar', () => { editor.editing.view.isFocused = true; } ); - it( 'should return a promise', () => { - setData( editor.document, 'b[a]r' ); - - const returned = contextualToolbar._showPanel(); - - expect( returned ).to.instanceof( Promise ); - - return returned; - } ); - it( 'should add #toolbarView to the #_balloon and attach the #_balloon to the selection for the forward selection', () => { setData( editor.document, 'b[a]r' ); const defaultPositions = BalloonPanelView.defaultPositions; - return contextualToolbar._showPanel().then( () => { - sinon.assert.calledWithExactly( balloonAddSpy, { - view: contextualToolbar.toolbarView, - balloonClassName: 'ck-toolbar-container ck-editor-toolbar-container', - position: { - target: sinon.match( value => value() == backwardSelectionRect ), - limiter: editor.ui.view.editable.element, - positions: [ - defaultPositions.southEastArrowNorth, - defaultPositions.southEastArrowNorthEast, - defaultPositions.southEastArrowNorthWest, - defaultPositions.northEastArrowSouth, - defaultPositions.northEastArrowSouthEast, - defaultPositions.northEastArrowSouthWest - ] - } - } ); + contextualToolbar._showPanel(); + + sinon.assert.calledWithExactly( balloonAddSpy, { + view: contextualToolbar.toolbarView, + balloonClassName: 'ck-toolbar-container ck-editor-toolbar-container', + position: { + target: sinon.match( value => value() == backwardSelectionRect ), + limiter: editor.ui.view.editable.element, + positions: [ + defaultPositions.southEastArrowNorth, + defaultPositions.southEastArrowNorthEast, + defaultPositions.southEastArrowNorthWest, + defaultPositions.northEastArrowSouth, + defaultPositions.northEastArrowSouthEast, + defaultPositions.northEastArrowSouthWest + ] + } } ); } ); @@ -181,25 +171,24 @@ describe( 'ContextualToolbar', () => { const defaultPositions = BalloonPanelView.defaultPositions; - return contextualToolbar._showPanel() - .then( () => { - sinon.assert.calledWithExactly( balloonAddSpy, { - view: contextualToolbar.toolbarView, - balloonClassName: 'ck-toolbar-container ck-editor-toolbar-container', - position: { - target: sinon.match( value => value() == forwardSelectionRect ), - limiter: editor.ui.view.editable.element, - positions: [ - defaultPositions.northWestArrowSouth, - defaultPositions.northWestArrowSouthWest, - defaultPositions.northWestArrowSouthEast, - defaultPositions.southWestArrowNorth, - defaultPositions.southWestArrowNorthWest, - defaultPositions.southWestArrowNorthEast - ] - } - } ); - } ); + contextualToolbar._showPanel(); + + sinon.assert.calledWithExactly( balloonAddSpy, { + view: contextualToolbar.toolbarView, + balloonClassName: 'ck-toolbar-container ck-editor-toolbar-container', + position: { + target: sinon.match( value => value() == forwardSelectionRect ), + limiter: editor.ui.view.editable.element, + positions: [ + defaultPositions.northWestArrowSouth, + defaultPositions.northWestArrowSouthWest, + defaultPositions.northWestArrowSouthEast, + defaultPositions.southWestArrowNorth, + defaultPositions.southWestArrowNorthWest, + defaultPositions.southWestArrowNorthEast + ] + } + } ); } ); it( 'should update balloon position on ViewDocument#render event while balloon is added to the #_balloon', () => { @@ -209,43 +198,34 @@ describe( 'ContextualToolbar', () => { editor.editing.view.fire( 'render' ); - return contextualToolbar._showPanel() - .then( () => { - sinon.assert.notCalled( spy ); - - editor.editing.view.fire( 'render' ); + contextualToolbar._showPanel(); + sinon.assert.notCalled( spy ); - sinon.assert.calledOnce( spy ); - } ); + editor.editing.view.fire( 'render' ); + sinon.assert.calledOnce( spy ); } ); it( 'should not add #toolbarView to the #_balloon more than once', () => { setData( editor.document, 'b[a]r' ); - return contextualToolbar._showPanel() - .then( () => contextualToolbar._showPanel() ) - .then( () => { - sinon.assert.calledOnce( balloonAddSpy ); - } ); + contextualToolbar._showPanel(); + contextualToolbar._showPanel(); + sinon.assert.calledOnce( balloonAddSpy ); } ); it( 'should not add #toolbarView to the #_balloon when editor is not focused', () => { setData( editor.document, 'b[a]r' ); editor.editing.view.isFocused = false; - return contextualToolbar._showPanel() - .then( () => { - sinon.assert.notCalled( balloonAddSpy ); - } ); + contextualToolbar._showPanel(); + sinon.assert.notCalled( balloonAddSpy ); } ); it( 'should not add #toolbarView to the #_balloon when selection is collapsed', () => { setData( editor.document, 'b[]ar' ); - return contextualToolbar._showPanel() - .then( () => { - sinon.assert.notCalled( balloonAddSpy ); - } ); + contextualToolbar._showPanel(); + sinon.assert.notCalled( balloonAddSpy ); } ); } ); @@ -260,12 +240,10 @@ describe( 'ContextualToolbar', () => { it( 'should remove #toolbarView from the #_balloon', () => { setData( editor.document, 'b[a]r' ); - return contextualToolbar._showPanel() - .then( () => { - contextualToolbar._hidePanel(); + contextualToolbar._showPanel(); - sinon.assert.calledWithExactly( removeBalloonSpy, contextualToolbar.toolbarView ); - } ); + contextualToolbar._hidePanel(); + sinon.assert.calledWithExactly( removeBalloonSpy, contextualToolbar.toolbarView ); } ); it( 'should stop update balloon position on ViewDocument#render event', () => { @@ -273,14 +251,11 @@ describe( 'ContextualToolbar', () => { const spy = sandbox.spy( balloon, 'updatePosition' ); - return contextualToolbar._showPanel() - .then( () => { - contextualToolbar._hidePanel(); - - editor.editing.view.fire( 'render' ); + contextualToolbar._showPanel(); + contextualToolbar._hidePanel(); - sinon.assert.notCalled( spy ); - } ); + editor.editing.view.fire( 'render' ); + sinon.assert.notCalled( spy ); } ); it( 'should not remove #ttolbarView when is not added to the #_balloon', () => { @@ -320,7 +295,6 @@ describe( 'ContextualToolbar', () => { beforeEach( () => { setData( editor.document, '[bar]' ); - // Methods are stubbed because return internal promise which can't be returned in test. showPanelSpy = sandbox.stub( contextualToolbar, '_showPanel', () => {} ); hidePanelSpy = sandbox.stub( contextualToolbar, '_hidePanel', () => {} ); } ); @@ -421,11 +395,8 @@ describe( 'ContextualToolbar', () => { contextualToolbar.on( 'beforeShow', spy ); setData( editor.document, 'b[a]r' ); - const promise = contextualToolbar._showPanel(); - + contextualToolbar._showPanel(); sinon.assert.calledOnce( spy ); - - return promise; } ); it( 'should not show the panel when `beforeShow` event is stopped', () => { @@ -437,9 +408,8 @@ describe( 'ContextualToolbar', () => { stop(); } ); - return contextualToolbar._showPanel().then( () => { - sinon.assert.notCalled( balloonAddSpy ); - } ); + contextualToolbar._showPanel(); + sinon.assert.notCalled( balloonAddSpy ); } ); } ); From 9a8de16941287d8d88ed81fde8ad5d5fdca76183 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Jul 2017 18:23:45 +0200 Subject: [PATCH 11/14] Made BalloonPanelView synchronous. --- tests/panel/balloon/balloonpanelview.js | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/tests/panel/balloon/balloonpanelview.js b/tests/panel/balloon/balloonpanelview.js index c561f5ff..64f2a9e3 100644 --- a/tests/panel/balloon/balloonpanelview.js +++ b/tests/panel/balloon/balloonpanelview.js @@ -113,9 +113,8 @@ describe( 'BalloonPanelView', () => { const button = new ButtonView( { t() {} } ); - return view.content.add( button ).then( () => { - expect( view.element.childNodes.length ).to.equal( 1 ); - } ); + view.content.add( button ); + expect( view.element.childNodes.length ).to.equal( 1 ); } ); } ); @@ -565,15 +564,14 @@ describe( 'BalloonPanelView', () => { sinon.assert.calledOnce( attachToSpy ); - return view.destroy().then( () => { - view = null; + view.destroy(); + view = null; - window.dispatchEvent( new Event( 'resize' ) ); - window.dispatchEvent( new Event( 'scroll' ) ); + window.dispatchEvent( new Event( 'resize' ) ); + window.dispatchEvent( new Event( 'scroll' ) ); - // Still once. - sinon.assert.calledOnce( attachToSpy ); - } ); + // Still once. + sinon.assert.calledOnce( attachToSpy ); } ); it( 'should set document.body as the default limiter', () => { From 909629e28a16779f357dfe6174de0d2549d51976 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 3 Jul 2017 18:24:18 +0200 Subject: [PATCH 12/14] Tests: Made a number of manual tests synchronous. --- tests/manual/tickets/126/1.js | 10 +++++----- tests/manual/tickets/170/1.js | 9 ++++----- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/tests/manual/tickets/126/1.js b/tests/manual/tickets/126/1.js index 6a416910..84b0b5aa 100644 --- a/tests/manual/tickets/126/1.js +++ b/tests/manual/tickets/126/1.js @@ -13,11 +13,11 @@ window.createPanel = selector => { const view = new BalloonPanelView(); view.element.innerHTML = `Parent of this panel has position:${ selector }.`; - view.init().then( () => { - document.querySelector( `#${ selector }-container` ).appendChild( view.element ); + view.init(); - view.attachTo( { - target: document.querySelector( `#anchor-${ selector }` ) - } ); + document.querySelector( `#${ selector }-container` ).appendChild( view.element ); + + view.attachTo( { + target: document.querySelector( `#anchor-${ selector }` ) } ); }; diff --git a/tests/manual/tickets/170/1.js b/tests/manual/tickets/170/1.js index 1245f710..7f7c3aa6 100644 --- a/tests/manual/tickets/170/1.js +++ b/tests/manual/tickets/170/1.js @@ -51,11 +51,10 @@ ClassicEditor.create( document.querySelector( '#editor-stick' ), { editor.ui.view.element.querySelector( '.ck-editor__editable' ).scrollTop = 360; - panel.init().then( () => { - panel.pin( { - target: editor.ui.view.element.querySelector( '.ck-editor__editable p strong' ), - limiter: editor.ui.view.editableElement - } ); + panel.init(); + panel.pin( { + target: editor.ui.view.element.querySelector( '.ck-editor__editable p strong' ), + limiter: editor.ui.view.editableElement } ); window.stickEditor = editor; From 6ddda9d3049f3e2f8d580549b3b9c9ecf02dffdd Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 4 Jul 2017 12:44:33 +0200 Subject: [PATCH 13/14] Tests: Fixed #170 manual test to use synchronous UI API. --- tests/manual/tickets/170/1.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/manual/tickets/170/1.js b/tests/manual/tickets/170/1.js index 7f7c3aa6..b94efdf7 100644 --- a/tests/manual/tickets/170/1.js +++ b/tests/manual/tickets/170/1.js @@ -25,11 +25,10 @@ ClassicEditor.create( document.querySelector( '#editor-attach' ), { editor.ui.view.element.querySelector( '.ck-editor__editable' ).scrollTop = 360; - panel.init().then( () => { - panel.attachTo( { - target: editor.ui.view.element.querySelector( '.ck-editor__editable p strong' ), - limiter: editor.ui.view.editableElement - } ); + panel.init(); + panel.attachTo( { + target: editor.ui.view.element.querySelector( '.ck-editor__editable p strong' ), + limiter: editor.ui.view.editableElement } ); window.attachEditor = editor; From c1c8d86625a323991c128339d0fd0c520a004387 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 4 Jul 2017 17:09:12 +0200 Subject: [PATCH 14/14] Minor fixes and improvements after the refactoring. --- src/view.js | 2 +- tests/toolbar/contextual/contextualtoolbar.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/view.js b/src/view.js index cb8bf86f..7c81c1a1 100644 --- a/src/view.js +++ b/src/view.js @@ -295,7 +295,7 @@ export default class View { } /** -f * Destroys the view instance and child views located in {@link #_viewCollections}. + * Destroys the view instance and child views located in {@link #_viewCollections}. */ destroy() { this.stopListening(); diff --git a/tests/toolbar/contextual/contextualtoolbar.js b/tests/toolbar/contextual/contextualtoolbar.js index 2eae0867..43e44503 100644 --- a/tests/toolbar/contextual/contextualtoolbar.js +++ b/tests/toolbar/contextual/contextualtoolbar.js @@ -295,8 +295,8 @@ describe( 'ContextualToolbar', () => { beforeEach( () => { setData( editor.document, '[bar]' ); - showPanelSpy = sandbox.stub( contextualToolbar, '_showPanel', () => {} ); - hidePanelSpy = sandbox.stub( contextualToolbar, '_hidePanel', () => {} ); + showPanelSpy = sandbox.spy( contextualToolbar, '_showPanel' ); + hidePanelSpy = sandbox.spy( contextualToolbar, '_hidePanel' ); } ); it( 'should open when selection stops changing', () => {