jenkins-bot has submitted this change and it was merged.
Change subject: Remove no-insertion metadata corner case from
`ve.dm.Transaction.pushReplace()`.
......................................................................
Remove no-insertion metadata corner case from `ve.dm.Transaction.pushReplace()`.
This version pushes a `replaceMetadata` operation after a `replace` to
fixup trailing metadata if there is no inserted region and the removed
region includes metadata. This avoids a corner case where the
size of the metadata arrays inserted/removed in `Transaction.pushReplace()`
do not match the size of the data arrays inserted/removed.
We remove the now-unused `Document.getMetadataReplace()` method.
We also adjust `MetaList.onTransact()` so that it continues to work
properly when the number of metadata entries in `replace.insert` is
not the same as the number of metadata entries in `replace.remove`.
Change-Id: I1d600405b855ca1cb569853bb885b0752df47173
---
M modules/ve/dm/ve.dm.Document.js
M modules/ve/dm/ve.dm.MetaList.js
M modules/ve/dm/ve.dm.Transaction.js
M modules/ve/test/dm/ve.dm.Document.test.js
M modules/ve/test/dm/ve.dm.Transaction.test.js
5 files changed, 120 insertions(+), 93 deletions(-)
Approvals:
Catrope: Looks good to me, approved
jenkins-bot: Verified
diff --git a/modules/ve/dm/ve.dm.Document.js b/modules/ve/dm/ve.dm.Document.js
index 739f850..daa30ac 100644
--- a/modules/ve/dm/ve.dm.Document.js
+++ b/modules/ve/dm/ve.dm.Document.js
@@ -334,46 +334,6 @@
};
/**
- * Get the metadata replace operation required to keep data & metadata in sync
after a splice
- *
- * @method
- * @param {number} offset Data offset to start at
- * @param {number} remove Number of elements being removed
- * @param {Array} insert Element data being inserted
- * @returns {Object} Metadata replace operation to keep data & metadata in sync
- */
-ve.dm.Document.prototype.getMetadataReplace = function ( offset, remove,
insert ) {
- var removeMetadata, insertMetadata, replace = {}, extend;
- if ( remove > 0 ) {
- // if we are removing anything we need to collapse the metadata
- if ( insert.length > 0 ) {
- // if there's at least one element inserted, collapse
the metadata
- // onto the first cursor position.
- extend = 0;
- } else {
- // Make the collapsed region is 1 larger than the
deleted region, so we
- // can put the collapsed metadata on the element
immediately
- // following the deleted region.
- extend = 1;
- }
- removeMetadata = this.getMetadata( new ve.Range( offset, offset
+ remove + extend ) );
- // check removeMetadata is non-empty
- if ( !ve.compare( removeMetadata, new Array(
removeMetadata.length ) ) ) {
- insertMetadata = ve.dm.MetaLinearData.static.merge(
removeMetadata );
- // pad out the end of `insertMetadata` so it is
- // `insert.length + extend` elements long. (if
`extend` is 1, then
- // `insert.length` is 0 and thus we don't need to pad
further.)
- if ( extend === 0 ) {
- ve.batchSplice( insertMetadata, 1, 0, new
Array( insert.length - 1 ) );
- }
- replace.remove = removeMetadata;
- replace.insert = insertMetadata;
- }
- }
- return replace;
-};
-
-/**
* Splice metadata into and/or out of the linear model.
*
* `this.metadata` will be updated accordingly.
diff --git a/modules/ve/dm/ve.dm.MetaList.js b/modules/ve/dm/ve.dm.MetaList.js
index 934b2db..4096de4 100644
--- a/modules/ve/dm/ve.dm.MetaList.js
+++ b/modules/ve/dm/ve.dm.MetaList.js
@@ -100,7 +100,32 @@
// offset and index directly
ins = reversed ? ops[i].removeMetadata :
ops[i].insertMetadata;
rm = reversed ? ops[i].insertMetadata :
ops[i].removeMetadata;
- if ( rm !== undefined ) {
+ if ( rm === undefined ) {
+ // no impact on metadata.
+ /* jshint noempty: false */
+ } else if ( ins.length === 0 ) {
+ for ( j = 0, jlen = rm.length; j <
jlen; j++ ) {
+ if ( rm[j] !== undefined ) {
+ for ( k = 0, klen =
rm[j].length; k < klen; k++ ) {
+ item =
this.deleteRemovedItem( offset + j, k );
+
removedItems.push( { 'item': item, 'offset': offset + j, 'index': k } );
+ }
+ }
+ }
+ } else if ( rm.length === 0 ) {
+ itemIndex = -Math.pow(2, 53); // INT_MIN
+ for ( j = 0, jlen = ins.length; j <
jlen; j++ ) {
+ if ( ins[j] !== undefined ) {
+ for ( k = 0, klen =
ins[j].length; k < klen; k++ ) {
+ item =
ve.dm.metaItemFactory.createFromElement( ins[j][k] );
+ // offset is
pre-transaction, but we'll fix it up w/ setMove
+
this.addInsertedItem( offset, itemIndex++, item );
+ item.setMove(
newOffset + j, k );
+
insertedItems.push( { 'item': item } );
+ }
+ }
+ }
+ } else {
// find the first itemIndex - the rest
should be in order after it
for ( j = 0, jlen = rm.length; j <
jlen; j++ ) {
if ( rm[j] !== undefined ) {
diff --git a/modules/ve/dm/ve.dm.Transaction.js
b/modules/ve/dm/ve.dm.Transaction.js
index 761c036..6719110 100644
--- a/modules/ve/dm/ve.dm.Transaction.js
+++ b/modules/ve/dm/ve.dm.Transaction.js
@@ -840,6 +840,10 @@
* Adds a replace op to remove the desired range and, where required, splices
in retain ops
* to prevent the deletion of internal data.
*
+ * An extra `replaceMetadata` operation might be pushed at the end if the
+ * affected region contains metadata; see
+ * {@link ve.dm.Transaction#pushReplace} for details.
+ *
* @param {ve.dm.Document} doc Document
* @param {number} removeStart Offset to start removing from
* @param {number} removeEnd Offset to remove to
@@ -869,7 +873,17 @@
};
/**
- * Add a replace operation, keeping metadata in sync if required
+ * Add a replace operation, keeping metadata in sync if required.
+ *
+ * Note that metadata attached to removed content is moved so that it
+ * attaches just before the inserted content. If there is
+ * metadata attached to the removed content but there is no inserted
+ * content, then an extra `replaceMetadata` operation is pushed in order
+ * to properly insert the merged metadata before the character immediately
+ * after the removed content. (Note that there is an extra metadata element
+ * after the final data element; if the removed region is at the very end of
+ * the document, the inserted `replaceMetadata` operation targets this
+ * final metadata element.)
*
* @method
* @param {ve.dm.Document} doc Document model
@@ -885,18 +899,44 @@
var op, end = this.operations.length - 1,
lastOp = end >= 0 ? this.operations[end] : null,
- remove = doc.getData( new ve.Range( offset, offset +
removeLength ) ),
- metadataReplace = doc.getMetadataReplace( offset, removeLength,
insert ),
- removeMetadata = metadataReplace.remove,
- insertMetadata = metadataReplace.insert;
+ penultOp = end >= 1 ? this.operations[ end - 1 ] : null,
+ range = new ve.Range( offset, offset + removeLength ),
+ remove = doc.getData( range ),
+ removeMetadata = doc.getMetadata( range ),
+ insertMetadata, extraMetadata;
+
+ if ( !ve.compare( removeMetadata, new Array( removeMetadata.length ) )
) {
+ // if we are removing a range which includes metadata, we need
to
+ // collapse it. If there's nothing to insert, we also need to
add
+ // an extra `replaceMetadata` operation later in order to
insert the
+ // collapsed metadata.
+ insertMetadata = ve.dm.MetaLinearData.static.merge(
removeMetadata );
+ if ( insert.length === 0 ) {
+ extraMetadata = insertMetadata[0];
+ insertMetadata = [];
+ } else {
+ // pad out at end so insert metadata is the same length
as insert data
+ ve.batchSplice( insertMetadata, 1, 0, new Array(
insert.length - 1 ) );
+ }
+ }
// simple replaces can be combined
// (but don't do this if there is metadata to be removed and the
previous
// replace had a non-zero insertion, because that would shift the
metadata
// location.)
if (
+ lastOp && lastOp.type === 'replaceMetadata' &&
+ lastOp.insert.length > 0 && lastOp.remove.length === 0 &&
+ penultOp && penultOp.type === 'replace' &&
+ penultOp.insert.length === 0 /* this is always true */
+ ) {
+ this.operations.pop();
+ lastOp = penultOp;
+ /* fall through */
+ }
+ if (
lastOp && lastOp.type === 'replace' &&
- !( lastOp.insert.length > 0 && removeMetadata !== undefined )
+ !( lastOp.insert.length > 0 && insertMetadata !== undefined )
) {
lastOp = this.operations.pop();
this.lengthDifference -= lastOp.insert.length -
lastOp.remove.length;
@@ -906,18 +946,29 @@
lastOp.remove.length + removeLength,
lastOp.insert.concat( insert )
);
- } else {
- op = {
- 'type': 'replace',
- 'remove': remove,
- 'insert': insert
- };
- if ( removeMetadata !== undefined ) {
- op.removeMetadata = removeMetadata;
- op.insertMetadata = insertMetadata;
- }
- this.operations.push( op );
- this.lengthDifference += insert.length - remove.length;
+ return;
+ }
+
+ if ( lastOp && lastOp.type === 'replaceMetadata' ) {
+ // `replace` operates on the metadata at the given offset; the
transaction
+ // touches the same region twice if `replace` follows a
`replaceMetadata`
+ // without a `retain` in between.
+ throw new Error( 'replace after replaceMetadata not allowed' );
+ }
+
+ op = {
+ 'type': 'replace',
+ 'remove': remove,
+ 'insert': insert
+ };
+ if ( insertMetadata !== undefined ) {
+ op.removeMetadata = removeMetadata;
+ op.insertMetadata = insertMetadata;
+ }
+ this.operations.push( op );
+ this.lengthDifference += insert.length - remove.length;
+ if ( extraMetadata !== undefined ) {
+ this.pushReplaceMetadata( [], extraMetadata );
}
};
diff --git a/modules/ve/test/dm/ve.dm.Document.test.js
b/modules/ve/test/dm/ve.dm.Document.test.js
index 1a387f7..0efe710 100644
--- a/modules/ve/test/dm/ve.dm.Document.test.js
+++ b/modules/ve/test/dm/ve.dm.Document.test.js
@@ -112,32 +112,6 @@
}
} );
-QUnit.test( 'getMetadataReplace', 3, function ( assert ) {
- var replace, expectedReplace,
- doc = ve.dm.example.createExampleDocument( 'withMeta' );
-
- replace = doc.getMetadataReplace( 10, 1, [] );
- expectedReplace = {
- 'remove': doc.getMetadata().slice( 10, 12 ),
- 'insert': ve.dm.MetaLinearData.static.merge(
doc.getMetadata().slice( 10, 12 ) )
- };
- assert.deepEqual( replace, expectedReplace, 'removing one element at
offset 10' );
-
- replace = doc.getMetadataReplace( 5, 2, [] );
- expectedReplace = {
- 'remove': doc.getMetadata().slice( 5, 8 ),
- 'insert': ve.dm.MetaLinearData.static.merge(
doc.getMetadata().slice( 5, 8 ) )
- };
- assert.deepEqual( replace, expectedReplace, 'removing two elements at
offset 5' );
-
- replace = doc.getMetadataReplace( 1, 8, [] );
- expectedReplace = {
- 'remove': doc.getMetadata().slice( 1, 10 ),
- 'insert': ve.dm.MetaLinearData.static.merge(
doc.getMetadata().slice( 1, 10 ) )
- };
- assert.deepEqual( replace, expectedReplace, 'blanking paragraph,
removing 8 elements at offset 1' );
-} );
-
QUnit.test( 'getNodeFromOffset', function ( assert ) {
var i, j, node,
doc = ve.dm.example.createExampleDocument(),
diff --git a/modules/ve/test/dm/ve.dm.Transaction.test.js
b/modules/ve/test/dm/ve.dm.Transaction.test.js
index ac04de0..96223ed 100644
--- a/modules/ve/test/dm/ve.dm.Transaction.test.js
+++ b/modules/ve/test/dm/ve.dm.Transaction.test.js
@@ -577,8 +577,13 @@
'type': 'replace',
'remove': ['B', 'a'],
'insert': [],
- 'removeMetadata':
metaDoc.getMetadata().slice( 7, 10 ),
- 'insertMetadata':
ve.dm.MetaLinearData.static.merge( metaDoc.getMetadata().slice( 7, 10 ) )
+ 'removeMetadata':
metaDoc.getMetadata().slice( 7, 9 ),
+ 'insertMetadata': []
+ },
+ {
+ 'type': 'replaceMetadata',
+ 'remove': [],
+ 'insert':
ve.dm.MetaLinearData.static.merge( metaDoc.getMetadata().slice( 7, 9 ) )[0]
},
{ 'type': 'retain', 'length': 4 }
]
@@ -1139,22 +1144,34 @@
{ 'type': 'replace',
'remove': [ { 'type': 'list' }, {
'type': 'listItem', 'attributes': { 'styles': ['bullet'] } } ],
'insert': [],
- 'insertMetadata':
ve.dm.MetaLinearData.static.merge( listMetaDoc.getMetadata().slice(0, 3) ),
- 'removeMetadata':
listMetaDoc.getMetadata().slice(0, 3)
+ 'insertMetadata': [],
+ 'removeMetadata':
listMetaDoc.getMetadata().slice(0, 2)
+ },
+ { 'type': 'replaceMetadata',
+ 'insert':
ve.dm.MetaLinearData.static.merge( listMetaDoc.getMetadata().slice(0, 2) )[0],
+ 'remove': []
},
{ 'type': 'retain', 'length': 3 },
{ 'type': 'replace',
'remove': [ { 'type': '/listItem' },
{ 'type': 'listItem', 'attributes': { 'styles': ['bullet'] } } ],
'insert': [],
- 'insertMetadata':
ve.dm.MetaLinearData.static.merge( listMetaDoc.getMetadata().slice(5, 8) ),
- 'removeMetadata':
listMetaDoc.getMetadata().slice(5, 8)
+ 'insertMetadata': [],
+ 'removeMetadata':
listMetaDoc.getMetadata().slice(5, 7)
+ },
+ { 'type': 'replaceMetadata',
+ 'insert':
ve.dm.MetaLinearData.static.merge( listMetaDoc.getMetadata().slice(5, 7) )[0],
+ 'remove': []
},
{ 'type': 'retain', 'length': 3 },
{ 'type': 'replace',
'remove': [ { 'type': '/listItem' },
{ 'type': '/list' } ],
'insert': [],
- 'insertMetadata':
ve.dm.MetaLinearData.static.merge( listMetaDoc.getMetadata().slice(10, 13) ),
- 'removeMetadata':
listMetaDoc.getMetadata().slice(10, 13)
+ 'insertMetadata': [],
+ 'removeMetadata':
listMetaDoc.getMetadata().slice(10, 12)
+ },
+ { 'type': 'replaceMetadata',
+ 'insert':
ve.dm.MetaLinearData.static.merge( listMetaDoc.getMetadata().slice(10, 12) )[0],
+ 'remove': []
}
]
},
--
To view, visit https://gerrit.wikimedia.org/r/82059
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I1d600405b855ca1cb569853bb885b0752df47173
Gerrit-PatchSet: 7
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Cscott <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: Cscott <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
