Skip to content

Commit

Permalink
Fix redundant CR in formatted text copy (#4190)
Browse files Browse the repository at this point in the history
## 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)
  • Loading branch information
mcpiroman authored and DHowett committed Jan 24, 2020
1 parent 0c77366 commit ea690e1
Showing 1 changed file with 46 additions and 42 deletions.
88 changes: 46 additions & 42 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 '<BR>' 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)
{
Expand All @@ -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 '<BR>' 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);
Expand All @@ -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;
}
}
}
Expand Down Expand Up @@ -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)
{
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
}
}
Expand Down

0 comments on commit ea690e1

Please sign in to comment.