From 5e4e2f5681c0d09b5f4222f5523b761363ef860b Mon Sep 17 00:00:00 2001 From: Henning Dieterichs Date: Thu, 27 Apr 2023 21:16:28 +0200 Subject: [PATCH 1/3] Improves code --- src/vs/base/common/observableImpl/autorun.ts | 38 ++-- src/vs/base/common/observableImpl/base.ts | 25 ++- src/vs/base/common/observableImpl/derived.ts | 43 ++++- .../browser/inlineCompletionsController.ts | 55 +++--- .../browser/inlineCompletionsModel.ts | 159 ++++++++-------- .../browser/inlineCompletionsSource.ts | 171 ++++++++++-------- 6 files changed, 272 insertions(+), 219 deletions(-) diff --git a/src/vs/base/common/observableImpl/autorun.ts b/src/vs/base/common/observableImpl/autorun.ts index 23718e12328c6..43d2b2218478f 100644 --- a/src/vs/base/common/observableImpl/autorun.ts +++ b/src/vs/base/common/observableImpl/autorun.ts @@ -4,31 +4,22 @@ *--------------------------------------------------------------------------------------------*/ import { DisposableStore, IDisposable, toDisposable } from 'vs/base/common/lifecycle'; -import { IReader, IObservable, IObserver } from 'vs/base/common/observableImpl/base'; +import { IReader, IObservable, IObserver, IChangeContext } from 'vs/base/common/observableImpl/base'; import { getLogger } from 'vs/base/common/observableImpl/logging'; export function autorun(debugName: string, fn: (reader: IReader) => void): IDisposable { - return new AutorunObserver(debugName, fn, undefined); + return new AutorunObserver(debugName, fn, undefined, undefined); } -interface IChangeContext { - readonly changedObservable: IObservable; - readonly change: unknown; - - didChange(observable: IObservable): this is { change: TChange }; -} - -export function autorunHandleChanges( +export function autorunHandleChanges( debugName: string, options: { - /** - * Returns if this change should cause a re-run of the autorun. - */ - handleChange: (context: IChangeContext) => boolean; + createEmptyChangeSummary?: () => TChangeSummary; + handleChange: (context: IChangeContext, changeSummary: TChangeSummary) => boolean; }, - fn: (reader: IReader) => void + fn: (reader: IReader, changeSummary: TChangeSummary) => void ): IDisposable { - return new AutorunObserver(debugName, fn, options.handleChange); + return new AutorunObserver(debugName, fn, options.createEmptyChangeSummary, options.handleChange); } export function autorunWithStore( @@ -63,18 +54,21 @@ const enum AutorunState { upToDate = 3, } -export class AutorunObserver implements IObserver, IReader, IDisposable { +export class AutorunObserver implements IObserver, IReader, IDisposable { private state = AutorunState.stale; private updateCount = 0; private disposed = false; private dependencies = new Set>(); private dependenciesToBeRemoved = new Set>(); + private changeSummary: TChangeSummary | undefined; constructor( public readonly debugName: string, - private readonly runFn: (reader: IReader) => void, - private readonly _handleChange: ((context: IChangeContext) => boolean) | undefined + private readonly runFn: (reader: IReader, changeSummary: TChangeSummary) => void, + private readonly createChangeSummary: (() => TChangeSummary) | undefined, + private readonly _handleChange: ((context: IChangeContext, summary: TChangeSummary) => boolean) | undefined, ) { + this.changeSummary = this.createChangeSummary?.(); getLogger()?.handleAutorunCreated(this); this._runIfNeeded(); } @@ -101,7 +95,9 @@ export class AutorunObserver implements IObserver, IReader, IDisposable { getLogger()?.handleAutorunTriggered(this); try { - this.runFn(this); + const changeSummary = this.changeSummary!; + this.changeSummary = this.createChangeSummary?.(); + this.runFn(this, changeSummary); } finally { // We don't want our observed observables to think that they are (not even temporarily) not being observed. // Thus, we only unsubscribe from observables that are definitely not read anymore. @@ -156,7 +152,7 @@ export class AutorunObserver implements IObserver, IReader, IDisposable { changedObservable: observable, change, didChange: o => o === observable as any, - }) : true; + }, this.changeSummary!) : true; if (shouldReact) { this.state = AutorunState.stale; } diff --git a/src/vs/base/common/observableImpl/base.ts b/src/vs/base/common/observableImpl/base.ts index 993afcb6742e6..b8d60522bf24e 100644 --- a/src/vs/base/common/observableImpl/base.ts +++ b/src/vs/base/common/observableImpl/base.ts @@ -203,6 +203,14 @@ export function transaction(fn: (tx: ITransaction) => void, getDebugName?: () => } } +export function subtransaction(tx: ITransaction | undefined, fn: (tx: ITransaction) => void, getDebugName?: () => string): void { + if (!tx) { + transaction(fn, getDebugName); + } else { + fn(tx); + } +} + export class TransactionImpl implements ITransaction { private updatingObservers: { observer: IObserver; observable: IObservable }[] | null = []; @@ -261,7 +269,7 @@ export class ObservableValue } public set(value: T, tx: ITransaction | undefined, change: TChange): void { - if (this._value === value) { + if (this._value === value && change === undefined) { return; } @@ -313,3 +321,18 @@ export class DisposableObservableValue; + readonly change: unknown; + + didChange(observable: IObservable): this is { change: TChange }; +} + +export interface IChangeTracker { + /** + * Returns if this change should cause an invalidation. + * Can record the changes to just process deltas. + */ + handleChange(context: IChangeContext): boolean; +} diff --git a/src/vs/base/common/observableImpl/derived.ts b/src/vs/base/common/observableImpl/derived.ts index b232fdbc892c0..8d912a1d45000 100644 --- a/src/vs/base/common/observableImpl/derived.ts +++ b/src/vs/base/common/observableImpl/derived.ts @@ -4,11 +4,21 @@ *--------------------------------------------------------------------------------------------*/ import { BugIndicatingError } from 'vs/base/common/errors'; -import { IReader, IObservable, BaseObservable, IObserver, _setDerived } from 'vs/base/common/observableImpl/base'; +import { IReader, IObservable, BaseObservable, IObserver, _setDerived, IChangeContext } from 'vs/base/common/observableImpl/base'; import { getLogger } from 'vs/base/common/observableImpl/logging'; export function derived(debugName: string | (() => string), computeFn: (reader: IReader) => T): IObservable { - return new Derived(debugName, computeFn); + return new Derived(debugName, computeFn, undefined, undefined); +} + +export function derivedHandleChanges( + debugName: string | (() => string), + options: { + createEmptyChangeSummary: () => TChangeSummary; + handleChange: (context: IChangeContext, changeSummary: TChangeSummary) => boolean; + }, + computeFn: (reader: IReader, changeSummary: TChangeSummary) => T): IObservable { + return new Derived(debugName, computeFn, options.createEmptyChangeSummary, options.handleChange); } _setDerived(derived); @@ -35,12 +45,13 @@ const enum DerivedState { upToDate = 3, } -export class Derived extends BaseObservable implements IReader, IObserver { +export class Derived extends BaseObservable implements IReader, IObserver { private state = DerivedState.initial; private value: T | undefined = undefined; private updateCount = 0; private dependencies = new Set>(); private dependenciesToBeRemoved = new Set>(); + private changeSummary: TChangeSummary | undefined = undefined; public override get debugName(): string { return typeof this._debugName === 'function' ? this._debugName() : this._debugName; @@ -48,10 +59,12 @@ export class Derived extends BaseObservable implements IReader, IObs constructor( private readonly _debugName: string | (() => string), - private readonly computeFn: (reader: IReader) => T + private readonly computeFn: (reader: IReader, changeSummary: TChangeSummary) => T, + private readonly createChangeSummary: (() => TChangeSummary) | undefined, + private readonly _handleChange: ((context: IChangeContext, summary: TChangeSummary) => boolean) | undefined, ) { super(); - + this.changeSummary = this.createChangeSummary?.(); getLogger()?.handleDerivedCreated(this); } @@ -72,7 +85,7 @@ export class Derived extends BaseObservable implements IReader, IObs if (this.observers.size === 0) { // Without observers, we don't know when to clean up stuff. // Thus, we don't cache anything to prevent memory leaks. - const result = this.computeFn(this); + const result = this.computeFn(this, this.createChangeSummary?.()!); // Clear new dependencies this.onLastObserverRemoved(); return result; @@ -113,9 +126,11 @@ export class Derived extends BaseObservable implements IReader, IObs const oldValue = this.value; this.state = DerivedState.upToDate; + const changeSummary = this.changeSummary!; + this.changeSummary = this.createChangeSummary?.(); try { /** might call {@link handleChange} indirectly, which could invalidate us */ - this.value = this.computeFn(this); + this.value = this.computeFn(this, changeSummary); } finally { // We don't want our observed observables to think that they are (not even temporarily) not being observed. // Thus, we only unsubscribe from observables that are definitely not read anymore. @@ -189,9 +204,19 @@ export class Derived extends BaseObservable implements IReader, IObs } } - public handleChange(observable: IObservable, _change: TChange): void { + public handleChange(observable: IObservable, change: TChange): void { const isUpToDate = this.state === DerivedState.upToDate; - if ((this.state === DerivedState.dependenciesMightHaveChanged || isUpToDate) && this.dependencies.has(observable)) { + let shouldReact = true; + + if (this._handleChange && this.dependencies.has(observable)) { + shouldReact = this._handleChange({ + changedObservable: observable, + change, + didChange: o => o === observable as any, + }, this.changeSummary!); + } + + if (shouldReact && (this.state === DerivedState.dependenciesMightHaveChanged || isUpToDate) && this.dependencies.has(observable)) { this.state = DerivedState.stale; if (isUpToDate) { for (const r of this.observers) { diff --git a/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsController.ts b/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsController.ts index cd4e3ef308047..9cf5ea4e380a1 100644 --- a/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsController.ts +++ b/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsController.ts @@ -57,6 +57,8 @@ export class InlineCompletionsController extends Disposable { { min: 50, max: 50 } ); + private readonly _enabled = observableFromEvent(this.editor.onDidChangeConfiguration, () => this.editor.getOption(EditorOption.inlineSuggest).enabled); + constructor( public readonly editor: ICodeEditor, @IInstantiationService private readonly instantiationService: IInstantiationService, @@ -71,30 +73,27 @@ export class InlineCompletionsController extends Disposable { this._register(new InlineCompletionContextKeys(this.contextKeyService, this.model)); - const enabled = observableFromEvent(editor.onDidChangeConfiguration, () => editor.getOption(EditorOption.inlineSuggest).enabled); + this._register(Event.runAndSubscribe(editor.onDidChangeModel, () => transaction(tx => { + this.model.set(undefined, tx); + this.updateObservables(tx, VersionIdChangeReason.Other); - this._register(Event.runAndSubscribe(editor.onDidChangeModel, () => { - transaction(tx => { - this.model.set(undefined, tx); // This disposes the model - this.updateObservables(tx, VersionIdChangeReason.Other); - const textModel = editor.getModel(); - if (textModel) { - const model = instantiationService.createInstance( - InlineCompletionsModel, - textModel, - this.suggestWidgetAdaptor.selectedItem, - this.cursorPosition, - this.textModelVersionId, - this._debounceValue, - observableFromEvent(editor.onDidChangeConfiguration, () => editor.getOption(EditorOption.suggest).preview), - observableFromEvent(editor.onDidChangeConfiguration, () => editor.getOption(EditorOption.suggest).previewMode), - observableFromEvent(editor.onDidChangeConfiguration, () => editor.getOption(EditorOption.inlineSuggest).mode), - enabled - ); - this.model.set(model, tx); - } - }); - })); + const textModel = editor.getModel(); + if (textModel) { + const model = instantiationService.createInstance( + InlineCompletionsModel, + textModel, + this.suggestWidgetAdaptor.selectedItem, + this.cursorPosition, + this.textModelVersionId, + this._debounceValue, + observableFromEvent(editor.onDidChangeConfiguration, () => editor.getOption(EditorOption.suggest).preview), + observableFromEvent(editor.onDidChangeConfiguration, () => editor.getOption(EditorOption.suggest).previewMode), + observableFromEvent(editor.onDidChangeConfiguration, () => editor.getOption(EditorOption.inlineSuggest).mode), + this._enabled, + ); + this.model.set(model, tx); + } + }))); this._register(editor.onDidChangeModelContent((e) => transaction(tx => this.updateObservables(tx, @@ -114,7 +113,7 @@ export class InlineCompletionsController extends Disposable { this._register(editor.onDidType(() => transaction(tx => { this.updateObservables(tx, VersionIdChangeReason.Other); - if (enabled.get()) { + if (this._enabled.get()) { this.model.get()?.trigger(tx); } }))); @@ -129,7 +128,7 @@ export class InlineCompletionsController extends Disposable { inlineSuggestCommitId, 'acceptSelectedSuggestion', ]); - if (commands.has(e.commandId) && editor.hasTextFocus() && enabled.get()) { + if (commands.has(e.commandId) && editor.hasTextFocus() && this._enabled.get()) { transaction(tx => { this.model.get()?.trigger(tx); }); @@ -204,7 +203,7 @@ export class InlineCompletionsController extends Disposable { this.cursorPosition.set(this.editor.getPosition() ?? new Position(1, 1), tx); } - shouldShowHoverAt(range: Range) { + public shouldShowHoverAt(range: Range) { const ghostText = this.model.get()?.ghostText.get(); if (ghostText) { return ghostText.parts.some(p => range.containsPosition(new Position(ghostText.lineNumber, p.column))); @@ -216,9 +215,9 @@ export class InlineCompletionsController extends Disposable { return this.ghostTextWidget.ownsViewZone(viewZoneId); } - hide() { + public hide() { transaction(tx => { - this?.model.get()?.stop(tx); + this.model.get()?.stop(tx); }); } } diff --git a/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsModel.ts b/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsModel.ts index 319cbe3e52907..09bf41957d862 100644 --- a/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsModel.ts +++ b/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsModel.ts @@ -6,20 +6,22 @@ import { mapFind } from 'vs/base/common/arrays'; import { BugIndicatingError, onUnexpectedExternalError } from 'vs/base/common/errors'; import { Disposable } from 'vs/base/common/lifecycle'; -import { IObservable, ITransaction, autorun, autorunHandleChanges, derived, observableSignal, observableValue, transaction } from 'vs/base/common/observable'; +import { IObservable, ITransaction, autorun, derived, observableSignal, observableValue, transaction } from 'vs/base/common/observable'; +import { subtransaction } from 'vs/base/common/observableImpl/base'; +import { derivedHandleChanges } from 'vs/base/common/observableImpl/derived'; import { isDefined } from 'vs/base/common/types'; import { ICodeEditor } from 'vs/editor/browser/editorBrowser'; import { EditOperation } from 'vs/editor/common/core/editOperation'; import { Position } from 'vs/editor/common/core/position'; import { Range } from 'vs/editor/common/core/range'; -import { InlineCompletionTriggerKind } from 'vs/editor/common/languages'; +import { InlineCompletionContext, InlineCompletionTriggerKind } from 'vs/editor/common/languages'; import { ILanguageConfigurationService } from 'vs/editor/common/languages/languageConfigurationRegistry'; import { EndOfLinePreference, ITextModel } from 'vs/editor/common/model'; import { IFeatureDebounceInformation } from 'vs/editor/common/services/languageFeatureDebounce'; import { GhostText } from 'vs/editor/contrib/inlineCompletions/browser/ghostText'; -import { addPositions, lengthOfText } from 'vs/editor/contrib/inlineCompletions/browser/utils'; import { InlineCompletionWithUpdatedRange, InlineCompletionsSource } from 'vs/editor/contrib/inlineCompletions/browser/inlineCompletionsSource'; import { SuggestItemInfo } from 'vs/editor/contrib/inlineCompletions/browser/suggestWidgetInlineCompletionProvider'; +import { addPositions, lengthOfText } from 'vs/editor/contrib/inlineCompletions/browser/utils'; import { SnippetController2 } from 'vs/editor/contrib/snippet/browser/snippetController2'; import { ICommandService } from 'vs/platform/commands/common/commands'; import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; @@ -33,7 +35,7 @@ export enum VersionIdChangeReason { export class InlineCompletionsModel extends Disposable { private readonly _source = this._register(this._instantiationService.createInstance(InlineCompletionsSource, this.textModel, this.textModelVersionId, this._debounceValue)); - private readonly _isActive = observableValue('isActive', false); + private readonly _isActive = observableValue('isActive', false); private readonly _forceUpdate = observableSignal('forceUpdate'); private _isAcceptingPartially = false; @@ -42,8 +44,6 @@ export class InlineCompletionsModel extends Disposable { private _isNavigatingCurrentInlineCompletion = false; public get isNavigatingCurrentInlineCompletion() { return this._isNavigatingCurrentInlineCompletion; } - private _updatePromise: Promise | undefined; // TODO make this a computed - constructor( public readonly textModel: ITextModel, public readonly selectedSuggestItem: IObservable, @@ -60,58 +60,8 @@ export class InlineCompletionsModel extends Disposable { ) { super(); - const preserveCurrentCompletionReasons = new Set([ - VersionIdChangeReason.Redo, - VersionIdChangeReason.Undo, - VersionIdChangeReason.AcceptWord, - ]); - - // TODO implement ChangeHandler concept - let preserveCurrentCompletion = false; - let inlineCompletionTriggerKind = InlineCompletionTriggerKind.Automatic; - - this._register(autorunHandleChanges('update', { - handleChange: (ctx) => { - if (ctx.didChange(this.textModelVersionId) && preserveCurrentCompletionReasons.has(ctx.change)) { - preserveCurrentCompletion = true; - } else if (ctx.didChange(this._forceUpdate)) { - inlineCompletionTriggerKind = ctx.change; - } - return true; - } - }, reader => { - this._forceUpdate.read(reader); - if ((this._enabled.read(reader) && this.selectedSuggestItem.read(reader)) || this._isActive.read(reader)) { - const shouldPreserveCurrentCompletion = preserveCurrentCompletion || (this.selectedInlineCompletion.get()?.inlineCompletion.source.inlineCompletions.enableForwardStability ?? false); - - const suggestItem = this.selectedSuggestItem.read(reader); - const cursorPosition = this.cursorPosition.read(reader); - this.textModelVersionId.read(reader); - - const suggestWidgetInlineCompletions = this._source.suggestWidgetInlineCompletions.get(); - if (suggestWidgetInlineCompletions && !suggestItem) { - const inlineCompletions = this._source.inlineCompletions.get(); - if (inlineCompletions && suggestWidgetInlineCompletions.request.versionId > inlineCompletions.request.versionId) { - this._source.inlineCompletions.set(suggestWidgetInlineCompletions.clone(), undefined); - } - this._source.clearSuggestWidgetInlineCompletions(); - } - - this._updatePromise = this._source.update( - cursorPosition, - { - triggerKind: inlineCompletionTriggerKind, - selectedSuggestionInfo: suggestItem?.toSelectedSuggestionInfo() - }, - shouldPreserveCurrentCompletion ? this.selectedInlineCompletion.get() : undefined - ); - } else { - this._updatePromise = undefined; - } - - // Reset local state - preserveCurrentCompletion = false; - inlineCompletionTriggerKind = InlineCompletionTriggerKind.Automatic; + this._register(autorun('automatically fetch inline completions', (reader) => { + this.fetchInlineCompletions.read(reader); })); let lastItem: InlineCompletionWithUpdatedRange | undefined = undefined; @@ -129,17 +79,71 @@ export class InlineCompletionsModel extends Disposable { })); } + private readonly preserveCurrentCompletionReasons = new Set([ + VersionIdChangeReason.Redo, + VersionIdChangeReason.Undo, + VersionIdChangeReason.AcceptWord, + ]); + + private readonly fetchInlineCompletions = derivedHandleChanges('fetch inline completions', { + createEmptyChangeSummary: () => ({ + preserveCurrentCompletion: false, + inlineCompletionTriggerKind: InlineCompletionTriggerKind.Automatic + }), + handleChange: (ctx, changeSummary) => { + if (ctx.didChange(this.textModelVersionId) && this.preserveCurrentCompletionReasons.has(ctx.change)) { + changeSummary.preserveCurrentCompletion = true; + } else if (ctx.didChange(this._forceUpdate)) { + changeSummary.inlineCompletionTriggerKind = ctx.change; + } + return true; + }, + }, (reader, changeSummary) => { + this._forceUpdate.read(reader); + const shouldUpdate = (this._enabled.read(reader) && this.selectedSuggestItem.read(reader)) || this._isActive.read(reader); + if (!shouldUpdate) { + this._source.cancelUpdate(); + return undefined; + } + + this.textModelVersionId.read(reader); + + const itemToPreserveCandidate = this.selectedInlineCompletion.get(); + const itemToPreserve = changeSummary.preserveCurrentCompletion || itemToPreserveCandidate?.forwardStable + ? itemToPreserveCandidate : undefined; + + const suggestWidgetInlineCompletions = this._source.suggestWidgetInlineCompletions.get(); + const suggestItem = this.selectedSuggestItem.read(reader); + if (suggestWidgetInlineCompletions && !suggestItem) { + const inlineCompletions = this._source.inlineCompletions.get(); + transaction(tx => { + /** @summary Seed inline completions with (newer) suggest widget inline completions */ + if (inlineCompletions && suggestWidgetInlineCompletions.request.versionId > inlineCompletions.request.versionId) { + this._source.inlineCompletions.set(suggestWidgetInlineCompletions.clone(), tx); + } + this._source.clearSuggestWidgetInlineCompletions(tx); + }); + } + + const cursorPosition = this.cursorPosition.read(reader); + const context: InlineCompletionContext = { + triggerKind: changeSummary.inlineCompletionTriggerKind, + selectedSuggestionInfo: suggestItem?.toSelectedSuggestionInfo(), + }; + return this._source.fetch(cursorPosition, context, itemToPreserve); + }); + public async trigger(tx?: ITransaction): Promise { this._isActive.set(true, tx); - await this._updatePromise; + await this.fetchInlineCompletions.get(); } - public async triggerExplicitly(): Promise { - transaction(tx => { + public async triggerExplicitly(tx?: ITransaction): Promise { + subtransaction(tx, tx => { this._isActive.set(true, tx); this._forceUpdate.trigger(tx, InlineCompletionTriggerKind.Explicit); }); - await this._updatePromise; + await this.fetchInlineCompletions.get(); } public stop(tx?: ITransaction): void { @@ -160,18 +164,16 @@ export class InlineCompletionsModel extends Disposable { }); // We use a semantic id to keep the same inline completion selected even if the provider reorders the completions. - private _selectedInlineCompletionId: string | undefined = undefined; - private readonly _selectedInlineCompletionIdChangeSignal = observableSignal('selectedCompletionIdChanged'); + private readonly _selectedInlineCompletionId = observableValue('selectedInlineCompletionId', undefined); public readonly selectedInlineCompletionIndex = derived('selectedCachedCompletionIndex', (reader) => { - this._selectedInlineCompletionIdChangeSignal.read(reader); + const selectedInlineCompletionId = this._selectedInlineCompletionId.read(reader); const filteredCompletions = this._filteredInlineCompletionItems.read(reader); - const idx = this._selectedInlineCompletionId === undefined - ? -1 - : filteredCompletions.findIndex(v => v.semanticId === this._selectedInlineCompletionId); + const idx = this._selectedInlineCompletionId === undefined ? -1 + : filteredCompletions.findIndex(v => v.semanticId === selectedInlineCompletionId); if (idx === -1) { // Reset the selection so that the selection does not jump back when it appears again - this._selectedInlineCompletionId = undefined; + this._selectedInlineCompletionId.set(undefined, undefined); return 0; } return idx; @@ -245,7 +247,7 @@ export class InlineCompletionsModel extends Disposable { return v.ghostText; }); - private async deltaIndex(delta: 1 | -1): Promise { + private async deltaSelectedInlineCompletionIndex(delta: 1 | -1): Promise { await this.triggerExplicitly(); this._isNavigatingCurrentInlineCompletion = true; @@ -253,22 +255,21 @@ export class InlineCompletionsModel extends Disposable { const completions = this._filteredInlineCompletionItems.get() || []; if (completions.length > 0) { const newIdx = (this.selectedInlineCompletionIndex.get() + delta + completions.length) % completions.length; - this._selectedInlineCompletionId = completions[newIdx].semanticId; + this._selectedInlineCompletionId.set(completions[newIdx].semanticId, undefined); } else { - this._selectedInlineCompletionId = undefined; + this._selectedInlineCompletionId.set(undefined, undefined); } - this._selectedInlineCompletionIdChangeSignal.trigger(undefined); } finally { this._isNavigatingCurrentInlineCompletion = false; } } public async next(): Promise { - await this.deltaIndex(1); + await this.deltaSelectedInlineCompletionIndex(1); } public async previous(): Promise { - await this.deltaIndex(-1); + await this.deltaSelectedInlineCompletionIndex(-1); } public accept(editor: ICodeEditor): void { @@ -320,7 +321,7 @@ export class InlineCompletionsModel extends Disposable { } public acceptNextWord(editor: ICodeEditor): void { - this.acceptNext(editor, (pos, text) => { + this._acceptNext(editor, (pos, text) => { const langId = this.textModel.getLanguageIdAtPosition(pos.lineNumber, pos.column); const config = this._languageConfigurationService.getLanguageConfiguration(langId); const wordRegExp = new RegExp(config.wordDefinition.source, config.wordDefinition.flags.replace('g', '')); @@ -349,7 +350,7 @@ export class InlineCompletionsModel extends Disposable { } public acceptNextLine(editor: ICodeEditor): void { - this.acceptNext(editor, (pos, text) => { + this._acceptNext(editor, (pos, text) => { const m = text.match(/\n/); if (m && m.index !== undefined) { return m.index + 1; @@ -358,7 +359,7 @@ export class InlineCompletionsModel extends Disposable { }); } - private acceptNext(editor: ICodeEditor, getAcceptUntilIndex: (position: Position, text: string) => number): void { + private _acceptNext(editor: ICodeEditor, getAcceptUntilIndex: (position: Position, text: string) => number): void { if (editor.getModel() !== this.textModel) { throw new BugIndicatingError(); } @@ -380,9 +381,7 @@ export class InlineCompletionsModel extends Disposable { } const firstPart = ghostText.parts[0]; const position = new Position(ghostText.lineNumber, firstPart.column); - const line = firstPart.lines.join('\n'); - const acceptUntilIndexExclusive = getAcceptUntilIndex(position, line); if (acceptUntilIndexExclusive === line.length && ghostText.parts.length === 1) { diff --git a/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsSource.ts b/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsSource.ts index 28bc5eed51df5..39c6b81b901ac 100644 --- a/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsSource.ts +++ b/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsSource.ts @@ -19,8 +19,7 @@ import { SingleTextEdit } from 'vs/editor/contrib/inlineCompletions/browser/sing import { InlineCompletionItem, InlineCompletionProviderResult, provideInlineCompletions } from 'vs/editor/contrib/inlineCompletions/browser/provideInlineCompletions'; export class InlineCompletionsSource extends Disposable { - private readonly updateOperation = this._register(new MutableDisposable()); - + private readonly _updateOperation = this._register(new MutableDisposable()); public readonly inlineCompletions = disposableObservableValue('inlineCompletions', undefined); public readonly suggestWidgetInlineCompletions = disposableObservableValue('suggestWidgetInlineCompletions', undefined); @@ -34,36 +33,23 @@ export class InlineCompletionsSource extends Disposable { super(); this._register(this.textModel.onDidChangeContent(() => { - this.updateOperation.clear(); + this._updateOperation.clear(); })); } - public clear(tx: ITransaction): void { - this.updateOperation.clear(); - this.inlineCompletions.set(undefined, tx); - this.suggestWidgetInlineCompletions.set(undefined, tx); - } - - public clearSuggestWidgetInlineCompletions(): void { - if (this.updateOperation.value?.request.context.selectedSuggestionInfo) { - this.updateOperation.clear(); - } - this.suggestWidgetInlineCompletions.set(undefined, undefined); - } - - public update(position: Position, context: InlineCompletionContext, activeInlineCompletion: InlineCompletionWithUpdatedRange | undefined): Promise { + public fetch(position: Position, context: InlineCompletionContext, activeInlineCompletion: InlineCompletionWithUpdatedRange | undefined): Promise { const request = new UpdateRequest(position, context, this.textModel.getVersionId()); const target = context.selectedSuggestionInfo ? this.suggestWidgetInlineCompletions : this.inlineCompletions; - if (this.updateOperation.value?.request.satisfies(request)) { - return this.updateOperation.value.promise; + if (this._updateOperation.value?.request.satisfies(request)) { + return this._updateOperation.value.promise; } else if (target.get()?.request.satisfies(request)) { return Promise.resolve(true); } - const updateOngoing = !!this.updateOperation.value; - this.updateOperation.clear(); + const updateOngoing = !!this._updateOperation.value; + this._updateOperation.clear(); const source = new CancellationTokenSource(); @@ -106,16 +92,33 @@ export class InlineCompletionsSource extends Disposable { transaction(tx => { target.set(completions, tx); }); - this.updateOperation.clear(); + this._updateOperation.clear(); return true; })(); const updateOperation = new UpdateOperation(request, source, promise); - this.updateOperation.value = updateOperation; + this._updateOperation.value = updateOperation; return promise; } + + public clear(tx: ITransaction): void { + this._updateOperation.clear(); + this.inlineCompletions.set(undefined, tx); + this.suggestWidgetInlineCompletions.set(undefined, tx); + } + + public clearSuggestWidgetInlineCompletions(tx: ITransaction): void { + if (this._updateOperation.value?.request.context.selectedSuggestionInfo) { + this._updateOperation.clear(); + } + this.suggestWidgetInlineCompletions.set(undefined, tx); + } + + public cancelUpdate(): void { + this._updateOperation.clear(); + } } function wait(ms: number, cancellationToken?: CancellationToken): Promise { @@ -176,20 +179,20 @@ export class UpToDateInlineCompletions implements IDisposable { private readonly _inlineCompletions: InlineCompletionWithUpdatedRange[]; public get inlineCompletions(): ReadonlyArray { return this._inlineCompletions; } - private refCount = 1; - private readonly prependedInlineCompletionItems: InlineCompletionItem[] = []; + private _refCount = 1; + private readonly _prependedInlineCompletionItems: InlineCompletionItem[] = []; - private counter = 0; - private readonly rangeVersion = derived('ranges', reader => { + private _rangeVersionIdValue = 0; + private readonly _rangeVersionId = derived('ranges', reader => { this.versionId.read(reader); let changed = false; for (const i of this._inlineCompletions) { changed = changed || i._updateRange(this.textModel); } if (changed) { - this.counter++; + this._rangeVersionIdValue++; } - return this.counter; + return this._rangeVersionIdValue; }); constructor( @@ -206,10 +209,26 @@ export class UpToDateInlineCompletions implements IDisposable { }))); this._inlineCompletions = inlineCompletionProviderResult.completions.map( - (i, index) => new InlineCompletionWithUpdatedRange(i, ids[index], this.rangeVersion) + (i, index) => new InlineCompletionWithUpdatedRange(i, ids[index], this._rangeVersionId) ); } + public clone(): this { + this._refCount++; + return this; + } + + public dispose(): void { + this._refCount--; + if (this._refCount === 0) { + this.textModel.deltaDecorations(this._inlineCompletions.map(i => i.decorationId), []); + this.inlineCompletionProviderResult.dispose(); + for (const i of this._prependedInlineCompletionItems) { + i.source.removeRef(); + } + } + } + public prepend(inlineCompletion: InlineCompletionItem, range: Range, addRefToSource: boolean): void { if (addRefToSource) { inlineCompletion.source.addRef(); @@ -221,32 +240,24 @@ export class UpToDateInlineCompletions implements IDisposable { description: 'inline-completion-tracking-range' }, }])[0]; - this._inlineCompletions.unshift(new InlineCompletionWithUpdatedRange(inlineCompletion, id, this.rangeVersion, range)); - this.prependedInlineCompletionItems.push(inlineCompletion); - } - - public clone(): this { - this.refCount++; - return this; - } - - public dispose(): void { - this.refCount--; - if (this.refCount === 0) { - this.textModel.deltaDecorations(this._inlineCompletions.map(i => i.decorationId), []); - this.inlineCompletionProviderResult.dispose(); - for (const i of this.prependedInlineCompletionItems) { - i.source.removeRef(); - } - } + this._inlineCompletions.unshift(new InlineCompletionWithUpdatedRange(inlineCompletion, id, this._rangeVersionId, range)); + this._prependedInlineCompletionItems.push(inlineCompletion); } } export class InlineCompletionWithUpdatedRange { - public readonly semanticId = JSON.stringify([this.inlineCompletion.filterText, this.inlineCompletion.insertText, this.inlineCompletion.range.getStartPosition().toString()]); + public readonly semanticId = JSON.stringify([ + this.inlineCompletion.filterText, + this.inlineCompletion.insertText, + this.inlineCompletion.range.getStartPosition().toString() + ]); private _updatedRange: Range; private _isValid = true; + public get forwardStable() { + return this.inlineCompletion.source.inlineCompletions.enableForwardStability ?? false; + } + constructor( public readonly inlineCompletion: InlineCompletionItem, public readonly decorationId: string, @@ -256,39 +267,20 @@ export class InlineCompletionWithUpdatedRange { this._updatedRange = initialRange ?? inlineCompletion.range; } - private getUpdatedRange(reader: IReader | undefined): Range { - this.rangeVersion.read(reader); // This makes sure all the ranges are updated. - return this._updatedRange; - } - - public _updateRange(textModel: ITextModel): boolean { - const range = textModel.getDecorationRange(this.decorationId); - if (!range) { - // A setValue call might flush all decorations. - this._isValid = false; - return true; - } - if (!this._updatedRange.equalsRange(range)) { - this._updatedRange = range; - return true; - } - return false; - } - public toInlineCompletion(reader: IReader | undefined): InlineCompletionItem { - return this.inlineCompletion.withRange(this.getUpdatedRange(reader)); + return this.inlineCompletion.withRange(this._getUpdatedRange(reader)); } public toSingleTextEdit(reader: IReader | undefined): SingleTextEdit { - return new SingleTextEdit(this.getUpdatedRange(reader), this.inlineCompletion.insertText); + return new SingleTextEdit(this._getUpdatedRange(reader), this.inlineCompletion.insertText); } public isVisible(model: ITextModel, cursorPosition: Position, reader: IReader | undefined): boolean { - const minimizedReplacement = this.toFilterTextReplacement(reader).removeCommonPrefix(model); + const minimizedReplacement = this._toFilterTextReplacement(reader).removeCommonPrefix(model); if ( !this._isValid - || !this.inlineCompletion.range.getStartPosition().equals(this.getUpdatedRange(reader).getStartPosition()) + || !this.inlineCompletion.range.getStartPosition().equals(this._getUpdatedRange(reader).getStartPosition()) || cursorPosition.lineNumber !== minimizedReplacement.range.startLineNumber ) { return false; @@ -322,20 +314,39 @@ export class InlineCompletionWithUpdatedRange { && !!matchesSubString(originalValueAfter, filterTextAfter); } - private toFilterTextReplacement(reader: IReader | undefined): SingleTextEdit { - return new SingleTextEdit(this.getUpdatedRange(reader), this.inlineCompletion.filterText); - } - public canBeReused(model: ITextModel, position: Position): boolean { const result = this._isValid - && this.getUpdatedRange(undefined).containsPosition(position) + && this._getUpdatedRange(undefined).containsPosition(position) && this.isVisible(model, position, undefined) - && !this.isSmallerThanOriginal(undefined); + && !this._isSmallerThanOriginal(undefined); return result; } - private isSmallerThanOriginal(reader: IReader | undefined): boolean { - return length(this.getUpdatedRange(reader)).isBefore(length(this.inlineCompletion.range)); + private _toFilterTextReplacement(reader: IReader | undefined): SingleTextEdit { + return new SingleTextEdit(this._getUpdatedRange(reader), this.inlineCompletion.filterText); + } + + private _isSmallerThanOriginal(reader: IReader | undefined): boolean { + return length(this._getUpdatedRange(reader)).isBefore(length(this.inlineCompletion.range)); + } + + private _getUpdatedRange(reader: IReader | undefined): Range { + this.rangeVersion.read(reader); // This makes sure all the ranges are updated. + return this._updatedRange; + } + + public _updateRange(textModel: ITextModel): boolean { + const range = textModel.getDecorationRange(this.decorationId); + if (!range) { + // A setValue call might flush all decorations. + this._isValid = false; + return true; + } + if (!this._updatedRange.equalsRange(range)) { + this._updatedRange = range; + return true; + } + return false; } } From 35c1b431623a282cd52909a6dc37cc3cc5acd934 Mon Sep 17 00:00:00 2001 From: Henning Dieterichs Date: Fri, 28 Apr 2023 17:07:35 +0200 Subject: [PATCH 2/3] Fixes begin/end update bug --- src/vs/base/common/observableImpl/autorun.ts | 5 +++ src/vs/base/common/observableImpl/base.ts | 2 -- src/vs/base/common/observableImpl/derived.ts | 18 ++++++++-- src/vs/base/test/common/observable.test.ts | 36 ++++++++++++++++++++ 4 files changed, 57 insertions(+), 4 deletions(-) diff --git a/src/vs/base/common/observableImpl/autorun.ts b/src/vs/base/common/observableImpl/autorun.ts index 43d2b2218478f..423192914b6aa 100644 --- a/src/vs/base/common/observableImpl/autorun.ts +++ b/src/vs/base/common/observableImpl/autorun.ts @@ -3,6 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ +import { BugIndicatingError } from 'vs/base/common/errors'; import { DisposableStore, IDisposable, toDisposable } from 'vs/base/common/lifecycle'; import { IReader, IObservable, IObserver, IChangeContext } from 'vs/base/common/observableImpl/base'; import { getLogger } from 'vs/base/common/observableImpl/logging'; @@ -138,6 +139,10 @@ export class AutorunObserver implements IObserver, IReader } while (this.state !== AutorunState.upToDate); } this.updateCount--; + + if (this.updateCount < 0) { + throw new BugIndicatingError(); + } } public handlePossibleChange(observable: IObservable): void { diff --git a/src/vs/base/common/observableImpl/base.ts b/src/vs/base/common/observableImpl/base.ts index b8d60522bf24e..3de2a5a91af25 100644 --- a/src/vs/base/common/observableImpl/base.ts +++ b/src/vs/base/common/observableImpl/base.ts @@ -171,7 +171,6 @@ export abstract class ConvenientObservable implements IObservable extends ConvenientObservable { protected readonly observers = new Set(); - /** @sealed */ public addObserver(observer: IObserver): void { const len = this.observers.size; this.observers.add(observer); @@ -180,7 +179,6 @@ export abstract class BaseObservable extends ConvenientObserv } } - /** @sealed */ public removeObserver(observer: IObserver): void { const deleted = this.observers.delete(observer); if (deleted && this.observers.size === 0) { diff --git a/src/vs/base/common/observableImpl/derived.ts b/src/vs/base/common/observableImpl/derived.ts index 8d912a1d45000..29df8e0154f2f 100644 --- a/src/vs/base/common/observableImpl/derived.ts +++ b/src/vs/base/common/observableImpl/derived.ts @@ -161,7 +161,7 @@ export class Derived extends BaseObservable im } // IObserver Implementation - public beginUpdate(): void { + public beginUpdate(_observable: IObservable): void { this.updateCount++; const propagateBeginUpdate = this.updateCount === 1; if (this.state === DerivedState.upToDate) { @@ -180,7 +180,7 @@ export class Derived extends BaseObservable im } } - public endUpdate(): void { + public endUpdate(_observable: IObservable): void { this.updateCount--; if (this.updateCount === 0) { // End update could change the observer list. @@ -237,4 +237,18 @@ export class Derived extends BaseObservable im this.dependenciesToBeRemoved.delete(observable); return value; } + + public override addObserver(observer: IObserver): void { + if (!this.observers.has(observer) && this.updateCount > 0) { + observer.beginUpdate(this); + } + super.addObserver(observer); + } + + public override removeObserver(observer: IObserver): void { + if (this.observers.has(observer) && this.updateCount > 0) { + observer.endUpdate(this); + } + super.removeObserver(observer); + } } diff --git a/src/vs/base/test/common/observable.test.ts b/src/vs/base/test/common/observable.test.ts index 8c987c30e39f5..64d08a6c4d55a 100644 --- a/src/vs/base/test/common/observable.test.ts +++ b/src/vs/base/test/common/observable.test.ts @@ -926,6 +926,42 @@ suite('observables', () => { 'myAutorun(myDerived3: 1 + 0)', ]); }); + + test('bug: Add observable in endUpdate', () => { + const myObservable1 = observableValue('myObservable1', 0); + const myObservable2 = observableValue('myObservable2', 0); + + const myDerived1 = derived('myDerived1', reader => { + return myObservable1.read(reader); + }); + + const myDerived2 = derived('myDerived2', reader => { + return myObservable2.read(reader); + }); + + const myDerivedA1 = derived('myDerivedA1', reader => { + const d1 = myDerived1.read(reader); + if (d1 === 1) { + // This adds an observer while myDerived is still in update mode. + // When myDerived exits update mode, the observer shouldn't receive + // more endUpdate than beginUpdate calls. + myDerived2.read(reader); + } + }); + + autorun('myAutorun1', reader => { + myDerivedA1.read(reader); + }); + + autorun('myAutorun2', reader => { + myDerived2.read(reader); + }); + + transaction(tx => { + myObservable1.set(1, tx); + myObservable2.set(1, tx); + }); + }); }); export class LoggingObserver implements IObserver { From f2d0b17ff165a628c1af822e4ffdc4489d6afa81 Mon Sep 17 00:00:00 2001 From: Henning Dieterichs Date: Fri, 28 Apr 2023 19:24:15 +0200 Subject: [PATCH 3/3] Minor code improvements --- src/vs/base/common/observableImpl/base.ts | 2 +- src/vs/base/common/observableImpl/utils.ts | 26 +++-- .../browser/inlineCompletionsController.ts | 95 ++++++++++--------- .../browser/inlineCompletionsModel.ts | 63 ++++++------ 4 files changed, 98 insertions(+), 88 deletions(-) diff --git a/src/vs/base/common/observableImpl/base.ts b/src/vs/base/common/observableImpl/base.ts index 3de2a5a91af25..7fc48ddf273b5 100644 --- a/src/vs/base/common/observableImpl/base.ts +++ b/src/vs/base/common/observableImpl/base.ts @@ -267,7 +267,7 @@ export class ObservableValue } public set(value: T, tx: ITransaction | undefined, change: TChange): void { - if (this._value === value && change === undefined) { + if (this._value === value) { return; } diff --git a/src/vs/base/common/observableImpl/utils.ts b/src/vs/base/common/observableImpl/utils.ts index c848dde53ff73..093f78773ec97 100644 --- a/src/vs/base/common/observableImpl/utils.ts +++ b/src/vs/base/common/observableImpl/utils.ts @@ -276,15 +276,16 @@ export function wasEventTriggeredRecently(event: Event, timeoutMs: number, } /** - * This ensures the observable cache is kept up-to-date, even if there are no subscribers. - * This is useful when the observables `get` method is used, but not its `read` method. + * This ensures the observable is being observed. + * Observed observables (such as {@link derived}s) can maintain a cache, as they receive invalidation events. + * Unobserved observables are forced to recompute their value from scratch every time they are read. * - * (Usually, when no one is actually observing the observable, getting its value will - * compute it from scratch, as the cache cannot be trusted: - * Because no one is actually observing its value, keeping the cache up-to-date would be too expensive) + * @param observable the observable to keep alive + * @param forceRecompute if true, the observable will be eagerly recomputed after it changed. + * Use this if recomputing the observables causes side-effects. */ -export function keepAlive(observable: IObservable): IDisposable { - const o = new KeepAliveObserver(); +export function keepAlive(observable: IObservable, forceRecompute?: boolean): IDisposable { + const o = new KeepAliveObserver(forceRecompute ?? false); observable.addObserver(o); return toDisposable(() => { observable.removeObserver(o); @@ -292,12 +293,19 @@ export function keepAlive(observable: IObservable): IDisposable { } class KeepAliveObserver implements IObserver { + private counter = 0; + + constructor(private readonly forceRecompute: boolean) { } + beginUpdate(observable: IObservable): void { - // NO OP + this.counter++; } endUpdate(observable: IObservable): void { - // NO OP + this.counter--; + if (this.counter === 0 && this.forceRecompute) { + observable.reportChanges(); + } } handlePossibleChange(observable: IObservable): void { diff --git a/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsController.ts b/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsController.ts index 9cf5ea4e380a1..18746e401ea38 100644 --- a/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsController.ts +++ b/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsController.ts @@ -16,6 +16,7 @@ import { Range } from 'vs/editor/common/core/range'; import { CursorChangeReason } from 'vs/editor/common/cursorEvents'; import { ILanguageFeatureDebounceService } from 'vs/editor/common/services/languageFeatureDebounce'; import { ILanguageFeaturesService } from 'vs/editor/common/services/languageFeatures'; +import { IModelContentChangedEvent } from 'vs/editor/common/textModelEvents'; import { inlineSuggestCommitId } from 'vs/editor/contrib/inlineCompletions/browser/commandIds'; import { GhostTextWidget } from 'vs/editor/contrib/inlineCompletions/browser/ghostTextWidget'; import { InlineCompletionContextKeys } from 'vs/editor/contrib/inlineCompletions/browser/inlineCompletionContextKeys'; @@ -35,15 +36,15 @@ export class InlineCompletionsController extends Disposable { return editor.getContribution(InlineCompletionsController.ID); } + public readonly model = disposableObservableValue('inlineCompletionModel', undefined); + private readonly textModelVersionId = observableValue('textModelVersionId', -1); + private readonly cursorPosition = observableValue('cursorPosition', new Position(1, 1)); private readonly suggestWidgetAdaptor = this._register(new SuggestWidgetAdaptor( this.editor, () => this.model.get()?.selectedInlineCompletion.get()?.toSingleTextEdit(undefined), (tx) => this.updateObservables(tx, VersionIdChangeReason.Other) )); - - private readonly textModelVersionId = observableValue('textModelVersionId', -1); - private readonly cursorPosition = observableValue('cursorPosition', new Position(1, 1)); - public readonly model = disposableObservableValue('inlineCompletionModel', undefined); + private readonly _enabled = observableFromEvent(this.editor.onDidChangeConfiguration, () => this.editor.getOption(EditorOption.inlineSuggest).enabled); private ghostTextWidget = this._register(this.instantiationService.createInstance(GhostTextWidget, this.editor, { ghostText: this.model.map((v, reader) => v?.ghostText.read(reader)), @@ -57,8 +58,6 @@ export class InlineCompletionsController extends Disposable { { min: 50, max: 50 } ); - private readonly _enabled = observableFromEvent(this.editor.onDidChangeConfiguration, () => this.editor.getOption(EditorOption.inlineSuggest).enabled); - constructor( public readonly editor: ICodeEditor, @IInstantiationService private readonly instantiationService: IInstantiationService, @@ -74,6 +73,7 @@ export class InlineCompletionsController extends Disposable { this._register(new InlineCompletionContextKeys(this.contextKeyService, this.model)); this._register(Event.runAndSubscribe(editor.onDidChangeModel, () => transaction(tx => { + /** @description onDidChangeModel */ this.model.set(undefined, tx); this.updateObservables(tx, VersionIdChangeReason.Other); @@ -95,16 +95,19 @@ export class InlineCompletionsController extends Disposable { } }))); + const getReason = (e: IModelContentChangedEvent): VersionIdChangeReason => { + if (e.isUndoing) { return VersionIdChangeReason.Undo; } + if (e.isRedoing) { return VersionIdChangeReason.Redo; } + if (this.model.get()?.isAcceptingPartially) { return VersionIdChangeReason.AcceptWord; } + return VersionIdChangeReason.Other; + }; this._register(editor.onDidChangeModelContent((e) => transaction(tx => - this.updateObservables(tx, - e.isUndoing ? VersionIdChangeReason.Undo - : e.isRedoing ? VersionIdChangeReason.Redo - : this.model.get()?.isAcceptingPartially ? VersionIdChangeReason.AcceptWord - : VersionIdChangeReason.Other - ) + /** @description onDidChangeModelContent */ + this.updateObservables(tx, getReason(e)) ))); this._register(editor.onDidChangeCursorPosition(e => transaction(tx => { + /** @description onDidChangeCursorPosition */ this.updateObservables(tx, VersionIdChangeReason.Other); if (e.reason === CursorChangeReason.Explicit) { this.model.get()?.stop(tx); @@ -112,29 +115,29 @@ export class InlineCompletionsController extends Disposable { }))); this._register(editor.onDidType(() => transaction(tx => { + /** @description onDidType */ this.updateObservables(tx, VersionIdChangeReason.Other); if (this._enabled.get()) { this.model.get()?.trigger(tx); } }))); - this._register( - this.commandService.onDidExecuteCommand((e) => { - // These commands don't trigger onDidType. - const commands = new Set([ - CoreEditingCommands.Tab.id, - CoreEditingCommands.DeleteLeft.id, - CoreEditingCommands.DeleteRight.id, - inlineSuggestCommitId, - 'acceptSelectedSuggestion', - ]); - if (commands.has(e.commandId) && editor.hasTextFocus() && this._enabled.get()) { - transaction(tx => { - this.model.get()?.trigger(tx); - }); - } - }) - ); + this._register(this.commandService.onDidExecuteCommand((e) => { + // These commands don't trigger onDidType. + const commands = new Set([ + CoreEditingCommands.Tab.id, + CoreEditingCommands.DeleteLeft.id, + CoreEditingCommands.DeleteRight.id, + inlineSuggestCommitId, + 'acceptSelectedSuggestion', + ]); + if (commands.has(e.commandId) && editor.hasTextFocus() && this._enabled.get()) { + transaction(tx => { + /** @description onDidExecuteCommand */ + this.model.get()?.trigger(tx); + }); + } + })); this._register(this.editor.onDidBlurEditorWidget(() => { // This is a hidden setting very useful for debugging @@ -146,23 +149,21 @@ export class InlineCompletionsController extends Disposable { return; } transaction(tx => { + /** @description onDidBlurEditorWidget */ this.model.get()?.stop(tx); }); })); this._register(autorun('forceRenderingAbove', reader => { - const model = this.model.read(reader); - const ghostText = model?.ghostText.read(reader); - const selectedSuggestItem = this.suggestWidgetAdaptor.selectedItem.read(reader); - if (selectedSuggestItem) { - if (ghostText && ghostText.lineCount >= 2) { + const state = this.model.read(reader)?.state.read(reader); + if (state?.suggestItem) { + if (state.ghostText.lineCount >= 2) { this.suggestWidgetAdaptor.forceRenderingAbove(); } } else { this.suggestWidgetAdaptor.stopForceRenderingAbove(); } })); - this._register(toDisposable(() => { this.suggestWidgetAdaptor.stopForceRenderingAbove(); })); @@ -170,33 +171,33 @@ export class InlineCompletionsController extends Disposable { let lastInlineCompletionId: string | undefined = undefined; this._register(autorun('play audio cue & read suggestion', reader => { const model = this.model.read(reader); - const currentInlineCompletion = model?.selectedInlineCompletion.read(reader); - if (!model || !currentInlineCompletion) { + const state = model?.state.read(reader); + if (!model || !state || !state.completion) { lastInlineCompletionId = undefined; return; } - const ghostText = model?.ghostText.get(); - if (!ghostText) { - lastInlineCompletionId = undefined; - return; - } - const lineText = model.textModel.getLineContent(ghostText.lineNumber); - - if (currentInlineCompletion.semanticId !== lastInlineCompletionId) { - lastInlineCompletionId = currentInlineCompletion.semanticId; + if (state.completion.semanticId !== lastInlineCompletionId) { + lastInlineCompletionId = state.completion.semanticId; if (model.isNavigatingCurrentInlineCompletion) { return; } + this.audioCueService.playAudioCue(AudioCue.inlineSuggestion).then(() => { if (this.editor.getOption(EditorOption.screenReaderAnnounceInlineSuggestion)) { - alert(ghostText.renderForScreenReader(lineText)); + const lineText = model.textModel.getLineContent(state.ghostText.lineNumber); + alert(state.ghostText.renderForScreenReader(lineText)); } }); } })); } + /** + * Copies over the relevant state from the text model to observables. + * This solves all kind of eventing issues, as we make sure we always operate on the latest state, + * regardless of who calls into us. + */ private updateObservables(tx: ITransaction, changeReason: VersionIdChangeReason): void { const newModel = this.editor.getModel(); this.textModelVersionId.set(newModel?.getVersionId() ?? -1, tx, changeReason); diff --git a/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsModel.ts b/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsModel.ts index 09bf41957d862..7fee3ff3f93ab 100644 --- a/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsModel.ts +++ b/src/vs/editor/contrib/inlineCompletions/browser/inlineCompletionsModel.ts @@ -6,7 +6,7 @@ import { mapFind } from 'vs/base/common/arrays'; import { BugIndicatingError, onUnexpectedExternalError } from 'vs/base/common/errors'; import { Disposable } from 'vs/base/common/lifecycle'; -import { IObservable, ITransaction, autorun, derived, observableSignal, observableValue, transaction } from 'vs/base/common/observable'; +import { IObservable, ITransaction, autorun, derived, keepAlive, observableSignal, observableValue, transaction } from 'vs/base/common/observable'; import { subtransaction } from 'vs/base/common/observableImpl/base'; import { derivedHandleChanges } from 'vs/base/common/observableImpl/derived'; import { isDefined } from 'vs/base/common/types'; @@ -18,7 +18,7 @@ import { InlineCompletionContext, InlineCompletionTriggerKind } from 'vs/editor/ import { ILanguageConfigurationService } from 'vs/editor/common/languages/languageConfigurationRegistry'; import { EndOfLinePreference, ITextModel } from 'vs/editor/common/model'; import { IFeatureDebounceInformation } from 'vs/editor/common/services/languageFeatureDebounce'; -import { GhostText } from 'vs/editor/contrib/inlineCompletions/browser/ghostText'; +import { GhostText, GhostTextOrReplacement } from 'vs/editor/contrib/inlineCompletions/browser/ghostText'; import { InlineCompletionWithUpdatedRange, InlineCompletionsSource } from 'vs/editor/contrib/inlineCompletions/browser/inlineCompletionsSource'; import { SuggestItemInfo } from 'vs/editor/contrib/inlineCompletions/browser/suggestWidgetInlineCompletionProvider'; import { addPositions, lengthOfText } from 'vs/editor/contrib/inlineCompletions/browser/utils'; @@ -38,6 +38,9 @@ export class InlineCompletionsModel extends Disposable { private readonly _isActive = observableValue('isActive', false); private readonly _forceUpdate = observableSignal('forceUpdate'); + // We use a semantic id to keep the same inline completion selected even if the provider reorders the completions. + private readonly _selectedInlineCompletionId = observableValue('selectedInlineCompletionId', undefined); + private _isAcceptingPartially = false; public get isAcceptingPartially() { return this._isAcceptingPartially; } @@ -60,13 +63,11 @@ export class InlineCompletionsModel extends Disposable { ) { super(); - this._register(autorun('automatically fetch inline completions', (reader) => { - this.fetchInlineCompletions.read(reader); - })); + this._register(keepAlive(this._fetchInlineCompletions, true)); let lastItem: InlineCompletionWithUpdatedRange | undefined = undefined; this._register(autorun('call handleItemDidShow', reader => { - const item = this.ghostTextAndCompletion.read(reader); + const item = this.state.read(reader); const completion = item?.completion; if (completion?.semanticId !== lastItem?.semanticId) { lastItem = completion; @@ -79,19 +80,18 @@ export class InlineCompletionsModel extends Disposable { })); } - private readonly preserveCurrentCompletionReasons = new Set([ + private readonly _preserveCurrentCompletionReasons = new Set([ VersionIdChangeReason.Redo, VersionIdChangeReason.Undo, VersionIdChangeReason.AcceptWord, ]); - - private readonly fetchInlineCompletions = derivedHandleChanges('fetch inline completions', { + private readonly _fetchInlineCompletions = derivedHandleChanges('fetch inline completions', { createEmptyChangeSummary: () => ({ preserveCurrentCompletion: false, inlineCompletionTriggerKind: InlineCompletionTriggerKind.Automatic }), handleChange: (ctx, changeSummary) => { - if (ctx.didChange(this.textModelVersionId) && this.preserveCurrentCompletionReasons.has(ctx.change)) { + if (ctx.didChange(this.textModelVersionId) && this._preserveCurrentCompletionReasons.has(ctx.change)) { changeSummary.preserveCurrentCompletion = true; } else if (ctx.didChange(this._forceUpdate)) { changeSummary.inlineCompletionTriggerKind = ctx.change; @@ -106,7 +106,7 @@ export class InlineCompletionsModel extends Disposable { return undefined; } - this.textModelVersionId.read(reader); + this.textModelVersionId.read(reader); // Refetch on text change const itemToPreserveCandidate = this.selectedInlineCompletion.get(); const itemToPreserve = changeSummary.preserveCurrentCompletion || itemToPreserveCandidate?.forwardStable @@ -117,7 +117,7 @@ export class InlineCompletionsModel extends Disposable { if (suggestWidgetInlineCompletions && !suggestItem) { const inlineCompletions = this._source.inlineCompletions.get(); transaction(tx => { - /** @summary Seed inline completions with (newer) suggest widget inline completions */ + /** @description Seed inline completions with (newer) suggest widget inline completions */ if (inlineCompletions && suggestWidgetInlineCompletions.request.versionId > inlineCompletions.request.versionId) { this._source.inlineCompletions.set(suggestWidgetInlineCompletions.clone(), tx); } @@ -135,7 +135,7 @@ export class InlineCompletionsModel extends Disposable { public async trigger(tx?: ITransaction): Promise { this._isActive.set(true, tx); - await this.fetchInlineCompletions.get(); + await this._fetchInlineCompletions.get(); } public async triggerExplicitly(tx?: ITransaction): Promise { @@ -143,16 +143,14 @@ export class InlineCompletionsModel extends Disposable { this._isActive.set(true, tx); this._forceUpdate.trigger(tx, InlineCompletionTriggerKind.Explicit); }); - await this.fetchInlineCompletions.get(); + await this._fetchInlineCompletions.get(); } public stop(tx?: ITransaction): void { - if (!tx) { - transaction(tx => this.stop(tx)); - return; - } - this._isActive.set(false, tx); - this._source.clear(tx); + subtransaction(tx, tx => { + this._isActive.set(false, tx); + this._source.clear(tx); + }); } private readonly _filteredInlineCompletionItems = derived('filteredInlineCompletionItems', (reader) => { @@ -163,9 +161,6 @@ export class InlineCompletionsModel extends Disposable { return filteredCompletions; }); - // We use a semantic id to keep the same inline completion selected even if the provider reorders the completions. - private readonly _selectedInlineCompletionId = observableValue('selectedInlineCompletionId', undefined); - public readonly selectedInlineCompletionIndex = derived('selectedCachedCompletionIndex', (reader) => { const selectedInlineCompletionId = this._selectedInlineCompletionId.read(reader); const filteredCompletions = this._filteredInlineCompletionItems.read(reader); @@ -185,7 +180,9 @@ export class InlineCompletionsModel extends Disposable { return filteredCompletions[idx]; }); - public readonly lastTriggerKind = this._source.inlineCompletions.map(v => v?.request.context.triggerKind); + public readonly lastTriggerKind: IObservable = this._source.inlineCompletions.map( + v => /** @description lastTriggerKind */ v?.request.context.triggerKind + ); public readonly inlineCompletionsCount = derived('selectedInlineCompletionsCount', reader => { if (this.lastTriggerKind.read(reader) === InlineCompletionTriggerKind.Explicit) { @@ -195,7 +192,11 @@ export class InlineCompletionsModel extends Disposable { } }); - public readonly ghostTextAndCompletion = derived('ghostTextAndCompletion', (reader) => { + public readonly state = derived<{ + suggestItem: SuggestItemInfo | undefined; + completion: InlineCompletionWithUpdatedRange | undefined; + ghostText: GhostTextOrReplacement; + } | undefined>('ghostTextAndCompletion', (reader) => { const model = this.textModel; const suggestItem = this.selectedSuggestItem.read(reader); @@ -227,7 +228,7 @@ export class InlineCompletionsModel extends Disposable { // Show an invisible ghost text to reserve space const ghostText = newGhostText ?? new GhostText(edit.range.endLineNumber, []); - return { ghostText, completion: augmentedCompletion?.completion }; + return { ghostText, completion: augmentedCompletion?.completion, suggestItem }; } else { if (!this._isActive.read(reader)) { return undefined; } const item = this.selectedInlineCompletion.read(reader); @@ -237,17 +238,17 @@ export class InlineCompletionsModel extends Disposable { const mode = this._inlineSuggestMode.read(reader); const cursor = this.cursorPosition.read(reader); const ghostText = replacement.computeGhostText(model, mode, cursor); - return ghostText ? { ghostText, completion: item } : undefined; + return ghostText ? { ghostText, completion: item, suggestItem: undefined } : undefined; } }); public readonly ghostText = derived('ghostText', (reader) => { - const v = this.ghostTextAndCompletion.read(reader); + const v = this.state.read(reader); if (!v) { return undefined; } return v.ghostText; }); - private async deltaSelectedInlineCompletionIndex(delta: 1 | -1): Promise { + private async _deltaSelectedInlineCompletionIndex(delta: 1 | -1): Promise { await this.triggerExplicitly(); this._isNavigatingCurrentInlineCompletion = true; @@ -265,11 +266,11 @@ export class InlineCompletionsModel extends Disposable { } public async next(): Promise { - await this.deltaSelectedInlineCompletionIndex(1); + await this._deltaSelectedInlineCompletionIndex(1); } public async previous(): Promise { - await this.deltaSelectedInlineCompletionIndex(-1); + await this._deltaSelectedInlineCompletionIndex(-1); } public accept(editor: ICodeEditor): void {