Skip to content

Commit

Permalink
WordHighlighter: fix queries against disposed models + blur logic (mi…
Browse files Browse the repository at this point in the history 
…crosoft#227607)

* wordHighlighter: fix queries against disposed models (use URIs)

* add blur logic + clear old workerReq logic from when highlighting was only singleFile

* imports

* dispose models that we ref for the occurrence request

* WHOOPS don't do that, it destroys every editor hah
  • Loading branch information
@Yoyokrazy
Yoyokrazy authored Sep 9, 2024
1 parent dcc7af1 commit 6092f24
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 73 deletions.
162 changes: 101 additions & 61 deletions src/vs/editor/contrib/wordHighlighter/browser/wordHighlighter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ import { CancellationToken } from '../../../../base/common/cancellation.js';
import { onUnexpectedError, onUnexpectedExternalError } from '../../../../base/common/errors.js';
import { KeyCode, KeyMod } from '../../../../base/common/keyCodes.js';
import { Disposable, DisposableStore } from '../../../../base/common/lifecycle.js';
import { ResourceMap } from '../../../../base/common/map.js';
import { matchesScheme, Schemas } from '../../../../base/common/network.js';
import { isEqual } from '../../../../base/common/resources.js';
import { URI } from '../../../../base/common/uri.js';
import { IContextKey, IContextKeyService, RawContextKey } from '../../../../platform/contextkey/common/contextkey.js';
import { ServicesAccessor } from '../../../../platform/instantiation/common/instantiation.js';
import { KeybindingWeight } from '../../../../platform/keybinding/common/keybindingsRegistry.js';
import { IActiveCodeEditor, ICodeEditor, isDiffEditor } from '../../../browser/editorBrowser.js';
import { EditorAction, EditorContributionInstantiation, IActionOptions, registerEditorAction, registerEditorContribution, registerModelAndPositionCommand } from '../../../browser/editorExtensions.js';
import { ICodeEditorService } from '../../../browser/services/codeEditorService.js';
Expand All @@ -21,20 +28,15 @@ import { IWordAtPosition } from '../../../common/core/wordHelper.js';
import { CursorChangeReason, ICursorPositionChangedEvent } from '../../../common/cursorEvents.js';
import { IDiffEditor, IEditorContribution, IEditorDecorationsCollection } from '../../../common/editorCommon.js';
import { EditorContextKeys } from '../../../common/editorContextKeys.js';
import { registerEditorFeature } from '../../../common/editorFeatures.js';
import { LanguageFeatureRegistry } from '../../../common/languageFeatureRegistry.js';
import { DocumentHighlight, DocumentHighlightProvider, MultiDocumentHighlightProvider } from '../../../common/languages.js';
import { score } from '../../../common/languageSelector.js';
import { IModelDeltaDecoration, ITextModel, shouldSynchronizeModel } from '../../../common/model.js';
import { ILanguageFeaturesService } from '../../../common/services/languageFeatures.js';
import { ITextModelService } from '../../../common/services/resolverService.js';
import { getHighlightDecorationOptions } from './highlightDecorations.js';
import { IContextKey, IContextKeyService, RawContextKey } from '../../../../platform/contextkey/common/contextkey.js';
import { ServicesAccessor } from '../../../../platform/instantiation/common/instantiation.js';
import { KeybindingWeight } from '../../../../platform/keybinding/common/keybindingsRegistry.js';
import { matchesScheme, Schemas } from '../../../../base/common/network.js';
import { ResourceMap } from '../../../../base/common/map.js';
import { score } from '../../../common/languageSelector.js';
import { isEqual } from '../../../../base/common/resources.js';
import { TextualMultiDocumentHighlightFeature } from './textualHighlightProvider.js';
import { registerEditorFeature } from '../../../common/editorFeatures.js';

const ctxHasWordHighlights = new RawContextKey<boolean>('hasWordHighlights', false);

Expand All @@ -58,7 +60,7 @@ export function getOccurrencesAtPosition(registry: LanguageFeatureRegistry<Docum
});
}

export function getOccurrencesAcrossMultipleModels(registry: LanguageFeatureRegistry<MultiDocumentHighlightProvider>, model: ITextModel, position: Position, wordSeparators: string, token: CancellationToken, otherModels: ITextModel[]): Promise<ResourceMap<DocumentHighlight[]> | null | undefined> {
export function getOccurrencesAcrossMultipleModels(registry: LanguageFeatureRegistry<MultiDocumentHighlightProvider>, model: ITextModel, position: Position, token: CancellationToken, otherModels: ITextModel[]): Promise<ResourceMap<DocumentHighlight[]> | null | undefined> {
const orderedByScore = registry.ordered(model);

// in order of score ask the occurrences provider
Expand All @@ -84,10 +86,9 @@ interface IOccurenceAtPositionRequest {

interface IWordHighlighterQuery {
modelInfo: {
model: ITextModel;
modelURI: URI;
selection: Selection;
} | null;
readonly word: IWordAtPosition | null;
}

abstract class OccurenceAtPositionRequest implements IOccurenceAtPositionRequest {
Expand Down Expand Up @@ -175,7 +176,7 @@ class MultiModelOccurenceRequest extends OccurenceAtPositionRequest {
}

protected override _compute(model: ITextModel, selection: Selection, wordSeparators: string, token: CancellationToken): Promise<ResourceMap<DocumentHighlight[]>> {
return getOccurrencesAcrossMultipleModels(this._providers, model, selection.getPosition(), wordSeparators, token, this._otherModels).then(value => {
return getOccurrencesAcrossMultipleModels(this._providers, model, selection.getPosition(), token, this._otherModels).then(value => {
if (!value) {
return new ResourceMap<DocumentHighlight[]>();
}
Expand All @@ -185,11 +186,11 @@ class MultiModelOccurenceRequest extends OccurenceAtPositionRequest {
}


function computeOccurencesAtPosition(registry: LanguageFeatureRegistry<DocumentHighlightProvider>, model: ITextModel, selection: Selection, word: IWordAtPosition | null, wordSeparators: string): IOccurenceAtPositionRequest {
function computeOccurencesAtPosition(registry: LanguageFeatureRegistry<DocumentHighlightProvider>, model: ITextModel, selection: Selection, wordSeparators: string): IOccurenceAtPositionRequest {
return new SemanticOccurenceAtPositionRequest(model, selection, wordSeparators, registry);
}

function computeOccurencesMultiModel(registry: LanguageFeatureRegistry<MultiDocumentHighlightProvider>, model: ITextModel, selection: Selection, word: IWordAtPosition | null, wordSeparators: string, otherModels: ITextModel[]): IOccurenceAtPositionRequest {
function computeOccurencesMultiModel(registry: LanguageFeatureRegistry<MultiDocumentHighlightProvider>, model: ITextModel, selection: Selection, wordSeparators: string, otherModels: ITextModel[]): IOccurenceAtPositionRequest {
return new MultiModelOccurenceRequest(model, selection, wordSeparators, registry, otherModels);
}

Expand All @@ -207,7 +208,10 @@ class WordHighlighter {
private readonly model: ITextModel;
private readonly decorations: IEditorDecorationsCollection;
private readonly toUnhook = new DisposableStore();

private readonly textModelService: ITextModelService;
private readonly codeEditorService: ICodeEditorService;

private occurrencesHighlight: string;

private workerRequestTokenId: number = 0;
Expand All @@ -221,20 +225,31 @@ class WordHighlighter {
private readonly _hasWordHighlights: IContextKey<boolean>;
private _ignorePositionChangeEvent: boolean;

private readonly runDelayer: Delayer<void> = this.toUnhook.add(new Delayer<void>(50));
private readonly runDelayer: Delayer<void> = this.toUnhook.add(new Delayer<void>(25));

private static storedDecorationIDs: ResourceMap<string[]> = new ResourceMap();
private static query: IWordHighlighterQuery | null = null;

constructor(editor: IActiveCodeEditor, providers: LanguageFeatureRegistry<DocumentHighlightProvider>, multiProviders: LanguageFeatureRegistry<MultiDocumentHighlightProvider>, contextKeyService: IContextKeyService, @ICodeEditorService codeEditorService: ICodeEditorService) {
constructor(
editor: IActiveCodeEditor,
providers: LanguageFeatureRegistry<DocumentHighlightProvider>,
multiProviders: LanguageFeatureRegistry<MultiDocumentHighlightProvider>,
contextKeyService: IContextKeyService,
@ITextModelService textModelService: ITextModelService,
@ICodeEditorService codeEditorService: ICodeEditorService,
) {
this.editor = editor;
this.providers = providers;
this.multiDocumentProviders = multiProviders;

this.codeEditorService = codeEditorService;
this.textModelService = textModelService;

this._hasWordHighlights = ctxHasWordHighlights.bindTo(contextKeyService);
this._ignorePositionChangeEvent = false;
this.occurrencesHighlight = this.editor.getOption(EditorOption.occurrencesHighlight);
this.model = this.editor.getModel();

this.toUnhook.add(editor.onDidChangeCursorPosition((e: ICursorPositionChangedEvent) => {
if (this._ignorePositionChangeEvent) {
// We are changing the position => ignore this event
Expand Down Expand Up @@ -267,10 +282,8 @@ class WordHighlighter {
this.toUnhook.add(editor.onDidChangeModel((e) => {
if (!e.newModelUrl && e.oldModelUrl) {
this._stopSingular();
} else {
if (WordHighlighter.query) {
this._run();
}
} else if (WordHighlighter.query) {
this._run();
}
}));
this.toUnhook.add(editor.onDidChangeConfiguration((e) => {
Expand All @@ -282,7 +295,7 @@ class WordHighlighter {
this._stopAll();
break;
case 'singleFile':
this._stopAll(WordHighlighter.query?.modelInfo?.model);
this._stopAll(WordHighlighter.query?.modelInfo?.modelURI);
break;
case 'multiFile':
if (WordHighlighter.query) {
Expand All @@ -295,6 +308,19 @@ class WordHighlighter {
}
}
}));
this.toUnhook.add(editor.onDidBlurEditorWidget(() => {
// logic is as follows
// - didBlur => active null => stopall
// - didBlur => active nb => if this.editor is notebook, do nothing (new cell, so we don't want to stopAll)
// active nb => if this.editor is NOT nb, stopAll

const activeEditor = this.codeEditorService.getFocusedCodeEditor();
if (!activeEditor) { // clicked into nb cell list, outline, terminal, etc
this._stopAll();
} else if (activeEditor.getModel()?.uri.scheme === Schemas.vscodeNotebookCell && this.editor.getModel()?.uri.scheme !== Schemas.vscodeNotebookCell) { // switched tabs from non-nb to nb
this._stopAll();
}
}));

this.decorations = this.editor.createDecorationsCollection();
this.workerRequestTokenId = 0;
Expand Down Expand Up @@ -396,12 +422,12 @@ class WordHighlighter {
}
}

private _removeAllDecorations(preservedModel?: ITextModel): void {
private _removeAllDecorations(preservedModel?: URI): void {
const currentEditors = this.codeEditorService.listCodeEditors();
const deleteURI = [];
// iterate over editors and store models in currentModels
for (const editor of currentEditors) {
if (!editor.hasModel() || isEqual(editor.getModel().uri, preservedModel?.uri)) {
if (!editor.hasModel() || isEqual(editor.getModel().uri, preservedModel)) {
continue;
}

Expand Down Expand Up @@ -435,7 +461,7 @@ class WordHighlighter {
this._removeSingleDecorations();

if (this.editor.hasTextFocus()) {
if (this.editor.getModel()?.uri.scheme !== Schemas.vscodeNotebookCell && WordHighlighter.query?.modelInfo?.model.uri.scheme !== Schemas.vscodeNotebookCell) { // clear query if focused non-nb editor
if (this.editor.getModel()?.uri.scheme !== Schemas.vscodeNotebookCell && WordHighlighter.query?.modelInfo?.modelURI.scheme !== Schemas.vscodeNotebookCell) { // clear query if focused non-nb editor
WordHighlighter.query = null;
this._run(); // TODO: @Yoyokrazy -- investigate why we need a full rerun here. likely addressed a case/patch in the first iteration of this feature
} else { // remove modelInfo to account for nb cell being disposed
Expand Down Expand Up @@ -464,7 +490,7 @@ class WordHighlighter {
}
}

private _stopAll(preservedModel?: ITextModel): void {
private _stopAll(preservedModel?: URI): void {
// Remove any existing decorations
// TODO: @Yoyokrazy -- this triggers as notebooks scroll, causing highlights to disappear momentarily.
// maybe a nb type check?
Expand Down Expand Up @@ -582,9 +608,8 @@ class WordHighlighter {
return currentModels;
}

private _run(multiFileConfigChange?: boolean): void {
private async _run(multiFileConfigChange?: boolean): Promise<void> {

let workerRequestIsValid;
const hasTextFocus = this.editor.hasTextFocus();

if (!hasTextFocus) { // new nb cell scrolled in, didChangeModel fires
Expand Down Expand Up @@ -615,41 +640,18 @@ class WordHighlighter {
return;
}

// All the effort below is trying to achieve this:
// - when cursor is moved to a word, trigger immediately a findOccurrences request
// - 250ms later after the last cursor move event, render the occurrences
// - no flickering!
workerRequestIsValid = (this.workerRequest && this.workerRequest.isValid(this.model, editorSelection, this.decorations));

WordHighlighter.query = {
modelInfo: {
model: this.model,
modelURI: this.model.uri,
selection: editorSelection,
},
word: word
}
};
}

// There are 4 cases:
// a) old workerRequest is valid & completed, renderDecorationsTimer fired
// b) old workerRequest is valid & completed, renderDecorationsTimer not fired
// c) old workerRequest is valid, but not completed
// d) old workerRequest is not valid

// For a) no action is needed
// For c), member 'lastCursorPositionChangeTime' will be used when installing the timer so no action is needed

this.lastCursorPositionChangeTime = (new Date()).getTime();

if (workerRequestIsValid) {
if (this.workerRequestCompleted && this.renderDecorationsTimer !== -1) {
// case b)
// Delay the firing of renderDecorationsTimer by an extra 250 ms
clearTimeout(this.renderDecorationsTimer);
this.renderDecorationsTimer = -1;
this._beginRenderDecorations();
}
} else if (isEqual(this.editor.getModel().uri, WordHighlighter.query.modelInfo?.model.uri)) { // only trigger new worker requests from the primary model that initiated the query
if (isEqual(this.editor.getModel().uri, WordHighlighter.query.modelInfo?.modelURI)) { // only trigger new worker requests from the primary model that initiated the query
// case d)

// check if the new queried word is contained in the range of a stored decoration for this model
Expand All @@ -664,7 +666,7 @@ class WordHighlighter {

// stop all previous actions if new word is highlighted
// if we trigger the run off a setting change -> multifile highlighting, we do not want to remove decorations from this model
this._stopAll(multiFileConfigChange ? this.model : undefined);
this._stopAll(multiFileConfigChange ? this.model.uri : undefined);

const myRequestId = ++this.workerRequestTokenId;
this.workerRequestCompleted = false;
Expand All @@ -675,10 +677,35 @@ class WordHighlighter {
// 1) we have text focus, and a valid query was updated.
// 2) we do not have text focus, and a valid query is cached.
// the query will ALWAYS have the correct data for the current highlight request, so it can always be passed to the workerRequest safely
if (!WordHighlighter.query || !WordHighlighter.query.modelInfo || WordHighlighter.query.modelInfo.model.isDisposed()) {
if (!WordHighlighter.query || !WordHighlighter.query.modelInfo) {
return;
}
this.workerRequest = this.computeWithModel(WordHighlighter.query.modelInfo.model, WordHighlighter.query.modelInfo.selection, WordHighlighter.query.word, otherModelsToHighlight);
const queryModelRef = await this.textModelService.createModelReference(WordHighlighter.query.modelInfo.modelURI);
const queryModel = queryModelRef.object.textEditorModel;
this.workerRequest = this.computeWithModel(queryModel, WordHighlighter.query.modelInfo.selection, otherModelsToHighlight);

this.workerRequest?.result.then(data => {
if (myRequestId === this.workerRequestTokenId) {
this.workerRequestCompleted = true;
this.workerRequestValue = data || [];
this._beginRenderDecorations();
}
}, onUnexpectedError);
} else if (this.model.uri.scheme === Schemas.vscodeNotebookCell) {
// new wordHighlighter coming from a different model, NOT the query model, need to create a textModel ref

// this._stopAll(multiFileConfigChange ? this.model.uri : undefined);

const myRequestId = ++this.workerRequestTokenId;
this.workerRequestCompleted = false;

if (!WordHighlighter.query || !WordHighlighter.query.modelInfo) {
return;
}

const queryModelRef = await this.textModelService.createModelReference(WordHighlighter.query.modelInfo.modelURI);
const queryModel = queryModelRef.object.textEditorModel;
this.workerRequest = this.computeWithModel(queryModel, WordHighlighter.query.modelInfo.selection, [this.model]);

this.workerRequest?.result.then(data => {
if (myRequestId === this.workerRequestTokenId) {
Expand All @@ -690,11 +717,11 @@ class WordHighlighter {
}
}

private computeWithModel(model: ITextModel, selection: Selection, word: IWordAtPosition | null, otherModels: ITextModel[]): IOccurenceAtPositionRequest | null {
private computeWithModel(model: ITextModel, selection: Selection, otherModels: ITextModel[]): IOccurenceAtPositionRequest | null {
if (!otherModels.length) {
return computeOccurencesAtPosition(this.providers, model, selection, word, this.editor.getOption(EditorOption.wordSeparators));
return computeOccurencesAtPosition(this.providers, model, selection, this.editor.getOption(EditorOption.wordSeparators));
} else {
return computeOccurencesMultiModel(this.multiDocumentProviders, model, selection, word, this.editor.getOption(EditorOption.wordSeparators), otherModels);
return computeOccurencesMultiModel(this.multiDocumentProviders, model, selection, this.editor.getOption(EditorOption.wordSeparators), otherModels);
}
}

Expand Down Expand Up @@ -755,6 +782,9 @@ class WordHighlighter {
}
}
}

// clear the worker request when decorations are completed
this.workerRequest = null;
}

public dispose(): void {
Expand All @@ -773,16 +803,26 @@ export class WordHighlighterContribution extends Disposable implements IEditorCo

private _wordHighlighter: WordHighlighter | null;

constructor(editor: ICodeEditor, @IContextKeyService contextKeyService: IContextKeyService, @ILanguageFeaturesService languageFeaturesService: ILanguageFeaturesService, @ICodeEditorService codeEditorService: ICodeEditorService) {
constructor(
editor: ICodeEditor,
@IContextKeyService contextKeyService: IContextKeyService,
@ILanguageFeaturesService languageFeaturesService: ILanguageFeaturesService,
@ICodeEditorService codeEditorService: ICodeEditorService,
@ITextModelService textModelService: ITextModelService,
) {
super();
this._wordHighlighter = null;
const createWordHighlighterIfPossible = () => {
if (editor.hasModel() && !editor.getModel().isTooLargeForTokenization()) {
this._wordHighlighter = new WordHighlighter(editor, languageFeaturesService.documentHighlightProvider, languageFeaturesService.multiDocumentHighlightProvider, contextKeyService, codeEditorService);
this._wordHighlighter = new WordHighlighter(editor, languageFeaturesService.documentHighlightProvider, languageFeaturesService.multiDocumentHighlightProvider, contextKeyService, textModelService, codeEditorService);
}
};
this._register(editor.onDidChangeModel((e) => {
if (this._wordHighlighter) {
if (!e.newModelUrl && e.oldModelUrl?.scheme !== Schemas.vscodeNotebookCell) { // happens when switching tabs to a notebook that has focus in the cell list, no new model URI (this also doesn't make it to the wordHighlighter, bc no editor.hasModel)
this.wordHighlighter?.stop();
}

this._wordHighlighter.dispose();
this._wordHighlighter = null;
}
Expand Down
Loading

0 comments on commit 6092f24

Please sign in to comment.