Resolve initial rename value #37691
Resolve initial rename value #37691
Conversation
|
Looks pretty good after a quick read... I have to check with my schedule but we make this happen in November... |
jrieken
added
api
editor-contrib
| @@ -79,6 +79,33 @@ export function rename(model: IReadOnlyModel, position: Position, newName: strin | |||
| }); | |||
| } | |||
|
|
|||
| function resolveInitialRenameValue(model: IReadOnlyModel, position: Position): TPromise<RenameInitialValue> { | |||
| const supports = RenameProviderRegistry.ordered(model); | |||
There was a problem hiding this comment.
We should just go for the first rename provider so that we always use the same for resolving the location and for the actual rename.
|
|
||
| let initialValue = await resolveInitialRenameValue(this.editor.getModel(), this.editor.getPosition()); | ||
|
|
||
| if(initialValue) { |
There was a problem hiding this comment.
We can maybe move this to a 'default' implementation of resolveRenameValue
There was a problem hiding this comment.
I'm not really sure, the code below is just setting values local to the run function. It's only small extension of the logic that was already there before.
| @@ -2482,6 +2482,11 @@ declare module 'vscode' { | |||
| appendVariable(name: string, defaultValue: string | ((snippet: SnippetString) => any)): SnippetString; | |||
| } | |||
|
|
|||
| export interface RenameInitialValue { | |||
There was a problem hiding this comment.
Needs maybe some better naming here but idea is good
There was a problem hiding this comment.
I am actually wondering when we would return both, so range and text (which then would be different from the text in range?). Do you have a use-case in mind?
| @@ -594,6 +594,7 @@ export interface ExtHostLanguageFeaturesShape { | |||
| $resolveWorkspaceSymbol(handle: number, symbol: modes.SymbolInformation): TPromise<IWorkspaceSymbol>; | |||
| $releaseWorkspaceSymbols(handle: number, id: number): void; | |||
| $provideRenameEdits(handle: number, resource: URI, position: IPosition, newName: string): TPromise<modes.WorkspaceEdit>; | |||
| $resolveInitialRenameValue(handle: number, resource: URI, position: IPosition): TPromise<modes.RenameInitialValue>; | |||
There was a problem hiding this comment.
We removed some automagic marshalling and UriComponent must be used now
|
@Krzysztof-Cieslak Are you still with us on this one? We have scheduled this for January. |
|
@jrieken, yes, I’ve been on vacations after Xmas. Give me day or two and I’ll get back to it. |
|
@jrieken I've rebased PR to latest master and moved to using the first provider for resolving. I'm not sure if I agree/understand second suggestion. Also, happy to hear any suggestions for the naming :) |
Krzysztof-Cieslak
changed the title
| return this._createDisposable(handle); | ||
| } | ||
|
|
||
| $provideRenameEdits(handle: number, resource: UriComponents, position: IPosition, newName: string): TPromise<modes.WorkspaceEdit> { | ||
| return this._withAdapter(handle, RenameAdapter, adapter => adapter.provideRenameEdits(URI.revive(resource), position, newName)); | ||
| } | ||
|
|
||
| $resolveInitialRenameValue(handle: number, resource: URI, position: IPosition): TPromise<modes.RenameInitialValue> { |
There was a problem hiding this comment.
The URI argument won't be of that type at runtime. That's because the function will be called by our JSON-rpc internals and the runtime type will be UriComponents (from which you then create an URI using revive).
| @@ -2482,6 +2482,11 @@ declare module 'vscode' { | |||
| appendVariable(name: string, defaultValue: string | ((snippet: SnippetString) => any)): SnippetString; | |||
| } | |||
|
|
|||
| export interface RenameInitialValue { | |||
There was a problem hiding this comment.
I am actually wondering when we would return both, so range and text (which then would be different from the text in range?). Do you have a use-case in mind?
| @@ -2567,6 +2572,8 @@ declare module 'vscode' { | |||
| * signaled by returning `undefined` or `null`. | |||
| */ | |||
| provideRenameEdits(document: TextDocument, position: Position, newName: string, token: CancellationToken): ProviderResult<WorkspaceEdit>; | |||
|
|
|||
| resolveInitialRenameValue?(document: TextDocument, position: Position, token: CancellationToken): ProviderResult<RenameInitialValue>; | |||
There was a problem hiding this comment.
Maybe RenameInformation? That would make it:
resolveRenameInformation?(document: TextDocument, position: Position, token: CancellationToken): ProviderResult<RenameInformation>;
provideRenameInformation?(document: TextDocument, position: Position, token: CancellationToken): ProviderResult<RenameInformation>;There was a problem hiding this comment.
Or something more clever sounding, like RenameForesight, RenamePreflight, etc...
|
Pulling this in but marking the new API as proposed which buys as some time to polish names etc. @Krzysztof-Cieslak Thanks for good work. |
|
@jrieken, thanks for merging! |
PR implements the functionality discussed in #7340. It's just a proposal, I've done it mostly to learn a bit about architecture around extensions API, so feel free to close it if it's not going in good direction.
CC: @aeschli & @jrieken