StylesProcessor is no longer singleton #1826

pomek · 2020-02-20T14:00:57Z

Suggested merge commit message (convention)

Other: StylesProcessor rules will not be stored in a singleton, which made them shared between editor instances. In order to allow binding a styles processor instance to a specific view document, we had to replace a dynamic #document property in view nodes with a static one, set upon node creation. Closes ckeditor/ckeditor5#6091.

MAJOR BREAKING CHANGES: EditingController requires an instance of StylesProcessor in its constructor.

MAJOR BREAKING CHANGES: DataController requires an instance of StylesProcessor in its constructor.

MAJOR BREAKING CHANGES: DomConverter, HtmlDataProcessor and XmlDataProcessor require an instance of the view document in their constructors.

MAJOR BREAKING CHANGES: The View class requires an instance of StylesProcessor as its first argument.

MAJOR BREAKING CHANGES: The createViewElementFromHighlightDescriptor() function that is exported by src/conversion/downcasthelpers.js file requires an instance of the view document as its first argument.

MAJOR BREAKING CHANGES: Method view.Document#addStyleProcessorRules() has been moved to DataController class.

MINOR BREAKING CHANGES: DataController does not accept the data processor instance any more.

MAJOR BREAKING CHANGES: The #document getter was removed from model nodes. Only the root element holds the reference to the model document. For attached nodes, use node.root.document to access it.

… "view.DocumentFragment".

…e()" methods.

…cause it always be defined.

…irst argument.

…ction.

…fter inserting a children to an element.

jodator

I think we're good to merge it now 👍

coveralls · 2020-02-26T14:28:52Z

Coverage decreased (-15.7%) to 84.292% when pulling fb8e77a on i/6091 into 5fdb765 on master.

Reinmar · 2020-02-26T14:40:18Z

I think we're good to merge it now 👍

What happened to the CI? :D It's bloody red.

jodator · 2020-02-27T08:25:02Z

What happened to the CI? :D It's bloody red.

@Reinmar #monorepo: https://travis-ci.org/ckeditor/ckeditor5/builds/654456582?utm_source=github_status&utm_medium=notification

Reinmar · 2020-02-27T08:59:08Z

@Reinmar #monorepo: https://travis-ci.org/ckeditor/ckeditor5/builds/654456582?utm_source=github_status&utm_medium=notification

Why have I asked? :D

Reinmar · 2020-02-27T09:38:49Z

OK, some more changes are needed, unfortunately. This time much smaller, though. I missed the fact that we should keep the model and view APIs as similar as possible. The following changes will ensure that:

model Node:
- Let's remove the #document getter. From what I can see, it's only used in the correct way on the root elements (e.g. position.root.document). On all other elements, since it can be null when the node is not attached, it's used for a single purpose – to check whether the node is attached. In other words – it's used in a non-idiomatic way.
  
  Note: to have it symmetrical with the view, where the #document is always available, we'd have to do huge changes in the model too, so that's why it's just easier to remove it for now (as it has no usefull applications here anyway).
- Let's introduce an isAttached() method for checking whether node is in a tree rooted in an element of the root type. It'll be a shorthand to this.root.is( 'rootElement' ).
  
  Note: Make sure to document that a model node which was moved to the graveyard is still attached. Make sure to test it too.
model RootElement
- Let's keep the #document here. As I mentioned above, it's used in many places and it makes sense there.
view Node:
- Let's add isAttached() and make it work the same way it works in the model – check whether it's in a root which is a root: this.root.is( 'rootEditableElement' ).
Note: nodes attached to a document fragment are not "attached". So "attached" means – attached to a document via one of its roots.
Finally, we need to review those places where #document was used before for some checks (one of these places is what @jodator found) and replace those with isAttached() (if that makes sense).

…sses.

pomek · 2020-02-28T12:00:53Z

Done. Ready for review once again. New CI => https://travis-ci.org/ckeditor/ckeditor5/builds/656226522.

Reinmar · 2020-03-02T14:26:21Z

src/model/node.js

@@ -189,20 +189,12 @@ export default class Node {
 	}

 	/**
-	 * {@link module:engine/model/document~Document Document} that owns this node or `null` if the node has no parent or is inside
-	 * a {@link module:engine/model/documentfragment~DocumentFragment DocumentFragment}.
+	 * Returns true if a node is in a tree rooted in an element of the root type.


Suggested change

* Returns true if a node is in a tree rooted in an element of the root type.

* Returns true if the node is in a tree rooted in the document

* (is a descendant of one of its roots).

Reinmar · 2020-03-02T14:27:24Z

src/view/node.js

@@ -109,19 +118,12 @@ export default class Node {
 	}

 	/**
-	 * {@link module:engine/view/document~Document View document} that owns this node, or `null` if the node is inside
-	 * {@link module:engine/view/documentfragment~DocumentFragment document fragment}.
+	 * Returns true if a node is in a tree rooted in an element of the root type.


Suggested change

* Returns true if a node is in a tree rooted in an element of the root type.

* Returns true if the node is in a tree rooted in the document

* (is a descendant of one of its roots).

Reinmar · 2020-03-02T14:28:34Z

src/view/placeholder.js

-
-	// The element was removed from document.
-	if ( !doc ) {
+export function needsPlaceholder( elementOrDocumentFragment ) {


Why these changes?

I can see that we should use here isAttached() and that we don't need the type change.

Reinmar · 2020-03-02T14:30:00Z

src/controller/datacontroller.js

 	 * @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 ) {


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

Because dataProcessor is not required. Following the docs:

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

There is also a test that says the same.

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

Reinmar · 2020-03-02T14:31:14Z

src/controller/datacontroller.js

+	/**
+	 * Adds a style processor normalization rules.
+	 *
+	 * The available style processors:


Suggested change

* The available style processors:

* You can implement your own rules as well as use one of the available processor rules:

Reinmar · 2020-03-02T14:31:16Z

src/controller/datacontroller.js

+	/**
+	 * Adds a style processor normalization rules.
+	 *
+	 * The available style processors:


Suggested change

* The available style processors:

* You can implement your own rules as well as use one of the available processor rules:

Reinmar · 2020-03-03T12:10:33Z

src/model/selection.js

@@ -829,7 +829,9 @@ function isUnvisitedBlock( element, visited ) {

 	visited.add( element );

-	return element.document.model.schema.isBlock( element ) && element.parent;
+	const document = element.is( 'rootElement' ) ? element.document : element.root.document;


That's not necessary. element.root.document will always work, even if element is a root.

Reinmar · 2020-03-03T12:48:21Z

src/view/downcastwriter.js

@@ -1807,7 +1810,7 @@ function breakTextNode( position ) {
 	position.parent._data = position.parent.data.slice( 0, position.offset );

 	// Insert new text node after position's parent text node.
-	position.parent.parent._insertChild( position.parent.index + 1, new Text( textToMove ) );
+	position.parent.parent._insertChild( position.parent.index + 1, new Text( position.parent.document, textToMove ) );


position.parent is slow. position.root will be much better here.

Reinmar · 2020-03-03T13:09:36Z

OK, I'm done reviewing this PR :) Now, need to check the other ones :O

pomek and others added 21 commits February 17, 2020 12:06

Added "#document" property to "view.Node" (and inherited classes) and…

09acfe9

… "view.DocumentFragment".

Corrected view.DowncastWriter() tests.

3ffb904

Added #document property to view.UpcastWriter().

2d99f8f

Adjusted tests and fixed "view.Text#_clone()" and "view.Element#_clon…

414f1c1

…e()" methods.

Reverted invalid changes.

2f4357f

Assign parent element document to the inserted children.

3b361f8

Cannot based on checking the "#document" property on view elements be…

c7e0226

…cause it always be defined.

DomConverter.domToView() requires an instance of view.Document as a f…

9831535

…irst argument.

Removed obsolete and invalid code.

88b9089

Use more precise name for an argument in the "needsPlaceholder()" fun…

034b044

…ction.

Adjusted all tests.

8af8720

Added test for checking whether #document property is being updated a…

1fdcb8b

…fter inserting a children to an element.

Improved writers constructor docs.

dd78af1

StylesProcessor is not singleton anymore.

8b56db1

Styles Processor must be unique across the entire editor instance.

8681e0a

Updated tests to new APIs.

a23ad5d

Update missing StylesProcessor instances in constructors.

5145ce0

Add missing StylesProcessor instance.

821ae00

Aligned to changes in Editor class.

2ce8492

Aligned tests to changes in API.

e2ed99c

Simplified the code.

d431e83

jodator approved these changes Feb 26, 2020

View reviewed changes

Merge branch 'master' into i/6091

5a4ab82

pomek added 2 commits February 28, 2020 13:00

Removed the 'model.Node#document.' property.

21390a9

Added a new method (#isAttached()) to both (view and model) Nodes cla…

8039a8d

…sses.

pomek mentioned this pull request Mar 2, 2020

Math plugin won't work after introducing #document property to DomConverter (and other classes) ckeditor/ckeditor5#6362

Closed

Reinmar reviewed Mar 2, 2020

View reviewed changes

Reinmar added 2 commits March 3, 2020 12:31

Removed the dataProcessor argument, as it was never used in practice.

b21a415

Docs.

054d8a2

Reinmar reviewed Mar 3, 2020

View reviewed changes

Docs and code simplification.

13dd24c

Reinmar reviewed Mar 3, 2020

View reviewed changes

Reinmar added 2 commits March 3, 2020 13:49

Docs and other improvements.

16301a1

Docs and other improvements.

fb8e77a

Reinmar merged commit 0e2f02e into master Mar 3, 2020

Reinmar deleted the i/6091 branch March 3, 2020 13:34

This was referenced Mar 16, 2020

Certain file pasted from word crashes in title feature sample (possibly in others) ckeditor/ckeditor5#6427

Closed

Added missing view.Document to ViewDocumentFragment ckeditor/ckeditor5-heading#141

Merged

mlewand mentioned this pull request Mar 16, 2020

[CKEditor 5] Upcoming editor v18.0.0 release has breaking changes that will crash MathType wiris/html-integrations#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StylesProcessor is no longer singleton #1826

StylesProcessor is no longer singleton #1826

pomek commented Feb 20, 2020 •

edited by Reinmar

Loading

jodator left a comment

coveralls commented Feb 26, 2020 •

edited

Loading

Reinmar commented Feb 26, 2020

jodator commented Feb 27, 2020

Reinmar commented Feb 27, 2020

Reinmar commented Feb 27, 2020

pomek commented Feb 28, 2020 •

edited

Loading

Reinmar Mar 2, 2020

Reinmar Mar 2, 2020

Reinmar Mar 2, 2020 •

edited

Loading

Reinmar Mar 3, 2020

Reinmar Mar 2, 2020

pomek Mar 2, 2020

pomek Mar 2, 2020

Reinmar Mar 3, 2020

Reinmar Mar 2, 2020

Reinmar Mar 2, 2020

Reinmar Mar 3, 2020

Reinmar Mar 3, 2020

Reinmar commented Mar 3, 2020

	* Returns true if a node is in a tree rooted in an element of the root type.
	* Returns true if the node is in a tree rooted in the document
	* (is a descendant of one of its roots).

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

StylesProcessor is no longer singleton #1826

StylesProcessor is no longer singleton #1826

Conversation

pomek commented Feb 20, 2020 • edited by Reinmar Loading

jodator left a comment

Choose a reason for hiding this comment

coveralls commented Feb 26, 2020 • edited Loading

Reinmar commented Feb 26, 2020

jodator commented Feb 27, 2020

Reinmar commented Feb 27, 2020

Reinmar commented Feb 27, 2020

pomek commented Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar Mar 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Mar 3, 2020

pomek commented Feb 20, 2020 •

edited by Reinmar

Loading

coveralls commented Feb 26, 2020 •

edited

Loading

pomek commented Feb 28, 2020 •

edited

Loading

Reinmar Mar 2, 2020 •

edited

Loading