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.

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

Add WT_PROFILE_ID to the environment of the spawned process #4852

Merged
23 commits merged into from
Apr 17, 2020

Conversation

oising
Copy link
Collaborator

@oising oising commented Mar 9, 2020

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

Sorry, something went wrong.

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.

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 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 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
Copy link
Collaborator Author

oising commented Mar 10, 2020

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...

image

wsl

image

(yes, need to normalize the guid formatting)

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
@zadjii-msft zadjii-msft added the Product-Terminal The new Windows Terminal. label Mar 11, 2020
@oising oising requested a review from DHowett-MSFT March 11, 2020 17:47
@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Mar 12, 2020
@ghost ghost requested review from miniksa, carlos-zamora and leonMSFT March 12, 2020 17:19
@DHowett-MSFT
Copy link
Contributor

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

@JustinGrote
Copy link

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
Copy link
Collaborator Author

oising commented Mar 24, 2020

(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
Copy link

@oising this was directed at me I believe.

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ghost 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
@oising
Copy link
Collaborator Author

oising commented Apr 10, 2020

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.

@ghost 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
@zadjii-msft
Copy link
Member

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
Copy link
Contributor

Jaykul commented Apr 12, 2020

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

Jaykul added a commit to Jaykul/EzTheme that referenced this pull request Apr 12, 2020
The PR is blocked until after 1.0, but it looks like this is what they're going to use
microsoft/terminal#4852
@oising
Copy link
Collaborator Author

oising commented Apr 14, 2020

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

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.

Thanks for bearing with us ☺️

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.

Just the one concern. Thanks Oisin!

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
src/cascadia/TerminalConnection/ConptyConnection.cpp Outdated Show resolved Hide resolved
@ghost 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
Co-Authored-By: Dustin L. Howett (MSFT) <duhowett@microsoft.com>
@ghost 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
@DHowett-MSFT 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 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
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.

SGTM thanks so much Oisin

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

ghost commented Apr 17, 2020

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.

@ghost ghost merged commit 38e7cb0 into microsoft:master Apr 17, 2020
@Jaykul
Copy link
Contributor

Jaykul commented Apr 18, 2020

Thanks for rethinking this one, y'all.

@ghost
Copy link

ghost commented Apr 22, 2020

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

Handy links:

sylveon pushed a commit to LazoCoder/Pokemon-Terminal that referenced this pull request May 1, 2020
* 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 oising deleted the feature/profile-env-vars branch May 2, 2020 21:11
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose the profile GUID to the environment (WT_PROFILE, WT_PROFILE_GUID?)
7 participants