diff --git a/packages/dds/tree/src/feature-libraries/default-schema/defaultEditBuilder.ts b/packages/dds/tree/src/feature-libraries/default-schema/defaultEditBuilder.ts index aad0fcf42e1c..c5a5d04f4404 100644 --- a/packages/dds/tree/src/feature-libraries/default-schema/defaultEditBuilder.ts +++ b/packages/dds/tree/src/feature-libraries/default-schema/defaultEditBuilder.ts @@ -316,6 +316,7 @@ export class DefaultEditBuilder implements ChangeFamilyEditor, IDefaultEditBuild const detachCellId = this.modularBuilder.generateId(count); const attachCellId: CellId = { localId: this.modularBuilder.generateId(count), revision }; if (compareFieldUpPaths(sourceField, destinationField)) { + // The source and destination fields are the same. const change = sequence.changeHandler.editor.move( sourceIndex, count, @@ -331,32 +332,53 @@ export class DefaultEditBuilder implements ChangeFamilyEditor, IDefaultEditBuild revision, ); } else { + // The source and destination fields are different. const detachPath = topDownPath(sourceField.parent); const attachPath = topDownPath(destinationField.parent); + /** + * The number of elements, starting from the root, that both paths have in common. + * This defines the lowest common ancestor node (LCA) of the source and destination fields. + */ const sharedDepth = getSharedPrefixLength(detachPath, attachPath); let adjustedAttachField = destinationField; - // After the above loop, `sharedDepth` is the number of elements, starting from the root, - // that both paths have in common. + // Check if the detach parent + // (the node directly above the location of detach) + // is along the attach path, + // in which case the attach path might need to be adjusted. + // This is synonymous with checking if the attach parent + // (the node directly above the location of attach) + // is either below or the same as the detach parent. + // If not, then the move does not need to be adjusted + // as the attach path will not be affected by the detach. if (sharedDepth === detachPath.length) { - const attachField = attachPath[sharedDepth]?.parentField ?? destinationField.field; - if (attachField === sourceField.field) { - // The detach occurs in an ancestor field of the field where the attach occurs. - let attachAncestorIndex = attachPath[sharedDepth]?.parentIndex ?? sourceIndex; - if (attachAncestorIndex < sourceIndex) { + const fieldOnDetachPathUnderLCA = sourceField.field; + const firstDifferentAttachAncestor = attachPath[sharedDepth]; + // Check if the detach location uses the same field as in + // the attach path. + // Note that when `firstDifferentAttachAncestor` is undefined, + // that means the parents are the same. Since earlier call + // to `compareFieldUpPaths` returned false, we know that the + // fields must be different AND thus no adjustment is needed. + if (fieldOnDetachPathUnderLCA === firstDifferentAttachAncestor?.parentField) { + // Now working at the level where detach happens along the + // attach path. + if (firstDifferentAttachAncestor.parentIndex < sourceIndex) { // The attach path runs through a node located before the detached nodes. // No need to adjust the attach path. - } else if (sourceIndex + count <= attachAncestorIndex) { + } else if (sourceIndex + count <= firstDifferentAttachAncestor.parentIndex) { // The attach path runs through a node located after the detached nodes. - // adjust the index for the node at that depth of the path, so that it is interpreted correctly + // Adjust the index for the node at that depth of the path, so that it is interpreted correctly // in the composition performed by `submitChanges`. - attachAncestorIndex -= count; - let parent: UpPath | undefined = attachPath[sharedDepth - 1]; - const parentField = attachPath[sharedDepth] ?? oob(); + // Start with path to root that remains intact. + let parent = firstDifferentAttachAncestor.parent; + // Extend with index-adjusted parent. parent = { parent, - parentIndex: attachAncestorIndex, - parentField: parentField.parentField, + parentIndex: firstDifferentAttachAncestor.parentIndex - count, + parentField: firstDifferentAttachAncestor.parentField, }; + // Extend with the rest of the attach path, which is unaffected + // apart from parentage replacements. for (let i = sharedDepth + 1; i < attachPath.length; i += 1) { parent = { ...(attachPath[i] ?? oob()), diff --git a/packages/dds/tree/src/test/feature-libraries/default-field-kinds/defaultChangeFamily.spec.ts b/packages/dds/tree/src/test/feature-libraries/default-field-kinds/defaultChangeFamily.spec.ts index f9b55323fb60..a1c547464a93 100644 --- a/packages/dds/tree/src/test/feature-libraries/default-field-kinds/defaultChangeFamily.spec.ts +++ b/packages/dds/tree/src/test/feature-libraries/default-field-kinds/defaultChangeFamily.spec.ts @@ -732,6 +732,56 @@ describe("DefaultEditBuilder", () => { assert.deepEqual(treeView, [expected]); }); + it("Can move nodes from field to a nested field under a sibling field of the same parent", () => { + const { builder, forest } = initializeEditableForest({ + type: brand(JsonAsTree.JsonObject.identifier), + fields: { + foo: [ + { type: brand(numberSchema.identifier), value: 0 }, + { type: brand(numberSchema.identifier), value: 1 }, + { type: brand(numberSchema.identifier), value: 2 }, + { type: brand(numberSchema.identifier), value: 3 }, + ], + bar: [ + { + type: brand(JsonAsTree.JsonObject.identifier), + fields: { + bar: [{ type: brand(numberSchema.identifier), value: 0 }], + }, + }, + ], + }, + }); + builder.move( + { parent: root, field: fooKey }, + 1, + 3, + { parent: root_bar0, field: barKey }, + 1, + ); + const treeView = toJsonableTreeFromForest(forest); + const expected: JsonableTree = { + type: brand(JsonAsTree.JsonObject.identifier), + fields: { + foo: [{ type: brand(numberSchema.identifier), value: 0 }], + bar: [ + { + type: brand(JsonAsTree.JsonObject.identifier), + fields: { + bar: [ + { type: brand(numberSchema.identifier), value: 0 }, + { type: brand(numberSchema.identifier), value: 1 }, + { type: brand(numberSchema.identifier), value: 2 }, + { type: brand(numberSchema.identifier), value: 3 }, + ], + }, + }, + ], + }, + }; + assert.deepEqual(treeView, [expected]); + }); + it("Can move nodes before an ancestor of the moved node", () => { const { builder, forest } = initializeEditableForest({ type: brand(JsonAsTree.JsonObject.identifier), @@ -824,6 +874,111 @@ describe("DefaultEditBuilder", () => { assert.deepEqual(treeView, [expected]); }); + it("Can move nodes from before an ancestor of the destination field", () => { + const { builder, forest } = initializeEditableForest({ + type: brand(JsonAsTree.JsonObject.identifier), + fields: { + foo: [ + { type: brand(numberSchema.identifier), value: 0 }, + { type: brand(numberSchema.identifier), value: 1 }, + { type: brand(numberSchema.identifier), value: 2 }, + { type: brand(numberSchema.identifier), value: 3 }, + { + type: brand(JsonAsTree.JsonObject.identifier), + fields: { + foo: [{ type: brand(numberSchema.identifier), value: 0 }], + }, + }, + ], + }, + }); + builder.move( + // Path to root.foo. + { parent: root, field: fooKey }, + 1, + 3, + // Path to root.foo[4].foo, which is after the moved nodes in their original location. + { + parent: { + parent: root, + parentField: fooKey, + parentIndex: 4, + }, + field: fooKey, + }, + 1, + ); + const treeView = toJsonableTreeFromForest(forest); + const expected: JsonableTree = { + type: brand(JsonAsTree.JsonObject.identifier), + fields: { + foo: [ + { type: brand(numberSchema.identifier), value: 0 }, + { + type: brand(JsonAsTree.JsonObject.identifier), + fields: { + foo: [ + { type: brand(numberSchema.identifier), value: 0 }, + { type: brand(numberSchema.identifier), value: 1 }, + { type: brand(numberSchema.identifier), value: 2 }, + { type: brand(numberSchema.identifier), value: 3 }, + ], + }, + }, + ], + }, + }; + assert.deepEqual(treeView, [expected]); + }); + + it("Can move nodes from after an ancestor of the destination field", () => { + const { builder, forest } = initializeEditableForest({ + type: brand(JsonAsTree.JsonObject.identifier), + fields: { + foo: [ + { + type: brand(JsonAsTree.JsonObject.identifier), + fields: { + foo: [{ type: brand(numberSchema.identifier), value: 0 }], + }, + }, + { type: brand(numberSchema.identifier), value: 1 }, + { type: brand(numberSchema.identifier), value: 2 }, + { type: brand(numberSchema.identifier), value: 3 }, + ], + }, + }); + builder.move( + // Path to root.foo. + { parent: root, field: fooKey }, + 1, + 3, + // Path to root.foo[0].foo, which is before the moved nodes in their original location. + { parent: root_foo0, field: fooKey }, + 1, + ); + const treeView = toJsonableTreeFromForest(forest); + const expected: JsonableTree = { + type: brand(JsonAsTree.JsonObject.identifier), + fields: { + foo: [ + { + type: brand(JsonAsTree.JsonObject.identifier), + fields: { + foo: [ + { type: brand(numberSchema.identifier), value: 0 }, + { type: brand(numberSchema.identifier), value: 1 }, + { type: brand(numberSchema.identifier), value: 2 }, + { type: brand(numberSchema.identifier), value: 3 }, + ], + }, + }, + ], + }, + }; + assert.deepEqual(treeView, [expected]); + }); + it("Errors when attempting to move a node under itself", () => { const statingState: JsonableTree = { type: brand(JsonAsTree.JsonObject.identifier), diff --git a/packages/dds/tree/src/test/shared-tree/editing.spec.ts b/packages/dds/tree/src/test/shared-tree/editing.spec.ts index ce32f2c7eb70..95f2829bfb48 100644 --- a/packages/dds/tree/src/test/shared-tree/editing.spec.ts +++ b/packages/dds/tree/src/test/shared-tree/editing.spec.ts @@ -1627,13 +1627,6 @@ describe("Editing", () => { expectJsonTree(tree, expectedState); }); - it("can move a node out from a field and into a field under a sibling", () => { - const tree = makeTreeFromJsonSequence(["A", {}]); - tree.editor.move(rootField, 0, 1, { parent: rootNode2, field: brand("foo") }, 0); - const expectedState: JsonCompatible = [{ foo: "A" }]; - expectJsonTree(tree, expectedState); - }); - it("can rebase a move over the deletion of the source parent", () => { const tree = makeTreeFromJson({ src: ["A", "B"], dst: ["C", "D"] }); const childBranch = tree.fork();