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 #1090 from ckeditor/t/1089
Browse files Browse the repository at this point in the history 
Feature: `model.LiveRange#event:change` got renamed to `change:range`. Introduced `model.LiveRange#event:change:content`. Closes #1089.
  • Loading branch information
Piotr Jasiun authored Aug 22, 2017
2 parents 1955869 + 6a4b11f commit ec22a29
Show file tree
Hide file tree
Showing 5 changed files with 178 additions and 65 deletions.
2 changes: 1 addition & 1 deletion src/model/documentselection.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ export default class DocumentSelection extends Selection {

const liveRange = LiveRange.createFromRange( range );

this.listenTo( liveRange, 'change', ( evt, oldRange, data ) => {
this.listenTo( liveRange, 'change:range', ( evt, oldRange, data ) => {
// If `LiveRange` is in whole moved to the graveyard, fix that range.
if ( liveRange.root == this._document.graveyard ) {
const sourceStart = data.sourcePosition;
Expand Down
40 changes: 35 additions & 5 deletions src/model/liverange.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,10 @@ export default class LiveRange extends Range {
*/

/**
* Fired when `LiveRange` instance is changed due to changes in the {@link module:engine/model/document~Document document}.
* Fired when `LiveRange` instance boundaries have changed due to changes in the
* {@link module:engine/model/document~Document document}.
*
* @event change
* @event change:range
* @param {module:engine/model/range~Range} oldRange Range with start and end position equal to start and end position of this live
* range before it got changed.
* @param {Object} data Object with additional information about the change. Those parameters are passed from
Expand All @@ -89,6 +90,21 @@ export default class LiveRange extends 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.
*/

/**
* Fired when `LiveRange` instance boundaries have not changed after a change in {@link module:engine/model/document~Document document}
* but the change took place inside the range, effectively changing its content.
*
* @event change:content
* @param {module:engine/model/range~Range} range Range with start and end position equal to start and end position of
* change 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.
*/
}

/**
Expand Down Expand Up @@ -152,14 +168,28 @@ function transform( changeType, deltaType, batch, targetRange, sourcePosition )

const updated = Range.createFromRanges( result );

// If anything changed, update the range and fire an event.
if ( !updated.isEqual( this ) ) {
const boundariesChanged = !updated.isEqual( this );

const rangeExpanded = this.containsPosition( targetPosition );
const rangeShrunk = sourcePosition && ( this.containsPosition( sourcePosition ) || this.start.isEqual( sourcePosition ) );
const contentChanged = rangeExpanded || rangeShrunk;

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

this.start = updated.start;
this.end = updated.end;

this.fire( 'change', oldRange, {
this.fire( 'change:range', oldRange, {
type: changeType,
batch,
range: targetRange,
sourcePosition
} );
} 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 ), {
type: changeType,
batch,
range: targetRange,
Expand Down
4 changes: 3 additions & 1 deletion src/model/markercollection.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,9 @@ class Marker {
*/
this._liveRange = liveRange;

this._liveRange.delegate( 'change' ).to( this );
// Delegating does not work with namespaces. Alternatively, we could delegate all events (using `*`).
this._liveRange.delegate( 'change:range' ).to( this );
this._liveRange.delegate( 'change:content' ).to( this );
}

/**
Expand Down
180 changes: 122 additions & 58 deletions tests/model/liverange.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,12 @@ describe( 'LiveRange', () => {
range.detach();
} );

it( 'should fire change event with proper data when it is changed', () => {
it( 'should fire change:range event with proper data 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 );

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

const moveSource = new Position( root, [ 2 ] );
const moveRange = new Range( new Position( root, [ 0, 2 ] ), new Position( root, [ 0, 3 ] ) );
Expand All @@ -118,18 +118,48 @@ describe( 'LiveRange', () => {
expect( spy.args[ 0 ][ 2 ].sourcePosition.isEqual( moveSource ) ).to.be.true;
} );

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

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

const moveSource = new Position( root, [ 2, 0 ] );
const moveRange = new Range( new Position( root, [ 4, 0 ] ), new Position( root, [ 4, 2 ] ) );

const changes = {
range: moveRange,
sourcePosition: moveSource
};
const batch = {};

doc.fire( 'change', 'move', changes, batch );

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.
// We compare to the `live` range, because boundaries should not have changed.
expect( spy.args[ 0 ][ 1 ].isEqual( live ) ).to.be.true;

// Second parameter is an object with data about model changes that caused the live range to change.
expect( spy.args[ 0 ][ 2 ].type ).to.equal( 'move' );
expect( spy.args[ 0 ][ 2 ].batch ).to.equal( batch );
expect( spy.args[ 0 ][ 2 ].range.isEqual( moveRange ) ).to.be.true;
expect( spy.args[ 0 ][ 2 ].sourcePosition.isEqual( moveSource ) ).to.be.true;
} );

// Examples may seem weird when you compare them with the tree structure generated at the beginning of tests.
// Since change event is fired _after_ operation is executed on tree model, you have to imagine that generated
// structure is representing what is _after_ operation is executed. So live LiveRange properties are describing
// virtual tree that is not existing anymore and event ranges are operating on the tree generated above.
describe( 'should get transformed if', () => {
describe( 'should get transformed and fire change:range if', () => {
let live, spy;

beforeEach( () => {
live = new LiveRange( new Position( root, [ 0, 1, 4 ] ), new Position( root, [ 0, 2, 2 ] ) );

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

afterEach( () => {
Expand Down Expand Up @@ -556,35 +586,114 @@ describe( 'LiveRange', () => {
} );
} );

describe( 'should not get transformed if', () => {
let otherRoot, spy, live, clone;

before( () => {
otherRoot = doc.createRoot( '$root', 'otherRoot' );
} );
describe( 'should not get transformed but fire change:content', () => {
let spy, live, clone;

beforeEach( () => {
live = new LiveRange( new Position( root, [ 0, 1, 4 ] ), new Position( root, [ 0, 2, 2 ] ) );
clone = Range.createFromRange( live );

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

afterEach( () => {
live.detach();
} );

describe( 'insertion', () => {
it( 'is in the same parent as range start and after it', () => {
it( 'inside the range', () => {
const insertRange = new Range( new Position( root, [ 0, 1, 7 ] ), new Position( root, [ 0, 1, 9 ] ) );

doc.fire( 'change', 'insert', { range: insertRange }, null );

expect( live.isEqual( clone ) ).to.be.true;
expect( spy.called ).to.be.false;
expect( spy.calledOnce ).to.be.true;
} );
} );

describe( 'range move', () => {
it( 'inside the range', () => {
const moveSource = new Position( root, [ 4 ] );
const moveRange = new Range( new Position( root, [ 0, 1, 7 ] ), new Position( root, [ 0, 1, 9 ] ) );

const changes = {
range: moveRange,
sourcePosition: moveSource
};
doc.fire( 'change', 'move', changes, null );

expect( live.isEqual( clone ) ).to.be.true;
expect( spy.calledOnce ).to.be.true;
} );

it( 'from the range', () => {
const moveSource = new Position( root, [ 0, 1, 6 ] );
const moveRange = new Range( new Position( root, [ 4, 0 ] ), new Position( root, [ 4, 3 ] ) );

const changes = {
range: moveRange,
sourcePosition: moveSource
};
doc.fire( 'change', 'move', changes, null );

expect( live.isEqual( clone ) ).to.be.true;
expect( spy.calledOnce ).to.be.true;
} );

it( 'from the beginning of range', () => {
const moveSource = new Position( root, [ 0, 1, 4 ] );
const moveRange = new Range( new Position( root, [ 4, 0 ] ), new Position( root, [ 4, 3 ] ) );

const changes = {
range: moveRange,
sourcePosition: moveSource
};
doc.fire( 'change', 'move', changes, null );

expect( live.isEqual( clone ) ).to.be.true;
expect( spy.calledOnce ).to.be.true;
} );

it( 'from the range to the range', () => {
live.end.path = [ 0, 1, 12 ];

const moveSource = new Position( root, [ 0, 1, 6 ] );
const moveRange = new Range( new Position( root, [ 0, 1, 8 ] ), new Position( root, [ 0, 1, 10 ] ) );

const changes = {
range: moveRange,
sourcePosition: moveSource
};
doc.fire( 'change', 'move', changes, null );

expect( live.start.path ).to.deep.equal( [ 0, 1, 4 ] );
expect( live.end.path ).to.deep.equal( [ 0, 1, 12 ] );
expect( spy.calledOnce ).to.be.true;
} );
} );
} );

describe( 'should not get transformed and not fire change event if', () => {
let otherRoot, spy, live, clone;

before( () => {
otherRoot = doc.createRoot( '$root', 'otherRoot' );
} );

beforeEach( () => {
live = new LiveRange( new Position( root, [ 0, 1, 4 ] ), new Position( root, [ 0, 2, 2 ] ) );
clone = Range.createFromRange( live );

spy = sinon.spy();
live.on( 'change', spy );
} );

afterEach( () => {
live.detach();
} );

describe( 'insertion', () => {
it( 'is in the same parent as range end and after it', () => {
const insertRange = new Range( new Position( root, [ 0, 2, 7 ] ), new Position( root, [ 0, 2, 9 ] ) );

Expand Down Expand Up @@ -614,20 +723,6 @@ describe( 'LiveRange', () => {
} );

describe( 'range move', () => {
it( 'is to the same parent as range start and after it', () => {
const moveSource = new Position( root, [ 4 ] );
const moveRange = new Range( new Position( root, [ 0, 1, 7 ] ), new Position( root, [ 0, 1, 9 ] ) );

const changes = {
range: moveRange,
sourcePosition: moveSource
};
doc.fire( 'change', 'move', changes, null );

expect( live.isEqual( clone ) ).to.be.true;
expect( spy.called ).to.be.false;
} );

it( 'is to the same parent as range end and after it', () => {
const moveSource = new Position( root, [ 4 ] );
const moveRange = new Range( new Position( root, [ 0, 2, 3 ] ), new Position( root, [ 0, 2, 5 ] ) );
Expand Down Expand Up @@ -656,20 +751,6 @@ describe( 'LiveRange', () => {
expect( spy.called ).to.be.false;
} );

it( 'is from the same parent as range start and after it', () => {
const moveSource = new Position( root, [ 0, 1, 6 ] );
const moveRange = new Range( new Position( root, [ 4, 0 ] ), new Position( root, [ 4, 3 ] ) );

const changes = {
range: moveRange,
sourcePosition: moveSource
};
doc.fire( 'change', 'move', changes, null );

expect( live.isEqual( clone ) ).to.be.true;
expect( spy.called ).to.be.false;
} );

it( 'is from the same parent as range end and after it', () => {
const moveSource = new Position( root, [ 0, 2, 4 ] );
const moveRange = new Range( new Position( root, [ 4, 0 ] ), new Position( root, [ 4, 2 ] ) );
Expand Down Expand Up @@ -725,23 +806,6 @@ describe( 'LiveRange', () => {
expect( live.isEqual( clone ) ).to.be.true;
expect( spy.called ).to.be.false;
} );

it( 'is inside live range and points to live range', () => {
live.end.path = [ 0, 1, 12 ];

const moveSource = new Position( root, [ 0, 1, 6 ] );
const moveRange = new Range( new Position( root, [ 0, 1, 8 ] ), new Position( root, [ 0, 1, 10 ] ) );

const changes = {
range: moveRange,
sourcePosition: moveSource
};
doc.fire( 'change', 'move', changes, null );

expect( live.start.path ).to.deep.equal( [ 0, 1, 4 ] );
expect( live.end.path ).to.deep.equal( [ 0, 1, 12 ] );
expect( spy.calledOnce ).to.be.false;
} );
} );
} );
} );
17 changes: 17 additions & 0 deletions tests/model/markercollection.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,4 +253,21 @@ describe( 'Marker', () => {
marker.getEnd();
} ).to.throw( CKEditorError, /^marker-destroyed/ );
} );

it( 'should delegate events from live range', () => {
const range = Range.createFromParentsAndOffsets( root, 1, root, 2 );
const marker = doc.markers.set( 'name', range );

const eventRange = sinon.spy();
const eventContent = sinon.spy();

marker.on( 'change:range', eventRange );
marker.on( 'change:content', eventContent );

marker._liveRange.fire( 'change:range', null, {} );
marker._liveRange.fire( 'change:content', null, {} );

expect( eventRange.calledOnce ).to.be.true;
expect( eventContent.calledOnce ).to.be.true;
} );
} );

0 comments on commit ec22a29

Please sign in to comment.