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

Merge pull request #14 from ckeditor/t/undo-2

T/undo 2
Piotr Jasiun 9 лет назад
Родитель
Сommit
dc73074954

+ 96 - 0
packages/ckeditor5-undo/src/basecommand.js

@@ -0,0 +1,96 @@
+/**
+ * @license Copyright (c) 2003-2016, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+'use strict';
+
+import Command from '../command/command.js';
+import transform from '../engine/model/delta/transform.js';
+
+/**
+ * Base class for undo feature commands: {@link undo.UndoCommand} and {@link undo.RedoCommand}.
+ *
+ * @protected
+ * @memberOf undo
+ */
+export default class BaseCommand extends Command {
+	constructor( editor ) {
+		super( editor );
+
+		/**
+		 * Stack of items stored by the command. These are pairs of:
+		 *
+		 * * {@link engine.model.Batch batch} saved by the command and,
+		 * * {@link engine.model.Selection selection} state at the moment of saving the batch.
+		 *
+		 * @protected
+		 * @member {Array} undo.BaseCommand#_stack
+		 */
+		this._stack = [];
+
+		/**
+		 * Stores all batches that were created by this command.
+		 *
+		 * @protected
+		 * @member {WeakSet.<engine.model.Batch>} undo.BaseCommand#_createdBatches
+		 */
+		this._createdBatches = new WeakSet();
+
+		// Refresh state, so command is inactive just after initialization.
+		this.refreshState();
+	}
+
+	/**
+	 * Stores a batch in the command, together with the selection state of the {@link engine.model.Document document}
+	 * created by the editor which this command is registered to.
+	 *
+	 * @param {engine.model.Batch} batch Batch to add.
+	 */
+	addBatch( batch ) {
+		const selection = {
+			ranges: Array.from( this.editor.document.selection.getRanges() ),
+			isBackward: this.editor.document.selection.isBackward
+		};
+
+		this._stack.push( { batch, selection } );
+		this.refreshState();
+	}
+
+	/**
+	 * Removes all items from the stack.
+	 */
+	clearStack() {
+		this._stack = [];
+		this.refreshState();
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	_checkEnabled() {
+		return this._stack.length > 0;
+	}
+}
+
+// Performs a transformation of delta set `setToTransform` by given delta set `setToTransformBy`.
+// If `setToTransform` deltas are more important than `setToTransformBy` deltas, `isStrong` should be true.
+export function transformDelta( setToTransform, setToTransformBy, isStrong = false ) {
+	let results = [];
+
+	for ( let toTransform of setToTransform ) {
+		let to = [ toTransform ];
+
+		for ( let t = 0; t < to.length; t++ ) {
+			for ( let transformBy of setToTransformBy ) {
+				let transformed = transform( to[ t ], transformBy, isStrong );
+				to.splice( t, 1, ...transformed );
+				t = t - 1 + transformed.length;
+			}
+		}
+
+		results = results.concat( to );
+	}
+
+	return results;
+}

+ 110 - 0
packages/ckeditor5-undo/src/redocommand.js

@@ -0,0 +1,110 @@
+/**
+ * @license Copyright (c) 2003-2016, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+'use strict';
+
+import BaseCommand from './basecommand.js';
+import { transformDelta as transformDelta } from './basecommand.js';
+
+/**
+ * Redo command stores {@link engine.model.Batch batches} that were used to undo a batch by {@link undo.UndoCommand UndoCommand}.
+ * It is able to redo a previously undone batch by reversing the undoing batches created by `UndoCommand`. Reversed batch is
+ * also transformed by batches from {@link engine.model.Document#history history} that happened after it and are not other redo batches.
+ *
+ * Redo command also takes care of restoring {@link engine.model.Document#selection selection} to the state before
+ * undone batch was applied.
+ *
+ * @memberOf undo
+ * @extends undo.BaseCommand
+ */
+export default class RedoCommand extends BaseCommand {
+	/**
+	 * Executes the command: reverts last {@link engine.model.Batch batch} added to the command's stack, applies
+	 * reverted and transformed version on the {@link engine.model.Document document} and removes the batch from the stack.
+	 * Then, restores {@link engine.model.Document#selection document selection}.
+	 *
+	 * @protected
+	 */
+	_doExecute() {
+		const item = this._stack.pop();
+
+		// All changes have 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._redo( item.batch );
+			this._restoreSelection( item.selection.ranges, item.selection.isBackward );
+		} );
+
+		this.refreshState();
+	}
+
+	/**
+	 * Re-does a batch by reversing the batch that undone it, transforming that batch and applying it. This is
+	 * a helper method for {@link undo.RedoCommand#_doExecute}.
+	 *
+	 * @private
+	 * @param {engine.model.Batch} storedBatch Batch, which deltas will be reversed, transformed and applied.
+	 * @param {engine.model.Batch} redoingBatch Batch that will contain transformed and applied deltas from `storedBatch`.
+	 * @param {engine.model.Document} document Document that is operated on by the command.
+	 */
+	_redo( storedBatch ) {
+		const document = this.editor.document;
+
+		// All changes done by the command execution will be saved as one batch.
+		const redoingBatch = document.batch();
+		this._createdBatches.add( redoingBatch );
+
+		const deltasToRedo = storedBatch.deltas.slice();
+		deltasToRedo.reverse();
+
+		// We will process each delta from `storedBatch`, in reverse order. If there was deltas A, B and C in stored batch,
+		// we need to revert them in reverse order, so first reverse C, then B, then A.
+		for ( let deltaToRedo of deltasToRedo ) {
+			// 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 nextBaseVersion = deltaToRedo.baseVersion + deltaToRedo.operations.length;
+
+			// As stated above, convert delta to array of deltas.
+			let reversedDelta = [ deltaToRedo.getReversed() ];
+
+			// 1. Transform that delta by deltas from history that happened after it.
+			// Omit deltas from "redo" batches, because reversed delta already bases on them. Transforming by them
+			// again will result in incorrect deltas.
+			for ( let historyDelta of document.history.getDeltas( nextBaseVersion ) ) {
+				if ( !this._createdBatches.has( historyDelta.batch ) ) {
+					reversedDelta = transformDelta( reversedDelta, [ historyDelta ], true );
+				}
+			}
+
+			// 2. After reversed delta has been transformed by all history deltas, apply it.
+			for ( let delta of reversedDelta ) {
+				// Fix base version.
+				delta.baseVersion = document.version;
+
+				// Before applying, add the delta to the `redoingBatch`.
+				redoingBatch.addDelta( delta );
+
+				// Now, apply all operations of the delta.
+				for ( let operation of delta.operations ) {
+					document.applyOperation( operation );
+				}
+			}
+		}
+	}
+
+	/**
+	 * Restores {@link engine.model.Document#selection document selection} state after a batch has been re-done. This
+	 * is a helper method for {@link undo.RedoCommand#_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.
+	 */
+	_restoreSelection( ranges, isBackward ) {
+		this.editor.document.selection.setRanges( ranges, isBackward );
+	}
+}

+ 102 - 0
packages/ckeditor5-undo/src/undo.js

@@ -14,6 +14,108 @@ import ButtonView from '../ui/button/buttonview.js';
 /**
  * Undo feature. Introduces the "Undo" and "Redo" buttons to the editor.
  *
+ * Below is the explanation of undo mechanism working together with {@link engine.model.CompressedHistory CompressedHistory}:
+ *
+ * Whenever a {@link engine.model.Delta delta} is applied to the {@link engine.model.Document document}, it is saved to
+ * `CompressedHistory` as is. The {@link engine.model.Batch batch} that owns that delta is also saved, in {@link undo.UndoCommand},
+ * together with selection that was on the document before the delta was applied. Batch is saved instead of delta because
+ * changes are undone batch-by-batch, not delta-by-delta and batch is seen as one undo step.
+ *
+ * After some changes happen to the document, we can represent `CompressedHistory` and `UndoCommand` stack as follows:
+ *
+ *		  history                           undo stack
+ *		===========             ==================================
+ *		[delta A1]              [batch A with selection before A1]
+ *		[delta B1]              [batch B with selection before B1]
+ *		[delta B2]              [batch C with selection before C1]
+ *		[delta C1]
+ *		[delta C2]
+ *		[delta B3]
+ *		[delta C3]
+ *
+ * Where deltas starting by the same letter are from same batch.
+ *
+ * Undoing a batch means that we need to generate set of deltas which will reverse effects of that batch. I.e. if batch
+ * added several letters, undoing batch should remove them. It is important to apply undoing deltas in reversed order,
+ * so if batch has delta `X`, `Y`, `Z` we should apply reversed deltas `Zr`, `Yr` and `Xr`. In other case, reversed delta
+ * `Xr` would operate on wrong document state, because delta `X` does not know that delta `Y` and `Z` happened.
+ *
+ * After deltas from undone batch got {@link engine.model.Delta#getReversed reversed} we need to make sure if they are
+ * ready to be applied. In our scenario, delta `C3` is the last delta so `C3r` bases on up-to-date document state so
+ * it can be applied to the document.
+ *
+ *		  history                           undo stack
+ *		===========             ==================================
+ *		[delta A1 ]             [batch A with selection before A1]
+ *		[delta B1 ]             [batch B with selection before B1]
+ *		[delta B2 ]             [   processing undoing batch C   ]
+ *		[delta C1 ]
+ *		[delta C2 ]
+ *		[delta B3 ]
+ *		[delta C3 ]
+ *		[delta C3r]
+ *
+ * Next is delta `C2`, reversed to `C2r`. `C2r` bases on `C2`, so it bases on wrong document state. It needs to be
+ * transformed by deltas from history that happened after it, so it "knows" about them. Let `C2' = C2r * B3 * C3 * C3r`,
+ * where `*` means "transformed by". As can be seen, `C2r` is transformed by a delta which is undone afterwards anyway.
+ * This brings two problems: lower effectiveness (obvious) and incorrect results. Bad results come from the fact that
+ * operational transformation algorithms assume there is no connection between two transformed operations when resolving
+ * conflicts, which is true for, i.e. collaborative editing, but is not true for undo algorithm.
+ *
+ * To prevent both problems, `CompressedHistory` introduces an API to {@link engine.model.CompressedDelta#removeDelta remove}
+ * deltas from history. It is used to remove undone and undoing deltas after they are applied. It feels right - delta is
+ * undone/reversed = "removed", there should be no sign of it in history (fig. 1). `---` symbolizes removed delta.
+ *
+ *		history (fig. 1)            history (fig. 2)            history (fig. 3)
+ *		================            ================            ================
+ *		   [delta A1]                  [delta A1]                  [delta A1]
+ *		   [delta B1]                  [delta B1]                  [delta B1]
+ *		   [delta B2]                  [delta B2]                  [delta B2]
+ *		   [delta C1]                  [delta C1]                  [---C1---]
+ *		   [delta C2]                  [---C2---]                  [---C2---]
+ *		   [delta B3]                  [delta B3]                  [delta B3]
+ *		   [---C3---]                  [---C3---]                  [---C3---]
+ *		   [---C3r--]                  [---C3r--]                  [---C3r--]
+ *		                               [---C2'--]                  [---C2'--]
+ *		                                                           [---C1'--]
+ *
+ * Now we can transform `C2r` only by `B3` and remove both it and `C2` (fig. 2). Same with `C1` (fig. 3). `'` symbolizes
+ * reversed delta that was later transformed.
+ *
+ * But what about that selection? For batch `C`, undo feature remembers selection just before `C1` was applied. It can be
+ * visualized between delta `B2` and `B3` (see fig. 3). As can be seen, some operations were applied to the document since the selection
+ * state was remembered. Setting document selection as it was remembered would be incorrect. It feels natural that
+ * selection state should also be transformed by deltas from history. Same pattern applies as with transforming deltas - ranges
+ * should not be transformed by undone and undoing deltas. Thankfully, those deltas are already removed from history.
+ *
+ * Unfortunately, a problem appears with delta `B3`. It still remembers context of deltas `C2` and `C1` on which it bases.
+ * It is an obvious error: i.e. transforming by that delta would lead to wrong results or "repeating" history would
+ * produce different document than actual.
+ *
+ * To prevent this situation, we have to also {@link engine.model.CompressedHistory#updateDelta update} `B3` in history.
+ * It should be kept in a state that "does not remember" deltas that has been removed from the history. It is easily
+ * achieved while transforming reversed delta. I.e., when `C2r` is transformed by `B3`, at the same time we transform
+ * `B3` by `C2r`. Transforming `B3` that remembers `C2` by delta reversing `C2` effectively makes `B3` "forget" about `C2`.
+ * By doing those transformation we effectively make `B3` base on `B2` which is a correct state of history (fig. 4).
+ *
+ *		     history (fig. 4)                         history (fig. 5)
+ *		===========================            ===============================
+ *		        [delta A1]                               [---A1---]
+ *		        [delta B1]                         [delta B1 "without A1"]
+ *		        [delta B2]                         [delta B2 "without A1"]
+ *		        [---C1---]                               [---C1---]
+ *		        [---C2---]                               [---C2---]
+ *		[delta B3 "without C2, C1"]            [delta B3 "without C2, C1, A1"]
+ *		        [---C3---]                               [---C3---]
+ *		        [---C3r--]                               [---C3r--]
+ *		        [---C2'--]                               [---C2'--]
+ *		        [---C1'--]                               [---C1'--]
+ *		                                                 [---A1'--]
+ *
+ * Selective undo works on the same basis, however instead of undoing the last batch in undo stack, any batch can be undone.
+ * Same algorithm applies: deltas from batch (i.e. `A1`) are reversed and then transformed by deltas stored in history,
+ * simultaneously updating them. Then deltas are applied to the document and removed from history (fig. 5).
+ *
  * @memberOf undo
  * @extends ckeditor5.Feature
  */

+ 244 - 155
packages/ckeditor5-undo/src/undocommand.js

@@ -5,207 +5,296 @@
 
 '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
+ * @extends undo.BaseCommand
  */
-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._stack.findIndex( ( a ) => a.batch == batch ) : this._stack.length - 1;
 
-		this._items.push( { batch, selection } );
-		this.refreshState();
-	}
+		const item = this._stack.splice( batchIndex, 1 )[ 0 ];
 
-	/**
-	 * Removes all batches from the stack.
-	 */
-	clearStack() {
-		this._items = [];
+		// 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 );
+			this._restoreSelection( item.selection.ranges, item.selection.isBackward, item.batch.baseVersion );
+		} );
+
+		this.fire( 'revert', item.batch );
 		this.refreshState();
 	}
 
 	/**
-	 * @inheritDoc
+	 * Returns index in {@link undo.BaseCommand#_stack} 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._stack.length; i++ ) {
+			if ( this._stack[ 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 );
-		}
-
-		const undoItem = this._items.splice( batchIndex, 1 )[ 0 ];
+	_undo( batchToUndo ) {
+		const document = this.editor.document;
+
+		// All changes done by the command execution will be saved as one batch.
+		const undoingBatch = document.batch();
+		this._createdBatches.add( undoingBatch );
+
+		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 );
+
+			// This is a safe valve in case of not finding delta to undo in history. This may come up if that delta
+			// got updated into no deltas, or removed from history.
+			if ( updatedDeltaToUndo === null ) {
+				continue;
+			}
 
-		// 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();
+			// 2. Reverse delta from the history.
+			updatedDeltaToUndo.reverse();
+			let reversedDelta = [];
 
-		// 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 );
+			for ( let delta of updatedDeltaToUndo ) {
+				reversedDelta.push( delta.getReversed() );
+			}
 
-			for ( let delta of updatedDeltas ) {
-				for ( let operation of delta.operations ) {
-					this.editor.document.applyOperation( operation );
+			// 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._stack[ itemIndex ].selection.ranges = transformRangesByDeltas( this._stack[ itemIndex ].selection.ranges, reversedDelta );
 				}
-			}
-		}
 
-		// Get the selection state stored with this batch.
-		const selectionState = undoItem.selection;
+				// 3.2. Transform history delta by reversed delta. We need this to update document history.
+				const updatedHistoryDelta = transformDelta( [ historyDelta ], reversedDelta, false );
 
-		// Take all selection ranges that were stored with undone batch.
-		const ranges = selectionState.ranges;
+				// 3.3. Transform reversed delta by history delta (in state before transformation above).
+				reversedDelta = transformDelta( reversedDelta, [ historyDelta ], true );
 
-		// 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 );
+				// 3.4. Store updated history delta. Later, it will be updated in `history`.
+				if ( !updatedHistoryDeltas[ historyDelta.baseVersion ] ) {
+					updatedHistoryDeltas[ historyDelta.baseVersion ] = [];
+				}
 
-		// This will keep the transformed ranges.
-		const transformedRanges = [];
+				updatedHistoryDeltas[ historyDelta.baseVersion ] = updatedHistoryDeltas[ historyDelta.baseVersion ].concat( updatedHistoryDelta );
+			}
+
+			// 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 ) {
-					// 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 ) {
+		const document = this.editor.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
-			// 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.
-			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 ) {
-				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;
+}

+ 24 - 21
packages/ckeditor5-undo/src/undoengine.js

@@ -7,6 +7,7 @@
 
 import Feature from '../feature.js';
 import UndoCommand from './undocommand.js';
+import RedoCommand from './redocommand.js';
 
 /**
  * Undo engine feature.
@@ -25,25 +26,23 @@ export default class UndoEngine extends Feature {
 		super( editor );
 
 		/**
-		 * Undo command which manages undo {@link engine.model.Batch batches} stack (history).
+		 * Command which manages undo {@link engine.model.Batch batches} stack (history).
 		 * Created and registered during {@link undo.UndoEngine#init feature initialization}.
 		 *
 		 * @private
 		 * @member {undo.UndoEngineCommand} undo.UndoEngine#_undoCommand
 		 */
-		this._undoCommand = null;
 
 		/**
-		 * Undo command which manages redo {@link engine.model.Batch batches} stack (history).
+		 * Command which manages redo {@link engine.model.Batch batches} stack (history).
 		 * Created and registered during {@link undo.UndoEngine#init feature initialization}.
 		 *
 		 * @private
 		 * @member {undo.UndoEngineCommand} undo.UndoEngine#_redoCommand
 		 */
-		this._redoCommand = null;
 
 		/**
-		 * Keeps track of which batch has already been added to undo manager.
+		 * Keeps track of which batch has been registered in Undo.
 		 *
 		 * @private
 		 * @member {WeakSet.<engine.model.Batch>} undo.UndoEngine#_batchRegistry
@@ -56,30 +55,34 @@ export default class UndoEngine extends Feature {
 	 */
 	init() {
 		// Create commands.
-		this._redoCommand = new UndoCommand( this.editor );
 		this._undoCommand = new UndoCommand( this.editor );
+		this._redoCommand = new RedoCommand( this.editor );
 
 		// Register command to the editor.
-		this.editor.commands.set( 'redo', this._redoCommand );
 		this.editor.commands.set( 'undo', this._undoCommand );
+		this.editor.commands.set( 'redo', this._redoCommand );
 
 		this.listenTo( this.editor.document, 'change', ( evt, type, changes, batch ) => {
-			// Whenever a new batch is created add it to the undo history and clear redo history.
-			if ( batch && !this._batchRegistry.has( batch ) ) {
-				this._batchRegistry.add( batch );
-				this._undoCommand.addBatch( batch );
-				this._redoCommand.clearStack();
+			// If changes are not a part of a batch or this is not a new batch, omit those changes.
+			if ( this._batchRegistry.has( batch ) || batch.type == 'transparent' ) {
+				return;
+			} else {
+				if ( this._undoCommand._createdBatches.has( batch ) ) {
+					// If this batch comes from `undoCommand`, add it to `redoCommand` stack.
+					this._redoCommand.addBatch( batch );
+				} else if ( this._redoCommand._createdBatches.has( batch ) ) {
+					// If this batch comes from `redoCommand`, add it to `undoCommand` stack.
+					this._undoCommand.addBatch( batch );
+				} else {
+					// A default batch - these are new changes in the document, not introduced by undo feature.
+					// Add them to `undoCommand` stack and clear `redoCommand` stack.
+					this._undoCommand.addBatch( batch );
+					this._redoCommand.clearStack();
+				}
 			}
-		} );
-
-		// Whenever batch is reverted by undo command, add it to redo history.
-		this.listenTo( this._redoCommand, 'revert', ( evt, batch ) => {
-			this._undoCommand.addBatch( batch );
-		} );
 
-		// Whenever batch is reverted by redo command, add it to undo history.
-		this.listenTo( this._undoCommand, 'revert', ( evt, batch ) => {
-			this._redoCommand.addBatch( batch );
+			// Add the batch to the registry so it will not be processed again.
+			this._batchRegistry.add( batch );
 		} );
 	}
 }

+ 53 - 0
packages/ckeditor5-undo/tests/basecommand.js

@@ -0,0 +1,53 @@
+/**
+ * @license Copyright (c) 2003-2016, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+'use strict';
+
+import ModelTestEditor from '/tests/ckeditor5/_utils/modeltesteditor.js';
+import BaseCommand from '/ckeditor5/undo/basecommand.js';
+
+let editor, doc, root, base;
+
+beforeEach( () => {
+	editor = new ModelTestEditor();
+	base = new BaseCommand( editor );
+
+	doc = editor.document;
+
+	root = doc.getRoot();
+} );
+
+afterEach( () => {
+	base.destroy();
+} );
+
+describe( 'BaseCommand', () => {
+	describe( 'constructor', () => {
+		it( 'should create command with empty batch stack', () => {
+			expect( base._checkEnabled() ).to.be.false;
+		} );
+	} );
+
+	describe( '_checkEnabled', () => {
+		it( 'should return false if there are no batches in command stack', () => {
+			expect( base._checkEnabled() ).to.be.false;
+		} );
+
+		it( 'should return true if there are batches in command stack', () => {
+			base.addBatch( doc.batch() );
+
+			expect( base._checkEnabled() ).to.be.true;
+		} );
+	} );
+
+	describe( 'clearStack', () => {
+		it( 'should remove all batches from the stack', () => {
+			base.addBatch( doc.batch() );
+			base.clearStack();
+
+			expect( base._checkEnabled() ).to.be.false;
+		} );
+	} );
+} );

+ 218 - 0
packages/ckeditor5-undo/tests/redocommand.js

@@ -0,0 +1,218 @@
+/**
+ * @license Copyright (c) 2003-2016, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+'use strict';
+
+import ModelTestEditor from '/tests/ckeditor5/_utils/modeltesteditor.js';
+import Range from '/ckeditor5/engine/model/range.js';
+import Position from '/ckeditor5/engine/model/position.js';
+import UndoCommand from '/ckeditor5/undo/undocommand.js';
+import RedoCommand from '/ckeditor5/undo/redocommand.js';
+
+let editor, doc, root, redo, undo;
+
+beforeEach( () => {
+	editor = new ModelTestEditor();
+	redo = new RedoCommand( editor );
+
+	doc = editor.document;
+
+	root = doc.getRoot();
+} );
+
+afterEach( () => {
+	redo.destroy();
+} );
+
+describe( 'RedoCommand', () => {
+	describe( '_execute', () => {
+		const p = pos => new Position( root, [].concat( pos ) );
+		const r = ( a, b ) => new Range( p( a ), p( b ) );
+
+		let batch0, batch1, batch2;
+		let batches = new Set();
+
+		beforeEach( () => {
+			undo = new UndoCommand( editor );
+
+			// Simple integration with undo.
+			doc.on( 'change', ( evt, type, data, batch ) => {
+				if ( undo._createdBatches.has( batch ) && !batches.has( batch ) ) {
+					redo.addBatch( batch );
+					batches.add( batch );
+				}
+			} );
+
+			/*
+			 [root]
+			 - {}
+			 */
+			editor.document.selection.setRanges( [ r( 0, 0 ) ] );
+			batch0 = doc.batch();
+			undo.addBatch( batch0 );
+			batch0.insert( p( 0 ), 'foobar' );
+			/*
+			 [root]
+			 - f
+			 - o
+			 - o
+			 - b
+			 - a
+			 - r{}
+			 */
+			// Let's make things spicy and this time, make a backward selection.
+			editor.document.selection.setRanges( [ r( 2, 4 ) ], true );
+			batch1 = doc.batch();
+			undo.addBatch( batch1 );
+			batch1.setAttr( 'key', 'value', r( 2, 4 ) );
+			/*
+			 [root]
+			 - f
+			 - o
+			 - {o (key: value)
+			 - b} (key: value)
+			 - a
+			 - r
+			 */
+			editor.document.selection.setRanges( [ r( 1, 3 ) ] );
+			batch2 = doc.batch();
+			undo.addBatch( batch2 );
+			batch2.move( r( 1, 3 ), p( 6 ) );
+			/*
+			 [root]
+			 - f
+			 - b (key: value)
+			 - a
+			 - r
+			 - {o
+			 - o} (key: value)
+			 */
+		} );
+
+		it( 'should redo batch undone by undo command', () => {
+			undo._execute( batch2 );
+
+			redo._execute();
+			// Should be back at original state:
+			/*
+			 [root]
+			 - f
+			 - b (key: value)
+			 - a
+			 - r
+			 - {o
+			 - o} (key: value)
+			 */
+			expect( Array.from( root._children._nodes.map( node => node.text ) ).join( '' ) ).to.equal( 'fbaroo' );
+			expect( root.getChild( 1 ).getAttribute( 'key' ) ).to.equal( 'value' );
+			expect( root.getChild( 5 ).getAttribute( 'key' ) ).to.equal( 'value' );
+
+			expect( editor.document.selection.getRanges().next().value.isEqual( r( 4, 6 ) ) ).to.be.true;
+			expect( editor.document.selection.isBackward ).to.be.false;
+		} );
+
+		it( 'should redo series of batches undone by undo command', () => {
+			undo._execute( batch2 );
+			undo._execute( batch1 );
+			undo._execute( batch0 );
+
+			redo._execute();
+			// Should be like after applying `batch0`:
+			/*
+			 [root]
+			 - f
+			 - o
+			 - {o
+			 - b}
+			 - a
+			 - r
+			 */
+			expect( Array.from( root._children._nodes.map( node => node.text ) ).join( '' ) ).to.equal( 'foobar' );
+			expect( root._children._nodes.find( node => node.hasAttribute( 'key' ) ) ).to.be.undefined;
+
+			expect( editor.document.selection.getRanges().next().value.isEqual( r( 2, 4 ) ) ).to.be.true;
+			expect( editor.document.selection.isBackward ).to.be.true;
+
+			redo._execute();
+			// Should be like after applying `batch1`:
+			/*
+			 [root]
+			 - f
+			 - {o
+			 - o} (key: value)
+			 - b (key: value)
+			 - a
+			 - r
+			 */
+			expect( Array.from( root._children._nodes.map( node => node.text ) ).join( '' ) ).to.equal( 'foobar' );
+			expect( root.getChild( 2 ).getAttribute( 'key' ) ).to.equal( 'value' );
+			expect( root.getChild( 3 ).getAttribute( 'key' ) ).to.equal( 'value' );
+
+			expect( editor.document.selection.getRanges().next().value.isEqual( r( 1, 3 ) ) ).to.be.true;
+			expect( editor.document.selection.isBackward ).to.be.false;
+
+			redo._execute();
+			// Should be like after applying `batch2`:
+			/*
+			 [root]
+			 - f
+			 - b (key: value)
+			 - a
+			 - r
+			 - {o
+			 - o} (key: value)
+			 */
+			expect( Array.from( root._children._nodes.map( node => node.text ) ).join( '' ) ).to.equal( 'fbaroo' );
+			expect( root.getChild( 1 ).getAttribute( 'key' ) ).to.equal( 'value' );
+			expect( root.getChild( 5 ).getAttribute( 'key' ) ).to.equal( 'value' );
+
+			expect( editor.document.selection.getRanges().next().value.isEqual( r( 4, 6 ) ) ).to.be.true;
+			expect( editor.document.selection.isBackward ).to.be.false;
+		} );
+
+		it( 'should redo batch selectively undone by undo command', () => {
+			undo._execute( batch0 );
+			redo._execute();
+
+			// Should be back to original state:
+			/*
+			 [root]
+			 - f
+			 - b (key: value)
+			 - a
+			 - r
+			 - {o
+			 - o} (key: value)
+			 */
+			expect( Array.from( root._children._nodes.map( node => node.text ) ).join( '' ) ).to.equal( 'fbaroo' );
+			expect( root.getChild( 1 ).getAttribute( 'key' ) ).to.equal( 'value' );
+			expect( root.getChild( 5 ).getAttribute( 'key' ) ).to.equal( 'value' );
+
+			expect( editor.document.selection.getRanges().next().value.isEqual( r( 4, 6 ) ) ).to.be.true;
+			expect( editor.document.selection.isBackward ).to.be.false;
+		} );
+
+		it( 'should transform redo batch by changes written in history that happened after undo but before redo', () => {
+			// Undo moving "oo" to the end of string. Now it is "foobar".
+			undo._execute( batch2 );
+
+			// Remove "ar".
+			editor.document.selection.setRanges( [ r( 4, 6 ) ] );
+			doc.batch().remove( r( 4, 6 ) );
+			editor.document.selection.setRanges( [ r( 4, 4 ) ] );
+
+			// Redo moving "oo" to the end of string. It should be "fboo".
+			redo._execute();
+
+			expect( Array.from( root._children._nodes.map( node => node.text ) ).join( '' ) ).to.equal( 'fboo' );
+			expect( root.getChild( 1 ).getAttribute( 'key' ) ).to.equal( 'value' );
+			expect( root.getChild( 3 ).getAttribute( 'key' ) ).to.equal( 'value' );
+
+			// Selection after redo is not working properly if there was another batch in-between.
+			// Thankfully this will be very rare situation outside of OT, because normally an applied batch
+			// would reset the redo stack so you won't be able to redo. #12
+		} );
+	} );
+} );

+ 149 - 80
packages/ckeditor5-undo/tests/undocommand.js

@@ -9,6 +9,7 @@ import ModelTestEditor from '/tests/ckeditor5/_utils/modeltesteditor.js';
 import Range from '/ckeditor5/engine/model/range.js';
 import Position from '/ckeditor5/engine/model/position.js';
 import UndoCommand from '/ckeditor5/undo/undocommand.js';
+import AttributeDelta from '/ckeditor5/engine/model/delta/attributedelta.js';
 
 let editor, doc, root, undo;
 
@@ -26,38 +27,10 @@ afterEach( () => {
 } );
 
 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;
-		} );
-	} );
+	const p = pos => new Position( root, [].concat( pos ) );
+	const r = ( a, b ) => new Range( p( a ), p( b ) );
 
 	describe( '_execute', () => {
-		const p = pos => new Position( root, [].concat( pos ) );
-		const r = ( a, b ) => new Range( p( a ), p( b ) );
-
 		let batch0, batch1, batch2, batch3;
 
 		beforeEach( () => {
@@ -113,9 +86,9 @@ describe( 'UndoCommand', () => {
 			 [root]
 			 - f
 			 - [p]
-			 	- {b (key: value)
-			 	- a
-			 	- r}
+			 - {b (key: value)
+			 - a
+			 - r}
 			 - o
 			 - o (key: value)
 			 */
@@ -124,9 +97,9 @@ describe( 'UndoCommand', () => {
 			/*
 			 [root]
 			 - [p]
-			 	- b (key: value)
-			 	- a
-			 	- r
+			 - b (key: value)
+			 - a
+			 - r
 			 - o
 			 - f
 			 - o{} (key: value)
@@ -139,13 +112,13 @@ describe( 'UndoCommand', () => {
 
 			// Selection is restored. Wrap is removed:
 			/*
-				[root]
-				- {b (key: value)
-				- a
-				- r}
-				- o
-				- f
-				- o (key: value)
+			 [root]
+			 - {b (key: value)
+			 - a
+			 - r}
+			 - o
+			 - f
+			 - o (key: value)
 			 */
 
 			expect( Array.from( root._children._nodes.map( node => node.text ) ).join( '' ) ).to.equal( 'barofo' );
@@ -159,13 +132,13 @@ describe( 'UndoCommand', () => {
 
 			// Two moves are removed:
 			/*
-				[root]
-				- f
-			 	- {o
-			 	- o} (key: value)
-				- b (key: value)
-				- a
-				- r
+			 [root]
+			 - f
+			 - {o
+			 - o} (key: value)
+			 - b (key: value)
+			 - a
+			 - r
 			 */
 
 			expect( Array.from( root._children._nodes.map( node => node.text ) ).join( '' ) ).to.equal( 'foobar' );
@@ -179,13 +152,13 @@ describe( 'UndoCommand', () => {
 
 			// Set attribute is undone:
 			/*
-				[root]
-				- f
-				- o
-				- {o
-				- b}
-				- a
-				- r
+			 [root]
+			 - f
+			 - o
+			 - {o
+			 - b}
+			 - a
+			 - r
 			 */
 
 			expect( Array.from( root._children._nodes.map( node => node.text ) ).join( '' ) ).to.equal( 'foobar' );
@@ -199,7 +172,7 @@ describe( 'UndoCommand', () => {
 
 			// Insert is undone:
 			/*
-				[root]
+			 [root]
 			 */
 
 			expect( root.getChildCount() ).to.equal( 0 );
@@ -212,14 +185,14 @@ describe( 'UndoCommand', () => {
 			undo._execute( batch1 );
 			// Remove attribute:
 			/*
-				[root]
-				- [p]
-					- b
-					- a
-					- r
-				- o
-				- f
-				- o
+			 [root]
+			 - [p]
+			 - b
+			 - a
+			 - r
+			 - o
+			 - f
+			 - o
 			 */
 
 			expect( root.getChild( 0 ).name ).to.equal( 'p' );
@@ -248,24 +221,22 @@ describe( 'UndoCommand', () => {
 			expect( root.getChildCount() ).to.equal( 1 );
 			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;
 
 			undo._execute( batch1 );
 			// Remove attributes.
 			// 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.getChild( 0 ).name ).to.equal( 'p' );
 
 			// 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( doc.graveyard.getChildCount() ).to.equal( 6 );
+			expect( doc.graveyard.getChild( 0 ).getChildCount() ).to.equal( 6 );
 
 			for ( let char of doc.graveyard._children ) {
 				expect( char.hasAttribute( 'key' ) ).to.be.false;
@@ -276,22 +247,120 @@ describe( 'UndoCommand', () => {
 			expect( root.getChildCount() ).to.equal( 0 );
 
 			// 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.isBackward ).to.be.false;
 		} );
+	} );
+
+	// Some tests to ensure 100% CC and proper behavior in edge cases.
+	describe( 'edge cases', () => {
+		function getText( root ) {
+			let text = '';
+
+			for ( let i = 0; i < root.getChildCount(); i++ ) {
+				let node = root.getChild( i );
+				text += node.getAttribute( 'uppercase' ) ? node.character.toUpperCase() : node.character;
+			}
+
+			return text;
+		}
+
+		it( 'correctly handles deltas in compressed history that were earlier updated into multiple deltas (or split when undoing)', () => {
+			// In this case we assume that one of the deltas in compressed history was updated to two deltas.
+			// This is a tricky edge case because it is almost impossible to come up with convincing scenario that produces it.
+			// At the moment of writing this test and comment, only Undo feature uses `CompressedHistory#updateDelta`.
+			// Because deltas that "stays" in history are transformed with `isStrong` flag set to `false`, `MoveOperation`
+			// won't get split and `AttributeDelta` can hold multiple `AttributeOperation` in it. So using most common deltas
+			// (`InsertDelta`, `RemoveDelta`, `MoveDelta`, `AttributeDelta`) and undo it's impossible to get to this edge case.
+			// Still there might be some weird scenarios connected with OT / Undo / Collaborative Editing / other deltas /
+			// fancy 3rd party plugin where it may come up, so it's better to be safe than sorry.
+
+			root.appendChildren( 'abcdef' );
+			expect( getText( root ) ).to.equal( 'abcdef' );
+
+			editor.document.selection.setRanges( [ r( 1, 4 ) ] );
+			let batch0 = doc.batch();
+			undo.addBatch( batch0 );
+			batch0.move( r( 1, 4 ), p( 5 ) );
+			expect( getText( root ) ).to.equal( 'aebcdf' );
+
+			editor.document.selection.setRanges( [ r( 1, 1 ) ]  );
+			let batch1 = doc.batch();
+			undo.addBatch( batch1 );
+			batch1.remove( r( 0, 1 ) );
+			expect( getText( root ) ).to.equal( 'ebcdf' );
+
+			editor.document.selection.setRanges( [ r( 0, 3 ) ] );
+			let batch2 = doc.batch();
+			undo.addBatch( batch2 );
+			batch2.setAttr( 'uppercase', true, r( 0, 3 ) );
+			expect( getText( root ) ).to.equal( 'EBCdf' );
 
-		it( 'should fire undo event with the undone batch', () => {
-			const batch = doc.batch();
-			const spy = sinon.spy();
+			undo._execute( batch0 );
+			expect( getText( root ) ).to.equal( 'BCdEf' );
+
+			// Let's simulate splitting the delta by updating the history by hand.
+			let attrHistoryDelta = doc.history.getDelta( 2 )[ 0 ];
+			let attrDelta1 = new AttributeDelta();
+			attrDelta1.addOperation( attrHistoryDelta.operations[ 0 ] );
+			let attrDelta2 = new AttributeDelta();
+			attrDelta2.addOperation( attrHistoryDelta.operations[ 1 ] );
+			doc.history.updateDelta( 2, [ attrDelta1, attrDelta2 ] );
+
+			undo._execute( batch1 );
+			// After this execution, undo algorithm should update both `attrDelta1` and `attrDelta2` with new
+			// versions, that have incremented offsets.
+			expect( getText( root ) ).to.equal( 'aBCdEf' );
+
+			undo._execute( batch2 );
+			// This execution checks whether undo algorithm correctly updated deltas in previous execution
+			// and also whether it correctly "reads" both deltas from history.
+			expect( getText( root ) ).to.equal( 'abcdef' );
+		} );
+
+		it( 'merges touching ranges when restoring selection', () => {
+			root.appendChildren( 'abcdef' );
+			expect( getText( root ) ).to.equal( 'abcdef' );
+
+			editor.document.selection.setRanges( [ r( 1, 4 ) ] );
+			let batch0 = doc.batch();
+			undo.addBatch( batch0 );
+			batch0.setAttr( 'uppercase', true, r( 1, 4 ) );
+			expect( getText( root ) ).to.equal( 'aBCDef' );
+
+			editor.document.selection.setRanges( [ r( 3, 4 ) ] );
+			let batch1 = doc.batch();
+			undo.addBatch( batch1 );
+			batch1.move( r( 3, 4 ), p( 1 ) );
+			expect( getText( root ) ).to.equal( 'aDBCef' );
+
+			undo._execute( batch0 );
+
+			// After undo-attr: acdbef <--- "cdb" should be selected, it would look weird if only "cd" or "b" is selected
+			// but the whole unbroken part "cdb" changed attribute.
+			expect( getText( root ) ).to.equal( 'adbcef' );
+			expect( editor.document.selection.getRanges().next().value.isEqual( r( 1, 4 ) ) ).to.be.true;
+		} );
+
+		it( 'does nothing (and not crashes) if delta to undo is no longer in history', () => {
+			// Also an edgy situation but it may come up if other plugins use `CompressedHistory` API.
+			root.appendChildren( 'abcdef' );
+			expect( getText( root ) ).to.equal( 'abcdef' );
+
+			editor.document.selection.setRanges( [ r( 0, 1 ) ] );
+			let batch0 = doc.batch();
+			undo.addBatch( batch0 );
+			batch0.setAttr( 'uppercase', true, r( 0, 1 ) );
+			expect( getText( root ) ).to.equal( 'Abcdef' );
 
-			undo.on( 'revert', spy );
+			doc.history.removeDelta( 0 );
+			root.getChild( 0 ).removeAttribute( 'uppercase' );
+			expect( getText( root ) ).to.equal( 'abcdef' );
 
 			undo._execute();
 
-			expect( spy.calledOnce ).to.be.true;
-			expect( spy.calledWith( batch ) );
+			// Nothing happened. We are still alive.
+			expect( getText( root ) ).to.equal( 'abcdef' );
 		} );
 	} );
 } );

+ 94 - 107
packages/ckeditor5-undo/tests/undoengine-integration.js

@@ -12,7 +12,7 @@ import UndoEngine from '/ckeditor5/undo/undoengine.js';
 
 import { setData, getData } from '/tests/engine/_utils/model.js';
 
-// import deleteContents from '/ckeditor5/engine/model/composer/deletecontents.js';
+import deleteContents from '/ckeditor5/engine/model/composer/deletecontents.js';
 
 let editor, doc, root;
 
@@ -115,12 +115,12 @@ describe( 'UndoEngine integration', () => {
 			output( '<p>o</p><p>b<selection /></p>' );
 
 			editor.execute( 'undo' );
-			// Here is an edge case that selection could be before or after `ar` but selection always ends up after.
-			output( '<p>o</p><p>bar<selection /></p>' );
+			// Here is an edge case that selection could be before or after `ar`.
+			output( '<p>o</p><p>b<selection />ar</p>' );
 
 			editor.execute( 'undo' );
 			// As above.
-			output( '<p>fo<selection />o</p><p>bar</p>' );
+			output( '<p><selection />foo</p><p>bar</p>' );
 
 			undoDisabled();
 		} );
@@ -144,45 +144,43 @@ describe( 'UndoEngine integration', () => {
 			undoDisabled();
 		} );
 
-		//it( 'add and remove same part and undo', () => {
-		//	// This test case fails because some operations are transformed to NoOperations incorrectly.
-		//	input( '<p>fo<selection />o</p><p>bar</p>' );
-		//
-		//	doc.batch().insert( doc.selection.getFirstPosition(), 'zzz' );
-		//	output( '<p>fozzz<selection />o</p><p>bar</p>' );
-		//
-		//	doc.batch().remove( Range.createFromPositionAndShift( new Position( root, [ 0, 2 ] ) , 3 ) );
-		//	output( '<p>fo<selection />o</p><p>bar</p>' );
-		//
-		//	editor.execute( 'undo' );
-		//	output( '<p>fozzz<selection />o</p><p>bar</p>' );
-		//
-		//	editor.execute( 'undo' );
-		//	output( '<p>fo<selection />o</p><p>bar</p>' );
-		//
-		//	undoDisabled();
-		//} );
+		it( 'add and remove same part and undo', () => {
+			input( '<p>fo<selection />o</p><p>bar</p>' );
+
+			doc.batch().insert( doc.selection.getFirstPosition(), 'zzz' );
+			output( '<p>fozzz<selection />o</p><p>bar</p>' );
+
+			doc.batch().remove( Range.createFromPositionAndShift( new Position( root, [ 0, 2 ] ) , 3 ) );
+			output( '<p>fo<selection />o</p><p>bar</p>' );
+
+			editor.execute( 'undo' );
+			output( '<p>fozzz<selection />o</p><p>bar</p>' );
+
+			editor.execute( 'undo' );
+			output( '<p>fo<selection />o</p><p>bar</p>' );
+
+			undoDisabled();
+		} );
 	} );
 
 	describe( 'moving', () => {
-		//it( 'move same content twice then undo', () => {
-		//	// This test case fails because some operations are transformed to NoOperations incorrectly.
-		//	input( '<p>f<selection>o</selection>z</p><p>bar</p>' );
-		//
-		//	doc.batch().move( doc.selection.getFirstRange(), new Position( root, [ 1, 0 ] ) );
-		//	output( '<p>fz</p><p><selection>o</selection>bar</p>' );
-		//
-		//	doc.batch().move( doc.selection.getFirstRange(), new Position( root, [ 0, 2 ] ) );
-		//	output( '<p>fz<selection>o</selection></p><p>bar</p>' );
-		//
-		//	editor.execute( 'undo' );
-		//	output( '<p>fz</p><p><selection>o</selection>bar</p>' );
-		//
-		//	editor.execute( 'undo' );
-		//	output( '<p>f<selection>o</selection>z</p><p>bar</p>' );
-		//
-		//	undoDisabled();
-		//} );
+		it( 'move same content twice then undo', () => {
+			input( '<p>f<selection>o</selection>z</p><p>bar</p>' );
+
+			doc.batch().move( doc.selection.getFirstRange(), new Position( root, [ 1, 0 ] ) );
+			output( '<p>fz</p><p><selection>o</selection>bar</p>' );
+
+			doc.batch().move( doc.selection.getFirstRange(), new Position( root, [ 0, 2 ] ) );
+			output( '<p>fz<selection>o</selection></p><p>bar</p>' );
+
+			editor.execute( 'undo' );
+			output( '<p>fz</p><p><selection>o</selection>bar</p>' );
+
+			editor.execute( 'undo' );
+			output( '<p>f<selection>o</selection>z</p><p>bar</p>' );
+
+			undoDisabled();
+		} );
 
 		it( 'move content and new parent then undo', () => {
 			input( '<p>f<selection>o</selection>z</p><p>bar</p>' );
@@ -230,7 +228,7 @@ describe( 'UndoEngine integration', () => {
 			input( 'fo<selection>zb</selection>ar' );
 
 			doc.batch().wrap( doc.selection.getFirstRange(), 'p' );
-			output( 'fo<p><selection>zb</selection></p>ar' );
+			output( 'fo<selection><p>zb</p></selection>ar' );
 
 			editor.execute( 'undo' );
 			output( 'fo<selection>zb</selection>ar' );
@@ -238,26 +236,25 @@ describe( 'UndoEngine integration', () => {
 			undoDisabled();
 		} );
 
-		//it( 'wrap, move and undo', () => {
-		//	input( 'fo<selection>zb</selection>ar' );
-		//
-		//	doc.batch().wrap( doc.selection.getFirstRange(), 'p' );
-		//	output( 'fo<p><selection>zb</selection></p>ar' );
-		//
-		//	setSelection( [ 2, 0 ], [ 2, 1 ] );
-		//	doc.batch().move( doc.selection.getFirstRange(), new Position( root, [ 0 ] ) );
-		//	output( '<selection>z</selection>fo<p>b</p>ar' );
-		//
-		//	editor.execute( 'undo' );
-		//	output( 'fo<p><selection>z</selection>b</p>ar' );
-		//
-		//	// This test case fails here for unknown reason, but "z" letter magically disappears.
-		//	// AssertionError: expected 'fo<selection>b</selection>ar' to equal 'fo<selection>zb</selection>ar'
-		//	editor.execute( 'undo' );
-		//	output( 'fo<selection>zb</selection>ar' );
-		//
-		//	undoDisabled();
-		//} );
+		it( 'wrap, move and undo', () => {
+			input( 'fo<selection>zb</selection>ar' );
+
+			doc.batch().wrap( doc.selection.getFirstRange(), 'p' );
+			// Would be better if selection was inside P.
+			output( 'fo<selection><p>zb</p></selection>ar' );
+
+			setSelection( [ 2, 0 ], [ 2, 1 ] );
+			doc.batch().move( doc.selection.getFirstRange(), new Position( root, [ 0 ] ) );
+			output( '<selection>z</selection>fo<p>b</p>ar' );
+
+			editor.execute( 'undo' );
+			output( 'fo<p><selection>z</selection>b</p>ar' );
+
+			editor.execute( 'undo' );
+			output( 'fo<selection>zb</selection>ar' );
+
+			undoDisabled();
+		} );
 
 		it( 'unwrap and undo', () => {
 			input( '<p>foo<selection />bar</p>' );
@@ -271,54 +268,44 @@ describe( 'UndoEngine integration', () => {
 			undoDisabled();
 		} );
 
-		//it( 'merge and undo', () => {
-		//	input( '<p>foo</p><p><selection />bar</p>' );
-		//
-		//	doc.batch().merge( new Position( root, [ 1 ] ) );
-		//	// This test fails here because selection is stuck with <p> element and ends up in graveyard.
-		//	// AssertionError: expected '<p>foobar</p>' to equal '<p>foo<selection />bar</p>'
-		//	output( '<p>foo<selection />bar</p>' );
-		//
-		//	editor.execute( 'undo' );
-		//	// This test fails because when selection is transformed it is first in empty <p> but when
-		//	// "bar" is inserted, it gets moved to the right.
-		//	// AssertionError: expected '<p>foo</p><p>bar<selection /></p>' to equal '<p>foo</p><p><selection />bar</p>'
-		//	output( '<p>foo</p><p><selection />bar</p>' );
-		//
-		//	undoDisabled();
-		//} );
-
-		//it( 'split and undo', () => {
-		//	input( '<p>foo<selection />bar</p>' );
-		//
-		//	doc.batch().split( doc.selection.getFirstPosition() );
-		//	// This test fails because selection ends up in wrong node after splitting.
-		//	// AssertionError: expected '<p>foo<selection /></p><p>bar</p>' to equal '<p>foo</p><p><selection />bar</p>'
-		//	output( '<p>foo</p><p><selection />bar</p>' );
-		//
-		//	editor.execute( 'undo' );
-		//	// This test fails because selection after transforming ends up after inserted test.
-		//	// AssertionError: expected '<p>foobar<selection /></p>' to equal '<p>foo<selection />bar</p>'
-		//	output( '<p>foo<selection />bar</p>' );
-		//
-		//	undoDisabled();
-		//} );
+		it( 'merge and undo', () => {
+			input( '<p>foo</p><p><selection />bar</p>' );
+
+			doc.batch().merge( new Position( root, [ 1 ] ) );
+			// Because selection is stuck with <p> it ends up in graveyard. We have to manually move it to correct node.
+			setSelection( [ 0, 3 ], [ 0, 3 ] );
+			output( '<p>foo<selection />bar</p>' );
+
+			editor.execute( 'undo' );
+			output( '<p>foo</p><p><selection />bar</p>' );
+
+			undoDisabled();
+		} );
+
+		it( 'split and undo', () => {
+			input( '<p>foo<selection />bar</p>' );
+
+			doc.batch().split( doc.selection.getFirstPosition() );
+			// Because selection is stuck with <p> it ends up in wrong node. We have to manually move it to correct node.
+			setSelection( [ 1, 0 ], [ 1, 0 ] );
+			output( '<p>foo</p><p><selection />bar</p>' );
+
+			editor.execute( 'undo' );
+			output( '<p>foo<selection />bar</p>' );
+
+			undoDisabled();
+		} );
 	} );
 
 	describe( 'other edge cases', () => {
-		//it( 'deleteContents between two nodes', () => {
-		//	input( '<p>fo<selection>o</p><p>b</selection>ar</p>' );
-		//
-		//	deleteContents( doc.batch(), doc.selection, { merge: true } );
-		//	output( '<p>fo<selection />ar</p>' );
-		//
-		//	// This test case fails because of OT problems.
-		//	// When the batch is undone, first MergeDelta is reversed to SplitDelta and it is undone.
-		//	// Then RemoveOperations are reversed to ReinsertOperation.
-		//	// Unfortunately, ReinsertOperation that inserts "o" points to the same position were split happened.
-		//	// Then, when ReinsertOperation is transformed by operations of SplitDelta, it ends up in wrong <p>.
-		//	editor.execute( 'undo' );
-		//	output( '<p>fo<selection>o</p><p>b</selection>ar</p>' );
-		//} );
+		it( 'deleteContents between two nodes', () => {
+			input( '<p>fo<selection>o</p><p>b</selection>ar</p>' );
+
+			deleteContents( doc.batch(), doc.selection, { merge: true } );
+			output( '<p>fo<selection />ar</p>' );
+
+			editor.execute( 'undo' );
+			output( '<p>fo<selection>o</p><p>b</selection>ar</p>' );
+		} );
 	} );
 } );

+ 20 - 19
packages/ckeditor5-undo/tests/undoengine.js

@@ -32,47 +32,48 @@ describe( 'UndoEngine', () => {
 		expect( editor.commands.get( 'redo' ) ).to.equal( undo._redoCommand );
 	} );
 
-	it( 'should add a batch to undo command whenever a new batch is applied to the document', () => {
+	it( 'should add a batch to undo command and clear redo stack, if it\'s type is "default"', () => {
 		sinon.spy( undo._undoCommand, 'addBatch' );
+		sinon.spy( undo._redoCommand, 'clearStack' );
 
 		expect( undo._undoCommand.addBatch.called ).to.be.false;
+		expect( undo._redoCommand.clearStack.called ).to.be.false;
 
 		batch.insert( new Position( root, [ 0 ] ), 'foobar' );
 
 		expect( undo._undoCommand.addBatch.calledOnce ).to.be.true;
-
-		batch.insert( new Position( root, [ 0 ] ), 'foobar' );
-
-		expect( undo._undoCommand.addBatch.calledOnce ).to.be.true;
+		expect( undo._redoCommand.clearStack.calledOnce ).to.be.true;
 	} );
 
-	it( 'should add a batch to redo command whenever a batch is undone by undo command', () => {
-		batch.insert( new Position( root, [ 0 ] ), 'foobar' );
-
-		sinon.spy( undo._redoCommand, 'addBatch' );
+	it( 'should add each batch only once', () => {
+		sinon.spy( undo._undoCommand, 'addBatch' );
 
-		undo._undoCommand.fire( 'revert', batch );
+		batch.insert( new Position( root, [ 0 ] ), 'foobar' ).insert( new Position( root, [ 0 ] ), 'foobar' );
 
-		expect( undo._redoCommand.addBatch.calledOnce ).to.be.true;
-		expect( undo._redoCommand.addBatch.calledWith( batch ) ).to.be.true;
+		expect( undo._undoCommand.addBatch.calledOnce ).to.be.true;
 	} );
 
-	it( 'should add a batch to undo command whenever a batch is redone by redo command', () => {
-		batch.insert( new Position( root, [ 0 ] ), 'foobar' );
-
+	it( 'should add a batch to undo command, if it\'s type is undo and it comes from redo command', () => {
 		sinon.spy( undo._undoCommand, 'addBatch' );
+		sinon.spy( undo._redoCommand, 'clearStack' );
+
+		undo._redoCommand._createdBatches.add( batch );
 
-		undo._redoCommand.fire( 'revert', batch );
+		batch.insert( new Position( root, [ 0 ] ), 'foobar' );
 
 		expect( undo._undoCommand.addBatch.calledOnce ).to.be.true;
-		expect( undo._undoCommand.addBatch.calledWith( batch ) ).to.be.true;
+		expect( undo._redoCommand.clearStack.called ).to.be.false;
 	} );
 
-	it( 'should clear redo command stack whenever a new batch is applied to the document', () => {
+	it( 'should add a batch to redo command, if it\'s type is undo', () => {
+		sinon.spy( undo._redoCommand, 'addBatch' );
 		sinon.spy( undo._redoCommand, 'clearStack' );
 
+		undo._undoCommand._createdBatches.add( batch );
+
 		batch.insert( new Position( root, [ 0 ] ), 'foobar' );
 
-		expect( undo._redoCommand.clearStack.calledOnce ).to.be.true;
+		expect( undo._redoCommand.addBatch.calledOnce ).to.be.true;
+		expect( undo._redoCommand.clearStack.called ).to.be.false;
 	} );
 } );