jenkins-bot has submitted this change and it was merged.
Change subject: Preserve location of reference body within key
......................................................................
Preserve location of reference body within key
Previously we populated the reference body into all <ref> tags
with the same key. Now we store an internal attribute marking
which element originally had the data.
If that tag is deleted the body is moved to the first <ref> tag
with that name.
Change-Id: If9f12bfb699e6ce85bb8f7d2ea9e6df528610a3d
---
M modules/ve/dm/nodes/ve.dm.MWReferenceNode.js
M modules/ve/dm/ve.dm.IndexValueStore.js
M modules/ve/dm/ve.dm.InternalList.js
M modules/ve/test/dm/ve.dm.IndexValueStore.test.js
M modules/ve/test/dm/ve.dm.InternalList.test.js
M modules/ve/test/dm/ve.dm.example.js
6 files changed, 127 insertions(+), 51 deletions(-)
Approvals:
Catrope: Looks good to me, approved
jenkins-bot: Verified
diff --git a/modules/ve/dm/nodes/ve.dm.MWReferenceNode.js
b/modules/ve/dm/nodes/ve.dm.MWReferenceNode.js
index 479c95b..3c46ff6 100644
--- a/modules/ve/dm/nodes/ve.dm.MWReferenceNode.js
+++ b/modules/ve/dm/nodes/ve.dm.MWReferenceNode.js
@@ -47,7 +47,9 @@
refGroup = mw.attrs.group || '',
listGroup = this.name + '/' + refGroup,
listKey = mw.attrs && mw.attrs.name !== undefined ?
mw.attrs.name : ve.getHash( body ),
- listIndex = converter.internalList.queueItemHtml( listGroup,
listKey, body );
+ queueResult = converter.internalList.queueItemHtml( listGroup,
listKey, body ),
+ listIndex = queueResult.index,
+ contentsUsed = ( body !== '' && queueResult.isNew );
dataElement = {
'type': this.name,
@@ -57,14 +59,15 @@
'listIndex': listIndex,
'listGroup': listGroup,
'listKey': listKey,
- 'refGroup': refGroup
+ 'refGroup': refGroup,
+ 'contentsUsed': contentsUsed
}
};
return dataElement;
};
ve.dm.MWReferenceNode.static.toDomElements = function ( dataElement, doc,
converter ) {
- var itemNodeHtml, mwAttr,
+ var itemNodeHtml, mwAttr, i, iLen, keyNodes, setContents,
span = doc.createElement( 'span' ),
itemNodeWrapper = doc.createElement( 'div' ),
itemNode = converter.internalList.getItemNode(
dataElement.attributes.listIndex ),
@@ -73,14 +76,39 @@
span.setAttribute( 'about', dataElement.attributes.about );
span.setAttribute( 'typeof', 'mw:Extension/ref' );
- converter.getDomSubtreeFromData(
- itemNode.getDocument().getData().slice( itemNodeRange.start,
itemNodeRange.end ),
- itemNodeWrapper
- ),
- itemNodeHtml = $( itemNodeWrapper ).html();
+ mwAttr = ve.copyObject( dataElement.attributes.mw ) || {};
- mwAttr = ( dataElement.attributes && ve.copyObject(
dataElement.attributes.mw ) ) || {};
- ve.setProp( mwAttr, 'body', 'html', itemNodeHtml );
+ setContents = dataElement.attributes.contentsUsed;
+
+ if ( !setContents ) {
+ // Check if any other nodes with this key provided content. If
not
+ // then we attach the contents to the first reference with this
key
+ keyNodes = converter.internalList
+ .getNodeGroup( dataElement.attributes.listGroup )
+ .keyNodes[dataElement.attributes.listKey];
+ // Check that this the first reference with its key
+ if ( dataElement === keyNodes[0].element ) {
+ setContents = true;
+ // Check no other reference originally defined the
contents
+ // As this is keyNodes[0] we can start at 1
+ for ( i = 1, iLen = keyNodes.length; i < iLen; i++ ) {
+ if (
keyNodes[i].element.attributes.contentsUsed ) {
+ setContents = false;
+ break;
+ }
+ }
+ }
+ }
+
+ if ( setContents ) {
+ converter.getDomSubtreeFromData(
+ itemNode.getDocument().getData().slice(
itemNodeRange.start, itemNodeRange.end ),
+ itemNodeWrapper
+ ),
+ itemNodeHtml = $( itemNodeWrapper ).html();
+ ve.setProp( mwAttr, 'body', 'html', itemNodeHtml );
+ }
+
span.setAttribute( 'data-mw', JSON.stringify( mwAttr ) );
return [ span ];
@@ -123,6 +151,12 @@
);
};
+ve.dm.MWReferenceNode.prototype.getClonedElement = function () {
+ var clone = ve.dm.LeafNode.prototype.getClonedElement.call( this );
+ delete clone.element.attributes.contentsUsed;
+ return clone;
+};
+
/* Registration */
ve.dm.modelRegistry.register( ve.dm.MWReferenceNode );
diff --git a/modules/ve/dm/ve.dm.IndexValueStore.js
b/modules/ve/dm/ve.dm.IndexValueStore.js
index 6e75d5e..350047e 100644
--- a/modules/ve/dm/ve.dm.IndexValueStore.js
+++ b/modules/ve/dm/ve.dm.IndexValueStore.js
@@ -28,21 +28,25 @@
* @method
* @param {Object|string|Array} value Value to lookup or store
* @param {string} [hash] Value hash. Uses ve.getHash( value ) if not provided.
+ * @param {boolean} [overwrite=false] Overwrite the value in the store if the
hash is already in use
* @returns {number} The index of the value in the store
*/
-ve.dm.IndexValueStore.prototype.index = function ( value, hash ) {
+ve.dm.IndexValueStore.prototype.index = function ( value, hash, overwrite ) {
var index;
if ( typeof hash !== 'string' ) {
hash = ve.getHash( value );
}
index = this.indexOfHash( hash );
- if ( index === null ) {
+ if ( index === null || overwrite ) {
+ if ( index === null ) {
+ index = this.valueStore.length;
+ }
if ( ve.isArray( value ) ) {
- index = this.valueStore.push( ve.copyArray( value ) ) -
1;
+ this.valueStore[index] = ve.copyArray( value );
} else if ( typeof value === 'object' ) {
- index = this.valueStore.push( ve.cloneObject( value ) )
- 1;
+ this.valueStore[index] = ve.cloneObject( value );
} else {
- index = this.valueStore.push( value ) - 1;
+ this.valueStore[index] = value;
}
this.hashStore[hash] = index;
}
diff --git a/modules/ve/dm/ve.dm.InternalList.js
b/modules/ve/dm/ve.dm.InternalList.js
index d787042..43fa5f9 100644
--- a/modules/ve/dm/ve.dm.InternalList.js
+++ b/modules/ve/dm/ve.dm.InternalList.js
@@ -46,21 +46,31 @@
/**
* Queues up an item's html for parsing later.
*
- * If an item with the specified group and key already exists it will be
ignored.
+ * If an item with the specified group and key already exists it will be
ignored, unless
+ * the data already stored is an empty string.
*
* @method
* @param {string} groupName Item group
* @param {string} key Item key
* @param {string} html Item contents
- * @returns {number} Index of the item in the index-value store, and also the
list
+ * @returns {Object} Object containing index of the item in the index-value
store
+ * (and also its index in the internal list node), and a flag indicating if it
is a new item.
*/
ve.dm.InternalList.prototype.queueItemHtml = function ( groupName, key, html )
{
- var index = this.getStore().indexOfHash( groupName + '/' + key );
+ var isNew = false, index = this.getStore().indexOfHash( groupName + '/'
+ key );
if ( index === null ) {
index = this.getStore().index( html, groupName + '/' + key );
this.itemHtmlQueue.push( index );
+ isNew = true;
+ } else if ( this.getStore().value( index ) === '' ) {
+ // Previous value with this key was empty, overwrite value in
store
+ this.getStore().index( html, groupName + '/' + key, true );
+ isNew = true;
}
- return index;
+ return {
+ 'index': index,
+ 'isNew': isNew
+ };
};
/**
diff --git a/modules/ve/test/dm/ve.dm.IndexValueStore.test.js
b/modules/ve/test/dm/ve.dm.IndexValueStore.test.js
index e0beeed..d5b4b2d 100644
--- a/modules/ve/test/dm/ve.dm.IndexValueStore.test.js
+++ b/modules/ve/test/dm/ve.dm.IndexValueStore.test.js
@@ -9,7 +9,7 @@
/* Tests */
-QUnit.test( 'index(es)/indexOfHash', 11, function ( assert ) {
+QUnit.test( 'index(es)/indexOfHash', 12, function ( assert ) {
var index, indexes,
object1 = { 'a': 1, 'b': 2 },
object2 = { 'c': 3, 'd': 4 },
@@ -26,8 +26,11 @@
index = store.index( object2, 'custom hash string' );
assert.equal( index, 2, 'Second object with custom hash stores in 2' );
index = store.index( object1, 'custom hash string' );
- assert.equal( index, 2, 'Using the same custom hash stores in 2 again'
);
- assert.deepEqual( store.value( 0 ), object1, '2nd item is second
object' );
+ assert.equal( index, 2, 'Using the same custom hash with a different
object returns index 2 again' );
+ assert.deepEqual( store.value( 2 ), object2, 'Second object was not
overwritten' );
+
+ index = store.index( object1, 'custom hash string', true );
+ assert.deepEqual( store.value( index ), object1, 'Second object is
overwritten when overwrite flag is set' );
assert.equal( store.indexOfHash( 'custom hash string' ), 2, 'Index of
custom hash is 2' );
assert.equal( store.indexOfHash( 'unused hash string' ), null, 'Index
of unused hash is null' );
diff --git a/modules/ve/test/dm/ve.dm.InternalList.test.js
b/modules/ve/test/dm/ve.dm.InternalList.test.js
index 704b019..28d745e 100644
--- a/modules/ve/test/dm/ve.dm.InternalList.test.js
+++ b/modules/ve/test/dm/ve.dm.InternalList.test.js
@@ -15,13 +15,34 @@
assert.deepEqual( internalList.getDocument(), doc, 'Returns original
document' );
} );
-QUnit.test( 'queueItemHtml/getItemHtmlQueue', 4, function ( assert ) {
+QUnit.test( 'queueItemHtml/getItemHtmlQueue', 5, function ( assert ) {
var doc = ve.dm.example.createExampleDocument(),
internalList = doc.getInternalList();
- assert.equal( internalList.queueItemHtml( 'reference', 'foo', 'Bar' ),
0, 'First queued item returns index 0' );
- assert.equal( internalList.queueItemHtml( 'reference', 'foo', 'Baz' ),
0, 'Duplicate key returns index 0' );
- assert.equal( internalList.queueItemHtml( 'reference', 'bar', 'Baz' ),
1, 'Second queued item returns index 1' );
- assert.deepEqual( internalList.getItemHtmlQueue(), ['Bar', 'Baz'],
'getItemHtmlQueue returns stored HTML items' );
+ assert.deepEqual(
+ internalList.queueItemHtml( 'reference', 'foo', 'Bar' ),
+ { 'index': 0, 'isNew': true },
+ 'First queued item returns index 0 and is new'
+ );
+ assert.deepEqual(
+ internalList.queueItemHtml( 'reference', 'foo', 'Baz' ),
+ { 'index': 0, 'isNew': false },
+ 'Duplicate key returns index 0 and is not new'
+ );
+ assert.deepEqual(
+ internalList.queueItemHtml( 'reference', 'bar', 'Baz' ),
+ { 'index': 1, 'isNew': true },
+ 'Second queued item returns index 1 and is new'
+ );
+
+ // Queue up empty data
+ internalList.queueItemHtml( 'reference', 'baz', '' ),
+ assert.deepEqual(
+ internalList.queueItemHtml( 'reference', 'baz', 'Quux' ),
+ { 'index': 2, 'isNew': true },
+ 'Third queued item is new because existing data in queue was
empty'
+ );
+
+ assert.deepEqual( internalList.getItemHtmlQueue(), ['Bar', 'Baz',
'Quux'], 'getItemHtmlQueue returns stored HTML items' );
} );
QUnit.test( 'convertToData', 2, function ( assert ) {
diff --git a/modules/ve/test/dm/ve.dm.example.js
b/modules/ve/test/dm/ve.dm.example.js
index f1e2482..6baa237 100644
--- a/modules/ve/test/dm/ve.dm.example.js
+++ b/modules/ve/test/dm/ve.dm.example.js
@@ -1187,32 +1187,32 @@
'<body>' +
'<p>Foo' +
'<span id="cite_ref-bar-1-0"
class="reference" about="#mwt5" typeof="mw:Extension/ref" ' +
-
'data-parsoid="{"src":"<ref
name=\\"bar\\">Bar</ref>"}"' +
-
'data-mw="{"body":{"html":"Bar"},"attrs":{"name":"bar"}}">'
+
+ 'data-parsoid="{}" ' +
+
'data-mw="{"body":{"html":""},"attrs":{"name":"bar"}}">'
+
'<a href="#cite_note-bar-1"
data-parsoid="{}">[1]</a>' +
'</span>' +
' Baz' +
'<span id="cite_ref-quux-2-0"
class="reference" about="#mwt6" typeof="mw:Extension/ref" ' +
-
'data-parsoid="{"src":"<ref
name=\\"quux\\">Quux</ref>"}"' +
+ 'data-parsoid="{}" ' +
'data-mw="{"body":{"html":"Quux"},"attrs":{"name":"quux"}}">'
+
'<a href="#cite_note-quux-2"
data-parsoid="{}">[2]</a>' +
'</span>' +
' Whee' +
'<span id="cite_ref-bar-1-1"
class="reference" about="#mwt7" typeof="mw:Extension/ref" ' +
-
'data-parsoid="{"src":"<ref name=\\"bar\\" />"}"'
+
-
'data-mw="{"body":{"html":""},"attrs":{"name":"bar"}}">'
+
+ 'data-parsoid="{}" ' +
+
'data-mw="{"body":{"html":"Bar"},"attrs":{"name":"bar"}}">'
+
'<a href="#cite_note-bar-1"
data-parsoid="{}">[1]</a>' +
'</span>' +
' Yay' +
'<span id="cite_ref-3-0"
class="reference" about="#mwt8" typeof="mw:Extension/ref" ' +
-
'data-parsoid="{"src":"<ref group=\\"g1\\">No
name</ref>"}"' +
+ 'data-parsoid="{}" ' +
'data-mw="{"body":{"html":"No
name"},"attrs":{"group":"g1"}}">' +
'<a href="#cite_note-3"
data-parsoid="{}">[3]</a>' +
'</span>' +
'</p>' +
'<ol class="references"
typeof="mw:Extension/references" ' +
'data-mw="{"name":"references","attrs":{}}" ' +
-
'data-parsoid="{"src":"<references />"}">' +
+ 'data-parsoid="{}">' +
'<li id="cite_note-quux-2"><a
href="#cite_ref-quux-2-0">u2191</a>Quux</li>' +
'</ol>' +
'</body>',
@@ -1227,7 +1227,8 @@
'listGroup': 'mwReference/',
'listKey': 'bar',
'refGroup': '',
- 'mw': { 'body': { 'html': 'Bar' },
'attrs': { 'name': 'bar' } }
+ 'mw': { 'body': { 'html': '' },
'attrs': { 'name': 'bar' } },
+ 'contentsUsed': false
},
'htmlAttributes': [
{
@@ -1235,8 +1236,8 @@
'values': {
'about': '#mwt5',
'class': 'reference',
- 'data-mw':
'{"body":{"html":"Bar"},"attrs":{"name":"bar"}}',
- 'data-parsoid':
'{"src":"<ref name=\\"bar\\">Bar</ref>"}',
+ 'data-mw':
'{"body":{"html":""},"attrs":{"name":"bar"}}',
+ 'data-parsoid': '{}',
'id':
'cite_ref-bar-1-0',
'typeof':
'mw:Extension/ref'
},
@@ -1262,7 +1263,8 @@
'listGroup': 'mwReference/',
'listKey': 'quux',
'refGroup': '',
- 'mw': { 'body': { 'html': 'Quux' },
'attrs': { 'name': 'quux' } }
+ 'mw': { 'body': { 'html': 'Quux' },
'attrs': { 'name': 'quux' } },
+ 'contentsUsed': true
},
'htmlAttributes': [
{
@@ -1271,7 +1273,7 @@
'about': '#mwt6',
'class': 'reference',
'data-mw':
'{"body":{"html":"Quux"},"attrs":{"name":"quux"}}',
- 'data-parsoid':
'{"src":"<ref name=\\"quux\\">Quux</ref>"}',
+ 'data-parsoid': '{}',
'id':
'cite_ref-quux-2-0',
'typeof':
'mw:Extension/ref'
},
@@ -1297,7 +1299,8 @@
'listGroup': 'mwReference/',
'listKey': 'bar',
'refGroup': '',
- 'mw': { 'body': { 'html': '' },
'attrs': { 'name': 'bar' } }
+ 'mw': { 'body': { 'html': 'Bar' },
'attrs': { 'name': 'bar' } },
+ 'contentsUsed': true
},
'htmlAttributes': [
{
@@ -1305,8 +1308,8 @@
'values': {
'about': '#mwt7',
'class': 'reference',
- 'data-mw':
'{"body":{"html":""},"attrs":{"name":"bar"}}',
- 'data-parsoid':
'{"src":"<ref name=\\"bar\\" />"}',
+ 'data-mw':
'{"body":{"html":"Bar"},"attrs":{"name":"bar"}}',
+ 'data-parsoid': '{}',
'id':
'cite_ref-bar-1-1',
'typeof':
'mw:Extension/ref'
},
@@ -1332,7 +1335,8 @@
'listGroup': 'mwReference/g1',
'listKey': '"No name"',
'refGroup': 'g1',
- 'mw': { 'body': { 'html': 'No name' },
'attrs': { 'group': 'g1' } }
+ 'mw': { 'body': { 'html': 'No name' },
'attrs': { 'group': 'g1' } },
+ 'contentsUsed': true
},
'htmlAttributes': [
{
@@ -1341,7 +1345,7 @@
'about': '#mwt8',
'class': 'reference',
'data-mw':
'{"body":{"html":"No name"},"attrs":{"group":"g1"}}',
- 'data-parsoid':
'{"src":"<ref group=\\"g1\\">No name</ref>"}',
+ 'data-parsoid': '{}',
'id': 'cite_ref-3-0',
'typeof':
'mw:Extension/ref'
},
@@ -1365,7 +1369,7 @@
'domElements': $(
'<ol class="references"
typeof="mw:Extension/references" '+
'data-mw="{"name":"references","attrs":{}}" ' +
-
'data-parsoid="{"src":"<references />"}">'+
+ 'data-parsoid="{}">'+
'<li
id="cite_note-quux-2"><a href="#cite_ref-quux-2-0">u2191</a>Quux</li>' +
'</ol>' ).get(),
'listGroup': 'mwReference/'
@@ -1393,28 +1397,28 @@
'normalizedHtml':
'<p>Foo' +
'<span id="cite_ref-bar-1-0" class="reference"
about="#mwt5" typeof="mw:Extension/ref" ' +
-
'data-parsoid="{"src":"<ref
name=\\"bar\\">Bar</ref>"}"' +
-
'data-mw="{"body":{"html":"Bar"},"attrs":{"name":"bar"}}">'
+
+ 'data-parsoid="{}" ' +
+
'data-mw="{"body":{"html":""},"attrs":{"name":"bar"}}">'
+
'</span>' +
' Baz' +
'<span id="cite_ref-quux-2-0" class="reference"
about="#mwt6" typeof="mw:Extension/ref" ' +
-
'data-parsoid="{"src":"<ref
name=\\"quux\\">Quux</ref>"}"' +
+ 'data-parsoid="{}" ' +
'data-mw="{"body":{"html":"Quux"},"attrs":{"name":"quux"}}">'
+
'</span>' +
' Whee' +
'<span id="cite_ref-bar-1-1" class="reference"
about="#mwt7" typeof="mw:Extension/ref" ' +
-
'data-parsoid="{"src":"<ref name=\\"bar\\" />"}"'
+
+ 'data-parsoid="{}" ' +
'data-mw="{"body":{"html":"Bar"},"attrs":{"name":"bar"}}">'
+
'</span>' +
' Yay' +
'<span id="cite_ref-3-0" class="reference"
about="#mwt8" typeof="mw:Extension/ref" ' +
-
'data-parsoid="{"src":"<ref group=\\"g1\\">No
name</ref>"}"' +
+ 'data-parsoid="{}" ' +
'data-mw="{"body":{"html":"No
name"},"attrs":{"group":"g1"}}">' +
'</span>' +
'</p>' +
'<ol class="references"
typeof="mw:Extension/references" ' +
'data-mw="{"name":"references","attrs":{}}" ' +
-
'data-parsoid="{"src":"<references />"}">' +
+ 'data-parsoid="{}">' +
'<li id="cite_note-quux-2"><a
href="#cite_ref-quux-2-0">u2191</a>Quux</li>' +
'</ol>'
},
--
To view, visit https://gerrit.wikimedia.org/r/67259
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If9f12bfb699e6ce85bb8f7d2ea9e6df528610a3d
Gerrit-PatchSet: 10
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Esanders <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: 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
