Implement ConEmu's OSC 9;9 to set the CWD #8330

skyline75489 · 2020-11-19T05:36:02Z

Summary of the Pull Request

This PR implement the OSC 9;9

Sequence	Descriptoin
ESC ] 9 ; 9 ; “cwd” ST	Inform ConEmu about shell current working directory.

References

#8214

PR Checklist

Closes Consider using OSC 9;9 to inform Windows Terminal about CWD #8166
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated.
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

skyline75489 · 2020-11-19T05:44:02Z

So this is basically a reborn version of #7668 . Thanks to prior work of @PankajBhojwani, this PR is even smaller than #7688 .

Based on previous thread like #3158 and #8214, I'd like to implement OSC 9;9 as a "Windows-first" sequence that will assume users will set Windows path . This can be seen as a starter towards a more comprehensive implementation of OSC 7, which will handle all kinds of paths based on the profile used.

As for this PR, it gives users the ability to make use the of the CWD when duplicating the current active tab. I don't really know how to handle splitting panes so I left it unchanged.

I'd like to hear you guys opinions.

skyline75489 · 2020-11-19T05:49:41Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -1015,6 +1015,13 @@ namespace winrt::TerminalApp::implementation
                    if (profileGuid.has_value())
                    {
                        const auto settings{ winrt::make<TerminalSettings>(_settings, profileGuid.value(), *_bindings) };
+                        const auto workingDirectory = terminalTab->GetActiveTerminalControl().WorkingDirectory();
+                        const auto validWorkingDirectory = !workingDirectory.empty();


I relaxed the validation per the comments by @zadjii-msft in #8166 (comment)

I'm not sure how we handle wt -d c:\some\invalid\path nowadays. Could use some help here.

So currently, wt -d c:\some\invalid\path will do this:

That's not a super useful error, ERROR_DIRECTORY - I guess that makes sense, though not totally obvious what's wrong here.

I'm okay with us leaving this for now. We do some validation of the startingDirectory on settings load, but nothing for directories passed as commandline args, and I think that's fine.

If we did want to try and pre-validate, we could pull Profile::EvaluateStartingDirectory into a helper, and only use the control's directory if it's valid. That's actually probably a good idea.

zadjii-msft

Overall, this looks pretty good. No major concerns - but I'm on the fence about validating the paths ahead of time. I want this to land, but maybe we should validate ahead of time.

I'll block for the discussion, but it's otherwise a fine PR

EDIT: I'm also blocking over the pane duplicating. We should probably do that.

zadjii-msft · 2020-12-15T17:04:41Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -1015,6 +1015,13 @@ namespace winrt::TerminalApp::implementation
                    if (profileGuid.has_value())
                    {
                        const auto settings{ winrt::make<TerminalSettings>(_settings, profileGuid.value(), *_bindings) };
+                        const auto workingDirectory = terminalTab->GetActiveTerminalControl().WorkingDirectory();
+                        const auto validWorkingDirectory = !workingDirectory.empty();


So currently, wt -d c:\some\invalid\path will do this:

That's not a super useful error, ERROR_DIRECTORY - I guess that makes sense, though not totally obvious what's wrong here.

I'm okay with us leaving this for now. We do some validation of the startingDirectory on settings load, but nothing for directories passed as commandline args, and I think that's fine.

If we did want to try and pre-validate, we could pull Profile::EvaluateStartingDirectory into a helper, and only use the control's directory if it's valid. That's actually probably a good idea.

zadjii-msft · 2020-12-15T17:07:45Z

As far as pane duplicating goes, that's down in TerminalPage::_SplitPane. Somewhere near:

            if (splitMode == SplitType::Duplicate)
            {
                std::optional<GUID> current_guid = focusedTab->GetFocusedProfile();
                if (current_guid)
                {
                    profileFound = true;
                    controlSettings = { winrt::make<TerminalSettings>(_settings, current_guid.value(), *_bindings) };
                    realGuid = current_guid.value();
                }

skyline75489 · 2020-12-16T03:02:41Z

Another thing that I'm not so sure: in #7668 I make both new-tab and duplicate-tab use the CWD. In this PR, I figured it's too aggresive so I changed it to just duplicate-tab. Is this too aggresive, or what people would want? Personally I'm OK with new-tab also accepting CWD but I'm not sure what others will feel about this.

zadjii-msft · 2020-12-16T13:02:57Z

I'd say let's just start with the duplicateTab action. We could probably consider adding an argument to newTab to allow it to duplicate paths as well. But that parameter can wait for a follow-up PR (let's just get this in to start with 😄). That's my opinion at least.

skyline75489 · 2020-12-17T10:28:20Z

I've been thinking about the path validation, then it occurs to me that the OSC 9;9 feature is currently a super opt-in kind of things. Users will need to modify their shell script in order to even get this up and running. So I guess even if they have wrongly configured the path and be frustrated by the error when launching x.exe, they are likely to know the root cause of it is exactly the misconfiguration created by themselves. Besides, with only duplicateTab/Pane enabled now, there's little chance that a WSL path is passed to powershell.exe or vice versa, which considerably reduces the chances of misformated URLs being passed around across different profiles. So yeah I think this is ready now.

zadjii-msft · 2020-12-18T11:48:25Z

Yea I'm fine with not path validating as well. Can we get this added to duplicating panes though too 😄?

zadjii-msft

I'm a derp, this does work with pane splitting too. So let's do this

ghost · 2021-01-11T18:00:55Z

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

## Summary of the Pull Request This PR implement the OSC 9;9 |Sequence|Descriptoin| | :------------- | :----------: | |ESC ] 9 ; 9 ; “cwd” ST | Inform ConEmu about shell current working directory.|  ## References microsoft#8214  ## PR Checklist * [X] Closes microsoft#8166 * [X] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx  ## Detailed Description of the Pull Request / Additional comments  ## Validation Steps Performed

This PR fixes the parsing of OSC 9;9 sequences with path surrounded by quotation marks. Original OSC 9;9 PR: #8330 Unit test added. Manually tested with oh-my-posh. Closes #8930

This PR fixes the parsing of OSC 9;9 sequences with path surrounded by quotation marks. Original OSC 9;9 PR: #8330 Unit test added. Manually tested with oh-my-posh. Closes #8930 (cherry picked from commit 0811c57)

tavrez · 2021-03-02T01:33:22Z

I can't use this in todays releases anymore(both stable and preview). Something changed?

skyline75489 · 2021-03-02T02:54:28Z

@tavrez No nothing changed I believe. Are you positive that you are using the "duplicate tab"(ctrl + shift + d) instead of "creating new tab" (ctrl + shift + t) ? If you have a consistent repro, feel free to open a new issue.

tavrez · 2021-03-02T14:36:54Z

Yes I'm using duplicate tab(with keyboard shortcut and from command pallete), It doesn't work, it goes back to starting directory. If you are not having this problem(simply open a new terminal, change directory, press duplicate tab) probably I'm doing something wrong(but this procedure worked for me in prev version it introduced in Terminal Preview)

skyline75489 · 2021-03-05T03:19:16Z

@tavrez It's working for me. Sorry I guess you'll need to check the shell configuration you're using.

LeaveTheCapital · 2021-03-19T13:38:49Z

This does not work for me either in latest preview, or in latest Windows Terminal, when using duplicate tab (ctrl + shift + d). Am I missing something? Is there a setting that needs to be enabled?

tavrez · 2021-03-19T13:41:36Z

@LeaveTheCapital just to make sure, do you send the sequence code (ESC ] 9 ; 9 ; “cwd” ST) before duplicating? If yes, you probably has a weird problem like me, I can't use this only on my personal PC

LeaveTheCapital · 2021-05-20T14:29:21Z

@LeaveTheCapital just to make sure, do you send the sequence code (ESC ] 9 ; 9 ; “cwd” ST) before duplicating? If yes, you probably has a weird problem like me, I can't use this only on my personal PC

I did try this, however I must have been doing something wrong - perhaps someone could help with an idiots guide, or a link to similar? Thank you

skyline75489 · 2021-05-24T06:18:00Z

@LeaveTheCapital You can try the gist to see if it helps. Make sure you are using "Duplicate Tab" (Ctrl+Shift+d, by default) and not "New Tab" (Ctrl + T)

Implement ConEmu's OSC 9;9 to set the CWD

2e32277

ghost added Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Nov 19, 2020

skyline75489 commented Nov 19, 2020

View reviewed changes

zadjii-msft self-assigned this Nov 20, 2020

zadjii-msft requested changes Dec 15, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 15, 2020

zadjii-msft removed their assignment Dec 15, 2020

This was referenced Dec 15, 2020

Tests we should maybe write one day #7001

Open

"Toast" notifications #8592

Open

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 16, 2020

skyline75489 added 2 commits December 17, 2020 17:56

Merge branch 'master' into chesterliu/dev/osc-9-9

733c205

Resolve comments

5aa3096

skyline75489 marked this pull request as ready for review December 17, 2020 10:28

skyline75489 mentioned this pull request Dec 17, 2020

Initial implementation of OSC 7 #7668

Closed

6 tasks

zadjii-msft approved these changes Dec 18, 2020

View reviewed changes

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Dec 18, 2020

ghost requested review from miniksa, carlos-zamora, DHowett, leonMSFT and PankajBhojwani December 18, 2020 12:09

skyline75489 mentioned this pull request Dec 21, 2020

Consider using OSC 9;9 to inform Windows Terminal about CWD #8166

Closed

carlos-zamora approved these changes Jan 8, 2021

View reviewed changes

Merge branch 'main' into chesterliu/dev/osc-9-9

8245f90

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 11, 2021

ghost merged commit e557a86 into microsoft:main Jan 11, 2021

TBBle mentioned this pull request Jan 20, 2021

Open new terminal tab in same directory as existing tab (OSC 7?) #3158

Open

skyline75489 deleted the chesterliu/dev/osc-9-9 branch January 25, 2021 02:57

skyline75489 mentioned this pull request Jan 29, 2021

Improve OSC 9;9 parsing logic & add tests #8934

Merged

floh96 mentioned this pull request Feb 13, 2021

Support for OSC 9;9 JanDeDobbeleer/oh-my-posh#409

Closed

zadjii-msft mentioned this pull request Mar 19, 2021

We should add a tutorial for using OSC9;9 MicrosoftDocs/terminal#285

Closed

zadjii-msft mentioned this pull request Apr 7, 2021

Setting startingDirectory to WSL path causes indefinite application hang #9541

Closed

zadjii-msft mentioned this pull request May 17, 2021

Duplicate the tab with the same path #10105

Closed

vamolessa mentioned this pull request Jun 30, 2021

Add support for OSC-7 to let terminals know the shell's CWD nushell/nushell#3707

Closed

skyline75489 mentioned this pull request Aug 13, 2021

Open a new pane with the environment vars of the parent #4259

Open

bfahrenfort mentioned this pull request Apr 23, 2022

Add an action to "Open in File Explorer" #12859

Open

davidanthoff mentioned this pull request Aug 14, 2024

Emit OSC 9;9 escape sequences on Windows JuliaLang/julia#55493

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ConEmu's OSC 9;9 to set the CWD #8330

Implement ConEmu's OSC 9;9 to set the CWD #8330

skyline75489 commented Nov 19, 2020 •

edited

Loading

skyline75489 commented Nov 19, 2020

skyline75489 Nov 19, 2020

zadjii-msft Dec 15, 2020

zadjii-msft left a comment •

edited

Loading

zadjii-msft Dec 15, 2020

zadjii-msft commented Dec 15, 2020

skyline75489 commented Dec 16, 2020

zadjii-msft commented Dec 16, 2020 •

edited

Loading

skyline75489 commented Dec 17, 2020 •

edited

Loading

zadjii-msft commented Dec 18, 2020

zadjii-msft left a comment

ghost commented Jan 11, 2021

tavrez commented Mar 2, 2021 •

edited

Loading

skyline75489 commented Mar 2, 2021

tavrez commented Mar 2, 2021

skyline75489 commented Mar 5, 2021

LeaveTheCapital commented Mar 19, 2021

tavrez commented Mar 19, 2021

LeaveTheCapital commented May 20, 2021

skyline75489 commented May 24, 2021

Implement ConEmu's OSC 9;9 to set the CWD #8330

Implement ConEmu's OSC 9;9 to set the CWD #8330

Conversation

skyline75489 commented Nov 19, 2020 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

skyline75489 commented Nov 19, 2020

skyline75489 Nov 19, 2020

Choose a reason for hiding this comment

zadjii-msft Dec 15, 2020

Choose a reason for hiding this comment

zadjii-msft left a comment • edited Loading

Choose a reason for hiding this comment

zadjii-msft Dec 15, 2020

Choose a reason for hiding this comment

zadjii-msft commented Dec 15, 2020

skyline75489 commented Dec 16, 2020

zadjii-msft commented Dec 16, 2020 • edited Loading

skyline75489 commented Dec 17, 2020 • edited Loading

zadjii-msft commented Dec 18, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

ghost commented Jan 11, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

tavrez commented Mar 2, 2021 • edited Loading

skyline75489 commented Mar 2, 2021

tavrez commented Mar 2, 2021

skyline75489 commented Mar 5, 2021

LeaveTheCapital commented Mar 19, 2021

tavrez commented Mar 19, 2021

LeaveTheCapital commented May 20, 2021

skyline75489 commented May 24, 2021

skyline75489 commented Nov 19, 2020 •

edited

Loading

zadjii-msft left a comment •

edited

Loading

zadjii-msft commented Dec 16, 2020 •

edited

Loading

skyline75489 commented Dec 17, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

tavrez commented Mar 2, 2021 •

edited

Loading