Merge pull request #26128 from overleaf/em-no-tracked-deletes-in-cm

History OT: Remove tracked deletes from CodeMirror

GitOrigin-RevId: 4e7f30cf2ed90b0c261eaa4ba51a2f54fe6e3cef

services/web/frontend/js/features/ide-react/editor/share-js-history-ot-type.ts

@@ -1,11 +1,7 @@
 import {
   EditOperation,
   EditOperationTransformer,
-  InsertOp,
-  RemoveOp,
-  RetainOp,
   StringFileData,
-  TextOperation,
 } from 'overleaf-editor-core'
 import { ShareDoc } from '../../../../../types/share-doc'
 
@@ -15,7 +11,6 @@ type Api = {
 
   getText(): string
   getLength(): number
-  _register(): void
 }
 
 const api: Api & ThisType<Api & ShareDoc & { snapshot: StringFileData }> = {
@@ -23,64 +18,12 @@ const api: Api & ThisType<Api & ShareDoc & { snapshot: StringFileData }> = {
   trackChangesUserId: null,
 
   getText() {
-    return this.snapshot.getContent()
+    return this.snapshot.getContent({ filterTrackedDeletes: true })
   },
 
   getLength() {
     return this.snapshot.getStringLength()
   },
-
-  _register() {
-    this.on('remoteop', (ops: EditOperation[], oldSnapshot: StringFileData) => {
-      const operation = ops[0]
-      if (operation instanceof TextOperation) {
-        const str = oldSnapshot.getContent()
-        if (str.length !== operation.baseLength)
-          throw new TextOperation.ApplyError(
-            "The operation's base length must be equal to the string's length.",
-            operation,
-            str
-          )
-
-        let outputCursor = 0
-        let inputCursor = 0
-        let trackedChangesInvalidated = false
-        for (const op of operation.ops) {
-          if (op instanceof RetainOp) {
-            inputCursor += op.length
-            outputCursor += op.length
-            if (op.tracking != null) {
-              trackedChangesInvalidated = true
-            }
-          } else if (op instanceof InsertOp) {
-            this.emit('insert', outputCursor, op.insertion, op.insertion.length)
-            outputCursor += op.insertion.length
-            trackedChangesInvalidated = true
-          } else if (op instanceof RemoveOp) {
-            this.emit(
-              'delete',
-              outputCursor,
-              str.slice(inputCursor, inputCursor + op.length)
-            )
-            inputCursor += op.length
-            trackedChangesInvalidated = true
-          }
-        }
-
-        if (inputCursor !== str.length) {
-          throw new TextOperation.ApplyError(
-            "The operation didn't operate on the whole string.",
-            operation,
-            str
-          )
-        }
-
-        if (trackedChangesInvalidated) {
-          this.emit('tracked-changes-invalidated')
-        }
-      }
-    })
-  },
 }
 
 export const historyOTType = {

services/web/frontend/js/features/source-editor/extensions/history-ot.ts

@@ -1,4 +1,4 @@
-import { Decoration, EditorView } from '@codemirror/view'
+import { Decoration, EditorView, WidgetType } from '@codemirror/view'
 import {
   ChangeSpec,
   EditorState,
@@ -14,69 +14,151 @@ import {
   TrackedChangeList,
 } from 'overleaf-editor-core'
 import { DocumentContainer } from '@/features/ide-react/editor/document-container'
+import { HistoryOTShareDoc } from '../../../../../types/share-doc'
 
 export const historyOT = (currentDoc: DocumentContainer) => {
-  const trackedChanges = currentDoc.doc?.getTrackedChanges()
+  const trackedChanges =
+    currentDoc.doc?.getTrackedChanges() ?? new TrackedChangeList([])
+  const positionMapper = new PositionMapper(trackedChanges)
   return [
     trackChangesUserIdState,
+    shareDocState.init(() => currentDoc?.doc?._doc ?? null),
     commentsState,
-    trackedChanges != null
-      ? trackedChangesState.init(() =>
-          buildTrackedChangesDecorations(trackedChanges)
-        )
-      : trackedChangesState,
+    trackedChangesState.init(() => ({
+      decorations: buildTrackedChangesDecorations(
+        trackedChanges,
+        positionMapper
+      ),
+      positionMapper,
+    })),
     trackedChangesFilter,
-    rangesTheme,
+    trackedChangesTheme,
   ]
 }
 
-const rangesTheme = EditorView.theme({
-  '.tracked-change-insertion': {
-    backgroundColor: 'rgba(0, 255, 0, 0.2)',
-  },
-  '.tracked-change-deletion': {
-    backgroundColor: 'rgba(255, 0, 0, 0.2)',
+export const shareDocState = StateField.define<HistoryOTShareDoc | null>({
+  create() {
+    return null
   },
-  '.comment': {
-    backgroundColor: 'rgba(255, 255, 0, 0.2)',
+
+  update(value, transaction) {
+    // this state is constant
+    return value
   },
 })
 
-const updateTrackedChangesEffect = StateEffect.define<TrackedChangeList>()
+const trackedChangesTheme = EditorView.baseTheme({
+  '.ol-cm-change-i, .ol-cm-change-highlight-i, .ol-cm-change-focus-i': {
+    backgroundColor: 'rgba(44, 142, 48, 0.30)',
+  },
+  '&light .ol-cm-change-c, &light .ol-cm-change-highlight-c, &light .ol-cm-change-focus-c':
+    {
+      backgroundColor: 'rgba(243, 177, 17, 0.30)',
+    },
+  '&dark .ol-cm-change-c, &dark .ol-cm-change-highlight-c, &dark .ol-cm-change-focus-c':
+    {
+      backgroundColor: 'rgba(194, 93, 11, 0.15)',
+    },
+  '.ol-cm-change': {
+    padding: 'var(--half-leading, 0) 0',
+  },
+  '.ol-cm-change-highlight': {
+    padding: 'var(--half-leading, 0) 0',
+  },
+  '.ol-cm-change-focus': {
+    padding: 'var(--half-leading, 0) 0',
+  },
+  '&light .ol-cm-change-d': {
+    borderLeft: '2px dotted #c5060b',
+    marginLeft: '-1px',
+  },
+  '&dark .ol-cm-change-d': {
+    borderLeft: '2px dotted #c5060b',
+    marginLeft: '-1px',
+  },
+  '&light .ol-cm-change-d-highlight': {
+    borderLeft: '3px solid #c5060b',
+    marginLeft: '-2px',
+  },
+  '&dark .ol-cm-change-d-highlight': {
+    borderLeft: '3px solid #c5060b',
+    marginLeft: '-2px',
+  },
+  '&light .ol-cm-change-d-focus': {
+    borderLeft: '3px solid #B83A33',
+    marginLeft: '-2px',
+  },
+  '&dark .ol-cm-change-d-focus': {
+    borderLeft: '3px solid #B83A33',
+    marginLeft: '-2px',
+  },
+})
 
-export const updateTrackedChanges = (trackedChanges: TrackedChangeList) => {
-  return {
-    effects: updateTrackedChangesEffect.of(trackedChanges),
+export const updateTrackedChangesEffect =
+  StateEffect.define<TrackedChangeList>()
+
+const buildTrackedChangesDecorations = (
+  trackedChanges: TrackedChangeList,
+  positionMapper: PositionMapper
+) => {
+  const decorations = []
+  for (const change of trackedChanges.asSorted()) {
+    if (change.tracking.type === 'insert') {
+      decorations.push(
+        Decoration.mark({
+          class: 'ol-cm-change ol-cm-change-i',
+          tracking: change.tracking,
+        }).range(
+          positionMapper.toCM6(change.range.pos),
+          positionMapper.toCM6(change.range.end)
+        )
+      )
+    } else {
+      decorations.push(
+        Decoration.widget({
+          widget: new ChangeDeletedWidget(),
+          side: 1,
+        }).range(positionMapper.toCM6(change.range.pos))
+      )
+    }
   }
+
+  return Decoration.set(decorations, true)
 }
 
-const buildTrackedChangesDecorations = (trackedChanges: TrackedChangeList) =>
-  Decoration.set(
-    trackedChanges.asSorted().map(change =>
-      Decoration.mark({
-        class:
-          change.tracking.type === 'insert'
-            ? 'tracked-change-insertion'
-            : 'tracked-change-deletion',
-        tracking: change.tracking,
-      }).range(change.range.pos, change.range.end)
-    ),
-    true
-  )
+class ChangeDeletedWidget extends WidgetType {
+  toDOM() {
+    const widget = document.createElement('span')
+    widget.classList.add('ol-cm-change')
+    widget.classList.add('ol-cm-change-d')
+    return widget
+  }
 
-const trackedChangesState = StateField.define({
+  eq(old: ChangeDeletedWidget) {
+    return true
+  }
+}
+
+export const trackedChangesState = StateField.define({
   create() {
-    return Decoration.none
+    return {
+      decorations: Decoration.none,
+      positionMapper: new PositionMapper(new TrackedChangeList([])),
+    }
   },
 
   update(value, transaction) {
-    if (transaction.docChanged) {
-      value = value.map(transaction.changes)
-    }
-
     for (const effect of transaction.effects) {
       if (effect.is(updateTrackedChangesEffect)) {
-        value = buildTrackedChangesDecorations(effect.value)
+        const trackedChanges = effect.value
+        const positionMapper = new PositionMapper(trackedChanges)
+        value = {
+          decorations: buildTrackedChangesDecorations(
+            effect.value,
+            positionMapper
+          ),
+          positionMapper,
+        }
       }
     }
 
@@ -84,7 +166,7 @@ const trackedChangesState = StateField.define({
   },
 
   provide(field) {
-    return EditorView.decorations.from(field)
+    return EditorView.decorations.from(field, value => value.decorations)
   },
 })
 
@@ -165,21 +247,28 @@ const trackedChangesFilter = EditorState.transactionFilter.of(tr => {
   }
 
   const trackingUserId = tr.startState.field(trackChangesUserIdState)
+  const positionMapper = tr.startState.field(trackedChangesState).positionMapper
   const startDoc = tr.startState.doc
   const changes: ChangeSpec[] = []
-  const opBuilder = new OperationBuilder(startDoc.length)
+  const effects = []
+  const opBuilder = new OperationBuilder(
+    positionMapper.toSnapshot(startDoc.length)
+  )
 
   if (trackingUserId == null) {
     // Not tracking changes
     tr.changes.iterChanges((fromA, toA, fromB, toB, inserted) => {
       // insert
       if (inserted.length > 0) {
-        opBuilder.insert(fromA, inserted.toString())
+        const pos = positionMapper.toSnapshot(fromA)
+        opBuilder.insert(pos, inserted.toString())
       }
 
       // deletion
       if (toA > fromA) {
-        opBuilder.delete(fromA, toA - fromA)
+        const start = positionMapper.toSnapshot(fromA)
+        const end = positionMapper.toSnapshot(toA)
+        opBuilder.delete(start, end - start)
       }
     })
   } else {
@@ -188,8 +277,9 @@ const trackedChangesFilter = EditorState.transactionFilter.of(tr => {
     tr.changes.iterChanges((fromA, toA, fromB, toB, inserted) => {
       // insertion
       if (inserted.length > 0) {
+        const pos = positionMapper.toSnapshot(fromA)
         opBuilder.trackedInsert(
-          fromA,
+          pos,
           inserted.toString(),
           trackingUserId,
           timestamp
@@ -198,23 +288,23 @@ const trackedChangesFilter = EditorState.transactionFilter.of(tr => {
 
       // deletion
       if (toA > fromA) {
-        const deleted = startDoc.sliceString(fromA, toA)
-        // re-insert the deleted text after the inserted text
-        changes.push({
-          from: fromB + inserted.length,
-          insert: deleted,
-        })
-
-        opBuilder.trackedDelete(fromA, toA - fromA, trackingUserId, timestamp)
+        const start = positionMapper.toSnapshot(fromA)
+        const end = positionMapper.toSnapshot(toA)
+        opBuilder.trackedDelete(start, end - start, trackingUserId, timestamp)
       }
     })
   }
 
   const op = opBuilder.finish()
-  return [
-    tr,
-    { changes, effects: historyOTOperationEffect.of([op]), sequential: true },
-  ]
+  const shareDoc = tr.startState.field(shareDocState)
+  if (shareDoc != null) {
+    shareDoc.submitOp([op])
+    effects.push(
+      updateTrackedChangesEffect.of(shareDoc.snapshot.getTrackedChanges())
+    )
+  }
+
+  return [tr, { changes, effects, sequential: true }]
 })
 
 /**
@@ -288,3 +378,74 @@ class OperationBuilder {
     return this.op
   }
 }
+
+type OffsetTable = { pos: number; map: (pos: number) => number }[]
+
+class PositionMapper {
+  private offsets: {
+    toCM6: OffsetTable
+    toSnapshot: OffsetTable
+  }
+
+  constructor(trackedChanges: TrackedChangeList) {
+    this.offsets = {
+      toCM6: [{ pos: 0, map: pos => pos }],
+      toSnapshot: [{ pos: 0, map: pos => pos }],
+    }
+
+    // Offset of the snapshot pos relative to the CM6 pos
+    let offset = 0
+    for (const change of trackedChanges.asSorted()) {
+      if (change.tracking.type === 'delete') {
+        const deleteLength = change.range.length
+        const deletePos = change.range.pos
+        const oldOffset = offset
+        const newOffset = offset + deleteLength
+        this.offsets.toSnapshot.push({
+          pos: change.range.pos - offset + 1,
+          map: pos => pos + newOffset,
+        })
+        this.offsets.toCM6.push({
+          pos: change.range.pos,
+          map: pos => deletePos - oldOffset,
+        })
+        this.offsets.toCM6.push({
+          pos: change.range.pos + deleteLength,
+          map: pos => pos - newOffset,
+        })
+        offset = newOffset
+      }
+    }
+  }
+
+  toCM6(snapshotPos: number) {
+    return this.mapPos(snapshotPos, this.offsets.toCM6)
+  }
+
+  toSnapshot(cm6Pos: number) {
+    return this.mapPos(cm6Pos, this.offsets.toSnapshot)
+  }
+
+  mapPos(pos: number, offsets: OffsetTable) {
+    // Binary search for the offset at the last position before pos
+    let low = 0
+    let high = offsets.length - 1
+    while (low < high) {
+      const middle = Math.ceil((low + high) / 2)
+      const entry = offsets[middle]
+      if (entry.pos < pos) {
+        // This entry could be the right offset, but lower entries are too low
+        // Because we used Math.ceil(), middle is higher than low and the
+        // algorithm progresses.
+        low = middle
+      } else if (entry.pos > pos) {
+        // This entry is too high
+        high = middle - 1
+      } else {
+        // This is the right entry
+        return entry.map(pos)
+      }
+    }
+    return offsets[low].map(pos)
+  }
+}

services/web/frontend/js/features/source-editor/extensions/realtime.ts

@@ -4,6 +4,7 @@ import {
   Annotation,
   ChangeSpec,
   Text,
+  StateEffect,
 } from '@codemirror/state'
 import { EditorView, ViewPlugin } from '@codemirror/view'
 import { EventEmitter } from 'events'
@@ -15,11 +16,18 @@ import {
 } from '../../../../../types/share-doc'
 import { debugConsole } from '@/utils/debugging'
 import { DocumentContainer } from '@/features/ide-react/editor/document-container'
-import { TrackedChangeList } from 'overleaf-editor-core'
 import {
-  updateTrackedChanges,
+  EditOperation,
+  TextOperation,
+  InsertOp,
+  RemoveOp,
+  RetainOp,
+} from 'overleaf-editor-core'
+import {
+  updateTrackedChangesEffect,
   setTrackChangesUserId,
-  historyOTOperationEffect,
+  trackedChangesState,
+  shareDocState,
 } from './history-ot'
 
 /*
@@ -143,10 +151,6 @@ export class EditorFacade extends EventEmitter {
     this.cmChange({ from: position, to: position + text.length }, origin)
   }
 
-  cmUpdateTrackedChanges(trackedChanges: TrackedChangeList) {
-    this.view.dispatch(updateTrackedChanges(trackedChanges))
-  }
-
   attachShareJs(shareDoc: ShareDoc, maxDocLength?: number) {
     this.otAdapter =
       shareDoc.otType === 'history-ot'
@@ -320,22 +324,11 @@ class HistoryOTAdapter {
   attachShareJs() {
     this.checkContent()
 
-    const onInsert = this.onShareJsInsert.bind(this)
-    const onDelete = this.onShareJsDelete.bind(this)
-    const onTrackedChangesInvalidated =
-      this.onShareJsTrackedChangesInvalidated.bind(this)
-
-    this.shareDoc.on('insert', onInsert)
-    this.shareDoc.on('delete', onDelete)
-    this.shareDoc.on('tracked-changes-invalidated', onTrackedChangesInvalidated)
+    const onRemoteOp = this.onRemoteOp.bind(this)
+    this.shareDoc.on('remoteop', onRemoteOp)
 
     this.shareDoc.detach_cm6 = () => {
-      this.shareDoc.removeListener('insert', onInsert)
-      this.shareDoc.removeListener('delete', onDelete)
-      this.shareDoc.removeListener(
-        'tracked-changes-invalidated',
-        onTrackedChangesInvalidated
-      )
+      this.shareDoc.removeListener('remoteop', onRemoteOp)
       delete this.shareDoc.detach_cm6
       this.editor.detachShareJs()
     }
@@ -357,22 +350,6 @@ class HistoryOTAdapter {
         return
       }
 
-      let snapshotUpdated = false
-      for (const effect of transaction.effects) {
-        if (effect.is(historyOTOperationEffect)) {
-          this.shareDoc.submitOp(effect.value)
-          snapshotUpdated = true
-        }
-      }
-
-      if (snapshotUpdated || transaction.annotation(Transaction.remote)) {
-        window.setTimeout(() => {
-          this.editor.cmUpdateTrackedChanges(
-            this.shareDoc.snapshot.getTrackedChanges()
-          )
-        }, 0)
-      }
-
       const origin = chooseOrigin(transaction)
       transaction.changes.iterChanges((fromA, toA, fromB, toB, inserted) => {
         this.onCodeMirrorChange(fromA, toA, fromB, toB, inserted, origin)
@@ -380,20 +357,70 @@ class HistoryOTAdapter {
     }
   }
 
-  onShareJsInsert(pos: number, text: string) {
-    this.editor.cmInsert(pos, text, 'remote')
-    this.checkContent()
-  }
+  onRemoteOp(operations: EditOperation[]) {
+    const positionMapper =
+      this.editor.view.state.field(trackedChangesState).positionMapper
+    const changes: ChangeSpec[] = []
+    let trackedChangesUpdated = false
+    for (const operation of operations) {
+      if (operation instanceof TextOperation) {
+        let cursor = 0
+        for (const op of operation.ops) {
+          if (op instanceof InsertOp) {
+            if (op.tracking?.type !== 'delete') {
+              changes.push({
+                from: positionMapper.toCM6(cursor),
+                insert: op.insertion,
+              })
+            }
+            trackedChangesUpdated = true
+          } else if (op instanceof RemoveOp) {
+            changes.push({
+              from: positionMapper.toCM6(cursor),
+              to: positionMapper.toCM6(cursor + op.length),
+            })
+            cursor += op.length
+            trackedChangesUpdated = true
+          } else if (op instanceof RetainOp) {
+            if (op.tracking != null) {
+              if (op.tracking.type === 'delete') {
+                changes.push({
+                  from: positionMapper.toCM6(cursor),
+                  to: positionMapper.toCM6(cursor + op.length),
+                })
+              }
+              trackedChangesUpdated = true
+            }
+            cursor += op.length
+          }
+        }
+      }
 
-  onShareJsDelete(pos: number, text: string) {
-    this.editor.cmDelete(pos, text, 'remote')
-    this.checkContent()
-  }
+      const view = this.editor.view
+      const effects: StateEffect<any>[] = []
+      const scrollEffect = view
+        .scrollSnapshot()
+        .map(view.state.changes(changes))
+      if (scrollEffect != null) {
+        effects.push(scrollEffect)
+      }
+      if (trackedChangesUpdated) {
+        const shareDoc = this.editor.view.state.field(shareDocState)
+        if (shareDoc != null) {
+          const trackedChanges = shareDoc.snapshot.getTrackedChanges()
+          effects.push(updateTrackedChangesEffect.of(trackedChanges))
+        }
+      }
 
-  onShareJsTrackedChangesInvalidated() {
-    this.editor.cmUpdateTrackedChanges(
-      this.shareDoc.snapshot.getTrackedChanges()
-    )
+      view.dispatch({
+        changes,
+        effects,
+        annotations: [
+          Transaction.remote.of(true),
+          Transaction.addToHistory.of(false),
+        ],
+      })
+    }
   }
 
   onCodeMirrorChange(