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

Fix Copy to Clipboard to preserve visual structure of block selection #8579

Merged
1 commit merged into from
Dec 14, 2020

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Dec 13, 2020

There are two issue with copy to clipboard when block is selected:

  • We don't add new lines for lines that were wrapped
  • We remove trailing whitespaces which is not intuitive in block selection.

Fixed the copy logic to always add newlines and not to remove
whitespaces when block is selected.

Even if shift is pressed!

Detailed Description of the Pull Request / Additional comments

  • Added optional parameter to TextBuffer::GetText
    that allows to apply formatting (includeCRLF / trimming)
    to lines that were wrapped
  • Changed Terminal::RetrieveSelectedTextFromBuffer
    to apply the following parameters when block is selected:
    • includeCRLF = true
    • trimTrailingWhitespaces = false
    • apply the formatting above to all rows, including the ones
      that were wrapped

Validation Steps Performed

  • Manual tests for both block and standard selection
  • Copy with both right-click and command
  • Added UT

Closes #6740

@ghost ghost 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. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Dec 13, 2020
@Don-Vito
Copy link
Contributor Author

This is how the block selection looks now:
image

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

The added tests make me super comfortable with this change. Thank you 😊

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

blocking per discussion earlier -- gotta think about this a bit 😄

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 14, 2020
@Don-Vito
Copy link
Contributor Author

blocking per discussion earlier -- gotta think about this a bit 😄

Let's probably start with preserving whitespaces, but at least move forward with the line wraps?

WDYT?

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 14, 2020
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Let's try this on for size.

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 14, 2020
@ghost
Copy link

ghost commented Dec 14, 2020

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit a1f42e8 into microsoft:main Dec 14, 2020
@ghost
Copy link

ghost commented Jan 28, 2021

🎉Windows Terminal Preview v1.6.10272.0 has been released which incorporates this pull request.:tada:

Handy links:

mpela81 pushed a commit to mpela81/terminal that referenced this pull request Jan 28, 2021
…microsoft#8579)

There are two issue with copy to clipboard when block is selected:
* We don't add new lines for lines that were wrapped
* We remove trailing whitespaces which is not intuitive in block selection.

Fixed the copy logic to always add newlines and not to remove
whitespaces when block is selected.

Even if shift is pressed!

## Detailed Description of the Pull Request / Additional comments
* Added optional parameter to `TextBuffer::GetText` 
that allows to apply formatting (includeCRLF / trimming) 
to lines that were wrapped
* Changed `Terminal::RetrieveSelectedTextFromBuffer` 
to apply the following parameters when block is selected:
  * includeCRLF = true
  * trimTrailingWhitespaces = false
  * apply the formatting above to all rows, including the ones 
that were wrapped 

## Validation Steps Performed
* Manual tests for both block and standard selection
* Copy with both right-click and command
* Added UT

Closes microsoft#6740
This pull request 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.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block copy with the ALT+MOUSE unexpectedly wraps the copied text into a single string
3 participants