jenkins-bot has submitted this change and it was merged.
Change subject: Correctly preserve metadata in `Transaction.newFromUnwrap`.
......................................................................
Correctly preserve metadata in `Transaction.newFromUnwrap`.
The `Transaction.pushReplace` method has a corner case if the removed
region has metadata and the inserted region is empty. This works fine
unless there are two adjacent `pushReplace` operations, which can occur
in `Transaction.newFromUnwrap`. Fix this by having `pushReplace` look
at a preceding replace and correctly merge the two operations if
possible (in particular in the tricky case where the previous case has
a zero-length insertion). Pleasantly, this can be done without a lot of
special-casing code in `pushReplace` or `newFromUnwrap`.
Add test cases verifying the `newFromUnwrap` works correctly (both
in commit and in rollback) when there is metadata present.
Change-Id: I6cfec0d2b1823dad724422f018a3c73dc0c7f186
---
M modules/ve/dm/ve.dm.Transaction.js
M modules/ve/test/dm/ve.dm.Transaction.test.js
M modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
M modules/ve/test/dm/ve.dm.example.js
4 files changed, 216 insertions(+), 8 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 a75e520..761c036 100644
--- a/modules/ve/dm/ve.dm.Transaction.js
+++ b/modules/ve/dm/ve.dm.Transaction.js
@@ -890,11 +890,22 @@
removeMetadata = metadataReplace.remove,
insertMetadata = metadataReplace.insert;
- if ( lastOp && lastOp.type === 'replace' && !lastOp.removeMetadata &&
!removeMetadata ) {
- // simple replaces can just be concatenated
- // TODO: allow replaces with meta to be merged?
- lastOp.insert = lastOp.insert.concat( insert );
- lastOp.remove = lastOp.remove.concat( remove );
+ // 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 === 'replace' &&
+ !( lastOp.insert.length > 0 && removeMetadata !== undefined )
+ ) {
+ lastOp = this.operations.pop();
+ this.lengthDifference -= lastOp.insert.length -
lastOp.remove.length;
+ this.pushReplace(
+ doc,
+ offset - lastOp.remove.length,
+ lastOp.remove.length + removeLength,
+ lastOp.insert.concat( insert )
+ );
} else {
op = {
'type': 'replace',
@@ -906,8 +917,8 @@
op.insertMetadata = insertMetadata;
}
this.operations.push( op );
+ this.lengthDifference += insert.length - remove.length;
}
- this.lengthDifference += insert.length - remove.length;
};
/**
diff --git a/modules/ve/test/dm/ve.dm.Transaction.test.js
b/modules/ve/test/dm/ve.dm.Transaction.test.js
index 776b521..ac04de0 100644
--- a/modules/ve/test/dm/ve.dm.Transaction.test.js
+++ b/modules/ve/test/dm/ve.dm.Transaction.test.js
@@ -1024,6 +1024,9 @@
QUnit.test( 'newFromWrap', function ( assert ) {
var i, key,
doc = ve.dm.example.createExampleDocument(),
+ metaDoc = ve.dm.example.createExampleDocument( 'withMeta' ),
+ listMetaDoc = ve.dm.example.createExampleDocument(
'listWithMeta' ),
+ listDoc = ve.dm.example.createExampleDocumentFromObject(
'listDoc', null, { 'listDoc': listMetaDoc.getData() } ),
cases = {
'changes a heading to a paragraph': {
'args': [doc, new ve.Range( 1, 4 ), [ { 'type':
'heading', 'attributes': { 'level': 1 } } ], [ { 'type': 'paragraph' } ], [],
[]],
@@ -1046,6 +1049,25 @@
{ 'type': 'retain', 'length': 10 },
{ 'type': 'replace', 'remove': [ {
'type': '/listItem' }, { 'type': '/list' } ], 'insert': [] },
{ 'type': 'retain', 'length': 37 }
+ ]
+ },
+ 'unwraps a multiple-item list': {
+ 'args': [listDoc, new ve.Range( 1, 11 ), [ {
'type': 'list' } ], [], [ { 'type': 'listItem', 'attributes': {'styles':
['bullet']} } ], [] ],
+ 'ops': [
+ { 'type': 'replace',
+ 'remove': [ { 'type': 'list' }, {
'type': 'listItem', 'attributes': { 'styles': ['bullet'] } } ],
+ 'insert': []
+ },
+ { 'type': 'retain', 'length': 3 },
+ { 'type': 'replace',
+ 'remove': [ { 'type': '/listItem' },
{ 'type': 'listItem', 'attributes': { 'styles': ['bullet'] } } ],
+ 'insert': []
+ },
+ { 'type': 'retain', 'length': 3 },
+ { 'type': 'replace',
+ 'remove': [ { 'type': '/listItem' },
{ 'type': '/list' } ],
+ 'insert': []
+ }
]
},
'replaces a table with a list': {
@@ -1094,6 +1116,48 @@
{ 'type': 'retain', 'length': 2 }
]
},
+ 'metadata is preserved on wrap': {
+ 'args': [metaDoc, new ve.Range( 1, 10 ), [ {
'type': 'paragraph' } ], [ { 'type': 'heading', 'level': 1 } ], [], [] ],
+ 'ops': [
+ { 'type': 'replace',
+ 'remove': [ { 'type': 'paragraph' } ],
+ 'insert': [ { 'type': 'heading',
'level': 1 } ],
+ 'insertMetadata':
metaDoc.getMetadata().slice(0, 1),
+ 'removeMetadata':
metaDoc.getMetadata().slice(0, 1)
+ },
+ { 'type': 'retain', 'length': 9 },
+ { 'type': 'replace',
+ 'remove': [ { 'type': '/paragraph' }
],
+ 'insert': [ { 'type': '/heading' } ]
+ },
+ { 'type': 'retain', 'length': 2 }
+ ]
+ },
+ 'metadata is preserved on unwrap': {
+ 'args': [listMetaDoc, new ve.Range( 1, 11 ), [
{ 'type': 'list' } ], [], [ { 'type': 'listItem', 'attributes': {'styles':
['bullet']} } ], [] ],
+ 'ops': [
+ { '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)
+ },
+ { '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)
+ },
+ { '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)
+ }
+ ]
+ },
'checks integrity of unwrapOuter parameter': {
'args': [doc, new ve.Range( 13, 32 ), [ {
'type': 'table' } ], [], [], []],
'exception': Error
diff --git a/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
b/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
index 382fd25..c420b05 100644
--- a/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
+++ b/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
@@ -452,6 +452,22 @@
data.splice( 2, 2 );
data.splice( 4, 3 );
}
+ },
+ 'preserves metadata on unwrap': {
+ 'data': ve.dm.example.listWithMeta,
+ 'calls': [
+ [ 'newFromWrap', new ve.Range( 1, 11 ),
+ [ { 'type': 'list' } ], [],
+ [ { 'type': 'listItem', 'attributes':
{'styles': ['bullet']} } ], [] ]
+ ],
+ 'expected': function ( data ) {
+ data.splice( 35, 1 ); // remove '/list'
+ data.splice( 32, 1 ); // remove
'/listItem'
+ data.splice( 20, 1 ); // remove
'listItem'
+ data.splice( 17, 1 ); // remove
'/listItem'
+ data.splice( 5, 1 ); // remove
'listItem'
+ data.splice( 2, 1 ); // remove 'list'
+ }
}
};
@@ -473,10 +489,15 @@
tx = new ve.dm.Transaction();
for ( i = 0; i < cases[msg].calls.length; i++ ) {
- // pushReplace needs the document as its first argument
- if ( cases[msg].calls[i][0] === 'pushReplace' ) {
+ // some calls need the document as its first argument
+ if ( /^(pushReplace$|new)/.test( cases[msg].calls[i][0]
) ) {
cases[msg].calls[i].splice( 1, 0, testDoc );
}
+ // special case static methods of Transaction
+ if ( /^new/.test( cases[msg].calls[i][0] ) ) {
+ tx =
ve.dm.Transaction[cases[msg].calls[i][0]].apply( null,
cases[msg].calls[i].slice( 1 ) );
+ break;
+ }
tx[cases[msg].calls[i][0]].apply( tx,
cases[msg].calls[i].slice( 1 ) );
}
diff --git a/modules/ve/test/dm/ve.dm.example.js
b/modules/ve/test/dm/ve.dm.example.js
index b21fe09..0f0e676 100644
--- a/modules/ve/test/dm/ve.dm.example.js
+++ b/modules/ve/test/dm/ve.dm.example.js
@@ -589,6 +589,118 @@
];
+ve.dm.example.listWithMeta = [
+ // 0 - Beginning of list
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<meta property="one" />' ).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': 'list' },
+ // 1 - Beginning of first list item
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<meta property="two" />' ).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': 'listItem', 'attributes': { 'styles': ['bullet'] } },
+ // 2 - Beginning of paragraph
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<meta property="three" />'
).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': 'paragraph' },
+ // 3 - Plain "a"
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<meta property="four" />' ).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ 'a',
+ // 4 - End of paragraph
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<meta property="five" />' ).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': '/paragraph' },
+ // 5 - End of first list item
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<meta property="six" />' ).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': '/listItem' },
+ // 6 - Beginning of second list item
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<meta property="seven" />'
).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': 'listItem', 'attributes': { 'styles': ['bullet'] } },
+ // 7 - Beginning of paragraph
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<meta property="eight" />'
).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': 'paragraph' },
+ // 8 - Plain "b"
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<meta property="nine" />' ).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ 'b',
+ // 9 - End of paragraph
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<meta property="ten" />' ).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': '/paragraph' },
+ // 10 - End of second list item
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<meta property="eleven" />'
).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': '/listItem' },
+ // 11 - End of list
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<meta property="twelve" />'
).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': '/list' }
+];
+
+
ve.dm.example.complexTableHtml =
'<table><caption>Foo</caption><thead><tr><th>Bar</th></tr></thead>' +
'<tfoot><tr><td>Baz</td></tr></tfoot><tbody><tr><td>Quux</td><td>Whee</td></tr></tbody></table>';
--
To view, visit https://gerrit.wikimedia.org/r/82363
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6cfec0d2b1823dad724422f018a3c73dc0c7f186
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Cscott <canan...@wikimedia.org>
Gerrit-Reviewer: Catrope <roan.katt...@gmail.com>
Gerrit-Reviewer: Cscott <canan...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
