set identifying environment variable for new connections #897

binarycrusader · 2019-05-19T07:15:29Z

Summary of the Pull Request

Set a WT_SESSION environment variable to a unique guid on every new connection to allow shell consumers to detect Windows Terminal and uniquely identify the session.

References

Unknown.

PR Checklist

Closes Consider setting environment variable when Terminal hosts cmd / powershell #840
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: Consider setting environment variable when Terminal hosts cmd / powershell #840

Detailed Description of the Pull Request / Additional comments

Set a new 'WT_SESSION' environment variable when creating new terminal connections to allow shells to detect a unique Windows Terminal session. The value of the variable is a stringified GUID as returned by
CoCreateGuid.

How verified:

"razzle" & vs debug build
runut
opencon
testcon
manual inspection

Uncertain about what tests to add and where; all of the existing ones passed.

I did some basic research to try to ensure that "WT_SESSION" is a "unique", non-conflicting environment variable and so shouldn't cause any problems with any existing programs. I'm certainly open to suggestions about the name though.

zadjii-msft

Overall looks really good

src/cascadia/TerminalConnection/ConhostConnection.cpp

src/inc/conpty-universal.h

DHowett-MSFT

This is really cool. Thanks for doing it!
I'm not 100% comfortable signing off, as I'd like @zadjii-msft and @adiviness to give it a twice-over, but I'm also not going to block.

src/cascadia/TerminalConnection/ConhostConnection.cpp

src/cascadia/TerminalConnection/ConhostConnection.h

src/inc/conpty-universal.h

src/cascadia/TerminalConnection/ConhostConnection.h

src/cascadia/TerminalConnection/ConhostConnection.cpp

src/inc/conpty-universal.h

src/cascadia/TerminalConnection/ConhostConnection.cpp

src/cascadia/TerminalConnection/ConhostConnection.h

src/cascadia/TerminalConnection/ConhostConnection.idl

Set a new 'WT_SESSION' environment variable when creating new terminal connections to allow shells to detect a unique Windows Terminal session. The value of the variable is a stringified GUID as returned by CoCreateGuid. How verified: - "razzle" & vs debug build - runut - manual inspection

* use Utils::GuidToString for guid stringification * expose guid parameter in ITerminalConnection idl

- misc. review fixes - throw if CreateConPty fails in ConhostConnection::Start - apply [[nodiscard]] and noexcept in various places

adiviness · 2019-05-22T17:54:08Z

src/cascadia/TerminalConnection/ConhostConnection.cpp

@@ -57,27 +62,41 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation

    void ConhostConnection::Start()
    {
-        std::wstring cmdline = _commandline.c_str();
+        std::wstring cmdline{ _commandline.c_str() };


I'd prefer std::wstring cmdline{ _commandline.begin(), _commandline.end() }; because then we get guarantees about the bounds o the string we're using to initialize and we don't need to dip into the behind-the-scenes data members of std::string

string has an explicit constructor from a T that has a data and size; maybe it can be used here?

(std::wstring cmdline{_commandline})

DHowett-MSFT · 2019-05-22T20:14:22Z

I am willing to push this with the nits outstanding. @adiviness, would you block on #897 (comment)?

adiviness · 2019-05-22T20:24:12Z

naw, it's probably ok. We can always fix it later as well.

DHowett-MSFT · 2019-05-22T20:33:43Z

@binarycrusader thanks so much for doing this. 👍

axelfontaine · 2022-02-17T13:29:53Z

WT_SESSION also doesn't work reliably in Win11 x64 with Windows Terminal set as default.

To reproduce, press the Windows key to open start, then type cmd + enter

This now opens a Windows Terminal cmd.exe tab via %USER_PROFILE%\AppData\Roaming\Microsoft\Windows\Start Menu\Programs\System Tools\Command Prompt.lnk where WT_SESSION is not set.

Since v0.1.1431.0, Windows Terminal sets `WT_SESSION` environment variable that we may use to detect it. > See <microsoft/terminal#897> `TERM_PROGRAM` is not supported (yet ?). > See <microsoft/terminal#1828>

sschuberth · 2023-09-18T06:58:49Z

WT_SESSION also doesn't work reliably in Win11 x64 with Windows Terminal set as default.

Right 😢 Also see #13006.

zadjii-msft requested changes May 20, 2019

View reviewed changes

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

src/inc/conpty-universal.h Outdated Show resolved Hide resolved

src/inc/conpty-universal.h 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 May 20, 2019

DHowett-MSFT reviewed May 20, 2019

View reviewed changes

zadjii-msft approved these changes May 21, 2019

View reviewed changes

src/inc/conpty-universal.h Outdated Show resolved Hide resolved

src/inc/conpty-universal.h Outdated Show resolved Hide resolved

binarycrusader mentioned this pull request May 21, 2019

Switch to v5 UUIDs as profile GUIDs for the default profiles #913

Merged

5 tasks

binarycrusader requested a review from adiviness May 21, 2019 18:14

adiviness suggested changes May 21, 2019

View reviewed changes

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 May 21, 2019

DHowett-MSFT approved these changes May 21, 2019

View reviewed changes

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

src/cascadia/TerminalConnection/ConhostConnection.h Outdated Show resolved Hide resolved

src/cascadia/TerminalConnection/ConhostConnection.idl Show resolved Hide resolved

binarycrusader added 5 commits May 21, 2019 21:32

* use winrt::guid type for connection guid

6b8e777

* use Utils::GuidToString for guid stringification * expose guid parameter in ITerminalConnection idl

- poke guid through ITerminalConnection

68e2f50

- misc. review fixes - throw if CreateConPty fails in ConhostConnection::Start - apply [[nodiscard]] and noexcept in various places

- simplify environment variable extraction in UpdateEnvironmentMapW

3be2169

- use Utils::CreateGuid instead of CoCreateGuid in ConHostConnection()

ed33510

adiviness approved these changes May 22, 2019

View reviewed changes

adiviness merged commit 1d9cdb3 into microsoft:master May 22, 2019

binarycrusader deleted the fw-sessionid branch May 23, 2019 02:21

This was referenced May 23, 2019

Code Health: apply [[nodiscard]] to functions that return status codes #951

Closed

Apply [[nodiscard]] to functions returning error codes #953

Merged

DHowett-MSFT mentioned this pull request May 28, 2019

Feature Request: Clients of terminal need a way to inquire about the capabilities of the terminal #1040

Open

karriebear mentioned this pull request May 22, 2020

On windows, show emojis only if latest windows terminal streamlit/streamlit#1490

Merged

Bios-Marcel mentioned this pull request Oct 21, 2020

Recognizing the new terminal as an application #7995

Closed

woacademy mentioned this pull request Nov 13, 2020

Please allow other spinners on Windows sindresorhus/ora#159

Closed

shrutiburman mentioned this pull request Sep 14, 2021

feat: DII 47 hyperlinks in help twilio/twilio-cli#290

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set identifying environment variable for new connections #897

set identifying environment variable for new connections #897

binarycrusader commented May 19, 2019 •

edited

Loading

zadjii-msft left a comment

DHowett-MSFT left a comment

adiviness May 22, 2019

DHowett-MSFT May 22, 2019

DHowett-MSFT May 22, 2019

DHowett-MSFT commented May 22, 2019

adiviness commented May 22, 2019

DHowett-MSFT commented May 22, 2019

axelfontaine commented Feb 17, 2022 •

edited

Loading

sschuberth commented Sep 18, 2023

set identifying environment variable for new connections #897

set identifying environment variable for new connections #897

Conversation

binarycrusader commented May 19, 2019 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

adiviness May 22, 2019

Choose a reason for hiding this comment

DHowett-MSFT May 22, 2019

Choose a reason for hiding this comment

DHowett-MSFT May 22, 2019

Choose a reason for hiding this comment

DHowett-MSFT commented May 22, 2019

adiviness commented May 22, 2019

DHowett-MSFT commented May 22, 2019

axelfontaine commented Feb 17, 2022 • edited Loading

sschuberth commented Sep 18, 2023

binarycrusader commented May 19, 2019 •

edited

Loading

axelfontaine commented Feb 17, 2022 •

edited

Loading