From 900a3fb6d9d4fcc72984950b06003ac20cd793dc Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 1 Feb 2018 11:19:21 +0100 Subject: [PATCH 01/17] Added `usingOperation` flag to `setMarker` and `removeMarker`. --- src/model/writer.js | 15 ++- tests/controller/datacontroller.js | 6 +- tests/controller/editingcontroller.js | 75 ++++++------- tests/conversion/buildmodelconverter.js | 80 +++++++------- .../model-selection-to-view-converters.js | 10 +- tests/conversion/model-to-view-converters.js | 100 +++++++++--------- tests/conversion/modelconversiondispatcher.js | 14 +-- tests/manual/highlight.js | 12 +-- tests/manual/markers.js | 12 +-- tests/model/document.js | 4 +- tests/model/operation/markeroperation.js | 4 +- tests/model/writer.js | 34 +++--- 12 files changed, 192 insertions(+), 174 deletions(-) diff --git a/src/model/writer.js b/src/model/writer.js index 8283d7092..bc9770ced 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -194,7 +194,7 @@ export default class Writer { markerRange.end._getCombined( rangeRootPosition, position ) ); - this.setMarker( markerName, range ); + this.setMarker( markerName, range, { usingOperation: true } ); } } } @@ -775,7 +775,7 @@ export default class Writer { * @param {module:engine/model/markercollection~Marker|String} markerOrName Marker or marker name to add or update. * @param {module:engine/model/range~Range} [newRange] Marker range. */ - setMarker( markerOrName, newRange ) { + setMarker( markerOrName, newRange, options = { usingOperation: false } ) { this._assertWriterUsedCorrectly(); const name = typeof markerOrName == 'string' ? markerOrName : markerOrName.name; @@ -790,6 +790,11 @@ export default class Writer { throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' ); } + if ( !options.usingOperation ) { + const marker = this.model.markers.set( name, newRange ); + return marker; + } + const currentRange = currentMarker ? currentMarker.getRange() : null; if ( !newRange ) { @@ -807,7 +812,7 @@ export default class Writer { * * @param {module:engine/model/markercollection~Marker|String} markerOrName Marker or marker name to remove. */ - removeMarker( markerOrName ) { + removeMarker( markerOrName, options = { usingOperation: false } ) { this._assertWriterUsedCorrectly(); const name = typeof markerOrName == 'string' ? markerOrName : markerOrName.name; @@ -821,6 +826,10 @@ export default class Writer { throw new CKEditorError( 'writer-removeMarker-no-marker: Trying to remove marker which does not exist.' ); } + if ( !options.usingOperation ) { + return this.model.markers.remove( name ); + } + const oldRange = this.model.markers.get( name ).getRange(); addMarkerOperation( this, name, oldRange, null ); diff --git a/tests/controller/datacontroller.js b/tests/controller/datacontroller.js index 341a8c71e..577950384 100644 --- a/tests/controller/datacontroller.js +++ b/tests/controller/datacontroller.js @@ -321,7 +321,7 @@ describe( 'DataController', () => { model.change( writer => { writer.insert( modelElement, modelRoot, 0 ); - writer.setMarker( 'marker:a', Range.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 ) ); + writer.setMarker( 'marker:a', Range.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 ), { usingOperation: true } ); } ); const viewDocumentFragment = data.toView( modelElement ); @@ -343,8 +343,8 @@ describe( 'DataController', () => { model.change( writer => { writer.insert( modelElement, modelRoot, 0 ); - writer.setMarker( 'marker:a', Range.createFromParentsAndOffsets( modelP1, 1, modelP1, 3 ) ); - writer.setMarker( 'marker:b', Range.createFromParentsAndOffsets( modelP2, 0, modelP2, 2 ) ); + writer.setMarker( 'marker:a', Range.createFromParentsAndOffsets( modelP1, 1, modelP1, 3 ), { usingOperation: true } ); + writer.setMarker( 'marker:b', Range.createFromParentsAndOffsets( modelP2, 0, modelP2, 2 ), { usingOperation: true } ); } ); const viewDocumentFragment = data.toView( modelP1 ); diff --git a/tests/controller/editingcontroller.js b/tests/controller/editingcontroller.js index baa866a3f..87452a126 100644 --- a/tests/controller/editingcontroller.js +++ b/tests/controller/editingcontroller.js @@ -232,8 +232,8 @@ describe( 'EditingController', () => { it( 'should convert adding marker', () => { const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) ); - model.change( () => { - model.markers.set( 'marker', range ); + model.change( writer => { + writer.setMarker( 'marker', range ); } ); expect( getViewData( editing.view, { withoutSelection: true } ) ) @@ -243,12 +243,12 @@ describe( 'EditingController', () => { it( 'should convert removing marker', () => { const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) ); - model.change( () => { - model.markers.set( 'marker', range ); + model.change( writer => { + writer.setMarker( 'marker', range ); } ); - model.change( () => { - model.markers.remove( 'marker' ); + model.change( writer => { + writer.removeMarker( 'marker' ); } ); expect( getViewData( editing.view, { withoutSelection: true } ) ) @@ -258,14 +258,14 @@ describe( 'EditingController', () => { it( 'should convert changing marker', () => { const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) ); - model.change( () => { - model.markers.set( 'marker', range ); + model.change( writer => { + writer.setMarker( 'marker', range ); } ); const range2 = new ModelRange( new ModelPosition( modelRoot, [ 0, 0 ] ), new ModelPosition( modelRoot, [ 0, 2 ] ) ); - model.change( () => { - model.markers.set( 'marker', range2 ); + model.change( writer => { + writer.setMarker( 'marker', range2 ); } ); expect( getViewData( editing.view, { withoutSelection: true } ) ) @@ -275,11 +275,8 @@ describe( 'EditingController', () => { it( 'should convert insertion into marker', () => { const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) ); - model.change( () => { - model.markers.set( 'marker', range ); - } ); - model.change( writer => { + writer.setMarker( 'marker', range ); writer.insertText( 'xyz', new ModelPosition( modelRoot, [ 1, 0 ] ) ); } ); @@ -290,8 +287,8 @@ describe( 'EditingController', () => { it( 'should convert move to marker', () => { const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) ); - model.change( () => { - model.markers.set( 'marker', range ); + model.change( writer => { + writer.setMarker( 'marker', range ); } ); model.change( writer => { @@ -308,8 +305,8 @@ describe( 'EditingController', () => { it( 'should convert move from marker', () => { const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) ); - model.change( () => { - model.markers.set( 'marker', range ); + model.change( writer => { + writer.setMarker( 'marker', range ); } ); model.change( writer => { @@ -326,8 +323,8 @@ describe( 'EditingController', () => { it( 'should convert the whole marker move', () => { const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 0, 3 ] ) ); - model.change( () => { - model.markers.set( 'marker', range ); + model.change( writer => { + writer.setMarker( 'marker', range ); } ); model.change( writer => { @@ -386,7 +383,7 @@ describe( 'EditingController', () => { it( 'should remove marker from view if it will be affected by insert operation', () => { model.change( writer => { - writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) ); + writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } ); } ); // Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority. @@ -404,7 +401,7 @@ describe( 'EditingController', () => { it( 'should remove marker from view if it will be affected by remove operation', () => { model.change( writer => { - writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) ); + writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } ); } ); // Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority. @@ -422,7 +419,7 @@ describe( 'EditingController', () => { it( 'should remove marker from view if it will be affected by move operation', () => { model.change( writer => { - writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) ); + writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } ); } ); // Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority. @@ -442,7 +439,11 @@ describe( 'EditingController', () => { it( 'should remove marker from view if it will be affected by rename operation', () => { model.change( writer => { - writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 ) ); + writer.setMarker( + 'marker', + ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 ), + { usingOperation: true } + ); } ); // Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority. @@ -460,7 +461,7 @@ describe( 'EditingController', () => { it( 'should remove marker from view if it will be affected by marker operation', () => { model.change( writer => { - writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) ); + writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } ); } ); // Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority. @@ -472,7 +473,7 @@ describe( 'EditingController', () => { model.change( writer => { const p2 = p1.nextSibling; - writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p2, 1, p2, 2 ) ); + writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p2, 1, p2, 2 ), { usingOperation: true } ); } ); expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '

foo

bar

' ); @@ -480,7 +481,7 @@ describe( 'EditingController', () => { it( 'should remove marker from view if it is removed through marker collection', () => { model.change( writer => { - writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) ); + writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } ); } ); // Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority. @@ -489,8 +490,8 @@ describe( 'EditingController', () => { expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '

foo

bar

' ); }, { priority: 'low' } ); - model.change( () => { - model.markers.remove( 'marker' ); + model.change( writer => { + writer.removeMarker( 'marker' ); } ); expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '

foo

bar

' ); @@ -498,7 +499,7 @@ describe( 'EditingController', () => { it( 'should not remove marker if applied operation is an attribute operation', () => { model.change( writer => { - writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) ); + writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } ); } ); // Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority. @@ -516,7 +517,7 @@ describe( 'EditingController', () => { it( 'should not crash if multiple operations affect a marker', () => { model.change( writer => { - writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) ); + writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } ); } ); model.change( writer => { @@ -530,12 +531,12 @@ describe( 'EditingController', () => { it( 'should not crash if marker is removed, added and removed #1', () => { model.change( writer => { - writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) ); + writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } ); } ); model.change( writer => { writer.insertText( 'a', p1, 0 ); - writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 3, p1, 4 ) ); + writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 3, p1, 4 ), { usingOperation: true } ); writer.insertText( 'a', p1, 0 ); } ); @@ -544,13 +545,13 @@ describe( 'EditingController', () => { it( 'should not crash if marker is removed, added and removed #2', () => { model.change( writer => { - writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) ); + writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } ); } ); model.change( writer => { - writer.removeMarker( 'marker' ); - writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 0, p1, 1 ) ); - writer.removeMarker( 'marker' ); + writer.removeMarker( 'marker', { usingOperation: true } ); + writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 0, p1, 1 ), { usingOperation: true } ); + writer.removeMarker( 'marker', { usingOperation: true } ); } ); expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '

foo

bar

' ); diff --git a/tests/conversion/buildmodelconverter.js b/tests/conversion/buildmodelconverter.js index db7c18c68..ceec5b6c0 100644 --- a/tests/conversion/buildmodelconverter.js +++ b/tests/conversion/buildmodelconverter.js @@ -348,8 +348,8 @@ describe( 'Model converter builder', () => { attributes: { title: 'highlight title' } } ); - model.change( () => { - model.markers.set( 'search', ModelRange.createFromParentsAndOffsets( modelElement, 2, modelElement, 4 ) ); + model.change( writer => { + writer.setMarker( 'search', ModelRange.createFromParentsAndOffsets( modelElement, 2, modelElement, 4 ) ); } ); expect( viewToString( viewRoot ) ).to.equal( @@ -363,8 +363,8 @@ describe( 'Model converter builder', () => { expect( viewRoot.getChild( 0 ).getChild( 1 ).priority ).to.equal( 3 ); - model.change( () => { - model.markers.remove( 'search' ); + model.change( writer => { + writer.removeMarker( 'search' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -377,8 +377,8 @@ describe( 'Model converter builder', () => { attributes: { title: 'highlight title' } } ) ); - model.change( () => { - model.markers.set( 'search', ModelRange.createFromParentsAndOffsets( modelElement, 2, modelElement, 4 ) ); + model.change( writer => { + writer.setMarker( 'search', ModelRange.createFromParentsAndOffsets( modelElement, 2, modelElement, 4 ) ); } ); expect( viewToString( viewRoot ) ).to.equal( @@ -392,8 +392,8 @@ describe( 'Model converter builder', () => { expect( viewRoot.getChild( 0 ).getChild( 1 ).priority ).to.equal( 12 ); - model.change( () => { - model.markers.remove( 'search' ); + model.change( writer => { + writer.removeMarker( 'search' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -404,14 +404,14 @@ describe( 'Model converter builder', () => { class: 'highlight' } ); - model.change( () => { - model.markers.set( 'search', ModelRange.createFromParentsAndOffsets( modelElement, 2, modelElement, 2 ) ); + model.change( writer => { + writer.setMarker( 'search', ModelRange.createFromParentsAndOffsets( modelElement, 2, modelElement, 2 ) ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); - model.change( () => { - model.markers.remove( 'search' ); + model.change( writer => { + writer.removeMarker( 'search' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -421,8 +421,8 @@ describe( 'Model converter builder', () => { buildModelConverter().for( dispatcher ).fromMarker( 'search' ).toHighlight( { class: 'highlight' } ); buildModelConverter().for( dispatcher ).fromMarker( 'search' ).withPriority( 'high' ).toHighlight( { class: 'override' } ); - model.change( () => { - model.markers.set( 'search', ModelRange.createFromParentsAndOffsets( modelElement, 2, modelElement, 4 ) ); + model.change( writer => { + writer.setMarker( 'search', ModelRange.createFromParentsAndOffsets( modelElement, 2, modelElement, 4 ) ); } ); expect( viewToString( viewRoot ) ).to.equal( @@ -468,14 +468,14 @@ describe( 'Model converter builder', () => { it( 'using passed view element name', () => { buildModelConverter().for( dispatcher ).fromMarker( 'search' ).toElement( 'span' ); - model.change( () => { - model.markers.set( 'search', range ); + model.change( writer => { + writer.setMarker( 'search', range ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); - model.change( () => { - model.markers.remove( 'search' ); + model.change( writer => { + writer.removeMarker( 'search' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -485,14 +485,14 @@ describe( 'Model converter builder', () => { const viewElement = new ViewUIElement( 'span', { class: 'search' } ); buildModelConverter().for( dispatcher ).fromMarker( 'search' ).toElement( viewElement ); - model.change( () => { - model.markers.set( 'search', range ); + model.change( writer => { + writer.setMarker( 'search', range ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); - model.change( () => { - model.markers.remove( 'search' ); + model.change( writer => { + writer.removeMarker( 'search' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -505,14 +505,14 @@ describe( 'Model converter builder', () => { return new ViewUIElement( 'span', { class: className } ); } ); - model.change( () => { - model.markers.set( 'search:red', range ); + model.change( writer => { + writer.setMarker( 'search:red', range ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); - model.change( () => { - model.markers.remove( 'search:red' ); + model.change( writer => { + writer.removeMarker( 'search:red' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -527,14 +527,14 @@ describe( 'Model converter builder', () => { it( 'using passed view element name', () => { buildModelConverter().for( dispatcher ).fromMarker( 'search' ).toElement( 'span' ); - model.change( () => { - model.markers.set( 'search', range ); + model.change( writer => { + writer.setMarker( 'search', range ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); - model.change( () => { - model.markers.remove( 'search' ); + model.change( writer => { + writer.removeMarker( 'search' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -544,16 +544,16 @@ describe( 'Model converter builder', () => { const viewElement = new ViewUIElement( 'span', { class: 'search' } ); buildModelConverter().for( dispatcher ).fromMarker( 'search' ).toElement( viewElement ); - model.change( () => { - model.markers.set( 'search', range ); + model.change( writer => { + writer.setMarker( 'search', range ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); - model.change( () => { - model.markers.remove( 'search' ); + model.change( writer => { + writer.removeMarker( 'search' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -566,16 +566,16 @@ describe( 'Model converter builder', () => { return new ViewUIElement( 'span', { class: className } ); } ); - model.change( () => { - model.markers.set( 'search:red', range ); + model.change( writer => { + writer.setMarker( 'search:red', range ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); - model.change( () => { - model.markers.remove( 'search:red' ); + model.change( writer => { + writer.removeMarker( 'search:red' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -588,8 +588,8 @@ describe( 'Model converter builder', () => { buildModelConverter().for( dispatcher ).fromMarker( 'search' ).toElement( 'normal' ); buildModelConverter().for( dispatcher ).fromMarker( 'search' ).withPriority( 'high' ).toElement( 'high' ); - model.change( () => { - model.markers.set( 'search', range ); + model.change( writer => { + writer.setMarker( 'search', range ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); diff --git a/tests/conversion/model-selection-to-view-converters.js b/tests/conversion/model-selection-to-view-converters.js index e09ac0ca5..29a12e2be 100644 --- a/tests/conversion/model-selection-to-view-converters.js +++ b/tests/conversion/model-selection-to-view-converters.js @@ -176,6 +176,8 @@ describe( 'model-selection-to-view-converters', () => { } ); describe( 'collapsed selection', () => { + let marker; + it( 'in container', () => { test( [ 1, 1 ], @@ -194,9 +196,9 @@ describe( 'model-selection-to-view-converters', () => { it( 'in attribute and marker', () => { setModelData( model, 'fo<$text bold="true">obar' ); - const marker = model.markers.set( 'marker', ModelRange.createFromParentsAndOffsets( modelRoot, 1, modelRoot, 5 ) ); model.change( writer => { + marker = writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( modelRoot, 1, modelRoot, 5 ) ); writer.setSelection( new ModelRange( ModelPosition.createAt( modelRoot, 3 ) ) ); } ); @@ -218,9 +220,9 @@ describe( 'model-selection-to-view-converters', () => { it( 'in attribute and marker - no attribute', () => { setModelData( model, 'fo<$text bold="true">obar' ); - const marker = model.markers.set( 'marker', ModelRange.createFromParentsAndOffsets( modelRoot, 1, modelRoot, 5 ) ); model.change( writer => { + marker = writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( modelRoot, 1, modelRoot, 5 ) ); writer.setSelection( new ModelRange( ModelPosition.createAt( modelRoot, 3 ) ) ); writer.removeSelectionAttribute( 'bold' ); } ); @@ -246,9 +248,9 @@ describe( 'model-selection-to-view-converters', () => { ) ); setModelData( model, 'foobar' ); - const marker = model.markers.set( 'marker2', ModelRange.createFromParentsAndOffsets( modelRoot, 1, modelRoot, 5 ) ); model.change( writer => { + marker = writer.setMarker( 'marker2', ModelRange.createFromParentsAndOffsets( modelRoot, 1, modelRoot, 5 ) ); writer.setSelection( new ModelRange( ModelPosition.createAt( modelRoot, 3 ) ) ); } ); @@ -271,9 +273,9 @@ describe( 'model-selection-to-view-converters', () => { dispatcher.on( 'addMarker:marker3', highlightText( () => null ) ); setModelData( model, 'foobar' ); - const marker = model.markers.set( 'marker3', ModelRange.createFromParentsAndOffsets( modelRoot, 1, modelRoot, 5 ) ); model.change( writer => { + marker = writer.setMarker( 'marker3', ModelRange.createFromParentsAndOffsets( modelRoot, 1, modelRoot, 5 ) ); writer.setSelection( new ModelRange( ModelPosition.createAt( modelRoot, 3 ) ) ); } ); diff --git a/tests/conversion/model-to-view-converters.js b/tests/conversion/model-to-view-converters.js index a0a82eea3..25dc83f44 100644 --- a/tests/conversion/model-to-view-converters.js +++ b/tests/conversion/model-to-view-converters.js @@ -370,14 +370,14 @@ describe( 'model-to-view-converters', () => { dispatcher.on( 'addMarker:marker', insertUIElement( viewUi ) ); dispatcher.on( 'removeMarker:marker', removeUIElement( viewUi ) ); - model.change( () => { - model.markers.set( 'marker', range ); + model.change( writer => { + writer.setMarker( 'marker', range ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); - model.change( () => { - model.markers.remove( 'marker' ); + model.change( writer => { + writer.removeMarker( 'marker' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -389,14 +389,14 @@ describe( 'model-to-view-converters', () => { dispatcher.on( 'addMarker:marker', insertUIElement( () => viewUi ) ); dispatcher.on( 'removeMarker:marker', removeUIElement( () => viewUi ) ); - model.change( () => { - model.markers.set( 'marker', range ); + model.change( writer => { + writer.setMarker( 'marker', range ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); - model.change( () => { - model.markers.remove( 'marker' ); + model.change( writer => { + writer.removeMarker( 'marker' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -422,14 +422,14 @@ describe( 'model-to-view-converters', () => { dispatcher.on( 'addMarker:marker', insertUIElement( () => null ) ); dispatcher.on( 'removeMarker:marker', removeUIElement( () => null ) ); - model.change( () => { - model.markers.set( 'marker', range ); + model.change( writer => { + writer.setMarker( 'marker', range ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); - model.change( () => { - model.markers.remove( 'marker' ); + model.change( writer => { + writer.removeMarker( 'marker' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -447,15 +447,15 @@ describe( 'model-to-view-converters', () => { dispatcher.on( 'addMarker:marker', insertUIElement( viewUi ) ); dispatcher.on( 'removeMarker:marker', removeUIElement( viewUi ) ); - model.change( () => { - model.markers.set( 'marker', range ); + model.change( writer => { + writer.setMarker( 'marker', range ); } ); expect( viewToString( viewRoot ) ) .to.equal( '

foobar

' ); - model.change( () => { - model.markers.remove( 'marker' ); + model.change( writer => { + writer.removeMarker( 'marker' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -467,15 +467,15 @@ describe( 'model-to-view-converters', () => { dispatcher.on( 'addMarker:marker', insertUIElement( viewUi ) ); dispatcher.on( 'removeMarker:marker', removeUIElement( viewUi ) ); - model.change( () => { - model.markers.set( 'marker', range ); + model.change( writer => { + writer.setMarker( 'marker', range ); } ); expect( viewToString( viewRoot ) ) .to.equal( '

foobar

' ); - model.change( () => { - model.markers.remove( 'marker' ); + model.change( writer => { + writer.removeMarker( 'marker' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -493,16 +493,16 @@ describe( 'model-to-view-converters', () => { dispatcher.on( 'addMarker:marker', insertUIElement( creator ) ); dispatcher.on( 'removeMarker:marker', removeUIElement( creator ) ); - model.change( () => { - model.markers.set( 'marker', range ); + model.change( writer => { + writer.setMarker( 'marker', range ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); - model.change( () => { - model.markers.remove( 'marker' ); + model.change( writer => { + writer.removeMarker( 'marker' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); @@ -726,8 +726,8 @@ describe( 'model-to-view-converters', () => { dispatcher.on( 'addMarker:marker', highlightElement( highlightDescriptor ) ); dispatcher.on( 'removeMarker:marker', removeHighlight( highlightDescriptor ) ); - model.change( () => { - model.markers.set( 'marker', markerRange ); + model.change( writer => { + writer.setMarker( 'marker', markerRange ); } ); expect( viewToString( viewRoot ) ).to.equal( @@ -741,8 +741,8 @@ describe( 'model-to-view-converters', () => { '' ); - model.change( () => { - model.markers.remove( 'marker' ); + model.change( writer => { + writer.removeMarker( 'marker' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foo

bar

' ); @@ -759,8 +759,8 @@ describe( 'model-to-view-converters', () => { dispatcher.on( 'addMarker:marker', highlightElement( newDescriptor ), { priority: 'high' } ); dispatcher.on( 'removeMarker:marker', removeHighlight( newDescriptor ), { priority: 'high' } ); - model.change( () => { - model.markers.set( 'marker', markerRange ); + model.change( writer => { + writer.setMarker( 'marker', markerRange ); } ); expect( viewToString( viewRoot ) ).to.equal( @@ -774,8 +774,8 @@ describe( 'model-to-view-converters', () => { '' ); - model.change( () => { - model.markers.remove( 'marker' ); + model.change( writer => { + writer.removeMarker( 'marker' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foo

bar

' ); @@ -786,14 +786,14 @@ describe( 'model-to-view-converters', () => { dispatcher.on( 'addMarker:marker', highlightElement( () => null ), { priority: 'high' } ); dispatcher.on( 'removeMarker:marker', removeHighlight( () => null ), { priority: 'high' } ); - model.change( () => { - model.markers.set( 'marker', markerRange ); + model.change( writer => { + writer.setMarker( 'marker', markerRange ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foo

bar

' ); - model.change( () => { - model.markers.remove( 'marker' ); + model.change( writer => { + writer.removeMarker( 'marker' ); } ); expect( viewToString( viewRoot ) ).to.equal( '

foo

bar

' ); @@ -840,8 +840,8 @@ describe( 'model-to-view-converters', () => { } ); it( 'should use addHighlight and removeHighlight on elements and not convert children nodes', () => { - model.change( () => { - model.markers.set( 'marker', markerRange ); + model.change( writer => { + writer.setMarker( 'marker', markerRange ); } ); expect( viewToString( viewRoot ) ).to.equal( @@ -852,8 +852,8 @@ describe( 'model-to-view-converters', () => { '' ); - model.change( () => { - model.markers.remove( 'marker' ); + model.change( writer => { + writer.removeMarker( 'marker' ); } ); expect( viewToString( viewRoot ) ).to.equal( '
foo
' ); @@ -866,8 +866,8 @@ describe( 'model-to-view-converters', () => { dispatcher.on( 'addMarker:marker', highlightElement( newDescriptor ), { priority: 'high' } ); dispatcher.on( 'removeMarker:marker', removeHighlight( newDescriptor ), { priority: 'high' } ); - model.change( () => { - model.markers.set( 'marker', markerRange ); + model.change( writer => { + writer.setMarker( 'marker', markerRange ); } ); expect( viewToString( viewRoot ) ).to.equal( @@ -878,8 +878,8 @@ describe( 'model-to-view-converters', () => { '' ); - model.change( () => { - model.markers.remove( 'marker' ); + model.change( writer => { + writer.removeMarker( 'marker' ); } ); expect( viewToString( viewRoot ) ).to.equal( '
foo
' ); @@ -901,8 +901,8 @@ describe( 'model-to-view-converters', () => { expect( id ).to.equal( 'marker:foo-bar-baz' ); } ); - model.change( () => { - model.markers.set( 'marker2', markerRange ); + model.change( writer => { + writer.setMarker( 'marker2', markerRange ); } ); } ); @@ -911,14 +911,14 @@ describe( 'model-to-view-converters', () => { dispatcher.on( 'addMarker:marker2', highlightElement( () => null ) ); dispatcher.on( 'removeMarker:marker2', removeHighlight( () => null ) ); - model.change( () => { - model.markers.set( 'marker2', markerRange ); + model.change( writer => { + writer.setMarker( 'marker2', markerRange ); } ); expect( viewToString( viewRoot ) ).to.equal( '
foo
' ); - model.change( () => { - model.markers.remove( 'marker2' ); + model.change( writer => { + writer.removeMarker( 'marker2' ); } ); expect( viewToString( viewRoot ) ).to.equal( '
foo
' ); diff --git a/tests/conversion/modelconversiondispatcher.js b/tests/conversion/modelconversiondispatcher.js index 287d850ec..879f5df42 100644 --- a/tests/conversion/modelconversiondispatcher.js +++ b/tests/conversion/modelconversiondispatcher.js @@ -333,10 +333,9 @@ describe( 'ModelConversionDispatcher', () => { writer.setSelection( new ModelRange( new ModelPosition( root, [ 1 ] ), new ModelPosition( root, [ 1 ] ) ) ); + writer.setMarker( 'name', ModelRange.createFromParentsAndOffsets( root, 0, root, 2 ) ); } ); - model.markers.set( 'name', ModelRange.createFromParentsAndOffsets( root, 0, root, 2 ) ); - sinon.spy( dispatcher, 'fire' ); const markers = Array.from( model.markers.getMarkersAtPosition( doc.selection.getFirstPosition() ) ); @@ -346,7 +345,9 @@ describe( 'ModelConversionDispatcher', () => { } ); it( 'should not fire events for markers for non-collapsed selection', () => { - model.markers.set( 'name', ModelRange.createFromParentsAndOffsets( root, 0, root, 2 ) ); + model.change( writer => { + writer.setMarker( 'name', ModelRange.createFromParentsAndOffsets( root, 0, root, 2 ) ); + } ); sinon.spy( dispatcher, 'fire' ); @@ -384,8 +385,8 @@ describe( 'ModelConversionDispatcher', () => { } }; - model.markers.set( 'name', ModelRange.createFromParentsAndOffsets( root, 0, root, 1 ) ); model.change( writer => { + writer.setMarker( 'name', ModelRange.createFromParentsAndOffsets( root, 0, root, 1 ) ); writer.setSelection( ModelRange.createFromParentsAndOffsets( caption, 1, caption, 1 ) ); } ); sinon.spy( dispatcher, 'fire' ); @@ -402,11 +403,10 @@ describe( 'ModelConversionDispatcher', () => { writer.setSelection( new ModelRange( new ModelPosition( root, [ 1 ] ), new ModelPosition( root, [ 1 ] ) ) ); + writer.setMarker( 'foo', ModelRange.createFromParentsAndOffsets( root, 0, root, 2 ) ); + writer.setMarker( 'bar', ModelRange.createFromParentsAndOffsets( root, 0, root, 2 ) ); } ); - model.markers.set( 'foo', ModelRange.createFromParentsAndOffsets( root, 0, root, 2 ) ); - model.markers.set( 'bar', ModelRange.createFromParentsAndOffsets( root, 0, root, 2 ) ); - sinon.spy( dispatcher, 'fire' ); dispatcher.on( 'addMarker:foo', ( evt, data, consumable ) => { diff --git a/tests/manual/highlight.js b/tests/manual/highlight.js index 5db788253..886e8bd30 100644 --- a/tests/manual/highlight.js +++ b/tests/manual/highlight.js @@ -103,9 +103,9 @@ ClassicEditor.create( global.document.querySelector( '#editor' ), { document.getElementById( 'remove-markers' ).addEventListener( 'mousedown', evt => { const markers = editor.model.markers; - editor.model.change( () => { + editor.model.change( writer => { for ( const marker of markers ) { - markers.remove( marker ); + writer.removeMarker( marker ); } } ); @@ -117,14 +117,14 @@ ClassicEditor.create( global.document.querySelector( '#editor' ), { } ); function addMarker( editor, color ) { - editor.model.change( () => { + editor.model.change( writer => { const range = ModelRange.createFromRange( editor.model.document.selection.getFirstRange() ); - editor.model.markers.set( 'marker:' + color, range ); + writer.setMarker( 'marker:' + color, range ); } ); } function removeMarker( editor, color ) { - editor.model.change( () => { - editor.model.markers.remove( 'marker:' + color ); + editor.model.change( writer => { + writer.removeMarker( 'marker:' + color ); } ); } diff --git a/tests/manual/markers.js b/tests/manual/markers.js index 5ba17068c..3a33fe786 100644 --- a/tests/manual/markers.js +++ b/tests/manual/markers.js @@ -73,13 +73,13 @@ ClassicEditor moveSelectionByOffset( 1 ); } ); - model.change( () => { + model.change( writer => { const root = model.document.getRoot(); const range = new Range( new Position( root, [ 0, 10 ] ), new Position( root, [ 0, 16 ] ) ); const name = 'highlight:yellow:' + uid(); markerNames.push( name ); - model.markers.set( name, range ); + writer.setMarker( name, range ); } ); } ) .catch( err => { @@ -91,17 +91,17 @@ function uid() { } function addHighlight( color ) { - model.change( () => { + model.change( writer => { const range = Range.createFromRange( model.document.selection.getFirstRange() ); const name = 'highlight:' + color + ':' + uid(); markerNames.push( name ); - model.markers.set( name, range ); + writer.setMarker( name, range ); } ); } function removeHighlight() { - model.change( () => { + model.change( writer => { const pos = model.document.selection.getFirstPosition(); for ( let i = 0; i < markerNames.length; i++ ) { @@ -110,7 +110,7 @@ function removeHighlight() { const range = marker.getRange(); if ( range.containsPosition( pos ) || range.start.isEqual( pos ) || range.end.isEqual( pos ) ) { - model.markers.remove( name ); + writer.removeMarker( name ); markerNames.splice( i, 1 ); break; diff --git a/tests/model/document.js b/tests/model/document.js index 31a90b822..ecdc4d430 100644 --- a/tests/model/document.js +++ b/tests/model/document.js @@ -237,8 +237,8 @@ describe( 'Document', () => { it( 'should buffer marker changes in differ', () => { sinon.spy( doc.differ, 'bufferMarkerChange' ); - model.change( () => { - model.markers.set( 'marker', Range.createCollapsedAt( doc.getRoot(), 0 ) ); + model.change( writer => { + writer.setMarker( 'marker', Range.createCollapsedAt( doc.getRoot(), 0 ) ); } ); expect( doc.differ.bufferMarkerChange.called ).to.be.true; diff --git a/tests/model/operation/markeroperation.js b/tests/model/operation/markeroperation.js index 88349d691..f106ba7f5 100644 --- a/tests/model/operation/markeroperation.js +++ b/tests/model/operation/markeroperation.js @@ -86,7 +86,9 @@ describe( 'MarkerOperation', () => { } ); it( 'should not fire document markers set event if newRange is same as current marker range', () => { - model.markers.set( 'name', range ); + model.change( writer => { + writer.setMarker( 'name', range ); + } ); sinon.spy( model.markers, 'fire' ); diff --git a/tests/model/writer.js b/tests/model/writer.js index 1b9650373..098f44b4d 100644 --- a/tests/model/writer.js +++ b/tests/model/writer.js @@ -1965,25 +1965,25 @@ describe( 'Writer', () => { } ); it( 'should add marker to the document marker collection', () => { - setMarker( 'name', range ); + setMarker( 'name', range, { usingOperation: true } ); expect( model.markers.get( 'name' ).getRange().isEqual( range ) ).to.be.true; } ); it( 'should update marker in the document marker collection', () => { - setMarker( 'name', range ); + setMarker( 'name', range, { usingOperation: true } ); const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 ); - setMarker( 'name', range2 ); + setMarker( 'name', range2, { usingOperation: true } ); expect( model.markers.get( 'name' ).getRange().isEqual( range2 ) ).to.be.true; } ); it( 'should accept marker instance', () => { - const marker = model.markers.set( 'name', range ); + const marker = setMarker( 'name', range ); const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 ); - setMarker( marker, range2 ); + setMarker( marker, range2, { usingOperation: true } ); const op = batch.deltas[ 0 ].operations[ 0 ]; expect( model.markers.get( 'name' ).getRange().isEqual( range2 ) ).to.be.true; @@ -1992,12 +1992,12 @@ describe( 'Writer', () => { } ); it( 'should accept empty range parameter if marker instance is passed', () => { - const marker = model.markers.set( 'name', range ); + const marker = setMarker( 'name', range ); const spy = sinon.spy(); model.on( 'applyOperation', spy ); - setMarker( marker ); + setMarker( marker, null, { usingOperation: true } ); const op = batch.deltas[ 0 ].operations[ 0 ]; expect( spy.calledOnce ).to.be.true; @@ -2013,11 +2013,11 @@ describe( 'Writer', () => { } ); it( 'should throw when trying to use detached writer', () => { - const marker = model.markers.set( 'name', range ); + const marker = setMarker( 'name', range ); const writer = new Writer( model, batch ); expect( () => { - writer.setMarker( marker ); + writer.setMarker( marker, null, { usingOperation: true } ); } ).to.throw( CKEditorError, /^writer-incorrect-use/ ); } ); } ); @@ -2033,7 +2033,7 @@ describe( 'Writer', () => { it( 'should remove marker from the document marker collection', () => { setMarker( 'name', range ); - removeMarker( 'name' ); + removeMarker( 'name', { usingOperation: true } ); expect( model.markers.get( 'name' ) ).to.be.null; } ); @@ -2056,7 +2056,7 @@ describe( 'Writer', () => { setMarker( 'name', range ); const marker = model.markers.get( 'name' ); - removeMarker( marker ); + removeMarker( marker, { usingOperation: true } ); expect( model.markers.get( 'name' ) ).to.be.null; } ); @@ -2353,15 +2353,19 @@ describe( 'Writer', () => { } ); } - function setMarker( markerOrName, newRange ) { + function setMarker( markerOrName, newRange, options ) { + let marker; + model.enqueueChange( batch, writer => { - writer.setMarker( markerOrName, newRange ); + marker = writer.setMarker( markerOrName, newRange, options ); } ); + + return marker; } - function removeMarker( markerOrName ) { + function removeMarker( markerOrName, options ) { model.enqueueChange( batch, writer => { - writer.removeMarker( markerOrName ); + writer.removeMarker( markerOrName, options ); } ); } From 8a7a5015da54b3fdf0f174a5489fd6f18075a05f Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 1 Feb 2018 12:23:21 +0100 Subject: [PATCH 02/17] Improved docs, added tests for setMarker. --- src/model/writer.js | 26 ++++++++++++++++++++------ tests/model/writer.js | 41 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 57 insertions(+), 10 deletions(-) diff --git a/src/model/writer.js b/src/model/writer.js index bc9770ced..c8af50cc0 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -774,11 +774,24 @@ export default class Writer { * * @param {module:engine/model/markercollection~Marker|String} markerOrName Marker or marker name to add or update. * @param {module:engine/model/range~Range} [newRange] Marker range. + * @param {Object} [options] + * @param {Boolean} [options.usingOperation=false] Flag indicated whether the marker should be added by MarkerOperation. + * @returns {module:engine/model/markercollection~Marker} Marker that was set. */ setMarker( markerOrName, newRange, options = { usingOperation: false } ) { this._assertWriterUsedCorrectly(); const name = typeof markerOrName == 'string' ? markerOrName : markerOrName.name; + + if ( !options.usingOperation ) { + // Marker set without using operation should always have range. + if ( !newRange ) { + throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' ); + } + + return this.model.markers.set( name, newRange ); + } + const currentMarker = this.model.markers.get( name ); if ( !newRange && !currentMarker ) { @@ -790,11 +803,6 @@ export default class Writer { throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' ); } - if ( !options.usingOperation ) { - const marker = this.model.markers.set( name, newRange ); - return marker; - } - const currentRange = currentMarker ? currentMarker.getRange() : null; if ( !newRange ) { @@ -805,12 +813,16 @@ export default class Writer { // Just change marker range. addMarkerOperation( this, name, currentRange, newRange ); } + + return this.model.markers.get( name ); } /** * Removes given {@link module:engine/model/markercollection~Marker marker} or marker with given name. * * @param {module:engine/model/markercollection~Marker|String} markerOrName Marker or marker name to remove. + * @param {Object} [options] + * @param {Boolean} [options.usingOperation=false] Flag indicated whether the marker should be removed by MarkerOperation. */ removeMarker( markerOrName, options = { usingOperation: false } ) { this._assertWriterUsedCorrectly(); @@ -827,7 +839,9 @@ export default class Writer { } if ( !options.usingOperation ) { - return this.model.markers.remove( name ); + this.model.markers.remove( name ); + + return; } const oldRange = this.model.markers.get( name ).getRange(); diff --git a/tests/model/writer.js b/tests/model/writer.js index 098f44b4d..7129589ba 100644 --- a/tests/model/writer.js +++ b/tests/model/writer.js @@ -1965,16 +1965,28 @@ describe( 'Writer', () => { } ); it( 'should add marker to the document marker collection', () => { - setMarker( 'name', range, { usingOperation: true } ); + setMarker( 'name', range ); expect( model.markers.get( 'name' ).getRange().isEqual( range ) ).to.be.true; } ); + it( 'should return marker if that was set directly', () => { + const marker = setMarker( 'name', range ); + + expect( model.markers.get( 'name' ) ).to.equal( marker ); + } ); + + it( 'should return marker if that was set using operation API', () => { + const marker = setMarker( 'name', range, { usingOperation: true } ); + + expect( model.markers.get( 'name' ) ).to.equal( marker ); + } ); + it( 'should update marker in the document marker collection', () => { - setMarker( 'name', range, { usingOperation: true } ); + setMarker( 'name', range ); const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 ); - setMarker( 'name', range2, { usingOperation: true } ); + setMarker( 'name', range2 ); expect( model.markers.get( 'name' ).getRange().isEqual( range2 ) ).to.be.true; } ); @@ -1991,13 +2003,28 @@ describe( 'Writer', () => { expect( op.newRange.isEqual( range2 ) ).to.be.true; } ); - it( 'should accept empty range parameter if marker instance is passed', () => { + it( 'should accept empty range parameter if marker instance is passed and usingOperation is set to true', () => { const marker = setMarker( 'name', range ); const spy = sinon.spy(); model.on( 'applyOperation', spy ); setMarker( marker, null, { usingOperation: true } ); + + const op = batch.deltas[ 0 ].operations[ 0 ]; + + expect( spy.calledOnce ).to.be.true; + expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' ); + expect( op.oldRange ).to.be.null; + expect( op.newRange.isEqual( range ) ).to.be.true; + } ); + + it( 'should use operations when having set usingOperations to true', () => { + const spy = sinon.spy(); + + model.on( 'applyOperation', spy ); + + setMarker( 'name', range, { usingOperation: true } ); const op = batch.deltas[ 0 ].operations[ 0 ]; expect( spy.calledOnce ).to.be.true; @@ -2007,6 +2034,12 @@ describe( 'Writer', () => { } ); it( 'should throw if marker with given name does not exist and range is not passed', () => { + expect( () => { + setMarker( 'name', null, { usingOperation: true } ); + } ).to.throw( CKEditorError, /^writer-setMarker-no-range/ ); + } ); + + it( 'should throw if marker is set directly and range is not passed', () => { expect( () => { setMarker( 'name' ); } ).to.throw( CKEditorError, /^writer-setMarker-no-range/ ); From da5b452e4172fc9072894299ddcb67fa8dff3205 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 1 Feb 2018 12:39:44 +0100 Subject: [PATCH 03/17] Added missing test for removeMarker. --- tests/model/writer.js | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/model/writer.js b/tests/model/writer.js index 7129589ba..460eb62c3 100644 --- a/tests/model/writer.js +++ b/tests/model/writer.js @@ -2066,7 +2066,7 @@ describe( 'Writer', () => { it( 'should remove marker from the document marker collection', () => { setMarker( 'name', range ); - removeMarker( 'name', { usingOperation: true } ); + removeMarker( 'name' ); expect( model.markers.get( 'name' ) ).to.be.null; } ); @@ -2089,8 +2089,23 @@ describe( 'Writer', () => { setMarker( 'name', range ); const marker = model.markers.get( 'name' ); + removeMarker( marker ); + + expect( model.markers.get( 'name' ) ).to.be.null; + } ); + + it( 'should use MarkerOperation when usingOperation is set to true', () => { + setMarker( 'name', range ); + + const marker = model.markers.get( 'name' ); + const spy = sinon.spy(); + + model.on( 'applyOperation', spy ); + removeMarker( marker, { usingOperation: true } ); + expect( spy.calledOnce ).to.be.true; + expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' ); expect( model.markers.get( 'name' ) ).to.be.null; } ); } ); From d96e19acb0352da29600d6d642f4fbcfdbdc4787 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 1 Feb 2018 12:46:03 +0100 Subject: [PATCH 04/17] Improved docs. --- src/model/writer.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/model/writer.js b/src/model/writer.js index c8af50cc0..facdd71ee 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -762,8 +762,11 @@ export default class Writer { /** * Adds or updates {@link module:engine/model/markercollection~Marker marker} with given name to given `range`. * + * It uses {@link module:engine/model/operation/markeroperation~MarkerOperation} when `options.usingOperation` is set to true. + * Otherwise adds directly to the {@link module:engine/model/MarkerCollection~MarkerCollection}. + * * If passed name is a name of already existing marker (or {@link module:engine/model/markercollection~Marker Marker} instance - * is passed), `range` parameter may be omitted. In this case marker will not be updated in + * is passed), `range` parameter may be omitted (only for setting markers using operation). In this case marker will not be updated in * {@link module:engine/model/model~Model#markers document marker collection}. However the marker will be added to * the document history. This may be important for other features, like undo. From document history point of view, it will * look like the marker was created and added to the document at the moment when it is set using this method. @@ -784,7 +787,6 @@ export default class Writer { const name = typeof markerOrName == 'string' ? markerOrName : markerOrName.name; if ( !options.usingOperation ) { - // Marker set without using operation should always have range. if ( !newRange ) { throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' ); } @@ -820,6 +822,9 @@ export default class Writer { /** * Removes given {@link module:engine/model/markercollection~Marker marker} or marker with given name. * + * It uses {@link module:engine/model/operation/markeroperation~MarkerOperation} when `options.usingOperation` is set to true. + * Otherwise removes directly from the {@link module:engine/model/MarkerCollection~MarkerCollection}. + * * @param {module:engine/model/markercollection~Marker|String} markerOrName Marker or marker name to remove. * @param {Object} [options] * @param {Boolean} [options.usingOperation=false] Flag indicated whether the marker should be removed by MarkerOperation. From c2901e461874434f6a1faccc15f8a280a51b9b96 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 1 Feb 2018 17:29:27 +0100 Subject: [PATCH 05/17] Improved docs. --- src/model/markercollection.js | 50 +++++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/src/model/markercollection.js b/src/model/markercollection.js index f07cd526a..612dbd9db 100644 --- a/src/model/markercollection.js +++ b/src/model/markercollection.js @@ -15,13 +15,34 @@ import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import mix from '@ckeditor/ckeditor5-utils/src/mix'; /** - * Creates, stores and manages {@link ~Marker markers}. + * Creates, stores and manages {@link module:engine/model/markercollection~Marker markers}. * - * Markers are created by {@link ~MarkerCollection#set setting} a name for a {@link module:engine/model/liverange~LiveRange live range} - * in `MarkerCollection`. Name is used to group and identify markers. Names have to be unique, but markers can be grouped by - * using common prefixes, separated with `:`, for example: `user:john` or `search:3`. + * Markers are designed to be a ready-to-use solution for having up-to-date ranges on the document with an easy access to them. + * They're built from a name and a {@link module:engine/model/liverange~LiveRange live range}. + * Name is used to group and identify markers. Names have to be unique, but markers can be grouped by + * using common prefixes, separated with `:`, for example: `user:john` or `search:3`. That's useful in term of creating namespaces + * for custom elements (e.g. comments, highlights). * - * Since markers are based on {@link module:engine/model/liverange~LiveRange live ranges}, for efficiency reasons, it's + * Markers can be watched for changes - both adding and removing markers from the `MarkerCollection` fires events + * (`~MarkerCollection#event:add` and `MarkerCollection#event:remove`). + * + * There're two types of markers. Both type of them should be added /updated by {@link module:engine/model/writer~Writer#setMarker} + * and removed by {@link module:engine/model/writer~Writer#removeMarker} methods. + * + * There're two types of markers. Both type of them should be added /updated by {@link module:engine/model/writer~Writer#setMarker} + * and removed by {@link module:engine/model/writer~Writer#removeMarker} methods. Both of them are stored in the {@link ~MarkerCollection}. + * + * 1. Markers added to the document. They're synchronized in the collaboration and handled in the undo. + * This type of markers is useful for solutions like spell checking or comments. + * Usage: + * writer.setMarker( markerOrName, ranges, { usingOperation: true } ); + * + * 2. Markers not added to document. They can be used as bookmarks or visual markers. They're great for showing results of the find, + * or select link when the focus is in the input see https://github.com/ckeditor/ckeditor5-link/issues/13. + * Usage: + * writer.setMarker( markerOrName, ranges ); + * + * Note: Since markers are based on {@link module:engine/model/liverange~LiveRange live ranges}, for efficiency reasons, it's * best to create and keep at least markers as possible. */ export default class MarkerCollection { @@ -219,6 +240,25 @@ mix( MarkerCollection, EmitterMixin ); * "special" and the marker is shrunk. Similarly, when a character is moved into the marker from other place in document * model, it starts being "special" and the marker is enlarged. * + * Markers are designed to be a ready-to-use solution for having up-to-date ranges on the document with an easy access to them. + * They're built from a name and a {@link module:engine/model/liverange~LiveRange live range}. + * Name is used to group and identify markers. Names have to be unique, but markers can be grouped by + * using common prefixes, separated with `:`, for example: `user:john` or `search:3`. That's useful in term of creating namespaces + * for custom elements (e.g. comments, highlights). + * + * There're two types of markers. Both type of them should be added /updated by {@link module:engine/model/writer~Writer#setMarker} + * and removed by {@link module:engine/model/writer~Writer#removeMarker} methods. Both of them are stored in the {@link ~MarkerCollection}. + * + * 1. Markers added to the document. They're synchronized in the collaboration and handled in the undo. + * This type of markers is useful for solutions like spell checking or comments. + * Usage: + * writer.setMarker( markerOrName, ranges, { usingOperation: true } ); + * + * 2. Markers not added to document. They can be used as bookmarks or visual markers. They're great for showing results of the find, + * or select link when the focus is in the input see https://github.com/ckeditor/ckeditor5-link/issues/13. + * Usage: + * writer.setMarker( markerOrName, ranges ); + * * Since markers are based on {@link module:engine/model/liverange~LiveRange live ranges}, for efficiency reasons, it's * best to create and keep at least markers as possible. * From c3720bc440d9ce0138db9d13a83670ff43a90b36 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 1 Feb 2018 18:02:06 +0100 Subject: [PATCH 06/17] Marked MarkerCollection's `set` and `remove` as protected. --- src/model/markercollection.js | 27 +++++++--- src/model/operation/markeroperation.js | 4 +- src/model/writer.js | 13 ++--- tests/model/markercollection.js | 64 ++++++++++++------------ tests/model/operation/markeroperation.js | 14 +++--- tests/model/writer.js | 18 +++++-- 6 files changed, 82 insertions(+), 58 deletions(-) diff --git a/src/model/markercollection.js b/src/model/markercollection.js index 612dbd9db..571263fbb 100644 --- a/src/model/markercollection.js +++ b/src/model/markercollection.js @@ -99,28 +99,30 @@ export default class MarkerCollection { * set is different, the marker in collection is removed and then new marker is added. If the range was same, nothing * happens and `false` is returned. * + * @protected * @fires module:engine/model/markercollection~MarkerCollection#event:add * @fires module:engine/model/markercollection~MarkerCollection#event:remove * @param {String|module:engine/model/markercollection~Marker} markerOrName Name of marker to add or Marker instance to update. * @param {module:engine/model/range~Range} range Marker range. + * @param {Boolean} managedUsingOperations Specifies whether the marker is managed using operations. * @returns {module:engine/model/markercollection~Marker} `Marker` instance added to the collection. */ - set( markerOrName, range ) { + _set( markerOrName, range, managedUsingOperations ) { const markerName = markerOrName instanceof Marker ? markerOrName.name : markerOrName; const oldMarker = this._markers.get( markerName ); if ( oldMarker ) { const oldRange = oldMarker.getRange(); - if ( oldRange.isEqual( range ) ) { + if ( oldRange.isEqual( range ) && managedUsingOperations === oldMarker.managedUsingOperations ) { return oldMarker; } - this.remove( markerName ); + this._remove( markerName ); } const liveRange = LiveRange.createFromRange( range ); - const marker = new Marker( markerName, liveRange ); + const marker = new Marker( markerName, liveRange, managedUsingOperations ); this._markers.set( markerName, marker ); this.fire( 'add:' + markerName, marker ); @@ -131,10 +133,11 @@ export default class MarkerCollection { /** * Removes given {@link ~Marker marker} or a marker with given name from the `MarkerCollection`. * + * @protected * @param {String} markerOrName Marker or name of a marker to remove. * @returns {Boolean} `true` if marker was found and removed, `false` otherwise. */ - remove( markerOrName ) { + _remove( markerOrName ) { const markerName = markerOrName instanceof Marker ? markerOrName.name : markerOrName; const oldMarker = this._markers.get( markerName ); @@ -281,20 +284,28 @@ class Marker { * @param {String} name Marker name. * @param {module:engine/model/liverange~LiveRange} liveRange Range marked by the marker. */ - constructor( name, liveRange ) { + constructor( name, liveRange, managedUsingOperations ) { /** * Marker's name. * * @readonly - * @member {String} #name + * @type {String} */ this.name = name; + /** + * Flag indicates if the marker should be managed using operations. + * + * @readonly + * @type {Boolean} + */ + this.managedUsingOperations = managedUsingOperations; + /** * Range marked by the marker. * * @protected - * @member {module:engine/model/liverange~LiveRange} #_liveRange + * @type {module:engine/model/liverange~LiveRange} */ this._liveRange = liveRange; diff --git a/src/model/operation/markeroperation.js b/src/model/operation/markeroperation.js index 381e866f5..5bc3bccad 100644 --- a/src/model/operation/markeroperation.js +++ b/src/model/operation/markeroperation.js @@ -87,9 +87,9 @@ export default class MarkerOperation extends Operation { * @inheritDoc */ _execute() { - const type = this.newRange ? 'set' : 'remove'; + const type = this.newRange ? '_set' : '_remove'; - this._markers[ type ]( this.name, this.newRange ); + this._markers[ type ]( this.name, this.newRange, true ); } /** diff --git a/src/model/writer.js b/src/model/writer.js index facdd71ee..286310bad 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -791,7 +791,7 @@ export default class Writer { throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' ); } - return this.model.markers.set( name, newRange ); + return this.model.markers._set( name, newRange, options.usingOperation ); } const currentMarker = this.model.markers.get( name ); @@ -827,9 +827,8 @@ export default class Writer { * * @param {module:engine/model/markercollection~Marker|String} markerOrName Marker or marker name to remove. * @param {Object} [options] - * @param {Boolean} [options.usingOperation=false] Flag indicated whether the marker should be removed by MarkerOperation. */ - removeMarker( markerOrName, options = { usingOperation: false } ) { + removeMarker( markerOrName ) { this._assertWriterUsedCorrectly(); const name = typeof markerOrName == 'string' ? markerOrName : markerOrName.name; @@ -843,13 +842,15 @@ export default class Writer { throw new CKEditorError( 'writer-removeMarker-no-marker: Trying to remove marker which does not exist.' ); } - if ( !options.usingOperation ) { - this.model.markers.remove( name ); + const marker = this.model.markers.get( name ); + + if ( !marker.managedUsingOperations ) { + this.model.markers._remove( name ); return; } - const oldRange = this.model.markers.get( name ).getRange(); + const oldRange = marker.getRange(); addMarkerOperation( this, name, oldRange, null ); } diff --git a/tests/model/markercollection.js b/tests/model/markercollection.js index 802776ced..54225af2b 100644 --- a/tests/model/markercollection.js +++ b/tests/model/markercollection.js @@ -26,8 +26,8 @@ describe( 'MarkerCollection', () => { describe( 'iterator', () => { it( 'should return markers added to the marker collection', () => { - markers.set( 'a', range ); - markers.set( 'b', range ); + markers._set( 'a', range ); + markers._set( 'b', range ); const markerA = markers.get( 'a' ); const markerB = markers.get( 'b' ); @@ -40,11 +40,11 @@ describe( 'MarkerCollection', () => { } ); } ); - describe( 'set', () => { + describe( '_set', () => { it( 'should create a marker, fire add: event and return true', () => { sinon.spy( markers, 'fire' ); - const result = markers.set( 'name', range ); + const result = markers._set( 'name', range ); const marker = markers.get( 'name' ); expect( result ).to.equal( marker ); @@ -54,11 +54,11 @@ describe( 'MarkerCollection', () => { } ); it( 'should fire remove: event, and create a new marker if marker with given name was in the collection', () => { - const marker1 = markers.set( 'name', range ); + const marker1 = markers._set( 'name', range ); sinon.spy( markers, 'fire' ); - const marker2 = markers.set( 'name', range2 ); + const marker2 = markers._set( 'name', range2 ); expect( markers.fire.calledWithExactly( 'remove:name', marker1 ) ).to.be.true; expect( markers.fire.calledWithExactly( 'add:name', marker2 ) ).to.be.true; @@ -70,21 +70,21 @@ describe( 'MarkerCollection', () => { } ); it( 'should not fire event and return the same marker if given marker has a range equal to given range', () => { - const marker1 = markers.set( 'name', range ); + const marker1 = markers._set( 'name', range ); sinon.spy( markers, 'fire' ); - const marker2 = markers.set( 'name', range ); + const marker2 = markers._set( 'name', range ); expect( marker1 ).to.equal( marker2 ); expect( markers.fire.notCalled ).to.be.true; } ); it( 'should accept marker instance instead of name', () => { - markers.set( 'name', range ); + markers._set( 'name', range ); const marker1 = markers.get( 'name' ); - const result = markers.set( marker1, range2 ); + const result = markers._set( marker1, range2 ); const marker2 = markers.get( 'name' ); expect( result ).to.equal( marker2 ); @@ -99,7 +99,7 @@ describe( 'MarkerCollection', () => { } ); it( 'should return true if marker with given name is in the collection', () => { - markers.set( 'name', range ); + markers._set( 'name', range ); expect( markers.has( 'name' ) ).to.be.true; } ); } ); @@ -114,13 +114,13 @@ describe( 'MarkerCollection', () => { } ); } ); - describe( 'remove', () => { + describe( '_remove', () => { it( 'should remove marker, return true and fire remove: event', () => { - const marker = markers.set( 'name', range ); + const marker = markers._set( 'name', range ); sinon.spy( markers, 'fire' ); - const result = markers.remove( 'name' ); + const result = markers._remove( 'name' ); expect( result ).to.be.true; expect( markers.fire.calledWithExactly( 'remove:name', marker ) ).to.be.true; @@ -128,13 +128,13 @@ describe( 'MarkerCollection', () => { } ); it( 'should destroy marker instance', () => { - const marker = markers.set( 'name', range ); + const marker = markers._set( 'name', range ); const liveRange = marker._liveRange; sinon.spy( marker, 'stopListening' ); sinon.spy( liveRange, 'detach' ); - markers.remove( 'name' ); + markers._remove( 'name' ); expect( marker.stopListening.calledOnce ).to.be.true; expect( marker._liveRange ).to.be.null; @@ -142,22 +142,22 @@ describe( 'MarkerCollection', () => { } ); it( 'should return false if name has not been found in collection', () => { - markers.set( 'name', range ); + markers._set( 'name', range ); sinon.spy( markers, 'fire' ); - const result = markers.remove( 'other' ); + const result = markers._remove( 'other' ); expect( result ).to.be.false; expect( markers.fire.notCalled ).to.be.true; } ); it( 'should accept marker instance instead of name', () => { - const marker = markers.set( 'name', range ); + const marker = markers._set( 'name', range ); sinon.spy( markers, 'fire' ); - const result = markers.remove( marker ); + const result = markers._remove( marker ); expect( result ).to.be.true; expect( markers.fire.calledWithExactly( 'remove:name', marker ) ).to.be.true; @@ -167,10 +167,10 @@ describe( 'MarkerCollection', () => { describe( 'getMarkersGroup', () => { it( 'returns all markers which names start on given prefix', () => { - const markerFooA = markers.set( 'foo:a', range ); - const markerFooB = markers.set( 'foo:b', range ); - markers.set( 'bar:a', range ); - markers.set( 'foobar:a', range ); + const markerFooA = markers._set( 'foo:a', range ); + const markerFooB = markers._set( 'foo:b', range ); + markers._set( 'bar:a', range ); + markers._set( 'foobar:a', range ); expect( Array.from( markers.getMarkersGroup( 'foo' ) ) ).to.deep.equal( [ markerFooA, markerFooB ] ); expect( Array.from( markers.getMarkersGroup( 'a' ) ) ).to.deep.equal( [] ); @@ -179,8 +179,8 @@ describe( 'MarkerCollection', () => { describe( 'getMarkersAtPosition', () => { it( 'should return iterator iterating over all markers that contains given position', () => { - markers.set( 'a', range ); - const markerB = markers.set( 'b', range2 ); + markers._set( 'a', range ); + const markerB = markers._set( 'b', range2 ); const result = Array.from( markers.getMarkersAtPosition( Position.createAt( root, 1 ) ) ); @@ -190,8 +190,8 @@ describe( 'MarkerCollection', () => { describe( 'destroy', () => { it( 'should make MarkerCollection stop listening to all events and destroy all markers', () => { - const markerA = markers.set( 'a', range ); - const markerB = markers.set( 'b', range2 ); + const markerA = markers._set( 'a', range ); + const markerB = markers._set( 'b', range2 ); sinon.spy( markers, 'stopListening' ); sinon.spy( markerA, 'stopListening' ); @@ -221,7 +221,7 @@ describe( 'Marker', () => { root.appendChildren( new Text( 'foo' ) ); const range = Range.createFromParentsAndOffsets( root, 1, root, 2 ); - const marker = model.markers.set( 'name', range ); + const marker = model.markers._set( 'name', range ); expect( marker.getRange().isEqual( range ) ).to.be.true; expect( marker.getStart().isEqual( range.start ) ).to.be.true; @@ -240,9 +240,9 @@ describe( 'Marker', () => { it( 'should throw when using the API if marker was removed from markers collection', () => { const range = Range.createFromParentsAndOffsets( root, 1, root, 2 ); - const marker = model.markers.set( 'name', range ); + const marker = model.markers._set( 'name', range ); - model.markers.remove( 'name' ); + model.markers._remove( 'name' ); expect( () => { marker.getRange(); @@ -259,7 +259,7 @@ describe( 'Marker', () => { it( 'should delegate events from live range', () => { const range = Range.createFromParentsAndOffsets( root, 1, root, 2 ); - const marker = model.markers.set( 'name', range ); + const marker = model.markers._set( 'name', range ); const eventRange = sinon.spy(); const eventContent = sinon.spy(); diff --git a/tests/model/operation/markeroperation.js b/tests/model/operation/markeroperation.js index f106ba7f5..b14b55b69 100644 --- a/tests/model/operation/markeroperation.js +++ b/tests/model/operation/markeroperation.js @@ -30,14 +30,14 @@ describe( 'MarkerOperation', () => { } ); it( 'should add marker to document marker collection', () => { - sinon.spy( model.markers, 'set' ); + sinon.spy( model.markers, '_set' ); model.applyOperation( wrapInDelta( new MarkerOperation( 'name', null, range, model.markers, doc.version ) ) ); expect( doc.version ).to.equal( 1 ); - expect( model.markers.set.calledWith( 'name', matchRange( range ) ) ); + expect( model.markers._set.calledWith( 'name', matchRange( range ) ) ); expect( model.markers.get( 'name' ).getRange().isEqual( range ) ).to.be.true; } ); @@ -48,14 +48,14 @@ describe( 'MarkerOperation', () => { const range2 = Range.createFromParentsAndOffsets( root, 0, root, 3 ); - sinon.spy( model.markers, 'set' ); + sinon.spy( model.markers, '_set' ); model.applyOperation( wrapInDelta( new MarkerOperation( 'name', range, range2, model.markers, doc.version ) ) ); expect( doc.version ).to.equal( 2 ); - expect( model.markers.set.calledWith( 'name', matchRange( range2 ) ) ); + expect( model.markers._set.calledWith( 'name', matchRange( range2 ) ) ); expect( model.markers.get( 'name' ).getRange().isEqual( range2 ) ).to.be.true; } ); @@ -64,14 +64,14 @@ describe( 'MarkerOperation', () => { new MarkerOperation( 'name', null, range, model.markers, doc.version ) ) ); - sinon.spy( model.markers, 'remove' ); + sinon.spy( model.markers, '_remove' ); model.applyOperation( wrapInDelta( new MarkerOperation( 'name', range, null, model.markers, doc.version ) ) ); expect( doc.version ).to.equal( 2 ); - expect( model.markers.remove.calledWith( 'name' ) ); + expect( model.markers._remove.calledWith( 'name' ) ); expect( model.markers.get( 'name' ) ).to.be.null; } ); @@ -87,7 +87,7 @@ describe( 'MarkerOperation', () => { it( 'should not fire document markers set event if newRange is same as current marker range', () => { model.change( writer => { - writer.setMarker( 'name', range ); + writer.setMarker( 'name', range, { usingOperation: true } ); } ); sinon.spy( model.markers, 'fire' ); diff --git a/tests/model/writer.js b/tests/model/writer.js index 460eb62c3..5140e0566 100644 --- a/tests/model/writer.js +++ b/tests/model/writer.js @@ -1982,6 +1982,18 @@ describe( 'Writer', () => { expect( model.markers.get( 'name' ) ).to.equal( marker ); } ); + it( 'should return marker with properly set managedUsingOperations (to true)', () => { + const marker = setMarker( 'name', range, { usingOperation: true } ); + + expect( marker.managedUsingOperations ).to.equal( true ); + } ); + + it( 'should return marker with properly set managedUsingOperations (to false)', () => { + const marker = setMarker( 'name', range, { usingOperation: false } ); + + expect( marker.managedUsingOperations ).to.equal( false ); + } ); + it( 'should update marker in the document marker collection', () => { setMarker( 'name', range ); @@ -2094,15 +2106,15 @@ describe( 'Writer', () => { expect( model.markers.get( 'name' ) ).to.be.null; } ); - it( 'should use MarkerOperation when usingOperation is set to true', () => { - setMarker( 'name', range ); + it( 'should use MarkerOperation when marker was created using operation', () => { + setMarker( 'name', range, { usingOperation: true } ); const marker = model.markers.get( 'name' ); const spy = sinon.spy(); model.on( 'applyOperation', spy ); - removeMarker( marker, { usingOperation: true } ); + removeMarker( marker ); expect( spy.calledOnce ).to.be.true; expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' ); From d4264beb6b957006288edde88d2f1d3f44f0c553 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 1 Feb 2018 18:34:18 +0100 Subject: [PATCH 07/17] Fixed test. --- tests/model/writer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/model/writer.js b/tests/model/writer.js index 5140e0566..8ffd9c35e 100644 --- a/tests/model/writer.js +++ b/tests/model/writer.js @@ -1989,7 +1989,7 @@ describe( 'Writer', () => { } ); it( 'should return marker with properly set managedUsingOperations (to false)', () => { - const marker = setMarker( 'name', range, { usingOperation: false } ); + const marker = setMarker( 'name', range ); expect( marker.managedUsingOperations ).to.equal( false ); } ); From 0a777bac32251225238091afe8859771242ed41d Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 2 Feb 2018 11:48:22 +0100 Subject: [PATCH 08/17] Enhanced setMarker signatures. --- src/model/writer.js | 60 +++++++++++++++++++++++++++++++++++-------- tests/model/writer.js | 22 +++++++++++++--- 2 files changed, 68 insertions(+), 14 deletions(-) diff --git a/src/model/writer.js b/src/model/writer.js index 286310bad..c39e1e3bd 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -40,6 +40,7 @@ import DocumentSelection from './documentselection'; import toMap from '@ckeditor/ckeditor5-utils/src/tomap'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; +import uid from '@ckeditor/ckeditor5-utils/src/uid'; /** * Model writer it the proper way of modifying model. It should be used whenever you wants to create node, modify @@ -775,26 +776,63 @@ export default class Writer { * is waiting for additional data, etc.). In this case, the marker may be first created directly through * {@link module:engine/model/markercollection~MarkerCollection MarkerCollection API} and only later added using `Batch` API. * - * @param {module:engine/model/markercollection~Marker|String} markerOrName Marker or marker name to add or update. - * @param {module:engine/model/range~Range} [newRange] Marker range. + * Create / update marker using `MarkerOperation` operation: + * + * setMarker( marker, range, { usingOperations: true } ); + * + * Create / update marker directly base on marker's name: + * + * setMarker( markerName, range ); + * + * Create marker with a unique id using `MarkerOperation` operation: + * + * setMarker( range, { usingOperations: true } ); + * + * Create marker directly with a unique id: + * + * setMarker( range ) + * + * Update marker using `MarkerOperation` operation. + * + * setMarker( marker, { usingOperations: true } ); + * + * @param {module:engine/model/markercollection~Marker|String|module:engine/model/range~Range} markerOrNameOrRange + * Name of marker to add, Marker instance to update or range for the marker with a unique name. + * @param {module:engine/model/range~Range|Object} [rangeOrOptions] Marker range or options. * @param {Object} [options] * @param {Boolean} [options.usingOperation=false] Flag indicated whether the marker should be added by MarkerOperation. * @returns {module:engine/model/markercollection~Marker} Marker that was set. */ - setMarker( markerOrName, newRange, options = { usingOperation: false } ) { + setMarker( markerOrNameOrRange, rangeOrManagedUsingOperations, options ) { this._assertWriterUsedCorrectly(); - const name = typeof markerOrName == 'string' ? markerOrName : markerOrName.name; + let markerName, newRange, usingOperation; - if ( !options.usingOperation ) { + if ( markerOrNameOrRange instanceof Range ) { + markerName = uid(); + newRange = markerOrNameOrRange; + usingOperation = !!rangeOrManagedUsingOperations && !!rangeOrManagedUsingOperations.usingOperation; + } else { + markerName = typeof markerOrNameOrRange === 'string' ? markerOrNameOrRange : markerOrNameOrRange.name; + + if ( rangeOrManagedUsingOperations instanceof Range ) { + newRange = rangeOrManagedUsingOperations; + usingOperation = !!options && !!options.usingOperation; + } else { + newRange = null; + usingOperation = !!rangeOrManagedUsingOperations && !!rangeOrManagedUsingOperations.usingOperation; + } + } + + if ( !usingOperation ) { if ( !newRange ) { throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' ); } - return this.model.markers._set( name, newRange, options.usingOperation ); + return this.model.markers._set( markerName, newRange, usingOperation ); } - const currentMarker = this.model.markers.get( name ); + const currentMarker = this.model.markers.get( markerName ); if ( !newRange && !currentMarker ) { /** @@ -810,19 +848,19 @@ export default class Writer { if ( !newRange ) { // If `newRange` is not given, treat this as synchronizing existing marker. // Create `MarkerOperation` with `oldRange` set to `null`, so reverse operation will remove the marker. - addMarkerOperation( this, name, null, currentRange ); + addMarkerOperation( this, markerName, null, currentRange ); } else { // Just change marker range. - addMarkerOperation( this, name, currentRange, newRange ); + addMarkerOperation( this, markerName, currentRange, newRange ); } - return this.model.markers.get( name ); + return this.model.markers.get( markerName ); } /** * Removes given {@link module:engine/model/markercollection~Marker marker} or marker with given name. * - * It uses {@link module:engine/model/operation/markeroperation~MarkerOperation} when `options.usingOperation` is set to true. + * It uses {@link module:engine/model/operation/markeroperation~MarkerOperation} when marker's `managedUsingOperation` is set to true. * Otherwise removes directly from the {@link module:engine/model/MarkerCollection~MarkerCollection}. * * @param {module:engine/model/markercollection~Marker|String} markerOrName Marker or marker name to remove. diff --git a/tests/model/writer.js b/tests/model/writer.js index 8ffd9c35e..bc421066c 100644 --- a/tests/model/writer.js +++ b/tests/model/writer.js @@ -2021,7 +2021,7 @@ describe( 'Writer', () => { model.on( 'applyOperation', spy ); - setMarker( marker, null, { usingOperation: true } ); + setMarker( marker, { usingOperation: true } ); const op = batch.deltas[ 0 ].operations[ 0 ]; @@ -2031,6 +2031,22 @@ describe( 'Writer', () => { expect( op.newRange.isEqual( range ) ).to.be.true; } ); + it( 'should create a unique id if the first param is type of range', () => { + const marker = setMarker( range ); + + expect( marker.name ).to.be.a( 'string' ); + } ); + + it( 'should create a unique id if the first param is type of range when usingOperations is set to true', () => { + const spy = sinon.spy(); + model.on( 'applyOperation', spy ); + + const marker = setMarker( range, { usingOperation: true } ); + + expect( marker.name ).to.be.a( 'string' ); + expect( spy.calledOnce ).to.be.true; + } ); + it( 'should use operations when having set usingOperations to true', () => { const spy = sinon.spy(); @@ -2413,11 +2429,11 @@ describe( 'Writer', () => { } ); } - function setMarker( markerOrName, newRange, options ) { + function setMarker( markerOrNameOrRange, rangeOrManagedUsingOperations, options ) { let marker; model.enqueueChange( batch, writer => { - marker = writer.setMarker( markerOrName, newRange, options ); + marker = writer.setMarker( markerOrNameOrRange, rangeOrManagedUsingOperations, options ); } ); return marker; From 6ab7c4bd0e3a95d95658d97691c0e6c12d81ca32 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 2 Feb 2018 12:39:29 +0100 Subject: [PATCH 09/17] Fixed test and improved CC. --- src/model/writer.js | 13 +++++++------ tests/model/writer.js | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/model/writer.js b/src/model/writer.js index c39e1e3bd..a7791f46c 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -764,7 +764,8 @@ export default class Writer { * Adds or updates {@link module:engine/model/markercollection~Marker marker} with given name to given `range`. * * It uses {@link module:engine/model/operation/markeroperation~MarkerOperation} when `options.usingOperation` is set to true. - * Otherwise adds directly to the {@link module:engine/model/MarkerCollection~MarkerCollection}. + * Otherwise adds directly to the {@link module:engine/model/MarkerCollection~MarkerCollection}. For additional information about + * the difference between those two types see {@link module:/engine/model/markercollection~MarkerCollection}. * * If passed name is a name of already existing marker (or {@link module:engine/model/markercollection~Marker Marker} instance * is passed), `range` parameter may be omitted (only for setting markers using operation). In this case marker will not be updated in @@ -826,6 +827,11 @@ export default class Writer { if ( !usingOperation ) { if ( !newRange ) { + /** + * Range parameter is required when adding a new marker. + * + * @error writer-setMarker-no-range + */ throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' ); } @@ -835,11 +841,6 @@ export default class Writer { const currentMarker = this.model.markers.get( markerName ); if ( !newRange && !currentMarker ) { - /** - * Range parameter is required when adding a new marker. - * - * @error writer-setMarker-no-range - */ throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' ); } diff --git a/tests/model/writer.js b/tests/model/writer.js index bc421066c..af127e8f9 100644 --- a/tests/model/writer.js +++ b/tests/model/writer.js @@ -2063,7 +2063,7 @@ describe( 'Writer', () => { it( 'should throw if marker with given name does not exist and range is not passed', () => { expect( () => { - setMarker( 'name', null, { usingOperation: true } ); + setMarker( 'name', { usingOperation: true } ); } ).to.throw( CKEditorError, /^writer-setMarker-no-range/ ); } ); From 5c5e40d885675b1227b4a3b1169729f32ec6c9ce Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 2 Feb 2018 16:41:56 +0100 Subject: [PATCH 10/17] Docs improvements. --- src/model/markercollection.js | 17 ++++++----------- src/model/writer.js | 23 ++++++++++++++++------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/model/markercollection.js b/src/model/markercollection.js index 571263fbb..01ca0e1f8 100644 --- a/src/model/markercollection.js +++ b/src/model/markercollection.js @@ -26,24 +26,19 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix'; * Markers can be watched for changes - both adding and removing markers from the `MarkerCollection` fires events * (`~MarkerCollection#event:add` and `MarkerCollection#event:remove`). * - * There're two types of markers. Both type of them should be added /updated by {@link module:engine/model/writer~Writer#setMarker} - * and removed by {@link module:engine/model/writer~Writer#removeMarker} methods. - * - * There're two types of markers. Both type of them should be added /updated by {@link module:engine/model/writer~Writer#setMarker} + * There're two types of markers. Both type of them should be added / updated by {@link module:engine/model/writer~Writer#setMarker} * and removed by {@link module:engine/model/writer~Writer#removeMarker} methods. Both of them are stored in the {@link ~MarkerCollection}. * * 1. Markers added to the document. They're synchronized in the collaboration and handled in the undo. - * This type of markers is useful for solutions like spell checking or comments. - * Usage: + * This type of markers is useful for solutions like spell checking or comments. Sample usage: * writer.setMarker( markerOrName, ranges, { usingOperation: true } ); * - * 2. Markers not added to document. They can be used as bookmarks or visual markers. They're great for showing results of the find, - * or select link when the focus is in the input see https://github.com/ckeditor/ckeditor5-link/issues/13. - * Usage: + * 2. Markers not added to document (default ones). They can be used as bookmarks or visual markers. + * They're great for showing results of the find, or select link when the focus is in the input, + * see https://github.com/ckeditor/ckeditor5-link/issues/13. Sample usage: * writer.setMarker( markerOrName, ranges ); * - * Note: Since markers are based on {@link module:engine/model/liverange~LiveRange live ranges}, for efficiency reasons, it's - * best to create and keep at least markers as possible. + * Note: For efficiency reasons, it's best to create and keep at least markers as possible. */ export default class MarkerCollection { /** diff --git a/src/model/writer.js b/src/model/writer.js index a7791f46c..ec7d65d54 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -762,10 +762,18 @@ export default class Writer { /** * Adds or updates {@link module:engine/model/markercollection~Marker marker} with given name to given `range`. + * If the marker, nor marker's name is not provided, new marker is created and returned with a unique name. * - * It uses {@link module:engine/model/operation/markeroperation~MarkerOperation} when `options.usingOperation` is set to true. - * Otherwise adds directly to the {@link module:engine/model/MarkerCollection~MarkerCollection}. For additional information about - * the difference between those two types see {@link module:/engine/model/markercollection~MarkerCollection}. + * There're two types of markers. + * + * 1. Markers added to the document. They're synchronized in the collaboration and handled in the undo. + * This type of markers is useful for solutions like spell checking or comments. Sample usage: + * writer.setMarker( markerOrName, ranges, { usingOperation: true } ); + * + * 2. Markers not added to document (default ones). They can be used as bookmarks or visual markers. + * They're great for showing results of the find, or select link when the focus is in the input, + * see https://github.com/ckeditor/ckeditor5-link/issues/13. Sample usage: + * writer.setMarker( markerOrName, ranges ); * * If passed name is a name of already existing marker (or {@link module:engine/model/markercollection~Marker Marker} instance * is passed), `range` parameter may be omitted (only for setting markers using operation). In this case marker will not be updated in @@ -789,7 +797,7 @@ export default class Writer { * * setMarker( range, { usingOperations: true } ); * - * Create marker directly with a unique id: + * Create marker directly with a unique name: * * setMarker( range ) * @@ -797,6 +805,8 @@ export default class Writer { * * setMarker( marker, { usingOperations: true } ); * + * Note: For efficiency reasons, it's best to create and keep at least markers as possible. + * * @param {module:engine/model/markercollection~Marker|String|module:engine/model/range~Range} markerOrNameOrRange * Name of marker to add, Marker instance to update or range for the marker with a unique name. * @param {module:engine/model/range~Range|Object} [rangeOrOptions] Marker range or options. @@ -860,9 +870,8 @@ export default class Writer { /** * Removes given {@link module:engine/model/markercollection~Marker marker} or marker with given name. - * - * It uses {@link module:engine/model/operation/markeroperation~MarkerOperation} when marker's `managedUsingOperation` is set to true. - * Otherwise removes directly from the {@link module:engine/model/MarkerCollection~MarkerCollection}. + * There's no `usingOperation` option available here, the marker destructing mechanism is hidden and used + * accordingly to how the marker was created, so if the marker was created with operation, it will be destroyed using operation. * * @param {module:engine/model/markercollection~Marker|String} markerOrName Marker or marker name to remove. * @param {Object} [options] From c6fc13025c3e8f8d34a1c07e2c2789cdd8af5978 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 2 Feb 2018 16:59:44 +0100 Subject: [PATCH 11/17] Docs improvements. --- src/model/markercollection.js | 25 +++++++++++++------------ src/model/writer.js | 11 ++++++----- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/src/model/markercollection.js b/src/model/markercollection.js index 01ca0e1f8..3b81209de 100644 --- a/src/model/markercollection.js +++ b/src/model/markercollection.js @@ -29,16 +29,18 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix'; * There're two types of markers. Both type of them should be added / updated by {@link module:engine/model/writer~Writer#setMarker} * and removed by {@link module:engine/model/writer~Writer#removeMarker} methods. Both of them are stored in the {@link ~MarkerCollection}. * - * 1. Markers added to the document. They're synchronized in the collaboration and handled in the undo. - * This type of markers is useful for solutions like spell checking or comments. Sample usage: - * writer.setMarker( markerOrName, ranges, { usingOperation: true } ); - * - * 2. Markers not added to document (default ones). They can be used as bookmarks or visual markers. + * 1. Markers not added to document (default ones). They can be used as bookmarks or visual markers. * They're great for showing results of the find, or select link when the focus is in the input, * see https://github.com/ckeditor/ckeditor5-link/issues/13. Sample usage: * writer.setMarker( markerOrName, ranges ); * + * 1. Markers added to the document. They're synchronized in the collaboration and handled in the undo. + * This type of markers is useful for solutions like spell checking or comments. Sample usage: + * writer.setMarker( markerOrName, ranges, { usingOperation: true } ); + * * Note: For efficiency reasons, it's best to create and keep at least markers as possible. + * + * @see {module:engine/model/liverange~LiveRange} */ export default class MarkerCollection { /** @@ -247,16 +249,15 @@ mix( MarkerCollection, EmitterMixin ); * There're two types of markers. Both type of them should be added /updated by {@link module:engine/model/writer~Writer#setMarker} * and removed by {@link module:engine/model/writer~Writer#removeMarker} methods. Both of them are stored in the {@link ~MarkerCollection}. * + * 1. Markers not added to document (default ones). They can be used as bookmarks or visual markers. + * They're great for showing results of the find, or select link when the focus is in the input, + * see https://github.com/ckeditor/ckeditor5-link/issues/13. Sample usage: + * writer.setMarker( markerOrName, ranges ); + * * 1. Markers added to the document. They're synchronized in the collaboration and handled in the undo. - * This type of markers is useful for solutions like spell checking or comments. - * Usage: + * This type of markers is useful for solutions like spell checking or comments. Sample usage: * writer.setMarker( markerOrName, ranges, { usingOperation: true } ); * - * 2. Markers not added to document. They can be used as bookmarks or visual markers. They're great for showing results of the find, - * or select link when the focus is in the input see https://github.com/ckeditor/ckeditor5-link/issues/13. - * Usage: - * writer.setMarker( markerOrName, ranges ); - * * Since markers are based on {@link module:engine/model/liverange~LiveRange live ranges}, for efficiency reasons, it's * best to create and keep at least markers as possible. * diff --git a/src/model/writer.js b/src/model/writer.js index ec7d65d54..89a416f7f 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -766,15 +766,15 @@ export default class Writer { * * There're two types of markers. * - * 1. Markers added to the document. They're synchronized in the collaboration and handled in the undo. - * This type of markers is useful for solutions like spell checking or comments. Sample usage: - * writer.setMarker( markerOrName, ranges, { usingOperation: true } ); - * - * 2. Markers not added to document (default ones). They can be used as bookmarks or visual markers. + * 1. Markers not added to document (default ones). They can be used as bookmarks or visual markers. * They're great for showing results of the find, or select link when the focus is in the input, * see https://github.com/ckeditor/ckeditor5-link/issues/13. Sample usage: * writer.setMarker( markerOrName, ranges ); * + * 1. Markers added to the document. They're synchronized in the collaboration and handled in the undo. + * This type of markers is useful for solutions like spell checking or comments. Sample usage: + * writer.setMarker( markerOrName, ranges, { usingOperation: true } ); + * * If passed name is a name of already existing marker (or {@link module:engine/model/markercollection~Marker Marker} instance * is passed), `range` parameter may be omitted (only for setting markers using operation). In this case marker will not be updated in * {@link module:engine/model/model~Model#markers document marker collection}. However the marker will be added to @@ -807,6 +807,7 @@ export default class Writer { * * Note: For efficiency reasons, it's best to create and keep at least markers as possible. * + * @see {module:engine/model/liverange~LiveRange} * @param {module:engine/model/markercollection~Marker|String|module:engine/model/range~Range} markerOrNameOrRange * Name of marker to add, Marker instance to update or range for the marker with a unique name. * @param {module:engine/model/range~Range|Object} [rangeOrOptions] Marker range or options. From 8c743e89c594dee3475f116735c86fe6ac4e399f Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 2 Feb 2018 18:22:33 +0100 Subject: [PATCH 12/17] Fixed event name and util name. --- src/model/markercollection.js | 10 +++++----- src/model/writer.js | 18 +++++++++--------- tests/model/markercollection.js | 6 +++--- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/model/markercollection.js b/src/model/markercollection.js index 3b81209de..895fa0d97 100644 --- a/src/model/markercollection.js +++ b/src/model/markercollection.js @@ -122,7 +122,7 @@ export default class MarkerCollection { const marker = new Marker( markerName, liveRange, managedUsingOperations ); this._markers.set( markerName, marker ); - this.fire( 'add:' + markerName, marker ); + this.fire( 'set:' + markerName, marker ); return marker; } @@ -211,10 +211,10 @@ export default class MarkerCollection { } /** - * Fired whenever marker is added to `MarkerCollection`. + * Fired whenever marker is added or updated in `MarkerCollection`. * - * @event add - * @param {module:engine/model/markercollection~Marker} The added marker. + * @event set + * @param {module:engine/model/markercollection~Marker} The set marker. */ /** @@ -246,7 +246,7 @@ mix( MarkerCollection, EmitterMixin ); * using common prefixes, separated with `:`, for example: `user:john` or `search:3`. That's useful in term of creating namespaces * for custom elements (e.g. comments, highlights). * - * There're two types of markers. Both type of them should be added /updated by {@link module:engine/model/writer~Writer#setMarker} + * There're two types of markers. Both type of them should be added / updated by {@link module:engine/model/writer~Writer#setMarker} * and removed by {@link module:engine/model/writer~Writer#removeMarker} methods. Both of them are stored in the {@link ~MarkerCollection}. * * 1. Markers not added to document (default ones). They can be used as bookmarks or visual markers. diff --git a/src/model/writer.js b/src/model/writer.js index 89a416f7f..d23eaceed 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -472,7 +472,7 @@ export default class Writer { const delta = new RemoveDelta(); this.batch.addDelta( delta ); - addRemoveOperation( position, howMany, delta, this.model ); + applyRemoveOperation( position, howMany, delta, this.model ); }; if ( itemOrRange instanceof Range ) { @@ -540,7 +540,7 @@ export default class Writer { delta.addOperation( move ); this.model.applyOperation( move ); - addRemoveOperation( position, 1, delta, this.model ); + applyRemoveOperation( position, 1, delta, this.model ); } /** @@ -757,7 +757,7 @@ export default class Writer { delta.addOperation( move ); this.model.applyOperation( move ); - addRemoveOperation( Position.createBefore( element ), 1, delta, this.model ); + applyRemoveOperation( Position.createBefore( element ), 1, delta, this.model ); } /** @@ -860,10 +860,10 @@ export default class Writer { if ( !newRange ) { // If `newRange` is not given, treat this as synchronizing existing marker. // Create `MarkerOperation` with `oldRange` set to `null`, so reverse operation will remove the marker. - addMarkerOperation( this, markerName, null, currentRange ); + applyMarkerOperation( this, markerName, null, currentRange ); } else { // Just change marker range. - addMarkerOperation( this, markerName, currentRange, newRange ); + applyMarkerOperation( this, markerName, currentRange, newRange ); } return this.model.markers.get( markerName ); @@ -901,7 +901,7 @@ export default class Writer { const oldRange = marker.getRange(); - addMarkerOperation( this, name, oldRange, null ); + applyMarkerOperation( this, name, oldRange, null ); } /** @@ -1192,14 +1192,14 @@ function setAttributeOnItem( writer, key, value, item ) { } } -// Creates and adds marker operation to {@link module:engine/model/delta/delta~Delta delta}. +// Creates and applies marker operation to {@link module:engine/model/delta/delta~Delta delta}. // // @private // @param {module:engine/model/writer~Writer} writer // @param {String} name Marker name. // @param {module:engine/model/range~Range} oldRange Marker range before the change. // @param {module:engine/model/range~Range} newRange Marker range after the change. -function addMarkerOperation( writer, name, oldRange, newRange ) { +function applyMarkerOperation( writer, name, oldRange, newRange ) { const model = writer.model; const doc = model.document; const delta = new MarkerDelta(); @@ -1219,7 +1219,7 @@ function addMarkerOperation( writer, name, oldRange, newRange ) { // @param {Number} howMany Number of nodes to remove. // @param {module:engine/model/delta~Delta} delta Delta to add new operation to. // @param {module:engine/model/model~Model} model Model instance on which operation will be applied. -function addRemoveOperation( position, howMany, delta, model ) { +function applyRemoveOperation( position, howMany, delta, model ) { let operation; if ( position.root.document ) { diff --git a/tests/model/markercollection.js b/tests/model/markercollection.js index 54225af2b..138a39083 100644 --- a/tests/model/markercollection.js +++ b/tests/model/markercollection.js @@ -41,7 +41,7 @@ describe( 'MarkerCollection', () => { } ); describe( '_set', () => { - it( 'should create a marker, fire add: event and return true', () => { + it( 'should create a marker, fire set: event and return true', () => { sinon.spy( markers, 'fire' ); const result = markers._set( 'name', range ); @@ -50,7 +50,7 @@ describe( 'MarkerCollection', () => { expect( result ).to.equal( marker ); expect( marker.name ).to.equal( 'name' ); expect( marker.getRange().isEqual( range ) ).to.be.true; - expect( markers.fire.calledWithExactly( 'add:name', marker ) ).to.be.true; + expect( markers.fire.calledWithExactly( 'set:name', marker ) ).to.be.true; } ); it( 'should fire remove: event, and create a new marker if marker with given name was in the collection', () => { @@ -61,7 +61,7 @@ describe( 'MarkerCollection', () => { const marker2 = markers._set( 'name', range2 ); expect( markers.fire.calledWithExactly( 'remove:name', marker1 ) ).to.be.true; - expect( markers.fire.calledWithExactly( 'add:name', marker2 ) ).to.be.true; + expect( markers.fire.calledWithExactly( 'set:name', marker2 ) ).to.be.true; expect( marker2.name ).to.equal( 'name' ); expect( marker2.getRange().isEqual( range2 ) ).to.be.true; From 00dc06e15be95cd952f257a6a85fbe7b757ad271 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 2 Feb 2018 18:32:35 +0100 Subject: [PATCH 13/17] Fixed introduced bug. --- src/model/document.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/model/document.js b/src/model/document.js index 0739bdfe1..307c0f957 100644 --- a/src/model/document.js +++ b/src/model/document.js @@ -161,7 +161,7 @@ export default class Document { // Buffer marker changes. // This is not covered in buffering operations because markers may change outside of them (when they // are modified using `model.markers` collection, not through `MarkerOperation`). - this.listenTo( model.markers, 'add', ( evt, marker ) => { + this.listenTo( model.markers, 'set', ( evt, marker ) => { // TODO: Should filter out changes of markers that are not in document. // Whenever a new marker is added, buffer that change. this.differ.bufferMarkerChange( marker.name, null, marker.getRange() ); From 4315bf1a70f15e790348333d7d3ff1ee1b6d5c8d Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 2 Feb 2018 21:33:46 +0100 Subject: [PATCH 14/17] Marker type shouldn't change during the runtime. --- src/model/markercollection.js | 10 +++++++++- src/model/writer.js | 10 +++++----- tests/model/writer.js | 17 ++++++++++++++--- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/src/model/markercollection.js b/src/model/markercollection.js index 895fa0d97..44aab4977 100644 --- a/src/model/markercollection.js +++ b/src/model/markercollection.js @@ -111,7 +111,15 @@ export default class MarkerCollection { if ( oldMarker ) { const oldRange = oldMarker.getRange(); - if ( oldRange.isEqual( range ) && managedUsingOperations === oldMarker.managedUsingOperations ) { + if ( managedUsingOperations !== oldMarker.managedUsingOperations ) { + /** + * Marker's type should be consistent. Provide correct `managedUsingOperations` parameter. + */ + throw new CKEditorError( 'marker-set-incorrect-marker-type:' + + ' Marker\'s type should be consistent. Provide correct `managedUsingOperations` parameter.' ); + } + + if ( oldRange.isEqual( range ) ) { return oldMarker; } diff --git a/src/model/writer.js b/src/model/writer.js index d23eaceed..1690cf24f 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -815,7 +815,7 @@ export default class Writer { * @param {Boolean} [options.usingOperation=false] Flag indicated whether the marker should be added by MarkerOperation. * @returns {module:engine/model/markercollection~Marker} Marker that was set. */ - setMarker( markerOrNameOrRange, rangeOrManagedUsingOperations, options ) { + setMarker( markerOrNameOrRange, rangeOrOptions, options ) { this._assertWriterUsedCorrectly(); let markerName, newRange, usingOperation; @@ -823,16 +823,16 @@ export default class Writer { if ( markerOrNameOrRange instanceof Range ) { markerName = uid(); newRange = markerOrNameOrRange; - usingOperation = !!rangeOrManagedUsingOperations && !!rangeOrManagedUsingOperations.usingOperation; + usingOperation = !!rangeOrOptions && !!rangeOrOptions.usingOperation; } else { markerName = typeof markerOrNameOrRange === 'string' ? markerOrNameOrRange : markerOrNameOrRange.name; - if ( rangeOrManagedUsingOperations instanceof Range ) { - newRange = rangeOrManagedUsingOperations; + if ( rangeOrOptions instanceof Range ) { + newRange = rangeOrOptions; usingOperation = !!options && !!options.usingOperation; } else { newRange = null; - usingOperation = !!rangeOrManagedUsingOperations && !!rangeOrManagedUsingOperations.usingOperation; + usingOperation = !!rangeOrOptions && !!rangeOrOptions.usingOperation; } } diff --git a/tests/model/writer.js b/tests/model/writer.js index af127e8f9..6882f533c 100644 --- a/tests/model/writer.js +++ b/tests/model/writer.js @@ -2004,11 +2004,14 @@ describe( 'Writer', () => { } ); it( 'should accept marker instance', () => { - const marker = setMarker( 'name', range ); + const marker = setMarker( 'name', range, { usingOperation: true } ); const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 ); setMarker( marker, range2, { usingOperation: true } ); - const op = batch.deltas[ 0 ].operations[ 0 ]; + + expect( batch.deltas.length ).to.equal( 2 ); + + const op = batch.deltas[ 1 ].operations[ 0 ]; expect( model.markers.get( 'name' ).getRange().isEqual( range2 ) ).to.be.true; expect( op.oldRange.isEqual( range ) ).to.be.true; @@ -2016,7 +2019,7 @@ describe( 'Writer', () => { } ); it( 'should accept empty range parameter if marker instance is passed and usingOperation is set to true', () => { - const marker = setMarker( 'name', range ); + const marker = setMarker( 'name', range, { usingOperation: true } ); const spy = sinon.spy(); model.on( 'applyOperation', spy ); @@ -2073,6 +2076,14 @@ describe( 'Writer', () => { } ).to.throw( CKEditorError, /^writer-setMarker-no-range/ ); } ); + it( 'should throw if marker is updated incorrectly', () => { + setMarker( 'name', range ); + + expect( () => { + setMarker( 'name', range, { usingOperation: true } ); + } ).to.throw( CKEditorError, /^marker-set-incorrect-marker-type/ ); + } ); + it( 'should throw when trying to use detached writer', () => { const marker = setMarker( 'name', range ); const writer = new Writer( model, batch ); From e44073b08defcf77ec4d185f704cc20d4d27eb62 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 5 Feb 2018 17:00:27 +0100 Subject: [PATCH 15/17] Added ablity to change marker type at runtime. --- src/model/markercollection.js | 10 +--------- src/model/writer.js | 10 ++++++++-- tests/model/writer.js | 35 +++++++++++++++++++++++++++++++---- 3 files changed, 40 insertions(+), 15 deletions(-) diff --git a/src/model/markercollection.js b/src/model/markercollection.js index 44aab4977..895fa0d97 100644 --- a/src/model/markercollection.js +++ b/src/model/markercollection.js @@ -111,15 +111,7 @@ export default class MarkerCollection { if ( oldMarker ) { const oldRange = oldMarker.getRange(); - if ( managedUsingOperations !== oldMarker.managedUsingOperations ) { - /** - * Marker's type should be consistent. Provide correct `managedUsingOperations` parameter. - */ - throw new CKEditorError( 'marker-set-incorrect-marker-type:' + - ' Marker\'s type should be consistent. Provide correct `managedUsingOperations` parameter.' ); - } - - if ( oldRange.isEqual( range ) ) { + if ( oldRange.isEqual( range ) && managedUsingOperations === oldMarker.managedUsingOperations ) { return oldMarker; } diff --git a/src/model/writer.js b/src/model/writer.js index 1690cf24f..2c52987c3 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -836,6 +836,8 @@ export default class Writer { } } + const currentMarker = this.model.markers.get( markerName ); + if ( !usingOperation ) { if ( !newRange ) { /** @@ -846,11 +848,15 @@ export default class Writer { throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' ); } + // If marker changes to marker that do not use operations then we need to create additional operation + // that removes that marker first. + if ( currentMarker && currentMarker.managedUsingOperations && !usingOperation ) { + applyMarkerOperation( this, markerName, currentMarker.getRange(), null ); + } + return this.model.markers._set( markerName, newRange, usingOperation ); } - const currentMarker = this.model.markers.get( markerName ); - if ( !newRange && !currentMarker ) { throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' ); } diff --git a/tests/model/writer.js b/tests/model/writer.js index 6882f533c..eb9c388bc 100644 --- a/tests/model/writer.js +++ b/tests/model/writer.js @@ -2076,12 +2076,39 @@ describe( 'Writer', () => { } ).to.throw( CKEditorError, /^writer-setMarker-no-range/ ); } ); - it( 'should throw if marker is updated incorrectly', () => { + it( 'should create additional operation when marker type changes to not managed using operation', () => { + const spy = sinon.spy(); + model.on( 'applyOperation', spy ); + + setMarker( 'name', range, { usingOperation: true } ); + const marker = setMarker( 'name', range ); + const op1 = batch.deltas[ 0 ].operations[ 0 ]; + const op2 = batch.deltas[ 1 ].operations[ 0 ]; + + expect( spy.calledTwice ).to.be.true; + expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' ); + expect( spy.secondCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' ); + + 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.managedUsingOperations ).to.be.false; + } ); + + it( 'should enable changing marker to be not managed using operation', () => { + const spy = sinon.spy(); + model.on( 'applyOperation', spy ); + setMarker( 'name', range ); + const marker = setMarker( 'name', range, { usingOperation: true } ); - expect( () => { - setMarker( 'name', range, { usingOperation: true } ); - } ).to.throw( CKEditorError, /^marker-set-incorrect-marker-type/ ); + expect( spy.calledOnce ).to.be.true; + expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' ); + + expect( marker.managedUsingOperations ).to.be.true; } ); it( 'should throw when trying to use detached writer', () => { From d9677fc8f66c3ce4fadd902798bf2e16f8de3118 Mon Sep 17 00:00:00 2001 From: Piotr Jasiun Date: Tue, 6 Feb 2018 15:54:10 +0100 Subject: [PATCH 16/17] Update docs. --- src/model/markercollection.js | 102 ++++++++++++++++++---------------- src/model/writer.js | 50 +++++++---------- 2 files changed, 74 insertions(+), 78 deletions(-) diff --git a/src/model/markercollection.js b/src/model/markercollection.js index 895fa0d97..975cb5709 100644 --- a/src/model/markercollection.js +++ b/src/model/markercollection.js @@ -15,32 +15,19 @@ import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import mix from '@ckeditor/ckeditor5-utils/src/mix'; /** - * Creates, stores and manages {@link module:engine/model/markercollection~Marker markers}. + * The collection of all {@link module:engine/model/markercollection~Marker markers} attached to the document. + * It lets you {@link module:engine/model/markercollection~MarkerCollection#get get} markers or track them using + * {@link module:engine/model/markercollection~MarkerCollection#event:set} and + * {@link module:engine/model/markercollection~MarkerCollection#event:remove} events. * - * Markers are designed to be a ready-to-use solution for having up-to-date ranges on the document with an easy access to them. - * They're built from a name and a {@link module:engine/model/liverange~LiveRange live range}. - * Name is used to group and identify markers. Names have to be unique, but markers can be grouped by - * using common prefixes, separated with `:`, for example: `user:john` or `search:3`. That's useful in term of creating namespaces - * for custom elements (e.g. comments, highlights). - * - * Markers can be watched for changes - both adding and removing markers from the `MarkerCollection` fires events - * (`~MarkerCollection#event:add` and `MarkerCollection#event:remove`). - * - * There're two types of markers. Both type of them should be added / updated by {@link module:engine/model/writer~Writer#setMarker} - * and removed by {@link module:engine/model/writer~Writer#removeMarker} methods. Both of them are stored in the {@link ~MarkerCollection}. + * To create, change or remove makers use {@link module:engine/model/writer~Writer model writers'} methods: + * {@link module:engine/model/writer~Writer#setMarker} or {@link module:engine/model/writer~Writer#removeMarker}. Since + * the writer is the only proper way to change the data model it is not possible to change markers directly using this + * collection. All markers created by the writer will be automatically added to this collection. * - * 1. Markers not added to document (default ones). They can be used as bookmarks or visual markers. - * They're great for showing results of the find, or select link when the focus is in the input, - * see https://github.com/ckeditor/ckeditor5-link/issues/13. Sample usage: - * writer.setMarker( markerOrName, ranges ); + * By default there is one marker collection available as {@link module:engine/model/model~Model#markers model property}. * - * 1. Markers added to the document. They're synchronized in the collaboration and handled in the undo. - * This type of markers is useful for solutions like spell checking or comments. Sample usage: - * writer.setMarker( markerOrName, ranges, { usingOperation: true } ); - * - * Note: For efficiency reasons, it's best to create and keep at least markers as possible. - * - * @see {module:engine/model/liverange~LiveRange} + * @see module:engine/model/markercollection~Marker */ export default class MarkerCollection { /** @@ -97,9 +84,9 @@ export default class MarkerCollection { * happens and `false` is returned. * * @protected - * @fires module:engine/model/markercollection~MarkerCollection#event:add + * @fires module:engine/model/markercollection~MarkerCollection#event:set * @fires module:engine/model/markercollection~MarkerCollection#event:remove - * @param {String|module:engine/model/markercollection~Marker} markerOrName Name of marker to add or Marker instance to update. + * @param {String|module:engine/model/markercollection~Marker} markerOrName Name of marker to set or marker instance to update. * @param {module:engine/model/range~Range} range Marker range. * @param {Boolean} managedUsingOperations Specifies whether the marker is managed using operations. * @returns {module:engine/model/markercollection~Marker} `Marker` instance added to the collection. @@ -230,48 +217,64 @@ mix( MarkerCollection, EmitterMixin ); /** * `Marker` is a continuous parts of model (like a range), is named and represent some kind of information about marked * part of model document. In contrary to {@link module:engine/model/node~Node nodes}, which are building blocks of - * model document tree, markers are not stored directly in document tree. Still, they are document data, by giving + * model document tree, markers are not stored directly in document tree but in + * {@link module:engine/model/model~Model#markers model markers' collection}. Still, they are document data, by giving * additional meaning to the part of a model document between marker start and marker end. * * In this sense, markers are similar to adding and converting attributes on nodes. The difference is that attribute is * connected with a given node (e.g. a character is bold no matter if it gets moved or content around it changes). - * Markers on the other hand are continuous ranges and are characterised by their start and end position. This means that + * Markers on the other hand are continuous ranges and are characterized by their start and end position. This means that * any character in the marker is marked by the marker. For example, if a character is moved outside of marker it stops being * "special" and the marker is shrunk. Similarly, when a character is moved into the marker from other place in document * model, it starts being "special" and the marker is enlarged. * - * Markers are designed to be a ready-to-use solution for having up-to-date ranges on the document with an easy access to them. - * They're built from a name and a {@link module:engine/model/liverange~LiveRange live range}. + * Another upside of markers is that finding marked part of document is fast and easy. Using attributes to mark some nodes + * and then trying to find that part of document would require traversing whole document tree. Marker gives instant access + * to the range which it is marking at the moment. + * + * Markers are built from a name and a range. + * + * Range of the marker is updated automatically when document changes, using + * {@link module:engine/model/liverange~LiveRange live range} mechanism. + * * Name is used to group and identify markers. Names have to be unique, but markers can be grouped by - * using common prefixes, separated with `:`, for example: `user:john` or `search:3`. That's useful in term of creating namespaces - * for custom elements (e.g. comments, highlights). + * using common prefixes, separated with `:`, for example: `user:john` or `search:3`. That's useful in term of creating + * namespaces for custom elements (e.g. comments, highlights). You can use this prefixes in + * {@link module:engine/model/markercollection~MarkerCollection#event:set} listeners to listen on changes in a group of markers. + * For instance: `model.markers.on( 'set:user', callback );` will be called whenever any `user:*` markers changes. * - * There're two types of markers. Both type of them should be added / updated by {@link module:engine/model/writer~Writer#setMarker} - * and removed by {@link module:engine/model/writer~Writer#removeMarker} methods. Both of them are stored in the {@link ~MarkerCollection}. + * There are two types of markers. * - * 1. Markers not added to document (default ones). They can be used as bookmarks or visual markers. - * They're great for showing results of the find, or select link when the focus is in the input, - * see https://github.com/ckeditor/ckeditor5-link/issues/13. Sample usage: - * writer.setMarker( markerOrName, ranges ); + * 1. Markers managed directly, without using operations. They are added directly by {@link module:engine/model/writer~Writer} + * to the {@link module:engine/model/markercollection~MarkerCollection} without any additional mechanism. They can be used + * as bookmarks or visual markers. They are great for showing results of the find, or select link when the focus is in the input. * - * 1. Markers added to the document. They're synchronized in the collaboration and handled in the undo. - * This type of markers is useful for solutions like spell checking or comments. Sample usage: - * writer.setMarker( markerOrName, ranges, { usingOperation: true } ); + * 1. Markers managed using operations. These markers are also stored in {@link module:engine/model/markercollection~MarkerCollection} + * but changes in these markers is managed the same way all other changes in the model structure - using operations. + * Therefore, they are handled in the undo stack and synchronized between clients if the collaboration plugin is enabled. + * This type of markers is useful for solutions like spell checking or comments. * - * Since markers are based on {@link module:engine/model/liverange~LiveRange live ranges}, for efficiency reasons, it's - * best to create and keep at least markers as possible. + * Both type of them should be added / updated by {@link module:engine/model/writer~Writer#setMarker} + * and removed by {@link module:engine/model/writer~Writer#removeMarker} methods. + * + * model.change( ( writer ) => { + * const marker = writer.setMarker( name, range, { usingOperations: true } ); + * + * // ... + * + * writer.removeMarker( marker ); + * } ); + * + * See {@link module:engine/model/writer~Writer} to find more examples. + * + * Since markers need to track change in the document, for efficiency reasons, it is best to create and keep as little + * markers as possible and remove them as soon as they are not needed anymore. * * Markers can be converted to view by adding appropriate converters for * {@link module:engine/conversion/modelconversiondispatcher~ModelConversionDispatcher#event:addMarker} and * {@link module:engine/conversion/modelconversiondispatcher~ModelConversionDispatcher#event:removeMarker} * events, or by building converters for {@link module:engine/conversion/modelconversiondispatcher~ModelConversionDispatcher} * using {@link module:engine/conversion/buildmodelconverter~buildModelConverter model converter builder}. - * - * Another upside of markers is that finding marked part of document is fast and easy. Using attributes to mark some nodes - * and then trying to find that part of document would require traversing whole document tree. Marker gives instant access - * to the range which it is marking at the moment. - * - * `Marker` instances are created and destroyed only by {@link ~MarkerCollection MarkerCollection}. */ class Marker { /** @@ -290,7 +293,8 @@ class Marker { this.name = name; /** - * Flag indicates if the marker should be managed using operations. + * Flag indicates if the marker is managed using operations or not. See {@link ~Marker marker class description} + * to learn more about marker types. See {@link module:engine/model/writer~Writer#setMarker}. * * @readonly * @type {Boolean} diff --git a/src/model/writer.js b/src/model/writer.js index 2c52987c3..381dfe93a 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -761,39 +761,30 @@ export default class Writer { } /** - * Adds or updates {@link module:engine/model/markercollection~Marker marker} with given name to given `range`. - * If the marker, nor marker's name is not provided, new marker is created and returned with a unique name. + * Adds or updates {@link module:engine/model/markercollection~Marker marker}. * - * There're two types of markers. + * As the first parameter you can set marker name or instance. If none of them is provided, new marker, with a unique + * name is created and returned. * - * 1. Markers not added to document (default ones). They can be used as bookmarks or visual markers. - * They're great for showing results of the find, or select link when the focus is in the input, - * see https://github.com/ckeditor/ckeditor5-link/issues/13. Sample usage: - * writer.setMarker( markerOrName, ranges ); + * Using this method you can change markers range or define if the marker is managed by operation or not. * - * 1. Markers added to the document. They're synchronized in the collaboration and handled in the undo. - * This type of markers is useful for solutions like spell checking or comments. Sample usage: - * writer.setMarker( markerOrName, ranges, { usingOperation: true } ); + * Marker tracks changes is the document and updates the range automatically, so you need to update the range only + * when it changes directly. You do not need to update it after each document change. * - * If passed name is a name of already existing marker (or {@link module:engine/model/markercollection~Marker Marker} instance - * is passed), `range` parameter may be omitted (only for setting markers using operation). In this case marker will not be updated in - * {@link module:engine/model/model~Model#markers document marker collection}. However the marker will be added to - * the document history. This may be important for other features, like undo. From document history point of view, it will - * look like the marker was created and added to the document at the moment when it is set using this method. + * The option parameter let you decide if the marker should be managed by operations or not. See + * {@link module:engine/model/markercollection~Marker marker class description} to learn about the difference between + * markers managed by operation and managed directly. You can change this option for existing marker. This is + * useful if a marker have been created earlier and need to be added to the document history later. * - * This is useful if the marker is created before it can be added to document history (e.g. a feature creating the marker - * is waiting for additional data, etc.). In this case, the marker may be first created directly through - * {@link module:engine/model/markercollection~MarkerCollection MarkerCollection API} and only later added using `Batch` API. - * - * Create / update marker using `MarkerOperation` operation: + * Update marker using operation: * * setMarker( marker, range, { usingOperations: true } ); * - * Create / update marker directly base on marker's name: + * Create/update marker directly base on marker's name: * * setMarker( markerName, range ); * - * Create marker with a unique id using `MarkerOperation` operation: + * Create marker with a unique id using operation: * * setMarker( range, { usingOperations: true } ); * @@ -801,18 +792,19 @@ export default class Writer { * * setMarker( range ) * - * Update marker using `MarkerOperation` operation. + * Change marker's option (start using operations to manage it): * * setMarker( marker, { usingOperations: true } ); * - * Note: For efficiency reasons, it's best to create and keep at least markers as possible. + * Note: For efficiency reasons, it's best to create and keep as little markers as possible. * - * @see {module:engine/model/liverange~LiveRange} - * @param {module:engine/model/markercollection~Marker|String|module:engine/model/range~Range} markerOrNameOrRange + * @see module:engine/model/markercollection~Marker + * @param {module:engine/model/markercollection~Marker|String} [markerOrName=uid()] * Name of marker to add, Marker instance to update or range for the marker with a unique name. - * @param {module:engine/model/range~Range|Object} [rangeOrOptions] Marker range or options. + * @param {module:engine/model/range~Range|Object} [range] Marker range or options. * @param {Object} [options] * @param {Boolean} [options.usingOperation=false] 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. */ setMarker( markerOrNameOrRange, rangeOrOptions, options ) { @@ -877,8 +869,8 @@ export default class Writer { /** * Removes given {@link module:engine/model/markercollection~Marker marker} or marker with given name. - * There's no `usingOperation` option available here, the marker destructing mechanism is hidden and used - * accordingly to how the marker was created, so if the marker was created with operation, it will be destroyed using operation. + * The marker is removed accordingly to how it has been created, so if the marker was created using operation, + * it will be destroyed using operation. * * @param {module:engine/model/markercollection~Marker|String} markerOrName Marker or marker name to remove. * @param {Object} [options] From c182c175ee6d59e2cab73889980096581c064fab Mon Sep 17 00:00:00 2001 From: Piotr Jasiun Date: Tue, 6 Feb 2018 15:58:10 +0100 Subject: [PATCH 17/17] Renamed `usingOperations` to `usingOperation` in docs. --- src/model/markercollection.js | 2 +- src/model/writer.js | 6 +++--- tests/model/writer.js | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/model/markercollection.js b/src/model/markercollection.js index 975cb5709..dcad1746a 100644 --- a/src/model/markercollection.js +++ b/src/model/markercollection.js @@ -258,7 +258,7 @@ mix( MarkerCollection, EmitterMixin ); * and removed by {@link module:engine/model/writer~Writer#removeMarker} methods. * * model.change( ( writer ) => { - * const marker = writer.setMarker( name, range, { usingOperations: true } ); + * const marker = writer.setMarker( name, range, { usingOperation: true } ); * * // ... * diff --git a/src/model/writer.js b/src/model/writer.js index 381dfe93a..4675098fb 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -778,7 +778,7 @@ export default class Writer { * * Update marker using operation: * - * setMarker( marker, range, { usingOperations: true } ); + * setMarker( marker, range, { usingOperation: true } ); * * Create/update marker directly base on marker's name: * @@ -786,7 +786,7 @@ export default class Writer { * * Create marker with a unique id using operation: * - * setMarker( range, { usingOperations: true } ); + * setMarker( range, { usingOperation: true } ); * * Create marker directly with a unique name: * @@ -794,7 +794,7 @@ export default class Writer { * * Change marker's option (start using operations to manage it): * - * setMarker( marker, { usingOperations: true } ); + * setMarker( marker, { usingOperation: true } ); * * Note: For efficiency reasons, it's best to create and keep as little markers as possible. * diff --git a/tests/model/writer.js b/tests/model/writer.js index eb9c388bc..a411da79b 100644 --- a/tests/model/writer.js +++ b/tests/model/writer.js @@ -2040,7 +2040,7 @@ describe( 'Writer', () => { expect( marker.name ).to.be.a( 'string' ); } ); - it( 'should create a unique id if the first param is type of range when usingOperations is set to true', () => { + it( 'should create a unique id if the first param is type of range when usingOperation is set to true', () => { const spy = sinon.spy(); model.on( 'applyOperation', spy ); @@ -2050,7 +2050,7 @@ describe( 'Writer', () => { expect( spy.calledOnce ).to.be.true; } ); - it( 'should use operations when having set usingOperations to true', () => { + it( 'should use operations when having set usingOperation to true', () => { const spy = sinon.spy(); model.on( 'applyOperation', spy );