Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/48 #71

Merged
merged 17 commits into from
Mar 3, 2017
Merged

T/48 #71

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0da72ce
Introduced InputCommand.
f1ames Feb 10, 2017
19f7f83
Tests: InputCommand unit tests.
f1ames Feb 10, 2017
42eb5c1
Fix selection after text insertion.
f1ames Feb 13, 2017
c755227
Use test utils for sinon.
f1ames Feb 13, 2017
cd737ae
Consider only text insertions when incrementing buffer size.
f1ames Feb 13, 2017
0d64eeb
Moved buffer back to input feature.
f1ames Feb 14, 2017
59885b8
Docs corrections.
f1ames Feb 15, 2017
3b2e801
Merge branch 'master' into t/48
f1ames Feb 16, 2017
69e4f0b
Tests: corrected selection misaligned with inserted text.
f1ames Feb 16, 2017
4367f57
Tests: spellchecking input tests.
f1ames Feb 16, 2017
4dbf13b
Try to recognize if mutation was generated by typing.
f1ames Feb 17, 2017
966fb08
Refactoring: buffer back to InputCommand, passing resultPosition to I…
f1ames Feb 27, 2017
e2c4ad2
Merge branch 'master' into t/48
f1ames Feb 27, 2017
07fdf0a
Insert text only if its length is > 0.
f1ames Feb 27, 2017
7ce5362
API docs fixes.
Reinmar Feb 28, 2017
5690acf
Do not check if range/text is not set.
f1ames Mar 2, 2017
8b04412
Text is optional and defaults to empty text.
f1ames Mar 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 35 additions & 84 deletions src/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@
*/

import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
import ChangeBuffer from './changebuffer';
import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range';
import ViewPosition from '@ckeditor/ckeditor5-engine/src/view/position';
import ViewText from '@ckeditor/ckeditor5-engine/src/view/text';
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';

/**
* Handles text input coming from the keyboard or other input methods.
Expand All @@ -28,36 +28,19 @@ export default class Input extends Plugin {
init() {
const editor = this.editor;
const editingView = editor.editing.view;
const inputCommand = new InputCommand( editor );

/**
* Typing's change buffer used to group subsequent changes into batches.
*
* @protected
* @member {typing.ChangeBuffer} #_buffer
*/
this._buffer = new ChangeBuffer( editor.document, 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', inputCommand );

this.listenTo( editingView, 'keydown', ( evt, data ) => {
this._handleKeydown( data );
this._handleKeydown( data, inputCommand.buffer );
}, { priority: 'lowest' } );

this.listenTo( editingView, 'mutations', ( evt, mutations, viewSelection ) => {
this._handleMutations( mutations, viewSelection );
} );
}

/**
* @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
Expand All @@ -72,29 +55,28 @@ export default class Input extends Plugin {
*
* @private
* @param {module:engine/view/observer/keyobserver~KeyEventData} evtData
* @param {typing.ChangeBuffer} buffer
*/
_handleKeydown( evtData ) {
_handleKeydown( evtData, buffer ) {
const doc = this.editor.document;

if ( isSafeKeystroke( evtData ) || doc.selection.isCollapsed ) {
return;
}

doc.enqueueChanges( () => {
this.editor.data.deleteContent( doc.selection, this._buffer.batch );
this.editor.data.deleteContent( doc.selection, buffer.batch );
} );
}

/**
* Handles DOM mutations.
*
* @param {Array.<engine.view.Document~MutatatedText|engine.view.Document~MutatatedChildren>} mutations
* @param {Array.<module:engine/view/document~MutatatedText|module:engine/view/document~MutatatedChildren>} mutations
* @param {module:engine/controller/editingcontroller~EditingController} viewSelection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong type.

*/
_handleMutations( mutations, viewSelection ) {
const doc = this.editor.document;
const handler = new MutationHandler( this.editor.editing, this._buffer );

doc.enqueueChanges( () => handler.handle( mutations, viewSelection ) );
new MutationHandler( this.editor ).handle( mutations, viewSelection );
}
}

Expand All @@ -107,45 +89,36 @@ class MutationHandler {
/**
* Creates an instance of the mutation handler.
*
* @param {module:engine/controller/editingcontroller~EditingController} editing
* @param {module:typing/changebuffer~ChangeBuffer} buffer
* @param {module:core/editor/editor~Editor} editor
*/
constructor( editing, buffer ) {
constructor( editor ) {
/**
* The editing controller.
* Editor instance for which mutations are handled.
*
* @member {engine.controller.EditingController} #editing
* @readonly
* @member {module:core/editor/editor~Editor} #editor
*/
this.editing = editing;
this.editor = editor;

/**
* The change buffer.
*
* @member {engine.controller.EditingController} #buffer
*/
this.buffer = buffer;

/**
* The number of inserted characters which need to be fed to the {@link #buffer change buffer}.
* The editing controller.
*
* @member {Number} #insertedCharacterCount
* @member {module:engine/controller/editingcontroller~EditingController} #editing
*/
this.insertedCharacterCount = 0;
this.editing = this.editor.editing;
}

/**
* Handles given mutations.
*
* @param {Array.<engine.view.Document~MutatatedText|engine.view.Document~MutatatedChildren>} mutations
* @param {Array.<module:engine/view/document~MutatatedText|module:engine/view/document~MutatatedChildren>} mutations
*/
handle( mutations, viewSelection ) {
for ( let mutation of mutations ) {
// Fortunately it will never be both.
this._handleTextMutation( mutation, viewSelection );
this._handleTextNodeInsertion( mutation );
}

this.buffer.input( Math.max( this.insertedCharacterCount, 0 ) );
}

_handleTextMutation( mutation, viewSelection ) {
Expand Down Expand Up @@ -211,20 +184,11 @@ class MutationHandler {
const viewPos = new ViewPosition( mutation.node, firstChangeAt );
const modelPos = this.editing.mapper.toModelPosition( viewPos );

// Remove appropriate number of characters from the model text node.
if ( deletions > 0 ) {
const removeRange = ModelRange.createFromPositionAndShift( modelPos, deletions );
this._remove( removeRange, deletions );
}

// Insert appropriate characters basing on `mutation.text`.
const insertedText = mutation.newText.substr( firstChangeAt, insertions );
this._insert( modelPos, insertedText );

// If there was `viewSelection` and it got correctly mapped, collapse selection at found model position.
if ( modelSelectionPosition ) {
this.editing.model.selection.collapse( modelSelectionPosition );
}
this.editor.execute( 'input', {
text: mutation.newText.substr( firstChangeAt, insertions ),
range: deletions > 0 ? ModelRange.createFromPositionAndShift( modelPos, deletions ) : null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the consts out of this object literal. Those variables can make the code more readable (and easier to debug) thanks to their names.

selectionAnchor: modelSelectionPosition
} );
}

_handleTextNodeInsertion( mutation ) {
Expand Down Expand Up @@ -255,29 +219,16 @@ class MutationHandler {

const viewPos = new ViewPosition( mutation.node, change.index );
const modelPos = this.editing.mapper.toModelPosition( viewPos );
let insertedText = change.values[ 0 ].data;

// Replace &nbsp; inserted by the browser with normal space.
// See comment in `_handleTextMutation`.
// 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.
insertedText = insertedText.replace( /\u00A0/g, ' ' );

this._insert( modelPos, insertedText );

this.editing.model.selection.collapse( modelPos.getShiftedBy( insertedText.length ) );
}

_insert( position, text ) {
this.buffer.batch.weakInsert( position, text );

this.insertedCharacterCount += text.length;
}

_remove( range, length ) {
this.buffer.batch.remove( range );

this.insertedCharacterCount -= length;
const insertedText = change.values[ 0 ].data;

this.editor.execute( 'input', {
// Replace &nbsp; inserted by the browser with normal space.
// See comment in `_handleTextMutation`.
// 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 )
} );
}
}

Expand Down
100 changes: 100 additions & 0 deletions src/inputcommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/**
* @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/**
* @module typing/inputcommand
*/

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.
*
* @extends core.command.Command
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old notation. Now should be @extends {module:blebleble~Command}.

*/
export default class InputCommand extends Command {
/**
* Creates an instance of the command.
*
* @param {module:core/editor/editor~Editor} editor
*/
constructor( editor ) {
super( editor );

/**
* Typing's change buffer used to group subsequent changes into batches.
*
* @protected
* @member {typing.ChangeBuffer} #_buffer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old type notation as well.

*/
this._buffer = new ChangeBuffer( editor.document, editor.config.get( 'typing.undoStep' ) || 20 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be given to the command as an option, not taken by it from the editor. A class like a command should depend on the world in as little details as possible.


// TODO The above default configuration value should be defined using editor.config.define() once it's fixed.
}

/**
* @inheritDoc
*/
destroy() {
super.destroy();

this._buffer.destroy();
this._buffer = null;
}

/**
* Returns the current buffer.
*
* @type {typing.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
* on the beginning of the range (which after removal is a collapsed range).
*
* @param {Object} [options] The command options.
* @param {String} [options.text=''] Text to be inserted.
* @param {module:engine/model/range~Range} [options.range=null] Range in which the text is inserted. Defaults
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the range could default to first selection's range if not passed. Will be pretty useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, it is stated in the comment: Range in which the text is inserted. Defaults to first range in the current selection. However, it is no stated in the param specs, should it be like [options.range] plus description or something like [options.range=document.selection.getFirstRange()]?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be [options.range] plus description. Can also be the latter. But definitely not =null :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes perfect sense, I'll go with first one.

* to first range in the current selection.
* @param {module:engine/model/position~Position} [options.selectionAnchor] Selection anchor which will be used
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This command should not need to accept the selection anchor (I don't even know how it's used). It should calculate the result selection position based on the given target range to remove and text which it's inserting. At least... I hope that it can be done this way ;>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For simple case it works as you described. But there are cases like e.g. spell checking and probably it was the reason that modelSelectionPosition was introduced at the first place.
If there is text like athat and spell checking is used to replaced it with that, based on mutation there is range like [a]that and inserted text is empty. I am not sure if it is possible to calculate proper selection position based on that (I assume after replacing the position should be after replaced/corrected word)?
I may propose changing the option name to e.g. targetPosition which may be a little more meaningful, unless ofc there is a way to correctly calculate selection in those cases?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you replace athat with that and according to the engine, the selection should be after "that", then the passes options.range should be [athat] and options.text = 'that'. The input command will remove the whole "athat", insert "that" and place the selection after "that". You don't need options.selectionAnchor for that.

I understand that this may not be fully intuitive at first, but think about this from OT perspective. If you're fixing the word's "athat" spelling you're replacing it with "that". The whole word, not just a single letter. This may be crucial when resolving some conflicting changes and, in general, is more semantical.

Hence, try to get rid of selectionAnchor. I wrote before that I'm not sure if it's doable, though, because I haven't checked input feature's implementation. Why did you implement it in the first place? Was it already strictly required by what the mutation observer produces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I was relying too much on what information we already have calculated based on mutations.

For case described above the mutation provided by mutation observer is:

newText: "foo that bar"
node: Text
oldText: "foo athat bar"
type: "text"

I assume based only on this information it is not possible to reliable calculate the range and text which will be passed to InputCommand, unless we may assume that the range (and text) will consist of all words which were modified by mutation. If not I think the mutation observer should be modified to provide some additional information in mutations.

The other spellchecking cases to consider are:
Foo hous a -> Foo house a
Foo hous e -> Foo house
Foo athat -> Food that
Foo xhat -> Foo chat

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, forgot about viewSelection which is passed along with mutations, will see if it will help solve this one :)

* to set selection on a data model.
*/
_doExecute( options = {} ) {
const doc = this.editor.document;
const range = options.range || doc.selection.getFirstRange();
const text = options.text || '';
const selectionAnchor = options.selectionAnchor;
let textInsertions = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed?


if ( range ) {
doc.enqueueChanges( () => {
const isCollapsedRange = range.isCollapsed;

if ( !isCollapsedRange ) {
this._buffer.batch.remove( range );
}

if ( text ) {
textInsertions = text.length;
this._buffer.batch.weakInsert( range.start, text );
}

if ( selectionAnchor ) {
this.editor.data.model.selection.collapse( selectionAnchor );
} 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 ) );
}

this._buffer.input( textInsertions );
} );
}
}
}
30 changes: 0 additions & 30 deletions tests/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,22 +65,6 @@ describe( 'Input feature', () => {
listenter.stopListening();
} );

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', [
Expand Down Expand Up @@ -325,19 +309,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 = sinon.spy();

typing.destroy();

expect( destroy.calledOnce ).to.be.true;
expect( typing._buffer ).to.be.null;
} );
} );
} );

Loading