From 39cbe6ceb553c01337ce6e69a43d065ab44528b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Fri, 9 Mar 2018 18:32:48 +0100 Subject: [PATCH 1/5] Checking if differ returns single change when using inline autoformatter. --- src/inlineautoformatediting.js | 115 +++++++++++++++++---------------- 1 file changed, 58 insertions(+), 57 deletions(-) diff --git a/src/inlineautoformatediting.js b/src/inlineautoformatediting.js index 14bf9f5..744206f 100644 --- a/src/inlineautoformatediting.js +++ b/src/inlineautoformatediting.js @@ -141,74 +141,75 @@ 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; - } + // Do nothing if selection is not collapsed. + if ( !selection.isCollapsed || !selection.focus || !selection.focus.parent ) { + return; + } - const selection = editor.model.document.selection; + const changes = Array.from( editor.model.document.differ.getChanges() ); + const entry = changes[ 0 ]; - if ( !selection.isCollapsed || !selection.focus || !selection.focus.parent ) { - continue; - } + // Typing is only a single change. + if ( changes.length != 1 || entry.type !== 'insert' || entry.name != '$text' ) { + return; + } + + const block = selection.focus.parent; + const text = getText( block ).slice( 0, selection.focus.offset ); + const ranges = testCallback( text ); + const rangesToFormat = []; - 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; + // Apply format before deleting text. + ranges.format.forEach( range => { + if ( range[ 0 ] === undefined || range[ 1 ] === undefined ) { + 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 ); + rangesToFormat.push( LiveRange.createFromParentsAndOffsets( + block, range[ 0 ], + block, range[ 1 ] + ) ); + } ); - // Apply format. - formatCallback( writer, validRanges ); + const rangesToRemove = []; - // Detach ranges used to apply Autoformat. Prevents memory leaks. #39 - rangesToFormat.forEach( range => range.detach() ); + // Reverse order to not mix the offsets while removing. + ranges.remove.slice().reverse().forEach( range => { + if ( range[ 0 ] === undefined || range[ 1 ] === undefined ) { + return; + } - // Remove delimiters. - for ( const range of rangesToRemove ) { - writer.remove( range ); + rangesToRemove.push( LiveRange.createFromParentsAndOffsets( + block, range[ 0 ], + block, range[ 1 ] + ) ); + } ); - // Prevents memory leaks. - // https://github.com/ckeditor/ckeditor5-autoformat/issues/39 - range.detach(); - } - } ); + if ( !( rangesToFormat.length && rangesToRemove.length ) ) { + 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 ); + + // Apply format. + formatCallback( writer, validRanges ); + + // Detach ranges used to apply Autoformat. Prevents memory leaks. #39 + rangesToFormat.forEach( range => range.detach() ); + + // Remove delimiters. + for ( const range of rangesToRemove ) { + writer.remove( range ); + + // Prevents memory leaks. + // https://github.com/ckeditor/ckeditor5-autoformat/issues/39 + range.detach(); + } + } ); } ); } } From 0c9cabc3988d5404a075a20e8dc0eb2bab15c0a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Mon, 12 Mar 2018 08:20:41 +0100 Subject: [PATCH 2/5] Added tests checking integration with undo. --- src/inlineautoformatediting.js | 2 +- tests/autoformat.js | 2 +- tests/undointegration.js | 193 +++++++++++++++++++++++++++++++++ 3 files changed, 195 insertions(+), 2 deletions(-) create mode 100644 tests/undointegration.js diff --git a/src/inlineautoformatediting.js b/src/inlineautoformatediting.js index 744206f..a4541f1 100644 --- a/src/inlineautoformatediting.js +++ b/src/inlineautoformatediting.js @@ -151,7 +151,7 @@ export default class InlineAutoformatEditing { const changes = Array.from( editor.model.document.differ.getChanges() ); const entry = changes[ 0 ]; - // Typing is only a single change. + // Typing is represented by only a single change. if ( changes.length != 1 || entry.type !== 'insert' || entry.name != '$text' ) { return; } diff --git a/tests/autoformat.js b/tests/autoformat.js index 0335e89..d73a090 100644 --- a/tests/autoformat.js +++ b/tests/autoformat.js @@ -197,7 +197,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, '>[]' ); model.change( writer => { writer.insertText( ' ', doc.selection.getFirstPosition() ); diff --git a/tests/undointegration.js b/tests/undointegration.js new file mode 100644 index 0000000..6a08a50 --- /dev/null +++ b/tests/undointegration.js @@ -0,0 +1,193 @@ +/** + * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +import Autoformat from '../src/autoformat'; + +import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; +import ListEditing from '@ckeditor/ckeditor5-list/src/listediting'; +import HeadingEditing from '@ckeditor/ckeditor5-heading/src/headingediting'; +import BoldEditing from '@ckeditor/ckeditor5-basic-styles/src/bold/boldediting'; +import CodeEditing from '@ckeditor/ckeditor5-basic-styles/src/code/codeediting'; +import ItalicEditing from '@ckeditor/ckeditor5-basic-styles/src/italic/italicediting'; +import BlockQuoteEditing from '@ckeditor/ckeditor5-block-quote/src/blockquoteediting'; +import Enter from '@ckeditor/ckeditor5-enter/src/enter'; +import Undo from '@ckeditor/ckeditor5-undo/src/undoediting'; + +import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; + +import { setData, getData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; +import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; + +testUtils.createSinonSandbox(); + +describe( 'Autoformat', () => { + let editor, model, doc; + + beforeEach( () => { + return VirtualTestEditor + .create( { + plugins: [ + Enter, + Undo, + Paragraph, + Autoformat, + ListEditing, + HeadingEditing, + BoldEditing, + ItalicEditing, + CodeEditing, + BlockQuoteEditing + ] + } ) + .then( newEditor => { + editor = newEditor; + model = editor.model; + doc = model.document; + } ); + } ); + + afterEach( () => { + return editor.destroy(); + } ); + + describe( 'inline', () => { + it( 'should undo replacing "**" with bold', () => { + setData( model, '**foobar*[]' ); + model.change( writer => { + writer.insertText( '*', doc.selection.getFirstPosition() ); + } ); + + expect( getData( model ) ).to.equal( '<$text bold="true">foobar[]' ); + editor.execute( 'undo' ); + expect( getData( model ) ).to.equal( '**foobar**[]' ); + } ); + + it( 'should undo replacing "__" with bold', () => { + setData( model, '__foobar_[]' ); + model.change( writer => { + writer.insertText( '_', doc.selection.getFirstPosition() ); + } ); + + expect( getData( model ) ).to.equal( '<$text bold="true">foobar[]' ); + editor.execute( 'undo' ); + expect( getData( model ) ).to.equal( '__foobar__[]' ); + } ); + + it( 'should undo replacing "*" with italic', () => { + setData( model, '*foobar[]' ); + model.change( writer => { + writer.insertText( '*', doc.selection.getFirstPosition() ); + } ); + + expect( getData( model ) ).to.equal( '<$text italic="true">foobar[]' ); + editor.execute( 'undo' ); + expect( getData( model ) ).to.equal( '*foobar*[]' ); + } ); + + it( 'should undo replacing "_" with italic', () => { + setData( model, '_foobar[]' ); + model.change( writer => { + writer.insertText( '_', doc.selection.getFirstPosition() ); + } ); + + expect( getData( model ) ).to.equal( '<$text italic="true">foobar[]' ); + editor.execute( 'undo' ); + expect( getData( model ) ).to.equal( '_foobar_[]' ); + } ); + + it( 'should undo replacing "`" with code', () => { + setData( model, '`foobar[]' ); + model.change( writer => { + writer.insertText( '`', doc.selection.getFirstPosition() ); + } ); + + expect( getData( model ) ).to.equal( '<$text code="true">foobar[]' ); + editor.execute( 'undo' ); + expect( getData( model ) ).to.equal( '`foobar`[]' ); + } ); + } ); + + describe( 'block', () => { + it( 'should work when replacing asterisk', () => { + setData( model, '*[]' ); + model.change( writer => { + writer.insertText( ' ', doc.selection.getFirstPosition() ); + } ); + + expect( getData( model ) ).to.equal( '[]' ); + + editor.execute( 'undo' ); + expect( getData( model ) ).to.equal( '* []' ); + } ); + + it( 'should work when replacing minus character', () => { + setData( model, '-[]' ); + model.change( writer => { + writer.insertText( ' ', doc.selection.getFirstPosition() ); + } ); + + expect( getData( model ) ).to.equal( '[]' ); + + editor.execute( 'undo' ); + expect( getData( model ) ).to.equal( '- []' ); + } ); + + it( 'should work when replacing digit with numbered list item using the dot format', () => { + setData( model, '1.[]' ); + model.change( writer => { + writer.insertText( ' ', doc.selection.getFirstPosition() ); + } ); + + expect( getData( model ) ).to.equal( '[]' ); + + editor.execute( 'undo' ); + expect( getData( model ) ).to.equal( '1. []' ); + } ); + + it( 'should work when replacing digit with numbered list item using the parenthesis format', () => { + setData( model, '1)[]' ); + model.change( writer => { + writer.insertText( ' ', doc.selection.getFirstPosition() ); + } ); + + expect( getData( model ) ).to.equal( '[]' ); + editor.execute( 'undo' ); + expect( getData( model ) ).to.equal( '1) []' ); + } ); + + it( 'should work when replacing hash character with heading', () => { + setData( model, '#[]' ); + model.change( writer => { + writer.insertText( ' ', doc.selection.getFirstPosition() ); + } ); + + expect( getData( model ) ).to.equal( '[]' ); + editor.execute( 'undo' ); + expect( getData( model ) ).to.equal( '# []' ); + } ); + + it( 'should work when replacing two hash characters with heading level 2', () => { + setData( model, '##[]' ); + model.change( writer => { + writer.insertText( ' ', doc.selection.getFirstPosition() ); + } ); + + expect( getData( model ) ).to.equal( '[]' ); + editor.execute( 'undo' ); + expect( getData( model ) ).to.equal( '## []' ); + } ); + + it( 'should work when replacing greater-than character with block quote', () => { + setData( model, '>[]' ); + model.change( writer => { + writer.insertText( ' ', doc.selection.getFirstPosition() ); + } ); + + expect( getData( model ) ).to.equal( '
[]
' ); + editor.execute( 'undo' ); + expect( getData( model ) ).to.equal( '> []' ); + } ); + } ); +} ); From fabac16a09b13dcba7e18d4131f601f18ca4f65f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Mon, 12 Mar 2018 14:10:37 +0100 Subject: [PATCH 3/5] Removed LiveRanges from the implementation. Small refactoring. --- src/inlineautoformatediting.js | 64 ++++++++++++-------------------- tests/inlineautoformatediting.js | 27 -------------- 2 files changed, 24 insertions(+), 67 deletions(-) diff --git a/src/inlineautoformatediting.js b/src/inlineautoformatediting.js index a4541f1..4dea6e6 100644 --- a/src/inlineautoformatediting.js +++ b/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, @@ -144,7 +144,7 @@ export default class InlineAutoformatEditing { const selection = editor.model.document.selection; // Do nothing if selection is not collapsed. - if ( !selection.isCollapsed || !selection.focus || !selection.focus.parent ) { + if ( !selection.isCollapsed ) { return; } @@ -152,40 +152,15 @@ export default class InlineAutoformatEditing { const entry = changes[ 0 ]; // Typing is represented by only a single change. - if ( changes.length != 1 || entry.type !== 'insert' || entry.name != '$text' ) { + if ( changes.length != 1 || entry.type !== 'insert' || entry.name != '$text' || entry.length != 1 ) { return; } 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 ] - ) ); - } ); + const testOutput = testCallback( text ); + const rangesToFormat = testOutputToRanges( block, testOutput.format ); + const rangesToRemove = testOutputToRanges( block, testOutput.remove ); if ( !( rangesToFormat.length && rangesToRemove.length ) ) { return; @@ -198,16 +173,9 @@ export default class InlineAutoformatEditing { // Apply format. formatCallback( writer, validRanges ); - // Detach ranges used to apply Autoformat. Prevents memory leaks. #39 - rangesToFormat.forEach( range => range.detach() ); - - // Remove delimiters. - for ( const range of rangesToRemove ) { + // Remove delimiters - use reversed order to not mix the offsets while removing. + for ( const range of rangesToRemove.reverse() ) { writer.remove( range ); - - // Prevents memory leaks. - // https://github.com/ckeditor/ckeditor5-autoformat/issues/39 - range.detach(); } } ); } ); @@ -222,3 +190,19 @@ 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.} arrays +function testOutputToRanges( block, arrays ) { + return arrays.map( array => { + if ( array[ 0 ] === undefined || array[ 1 ] === undefined ) { + return; + } + + return ModelRange.createFromParentsAndOffsets( block, array[ 0 ], block, array[ 1 ] ); + } ).filter( range => !!range ); +} diff --git a/tests/inlineautoformatediting.js b/tests/inlineautoformatediting.js index aabada9..b2b8ddf 100644 --- a/tests/inlineautoformatediting.js +++ b/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, '*foobar[]' ); - - 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' ) ); - } - } ); } ); } ); From 9136905097cac63eb4c78edf0b3efd0bc34c00fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Mon, 12 Mar 2018 14:30:20 +0100 Subject: [PATCH 4/5] Simplified block auto formatting. --- src/blockautoformatediting.js | 43 ++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/src/blockautoformatediting.js b/src/blockautoformatediting.js index fdee5e5..5e8d5e0 100644 --- a/src/blockautoformatediting.js +++ b/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 } ); + } ); } ); } } From 9024f2b7619526006c9b6b567c85bab0a503bccb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Mon, 12 Mar 2018 15:41:56 +0100 Subject: [PATCH 5/5] Small refactoring of helper function in InlineAutoformatEditing. --- src/inlineautoformatediting.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/inlineautoformatediting.js b/src/inlineautoformatediting.js index 4dea6e6..9dcb21c 100644 --- a/src/inlineautoformatediting.js +++ b/src/inlineautoformatediting.js @@ -198,11 +198,7 @@ function getText( element ) { // @param {module:engine/model/element~Element} block // @param {Array.} arrays function testOutputToRanges( block, arrays ) { - return arrays.map( array => { - if ( array[ 0 ] === undefined || array[ 1 ] === undefined ) { - return; - } - - return ModelRange.createFromParentsAndOffsets( block, array[ 0 ], block, array[ 1 ] ); - } ).filter( range => !!range ); + return arrays + .filter( array => ( array[ 0 ] !== undefined && array[ 1 ] !== undefined ) ) + .map( array => ModelRange.createFromParentsAndOffsets( block, array[ 0 ], block, array[ 1 ] ) ); }