Esanders has uploaded a new change for review.
https://gerrit.wikimedia.org/r/49363
Change subject: (bug 45061) Remove all code related to change markers.
......................................................................
(bug 45061) Remove all code related to change markers.
Specifically by looking for "data-ve-changed",
"ChangeMarker*" and internal.changed.
Various tests, test counters and unused variables also
affected.
Change-Id: Ibd1ee68e0d650979d40574eff9cebded1a28499f
---
M modules/ve/dm/ve.dm.Converter.js
M modules/ve/dm/ve.dm.Transaction.js
M modules/ve/dm/ve.dm.TransactionProcessor.js
M modules/ve/init/mw/ve.init.mw.Platform.js
M modules/ve/init/ve.init.Platform.js
M modules/ve/test/dm/ve.dm.Converter.test.js
M modules/ve/test/dm/ve.dm.SurfaceFragment.test.js
M modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
M modules/ve/test/dm/ve.dm.example.js
9 files changed, 14 insertions(+), 291 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/VisualEditor
refs/changes/63/49363/1
diff --git a/modules/ve/dm/ve.dm.Converter.js b/modules/ve/dm/ve.dm.Converter.js
index aa10696..3862ab5 100644
--- a/modules/ve/dm/ve.dm.Converter.js
+++ b/modules/ve/dm/ve.dm.Converter.js
@@ -134,16 +134,6 @@
}
}
}
- // Change markers
- if (
- dataElement.internal && dataElement.internal.changed &&
- !ve.isEmptyObject( dataElement.internal.changed ) &&
- ve.init.platform.useChangeMarkers()
- ) {
- domElement.setAttribute( 'data-ve-changed',
- JSON.stringify( dataElement.internal.changed )
- );
- }
return domElement;
};
@@ -747,9 +737,9 @@
* @returns {HTMLElement} Wrapper div containing the resulting HTML
*/
ve.dm.Converter.prototype.getDomFromData = function ( data ) {
- var text, i, j, k, annotations, annotation, annotationElement,
dataElement, arr,
+ var text, i, j, annotations, annotation, annotationElement,
dataElement, arr,
childDomElement, pre, ours, theirs, parentDomElement,
startClosingAt,
- isContentNode, changed, parentChanged,
+ isContentNode,
container = document.createElement( 'div' ),
domElement = container,
annotationStack = new ve.AnnotationSet();
@@ -946,27 +936,6 @@
domElement.firstChild,
domElement
);
- }
- // Transfer change markers
- changed = domElement.getAttribute(
'data-ve-changed' );
- if ( changed ) {
- parentChanged =
parentDomElement.getAttribute( 'data-ve-changed' );
- if ( parentChanged ) {
- changed = $.parseJSON(
changed );
- parentChanged =
$.parseJSON( parentChanged );
- for ( k in changed ) {
- if ( k in
parentChanged ) {
-
parentChanged[k] += changed[k];
- } else {
-
parentChanged[k] = changed[k];
- }
- }
-
parentDomElement.setAttribute( 'data-ve-changed',
- JSON.stringify(
parentChanged ) );
- } else {
-
parentDomElement.setAttribute( 'data-ve-changed',
- changed );
- }
}
parentDomElement.removeChild(
domElement );
}
diff --git a/modules/ve/dm/ve.dm.Transaction.js
b/modules/ve/dm/ve.dm.Transaction.js
index 15bb450..a8babad 100644
--- a/modules/ve/dm/ve.dm.Transaction.js
+++ b/modules/ve/dm/ve.dm.Transaction.js
@@ -15,7 +15,6 @@
this.operations = [];
this.lengthDifference = 0;
this.applied = false;
- this.changeMarkers = {};
};
/* Static Methods */
@@ -863,72 +862,4 @@
'bias': 'stop',
'annotation': annotation
} );
-};
-
-/**
- * Get the change markers for the 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 5682e81..3cc8ba1 100644
--- a/modules/ve/dm/ve.dm.TransactionProcessor.js
+++ b/modules/ve/dm/ve.dm.TransactionProcessor.js
@@ -122,13 +122,6 @@
*/
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
@@ -138,10 +131,6 @@
}
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();
};
@@ -225,63 +214,8 @@
offset = !selection[i].node.isWrapped() &&
selection[i].parentOuterRange ?
selection[i].parentOuterRange.start :
selection[i].nodeOuterRange.start;
- this.setChangeMarker( offset + this.adjustment,
'annotations' );
}
this.synchronizer.pushAnnotation( new ve.Range( this.cursor, to
) );
- }
-};
-
-/**
- * Set a change marker on the transaction when in commit mode.
- *
- * This function is a no-op in rollback mode.
- *
- * @see ve.dm.Transaction#setChangeMarker
- *
- * @method
- * @param {number} offset
- * @param {string} type
- * @param {boolean} increment
- */
-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.
- *
- * @method
- */
-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;
- }
- }
- }
}
};
@@ -400,7 +334,6 @@
op.key,
from, to
);
- this.setChangeMarker( this.cursor, 'attributes' );
};
/**
@@ -422,7 +355,7 @@
* @param {Array} op.insert Linear model data to insert
*/
ve.dm.TransactionProcessor.processors.replace = function ( op ) {
- var node, selection, range, parentOffset,
+ var node, selection, range,
remove = this.reversed ? op.insert : op.remove,
insert = this.reversed ? op.remove : op.insert,
removeIsContent = ve.dm.Document.isContentData( remove ),
@@ -478,11 +411,6 @@
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++ ) {
- parentOffset = ( selection[i].parentOuterRange ||
selection[i].nodeOuterRange ).start;
- this.setChangeMarker( parentOffset + this.adjustment,
'content' );
- }
// Advance the cursor
this.advanceCursor( insert.length );
this.adjustment += insert.length - remove.length;
@@ -514,18 +442,6 @@
), '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
@@ -566,18 +482,11 @@
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' );
}
}
}
diff --git a/modules/ve/init/mw/ve.init.mw.Platform.js
b/modules/ve/init/mw/ve.init.mw.Platform.js
index 979d903..875fc47 100644
--- a/modules/ve/init/mw/ve.init.mw.Platform.js
+++ b/modules/ve/init/mw/ve.init.mw.Platform.js
@@ -55,22 +55,6 @@
};
/**
- * Check whether to use change markers.
- *
- * Uses the vechangemarkers query string variable.
- *
- * @method
- * @return {boolean}
- */
-ve.init.mw.Platform.prototype.useChangeMarkers = function () {
- var currentUri = new mw.Uri( window.location.toString() );
- if ( currentUri && 'venochangemarkers' in currentUri.query ) {
- return false;
- }
- return true;
-};
-
-/**
* Add multiple messages to the localization system.
*
* Wrapper for mw.msg system.
diff --git a/modules/ve/init/ve.init.Platform.js
b/modules/ve/init/ve.init.Platform.js
index b5af3e8..1d660f9 100644
--- a/modules/ve/init/ve.init.Platform.js
+++ b/modules/ve/init/ve.init.Platform.js
@@ -46,16 +46,6 @@
};
/**
- * Check whether to use change markers.
- *
- * @method
- * @returns {boolean}
- */
-ve.init.Platform.prototype.useChangeMarkers = function () {
- return true;
-};
-
-/**
* Add multiple messages to the localization system.
*
* @method
diff --git a/modules/ve/test/dm/ve.dm.Converter.test.js
b/modules/ve/test/dm/ve.dm.Converter.test.js
index 84b3529..2011056 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( 'getDomFromData', 53, function ( assert ) {
+QUnit.test( 'getDomFromData', 52, 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 1a7e418..fc8773e 100644
--- a/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js
+++ b/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js
@@ -77,7 +77,6 @@
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(),
@@ -127,10 +126,9 @@
[
{
'type': 'list',
- 'attributes': { 'style': 'bullet' },
- 'internal': { 'changed': { 'created': 1 } }
+ 'attributes': { 'style': 'bullet' }
},
- { 'type': 'listItem', 'internal': { 'changed': {
'created': 1 } } },
+ { 'type': 'listItem' },
{ 'type': 'paragraph' },
'l',
{ 'type': '/paragraph' },
@@ -138,10 +136,9 @@
{ 'type': '/list' },
{
'type': 'list',
- 'attributes': { 'style': 'bullet' },
- 'internal': { 'changed': { 'created': 1 } }
+ 'attributes': { 'style': 'bullet' }
},
- { 'type': 'listItem', 'internal': { 'changed': {
'created': 1 } } },
+ { 'type': 'listItem' },
{ 'type': 'paragraph' },
'm',
{ 'type': '/paragraph' },
@@ -160,10 +157,9 @@
[
{
'type': 'list',
- 'attributes': { 'style': 'bullet' },
- 'internal': { 'changed': { 'created': 1 } }
+ 'attributes': { 'style': 'bullet' }
},
- { 'type': 'listItem', 'internal': { 'changed': {
'created': 1 } } },
+ { 'type': 'listItem' },
{ 'type': 'paragraph' },
'd',
{ 'type': '/paragraph' },
@@ -187,10 +183,9 @@
[
{
'type': 'list',
- 'attributes': { 'style': 'bullet' },
- 'internal': { 'changed': { 'created': 1 } }
+ 'attributes': { 'style': 'bullet' }
},
- { 'type': 'listItem', 'internal': { 'changed': {
'created': 1 } } },
+ { 'type': 'listItem' },
{ 'type': 'paragraph' },
'l',
{ 'type': '/paragraph' },
@@ -212,10 +207,9 @@
[
{
'type': 'list',
- 'attributes': { 'style': 'bullet' },
- 'internal': { 'changed': { 'created': 1 } }
+ 'attributes': { 'style': 'bullet' }
},
- { 'type': 'listItem', 'internal': { 'changed': {
'created': 1 } } },
+ { 'type': 'listItem' },
{ '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 4fc0c0d..bfd1202 100644
--- a/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
+++ b/modules/ve/test/dm/ve.dm.TransactionProcessor.test.js
@@ -82,7 +82,6 @@
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': {
@@ -96,8 +95,6 @@
data[38] = ['h', new ve.AnnotationSet(
[ bold ] )];
data[39].annotations = new
ve.AnnotationSet( [ bold ] );
data[41] = ['i', new ve.AnnotationSet(
[ bold ] )];
- ve.setProp( data[37], 'internal',
'changed', 'annotations', 2 );
- ve.setProp( data[39], 'internal',
'changed', 'annotations', 1 );
}
},
'using an annotation method other than set or clear
throws an exception': {
@@ -157,9 +154,6 @@
data[12].attributes.style = 'number';
data[12].attributes.test = 'abcd';
delete
data[39].attributes['html/0/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': {
@@ -176,7 +170,6 @@
],
'expected': function ( data ) {
data.splice( 1, 0, 'F', 'O', 'O' );
- ve.setProp( data[0], 'internal',
'changed', 'content', 1 );
}
},
'removing text': {
@@ -186,7 +179,6 @@
],
'expected': function ( data ) {
data.splice( 1, 1 );
- ve.setProp( data[0], 'internal',
'changed', 'content', 1 );
}
},
'replacing text': {
@@ -196,7 +188,6 @@
],
'expected': function ( data ) {
data.splice( 1, 1, 'F', 'O', 'O' );
- ve.setProp( data[0], 'internal',
'changed', 'content', 1 );
}
},
'emptying text': {
@@ -206,7 +197,6 @@
],
'expected': function ( data ) {
data.splice( 10, 1 );
- ve.setProp( data[9], 'internal',
'changed', 'content', 1 );
}
},
'inserting mixed content': {
@@ -216,7 +206,6 @@
],
'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': {
@@ -233,7 +222,6 @@
data[0].type = 'paragraph';
delete data[0].attributes;
data[4].type = '/paragraph';
- ve.setProp( data[0], 'internal',
'changed', 'created', 1 );
}
},
'splitting an element': {
@@ -252,8 +240,6 @@
{ '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': {
@@ -267,7 +253,6 @@
],
'expected': function ( data ) {
data.splice( 57, 2 );
- ve.setProp( data[55], 'internal',
'changed', 'content', 1 );
}
},
'stripping elements': {
@@ -288,8 +273,6 @@
'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 );
}
},
'inserting text after alien node at the end': {
@@ -306,7 +289,6 @@
],
'expected': function ( data ) {
data.splice( 4, 0, 'b' );
- ve.setProp( data[0], 'internal',
'changed', 'content', 1 );
}
},
'inserting metadata element into existing element
list': {
diff --git a/modules/ve/test/dm/ve.dm.example.js
b/modules/ve/test/dm/ve.dm.example.js
index d4025e5..cd340f6 100644
--- a/modules/ve/test/dm/ve.dm.example.js
+++ b/modules/ve/test/dm/ve.dm.example.js
@@ -1731,42 +1731,6 @@
'<meta typeof="mw:Placeholder" data-parsoid="foobar"
/>',
'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' },
- { 'type': 'list', 'attributes': { 'style': 'bullet' } },
- { 'type': 'listItem' },
- {
- 'type': 'paragraph',
- 'internal': {
- 'generated': 'wrapper',
- 'changed': { 'content': 1 }
- }
- },
- 'B',
- 'a',
- 'z',
- { 'type': '/paragraph' },
- { 'type': '/listItem' },
- { 'type': '/list' }
- ],
- 'normalizedHtml': '<p
data-ve-changed="{"content":1}">' +
- 'Foo<img
data-ve-changed="{"attributes":2}" />' +
- '</p><p
data-ve-changed="{"created":1}">Bar</p>' +
- '<ul><li
data-ve-changed="{"content":1}">Baz</li></ul>'
- },
'about grouping': {
'html': '<div typeof="mw:Placeholder" about="#mwt1">Foo</div>' +
'<figure typeof="mw:Placeholder"
about="#mwt1">Bar</figure>' +
--
To view, visit https://gerrit.wikimedia.org/r/49363
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibd1ee68e0d650979d40574eff9cebded1a28499f
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Esanders <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
