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

command expansions #3393

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
21 changes: 11 additions & 10 deletions helix-term/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,16 +185,17 @@ impl MappableCommand {
pub fn execute(&self, cx: &mut Context) {
match &self {
Self::Typable { name, args, doc: _ } => {
let args: Vec<Cow<str>> = args.iter().map(Cow::from).collect();
if let Some(command) = typed::TYPABLE_COMMAND_MAP.get(name.as_str()) {
let mut cx = compositor::Context {
editor: cx.editor,
jobs: cx.jobs,
scroll: None,
};
if let Err(e) = (command.fun)(&mut cx, &args[..], PromptEvent::Validate) {
cx.editor.set_error(format!("{}", e));
}
let mut cx = compositor::Context {
editor: cx.editor,
jobs: cx.jobs,
scroll: None,
};
if let Err(e) = typed::process_cmd(
&mut cx,
&format!("{} {}", name, args.join(" ")),
PromptEvent::Validate,
) {
cx.editor.set_error(format!("{}", e));
}
}
Self::Static { fun, .. } => (fun)(cx),
Expand Down
114 changes: 89 additions & 25 deletions helix-term/src/commands/typed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2165,6 +2165,42 @@ fn reset_diff_change(
Ok(())
}

pub fn process_cmd(
cx: &mut compositor::Context,
input: &str,
event: PromptEvent,
) -> anyhow::Result<()> {
let input = expand_args(cx.editor, input);
let parts = input.split_whitespace().collect::<Vec<&str>>();
if parts.is_empty() {
return Ok(());
}

// If command is numeric, interpret as line number and go there.
if parts.len() == 1 && parts[0].parse::<usize>().ok().is_some() {
if let Err(e) = typed::goto_line_number(cx, &[Cow::from(parts[0])], event) {
cx.editor.set_error(format!("{}", e));
return Err(e);
}
return Ok(());
}

// Handle typable commands
if let Some(cmd) = typed::TYPABLE_COMMAND_MAP.get(parts[0]) {
let shellwords = shellwords::Shellwords::from(input.as_ref());
let args = shellwords.words();

if let Err(e) = (cmd.fun)(cx, &args[1..], event) {
cx.editor.set_error(format!("{}", e));
return Err(e);
}
} else if event == PromptEvent::Validate {
cx.editor
.set_error(format!("no such command: '{}'", parts[0]));
}
Comment on lines +2197 to +2200
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this return an error too?

Ok(())
}

pub const TYPABLE_COMMAND_LIST: &[TypableCommand] = &[
TypableCommand {
name: "quit",
Expand Down Expand Up @@ -2794,31 +2830,7 @@ pub(super) fn command_mode(cx: &mut Context) {
}
}, // completion
move |cx: &mut compositor::Context, input: &str, event: PromptEvent| {
let parts = input.split_whitespace().collect::<Vec<&str>>();
if parts.is_empty() {
return;
}

// If command is numeric, interpret as line number and go there.
if parts.len() == 1 && parts[0].parse::<usize>().ok().is_some() {
if let Err(e) = typed::goto_line_number(cx, &[Cow::from(parts[0])], event) {
cx.editor.set_error(format!("{}", e));
}
return;
}

// Handle typable commands
if let Some(cmd) = typed::TYPABLE_COMMAND_MAP.get(parts[0]) {
let shellwords = Shellwords::from(input);
let args = shellwords.words();

if let Err(e) = (cmd.fun)(cx, &args[1..], event) {
cx.editor.set_error(format!("{}", e));
}
} else if event == PromptEvent::Validate {
cx.editor
.set_error(format!("no such command: '{}'", parts[0]));
}
let _ = process_cmd(cx, input, event);
},
);
prompt.doc_fn = Box::new(|input: &str| {
Expand All @@ -2841,6 +2853,58 @@ pub(super) fn command_mode(cx: &mut Context) {
cx.push_layer(Box::new(prompt));
}

fn expand_args<'a>(editor: &mut Editor, args: &'a str) -> Cow<'a, str> {
let reg = Regex::new(r"%(\w+)\s*\{(.*)").unwrap();
reg.replace(args, |caps: &regex::Captures| {
let remaining = &caps[2];
let end = find_first_open_right_braces(remaining);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we auto-complete for users? I think %sh{echo hello shouldn't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, above command won't expand and will be treated literately . The auto-complete is a very nice to have feature, how about we adding it after this PR.

let exp = expand_args(editor, &remaining[..end]);
let doc = doc!(editor);
let rep = match &caps[1] {
"val" => match exp.trim() {
"filename" => doc.path().and_then(|p| p.to_str()).unwrap_or("").to_owned(),
"dirname" => doc
.path()
.and_then(|p| p.parent())
.and_then(|p| p.to_str())
.unwrap_or("")
.to_owned(),
_ => "".into(),
},
"sh" => {
let shell = &editor.config().shell;
if let Ok((output, _)) = shell_impl(shell, &exp, None) {
output.trim().into()
} else {
"".into()
}
}
_ => "".into(),
};
let next = expand_args(editor, remaining.get(end + 1..).unwrap_or(""));
format!("{rep} {next}")
Copy link
Contributor

Choose a reason for hiding this comment

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

The added space between rep and next is a problem when you want to compose a single argument with the substitution. e.g.
:sh echo %val{filename},v
This currently produces two arguments "filename", ",v" instead of the desired "filename,v"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will fix it once I have time.

})
}

fn find_first_open_right_braces(str: &str) -> usize {
let mut left_count = 1;
for (i, &b) in str.as_bytes().iter().enumerate() {
match char::from_u32(b as u32) {
Comment on lines +2891 to +2892
Copy link
Contributor

@pickfire pickfire Jan 25, 2023

Choose a reason for hiding this comment

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

Shouldn't we iterate over char directly instead? I think unicode might crash in this case when we are reading bytes within multiple bytes char.

Also, the count with unicode characters is incorrect now, as_bytes may increment i more times than the actual character length str.len.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the b is a byte so that this won't crash.As we only use ascii chars in our patterns below, I didn't treat them as Unicode char, this shall work until we introduce other Unicode pattern beside %val etc.

Copy link
Contributor

@g-re-g g-re-g Jan 27, 2023

Choose a reason for hiding this comment

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

If these patterns are coming from user input there's always a chance that they can be multi-byte, or even multi code-point "characters". Because of that you might want the parser to be aware / tolerant of it.

If that is the case then String.chars probably still isn't enough and you'll need something more unicode aware if you're interested in knowing a particular characters index as a human would see it. See this note from the rust std library:

let y = "y̆";
let mut chars = y.chars();
assert_eq!(Some('y'), chars.next()); // not 'y̆'

The typical crate I've used / seen used is https://crates.io/crates/unicode-segmentation . Specifically for this implementation you could use grapheme_indices https://unicode-rs.github.io/unicode-segmentation/unicode_segmentation/trait.UnicodeSegmentation.html#tymethod.grapheme_indices .

Copy link
Contributor

@trink trink Jan 29, 2023

Choose a reason for hiding this comment

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

The returned index will always be in bounds (the enumerator and len are both byte based). Trying something like the following doesn't cause any issues
sh echo %val{𒀀𒀀𒀀𒀀𒀀𒀀𒀀} // 4 byte char f0928080

I think this should remain as is.

However, downstream I am seeing something off in shellwords (some multi-byte characters are lost in the generated argument list, I am investigating it now but it is unrelated to this PR)
edit: #5738

Some('}') => {
left_count -= 1;
if left_count == 0 {
return i;
}
}
Some('{') => {
left_count += 1;
}
_ => {}
}
}
str.len()
}

fn argument_number_of(shellwords: &Shellwords) -> usize {
if shellwords.ends_with_whitespace() {
shellwords.words().len().saturating_sub(1)
Expand Down