Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use configured default program when running subcommand "start" #851

Closed
wants to merge 2 commits into from
Closed

Use configured default program when running subcommand "start" #851

wants to merge 2 commits into from

Conversation

exactly-one-kas
Copy link
Contributor

Running wezterm-gui start --cwd . starts the system's default shell, regardless of the configured default_prog. This PR fixes that.

Copy link
Member

@wez wez left a comment

Choose a reason for hiding this comment

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

Thanks for digging in!
I've made a suggested alternative way to fix this bug below.
I'd prefer it if you separated out the other change to the GUI current dir as I don't think it is related to this issue and I'm not sure of your intent with that change.

Thanks!

@@ -326,7 +327,13 @@ fn run_terminal_gui(opts: StartCommand) -> anyhow::Result<()> {

let cmd = if need_builder {
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 doing this is a little cleaner:

Suggested change
let cmd = if need_builder {
let config = config::configuration();
let prog = opts.prog.iter().map(|s| s.as_os_str()).collect::<Vec<_>>();
let mut builder = config.build_prog(if prog.is_empty() { None } else { Some(prog) })?;
if let Some(cwd) = opts.cwd {
builder.cwd(cwd);
}
Some(builder)

@@ -508,7 +517,22 @@ fn run() -> anyhow::Result<()> {
SetStdHandle(STD_ERROR_HANDLE, stderr.into_raw_file_descriptor());
*/

std::env::set_current_dir(config::HOME_DIR.as_path())?;
// Only change the current directory to the user's home path if
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't appear to be related to the issue that you set out to resolve in the PR.
Please remove it from here and track this separately, along with an explanation of why it is needed.

FWIW, I don't think this is necessary: this is controlling the current directory of the GUI process itself, and not its child processes.

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 for starting wezterm in the current working directory from a GUI context. My use case is starting a terminal from my file manager, which starts the process in the currently opened directory. Since there's no console available, the current directory gets reset to the user's home directory. I'll put this into a separate PR.

@wez wez closed this in 4fcfb4b Jun 10, 2021
wez added a commit that referenced this pull request Jun 10, 2021
@wez
Copy link
Member

wez commented Jun 10, 2021

Thanks; I pulled in and amended the first part of this!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants