From 5ae6f1e82930572167dd27a86570e49f9ae658f0 Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Fri, 13 Dec 2024 18:04:38 +0100 Subject: [PATCH] chat overlay fixes (#236079) * fixes disabled button color in chat overlay * fixes https://github.com/microsoft/vscode-copilot/issues/10930 * fix reveal after streaming issues --- .../chat/browser/chatEditorController.ts | 106 ++++++++++-------- .../contrib/chat/browser/chatEditorOverlay.ts | 31 +++-- .../chat/browser/media/chatEditorOverlay.css | 12 +- 3 files changed, 78 insertions(+), 71 deletions(-) diff --git a/src/vs/workbench/contrib/chat/browser/chatEditorController.ts b/src/vs/workbench/contrib/chat/browser/chatEditorController.ts index 0cca055ece25c..18a7c1a258ef1 100644 --- a/src/vs/workbench/contrib/chat/browser/chatEditorController.ts +++ b/src/vs/workbench/contrib/chat/browser/chatEditorController.ts @@ -6,7 +6,7 @@ import './media/chatEditorController.css'; import { addStandardDisposableListener, getTotalWidth } from '../../../../base/browser/dom.js'; import { Disposable, DisposableStore, dispose, toDisposable } from '../../../../base/common/lifecycle.js'; -import { autorun, autorunWithStore, derived, IObservable, observableFromEvent, observableValue } from '../../../../base/common/observable.js'; +import { autorun, autorunWithStore, derived, IObservable, observableFromEvent, observableValue, transaction } from '../../../../base/common/observable.js'; import { themeColorFromId } from '../../../../base/common/themables.js'; import { ICodeEditor, IOverlayWidget, IOverlayWidgetPosition, IOverlayWidgetPositionCoordinates, IViewZone, MouseTargetType } from '../../../../editor/browser/editorBrowser.js'; import { LineSource, renderLines, RenderOptions } from '../../../../editor/browser/widget/diffEditor/components/diffEditorViewZones/renderLines.js'; @@ -55,8 +55,11 @@ export class ChatEditorController extends Disposable implements IEditorContribut return controller; } - private readonly _currentChange = observableValue(this, undefined); - readonly currentChange: IObservable = this._currentChange; + private readonly _currentEntryIndex = observableValue(this, undefined); + readonly currentEntryIndex: IObservable = this._currentEntryIndex; + + private readonly _currentChangeIndex = observableValue(this, undefined); + readonly currentChangeIndex: IObservable = this._currentChangeIndex; private _scrollLock: boolean = false; @@ -76,20 +79,28 @@ export class ChatEditorController extends Disposable implements IEditorContribut const lineHeightObs = observableCodeEditor(this._editor).getOption(EditorOption.lineHeight); const modelObs = observableCodeEditor(this._editor).model; - // scroll along unless "another" scroll happens - let ignoreScrollEvent = false; - this._store.add(this._editor.onDidScrollChange(e => { - if (e.scrollTopChanged && !ignoreScrollEvent) { - // this._scrollLock = true; - } - })); this._store.add(autorun(r => { const session = this._chatEditingService.currentEditingSessionObs.read(r); this._ctxRequestInProgress.set(session?.state.read(r) === ChatEditingSessionState.StreamingEdits); })); - let didReveal = false; + + const entryForEditor = derived(r => { + const model = modelObs.read(r); + const session = this._chatEditingService.currentEditingSessionObs.read(r); + if (!session) { + return undefined; + } + const entry = model?.uri ? session.readEntry(model.uri, r) : undefined; + if (!entry || entry.state.read(r) !== WorkingSetEntryState.Modified) { + return undefined; + } + return { session, entry }; + }); + + + let didReval = false; this._register(autorunWithStore((r, store) => { @@ -98,55 +109,48 @@ export class ChatEditorController extends Disposable implements IEditorContribut return; } - fontInfoObs.read(r); - lineHeightObs.read(r); - - const model = modelObs.read(r); + const currentEditorEntry = entryForEditor.read(r); - const session = this._chatEditingService.currentEditingSessionObs.read(r); - const entry = model?.uri ? session?.readEntry(model.uri, r) : undefined; - - if (!entry || entry.state.read(r) !== WorkingSetEntryState.Modified) { + if (!currentEditorEntry) { this._clearRendering(); - didReveal = false; + didReval = false; return; } + const { session, entry } = currentEditorEntry; + + const entryIndex = session.entries.read(r).indexOf(entry); + this._currentEntryIndex.set(entryIndex, undefined); + if (entry.isCurrentlyBeingModified.read(r)) { + // while modified: scroll along unless locked + if (!this._scrollLock) { + const maxLineNumber = entry.maxLineNumber.read(r); + this._editor.revealLineNearTop(maxLineNumber, ScrollType.Smooth); + } const domNode = this._editor.getDomNode(); if (domNode) { store.add(addStandardDisposableListener(domNode, 'wheel', () => { this._scrollLock = true; })); } - } + } else { + // done: render diff + fontInfoObs.read(r); + lineHeightObs.read(r); - try { - ignoreScrollEvent = true; - if (!this._scrollLock && !didReveal) { - const maxLineNumber = entry.maxLineNumber.read(r); - this._editor.revealLineNearTop(maxLineNumber, ScrollType.Smooth); - } const diff = entry?.diffInfo.read(r); this._updateWithDiff(entry, diff); - if (!entry.isCurrentlyBeingModified.read(r)) { - this.initNavigation(); - - if (!this._scrollLock && !didReveal) { - const currentPosition = this._currentChange.read(r); - if (currentPosition) { - this._editor.revealLine(currentPosition.lineNumber, ScrollType.Smooth); - didReveal = true; - } else { - didReveal = this.revealNext(); - } - } + + if (!didReval && !diff.identical) { + didReval = true; + this.revealNext(); } - } finally { - ignoreScrollEvent = false; } })); + // ---- readonly while streaming + const shouldBeReadOnly = derived(this, r => { const value = this._chatEditingService.currentEditingSessionObs.read(r); if (!value || value.state.read(r) !== ChatEditingSessionState.StreamingEdits) { @@ -200,7 +204,10 @@ export class ChatEditorController extends Disposable implements IEditorContribut this._diffVisualDecorations.clear(); this._diffLineDecorations.clear(); this._ctxHasEditorModification.reset(); - this._currentChange.set(undefined, undefined); + transaction(tx => { + this._currentEntryIndex.set(undefined, tx); + this._currentChangeIndex.set(undefined, tx); + }); this._scrollLock = false; } @@ -423,10 +430,8 @@ export class ChatEditorController extends Disposable implements IEditorContribut initNavigation(): void { const position = this._editor.getPosition(); - const range = position && this._diffLineDecorations.getRanges().find(r => r.containsPosition(position)); - if (range) { - this._currentChange.set(position, undefined); - } + const target = position ? this._diffLineDecorations.getRanges().findIndex(r => r.containsPosition(position)) : -1; + this._currentChangeIndex.set(target >= 0 ? target : undefined, undefined); } revealNext(strict = false): boolean { @@ -440,6 +445,7 @@ export class ChatEditorController extends Disposable implements IEditorContribut private _reveal(next: boolean, strict: boolean): boolean { const position = this._editor.getPosition(); if (!position) { + this._currentChangeIndex.set(undefined, undefined); return false; } @@ -448,6 +454,7 @@ export class ChatEditorController extends Disposable implements IEditorContribut .sort((a, b) => Range.compareRangesUsingStarts(a, b)); if (decorations.length === 0) { + this._currentChangeIndex.set(undefined, undefined); return false; } @@ -464,18 +471,19 @@ export class ChatEditorController extends Disposable implements IEditorContribut } if (strict && (target < 0 || target >= decorations.length)) { + this._currentChangeIndex.set(undefined, undefined); return false; } target = (target + decorations.length) % decorations.length; - const targetPosition = next ? decorations[target].getStartPosition() : decorations[target].getEndPosition(); - - this._currentChange.set(targetPosition, undefined); + this._currentChangeIndex.set(target, undefined); + const targetPosition = next ? decorations[target].getStartPosition() : decorations[target].getEndPosition(); this._editor.setPosition(targetPosition); this._editor.revealPositionInCenter(targetPosition, ScrollType.Smooth); this._editor.focus(); + return true; } diff --git a/src/vs/workbench/contrib/chat/browser/chatEditorOverlay.ts b/src/vs/workbench/contrib/chat/browser/chatEditorOverlay.ts index 6a09f5ef2751f..bb94d4e0fc91c 100644 --- a/src/vs/workbench/contrib/chat/browser/chatEditorOverlay.ts +++ b/src/vs/workbench/contrib/chat/browser/chatEditorOverlay.ts @@ -203,25 +203,24 @@ class ChatEditorOverlayWidget implements IOverlayWidget { this._showStore.add(autorun(r => { - const position = ChatEditorController.get(this._editor)?.currentChange.read(r); + const ctrl = ChatEditorController.get(this._editor); + + const entryIndex = ctrl?.currentEntryIndex.read(r); + const changeIndex = ctrl?.currentChangeIndex.read(r); + const entries = session.entries.read(r); + let activeIdx = entryIndex !== undefined && changeIndex !== undefined + ? changeIndex + : -1; + let changes = 0; - let activeIdx = -1; - for (const entry of entries) { - const diffInfo = entry.diffInfo.read(r); - - if (activeIdx !== -1 || entry !== activeEntry) { - // just add up - changes += diffInfo.changes.length; - - } else { - for (const change of diffInfo.changes) { - if (position && change.modified.includes(position.lineNumber)) { - activeIdx = changes; - } - changes += 1; - } + for (let i = 0; i < entries.length; i++) { + const diffInfo = entries[i].diffInfo.read(r); + changes += diffInfo.changes.length; + + if (entryIndex !== undefined && i < entryIndex) { + activeIdx += diffInfo.changes.length; } } diff --git a/src/vs/workbench/contrib/chat/browser/media/chatEditorOverlay.css b/src/vs/workbench/contrib/chat/browser/media/chatEditorOverlay.css index 67dfaedc4b9f5..26ece80c64cad 100644 --- a/src/vs/workbench/contrib/chat/browser/media/chatEditorOverlay.css +++ b/src/vs/workbench/contrib/chat/browser/media/chatEditorOverlay.css @@ -53,10 +53,10 @@ color: var(--vscode-button-foreground); } -.chat-editor-overlay-widget .action-item.disabled > .action-label.codicon::before, -.chat-editor-overlay-widget .action-item.disabled > .action-label.codicon, -.chat-editor-overlay-widget .action-item.disabled > .action-label, -.chat-editor-overlay-widget .action-item.disabled > .action-label:hover { +.chat-editor-overlay-widget .monaco-action-bar .action-item.disabled > .action-label.codicon::before, +.chat-editor-overlay-widget .monaco-action-bar .action-item.disabled > .action-label.codicon, +.chat-editor-overlay-widget .monaco-action-bar .action-item.disabled > .action-label, +.chat-editor-overlay-widget .monaco-action-bar .action-item.disabled > .action-label:hover { color: var(--vscode-button-foreground); opacity: 0.7; } @@ -66,8 +66,8 @@ font-variant-numeric: tabular-nums; } -.chat-editor-overlay-widget .action-item.label-item > .action-label, -.chat-editor-overlay-widget .action-item.label-item > .action-label:hover { +.chat-editor-overlay-widget .monaco-action-bar .action-item.label-item > .action-label, +.chat-editor-overlay-widget .monaco-action-bar .action-item.label-item > .action-label:hover { color: var(--vscode-button-foreground); opacity: 1; }