Add WT_PROFILE_ID to the environment of the spawned process #4852

oising · 2020-03-09T20:11:47Z

This commit adds a WT_PROFILE_ID environment variable, which contains
the guid of the active profile.

It also teaches ConptyConnection to take an environment map on creation.

We had to do a little manual jiggery with the WSLENV environment
variable as passed by the creator.

CLA signed
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already.

Ran terminal, validated vars and translated paths under windows and WSL.

References #4566 (this PR originally introduced WT_SETTINGS/DEFAULTS)
Closes #3589

zadjii-msft

Honestly this looks okay to me. Plumbing the StringMap through makes it easier to add some of the other requests (looking at #3589) as well.

I've just got some nits about dead code, but the changes to OpenConsole.sln probably shouldn't be here. That's literally the only thing blocking me

OpenConsole.sln

src/cascadia/TerminalConnection/ConptyConnection.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

oising · 2020-03-10T18:01:49Z

Honestly this looks okay to me. Plumbing the StringMap through makes it easier to add some of the other requests (looking at #3589) as well.

It seems trivial to roll #3589 into this PR as well -- shall I? it's another three lines of code...

wsl

(yes, need to normalize the guid formatting)

src/cascadia/TerminalApp/TerminalPage.cpp

DHowett-MSFT · 2020-03-18T00:38:48Z

(On account of you deleted your comment, I'm guessing something else was up 😉)

JustinGrote · 2020-03-18T06:09:31Z

If it was me, I realized my comment was incorrect, it was special local config I did. All is good and looking forward to seeing this in 0.11!

oising · 2020-03-24T19:54:54Z

(On account of you deleted your comment, I'm guessing something else was up 😉)

Hey @DHowett-MSFT -- did I miss something? Do I need to update the PR?

JustinGrote · 2020-03-24T20:55:35Z

@oising this was directed at me I believe.

miniksa

Looks good to me.

oising · 2020-04-10T19:31:28Z

I'm switching myself to make sure this doesn't get automerged into 1.0 accidentally. I believe the consensus on the team is that this is a bit too much of a "feature" to get merged this close to 1.0 tbh. I've moved it into 1.x to have the discussion once the official 1.0 build has been cut 😄

That's disappointing. You guys are shipping 1.0 without a means to edit settings from the UI, and now making it even harder to find the files if you do need to edit them.

zadjii-msft · 2020-04-10T19:42:24Z

I mean, Ctrl+, will open the settings file by default, and %LOCALAPPDATA%\packages\Microsoft.WindowsTerminal_8wekyb3d8bbwe\LocalState\settings.json == %WT_SETTINGS% for all intents and purposes. Sure it's more verbose, but it's no less correct. I get how this is useful, but we're just in a hard place where accepting new features like this late in the cycle is a little hard to sell up the chain unfortunately 😕

Just because it's not making the 1.0 cut doesn't mean it's never happening. It's not like we're gonna ship 1.0 then all get the 'rona and disappear. We just need to make hard cuts on what actually gets in at this point unfortunately (case in point #3789). We'll certainly come back to this ☺️

Jaykul · 2020-04-12T15:40:13Z

In my opinion, the thing that's really missing in 1.0 is WT_PROFILE_ID (#3589 which cannot be discovered at all ... @zadjii-msft

And since the VT color queries were also never finished, there's no way to discover what crazy colors someone's using in their terminal

The PR is blocked until after 1.0, but it looks like this is what they're going to use microsoft/terminal#4852

…T_PROFILE_ID

…terminal into feature/profile-env-vars

oising · 2020-04-14T15:18:18Z

As per discussion with @DHowett-MSFT at #4566 (comment) - removed WT_SETTINGS and WT_DEFAULTS and left WT_PROFILE_ID

zadjii-msft

Thanks for bearing with us ☺️

DHowett-MSFT

Just the one concern. Thanks Oisin!

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalConnection/ConptyConnection.cpp

Co-Authored-By: Dustin L. Howett (MSFT) <duhowett@microsoft.com>

src/cascadia/TerminalConnection/ConptyConnection.cpp

DHowett-MSFT

SGTM thanks so much Oisin

ghost · 2020-04-17T17:15:02Z

Hello @DHowett-MSFT!

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.

Jaykul · 2020-04-18T03:17:23Z

Thanks for rethinking this one, y'all.

ghost · 2020-04-22T17:14:30Z

🎉Windows Terminal Preview v0.11.1121.0 has been released which incorporates this pull request.:tada:

Handy links:

* Add Windows Terminal adapter * [Windows Terminal] Get defaultProfile on globals * [Windows Terminal] Update with latest version - profiles.json to settings.json microsoft/terminal#5199 - Change current profile microsoft/terminal#4852 * [Windows Terminal] Rename adapter to pass tests * [Windows Terminal] Update with default profile settings https://github.com/microsoft/terminal/blob/master/doc/user-docs/UsingJsonSettings.md#default-settings

oising added 6 commits March 9, 2020 12:04

add wt_defaults and wt_profile env vars

9aaae49

remove commented code

c06b6e7

wt_profile -> wt_profiles (match filename)

410f9ff

as per devops nit, invoke-codeformat

bf2c2bd

add nullptr check around _environment (az shell won't set these vars)

fd71f0a

make nullptr check idiomatic with rest of codebase

5ad2c6a

zadjii-msft requested changes Mar 10, 2020

View reviewed changes

OpenConsole.sln Outdated Show resolved Hide resolved

src/cascadia/TerminalConnection/ConptyConnection.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Mar 10, 2020

oising added 4 commits March 10, 2020 14:48

address review nits; add WT_PROFILE_ID var for current profile

1d5524b

undo VS mods to openconsole.sln

fe534fa

remove more dead code

bd75029

add newline to end of openconsole.sln... grrr

e8309b8

JustinGrote mentioned this pull request Mar 10, 2020

Expose the profile GUID to the environment (WT_PROFILE, WT_PROFILE_GUID?) #3589

Closed

oising self-assigned this Mar 10, 2020

oising requested a review from zadjii-msft March 10, 2020 20:13

zadjii-msft approved these changes Mar 10, 2020

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved

zadjii-msft added the Product-Terminal The new Windows Terminal. label Mar 11, 2020

address review: switch to Utils::GuidToString

10fbf81

oising requested a review from DHowett-MSFT March 11, 2020 17:47

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

ghost requested review from miniksa, carlos-zamora and leonMSFT March 12, 2020 17:19

miniksa approved these changes Mar 25, 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 Apr 10, 2020

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

Jaykul added a commit to Jaykul/EzTheme that referenced this pull request Apr 12, 2020

Switch to WT_PROFILE_ID per pr 4852

bd94ecf

The PR is blocked until after 1.0, but it looks like this is what they're going to use microsoft/terminal#4852

DHowett-MSFT mentioned this pull request Apr 13, 2020

Consider adding a WT_PROFILE env var pointing to settings file #4566

Open

oising added 2 commits April 14, 2020 11:15

Remove WT_DEFAULTS and WT_SETTINGS env vars for the moment, leaving W…

e1c0051

…T_PROFILE_ID

Merge branch 'feature/profile-env-vars' of https://github.com/oising/…

6b5e65e

…terminal into feature/profile-env-vars

zadjii-msft approved these changes Apr 14, 2020

View reviewed changes

Merge branch 'master' into feature/profile-env-vars

8bfd932

DHowett-MSFT suggested changes Apr 16, 2020

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved

src/cascadia/TerminalConnection/ConptyConnection.cpp Outdated Show resolved Hide resolved

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

Update src/cascadia/TerminalConnection/ConptyConnection.cpp

898d128

Co-Authored-By: Dustin L. Howett (MSFT) <duhowett@microsoft.com>

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

oising added 2 commits April 16, 2020 21:18

address dustin's comments (don't clobber wslenv)

afa43fa

fix code formatting

c6ed972

jsoref reviewed Apr 17, 2020

View reviewed changes

src/cascadia/TerminalConnection/ConptyConnection.cpp Outdated Show resolved Hide resolved

DHowett-MSFT changed the title ~~add wt_defaults and wt_profiles env vars~~ Add WT_PROFILE_ID and add an env map to ConptyConnection Apr 17, 2020

DHowett-MSFT changed the title ~~Add WT_PROFILE_ID and add an env map to ConptyConnection~~ Add WT_PROFILE_ID to the environment of the spawned process Apr 17, 2020

DHowett-MSFT approved these changes Apr 17, 2020

View reviewed changes

DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 17, 2020

ghost merged commit 38e7cb0 into microsoft:master Apr 17, 2020

oising deleted the feature/profile-env-vars branch May 2, 2020 21:11

abarani mentioned this pull request Jul 30, 2020

Duplicate WT_SESSION and WT_PROFILE_ID in WSLENV env var #7130

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WT_PROFILE_ID to the environment of the spawned process #4852

Add WT_PROFILE_ID to the environment of the spawned process #4852

oising commented Mar 9, 2020 •

edited by DHowett-MSFT

Loading

zadjii-msft left a comment

oising commented Mar 10, 2020 •

edited

Loading

DHowett-MSFT commented Mar 18, 2020

JustinGrote commented Mar 18, 2020

oising commented Mar 24, 2020

JustinGrote commented Mar 24, 2020

miniksa left a comment

oising commented Apr 10, 2020

zadjii-msft commented Apr 10, 2020

Jaykul commented Apr 12, 2020 •

edited

Loading

oising commented Apr 14, 2020

zadjii-msft left a comment

DHowett-MSFT left a comment

DHowett-MSFT left a comment

ghost commented Apr 17, 2020

Jaykul commented Apr 18, 2020

ghost commented Apr 22, 2020

Add WT_PROFILE_ID to the environment of the spawned process #4852

Add WT_PROFILE_ID to the environment of the spawned process #4852

Conversation

oising commented Mar 9, 2020 • edited by DHowett-MSFT Loading

zadjii-msft left a comment

Choose a reason for hiding this comment

oising commented Mar 10, 2020 • edited Loading

DHowett-MSFT commented Mar 18, 2020

JustinGrote commented Mar 18, 2020

oising commented Mar 24, 2020

JustinGrote commented Mar 24, 2020

miniksa left a comment

Choose a reason for hiding this comment

oising commented Apr 10, 2020

zadjii-msft commented Apr 10, 2020

Jaykul commented Apr 12, 2020 • edited Loading

oising commented Apr 14, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

ghost commented Apr 17, 2020

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.

Jaykul commented Apr 18, 2020

ghost commented Apr 22, 2020

oising commented Mar 9, 2020 •

edited by DHowett-MSFT

Loading

oising commented Mar 10, 2020 •

edited

Loading

Jaykul commented Apr 12, 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.