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

Commit

Permalink
Merge pull request #983 from ckeditor/t/982
Browse files Browse the repository at this point in the history 
Other: Changed the `merge` option of `DataController.deleteContent()` to `leaveUnmerged`. The default value stayed `false`, so the default behavior of the function was changed to merge blocks. Closes #982.

BREAKING CHANGE: The `DataController.deleteContent()` option was renamed from `merge` to `leaveUnmerged` and the default behavior of the function was changed to merge blocks.
  • Loading branch information
@szymonkups
szymonkups authored Jun 28, 2017
2 parents d3e7afa + 945d7aa commit 56347d1
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 58 deletions.
13 changes: 8 additions & 5 deletions src/controller/deletecontent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ import Element from '../model/element';
* @param {module:engine/model/selection~Selection} selection Selection of which the content should be deleted.
* @param {module:engine/model/batch~Batch} batch Batch to which the deltas will be added.
* @param {Object} [options]
* @param {Boolean} [options.merge=false] Merge elements after removing the contents of the selection.
* For example, `<h>x[x</h><p>y]y</p>` will become: `<h>x^y</h>` with the option enabled
* and: `<h>x^</h><p>y</p>` without it.
* @param {Boolean} [options.leaveUnmerged=false] Whether to merge elements after removing the content of the selection.
*
* For example `<h>x[x</h><p>y]y</p>` will become:
* * `<h>x^y</h>` with the option disabled (`leaveUnmerged == false`)
* * `<h>x^</h><p>y</p>` with enabled (`leaveUnmerged == true`).
*
* Note: {@link module:engine/model/schema~Schema#objects object} and {@link module:engine/model/schema~Schema#limits limit}
* elements will not be merged.
*/
Expand All @@ -34,7 +37,7 @@ export default function deleteContent( selection, batch, options = {} ) {
const startPos = selRange.start;
const endPos = LivePosition.createFromPosition( selRange.end );

// 1. Remove the contents if there are any.
// 1. Remove the content if there is any.
if ( !selRange.start.isTouching( selRange.end ) ) {
batch.remove( selRange );
}
Expand All @@ -47,7 +50,7 @@ export default function deleteContent( selection, batch, options = {} ) {
// However, the algorithm supports also merging deeper structures (up to the depth of the shallower branch),
// as it's hard to imagine what should actually be the default behavior. Usually, specific features will
// want to override that behavior anyway.
if ( options.merge ) {
if ( !options.leaveUnmerged ) {
mergeBranches( batch, startPos, endPos );
}

Expand Down
4 changes: 1 addition & 3 deletions src/controller/insertcontent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ export default function insertContent( dataController, content, selection, batch
}

if ( !selection.isCollapsed ) {
dataController.deleteContent( selection, batch, {
merge: true
} );
dataController.deleteContent( selection, batch );
}

const insertion = new Insertion( dataController, batch, selection.anchor );
Expand Down
81 changes: 31 additions & 50 deletions tests/controller/deletecontent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,7 @@ describe( 'DataController', () => {
test(
'does not break things when option.merge passed',
'w[x<image></image>y]z',
'w[]z',
{ merge: true }
'w[]z'
);
} );

Expand Down Expand Up @@ -173,35 +172,32 @@ describe( 'DataController', () => {
test(
'do not merge when no need to',
'<paragraph>x</paragraph><paragraph>[foo]</paragraph><paragraph>y</paragraph>',
'<paragraph>x</paragraph><paragraph>[]</paragraph><paragraph>y</paragraph>',
{ merge: true }
'<paragraph>x</paragraph><paragraph>[]</paragraph><paragraph>y</paragraph>'
);

test(
'merges second element into the first one (same name)',
'<paragraph>x</paragraph><paragraph>fo[o</paragraph><paragraph>b]ar</paragraph><paragraph>y</paragraph>',
'<paragraph>x</paragraph><paragraph>fo[]ar</paragraph><paragraph>y</paragraph>',
{ merge: true }
'<paragraph>x</paragraph><paragraph>fo[]ar</paragraph><paragraph>y</paragraph>'
);

test(
'does not merge second element into the first one (same name, !option.merge)',
'<paragraph>x</paragraph><paragraph>fo[o</paragraph><paragraph>b]ar</paragraph><paragraph>y</paragraph>',
'<paragraph>x</paragraph><paragraph>fo[]</paragraph><paragraph>ar</paragraph><paragraph>y</paragraph>'
'<paragraph>x</paragraph><paragraph>fo[]</paragraph><paragraph>ar</paragraph><paragraph>y</paragraph>',
{ leaveUnmerged: true }
);

test(
'merges second element into the first one (same name)',
'<paragraph>x</paragraph><paragraph>fo[o</paragraph><paragraph>b]ar</paragraph><paragraph>y</paragraph>',
'<paragraph>x</paragraph><paragraph>fo[]ar</paragraph><paragraph>y</paragraph>',
{ merge: true }
'<paragraph>x</paragraph><paragraph>fo[]ar</paragraph><paragraph>y</paragraph>'
);

test(
'merges second element into the first one (different name)',
'<paragraph>x</paragraph><heading1>fo[o</heading1><paragraph>b]ar</paragraph><paragraph>y</paragraph>',
'<paragraph>x</paragraph><heading1>fo[]ar</heading1><paragraph>y</paragraph>',
{ merge: true }
'<paragraph>x</paragraph><heading1>fo[]ar</heading1><paragraph>y</paragraph>'
);

// Note: in all these cases we ignore the direction of merge.
Expand All @@ -214,37 +210,33 @@ describe( 'DataController', () => {
{ lastRangeBackward: true }
);

deleteContent( doc.selection, doc.batch(), { merge: true } );
deleteContent( doc.selection, doc.batch() );

expect( getData( doc ) ).to.equal( '<paragraph>x</paragraph><heading1>fo[]ar</heading1><paragraph>y</paragraph>' );
} );

test(
'merges second element into the first one (different attrs)',
'<paragraph>x</paragraph><paragraph align="l">fo[o</paragraph><paragraph>b]ar</paragraph><paragraph>y</paragraph>',
'<paragraph>x</paragraph><paragraph align="l">fo[]ar</paragraph><paragraph>y</paragraph>',
{ merge: true }
'<paragraph>x</paragraph><paragraph align="l">fo[]ar</paragraph><paragraph>y</paragraph>'
);

test(
'merges second element to an empty first element',
'<paragraph>x</paragraph><heading1>[</heading1><paragraph>fo]o</paragraph><paragraph>y</paragraph>',
'<paragraph>x</paragraph><heading1>[]o</heading1><paragraph>y</paragraph>',
{ merge: true }
'<paragraph>x</paragraph><heading1>[]o</heading1><paragraph>y</paragraph>'
);

test(
'merges empty element into the first element',
'<heading1>f[oo</heading1><paragraph>bar]</paragraph><paragraph>x</paragraph>',
'<heading1>f[]</heading1><paragraph>x</paragraph>',
{ merge: true }
'<heading1>f[]</heading1><paragraph>x</paragraph>'
);

test(
'leaves just one element when all selected',
'<heading1>[x</heading1><paragraph>foo</paragraph><paragraph>y]</paragraph>',
'<heading1>[]</heading1>',
{ merge: true }
'<heading1>[]</heading1>'
);

it( 'uses remove delta instead of merge delta if merged element is empty', () => {
Expand All @@ -254,7 +246,7 @@ describe( 'DataController', () => {
const spyMerge = sinon.spy( batch, 'merge' );
const spyRemove = sinon.spy( batch, 'remove' );

deleteContent( doc.selection, batch, { merge: true } );
deleteContent( doc.selection, batch );

expect( getData( doc ) ).to.equal( '<paragraph>ab[]</paragraph>' );

Expand All @@ -269,7 +261,7 @@ describe( 'DataController', () => {
const spyMerge = sinon.spy( batch, 'merge' );
const spyMove = sinon.spy( batch, 'move' );

deleteContent( doc.selection, batch, { merge: true } );
deleteContent( doc.selection, batch );

expect( getData( doc ) ).to.equal( '<paragraph>ab[]gh</paragraph>' );

Expand All @@ -284,8 +276,7 @@ describe( 'DataController', () => {
test(
'merges elements when deep nested',
'<paragraph>x<pchild>fo[o</pchild></paragraph><paragraph><pchild>b]ar</pchild>y</paragraph>',
'<paragraph>x<pchild>fo[]ar</pchild>y</paragraph>',
{ merge: true }
'<paragraph>x<pchild>fo[]ar</pchild>y</paragraph>'
);

it( 'merges elements when deep nested (3rd level)', () => {
Expand Down Expand Up @@ -322,7 +313,7 @@ describe( 'DataController', () => {

doc.selection.setRanges( [ range ] );

deleteContent( doc.selection, doc.batch(), { merge: true } );
deleteContent( doc.selection, doc.batch() );

expect( getData( doc ) )
.to.equal( '<pparent>x<paragraph>x<pchild>fo[]ar</pchild>y</paragraph>y</pparent>' );
Expand All @@ -331,15 +322,13 @@ describe( 'DataController', () => {
test(
'merges elements when left end deep nested',
'<paragraph>x<pchild>fo[o</pchild></paragraph><paragraph>b]ary</paragraph><paragraph>x</paragraph>',
'<paragraph>x<pchild>fo[]ary</pchild></paragraph><paragraph>x</paragraph>',
{ merge: true }
'<paragraph>x<pchild>fo[]ary</pchild></paragraph><paragraph>x</paragraph>'
);

test(
'merges elements when right end deep nested',
'<paragraph>x</paragraph><paragraph>fo[o</paragraph><paragraph><pchild>b]ar</pchild>x</paragraph>',
'<paragraph>x</paragraph><paragraph>fo[]ar</paragraph><paragraph>x</paragraph>',
{ merge: true }
'<paragraph>x</paragraph><paragraph>fo[]ar</paragraph><paragraph>x</paragraph>'
);

it( 'merges elements when left end deep nested (3rd level)', () => {
Expand Down Expand Up @@ -369,7 +358,7 @@ describe( 'DataController', () => {

doc.selection.setRanges( [ range ] );

deleteContent( doc.selection, doc.batch(), { merge: true } );
deleteContent( doc.selection, doc.batch() );

expect( getData( doc ) )
.to.equal( '<pparent>x<paragraph>foo<pchild>ba[]om</pchild></paragraph></pparent>' );
Expand All @@ -378,15 +367,13 @@ describe( 'DataController', () => {
test(
'merges elements when right end deep nested (in an empty container)',
'<paragraph>fo[o</paragraph><paragraph><pchild>bar]</pchild></paragraph>',
'<paragraph>fo[]</paragraph>',
{ merge: true }
'<paragraph>fo[]</paragraph>'
);

test(
'merges elements when left end deep nested (in an empty container)',
'<paragraph><pchild>[foo</pchild></paragraph><paragraph>b]ar</paragraph><paragraph>x</paragraph>',
'<paragraph><pchild>[]ar</pchild></paragraph><paragraph>x</paragraph>',
{ merge: true }
'<paragraph><pchild>[]ar</pchild></paragraph><paragraph>x</paragraph>'
);

it( 'merges elements when left end deep nested (3rd level)', () => {
Expand Down Expand Up @@ -414,7 +401,7 @@ describe( 'DataController', () => {

doc.selection.setRanges( [ range ] );

deleteContent( doc.selection, doc.batch(), { merge: true } );
deleteContent( doc.selection, doc.batch() );

expect( getData( doc ) )
.to.equal( '<paragraph>fo[]</paragraph>' );
Expand All @@ -440,15 +427,13 @@ describe( 'DataController', () => {
test(
'does not merge an object element (if it is first)',
'<blockWidget><nestedEditable>fo[o</nestedEditable></blockWidget><paragraph>b]ar</paragraph>',
'<blockWidget><nestedEditable>fo[]</nestedEditable></blockWidget><paragraph>ar</paragraph>',
{ merge: true }
'<blockWidget><nestedEditable>fo[]</nestedEditable></blockWidget><paragraph>ar</paragraph>'
);

test(
'does not merge an object element (if it is second)',
'<paragraph>ba[r</paragraph><blockWidget><nestedEditable>f]oo</nestedEditable></blockWidget>',
'<paragraph>ba[]</paragraph><blockWidget><nestedEditable>oo</nestedEditable></blockWidget>',
{ merge: true }
'<paragraph>ba[]</paragraph><blockWidget><nestedEditable>oo</nestedEditable></blockWidget>'
);
} );
} );
Expand Down Expand Up @@ -611,22 +596,19 @@ describe( 'DataController', () => {
test(
'merge option should be ignored if both elements are limits',
'<inlineLimit>foo [bar</inlineLimit><inlineLimit>baz] qux</inlineLimit>',
'<inlineLimit>foo []</inlineLimit><inlineLimit> qux</inlineLimit>',
{ merge: true }
'<inlineLimit>foo []</inlineLimit><inlineLimit> qux</inlineLimit>'
);

test(
'merge option should be ignored if the first element is a limit',
'<inlineLimit>foo [bar</inlineLimit><x>baz] qux</x>',
'<inlineLimit>foo []</inlineLimit><x> qux</x>',
{ merge: true }
'<inlineLimit>foo []</inlineLimit><x> qux</x>'
);

test(
'merge option should be ignored if the second element is a limit',
'<x>baz [qux</x><inlineLimit>foo] bar</inlineLimit>',
'<x>baz []</x><inlineLimit> bar</inlineLimit>',
{ merge: true }
'<x>baz []</x><inlineLimit> bar</inlineLimit>'
);
} );

Expand All @@ -648,14 +630,14 @@ describe( 'DataController', () => {
test(
'should delete inside block limit element',
'<blockLimit><paragraph>fo[o</paragraph><paragraph>b]ar</paragraph></blockLimit>',
'<blockLimit><paragraph>fo[]</paragraph><paragraph>ar</paragraph></blockLimit>'
'<blockLimit><paragraph>fo[]</paragraph><paragraph>ar</paragraph></blockLimit>',
{ leaveUnmerged: true }
);

test(
'should delete inside block limit element (with merge)',
'<blockLimit><paragraph>fo[o</paragraph><paragraph>b]ar</paragraph></blockLimit>',
'<blockLimit><paragraph>fo[]ar</paragraph></blockLimit>',
{ merge: true }
'<blockLimit><paragraph>fo[]ar</paragraph></blockLimit>'
);

test(
Expand All @@ -673,8 +655,7 @@ describe( 'DataController', () => {
test(
'merge option should be ignored if any of the elements is a limit',
'<blockLimit><paragraph>foo [bar</paragraph></blockLimit><blockLimit><paragraph>baz] qux</paragraph></blockLimit>',
'<blockLimit><paragraph>foo []</paragraph></blockLimit><blockLimit><paragraph> qux</paragraph></blockLimit>',
{ merge: true }
'<blockLimit><paragraph>foo []</paragraph></blockLimit><blockLimit><paragraph> qux</paragraph></blockLimit>'
);
} );

Expand Down

0 comments on commit 56347d1

Please sign in to comment.