Merge pull request #1220 from ckeditor/t/1211

Other: Operations that do not operate on a document should have `baseVersion` set to `null`. Closes #1211.
Fixed: Markers again are properly converted in `engine.controller.DataController`.
Fixed: Markers are cleared now before an operation is applied to `model.Document` tree to fix scenarios where marker range could not be converted to the view after the model changed.

Author: Piotr Jasiun

src/controller/datacontroller.js

@@ -135,10 +135,10 @@ export default class DataController {
 	 * @returns {String} Output data.
 	 */
 	stringify( modelElementOrFragment ) {
-		// model -> view
+		// Model -> view.
 		const viewDocumentFragment = this.toView( modelElementOrFragment );
 
-		// view -> data
+		// View -> data.
 		return this.processor.toData( viewDocumentFragment );
 	}
 
@@ -153,13 +153,25 @@ export default class DataController {
 	 * @returns {module:engine/view/documentfragment~DocumentFragment} Output view DocumentFragment.
 	 */
 	toView( modelElementOrFragment ) {
+		// First, convert elements.
 		const modelRange = ModelRange.createIn( modelElementOrFragment );
 
 		const viewDocumentFragment = new ViewDocumentFragment();
 		this.mapper.bindElements( modelElementOrFragment, viewDocumentFragment );
 
 		this.modelToView.convertInsert( modelRange );
 
+		if ( !modelElementOrFragment.is( 'documentFragment' ) ) {
+			// Then, if a document element is converted, convert markers.
+			// From all document markers, get those, which "intersect" with the converter element.
+			const markers = _getMarkersRelativeToElement( modelElementOrFragment );
+
+			for ( const [ name, range ] of markers ) {
+				this.modelToView.convertMarkerAdd( name, range );
+			}
+		}
+
+		// Clear bindings so the next call to this method gives correct results.
 		this.mapper.clearBindings();
 
 		return viewDocumentFragment;
@@ -234,3 +246,28 @@ export default class DataController {
 }
 
 mix( DataController, ObservableMixin );
+
+// Helper function for converting part of a model to view.
+//
+// Takes a document element (element that is added to a model document) and checks which markers are inside it
+// and which markers are containing it. If the marker is intersecting with element, the intersection is returned.
+function _getMarkersRelativeToElement( element ) {
+	const result = [];
+	const doc = element.root.document;
+
+	if ( !doc ) {
+		return [];
+	}
+
+	const elementRange = ModelRange.createIn( element );
+
+	for ( const marker of doc.model.markers ) {
+		const intersection = elementRange.getIntersection( marker.getRange() );
+
+		if ( intersection ) {
+			result.push( [ marker.name, intersection ] );
+		}
+	}
+
+	return result;
+}

src/controller/editingcontroller.js

@@ -85,15 +85,12 @@ export default class EditingController {
 
 		const doc = this.model.document;
 
-		// 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( doc, 'change', () => {
-			// Convert changes stored in `modelDiffer`.
 			this.modelToView.convertChanges( doc.differ );
+		}, { priority: 'low' } );
 
-			// After the view is ready, convert selection from model to view.
+		this.listenTo( model, '_change', () => {
 			this.modelToView.convertSelection( doc.selection );
-
-			// When everything is converted to the view, render it to DOM.
 			this.view.render();
 		}, { priority: 'low' } );
 
@@ -110,6 +107,51 @@ export default class EditingController {
 		this.modelToView.on( 'selection', convertRangeSelection(), { priority: 'low' } );
 		this.modelToView.on( 'selection', convertCollapsedSelection(), { priority: 'low' } );
 
+		// Convert markers removal.
+		//
+		// Markers should be removed from the view before changes to the model are applied. This is because otherwise
+		// it would be impossible to map some markers to the view (if, for example, the marker's boundary parent got removed).
+		//
+		// `removedMarkers` keeps information which markers already has been removed to prevent removing them twice.
+		const removedMarkers = new Set();
+
+		this.listenTo( model, 'applyOperation', ( evt, args ) => {
+			// Before operation is applied...
+			const operation = args[ 0 ];
+
+			for ( const marker of model.markers ) {
+				// Check all markers, that aren't already removed...
+				if ( removedMarkers.has( marker.name ) ) {
+					continue;
+				}
+
+				const markerRange = marker.getRange();
+
+				if ( _operationAffectsMarker( operation, marker ) ) {
+					// And if the operation in any way modifies the marker, remove the marker from the view.
+					removedMarkers.add( marker.name );
+					this.modelToView.convertMarkerRemove( marker.name, markerRange );
+
+					// TODO: This stinks but this is the safest place to have this code.
+					this.model.document.differ.bufferMarkerChange( marker.name, markerRange, markerRange );
+				}
+			}
+		}, { priority: 'high' } );
+
+		// If a marker is removed through `model.Model#markers` directly (not through operation), just remove it (if
+		// it was not removed already).
+		this.listenTo( model.markers, 'remove', ( evt, marker ) => {
+			if ( !removedMarkers.has( marker.name ) ) {
+				removedMarkers.add( marker.name );
+				this.modelToView.convertMarkerRemove( marker.name, marker.getRange() );
+			}
+		} );
+
+		// When all changes are done, clear `removedMarkers` set.
+		this.listenTo( model, '_change', () => {
+			removedMarkers.clear();
+		}, { priority: 'low' } );
+
 		// Binds {@link module:engine/view/document~Document#roots view roots collection} to
 		// {@link module:engine/model/document~Document#roots model roots collection} so creating
 		// model root automatically creates corresponding view root.
@@ -140,3 +182,23 @@ export default class EditingController {
 }
 
 mix( EditingController, ObservableMixin );
+
+// Helper function which checks whether given operation will affect given marker after the operation is applied.
+function _operationAffectsMarker( operation, marker ) {
+	const range = marker.getRange();
+
+	if ( operation.type == 'insert' || operation.type == 'rename' ) {
+		return _positionAffectsRange( operation.position, range );
+	} else if ( operation.type == 'move' || operation.type == 'remove' || operation.type == 'reinsert' ) {
+		return _positionAffectsRange( operation.targetPosition, range ) || _positionAffectsRange( operation.sourcePosition, range );
+	} else if ( operation.type == 'marker' && operation.name == marker.name ) {
+		return true;
+	}
+
+	return false;
+}
+
+// Helper function which checks whether change at given position affects given range.
+function _positionAffectsRange( position, range ) {
+	return range.containsPosition( position ) || !range.start._getTransformedByInsertion( position, 1, true ).isEqual( range.start );
+}

src/conversion/modelconversiondispatcher.js

@@ -128,11 +128,6 @@ export default class ModelConversionDispatcher {
 	 * @param {module:engine/model/differ~Differ} differ Differ object with buffered changes.
 	 */
 	convertChanges( differ ) {
-		// First, before changing view structure, remove all markers that has changed.
-		for ( const change of differ.getMarkersToRemove() ) {
-			this.convertMarkerRemove( change.name, change.range );
-		}
-
 		// Convert changes that happened on model tree.
 		for ( const entry of differ.getChanges() ) {
 			if ( entry.type == 'insert' ) {

src/model/batch.js

@@ -50,14 +50,20 @@ export default class Batch {
 	}
 
 	/**
-	 * Returns this batch base version, which is equal to the base version of first delta in the batch.
-	 * If there are no deltas in the batch, it returns `null`.
+	 * Returns this batch base version, which is equal to the base version of first delta (which has base version set)
+	 * in the batch. If there are no deltas in the batch or neither delta has base version set, it returns `null`.
 	 *
 	 * @readonly
 	 * @type {Number|null}
 	 */
 	get baseVersion() {
-		return this.deltas.length > 0 ? this.deltas[ 0 ].baseVersion : null;
+		for ( const delta of this.deltas ) {
+			if ( delta.baseVersion !== null ) {
+				return delta.baseVersion;
+			}
+		}
+
+		return null;
 	}
 
 	/**

src/model/operation/attributeoperation.js

@@ -37,7 +37,8 @@ export default class AttributeOperation extends Operation {
 	 * @param {String} key Key of an attribute to change or remove.
 	 * @param {*} oldValue Old value of the attribute with given key or `null`, if attribute was not set before.
 	 * @param {*} newValue New value of the attribute with given key or `null`, if operation should remove attribute.
-	 * @param {Number} baseVersion {@link module:engine/model/document~Document#version} on which the operation can be applied.
+	 * @param {Number|null} baseVersion Document {@link module:engine/model/document~Document#version} on which operation
+	 * can be applied or `null` if the operation operates on detached (non-document) tree.
 	 */
 	constructor( range, key, oldValue, newValue, baseVersion ) {
 		super( baseVersion );
@@ -73,11 +74,6 @@ export default class AttributeOperation extends Operation {
 		 * @member {*}
 		 */
 		this.newValue = newValue === undefined ? null : newValue;
-
-		/**
-		 * @inheritDoc
-		 */
-		this.isDocumentOperation = !!this.range.root.document;
 	}
 
 	/**

src/model/operation/detachoperation.js

@@ -27,10 +27,9 @@ export default class DetachOperation extends Operation {
 	 * Position before the first {@link module:engine/model/item~Item model item} to move.
 	 * @param {Number} howMany Offset size of moved range. Moved range will start from `sourcePosition` and end at
 	 * `sourcePosition` with offset shifted by `howMany`.
-	 * @param {Number} baseVersion {@link module:engine/model/document~Document#version} on which operation can be applied.
 	 */
-	constructor( sourcePosition, howMany, baseVersion ) {
-		super( baseVersion );
+	constructor( sourcePosition, howMany ) {
+		super( null );
 
 		/**
 		 * Position before the first {@link module:engine/model/item~Item model item} to detach.
@@ -45,11 +44,6 @@ export default class DetachOperation extends Operation {
 		 * @member {Number} #howMany
 		 */
 		this.howMany = howMany;
-
-		/**
-		 * @inheritDoc
-		 */
-		this.isDocumentOperation = false;
 	}
 
 	/**

src/model/operation/insertoperation.js

@@ -27,7 +27,8 @@ export default class InsertOperation extends Operation {
 	 *
 	 * @param {module:engine/model/position~Position} position Position of insertion.
 	 * @param {module:engine/model/node~NodeSet} nodes The list of nodes to be inserted.
-	 * @param {Number} baseVersion {@link module:engine/model/document~Document#version} on which operation can be applied.
+	 * @param {Number|null} baseVersion Document {@link module:engine/model/document~Document#version} on which operation
+	 * can be applied or `null` if the operation operates on detached (non-document) tree.
 	 */
 	constructor( position, nodes, baseVersion ) {
 		super( baseVersion );
@@ -47,11 +48,6 @@ export default class InsertOperation extends Operation {
 		 * @member {module:engine/model/nodelist~NodeList} module:engine/model/operation/insertoperation~InsertOperation#nodeList
 		 */
 		this.nodes = new NodeList( _normalizeNodes( nodes ) );
-
-		/**
-		 * @inheritDoc
-		 */
-		this.isDocumentOperation = !!this.position.root.document;
 	}
 
 	/**

src/model/operation/markeroperation.js

@@ -19,7 +19,8 @@ export default class MarkerOperation extends Operation {
 	 * @param {module:engine/model/range~Range} oldRange Marker range before the change.
 	 * @param {module:engine/model/range~Range} newRange Marker range after the change.
 	 * @param {module:engine/model/markercollection~MarkerCollection} markers Marker collection on which change should be executed.
-	 * @param {Number} baseVersion {@link module:engine/model/document~Document#version} on which the operation can be applied.
+	 * @param {Number|null} baseVersion Document {@link module:engine/model/document~Document#version} on which operation
+	 * can be applied or `null` if the operation operates on detached (non-document) tree.
 	 */
 	constructor( name, oldRange, newRange, markers, baseVersion ) {
 		super( baseVersion );
@@ -55,11 +56,6 @@ export default class MarkerOperation extends Operation {
 		 * @member {module:engine/model/markercollection~MarkerCollection}
 		 */
 		this._markers = markers;
-
-		/**
-		 * @inheritDoc
-		 */
-		this.isDocumentOperation = this._isDocumentOperation();
 	}
 
 	/**
@@ -69,24 +65,6 @@ export default class MarkerOperation extends Operation {
 		return 'marker';
 	}
 
-	/**
-	 * Checks if operation is executed on document or document fragment nodes.
-	 *
-	 * @private
-	 */
-	_isDocumentOperation() {
-		if ( this.newRange ) {
-			return !!this.newRange.root.document;
-		}
-
-		if ( this.oldRange ) {
-			return !!this.oldRange.root.document;
-		}
-
-		// This is edge and might happen only on data from the server.
-		return true;
-	}
-
 	/**
 	 * Creates and returns an operation that has the same parameters as this operation.
 	 *

src/model/operation/moveoperation.js

@@ -29,7 +29,8 @@ export default class MoveOperation extends Operation {
 	 * @param {Number} howMany Offset size of moved range. Moved range will start from `sourcePosition` and end at
 	 * `sourcePosition` with offset shifted by `howMany`.
 	 * @param {module:engine/model/position~Position} targetPosition Position at which moved nodes will be inserted.
-	 * @param {Number} baseVersion {@link module:engine/model/document~Document#version} on which operation can be applied.
+	 * @param {Number|null} baseVersion Document {@link module:engine/model/document~Document#version} on which operation
+	 * can be applied or `null` if the operation operates on detached (non-document) tree.
 	 */
 	constructor( sourcePosition, howMany, targetPosition, baseVersion ) {
 		super( baseVersion );
@@ -64,17 +65,6 @@ export default class MoveOperation extends Operation {
 		 * @member {Boolean} module:engine/model/operation/moveoperation~MoveOperation#isSticky
 		 */
 		this.isSticky = false;
-
-		/**
-		 * Defines whether operation is executed on attached or detached {@link module:engine/model/item~Item items}.
-		 *
-		 * Note that range cannot be moved within different documents e.g. from docFrag to document root so
-		 * root of source and target positions is always the same.
-		 *
-		 * @readonly
-		 * @member {Boolean} #isDocumentOperation
-		 */
-		this.isDocumentOperation = !!this.targetPosition.root.document;
 	}
 
 	/**

src/model/operation/nooperation.js

@@ -20,18 +20,6 @@ import Operation from './operation';
  * @extends module:engine/model/operation/operation~Operation
  */
 export default class NoOperation extends Operation {
-	/**
-	 * @inheritDoc
-	 */
-	constructor( baseVersion ) {
-		super( baseVersion );
-
-		/**
-		 * @inheritDoc
-		 */
-		this.isDocumentOperation = true;
-	}
-
 	get type() {
 		return 'noop';
 	}