Skip to content

Commit

Permalink
Merge pull request #11506 from ckeditor/cf/4495
Browse files Browse the repository at this point in the history 
Fix (engine): Marker changes sometimes did not trigger `change:data` event which resulted in errors in features using markers (for example, annotations not showing up in the sidebar).
  • Loading branch information
@niegowski
niegowski authored Mar 22, 2022
2 parents b50581d + 9fe69c1 commit 59c0319
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 43 deletions.
18 changes: 11 additions & 7 deletions packages/ckeditor5-engine/src/model/differ.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -336,23 +336,27 @@ export default class Differ {
* @returns {Boolean}
*/
hasDataChanges() {
if ( this._changesInElement.size > 0 ) {
return true;
}

for ( const { newMarkerData, oldMarkerData } of this._changedMarkers.values() ) {
if ( newMarkerData.affectsData !== oldMarkerData.affectsData ) {
return true;
}

if ( newMarkerData.affectsData ) {
// Skip markers, which ranges have not changed.
if ( newMarkerData.range && oldMarkerData.range && newMarkerData.range.isEqual( oldMarkerData.range ) ) {
return false;
}
const markerAdded = newMarkerData.range && !oldMarkerData.range;
const markerRemoved = !newMarkerData.range && oldMarkerData.range;
const markerChanged = newMarkerData.range && oldMarkerData.range && !newMarkerData.range.isEqual( oldMarkerData.range );

return true;
if ( markerAdded || markerRemoved || markerChanged ) {
return true;
}
}
}

// If markers do not affect the data, check whether there are some changes in elements.
return this._changesInElement.size > 0;
return false;
}

/**
Expand Down
105 changes: 69 additions & 36 deletions packages/ckeditor5-engine/tests/model/differ.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1516,6 +1516,26 @@ describe( 'Differ', () => {
] );
} );

it( 'add marker not affecting data', () => {
differ.bufferMarkerChange( 'name', { range: null, affectsData: false }, { range, affectsData: false } );

expect( differ.getMarkersToRemove() ).to.deep.equal( [] );

expect( differ.getMarkersToAdd() ).to.deep.equal( [
{ name: 'name', range }
] );

expect( differ.getChangedMarkers() ).to.deep.equal( [
{
name: 'name',
data: {
oldRange: null,
newRange: range
}
}
] );
} );

it( 'remove marker', () => {
differ.bufferMarkerChange( 'name', { range, affectsData: true }, { range: null, affectsData: true } );

Expand Down Expand Up @@ -1558,30 +1578,16 @@ describe( 'Differ', () => {
] );
} );

it( 'add marker not affecting data', () => {
differ.bufferMarkerChange( 'name', { range, affectsData: false }, { range: rangeB, affectsData: false } );

expect( differ.hasDataChanges() ).to.be.false;
} );

it( 'add marker affecting data', () => {
differ.bufferMarkerChange( 'name', { range, affectsData: true }, { range: rangeB, affectsData: true } );

expect( differ.hasDataChanges() ).to.be.true;
} );

it( 'add marker and remove it', () => {
differ.bufferMarkerChange( 'name', { range: null, affectsData: true }, { range, affectsData: true } );
differ.bufferMarkerChange( 'name', { range, affectsData: true }, { range: null, affectsData: true } );

expect( differ.getMarkersToRemove() ).to.deep.equal( [] );
expect( differ.getMarkersToAdd() ).to.deep.equal( [] );
expect( differ.getChangedMarkers() ).to.deep.equal( [] );

expect( differ.hasDataChanges() ).to.be.false;
} );

it( 'add marker and change it', () => {
it( 'add marker and change range', () => {
differ.bufferMarkerChange( 'name', { range: null, affectsData: true }, { range, affectsData: true } );
differ.bufferMarkerChange( 'name', { range, affectsData: true }, { range: rangeB, affectsData: true } );

Expand All @@ -1602,46 +1608,73 @@ describe( 'Differ', () => {
] );
} );

it( 'add marker and change affectsData', () => {
differ.bufferMarkerChange( 'name', { range: null, affectsData: false }, { range, affectsData: false } );
differ.bufferMarkerChange( 'name', { range, affectsData: false }, { range, affectsData: true } );

expect( differ.getMarkersToRemove() ).to.deep.equal( [] );

expect( differ.getMarkersToAdd() ).to.deep.equal( [
{ name: 'name', range }
] );

expect( differ.getChangedMarkers() ).to.deep.equal( [
{
name: 'name',
data: {
oldRange: null,
newRange: range
}
}
] );
} );

describe( 'hasDataChanges()', () => {
it( 'should return `true` when marker stops affecting data', () => {
it( 'should return `true` when the range changes and the marker affects data', () => {
differ.bufferMarkerChange( 'name', { range, affectsData: true }, { range: rangeB, affectsData: true } );
differ.bufferMarkerChange( 'name', { range: rangeB, affectsData: true }, { range, affectsData: false } );

// As marker is longer outputted to the data, the data may be changed.
expect( differ.hasDataChanges() ).to.be.true;
} );

it( 'should return `true` when marker starts affecting data', () => {
differ.bufferMarkerChange( 'name', { range, affectsData: false }, { range: rangeB, affectsData: false } );
differ.bufferMarkerChange( 'name', { range: rangeB, affectsData: false }, { range, affectsData: true } );
it( 'should return `false` when the range does not change', () => {
differ.bufferMarkerChange( 'name', { range, affectsData: true }, { range, affectsData: true } );

// As marker is longer outputted to the data, the data may be changed.
expect( differ.hasDataChanges() ).to.be.true;
expect( differ.hasDataChanges() ).to.be.false;
} );

it( 'should return `false` when continuous marker changes do not change affecting data and was initially set to false', () => {
differ.bufferMarkerChange( 'name', { range, affectsData: false }, { range: rangeB, affectsData: false } );
differ.bufferMarkerChange( 'name', { range: rangeB, affectsData: false }, { range, affectsData: true } );
differ.bufferMarkerChange( 'name', { range, affectsData: true }, { range, affectsData: false } );
it( 'should return `false` when multiple changes result in not changed range', () => {
differ.bufferMarkerChange( 'name', { range, affectsData: true }, { range: rangeB, affectsData: true } );
differ.bufferMarkerChange( 'name', { range: rangeB, affectsData: true }, { range, affectsData: true } );

// As marker is longer outputted to the data, the data may be changed.
expect( differ.hasDataChanges() ).to.be.false;
} );

it( 'should return `false` when the range does not change', () => {
differ.bufferMarkerChange( 'name', { range, affectsData: true }, { range, affectsData: true } );
it( 'should return `true` when marker stops affecting data', () => {
differ.bufferMarkerChange( 'name', { range, affectsData: true }, { range, affectsData: false } );

// As marker is longer outputted to the data, the data may be changed.
expect( differ.hasDataChanges() ).to.be.false;
expect( differ.hasDataChanges() ).to.be.true;
} );

it( 'should return `false` when the continuous changes result in not changed range', () => {
differ.bufferMarkerChange( 'name', { range, affectsData: true }, { range: rangeB, affectsData: true } );
differ.bufferMarkerChange( 'name', { range: rangeB, affectsData: true }, { range, affectsData: true } );
it( 'should return `true` when marker starts affecting data', () => {
differ.bufferMarkerChange( 'name', { range, affectsData: false }, { range, affectsData: true } );

expect( differ.hasDataChanges() ).to.be.true;
} );

it( 'should return `false` when multiple marker changes do not change affecting data (which is false)', () => {
differ.bufferMarkerChange( 'name', { range, affectsData: false }, { range: rangeB, affectsData: true } );
differ.bufferMarkerChange( 'name', { range: rangeB, affectsData: true }, { range: rangeB, affectsData: false } );

// As marker is longer outputted to the data, the data may be changed.
expect( differ.hasDataChanges() ).to.be.false;
} );

it( 'should return `true` if at least one marker changed', () => {
differ.bufferMarkerChange( 'nameA', { range, affectsData: true }, { range, affectsData: true } );
differ.bufferMarkerChange( 'nameB', { range, affectsData: true }, { range: rangeB, affectsData: true } );
differ.bufferMarkerChange( 'nameC', { range, affectsData: true }, { range, affectsData: true } );

expect( differ.hasDataChanges() ).to.be.true;
} );
} );

it( 'change marker and remove it', () => {
Expand Down

0 comments on commit 59c0319

Please sign in to comment.