From 7eeb6c7cacf62f33d42bc7ce3b3fd93fc6a884e1 Mon Sep 17 00:00:00 2001 From: Catrope Date: Thu, 10 May 2012 19:28:37 -0700 Subject: [PATCH 1/5] Throw an exception when DocumentFragment gets unbalanced input Change-Id: Ie891bd7ea4d9e9b1c84e7a0390f1af39c0e55fd4 --- modules/ve2/dm/ve.dm.DocumentFragment.js | 4 ++++ tests/ve2/dm/ve.dm.DocumentFragment.test.js | 13 ++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/modules/ve2/dm/ve.dm.DocumentFragment.js b/modules/ve2/dm/ve.dm.DocumentFragment.js index 28c5b5523a..0a14a3d0f1 100644 --- a/modules/ve2/dm/ve.dm.DocumentFragment.js +++ b/modules/ve2/dm/ve.dm.DocumentFragment.js @@ -115,6 +115,10 @@ ve.dm.DocumentFragment = function( data, parentDocument ) { children = stack.pop(); currentStack = parentStack; parentStack = stack[stack.length - 2]; + if ( !parentStack ) { + // This can only happen if we got unbalanced data + throw 'Unbalanced input passed to DocumentFragment'; + } // Attach the children to the node ve.batchSplice( currentNode, 0, 0, children ); } diff --git a/tests/ve2/dm/ve.dm.DocumentFragment.test.js b/tests/ve2/dm/ve.dm.DocumentFragment.test.js index 874129e6c6..b37d8f4d55 100644 --- a/tests/ve2/dm/ve.dm.DocumentFragment.test.js +++ b/tests/ve2/dm/ve.dm.DocumentFragment.test.js @@ -2,10 +2,21 @@ module( 've.dm.DocumentFragment' ); /* Tests */ -test( 'constructor', 114, function() { +test( 'constructor', 115, function() { var fragment = new ve.dm.DocumentFragment( ve.dm.example.data ); // Test count: ( ( 4 tests x 21 branch nodes ) + ( 3 tests x 10 leaf nodes ) ) = 114 ve.example.nodeTreeEqual( fragment.getDocumentNode(), ve.dm.example.tree ); + + raises( + function() { + fragment = new ve.dm.DocumentFragment( [ + { 'type': '/paragraph' }, + { 'type': 'paragraph' } + ] ); + }, + /^Unbalanced input passed to DocumentFragment$/, + 'unbalanced input causes exception' + ); } ); test( 'getData', 1, function() { From 379bc8da3c0eaf65be015b3d423af23814a62bff Mon Sep 17 00:00:00 2001 From: Catrope Date: Thu, 10 May 2012 20:04:13 -0700 Subject: [PATCH 2/5] selectNodes() fixes: * Initialize startOffset to 0 not 1, don't know what I was thinking * Use currentFrame for nodeRange instead of parentFrame, don't know what I was thinking there either * If the returned node has no parent (is the document node), don't attempt to access parentFrame and don't set index Change-Id: Iad969a7c29436cdf4151ead7e9d3d8e2a30befb3 --- modules/ve2/ve.Document.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/modules/ve2/ve.Document.js b/modules/ve2/ve.Document.js index f9664adb0a..6b62d16ef4 100644 --- a/modules/ve2/ve.Document.js +++ b/modules/ve2/ve.Document.js @@ -34,7 +34,7 @@ ve.Document.prototype.getDocumentNode = function() { * @returns {Array} List of objects describing nodes in the selection and the ranges therein * 'node': Reference to a ve.dm.Node * 'range': ve.Range, missing if the entire node is covered - * 'index': Index of the node in its parent + * 'index': Index of the node in its parent, missing if node has no parent * 'nodeRange': Range covering the inside of the entire node * @throws 'Invalid start offset' if range.start is out of range * @throws 'Invalid end offset' if range.end is out of range @@ -51,7 +51,7 @@ ve.Document.prototype.selectNodes = function( range, mode ) { // Index of the child in node we're visiting 'index': 0, // First offset inside node - 'startOffset': 1 + 'startOffset': 0 } ], node, prevNode, @@ -106,15 +106,17 @@ ve.Document.prototype.selectNodes = function( range, mode ) { if ( start == end && ( startBetween || endBetween ) && node.isWrapped() ) { // Empty range in the parent, outside of any child - parentFrame = stack[stack.length - 2]; - return [ { + retval = [ { 'node': currentFrame.node, 'range': new ve.Range( start, end ), - 'index': parentFrame.index, - 'nodeRange': new ve.Range( parentFrame.startOffset, - parentFrame.startOffset + currentFrame.node.getLength() + 'nodeRange': new ve.Range( currentFrame.startOffset, + currentFrame.startOffset + currentFrame.node.getLength() ) } ]; + parentFrame = stack[stack.length - 2]; + if ( parentFrame ) { + retval[0].index = parentFrame.index; + } } else if ( startBetween ) { // start is between the previous sibling and node // so the selection covers all of node and possibly more From 6652f7573b401fd3fe14aa4f02b18db073b61825 Mon Sep 17 00:00:00 2001 From: Catrope Date: Thu, 10 May 2012 20:08:12 -0700 Subject: [PATCH 3/5] Add indexInNode to selectNodes() output This is for the case where we have a zero-length range in between two siblings, and we need to know what index that corresponds to in order to be able to insert nodes there (rebuildNodes() will use it for this purpose) Change-Id: I357d1cd665667a76f955a10b8d9d2810976cdbd7 --- modules/ve2/ve.Document.js | 5 +++++ tests/ve2/ve.example.js | 15 +++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/modules/ve2/ve.Document.js b/modules/ve2/ve.Document.js index 6b62d16ef4..f90fc4c7f8 100644 --- a/modules/ve2/ve.Document.js +++ b/modules/ve2/ve.Document.js @@ -35,6 +35,10 @@ ve.Document.prototype.getDocumentNode = function() { * 'node': Reference to a ve.dm.Node * 'range': ve.Range, missing if the entire node is covered * 'index': Index of the node in its parent, missing if node has no parent + * 'indexInNode': If range is a zero-length range between two children of node, + * this is set to the index of the child following range (or to + * node.children.length+1 if range is between the last child and + * the end). Missing in all other cases * 'nodeRange': Range covering the inside of the entire node * @throws 'Invalid start offset' if range.start is out of range * @throws 'Invalid end offset' if range.end is out of range @@ -108,6 +112,7 @@ ve.Document.prototype.selectNodes = function( range, mode ) { // Empty range in the parent, outside of any child retval = [ { 'node': currentFrame.node, + 'indexInNode': currentFrame.index + ( endBetween ? 1 : 0 ), 'range': new ve.Range( start, end ), 'nodeRange': new ve.Range( currentFrame.startOffset, currentFrame.startOffset + currentFrame.node.getLength() diff --git a/tests/ve2/ve.example.js b/tests/ve2/ve.example.js index 5a6fa1cb76..76b83fb053 100644 --- a/tests/ve2/ve.example.js +++ b/tests/ve2/ve.example.js @@ -154,6 +154,20 @@ ve.example.getSelectNodesCases = function( doc ) { 'nodeRange': new ve.Range( 1, 4 ) } ] + }, + // Zero-length range between two children of the document + { + 'actual': doc.selectNodes( new ve.Range( 5, 5 ), 'leaves' ), + 'expected': [ + // document + { + 'node': documentNode, + 'range': new ve.Range( 5, 5 ), + // no 'index' because documentNode has no parent + 'indexInNode': 1, + 'nodeRange': new ve.Range( 0, 53 ) + } + ] } ]; }; @@ -196,6 +210,7 @@ ve.example.nodeSelectionEqual = function( a, b ) { strictEqual( 'range' in a[i], 'range' in b[i], 'range existence match' ); } deepEqual( a[i].index, b[i].index, 'index match' ); + deepEqual( a[i].indexInNode, b[i].indexInNode, 'indexInNode match' ); deepEqual( a[i].nodeRange, b[i].nodeRange, 'nodeRange match' ); } }; From 3cc22b00cb69e8e0a61298974a5e22c374c48ba6 Mon Sep 17 00:00:00 2001 From: Catrope Date: Thu, 10 May 2012 20:09:34 -0700 Subject: [PATCH 4/5] Use index and indexFromNode (from selectNodes()) in DocumentSynchronizer This makes insertions work (using indexFromNode), and eliminates the indexOf() call (using index) Change-Id: Ibfa77353af99534edc324c0e314c8b95c5136d8b --- modules/ve2/dm/ve.dm.DocumentSynchronizer.js | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/modules/ve2/dm/ve.dm.DocumentSynchronizer.js b/modules/ve2/dm/ve.dm.DocumentSynchronizer.js index 24d692f146..e8e4b5249f 100644 --- a/modules/ve2/dm/ve.dm.DocumentSynchronizer.js +++ b/modules/ve2/dm/ve.dm.DocumentSynchronizer.js @@ -89,12 +89,20 @@ ve.dm.DocumentSynchronizer.synchronizers = { return; } - // TODO index of firstNode in parent should be in the selectNodes result - var firstNode = selection[0].node, + var firstNode, parent, index, numNodes; + if ( 'indexInNode' in selection[0] ) { + // Insertion + parent = selection[0].node; + index = selection[0].indexInNode; + numNodes = 0; + } else { + // Rebuild + firstNode = selection[0].node, parent = firstNode.getParent(), - index = parent.indexOf( firstNode ); - - this.document.rebuildNodes( parent, index, selection.length, action.oldRange.from, + index = selection[0].index; + numNodes = selection.length; + } + this.document.rebuildNodes( parent, index, numNodes, action.oldRange.from, action.newRange.getLength() ); } From ca17ea09433823947add19c7ef773783cfb0a036 Mon Sep 17 00:00:00 2001 From: Catrope Date: Thu, 10 May 2012 20:12:07 -0700 Subject: [PATCH 5/5] Port structural replace code from the old VE code This makes TransactionProcessor work for regular replacements, as well as insertions and deletions of self-contained pieces of data. This does NOT yet work for inserting and deleting unbalanced data (splitting/merging nodes). I've tested this from the console for insertions and deletions and simple replacements, but I haven't tested wrappings. We should write a bunch of unit tests for this some time :) Change-Id: Ic2fd75d1cf2e127bc9ae58debce67576be2c912f --- modules/ve2/dm/ve.dm.TransactionProcessor.js | 73 +++++++++++++++++++- 1 file changed, 72 insertions(+), 1 deletion(-) diff --git a/modules/ve2/dm/ve.dm.TransactionProcessor.js b/modules/ve2/dm/ve.dm.TransactionProcessor.js index ebec6821e2..78d38ac794 100644 --- a/modules/ve2/dm/ve.dm.TransactionProcessor.js +++ b/modules/ve2/dm/ve.dm.TransactionProcessor.js @@ -232,6 +232,77 @@ ve.dm.TransactionProcessor.prototype.replace = function( op ) { this.cursor += replacement.length; } else { // Structural replacement - // TODO implement + // TODO generalize for insert/remove + + // It's possible that multiple replace operations are needed before the + // model is back in a consistent state. This loop applies the current + // replace operation to the linear model, then keeps applying subsequent + // operations until the model is consistent. We keep track of the changes + // and queue a single rebuild after the loop finishes. + var operation = op, + removeLevel = 0, + replaceLevel = 0, + startOffset = this.cursor, + adjustment = 0, + i, + type; + + while ( true ) { + if ( operation.type == 'replace' ) { + var opRemove = this.reversed ? operation.replacement : operation.remove, + opReplacement = this.reversed ? operation.remove : operation.replacement; + // Update the linear model for this replacement + ve.batchSplice( this.document.data, this.cursor, opRemove.length, opReplacement ); + this.cursor += opReplacement.length; + adjustment += opReplacement.length - opRemove.length; + + // Walk through the remove and replacement data + // and keep track of the element depth change (level) + // for each of these two separately. The model is + // only consistent if both levels are zero. + for ( i = 0; i < opRemove.length; i++ ) { + type = opRemove[i].type; + if ( type === undefined ) { + // This is content, ignore + } else if ( type.charAt( 0 ) === '/' ) { + // Closing element + removeLevel--; + } else { + // Opening element + removeLevel++; + } + } + for ( i = 0; i < opReplacement.length; i++ ) { + type = opReplacement[i].type; + if ( type === undefined ) { + // This is content, ignore + } else if ( type.charAt( 0 ) === '/' ) { + // Closing element + replaceLevel--; + } else { + // Opening element + replaceLevel++; + } + } + } else { + // We know that other operations won't cause adjustments, so we + // don't have to update adjustment + this.executeOperation( operation ); + } + + if ( removeLevel === 0 && replaceLevel === 0 ) { + // The model is back in a consistent state, so we're done + break; + } + + // Get the next operation + operation = this.nextOperation(); + if ( !operation ) { + throw 'Unbalanced set of replace operations found'; + } + } + // Queue a rebuild for the replaced node + this.synchronizer.pushRebuild( new ve.Range( startOffset, this.cursor - adjustment ), + new ve.Range( startOffset, this.cursor ) ); } };