From 53a7b286b865c01502a27b29086058e38a641b22 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Mon, 8 Nov 2021 18:15:58 +0100 Subject: [PATCH 01/26] Move content and list iteration abstraction --- src/internals.js | 1 + src/structs/Item.js | 72 ++++++++++++++++++--- src/types/AbstractType.js | 127 ++++++++++++++++++++++++++++++++++++++ src/types/YArray.js | 6 +- src/utils/StructStore.js | 5 +- src/utils/Transaction.js | 9 ++- tests/encoding.tests.js | 5 +- 7 files changed, 207 insertions(+), 18 deletions(-) diff --git a/src/internals.js b/src/internals.js index bc386f0a..77f5a0ec 100644 --- a/src/internals.js +++ b/src/internals.js @@ -38,6 +38,7 @@ export * from './structs/ContentFormat.js' export * from './structs/ContentJSON.js' export * from './structs/ContentAny.js' export * from './structs/ContentString.js' +export * from './structs/ContentMove.js' export * from './structs/ContentType.js' export * from './structs/Item.js' export * from './structs/Skip.js' diff --git a/src/structs/Item.js b/src/structs/Item.js index 656f5e5b..5f0a3f44 100644 --- a/src/structs/Item.js +++ b/src/structs/Item.js @@ -21,6 +21,7 @@ import { createID, readContentFormat, readContentType, + readContentMove, addChangedTypeToTransaction, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, ContentType, ContentDeleted, StructStore, ID, AbstractType, Transaction // eslint-disable-line } from '../internals.js' @@ -281,11 +282,14 @@ export class Item extends AbstractStruct { */ this.parentSub = parentSub /** - * If this type's effect is reundone this type refers to the type that undid + * If this type is deleted: If this type's effect is reundone this type refers to the type-id that undid * this operation. - * @type {ID | null} + * + * If this item is not deleted: This property is reused by the moved prop. In this case this property refers to an Item. + * + * @type {ID | Item | null} */ - this.redone = null + this._ref = null /** * @type {AbstractContent} */ @@ -295,11 +299,57 @@ export class Item extends AbstractStruct { * bit2: countable * bit3: deleted * bit4: mark - mark node as fast-search-marker + * bit5: moved - whether this item has been moved. The moved item is then referred to on the "redone" prop. * @type {number} byte */ this.info = this.content.isCountable() ? binary.BIT2 : 0 } + /** + * If this type's effect is reundone this type refers to the type-id that undid + * this operation. + * + * @return {ID | null} + */ + get redone () { + return /** @type {ID | null} */ (this._ref) + } + + /** + * @param {ID | null} id + */ + set redone (id) { + this._ref = id + } + + /** + * If this item has been moved, the moved property will referr to the item that moved this content. + * + * @param {Item | null} item + */ + set movedBy (item) { + this._ref = item + if (item != null) { + this.info |= binary.BIT5 + } else if (this.moved) { + this.info ^= binary.BIT5 + } + } + + /** + * @return {Item | null} + */ + get movedBy () { + return this.moved ? /** @type {Item} */ (this._ref) : null + } + + /** + * @return {boolean} + */ + get moved () { + return (this.info & binary.BIT5) > 0 + } + /** * This is used to mark the item as an indexed fast-search marker * @@ -371,7 +421,7 @@ export class Item extends AbstractStruct { // We have all missing ids, now find the items if (this.origin) { - this.left = getItemCleanEnd(transaction, store, this.origin) + this.left = getItemCleanEnd(transaction, this.origin) this.origin = this.left.lastId } if (this.rightOrigin) { @@ -409,7 +459,7 @@ export class Item extends AbstractStruct { integrate (transaction, offset) { if (offset > 0) { this.id.clock += offset - this.left = getItemCleanEnd(transaction, transaction.doc.store, createID(this.id.client, this.id.clock - 1)) + this.left = getItemCleanEnd(transaction, createID(this.id.client, this.id.clock - 1)) this.origin = this.left.lastId this.content = this.content.splice(offset) this.length -= offset @@ -567,8 +617,8 @@ export class Item extends AbstractStruct { this.id.client === right.id.client && this.id.clock + this.length === right.id.clock && this.deleted === right.deleted && - this.redone === null && - right.redone === null && + this._ref === right._ref && + (!this.deleted || this.redone === null) && this.content.constructor === right.content.constructor && this.content.mergeWith(right.content) ) { @@ -613,7 +663,7 @@ export class Item extends AbstractStruct { this.markDeleted() addToDeleteSet(transaction.deleteSet, this.id.client, this.id.clock, this.length) addChangedTypeToTransaction(transaction, parent, this.parentSub) - this.content.delete(transaction) + this.content.delete(transaction, this) } } @@ -710,7 +760,8 @@ export const contentRefs = [ readContentType, // 7 readContentAny, // 8 readContentDoc, // 9 - () => { error.unexpectedCase() } // 10 - Skip is not ItemContent + () => { error.unexpectedCase() }, // 10 - Skip is not ItemContent + readContentMove // 11 ] /** @@ -777,8 +828,9 @@ export class AbstractContent { /** * @param {Transaction} transaction + * @param {Item} item */ - delete (transaction) { + delete (transaction, item) { throw error.methodUnimplemented() } diff --git a/src/types/AbstractType.js b/src/types/AbstractType.js index 43c6127d..9c468ce6 100644 --- a/src/types/AbstractType.js +++ b/src/types/AbstractType.js @@ -11,6 +11,8 @@ import { ContentAny, ContentBinary, getItemCleanStart, + ContentMove, + getMovedCoords, ContentDoc, YText, YArray, UpdateEncoderV1, UpdateEncoderV2, Doc, Snapshot, Transaction, EventHandler, YEvent, Item, // eslint-disable-line } from '../internals.js' @@ -395,6 +397,131 @@ export class AbstractType { toJSON () {} } +export class ListPosition { + /** + * @param {AbstractType} type + * @param {Transaction} tr + */ + constructor (type, tr) { + this.type = type + /** + * Current index-position + */ + this.index = 0 + /** + * Relative position to the current item (if item.content.length > 1) + */ + this.rel = 0 + /** + * This refers to the current right item, unless reachedEnd is true. Then it refers to the left item. + */ + this.item = type._start + this.reachedEnd = type._start === null + /** + * @type {Item | null} + */ + this.currMove = null + /** + * @type {Item | null} + */ + this.currMoveEnd = null + /** + * @type {Array<{ end: Item | null, move: Item }>} + */ + this.movedStack = [] + this.tr = tr + } + + /** + * @param {number} len + */ + forward (len) { + let item = this.item + this.index += len + if (this.rel) { + len += this.rel + this.rel = 0 + } + while (item && !this.reachedEnd && (len > 0 || (len === 0 && (!item.countable || item.deleted)))) { + if (item.countable && !item.deleted && item.movedBy === this.currMove) { + len -= item.length + if (len <= 0) { + this.rel = -len + break + } + } else if (item.content.constructor === ContentMove) { + if (this.currMove) { + this.movedStack.push({ end: this.currMoveEnd, move: this.currMove }) + } + const { start, end } = getMovedCoords(item.content, this.tr) + this.currMove = item + this.currMoveEnd = end + this.item = start + continue + } + if (item === this.currMoveEnd) { + this.item = this.currMove // we iterate to the right after the current condition + const { end, move } = this.movedStack.pop() || { end: null, move: null } + this.currMove = move + this.currMoveEnd = end + } + if (item.right) { + item = item.right + } else { + this.reachedEnd = true + } + } + this.index -= len + this.item = item + } + + /** + * @param {number} len + */ + slice (len) { + const result = [] + while (len > 0 && !this.reachedEnd) { + while (this.item && this.item.countable && !this.reachedEnd && len > 0) { + if (!this.item.deleted) { + const content = this.item.content.getContent() + const slicedContent = content.length <= len || this.rel > 0 ? content : content.slice(this.rel, len) + len -= slicedContent.length + result.push(...slicedContent) + if (content.length !== slicedContent.length) { + if (this.rel + slicedContent.length === content.length) { + this.rel = 0 + } else { + this.rel += slicedContent.length + continue // do not iterate to item.right + } + } + } + if (this.item.right) { + this.item = this.item.right + } else { + this.reachedEnd = true + } + } + if (this.item && !this.reachedEnd && len > 0) { + this.forward(0) + } + } + return result + } + + [Symbol.iterator] () { + return this + } + + next () { + const [value] = this.slice(1) + return { + done: value == null, + value: value + } + } +} + /** * @param {AbstractType} type * @param {number} start diff --git a/src/types/YArray.js b/src/types/YArray.js index 7c210fa6..c662097b 100644 --- a/src/types/YArray.js +++ b/src/types/YArray.js @@ -6,7 +6,6 @@ import { YEvent, AbstractType, typeListGet, - typeListToArray, typeListForEach, typeListCreateIterator, typeListInsertGenerics, @@ -15,6 +14,7 @@ import { YArrayRefID, callTypeObservers, transact, + ListPosition, ArraySearchMarker, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, Doc, Transaction, Item // eslint-disable-line } from '../internals.js' import { typeListSlice } from './AbstractType.js' @@ -188,7 +188,9 @@ export class YArray extends AbstractType { * @return {Array} */ toArray () { - return typeListToArray(this) + return transact(/** @type {Doc} */ (this.doc), tr => + new ListPosition(this, tr).slice(this.length) + ) } /** diff --git a/src/utils/StructStore.js b/src/utils/StructStore.js index 7a2e256c..079d3bc3 100644 --- a/src/utils/StructStore.js +++ b/src/utils/StructStore.js @@ -199,19 +199,18 @@ export const getItemCleanStart = (transaction, id) => { * Expects that id is actually in store. This function throws or is an infinite loop otherwise. * * @param {Transaction} transaction - * @param {StructStore} store * @param {ID} id * @return {Item} * * @private * @function */ -export const getItemCleanEnd = (transaction, store, id) => { +export const getItemCleanEnd = (transaction, id) => { /** * @type {Array} */ // @ts-ignore - const structs = store.clients.get(id.client) + const structs = transaction.doc.store.clients.get(id.client) const index = findIndexSS(structs, id.clock) const struct = structs[index] if (id.clock !== struct.id.clock + struct.length - 1 && struct.constructor !== GC) { diff --git a/src/utils/Transaction.js b/src/utils/Transaction.js index a9ab6afa..7fb507f1 100644 --- a/src/utils/Transaction.js +++ b/src/utils/Transaction.js @@ -377,9 +377,12 @@ const cleanupTransactions = (transactionCleanups, i) => { /** * Implements the functionality of `y.transact(()=>{..})` * + * @template T + * * @param {Doc} doc - * @param {function(Transaction):void} f + * @param {function(Transaction):T} f * @param {any} [origin=true] + * @return {T} * * @function */ @@ -395,8 +398,9 @@ export const transact = (doc, f, origin = null, local = true) => { } doc.emit('beforeTransaction', [doc._transaction, doc]) } + let res try { - f(doc._transaction) + res = f(doc._transaction) } finally { if (initialCall && transactionCleanups[0] === doc._transaction) { // The first transaction ended, now process observer calls. @@ -410,4 +414,5 @@ export const transact = (doc, f, origin = null, local = true) => { cleanupTransactions(transactionCleanups, 0) } } + return res } diff --git a/tests/encoding.tests.js b/tests/encoding.tests.js index 6a191009..73c396aa 100644 --- a/tests/encoding.tests.js +++ b/tests/encoding.tests.js @@ -12,6 +12,7 @@ import { readContentFormat, readContentAny, readContentDoc, + readContentMove, Doc, PermanentUserData, encodeStateAsUpdate, @@ -24,7 +25,8 @@ import * as Y from '../src/index.js' * @param {t.TestCase} tc */ export const testStructReferences = tc => { - t.assert(contentRefs.length === 11) + t.assert(contentRefs.length === 12) + // contentRefs[0] is reserved for GC t.assert(contentRefs[1] === readContentDeleted) t.assert(contentRefs[2] === readContentJSON) // TODO: deprecate content json? t.assert(contentRefs[3] === readContentBinary) @@ -35,6 +37,7 @@ export const testStructReferences = tc => { t.assert(contentRefs[8] === readContentAny) t.assert(contentRefs[9] === readContentDoc) // contentRefs[10] is reserved for Skip structs + t.assert(contentRefs[11] === readContentMove) } /** From 56ab251e7935352eb98a10522df392405afb1a83 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Mon, 8 Nov 2021 18:33:26 +0100 Subject: [PATCH 02/26] make moved a separate prop on item --- src/structs/ContentMove.js | 237 +++++++++++++++++++++++++++++++++++++ src/structs/Item.js | 65 ++-------- src/types/AbstractType.js | 2 +- 3 files changed, 250 insertions(+), 54 deletions(-) create mode 100644 src/structs/ContentMove.js diff --git a/src/structs/ContentMove.js b/src/structs/ContentMove.js new file mode 100644 index 00000000..882d4283 --- /dev/null +++ b/src/structs/ContentMove.js @@ -0,0 +1,237 @@ + +import * as error from 'lib0/error' +import * as decoding from 'lib0/decoding' +import { + AbstractType, ContentType, ID, RelativePosition, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, Transaction, Item, StructStore, getItem, getItemCleanStart, getItemCleanEnd // eslint-disable-line +} from '../internals.js' + +/** + * @param {ContentMove} moved + * @param {Transaction} tr + * @return {{ start: Item | null, end: Item | null }} $start (inclusive) is the beginning and $end (exclusive) is the end of the moved area + */ +export const getMovedCoords = (moved, tr) => { + let start // this (inclusive) is the beginning of the moved area + let end // this (exclusive) is the first item after start that is not part of the moved area + if (moved.start.item) { + if (moved.start.assoc < 0) { + start = getItemCleanEnd(tr, moved.start.item) + start = start.right + } else { + start = getItemCleanStart(tr, moved.start.item) + } + } else if (moved.start.tname != null) { + start = tr.doc.get(moved.start.tname)._start + } else if (moved.start.type) { + start = /** @type {ContentType} */ (getItem(tr.doc.store, moved.start.type).content).type._start + } else { + error.unexpectedCase() + } + if (moved.end.item) { + if (moved.end.assoc < 0) { + end = getItemCleanEnd(tr, moved.end.item) + end = end.right + } else { + end = getItemCleanStart(tr, moved.end.item) + } + } else { + end = null + } + return { start, end } +} + +/** + * @param {ContentMove} moved + * @param {Item} movedItem + * @param {Transaction} tr + * @param {function(Item):void} cb + */ +export const iterateMoved = (moved, movedItem, tr, cb) => { + 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 + } +} + +/** + * @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) => { + if (trackedMovedItems.has(movedItem)) { + return true + } + trackedMovedItems.add(movedItem) + 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 + } + } + start = start.right + } + return false +} + +/** + * @private + */ +export class ContentMove { + /** + * @param {RelativePosition} start + * @param {RelativePosition} end + * @param {number} priority if we want to move content that is already moved, we need to assign a higher priority to this move operation. + */ + constructor (start, end, priority) { + this.start = start + this.end = end + this.priority = priority + /** + * We store which Items+ContentMove we override. Once we delete + * this ContentMove, we need to re-integrate the overridden items. + * + * This representation can be improved if we ever run into memory issues because of too many overrides. + * Ideally, we should probably just re-iterate the document and re-integrate all moved items. + * This is fast enough and reduces memory footprint significantly. + * + * @type {Set} + */ + this.overrides = new Set() + } + + /** + * @return {number} + */ + getLength () { + return 1 + } + + /** + * @return {Array} + */ + getContent () { + return [null] + } + + /** + * @return {boolean} + */ + isCountable () { + return false + } + + /** + * @return {ContentMove} + */ + copy () { + return new ContentMove(this.start, this.end, this.priority) + } + + /** + * @param {number} offset + * @return {ContentMove} + */ + splice (offset) { + return this + } + + /** + * @param {ContentMove} right + * @return {boolean} + */ + mergeWith (right) { + return false + } + + /** + * @param {Transaction} transaction + * @param {Item} item + */ + integrate (transaction, item) { + /** @type {AbstractType} */ (item.parent)._searchMarker = [] + let { start, end } = getMovedCoords(this, transaction) + while (start !== end && start != null) { + if (!start.deleted) { + const currMoved = start.moved + if (currMoved === null || /** @type {ContentMove} */ (currMoved.content).priority < 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) + } + start.moved = item + } else { + /** @type {ContentMove} */ (currMoved.content).overrides.add(item) + } + } + start = start.right + } + } + + /** + * @param {Transaction} transaction + * @param {Item} item + */ + delete (transaction, item) { + let { start, end } = getMovedCoords(this, transaction) + while (start !== end && start != null) { + if (start.moved === item) { + start.moved = null + } + start = start.right + } + /** + * @param {Item} reIntegrateItem + */ + const reIntegrate = reIntegrateItem => { + const content = /** @type {ContentMove} */ (reIntegrateItem.content) + if (reIntegrateItem.deleted) { + // potentially we can integrate the items that reIntegrateItem overrides + content.overrides.forEach(reIntegrate) + } else { + content.integrate(transaction, reIntegrateItem) + } + } + this.overrides.forEach(reIntegrate) + } + + /** + * @param {StructStore} store + */ + gc (store) {} + + /** + * @param {UpdateEncoderV1 | UpdateEncoderV2} encoder + * @param {number} offset + */ + write (encoder, offset) { + encoder.writeAny(this.start) + encoder.writeAny(this.end) + } + + /** + * @return {number} + */ + getRef () { + return 11 + } +} + +/** + * @private + * + * @param {UpdateDecoderV1 | UpdateDecoderV2} decoder + * @return {ContentMove} + */ +export const readContentMove = decoder => new ContentMove(decoder.readAny(), decoder.readAny(), decoding.readVarUint(decoder.restDecoder)) diff --git a/src/structs/Item.js b/src/structs/Item.js index 5f0a3f44..6cf77e9d 100644 --- a/src/structs/Item.js +++ b/src/structs/Item.js @@ -282,14 +282,18 @@ export class Item extends AbstractStruct { */ this.parentSub = parentSub /** - * If this type is deleted: If this type's effect is reundone this type refers to the type-id that undid + * If this type's effect is reundone this type refers to the type-id that undid * this operation. * - * If this item is not deleted: This property is reused by the moved prop. In this case this property refers to an Item. - * - * @type {ID | Item | null} + * @type {ID | null} */ - this._ref = null + this.redone = null + /** + * This property is reused by the moved prop. In this case this property refers to an Item. + * + * @type {Item | null} + */ + this.moved = null /** * @type {AbstractContent} */ @@ -299,57 +303,11 @@ export class Item extends AbstractStruct { * bit2: countable * bit3: deleted * bit4: mark - mark node as fast-search-marker - * bit5: moved - whether this item has been moved. The moved item is then referred to on the "redone" prop. * @type {number} byte */ this.info = this.content.isCountable() ? binary.BIT2 : 0 } - /** - * If this type's effect is reundone this type refers to the type-id that undid - * this operation. - * - * @return {ID | null} - */ - get redone () { - return /** @type {ID | null} */ (this._ref) - } - - /** - * @param {ID | null} id - */ - set redone (id) { - this._ref = id - } - - /** - * If this item has been moved, the moved property will referr to the item that moved this content. - * - * @param {Item | null} item - */ - set movedBy (item) { - this._ref = item - if (item != null) { - this.info |= binary.BIT5 - } else if (this.moved) { - this.info ^= binary.BIT5 - } - } - - /** - * @return {Item | null} - */ - get movedBy () { - return this.moved ? /** @type {Item} */ (this._ref) : null - } - - /** - * @return {boolean} - */ - get moved () { - return (this.info & binary.BIT5) > 0 - } - /** * This is used to mark the item as an indexed fast-search marker * @@ -617,8 +575,9 @@ export class Item extends AbstractStruct { this.id.client === right.id.client && this.id.clock + this.length === right.id.clock && this.deleted === right.deleted && - this._ref === right._ref && - (!this.deleted || this.redone === null) && + this.redone === null && + right.redone === null && + this.moved === right.moved && this.content.constructor === right.content.constructor && this.content.mergeWith(right.content) ) { diff --git a/src/types/AbstractType.js b/src/types/AbstractType.js index 9c468ce6..def38b08 100644 --- a/src/types/AbstractType.js +++ b/src/types/AbstractType.js @@ -443,7 +443,7 @@ export class ListPosition { this.rel = 0 } while (item && !this.reachedEnd && (len > 0 || (len === 0 && (!item.countable || item.deleted)))) { - if (item.countable && !item.deleted && item.movedBy === this.currMove) { + if (item.countable && !item.deleted && item.moved === this.currMove) { len -= item.length if (len <= 0) { this.rel = -len From a723c32557ac70476acfb2f8b92ce529faced4bb Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Tue, 9 Nov 2021 20:20:04 +0100 Subject: [PATCH 03/26] use new ListPosition abstraction in Y.Array .slice and .get --- src/types/AbstractType.js | 10 +++++++--- src/types/YArray.js | 10 ++++++---- tests/y-array.tests.js | 2 ++ 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/types/AbstractType.js b/src/types/AbstractType.js index def38b08..de169271 100644 --- a/src/types/AbstractType.js +++ b/src/types/AbstractType.js @@ -445,8 +445,8 @@ export class ListPosition { while (item && !this.reachedEnd && (len > 0 || (len === 0 && (!item.countable || item.deleted)))) { if (item.countable && !item.deleted && item.moved === this.currMove) { len -= item.length - if (len <= 0) { - this.rel = -len + if (len < 0) { + this.rel = item.length + len break } } else if (item.content.constructor === ContentMove) { @@ -473,6 +473,10 @@ export class ListPosition { } this.index -= len this.item = item + if (len > 0) { + throw lengthExceeded + } + return this } /** @@ -484,7 +488,7 @@ export class ListPosition { while (this.item && this.item.countable && !this.reachedEnd && len > 0) { if (!this.item.deleted) { const content = this.item.content.getContent() - const slicedContent = content.length <= len || this.rel > 0 ? content : content.slice(this.rel, len) + const slicedContent = content.length <= len && this.rel === 0 ? content : content.slice(this.rel, this.rel + len) len -= slicedContent.length result.push(...slicedContent) if (content.length !== slicedContent.length) { diff --git a/src/types/YArray.js b/src/types/YArray.js index c662097b..52a931ff 100644 --- a/src/types/YArray.js +++ b/src/types/YArray.js @@ -5,7 +5,6 @@ import { YEvent, AbstractType, - typeListGet, typeListForEach, typeListCreateIterator, typeListInsertGenerics, @@ -17,7 +16,6 @@ import { ListPosition, ArraySearchMarker, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, Doc, Transaction, Item // eslint-disable-line } from '../internals.js' -import { typeListSlice } from './AbstractType.js' /** * Event that describes the changes on a YArray @@ -179,7 +177,9 @@ export class YArray extends AbstractType { * @return {T} */ get (index) { - return typeListGet(this, index) + return transact(/** @type {Doc} */ (this.doc), tr => + new ListPosition(this, tr).forward(index).slice(1)[0] + ) } /** @@ -201,7 +201,9 @@ export class YArray extends AbstractType { * @return {Array} */ slice (start = 0, end = this.length) { - return typeListSlice(this, start, end) + return transact(/** @type {Doc} */ (this.doc), tr => + new ListPosition(this, tr).forward(start).slice(end < 0 ? this.length + end - start : end - start) + ) } /** diff --git a/tests/y-array.tests.js b/tests/y-array.tests.js index 8cdaac5b..3a77e6d1 100644 --- a/tests/y-array.tests.js +++ b/tests/y-array.tests.js @@ -456,6 +456,8 @@ const getUniqueNumber = () => _uniqueNumber++ /** * @type {Array} + * + * @todo to replace content to a separate data structure so we know that insert & returns work as expected!!! */ const arrayTransactions = [ function insert (user, gen) { From b9ccbb2dc766a36c95ca01c53ed8436fb4ed49ca Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Fri, 19 Nov 2021 16:07:03 +0100 Subject: [PATCH 04/26] created new abstraction for search marker --- src/internals.js | 1 + src/structs/Item.js | 19 +- src/types/AbstractType.js | 460 ++++---------------------------------- src/types/YArray.js | 44 ++-- src/types/YText.js | 19 +- src/types/YXmlFragment.js | 26 ++- src/utils/ListIterator.js | 435 +++++++++++++++++++++++++++++++++++ tests/y-array.tests.js | 2 +- 8 files changed, 544 insertions(+), 462 deletions(-) create mode 100644 src/utils/ListIterator.js diff --git a/src/internals.js b/src/internals.js index 77f5a0ec..e84e7ad2 100644 --- a/src/internals.js +++ b/src/internals.js @@ -8,6 +8,7 @@ export * from './utils/encoding.js' export * from './utils/EventHandler.js' export * from './utils/ID.js' export * from './utils/isParentOf.js' +export * from './utils/ListIterator.js' export * from './utils/logging.js' export * from './utils/PermanentUserData.js' export * from './utils/RelativePosition.js' diff --git a/src/structs/Item.js b/src/structs/Item.js index 6cf77e9d..2fdff261 100644 --- a/src/structs/Item.js +++ b/src/structs/Item.js @@ -581,18 +581,17 @@ export class Item extends AbstractStruct { this.content.constructor === right.content.constructor && this.content.mergeWith(right.content) ) { - const searchMarker = /** @type {AbstractType} */ (this.parent)._searchMarker - if (searchMarker) { - searchMarker.forEach(marker => { - if (marker.p === right) { - // right is going to be "forgotten" so we need to update the marker - marker.p = this - // adjust marker index - if (!this.deleted && this.countable) { - marker.index -= this.length + if (right.marker) { + // Right will be "forgotten", so we delete all + // search markers that reference right. + const searchMarker = /** @type {AbstractType} */ (this.parent)._searchMarker + if (searchMarker) { + for (let i = searchMarker.length - 1; i >= 0; i--) { + if (searchMarker[i].nextItem === right) { + searchMarker.splice(i, 1) } } - }) + } } if (right.keep) { this.keep = true diff --git a/src/types/AbstractType.js b/src/types/AbstractType.js index de169271..6e059721 100644 --- a/src/types/AbstractType.js +++ b/src/types/AbstractType.js @@ -10,9 +10,7 @@ import { createID, ContentAny, ContentBinary, - getItemCleanStart, - ContentMove, - getMovedCoords, + ListIterator, ContentDoc, YText, YArray, UpdateEncoderV1, UpdateEncoderV2, Doc, Snapshot, Transaction, EventHandler, YEvent, Item, // eslint-disable-line } from '../internals.js' @@ -23,67 +21,6 @@ import * as math from 'lib0/math' const maxSearchMarker = 80 -/** - * A unique timestamp that identifies each marker. - * - * Time is relative,.. this is more like an ever-increasing clock. - * - * @type {number} - */ -let globalSearchMarkerTimestamp = 0 - -export class ArraySearchMarker { - /** - * @param {Item} p - * @param {number} index - */ - constructor (p, index) { - p.marker = true - this.p = p - this.index = index - this.timestamp = globalSearchMarkerTimestamp++ - } -} - -/** - * @param {ArraySearchMarker} marker - */ -const refreshMarkerTimestamp = marker => { marker.timestamp = globalSearchMarkerTimestamp++ } - -/** - * This is rather complex so this function is the only thing that should overwrite a marker - * - * @param {ArraySearchMarker} marker - * @param {Item} p - * @param {number} index - */ -const overwriteMarker = (marker, p, index) => { - marker.p.marker = false - marker.p = p - p.marker = true - marker.index = index - marker.timestamp = globalSearchMarkerTimestamp++ -} - -/** - * @param {Array} searchMarker - * @param {Item} p - * @param {number} index - */ -const markPosition = (searchMarker, p, index) => { - if (searchMarker.length >= maxSearchMarker) { - // override oldest marker (we don't want to create more objects) - const marker = searchMarker.reduce((a, b) => a.timestamp < b.timestamp ? a : b) - overwriteMarker(marker, p, index) - return marker - } else { - // create new marker - const pm = new ArraySearchMarker(p, index) - searchMarker.push(pm) - return pm - } -} - /** * Search marker help us to find positions in the associative array faster. * @@ -91,82 +28,47 @@ const markPosition = (searchMarker, p, index) => { * * A maximum of `maxSearchMarker` objects are created. * - * This function always returns a refreshed marker (updated timestamp) - * + * @template T + * @param {Transaction} tr * @param {AbstractType} yarray * @param {number} index + * @param {function(ListIterator):T} f */ -export const findMarker = (yarray, index) => { - if (yarray._start === null || index === 0 || yarray._searchMarker === null) { - return null +export const useSearchMarker = (tr, yarray, index, f) => { + const searchMarker = yarray._searchMarker + if (searchMarker === null || yarray._start === null || index < 30) { + return f(new ListIterator(yarray).forward(tr, index)) } - const marker = yarray._searchMarker.length === 0 ? null : yarray._searchMarker.reduce((a, b) => math.abs(index - a.index) < math.abs(index - b.index) ? a : b) - let p = yarray._start - let pindex = 0 - if (marker !== null) { - p = marker.p - pindex = marker.index - refreshMarkerTimestamp(marker) // we used it, we might need to use it again + if (searchMarker.length === 0) { + const sm = new ListIterator(yarray).forward(tr, index) + searchMarker.push(sm) + if (sm.nextItem) sm.nextItem.marker = true + return f(sm) } - // iterate to right if possible - while (p.right !== null && pindex < index) { - if (!p.deleted && p.countable) { - if (index < pindex + p.length) { - break - } - pindex += p.length - } - p = p.right + const sm = searchMarker.reduce( + (a, b, arrayIndex) => math.abs(index - a.index) < math.abs(index - b.index) ? a : b + ) + const createFreshMarker = searchMarker.length < maxSearchMarker && math.abs(sm.index - index) > 30 + const fsm = createFreshMarker ? sm.clone() : sm + const prevItem = /** @type {Item} */ (sm.nextItem) + if (createFreshMarker) { + searchMarker.push(fsm) } - // iterate to left if necessary (might be that pindex > index) - while (p.left !== null && pindex > index) { - p = p.left - if (!p.deleted && p.countable) { - pindex -= p.length + const result = f(fsm) + if (!createFreshMarker && fsm.nextItem !== prevItem) { + // reused old marker and we moved to a different position + prevItem.marker = false + } + const fsmItem = fsm.nextItem + if (fsmItem) { + if (fsmItem.marker) { + // already marked, forget current iterator + searchMarker.splice(searchMarker.findIndex(m => m === fsm), 1) + } else { + fsmItem.marker = true } } - // we want to make sure that p can't be merged with left, because that would screw up everything - // in that cas just return what we have (it is most likely the best marker anyway) - // iterate to left until p can't be merged with left - while (p.left !== null && p.left.id.client === p.id.client && p.left.id.clock + p.left.length === p.id.clock) { - p = p.left - if (!p.deleted && p.countable) { - pindex -= p.length - } - } - - // @todo remove! - // assure position - // { - // let start = yarray._start - // let pos = 0 - // while (start !== p) { - // if (!start.deleted && start.countable) { - // pos += start.length - // } - // start = /** @type {Item} */ (start.right) - // } - // if (pos !== pindex) { - // debugger - // throw new Error('Gotcha position fail!') - // } - // } - // if (marker) { - // if (window.lengthes == null) { - // window.lengthes = [] - // window.getLengthes = () => window.lengthes.sort((a, b) => a - b) - // } - // window.lengthes.push(marker.index - pindex) - // console.log('distance', marker.index - pindex, 'len', p && p.parent.length) - // } - if (marker !== null && math.abs(marker.index - pindex) < /** @type {YText|YArray} */ (p.parent).length / maxSearchMarker) { - // adjust existing marker - overwriteMarker(marker, p, pindex) - return marker - } else { - // create new marker - return markPosition(yarray._searchMarker, p, pindex) - } + return result } /** @@ -174,39 +76,22 @@ export const findMarker = (yarray, index) => { * * This should be called before doing a deletion! * - * @param {Array} searchMarker + * @param {Transaction} tr + * @param {Array} searchMarker * @param {number} index * @param {number} len If insertion, len is positive. If deletion, len is negative. */ -export const updateMarkerChanges = (searchMarker, index, len) => { +export const updateMarkerChanges = (tr, searchMarker, index, len) => { for (let i = searchMarker.length - 1; i >= 0; i--) { - const m = searchMarker[i] - if (len > 0) { - /** - * @type {Item|null} - */ - let p = m.p - p.marker = false - // Ideally we just want to do a simple position comparison, but this will only work if - // search markers don't point to deleted items for formats. - // Iterate marker to prev undeleted countable position so we know what to do when updating a position - while (p && (p.deleted || !p.countable)) { - p = p.left - if (p && !p.deleted && p.countable) { - // adjust position. the loop should break now - m.index -= p.length - } - } - if (p === null || p.marker === true) { - // remove search marker if updated position is null or if position is already marked - searchMarker.splice(i, 1) - continue - } - m.p = p - p.marker = true + const marker = searchMarker[i] + if (len > 0 && index === marker.index) { + // inserting at a marked position deletes the marked position because we can't do a simple transformation + // (we don't know whether to insert directly before or directly after the position) + searchMarker.splice(i, 1) + continue } - if (index < m.index || (len > 0 && index === m.index)) { // a simple index <= m.index check would actually suffice - m.index = math.max(index, m.index + len) + if (index < marker.index) { // a simple index <= m.index check would actually suffice + marker.index = math.max(index, marker.index + len) } } } @@ -284,7 +169,7 @@ export class AbstractType { */ this._dEH = createEventHandler() /** - * @type {null | Array} + * @type {null | Array} */ this._searchMarker = null } @@ -397,135 +282,6 @@ export class AbstractType { toJSON () {} } -export class ListPosition { - /** - * @param {AbstractType} type - * @param {Transaction} tr - */ - constructor (type, tr) { - this.type = type - /** - * Current index-position - */ - this.index = 0 - /** - * Relative position to the current item (if item.content.length > 1) - */ - this.rel = 0 - /** - * This refers to the current right item, unless reachedEnd is true. Then it refers to the left item. - */ - this.item = type._start - this.reachedEnd = type._start === null - /** - * @type {Item | null} - */ - this.currMove = null - /** - * @type {Item | null} - */ - this.currMoveEnd = null - /** - * @type {Array<{ end: Item | null, move: Item }>} - */ - this.movedStack = [] - this.tr = tr - } - - /** - * @param {number} len - */ - forward (len) { - let item = this.item - this.index += len - if (this.rel) { - len += this.rel - this.rel = 0 - } - while (item && !this.reachedEnd && (len > 0 || (len === 0 && (!item.countable || item.deleted)))) { - if (item.countable && !item.deleted && item.moved === this.currMove) { - len -= item.length - if (len < 0) { - this.rel = item.length + len - break - } - } else if (item.content.constructor === ContentMove) { - if (this.currMove) { - this.movedStack.push({ end: this.currMoveEnd, move: this.currMove }) - } - const { start, end } = getMovedCoords(item.content, this.tr) - this.currMove = item - this.currMoveEnd = end - this.item = start - continue - } - if (item === this.currMoveEnd) { - this.item = this.currMove // we iterate to the right after the current condition - const { end, move } = this.movedStack.pop() || { end: null, move: null } - this.currMove = move - this.currMoveEnd = end - } - if (item.right) { - item = item.right - } else { - this.reachedEnd = true - } - } - this.index -= len - this.item = item - if (len > 0) { - throw lengthExceeded - } - return this - } - - /** - * @param {number} len - */ - slice (len) { - const result = [] - while (len > 0 && !this.reachedEnd) { - while (this.item && this.item.countable && !this.reachedEnd && len > 0) { - if (!this.item.deleted) { - const content = this.item.content.getContent() - const slicedContent = content.length <= len && this.rel === 0 ? content : content.slice(this.rel, this.rel + len) - len -= slicedContent.length - result.push(...slicedContent) - if (content.length !== slicedContent.length) { - if (this.rel + slicedContent.length === content.length) { - this.rel = 0 - } else { - this.rel += slicedContent.length - continue // do not iterate to item.right - } - } - } - if (this.item.right) { - this.item = this.item.right - } else { - this.reachedEnd = true - } - } - if (this.item && !this.reachedEnd && len > 0) { - this.forward(0) - } - } - return result - } - - [Symbol.iterator] () { - return this - } - - next () { - const [value] = this.slice(1) - return { - done: value == null, - value: value - } - } -} - /** * @param {AbstractType} type * @param {number} start @@ -725,31 +481,6 @@ export const typeListForEachSnapshot = (type, f, snapshot) => { } } -/** - * @param {AbstractType} type - * @param {number} index - * @return {any} - * - * @private - * @function - */ -export const typeListGet = (type, index) => { - const marker = findMarker(type, index) - let n = type._start - if (marker !== null) { - n = marker.p - index -= marker.index - } - for (; n !== null; n = n.right) { - if (!n.deleted && n.countable) { - if (index < n.length) { - return n.content.getContent()[index] - } - index -= n.length - } - } -} - /** * @param {Transaction} transaction * @param {AbstractType} parent @@ -814,105 +545,6 @@ export const typeListInsertGenericsAfter = (transaction, parent, referenceItem, packJsonContent() } -const lengthExceeded = error.create('Length exceeded!') - -/** - * @param {Transaction} transaction - * @param {AbstractType} parent - * @param {number} index - * @param {Array|Array|number|null|string|Uint8Array>} content - * - * @private - * @function - */ -export const typeListInsertGenerics = (transaction, parent, index, content) => { - if (index > parent._length) { - throw lengthExceeded - } - if (index === 0) { - if (parent._searchMarker) { - updateMarkerChanges(parent._searchMarker, index, content.length) - } - return typeListInsertGenericsAfter(transaction, parent, null, content) - } - const startIndex = index - const marker = findMarker(parent, index) - let n = parent._start - if (marker !== null) { - n = marker.p - index -= marker.index - // we need to iterate one to the left so that the algorithm works - if (index === 0) { - // @todo refactor this as it actually doesn't consider formats - n = n.prev // important! get the left undeleted item so that we can actually decrease index - index += (n && n.countable && !n.deleted) ? n.length : 0 - } - } - for (; n !== null; n = n.right) { - if (!n.deleted && n.countable) { - if (index <= n.length) { - if (index < n.length) { - // insert in-between - getItemCleanStart(transaction, createID(n.id.client, n.id.clock + index)) - } - break - } - index -= n.length - } - } - if (parent._searchMarker) { - updateMarkerChanges(parent._searchMarker, startIndex, content.length) - } - return typeListInsertGenericsAfter(transaction, parent, n, content) -} - -/** - * @param {Transaction} transaction - * @param {AbstractType} parent - * @param {number} index - * @param {number} length - * - * @private - * @function - */ -export const typeListDelete = (transaction, parent, index, length) => { - if (length === 0) { return } - const startIndex = index - const startLength = length - const marker = findMarker(parent, index) - let n = parent._start - if (marker !== null) { - n = marker.p - index -= marker.index - } - // compute the first item to be deleted - for (; n !== null && index > 0; n = n.right) { - if (!n.deleted && n.countable) { - if (index < n.length) { - getItemCleanStart(transaction, createID(n.id.client, n.id.clock + index)) - } - index -= n.length - } - } - // delete all items until done - while (length > 0 && n !== null) { - if (!n.deleted) { - if (length < n.length) { - getItemCleanStart(transaction, createID(n.id.client, n.id.clock + length)) - } - n.delete(transaction) - length -= n.length - } - n = n.right - } - if (length > 0) { - throw lengthExceeded - } - if (parent._searchMarker) { - updateMarkerChanges(parent._searchMarker, startIndex, -startLength + length /* in case we remove the above exception */) - } -} - /** * @param {Transaction} transaction * @param {AbstractType} parent diff --git a/src/types/YArray.js b/src/types/YArray.js index 52a931ff..cd1cad71 100644 --- a/src/types/YArray.js +++ b/src/types/YArray.js @@ -2,19 +2,15 @@ * @module YArray */ +import { useSearchMarker } from 'tests/testHelper.js' import { YEvent, AbstractType, - typeListForEach, - typeListCreateIterator, - typeListInsertGenerics, - typeListDelete, - typeListMap, YArrayRefID, callTypeObservers, transact, - ListPosition, - ArraySearchMarker, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, Doc, Transaction, Item // eslint-disable-line + ListIterator, + UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, Doc, Transaction, Item // eslint-disable-line } from '../internals.js' /** @@ -47,7 +43,7 @@ export class YArray extends AbstractType { */ this._prelimContent = [] /** - * @type {Array} + * @type {Array} */ this._searchMarker = [] } @@ -129,7 +125,9 @@ export class YArray extends AbstractType { insert (index, content) { if (this.doc !== null) { transact(this.doc, transaction => { - typeListInsertGenerics(transaction, this, index, content) + useSearchMarker(transaction, this, index, walker => + walker.insertArrayValue(transaction, content) + ) }) } else { /** @type {Array} */ (this._prelimContent).splice(index, 0, ...content) @@ -163,7 +161,9 @@ export class YArray extends AbstractType { delete (index, length = 1) { if (this.doc !== null) { transact(this.doc, transaction => { - typeListDelete(transaction, this, index, length) + useSearchMarker(transaction, this, index, walker => + walker.delete(transaction, length) + ) }) } else { /** @type {Array} */ (this._prelimContent).splice(index, length) @@ -177,8 +177,10 @@ export class YArray extends AbstractType { * @return {T} */ get (index) { - return transact(/** @type {Doc} */ (this.doc), tr => - new ListPosition(this, tr).forward(index).slice(1)[0] + return transact(/** @type {Doc} */ (this.doc), transaction => + useSearchMarker(transaction, this, index, walker => + walker.slice(transaction, 1)[0] + ) ) } @@ -189,7 +191,7 @@ export class YArray extends AbstractType { */ toArray () { return transact(/** @type {Doc} */ (this.doc), tr => - new ListPosition(this, tr).slice(this.length) + new ListIterator(this).slice(tr, this.length) ) } @@ -201,8 +203,10 @@ export class YArray extends AbstractType { * @return {Array} */ slice (start = 0, end = this.length) { - return transact(/** @type {Doc} */ (this.doc), tr => - new ListPosition(this, tr).forward(start).slice(end < 0 ? this.length + end - start : end - start) + return transact(/** @type {Doc} */ (this.doc), transaction => + useSearchMarker(transaction, this, start, walker => + walker.slice(transaction, end < 0 ? this.length + end - start : end - start) + ) ) } @@ -225,7 +229,9 @@ export class YArray extends AbstractType { * callback function */ map (f) { - return typeListMap(this, /** @type {any} */ (f)) + return transact(/** @type {Doc} */ (this.doc), tr => + new ListIterator(this).map(tr, f) + ) } /** @@ -234,14 +240,16 @@ export class YArray extends AbstractType { * @param {function(T,number,YArray):void} f A function to execute on every element of this YArray. */ forEach (f) { - typeListForEach(this, f) + return transact(/** @type {Doc} */ (this.doc), tr => + new ListIterator(this).forEach(tr, f) + ) } /** * @return {IterableIterator} */ [Symbol.iterator] () { - return typeListCreateIterator(this) + return this.toArray().values() } /** diff --git a/src/types/YText.js b/src/types/YText.js index 328795e0..46beac05 100644 --- a/src/types/YText.js +++ b/src/types/YText.js @@ -20,14 +20,14 @@ import { splitSnapshotAffectedStructs, iterateDeletedStructs, iterateStructs, - findMarker, typeMapDelete, typeMapSet, typeMapGet, typeMapGetAll, updateMarkerChanges, ContentType, - ArraySearchMarker, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, ID, Doc, Item, Snapshot, Transaction // eslint-disable-line + useSearchMarker, + ListIterator, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, ID, Doc, Item, Snapshot, Transaction // eslint-disable-line } from '../internals.js' import * as object from 'lib0/object' @@ -125,10 +125,11 @@ const findNextPosition = (transaction, pos, count) => { */ const findPosition = (transaction, parent, index) => { const currentAttributes = new Map() - const marker = findMarker(parent, index) - if (marker) { - const pos = new ItemTextListPosition(marker.p.left, marker.p, marker.index, currentAttributes) - return findNextPosition(transaction, pos, index - marker.index) + if (parent._searchMarker) { + return useSearchMarker(transaction, parent, index, listIter => { + const pos = new ItemTextListPosition(listIter.left, listIter.right, listIter.index, currentAttributes) + return findNextPosition(transaction, pos, index - listIter.index) + }) } else { const pos = new ItemTextListPosition(null, parent._start, 0, currentAttributes) return findNextPosition(transaction, pos, index) @@ -264,7 +265,7 @@ const insertText = (transaction, parent, currPos, text, attributes) => { const content = text.constructor === String ? new ContentString(/** @type {string} */ (text)) : (text instanceof AbstractType ? new ContentType(text) : new ContentEmbed(text)) let { left, right, index } = currPos if (parent._searchMarker) { - updateMarkerChanges(parent._searchMarker, currPos.index, content.getLength()) + updateMarkerChanges(transaction, parent._searchMarker, currPos.index, content.getLength()) } right = new Item(createID(ownClientId, getState(doc.store, ownClientId)), left, left && left.lastId, right, right && right.id, parent, null, content) right.integrate(transaction, 0) @@ -469,7 +470,7 @@ const deleteText = (transaction, currPos, length) => { } const parent = /** @type {AbstractType} */ (/** @type {Item} */ (currPos.left || currPos.right).parent) if (parent._searchMarker) { - updateMarkerChanges(parent._searchMarker, currPos.index, -startLength + length) + updateMarkerChanges(transaction, parent._searchMarker, currPos.index, -startLength + length) } return currPos } @@ -764,7 +765,7 @@ export class YText extends AbstractType { */ this._pending = string !== undefined ? [() => this.insert(0, string)] : [] /** - * @type {Array} + * @type {Array} */ this._searchMarker = [] } diff --git a/src/types/YXmlFragment.js b/src/types/YXmlFragment.js index c18b9ca0..9f957475 100644 --- a/src/types/YXmlFragment.js +++ b/src/types/YXmlFragment.js @@ -8,15 +8,13 @@ import { AbstractType, typeListMap, typeListForEach, - typeListInsertGenerics, typeListInsertGenericsAfter, - typeListDelete, typeListToArray, YXmlFragmentRefID, callTypeObservers, transact, - typeListGet, typeListSlice, + useSearchMarker, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, Doc, ContentType, Transaction, Item, YXmlText, YXmlHook, Snapshot // eslint-disable-line } from '../internals.js' @@ -304,9 +302,11 @@ export class YXmlFragment extends AbstractType { */ insert (index, content) { if (this.doc !== null) { - transact(this.doc, transaction => { - typeListInsertGenerics(transaction, this, index, content) - }) + return transact(/** @type {Doc} */ (this.doc), transaction => + useSearchMarker(transaction, this, index, walker => + walker.insertArrayValue(transaction, content) + ) + ) } else { // @ts-ignore _prelimContent is defined because this is not yet integrated this._prelimContent.splice(index, 0, ...content) @@ -347,9 +347,11 @@ export class YXmlFragment extends AbstractType { */ delete (index, length = 1) { if (this.doc !== null) { - transact(this.doc, transaction => { - typeListDelete(transaction, this, index, length) - }) + transact(/** @type {Doc} */ (this.doc), transaction => + useSearchMarker(transaction, this, index, walker => + walker.delete(transaction, length) + ) + ) } else { // @ts-ignore _prelimContent is defined because this is not yet integrated this._prelimContent.splice(index, length) @@ -390,7 +392,11 @@ export class YXmlFragment extends AbstractType { * @return {YXmlElement|YXmlText} */ get (index) { - return typeListGet(this, index) + return transact(/** @type {Doc} */ (this.doc), transaction => + useSearchMarker(transaction, this, index, walker => + walker.slice(transaction, 1)[0] + ) + ) } /** diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js new file mode 100644 index 00000000..fe6da753 --- /dev/null +++ b/src/utils/ListIterator.js @@ -0,0 +1,435 @@ +import * as error from 'lib0/error' + +import { + getItemCleanStart, + createID, + getMovedCoords, + updateMarkerChanges, + getState, + ContentAny, + ContentBinary, + ContentType, + ContentDoc, + Doc, + ID, AbstractContent, ContentMove, Transaction, Item, AbstractType // eslint-disable-line +} from '../internals.js' + +const lengthExceeded = error.create('Length exceeded!') + +/** + * @todo rename to walker? + */ +export class ListIterator { + /** + * @param {AbstractType} type + */ + constructor (type) { + this.type = type + /** + * Current index-position + */ + this.index = 0 + /** + * Relative position to the current item (if item.content.length > 1) + */ + this.rel = 0 + /** + * This refers to the current right item, unless reachedEnd is true. Then it refers to the left item. + * + * @public + * @type {Item | null} + */ + this.nextItem = type._start + this.reachedEnd = type._start === null + /** + * @type {Item | null} + */ + this.currMove = null + /** + * @type {Item | null} + */ + this.currMoveStart = null + /** + * @type {Item | null} + */ + this.currMoveEnd = null + /** + * @type {Array<{ start: Item | null, end: Item | null, move: Item }>} + */ + this.movedStack = [] + } + + clone () { + const iter = new ListIterator(this.type) + iter.index = this.index + iter.rel = this.rel + iter.nextItem = this.nextItem + iter.reachedEnd = this.reachedEnd + iter.currMove = this.currMove + iter.currMoveStart = this.currMoveStart + iter.currMoveEnd = this.currMoveEnd + iter.movedStack = this.movedStack.slice() + return iter + } + + /** + * @type {Item | null} + */ + get left () { + if (this.reachedEnd) { + return this.nextItem + } else { + return this.nextItem && this.nextItem.left + } + } + + /** + * @type {Item | null} + */ + get right () { + if (this.reachedEnd) { + return null + } else { + return this.nextItem + } + } + + /** + * @param {Transaction} tr + * @param {number} index + */ + moveTo (tr, index) { + const diff = index - this.index + if (diff > 0) { + this.forward(tr, diff) + } else if (diff < 0) { + this.backward(tr, -diff) + } + } + + /** + * @param {Transaction} tr + * @param {number} len + */ + forward (tr, len) { + if (this.index + len > this.type._length) { + throw lengthExceeded + } + let item = this.nextItem + this.index += len + if (this.rel) { + len += this.rel + this.rel = 0 + } + while (item && !this.reachedEnd && (len > 0 || (len === 0 && (!item.countable || item.deleted)))) { + if (item.countable && !item.deleted && item.moved === this.currMove) { + len -= item.length + if (len < 0) { + this.rel = item.length + len + break + } + } else if (item.content.constructor === ContentMove && item.moved === this.currMove) { + if (this.currMove) { + this.movedStack.push({ start: this.currMoveStart, end: this.currMoveEnd, move: this.currMove }) + } + const { start, end } = getMovedCoords(item.content, tr) + this.currMove = item + this.currMoveStart = start + this.currMoveEnd = end + item = start + continue + } + if (item === this.currMoveEnd) { + 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 + } + if (item.right) { + item = item.right + } else { + this.reachedEnd = true + } + } + this.index -= len + this.nextItem = item + return this + } + + /** + * @param {Transaction} tr + * @param {number} len + */ + backward (tr, len) { + if (this.index - len < 0) { + throw lengthExceeded + } + let item = this.nextItem && this.nextItem.left + this.index -= len + if (this.rel) { + len -= this.rel + this.rel = 0 + } + while (item && len > 0) { + if (item.countable && !item.deleted && item.moved === this.currMove) { + len -= item.length + if (len < 0) { + this.rel = item.length + len + break + } + } else if (item.content.constructor === ContentMove && item.moved === this.currMove) { + if (this.currMove) { + this.movedStack.push({ start: this.currMoveStart, end: this.currMoveEnd, move: this.currMove }) + } + const { start, end } = getMovedCoords(item.content, tr) + this.currMove = item + this.currMoveStart = start + this.currMoveEnd = end + item = start + continue + } + 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 + } + item = item.left + } + this.index -= len + this.nextItem = item + return this + } + + /** + * @template {{length: number}} T + * @param {Transaction} tr + * @param {number} len + * @param {T} value the initial content + * @param {function(AbstractContent, number, number):T} slice + * @param {function(T, T): T} concat + */ + _slice (tr, len, value, slice, concat) { + while (len > 0 && !this.reachedEnd) { + while (this.nextItem && this.nextItem.countable && !this.reachedEnd && len > 0) { + if (!this.nextItem.deleted) { + const item = this.nextItem + const slicedContent = slice(this.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.nextItem.right) { + this.nextItem = this.nextItem.right + } else { + this.reachedEnd = true + } + } + if (this.nextItem && !this.reachedEnd && len > 0) { + this.forward(tr, 0) + } + } + return value + } + + /** + * @param {Transaction} tr + * @param {number} len + */ + delete (tr, len) { + const startLength = len + const sm = this.type._searchMarker + let item = this.nextItem + while (len > 0 && !this.reachedEnd) { + while (item && item.countable && !this.reachedEnd && len > 0) { + if (!item.deleted) { + if (this.rel > 0) { + item = getItemCleanStart(tr, createID(item.id.client, item.id.clock + this.rel)) + this.rel = 0 + } + if (len < item.length) { + getItemCleanStart(tr, createID(item.id.client, item.id.clock + len)) + } + len -= item.length + item.delete(tr) + } + if (item.right) { + item = item.right + } else { + this.reachedEnd = true + } + } + if (item && !this.reachedEnd && len > 0) { + this.nextItem = item + this.forward(tr, 0) + } + } + this.nextItem = item + if (sm) { + updateMarkerChanges(tr, sm, this.index, -startLength + len) + } + } + + /** + * @param {Transaction} tr + * @param {Array|Array|boolean|number|null|string|Uint8Array>} content + */ + insertArrayValue (tr, content) { + /** + * @type {Item | null} + */ + let item = this.nextItem + if (this.rel > 0) { + /** + * @type {ID} + */ + const itemid = /** @type {Item} */ (item).id + item = getItemCleanStart(tr, createID(itemid.client, itemid.clock + this.rel)) + this.rel = 0 + } + const parent = this.type + const store = tr.doc.store + const ownClientId = tr.doc.clientID + /** + * @type {Item | null} + */ + const right = this.right + + /** + * @type {Item | null} + */ + let left = this.left + /** + * @type {Array|number|null>} + */ + let jsonContent = [] + const packJsonContent = () => { + if (jsonContent.length > 0) { + left = new Item(createID(ownClientId, getState(store, ownClientId)), left, left && left.lastId, right, right && right.id, parent, null, new ContentAny(jsonContent)) + left.integrate(tr, 0) + jsonContent = [] + } + } + content.forEach(c => { + if (c === null) { + jsonContent.push(c) + } else { + switch (c.constructor) { + case Number: + case Object: + case Boolean: + case Array: + case String: + jsonContent.push(c) + break + default: + packJsonContent() + switch (c.constructor) { + case Uint8Array: + case ArrayBuffer: + left = new Item(createID(ownClientId, getState(store, ownClientId)), left, left && left.lastId, right, right && right.id, parent, null, new ContentBinary(new Uint8Array(/** @type {Uint8Array} */ (c)))) + left.integrate(tr, 0) + break + case Doc: + left = new Item(createID(ownClientId, getState(store, ownClientId)), left, left && left.lastId, right, right && right.id, parent, null, new ContentDoc(/** @type {Doc} */ (c))) + left.integrate(tr, 0) + break + default: + if (c instanceof AbstractType) { + left = new Item(createID(ownClientId, getState(store, ownClientId)), left, left && left.lastId, right, right && right.id, parent, null, new ContentType(c)) + left.integrate(tr, 0) + } else { + throw new Error('Unexpected content type in insert operation') + } + } + } + } + }) + packJsonContent() + if (right === null && left !== null) { + item = left + this.reachedEnd = true + } else { + item = right + } + this.nextItem = item + } + + /** + * @param {Transaction} tr + * @param {number} len + */ + slice (tr, len) { + return this._slice(tr, len, [], sliceArrayContent, concatArrayContent) + } + + /** + * @param {Transaction} tr + * @param {function(any, number, any):void} f + */ + forEach (tr, f) { + for (const val of this.values(tr)) { + f(val, this.index, this.type) + } + } + + /** + * @template T + * @param {Transaction} tr + * @param {function(any, number, any):T} f + * @return {Array} + */ + map (tr, f) { + const arr = new Array(this.type._length - this.index) + let i = 0 + for (const val of this.values(tr)) { + arr[i++] = f(val, this.index, this.type) + } + return arr + } + + /** + * @param {Transaction} tr + */ + values (tr) { + return { + [Symbol.iterator] () { + return this + }, + next: () => { + const [value] = this.slice(tr, 1) + return { + done: value == null, + value: value + } + } + } + } +} + +/** + * @param {AbstractContent} itemcontent + * @param {number} start + * @param {number} len + */ +const sliceArrayContent = (itemcontent, start, len) => { + const content = itemcontent.getContent() + return content.length <= len && start === 0 ? content : content.slice(start, start + len) +} +/** + * @param {Array} content + * @param {Array} added + */ +const concatArrayContent = (content, added) => { + content.push(added) + return content +} diff --git a/tests/y-array.tests.js b/tests/y-array.tests.js index 3a77e6d1..b19750ce 100644 --- a/tests/y-array.tests.js +++ b/tests/y-array.tests.js @@ -522,7 +522,7 @@ const arrayTransactions = [ * @param {t.TestCase} tc */ export const testRepeatGeneratingYarrayTests6 = tc => { - applyRandomTests(tc, arrayTransactions, 6) + applyRandomTests(tc, arrayTransactions, 10) } /** From a77221ffd2defe61eaf4a19a804fefc31d661acb Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Fri, 19 Nov 2021 16:36:58 +0100 Subject: [PATCH 05/26] fix toJSON value --- src/types/YArray.js | 2 +- src/utils/ListIterator.js | 2 +- tests/doc.tests.js | 1 + tests/undo-redo.tests.js | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/types/YArray.js b/src/types/YArray.js index cd1cad71..0fc6f70f 100644 --- a/src/types/YArray.js +++ b/src/types/YArray.js @@ -2,7 +2,6 @@ * @module YArray */ -import { useSearchMarker } from 'tests/testHelper.js' import { YEvent, AbstractType, @@ -10,6 +9,7 @@ import { callTypeObservers, transact, ListIterator, + useSearchMarker, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, Doc, Transaction, Item // eslint-disable-line } from '../internals.js' diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index fe6da753..cfa03692 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -430,6 +430,6 @@ const sliceArrayContent = (itemcontent, start, len) => { * @param {Array} added */ const concatArrayContent = (content, added) => { - content.push(added) + content.push(...added) return content } diff --git a/tests/doc.tests.js b/tests/doc.tests.js index 994ecaeb..c167019e 100644 --- a/tests/doc.tests.js +++ b/tests/doc.tests.js @@ -40,6 +40,7 @@ export const testToJSON = tc => { const arr = doc.getArray('array') arr.push(['test1']) + t.compare(arr.toJSON(), ['test1']) const map = doc.getMap('map') map.set('k1', 'v1') diff --git a/tests/undo-redo.tests.js b/tests/undo-redo.tests.js index 991225ca..1bf25c82 100644 --- a/tests/undo-redo.tests.js +++ b/tests/undo-redo.tests.js @@ -1,4 +1,4 @@ -import { init, compare, applyRandomTests, Doc } from './testHelper.js' // eslint-disable-line +import { init, compare, applyRandomTests, Doc, UndoManager } from './testHelper.js' // eslint-disable-line import * as Y from '../src/index.js' import * as t from 'lib0/testing' From 8b82c573c47afacaf036555c7d788c6ac058079b Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Fri, 19 Nov 2021 17:14:24 +0100 Subject: [PATCH 06/26] fix basic inserd & delete bug --- src/types/YText.js | 3 ++- src/utils/ListIterator.js | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/types/YText.js b/src/types/YText.js index 46beac05..76b66de6 100644 --- a/src/types/YText.js +++ b/src/types/YText.js @@ -127,8 +127,9 @@ const findPosition = (transaction, parent, index) => { const currentAttributes = new Map() if (parent._searchMarker) { return useSearchMarker(transaction, parent, index, listIter => { + // @todo this should simply split if .rel > 0 const pos = new ItemTextListPosition(listIter.left, listIter.right, listIter.index, currentAttributes) - return findNextPosition(transaction, pos, index - listIter.index) + return findNextPosition(transaction, pos, index - listIter.index + listIter.rel) }) } else { const pos = new ItemTextListPosition(null, parent._start, 0, currentAttributes) diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index cfa03692..7e3463a5 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -126,6 +126,7 @@ export class ListIterator { len -= item.length if (len < 0) { this.rel = item.length + len + len = 0 break } } else if (item.content.constructor === ContentMove && item.moved === this.currMove) { @@ -176,6 +177,7 @@ export class ListIterator { len -= item.length if (len < 0) { this.rel = item.length + len + len = 0 break } } else if (item.content.constructor === ContentMove && item.moved === this.currMove) { From a057bf1cf0dae3ac06db4db913548748ff3ee900 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Fri, 19 Nov 2021 17:27:23 +0100 Subject: [PATCH 07/26] fix disconnect issue --- src/utils/ListIterator.js | 10 ++++------ tests/y-text.tests.js | 1 + 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index 7e3463a5..6c1c3dac 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -289,13 +289,12 @@ export class ListIterator { /** * @type {Item | null} */ - let item = this.nextItem if (this.rel > 0) { /** * @type {ID} */ - const itemid = /** @type {Item} */ (item).id - item = getItemCleanStart(tr, createID(itemid.client, itemid.clock + this.rel)) + const itemid = /** @type {Item} */ (this.nextItem).id + this.nextItem = getItemCleanStart(tr, createID(itemid.client, itemid.clock + this.rel)) this.rel = 0 } const parent = this.type @@ -358,12 +357,11 @@ export class ListIterator { }) packJsonContent() if (right === null && left !== null) { - item = left + this.nextItem = left this.reachedEnd = true } else { - item = right + this.nextItem = right } - this.nextItem = item } /** diff --git a/tests/y-text.tests.js b/tests/y-text.tests.js index 2a555237..f4ede339 100644 --- a/tests/y-text.tests.js +++ b/tests/y-text.tests.js @@ -327,6 +327,7 @@ export const testFormattingDeltaUnnecessaryAttributeChange = tc => { * @param {t.TestCase} tc */ export const testInsertAndDeleteAtRandomPositions = tc => { + // @todo optimize to run at least as fast as previous marker approach const N = 100000 const { text0 } = init(tc, { users: 1 }) const gen = tc.prng From fc38f3b8482d2bbe2c1793c19121f7c524a64303 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Fri, 19 Nov 2021 17:34:14 +0100 Subject: [PATCH 08/26] formatting bug --- tests/y-text.tests.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/y-text.tests.js b/tests/y-text.tests.js index f4ede339..f066782c 100644 --- a/tests/y-text.tests.js +++ b/tests/y-text.tests.js @@ -553,8 +553,6 @@ export const testSearchMarkerBug1 = tc => { } /** - * Reported in https://github.com/yjs/yjs/pull/32 - * * @param {t.TestCase} tc */ export const testFormattingBug = async tc => { @@ -564,7 +562,6 @@ export const testFormattingBug = async tc => { text1.insert(0, '\n\n\n') text1.format(0, 3, { url: 'http://example.com' }) ydoc1.getText().format(1, 1, { url: 'http://docs.yjs.dev' }) - ydoc2.getText().format(1, 1, { url: 'http://docs.yjs.dev' }) Y.applyUpdate(ydoc2, Y.encodeStateAsUpdate(ydoc1)) const text2 = ydoc2.getText() const expectedResult = [ From 6df152c4ec101815b65d58c69dea502108d3add6 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Fri, 19 Nov 2021 18:54:27 +0100 Subject: [PATCH 09/26] proper iteration through arrays (for mappings, toJSON, ..) --- src/utils/ListIterator.js | 15 ++++++++++++++- tests/y-array.tests.js | 3 ++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index 6c1c3dac..b38f743e 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -18,6 +18,7 @@ const lengthExceeded = error.create('Length exceeded!') /** * @todo rename to walker? + * @todo check that inserting character one after another always reuses ListIterators */ export class ListIterator { /** @@ -214,6 +215,7 @@ export class ListIterator { * @param {function(T, T): T} concat */ _slice (tr, len, value, slice, concat) { + this.index += len while (len > 0 && !this.reachedEnd) { while (this.nextItem && this.nextItem.countable && !this.reachedEnd && len > 0) { if (!this.nextItem.deleted) { @@ -240,6 +242,9 @@ export class ListIterator { this.forward(tr, 0) } } + if (len < 0) { + this.index -= len + } return value } @@ -297,6 +302,7 @@ export class ListIterator { this.nextItem = getItemCleanStart(tr, createID(itemid.client, itemid.clock + this.rel)) this.rel = 0 } + const sm = this.type._searchMarker const parent = this.type const store = tr.doc.store const ownClientId = tr.doc.clientID @@ -362,6 +368,10 @@ export class ListIterator { } else { this.nextItem = right } + if (sm) { + updateMarkerChanges(tr, sm, this.index, content.length) + } + this.index += content.length } /** @@ -406,9 +416,12 @@ export class ListIterator { return this }, next: () => { + if (this.reachedEnd) { + return { done: true } + } const [value] = this.slice(tr, 1) return { - done: value == null, + done: false, value: value } } diff --git a/tests/y-array.tests.js b/tests/y-array.tests.js index b19750ce..eaf085e3 100644 --- a/tests/y-array.tests.js +++ b/tests/y-array.tests.js @@ -1,4 +1,4 @@ -import { init, compare, applyRandomTests, Doc } from './testHelper.js' // eslint-disable-line +import { init, compare, applyRandomTests, Doc, AbstractType } from './testHelper.js' // eslint-disable-line import * as Y from '../src/index.js' import * as t from 'lib0/testing' @@ -473,6 +473,7 @@ const arrayTransactions = [ yarray.insert(pos, content) oldContent.splice(pos, 0, ...content) t.compareArrays(yarray.toArray(), oldContent) // we want to make sure that fastSearch markers insert at the correct position + t.compare(yarray.toJSON(), yarray.toArray().map(x => x instanceof AbstractType ? x.toJSON() : x)) }, function insertTypeArray (user, gen) { const yarray = user.getArray('array') From 4a8ebc31f7a31e9771a51e5c92d2452551de9894 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Sun, 21 Nov 2021 00:44:37 +0100 Subject: [PATCH 10/26] fix listiterator.map returning undefined as the last element --- src/utils/ListIterator.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index b38f743e..a38375cf 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -416,7 +416,7 @@ export class ListIterator { return this }, next: () => { - if (this.reachedEnd) { + if (this.reachedEnd || this.index === this.type._length) { return { done: true } } const [value] = this.slice(tr, 1) From 40c3be1732eb5b69d340bb34f732a72812d2f622 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Sun, 21 Nov 2021 14:00:02 +0100 Subject: [PATCH 11/26] fix backwards edge case --- src/types/AbstractType.js | 37 +++++++++++++++++++++++++++---------- src/utils/ListIterator.js | 7 ++++--- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/src/types/AbstractType.js b/src/types/AbstractType.js index 6e059721..91972316 100644 --- a/src/types/AbstractType.js +++ b/src/types/AbstractType.js @@ -54,7 +54,21 @@ export const useSearchMarker = (tr, yarray, index, f) => { if (createFreshMarker) { searchMarker.push(fsm) } + const diff = fsm.index - index + if (diff > 0) { + fsm.backward(tr, diff) + } else { + fsm.forward(tr, -diff) + } const result = f(fsm) + if (fsm.reachedEnd) { + fsm.reachedEnd = false + const nextItem = /** @type {Item} */ (fsm.nextItem) + if (nextItem.countable && !nextItem.deleted) { + fsm.index -= nextItem.length + } + fsm.rel = 0 + } if (!createFreshMarker && fsm.nextItem !== prevItem) { // reused old marker and we moved to a different position prevItem.marker = false @@ -76,22 +90,25 @@ export const useSearchMarker = (tr, yarray, index, f) => { * * This should be called before doing a deletion! * - * @param {Transaction} tr * @param {Array} searchMarker * @param {number} index * @param {number} len If insertion, len is positive. If deletion, len is negative. + * @param {ListIterator} origSearchMarker Do not update this searchmarker because it is the one we used to manipulate. */ -export const updateMarkerChanges = (tr, searchMarker, index, len) => { +export const updateMarkerChanges = (searchMarker, index, len, origSearchMarker) => { for (let i = searchMarker.length - 1; i >= 0; i--) { const marker = searchMarker[i] - if (len > 0 && index === marker.index) { - // inserting at a marked position deletes the marked position because we can't do a simple transformation - // (we don't know whether to insert directly before or directly after the position) - searchMarker.splice(i, 1) - continue - } - if (index < marker.index) { // a simple index <= m.index check would actually suffice - marker.index = math.max(index, marker.index + len) + if (marker !== origSearchMarker) { + if (len > 0 && index === marker.index) { + // inserting at a marked position deletes the marked position because we can't do a simple transformation + // (we don't know whether to insert directly before or directly after the position) + searchMarker.splice(i, 1) + if (marker.nextItem) marker.nextItem.marker = false + continue + } + if (index < marker.index) { // a simple index <= m.index check would actually suffice + marker.index = math.max(index, marker.index + len) + } } } } diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index a38375cf..08d28e15 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -202,7 +202,7 @@ export class ListIterator { item = item.left } this.index -= len - this.nextItem = item + this.nextItem = item && item.right return this } @@ -278,11 +278,12 @@ export class ListIterator { if (item && !this.reachedEnd && len > 0) { this.nextItem = item this.forward(tr, 0) + item = this.nextItem } } this.nextItem = item if (sm) { - updateMarkerChanges(tr, sm, this.index, -startLength + len) + updateMarkerChanges(sm, this.index, -startLength + len, this) } } @@ -369,7 +370,7 @@ export class ListIterator { this.nextItem = right } if (sm) { - updateMarkerChanges(tr, sm, this.index, content.length) + updateMarkerChanges(sm, this.index, content.length, this) } this.index += content.length } From 2a33507c00fbc85dd24bb960651d0e89c719abec Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Sun, 21 Nov 2021 19:53:53 +0100 Subject: [PATCH 12/26] fixed pos.rel cases --- src/types/AbstractType.js | 10 +++++++++- src/types/YText.js | 27 +++++++++++++++++++++++---- src/utils/ListIterator.js | 14 ++++++++++---- tests/testHelper.js | 23 +++++++++++++++++++++++ 4 files changed, 65 insertions(+), 9 deletions(-) diff --git a/src/types/AbstractType.js b/src/types/AbstractType.js index 91972316..dcacbb3a 100644 --- a/src/types/AbstractType.js +++ b/src/types/AbstractType.js @@ -55,11 +55,19 @@ export const useSearchMarker = (tr, yarray, index, f) => { searchMarker.push(fsm) } const diff = fsm.index - index + // @todo create fresh marker if diff > index if (diff > 0) { fsm.backward(tr, diff) } else { fsm.forward(tr, -diff) } + // @todo remove this tests + const otherTesting = new ListIterator(yarray) + otherTesting.forward(tr, index) + if (otherTesting.nextItem !== fsm.nextItem || otherTesting.index !== fsm.index || otherTesting.reachedEnd !== fsm.reachedEnd) { + throw new Error('udtirane') + } + const result = f(fsm) if (fsm.reachedEnd) { fsm.reachedEnd = false @@ -93,7 +101,7 @@ export const useSearchMarker = (tr, yarray, index, f) => { * @param {Array} searchMarker * @param {number} index * @param {number} len If insertion, len is positive. If deletion, len is negative. - * @param {ListIterator} origSearchMarker Do not update this searchmarker because it is the one we used to manipulate. + * @param {ListIterator|null} origSearchMarker Do not update this searchmarker because it is the one we used to manipulate. @todo !=null for improved perf in ytext */ export const updateMarkerChanges = (searchMarker, index, len, origSearchMarker) => { for (let i = searchMarker.length - 1; i >= 0; i--) { diff --git a/src/types/YText.js b/src/types/YText.js index 76b66de6..e327a640 100644 --- a/src/types/YText.js +++ b/src/types/YText.js @@ -27,6 +27,7 @@ import { updateMarkerChanges, ContentType, useSearchMarker, + findIndexCleanStart, ListIterator, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, ID, Doc, Item, Snapshot, Transaction // eslint-disable-line } from '../internals.js' @@ -127,9 +128,27 @@ const findPosition = (transaction, parent, index) => { const currentAttributes = new Map() if (parent._searchMarker) { return useSearchMarker(transaction, parent, index, listIter => { + let left, right + if (listIter.rel > 0) { + // must exist because rel > 0 + const nextItem = /** @type {Item} */ (listIter.nextItem) + if (listIter.rel === nextItem.length) { + left = nextItem + right = left.right + } else { + const structs = /** @type {Array} */ (transaction.doc.store.clients.get(nextItem.id.client)) + const after = /** @type {Item} */ (structs[findIndexCleanStart(transaction, structs, nextItem.id.clock + listIter.rel)]) + listIter.nextItem = after + listIter.rel = 0 + left = listIter.left + right = listIter.right + } + } else { + left = listIter.left + right = listIter.right + } // @todo this should simply split if .rel > 0 - const pos = new ItemTextListPosition(listIter.left, listIter.right, listIter.index, currentAttributes) - return findNextPosition(transaction, pos, index - listIter.index + listIter.rel) + return new ItemTextListPosition(left, right, index, currentAttributes) }) } else { const pos = new ItemTextListPosition(null, parent._start, 0, currentAttributes) @@ -266,7 +285,7 @@ const insertText = (transaction, parent, currPos, text, attributes) => { const content = text.constructor === String ? new ContentString(/** @type {string} */ (text)) : (text instanceof AbstractType ? new ContentType(text) : new ContentEmbed(text)) let { left, right, index } = currPos if (parent._searchMarker) { - updateMarkerChanges(transaction, parent._searchMarker, currPos.index, content.getLength()) + updateMarkerChanges(parent._searchMarker, currPos.index, content.getLength(), null) } right = new Item(createID(ownClientId, getState(doc.store, ownClientId)), left, left && left.lastId, right, right && right.id, parent, null, content) right.integrate(transaction, 0) @@ -471,7 +490,7 @@ const deleteText = (transaction, currPos, length) => { } const parent = /** @type {AbstractType} */ (/** @type {Item} */ (currPos.left || currPos.right).parent) if (parent._searchMarker) { - updateMarkerChanges(transaction, parent._searchMarker, currPos.index, -startLength + length) + updateMarkerChanges(parent._searchMarker, currPos.index, -startLength + length, null) } return currPos } diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index 08d28e15..d3bdf562 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -162,13 +162,18 @@ export class ListIterator { /** * @param {Transaction} tr * @param {number} len + * @return {ListIterator} */ backward (tr, len) { if (this.index - len < 0) { throw lengthExceeded } - let item = this.nextItem && this.nextItem.left this.index -= len + if (this.rel > len) { + this.rel -= len + return this + } + let item = this.nextItem && this.nextItem.left if (this.rel) { len -= this.rel this.rel = 0 @@ -177,8 +182,10 @@ export class ListIterator { if (item.countable && !item.deleted && item.moved === this.currMove) { len -= item.length if (len < 0) { - this.rel = item.length + len + this.rel = -len len = 0 + } + if (len === 0) { break } } else if (item.content.constructor === ContentMove && item.moved === this.currMove) { @@ -201,8 +208,7 @@ export class ListIterator { } item = item.left } - this.index -= len - this.nextItem = item && item.right + this.nextItem = item return this } diff --git a/tests/testHelper.js b/tests/testHelper.js index 4df1d11e..542abe39 100644 --- a/tests/testHelper.js +++ b/tests/testHelper.js @@ -373,6 +373,29 @@ export const compare = users => { t.compare(Y.encodeStateVector(users[i]), Y.encodeStateVector(users[i + 1])) compareDS(Y.createDeleteSetFromStructStore(users[i].store), Y.createDeleteSetFromStructStore(users[i + 1].store)) compareStructStores(users[i].store, users[i + 1].store) + // test list-iterator + { + const user = users[0] + user.transact(tr => { + const type = user.getArray('array') + Y.useSearchMarker(tr, type, type.length, walker => { + for (let i = type.length; i >= 0; i--) { + const otherWalker = new Y.ListIterator(type) + otherWalker.forward(tr, walker.index) + otherWalker.forward(tr, 0) + walker.forward(tr, 0) + t.assert(walker.index === i) + t.assert(walker.left === otherWalker.left) + t.assert(walker.right === otherWalker.right) + t.assert(walker.nextItem === otherWalker.nextItem) + t.assert(walker.reachedEnd === otherWalker.reachedEnd) + if (i > 0) { + walker.backward(tr, 1) + } + } + }) + }) + } } users.map(u => u.destroy()) } From d314c3e1a68dea79de1b3c1c22aa2537b05af63b Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Mon, 22 Nov 2021 21:36:29 +0100 Subject: [PATCH 13/26] fixed ListIterator.reachedEnd edge case --- src/utils/ListIterator.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index d3bdf562..42aa6237 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -169,7 +169,12 @@ export class ListIterator { throw lengthExceeded } this.index -= len - if (this.rel > len) { + if (this.reachedEnd) { + const nextItem = /** @type {Item} */ (this.nextItem) + this.rel = nextItem.countable && !nextItem.deleted ? nextItem.length : 0 + this.reachedEnd = false + } + if (this.rel >= len) { this.rel -= len return this } From fc5e36158fe67e10d5d3224144ba1fca2dcf2dc8 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Mon, 6 Dec 2021 13:23:01 +0100 Subject: [PATCH 14/26] made simple one-time move work --- src/structs/ContentMove.js | 20 ++++- src/types/AbstractType.js | 13 ++-- src/types/YArray.js | 27 +++++++ src/utils/ListIterator.js | 110 +++++++++++++++++----------- src/utils/YEvent.js | 146 ++++++++++++++++++++++++------------- tests/testHelper.js | 4 + tests/y-array.tests.js | 40 +++++++++- 7 files changed, 258 insertions(+), 102 deletions(-) diff --git a/src/structs/ContentMove.js b/src/structs/ContentMove.js index 882d4283..4ffc8c0f 100644 --- a/src/structs/ContentMove.js +++ b/src/structs/ContentMove.js @@ -1,6 +1,7 @@ import * as error from 'lib0/error' import * as decoding from 'lib0/decoding' +import * as encoding from 'lib0/encoding' import { AbstractType, ContentType, ID, RelativePosition, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, Transaction, Item, StructStore, getItem, getItemCleanStart, getItemCleanEnd // eslint-disable-line } from '../internals.js' @@ -8,7 +9,7 @@ import { /** * @param {ContentMove} moved * @param {Transaction} tr - * @return {{ start: Item | null, end: Item | null }} $start (inclusive) is the beginning and $end (exclusive) is the end of the moved area + * @return {{ start: Item, end: Item | null }} $start (inclusive) is the beginning and $end (exclusive) is the end of the moved area */ export const getMovedCoords = (moved, tr) => { let start // this (inclusive) is the beginning of the moved area @@ -37,16 +38,21 @@ export const getMovedCoords = (moved, tr) => { } else { end = null } - return { start, end } + 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) { @@ -74,6 +80,9 @@ export const findMoveLoop = (moved, movedItem, trackedMovedItems, tr) => { return true } trackedMovedItems.add(movedItem) + /** + * @type {{ start: Item | null, end: Item | null }} + */ let { start, end } = getMovedCoords(moved, tr) while (start !== end && start != null) { if (start.deleted && start.moved === movedItem && start.content.constructor === ContentMove) { @@ -162,6 +171,9 @@ export class ContentMove { */ integrate (transaction, item) { /** @type {AbstractType} */ (item.parent)._searchMarker = [] + /** + * @type {{ start: Item | null, end: Item | null }} + */ let { start, end } = getMovedCoords(this, transaction) while (start !== end && start != null) { if (!start.deleted) { @@ -184,6 +196,9 @@ export class ContentMove { * @param {Item} item */ delete (transaction, item) { + /** + * @type {{ start: Item | null, end: Item | null }} + */ let { start, end } = getMovedCoords(this, transaction) while (start !== end && start != null) { if (start.moved === item) { @@ -218,6 +233,7 @@ export class ContentMove { write (encoder, offset) { encoder.writeAny(this.start) encoder.writeAny(this.end) + encoding.writeVarUint(encoder.restEncoder, this.priority) } /** diff --git a/src/types/AbstractType.js b/src/types/AbstractType.js index dcacbb3a..8d3beb8b 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 < 30) { + if (searchMarker === null || yarray._start === null || index < 5) { return f(new ListIterator(yarray).forward(tr, index)) } if (searchMarker.length === 0) { @@ -48,26 +48,27 @@ 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 createFreshMarker = searchMarker.length < maxSearchMarker && math.abs(sm.index - index) > 30 - const fsm = createFreshMarker ? sm.clone() : sm + const newIsCheaper = math.abs(sm.index - index) > 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) if (createFreshMarker) { searchMarker.push(fsm) } const diff = fsm.index - index - // @todo create fresh marker if diff > index if (diff > 0) { fsm.backward(tr, diff) } else { fsm.forward(tr, -diff) } // @todo remove this tests + /* const otherTesting = new ListIterator(yarray) otherTesting.forward(tr, index) if (otherTesting.nextItem !== fsm.nextItem || otherTesting.index !== fsm.index || otherTesting.reachedEnd !== fsm.reachedEnd) { throw new Error('udtirane') } - + */ const result = f(fsm) if (fsm.reachedEnd) { fsm.reachedEnd = false @@ -77,7 +78,7 @@ export const useSearchMarker = (tr, yarray, index, f) => { } fsm.rel = 0 } - if (!createFreshMarker && fsm.nextItem !== prevItem) { + if (!createFreshMarker) { // reused old marker and we moved to a different position prevItem.marker = false } diff --git a/src/types/YArray.js b/src/types/YArray.js index 0fc6f70f..a40dd92c 100644 --- a/src/types/YArray.js +++ b/src/types/YArray.js @@ -10,6 +10,7 @@ import { transact, ListIterator, useSearchMarker, + createRelativePositionFromTypeIndex, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, Doc, Transaction, Item // eslint-disable-line } from '../internals.js' @@ -134,6 +135,32 @@ export class YArray extends AbstractType { } } + /** + * @param {number} start Inclusive move-start + * @param {number} end Inclusive move-end + * @param {number} target + * @param {number} assocStart >=0 if start should be associated with the right character. See relative-position assoc parameter. + * @param {number} assocEnd >= 0 if end should be associated with the right character. + */ + move (start, end, target, assocStart = 1, assocEnd = -1) { + if (start <= target && target <= end) { + // It doesn't make sense to move a range into the same range (it's basically a no-op). + return + } + if (this.doc !== null) { + transact(this.doc, transaction => { + useSearchMarker(transaction, this, target, walker => { + const left = createRelativePositionFromTypeIndex(this, start, assocStart) + const right = createRelativePositionFromTypeIndex(this, end + 1, assocEnd) + walker.insertMove(transaction, left, right) + }) + }) + } else { + const content = /** @type {Array} */ (this._prelimContent).splice(start, end - start + 1) + ;/** @type {Array} */ (this._prelimContent).splice(target, 0, ...content) + } + } + /** * Appends content to this YArray. * diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index 42aa6237..18a2d6ed 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -11,7 +11,7 @@ import { ContentType, ContentDoc, Doc, - ID, AbstractContent, ContentMove, Transaction, Item, AbstractType // eslint-disable-line + RelativePosition, ID, AbstractContent, ContentMove, Transaction, Item, AbstractType // eslint-disable-line } from '../internals.js' const lengthExceeded = error.create('Length exceeded!') @@ -122,8 +122,8 @@ export class ListIterator { len += this.rel this.rel = 0 } - while (item && !this.reachedEnd && (len > 0 || (len === 0 && (!item.countable || item.deleted)))) { - if (item.countable && !item.deleted && item.moved === this.currMove) { + while (item && !this.reachedEnd && (len > 0 || (len === 0 && (!item.countable || item.deleted || item === this.currMoveEnd)))) { + if (item.countable && !item.deleted && item.moved === this.currMove && len > 0) { len -= item.length if (len < 0) { this.rel = item.length + len @@ -228,10 +228,10 @@ export class ListIterator { _slice (tr, len, value, slice, concat) { this.index += len while (len > 0 && !this.reachedEnd) { - while (this.nextItem && this.nextItem.countable && !this.reachedEnd && len > 0) { - if (!this.nextItem.deleted) { + 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(this.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) { @@ -268,18 +268,16 @@ export class ListIterator { const sm = this.type._searchMarker let item = this.nextItem while (len > 0 && !this.reachedEnd) { - while (item && item.countable && !this.reachedEnd && len > 0) { - if (!item.deleted) { - if (this.rel > 0) { - item = getItemCleanStart(tr, createID(item.id.client, item.id.clock + this.rel)) - this.rel = 0 - } - if (len < item.length) { - getItemCleanStart(tr, createID(item.id.client, item.id.clock + len)) - } - len -= item.length - item.delete(tr) + while (item && !item.deleted && item.countable && !this.reachedEnd && len > 0) { + if (this.rel > 0) { + item = getItemCleanStart(tr, createID(item.id.client, item.id.clock + this.rel)) + this.rel = 0 } + if (len < item.length) { + getItemCleanStart(tr, createID(item.id.client, item.id.clock + len)) + } + len -= item.length + item.delete(tr) if (item.right) { item = item.right } else { @@ -300,12 +298,8 @@ export class ListIterator { /** * @param {Transaction} tr - * @param {Array|Array|boolean|number|null|string|Uint8Array>} content */ - insertArrayValue (tr, content) { - /** - * @type {Item | null} - */ + _splitRel (tr) { if (this.rel > 0) { /** * @type {ID} @@ -314,6 +308,14 @@ export class ListIterator { this.nextItem = getItemCleanStart(tr, createID(itemid.client, itemid.clock + this.rel)) this.rel = 0 } + } + + /** + * @param {Transaction} tr + * @param {Array} content + */ + insertContents (tr, content) { + this._splitRel(tr) const sm = this.type._searchMarker const parent = this.type const store = tr.doc.store @@ -327,18 +329,55 @@ export class ListIterator { * @type {Item | null} */ let left = this.left + content.forEach(c => { + left = new Item(createID(ownClientId, getState(store, ownClientId)), left, left && left.lastId, right, right && right.id, parent, null, c) + left.integrate(tr, 0) + }) + if (right === null && left !== null) { + this.nextItem = left + this.reachedEnd = true + } else { + this.nextItem = right + } + if (sm) { + updateMarkerChanges(sm, this.index, content.length, this) + } + } + + /** + * @param {Transaction} tr + * @param {RelativePosition} start + * @param {RelativePosition} end + */ + insertMove (tr, start, end) { + this.insertContents(tr, [new ContentMove(start, end, 1)]) // @todo adjust priority + // @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 + } + + /** + * @param {Transaction} tr + * @param {Array|Array|boolean|number|null|string|Uint8Array>} values + */ + insertArrayValue (tr, values) { + this._splitRel(tr) + /** + * @type {Array} + */ + const contents = [] /** * @type {Array|number|null>} */ let jsonContent = [] const packJsonContent = () => { if (jsonContent.length > 0) { - left = new Item(createID(ownClientId, getState(store, ownClientId)), left, left && left.lastId, right, right && right.id, parent, null, new ContentAny(jsonContent)) - left.integrate(tr, 0) + contents.push(new ContentAny(jsonContent)) jsonContent = [] } } - content.forEach(c => { + values.forEach(c => { if (c === null) { jsonContent.push(c) } else { @@ -355,17 +394,14 @@ export class ListIterator { switch (c.constructor) { case Uint8Array: case ArrayBuffer: - left = new Item(createID(ownClientId, getState(store, ownClientId)), left, left && left.lastId, right, right && right.id, parent, null, new ContentBinary(new Uint8Array(/** @type {Uint8Array} */ (c)))) - left.integrate(tr, 0) + contents.push(new ContentBinary(new Uint8Array(/** @type {Uint8Array} */ (c)))) break case Doc: - left = new Item(createID(ownClientId, getState(store, ownClientId)), left, left && left.lastId, right, right && right.id, parent, null, new ContentDoc(/** @type {Doc} */ (c))) - left.integrate(tr, 0) + contents.push(new ContentDoc(/** @type {Doc} */ (c))) break default: if (c instanceof AbstractType) { - left = new Item(createID(ownClientId, getState(store, ownClientId)), left, left && left.lastId, right, right && right.id, parent, null, new ContentType(c)) - left.integrate(tr, 0) + contents.push(new ContentType(c)) } else { throw new Error('Unexpected content type in insert operation') } @@ -374,16 +410,8 @@ export class ListIterator { } }) packJsonContent() - if (right === null && left !== null) { - this.nextItem = left - this.reachedEnd = true - } else { - this.nextItem = right - } - if (sm) { - updateMarkerChanges(sm, this.index, content.length, this) - } - this.index += content.length + this.insertContents(tr, contents) + this.index += values.length } /** diff --git a/src/utils/YEvent.js b/src/utils/YEvent.js index 3501b587..a87cd181 100644 --- a/src/utils/YEvent.js +++ b/src/utils/YEvent.js @@ -1,7 +1,8 @@ import { isDeleted, - Item, AbstractType, Transaction, AbstractStruct // eslint-disable-line + getMovedCoords, + ContentMove, Item, AbstractType, Transaction, AbstractStruct // eslint-disable-line } from '../internals.js' import * as set from 'lib0/set' @@ -153,62 +154,105 @@ export class YEvent { get changes () { let changes = this._changes if (changes === null) { - const target = this.target - const added = set.create() - const deleted = set.create() - /** - * @type {Array<{insert:Array}|{delete:number}|{retain:number}>} - */ - const delta = [] - changes = { - added, - deleted, - delta, - keys: this.keys - } - const changed = /** @type Set */ (this.transaction.changed.get(target)) - if (changed.has(null)) { + this.transaction.doc.transact(tr => { + const target = this.target + const added = set.create() + const deleted = set.create() /** - * @type {any} + * @type {Array<{insert:Array}|{delete:number}|{retain:number}>} */ - let lastOp = null - const packOp = () => { - if (lastOp) { - delta.push(lastOp) - } + const delta = [] + changes = { + added, + deleted, + delta, + keys: this.keys } - for (let item = target._start; item !== null; item = item.right) { - if (item.deleted) { - if (this.deletes(item) && !this.adds(item)) { - if (lastOp === null || lastOp.delete === undefined) { - packOp() - lastOp = { delete: 0 } - } - lastOp.delete += item.length - deleted.add(item) - } // else nop - } else { - if (this.adds(item)) { - if (lastOp === null || lastOp.insert === undefined) { - packOp() - lastOp = { insert: [] } - } - lastOp.insert = lastOp.insert.concat(item.content.getContent()) - added.add(item) - } else { - if (lastOp === null || lastOp.retain === undefined) { - packOp() - lastOp = { retain: 0 } - } - lastOp.retain += item.length + const changed = /** @type Set */ (this.transaction.changed.get(target)) + if (changed.has(null)) { + /** + * @type {Array<{ end: Item | null, move: Item | null, isNew : boolean }>} + */ + const movedStack = [] + /** + * @type {Item | null} + */ + let currMove = null + /** + * @type {boolean} + */ + let currMoveIsNew = false + /** + * @type {Item | null} + */ + let currMoveEnd = null + /** + * @type {any} + */ + let lastOp = null + const packOp = () => { + if (lastOp) { + delta.push(lastOp) } } + for (let item = target._start; item !== null;) { + if (item === currMoveEnd) { + item = currMove + const { end, move, isNew } = movedStack.pop() || { end: null, move: null, isNew: false } + currMoveIsNew = isNew + currMoveEnd = end + currMove = move + } else if (item.content.constructor === ContentMove) { + if (item.moved === currMove) { + movedStack.push({ end: currMoveEnd, move: currMove, isNew: currMoveIsNew }) + const { start, end } = getMovedCoords(item.content, tr) + currMove = item + currMoveEnd = end + currMoveIsNew = this.adds(item) + item = start + continue // do not move to item.right + } + } else if (item.moved !== currMove) { + if (!currMoveIsNew && item.countable && !this.adds(item)) { + if (lastOp === null || lastOp.delete === undefined) { + packOp() + lastOp = { delete: 0 } + } + lastOp.delete += item.length + } + } else if (item.deleted) { + if (!currMoveIsNew && this.deletes(item) && !this.adds(item)) { + if (lastOp === null || lastOp.delete === undefined) { + packOp() + lastOp = { delete: 0 } + } + lastOp.delete += item.length + deleted.add(item) + } + } else { + if (currMoveIsNew || this.adds(item)) { + if (lastOp === null || lastOp.insert === undefined) { + packOp() + lastOp = { insert: [] } + } + lastOp.insert = lastOp.insert.concat(item.content.getContent()) + added.add(item) + } else { + if (lastOp === null || lastOp.retain === undefined) { + packOp() + lastOp = { retain: 0 } + } + lastOp.retain += item.length + } + } + item = /** @type {Item} */ (item).right + } + if (lastOp !== null && lastOp.retain === undefined) { + packOp() + } } - if (lastOp !== null && lastOp.retain === undefined) { - packOp() - } - } - this._changes = changes + this._changes = changes + }) } return /** @type {any} */ (changes) } diff --git a/tests/testHelper.js b/tests/testHelper.js index 542abe39..9963e16e 100644 --- a/tests/testHelper.js +++ b/tests/testHelper.js @@ -373,7 +373,10 @@ export const compare = users => { t.compare(Y.encodeStateVector(users[i]), Y.encodeStateVector(users[i + 1])) compareDS(Y.createDeleteSetFromStructStore(users[i].store), Y.createDeleteSetFromStructStore(users[i + 1].store)) compareStructStores(users[i].store, users[i + 1].store) + // @todo // test list-iterator + // console.log('dutiraneduiaentdr', users[0].getArray('array')._searchMarker) + /* { const user = users[0] user.transact(tr => { @@ -396,6 +399,7 @@ export const compare = users => { }) }) } + */ } users.map(u => u.destroy()) } diff --git a/tests/y-array.tests.js b/tests/y-array.tests.js index eaf085e3..02e55ae1 100644 --- a/tests/y-array.tests.js +++ b/tests/y-array.tests.js @@ -1,4 +1,4 @@ -import { init, compare, applyRandomTests, Doc, AbstractType } from './testHelper.js' // eslint-disable-line +import { init, compare, applyRandomTests, Doc, AbstractType, TestConnector } from './testHelper.js' // eslint-disable-line import * as Y from '../src/index.js' import * as t from 'lib0/testing' @@ -432,6 +432,43 @@ export const testEventTargetIsSetCorrectlyOnRemote = tc => { compare(users) } +/** + * @param {t.TestCase} tc + */ +export const testMove = tc => { + { + // move in uninitialized type + const yarr = new Y.Array() + yarr.insert(0, [1, 2, 3]) + yarr.move(1, 1, 0) + // @ts-ignore + t.compare(yarr._prelimContent, [2, 1, 3]) + } + const { array0, array1, users } = init(tc, { users: 3 }) + /** + * @type {any} + */ + let event0 = null + /** + * @type {any} + */ + let event1 = null + array0.observe(event => { + event0 = event + }) + array1.observe(event => { + event1 = event + }) + array0.insert(0, [1, 2, 3]) + array0.move(1, 1, 0) + t.compare(array0.toArray(), [2, 1, 3]) + t.compare(event0.delta, [{ insert: [2] }, { retain: 1 }, { delete: 1 }]) + Y.applyUpdate(users[1], Y.encodeStateAsUpdate(users[0])) + t.compare(array1.toArray(), [2, 1, 3]) + t.compare(event1.delta, [{ insert: [2, 1, 3] }]) + compare(users) +} + /** * @param {t.TestCase} tc */ @@ -473,7 +510,6 @@ const arrayTransactions = [ yarray.insert(pos, content) oldContent.splice(pos, 0, ...content) t.compareArrays(yarray.toArray(), oldContent) // we want to make sure that fastSearch markers insert at the correct position - t.compare(yarray.toJSON(), yarray.toArray().map(x => x instanceof AbstractType ? x.toJSON() : x)) }, function insertTypeArray (user, gen) { const yarray = user.getArray('array') From 09482294224cac397b06950712110b725e86971d Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Mon, 6 Dec 2021 15:01:37 +0100 Subject: [PATCH 15/26] handle nested moves --- src/structs/ContentMove.js | 20 ++++++++++++++++++-- src/structs/Item.js | 6 ++++++ src/types/YArray.js | 4 ++-- src/utils/ListIterator.js | 4 ++-- src/utils/RelativePosition.js | 27 ++++++++++++--------------- src/utils/Transaction.js | 8 ++++++++ src/utils/YEvent.js | 2 +- tests/y-array.tests.js | 3 +++ 8 files changed, 52 insertions(+), 22 deletions(-) diff --git a/src/structs/ContentMove.js b/src/structs/ContentMove.js index 4ffc8c0f..1366b2d5 100644 --- a/src/structs/ContentMove.js +++ b/src/structs/ContentMove.js @@ -2,8 +2,9 @@ import * as error from 'lib0/error' import * as decoding from 'lib0/decoding' import * as encoding from 'lib0/encoding' +import * as math from 'lib0/math' import { - AbstractType, ContentType, ID, RelativePosition, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, Transaction, Item, StructStore, getItem, getItemCleanStart, getItemCleanEnd // eslint-disable-line + AbstractType, ContentType, RelativePosition, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, Transaction, Item, StructStore, getItem, getItemCleanStart, getItemCleanEnd // eslint-disable-line } from '../internals.js' /** @@ -175,13 +176,24 @@ export class ContentMove { * @type {{ start: Item | null, end: Item | null }} */ let { start, end } = getMovedCoords(this, transaction) + let maxPriority = 0 + // If this ContentMove was created locally, we set prio = -1. This indicates + // 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 - if (currMoved === null || /** @type {ContentMove} */ (currMoved.content).priority < this.priority || currMoved.id.client < item.id.client || (currMoved.id.client === item.id.client && currMoved.id.clock < item.id.clock)) { + const nextPrio = currMoved ? /** @type {ContentMove} */ (currMoved.content).priority : -1 + if (currMoved === null || adaptPriority || 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 + if (start.moved && !transaction.prevMoved.has(start)) { + // we need to know which item previously moved an item + transaction.prevMoved.set(start, start.moved) + } start.moved = item } else { /** @type {ContentMove} */ (currMoved.content).overrides.add(item) @@ -189,6 +201,9 @@ export class ContentMove { } start = start.right } + if (adaptPriority) { + this.priority = maxPriority + 1 + } } /** @@ -246,6 +261,7 @@ export class ContentMove { /** * @private + * @todo use binary encoding option for start & end relpos's * * @param {UpdateDecoderV1 | UpdateDecoderV2} decoder * @return {ContentMove} diff --git a/src/structs/Item.js b/src/structs/Item.js index 2fdff261..44538faf 100644 --- a/src/structs/Item.js +++ b/src/structs/Item.js @@ -117,6 +117,12 @@ export const splitItem = (transaction, leftItem, diff) => { /** @type {AbstractType} */ (rightItem.parent)._map.set(rightItem.parentSub, rightItem) } leftItem.length = diff + if (leftItem.moved) { + const m = transaction.prevMoved.get(leftItem) + if (m) { + transaction.prevMoved.set(rightItem, m) + } + } return rightItem } diff --git a/src/types/YArray.js b/src/types/YArray.js index a40dd92c..2f425a58 100644 --- a/src/types/YArray.js +++ b/src/types/YArray.js @@ -149,9 +149,9 @@ export class YArray extends AbstractType { } if (this.doc !== null) { transact(this.doc, transaction => { + const left = createRelativePositionFromTypeIndex(this, start, assocStart) + const right = createRelativePositionFromTypeIndex(this, end + 1, assocEnd) useSearchMarker(transaction, this, target, walker => { - const left = createRelativePositionFromTypeIndex(this, start, assocStart) - const right = createRelativePositionFromTypeIndex(this, end + 1, assocEnd) walker.insertMove(transaction, left, right) }) }) diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index 18a2d6ed..1e7385dc 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -122,7 +122,7 @@ export class ListIterator { len += this.rel this.rel = 0 } - while (item && !this.reachedEnd && (len > 0 || (len === 0 && (!item.countable || item.deleted || item === this.currMoveEnd)))) { + while (item && !this.reachedEnd && (len > 0 || (len === 0 && (!item.countable || item.deleted || item === this.currMoveEnd || item.moved !== this.currMove)))) { if (item.countable && !item.deleted && item.moved === this.currMove && len > 0) { len -= item.length if (len < 0) { @@ -350,7 +350,7 @@ export class ListIterator { * @param {RelativePosition} end */ insertMove (tr, start, end) { - this.insertContents(tr, [new ContentMove(start, end, 1)]) // @todo adjust priority + this.insertContents(tr, [new ContentMove(start, end, -1)]) // @todo adjust priority // @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 diff --git a/src/utils/RelativePosition.js b/src/utils/RelativePosition.js index 614c0bc5..719d7ba6 100644 --- a/src/utils/RelativePosition.js +++ b/src/utils/RelativePosition.js @@ -9,6 +9,8 @@ import { createID, ContentType, followRedone, + transact, + useSearchMarker, ID, Doc, AbstractType // eslint-disable-line } from '../internals.js' @@ -161,7 +163,6 @@ export const createRelativePosition = (type, item, assoc) => { * @function */ export const createRelativePositionFromTypeIndex = (type, index, assoc = 0) => { - let t = type._start if (assoc < 0) { // associated to the left character or the beginning of a type, increment index if possible. if (index === 0) { @@ -169,21 +170,17 @@ export const createRelativePositionFromTypeIndex = (type, index, assoc = 0) => { } index-- } - while (t !== null) { - if (!t.deleted && t.countable) { - if (t.length > index) { - // case 1: found position somewhere in the linked list - return createRelativePosition(type, createID(t.id.client, t.id.clock + index), assoc) + return transact(/** @type {Doc} */ (type.doc), tr => + useSearchMarker(tr, type, index, walker => { + if (walker.reachedEnd) { + const item = assoc < 0 ? /** @type {Item} */ (walker.nextItem).lastId : null + return createRelativePosition(type, item, assoc) + } else { + const id = /** @type {Item} */ (walker.nextItem).id + return createRelativePosition(type, createID(id.client, id.clock + walker.rel), assoc) } - index -= t.length - } - if (t.right === null && assoc < 0) { - // left-associated position, return last available id - return createRelativePosition(type, t.lastId, assoc) - } - t = t.right - } - return createRelativePosition(type, null, assoc) + }) + ) } /** diff --git a/src/utils/Transaction.js b/src/utils/Transaction.js index 7fb507f1..6476fa30 100644 --- a/src/utils/Transaction.js +++ b/src/utils/Transaction.js @@ -114,6 +114,14 @@ export class Transaction { * @type {Set} */ this.subdocsLoaded = new Set() + /** + * We store the reference that last moved an item. + * This is needed to compute the delta when multiple ContentMove move + * the same item. + * + * @type {Map} + */ + this.prevMoved = new Map() } } diff --git a/src/utils/YEvent.js b/src/utils/YEvent.js index a87cd181..10c757de 100644 --- a/src/utils/YEvent.js +++ b/src/utils/YEvent.js @@ -213,7 +213,7 @@ export class YEvent { continue // do not move to item.right } } else if (item.moved !== currMove) { - if (!currMoveIsNew && item.countable && !this.adds(item)) { + if (!currMoveIsNew && item.countable && !this.adds(item) && (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 02e55ae1..6cc65c8e 100644 --- a/tests/y-array.tests.js +++ b/tests/y-array.tests.js @@ -466,6 +466,9 @@ export const testMove = tc => { Y.applyUpdate(users[1], Y.encodeStateAsUpdate(users[0])) t.compare(array1.toArray(), [2, 1, 3]) t.compare(event1.delta, [{ insert: [2, 1, 3] }]) + array0.move(0, 0, 2) + t.compare(array0.toArray(), [1, 2, 3]) + t.compare(event0.delta, [{ delete: 1 }, { retain: 1 }, { insert: [2] }]) compare(users) } From 4356d70ed0bbc6e5ff68de43c2701844230288be Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Mon, 6 Dec 2021 21:00:20 +0100 Subject: [PATCH 16/26] reinit search marker after transaction --- src/types/AbstractType.js | 2 ++ src/types/YArray.js | 18 ++++++++++-------- src/utils/ListIterator.js | 15 ++++++++++++++- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/types/AbstractType.js b/src/types/AbstractType.js index 8d3beb8b..bd4ccc2e 100644 --- a/src/types/AbstractType.js +++ b/src/types/AbstractType.js @@ -54,6 +54,8 @@ export const useSearchMarker = (tr, yarray, index, f) => { const prevItem = /** @type {Item} */ (sm.nextItem) if (createFreshMarker) { searchMarker.push(fsm) + } else { + fsm.reinit(tr) } const diff = fsm.index - index if (diff > 0) { diff --git a/src/types/YArray.js b/src/types/YArray.js index 2f425a58..3a0d806e 100644 --- a/src/types/YArray.js +++ b/src/types/YArray.js @@ -124,14 +124,16 @@ export class YArray extends AbstractType { * @param {Array} content The array of content */ insert (index, content) { - if (this.doc !== null) { - transact(this.doc, transaction => { - useSearchMarker(transaction, this, index, walker => - walker.insertArrayValue(transaction, content) - ) - }) - } else { - /** @type {Array} */ (this._prelimContent).splice(index, 0, ...content) + if (content.length > 0) { + if (this.doc !== null) { + transact(this.doc, transaction => { + useSearchMarker(transaction, this, index, walker => + walker.insertArrayValue(transaction, content) + ) + }) + } else { + /** @type {Array} */ (this._prelimContent).splice(index, 0, ...content) + } } } diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index 1e7385dc..1258e13d 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -1,4 +1,5 @@ import * as error from 'lib0/error' +import { getItem } from 'yjs' import { getItemCleanStart, @@ -73,6 +74,18 @@ export class ListIterator { return iter } + /** + * @param {Transaction} tr + */ + reinit (tr) { + if (this.nextItem) { + const nextId = this.nextItem.id + const reinitId = createID(nextId.client, nextId.clock + this.rel) + this.nextItem = getItem(tr.doc.store, reinitId) + this.rel = reinitId.clock - this.nextItem.id.clock + } + } + /** * @type {Item | null} */ @@ -333,7 +346,7 @@ export class ListIterator { left = new Item(createID(ownClientId, getState(store, ownClientId)), left, left && left.lastId, right, right && right.id, parent, null, c) left.integrate(tr, 0) }) - if (right === null && left !== null) { + if (right === null) { this.nextItem = left this.reachedEnd = true } else { From 6230abb78c39f5ddb7c60ed51f005abef4a11c22 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Mon, 6 Dec 2021 21:22:18 +0100 Subject: [PATCH 17/26] make sure that markers are correct without reinit --- src/structs/Item.js | 1 + src/types/AbstractType.js | 3 --- src/utils/ListIterator.js | 13 ------------- 3 files changed, 1 insertion(+), 16 deletions(-) diff --git a/src/structs/Item.js b/src/structs/Item.js index 44538faf..392321e0 100644 --- a/src/structs/Item.js +++ b/src/structs/Item.js @@ -594,6 +594,7 @@ export class Item extends AbstractStruct { if (searchMarker) { for (let i = searchMarker.length - 1; i >= 0; i--) { if (searchMarker[i].nextItem === right) { + // @todo do something more efficient than splicing.. searchMarker.splice(i, 1) } } diff --git a/src/types/AbstractType.js b/src/types/AbstractType.js index bd4ccc2e..2f01dc14 100644 --- a/src/types/AbstractType.js +++ b/src/types/AbstractType.js @@ -43,7 +43,6 @@ export const useSearchMarker = (tr, yarray, index, f) => { const sm = new ListIterator(yarray).forward(tr, index) searchMarker.push(sm) if (sm.nextItem) sm.nextItem.marker = true - return f(sm) } const sm = searchMarker.reduce( (a, b, arrayIndex) => math.abs(index - a.index) < math.abs(index - b.index) ? a : b @@ -54,8 +53,6 @@ export const useSearchMarker = (tr, yarray, index, f) => { const prevItem = /** @type {Item} */ (sm.nextItem) if (createFreshMarker) { searchMarker.push(fsm) - } else { - fsm.reinit(tr) } const diff = fsm.index - index if (diff > 0) { diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index 1258e13d..7754f5a6 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -1,5 +1,4 @@ import * as error from 'lib0/error' -import { getItem } from 'yjs' import { getItemCleanStart, @@ -74,18 +73,6 @@ export class ListIterator { return iter } - /** - * @param {Transaction} tr - */ - reinit (tr) { - if (this.nextItem) { - const nextId = this.nextItem.id - const reinitId = createID(nextId.client, nextId.clock + this.rel) - this.nextItem = getItem(tr.doc.store, reinitId) - this.rel = reinitId.clock - this.nextItem.id.clock - } - } - /** * @type {Item | null} */ From f5781f836623d7a3a5f9265bfcaee6f162772f6e Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Mon, 6 Dec 2021 22:07:46 +0100 Subject: [PATCH 18/26] update searchmarkers after insert correctly --- src/utils/ListIterator.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index 7754f5a6..2038b1c2 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -311,12 +311,13 @@ export class ListIterator { } /** + * Important: you must update markers after calling this method! + * * @param {Transaction} tr * @param {Array} content */ insertContents (tr, content) { this._splitRel(tr) - const sm = this.type._searchMarker const parent = this.type const store = tr.doc.store const ownClientId = tr.doc.clientID @@ -339,9 +340,6 @@ export class ListIterator { } else { this.nextItem = right } - if (sm) { - updateMarkerChanges(sm, this.index, content.length, this) - } } /** @@ -363,6 +361,7 @@ export class ListIterator { */ insertArrayValue (tr, values) { this._splitRel(tr) + const sm = this.type._searchMarker /** * @type {Array} */ @@ -412,6 +411,9 @@ export class ListIterator { packJsonContent() this.insertContents(tr, contents) this.index += values.length + if (sm) { + updateMarkerChanges(sm, this.index, values.length, this) + } } /** From bd47efe0ee30fe1e032bc08a8336032a21b2a54b Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Mon, 6 Dec 2021 22:21:55 +0100 Subject: [PATCH 19/26] fix all tests --- src/utils/ListIterator.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index 2038b1c2..48c6760c 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -412,7 +412,7 @@ export class ListIterator { this.insertContents(tr, contents) this.index += values.length if (sm) { - updateMarkerChanges(sm, this.index, values.length, this) + updateMarkerChanges(sm, this.index - values.length, values.length, this) } } From 3d31ba8759a1b871d6f4fe39653306e8ffe06b14 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Tue, 7 Dec 2021 12:36:42 +0100 Subject: [PATCH 20/26] adding more sanity checkss to yarray.tests --- src/structs/ContentMove.js | 23 ++++++++++++-- src/types/YArray.js | 30 +++++++++++++++++- src/utils/ListIterator.js | 40 ++++++++++++++++++------ src/utils/RelativePosition.js | 4 +++ src/utils/YEvent.js | 2 +- src/utils/updates.js | 1 + tests/y-array.tests.js | 59 ++++++++++++++++++++++++++++++++--- 7 files changed, 140 insertions(+), 19 deletions(-) diff --git a/src/structs/ContentMove.js b/src/structs/ContentMove.js index 1366b2d5..a196ac02 100644 --- a/src/structs/ContentMove.js +++ b/src/structs/ContentMove.js @@ -6,6 +6,7 @@ import * as math from 'lib0/math' import { AbstractType, ContentType, RelativePosition, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, Transaction, Item, StructStore, getItem, getItemCleanStart, getItemCleanEnd // eslint-disable-line } from '../internals.js' +import { decodeRelativePosition, encodeRelativePosition } from 'yjs' /** * @param {ContentMove} moved @@ -246,8 +247,12 @@ export class ContentMove { * @param {number} offset */ write (encoder, offset) { - encoder.writeAny(this.start) - encoder.writeAny(this.end) + const isCollapsed = this.isCollapsed() + encoding.writeUint8(encoder.restEncoder, isCollapsed ? 1 : 0) + encoder.writeBuf(encodeRelativePosition(this.start)) + if (!isCollapsed) { + encoder.writeBuf(encodeRelativePosition(this.end)) + } encoding.writeVarUint(encoder.restEncoder, this.priority) } @@ -257,6 +262,10 @@ export class ContentMove { getRef () { return 11 } + + isCollapsed () { + return this.start.item === this.end.item && this.start.item !== null + } } /** @@ -266,4 +275,12 @@ export class ContentMove { * @param {UpdateDecoderV1 | UpdateDecoderV2} decoder * @return {ContentMove} */ -export const readContentMove = decoder => new ContentMove(decoder.readAny(), decoder.readAny(), decoding.readVarUint(decoder.restDecoder)) +export const readContentMove = decoder => { + const isCollapsed = decoding.readUint8(decoder.restDecoder) === 1 + const start = decodeRelativePosition(decoder.readBuf()) + const end = isCollapsed ? start.clone() : decodeRelativePosition(decoder.readBuf()) + if (isCollapsed) { + end.assoc = -1 + } + return new ContentMove(start, end, decoding.readVarUint(decoder.restDecoder)) +} diff --git a/src/types/YArray.js b/src/types/YArray.js index 3a0d806e..e04f5793 100644 --- a/src/types/YArray.js +++ b/src/types/YArray.js @@ -137,6 +137,34 @@ export class YArray extends AbstractType { } } + /** + * Move a single item from $index to $target. + * + * @todo make sure that collapsed moves are removed (i.e. when moving the same item twice) + * + * @param {number} index + * @param {number} target + */ + move (index, target) { + if (index === target || index + 1 === target || index >= this.length) { + // It doesn't make sense to move a range into the same range (it's basically a no-op). + return + } + if (this.doc !== null) { + transact(this.doc, transaction => { + const left = createRelativePositionFromTypeIndex(this, index, 1) + const right = left.clone() + right.assoc = -1 + useSearchMarker(transaction, this, target, walker => { + walker.insertMove(transaction, left, right) + }) + }) + } else { + const content = /** @type {Array} */ (this._prelimContent).splice(index, 1) + ;/** @type {Array} */ (this._prelimContent).splice(target, 0, ...content) + } + } + /** * @param {number} start Inclusive move-start * @param {number} end Inclusive move-end @@ -144,7 +172,7 @@ export class YArray extends AbstractType { * @param {number} assocStart >=0 if start should be associated with the right character. See relative-position assoc parameter. * @param {number} assocEnd >= 0 if end should be associated with the right character. */ - move (start, end, target, assocStart = 1, assocEnd = -1) { + moveRange (start, end, target, assocStart = 1, assocEnd = -1) { if (start <= target && target <= end) { // It doesn't make sense to move a range into the same range (it's basically a no-op). return diff --git a/src/utils/ListIterator.js b/src/utils/ListIterator.js index 48c6760c..4fd830c8 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -122,8 +122,17 @@ export class ListIterator { len += this.rel this.rel = 0 } - while (item && !this.reachedEnd && (len > 0 || (len === 0 && (!item.countable || item.deleted || item === this.currMoveEnd || item.moved !== this.currMove)))) { - if (item.countable && !item.deleted && item.moved === this.currMove && len > 0) { + 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 + } else if (item === null) { + break + } else if (item.countable && !item.deleted && item.moved === this.currMove && len > 0) { len -= item.length if (len < 0) { this.rel = item.length + len @@ -141,13 +150,6 @@ export class ListIterator { item = start continue } - if (item === this.currMoveEnd) { - 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 - } if (item.right) { item = item.right } else { @@ -159,6 +161,23 @@ export class ListIterator { return this } + /** + * @param {Transaction} tr + */ + reduceMoves (tr) { + let item = this.nextItem + 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 + } + this.nextItem = item + } + } + /** * @param {Transaction} tr * @param {number} len @@ -249,7 +268,7 @@ export class ListIterator { this.reachedEnd = true } } - if (this.nextItem && !this.reachedEnd && len > 0) { + if (this.nextItem && (!this.reachedEnd || this.currMove !== null) && len > 0) { this.forward(tr, 0) } } @@ -317,6 +336,7 @@ export class ListIterator { * @param {Array} content */ insertContents (tr, content) { + this.reduceMoves(tr) this._splitRel(tr) const parent = this.type const store = tr.doc.store diff --git a/src/utils/RelativePosition.js b/src/utils/RelativePosition.js index 719d7ba6..ea3e33e9 100644 --- a/src/utils/RelativePosition.js +++ b/src/utils/RelativePosition.js @@ -75,6 +75,10 @@ export class RelativePosition { */ this.assoc = assoc } + + clone () { + return new RelativePosition(this.type, this.tname, this.item, this.assoc) + } } /** diff --git a/src/utils/YEvent.js b/src/utils/YEvent.js index 10c757de..f50b9d70 100644 --- a/src/utils/YEvent.js +++ b/src/utils/YEvent.js @@ -213,7 +213,7 @@ export class YEvent { continue // do not move to item.right } } else if (item.moved !== currMove) { - if (!currMoveIsNew && item.countable && !this.adds(item) && (this.transaction.prevMoved.get(item) || null) === currMove) { + if (!currMoveIsNew && item.countable && 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/src/utils/updates.js b/src/utils/updates.js index fa6add00..0a9072c5 100644 --- a/src/utils/updates.js +++ b/src/utils/updates.js @@ -308,6 +308,7 @@ export const mergeUpdatesV2 = (updates, YDecoder = UpdateDecoderV2, YEncoder = U // Note: Should handle that some operations cannot be applied yet () while (true) { + // @todo this incurs an exponential overhead. We could instead only sort the item that changed. // Write higher clients first ⇒ sort by clientID & clock and remove decoders without content lazyStructDecoders = lazyStructDecoders.filter(dec => dec.curr !== null) lazyStructDecoders.sort( diff --git a/tests/y-array.tests.js b/tests/y-array.tests.js index 6cc65c8e..3c5ff268 100644 --- a/tests/y-array.tests.js +++ b/tests/y-array.tests.js @@ -440,7 +440,7 @@ export const testMove = tc => { // move in uninitialized type const yarr = new Y.Array() yarr.insert(0, [1, 2, 3]) - yarr.move(1, 1, 0) + yarr.move(1, 0) // @ts-ignore t.compare(yarr._prelimContent, [2, 1, 3]) } @@ -460,13 +460,13 @@ export const testMove = tc => { event1 = event }) array0.insert(0, [1, 2, 3]) - array0.move(1, 1, 0) + array0.move(1, 0) t.compare(array0.toArray(), [2, 1, 3]) t.compare(event0.delta, [{ insert: [2] }, { retain: 1 }, { delete: 1 }]) Y.applyUpdate(users[1], Y.encodeStateAsUpdate(users[0])) t.compare(array1.toArray(), [2, 1, 3]) t.compare(event1.delta, [{ insert: [2, 1, 3] }]) - array0.move(0, 0, 2) + array0.move(0, 2) t.compare(array0.toArray(), [1, 2, 3]) t.compare(event0.delta, [{ delete: 1 }, { retain: 1 }, { insert: [2] }]) compare(users) @@ -500,6 +500,19 @@ 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 + }, function insert (user, gen) { const yarray = user.getArray('array') const uniqueNumber = getUniqueNumber() @@ -558,11 +571,49 @@ const arrayTransactions = [ } ] +/** + * @param {Y.Doc} user + */ +const monitorArrayTestObject = user => { + /** + * @type {Array} + */ + const arr = [] + const yarr = user.getArray('array') + yarr.observe(event => { + let currpos = 0 + const delta = event.delta + for (let i = 0; i < delta.length; i++) { + const d = delta[i] + if (d.insert != null) { + arr.splice(currpos, 0, ...(/** @type {Array} */ (d.insert))) + currpos += /** @type {Array} */ (d.insert).length + } else if (d.retain != null) { + currpos += d.retain + } else { + arr.splice(currpos, d.delete) + } + } + }) + return arr +} + +/** + * @param {{ testObjects: Array>, users: Array }} cmp + */ +const compareTestobjects = cmp => { + const arrs = cmp.testObjects + for (let i = 0; i < arrs.length; i++) { + const type = cmp.users[i].getArray('array') + t.compareArrays(arrs[i], type.toArray()) + } +} + /** * @param {t.TestCase} tc */ export const testRepeatGeneratingYarrayTests6 = tc => { - applyRandomTests(tc, arrayTransactions, 10) + compareTestobjects(applyRandomTests(tc, arrayTransactions, 3, monitorArrayTestObject)) } /** From b75682022e45458b59ab367a2ef799ca7d5b16ad Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Tue, 7 Dec 2021 12:41:40 +0100 Subject: [PATCH 21/26] skip tests in preversion (should be handled by np) --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index d4a69b1f..e481d5b3 100644 --- a/package.json +++ b/package.json @@ -19,7 +19,7 @@ "lint": "markdownlint README.md && standard && tsc", "docs": "rm -rf docs; jsdoc --configure ./.jsdoc.json --verbose --readme ./README.md --package ./package.json || true", "serve-docs": "npm run docs && http-server ./docs/", - "preversion": "npm run lint && PRODUCTION=1 npm run dist && npm run docs && node ./dist/tests.cjs --repetition-time 1000 && test -e dist/src/index.d.ts && test -e dist/yjs.cjs && test -e dist/yjs.cjs", + "preversion": "npm run lint && PRODUCTION=1 npm run dist && npm run docs && test -e dist/src/index.d.ts && test -e dist/yjs.cjs && test -e dist/yjs.cjs", "debug": "concurrently 'http-server -o test.html' 'npm run watch'", "trace-deopt": "clear && rollup -c && node --trace-deopt dist/test.cjs", "trace-opt": "clear && rollup -c && node --trace-opt dist/test.cjs", From 24bca2af4333135b4538717488ea372c8c19dfcd Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Tue, 7 Dec 2021 12:42:32 +0100 Subject: [PATCH 22/26] 13.6.0-0 --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 6d8ad924..ade14b93 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "yjs", - "version": "13.5.22", + "version": "13.6.0-0", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index e481d5b3..3cbb67fc 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "yjs", - "version": "13.5.22", + "version": "13.6.0-0", "description": "Shared Editing Library", "main": "./dist/yjs.cjs", "module": "./dist/yjs.mjs", From 53e2c83f8675823829950a543e9d717669565c09 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Tue, 7 Dec 2021 13:53:28 +0100 Subject: [PATCH 23/26] add meta property to AbstractType --- src/types/AbstractType.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/types/AbstractType.js b/src/types/AbstractType.js index 2f01dc14..9e1a877c 100644 --- a/src/types/AbstractType.js +++ b/src/types/AbstractType.js @@ -197,6 +197,13 @@ export class AbstractType { * @type {null | Array} */ this._searchMarker = null + /** + * You can store custom stuff here. + * This might be useful to associate your application state to this shared type. + * + * @type {Map} + */ + this.meta = new Map() } /** From 6fc4fbd46631f0a8878a1490bb0b31ba19bbbed0 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Tue, 7 Dec 2021 13:54:22 +0100 Subject: [PATCH 24/26] 13.6.0-1 --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index ade14b93..f279134d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "yjs", - "version": "13.6.0-0", + "version": "13.6.0-1", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 3cbb67fc..48b33c28 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "yjs", - "version": "13.6.0-0", + "version": "13.6.0-1", "description": "Shared Editing Library", "main": "./dist/yjs.cjs", "module": "./dist/yjs.mjs", From d3dcd24ef44b906b4fe271f1d2c1bd2ebfa0940e Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Wed, 8 Dec 2021 16:10:49 +0100 Subject: [PATCH 25/26] fix various tests --- src/structs/Item.js | 15 +++++++++++++- src/utils/ListIterator.js | 6 +++--- src/utils/YEvent.js | 8 +++++--- tests/y-array.tests.js | 42 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 63 insertions(+), 8 deletions(-) diff --git a/src/structs/Item.js b/src/structs/Item.js index 392321e0..44e9aeef 100644 --- a/src/structs/Item.js +++ b/src/structs/Item.js @@ -28,6 +28,7 @@ import { import * as error from 'lib0/error' import * as binary from 'lib0/binary' +import { ContentMove } from './ContentMove.js' /** * @todo This should return several items @@ -381,9 +382,19 @@ export class Item extends AbstractStruct { if (this.parent && this.parent.constructor === ID && this.id.client !== this.parent.client && this.parent.clock >= getState(store, this.parent.client)) { return this.parent.client } + if (this.content.constructor === ContentMove) { + const c = /** @type {ContentMove} */ (this.content) + const start = c.start.item + const end = c.isCollapsed() ? null : c.end.item + if (start && start.clock >= getState(store, start.client)) { + return start.client + } + if (end && end.clock >= getState(store, end.client)) { + return end.client + } + } // We have all missing ids, now find the items - if (this.origin) { this.left = getItemCleanEnd(transaction, this.origin) this.origin = this.left.lastId @@ -413,6 +424,7 @@ export class Item extends AbstractStruct { this.parent = /** @type {ContentType} */ (parentItem.content).type } } + return null } @@ -640,6 +652,7 @@ 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/utils/ListIterator.js b/src/utils/ListIterator.js index 4fd830c8..4404f23c 100644 --- a/src/utils/ListIterator.js +++ b/src/utils/ListIterator.js @@ -286,8 +286,8 @@ export class ListIterator { const startLength = len const sm = this.type._searchMarker let item = this.nextItem - while (len > 0 && !this.reachedEnd) { - while (item && !item.deleted && item.countable && !this.reachedEnd && len > 0) { + while (len > 0) { + while (item && !item.deleted && item.countable && !this.reachedEnd && len > 0 && item.moved === this.currMove && item !== this.currMoveEnd) { if (this.rel > 0) { item = getItemCleanStart(tr, createID(item.id.client, item.id.clock + this.rel)) this.rel = 0 @@ -303,7 +303,7 @@ export class ListIterator { this.reachedEnd = true } } - if (item && !this.reachedEnd && len > 0) { + if (len > 0) { this.nextItem = item this.forward(tr, 0) item = this.nextItem diff --git a/src/utils/YEvent.js b/src/utils/YEvent.js index f50b9d70..e583fed3 100644 --- a/src/utils/YEvent.js +++ b/src/utils/YEvent.js @@ -195,13 +195,15 @@ export class YEvent { delta.push(lastOp) } } - for (let item = target._start; item !== null;) { - if (item === currMoveEnd) { + for (let item = target._start; ;) { + if (item === currMoveEnd && currMove) { item = currMove const { end, move, isNew } = movedStack.pop() || { end: null, move: null, isNew: false } currMoveIsNew = isNew currMoveEnd = end currMove = move + } else if (item === null) { + break } else if (item.content.constructor === ContentMove) { if (item.moved === currMove) { movedStack.push({ end: currMoveEnd, move: currMove, isNew: currMoveIsNew }) @@ -213,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.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 3c5ff268..0d0e84fa 100644 --- a/tests/y-array.tests.js +++ b/tests/y-array.tests.js @@ -472,6 +472,46 @@ export const testMove = tc => { compare(users) } +/** + * @param {t.TestCase} tc + */ +export const testMove2 = tc => { + { + // move in uninitialized type + const yarr = new Y.Array() + yarr.insert(0, [1, 2]) + yarr.move(1, 0) + // @ts-ignore + t.compare(yarr._prelimContent, [2, 1]) + } + const { array0, array1, users } = init(tc, { users: 3 }) + /** + * @type {any} + */ + let event0 = null + /** + * @type {any} + */ + let event1 = null + array0.observe(event => { + event0 = event + }) + array1.observe(event => { + event1 = event + }) + array0.insert(0, [1, 2]) + array0.move(1, 0) + t.compare(array0.toArray(), [2, 1]) + t.compare(event0.delta, [{ insert: [2] }, { retain: 1 }, { delete: 1 }]) + Y.applyUpdate(users[1], Y.encodeStateAsUpdate(users[0])) + t.compare(array1.toArray(), [2, 1]) + t.compare(event1.delta, [{ insert: [2, 1] }]) + array0.move(0, 2) + t.compare(array0.toArray(), [1, 2]) + t.compare(event0.delta, [{ delete: 1 }, { retain: 1 }, { insert: [2] }]) + compare(users) +} + /** * @param {t.TestCase} tc */ @@ -613,7 +653,7 @@ const compareTestobjects = cmp => { * @param {t.TestCase} tc */ export const testRepeatGeneratingYarrayTests6 = tc => { - compareTestobjects(applyRandomTests(tc, arrayTransactions, 3, monitorArrayTestObject)) + compareTestobjects(applyRandomTests(tc, arrayTransactions, 7, monitorArrayTestObject)) } /** From cc93f346ceb67f97dc672a24ffcffae8390f51db Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Wed, 8 Dec 2021 16:12:11 +0100 Subject: [PATCH 26/26] 13.6.0-2 --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index f279134d..7a733470 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "yjs", - "version": "13.6.0-1", + "version": "13.6.0-2", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 48b33c28..aa070c27 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "yjs", - "version": "13.6.0-1", + "version": "13.6.0-2", "description": "Shared Editing Library", "main": "./dist/yjs.cjs", "module": "./dist/yjs.mjs",