From 6fbb3f11849d019c4dbc8cdf305ba244f8bb7f1b Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 13 Aug 2019 09:25:20 +0200 Subject: [PATCH 1/6] Docs: Fixed typo. --- src/view/downcastwriter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/view/downcastwriter.js b/src/view/downcastwriter.js index 6abdf3316..a930e2dda 100644 --- a/src/view/downcastwriter.js +++ b/src/view/downcastwriter.js @@ -69,7 +69,7 @@ export default class DowncastWriter { * writer.setSelection( range ); * * // Sets selection to given ranges. - * const ranges = [ writer.createRange( start1, end2 ), writer.createRange( star2, end2 ) ]; + * const ranges = [ writer.createRange( start1, end2 ), writer.createRange( start2, end2 ) ]; * writer.setSelection( range ); * * // Sets selection to the other selection. From 618bdc274f6ba0fddb676d0512a288a8416a88e7 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 13 Aug 2019 10:35:31 +0200 Subject: [PATCH 2/6] Internal: Introduced `Mapper#_unboundMarkers`. --- src/conversion/mapper.js | 52 +++++++++++++++++++++++++++++++++----- tests/conversion/mapper.js | 33 ++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 6 deletions(-) diff --git a/src/conversion/mapper.js b/src/conversion/mapper.js index d0734594e..703f0e23e 100644 --- a/src/conversion/mapper.js +++ b/src/conversion/mapper.js @@ -77,6 +77,25 @@ export default class Mapper { */ this._markerNameToElements = new Map(); + /** + * View element to model marker names mapping. + * + * This is reverse to {@link ~Mapper#_markerNameToElements} map. + * + * @private + * @member {Map} + */ + this._elementToMarkerNames = new Map(); + + /** + * Stores markers which has changed due to unbinding a view element (so it is assumed that the view element has been removed, + * moved or renamed). In some cases such markers need to be refreshed (re-rendered). + * + * @protected + * @member {Set.} + */ + this._unboundMarkers = new Set(); + // Default mapper algorithm for mapping model position to view position. this.on( 'modelToViewPosition', ( evt, data ) => { if ( data.viewPosition ) { @@ -132,6 +151,12 @@ export default class Mapper { this._viewToModelMapping.delete( viewElement ); + if ( this._elementToMarkerNames.has( viewElement ) ) { + for ( const markerName of this._elementToMarkerNames.get( viewElement ) ) { + this._unboundMarkers.add( markerName ); + } + } + if ( this._modelToViewMapping.get( modelElement ) == viewElement ) { this._modelToViewMapping.delete( modelElement ); } @@ -167,10 +192,13 @@ export default class Mapper { */ bindElementToMarker( element, name ) { const elements = this._markerNameToElements.get( name ) || new Set(); - elements.add( element ); + const names = this._elementToMarkerNames.get( element ) || new Set(); + names.add( name ); + this._markerNameToElements.set( name, elements ); + this._elementToMarkerNames.set( element, names ); } /** @@ -182,12 +210,22 @@ export default class Mapper { unbindElementFromMarkerName( element, name ) { const elements = this._markerNameToElements.get( name ); - if ( elements ) { - elements.delete( element ); + if ( !elements ) { + return; + } - if ( elements.size == 0 ) { - this._markerNameToElements.delete( name ); - } + elements.delete( element ); + + if ( elements.size == 0 ) { + this._markerNameToElements.delete( name ); + } + + const names = this._elementToMarkerNames.get( element ); + + names.delete( name ); + + if ( names.size == 0 ) { + this._elementToMarkerNames.delete( element ); } } @@ -198,6 +236,8 @@ export default class Mapper { this._modelToViewMapping = new WeakMap(); this._viewToModelMapping = new WeakMap(); this._markerNameToElements = new Map(); + this._elementToMarkerNames = new Map(); + this._unboundMarkers = new Set(); } /** diff --git a/tests/conversion/mapper.js b/tests/conversion/mapper.js index 1afea7c3f..d667d9882 100644 --- a/tests/conversion/mapper.js +++ b/tests/conversion/mapper.js @@ -23,15 +23,23 @@ describe( 'Mapper', () => { const viewA = new ViewElement( 'a' ); const viewB = new ViewElement( 'b' ); const viewC = new ViewElement( 'c' ); + const viewD = new ViewElement( 'd' ); const modelA = new ModelElement( 'a' ); const modelB = new ModelElement( 'b' ); const modelC = new ModelElement( 'c' ); + const modelD = new ModelElement( 'd' ); const mapper = new Mapper(); + mapper.bindElements( modelA, viewA ); mapper.bindElements( modelB, viewB ); mapper.bindElements( modelC, viewC ); + mapper.bindElements( modelD, viewD ); + + mapper.bindElementToMarker( viewA, 'foo' ); + mapper.bindElementToMarker( viewD, 'foo' ); + mapper.bindElementToMarker( viewD, 'bar' ); expect( mapper.toModelElement( viewA ) ).to.equal( modelA ); expect( mapper.toModelElement( viewB ) ).to.equal( modelB ); @@ -41,6 +49,11 @@ describe( 'Mapper', () => { expect( mapper.toViewElement( modelB ) ).to.equal( viewB ); expect( mapper.toViewElement( modelC ) ).to.equal( viewC ); + expect( Array.from( mapper.markerNameToElements( 'foo' ) ) ).to.deep.equal( [ viewA, viewD ] ); + + mapper.unbindViewElement( viewD ); + expect( Array.from( mapper._unboundMarkers ) ).to.deep.equal( [ 'foo', 'bar' ] ); + mapper.clearBindings(); expect( mapper.toModelElement( viewA ) ).to.be.undefined; @@ -50,6 +63,9 @@ describe( 'Mapper', () => { expect( mapper.toViewElement( modelA ) ).to.be.undefined; expect( mapper.toViewElement( modelB ) ).to.be.undefined; expect( mapper.toViewElement( modelC ) ).to.be.undefined; + + expect( mapper.markerNameToElements( 'foo' ) ).to.be.null; + expect( Array.from( mapper._unboundMarkers ) ).to.deep.equal( [] ); } ); } ); @@ -123,6 +139,21 @@ describe( 'Mapper', () => { expect( mapper.toModelElement( viewA ) ).to.be.undefined; expect( mapper.toViewElement( modelA ) ).to.equal( viewB ); } ); + + it( 'should add marker names to _unboundMarkers set', () => { + const viewA = new ViewElement( 'a' ); + const modelA = new ModelElement( 'a' ); + + const mapper = new Mapper(); + mapper.bindElements( modelA, viewA ); + + mapper.bindElementToMarker( viewA, 'foo' ); + mapper.bindElementToMarker( viewA, 'bar' ); + + mapper.unbindViewElement( viewA ); + + expect( Array.from( mapper._unboundMarkers ) ).to.deep.equal( [ 'foo', 'bar' ] ); + } ); } ); describe( 'Standard mapping', () => { @@ -648,11 +679,13 @@ describe( 'Mapper', () => { const viewB = new ViewElement( 'b' ); mapper.bindElementToMarker( viewA, 'marker' ); + mapper.bindElementToMarker( viewA, 'markerB' ); mapper.bindElementToMarker( viewB, 'marker' ); mapper.unbindElementFromMarkerName( viewA, 'marker' ); expect( Array.from( mapper.markerNameToElements( 'marker' ) ) ).to.deep.equal( [ viewB ] ); + expect( Array.from( mapper.markerNameToElements( 'markerB' ) ) ).to.deep.equal( [ viewA ] ); mapper.unbindElementFromMarkerName( viewB, 'marker' ); From 0a3c2184769c5236646630b9e8f370e2c7d55f43 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 13 Aug 2019 10:58:15 +0200 Subject: [PATCH 3/6] Internal: Introduced automatic marker re-rendering during conversion for markers which view element was unbound. --- src/controller/editingcontroller.js | 2 +- src/conversion/downcastdispatcher.js | 16 ++++++++-- tests/conversion/downcastdispatcher.js | 42 +++++++++++++++++++++----- 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/src/controller/editingcontroller.js b/src/controller/editingcontroller.js index 816447aec..03f4a6eaa 100644 --- a/src/controller/editingcontroller.js +++ b/src/controller/editingcontroller.js @@ -87,7 +87,7 @@ export default class EditingController { // Also convert model selection. this.listenTo( doc, 'change', () => { this.view.change( writer => { - this.downcastDispatcher.convertChanges( doc.differ, writer ); + this.downcastDispatcher.convertChanges( doc.differ, markers, writer ); this.downcastDispatcher.convertSelection( selection, markers, writer ); } ); }, { priority: 'low' } ); diff --git a/src/conversion/downcastdispatcher.js b/src/conversion/downcastdispatcher.js index ae2f63b52..edc3505e8 100644 --- a/src/conversion/downcastdispatcher.js +++ b/src/conversion/downcastdispatcher.js @@ -109,7 +109,7 @@ export default class DowncastDispatcher { * @param {Object} [conversionApi] Additional properties for interface that will be passed to events fired * by `DowncastDispatcher`. */ - constructor( conversionApi = {} ) { + constructor( conversionApi ) { /** * Interface passed by dispatcher to the events callbacks. * @@ -122,9 +122,10 @@ export default class DowncastDispatcher { * Takes {@link module:engine/model/differ~Differ model differ} object with buffered changes and fires conversion basing on it. * * @param {module:engine/model/differ~Differ} differ Differ object with buffered changes. + * @param {module:engine/model/markercollection~MarkerCollection} markers Markers connected with converted model. * @param {module:engine/view/downcastwriter~DowncastWriter} writer View writer that should be used to modify view document. */ - convertChanges( differ, writer ) { + convertChanges( differ, markers, writer ) { // Before the view is updated, remove markers which have changed. for ( const change of differ.getMarkersToRemove() ) { this.convertMarkerRemove( change.name, change.range, writer ); @@ -142,6 +143,15 @@ export default class DowncastDispatcher { } } + for ( const markerName of this.conversionApi.mapper._unboundMarkers ) { + const markerRange = markers.get( markerName ).getRange(); + + this.convertMarkerRemove( markerName, markerRange, writer ); + this.convertMarkerAdd( markerName, markerRange, writer ); + } + + this.conversionApi.mapper._unboundMarkers.clear(); + // After the view is updated, convert markers which have changed. for ( const change of differ.getMarkersToAdd() ) { this.convertMarkerAdd( change.name, change.range, writer ); @@ -252,7 +262,7 @@ export default class DowncastDispatcher { * @fires addMarker * @fires attribute * @param {module:engine/model/selection~Selection} selection Selection to convert. - * @param {Array.} markers Array of markers containing model markers. + * @param {module:engine/model/markercollection~MarkerCollection} markers Markers connected with converted model. * @param {module:engine/view/downcastwriter~DowncastWriter} writer View writer that should be used to modify view document. */ convertSelection( selection, markers, writer ) { diff --git a/tests/conversion/downcastdispatcher.js b/tests/conversion/downcastdispatcher.js index 890aacffb..5646be95e 100644 --- a/tests/conversion/downcastdispatcher.js +++ b/tests/conversion/downcastdispatcher.js @@ -4,6 +4,8 @@ */ import DowncastDispatcher from '../../src/conversion/downcastdispatcher'; +import Mapper from '../../src/conversion/mapper'; + import Model from '../../src/model/model'; import ModelText from '../../src/model/text'; import ModelElement from '../../src/model/element'; @@ -13,13 +15,14 @@ import View from '../../src/view/view'; import ViewContainerElement from '../../src/view/containerelement'; describe( 'DowncastDispatcher', () => { - let dispatcher, doc, root, differStub, model, view; + let dispatcher, doc, root, differStub, model, view, mapper; beforeEach( () => { model = new Model(); view = new View(); doc = model.document; - dispatcher = new DowncastDispatcher(); + mapper = new Mapper(); + dispatcher = new DowncastDispatcher( { mapper } ); root = doc.createRoot(); differStub = { @@ -48,7 +51,7 @@ describe( 'DowncastDispatcher', () => { differStub.getChanges = () => [ { type: 'insert', position, length: 1 } ]; view.change( writer => { - dispatcher.convertChanges( differStub, writer ); + dispatcher.convertChanges( differStub, model.markers, writer ); } ); expect( dispatcher.convertInsert.calledOnce ).to.be.true; @@ -63,7 +66,7 @@ describe( 'DowncastDispatcher', () => { differStub.getChanges = () => [ { type: 'remove', position, length: 2, name: '$text' } ]; view.change( writer => { - dispatcher.convertChanges( differStub, writer ); + dispatcher.convertChanges( differStub, model.markers, writer ); } ); expect( dispatcher.convertRemove.calledWith( position, 2, '$text' ) ).to.be.true; @@ -80,7 +83,7 @@ describe( 'DowncastDispatcher', () => { ]; view.change( writer => { - dispatcher.convertChanges( differStub, writer ); + dispatcher.convertChanges( differStub, model.markers, writer ); } ); expect( dispatcher.convertAttribute.calledWith( range, 'key', null, 'foo' ) ).to.be.true; @@ -102,7 +105,7 @@ describe( 'DowncastDispatcher', () => { ]; view.change( writer => { - dispatcher.convertChanges( differStub, writer ); + dispatcher.convertChanges( differStub, model.markers, writer ); } ); expect( dispatcher.convertInsert.calledTwice ).to.be.true; @@ -122,7 +125,7 @@ describe( 'DowncastDispatcher', () => { ]; view.change( writer => { - dispatcher.convertChanges( differStub, writer ); + dispatcher.convertChanges( differStub, model.markers, writer ); } ); expect( dispatcher.convertMarkerAdd.calledWith( 'foo', fooRange ) ); @@ -141,11 +144,34 @@ describe( 'DowncastDispatcher', () => { ]; view.change( writer => { - dispatcher.convertChanges( differStub, writer ); + dispatcher.convertChanges( differStub, model.markers, writer ); + } ); + + expect( dispatcher.convertMarkerRemove.calledWith( 'foo', fooRange ) ); + expect( dispatcher.convertMarkerRemove.calledWith( 'bar', barRange ) ); + } ); + + it( 'should re-render markers which view elements got unbound during conversion', () => { + sinon.stub( dispatcher, 'convertMarkerRemove' ); + sinon.stub( dispatcher, 'convertMarkerAdd' ); + + const fooRange = model.createRange( model.createPositionAt( root, 0 ), model.createPositionAt( root, 1 ) ); + const barRange = model.createRange( model.createPositionAt( root, 3 ), model.createPositionAt( root, 6 ) ); + + model.markers._set( 'foo', fooRange ); + model.markers._set( 'bar', barRange ); + + // Stub `_unboundMarkers`. + dispatcher.conversionApi.mapper._unboundMarkers = new Set( [ 'foo', 'bar' ] ); + + view.change( writer => { + dispatcher.convertChanges( differStub, model.markers, writer ); } ); expect( dispatcher.convertMarkerRemove.calledWith( 'foo', fooRange ) ); expect( dispatcher.convertMarkerRemove.calledWith( 'bar', barRange ) ); + expect( dispatcher.convertMarkerAdd.calledWith( 'foo', fooRange ) ); + expect( dispatcher.convertMarkerAdd.calledWith( 'bar', barRange ) ); } ); } ); From 509c254fb659f9c6644978bdf5b431fceccf328a Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 13 Aug 2019 13:02:40 +0200 Subject: [PATCH 4/6] Docs: Fix in docs. --- src/conversion/downcastdispatcher.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conversion/downcastdispatcher.js b/src/conversion/downcastdispatcher.js index edc3505e8..fb3dddaf5 100644 --- a/src/conversion/downcastdispatcher.js +++ b/src/conversion/downcastdispatcher.js @@ -106,7 +106,7 @@ export default class DowncastDispatcher { * Creates a `DowncastDispatcher` instance. * * @see module:engine/conversion/downcastdispatcher~DowncastConversionApi - * @param {Object} [conversionApi] Additional properties for interface that will be passed to events fired + * @param {Object} conversionApi Additional properties for interface that will be passed to events fired * by `DowncastDispatcher`. */ constructor( conversionApi ) { From 4eb6210c19b9410ebff90b09c87749736370b744 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 13 Aug 2019 15:28:44 +0200 Subject: [PATCH 5/6] Fix: Fixed `Mapper` crashing in scenario where attribute element bound with marker is broken. --- src/conversion/mapper.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/conversion/mapper.js b/src/conversion/mapper.js index 703f0e23e..0dc713f06 100644 --- a/src/conversion/mapper.js +++ b/src/conversion/mapper.js @@ -222,10 +222,12 @@ export default class Mapper { const names = this._elementToMarkerNames.get( element ); - names.delete( name ); + if ( names ) { + names.delete( name ); - if ( names.size == 0 ) { - this._elementToMarkerNames.delete( element ); + if ( names.size == 0 ) { + this._elementToMarkerNames.delete( element ); + } } } From d2894d5fa5d1aa6482beb8fc9c401927848b27de Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 16 Aug 2019 14:34:44 +0200 Subject: [PATCH 6/6] Feature: Introduced `conversion.Mapper#flushUnboundMarkers`. --- src/conversion/downcastdispatcher.js | 4 +--- src/conversion/mapper.js | 26 ++++++++++++++++++++------ tests/conversion/mapper.js | 25 ++++++++++++++++++++++--- 3 files changed, 43 insertions(+), 12 deletions(-) diff --git a/src/conversion/downcastdispatcher.js b/src/conversion/downcastdispatcher.js index fb3dddaf5..ee45d46b3 100644 --- a/src/conversion/downcastdispatcher.js +++ b/src/conversion/downcastdispatcher.js @@ -143,15 +143,13 @@ export default class DowncastDispatcher { } } - for ( const markerName of this.conversionApi.mapper._unboundMarkers ) { + for ( const markerName of this.conversionApi.mapper.flushUnboundMarkerNames() ) { const markerRange = markers.get( markerName ).getRange(); this.convertMarkerRemove( markerName, markerRange, writer ); this.convertMarkerAdd( markerName, markerRange, writer ); } - this.conversionApi.mapper._unboundMarkers.clear(); - // After the view is updated, convert markers which have changed. for ( const change of differ.getMarkersToAdd() ) { this.convertMarkerAdd( change.name, change.range, writer ); diff --git a/src/conversion/mapper.js b/src/conversion/mapper.js index 0dc713f06..7dba3bf94 100644 --- a/src/conversion/mapper.js +++ b/src/conversion/mapper.js @@ -88,13 +88,13 @@ export default class Mapper { this._elementToMarkerNames = new Map(); /** - * Stores markers which has changed due to unbinding a view element (so it is assumed that the view element has been removed, - * moved or renamed). In some cases such markers need to be refreshed (re-rendered). + * Stores marker names of markers which has changed due to unbinding a view element (so it is assumed that the view element + * has been removed, moved or renamed). * - * @protected + * @private * @member {Set.} */ - this._unboundMarkers = new Set(); + this._unboundMarkerNames = new Set(); // Default mapper algorithm for mapping model position to view position. this.on( 'modelToViewPosition', ( evt, data ) => { @@ -153,7 +153,7 @@ export default class Mapper { if ( this._elementToMarkerNames.has( viewElement ) ) { for ( const markerName of this._elementToMarkerNames.get( viewElement ) ) { - this._unboundMarkers.add( markerName ); + this._unboundMarkerNames.add( markerName ); } } @@ -231,6 +231,20 @@ export default class Mapper { } } + /** + * Returns all marker names of markers which has changed due to unbinding a view element (so it is assumed that the view element + * has been removed, moved or renamed) since the last flush. After returning, the marker names list is cleared. + * + * @returns {Array.} + */ + flushUnboundMarkerNames() { + const markerNames = Array.from( this._unboundMarkerNames ); + + this._unboundMarkerNames.clear(); + + return markerNames; + } + /** * Removes all model to view and view to model bindings. */ @@ -239,7 +253,7 @@ export default class Mapper { this._viewToModelMapping = new WeakMap(); this._markerNameToElements = new Map(); this._elementToMarkerNames = new Map(); - this._unboundMarkers = new Set(); + this._unboundMarkerNames = new Set(); } /** diff --git a/tests/conversion/mapper.js b/tests/conversion/mapper.js index d667d9882..4aa252619 100644 --- a/tests/conversion/mapper.js +++ b/tests/conversion/mapper.js @@ -52,8 +52,6 @@ describe( 'Mapper', () => { expect( Array.from( mapper.markerNameToElements( 'foo' ) ) ).to.deep.equal( [ viewA, viewD ] ); mapper.unbindViewElement( viewD ); - expect( Array.from( mapper._unboundMarkers ) ).to.deep.equal( [ 'foo', 'bar' ] ); - mapper.clearBindings(); expect( mapper.toModelElement( viewA ) ).to.be.undefined; @@ -65,7 +63,7 @@ describe( 'Mapper', () => { expect( mapper.toViewElement( modelC ) ).to.be.undefined; expect( mapper.markerNameToElements( 'foo' ) ).to.be.null; - expect( Array.from( mapper._unboundMarkers ) ).to.deep.equal( [] ); + expect( mapper.flushUnboundMarkerNames() ).to.deep.equal( [] ); } ); } ); @@ -784,4 +782,25 @@ describe( 'Mapper', () => { expect( viewMappedAncestor ).to.equal( viewP ); } ); } ); + + describe( 'flushUnboundMarkerNames()', () => { + it( 'should return marker names of markers which elements has been unbound and clear that list', () => { + const viewA = new ViewElement( 'a' ); + const viewB = new ViewElement( 'b' ); + + const mapper = new Mapper(); + + mapper.bindElementToMarker( viewA, 'foo' ); + mapper.bindElementToMarker( viewA, 'bar' ); + mapper.bindElementToMarker( viewB, 'bar' ); + + expect( mapper.flushUnboundMarkerNames() ).to.deep.equal( [ 'foo', 'bar' ] ); + expect( mapper.flushUnboundMarkerNames() ).to.deep.equal( [] ); + + mapper.bindElementToMarker( viewA, 'foo' ); + + expect( mapper.flushUnboundMarkerNames() ).to.deep.equal( [ 'foo' ] ); + expect( mapper.flushUnboundMarkerNames() ).to.deep.equal( [] ); + } ); + } ); } );