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

terminal: Override default working directory to be home dir #17838

Closed

Conversation

accidentaldevelopment
Copy link

If there is not a specific project directory, the terminal defaults to /.

The working_directory setting goes through some processing, and then eventually gets passed through to the alacritty_terminal crate to actually handle the terminal.

It seems that alacritty, if no working directory is set (None), will default to the directory of the foreground process. If Zed is not run from the command line with a directory argument, or doesn't have a project directory, the running directory would be /, so terminals open at root.

Zed's various working_directory options all fall back to the default None. This change replaces that None with the user's home directory. Relying on alacritty to do that with the default could potentially disrupt Zed functionality if their functionality ever changes.

Closes #15969

Release Notes:

There are several ways to resolve this, and I'm not completely sure this is the correct way. The idea is to deal with a None value in Zed instead of Alacritty. Alacritty defaults could change, and this change is agnostic of that.

My concern is that util::paths::home_dir() panics if the homedir can't be determined, which would crash at some arbitrary point when the user starts the terminal. Something can still result in None would allow alacritty to choose its default and avoid a panic.

Happy to iterate on this and find something that fixes both scenarios, but this seemed the simplest for now.

Copy link

cla-bot bot commented Sep 15, 2024

We require contributors to sign our Contributor License Agreement, and we don't have @accidentaldevelopment on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

@accidentaldevelopment
Copy link
Author

@cla-bot check

Copy link

cla-bot bot commented Sep 15, 2024

The cla-bot has been summoned, and re-checked this pull request!

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Sep 15, 2024
@maxdeviant maxdeviant changed the title terminal: overwrite default working directory to be home dir terminal: Override default working directory to be home dir Sep 15, 2024
If there is not a specific project directory, the terminal defaults to
`/`.

The `working_directory` setting goes through some processing, and then
eventually gets passed through to the `alacritty_terminal` crate to
actually handle the terminal.

It seems that alacritty, if no working directory is set (`None`), will
default to the directory of the foreground process. If Zed is not run
from the command line with a directory argument, or doesn't have a
project directory, the running directory would be /, so terminals open
at root.

Zed's various `working_directory` options all fall back to the default
`None`. This change replaces that `None` with the user's home directory.
Relying on alacritty to do that with the default could potentially
disrupt Zed functionality if their functionality ever changes.
Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

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

The idea idea is nice generally, yet it feels that the implementation could be different: certain cases such as ssh remoting might use None as "irrelevant dir that will be set via ssh means instead", and using it in the lowest, new, method makes it impossible to override.

Also, the new working_directory is not Option<> anymore but some path — having it not reflected in the code is also odd.

What if we adjust the doc and add some enum instead of Option here:

///Gets the working directory for the given workspace, respecting the user's settings.
/// None implies "~" on whichever machine we end up on.
pub fn default_working_directory(workspace: &Workspace, cx: &AppContext) -> Option<PathBuf> {
match &TerminalSettings::get_global(cx).working_directory {

so all places like

TerminalKind::Shell(path) => path.as_ref().map(|path| path.to_path_buf()),

let local_path = if ssh_command.is_none() {
path.clone()
} else {
None
};

will have to use a special method of that enum to get the Option<PathBuf> instead?

E.g.

/// some docs are nice to have
enum DefaultWorkingDirectory {
    Home,
    Path(Option<PathBuf>),
}

impl DefaultWorkingDirectory {
    pub fn path(&self) -> Option<PathBuf> { ... }
}

alternatively, we can go over all TerminalKind::Shell and default_working_directory calls and double check the semantics — nicely noticed that Alacritty uses something else.

@SomeoneToIgnore SomeoneToIgnore self-assigned this Sep 16, 2024
@SomeoneToIgnore
Copy link
Contributor

Closing this as stale, but overall the idea is nice — it's the approach that has to be different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terminal: cwd default use $HOME
2 participants