|
|
@@ -4,6 +4,7 @@ import {
|
|
|
StateEffect,
|
|
|
StateField,
|
|
|
Transaction,
|
|
|
+ TransactionSpec,
|
|
|
} from '@codemirror/state'
|
|
|
import {
|
|
|
Decoration,
|
|
|
@@ -21,14 +22,39 @@ import {
|
|
|
StoredComment,
|
|
|
} from './changes/comments'
|
|
|
import { invertedEffects } from '@codemirror/commands'
|
|
|
-import { Change, DeleteOperation } from '../../../../../types/change'
|
|
|
+import {
|
|
|
+ Change,
|
|
|
+ DeleteOperation,
|
|
|
+ EditOperation,
|
|
|
+} from '../../../../../types/change'
|
|
|
import { ChangeManager } from './changes/change-manager'
|
|
|
import { debugConsole } from '@/utils/debugging'
|
|
|
-import { isCommentOperation, isDeleteOperation } from '@/utils/operations'
|
|
|
+import {
|
|
|
+ isCommentOperation,
|
|
|
+ isDeleteOperation,
|
|
|
+ isInsertOperation,
|
|
|
+} from '@/utils/operations'
|
|
|
import {
|
|
|
DocumentContainer,
|
|
|
RangesTrackerWithResolvedThreadIds,
|
|
|
} from '@/features/ide-react/editor/document-container'
|
|
|
+import { trackChangesAnnotation } from '@/features/source-editor/extensions/realtime'
|
|
|
+import { Ranges } from '@/features/review-panel-new/context/ranges-context'
|
|
|
+import { Threads } from '@/features/review-panel-new/context/threads-context'
|
|
|
+import { isSplitTestEnabled } from '@/utils/splitTestUtils'
|
|
|
+
|
|
|
+type RangesData = {
|
|
|
+ ranges: Ranges
|
|
|
+ threads: Threads
|
|
|
+}
|
|
|
+
|
|
|
+const updateRangesEffect = StateEffect.define<RangesData>()
|
|
|
+
|
|
|
+export const updateRanges = (data: RangesData): TransactionSpec => {
|
|
|
+ return {
|
|
|
+ effects: updateRangesEffect.of(data),
|
|
|
+ }
|
|
|
+}
|
|
|
|
|
|
const clearChangesEffect = StateEffect.define()
|
|
|
const buildChangesEffect = StateEffect.define()
|
|
|
@@ -46,7 +72,9 @@ const restoreDetachedCommentsEffect = StateEffect.define<RangeSet<any>>({
|
|
|
|
|
|
type Options = {
|
|
|
currentDoc: DocumentContainer
|
|
|
- loadingThreads: boolean
|
|
|
+ loadingThreads?: boolean
|
|
|
+ ranges?: Ranges
|
|
|
+ threads?: Threads
|
|
|
}
|
|
|
|
|
|
/**
|
|
|
@@ -54,8 +82,8 @@ type Options = {
|
|
|
* and produces decorations for tracked changes and comments.
|
|
|
*/
|
|
|
export const trackChanges = (
|
|
|
- { currentDoc, loadingThreads }: Options,
|
|
|
- changeManager: ChangeManager
|
|
|
+ { currentDoc, loadingThreads, ranges, threads }: Options,
|
|
|
+ changeManager?: ChangeManager
|
|
|
) => {
|
|
|
// A state field that stored any comments found within the ranges of a "cut" transaction,
|
|
|
// to be restored when pasting matching text.
|
|
|
@@ -120,18 +148,20 @@ export const trackChanges = (
|
|
|
cutCommentsState,
|
|
|
|
|
|
// initialize/destroy the change manager, and handle any updates
|
|
|
- ViewPlugin.define(() => {
|
|
|
- changeManager.initialize()
|
|
|
-
|
|
|
- return {
|
|
|
- update: update => {
|
|
|
- changeManager.handleUpdate(update)
|
|
|
- },
|
|
|
- destroy: () => {
|
|
|
- changeManager.destroy()
|
|
|
- },
|
|
|
- }
|
|
|
- }),
|
|
|
+ changeManager
|
|
|
+ ? ViewPlugin.define(() => {
|
|
|
+ changeManager.initialize()
|
|
|
+
|
|
|
+ return {
|
|
|
+ update: update => {
|
|
|
+ changeManager.handleUpdate(update)
|
|
|
+ },
|
|
|
+ destroy: () => {
|
|
|
+ changeManager.destroy()
|
|
|
+ },
|
|
|
+ }
|
|
|
+ })
|
|
|
+ : [],
|
|
|
|
|
|
// draw change decorations
|
|
|
ViewPlugin.define<
|
|
|
@@ -140,10 +170,20 @@ export const trackChanges = (
|
|
|
}
|
|
|
>(
|
|
|
() => {
|
|
|
+ let decorations = Decoration.none
|
|
|
+ if (isSplitTestEnabled('review-panel-redesign')) {
|
|
|
+ if (ranges && threads) {
|
|
|
+ decorations = buildChangeDecorations(currentDoc, {
|
|
|
+ ranges,
|
|
|
+ threads,
|
|
|
+ })
|
|
|
+ }
|
|
|
+ } else if (!loadingThreads) {
|
|
|
+ decorations = buildChangeDecorations(currentDoc)
|
|
|
+ }
|
|
|
+
|
|
|
return {
|
|
|
- decorations: loadingThreads
|
|
|
- ? Decoration.none
|
|
|
- : buildChangeDecorations(currentDoc),
|
|
|
+ decorations,
|
|
|
update(update) {
|
|
|
for (const transaction of update.transactions) {
|
|
|
this.decorations = this.decorations.map(transaction.changes)
|
|
|
@@ -153,6 +193,11 @@ export const trackChanges = (
|
|
|
this.decorations = Decoration.none
|
|
|
} else if (effect.is(buildChangesEffect)) {
|
|
|
this.decorations = buildChangeDecorations(currentDoc)
|
|
|
+ } else if (effect.is(updateRangesEffect)) {
|
|
|
+ this.decorations = buildChangeDecorations(
|
|
|
+ currentDoc,
|
|
|
+ effect.value
|
|
|
+ )
|
|
|
}
|
|
|
}
|
|
|
}
|
|
|
@@ -181,18 +226,23 @@ export const buildChangeMarkers = () => {
|
|
|
}
|
|
|
}
|
|
|
|
|
|
-const buildChangeDecorations = (currentDoc: DocumentContainer) => {
|
|
|
- if (!currentDoc.ranges) {
|
|
|
+const buildChangeDecorations = (
|
|
|
+ currentDoc: DocumentContainer,
|
|
|
+ data?: RangesData
|
|
|
+) => {
|
|
|
+ const ranges = data ? data.ranges : currentDoc.ranges
|
|
|
+
|
|
|
+ if (!ranges) {
|
|
|
return Decoration.none
|
|
|
}
|
|
|
|
|
|
- const changes = [...currentDoc.ranges.changes, ...currentDoc.ranges.comments]
|
|
|
+ const changes = [...ranges.changes, ...ranges.comments]
|
|
|
|
|
|
const decorations = []
|
|
|
|
|
|
for (const change of changes) {
|
|
|
try {
|
|
|
- decorations.push(...createChangeRange(change, currentDoc))
|
|
|
+ decorations.push(...createChangeRange(change, currentDoc, data))
|
|
|
} catch (error) {
|
|
|
// ignore invalid changes
|
|
|
debugConsole.debug('invalid change position', error)
|
|
|
@@ -251,7 +301,11 @@ class ChangeCalloutWidget extends WidgetType {
|
|
|
}
|
|
|
}
|
|
|
|
|
|
-const createChangeRange = (change: Change, currentDoc: DocumentContainer) => {
|
|
|
+const createChangeRange = (
|
|
|
+ change: Change,
|
|
|
+ currentDoc: DocumentContainer,
|
|
|
+ data?: RangesData
|
|
|
+) => {
|
|
|
const { id, metadata, op } = change
|
|
|
|
|
|
const from = op.p
|
|
|
@@ -289,6 +343,20 @@ const createChangeRange = (change: Change, currentDoc: DocumentContainer) => {
|
|
|
return []
|
|
|
}
|
|
|
|
|
|
+ if (_isCommentOperation) {
|
|
|
+ if (data) {
|
|
|
+ const thread = data.threads[op.t]
|
|
|
+ if (!thread || thread.resolved) {
|
|
|
+ return []
|
|
|
+ }
|
|
|
+ } else if (
|
|
|
+ (currentDoc.ranges as RangesTrackerWithResolvedThreadIds)
|
|
|
+ .resolvedThreadIds![op.t]
|
|
|
+ ) {
|
|
|
+ return []
|
|
|
+ }
|
|
|
+ }
|
|
|
+
|
|
|
const opType = _isCommentOperation ? 'c' : 'i'
|
|
|
const changedText = _isCommentOperation ? op.c : op.i
|
|
|
const to = from + changedText.length
|
|
|
@@ -316,6 +384,107 @@ const createChangeRange = (change: Change, currentDoc: DocumentContainer) => {
|
|
|
return [calloutWidget.range(from, from), changeMark.range(from, to)]
|
|
|
}
|
|
|
|
|
|
+/**
|
|
|
+ * Remove tracked changes from the range tracker when they're rejected,
|
|
|
+ * and restore the original content
|
|
|
+ */
|
|
|
+export const rejectChanges = (
|
|
|
+ state: EditorState,
|
|
|
+ ranges: DocumentContainer['ranges'],
|
|
|
+ changeIds: string[]
|
|
|
+) => {
|
|
|
+ const changes = ranges!.getChanges(changeIds) as Change<EditOperation>[]
|
|
|
+
|
|
|
+ if (changes.length === 0) {
|
|
|
+ return {}
|
|
|
+ }
|
|
|
+
|
|
|
+ // When doing bulk rejections, adjacent changes might interact with each other.
|
|
|
+ // Consider an insertion with an adjacent deletion (which is a common use-case, replacing words):
|
|
|
+ //
|
|
|
+ // "foo bar baz" -> "foo quux baz"
|
|
|
+ //
|
|
|
+ // The change above will be modeled with two ops, with the insertion going first:
|
|
|
+ //
|
|
|
+ // foo quux baz
|
|
|
+ // |--| -> insertion of "quux", op 1, at position 4
|
|
|
+ // | -> deletion of "bar", op 2, pushed forward by "quux" to position 8
|
|
|
+ //
|
|
|
+ // When rejecting these changes at once, if the insertion is rejected first, we get unexpected
|
|
|
+ // results. What happens is:
|
|
|
+ //
|
|
|
+ // 1) Rejecting the insertion deletes the added word "quux", i.e., it removes 4 chars
|
|
|
+ // starting from position 4;
|
|
|
+ //
|
|
|
+ // "foo quux baz" -> "foo baz"
|
|
|
+ // |--| -> 4 characters to be removed
|
|
|
+ //
|
|
|
+ // 2) Rejecting the deletion adds the deleted word "bar" at position 8 (i.e. it will act as if
|
|
|
+ // the word "quuux" was still present).
|
|
|
+ //
|
|
|
+ // "foo baz" -> "foo bazbar"
|
|
|
+ // | -> deletion of "bar" is reverted by reinserting "bar" at position 8
|
|
|
+ //
|
|
|
+ // While the intended result would be "foo bar baz", what we get is:
|
|
|
+ //
|
|
|
+ // "foo bazbar" (note "bar" readded at position 8)
|
|
|
+ //
|
|
|
+ // The issue happens because of step 1. To revert the insertion of "quux", 4 characters are deleted
|
|
|
+ // from position 4. This includes the position where the deletion exists; when that position is
|
|
|
+ // cleared, the RangesTracker considers that the deletion is gone and stops tracking/updating it.
|
|
|
+ // As we still hold a reference to it, the code tries to revert it by readding the deleted text, but
|
|
|
+ // does so at the outdated position (position 8, which was valid when "quux" was present).
|
|
|
+ //
|
|
|
+ // To avoid this kind of problem, we need to make sure that reverting operations doesn't affect
|
|
|
+ // subsequent operations that come after. Reverse sorting the operations based on position will
|
|
|
+ // achieve it; in the case above, it makes sure that the the deletion is reverted first:
|
|
|
+ //
|
|
|
+ // 1) Rejecting the deletion adds the deleted word "bar" at position 8
|
|
|
+ //
|
|
|
+ // "foo quux baz" -> "foo quuxbar baz"
|
|
|
+ // | -> deletion of "bar" is reverted by
|
|
|
+ // reinserting "bar" at position 8
|
|
|
+ //
|
|
|
+ // 2) Rejecting the insertion deletes the added word "quux", i.e., it removes 4 chars
|
|
|
+ // starting from position 4 and achieves the expected result:
|
|
|
+ //
|
|
|
+ // "foo quuxbar baz" -> "foo bar baz"
|
|
|
+ // |--| -> 4 characters to be removed
|
|
|
+
|
|
|
+ changes.sort((a, b) => b.op.p - a.op.p)
|
|
|
+
|
|
|
+ const changesToDispatch = changes.map(change => {
|
|
|
+ const { op } = change
|
|
|
+
|
|
|
+ if (isInsertOperation(op)) {
|
|
|
+ const from = op.p
|
|
|
+ const content = op.i
|
|
|
+ const to = from + content.length
|
|
|
+
|
|
|
+ const text = state.doc.sliceString(from, to)
|
|
|
+
|
|
|
+ if (text !== content) {
|
|
|
+ throw new Error(`Op to be removed does not match editor text`)
|
|
|
+ }
|
|
|
+
|
|
|
+ return { from, to, insert: '' }
|
|
|
+ } else if (isDeleteOperation(op)) {
|
|
|
+ return {
|
|
|
+ from: op.p,
|
|
|
+ to: op.p,
|
|
|
+ insert: op.d,
|
|
|
+ }
|
|
|
+ } else {
|
|
|
+ throw new Error(`unknown change type: ${JSON.stringify(change)}`)
|
|
|
+ }
|
|
|
+ })
|
|
|
+
|
|
|
+ return {
|
|
|
+ changes: changesToDispatch,
|
|
|
+ annotations: [trackChangesAnnotation.of('reject')],
|
|
|
+ }
|
|
|
+}
|
|
|
+
|
|
|
const trackChangesTheme = EditorView.baseTheme({
|
|
|
'.cm-line': {
|
|
|
overflowX: 'hidden', // needed so the callout elements don't overflow (requires line wrapping to be on)
|