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

Return updated items in resolve* operations #74

Merged
merged 1 commit into from
Apr 25, 2018

Conversation

AlexTugarev
Copy link
Contributor

at least with latest monaco 0.12.0 the resolution of suggestion items is broken in some cases. it can be reproduced with JDT LS, when you continue typing a prefix of a suggestion shown after the suggestions were triggered explicitly. it happens that the completion/resolve params are missing a data field.

the actual issue is, that monaco looses information, because the CompletionItemProvider won't fill up the given item, but returns a new one, which is the converted result from LS.

monaco's SuggestAdapter, which calls the completion item provider, will just update the given suggestion item and overwrites such additional fields, if they are not preserved by LSs.

reading the docs and looking at other CompletionItemProvider implementations, shows that it's OK to return the given item, which can be updated with more data from LS response.

		/**
		 * Given a completion item fill in more data, like [doc-comment](#CompletionItem.documentation)
		 * or [details](#CompletionItem.detail).
		 *
		 * The editor will only resolve a completion item once.
		 *
		 * @param item A completion item currently active in the UI.
		 * @param token A cancellation token.
		 * @return The resolved completion item or a thenable that resolves to of such. It is OK to return the given
		 * `item`. When no result is returned, the given `item` will be used.
		 */
		resolveCompletionItem?(item: CompletionItem, token: CancellationToken): ProviderResult<CompletionItem>;

the resolution of CodeLenses is also broken with JDT LS.

Copy link
Contributor

@rcjsuen rcjsuen left a comment

Choose a reason for hiding this comment

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

What you described makes sense to me and the changes look good, Alex. Thanks!

@akosyakov
Copy link
Contributor

@AlexTugarev thank you, please merge

@AlexTugarev AlexTugarev merged commit 17c9b47 into master Apr 25, 2018
@AlexTugarev AlexTugarev deleted the at/resolve-should-update-item branch July 26, 2018 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants