From fe37bc0926d126bd08add95570f5b218c78c1275 Mon Sep 17 00:00:00 2001 From: Johannes Date: Mon, 13 Jan 2025 12:54:05 +0100 Subject: [PATCH 1/2] improve diff for /edits * no double-diff for chat modified entries * align whitespace property * overflow logic for overlay widget --- .../chatEditingModifiedFileEntry.ts | 17 ++- .../chat/browser/chatEditorController.ts | 100 +++++++++++------- .../contrib/chat/browser/chatEditorOverlay.ts | 20 +++- .../chat/browser/media/chatEditorOverlay.css | 1 + 4 files changed, 93 insertions(+), 45 deletions(-) diff --git a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingModifiedFileEntry.ts b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingModifiedFileEntry.ts index 37a6b9a2b68df..e022f402c79c2 100644 --- a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingModifiedFileEntry.ts +++ b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingModifiedFileEntry.ts @@ -7,7 +7,7 @@ import { RunOnceScheduler } from '../../../../../base/common/async.js'; import { Emitter } from '../../../../../base/common/event.js'; import { Disposable, IReference, toDisposable } from '../../../../../base/common/lifecycle.js'; import { Schemas } from '../../../../../base/common/network.js'; -import { IObservable, ITransaction, observableValue, transaction } from '../../../../../base/common/observable.js'; +import { autorun, IObservable, ITransaction, observableValue, transaction } from '../../../../../base/common/observable.js'; import { themeColorFromId } from '../../../../../base/common/themables.js'; import { URI } from '../../../../../base/common/uri.js'; import { EditOperation, ISingleEditOperation } from '../../../../../editor/common/core/editOperation.js'; @@ -26,7 +26,9 @@ import { IModelService } from '../../../../../editor/common/services/model.js'; import { IResolvedTextEditorModel, ITextModelService } from '../../../../../editor/common/services/resolverService.js'; import { IModelContentChangedEvent } from '../../../../../editor/common/textModelEvents.js'; import { localize } from '../../../../../nls.js'; +import { IConfigurationService } from '../../../../../platform/configuration/common/configuration.js'; import { IFileService } from '../../../../../platform/files/common/files.js'; +import { observableConfigValue } from '../../../../../platform/observable/common/platformObservableUtils.js'; import { editorSelectionBackground } from '../../../../../platform/theme/common/colorRegistry.js'; import { IUndoRedoService } from '../../../../../platform/undoRedo/common/undoRedo.js'; import { SaveReason } from '../../../../common/editor.js'; @@ -130,6 +132,8 @@ export class ChatEditingModifiedFileEntry extends Disposable implements IModifie return this._telemetryInfo.requestId; } + private readonly _diffTrimWhitespace: IObservable; + constructor( resourceRef: IReference, private readonly _multiDiffEntryDelegate: { collapse: (transaction: ITransaction | undefined) => void }, @@ -139,6 +143,7 @@ export class ChatEditingModifiedFileEntry extends Disposable implements IModifie @IModelService modelService: IModelService, @ITextModelService textModelService: ITextModelService, @ILanguageService languageService: ILanguageService, + @IConfigurationService configService: IConfigurationService, @IChatService private readonly _chatService: IChatService, @IEditorWorkerService private readonly _editorWorkerService: IEditorWorkerService, @IUndoRedoService private readonly _undoRedoService: IUndoRedoService, @@ -186,6 +191,12 @@ export class ChatEditingModifiedFileEntry extends Disposable implements IModifie this._register(toDisposable(() => { this._clearCurrentEditLineDecoration(); })); + + this._diffTrimWhitespace = observableConfigValue('diffEditor.ignoreTrimWhitespace', true, configService); + this._register(autorun(r => { + this._diffTrimWhitespace.read(r); + this._updateDiffInfoSeq(); + })); } private _clearCurrentEditLineDecoration() { @@ -413,10 +424,12 @@ export class ChatEditingModifiedFileEntry extends Disposable implements IModifie const docVersionNow = this.doc.getVersionId(); const snapshotVersionNow = this.docSnapshot.getVersionId(); + const ignoreTrimWhitespace = this._diffTrimWhitespace.get(); + const diff = await this._editorWorkerService.computeDiff( this.docSnapshot.uri, this.doc.uri, - { computeMoves: true, ignoreTrimWhitespace: false, maxComputationTimeMs: 3000 }, + { ignoreTrimWhitespace, computeMoves: false, maxComputationTimeMs: 3000 }, 'advanced' ); diff --git a/src/vs/workbench/contrib/chat/browser/chatEditorController.ts b/src/vs/workbench/contrib/chat/browser/chatEditorController.ts index cfe1214f7d86b..78a198389f66a 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, transaction } from '../../../../base/common/observable.js'; +import { autorun, autorunWithStore, derived, IObservable, observableFromEvent, observableValue } 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'; @@ -80,9 +80,10 @@ export class ChatEditorController extends Disposable implements IEditorContribut this._ctxHasEditorModification = ctxHasEditorModification.bindTo(contextKeyService); this._ctxRequestInProgress = ctxHasRequestInProgress.bindTo(contextKeyService); - const fontInfoObs = observableCodeEditor(this._editor).getOption(EditorOption.fontInfo); - const lineHeightObs = observableCodeEditor(this._editor).getOption(EditorOption.lineHeight); - const modelObs = observableCodeEditor(this._editor).model; + const editorObs = observableCodeEditor(this._editor); + const fontInfoObs = editorObs.getOption(EditorOption.fontInfo); + const lineHeightObs = editorObs.getOption(EditorOption.lineHeight); + const modelObs = editorObs.model; this._store.add(autorun(r => { @@ -112,13 +113,13 @@ export class ChatEditorController extends Disposable implements IEditorContribut const currentEditorEntry = entryForEditor.read(r); if (!currentEditorEntry) { - this._clearRendering(); + this._clear(); didReval = false; return; } if (this._editor.getOption(EditorOption.inDiffEditor) && !_instantiationService.invokeFunction(isDiffEditorForEntry, currentEditorEntry.entry, this._editor)) { - this._clearRendering(); + this._clear(); return; } @@ -145,7 +146,16 @@ export class ChatEditorController extends Disposable implements IEditorContribut lineHeightObs.read(r); const diff = entry?.diffInfo.read(r); - this._updateWithDiff(entry, diff); + + // Add line decorations (just markers, no UI) for diff navigation + this._updateDiffLineDecorations(diff); + + // Add diff decoration to the UI (unless in diff editor) + if (!this._editor.getOption(EditorOption.inDiffEditor)) { + this._updateDiffRendering(entry, diff); + } else { + this._clearDiffRendering(); + } if (!didReval && !diff.identical) { didReval = true; @@ -199,11 +209,19 @@ export class ChatEditorController extends Disposable implements IEditorContribut } override dispose(): void { - this._clearRendering(); + this._clear(); super.dispose(); } - private _clearRendering() { + private _clear() { + this._clearDiffRendering(); + this._diffLineDecorations.clear(); + this._currentChangeIndex.set(undefined, undefined); + this._currentEntryIndex.set(undefined, undefined); + this._ctxHasEditorModification.reset(); + } + + private _clearDiffRendering() { this._editor.changeViewZones((viewZoneChangeAccessor) => { for (const id of this._viewZones) { viewZoneChangeAccessor.removeZone(id); @@ -212,18 +230,11 @@ export class ChatEditorController extends Disposable implements IEditorContribut this._viewZones = []; this._diffHunksRenderStore.clear(); this._diffVisualDecorations.clear(); - this._diffLineDecorations.clear(); - this._ctxHasEditorModification.reset(); - transaction(tx => { - this._currentEntryIndex.set(undefined, tx); - this._currentChangeIndex.set(undefined, tx); - }); this._scrollLock = false; } - private _updateWithDiff(entry: IModifiedFileEntry, diff: IDocumentDiff): void { + private _updateDiffRendering(entry: IModifiedFileEntry, diff: IDocumentDiff): void { - this._ctxHasEditorModification.set(!diff.identical); const originalModel = entry.originalModel; const chatDiffAddDecoration = ModelDecorationOptions.createDynamic({ @@ -255,7 +266,6 @@ export class ChatEditorController extends Disposable implements IEditorContribut } this._viewZones = []; const modifiedVisualDecorations: IModelDeltaDecoration[] = []; - const modifiedLineDecorations: IModelDeltaDecoration[] = []; const mightContainNonBasicASCII = originalModel.mightContainNonBasicASCII(); const mightContainRTL = originalModel.mightContainRTL(); const renderOptions = RenderOptions.fromEditor(this._editor); @@ -344,16 +354,9 @@ export class ChatEditorController extends Disposable implements IEditorContribut stickiness: TrackedRangeStickiness.AlwaysGrowsWhenTypingAtEdges } }); - - // Add line decorations for diff navigation - modifiedLineDecorations.push({ - range: diffEntry.modified.toInclusiveRange() ?? new Range(diffEntry.modified.startLineNumber, 1, diffEntry.modified.startLineNumber, Number.MAX_SAFE_INTEGER), - options: ChatEditorController._diffLineDecorationData - }); } this._diffVisualDecorations.set(modifiedVisualDecorations); - this._diffLineDecorations.set(modifiedLineDecorations); }); const diffHunkDecoCollection = this._editor.createDecorationsCollection(diffHunkDecorations); @@ -428,6 +431,20 @@ export class ChatEditorController extends Disposable implements IEditorContribut })); } + private _updateDiffLineDecorations(diff: IDocumentDiff): void { + this._ctxHasEditorModification.set(!diff.identical); + + const modifiedLineDecorations: IModelDeltaDecoration[] = []; + + for (const diffEntry of diff.changes) { + modifiedLineDecorations.push({ + range: diffEntry.modified.toInclusiveRange() ?? new Range(diffEntry.modified.startLineNumber, 1, diffEntry.modified.startLineNumber, Number.MAX_SAFE_INTEGER), + options: ChatEditorController._diffLineDecorationData + }); + } + this._diffLineDecorations.set(modifiedLineDecorations); + } + unlockScroll(): void { this._scrollLock = false; } @@ -533,6 +550,12 @@ export class ChatEditorController extends Disposable implements IEditorContribut if (!this._editor.hasModel()) { return; } + + const entry = this._chatEditingService.currentEditingSessionObs.get()?.getEntry(this._editor.getModel().uri); + if (!entry) { + return; + } + const lineRelativeTop = this._editor.getTopForLineNumber(this._editor.getPosition().lineNumber) - this._editor.getScrollTop(); let closestDistance = Number.MAX_VALUE; @@ -549,33 +572,30 @@ export class ChatEditorController extends Disposable implements IEditorContribut } } - if (!(widget instanceof DiffHunkWidget)) { - return; - } - - - const lineNumber = widget.getStartLineNumber(); - const position = lineNumber ? new Position(lineNumber, 1) : undefined; let selection = this._editor.getSelection(); - if (position && !selection.containsPosition(position)) { - selection = Selection.fromPositions(position); + if (widget instanceof DiffHunkWidget) { + const lineNumber = widget.getStartLineNumber(); + const position = lineNumber ? new Position(lineNumber, 1) : undefined; + if (position && !selection.containsPosition(position)) { + selection = Selection.fromPositions(position); + } } const isDiffEditor = this._editor.getOption(EditorOption.inDiffEditor); if (isDiffEditor) { // normal EDITOR - await this._editorService.openEditor({ resource: widget.entry.modifiedURI }); + await this._editorService.openEditor({ resource: entry.modifiedURI }); } else { // DIFF editor const defaultAgentName = this._chatAgentService.getDefaultAgent(ChatAgentLocation.EditingSession)?.fullName; const diffEditor = await this._editorService.openEditor({ - original: { resource: widget.entry.originalURI, options: { selection: undefined } }, - modified: { resource: widget.entry.modifiedURI, options: { selection } }, + original: { resource: entry.originalURI, options: { selection: undefined } }, + modified: { resource: entry.modifiedURI, options: { selection } }, label: defaultAgentName - ? localize('diff.agent', '{0} (changes from {1})', basename(widget.entry.modifiedURI), defaultAgentName) - : localize('diff.generic', '{0} (changes from chat)', basename(widget.entry.modifiedURI)) + ? localize('diff.agent', '{0} (changes from {1})', basename(entry.modifiedURI), defaultAgentName) + : localize('diff.generic', '{0} (changes from chat)', basename(entry.modifiedURI)) }); if (diffEditor && diffEditor.input) { @@ -586,7 +606,7 @@ export class ChatEditorController extends Disposable implements IEditorContribut // close diff editor when entry is decided const d = autorun(r => { - const state = widget.entry.state.read(r); + const state = entry.state.read(r); if (state === WorkingSetEntryState.Accepted || state === WorkingSetEntryState.Rejected) { d.dispose(); this._editorService.closeEditor(editorIdent); diff --git a/src/vs/workbench/contrib/chat/browser/chatEditorOverlay.ts b/src/vs/workbench/contrib/chat/browser/chatEditorOverlay.ts index 6306e2361ce68..f0c1ca0a91a7b 100644 --- a/src/vs/workbench/contrib/chat/browser/chatEditorOverlay.ts +++ b/src/vs/workbench/contrib/chat/browser/chatEditorOverlay.ts @@ -28,10 +28,11 @@ import { ChatEditorController } from './chatEditorController.js'; import { EditorOption } from '../../../../editor/common/config/editorOptions.js'; import { isDiffEditorForEntry } from './chatEditing/chatEditing.js'; import './media/chatEditorOverlay.css'; +import { findDiffEditorContainingCodeEditor } from '../../../../editor/browser/widget/diffEditor/commands.js'; class ChatEditorOverlayWidget implements IOverlayWidget { - readonly allowEditorOverflow = false; + readonly allowEditorOverflow = true; private readonly _domNode: HTMLElement; private readonly _progressNode: HTMLElement; @@ -47,7 +48,7 @@ class ChatEditorOverlayWidget implements IOverlayWidget { constructor( private readonly _editor: ICodeEditor, @IEditorService editorService: IEditorService, - @IInstantiationService instaService: IInstantiationService, + @IInstantiationService private readonly _instaService: IInstantiationService, ) { this._domNode = document.createElement('div'); this._domNode.classList.add('chat-editor-overlay-widget'); @@ -62,7 +63,7 @@ class ChatEditorOverlayWidget implements IOverlayWidget { toolbarNode.classList.add('chat-editor-overlay-toolbar'); this._domNode.appendChild(toolbarNode); - this._toolbar = instaService.createInstance(MenuWorkbenchToolBar, toolbarNode, MenuId.ChatEditingEditorContent, { + this._toolbar = _instaService.createInstance(MenuWorkbenchToolBar, toolbarNode, MenuId.ChatEditingEditorContent, { telemetrySource: 'chatEditor.overlayToolbar', hiddenItemStrategy: HiddenItemStrategy.Ignore, toolbarOptions: { @@ -228,6 +229,19 @@ class ChatEditorOverlayWidget implements IOverlayWidget { this._navigationBearings.set({ changeCount: changes, activeIdx, entriesCount: entries.length }, undefined); })); + + const editorWithObs = observableFromEvent(this._editor.onDidLayoutChange, () => { + const diffEditor = this._instaService.invokeFunction(findDiffEditorContainingCodeEditor, this._editor); + return diffEditor + ? diffEditor.getOriginalEditor().getLayoutInfo().contentWidth + diffEditor.getModifiedEditor().getLayoutInfo().contentWidth + : this._editor.getLayoutInfo().contentWidth; + }); + + this._showStore.add(autorun(r => { + const width = editorWithObs.read(r); + this._domNode.style.maxWidth = `${width - 20}px`; + })); + if (!this._isAdded) { this._editor.addOverlayWidget(this); this._isAdded = true; diff --git a/src/vs/workbench/contrib/chat/browser/media/chatEditorOverlay.css b/src/vs/workbench/contrib/chat/browser/media/chatEditorOverlay.css index b7a54ada1a2ad..153da41fe6eee 100644 --- a/src/vs/workbench/contrib/chat/browser/media/chatEditorOverlay.css +++ b/src/vs/workbench/contrib/chat/browser/media/chatEditorOverlay.css @@ -13,6 +13,7 @@ align-items: center; z-index: 10; box-shadow: 0 2px 8px var(--vscode-widget-shadow); + overflow: hidden; } @keyframes pulse { From 70989f972c7cb8d20f7053f4417e5219a8460687 Mon Sep 17 00:00:00 2001 From: Johannes Date: Mon, 13 Jan 2025 14:13:13 +0100 Subject: [PATCH 2/2] fix compiler --- .../browser/chatEditing/chatEditingModifiedNotebookEntry.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingModifiedNotebookEntry.ts b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingModifiedNotebookEntry.ts index d6cc236478aa0..4e2854a7b3730 100644 --- a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingModifiedNotebookEntry.ts +++ b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingModifiedNotebookEntry.ts @@ -9,6 +9,7 @@ import { ILanguageService } from '../../../../../editor/common/languages/languag import { IEditorWorkerService } from '../../../../../editor/common/services/editorWorker.js'; import { IModelService } from '../../../../../editor/common/services/model.js'; import { IResolvedTextEditorModel, ITextModelService } from '../../../../../editor/common/services/resolverService.js'; +import { IConfigurationService } from '../../../../../platform/configuration/common/configuration.js'; import { IFileService } from '../../../../../platform/files/common/files.js'; import { IUndoRedoService } from '../../../../../platform/undoRedo/common/undoRedo.js'; import { SaveReason } from '../../../../common/editor.js'; @@ -33,8 +34,9 @@ export class ChatEditingModifiedNotebookEntry extends ChatEditingModifiedFileEnt @IEditorWorkerService _editorWorkerService: IEditorWorkerService, @IUndoRedoService _undoRedoService: IUndoRedoService, @IFileService _fileService: IFileService, + @IConfigurationService configService: IConfigurationService ) { - super(resourceRef, _multiDiffEntryDelegate, _telemetryInfo, kind, initialContent, modelService, textModelService, languageService, _chatService, _editorWorkerService, _undoRedoService, _fileService); + super(resourceRef, _multiDiffEntryDelegate, _telemetryInfo, kind, initialContent, modelService, textModelService, languageService, configService, _chatService, _editorWorkerService, _undoRedoService, _fileService); this.resolveTextFileEditorModel = resourceRef.object as IResolvedTextFileEditorModel; }