T/1211 Operations that do not operate on document should have baseVersion set to null #1220

scofalik · 2017-12-28T17:27:46Z

Suggested merge commit message (convention)

Other: Operations that do not operate on a document should have baseVersion set to null. Closes ckeditor/ckeditor5#4223.
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.

Additional information

This PR bases on #1216. Please close that PR first.

coveralls · 2017-12-28T17:32:22Z

Coverage remained the same at 100.0% when pulling c57548c on t/1211 into aea6119 on master.

coveralls · 2017-12-28T17:41:02Z

Coverage remained the same at 100.0% when pulling f33f579 on t/1211 into aea6119 on master.

pjasiun · 2018-01-05T14:59:13Z

I will wait with the review to close #1216.

coveralls · 2018-01-09T12:30:36Z

Coverage remained the same at 100.0% when pulling f571c52 on t/1211 into 2592bf1 on master.

coveralls · 2018-01-09T15:46:06Z

Coverage remained the same at 100.0% when pulling b6b74e0 on t/1211 into 2592bf1 on master.

coveralls · 2018-01-10T10:55:58Z

Coverage remained the same at 100.0% when pulling 2f51f45 on t/1211 into 2592bf1 on master.

…e version.

…before an operation affecting marker is applied.

coveralls · 2018-01-11T15:43:22Z

Coverage remained the same at 100.0% when pulling f9d3887 on t/1211 into f56bddf on master.

pjasiun · 2018-01-18T12:38:29Z

src/controller/datacontroller.js

 		const modelRange = ModelRange.createIn( modelElementOrFragment );

 		const viewDocumentFragment = new ViewDocumentFragment();
 		this.mapper.bindElements( modelElementOrFragment, viewDocumentFragment );

 		this.modelToView.convertInsert( modelRange );

+		if ( !modelElementOrFragment.is( 'documentFragment' ) ) {
+			// Then, if document element is converted, convert markers.
+			// Get only those markers that contain or are contained by the element.


What other elements do we have?

pjasiun · 2018-01-18T12:41:19Z

src/controller/editingcontroller.js

+		// it would be impossible to map some markers to view (if, for example, marker boundary parent got removed).
+		//
+		// `removedMarkers` keeps information which markers already has been removed to prevent removing them twice.
+		// (The second remove would not be at "the soonest moment" and it could crash.)


(The second remove would not be at "the soonest moment" and it could crash.)
I don't get it.

pjasiun · 2018-01-18T12:44:13Z

src/controller/editingcontroller.js

+					removedMarkers.add( marker.name );
+					this.modelToView.convertMarkerRemove( marker.name, markerRange );
+
+					// TODO: This stinks but this is the safest place to have this code.


You mean all of these listeners? ;)

pjasiun · 2018-01-18T12:46:41Z

src/controller/editingcontroller.js

+		}, { priority: 'high' } );
+
+		// If a marker with given name was added again, remove it from `removedMarkers`. This is to prevent a bug in
+		// a situation when marker is removed, then added, then removed again (would not got removed if it is saved in `removedMarkers`).


Are you sure we need this? If a marker is added, we add it on "changes done", so it should not be removed immediately, only removed from differ.

Wow, good catch! I've added a test and it is true that it is not only unnecessary but it's even harmful.

pjasiun · 2018-01-18T12:52:08Z

src/model/writer.js

@@ -574,7 +564,9 @@ export default class Writer {
 		const delta = new RenameDelta();
 		this.batch.addDelta( delta );

-		const renameOperation = new RenameOperation( Position.createBefore( element ), element.name, newName, this.model.document.version );
+		const version = element.root.document ? this.model.document.version : null;


More natural is: element.root.document ? element.root.document.version : null;

pjasiun · 2018-01-18T12:52:28Z

src/model/writer.js

@@ -605,21 +597,24 @@ export default class Writer {
 		}

 		const copy = new Element( splitElement.name, splitElement.getAttributes() );
+		const insertVersion = splitElement.root.document ? this.model.document.version : null;


splitElement.root.document ? splitElement.root.document.version : null;

pjasiun · 2018-01-18T12:53:06Z

src/model/writer.js

@@ -670,16 +665,20 @@ export default class Writer {
 		const delta = new WrapDelta();
 		this.batch.addDelta( delta );

-		const insert = new InsertOperation( range.end, element, this.model.document.version );
+		const insertVersion = range.root.document ? this.model.document.version : null;


range.root.document ? range.root.document.version : null;

pjasiun · 2018-01-18T12:53:29Z

src/model/writer.js

@@ -707,26 +706,20 @@ export default class Writer {
 		this.batch.addDelta( delta );

 		const sourcePosition = Position.createFromParentAndOffset( element, 0 );
+		const moveVersion = sourcePosition.root.document ? this.model.document.version : null;


sourcePosition.root.document ? sourcePosition.root.document : null;

pjasiun · 2018-01-18T13:05:55Z

src/model/writer.js

+
+	if ( position.root.document ) {
+		const doc = model.document;
+		const gyPosition = new Position( doc.graveyard, [ 0 ] );


graveyardPosition

Changed: Minor refactor tweaks and doc tweaks.

coveralls · 2018-01-18T14:26:40Z

Coverage remained the same at 100.0% when pulling 13bc98c on t/1211 into f56bddf on master.

scofalik force-pushed the t/1211 branch from f33f579 to f571c52 Compare January 9, 2018 12:26

scofalik force-pushed the t/1211 branch from dd47ac0 to b6b74e0 Compare January 9, 2018 15:28

scofalik force-pushed the t/1211 branch from b6b74e0 to 2f51f45 Compare January 10, 2018 10:51

pjasiun added the status:review- label Jan 11, 2018

scofalik added 6 commits January 11, 2018 15:57

Changed: Operations that does not operate on document have null bas…

ebd847b

…e version.

Tests: Added missing tests.

a28dc41

Added: Enabled markers conversion in engine.DataController.

23d44dc

Docs: Minor tweaks.

a61634d

Changed: Markers model-to-view clearing conversion moved to a moment …

cfd513b

…before an operation affecting marker is applied.

Tests: Fixed some tests that were broken after rebase.

040d58b

scofalik force-pushed the t/1211 branch from 2f51f45 to f9d3887 Compare January 11, 2018 15:35

scofalik removed the status:review- label Jan 11, 2018

Tests: Fixed failing test.

f9d3887

Reinmar requested a review from pjasiun January 16, 2018 13:30

This was referenced Jan 16, 2018

T/1214 Consumable type name is now normalized inside conversion.ModelConsumable methods #1241

Merged

T/1239 Allowed passing true as view.Matcher's attribute match to check if that attribute is set #1242

Merged

pjasiun reviewed Jan 18, 2018

View reviewed changes

pjasiun added the status:review- label Jan 18, 2018

Changed: Removed harmful listener in markers remove conversion.

13bc98c

Changed: Minor refactor tweaks and doc tweaks.

scofalik removed the status:review- label Jan 18, 2018

pjasiun merged commit b527d7f into master Jan 18, 2018

pjasiun deleted the t/1211 branch January 18, 2018 14:22

pjasiun mentioned this pull request Oct 9, 2019

Selection should not be converted on _change ckeditor/ckeditor5#4247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/1211 Operations that do not operate on document should have baseVersion set to null #1220

T/1211 Operations that do not operate on document should have baseVersion set to null #1220

scofalik commented Dec 28, 2017 •

edited by pomek

Loading

coveralls commented Dec 28, 2017

coveralls commented Dec 28, 2017

pjasiun commented Jan 5, 2018

coveralls commented Jan 9, 2018

coveralls commented Jan 9, 2018

coveralls commented Jan 10, 2018

coveralls commented Jan 11, 2018

pjasiun Jan 18, 2018

pjasiun Jan 18, 2018

pjasiun Jan 18, 2018

scofalik Jan 18, 2018

pjasiun Jan 18, 2018

scofalik Jan 18, 2018

pjasiun Jan 18, 2018

pjasiun Jan 18, 2018

pjasiun Jan 18, 2018

pjasiun Jan 18, 2018

pjasiun Jan 18, 2018

coveralls commented Jan 18, 2018

T/1211 Operations that do not operate on document should have baseVersion set to null #1220

T/1211 Operations that do not operate on document should have baseVersion set to null #1220

Conversation

scofalik commented Dec 28, 2017 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

coveralls commented Dec 28, 2017

coveralls commented Dec 28, 2017

pjasiun commented Jan 5, 2018

coveralls commented Jan 9, 2018

coveralls commented Jan 9, 2018

coveralls commented Jan 10, 2018

coveralls commented Jan 11, 2018

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

coveralls commented Jan 18, 2018

scofalik commented Dec 28, 2017 •

edited by pomek

Loading