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

Add a WT_SETTINGS_DIR env variable that portable profiles can use #16949

Merged
merged 3 commits into from
Mar 28, 2024
Merged

Add a WT_SETTINGS_DIR env variable that portable profiles can use #16949

merged 3 commits into from
Mar 28, 2024

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Mar 27, 2024 •
edited
Loading

Basically, title.

It'd be a neat idea for portable installs of the Terminal to reference files that are right there in the portable install.

This PR adds a WT_SETTINGS_DIR var to Terminal's own env block. This allows us to resolve profiles relative to our own settings folder.

Closes #16295

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Mar 27, 2024
DHowett
DHowett requested changes Mar 27, 2024
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.

I love it. I've also got a couple thoughts...

If we want to include the root path for WT, can it be WT_ROOT instead?

Alternatively... should it actually be a path to the settings root? That would let an entire settings folder -- whether it is in LocalAppData, the Package storage, or in a portable install -- roam independent of its EXE path. I ask mainly because the package root is immutable and useless to most people (for example).

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 27, 2024
@zadjii-msft
Copy link
Member Author

Alternatively... should it actually be a path to the settings root? That would let an entire settings folder -- whether it is in LocalAppData, the Package storage, or in a portable install

This is a better idea.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Mar 27, 2024
@zadjii-msft zadjii-msft self-assigned this Mar 27, 2024
@zadjii-msft
Copy link
Member Author

image

@zadjii-msft zadjii-msft assigned DHowett and unassigned zadjii-msft Mar 28, 2024
DHowett
DHowett requested changes Mar 28, 2024
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.

Still wondering if it should be WT_SETTINGS_ROOT (or DIR) since it doesn’t point at the json file itself! Also make sure you update your PR title/body!

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 28, 2024
@lhecker
Copy link
Member

lhecker commented Mar 28, 2024

I believe _DIR is better than _ROOT because dirname exists but rootname doesn't.

@zadjii-msft zadjii-msft changed the title Add a WT_FOLDER_PATH env variable that portable profiles can use Add a WT_SETTINGS_DIR env variable that portable profiles can use Mar 28, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 28, 2024
@DHowett DHowett enabled auto-merge March 28, 2024 14:03
@DHowett DHowett requested a review from lhecker March 28, 2024 19:57
@DHowett DHowett added this pull request to the merge queue Mar 28, 2024
lhecker
lhecker reviewed Mar 28, 2024
View reviewed changes
src/cascadia/TerminalApp/AppLogic.cpp
// Do this here, rather than at the top of main. This will prevent us from
// including this variable in the vars we serialize in the
// Remoting::CommandlineArgs up in HandleCommandlineArgs.
_setupFolderPathEnvVar();
Copy link
Member

Choose a reason for hiding this comment

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

FWIW an alternative, but more robust approach may be to have our own ExpandEnvironmentStringsW. It would also work better together with virtual env vars and the like. We already have such a function in a way in til::env already too.

Merged via the queue into main with commit 36c81f2 Mar 28, 2024
20 checks passed
@DHowett DHowett deleted the dev/migrie/b/16295-portable-env-var branch March 28, 2024 20:44
DHowett pushed a commit that referenced this pull request Apr 22, 2024
@zadjii-msft @DHowett
Add a WT_SETTINGS_DIR env variable that portable profiles can use (#1…
b5b32da 
…6949)

Basically, title.

It'd be a neat idea for portable installs of the Terminal to reference
files that are right there in the portable install.

This PR adds a `WT_SETTINGS_DIR` var to Terminal's own env block. This
allows us to resolve profiles relative to our own settings folder.

Closes #16295

(cherry picked from commit 36c81f2)
Service-Card-Id: 92352620
Service-Version: 1.20
@daxgames daxgames mentioned this pull request May 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhecker lhecker lhecker approved these changes

@DHowett DHowett DHowett approved these changes

Assignees

@DHowett DHowett

Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Attention The core contributors need to come back around and look at this ASAP. Product-Terminal The new Windows Terminal.
Projects
1.20 Servicing Pipeline
Status: Shipped
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set environment variable WT_FOLDER_PATH to the folder path that contains WindowsTerminal.exe
3 participants
@zadjii-msft @lhecker @DHowett