Merge pull request #85 from ckeditor/t/84

Fix: `UndoCommand` and `RedoCommand` should pass batch in `model.Model#enqueueChange` call. Closes #84.

src/basecommand.js

@@ -8,7 +8,6 @@
  */
 
 import Command from '@ckeditor/ckeditor5-core/src/command';
-import Batch from '@ckeditor/ckeditor5-engine/src/model/batch';
 
 /**
  * Base class for undo feature commands: {@link module:undo/undocommand~UndoCommand} and {@link module:undo/redocommand~RedoCommand}.
@@ -123,13 +122,13 @@ export default class BaseCommand extends Command {
 	 *
 	 * @protected
 	 * @param {module:engine/model/batch~Batch} batchToUndo The batch to be undone.
+	 * @param {module:engine/model/batch~Batch} undoingBatch The batch that will contain undoing changes.
 	 */
-	_undo( batchToUndo ) {
+	_undo( batchToUndo, undoingBatch ) {
 		const model = this.editor.model;
 		const document = model.document;
 
 		// All changes done by the command execution will be saved as one batch.
-		const undoingBatch = new Batch();
 		this._createdBatches.add( undoingBatch );
 
 		const deltasToUndo = batchToUndo.deltas.slice();
@@ -168,8 +167,6 @@ export default class BaseCommand extends Command {
 				}
 			}
 		}
-
-		return undoingBatch;
 	}
 }
 

src/redocommand.js

@@ -8,6 +8,7 @@
  */
 
 import BaseCommand from './basecommand';
+import Batch from '@ckeditor/ckeditor5-engine/src/model/batch';
 
 /**
  * The redo command stores {@link module:engine/model/batch~Batch batches} that were used to undo a batch by
@@ -30,16 +31,17 @@ export default class RedoCommand extends BaseCommand {
 	 */
 	execute() {
 		const item = this._stack.pop();
+		const redoingBatch = new Batch();
 
 		// All changes have to be done in one `enqueueChange` 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.model.enqueueChange( () => {
+		this.editor.model.enqueueChange( redoingBatch, () => {
 			const lastDelta = item.batch.deltas[ item.batch.deltas.length - 1 ];
 			const nextBaseVersion = lastDelta.baseVersion + lastDelta.operations.length;
 			const deltas = this.editor.model.document.history.getDeltas( nextBaseVersion );
 
 			this._restoreSelection( item.selection.ranges, item.selection.isBackward, deltas );
-			this._undo( item.batch );
+			this._undo( item.batch, redoingBatch );
 		} );
 
 		this.refresh();

src/undocommand.js

@@ -8,6 +8,7 @@
  */
 
 import BaseCommand from './basecommand';
+import Batch from '@ckeditor/ckeditor5-engine/src/model/batch';
 
 /**
  * The undo command stores {@link module:engine/model/batch~Batch batches} applied to the
@@ -33,11 +34,12 @@ export default class UndoCommand extends BaseCommand {
 		const batchIndex = batch ? this._stack.findIndex( a => a.batch == batch ) : this._stack.length - 1;
 
 		const item = this._stack.splice( batchIndex, 1 )[ 0 ];
+		const undoingBatch = new Batch();
 
 		// All changes has to be done in one `enqueueChange` 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.model.enqueueChange( () => {
-			const undoingBatch = this._undo( item.batch );
+		this.editor.model.enqueueChange( undoingBatch, () => {
+			this._undo( item.batch, undoingBatch );
 
 			const deltas = this.editor.model.document.history.getDeltas( item.batch.baseVersion );
 			this._restoreSelection( item.selection.ranges, item.selection.isBackward, deltas );

tests/redocommand.js

@@ -270,6 +270,23 @@ describe( 'RedoCommand', () => {
 				expect( editor.model.document.selection.getFirstRange().isEqual( r( 4, 6 ) ) ).to.be.true;
 				expect( editor.model.document.selection.isBackward ).to.be.false;
 			} );
+
+			it( 'should pass redoing batch to enqueueChange method', () => {
+				undo.execute( batch2 );
+
+				const enqueueChangeSpy = sinon.spy( model, 'enqueueChange' );
+				const undoSpy = sinon.spy( redo, '_undo' );
+
+				redo.execute();
+
+				sinon.assert.calledOnce( enqueueChangeSpy );
+				sinon.assert.calledOnce( undoSpy );
+
+				const redoingBatch = enqueueChangeSpy.firstCall.args[ 0 ];
+
+				expect( redoingBatch instanceof Batch ).to.be.true;
+				expect( undoSpy.firstCall.args[ 1 ] ).to.equal( redoingBatch );
+			} );
 		} );
 	} );
 } );

tests/undocommand.js

@@ -297,6 +297,21 @@ describe( 'UndoCommand', () => {
 				expect( element.getAttribute( 'foo' ) ).to.equal( 'bar' );
 				expect( root.getAttribute( 'foo' ) ).to.not.equal( 'bar' );
 			} );
+
+			it( 'should pass undoing batch to enqueueChange method', () => {
+				const enqueueChangeSpy = sinon.spy( model, 'enqueueChange' );
+				const undoSpy = sinon.spy( undo, '_undo' );
+
+				undo.execute();
+
+				sinon.assert.calledOnce( enqueueChangeSpy );
+				sinon.assert.calledOnce( undoSpy );
+
+				const undoingBatch = enqueueChangeSpy.firstCall.args[ 0 ];
+
+				expect( undoingBatch instanceof Batch ).to.be.true;
+				expect( undoSpy.firstCall.args[ 1 ] ).to.equal( undoingBatch );
+			} );
 		} );
 
 		it( 'merges touching ranges when restoring selection', () => {

tests/undoediting-integration.js

@@ -48,6 +48,10 @@ describe( 'UndoEditing integration', () => {
 			} );
 	} );
 
+	afterEach( () => {
+		return editor.destroy();
+	} );
+
 	function setSelection( pathA, pathB ) {
 		model.change( writer => {
 			writer.setSelection( new Range( new Position( root, pathA ), new Position( root, pathB ) ) );
@@ -1044,6 +1048,44 @@ describe( 'UndoEditing integration', () => {
 		} );
 	} );
 
+	it( 'postfixers should not add another undo step when fixing undo changes', () => {
+		input( '<paragraph>[]</paragraph>' );
+		const paragraph = root.getChild( 0 );
+
+		// 1. Step - add text and marker to the paragraph.
+		model.change( writer => {
+			writer.appendText( 'foo', paragraph );
+			writer.setMarker( 'marker', Range.createIn( paragraph ), { usingOperation: true } );
+		} );
+
+		// 2. Step - remove text from paragraph.
+		model.change( writer => {
+			writer.remove( Range.createIn( paragraph ) );
+		} );
+
+		// Register post fixer to remove marker from non-empty paragraph.
+		model.document.registerPostFixer( writer => {
+			const hasMarker = model.markers.has( 'marker' );
+			const isEmpty = paragraph.childCount === 0;
+
+			// Remove marker when paragraph is empty.
+			if ( hasMarker && !isEmpty ) {
+				writer.removeMarker( 'marker' );
+
+				return true;
+			}
+
+			return false;
+		} );
+
+		editor.execute( 'undo' );
+
+		// Check if postfixer changes are executed together with undo and there is no additional undo steps.
+		expect( model.markers.has( 'marker' ) ).to.be.false;
+		expect( editor.commands.get( 'undo' )._stack.length ).to.equal( 1 );
+		output( '<paragraph>foo[]</paragraph>' );
+	} );
+
 	function split( path ) {
 		setSelection( path.slice(), path.slice() );
 		editor.execute( 'enter' );