T/ckeditor5/1243: Unify widget insertion #1545

jodator · 2018-09-13T16:59:23Z

Suggested merge commit message (convention)

Other: The model.insertContent() accepts range and position. Closes ckeditor/ckeditor5#1243.

Additional information

Associated PRs:

API updates-only changes on branches (no PRs):
suggested merge message for list, autosave & clipboard:

Internal: Remove default parameter from `model.insertContent()` calls.

Above branch constellation is commited on t/1243 branch on ckeditor5 repo.

jodator · 2018-09-13T17:02:15Z

@pjasiun The code is ready to review. I'll need only to update BREAKING CHANGES entries in associated PRs.

coveralls · 2018-09-13T17:08:58Z

Coverage remained the same at 100.0% when pulling 3cb0b59 on t/ckeditor5/1243 into ee8eeb3 on master.

…ertContent()` needs to create `Selection` instance.

pjasiun · 2018-09-14T12:06:43Z

src/model/utils/insertcontent.js

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


or DocumentSelection

Unfortunately DocumentSelection do not extend Selection, because it has limited API.

pjasiun · 2018-09-14T12:17:25Z

src/model/model.js

+	 * @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. If not provided the current model document selection will be used.


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.

pjasiun · 2018-09-14T12:19:36Z

tests/model/utils/insertcontent.js

@@ -47,6 +48,52 @@ describe( 'DataController utils', () => {
 			} );


Tests should check if the selection was modified.

jodator · 2018-09-14T12:51:29Z

@pjasiun you can check the updates - I've added some examples to the docs but I'm not 100% sure if they are useful.

pjasiun · 2018-09-14T13:59:48Z

src/model/utils/insertcontent.js

+ *		// The selection contains a range to be selected - ie can be used to set selection.
+ *		model.change( ( writer ) => {
+ *			writer.setSelection( selection );
+ *		} );


I think these docs would be more useful for model.insertContent because this the API users will use.

pjasiun · 2018-09-14T14:00:50Z

src/model/utils/insertcontent.js

+ *		} );
+ *
+ *		// The selection contains a range to be selected - ie can be used to set selection.
+ *		model.change( ( writer ) => {


Both commands should be in the same model.change block.

Reinmar · 2018-09-14T14:14:25Z

src/model/model.js

+	 *		const selection = new Selection( new Position( doc.getRoot(), [ 2 ] ), new Position( doc.getRoot(), [ 5 ] ) );
+	 *
+	 *		editor.model.change( ( writer ) => {
+	 *			editor.model.insertContent( model, new Text( 'x' ), selection );


new Text() => writer.createText()

Reinmar · 2018-09-14T14:16:09Z

src/model/model.js

+	 *			editor.model.insertContent( model, new Text( 'x' ), selection );
+	 *
+	 *			// The selection contains a range to be selected - ie can be used to set selection.
+	 *			writer.setSelection( 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().

…tent()` from `insertContent()`.

Reinmar · 2018-09-14T14:17:56Z

src/model/model.js

-	 * 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]


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.

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

Still missing the default value in the documentation:

[selectable=model.document.selection]

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.

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

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

Reinmar · 2018-09-14T14:19:40Z

tests/model/utils/insertcontent.js

+			expect( selection.isEqual( selectionCopy ) ).to.be.true;
+
+			model.change( () => {
+				insertContent( model, new Text( 'x' ), selection );


Use the writer.

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

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.

Reinmar · 2018-09-14T14:20:05Z

src/model/model.js

+	 *		// Insert text replacing given selection instance.
+	 *		const selection = new Selection( new Position( doc.getRoot(), [ 2 ] ), new Position( doc.getRoot(), [ 5 ] ) );
+	 *
+	 *		editor.model.change( ( writer ) => {


No parenthesis around writer.

Reinmar · 2018-09-14T14:23:10Z

src/model/model.js

@@ -298,13 +298,28 @@ export default class Model {
 	 *
 	 *		editor.model.insertContent( modelFragment, editor.model.document.selection );
 	 *
+	 * 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).


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).

pjasiun · 2018-09-14T14:24:14Z

src/model/model.js

@@ -298,13 +298,29 @@ export default class Model {
 	 *
 	 *		editor.model.insertContent( modelFragment, editor.model.document.selection );


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

pjasiun · 2018-09-14T14:28:09Z

src/model/model.js

+	 *			// Note: This is not necessary when you passed document selection to insertContent().
+	 *			writer.setSelection( selection );
+	 *		} );
+	 *


Sample with range/position is missing.

…ption.

jodator · 2018-09-14T14:38:51Z

@Reinmar & @pjasiun I've updated the docs according to your comments. The remaing thing is with tests & @Reinmar's comment:

use writer

The added test were written similarly to other tests in that file so using writer there should update all the tests?

Reinmar · 2018-09-14T14:44:20Z

The added test were written similarly to other tests in that file so using writer there should update all the tests?

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.

jodator · 2018-09-14T14:47:02Z

@Reinmar - sure - I've updated the tests that were creating text inside model.change() blocks. So all issues are resolved.

Reinmar · 2018-09-14T15:11:33Z

src/model/model.js

-	 *		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.


this method

the document selection

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

Reinmar · 2018-09-14T15:11:44Z

src/model/model.js

+	 *
+	 * By default the method will use document selection but it can be also used with position, range or selection instances.
+	 *
+	 *		// Insert text at current document selection position.


the current

Reinmar · 2018-09-14T15:12:42Z

src/model/model.js

+	 *		// Insert text at current document selection position.
+	 *		editor.model.change( writer => {
+	 *			editor.model.insertContent( writer.createText( 'x' ) );
+	 *			// equal to editor.model.insertContent( model, writer.createText( 'x' ), editor.model.document.selection );


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

Reinmar · 2018-09-14T15:14:17Z

src/model/model.js

+	 *			// equal to editor.model.insertContent( model, writer.createText( 'x' ), editor.model.document.selection );
+	 *		} );
+	 *
+	 *		// Insert text at some position - model's selection will not be modified.


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.

Reinmar · 2018-09-14T15:19:02Z

src/model/model.js

+	 * 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 ] ) );


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' );

Reinmar

LGTM

jodator added 2 commits September 13, 2018 18:40

Docs: Fix code samples in model/selection.

7cd3f5e

Changed: The insertContent() should accept position & range.

97cb888

jodator requested a review from pjasiun September 13, 2018 16:59

This was referenced Sep 13, 2018

Unify widget insertion ckeditor/ckeditor5-media-embed#34

Merged

Unify widget insertion ckeditor/ckeditor5-image#232

Merged

Unify widget insertion ckeditor/ckeditor5-table#111

Merged

Unify widget insertion ckeditor/ckeditor5-widget#53

Merged

jodator added 2 commits September 14, 2018 10:57

Internal: Remove default parameter from model.insertContent() calls.

4c233e5

Use Selection class instead of DocumentSelection to check if `ins…

b91dc3b

…ertContent()` needs to create `Selection` instance.

jodator force-pushed the t/ckeditor5/1243 branch from 4045745 to b91dc3b Compare September 14, 2018 09:29

pjasiun reviewed Sep 14, 2018

View reviewed changes

pjasiun added the status:review- label Sep 14, 2018

jodator added 2 commits September 14, 2018 14:32

The insertContent() function should also work with DocumentSelection.

e0ba244

Docs: Update insertContent() documentation.

f18462a

jodator removed the status:review- label Sep 14, 2018

pjasiun reviewed Sep 14, 2018

View reviewed changes

Reinmar reviewed Sep 14, 2018

View reviewed changes

Docs: Move documentation about changing selection to `model.insertCon…

b00ca26

…tent()` from `insertContent()`.

jodator force-pushed the t/ckeditor5/1243 branch from 6b10326 to b00ca26 Compare September 14, 2018 14:16

Docs: Improve the model.insertContent() sample docs.

dcb0c54

Reinmar suggested changes Sep 14, 2018

View reviewed changes

pjasiun reviewed Sep 14, 2018

View reviewed changes

Docs: Add more examples to the model.insertContent(). Reword descri…

6163265

…ption.

Tests: Use writer.insertText in some insertContent() tests.

abfff1f

Small fixes in docs.

259bd57

pjasiun approved these changes Sep 14, 2018

View reviewed changes

Reinmar reviewed Sep 14, 2018

View reviewed changes

Docs: Update wording on model.insertContent() docs.

6e779db

Reinmar reviewed Sep 14, 2018

View reviewed changes

Docs: Use more human readable code examples for model.insertContent().

63e4f18

pomek mentioned this pull request Sep 17, 2018

Schema#schema.checkAttributeInSelection() checks attributes of text #1550

Merged

Docs: Add default values to the selectable in insertContent().

aa31f0f

Reinmar approved these changes Sep 17, 2018

View reviewed changes

jodator added 2 commits September 17, 2018 14:50

Merge branch 'master' into t/ckeditor5/1243

e95e695

Docs: Add information about insertContent event params.

3cb0b59

pjasiun merged commit bcdaaa9 into master Sep 18, 2018

pjasiun deleted the t/ckeditor5/1243 branch September 18, 2018 10:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/ckeditor5/1243: Unify widget insertion #1545

T/ckeditor5/1243: Unify widget insertion #1545

jodator commented Sep 13, 2018 •

edited

Loading

jodator commented Sep 13, 2018

coveralls commented Sep 13, 2018 •

edited

Loading

pjasiun Sep 14, 2018

pjasiun Sep 14, 2018

jodator Sep 14, 2018

pjasiun Sep 14, 2018

pjasiun Sep 14, 2018

jodator commented Sep 14, 2018

pjasiun Sep 14, 2018

pjasiun Sep 14, 2018

Reinmar Sep 14, 2018

Reinmar Sep 14, 2018

Reinmar Sep 14, 2018

Reinmar Sep 14, 2018

Reinmar Sep 14, 2018

Reinmar Sep 14, 2018

jodator Sep 17, 2018

jodator Sep 17, 2018

Reinmar Sep 14, 2018

jodator Sep 14, 2018

Reinmar Sep 14, 2018

Reinmar Sep 14, 2018

Reinmar Sep 14, 2018

pjasiun Sep 14, 2018

pjasiun Sep 14, 2018

jodator commented Sep 14, 2018

Reinmar commented Sep 14, 2018

jodator commented Sep 14, 2018 •

edited

Loading

Reinmar Sep 14, 2018

Reinmar Sep 14, 2018

Reinmar Sep 14, 2018

Reinmar Sep 14, 2018

Reinmar Sep 14, 2018

Reinmar left a comment

		@@ -47,6 +48,52 @@ describe( 'DataController utils', () => {
		} );

		@@ -298,13 +298,29 @@ export default class Model {
		*
		* editor.model.insertContent( modelFragment, editor.model.document.selection );

T/ckeditor5/1243: Unify widget insertion #1545

T/ckeditor5/1243: Unify widget insertion #1545

Conversation

jodator commented Sep 13, 2018 • edited Loading

Suggested merge commit message (convention)

Additional information

jodator commented Sep 13, 2018

coveralls commented Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodator commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodator commented Sep 14, 2018

Reinmar commented Sep 14, 2018

jodator commented Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar left a comment

Choose a reason for hiding this comment

jodator commented Sep 13, 2018 •

edited

Loading

coveralls commented Sep 13, 2018 •

edited

Loading

jodator commented Sep 14, 2018 •

edited

Loading