DLynch has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/344813 )
Change subject: SurfaceFragment: Avoid double-annotating in insertDocument, as
well
......................................................................
SurfaceFragment: Avoid double-annotating in insertDocument, as well
This applies e0933438's fix to insertContent to insertDocument. Took the
opportunity to switch insertDocument to use addAnnotationsToData as well,
though had to add a few new parameters to handle inserting from a different
store.
Bug: T143261
Change-Id: I13c30b38d33e7fffb2f50d06c87c22cead893c88
---
M src/dm/ve.dm.Document.js
M src/dm/ve.dm.SurfaceFragment.js
M tests/dm/ve.dm.SurfaceFragment.test.js
3 files changed, 43 insertions(+), 26 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor
refs/changes/13/344813/1
diff --git a/src/dm/ve.dm.Document.js b/src/dm/ve.dm.Document.js
index 1f5700b..972eabe 100644
--- a/src/dm/ve.dm.Document.js
+++ b/src/dm/ve.dm.Document.js
@@ -165,17 +165,29 @@
* @param {Array} data Data to apply annotations to
* @param {ve.dm.AnnotationSet} annotationSet Annotations to apply
* @param {boolean} [replaceComparable] Whether to remove annotations from the
data which are comparable to those in annotationSet
+ * @param {ve.dm.IndexValueStore} [store] Store associated with the data; only
needs to be provided if that data is associated with a different store than
annotationSet
+ * @param {number} [offset] Offset at which to insert annotationSet; defaults
to the end of the set
*/
-ve.dm.Document.static.addAnnotationsToData = function ( data, annotationSet,
replaceComparable ) {
- var i, length, newAnnotationSet, store = annotationSet.getStore();
+ve.dm.Document.static.addAnnotationsToData = function ( data, annotationSet,
replaceComparable, store, offset ) {
+ var i, length, newAnnotationSet;
if ( annotationSet.isEmpty() ) {
// Nothing to do
return;
}
+ store = store || annotationSet.getStore();
// Apply annotations to data
for ( i = 0, length = data.length; i < length; i++ ) {
if ( data[ i ].type ) {
// Element
+ if ( ve.dm.LinearData.static.isOpenElementData( data[ i
] ) ) {
+ // data[ i ].annotations =
annotations.storeIndexes.concat( item.annotations || [] );
+ newAnnotationSet = new ve.dm.AnnotationSet(
store, data[ i ].annotations || [] );
+ if ( replaceComparable ) {
+ newAnnotationSet =
newAnnotationSet.withoutComparableSet( annotationSet );
+ }
+ newAnnotationSet.addSet( annotationSet.clone(),
offset );
+ data[ i ].annotations =
newAnnotationSet.getIndexes();
+ }
continue;
} else if ( !Array.isArray( data[ i ] ) ) {
// Wrap in array
@@ -189,7 +201,7 @@
// comparable to those in annotationSet
newAnnotationSet =
newAnnotationSet.withoutComparableSet( annotationSet );
}
- newAnnotationSet.addSet( annotationSet.clone() );
+ newAnnotationSet.addSet( annotationSet.clone(), offset
);
}
data[ i ][ 1 ] = newAnnotationSet.getIndexes();
}
diff --git a/src/dm/ve.dm.SurfaceFragment.js b/src/dm/ve.dm.SurfaceFragment.js
index e5714ec..5e8eda8 100644
--- a/src/dm/ve.dm.SurfaceFragment.js
+++ b/src/dm/ve.dm.SurfaceFragment.js
@@ -818,7 +818,7 @@
// wrapping any annotations which are identical apart
from their
// original DOM element so we don't generate markup like
// <b>f<b>o</b>o</b> when pasting.
- ve.dm.Document.static.addAnnotationsToData( content,
annotations, true );
+ ve.dm.Document.static.addAnnotationsToData( content,
annotations, true, doc.store );
}
tx = ve.dm.TransactionBuilder.static.newFromInsertion( doc,
offset, content );
// Set the range to cover the inserted content; the offset
translation will be wrong
@@ -861,7 +861,7 @@
* @chainable
*/
ve.dm.SurfaceFragment.prototype.insertDocument = function ( newDoc,
newDocRange, annotate ) {
- var tx, newRange, annotations, offset, i, iLen, item, annotatedData,
annotatedDoc,
+ var tx, newRange, annotations, offset, annotatedData, annotatedDoc,
range = this.getSelection().getCoveringRange(),
doc = this.getDocument();
@@ -889,28 +889,10 @@
annotatedDoc = newDoc;
} else {
// Build shallow-cloned annotatedData array, copying on write
as we go
+ // FIXME T126022: the logic we actually need for annotating
inserted content
+ // correctly is much more complicated
annotatedData = newDoc.data.slice();
- for ( i = 0, iLen = newDoc.data.getLength(); i < iLen; i++ ) {
- item = annotatedData[ i ];
- // Insert surrounding annotations below newDoc
annotations
- // FIXME T126022: the logic we actually need for
annotating inserted
- // content correctly is MUCH more complicated
- if ( ve.dm.LinearData.static.isOpenElementData( item )
) {
- item = annotatedData[ i ] = ve.copy( item );
- item.annotations =
annotations.storeIndexes.concat( item.annotations || [] );
- } else if ( ve.dm.LinearData.static.isCloseElementData(
item ) ) {
- annotatedData[ i ] = ve.copy( item );
- } else if ( Array.isArray( item ) ) {
- annotatedData[ i ] = [
- item[ 0 ],
- annotations.storeIndexes.concat( item[
1 ] )
- ];
- } else if ( typeof item === 'string' ) {
- annotatedData[ i ] = [ item,
annotations.storeIndexes.slice() ];
- } else {
- throw new Error( 'Unknown item type' );
- }
- }
+ ve.dm.Document.static.addAnnotationsToData( annotatedData,
annotations, true, newDoc.store, 0 );
annotatedDoc = newDoc.cloneWithData( annotatedData );
}
tx = ve.dm.TransactionBuilder.static.newFromDocumentInsertion( doc,
offset, annotatedDoc, newDocRange );
diff --git a/tests/dm/ve.dm.SurfaceFragment.test.js
b/tests/dm/ve.dm.SurfaceFragment.test.js
index 86c5b70..d6279d0 100644
--- a/tests/dm/ve.dm.SurfaceFragment.test.js
+++ b/tests/dm/ve.dm.SurfaceFragment.test.js
@@ -599,6 +599,29 @@
],
'inserting content (annotate=true) reuses comparable
annotations on existing content'
);
+
+ doc = ve.dm.example.createExampleDocumentFromData( [
+ { type: 'paragraph' },
+ [ 'F', [ ve.dm.example.bold ] ],
+ [ 'o', [ ve.dm.example.bold ] ],
+ [ 'o', [ ve.dm.example.bold ] ],
+ { type: '/paragraph' },
+ { type: 'internalList' },
+ { type: '/internalList' }
+ ] );
+ surface = new ve.dm.Surface( doc );
+ fragment = surface.getLinearFragment( new ve.Range( 2 ) );
+ fragment.insertDocument( ve.dm.example.createExampleDocumentFromData( [
+ { type: 'paragraph' }, [ 'x', [ { type: 'textStyle/bold',
attributes: { nodeName: 'b', irrelevant: true } } ] ], { type: '/paragraph' },
+ { type: 'internalList' }, { type: '/internalList' }
+ ] ), new ve.Range( 1, 3 ), true );
+ assert.deepEqual(
+ doc.getData( new ve.Range( 2, 3 ) ),
+ [
+ [ 'x', [ ve.dm.example.boldIndex ] ]
+ ],
+ 'inserting document (annotate=true) reuses comparable
annotations on existing content'
+ );
} );
QUnit.test( 'changeAttributes', 1, function ( assert ) {
--
To view, visit https://gerrit.wikimedia.org/r/344813
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I13c30b38d33e7fffb2f50d06c87c22cead893c88
Gerrit-PatchSet: 1
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: DLynch <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
