Esanders has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/367921 )
Change subject: Abstract definition of type equality when comparing inline nodes
......................................................................
Abstract definition of type equality when comparing inline nodes
Bug: T166801
Change-Id: I0f24d9d81b5491a8f09bc59e5f544f99751fd506
---
M src/dm/ve.dm.Model.js
M src/dm/ve.dm.VisualDiff.js
M src/ve.DiffMatchPatch.js
M tests/ui/ve.ui.DiffElement.test.js
4 files changed, 67 insertions(+), 8 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor
refs/changes/21/367921/1
diff --git a/src/dm/ve.dm.Model.js b/src/dm/ve.dm.Model.js
index 007e3ba..399f5c0 100644
--- a/src/dm/ve.dm.Model.js
+++ b/src/dm/ve.dm.Model.js
@@ -280,6 +280,20 @@
}
};
+/**
+ * Check if this element is of the same type as another element for the
purposes of diffing.
+ *
+ * Elements which aren't of the same type will always be shown as removal and
an insertion,
+ * whereas comarable elements will be shown as an attribute change.
+ *
+ * @param {Object} element This element
+ * @param {Object} other Another element
+ * @return {boolean} Elements are of a comparable type
+ */
+ve.dm.Model.static.isDiffComparable = function ( element, other ) {
+ return element.type === other.type;
+};
+
/* Methods */
/**
@@ -417,3 +431,7 @@
ve.dm.Model.prototype.getHashObject = function () {
return this.constructor.static.getHashObject( this.element );
};
+
+ve.dm.Model.prototype.isDiffComparable = function ( other ) {
+ return this.constructor.static.isDiffComparable( this.element,
other.element );
+};
diff --git a/src/dm/ve.dm.VisualDiff.js b/src/dm/ve.dm.VisualDiff.js
index c6e2806..8104d02 100644
--- a/src/dm/ve.dm.VisualDiff.js
+++ b/src/dm/ve.dm.VisualDiff.js
@@ -163,7 +163,7 @@
ve.dm.VisualDiff.prototype.compareDocChildren = function ( oldDocChild,
newDocChild ) {
var i, ilen, oldData, newData, oldStore, newStore;
- if ( oldDocChild.length !== newDocChild.length || oldDocChild.type !==
newDocChild.type ) {
+ if ( oldDocChild.length !== newDocChild.length ||
!oldDocChild.isDiffComparable( newDocChild ) ) {
return false;
}
@@ -327,7 +327,7 @@
if ( !oldNode.canContainContent() &&
!newNode.canContainContent() ) {
// There is no content change
- replacement = oldNode.type !== newNode.type;
+ replacement = !oldNode.isDiffComparable(
newNode );
diffInfo[ i ] = {
linearDiff: null,
replacement: replacement,
@@ -362,7 +362,7 @@
// If we got this far, they are both CBNs
} else {
- replacement = oldNode.type !== newNode.type;
+ replacement = !oldNode.isDiffComparable(
newNode );
if ( !replacement && new Date().getTime() <
this.endTime ) {
linearDiff =
this.linearDiffer.getCleanDiff(
diff --git a/src/ve.DiffMatchPatch.js b/src/ve.DiffMatchPatch.js
index 85339aa..1e67a97 100644
--- a/src/ve.DiffMatchPatch.js
+++ b/src/ve.DiffMatchPatch.js
@@ -141,12 +141,20 @@
remove = [],
insert = [];
- function equalUnannotated( element, index, other ) {
+ function equalUnannotated( other, element, index ) {
return
ve.dm.ElementLinearData.static.compareElementsUnannotated( element, other[
index ] );
}
- function equalElements( element, index, other ) {
- return element.type && other[ index ].type &&
element.type === other[ index ].type;
+ function equalElements( other, element, index ) {
+ if ( !( element.type && other[ index ].type &&
element.type === other[ index ].type ) ) {
+ return false;
+ }
+ if ( ve.dm.LinearData.static.isOpenElementData( element
) ) {
+ // Elements have equal types, do a comparison
+ return ve.dm.modelRegistry.lookup( element.type
).static.isDiffComparable( element, other[ index ] );
+ } else {
+ return true;
+ }
}
function isWhitespace( element ) {
@@ -294,7 +302,7 @@
aData.length === bData.length &&
( ( aAction === DIFF_DELETE && bAction ===
DIFF_INSERT ) || ( aAction === DIFF_INSERT && bAction === DIFF_DELETE ) )
) {
- if ( aData.every( equalUnannotated.bind( bData
) ) ) {
+ if ( aData.every( equalUnannotated.bind( this,
bData ) ) ) {
aAnnotations = new
ve.dm.ElementLinearData( store, aData ).getAnnotationsFromRange( new ve.Range(
0, aData.length ), true );
bAnnotations = new
ve.dm.ElementLinearData( store, bData ).getAnnotationsFromRange( new ve.Range(
0, bData.length ), true );
@@ -313,7 +321,7 @@
cleanDiff[ i + 1 ][ 0 ] =
bAction === DIFF_DELETE ? DIFF_CHANGE_DELETE : DIFF_CHANGE_INSERT;
}
}
- if ( aData.every( equalElements.bind( bData ) )
) {
+ if ( aData.every( equalElements.bind( this,
bData ) ) ) {
attributeChanges = [];
// eslint-disable-next-line no-loop-func
bData.forEach( function ( element, i ) {
diff --git a/tests/ui/ve.ui.DiffElement.test.js
b/tests/ui/ve.ui.DiffElement.test.js
index 9622ba7..befa3d3 100644
--- a/tests/ui/ve.ui.DiffElement.test.js
+++ b/tests/ui/ve.ui.DiffElement.test.js
@@ -431,9 +431,42 @@
'<li><p>foo</p></li>' +
'</ul>' +
'</div>'
+ },
+ {
+ msg: 'Inline widget with same type but not diff
comparable is marked as a remove/insert',
+ oldDoc: '<p>Foo bar baz<span
rel="test:inlineWidget" data-name="FooWidget"></span></p>',
+ newDoc: '<p>Foo bar baz<span
rel="test:inlineWidget" data-name="BarWidget"></span></p>',
+ expected:
+ '<div
class="ve-ui-diffElement-doc-child-change">' +
+ '<p>Foo bar baz' +
+ '<del
data-diff-action="remove"><span rel="test:inlineWidget"
data-name="FooWidget"></span></del>' +
+ '<ins
data-diff-action="insert"><span rel="test:inlineWidget"
data-name="BarWidget"></span></ins>' +
+ '</p>' +
+ '</div>'
}
];
+ function InlineWidgetNode() {
+ InlineWidgetNode.super.apply( this, arguments );
+ }
+ OO.inheritClass( InlineWidgetNode, ve.dm.LeafNode );
+ InlineWidgetNode.static.name = 'testInlineWidget';
+ InlineWidgetNode.static.matchTagNames = [ 'span' ];
+ InlineWidgetNode.static.matchRdfaTypes = [ 'test:inlineWidget' ];
+ InlineWidgetNode.static.isContent = true;
+ InlineWidgetNode.static.toDataElement = function ( domElements ) {
+ return {
+ type: this.name,
+ attributes: {
+ name: domElements[ 0 ].getAttribute(
'data-name' )
+ }
+ };
+ };
+ InlineWidgetNode.static.isDiffComparable = function ( element, other ) {
+ return element.attributes.name === other.attributes.name;
+ };
+ ve.dm.modelRegistry.register( InlineWidgetNode );
+
for ( i = 0, len = cases.length; i < len; i++ ) {
oldDoc = ve.dm.converter.getModelFromDom(
ve.createDocumentFromHtml( cases[ i ].oldDoc ) );
newDoc = ve.dm.converter.getModelFromDom(
ve.createDocumentFromHtml( cases[ i ].newDoc ) );
--
To view, visit https://gerrit.wikimedia.org/r/367921
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0f24d9d81b5491a8f09bc59e5f544f99751fd506
Gerrit-PatchSet: 1
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Esanders <esand...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
