Browse Source

Changed: Removed harmful listener in markers remove conversion.
Changed: Minor refactor tweaks and doc tweaks.

Szymon Cofalik 8 years ago
parent
commit
1acbebba21

+ 2 - 2
packages/ckeditor5-engine/src/controller/datacontroller.js

@@ -162,8 +162,8 @@ export default class DataController {
 		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.
+			// Then, if a document element is converted, convert markers.
+			// From all document markers, get those, which "intersect" with the converter element.
 			const markers = _getMarkersRelativeToElement( modelElementOrFragment );
 
 			for ( const [ name, range ] of markers ) {

+ 2 - 9
packages/ckeditor5-engine/src/controller/editingcontroller.js

@@ -109,11 +109,10 @@ export default class EditingController {
 
 		// Convert markers removal.
 		//
-		// Markers should be removed from the view before changes to model are applied. This is because otherwise
-		// it would be impossible to map some markers to view (if, for example, marker boundary parent got removed).
+		// Markers should be removed from the view before changes to the model are applied. This is because otherwise
+		// it would be impossible to map some markers to the view (if, for example, the marker's 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.)
 		const removedMarkers = new Set();
 
 		this.listenTo( model, 'applyOperation', ( evt, args ) => {
@@ -139,12 +138,6 @@ export default class EditingController {
 			}
 		}, { 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`).
-		this.listenTo( model.markers, 'add', ( evt, marker ) => {
-			removedMarkers.delete( marker.name );
-		} );
-
 		// If a marker is removed through `model.Model#markers` directly (not through operation), just remove it (if
 		// it was not removed already).
 		this.listenTo( model.markers, 'remove', ( evt, marker ) => {

+ 9 - 9
packages/ckeditor5-engine/src/model/writer.js

@@ -175,7 +175,7 @@ export default class Writer {
 			}
 		}
 
-		const version = position.root.document ? this.model.document.version : null;
+		const version = position.root.document ? position.root.document.version : null;
 
 		const insert = new InsertOperation( position, item, version );
 
@@ -451,7 +451,7 @@ export default class Writer {
 		const delta = new MoveDelta();
 		this.batch.addDelta( delta );
 
-		const version = range.root.document ? this.model.document.version : null;
+		const version = range.root.document ? range.root.document.version : null;
 
 		const operation = new MoveOperation( range.start, range.end.offset - range.start.offset, position, version );
 		delta.addOperation( operation );
@@ -525,7 +525,7 @@ export default class Writer {
 		const positionAfter = Position.createFromParentAndOffset( nodeAfter, 0 );
 		const positionBefore = Position.createFromParentAndOffset( nodeBefore, nodeBefore.maxOffset );
 
-		const moveVersion = position.root.document ? this.model.document.version : null;
+		const moveVersion = position.root.document ? position.root.document.version : null;
 
 		const move = new MoveOperation(
 			positionAfter,
@@ -564,7 +564,7 @@ export default class Writer {
 		const delta = new RenameDelta();
 		this.batch.addDelta( delta );
 
-		const version = element.root.document ? this.model.document.version : null;
+		const version = element.root.document ? element.root.document.version : null;
 
 		const renameOperation = new RenameOperation( Position.createBefore( element ), element.name, newName, version );
 		delta.addOperation( renameOperation );
@@ -597,7 +597,7 @@ export default class Writer {
 		}
 
 		const copy = new Element( splitElement.name, splitElement.getAttributes() );
-		const insertVersion = splitElement.root.document ? this.model.document.version : null;
+		const insertVersion = splitElement.root.document ? splitElement.root.document.version : null;
 
 		const insert = new InsertOperation(
 			Position.createAfter( splitElement ),
@@ -665,7 +665,7 @@ export default class Writer {
 		const delta = new WrapDelta();
 		this.batch.addDelta( delta );
 
-		const insertVersion = range.root.document ? this.model.document.version : null;
+		const insertVersion = range.root.document ? range.root.document.version : null;
 
 		const insert = new InsertOperation( range.end, element, insertVersion );
 		delta.addOperation( insert );
@@ -706,7 +706,7 @@ export default class Writer {
 		this.batch.addDelta( delta );
 
 		const sourcePosition = Position.createFromParentAndOffset( element, 0 );
-		const moveVersion = sourcePosition.root.document ? this.model.document.version : null;
+		const moveVersion = sourcePosition.root.document ? sourcePosition.root.document.version : null;
 
 		const move = new MoveOperation(
 			sourcePosition,
@@ -952,9 +952,9 @@ function addRemoveOperation( position, howMany, delta, model ) {
 
 	if ( position.root.document ) {
 		const doc = model.document;
-		const gyPosition = new Position( doc.graveyard, [ 0 ] );
+		const graveyardPosition = new Position( doc.graveyard, [ 0 ] );
 
-		operation = new RemoveOperation( position, howMany, gyPosition, doc.version );
+		operation = new RemoveOperation( position, howMany, graveyardPosition, doc.version );
 	} else {
 		operation = new DetachOperation( position, howMany );
 	}

+ 28 - 0
packages/ckeditor5-engine/tests/controller/editingcontroller.js

@@ -524,6 +524,34 @@ describe( 'EditingController', () => {
 
 			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>aaaf<span>o</span>o</p><p>bar</p>' );
 		} );
+
+		it( 'should not crash if marker is removed, added and removed #1', () => {
+			model.change( writer => {
+				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) );
+			} );
+
+			model.change( writer => {
+				writer.insertText( 'a', p1, 0 );
+				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 3, p1, 4 ) );
+				writer.insertText( 'a', p1, 0 );
+			} );
+
+			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>aafo<span>o</span></p><p>bar</p>' );
+		} );
+
+		it( 'should not crash if marker is removed, added and removed #2', () => {
+			model.change( writer => {
+				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) );
+			} );
+
+			model.change( writer => {
+				writer.removeMarker( 'marker' );
+				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 0, p1, 1 ) );
+				writer.removeMarker( 'marker' );
+			} );
+
+			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
+		} );
 	} );
 
 	describe( 'destroy()', () => {

+ 1 - 4
packages/ckeditor5-engine/tests/model/liverange.js

@@ -694,11 +694,8 @@ describe( 'LiveRange', () => {
 	describe( 'should not get transformed and not fire change event if', () => {
 		let otherRoot, spy, live, clone;
 
-		before( () => {
-			otherRoot = doc.createRoot( '$root', 'otherRoot' );
-		} );
-
 		beforeEach( () => {
+			otherRoot = doc.createRoot( '$root', 'otherRoot' );
 			live = new LiveRange( new Position( root, [ 0, 1, 4 ] ), new Position( root, [ 0, 2, 2 ] ) );
 			clone = Range.createFromRange( live );