Fix progress rendering when using double-wide unicode #21303

SteveL-MSFT · 2024-03-04T23:29:49Z

PR Summary

The previous minimal progress rendering code was using the string length which counts characters and not the buffer cells required. This means that double-wide east asian characters counted as 1 when it should be counted as 2 so that it renders correctly in the console. The fix is to change all usage of getting the length to using a helper function that returns the number of buffer cells for a string.

Here's what it looked like before and after the change:

Recording.2024-03-04.152825.mp4

PR Context

Fix #21293

PR Checklist

pull-request-quantifier-deprecated · 2024-03-04T23:29:54Z

This PR has 18 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Small
Size       : +9 -9
Percentile : 7.2%

Total files changed: 1

Change summary by file extension:
.cs : +9 -9

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

daxian-dbw · 2024-03-05T00:33:12Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs


-            int emptyPadLength = barWidth + PSStyle.Instance.Reverse.Length - sb.Length - secRemainLength;
+            int emptyPadLength = barWidth - rawUI.LengthInBufferCells(sb.ToString()) - secRemainLength;


It looks that sb contains VT sequences, does LengthInBufferCells strip off VT sequences when counting cells?

LengthInBufferCells seems to only handle double-wide chars and nothing with VT. It seems that we shouldn't allow VT in the status description since it would screw up rendering anyways, so I'll add a change that will error if VT is in the status description.

It's good to prevent a user from using VT decorated Status and Activity.
However, here we already appended PSStyle.Instance.Reverse to the sb, so the returned result from rawUI.LengthInBufferCells(sb.ToString()) will be incorrect. You will need the original + PSStyle.Instance.Reverse.Length.

Or, maybe consider to not add the Reverse prefix until we are wrapping up constructing sb at the line 440 and 445, where ReverseOff VT sequence is added. That will make the code clearer and more readable.

PSStyle.Instance.Reverse should have a buffer cell length of zero since it shouldn't have any printable characters. So the length of it shouldn't be used at all. Removed it in the calculation of the length of the bar.

Exactly, it should have a buffer cell length of zero, but the returned value from rawUI.LengthInBufferCells(sb.ToString()) will include the length of PSStyle.Instance.Reverse, isn't it
I think that's why + PSStyle.Instance.Reverse.Length was there to offset it.

pull-request-quantifier-deprecated · 2024-03-06T21:24:38Z

This PR has 36 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Small
Size       : +21 -15
Percentile : 14.4%

Total files changed: 4

Change summary by file extension:
.cs : +16 -15
.resx : +3 -0
.ps1 : +2 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs

daxian-dbw · 2024-03-15T22:44:50Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs


-            int emptyPadLength = barWidth + PSStyle.Instance.Reverse.Length - sb.Length - secRemainLength;
+            int emptyPadLength = barWidth - rawUI.LengthInBufferCells(sb.ToString()) - secRemainLength;


It's good to prevent a user from using VT decorated Status and Activity.
However, here we already appended PSStyle.Instance.Reverse to the sb, so the returned result from rawUI.LengthInBufferCells(sb.ToString()) will be incorrect. You will need the original + PSStyle.Instance.Reverse.Length.

Or, maybe consider to not add the Reverse prefix until we are wrapping up constructing sb at the line 440 and 445, where ReverseOff VT sequence is added. That will make the code clearer and more readable.

src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs

daxian-dbw · 2024-03-15T22:49:10Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs

@@ -432,7 +435,7 @@ internal static bool IsMinimalProgressRenderingEnabled()
                    barLength = barWidth - 1;
                }

-                if (barLength < sb.Length)
+                if (barLength < rawUI.LengthInBufferCells(sb.ToString()))


Same here, rawUI.LengthInBufferCells(sb.ToString()) will return the incorrect value as it counts the Reverse VT sequences in. It would be the best if you can insert the Reverse prefix when adding the ReverseOff sequences.

This one doesn't seem to be resolved.

pull-request-quantifier-deprecated · 2024-03-15T23:18:16Z

This PR has 38 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Small
Size       : +22 -16
Percentile : 15.2%

Total files changed: 4

Change summary by file extension:
.cs : +17 -16
.resx : +3 -0
.ps1 : +2 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

pull-request-quantifier-deprecated · 2024-03-15T23:59:37Z

This PR has 40 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Small
Size       : +23 -17
Percentile : 16%

Total files changed: 4

Change summary by file extension:
.cs : +18 -17
.resx : +3 -0
.ps1 : +2 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

Co-authored-by: Dongbo Wang <dongbow@microsoft.com>

pull-request-quantifier-deprecated · 2024-03-26T16:22:59Z

This PR has 42 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Small
Size       : +24 -18
Percentile : 16.8%

Total files changed: 4

Change summary by file extension:
.cs : +19 -18
.resx : +3 -0
.ps1 : +2 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

daxian-dbw · 2024-04-02T00:45:59Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs

-            if (PercentComplete >= 0 && PercentComplete < 100 && barWidth > 0)
+            if (PercentComplete >= 0 && barWidth > 0)


I don't think PercentComplete < 100 should be removed. This check excluded the case where PercentComplete == 100, and maybe that was on purpose.

daxian-dbw · 2024-04-02T00:49:01Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs

                if (barLength >= barWidth)
                {
                    barLength = barWidth - 1;
                }


My comment https://github.com/PowerShell/PowerShell/pull/21303/files#r1526931375 actually meant this if-block.

I found this if-block check seems to never be true. PercentComplete >= 0 && PercentComplete < 100, so PercentComplete * barWidth / 100 will always be less than barWidth. Maybe consider remove this if-block?

daxian-dbw · 2024-04-02T00:55:06Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs


-            int emptyPadLength = barWidth + PSStyle.Instance.Reverse.Length - sb.Length - secRemainLength;
+            int emptyPadLength = barWidth - rawUI.LengthInBufferCells(sb.ToString()) - secRemainLength;


Exactly, it should have a buffer cell length of zero, but the returned value from rawUI.LengthInBufferCells(sb.ToString()) will include the length of PSStyle.Instance.Reverse, isn't it
I think that's why + PSStyle.Instance.Reverse.Length was there to offset it.

daxian-dbw · 2024-04-02T00:56:12Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs

@@ -432,7 +435,7 @@ internal static bool IsMinimalProgressRenderingEnabled()
                    barLength = barWidth - 1;
                }

-                if (barLength < sb.Length)
+                if (barLength < rawUI.LengthInBufferCells(sb.ToString()))


This one doesn't seem to be resolved.

Fix progress rendering when using double-wide unicode

16e8b24

SteveL-MSFT requested review from daxian-dbw and anmenaga as code owners March 4, 2024 23:29

pull-request-quantifier-deprecated bot added the Extra Small label Mar 4, 2024

daxian-dbw reviewed Mar 5, 2024

View reviewed changes

address Dongbo's feedback not allowing VT for activity or status

0513753

microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Mar 14, 2024

daxian-dbw requested changes Mar 15, 2024

View reviewed changes

microsoft-github-policy-service bot added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels Mar 15, 2024

remove adding length of reverse VT

f988656

microsoft-github-policy-service bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Mar 15, 2024

remove unnecessary check

1b53c1a

microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Mar 23, 2024

Update src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs

f3eb355

Co-authored-by: Dongbo Wang <dongbow@microsoft.com>

SteveL-MSFT closed this Apr 1, 2024

SteveL-MSFT reopened this Apr 1, 2024

microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Apr 1, 2024

daxian-dbw requested changes Apr 2, 2024

View reviewed changes

microsoft-github-policy-service bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 2, 2024

microsoft-github-policy-service bot added the Stale label Apr 17, 2024

microsoft-github-policy-service bot closed this Apr 27, 2024

microsoft-github-policy-service bot removed the Stale label Apr 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix progress rendering when using double-wide unicode #21303

Fix progress rendering when using double-wide unicode #21303

SteveL-MSFT commented Mar 4, 2024 •

edited

pull-request-quantifier-deprecated bot commented Mar 4, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

daxian-dbw Mar 5, 2024

SteveL-MSFT Mar 6, 2024

daxian-dbw Mar 15, 2024

SteveL-MSFT Mar 15, 2024

daxian-dbw Apr 2, 2024

pull-request-quantifier-deprecated bot commented Mar 6, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

daxian-dbw Mar 15, 2024

daxian-dbw Mar 15, 2024

daxian-dbw Apr 2, 2024

pull-request-quantifier-deprecated bot commented Mar 15, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

pull-request-quantifier-deprecated bot commented Mar 15, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

pull-request-quantifier-deprecated bot commented Mar 26, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

daxian-dbw Apr 2, 2024 •

edited

daxian-dbw Apr 2, 2024

daxian-dbw Apr 2, 2024

daxian-dbw Apr 2, 2024


		int emptyPadLength = barWidth + PSStyle.Instance.Reverse.Length - sb.Length - secRemainLength;
		int emptyPadLength = barWidth - rawUI.LengthInBufferCells(sb.ToString()) - secRemainLength;

		if (PercentComplete >= 0 && PercentComplete < 100 && barWidth > 0)
		if (PercentComplete >= 0 && barWidth > 0)

Fix progress rendering when using double-wide unicode #21303

Fix progress rendering when using double-wide unicode #21303

Conversation

SteveL-MSFT commented Mar 4, 2024 • edited

PR Summary

PR Context

PR Checklist

pull-request-quantifier-deprecated bot commented Mar 4, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pull-request-quantifier-deprecated bot commented Mar 6, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pull-request-quantifier-deprecated bot commented Mar 15, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

pull-request-quantifier-deprecated bot commented Mar 15, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

pull-request-quantifier-deprecated bot commented Mar 26, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

daxian-dbw Apr 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveL-MSFT commented Mar 4, 2024 •

edited

daxian-dbw Apr 2, 2024 •

edited