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

Windows Terminal CWD Shell integration (OSC 9;9;) #10166

Closed
meskill opened this issue Aug 31, 2023 · 18 comments
Closed

Windows Terminal CWD Shell integration (OSC 9;9;) #10166

meskill opened this issue Aug 31, 2023 · 18 comments
Labels
enhancement New feature or request needs-triage An issue that hasn't had any proper look
Milestone

Comments

@meskill
Copy link
Contributor

meskill commented Aug 31, 2023

Related problem

Windows Terminal for tracking the current dir in shell supports only osc 9;9; while nushell generates only osc 7 to track the cwd.

Describe the solution you'd like

Add output for osc 9;9; in nushell based on the docs that will allow to track the cwd in Windows Terminal.

Also, it should help with shell integration inside Vs Code that currently handles osc 7 differently that prevents from opening new instance completely on windows + nushell.

Describe alternatives you've considered

Users can add custom hook to implement this, e.g.

if $env.config.shell_integration {
  $env.config.hooks.env_change = ($env.config.hooks.env_change | upsert PWD {|config|
    let val = $config.PWD? | default []

    $val | append {|_, after|
      mut val = $after

      if not ($env.IS_WSL? | is-empty) {
        $val = (wslpath -w $val)
      }

      print -n $"(ansi -o '9;9;')($val)(ansi "st")"
    }
  })
}

I've tested this and it works good in Windows Terminal and Vs code integrated terminal. But it requires additional setup from users while Windows Terminal will soon or already become default terminal on Windows.

Additional context and details

I could make pr myself but I wanted to sure it is appropriate to add windows specific shell_integration (though, I believe many windows users will benefit from it). Also, it requires specific treatment for wsl shell i.e. converting path properly - it should be simple using embedded wslpath command but is it ok to call some external on every cwd change? (I haven't found any existing solution to convert path without utilizing call to the command)

@meskill meskill added enhancement New feature or request needs-triage An issue that hasn't had any proper look labels Aug 31, 2023
@fdncred
Copy link
Collaborator

fdncred commented Aug 31, 2023

IIRC, we send OSC7 on every new prompt if shell_integration is enabled. I wouldn't want to shell out to wslpath on every prompt. I'd be interested if you plan on sending both OSC7 and OSC9 on each prompt? I think we'd probably be up for this, but we need to talk it through, either here or in Discord #design-discussion channel.

@meskill
Copy link
Contributor Author

meskill commented Aug 31, 2023

Regarding that it happens on every prompt you're right as it happens on every loop of the repl.
Different terminals supports different codes so we need to support both osc 7 and osc 9;9 here.

Meanwhile, I've started the discussion in Discord

@meskill
Copy link
Contributor Author

meskill commented Sep 1, 2023

I found another enhancement related to wsl - #6436 that could be done by using wslpath too.

@fdncred
Copy link
Collaborator

fdncred commented Sep 1, 2023

As a rule of thumb, I don't like shelling out for anything as it causes performance problems. Depending on the implementation, I could be convinced to ignore that rule.

Before calling out to wslpath we'd have to be sure that

  1. We're running on Windows 10 1809 or newer. I don't think it existed before that.
  2. We're actually in WSL2. I think we could check for the existence of WSLENV.
  3. Obviously wslpath has to exist. We could probably find that using where or if it's in a known location, just check there. Like mine is in /usr/bin.
  4. Has to be an efficient implementation.

I found this that may be helpful. There may be other usages on github that may be more helpful. I just did a quick search.
https://github.com/microsoft/vscode/blob/2c021905ad67daf18551e06661f86a76e2c64129/src/vs/platform/terminal/node/ptyService.ts#L444-L484

So, if you want to take a swing at a PR, I'm up for it too.

@meskill
Copy link
Contributor Author

meskill commented Sep 1, 2023

I'll try to work on this when I have time.

Thanks for referencing example implementation.

Current points I'm considering to make it efficient as much as possible:

  1. cache the check that we are inside wsl.
  2. cache the path to wslpath executable
  3. probably changing the output of cwd related osc to execute only on actual cwd change

If anyone has any additional thoughts, please spit them out.

@Araxeus
Copy link

Araxeus commented May 29, 2024

Hey guys, not very proficient in the nu language, how would I implement shell integration for nushell like the powershell prompt in the links below? (assuming a normal windows machine without wsl)

https://learn.microsoft.com/en-us/windows/terminal/tutorials/shell-integration

https://devblogs.microsoft.com/commandline/shell-integration-in-the-windows-terminal/

(the links above are old, this feature is available on stable Windows Terminal builds, not only preview builds)

@fdncred
Copy link
Collaborator

fdncred commented May 29, 2024

@Araxeus I'd start by using nushell 0.94.0 and enabling the particular shell integration feature you wish to have from the nushell config.nu.

9;9 is implemented now so closing this issue.

@fdncred fdncred closed this as completed May 29, 2024
@Araxeus
Copy link

Araxeus commented May 29, 2024

cool timing, I have updated to 0.94.0, enabled all shell integrations available (including osc9_9) and sadly none of the Windows Terminal shell features seem to work

  • cannot copy command/output
  • checkmarks dont seem to work

even duplicate tab fails because the current pwd is malformed:
image

@hustcer hustcer added this to the v0.94.0 milestone May 29, 2024
@fdncred
Copy link
Collaborator

fdncred commented May 29, 2024

Looks like you don't have something setup right. I'd guess how WT is launching nushell. I have all the shell_integration items enabled and don't have issues with nushell.

@Araxeus
Copy link

Araxeus commented May 29, 2024

  • cannot copy command/output
  • checkmarks dont seem to work

resolved by itself, I just needed to completely close windows terminal apparently

even duplicate tab fails because the current pwd is malformed:
image

but that didn't resolve, could you check if duplicate tab works for you? there seems to be an extra trailing / at the beggining

@fdncred
Copy link
Collaborator

fdncred commented May 29, 2024

I get something similar with duplicate tab. Could be a bug, not sure.

@fdncred
Copy link
Collaborator

fdncred commented May 29, 2024

yup, it's a bug. i see it here.

2024-05-29 03:54:08.865 PM [WARN ] nu_cli::repl: "\u{1b}]9;9;/C:\\Users\\username\\source\\repos\\forks\\nushell\u{1b}\\"

@meskill
Copy link
Contributor Author

meskill commented May 29, 2024

Also, by default, it won't work for WSL tabs without wslpath conversion compared to the hook from the description.

@fdncred
Copy link
Collaborator

fdncred commented May 29, 2024

I'm not sure of all of the magic that is in wslpath. I wonder if it's open source?

@meskill
Copy link
Contributor Author

meskill commented May 29, 2024

I'm not sure of all of the magic that is in wslpath. I wonder if it's open source?

no, it's a built-in tool in WSL. Another option could be to try open-source alternatives and I saw only this one for rust

@fdncred
Copy link
Collaborator

fdncred commented May 29, 2024

I can't get that one working.

fdncred added a commit that referenced this issue May 29, 2024
# Description

This fixes a bug in the `OSC 9;9` functionality where the path wasn't
being constructed properly and therefore wasn't getting set right for
things like "Duplicate Tab" in Windows Terminal. Thanks to @Araxeus for
finding it.

Related to #10166

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use toolkit.nu; toolkit test stdlib"` to run the
tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
@fdncred
Copy link
Collaborator

fdncred commented May 30, 2024

It looks like shelling out to wslpath may be the best way to do this for WSL but it's kind of niche, so I'm not sure it's worth it.

@fdncred
Copy link
Collaborator

fdncred commented May 30, 2024

After looking at this for a few hours, the version of WSL2 that I have is terribly buggy wrt OSC 9;9. I can't upgrade right now. So, this will have to wait.

Just to document some debug code and what I was doing, here it is.

fn run_shell_integration_osc9_9(engine_state: &EngineState, stack: &mut Stack, use_color: bool) {
    #[allow(deprecated)]
    if let Ok(path) = current_dir_str(engine_state, stack) {
        let start_time = Instant::now();

        // Let's check to see if we're in Microsoft's WSL
        let wsl_distro = engine_state
            .get_env_var("WSL_DISTRO_NAME")
            .map_or("NO_DISTRO".into(), |v| {
                v.to_expanded_string("", engine_state.get_config())
            });

        if wsl_distro == "NO_DISTRO" {
            // Otherwise, communicate the path as OSC 9;9 from ConEmu (often used for spawning new tabs in the same dir)
            // This is helpful in Windows Terminal with Duplicate Tab
            eprintln!("path: {path}");
            run_ansi_sequence(&format!(
                "\x1b]9;9;{}\x1b\\",
                percent_encoding::utf8_percent_encode(&path, percent_encoding::CONTROLS)
            ));
        } else {
            // Otherwise, communicate the path as OSC 9;9 from WSL (often used for spawning new tabs in the same dir)
            // This is helpful in Windows Terminal with Duplicate Tab
            let wsl_path = format!(
                "\\\\wsl.localhost\\{}{}",
                wsl_distro,
                path.replace("/", "\\")
            );
            // let wsl_path = format!("\\\\wsl$\\{}{}", wsl_distro, path.replace("/", "\\"));

            // let wsl_path = format!("//wsl.localhost/{}{}", wsl_distro, path);

            eprintln!("wsl_path: {wsl_path}");
            let seq = &format!(
                "\x1b]9;9;{}\x1b\\",
                percent_encoding::utf8_percent_encode(&wsl_path, percent_encoding::CONTROLS)
            );
            eprintln!("seq: {:#?}", seq);
            run_ansi_sequence(seq);
        }

        perf(
            "communicate path to terminal with osc9;9",
            start_time,
            file!(),
            line!(),
            column!(),
            use_color,
        );
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-triage An issue that hasn't had any proper look
Projects
None yet
Development

No branches or pull requests

4 participants