diff --git a/src/structs/Item.js b/src/structs/Item.js index 656f5e5b..874d2fff 100644 --- a/src/structs/Item.js +++ b/src/structs/Item.js @@ -22,7 +22,8 @@ import { readContentFormat, readContentType, addChangedTypeToTransaction, - UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, ContentType, ContentDeleted, StructStore, ID, AbstractType, Transaction // eslint-disable-line + isDeleted, + DeleteSet, UpdateDecoderV1, UpdateDecoderV2, UpdateEncoderV1, UpdateEncoderV2, ContentType, ContentDeleted, StructStore, ID, AbstractType, Transaction // eslint-disable-line } from '../internals.js' import * as error from 'lib0/error' @@ -125,7 +126,7 @@ export const splitItem = (transaction, leftItem, diff) => { * @param {Transaction} transaction The Yjs instance. * @param {Item} item * @param {Set} redoitems - * @param {Array} itemsToDelete + * @param {DeleteSet} itemsToDelete * * @return {Item|null} * @@ -143,42 +144,27 @@ export const redoItem = (transaction, item, redoitems, itemsToDelete) => { /** * @type {Item|null} */ - let left + let left = null /** * @type {Item|null} */ let right + // make sure that parent is redone + if (parentItem !== null && parentItem.deleted === true) { + // try to undo parent if it will be undone anyway + if (parentItem.redone === null && (!redoitems.has(parentItem) || redoItem(transaction, parentItem, redoitems, itemsToDelete) === null)) { + return null + } + while (parentItem.redone !== null) { + parentItem = getItemCleanStart(transaction, parentItem.redone) + } + } + const parentType = parentItem === null ? /** @type {AbstractType} */ (item.parent) : /** @type {ContentType} */ (parentItem.content).type + if (item.parentSub === null) { // Is an array item. Insert at the old position left = item.left right = item - } else { - // Is a map item. Insert as current value - left = item - while (left.right !== null) { - left = left.right - if (left.id.client !== ownClientID) { - // It is not possible to redo this item because it conflicts with a - // change from another client - return null - } - } - if (left.right !== null) { - left = /** @type {Item} */ (/** @type {AbstractType} */ (item.parent)._map.get(item.parentSub)) - } - right = null - } - // make sure that parent is redone - if (parentItem !== null && parentItem.deleted === true && parentItem.redone === null) { - // try to undo parent if it will be undone anyway - if (!redoitems.has(parentItem) || redoItem(transaction, parentItem, redoitems, itemsToDelete) === null) { - return null - } - } - if (parentItem !== null && parentItem.redone !== null) { - while (parentItem.redone !== null) { - parentItem = getItemCleanStart(transaction, parentItem.redone) - } // find next cloned_redo items while (left !== null) { /** @@ -210,10 +196,32 @@ export const redoItem = (transaction, item, redoitems, itemsToDelete) => { } right = right.right } - // Iterate right while right is in itemsToDelete - // If it is intended to delete right while item is redone, we can expect that item should replace right. - while (left !== null && left.right !== null && left.right !== right && itemsToDelete.findIndex(d => d === /** @type {Item} */ (left).right) >= 0) { - left = left.right + } else { + right = null + if (item.right) { + left = item + // Iterate right while right is in itemsToDelete + // If it is intended to delete right while item is redone, we can expect that item should replace right. + while (left !== null && left.right !== null && isDeleted(itemsToDelete, left.right.id)) { + left = left.right + } + // follow redone + // trace redone until parent matches + while (left !== null && left.redone !== null) { + left = getItemCleanStart(transaction, left.redone) + } + // check wether we were allowed to follow right (indicating that originally this op was replaced by another item) + if (left === null || /** @type {AbstractType} */ (left.parent)._item !== parentItem) { + // invalid parent; should never happen + return null + } + if (left && left.right !== null) { + // It is not possible to redo this item because it conflicts with a + // change from another client + return null + } + } else { + left = parentType._map.get(item.parentSub) || null } } const nextClock = getState(store, ownClientID) @@ -222,7 +230,7 @@ export const redoItem = (transaction, item, redoitems, itemsToDelete) => { nextId, left, left && left.lastId, right, right && right.id, - parentItem === null ? item.parent : /** @type {ContentType} */ (parentItem.content).type, + parentType, item.parentSub, item.content.copy() ) diff --git a/src/utils/UndoManager.js b/src/utils/UndoManager.js index 391f82ac..a7c45e9d 100644 --- a/src/utils/UndoManager.js +++ b/src/utils/UndoManager.js @@ -88,7 +88,7 @@ const popStackItem = (undoManager, stack, eventType) => { } }) itemsToRedo.forEach(struct => { - performedChange = redoItem(transaction, struct, itemsToRedo, itemsToDelete) !== null || performedChange + performedChange = redoItem(transaction, struct, itemsToRedo, stackItem.insertions) !== null || performedChange }) // We want to delete in reverse order so that children are deleted before // parents, so we have more information available when items are filtered. diff --git a/tests/undo-redo.tests.js b/tests/undo-redo.tests.js index b99df6c3..40e6a718 100644 --- a/tests/undo-redo.tests.js +++ b/tests/undo-redo.tests.js @@ -372,3 +372,158 @@ export const testUndoNestedUndoIssue = tc => { undoManager.redo() t.compare(design.toJSON(), { text: { blocks: { text: 'Something Else' } } }) } + +/** + * This issue has been reported in https://github.com/yjs/yjs/issues/355 + * + * @param {t.TestCase} tc + */ +export const testConsecutiveRedoBug = tc => { + const doc = new Y.Doc() + const yRoot = doc.getMap() + const undoMgr = new Y.UndoManager(yRoot) + + let yPoint = new Y.Map() + yPoint.set('x', 0) + yPoint.set('y', 0) + yRoot.set('a', yPoint) + undoMgr.stopCapturing() + + yPoint.set('x', 100) + yPoint.set('y', 100) + undoMgr.stopCapturing() + + yPoint.set('x', 200) + yPoint.set('y', 200) + undoMgr.stopCapturing() + + yPoint.set('x', 300) + yPoint.set('y', 300) + undoMgr.stopCapturing() + + t.compare(yPoint.toJSON(), { x: 300, y: 300 }) + + undoMgr.undo() // x=200, y=200 + t.compare(yPoint.toJSON(), { x: 200, y: 200 }) + undoMgr.undo() // x=100, y=100 + t.compare(yPoint.toJSON(), { x: 100, y: 100 }) + undoMgr.undo() // x=0, y=0 + t.compare(yPoint.toJSON(), { x: 0, y: 0 }) + undoMgr.undo() // nil + t.compare(yRoot.get('a'), undefined) + + undoMgr.redo() // x=0, y=0 + yPoint = yRoot.get('a') + + t.compare(yPoint.toJSON(), { x: 0, y: 0 }) + undoMgr.redo() // x=100, y=100 + t.compare(yPoint.toJSON(), { x: 100, y: 100 }) + undoMgr.redo() // x=200, y=200 + t.compare(yPoint.toJSON(), { x: 200, y: 200 }) + undoMgr.redo() // expected x=300, y=300, actually nil + t.compare(yPoint.toJSON(), { x: 300, y: 300 }) +} + +/** + * This issue has been reported in https://github.com/yjs/yjs/issues/304 + * + * @param {t.TestCase} tc + */ +export const testUndoXmlBug = tc => { + const origin = 'origin' + const doc = new Y.Doc() + const fragment = doc.getXmlFragment('t') + const undoManager = new Y.UndoManager(fragment, { + captureTimeout: 0, + trackedOrigins: new Set([origin]) + }) + + // create element + doc.transact(() => { + const e = new Y.XmlElement('test-node') + e.setAttribute('a', '100') + e.setAttribute('b', '0') + fragment.insert(fragment.length, [e]) + }, origin) + + // change one attribute + doc.transact(() => { + const e = fragment.get(0) + e.setAttribute('a', '200') + }, origin) + + // change both attributes + doc.transact(() => { + const e = fragment.get(0) + e.setAttribute('a', '180') + e.setAttribute('b', '50') + }, origin) + + undoManager.undo() + undoManager.undo() + undoManager.undo() + + undoManager.redo() + undoManager.redo() + undoManager.redo() + t.compare(fragment.toString(), '') +} + +/** + * This issue has been reported in https://github.com/yjs/yjs/issues/343 + * + * @param {t.TestCase} tc + */ +export const testUndoBlockBug = tc => { + const doc = new Y.Doc({ gc: false }) + const design = doc.getMap() + + const undoManager = new Y.UndoManager(design, { captureTimeout: 0 }) + + const text = new Y.Map() + + const blocks1 = new Y.Array() + const blocks1block = new Y.Map() + doc.transact(() => { + blocks1block.set('text', '1') + blocks1.push([blocks1block]) + + text.set('blocks', blocks1block) + design.set('text', text) + }) + + const blocks2 = new Y.Array() + const blocks2block = new Y.Map() + doc.transact(() => { + blocks2block.set('text', '2') + blocks2.push([blocks2block]) + text.set('blocks', blocks2block) + }) + + const blocks3 = new Y.Array() + const blocks3block = new Y.Map() + doc.transact(() => { + blocks3block.set('text', '3') + blocks3.push([blocks3block]) + text.set('blocks', blocks3block) + }) + + const blocks4 = new Y.Array() + const blocks4block = new Y.Map() + doc.transact(() => { + blocks4block.set('text', '4') + blocks4.push([blocks4block]) + text.set('blocks', blocks4block) + }) + + // {"text":{"blocks":{"text":"4"}}} + undoManager.undo() // {"text":{"blocks":{"3"}}} + undoManager.undo() // {"text":{"blocks":{"text":"2"}}} + undoManager.undo() // {"text":{"blocks":{"text":"1"}}} + undoManager.undo() // {} + undoManager.redo() // {"text":{"blocks":{"text":"1"}}} + undoManager.redo() // {"text":{"blocks":{"text":"2"}}} + undoManager.redo() // {"text":{"blocks":{"text":"3"}}} + undoManager.redo() // {"text":{}} + t.compare(design.toJSON(), { text: { blocks: { text: '4' } } }) +}