diff --git a/libraries/ranges-tracker/index.cjs b/libraries/ranges-tracker/index.cjs index 71eb40f14b..9ec9d44024 100644 --- a/libraries/ranges-tracker/index.cjs +++ b/libraries/ranges-tracker/index.cjs @@ -145,11 +145,7 @@ class RangesTracker { } removeChangeId(changeId) { - const change = this.getChange(changeId) - if (change == null) { - return - } - this._removeChange(change) + this.removeChangeIds([changeId]) } removeChangeIds(ids) { @@ -310,6 +306,7 @@ class RangesTracker { const opLength = op.i.length const opEnd = op.p + opLength const undoing = !!op.u + const fixedRemoveChange = op.fixedRemoveChange let alreadyMerged = false let previousChange = null @@ -477,7 +474,11 @@ class RangesTracker { } for (change of removeChanges) { - this._removeChange(change) + if (fixedRemoveChange) { + this._removeChange(change) + } else { + this._brokenRemoveChange(change) + } } for (change of movedChanges) { @@ -490,6 +491,7 @@ class RangesTracker { const opLength = op.d.length const opEnd = op.p + opLength const removeChanges = [] + const fixedRemoveChange = op.fixedRemoveChange let movedChanges = [] // We might end up modifying our delete op if it merges with existing deletes, or cancels out @@ -617,7 +619,11 @@ class RangesTracker { movedChanges.push(change) op.d = '' // stop it being added } else { - this._removeChange(change) + if (fixedRemoveChange) { + this._removeChange(change) + } else { + this._brokenRemoveChange(change) + } } } @@ -633,7 +639,11 @@ class RangesTracker { const results = this._scanAndMergeAdjacentUpdates() movedChanges = movedChanges.concat(results.movedChanges) for (const change of results.removeChanges) { - this._removeChange(change) + if (fixedRemoveChange) { + this._removeChange(change) + } else { + this._brokenRemoveChange(change) + } movedChanges = movedChanges.filter(c => c !== change) } } @@ -673,6 +683,11 @@ class RangesTracker { } _removeChange(change) { + this.changes = this.changes.filter(c => c !== change) + this._markAsDirty(change, 'change', 'removed') + } + + _brokenRemoveChange(change) { this.changes = this.changes.filter(c => c.id !== change.id) this._markAsDirty(change, 'change', 'removed') } diff --git a/libraries/ranges-tracker/test/unit/ranges-tracker-test.js b/libraries/ranges-tracker/test/unit/ranges-tracker-test.js index f5945c7aa1..55d73f7b85 100644 --- a/libraries/ranges-tracker/test/unit/ranges-tracker-test.js +++ b/libraries/ranges-tracker/test/unit/ranges-tracker-test.js @@ -28,6 +28,101 @@ describe('RangesTracker', function () { }) }) + describe('with duplicate tracked insert ids', function () { + beforeEach(function () { + this.comments = [] + this.changes = [ + { id: 'id1', op: { p: 10, i: 'one' } }, + { id: 'id1', op: { p: 20, i: 'two' } }, + { id: 'id1', op: { p: 30, d: 'three' } }, + ] + this.rangesTracker = new RangesTracker(this.changes, this.comments) + }) + + it("deleting one tracked insert doesn't delete the others", function () { + this.rangesTracker.applyOp({ p: 20, d: 'two', fixedRemoveChange: true }) + expect(this.rangesTracker.changes).to.deep.equal([ + this.changes[0], + this.changes[2], + ]) + }) + }) + + describe('with duplicate tracked delete ids', function () { + beforeEach(function () { + this.comments = [] + this.changes = [ + { id: 'id1', op: { p: 10, d: 'one' } }, + { id: 'id1', op: { p: 20, d: 'two' } }, + { id: 'id1', op: { p: 30, d: 'three' } }, + ] + this.rangesTracker = new RangesTracker(this.changes, this.comments) + }) + + it('deleting over tracked deletes in tracked changes mode removes the tracked deletes covered', function () { + this.rangesTracker.track_changes = true + this.rangesTracker.applyOp({ + p: 15, + d: '567890123456789012345', + fixedRemoveChange: true, + }) + expect(this.rangesTracker.changes.map(c => c.op)).to.deep.equal([ + { p: 10, d: 'one' }, + { p: 15, d: '56789two0123456789three012345' }, + ]) + }) + + it('a tracked delete between two tracked deletes joins them into a single tracked delete', function () { + this.rangesTracker.track_changes = true + this.rangesTracker.applyOp({ + p: 20, + d: '0123456789', + fixedRemoveChange: true, + }) + expect(this.rangesTracker.changes.map(c => c.op)).to.deep.equal([ + { p: 10, d: 'one' }, + { p: 20, d: 'two0123456789three' }, + ]) + }) + + it("rejecting one tracked delete doesn't reject the others", function () { + this.rangesTracker.track_changes = true + this.rangesTracker.applyOp({ + p: 20, + i: 'two', + u: true, + fixedRemoveChange: true, + }) + expect(this.rangesTracker.changes.map(c => c.op)).to.deep.equal([ + { p: 10, d: 'one' }, + { p: 33, d: 'three' }, + ]) + }) + + it("rejecting all tracked deletes doesn't introduce tracked inserts", function () { + this.rangesTracker.track_changes = true + this.rangesTracker.applyOp({ + p: 10, + i: 'one', + u: true, + fixedRemoveChange: true, + }) + this.rangesTracker.applyOp({ + p: 23, + i: 'two', + u: true, + fixedRemoveChange: true, + }) + this.rangesTracker.applyOp({ + p: 36, + i: 'three', + u: true, + fixedRemoveChange: true, + }) + expect(this.rangesTracker.changes.map(c => c.op)).to.deep.equal([]) + }) + }) + describe('with multiple tracked deletes at the same position', function () { beforeEach(function () { this.comments = [] diff --git a/services/web/frontend/js/vendor/libs/sharejs.js b/services/web/frontend/js/vendor/libs/sharejs.js index 03689e3f16..2d200a16ff 100644 --- a/services/web/frontend/js/vendor/libs/sharejs.js +++ b/services/web/frontend/js/vendor/libs/sharejs.js @@ -699,6 +699,7 @@ export const { Doc } = (() => { // TODO: This flag is temporary. It is only necessary while we change // the behaviour of tracked delete rejections in RangesTracker op.orderedRejections = true; + op.fixedRemoveChange = true; } op = [op];