Fix Minimal ProgressView to handle Activity that is longer than console width
#15264
Conversation
| string activity; | ||
| if (Activity.Length > maxWidth / 2) | ||
| { | ||
| activity = Activity.Substring(0, maxWidth / 2) + PSObjectHelper.Ellipsis; |
There was a problem hiding this comment.
Looking on Archive expand example in #14414 I think we break the scenario.
There was a problem hiding this comment.
I just tried expand-archive with this branch and works fine with a narrow console window. As expected, activity and status are truncated.
There was a problem hiding this comment.
I guess they are truncated too early while there is still a lot of free space on the right.
[1234567890 ]
[12345678 ]
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I opened #15407 to track the resizing issue.
instead of final Ellipsis I see single dot (direct output Ellipsis works on the console
@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?
There was a problem hiding this comment.
I'll take a look at the other issues in a separate PR since this specific issue is affecting users with real world scenarios
There was a problem hiding this comment.
Sounds good. #15412 is opened to track the review comment.
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
Codefactor issue is "won't fix" |
|
🎉 Handy links: |
PR Summary
The current code handles the case where
Statusis too long, but doesn't handle the case whereActivityis longer than console width so that when calculating the space for status, it becomes a negative value. Fix is to check ifActivityis longer than the console width and truncate it so that it doesn't take up more than half the console width allowing space for theStatus.PR Context
Fix #15251
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).