Catrope has uploaded a new change for review.
https://gerrit.wikimedia.org/r/80526
Change subject: [WIP] Fix handling of replace transactions in MetaList
......................................................................
[WIP] Fix handling of replace transactions in MetaList
Replace transactions generated by newFromDocumentInsertion() remove
and re-insert large chunks of data including metadata, which wasn't
possible before. The MetaList code didn't expect this and choked on it.
Rewrote the replace handling to deal with this case.
Change-Id: I8cdbc9a4f69bfa34da8aca013b0c1efb888a188a
---
M modules/ve/dm/ve.dm.MetaItem.js
M modules/ve/dm/ve.dm.MetaList.js
M modules/ve/test/dm/ve.dm.MetaList.test.js
3 files changed, 117 insertions(+), 108 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/VisualEditor
refs/changes/26/80526/1
diff --git a/modules/ve/dm/ve.dm.MetaItem.js b/modules/ve/dm/ve.dm.MetaItem.js
index 86a60ce..0927380 100644
--- a/modules/ve/dm/ve.dm.MetaItem.js
+++ b/modules/ve/dm/ve.dm.MetaItem.js
@@ -129,39 +129,6 @@
};
/**
- * Queue up a change to the item's offset and index.
- * @param {number} offset New offset
- * @param {number} index New index
- */
-ve.dm.MetaItem.prototype.setMove = function ( offset, index ) {
- this.move = {
- 'offset': offset,
- 'index': index
- };
-};
-
-/**
- * Whether or not a move is pending.
- * @returns {boolean} A move is pending
- */
-ve.dm.MetaItem.prototype.isMovePending = function () {
- return this.move !== null;
-};
-
-/**
- * Apply the pending move and clear it.
- * @throws No move pending
- */
-ve.dm.MetaItem.prototype.applyMove = function () {
- if ( this.move === null ) {
- throw new Error( 'No move pending' );
- }
- this.setOffset( this.move.offset );
- this.setIndex( this.move.index );
- this.move = null;
-};
-
-/**
* Attach this item to a MetaList.
* @param {ve.dm.MetaList} list Parent list to attach to
* @param {number} offset Offset of this item in the parent list's document
diff --git a/modules/ve/dm/ve.dm.MetaList.js b/modules/ve/dm/ve.dm.MetaList.js
index 1e88f2e..87027c9 100644
--- a/modules/ve/dm/ve.dm.MetaList.js
+++ b/modules/ve/dm/ve.dm.MetaList.js
@@ -80,17 +80,30 @@
* @emits remove
*/
ve.dm.MetaList.prototype.onTransact = function ( tx, reversed ) {
- var i, j, k, ilen, jlen, klen, ins, rm, retain, itemIndex, item,
- offset = 0, newOffset = 0, index = 0, ops = tx.getOperations(),
- insertedItems = [], removedItems = [];
- // Look for replaceMetadata operations in the transaction and
insert/remove items as appropriate
- // This requires we also inspect retain, replace and replaceMetadata
operations in order to
- // track the offset and index. We track the pre-transaction offset, we
need to do that in
- // order to remove items correctly. This also means inserted items are
initially at the wrong
- // offset, but we translate it later.
+ var i, ilen, j, jlen, k, klen, ins, rm, insMeta, rmMeta, retainMeta,
item, indexAdjustment,
+ slicedItems, offset = 0, newOffset = 0, index = 0,
insertedItems = [], removedItems = [],
+ ops = tx.getOperations(), metaList = this;
+
+ function adjustOffsets( from, to, adjustment ) {
+ var index;
+ if ( adjustment === 0 ) {
+ return;
+ }
+ index = metaList.findItem( from, 0, null, true );
+ while ( index < metaList.items.length && (
metaList.items[index].offset < to || to === null ) ) {
+ if ( metaList.items[index].offsetAdjustment ===
undefined ) {
+ metaList.items[index].offsetAdjustment =
adjustment;
+ }
+ index++;
+ }
+ }
+
for ( i = 0, ilen = ops.length; i < ilen; i++ ) {
switch ( ops[i].type ) {
case 'retain':
+ // Schedule the things we're retaining to be
moved
+ adjustOffsets( offset, offset + ops[i].length,
newOffset - offset );
+
offset += ops[i].length;
newOffset += ops[i].length;
index = 0;
@@ -98,91 +111,120 @@
case 'retainMetadata':
index += ops[i].length;
break;
- case 'replace':
- /*
- // if we have metadata replace info we can
calculate the new
- // offset and index directly
- ins = reversed ? ops[i].removeMetadata :
ops[i].insertMetadata;
- rm = reversed ? ops[i].insertMetadata :
ops[i].removeMetadata;
- retain = ops[i].retainMetadata || 0;
- if ( rm !== undefined ) {
- // find the first itemIndex - the rest
should be in order after it
- for ( j = 0, jlen = rm.length; j <
jlen; j++ ) {
- if ( rm[j] !== undefined ) {
- itemIndex =
this.findItem( offset + retain + j, 0 );
- break;
- }
+ case 'replaceMetadata':
+ insMeta = reversed ? ops[i].remove :
ops[i].insert;
+ rmMeta = reversed ? ops[i].insert :
ops[i].remove;
+ indexAdjustment = insMeta.length -
rmMeta.length;
+
+ // Remove removed items from the list
+ for ( j = 0, jlen = rmMeta.length; j < jlen;
j++ ) {
+ item = this.deleteRemovedItem( offset,
index + j );
+ removedItems.push( { 'item': item,
'offset': offset, 'index': index + j } );
+ }
+
+ // Find all items at this offset past the
replacement and adjust their indexes
+ // This has the potential to adjust the same
item multiple times if there are
+ // multiple replaceMetadata operations at the
same offset, but that's rare
+ j = this.findItem( offset, index +
rmMeta.length );
+ if ( j !== null ) {
+ while ( j < this.items.length &&
this.items[j].getOffset() === offset ) {
+ this.items[j].setIndex(
this.items[j].getIndex() + indexAdjustment );
+ j++;
}
- // iterate through all the inserted
metaItems
- for ( j = 0, jlen = ins.length; j <
jlen; j++ ) {
- item = ins[j];
- if ( item !== undefined ) {
- for ( k = 0, klen =
item.length; k < klen; k++ ) {
- // Queue up the
move for later so we don't break the metaItem ordering
-
this.items[itemIndex].setMove( newOffset + retain + j, k );
- itemIndex++;
+ }
+
+ // Insert inserted items into the list
+ for ( j = 0, jlen = insMeta.length; j < jlen;
j++ ) {
+ item =
ve.dm.metaItemFactory.createFromElement( insMeta[j] );
+ this.addInsertedItem( offset, index +
j, item );
+ // Do we need to schedule the new item
to be moved, maybe?
+ // No, I don't think so, because we'll
retain across this offset later
+ insertedItems.push( item );
+ }
+
+ index += insMeta.length; // because the indexes
have already been adjusted
+ break;
+ case 'replace':
+ ins = reversed ? ops[i].remove : ops[i].insert;
+ rm = reversed ? ops[i].insert : ops[i].remove;
+ if ( ops[i].retainMetadata !== undefined ) {
+ insMeta = reversed ?
ops[i].removeMetadata : ops[i].insertMetadata;
+ rmMeta = reversed ?
ops[i].insertMetadata : ops[i].removeMetadata;
+ retainMeta = ops[i].retainMetadata;
+
+ // Schedule the metadata we're
retaining to be moved
+ adjustOffsets( offset, offset +
retainMeta, newOffset - offset );
+
+ // Remove removed items from the list
+ for ( j = 0, jlen = rmMeta.length; j <
jlen; j++ ) {
+ if ( rmMeta[j] ) {
+ for ( k = 0, klen =
rmMeta[j].length; k < klen; k++ ) {
+ item =
this.deleteRemovedItem( offset + retainMeta + j, k );
+
removedItems.push( { 'item': item, 'offset': offset + retainMeta + j, 'index':
k } );
}
}
}
- }
- offset += reversed ? ops[i].insert.length :
ops[i].remove.length;
- newOffset += reversed ? ops[i].remove.length :
ops[i].insert.length;
- index = 0;
- break;
- */
- case 'replaceMetadata':
- if ( ops[i].type === 'replace' ) {
- ins = reversed ? ops[i].removeMetadata
: ops[i].insertMetadata;
- rm = reversed ? ops[i].insertMetadata :
ops[i].removeMetadata;
- retain = ops[i].retainMetadata || 0;
+
+ // Insert inserted items into the list
+ // Items are inserted at
pre-transaction offsets, then scheduled to be moved
+ // to post-transaction offsets
+ for ( j = 0, jlen = insMeta.length; j <
jlen; j++ ) {
+ if ( insMeta[j] ) {
+ for ( k = 0, klen =
insMeta[j].length; k < klen; k++ ) {
+ item =
ve.dm.metaItemFactory.createFromElement( insMeta[j][k] );
+
this.addInsertedItem( offset + retainMeta + j, k, item );
+
item.offsetAdjustment = newOffset - offset;
+
insertedItems.push( item );
+ }
+ }
+ }
} else {
- ins = reversed ? ops[i].remove :
ops[i].insert;
- rm = reversed ? ops[i].insert :
ops[i].remove;
- retain = 0;
+ // No metadata inserted or removed, but
we might be retaining across meta items
+ // Schedule any such items to be moved
+ adjustOffsets( offset, offset +
rm.length, newOffset - offset );
}
- for ( j = 0, jlen = rm.length; j < jlen; j++ ) {
- item = this.deleteRemovedItem( offset,
index + j );
- removedItems.push( { 'item': item,
'offset': offset, 'index': index } );
- }
- for ( j = 0, jlen = ins.length; j < jlen; j++ )
{
- item =
ve.dm.metaItemFactory.createFromElement( ins[j] );
- // offset and index are
pre-transaction, but we'll fix them later
- this.addInsertedItem( offset, index +
j, item );
- insertedItems.push( { 'item': item } );
- }
- index += rm.length;
+ offset += rm.length;
+ newOffset += ins.length;
break;
}
}
- // Translate the offsets of all items, and reindex them too
- // Reindexing is simple because the above ensures the items are already
in the right order
- offset = -1;
- index = 0;
- for ( i = 0, ilen = this.items.length; i < ilen; i++ ) {
- if ( this.items[i].isMovePending() ) {
- // move was calculated from metadata replace info, just
apply it
- this.items[i].applyMove();
- } else {
- // otherwise calculate the new offset from the
transaction
- this.items[i].setOffset( tx.translateOffset(
this.items[i].getOffset(), reversed ) );
- if ( this.items[i].getOffset() === offset ) {
- index++;
+ // Schedule everything else up to the end to be moved
+ adjustOffsets( offset, null, newOffset - offset );
+
+ // Process scheduled moves
+ // We have to operate on a shallow clone of the array because items are
gonna move around
+ // during the loop
+ slicedItems = this.items.slice();
+ for ( i = 0, ilen = slicedItems.length; i < ilen; i++ ) {
+ if ( slicedItems[i].offsetAdjustment !== undefined ) {
+ // We can't just change the offset of the item because
that can cause the items to
+ // get out of order. So instead, we remove it and
insert it again
+ offset = slicedItems[i].offset; // Cache these because
deleteRemovedItem() blanks them
+ index = slicedItems[i].index;
+ this.deleteRemovedItem( offset, index );
+
+ j = this.findItem( offset, 0 );
+ if ( j !== null ) {
+ // Our move conflicts with existing items,
reindex the entire offset
+ // TODO write this later
+ // TODO or maybe figure out in which direction
things should be reindexed
} else {
- index = 0;
+ // Our move doesn't conflict with existing
items, so we're fine
+ this.addInsertedItem( offset +
slicedItems[i].offsetAdjustment, index, slicedItems[i] );
}
- this.items[i].setIndex( index );
+ delete slicedItems[i].offsetAdjustment;
}
- offset = this.items[i].getOffset();
}
- for ( i = 0, ilen = insertedItems.length; i < ilen; i++ ) {
- this.emit( 'insert', insertedItems[i].item );
- }
+ // Emit events
for ( i = 0, ilen = removedItems.length; i < ilen; i++ ) {
this.emit( 'remove', removedItems[i].item,
removedItems[i].offset, removedItems[i].index );
}
+ for ( i = 0, ilen = insertedItems.length; i < ilen; i++ ) {
+ this.emit( 'insert', insertedItems[i] );
+ }
};
/**
diff --git a/modules/ve/test/dm/ve.dm.MetaList.test.js
b/modules/ve/test/dm/ve.dm.MetaList.test.js
index 8463a89..3a0108a 100644
--- a/modules/ve/test/dm/ve.dm.MetaList.test.js
+++ b/modules/ve/test/dm/ve.dm.MetaList.test.js
@@ -123,7 +123,7 @@
for ( j = 0; j < cases[i].calls.length; j++ ) {
tx[cases[i].calls[j][0]].apply( tx,
cases[i].calls[j].slice( 1 ) );
}
- doc = ve.dm.example.createExampleDocument( 'withMeta' ),
+ doc = ve.dm.example.createExampleDocument( 'withMeta' );
surface = new ve.dm.Surface( doc );
list = new ve.dm.MetaList( surface );
// Test both the transaction-via-surface and
transaction-via-document flows
--
To view, visit https://gerrit.wikimedia.org/r/80526
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8cdbc9a4f69bfa34da8aca013b0c1efb888a188a
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Catrope <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
