Fix redundant CR in formatted text copy #4190
Conversation
mcpiroman
changed the title
CR in formatted text copy
|
@msftbot make sure @carlos-zamora signs off on this |
ghost
added
the
AutoMerge
|
Hello @zadjii-msft! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
|
Reran failed jobs. @DHowett-MSFT, it's the privileged instruction thing again. Maybe there's some memory corruption in these tests causing it to jump occasionally to a privileged instruction? |
|
I'm not sure how the PR merge magic works but now that #4202 is merged, I'm re-running the x86 build right now hoping that the updated merge with master will pass the test. If not, we might need to merge master into this branch first before it'll be happy. |
ghost
merged commit 
## Summary of the Pull Request When `GenHTML` or `GenRTF` encountered an empty line, they assumed that `CR` is the last character of the row and wrote it, even though in general `CR` and `LF` just break the line and instead of them either `<BR>` in HTML or `\line` in RTF is written. Don't know how I missed that in #2038. Another question is whether the `TextAndColor` structure which these methods receive and which is generated by `TextBuffer::GetTextForClipboard` should really contain `\r\n` at the end of each row. I think it'd be cleaner if it didn't esp. that afaik these last 2 characters don't have associated valid color information. ## References <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [X] Closes #4187 * [X] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed - there aren't any related tests, right? * [ ] Requires documentation to be 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: #4147 <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed Copied various terminal states and verified the generated HTML. (cherry picked from commit 1ca2912)
|
🎉 Handy links: |
ghost
mentioned this pull request
|
🎉 Once again, thanks for the contribution! This pull request was included in a set of conhost changes that was just |
Summary of the Pull Request
When
GenHTMLorGenRTFencountered an empty line, they assumed thatCRis the last character of the row and wrote it, even though in generalCRandLFjust break the line and instead of them either<BR>in HTML or\linein RTF is written. Don't know how I missed that in #2038.Another question is whether the
TextAndColorstructure which these methods receive and which is generated byTextBuffer::GetTextForClipboardshould really contain\r\nat the end of each row. I think it'd be cleaner if it didn't esp. that afaik these last 2 characters don't have associated valid color information.References
PR Checklist
Validation Steps Performed
Copied various terminal states and verified the generated HTML.