T/1002: Introduce Position#getCommonAncestor( otherPosition ) and Range#getCommonAncestor() #1005

pomek · 2017-07-07T13:58:39Z

Suggested merge commit message (convention)

Feature: Introduced Position#getCommonAncestor( position ) and Range#getCommonAncestor() methods for view and model. Closes ckeditor/ckeditor5#4105.

…etCommonAncestor()` methods for `view` and `model`.

Reinmar · 2017-07-07T14:02:46Z

src/model/position.js

+	 * of these two positions must be identical.
+	 *
+	 * @param {module:engine/model/position~Position} position The second position.
+	 * @returns {module:engine/model/node~Node|null} Node that contains both positions.


DocumentFragment is not a node. So the correct notation is Element|DF.

Reinmar · 2017-07-07T14:04:49Z

src/model/position.js

+			return null;
+		}
+
+		const ancestorsA = this.getAncestors();


I liked the previous implementation more. The one which reused getCommonPath(). It was shorter and more readable.

Let me guess – you decided to skip it because DF doesn't implement getNodeByPath()? ;) Why not borrowing it from Element for DF?

You are right :D

If it does not a problem, I could do it as you said.

It will make a lot of sense to unify the APIs of Element and DF, so it's definitely good to add this method there.

Reinmar · 2017-07-07T14:05:01Z

src/model/range.js

@@ -446,6 +446,15 @@ export default class Range {
 	}

 	/**
+	 * Returns a {@link module:engine/model/node~Node} which is a common ancestor for both positions.
+	 *
+	 * @returns {module:engine/model/node~Node|null}


Again, wrong type.

Reinmar · 2017-07-07T14:06:57Z

src/view/position.js

+	 * @returns {module:engine/view/node~Node|null}
+	 */
+	getCommonAncestor( position ) {
+		const ancestorsA = this.getAncestors();


getAncestors() accept options.includeNode so will this work in such a case: <p>x{y}z</p>? For such positions the text node should be returned. And for such <p>[x]</p> the element.

The second issue – you didn't pass options.parentFirst too so how can you iterate from i==0 (leafs)? What if one ancestors array is longer than the other?

Reinmar · 2017-07-07T14:09:09Z

src/view/position.js

+	 * Returns a {@link module:engine/view/node~Node} which is a common ancestor for both positions.
+	 *
+	 * @param {module:engine/view/position~Position} position
+	 * @returns {module:engine/view/node~Node|null}


In this case it should be DF|Node because here we can also get text nodes.

Reinmar · 2017-07-07T14:09:56Z

tests/model/position.js

@@ -844,4 +844,46 @@ describe( 'Position', () => {
 			).to.throw( CKEditorError, /model-position-fromjson-no-root/ );
 		} );
 	} );
+
+	describe( 'getCommonAncestor()', () => {


All tests use the same length of paths.

Reinmar · 2017-07-07T14:10:46Z

tests/view/position.js

@@ -519,4 +519,43 @@ describe( 'Position', () => {
 			document.destroy();
 		} );
 	} );
+
+	describe( 'getCommonAncestor()', () => {


Here you use positions on the same depts as well.

… docs.

Reinmar · 2017-07-10T09:14:56Z

src/model/position.js

+
+		const node = this.root.getNodeByPath( this.getCommonPath( position ) );
+
+		if ( node.is( 'text' ) ) {


This is something I expected from this method's model implementation. But I wonder what other think. Is "text node" a position's ancestor?

OK, @scofalik and @pjasiun confirm that a text node is not an ancestor of position.

Let's add a comment here saying that "Although paths can indicate a text node, text node is not an ancestor of a position."

Reinmar · 2017-07-10T09:43:38Z

@pjasiun and @scofalik pointed out that the tests miss these 2 tests:

<p>{}<a></a></p>
<p>xxx{}xxx</p>

They will prove that using getCommonPath() will not work.

Reinmar · 2017-07-10T09:45:07Z

tests/view/position.js

+			test( fPosition, zPosition, li1 );
+		} );
+
+		it( 'works when one positions is nested deeper than the other', () => {


"when one position"

or rather "one of the positions"

Reinmar · 2017-07-10T09:56:42Z

tests/view/position.js

@@ -519,4 +519,69 @@ describe( 'Position', () => {
 			document.destroy();
 		} );
 	} );
+
+	describe( 'getCommonAncestor()', () => {


There's just a single test with positions in text nodes and it uses a 0 offset. Please add more, with better offsets (ones which have a chance to break something).

…onAncestor()" method.

Introduced Position#getCommonAncestor( otherPosition ) and `Range#g…

3cd1cef

…etCommonAncestor()` methods for `view` and `model`.

pomek requested a review from Reinmar July 7, 2017 13:58

Reinmar reviewed Jul 7, 2017

View reviewed changes

Reinmar added the status:review- label Jul 7, 2017

Reinmar force-pushed the master branch from 9cd2b67 to 34498a8 Compare July 7, 2017 15:03

Kamil Piechaczek added 4 commits July 10, 2017 09:12

Added method "DocumentFragment#getNodeByPath()".

7267e15

Simplified "Position#getCommonAncestor()" (model) and fixed incorrect…

22926a1

… docs.

Improved docs and tests.

a43a195

Improved the tests.

98dda91

pomek removed the status:review- label Jul 10, 2017

Fixed indent in comment. [skip ci]

bc2ae56

Reinmar reviewed Jul 10, 2017

View reviewed changes

Added comment with explanation. [skip ci]

bf294b3

Reinmar reviewed Jul 10, 2017

View reviewed changes

Kamil Piechaczek and others added 3 commits July 10, 2017 13:44

Improvements for the tests and refactored the "model.Position#getComm…

42b5a7b

…onAncestor()" method.

Merge branch 'master' into t/1002

f6e287c

Minor docs and code cleanup.

42f0b8e

Reinmar merged commit 0e29844 into master Jul 11, 2017

Reinmar deleted the t/1002 branch July 11, 2017 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/1002: Introduce Position#getCommonAncestor( otherPosition ) and Range#getCommonAncestor() #1005

T/1002: Introduce Position#getCommonAncestor( otherPosition ) and Range#getCommonAncestor() #1005

pomek commented Jul 7, 2017 •

edited

Loading

Reinmar Jul 7, 2017

Reinmar Jul 7, 2017

pomek Jul 7, 2017

Reinmar Jul 7, 2017

Reinmar Jul 7, 2017

Reinmar Jul 7, 2017

Reinmar Jul 7, 2017

Reinmar Jul 7, 2017

Reinmar Jul 7, 2017

Reinmar Jul 7, 2017

Reinmar Jul 10, 2017

Reinmar Jul 10, 2017

Reinmar Jul 10, 2017

Reinmar commented Jul 10, 2017

Reinmar Jul 10, 2017

Reinmar Jul 10, 2017

Reinmar Jul 10, 2017


		const node = this.root.getNodeByPath( this.getCommonPath( position ) );

		if ( node.is( 'text' ) ) {

T/1002: Introduce Position#getCommonAncestor( otherPosition ) and Range#getCommonAncestor() #1005

T/1002: Introduce Position#getCommonAncestor( otherPosition ) and Range#getCommonAncestor() #1005

Conversation

pomek commented Jul 7, 2017 • edited Loading

Suggested merge commit message (convention)

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

Reinmar commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomek commented Jul 7, 2017 •

edited

Loading