jenkins-bot has submitted this change and it was merged.
Change subject: Cleanups for ve.dm.Transaction
......................................................................
Cleanups for ve.dm.Transaction
Avoid making a copy of the entire data array by using `doc.data` and
`doc.data.getLength()` instead of `doc.getData()` and
`doc.getData().length`.
Get rid of some unnecessary conditionals in `Transaction.newFromWrap`: the
`tx.pushReplace()` and `tx.pushRetain()` methods will already gracefully
no-op if the replaced/retained region is empty; we don't need to add
extra control flow to handle that case.
Clarify offset math in `Transaction.newFromWrap`.
Fix a minor bug in `Transaction.newFromInsertion`: the final retain didn't
account for the length of the removed region, if `doc.fixupInsertion`
creates one. (This usually doesn't happen.)
Remove an inaccurate TODO in `Transaction.pushReplaceMetadata()` -- it is
no longer a straight copy/paste of `Transaction.pushReplace`, so a refactor
isn't really called for.
Change-Id: I7d86a2449978365d69d4a5ed43116c2e9945475d
---
M modules/ve/dm/ve.dm.Transaction.js
1 file changed, 14 insertions(+), 16 deletions(-)
Approvals:
Catrope: Looks good to me, approved
jenkins-bot: Verified
diff --git a/modules/ve/dm/ve.dm.Transaction.js
b/modules/ve/dm/ve.dm.Transaction.js
index f5b7e3a..661236e 100644
--- a/modules/ve/dm/ve.dm.Transaction.js
+++ b/modules/ve/dm/ve.dm.Transaction.js
@@ -31,7 +31,7 @@
*/
ve.dm.Transaction.newFromInsertion = function ( doc, offset, insertion ) {
var tx = new ve.dm.Transaction(),
- data = doc.getData();
+ data = doc.data;
// Fix up the insertion
insertion = doc.fixupInsertion( insertion, offset );
// Retain up to insertion point, if needed
@@ -39,7 +39,7 @@
// Insert data
tx.pushReplace( doc, insertion.offset, insertion.remove, insertion.data
);
// Retain to end of document, if needed (for completeness)
- tx.pushRetain( data.length - insertion.offset );
+ tx.pushRetain( data.getLength() - ( insertion.offset + insertion.remove
) );
return tx;
};
@@ -73,11 +73,11 @@
removeStart = null,
removeEnd = null,
tx = new ve.dm.Transaction(),
- data = doc.getData();
+ data = doc.data;
// Validate range
if ( range.isCollapsed() ) {
// Empty range, nothing to remove, retain up to the end of the
document (for completeness)
- tx.pushRetain( data.length );
+ tx.pushRetain( data.getLength() );
return tx;
}
// Select nodes and validate selection
@@ -102,7 +102,7 @@
}
tx.pushRetain( removeStart );
tx.addSafeRemoveOps( doc, removeStart, removeEnd );
- tx.pushRetain( data.length - removeEnd );
+ tx.pushRetain( data.getLength() - removeEnd );
// All done
return tx;
}
@@ -149,7 +149,7 @@
offset = removeEnd;
}
// Retain up to the end of the document
- tx.pushRetain( data.length - offset );
+ tx.pushRetain( data.getLength() - offset );
return tx;
};
@@ -570,7 +570,7 @@
tx.pushReplace( doc, i,
unwrapEach.length, ve.copy( wrapEach ) );
// Store this offset for later
- startOffset = i;
+ startOffset = i +
unwrapEach.length;
}
depth++;
} else {
@@ -578,10 +578,12 @@
depth--;
if ( depth === 0 ) {
// We are at the end of a
top-level element
+ // Advance past the element,
then back up past the unwrapEach
+ j = ( i + 1 ) -
unwrapEach.length;
// Retain the contents of what
we're wrapping
- tx.pushRetain( i - startOffset
+ 1 - unwrapEach.length*2 );
+ tx.pushRetain( j - startOffset
);
// Replace the closing elements
- tx.pushReplace( doc, i + 1 -
unwrapEach.length, unwrapEach.length, ve.copy( closingWrapEach ) );
+ tx.pushReplace( doc, j,
unwrapEach.length, ve.copy( closingWrapEach ) );
}
}
}
@@ -592,14 +594,11 @@
tx.pushRetain( range.end - range.start );
}
- if ( wrapOuter.length > 0 || unwrapOuter.length > 0 ) {
- tx.pushReplace( doc, range.end, unwrapOuter.length,
closingArray( wrapOuter ) );
- }
+ // this is a no-op if unwrapOuter.length===0 and wrapOuter.length===0
+ tx.pushReplace( doc, range.end, unwrapOuter.length, closingArray(
wrapOuter ) );
// Retain up to the end of the document
- if ( range.end < doc.data.getLength() ) {
- tx.pushRetain( doc.data.getLength() - range.end -
unwrapOuter.length );
- }
+ tx.pushRetain( doc.data.getLength() - range.end - unwrapOuter.length );
return tx;
};
@@ -915,7 +914,6 @@
/**
* Add a replace metadata operation
- * // TODO: this is a copy/paste of pushReplace (at the moment). Consider a
refactor.
*
* @method
* @param {Array} remove Metadata to remove
--
To view, visit https://gerrit.wikimedia.org/r/82120
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I7d86a2449978365d69d4a5ed43116c2e9945475d
Gerrit-PatchSet: 5
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Cscott <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
