From 1116f9ea9ad6f53a2ff878237271b2338d7012bf Mon Sep 17 00:00:00 2001 From: Mathias Jakobsen Date: Tue, 16 Apr 2024 09:55:22 +0100 Subject: [PATCH] [overleaf-editor-core+project-history] Clean up TrackedChangeList api (#17740) * [overleaf-editor-core+project-history] Mark TC list backing array as private * [overleaf-editor-core] Add invariant for overlapping comment ranges * [overleaf-editor-core] Assert that ranges are non-empty GitOrigin-RevId: e60a3712eba2326e0767a75a3ffc75333311c057 --- libraries/overleaf-editor-core/lib/comment.js | 7 ++- .../lib/file_data/string_file_data.js | 2 +- .../lib/file_data/tracked_change_list.js | 56 ++++++++++++------- .../lib/operation/text_operation.js | 5 +- .../overleaf-editor-core/test/comment.test.js | 11 ++-- .../test/tracked_change_list.test.js | 54 +++++++++--------- .../project-history/app/js/ChunkTranslator.js | 24 ++++---- 7 files changed, 92 insertions(+), 67 deletions(-) diff --git a/libraries/overleaf-editor-core/lib/comment.js b/libraries/overleaf-editor-core/lib/comment.js index 3c6729075c..d638064f63 100644 --- a/libraries/overleaf-editor-core/lib/comment.js +++ b/libraries/overleaf-editor-core/lib/comment.js @@ -167,7 +167,12 @@ class Comment { continue } const lastMerged = mergedRanges[mergedRanges.length - 1] - + if (lastMerged?.overlaps(range)) { + throw new Error('Ranges cannot overlap') + } + if (range.isEmpty()) { + throw new Error('Comment range cannot be empty') + } if (lastMerged?.canMerge(range)) { mergedRanges[mergedRanges.length - 1] = lastMerged.merge(range) } else { diff --git a/libraries/overleaf-editor-core/lib/file_data/string_file_data.js b/libraries/overleaf-editor-core/lib/file_data/string_file_data.js index 698012bd82..ef93b8790d 100644 --- a/libraries/overleaf-editor-core/lib/file_data/string_file_data.js +++ b/libraries/overleaf-editor-core/lib/file_data/string_file_data.js @@ -74,7 +74,7 @@ class StringFileData extends FileData { let content = '' let cursor = 0 if (opts.filterTrackedDeletes) { - for (const tc of this.trackedChanges.trackedChanges) { + for (const tc of this.trackedChanges.asSorted()) { if (tc.tracking.type !== 'delete') { continue } 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 5320df5d7a..dd402690c4 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 @@ -13,7 +13,10 @@ class TrackedChangeList { * @param {TrackedChange[]} trackedChanges */ constructor(trackedChanges) { - this.trackedChanges = trackedChanges + /** + * @type {TrackedChange[]} + */ + this._trackedChanges = trackedChanges } /** @@ -30,11 +33,20 @@ class TrackedChangeList { * @returns {TrackedChangeRawData[]} */ toRaw() { - return this.trackedChanges.map(change => change.toRaw()) + return this._trackedChanges.map(change => change.toRaw()) } get length() { - return this.trackedChanges.length + return this._trackedChanges.length + } + + /** + * @returns {readonly TrackedChange[]} + */ + asSorted() { + // NOTE: Once all code dependent on this is typed, we can just return + // _trackedChanges. + return Array.from(this._trackedChanges) } /** @@ -43,7 +55,7 @@ class TrackedChangeList { * @returns {TrackedChange[]} */ inRange(range) { - return this.trackedChanges.filter(change => range.contains(change.range)) + return this._trackedChanges.filter(change => range.contains(change.range)) } /** @@ -52,7 +64,7 @@ class TrackedChangeList { * @returns {TrackingProps | undefined} */ propsAtRange(range) { - return this.trackedChanges.find(change => change.range.contains(range)) + return this._trackedChanges.find(change => change.range.contains(range)) ?.tracking } @@ -61,7 +73,7 @@ class TrackedChangeList { * @param {Range} range */ removeInRange(range) { - this.trackedChanges = this.trackedChanges.filter( + this._trackedChanges = this._trackedChanges.filter( change => !range.contains(change.range) ) } @@ -71,7 +83,7 @@ class TrackedChangeList { * @param {TrackedChange} trackedChange */ add(trackedChange) { - this.trackedChanges.push(trackedChange) + this._trackedChanges.push(trackedChange) this._mergeRanges() } @@ -80,22 +92,28 @@ class TrackedChangeList { * @returns {void} */ _mergeRanges() { - if (this.trackedChanges.length < 2) { + if (this._trackedChanges.length < 2) { return } // ranges are non-overlapping so we can sort based on their first indices - this.trackedChanges.sort((a, b) => a.range.start - b.range.start) - const newTrackedChanges = [this.trackedChanges[0]] - for (let i = 1; i < this.trackedChanges.length; i++) { + this._trackedChanges.sort((a, b) => a.range.start - b.range.start) + const newTrackedChanges = [this._trackedChanges[0]] + for (let i = 1; i < this._trackedChanges.length; i++) { const last = newTrackedChanges[newTrackedChanges.length - 1] - const current = this.trackedChanges[i] + const current = this._trackedChanges[i] + if (last.range.overlaps(current.range)) { + throw new Error('Ranges cannot overlap') + } + if (current.range.isEmpty()) { + throw new Error('Tracked changes range cannot be empty') + } if (last.canMerge(current)) { newTrackedChanges[newTrackedChanges.length - 1] = last.merge(current) } else { newTrackedChanges.push(current) } } - this.trackedChanges = newTrackedChanges + this._trackedChanges = newTrackedChanges } /** @@ -106,7 +124,7 @@ class TrackedChangeList { */ applyInsert(cursor, insertedText, opts = {}) { const newTrackedChanges = [] - for (const trackedChange of this.trackedChanges) { + for (const trackedChange of this._trackedChanges) { if ( // If the cursor is before or at the insertion point, we need to move // the tracked change @@ -152,7 +170,7 @@ class TrackedChangeList { ) newTrackedChanges.push(newTrackedChange) } - this.trackedChanges = newTrackedChanges + this._trackedChanges = newTrackedChanges this._mergeRanges() } @@ -163,7 +181,7 @@ class TrackedChangeList { */ applyDelete(cursor, length) { const newTrackedChanges = [] - for (const trackedChange of this.trackedChanges) { + for (const trackedChange of this._trackedChanges) { const deletedRange = new Range(cursor, length) // If the tracked change is after the deletion, we need to move it if (deletedRange.contains(trackedChange.range)) { @@ -186,7 +204,7 @@ class TrackedChangeList { newTrackedChanges.push(trackedChange) } } - this.trackedChanges = newTrackedChanges + this._trackedChanges = newTrackedChanges this._mergeRanges() } @@ -202,7 +220,7 @@ class TrackedChangeList { } const newTrackedChanges = [] const retainedRange = new Range(cursor, length) - for (const trackedChange of this.trackedChanges) { + for (const trackedChange of this._trackedChanges) { if (retainedRange.contains(trackedChange.range)) { // Remove the range } else if (retainedRange.overlaps(trackedChange.range)) { @@ -250,7 +268,7 @@ class TrackedChangeList { const newTrackedChange = new TrackedChange(retainedRange, opts.tracking) newTrackedChanges.push(newTrackedChange) } - this.trackedChanges = newTrackedChanges + this._trackedChanges = newTrackedChanges 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 8b0b41a971..b2f13cb63f 100644 --- a/libraries/overleaf-editor-core/lib/operation/text_operation.js +++ b/libraries/overleaf-editor-core/lib/operation/text_operation.js @@ -31,6 +31,7 @@ const TrackingProps = require('../file_data/tracking_props') * @typedef {import('../file_data/string_file_data')} StringFileData * @typedef {import('../types').RawTextOperation} RawTextOperation * @typedef {import('../operation/scan_op').ScanOp} ScanOp + * @typedef {import('../file_data/tracked_change_list')} TrackedChangeList */ /** @@ -826,7 +827,7 @@ function getStartIndex(operation) { * @param {number} cursor * @param {number} length * @param {import('../file_data/comment_list')} commentsList - * @param {import('../file_data/tracked_change_list')} trackedChangeList + * @param {TrackedChangeList} trackedChangeList * @returns {{length: number, commentIds?: string[], tracking?: TrackingProps}[]} */ function calculateTrackingCommentSegments( @@ -853,7 +854,7 @@ function calculateTrackingCommentSegments( } } // Add tracked change boundaries - for (const trackedChange of trackedChangeList.trackedChanges) { + for (const trackedChange of trackedChangeList.asSorted()) { addBreak(trackedChange.range.start) addBreak(trackedChange.range.end) } diff --git a/libraries/overleaf-editor-core/test/comment.test.js b/libraries/overleaf-editor-core/test/comment.test.js index 9e066700ed..909de6d548 100644 --- a/libraries/overleaf-editor-core/test/comment.test.js +++ b/libraries/overleaf-editor-core/test/comment.test.js @@ -94,13 +94,10 @@ describe('Comment', function () { expect(comment.ranges).to.eql([new Range(5, 30), new Range(50, 10)]) }) - it('should ignore overlapped range', function () { - const comment = new Comment([ - new Range(5, 10), - new Range(10, 5), - new Range(50, 10), - ]) - expect(comment.ranges).to.eql([new Range(5, 10), new Range(50, 10)]) + it('should throw error when ranges overlap', function () { + expect( + () => new Comment([new Range(5, 10), new Range(10, 5), new Range(50, 10)]) + ).to.throw() }) it('should join touching ranges', function () { diff --git a/libraries/overleaf-editor-core/test/tracked_change_list.test.js b/libraries/overleaf-editor-core/test/tracked_change_list.test.js index a03113818a..04dcdf450f 100644 --- a/libraries/overleaf-editor-core/test/tracked_change_list.test.js +++ b/libraries/overleaf-editor-core/test/tracked_change_list.test.js @@ -25,7 +25,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(1) + expect(trackedChanges.length).to.equal(1) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 6 }, @@ -56,7 +56,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(1) + expect(trackedChanges.length).to.equal(1) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 16 }, @@ -87,7 +87,7 @@ describe('TrackedChangeList', function () { ts: '2023-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(1) + expect(trackedChanges.length).to.equal(1) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 6 }, @@ -118,7 +118,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(2) + expect(trackedChanges.length).to.equal(2) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 3 }, @@ -157,7 +157,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(2) + expect(trackedChanges.length).to.equal(2) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 4, length: 3 }, @@ -198,7 +198,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(2) + expect(trackedChanges.length).to.equal(2) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 3 }, @@ -237,7 +237,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(2) + expect(trackedChanges.length).to.equal(2) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 3 }, @@ -276,7 +276,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(3) + expect(trackedChanges.length).to.equal(3) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 5 }, @@ -323,7 +323,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(2) + expect(trackedChanges.length).to.equal(2) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 5 }, @@ -362,7 +362,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(3) + expect(trackedChanges.length).to.equal(3) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 4 }, @@ -409,7 +409,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(2) + expect(trackedChanges.length).to.equal(2) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 5, length: 6 }, @@ -445,7 +445,7 @@ describe('TrackedChangeList', function () { }, ]) trackedChanges.applyDelete(5, 2) - expect(trackedChanges.trackedChanges.length).to.equal(1) + expect(trackedChanges.length).to.equal(1) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 8 }, @@ -470,7 +470,7 @@ describe('TrackedChangeList', function () { }, ]) trackedChanges.applyDelete(0, 10) - expect(trackedChanges.trackedChanges.length).to.equal(0) + expect(trackedChanges.length).to.equal(0) expect(trackedChanges.toRaw()).to.deep.equal([]) }) @@ -486,7 +486,7 @@ describe('TrackedChangeList', function () { }, ]) trackedChanges.applyDelete(0, 25) - expect(trackedChanges.trackedChanges.length).to.equal(0) + expect(trackedChanges.length).to.equal(0) expect(trackedChanges.toRaw()).to.deep.equal([]) }) @@ -502,7 +502,7 @@ describe('TrackedChangeList', function () { }, ]) trackedChanges.applyDelete(1, 9) - expect(trackedChanges.trackedChanges.length).to.equal(1) + expect(trackedChanges.length).to.equal(1) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 1 }, @@ -527,7 +527,7 @@ describe('TrackedChangeList', function () { }, ]) trackedChanges.applyDelete(0, 9) - expect(trackedChanges.trackedChanges.length).to.equal(1) + expect(trackedChanges.length).to.equal(1) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 1 }, @@ -574,7 +574,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(1) + expect(trackedChanges.length).to.equal(1) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 10 }, @@ -605,7 +605,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(2) + expect(trackedChanges.length).to.equal(2) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 5 }, @@ -644,7 +644,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(3) + expect(trackedChanges.length).to.equal(3) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 5 }, @@ -691,7 +691,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(1) + expect(trackedChanges.length).to.equal(1) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 13 }, @@ -716,7 +716,7 @@ describe('TrackedChangeList', function () { }, ]) trackedChanges.applyRetain(0, 10) - expect(trackedChanges.trackedChanges.length).to.equal(1) + expect(trackedChanges.length).to.equal(1) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 10 }, @@ -741,7 +741,7 @@ describe('TrackedChangeList', function () { }, ]) trackedChanges.applyRetain(4, 1) - expect(trackedChanges.trackedChanges.length).to.equal(1) + expect(trackedChanges.length).to.equal(1) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 0, length: 10 }, @@ -772,7 +772,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(0) + expect(trackedChanges.length).to.equal(0) expect(trackedChanges.toRaw()).to.deep.equal([]) }) @@ -794,7 +794,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(0) + expect(trackedChanges.length).to.equal(0) expect(trackedChanges.toRaw()).to.deep.equal([]) }) @@ -816,7 +816,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(0) + expect(trackedChanges.length).to.equal(0) expect(trackedChanges.toRaw()).to.deep.equal([]) }) @@ -838,7 +838,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(0) + expect(trackedChanges.length).to.equal(0) expect(trackedChanges.toRaw()).to.deep.equal([]) }) @@ -860,7 +860,7 @@ describe('TrackedChangeList', function () { ts: '2024-01-01T00:00:00.000Z', }), }) - expect(trackedChanges.trackedChanges.length).to.equal(2) + expect(trackedChanges.length).to.equal(2) expect(trackedChanges.toRaw()).to.deep.equal([ { range: { pos: 4, length: 4 }, diff --git a/services/project-history/app/js/ChunkTranslator.js b/services/project-history/app/js/ChunkTranslator.js index c1b2a1d7c7..2648cc0b44 100644 --- a/services/project-history/app/js/ChunkTranslator.js +++ b/services/project-history/app/js/ChunkTranslator.js @@ -263,9 +263,9 @@ class UpdateSetBuilder { function removeTrackedDeletesFromString(content, trackedChanges) { let result = '' let cursor = 0 - const trackedDeletes = trackedChanges.trackedChanges.filter( - tc => tc.tracking.type === 'delete' - ) + const trackedDeletes = trackedChanges + .asSorted() + .filter(tc => tc.tracking.type === 'delete') for (const trackedChange of trackedDeletes) { if (cursor < trackedChange.range.start) { result += content.slice(cursor, trackedChange.range.start) @@ -455,11 +455,13 @@ class TextUpdateBuilder { // We are modifying existing tracked deletes. We need to treat removal // (type insert/none) of a tracked delete as an insertion. Similarly, any // range we introduce as a tracked deletion must be reported as a deletion. - const trackedDeletes = this.trackedChanges.trackedChanges.filter( - tc => - tc.tracking.type === 'delete' && - tc.range.overlaps(resultRetentionRange) - ) + const trackedDeletes = this.trackedChanges + .asSorted() + .filter( + tc => + tc.tracking.type === 'delete' && + tc.range.overlaps(resultRetentionRange) + ) for (const trackedDelete of trackedDeletes) { const resultTrackedDelete = trackedDelete.range @@ -554,7 +556,8 @@ class TextUpdateBuilder { const sourceDeletionRange = new Range(this.sourceCursor, deletion.length) const resultDeletionRange = new Range(this.result.length, deletion.length) - const trackedDeletes = this.trackedChanges.trackedChanges + const trackedDeletes = this.trackedChanges + .asSorted() .filter( tc => tc.tracking.type === 'delete' && @@ -604,7 +607,8 @@ class TextUpdateBuilder { if ('p' in op && typeof op.p === 'number') { // Maybe we have to move the position of the deletion to account for // tracked changes that we're hiding in the UI. - op.p -= this.trackedChanges.trackedChanges + op.p -= this.trackedChanges + .asSorted() .filter(tc => tc.tracking.type === 'delete' && tc.range.start < op.p) .map(tc => { if (tc.range.end < op.p) {