diff --git a/libraries/overleaf-editor-core/lib/file_data/tracked_change.js b/libraries/overleaf-editor-core/lib/file_data/tracked_change.js index d0e6517d0f..e789a427b0 100644 --- a/libraries/overleaf-editor-core/lib/file_data/tracked_change.js +++ b/libraries/overleaf-editor-core/lib/file_data/tracked_change.js @@ -84,6 +84,21 @@ class TrackedChange { ) ) } + + /** + * Return an equivalent tracked change whose extent is limited to the given + * range + * + * @param {Range} range + * @returns {TrackedChange | null} - the result or null if the intersection is empty + */ + intersectRange(range) { + const intersection = this.range.intersect(range) + if (intersection == null) { + return null + } + return new TrackedChange(intersection, this.tracking) + } } module.exports = TrackedChange diff --git a/libraries/overleaf-editor-core/lib/file_data/tracked_change_list.js b/libraries/overleaf-editor-core/lib/file_data/tracked_change_list.js index 263b37ab50..b302865c70 100644 --- a/libraries/overleaf-editor-core/lib/file_data/tracked_change_list.js +++ b/libraries/overleaf-editor-core/lib/file_data/tracked_change_list.js @@ -2,9 +2,11 @@ const Range = require('../range') const TrackedChange = require('./tracked_change') const TrackingProps = require('../file_data/tracking_props') +const { InsertOp, RemoveOp, RetainOp } = require('../operation/scan_op') /** * @import { TrackingDirective, TrackedChangeRawData } from "../types" + * @import TextOperation from "../operation/text_operation" */ class TrackedChangeList { @@ -58,6 +60,22 @@ class TrackedChangeList { return this._trackedChanges.filter(change => range.contains(change.range)) } + /** + * Returns tracked changes that overlap with the given range + * @param {Range} range + * @returns {TrackedChange[]} + */ + intersectRange(range) { + const changes = [] + for (const change of this._trackedChanges) { + const intersection = change.intersectRange(range) + if (intersection != null) { + changes.push(intersection) + } + } + return changes + } + /** * Returns the tracking props for a given range. * @param {Range} range @@ -89,6 +107,8 @@ class TrackedChangeList { /** * Collapses consecutive (and compatible) ranges + * + * @private * @returns {void} */ _mergeRanges() { @@ -117,12 +137,28 @@ class TrackedChangeList { } /** + * Apply an insert operation * * @param {number} cursor * @param {string} insertedText * @param {{tracking?: TrackingProps}} opts */ applyInsert(cursor, insertedText, opts = {}) { + this._applyInsert(cursor, insertedText, opts) + this._mergeRanges() + } + + /** + * Apply an insert operation + * + * This method will not merge ranges at the end + * + * @private + * @param {number} cursor + * @param {string} insertedText + * @param {{tracking?: TrackingProps}} [opts] + */ + _applyInsert(cursor, insertedText, opts = {}) { const newTrackedChanges = [] for (const trackedChange of this._trackedChanges) { if ( @@ -171,15 +207,29 @@ class TrackedChangeList { newTrackedChanges.push(newTrackedChange) } this._trackedChanges = newTrackedChanges - this._mergeRanges() } /** + * Apply a delete operation to the list of tracked changes * * @param {number} cursor * @param {number} length */ applyDelete(cursor, length) { + this._applyDelete(cursor, length) + this._mergeRanges() + } + + /** + * Apply a delete operation to the list of tracked changes + * + * This method will not merge ranges at the end + * + * @private + * @param {number} cursor + * @param {number} length + */ + _applyDelete(cursor, length) { const newTrackedChanges = [] for (const trackedChange of this._trackedChanges) { const deletedRange = new Range(cursor, length) @@ -205,15 +255,31 @@ class TrackedChangeList { } } this._trackedChanges = newTrackedChanges + } + + /** + * Apply a retain operation to the list of tracked changes + * + * @param {number} cursor + * @param {number} length + * @param {{tracking?: TrackingDirective}} [opts] + */ + applyRetain(cursor, length, opts = {}) { + this._applyRetain(cursor, length, opts) this._mergeRanges() } /** + * Apply a retain operation to the list of tracked changes + * + * This method will not merge ranges at the end + * + * @private * @param {number} cursor * @param {number} length * @param {{tracking?: TrackingDirective}} opts */ - applyRetain(cursor, length, opts = {}) { + _applyRetain(cursor, length, opts = {}) { // If there's no tracking info, leave everything as-is if (!opts.tracking) { return @@ -269,6 +335,31 @@ class TrackedChangeList { newTrackedChanges.push(newTrackedChange) } this._trackedChanges = newTrackedChanges + } + + /** + * Apply a text operation to the list of tracked changes + * + * Ranges are merged only once at the end, for performance and to avoid + * problematic edge cases where intermediate ranges get incorrectly merged. + * + * @param {TextOperation} operation + */ + applyTextOperation(operation) { + // this cursor tracks the destination document that gets modified as + // operations are applied to it. + let cursor = 0 + for (const op of operation.ops) { + if (op instanceof InsertOp) { + this._applyInsert(cursor, op.insertion, { tracking: op.tracking }) + cursor += op.insertion.length + } else if (op instanceof RemoveOp) { + this._applyDelete(cursor, op.length) + } else if (op instanceof RetainOp) { + this._applyRetain(cursor, op.length, { tracking: op.tracking }) + cursor += op.length + } + } this._mergeRanges() } } diff --git a/libraries/overleaf-editor-core/lib/operation/text_operation.js b/libraries/overleaf-editor-core/lib/operation/text_operation.js index 148570fa42..61c7f124b4 100644 --- a/libraries/overleaf-editor-core/lib/operation/text_operation.js +++ b/libraries/overleaf-editor-core/lib/operation/text_operation.js @@ -314,25 +314,18 @@ class TextOperation extends EditOperation { str ) } - file.trackedChanges.applyRetain(result.length, op.length, { - tracking: op.tracking, - }) result += str.slice(inputCursor, inputCursor + op.length) inputCursor += op.length } else if (op instanceof InsertOp) { if (containsNonBmpChars(op.insertion)) { throw new InvalidInsertionError(str, op.toJSON()) } - file.trackedChanges.applyInsert(result.length, op.insertion, { - tracking: op.tracking, - }) file.comments.applyInsert( new Range(result.length, op.insertion.length), { commentIds: op.commentIds } ) result += op.insertion } else if (op instanceof RemoveOp) { - file.trackedChanges.applyDelete(result.length, op.length) file.comments.applyDelete(new Range(result.length, op.length)) inputCursor += op.length } else { @@ -352,6 +345,8 @@ class TextOperation extends EditOperation { throw new TextOperation.TooLongError(operation, result.length) } + file.trackedChanges.applyTextOperation(this) + file.content = result } @@ -400,44 +395,36 @@ class TextOperation extends EditOperation { for (let i = 0, l = ops.length; i < l; i++) { const op = ops[i] if (op instanceof RetainOp) { - // Where we need to end up after the retains - const target = strIndex + op.length - // A previous retain could have overriden some tracking info. Now we - // need to restore it. - const previousRanges = previousState.trackedChanges.inRange( - new Range(strIndex, op.length) - ) - - let removeTrackingInfoIfNeeded if (op.tracking) { - removeTrackingInfoIfNeeded = new ClearTrackingProps() - } + // Where we need to end up after the retains + const target = strIndex + op.length + // A previous retain could have overriden some tracking info. Now we + // need to restore it. + const previousChanges = previousState.trackedChanges.intersectRange( + new Range(strIndex, op.length) + ) - for (const trackedChange of previousRanges) { - if (strIndex < trackedChange.range.start) { - inverse.retain(trackedChange.range.start - strIndex, { - tracking: removeTrackingInfoIfNeeded, + for (const change of previousChanges) { + if (strIndex < change.range.start) { + inverse.retain(change.range.start - strIndex, { + tracking: new ClearTrackingProps(), + }) + strIndex = change.range.start + } + inverse.retain(change.range.length, { + tracking: change.tracking, }) - strIndex = trackedChange.range.start + strIndex += change.range.length } - if (trackedChange.range.end < strIndex + op.length) { - inverse.retain(trackedChange.range.length, { - tracking: trackedChange.tracking, + if (strIndex < target) { + inverse.retain(target - strIndex, { + tracking: new ClearTrackingProps(), }) - strIndex = trackedChange.range.end + strIndex = target } - if (trackedChange.range.end !== strIndex) { - // No need to split the range at the end - const [left] = trackedChange.range.splitAt(strIndex) - inverse.retain(left.length, { tracking: trackedChange.tracking }) - strIndex = left.end - } - } - if (strIndex < target) { - inverse.retain(target - strIndex, { - tracking: removeTrackingInfoIfNeeded, - }) - strIndex = target + } else { + inverse.retain(op.length) + strIndex += op.length } } else if (op instanceof InsertOp) { inverse.remove(op.insertion.length) diff --git a/libraries/overleaf-editor-core/lib/range.js b/libraries/overleaf-editor-core/lib/range.js index bc47632f92..b3fb2bd78b 100644 --- a/libraries/overleaf-editor-core/lib/range.js +++ b/libraries/overleaf-editor-core/lib/range.js @@ -86,10 +86,32 @@ class Range { } /** - * @param {Range} range + * Does this range overlap another range? + * + * Overlapping means that the two ranges have at least one character in common + * + * @param {Range} other - the other range */ - overlaps(range) { - return this.start < range.end && this.end > range.start + overlaps(other) { + return this.start < other.end && this.end > other.start + } + + /** + * Does this range overlap the start of another range? + * + * @param {Range} other - the other range + */ + overlapsStart(other) { + return this.start <= other.start && this.end > other.start + } + + /** + * Does this range overlap the end of another range? + * + * @param {Range} other - the other range + */ + overlapsEnd(other) { + return this.start < other.end && this.end >= other.end } /** @@ -227,6 +249,26 @@ class Range { ) return [rangeUpToCursor, rangeAfterCursor] } + + /** + * Returns the intersection of this range with another range + * + * @param {Range} other - the other range + * @return {Range | null} the intersection or null if the intersection is empty + */ + intersect(other) { + if (this.contains(other)) { + return other + } else if (other.contains(this)) { + return this + } else if (other.overlapsStart(this)) { + return new Range(this.pos, other.end - this.start) + } else if (other.overlapsEnd(this)) { + return new Range(other.pos, this.end - other.start) + } else { + return null + } + } } module.exports = Range diff --git a/libraries/overleaf-editor-core/test/range.test.js b/libraries/overleaf-editor-core/test/range.test.js index daad8fd6ed..9a048d5c03 100644 --- a/libraries/overleaf-editor-core/test/range.test.js +++ b/libraries/overleaf-editor-core/test/range.test.js @@ -1,4 +1,3 @@ -// @ts-check 'use strict' const { expect } = require('chai') @@ -449,4 +448,44 @@ describe('Range', function () { expect(() => range.insertAt(16, 3)).to.throw() }) }) + + describe('intersect', function () { + it('should handle partially overlapping ranges', function () { + const range1 = new Range(5, 10) + const range2 = new Range(3, 6) + const intersection1 = range1.intersect(range2) + expect(intersection1.pos).to.equal(5) + expect(intersection1.length).to.equal(4) + const intersection2 = range2.intersect(range1) + expect(intersection2.pos).to.equal(5) + expect(intersection2.length).to.equal(4) + }) + + it('should intersect with itself', function () { + const range = new Range(5, 10) + const intersection = range.intersect(range) + expect(intersection.pos).to.equal(5) + expect(intersection.length).to.equal(10) + }) + + it('should handle nested ranges', function () { + const range1 = new Range(5, 10) + const range2 = new Range(7, 2) + const intersection1 = range1.intersect(range2) + expect(intersection1.pos).to.equal(7) + expect(intersection1.length).to.equal(2) + const intersection2 = range2.intersect(range1) + expect(intersection2.pos).to.equal(7) + expect(intersection2.length).to.equal(2) + }) + + it('should handle disconnected ranges', function () { + const range1 = new Range(5, 10) + const range2 = new Range(20, 30) + const intersection1 = range1.intersect(range2) + expect(intersection1).to.be.null + const intersection2 = range2.intersect(range1) + expect(intersection2).to.be.null + }) + }) }) diff --git a/libraries/overleaf-editor-core/test/text_operation.test.js b/libraries/overleaf-editor-core/test/text_operation.test.js index fa9bc62dc3..43b8c707a6 100644 --- a/libraries/overleaf-editor-core/test/text_operation.test.js +++ b/libraries/overleaf-editor-core/test/text_operation.test.js @@ -322,6 +322,47 @@ describe('TextOperation', function () { new TextOperation().retain(4).remove(4).retain(3) ) }) + + it('undoing a tracked delete restores the tracked changes', function () { + expectInverseToLeadToInitialState( + new StringFileData( + 'the quick brown fox jumps over the lazy dog', + undefined, + [ + { + range: { pos: 5, length: 5 }, + tracking: { + ts: '2023-01-01T00:00:00.000Z', + type: 'insert', + userId: 'user1', + }, + }, + { + range: { pos: 12, length: 3 }, + tracking: { + ts: '2023-01-01T00:00:00.000Z', + type: 'delete', + userId: 'user1', + }, + }, + { + range: { pos: 18, length: 5 }, + tracking: { + ts: '2023-01-01T00:00:00.000Z', + type: 'insert', + userId: 'user1', + }, + }, + ] + ), + new TextOperation() + .retain(7) + .retain(13, { + tracking: new TrackingProps('delete', 'user1', new Date()), + }) + .retain(23) + ) + }) }) describe('compose', function () {