From 281e8e50d700fd9c31d5c1547a16f55f79f850d8 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 8 Sep 2017 11:24:24 +0200 Subject: [PATCH 1/2] Fixed: Improved range by merge delta transformation case. --- src/model/range.js | 5 ++++- tests/model/range.js | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/model/range.js b/src/model/range.js index 57eb98d39..66f7d67bd 100644 --- a/src/model/range.js +++ b/src/model/range.js @@ -493,7 +493,10 @@ export default class Range { // Without fix, the range would end up in the graveyard, together with removed element. //

foo

[]bar

->

foobar

[]

->

foobar

->

foo[]bar

//

foo

bar[]

- return [ new Range( targetPosition.getShiftedBy( this.start.offset ) ) ]; + // return [ new Range( targetPosition.getShiftedBy( this.start.offset ) ) ]; + const offset = this.start.offset - sourceRange.start.offset; + + return [ new Range( targetPosition.getShiftedBy( offset ) ) ]; } // // Other edge cases: diff --git a/tests/model/range.js b/tests/model/range.js index 59ae001c3..1ba8d83b3 100644 --- a/tests/model/range.js +++ b/tests/model/range.js @@ -9,6 +9,9 @@ import Element from '../../src/model/element'; import Text from '../../src/model/text'; import Document from '../../src/model/document'; import TreeWalker from '../../src/model/treewalker'; +import MergeDelta from '../../src/model/delta/mergedelta'; +import MoveOperation from '../../src/model/operation/moveoperation'; +import RemoveOperation from '../../src/model/operation/removeoperation'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import { jsonParseStringify } from '../../tests/model/_utils/utils'; @@ -1056,6 +1059,22 @@ describe( 'Range', () => { expect( transformed[ 0 ].start.path ).to.deep.equal( [ 0, 0, 1 ] ); expect( transformed[ 0 ].end.path ).to.deep.equal( [ 0, 1, 1 ] ); } ); + + // #1132 + it( 'merge delta has move operation that does not start from offset 0', () => { + const range = new Range( new Position( root, [ 1, 10 ] ), new Position( root, [ 1, 20 ] ) ); + + // Such unusual delta may be a result of transformation. + const delta = new MergeDelta(); + delta.addOperation( new MoveOperation( new Position( root, [ 1, 10 ] ), 10, new Position( root, [ 0, 10 ] ), 0 ) ); + delta.addOperation( new RemoveOperation( new Position( root, [ 1 ] ), 1, new Position( doc.graveyard, [ 0 ] ), 1 ) ); + + const transformed = range.getTransformedByDelta( delta ); + + expect( transformed.length ).to.equal( 1 ); + expect( transformed[ 0 ].start.path ).to.deep.equal( [ 0, 10 ] ); + expect( transformed[ 0 ].end.path ).to.deep.equal( [ 0, 20 ] ); + } ); } ); describe( 'by WrapDelta', () => { From df51d0dc37d613cb5dbd2a2746a03b972195d58d Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 13 Sep 2017 09:43:33 +0200 Subject: [PATCH 2/2] Docs: Explained confusing code in inline comments. --- src/model/range.js | 9 +++++++-- tests/model/range.js | 25 +++++++++++++++++++------ 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/model/range.js b/src/model/range.js index 66f7d67bd..57763a269 100644 --- a/src/model/range.js +++ b/src/model/range.js @@ -492,8 +492,13 @@ export default class Range { // Collapsed range is in merged element, at the beginning or at the end of it. // Without fix, the range would end up in the graveyard, together with removed element. //

foo

[]bar

->

foobar

[]

->

foobar

->

foo[]bar

- //

foo

bar[]

- // return [ new Range( targetPosition.getShiftedBy( this.start.offset ) ) ]; + //

foo

bar[]

->

foobar

[]

->

foobar

->

foobar[]

+ // + // In most cases, `sourceRange.start.offset` for merge delta's move operation would be 0, + // so this formula might look overcomplicated. + // However in some scenarios, after operational transformation, move operation might not + // in fact start from 0 and we need to properly count new offset. + // https://github.com/ckeditor/ckeditor5-engine/pull/1133#issuecomment-329080668. const offset = this.start.offset - sourceRange.start.offset; return [ new Range( targetPosition.getShiftedBy( offset ) ) ]; diff --git a/tests/model/range.js b/tests/model/range.js index 1ba8d83b3..cc9f36b5e 100644 --- a/tests/model/range.js +++ b/tests/model/range.js @@ -1060,20 +1060,33 @@ describe( 'Range', () => { expect( transformed[ 0 ].end.path ).to.deep.equal( [ 0, 1, 1 ] ); } ); - // #1132 + // #1132. it( 'merge delta has move operation that does not start from offset 0', () => { - const range = new Range( new Position( root, [ 1, 10 ] ), new Position( root, [ 1, 20 ] ) ); + // This scenario is a test for a rare situation, that after some OT, a move operation in + // merge delta does not start from 0 offset. + // + // It happens that move operation in merge delta becomes "do nothing move operation", something like: + // + // move range [ a, x ] - [ a, y ] to [ a, x ] + // for example: move [ 0, 3 ] - [ 0, 6 ] -> [ 0, 3 ] + // + // This is a result of valid transformation and we need to check if range is properly transformed + // when such unusual delta is generated. + // For more see: https://github.com/ckeditor/ckeditor5-engine/pull/1133#issuecomment-329080668. + // + // For this test scenario assume:

foobar[]

, "bar" is moved between "o" and "b". + // Expect state after transformation is that nothing has changed. + const range = new Range( new Position( root, [ 0, 6 ] ), new Position( root, [ 0, 6 ] ) ); - // Such unusual delta may be a result of transformation. const delta = new MergeDelta(); - delta.addOperation( new MoveOperation( new Position( root, [ 1, 10 ] ), 10, new Position( root, [ 0, 10 ] ), 0 ) ); + delta.addOperation( new MoveOperation( new Position( root, [ 0, 3 ] ), 3, new Position( root, [ 0, 3 ] ), 0 ) ); delta.addOperation( new RemoveOperation( new Position( root, [ 1 ] ), 1, new Position( doc.graveyard, [ 0 ] ), 1 ) ); const transformed = range.getTransformedByDelta( delta ); expect( transformed.length ).to.equal( 1 ); - expect( transformed[ 0 ].start.path ).to.deep.equal( [ 0, 10 ] ); - expect( transformed[ 0 ].end.path ).to.deep.equal( [ 0, 20 ] ); + expect( transformed[ 0 ].start.path ).to.deep.equal( [ 0, 6 ] ); + expect( transformed[ 0 ].end.path ).to.deep.equal( [ 0, 6 ] ); } ); } );