Skip to content

Fix handling of surrogate pairs in length calculation#4462

Open
totkeks wants to merge 4 commits intoPowerShell:masterfrom
totkeks:fix/surrogate-pairs
Open

Fix handling of surrogate pairs in length calculation#4462
totkeks wants to merge 4 commits intoPowerShell:masterfrom
totkeks:fix/surrogate-pairs

Conversation

@totkeks
Copy link
Copy Markdown

@totkeks totkeks commented Feb 7, 2025

PR Summary

Adds new conditions to handle surrogate pairs when calculating the buffer length of a string.

Fixes #3857.

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure you've added one or more new tests
  • Make sure you've tested these changes in terminals that PowerShell is commonly used in (i.e. conhost.exe, Windows Terminal, Visual Studio Code Integrated Terminal, etc.)
  • User-facing changes
    • Not Applicable
Microsoft Reviewers: Open in CodeFlow

@totkeks
Copy link
Copy Markdown
Author

totkeks commented Feb 8, 2025

@microsoft-github-policy-service agree

@springcomp
Copy link
Copy Markdown
Contributor

springcomp commented Apr 2, 2025

I think this pull request needs a unit-test to demonstrate the requirements, but a couple unit tests would be nice.

@andyleejordan
Copy link
Copy Markdown
Member

Oh I'm so sorry, we should've gotten to this sooner. Looks like the correct fix the identified bug to me.

I reviewed and they seem to correctly cover the reported (and solved) issues with glyphs / surrogate pairs.
@andyleejordan
Copy link
Copy Markdown
Member

Hey @totkeks I merged the main branch in (it's sadly still called "master") and added some Claude generated unit tests which looked correct to me. Can you take a look and confirm the assertions are covering the bugfix?

And short circuits faster if at end of the string.
Copy link
Copy Markdown
Member

@andyleejordan andyleejordan left a comment

Choose a reason for hiding this comment

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

Looks good to me. @daxian-dbw?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using an emoji as the prompt text parse error replacement causes glitchy text to remain in the text buffer

3 participants