T/389 ModelRange#getTransformedByMove and other

src/treemodel/document.js

@@ -162,7 +162,11 @@ export default class Document {
 		this.history.addOperation( operation );
 
 		const batch = operation.delta && operation.delta.batch;
-		this.fire( 'change', operation.type, changes, batch );
+
+		if ( changes ) {
+			// `NoOperation` returns no changes, do not fire event for it.
+			this.fire( 'change', operation.type, changes, batch );
+		}
 	}
 
 	/**
@@ -307,16 +311,15 @@ export default class Document {
 	 * * 'removeRootAttribute' when attribute for root is removed,
 	 * * 'changeRootAttribute' when attribute for root changes.
 	 *
-	 * Change event is fired after the change is done. This means that any ranges or positions passed in
-	 * `data` are referencing nodes and paths in updated tree model.
-	 *
 	 * @event engine.treeModel.Document.change
 	 * @param {String} type Change type, possible option: 'insert', 'remove', 'reinsert', 'move', 'attribute'.
 	 * @param {Object} data Additional information about the change.
-	 * @param {engine.treeModel.Range} data.range Range containing changed nodes. Note that for 'remove' the range will be in the
-	 * {@link engine.treeModel.Document#graveyard graveyard root}. This is undefined for root types.
+	 * @param {engine.treeModel.Range} data.range Range in model containing changed nodes. Note that the range state is
+	 * after changes has been done, i.e. for 'remove' the range will be in the {@link engine.treeModel.Document#graveyard graveyard root}.
+	 * This is `undefined` for "...root..." types.
 	 * @param {engine.treeModel.Position} [data.sourcePosition] Change source position. Exists for 'remove', 'reinsert' and 'move'.
-	 * Note that for 'reinsert' the source position will be in the {@link engine.treeModel.Document#graveyard graveyard root}.
+	 * Note that this position state is before changes has been done, i.e. for 'reinsert' the source position will be in the
+	 * {@link engine.treeModel.Document#graveyard graveyard root}.
 	 * @param {String} [data.key] Only for attribute types. Key of changed / inserted / removed attribute.
 	 * @param {*} [data.oldValue] Only for 'removeAttribute', 'removeRootAttribute', 'changeAttribute' or
 	 * 'changeRootAttribute' type.

src/treemodel/history.js

@@ -6,9 +6,8 @@
 'use strict';
 
 // Load all basic deltas and transformations, they register themselves, but they need to be imported somewhere.
-import deltas from './delta/basic-deltas.js';
-import transformations from './delta/basic-transformations.js';
-/*jshint unused: false*/
+import deltas from './delta/basic-deltas.js'; // jshint ignore:line
+import transformations from './delta/basic-transformations.js'; // jshint ignore:line
 
 import transform from './delta/transform.js';
 import CKEditorError from '../../utils/ckeditorerror.js';
@@ -89,16 +88,9 @@ export default class History {
 			return [ delta ];
 		}
 
-		let index = this._historyPoints.get( delta.baseVersion );
-
-		if ( index === undefined ) {
-			throw new CKEditorError( 'history-wrong-version: Cannot retrieve point in history that is a base for given delta.' );
-		}
-
 		let transformed = [ delta ];
 
-		while ( index < this._deltas.length ) {
-			const historyDelta = this._deltas[ index ];
+		for ( let historyDelta of this.getDeltas( delta.baseVersion ) ) {
 			let allResults = [];
 
 			for ( let deltaToTransform of transformed ) {
@@ -107,18 +99,36 @@ export default class History {
 			}
 
 			transformed = allResults;
-			index++;
 		}
 
 		return transformed;
 	}
 
+	/**
+	 * Returns all deltas from history, starting from given history point (if passed).
+	 *
+	 * @param {Number} from History point.
+	 * @returns {Iterator.<engine.treeModel.delta.Delta>} Deltas from given history point to the end of history.
+	 */
+	*getDeltas( from = 0 ) {
+		let i = this._historyPoints.get( from );
+
+		if ( i === undefined ) {
+			throw new CKEditorError( 'history-wrong-version: Cannot retrieve given point in the history.' );
+		}
+
+		for ( ; i < this._deltas.length; i++ ) {
+			yield this._deltas[ i ];
+		}
+	}
+
 	/**
 	 * Transforms given delta by another given delta. Exposed for testing purposes.
 	 *
 	 * @protected
 	 * @param {engine.treeModel.delta.Delta} toTransform Delta to be transformed.
 	 * @param {engine.treeModel.delta.Delta} transformBy Delta to transform by.
+	 * @returns {Array.<engine.treeModel.delta.Delta>} Result of the transformation.
 	 */
 	static _transform( toTransform, transformBy ) {
 		return transform( toTransform, transformBy, false );

src/treemodel/liverange.js

@@ -6,17 +6,14 @@
 'use strict';
 
 import Range from './range.js';
-import LivePosition from './liveposition.js';
 import EmitterMixin from '../../utils/emittermixin.js';
 import utils from '../../utils/utils.js';
 
 /**
  * LiveRange is a Range in the Tree Model that updates itself as the tree changes. It may be used as a bookmark.
  *
- * **Note:** Constructor creates it's own {@link engine.treeModel.LivePosition} instances basing on passed values.
- *
- * **Note:** Be very careful when dealing with LiveRange. Each LiveRange instance bind events that might
- * have to be unbound. Use {@link engine.treeModel.LiveRange#detach detach} whenever you don't need LiveRange anymore.
+ * **Note:** Be very careful when dealing with `LiveRange`. Each `LiveRange` instance bind events that might
+ * have to be unbound. Use {@link engine.treeModel.LiveRange#detach detach} whenever you don't need `LiveRange` anymore.
  *
  * @memberOf engine.treeModel
  */
@@ -29,9 +26,6 @@ export default class LiveRange extends Range {
 	constructor( start, end ) {
 		super( start, end );
 
-		this.start = new LivePosition( this.start.root, this.start.path.slice(), 'STICKS_TO_NEXT' );
-		this.end = new LivePosition( this.end.root, this.end.path.slice(), 'STICKS_TO_PREVIOUS' );
-
 		bindWithDocument.call( this );
 	}
 
@@ -41,8 +35,6 @@ export default class LiveRange extends Range {
 	 * referring to it).
 	 */
 	detach() {
-		this.start.detach();
-		this.end.detach();
 		this.stopListening();
 	}
 
@@ -117,23 +109,50 @@ function bindWithDocument() {
  */
 function fixBoundaries( type, range, position ) {
 	/* jshint validthis: true */
+	let updated;
+	const howMany = range.end.offset - range.start.offset;
+
+	switch ( type ) {
+		case 'insert':
+			updated = this.getTransformedByInsertion( range.start, howMany )[ 0 ];
+			break;
+
+		case 'move':
+		case 'remove':
+		case 'reinsert':
+			const sourcePosition = position;
+
+			// Range.getTransformedByMove is expecting `targetPosition` to be "before" move
+			// (before transformation). `range.start` is already after the move happened.
+			// We have to revert `range.start` to the state before the move.
+			const targetPosition = range.start.getTransformedByInsertion( sourcePosition, howMany );
+
+			const result = this.getTransformedByMove( sourcePosition, targetPosition, howMany );
+
+			// First item in the array is the "difference" part, so a part of the range
+			// that did not get moved. We use it as reference range and expand if possible.
+			updated = result[ 0 ];
+
+			// We will check if there is other range and if it is touching the reference range.
+			// If it does, we will expand the reference range (at the beginning or at the end).
+			// Keep in mind that without settings `spread` flag, `getTransformedByMove` may
+			// return maximum two ranges.
+			if ( result.length > 1 ) {
+				let otherRange = result[ 1 ];
+
+				if ( updated.start.isTouching( otherRange.end ) ) {
+					updated.start = otherRange.start;
+				} else if ( updated.end.isTouching( otherRange.start ) ) {
+					updated.end = otherRange.end;
+				}
+			}
+
+			break;
+	}
 
-	if ( type == 'move' || type == 'remove' || type == 'reinsert' ) {
-		let containsStart = range.containsPosition( this.start ) || range.start.isEqual( this.start );
-		let containsEnd = range.containsPosition( this.end ) || range.end.isEqual( this.end );
-		position = position.getTransformedByInsertion( range.start, range.end.offset - range.start.offset, true );
-
-		// If the range contains both start and end, don't do anything - LivePositions that are boundaries of
-		// this LiveRange are in correct places, they got correctly transformed.
-		if ( containsStart && !containsEnd && !range.end.isTouching( position ) ) {
-			this.start.path = position.path.slice();
-			this.start.root = position.root;
-		}
-
-		if ( containsEnd && !containsStart && !range.start.isTouching( position ) ) {
-			this.end.path = position.path.slice();
-			this.end.root = position.root;
-		}
+	if ( updated ) {
+		this.start = updated.start;
+		this.end = updated.end;
 	}
 }
 

src/treemodel/operation/moveoperation.js

@@ -50,13 +50,29 @@ export default class MoveOperation extends Operation {
 		 */
 		this.targetPosition = Position.createFromPosition( targetPosition );
 
+		/**
+		 * Position of the start of the moved range after it got moved. This may be different than
+		 * {@link engine.treeModel.operation.MoveOperation#targetPosition} in some cases, i.e. when a range is moved
+		 * inside the same parent but {@link engine.treeModel.operation.MoveOperation#targetPosition targetPosition}
+		 * is after {@link engine.treeModel.operation.MoveOperation#sourcePosition sourcePosition}.
+		 *
+		 *		 vv              vv
+		 *		abcdefg ===> adefbcg
+		 *		     ^          ^
+		 *		     targetPos	movedRangeStart
+		 *		     offset 6	offset 4
+		 *
+		 * @member {engine.treeModel.Position} engine.treeModel.operation.MoveOperation#movedRangeStart
+		 */
+		this.movedRangeStart = this.targetPosition.getTransformedByDeletion( this.sourcePosition, this.howMany );
+
 		/**
 		 * Defines whether `MoveOperation` is sticky. If `MoveOperation` is sticky, during
 		 * {@link engine.treeModel.operation.transform operational transformation} if there will be an operation that
 		 * inserts some nodes at the position equal to the boundary of this `MoveOperation`, that operation will
 		 * get their insertion path updated to the position where this `MoveOperation` moves the range.
 		 *
-		 * @type {Boolean}
+		 * @member {Boolean} engine.treeModel.operation.MoveOperation#isSticky
 		 */
 		this.isSticky = false;
 	}
@@ -79,10 +95,9 @@ export default class MoveOperation extends Operation {
 	 * @returns {engine.treeModel.operation.MoveOperation}
 	 */
 	getReversed() {
-		let newSourcePosition = this.targetPosition.getTransformedByDeletion( this.sourcePosition, this.howMany );
 		let newTargetPosition = this.sourcePosition.getTransformedByInsertion( this.targetPosition, this.howMany );
 
-		const op = new this.constructor( newSourcePosition, this.howMany, newTargetPosition, this.baseVersion + 1 );
+		const op = new this.constructor( this.movedRangeStart, this.howMany, newTargetPosition, this.baseVersion + 1 );
 		op.isSticky = this.isSticky;
 
 		return op;
@@ -154,7 +169,7 @@ export default class MoveOperation extends Operation {
 
 		return {
 			sourcePosition: this.sourcePosition,
-			range: Range.createFromPositionAndShift( this.targetPosition, this.howMany )
+			range: Range.createFromPositionAndShift( this.movedRangeStart, this.howMany )
 		};
 	}
 }

src/treemodel/operation/transform.js

@@ -80,10 +80,9 @@ const ot = {
 		// than `b` when it comes to resolving conflicts. Returns results as an array of operations.
 		MoveOperation( a, b, isStrong ) {
 			const transformed = a.clone();
-			const moveTargetPosition = b.targetPosition.getTransformedByDeletion( b.sourcePosition, b.howMany );
 
 			// Transform insert position by the other operation parameters.
-			transformed.position = a.position.getTransformedByMove( b.sourcePosition, moveTargetPosition, b.howMany, !isStrong, b.isSticky );
+			transformed.position = a.position.getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !isStrong, b.isSticky );
 
 			return [ transformed ];
 		}
@@ -144,9 +143,6 @@ const ot = {
 			// Convert MoveOperation properties into a range.
 			const rangeB = Range.createFromPositionAndShift( b.sourcePosition, b.howMany );
 
-			// Get target position from the state "after" nodes specified by MoveOperation are "detached".
-			const newTargetPosition = b.targetPosition.getTransformedByDeletion( b.sourcePosition, b.howMany );
-
 			// This will aggregate transformed ranges.
 			let ranges = [];
 
@@ -171,15 +167,15 @@ const ot = {
 				// previously transformed target position.
 				// Note that we do not use Position.getTransformedByMove on range boundaries because we need to
 				// transform by insertion a range as a whole, since newTargetPosition might be inside that range.
-				ranges = difference.getTransformedByInsertion( newTargetPosition, b.howMany, true, false ).reverse();
+				ranges = difference.getTransformedByInsertion( b.movedRangeStart, b.howMany, true, false ).reverse();
 			}
 
 			if ( common !== null ) {
 				// Here we do not need to worry that newTargetPosition is inside moved range, because that
 				// would mean that the MoveOperation targets into itself, and that is incorrect operation.
 				// Instead, we calculate the new position of that part of original range.
-				common.start = common.start._getCombined( b.sourcePosition, newTargetPosition );
-				common.end = common.end._getCombined( b.sourcePosition, newTargetPosition );
+				common.start = common.start._getCombined( b.sourcePosition, b.movedRangeStart );
+				common.end = common.end._getCombined( b.sourcePosition, b.movedRangeStart );
 
 				ranges.push( common );
 			}
@@ -257,18 +253,15 @@ const ot = {
 			const rangeA = Range.createFromPositionAndShift( a.sourcePosition, a.howMany );
 			const rangeB = Range.createFromPositionAndShift( b.sourcePosition, b.howMany );
 
-			// Get target position from the state "after" nodes specified by other MoveOperation are "detached".
-			const moveTargetPosition = b.targetPosition.getTransformedByDeletion( b.sourcePosition, b.howMany );
-
 			// This will aggregate transformed ranges.
 			let ranges = [];
 
 			// All the other non-special cases are treated by generic algorithm below.
 			let difference = joinRanges( rangeA.getDifference( rangeB ) );
 
 			if ( difference ) {
-				difference.start = difference.start.getTransformedByMove( b.sourcePosition, moveTargetPosition, b.howMany, !a.isSticky, false );
-				difference.end = difference.end.getTransformedByMove( b.sourcePosition, moveTargetPosition, b.howMany, a.isSticky, false );
+				difference.start = difference.start.getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !a.isSticky, false );
+				difference.end = difference.end.getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, a.isSticky, false );
 
 				ranges.push( difference );
 			}
@@ -301,8 +294,8 @@ const ot = {
 				// Here we do not need to worry that newTargetPosition is inside moved range, because that
 				// would mean that the MoveOperation targets into itself, and that is incorrect operation.
 				// Instead, we calculate the new position of that part of original range.
-				common.start = common.start._getCombined( b.sourcePosition, moveTargetPosition );
-				common.end = common.end._getCombined( b.sourcePosition, moveTargetPosition );
+				common.start = common.start._getCombined( b.sourcePosition, b.movedRangeStart );
+				common.end = common.end._getCombined( b.sourcePosition, b.movedRangeStart );
 
 				// We have to take care of proper range order.
 				if ( difference && difference.start.isBefore( common.start ) ) {
@@ -319,7 +312,7 @@ const ot = {
 			}
 
 			// Target position also could be affected by the other MoveOperation. We will transform it.
-			let newTargetPosition = a.targetPosition.getTransformedByMove( b.sourcePosition, moveTargetPosition, b.howMany, !isStrong, b.isSticky );
+			let newTargetPosition = a.targetPosition.getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !isStrong, b.isSticky );
 
 			// Map transformed range(s) to operations and return them.
 			return ranges.reverse().map( ( range ) => {

src/treemodel/position.js

@@ -299,7 +299,10 @@ export default class Position {
 		// Moving a range removes nodes from their original position. We acknowledge this by proper transformation.
 		let transformed = this.getTransformedByDeletion( sourcePosition, howMany );
 
-		if ( transformed === null || ( transformed.isEqual( sourcePosition ) && sticky ) ) {
+		// Then we update target position, as it could be affected by nodes removal too.
+		targetPosition = targetPosition.getTransformedByDeletion( sourcePosition, howMany );
+
+		if ( transformed === null || ( sticky && transformed.isEqual( sourcePosition ) ) ) {
 			// This position is inside moved range (or sticks to it).
 			// In this case, we calculate a combination of this position, move source position and target position.
 			transformed = this._getCombined( sourcePosition, targetPosition );
@@ -387,13 +390,13 @@ export default class Position {
 				return true;
 
 			case 'BEFORE':
-				left = this;
-				right = otherPosition;
+				left = Position.createFromPosition( this );
+				right = Position.createFromPosition( otherPosition );
 				break;
 
 			case 'AFTER':
-				left = otherPosition;
-				right = this;
+				left = Position.createFromPosition( otherPosition );
+				right = Position.createFromPosition( this );
 				break;
 
 			default: