Merge pull request #1302 from ckeditor/t/1295

Other: Introduced several improvements to conversion helpers. Closes #1295. Closes #1293. Closes #1292. Closes #1291. Closes #1290. Closes #1305.

src/conversion/upcast-converters.js

@@ -26,7 +26,7 @@ import cloneDeep from '@ckeditor/ckeditor5-utils/src/lib/lodash/cloneDeep';
  *
  *		upcastElementToElement( { view: 'p', model: 'paragraph' } );
  *
- *		upcastElementToElement( { view: 'p', model: 'paragraph' }, 'high' );
+ *		upcastElementToElement( { view: 'p', model: 'paragraph', priority: 'high' } );
  *
  *		upcastElementToElement( {
  *			view: {
@@ -37,19 +37,13 @@ import cloneDeep from '@ckeditor/ckeditor5-utils/src/lib/lodash/cloneDeep';
  *		} );
  *
  *		upcastElementToElement( {
- *			view: {
- *				name: 'p',
- *				class: 'fancy'
- *			},
- *			model: new ModelElement( 'p', { fancy: true } )
- *		} );
- *
- *		upcastElementToElement( {
  * 			view: {
  *				name: 'p',
  *				class: 'heading'
  * 			},
- * 			model: viewElement => new ModelElement( 'heading', { level: viewElement.getAttribute( 'data-level' ) } )
+ * 			model: ( viewElement, modelWriter ) => {
+ * 				return modelWriter.createElement( 'heading', { level: viewElement.getAttribute( 'data-level' ) } );
+ * 			}
  * 		} );
  *
  * See {@link module:engine/conversion/conversion~Conversion#for} to learn how to add converter to conversion process.
@@ -58,10 +52,10 @@ import cloneDeep from '@ckeditor/ckeditor5-utils/src/lib/lodash/cloneDeep';
  * @param {module:engine/view/matcher~MatcherPattern} config.view Pattern matching all view elements which should be converted.
  * @param {String|module:engine/model/element~Element|Function} config.model Name of the model element, a model element
  * instance or a function that takes a view element and returns a model element. The model element will be inserted in the model.
- * @param {module:utils/priorities~PriorityString} [priority='normal'] Converter priority.
+ * @param {module:utils/priorities~PriorityString} [config.priority='normal'] Converter priority.
  * @returns {Function} Conversion helper.
  */
-export function upcastElementToElement( config, priority = 'normal' ) {
+export function upcastElementToElement( config ) {
 	config = cloneDeep( config );
 
 	const converter = _prepareToElementConverter( config );
@@ -70,7 +64,7 @@ export function upcastElementToElement( config, priority = 'normal' ) {
 	const eventName = elementName ? 'element:' + elementName : 'element';
 
 	return dispatcher => {
-		dispatcher.on( eventName, converter, { priority } );
+		dispatcher.on( eventName, converter, { priority: config.priority || 'normal' } );
 	};
 }
 
@@ -84,7 +78,7 @@ export function upcastElementToElement( config, priority = 'normal' ) {
  *
  *		upcastElementToAttribute( { view: 'strong', model: 'bold' } );
  *
- *		upcastElementToAttribute( { view: 'strong', model: 'bold' }, 'normal' );
+ *		upcastElementToAttribute( { view: 'strong', model: 'bold', priority: 'high' } );
  *
  *		upcastElementToAttribute( {
  *			view: {
@@ -136,10 +130,10 @@ export function upcastElementToElement( config, priority = 'normal' ) {
  * @param {String|Object} config.model Model attribute key or an object with `key` and `value` properties, describing
  * the model attribute. `value` property may be set as a function that takes a view element and returns the value.
  * If `String` is given, the model attribute value will be set to `true`.
- * @param {module:utils/priorities~PriorityString} [priority='low'] Converter priority.
+ * @param {module:utils/priorities~PriorityString} [config.priority='normal'] Converter priority.
  * @returns {Function} Conversion helper.
  */
-export function upcastElementToAttribute( config, priority = 'low' ) {
+export function upcastElementToAttribute( config ) {
 	config = cloneDeep( config );
 
 	_normalizeModelAttributeConfig( config );
@@ -150,7 +144,7 @@ export function upcastElementToAttribute( config, priority = 'low' ) {
 	const eventName = elementName ? 'element:' + elementName : 'element';
 
 	return dispatcher => {
-		dispatcher.on( eventName, converter, { priority } );
+		dispatcher.on( eventName, converter, { priority: config.priority || 'normal' } );
 	};
 }
 
@@ -166,7 +160,7 @@ export function upcastElementToAttribute( config, priority = 'low' ) {
  *
  *		upcastAttributeToAttribute( { view: { key: 'src' }, model: 'source' } );
  *
- *		upcastAttributeToAttribute( { view: { key: 'src' }, model: 'source' }, 'normal' );
+ *		upcastAttributeToAttribute( { view: { key: 'src' }, model: 'source', priority: 'normal' } );
  *
  *		upcastAttributeToAttribute( {
  *			view: {
@@ -178,7 +172,7 @@ export function upcastElementToAttribute( config, priority = 'low' ) {
  *
  *		upcastAttributeToAttribute( {
  *			view: {
- *				name: 'span',
+ *				name: 'img',
  *				key: 'class',
  *				value: 'styled-dark'
  *			},
@@ -215,10 +209,10 @@ export function upcastElementToAttribute( config, priority = 'low' ) {
  * @param {String|Object} config.model Model attribute key or an object with `key` and `value` properties, describing
  * the model attribute. `value` property may be set as a function that takes a view element and returns the value.
  * If `String` is given, the model attribute value will be same as view attribute value.
- * @param {module:utils/priorities~PriorityString} [priority='low'] Converter priority.
+ * @param {module:utils/priorities~PriorityString} [config.priority='low'] Converter priority.
  * @returns {Function} Conversion helper.
  */
-export function upcastAttributeToAttribute( config, priority = 'low' ) {
+export function upcastAttributeToAttribute( config ) {
 	config = cloneDeep( config );
 
 	let viewKey = null;
@@ -232,7 +226,7 @@ export function upcastAttributeToAttribute( config, priority = 'low' ) {
 	const converter = _prepareToAttributeConverter( config, false );
 
 	return dispatcher => {
-		dispatcher.on( 'element', converter, { priority } );
+		dispatcher.on( 'element', converter, { priority: config.priority || 'low' } );
 	};
 }
 
@@ -246,7 +240,7 @@ export function upcastAttributeToAttribute( config, priority = 'low' ) {
  *
  *		upcastElementToMarker( { view: 'marker-search', model: 'search' } );
  *
- *		upcastElementToMarker( { view: 'marker-search', model: 'search' }, 'high' );
+ *		upcastElementToMarker( { view: 'marker-search', model: 'search', priority: 'high' } );
  *
  *		upcastElementToMarker( {
  *			view: 'marker-search',
@@ -269,15 +263,15 @@ export function upcastAttributeToAttribute( config, priority = 'low' ) {
  * @param {module:engine/view/matcher~MatcherPattern} config.view Pattern matching all view elements which should be converted.
  * @param {String|Function} config.model Name of the model marker, or a function that takes a view element and returns
  * a model marker name.
- * @param {module:utils/priorities~PriorityString} [priority='normal'] Converter priority.
+ * @param {module:utils/priorities~PriorityString} [config.priority='normal'] Converter priority.
  * @returns {Function} Conversion helper.
  */
-export function upcastElementToMarker( config, priority = 'normal' ) {
+export function upcastElementToMarker( config ) {
 	config = cloneDeep( config );
 
 	_normalizeToMarkerConfig( config );
 
-	return upcastElementToElement( config, priority );
+	return upcastElementToElement( config );
 }
 
 // Helper function for from-view-element conversion. Checks if `config.view` directly specifies converted view element's name
@@ -313,6 +307,9 @@ function _prepareToElementConverter( config ) {
 			return;
 		}
 
+		// Force consuming element's name.
+		match.match.name = true;
+
 		// Create model element basing on config.
 		const modelElement = _getModelElement( config.model, data.viewItem, conversionApi.writer );
 
@@ -380,10 +377,8 @@ function _prepareToElementConverter( config ) {
 function _getModelElement( model, input, writer ) {
 	if ( model instanceof Function ) {
 		return model( input, writer );
-	} else if ( typeof model == 'string' ) {
-		return writer.createElement( model );
 	} else {
-		return model;
+		return writer.createElement( model );
 	}
 }
 
@@ -399,13 +394,21 @@ function _normalizeViewAttributeKeyValueConfig( config ) {
 	}
 
 	const key = config.view.key;
-	const value = typeof config.view.value == 'undefined' ? /[\s\S]*/ : config.view.value;
+	let normalized;
 
-	const normalized = {
-		attribute: {
-			[ key ]: value
-		}
-	};
+	if ( key == 'class' || key == 'style' ) {
+		normalized = {
+			[ key ]: config.view.value
+		};
+	} else {
+		const value = typeof config.view.value == 'undefined' ? /[\s\S]*/ : config.view.value;
+
+		normalized = {
+			attribute: {
+				[ key ]: value
+			}
+		};
+	}
 
 	if ( config.view.name ) {
 		normalized.name = config.view.name;
@@ -437,7 +440,8 @@ function _normalizeModelAttributeConfig( config, viewAttributeKeyToCopy = null )
 //
 // @param {String} modelAttributeKey The key of the model attribute to set on a model node.
 // @param {Object|Array.<Object>} config Conversion configuration. It is possible to provide multiple configurations in an array.
-// @param {Boolean} consumeName If set to `true` converter will not consume element's name.
+// @param {Boolean} consumeName If set to `true` converter will try to consume name. If set to `false` converter will not try to
+// consume name. This flag overwrites parameter returned by `Matcher#match`.
 function _prepareToAttributeConverter( config, consumeName ) {
 	const matcher = new Matcher( config.view );
 
@@ -460,6 +464,8 @@ function _prepareToAttributeConverter( config, consumeName ) {
 		if ( !consumeName ) {
 			// Do not test or consume `name` consumable.
 			delete match.match.name;
+		} else {
+			match.match.name = true;
 		}
 
 		// Try to consume appropriate values from consumable values list.
@@ -512,10 +518,10 @@ function _setAttributeOn( modelRange, modelAttribute, conversionApi ) {
 function _normalizeToMarkerConfig( config ) {
 	const oldModel = config.model;
 
-	config.model = ( viewElement, writer ) => {
+	config.model = ( viewElement, modelWriter ) => {
 		const markerName = typeof oldModel == 'string' ? oldModel : oldModel( viewElement );
 
-		return writer.createElement( '$marker', { 'data-name': markerName } );
+		return modelWriter.createElement( '$marker', { 'data-name': markerName } );
 	};
 }
 

src/dev-utils/model.js

@@ -16,24 +16,25 @@ import Model from '../model/model';
 import Batch from '../model/batch';
 import ModelRange from '../model/range';
 import ModelPosition from '../model/position';
-import DowncastDispatcher from '../conversion/downcastdispatcher';
 import ModelSelection from '../model/selection';
 import ModelDocumentFragment from '../model/documentfragment';
 import DocumentSelection from '../model/documentselection';
 
 import View from '../view/view';
-import UpcastDispatcher from '../conversion/upcastdispatcher';
 import ViewContainerElement from '../view/containerelement';
-import ViewAttributeElement from '../view/attributeelement';
 import ViewRootEditableElement from '../view/rooteditableelement';
 
-import Mapper from '../conversion/mapper';
 import { parse as viewParse, stringify as viewStringify } from '../../src/dev-utils/view';
+
+import DowncastDispatcher from '../conversion/downcastdispatcher';
+import UpcastDispatcher from '../conversion/upcastdispatcher';
+import Mapper from '../conversion/mapper';
 import {
 	convertRangeSelection,
 	convertCollapsedSelection,
 } from '../conversion/downcast-selection-converters';
 import { insertText, insertElement, wrap } from '../conversion/downcast-converters';
+
 import isPlainObject from '@ckeditor/ckeditor5-utils/src/lib/lodash/isPlainObject';
 import toMap from '@ckeditor/ckeditor5-utils/src/tomap';
 
@@ -208,11 +209,18 @@ export function stringify( node, selectionOrPositionOrRange = null ) {
 	mapper.bindElements( node.root, viewRoot );
 
 	downcastDispatcher.on( 'insert:$text', insertText() );
-	downcastDispatcher.on( 'attribute', wrap( ( value, data ) => {
+	downcastDispatcher.on( 'attribute', ( evt, data, consumable, conversionApi ) => {
 		if ( data.item instanceof ModelSelection || data.item instanceof DocumentSelection || data.item.is( 'textProxy' ) ) {
-			return new ViewAttributeElement( 'model-text-with-attributes', { [ data.attributeKey ]: stringifyAttributeValue( value ) } );
+			const converter = wrap( ( modelAttributeValue, viewWriter ) => {
+				return viewWriter.createAttributeElement(
+					'model-text-with-attributes',
+					{ [ data.attributeKey ]: stringifyAttributeValue( modelAttributeValue ) }
+				);
+			} );
+
+			converter( evt, data, consumable, conversionApi );
 		}
-	} ) );
+	} );
 	downcastDispatcher.on( 'insert', insertElement( modelItem => {
 		// Stringify object types values for properly display as an output string.
 		const attributes = convertAttributes( modelItem.getAttributes(), stringifyAttributeValue );

src/model/markercollection.js

@@ -80,7 +80,8 @@ export default class MarkerCollection {
 	 *
 	 * If `MarkerCollection` already had a marker with given name (or {@link ~Marker marker} was passed), the marker in
 	 * collection is updated and {@link module:engine/model/markercollection~MarkerCollection#event:update} event is fired
-	 * but only if there was a change (marker range or {@link ~Marker#managedUsingOperations} flag has changed.
+	 * but only if there was a change (marker range or {@link module:engine/model/markercollection~Marker#managedUsingOperations}
+	 * flag has changed.
 	 *
 	 * @protected
 	 * @fires module:engine/model/markercollection~MarkerCollection#event:update
@@ -246,7 +247,7 @@ mix( MarkerCollection, EmitterMixin );
  * Name is used to group and identify markers. Names have to be unique, but markers can be grouped by
  * using common prefixes, separated with `:`, for example: `user:john` or `search:3`. That's useful in term of creating
  * namespaces for custom elements (e.g. comments, highlights). You can use this prefixes in
- * {@link module:engine/model/markercollection~MarkerCollection#event:set} listeners to listen on changes in a group of markers.
+ * {@link module:engine/model/markercollection~MarkerCollection#event:update} listeners to listen on changes in a group of markers.
  * For instance: `model.markers.on( 'set:user', callback );` will be called whenever any `user:*` markers changes.
  *
  * There are two types of markers.
@@ -425,7 +426,7 @@ class Marker {
 	 *
 	 * When marker is removed from {@link module:engine/model/markercollection~MarkerCollection MarkerCollection},
 	 * all event listeners listening to it should be removed. It is best to do it on
-	 * {@link module:engine/model/markercollection~MarkerCollection#event:remove MarkerCollection remove event}.
+	 * {@link module:engine/model/markercollection~MarkerCollection#event:update MarkerCollection update event}.
 	 *
 	 * @see module:engine/model/liverange~LiveRange#event:change:range
 	 * @event change:range
@@ -439,7 +440,7 @@ class Marker {
 	 *
 	 * When marker is removed from {@link module:engine/model/markercollection~MarkerCollection MarkerCollection},
 	 * all event listeners listening to it should be removed. It is best to do it on
-	 * {@link module:engine/model/markercollection~MarkerCollection#event:remove MarkerCollection remove event}.
+	 * {@link module:engine/model/markercollection~MarkerCollection#event:update MarkerCollection update event}.
 	 *
 	 * @see module:engine/model/liverange~LiveRange#event:change:content
 	 * @event change:content

src/model/writer.js

@@ -761,34 +761,32 @@ export default class Writer {
 	}
 
 	/**
-	 * Adds or updates {@link module:engine/model/markercollection~Marker marker}.
+	 * Adds or updates a {@link module:engine/model/markercollection~Marker marker}. Marker is a named range, which tracks
+	 * changes in the document and updates its range automatically, when model tree changes. Still, it is possible to change the
+	 * marker's range directly using this method.
 	 *
 	 * As the first parameter you can set marker name or instance. If none of them is provided, new marker, with a unique
 	 * name is created and returned.
 	 *
-	 * Using this method you can change markers range or define if the marker is managed by operation or not.
+	 * The `options.usingOperation` parameter lets you decide if the marker should be managed by operations or not. See
+	 * {@link module:engine/model/markercollection~Marker marker class description} to learn about the difference between
+	 * markers managed by operations and not-managed by operations. It is possible to change this option for an existing marker.
+	 * This is useful when a marker have been created earlier and then later, it needs to be added to the document history.
 	 *
-	 * Marker tracks changes is the document and updates the range automatically, so you need to update the range only
-	 * when it changes directly. You do not need to update it after each document change.
+	 * Create/update marker directly base on marker's name:
 	 *
-	 * The option parameter let you decide if the marker should be managed by operations or not. See
-	 * {@link module:engine/model/markercollection~Marker marker class description} to learn about the difference between
-	 * markers managed by operation and managed directly. You can change this option for existing marker. This is
-	 * useful if a marker have been created earlier and need to be added to the document history later.
+	 * 		setMarker( markerName, range );
 	 *
 	 * Update marker using operation:
 	 *
 	 * 		setMarker( marker, range, { usingOperation: true } );
-	 *
-	 * Create/update marker directly base on marker's name:
-	 *
-	 * 		setMarker( markerName, range );
+	 * 		setMarker( markerName, range, { usingOperation: true } );
 	 *
 	 * Create marker with a unique id using operation:
 	 *
 	 * 		setMarker( range, { usingOperation: true } );
 	 *
-	 * Create marker directly with a unique name:
+	 * Create marker directly without using operations:
 	 *
 	 * 		setMarker( range )
 	 *
@@ -799,9 +797,9 @@ export default class Writer {
 	 * Note: For efficiency reasons, it's best to create and keep as little markers as possible.
 	 *
 	 * @see module:engine/model/markercollection~Marker
-	 * @param {module:engine/model/markercollection~Marker|String} [markerOrName=uid()]
-	 * Name of marker to add, Marker instance to update or range for the marker with a unique name.
-	 * @param {module:engine/model/range~Range|Object} [range] Marker range or options.
+	 * @param {module:engine/model/markercollection~Marker|String} [markerOrName]
+	 * Name of a marker to create or update, or `Marker` instance to update, or range for the marker with a unique name.
+	 * @param {module:engine/model/range~Range} [range] Marker range.
 	 * @param {Object} [options]
 	 * @param {Boolean} [options.usingOperation=false] Flag indicated whether the marker should be added by MarkerOperation.
 	 * See {@link module:engine/model/markercollection~Marker#managedUsingOperations}.