jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/374340 )
Change subject: VisualDiff: Don't diff close elements
......................................................................
VisualDiff: Don't diff close elements
Force the content diff to be balanced by removing
close elements prior to diffing, then re-inserting
afterwards.
Bug: T171862
Change-Id: I84aa157897ed5774b170e0dc4a3f7d264663c24d
---
M src/dm/ve.dm.VisualDiff.js
M src/ve.DiffMatchPatch.js
M tests/ui/ve.ui.DiffElement.test.js
3 files changed, 64 insertions(+), 7 deletions(-)
Approvals:
Esanders: Looks good to me, approved
jenkins-bot: Verified
diff --git a/src/dm/ve.dm.VisualDiff.js b/src/dm/ve.dm.VisualDiff.js
index 524cd89..022ee44 100644
--- a/src/dm/ve.dm.VisualDiff.js
+++ b/src/dm/ve.dm.VisualDiff.js
@@ -562,8 +562,7 @@
// Diff internal list items
diff = this.computeDiff(
oldDocInternalListItems.toDiff,
- newDocInternalListItems.toDiff,
- this.internalListDiff
+ newDocInternalListItems.toDiff
);
// Check there actually are any changes
diff --git a/src/ve.DiffMatchPatch.js b/src/ve.DiffMatchPatch.js
index 3d20936..b0f19a7 100644
--- a/src/ve.DiffMatchPatch.js
+++ b/src/ve.DiffMatchPatch.js
@@ -70,14 +70,39 @@
return [];
};
-ve.DiffMatchPatch.prototype.getCleanDiff = function () {
- var diffs = this.diff_main.apply( this, arguments ),
+ve.DiffMatchPatch.prototype.getCleanDiff = function ( oldData, newData,
options ) {
+ var cleanDiff, i, ilen, j,
store = this.store,
DIFF_DELETE = this.constructor.static.DIFF_DELETE,
DIFF_INSERT = this.constructor.static.DIFF_INSERT,
DIFF_EQUAL = this.constructor.static.DIFF_EQUAL,
DIFF_CHANGE_DELETE = this.constructor.static.DIFF_CHANGE_DELETE,
DIFF_CHANGE_INSERT = this.constructor.static.DIFF_CHANGE_INSERT;
+
+ /**
+ * Remove the close elements from the linear data, to force a balanced
diff.
+ *
+ * Otherwise, for example, removing the second of two consecutive
comment
+ * nodes could result in a diff where [{type: '/comment'}, {type:
'comment'}]
+ * is removed, which is unbalanced.
+ *
+ * Warning: this step assumes that, within a content branch node, an
element
+ * is always immediately followed by its close element.
+ *
+ * @param {Array} data Linear data
+ * @return {Array} Linear data without close elements
+ */
+ function removeCloseElements( data ) {
+ var i, ilen;
+ for ( i = 0, ilen = data.length; i < ilen; i++ ) {
+ if ( data[ i ].type && data[ i ].type[ 0 ] === '/' ) {
+ data.splice( i, 1 );
+ ilen--;
+ i--;
+ }
+ }
+ return data;
+ }
/**
* Get the index of the the first or last wordbreak in a data array
@@ -285,8 +310,8 @@
cleanDiff.push( [ DIFF_INSERT, insert ] );
}
- // Finally, go over any consecutive remove-inserts (also
insert-removes?)
- // and if they have the same character data, or are modified
content nodes,
+ // Now go over any consecutive remove-inserts (also
insert-removes?) and
+ // if they have the same character data, or are modified
content nodes,
// make them changes instead
for ( i = 0, ilen = cleanDiff.length - 1; i < ilen; i++ ) {
aItem = cleanDiff[ i ];
@@ -345,5 +370,24 @@
return cleanDiff;
}
- return getCleanDiff( diffs );
+ // Remove the close elements
+ oldData = removeCloseElements( oldData );
+ newData = removeCloseElements( newData );
+
+ // Get the diff
+ cleanDiff = getCleanDiff( this.diff_main( oldData, newData, options ) );
+
+ // Re-insert the close elements
+ for ( i = 0, ilen = cleanDiff.length; i < ilen; i++ ) {
+ for ( j = 0; j < cleanDiff[ i ][ 1 ].length; j++ ) {
+ if ( cleanDiff[ i ][ 1 ][ j ].type ) {
+ cleanDiff[ i ][ 1 ].splice( j + 1, 0, {
+ type: '/' + cleanDiff[ i ][ 1 ][ j
].type
+ } );
+ j++;
+ }
+ }
+ }
+
+ return cleanDiff;
};
diff --git a/tests/ui/ve.ui.DiffElement.test.js
b/tests/ui/ve.ui.DiffElement.test.js
index d3e66dc..665e136 100644
--- a/tests/ui/ve.ui.DiffElement.test.js
+++ b/tests/ui/ve.ui.DiffElement.test.js
@@ -473,6 +473,20 @@
'<ins
data-diff-action="insert"><span rel="test:inlineWidget"
data-name="BarWidget"></span></ins>' +
'</p>' +
'</div>'
+ },
+ {
+ msg: 'Adjacent inline node removed with common
prefix modified',
+ oldDoc: '<p>foo bar
<!--whee--><!--wibble--></p>',
+ newDoc: '<p>foo quux bar <!--whee--></p>',
+ expected:
+ '<div
class="ve-ui-diffElement-doc-child-change">' +
+ '<p>foo ' +
+ '<ins
data-diff-action="insert">quux </ins>' +
+ 'bar ' +
+ '<span rel="ve:Comment"
data-ve-comment="whee"> </span>' +
+ '<del
data-diff-action="remove"><span rel="ve:Comment"
data-ve-comment="wibble"> </span></del>' +
+ '</p>' +
+ '</div>'
}
];
--
To view, visit https://gerrit.wikimedia.org/r/374340
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I84aa157897ed5774b170e0dc4a3f7d264663c24d
Gerrit-PatchSet: 3
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Tchanders <[email protected]>
Gerrit-Reviewer: Esanders <[email protected]>
Gerrit-Reviewer: Tchanders <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
