From 1ca29128d4c2ef677d360f78f6ff87bc4eac0fcb Mon Sep 17 00:00:00 2001 From: mcpiroman <38111589+mcpiroman@users.noreply.github.com> Date: Tue, 14 Jan 2020 18:07:06 +0100 Subject: [PATCH] Fix redundant CR in formatted text copy (#4190) ## 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 `
` 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 ## 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 ## Validation Steps Performed Copied various terminal states and verified the generated HTML. --- src/buffer/out/textBuffer.cpp | 88 ++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 42 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index a4eaa73140..70973fd925 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1243,26 +1243,6 @@ std::string TextBuffer::GenHTML(const TextAndColor& rows, const int fontHeightPo for (size_t col = 0; col < rows.text.at(row).length(); col++) { - // do not include \r nor \n as they don't have attributes - // and are not HTML friendly. For line break use '
' instead. - const bool isLastCharInRow = - col == rows.text.at(row).length() - 1 || - rows.text.at(row).at(col + 1) == '\r' || - rows.text.at(row).at(col + 1) == '\n'; - - bool colorChanged = false; - if (!fgColor.has_value() || rows.FgAttr.at(row).at(col) != fgColor.value()) - { - fgColor = rows.FgAttr.at(row).at(col); - colorChanged = true; - } - - if (!bkColor.has_value() || rows.BkAttr.at(row).at(col) != bkColor.value()) - { - bkColor = rows.BkAttr.at(row).at(col); - colorChanged = true; - } - const auto writeAccumulatedChars = [&](bool includeCurrent) { if (col >= startOffset) { @@ -1289,6 +1269,27 @@ std::string TextBuffer::GenHTML(const TextAndColor& rows, const int fontHeightPo } }; + if (rows.text.at(row).at(col) == '\r' || rows.text.at(row).at(col) == '\n') + { + // do not include \r nor \n as they don't have color attributes + // and are not HTML friendly. For line break use '
' instead. + writeAccumulatedChars(false); + break; + } + + bool colorChanged = false; + if (!fgColor.has_value() || rows.FgAttr.at(row).at(col) != fgColor.value()) + { + fgColor = rows.FgAttr.at(row).at(col); + colorChanged = true; + } + + if (!bkColor.has_value() || rows.BkAttr.at(row).at(col) != bkColor.value()) + { + bkColor = rows.BkAttr.at(row).at(col); + colorChanged = true; + } + if (colorChanged) { writeAccumulatedChars(false); @@ -1310,10 +1311,10 @@ std::string TextBuffer::GenHTML(const TextAndColor& rows, const int fontHeightPo hasWrittenAnyText = true; - if (isLastCharInRow) + // if this is the last character in the row, flush the whole row + if (col == rows.text.at(row).length() - 1) { writeAccumulatedChars(true); - break; } } } @@ -1430,24 +1431,6 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi for (size_t col = 0; col < rows.text.at(row).length(); ++col) { - const bool isLastCharInRow = - col == rows.text.at(row).length() - 1 || - rows.text.at(row).at(col + 1) == '\r' || - rows.text.at(row).at(col + 1) == '\n'; - - bool colorChanged = false; - if (!fgColor.has_value() || rows.FgAttr.at(row).at(col) != fgColor.value()) - { - fgColor = rows.FgAttr.at(row).at(col); - colorChanged = true; - } - - if (!bkColor.has_value() || rows.BkAttr.at(row).at(col) != bkColor.value()) - { - bkColor = rows.BkAttr.at(row).at(col); - colorChanged = true; - } - const auto writeAccumulatedChars = [&](bool includeCurrent) { if (col >= startOffset) { @@ -1470,6 +1453,27 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi } }; + if (rows.text.at(row).at(col) == '\r' || rows.text.at(row).at(col) == '\n') + { + // do not include \r nor \n as they don't have color attributes. + // For line break use \line instead. + writeAccumulatedChars(false); + break; + } + + bool colorChanged = false; + if (!fgColor.has_value() || rows.FgAttr.at(row).at(col) != fgColor.value()) + { + fgColor = rows.FgAttr.at(row).at(col); + colorChanged = true; + } + + if (!bkColor.has_value() || rows.BkAttr.at(row).at(col) != bkColor.value()) + { + bkColor = rows.BkAttr.at(row).at(col); + colorChanged = true; + } + if (colorChanged) { writeAccumulatedChars(false); @@ -1513,10 +1517,10 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi << " "; } - if (isLastCharInRow) + // if this is the last character in the row, flush the whole row + if (col == rows.text.at(row).length() - 1) { writeAccumulatedChars(true); - break; } } }