-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Added command to rename current document, with lsp support #5531
Changes from all commits
79a5d35
398a820
25ca3d5
3b23489
fdef4f2
258aaa5
6ef8fad
406fd88
289220c
9526d7c
826d9b7
37d57b0
17904c6
e1d628d
403a38b
9925f3e
e787750
411be74
a9a30c1
c820566
c0e7f50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -442,6 +442,11 @@ impl Client { | |
execute_command: Some(lsp::DynamicRegistrationClientCapabilities { | ||
dynamic_registration: Some(false), | ||
}), | ||
file_operations: Some(lsp::WorkspaceFileOperationsClientCapabilities { | ||
will_rename: Some(true), | ||
did_rename: Some(true), | ||
..Default::default() | ||
}), | ||
inlay_hint: Some(lsp::InlayHintWorkspaceClientCapabilities { | ||
refresh_support: Some(false), | ||
}), | ||
|
@@ -592,6 +597,25 @@ impl Client { | |
) | ||
} | ||
|
||
pub fn will_rename_files( | ||
&self, | ||
params: &Vec<lsp::FileRename>, | ||
) -> impl Future<Output = Result<lsp::WorkspaceEdit>> { | ||
let files = params.to_owned(); | ||
let request = self.call::<lsp::request::WillRenameFiles>(lsp::RenameFilesParams { files }); | ||
|
||
async move { | ||
let json = request.await?; | ||
let response: Option<lsp::WorkspaceEdit> = serde_json::from_value(json)?; | ||
Ok(response.unwrap_or_default()) | ||
} | ||
} | ||
|
||
pub fn did_rename_files(&self, params: &[lsp::FileRename]) -> impl Future<Output = Result<()>> { | ||
let files = params.to_owned(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this must check the server capabiliy |
||
self.notify::<lsp::notification::DidRenameFiles>(lsp::RenameFilesParams { files }) | ||
} | ||
|
||
pub fn did_change_workspace( | ||
&self, | ||
added: Vec<WorkspaceFolder>, | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,3 +1,4 @@ | ||||||
use std::ffi::OsStr; | ||||||
use std::fmt::Write; | ||||||
use std::ops::Deref; | ||||||
|
||||||
|
@@ -6,6 +7,7 @@ use crate::job::Job; | |||||
use super::*; | ||||||
|
||||||
use helix_core::{encoding, shellwords::Shellwords}; | ||||||
use helix_lsp::{lsp, Url}; | ||||||
use helix_view::document::DEFAULT_LANGUAGE_NAME; | ||||||
use helix_view::editor::{Action, CloseError, ConfigEvent}; | ||||||
use serde_json::Value; | ||||||
|
@@ -2167,6 +2169,62 @@ fn reset_diff_change( | |||||
Ok(()) | ||||||
} | ||||||
|
||||||
fn rename_buffer( | ||||||
cx: &mut compositor::Context, | ||||||
args: &[Cow<str>], | ||||||
event: PromptEvent, | ||||||
) -> anyhow::Result<()> { | ||||||
if event != PromptEvent::Validate { | ||||||
return Ok(()); | ||||||
} | ||||||
ensure!(args.len() == 1, ":rename takes one argument"); | ||||||
let new_name = args.first().unwrap(); | ||||||
|
||||||
let (_, doc) = current!(cx.editor); | ||||||
let path = doc | ||||||
.path() | ||||||
.ok_or_else(|| anyhow!("Scratch buffers can not be renamed, use :write"))? | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
let mut path_new = path.clone(); | ||||||
path_new.set_file_name(OsStr::new(new_name.as_ref())); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think we should allow moving to new paths not just new filenames There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note that this means that the destination directory might not exist so we need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a sort of weird one, if we change the command name to On the other hand I don't think a Imo, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it might make mose sense to just call the entire command move and make the destination a path relative to CWD. If somebody wants to just change the filename they can use I think that generally move is a better idea than rename because it can do more (and the LS can handle full moves for example RA can move a module to a totally different module |
||||||
if path_new.exists() { | ||||||
bail!("Destination already exists"); | ||||||
} | ||||||
|
||||||
if let Err(e) = std::fs::rename(&path, &path_new) { | ||||||
bail!("Could not rename file: {e}"); | ||||||
} | ||||||
let (_, doc) = current!(cx.editor); | ||||||
doc.set_path(Some(path_new.as_path())) | ||||||
.map_err(|_| anyhow!("File renamed, but could not set path of the document"))?; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think that this should probably happen before the rename. If the fails later we can roll this back pretty easily (ideally with an unchecked method) while rolling back IO is pretty hard |
||||||
|
||||||
if let Some(lsp_client) = doc.language_server() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the wii rename request must be sent (and processed) before the rename happens so this needs to be moved to the start of the function:
At the end of the function you must send didRename:
you also need to support mullti LSP here. Probably ok to just send wiiRename to the first server that supports it and didRename to all of them |
||||||
if let Ok(old_uri_str) = Url::from_file_path(&path) { | ||||||
let old_uri = old_uri_str.to_string(); | ||||||
if let Ok(new_uri_str) = Url::from_file_path(&path_new) { | ||||||
let new_uri = new_uri_str.to_string(); | ||||||
let files = vec![lsp::FileRename { old_uri, new_uri }]; | ||||||
match helix_lsp::block_on(lsp_client.will_rename_files(&files)) { | ||||||
Ok(edit) => { | ||||||
if apply_workspace_edit(cx.editor, helix_lsp::OffsetEncoding::Utf8, &edit) | ||||||
.is_err() | ||||||
{ | ||||||
log::error!(":rename command failed to apply edits") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we already show an error message herer you don't need to display it. The returned result is just forcases where we need to communicate to the LSP that the edit failed (not here) |
||||||
} | ||||||
} | ||||||
Err(err) => log::error!("willRename request failed: {err}"), | ||||||
} | ||||||
} else { | ||||||
log::error!(":rename command could not get new path uri") | ||||||
} | ||||||
} else { | ||||||
log::error!(":rename command could not get current path uri") | ||||||
} | ||||||
} | ||||||
cx.editor | ||||||
.set_status(format!("Renamed file to {}", new_name)); | ||||||
Ok(()) | ||||||
} | ||||||
|
||||||
fn clear_register( | ||||||
cx: &mut compositor::Context, | ||||||
args: &[Cow<str>], | ||||||
|
@@ -2752,6 +2810,13 @@ pub const TYPABLE_COMMAND_LIST: &[TypableCommand] = &[ | |||||
fun: reset_diff_change, | ||||||
signature: CommandSignature::none(), | ||||||
}, | ||||||
TypableCommand { | ||||||
name: "rename", | ||||||
aliases: &["rnm"], | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do set the alias to move, we're probably better of with calling the command itself There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have other commands that have totally different aliases too so I think leaving it like that is fine too |
||||||
doc: "Rename the currently selected buffer", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think we need to highlight here that this is different than just |
||||||
fun: rename_buffer, | ||||||
signature: CommandSignature::none(), | ||||||
}, | ||||||
TypableCommand { | ||||||
name: "clear-register", | ||||||
aliases: &[], | ||||||
|
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.
you need to check
workspace.fileOperations.willRename
herer (or even better at the callsite to support multiple LSPs)