jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/353331 )
Change subject: VisualDiff: Show attribute changes for inline objects
......................................................................
VisualDiff: Show attribute changes for inline objects
Change-Id: I788e2d9dd44466c6683dd052818654df2f2334c3
---
M i18n/en.json
M i18n/qqq.json
M src/dm/nodes/ve.dm.CommentNode.js
M src/ui/elements/ve.ui.DiffElement.js
M src/ve.DiffMatchPatch.js
M tests/ui/ve.ui.DiffElement.test.js
6 files changed, 90 insertions(+), 38 deletions(-)
Approvals:
jenkins-bot: Verified
Jforrester: Looks good to me, approved
diff --git a/i18n/en.json b/i18n/en.json
index 6dc422f..80e3ea2 100644
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -37,6 +37,7 @@
"visualeditor-annotationbutton-underline-tooltip": "Underline",
"visualeditor-changedesc-align": "Alignment changed from $1 to $2",
"visualeditor-changedesc-changed": "$1 changed from $2 to $3",
+ "visualeditor-changedesc-comment": "Comment changed from $1 to $2",
"visualeditor-changedesc-image-size": "Size changed from $1 to $2",
"visualeditor-changedesc-language": "Language changed from $1 to $2",
"visualeditor-changedesc-link-href": "Link target changed from $1 to
$2",
diff --git a/i18n/qqq.json b/i18n/qqq.json
index 79c810a..f477435 100644
--- a/i18n/qqq.json
+++ b/i18n/qqq.json
@@ -45,6 +45,7 @@
"visualeditor-annotationbutton-underline-tooltip": "Tooltip text for
underline
button.\n{{Related|Visualeditor-annotationbutton}}\n{{Identical|Underline}}",
"visualeditor-changedesc-align": "Fallback description of an item's
'align' attribute value change for visual diffing, if no specific i18n exists
for that kind of item.\n\nValues:\n* $1 – Previous align value, like 'left'\n*
$2 – New align value, like 'center'",
"visualeditor-changedesc-changed": "Fallback description of an
attribute value change for visual diffing, if no specific i18n exists for that
kind of change.\n\nValues:\n* $1 – Attribute name, like 'lang' or 'dir'\n* $2 –
Previous attribute value, like 'ar' or 'rtl'\n* $3 – New attribute value, like
'fa' or 'auto'",
+ "visualeditor-changedesc-comment": "Description of a comment's change
for visual diffing.\n\nValues:\n* $1 – Previous comment\n* $2 – New comment",
"visualeditor-changedesc-image-size": "Description of an image's size
change for visual diffing.\n\nValues:\n* $1 – Previous size, like
'200x133px'\n* $2 – New size, like '300x200px'",
"visualeditor-changedesc-language": "Description of a language
annotation's language change for visual diffing.\n\nValues:\n* $1 – Previous
language, like 'ar' or 'fr'\n* $2 – New language, like 'de' or 'fa'",
"visualeditor-changedesc-link-href": "Description of a links's target
change for visual diffing.\n\nValues:\n* $1 – Previous target, like 'Hello
world' or 'https://www.wikimedia.org/'\n* $2 – New target, like 'Hello Earth'
or 'https://www.mediawiki.org/'",
diff --git a/src/dm/nodes/ve.dm.CommentNode.js
b/src/dm/nodes/ve.dm.CommentNode.js
index fe49988..f6792d0 100644
--- a/src/dm/nodes/ve.dm.CommentNode.js
+++ b/src/dm/nodes/ve.dm.CommentNode.js
@@ -70,6 +70,13 @@
}
};
+ve.dm.CommentNode.static.describeChange = function ( key, change ) {
+ if ( key === 'text' ) {
+ // TODO: Run comment changes through a linear differ.
+ return ve.msg( 'visualeditor-changedesc-comment', change.from,
change.to );
+ }
+};
+
/**
* @class
* @extends ve.dm.CommentNode
diff --git a/src/ui/elements/ve.ui.DiffElement.js
b/src/ui/elements/ve.ui.DiffElement.js
index e6cfd07..67e3961 100644
--- a/src/ui/elements/ve.ui.DiffElement.js
+++ b/src/ui/elements/ve.ui.DiffElement.js
@@ -696,7 +696,7 @@
*/
ve.ui.DiffElement.prototype.annotateNode = function ( linearDiff ) {
var i, ilen, range, type, typeAsString, annType, domElementType,
changes, item,
- annIndex, annIndexLists, j, height,
+ annIndex, annIndexLists, j, height, element,
DIFF_DELETE = ve.DiffMatchPatch.static.DIFF_DELETE,
DIFF_INSERT = ve.DiffMatchPatch.static.DIFF_INSERT,
DIFF_CHANGE_DELETE =
ve.DiffMatchPatch.static.DIFF_CHANGE_DELETE,
@@ -749,9 +749,10 @@
domElement.setAttribute( 'data-diff-action',
typeAsString );
domElements = [ domElement ];
+ changes = [];
if ( linearDiff[ i ].annotationChanges ) {
- changes = [];
- linearDiff[ i
].annotationChanges.forEach( function ( annotationChange ) { //
eslint-disable-line no-loop-func
+ // eslint-disable-next-line no-loop-func
+ linearDiff[ i
].annotationChanges.forEach( function ( annotationChange ) {
var attributeChanges;
if (
annotationChange.oldAnnotation && annotationChange.newAnnotation ) {
attributeChanges =
diffElement.constructor.static.compareAttributes(
@@ -763,11 +764,24 @@
) );
}
} );
- if ( changes.length ) {
- item =
diffElement.getChangeDescriptionItem( changes );
- domElement.setAttribute(
'data-diff-id', item.getData() );
- items.push( item );
- }
+ }
+ if ( linearDiff[ i ].attributeChanges ) {
+ element = linearDiff[ i ][ 1 ][ 0 ];
+ // eslint-disable-next-line no-loop-func
+ linearDiff[ i
].attributeChanges.forEach( function ( attributeChange ) {
+ var attributeChanges =
diffElement.constructor.static.compareAttributes(
+
attributeChange.oldAttributes,
+
attributeChange.newAttributes
+ );
+ changes = changes.concat(
ve.dm.modelRegistry.lookup( element.type ).static.describeChanges(
+ attributeChanges,
element.attributes, element
+ ) );
+ } );
+ }
+ if ( changes.length ) {
+ item =
diffElement.getChangeDescriptionItem( changes );
+ domElement.setAttribute(
'data-diff-id', item.getData() );
+ items.push( item );
}
originalDomElementsIndex =
diffDoc.getStore().index(
diff --git a/src/ve.DiffMatchPatch.js b/src/ve.DiffMatchPatch.js
index fc18ffa..59d250e 100644
--- a/src/ve.DiffMatchPatch.js
+++ b/src/ve.DiffMatchPatch.js
@@ -133,15 +133,19 @@
function getCleanDiff( diff ) {
var i, ilen, j, action, data, firstWordbreak, lastWordbreak,
start, end, aItem, bItem, aAction, bAction, aData,
bData,
- aAnnotations, bAnnotations, annotationChanges,
+ aAnnotations, bAnnotations, annotationChanges,
attributeChanges,
previousData = null,
previousAction = null,
cleanDiff = [],
remove = [],
insert = [];
- function compareData( element, index ) {
- return
ve.dm.ElementLinearData.static.compareElementsUnannotated( element, bData[
index ] );
+ function equalUnannotated( element, index, other ) {
+ 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 isWhitespace( element ) {
@@ -270,7 +274,8 @@
}
// Finally, go over any consecutive remove-inserts (also
insert-removes?)
- // and if they have the same character data, make them changes
instead
+ // 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 ];
bItem = cleanDiff[ i + 1 ];
@@ -284,25 +289,40 @@
// (2)
if (
aData.length === bData.length &&
- ( ( aAction === DIFF_DELETE && bAction ===
DIFF_INSERT ) || ( aAction === DIFF_INSERT && bAction === DIFF_DELETE ) ) &&
- aData.every( compareData )
+ ( ( aAction === DIFF_DELETE && bAction ===
DIFF_INSERT ) || ( aAction === DIFF_INSERT && bAction === DIFF_DELETE ) )
) {
- 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 );
+ if ( aData.every( equalUnannotated.bind( 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 );
- annotationChanges = [];
- bAnnotations.get().forEach( function ( b ) { //
eslint-disable-line no-loop-func
- var sameName =
aAnnotations.getAnnotationsByName( b.name );
- if ( sameName.getLength() &&
!aAnnotations.containsComparable( b ) ) {
- // Annotations which have the
same type, but are non-comparable, e.g. link with a different href
- annotationChanges.push( {
oldAnnotation: sameName.get( 0 ), newAnnotation: b } );
+ annotationChanges = [];
+ bAnnotations.get().forEach( function (
b ) { // eslint-disable-line no-loop-func
+ var sameName =
aAnnotations.getAnnotationsByName( b.name );
+ if ( sameName.getLength() &&
!aAnnotations.containsComparable( b ) ) {
+ // Annotations which
have the same type, but are non-comparable, e.g. link with a different href
+ annotationChanges.push(
{ oldAnnotation: sameName.get( 0 ), newAnnotation: b } );
+ }
+ } );
+
+ if ( annotationChanges.length ) {
+ cleanDiff[ i + 1
].annotationChanges = annotationChanges;
+ cleanDiff[ i ][ 0 ] = aAction
=== DIFF_DELETE ? DIFF_CHANGE_DELETE : DIFF_CHANGE_INSERT;
+ cleanDiff[ i + 1 ][ 0 ] =
bAction === DIFF_DELETE ? DIFF_CHANGE_DELETE : DIFF_CHANGE_INSERT;
}
- } );
-
- if ( annotationChanges.length ) {
- cleanDiff[ i + 1 ].annotationChanges =
annotationChanges;
- cleanDiff[ i ][ 0 ] = aAction ===
DIFF_DELETE ? DIFF_CHANGE_DELETE : DIFF_CHANGE_INSERT;
- cleanDiff[ i + 1 ][ 0 ] = bAction ===
DIFF_DELETE ? DIFF_CHANGE_DELETE : DIFF_CHANGE_INSERT;
+ }
+ if ( aData.every( equalElements.bind( bData ) )
) {
+ attributeChanges = [];
+ // eslint-disable-next-line no-loop-func
+ bData.forEach( function ( element, i ) {
+ if (
ve.dm.LinearData.static.isOpenElementData( element ) ) {
+ attributeChanges.push(
{ oldAttributes: aData[ i ].attributes, newAttributes: element.attributes,
index: i } );
+ }
+ } );
+ if ( attributeChanges.length ) {
+ cleanDiff[ i + 1
].attributeChanges = attributeChanges;
+ cleanDiff[ i ][ 0 ] = aAction
=== DIFF_DELETE ? DIFF_CHANGE_DELETE : DIFF_CHANGE_INSERT;
+ cleanDiff[ i + 1 ][ 0 ] =
bAction === DIFF_DELETE ? DIFF_CHANGE_DELETE : DIFF_CHANGE_INSERT;
+ }
}
// No need to check bItem against the following
item
diff --git a/tests/ui/ve.ui.DiffElement.test.js
b/tests/ui/ve.ui.DiffElement.test.js
index 2926650..209e5a7 100644
--- a/tests/ui/ve.ui.DiffElement.test.js
+++ b/tests/ui/ve.ui.DiffElement.test.js
@@ -154,43 +154,45 @@
{
msg: 'Inline node inserted',
oldDoc: '<p>foo bar baz quux</p>',
- newDoc: '<p>foo bar <!-- whee --> baz quux</p>',
+ newDoc: '<p>foo bar <!--whee--> baz quux</p>',
expected:
'<div
class="ve-ui-diffElement-doc-child-change">' +
'<p>' +
'foo bar ' +
- '<ins
data-diff-action="insert">' + comment( ' whee ' ) + ' </ins>' +
+ '<ins
data-diff-action="insert">' + comment( 'whee' ) + ' </ins>' +
'baz quux' +
'</p>' +
'</div>'
},
{
msg: 'Inline node removed',
- oldDoc: '<p>foo bar <!-- whee --> baz quux</p>',
+ oldDoc: '<p>foo bar <!--whee--> baz quux</p>',
newDoc: '<p>foo bar baz quux</p>',
expected:
'<div
class="ve-ui-diffElement-doc-child-change">' +
'<p>' +
'foo bar ' +
- '<del
data-diff-action="remove">' + comment( ' whee ' ) + ' </del>' +
+ '<del
data-diff-action="remove">' + comment( 'whee' ) + ' </del>' +
'baz quux' +
'</p>' +
'</div>'
},
{
msg: 'Inline node modified',
- oldDoc: '<p>foo bar <!-- whee --> baz quux</p>',
- newDoc: '<p>foo bar <!-- wibble --> baz
quux</p>',
+ oldDoc: '<p>foo bar <!--whee--> baz quux</p>',
+ newDoc: '<p>foo bar <!--wibble--> baz quux</p>',
expected:
'<div
class="ve-ui-diffElement-doc-child-change">' +
'<p>' +
'foo bar ' +
- '<del
data-diff-action="remove">' + comment( ' whee ' ) + '</del>' +
- '<ins
data-diff-action="insert">' + comment( ' wibble ' ) + '</ins>' +
+ '<span
data-diff-action="change-remove">' + comment( 'whee' ) + '</span>' +
+ '<span
data-diff-action="change-insert" data-diff-id="0">' + comment( 'wibble' ) +
'</span>' +
' baz quux' +
'</p>' +
- '</div>'
-
+ '</div>',
+ expectedDescriptions: [
+
'visualeditor-changedesc-comment,whee,wibble'
+ ]
},
{
msg: 'Paragraphs moved',
@@ -402,6 +404,13 @@
visualDiff = new ve.dm.VisualDiff( oldDoc, newDoc );
diffElement = new ve.ui.DiffElement( visualDiff );
assert.strictEqual( diffElement.$document.html(), cases[ i
].expected, cases[ i ].msg );
+ if ( cases[ i ].expectedDescriptions !== undefined ) {
+ assert.deepEqual(
+ diffElement.descriptions.items.map( function (
item ) { return item.$label.text(); } ),
+ cases[ i ].expectedDescriptions,
+ cases[ i ].msg + ': sidebar'
+ );
+ }
}
} );
--
To view, visit https://gerrit.wikimedia.org/r/353331
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I788e2d9dd44466c6683dd052818654df2f2334c3
Gerrit-PatchSet: 3
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Esanders <[email protected]>
Gerrit-Reviewer: Jforrester <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
