From 2117bfe29de32367e0c4f8e55416423c7a10af36 Mon Sep 17 00:00:00 2001 From: David <33458145+davidmcpowell@users.noreply.github.com> Date: Wed, 2 Oct 2024 13:41:47 +0100 Subject: [PATCH] Merge pull request #20541 from overleaf/dk-delete-widgets Handle highlight/focus effects for track deletes in new review panel GitOrigin-RevId: 102eed9e8af04599823c1bcf0598a0328901bdba --- .../components/review-panel-entry.tsx | 7 +- .../source-editor/extensions/ranges.ts | 128 +++++++++++++++--- 2 files changed, 117 insertions(+), 18 deletions(-) diff --git a/services/web/frontend/js/features/review-panel-new/components/review-panel-entry.tsx b/services/web/frontend/js/features/review-panel-new/components/review-panel-entry.tsx index 59cb3b4745..1954baba52 100644 --- a/services/web/frontend/js/features/review-panel-new/components/review-panel-entry.tsx +++ b/services/web/frontend/js/features/review-panel-new/components/review-panel-entry.tsx @@ -6,7 +6,10 @@ import { } from '@/features/source-editor/components/codemirror-context' import { isSelectionWithinOp } from '../utils/is-selection-within-op' import classNames from 'classnames' -import { highlightRanges } from '@/features/source-editor/extensions/ranges' +import { + clearHighlightRanges, + highlightRanges, +} from '@/features/source-editor/extensions/ranges' import { useEditorManagerContext } from '@/features/ide-react/context/editor-manager-context' export const ReviewPanelEntry: FC<{ @@ -52,7 +55,7 @@ export const ReviewPanelEntry: FC<{ }} onMouseLeave={() => { if (hoverRanges) { - view.dispatch(highlightRanges()) + view.dispatch(clearHighlightRanges(op)) } }} role="button" diff --git a/services/web/frontend/js/features/source-editor/extensions/ranges.ts b/services/web/frontend/js/features/source-editor/extensions/ranges.ts index 31c951a40a..d649216243 100644 --- a/services/web/frontend/js/features/source-editor/extensions/ranges.ts +++ b/services/web/frontend/js/features/source-editor/extensions/ranges.ts @@ -28,19 +28,24 @@ type RangesData = { } const updateRangesEffect = StateEffect.define() -const highlightRangesEffect = StateEffect.define() +const highlightRangesEffect = StateEffect.define() +const clearHighlightRangesEffect = StateEffect.define() export const updateRanges = (data: RangesData): TransactionSpec => { return { effects: updateRangesEffect.of(data), } } - -export const highlightRanges = (op?: AnyOperation): TransactionSpec => { +export const highlightRanges = (op: AnyOperation): TransactionSpec => { return { effects: highlightRangesEffect.of(op), } } +export const clearHighlightRanges = (op: AnyOperation): TransactionSpec => { + return { + effects: clearHighlightRangesEffect.of(op), + } +} export const rangesDataField = StateField.define({ create() { @@ -97,8 +102,45 @@ export const ranges = () => [ for (const effect of transaction.effects) { if (effect.is(updateRangesEffect)) { this.decorations = buildChangeDecorations(effect.value) + } else if ( + effect.is(highlightRangesEffect) && + isDeleteOperation(effect.value) + ) { + this.decorations = updateDeleteWidgetHighlight( + this.decorations, + widget => + widget.change.op.p === effect.value.p && + widget.highlightType !== 'focus', + 'highlight' + ) + } else if ( + effect.is(clearHighlightRangesEffect) && + isDeleteOperation(effect.value) + ) { + this.decorations = updateDeleteWidgetHighlight( + this.decorations, + widget => + widget.change.op.p === effect.value.p && + widget.highlightType !== 'focus', + null + ) } } + + if (transaction.selection) { + this.decorations = updateDeleteWidgetHighlight( + this.decorations, + ({ change }) => + isSelectionWithinOp(change.op, update.state.selection.main), + 'focus' + ) + this.decorations = updateDeleteWidgetHighlight( + this.decorations, + ({ change }) => + !isSelectionWithinOp(change.op, update.state.selection.main), + null + ) + } } }, } @@ -127,6 +169,8 @@ export const ranges = () => [ 'ol-cm-change-highlight', effect.value ) + } else if (effect.is(clearHighlightRangesEffect)) { + this.decorations = Decoration.none } } } @@ -214,14 +258,40 @@ const buildChangeDecorations = (data: RangesData) => { return Decoration.set(decorations, true) } -const buildHighlightDecorations = (className: string, op?: AnyOperation) => { - if (!op) { - return Decoration.none +const updateDeleteWidgetHighlight = ( + decorations: DecorationSet, + predicate: (widget: ChangeDeletedWidget) => boolean, + highlightType?: 'focus' | 'highlight' | null +) => { + const widgetsToReplace: ChangeDeletedWidget[] = [] + const cursor = decorations.iter() + while (cursor.value) { + const widget = cursor.value.spec?.widget + if (widget instanceof ChangeDeletedWidget && predicate(widget)) { + widgetsToReplace.push(cursor.value.spec.widget) + } + cursor.next() } + return decorations.update({ + filter: (from, to, decoration) => { + return !widgetsToReplace.includes(decoration.spec?.widget) + }, + add: widgetsToReplace.map(({ change }) => + Decoration.widget({ + widget: new ChangeDeletedWidget(change, highlightType), + side: 1, + opType: 'd', + id: change.id, + metadata: change.metadata, + }).range(change.op.p, change.op.p) + ), + }) +} + +const buildHighlightDecorations = (className: string, op: AnyOperation) => { if (isDeleteOperation(op)) { - // nothing to highlight for deletions (for now) - // TODO: add highlight when delete indicator is done + // delete indicators are handled in change decorations return Decoration.none } @@ -229,6 +299,10 @@ const buildHighlightDecorations = (className: string, op?: AnyOperation) => { const opLength = isInsertOperation(op) ? op.i.length : op.c.length const opType = isInsertOperation(op) ? 'i' : 'c' + if (opLength === 0) { + return Decoration.none + } + return Decoration.set( Decoration.mark({ class: `${className} ${className}-${opType}`, @@ -238,7 +312,10 @@ const buildHighlightDecorations = (className: string, op?: AnyOperation) => { } class ChangeDeletedWidget extends WidgetType { - constructor(public change: Change) { + constructor( + public change: Change, + public highlightType: 'highlight' | 'focus' | null = null + ) { super() } @@ -246,12 +323,15 @@ class ChangeDeletedWidget extends WidgetType { const widget = document.createElement('span') widget.classList.add('ol-cm-change') widget.classList.add('ol-cm-change-d') - + widget.textContent = '[ — ]' + if (this.highlightType) { + widget.classList.add(`ol-cm-change-d-${this.highlightType}`) + } return widget } - eq() { - return true + eq(old: ChangeDeletedWidget) { + return old.highlightType === this.highlightType } } @@ -259,7 +339,6 @@ const createChangeRange = (change: Change, data: RangesData) => { const { id, metadata, op } = change const from = op.p - // TODO: find valid positions? if (isDeleteOperation(op)) { const opType = 'd' @@ -326,9 +405,26 @@ const trackChangesTheme = EditorView.baseTheme({ '.ol-cm-change-focus': { padding: 'var(--half-leading, 0) 0', }, - '.ol-cm-change-d': { - borderLeft: '2px dotted #c5060b', - marginLeft: '-1px', + // TODO: fix dark mode colors + '&light .ol-cm-change-d': { + color: '#c5060b', + backgroundColor: '#f5beba57', + }, + '&dark .ol-cm-change-d': { + color: '#c5060b', + backgroundColor: '#f5beba57', + }, + '&light .ol-cm-change-d-highlight': { + backgroundColor: '#f5bebaa4', + }, + '&dark .ol-cm-change-d-highlight': { + backgroundColor: '#f5bebaa4', + }, + '&light .ol-cm-change-d-focus': { + backgroundColor: '#F5BEBA', + }, + '&dark .ol-cm-change-d-focus': { + backgroundColor: '#F5BEBA', }, '&light .ol-cm-change-highlight-i': { backgroundColor: '#b8dbc899',