diff --git a/libraries/ranges-tracker/index.cjs b/libraries/ranges-tracker/index.cjs index 948130b714..8d3cb840c0 100644 --- a/libraries/ranges-tracker/index.cjs +++ b/libraries/ranges-tracker/index.cjs @@ -313,11 +313,10 @@ class RangesTracker { let alreadyMerged = false let previousChange = null - let trackedDeleteRejected = false const movedChanges = [] const removeChanges = [] const newChanges = [] - const trackedDeletesAtOpPosition = [] + for (let i = 0; i < this.changes.length; i++) { change = this.changes[i] const changeStart = change.op.p @@ -328,22 +327,13 @@ class RangesTracker { change.op.p += opLength movedChanges.push(change) } else if (opStart === changeStart) { - // Keep track of tracked deletes that are at the same position as the - // insert. At the end of the loop, if we didn't find a tracked delete - // to reject, we'll move these deletes after the insert. - trackedDeletesAtOpPosition.push(change) - - if (trackedDeleteRejected && op.orderedRejections) { - // We rejected a tracked delete. Move the remaining tracked deletes after the insert - change.op.p += opLength - movedChanges.push(change) - } else if ( + // If we are undoing, then we want to cancel any existing delete ranges if we can. + // Check if the insert matches the start of the delete, and just remove it from the delete instead if so. + if ( undoing && change.op.d.length >= op.i.length && change.op.d.slice(0, op.i.length) === op.i ) { - // If we are undoing, then we want to reject any existing tracked delete if we can. - // Check if the insert matches the start of the delete, and just remove it from the delete instead if so. change.op.d = change.op.d.slice(op.i.length) change.op.p += op.i.length if (change.op.d === '') { @@ -352,7 +342,9 @@ class RangesTracker { movedChanges.push(change) } alreadyMerged = true - trackedDeleteRejected = true + } else { + change.op.p += opLength + movedChanges.push(change) } } } else if (change.op.i != null) { @@ -457,15 +449,6 @@ class RangesTracker { previousChange = change } - if (!trackedDeleteRejected || !op.orderedRejections) { - // We didn't reject any tracked delete. Move all existing tracked deletes - // at the insert position after the insert - for (const change of trackedDeletesAtOpPosition) { - change.op.p += opLength - movedChanges.push(change) - } - } - if (this.track_changes && !alreadyMerged) { this._addOp(op, metadata) } @@ -641,13 +624,9 @@ class RangesTracker { } _addOp(op, metadata) { - // Don't take a reference to the existing op since we'll modify this in place with future changes - op = this._clone(op) - // TODO: Remove this when the orderedRejections transition is over - delete op.orderedRejections const change = { id: this.newId(), - op, + op: this._clone(op), // Don't take a reference to the existing op since we'll modify this in place with future changes metadata: this._clone(metadata), } this.changes.push(change) diff --git a/libraries/ranges-tracker/test/unit/ranges-tracker-test.js b/libraries/ranges-tracker/test/unit/ranges-tracker-test.js index 77b6715542..75d7024d09 100644 --- a/libraries/ranges-tracker/test/unit/ranges-tracker-test.js +++ b/libraries/ranges-tracker/test/unit/ranges-tracker-test.js @@ -4,7 +4,6 @@ const RangesTracker = require('../..') describe('RangesTracker', function () { describe('with duplicate change ids', function () { beforeEach(function () { - this.comments = [] this.changes = [ { id: 'id1', op: { p: 1, i: 'hello' } }, { id: 'id2', op: { p: 10, i: 'world' } }, @@ -27,116 +26,4 @@ describe('RangesTracker', function () { expect(this.rangesTracker.changes).to.deep.equal([this.changes[2]]) }) }) - - describe('with multiple tracked deletes at the same position', function () { - beforeEach(function () { - this.comments = [] - this.changes = [ - { id: 'id1', op: { p: 33, d: 'before' } }, - { id: 'id2', op: { p: 50, d: 'right before' } }, - { id: 'id3', op: { p: 50, d: 'this one' } }, - { id: 'id4', op: { p: 50, d: 'right after' } }, - { id: 'id5', op: { p: 75, d: 'long after' } }, - ] - this.rangesTracker = new RangesTracker(this.changes, this.comments) - }) - - describe('with the orderedRejections flag', function () { - it('preserves the text order when rejecting changes', function () { - this.rangesTracker.applyOp( - { p: 50, i: 'this one', u: true, orderedRejections: true }, - { user_id: 'user-id' } - ) - expect(this.rangesTracker.changes).to.deep.equal([ - { id: 'id1', op: { p: 33, d: 'before' } }, - { id: 'id2', op: { p: 50, d: 'right before' } }, - { id: 'id4', op: { p: 58, d: 'right after' } }, - { id: 'id5', op: { p: 83, d: 'long after' } }, - ]) - }) - - it('moves all tracked deletes after the insert if not rejecting changes', function () { - this.rangesTracker.applyOp( - { p: 50, i: 'some other text', u: true, orderedRejections: true }, - { user_id: 'user-id' } - ) - expect(this.rangesTracker.changes).to.deep.equal([ - { id: 'id1', op: { p: 33, d: 'before' } }, - { id: 'id2', op: { p: 65, d: 'right before' } }, - { id: 'id3', op: { p: 65, d: 'this one' } }, - { id: 'id4', op: { p: 65, d: 'right after' } }, - { id: 'id5', op: { p: 90, d: 'long after' } }, - ]) - }) - }) - - describe('without the orderedRejections flag', function () { - it('puts the insert before tracked deletes when rejecting changes', function () { - this.rangesTracker.applyOp( - { p: 50, i: 'this one', u: true }, - { user_id: 'user-id' } - ) - expect(this.rangesTracker.changes).to.deep.equal([ - { id: 'id1', op: { p: 33, d: 'before' } }, - { id: 'id2', op: { p: 58, d: 'right before' } }, - { id: 'id4', op: { p: 58, d: 'right after' } }, - { id: 'id5', op: { p: 83, d: 'long after' } }, - ]) - }) - - it('moves all tracked deletes after the insert if not rejecting changes', function () { - this.rangesTracker.applyOp( - { p: 50, i: 'some other text', u: true }, - { user_id: 'user-id' } - ) - expect(this.rangesTracker.changes).to.deep.equal([ - { id: 'id1', op: { p: 33, d: 'before' } }, - { id: 'id2', op: { p: 65, d: 'right before' } }, - { id: 'id3', op: { p: 65, d: 'this one' } }, - { id: 'id4', op: { p: 65, d: 'right after' } }, - { id: 'id5', op: { p: 90, d: 'long after' } }, - ]) - }) - }) - }) - - describe('with multiple tracked deletes at the same position with the same content', function () { - beforeEach(function () { - this.comments = [] - this.changes = [ - { id: 'id1', op: { p: 10, d: 'cat' } }, - { id: 'id2', op: { p: 10, d: 'giraffe' } }, - { id: 'id3', op: { p: 10, d: 'cat' } }, - { id: 'id4', op: { p: 10, d: 'giraffe' } }, - ] - this.rangesTracker = new RangesTracker(this.changes, this.comments) - }) - - describe('with the orderedRejections flag', function () { - it('removes only the first matching tracked delete', function () { - this.rangesTracker.applyOp( - { p: 10, i: 'giraffe', u: true, orderedRejections: true }, - { user_id: 'user-id' } - ) - expect(this.rangesTracker.changes).to.deep.equal([ - { id: 'id1', op: { p: 10, d: 'cat' } }, - { id: 'id3', op: { p: 17, d: 'cat' } }, - { id: 'id4', op: { p: 17, d: 'giraffe' } }, - ]) - }) - }) - - describe('without the orderedRejections flag', function () { - it('removes all matching tracked delete', function () { - this.rangesTracker.applyOp( - { p: 10, i: 'giraffe', u: true }, - { user_id: 'user-id' } - ) - expect(this.rangesTracker.changes).to.deep.equal([ - { id: 'id1', op: { p: 17, d: 'cat' } }, - { id: 'id3', op: { p: 17, d: 'cat' } }, - ]) - }) - }) - }) }) diff --git a/services/document-updater/app/js/RangesManager.js b/services/document-updater/app/js/RangesManager.js index 37006ac1d0..9b8a50c526 100644 --- a/services/document-updater/app/js/RangesManager.js +++ b/services/document-updater/app/js/RangesManager.js @@ -352,12 +352,6 @@ function getHistoryOpForInsert(op, comments, changes) { } } - // If it's determined that the op is a tracked delete rejection, we have to - // calculate its proper history position. If multiple tracked deletes are - // found at the same position as the insert, the tracked deletes that come - // before the tracked delete that was actually rejected offset the history - // position. - let trackedDeleteRejectionOffset = 0 for (const change of changes) { if (!isDelete(change.op)) { // We're only interested in tracked deletes @@ -368,25 +362,14 @@ function getHistoryOpForInsert(op, comments, changes) { // Tracked delete is before the op. Move the op forward. hpos += change.op.d.length } else if (change.op.p === op.p) { - // Tracked delete is at the same position as the op. + // Tracked delete is at the same position as the op. The insert comes before + // the tracked delete so it doesn't move. if (op.u && change.op.d.startsWith(op.i)) { // We're undoing and the insert matches the start of the tracked // delete. RangesManager treats this as a tracked delete rejection. We // will note this in the op so that project-history can take the // appropriate action. trackedDeleteRejection = true - - // The history must be updated to take into account all preceding - // tracked deletes at the same position - hpos += trackedDeleteRejectionOffset - - // No need to continue. All subsequent tracked deletes are after the - // insert. - break - } else { - // This tracked delete does not match the insert. Note its length in - // case we find a tracked delete that matches later. - trackedDeleteRejectionOffset += change.op.d.length } } else { // Tracked delete is after the insert. Tracked deletes are ordered, so diff --git a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js index 4053aafb01..ec1c8703e9 100644 --- a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js +++ b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js @@ -323,44 +323,6 @@ describe('RangesManager', function () { }) }) - describe('tracked delete rejections with multiple tracked deletes at the same position', function () { - beforeEach(function () { - // original text is "one [two ][three ][four ]five" - // [] denotes tracked deletes - this.ranges = { - changes: makeRanges([ - { d: 'two ', p: 4 }, - { d: 'three ', p: 4 }, - { d: 'four ', p: 4 }, - ]), - } - this.updates = makeUpdates([{ i: 'three ', p: 4, u: true }]) - this.newDocLines = ['one three five'] - this.result = this.RangesManager.applyUpdate( - this.project_id, - this.doc_id, - this.ranges, - this.updates, - this.newDocLines, - { historyRangesSupport: true } - ) - }) - - it('should insert the text at the right history position', function () { - expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ - [ - { - i: 'three ', - p: 4, - hpos: 8, - u: true, - trackedDeleteRejection: true, - }, - ], - ]) - }) - }) - describe('deletes over tracked changes', function () { beforeEach(function () { // original text is "on[1]e [22](three) f[333]ou[4444]r [55555]five" diff --git a/services/web/frontend/js/vendor/libs/sharejs.js b/services/web/frontend/js/vendor/libs/sharejs.js index 0765813a18..6b9ea123d7 100644 --- a/services/web/frontend/js/vendor/libs/sharejs.js +++ b/services/web/frontend/js/vendor/libs/sharejs.js @@ -695,9 +695,6 @@ export const { Doc } = (() => { var op = { p: pos, i: text }; if (fromUndo) { op.u = true; - // TODO: This flag is temporary. It is only necessary while we change - // the behaviour of tracked delete rejections in RangesTracker - op.orderedRejections = true; } op = [op];