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 #447

Merged
merged 5 commits into from
Mar 3, 2017

Conversation

dahlbyk
Copy link
Owner

@dahlbyk dahlbyk commented Mar 1, 2017

See #304 for detail and much conversation. For now I have just rebased on develop, resolved all the conflicts, and updated the new default prompt.

Closes #282 and Jaykul/PowerLine#1.

@dahlbyk dahlbyk added this to the v1.0 milestone Mar 1, 2017
@dahlbyk
Copy link
Owner Author

dahlbyk commented Mar 1, 2017

Collect Git prompt in ArrayList instead of strings

Not sure if this is a good optimization or not. Thoughts, @lzybkr?


More generally, I wonder if this is Close Enough™ for now to merge, giving basic support and a fixed point from which to iterate on how exactly ANSI colors work. In addition to the actual ANSI implementation (dedicated module?), colors play a key role in prompt customization (cc #340 #345).

@dahlbyk dahlbyk requested a review from rkeithhill March 1, 2017 06:53
@@ -0,0 +1,103 @@
# Hack! https://gist.github.com/lzybkr/f2059cb2ee8d0c13c65ab933b75e998c

if ($IsWindows -eq $false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For non-cross-platform PowerShell Core, we should test $PSVersionTable.PSVersion.Major -ge 6 -and $IsWindows. As that variable is only defined in PowerShell Core. We can assume a major version of -lt 6 is Windows only.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thought is that $IsWindows will be $null for PS -lt 6, and $null -ne $false. Net result is that we'll only create NativeConsoleMethods if we're not explicitly on not-Windows.

Or are you thinking that we shouldn't create NativeConsoleMethods for older versions of PS?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I find the logic a bit cryptic. Maybe this is worth a helper function IsWindows to encapsulate the logic. RE NativeConsoleMethods no need to create these if they are not used and we won't use them except on Windows w/ANSI support, right?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I wonder if the native console methods have an impact on ConEmu and other console hosts? Do they even exist on Windows pre-10?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the initial impetus for this? PowerShell sets the console mode to enable ANSI seqs. Or was there a version (5.0) that didn't do this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The api exists on all versions of Windows - but the flag we need is not recognized universally - just Win10 Anniversary and later I think. The api call will fail (I think harmlessly) if it doesn't recognize the flag.

The workaround should not be needed in V6 - I fixed it in PowerShell: PowerShell/PowerShell#2991

@@ -97,6 +97,8 @@ $global:GitPromptSettings = [pscustomobject]@{

EnableWindowTitle = 'posh~git ~ '

AnsiConsole = $Host.UI.SupportsVirtualTerminal -or ($Env:ConEmuANSI -eq "ON")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the SupportsVirtualTerminal property was added in PS 5.1. I have started a prototype for ANSI stuff based on v5 classes. In it, I have a PromptRenderer class that has a HostSupportsAnsi method. That method returns $false for version less than 5.1. It returns false for Windows version less than 10.0.10586.0 (which is the build that support conhost ANSI seqs). Finally, it returns the value of $Host.UI.SupportsVirtualTerminal.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm not sure there's a practical difference, is there? If SupportsVirtualTerminal isn't set, it's falsey.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're probably right. I carry some baggage having written scripts back in the early PS days that were affected by users setting Set-StrictMode -Version Latest in their global session. Fortunately that does not impact modules. That and I still cling to statically typed notions and referring to a property that may or may not exist just feels wrong. ;-)

Copy link
Owner Author

Choose a reason for hiding this comment

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

That and I still cling to statically typed notions and referring to a property that may or may not exist just feels wrong. ;-)

Understood. I'll try to help you get over it. 😀

@@ -128,11 +130,18 @@ if (Get-Module NuGet) {
}

function Write-Prompt($Object, $ForegroundColor, $BackgroundColor = -1) {
if ($GitPromptSettings.AnsiConsole) {
Copy link
Collaborator

@rkeithhill rkeithhill Mar 1, 2017

Choose a reason for hiding this comment

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

So in the PromptRenderer class I was working on, the idea was that it maintained an internal StringBuilder (for ANSI hosts) and it uses Write-Host for non-ANSI. Currently it takes an array of TextSpan objs (text, fg, bg in a class) but I was thinking it might be better to have a BeginWrite/Write/EndWrite set of methods where the Write method takes a TextSpan object.

Begin/EndWrite for non-ANSI would be a no-op but for ANSI BeginWrite would init StringBuilder and EndWrite would return contents of StringBuider. For non-ANSI, EndWrite would return the requisite single space to get rid of PS> showing up.

$e = [char]27 + "["
$f = Get-ForegroundVirtualTerminalSequence $ForegroundColor
$b = Get-BackgroundVirtualTerminalSequence $BackgroundColor
return "${f}${b}${Object}${e}0m"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to be able to handle terms that support 24-bit color like the upcoming Windows 10 CU drop. See my Twitter post on that - https://twitter.com/r_keith_hill/statuses/835556870697930753 This was something I was encapsulating in a Color class in my prototype.

Copy link
Owner Author

Choose a reason for hiding this comment

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

$ForegroundColor and $BackgroundColor are explicitly untyped, so in theory there's no reason Get-ForegroundVirtualTerminalSequence and Get-BackgroundVirtualTerminalSequence couldn't emit valid escape sequences given a variety of input types ("#rrggbb", System.Drawing.Color, etc).

(Not that a Color class couldn't have equivalent support.)

@rkeithhill
Copy link
Collaborator

Give me a day or so and I can commit my (very) prototype branch that uses v5 classes. It is missing stuff you have like the setting the console mode and conversion from console colors to ANSI seq indices. Maybe we can meld the two?

@lzybkr
Copy link
Collaborator

lzybkr commented Mar 1, 2017

I'm not sure about ArrayList w/ -join versus += on a string - my intuition says it won't matter - the strings are short anyway.

In general, use of StringBuilder would be a better optimization because you'll allocate fewer objects.

@dahlbyk
Copy link
Owner Author

dahlbyk commented Mar 1, 2017

A quick benchmark confirms that it really doesn't matter for small strings, but for larger strings StringBuilder is an order of magnitude faster than ArrayList, which is maybe 25% faster than string concatenation.

We may end up wanting a StringBuilder, but I'm content to leave it for now.

Give me a day or so and I can commit my (very) prototype branch that uses v5 classes. It is missing stuff you have like the setting the console mode and conversion from console colors to ANSI seq indices. Maybe we can meld the two?

I guess my question is if this branch is a reasonable enough starting point for people to 1) test it out, if they want, and 2) serve as a base for iterating on how we handle ANSI (your PromptRenderer or otherwise). Thoughts?

src/Utils.ps1 Outdated
function Get-BackgroundVirtualTerminalSequence($Color) {
return Get-VirtualTerminalSequence $Color 10
}

function dbg($Message, [Diagnostics.Stopwatch]$Stopwatch) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Eventually we might to put these into an Ansi.ps1 or AnsiUtils.ps1 file but that can wait.

@rkeithhill
Copy link
Collaborator

I guess my question is if this branch is a reasonable enough starting point

Good point. Go ahead and merge it. I have an Ubuntu VM I work on everyday for embedded Linux development work. I could take it for a spin there.

- Make Set-ConsoleMode a no-op on non-Windows
- Suppress a lint warning
- Remove unused functions
Semantics are still quite strange...
- If -Builder is specified (piped in), it will be returned, but only
  modified if -AnsiConsole is also specified
- If -AnsiConsole is specified, a string with ANSI color escape sequences
  is returned.
- Otherwise, an empty string is returned.
@dahlbyk
Copy link
Owner Author

dahlbyk commented Mar 2, 2017

Just rewrote the world:

  • ANSI stuff moved to separate file
  • Squashed a few fixup-y commits
  • Added a spike using StringBuilder just for fun... Write-Prompt semantics are now even more odd

@@ -0,0 +1,36 @@
# Color codes from https://msdn.microsoft.com/en-us/library/windows/desktop/mt638032(v=vs.85).aspx
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hehe, that is exactly the filename I'm using in my classes prototype branch. Did some work on that last night but not quite ready yet for evaluation.

@dahlbyk dahlbyk merged commit d42d7a7 into develop Mar 3, 2017
@dahlbyk dahlbyk deleted the SupportsVirtualTerminal branch March 3, 2017 06:14
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.

3 participants