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

Use ANSI color codes if SupportsVirtualTerminal #304

Closed
wants to merge 4 commits into from

Conversation

dahlbyk
Copy link
Owner

@dahlbyk dahlbyk commented Sep 15, 2016

I have been nerd-sniped: Jaykul/PowerLine#1.

The notion of Write-Prompt either returning a string (maybe empty) or actually doing a Write-Host seems quite wrong, but I'm not sure how else to handle it without breaking users of older hosts. Any ideas, @rkeithhill?

Opened as WIP because I'm seeing some weird behavior in the prompt after running Git commands that use the pager. Haven't had a chance to dig into what might be going on there, but I'm open to ideas.
powershell_2016-09-14_23-39-18

Closes #282


Also, for SEO purposes (because I was surprised that I had to write them myself), herein can be found PowerShell functions to convert ConsoleColor to ANSI color escape codes/sequences.

@dahlbyk
Copy link
Owner Author

dahlbyk commented Sep 16, 2016

Opened as WIP because I'm seeing some weird behavior in the prompt after running Git commands that use the pager. Haven't had a chance to dig into what might be going on there, but I'm open to ideas.

It's pretty clearly an issue with less.exe or PowerShell's virtual terminal support, but I'm way out of my depth on troubleshooting this (cc @shiftkey). A child powershell process affects its parent session, but a new child shell is unaffected. 😒
powershell_2016-09-15_23-22-30

I have confirmed that the LESS and TERM environment variables are not set. Various fiddling hasn't found any magic combinations that fix it.

The first hit on "powershell virtual terminal" brings up PowerShell/PowerShell#1177, which references less.exe. Will drop a note over there, too.

@dahlbyk
Copy link
Owner Author

dahlbyk commented Sep 16, 2016

Hrm... wondering if Write-ColoredHost from https://blog.fastconnect.fr/?p=568 would be a better approach? (via @aaronmcdavid in #282 (comment))

@mcoolive, I don't see a license on your blog - can we have permission to use that code? (BTW, your blog gives a certificate error: NET::ERR_CERT_COMMON_NAME_INVALID.)

Thinking implementation would look something like...

  1. Replace Write-Prompt with Get-ColoredString
  2. Extract Get-GitStatusPrompt from Write-GitStatus, keeping the latter for backward compatibility using Write-ColoredHost
  3. Export Get-GitStatusPrompt and Get-VcsStatusPrompt

@lzybkr
Copy link
Collaborator

lzybkr commented Sep 16, 2016

You are hitting the issue I reported (well, it's my bug) with escape sequences being emitted after running git.

The workaround for now is to set the console mode. It's a simple P/Invoke, I have a sample (that does more than you need) here: https://gist.github.com/lzybkr/f2059cb2ee8d0c13c65ab933b75e998c

In my personal prompt, I simply call Set-ConsoleMode -ANSI using the above gist, I think it's reasonable to do the equivalent in a function like Get-VcsStatusPrompt.

function Get-ForegroundVirtualTerminalSequence($Color) {
$e = [char]27 + "["
switch ($Color) {
([ConsoleColor]::Black) { "${e}30m" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should use break, otherwise every case is needlessly tested.
You should also consider ordering the cases so that the more commonly used colors appear first - PowerShell does not generate efficient switch tables so each case is tested before the preceding cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, using an array would be more efficient - indexing into the array will be much faster.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Check my latest? (I moved these functions to a different file, too.)

@Jaykul
Copy link

Jaykul commented Sep 19, 2016

@eweilnau is working on a Write-AnsiHost if you want to go that route (the idea being to accept escaped text and output using write-host for compatibilty) -- he's trying to implement a little more than just colors (i.e. movement).

@dahlbyk
Copy link
Owner Author

dahlbyk commented Sep 19, 2016

In my personal prompt, I simply call Set-ConsoleMode -ANSI using the above gist, I think it's reasonable to do the equivalent in a function like Get-VcsStatusPrompt.

👍

What would be the preferred way to guard against trying to compile or use that type on non-Windows?


@eweilnau is working on a Write-AnsiHost if you want to go that route (the idea being to accept escaped text and output using write-host for compatibilty) -- he's trying to implement a little more than just colors (i.e. movement).

🆒 not sure if it would make sense to take a dependency on PowerLine or copy the relevant bits here.

@@ -262,7 +273,10 @@ if ($Host.UI.RawUI.BackgroundColor -eq [ConsoleColor]::DarkMagenta) {
$s.WorkingForegroundColor = $s.WorkingForegroundBrightColor
}

function Global:Write-VcsStatus { $Global:VcsPromptStatuses | foreach { & $_ } }
function Global:Write-VcsStatus {
Set-ConsoleMode -ANSI
Copy link
Owner Author

Choose a reason for hiding this comment

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

It works! 🎉

@dahlbyk dahlbyk changed the title [WIP] Use ANSI color codes if SupportsVirtualTerminal Use ANSI color codes if SupportsVirtualTerminal Sep 19, 2016
@Jaykul
Copy link

Jaykul commented Sep 20, 2016

We could definitely consider collaborating on a "escape sequences to write-host" translation stuff as a small external LegacyHost module, and PowerLine and Posh-Git could both take dependencies on that, if you like?

By the way, I forgot to mention this: I don't just test for SupportsVirtualTerminal, I also explicitly support ConEmu -- because currently, their ANSI support is far more thorough than Windows', and it works way back on old versions of Windows 😁

$Host.UI.SupportsVirtualTerminal -or ($Env:ConEmuANSI -eq "ON")

- Enabled if $Host.UI.SupportsVirtualTerminal
- Enabled if %ConEmuANSI% is 'ON'
- Can be manually set in $PROFILE for other ANSI-capable consoles
@dahlbyk
Copy link
Owner Author

dahlbyk commented Sep 21, 2016

We could definitely consider collaborating on a "escape sequences to write-host" translation stuff as a small external LegacyHost module, and PowerLine and Posh-Git could both take dependencies on that, if you like?

👍

By the way, I forgot to mention this: I don't just test for SupportsVirtualTerminal, I also explicitly support ConEmu -- because currently, their ANSI support is far more thorough than Windows', and it works way back on old versions of Windows 😁

$Host.UI.SupportsVirtualTerminal -or ($Env:ConEmuANSI -eq "ON")

Good tip. I've moved the check to the initialization of $GitPromptSettings.AnsiConsole, which should allow users of other ANSI-capable consoles to make the switch.

@Jaykul
Copy link

Jaykul commented Sep 22, 2016

Oh, I like that, I'll add a $UseAnsi switch parameter in my set-powerlineprompt instead of testing myself.

@rkeithhill
Copy link
Collaborator

@dahlbyk

What would be the preferred way to guard against trying to compile or use that type on non-Windows?

This is what I use for testing for isAdminProcess:

if (($PSVersionTable.PSVersion.Major -le 5) -or $IsWindows) {

Cross-platform PowerShell starts at v6. At v6 and after, you can test for the Windows platform via the built-in variable $IsWindows.

@dahlbyk dahlbyk mentioned this pull request Dec 29, 2016
13 tasks
@dahlbyk
Copy link
Owner Author

dahlbyk commented Dec 29, 2016

This is what I use for testing for isAdminProcess:

if (($PSVersionTable.PSVersion.Major -le 5) -or $IsWindows) {

Cross-platform PowerShell starts at v6. At v6 and after, you can test for the Windows platform via the built-in variable $IsWindows.

Would it be unreasonable to just do something like this early on, to use throughout?

$IsWindows = $IsWindows -or ($PSVersionTable.PSVersion.Major -le 5)

@rkeithhill
Copy link
Collaborator

rkeithhill commented Dec 29, 2016

$IsWindows is readonly on PowerShell v6 so let's come up with a slightly different name. Also do the $PSVersionTable.PSVersion.Major -le 5 first because if it is $true it will short-circuit the second test $IsWindows. This variable doesn't exist on v5 or lower.

There is also another rub here. It turns out that for IsAdminProcess testing, the required types are available on .NET Core but only on Windows. In other cases, we may want to test specifically for PS Core or PS Desktop. For that, we test first for ($PSVersionTable.PSVersion.Major -le 5) and if that is $true we must be on PS Desktop. If false, then we test for $PSVersionTable.PSEdition. It will be Desktop for the Windows desktop and Core for PS Core on any platform.

@Jaykul
Copy link

Jaykul commented Feb 27, 2017

You should be able to just test: if($global:IsWindows -ne $False) { "Yep, it's Windows" } unless you do that strict-mode thing

@dahlbyk
Copy link
Owner Author

dahlbyk commented Mar 1, 2017

unless you do that strict-mode thing

Speaking of which, we've had a few PRs come in about fixing strict mode errors but I don't really have an opinion. Does the PS community have a convention on modules' handling of strict mode?

@dahlbyk
Copy link
Owner Author

dahlbyk commented Mar 1, 2017

Reopening targeting develop for 1.0.

@Jaykul
Copy link

Jaykul commented Mar 1, 2017

we've had a few PRs come in about fixing strict mode errors but I don't really have an opinion. Does the PS community have a convention on modules' handling of strict mode?

Let me just say:

  1. Shell languages are dynamically scoped. All of them.
  2. Nobody should set StrictMode in the global scope.

I can't speak for the community, but I don't see much value in strict mode -- because it largely forces me to give up dynamic scoping. I mean, it's great that it can catch the fact that $SomeVariable isn't set before it's read -- maybe that was a mistake. But in this case (and many others) that's the whole point.

Who thinks this is more maintainable?:

if((Get-Variable IsWindows -Scope Global -ValueOnly -ErrorAction Ignore) -ne $False) { "Yep, it's Windows" } 

@rkeithhill
Copy link
Collaborator

From some (very limited) testing I did it seems that Set-StrictMode in global scope doesn't impact script in a module. RE someVariable isn't set - PSSA helps with that to some extent (when it isn't generating false positives).

@dahlbyk
Copy link
Owner Author

dahlbyk commented Mar 1, 2017

Who thinks this is more maintainable?

:trollface:

I tend to agree - if we're going to use a dynamic language, we might as well take advantage of it being dynamic.

@Jaykul
Copy link

Jaykul commented Mar 2, 2017

From some (very limited) testing I did it seems that Set-StrictMode in global scope doesn't impact script in a module. RE someVariable isn't set - PSSA helps with that to some extent (when it isn't generating false positives).

I believe @rkeithhill is correct, although I don't quite know why -- StrictMode is supposed to affect child scopes, and modules are (technically?) in the global scope. But I'll take it 😉

Anyway, bottom line is: don't worry about it.

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.

None yet

4 participants