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

Commit

Permalink
Changed: Properly handle marker update with changes to be manged by o…
Browse files Browse the repository at this point in the history 
…perations.
  • Loading branch information
@jodator
jodator committed Apr 5, 2018
1 parent 76f0608 commit 49b99b4
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 46 deletions.
46 changes: 29 additions & 17 deletions src/model/writer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -884,7 +884,6 @@ export default class Writer {
* @param {module:engine/model/range~Range} [options.range] Marker range to update.
* @param {Boolean} [options.usingOperation] Flag indicated whether the marker should be added by MarkerOperation.
* See {@link module:engine/model/markercollection~Marker#managedUsingOperations}.
* @returns {module:engine/model/markercollection~Marker} Marker that was set.
*/
updateMarker( markerOrName, options ) {
this._assertWriterUsedCorrectly();
Expand All @@ -902,30 +901,43 @@ export default class Writer {
throw new CKEditorError( 'writer-updateMarker-marker-not-exists: Marker with provided name does not exists.' );
}

const range = options && options.range;

const newRange = options && options.range;
const hasUsingOperationDefined = !!options && typeof options.usingOperation == 'boolean';
const usingOperation = !!options && !!options.usingOperation; // : currentMarker.managedUsingOperations;

if ( !usingOperation ) {
// If marker changes to a marker that do not use operations then we need to create additional operation
// that removes that marker first.
if ( hasUsingOperationDefined && currentMarker.managedUsingOperations ) {
applyMarkerOperation( this, markerName, currentMarker.getRange(), null );
}
if ( !hasUsingOperationDefined && !newRange ) {
/**
* One of options is required - provide range or usingOperations.
*
* @error writer-updateMarker-wrong-options
*/
throw new CKEditorError( 'writer-updateMarker-wrong-options: One of options is required - provide range or usingOperations.' );
}

if ( hasUsingOperationDefined && options.usingOperation !== currentMarker.managedUsingOperations ) {
// The marker type is changed so it's necessary to create proper operations.
if ( options.usingOperation ) {
// If marker changes to a managed one treat this as synchronizing existing marker.
// If marker changes to a managed one treat this as synchronizing existing marker.
// Create `MarkerOperation` with `oldRange` set to `null`, so reverse operation will remove the marker.
applyMarkerOperation( this, markerName, null, newRange ? newRange : currentMarker.getRange() );
} else {
// If marker changes to a marker that do not use operations then we need to create additional operation
// that removes that marker first.
const currentRange = currentMarker.getRange();
applyMarkerOperation( this, markerName, currentRange, null );

this.model.markers._set( markerName, range, hasUsingOperationDefined ? usingOperation : currentMarker.managedUsingOperations );
// Although not managed the marker itself should stay in model and its range should be preserver or changed to passed range.
this.model.markers._set( markerName, newRange ? newRange : currentRange );
}

return;
}

if ( !range ) {
// If `range` is not given, treat this as synchronizing existing marker.
// Create `MarkerOperation` with `oldRange` set to `null`, so reverse operation will remove the marker.
applyMarkerOperation( this, markerName, null, currentMarker.getRange() );
// Marker's type doesn't change so update it accordingly.
if ( currentMarker.managedUsingOperations ) {
applyMarkerOperation( this, markerName, currentMarker.getRange(), newRange );
} else {
// Just change marker range.
applyMarkerOperation( this, markerName, currentMarker.getRange(), range );
this.model.markers._set( markerName, newRange );
}
}

Expand Down
110 changes: 81 additions & 29 deletions tests/model/writer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2049,11 +2049,11 @@ describe( 'Writer', () => {
range = Range.createIn( root );
} );

it( 'should accept marker instance', () => {
it( 'should update managed marker\'s range by marker instance using operations', () => {
const marker = addMarker( 'name', { range, usingOperation: true } );
const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );

updateMarker( marker, { range: range2, usingOperation: true } );
updateMarker( marker, { range: range2 } );

expect( batch.deltas.length ).to.equal( 2 );

Expand All @@ -2064,40 +2064,69 @@ describe( 'Writer', () => {
expect( op.newRange.isEqual( range2 ) ).to.be.true;
} );

it( 'should not use operations when updating marker which does not use operations', () => {
const spy = sinon.spy();
model.on( 'applyOperation', spy );
it( 'should update managed marker\'s range by marker name using operations', () => {
const marker = addMarker( 'name', { range, usingOperation: true } );
const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );

const marker = addMarker( 'name', { range, usingOperation: false } );
updateMarker( 'name', { range: range2 } );

expect( batch.deltas.length ).to.equal( 2 );

const op = batch.deltas[ 1 ].operations[ 0 ];

expect( marker.getRange().isEqual( range2 ) ).to.be.true;
expect( op.oldRange.isEqual( range ) ).to.be.true;
expect( op.newRange.isEqual( range2 ) ).to.be.true;
} );

it( 'should update managed marker\'s range by marker instance using operations and usingOperation explicitly passed', () => {
const marker = addMarker( 'name', { range, usingOperation: true } );
const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );

updateMarker( marker, { range: range2, usingOperation: false } );
updateMarker( marker, { range: range2, usingOperation: true } );

sinon.assert.notCalled( spy );
expect( batch.deltas.length ).to.equal( 2 );

const op = batch.deltas[ 1 ].operations[ 0 ];

expect( marker.getRange().isEqual( range2 ) ).to.be.true;
expect( op.oldRange.isEqual( range ) ).to.be.true;
expect( op.newRange.isEqual( range2 ) ).to.be.true;
} );

it( 'should accept empty range parameter if marker instance is passed and usingOperation is set to true', () => {
it( 'should update managed marker\'s range by marker name using operations and usingOperation explicitly passed', () => {
const marker = addMarker( 'name', { range, usingOperation: true } );
const spy = sinon.spy();
const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );

updateMarker( 'name', { range: range2, usingOperation: true } );

expect( batch.deltas.length ).to.equal( 2 );

const op = batch.deltas[ 1 ].operations[ 0 ];

expect( marker.getRange().isEqual( range2 ) ).to.be.true;
expect( op.oldRange.isEqual( range ) ).to.be.true;
expect( op.newRange.isEqual( range2 ) ).to.be.true;
} );

it( 'should not use operations when updating marker which does not use operations', () => {
const spy = sinon.spy();
model.on( 'applyOperation', spy );

updateMarker( marker, { usingOperation: true } );
const marker = addMarker( 'name', { range, usingOperation: false } );
const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );

const op = batch.deltas[ 0 ].operations[ 0 ];
updateMarker( marker, { range: range2 } );

sinon.assert.calledOnce( spy );
expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' );
expect( op.oldRange ).to.be.null;
expect( op.newRange.isEqual( range ) ).to.be.true;
sinon.assert.notCalled( spy );
} );

it( 'should create additional operation when marker type changes to not managed using operation', () => {
const spy = sinon.spy();
model.on( 'applyOperation', spy );

addMarker( 'name', { range, usingOperation: true } );
updateMarker( 'name', { range, usingOperation: false } );
updateMarker( 'name', { usingOperation: false } );

const marker = model.markers.get( 'name' );

Expand All @@ -2117,28 +2146,40 @@ describe( 'Writer', () => {
expect( marker.managedUsingOperations ).to.be.false;
} );

it( 'should not create additional operation when using operation is not specified', () => {
it( 'should create additional operation when marker type changes to not managed using operation and changing its range', () => {
const spy = sinon.spy();
model.on( 'applyOperation', spy );
const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );

addMarker( 'name', { range, usingOperation: true } );
updateMarker( 'name', { range: range2, usingOperation: false } );

sinon.assert.calledOnce( spy );
const marker = model.markers.get( 'name' );

updateMarker( 'name', { range } );
const op1 = batch.deltas[ 0 ].operations[ 0 ];
const op2 = batch.deltas[ 1 ].operations[ 0 ];

const marker = model.markers.get( 'name' );
sinon.assert.calledTwice( spy );
expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' );
expect( spy.secondCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' );

sinon.assert.calledOnce( spy );
expect( marker.managedUsingOperations ).to.be.true;
expect( op1.oldRange ).to.be.null;
expect( op1.newRange.isEqual( range ) ).to.be.true;

expect( op2.oldRange.isEqual( range ) ).to.be.true;
expect( op2.newRange ).to.be.null;

expect( marker.getRange().isEqual( range2 ) );

expect( marker.managedUsingOperations ).to.be.false;
} );

it( 'should enable changing marker to be managed using operation', () => {
const spy = sinon.spy();
model.on( 'applyOperation', spy );

addMarker( 'name', { range, usingOperation: false } );
updateMarker( 'name', { range, usingOperation: true } );
updateMarker( 'name', { usingOperation: true } );

const marker = model.markers.get( 'name' );

Expand All @@ -2148,17 +2189,28 @@ describe( 'Writer', () => {
expect( marker.managedUsingOperations ).to.be.true;
} );

it( 'should enable changing marker to be not managed using operation', () => {
it( 'should enable changing marker to be managed using operation while changing range', () => {
const spy = sinon.spy();
model.on( 'applyOperation', spy );
const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );

addMarker( 'name', { range, usingOperation: true } );
updateMarker( 'name', { range, usingOperation: false } );
addMarker( 'name', { range, usingOperation: false } );
updateMarker( 'name', { range: range2, usingOperation: true } );

const marker = model.markers.get( 'name' );

sinon.assert.calledTwice( spy );
expect( marker.managedUsingOperations ).to.be.false;
sinon.assert.calledOnce( spy );
expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' );
expect( marker.getRange().isEqual( range2 ) ).to.be.true;

expect( marker.managedUsingOperations ).to.be.true;
} );

it( 'should throw when range and usingOperations were not provided', () => {
expect( () => {
addMarker( 'name', { range, usingOperation: false } );
updateMarker( 'name' );
} ).to.throw( CKEditorError, /^writer-updateMarker-wrong-options/ );
} );

it( 'should throw when marker provided by name does not exists', () => {
Expand Down

0 comments on commit 49b99b4

Please sign in to comment.