Catrope has uploaded a new change for review.
https://gerrit.wikimedia.org/r/173240
Change subject: Use strict comparison for insertionAnnotationsChange
......................................................................
Use strict comparison for insertionAnnotationsChange
74fc34ac changed the condition for emitting insertionAnnotationsChange
from a.containsAllOf( b ) && b.containsAllOf( a ) to a.compareTo( b ).
The containsAllOf test was stricter: if b contains an annotation
that is comparable to one in a, but not the same, the containsAllOf
test would fail, but the compareTo test would succeed.
This caused a bug with annotations with htmlAttributes. If you edited a
document containing <b>Foo</b> <b class="y">Bar</b>, placed
the cursor at Fo|o, typed "x", then placed the cursor at Ba|r,
and typed "x", you would expect <b>Foxo</b> <b class="y">Baxr</b>,
but you'd get <b>Foxo</b> <b class="y">Ba</b><b>x</b><b class="y">o</b>.
This happened because the insertionAnnotations were set to <b>
when you put the cursor in "Foo", and were not changed when
you put the cursor in "Bar" because <strong> is comparable to <b>.
Even the original containsAllOf test wasn't strict enough, because
it didn't take order into account: it would consider [b, i]
the same as [i, b], so you could still get the same broken behavior
with <b><i>Foo</i></b> <i><b>Bar</b></i> (although that seems to
be straightened out by the openAndCloseAnnotations() in the
converter in practice, but it was still being put in the DM wrong).
To fix both of these problems, use ve.dm.AnnotationSet#compareToStrict
which checks for exact equality: equal store indexes in the same order.
Change-Id: I2cb9ab66cc6e0933134cd313fe30d7d88e0844de
---
M src/dm/ve.dm.AnnotationSet.js
M src/dm/ve.dm.Surface.js
2 files changed, 26 insertions(+), 2 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor
refs/changes/40/173240/1
diff --git a/src/dm/ve.dm.AnnotationSet.js b/src/dm/ve.dm.AnnotationSet.js
index 5551937..2edf281 100644
--- a/src/dm/ve.dm.AnnotationSet.js
+++ b/src/dm/ve.dm.AnnotationSet.js
@@ -333,6 +333,29 @@
};
/**
+ * Strictly compare two annotation sets.
+ *
+ * This method only considers two annotation sets to be equal if they contain
exactly the same
+ * annotations (not just comparable, but with the same index in the
IndexValueStore)
+ * in exactly the same order.
+ *
+ * @param {ve.dm.AnnotationSet} annotationSet The annotation set to compare
this one to
+ * @returns {boolean} The annotation sets are equal
+ */
+ve.dm.AnnotationSet.prototype.compareToStrict = function ( annotationSet ) {
+ var i, len, ourIndexes = this.getIndexes(), theirIndexes =
annotationSet.getIndexes();
+ if ( ourIndexes.length !== theirIndexes.length ) {
+ return false;
+ }
+ for ( i = 0, len = ourIndexes.length; i < len; i++ ) {
+ if ( ourIndexes[i] !== theirIndexes[i] ) {
+ return false;
+ }
+ }
+ return true;
+};
+
+/**
* Add an annotation to the set.
*
* If the annotation is already present in the set, nothing happens.
diff --git a/src/dm/ve.dm.Surface.js b/src/dm/ve.dm.Surface.js
index 046fcb8..4a2f628 100644
--- a/src/dm/ve.dm.Surface.js
+++ b/src/dm/ve.dm.Surface.js
@@ -650,8 +650,9 @@
}
}
- // Only emit an annotations change event if there's a
meaningful difference
- if ( !insertionAnnotations.compareTo( this.insertionAnnotations
) ) {
+ // Only emit an annotations change event if there's a difference
+ // Note that ANY difference matters here, even order
+ if ( !insertionAnnotations.compareToStrict(
this.insertionAnnotations ) ) {
this.setInsertionAnnotations( insertionAnnotations );
}
}
--
To view, visit https://gerrit.wikimedia.org/r/173240
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2cb9ab66cc6e0933134cd313fe30d7d88e0844de
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
