Catrope has uploaded a new change for review.
https://gerrit.wikimedia.org/r/141063
Change subject: Also annotate metadata in TransactionProcessor
......................................................................
Also annotate metadata in TransactionProcessor
Annotation transactions weren't taking metadata into account at all.
This caused nasty issues when annotating across a mix of data and
metadata.
Changed applyAnnotations() to apply annotations to metadata as well
as data. To be safe and conservative, I only applied this to metadata
offsets entirely inside the range, not to the offsets on the edges.
This means annotations with comments at the end will still cause
problems. We may be able to fix that by making annotations smarter
and having them make themselves disappear when only applied to
metadata. Alternatively, reunifying data and metadata into a single
band will resolve this ambiguity.
Also factored out common code into a helper function and cleaned up
applyAnnotations() while I was in there, removing unneeded variables
in favor of calls to ElementLinearData methods.
Bug: 52127
Change-Id: Ie93fe50a46b13c240a6f56a25a989e0c978b663f
---
M modules/ve/dm/lineardata/ve.dm.MetaLinearData.js
M modules/ve/dm/ve.dm.TransactionProcessor.js
M modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
3 files changed, 162 insertions(+), 18 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor
refs/changes/63/141063/1
diff --git a/modules/ve/dm/lineardata/ve.dm.MetaLinearData.js
b/modules/ve/dm/lineardata/ve.dm.MetaLinearData.js
index 3d54b6d..c6d2531 100644
--- a/modules/ve/dm/lineardata/ve.dm.MetaLinearData.js
+++ b/modules/ve/dm/lineardata/ve.dm.MetaLinearData.js
@@ -106,3 +106,50 @@
}
return n;
};
+
+/**
+ * Get annotations' store indexes covered by an offset and index.
+ *
+ * @method
+ * @param {number} offset Offset to get annotations for
+ * @param {number} index Index to get annotations for
+ * @returns {number[]} An array of annotation store indexes the offset is
covered by
+ */
+ve.dm.MetaLinearData.prototype.getAnnotationIndexesFromOffsetAndIndex =
function ( offset, index ) {
+ var item = this.getData( offset, index );
+ return item && item.annotations || [];
+};
+
+/**
+ * Get annotations covered by an offset.
+ *
+ * The returned AnnotationSet is a clone of the one in the data.
+ *
+ * @method
+ * @param {number} offset Offset to get annotations for
+ * @param {number} index Index to get annotations for
+ * @returns {ve.dm.AnnotationSet} A set of all annotation objects offset is
covered by
+ */
+ve.dm.MetaLinearData.prototype.getAnnotationsFromOffsetAndIndex = function (
offset, index ) {
+ return new ve.dm.AnnotationSet( this.getStore(),
this.getAnnotationIndexesFromOffsetAndIndex( offset, index ) );
+};
+
+/**
+ * Set annotations of data at a specified offset.
+ *
+ * Cleans up data structure if annotation set is empty.
+ *
+ * @method
+ * @param {number} offset Offset to set annotations at
+ * @param {number} metadataOffset Index to set annotations at
+ * @param {ve.dm.AnnotationSet} annotations Annotations to set
+ */
+ve.dm.MetaLinearData.prototype.setAnnotationsAtOffsetAndIndex = function (
offset, index, annotations ) {
+ var item = this.getData( offset, index );
+ if ( annotations.isEmpty() ) {
+ // Clean up
+ delete item.annotations;
+ } else {
+ item.annotations = this.getStore().indexes( annotations.get() );
+ }
+};
diff --git a/modules/ve/dm/ve.dm.TransactionProcessor.js
b/modules/ve/dm/ve.dm.TransactionProcessor.js
index 809ebf3..7f41da4 100644
--- a/modules/ve/dm/ve.dm.TransactionProcessor.js
+++ b/modules/ve/dm/ve.dm.TransactionProcessor.js
@@ -114,13 +114,25 @@
* @throws {Error} Annotation to be cleared is not set
*/
ve.dm.TransactionProcessor.prototype.applyAnnotations = function ( to ) {
- var item, isElement, annotated, annotations, i, range, selection,
- store = this.document.getStore();
+ function setAndClear( anns, set, clear ) {
+ if ( anns.containsAnyOf( set ) ) {
+ throw new Error( 'Invalid transaction, annotation to be
set is already set' );
+ } else {
+ anns.addSet( set );
+ }
+ if ( !anns.containsAllOf( clear ) ) {
+ throw new Error( 'Invalid transaction, annotation to be
cleared is not set' );
+ } else {
+ anns.removeSet( clear );
+ }
+ }
+
+ var isElement, annotations, i, j, jlen, range, selection;
if ( this.set.isEmpty() && this.clear.isEmpty() ) {
return;
}
+ // Set/clear annotations on data
for ( i = this.cursor; i < to; i++ ) {
- item = this.document.data.getData( i );
isElement = this.document.data.isElementData( i );
if ( isElement ) {
if ( !ve.dm.nodeFactory.isNodeContent(
this.document.data.getType( i ) ) ) {
@@ -131,24 +143,20 @@
continue;
}
}
- annotated = isElement ? 'annotations' in item : ve.isArray(
item );
- annotations = annotated ?
- new ve.dm.AnnotationSet( store, isElement ?
item.annotations : item[1] ) :
- new ve.dm.AnnotationSet( store );
- // Set and clear annotations
- if ( annotations.containsAnyOf( this.set ) ) {
- throw new Error( 'Invalid transaction, annotation to be
set is already set' );
- } else {
- annotations.addSet( this.set );
- }
- if ( !annotations.containsAllOf( this.clear ) ) {
- throw new Error( 'Invalid transaction, annotation to be
cleared is not set' );
- } else {
- annotations.removeSet( this.clear );
- }
+ annotations = this.document.data.getAnnotationsFromOffset( i );
+ setAndClear( annotations, this.set, this.clear );
// Store annotation indexes in linear model
this.document.data.setAnnotationsAtOffset( i, annotations );
}
+ // Set/clear annotations on metadata, but not on the edges of the range
+ for ( i = this.cursor + 1; i < to; i++ ) {
+ for ( j = 0, jlen = this.document.metadata.getDataLength( i );
j < jlen; j++ ) {
+ annotations =
this.document.metadata.getAnnotationsFromOffsetAndIndex( i, j );
+ setAndClear( annotations, this.set, this.clear );
+ this.document.metadata.setAnnotationsAtOffsetAndIndex(
i, j, annotations );
+ }
+ }
+ // Notify the synchronizer
if ( this.cursor < to ) {
range = new ve.Range( this.cursor, to );
selection = this.document.selectNodes(
diff --git a/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
b/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
index be7ec3d..d27013c 100644
--- a/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
+++ b/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
@@ -25,6 +25,35 @@
}
},
metaElementInsertClose = { 'type': '/alienMeta' },
+ metadataExample = [
+ { 'type': 'paragraph' },
+ 'a', 'b',
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<!-- comment -->'
).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ 'c', 'd',
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<!-- comment -->'
).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ 'e', 'f',
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<!-- comment -->'
).toArray()
+ }
+ },
+ { 'type': '/alienMeta' },
+ 'g', 'h',
+ { 'type': '/paragraph' }
+ ],
cases = {
'no operations': {
'calls': [],
@@ -68,6 +97,66 @@
data[41] = ['i', store.indexes( [ bold
] )];
}
},
+ 'annotating across metadata': {
+ 'data': metadataExample,
+ 'calls': [
+ ['pushRetain', 2],
+ ['pushStartAnnotating', 'set', bold],
+ ['pushRetain', 2],
+ ['pushStopAnnotating', 'set', bold],
+ ['pushRetain', 6]
+ ],
+ 'expected': function ( data ) {
+ data[2] = ['b', store.indexes( [ bold ]
)];
+ data[3].annotations = store.indexes( [
bold ] );
+ data[5] = ['c', store.indexes( [ bold ]
)];
+ }
+ },
+ 'annotating with metadata at edges': {
+ 'data': metadataExample,
+ 'calls': [
+ ['pushRetain', 3],
+ ['pushStartAnnotating', 'set', bold],
+ ['pushRetain', 4],
+ ['pushStopAnnotating', 'set', bold],
+ ['pushRetain', 3]
+ ],
+ 'expected': function ( data ) {
+ data[7].annotations = store.indexes( [
bold ] );
+ data[5] = ['c', store.indexes( [ bold ]
)];
+ data[6] = ['d', store.indexes( [ bold ]
)];
+ data[9] = ['e', store.indexes( [ bold ]
)];
+ data[10] = ['f', store.indexes( [ bold
] )];
+ }
+ },
+ 'unannotating metadata': {
+ 'data': [
+ { 'type': 'paragraph' },
+ 'a', ['b', store.indexes( [ bold ] )],
+ {
+ 'type': 'alienMeta',
+ 'attributes': {
+ 'domElements': $( '<!--
comment -->' ).toArray()
+ },
+ 'annotations': store.indexes( [
bold ] )
+ },
+ { 'type': '/alienMeta' },
+ ['c', store.indexes( [ bold ] )], 'd',
+ { 'type': '/paragraph' }
+ ],
+ 'calls': [
+ ['pushRetain', 2],
+ ['pushStartAnnotating', 'clear', bold],
+ ['pushRetain', 2],
+ ['pushStopAnnotating', 'clear', bold],
+ ['pushRetain', 6]
+ ],
+ 'expected': function ( data ) {
+ data[2] = 'b';
+ data[5] = 'c';
+ delete data[3].annotations;
+ }
+ },
'using an annotation method other than set or clear
throws an exception': {
'calls': [
['pushStartAnnotating',
'invalid-method', bold],
--
To view, visit https://gerrit.wikimedia.org/r/141063
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie93fe50a46b13c240a6f56a25a989e0c978b663f
Gerrit-PatchSet: 1
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Catrope <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
