From 937ddc8b3dc3fed7bad48967f1b5387765ba34c1 Mon Sep 17 00:00:00 2001 From: Martin Kleppmann Date: Wed, 3 Feb 2021 16:08:27 +0000 Subject: [PATCH] Compute indexes for cursors --- backend/op_set.js | 132 +++++++++++++++++++++++++--------------- frontend/apply_patch.js | 2 +- frontend/context.js | 7 ++- frontend/cursor.js | 2 +- test/backend_test.js | 4 +- test/cursor_test.js | 71 ++++++++++++++++++--- 6 files changed, 156 insertions(+), 62 deletions(-) diff --git a/backend/op_set.js b/backend/op_set.js index 12eb4693f..b4465c376 100644 --- a/backend/op_set.js +++ b/backend/op_set.js @@ -71,6 +71,32 @@ function applyInsert(opSet, op) { .setIn(['byObject', objectId, '_insertion', opId], op) } +// Find the index of the closest visible list element that precedes the given element ID. +// Returns -1 if there is no such element. +function getPrecedingListIndex(opSet, objectId, elemId) { + const elemIds = opSet.getIn(['byObject', objectId, '_elemIds']) + + let prevId = elemId, index + while (true) { + index = -1 + prevId = getPrevious(opSet, objectId, prevId) + if (!prevId) break + index = elemIds.indexOf(prevId) + if (index >= 0) break + } + + return index +} + +// Finds the index of the list element with a given ID. If that list element is not visible +// (typically because it has been deleted), returns the index of the closest visible preceding +// element. Returns -1 if there is no such preceding element. +function getListIndex(opSet, objectId, elemId) { + const index = opSet.getIn(['byObject', objectId, '_elemIds']).indexOf(elemId) + if (index >= 0) return index + return getPrecedingListIndex(opSet, objectId, elemId) +} + function updateListElement(opSet, objectId, elemId, patch) { const ops = getFieldOps(opSet, objectId, elemId) let elemIds = opSet.getIn(['byObject', objectId, '_elemIds']) @@ -87,21 +113,9 @@ function updateListElement(opSet, objectId, elemId, patch) { } else { elemIds = elemIds.setValue(elemId, ops.first().get('value')) } - } else { if (ops.isEmpty()) return opSet // deleting a non-existent element = no-op - - // find the index of the closest preceding list element - let prevId = elemId - while (true) { - index = -1 - prevId = getPrevious(opSet, objectId, prevId) - if (!prevId) break - index = elemIds.indexOf(prevId) - if (index >= 0) break - } - - index += 1 + index = getPrecedingListIndex(opSet, objectId, elemId) + 1 elemIds = elemIds.insertIndex(index, elemId, ops.first().get('value')) if (patch) patch.edits.push({action: 'insert', index, elemId}) } @@ -141,24 +155,8 @@ function getOperationKey(op) { function applyAssign(opSet, op, patch) { const objectId = op.get('obj'), action = op.get('action'), key = getOperationKey(op) if (!opSet.get('byObject').has(objectId)) throw new RangeError(`Modification of unknown object ${objectId}`) - const type = getObjectType(opSet, objectId) - - if (patch) { - patch.objectId = patch.objectId || objectId - if (patch.objectId !== objectId) { - throw new RangeError(`objectId mismatch in patch: ${patch.objectId} != ${objectId}`) - } - if (patch.props === undefined) { - patch.props = {} - } - if (patch.props[key] === undefined) { - patch.props[key] = {} - } - - patch.type = patch.type || type - if (patch.type !== type) { - throw new RangeError(`object type mismatch in patch: ${patch.type} != ${type}`) - } + if (patch && patch.props[key] === undefined) { + patch.props[key] = {} } if (action.startsWith('make')) { @@ -194,6 +192,21 @@ function applyAssign(opSet, op, patch) { opSet = opSet.updateIn(['byObject', getChildId(old), '_inbound'], ops => ops.remove(old)) } + // Add any new cursors, and remove any overwritten cursors + for (let oldCursor of overwritten.filter(op => op.get('datatype') === 'cursor')) { + opSet = opSet.update('cursors', cursors => cursors.remove(oldCursor.get('opId'))) + } + if (op.get('datatype') === 'cursor') { + // Find the list/text object that the cursor points to by iterating over all objects + let refObjectId + for (let [listId, listObj] of opSet.get('byObject').entries()) { + if (listObj.hasIn(['_insertion', op.get('ref')])) refObjectId = listId + } + if (!refObjectId) throw new RangeError(`Cursor points at unknown list element ${op.get('ref')}`) + op = op.set('refObjectId', refObjectId) + opSet = opSet.setIn(['cursors', op.get('opId')], op) + } + if (isChildOp(op)) { opSet = opSet.updateIn(['byObject', getChildId(op), '_inbound'], Set(), ops => ops.add(op)) } @@ -204,6 +217,7 @@ function applyAssign(opSet, op, patch) { opSet = opSet.setIn(['byObject', objectId, '_keys', key], remaining) setPatchProps(opSet, objectId, key, patch) + const type = getObjectType(opSet, objectId) if (type === 'list' || type === 'text') { opSet = updateListElement(opSet, objectId, key, patch) } @@ -211,13 +225,9 @@ function applyAssign(opSet, op, patch) { } /** - * Updates `patch` with the fields required in a patch. `pathOp` is an operation - * along the path from the root to the object being modified, as returned by - * `getPath()`. Returns the sub-object representing the child identified by this - * operation. + * Mutates `patch` to contain updates for the object with ID `objectId`. */ -function initializePatch(opSet, pathOp, patch) { - const objectId = pathOp.get('obj'), opId = pathOp.get('opId'), key = getOperationKey(pathOp) +function initializePatchObject(opSet, objectId, patch) { const type = getObjectType(opSet, objectId) patch.objectId = patch.objectId || objectId patch.type = patch.type || type @@ -228,12 +238,26 @@ function initializePatch(opSet, pathOp, patch) { if (patch.type !== type) { throw new RangeError(`object type mismatch in path: ${patch.type} != ${type}`) } - setPatchProps(opSet, objectId, key, patch) +} - if (patch.props[key][opId] === undefined) { - throw new RangeError(`field ops for ${key} did not contain opId ${opId}`) +/** + * Mutates `patch` to contain the object with ID `updatedObjectId`, and the path + * from the root to that object. Returns the subpatch for that object. + */ +function initializePatch(opSet, updatedObjectId, patch) { + for (let pathOp of getPath(opSet, updatedObjectId)) { + const objectId = pathOp.get('obj'), opId = pathOp.get('opId'), key = getOperationKey(pathOp) + initializePatchObject(opSet, objectId, patch) + setPatchProps(opSet, objectId, key, patch) + if (patch.props[key][opId] === undefined) { + throw new RangeError(`field ops for ${key} did not contain opId ${opId}`) + } + patch = patch.props[key][opId] } - return patch.props[key][opId] + + initializePatchObject(opSet, updatedObjectId, patch) + if (patch.props === undefined) patch.props = {} + return patch } /** @@ -256,7 +280,9 @@ function setPatchProps(opSet, objectId, key, patch) { if (op.get('action') === 'set') { if (op.get('datatype') === 'cursor') { - patch.props[key][opId] = {elemId: op.get('ref'), datatype: 'cursor'} + const refObjectId = op.get('refObjectId'), elemId = op.get('ref') + const index = getListIndex(opSet, refObjectId, elemId) + patch.props[key][opId] = {refObjectId, elemId, index, datatype: 'cursor'} } else { patch.props[key][opId] = {value: op.get('value')} if (op.get('datatype')) { @@ -327,13 +353,7 @@ function applyOps(opSet, change, patch) { throw new RangeError(`Missing 'pred' field in operation ${op}`) } - let localPatch = patch - if (patch) { - for (let pathOp of getPath(opSet, obj)) { - localPatch = initializePatch(opSet, pathOp, localPatch) - } - } - + const localPatch = patch && initializePatch(opSet, obj, patch) const opWithId = op.merge({opId: `${startOp + index}@${actor}`}) if (insert) { opSet = applyInsert(opSet, opWithId) @@ -343,6 +363,19 @@ function applyOps(opSet, change, patch) { } opSet = applyAssign(opSet, opWithId, localPatch) }) + + // Recompute cursor indexes. This could perhaps be made more efficient by only recomputing if we + // updated the list that the cursor references; for now we just recompute them all. + if (patch) { + for (let cursor of opSet.get('cursors').values()) { + const index = getListIndex(opSet, cursor.get('refObjectId'), cursor.get('ref')) + if (index !== cursor.get('index')) { + opSet = opSet.setIn(['cursors', cursor.get('opId'), 'index'], index) + const localPatch = initializePatch(opSet, cursor.get('obj'), patch) + setPatchProps(opSet, cursor.get('obj'), getOperationKey(cursor), localPatch) + } + } + } return opSet } @@ -428,6 +461,7 @@ function init() { .set('states', Map()) .set('history', List()) .set('byObject', Map().set('_root', Map().set('_keys', Map()))) + .set('cursors', Map()) .set('hashes', Map()) .set('deps', Set()) .set('maxOp', 0) diff --git a/frontend/apply_patch.js b/frontend/apply_patch.js index 091f46cb9..63386eb81 100644 --- a/frontend/apply_patch.js +++ b/frontend/apply_patch.js @@ -22,7 +22,7 @@ function getValue(patch, object, updated) { } else if (patch.datatype === 'counter') { return new Counter(patch.value) } else if (patch.datatype === 'cursor') { - return new Cursor('', patch.index, patch.elemId) + return new Cursor(patch.refObjectId, patch.index, patch.elemId) } else if (patch.datatype !== undefined) { throw new TypeError(`Unknown datatype: ${patch.datatype}`) } else { diff --git a/frontend/context.js b/frontend/context.js index cc78cfe24..713002734 100644 --- a/frontend/context.js +++ b/frontend/context.js @@ -54,7 +54,12 @@ class Context { return {value: value.value, datatype: 'counter'} } else if (value instanceof Cursor) { - return {elemId: value.elemId, datatype: 'cursor'} + return { + refObjectId: value.objectId, + elemId: value.elemId, + index: value.index, + datatype: 'cursor' + } } else { // Nested object (map, list, text, or table) diff --git a/frontend/cursor.js b/frontend/cursor.js index 9e15f9fc3..0e943a3ea 100644 --- a/frontend/cursor.js +++ b/frontend/cursor.js @@ -17,7 +17,7 @@ class Cursor { this.objectId = object[OBJECT_ID] this.elemId = object.getElemId(index) this.index = index - } else if (typeof object == 'string' && /*typeof index === 'number' &&*/ typeof elemId === 'string') { + } else if (typeof object == 'string' && typeof index === 'number' && typeof elemId === 'string') { this.objectId = object this.elemId = elemId this.index = index diff --git a/test/backend_test.js b/test/backend_test.js index 02ddd582b..fd70710b4 100644 --- a/test/backend_test.js +++ b/test/backend_test.js @@ -271,7 +271,7 @@ describe('Automerge.Backend', () => { edits: [{action: 'insert', index: 0, elemId: `2@${actor}`}], props: {0: {[`2@${actor}`]: {value: 'fish'}}} }}, - cursor: {[`3@${actor}`]: {elemId: `2@${actor}`, datatype: 'cursor'}} + cursor: {[`3@${actor}`]: {refObjectId: `1@${actor}`, elemId: `2@${actor}`, index: 0, datatype: 'cursor'}} }} }) }) @@ -668,7 +668,7 @@ describe('Automerge.Backend', () => { edits: [{action: 'insert', index: 0, elemId: `2@${actor}`}], props: {0: {[`2@${actor}`]: {value: 'fish'}}} }}, - cursor: {[`3@${actor}`]: {elemId: `2@${actor}`, datatype: 'cursor'}} + cursor: {[`3@${actor}`]: {refObjectId: `1@${actor}`, elemId: `2@${actor}`, index: 0, datatype: 'cursor'}} }} }) }) diff --git a/test/cursor_test.js b/test/cursor_test.js index dcff52de5..59a149daa 100644 --- a/test/cursor_test.js +++ b/test/cursor_test.js @@ -8,17 +8,16 @@ describe('Automerge.Cursor', () => { doc.cursor = new Automerge.Cursor(doc.list, 2) assert.ok(doc.cursor instanceof Automerge.Cursor) assert.strictEqual(doc.cursor.elemId, `4@${Automerge.getActorId(doc)}`) + assert.strictEqual(doc.cursor.index, 2) }) assert.ok(s1.cursor instanceof Automerge.Cursor) assert.strictEqual(s1.cursor.elemId, `4@${Automerge.getActorId(s1)}`) + assert.strictEqual(s1.cursor.index, 2) let s2 = Automerge.applyChanges(Automerge.init(), Automerge.getAllChanges(s1)) assert.ok(s2.cursor instanceof Automerge.Cursor) assert.strictEqual(s2.cursor.elemId, `4@${Automerge.getActorId(s1)}`) - - let s3 = Automerge.load(Automerge.save(s1)) - assert.ok(s3.cursor instanceof Automerge.Cursor) - assert.strictEqual(s3.cursor.elemId, `4@${Automerge.getActorId(s1)}`) + assert.strictEqual(s2.cursor.index, 2) }) it('should allow a cursor on a text character', () => { @@ -27,17 +26,16 @@ describe('Automerge.Cursor', () => { doc.cursor = doc.text.getCursorAt(2) assert.ok(doc.cursor instanceof Automerge.Cursor) assert.strictEqual(doc.cursor.elemId, `4@${Automerge.getActorId(doc)}`) + assert.strictEqual(doc.cursor.index, 2) }) assert.ok(s1.cursor instanceof Automerge.Cursor) assert.strictEqual(s1.cursor.elemId, `4@${Automerge.getActorId(s1)}`) + assert.strictEqual(s1.cursor.index, 2) let s2 = Automerge.applyChanges(Automerge.init(), Automerge.getAllChanges(s1)) assert.ok(s2.cursor instanceof Automerge.Cursor) assert.strictEqual(s2.cursor.elemId, `4@${Automerge.getActorId(s1)}`) - - let s3 = Automerge.load(Automerge.save(s1)) - assert.ok(s3.cursor instanceof Automerge.Cursor) - assert.strictEqual(s3.cursor.elemId, `4@${Automerge.getActorId(s1)}`) + assert.strictEqual(s2.cursor.index, 2) }) it('should not allow an index beyond the length of the list', () => { @@ -54,4 +52,61 @@ describe('Automerge.Cursor', () => { }) }, /index out of bounds/) }) + + it('should allow a cursor to be updated', () => { + const s1 = Automerge.change(Automerge.init(), doc => { + doc.text = new Automerge.Text(['a', 'b', 'c']) + doc.cursor = doc.text.getCursorAt(1) + }) + assert.strictEqual(s1.cursor.elemId, `3@${Automerge.getActorId(s1)}`) + assert.strictEqual(s1.cursor.index, 1) + const s2 = Automerge.change(s1, doc => { + doc.cursor = doc.text.getCursorAt(2) + }) + assert.strictEqual(s2.cursor.elemId, `4@${Automerge.getActorId(s1)}`) + assert.strictEqual(s2.cursor.index, 2) + }) + + it('should update a cursor when its index changes', () => { + const s1 = Automerge.change(Automerge.init(), doc => { + doc.text = new Automerge.Text(['b', 'c']) + doc.cursor = doc.text.getCursorAt(1) + }) + assert.strictEqual(s1.cursor.elemId, `3@${Automerge.getActorId(s1)}`) + assert.strictEqual(s1.cursor.index, 1) + const s2 = Automerge.change(s1, doc => { + doc.text.insertAt(0, 'a') + }) + assert.strictEqual(s2.cursor.elemId, `3@${Automerge.getActorId(s1)}`) + assert.strictEqual(s2.cursor.index, 2) + }) + + it('should support cursors in deeply nested objects', () => { + const s1 = Automerge.change(Automerge.init(), doc => { + doc.paragraphs = [{text: new Automerge.Text(['b', 'c']), style: []}] + doc.paragraphs[0].style.push({ + format: 'bold', + from: doc.paragraphs[0].text.getCursorAt(0), + to: doc.paragraphs[0].text.getCursorAt(1) + }) + }) + assert.strictEqual(s1.paragraphs[0].style[0].from.elemId, `5@${Automerge.getActorId(s1)}`) + assert.strictEqual(s1.paragraphs[0].style[0].from.index, 0) + const s2 = Automerge.change(s1, doc => { + doc.paragraphs[0].text.insertAt(0, 'a') + }) + assert.strictEqual(s2.paragraphs[0].style[0].from.elemId, `5@${Automerge.getActorId(s1)}`) + assert.strictEqual(s2.paragraphs[0].style[0].from.index, 1) + }) + + it.skip('should restore cursors on load', () => { + let s1 = Automerge.change(Automerge.init(), doc => { + doc.text = new Automerge.Text(['a', 'b', 'c']) + doc.cursor = doc.text.getCursorAt(1) + }) + let s2 = Automerge.load(Automerge.save(s1)) + assert.ok(s2.cursor instanceof Automerge.Cursor) + assert.strictEqual(s2.cursor.elemId, `4@${Automerge.getActorId(s1)}`) + assert.strictEqual(s2.cursor.index, 1) + }) })