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 <thalia.e.c...@googlemail.com> Gerrit-Reviewer: Esanders <esand...@wikimedia.org> Gerrit-Reviewer: Tchanders <thalia.e.c...@googlemail.com> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits