T/1555: Expose Position, Range and Selection factories. #1585

jodator · 2018-10-26T09:38:06Z

Suggested merge commit message (convention)

Type: Implement a builtin Position, Range and Selection factories. Closes ckeditor/ckeditor5#4427.

BREAKING CHANGE: The model Position.createAt() was removed from public API. Use writer.createPositionAt() instead.
BREAKING CHANGE: The model Position.createAfter() method was removed from public API. Use writer.createPositionAfter() instead.
BREAKING CHANGE: The model Position.createBefore() method was removed from public API. Use writer.createPositionBefore() instead.
BREAKING CHANGE: The model Position.createFromPosition() method was removed. Use writer.createPositionAt( position ) to create a new position instance.
BREAKING CHANGE: The model Position.createFromParentAndOffset() method was removed. Use writer.createPositionAt( parent, offset ) instead.

BREAKING CHANGE: The model Range.createIn() method was removed from public API. Use writer.createRangeIn() instead.
BREAKING CHANGE: The model Range.createOn() method was removed from public API. Use writer.createRangeOn() instead.
BREAKING CHANGE: The model Range.createFromRange() method was removed from public API.
BREAKING CHANGE: The model Range.createFromParentsAndOffsets() method was removed from public API.
BREAKING CHANGE: The model Range.createFromPositionAndShift() - method was removed from public API.
BREAKING CHANGE: The model Range.createCollapsedAt() - removed method was removed. Use writer.createRange( position ) to create collapsed range.

BREAKING CHANGE: The model Range.createFromRanges() method was removed from public API.

BREAKING CHANGE: The view Position.createAt() was removed from public API. Use writer.createPositionAt() instead.
BREAKING CHANGE: The view Position.createAfter() method was removed from public API. Use writer.createPositionAfter() instead.
BREAKING CHANGE: The view Position.createBefore() method was removed from public API. Use writer.createPositionBefore() instead.
BREAKING CHANGE: The view Position.createFromPosition() method was removed. Use writer.createPositionAt( position ) to create a new position instance.

BREAKING CHANGE: The view Range.createIn() method was removed from public API. Use writer.createRangeIn() instead.
BREAKING CHANGE: The view Range.createOn() method was removed from public API. Use writer.createRangeOn() instead.
BREAKING CHANGE: The view Range.createFromRange() method was removed from public API.
BREAKING CHANGE: The view Range.createFromPositionAndShift() - method was removed from public API.
BREAKING CHANGE: The view Range.createFromParentsAndOffsets() method was removed from public API.
BREAKING CHANGE: The view Range.createCollapsedAt() - removed method was removed. Use writer.createRange( position ) to create collapsed range.

Additional information

This PR requires T/1554: Model.insertContent() should work with item & related changes #1574 to be closed (branch Implement a builtin Position, Range and Selection factories ckeditor5#4427 is based on model.insertContentent( content, element ) does not work (and related issues) ckeditor5#4426 anyway)
This comes with constellation of PRs gathered at mgit.json on branch:
- alignment - PR
- autoformat - PR
- autosave - PR
- basic-styles - PR
- block-quote - PR
- clipboard - PR
- font - PR
- heading - PR
- highlight - PR
- image - PR
- link - PR
- list - PR
- media-embed - PR
- paragraph - PR
- table - PR
- typing - PR
- ui - PR
- undo - PR
- widget - PR

This PR might close ckeditor/ckeditor5#4426 as well as I had to base this PR on t/1554 branch. Might require adding some packages with ckeditor/ckeditor5#4426 only changes though.

Brief summary of changes:

Most of Position, Range and Selection imports (model/view) were removed from packages. AFAIR. Remaining engine imports. Most packages has now engine only as a devDependency in package.json - ps.: I've sorted some package.json entries cause my eyes were hurt.
Docs were reviewed for new Position() and Positon.createAt() usages in examples and were replaced by writer.createPositionAt() in most cases - some have model.createPostionAt() examples - where it had sense only. So the writer.createFoo() methods are preferred in the docs. AFAICS I've only left new Position() examples in the src/dev-utils/ examples as they are low-level anyway.
I've removed element instanceOf Element and similar checks from packages (not engine) and used supsect.is( 'element' ) checks instead.
I've removed some imports also in the engine - especially in the writer context (model.change() blocks)
5 I've added:

schema.createContext() method to create a context without import SchemaContext
model.createBatch() method for those 2 cases were new Batch() was needed (obviously I do not created writer.createBatch()).

Things to discuss:

I've failed with the .clone() and LiveRange.fromPosition() changes as in some cases methods could have both Position or LivePosition and I wasn't able to catch them all. But:

I've removed Position.createFromPosition() - it is LivePosition.fromPosition() and livePos.toPosition() now - I've replaced it were it was obious. Also you can copy position by Position.createAt( position ).

The writer.createPositionFromPath() is tooooo long IMHO. I'd call that writer.createPosition() - similarly to writer.createRange().
Range copy - right now we do not have writer methods for:

Range._createFromRange() - it could be exposed as writer.createRange( range ) alternative.
Range._createFromRanges() - this might be as well a utility function as it is only used in about 5 places in the engine.

I had minor conceputal issues with rewriting the new DocumentSelection() examples - mostly in the createSeleciton() docs. Does public API should/or will expose creator for this? Or we just remove lines about that?
I've created factory methods on model and then in the writer I use model instance. Obviously I've linked it properly (hope so) in docs. Now, for the view the DowncastWriter does not have a reference to the view so I could not reuse it. Probably a minor change to pass view to the writer but I wanted to ask you guys about such change. Right now the code is duplicated for view and downcastWriter.

…teFromPosition( position ) to clone position.

…mParentsAndOffsets().

…ods.

…tory methods.

# Conflicts: # src/model/operation/transform.js

coveralls · 2018-10-26T09:43:51Z

Coverage decreased (-5.9%) to 94.139% when pulling 15dd8c5 on t/1555 into fec045d on master.

jodator · 2018-10-26T12:12:45Z

Remaining engine imports (source code - not tests) from other packages (exluding core & editors):

import bindTwoStepCaretToAttribute from '@ckeditor/ckeditor5-engine/src/utils/bindtwostepcarettoattribute';

import HtmlDataProcessor from '@ckeditor/ckeditor5-engine/src/dataprocessor/htmldataprocessor';

import Observer from '@ckeditor/ckeditor5-engine/src/view/observer/observer';
import ClickObserver from '@ckeditor/ckeditor5-engine/src/view/observer/clickobserver';
import DomEventObserver from '@ckeditor/ckeditor5-engine/src/view/observer/domeventobserver';
import MouseObserver from '@ckeditor/ckeditor5-engine/src/view/observer/mouseobserver';

import DomEventData from '@ckeditor/ckeditor5-engine/src/view/observer/domeventdata';

import DomConverter from '@ckeditor/ckeditor5-engine/src/view/domconverter';

import { attachPlaceholder } from '@ckeditor/ckeditor5-engine/src/view/placeholder';

import { transformSets } from '@ckeditor/ckeditor5-engine/src/model/operation/transform';

import { downcastAttributeToElement } from '@ckeditor/ckeditor5-engine/src/conversion/downcast-converters';
import { downcastElementToElement } from '@ckeditor/ckeditor5-engine/src/conversion/downcast-converters';

import { upcastElementToAttribute } from '@ckeditor/ckeditor5-engine/src/conversion/upcast-converters';
import { upcastElementToElement } from '@ckeditor/ckeditor5-engine/src/conversion/upcast-converters';

import LivePosition from '@ckeditor/ckeditor5-engine/src/model/liveposition';
import LiveRange from '@ckeditor/ckeditor5-engine/src/model/liverange';

jodator · 2018-10-26T12:29:21Z

src/model/model.js

+	 *		const otherSelection = writer.createSelection();
+	 *		const selection = writer.createSelection( otherSelection );
+	 *
+	 *		// Creates selection from the given document selection.


This line bothers me - I'd wrote this as:

const documentSelection = doc.selection; const selection = writer.createSelection( documentSelection );

pjasiun · 2018-10-26T15:53:55Z

I reviewed how plugins use this API (I will check the engine on Monday) and... wow, that's huge! Good job @jodator!

What I like is that usually writer or model is available and it needs to be passed to the function only in a few cases.

What I do not like is the way how the selection is created from a position, for instance:

writer.setSelection( writer.createRange(
	writer.createPositionAt( root.getNodeByPath( [ 0 ] ), 2 )
) );

https://github.com/ckeditor/ckeditor5-basic-styles/pull/79/files#diff-b26fa587bf35fdff179cbebd6e3dff8bR228

I think we should have something like writer.setSelectionAt:

writer.setSelectionAt( root.getNodeByPath( [ 0 ] ), 2 );

The construction writer.setSelection( writer.createRange( writer.createPositionAt( ... ) ) ); appears often:

# Conflicts: # src/model/model.js # src/view/position.js # tests/model/position.js # tests/view/position.js

jodator · 2018-10-29T12:45:47Z

writer.setSelectionAt( root.getNodeByPath( [ 0 ] ), 2 );

Nah, just use writer.setSelection( node, offset ) as this is already supported. I can review usage of such construct though.

jodator · 2018-10-29T13:30:35Z

All engine tests passes locally - Right now engine requires parapraph which requires engine which requires paragraph .... hence the errors on Travis 😞

# Conflicts: # src/model/model.js

pjasiun · 2018-10-29T14:01:23Z

src/view/documentselection.js

@@ -35,19 +35,19 @@ export default class DocumentSelection {
 	 *		const selection = new DocumentSelection();
 	 *
 	 *		// Creates selection at the given range.
-	 *		const range = new Range( start, end );
+	 *		const range = writer.createSelection( start, end );


const range = writer.createSelection( start, end );

There is something wrong here.

pjasiun · 2018-10-29T15:52:32Z

Things to discuss:

I've failed with the .clone() and LiveRange.fromPosition() changes as in some cases methods could have both Position or LivePosition and I wasn't able to catch them all. But:

I've removed Position.createFromPosition() - it is LivePosition.fromPosition() and livePos.toPosition() now - I've replaced it were it was obious. Also you can copy position by Position.createAt( position ).

I notice it reviewing engine code. And I need to say that it is a little bit messy now. We have LivePosition.fromPosition vs Range._createFromRange vs no cloning method for the position now. What we agreed, clone method for Position and Range and(from|to)(Position|Range) methods in LivePosition and LiveRange would be great.

The writer.createPositionFromPath() is tooooo long IMHO. I'd call that writer.createPosition() - similarly to writer.createRange().

I think it is good that this method is long. It should be used only in special cases, so the long name properly suggestion that this should not be your first choice.

Range copy - right now we do not have writer methods for:

Range._createFromRange() - it could be exposed as writer.createRange( range ) alternative.

As mentioned before, I prefer range.clone().

Range._createFromRanges() - this might be as well a utility function as it is only used in about 5 places in the engine.

That's a good idea, but we can keep it as internal engine method at the moment.

I had minor conceptual issues with rewriting the new DocumentSelection() examples - mostly in the createSeleciton() docs. Does public API should/or will expose creator for this? Or we just remove lines about that?

We should not expose DocumentSelection constructor. There should be only a single document selection (doc.selection), so your suggestion to change docs is good.

I've created factory methods on model and then in the writer I use model instance. Obviously, I've linked it properly (hope so) in docs. Now, for the view the DowncastWriter does not have a reference to the view so I could not reuse it. Probably a minor change to pass view to the writer but I wanted to ask you guys about such change. Right now the code is duplicated for view and downcastWriter.

I think we can keep it the way it is now. Users are able to create view writers whenever when need it and I think that better docs are not a good enough reason to add view instance to the constructor.

Also, these methods should be added to upcastWriter.

jodator · 2018-10-30T10:10:11Z

I notice it reviewing engine code. And I need to say that it is a little bit messy now. We have LivePosition.fromPosition vs Range._createFromRange vs no cloning method for the position now. What we agreed, clone method for Position and Range and(from|to)(Position|Range) methods in LivePosition and LiveRange would be great.

@pjasiun again - many methods in Position assumes instance of Position not LivePosition. The must must Postion._createAt( positionOrLivePosition ) to not change LivePosition path - it will trigger events and some loops will occur (probably). So actual position.clone() to be used must be in context where you are 100% sure that you have position. So now I'm not 100% sure that we need position.clone() method.

jodator · 2018-10-30T15:18:10Z

@pjasiun again - many methods in Position assumes instance of Position not LivePosition. The must must Postion._createAt( positionOrLivePosition ) to not change LivePosition path - it will trigger events and some loops will occur (probably). So actual position.clone() to be used must be in context where you are 100% sure that you have position. So now I'm not 100% sure that we need position.clone() method.

I think that I've done my best to overcome this - I've just put new Range() when the Range class is needed - special cases.

jodator · 2018-10-30T15:44:56Z

@pjasiun & @Reinmar I think I've included all the suggestions from the comments. Feel free to merge ;)

jodator added 20 commits October 9, 2018 12:40

WiP: Introduce Position, Range & Selection APIs on model & model/writer.

2cf767c

Changed: Make factory methods of model Position protected.

17626ed

Changed: Use Position._createAt( position ) instead of Position._crea…

a9c850d

…teFromPosition( position ) to clone position.

Changed: Make Range factory methods protected. Remove Range.createFro…

210bac5

…mParentsAndOffsets().

Changed: Make view Position & Range factory methods protected.

1dc6154

Other: Introduce ViewPosition.getWalker().

dbe76e4

Docs: Update documentation on Position/Range/Selection factory methods.

a3d34e7

Other: Remove unused Range.createCollapsedAt().

4ade501

Docs: Update protected model factory methods in Position & Range.

5edb9e6

Docs: Remove Position._createAt() references from docs.

bf7c01f

Update model/view position docs and tests.

ed0605b

Update model/view range docs and tests.

7c49b92

Update model/view selection docs and tests.

891bb37

Create stub tests for model Position/Writer/Selection factory methods.

ceac18a

Create stub tests for writer's Position/Writer/Selection factory meth…

237538a

…ods.

Create stub tests for downcast writer's Position/Writer/Selection fac…

0956136

…tory methods.

Create stub tests for views's Position/Writer/Selection factory methods.

0e424d3

Add tests for editableElement.is( 'editableElement' ).

2b6e101

Add tests for schema.createContext().

8e0fc9e

Merge branch 'master' into t/1555

6c1e160

# Conflicts: # src/model/operation/transform.js

jodator added 2 commits October 26, 2018 12:50

Fix code after merging master.

dcd33cd

Use writer to create Ranges/Positions in private API.

5ee057b

jodator commented Oct 26, 2018

View reviewed changes

Reinmar self-requested a review October 26, 2018 12:39

Remove ViewRange import from downcast-converts.

d0e54f1

jodator changed the title ~~T/1555: WiP~~ T/1555: Expose Position, Range and Selection factories. Oct 26, 2018

Merge branch 'master' into t/1555

6714db2

# Conflicts: # src/model/model.js # src/view/position.js # tests/model/position.js # tests/view/position.js

Review writerSetSelection() API calls with single position.

d73756f

Merge branch 'master' into t/1555

98422ea

# Conflicts: # src/model/model.js

pjasiun reviewed Oct 29, 2018

View reviewed changes

jodator added 2 commits October 30, 2018 13:03

Add model/view Position.clone() method.

d3644a9

Add model/view Range.clone() method.

f7b4ac8

jodator added 3 commits October 30, 2018 16:22

Remove new DocumentSelection from docs.

e6b1f99

Add Position/Range/Selection factory methods to UpcastWriter.

a40fc17

Fix document selection examples.

6c14fba

Docs: A lot of corrections all across the docs.

15dd8c5

Reinmar merged commit e7f8467 into master Nov 1, 2018

Reinmar deleted the t/1555 branch November 1, 2018 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/1555: Expose Position, Range and Selection factories. #1585

T/1555: Expose Position, Range and Selection factories. #1585

jodator commented Oct 26, 2018 •

edited by pomek

Loading

coveralls commented Oct 26, 2018 •

edited

Loading

jodator commented Oct 26, 2018

jodator Oct 26, 2018

pjasiun commented Oct 26, 2018

jodator commented Oct 29, 2018

jodator commented Oct 29, 2018

pjasiun Oct 29, 2018

pjasiun commented Oct 29, 2018

jodator commented Oct 30, 2018

jodator commented Oct 30, 2018

jodator commented Oct 30, 2018

T/1555: Expose Position, Range and Selection factories. #1585

T/1555: Expose Position, Range and Selection factories. #1585

Conversation

jodator commented Oct 26, 2018 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

coveralls commented Oct 26, 2018 • edited Loading

jodator commented Oct 26, 2018

jodator Oct 26, 2018

Choose a reason for hiding this comment

pjasiun commented Oct 26, 2018

jodator commented Oct 29, 2018

jodator commented Oct 29, 2018

pjasiun Oct 29, 2018

Choose a reason for hiding this comment

pjasiun commented Oct 29, 2018

jodator commented Oct 30, 2018

jodator commented Oct 30, 2018

jodator commented Oct 30, 2018

jodator commented Oct 26, 2018 •

edited by pomek

Loading

coveralls commented Oct 26, 2018 •

edited

Loading