Skip to content

Fix RTF generation for Unicode characters#12586

Merged
4 commits merged intomicrosoft:mainfrom
ianjoneill:f-fix-rtf-unicode
Mar 1, 2022
Merged

Fix RTF generation for Unicode characters#12586
4 commits merged intomicrosoft:mainfrom
ianjoneill:f-fix-rtf-unicode

Conversation

@ianjoneill
Copy link
Contributor

Summary of the Pull Request

Fixes RTF generation for text with Unicode characters.

PR Checklist

  • Closes RTF formatted copy doesn't work for Unicode characters #12379
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated.
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Validation Steps Performed

Added some unit tests.

Ran the following in PowerShell and copied the emitted text into WordPad.

echo "This is some Ascii \ {}`nLow code units: á é í ó ú `u{2b81} `u{2b82}`nHigh code units: `u{a7b5} `u{a7b7}`nSurrogates: `u{1f366} `u{1f47e} `u{1f440}"

@ghost ghost added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Impact-Correctness It be wrong. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) labels Feb 26, 2022
@ianjoneill
Copy link
Contributor Author

Video of the validation:

UnicodeRtf.mp4

@lhecker
Copy link
Member

lhecker commented Mar 1, 2022

You can also remove the extra else if (codeUnit <= 32767) branch now that you use the bit-cast.
But this is really more like an "interesting nit" than anything. Since I approved it, you don't have to address my feedback, unless you feel the same way. 🙂

@ianjoneill
Copy link
Contributor Author

Good point - I didn't engage brain - done (again)

@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 1, 2022
@ghost
Copy link

ghost commented Mar 1, 2022

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 00113e3 into microsoft:main Mar 1, 2022
DHowett pushed a commit that referenced this pull request Mar 10, 2022
## Summary of the Pull Request
Fixes RTF generation for text with Unicode characters.

## PR Checklist
* [x] Closes #12379
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Validation Steps Performed
Added some unit tests.

Ran the following in PowerShell and copied the emitted text into WordPad.
```pwsh
echo "This is some Ascii \ {}`nLow code units: á é í ó ú `u{2b81} `u{2b82}`nHigh code units: `u{a7b5} `u{a7b7}`nSurrogates: `u{1f366} `u{1f47e} `u{1f440}"
```

(cherry picked from commit 00113e3)
DHowett pushed a commit that referenced this pull request Mar 10, 2022
## Summary of the Pull Request
Fixes RTF generation for text with Unicode characters.

## PR Checklist
* [x] Closes #12379
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Validation Steps Performed
Added some unit tests.

Ran the following in PowerShell and copied the emitted text into WordPad.
```pwsh
echo "This is some Ascii \ {}`nLow code units: á é í ó ú `u{2b81} `u{2b82}`nHigh code units: `u{a7b5} `u{a7b7}`nSurrogates: `u{1f366} `u{1f47e} `u{1f440}"
```

(cherry picked from commit 00113e3)
@ghost
Copy link

ghost commented Mar 25, 2022

🎉Windows Terminal v1.12.1073 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Mar 25, 2022

🎉Windows Terminal Preview v1.13.1073 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Impact-Correctness It be wrong. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RTF formatted copy doesn't work for Unicode characters

4 participants