diff --git a/packages/dds/tree/api-report/tree.api.md b/packages/dds/tree/api-report/tree.api.md index d4bfa425d7c6..800801dcf179 100644 --- a/packages/dds/tree/api-report/tree.api.md +++ b/packages/dds/tree/api-report/tree.api.md @@ -205,7 +205,8 @@ export type ChangesetLocalId = Brand; // @internal export interface CheckoutEvents { afterBatch(): void; - revertible(revertible: Revertible): void; + newRevertible(revertible: Revertible): void; + revertibleDisposed(revertible: Revertible): void; } // @internal @@ -388,12 +389,6 @@ export type DetachedPlaceUpPath = Brand, "DetachedRa // @internal export type DetachedRangeUpPath = Brand, "DetachedRangeUpPath">; -// @internal -export enum DiscardResult { - Failure = 1, - Success = 0 -} - // @public export const disposeSymbol: unique symbol; @@ -1357,12 +1352,14 @@ export type RestrictiveReadonlyRecord = { // @internal export interface Revertible { - discard(): DiscardResult; + discard(): RevertibleResult; readonly kind: RevertibleKind; readonly origin: { readonly isLocal: boolean; }; - revert(): RevertResult; + retain(): RevertibleResult; + revert(): RevertibleResult; + readonly status: RevertibleStatus; } // @internal @@ -1374,11 +1371,17 @@ export enum RevertibleKind { } // @internal -export enum RevertResult { +export enum RevertibleResult { Failure = 1, Success = 0 } +// @internal +export enum RevertibleStatus { + Disposed = 1, + Valid = 0 +} + // @internal export type RevisionTag = SessionSpaceCompressedId | "root"; diff --git a/packages/dds/tree/src/core/index.ts b/packages/dds/tree/src/core/index.ts index d05c3a9df9c8..4d93845aa1c2 100644 --- a/packages/dds/tree/src/core/index.ts +++ b/packages/dds/tree/src/core/index.ts @@ -192,4 +192,9 @@ export { AllowedUpdateType, } from "./schema-view/index.js"; -export { Revertible, RevertibleKind, RevertResult, DiscardResult } from "./revertible/index.js"; +export { + Revertible, + RevertibleKind, + RevertibleStatus, + RevertibleResult, +} from "./revertible/index.js"; diff --git a/packages/dds/tree/src/core/revertible/index.ts b/packages/dds/tree/src/core/revertible/index.ts index 67741f4741d3..3f0c94ed72f3 100644 --- a/packages/dds/tree/src/core/revertible/index.ts +++ b/packages/dds/tree/src/core/revertible/index.ts @@ -3,4 +3,4 @@ * Licensed under the MIT License. */ -export { Revertible, RevertibleKind, RevertResult, DiscardResult } from "./revertible.js"; +export { Revertible, RevertibleKind, RevertibleStatus, RevertibleResult } from "./revertible.js"; diff --git a/packages/dds/tree/src/core/revertible/revertible.ts b/packages/dds/tree/src/core/revertible/revertible.ts index e828d8ef1aa0..6eeddbf89642 100644 --- a/packages/dds/tree/src/core/revertible/revertible.ts +++ b/packages/dds/tree/src/core/revertible/revertible.ts @@ -22,13 +22,22 @@ export interface Revertible { readonly isLocal: boolean; }; /** - * Can be called in order to revert a change. A successful revert will automatically discard resources. + * The current status of the revertible. */ - revert(): RevertResult; + readonly status: RevertibleStatus; /** - * Should be called to garbage collect any resources associated with the revertible. + * Reverts the associated change and decrements the reference count of the revertible. */ - discard(): DiscardResult; + revert(): RevertibleResult; + /** + * Increments the reference count of the revertible. + * Should be called to prevent/delay the garbage collection of the resources associated with this revertible. + */ + retain(): RevertibleResult; + /** + * Decrements the reference count of the revertible. + */ + discard(): RevertibleResult; } /** @@ -51,25 +60,25 @@ export enum RevertibleKind { } /** - * The result of a revert operation. + * The status of a {@link Revertible}. * * @internal */ -export enum RevertResult { - /** The revert was successful. */ - Success, - /** The revert failed. */ - Failure, +export enum RevertibleStatus { + /** The revertible can be reverted. */ + Valid, + /** The revertible has been disposed. Reverting it will have no effect. */ + Disposed, } /** - * The result of a discard operation. + * The result of a revert operation. * * @internal */ -export enum DiscardResult { - /** The discard was successful. */ +export enum RevertibleResult { + /** The operation was successful. */ Success, - /** The discard failed. */ + /** The operation failed. This occurs when attempting an operation on a disposed revertible */ Failure, } diff --git a/packages/dds/tree/src/index.ts b/packages/dds/tree/src/index.ts index 96343468eef8..7df9b27e47eb 100644 --- a/packages/dds/tree/src/index.ts +++ b/packages/dds/tree/src/index.ts @@ -84,8 +84,8 @@ export { MapTree, Revertible, RevertibleKind, - RevertResult, - DiscardResult, + RevertibleStatus, + RevertibleResult, forbiddenFieldKindIdentifier, StoredSchemaCollection, ErasedTreeNodeSchemaDataFormat, diff --git a/packages/dds/tree/src/shared-tree-core/branch.ts b/packages/dds/tree/src/shared-tree-core/branch.ts index 37d60f3ca0bc..22f4c614faa9 100644 --- a/packages/dds/tree/src/shared-tree-core/branch.ts +++ b/packages/dds/tree/src/shared-tree-core/branch.ts @@ -18,14 +18,13 @@ import { makeAnonChange, Revertible, RevertibleKind, - RevertResult, - DiscardResult, + RevertibleResult, + RevertibleStatus, BranchRebaseResult, rebaseChangeOverChanges, tagRollbackInverse, } from "../core/index.js"; import { EventEmitter, ISubscribable } from "../events/index.js"; -import { fail } from "../util/index.js"; import { TransactionStack } from "./transactionStack.js"; /** @@ -110,12 +109,16 @@ export interface SharedTreeBranchEvents exten // If this is not part of a transaction, emit a revertible event if (!this.isTransacting()) { - this.emit("revertible", this.makeSharedTreeRevertible(newHead, revertibleKind)); + this.emitNewRevertible(newHead, revertibleKind); } this.emit("afterChange", changeEvent); @@ -302,7 +305,7 @@ export class SharedTreeBranch exten // If this transaction is not nested, emit a revertible event if (!this.isTransacting()) { - this.emit("revertible", this.makeSharedTreeRevertible(newHead, RevertibleKind.Default)); + this.emitNewRevertible(newHead, RevertibleKind.Default); } this.emit("afterChange", changeEvent); @@ -389,59 +392,46 @@ export class SharedTreeBranch exten } } - private makeSharedTreeRevertible( - commit: GraphCommit, - kind: RevertibleKind, - ): Revertible { - this._revertibleCommits.set(commit.revision, commit); - let discarded = false; - const revertible = { + private emitNewRevertible(commit: GraphCommit, kind: RevertibleKind): void { + if (!this.hasListeners("newRevertible")) { + // No point generating revertibles if no one cares about them + return; + } + const revertible = new RevertibleRevision( kind, - origin: { - // This is currently always the case, but we may want to support reverting remote ops - isLocal: true, - }, - revert: () => { - if (discarded) { - fail("revertible has already been discarded"); - } - const revertCommit = this.revert(commit.revision, kind); - if (revertCommit !== undefined) { - revertible.discard(); - return RevertResult.Success; - } - return RevertResult.Failure; - }, - discard: () => { - if (discarded) { - fail("revertible has already been discarded"); - } - // TODO: delete the repair data from the forest - this._revertibleCommits.delete(commit.revision); - this.revertibles.delete(revertible); - discarded = true; - this.emit("revertibleDispose", commit.revision); - return DiscardResult.Success; - }, - }; + commit.revision, + this.revertRevertible.bind(this), + this.disposeRevertible.bind(this), + ); + this._revertibleCommits.set(commit.revision, commit); this.revertibles.add(revertible); - return revertible; + this.emit("newRevertible", revertible); + // Decrements the ref count for the revertible. + // This ensures that the revertible is disposed if no listener has retained it. + revertible.discard(); } - private revert( - revision: RevisionTag, - revertibleKind: RevertibleKind, - ): [change: TChange, newCommit: GraphCommit] | undefined { + private disposeRevertible(revertible: RevertibleRevision): void { + // TODO: delete the repair data from the forest + this._revertibleCommits.delete(revertible.revision); + this.revertibles.delete(revertible); + this.emit("revertibleDisposed", revertible, revertible.revision); + } + + private revertRevertible(revertible: RevertibleRevision): void { assert(!this.isTransacting(), 0x7cb /* Undo is not yet supported during transactions */); - const commit = this._revertibleCommits.get(revision); + const commit = this._revertibleCommits.get(revertible.revision); assert(commit !== undefined, 0x7cc /* expected to find a revertible commit */); - let change = this.changeFamily.rebaser.invert(tagChange(commit.change, revision), false); + let change = this.changeFamily.rebaser.invert( + tagChange(commit.change, revertible.revision), + false, + ); const headCommit = this.getHead(); // Rebase the inverted change onto any commits that occurred after the undoable commits. - if (revision !== headCommit.revision) { + if (revertible.revision !== headCommit.revision) { const pathAfterUndoable: GraphCommit[] = []; const ancestor = findCommonAncestor([commit], [headCommit, pathAfterUndoable]); assert( @@ -451,10 +441,10 @@ export class SharedTreeBranch exten change = rebaseChangeOverChanges(this.changeFamily.rebaser, change, pathAfterUndoable); } - return this.applyChange( + this.applyChange( change, this.mintRevisionTag(), - revertibleKind === RevertibleKind.Default || revertibleKind === RevertibleKind.Redo + revertible.kind === RevertibleKind.Default || revertible.kind === RevertibleKind.Redo ? RevertibleKind.Undo : RevertibleKind.Redo, ); @@ -656,3 +646,63 @@ export function onForkTransitive void ); return () => offs.forEach((off) => off()); } + +class RevertibleRevision implements Revertible { + public readonly origin: Revertible["origin"]; + + private referenceCount = 1; + + public constructor( + public readonly kind: RevertibleKind, + public readonly revision: RevisionTag, + private readonly onRevert: (revertible: RevertibleRevision) => void, + private readonly onDispose: (revertible: RevertibleRevision) => void, + ) { + this.kind = kind; + this.revision = revision; + // This is currently always the case, but we may want to support reverting remote ops + this.origin = { isLocal: true }; + } + + public get status(): RevertibleStatus { + return this.referenceCount === 0 ? RevertibleStatus.Disposed : RevertibleStatus.Valid; + } + + public revert(): RevertibleResult { + if (this.status === RevertibleStatus.Valid) { + this.onRevert(this); + this.dispose(); + return RevertibleResult.Success; + } + return RevertibleResult.Failure; + } + + public retain(): RevertibleResult { + if (this.status === RevertibleStatus.Valid) { + this.referenceCount += 1; + return RevertibleResult.Success; + } + return RevertibleResult.Failure; + } + + public discard(): RevertibleResult { + if (this.status === RevertibleStatus.Valid) { + if (this.referenceCount === 1) { + this.dispose(); + } else { + this.referenceCount -= 1; + } + return RevertibleResult.Success; + } + return RevertibleResult.Failure; + } + + private dispose(): void { + assert( + this.status === RevertibleStatus.Valid, + "Cannot dispose already disposed revertible", + ); + this.referenceCount = 0; + this.onDispose(this); + } +} diff --git a/packages/dds/tree/src/shared-tree-core/editManager.ts b/packages/dds/tree/src/shared-tree-core/editManager.ts index 02b115df9d19..e58195e91878 100644 --- a/packages/dds/tree/src/shared-tree-core/editManager.ts +++ b/packages/dds/tree/src/shared-tree-core/editManager.ts @@ -22,6 +22,7 @@ import { GraphCommit, mintCommit, rebaseChange, + Revertible, RevisionTag, } from "../core/index.js"; import { getChangeReplaceType, onForkTransitive, SharedTreeBranch } from "./branch.js"; @@ -141,7 +142,7 @@ export class EditManager< mintRevisionTag, ); - this.localBranch.on("revertibleDispose", this.onRevertibleDisposed()); + this.localBranch.on("revertibleDisposed", this.onRevertibleDisposed.bind(this)); // Track all forks of the local branch for purposes of trunk eviction. Unlike the local branch, they have // an unknown lifetime and rebase frequency, so we can not make any assumptions about which trunk commits @@ -225,19 +226,17 @@ export class EditManager< return this._oldestRevertibleSequenceId; } - private onRevertibleDisposed(): (revision: RevisionTag) => void { - return (revision: RevisionTag) => { - const metadata = this.trunkMetadata.get(revision); + private onRevertibleDisposed(revertible: Revertible, revision: RevisionTag): void { + const metadata = this.trunkMetadata.get(revision); - // if this revision hasn't been sequenced, it won't be evicted - if (metadata !== undefined) { - const { sequenceId: id } = metadata; - // if this revision corresponds with the current oldest revertible sequence id, replace it with the new oldest - if (id === this._oldestRevertibleSequenceId) { - this._oldestRevertibleSequenceId = undefined; - } + // if this revision hasn't been sequenced, it won't be evicted + if (metadata !== undefined) { + const { sequenceId: id } = metadata; + // if this revision corresponds with the current oldest revertible sequence id, replace it with the new oldest + if (id === this._oldestRevertibleSequenceId) { + this._oldestRevertibleSequenceId = undefined; } - }; + } } /** diff --git a/packages/dds/tree/src/shared-tree/treeCheckout.ts b/packages/dds/tree/src/shared-tree/treeCheckout.ts index 60550227c500..0856db2e1a15 100644 --- a/packages/dds/tree/src/shared-tree/treeCheckout.ts +++ b/packages/dds/tree/src/shared-tree/treeCheckout.ts @@ -55,14 +55,24 @@ export interface CheckoutEvents { afterBatch(): void; /** - * A revertible change has been made to this view. - * Applications which subscribe to this event are expected to revert or discard revertibles they acquire, if they so choose (failure to do so will leak memory). + * Fired when a revertible change has been made to this view. + * + * Applications which subscribe to this event are expected to revert or discard revertibles they acquire (failure to do so will leak memory). * The provided revertible is inherently bound to the view that raised the event, calling `revert` won't apply to forked views. * - * @remarks - * This event provides a {@link Revertible} object that can be used to revert the change. + * @param revertible - The revertible that can be used to revert the change. + */ + newRevertible(revertible: Revertible): void; + + /** + * Fired when a revertible is either reverted or discarded. + * + * This event can be used to maintain a list or set of active revertibles. + * @param revertible - The revertible that was disposed. + * This revertible was previously passed to the `newRevertible` event. + * Calling `discard` on this revertible is not necessary but is safe to do. */ - revertible(revertible: Revertible): void; + revertibleDisposed(revertible: Revertible): void; } /** @@ -360,13 +370,12 @@ export class TreeCheckout implements ITreeCheckoutFork { } } }); - branch.on("revertible", (revertible) => { - // if there are no listeners, discard the revertible to avoid memory leaks - if (!this.events.hasListeners("revertible")) { - revertible.discard(); - } else { - this.events.emit("revertible", revertible); - } + branch.on("newRevertible", (revertible) => { + this.events.emit("newRevertible", revertible); + }); + branch.on("revertibleDisposed", (revertible, revision) => { + // We do not expose the revision in this API + this.events.emit("revertibleDisposed", revertible); }); } diff --git a/packages/dds/tree/src/test/shared-tree-core/branch.spec.ts b/packages/dds/tree/src/test/shared-tree-core/branch.spec.ts index 181a58347f6a..dc4715c9b1e6 100644 --- a/packages/dds/tree/src/test/shared-tree-core/branch.spec.ts +++ b/packages/dds/tree/src/test/shared-tree-core/branch.spec.ts @@ -12,6 +12,9 @@ import { } from "../../shared-tree-core/index.js"; import { GraphCommit, + Revertible, + RevertibleResult, + RevertibleStatus, RevisionTag, findAncestor, findCommonAncestor, @@ -595,6 +598,238 @@ describe("Branches", () => { }); }); + describe("Revertibles", () => { + it("triggers a revertible event for changes made to the local branch", () => { + const branch = create(); + + const revertiblesCreated: Revertible[] = []; + const unsubscribe = branch.on("newRevertible", (revertible) => { + assert.equal(revertible.status, RevertibleStatus.Valid); + const retainResult = revertible.retain(); + assert.equal(retainResult, RevertibleResult.Success); + revertiblesCreated.push(revertible); + }); + + change(branch); + + assert.equal(revertiblesCreated.length, 1); + + change(branch); + + assert.equal(revertiblesCreated.length, 2); + + // Each revert also leads to the creation of a revertible event + revertiblesCreated[1].revert(); + + assert.equal(revertiblesCreated.length, 3); + + unsubscribe(); + }); + + it("triggers a revertibleDisposed event for discarded and reverted revertibles", () => { + const branch = create(); + + const revertiblesCreated: Revertible[] = []; + const unsubscribe1 = branch.on("newRevertible", (revertible) => { + assert.equal(revertible.status, RevertibleStatus.Valid); + const retainResult = revertible.retain(); + assert.equal(retainResult, RevertibleResult.Success); + revertiblesCreated.push(revertible); + }); + const revertiblesDisposed: Revertible[] = []; + const unsubscribe2 = branch.on("revertibleDisposed", (revertible) => { + assert.equal(revertible.status, RevertibleStatus.Disposed); + revertiblesDisposed.push(revertible); + }); + + change(branch); + change(branch); + + assert.equal(revertiblesCreated.length, 2); + assert.equal(revertiblesDisposed.length, 0); + + const discardResult = revertiblesCreated[0].discard(); + assert.equal(discardResult, RevertibleResult.Success); + + assert.equal(revertiblesDisposed.length, 1); + assert.equal(revertiblesDisposed[0], revertiblesCreated[0]); + + const revertResult = revertiblesCreated[1].revert(); + assert.equal(revertResult, RevertibleResult.Success); + + assert.equal(revertiblesDisposed.length, 2); + assert.equal(revertiblesDisposed[1], revertiblesCreated[1]); + + unsubscribe1(); + unsubscribe2(); + }); + + it("Non-retained Revertibles are automatically GC'ed", () => { + const branch = create(); + + const revertiblesCreated: Revertible[] = []; + const unsubscribe1 = branch.on("newRevertible", (revertible) => { + assert.equal(revertible.status, RevertibleStatus.Valid); + revertiblesCreated.push(revertible); + }); + const revertiblesDisposed: Revertible[] = []; + const unsubscribe2 = branch.on("revertibleDisposed", (revertible) => { + assert.equal(revertible.status, RevertibleStatus.Disposed); + revertiblesDisposed.push(revertible); + }); + + change(branch); + + assert.equal(revertiblesCreated.length, 1); + assert.equal(revertiblesDisposed.length, 1); + assert.equal(revertiblesDisposed[0], revertiblesCreated[0]); + assert.equal(revertiblesDisposed[0].status, RevertibleStatus.Disposed); + + unsubscribe1(); + unsubscribe2(); + }); + + it("Retained Revertibles are not GC'ed until they are reverted or discarded", () => { + const branch = create(); + + const revertiblesCreated: Revertible[] = []; + const unsubscribe1 = branch.on("newRevertible", (revertible) => { + assert.equal(revertible.status, RevertibleStatus.Valid); + const retainResult = revertible.retain(); + assert.equal(retainResult, RevertibleResult.Success); + revertiblesCreated.push(revertible); + }); + const revertiblesDisposed: Revertible[] = []; + const unsubscribe2 = branch.on("revertibleDisposed", (revertible) => { + assert.equal(revertible.status, RevertibleStatus.Disposed); + revertiblesDisposed.push(revertible); + }); + + // Make change that we will retain then revert + change(branch); + + assert.equal(revertiblesCreated.length, 1); + assert.equal(revertiblesDisposed.length, 0); + assert.equal(revertiblesCreated[0].status, RevertibleStatus.Valid); + + const revertResult = revertiblesCreated[0].revert(); + assert.equal(revertResult, RevertibleResult.Success); + + assert.equal(revertiblesCreated.length, 2); // The revert creates a new revertible + assert.equal(revertiblesDisposed.length, 1); + assert.equal(revertiblesDisposed[0], revertiblesCreated[0]); + assert.equal(revertiblesDisposed[0].status, RevertibleStatus.Disposed); + + revertiblesCreated.length = 0; + revertiblesDisposed.length = 0; + + // Make change that we will retain then discard + change(branch); + + assert.equal(revertiblesCreated.length, 1); + assert.equal(revertiblesDisposed.length, 0); + assert.equal(revertiblesCreated[0].status, RevertibleStatus.Valid); + + const discardResult = revertiblesCreated[0].discard(); + assert.equal(discardResult, RevertibleResult.Success); + + assert.equal(revertiblesCreated.length, 1); + assert.equal(revertiblesDisposed.length, 1); + assert.equal(revertiblesDisposed[0], revertiblesCreated[0]); + assert.equal(revertiblesDisposed[0].status, RevertibleStatus.Disposed); + + unsubscribe1(); + unsubscribe2(); + }); + + it("Revertibles can be retained by multiple listeners", () => { + const branch = create(); + + const revertiblesCreated1: Revertible[] = []; + const unsubscribe1 = branch.on("newRevertible", (r) => { + assert.equal(r.status, RevertibleStatus.Valid); + const retainResult = r.retain(); + assert.equal(retainResult, RevertibleResult.Success); + revertiblesCreated1.push(r); + }); + + const revertiblesCreated2: Revertible[] = []; + const unsubscribe2 = branch.on("newRevertible", (r) => { + assert.equal(r.status, RevertibleStatus.Valid); + const retainResult = r.retain(); + assert.equal(retainResult, RevertibleResult.Success); + revertiblesCreated2.push(r); + }); + + change(branch); + + assert.equal(revertiblesCreated1.length, 1); + assert.equal(revertiblesCreated2.length, 1); + assert.equal(revertiblesCreated1[0], revertiblesCreated2[0]); + const revertible = revertiblesCreated1[0]; + + const discard1Result = revertible.discard(); + assert.equal(discard1Result, RevertibleResult.Success); + assert.equal(revertible.status, RevertibleStatus.Valid); + + const discard2Result = revertible.discard(); + assert.equal(discard2Result, RevertibleResult.Success); + assert.equal(revertible.status, RevertibleStatus.Disposed); + + unsubscribe1(); + unsubscribe2(); + }); + + it("Disposed revertibles cannot be retained or discarded or reverted", () => { + const branch = create(); + + const revertiblesCreated: Revertible[] = []; + const unsubscribe = branch.on("newRevertible", (r) => { + assert.equal(r.status, RevertibleStatus.Valid); + const retainResult = r.retain(); + assert.equal(retainResult, RevertibleResult.Success); + revertiblesCreated.push(r); + }); + + change(branch); + + assert.equal(revertiblesCreated.length, 1); + const revertible = revertiblesCreated[0]; + + const discard1Result = revertible.discard(); + assert.equal(discard1Result, RevertibleResult.Success); + assert.equal(revertible.status, RevertibleStatus.Disposed); + + assert.equal(revertible.retain(), RevertibleResult.Failure); + assert.equal(revertible.discard(), RevertibleResult.Failure); + assert.equal(revertible.revert(), RevertibleResult.Failure); + + assert.equal(revertible.status, RevertibleStatus.Disposed); + unsubscribe(); + }); + + it.skip("triggers revertible events for each change merged into the local branch", () => { + const parentBranch = create(); + const childBranch = parentBranch.fork(); + + let revertibleCount = 0; + const unsubscribe = parentBranch.on("newRevertible", () => { + revertibleCount += 1; + }); + + change(childBranch); + change(childBranch); + + assert.equal(revertibleCount, 0); + + parentBranch.merge(childBranch); + + assert.equal(revertibleCount, 2); + + unsubscribe(); + }); + }); + /** Creates a new root branch */ function create( onChange?: (change: SharedTreeBranchChange) => void, diff --git a/packages/dds/tree/src/test/shared-tree-core/edit-manager/editManagerTestUtils.ts b/packages/dds/tree/src/test/shared-tree-core/edit-manager/editManagerTestUtils.ts index 5f8794b45e0e..d24d022de7b1 100644 --- a/packages/dds/tree/src/test/shared-tree-core/edit-manager/editManagerTestUtils.ts +++ b/packages/dds/tree/src/test/shared-tree-core/edit-manager/editManagerTestUtils.ts @@ -57,7 +57,7 @@ export function editManagerFactory( if (autoDiscardRevertibles === true) { // by default, discard revertibles in the edit manager tests - manager.localBranch.on("revertible", (revertible) => { + manager.localBranch.on("newRevertible", (revertible) => { revertible.discard(); }); } diff --git a/packages/dds/tree/src/test/shared-tree-core/sharedTreeCore.spec.ts b/packages/dds/tree/src/test/shared-tree-core/sharedTreeCore.spec.ts index cdb8cd50a3ed..cf71d2faeeea 100644 --- a/packages/dds/tree/src/test/shared-tree-core/sharedTreeCore.spec.ts +++ b/packages/dds/tree/src/test/shared-tree-core/sharedTreeCore.spec.ts @@ -167,7 +167,7 @@ describe("SharedTreeCore", () => { }); // discard revertibles so that the trunk can be trimmed based on the minimum sequence number - tree.getLocalBranch().on("revertible", (revertible) => { + tree.getLocalBranch().on("newRevertible", (revertible) => { revertible.discard(); }); @@ -198,7 +198,7 @@ describe("SharedTreeCore", () => { }); // discard revertibles so that the trunk can be trimmed based on the minimum sequence number - tree.getLocalBranch().on("revertible", (revertible) => { + tree.getLocalBranch().on("newRevertible", (revertible) => { revertible.discard(); }); @@ -224,7 +224,7 @@ describe("SharedTreeCore", () => { }); // discard revertibles so that the trunk can be trimmed based on the minimum sequence number - tree.getLocalBranch().on("revertible", (revertible) => { + tree.getLocalBranch().on("newRevertible", (revertible) => { revertible.discard(); }); diff --git a/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts b/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts index 0f2b1751f054..7f6b3892c8cc 100644 --- a/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts +++ b/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts @@ -119,35 +119,6 @@ describe("sharedTreeView", () => { "editStart", ]); }); - - // TODO: unskip once forking revertibles is supported - it.skip("triggers a revertible event for a changes merged into the local branch", () => { - const tree1 = checkoutWithContent({ - schema: jsonSequenceRootSchema, - initialTree: [], - }); - const branch = tree1.fork(); - - const { undoStack: undoStack1, unsubscribe: unsubscribe1 } = createTestUndoRedoStacks( - tree1.events, - ); - const { undoStack: undoStack2, unsubscribe: unsubscribe2 } = createTestUndoRedoStacks( - branch.events, - ); - - // Insert node - insertFirstNode(branch, "42"); - - assert.equal(undoStack1.length, 0); - assert.equal(undoStack2.length, 1); - - tree1.merge(branch); - assert.equal(undoStack1.length, 1); - assert.equal(undoStack2.length, 1); - - unsubscribe1(); - unsubscribe2(); - }); }); describe("Views", () => { diff --git a/packages/dds/tree/src/test/utils.ts b/packages/dds/tree/src/test/utils.ts index 21814bcb5971..f68f590b1f1c 100644 --- a/packages/dds/tree/src/test/utils.ts +++ b/packages/dds/tree/src/test/utils.ts @@ -1048,7 +1048,10 @@ export function rootFromDeltaFieldMap( } export function createTestUndoRedoStacks( - events: ISubscribable<{ revertible(type: Revertible): void }>, + events: ISubscribable<{ + newRevertible(type: Revertible): void; + revertibleDisposed(revertible: Revertible): void; + }>, ): { undoStack: Revertible[]; redoStack: Revertible[]; @@ -1057,7 +1060,8 @@ export function createTestUndoRedoStacks( const undoStack: Revertible[] = []; const redoStack: Revertible[] = []; - const unsubscribe = events.on("revertible", (revertible) => { + const unsubscribeFromNew = events.on("newRevertible", (revertible) => { + revertible.retain(); if (revertible.kind === RevertibleKind.Undo) { redoStack.push(revertible); } else { @@ -1065,6 +1069,30 @@ export function createTestUndoRedoStacks( } }); + const unsubscribeFromDisposed = events.on("revertibleDisposed", (revertible) => { + if (revertible.kind === RevertibleKind.Undo) { + const index = redoStack.indexOf(revertible); + if (index !== -1) { + redoStack.splice(index, 1); + } + } else { + const index = undoStack.indexOf(revertible); + if (index !== -1) { + undoStack.splice(index, 1); + } + } + }); + + const unsubscribe = () => { + unsubscribeFromNew(); + unsubscribeFromDisposed(); + for (const revertible of undoStack) { + revertible.discard(); + } + for (const revertible of redoStack) { + revertible.discard(); + } + }; return { undoStack, redoStack, unsubscribe }; }