Merge pull request #1206 from ckeditor/t/1172-b

Other: Refactoring: Conversion refactoring. Introduced `model.Differ`. Changes now will be converted after all changes in a change block are done. Closes #1172.

Author: Piotr Jasiun

src/controller/datacontroller.js

@@ -167,7 +167,7 @@ export default class DataController {
 		const viewDocumentFragment = new ViewDocumentFragment();
 		this.mapper.bindElements( modelElementOrFragment, viewDocumentFragment );
 
-		this.modelToView.convertInsertion( modelRange );
+		this.modelToView.convertInsert( modelRange );
 
 		this.mapper.clearBindings();
 

src/controller/editingcontroller.js

@@ -7,6 +7,7 @@
  * @module engine/controller/editingcontroller
  */
 
+import ModelDiffer from '../model/differ';
 import ViewDocument from '../view/document';
 import Mapper from '../conversion/mapper';
 import ModelConversionDispatcher from '../conversion/modelconversiondispatcher';
@@ -64,10 +65,9 @@ export default class EditingController {
 		this.mapper = new Mapper();
 
 		/**
-		 * Model to view conversion dispatcher, which converts changes from the model to
-		 * {@link #view editing view}.
+		 * Model to view conversion dispatcher, which converts changes from the model to {@link #view the editing view}.
 		 *
-		 * To attach model to view converter to the editing pipeline you need to add lister to this property:
+		 * To attach model-to-view converter to the editing pipeline you need to add a listener to this dispatcher:
 		 *
 		 *		editing.modelToView( 'insert:$element', customInsertConverter );
 		 *
@@ -83,36 +83,60 @@ export default class EditingController {
 			viewSelection: this.view.selection
 		} );
 
-		// Convert changes in model to view.
-		this.listenTo( this.model.document, 'change', ( evt, type, changes ) => {
-			this.modelToView.convertChange( type, changes );
-		}, { priority: 'low' } );
+		// Model differ object. It's role is to buffer changes done on model and then calculates a diff of those changes.
+		// The diff is then passed to model conversion dispatcher which generates proper events and kicks-off conversion.
+		const modelDiffer = new ModelDiffer();
 
-		// Convert model selection to view.
-		this.listenTo( this.model.document, 'changesDone', () => {
-			const selection = this.model.document.selection;
+		// Before an operation is applied on model, buffer the change in differ.
+		this.listenTo( this.model, 'applyOperation', ( evt, args ) => {
+			const operation = args[ 0 ];
 
-			this.modelToView.convertSelection( selection );
-			this.view.render();
-		}, { priority: 'low' } );
+			if ( operation.isDocumentOperation ) {
+				modelDiffer.bufferOperation( operation );
+			}
+		}, { priority: 'high' } );
 
-		// Convert model markers changes.
+		// Buffer marker changes.
+		// This is not covered in buffering operations because markers may change outside of them (when they
+		// are modified using `model.document.markers` collection, not through `MarkerOperation`).
 		this.listenTo( this.model.markers, 'add', ( evt, marker ) => {
-			this.modelToView.convertMarker( 'addMarker', marker.name, marker.getRange() );
+			// Whenever a new marker is added, buffer that change.
+			modelDiffer.bufferMarkerChange( marker.name, null, marker.getRange() );
+
+			// Whenever marker changes, buffer that.
+			marker.on( 'change', ( evt, oldRange ) => {
+				modelDiffer.bufferMarkerChange( marker.name, oldRange, marker.getRange() );
+			} );
 		} );
 
 		this.listenTo( this.model.markers, 'remove', ( evt, marker ) => {
-			this.modelToView.convertMarker( 'removeMarker', marker.name, marker.getRange() );
+			// Whenever marker is removed, buffer that change.
+			modelDiffer.bufferMarkerChange( marker.name, marker.getRange(), null );
 		} );
 
-		// Convert view selection to model.
+		// When all changes are done, get the model diff containing all the changes and convert them to view and then render to DOM.
+		this.listenTo( this.model, 'changesDone', () => {
+			// Convert changes stored in `modelDiffer`.
+			this.modelToView.convertChanges( modelDiffer );
+
+			// Reset model diff object. When next operation is applied, new diff will be created.
+			modelDiffer.reset();
+
+			// After the view is ready, convert selection from model to view.
+			this.modelToView.convertSelection( this.model.document.selection );
+
+			// When everything is converted to the view, render it to DOM.
+			this.view.render();
+		}, { priority: 'low' } );
+
+		// Convert selection from view to model.
 		this.listenTo( this.view, 'selectionChange', convertSelectionChange( this.model, this.mapper ) );
 
-		// Attach default content converters.
+		// Attach default model converters.
 		this.modelToView.on( 'insert:$text', insertText(), { priority: 'lowest' } );
 		this.modelToView.on( 'remove', remove(), { priority: 'low' } );
 
-		// Attach default selection converters.
+		// Attach default model selection converters.
 		this.modelToView.on( 'selection', clearAttributes(), { priority: 'low' } );
 		this.modelToView.on( 'selection', clearFakeSelection(), { priority: 'low' } );
 		this.modelToView.on( 'selection', convertRangeSelection(), { priority: 'low' } );

src/conversion/buildmodelconverter.js

@@ -10,13 +10,12 @@
 import {
 	insertElement,
 	insertUIElement,
-	setAttribute,
-	removeAttribute,
 	removeUIElement,
-	wrapItem,
-	unwrapItem,
+	changeAttribute,
+	wrap,
 	highlightText,
-	highlightElement
+	highlightElement,
+	removeHighlight
 } from './model-to-view-converters';
 
 import { convertSelectionAttribute, convertSelectionMarker } from './model-selection-to-view-converters';
@@ -254,15 +253,13 @@ class ModelConverterBuilder {
 
 				dispatcher.on( 'insert:' + this._from.name, insertElement( element ), { priority } );
 			} else if ( this._from.type == 'attribute' ) {
-				// From model attribute to view element -> wrap and unwrap.
+				// From model attribute to view element -> wrap.
 				element = typeof element == 'string' ? new ViewAttributeElement( element ) : element;
 
-				dispatcher.on( 'addAttribute:' + this._from.key, wrapItem( element ), { priority } );
-				dispatcher.on( 'changeAttribute:' + this._from.key, wrapItem( element ), { priority } );
-				dispatcher.on( 'removeAttribute:' + this._from.key, unwrapItem( element ), { priority } );
-
+				dispatcher.on( 'attribute:' + this._from.key, wrap( element ), { priority } );
 				dispatcher.on( 'selectionAttribute:' + this._from.key, convertSelectionAttribute( element ), { priority } );
-			} else { // From marker to element.
+			} else {
+				// From marker to element.
 				const priority = this._from.priority === null ? 'normal' : this._from.priority;
 
 				element = typeof element == 'string' ? new ViewUIElement( element ) : element;
@@ -326,12 +323,10 @@ class ModelConverterBuilder {
 		}
 
 		for ( const dispatcher of this._dispatchers ) {
-			// Separate converters for converting texts and elements inside marker's range.
 			dispatcher.on( 'addMarker:' + this._from.name, highlightText( highlightDescriptor ), { priority } );
 			dispatcher.on( 'addMarker:' + this._from.name, highlightElement( highlightDescriptor ), { priority } );
 
-			dispatcher.on( 'removeMarker:' + this._from.name, highlightText( highlightDescriptor ), { priority } );
-			dispatcher.on( 'removeMarker:' + this._from.name, highlightElement( highlightDescriptor ), { priority } );
+			dispatcher.on( 'removeMarker:' + this._from.name, removeHighlight( highlightDescriptor ), { priority } );
 
 			dispatcher.on( 'selectionMarker:' + this._from.name, convertSelectionMarker( highlightDescriptor ), { priority } );
 		}
@@ -383,7 +378,7 @@ class ModelConverterBuilder {
 
 		if ( !keyOrCreator ) {
 			// If `keyOrCreator` is not set, we assume default behavior which is 1:1 attribute re-write.
-			// This is also a default behavior for `setAttribute` converter when no attribute creator is passed.
+			// This is also a default behavior for `changeAttribute` converter when no attribute creator is passed.
 			attributeCreator = undefined;
 		} else if ( typeof keyOrCreator == 'string' ) {
 			// `keyOrCreator` is an attribute key.
@@ -407,9 +402,7 @@ class ModelConverterBuilder {
 		for ( const dispatcher of this._dispatchers ) {
 			const options = { priority: this._from.priority || 'normal' };
 
-			dispatcher.on( 'addAttribute:' + this._from.key, setAttribute( attributeCreator ), options );
-			dispatcher.on( 'changeAttribute:' + this._from.key, setAttribute( attributeCreator ), options );
-			dispatcher.on( 'removeAttribute:' + this._from.key, removeAttribute( attributeCreator ), options );
+			dispatcher.on( 'attribute:' + this._from.key, changeAttribute( attributeCreator ), options );
 		}
 	}
 }

src/conversion/mapper.js

@@ -110,23 +110,43 @@ export default class Mapper {
 	/**
 	 * Unbinds given {@link module:engine/view/element~Element view element} from the map.
 	 *
+	 * **Note:** view-to-model binding will be removed, if it existed. However, corresponding model-to-view binding
+	 * will be removed only if model element is still bound to passed `viewElement`.
+	 *
+	 * This behavior lets for re-binding model element to another view element without fear of losing the new binding
+	 * when the previously bound view element is unbound.
+	 *
 	 * @param {module:engine/view/element~Element} viewElement View element to unbind.
 	 */
 	unbindViewElement( viewElement ) {
 		const modelElement = this.toModelElement( viewElement );
 
-		this._unbindElements( modelElement, viewElement );
+		this._viewToModelMapping.delete( viewElement );
+
+		if ( this._modelToViewMapping.get( modelElement ) == viewElement ) {
+			this._modelToViewMapping.delete( modelElement );
+		}
 	}
 
 	/**
 	 * Unbinds given {@link module:engine/model/element~Element model element} from the map.
 	 *
+	 * **Note:** model-to-view binding will be removed, if it existed. However, corresponding view-to-model binding
+	 * will be removed only if view element is still bound to passed `modelElement`.
+	 *
+	 * This behavior lets for re-binding view element to another model element without fear of losing the new binding
+	 * when the previously bound model element is unbound.
+	 *
 	 * @param {module:engine/model/element~Element} modelElement Model element to unbind.
 	 */
 	unbindModelElement( modelElement ) {
 		const viewElement = this.toViewElement( modelElement );
 
-		this._unbindElements( modelElement, viewElement );
+		this._modelToViewMapping.delete( modelElement );
+
+		if ( this._viewToModelMapping.get( viewElement ) == modelElement ) {
+			this._viewToModelMapping.delete( viewElement );
+		}
 	}
 
 	/**
@@ -202,12 +222,16 @@ export default class Mapper {
 	 *
 	 * @fires modelToViewPosition
 	 * @param {module:engine/model/position~Position} modelPosition Model position.
+	 * @param {Object} [options] Additional options for position mapping process.
+	 * @param {Boolean} [options.isPhantom=false] Should be set to `true` if the model position to map is pointing to a place
+	 * in model tree which no longer exists. For example, it could be an end of a removed model range.
 	 * @returns {module:engine/view/position~Position} Corresponding view position.
 	 */
-	toViewPosition( modelPosition ) {
+	toViewPosition( modelPosition, options = { isPhantom: false } ) {
 		const data = {
 			modelPosition,
-			mapper: this
+			mapper: this,
+			isPhantom: options.isPhantom
 		};
 
 		this.fire( 'modelToViewPosition', data );
@@ -292,18 +316,6 @@ export default class Mapper {
 		return modelOffset;
 	}
 
-	/**
-	 * Removes binding between given elements.
-	 *
-	 * @private
-	 * @param {module:engine/model/element~Element} modelElement Model element to unbind.
-	 * @param {module:engine/view/element~Element} viewElement View element to unbind.
-	 */
-	_unbindElements( modelElement, viewElement ) {
-		this._viewToModelMapping.delete( viewElement );
-		this._modelToViewMapping.delete( modelElement );
-	}
-
 	/**
 	 * Gets the length of the view element in the model.
 	 *
@@ -460,19 +472,33 @@ export default class Mapper {
 	 *			}
 	 *		} );
 	 *
-	 * **Note:** keep in mind that custom callback provided for this event should use provided `data.modelPosition` only to check
-	 * what is before the position (or position's parent). This is important when model-to-view position mapping is used in
-	 * remove change conversion. Model after the removed position (that is being mapped) does not correspond to view, so it cannot be used:
+	 * **Note:** keep in mind that sometimes a "phantom" model position is being converted. "Phantom" model position is
+	 * a position that points to a non-existing place in model. Such position might still be valid for conversion, though
+	 * (it would point to a correct place in view when converted). One example of such situation is when a range is
+	 * removed from model, there may be a need to map the range's end (which is no longer valid model position). To
+	 * handle such situation, check `data.isPhantom` flag:
+	 *
+	 * 		// Assume that there is "customElement" model element and whenever position is before it, we want to move it
+	 * 		// to the inside of the view element bound to "customElement".
+	 *		mapper.on( 'modelToViewPosition', ( evt, data ) => {
+	 *			if ( data.isPhantom ) {
+	 *				return;
+	 *			}
 	 *
-	 *		// Incorrect:
-	 *		const modelElement = data.modelPosition.nodeAfter;
-	 *		const viewElement = data.mapper.toViewElement( modelElement );
-	 *		// ... Do something with `viewElement` and set `data.viewPosition`.
+	 *			// Below line might crash for phantom position that does not exist in model.
+	 *			const sibling = data.modelPosition.nodeBefore;
+	 *
+	 *			// Check if this is the element we are interested in.
+	 *			if ( !sibling.is( 'customElement' ) ) {
+	 *				return;
+	 *			}
 	 *
-	 *		// Correct:
-	 *		const prevModelElement = data.modelPosition.nodeBefore;
-	 *		const prevViewElement = data.mapper.toViewElement( prevModelElement );
-	 *		// ... Use `prevViewElement` to find correct `data.viewPosition`.
+	 *			const viewElement = data.mapper.toViewElement( sibling );
+	 *
+	 *			data.viewPosition = new ViewPosition( sibling, 0 );
+	 *
+	 *			evt.stop();
+	 *		} );
 	 *
 	 * **Note:** default mapping callback is provided with `low` priority setting and does not cancel the event, so it is possible to
 	 * attach a custom callback after default callback and also use `data.viewPosition` calculated by default callback