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

T/ckeditor5/1243: Unify widget insertion #1545

Merged
merged 16 commits into from
Sep 18, 2018
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
24 changes: 20 additions & 4 deletions src/model/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -298,13 +298,29 @@ export default class Model {
*
* editor.model.insertContent( modelFragment, editor.model.document.selection );
Copy link

Choose a reason for hiding this comment

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

editor.model.document.selection should be removed, I believe.

*
* If an instance of {module:engine/model/selection~Selection} is passed as `selectable` it will be modified
* to the insertion selection (equal to a range to be selected after insertion).
Copy link
Member

Choose a reason for hiding this comment

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

I'd propose something like:

If an instance of {module:engine/model/selection~Selection} is passed as selectable it will be moved to the target position (where the document selection should be moved after the insertion).

*
* // Insert text replacing given selection instance.
* const selection = new Selection( new Position( doc.getRoot(), [ 2 ] ), new Position( doc.getRoot(), [ 5 ] ) );
Copy link
Member

Choose a reason for hiding this comment

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

Let's maybe use a more human readable method, like – Position.createAt( doc.getRoot(), x ).

Also, I don't think that Selection( pos, pos ) is supported. You need to create a Range. So I'd go with something simpel like:

const selection = new Selection( paragraph, 'in' );

*
* editor.model.change( ( writer ) => {
Copy link
Member

Choose a reason for hiding this comment

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

No parenthesis around writer.

* editor.model.insertContent( model, writer.createText( 'x' ), selection );
*
* // insertContent() modifies the passed selection instance so it can be used to set the document selection.
* // Note: This is not necessary when you passed document selection to insertContent().
* writer.setSelection( selection );
Copy link
Member

Choose a reason for hiding this comment

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

// insertContent() modifies the passed selection instance so it can be used to set the document selection.
// Note: This is not necessary when you passed document selection to insertContent().

* } );
*
Copy link

Choose a reason for hiding this comment

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

Sample with range/position is missing.

* @fires insertContent
* @param {module:engine/model/documentfragment~DocumentFragment|module:engine/model/item~Item} content The content to insert.
* @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection} selection
* Selection into which the content should be inserted.
* @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection|
* module:engine/model/position~Position|module:engine/model/element~Element|
* Iterable.<module:engine/model/range~Range>|module:engine/model/range~Range|null} [selectable]
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't it supposed to default to the document selection?

If yes, make sure to remove editor.model.document.selection from examples in this doc string.

Copy link
Member

Choose a reason for hiding this comment

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

I see that you made this change. So you also need to update these docs.

Copy link
Member

Choose a reason for hiding this comment

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

Still missing the default value in the documentation:

[selectable=model.document.selection]

Copy link
Member

Choose a reason for hiding this comment

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

Bonus – I think that we should define a typedef called Selectable. This param is repeated in many places (e.g. the writer and Selection()). This can be a followup of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - I though about this also - I can make it here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

* Selection into which the content should be inserted. If not provided the current model document selection will be used.
Copy link

Choose a reason for hiding this comment

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

You can add a note that if you use a selection object, it will be modified and you will be able to get the insertion selection.

*/
insertContent( content, selection ) {
insertContent( this, content, selection );
insertContent( content, selectable ) {
insertContent( this, content, selectable );
}

/**
Expand Down
32 changes: 16 additions & 16 deletions src/model/selection.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,46 +35,46 @@ export default class Selection {
* or on the given {@link module:engine/model/element~Element element},
* or creates an empty selection if no arguments were passed.
*
* // Creates empty selection without ranges.
* // Creates empty selection without ranges.
* const selection = new Selection();
*
* // Creates selection at the given range.
* const range = new Range( start, end );
* const selection = new Selection( range );
*
* // Creates selection at the given ranges
* const ranges = [ new Range( start1, end2 ), new Range( star2, end2 ) ];
* const ranges = [ new Range( start1, end2 ), new Range( star2, end2 ) ];
* const selection = new Selection( ranges );
*
* // Creates selection from the other selection.
* // Note: It doesn't copies selection attributes.
* const otherSelection = new Selection();
* const selection = new Selection( otherSelection );
*
* // Creates selection from the given document selection.
* // Creates selection from the given document selection.
* // Note: It doesn't copies selection attributes.
* const documentSelection = new DocumentSelection( doc );
* const selection = new Selection( documentSelection );
*
* // Creates selection at the given position.
* // Creates selection at the given position.
* const position = new Position( root, path );
* const selection = new Selection( position );
*
* // Creates selection at the start position of the given element.
* // Creates selection at the start position of the given element.
* const paragraph = writer.createElement( 'paragraph' );
* const selection = new Selection( paragraph, offset );
*
* // Creates a range inside an {@link module:engine/model/element~Element element} which starts before the
* // first child of that element and ends after the last child of that element.
* // Creates a range inside an {@link module:engine/model/element~Element element} which starts before the
* // first child of that element and ends after the last child of that element.
* const selection = new Selection( paragraph, 'in' );
*
* // Creates a range on an {@link module:engine/model/item~Item item} which starts before the item and ends
* // just after the item.
* // Creates a range on an {@link module:engine/model/item~Item item} which starts before the item and ends
* // just after the item.
* const selection = new Selection( paragraph, 'on' );
*
* Selection's constructor allow passing additional options (`'backward'`) as the last argument.
*
* // Creates backward selection.
* // Creates backward selection.
* const selection = new Selection( range, { backward: true } );
*
* @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection|
Expand Down Expand Up @@ -326,32 +326,32 @@ export default class Selection {
* {@link module:engine/model/element~Element element}, {@link module:engine/model/position~Position position},
* {@link module:engine/model/range~Range range}, an iterable of {@link module:engine/model/range~Range ranges} or null.
*
* // Removes all selection's ranges.
* // Removes all selection's ranges.
* selection.setTo( null );
*
* // Sets selection to the given range.
* const range = new Range( start, end );
* selection.setTo( range );
*
* // Sets selection to given ranges.
* const ranges = [ new Range( start1, end2 ), new Range( star2, end2 ) ];
* const ranges = [ new Range( start1, end2 ), new Range( star2, end2 ) ];
* selection.setTo( ranges );
*
* // Sets selection to other selection.
* // Note: It doesn't copies selection attributes.
* const otherSelection = new Selection();
* selection.setTo( otherSelection );
*
* // Sets selection to the given document selection.
* // Sets selection to the given document selection.
* // Note: It doesn't copies selection attributes.
* const documentSelection = new DocumentSelection( doc );
* selection.setTo( documentSelection );
*
* // Sets collapsed selection at the given position.
* // Sets collapsed selection at the given position.
* const position = new Position( root, path );
* selection.setTo( position );
*
* // Sets collapsed selection at the position of the given node and an offset.
* // Sets collapsed selection at the position of the given node and an offset.
* selection.setTo( paragraph, offset );
*
* Creates a range inside an {@link module:engine/model/element~Element element} which starts before the first child of
Expand All @@ -365,7 +365,7 @@ export default class Selection {
*
* `Selection#setTo()`' method allow passing additional options (`backward`) as the last argument.
*
* // Sets backward selection.
* // Sets backward selection.
* const selection = new Selection( range, { backward: true } );
*
* @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection|
Expand Down
23 changes: 19 additions & 4 deletions src/model/utils/insertcontent.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,24 +13,39 @@ import Element from '../element';
import Range from '../range';
import log from '@ckeditor/ckeditor5-utils/src/log';
import DocumentSelection from '../documentselection';
import Selection from '../selection';

/**
* Inserts content into the editor (specified selection) as one would expect the paste
* functionality to work.
*
* If an instance of {module:engine/model/selection~Selection} is passed as `selectable` it will be modified
* to the insertion selection (equal to a range to be selected after insertion).
*
* **Note:** Use {@link module:engine/model/model~Model#insertContent} instead of this function.
* This function is only exposed to be reusable in algorithms
* which change the {@link module:engine/model/model~Model#insertContent}
* This function is only exposed to be reusable in algorithms which change the {@link module:engine/model/model~Model#insertContent}
* method's behavior.
*
* @param {module:engine/model/model~Model} model The model in context of which the insertion
* should be performed.
* @param {module:engine/model/documentfragment~DocumentFragment|module:engine/model/item~Item} content The content to insert.
* @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection} selection
* @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection|
* module:engine/model/position~Position|module:engine/model/element~Element|
* Iterable.<module:engine/model/range~Range>|module:engine/model/range~Range|null} [selectable]
* Selection into which the content should be inserted.
*/
export default function insertContent( model, content, selection ) {
export default function insertContent( model, content, selectable ) {
model.change( writer => {
let selection;

if ( !selectable ) {
selection = model.document.selection;
} else if ( selectable instanceof Selection || selectable instanceof DocumentSelection ) {
selection = selectable;
} else {
selection = new Selection( selectable );
}

if ( !selection.isCollapsed ) {
model.deleteContent( selection );
}
Expand Down
18 changes: 14 additions & 4 deletions tests/model/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ describe( 'Model', () => {

model.on( 'insertContent', spy );

model.insertContent( new ModelText( 'a' ), model.document.selection );
model.insertContent( new ModelText( 'a' ) );

expect( spy.calledOnce ).to.be.true;
} );
Expand All @@ -357,7 +357,7 @@ describe( 'Model', () => {

setData( model, '<paragraph>fo[]ar</paragraph>' );

model.insertContent( new ModelText( 'ob' ), model.document.selection );
model.insertContent( new ModelText( 'ob' ) );

expect( getData( model ) ).to.equal( '<paragraph>foob[]ar</paragraph>' );
} );
Expand All @@ -367,7 +367,17 @@ describe( 'Model', () => {

setData( model, '<paragraph>fo[]ar</paragraph>' );

model.insertContent( new ModelDocumentFragment( [ new ModelText( 'ob' ) ] ), model.document.selection );
model.insertContent( new ModelDocumentFragment( [ new ModelText( 'ob' ) ] ) );

expect( getData( model ) ).to.equal( '<paragraph>foob[]ar</paragraph>' );
} );

it( 'should use current model selection if no selectable passed', () => {
schema.register( 'paragraph', { inheritAllFrom: '$block' } );

setData( model, '<paragraph>fo[]ar</paragraph>' );

model.insertContent( new ModelText( 'ob' ) );

expect( getData( model ) ).to.equal( '<paragraph>foob[]ar</paragraph>' );
} );
Expand All @@ -377,7 +387,7 @@ describe( 'Model', () => {
setData( model, '<paragraph>[]</paragraph>' );

model.change( writer => {
model.insertContent( new ModelText( 'abc' ), model.document.selection );
model.insertContent( new ModelText( 'abc' ) );
expect( writer.batch.operations ).to.length( 1 );
} );
} );
Expand Down
96 changes: 90 additions & 6 deletions tests/model/utils/insertcontent.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import Selection from '../../../src/model/selection';
import Position from '../../../src/model/position';

import { setData, getData, parse } from '../../../src/dev-utils/model';
import Range from '../../../src/model/range';

describe( 'DataController utils', () => {
let model, doc;
Expand All @@ -26,7 +27,7 @@ describe( 'DataController utils', () => {
setData( model, 'x[]x' );

model.change( writer => {
insertContent( model, new Text( 'a' ), doc.selection );
insertContent( model, new Text( 'a' ) );
expect( writer.batch.operations ).to.length( 1 );
} );
} );
Expand All @@ -47,6 +48,89 @@ describe( 'DataController utils', () => {
} );
Copy link

Choose a reason for hiding this comment

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

Tests should check if the selection was modified.

} );

it( 'should modify passed selection instance', () => {
model = new Model();
doc = model.document;
doc.createRoot();

model.schema.extend( '$text', { allowIn: '$root' } );
setData( model, 'a[]bc' );

const selection = new Selection( new Position( doc.getRoot(), [ 2 ] ) );
const selectionCopy = new Selection( new Position( doc.getRoot(), [ 2 ] ) );

expect( selection.isEqual( selectionCopy ) ).to.be.true;

model.change( () => {
insertContent( model, new Text( 'x' ), selection );
Copy link
Member

Choose a reason for hiding this comment

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

Use the writer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest I've copied tests from other cases so should we change all the tests then?

Copy link
Member

Choose a reason for hiding this comment

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

Hm... I think so. Just to make sure it's going to work with how other people will use it and to avoid confusing people who may see our tests.

} );

expect( selection.isEqual( selectionCopy ) ).to.be.false;

const insertionSelection = new Selection( new Position( doc.getRoot(), [ 3 ] ) );
expect( selection.isEqual( insertionSelection ) ).to.be.true;
} );

it( 'should be able to insert content at custom position', () => {
model = new Model();
doc = model.document;
doc.createRoot();

model.schema.extend( '$text', { allowIn: '$root' } );
setData( model, 'a[]bc' );

const position = new Position( doc.getRoot(), [ 2 ] );

model.change( () => {
insertContent( model, new Text( 'x' ), position );
expect( getData( model ) ).to.equal( 'a[]bxc' );
} );
} );

it( 'should be able to insert content at custom range', () => {
model = new Model();
doc = model.document;
doc.createRoot();

model.schema.extend( '$text', { allowIn: '$root' } );
setData( model, 'a[]bc' );

const range = new Range( new Position( doc.getRoot(), [ 2 ] ), new Position( doc.getRoot(), [ 3 ] ) );

model.change( () => {
insertContent( model, new Text( 'x' ), range );
expect( getData( model ) ).to.equal( 'a[]bx' );
} );
} );

it( 'should be able to insert content at model selection if document selection is passed', () => {
model = new Model();
doc = model.document;
doc.createRoot();

model.schema.extend( '$text', { allowIn: '$root' } );
setData( model, 'a[]bc' );

model.change( () => {
insertContent( model, new Text( 'x' ), model.document.selection );
expect( getData( model ) ).to.equal( 'ax[]bc' );
} );
} );

it( 'should be able to insert content at model selection if none passed', () => {
model = new Model();
doc = model.document;
doc.createRoot();

model.schema.extend( '$text', { allowIn: '$root' } );
setData( model, 'a[]bc' );

model.change( () => {
insertContent( model, new Text( 'x' ) );
expect( getData( model ) ).to.equal( 'ax[]bc' );
} );
} );

it( 'accepts DocumentFragment', () => {
model = new Model();
doc = model.document;
Expand All @@ -56,7 +140,7 @@ describe( 'DataController utils', () => {

setData( model, 'x[]x' );

insertContent( model, new DocumentFragment( [ new Text( 'a' ) ] ), doc.selection );
insertContent( model, new DocumentFragment( [ new Text( 'a' ) ] ) );

expect( getData( model ) ).to.equal( 'xa[]x' );
} );
Expand All @@ -70,7 +154,7 @@ describe( 'DataController utils', () => {

setData( model, 'x[]x' );

insertContent( model, new Text( 'a' ), doc.selection );
insertContent( model, new Text( 'a' ) );

expect( getData( model ) ).to.equal( 'xa[]x' );
} );
Expand All @@ -90,7 +174,7 @@ describe( 'DataController utils', () => {

setData( model, '<paragraph>foo[]</paragraph>' );

insertContent( model, content, doc.selection );
insertContent( model, content );

expect( doc.getRoot().getChild( 0 ).getChild( 1 ) ).to.equal( content );
} );
Expand Down Expand Up @@ -303,7 +387,7 @@ describe( 'DataController utils', () => {
] );

setData( model, '[<heading2>foo</heading2>]' );
insertContent( model, content, doc.selection );
insertContent( model, content );
expect( getData( model ) ).to.equal( '<heading1>bar[]</heading1>' );
} );

Expand Down Expand Up @@ -854,6 +938,6 @@ describe( 'DataController utils', () => {
} );
}

insertContent( model, content, doc.selection );
insertContent( model, content );
}
} );