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

Improve startingDirectory functionality #604

Merged
merged 6 commits into from
May 11, 2019
Merged

Improve startingDirectory functionality #604

merged 6 commits into from
May 11, 2019

Conversation

fghzxm
Copy link
Contributor

@fghzxm fghzxm commented May 9, 2019

This commit adds the startingDirectory property to the default-created
cmd and powershell profiles, with the default value
%HOMEDRIVE%%HOMEPATH%.

Signed-off-by: Fred Miller fghzxm@outlook.com

@fghzxm
Improve startingDirectory functionality
e4ef0cf 
This commit adds the `startingDirectory` property to the default-created
`cmd` and `powershell` profiles, with the default value
`%HOMEDRIVE%%HOMEPATH%`.

Signed-off-by: Fred Miller <fghzxm@outlook.com>
@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented May 9, 2019 via email

@fghzxm
Copy link
Contributor Author

fghzxm commented May 9, 2019

@DHowett-MSFT I inspected the lnks to cmd.exe and powershell.exe in my Start Menu, and they all have %HOMEDRIVE%%HOMEPATH% as their starting directories. I wanted to match that behavior as strictly as possible.

binarycrusader
binarycrusader reviewed May 9, 2019
View reviewed changes
src/cascadia/TerminalApp/CascadiaSettings.cpp Outdated
@@ -123,6 +122,7 @@ void CascadiaSettings::_CreateDefaultProfiles()
Profile defaultProfile{};
defaultProfile.SetFontFace(L"Consolas");
defaultProfile.SetCommandline(L"cmd.exe");
defaultProfile.SetStartingDirectory(L"%HOMEDRIVE%%HOMEPATH%");
Copy link
Member

Choose a reason for hiding this comment

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

HOMEDRIVE and HOMEPATH may not be set in all scenarios. If you look at powershell core it has to deal with this today. So unless an empty result here is valid, this logic probably needs to be centralized.

Copy link
Member

Choose a reason for hiding this comment

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

See notes here:
https://stackoverflow.com/posts/36392591/revisions

@binarycrusader
Copy link
Member

binarycrusader commented May 9, 2019 •
edited
Loading

@DHowett-MSFT I inspected the lnks to cmd.exe and powershell.exe in my Start Menu, and they all have %HOMEDRIVE%%HOMEPATH% as their starting directories. I wanted to match that behavior as strictly as possible.

Correct:
image

But as far as I can tell, that default isn't set in Powershell code, it's set in the shortcut. Should the same be true of Terminal? Should the shortcut created by the deployment point there? Can a UWP app do that?

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented May 9, 2019 via email

@fghzxm
Use %USERPROFILE% to replace %HOMEDRIVE%%HOMEPATH%
43e9b9c 
This commit changes `%USERPROFILE%` in the default profiles to
`%HOMEDRIVE%%HOMEPATH%`.

https://stackoverflow.com/posts/36392591/revisions says `%USERPROFILE%`
is better than `%HOMEDRIVE%%HOMEPATH%`, so changed it.

Signed-off-by: Fred Miller <fghzxm@outlook.com>
@fghzxm
Copy link
Contributor Author

fghzxm commented May 9, 2019

BTW, should we use %SystemRoot%\system32\cmd.exe and %SystemRoot%\system32\WindowsPowerShell\v1.0\powershell.exe instead of simply cmd.exe and powershell.exe? (I guess we'll want path qualification if we want to have seperate 64-bit and 32-bit PowerShell profiles.)

@DHowett-MSFT
Copy link
Contributor

BTW, should we use %SystemRoot%\system32\cmd.exe and %SystemRoot%\system32\WindowsPowerShell\v1.0\powershell.exe instead of simply cmd.exe and powershell.exe? (I guess we'll want path qualification if we want to have seperate 64-bit and 32-bit PowerShell profiles.)

Ah, almost certainly. I'm on the fence about pwsh.exe (for the powershell core installation), because I imagine that people put the right one on their %PATH%... what do you think?

@zadjii-msft zadjii-msft added this to the Windows Terminal v1.0 milestone May 9, 2019
@fghzxm
Copy link
Contributor Author

fghzxm commented May 9, 2019

I'm on the fence about pwsh.exe (for the powershell core installation), because I imagine that people put the right one on their %PATH%... what do you think?

@DHowett-MSFT Wait, I don't think PowerShell Core is shipped with Windows.

@zadjii-msft
Copy link
Member

@fghzxm we actually have some code in the terminal already that attempts to detect if powershell core is installed, and uses pwsh instead of powershell if it is :)

@ccurrens ccurrens mentioned this pull request May 10, 2019
Closed
@fghzxm
Improve startingDirectory functionality
37ac537 
This commit adds the `startingDirectory` property to the default-created
`cmd` and `powershell` profiles, with the default value
`%HOMEDRIVE%%HOMEPATH%`.

Signed-off-by: Fred Miller <fghzxm@outlook.com>
@fghzxm
Use %USERPROFILE% to replace %HOMEDRIVE%%HOMEPATH%
94ea1d7 
This commit changes `%USERPROFILE%` in the default profiles to
`%HOMEDRIVE%%HOMEPATH%`.

https://stackoverflow.com/posts/36392591/revisions says `%USERPROFILE%`
is better than `%HOMEDRIVE%%HOMEPATH%`, so changed it.

Signed-off-by: Fred Miller <fghzxm@outlook.com>
@fghzxm
Merge upstream master
e65f8ff 
Signed-off-by: Fred Miller <fghzxm@outlook.com>
DHowett-MSFT
DHowett-MSFT approved these changes May 11, 2019
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

This is great, thanks @fghzxm.

We'll probably not merge until Monday once we're all back in the office from the weekend. 😄

@fghzxm
Consolidate constant
a4e4780 
Refer to the externally defined constant in code.

Signed-off-by: Fred Miller <fghzxm@outlook.com>
@DHowett-MSFT DHowett-MSFT merged commit 6088134 into microsoft:master May 11, 2019
@fghzxm fghzxm deleted the starting-directory branch May 11, 2019 08:13
@DHowett-MSFT DHowett-MSFT mentioned this pull request May 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@binarycrusader binarycrusader binarycrusader left review comments

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Terminal v1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@fghzxm @DHowett-MSFT @binarycrusader @zadjii-msft