-
-
Notifications
You must be signed in to change notification settings - Fork 802
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
Improved WindowTitle customization experience #541
Conversation
Needs more Pester tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also you made a comment about evaluating the WindowTitle scriptblock during prompt eval. It does seem a bit unusual that we change the WindowTitle in the Write-GitStatus function. Is the proposal to move the setting of the host WindowTitle to the built-in prompt?
Yes, let's think of it as a title-customization feature with Git support, rather than a Git title customization feature. To that end, our default WindowTitle
should do something reasonable if $GitStatus
is $null
.
BTW there was some code we used to determine WindowTitleSupported that I've thought was circumspect for a while. This gave me a chance to take a better (I hope) approach.
💯
src/GitPrompt.ps1
Outdated
$WindowTitleSupported = $false | ||
try { | ||
$global:PreviousWindowTitle = $Host.UI.RawUI.WindowTitle | ||
$newTitle = "$origTitle " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be "$PreviousWindowTitle "
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh! Yes it should be. Thanks. I'll fix it tonight.
src/PoshGitTypes.ps1
Outdated
[string]$DescribeStyle = '' | ||
[psobject]$EnableWindowTitle = 'posh~git ~ ' | ||
[string]$DescribeStyle = '' | ||
[bool]$EnableWindowTitle = $true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think we might want to keep
EnableWindowTitle
as a simple bool. If you disable the WindowTitle by setting it to $null, you lose the original definition.
I'm not too worried about this, since resetting is as simple as opening a new shell. Fewer settings seems better than many.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we have reduced the number of "top-level" settings a lot. :-) But I don't feel that strongly about this.
Add more Pester tests.
This last commit cleans up GitPrompt.ps1 real nice. No "extraneous" code in it now. BTW, in the $GitStatus is null case as well as the case where the user's WindowTitle scriptblock/string generates an error, we fall back to the $global:PreviousWindowTitle. |
Is that what we want, though? It seems more useful if this feature can be used to customize the window title at all times (e.g. showing |
I'm definitely open to making changes here. It always seemed a bit odd to me that the title swapped to the default/startup text in non-Git dirs. It would complicate the default scriptblock a bit but I think that is OK. I could see it being something like this: {
param($GitStatus, [bool]$IsAdmin)
"$(if ($IsAdmin) {'Administrator: '})$(if ($GitStatus) {"posh~git ~ $($GitStatus.RepoName) [$($GitStatus.Branch)] "})PowerShell $(if ([IntPtr]::Size -eq 8) {'x64'} else {'x86'}) $($PSVersionTable.PSVersion) ($PID)"
} That gives title bar text like this outside a git repo (as admin):
and inside a Git repo (not as admin):
When we display the posh-git info, perhaps we need another separator before
Thoughts? The |
I removed the x64/x86 bit for now. BTW you have to set BTW I have an updated CHANGELOG I'll submit in a different PR once this PR has been merged. |
Thanks. I think that is what folks will expect. I can't think of any real user scenarios where someone would want to set their title bar to |
Nice work! I'll see about back-porting for 0.7.2. |
v1 fix for #537
Needs more Pester tests. Also, I think we might want to keep
EnableWindowTitle
as a simple bool. If you disable the WindowTitle by setting it to $null, you lose the original definition.Also you made a comment about evaluating the WindowTitle scriptblock during prompt eval. It does seem a bit unusual that we change the WindowTitle in the Write-GitStatus function. Is the proposal to move the setting of the host WindowTitle to the built-in prompt?
BTW there was some code we used to determine WindowTitleSupported that I've thought was circumspect for a while. This gave me a chance to take a better (I hope) approach.