-
-
Notifications
You must be signed in to change notification settings - Fork 807
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
Rename tabs to match the repo and branch when in powershell ise #605
base: master
Are you sure you want to change the base?
Conversation
I've not heard of using ISE for the MDI…interesting. I am certainly not opposed to making like better for folks using ISE, especially since it's cheap to skip for non-ISE sessions. What's the relationship between ISE tabs and I have some thoughts in general, but for starters can you move this new code into |
In addition to moving to |
Hi guys, Thanks for the feedback. Maybe I should make another CmdLet called Set-TabTitle and call it within Set-WindowTitle is called. With the GitPromptSettings should it default to on or off? I would say this functionality would be desired by defualt for MDI IDEs. I'll have a crack at the changes when I get some time and you can review. I think the tabs and title are managed seperately. I have noticed some behaviour that might freak people out a bit. If you have 2 tabs pointing to the same repo and you change branch in one it'll obviously change the branch in the other tab but that won't be reflected in the ide until you run another git command. It makes sense when you know how git works but though I'd point it out. |
8e359f9
to
fc5dec1
Compare
Changes to be committed: modified: src/GitPrompt.ps1 remove the old code modified: src/PoshGitTypes.ps1 add config entry to disable tab labelling modified: src/WindowTitle.ps1 added functions to label tabs modified: src/posh-git.psm1 added call to tab labelling code
src/PoshGitTypes.ps1
Outdated
@@ -273,6 +273,8 @@ class PoshGitPromptSettings { | |||
[string]$DescribeStyle = '' | |||
[psobject]$WindowTitle = {param($GitStatus, [bool]$IsAdmin) "$(if ($IsAdmin) {'Admin: '})$(if ($GitStatus) {"$($GitStatus.RepoName) [$($GitStatus.Branch)]"} else {Get-PromptPath}) ~ PowerShell $($PSVersionTable.PSVersion) $([IntPtr]::Size * 8)-bit ($PID)"} | |||
|
|||
[bool]$TabTitle = $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.
I think we should consider following the pattern that $WindowTitle
uses. By specifying a scriptblock, users will be able to easily customize the text that gets put in the tab title. To simplify the scriptblock, you can use functions. Perhaps a Get-UniqueTabTitle
to handle the logic for appending a number to the title to make it unique. Perhaps it would be something like this:
[psobject]$TabTitle = {param($GitStatus, [bool]$IsAdmin) "if ($GitStatus) { Get-UniqueTabTitle "$($GitStatus.RepoName) [$($GitStatus.Branch)]" }}
src/WindowTitle.ps1
Outdated
} | ||
|
||
# If the user is running Powershell ISE then name the tab | ||
if($psISE -and $GitStatus){ |
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.
The code style for this project uses a single space after keywords like if
, foreach
, etc and a single space before an opening {
.
src/posh-git.psm1
Outdated
@@ -78,6 +78,7 @@ $GitPromptScriptBlock = { | |||
|
|||
# This has to be *after* the call to Write-VcsStatus, which populates $global:GitStatus | |||
Set-WindowTitle $global:GitStatus $IsAdmin | |||
Set-TabTitle $global:GitStatus |
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.
Even if folks don't use $IsAdmin in their customized scriptblock, I think we should pass it. No harm in having this extra bit of info injected into the scriptblock.
src/WindowTitle.ps1
Outdated
@@ -58,3 +58,45 @@ function Set-WindowTitle { | |||
} | |||
} | |||
} | |||
|
|||
function Set-TabTitle { |
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 change the setting to be a scriptblock, then this function becomes very similar to the Set-WindowTitle function above except that where it sets the title, it uses $psise.CurrentPowerShellTab.DisplayName = …
src/WindowTitle.ps1
Outdated
} | ||
} | ||
|
||
function Get-TabTitle { |
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.
I think we change this to Get-UniqueTableTitle
with a single parameter of $Title
and this function simply ensures that $Title is unique and if not, it will append numbers until a unique title is found.
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.
👍
I think you can simplify a bit, too:
function Get-UniqueTabDisplayName ($Name, $Format="{0} {1}") {
$tabNumber = 1
$names = $psISE.PowerShellTabs | Select-Object -ExpandProperty DisplayName
$newName = $Name
while ($names -Contains $newName) {
$newName = $Format -f $Name,$tabNumber++
}
return $newName
}
Note I dropped the parentheses to align with ISE's default numbering scheme.
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.
I think I tried it like this initially but noticed an edge case where if you open a few tabs to the same repo i.e. posh-git [master], posh-git [master] (1), posh-git [master] (2) but then you close tab posh-git [master] (1) and open a new tab at the same repo it tries to create a new tab called posh-git [master] (2) and fails. Hence the logic to get the highest number that has been assigned so far.
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.
I believe this correctly fills in the gaps:
That said, this does open the possibility of changing branches/tabs such that you get:
[ "posh-git [master]", "posh-git [master] (1)", "posh-git [master] (2)"]
[ "posh-git [master]", "posh-git [george]", "posh-git [master] (2)"]
[ "posh-git [master]", "posh-git [george]", "posh-git [george] (1)"]
[ "posh-git [george] (2)", "posh-git [george]", "posh-git [george] (1)"]
As opposed to:
[ "posh-git [master]", "posh-git [master] (1)", "posh-git [master] (2)"]
[ "posh-git [master]", "posh-git [george] (1)", "posh-git [master] (2)"]
[ "posh-git [master]", "posh-git [george] (1)", "posh-git [george] (2)"]
[ "posh-git [george]", "posh-git [george] (1)", "posh-git [george] (2)"]
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.
You're right this is much better and simpler. I hadn't looked at your looping logic properly I thought it was simply counting the number of tabs with the same name and using that count to name the tab. Wrist slapped.
Personally, I don't really think it matters what the number is as long as it's unique and doesn't change for a given tab.
src/WindowTitle.ps1
Outdated
$tabName= "$tabName ($tabCount)" | ||
} | ||
return $tabName | ||
} |
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.
Please add a final newline to this file.
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.
@rkeithhill's feedback is all 💯
src/WindowTitle.ps1
Outdated
} | ||
} | ||
|
||
function Get-TabTitle { |
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.
👍
I think you can simplify a bit, too:
function Get-UniqueTabDisplayName ($Name, $Format="{0} {1}") {
$tabNumber = 1
$names = $psISE.PowerShellTabs | Select-Object -ExpandProperty DisplayName
$newName = $Name
while ($names -Contains $newName) {
$newName = $Format -f $Name,$tabNumber++
}
return $newName
}
Note I dropped the parentheses to align with ISE's default numbering scheme.
Thanks for all the feedback guys. I'll try to get it all cleaned up. I don't use powershell that much is there something like FxCop for powershell so I can make sure I conform to your coding standards? |
@georgecheyne If you use Visual Studio Code and install the PowerShell extension, you can select your code and press |
modified: src/PoshGitTypes.ps1 Made the TabTitle setting more like WindowTitle so it can be customised by users modified: src/WindowTitle.ps1 Removed tab handling code from here new file: src/TabTitle.ps1 moved all Tab related functionality here added code to reset tab names when leaving a repo space also will reset when unloading the posh-git module implemented simpler unique tab naming algorithm as suggested modified: src/posh-git.psm1 Added references to the new TabTitle script file and it's functions
Hi guys, I've refactored it to make it much more like the window title implementation. I think I've got the coding standards OK but any feedback welcome. Cheers, George |
Changes look good - I like how you've tried to make this not specific to ISE. However, I noticed my suggested
I also noticed this, too. If we're going to update the current tab, we might as well try to update other tabs for the current repo as well. I don't know exactly how it would work, but I wonder if we could use Thoughts? |
I'm not sure if you want to embed IDE specific code in here at all. If you do this also probably isn't the best place.
However I find this useful. One of the main reasons I use posh-git is because I can use it in Powershell ISE and use the MDI to have a few repos available at once.
I thought I'd share the code and see if you like it/want to incorporate it in some way.
Cheers,
George