Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Fix: Split x merge transformation different content bug.
Browse files Browse the repository at this point in the history
  • Loading branch information
scofalik committed Sep 14, 2018
1 parent d248712 commit f02e994
Show file tree
Hide file tree
Showing 4 changed files with 164 additions and 2 deletions.
69 changes: 68 additions & 1 deletion src/model/operation/transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -2031,7 +2031,74 @@ setTransformation( SplitOperation, InsertOperation, ( a, b ) => {
return [ a ];
} );

setTransformation( SplitOperation, MergeOperation, ( a, b ) => {
setTransformation( SplitOperation, MergeOperation, ( a, b, context ) => {
// Case 1:
//
// Split element got merged. If two different elements were merged, clients will have different content.
//
// Example. Merge at `{}`, split at `[]`:
// <heading>Foo</heading>{}<paragraph>B[]ar</paragraph>
//
// On merge side it will look like this:
// <heading>FooB[]ar</heading>
// <heading>FooB</heading><heading>ar</heading>
//
// On split side it will look like this:
// <heading>Foo</heading>{}<paragraph>B</paragraph><paragraph>ar</paragraph>
// <heading>FooB</heading><paragraph>ar</paragraph>
//
// Clearly, the second element is different for both clients.
//
// We could use the removed merge element from graveyard as a split element but then clients would have a different
// model state (in graveyard), because the split side client would still have an element in graveyard (removed by merge).
//
// To overcome this, in `SplitOperation` x `MergeOperation` transformation we will add additional `SplitOperation`
// in the graveyard, which will actually clone the merged-and-deleted element. Then, that cloned element will be
// used for splitting. Example below.
//
// Original state:
// <heading>Foo</heading>{}<paragraph>B[]ar</paragraph>
//
// Merge side client:
//
// After merge:
// <heading>FooB[]ar</heading> graveyard: <paragraph></paragraph>
//
// Extra split:
// <heading>FooB[]ar</heading> graveyard: <paragraph></paragraph><paragraph></paragraph>
//
// Use the "cloned" element from graveyard:
// <heading>FooB</heading><paragraph>ar</paragraph> graveyard: <paragraph></paragraph>
//
// Split side client:
//
// After split:
// <heading>Foo</heading>{}<paragraph>B</paragraph><paragraph>ar</paragraph>
//
// After merge:
// <heading>FooB</heading><paragraph>ar</paragraph> graveyard: <paragraph></paragraph>
//
// This special case scenario only applies if the original split operation clones the split element.
// If the original split operation has `graveyardPosition` set, it all doesn't have sense because split operation
// knows exactly which element it should use. So there would be no original problem with different contents.
//
// Additionally, the special case applies only if the merge wasn't already undone.
//
if ( !a.graveyardPosition && !context.bWasUndone && a.position.hasSameParentAs( b.sourcePosition ) ) {
const splitPath = b.graveyardPosition.path.slice();
splitPath.push( 0 );

const additionalSplit = new SplitOperation( new Position( b.graveyardPosition.root, splitPath ), 0, null, 0 );

a.position = a.position._getTransformedByMergeOperation( b );
a.graveyardPosition = Position.createFromPosition( additionalSplit.insertionPosition );
a.graveyardPosition.stickiness = 'toNext';

return [ additionalSplit, a ];
}

// The default case.
//
if ( a.position.hasSameParentAs( b.deletionPosition ) && !a.position.isAfter( b.deletionPosition ) ) {
a.howMany--;
}
Expand Down
44 changes: 44 additions & 0 deletions tests/model/operation/transform/merge.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,50 @@ describe( 'transform', () => {
} );
} );

describe( 'by remove', () => {
it( 'remove merged element', () => {
john.setData( '<paragraph>Foo</paragraph>[]<paragraph>Bar</paragraph>' );
kate.setData( '<paragraph>Foo</paragraph>[<paragraph>Bar</paragraph>]' );

john.merge();
kate.remove();

syncClients();

expectClients( '<paragraph>Foo</paragraph>' );
} );

it( 'remove merged element then undo #1', () => {
john.setData( '<paragraph>Foo</paragraph>[]<paragraph>Bar</paragraph>' );
kate.setData( '<paragraph>Foo</paragraph>[<paragraph>Bar</paragraph>]' );

john.merge();
kate.remove();

syncClients();
expectClients( '<paragraph>Foo</paragraph>' );

kate.undo();

syncClients();

expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
} );

it( 'remove merged element then undo #2', () => {
john.setData( '<paragraph>Foo</paragraph>[]<paragraph>Bar</paragraph>' );
kate.setData( '<paragraph>Foo</paragraph>[<paragraph>Bar</paragraph>]' );

john.merge();
kate.remove();
kate.undo();

syncClients();

expectClients( '<paragraph>FooBar</paragraph>' );
} );
} );

describe( 'by delete', () => {
it( 'text from two elements', () => {
john.setData( '<paragraph>Foo</paragraph>[]<paragraph>Bar</paragraph>' );
Expand Down
50 changes: 50 additions & 0 deletions tests/model/operation/transform/split.js
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,56 @@ describe( 'transform', () => {
syncClients();
expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
} );

it( 'element into heading', () => {
john.setData( '<heading1>Foo</heading1><paragraph>B[]ar</paragraph>' );
kate.setData( '<heading1>Foo</heading1>[]<paragraph>Bar</paragraph>' );

john.split();
kate.merge();

syncClients();
expectClients(
'<heading1>FooB</heading1>' +
'<paragraph>ar</paragraph>'
);
} );

it( 'element into heading with undo #1', () => {
john.setData( '<heading1>Foo</heading1><paragraph>B[]ar</paragraph>' );
kate.setData( '<heading1>Foo</heading1>[]<paragraph>Bar</paragraph>' );

john.split();
kate.merge();

syncClients();
expectClients(
'<heading1>FooB</heading1>' +
'<paragraph>ar</paragraph>'
);

john.undo();
kate.undo();

syncClients();
expectClients( '<heading1>Foo</heading1><paragraph>Bar</paragraph>' );
} );

it( 'element into heading with undo #2', () => {
john.setData( '<heading1>Foo</heading1><paragraph>B[]ar</paragraph>' );
kate.setData( '<heading1>Foo</heading1>[]<paragraph>Bar</paragraph>' );

john.split();
kate.merge();
kate.undo();

syncClients();
expectClients(
'<heading1>Foo</heading1>' +
'<paragraph>B</paragraph>' +
'<paragraph>ar</paragraph>'
);
} );
} );

describe( 'by delete', () => {
Expand Down
3 changes: 2 additions & 1 deletion tests/model/operation/transform/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
import Typing from '@ckeditor/ckeditor5-typing/src/typing';
import UndoEditing from '@ckeditor/ckeditor5-undo/src/undoediting';
import BlockQuoteEditing from '@ckeditor/ckeditor5-block-quote/src/blockquoteediting';
import HeadingEditing from '@ckeditor/ckeditor5-heading/src/headingediting';

import { getData, parse } from '../../../../src/dev-utils/model';
import { transformSets } from '../../../../src/model/operation/transform';
Expand All @@ -29,7 +30,7 @@ export class Client {
// Typing is needed for delete command.
// UndoEditing is needed for undo command.
// Block plugins are needed for proper data serializing.
plugins: [ Typing, Paragraph, ListEditing, UndoEditing, BlockQuoteEditing ]
plugins: [ Typing, Paragraph, ListEditing, UndoEditing, BlockQuoteEditing, HeadingEditing ]
} ).then( editor => {
this.editor = editor;
this.document = editor.model.document;
Expand Down

0 comments on commit f02e994

Please sign in to comment.