Kaynağa Gözat

Changed: rewritten undo.UndoCommand with new transformation algorithm and improved selection restoring algorithm.

Szymon Cofalik 9 yıl önce
ebeveyn
işleme
fd5969b0df

+ 234 - 156
packages/ckeditor5-undo/src/undocommand.js

@@ -5,207 +5,285 @@
 
 
 'use strict';
 'use strict';
 
 
-import Command from '../command/command.js';
+import BaseCommand from './basecommand.js';
+import { transformDelta as transformDelta } from './basecommand.js';
 
 
 /**
 /**
- * Undo command stores batches in itself and is able to and apply reverted versions of them on the document.
+ * Undo command stores {@link engine.model.Batch batches} applied to the {@link engine.model.Document document}
+ * and is able to undo a batch by reversing it and transforming by other batches from {@link engine.model.Document#history history}
+ * that happened after the reversed batch.
+ *
+ * Undo command also takes care of restoring {@link engine.model.Document#selection selection} to the state before the
+ * undone batch was applied.
  *
  *
  * @memberOf undo
  * @memberOf undo
  */
  */
-export default class UndoCommand extends Command {
-	constructor( editor ) {
-		super( editor );
-
-		/**
-		 * Items that are pairs of:
-		 *
-		 * * batches which are saved by the command and,
-		 * * model selection state at the moment of saving the batch.
-		 *
-		 * @private
-		 * @member {Array} undo.UndoCommand#_items
-		 */
-		this._items = [];
-	}
-
+export default class UndoCommand extends BaseCommand {
 	/**
 	/**
-	 * Stores a batch in the command. Stored batches can be then reverted.
+	 * Executes the command: reverts a {@link engine.model.Batch batch} added to the command's stack, transforms
+	 * and applies reverted version on the {@link engine.model.Document document} and removes the batch from the stack.
+	 * Then, restores {@link engine.model.Document#selection document selection}.
 	 *
 	 *
-	 * @param {engine.model.Batch} batch Batch to add.
+	 * @protected
+	 * @fires undo.undoCommand#event:revert
+	 * @param {engine.model.Batch} [batch] Batch that should be undone. If not set, the last added batch will be undone.
 	 */
 	 */
-	addBatch( batch ) {
-		const selection = {
-			ranges: Array.from( this.editor.document.selection.getRanges() ),
-			isBackward: this.editor.document.selection.isBackward
-		};
+	_doExecute( batch = null ) {
+		// If batch is not given, set `batchIndex` to the last index in command stack.
+		let batchIndex = batch ? this._items.findIndex( ( a ) => a.batch == batch ) : this._items.length - 1;
 
 
-		this._items.push( { batch, selection } );
-		this.refreshState();
-	}
+		const item = this._items.splice( batchIndex, 1 )[ 0 ];
 
 
-	/**
-	 * Removes all batches from the stack.
-	 */
-	clearStack() {
-		this._items = [];
+		// All changes done by the command execution will be saved as one batch.
+		const newBatch = this.editor.document.batch();
+		newBatch.type = 'undo';
+
+		// All changes has to be done in one `enqueueChanges` callback so other listeners will not
+		// step between consecutive deltas, or won't do changes to the document before selection is properly restored.
+		this.editor.document.enqueueChanges( () => {
+			this._undo( item.batch, newBatch, this.editor.document );
+			this._restoreSelection( item.selection.ranges, item.selection.isBackward, item.batch.baseVersion, this.editor.document );
+		} );
+
+		this.fire( 'revert', item.batch );
 		this.refreshState();
 		this.refreshState();
 	}
 	}
 
 
 	/**
 	/**
-	 * @inheritDoc
+	 * Returns index in {@link undo.BaseCommand#_items} pointing to the item that is storing a batch that has given
+	 * {@link engine.model.Batch#baseVersion}.
+	 *
+	 * @private
+	 * @param {Number} baseVersion Base version of the batch to find.
+	 * @returns {Number|null}
 	 */
 	 */
-	_checkEnabled() {
-		return this._items.length > 0;
+	_getItemIndexFromBaseVersion( baseVersion ) {
+		for ( let i = 0; i < this._items.length; i++ ) {
+			if ( this._items[ i ].batch.baseVersion == baseVersion ) {
+				return i;
+			}
+		}
+
+		return null;
 	}
 	}
 
 
 	/**
 	/**
-	 * Executes the command: reverts a {@link engine.model.Batch batch} added to the command's stack,
-	 * applies it on the document and removes the batch from the stack.
+	 * Un-does a batch by reversing a batch from history, transforming that reversed batch and applying it. This is
+	 * a helper method for {@link undo.UndoCommand#_doExecute}.
 	 *
 	 *
-	 * @protected
-	 * @fires undo.undoCommand#event:revert
-	 * @param {engine.model.Batch} [batch] If set, batch that should be undone. If not set, the last added batch will be undone.
+	 * @private
+	 * @param {engine.model.Batch} batchToUndo Batch, which deltas will be reversed, transformed and applied.
+	 * @param {engine.model.Batch} undoingBatch Batch that will contain transformed and applied deltas from `batchToUndo`.
+	 * @param {engine.model.Document} document Document that is operated on by the command.
 	 */
 	 */
-	_doExecute( batch ) {
-		let batchIndex;
-
-		// If batch is not given, set `batchIndex` to the last index in command stack.
-		// If it is given, find it on the stack.
-		if ( !batch ) {
-			batchIndex = this._items.length - 1;
-		} else {
-			batchIndex = this._items.findIndex( item => item.batch == batch );
-		}
+	_undo( batchToUndo, undoingBatch, document ) {
+		const history = document.history;
+		const deltasToUndo = batchToUndo.deltas.slice();
+		deltasToUndo.reverse();
+
+		// We will process each delta from `batchToUndo`, in reverse order. If there was deltas A, B and C in undone batch,
+		// we need to revert them in reverse order, so first reverse C, then B, then A.
+		for ( let deltaToUndo of deltasToUndo ) {
+			// Keep in mind that all algorithms return arrays. That's because the transformation might result in multiple
+			// deltas, so we need arrays to handle them anyway. To simplify algorithms, it is better to always have arrays
+			// in mind. For simplicity reasons, we will use singular form in descriptions and names.
+			const baseVersion = deltaToUndo.baseVersion;
+			const nextBaseVersion = baseVersion + deltaToUndo.operations.length;
+
+			// 1. Get updated version of the delta from the history.
+			// Batch stored in the undo command might have an outdated version of the delta that should be undone.
+			// To prevent errors, we will take an updated version of it from the history, basing on delta's `baseVersion`.
+			const updatedDeltaToUndo = history.getDelta( baseVersion );
+
+			// 2. Reverse delta from the history.
+			updatedDeltaToUndo.reverse();
+			let reversedDelta = [];
+
+			for ( let delta of updatedDeltaToUndo ) {
+				reversedDelta.push( delta.getReversed() );
+			}
 
 
-		const undoItem = this._items.splice( batchIndex, 1 )[ 0 ];
+			// Stores history deltas transformed by `deltaToUndo`. Will be used later for updating document history.
+			const updatedHistoryDeltas = {};
+
+			// 3. Transform reversed delta by history deltas that happened after delta to undo. We have to bring
+			// reversed delta to the current state of document. While doing this, we will also update history deltas
+			// to the state which "does not remember" delta that we undo.
+			for ( let historyDelta of history.getDeltas( nextBaseVersion ) ) {
+				// 3.1. Transform selection range stored with history batch by reversed delta.
+				// It is important to keep stored selection ranges updated. As we are removing and updating deltas in the history,
+				// selection ranges would base on outdated history state.
+				const itemIndex = this._getItemIndexFromBaseVersion( historyDelta.baseVersion );
+
+				// `itemIndex` will be `null` for `historyDelta` if it is not the first delta in it's batch.
+				// This is fine, because we want to transform each selection only once, before transforming reversed delta
+				// by the first delta of the batch connected with the ranges.
+				if ( itemIndex !== null ) {
+					this._items[ itemIndex ].selection.ranges = transformRangesByDeltas( this._items[ itemIndex ].selection.ranges, reversedDelta );
+				}
 
 
-		// Get the batch to undo.
-		const undoBatch = undoItem.batch;
-		const undoDeltas = undoBatch.deltas.slice();
-		// Deltas have to be applied in reverse order, so if batch did A B C, it has to do reversed C, reversed B, reversed A.
-		undoDeltas.reverse();
+				// 3.2. Transform history delta by reversed delta. We need this to update document history.
+				const updatedHistoryDelta = transformDelta( [ historyDelta ], reversedDelta, false );
 
 
-		// Reverse the deltas from the batch, transform them, apply them.
-		for ( let undoDelta of undoDeltas ) {
-			const undoDeltaReversed = undoDelta.getReversed();
-			const updatedDeltas = this.editor.document.history.getTransformedDelta( undoDeltaReversed );
+				// 3.3. Transform reversed delta by history delta (in state before transformation above).
+				reversedDelta = transformDelta( reversedDelta, [ historyDelta ], true );
 
 
-			for ( let delta of updatedDeltas ) {
-				for ( let operation of delta.operations ) {
-					this.editor.document.applyOperation( operation );
+				// 3.4. Store updated history delta. Later, it will be updated in `history`.
+				if ( !updatedHistoryDeltas[ historyDelta.baseVersion ] ) {
+					updatedHistoryDeltas[ historyDelta.baseVersion ] = [];
 				}
 				}
-			}
-		}
 
 
-		// Get the selection state stored with this batch.
-		const selectionState = undoItem.selection;
-
-		// Take all selection ranges that were stored with undone batch.
-		const ranges = selectionState.ranges;
-
-		// The ranges will be transformed by deltas from history that took place
-		// after the selection got stored.
-		const deltas = this.editor.document.history.getDeltas( undoBatch.deltas[ 0 ].baseVersion );
+				updatedHistoryDeltas[ historyDelta.baseVersion ] = updatedHistoryDeltas[ historyDelta.baseVersion ].concat( updatedHistoryDelta );
+			}
 
 
-		// This will keep the transformed ranges.
-		const transformedRanges = [];
+			// 4. After reversed delta has been transformed by all history deltas, apply it.
+			for ( let delta of reversedDelta ) {
+				// Fix base version.
+				delta.baseVersion = document.version;
 
 
-		for ( let originalRange of ranges ) {
-			// We create `transformed` array. At the beginning it will have only the original range.
-			// During transformation the original range will change or even break into smaller ranges.
-			// After the range is broken into two ranges, we have to transform both of those ranges separately.
-			// For that reason, we keep all transformed ranges in one array and operate on it.
-			let transformed = [ originalRange ];
+				// Before applying, add the delta to the `undoingBatch`.
+				undoingBatch.addDelta( delta );
 
 
-			for ( let delta of deltas ) {
+				// Now, apply all operations of the delta.
 				for ( let operation of delta.operations ) {
 				for ( let operation of delta.operations ) {
-					// We look through all operations from all deltas.
-
-					for ( let t = 0; t < transformed.length; t++ ) {
-						// We transform every range by every operation.
-						// We keep current state of transformation in `transformed` array and update it.
-						let result;
-
-						switch ( operation.type ) {
-							case 'insert':
-								result = transformed[ t ].getTransformedByInsertion(
-									operation.position,
-									operation.nodeList.length,
-									true
-								);
-								break;
-
-							case 'move':
-							case 'remove':
-							case 'reinsert':
-								result = transformed[ t ].getTransformedByMove(
-									operation.sourcePosition,
-									operation.targetPosition,
-									operation.howMany,
-									true
-								);
-								break;
-						}
-
-						// If we have a transformation result, we substitute it in `transformed` array with
-						// the range that got transformed. Keep in mind that the result is an array
-						// and may contain multiple ranges.
-						if ( result ) {
-							transformed.splice( t, 1, ...result );
-
-							// Fix iterator.
-							t = t + result.length - 1;
-						}
-					}
+					document.applyOperation( operation );
 				}
 				}
 			}
 			}
 
 
-			// After `originalRange` got transformed, we have an array of ranges. Some of those
-			// ranges may be "touching" -- they can be next to each other and could be merged.
-			// Let's do this. First, we have to sort those ranges because they don't have to be
-			// in an order.
-			transformed.sort( ( a, b ) => a.start.isBefore( b.start ) ? -1 : 1 );
-
-			// Then we check if two consecutive ranges are touching. We can do it pair by pair
-			// in one dimensional loop because ranges are sorted.
-			for ( let i = 1 ; i < transformed.length; i++ ) {
-				let a = transformed[ i - 1 ];
-				let b = transformed[ i ];
-
-				if ( a.end.isTouching( b.start ) ) {
-					a.end = b.end;
-					transformed.splice( i, 1 );
-					i--;
-				}
+			// 5. Remove reversed delta from the history.
+			history.removeDelta( baseVersion );
+
+			// And all deltas that are reversing it.
+			// So the history looks like both original and reversing deltas never happened.
+			// That's why we have to update history deltas - some of them might have been basing on deltas that we are now removing.
+			for ( let delta of reversedDelta ) {
+				history.removeDelta( delta.baseVersion );
+			}
+
+			// 6. Update history deltas in history.
+			for ( let historyBaseVersion in updatedHistoryDeltas ) {
+				history.updateDelta( Number( historyBaseVersion ), updatedHistoryDeltas[ historyBaseVersion ] );
 			}
 			}
+		}
+	}
+
+	/**
+	 * Restores {@link engine.model.Document#selection document selection} state after a batch has been undone. This
+	 * is a helper method for {@link undo.UndoCommand#_doExecute}.
+	 *
+	 * @private
+	 * @param {Array.<engine.model.Range>} ranges Ranges to be restored.
+	 * @param {Boolean} isBackward Flag describing if restored range was selected forward or backward.
+	 * @param {Number} baseVersion
+	 * @param {engine.model.Document} document Document that is operated on by the command.
+	 */
+	_restoreSelection( ranges, isBackward, baseVersion, document ) {
+		// This will keep the transformed selection ranges.
+		const selectionRanges = [];
+
+		// Transform all ranges from the restored selection.
+		for ( let range of ranges ) {
+			const transformedRanges = transformSelectionRange( range, baseVersion, document );
 
 
-			// For each `originalRange` from `ranges`, we take only one transformed range.
+			// For each `range` from `ranges`, we take only one transformed range.
 			// This is because we want to prevent situation where single-range selection
 			// This is because we want to prevent situation where single-range selection
-			// got transformed to mulit-range selection. We will take the first range that
+			// got transformed to multi-range selection. We will take the first range that
 			// is not in the graveyard.
 			// is not in the graveyard.
-			const transformedRange = transformed.find(
-				( range ) => range.start.root != this.editor.document.graveyard
+			const transformedRange = transformedRanges.find(
+				( range ) => range.start.root != document.graveyard
 			);
 			);
 
 
+			// `transformedRange` might be `undefined` if transformed range ended up in graveyard.
 			if ( transformedRange ) {
 			if ( transformedRange ) {
-				transformedRanges.push( transformedRange );
+				selectionRanges.push( transformedRange );
 			}
 			}
 		}
 		}
 
 
-		// `transformedRanges` may be empty if all ranges ended up in graveyard.
-		// If that is the case, do not restore selection.
-		if ( transformedRanges.length ) {
-			this.editor.document.selection.setRanges( transformedRanges, selectionState.isBackward );
+		// `selectionRanges` may be empty if all ranges ended up in graveyard. If that is the case, do not restore selection.
+		if ( selectionRanges.length ) {
+			document.selection.setRanges( selectionRanges, isBackward );
 		}
 		}
+	}
+}
 
 
-		this.refreshState();
-		this.fire( 'revert', undoBatch );
+// Transforms given range `range` by deltas from `document` history, starting from a delta with given `baseVersion`.
+// Returns an array containing one or more ranges, which are result of the transformation.
+function transformSelectionRange( range, baseVersion, document ) {
+	const history = document.history;
+
+	// We create `transformed` array. At the beginning it will have only the original range.
+	// During transformation the original range will change or even break into smaller ranges.
+	// After the range is broken into two ranges, we have to transform both of those ranges separately.
+	// For that reason, we keep all transformed ranges in one array and operate on it.
+	let transformed = [ range ];
+
+	// The ranges will be transformed by history deltas that happened after the selection got stored.
+	// Note, that at this point, the document history is already updated by undo command execution. We will
+	// not transform the range by deltas that got undone or their reversing counterparts.
+	transformed = transformRangesByDeltas( transformed, history.getDeltas( baseVersion ) );
+
+	// After `range` got transformed, we have an array of ranges. Some of those
+	// ranges may be "touching" -- they can be next to each other and could be merged.
+	// First, we have to sort those ranges because they don't have to be in an order.
+	transformed.sort( ( a, b ) => a.start.isBefore( b.start ) ? -1 : 1 );
+
+	// Then, we check if two consecutive ranges are touching.
+	for ( let i = 1 ; i < transformed.length; i++ ) {
+		let a = transformed[ i - 1 ];
+		let b = transformed[ i ];
+
+		if ( a.end.isTouching( b.start ) ) {
+			a.end = b.end;
+			transformed.splice( i, 1 );
+			i--;
+		}
 	}
 	}
+
+	return transformed;
 }
 }
 
 
-/**
- * Fired after `UndoCommand` reverts a batch.
- *
- * @event undo.UndoCommand#revert
- * @param {engine.model.Batch} undoBatch The batch instance that got reverted.
- */
+// Transforms given set of `ranges` by given set of `deltas`. Returns transformed `ranges`.
+function transformRangesByDeltas( ranges, deltas ) {
+	for ( let delta of deltas ) {
+		for ( let operation of delta.operations ) {
+			// We look through all operations from all deltas.
+
+			for ( let i = 0; i < ranges.length; i++ ) {
+				// We transform every range by every operation.
+				let result;
+
+				switch ( operation.type ) {
+					case 'insert':
+						result = ranges[ i ].getTransformedByInsertion(
+							operation.position,
+							operation.nodeList.length,
+							true
+						);
+						break;
+
+					case 'move':
+					case 'remove':
+					case 'reinsert':
+						result = ranges[ i ].getTransformedByMove(
+							operation.sourcePosition,
+							operation.targetPosition,
+							operation.howMany,
+							true
+						);
+						break;
+				}
+
+				// If we have a transformation result, we substitute transformed range with it in `transformed` array.
+				// Keep in mind that the result is an array and may contain multiple ranges.
+				if ( result ) {
+					ranges.splice( i, 1, ...result );
+
+					// Fix iterator.
+					i = i + result.length - 1;
+				}
+			}
+		}
+	}
+
+	return ranges;
+}

+ 4 - 48
packages/ckeditor5-undo/tests/undocommand.js

@@ -26,34 +26,6 @@ afterEach( () => {
 } );
 } );
 
 
 describe( 'UndoCommand', () => {
 describe( 'UndoCommand', () => {
-	describe( 'constructor', () => {
-		it( 'should create undo command with empty batch stack', () => {
-			expect( undo._checkEnabled() ).to.be.false;
-		} );
-	} );
-
-	describe( 'clearStack', () => {
-		it( 'should remove all batches from the stack', () => {
-			undo.addBatch( doc.batch() );
-			expect( undo._checkEnabled() ).to.be.true;
-
-			undo.clearStack();
-			expect( undo._checkEnabled() ).to.be.false;
-		} );
-	} );
-
-	describe( '_checkEnabled', () => {
-		it( 'should return false if there are no batches in command stack', () => {
-			expect( undo._checkEnabled() ).to.be.false;
-		} );
-
-		it( 'should return true if there are batches in command stack', () => {
-			undo.addBatch( doc.batch() );
-
-			expect( undo._checkEnabled() ).to.be.true;
-		} );
-	} );
-
 	describe( '_execute', () => {
 	describe( '_execute', () => {
 		const p = pos => new Position( root, [].concat( pos ) );
 		const p = pos => new Position( root, [].concat( pos ) );
 		const r = ( a, b ) => new Range( p( a ), p( b ) );
 		const r = ( a, b ) => new Range( p( a ), p( b ) );
@@ -248,24 +220,22 @@ describe( 'UndoCommand', () => {
 			expect( root.getChildCount() ).to.equal( 1 );
 			expect( root.getChildCount() ).to.equal( 1 );
 			expect( root.getChild( 0 ).name ).to.equal( 'p' );
 			expect( root.getChild( 0 ).name ).to.equal( 'p' );
 
 
-			// Because P element was inserted in the middle of removed text and it was not removed,
-			// the selection is set after it.
-			expect( editor.document.selection.getRanges().next().value.isEqual( r( 1, 1 ) ) ).to.be.true;
+			expect( editor.document.selection.getRanges().next().value.isEqual( r( 0, 0 ) ) ).to.be.true;
 			expect( editor.document.selection.isBackward ).to.be.false;
 			expect( editor.document.selection.isBackward ).to.be.false;
 
 
 			undo._execute( batch1 );
 			undo._execute( batch1 );
 			// Remove attributes.
 			// Remove attributes.
 			// This does nothing in the `root` because attributes were set on nodes that already got removed.
 			// This does nothing in the `root` because attributes were set on nodes that already got removed.
-			// But those nodes should change in they graveyard and we can check them there.
+			// But those nodes should change in the graveyard and we can check them there.
 
 
 			expect( root.getChildCount() ).to.equal( 1 );
 			expect( root.getChildCount() ).to.equal( 1 );
 			expect( root.getChild( 0 ).name ).to.equal( 'p' );
 			expect( root.getChild( 0 ).name ).to.equal( 'p' );
 
 
 			// Operations for undoing that batch were working on graveyard so document selection should not change.
 			// Operations for undoing that batch were working on graveyard so document selection should not change.
-			expect( editor.document.selection.getRanges().next().value.isEqual( r( 1, 1 ) ) ).to.be.true;
+			expect( editor.document.selection.getRanges().next().value.isEqual( r( 0, 0 ) ) ).to.be.true;
 			expect( editor.document.selection.isBackward ).to.be.false;
 			expect( editor.document.selection.isBackward ).to.be.false;
 
 
-			expect( doc.graveyard.getChildCount() ).to.equal( 6 );
+			expect( doc.graveyard.getChild( 0 ).getChildCount() ).to.equal( 6 );
 
 
 			for ( let char of doc.graveyard._children ) {
 			for ( let char of doc.graveyard._children ) {
 				expect( char.hasAttribute( 'key' ) ).to.be.false;
 				expect( char.hasAttribute( 'key' ) ).to.be.false;
@@ -276,22 +246,8 @@ describe( 'UndoCommand', () => {
 			expect( root.getChildCount() ).to.equal( 0 );
 			expect( root.getChildCount() ).to.equal( 0 );
 
 
 			// Once again transformed range ends up in the graveyard.
 			// Once again transformed range ends up in the graveyard.
-			// So we do not restore it. But since Selection is a LiveRange itself it will update
-			// because the node before it (P element) got removed.
 			expect( editor.document.selection.getRanges().next().value.isEqual( r( 0, 0 ) ) ).to.be.true;
 			expect( editor.document.selection.getRanges().next().value.isEqual( r( 0, 0 ) ) ).to.be.true;
 			expect( editor.document.selection.isBackward ).to.be.false;
 			expect( editor.document.selection.isBackward ).to.be.false;
 		} );
 		} );
-
-		it( 'should fire undo event with the undone batch', () => {
-			const batch = doc.batch();
-			const spy = sinon.spy();
-
-			undo.on( 'revert', spy );
-
-			undo._execute();
-
-			expect( spy.calledOnce ).to.be.true;
-			expect( spy.calledWith( batch ) );
-		} );
 	} );
 	} );
 } );
 } );