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

Bugfix: Line Selection Copy #2755

Closed
wants to merge 1 commit into from

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Sep 14, 2019

Summary of the Pull Request

Looks like this got moved from ConHost improperly. Thankfully, the comment helped track this down. I'll add tests on Monday.

References

PR Checklist

Validation Steps Performed

For CMD, Pwsh, and Ubuntu:

  • run dir/ls command
  • select first three lines
  • copy/copyTextWithoutNewlines
  • paste to notepad

Compare this behavior to ConHost.

@carlos-zamora carlos-zamora self-assigned this Sep 14, 2019
@DHowett-MSFT
Copy link
Contributor

Yay! There’s actually more bug numbers for this, if I recall properly?

@carlos-zamora
Copy link
Member Author

carlos-zamora commented Sep 14, 2019

TODOs:

  • update doc/user-docs/index.md section on copy/paste
  • update doc/user-docs/UsingJsonSettings.md section (Note is wrong at the bottom)
  • Tests failed, so it looks like the variable name was wrong and TermControl should be passing in the opposite value.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love a unit test here to prevent this from regressing again, if at all possible. I also want this shipped so do what you need to.

@carlos-zamora
Copy link
Member Author

Unfortunately, after further investigation, this isn't the correct solution. Look at the following code (particularly the lines with --> on the left:

        if (trimTrailingWhitespace)
        {
            const ROW& Row = GetRowByOffset(iRow);

            // FOR LINE SELECTION ONLY: if the row was wrapped, don't remove the spaces at the end.
-->         if (!lineSelection || !Row.GetCharRow().WasWrapForced())
            {
                while (!selectionText.empty() && selectionText.back() == UNICODE_SPACE)
                {
                    selectionText.pop_back();
                    selectionFgAttr.pop_back();
                    selectionBkAttr.pop_back();
                }
            }

            // apply CR/LF to the end of the final string, unless we're the last line.
            // a.k.a if we're earlier than the bottom, then apply CR/LF.
            if (i < selectionRects.size() - 1)
            {
                // FOR LINE SELECTION ONLY: if the row was wrapped, do not apply CR/LF.
                // a.k.a. if the row was NOT wrapped, then we can assume a CR/LF is proper
                // always apply \r\n for box selection
-->             if (!lineSelection || !GetRowByOffset(iRow).GetCharRow().WasWrapForced())
                {
                    COLORREF const Blackness = RGB(0x00, 0x00, 0x00); // cant see CR/LF so just use black FG & BK

                    selectionText.push_back(UNICODE_CARRIAGERETURN);
                    selectionText.push_back(UNICODE_LINEFEED);
                    selectionFgAttr.push_back(Blackness);
                    selectionFgAttr.push_back(Blackness);
                    selectionBkAttr.push_back(Blackness);
                    selectionBkAttr.push_back(Blackness);
                }
            }
        }

When doing the test described in the attached bug report, Row.GetCharRow().WasWrapForced() is false in ConHost, but true in Windows Terminal. ConHost has the correct behavior here. I'm investigating this issue further and closing the PR for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy inconsistently copies as "copyTextWithoutNewlines"
4 participants