Merge pull request #54 from ckeditor/t/53

Fix: Fixed integration with undo. Closes #53.

Author: scofalik

src/blockautoformatediting.js

@@ -64,34 +64,35 @@ export default class BlockAutoformatEditing {
 		}
 
 		editor.model.document.on( 'change', () => {
-			const changes = editor.model.document.differ.getChanges();
+			const changes = Array.from( editor.model.document.differ.getChanges() );
+			const entry = changes[ 0 ];
 
-			for ( const entry of changes ) {
-				if ( entry.type == 'insert' && entry.name == '$text' ) {
-					const item = entry.position.textNode || entry.position.nodeAfter;
+			// Typing is represented by only a single change.
+			if ( changes.length != 1 || entry.type !== 'insert' || entry.name != '$text' || entry.length != 1 ) {
+				return;
+			}
+			const item = entry.position.textNode || entry.position.nodeAfter;
 
-					if ( !item.parent.is( 'paragraph' ) ) {
-						continue;
-					}
+			if ( !item.parent.is( 'paragraph' ) ) {
+				return;
+			}
 
-					const match = pattern.exec( item.data );
+			const match = pattern.exec( item.data );
 
-					if ( !match ) {
-						continue;
-					}
+			if ( !match ) {
+				return;
+			}
 
-					// Use enqueueChange to create new batch to separate typing batch from the auto-format changes.
-					editor.model.enqueueChange( writer => {
-						// Matched range.
-						const range = Range.createFromParentsAndOffsets( item.parent, 0, item.parent, match[ 0 ].length );
+			// Use enqueueChange to create new batch to separate typing batch from the auto-format changes.
+			editor.model.enqueueChange( writer => {
+				// Matched range.
+				const range = Range.createFromParentsAndOffsets( item.parent, 0, item.parent, match[ 0 ].length );
 
-						// Remove matched text.
-						writer.remove( range );
+				// Remove matched text.
+				writer.remove( range );
 
-						callback( { match } );
-					} );
-				}
-			}
+				callback( { match } );
+			} );
 		} );
 	}
 }

src/inlineautoformatediting.js

@@ -7,7 +7,7 @@
  * @module autoformat/inlineautoformatediting
  */
 
-import LiveRange from '@ckeditor/ckeditor5-engine/src/model/liverange';
+import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range';
 
 /**
  * The inline autoformatting engine. It allows to format various inline patterns. For example,
@@ -141,74 +141,43 @@ export default class InlineAutoformatEditing {
 		} );
 
 		editor.model.document.on( 'change', () => {
-			const changes = editor.model.document.differ.getChanges();
+			const selection = editor.model.document.selection;
 
-			for ( const entry of changes ) {
-				if ( entry.type != 'insert' || entry.name != '$text' ) {
-					continue;
-				}
-
-				const selection = editor.model.document.selection;
+			// Do nothing if selection is not collapsed.
+			if ( !selection.isCollapsed ) {
+				return;
+			}
 
-				if ( !selection.isCollapsed || !selection.focus || !selection.focus.parent ) {
-					continue;
-				}
+			const changes = Array.from( editor.model.document.differ.getChanges() );
+			const entry = changes[ 0 ];
 
-				const block = selection.focus.parent;
-				const text = getText( block ).slice( 0, selection.focus.offset );
-				const ranges = testCallback( text );
-				const rangesToFormat = [];
-
-				// Apply format before deleting text.
-				ranges.format.forEach( range => {
-					if ( range[ 0 ] === undefined || range[ 1 ] === undefined ) {
-						return;
-					}
-
-					rangesToFormat.push( LiveRange.createFromParentsAndOffsets(
-						block, range[ 0 ],
-						block, range[ 1 ]
-					) );
-				} );
-
-				const rangesToRemove = [];
-
-				// Reverse order to not mix the offsets while removing.
-				ranges.remove.slice().reverse().forEach( range => {
-					if ( range[ 0 ] === undefined || range[ 1 ] === undefined ) {
-						return;
-					}
-
-					rangesToRemove.push( LiveRange.createFromParentsAndOffsets(
-						block, range[ 0 ],
-						block, range[ 1 ]
-					) );
-				} );
-
-				if ( !( rangesToFormat.length && rangesToRemove.length ) ) {
-					continue;
-				}
+			// Typing is represented by only a single change.
+			if ( changes.length != 1 || entry.type !== 'insert' || entry.name != '$text' || entry.length != 1 ) {
+				return;
+			}
 
-				// Use enqueueChange to create new batch to separate typing batch from the auto-format changes.
-				editor.model.enqueueChange( writer => {
-					const validRanges = editor.model.schema.getValidRanges( rangesToFormat, attributeKey );
+			const block = selection.focus.parent;
+			const text = getText( block ).slice( 0, selection.focus.offset );
+			const testOutput = testCallback( text );
+			const rangesToFormat = testOutputToRanges( block, testOutput.format );
+			const rangesToRemove = testOutputToRanges( block, testOutput.remove );
 
-					// Apply format.
-					formatCallback( writer, validRanges );
+			if ( !( rangesToFormat.length && rangesToRemove.length ) ) {
+				return;
+			}
 
-					// Detach ranges used to apply Autoformat. Prevents memory leaks. #39
-					rangesToFormat.forEach( range => range.detach() );
+			// Use enqueueChange to create new batch to separate typing batch from the auto-format changes.
+			editor.model.enqueueChange( writer => {
+				const validRanges = editor.model.schema.getValidRanges( rangesToFormat, attributeKey );
 
-					// Remove delimiters.
-					for ( const range of rangesToRemove ) {
-						writer.remove( range );
+				// Apply format.
+				formatCallback( writer, validRanges );
 
-						// Prevents memory leaks.
-						// https://github.com/ckeditor/ckeditor5-autoformat/issues/39
-						range.detach();
-					}
-				} );
-			}
+				// Remove delimiters - use reversed order to not mix the offsets while removing.
+				for ( const range of rangesToRemove.reverse() ) {
+					writer.remove( range );
+				}
+			} );
 		} );
 	}
 }
@@ -221,3 +190,15 @@ export default class InlineAutoformatEditing {
 function getText( element ) {
 	return Array.from( element.getChildren() ).reduce( ( a, b ) => a + b.data, '' );
 }
+
+// Converts output of the test function provided to the InlineAutoformatEditing and converts it to the model ranges
+// inside provided block.
+//
+// @private
+// @param {module:engine/model/element~Element} block
+// @param {Array.<Array>} arrays
+function testOutputToRanges( block, arrays ) {
+	return arrays
+		.filter( array => ( array[ 0 ] !== undefined && array[ 1 ] !== undefined ) )
+		.map( array => ModelRange.createFromParentsAndOffsets( block, array[ 0 ], block, array[ 1 ] ) );
+}

tests/autoformat.js

@@ -190,7 +190,7 @@ describe( 'Autoformat', () => {
 	} );
 
 	describe( 'Block quote', () => {
-		it( 'should replace greater-than character with heading', () => {
+		it( 'should replace greater-than character with block quote', () => {
 			setData( model, '<paragraph>>[]</paragraph>' );
 			model.change( writer => {
 				writer.insertText( ' ', doc.selection.getFirstPosition() );

tests/inlineautoformatediting.js

@@ -115,32 +115,5 @@ describe( 'InlineAutoformatEditing', () => {
 
 			sinon.assert.notCalled( formatSpy );
 		} );
-
-		it( 'should detach removed ranges', () => {
-			const detachSpies = [];
-			const callback = fixBatch => testUtils.sinon.stub( fixBatch, 'remove' ).callsFake( saveDetachSpy );
-			testUtils.sinon.stub( editor.model.schema, 'getValidRanges' )
-				.callThrough()
-				.callsFake( ranges => ranges.map( saveDetachSpy ) );
-
-			new InlineAutoformatEditing( editor, /(\*)(.+?)(\*)/g, callback ); // eslint-disable-line no-new
-
-			setData( model, '<paragraph>*foobar[]</paragraph>' );
-
-			model.change( writer => {
-				writer.insertText( '*', doc.selection.getFirstPosition() );
-			} );
-
-			// There should be two removed ranges and one range used to apply autoformat.
-			expect( detachSpies ).to.have.length( 3 );
-
-			for ( const spy of detachSpies ) {
-				testUtils.sinon.assert.calledOnce( spy );
-			}
-
-			function saveDetachSpy( range ) {
-				detachSpies.push( testUtils.sinon.spy( range, 'detach' ) );
-			}
-		} );
 	} );
 } );