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

Provide API commands to resolve codeLens, completion items .... #44846

Closed
dbaeumer opened this issue Mar 1, 2018 · 16 comments
Closed

Provide API commands to resolve codeLens, completion items .... #44846

dbaeumer opened this issue Mar 1, 2018 · 16 comments
Assignees
Labels
api under-discussion Issue is under discussion for relevance, priority, approach verified Verification succeeded
Milestone

Comments

@dbaeumer
Copy link
Member

dbaeumer commented Mar 1, 2018

VS Code provides commands to provide these data types. However if they need to be resolved later (for example on user action) there is no API to do so.

@dbaeumer dbaeumer changed the title Provide commands to resolve codeLens, completion items .... Provide API commands to resolve codeLens, completion items .... Mar 1, 2018
@jrieken jrieken added the api label Mar 1, 2018
@jrieken
Copy link
Member

jrieken commented Mar 1, 2018

tricky

@jrieken jrieken added the under-discussion Issue is under discussion for relevance, priority, approach label Mar 1, 2018
@srivatsn
Copy link

For completeness, DocumentLinkProvider also has a resolve method.

@dbaeumer
Copy link
Member Author

@jrieken would it be easier if the API requires that the item passed to the resolve call is === to the item return from provide API command. At least for the CompletionItem the LSP provides a data slot so that items that travel over the wire can be re-associated.

@jrieken
Copy link
Member

jrieken commented Mar 27, 2018

would it be easier if the API requires that the item passed to the resolve call is === to the item return from provide API command.

The implementation usually does that, e.g the SuggestAdapter has a cache for those items. The tricky bit is the lifecycle which is managed by the UI...

@jrieken
Copy link
Member

jrieken commented Apr 13, 2018

I have pushed a change to the vscode.executeCompletionItemProvider-command that allows to pass an extra argument which denotes the number of items to resolve. This large numbers a slowdown is to be expected. For a sample see: https://github.com/Microsoft/vscode/blob/8973bea2ed78d82722b5f0d61c6fd201bd4c12b7/src/vs/workbench/test/electron-browser/api/extHostApiCommands.test.ts#L445

@srivatsn
Copy link

Is there going to be a way to resolve the item separate from this? This may be ok in some cases but ideally we wouldn't want to resolve completion items (especially for live share where there's already latency for those requests).

@jrieken
Copy link
Member

jrieken commented Apr 17, 2018

Is there going to be a way to resolve the item separate from this?

Not without a major investment. To achieve that we need to expose the language feature registries (that currently live in the renderer, not extension host process). We need to expose API for all of them and we need to document/expose the internal view model and how results are merged etc. The approach currently taken is pragmatic and should yield very similar results.

@srivatsn
Copy link

Are you planning to add this max parameter to the executeCodeLens command as well? Right now, we get Undefined as the name of codelens and so this is more important for Codelens than completion for us. Would it be possible to specify resolve all items and we can pass that if we are calling it from non-perf sensitive path. For CodeLens for eg., the indicators showing up doesn't block users and so there we could say resolve all.

@jrieken
Copy link
Member

jrieken commented Apr 18, 2018

Yeah, I can try to squeeze that in

@jrieken
Copy link
Member

jrieken commented Apr 19, 2018

@srivatsn I have pushed a change that allows to resolve code lenses in a similar spirit. You can pass a number, the semantics are that only so many lenses are resolved and return, e.g. no unresolved code lens is return.

@srivatsn
Copy link

srivatsn commented Apr 19, 2018

Why restrict it to a certain number instead of resolving all results (with the understanding that there's a perf cost which is probably acceptable if it's in the background)? I don't know what a reasonable number would be to pick and based on that number the experience in the editor would feel arbitrary.

@jrieken
Copy link
Member

jrieken commented Apr 19, 2018

I don't know what a reasonable number

I don't know either, that's why I made it a parameter 😄 The restriction is because an unresolved code lens, so one with just a range but with a command, cannot be rendered a meaningful way.

@srivatsn
Copy link

Sorry I am not following. My suggestion is to take a boolean parameter called resolveCodeLenses and here change it to else if(resolveCodeLenses) - would that work?

@jrieken
Copy link
Member

jrieken commented Apr 20, 2018

Wouldn't that be the same as using Number.MAX_VALUE just without flexibility?

@srivatsn
Copy link

Fair enough. My initial reaction was that a Boolean would be cleaner but on further thought this is fine.

@jrieken jrieken closed this as completed Apr 23, 2018
@dbaeumer dbaeumer added the verified Verification succeeded label Apr 27, 2018
@dbaeumer
Copy link
Member Author

Verified coding up a small extension. However only did for completion items.

@vscodebot vscodebot bot locked and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api under-discussion Issue is under discussion for relevance, priority, approach verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

3 participants