8
0
Просмотр исходного кода

Changed: Operations no longer remove changes object on `_execute()`.

Szymon Cofalik 8 лет назад
Родитель
Сommit
c44336d58a

+ 1 - 1
packages/ckeditor5-engine/src/model/document.js

@@ -307,7 +307,7 @@ export default class Document {
 	 *
 	 *
 	 * Execution of a feature may lead to an incorrect document tree state. The callbacks are used to fix document tree after
 	 * Execution of a feature may lead to an incorrect document tree state. The callbacks are used to fix document tree after
 	 * it has changed. Post-fixers are fired just after all changes from the outermost change block were applied but
 	 * it has changed. Post-fixers are fired just after all changes from the outermost change block were applied but
-	 * before {@link module:engine/model/model~Document#event:change} is fired. If a post-fixer callback made a change,
+	 * before {@link module:engine/model/document~Document#event:change} is fired. If a post-fixer callback made a change,
 	 * it should return `true`. When this happens, all post-fixers are fired again to check if something else should
 	 * it should return `true`. When this happens, all post-fixers are fired again to check if something else should
 	 * not be fixed in the new document tree state.
 	 * not be fixed in the new document tree state.
 	 *
 	 *

+ 13 - 12
packages/ckeditor5-engine/src/model/documentselection.js

@@ -110,23 +110,20 @@ export default class DocumentSelection extends Selection {
 				return;
 				return;
 			}
 			}
 
 
-			const type = operation.type;
-			const changes = evt.return;
-			const batch = operation.delta.batch;
-
 			// Whenever attribute operation is performed on document, update selection attributes.
 			// Whenever attribute operation is performed on document, update selection attributes.
 			// This is not the most efficient way to update selection attributes, but should be okay for now.
 			// This is not the most efficient way to update selection attributes, but should be okay for now.
-			if ( attrOpTypes.has( type ) ) {
+			if ( attrOpTypes.has( operation.type ) ) {
 				this._updateAttributes( false );
 				this._updateAttributes( false );
 			}
 			}
 
 
+			const batch = operation.delta.batch;
+
 			// Batch may not be passed to the document#change event in some tests.
 			// Batch may not be passed to the document#change event in some tests.
 			// See https://github.com/ckeditor/ckeditor5-engine/issues/1001#issuecomment-314202352
 			// See https://github.com/ckeditor/ckeditor5-engine/issues/1001#issuecomment-314202352
-			// Ignore also transparent batches because they are... transparent.
-			if ( batch && batch.type !== 'transparent' ) {
+			if ( batch ) {
 				// Whenever element which had selection's attributes stored in it stops being empty,
 				// Whenever element which had selection's attributes stored in it stops being empty,
 				// the attributes need to be removed.
 				// the attributes need to be removed.
-				clearAttributesStoredInElement( changes, this._model, batch );
+				clearAttributesStoredInElement( operation, this._model, batch );
 			}
 			}
 		}, { priority: 'low' } );
 		}, { priority: 'low' } );
 	}
 	}
@@ -754,11 +751,15 @@ function getAttrsIfCharacter( node ) {
 }
 }
 
 
 // Removes selection attributes from element which is not empty anymore.
 // Removes selection attributes from element which is not empty anymore.
-function clearAttributesStoredInElement( changes, model, batch ) {
-	const changeParent = changes.range && changes.range.start.parent;
+function clearAttributesStoredInElement( operation, model, batch ) {
+	let changeParent = null;
+
+	if ( operation.type == 'insert' ) {
+		changeParent = operation.position.parent;
+	} else if ( operation.type == 'move' || operation.type == 'reinsert' || operation.type == 'remove' ) {
+		changeParent = operation.getMovedRangeStart().parent;
+	}
 
 
-	// `changes.range` is not set in case of rename, root and marker operations.
-	// None of them may lead to the element becoming non-empty.
 	if ( !changeParent || changeParent.isEmpty ) {
 	if ( !changeParent || changeParent.isEmpty ) {
 		return;
 		return;
 	}
 	}

+ 15 - 10
packages/ckeditor5-engine/src/model/liveposition.js

@@ -149,11 +149,8 @@ function bindWithDocument() {
 				return;
 				return;
 			}
 			}
 
 
-			const type = operation.type;
-			const changes = event.return;
-
-			if ( supportedTypes.has( type ) ) {
-				transform.call( this, type, changes.range, changes.sourcePosition );
+			if ( supportedTypes.has( operation.type ) ) {
+				transform.call( this, operation );
 			}
 			}
 		},
 		},
 		{ priority: 'low' }
 		{ priority: 'low' }
@@ -166,16 +163,24 @@ function bindWithDocument() {
  * @ignore
  * @ignore
  * @private
  * @private
  * @method transform
  * @method transform
- * @param {String} type Type of changes applied to the Tree Model.
- * @param {module:engine/model/range~Range} range Range containing the result of applied change.
- * @param {module:engine/model/position~Position} [position] Additional position parameter provided by some change events.
+ * @param {module:engine/model/operation/operation~Operation} operation Executed operation.
  */
  */
-function transform( type, range, position ) {
+function transform( operation ) {
 	/* eslint-disable no-case-declarations */
 	/* eslint-disable no-case-declarations */
+	let range;
+	let position;
+
+	if ( operation.type == 'insert' ) {
+		range = Range.createFromPositionAndShift( operation.position, operation.nodes.maxOffset );
+	} else {
+		range = Range.createFromPositionAndShift( operation.getMovedRangeStart(), operation.howMany );
+		position = operation.sourcePosition;
+	}
+
 	const howMany = range.end.offset - range.start.offset;
 	const howMany = range.end.offset - range.start.offset;
 	let transformed;
 	let transformed;
 
 
-	switch ( type ) {
+	switch ( operation.type ) {
 		case 'insert':
 		case 'insert':
 			const insertBefore = this.stickiness == 'sticksToNext';
 			const insertBefore = this.stickiness == 'sticksToNext';
 			transformed = this._getTransformedByInsertion( range.start, howMany, insertBefore );
 			transformed = this._getTransformedByInsertion( range.start, howMany, insertBefore );

+ 18 - 14
packages/ckeditor5-engine/src/model/liverange.js

@@ -129,13 +129,8 @@ function bindWithDocument() {
 				return;
 				return;
 			}
 			}
 
 
-			const type = operation.type;
-			const changes = event.return;
-			const deltaType = operation.delta.type;
-			const batch = operation.delta.batch;
-
-			if ( supportedTypes.has( type ) ) {
-				transform.call( this, type, deltaType, batch, changes.range, changes.sourcePosition );
+			if ( supportedTypes.has( operation.type ) ) {
+				transform.call( this, operation );
 			}
 			}
 		},
 		},
 		{ priority: 'low' }
 		{ priority: 'low' }
@@ -148,13 +143,22 @@ function bindWithDocument() {
  * @ignore
  * @ignore
  * @private
  * @private
  * @method transform
  * @method transform
- * @param {String} [changeType] Type of change applied to the model document.
- * @param {String} [deltaType] Type of delta which introduced the change.
- * @param {module:engine/model/batch~Batch} batch Batch which changes the live range.
- * @param {module:engine/model/range~Range} targetRange Range containing the result of applied change.
- * @param {module:engine/model/position~Position} [sourcePosition] Source position for move, remove and reinsert change types.
+ * @param {module:engine/model/operation/operation~Operation} operation Executed operation.
  */
  */
-function transform( changeType, deltaType, batch, targetRange, sourcePosition ) {
+function transform( operation ) {
+	const changeType = operation.type;
+	const batch = operation.delta.batch;
+
+	let targetRange;
+	let sourcePosition;
+
+	if ( changeType == 'insert' ) {
+		targetRange = Range.createFromPositionAndShift( operation.position, operation.nodes.maxOffset );
+	} else {
+		targetRange = Range.createFromPositionAndShift( operation.getMovedRangeStart(), operation.howMany );
+		sourcePosition = operation.sourcePosition;
+	}
+
 	const howMany = targetRange.end.offset - targetRange.start.offset;
 	const howMany = targetRange.end.offset - targetRange.start.offset;
 	let targetPosition = targetRange.start;
 	let targetPosition = targetRange.start;
 
 
@@ -165,7 +169,7 @@ function transform( changeType, deltaType, batch, targetRange, sourcePosition )
 		targetPosition = targetPosition._getTransformedByInsertion( sourcePosition, howMany );
 		targetPosition = targetPosition._getTransformedByInsertion( sourcePosition, howMany );
 	}
 	}
 
 
-	const result = this._getTransformedByDocumentChange( changeType, deltaType, targetPosition, howMany, sourcePosition );
+	const result = this._getTransformedByDocumentChange( changeType, operation.delta.type, targetPosition, howMany, sourcePosition );
 
 
 	// Decide whether moved part should be included in the range.
 	// Decide whether moved part should be included in the range.
 	//
 	//

+ 1 - 3
packages/ckeditor5-engine/src/model/model.js

@@ -238,11 +238,9 @@ export default class Model {
 	 * {@link module:engine/model/operation/operation~Operation operations} on the model.
 	 * {@link module:engine/model/operation/operation~Operation operations} on the model.
 	 *
 	 *
 	 * @param {module:engine/model/operation/operation~Operation} operation Operation to apply
 	 * @param {module:engine/model/operation/operation~Operation} operation Operation to apply
-	 * @returns {Object} Object with additional information about the applied changes. It properties depends on the
-	 * operation type.
 	 */
 	 */
 	applyOperation( operation ) {
 	applyOperation( operation ) {
-		return operation._execute();
+		operation._execute();
 	}
 	}
 
 
 	/**
 	/**

+ 0 - 2
packages/ckeditor5-engine/src/model/operation/attributeoperation.js

@@ -157,8 +157,6 @@ export default class AttributeOperation extends Operation {
 			// Execution.
 			// Execution.
 			_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 };
 	}
 	}
 
 
 	/**
 	/**

+ 1 - 3
packages/ckeditor5-engine/src/model/operation/detachoperation.js

@@ -78,9 +78,7 @@ export default class DetachOperation extends Operation {
 	 * @inheritDoc
 	 * @inheritDoc
 	 */
 	 */
 	_execute() {
 	_execute() {
-		const nodes = _remove( Range.createFromPositionAndShift( this.sourcePosition, this.howMany ) );
-
-		return { nodes };
+		_remove( Range.createFromPositionAndShift( this.sourcePosition, this.howMany ) );
 	}
 	}
 
 
 	/**
 	/**

+ 1 - 3
packages/ckeditor5-engine/src/model/operation/insertoperation.js

@@ -113,9 +113,7 @@ export default class InsertOperation extends Operation {
 		const originalNodes = this.nodes;
 		const originalNodes = this.nodes;
 		this.nodes = new NodeList( [ ...originalNodes ].map( node => node.clone( true ) ) );
 		this.nodes = new NodeList( [ ...originalNodes ].map( node => node.clone( true ) ) );
 
 
-		const range = _insert( this.position, originalNodes );
-
-		return { range };
+		_insert( this.position, originalNodes );
 	}
 	}
 
 
 	/**
 	/**

+ 0 - 2
packages/ckeditor5-engine/src/model/operation/markeroperation.js

@@ -112,8 +112,6 @@ export default class MarkerOperation extends Operation {
 		const type = this.newRange ? 'set' : 'remove';
 		const type = this.newRange ? 'set' : 'remove';
 
 
 		this._markers[ type ]( this.name, this.newRange );
 		this._markers[ type ]( this.name, this.newRange );
-
-		return { name: this.name, type };
 	}
 	}
 
 
 	/**
 	/**

+ 1 - 6
packages/ckeditor5-engine/src/model/operation/moveoperation.js

@@ -189,12 +189,7 @@ export default class MoveOperation extends Operation {
 	 * @inheritDoc
 	 * @inheritDoc
 	 */
 	 */
 	_execute() {
 	_execute() {
-		const range = _move( Range.createFromPositionAndShift( this.sourcePosition, this.howMany ), this.targetPosition );
-
-		return {
-			sourcePosition: this.sourcePosition,
-			range
-		};
+		_move( Range.createFromPositionAndShift( this.sourcePosition, this.howMany ), this.targetPosition );
 	}
 	}
 
 
 	/**
 	/**

+ 0 - 4
packages/ckeditor5-engine/src/model/operation/nooperation.js

@@ -54,11 +54,7 @@ export default class NoOperation extends Operation {
 		return new NoOperation( this.baseVersion + 1 );
 		return new NoOperation( this.baseVersion + 1 );
 	}
 	}
 
 
-	/**
-	 * @inheritDoc
-	 */
 	_execute() {
 	_execute() {
-		return {};
 	}
 	}
 
 
 	/**
 	/**

+ 1 - 3
packages/ckeditor5-engine/src/model/operation/operation.js

@@ -73,12 +73,10 @@ export default class Operation {
 		 */
 		 */
 
 
 		/**
 		/**
-		 * Executes the operation - modifications described by the operation attributes will be applied to the tree model.
+		 * Executes the operation - modifications described by the operation properties will be applied to the model tree.
 		 *
 		 *
 		 * @protected
 		 * @protected
 		 * @method #_execute
 		 * @method #_execute
-		 * @returns {Object} Object with additional information about the applied changes. It properties depends on the
-		 * operation type.
 		 */
 		 */
 	}
 	}
 
 

+ 0 - 7
packages/ckeditor5-engine/src/model/operation/reinsertoperation.js

@@ -82,13 +82,6 @@ export default class ReinsertOperation extends MoveOperation {
 		}
 		}
 	}
 	}
 
 
-	/**
-	 * @inheritDoc
-	 */
-	_execute() {
-		return super._execute();
-	}
-
 	/**
 	/**
 	 * @inheritDoc
 	 * @inheritDoc
 	 */
 	 */

+ 0 - 7
packages/ckeditor5-engine/src/model/operation/removeoperation.js

@@ -67,13 +67,6 @@ export default class RemoveOperation extends MoveOperation {
 		}
 		}
 	}
 	}
 
 
-	/**
-	 * @inheritDoc
-	 */
-	_execute() {
-		return super._execute();
-	}
-
 	/**
 	/**
 	 * @inheritDoc
 	 * @inheritDoc
 	 */
 	 */

+ 0 - 2
packages/ckeditor5-engine/src/model/operation/renameoperation.js

@@ -117,8 +117,6 @@ export default class RenameOperation extends Operation {
 		const element = this.position.nodeAfter;
 		const element = this.position.nodeAfter;
 
 
 		element.name = this.newName;
 		element.name = this.newName;
-
-		return { element, oldName: this.oldName };
 	}
 	}
 
 
 	/**
 	/**

+ 0 - 2
packages/ckeditor5-engine/src/model/operation/rootattributeoperation.js

@@ -149,8 +149,6 @@ export default class RootAttributeOperation extends Operation {
 		} else {
 		} else {
 			this.root.removeAttribute( this.key );
 			this.root.removeAttribute( this.key );
 		}
 		}
-
-		return { root: this.root, key: this.key, oldValue: this.oldValue, newValue: this.newValue };
 	}
 	}
 
 
 	/**
 	/**

+ 0 - 14
packages/ckeditor5-engine/tests/model/documentselection.js

@@ -1128,20 +1128,6 @@ describe( 'DocumentSelection', () => {
 				expect( emptyP.parent ).to.equal( root ); // Just to be sure we're checking the right element.
 				expect( emptyP.parent ).to.equal( root ); // Just to be sure we're checking the right element.
 			} );
 			} );
 
 
-			it( 'are not removed on transparent batches', () => {
-				selection.setRanges( [ rangeInEmptyP ] );
-				selection.setAttribute( 'foo', 'bar' );
-
-				model.enqueueChange( 'transparent', writer => {
-					sinon.spy( model, 'enqueueChange' );
-
-					writer.insertText( 'x', rangeInEmptyP.start );
-
-					expect( model.enqueueChange.called ).to.be.false;
-					expect( emptyP.getAttribute( fooStoreAttrKey ) ).to.equal( 'bar' );
-				} );
-			} );
-
 			// Rename and some other deltas don't specify range in doc#change event.
 			// Rename and some other deltas don't specify range in doc#change event.
 			// So let's see if there's no crash or something.
 			// So let's see if there's no crash or something.
 			it( 'are not removed on rename', () => {
 			it( 'are not removed on rename', () => {

+ 2 - 5
packages/ckeditor5-engine/tests/model/model.js

@@ -324,18 +324,15 @@ describe( 'Model', () => {
 	} );
 	} );
 
 
 	describe( 'applyOperation()', () => {
 	describe( 'applyOperation()', () => {
-		it( 'should execute provided operation end return the result of operation', () => {
-			const returnValue = { foo: 'bar' };
-
+		it( 'should execute provided operation', () => {
 			const operation = {
 			const operation = {
-				_execute: sinon.stub().returns( returnValue ),
+				_execute: sinon.spy(),
 				_validate: () => true
 				_validate: () => true
 			};
 			};
 
 
 			model.applyOperation( operation );
 			model.applyOperation( operation );
 
 
 			sinon.assert.calledOnce( operation._execute );
 			sinon.assert.calledOnce( operation._execute );
-			expect( model.applyOperation( operation ) ).to.equal( returnValue );
 		} );
 		} );
 	} );
 	} );
 
 

+ 0 - 4
packages/ckeditor5-engine/tests/model/operation/nooperation.js

@@ -20,10 +20,6 @@ describe( 'NoOperation', () => {
 		expect( () => model.applyOperation( wrapInDelta( noop ) ) ).to.not.throw( Error );
 		expect( () => model.applyOperation( wrapInDelta( noop ) ) ).to.not.throw( Error );
 	} );
 	} );
 
 
-	it( 'should return empty object when executed', () => {
-		expect( noop._execute() ).to.deep.equal( {} );
-	} );
-
 	it( 'should create a NoOperation as a reverse', () => {
 	it( 'should create a NoOperation as a reverse', () => {
 		const reverse = noop.getReversed();
 		const reverse = noop.getReversed();