r103377 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r103376‎ | r103377 | r103378 >
Date:20:39, 16 November 2011
Author:tparscal
Status:deferred (Comments)
Tags:
Comment:
* Added support for inserting content at a structural offset
* Broke rebuildNodes into 2 parts so insert can use just buildNodes when inserting whole nodes
* Added getIndexFromOffset to es.DocumentModelBranchNode objects, which returns an index of a child node from an offset
Modified paths:
  • /trunk/extensions/VisualEditor/modules/es/bases/es.DocumentBranchNode.js (modified) (history)
  • /trunk/extensions/VisualEditor/modules/es/es.TransactionProcessor.js (modified) (history)

Diff [purge]

Index: trunk/extensions/VisualEditor/modules/es/es.TransactionProcessor.js
@@ -101,6 +101,10 @@
102102 // Remove the node we are about to insert into from the model tree
103103 parent.splice( index, oldNodes.length );
104104 }
 105+ this.buildNodes( newData, parent, index );
 106+};
 107+
 108+es.TransactionProcessor.prototype.buildNodes = function( newData, parent, index ) {
105109 // Regenerate nodes for the data we've affected
106110 var newNodes = es.DocumentModel.createNodesFromData( newData );
107111 // Insert new elements into the tree where the old ones used to be
@@ -177,7 +181,12 @@
178182
179183 es.TransactionProcessor.prototype.insert = function( op ) {
180184 if ( es.DocumentModel.isStructuralOffset( this.model.data, this.cursor ) ) {
181 - // TODO: Support tree updates when inserting between elements
 185+ es.insertIntoArray( this.model.data, this.cursor, op.data );
 186+ this.applyAnnotations( this.cursor + op.data.length );
 187+ var parent = this.model.getNodeFromOffset( this.cursor ),
 188+ index = parent.getIndexFromOffset( this.cursor );
 189+ console.log( parent, index );
 190+ this.buildNodes( op.data, parent, index );
182191 } else {
183192 // Get the node we are about to insert into at the lowest depth possible
184193 var node = this.getScope( this.model.getNodeFromOffset( this.cursor ), op.data );
Index: trunk/extensions/VisualEditor/modules/es/bases/es.DocumentBranchNode.js
@@ -257,6 +257,26 @@
258258 };
259259
260260 /**
 261+ * Gets the index of a child node from a given offset.
 262+ *
 263+ * @method
 264+ * @param {Integer} offset Offset to find index of
 265+ * @returns {Integer} Index of child node at offset or -1 if offset was out of range
 266+ */
 267+es.DocumentBranchNode.prototype.getIndexFromOffset = function( offset ) {
 268+ var left = 0,
 269+ elementLength;
 270+ for ( var i = 0; i < this.children.length; i++ ) {
 271+ elementLength = this.children[i].getElementLength();
 272+ if ( offset >= left && offset < left + elementLength ) {
 273+ return i;
 274+ }
 275+ left += elementLength;
 276+ }
 277+ return -1;
 278+};
 279+
 280+/**
261281 * Gets a list of nodes and their sub-ranges which are covered by a given range.
262282 *
263283 * @method

Follow-up revisions

RevisionCommit summaryAuthorDate
r103479Fix for issue identified in review of r103377 - this will make it so insertio...tparscal18:05, 17 November 2011

Comments

#Comment by Catrope (talk | contribs)   12:50, 17 November 2011
+			index = parent.getIndexFromOffset( this.cursor );

This looks wrong. this.cursor is a global offset (relative to the document), but getIndexFromOffset() expects a local offset (relative to parent). I suspect your testing didn't show this because it so happened that parent == document. r103447 touches a lot of this code, but it looks like this issue still exists.

Speaking of testing, you should add test cases for the code you introduced.

+		console.log( parent, index );

was removed in r103447.

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:05, 17 November 2011

See r103479 for fix

Status & tagging log