T/897: Make Position/Range immutable. #1175

jodator · 2017-10-25T11:21:51Z

Suggested merge commit message (convention)

Other: Make Position and Range immutable in model and view. Closes ckeditor/ckeditor5#4032.

BREAKING CHANGE: model.Position#offset and view.Position#offset now have only a getter. To modify offset use Postion#getShiftedBy() or Position#getShiftedTo().

BREAKING CHANGE: model.Position#path and view.Position#path now have only a getter and are immutable.

BREAKING CHANGE: model.Range#start and model.Range#end properties now have only a getter.

BREAKING CHANGE: view.Range#start and view.Range#end properties now have only a getter.

Additional information

This PR required minor changes in other repos (centralized in: https://github.com/ckeditor/ckeditor5/tree/t/ckeditor5-engine/897)

Merge commit message for related branches:

Internal: Aligned Positions and Ranges usage to the latest API of the framework (see ckeditor/ckeditor5#4032).

Other:

Main idea behid this PR is to reduce the need for creating new Position/Range objects when using it in other methods. Now those object can be safely passed around without side effects.
The remove() method in view/writer still does something with passed

scofalik · 2017-11-09T08:16:45Z

I just started to review this, but I wonder if it will be a positive change at all. AFAICS now positions will be created in some scenarios when they weren't, pretty much a lot in OT.

Edit: Maybe when we will refactor OT we will be able to introduce methods that work on paths directly instead of positions and ranges, to reduce the number of created Position and Range instances.

scofalik · 2017-11-09T08:40:51Z

src/model/operation/transform.js

@@ -179,25 +179,29 @@ const ot = {
 				// Take the start and the end of the range and transform them by deletion of moved nodes.
 				// Note that if rangeB was inside AttributeOperation range, only difference.end will be transformed.
 				// This nicely covers the joining simplification we did in the previous step.
-				difference.start = difference.start._getTransformedByDeletion( b.sourcePosition, b.howMany );
-				difference.end = difference.end._getTransformedByDeletion( b.sourcePosition, b.howMany );
+				const range = new Range(


Please change this const name to differenceTransformed.

scofalik · 2017-11-09T08:41:11Z

src/model/operation/transform.js

 			}

 			if ( common !== null ) {
 				// Here we do not need to worry that newTargetPosition is inside moved range, because that
 				// would mean that the MoveOperation targets into itself, and that is incorrect operation.
 				// Instead, we calculate the new position of that part of original range.
-				common.start = common.start._getCombined( b.sourcePosition, b.getMovedRangeStart() );
-				common.end = common.end._getCombined( b.sourcePosition, b.getMovedRangeStart() );
+				const range = new Range(


commonTransformed.

scofalik · 2017-11-09T09:04:22Z

src/model/position.js

 		// This position can't be affected if deletion was in a different root.
 		if ( this.root != deletePosition.root ) {
-			return transformed;
+			return this;


Are we sure this is not harmful in any way? I know that positions are now immutable but this method, by name, suggests that a new instance is returned. I think we don't have (and maybe won't have) such case in code, but one thing that comes to my mind is storing values in Map with Positions as keys.

Yeah.. maybe I've gone too deep into removing new instances. Here might be better to always create a copy.

scofalik · 2017-11-09T09:08:10Z

src/model/position.js

 		// This position can't be affected if insertion was in a different root.
 		if ( this.root != insertPosition.root ) {
-			return transformed;
+			return this;


Same as above.

scofalik · 2017-11-09T09:09:01Z

src/model/position.js

+		};
+	}
+
+	/**
 	 * Returns a copy of this position that is updated by removing `howMany` nodes starting from `deletePosition`.


Docs might need an update if we won't be returning a copy in every scenario (see comment in code below).

scofalik · 2017-11-09T09:11:37Z

src/model/position.js

+// @private
+// @param {Array.<Number>} path Position path. See {@link module:engine/model/position~Position#path}.
+function getOffset( path ) {
+	return last( path ) || 0;


Why || 0? Wouldn't undefined be better?

Anyway, it is an incorrect situation if path has no items. So maybe just return last( path );. I am not even sure we need this helper if it is that easy.

I didn't want to change logic behind this AFAIR. It was:

get offset() { return last( path ) || 0; }

I don't know if it would have any side effects or not.

Weird. Position always has to have an offset :S. Try if anything breaks without it ;).

@scofalik: I was able to fail a test after removing || 0:

expect( new Position( root, [ false ] ) ).to.have.property( 'offset' ).that.equals( 0 ); expect( new Position( root, [ undefined ] ) ).to.have.property( 'offset' ).that.equals( 0 );

But this looks like a bad API use - so we might as well drop || 0 check as position accepts only non-empty array as path.

Yeah, I'd remove || 0 and the test.

scofalik · 2017-11-09T09:44:43Z

src/model/range.js

@@ -804,13 +829,13 @@ export default class Range {
 		// 4. At this moment we don't need the original range.
 		// We are going to modify the result and we need to return a new instance of Range.
 		// We have to create a copy of the reference range.
-		const result = new this( ref.start, ref.end );
+		let result = new this( ref.start, ref.end );


AFAICS, range result is not used directly, just its start and end properties. Maybe we could just go with storing Positions and it would be better?

scofalik · 2017-11-09T09:46:50Z

src/model/treewalker.js

@@ -204,33 +204,37 @@ export default class TreeWalker {
 	 */
 	_next() {
 		const previousPosition = this.position;


Maybe we can drop position variable in this function? It is confusing and I am not really sure it is needed.

scofalik · 2017-11-09T09:48:45Z

src/model/treewalker.js

@@ -278,27 +285,31 @@ export default class TreeWalker {
 	 */
 	_previous() {


Same with position here.

scofalik · 2017-11-09T09:57:19Z

src/view/treewalker.js

@@ -187,7 +187,7 @@ export default class TreeWalker {
 	 * @returns {module:engine/view/treewalker~TreeWalkerValue} return.value Information about taken step.
 	 */
 	_next() {
-		let position = Position.createFromPosition( this.position );
+		let position = this.position;


Same with position.

scofalik · 2017-11-09T09:57:30Z

src/view/treewalker.js

@@ -292,7 +293,7 @@ export default class TreeWalker {
 	 * @returns {module:engine/view/treewalker~TreeWalkerValue} return.value Information about taken step.
 	 */
 	_previous() {
-		let position = Position.createFromPosition( this.position );
+		let position = this.position;


Same with position.

scofalik · 2017-11-09T10:06:42Z

All in all, good job! @Reinmar can you check what impact does it have on the original issue?

scofalik · 2017-11-09T10:09:29Z

Could you make PRs out of commits in other repos?

scofalik · 2017-11-09T10:15:24Z

@Reinmar @jodator Original issue described problem with copying selection. Unfortunately, this PR only partially fixes it. Selection#_pushRange is still the same. One of the reasons is that we don't want people to push LiveRanges to Selection. So there is still Range#createFromRange() there. What do we do with it?

Edit: sorry, I got confused -- above concerns model.Selection, the issue was about view.Selection (I think so, as there is no getTrimmed() method in model.Range).

Edit2: So I guess, that this should change too:

* getRanges() {
	for ( const range of this._ranges ) {
		yield Range.createFromRange( range );
	}
}

Both in model.Selection and view.Selection?

Same with getFirstRange and getLastRange.

pjasiun · 2017-11-09T10:27:03Z

src/model/position.js

+ * @param {module:engine/model/position~Position} position Position of which path should be modified.
+ * @param {Array.<Number>} path Position path. See {@link module:engine/model/position~Position#path}.
+ */
+export function setPath( position, path ) {


I would make this a protected class method.

I have mixed feelings about this.

From one point we don't want to mess around with position path. On the other hand, though, we need this or LivePosition won't work (there is an exactly same problem with LiveRange#start and LiveRange#end).

If these are protected methods then we don't have to really refactor some parts of the code. Just use the new protected methods! But is this really the way to go?

So IDK, I am torn. I liked this as a utility function because it is more discouraging to use it. But IDK. Maybe LiveRange should be in the same file as Range. Then we would just use the symbol defined in range.js.

For me, it the matter of convention. We have classes with protected methods exposed for other related classes in modules. For instance:

ckeditor5-engine/src/model/operation/insertoperation.js

Line 84 in 79da6fb

_execute() {

We already had a talk about exposing protected methods for child classes at some point of designing editor class. We never did it as a separate export.

I know what is the purpose of @protected tag and how we use it. The question is whether we want to have such @protected method at all. Because if we do, we can revert (or rather re-do) many of the changes proposed in this PR (at least in OT and tree walkers as these are used multiple time consecutively).

Now I see that also there is a symbol used for the property. I would go with the @protected property (without a symbol) and public getters. We have such pair already in some places in the code. We can even write that the property is exposed only for LiveRange and should not be used anywhere.

pjasiun · 2017-11-09T11:00:06Z

src/model/position.js

@@ -13,6 +13,9 @@ import compareArrays from '@ckeditor/ckeditor5-utils/src/comparearrays';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 import Text from './text';

+const _path = Symbol( 'path' );
+const _root = Symbol( 'root' );


We don't use symbols for private properties. @private is enough.

It is not really @private, it is read-only. Also, we do use Symbols in a couple of places. I think these are fine here.

But symbols make them private too.

But symbols make them private too.

A bit better hidden but also accessible (more work required though...) also not enumerable.

But I get that in CKEditor 5 there is already established convention and using straightforward this._path in child classes would be simpler.

I only don like "duplication" on object as the getter this.path would just return this._path.

Feel free to create a separate ticket to change it, but since we already do it this way, we should follow the convention:

ckeditor5-engine/src/model/rootelement.js

Lines 53 to 55 in 79da6fb

get document() {

return this._doc;

}

ckeditor5-engine/src/view/selection.js

Lines 107 to 119 in 79da6fb

get isFake() {

return this._isFake;

}

/**

* Returns fake selection label.

*

* @see #setFake

* @returns {String}

*/

get fakeSelectionLabel() {

return this._fakeSelectionLabel;

}

ckeditor5-engine/src/view/text.js

Lines 58 to 60 in 79da6fb

get data() {

return this._data;

}

I never liked this._data and was wondering why it is this way. I guess I will like to change it after more important subjects are dealt with.

Reinmar · 2017-11-09T15:44:58Z

Please don't merge this PR before we'll release alpha.2. It's a bit risky change.

Reinmar · 2017-11-16T12:46:44Z

Since alpha.2 is out, please conclude this PR and let us know when we could have a final check on it.

…ition and Range.

…e.createFromRanges().

jodator · 2017-11-17T08:27:53Z

 * getRanges() {
	for ( const range of this._ranges ) {
		yield Range.createFromRange( range );
	}
}
Both in model.Selection and view.Selection?

Same with getFirstRange and getLastRange.

So probably directe getters are to be created as it would make no sense for getBlabla methods?

scofalik · 2017-11-17T08:31:46Z

No matter what getRanges() will return, it should not let modify selection ranges without using selection API. So:

get ranges() {
    return this._ranges;
}

Is an incorrect solution if you ask me.

As for getFirstRange and getLastRange. Making it function get firstRange() { is "a bit risky" IMHO. Having it look like a property conceals the fact that the function does something more behind scenes (here iterating through all ranges in selection and comparing them). On the other hand, in 99% cases there would be just one range in selection. So I will be fine however those will end up.

jodator · 2017-11-17T08:36:55Z

@scofalik

return [ ...this._ranges ];

?

scofalik · 2017-11-17T08:37:54Z

Technically it is fine. Although it was a generator function before, and some people on this project love their generators...

jodator · 2017-11-17T11:15:09Z

OK so what to do with those iterators?

I'll grab 🍿 and let you @pjasiun ⚔️ @scofalik fight over removing or not iterators :)

ps.: I'm for returning new array:

get ranges() {
    return [ ...this._ranges ];

to not mess with internal array.

scofalik · 2017-11-17T11:25:11Z

I am for returning array, because in this case you always want to handle all of them. If not, there are already getFirstRange() and getLastRange() methods.

pjasiun · 2017-11-20T13:41:13Z

I would go with:

get ranges() {
    return this. _ranges[ Symbol.iterator ]();
}

This way you will not create any new object and it will be still not mutable.

…() and getLastRange() methods.

jodator · 2017-11-20T14:48:16Z

@scofalik I've done with all the suggested changes.

I've also removed Range.createFromRange() from getFirst/LastRange() and updated docs for those methods.

scofalik · 2017-11-24T09:55:36Z

src/model/range.js

@@ -291,8 +311,11 @@ export default class Range {
 				ranges.push( new Range( pos, pos.getShiftedBy( howMany ) ) );
 			}

-			pos.path = pos.path.slice( 0, -1 );
-			pos.offset++;
+			const path = pos.getParentPath();


I think this is same as Position.createAfter( pos.parent )?

scofalik · 2017-11-24T10:03:22Z

src/model/range.js

@@ -804,13 +839,14 @@ export default class Range {
 		// 4. At this moment we don't need the original range.
 		// We are going to modify the result and we need to return a new instance of Range.
 		// We have to create a copy of the reference range.
-		const result = new this( ref.start, ref.end );
+		let start = ref.start;
+		let end = ref.end;

 		// 5. Ranges should be checked and glued starting from the range that is closest to the reference range.
 		// Since ranges are sorted, start with the range with index that is closest to reference range index.
 		for ( let i = refIndex - 1; i >= 0; i++ ) {


I wonder how the hell this works if the condition is i >= 0 and i is increment in each loop. Seems wrong.

scofalik · 2017-11-24T10:12:05Z

src/view/position.js

-		 * @member {Number} module:engine/view/position~Position#offset
-		 */
-		this.offset = offset;
+		this[ _parent ] = parent;


Didn't we agree that this should be this._parent and this._offset?

scofalik · 2017-11-24T10:14:39Z

src/view/range.js

-		 * @member {module:engine/view/position~Position}
-		 */
-		this.end = end ? Position.createFromPosition( end ) : Position.createFromPosition( start );
+		this[ _start ] = start;


this._parent and this._offset

scofalik · 2017-11-24T10:17:50Z

src/view/writer.js

-	const mergePosition = mergeAttributes( breakStart );
-	range.start = mergePosition;
-	range.end = Position.createFromPosition( mergePosition );
+	mergeAttributes( breakStart );


Note to self: check how mergeAttributes works now.

scofalik · 2017-11-24T13:09:42Z

I've pushed some minor changes.

scofalik · 2017-11-24T13:17:33Z

Hm, for some reason it broke: https://travis-ci.org/ckeditor/ckeditor5-engine/builds/306753449?utm_source=email&utm_medium=notification although tests pass for engine for me locally.

This reverts commit 836dfd8, reversing changes made to fc7da80.

Reinmar · 2017-11-24T16:22:02Z

I had to revert this change because it completely blows up editor tests.

Also, see https://github.com/ckeditor/ckeditor5-engine/issues/897#issuecomment-346862873.

scofalik · 2017-11-27T07:46:55Z

What do you mean exactly by "blows up editor tests"? Performance issues or something else?

Reinmar · 2017-11-27T09:16:59Z

What do you mean exactly by "blows up editor tests"? Performance issues or something else?

Many failing tests in lists, images and undo.

This reverts commit a504fbd

jodator added 8 commits October 23, 2017 16:50

Other: Make Position immutable.

9548f45

Other: Make Range immutable.

b67f106

Other: Remove non semantic methods from Position.

6d36119

Other: Remove redundant Position.createFromPosition calls.

d3a3645

Other: Make view Position immutable.

2fc0f81

Other: Remove excessive view Position.createFromPosition calls.

9f3a6aa

Other: Make view Range immutable.

8eae307

Docs: Update writer.remove() method docs.

74eca6a

scofalik self-requested a review October 25, 2017 12:23

scofalik reviewed Nov 9, 2017

View reviewed changes

src/model/treewalker.js

@@ -278,27 +285,31 @@ export default class TreeWalker {

*/

_previous() {

Copy link

Contributor

scofalik Nov 9, 2017

Choose a reason for hiding this comment

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

Same with position here.

scofalik reviewed Nov 9, 2017

View reviewed changes

scofalik added the status:review- label Nov 9, 2017

pjasiun reviewed Nov 9, 2017

View reviewed changes

Code style: Name variables accordingly in operation/transform.js.

1aaa955

jodator added 3 commits November 17, 2017 09:02

Code style: Don't use Symbols for protected properties in model's Pos…

ed73838

…ition and Range.

Other: User positions rather then creating new range in loops in Rang…

010cfc6

…e.createFromRanges().

Code style: Remove confusing position variable from TreeWalker methods.

f076096

Other: Remove redundant check from getOffset in Position.

3c496a5

jodator added 2 commits November 20, 2017 15:26

Merge branch 'master' into t/897

0104ef7

Other: Remove range copying from selection getRanges(), getFirstRange…

44553fe

…() and getLastRange() methods.

jodator removed the status:review- label Nov 20, 2017

scofalik reviewed Nov 24, 2017

View reviewed changes

Changed: Minor fixes.

a2db1b0

scofalik merged commit 836dfd8 into master Nov 24, 2017

Reinmar added a commit that referenced this pull request Nov 24, 2017

Revert "Merge pull request #1175 from ckeditor/t/897"

a504fbd

This reverts commit 836dfd8, reversing changes made to fc7da80.

jodator added a commit that referenced this pull request Nov 28, 2017

Revert "Revert "Merge pull request #1175 from ckeditor/t/897""

9a01b9d

This reverts commit a504fbd

jodator mentioned this pull request Nov 28, 2017

Fixed T/897: Make Position/Range immutable. #1192

Closed

		@@ -278,27 +285,31 @@ export default class TreeWalker {
		*/
		_previous() {

	get isFake() {
	return this._isFake;
	}

	/**
	* Returns fake selection label.
	*
	* @see #setFake
	* @returns {String}
	*/
	get fakeSelectionLabel() {
	return this._fakeSelectionLabel;
	}

T/897: Make Position/Range immutable. #1175

T/897: Make Position/Range immutable. #1175

Conversation

jodator commented Oct 25, 2017 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

scofalik commented Nov 9, 2017 • 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

Choose a reason for hiding this comment

scofalik Nov 9, 2017 • 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

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

scofalik commented Nov 9, 2017

scofalik commented Nov 9, 2017

scofalik commented Nov 9, 2017 • 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

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

Reinmar commented Nov 9, 2017

Reinmar commented Nov 16, 2017 • edited Loading

jodator commented Nov 17, 2017 • edited Loading

scofalik commented Nov 17, 2017 • edited Loading

jodator commented Nov 17, 2017

scofalik commented Nov 17, 2017 • edited Loading

jodator commented Nov 17, 2017

scofalik commented Nov 17, 2017

pjasiun commented Nov 20, 2017

jodator commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scofalik Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scofalik Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

scofalik commented Nov 24, 2017

scofalik commented Nov 24, 2017

Reinmar commented Nov 24, 2017

scofalik commented Nov 27, 2017

Reinmar commented Nov 27, 2017

jodator commented Oct 25, 2017 •

edited by pomek

Loading

scofalik commented Nov 9, 2017 •

edited

Loading

scofalik Nov 9, 2017 •

edited

Loading

scofalik commented Nov 9, 2017 •

edited

Loading

Reinmar commented Nov 16, 2017 •

edited

Loading

jodator commented Nov 17, 2017 •

edited

Loading

scofalik commented Nov 17, 2017 •

edited

Loading

scofalik commented Nov 17, 2017 •

edited

Loading

scofalik Nov 24, 2017 •

edited

Loading

scofalik Nov 24, 2017 •

edited

Loading