Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

\r ends up unescaped in formatted copy on blank lines #4187

Closed
mcpiroman opened this issue Jan 12, 2020 · 3 comments · Fixed by #4190
Closed

\r ends up unescaped in formatted copy on blank lines #4187

mcpiroman opened this issue Jan 12, 2020 · 3 comments · Fixed by #4190
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@mcpiroman
Copy link
Contributor

Introduced by #2038

Select some text in terminal, copy and paste to e.g. Word. For every blank line in terminal there, are 2 blank lines in the output. One from <BR> and one from \r, which sneaks in unescaped.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jan 12, 2020
@ghost ghost added the In-PR This issue has a related PR label Jan 12, 2020
@zadjii-msft zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Jan 13, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 13, 2020
@zadjii-msft
Copy link
Member

Wow yea that's absolutely true. Good catch 😄

@zadjii-msft zadjii-msft added this to the Terminal v0.9 milestone Jan 13, 2020
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 13, 2020
@DHowett-MSFT
Copy link
Contributor

That's not great! Thanks.

@ghost ghost closed this as completed in #4190 Jan 14, 2020
ghost pushed a commit that referenced this issue Jan 14, 2020
## 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.
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jan 14, 2020
DHowett-MSFT pushed a commit that referenced this issue Jan 24, 2020
## 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)
@ghost
Copy link

ghost commented Jan 27, 2020

🎉This issue was addressed in #4190, which has now been successfully released as Windows Terminal Preview v0.8.10261.0.:tada:

Handy links:

This issue 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.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants