Esanders has uploaded a new change for review. ( 
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
5 files changed, 72 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor 
refs/changes/31/353331/1

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..fb9bfc2 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,12 +764,26 @@
                                                        ) );
                                                }
                                        } );
-                                       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(
                                        domElements,
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

-- 
To view, visit https://gerrit.wikimedia.org/r/353331
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I788e2d9dd44466c6683dd052818654df2f2334c3
Gerrit-PatchSet: 1
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Esanders <[email protected]>

_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to