From 04a999f991b59d5350eb36230663cc22732d2672 Mon Sep 17 00:00:00 2001 From: Catrope Date: Mon, 22 Oct 2012 17:53:58 -0700 Subject: [PATCH] Add change marking for Parsoid's benefit * Add map of change markers per offset to Transaction * Map is populated by TransactionProcessor * Markers are reversed on rollback * Removals aren't marked, Parsoid can detect these using DSR discontinuities Change-Id: I2290886ab411c6ad6162044ed85c091313613e51 --- modules/ve/dm/ve.dm.Converter.js | 9 ++ modules/ve/dm/ve.dm.Transaction.js | 66 +++++++++++ modules/ve/dm/ve.dm.TransactionProcessor.js | 107 +++++++++++++++++- modules/ve/test/dm/ve.dm.Converter.test.js | 2 +- .../ve/test/dm/ve.dm.SurfaceFragment.test.js | 51 ++++++--- .../dm/ve.dm.TransactionProcessor.test.js | 16 +++ modules/ve/test/dm/ve.dm.example.js | 20 ++++ 7 files changed, 252 insertions(+), 19 deletions(-) diff --git a/modules/ve/dm/ve.dm.Converter.js b/modules/ve/dm/ve.dm.Converter.js index 9c75f24282..0533678f2d 100644 --- a/modules/ve/dm/ve.dm.Converter.js +++ b/modules/ve/dm/ve.dm.Converter.js @@ -120,6 +120,15 @@ ve.dm.Converter.prototype.getDomElementFromDataElement = function ( dataElement } } } + // Change markers + if ( + dataElement.internal && dataElement.internal.changed && + !ve.isEmptyObject( dataElement.internal.changed ) + ) { + domElement.setAttribute( 'data-ve-changed', + JSON.stringify( dataElement.internal.changed ) + ); + } return domElement; }; diff --git a/modules/ve/dm/ve.dm.Transaction.js b/modules/ve/dm/ve.dm.Transaction.js index 1f964a1d3f..244adb625e 100644 --- a/modules/ve/dm/ve.dm.Transaction.js +++ b/modules/ve/dm/ve.dm.Transaction.js @@ -15,6 +15,7 @@ ve.dm.Transaction = function VeDmTransaction() { this.operations = []; this.lengthDifference = 0; this.applied = false; + this.changeMarkers = {}; }; /* Static Methods */ @@ -652,3 +653,68 @@ ve.dm.Transaction.prototype.pushStopAnnotating = function ( method, annotation ) 'annotation': annotation } ); }; + +/** + * Get the change markers for this transaction. Change markers are added using setChangeMarker(). + * + * @returns {Object} { offset: { markerType: number } } + */ +ve.dm.Transaction.prototype.getChangeMarkers = function () { + return this.changeMarkers; +}; + +/** + * Store a change marker to mark a change made while applying the transaction. Markers are stored + * in the .internal.changed property of elements in the linear model, as well as in the Transaction + * that effected the changes. + * + * The purpose of storing change markers in the linear model is so the linmod->HTML converter can + * mark what has changed relative to the HTML we originally received. For that reason, change + * markers only track what has changed relative to the original state of the document. This means + * we avoid reporting changes that cancel each other out, where possible. For instance, if an + * element marked 'created' is changed, this doesn't result in an additional change marker. + * In particular, rolling back a transaction causes all change marking done by that transaction to + * be undone. For that reason, change markers are stored in the Transaction object as well, so it's + * easy to undo a transaction's markers when rolling back. + * + * Marker types: + * - 'created': This element was newly created and did not exist in the original document + * - 'attributes': This element's attributes have changed + * - 'content': This element's content changed (content-containing elements only) + * - 'annotations': The annotations within this element changed (content-containing elements only) + * - 'rebuilt': This element and its children/contents changed in some way, no details available + * + * Change markers are numbers, which are incremented when setting a marker and decremented when + * unsetting it. This is because the same event can occur multiple times for the same element, and + * we want to be able to keep track of whether all the changes have canceled each other out. + * + * @param {Number} offset Linear model offset (post-transaction) of the element to mark + * @param {String} marker Marker type + * @param {Number} [increment=1] Number to add to the change marker counter + */ +ve.dm.Transaction.prototype.setChangeMarker = function ( offset, marker, increment ) { + increment = increment || 1; + if ( this.changeMarkers[offset] === undefined ) { + this.changeMarkers[offset] = {}; + } + if ( this.changeMarkers[offset].created ) { + // Can't set any other markers on a 'created' element + return; + } + if ( marker === 'created' ) { + // Clear other markers prior to setting 'created' + this.changeMarkers[offset] = {}; + } + if ( this.changeMarkers[offset][marker] === undefined ) { + this.changeMarkers[offset][marker] = increment; + } else { + this.changeMarkers[offset][marker] += increment; + } +}; + +/** + * Clear all change markers. + */ +ve.dm.Transaction.prototype.clearChangeMarkers = function () { + this.changeMarkers = {}; +}; diff --git a/modules/ve/dm/ve.dm.TransactionProcessor.js b/modules/ve/dm/ve.dm.TransactionProcessor.js index d4e5dee137..7afdf20a60 100644 --- a/modules/ve/dm/ve.dm.TransactionProcessor.js +++ b/modules/ve/dm/ve.dm.TransactionProcessor.js @@ -174,6 +174,7 @@ ve.dm.TransactionProcessor.processors.attribute = function ( op ) { op.key, from, to ); + this.setChangeMarker( this.cursor, 'attributes' ); }; /** @@ -196,7 +197,7 @@ ve.dm.TransactionProcessor.processors.attribute = function ( op ) { * @param {Array} op.insert Linear model data to insert */ ve.dm.TransactionProcessor.processors.replace = function ( op ) { - var node, selection, range, + var node, selection, range, parentOffset, remove = this.reversed ? op.insert : op.remove, insert = this.reversed ? op.remove : op.insert, removeIsContent = ve.dm.Document.isContentData( remove ), @@ -252,6 +253,13 @@ ve.dm.TransactionProcessor.processors.replace = function ( op ) { range.end + this.adjustment + insert.length - remove.length ) ); } + // Set change markers on the parents of the affected nodes + for ( i = 0; i < selection.length; i++ ) { + this.setChangeMarker( + selection[i].parentOuterRange.start + this.adjustment, + 'content' + ); + } // Advance the cursor this.cursor += insert.length; this.adjustment += insert.length - remove.length; @@ -283,6 +291,18 @@ ve.dm.TransactionProcessor.processors.replace = function ( op ) { ), 'siblings' ); for ( i = 0; i < selection.length; i++ ) { affectedRanges.push( selection[i].nodeOuterRange ); + if ( + selection[i].nodeOuterRange.start < prevCursor - this.adjustment && + selection[i].node.canContainContent() + ) { + // The opening element survives, so this + // node will have some of its content + // removed and/or have another node merged + // into it. Mark the node. + // TODO detect special case where closing is replaced + parentOffset = selection[i].nodeOuterRange.start + this.adjustment; + this.setChangeMarker( parentOffset, 'content' ); + } } } // Walk through the remove and insert data @@ -322,11 +342,18 @@ ve.dm.TransactionProcessor.processors.replace = function ( op ) { affectedRanges.push( new ve.Range( scopeStart, scopeEnd ) ); // Update scope scope = scope.getParent() || scope; + // Set change marker + this.transaction.setChangeMarker( + scopeStart + this.adjustment, + 'rebuilt' + ); } } else { // Opening element insertLevel++; + // Mark as 'created' + this.setChangeMarker( prevCursor + i, 'created' ); } } } @@ -396,6 +423,14 @@ ve.dm.TransactionProcessor.prototype.executeOperation = function ( op ) { */ ve.dm.TransactionProcessor.prototype.process = function () { var op; + if ( this.reversed ) { + // Undo change markers before rolling back the transaction, because the offsets + // are relevant to the post-commit state + this.applyChangeMarkers(); + // Unset the change markers we've just undone + this.transaction.clearChangeMarkers(); + } + // This loop is factored this way to allow operations to be skipped over or executed // from within other operations this.operationIndex = 0; @@ -403,6 +438,12 @@ ve.dm.TransactionProcessor.prototype.process = function () { this.executeOperation( op ); } this.synchronizer.synchronize(); + + if ( !this.reversed ) { + // Apply the change markers we've accumulated while processing the transaction + this.applyChangeMarkers(); + } + // Mark the transaction as committed or rolled back, as appropriate this.transaction.toggleApplied(); }; @@ -417,7 +458,7 @@ ve.dm.TransactionProcessor.prototype.process = function () { * @throws 'Invalid transaction, annotation to be cleared is not set' */ ve.dm.TransactionProcessor.prototype.applyAnnotations = function ( to ) { - var item, element, annotated, annotations, i; + var item, element, annotated, annotations, i, range, selection, offset; if ( this.set.isEmpty() && this.clear.isEmpty() ) { return; } @@ -464,5 +505,65 @@ ve.dm.TransactionProcessor.prototype.applyAnnotations = function ( to ) { } } } - this.synchronizer.pushAnnotation( new ve.Range( this.cursor, to ) ); + if ( this.cursor < to ) { + range = new ve.Range( this.cursor, to ); + selection = this.document.selectNodes( + new ve.Range( + this.cursor - this.adjustment, + to - this.adjustment + ), + 'leaves' + ); + for ( i = 0; i < selection.length; i++ ) { + offset = selection[i].node.isWrapped() ? + selection[i].nodeOuterRange.start : + selection[i].parentOuterRange.start; + this.setChangeMarker( offset + this.adjustment, 'annotations' ); + } + this.synchronizer.pushAnnotation( new ve.Range( this.cursor, to ) ); + } +}; + +/** + * Set a change marker on our transaction, if we are in commit mode. This function is a no-op in + * rollback mode. + * @see {ve.dm.Transaction.setChangeMarker} + */ +ve.dm.TransactionProcessor.prototype.setChangeMarker = function ( offset, type, increment ) { + // Refuse to set any new change markers while reversing transactions + if ( !this.reversed ) { + this.transaction.setChangeMarker( offset, type, increment ); + } +} + +/** + * Apply the change markers on this.transaction to this.document . Change markers are set + * (incremented) in commit mode, and unset (decremented) in rollback mode. + */ +ve.dm.TransactionProcessor.prototype.applyChangeMarkers = function () { + var offset, type, previousValue, newValue, element, + markers = this.transaction.getChangeMarkers(), + m = this.reversed ? -1 : 1; + for ( offset in markers ) { + for ( type in markers[offset] ) { + offset = Number( offset ); + element = this.document.data[offset]; + previousValue = ve.getProp( element, 'internal', 'changed', type ); + newValue = ( previousValue || 0 ) + m*markers[offset][type]; + if ( newValue != 0 ) { + ve.setProp( element, 'internal', 'changed', type, newValue ); + } else if ( previousValue !== undefined ) { + // Value was set but becomes zero, delete the key + delete element.internal.changed[type]; + // If that made .changed empty, delete it + if ( ve.isEmptyObject( element.internal.changed ) ) { + delete element.internal.changed; + } + // If that made .internal empty, delete it + if ( ve.isEmptyObject( element.internal ) ) { + delete element.internal; + } + } + } + } }; diff --git a/modules/ve/test/dm/ve.dm.Converter.test.js b/modules/ve/test/dm/ve.dm.Converter.test.js index b11d19fd10..da999ce534 100644 --- a/modules/ve/test/dm/ve.dm.Converter.test.js +++ b/modules/ve/test/dm/ve.dm.Converter.test.js @@ -51,7 +51,7 @@ QUnit.test( 'getDataFromDom', 30, function ( assert ) { } } ); -QUnit.test( 'getDomFromData', 31, function ( assert ) { +QUnit.test( 'getDomFromData', 32, function ( assert ) { var msg, cases = ve.dm.example.domToDataCases; diff --git a/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js b/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js index 1e4f1bddfc..abe5974d7d 100644 --- a/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js +++ b/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js @@ -73,14 +73,15 @@ QUnit.test( 'expandRange', 1, function ( assert ) { QUnit.test( 'removeContent', 2, function ( assert ) { var doc = new ve.dm.Document( ve.copyArray( ve.dm.example.data ) ), surface = new ve.dm.Surface( doc ), - fragment = new ve.dm.SurfaceFragment( surface, new ve.Range( 1, 56 ) ); + fragment = new ve.dm.SurfaceFragment( surface, new ve.Range( 1, 56 ) ), + expectedData = ve.copyArray( ve.dm.example.data.slice( 0, 1 ) ) + .concat( ve.copyArray( ve.dm.example.data.slice( 4, 5 ) ) ) + .concat( ve.copyArray( ve.dm.example.data.slice( 55 ) ) ); + ve.setProp( expectedData[0], 'internal', 'changed', 'content', 1 ); fragment.removeContent(); assert.deepEqual( doc.getData(), - ve.dm.example.data.slice( 0, 1 ) - .concat( ve.dm.example.data.slice( 4, 5 ) ) - .concat( ve.dm.example.data.slice( 55 ) - ), + expectedData, 'removing content drops fully covered nodes and strips partially covered ones' ); assert.deepEqual( @@ -124,15 +125,23 @@ QUnit.test( 'wrapNodes', 2, function ( assert ) { assert.deepEqual( doc.getData( new ve.Range( 55, 69 ) ), [ - { 'type': 'list', 'attributes': { 'style': 'bullet' } }, - { 'type': 'listItem' }, + { + 'type': 'list', + 'attributes': { 'style': 'bullet' }, + 'internal': { 'changed': { 'created': 1 } } + }, + { 'type': 'listItem', 'internal': { 'changed': { 'created': 1 } } }, { 'type': 'paragraph' }, 'l', { 'type': '/paragraph' }, { 'type': '/listItem' }, { 'type': '/list' }, - { 'type': 'list', 'attributes': { 'style': 'bullet' } }, - { 'type': 'listItem' }, + { + 'type': 'list', + 'attributes': { 'style': 'bullet' }, + 'internal': { 'changed': { 'created': 1 } } + }, + { 'type': 'listItem', 'internal': { 'changed': { 'created': 1 } } }, { 'type': 'paragraph' }, 'm', { 'type': '/paragraph' }, @@ -149,8 +158,12 @@ QUnit.test( 'wrapNodes', 2, function ( assert ) { assert.deepEqual( doc.getData( new ve.Range( 9, 16 ) ), [ - { 'type': 'list', 'attributes': { 'style': 'bullet' } }, - { 'type': 'listItem' }, + { + 'type': 'list', + 'attributes': { 'style': 'bullet' }, + 'internal': { 'changed': { 'created': 1 } } + }, + { 'type': 'listItem', 'internal': { 'changed': { 'created': 1 } } }, { 'type': 'paragraph' }, 'd', { 'type': '/paragraph' }, @@ -172,8 +185,12 @@ QUnit.test( 'wrapAllNodes', 2, function ( assert ) { assert.deepEqual( doc.getData( new ve.Range( 55, 65 ) ), [ - { 'type': 'list', 'attributes': { 'style': 'bullet' } }, - { 'type': 'listItem' }, + { + 'type': 'list', + 'attributes': { 'style': 'bullet' }, + 'internal': { 'changed': { 'created': 1 } } + }, + { 'type': 'listItem', 'internal': { 'changed': { 'created': 1 } } }, { 'type': 'paragraph' }, 'l', { 'type': '/paragraph' }, @@ -193,8 +210,12 @@ QUnit.test( 'wrapAllNodes', 2, function ( assert ) { assert.deepEqual( doc.getData( new ve.Range( 9, 16 ) ), [ - { 'type': 'list', 'attributes': { 'style': 'bullet' } }, - { 'type': 'listItem' }, + { + 'type': 'list', + 'attributes': { 'style': 'bullet' }, + 'internal': { 'changed': { 'created': 1 } } + }, + { 'type': 'listItem', 'internal': { 'changed': { 'created': 1 } } }, { 'type': 'paragraph' }, 'd', { 'type': '/paragraph' }, diff --git a/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js b/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js index 59301749a8..b716fb4e29 100644 --- a/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js +++ b/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js @@ -74,6 +74,7 @@ QUnit.test( 'commit/rollback', 58, function ( assert ) { data[1] = ['a', new ve.AnnotationSet( [ bold ] )]; data[2] = ['b', new ve.AnnotationSet( [ bold ] )]; data[3] = ['c', new ve.AnnotationSet( [ bold, underline ] )]; + ve.setProp( data[0], 'internal', 'changed', 'annotations', 2 ); } }, 'annotating content and leaf elements': { @@ -86,6 +87,8 @@ QUnit.test( 'commit/rollback', 58, function ( assert ) { 'expected': function ( data ) { data[38] = ['h', new ve.AnnotationSet( [ bold ] )]; data[39].annotations = new ve.AnnotationSet( [ bold ] ); + ve.setProp( data[37], 'internal', 'changed', 'annotations', 1 ); + ve.setProp( data[39], 'internal', 'changed', 'annotations', 1 ); } }, 'using an annotation method other than set or clear throws an exception': { @@ -145,6 +148,9 @@ QUnit.test( 'commit/rollback', 58, function ( assert ) { data[12].attributes.style = 'number'; data[12].attributes.test = 'abcd'; delete data[39].attributes['html/src']; + ve.setProp( data[0], 'internal', 'changed', 'attributes', 1 ); + ve.setProp( data[12], 'internal', 'changed', 'attributes', 2 ); + ve.setProp( data[39], 'internal', 'changed', 'attributes', 1 ); } }, 'changing attributes on non-element data throws an exception': { @@ -161,6 +167,7 @@ QUnit.test( 'commit/rollback', 58, function ( assert ) { ], 'expected': function ( data ) { data.splice( 1, 0, 'F', 'O', 'O' ); + ve.setProp( data[0], 'internal', 'changed', 'content', 1 ); } }, 'removing text': { @@ -170,6 +177,7 @@ QUnit.test( 'commit/rollback', 58, function ( assert ) { ], 'expected': function ( data ) { data.splice( 1, 1 ); + ve.setProp( data[0], 'internal', 'changed', 'content', 1 ); } }, 'replacing text': { @@ -179,6 +187,7 @@ QUnit.test( 'commit/rollback', 58, function ( assert ) { ], 'expected': function ( data ) { data.splice( 1, 1, 'F', 'O', 'O' ); + ve.setProp( data[0], 'internal', 'changed', 'content', 1 ); } }, 'inserting mixed content': { @@ -188,6 +197,7 @@ QUnit.test( 'commit/rollback', 58, function ( assert ) { ], 'expected': function ( data ) { data.splice( 1, 1, 'F', 'O', 'O', {'type':'image'}, {'type':'/image'}, 'B', 'A', 'R' ); + ve.setProp( data[0], 'internal', 'changed', 'content', 1 ); } }, 'converting an element': { @@ -204,6 +214,7 @@ QUnit.test( 'commit/rollback', 58, function ( assert ) { data[0].type = 'paragraph'; delete data[0].attributes; data[4].type = '/paragraph'; + ve.setProp( data[0], 'internal', 'changed', 'created', 1 ); } }, 'splitting an element': { @@ -222,6 +233,8 @@ QUnit.test( 'commit/rollback', 58, function ( assert ) { { 'type': '/heading' }, { 'type': 'heading', 'attributes': { 'level': 1 } } ); + ve.setProp( data[0], 'internal', 'changed', 'rebuilt', 1 ); + ve.setProp( data[3], 'internal', 'changed', 'created', 1 ); } }, 'merging an element': { @@ -235,6 +248,7 @@ QUnit.test( 'commit/rollback', 58, function ( assert ) { ], 'expected': function ( data ) { data.splice( 57, 2 ); + ve.setProp( data[55], 'internal', 'changed', 'content', 1 ); } }, 'stripping elements': { @@ -255,6 +269,8 @@ QUnit.test( 'commit/rollback', 58, function ( assert ) { 'expected': function ( data ) { data.splice( 10, 1 ); data.splice( 3, 1 ); + ve.setProp( data[0], 'internal', 'changed', 'content', 1 ); + ve.setProp( data[8], 'internal', 'changed', 'content', 1 ); } } }; diff --git a/modules/ve/test/dm/ve.dm.example.js b/modules/ve/test/dm/ve.dm.example.js index 0ddac8c361..03a5261274 100644 --- a/modules/ve/test/dm/ve.dm.example.js +++ b/modules/ve/test/dm/ve.dm.example.js @@ -1465,5 +1465,25 @@ ve.dm.example.domToDataCases = { '' + '', 'data': ve.dm.example.withMeta + }, + 'change markers': { + 'html': null, + 'data': [ + { 'type': 'paragraph', 'internal': { 'changed': { 'content': 1 } } }, + 'F', + 'o', + 'o', + { 'type': 'image', 'internal': { 'changed': { 'attributes': 2 } } }, + { 'type': '/image' }, + { 'type': '/paragraph' }, + { 'type': 'paragraph', 'internal': { 'changed': { 'created': 1 } } }, + 'B', + 'a', + 'r', + { 'type': '/paragraph' } + ], + 'normalizedHtml': '

' + + 'Foo' + + '

Bar

' } };