From abff96bc997e44f6f64b3f1c5e0ffe23ff845684 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 23 Jan 2020 19:05:07 +0100 Subject: [PATCH 1/5] Feature: Introduced `DocumentSelection#event:change:marker`. --- src/model/documentselection.js | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/model/documentselection.js b/src/model/documentselection.js index 53c1bd119..814e9f29b 100644 --- a/src/model/documentselection.js +++ b/src/model/documentselection.js @@ -63,6 +63,7 @@ export default class DocumentSelection { this._selection.delegate( 'change:range' ).to( this ); this._selection.delegate( 'change:attribute' ).to( this ); + this._selection.delegate( 'change:marker' ).to( this ); } /** @@ -542,6 +543,17 @@ mix( DocumentSelection, EmitterMixin ); * @param {Array.} attributeKeys Array containing keys of attributes that changed. */ +/** + * Fired when selection marker(s) changed. + * + * @event change:marker + * @param {Boolean} directChange This is always set to `false` in case of `change:marker` event as there is no possibility + * to change markers directly through {@link module:engine/model/documentselection~DocumentSelection} API. + * See also {@link module:engine/model/documentselection~DocumentSelection#event:change:range} and + * {@link module:engine/model/documentselection~DocumentSelection#event:change:attribute}. + * @param {Array.} oldMarkers Markers in which the selection was before the change. + */ + // `LiveSelection` is used internally by {@link module:engine/model/documentselection~DocumentSelection} and shouldn't be used directly. // // LiveSelection` is automatically updated upon changes in the {@link module:engine/model/document~Document document} @@ -830,6 +842,7 @@ class LiveSelection extends Selection { _updateMarkers() { const markers = []; + let changed = false; for ( const marker of this._model.markers ) { const markerRange = marker.getRange(); @@ -841,17 +854,27 @@ class LiveSelection extends Selection { } } + const oldMarkers = Array.from( this.markers ); + for ( const marker of markers ) { if ( !this.markers.has( marker ) ) { this.markers.add( marker ); + + changed = true; } } for ( const marker of Array.from( this.markers ) ) { if ( !markers.includes( marker ) ) { this.markers.remove( marker ); + + changed = true; } } + + if ( changed ) { + this.fire( 'change:marker', { oldMarkers } ); + } } // Updates this selection attributes according to its ranges and the {@link module:engine/model/document~Document model document}. From 98098c845fc2ed1653d0c000108dfdf50c1cc4b4 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 24 Jan 2020 11:52:49 +0100 Subject: [PATCH 2/5] Other: `DocumentSelection#event:change:marker` should be fired just after a selection range change, like `event:change:attribute`. --- src/model/documentselection.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/model/documentselection.js b/src/model/documentselection.js index 814e9f29b..0ebf2f1aa 100644 --- a/src/model/documentselection.js +++ b/src/model/documentselection.js @@ -733,11 +733,13 @@ class LiveSelection extends Selection { setTo( selectable, optionsOrPlaceOrOffset, options ) { super.setTo( selectable, optionsOrPlaceOrOffset, options ); this._updateAttributes( true ); + this._updateMarkers(); } setFocus( itemOrPosition, offset ) { super.setFocus( itemOrPosition, offset ); this._updateAttributes( true ); + this._updateMarkers(); } setAttribute( key, value ) { @@ -873,7 +875,7 @@ class LiveSelection extends Selection { } if ( changed ) { - this.fire( 'change:marker', { oldMarkers } ); + this.fire( 'change:marker', { oldMarkers, directChange: false } ); } } From ca1161217fb3132384a3834e51faee24de9966b2 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 24 Jan 2020 11:53:03 +0100 Subject: [PATCH 3/5] Tests: Tests for `DocumentSelection#event:change:marker`. --- tests/model/documentselection.js | 44 ++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/model/documentselection.js b/tests/model/documentselection.js index acb3f93b4..39243a629 100644 --- a/tests/model/documentselection.js +++ b/tests/model/documentselection.js @@ -442,6 +442,50 @@ describe( 'DocumentSelection', () => { expect( selection.markers.map( marker => marker.name ) ).to.have.members( [ 'marker' ] ); } ); + + it( 'should fire change:marker event when selection markers change', () => { + model.change( writer => { + const spy = sinon.spy(); + + model.document.selection.on( 'change:marker', spy ); + + writer.setSelection( writer.createRange( + writer.createPositionFromPath( root, [ 2, 1 ] ), + writer.createPositionFromPath( root, [ 2, 2 ] ) + ) ); + + expect( spy.called ).to.be.false; + + writer.addMarker( 'marker-1', { + range: writer.createRange( + writer.createPositionFromPath( root, [ 2, 0 ] ), + writer.createPositionFromPath( root, [ 2, 5 ] ) + ), + usingOperation: false + } ); + + expect( spy.calledOnce ).to.be.true; + + writer.addMarker( 'marker-2', { + range: writer.createRange( + writer.createPositionFromPath( root, [ 2, 0 ] ), + writer.createPositionFromPath( root, [ 2, 3 ] ) + ), + usingOperation: false + } ); + + expect( spy.calledTwice ).to.be.true; + spy.resetHistory(); + + writer.setSelection( writer.createPositionFromPath( root, [ 2, 6 ] ) ); + + expect( spy.calledOnce ).to.be.true; + + writer.setSelection( writer.createPositionFromPath( root, [ 2, 2 ] ) ); + + expect( spy.calledTwice ).to.be.true; + } ); + } ); } ); describe( 'destroy()', () => { From edd36c48975b636da8bc502f614c7be2942119f2 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 5 Feb 2020 16:53:49 +0100 Subject: [PATCH 4/5] Tests: Written more test cases. --- tests/model/documentselection.js | 224 +++++++++++++++++++++++++++---- 1 file changed, 199 insertions(+), 25 deletions(-) diff --git a/tests/model/documentselection.js b/tests/model/documentselection.js index 39243a629..4876879f7 100644 --- a/tests/model/documentselection.js +++ b/tests/model/documentselection.js @@ -443,47 +443,221 @@ describe( 'DocumentSelection', () => { expect( selection.markers.map( marker => marker.name ) ).to.have.members( [ 'marker' ] ); } ); - it( 'should fire change:marker event when selection markers change', () => { - model.change( writer => { + describe( 'should fire change:marker event when', () => { + // Set marker to range 0-4. + beforeEach( () => { + model.change( writer => { + writer.addMarker( 'marker-1', { + range: writer.createRange( + writer.createPositionFromPath( root, [ 2, 0 ] ), + writer.createPositionFromPath( root, [ 2, 4 ] ) + ), + usingOperation: false + } ); + } ); + } ); + + it( 'selection ranges change (marker added to the selection)', () => { const spy = sinon.spy(); - model.document.selection.on( 'change:marker', spy ); + model.change( writer => { + // The selection has no markers before the change. + model.document.selection.on( 'change:marker', ( evt, data ) => { + expect( data.oldMarkers ).to.deep.equal( [] ); + spy(); + } ); + + // Move selection to 1-2, that is inside 0-4 marker. + writer.setSelection( writer.createRange( + writer.createPositionFromPath( root, [ 2, 1 ] ), + writer.createPositionFromPath( root, [ 2, 2 ] ) + ) ); + } ); - writer.setSelection( writer.createRange( - writer.createPositionFromPath( root, [ 2, 1 ] ), - writer.createPositionFromPath( root, [ 2, 2 ] ) - ) ); + expect( spy.calledOnce ).to.be.true; + } ); - expect( spy.called ).to.be.false; + it( 'selection ranges change (marker removed from the selection)', () => { + const spy = sinon.spy(); - writer.addMarker( 'marker-1', { - range: writer.createRange( - writer.createPositionFromPath( root, [ 2, 0 ] ), - writer.createPositionFromPath( root, [ 2, 5 ] ) - ), - usingOperation: false + model.change( writer => { + writer.setSelection( writer.createRange( + writer.createPositionFromPath( root, [ 2, 1 ] ), + writer.createPositionFromPath( root, [ 2, 2 ] ) + ) ); + + // The selection is in a marker before the change. + model.document.selection.on( 'change:marker', ( evt, data ) => { + expect( data.oldMarkers.map( marker => marker.name ) ).to.deep.equal( [ 'marker-1' ] ); + spy(); + } ); + + // Move the selection out of the marker. + writer.setSelection( writer.createPositionFromPath( root, [ 2, 5 ] ) ); } ); expect( spy.calledOnce ).to.be.true; + } ); - writer.addMarker( 'marker-2', { - range: writer.createRange( - writer.createPositionFromPath( root, [ 2, 0 ] ), - writer.createPositionFromPath( root, [ 2, 3 ] ) - ), - usingOperation: false + it( 'selection focus changes (marker removed from the selection)', () => { + const spy = sinon.spy(); + + model.change( writer => { + writer.setSelection( writer.createPositionFromPath( root, [ 2, 2 ] ) ); + + // The selection is in a marker before the change. + model.document.selection.on( 'change:marker', ( evt, data ) => { + expect( data.oldMarkers.map( marker => marker.name ) ).to.deep.equal( [ 'marker-1' ] ); + spy(); + } ); + + // Move the selection focus out of the marker. + writer.setSelectionFocus( writer.createPositionFromPath( root, [ 2, 5 ] ) ); + } ); + + expect( spy.calledOnce ).to.be.true; + } ); + + it( 'a new marker contains the selection', () => { + const spy = sinon.spy(); + + model.change( writer => { + writer.setSelection( writer.createPositionFromPath( root, [ 2, 5 ] ) ); + + // The selection is not in a marker before the change. + model.document.selection.on( 'change:marker', ( evt, data ) => { + expect( data.oldMarkers ).to.deep.equal( [] ); + spy(); + } ); + + writer.updateMarker( 'marker-1', { + range: writer.createRange( + writer.createPositionFromPath( root, [ 2, 0 ] ), + writer.createPositionFromPath( root, [ 2, 6 ] ) + ) + } ); } ); - expect( spy.calledTwice ).to.be.true; - spy.resetHistory(); + expect( spy.calledOnce ).to.be.true; + } ); - writer.setSelection( writer.createPositionFromPath( root, [ 2, 6 ] ) ); + it( 'a marker stops contains the selection', () => { + const spy = sinon.spy(); + + model.change( writer => { + writer.setSelection( writer.createPositionFromPath( root, [ 2, 3 ] ) ); + + // The selection is in a marker before the change. + model.document.selection.on( 'change:marker', ( evt, data ) => { + expect( data.oldMarkers.map( marker => marker.name ) ).to.deep.equal( [ 'marker-1' ] ); + spy(); + } ); + + writer.updateMarker( 'marker-1', { + range: writer.createRange( + writer.createPositionFromPath( root, [ 2, 0 ] ), + writer.createPositionFromPath( root, [ 2, 1 ] ) + ) + } ); + } ); expect( spy.calledOnce ).to.be.true; + } ); + } ); + + describe( 'should not fire change:marker event when', () => { + // Set marker to range 0-4. + beforeEach( () => { + model.change( writer => { + writer.addMarker( 'marker-1', { + range: writer.createRange( + writer.createPositionFromPath( root, [ 2, 0 ] ), + writer.createPositionFromPath( root, [ 2, 4 ] ) + ), + usingOperation: false + } ); + } ); + } ); + + it( 'selection ranges change does not change selection markers - no markers', () => { + const spy = sinon.spy(); + + model.document.selection.on( 'change:marker', spy ); + + model.change( writer => { + writer.setSelection( writer.createPositionFromPath( root, [ 2, 5 ] ) ); + } ); + + expect( spy.called ).to.be.false; + } ); - writer.setSelection( writer.createPositionFromPath( root, [ 2, 2 ] ) ); + it( 'selection ranges change does not change selection markers - same markers', () => { + model.change( writer => { + writer.setSelection( writer.createPositionFromPath( root, [ 2, 2 ] ) ); + } ); + + const spy = sinon.spy(); + + model.document.selection.on( 'change:marker', spy ); + + model.change( writer => { + writer.setSelection( writer.createPositionFromPath( root, [ 2, 3 ] ) ); + } ); + + expect( spy.called ).to.be.false; + } ); + + it( 'selection focus change does not change selection markers', () => { + model.change( writer => { + writer.setSelection( writer.createPositionFromPath( root, [ 2, 2 ] ) ); + } ); + + const spy = sinon.spy(); + + model.document.selection.on( 'change:marker', spy ); - expect( spy.calledTwice ).to.be.true; + model.change( writer => { + writer.setSelectionFocus( writer.createPositionFromPath( root, [ 2, 3 ] ) ); + } ); + + expect( spy.called ).to.be.false; + } ); + + it( 'changed marker still contains the selection', () => { + model.change( writer => { + writer.setSelection( writer.createPositionFromPath( root, [ 2, 2 ] ) ); + } ); + + const spy = sinon.spy(); + + model.document.selection.on( 'change:marker', spy ); + + model.change( writer => { + writer.updateMarker( 'marker-1', { + range: writer.createRange( + writer.createPositionFromPath( root, [ 2, 0 ] ), + writer.createPositionFromPath( root, [ 2, 5 ] ) + ) + } ); + } ); + + expect( spy.called ).to.be.false; + } ); + + it( 'removed marker did not contain the selection', () => { + model.change( writer => { + writer.setSelection( writer.createPositionFromPath( root, [ 2, 5 ] ) ); + } ); + + const spy = sinon.spy(); + + model.document.selection.on( 'change:marker', spy ); + + model.change( writer => { + writer.removeMarker( 'marker-1' ); + } ); + + expect( spy.called ).to.be.false; } ); } ); } ); From 5822fd403b11a7fc0351a9e4050a50157c5768de Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 5 Feb 2020 16:55:25 +0100 Subject: [PATCH 5/5] Tests: Changed names of two tests. --- tests/model/documentselection.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/model/documentselection.js b/tests/model/documentselection.js index 4876879f7..84cba4fbc 100644 --- a/tests/model/documentselection.js +++ b/tests/model/documentselection.js @@ -579,7 +579,7 @@ describe( 'DocumentSelection', () => { } ); } ); - it( 'selection ranges change does not change selection markers - no markers', () => { + it( 'selection ranges change does not change selection markers (no markers)', () => { const spy = sinon.spy(); model.document.selection.on( 'change:marker', spy ); @@ -591,7 +591,7 @@ describe( 'DocumentSelection', () => { expect( spy.called ).to.be.false; } ); - it( 'selection ranges change does not change selection markers - same markers', () => { + it( 'selection ranges change does not change selection markers (same markers)', () => { model.change( writer => { writer.setSelection( writer.createPositionFromPath( root, [ 2, 2 ] ) ); } );