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

T/ckeditor5/1243: Unify widget insertion #1545

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
7cd3f5e
Docs: Fix code samples in model/selection.
jodator Sep 13, 2018
97cb888
Changed: The insertContent() should accept position & range.
jodator Sep 13, 2018
4c233e5
Internal: Remove default parameter from `model.insertContent()` calls.
jodator Sep 14, 2018
b91dc3b
Use `Selection` class instead of `DocumentSelection` to check if `ins…
jodator Sep 14, 2018
e0ba244
The insertContent() function should also work with DocumentSelection.
jodator Sep 14, 2018
f18462a
Docs: Update `insertContent()` documentation.
jodator Sep 14, 2018
b00ca26
Docs: Move documentation about changing selection to `model.insertCon…
jodator Sep 14, 2018
dcb0c54
Docs: Improve the model.insertContent() sample docs.
jodator Sep 14, 2018
6163265
Docs: Add more examples to the `model.insertContent()`. Reword descri…
jodator Sep 14, 2018
abfff1f
Tests: Use writer.insertText in some insertContent() tests.
jodator Sep 14, 2018
259bd57
Small fixes in docs.
Sep 14, 2018
6e779db
Docs: Update wording on model.insertContent() docs.
jodator Sep 14, 2018
63e4f18
Docs: Use more human readable code examples for model.insertContent().
jodator Sep 17, 2018
aa31f0f
Docs: Add default values to the selectable in insertContent().
jodator Sep 17, 2018
e95e695
Merge branch 'master' into t/ckeditor5/1243
jodator Sep 17, 2018
3cb0b59
Docs: Add information about `insertContent` event params.
jodator Sep 17, 2018
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
41 changes: 35 additions & 6 deletions src/model/model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ export default class Model {
*
* // insertContent() doesn't have to be used in a change() block. It can, though,
* // so this code could be moved to the callback defined above.
* editor.model.insertContent( docFrag, editor.model.document.selection );
* editor.model.insertContent( docFrag );
*
* Using `insertContent()` with HTML string converted to a model document fragment (similar to the pasting mechanism):
*
Expand All @@ -296,15 +296,44 @@ export default class Model {
* // which has a loosened schema.
* const modelFragment = editor.data.toModel( viewFragment );
*
* editor.model.insertContent( modelFragment, editor.model.document.selection );
* editor.model.insertContent( modelFragment );
*
* By default the method will use document selection but it can be also used with position, range or selection instances.
Copy link
Member

Choose a reason for hiding this comment

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

this method

the document selection

it can also be used with a position, range or selection instance

*
* // Insert text at current document selection position.
Copy link
Member

Choose a reason for hiding this comment

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

the current

* editor.model.change( writer => {
* editor.model.insertContent( writer.createText( 'x' ) );
* // equal to editor.model.insertContent( model, writer.createText( 'x' ), editor.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.

I think it's a bit unnecessary and makes it more visually complex. Plus, there's a mistake (the model param).

* } );
*
* // Insert text at some position - model's selection will not be modified.
Copy link
Member

Choose a reason for hiding this comment

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

at a given position

document selection

there are two kinds of model selection - static selection and document selection; the one you refer to is called the document selection.

* editor.model.change( writer => {
* editor.model.insertContent( writer.createText( 'x' ), new Position( doc.getRoot(), [ 2 ] ) );
* } );
*
* 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 => {
* editor.model.insertContent( 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.

Follow up: https://github.com/ckeditor/ckeditor5-engine/issues/1551

* 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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading