From 9f1ae9e8c0c2b2a46af130bfd510f98331e8a6d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Wed, 7 Jun 2017 15:12:39 +0200 Subject: [PATCH 1/3] Started using decorate() in data controller. --- src/controller/datacontroller.js | 58 ++++------ tests/controller/datacontroller.js | 180 ++++++++++------------------- 2 files changed, 80 insertions(+), 158 deletions(-) diff --git a/src/controller/datacontroller.js b/src/controller/datacontroller.js index 9a31aed7a..887c05078 100644 --- a/src/controller/datacontroller.js +++ b/src/controller/datacontroller.js @@ -8,7 +8,7 @@ */ import mix from '@ckeditor/ckeditor5-utils/src/mix'; -import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin'; +import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin'; import Mapper from '../conversion/mapper'; @@ -38,7 +38,7 @@ import getSelectedContent from './getselectedcontent'; * * {@link module:engine/conversion/modelconversiondispatcher~ModelConversionDispatcher model to view} and * * {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher view to model} converters. * - * @mixes module:utils/emittermixin~EmitterMixin + * @mixes module:utils/emittermixin~ObservableMixin */ export default class DataController { /** @@ -117,12 +117,7 @@ export default class DataController { this.viewToModel.on( 'element', convertToModelFragment(), { priority: 'lowest' } ); this.viewToModel.on( 'documentFragment', convertToModelFragment(), { priority: 'lowest' } ); - this.on( 'insertContent', ( evt, data ) => insertContent( this, data.content, data.selection, data.batch ) ); - this.on( 'deleteContent', ( evt, data ) => deleteContent( data.selection, data.batch, data.options ) ); - this.on( 'modifySelection', ( evt, data ) => modifySelection( this, data.selection, data.options ) ); - this.on( 'getSelectedContent', ( evt, data ) => { - data.content = getSelectedContent( data.selection ); - } ); + this.decorate( 'insertContent', 'deleteContent', 'modifySelection', 'getSelectedContent' ); } /** @@ -257,7 +252,7 @@ export default class DataController { * changes will be added to a new batch. */ insertContent( content, selection, batch ) { - this.fire( 'insertContent', { content, selection, batch } ); + insertContent( this, content, selection, batch ); } /** @@ -277,7 +272,7 @@ export default class DataController { * @param {Object} options See {@link module:engine/controller/deletecontent~deleteContent}'s options. */ deleteContent( selection, batch, options ) { - this.fire( 'deleteContent', { batch, selection, options } ); + deleteContent( selection, batch, options ); } /** @@ -288,7 +283,7 @@ export default class DataController { * @param {Object} options See {@link module:engine/controller/modifyselection~modifySelection}'s options. */ modifySelection( selection, options ) { - this.fire( 'modifySelection', { selection, options } ); + modifySelection( this, selection, options ); } /** @@ -299,59 +294,48 @@ export default class DataController { * @returns {module:engine/model/documentfragment~DocumentFragment} Document fragment holding the clone of the selected content. */ getSelectedContent( selection ) { - const evtData = { selection }; - - this.fire( 'getSelectedContent', evtData ); - - return evtData.content; + return getSelectedContent( selection ); } } -mix( DataController, EmitterMixin ); +mix( DataController, ObservableMixin ); /** * Event fired when {@link #insertContent} method is called. + * * The {@link #insertContent default action of that method} is implemented as a * listener to this event so it can be fully customized by the features. * * @event insertContent - * @param {Object} data - * @param {module:engine/view/documentfragment~DocumentFragment|module:engine/model/item~Item} data.content The content to insert. - * @param {module:engine/model/selection~Selection} data.selection Selection into which the content should be inserted. - * @param {module:engine/model/batch~Batch} [data.batch] Batch to which deltas will be added. + * @param {Array} args The arguments passed to the original method. */ /** * Event fired when {@link #deleteContent} method is called. - * The {@link module:engine/controller/deletecontent~deleteContent default action of that method} is implemented as a + * + * The {@link #deleteContent default action of that method} is implemented as a * listener to this event so it can be fully customized by the features. * * @event deleteContent - * @param {Object} data - * @param {module:engine/model/batch~Batch} data.batch - * @param {module:engine/model/selection~Selection} data.selection - * @param {Object} data.options See {@link module:engine/controller/deletecontent~deleteContent}'s options. + * @param {Array} args The arguments passed to the original method. */ /** * Event fired when {@link #modifySelection} method is called. - * The {@link module:engine/controller/modifyselection~modifySelection default action of that method} is implemented as a + * + * The {@link #modifySelection default action of that method} is implemented as a * listener to this event so it can be fully customized by the features. * * @event modifySelection - * @param {Object} data - * @param {module:engine/model/selection~Selection} data.selection - * @param {Object} data.options See {@link module:engine/controller/modifyselection~modifySelection}'s options. + * @param {Array} args The arguments passed to the original method. */ /** - * Event fired when {@link module:engine/controller/datacontroller~DataController#getSelectedContent} method is called. - * The {@link module:engine/controller/getselectedcontent~getSelectedContent default action of that method} is implemented as a + * Event fired when {@link #getSelectedContent} method is called. + * + * The {@link #getSelectedContent default action of that method} is implemented as a * listener to this event so it can be fully customized by the features. * - * @event module:engine/controller/datacontroller~DataController#getSelectedContent - * @param {Object} data - * @param {module:engine/model/selection~Selection} data.selection - * @param {module:engine/model/documentfragment~DocumentFragment} data.content The document fragment to return - * (holding a clone of the selected content). + * @event getSelectedContent + * @param {Array} args The arguments passed to the original method. */ diff --git a/tests/controller/datacontroller.js b/tests/controller/datacontroller.js index 643082a44..bf872706d 100644 --- a/tests/controller/datacontroller.js +++ b/tests/controller/datacontroller.js @@ -42,94 +42,6 @@ describe( 'DataController', () => { expect( data.processor ).to.be.undefined; } ); - - it( 'should add insertContent listener and inserts document fragment when event fires', () => { - const batch = modelDocument.batch(); - const content = new ModelDocumentFragment( [ new ModelText( 'x' ) ] ); - - schema.registerItem( 'paragraph', '$block' ); - - setData( modelDocument, 'a[]b' ); - - data.fire( 'insertContent', { content, selection: modelDocument.selection, batch } ); - - expect( getData( modelDocument ) ).to.equal( 'ax[]b' ); - expect( batch.deltas.length ).to.be.above( 0 ); - } ); - - it( 'should add insertContent listener and inserts an item when event fires', () => { - const batch = modelDocument.batch(); - const content = new ModelText( 'x' ); - - schema.registerItem( 'paragraph', '$block' ); - - setData( modelDocument, 'a[]b' ); - - data.fire( 'insertContent', { content, selection: modelDocument.selection, batch } ); - - expect( getData( modelDocument ) ).to.equal( 'ax[]b' ); - expect( batch.deltas.length ).to.be.above( 0 ); - } ); - - it( 'should add deleteContent listener', () => { - schema.registerItem( 'paragraph', '$block' ); - - setData( modelDocument, 'f[ooba]r' ); - - const batch = modelDocument.batch(); - - data.fire( 'deleteContent', { batch, selection: modelDocument.selection } ); - - expect( getData( modelDocument ) ).to.equal( 'f[]r' ); - expect( batch.deltas ).to.not.be.empty; - } ); - - it( 'should add deleteContent listener which passes ', () => { - schema.registerItem( 'paragraph', '$block' ); - - setData( modelDocument, 'f[ooba]r' ); - - const batch = modelDocument.batch(); - - data.fire( 'deleteContent', { - batch, - selection: modelDocument.selection, - options: { merge: true } - } ); - - expect( getData( modelDocument ) ).to.equal( 'f[]r' ); - } ); - - it( 'should add modifySelection listener', () => { - schema.registerItem( 'paragraph', '$block' ); - - setData( modelDocument, 'foo[]bar' ); - - data.fire( 'modifySelection', { - selection: modelDocument.selection, - options: { - direction: 'backward' - } - } ); - - expect( getData( modelDocument ) ) - .to.equal( 'fo[o]bar' ); - expect( modelDocument.selection.isBackward ).to.true; - } ); - - it( 'should add getSelectedContent listener', () => { - schema.registerItem( 'paragraph', '$block' ); - - setData( modelDocument, 'fo[ob]ar' ); - - const evtData = { - selection: modelDocument.selection - }; - - data.fire( 'getSelectedContent', evtData ); - - expect( stringify( evtData.content ) ).to.equal( 'ob' ); - } ); } ); describe( 'parse', () => { @@ -425,58 +337,88 @@ describe( 'DataController', () => { } ); describe( 'insertContent', () => { - it( 'should fire the insertContent event', () => { + it( 'should be decorated', () => { + schema.allow( { name: '$text', inside: '$root' } ); // To surpress warnings. + const spy = sinon.spy(); - const content = new ModelDocumentFragment( [ new ModelText( 'x' ) ] ); - const batch = modelDocument.batch(); - schema.allow( { name: '$text', inside: '$root' } ); data.on( 'insertContent', spy ); - data.insertContent( content, modelDocument.selection, batch ); + data.insertContent( new ModelText( 'a' ), modelDocument.selection ); + + expect( spy.calledOnce ).to.be.true; + } ); + + it( 'should insert content (item)', () => { + schema.registerItem( 'paragraph', '$block' ); + + setData( modelDocument, 'fo[]ar' ); + + data.insertContent( new ModelText( 'ob' ), modelDocument.selection ); + + expect( getData( modelDocument ) ).to.equal( 'foob[]ar' ); + } ); + + it( 'should insert content (document fragment)', () => { + schema.registerItem( 'paragraph', '$block' ); + + setData( modelDocument, 'fo[]ar' ); + + data.insertContent( new ModelDocumentFragment( [ new ModelText( 'ob' ) ] ), modelDocument.selection ); - expect( spy.args[ 0 ][ 1 ] ).to.deep.equal( { - batch, - selection: modelDocument.selection, - content - } ); + expect( getData( modelDocument ) ).to.equal( 'foob[]ar' ); } ); } ); describe( 'deleteContent', () => { - it( 'should fire the deleteContent event', () => { + it( 'should be decorated', () => { const spy = sinon.spy(); - const batch = modelDocument.batch(); data.on( 'deleteContent', spy ); - data.deleteContent( modelDocument.selection, batch ); + data.deleteContent( modelDocument.selection ); - const evtData = spy.args[ 0 ][ 1 ]; + expect( spy.calledOnce ).to.be.true; + } ); - expect( evtData.batch ).to.equal( batch ); - expect( evtData.selection ).to.equal( modelDocument.selection ); + it( 'should delete selected content', () => { + schema.registerItem( 'paragraph', '$block' ); + + setData( modelDocument, 'fo[ob]ar' ); + + data.deleteContent( modelDocument.selection, modelDocument.batch() ); + + expect( getData( modelDocument ) ).to.equal( 'fo[]ar' ); } ); } ); describe( 'modifySelection', () => { - it( 'should fire the deleteContent event', () => { + it( 'should be decorated', () => { + schema.registerItem( 'paragraph', '$block' ); + setData( modelDocument, 'fo[ob]ar' ); + const spy = sinon.spy(); - const opts = { direction: 'backward' }; data.on( 'modifySelection', spy ); - data.modifySelection( modelDocument.selection, opts ); + data.modifySelection( modelDocument.selection ); - const evtData = spy.args[ 0 ][ 1 ]; + expect( spy.calledOnce ).to.be.true; + } ); - expect( evtData.selection ).to.equal( modelDocument.selection ); - expect( evtData.options ).to.equal( opts ); + it( 'should modify a selection', () => { + schema.registerItem( 'paragraph', '$block' ); + + setData( modelDocument, 'fo[ob]ar' ); + + data.modifySelection( modelDocument.selection, { direction: 'backward' } ); + + expect( getData( modelDocument ) ).to.equal( 'fo[o]bar' ); } ); } ); describe( 'getSelectedContent', () => { - it( 'should fire the getSelectedContent event', () => { + it( 'should be decorated', () => { const spy = sinon.spy(); const sel = new ModelSelection(); @@ -484,21 +426,17 @@ describe( 'DataController', () => { data.getSelectedContent( sel ); - const evtData = spy.args[ 0 ][ 1 ]; - - expect( evtData.selection ).to.equal( sel ); + expect( spy.calledOnce ).to.be.true; } ); - it( 'should return the evtData.content of the getSelectedContent event', () => { - const frag = new ModelDocumentFragment(); + it( 'should return selected content', () => { + schema.registerItem( 'paragraph', '$block' ); - data.on( 'getSelectedContent', ( evt, data ) => { - data.content = frag; + setData( modelDocument, 'fo[ob]ar' ); - evt.stop(); - }, { priority: 'high' } ); + const content = data.getSelectedContent( modelDocument.selection ); - expect( data.getSelectedContent() ).to.equal( frag ); + expect( stringify( content ) ).to.equal( 'ob' ); } ); } ); } ); From bc9f00fa2273141624533c84f2a2ec8e9febd80e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Mon, 12 Jun 2017 14:06:41 +0200 Subject: [PATCH 2/3] Aligned decorate() usage to changes in utils. --- src/controller/datacontroller.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/controller/datacontroller.js b/src/controller/datacontroller.js index 887c05078..e4c474fda 100644 --- a/src/controller/datacontroller.js +++ b/src/controller/datacontroller.js @@ -117,7 +117,8 @@ export default class DataController { this.viewToModel.on( 'element', convertToModelFragment(), { priority: 'lowest' } ); this.viewToModel.on( 'documentFragment', convertToModelFragment(), { priority: 'lowest' } ); - this.decorate( 'insertContent', 'deleteContent', 'modifySelection', 'getSelectedContent' ); + [ 'insertContent', 'deleteContent', 'modifySelection', 'getSelectedContent' ] + .forEach( methodName => this.decorate( methodName ) ); } /** From 01a590d8a925d174a4e1eb9b5f25e84495264519 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Mon, 12 Jun 2017 19:44:49 +0200 Subject: [PATCH 3/3] Decorated view.Document#render(). --- src/view/document.js | 12 +++------ tests/view/document/document.js | 46 ++++++++++++++++++++++----------- 2 files changed, 35 insertions(+), 23 deletions(-) diff --git a/src/view/document.js b/src/view/document.js index 977dd1d90..afc66a219 100644 --- a/src/view/document.js +++ b/src/view/document.js @@ -117,13 +117,7 @@ export default class Document { injectQuirksHandling( this ); - // Listens `render` event on default priority. - // This way we can attach other listeners before or after rendering execution. - this.on( 'render', () => { - this.disableObservers(); - this.renderer.render(); - this.enableObservers(); - } ); + this.decorate( 'render' ); } /** @@ -266,7 +260,9 @@ export default class Document { * @fires render */ render() { - this.fire( 'render' ); + this.disableObservers(); + this.renderer.render(); + this.enableObservers(); } /** diff --git a/tests/view/document/document.js b/tests/view/document/document.js index 1dc359584..f407c7916 100644 --- a/tests/view/document/document.js +++ b/tests/view/document/document.js @@ -26,19 +26,13 @@ describe( 'Document', () => { const DEFAULT_OBSERVERS_COUNT = 5; let ObserverMock, ObserverMockGlobalCount, instantiated, enabled, domRoot, viewDocument; - before( () => { + beforeEach( () => { domRoot = createElement( document, 'div', { id: 'editor', contenteditable: 'true' } ); document.body.appendChild( domRoot ); - } ); - after( () => { - domRoot.parentElement.removeChild( domRoot ); - } ); - - beforeEach( () => { instantiated = 0; enabled = 0; @@ -70,6 +64,7 @@ describe( 'Document', () => { afterEach( () => { viewDocument.destroy(); + domRoot.remove(); } ); describe( 'constructor()', () => { @@ -91,7 +86,7 @@ describe( 'Document', () => { } ); } ); - describe( 'createRoot', () => { + describe( 'createRoot()', () => { it( 'should create root', () => { const domP = document.createElement( 'p' ); const domDiv = document.createElement( 'div' ); @@ -175,7 +170,7 @@ describe( 'Document', () => { } ); } ); - describe( 'attachDomRoot', () => { + describe( 'attachDomRoot()', () => { it( 'should create root without attach DOM element to the view element', () => { const domDiv = document.createElement( 'div' ); const viewRoot = viewDocument.createRoot( 'div' ); @@ -217,7 +212,7 @@ describe( 'Document', () => { } ); } ); - describe( 'getRoot', () => { + describe( 'getRoot()', () => { it( 'should return "main" root', () => { viewDocument.createRoot( document.createElement( 'div' ) ); @@ -235,7 +230,7 @@ describe( 'Document', () => { } ); } ); - describe( 'addObserver', () => { + describe( 'addObserver()', () => { beforeEach( () => { // The variable will be overwritten. viewDocument.destroy(); @@ -311,7 +306,7 @@ describe( 'Document', () => { } ); } ); - describe( 'getObserver', () => { + describe( 'getObserver()', () => { it( 'should return observer it it is added', () => { const addedObserverMock = viewDocument.addObserver( ObserverMock ); const getObserverMock = viewDocument.getObserver( ObserverMock ); @@ -327,7 +322,7 @@ describe( 'Document', () => { } ); } ); - describe( 'disableObservers', () => { + describe( 'disableObservers()', () => { it( 'should disable observers', () => { const addedObserverMock = viewDocument.addObserver( ObserverMock ); @@ -341,7 +336,7 @@ describe( 'Document', () => { } ); } ); - describe( 'enableObservers', () => { + describe( 'enableObservers()', () => { it( 'should enable observers', () => { const addedObserverMock = viewDocument.addObserver( ObserverMock ); @@ -369,7 +364,7 @@ describe( 'Document', () => { } ); } ); - describe( 'focus', () => { + describe( 'focus()', () => { let domEditable, viewEditable; beforeEach( () => { @@ -421,4 +416,25 @@ describe( 'Document', () => { expect( logSpy.args[ 0 ][ 0 ] ).to.match( /^view-focus-no-selection/ ); } ); } ); + + describe( 'render()', () => { + it( 'should fire an event', () => { + const spy = sinon.spy(); + + viewDocument.on( 'render', spy ); + + viewDocument.render(); + + expect( spy.calledOnce ).to.be.true; + } ); + + it( 'disable observers, renders and enable observers', () => { + const observerMock = viewDocument.addObserver( ObserverMock ); + const renderStub = sinon.stub( viewDocument.renderer, 'render' ); + + viewDocument.render(); + + sinon.assert.callOrder( observerMock.disable, renderStub, observerMock.enable ); + } ); + } ); } );