Merge pull request #1781 from ckeditor/t/1780

Other: Introduced automatic marker re-rendering during conversion for markers which view element was unbound. Closes #1780.

BREAKING CHANGE: New parameter introduced in `DowncastDispatcher#convertChanges()`. Now it is `convertChanges( differ, markers, writer )`.
BREAKING CHANGE: Although it was rather impossible to use `DowncastDispatcher` without specifying any conversion API in the constructor, now it is a required parameter.

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' } );

src/conversion/downcastdispatcher.js

@@ -106,10 +106,10 @@ 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 = {} ) {
+	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,13 @@ export default class DowncastDispatcher {
 			}
 		}
 
+		for ( const markerName of this.conversionApi.mapper.flushUnboundMarkerNames() ) {
+			const markerRange = markers.get( markerName ).getRange();
+
+			this.convertMarkerRemove( markerName, markerRange, writer );
+			this.convertMarkerAdd( markerName, markerRange, writer );
+		}
+
 		// 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 +260,7 @@ export default class DowncastDispatcher {
 	 * @fires addMarker
 	 * @fires attribute
 	 * @param {module:engine/model/selection~Selection} selection Selection to convert.
-	 * @param {Array.<module:engine/model/markercollection~Marker>} 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 ) {

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 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).
+		 *
+		 * @private
+		 * @member {Set.<module:engine/model/markercollection~Marker>}
+		 */
+		this._unboundMarkerNames = 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._unboundMarkerNames.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 );
 	}
 
 	/**
@@ -180,15 +208,39 @@ export default class Mapper {
 	 * @param {String} name Marker name.
 	 */
 	unbindElementFromMarkerName( element, name ) {
-		const elements = this._markerNameToElements.get( name );
+		const nameToElements = this._markerNameToElements.get( name );
 
-		if ( elements ) {
-			elements.delete( element );
+		if ( nameToElements ) {
+			nameToElements.delete( element );
 
-			if ( elements.size == 0 ) {
+			if ( nameToElements.size == 0 ) {
 				this._markerNameToElements.delete( name );
 			}
 		}
+
+		const elementToNames = this._elementToMarkerNames.get( element );
+
+		if ( elementToNames ) {
+			elementToNames.delete( name );
+
+			if ( elementToNames.size == 0 ) {
+				this._elementToMarkerNames.delete( element );
+			}
+		}
+	}
+
+	/**
+	 * 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.<String>}
+	 */
+	flushUnboundMarkerNames() {
+		const markerNames = Array.from( this._unboundMarkerNames );
+
+		this._unboundMarkerNames.clear();
+
+		return markerNames;
 	}
 
 	/**
@@ -198,6 +250,8 @@ export default class Mapper {
 		this._modelToViewMapping = new WeakMap();
 		this._viewToModelMapping = new WeakMap();
 		this._markerNameToElements = new Map();
+		this._elementToMarkerNames = new Map();
+		this._unboundMarkerNames = new Set();
 	}
 
 	/**

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.

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 `Mapper#flushUnboundMarkerNames`.
+			dispatcher.conversionApi.mapper.flushUnboundMarkerNames = () => [ '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 ) );
 		} );
 	} );
 

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,9 @@ 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 );
 			mapper.clearBindings();
 
 			expect( mapper.toModelElement( viewA ) ).to.be.undefined;
@@ -50,6 +61,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( mapper.flushUnboundMarkerNames() ).to.deep.equal( [] );
 		} );
 	} );
 
@@ -648,11 +662,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' );
 
@@ -751,4 +767,27 @@ 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' );
+
+			mapper.unbindViewElement( viewA );
+
+			expect( mapper.flushUnboundMarkerNames() ).to.deep.equal( [ 'foo', 'bar' ] );
+			expect( mapper.flushUnboundMarkerNames() ).to.deep.equal( [] );
+
+			mapper.unbindViewElement( viewB );
+
+			expect( mapper.flushUnboundMarkerNames() ).to.deep.equal( [ 'bar' ] );
+			expect( mapper.flushUnboundMarkerNames() ).to.deep.equal( [] );
+		} );
+	} );
 } );