Cscott has uploaded a new change for review.
https://gerrit.wikimedia.org/r/81928
Change subject: Collapse metadata on any removal.
......................................................................
Collapse metadata on any removal.
In ve.dm.Document.getMetadataReplace(), we used to only merge metadata
if the amount removed is larger than the amount inserted. But this
could end up putting metadata in odd positions, for example if you
have Foo[[Category:Bar]]Baz and you delete 'ooBa' and replace it with
{image}xxx{/image}, then the category ends up inside the image.
We should always merge metadata when a segment is deleted, so that it
appears outside any new structure added.
Bug: 53444
Bug: 53445
Change-Id: I51d55fb370b473273f9cf152fdd0f356377d4109
---
M modules/ve/dm/ve.dm.Document.js
M modules/ve/dm/ve.dm.Transaction.js
M modules/ve/dm/ve.dm.TransactionProcessor.js
M modules/ve/test/dm/ve.dm.Document.test.js
M modules/ve/test/dm/ve.dm.Transaction.test.js
M modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
6 files changed, 134 insertions(+), 23 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/VisualEditor
refs/changes/28/81928/1
diff --git a/modules/ve/dm/ve.dm.Document.js b/modules/ve/dm/ve.dm.Document.js
index fa282ac..fddaee2 100644
--- a/modules/ve/dm/ve.dm.Document.js
+++ b/modules/ve/dm/ve.dm.Document.js
@@ -343,23 +343,29 @@
* @returns {Object} Metadata replace operation to keep data & metadata in sync
*/
ve.dm.Document.prototype.getMetadataReplace = function ( offset, remove,
insert ) {
- var removeMetadata, insertMetadata, replace = {};
- if ( remove > insert.length ) {
- // if we are removing more than we are inserting we need to
collapse the excess metadata
- removeMetadata = this.getMetadata( new ve.Range( offset +
insert.length, offset + remove + 1 ) );
+ 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 );
- replace.retain = insert.length;
+ if ( extend === 0 ) {
+ ve.batchSplice( insertMetadata, 1, 0, new
Array( insert.length - 1 ) );
+ }
replace.remove = removeMetadata;
replace.insert = insertMetadata;
}
- }
- // if insert.length === remove metadata can just stay where it is
- if ( insert.length > remove ) {
- // if we are inserting more than we are removing then we need
to pad out with undefineds
- replace.retain = remove;
- replace.insert = new Array( insert.length - remove );
}
return replace;
};
diff --git a/modules/ve/dm/ve.dm.Transaction.js
b/modules/ve/dm/ve.dm.Transaction.js
index f5b7e3a..38674b4 100644
--- a/modules/ve/dm/ve.dm.Transaction.js
+++ b/modules/ve/dm/ve.dm.Transaction.js
@@ -888,7 +888,6 @@
lastOp = end >= 0 ? this.operations[end] : null,
remove = doc.getData( new ve.Range( offset, offset +
removeLength ) ),
metadataReplace = doc.getMetadataReplace( offset, removeLength,
insert ),
- retainMetadata = metadataReplace.retain,
removeMetadata = metadataReplace.remove,
insertMetadata = metadataReplace.insert;
@@ -904,7 +903,6 @@
'insert': insert
};
if ( removeMetadata !== undefined ) {
- op.retainMetadata = retainMetadata;
op.removeMetadata = removeMetadata;
op.insertMetadata = insertMetadata;
}
diff --git a/modules/ve/dm/ve.dm.TransactionProcessor.js
b/modules/ve/dm/ve.dm.TransactionProcessor.js
index e24807b..959c404 100644
--- a/modules/ve/dm/ve.dm.TransactionProcessor.js
+++ b/modules/ve/dm/ve.dm.TransactionProcessor.js
@@ -332,11 +332,9 @@
this.document.data.batchSplice( this.cursor, remove.length,
insert );
// Keep the meta linear model in sync
if ( removeMetadata !== undefined ) {
- this.document.metadata.batchSplice( this.cursor +
retainMetadata, removeMetadata.length, insertMetadata );
- } else if ( insert.length > remove.length ) {
- this.document.metadata.batchSplice( this.cursor +
remove.length, 0, new Array( insert.length - remove.length ) );
- } else if ( insert.length < remove.length ) {
- this.document.metadata.batchSplice( this.cursor +
insert.length, remove.length - insert.length, [] );
+ this.document.metadata.batchSplice( this.cursor,
removeMetadata.length, insertMetadata );
+ } else {
+ this.document.metadata.batchSplice( this.cursor,
remove.length, new Array( insert.length ) );
}
this.applyAnnotations( this.cursor + insert.length );
// Get the node containing the replaced content
diff --git a/modules/ve/test/dm/ve.dm.Document.test.js
b/modules/ve/test/dm/ve.dm.Document.test.js
index bcacb2d..1a387f7 100644
--- a/modules/ve/test/dm/ve.dm.Document.test.js
+++ b/modules/ve/test/dm/ve.dm.Document.test.js
@@ -118,7 +118,6 @@
replace = doc.getMetadataReplace( 10, 1, [] );
expectedReplace = {
- 'retain': 0,
'remove': doc.getMetadata().slice( 10, 12 ),
'insert': ve.dm.MetaLinearData.static.merge(
doc.getMetadata().slice( 10, 12 ) )
};
@@ -126,7 +125,6 @@
replace = doc.getMetadataReplace( 5, 2, [] );
expectedReplace = {
- 'retain': 0,
'remove': doc.getMetadata().slice( 5, 8 ),
'insert': ve.dm.MetaLinearData.static.merge(
doc.getMetadata().slice( 5, 8 ) )
};
@@ -134,7 +132,6 @@
replace = doc.getMetadataReplace( 1, 8, [] );
expectedReplace = {
- 'retain': 0,
'remove': doc.getMetadata().slice( 1, 10 ),
'insert': ve.dm.MetaLinearData.static.merge(
doc.getMetadata().slice( 1, 10 ) )
};
diff --git a/modules/ve/test/dm/ve.dm.Transaction.test.js
b/modules/ve/test/dm/ve.dm.Transaction.test.js
index 4a5c31a..776b521 100644
--- a/modules/ve/test/dm/ve.dm.Transaction.test.js
+++ b/modules/ve/test/dm/ve.dm.Transaction.test.js
@@ -577,7 +577,6 @@
'type': 'replace',
'remove': ['B', 'a'],
'insert': [],
- 'retainMetadata': 0,
'removeMetadata':
metaDoc.getMetadata().slice( 7, 10 ),
'insertMetadata':
ve.dm.MetaLinearData.static.merge( metaDoc.getMetadata().slice( 7, 10 ) )
},
diff --git a/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
b/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
index 886f1b4..726e1d4 100644
--- a/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
+++ b/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
@@ -334,7 +334,120 @@
['pushReplace', 0, 5, [ { 'type':
'table' }, { 'type': '/table' } ]]
],
'expected': function ( data ) {
- data.splice( 0, 2, { 'type': 'table' },
{ 'type': '/table' } );
+ data.splice( 0, 2 );
+ data.splice( 2, 3, { 'type': 'table' },
{ 'type': '/table' } );
+ }
+ },
+ 'structural replacement starting at an offset with
metadata': {
+ 'data': [
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<!--
foo -->' ).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': 'paragraph' },
+ 'F',
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'style': 'comment',
+ 'text': ' inline '
+ }
+ },
+ { 'type': '/alienMeta' },
+ 'o', 'o',
+ { 'type': '/paragraph' }
+ ],
+ 'calls': [
+ ['pushReplace', 0, 5, [ { 'type':
'table' }, { 'type': '/table' } ]]
+ ],
+ 'expected': function ( data ) {
+ // metadata is merged.
+ data.splice( 2, 2 );
+ data.splice( 4, 3, { 'type': 'table' },
{ 'type': '/table' } );
+ }
+ },
+ 'structural replacement ending at an offset with
metadata': {
+ 'data': [
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<!--
foo -->' ).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': 'paragraph' },
+ 'F',
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'style': 'comment',
+ 'text': ' inline '
+ }
+ },
+ { 'type': '/alienMeta' },
+ 'o', 'o',
+ { 'type': '/paragraph' },
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<!--
bar -->' ).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': 'paragraph' },
+ 'B', 'a', 'r',
+ { 'type': '/paragraph' }
+ ],
+ 'calls': [
+ ['pushReplace', 0, 5, [ { 'type':
'table' }, { 'type': '/table' } ]]
+ ],
+ 'expected': function ( data ) {
+ // metadata is merged.
+ data.splice( 2, 2 );
+ data.splice( 4, 3, { 'type': 'table' },
{ 'type': '/table' } );
+ }
+ },
+ 'structural deletion ending at an offset with
metadata': {
+ 'data': [
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<!--
foo -->' ).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': 'paragraph' },
+ 'F',
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'style': 'comment',
+ 'text': ' inline '
+ }
+ },
+ { 'type': '/alienMeta' },
+ 'o', 'o',
+ { 'type': '/paragraph' },
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<!--
bar -->' ).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ { 'type': 'paragraph' },
+ 'B', 'a', 'r',
+ { 'type': '/paragraph' }
+ ],
+ 'calls': [
+ ['pushReplace', 0, 5, [] ]
+ ],
+ 'expected': function ( data ) {
+ // metadata is merged.
+ data.splice( 2, 2 );
data.splice( 4, 3 );
}
}
--
To view, visit https://gerrit.wikimedia.org/r/81928
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I51d55fb370b473273f9cf152fdd0f356377d4109
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Cscott <canan...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
