From 0a7e029a3132e4944520e5bc0d26d3babb886976 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 7 Feb 2019 12:42:04 +0100 Subject: [PATCH 01/11] Extract ConversionHelpers to separate files. --- src/conversion/conversion.js | 45 ------------------------------- src/conversion/downcasthelpers.js | 4 +-- src/conversion/upcasthelpers.js | 4 +-- 3 files changed, 4 insertions(+), 49 deletions(-) diff --git a/src/conversion/conversion.js b/src/conversion/conversion.js index 276d27d4c..7fe0738d7 100644 --- a/src/conversion/conversion.js +++ b/src/conversion/conversion.js @@ -605,48 +605,3 @@ function* _getUpcastDefinition( model, view, upcastAlso ) { } } } - -/** - * Base class for conversion helpers. - */ -export class ConversionHelpers { - /** - * Creates a conversion helpers instance. - * - * @param {Array.} dispatcher - */ - constructor( dispatcher ) { - this._dispatchers = Array.isArray( dispatcher ) ? dispatcher : [ dispatcher ]; - } - - /** - * Registers a conversion helper. - * - * **Note**: See full usage example in the `{@link module:engine/conversion/conversion~Conversion#for conversion.for()}` - * method description. - * - * @param {Function} conversionHelper The function to be called on event. - * @returns {module:engine/conversion/downcasthelpers~DowncastHelpers|module:engine/conversion/upcasthelpers~UpcastHelpers} - */ - add( conversionHelper ) { - this._addToDispatchers( conversionHelper ); - - return this; - } - - /** - * Helper function for the `Conversion` `.add()` method. - * - * Calls `conversionHelper` on each dispatcher from the group specified earlier in the `.for()` call, effectively - * adding converters to all specified dispatchers. - * - * @private - * @param {Function} conversionHelper - */ - _addToDispatchers( conversionHelper ) { - for ( const dispatcher of this._dispatchers ) { - conversionHelper( dispatcher ); - } - } -} diff --git a/src/conversion/downcasthelpers.js b/src/conversion/downcasthelpers.js index 6a8d3c392..7d4d16649 100644 --- a/src/conversion/downcasthelpers.js +++ b/src/conversion/downcasthelpers.js @@ -9,7 +9,7 @@ import ModelElement from '../model/element'; import ViewAttributeElement from '../view/attributeelement'; import DocumentSelection from '../model/documentselection'; -import { ConversionHelpers } from './conversion'; +import ConversionHelpers from './conversionhelpers'; import log from '@ckeditor/ckeditor5-utils/src/log'; import { cloneDeep } from 'lodash-es'; @@ -23,7 +23,7 @@ import { cloneDeep } from 'lodash-es'; /** * Downcast conversion helper functions. * - * @extends module:engine/conversion/conversion~ConversionHelpers + * @extends module:engine/conversion/conversionhelpers~ConversionHelpers */ export default class DowncastHelpers extends ConversionHelpers { /** diff --git a/src/conversion/upcasthelpers.js b/src/conversion/upcasthelpers.js index 3ab2eece9..f80a450a4 100644 --- a/src/conversion/upcasthelpers.js +++ b/src/conversion/upcasthelpers.js @@ -5,7 +5,7 @@ import Matcher from '../view/matcher'; import ModelRange from '../model/range'; -import { ConversionHelpers } from './conversion'; +import ConversionHelpers from './conversionhelpers'; import { cloneDeep } from 'lodash-es'; import ModelSelection from '../model/selection'; @@ -20,7 +20,7 @@ import ModelSelection from '../model/selection'; /** * Upcast conversion helper functions. * - * @extends module:engine/conversion/conversion~ConversionHelpers + * @extends module:engine/conversion/conversionhelpers~ConversionHelpers */ export default class UpcastHelpers extends ConversionHelpers { /** From bdc0f97cc15d5e4bc52dcae34253d6e1e73195b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 7 Feb 2019 12:42:20 +0100 Subject: [PATCH 02/11] Remove Conversion#register() method. --- src/conversion/conversion.js | 60 +++++++++++++++++++++-------- src/conversion/conversionhelpers.js | 53 +++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 17 deletions(-) create mode 100644 src/conversion/conversionhelpers.js diff --git a/src/conversion/conversion.js b/src/conversion/conversion.js index 7fe0738d7..450767269 100644 --- a/src/conversion/conversion.js +++ b/src/conversion/conversion.js @@ -8,6 +8,8 @@ */ import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; +import UpcastHelpers from './upcasthelpers'; +import DowncastHelpers from './downcasthelpers'; /** * A utility class that helps add converters to upcast and downcast dispatchers. @@ -57,37 +59,59 @@ export default class Conversion { /** * Creates a new conversion instance. */ - constructor() { + constructor( downcastDispatchers, upcastDispatchers ) { /** * @private * @member {Map} */ - this._conversionHelpers = new Map(); + this._helpers = new Map(); + this._groups = new Map(); + + this._defineGroup( 'downcast', downcastDispatchers, DowncastHelpers ); + this._defineGroup( 'upcast', upcastDispatchers, UpcastHelpers ); } /** - * Registers one or more converters under a given group name. The group name can then be used to assign a converter - * to multiple dispatchers at once. + * Define an alias for registered dispatcher. + * + * const conversion = new Conversion( [ dataDowncastDispatcher, editingDowncastDispatcher ], upcastDispatcher ); * - * If a given group name is used for the second time, the - * {@link module:utils/ckeditorerror~CKEditorError `conversion-register-group-exists` error} is thrown. + * conversion.addAlias( 'dataDowncast', dataDowncastDispatcher ); * - * @param {String} name The name for dispatchers group. - * @param {module:engine/conversion/downcasthelpers~DowncastHelpers| - * module:engine/conversion/upcasthelpers~UpcastHelpers} conversionHelpers + * @param {String} alias An alias of a dispatcher. + * @param {module:engine/conversion/downcastdispatcher~DowncastDispatcher| + * module:engine/conversion/upcastdispatcher~UpcastDispatcher} dispatcher Dispatcher which should have an alias. */ - register( name, conversionHelpers ) { - if ( this._conversionHelpers.has( name ) ) { + addAlias( alias, dispatcher ) { + const groupEntry = [ ...this._groups.entries() ].find( ( [ , dispatchers ] ) => dispatchers.includes( dispatcher ) ); + + const groupName = groupEntry ? groupEntry[ 0 ] : null; + + const helper = this._helpers.get( groupName ); + + this._defineGroup( alias, dispatcher, helper ); + } + + _defineGroup( name, dispatcher, helpers ) { + if ( this._groups.has( name ) ) { /** * Trying to register a group name that has already been registered. * * @error conversion-register-group-exists */ - throw new CKEditorError( 'conversion-register-group-exists: Trying to register' + - 'a group name that has already been registered.' ); + throw new CKEditorError( 'conversion-group-exists: Trying to register a group name that has already been registered.' ); + } + + const group = []; + + const dispatchers = Array.isArray( dispatcher ) ? dispatcher : [ dispatcher ]; + + for ( const dispatcher of dispatchers ) { + group.push( dispatcher ); } - this._conversionHelpers.set( name, conversionHelpers ); + this._groups.set( name, group ); + this._helpers.set( name, helpers ); } /** @@ -149,7 +173,9 @@ export default class Conversion { * @returns {module:engine/conversion/downcasthelpers~DowncastHelpers|module:engine/conversion/upcasthelpers~UpcastHelpers} */ for( groupName ) { - return this._getConversionHelpers( groupName ); + const ConversionHelper = this._getConversionHelpers( groupName ); + + return new ConversionHelper( this._groups.get( groupName ) ); } /** @@ -545,7 +571,7 @@ export default class Conversion { * @returns {module:engine/conversion/downcasthelpers~DowncastHelpers|module:engine/conversion/upcasthelpers~UpcastHelpers} */ _getConversionHelpers( groupName ) { - if ( !this._conversionHelpers.has( groupName ) ) { + if ( !this._helpers.has( groupName ) ) { /** * Trying to add a converter to an unknown dispatchers group. * @@ -554,7 +580,7 @@ export default class Conversion { throw new CKEditorError( 'conversion-for-unknown-group: Trying to add a converter to an unknown dispatchers group.' ); } - return this._conversionHelpers.get( groupName ); + return this._helpers.get( groupName ); } } diff --git a/src/conversion/conversionhelpers.js b/src/conversion/conversionhelpers.js new file mode 100644 index 000000000..e282aff92 --- /dev/null +++ b/src/conversion/conversionhelpers.js @@ -0,0 +1,53 @@ +/** + * @license Copyright (c) 2003-2019, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +/** + * @module engine/conversion/conversionhelpers + */ + +/** + * Base class for conversion helpers. + */ +export default class ConversionHelpers { + /** + * Creates a conversion helpers instance. + * + * @param {Array.} dispatcher + */ + constructor( dispatcher ) { + this._dispatchers = Array.isArray( dispatcher ) ? dispatcher : [ dispatcher ]; + } + + /** + * Registers a conversion helper. + * + * **Note**: See full usage example in the `{@link module:engine/conversion/conversion~Conversion#for conversion.for()}` + * method description. + * + * @param {Function} conversionHelper The function to be called on event. + * @returns {module:engine/conversion/downcasthelpers~DowncastHelpers|module:engine/conversion/upcasthelpers~UpcastHelpers} + */ + add( conversionHelper ) { + this._addToDispatchers( conversionHelper ); + + return this; + } + + /** + * Helper function for the `Conversion` `.add()` method. + * + * Calls `conversionHelper` on each dispatcher from the group specified earlier in the `.for()` call, effectively + * adding converters to all specified dispatchers. + * + * @private + * @param {Function} conversionHelper + */ + _addToDispatchers( conversionHelper ) { + for ( const dispatcher of this._dispatchers ) { + conversionHelper( dispatcher ); + } + } +} From cd179d8931d83834dbd4554c5fe9350843f3b926 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 7 Feb 2019 13:25:11 +0100 Subject: [PATCH 03/11] Refactor tests for ConversionHelpers. --- tests/conversion/conversionhelpers.js | 41 +++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 tests/conversion/conversionhelpers.js diff --git a/tests/conversion/conversionhelpers.js b/tests/conversion/conversionhelpers.js new file mode 100644 index 000000000..fac23e7a9 --- /dev/null +++ b/tests/conversion/conversionhelpers.js @@ -0,0 +1,41 @@ +/** + * @license Copyright (c) 2003-2019, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +import ConversionHelpers from '../../src/conversion/conversionhelpers'; + +describe( 'ConversionHelpers', () => { + describe( 'add()', () => { + const dispA = Symbol( 'dispA' ); + const dispB = Symbol( 'dispB' ); + + it( 'should call a helper for one defined dispatcher', () => { + const spy = sinon.spy(); + const helpers = new ConversionHelpers( dispA ); + + helpers.add( spy ); + + sinon.assert.calledOnce( spy ); + sinon.assert.calledWithExactly( spy, dispA ); + } ); + + it( 'should call helper for all defined dispatcherers', () => { + const spy = sinon.spy(); + const helpers = new ConversionHelpers( [ dispA, dispB ] ); + + helpers.add( spy ); + + sinon.assert.calledTwice( spy ); + sinon.assert.calledWithExactly( spy, dispA ); + sinon.assert.calledWithExactly( spy, dispB ); + } ); + + it( 'should be chainable', () => { + const spy = sinon.spy(); + const helpers = new ConversionHelpers( dispA ); + + expect( helpers.add( spy ) ).to.equal( helpers ); + } ); + } ); +} ); From 3089b5884cdd0a4f9bdbb9e29e0d6fea00374cad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 7 Feb 2019 13:37:36 +0100 Subject: [PATCH 04/11] Align Conversion tests to the changes in Conversion constructor API. --- tests/conversion/conversion.js | 102 +++++++++++---------------------- 1 file changed, 35 insertions(+), 67 deletions(-) diff --git a/tests/conversion/conversion.js b/tests/conversion/conversion.js index b32f65eef..5ab8a410e 100644 --- a/tests/conversion/conversion.js +++ b/tests/conversion/conversion.js @@ -3,7 +3,7 @@ * For licensing, see LICENSE.md. */ -import Conversion, { ConversionHelpers } from '../../src/conversion/conversion'; +import Conversion from '../../src/conversion/conversion'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import UpcastDispatcher from '../../src/conversion/upcastdispatcher'; @@ -17,35 +17,33 @@ import Model from '../../src/model/model'; import { parse as viewParse, stringify as viewStringify } from '../../src/dev-utils/view'; import { stringify as modelStringify } from '../../src/dev-utils/model'; +import ConversionHelpers from '../../src/conversion/conversionhelpers'; describe( 'Conversion', () => { - let conversion, dispA, dispB, dispC; + let conversion, downcastDispA, upcastDispaA, downcastDispB; beforeEach( () => { - conversion = new Conversion(); - // Placeholders. Will be used only to see if their were given as attribute for a spy function. - dispA = Symbol( 'dispA' ); - dispB = Symbol( 'dispB' ); - dispC = Symbol( 'dispC' ); - - conversion.register( 'ab', new UpcastHelpers( [ dispA, dispB ] ) ); - conversion.register( 'a', new UpcastHelpers( dispA ) ); - conversion.register( 'b', new UpcastHelpers( dispB ) ); - conversion.register( 'c', new DowncastHelpers( dispC ) ); + downcastDispA = Symbol( 'downA' ); + downcastDispB = Symbol( 'downB' ); + + upcastDispaA = Symbol( 'upA' ); + + conversion = new Conversion( [ downcastDispA, downcastDispB ], upcastDispaA ); } ); - describe( 'register()', () => { + describe( 'addAlias()', () => { it( 'should throw when trying to use same group name twice', () => { expect( () => { - conversion.register( 'ab' ); - } ).to.throw( CKEditorError, /conversion-register-group-exists/ ); + conversion.addAlias( 'upcast', upcastDispaA ); + } ).to.throw( CKEditorError, /conversion-group-exists/ ); } ); } ); describe( 'for()', () => { it( 'should return ConversionHelpers', () => { - expect( conversion.for( 'ab' ) ).to.be.instanceof( ConversionHelpers ); + expect( conversion.for( 'upcast' ) ).to.be.instanceof( ConversionHelpers ); + expect( conversion.for( 'downcast' ) ).to.be.instanceof( ConversionHelpers ); } ); it( 'should throw if non-existing group name has been used', () => { @@ -55,10 +53,15 @@ describe( 'Conversion', () => { } ); it( 'should return proper helpers for group', () => { - expect( conversion.for( 'ab' ) ).to.be.instanceof( UpcastHelpers ); - expect( conversion.for( 'a' ) ).to.be.instanceof( UpcastHelpers ); - expect( conversion.for( 'b' ) ).to.be.instanceof( UpcastHelpers ); - expect( conversion.for( 'c' ) ).to.be.instanceof( DowncastHelpers ); + expect( conversion.for( 'upcast' ) ).to.be.instanceof( UpcastHelpers ); + + conversion.addAlias( 'foo', upcastDispaA ); + expect( conversion.for( 'foo' ) ).to.be.instanceof( UpcastHelpers ); + + expect( conversion.for( 'downcast' ) ).to.be.instanceof( DowncastHelpers ); + + conversion.addAlias( 'bar', downcastDispB ); + expect( conversion.for( 'bar' ) ).to.be.instanceof( DowncastHelpers ); } ); } ); @@ -71,22 +74,24 @@ describe( 'Conversion', () => { } ); it( 'should be chainable', () => { - const forResult = conversion.for( 'ab' ); - const addResult = forResult.add( () => {} ); + const helpers = conversion.for( 'upcast' ); + const addResult = helpers.add( () => {} ); - expect( addResult ).to.equal( addResult.add( () => {} ) ); + expect( addResult ).to.equal( helpers ); } ); it( 'should fire given helper for every dispatcher in given group', () => { - conversion.for( 'ab' ).add( helperA ); + conversion.for( 'downcast' ).add( helperA ); - expect( helperA.calledWithExactly( dispA ) ).to.be.true; - expect( helperA.calledWithExactly( dispB ) ).to.be.true; + expect( helperA.calledWithExactly( downcastDispA ) ).to.be.true; + expect( helperA.calledWithExactly( downcastDispB ) ).to.be.true; + expect( helperA.calledWithExactly( upcastDispaA ) ).to.be.false; - conversion.for( 'b' ).add( helperB ); + conversion.for( 'upcast' ).add( helperB ); - expect( helperB.calledWithExactly( dispA ) ).to.be.false; - expect( helperB.calledWithExactly( dispB ) ).to.be.true; + expect( helperB.calledWithExactly( downcastDispA ) ).to.be.false; + expect( helperB.calledWithExactly( downcastDispB ) ).to.be.false; + expect( helperB.calledWithExactly( upcastDispaA ) ).to.be.true; } ); } ); @@ -121,9 +126,7 @@ describe( 'Conversion', () => { viewDispatcher.on( 'element', convertToModelFragment(), { priority: 'lowest' } ); viewDispatcher.on( 'documentFragment', convertToModelFragment(), { priority: 'lowest' } ); - conversion = new Conversion(); - conversion.register( 'upcast', new UpcastHelpers( [ viewDispatcher ] ) ); - conversion.register( 'downcast', new DowncastHelpers( controller.downcastDispatcher ) ); + conversion = new Conversion( controller.downcastDispatcher, viewDispatcher ); } ); describe( 'elementToElement', () => { @@ -696,38 +699,3 @@ describe( 'Conversion', () => { } } ); } ); - -describe( 'ConversionHelpers', () => { - describe( 'add()', () => { - const dispA = Symbol( 'dispA' ); - const dispB = Symbol( 'dispB' ); - - it( 'should call a helper for one defined dispatcher', () => { - const spy = sinon.spy(); - const helpers = new ConversionHelpers( dispA ); - - helpers.add( spy ); - - sinon.assert.calledOnce( spy ); - sinon.assert.calledWithExactly( spy, dispA ); - } ); - - it( 'should call helper for all defined dispatcherers', () => { - const spy = sinon.spy(); - const helpers = new ConversionHelpers( [ dispA, dispB ] ); - - helpers.add( spy ); - - sinon.assert.calledTwice( spy ); - sinon.assert.calledWithExactly( spy, dispA ); - sinon.assert.calledWithExactly( spy, dispB ); - } ); - - it( 'should be chainable', () => { - const spy = sinon.spy(); - const helpers = new ConversionHelpers( dispA ); - - expect( helpers.add( spy ) ).to.equal( helpers ); - } ); - } ); -} ); From 37bf7ac89a5a69557ea1c22606cb53d82d8dbe9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 7 Feb 2019 13:42:17 +0100 Subject: [PATCH 05/11] Align UpcastHelpers tests to the changes in Conversion API. --- tests/conversion/upcasthelpers.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/conversion/upcasthelpers.js b/tests/conversion/upcasthelpers.js index 4b8ab5f2b..31f3f1ad3 100644 --- a/tests/conversion/upcasthelpers.js +++ b/tests/conversion/upcasthelpers.js @@ -3,7 +3,6 @@ * For licensing, see LICENSE.md. */ -import Conversion from '../../src/conversion/conversion'; import UpcastDispatcher from '../../src/conversion/upcastdispatcher'; import ViewContainerElement from '../../src/view/containerelement'; @@ -30,7 +29,7 @@ import ViewSelection from '../../src/view/selection'; import ViewRange from '../../src/view/range'; describe( 'UpcastHelpers', () => { - let upcastDispatcher, model, schema, conversion, upcastHelpers; + let upcastDispatcher, model, schema, upcastHelpers; beforeEach( () => { model = new Model(); @@ -51,9 +50,7 @@ describe( 'UpcastHelpers', () => { upcastDispatcher.on( 'element', convertToModelFragment(), { priority: 'lowest' } ); upcastDispatcher.on( 'documentFragment', convertToModelFragment(), { priority: 'lowest' } ); - conversion = new Conversion(); upcastHelpers = new UpcastHelpers( upcastDispatcher ); - conversion.register( 'upcast', upcastHelpers ); } ); describe( '.elementToElement()', () => { From e9a28e91d17c4a0d0437e7cee30873c4095f6da1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 7 Feb 2019 13:42:28 +0100 Subject: [PATCH 06/11] Align DowncastHelpers tests to the changes in Conversion API. --- tests/conversion/downcasthelpers.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/conversion/downcasthelpers.js b/tests/conversion/downcasthelpers.js index fa0f6b231..78c355e9c 100644 --- a/tests/conversion/downcasthelpers.js +++ b/tests/conversion/downcasthelpers.js @@ -5,8 +5,6 @@ import EditingController from '../../src/controller/editingcontroller'; -import Conversion from '../../src/conversion/conversion'; - import Model from '../../src/model/model'; import ModelElement from '../../src/model/element'; import ModelText from '../../src/model/text'; @@ -36,7 +34,7 @@ import createViewRoot from '../view/_utils/createroot'; import { setData as setModelData } from '../../src/dev-utils/model'; describe( 'DowncastHelpers', () => { - let conversion, model, modelRoot, viewRoot, downcastHelpers, controller; + let model, modelRoot, viewRoot, downcastHelpers, controller; let modelRootStart; @@ -55,9 +53,6 @@ describe( 'DowncastHelpers', () => { downcastHelpers = new DowncastHelpers( controller.downcastDispatcher ); - conversion = new Conversion(); - conversion.register( 'downcast', downcastHelpers ); - modelRootStart = model.createPositionAt( modelRoot, 0 ); } ); From 9076ac90a09ef4ed9384caad1845c80a4ffd4366 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 7 Feb 2019 13:50:09 +0100 Subject: [PATCH 07/11] Throw error when trying to add an alias for not registered dispatcher. --- src/conversion/conversion.js | 14 ++++++++++++-- tests/conversion/conversion.js | 6 ++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/conversion/conversion.js b/src/conversion/conversion.js index 450767269..0f9b72999 100644 --- a/src/conversion/conversion.js +++ b/src/conversion/conversion.js @@ -85,7 +85,17 @@ export default class Conversion { addAlias( alias, dispatcher ) { const groupEntry = [ ...this._groups.entries() ].find( ( [ , dispatchers ] ) => dispatchers.includes( dispatcher ) ); - const groupName = groupEntry ? groupEntry[ 0 ] : null; + if ( !groupEntry ) { + /** + * Trying to register and alias for a dispatcher that nas not been registered. + * + * @error conversion-add-alias-dispatcher-not-registered + */ + throw new CKEditorError( 'conversion-add-alias-dispatcher-not-registered: ' + + 'Trying to register and alias for a dispatcher that nas not been registered.' ); + } + + const groupName = groupEntry[ 0 ]; const helper = this._helpers.get( groupName ); @@ -97,7 +107,7 @@ export default class Conversion { /** * Trying to register a group name that has already been registered. * - * @error conversion-register-group-exists + * @error conversion-group-exists */ throw new CKEditorError( 'conversion-group-exists: Trying to register a group name that has already been registered.' ); } diff --git a/tests/conversion/conversion.js b/tests/conversion/conversion.js index 5ab8a410e..a4d1c60f7 100644 --- a/tests/conversion/conversion.js +++ b/tests/conversion/conversion.js @@ -38,6 +38,12 @@ describe( 'Conversion', () => { conversion.addAlias( 'upcast', upcastDispaA ); } ).to.throw( CKEditorError, /conversion-group-exists/ ); } ); + + it( 'should throw when trying to add not registered dispatcher', () => { + expect( () => { + conversion.addAlias( 'foo', {} ); + } ).to.throw( CKEditorError, /conversion-add-alias-dispatcher-not-registered/ ); + } ); } ); describe( 'for()', () => { From e1b30c032c0915cb181261a38e17204aa546379b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 7 Feb 2019 14:47:57 +0100 Subject: [PATCH 08/11] Simplify ConversionHelpers class. --- src/conversion/conversion.js | 76 +++++++++++++++++---------- src/conversion/conversionhelpers.js | 23 ++------ tests/controller/datacontroller.js | 4 +- tests/controller/editingcontroller.js | 2 +- tests/conversion/conversionhelpers.js | 4 +- tests/conversion/downcasthelpers.js | 10 ++-- tests/conversion/upcasthelpers.js | 2 +- 7 files changed, 65 insertions(+), 56 deletions(-) diff --git a/src/conversion/conversion.js b/src/conversion/conversion.js index 0f9b72999..0dc3601f7 100644 --- a/src/conversion/conversion.js +++ b/src/conversion/conversion.js @@ -58,17 +58,35 @@ import DowncastHelpers from './downcasthelpers'; export default class Conversion { /** * Creates a new conversion instance. + * + * @param {module:engine/conversion/downcastdispatcher~DowncastDispatcher| + * module:engine/conversion/upcastdispatcher~UpcastDispatcher|Array.} downcastDispatchers + * @param {module:engine/conversion/downcastdispatcher~DowncastDispatcher| + * module:engine/conversion/upcastdispatcher~UpcastDispatcher|Array.} upcastDispatchers */ constructor( downcastDispatchers, upcastDispatchers ) { /** + * Maps dispatchers group name to ConversionHelpers class (Upcast or Downcast). + * * @private - * @member {Map} + * @member {Map.} */ this._helpers = new Map(); + + /** + * Maps dispatchers group name to array with dispatchers. + * + * @private + * @member {Map.>} + */ this._groups = new Map(); - this._defineGroup( 'downcast', downcastDispatchers, DowncastHelpers ); - this._defineGroup( 'upcast', upcastDispatchers, UpcastHelpers ); + // Define default 'downcast' & 'upcast' dispatchers group. + this._defineDispatchersGroup( 'downcast', downcastDispatchers, DowncastHelpers ); + this._defineDispatchersGroup( 'upcast', upcastDispatchers, UpcastHelpers ); } /** @@ -99,36 +117,14 @@ export default class Conversion { const helper = this._helpers.get( groupName ); - this._defineGroup( alias, dispatcher, helper ); - } - - _defineGroup( name, dispatcher, helpers ) { - if ( this._groups.has( name ) ) { - /** - * Trying to register a group name that has already been registered. - * - * @error conversion-group-exists - */ - throw new CKEditorError( 'conversion-group-exists: Trying to register a group name that has already been registered.' ); - } - - const group = []; - - const dispatchers = Array.isArray( dispatcher ) ? dispatcher : [ dispatcher ]; - - for ( const dispatcher of dispatchers ) { - group.push( dispatcher ); - } - - this._groups.set( name, group ); - this._helpers.set( name, helpers ); + this._defineDispatchersGroup( alias, dispatcher, helper ); } /** * Provides a chainable API to assign converters to conversion dispatchers. * * You can use conversion helpers available directly in the `for()` chain or your custom ones via - * the {@link module:engine/conversion/conversion~ConversionHelpers#add `add()`} method. + * the {@link module:engine/conversion/conversionhelpers~ConversionHelpers#add `add()`} method. * * # Using bulit-in conversion helpers * @@ -570,6 +566,32 @@ export default class Conversion { } } + /** + * Registers dispatchers group. + * + * @private + * @param {String} name Group name. + * @param {module:engine/conversion/downcastdispatcher~DowncastDispatcher| + * module:engine/conversion/upcastdispatcher~UpcastDispatcher|Array.} dispatcher + * @param {module:engine/conversion/conversionhelpers~ConversionHelpers} helpers + */ + _defineDispatchersGroup( name, dispatcher, helpers ) { + if ( this._groups.has( name ) ) { + /** + * Trying to register a group name that has already been registered. + * + * @error conversion-group-exists + */ + throw new CKEditorError( 'conversion-group-exists: Trying to register a group name that has already been registered.' ); + } + + const dispatchers = Array.isArray( dispatcher ) ? dispatcher : [ dispatcher ]; + + this._groups.set( name, dispatchers ); + this._helpers.set( name, helpers ); + } + /** * Returns conversion helpers registered under a given name. * diff --git a/src/conversion/conversionhelpers.js b/src/conversion/conversionhelpers.js index e282aff92..c32dfd9cf 100644 --- a/src/conversion/conversionhelpers.js +++ b/src/conversion/conversionhelpers.js @@ -15,10 +15,10 @@ export default class ConversionHelpers { * Creates a conversion helpers instance. * * @param {Array.} dispatcher + * module:engine/conversion/upcastdispatcher~UpcastDispatcher>} dispatchers */ - constructor( dispatcher ) { - this._dispatchers = Array.isArray( dispatcher ) ? dispatcher : [ dispatcher ]; + constructor( dispatchers ) { + this._dispatchers = dispatchers; } /** @@ -31,23 +31,10 @@ export default class ConversionHelpers { * @returns {module:engine/conversion/downcasthelpers~DowncastHelpers|module:engine/conversion/upcasthelpers~UpcastHelpers} */ add( conversionHelper ) { - this._addToDispatchers( conversionHelper ); - - return this; - } - - /** - * Helper function for the `Conversion` `.add()` method. - * - * Calls `conversionHelper` on each dispatcher from the group specified earlier in the `.for()` call, effectively - * adding converters to all specified dispatchers. - * - * @private - * @param {Function} conversionHelper - */ - _addToDispatchers( conversionHelper ) { for ( const dispatcher of this._dispatchers ) { conversionHelper( dispatcher ); } + + return this; } } diff --git a/tests/controller/datacontroller.js b/tests/controller/datacontroller.js index 94cf42e75..fa8cc004c 100644 --- a/tests/controller/datacontroller.js +++ b/tests/controller/datacontroller.js @@ -39,8 +39,8 @@ describe( 'DataController', () => { data = new DataController( model, htmlDataProcessor ); - upcastHelpers = new UpcastHelpers( data.upcastDispatcher ); - downcastHelpers = new DowncastHelpers( data.downcastDispatcher ); + upcastHelpers = new UpcastHelpers( [ data.upcastDispatcher ] ); + downcastHelpers = new DowncastHelpers( [ data.downcastDispatcher ] ); } ); describe( 'constructor()', () => { diff --git a/tests/controller/editingcontroller.js b/tests/controller/editingcontroller.js index 3017741bb..83baf6417 100644 --- a/tests/controller/editingcontroller.js +++ b/tests/controller/editingcontroller.js @@ -90,7 +90,7 @@ describe( 'EditingController', () => { model.schema.register( 'paragraph', { inheritAllFrom: '$block' } ); model.schema.register( 'div', { inheritAllFrom: '$block' } ); - const downcastHelpers = new DowncastHelpers( editing.downcastDispatcher ); + const downcastHelpers = new DowncastHelpers( [ editing.downcastDispatcher ] ); downcastHelpers.elementToElement( { model: 'paragraph', view: 'p' } ); downcastHelpers.elementToElement( { model: 'div', view: 'div' } ); diff --git a/tests/conversion/conversionhelpers.js b/tests/conversion/conversionhelpers.js index fac23e7a9..6f97d6e10 100644 --- a/tests/conversion/conversionhelpers.js +++ b/tests/conversion/conversionhelpers.js @@ -12,7 +12,7 @@ describe( 'ConversionHelpers', () => { it( 'should call a helper for one defined dispatcher', () => { const spy = sinon.spy(); - const helpers = new ConversionHelpers( dispA ); + const helpers = new ConversionHelpers( [ dispA ] ); helpers.add( spy ); @@ -33,7 +33,7 @@ describe( 'ConversionHelpers', () => { it( 'should be chainable', () => { const spy = sinon.spy(); - const helpers = new ConversionHelpers( dispA ); + const helpers = new ConversionHelpers( [ dispA ] ); expect( helpers.add( spy ) ).to.equal( helpers ); } ); diff --git a/tests/conversion/downcasthelpers.js b/tests/conversion/downcasthelpers.js index 78c355e9c..1dcc6594b 100644 --- a/tests/conversion/downcasthelpers.js +++ b/tests/conversion/downcasthelpers.js @@ -51,7 +51,7 @@ describe( 'DowncastHelpers', () => { viewRoot = controller.view.document.getRoot(); - downcastHelpers = new DowncastHelpers( controller.downcastDispatcher ); + downcastHelpers = new DowncastHelpers( [ controller.downcastDispatcher ] ); modelRootStart = model.createPositionAt( modelRoot, 0 ); } ); @@ -1469,7 +1469,7 @@ describe( 'downcast converters', () => { controller.view.document.getRoot()._name = 'div'; dispatcher = controller.downcastDispatcher; - const downcastHelpers = new DowncastHelpers( dispatcher ); + const downcastHelpers = new DowncastHelpers( [ dispatcher ] ); downcastHelpers.elementToElement( { model: 'paragraph', view: 'p' } ); modelRootStart = model.createPositionAt( modelRoot, 0 ); @@ -1625,7 +1625,7 @@ describe( 'downcast converters', () => { const modelP = new ModelElement( 'paragraph', null, new ModelText( 'foo' ) ); const modelWidget = new ModelElement( 'widget', null, modelP ); - const downcastHelpers = new DowncastHelpers( dispatcher ); + const downcastHelpers = new DowncastHelpers( [ dispatcher ] ); downcastHelpers.elementToElement( { model: 'widget', view: 'widget' } ); model.change( writer => { @@ -1801,7 +1801,7 @@ describe( 'downcast selection converters', () => { dispatcher.on( 'insert:$text', insertText() ); - downcastHelpers = new DowncastHelpers( dispatcher ); + downcastHelpers = new DowncastHelpers( [ dispatcher ] ); downcastHelpers.attributeToElement( { model: 'bold', view: 'strong' } ); downcastHelpers.markerToHighlight( { model: 'marker', view: { classes: 'marker' }, converterPriority: 1 } ); @@ -2248,7 +2248,7 @@ describe( 'downcast selection converters', () => { model.schema.extend( 'td', { allowIn: 'tr' } ); model.schema.extend( '$text', { allowIn: 'td' } ); - const downcastHelpers = new DowncastHelpers( dispatcher ); + const downcastHelpers = new DowncastHelpers( [ dispatcher ] ); // "Universal" converter to convert table structure. downcastHelpers.elementToElement( { model: 'table', view: 'table' } ); diff --git a/tests/conversion/upcasthelpers.js b/tests/conversion/upcasthelpers.js index 31f3f1ad3..99cb62254 100644 --- a/tests/conversion/upcasthelpers.js +++ b/tests/conversion/upcasthelpers.js @@ -50,7 +50,7 @@ describe( 'UpcastHelpers', () => { upcastDispatcher.on( 'element', convertToModelFragment(), { priority: 'lowest' } ); upcastDispatcher.on( 'documentFragment', convertToModelFragment(), { priority: 'lowest' } ); - upcastHelpers = new UpcastHelpers( upcastDispatcher ); + upcastHelpers = new UpcastHelpers( [ upcastDispatcher ] ); } ); describe( '.elementToElement()', () => { From 4c85d0c565105911bc8b35135fab6e354af8ce9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 7 Feb 2019 14:55:19 +0100 Subject: [PATCH 09/11] Update documentation for Conversion class. --- src/conversion/conversion.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/conversion/conversion.js b/src/conversion/conversion.js index 0dc3601f7..97cf67ffe 100644 --- a/src/conversion/conversion.js +++ b/src/conversion/conversion.js @@ -60,11 +60,9 @@ export default class Conversion { * Creates a new conversion instance. * * @param {module:engine/conversion/downcastdispatcher~DowncastDispatcher| - * module:engine/conversion/upcastdispatcher~UpcastDispatcher|Array.} downcastDispatchers - * @param {module:engine/conversion/downcastdispatcher~DowncastDispatcher| - * module:engine/conversion/upcastdispatcher~UpcastDispatcher|Array.} upcastDispatchers + * Array.} downcastDispatchers + * @param {module:engine/conversion/upcastdispatcher~UpcastDispatcher| + * Array.} upcastDispatchers */ constructor( downcastDispatchers, upcastDispatchers ) { /** @@ -84,7 +82,7 @@ export default class Conversion { */ this._groups = new Map(); - // Define default 'downcast' & 'upcast' dispatchers group. + // Define default 'downcast' & 'upcast' dispatchers groups. Those groups are always available as two-way converters needs them. this._defineDispatchersGroup( 'downcast', downcastDispatchers, DowncastHelpers ); this._defineDispatchersGroup( 'upcast', upcastDispatchers, UpcastHelpers ); } @@ -92,7 +90,10 @@ export default class Conversion { /** * Define an alias for registered dispatcher. * - * const conversion = new Conversion( [ dataDowncastDispatcher, editingDowncastDispatcher ], upcastDispatcher ); + * const conversion = new Conversion( + * [ dataDowncastDispatcher, editingDowncastDispatcher ], + * upcastDispatcher + * ); * * conversion.addAlias( 'dataDowncast', dataDowncastDispatcher ); * From 637287e83e20927d2922fe24195966f7db969888 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 7 Feb 2019 14:59:44 +0100 Subject: [PATCH 10/11] Simplify Conversion class by removing redundant method. --- src/conversion/conversion.js | 39 +++++++++++++----------------------- 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/src/conversion/conversion.js b/src/conversion/conversion.js index 97cf67ffe..db126f9aa 100644 --- a/src/conversion/conversion.js +++ b/src/conversion/conversion.js @@ -122,7 +122,10 @@ export default class Conversion { } /** - * Provides a chainable API to assign converters to conversion dispatchers. + * Provides a chainable API to assign converters to conversion dispatchers group. + * + * If the given group name has not been registered, the + * {@link module:utils/ckeditorerror~CKEditorError `conversion-for-unknown-group` error} is thrown. * * You can use conversion helpers available directly in the `for()` chain or your custom ones via * the {@link module:engine/conversion/conversionhelpers~ConversionHelpers#add `add()`} method. @@ -180,7 +183,16 @@ export default class Conversion { * @returns {module:engine/conversion/downcasthelpers~DowncastHelpers|module:engine/conversion/upcasthelpers~UpcastHelpers} */ for( groupName ) { - const ConversionHelper = this._getConversionHelpers( groupName ); + if ( !this._groups.has( groupName ) ) { + /** + * Trying to add a converter to an unknown dispatchers group. + * + * @error conversion-for-unknown-group + */ + throw new CKEditorError( 'conversion-for-unknown-group: Trying to add a converter to an unknown dispatchers group.' ); + } + + const ConversionHelper = this._helpers.get( groupName ); return new ConversionHelper( this._groups.get( groupName ) ); } @@ -592,29 +604,6 @@ export default class Conversion { this._groups.set( name, dispatchers ); this._helpers.set( name, helpers ); } - - /** - * Returns conversion helpers registered under a given name. - * - * If the given group name has not been registered, the - * {@link module:utils/ckeditorerror~CKEditorError `conversion-for-unknown-group` error} is thrown. - * - * @private - * @param {String} groupName - * @returns {module:engine/conversion/downcasthelpers~DowncastHelpers|module:engine/conversion/upcasthelpers~UpcastHelpers} - */ - _getConversionHelpers( groupName ) { - if ( !this._helpers.has( groupName ) ) { - /** - * Trying to add a converter to an unknown dispatchers group. - * - * @error conversion-for-unknown-group - */ - throw new CKEditorError( 'conversion-for-unknown-group: Trying to add a converter to an unknown dispatchers group.' ); - } - - return this._helpers.get( groupName ); - } } /** From ad0cee7c14491f685c3eb7391a1a544dad409b46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 8 Feb 2019 12:13:25 +0100 Subject: [PATCH 11/11] Change how dispatchers and conversion helpers are stored in Conversion. --- src/conversion/conversion.js | 57 +++++++++++++++--------------------- 1 file changed, 23 insertions(+), 34 deletions(-) diff --git a/src/conversion/conversion.js b/src/conversion/conversion.js index db126f9aa..1f8445b3d 100644 --- a/src/conversion/conversion.js +++ b/src/conversion/conversion.js @@ -66,25 +66,20 @@ export default class Conversion { */ constructor( downcastDispatchers, upcastDispatchers ) { /** - * Maps dispatchers group name to ConversionHelpers class (Upcast or Downcast). + * Maps dispatchers group name to ConversionHelpers instances. * * @private - * @member {Map.} + * @member {Map.} */ this._helpers = new Map(); - /** - * Maps dispatchers group name to array with dispatchers. - * - * @private - * @member {Map.>} - */ - this._groups = new Map(); - // Define default 'downcast' & 'upcast' dispatchers groups. Those groups are always available as two-way converters needs them. - this._defineDispatchersGroup( 'downcast', downcastDispatchers, DowncastHelpers ); - this._defineDispatchersGroup( 'upcast', upcastDispatchers, UpcastHelpers ); + this._downcast = Array.isArray( downcastDispatchers ) ? downcastDispatchers : [ downcastDispatchers ]; + this._createConversionHelpers( { name: 'downcast', dispatchers: this._downcast, isDowncast: true } ); + + this._upcast = Array.isArray( upcastDispatchers ) ? upcastDispatchers : [ upcastDispatchers ]; + this._createConversionHelpers( { name: 'upcast', dispatchers: this._upcast, isDowncast: false } ); + } /** @@ -102,9 +97,10 @@ export default class Conversion { * module:engine/conversion/upcastdispatcher~UpcastDispatcher} dispatcher Dispatcher which should have an alias. */ addAlias( alias, dispatcher ) { - const groupEntry = [ ...this._groups.entries() ].find( ( [ , dispatchers ] ) => dispatchers.includes( dispatcher ) ); + const isDowncast = this._downcast.includes( dispatcher ); + const isUpcast = this._upcast.includes( dispatcher ); - if ( !groupEntry ) { + if ( !isUpcast && !isDowncast ) { /** * Trying to register and alias for a dispatcher that nas not been registered. * @@ -114,11 +110,7 @@ export default class Conversion { 'Trying to register and alias for a dispatcher that nas not been registered.' ); } - const groupName = groupEntry[ 0 ]; - - const helper = this._helpers.get( groupName ); - - this._defineDispatchersGroup( alias, dispatcher, helper ); + this._createConversionHelpers( { name: alias, dispatchers: [ dispatcher ], isDowncast: isDowncast } ); } /** @@ -183,7 +175,7 @@ export default class Conversion { * @returns {module:engine/conversion/downcasthelpers~DowncastHelpers|module:engine/conversion/upcasthelpers~UpcastHelpers} */ for( groupName ) { - if ( !this._groups.has( groupName ) ) { + if ( !this._helpers.has( groupName ) ) { /** * Trying to add a converter to an unknown dispatchers group. * @@ -192,9 +184,7 @@ export default class Conversion { throw new CKEditorError( 'conversion-for-unknown-group: Trying to add a converter to an unknown dispatchers group.' ); } - const ConversionHelper = this._helpers.get( groupName ); - - return new ConversionHelper( this._groups.get( groupName ) ); + return this._helpers.get( groupName ); } /** @@ -580,17 +570,17 @@ export default class Conversion { } /** - * Registers dispatchers group. + * Creates and caches conversion helpers for given dispatchers group. * * @private - * @param {String} name Group name. - * @param {module:engine/conversion/downcastdispatcher~DowncastDispatcher| - * module:engine/conversion/upcastdispatcher~UpcastDispatcher|Array.} dispatcher - * @param {module:engine/conversion/conversionhelpers~ConversionHelpers} helpers + * @param {Object} options + * @param {String} options.name Group name. + * @param {Array.} options.dispatchers + * @param {Boolean} options.isDowncast */ - _defineDispatchersGroup( name, dispatcher, helpers ) { - if ( this._groups.has( name ) ) { + _createConversionHelpers( { name, dispatchers, isDowncast } ) { + if ( this._helpers.has( name ) ) { /** * Trying to register a group name that has already been registered. * @@ -599,9 +589,8 @@ export default class Conversion { throw new CKEditorError( 'conversion-group-exists: Trying to register a group name that has already been registered.' ); } - const dispatchers = Array.isArray( dispatcher ) ? dispatcher : [ dispatcher ]; + const helpers = isDowncast ? new DowncastHelpers( dispatchers ) : new UpcastHelpers( dispatchers ); - this._groups.set( name, dispatchers ); this._helpers.set( name, helpers ); } }