diff --git a/src/structs/ContentMove.js b/src/structs/ContentMove.js index 0890c16a..d6f4b07e 100644 --- a/src/structs/ContentMove.js +++ b/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 - } - 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 + error.unexpectedCase() } + return { start: /** @type {Item} */ (start), end: /** @type {Item} */ (end) } } /** @@ -172,7 +145,8 @@ export class ContentMove { * @param {Item} item */ integrate (transaction, item) { - /** @type {AbstractType} */ (item.parent)._searchMarker = [] + const sm = /** @type {AbstractType} */ (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 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 (currMoved !== null) { - 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) + const currMoved = start.moved + 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 (currMoved !== null) { + 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) } start = start.right } diff --git a/src/structs/Item.js b/src/structs/Item.js index 44e9aeef..11e4f4f5 100644 --- a/src/structs/Item.js +++ b/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)) diff --git a/src/types/AbstractType.js b/src/types/AbstractType.js index 8fc8eca9..b2188261 100644 --- a/src/types/AbstractType.js +++ b/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.rel = 0 - } + fsm.index -= fsm.rel + fsm.rel = 0 if (!createFreshMarker) { // reused old marker and we moved to a different position prevItem.marker = false diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index 829d3993..0f297173 100644 --- a/src/utils/ListIterator.js +++ b/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 } - this.currMove = move - this.currMoveStart = start - this.currMoveEnd = end - this.reachedEnd = false + popMovedStack(tr, this) } 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 } - this.currMove = move - this.currMoveStart = start - this.currMoveEnd = end + popMovedStack(tr, this) } 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 } - this.currMove = move - this.currMoveStart = start - this.currMoveEnd = end + popMovedStack(tr, this) } 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) { - if (!this.nextItem.deleted && this.nextItem.moved === this.currMove) { - const item = this.nextItem - const slicedContent = slice(item.content, this.rel, len) + while (nextItem.countable && !this.reachedEnd && len > 0 && nextItem !== this.currMoveEnd) { + if (!nextItem.deleted && nextItem.moved === this.currMove) { + const slicedContent = slice(nextItem.content, this.rel, len) len -= slicedContent.length value = concat(value, slicedContent) - if (item.length !== slicedContent.length) { - if (this.rel + slicedContent.length === item.length) { - this.rel = 0 - } else { - this.rel += slicedContent.length - continue // do not iterate to item.right - } + if (this.rel + slicedContent.length === nextItem.length) { + this.rel = 0 + } else { + this.rel += slicedContent.length + continue // do not iterate to item.right } } - if (this.nextItem.right) { - this.nextItem = this.nextItem.right + if (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 } /** diff --git a/src/utils/YEvent.js b/src/utils/YEvent.js index e583fed3..cdbc18a2 100644 --- a/src/utils/YEvent.js +++ b/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 } diff --git a/tests/y-array.tests.js b/tests/y-array.tests.js index 1ffa1a86..e200aab9 100644 --- a/tests/y-array.tests.js +++ b/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 */