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

Refactoring: buffer back to InputCommand, passing resultPosition to InputCommand.

Krzysztof Krztoń 9 лет назад
Родитель
Сommit
a11e78c874

+ 19 - 97
packages/ckeditor5-typing/src/input.js

@@ -15,7 +15,6 @@ import diff from '@ckeditor/ckeditor5-utils/src/diff';
 import diffToChanges from '@ckeditor/ckeditor5-utils/src/difftochanges';
 import { getCode } from '@ckeditor/ckeditor5-utils/src/keyboard';
 import InputCommand from './inputcommand';
-import ChangeBuffer from './changebuffer';
 
 /**
  * Handles text input coming from the keyboard or other input methods.
@@ -29,21 +28,14 @@ export default class Input extends Plugin {
 	init() {
 		const editor = this.editor;
 		const editingView = editor.editing.view;
-
-		/**
-		 * Typing's change buffer used to group subsequent changes into batches.
-		 *
-		 * @protected
-		 * @member {module:typing/changebuffer~ChangeBuffer} #_buffer
-		 */
-		this._buffer = new ChangeBuffer( editor.document, editor.config.get( 'typing.undoStep' ) || 20 );
+		const inputCommand = new InputCommand( editor, editor.config.get( 'typing.undoStep' ) || 20 );
 
 		// TODO The above default configuration value should be defined using editor.config.define() once it's fixed.
 
-		editor.commands.set( 'input', new InputCommand( editor ) );
+		editor.commands.set( 'input', inputCommand );
 
 		this.listenTo( editingView, 'keydown', ( evt, data ) => {
-			this._handleKeydown( data );
+			this._handleKeydown( data, inputCommand.buffer );
 		}, { priority: 'lowest' } );
 
 		this.listenTo( editingView, 'mutations', ( evt, mutations, viewSelection ) => {
@@ -51,16 +43,6 @@ export default class Input extends Plugin {
 		} );
 	}
 
-	/**
-	 * @inheritDoc
-	 */
-	destroy() {
-		super.destroy();
-
-		this._buffer.destroy();
-		this._buffer = null;
-	}
-
 	/**
 	 * Handles the keydown event. We need to guess whether such keystroke is going to result
 	 * in typing. If so, then before character insertion happens, any selected content needs
@@ -75,8 +57,9 @@ export default class Input extends Plugin {
 	 *
 	 * @private
 	 * @param {module:engine/view/observer/keyobserver~KeyEventData} evtData
+	 * @param {module:typing/changebuffer~ChangeBuffer} buffer
 	 */
-	_handleKeydown( evtData ) {
+	_handleKeydown( evtData, buffer ) {
 		const doc = this.editor.document;
 
 		if ( isSafeKeystroke( evtData ) || doc.selection.isCollapsed ) {
@@ -84,7 +67,7 @@ export default class Input extends Plugin {
 		}
 
 		doc.enqueueChanges( () => {
-			this.editor.data.deleteContent( doc.selection, this._buffer.batch );
+			this.editor.data.deleteContent( doc.selection, buffer.batch );
 		} );
 	}
 
@@ -96,7 +79,7 @@ export default class Input extends Plugin {
 	 * @param {module:engine/view/selection~Selection|null} viewSelection
 	 */
 	_handleMutations( mutations, viewSelection ) {
-		new MutationHandler( this.editor, this._buffer ).handle( mutations, viewSelection );
+		new MutationHandler( this.editor ).handle( mutations, viewSelection );
 	}
 }
 
@@ -110,9 +93,8 @@ class MutationHandler {
 	 * Creates an instance of the mutation handler.
 	 *
 	 * @param {module:core/editor/editor~Editor} editor
-	 * @param {module:typing/changebuffer~ChangeBuffer} buffer
 	 */
-	constructor( editor, buffer ) {
+	constructor( editor ) {
 		/**
 		 * Editor instance for which mutations are handled.
 		 *
@@ -128,14 +110,6 @@ class MutationHandler {
 		 * @member {module:engine/controller/editingcontroller~EditingController} #editing
 		 */
 		this.editing = this.editor.editing;
-
-		/**
-		 * The change buffer;
-		 *
-		 * @readonly
-		 * @member {module:typing/changebuffer~ChangeBuffer} #buffer
-		 */
-		this.buffer = buffer;
 	}
 
 	/**
@@ -146,26 +120,12 @@ class MutationHandler {
 	 */
 	handle( mutations, viewSelection ) {
 		for ( let mutation of mutations ) {
-			// console.log( 'if', mutation );
 			// Fortunately it will never be both.
 			this._handleTextMutation( mutation, viewSelection );
 			this._handleTextNodeInsertion( mutation );
 		}
 	}
 
-	// TODO needs proper description
-	// Check if mutation is normal typing.
-	// There is also composition (mutations will be blocked during composing in future) and spellchecking.
-	// There are also cases when spell checking generates one insertion and no deletions (like hous -> house)
-	// and the mutation is identical as typing.
-	_isTyping( insertions, deletions, firstChangeAt, lastChangeAt, viewSelection ) {
-		const viewSelectionAnchorOffset = viewSelection ? viewSelection.anchor.offset : null;
-
-		return deletions === 0 && insertions == 1 &&
-			firstChangeAt && lastChangeAt && ( lastChangeAt - firstChangeAt === 0 ) &&
-			( viewSelectionAnchorOffset <= firstChangeAt + 1 );
-	}
-
 	_handleTextMutation( mutation, viewSelection ) {
 		if ( mutation.type != 'text' ) {
 			return;
@@ -218,60 +178,23 @@ class MutationHandler {
 			}
 		}
 
-		// TODO transform into human readable and understandable text.
-		// For insertions of the same character in a row, like
-		// ab^cde - inserting c
-		// and
-		// abc^de - inserting c
-		// the diff is the same.
-		// This causes the problem with using ModelRange.createFromPositionAndShift( modelPos, 0 ); (passing when there is no removeRange)
-		// because the last character in the sequence of the same characters is always recognized as an insertion.
-		// From the other hand without ModelRange.createFromPositionAndShift( modelPos, 0 ); it works fine, but spellchecking
-		// cases which cannot be differentiated from typing ( hous -> house ) is broken because `InputCommand` wll use
-		// default selection which is [hous] to do text replacement (results in e[]).
-
-		// Get the position in view and model where the changes will happen.
-		let viewPos = new ViewPosition( mutation.node, firstChangeAt );
+		// Try setting new model selection according to passed view selection.
+		let modelSelectionPosition = null;
 
-		// TODO references to previous comment about diff with same character sequence. Needs proper, dteailed description.
-		if ( viewSelection && viewSelection.anchor.offset <= firstChangeAt ) {
-			viewPos = new ViewPosition( mutation.node, viewSelection.anchor.offset - 1 );
+		if ( viewSelection ) {
+			modelSelectionPosition = this.editing.mapper.toModelPosition( viewSelection.anchor );
 		}
 
-		let modelPos = this.editing.mapper.toModelPosition( viewPos );
-		let removeRange = ModelRange.createFromPositionAndShift( modelPos, deletions || 0 );
-		let insertText = newText.substr( firstChangeAt, insertions );
-
-		// TODO detailed description what is going on here.
-		if ( viewSelection && !this._isTyping( insertions, deletions, firstChangeAt, lastChangeAt, viewSelection ) ) {
-			// The beginning of the corrected word is always at the fixed position no matter what was changed
-			// by spellchecking mechanism so it may be recognized by getting last space before corrected word.
-			let lastSpaceBeforeChangeAt = 0;
-
-			for ( let i = 0; i < newText.length; i++ ) {
-				if ( newText[ i ] === ' ' ) {
-					if ( i < firstChangeAt ) {
-						lastSpaceBeforeChangeAt = i + 1;
-					} else {
-						break;
-					}
-				}
-			}
-
-			let correctedText = newText.substring( lastSpaceBeforeChangeAt, viewSelection.anchor.offset );
-
-			if ( correctedText.length ) {
-				insertText = correctedText;
-				viewPos = new ViewPosition( mutation.node, lastSpaceBeforeChangeAt );
-				modelPos = this.editing.mapper.toModelPosition( viewPos );
-				removeRange = ModelRange.createFromPositionAndShift( modelPos, insertText.length - insertions + deletions );
-			}
-		}
+		// Get the position in view and model where the changes will happen.
+		const viewPos = new ViewPosition( mutation.node, firstChangeAt );
+		const modelPos = this.editing.mapper.toModelPosition( viewPos );
+		const removeRange = ModelRange.createFromPositionAndShift( modelPos, deletions );
+		const insertText = newText.substr( firstChangeAt, insertions );
 
 		this.editor.execute( 'input', {
 			text: insertText,
 			range: removeRange,
-			buffer: this.buffer
+			resultPosition: modelSelectionPosition
 		} );
 	}
 
@@ -311,8 +234,7 @@ class MutationHandler {
 			// In this case we don't need to do this before `diff` because we diff whole nodes.
 			// Just change &nbsp; in case there are some.
 			text: insertedText.replace( /\u00A0/g, ' ' ),
-			range: new ModelRange( modelPos ),
-			buffer: this.buffer
+			range: new ModelRange( modelPos )
 		} );
 	}
 }

+ 49 - 11
packages/ckeditor5-typing/src/inputcommand.js

@@ -8,6 +8,7 @@
  */
 
 import Command from '@ckeditor/ckeditor5-core/src/command/command';
+import ChangeBuffer from './changebuffer';
 
 /**
  * The input command. Used by the {@link module:typing/input~Input input feature} to handle typing.
@@ -15,6 +16,44 @@ import Command from '@ckeditor/ckeditor5-core/src/command/command';
  * @extends module:core/command/command~Command
  */
 export default class InputCommand extends Command {
+	/**
+	 * Creates an instance of the command.
+	 *
+	 * @param {module:core/editor/editor~Editor} editor
+	 * @param {Number} undoStep The maximum number of atomic changes which can be contained in one batch.
+	 */
+	constructor( editor, undoStep ) {
+		super( editor );
+
+		/**
+		 * Typing's change buffer used to group subsequent changes into batches.
+		 *
+		 * @readonly
+		 * @private
+		 * @member {module:typing/changebuffer~ChangeBuffer} #_buffer
+		 */
+		this._buffer = new ChangeBuffer( editor.document, undoStep );
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	destroy() {
+		super.destroy();
+
+		this._buffer.destroy();
+		this._buffer = null;
+	}
+
+	/**
+	 * Returns the current buffer.
+	 *
+	 * @type {module:typing/changebuffer~ChangeBuffer}
+	 */
+	get buffer() {
+		return this._buffer;
+	}
+
 	/**
 	 * Executes the input command. It replaces the content within the given range with the given text.
 	 * Replacing is a two step process, first content within the range is removed and then new text is inserted
@@ -24,39 +63,38 @@ export default class InputCommand extends Command {
 	 * @param {String} [options.text=''] Text to be inserted.
 	 * @param {module:engine/model/range~Range} [options.range] Range in which the text is inserted. Defaults
 	 * to the first range in the current selection.
-	 * @param {module:engine/model/position~Position} [options.selectionAnchor] Selection anchor which will be used
+	 * @param {module:engine/model/position~Position} [options.resultPosition] Position which will be used
 	 * to set selection on a data model.
-	 * @param {module:typing/changebuffer~ChangeBuffer} [options.buffer]
 	 */
 	_doExecute( options = {} ) {
 		const doc = this.editor.document;
+		const text = options.text;
 		const range = options.range || doc.selection.getFirstRange();
-		const text = options.text || '';
-		const selectionAnchor = options.selectionAnchor;
-		const buffer = options.buffer;
+		const resultPosition = options.resultPosition;
+
 		let textInsertions = 0;
 
-		if ( range && buffer ) {
+		if ( range && text !== undefined ) {
 			doc.enqueueChanges( () => {
 				const isCollapsedRange = range.isCollapsed;
 
 				if ( !isCollapsedRange ) {
-					buffer.batch.remove( range );
+					this._buffer.batch.remove( range );
 				}
 
 				if ( text ) {
 					textInsertions = text.length;
-					buffer.batch.weakInsert( range.start, text );
+					this._buffer.batch.weakInsert( range.start, text );
 				}
 
-				if ( selectionAnchor ) {
-					this.editor.data.model.selection.collapse( selectionAnchor );
+				if ( resultPosition ) {
+					this.editor.data.model.selection.collapse( resultPosition );
 				} else if ( isCollapsedRange ) {
 					// If range was collapsed just shift the selection by the number of inserted characters.
 					this.editor.data.model.selection.collapse( range.start.getShiftedBy( textInsertions ) );
 				}
 
-				buffer.input( textInsertions );
+				this._buffer.input( textInsertions );
 			} );
 		}
 	}

+ 13 - 51
packages/ckeditor5-typing/tests/input.js

@@ -68,24 +68,6 @@ describe( 'Input feature', () => {
 		listenter.stopListening();
 	} );
 
-	describe( 'buffer', () => {
-		it( 'has a buffer configured to default value of config.typing.undoStep', () => {
-			expect( editor.plugins.get( Input )._buffer ).to.have.property( 'limit', 20 );
-		} );
-
-		it( 'has a buffer configured to config.typing.undoStep', () => {
-			return VirtualTestEditor.create( {
-				plugins: [ Input ],
-				typing: {
-					undoStep: 5
-				}
-			} )
-				.then( editor => {
-					expect( editor.plugins.get( Input )._buffer ).to.have.property( 'limit', 5 );
-				} );
-		} );
-	} );
-
 	describe( 'mutations handling', () => {
 		it( 'should handle text mutation', () => {
 			view.fire( 'mutations', [
@@ -227,7 +209,7 @@ describe( 'Input feature', () => {
 			expect( getViewData( view ) ).to.equal( '<p>foodar{}</p>' );
 		} );
 
-		it( 'should use up to one insert and remove operations', () => {
+		it( 'should use up to one insert and remove operations (spellchecker)', () => {
 			// This test case emulates spellchecker correction.
 
 			const viewSelection = new ViewSelection();
@@ -254,11 +236,8 @@ describe( 'Input feature', () => {
 			// This test case emulates spellchecker correction.
 			editor.setData( '<p>Foo hous a</p>' );
 
-			model.enqueueChanges( () => {
-				model.selection.setRanges( [
-					ModelRange.createFromParentsAndOffsets( modelRoot.getChild( 0 ), 4, modelRoot.getChild( 0 ), 8 )
-				] );
-			} );
+			const viewSelection = new ViewSelection();
+			viewSelection.collapse( viewRoot.getChild( 0 ).getChild( 0 ), 9 );
 
 			view.fire( 'mutations',
 				[ {
@@ -266,7 +245,8 @@ describe( 'Input feature', () => {
 					oldText: 'Foo hous a',
 					newText: 'Foo house a',
 					node: viewRoot.getChild( 0 ).getChild( 0 )
-				} ]
+				} ],
+				viewSelection
 			);
 
 			expect( getModelData( model ) ).to.equal( '<paragraph>Foo house[] a</paragraph>' );
@@ -277,11 +257,8 @@ describe( 'Input feature', () => {
 			// This test case emulates spellchecker correction.
 			editor.setData( '<p>Bar athat foo</p>' );
 
-			model.enqueueChanges( () => {
-				model.selection.setRanges( [
-					ModelRange.createFromParentsAndOffsets( modelRoot.getChild( 0 ), 4, modelRoot.getChild( 0 ), 9 )
-				] );
-			} );
+			const viewSelection = new ViewSelection();
+			viewSelection.collapse( viewRoot.getChild( 0 ).getChild( 0 ), 8 );
 
 			view.fire( 'mutations',
 				[ {
@@ -289,7 +266,8 @@ describe( 'Input feature', () => {
 					oldText: 'Bar athat foo',
 					newText: 'Bar that foo',
 					node: viewRoot.getChild( 0 ).getChild( 0 )
-				} ]
+				} ],
+				viewSelection
 			);
 
 			expect( getModelData( model ) ).to.equal( '<paragraph>Bar that[] foo</paragraph>' );
@@ -300,11 +278,8 @@ describe( 'Input feature', () => {
 			// This test case emulates spellchecker correction.
 			editor.setData( '<p>Foo hous e</p>' );
 
-			model.enqueueChanges( () => {
-				model.selection.setRanges( [
-					ModelRange.createFromParentsAndOffsets( modelRoot.getChild( 0 ), 4, modelRoot.getChild( 0 ), 10 )
-				] );
-			} );
+			const viewSelection = new ViewSelection();
+			viewSelection.collapse( viewRoot.getChild( 0 ).getChild( 0 ), 9 );
 
 			view.fire( 'mutations',
 				[ {
@@ -312,7 +287,8 @@ describe( 'Input feature', () => {
 					oldText: 'Foo hous e',
 					newText: 'Foo house',
 					node: viewRoot.getChild( 0 ).getChild( 0 )
-				} ]
+				} ],
+				viewSelection
 			);
 
 			expect( getModelData( model ) ).to.equal( '<paragraph>Foo house[]</paragraph>' );
@@ -459,19 +435,5 @@ describe( 'Input feature', () => {
 			expect( getModelData( model ) ).to.equal( '<paragraph>foo[]bar</paragraph>' );
 		} );
 	} );
-
-	describe( 'destroy', () => {
-		it( 'should destroy change buffer', () => {
-			const typing = new Input( new VirtualTestEditor() );
-			typing.init();
-
-			const destroy = typing._buffer.destroy = testUtils.sinon.spy();
-
-			typing.destroy();
-
-			expect( destroy.calledOnce ).to.be.true;
-			expect( typing._buffer ).to.be.null;
-		} );
-	} );
 } );
 

+ 43 - 24
packages/ckeditor5-typing/tests/inputcommand.js

@@ -3,11 +3,13 @@
  * For licensing, see LICENSE.md.
  */
 
+import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
 import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltesteditor';
 import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
 import { getData, setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
 import InputCommand from '../src/inputcommand';
 import ChangeBuffer from '../src/changebuffer';
+import Input from '../src/input';
 
 describe( 'InputCommand', () => {
 	let editor, doc, buffer;
@@ -19,9 +21,11 @@ describe( 'InputCommand', () => {
 			.then( newEditor => {
 				editor = newEditor;
 				doc = editor.document;
-				buffer = new ChangeBuffer( doc, 20 );
 
-				editor.commands.set( 'input', new InputCommand( editor ) );
+				const inputCommand = new InputCommand( editor, 20 );
+				editor.commands.set( 'input', inputCommand );
+
+				buffer = inputCommand.buffer;
 
 				doc.schema.registerItem( 'p', '$block' );
 				doc.schema.registerItem( 'h1', '$block' );
@@ -32,6 +36,28 @@ describe( 'InputCommand', () => {
 		buffer.size = 0;
 	} );
 
+	describe( 'buffer', () => {
+		it( 'has buffer getter', () => {
+			expect( editor.commands.get( 'input' ).buffer ).to.be.an.instanceof( ChangeBuffer );
+		} );
+
+		it( 'has a buffer limit configured to default value of 20', () => {
+			expect( editor.commands.get( 'input' )._buffer ).to.have.property( 'limit', 20 );
+		} );
+
+		it( 'has a buffer configured to config.typing.undoStep', () => {
+			return VirtualTestEditor.create( {
+				plugins: [ Input ],
+				typing: {
+					undoStep: 5
+				}
+			} )
+				.then( editor => {
+					expect( editor.commands.get( 'input' )._buffer ).to.have.property( 'limit', 5 );
+				} );
+		} );
+	} );
+
 	describe( 'execute', () => {
 		it( 'uses enqueueChanges', () => {
 			setData( doc, '<p>foo[]bar</p>' );
@@ -39,7 +65,7 @@ describe( 'InputCommand', () => {
 			const spy = testUtils.sinon.spy( doc, 'enqueueChanges' );
 
 			editor.execute( 'input', {
-				buffer: buffer
+				text: ''
 			} );
 
 			expect( spy.calledOnce ).to.be.true;
@@ -49,7 +75,6 @@ describe( 'InputCommand', () => {
 			setData( doc, '<p>foo[]</p>' );
 
 			editor.execute( 'input', {
-				buffer: buffer,
 				text: 'bar',
 				range: editor.document.selection.getFirstRange()
 			} );
@@ -62,7 +87,6 @@ describe( 'InputCommand', () => {
 			setData( doc, '<p>[fooba]r</p>' );
 
 			editor.execute( 'input', {
-				buffer: buffer,
 				text: 'rab',
 				range: editor.document.selection.getFirstRange()
 			} );
@@ -75,7 +99,6 @@ describe( 'InputCommand', () => {
 			setData( doc, '<p>fo[oba]r</p>' );
 
 			editor.execute( 'input', {
-				buffer: buffer,
 				text: 'bazz',
 				range: editor.document.selection.getFirstRange()
 			} );
@@ -88,7 +111,6 @@ describe( 'InputCommand', () => {
 			setData( doc, '<p>fooba[r]</p>' );
 
 			editor.execute( 'input', {
-				buffer: buffer,
 				text: 'zzz',
 				range: editor.document.selection.getFirstRange()
 			} );
@@ -101,7 +123,6 @@ describe( 'InputCommand', () => {
 			setData( doc, '<h1>F[OO</h1><p>b]ar</p>' );
 
 			editor.execute( 'input', {
-				buffer: buffer,
 				text: 'unny c',
 				range: editor.document.selection.getFirstRange()
 			} );
@@ -114,7 +135,6 @@ describe( 'InputCommand', () => {
 			setData( doc, '<p>foob[ar]</p>' );
 
 			editor.execute( 'input', {
-				buffer: buffer,
 				text: 'az'
 			} );
 
@@ -122,11 +142,11 @@ describe( 'InputCommand', () => {
 			expect( buffer.size ).to.be.equal( 2 );
 		} );
 
-		it( 'only removes content when text is not given', () => {
+		it( 'only removes content when empty text given', () => {
 			setData( doc, '<p>[fo]obar</p>' );
 
 			editor.execute( 'input', {
-				buffer: buffer,
+				text: '',
 				range: editor.document.selection.getFirstRange()
 			} );
 
@@ -140,7 +160,6 @@ describe( 'InputCommand', () => {
 			testUtils.sinon.stub( editor.document.selection, 'getFirstRange' ).returns( null );
 
 			editor.execute( 'input', {
-				buffer: buffer,
 				text: 'baz'
 			} );
 
@@ -148,18 +167,6 @@ describe( 'InputCommand', () => {
 			expect( buffer.size ).to.be.equal( 0 );
 		} );
 
-		it( 'does nothing when there is no buffer', () => {
-			setData( doc, '<p>[fo]obar</p>' );
-
-			editor.execute( 'input', {
-				text: 'baz',
-				range: editor.document.selection.getFirstRange()
-			} );
-
-			expect( getData( doc, { selection: true } ) ).to.be.equal( '<p>[fo]obar</p>' );
-			expect( buffer.size ).to.be.equal( 0 );
-		} );
-
 		it( 'does nothing when there is no options object provided', () => {
 			setData( doc, '<p>[fo]obar</p>' );
 
@@ -172,4 +179,16 @@ describe( 'InputCommand', () => {
 			expect( buffer.size ).to.be.equal( 0 );
 		} );
 	} );
+
+	describe( 'destroy', () => {
+		it( 'should destroy change buffer', () => {
+			const command = editor.commands.get( 'input' );
+			const destroy = command._buffer.destroy = testUtils.sinon.spy();
+
+			command.destroy();
+
+			expect( destroy.calledOnce ).to.be.true;
+			expect( command._buffer ).to.be.null;
+		} );
+	} )
 } );