Skip to content

Fix Minimal ProgressView to handle Activity that is longer than console width #15264

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
daxian-dbw merged 4 commits into from
May 17, 2021
Merged

Fix Minimal ProgressView to handle Activity that is longer than console width #15264

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
90d5099
Fix `Minimal` ProgressView to handle Activity that is longer than con…
SteveL-MSFT Apr 18, 2021
47d420f
handle case where console window width is extremely narrow
SteveL-MSFT Apr 20, 2021
6400648
address codefactor
SteveL-MSFT Apr 20, 2021
5dc7d8e
address Ilya's feedback to not modify original string
SteveL-MSFT Apr 20, 2021
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
32 changes: 26 additions & 6 deletions src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,14 +387,26 @@ internal static bool IsMinimalProgressRenderingEnabled()
maxWidth = PSStyle.Instance.Progress.MaxWidth;
}

// if the activity is really long, only use up to half the width
string activity;
if (Activity.Length > maxWidth / 2)
{
activity = Activity.Substring(0, maxWidth / 2) + PSObjectHelper.Ellipsis;
Copy link
Collaborator

@iSazonov iSazonov Apr 20, 2021

Choose a reason for hiding this comment

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

Looking on Archive expand example in #14414 I think we break the scenario.

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Apr 20, 2021

Choose a reason for hiding this comment

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

I just tried expand-archive with this branch and works fine with a narrow console window. As expected, activity and status are truncated.

Copy link
Collaborator

@iSazonov iSazonov Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I guess they are truncated too early while there is still a lot of free space on the right.

[1234567890          ]
[12345678        ]

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Apr 22, 2021

Choose a reason for hiding this comment

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

The bar area is also used for Status text which can change as progress is being written. So it's possible to fill up as much of Activity and Status as possible so whitespace is limited, it would also mean that the Status area would resize if the text changes which would make it hard to read.

Copy link
Collaborator

@iSazonov iSazonov Apr 22, 2021

Choose a reason for hiding this comment

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

I tried the build and I see the resizing scenario doesn't work at all. But this doesn't depend on the PR.
Run follow and use mouse to change windows width:

1..100 | % { Write-Progress -Activity "123456789012345678901234567890123456789012345678901234567890" -Status "S" -PercentComplete $_ -SecondsRemaining $_ ; Sleep 1}

Results:

  • after size changed (both left and right directions) the progress bar doesn't work
  • screen destroyed
  • after the command finished (or Ctrl-C pressed) screen buffer destroyed
  • instead of final Ellipsis I see single dot (direct output Ellipsis works on the console

(only last related to the PR)

Copy link
Member

@daxian-dbw daxian-dbw May 13, 2021

Choose a reason for hiding this comment

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

@iSazonov Can you please take another look and see if you have any outstanding concerns for the changes in this particular PR? For the points you brought up above in #15264 (comment), they look like out of the scope of this PR (correct me if I'm wrong), and maybe track them in a separate issue?

Copy link
Collaborator

@iSazonov iSazonov May 14, 2021

Choose a reason for hiding this comment

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

@daxian-dbw This removes the exception but I personally do not like the result of the display, although it is not fundamental. The fundamental thing is that the resizing scenario works horribly. But I don't see the point of opening a new issue since MSFT team doesn't have the resources to fix 3000 issues anyway.

Copy link
Member

@daxian-dbw daxian-dbw May 14, 2021

Choose a reason for hiding this comment

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

I opened #15407 to track the resizing issue.

instead of final Ellipsis I see single dot (direct output Ellipsis works on the console

image

@SteveL-MSFT It does look weird to have a single dot instead of Ellipsis when truncation happens. Should that be addressed in this PR or you think it should be a separate one?

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT May 17, 2021

Choose a reason for hiding this comment

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

I'll take a look at the other issues in a separate PR since this specific issue is affecting users with real world scenarios

Copy link
Member

@daxian-dbw daxian-dbw May 17, 2021

Choose a reason for hiding this comment

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

Sounds good. #15412 is opened to track the review comment.

}
else
{
activity = Activity;
}

// 4 is for the extra space and square brackets below and one extra space
int barWidth = maxWidth - Activity.Length - indentation - 4;
int barWidth = maxWidth - activity.Length - indentation - 4;

var sb = new StringBuilder();
int padding = maxWidth + PSStyle.Instance.Progress.Style.Length + PSStyle.Instance.Reverse.Length + PSStyle.Instance.ReverseOff.Length;
sb.Append(PSStyle.Instance.Reverse);

if (StatusDescription.Length > barWidth - secRemainLength)
int maxStatusLength = barWidth - secRemainLength - 1;
if (maxStatusLength > 0 && StatusDescription.Length > barWidth - secRemainLength)
{
sb.Append(StatusDescription.Substring(0, barWidth - secRemainLength - 1));
sb.Append(PSObjectHelper.Ellipsis);
Expand All @@ -404,18 +416,26 @@ internal static bool IsMinimalProgressRenderingEnabled()
sb.Append(StatusDescription);
}

sb.Append(string.Empty.PadRight(barWidth + PSStyle.Instance.Reverse.Length - sb.Length - secRemainLength));
int emptyPadLength = barWidth + PSStyle.Instance.Reverse.Length - sb.Length - secRemainLength;
if (emptyPadLength > 0)
{
sb.Append(string.Empty.PadRight(emptyPadLength));
}

sb.Append(secRemain);

if (PercentComplete > 0 && PercentComplete < 100)
if (PercentComplete > 0 && PercentComplete < 100 && barWidth > 0)
{
int barLength = PercentComplete * barWidth / 100;
if (barLength >= barWidth)
{
barLength = barWidth - 1;
}

sb.Insert(barLength + PSStyle.Instance.Reverse.Length, PSStyle.Instance.ReverseOff);
if (barLength < sb.Length)
{
sb.Insert(barLength + PSStyle.Instance.Reverse.Length, PSStyle.Instance.ReverseOff);
}
}
else
{
Expand All @@ -427,7 +447,7 @@ internal static bool IsMinimalProgressRenderingEnabled()
"{0}{1}{2} [{3}]{4}",
indent,
PSStyle.Instance.Progress.Style,
Activity,
activity,
sb.ToString(),
PSStyle.Instance.Reset)
.PadRight(padding));
Expand Down
4 changes: 4 additions & 0 deletions test/powershell/Modules/Microsoft.PowerShell.Utility/Write-Progress.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,8 @@ Describe "Write-Progress DRT Unit Tests" -Tags "CI" {
It "all params works" -Pending {
{ Write-Progress -Activity 'myactivity' -Status 'mystatus' -Id 1 -ParentId 2 -Completed:$false -current 'current' -sec 1 -percent 1 } | Should -Not -Throw
}

It 'Activity longer than console width works' {
{ Write-Progress -Activity ('a' * ([console]::WindowWidth + 1)) -Status ('b' * ([console]::WindowWidth + 1)) -Id 1 } | Should -Not -Throw
}
}