8
0
Эх сурвалжийг харах

Changed Batch.move to throw error if non-flat range is passed. Renamed InsertWeakDelta to WeakInsertDelta. Tests refactor for deltas. Docs update.

Szymon Cofalik 10 жил өмнө
parent
commit
677a784fcd

+ 1 - 1
packages/ckeditor5-engine/src/treemodel/batch.js

@@ -12,7 +12,7 @@
 CKEDITOR.define( [
 	'treemodel/delta/batch-base',
 	'treemodel/delta/insertdelta',
-	'treemodel/delta/insertweakdelta',
+	'treemodel/delta/weakinsertdelta',
 	'treemodel/delta/movedelta',
 	'treemodel/delta/removedelta',
 	'treemodel/delta/attributedelta',

+ 12 - 10
packages/ckeditor5-engine/src/treemodel/delta/movedelta.js

@@ -10,8 +10,9 @@ CKEDITOR.define( [
 	'treemodel/delta/register',
 	'treemodel/operation/moveoperation',
 	'treemodel/position',
-	'treemodel/range'
-], ( Delta, register, MoveOperation, Position, Range ) => {
+	'treemodel/range',
+	'ckeditorerror'
+], ( Delta, register, MoveOperation, Position, Range, CKEditorError ) => {
 	/**
 	 * To provide specific OT behavior and better collisions solving, {@link treeModel.Batch#move} method
 	 * uses the `MoveDelta` class which inherits from the `Delta` class and may overwrite some methods.
@@ -37,17 +38,18 @@ CKEDITOR.define( [
 	 */
 	register( 'move', function( nodeOrRange, targetPosition ) {
 		const delta = new MoveDelta();
-		let ranges;
 
 		if ( nodeOrRange instanceof Range ) {
-			// The array is reversed, so the furthest ranges get moved first.
-			// This will keep the order of nodes in ranges correct.
-			// It's like using .pop() + .unshift() when moving elements between arrays.
-			ranges = nodeOrRange.getMinimalFlatRanges().reverse();
-
-			for ( let flat of ranges ) {
-				addMoveOperation( this, delta, flat.start, flat.end.offset - flat.start.offset, targetPosition );
+			if ( !nodeOrRange.isFlat ) {
+				/**
+				 * Range to move is not flat.
+				 *
+				 * @error batch-move-range-not-flat
+				 */
+				throw new CKEditorError( 'batch-move-range-not-flat: Range to move is not flat.' );
 			}
+
+			addMoveOperation( this, delta, nodeOrRange.start, nodeOrRange.end.offset - nodeOrRange.start.offset, targetPosition );
 		} else {
 			addMoveOperation( this, delta, Position.createBefore( nodeOrRange ), 1, targetPosition );
 		}

+ 14 - 17
packages/ckeditor5-engine/src/treemodel/delta/insertweakdelta.js → packages/ckeditor5-engine/src/treemodel/delta/weakinsertdelta.js

@@ -13,35 +13,32 @@ CKEDITOR.define( [
 ], ( Delta, register, InsertOperation, NodeList ) => {
 	/**
 	 * To provide specific OT behavior and better collisions solving, the {@link treeModel.Batch#insert} method
-	 * uses the `InsertWeakDelta` class which inherits from the `Delta` class and may overwrite some methods.
+	 * uses the `WeakInsertDelta` class which inherits from the `Delta` class and may overwrite some methods.
 	 *
-	 * @class treeModel.delta.InsertWeakDelta
+	 * @class treeModel.delta.WeakInsertDelta
 	 */
-	class InsertWeakDelta extends Delta {}
+	class WeakInsertDelta extends Delta {}
 
 	/**
-	 * Inserts a node or nodes at the given position. {@link treeModel.Batch#insertWeak} is commonly used for actions
+	 * Inserts a node or nodes at the given position. {@link treeModel.Batch#weakInsert} is commonly used for actions
 	 * like typing or plain-text paste (without formatting). There are two differences between
-	 * {@link treeModel.Batch#insert} and {@link treeModel.Batch#insertWeak}:
-	 * * When using `insertWeak`, inserted nodes will have same attributes as the current attributes of
+	 * {@link treeModel.Batch#insert} and {@link treeModel.Batch#weakInsert}:
+	 * * When using `weakInsert`, inserted nodes will have same attributes as the current attributes of
 	 * {@link treeModel.Document#selection document selection}.
-	 * * During {@link treeModel.operation.transform operational transformation}, if nodes are inserted between moved nodes
-	 * they end up inserted at the position before moved range (so they do not move with the range). `insertWeak` changes
-	 * this behavior - inserted nodes "sticks" with range and end up in moved range.
-	 *
-	 *		       |----------|                move + insert              move + insertWeak
-	 *		<p>fo[o^ba]r</p><p>|</p>      <p>foxyzr</p><p>oba</p>       <p>for</p><p>oxyzba</p>
-	 *		     "xyz"
+	 * * The above has to be reflected during {@link treeModel.operation.transform operational transformation}. Normal
+	 * behavior is that inserting inside range changed by {@link treeModel.operation.AttributeOperation} splits the operation
+	 * into two operations, which "omit" the inserted nodes. The correct behavior for `WeakInsertDelta` is that
+	 * {@link treeModel.operation.AttributeOperation} does not "break" and also applies attributes for inserted nodes.
 	 *
 	 * @chainable
 	 * @memberOf treeModel.Batch
-	 * @method insertWeak
+	 * @method weakInsert
 	 * @param {treeModel.Position} position Position of insertion.
 	 * @param {treeModel.Node|treeModel.Text|treeModel.NodeList|String|Iterable} nodes The list of nodes to be inserted.
 	 * List of nodes can be of any type accepted by the {@link treeModel.NodeList} constructor.
 	 */
-	register( 'insertWeak', function( position, nodes ) {
-		const delta = new InsertWeakDelta();
+	register( 'weakInsert', function( position, nodes ) {
+		const delta = new WeakInsertDelta();
 
 		nodes = new NodeList( nodes );
 
@@ -58,5 +55,5 @@ CKEDITOR.define( [
 		return this;
 	} );
 
-	return InsertWeakDelta;
+	return WeakInsertDelta;
 } );

+ 28 - 0
packages/ckeditor5-engine/tests/_tools/tools.js

@@ -67,4 +67,32 @@
 			return count;
 		}
 	};
+
+	bender.tools.treemodel = {
+		/**
+		 * Returns tree structure as a simplified string. Elements are uppercase and characters are lowercase.
+		 * Start and end of an element is marked the same way, by the element's name (in uppercase).
+		 *
+		 *		let element = new Element( 'div', [], [ 'abc', new Element( 'p', [], 'foo' ), 'xyz' ] );
+		 *		bender.tools.treemodel.getNodesAndText( element ); // abcPfooPxyz
+		 *
+		 * @param {treeModel.Range} range Range to stringify.
+		 * @returns {String} String representing element inner structure.
+		 */
+		getNodesAndText: ( range ) => {
+			let txt = '';
+
+			for ( let step of range ) {
+				let node = step.node;
+
+				if ( node.character ) {
+					txt += node.character.toLowerCase();
+				} else if ( node.name ) {
+					txt += node.name.toUpperCase();
+				}
+			}
+
+			return txt;
+		}
+	};
 } )();

+ 20 - 34
packages/ckeditor5-engine/tests/treemodel/delta/movedelta.js

@@ -4,26 +4,31 @@
  */
 
 /* bender-tags: treemodel, delta */
+/* bender-include: ../../_tools/tools.js */
 
 'use strict';
 
+const getNodesAndText = bender.tools.treemodel.getNodesAndText;
+
 const modules = bender.amd.require(
 	'treemodel/document',
 	'treemodel/position',
 	'treemodel/range',
-	'treemodel/element'
+	'treemodel/element',
+	'ckeditorerror'
 );
 
 describe( 'Batch', () => {
-	let Document, Position, Range, Element;
+	let Document, Position, Range, Element, CKEditorError;
 
-	let doc, root, div, p, batch, chain, range;
+	let doc, root, div, p, batch, chain;
 
 	before( () => {
 		Document = modules[ 'treemodel/document' ];
 		Position = modules[ 'treemodel/position' ];
 		Range = modules[ 'treemodel/range' ];
 		Element = modules[ 'treemodel/element' ];
+		CKEditorError = modules.ckeditorerror;
 	} );
 
 	beforeEach( () => {
@@ -39,54 +44,35 @@ describe( 'Batch', () => {
 		root.insertChildren( 0, [ div, p ] );
 
 		batch = doc.batch();
-
-		// Range starts in ROOT > DIV > P > gg|gg.
-		// Range ends in ROOT > DIV > ...|ar.
-		range = new Range( new Position( root, [ 0, 2, 2 ] ), new Position( root, [ 0, 6 ] ) );
 	} );
 
-	function getNodesAndText( element ) {
-		let range = Range.createFromElement( element );
-		let txt = '';
-
-		for ( let step of range ) {
-			let node = step.node;
-
-			if ( node.character ) {
-				txt += node.character.toLowerCase();
-			} else if ( node.name ) {
-				txt += node.name.toUpperCase();
-			}
-		}
-
-		return txt;
-	}
-
 	describe( 'move', () => {
 		it( 'should move specified node', () => {
 			batch.move( div, new Position( root, [ 2 ] ) );
 
 			expect( root.getChildCount() ).to.equal( 2 );
-			expect( getNodesAndText( root.getChild( 0 ) ) ).to.equal( 'abcxyz' );
-			expect( getNodesAndText( root.getChild( 1 ) ) ).to.equal( 'foPhhhhPobPggggPar' );
+			expect( getNodesAndText( Range.createFromElement( root.getChild( 0 ) ) ) ).to.equal( 'abcxyz' );
+			expect( getNodesAndText( Range.createFromElement( root.getChild( 1 ) ) ) ).to.equal( 'foPhhhhPobPggggPar' );
 		} );
 
-		it( 'should move any range of nodes', () => {
+		it( 'should move flat range of nodes', () => {
+			let range = new Range( new Position( root, [ 0, 3 ] ), new Position( root, [ 0, 7 ] ) );
 			batch.move( range, new Position( root, [ 1, 3 ] ) );
 
-			expect( getNodesAndText( root.getChild( 0 ) ) ).to.equal( 'foPhhPar' );
-			expect( getNodesAndText( root.getChild( 1 ) ) ).to.equal( 'abchhobPggggPxyz' );
+			expect( getNodesAndText( Range.createFromElement( root.getChild( 0 ) ) ) ).to.equal( 'foPhhhhPr' );
+			expect( getNodesAndText( Range.createFromElement( root.getChild( 1 ) ) ) ).to.equal( 'abcobPggggPaxyz' );
 		} );
 
-		it( 'should create minimal number of operations when moving a range', () => {
-			batch.move( range, new Position( root, [ 1, 3 ] ) );
+		it( 'should throw if given range is not flat', () => {
+			let notFlatRange = new Range( new Position( root, [ 0, 2, 2 ] ), new Position( root, [ 0, 6 ] ) );
 
-			expect( batch.deltas.length ).to.equal( 1 );
-			expect( batch.deltas[ 0 ].operations.length ).to.equal( 2 );
+			expect( () => {
+				doc.batch().move( notFlatRange, new Position( root, [ 1, 3 ] ) );
+			} ).to.throw( CKEditorError, /^batch-move-range-not-flat/ );
 		} );
 
 		it( 'should be chainable', () => {
-			chain = batch.move( range, new Position( root, [ 1, 3 ] ) );
+			chain = batch.move( div, new Position( root, [ 1, 3 ] ) );
 
 			expect( chain ).to.equal( batch );
 		} );

+ 6 - 20
packages/ckeditor5-engine/tests/treemodel/delta/removedelta.js

@@ -4,9 +4,12 @@
  */
 
 /* bender-tags: treemodel, delta */
+/* bender-include: ../../_tools/tools.js */
 
 'use strict';
 
+const getNodesAndText = bender.tools.treemodel.getNodesAndText;
+
 const modules = bender.amd.require(
 	'treemodel/document',
 	'treemodel/position',
@@ -45,36 +48,19 @@ describe( 'Batch', () => {
 		range = new Range( new Position( root, [ 0, 2, 2 ] ), new Position( root, [ 0, 6 ] ) );
 	} );
 
-	function getNodesAndText( element ) {
-		let range = Range.createFromElement( element );
-		let txt = '';
-
-		for ( let step of range ) {
-			let node = step.node;
-
-			if ( node.character ) {
-				txt += node.character.toLowerCase();
-			} else if ( node.name ) {
-				txt += node.name.toUpperCase();
-			}
-		}
-
-		return txt;
-	}
-
 	describe( 'remove', () => {
 		it( 'should remove specified node', () => {
 			batch.remove( div );
 
 			expect( root.getChildCount() ).to.equal( 1 );
-			expect( getNodesAndText( root.getChild( 0 ) ) ).to.equal( 'abcxyz' );
+			expect( getNodesAndText( Range.createFromElement( root.getChild( 0 ) ) ) ).to.equal( 'abcxyz' );
 		} );
 
 		it( 'should move any range of nodes', () => {
 			batch.remove( range );
 
-			expect( getNodesAndText( root.getChild( 0 ) ) ).to.equal( 'foPhhPar' );
-			expect( getNodesAndText( root.getChild( 1 ) ) ).to.equal( 'abcxyz' );
+			expect( getNodesAndText( Range.createFromElement( root.getChild( 0 ) ) ) ).to.equal( 'foPhhPar' );
+			expect( getNodesAndText( Range.createFromElement( root.getChild( 1 ) ) ) ).to.equal( 'abcxyz' );
 		} );
 
 		it( 'should create minimal number of operations when removing a range', () => {

+ 1 - 1
packages/ckeditor5-engine/tests/treemodel/delta/insertweakdelta.js → packages/ckeditor5-engine/tests/treemodel/delta/weakinsertdelta.js

@@ -39,7 +39,7 @@ describe( 'Batch', () => {
 
 		doc.selection.setAttrsTo( attrs );
 
-		chain = batch.insertWeak( new Position( root, [ 2 ] ), 'xyz' );
+		chain = batch.weakInsert( new Position( root, [ 2 ] ), 'xyz' );
 	} );
 
 	describe( 'insert', () => {