Merge pull request #26367 from overleaf/em-history-ot-undo

Fixes to TextOperation invert algorithm

GitOrigin-RevId: dd655660f6ecad7b6e9b2d4435dc9a5364d0fde2

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

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

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) {
+          for (const change of previousChanges) {
-          if (strIndex < trackedChange.range.start) {
+            if (strIndex < change.range.start) {
-            inverse.retain(trackedChange.range.start - strIndex, {
+              inverse.retain(change.range.start - strIndex, {
-              tracking: removeTrackingInfoIfNeeded,
+                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) {
+          if (strIndex < target) {
-            inverse.retain(trackedChange.range.length, {
+            inverse.retain(target - strIndex, {
-              tracking: trackedChange.tracking,
+              tracking: new ClearTrackingProps(),
             })
-            strIndex = trackedChange.range.end
+            strIndex = target
           }
-          if (trackedChange.range.end !== strIndex) {
+        } else {
-            // No need to split the range at the end
+          inverse.retain(op.length)
-            const [left] = trackedChange.range.splitAt(strIndex)
+          strIndex += op.length
-            inverse.retain(left.length, { tracking: trackedChange.tracking })
-            strIndex = left.end
-          }
-        }
-        if (strIndex < target) {
-          inverse.retain(target - strIndex, {
-            tracking: removeTrackingInfoIfNeeded,
-          })
-          strIndex = target
         }
       } else if (op instanceof InsertOp) {
         inverse.remove(op.insertion.length)

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) {
+  overlaps(other) {
-    return this.start < range.end && this.end > range.start
+    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

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

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