-
Notifications
You must be signed in to change notification settings - Fork 182
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 willRename and didRename fileOperations #2498
base: main
Are you sure you want to change the base?
Conversation
11b3618
to
6f65462
Compare
I will highlight things implemented from the LSP spec. Renaming a document
That part of the spec can be seen on these two lines:
Note The user could rename WillRenameFiles Request
That can be seen here: Lines 95 to 98 in 6f65462
I didn't implement this. DidRenameFiles NotificationWhen a rename happened, all sessions who have the |
Here is what to expect from this PR.
For session that don't support the |
The built-in Line 209 in 293f4a4
While for other features like document symbols or goto symbol, the similar built-in commands are not overridden because they can return different results as the LSP commands, so it might be useful to have both variants. But I think for the rename this is not necessary and to add two more LSP variants into the menus would not be optimal for usability. What do you think? |
plugin/rename_file.py
Outdated
open_file_uri(self.window, new_path).then(restore_regions) | ||
|
||
def notify_did_rename(self, rename_file_params: RenameFilesParams): | ||
sessions = [s for s in self.sessions() if s.has_capability('workspace.fileOperations.didRename')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The server capability is defined as 'workspace.fileOperations.didRename'
:
interface ServerCapabilities {
workspace: {
//...
fileOperations: {
// ...
didRename?: FileOperationRegistrationOptions
}
}
interface FileOperationRegistrationOptions {
filters: FileOperationFilter[];
}
export interface FileOperationFilter {
scheme?: string;
pattern: FileOperationPattern
}
interface FileOperationPattern {
glob: string;
matches?: FileOperationPatternKind;
options?: FileOperationPatternOptions;
}
To implement this properly I would need to respect the provided pattern in FileOperationRegistrationOptions
,
but that seem like more work. The current code also works, at least with the server I am testing this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I am unsure if I need to implement FileOperationRegistrationOptions in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should try implementing that. filters
is not an optional feature so we won't be complaint otherwise.
@jwortmann I tried to implement your suggestion on a different branch, |
I will try a workaround sublimehq/sublime_text#2234 (comment) |
@jwortmann I liked the idea, that way we do not introducing new commands, instead relying on existing ones. |
…s from that folder to new location so we do not lose changes The ST default rename_path command doesn't do this VS Code does this LSP will do this
It is not necessary
…in the sidebar context menu
…r of match_file_operation_filters
…ys LSP: Rename File I cannot tweak the text inside the TextInputHandler use show_input_panel instead...
Co-authored-by: jwortmann <[email protected]>
…g ST rename_path command
Will this allow functionality like described in sublimelsp/LSP-intelephense#112 that works in Vscode, where a symbol is mapped to a file name, so the file name would be automatically changed if the symbol is renamed? |
I believe no, this PR would only work in the other direction, i.e. if you rename a file, then the corresponding class name would be updated in that file and in other files. For the functionality from the linked issue, we need to add support for Lines 19 to 22 in f983345
|
@jwortmann thanks for that info. |
✅ Deploy Preview for sublime-lsp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I've noticed that renaming unsaved file is a bit broken because it triggers a dialog to either save or not the changes but at this point the file is already renamed. Then pressing save tries to save the file to the old location and not saving changes leaves the old view open. I suppose that's related to not using |
Another difference compared to native behavior is that if you try to rename a file that is not "fully open" (only previewed on right clicking in the sidebar) then native behavior will keep the renamed one in preview mode while LSP will open the file in non-preview mode. That doesn't seem like much of an issue but there might be it might create some extra issues that I'm not seeing right now. |
Rafal thanks for catching this. Currently the code closes and opens the file after rename. |
closes #2199
Here is a example video:
Kapture.2024-06-25.at.18.24.36.mp4
Notice the import changing in file
a.ts
when we rename theb.ts
file.example.zip