Merge pull request #80 from ckeditor/t/21

Fix: New undo step should be created on selection change or applying an attribute. Closes #20. Closes #21.

src/changebuffer.js

@@ -60,12 +60,28 @@ export default class ChangeBuffer {
 		 */
 		this.limit = limit;
 
+		/**
+		 * Whether the buffer is locked. The locked buffer cannot be reset unless it gets unlocked.
+		 *
+		 * @readonly
+		 * @member {Boolean} #isLocked
+		 */
+		this.isLocked = false;
+
 		this._changeCallback = ( evt, type, changes, batch ) => {
 			this._onBatch( batch );
 		};
 
+		this._selectionChangeCallback = () => {
+			this._reset();
+		};
+
 		doc.on( 'change', this._changeCallback );
 
+		doc.selection.on( 'change:range', this._selectionChangeCallback );
+
+		doc.selection.on( 'change:attribute', this._selectionChangeCallback );
+
 		/**
 		 * The current batch instance.
 		 *
@@ -79,13 +95,20 @@ export default class ChangeBuffer {
 		 * @private
 		 * @member #_changeCallback
 		 */
+
+		/**
+		 * The callback to document selection change:attribute and change:range events which resets the buffer.
+		 *
+		 * @private
+		 * @member #_selectionChangeCallback
+		 */
 	}
 
 	/**
 	 * The current batch to which a feature should add its deltas. Once the {@link #size}
 	 * is reached or exceeds the {@link #limit}, the batch is set to a new instance and the size is reset.
 	 *
-	 * @type {engine.treeModel.batch.Batch}
+	 * @type {module:engine/model/batch~Batch}
 	 */
 	get batch() {
 		if ( !this._batch ) {
@@ -105,15 +128,31 @@ export default class ChangeBuffer {
 		this.size += changeCount;
 
 		if ( this.size >= this.limit ) {
-			this._reset();
+			this._reset( true );
 		}
 	}
 
+	/**
+	 * Locks the buffer.
+	 */
+	lock() {
+		this.isLocked = true;
+	}
+
+	/**
+	 * Unlocks the buffer.
+	 */
+	unlock() {
+		this.isLocked = false;
+	}
+
 	/**
 	 * Destroys the buffer.
 	 */
 	destroy() {
 		this.document.off( 'change', this._changeCallback );
+		this.document.selection.off( 'change:range', this._selectionChangeCallback );
+		this.document.selection.off( 'change:attribute', this._selectionChangeCallback );
 	}
 
 	/**
@@ -130,17 +169,20 @@ export default class ChangeBuffer {
 	_onBatch( batch ) {
 		// One operation means a newly created batch.
 		if ( batch.type != 'transparent' && batch !== this._batch && count( batch.getOperations() ) <= 1 ) {
-			this._reset();
+			this._reset( true );
 		}
 	}
 
 	/**
 	 * Resets the change buffer.
 	 *
 	 * @private
+	 * @param {Boolean} [ignoreLock] Whether internal lock {@link #isLocked} should be ignored.
 	 */
-	_reset() {
-		this._batch = null;
-		this.size = 0;
+	_reset( ignoreLock ) {
+		if ( !this.isLocked || ignoreLock ) {
+			this._batch = null;
+			this.size = 0;
+		}
 	}
 }

src/deletecommand.js

@@ -61,6 +61,8 @@ export default class DeleteCommand extends Command {
 		const dataController = this.editor.data;
 
 		doc.enqueueChanges( () => {
+			this._buffer.lock();
+
 			const selection = Selection.createFromSelection( doc.selection );
 
 			// Try to extend the selection in the specified direction.
@@ -85,6 +87,8 @@ export default class DeleteCommand extends Command {
 			this._buffer.input( changeCount );
 
 			doc.selection.setRanges( selection.getRanges(), selection.isBackward );
+
+			this._buffer.unlock();
 		} );
 	}
 }

src/input.js

@@ -66,9 +66,13 @@ export default class Input extends Plugin {
 			return;
 		}
 
+		buffer.lock();
+
 		doc.enqueueChanges( () => {
 			this.editor.data.deleteContent( doc.selection, buffer.batch );
 		} );
+
+		buffer.unlock();
 	}
 
 	/**

src/inputcommand.js

@@ -78,6 +78,8 @@ export default class InputCommand extends Command {
 		doc.enqueueChanges( () => {
 			const isCollapsedRange = range.isCollapsed;
 
+			this._buffer.lock();
+
 			if ( !isCollapsedRange ) {
 				this._buffer.batch.remove( range );
 			}
@@ -91,6 +93,8 @@ export default class InputCommand extends Command {
 				this.editor.data.model.selection.collapse( range.start.getShiftedBy( textInsertions ) );
 			}
 
+			this._buffer.unlock();
+
 			this._buffer.input( textInsertions );
 		} );
 	}

tests/changebuffer.js

@@ -23,6 +23,7 @@ describe( 'ChangeBuffer', () => {
 			expect( buffer ).to.have.property( 'document', doc );
 			expect( buffer ).to.have.property( 'limit', CHANGE_LIMIT );
 			expect( buffer ).to.have.property( 'size', 0 );
+			expect( buffer ).to.have.property( 'isLocked', false );
 		} );
 
 		it( 'sets limit property according to default value', () => {
@@ -32,6 +33,26 @@ describe( 'ChangeBuffer', () => {
 		} );
 	} );
 
+	describe( 'locking', () => {
+		it( 'is unlocked by default', () => {
+			expect( buffer.isLocked ).to.be.false;
+		} );
+
+		it( 'is locked by lock method', () => {
+			buffer.lock();
+
+			expect( buffer.isLocked ).to.be.true;
+		} );
+
+		it( 'is unlocked by unlock method', () => {
+			buffer.isLocked = true;
+
+			buffer.unlock();
+
+			expect( buffer.isLocked ).to.be.false;
+		} );
+	} );
+
 	describe( 'batch', () => {
 		it( 'it is set initially', () => {
 			expect( buffer ).to.have.property( 'batch' );
@@ -107,6 +128,106 @@ describe( 'ChangeBuffer', () => {
 
 			expect( buffer.batch ).to.equal( bufferBatch );
 		} );
+
+		it( 'is not reset while locked', () => {
+			const initialBatch = buffer.batch;
+
+			buffer.lock();
+
+			buffer.input( 1 );
+			buffer._reset();
+
+			buffer.unlock();
+
+			expect( buffer.batch ).to.be.equal( initialBatch );
+			expect( buffer.size ).to.equal( 1 );
+		} );
+
+		it( 'is reset while locked with ignoreLock used', () => {
+			const initialBatch = buffer.batch;
+
+			buffer.lock();
+
+			buffer.input( 1 );
+			buffer._reset( true );
+
+			buffer.unlock();
+
+			expect( buffer.batch ).to.not.equal( initialBatch );
+			expect( buffer.size ).to.equal( 0 );
+		} );
+
+		it( 'is reset while locked and limit exceeded', () => {
+			const initialBatch = buffer.batch;
+
+			buffer.lock();
+
+			buffer.input( CHANGE_LIMIT + 1 );
+
+			buffer.unlock();
+
+			expect( buffer.batch ).to.not.equal( initialBatch );
+			expect( buffer.size ).to.equal( 0 );
+		} );
+
+		it( 'is reset while locked and new batch is applied', () => {
+			const initialBatch = buffer.batch;
+
+			buffer.lock();
+
+			doc.batch().insert( Position.createAt( root, 0 ), 'a' );
+
+			buffer.unlock();
+
+			expect( buffer.batch ).to.not.equal( initialBatch );
+			expect( buffer.size ).to.equal( 0 );
+		} );
+
+		it( 'is reset on selection change:range', () => {
+			const initialBatch = buffer.batch;
+
+			doc.selection.fire( 'change:range' );
+
+			expect( buffer.batch ).to.not.equal( initialBatch );
+			expect( buffer.size ).to.equal( 0 );
+		} );
+
+		it( 'is reset on selection change:attribute', () => {
+			const initialBatch = buffer.batch;
+
+			doc.selection.fire( 'change:attribute' );
+
+			expect( buffer.batch ).to.not.equal( initialBatch );
+			expect( buffer.size ).to.equal( 0 );
+		} );
+
+		it( 'is not reset on selection change:range while locked', () => {
+			const initialBatch = buffer.batch;
+			buffer.size = 1;
+
+			buffer.lock();
+
+			doc.selection.fire( 'change:range' );
+
+			buffer.unlock();
+
+			expect( buffer.batch ).to.be.equal( initialBatch );
+			expect( buffer.size ).to.equal( 1 );
+		} );
+
+		it( 'is not reset on selection change:attribute while locked', () => {
+			const initialBatch = buffer.batch;
+			buffer.size = 1;
+
+			buffer.lock();
+
+			doc.selection.fire( 'change:attribute' );
+
+			buffer.unlock();
+
+			expect( buffer.batch ).to.be.equal( initialBatch );
+			expect( buffer.size ).to.equal( 1 );
+		} );
 	} );
 
 	describe( 'destroy', () => {
@@ -119,5 +240,25 @@ describe( 'ChangeBuffer', () => {
 
 			expect( buffer.batch ).to.equal( batch1 );
 		} );
+
+		it( 'offs the buffer from the selection change:range', () => {
+			const batch1 = buffer.batch;
+
+			buffer.destroy();
+
+			doc.selection.fire( 'change:attribute' );
+
+			expect( buffer.batch ).to.equal( batch1 );
+		} );
+
+		it( 'offs the buffer from the selection change:attribute', () => {
+			const batch1 = buffer.batch;
+
+			buffer.destroy();
+
+			doc.selection.fire( 'change:range' );
+
+			expect( buffer.batch ).to.equal( batch1 );
+		} );
 	} );
 } );

tests/deletecommand.js

@@ -6,10 +6,13 @@
 import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltesteditor';
 import DeleteCommand from '../src/deletecommand';
 import { getData, setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
+import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
 
 describe( 'DeleteCommand', () => {
 	let editor, doc;
 
+	testUtils.createSinonSandbox();
+
 	beforeEach( () => {
 		return ModelTestEditor.create( )
 			.then( newEditor => {
@@ -33,13 +36,26 @@ describe( 'DeleteCommand', () => {
 		it( 'uses enqueueChanges', () => {
 			setData( doc, '<p>foo[]bar</p>' );
 
-			const spy = sinon.spy( doc, 'enqueueChanges' );
+			const spy = testUtils.sinon.spy( doc, 'enqueueChanges' );
 
 			editor.execute( 'delete' );
 
 			expect( spy.calledOnce ).to.be.true;
 		} );
 
+		it( 'locks buffer when executing', () => {
+			setData( doc, '<p>foo[]bar</p>' );
+
+			const buffer = editor.commands.get( 'delete' )._buffer;
+			const lockSpy = testUtils.sinon.spy( buffer, 'lock' );
+			const unlockSpy = testUtils.sinon.spy( buffer, 'unlock' );
+
+			editor.execute( 'delete' );
+
+			expect( lockSpy.calledOnce ).to.be.true;
+			expect( unlockSpy.calledOnce ).to.be.true;
+		} );
+
 		it( 'deletes previous character when selection is collapsed', () => {
 			setData( doc, '<p>foo[]bar</p>' );