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.

Sign up for GitHub

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

Jump to bottom

Set working directory when invoked from shell extension #10546

Merged
1 commit merged into from
Jul 9, 2021
Merged

Set working directory when invoked from shell extension #10546

1 commit merged into from
Jul 9, 2021

Conversation

ianjoneill
Copy link
Contributor

@ianjoneill ianjoneill commented Jul 2, 2021 •
edited by DHowett
Loading

Sets the working directory of the terminal when invoked from the shell extension. This ensures that new tabs opened with a starting directory of . open in the directory that the terminal was invoked from.

Closes #8933

Validation Steps Performed

Manually tested - default PowerShell profile set to use home directory, Windows PowerShell profile set to use current directory. Launched via the shell extension and the default profile opened in the explorer directory, as did a new Windows PowerShell tab.

@ghost ghost added Area-ShellExtension For issues related to the explorer right-click context menu Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Jul 2, 2021
lhecker
lhecker approved these changes Jul 5, 2021
View reviewed changes
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

The code is correct, but I hope someone else can review the logical correctness.

zadjii-msft
zadjii-msft previously requested changes Jul 6, 2021
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I hope someone else can review the logical correctness.

blocking for aforementioned logical correctness discussion

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 6, 2021
@ianjoneill
Copy link
Contributor Author

So my thinking behind this was that it would make it similar to you running wt -d . from the address bar in explorer.

The screenshot below shows the current directory for different launch types:

  1. Launch from shell extension without this change
  2. Launch from shell extension with this change
  3. Launch from wt -d . from the address bar in explorer

Current Directory

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 7, 2021
@DHowett
Copy link
Member

DHowett commented Jul 9, 2021

So, this will definitely work... but only for the default configuration of Terminal. It starts to get a little dicey when you use the "New windows open in ... existing instance of terminal" option. Then again, this doesn't make it worse for anyone else; it is just as ambiguous for reused instances as it ever was. 😁

DHowett
DHowett approved these changes Jul 9, 2021
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

On account of the thing I said, I'm cool with this. It's not worse than it was, and it is better for our default configuration. We'll have to figure out what to do in general with the cwd when Remoting is on.

@DHowett DHowett dismissed zadjii-msft’s stale review July 9, 2021 18:53

logical review complete

@DHowett
Copy link
Member

DHowett commented Jul 9, 2021

Thanks for the contribution!

@DHowett DHowett added zPreview-Service-Consider AutoMerge Marked for automatic merge by the bot when requirements are met labels Jul 9, 2021
@ghost
Copy link

ghost commented Jul 9, 2021

Hello @DHowett!

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.

@ghost ghost merged commit f152573 into microsoft:main Jul 9, 2021
@ianjoneill
Copy link
Contributor Author

Yeah I see what you mean.

The current behaviour (new tabs get the working directory that was used to launch the "parent" terminal window) makes sense from a process hierarchy perspective, but I could understand people wanting something else. If it were to be made configurable I get the impression could be a bit tricky to describe in a clear way!

DHowett pushed a commit that referenced this pull request Jul 12, 2021
@ianjoneill @DHowett
Set working directory when invoked from shell extension (#10546)
5c93e4b 
Sets the working directory of the terminal when invoked from the shell extension. This ensures that new tabs opened with a starting directory of `.` open in the directory that the terminal was invoked from.

Closes #8933

## Validation Steps Performed
Manually tested - default PowerShell profile set to use home directory, Windows PowerShell profile set to use current directory. Launched via the shell extension and the default profile opened in the explorer directory, as did a new Windows PowerShell tab.

(cherry picked from commit f152573)
@ghost
Copy link

ghost commented Jul 14, 2021

🎉Windows Terminal v1.9.1942.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Jul 14, 2021
Closed
@ghost
Copy link

ghost commented Jul 14, 2021

🎉Windows Terminal Preview v1.10.1933.0 has been released which incorporates this pull request.:tada:

Handy links:

@adrianghc
Copy link

adrianghc commented Jul 24, 2021 •
edited
Loading

This feature does not work for me. I'm using WT Preview 1.10.1933.0. New tabs don't launch in the directory of the first tab when launching WT from the shell extension.

@ianjoneill
Copy link
Contributor Author

This feature does not work for me. I'm using WT Preview 1.10.1933.0. New tabs don't launch in the directory of the first tab when launching WT from the shell extension.

Hi @adrianghc, could you open a new issue and provide a copy of your settings.json?

@adrianghc adrianghc mentioned this pull request Jul 25, 2021
Closed
@adrianghc
Copy link

This feature does not work for me. I'm using WT Preview 1.10.1933.0. New tabs don't launch in the directory of the first tab when launching WT from the shell extension.

Hi @adrianghc, could you open a new issue and provide a copy of your settings.json?

Done at #10789.

@zadjii-msft zadjii-msft mentioned this pull request Aug 16, 2021
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett DHowett DHowett approved these changes

@lhecker lhecker lhecker approved these changes

@zadjii-msft zadjii-msft zadjii-msft left review comments

Assignees
No one assigned
Labels
Area-ShellExtension For issues related to the explorer right-click context menu AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"Open Windows Terminal here" does not continue to work for subsequent tabs, panes
5 participants
@ianjoneill @DHowett @adrianghc @lhecker @zadjii-msft