[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

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)
       }

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)
   }
 
   /**

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

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

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 () {