fix all remaining bugs for single-item moves (mainly event bugs)

src/structs/ContentMove.js

@@ -11,7 +11,7 @@ import { decodeRelativePosition, encodeRelativePosition } from 'yjs'
 /**
  * @param {ContentMove} moved
  * @param {Transaction} tr
- * @return {{ start: Item, end: Item | null }} $start (inclusive) is the beginning and $end (exclusive) is the end of the moved area
+ * @return {{ start: Item, end: Item }} $start (inclusive) is the beginning and $end (inclusive) is the end of the moved area
  */
 export const getMovedCoords = (moved, tr) => {
   let start // this (inclusive) is the beginning of the moved area
@@ -38,36 +38,9 @@ export const getMovedCoords = (moved, tr) => {
       end = getItemCleanStart(tr, moved.end.item)
     }
   } else {
-    end = null
+    error.unexpectedCase()
-  }
-  return { start: /** @type {Item} */ (start), end }
-}
-
-/**
- * @todo remove this if not needed
- *
- * @param {ContentMove} moved
- * @param {Item} movedItem
- * @param {Transaction} tr
- * @param {function(Item):void} cb
- */
-export const iterateMoved = (moved, movedItem, tr, cb) => {
-  /**
-   * @type {{ start: Item | null, end: Item | null }}
-   */
-  let { start, end } = getMovedCoords(moved, tr)
-  while (start !== end && start != null) {
-    if (!start.deleted) {
-      if (start.moved === movedItem) {
-        if (start.content.constructor === ContentMove) {
-          iterateMoved(start.content, start, tr, cb)
-        } else {
-          cb(start)
-        }
-      }
-    }
-    start = start.right
   }
+  return { start: /** @type {Item} */ (start), end: /** @type {Item} */ (end) }
 }
 
 /**
@@ -172,7 +145,8 @@ export class ContentMove {
    * @param {Item} item
    */
   integrate (transaction, item) {
-    /** @type {AbstractType<any>} */ (item.parent)._searchMarker = []
+    const sm = /** @type {AbstractType<any>} */ (item.parent)._searchMarker
+    if (sm) sm.length = 0
     /**
      * @type {{ start: Item | null, end: Item | null }}
      */
@@ -182,25 +156,23 @@ export class ContentMove {
     // that we want to set prio to the current prio-maximum of the moved range.
     const adaptPriority = this.priority < 0
     while (start !== end && start != null) {
-      if (!start.deleted) {
+      const currMoved = start.moved
-        const currMoved = start.moved
+      const nextPrio = currMoved ? /** @type {ContentMove} */ (currMoved.content).priority : -1
-        const nextPrio = currMoved ? /** @type {ContentMove} */ (currMoved.content).priority : -1
+      if (adaptPriority || nextPrio < this.priority || (currMoved != null && nextPrio === this.priority && (currMoved.id.client < item.id.client || (currMoved.id.client === item.id.client && currMoved.id.clock < item.id.clock)))) {
-        if (adaptPriority || nextPrio < this.priority || (currMoved != null && nextPrio === this.priority && (currMoved.id.client < item.id.client || (currMoved.id.client === item.id.client && currMoved.id.clock < item.id.clock)))) {
+        if (currMoved !== null) {
-          if (currMoved !== null) {
+          this.overrides.add(currMoved)
-            this.overrides.add(currMoved)
-          }
-          maxPriority = math.max(maxPriority, nextPrio)
-          // was already moved
-          const prevMove = start.moved
-          if (prevMove && !transaction.prevMoved.has(start) && prevMove.id.clock < (transaction.beforeState.get(prevMove.id.client) || 0)) {
-            // only override prevMoved if the prevMoved item is not new
-            // we need to know which item previously moved an item
-            transaction.prevMoved.set(start, prevMove)
-          }
-          start.moved = item
-        } else if (currMoved != null) {
-          /** @type {ContentMove} */ (currMoved.content).overrides.add(item)
         }
+        maxPriority = math.max(maxPriority, nextPrio)
+        // was already moved
+        const prevMove = start.moved
+        if (prevMove && !transaction.prevMoved.has(start) && prevMove.id.clock < (transaction.beforeState.get(prevMove.id.client) || 0)) {
+          // only override prevMoved if the prevMoved item is not new
+          // we need to know which item previously moved an item
+          transaction.prevMoved.set(start, prevMove)
+        }
+        start.moved = item
+      } else if (currMoved != null) {
+        /** @type {ContentMove} */ (currMoved.content).overrides.add(item)
       }
       start = start.right
     }

src/structs/Item.js

@@ -652,7 +652,6 @@ export class Item extends AbstractStruct {
     if (!this.deleted) {
       throw error.unexpectedCase()
     }
-    this.moved = null
     this.content.gc(store)
     if (parentGCd) {
       replaceStruct(store, this, new GC(this.id, this.length))

src/types/AbstractType.js

@@ -36,7 +36,7 @@ const maxSearchMarker = 80
  */
 export const useSearchMarker = (tr, yarray, index, f) => {
   const searchMarker = yarray._searchMarker
-  if (searchMarker === null || yarray._start === null || index < 5) {
+  if (searchMarker === null || yarray._start === null) { // @todo add condition `index < 5`
     return f(new ListIterator(yarray).forward(tr, index))
   }
   if (searchMarker.length === 0) {
@@ -47,7 +47,7 @@ export const useSearchMarker = (tr, yarray, index, f) => {
   const sm = searchMarker.reduce(
     (a, b, arrayIndex) => math.abs(index - a.index) < math.abs(index - b.index) ? a : b
   )
-  const newIsCheaper = math.abs(sm.index - index) > index
+  const newIsCheaper = math.abs(sm.index - index) > index // @todo use >= index
   const createFreshMarker = searchMarker.length < maxSearchMarker && (math.abs(sm.index - index) > 5 || newIsCheaper)
   const fsm = createFreshMarker ? (newIsCheaper ? new ListIterator(yarray) : sm.clone()) : sm
   const prevItem = /** @type {Item} */ (sm.nextItem)
@@ -60,7 +60,7 @@ export const useSearchMarker = (tr, yarray, index, f) => {
   } else {
     fsm.forward(tr, -diff)
   }
-  // @todo remove this tests
+  // @todo remove this test
   /*
   const otherTesting = new ListIterator(yarray)
   otherTesting.forward(tr, index)
@@ -77,10 +77,8 @@ export const useSearchMarker = (tr, yarray, index, f) => {
     }
     fsm.rel = 0
   }
-  if (fsm.rel > 0) {
+  fsm.index -= fsm.rel
-    fsm.index -= fsm.rel
+  fsm.rel = 0
-    fsm.rel = 0
-  }
   if (!createFreshMarker) {
     // reused old marker and we moved to a different position
     prevItem.marker = false

src/utils/ListIterator.js

@@ -11,11 +11,55 @@ import {
   ContentType,
   ContentDoc,
   Doc,
+  compareIDs,
   RelativePosition, ID, AbstractContent, ContentMove, Transaction, Item, AbstractType // eslint-disable-line
 } from '../internals.js'
 
 const lengthExceeded = error.create('Length exceeded!')
 
+/**
+ * We keep the moved-stack across several transactions. Local or remote changes can invalidate
+ * "moved coords" on the moved-stack.
+ *
+ * The reason for this is that if assoc < 0, then getMovedCoords will return the target.right item.
+ * While the computed item is on the stack, it is possible that a user inserts something between target
+ * and the item on the stack. Then we expect that the newly inserted item is supposed to be on the new
+ * computed item.
+ *
+ * @param {Transaction} tr
+ * @param {ListIterator} li
+ */
+const popMovedStack = (tr, li) => {
+  let { start, end, move } = li.movedStack.pop() || { start: null, end: null, move: null }
+  if (move) {
+    const moveContent = /** @type {ContentMove} */ (move.content)
+    if (
+      (
+        moveContent.start.assoc < 0 && (
+          (start === null && moveContent.start.item !== null) ||
+          (start !== null && !compareIDs(/** @type {Item} */ (start.left).lastId, moveContent.start.item))
+        )
+      ) || (
+        moveContent.end.assoc < 0 && (
+          (end === null && moveContent.end.item !== null) ||
+          (end !== null && !compareIDs(/** @type {Item} */ (end.left).lastId, moveContent.end.item))
+        )
+      )
+    ) {
+      const coords = getMovedCoords(moveContent, tr)
+      start = coords.start
+      end = coords.end
+      // @todo why are we not running into this?
+      console.log('found edge case 42') // @todo remove
+      debugger
+    }
+  }
+  li.currMove = move
+  li.currMoveStart = start
+  li.currMoveEnd = end
+  li.reachedEnd = false
+}
+
 /**
  * @todo rename to walker?
  * @todo check that inserting character one after another always reuses ListIterators
@@ -113,10 +157,13 @@ export class ListIterator {
    * @param {number} len
    */
   forward (tr, len) {
-    if (this.index + len > this.type._length) {
+    if (len === 0 && this.nextItem == null) {
+      return this
+    }
+    if (this.index + len > this.type._length || this.nextItem == null) {
       throw lengthExceeded
     }
-    let item = this.nextItem
+    let item = /** @type {Item} */ (this.nextItem)
     this.index += len
     // @todo this condition is not needed, better to remove it (can always be applied)
     if (this.rel) {
@@ -126,13 +173,9 @@ export class ListIterator {
     while ((!this.reachedEnd || this.currMove !== null) && (len > 0 || (len === 0 && item && (!item.countable || item.deleted || item === this.currMoveEnd || (this.reachedEnd && this.currMoveEnd === null) || item.moved !== this.currMove)))) {
       if (item === this.currMoveEnd || (this.currMoveEnd === null && this.reachedEnd && this.currMove)) {
         item = /** @type {Item} */ (this.currMove) // we iterate to the right after the current condition
-        const { start, end, move } = this.movedStack.pop() || { start: null, end: null, move: null }
+        popMovedStack(tr, this)
-        this.currMove = move
-        this.currMoveStart = start
-        this.currMoveEnd = end
-        this.reachedEnd = false
       } else if (item === null) {
-        break
+        error.unexpectedCase() // should never happen
       } else if (item.countable && !item.deleted && item.moved === this.currMove && len > 0) {
         len -= item.length
         if (len < 0) {
@@ -151,10 +194,13 @@ export class ListIterator {
         item = start
         continue
       }
+      if (this.reachedEnd) {
+        throw error.unexpectedCase
+      }
       if (item.right) {
         item = item.right
       } else {
-        this.reachedEnd = true
+        this.reachedEnd = true // @todo we need to ensure to iterate further if this.currMoveEnd === null
       }
     }
     this.index -= len
@@ -170,10 +216,7 @@ export class ListIterator {
     if (item !== null) {
       while (item === this.currMoveStart) {
         item = /** @type {Item} */ (this.currMove) // we iterate to the left after the current condition
-        const { start, end, move } = this.movedStack.pop() || { start: null, end: null, move: null }
+        popMovedStack(tr, this)
-        this.currMove = move
-        this.currMoveStart = start
-        this.currMoveEnd = end
       }
       this.nextItem = item
     }
@@ -228,10 +271,7 @@ export class ListIterator {
       }
       if (item === this.currMoveStart) {
         item = /** @type {Item} */ (this.currMove) // we iterate to the left after the current condition
-        const { start, end, move } = this.movedStack.pop() || { start: null, end: null, move: null }
+        popMovedStack(tr, this)
-        this.currMove = move
-        this.currMoveStart = start
-        this.currMoveEnd = end
       }
       item = item.left
     }
@@ -248,33 +288,45 @@ export class ListIterator {
    * @param {function(T, T): T} concat
    */
   _slice (tr, len, value, slice, concat) {
+    if (this.index + len > this.type._length) {
+      throw lengthExceeded
+    }
     this.index += len
+    /**
+     * We store nextItem in a variable because this version cannot be null.
+     */
+    let nextItem = /** @type {Item} */ (this.nextItem)
     while (len > 0 && !this.reachedEnd) {
-      while (this.nextItem && this.nextItem.countable && !this.reachedEnd && len > 0 && this.nextItem !== this.currMoveEnd) {
+      while (nextItem.countable && !this.reachedEnd && len > 0 && nextItem !== this.currMoveEnd) {
-        if (!this.nextItem.deleted && this.nextItem.moved === this.currMove) {
+        if (!nextItem.deleted && nextItem.moved === this.currMove) {
-          const item = this.nextItem
+          const slicedContent = slice(nextItem.content, this.rel, len)
-          const slicedContent = slice(item.content, this.rel, len)
           len -= slicedContent.length
           value = concat(value, slicedContent)
-          if (item.length !== slicedContent.length) {
+          if (this.rel + slicedContent.length === nextItem.length) {
-            if (this.rel + slicedContent.length === item.length) {
+            this.rel = 0
-              this.rel = 0
+          } else {
-            } else {
+            this.rel += slicedContent.length
-              this.rel += slicedContent.length
+            continue // do not iterate to item.right
-              continue // do not iterate to item.right
-            }
           }
         }
-        if (this.nextItem.right) {
+        if (nextItem.right) {
-          this.nextItem = this.nextItem.right
+          nextItem = nextItem.right
+          this.nextItem = nextItem
         } else {
           this.reachedEnd = true
         }
       }
-      if (this.nextItem && (!this.reachedEnd || this.currMove !== null) && len > 0) {
+      if ((!this.reachedEnd || this.currMove !== null) && len > 0) {
+        // always set nextItem before any method call
+        this.nextItem = nextItem
         this.forward(tr, 0)
+        if (this.nextItem == null) {
+          throw new Error('debug me') // @todo remove
+        }
+        nextItem = this.nextItem
       }
     }
+    this.nextItem = nextItem
     if (len < 0) {
       this.index -= len
     }
@@ -378,7 +430,7 @@ export class ListIterator {
     // @todo is there a better alrogirthm to update searchmarkers? We could simply remove the markers that are in the updated range.
     // Also note that searchmarkers are updated in insertContents as well.
     const sm = this.type._searchMarker
-    if (sm) sm.length = 0
+    if (sm) sm.length = 0 // @todo instead, iterate through sm and delete all marked properties on items
   }
 
   /**

src/utils/YEvent.js

@@ -215,7 +215,7 @@ export class YEvent {
                 continue // do not move to item.right
               }
             } else if (item.moved !== currMove) {
-              if (!currMoveIsNew && item.countable && item.moved && !this.adds(item) && this.adds(item.moved) && (this.transaction.prevMoved.get(item) || null) === currMove) {
+              if (!currMoveIsNew && item.countable && (!item.deleted || this.deletes(item)) && item.moved && !this.adds(item) && this.adds(item.moved) && (this.transaction.prevMoved.get(item) || null) === currMove) {
                 if (lastOp === null || lastOp.delete === undefined) {
                   packOp()
                   lastOp = { delete: 0 }

tests/y-array.tests.js

@@ -540,20 +540,6 @@ const getUniqueNumber = () => _uniqueNumber++
  * @todo to replace content to a separate data structure so we know that insert & returns work as expected!!!
  */
 const arrayTransactions = [
-  function move (user, gen) {
-    const yarray = user.getArray('array')
-    if (yarray.length === 0) {
-      return
-    }
-    const pos = prng.int32(gen, 0, yarray.length - 1)
-    const newPos = prng.int32(gen, 0, yarray.length)
-    const oldContent = yarray.toArray()
-    yarray.move(pos, newPos)
-    const [x] = oldContent.splice(pos, 1)
-    oldContent.splice(pos < newPos ? newPos - 1 : newPos, 0, x)
-    t.compareArrays(yarray.toArray(), oldContent) // we want to make sure that fastSearch markers insert at the correct position
-  },
-  // @todo remove this duplicate!!
   function move (user, gen) {
     const yarray = user.getArray('array')
     if (yarray.length === 0) {
@@ -671,6 +657,13 @@ export const testRepeatGeneratingYarrayTests6 = tc => {
   compareTestobjects(applyRandomTests(tc, arrayTransactions, 6, monitorArrayTestObject))
 }
 
+/**
+ * @param {t.TestCase} tc
+ */
+export const testRepeatGeneratingYarrayTests10 = tc => {
+  compareTestobjects(applyRandomTests(tc, arrayTransactions, 10, monitorArrayTestObject))
+}
+
 /**
  * @param {t.TestCase} tc
  */