diff --git a/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeFamily.ts b/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeFamily.ts index bfa8fd9a6d1b..f94f9e8143cd 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeFamily.ts +++ b/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeFamily.ts @@ -782,6 +782,14 @@ export class ModularChangeFamily const crossFieldKeys = this.makeCrossFieldKeyTable(invertedFields, invertedNodes); + const constraintState = newConstraintState(0); + this.updateConstraintsForFields( + invertedFields, + NodeAttachState.Attached, + constraintState, + invertedNodes, + ); + return makeModularChangeset({ fieldChanges: invertedFields, nodeChanges: invertedNodes, @@ -790,8 +798,7 @@ export class ModularChangeFamily crossFieldKeys, maxId: genId.getMaxId(), revisions: revInfos, - constraintViolationCount: change.change.constraintViolationCountOnRevert, - constraintViolationCountOnRevert: change.change.constraintViolationCount, + constraintViolationCount: constraintState.violationCount, noChangeConstraint, noChangeConstraintOnRevert, destroys, @@ -939,9 +946,6 @@ export class ModularChangeFamily ); const constraintState = newConstraintState(change.constraintViolationCount ?? 0); - const revertConstraintState = newConstraintState( - change.constraintViolationCountOnRevert ?? 0, - ); let noChangeConstraint = change.noChangeConstraint; if ( @@ -956,9 +960,7 @@ export class ModularChangeFamily this.updateConstraintsForFields( rebasedFields, NodeAttachState.Attached, - NodeAttachState.Attached, constraintState, - revertConstraintState, rebasedNodes, ); @@ -971,7 +973,6 @@ export class ModularChangeFamily maxId: idState.maxId, revisions: change.revisions, constraintViolationCount: constraintState.violationCount, - constraintViolationCountOnRevert: revertConstraintState.violationCount, noChangeConstraint, noChangeConstraintOnRevert: change.noChangeConstraintOnRevert, builds: change.builds, @@ -1410,32 +1411,21 @@ export class ModularChangeFamily private updateConstraintsForFields( fields: FieldChangeMap, parentInputAttachState: NodeAttachState, - parentOutputAttachState: NodeAttachState, constraintState: ConstraintState, - revertConstraintState: ConstraintState, nodes: ChangeAtomIdBTree, ): void { for (const field of fields.values()) { const handler = getChangeHandler(this.fieldKinds, field.fieldKind); - for (const [nodeId, inputIndex, outputIndex] of handler.getNestedChanges(field.change)) { + for (const [nodeId, inputIndex, _outputIndex] of handler.getNestedChanges( + field.change, + )) { const isInputDetached = inputIndex === undefined; const inputAttachState = parentInputAttachState === NodeAttachState.Detached || isInputDetached ? NodeAttachState.Detached : NodeAttachState.Attached; - const isOutputDetached = outputIndex === undefined; - const outputAttachState = - parentOutputAttachState === NodeAttachState.Detached || isOutputDetached - ? NodeAttachState.Detached - : NodeAttachState.Attached; - this.updateConstraintsForNode( - nodeId, - inputAttachState, - outputAttachState, - nodes, - constraintState, - revertConstraintState, - ); + + this.updateConstraintsForNode(nodeId, inputAttachState, nodes, constraintState); } } } @@ -1443,10 +1433,8 @@ export class ModularChangeFamily private updateConstraintsForNode( nodeId: NodeId, inputAttachState: NodeAttachState, - outputAttachState: NodeAttachState, nodes: ChangeAtomIdBTree, constraintState: ConstraintState, - revertConstraintState: ConstraintState, ): void { const node = nodes.get([nodeId.revision, nodeId.localId]) ?? fail(0xb24 /* Unknown node ID */); @@ -1460,24 +1448,12 @@ export class ModularChangeFamily constraintState.violationCount += isNowViolated ? 1 : -1; } } - if (node.nodeExistsConstraintOnRevert !== undefined) { - const isNowViolated = outputAttachState === NodeAttachState.Detached; - if (node.nodeExistsConstraintOnRevert.violated !== isNowViolated) { - node.nodeExistsConstraintOnRevert = { - ...node.nodeExistsConstraintOnRevert, - violated: isNowViolated, - }; - revertConstraintState.violationCount += isNowViolated ? 1 : -1; - } - } if (node.fieldChanges !== undefined) { this.updateConstraintsForFields( node.fieldChanges, inputAttachState, - outputAttachState, constraintState, - revertConstraintState, nodes, ); } @@ -2003,7 +1979,6 @@ export function updateRefreshers( maxId, revisions, constraintViolationCount, - constraintViolationCountOnRevert, builds, destroys, } = change; @@ -2017,7 +1992,6 @@ export function updateRefreshers( maxId: maxId as number, revisions, constraintViolationCount, - constraintViolationCountOnRevert, builds, destroys, refreshers, @@ -2632,7 +2606,6 @@ function makeModularChangeset(props?: { maxId: number; revisions?: readonly RevisionInfo[]; constraintViolationCount?: number; - constraintViolationCountOnRevert?: number; noChangeConstraint?: NoChangeConstraint; noChangeConstraintOnRevert?: NoChangeConstraint; builds?: ChangeAtomIdBTree; @@ -2657,12 +2630,6 @@ function makeModularChangeset(props?: { if (p.constraintViolationCount !== undefined && p.constraintViolationCount > 0) { changeset.constraintViolationCount = p.constraintViolationCount; } - if ( - p.constraintViolationCountOnRevert !== undefined && - p.constraintViolationCountOnRevert > 0 - ) { - changeset.constraintViolationCountOnRevert = p.constraintViolationCountOnRevert; - } if (p.noChangeConstraint !== undefined) { changeset.noChangeConstraint = p.noChangeConstraint; } diff --git a/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeTypes.ts b/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeTypes.ts index c4fae1d0913a..c00c8102f1ae 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeTypes.ts +++ b/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeTypes.ts @@ -64,11 +64,6 @@ export interface ModularChangeset extends Readonly { readonly noChangeConstraint?: NoChangeConstraint; /** Constraint that the document must be in the same state before the revert of this change is applied as it was after this change was applied */ readonly noChangeConstraintOnRevert?: NoChangeConstraint; - /** - * The number of constraint violations that apply to the revert of the changeset. If this count is greater than 0, it will - * prevent the changeset from being reverted or undone. - */ - readonly constraintViolationCountOnRevert?: number; readonly builds?: ChangeAtomIdBTree; readonly destroys?: ChangeAtomIdBTree; readonly refreshers?: ChangeAtomIdBTree; 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..493a0759fcdf 100644 --- a/packages/dds/tree/src/test/shared-tree/editing.spec.ts +++ b/packages/dds/tree/src/test/shared-tree/editing.spec.ts @@ -3218,6 +3218,34 @@ describe("Editing", () => { stack.unsubscribe(); }); + + it("inverse node existence constraint not violated by unrelated change on moved node", () => { + const tree = makeTreeFromJsonSequence(["A", "B"]); + const branch = tree.fork(); + const stack = createTestUndoRedoStacks(tree.events); + + // Make transaction that does the following: + // 1. Moves "A" after "B". + // 2. Adds inverse constraint on existence of node "A". + branch.transaction.start(); + branch.editor.move(rootField, 0, 1, rootField, 2); + branch.editor.addNodeExistsConstraintOnRevert(rootNode2); + branch.transaction.commit(); + expectJsonTree(branch, ["B", "A"]); + + insert(tree, 2, "C"); + expectJsonTree(tree, ["A", "B", "C"]); + + tree.merge(branch); + expectJsonTree(tree, ["B", "A", "C"]); + const moveRevertible = stack.undoStack[1] ?? assert.fail("Missing undo"); + + // The inverse constraint should not be violated. + moveRevertible.revert(); + expectJsonTree(tree, ["A", "B", "C"]); + + stack.unsubscribe(); + }); }); it("Rebase over conflicted change", () => {