-
-
Notifications
You must be signed in to change notification settings - Fork 803
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
Do not update WindowTitle at all in prompt func if setting is $null #597
Conversation
We used to restore the previousWindowTitle but this had the effect of always updating the WindowTitle with a fixed string - whatever we captured in $PreviousWindowTitle. However, we still restore the original window title when the module is removed. But we only do this if the module is configured to supply the window title. Rename variable to make its intent clearer. Fixes #594
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.
Thanks for getting this change going! I think there's enough complexity here that we're better off moving it out of the psm1
. Thoughts?
src/posh-git.psm1
Outdated
@@ -18,16 +18,17 @@ if (!$Env:HOME) { $Env:HOME = "$Env:USERPROFILE" } | |||
$IsAdmin = Test-Administrator | |||
|
|||
# Probe $Host.UI.RawUI.WindowTitle to see if it can be set without errors | |||
$WindowTitleSupported = $false | |||
$HostSupportsSettingWindowTitle = $false |
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.
If we're going to let $GitPromptSettings.WindowTitle = $false
really suppress touching WindowTitle
, we might as well avoid this, too.
Thoughts on moving this whole block into a Test-WindowTitle
function (in Utils.ps1
)? I figure that function would close over a script variable that's $null
until we actually need to probe, at which point it would probe and remember for future checks.
The check below would then be:
if ($settings.WindowTitle -and (Test-WindowTitle)) {
# ...
This would short-circuit the probe until $settings.WindowTitle
isn't falsey.
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.
But when $settings.WindowTitle
isn't falsey, we would be doing this "writable" check during every prompt eval - effectively setting the title three times.
I think it is better to do this check once during module load but I'm fine with creating a function in Utils.ps1 so we'd have:
$IsAdmin = Test-Administrator
$IsWindowTitleWritable = Test-WindowTitleIsWritable
src/posh-git.psm1
Outdated
try { | ||
$global:PreviousWindowTitle = $Host.UI.RawUI.WindowTitle | ||
$newTitle = "${global:PreviousWindowTitle} " | ||
$global:PoshGitOrigWindowTitle = $Host.UI.RawUI.WindowTitle |
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.
Does this actually need to be global? Could it be script-scoped alongside $HostSupportsSettingWindowTitle
, with a corresponding Reset-WindowTitle
to use in OnRemove
?
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.
Yeah, it could be script scoped.
src/posh-git.psm1
Outdated
if (!$windowTitle) { | ||
if ($global:PreviousWindowTitle) { | ||
$Host.UI.RawUI.WindowTitle = $global:PreviousWindowTitle | ||
if ($HostSupportsSettingWindowTitle -and $settings.WindowTitle) { |
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.
While we're at it, should this whole block move into a Set-WindowTitle
function?
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.
Sure, but we'd need to export it from the module to make it callable from the prompt func. How about Reset-WindowTitle
- does it need to be exported or would we be the only caller in the OnRemove event handler.
03d5050
to
8c1c74a
Compare
I was pretty sure this wasn't the case, and my experiment to find out may have gone a bit far. Anyway, the new commits here seem to work as expected and the module is much cleaner. |
$OriginalWindowTitle = $null | ||
|
||
function Test-WindowTitleIsWriteable { | ||
if ($null -eq $HostSupportsSettingWindowTitle) { |
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.
👍
Looks good! My daughter just got back from her choir trip to Europe so didn't have time to tackle this last night. Thanks!! Go ahead and merge whenever you're ready. I have to say I'm a bit surprised the scriptblock we assign to the global func prompt can see that non-exported command. I was assuming the scriptblock closed over the variables it used. TIL that scriptblock seems to have access to module scope. :-) |
We used to restore the previousWindowTitle but this had the effect of
always updating the WindowTitle with a fixed string - whatever we
captured in $PreviousWindowTitle.
However, we still restore the original window title when the module is
removed. But we only do this if the module is configured to supply the
window title.
Rename variable to make its intent clearer.
Fixes #594