Merge pull request #1175 from ckeditor/t/897

Other: Make `Position` and `Range` immutable in model and view. Closes #897.

Author: scofalik

src/conversion/viewconversiondispatcher.js

@@ -303,10 +303,11 @@ function extractMarkersFromModelFragment( modelItem ) {
 
 		// When marker of given name is not stored it means that we have found the beginning of the range.
 		if ( !markers.has( markerName ) ) {
-			markers.set( markerName, new ModelRange( ModelPosition.createFromPosition( currentPosition ) ) );
+			markers.set( markerName, new ModelRange( currentPosition ) );
 		// Otherwise is means that we have found end of the marker range.
 		} else {
-			markers.get( markerName ).end = ModelPosition.createFromPosition( currentPosition );
+			const oldMarker = markers.get( markerName );
+			markers.set( markerName, new ModelRange( oldMarker.start, currentPosition ) );
 		}
 
 		// Remove marker element from DocumentFragment.

src/dev-utils/view.js

@@ -541,16 +541,15 @@ class RangeParser {
 				throw new Error( `Parse error - end of range was found '${ item.bracket }' but range was not started before.` );
 			}
 
-			// When second start of range is found when one is already opened - selection does not allow intersecting
-			// ranges.
+			// When second start of range is found when one is already opened - selection does not allow intersecting ranges.
 			if ( range && ( item.bracket == ELEMENT_RANGE_START_TOKEN || item.bracket == TEXT_RANGE_START_TOKEN ) ) {
 				throw new Error( `Parse error - start of range was found '${ item.bracket }' but one range is already started.` );
 			}
 
 			if ( item.bracket == ELEMENT_RANGE_START_TOKEN || item.bracket == TEXT_RANGE_START_TOKEN ) {
 				range = new Range( item.position, item.position );
 			} else {
-				range.end = item.position;
+				range = new Range( range.start, item.position );
 				ranges.push( range );
 				range = null;
 			}

src/model/delta/basic-transformations.js

@@ -73,8 +73,10 @@ addTransformationCase( AttributeDelta, SplitDelta, ( a, b, context ) => {
 			const additionalAttributeDelta = new AttributeDelta();
 
 			const rangeStart = splitPosition.getShiftedBy( 1 );
-			const rangeEnd = Position.createFromPosition( rangeStart );
-			rangeEnd.path.push( 0 );
+
+			const rangeEndPath = rangeStart.path.slice();
+			rangeEndPath.push( 0 );
+			const rangeEnd = new Position( rangeStart.root, rangeEndPath );
 
 			const oldValue = b._cloneOperation.nodes.getNode( 0 ).getAttribute( operation.key );
 
@@ -236,7 +238,7 @@ addTransformationCase( SplitDelta, SplitDelta, ( a, b, context ) => {
 				a._cloneOperation instanceof ReinsertOperation && b._cloneOperation instanceof ReinsertOperation &&
 				a._cloneOperation.sourcePosition.offset > b._cloneOperation.sourcePosition.offset
 			) {
-				a._cloneOperation.sourcePosition.offset--;
+				a._cloneOperation.sourcePosition = a._cloneOperation.sourcePosition.getShiftedBy( -1 );
 			}
 
 			// `a` splits closer or at same offset.
@@ -317,29 +319,33 @@ addTransformationCase( SplitDelta, WrapDelta, ( a, b, context ) => {
 		// Wrapping element is the element inserted by WrapDelta (re)insert operation.
 		// It is inserted after the wrapped range, but the wrapped range will be moved inside it.
 		// Having this in mind, it is correct to use wrapped range start position as the position before wrapping element.
-		const splitNodePos = Position.createFromPosition( b.range.start );
+
 		// Now, `splitNodePos` points before wrapping element.
 		// To get a position before last children of that element, we expand position's `path` member by proper offset.
-		splitNodePos.path.push( b.howMany - 1 );
+		const splitPath = b.range.start.path.slice();
+		splitPath.push( b.howMany - 1 );
+
+		const splitNodePos = new Position( b.range.start.root, splitPath );
 
 		// SplitDelta insert operation position should be right after the node we split.
-		const insertPos = splitNodePos.getShiftedBy( 1 );
-		delta._cloneOperation.position = insertPos;
+		delta._cloneOperation.position = splitNodePos.getShiftedBy( 1 );
 
 		// 2. Fix move operation source position.
 		// Nodes moved by SplitDelta will be moved from new position, modified by WrapDelta.
 		// To obtain that new position, `splitNodePos` will be used, as this is the node we are extracting children from.
-		const sourcePos = Position.createFromPosition( splitNodePos );
 		// Nothing changed inside split node so it is correct to use the original split position offset.
-		sourcePos.path.push( a.position.offset );
-		delta._moveOperation.sourcePosition = sourcePos;
+		const sourcePath = splitNodePos.path.slice();
+		sourcePath.push( a.position.offset );
+
+		delta._moveOperation.sourcePosition = new Position( splitNodePos.root, sourcePath );
 
 		// 3. Fix move operation target position.
 		// SplitDelta move operation target position should be inside the node inserted by operation above.
 		// Since the node is empty, we will insert at offset 0.
-		const targetPos = Position.createFromPosition( insertPos );
-		targetPos.path.push( 0 );
-		delta._moveOperation.targetPosition = targetPos;
+		const targetPath = splitNodePos.getShiftedBy( 1 ).path.slice();
+		targetPath.push( 0 );
+
+		delta._moveOperation.targetPosition = new Position( splitNodePos.root, targetPath );
 
 		return [ delta ];
 	}
@@ -434,13 +440,17 @@ addTransformationCase( WrapDelta, SplitDelta, ( a, b, context ) => {
 		const delta = a.clone();
 
 		// Move wrapping element insert position one node further so it is after the split node insertion.
-		delta._insertOperation.position.offset++;
+		delta._insertOperation.position = delta._insertOperation.position.getShiftedBy( 1 );
 
 		// Include the split node copy.
 		delta._moveOperation.howMany++;
 
 		// Change the path to wrapping element in move operation.
-		delta._moveOperation.targetPosition.path[ delta._moveOperation.targetPosition.path.length - 2 ]++;
+		const index = delta._moveOperation.targetPosition.path.length - 2;
+
+		const path = delta._moveOperation.targetPosition.path.slice();
+		path[ index ] += 1;
+		delta._moveOperation.targetPosition = new Position( delta._moveOperation.targetPosition.root, path );
 
 		return [ delta ];
 	}

src/model/documentselection.js

@@ -7,7 +7,6 @@
  * @module engine/model/documentselection
  */
 
-import Position from './position';
 import Range from './range';
 import LiveRange from './liverange';
 import Text from './text';
@@ -666,10 +665,9 @@ export default class DocumentSelection extends Selection {
 	_fixGraveyardSelection( liveRange, removedRangeStart ) {
 		// The start of the removed range is the closest position to the `liveRange` - the original selection range.
 		// This is a good candidate for a fixed selection range.
-		const positionCandidate = Position.createFromPosition( removedRangeStart );
 
 		// Find a range that is a correct selection range and is closest to the start of removed range.
-		const selectionRange = this._document.getNearestSelectionRange( positionCandidate );
+		const selectionRange = this._document.getNearestSelectionRange( removedRangeStart );
 
 		// Remove the old selection range before preparing and adding new selection range. This order is important,
 		// because new range, in some cases, may intersect with old range (it depends on `getNearestSelectionRange()` result).

src/model/liveposition.js

@@ -194,8 +194,8 @@ function transform( type, range, position ) {
 	if ( !this.isEqual( transformed ) ) {
 		const oldPosition = Position.createFromPosition( this );
 
-		this.path = transformed.path;
-		this.root = transformed.root;
+		this._path = transformed.path;
+		this._root = transformed.root;
 
 		this.fire( 'change', oldPosition );
 	}

src/model/liverange.js

@@ -178,8 +178,8 @@ function transform( changeType, deltaType, batch, targetRange, sourcePosition )
 		// If range boundaries have changed, fire `change:range` event.
 		const oldRange = Range.createFromRange( this );
 
-		this.start = updated.start;
-		this.end = updated.end;
+		this._start = updated.start;
+		this._end = updated.end;
 
 		this.fire( 'change:range', oldRange, {
 			type: changeType,

src/model/operation/insertoperation.js

@@ -37,7 +37,7 @@ export default class InsertOperation extends Operation {
 		 * @readonly
 		 * @member {module:engine/model/position~Position} module:engine/model/operation/insertoperation~InsertOperation#position
 		 */
-		this.position = Position.createFromPosition( position );
+		this.position = position;
 
 		/**
 		 * List of nodes to insert.

src/model/operation/renameoperation.js

@@ -66,7 +66,7 @@ export default class RenameOperation extends Operation {
 	 * @returns {module:engine/model/operation/renameoperation~RenameOperation} Clone of this operation.
 	 */
 	clone() {
-		return new RenameOperation( Position.createFromPosition( this.position ), this.oldName, this.newName, this.baseVersion );
+		return new RenameOperation( this.position, this.oldName, this.newName, this.baseVersion );
 	}
 
 	/**
@@ -75,7 +75,7 @@ export default class RenameOperation extends Operation {
 	 * @returns {module:engine/model/operation/renameoperation~RenameOperation}
 	 */
 	getReversed() {
-		return new RenameOperation( Position.createFromPosition( this.position ), this.newName, this.oldName, this.baseVersion + 1 );
+		return new RenameOperation( this.position, this.newName, this.oldName, this.baseVersion + 1 );
 	}
 
 	/**

src/model/operation/transform.js

@@ -179,25 +179,29 @@ const ot = {
 				// Take the start and the end of the range and transform them by deletion of moved nodes.
 				// Note that if rangeB was inside AttributeOperation range, only difference.end will be transformed.
 				// This nicely covers the joining simplification we did in the previous step.
-				difference.start = difference.start._getTransformedByDeletion( b.sourcePosition, b.howMany );
-				difference.end = difference.end._getTransformedByDeletion( b.sourcePosition, b.howMany );
+				const differenceTransformed = new Range(
+					difference.start._getTransformedByDeletion( b.sourcePosition, b.howMany ),
+					difference.end._getTransformedByDeletion( b.sourcePosition, b.howMany )
+				);
 
 				// MoveOperation pastes nodes into target position. We acknowledge this by proper transformation.
 				// Note that since we operate on transformed difference range, we should transform by
 				// 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( b.getMovedRangeStart(), b.howMany, true, false ).reverse();
+				ranges = differenceTransformed._getTransformedByInsertion( b.getMovedRangeStart(), 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, b.getMovedRangeStart() );
-				common.end = common.end._getCombined( b.sourcePosition, b.getMovedRangeStart() );
+				const commonTransformed = new Range(
+					common.start._getCombined( b.sourcePosition, b.getMovedRangeStart() ),
+					common.end._getCombined( b.sourcePosition, b.getMovedRangeStart() )
+				);
 
-				ranges.push( common );
+				ranges.push( commonTransformed );
 			}
 
 			// Map transformed range(s) to operations and return them.
@@ -376,7 +380,7 @@ const ot = {
 			// Setting and evaluating some variables that will be used in special cases and default algorithm.
 			//
 			// Create ranges from `MoveOperations` properties.
-			const rangeA = Range.createFromPositionAndShift( a.sourcePosition, a.howMany );
+			let rangeA = Range.createFromPositionAndShift( a.sourcePosition, a.howMany );
 			const rangeB = Range.createFromPositionAndShift( b.sourcePosition, b.howMany );
 
 			// Assign `context.isStrong` to a different variable, because the value may change during execution of
@@ -428,8 +432,10 @@ const ot = {
 			if ( bTargetsToA && rangeA.containsRange( rangeB, true ) ) {
 				// There is a mini-special case here, where `rangeB` is on other level than `rangeA`. That's why
 				// we need to transform `a` operation anyway.
-				rangeA.start = rangeA.start._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !includeB );
-				rangeA.end = rangeA.end._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, includeB );
+				rangeA = new Range(
+					rangeA.start._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !includeB ),
+					rangeA.end._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, includeB )
+				);
 
 				return makeMoveOperationsFromRanges( [ rangeA ], newTargetPosition, a );
 			}
@@ -444,8 +450,10 @@ const ot = {
 			if ( aTargetsToB && rangeB.containsRange( rangeA, true ) ) {
 				// `a` operation is "moved together" with `b` operation.
 				// Here, just move `rangeA` "inside" `rangeB`.
-				rangeA.start = rangeA.start._getCombined( b.sourcePosition, b.getMovedRangeStart() );
-				rangeA.end = rangeA.end._getCombined( b.sourcePosition, b.getMovedRangeStart() );
+				rangeA = new Range(
+					rangeA.start._getCombined( b.sourcePosition, b.getMovedRangeStart() ),
+					rangeA.end._getCombined( b.sourcePosition, b.getMovedRangeStart() )
+				);
 
 				return makeMoveOperationsFromRanges( [ rangeA ], newTargetPosition, a );
 			}
@@ -466,8 +474,10 @@ const ot = {
 				// Transform `rangeA` by `b` operation and make operation out of it, and that's all.
 				// Note that this is a simplified version of default case, but here we treat the common part (whole `rangeA`)
 				// like a one difference part.
-				rangeA.start = rangeA.start._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !includeB );
-				rangeA.end = rangeA.end._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, includeB );
+				rangeA = new Range(
+					rangeA.start._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !includeB ),
+					rangeA.end._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, includeB )
+				);
 
 				return makeMoveOperationsFromRanges( [ rangeA ], newTargetPosition, a );
 			}
@@ -500,10 +510,12 @@ const ot = {
 			// This is an array with one or two ranges. Two ranges if `rangeB` is inside `rangeA`.
 			const difference = rangeA.getDifference( rangeB );
 
-			for ( const range of difference ) {
+			for ( const rangeInDiff of difference ) {
 				// Transform those ranges by `b` operation. For example if `b` moved range from before those ranges, fix those ranges.
-				range.start = range.start._getTransformedByDeletion( b.sourcePosition, b.howMany );
-				range.end = range.end._getTransformedByDeletion( b.sourcePosition, b.howMany );
+				const range = new Range(
+					rangeInDiff.start._getTransformedByDeletion( b.sourcePosition, b.howMany ),
+					rangeInDiff.end._getTransformedByDeletion( b.sourcePosition, b.howMany )
+				);
 
 				// If `b` operation targets into `rangeA` on the same level, spread `rangeA` into two ranges.
 				const shouldSpread = compareArrays( range.start.getParentPath(), b.getMovedRangeStart().getParentPath() ) == 'same';
@@ -513,12 +525,14 @@ const ot = {
 			}
 
 			// Then, we have to manage the "common part" of both move ranges.
-			const common = rangeA.getIntersection( rangeB );
+			const intersectionRange = rangeA.getIntersection( rangeB );
 
-			if ( common !== null && isStrong && !bTargetsToA ) {
+			if ( intersectionRange !== null && isStrong && !bTargetsToA ) {
 				// Calculate the new position of that part of original range.
-				common.start = common.start._getCombined( b.sourcePosition, b.getMovedRangeStart() );
-				common.end = common.end._getCombined( b.sourcePosition, b.getMovedRangeStart() );
+				const common = new Range(
+					intersectionRange.start._getCombined( b.sourcePosition, b.getMovedRangeStart() ),
+					intersectionRange.end._getCombined( b.sourcePosition, b.getMovedRangeStart() )
+				);
 
 				// Take care of proper range order.
 				//
@@ -627,9 +641,10 @@ function joinRanges( ranges ) {
 	} else if ( ranges.length == 1 ) {
 		return ranges[ 0 ];
 	} else {
-		ranges[ 0 ].end = ranges[ ranges.length - 1 ].end;
-
-		return ranges[ 0 ];
+		return new Range(
+			ranges[ 0 ].start,
+			ranges[ ranges.length - 1 ].end
+		);
 	}
 }