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.

Sign up for GitHub

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

Jump to bottom

Add 'copyFormatting' global setting #5263

Merged
2 commits merged into from
Apr 9, 2020
Merged

Add 'copyFormatting' global setting #5263

2 commits merged into from
Apr 9, 2020

Conversation

carlos-zamora
Copy link
Member

Summary of the Pull Request

Implements copyFormatting as a global setting. When enabled, formatting such as font and foreground/background colors are copied to the clipboard on all copy operations.

Also updates the schema and docs.

References

#5212 - Spec for Formatted Copying
#4191 - Setting to enable/disable formatted copy

This feature will also have an impact on these yet-to-be-implemented features:

PR Checklist

Detailed Description of the Pull Request / Additional comments

We already check if the hstring passed into the clipboard is empty before setting it. So the majority of the changes are actually just adding the global setting in.

Validation Steps Performed

copyFormatting Mouse Copy Keyboard Copy
not set (false) ✔ ✔
true ✔ ✔
false ✔ ✔

@carlos-zamora carlos-zamora added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Apr 6, 2020
@carlos-zamora
Copy link
Member Author

Question: Should this actually close #4191?

  • Yes: the sentiment behind this issue was to disable/enable formatted copy
  • No: the proposal was to copy specific formats

For now, I've assumed yes, but still wanted to point it out :)

DHowett-MSFT
DHowett-MSFT suggested changes Apr 6, 2020
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

Blocking until the spec is reviewed and committed.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 6, 2020
zadjii-msft
zadjii-msft approved these changes Apr 7, 2020
View reviewed changes
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'm approving even though I haven't signed off on the spec yet. This matches the spec we discussed, but I'll let Dustin make sure the actual spec is signed off

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 8, 2020
DHowett-MSFT
DHowett-MSFT suggested changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is it worth plumbing this all the way down into the control settings when it would suffice to check the GlobalAppSettings at the TerminalPage layer? This is what I was talking about in our meeting. People aren't asking for us to turn off formatted copy because they don't like its performance characteristics, they're asking for us to turn it off because they don't want it on their clipboards.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 8, 2020
@zadjii-msft
Copy link
Member

Is it worth plumbing this all the way down into the control settings

My thought is this is something VS might also want to be able to disable in the WPF control as well.

@DHowett-MSFT
Copy link
Contributor

If that’s the case we didn’t plumb it down deep enough because this check happens in our XAML control :)

@DHowett-MSFT
Copy link
Contributor

Generally though it’d be more flexible if we gave them every format and let them choose

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 9, 2020
DHowett-MSFT
DHowett-MSFT approved these changes Apr 9, 2020
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

This is way simpler, even if it wastes some CPU cycles generating formatted information that gets deleted. Are we okay with simple here? I don't want to "win" because I was the loudest voice 😄

@carlos-zamora
Copy link
Member Author

Generally though it’d be more flexible if we gave them every format and let them choose

You won me over with this comment up here haha

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 9, 2020
@ghost
Copy link

ghost commented Apr 9, 2020

Hello @carlos-zamora!

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 c55d9cd into master Apr 9, 2020
@ghost ghost deleted the dev/cazamor/html-global branch April 9, 2020 18:03
@DHowett-MSFT
Copy link
Contributor

come on. The spec didn’t merge.

DHowett-MSFT pushed a commit that referenced this pull request Apr 9, 2020
@DHowett
Revert "Add 'copyFormatting' global setting (#5263)"
2f8b947 
This reverts commit c55d9cd.
@carlos-zamora
Copy link
Member Author

come on. The spec didn’t merge.

You're approval threw me off. Sorry!

DHowett-MSFT pushed a commit that referenced this pull request Apr 9, 2020
@DHowett
Revert "Add 'copyFormatting' global setting (#5263)" (#5298)
edf79b2 
This reverts commit c55d9cd.
It was merged with its corresponding specification unmerged.
@DHowett-MSFT DHowett-MSFT restored the dev/cazamor/html-global branch April 9, 2020 18:13
@carlos-zamora carlos-zamora mentioned this pull request Apr 9, 2020
Merged
ghost pushed a commit that referenced this pull request Apr 9, 2020
@carlos-zamora
Add 'copyFormatting' global setting (#5299)
ea1bb2e 
## Summary of the Pull Request
Implements `copyFormatting` as a global setting. When enabled, formatting such as font and foreground/background colors are copied to the clipboard on _all_ copy operations.

Also updates the schema and docs.

## References
#5212 - Spec for Formatted Copying
#4191 - Setting to enable/disable formatted copy

#5263 - PR prematurely merged without approval of #5212 

This feature will also have an impact on these yet-to-be-implemented features:
- #5262 - copyFormatting Keybinding Arg for Copy
- #1553 - Pointer Bindings
- #4191 - add array support for `copyFormatting`


## Detailed Description of the Pull Request
We already check if the hstring passed into the clipboard is empty before setting it. So the majority of the changes are actually just adding the global setting in.

## Validation Steps Performed
| `copyFormatting` | Mouse Copy | Keyboard Copy |
|--|--|--|
| not set (`false`) | ✔ | ✔ |
| `true` | ✔ | ✔ |
| `false` | ✔ | ✔ |
@ghost ghost deleted the dev/cazamor/html-global branch April 9, 2020 23:32
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

@cinnamon-msft cinnamon-msft Awaiting requested review from cinnamon-msft

Assignees
No one assigned
Labels
Area-Settings Issues related to settings and customizability, for console or terminal AutoMerge Marked for automatic merge by the bot when requirements are met Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add setting to select which formats of copy to perform (text, rtf or html)
3 participants
@carlos-zamora @zadjii-msft @DHowett-MSFT