From 11968a27d938b78bd862e16a5c694b0136128d30 Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Thu, 14 Mar 2024 10:51:47 -0700 Subject: [PATCH] Clean up stale local references referring to old interval endpoints (#20026) During the interval change codepath, many local references are created and updated, but local references for removed interval endpoints were never cleaned up. This was causing a bug where after removing a range of text and reverting the remove, the interval endpoints would not return to the position at which they were created. Some existing unit tests also used the stale references, and these only worked in the past because we didn't do any clean up. Those tests now use the updated references. [AB#7044](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/7044), #19352 --------- Co-authored-by: Scarlett Lee <90647596+scarlettjlee@users.noreply.github.com> --- .../dds/sequence/src/intervalCollection.ts | 4 ++ .../test/intervalCollection.detached.spec.ts | 6 +- .../dds/sequence/src/test/revertibles.spec.ts | 65 ++++++++++++++++++- 3 files changed, 71 insertions(+), 4 deletions(-) diff --git a/packages/dds/sequence/src/intervalCollection.ts b/packages/dds/sequence/src/intervalCollection.ts index 2a2d9255336f..05dcdbe914a6 100644 --- a/packages/dds/sequence/src/intervalCollection.ts +++ b/packages/dds/sequence/src/intervalCollection.ts @@ -1421,6 +1421,10 @@ export class IntervalCollection if (newInterval) { this.addPendingChange(id, serializedInterval); this.emitChange(newInterval, interval, true, false); + if (interval instanceof SequenceInterval) { + this.client?.removeLocalReferencePosition(interval.start); + this.client?.removeLocalReferencePosition(interval.end); + } } return newInterval; } diff --git a/packages/dds/sequence/src/test/intervalCollection.detached.spec.ts b/packages/dds/sequence/src/test/intervalCollection.detached.spec.ts index 66282f8e7d8d..ec5cbb0ecc85 100644 --- a/packages/dds/sequence/src/test/intervalCollection.detached.spec.ts +++ b/packages/dds/sequence/src/test/intervalCollection.detached.spec.ts @@ -88,9 +88,11 @@ describe("IntervalCollection detached", () => { describe("interval changed while detached", () => { it("slides immediately on segment removal", () => { sharedString.insertText(0, "0123"); - const interval = collection.add({ start: 0, end: 2 }); - collection.change(interval.getIntervalId(), { start: 0, end: 0 }); + const id = collection.add({ start: 0, end: 2 }).getIntervalId(); + collection.change(id, { start: 0, end: 0 }); sharedString.removeText(0, 1); + const interval = collection.getIntervalById(id); + assert(interval !== undefined, "interval should be defined"); assert.equal((interval.start.getSegment() as TextSegment)?.text, "123"); assert.equal((interval.end.getSegment() as TextSegment)?.text, "123"); }); diff --git a/packages/dds/sequence/src/test/revertibles.spec.ts b/packages/dds/sequence/src/test/revertibles.spec.ts index bc50f328e9ec..cc84f3f89ace 100644 --- a/packages/dds/sequence/src/test/revertibles.spec.ts +++ b/packages/dds/sequence/src/test/revertibles.spec.ts @@ -283,6 +283,64 @@ describe("Sequence.Revertibles with Local Edits", () => { revertSharedStringRevertibles(sharedString, revertibles.splice(0)); assertSequenceIntervals(sharedString, collection, [{ start: 1, end: 3 }]); }); + it("performs two acked interval removes and reverts to ensure interval returns to correct position", () => { + const undoRevertibles: SharedStringRevertible[] = []; + const redoRevertibles: SharedStringRevertible[] = []; + let currentRevertStack = undoRevertibles; + + sharedString.insertText(0, "123456789"); + collection.add({ start: 2, end: 2 }); + + sharedString.on("sequenceDelta", (op) => { + if (op.isLocal) { + appendSharedStringDeltaToRevertibles(sharedString, op, currentRevertStack); + } + }); + collection.on("changeInterval", (interval, previousInterval, local, op, slide) => { + if ( + slide === false && + (interval.end !== previousInterval.end || interval.start !== previousInterval.start) + ) { + appendChangeIntervalToRevertibles( + sharedString, + interval, + previousInterval, + currentRevertStack, + ); + } + }); + // remove "34" + sharedString.removeRange(2, 4); + containerRuntimeFactory.processAllMessages(); + assert.equal(sharedString.getText(), "1256789"); + assertSequenceIntervals(sharedString, collection, [{ start: 2, end: 2 }]); + + // undo to reinsert "34" + currentRevertStack = redoRevertibles; + revertSharedStringRevertibles(sharedString, undoRevertibles.splice(0)); + currentRevertStack = undoRevertibles; + containerRuntimeFactory.processAllMessages(); + assert.equal(undoRevertibles.length, 0); + assert.equal(redoRevertibles.length, 2); + + assert.equal(sharedString.getText(), "123456789"); + assertSequenceIntervals(sharedString, collection, [{ start: 2, end: 2 }]); + + // remove "5" + sharedString.removeRange(4, 5); + containerRuntimeFactory.processAllMessages(); + assert.equal(sharedString.getText(), "12346789"); + assert.equal(undoRevertibles.length, 1); + assertSequenceIntervals(sharedString, collection, [{ start: 2, end: 2 }]); + + // undo to reinsert "5" + currentRevertStack = redoRevertibles; + revertSharedStringRevertibles(sharedString, undoRevertibles.splice(0)); + currentRevertStack = undoRevertibles; + containerRuntimeFactory.processAllMessages(); + assert.equal(sharedString.getText(), "123456789"); + assertSequenceIntervals(sharedString, collection, [{ start: 2, end: 2 }]); + }); }); describe("Sequence.Revertibles with Remote Edits", () => { let sharedString: SharedString; @@ -850,6 +908,7 @@ describe("Undo/redo for string remove containing intervals", () => { }); const interval = collection.add({ start: 2, end: 4 }); + const id = interval.getIntervalId(); sharedString.removeRange(0, 6); @@ -861,15 +920,17 @@ describe("Undo/redo for string remove containing intervals", () => { assert.equal(revertibles.length, 1, "revertibles.length is not 1"); revertSharedStringRevertibles(sharedString, revertibles.splice(0)); + const updatedInterval = collection.getIntervalById(id); + assert(updatedInterval !== undefined, "updatedInterval is undefined"); assert.equal(sharedString.getText(), "hello world"); assertSequenceIntervals(sharedString, collection, [{ start: 2, end: 4 }]); assert.equal( - interval.start.getOffset(), + updatedInterval.start.getOffset(), 2, `after remove start.getOffset() is ${interval.start.getOffset()}`, ); assert.equal( - interval.end.getOffset(), + updatedInterval.end.getOffset(), 4, `after remove start.getOffset() is ${interval.end.getOffset()}`, );