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

Added command to rename current document, with lsp support #5531

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions book/src/generated/typable-cmd.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,5 @@
| `:pipe-to` | Pipe each selection to the shell command, ignoring output. |
| `:run-shell-command`, `:sh` | Run a shell command |
| `:reset-diff-change`, `:diffget`, `:diffg` | Reset the diff change at the cursor position. |
| `:rename`, `:rnm` | Rename the currently selected buffer |
| `:clear-register` | Clear given register. If no argument is provided, clear all registers. |
24 changes: 24 additions & 0 deletions helix-lsp/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}),
Expand Down Expand Up @@ -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();
Copy link
Member

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)

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();
Copy link
Member

Choose a reason for hiding this comment

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

this must check the server capabiliy workspace.fileOperations.didRename

self.notify::<lsp::notification::DidRenameFiles>(lsp::RenameFilesParams { files })
}

pub fn did_change_workspace(
&self,
added: Vec<WorkspaceFolder>,
Expand Down
67 changes: 67 additions & 0 deletions helix-term/src/commands/typed.rs
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;

Expand All @@ -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;
Expand Down Expand Up @@ -2167,6 +2169,64 @@ 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
.relative_path()
ontley marked this conversation as resolved.
Show resolved Hide resolved
.ok_or_else(|| anyhow!("Buffer not saved, use :write"))?
ontley marked this conversation as resolved.
Show resolved Hide resolved
.canonicalize()
.map_err(|_| anyhow!("Could not get absolute path of the document"))?;
ontley marked this conversation as resolved.
Show resolved Hide resolved
let mut path_new = path.clone();
path_new.set_file_name(OsStr::new(new_name.as_ref()));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
path_new.set_file_name(OsStr::new(new_name.as_ref()));
path_new.parent().expect("is a file").join(new_name);

I think we should allow moving to new paths not just new filenames

Copy link
Member

Choose a reason for hiding this comment

The 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 rename! too

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 move as previously said, probably a good ide, thoguh mv in bash takes absolute paths, not ones relative to the file but I think it's work

On the other hand I don't think a file renaming function should be able to move it as most file managers just let you rename it

Imo, move is probably better

Copy link
Member

Choose a reason for hiding this comment

The 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 %filname/../<new_name> in the future.

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!("This file already exists");
}
ontley marked this conversation as resolved.
Show resolved Hide resolved

if let Err(e) = std::fs::rename(&path, &path_new) {
bail!("Could not rename file, error: {}", e);
ontley marked this conversation as resolved.
Show resolved Hide resolved
}
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"))?;
Copy link
Member

Choose a reason for hiding this comment

The 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() {
Copy link
Member

Choose a reason for hiding this comment

The 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:

The will rename files request is sent from the client to the server before files are actually renamed as long as the rename is triggered from within the client either by a user action or by applying a workspace edit. The request can return a WorkspaceEdit which will be applied to workspace before the files are renamed.

At the end of the function you must send didRename:

The did rename files notification is sent from the client to the server when files were renamed from within the client.

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")
Copy link
Member

Choose a reason for hiding this comment

The 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!("Language server error: {}", err),
ontley marked this conversation as resolved.
Show resolved Hide resolved
}
} 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>],
Expand Down Expand Up @@ -2752,6 +2812,13 @@ pub const TYPABLE_COMMAND_LIST: &[TypableCommand] = &[
fun: reset_diff_change,
signature: CommandSignature::none(),
},
TypableCommand {
name: "rename",
aliases: &["rnm"],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
aliases: &["rnm"],
aliases: &["mv"],

I think rnm is a bit of a weird alias and not commonly used. Considering how widely used the shell command is I think mv could be a good alias

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 move or move_file

Copy link
Member

Choose a reason for hiding this comment

The 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",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
doc: "Rename the currently selected buffer",
doc: "Move the current buffer and its corresponding file to a different path",

I think we need to highlight here that this is different than just :w new_path. Ofcourse this just a short help string but I think calling it move is generally clearer

fun: rename_buffer,
signature: CommandSignature::none(),
},
TypableCommand {
name: "clear-register",
aliases: &[],
Expand Down