diff --git a/src/structs/ContentMove.js b/src/structs/ContentMove.js index d6f4b07e..150e3d38 100644 --- a/src/structs/ContentMove.js +++ b/src/structs/ContentMove.js @@ -44,13 +44,13 @@ export const getMovedCoords = (moved, tr) => { } /** + * @param {Transaction} tr * @param {ContentMove} moved * @param {Item} movedItem * @param {Set} trackedMovedItems - * @param {Transaction} tr * @return {boolean} true if there is a loop */ -export const findMoveLoop = (moved, movedItem, trackedMovedItems, tr) => { +export const findMoveLoop = (tr, moved, movedItem, trackedMovedItems) => { if (trackedMovedItems.has(movedItem)) { return true } @@ -60,10 +60,13 @@ export const findMoveLoop = (moved, movedItem, trackedMovedItems, tr) => { */ let { start, end } = getMovedCoords(moved, tr) while (start !== end && start != null) { - if (start.deleted && start.moved === movedItem && start.content.constructor === ContentMove) { - if (findMoveLoop(start.content, start, trackedMovedItems, tr)) { - return true - } + if ( + !start.deleted && + start.moved === movedItem && + start.content.constructor === ContentMove && + findMoveLoop(tr, start.content, start, trackedMovedItems) + ) { + return true } start = start.right } @@ -171,6 +174,10 @@ export class ContentMove { transaction.prevMoved.set(start, prevMove) } start.moved = item + if (!start.deleted && start.content.constructor === ContentMove && findMoveLoop(transaction, start.content, start, new Set([item]))) { + item.deleteAsCleanup(transaction) + return + } } else if (currMoved != null) { /** @type {ContentMove} */ (currMoved.content).overrides.add(item) } diff --git a/src/structs/Item.js b/src/structs/Item.js index 11e4f4f5..b23ed6d3 100644 --- a/src/structs/Item.js +++ b/src/structs/Item.js @@ -119,6 +119,7 @@ export const splitItem = (transaction, leftItem, diff) => { } leftItem.length = diff if (leftItem.moved) { + rightItem.moved = leftItem.moved const m = transaction.prevMoved.get(leftItem) if (m) { transaction.prevMoved.set(rightItem, m) @@ -534,6 +535,24 @@ export class Item extends AbstractStruct { if (this.parentSub === null && this.countable && !this.deleted) { /** @type {AbstractType} */ (this.parent)._length += this.length } + // check if this item is in a moved range + if ((this.left && this.left.moved) || (this.right && this.right.moved)) { + const leftMoved = this.left && this.left.moved && /** @type {ContentMove} */ (this.left.moved.content) + const rightMoved = this.right && this.right.moved && /** @type {ContentMove} */ (this.right.moved.content) + if (leftMoved === rightMoved) { + this.moved = /** @type {Item} */ (this.left).moved + } else if ( + (leftMoved != null && !leftMoved.isCollapsed()) || + (rightMoved != null && !rightMoved.isCollapsed()) + ) { + // We know that this item is on the edge of a moved range. + // @todo Instead, we could check to which moved-range this item belongs + // This approach (reintegration) is pretty expensive in some scenarios + leftMoved && leftMoved.integrate(transaction, /** @type {any} */ (this.left).moved) + rightMoved && rightMoved.integrate(transaction, /** @type {any} */ (this.right).moved) + } + } + addStruct(transaction.doc.store, this) this.content.integrate(transaction, this) // add parent to transaction.changed @@ -644,6 +663,19 @@ export class Item extends AbstractStruct { } } + /** + * Similar to `this.delete(tr)`, but additionally ensures + * that the deleted range is broadcasted using a different + * origin/source in a separate update event, so that + * the providers don't filter this message. + * + * @param {Transaction} transaction + */ + deleteAsCleanup (transaction) { + this.delete(transaction) + addToDeleteSet(transaction.cleanupDeletions, this.id.client, this.id.clock, this.length) + } + /** * @param {StructStore} store * @param {boolean} parentGCd diff --git a/src/utils/Transaction.js b/src/utils/Transaction.js index 6476fa30..95749017 100644 --- a/src/utils/Transaction.js +++ b/src/utils/Transaction.js @@ -14,6 +14,7 @@ import { UpdateEncoderV1, UpdateEncoderV2, GC, StructStore, AbstractType, AbstractStruct, YEvent, Doc // eslint-disable-line } from '../internals.js' +import * as encoding from 'lib0/encoding' import * as map from 'lib0/map' import * as math from 'lib0/math' import * as set from 'lib0/set' @@ -61,6 +62,13 @@ export class Transaction { * @type {DeleteSet} */ this.deleteSet = new DeleteSet() + /** + * These deletes were used to cleanup the document and + * should be broadcasted again using a different transaction-origin. + * + * @type {DeleteSet} + */ + this.cleanupDeletions = new DeleteSet() /** * Holds the state before the transaction started. * @type {Map} @@ -140,6 +148,18 @@ export const writeUpdateMessageFromTransaction = (encoder, transaction) => { return true } +/** + * @param {UpdateEncoderV1 | UpdateEncoderV2} encoder + * @param {Transaction} transaction + */ +export const writeCleanupMessageFromTransaction = (encoder, transaction) => { + const ds = transaction.cleanupDeletions + sortAndMergeDeleteSet(ds) + // write structs: 0 structs were created + encoding.writeVarUint(encoder.restEncoder, 0) + writeDeleteSet(encoder, ds) +} + /** * @param {Transaction} transaction * @@ -344,11 +364,17 @@ const cleanupTransactions = (transactionCleanups, i) => { } // @todo Merge all the transactions into one and provide send the data as a single update message doc.emit('afterTransactionCleanup', [transaction, doc]) + const needsCleanupEvent = transaction.cleanupDeletions.clients.size > 0 if (doc._observers.has('update')) { const encoder = new UpdateEncoderV1() const hasContent = writeUpdateMessageFromTransaction(encoder, transaction) if (hasContent) { doc.emit('update', [encoder.toUint8Array(), transaction.origin, doc, transaction]) + if (needsCleanupEvent) { + const encoder = new UpdateEncoderV1() + writeCleanupMessageFromTransaction(encoder, transaction) + doc.emit('update', [encoder.toUint8Array(), 'cleanup', doc, transaction]) + } } } if (doc._observers.has('updateV2')) { @@ -356,6 +382,11 @@ const cleanupTransactions = (transactionCleanups, i) => { const hasContent = writeUpdateMessageFromTransaction(encoder, transaction) if (hasContent) { doc.emit('updateV2', [encoder.toUint8Array(), transaction.origin, doc, transaction]) + if (needsCleanupEvent) { + const encoder = new UpdateEncoderV2() + writeCleanupMessageFromTransaction(encoder, transaction) + doc.emit('updateV2', [encoder.toUint8Array(), 'cleanup', doc, transaction]) + } } } const { subdocsAdded, subdocsLoaded, subdocsRemoved } = transaction diff --git a/tests/y-array.tests.js b/tests/y-array.tests.js index e200aab9..15f59366 100644 --- a/tests/y-array.tests.js +++ b/tests/y-array.tests.js @@ -512,6 +512,23 @@ export const testMove2 = tc => { compare(users) } +/** + * @param {t.TestCase} tc + */ +export const testMoveCircles = tc => { + const { testConnector, array0, array1 } = init(tc, { users: 3 }) + array0.insert(0, [1, 2, 3, 4]) + testConnector.flushAllMessages() + array0.moveRange(0, 1, 3) + t.compare(array0.toArray(), [3, 1, 2, 4]) + array1.moveRange(2, 3, 1) + t.compare(array1.toArray(), [1, 3, 4, 2]) + testConnector.flushAllMessages() + t.assert(array0.length === 4) + t.assert(array0.length === array0.toArray().length) + t.compareArrays(array0.toArray(), array1.toArray()) +} + /** * @param {t.TestCase} tc */