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 TERM_PROGRAM[_VERSION] env vars, partial fix #1040 #1828

Closed

Conversation

rkeithhill
Copy link
Contributor

Fix potential loss of data warning, size_t to int.

Summary of the Pull Request

Create the following environment variables for every conhost app spawned:

TERM_PROGRAM                   WindowsTerminal
TERM_PROGRAM_VERSION           0.0.1.0

This allows shell, shell scripts, PowerShell modules and console applications to determine if they need to tweak the console environment. For example, in the posh-git module we actually mess with low-level console mode settings for old versions of PowerShell.

https://github.com/dahlbyk/posh-git/blob/a64e5e073f6ce4dcd01394965bf0bbc91a0e3016/src/ConsoleMode.ps1#L3-L4

Even for an old version (v5) of Windows PowerShell, if it's running in Windows Terminal, we probably do not need to call SetConsoleMode() to set the ENABLE_VIRTUAL_TERMINAL_PROCESSING flag. I'd like to change that test above to also not mess with the console mode if $env:TERM_PROGRAM is defined.

References

Partially addresses #1040

PR Checklist

  • Closes #xxx
  • [x ] 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: #xxx

Detailed Description of the Pull Request / Additional comments

I "think" the right place to set these env vars is early on in the WindowsTerminal main() entry point. I wasn't able to access the package version number directly from that project. So I expose it from the TerminalApp project's App class. That is then in turn exposed by the WindowsTerminal AppHost class. I'm completely open to a better way to get the package version. Or if the env vars should be set in another project - like the headless conhost.

Validation Steps Performed

Start WindowsTerminal and checked for the existence of these two env vars in powershell.exe, pwsh.exe and cmd.exe.

Fix potential loss of data warning, size_t to int.
@sindresorhus
Copy link

TERM_PROGRAM_VERSION 0.0.1.0

Would be nicer if the version could be valid semver, to ease parsing.

I'm not sure what the last part indicates. If it's a build number, it should be 0.0.0+0, or if it's a prerelease indicator, it should be 0.0.1-0.

@rkeithhill
Copy link
Contributor Author

@sindresorhus The fourth number indicates the revision level. This comes from the UWP package version and is typical for Windows/UWP version numbers which pre-date semver. The format is major.minor.build.revision. This is the same version number displayed in the About dialog box. For the Windows Store installed version it would be 0.2.1831.0.

That said, this version number could be tweaked to either lop off the revision e.g. 0.2.1831 or add it as build metadata 0.2.1831+0.

@zadjii-msft
Copy link
Member

I'm still not totally sure that this is the right solution to feature detection.

First off, I'm almost certain that just because your application is running attached to the Windows Terminal doesn't mean you don't need to call ENABLE_VIRTUAL_TERMINAL_PROCESSING. You'll still need to do this, since the app is still running attached to a conhost (acting as conpty) first and foremost.

Secondly, I'm not certain that this is something that we'd want to expose to commandline apps. I'd think that for the most part, commandline apps should behave the same in conhost as they do in Windows Terminal. while there might be differences now, the goal is to make sure there's feature parity. What feature of Windows Terminal are we trying to detect with the addition of this variable, and is there not a better way of checking if that specific feature works?

Thirdly, mintty/mintty#776 makes me particularly worried about implementing this. Namely:

The problem with such variables is that they are not only set in the child shells but also in any other terminal started from them. So if you run mintty and start an xterm in it, it will still have those values.

So with this change, if you run start cmd from a Windows Terminal window, the conhost that spawns to host the new cmd.exe instance is still going to have these variables set, so they're not really helping in that case.

@rkeithhill
Copy link
Contributor Author

rkeithhill commented Jul 8, 2019

there might be differences now

There are definitely differences right now particularly when it comes to PowerShell/PSReadLine line-editing shortcuts. Shortcuts like ctrl+backspace do not work as expected so users need to define new keyboard shortcuts to get the equivalent functionality (BackwordKillWord in this case).

Right now, folks are using the existing WT_SESSION env var that does get created. Maybe that is enough? I do appreciate that the ideal is that there would be no difference vs running in a regular console. Is that realistic though?

@zadjii-msft
Copy link
Member

I think it'll be possible in the future to have there be effectively no differences between the Terminal and conhost. The Ctrl+Backspace thing you mention is something that needs to be fixed, by # #530.

@miniksa
Copy link
Member

miniksa commented Feb 12, 2020

This has been open for many months and we're not really sure what we want to do here still. So I'm going to close this for now so it stops showing on the review queue and we can revisit it in the future if need be.

@miniksa miniksa closed this Feb 12, 2020
HorlogeSkynet pushed a commit to HorlogeSkynet/archey4 that referenced this pull request Sep 18, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants