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

Second parameter of LiveRange#event:change is now deletionPosition not operation that changed the range #1487

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/model/documentselection.js
Original file line number Diff line number Diff line change
Expand Up @@ -750,14 +750,14 @@ class LiveSelection extends Selection {

const liveRange = LiveRange.createFromRange( range );

liveRange.on( 'change:range', ( evt, oldRange, operation ) => {
liveRange.on( 'change:range', ( evt, oldRange, deletionPosition ) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be an object in case we need to add more properties in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

this._hasChangedRange = true;

// If `LiveRange` is in whole moved to the graveyard, save necessary data. It will be fixed on `Model#applyOperation` event.
if ( liveRange.root == this._document.graveyard ) {
this._fixGraveyardRangesData.push( {
liveRange,
sourcePosition: operation.sourcePosition
sourcePosition: deletionPosition
} );
}
} );
Expand Down
19 changes: 14 additions & 5 deletions src/model/liverange.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,7 @@ export default class LiveRange extends Range {
* @param {Object} data Object with additional information about the change. Those parameters are passed from
* {@link module:engine/model/document~Document#event:change document change event}.
* @param {String} data.type Change type.
* @param {module:engine/model/batch~Batch} data.batch Batch which changed the live range.
* @param {module:engine/model/range~Range} data.range Range containing the result of applied change.
* @param {module:engine/model/position~Position} data.sourcePosition Source position for move, remove and reinsert change types.
* @param {module:engine/model/position~Position|null} deletionPosition Source position for move, remove and reinsert change types.
*/

/**
Expand Down Expand Up @@ -147,16 +145,27 @@ function transform( operation ) {
const contentChanged = doesOperationChangeRangeContent( this, operation );

if ( boundariesChanged ) {
let deletionPosition = null;

if ( result.root.rootName == '$graveyard' ) {
if ( operation.type == 'remove' ) {
deletionPosition = operation.sourcePosition;
} else {
// Merge operation.
deletionPosition = operation.deletionPosition;
}
}

// If range boundaries have changed, fire `change:range` event.
const oldRange = Range.createFromRange( this );

this.start = result.start;
this.end = result.end;

this.fire( 'change:range', oldRange, operation );
this.fire( 'change:range', oldRange, deletionPosition );
} else if ( contentChanged ) {
// If range boundaries have not changed, but there was change inside the range, fire `change:content` event.
this.fire( 'change:content', Range.createFromRange( this ), operation );
this.fire( 'change:content', Range.createFromRange( this ), null );
}
}

Expand Down
57 changes: 40 additions & 17 deletions tests/model/liverange.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* For licensing, see LICENSE.md.
*/

import Batch from '../../src/model/batch';
import Model from '../../src/model/model';
import Element from '../../src/model/element';
import Position from '../../src/model/position';
Expand Down Expand Up @@ -90,7 +89,7 @@ describe( 'LiveRange', () => {
range.detach();
} );

it( 'should fire change:range event with proper data when its boundaries are changed', () => {
it( 'should fire change:range event with when its boundaries are changed', () => {
const live = new LiveRange( new Position( root, [ 0, 1, 4 ] ), new Position( root, [ 0, 2, 2 ] ) );
const copy = Range.createFromRange( live );

Expand All @@ -99,52 +98,76 @@ describe( 'LiveRange', () => {

const sourcePosition = new Position( root, [ 2 ] );
const targetPosition = new Position( root, [ 0 ] );
const batch = new Batch();
let op = null;

model.enqueueChange( batch, writer => {
model.change( writer => {
const sourceRange = Range.createFromPositionAndShift( sourcePosition, 1 );

writer.move( sourceRange, targetPosition );

op = batch.operations[ 0 ];
} );

expect( spy.calledOnce ).to.be.true;

// First parameter available in event should be a range that is equal to the live range before the live range changed.
expect( spy.args[ 0 ][ 1 ].isEqual( copy ) ).to.be.true;

// Second parameter is the operation that changed the range.
expect( spy.args[ 0 ][ 2 ] ).to.equal( op );
// Second parameter is null for operations that did not move the range into graveyard.
expect( spy.args[ 0 ][ 2 ] ).to.be.null;
} );

it( 'should fire change:content event with proper data when content inside the range has changed', () => {
it( 'should fire change:content event when content inside the range has changed', () => {
const live = new LiveRange( new Position( root, [ 0, 1 ] ), new Position( root, [ 0, 3 ] ) );

const spy = sinon.spy();
live.on( 'change:content', spy );

const sourcePosition = new Position( root, [ 0, 2, 0 ] );
const targetPosition = new Position( root, [ 0, 4, 0 ] );
const batch = new Batch();
let op = null;

model.enqueueChange( batch, writer => {
model.change( writer => {
const sourceRange = Range.createFromPositionAndShift( sourcePosition, 2 );

writer.move( sourceRange, targetPosition );

op = batch.operations[ 0 ];
} );

expect( spy.calledOnce ).to.be.true;

// First parameter available in event should be a range that is equal to the live range before the live range changed.
expect( spy.args[ 0 ][ 1 ].isEqual( live ) ).to.be.true;

// Second parameter is the operation that changed the range.
expect( spy.args[ 0 ][ 2 ] ).to.equal( op );
// Second parameter is null for operations that did not move the range into graveyard.
expect( spy.args[ 0 ][ 2 ] ).to.be.null;
} );

it( 'should pass deletion position if range was removed (remove)', () => {
const live = new LiveRange( new Position( root, [ 0, 2 ] ), new Position( root, [ 0, 4 ] ) );

const spy = sinon.spy();
live.on( 'change:range', spy );

const sourcePosition = new Position( root, [ 0, 0 ] );

model.change( writer => {
writer.remove( Range.createFromPositionAndShift( sourcePosition, 6 ) );
} );

// Second parameter is deletion position.
expect( spy.args[ 0 ][ 2 ].isEqual( sourcePosition ) ).to.be.true;
} );

it( 'should pass deletion position if range was removed (merge)', () => {
const live = new LiveRange( new Position( root, [ 1 ] ), new Position( root, [ 2 ] ) );

const spy = sinon.spy();
live.on( 'change:range', spy );

const mergePosition = new Position( root, [ 1 ] );

model.change( writer => {
writer.merge( mergePosition );
} );

// Second parameter is deletion position.
expect( spy.args[ 0 ][ 2 ].isEqual( mergePosition ) ).to.be.true;
} );

describe( 'should get transformed and fire change:range if', () => {
Expand Down