Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multi-document highlight provider feature #198467

Merged
merged 7 commits into from
Nov 22, 2023
Merged

Conversation

Yoyokrazy
Copy link
Contributor

@Yoyokrazy Yoyokrazy commented Nov 16, 2023

Fixes #184259
re #196240
Fixes #198469
re #196354

This pull request adds support for multi-document highlight providers, allowing for the highlighting of occurrences of a variable.

It includes:

  • proposed API for multi-doc highlight providers, and registering
  • generic editor feature for textual highlight provider
    • remove textual highlight from mainThreadLangFeat
  • improvement to typescript-language-features extension to hook up multi-doc semantic highlighting
  • hooked up exthost types and everything for multi-doc highlights
  • introduced MultiDocumentHighlight
class MultiDocumentHighlight {

    /**
     * The URI of the document containing the highlights.
     */
    uri: Uri;

    /**
     * The highlights for the document.
     */
    highlights: DocumentHighlight[];

    /**
     * Creates a new instance of MultiDocumentHighlight.
     * @param uri The URI of the document containing the highlights.
     * @param highlights The highlights for the document.
     */
    constructor(uri: Uri, highlights: DocumentHighlight[]);
}

@Yoyokrazy Yoyokrazy self-assigned this Nov 16, 2023
@vscodenpa vscodenpa added this to the November 2023 milestone Nov 16, 2023
@Yoyokrazy Yoyokrazy added the editor-highlight Editor selection/word highlight issues label Nov 16, 2023
@Yoyokrazy
Copy link
Contributor Author

Yoyokrazy commented Nov 17, 2023

Todo:

  • Textual provider being called when typescript files are highlighted. Should not be using that.
    • need to score the textual provider lower/0 when there is a higher scored model?
  • Potentially remove TextualOccurenceRequest from WordHighlighter.ts. Should always have the generic TextualHighlightProvider to fall back on.

Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments and suggestions tho this is already looking very good. Nice work!

@Yoyokrazy Yoyokrazy force-pushed the milively/multi-doc-api branch from 0a0066e to b9e92d5 Compare November 17, 2023 19:29
@Yoyokrazy Yoyokrazy requested a review from jrieken November 20, 2023 20:01
@Yoyokrazy
Copy link
Contributor Author

Yoyokrazy commented Nov 21, 2023

All nits + feedback has been addressed. Update on the todo with the textual pieces:

  • Having issues with the textual highlight provider registered editor getting called when the typescript highlight provider returns a valid (albeit empty) result. The current check leverages first and returns when a provider gives a result that evaluates true with result instanceof ResourceMap && result.size > 0. Current (not-functional) code:
// in order of score ask the occurrences provider
// until someone response with a good result
// (good = none empty array)
return first<ResourceMap<DocumentHighlight[]> | null | undefined>(orderedByScore.map(provider => () => {
	const filteredModels = otherModels.filter(otherModel => {
		return score(provider.selector, otherModel.uri, otherModel.getLanguageId(), true, undefined, undefined) > 0;
	});

	return Promise.resolve(provider.provideMultiDocumentHighlights(model, position, filteredModels, token))
		.then(undefined, onUnexpectedExternalError);
}), (t: ResourceMap<DocumentHighlight[]> | null | undefined): t is ResourceMap<DocumentHighlight[]> => t instanceof ResourceMap && t.size > 0);
  • My thought was to shift to a "good" result being = non-null and non-undefined. In other words, a Resource map could be returned containing any number of given URIs and simply not have any DocumentHighlights, and that would evaluate to true and pop out of first(). That changed the final line of the above snippet to:
(t: ResourceMap<DocumentHighlight[]> | null | undefined): t is ResourceMap<DocumentHighlight[]> => t instanceof ResourceMap && t !== null && t !== undefined);
  • That continued to not pop out of first() so my further thought is that likely the typescript highligh provider was returning an empty MultiDocHighlight array upon no results. I then tweaked the implementation within documentHighlight.ts in typescript-lang-feat and set it to return undefined upon a body length of 0 (body of the response is just an empty array when clicking import). This still doesn't pop out of first(), leaving me a bit stumped on where to take this.

Currently the TextualHighlightProvider is defined, but not registered. This results in only falling back to a TextualOccurrenceRequest within WordHighlighter.ts when there is no provider in the registry for the model. This means that currently it works as intended, however just isn't an optimal situation.

Would love some feedback on this issue @jrieken, thanks! This PR is at a state that it can be merged without risk of breaking core features, it just doesn't add the generic Textual editor feature that we initially set out to implement alongside this multi-doc feature. That will get shifted to a debt item that gets addressed in the coming December work cycle.

@Yoyokrazy Yoyokrazy dismissed jrieken’s stale review November 22, 2023 18:06

oof and feedback/nits addressed

@Yoyokrazy Yoyokrazy merged commit e33219d into main Nov 22, 2023
5 checks passed
@Yoyokrazy Yoyokrazy deleted the milively/multi-doc-api branch November 22, 2023 18:07
@github-actions github-actions bot locked and limited conversation to collaborators Jan 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
editor-highlight Editor selection/word highlight issues
Projects
None yet
5 participants