Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

StylesProcessor is no longer singleton #1826

Merged
merged 37 commits into from
Mar 3, 2020
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
09acfe9
Added "#document" property to "view.Node" (and inherited classes) and…
pomek Jan 30, 2020
3ffb904
Corrected view.DowncastWriter() tests.
pomek Feb 3, 2020
2d99f8f
Added #document property to view.UpcastWriter().
pomek Feb 3, 2020
414f1c1
Adjusted tests and fixed "view.Text#_clone()" and "view.Element#_clon…
pomek Feb 17, 2020
2f4357f
Reverted invalid changes.
pomek Feb 13, 2020
3b361f8
Assign parent element document to the inserted children.
jodator Feb 13, 2020
c7e0226
Cannot based on checking the "#document" property on view elements be…
pomek Feb 17, 2020
9831535
DomConverter.domToView() requires an instance of view.Document as a f…
pomek Feb 17, 2020
88b9089
Removed obsolete and invalid code.
pomek Feb 17, 2020
034b044
Use more precise name for an argument in the "needsPlaceholder()" fun…
pomek Feb 17, 2020
8af8720
Adjusted all tests.
pomek Feb 17, 2020
1fdcb8b
Added test for checking whether #document property is being updated a…
pomek Feb 17, 2020
dd78af1
Improved writers constructor docs.
pomek Feb 17, 2020
8b56db1
StylesProcessor is not singleton anymore.
pomek Feb 18, 2020
8681e0a
Styles Processor must be unique across the entire editor instance.
pomek Feb 18, 2020
a23ad5d
Updated tests to new APIs.
pomek Feb 19, 2020
5145ce0
Update missing StylesProcessor instances in constructors.
jodator Feb 19, 2020
821ae00
Add missing StylesProcessor instance.
jodator Feb 19, 2020
2ce8492
Aligned to changes in Editor class.
pomek Feb 20, 2020
e2ed99c
Aligned tests to changes in API.
pomek Feb 20, 2020
d431e83
Simplified the code.
pomek Feb 20, 2020
e7f04b5
Merge branch 'master' into i/6091
pomek Feb 20, 2020
066db20
Requested changes during the review.
pomek Feb 24, 2020
c62176b
Fixed docs after moving method between classes.
pomek Feb 25, 2020
42fa0f7
Simplified tests.
pomek Feb 25, 2020
6b9ac71
Remove empty before.
jodator Feb 21, 2020
cd0f3b6
Reverted changes in Renderer class.
pomek Feb 25, 2020
3c3e902
"This should be inlined."
pomek Feb 25, 2020
fabfb3a
Merge branch 'i/6091' of github.com:ckeditor/ckeditor5-engine into i/…
pomek Feb 25, 2020
5a4ab82
Merge branch 'master' into i/6091
jodator Feb 26, 2020
21390a9
Removed the 'model.Node#document.' property.
pomek Feb 27, 2020
8039a8d
Added a new method (#isAttached()) to both (view and model) Nodes cla…
pomek Feb 28, 2020
b21a415
Removed the dataProcessor argument, as it was never used in practice.
Reinmar Mar 3, 2020
054d8a2
Docs.
Reinmar Mar 3, 2020
13dd24c
Docs and code simplification.
Reinmar Mar 3, 2020
16301a1
Docs and other improvements.
Reinmar Mar 3, 2020
fb8e77a
Docs and other improvements.
Reinmar Mar 3, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions src/controller/datacontroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,12 @@ export default class DataController {
/**
* Creates a data controller instance.
*
* @param {module:engine/view/stylesmap~StylesProcessor} stylesProcessor Styles processor.
* @param {module:engine/model/model~Model} model Data model.
* @param {module:engine/dataprocessor/dataprocessor~DataProcessor} [dataProcessor] Data processor that should be used
* by the controller.
*/
constructor( model, dataProcessor ) {
constructor( stylesProcessor, model, dataProcessor ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this the first param? It should be the last one. It is the last one in the EditingController already.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because dataProcessor is not required. Following the docs:

@param {module:engine/dataprocessor/dataprocessor~DataProcessor} [dataProcessor]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also a test that says the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, the model is required. So if anything, this should be the 2nd arg.

/**
* Data model.
*
Expand All @@ -59,6 +60,14 @@ export default class DataController {
*/
this.model = model;

/**
* StylesProcessor is responsible for writing and reading a normalized styles object.
*
* @readonly
* @member {module:engine/view/stylesmap~StylesProcessor}
*/
this.stylesProcessor = stylesProcessor;

/**
* Data processor used during the conversion.
*
Expand Down Expand Up @@ -187,12 +196,13 @@ export default class DataController {

// First, convert elements.
const modelRange = ModelRange._createIn( modelElementOrFragment );
const viewDocument = new ViewDocument( this.stylesProcessor );

const viewDocumentFragment = new ViewDocumentFragment();
const viewDocumentFragment = new ViewDocumentFragment( viewDocument );

// Create separate ViewDowncastWriter just for data conversion purposes.
// We have no view controller and rendering do DOM in DataController so view.change() block is not used here.
const viewWriter = new ViewDowncastWriter( new ViewDocument() );
const viewWriter = new ViewDowncastWriter( viewDocument );
this.mapper.bindElements( modelElementOrFragment, viewDocumentFragment );

this.downcastDispatcher.convertInsert( modelRange, viewWriter );
Expand Down Expand Up @@ -371,6 +381,22 @@ export default class DataController {
} );
}

/**
* Adds a style processor normalization rules.
*
* The available style processors:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* The available style processors:
* You can implement your own rules as well as use one of the available processor rules:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* The available style processors:
* You can implement your own rules as well as use one of the available processor rules:

*
* * background: {@link module:engine/view/styles/background~addBackgroundRules}
* * border: {@link module:engine/view/styles/border~addBorderRules}
* * margin: {@link module:engine/view/styles/margin~addMarginRules}
* * padding: {@link module:engine/view/styles/padding~addPaddingRules}
*
* @param {Function} callback
*/
addStyleProcessorRules( callback ) {
callback( this.stylesProcessor );
}

/**
* Removes all event listeners set by the DataController.
*/
Expand Down
8 changes: 4 additions & 4 deletions src/controller/editingcontroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@ export default class EditingController {
* Creates an editing controller instance.
*
* @param {module:engine/model/model~Model} model Editing model.
* @param {module:engine/view/stylesmap~StylesProcessor} stylesProcessor Styles processor.
*/
constructor( model ) {
constructor( model, stylesProcessor ) {
/**
* Editor model.
*
Expand All @@ -47,7 +48,7 @@ export default class EditingController {
* @readonly
* @member {module:engine/view/view~View}
*/
this.view = new View();
this.view = new View( stylesProcessor );

/**
* Mapper which describes the model-view binding.
Expand Down Expand Up @@ -115,10 +116,9 @@ export default class EditingController {
return null;
}

const viewRoot = new RootEditableElement( root.name );
const viewRoot = new RootEditableElement( this.view.document, root.name );

viewRoot.rootName = root.rootName;
viewRoot._document = this.view.document;
this.mapper.bindElements( root, viewRoot );

return viewRoot;
Expand Down
11 changes: 6 additions & 5 deletions src/conversion/downcasthelpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -416,11 +416,12 @@ export function remove() {
* provided by the {@link module:engine/conversion/downcasthelpers~HighlightDescriptor highlight descriptor} object. If a priority
* is not provided in the descriptor, the default priority will be used.
*
* @param {module:engine/view/downcastwriter~DowncastWriter} writer
* @param {module:engine/conversion/downcasthelpers~HighlightDescriptor} descriptor
* @returns {module:engine/view/attributeelement~AttributeElement}
*/
export function createViewElementFromHighlightDescriptor( descriptor ) {
const viewElement = new ViewAttributeElement( 'span', descriptor.attributes );
export function createViewElementFromHighlightDescriptor( writer, descriptor ) {
const viewElement = writer.createAttributeElement( 'span', descriptor.attributes );

if ( descriptor.classes ) {
viewElement._addClass( descriptor.classes );
Expand Down Expand Up @@ -543,7 +544,7 @@ export function clearAttributes() {
// Not collapsed selection should not have artifacts.
if ( range.isCollapsed ) {
// Position might be in the node removed by the view writer.
if ( range.end.parent.document ) {
if ( range.end.parent.parent ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: check this later. Seems dangerous.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this changes the semantics and I'm also unsure about it. But it means that if this change didn't cause any test to fail, then we may be missing a test. cc @scofalik

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scofalik ping - would be nice to have this confirmed or debunked ;)

conversionApi.writer.mergeAttributes( range.start );
}
}
Expand Down Expand Up @@ -919,8 +920,8 @@ function highlightText( highlightDescriptor ) {
return;
}

const viewElement = createViewElementFromHighlightDescriptor( descriptor );
const viewWriter = conversionApi.writer;
const viewElement = createViewElementFromHighlightDescriptor( viewWriter, descriptor );
const viewSelection = viewWriter.document.selection;

if ( data.item instanceof ModelSelection || data.item instanceof DocumentSelection ) {
Expand Down Expand Up @@ -1034,7 +1035,7 @@ function removeHighlight( highlightDescriptor ) {
}

// View element that will be used to unwrap `AttributeElement`s.
const viewHighlightElement = createViewElementFromHighlightDescriptor( descriptor );
const viewHighlightElement = createViewElementFromHighlightDescriptor( conversionApi.writer, descriptor );

// Get all elements bound with given marker name.
const elements = conversionApi.mapper.markerNameToElements( data.markerName );
Expand Down
21 changes: 15 additions & 6 deletions src/conversion/viewconsumable.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

import { isArray } from 'lodash-es';
import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
import StylesMap from '../view/stylesmap';

/**
* Class used for handling consumption of view {@link module:engine/view/element~Element elements},
Expand Down Expand Up @@ -89,7 +88,7 @@ export default class ViewConsumable {

// For elements create new ViewElementConsumables or update already existing one.
if ( !this._consumables.has( element ) ) {
elementConsumables = new ViewElementConsumables();
elementConsumables = new ViewElementConsumables( element );
this._consumables.set( element, elementConsumables );
} else {
elementConsumables = this._consumables.get( element );
Expand Down Expand Up @@ -239,6 +238,7 @@ export default class ViewConsumable {
*/
static consumablesFromElement( element ) {
const consumables = {
element,
name: true,
attributes: [],
classes: [],
Expand Down Expand Up @@ -284,7 +284,7 @@ export default class ViewConsumable {
*/
static createFrom( from, instance ) {
if ( !instance ) {
instance = new ViewConsumable();
instance = new ViewConsumable( from );
jodator marked this conversation as resolved.
Show resolved Hide resolved
}

if ( from.is( 'text' ) ) {
Expand Down Expand Up @@ -319,8 +319,17 @@ export default class ViewConsumable {
class ViewElementConsumables {
/**
* Creates ViewElementConsumables instance.
*
* @param {module:engine/view/node~Node|module:engine/view/documentfragment~DocumentFragment} from View node or document fragment
* from which `ViewElementConsumables` is being created.
*/
constructor() {
constructor( from ) {
/**
* @readonly
* @member {module:engine/view/node~Node|module:engine/view/documentfragment~DocumentFragment}
*/
this.element = from;
pomek marked this conversation as resolved.
Show resolved Hide resolved

/**
* Flag indicating if name of the element can be consumed.
*
Expand Down Expand Up @@ -510,7 +519,7 @@ class ViewElementConsumables {
consumables.set( name, true );

if ( type === 'styles' ) {
for ( const alsoName of StylesMap.getRelatedStyles( name ) ) {
for ( const alsoName of this.element.document.stylesProcessor.getRelatedStyles( name ) ) {
consumables.set( alsoName, true );
}
}
Expand Down Expand Up @@ -577,7 +586,7 @@ class ViewElementConsumables {
consumables.set( name, false );

if ( type == 'styles' ) {
for ( const toConsume of StylesMap.getRelatedStyles( name ) ) {
for ( const toConsume of this.element.document.stylesProcessor.getRelatedStyles( name ) ) {
consumables.set( toConsume, false );
}
}
Expand Down
6 changes: 4 additions & 2 deletions src/dataprocessor/htmldataprocessor.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ import DomConverter from '../view/domconverter';
export default class HtmlDataProcessor {
/**
* Creates a new instance of the HTML data processor class.
*
* @param {module:engine/view/document~Document} document
*/
constructor() {
constructor( document ) {
/**
* A DOM parser instance used to parse an HTML string to an HTML document.
*
Expand All @@ -37,7 +39,7 @@ export default class HtmlDataProcessor {
* @private
* @member {module:engine/view/domconverter~DomConverter}
*/
this._domConverter = new DomConverter( { blockFillerMode: 'nbsp' } );
this._domConverter = new DomConverter( document, { blockFillerMode: 'nbsp' } );

/**
* A basic HTML writer instance used to convert DOM elements to an HTML string.
Expand Down
5 changes: 3 additions & 2 deletions src/dataprocessor/xmldataprocessor.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,11 @@ export default class XmlDataProcessor {
/**
* Creates a new instance of the XML data processor class.
*
* @param {module:engine/view/document~Document} document
* @param {Object} options Configuration options.
* @param {Array<String>} [options.namespaces=[]] A list of namespaces allowed to use in the XML input.
*/
constructor( options = {} ) {
constructor( document, options = {} ) {
/**
* A list of namespaces allowed to use in the XML input.
*
Expand All @@ -53,7 +54,7 @@ export default class XmlDataProcessor {
* @private
* @member {module:engine/view/domconverter~DomConverter}
*/
this._domConverter = new DomConverter( { blockFillerMode: 'nbsp' } );
this._domConverter = new DomConverter( document, { blockFillerMode: 'nbsp' } );

/**
* A basic HTML writer instance used to convert DOM elements to an XML string.
Expand Down
9 changes: 5 additions & 4 deletions src/dev-utils/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {

import { isPlainObject } from 'lodash-es';
import toMap from '@ckeditor/ckeditor5-utils/src/tomap';
import { StylesProcessor } from '../view/stylesmap';

/**
* Writes the content of a model {@link module:engine/model/document~Document document} to an HTML-like string.
Expand Down Expand Up @@ -210,12 +211,12 @@ export function stringify( node, selectionOrPositionOrRange = null, markers = nu

// Set up conversion.
// Create a temporary view controller.
const view = new View();
const stylesProcessor = new StylesProcessor();
const view = new View( stylesProcessor );
const viewDocument = view.document;
const viewRoot = new ViewRootEditableElement( 'div' );
const viewRoot = new ViewRootEditableElement( viewDocument, 'div' );

// Create a temporary root element in view document.
viewRoot._document = view.document;
viewRoot.rootName = 'main';
viewDocument.roots.add( viewRoot );

Expand All @@ -242,7 +243,7 @@ export function stringify( node, selectionOrPositionOrRange = null, markers = nu
// Stringify object types values for properly display as an output string.
const attributes = convertAttributes( modelItem.getAttributes(), stringifyAttributeValue );

return new ViewContainerElement( modelItem.name, attributes );
return new ViewContainerElement( viewDocument, modelItem.name, attributes );
} ) );

downcastDispatcher.on( 'selection', convertRangeSelection() );
Expand Down
16 changes: 12 additions & 4 deletions src/dev-utils/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/

import View from '../view/view';
import ViewDocument from '../view/document';
import ViewDocumentFragment from '../view/documentfragment';
import XmlDataProcessor from '../dataprocessor/xmldataprocessor';
import ViewElement from '../view/element';
Expand All @@ -24,6 +25,7 @@ import AttributeElement from '../view/attributeelement';
import ContainerElement from '../view/containerelement';
import EmptyElement from '../view/emptyelement';
import UIElement from '../view/uielement';
import { StylesProcessor } from '../view/stylesmap';

const ELEMENT_RANGE_START_TOKEN = '[';
const ELEMENT_RANGE_END_TOKEN = ']';
Expand Down Expand Up @@ -321,16 +323,19 @@ export function stringify( node, selectionOrPositionOrRange = null, options = {}
* this node will be used as the root for all parsed nodes.
* @param {Boolean} [options.sameSelectionCharacters=false] When set to `false`, the selection inside the text should be marked using
* `{` and `}` and the selection outside the ext using `[` and `]`. When set to `true`, both should be marked with `[` and `]` only.
* @param {module:engine/view/stylesmap~StylesProcessor} [options.stylesProcessor] Styles processor.
* @returns {module:engine/view/text~Text|module:engine/view/element~Element|module:engine/view/documentfragment~DocumentFragment|Object}
* Returns the parsed view node or an object with two fields: `view` and `selection` when selection ranges were included in the data
* to parse.
*/
export function parse( data, options = {} ) {
const viewDocument = new ViewDocument( new StylesProcessor() );

options.order = options.order || [];
const rangeParser = new RangeParser( {
sameSelectionCharacters: options.sameSelectionCharacters
} );
const processor = new XmlDataProcessor( {
const processor = new XmlDataProcessor( viewDocument, {
namespaces: Object.keys( allowedTypes )
} );

Expand Down Expand Up @@ -927,7 +932,10 @@ class ViewStringify {
function _convertViewElements( rootNode ) {
if ( rootNode.is( 'element' ) || rootNode.is( 'documentFragment' ) ) {
// Convert element or leave document fragment.
const convertedElement = rootNode.is( 'documentFragment' ) ? new ViewDocumentFragment() : _convertElement( rootNode );

const convertedElement = rootNode.is( 'documentFragment' ) ?
new ViewDocumentFragment( rootNode.document ) :
_convertElement( rootNode.document, rootNode );

// Convert all child nodes.
// Cache the nodes in array. Otherwise, we would skip some nodes because during iteration we move nodes
Expand Down Expand Up @@ -973,10 +981,10 @@ function _convertViewElements( rootNode ) {
// module:engine/view/emptyelement~EmptyElement|module:engine/view/uielement~UIElement|
// module:engine/view/containerelement~ContainerElement} A tree view
// element converted according to its name.
function _convertElement( viewElement ) {
function _convertElement( viewDocument, viewElement ) {
const info = _convertElementNameAndInfo( viewElement );
const ElementConstructor = allowedTypes[ info.type ];
const newElement = ElementConstructor ? new ElementConstructor( info.name ) : new ViewElement( info.name );
const newElement = ElementConstructor ? new ElementConstructor( viewDocument, info.name ) : new ViewElement( viewDocument, info.name );

if ( newElement.is( 'attributeElement' ) ) {
if ( info.priority !== null ) {
Expand Down
2 changes: 1 addition & 1 deletion src/model/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ export default class Model {
*
* // You can create your own HtmlDataProcessor instance or use editor.data.processor
* // if you have not overridden the default one (which is the HtmlDataProcessor instance).
* const htmlDP = new HtmlDataProcessor();
* const htmlDP = new HtmlDataProcessor( viewDocument );
*
* // Convert an HTML string to a view document fragment:
* const viewFragment = htmlDP.toView( htmlString );
Expand Down
9 changes: 7 additions & 2 deletions src/view/attributeelement.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,14 @@ export default class AttributeElement extends Element {
* @see module:engine/view/downcastwriter~DowncastWriter#createAttributeElement
* @see module:engine/view/element~Element
* @protected
* @param {module:engine/view/document~Document} document A document where the element belongs to.
* @param {String} name Node name.
* @param {Object|Iterable} [attrs] Collection of attributes.
* @param {module:engine/view/node~Node|Iterable.<module:engine/view/node~Node>} [children]
* A list of nodes to be inserted into created element.
*/
constructor( name, attrs, children ) {
super( name, attrs, children );
constructor( document, name, attrs, children ) {
super( document, name, attrs, children );

/**
* Returns block {@link module:engine/view/filler filler} offset or `null` if block filler is not needed.
Expand Down
9 changes: 7 additions & 2 deletions src/view/containerelement.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,14 @@ export default class ContainerElement extends Element {
* @see module:engine/view/downcastwriter~DowncastWriter#createContainerElement
* @see module:engine/view/element~Element
* @protected
* @param {module:engine/view/document~Document} document A document where the element belongs to.
* @param {String} name Node name.
* @param {Object|Iterable} [attrs] Collection of attributes.
* @param {module:engine/view/node~Node|Iterable.<module:engine/view/node~Node>} [children]
* A list of nodes to be inserted into created element.
*/
constructor( name, attrs, children ) {
super( name, attrs, children );
constructor( document, name, attrs, children ) {
super( document, name, attrs, children );

/**
* Returns block {@link module:engine/view/filler filler} offset or `null` if block filler is not needed.
Expand Down
Loading