Skip to content

Fix formatting of tables where headers span multiple rows #6504

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.

Sign up for GitHub

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

Merged
iSazonov merged 4 commits into from
Apr 3, 2018
Merged

Fix formatting of tables where headers span multiple rows #6504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ed5ae7d
In cases where the header spans multiple rows, need to correctly calc…
SteveL-MSFT Mar 27, 2018
34a3fdf
address Ilya's feedback
SteveL-MSFT Mar 28, 2018
d198571
added new ref assemblies to Files.wxs
SteveL-MSFT Mar 28, 2018
35e24af
[package] force package tests
TravisEz13 Apr 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion assets/files.wxs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>
<Wix xmlns="http://schemas.microsoft.com/wix/2006/wi">
<?define ProductDirectoryName = "$(env.ProductDirectoryName)" ?>
<?define FileArchitecture = "$(env.FileArchitecture)" ?>
Expand All @@ -7,6 +7,12 @@
<Component Id="cmp3CC027D3F160412C9F0044EBED3115DD" Guid="{8D7CAA67-8F28-422C-85FB-BDE04902E64F}">
<File Id="filFD2EF6BC74AF459D1BB52CA1E8C6E33B" KeyPath="yes" Source="$(env.ProductSourcePath)\NJsonSchema.dll" />
</Component>
<Component Id="cmp85032A09199C30E63F9F2CF55B6FA9B9" Guid="{D3414CD0-7366-4008-A406-DC9E69224F42}">
<File Id="fil85032A09199C30E63F9F2CF55B6FA9B9" KeyPath="yes" Source="$(env.ProductSourcePath)\System.Memory.dll" />
</Component>
<Component Id="cmp12FFA2A71C4F95630792652F192DAAB3" Guid="{9F6FF303-33C5-4C51-9092-B03BC1593AEF}">
<File Id="fil12FFA2A71C4F95630792652F192DAAB3" KeyPath="yes" Source="$(env.ProductSourcePath)\System.Runtime.CompilerServices.Unsafe.dll" />
</Component>
<Component Id="cmp3B130879A26D2E954251BB81E8948069" Guid="{CD268615-4603-4A5F-B126-340ADF2EDDD5}">
<File Id="filAACDEEE28FEA076C73D082A0AD21B8E0" KeyPath="yes" Source="$(env.ProductSourcePath)\Microsoft.CSharp.dll" />
</Component>
Expand Down Expand Up @@ -1822,6 +1828,8 @@
<Fragment>
<ComponentGroup Id="$(var.ProductDirectoryName)">
<ComponentRef Id="cmp3CC027D3F160412C9F0044EBED3115DD" />
<ComponentRef Id="cmp85032A09199C30E63F9F2CF55B6FA9B9" />
<ComponentRef Id="cmp12FFA2A71C4F95630792652F192DAAB3" />
<ComponentRef Id="cmp3B130879A26D2E954251BB81E8948069" />
<ComponentRef Id="cmp02ABBE4A3EDBEBFD05DC17A009A2B79D" />
<ComponentRef Id="cmpAA10498DF244C013CB5043C62E3AA83A" />
Expand Down
72 changes: 61 additions & 11 deletions src/System.Management.Automation/FormatAndOutput/common/TableWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ private string[] GenerateTableRow(string[] values, int[] alignment, DisplayCells
// pad with a blank (or any character that ALWAYS maps to a single screen cell
if (k > 0)
{
// skipping the first ones, add a separator for catenation
// skipping the first ones, add a separator for concatenation
for (int j = 0; j < scArray[k].Count; j++)
{
scArray[k][j] = StringUtil.Padding(ScreenInfo.separatorCharacterCount) + scArray[k][j];
Expand Down Expand Up @@ -289,22 +289,64 @@ private string[] GenerateTableRow(string[] values, int[] alignment, DisplayCells
screenRows = scArray[k].Count;
}

// column headers can span multiple rows if the width of the column is shorter than the header text like:
//
// Long Header2 Head
// Head er3
// er
// ---- ------- ----
// 1 2 3
//
// To ensure we don't add whitespace to the end, we need to determine the last column in each row with content

System.Span<int> lastColWithContent = stackalloc int[screenRows];
for (int row = 0; row < screenRows; row++)
{
for (int col = scArray.Length - 1; col > 0; col--)
{
int colWidth = _si.columnInfo[validColumnArray[col]].width;
int headerLength = values[col].Length;
if (headerLength / colWidth >= row && headerLength % colWidth > 0)
{
lastColWithContent[row] = col;
break;
}
}
}

// add padding for the columns that are shorter
for (int col = 0; col < scArray.Length-1; col++)
for (int col = 0; col < scArray.Length; col++)
{
int paddingBlanks = _si.columnInfo[validColumnArray[col]].width;
if (col > 0)
paddingBlanks += ScreenInfo.separatorCharacterCount;
else
int paddingBlanks = 0;

// don't pad if last column
if (col < scArray.Length - 1)
{
paddingBlanks += _startColumn;
paddingBlanks = _si.columnInfo[validColumnArray[col]].width;
if (col > 0)
{
paddingBlanks += ScreenInfo.separatorCharacterCount;
}
else
{
paddingBlanks += _startColumn;
}
}

int paddingEntries = screenRows - scArray[col].Count;
if (paddingEntries > 0)
{
for (int j = 0; j < paddingEntries; j++)
for (int row = screenRows - paddingEntries; row < screenRows; row++)
{
scArray[col].Add(StringUtil.Padding(paddingBlanks));
// if the column is beyond the last column with content, just use empty string
if (col > lastColWithContent[row])
{
scArray[col].Add("");
}
else
{
scArray[col].Add(StringUtil.Padding(paddingBlanks));
}
}
}
}
Expand All @@ -314,10 +356,18 @@ private string[] GenerateTableRow(string[] values, int[] alignment, DisplayCells
for (int row = 0; row < rows.Length; row++)
{
StringBuilder sb = new StringBuilder();
// for a give row, walk the columns
// for a given row, walk the columns
for (int col = 0; col < scArray.Length; col++)
Copy link
Collaborator

@iSazonov iSazonov Mar 29, 2018

Choose a reason for hiding this comment

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

Not for th PR - we have several similar repetitive cycles which we could replace one.

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Mar 29, 2018

Choose a reason for hiding this comment

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

Please open an issue and mark as code cleanup. Thanks!

Copy link
Collaborator

@iSazonov iSazonov Mar 29, 2018

Choose a reason for hiding this comment

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

Done #6526.

{
sb.Append(scArray[col][row]);
// if the column is the last column with content, we need to trim trailing whitespace
if (col == lastColWithContent[row])
{
sb.Append(scArray[col][row].TrimEnd());
}
else
{
sb.Append(scArray[col][row]);
}
}
rows[row] = sb.ToString();
}
Expand Down
2 changes: 2 additions & 0 deletions src/System.Management.Automation/System.Management.Automation.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
<Description>PowerShell Core's System.Management.Automation project</Description>
<NoWarn>$(NoWarn);CS1570;CS1734</NoWarn>
<AssemblyName>System.Management.Automation</AssemblyName>
<LangVersion>7.2</LangVersion>
Copy link
Collaborator

@iSazonov iSazonov Mar 28, 2018

Choose a reason for hiding this comment

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

Do we really need this? Currently we use default and seems the default is 7.2.

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Mar 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Span was not found without this and from what I could find, currently we need to opt into 7.2 features. Perhaps it will be default once dotnetcore2.1 GAs.

Copy link
Collaborator

@iSazonov iSazonov Mar 30, 2018

Choose a reason for hiding this comment

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

@SteveL-MSFT I found in docs that we could use Latest - if this works maybe we expand this on all proj files?

/cc @daxian-dbw

Copy link
Member

@daxian-dbw daxian-dbw Mar 30, 2018

Choose a reason for hiding this comment

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

The <LanguageVersion> tag should be put in PowerShell.Common.props, I think.

Copy link
Collaborator

@iSazonov iSazonov Apr 3, 2018

Choose a reason for hiding this comment

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

I opened the tracking Issue #6547.

</PropertyGroup>

<ItemGroup>
Expand All @@ -13,6 +14,7 @@
<!-- the following package(s) are from https://github.com/dotnet/corefx -->
<PackageReference Include="Microsoft.Win32.Registry.AccessControl" Version="4.4.0" />
<PackageReference Include="System.IO.FileSystem.AccessControl" Version="4.4.0" />
<PackageReference Include="System.Memory" Version="4.5.0-*"/>
<PackageReference Include="System.Security.AccessControl" Version="4.4.1" />
<PackageReference Include="System.Security.Cryptography.Pkcs" Version="4.4.0" />
<PackageReference Include="System.Security.Permissions" Version="4.4.1" />
Expand Down
137 changes: 137 additions & 0 deletions test/powershell/Modules/Microsoft.PowerShell.Utility/Format-Table.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,4 +339,141 @@ Left Center Right
$output = $ps.Invoke()
$output.Replace("`n","").Replace("`r","") | Should -BeExactly $expected
}

It "Format-Table should correctly render headers that span multiple rows: <variation>" -TestCases @(
@{ view = "Default"; widths = 4,7,4; variation = "4 row, 1 row, 2 row"; expectedTable = @"

Long Header2 Head
Long er3
Head
er
---- ------- ----
Copy link
Collaborator

@iSazonov iSazonov Mar 28, 2018

Choose a reason for hiding this comment

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

I believe we should have a test for edge case - one column.

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Mar 28, 2018

Choose a reason for hiding this comment

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

Will add

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Mar 28, 2018

Choose a reason for hiding this comment

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

I'm also going to turn the ps1xml into a template since most of it is the same

1 2 3



"@ },
@{ view = "Default"; widths = 4,4,7; variation = "4 row, 2 row, 1 row"; expectedTable = @"

Long Head Header3
Long er2
Head
er
---- ---- -------
1 2 3



"@ },
@{ view = "Default"; widths = 14,7,3; variation = "1 row, 1 row, 3 row"; expectedTable = @"

LongLongHeader Header2 Hea
der
3
-------------- ------- ---
1 2 3



"@ },
@{ view = "One"; widths = 4,1,1; variation = "1 column"; expectedTable = @"

Long
Long
Head
er
----
1



"@ }
) {
param($view, $widths, $expectedTable)
$ps1xml = @"
Copy link
Member

@TravisEz13 TravisEz13 Mar 29, 2018

Choose a reason for hiding this comment

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

NIT: This is rather hard to read with large here strings in the middle of the code.

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Mar 29, 2018

Choose a reason for hiding this comment

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

In VSCode, the coloring makes it much easier to read. Collapsing the XML will make it harder to read the XML.

Copy link
Collaborator

@iSazonov iSazonov Mar 29, 2018

Choose a reason for hiding this comment

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

I agree too that it is more editors and formatting plugins issue and it strongly depends on personal preferences. We could be more happy with the support of YAML.

<Configuration>
<ViewDefinitions>
<View>
<Name>Default</Name>
<ViewSelectedBy>
<TypeName>Test.Format</TypeName>
</ViewSelectedBy>
<TableControl>
<TableHeaders>
<TableColumnHeader>
<Label>LongLongHeader</Label>
<Width>{0}</Width>
</TableColumnHeader>
<TableColumnHeader>
<Label>Header2</Label>
<Width>{1}</Width>
</TableColumnHeader>
<TableColumnHeader>
<Label>Header3</Label>
<Width>{2}</Width>
</TableColumnHeader>
</TableHeaders>
<TableRowEntries>
<TableRowEntry>
<TableColumnItems>
<TableColumnItem>
<PropertyName>First</PropertyName>
</TableColumnItem>
<TableColumnItem>
<PropertyName>Second</PropertyName>
</TableColumnItem>
<TableColumnItem>
<PropertyName>Third</PropertyName>
</TableColumnItem>
</TableColumnItems>
</TableRowEntry>
</TableRowEntries>
</TableControl>
</View>
<View>
<Name>One</Name>
<ViewSelectedBy>
<TypeName>Test.Format</TypeName>
</ViewSelectedBy>
<TableControl>
<TableHeaders>
<TableColumnHeader>
<Label>LongLongHeader</Label>
<Width>{0}</Width>
</TableColumnHeader>
</TableHeaders>
<TableRowEntries>
<TableRowEntry>
<TableColumnItems>
<TableColumnItem>
<PropertyName>First</PropertyName>
</TableColumnItem>
</TableColumnItems>
</TableRowEntry>
</TableRowEntries>
</TableControl>
</View>
</ViewDefinitions>
</Configuration>
"@
$ps1xml = $ps1xml.Replace("{0}", $widths[0]).Replace("{1}", $widths[1]).Replace("{2}", $widths[2])
$ps1xmlPath = Join-Path -Path $TestDrive -ChildPath "test.format.ps1xml"
Set-Content -Path $ps1xmlPath -Value $ps1xml
Copy link
Collaborator

@iSazonov iSazonov Mar 29, 2018

Choose a reason for hiding this comment

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

Maybe add -Force?

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Mar 29, 2018

Choose a reason for hiding this comment

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

Isn't needed here, -Force is only needed if the file is read-only

# run in own runspace so not affect global sessionstate
$ps = [powershell]::Create()
$ps.AddScript( {
param($ps1xmlPath, $view)
Update-FormatData -AppendPath $ps1xmlPath
$a = [PSCustomObject]@{First=1;Second=2;Third=3}
$a.PSObject.TypeNames.Insert(0,"Test.Format")
$a | Format-Table -View $view | Out-String
} ).AddArgument($ps1xmlPath).AddArgument($view) | Out-Null
$output = $ps.Invoke()
foreach ($e in $ps.Streams.Error)
{
Write-Verbose $e.ToString() -Verbose
}
$ps.HadErrors | Should -BeFalse
$output.Replace("`r","").Replace(" ",".").Replace("`n","``") | Should -BeExactly $expectedTable.Replace("`r","").Replace(" ",".").Replace("`n","``")
}
}