From 8b571bae9cfc0be986fb7106c13f333e158ec56b Mon Sep 17 00:00:00 2001 From: Domagoj Kriskovic Date: Tue, 13 Feb 2024 16:08:54 +0100 Subject: [PATCH] [overleaf-editor-core] Make range methods immutable (#16890) * make range class immutable * rename variable * use newRanges * range readonly props * skrinkBy test * Fix range shrinking bug GitOrigin-RevId: ea9a568b28f53e74dec4c500be3d5dba65abf0ad --- .../lib/file_data/comment.js | 41 +++++---- .../lib/file_data/range.js | 42 +++++---- .../lib/file_data/tracked_change.js | 2 +- .../lib/file_data/tracked_change_list.js | 13 +-- .../overleaf-editor-core/test/range.test.js | 91 +++++++++++-------- 5 files changed, 110 insertions(+), 79 deletions(-) diff --git a/libraries/overleaf-editor-core/lib/file_data/comment.js b/libraries/overleaf-editor-core/lib/file_data/comment.js index 44a87520f5..2c8483a199 100644 --- a/libraries/overleaf-editor-core/lib/file_data/comment.js +++ b/libraries/overleaf-editor-core/lib/file_data/comment.js @@ -45,30 +45,32 @@ class Comment { */ applyInsert(cursor, length, extendComment = false) { let existingRangeExtended = false - const splittedRanges = [] + const newRanges = [] for (const commentRange of this.ranges) { if (cursor === commentRange.end) { // insert right after the comment if (extendComment) { - commentRange.extendBy(length) + newRanges.push(commentRange.extendBy(length)) existingRangeExtended = true + } else { + newRanges.push(commentRange) } } else if (cursor === commentRange.start) { // insert at the start of the comment if (extendComment) { - commentRange.extendBy(length) + newRanges.push(commentRange.extendBy(length)) existingRangeExtended = true } else { - commentRange.moveBy(length) + newRanges.push(commentRange.moveBy(length)) } } else if (commentRange.startIsAfter(cursor)) { // insert before the comment - commentRange.moveBy(length) + newRanges.push(commentRange.moveBy(length)) } else if (commentRange.containsCursor(cursor)) { // insert is inside the comment if (extendComment) { - commentRange.extendBy(length) + newRanges.push(commentRange.extendBy(length)) existingRangeExtended = true } else { const [rangeUpToCursor, , rangeAfterCursor] = commentRange.insertAt( @@ -77,17 +79,17 @@ class Comment { ) // use current commentRange for the part before the cursor - commentRange.length = rangeUpToCursor.length + newRanges.push(new Range(commentRange.pos, rangeUpToCursor.length)) // add the part after the cursor as a new range - splittedRanges.push(rangeAfterCursor) + newRanges.push(rangeAfterCursor) } + } else { + // insert is after the comment + newRanges.push(commentRange) } } - // add the splitted ranges - for (const range of splittedRanges) { - this.addRange(range) - } + this.ranges = newRanges // if the insert is not inside any range, add a new range if (extendComment && !existingRangeExtended) { @@ -100,14 +102,19 @@ class Comment { * @param {Range} deletedRange */ applyDelete(deletedRange) { + const newRanges = [] + for (const commentRange of this.ranges) { if (commentRange.overlaps(deletedRange)) { - commentRange.subtract(deletedRange) + newRanges.push(commentRange.subtract(deletedRange)) } else if (commentRange.startsAfter(deletedRange)) { - commentRange.pos -= deletedRange.length + newRanges.push(commentRange.moveBy(-deletedRange.length)) + } else { + newRanges.push(commentRange) } } + this.ranges = newRanges this.mergeRanges() } @@ -130,10 +137,10 @@ class Comment { if (range.isEmpty()) { continue } - const lastRange = mergedRanges[mergedRanges.length - 1] + const lastMerged = mergedRanges[mergedRanges.length - 1] - if (lastRange?.canMerge(range)) { - lastRange.merge(range) + if (lastMerged?.canMerge(range)) { + mergedRanges[mergedRanges.length - 1] = lastMerged.merge(range) } else { mergedRanges.push(range) } diff --git a/libraries/overleaf-editor-core/lib/file_data/range.js b/libraries/overleaf-editor-core/lib/file_data/range.js index c7db354110..3fd83e88f2 100644 --- a/libraries/overleaf-editor-core/lib/file_data/range.js +++ b/libraries/overleaf-editor-core/lib/file_data/range.js @@ -5,7 +5,9 @@ class Range { * @param {number} length */ constructor(pos, length) { + /** @readonly */ this.pos = pos + /** @readonly */ this.length = length } @@ -74,34 +76,28 @@ class Range { /** * @param {Range} range - * @returns {number} the length of the intersected range + * @returns {Range} */ subtract(range) { if (this.contains(range)) { - this.length -= range.length - return range.length + return this.shrinkBy(range.length) } if (range.contains(this)) { - const intersectedLength = this.length - this.length = 0 - return intersectedLength + return new Range(this.pos, 0) } if (range.overlaps(this)) { if (range.start < this.start) { const intersectedLength = range.end - this.start - this.length -= intersectedLength - this.pos = range.pos - return intersectedLength + return new Range(range.pos, this.length - intersectedLength) } else { const intersectedLength = this.end - range.start - this.length -= intersectedLength - return intersectedLength + return new Range(this.pos, this.length - intersectedLength) } } - return 0 + return new Range(this.pos, this.length) } /** @@ -121,8 +117,8 @@ class Range { } const newPos = Math.min(this.pos, range.pos) const newEnd = Math.max(this.end, range.end) - this.pos = newPos - this.length = newEnd - newPos + + return new Range(newPos, newEnd - newPos) } /** @@ -130,7 +126,7 @@ class Range { * @param {number} length */ moveBy(length) { - this.pos += length + return new Range(this.pos + length, this.length) } /** @@ -138,7 +134,21 @@ class Range { * @param {number} extensionLength */ extendBy(extensionLength) { - this.length += extensionLength + return new Range(this.pos, this.length + extensionLength) + } + + /** + * Shrinks the range by a given number + * @param {number} shrinkLength + */ + shrinkBy(shrinkLength) { + const newLength = this.length - shrinkLength + + if (newLength < 0) { + throw new Error('Cannot shrink range by more than its length') + } + + return new Range(this.pos, newLength) } /** 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 a9b44d0754..ad4b8ee85d 100644 --- a/libraries/overleaf-editor-core/lib/file_data/tracked_change.js +++ b/libraries/overleaf-editor-core/lib/file_data/tracked_change.js @@ -66,7 +66,7 @@ class TrackedChange { if (!this.canMerge(other)) { throw new Error('Cannot merge tracked changes') } - this.range.merge(other.range) + this.range = this.range.merge(other.range) this.tracking.ts = this.tracking.ts.getTime() > other.tracking.ts.getTime() ? this.tracking.ts 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 042cb75def..d6399b76e7 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 @@ -103,7 +103,7 @@ class TrackedChangeList { trackedChange.range.startIsAfter(cursor) || cursor === trackedChange.range.start ) { - trackedChange.range.moveBy(insertedText.length) + trackedChange.range = trackedChange.range.moveBy(insertedText.length) newTrackedChanges.push(trackedChange) } else if (cursor === trackedChange.range.end) { // The insertion is at the end of the tracked change. So we don't need @@ -157,10 +157,10 @@ class TrackedChangeList { if (deletedRange.contains(trackedChange.range)) { continue } else if (deletedRange.overlaps(trackedChange.range)) { - trackedChange.range.subtract(deletedRange) + trackedChange.range = trackedChange.range.subtract(deletedRange) newTrackedChanges.push(trackedChange) } else if (trackedChange.range.startIsAfter(cursor)) { - trackedChange.range.pos -= length + trackedChange.range = trackedChange.range.moveBy(-length) newTrackedChanges.push(trackedChange) } else { newTrackedChanges.push(trackedChange) @@ -188,13 +188,14 @@ class TrackedChangeList { } else if (retainedRange.overlaps(trackedChange.range)) { if (trackedChange.range.contains(retainedRange)) { const [leftRange, rightRange] = trackedChange.range.splitAt(cursor) - rightRange.pos += length - rightRange.length -= length newTrackedChanges.push( new TrackedChange(leftRange, trackedChange.tracking.clone()) ) newTrackedChanges.push( - new TrackedChange(rightRange, trackedChange.tracking.clone()) + new TrackedChange( + rightRange.moveBy(length).shrinkBy(length), + trackedChange.tracking.clone() + ) ) } else if (retainedRange.start <= trackedChange.range.start) { // overlaps to the left diff --git a/libraries/overleaf-editor-core/test/range.test.js b/libraries/overleaf-editor-core/test/range.test.js index 02865d16cc..c636511626 100644 --- a/libraries/overleaf-editor-core/test/range.test.js +++ b/libraries/overleaf-editor-core/test/range.test.js @@ -186,51 +186,51 @@ describe('Range', function () { it('should not subtract', function () { const from1to5 = new Range(1, 6) const from0to1 = new Range(0, 1) - expect(from1to5.subtract(from0to1)).to.eql(0) - expect(from1to5.start).to.eql(1) - expect(from1to5.length).to.eql(6) + const subtracted = from1to5.subtract(from0to1) + expect(subtracted.start).to.eql(1) + expect(subtracted.length).to.eql(6) }) it('should subtract from the left', function () { const from5to19 = new Range(5, 15) const from15to24 = new Range(15, 10) - expect(from15to24.subtract(from5to19)).to.eql(5) - expect(from15to24.start).to.eql(5) - expect(from15to24.end).to.eql(10) + const subtracted = from15to24.subtract(from5to19) + expect(subtracted.start).to.eql(5) + expect(subtracted.end).to.eql(10) }) it('should subtract from the right', function () { const from10to24 = new Range(10, 15) const from5to19 = new Range(5, 15) - expect(from5to19.subtract(from10to24)).to.eql(10) - expect(from5to19.start).to.eql(5) - expect(from5to19.end).to.eql(10) + const subtracted = from5to19.subtract(from10to24) + expect(subtracted.start).to.eql(5) + expect(subtracted.end).to.eql(10) }) it('should subtract from the middle', function () { const from5to19 = new Range(5, 15) const from10to14 = new Range(10, 5) - expect(from5to19.subtract(from10to14)).to.eql(5) - expect(from5to19.start).to.eql(5) - expect(from5to19.end).to.eql(15) + const subtracted = from5to19.subtract(from10to14) + expect(subtracted.start).to.eql(5) + expect(subtracted.end).to.eql(15) }) it('should delete entire range', function () { const from0to99 = new Range(0, 100) const from5to19 = new Range(5, 15) - expect(from5to19.subtract(from0to99)).to.eql(15) - expect(from5to19.start).to.eql(5) - expect(from5to19.end).to.eql(5) - expect(from5to19.length).to.eql(0) + const subtracted = from5to19.subtract(from0to99) + expect(subtracted.start).to.eql(5) + expect(subtracted.end).to.eql(5) + expect(subtracted.length).to.eql(0) }) it('should not subtract if ranges do not overlap', function () { const from5to14 = new Range(5, 10) const from20to29 = new Range(20, 10) - expect(from5to14.subtract(from20to29)).to.eql(0) - expect(from20to29.subtract(from5to14)).to.eql(0) - expect(from5to14.start).to.eql(5) - expect(from5to14.end).to.eql(15) + const subtracted1 = from5to14.subtract(from20to29) + const subtracted2 = from20to29.subtract(from5to14) + expect(subtracted1.toRaw()).deep.equal(from5to14.toRaw()) + expect(subtracted2.toRaw()).deep.equal(from20to29.toRaw()) }) }) @@ -239,35 +239,35 @@ describe('Range', function () { const from5to14 = new Range(5, 10) const from10to19 = new Range(10, 10) expect(from5to14.canMerge(from10to19)).to.eql(true) - from5to14.merge(from10to19) - expect(from5to14.start).to.eql(5) - expect(from5to14.end).to.eql(20) + const result = from5to14.merge(from10to19) + expect(result.start).to.eql(5) + expect(result.end).to.eql(20) }) it('should merge ranges overlaping at the start', function () { const from5to14 = new Range(5, 10) const from0to9 = new Range(0, 10) expect(from5to14.canMerge(from0to9)).to.eql(true) - from5to14.merge(from0to9) - expect(from5to14.start).to.eql(0) - expect(from5to14.end).to.eql(15) + const result = from5to14.merge(from0to9) + expect(result.start).to.eql(0) + expect(result.end).to.eql(15) }) it('should merge ranges if one is covered by another', function () { const from5to14 = new Range(5, 10) const from0to19 = new Range(0, 20) expect(from5to14.canMerge(from0to19)).to.eql(true) - from5to14.merge(from0to19) - expect(from5to14.toRaw()).deep.equal(from0to19.toRaw()) + const result = from5to14.merge(from0to19) + expect(result.toRaw()).deep.equal(from0to19.toRaw()) }) it('should produce the same length after merge', function () { const from5to14 = new Range(5, 10) const from0to19 = new Range(0, 20) expect(from0to19.canMerge(from5to14)).to.eql(true) - from0to19.merge(from5to14) - expect(from0to19.start).to.eql(0) - expect(from0to19.end).to.eql(20) + const result = from0to19.merge(from5to14) + expect(result.start).to.eql(0) + expect(result.end).to.eql(20) }) it('should not merge ranges if they do not overlap', function () { @@ -329,18 +329,31 @@ describe('Range', function () { it('should extend the range', function () { const from5to14 = new Range(5, 10) - from5to14.extendBy(3) - expect(from5to14.length).to.eql(13) - expect(from5to14.start).to.eql(5) - expect(from5to14.end).to.eql(18) + const result = from5to14.extendBy(3) + expect(result.length).to.eql(13) + expect(result.start).to.eql(5) + expect(result.end).to.eql(18) + }) + + it('should shrink the range', function () { + const from5to14 = new Range(5, 10) + const result = from5to14.shrinkBy(3) + expect(result.length).to.eql(7) + expect(result.start).to.eql(5) + expect(result.end).to.eql(12) + }) + + it('should throw if shrinking too much', function () { + const from5to14 = new Range(5, 10) + expect(() => from5to14.shrinkBy(11)).to.throw() }) it('should move the range', function () { const from5to14 = new Range(5, 10) - from5to14.moveBy(3) - expect(from5to14.length).to.eql(10) - expect(from5to14.start).to.eql(8) - expect(from5to14.end).to.eql(18) + const result = from5to14.moveBy(3) + expect(result.length).to.eql(10) + expect(result.start).to.eql(8) + expect(result.end).to.eql(18) }) describe('splitAt', function () {