-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Conversation
Fix potential loss of data warning, size_t to int.
@sindresorhus The fourth number indicates the That said, this version number could be tweaked to either lop off the |
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 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:
So with this change, if you run |
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 ( Right now, folks are using the existing |
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. |
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. |
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>
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:
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 theENABLE_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
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 theTerminalApp
project'sApp
class. That is then in turn exposed by the WindowsTerminalAppHost
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.