ソースを参照

Changed format of data returned by conversionApi#convertItem and conversionApi#convertChildren.

Oskar Wróbel 7 年 前
コミット
b4d541760d

+ 21 - 21
packages/ckeditor5-engine/src/conversion/viewconversiondispatcher.js

@@ -12,9 +12,9 @@ import ModelRange from '../model/range';
 import ModelPosition from '../model/position';
 import { SchemaContext } from '../model/schema';
 
+import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin';
 import mix from '@ckeditor/ckeditor5-utils/src/mix';
-import log from '@ckeditor/ckeditor5-utils/src/log';
 
 /**
  * `ViewConversionDispatcher` is a central point of {@link module:engine/view/view view} conversion, which is a process of
@@ -178,13 +178,13 @@ export default class ViewConversionDispatcher {
 			this.conversionApi.data = {};
 
 			// Do the conversion.
-			const range = this._convertItem( viewItem, this._contextPosition );
+			const { modelRange } = this._convertItem( viewItem, this._contextPosition );
 
 			// Conversion result is always a document fragment so let's create this fragment.
 			const documentFragment = writer.createDocumentFragment();
 
 			// When there is a conversion result.
-			if ( range ) {
+			if ( modelRange ) {
 				// Remove all empty elements that was created as a result of split.
 				this._removeEmptySplitElements();
 
@@ -227,25 +227,19 @@ export default class ViewConversionDispatcher {
 			this.fire( 'documentFragment', data, this.conversionApi );
 		}
 
-		// Handle incorrect `data.output`.
+		// Handle incorrect conversion result.
 		if ( data.modelRange && !( data.modelRange instanceof ModelRange ) ) {
 			/**
 			 * Incorrect conversion result was dropped.
 			 *
-			 * Item may be converted to either {@link module:engine/model/node~Node model node} or
-			 * {@link module:engine/model/documentfragment~DocumentFragment model document fragment}.
+			 * {@link module:engine/model/range~Range Model range} should be a conversion result.
 			 *
 			 * @error view-conversion-dispatcher-incorrect-result
 			 */
-			log.warn(
-				'view-conversion-dispatcher-incorrect-result: Incorrect conversion result was dropped.',
-				[ viewItem, data.modelRange ]
-			);
-
-			return null;
+			throw new CKEditorError( 'view-conversion-dispatcher-incorrect-result: Incorrect conversion result was dropped.' );
 		}
 
-		return data.modelRange;
+		return { modelRange: data.modelRange, cursorPosition: data.cursorPosition };
 	}
 
 	/**
@@ -253,17 +247,19 @@ export default class ViewConversionDispatcher {
 	 * @see module:engine/conversion/viewconversiondispatcher~ViewConversionApi#convertChildren
 	 */
 	_convertChildren( viewItem, cursorPosition ) {
-		const childrenRange = new ModelRange( cursorPosition );
+		const modelRange = new ModelRange( cursorPosition );
+		let nextCursorPosition = cursorPosition;
 
 		for ( const viewChild of Array.from( viewItem.getChildren() ) ) {
-			const itemRange = this._convertItem( viewChild, childrenRange.end );
+			const result = this._convertItem( viewChild, nextCursorPosition );
 
-			if ( itemRange instanceof ModelRange ) {
-				childrenRange.end = itemRange.end;
+			if ( result.modelRange instanceof ModelRange ) {
+				modelRange.end = result.modelRange.end;
+				nextCursorPosition = result.cursorPosition;
 			}
 		}
 
-		return childrenRange;
+		return { modelRange, cursorPosition: nextCursorPosition };
 	}
 
 	/**
@@ -440,7 +436,7 @@ function contextToPosition( contextDefinition, writer ) {
  * and is passed as one of parameters when {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher dispatcher}
  * fires it's events.
  *
- * TODO more explanation.
+ * TODO better explanation.
  *
  * @interface ViewConversionApi
  */
@@ -458,8 +454,10 @@ function contextToPosition( contextDefinition, writer ) {
  * @fires module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:documentFragment
  * @param {module:engine/view/item~Item} viewItem Item to convert.
  * @param {module:engine/model/position~Position} cursorPosition Position of conversion.
- * @returns {module:engine/model/range~Range|null} Model range containing result of item conversion,
+ * @returns {Object} result Conversion result.
+ * @returns {module:engine/model/range~Range|null} result.modelRange Model range containing result of item conversion,
  * created and modified by callbacks attached to fired event, or `null` if the conversion result was incorrect.
+ * @returns {module:engine/model/position~Position} result.cursorPosition Position where conversion should be continued.
  */
 
 /**
@@ -471,8 +469,10 @@ function contextToPosition( contextDefinition, writer ) {
  * @fires module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:documentFragment
  * @param {module:engine/view/item~Item} viewItem Item to convert.
  * @param {module:engine/model/position~Position} cursorPosition Position of conversion.
- * @returns {module:engine/model/range~Range} Model range containing results of conversion of all children of given item.
+ * @returns {Object} result Conversion result.
+ * @returns {module:engine/model/range~Range} result.modelRange Model range containing results of conversion of all children of given item.
  * When no children was converted then range is collapsed.
+ * @returns {module:engine/model/position~Position} result.cursorPosition Position where conversion should be continued.
  */
 
 /**

+ 34 - 80
packages/ckeditor5-engine/tests/conversion/viewconversiondispatcher.js

@@ -18,17 +18,13 @@ import ModelRange from '../../src/model/range';
 import ModelWriter from '../../src/model/writer';
 
 import first from '@ckeditor/ckeditor5-utils/src/first';
-import log from '@ckeditor/ckeditor5-utils/src/log';
-
-// Stored in case it is silenced and has to be restored.
-const logWarn = log.warn;
+import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 describe( 'ViewConversionDispatcher', () => {
 	let model;
 
 	beforeEach( () => {
 		model = new Model();
-		log.warn = logWarn;
 	} );
 
 	describe( 'constructor()', () => {
@@ -116,8 +112,6 @@ describe( 'ViewConversionDispatcher', () => {
 		} );
 
 		it( 'should fire viewCleanup event on converted view part', () => {
-			silenceWarnings();
-
 			sinon.spy( dispatcher, 'fire' );
 
 			const viewP = new ViewContainerElement( 'p' );
@@ -127,8 +121,6 @@ describe( 'ViewConversionDispatcher', () => {
 		} );
 
 		it( 'should fire proper events', () => {
-			silenceWarnings();
-
 			const viewText = new ViewText( 'foobar' );
 			const viewElement = new ViewContainerElement( 'p', null, viewText );
 			const viewFragment = new ViewDocumentFragment( viewElement );
@@ -444,23 +436,25 @@ describe( 'ViewConversionDispatcher', () => {
 
 		describe( 'convertItem()', () => {
 			it( 'should call proper converter and return correct data', () => {
-				silenceWarnings();
-
 				dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => {
 					spy();
 
-					const result1 = conversionApi.convertItem( viewP, data.cursorPosition );
-					expect( result1 ).instanceof( ModelRange );
-					expect( result1.start.path ).to.deep.equal( [ 0 ] );
-					expect( result1.end.path ).to.deep.equal( [ 1 ] );
-					expect( first( result1.getItems() ) ).to.equal( modelP );
-
-					const result2 = conversionApi.convertItem( viewText, data.cursorPosition );
-					expect( result2 ).instanceof( ModelRange );
-					expect( result2.start.path ).to.deep.equal( [ 1 ] );
-					expect( result2.end.path ).to.deep.equal( [ 7 ] );
-					expect( first( result2.getItems() ) ).to.instanceof( ModelTextProxy );
-					expect( first( result2.getItems() ).data ).to.equal( 'foobar' );
+					const pResult = conversionApi.convertItem( viewP, data.cursorPosition );
+					expect( pResult.modelRange ).instanceof( ModelRange );
+					expect( pResult.modelRange.start.path ).to.deep.equal( [ 0 ] );
+					expect( pResult.modelRange.end.path ).to.deep.equal( [ 1 ] );
+					expect( first( pResult.modelRange.getItems() ) ).to.equal( modelP );
+					expect( pResult.cursorPosition ).to.instanceof( ModelPosition );
+					expect( pResult.cursorPosition.path ).to.deep.equal( [ 1 ] );
+
+					const textResult = conversionApi.convertItem( viewText, data.cursorPosition );
+					expect( textResult.modelRange ).instanceof( ModelRange );
+					expect( textResult.modelRange.start.path ).to.deep.equal( [ 1 ] );
+					expect( textResult.modelRange.end.path ).to.deep.equal( [ 7 ] );
+					expect( first( textResult.modelRange.getItems() ) ).to.instanceof( ModelTextProxy );
+					expect( first( textResult.modelRange.getItems() ).data ).to.equal( 'foobar' );
+					expect( textResult.cursorPosition ).to.instanceof( ModelPosition );
+					expect( textResult.cursorPosition.path ).to.deep.equal( [ 7 ] );
 				} );
 
 				dispatcher.convert( new ViewDocumentFragment() );
@@ -471,60 +465,53 @@ describe( 'ViewConversionDispatcher', () => {
 			} );
 
 			it( 'should do nothing if element was not converted', () => {
-				sinon.stub( log, 'warn' );
-
 				dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => {
 					spy();
 
-					expect( conversionApi.convertItem( viewDiv, data.cursorPosition ) ).to.equal( null );
-					expect( conversionApi.convertItem( viewNull, data.cursorPosition ) ).to.equal( null );
+					expect( conversionApi.convertItem( viewDiv, data.cursorPosition ).modelRange ).to.equal( null );
+					expect( conversionApi.convertItem( viewNull, data.cursorPosition ).modelRange ).to.equal( null );
 				} );
 
 				dispatcher.convert( new ViewDocumentFragment() );
 
 				expect( spy.calledOnce ).to.be.true;
 				expect( spyNull.calledOnce ).to.be.true;
-				expect( log.warn.called ).to.be.false;
-
-				log.warn.restore();
 			} );
 
-			it( 'should return null if element was incorrectly converted and log a warning', () => {
-				sinon.stub( log, 'warn' );
-
+			it( 'should throw an error if element was incorrectly converted', () => {
 				dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => {
 					spy();
 
-					expect( conversionApi.convertItem( viewArray, data.cursorPosition ) ).to.equal( null );
+					conversionApi.convertItem( viewArray, data.cursorPosition );
 				} );
 
-				dispatcher.convert( new ViewDocumentFragment() );
+				expect( () => {
+					dispatcher.convert( new ViewDocumentFragment() );
+				} ).to.throw( CKEditorError, /^view-conversion-dispatcher-incorrect-result/ );
 
 				expect( spy.calledOnce ).to.be.true;
 				expect( spyArray.calledOnce ).to.be.true;
-				expect( log.warn.calledOnce ).to.be.true;
-
-				log.warn.restore();
 			} );
 		} );
 
 		describe( 'convertChildren()', () => {
 			it( 'should fire conversion for all children of passed element and return conversion results ' +
 				'wrapped in document fragment', () => {
-				silenceWarnings();
-
 				dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => {
 					spy();
 
 					const result = conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( rootMock ) );
 
-					expect( result ).to.be.instanceof( ModelRange );
-					expect( result.start.path ).to.deep.equal( [ 0 ] );
-					expect( result.end.path ).to.deep.equal( [ 7 ] );
-					expect( Array.from( result.getItems() ) ).to.length( 2 );
-					expect( Array.from( result.getItems() )[ 0 ] ).to.equal( modelP );
-					expect( Array.from( result.getItems() )[ 1 ] ).to.instanceof( ModelTextProxy );
-					expect( Array.from( result.getItems() )[ 1 ].data ).to.equal( 'foobar' );
+					expect( result.modelRange ).to.be.instanceof( ModelRange );
+					expect( result.modelRange.start.path ).to.deep.equal( [ 0 ] );
+					expect( result.modelRange.end.path ).to.deep.equal( [ 7 ] );
+					expect( Array.from( result.modelRange.getItems() ) ).to.length( 2 );
+					expect( Array.from( result.modelRange.getItems() )[ 0 ] ).to.equal( modelP );
+					expect( Array.from( result.modelRange.getItems() )[ 1 ] ).to.instanceof( ModelTextProxy );
+					expect( Array.from( result.modelRange.getItems() )[ 1 ].data ).to.equal( 'foobar' );
+
+					expect( result.cursorPosition ).instanceof( ModelPosition );
+					expect( result.cursorPosition.path ).to.deep.equal( [ 7 ] );
 				} );
 
 				dispatcher.convert( new ViewDocumentFragment( [ viewP, viewText ] ) );
@@ -533,33 +520,6 @@ describe( 'ViewConversionDispatcher', () => {
 				expect( spyP.calledOnce ).to.be.true;
 				expect( spyText.calledOnce ).to.be.true;
 			} );
-
-			it( 'should filter out incorrectly converted elements and log warnings', () => {
-				sinon.stub( log, 'warn' );
-
-				dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => {
-					spy();
-
-					const result = conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( rootMock ) );
-
-					expect( result ).to.be.instanceof( ModelRange );
-					expect( result.start.path ).to.deep.equal( [ 0 ] );
-					expect( result.end.path ).to.deep.equal( [ 7 ] );
-					expect( Array.from( result.getItems() ) ).to.length( 2 );
-					expect( Array.from( result.getItems() )[ 0 ] ).to.equal( modelP );
-					expect( Array.from( result.getItems() )[ 1 ] ).to.instanceof( ModelTextProxy );
-					expect( Array.from( result.getItems() )[ 1 ].data ).to.equal( 'foobar' );
-				} );
-
-				dispatcher.convert( new ViewDocumentFragment( [ viewArray, viewP, viewDiv, viewText, viewNull ] ) );
-
-				expect( spy.calledOnce ).to.be.true;
-				expect( spyNull.calledOnce ).to.be.true;
-				expect( spyArray.calledOnce ).to.be.true;
-				expect( log.warn.calledOnce ).to.be.true;
-
-				log.warn.restore();
-			} );
 		} );
 
 		describe( 'splitToAllowedParent()', () => {
@@ -649,10 +609,4 @@ describe( 'ViewConversionDispatcher', () => {
 			} );
 		} );
 	} );
-
-	// Silences warnings that pop up in tests. Use when the test checks a specific functionality and we are not interested in those logs.
-	// No need to restore `log.warn` - it is done in `afterEach()`.
-	function silenceWarnings() {
-		log.warn = () => {};
-	}
 } );