Merge pull request #1196 from ckeditor/t/858

Other: Refactoring: make writer a protected operations util.

Author: Piotr Jasiun

src/controller/datacontroller.js

@@ -295,10 +295,11 @@ export default class DataController {
 	 *
 	 * @fires module:engine/controller/datacontroller~DataController#getSelectedContent
 	 * @param {module:engine/model/selection~Selection} selection The selection of which content will be retrieved.
+	 * @param {module:engine/model/batch~Batch} batch Batch to which deltas will be added.
 	 * @returns {module:engine/model/documentfragment~DocumentFragment} Document fragment holding the clone of the selected content.
 	 */
-	getSelectedContent( selection ) {
-		return getSelectedContent( selection );
+	getSelectedContent( selection, batch ) {
+		return getSelectedContent( selection, batch );
 	}
 
 	/**

src/controller/getselectedcontent.js

@@ -7,11 +7,8 @@
  * @module engine/controller/getselectedcontent
  */
 
-import DocumentFragment from '../model/documentfragment';
 import Range from '../model/range';
 import Position from '../model/position';
-import Text from '../model/text';
-import { remove } from '../model/writer';
 
 /**
  * Gets a clone of the selected content.
@@ -25,10 +22,11 @@ import { remove } from '../model/writer';
  *		<quote><h>st</h></quote><p>se</p>
  *
  * @param {module:engine/model/selection~Selection} selection The selection of which content will be returned.
+ * @param {module:engine/model/batch~Batch} batch Batch to which deltas will be added.
  * @returns {module:engine/model/documentfragment~DocumentFragment}
  */
-export default function getSelectedContent( selection ) {
-	const frag = new DocumentFragment();
+export default function getSelectedContent( selection, batch ) {
+	const frag = batch.createDocumentFragment();
 	const range = selection.getFirstRange();
 
 	if ( !range || range.isCollapsed ) {
@@ -69,9 +67,9 @@ export default function getSelectedContent( selection ) {
 	// Clone the whole contents.
 	for ( const item of flatSubtreeRange.getItems( { shallow: true } ) ) {
 		if ( item.is( 'textProxy' ) ) {
-			frag.appendChildren( new Text( item.data, item.getAttributes() ) );
+			batch.appendText( item.data, item.getAttributes(), frag );
 		} else {
-			frag.appendChildren( item.clone( true ) );
+			batch.append( item.clone( true ), frag );
 		}
 	}
 
@@ -97,16 +95,16 @@ export default function getSelectedContent( selection ) {
 		const leftExcessRange = new Range( Position.createAt( frag ), newRange.start );
 		const rightExcessRange = new Range( newRange.end, Position.createAt( frag, 'end' ) );
 
-		removeRangeContent( rightExcessRange );
-		removeRangeContent( leftExcessRange );
+		removeRangeContent( rightExcessRange, batch );
+		removeRangeContent( leftExcessRange, batch );
 	}
 
 	return frag;
 }
 
 // After https://github.com/ckeditor/ckeditor5-engine/issues/690 is fixed,
 // this function will, most likely, be able to rewritten using getMinimalFlatRanges().
-function removeRangeContent( range ) {
+function removeRangeContent( range, batch ) {
 	const parentsToCheck = [];
 
 	Array.from( range.getItems( { direction: 'backward' } ) )
@@ -128,7 +126,7 @@ function removeRangeContent( range ) {
 		.forEach( itemRange => {
 			parentsToCheck.push( itemRange.start.parent );
 
-			remove( itemRange );
+			batch.remove( itemRange );
 		} );
 
 	// Remove ancestors of the removed items if they turned to be empty now
@@ -141,7 +139,7 @@ function removeRangeContent( range ) {
 
 			parent = parent.parent;
 
-			remove( removeRange );
+			batch.remove( removeRange );
 		}
 	} );
 }

src/dev-utils/model.js

@@ -20,7 +20,6 @@ import ModelSelection from '../model/selection';
 import ModelDocumentFragment from '../model/documentfragment';
 import ModelElement from '../model/element';
 import ModelText from '../model/text';
-import modelWriter from '../model/writer';
 
 import ViewConversionDispatcher from '../conversion/viewconversiondispatcher';
 import ViewSelection from '../view/selection';
@@ -324,9 +323,7 @@ export function parse( data, schema, batch, options = {} ) {
 
 function convertToModelFragment() {
 	return ( evt, data, consumable, conversionApi ) => {
-		const convertedChildren = conversionApi.convertChildren( data.input, consumable, data );
-
-		data.output = new ModelDocumentFragment( modelWriter.normalizeNodes( convertedChildren ) );
+		data.output = conversionApi.convertChildren( data.input, consumable, data );
 		conversionApi.mapper.bindElements( data.output, data.input );
 
 		evt.stop();

src/model/operation/attributeoperation.js

@@ -10,7 +10,7 @@
 import Operation from './operation';
 import Range from '../range';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
-import writer from '../writer';
+import { _setAttribute } from './utils';
 import isEqual from '@ckeditor/ckeditor5-utils/src/lib/lodash/isEqual';
 
 /**
@@ -151,7 +151,7 @@ export default class AttributeOperation extends Operation {
 		// If value to set is same as old value, don't do anything.
 		if ( !isEqual( this.oldValue, this.newValue ) ) {
 			// Execution.
-			writer.setAttribute( this.range, this.key, this.newValue );
+			_setAttribute( this.range, this.key, this.newValue );
 		}
 
 		return { range: this.range, key: this.key, oldValue: this.oldValue, newValue: this.newValue };

src/model/operation/detachoperation.js

@@ -10,7 +10,7 @@
 import Operation from './operation';
 import Position from '../position';
 import Range from '../range';
-import { remove } from '../writer';
+import { _remove } from './utils';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 /**
@@ -73,7 +73,7 @@ export default class DetachOperation extends Operation {
 			throw new CKEditorError( 'detach-operation-on-document-node: Cannot detach document node.' );
 		}
 
-		const nodes = remove( Range.createFromPositionAndShift( this.sourcePosition, this.howMany ) );
+		const nodes = _remove( Range.createFromPositionAndShift( this.sourcePosition, this.howMany ) );
 
 		return { nodes };
 	}

src/model/operation/insertoperation.js

@@ -11,7 +11,7 @@ import Operation from './operation';
 import Position from '../position';
 import NodeList from '../nodelist';
 import RemoveOperation from './removeoperation';
-import { insert, normalizeNodes } from '../writer';
+import { _insert, _normalizeNodes } from './utils';
 import Text from '../text';
 import Element from '../element';
 
@@ -45,7 +45,7 @@ export default class InsertOperation extends Operation {
 		 * @readonly
 		 * @member {module:engine/model/nodelist~NodeList} module:engine/model/operation/insertoperation~InsertOperation#nodeList
 		 */
-		this.nodes = new NodeList( normalizeNodes( nodes ) );
+		this.nodes = new NodeList( _normalizeNodes( nodes ) );
 
 		/**
 		 * @inheritDoc
@@ -94,7 +94,7 @@ export default class InsertOperation extends Operation {
 		const originalNodes = this.nodes;
 		this.nodes = new NodeList( [ ...originalNodes ].map( node => node.clone( true ) ) );
 
-		const range = insert( this.position, originalNodes );
+		const range = _insert( this.position, originalNodes );
 
 		return { range };
 	}

src/model/operation/moveoperation.js

@@ -12,7 +12,7 @@ import Position from '../position';
 import Range from '../range';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 import compareArrays from '@ckeditor/ckeditor5-utils/src/comparearrays';
-import writer from './../writer';
+import { _move } from './utils';
 
 /**
  * Operation to move a range of {@link module:engine/model/item~Item model items}
@@ -184,7 +184,7 @@ export default class MoveOperation extends Operation {
 			}
 		}
 
-		const range = writer.move( Range.createFromPositionAndShift( this.sourcePosition, this.howMany ), this.targetPosition );
+		const range = _move( Range.createFromPositionAndShift( this.sourcePosition, this.howMany ), this.targetPosition );
 
 		return {
 			sourcePosition: this.sourcePosition,

src/model/writer.js renamed to src/model/operation/utils.js

@@ -4,54 +4,36 @@
  */
 
 /**
- * @module engine/model/writer
+ * @module engine/model/operation/utils
  */
 
-import Node from './node';
-import Text from './text';
-import TextProxy from './textproxy';
-import Range from './range';
-import DocumentFragment from './documentfragment';
-import NodeList from './nodelist';
+import Node from '../node';
+import Text from '../text';
+import TextProxy from '../textproxy';
+import Range from '../range';
+import DocumentFragment from '../documentfragment';
+import NodeList from '../nodelist';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 /**
- * Contains functions used for composing model tree, grouped together under "model writer" name. Those functions
- * are built on top of {@link module:engine/model/node~Node node}, and it's child classes', APIs.
+ * Contains functions used for composing model tree by {@link module:engine/model/operation~Operation operations}.
+ * Those functions are built on top of {@link module:engine/model/node~Node node}, and it's child classes', APIs.
  *
- * Model writer API has multiple advantages and it is highly recommended to use it when changing model tree and nodes:
- * * model writer API {@link module:engine/model/writer~writer.normalizeNodes normalizes inserted nodes}, which means that you can insert
- * not only {@link module:engine/model/node~Node nodes}, but also `String`s, {@link module:engine/model/textproxy~TextProxy text proxies}
- * and
- * {@link module:engine/model/documentfragment~DocumentFragment document fragments},
- * * model writer API operates on {@link module:engine/model/position~Position positions}, which means that you have
- * better control over manipulating model tree as positions operate on offsets rather than indexes,
- * * model writer API automatically merges {@link module:engine/model/text~Text text nodes} with same attributes, which means
- * lower memory usage and better efficiency.
- *
- * @namespace writer
+ * @protected
+ * @namespace utils
  */
-const writer = {
-	insert,
-	remove,
-	move,
-	setAttribute,
-	removeAttribute,
-	normalizeNodes
-};
-
-export default writer;
 
 /**
  * Inserts given nodes at given position.
  *
- * @function module:engine/model/writer~writer.insert
+ * @protected
+ * @function module:engine/model/operation/utils~utils.insert
  * @param {module:engine/model/position~Position} position Position at which nodes should be inserted.
  * @param {module:engine/model/node~NodeSet} nodes Nodes to insert.
  * @returns {module:engine/model/range~Range} Range spanning over inserted elements.
  */
-export function insert( position, nodes ) {
-	nodes = normalizeNodes( nodes );
+export function _insert( position, nodes ) {
+	nodes = _normalizeNodes( nodes );
 
 	// We have to count offset before inserting nodes because they can get merged and we would get wrong offsets.
 	const offset = nodes.reduce( ( sum, node ) => sum + node.offsetSize, 0 );
@@ -75,18 +57,19 @@ export function insert( position, nodes ) {
 /**
  * Removed nodes in given range. Only {@link module:engine/model/range~Range#isFlat flat} ranges are accepted.
  *
- * @function module:engine/model/writer~writer.remove
+ * @protected
+ * @function module:engine/model/operation/utils~utils.remove
  * @param {module:engine/model/range~Range} range Range containing nodes to remove.
  * @returns {Array.<module:engine/model/node~Node>}
  */
-export function remove( range ) {
+export function _remove( range ) {
 	if ( !range.isFlat ) {
 		/**
 		 * Trying to remove a range which starts and ends in different element.
 		 *
-		 * @error model-writer-remove-range-not-flat
+		 * @error operation-utils-remove-range-not-flat
 		 */
-		throw new CKEditorError( 'model-writer-remove-range-not-flat: ' +
+		throw new CKEditorError( 'operation-utils-remove-range-not-flat: ' +
 			'Trying to remove a range which starts and ends in different element.' );
 	}
 
@@ -109,38 +92,42 @@ export function remove( range ) {
 /**
  * Moves nodes in given range to given target position. Only {@link module:engine/model/range~Range#isFlat flat} ranges are accepted.
  *
+ * @protected
+ * @function module:engine/model/operation/utils~utils.move
  * @param {module:engine/model/range~Range} sourceRange Range containing nodes to move.
  * @param {module:engine/model/position~Position} targetPosition Position to which nodes should be moved.
  * @returns {module:engine/model/range~Range} Range containing moved nodes.
  */
-export function move( sourceRange, targetPosition ) {
+export function _move( sourceRange, targetPosition ) {
 	if ( !sourceRange.isFlat ) {
 		/**
 		 * Trying to move a range which starts and ends in different element.
 		 *
-		 * @error model-writer-move-range-not-flat
+		 * @error operation-utils-move-range-not-flat
 		 */
-		throw new CKEditorError( 'model-writer-move-range-not-flat: ' +
+		throw new CKEditorError( 'operation-utils-move-range-not-flat: ' +
 			'Trying to move a range which starts and ends in different element.' );
 	}
 
-	const nodes = this.remove( sourceRange );
+	const nodes = _remove( sourceRange );
 
 	// We have to fix `targetPosition` because model changed after nodes from `sourceRange` got removed and
 	// that change might have an impact on `targetPosition`.
 	targetPosition = targetPosition._getTransformedByDeletion( sourceRange.start, sourceRange.end.offset - sourceRange.start.offset );
 
-	return this.insert( targetPosition, nodes );
+	return _insert( targetPosition, nodes );
 }
 
 /**
  * Sets given attribute on nodes in given range.
  *
+ * @protected
+ * @function module:engine/model/operation/utils~utils.setAttribute
  * @param {module:engine/model/range~Range} range Range containing nodes that should have the attribute set.
  * @param {String} key Key of attribute to set.
  * @param {*} value Attribute value.
  */
-export function setAttribute( range, key, value ) {
+export function _setAttribute( range, key, value ) {
 	// Range might start or end in text nodes, so we have to split them.
 	_splitNodeAtPosition( range.start );
 	_splitNodeAtPosition( range.end );
@@ -166,24 +153,16 @@ export function setAttribute( range, key, value ) {
 	_mergeNodesAtIndex( range.end.parent, range.end.index );
 }
 
-/**
- * Removes given attribute from nodes in given range.
- *
- * @param {module:engine/model/range~Range} range Range containing nodes that should have the attribute removed.
- * @param {String} key Key of attribute to remove.
- */
-export function removeAttribute( range, key ) {
-	this.setAttribute( range, key, null );
-}
-
 /**
  * Normalizes given object or an array of objects to an array of {@link module:engine/model/node~Node nodes}. See
  * {@link module:engine/model/node~NodeSet NodeSet} for details on how normalization is performed.
  *
+ * @protected
+ * @function module:engine/model/operation/utils~utils.normalizeNodes
  * @param {module:engine/model/node~NodeSet} nodes Objects to normalize.
  * @returns {Array.<module:engine/model/node~Node>} Normalized nodes.
  */
-export function normalizeNodes( nodes ) {
+export function _normalizeNodes( nodes ) {
 	const normalized = [];
 
 	if ( !( nodes instanceof Array ) ) {

tests/controller/datacontroller.js

@@ -433,7 +433,7 @@ describe( 'DataController', () => {
 
 			data.on( 'getSelectedContent', spy );
 
-			data.getSelectedContent( sel );
+			data.getSelectedContent( sel, modelDocument.batch() );
 
 			expect( spy.calledOnce ).to.be.true;
 		} );
@@ -443,7 +443,7 @@ describe( 'DataController', () => {
 
 			setData( modelDocument, '<paragraph>fo[ob]ar</paragraph>' );
 
-			const content = data.getSelectedContent( modelDocument.selection );
+			const content = data.getSelectedContent( modelDocument.selection, modelDocument.batch() );
 
 			expect( stringify( content ) ).to.equal( 'ob' );
 		} );