Introduced markers serialization #845

oskarwrobel · 2017-02-26T11:14:26Z

Suggested merge commit message (convention)

Feature: Introduced markers serialization. Closes ckeditor/ckeditor5#3961. Closes ckeditor/ckeditor5#4003.

BREAKING CHANGES: BuildModelConverter#fromMarkerCollapsed is removed. Use BuildModelConverter#fromMarker instead. Format of data returned by ViewConversionDispatcher#convert has changed from single model item to object with model item and list of static markers.

NOTE: insertUIElement model to view converter now supports collapsed and non-collapsed ranges.

Related to: ckeditor/ckeditor5-clipboard#15

Follow-ups:

Fixed https://github.com/ckeditor/ckeditor5-engine/issues/846 - Create diffrent opening and closing element in marker conversion

scofalik · 2017-02-27T12:48:10Z

src/controller/datacontroller.js

@@ -180,7 +183,10 @@ export default class DataController {

 /**
 * Sets input data parsed by the {@link #processor data processor} and
- * converted by the {@link #viewToModel view to model converters}.
+ * converted by the {@link #viewToModel view to model converters}. When markers where converted


scofalik · 2017-02-27T12:48:26Z

src/controller/datacontroller.js

@@ -180,7 +183,10 @@ export default class DataController {

 /**
 * Sets input data parsed by the {@link #processor data processor} and
- * converted by the {@link #viewToModel view to model converters}.
+ * converted by the {@link #viewToModel view to model converters}. When markers where converted
+ * from view to model as temporary {@link module:engine/model/element/~Element model elements} then those element


scofalik · 2017-02-27T12:53:12Z

src/controller/datacontroller.js

+ markerData.endPath ? new ModelPosition( modelRoot, markerData.endPath ) : null
+ );
+
+ batch.setMarker( this.model.markers.set( markerName, range ) );


AFAIR you can directly use setMarker - no need to add that marker to MarkerCollection first. MarkerOperation, upon execution, will do it.

scofalik · 2017-02-27T12:58:25Z

src/controller/datacontroller.js

@@ -306,6 +335,57 @@ export default class DataController {

 mix( DataController, EmitterMixin );

+// Traverses given DocumentFragment and searches elements which marks marker range. Founded element is removed from


scofalik · 2017-02-27T12:58:56Z

src/controller/datacontroller.js

@@ -306,6 +335,57 @@ export default class DataController {

 mix( DataController, EmitterMixin );

+// Traverses given DocumentFragment and searches elements which marks marker range. Founded element is removed from
+// DocumentFragment but path of this element is stored in Map.


in a Map which is then returned.

scofalik · 2017-02-27T13:10:33Z

src/controller/datacontroller.js

+//
+// @param {module:engine/view/documentfragment~DocumentFragment} documentFragment Model DocumentFragment.
+// @returns {Object} Object with markers data and cleaned up document fragment.
+function extractMarkersFromModelFragment( documentFragment ) {


To simplify this function and avoid refreshing walker, you could simply first find all elements to be extracted, and then remove them one by one after tree walker already did it's job:

const markerStamps = new Set(); // ... for ( const value of walker ) { // ... markerStamps.add( value.item ) // ... } for ( const stamp of markerStamps ) { // ... }

AFAIR, Set remembers the order in which items where stored.

Also, since documentFragment is passed by reference, you only need to return the markers. It will be more elegant to use.

Finally, this could be a private method too, but I also have this "problem" of using sometimes private "static-like" methods and sometimes not exported functions, so as you wish :)

Also, since documentFragment is passed by reference, you only need to return the markers. It will be more elegant to use.

That's how it looked in my first implementation but I assumed that it'll be more readable to return data.

scofalik · 2017-02-27T14:44:46Z

src/conversion/model-to-view-converters.js

+ viewWriter.insert( mapper.toViewPosition( data.range.start ), viewElement );
+
+ if ( !data.range.isCollapsed ) {
+ viewWriter.insert( mapper.toViewPosition( data.range.end ), viewElement.clone( true ) );


We've been discussing that it would be nice if closing element is different than opening element, so this information would have to be send to creator and two view elements would have to be created.

If that is not too complicated, it would be nice to have this, but it is not a strict requirement. Take both model->view and view->model conversion into consideration.

Follow-up: https://github.com/ckeditor/ckeditor5-engine/issues/846

scofalik · 2017-02-27T14:49:53Z

Changed proposed commit message from

NOTE: BuildModelConverter#fromMarker and BuildModelConverter#fromCollapsedMarker are unified, insertUIElement method of model -> view converter is enhanced to supports collapsed and non-collapsed ranges.

to

BREAKING CHANGES: BuildModelConverter#fromMarker and #fromCollapsedMarker are unified, and the latter is removed.
NOTE: insertUIElement model->view converter now supports collapsed and non-collapsed ranges.

Reinmar · 2017-02-27T15:02:53Z

I added missing empty line between both blocks :D

pjasiun · 2017-02-27T16:21:44Z

src/controller/datacontroller.js

+ }
+
+ // Walk through collected marker elements store its path and remove its from the DocumentFragment.
+ return Array.from( markerStamps ).reduce( ( result, stamp ) => {


Iterators FTW!

For var i = 0; i < length; ++i FTW!

I was really curious what will you say guys about this reduce :)

scofalik · 2017-02-27T16:25:54Z

src/controller/datacontroller.js

+ }
+
+ // Walk through collected marker elements store its path and remove its from the DocumentFragment.
+ return Array.from( markerStamps ).reduce( ( result, stamp ) => {


Loops FTW!

On serious note, this looks overcomplicated. Simple loop over markerStamps would be okay too.

Anyway, the whole function should be moved from the DataController to conversion view dispatcher or view converter helper.

pjasiun · 2017-02-27T16:28:37Z

src/controller/datacontroller.js

+ * converted by the {@link #viewToModel view to model converters}. If markers where converted
+ * from view to model as temporary {@link module:engine/model/element/~Element model elements} then those elements
+ * will be removed from parsed {@link module:engine/model/element/~DocumentFragment} and added to the
+ * {@link module:engine/model/document~Document#markers markers collection}.


This should not be here. All what DataController is, is a facade for the complex API. I should not have any complex logic because quickly it became uber-class.

Especially since now parse does not return any useful data, and the whole processing is done outside this function.

pjasiun · 2017-02-27T16:33:26Z

src/controller/datacontroller.js

 * @fires insertContent
 * @param {module:engine/model/documentfragment~DocumentFragment} content The content to insert.
 * @param {module:engine/model/selection~Selection} selection Selection into which the content should be inserted.
 * @param {module:engine/model/batch~Batch} [batch] Batch to which deltas will be added. If not specified, then
 * changes will be added to a new batch.
 */
 insertContent( content, selection, batch ) {
+ extractMarkersFromModelFragment( content );


Do we need to extract anything on insert content?

Why is it here? Any clean up should be done beforehand, using the clipboard events (#paste or #clipboardInput).

Reinmar · 2017-02-27T16:36:31Z

This branch has conflicts! Git FTW!

pjasiun · 2017-03-01T15:01:58Z

src/controller/datacontroller.js

@@ -198,10 +198,28 @@ export default class DataController {
 this.model.selection.removeAllRanges();
 this.model.selection.clearAttributes();

+ // Parse data to model and extract markers from parsed document fragment.
+ const { modelItem, markersData } = this.parse( data );


I was thinking a while about the return format. I don't like the code bellow needed to translate markersData to document.makers. It's not short nor simple, and you will have to write it every time you need a custom handling of parsed data. Of course, we could have a helper function, but where-ever we put such function it will be ugly. Then I thought that maybe markers should be returned as MarkersCollection.

At the same time, it's strange that you can not return one object representing a parsed fragment of the document, something like... DocumentFragment?

Markers are integrated part of the document data. They are not like selection or history, they are more like attributes or children, so maybe we should have DocumentFragment.makers? Then, when you add document fragment to another document, makers from it should be taken and added to the parent document, the same way like children are moved to the parent element. It could be done by the model.writer or element.insert (I hope soon we will get rid off model.writer anyway).

After this change, the code here will not change much.

I know I am usually the one who is against magic, but the possibility of having all data in one object is worth trying.

I like this idea. Converter still will be returning Object with conversion result and markers data, but parser will merge it together into DocumentFragment with its own MarkersCollection ?

Creating the real MarkersCollection on DocumentFragment might be more compilated because Marker keeps LiveRange inside but LiveRange work only for Document. We can:

try to improve DocumentFragment and LiveRange to work each other but it might be overengineering.

don't create the real MarkersCollection on DocumentFragmentbut only something to store markers data. I don't like it because every change on DocFrag will make markers positions out of date.

pjasiun · 2017-03-02T10:23:06Z

src/conversion/buildviewconverter.js

+ * @param {Function} [creator] Creator function.
+ */
+ toMarker( creator ) {
+ const eventCallbackGen = function( from ) {


Why not function eventCallbackGen( from )?

I'm also for named function instead of anonymous function assigned to const, but this is consistent with the other methods. I'll fix it.

pjasiun · 2017-03-02T10:24:44Z

src/conversion/viewconversiondispatcher.js

+ const walker = new ModelTreeWalker( {
+ startPosition: ModelPosition.createAt( modelItem, 0 ),
+ ignoreElementEnd: true,
+ shallow: false


AFAIR shallow is false by default.

…apsed ranges.

pjasiun · 2017-03-06T10:28:14Z

src/controller/datacontroller.js

@@ -236,7 +237,19 @@ export default class DataController {
 * @returns {module:engine/model/documentfragment~DocumentFragment} Output document fragment.
 */
 toModel( viewElementOrFragment, context = '$root' ) {
- return this.viewToModel.convert( viewElementOrFragment, { context: [ context ] } );
+ const { conversionResult, markers } = this.viewToModel.convert( viewElementOrFragment, { context: [ context ] } );


I think convert should simply return documentFragmet with markers which can be added instead of transforming it here. DataController should not contain any transformation logic, it should be as simple as possible.

pjasiun · 2017-03-06T10:36:33Z

src/conversion/buildmodelconverter.js

+ * buildModelConverter().for( dispatcher )
+ * .fromMarker( 'search' )
+ * .toStamp( ( data, isOpening ) => {
+ * if ( isOpening ) {


isOpening should be a part of the data (data.isOpening).

pjasiun · 2017-03-06T10:49:15Z

src/conversion/buildmodelconverter.js

+ */
+ toStamp( element ) {
+ for ( let dispatcher of this._dispatchers ) {
+ if ( this._from.type == 'element' || this._from.type == 'attribute' ) {


Why not if ( this._from.type != 'marker' )? If we will introduce a new type it most probably should not be transformed to stamp as well.

pjasiun · 2017-03-06T10:59:22Z

src/model/writer.js

+ } else {
+ log.warn( 'model-writer-insert-lose-markers: Element containing markers is set to element without MarkersCollection.' );
+ }
+ }


Will it work fine if I insert DocumentFragment with markers to another DocumentFragment?

I did a small fix and wrote test for it and it works.

…ment.

…ter.

pjasiun · 2017-03-07T13:47:40Z

src/conversion/viewconversiondispatcher.js

+ // When conversion output is not a Node or Element we just return it.
+ if ( !( conversionResult instanceof ModelNode || conversionResult instanceof ModelDocumentFragment ) ) {
+ return conversionResult;
+ }


I don't get this condition. Is it for text nodes? I think that there is nothing wrong if we will wrap everything in the document fragment.

This is because conversion output doesn't have to be a node. Theoretically it can be any value but we use it to provide converted element. @scofalik knows the details :) I know that it might be confusing especially when there is no explanation in docs.

Yea, I know. It was even my idea, but it was a very theoretical idea. You can remove these tests because at this point it's pretty clear that we will not use dispatcher this way, soon.

Follow-up: https://github.com/ckeditor/ckeditor5-engine/issues/860

pjasiun · 2017-03-07T13:48:44Z

src/conversion/viewconversiondispatcher.js


- return this._convertItem( viewItem, consumable, additionalData );
+ // When conversion output is not a Node or Element we just return it.


This comment is useless. It tells exactly what the next line do. I can read code instead. What comment should do is tell me why this code is needed.

pjasiun · 2017-03-07T13:51:16Z

src/controller/datacontroller.js

 * {@link module:engine/view/documentfragment~DocumentFragment view document fragment} converted by the
- * {@link #viewToModel view to model converters} to a
- * {@link module:engine/model/documentfragment~DocumentFragment model document fragment}.
+ * {@link #viewToModel view to model converters}.


You could add a note to toModel and viewToModel.convert that the document fragment may contain markers.

pjasiun · 2017-03-07T13:51:57Z

src/model/documentfragment.js

@@ -25,6 +29,15 @@ export default class DocumentFragment {
 */
 constructor( children ) {
 /**
+ * DocumentFragment static markers list. This is a list of names and {@link module:engine/model/range~Range ranges}


It's map, not list.

oskarwrobel force-pushed the t/787 branch from 2e41fe6 to 3efd473 Compare February 26, 2017 11:37

oskarwrobel requested a review from scofalik February 27, 2017 12:25

scofalik reviewed Feb 27, 2017

View reviewed changes

scofalik added the status:review- label Feb 27, 2017

oskarwrobel removed the status:review- label Feb 27, 2017

oskarwrobel requested a review from pjasiun February 27, 2017 15:46

pjasiun reviewed Feb 27, 2017

View reviewed changes

scofalik reviewed Feb 27, 2017

View reviewed changes

pjasiun reviewed Feb 27, 2017

View reviewed changes

pjasiun added the status:review- label Feb 27, 2017

oskarwrobel force-pushed the t/787 branch from 3d1e2a2 to 26cd187 Compare February 28, 2017 08:13

oskarwrobel added status:review- and removed status:review- labels Feb 28, 2017

pjasiun reviewed Mar 1, 2017

View reviewed changes

pjasiun reviewed Mar 2, 2017

View reviewed changes

pjasiun added the status:review- label Mar 2, 2017

oskarwrobel added 2 commits March 3, 2017 10:19

Improved insert and remove UIElement to handle collapsed and non-coll…

6b43114

…apsed ranges.

Added test for wrap / unwrap collpased range.

c0c9632

oskarwrobel added 2 commits March 3, 2017 14:23

Added option to create different opening and closing marker stamp.

e4963aa

Added warning about losing markers.

04e6ebf

pjasiun reviewed Mar 6, 2017

View reviewed changes

pjasiun added the status:review- label Mar 6, 2017

oskarwrobel added 8 commits March 6, 2017 12:48

Improved condition checking type of conversion.

a9e0311

Moved isOpening parameter to data object.

93db8f0

Fixed transfering mrkers to work for documentFragment -> documentFrag…

70ba353

…ment.

Improved tests of transfering markers.

8437adc

Changed format of view -> model converter result.

aa0daed

Aligned DataController to new format of view -> model conversion result.

9b268df

Improved output returned by the converter.

ce35560

Aligned tests to new data format returned by the view -> model conver…

e7b6c3d

…ter.

oskarwrobel removed the status:review- label Mar 6, 2017

pjasiun reviewed Mar 7, 2017

View reviewed changes

oskarwrobel added 2 commits March 7, 2017 15:13

Improved docs.

f9abd15

Merge branch 'master' into t/787

e72ab66

oskarwrobel merged commit 2e7f75d into master Mar 7, 2017

oskarwrobel deleted the t/787 branch March 7, 2017 17:16

Reinmar added this to the iteration 9 milestone Mar 7, 2017

This was referenced Oct 9, 2019

Align to latest ckeditor5-engine API ckeditor/ckeditor5#2657

Closed

Align code to latest ckeditor5-engine API ckeditor/ckeditor5#3297

Closed

Create diffrent opening and closing element in marker conversion ckeditor/ckeditor5#4003

Closed

pjasiun mentioned this pull request Oct 9, 2019

Keep all elements in the document a.k.a. One API to rule them all ckeditor/ckeditor5#4009

Closed

oskarwrobel mentioned this pull request Oct 9, 2019

View -> model converter should return only DocumentFragment ckeditor/ckeditor5#4011

Closed

		@@ -306,6 +335,57 @@ export default class DataController {

		mix( DataController, EmitterMixin );

		// Traverses given DocumentFragment and searches elements which marks marker range. Founded element is removed from


		return this._convertItem( viewItem, consumable, additionalData );
		// When conversion output is not a Node or Element we just return it.

Introduced markers serialization #845

Introduced markers serialization #845

Conversation

oskarwrobel commented Feb 26, 2017 • edited by pomek Loading

Suggested merge commit message (convention)

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

scofalik Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

scofalik Feb 27, 2017 • 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

oskarwrobel Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

scofalik commented Feb 27, 2017

Reinmar commented Feb 27, 2017

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

pjasiun Feb 27, 2017 • 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

Reinmar commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarwrobel Mar 1, 2017 • 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

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

oskarwrobel Mar 7, 2017 • 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

oskarwrobel commented Feb 26, 2017 •

edited by pomek

Loading

scofalik Feb 27, 2017 •

edited

Loading

scofalik Feb 27, 2017 •

edited

Loading

oskarwrobel Feb 27, 2017 •

edited

Loading

pjasiun Feb 27, 2017 •

edited

Loading

oskarwrobel Mar 1, 2017 •

edited

Loading

oskarwrobel Mar 7, 2017 •

edited

Loading