fix circlic move-loops

src/structs/ContentMove.js

@@ -44,13 +44,13 @@ export const getMovedCoords = (moved, tr) => {
 }
 
 /**
+ * @param {Transaction} tr
  * @param {ContentMove} moved
  * @param {Item} movedItem
  * @param {Set<Item>} 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)
       }

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<any>} */ (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

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<Number,Number>}
@@ -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

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
  */