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

Run shell commands asynchronously #6373

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

sarah-quinones
Copy link

@sarah-quinones sarah-quinones commented Mar 20, 2023

this pr adds a basic implementation of run-async-shell-command (alias = 'async-sh') which runs the shell command asynchronously while discarding its output. this is helpful when the command can run for a long time and its output doesn't matter (or can be piped to a file if needed), and it may be desirable to do so without blocking the editor

@pascalkuthe
Copy link
Member

pascalkuthe commented Mar 20, 2023

I think the behavior of :sh should be changed instead of adding a new command. There is no reason for :sh to block the editor as it doesn't insert any text into the buffer.

I general I want to move helix to a point where we never block the editor on IO unless absolutely necessary (pasting from the clipboard, or piping into the editor, in which case there should be a timeout) so this would be a nice enhancement 👍 You can just replace the call to shell_impl with shell_async_impl and await the result in async callback we already have (you would just need to move the output.is_empty check and setting the editor status into the callback

@pascalkuthe pascalkuthe added C-enhancement Category: Improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. E-easy Call for participation: Experience needed to fix: Easy / not much labels Mar 20, 2023
@sarah-quinones
Copy link
Author

i changed it so that sh is non blocking. and displays the output once everything's done. is there a way to make it so that the output is displayed incrementally as new lines appear?

@pascalkuthe
Copy link
Member

pascalkuthe commented Mar 20, 2023

is there a way to make it so that the output is displayed incrementally as new lines appear?

Yes but that's possible but would require larger changes and would probably be a bit tricky to implement. You would need to split shell_impl_async into a subfunction that just returns the spawned process without waiting for it to finish. so that it just returns the process (and the input_task if necessary). You can then use the ChildStdout reader to asyncrounsly read lines. Currently the Markdown popup used for rendering shell output can't change it's content so you would need a way to do that. The best way would probably to just use an Arc<Mutex<String>> and to push to that String from the background task whenever a full line has been read to a buffer. You probably need to add an AtomicBool into the Arc that is set to true by the rendering thread as so as it reaches the Mutex that is checked reguarlarly by the background thread so that it doesn't try to lock the Mutex while rendering (which would block rendering) and give up the lock if already acquired.

It's definitely a nice improvement I have thaught about before but a bit more work. You can decide yourself If you want to do it in this PR or a seperatly.

@sarah-quinones
Copy link
Author

i think doing it in a separate PR is best. i don't think im familiar with the codebase enough to accomplish that yet. but i'll be giving a try once i find some time

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor nit 👍

Comment on lines 5040 to 5049
fn async_shell_impl(
shell: &[String],
cmd: &str,
input: Option<Rope>,
) -> impl std::future::Future<Output = anyhow::Result<(Tendril, bool)>> {
let shell = shell.to_vec();
let cmd = cmd.to_string();
async move { shell_impl_async(&shell, &cmd, input).await }
}

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we really need a helper for this. I don't foresee this pattern really being used in other commands and it's simple enough to just inline into the callsite

Copy link
Author

Choose a reason for hiding this comment

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

i pushed a commit that inlines it, and updated the commit message

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

LGTM now, nice usability improvement 👍

@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 20, 2023
@the-mikedavis the-mikedavis changed the title add async shell command Run shell commands asynchronously Mar 21, 2023
@the-mikedavis the-mikedavis merged commit 28632c6 into helix-editor:master Mar 21, 2023
sagnibak pushed a commit to sagnibak/helix that referenced this pull request Mar 21, 2023
icecreammatt pushed a commit to icecreammatt/helix that referenced this pull request Apr 19, 2023
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
@David-Else
Copy link
Contributor

I think I have hit an example where async is not doing what I would like:

Z = { Z = [":sh kitty @ close-window --match title:^hx- --ignore-no-match", ":wq" ], Q = ":q!" }

ZZ does not close the kitty window, but if I remove ":wq" it does. I assume it is a problem with the delay in the kitty command, any idea of a solution? Thanks!

smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
@nikaro
Copy link
Contributor

nikaro commented Aug 13, 2024

I run into a smiliar issue than @David-Else, i'm trying to do this:

[keys.normal.space]
t = [":sh theme-sync", ":config-reload"]

The config is reloaded before my command terminates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants