Parcourir la source

Merge pull request #865 from ckeditor/t/862

Moved transferring markers from writer to batch
Piotr Jasiun il y a 8 ans
Parent
commit
e4acb48390

+ 23 - 0
packages/ckeditor5-engine/src/model/delta/insertdelta.js

@@ -10,8 +10,14 @@
 import Delta from './delta';
 import DeltaFactory from './deltafactory';
 import RemoveDelta from './removedelta';
+import MarkerDelta from './markerdelta';
 import { register } from '../batch';
 import InsertOperation from '../operation/insertoperation';
+import MarkerOperation from '../operation/markeroperation';
+
+import DocumentFragment from '../documentfragment';
+import Range from '../../model/range.js';
+import Position from '../../model/position.js';
 
 /**
  * @classdesc
@@ -82,6 +88,9 @@ export default class InsertDelta extends Delta {
 /**
  * Inserts a node or nodes at the given position.
  *
+ * When inserted element is a {@link engine/model/documentfragment~DocumentFragment} and has markers its markers will
+ * be set to {@link engine/model/document~Document#markers}.
+ *
  * @chainable
  * @method module:engine/model/batch~Batch#insert
  * @param {module:engine/model/position~Position} position Position of insertion.
@@ -95,6 +104,20 @@ register( 'insert', function( position, nodes ) {
 	delta.addOperation( insert );
 	this.document.applyOperation( insert );
 
+	// When element is a DocumentFragment we need to move its markers to Document#markers.
+	if ( nodes instanceof DocumentFragment ) {
+		for ( const [ markerName, markerRange ] of nodes.markers ) {
+			// We need to migrate marker range from DocumentFragment to Document.
+			const rangeRootPosition = Position.createAt( markerRange.root );
+			const range = new Range(
+				markerRange.start._getCombined( rangeRootPosition, position ),
+				markerRange.end._getCombined( rangeRootPosition, position )
+			);
+
+			this.setMarker( markerName, range );
+		}
+	}
+
 	return this;
 } );
 

+ 4 - 27
packages/ckeditor5-engine/src/model/writer.js

@@ -11,11 +11,9 @@ import Node from './node';
 import Text from './text';
 import TextProxy from './textproxy';
 import Range from './range';
-import Position from './position';
 import DocumentFragment from './documentfragment';
 import NodeList from './nodelist';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
-import log from '@ckeditor/ckeditor5-utils/src/log';
 
 /**
  * Contains functions used for composing model tree, grouped together under "model writer" name. Those functions
@@ -47,20 +45,16 @@ export default writer;
 /**
  * Inserts given nodes at given position.
  *
- * When inserted element has {@link engine/model/markercollection~MarkersCollection} and is being inserted
- * to the element which root also has {@link engine/model/markercollection~MarkersCollection} then markers will
- * be transferred as well.
- *
  * @function module:engine/model/writer~writer.insert
  * @param {module:engine/model/position~Position} position Position at which nodes should be inserted.
  * @param {module:engine/model/node~NodeSet} nodes Nodes to insert.
  * @returns {module:engine/model/range~Range} Range spanning over inserted elements.
  */
 export function insert( position, nodes ) {
-	const normalizedNodes = normalizeNodes( nodes );
+	nodes = normalizeNodes( nodes );
 
 	// We have to count offset before inserting nodes because they can get merged and we would get wrong offsets.
-	const offset = normalizedNodes.reduce( ( sum, node ) => sum + node.offsetSize, 0 );
+	const offset = nodes.reduce( ( sum, node ) => sum + node.offsetSize, 0 );
 	const parent = position.parent;
 
 	// Insertion might be in a text node, we should split it if that's the case.
@@ -69,29 +63,12 @@ export function insert( position, nodes ) {
 
 	// Insert nodes at given index. After splitting we have a proper index and insertion is between nodes,
 	// using basic `Element` API.
-	parent.insertChildren( index, normalizedNodes );
+	parent.insertChildren( index, nodes );
 
 	// Merge text nodes, if possible. Merging is needed only at points where inserted nodes "touch" "old" nodes.
-	_mergeNodesAtIndex( parent, index + normalizedNodes.length );
+	_mergeNodesAtIndex( parent, index + nodes.length );
 	_mergeNodesAtIndex( parent, index );
 
-	// If given element is a DocumentFragment and has markers.
-	if ( nodes instanceof DocumentFragment && nodes.markers.size ) {
-		// If node is being inserted to the element attached to Document or element which root element is DocumentFragment.
-		const targetElement = position.root.document || position.root;
-
-		if ( targetElement.markers ) {
-			// We need to transfer its markers and update position markers positions.
-			for ( const marker of nodes.markers ) {
-				const range = new Range( new Position( parent, marker[ 1 ].start.path ),  new Position( parent, marker[ 1 ].end.path ) );
-				targetElement.markers.set( marker[ 0 ], range );
-			}
-		// Otherwise we need to show warning about losing markers.
-		} else {
-			log.warn( 'model-writer-insert-lose-markers: Element containing markers is set to element without MarkersCollection.' );
-		}
-	}
-
 	return new Range( position, position.getShiftedBy( offset ) );
 }
 

+ 36 - 1
packages/ckeditor5-engine/tests/model/delta/insertdelta.js

@@ -5,15 +5,20 @@
 
 import Document from '../../../src/model/document';
 import Element from '../../../src/model/element';
-import Position from '../../../src/model/position';
+import DocumentFragment from '../../../src/model/documentfragment';
 import Text from '../../../src/model/text';
+import Position from '../../../src/model/position';
+import Range from '../../../src/model/range';
 
 import InsertOperation from '../../../src/model/operation/insertoperation';
+import MarkerOperation from '../../../src/model/operation/markeroperation';
 import InsertDelta from '../../../src/model/delta/insertdelta';
 
 import RemoveDelta from '../../../src/model/delta/removedelta';
 import RemoveOperation from '../../../src/model/operation/removeoperation';
 
+import { stringify } from '../../../src/dev-utils/model';
+
 describe( 'Batch', () => {
 	let doc, root, batch, p, ul, chain;
 
@@ -52,6 +57,36 @@ describe( 'Batch', () => {
 
 			expect( doc.applyOperation.calledWith( correctDeltaMatcher ) ).to.be.true;
 		} );
+
+		it( 'should transfer markers from given DocumentFragment', () => {
+			const documentFragment = new DocumentFragment( [ new Element( 'li', null, [ new Text( 'foo bar' ) ] ) ] );
+			const marker = new Range( new Position( documentFragment, [ 0, 1 ] ), new Position( documentFragment, [ 0, 5 ] ) );
+
+			documentFragment.markers.set( 'marker', marker );
+
+			batch.insert( new Position( root, [ 3, 0 ] ), documentFragment );
+
+			expect( Array.from( doc.markers ).length ).to.equal( 1 );
+			expect( stringify( root, doc.markers.get( 'marker' ).getRange() ) ).to.equal( 'ab<p></p><ul><li>f[oo b]ar</li></ul>c' );
+		} );
+
+		it( 'should set each marker as separate operation', () => {
+			sinon.spy( doc, 'applyOperation' );
+
+			const documentFragment = new DocumentFragment( [ new Element( 'li', null, [ new Text( 'foo bar' ) ] ) ] );
+			const marker1 = new Range( new Position( documentFragment, [ 0, 1 ] ), new Position( documentFragment, [ 0, 2 ] ) );
+			const marker2 = new Range( new Position( documentFragment, [ 0, 5 ] ), new Position( documentFragment, [ 0, 6 ] ) );
+
+			documentFragment.markers.set( 'marker1', marker1 );
+			documentFragment.markers.set( 'marker2', marker2 );
+
+			batch.insert( new Position( root, [ 3, 0 ] ), documentFragment );
+
+			expect( doc.applyOperation.calledThrice );
+			expect( doc.applyOperation.firstCall.calledWith( sinon.match( ( operation ) => operation instanceof InsertOperation ) ) );
+			expect( doc.applyOperation.secondCall.calledWith( sinon.match( ( operation ) => operation instanceof MarkerOperation ) ) );
+			expect( doc.applyOperation.thirdCall.calledWith( sinon.match( ( operation ) => operation instanceof MarkerOperation ) ) );
+		} );
 	} );
 } );
 

+ 1 - 56
packages/ckeditor5-engine/tests/model/writer.js

@@ -11,10 +11,9 @@ import TextProxy from '../../src/model/textproxy';
 import Position from '../../src/model/position';
 import Range from '../../src/model/range';
 import writer from '../../src/model/writer';
-import { getData, stringify } from '../../src/dev-utils/model';
+import { getData } from '../../src/dev-utils/model';
 
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
-import log from '@ckeditor/ckeditor5-utils/src/log';
 
 let doc, root;
 
@@ -55,60 +54,6 @@ describe( 'writer', () => {
 
 			expectData( 'foo<$text bold="true">xxxbar</$text><image src="img.jpg"></image>xyz' );
 		} );
-
-		it( 'should transfer markers from given node to the root element of target position', () => {
-			root.appendChildren( [ new Element( 'div' ) ] );
-
-			const docFrag = new DocumentFragment( [
-				new Element( 'paragraph', null, [
-					new Text( 'foo bar' ),
-				] )
-			] );
-
-			docFrag.markers.set( 'foo', new Range( new Position( docFrag, [ 0, 2 ] ), new Position( docFrag, [ 0, 6 ] ) ) );
-
-			writer.insert( new Position( root, [ 10, 0 ] ), docFrag );
-
-			expect( stringify( root, doc.markers.get( 'foo' ).getRange() ) ).to.equal(
-				'foo<$text bold="true">bar</$text><image src="img.jpg"></image>xyz<div><paragraph>fo[o ba]r</paragraph></div>'
-			);
-		} );
-
-		it( 'should transfer markers from given node to the root element of target position when root is a documentFragment', () => {
-			const targetDocFrag = new DocumentFragment( [ new Element( 'div' ) ] );
-
-			const docFrag = new DocumentFragment( [
-				new Element( 'paragraph', null, [
-					new Text( 'foo bar' ),
-				] )
-			] );
-
-			docFrag.markers.set( 'foo', new Range( new Position( docFrag, [ 0, 2 ] ), new Position( docFrag, [ 0, 6 ] ) ) );
-
-			writer.insert( new Position( targetDocFrag, [ 0, 0 ] ), docFrag );
-
-			expect( stringify( targetDocFrag, targetDocFrag.markers.get( 'foo' ) ) )
-				.to.equal( '<div><paragraph>fo[o ba]r</paragraph></div>' );
-		} );
-
-		it( 'should log warning when element with markers is set to the element without markers collection', () => {
-			const warnSpy = sinon.spy( log, 'warn' );
-			const target = new Element( 'div' );
-			const docFrag = new DocumentFragment( [
-				new Element( 'paragraph', null, [
-					new Text( 'foo bar' ),
-				] )
-			] );
-			const range = new Range( new Position( docFrag, [ 1, 2 ] ), new Position( docFrag, [ 1, 4 ] ) );
-
-			docFrag.markers.set( 'foo', range );
-
-			writer.insert( new Position( target, [ 0 ] ), docFrag );
-
-			expect( warnSpy.calledWithExactly(
-				'model-writer-insert-lose-markers: Element containing markers is set to element without MarkersCollection.'
-			) ).to.true;
-		} );
 	} );
 
 	describe( 'remove', () => {