doc: add a spec for the change to formatted copying #5212

cinnamon-msft · 2020-04-01T18:23:57Z

Summary of the Pull Request

This is the spec that goes into what we do with HTML copy once we set the default copy behavior to plain text.

References

#4191

PR Checklist

Closes #xxx
CLA signed. If not, go over here and sign the CLA
Tests added/passed
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: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

doc/specs/#4191 - Formatted Copy/spec.md

carlos-zamora · 2020-04-01T18:44:52Z

doc/specs/#4191 - Formatted Copy/spec.md

+
+One possible issue is that discovering how to copy the formatting might be difficult to find. We could mitigate this by adding it into the settings.json file and commenting it out.
+
+## Future considerations


#1553 - We should just treat Alt like we do with Shift in VTMM. It's a non-rebindable keybinding until then.

doc/specs/#4191 - Formatted Copy/spec.md

carlos-zamora · 2020-04-01T19:05:35Z

doc/specs/#4191 - Formatted Copy/spec.md

+a. Just like the the `trimWhitespace` argument you can add to the `copy` key binding, we could add one for text formatting.
+
+`{"command": {"action": "copy", "keepFormatting": true}, "keys": "ctrl+a"}`


Personally, like this idea the best. It's simple and straightforward. At some point, after deciding which route to go with, we should decide what the default keybinding will be, if any.

DHowett-MSFT · 2020-04-01T19:07:58Z

I have a strong distaste for changing our default because of a handful of applications that lack "paste plain text" as an option. It's not a hill I'm willing to die on, but look:

people say they want plain text, they can have it (via a couple steps)
pasting into a terminal already strips HTML/RTF
pasting into a rich document preserves rich formatting
pasting into a spreadsheet auto-strips html/rtf
pasting into a preformatted code block or code editor auto-strips html/rtf
people are going to be stuck in the mid 1900s forever, and we just look like we simply don't support html copy (because nobody's going to read our json file, let's be honest)

carlos-zamora · 2020-04-01T19:15:13Z

I have a strong distaste for changing our default because of a handful of applications that lack "paste plain text" as an option. It's not a hill I'm willing to die on, but look:

people say they want plain text, they can have it (via a couple steps)

pasting into a terminal already strips HTML/RTF

pasting into a rich document preserves rich formatting

pasting into a spreadsheet auto-strips html/rtf

pasting into a preformatted code block or code editor auto-strips html/rtf

people are going to be stuck in the mid 1900s forever, and we just look like we simply don't support html copy (because nobody's going to read our json file, let's be honest)

That seems fair. So then the keybinding arg would default to "true" or "rtf, html, plain"? (this should be in the spec)

carlos-zamora · 2020-04-01T19:22:48Z

(also maybe worth adding to the spec)
I was wondering "what if somebody wants to just copy a specific aspect of the formatting like the font size/family?". I feel that the main annoyance with html copy is the following scenario:

I'll have a dark background color but acrylic tuned to make it legible
I then copy it to MSFT Word
since acrylic can't be copied over (and that's just impossible to do) it may no longer be legible
But if that's the case, and all I really wanted was the font family, then that should be something we allow as a part of FR: Customizable HTML/formated copy #2131 later. For now, the user would just have to paste with the current formatting, then tweak to make it look better, which I feel is fair.

My bad. I didn't realize I approved this spec until just now.

Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>

…t/terminal into cinnamon/html-copy-spec

cinnamon-msft · 2020-04-01T23:31:30Z

I am strongly of the opinion that our default should be plain text copy. The votes were overwhelming towards plain text and other terminal emulators behave in this way (including conhost). We cannot guarantee that the receiving application will give you the ability to format your pasted text (Microsoft Teams for example).

To address your concern about discoverability, if it's a global setting, it would be easy to find in a settings UI. We could also ship either the key binding or the global setting inside settings.json.

zadjii-msft

Okay so I've got some thoughts off the cuff:

I think I prefer option 2, the keybinding arg one. That way users could specify different bindings for different formats.
- maybe if we had a global setting, then that would apply to all copy actions, unless they set format manually. So you could use ["html", "rtf", "plain"] globally for all copy actions, but then a particular keybinding only has ["plain"]. This is more of a 1&2 combo solution.
I don't love the magic alt logic on right-click. Especially if we go with the global setting route. This would make more sense once we had mouse bindings, but I'm not so sure about it nowadays.
We could always support true == ["html", "rtf", "plain"], so the user could specify both? though the naming might be confusing then
I'd really love if we could have the "default" in userDefaults.json (the one that's created for new settings files) that includes all of ["html", "rtf", "plain"] by default, with a note that the user can remove whatever formats they want. That way, it's immensly more discoverable that those are options, and that they're trivial to opt out of.
- The whole "copy as formatted text" thing was something that I wouldn't have thought I wanted until I had it and now I love it. If it's not immediately obvious as an option, I wouldn't have even looked for it.
This sure sounds like a lot of work, more than a bugfix. Are we planning on getting this in on this side of 1.0?

carlos-zamora · 2020-04-02T23:34:19Z

We could always support true == ["html", "rtf", "plain"], so the user could specify both? though the naming might be confusing then

I really like this. So then the following two keybindings would be deserialized to be the same thing:

{"command": {"action": "copy", "formats": ["html","rtf","plain"]}, "keys": "ctrl+a"}

and

{"command": {"action": "copy", "formats": true}, "keys": "ctrl+a"}

I'd really love if we could have the "default" in userDefaults.json (the one that's created for new settings files) that includes all of ["html", "rtf", "plain"] by default, with a note that the user can remove whatever formats they want. That way, it's immensly more discoverable that those are options, and that they're trivial to opt out of.

Also agree (this should go in the spec)

doc/specs/#4191 - Formatted Copy/spec.md

Co-Authored-By: Josh Soref <jsoref@users.noreply.github.com>

doc/specs/#4191 - Formatted Copy/spec.md

DHowett-MSFT · 2020-04-07T17:50:33Z

doc/specs/#4191 - Formatted Copy/spec.md

+
+`"copyFormats": ["html","rtf","plain"]`
+
+b. We could also just combine html and rtf into a single boolean. Users would either get plain text only (`false`) or all formatting (`true`) onto their clipboard. If this is set to `true`, the default right click behavior is reversed: right clicking copies the formatting and holding `alt` copies only the plain text.


like other specs before it, make sure to call out the option we finally decided upon and move all unchosen options into an investigation notes section.

A spec isn't useful if it just says "we could do a, b, c" without it coming to its own consensus.

I followed the same formatting as the default profile settings spec and added a conclusions section at the bottom.

I think it might be clearer to make sure to mention "here are some proposals, with conclusions at the bottom" then, because I too got confused

leonMSFT

Just a couple of clarifications!

doc/specs/#4191 - Formatted Copy/spec.md

leonMSFT · 2020-04-09T06:36:41Z

doc/specs/#4191 - Formatted Copy/spec.md

+
+`"copyFormats": ["html","rtf","plain"]`
+
+b. We could also just combine html and rtf into a single boolean. Users would either get plain text only (`false`) or all formatting (`true`) onto their clipboard. If this is set to `true`, the default right click behavior is reversed: right clicking copies the formatting and holding `alt` copies only the plain text.


Correct me if I'm wrong, but if option b is the one we're going for, we're not having the Alt-RightClick to copy plaintext/formatting right? Right click should just copy formatting if the setting is true, and plain text if false.

Ah nice catch. I just removed references to alt right click functionality. 👍

Co-Authored-By: Leon Liang <57155886+leonMSFT@users.noreply.github.com>

…t/terminal into cinnamon/html-copy-spec

zadjii-msft

Yea these are kind of nits, so I'm not gonna block

zadjii-msft · 2020-04-09T16:58:00Z

doc/specs/#4191 - Formatted Copy/spec.md

+
+`"copyFormats": ["html","rtf","plain"]`
+
+b. We could also just combine html and rtf into a single boolean. Users would either get plain text only (`false`) or all formatting (`true`) onto their clipboard. If this is set to `true`, the default right click behavior is reversed: right clicking copies the formatting and holding `alt` copies only the plain text.


I think it might be clearer to make sure to mention "here are some proposals, with conclusions at the bottom" then, because I too got confused

doc/specs/#4191 - Formatted Copy/spec.md

zadjii-msft · 2020-04-09T17:03:19Z

doc/specs/#4191 - Formatted Copy/spec.md

+
+## Conclusions
+
+The team has decided to have plain text as the default copy behavior and to enable formatted copying with a global setting that accepts a boolean value (settings option 1 - global setting, option b). In the future, we can modify this setting to also accept an array, so the user can specify which formats they would like to copy. Additionally, a key binding can be added to allow for greater flexibility.


Wait didn't we decide on a combo of both 2 & 3?

We do the global setting now, as a boolean, because that's easy. (This is 2a)

We can then update that to additionally accept an array in the future, where true == [html, rtf, plain] and false==[plain] (This is 2b)

Then, we could further add an optional parameter to the copy ShortcutAction. If it's set, we'll copy with the provided formatting. Otherwise, we'll use the value of the global setting? (This is 3)

Okay I guess this is kinda what it does say, I just didn't immediately parse that this was a phased implementation plan

doc/specs/#4191 - Formatted Copy/spec.md

leonMSFT

lookin gud

## 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: - #5262 - copyFormatting Keybinding Arg for Copy - #1553 - Pointer Bindings ## PR Checklist * [X] Closes #4191 ## 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` | ✔ | ✔ |

cinnamon-msft · 2020-04-09T20:33:02Z

I addressed @zadjii-msft's nits as well as added @carlos-zamora's comment to the Future Considerations section. :)

carlos-zamora · 2020-04-09T22:04:37Z

I addressed @zadjii-msft's nits as well as added @carlos-zamora's comment to the Future Considerations section. :)

The main thing holding me back is still that the GitHub issues aren't linked. That makes things like this easier to track.

zadjii-msft

Thanks

## 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` | ✔ | ✔ |

Adds array support for the existing `copyFormatting` global setting. This allows users to define which formats they would specifically like to be copied. A boolean value is still accepted and is translated to the following: - `false` --> `"none"` or `[]` - `true` --> `"all"` or `["html", "rtf"]` This also adds `copyFormatting` as a keybinding arg for `copy`. As with the global setting, a boolean value and array value is accepted. CopyFormat is a WinRT enum where each accepted format is a flag. Currently accepted formats include `html`, and `rtf`. A boolean value is accepted and converted. `true` is a conjunction of all the formats. `false` only includes plain text. For the global setting, `null` is not accepted. We already have a default value from before so no worries there. For the keybinding arg, `null` (the default value) means that we just do what the global arg says to do. Overall, the `copyFormatting` keybinding arg is an override of the global setting **when using that keybinding**. References #5212 - Spec for formatted copying References #2690 - disable html copy Validated behavior with every combination of values below: - `copyFormatting` global: { `true`, `false`, `[]`, `["html"]` } - `copyFormatting` copy arg: { `null`, `true`, `false`, `[]`, `[, "html"]`} Closes #4191 Closes #5262

spec for html copy

e90193c

cinnamon-msft linked an issue Apr 1, 2020 that may be closed by this pull request

Add setting to select which formats of copy to perform (text, rtf or html) #4191

Closed

cinnamon-msft removed a link to an issue Apr 1, 2020

Add setting to select which formats of copy to perform (text, rtf or html) #4191

Closed

DHowett-MSFT suggested changes Apr 1, 2020

View reviewed changes

doc/specs/#4191 - Formatted Copy/spec.md Outdated Show resolved Hide resolved

doc/specs/#4191 - Formatted Copy/spec.md Outdated Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 1, 2020

carlos-zamora previously approved these changes Apr 1, 2020

View reviewed changes

Update doc/specs/#4191 - Formatted Copy/spec.md

b691666

Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 1, 2020

cinnamon-msft added 2 commits April 1, 2020 16:19

addressed feedback

fa1e65b

Merge branch 'cinnamon/html-copy-spec' of https://github.com/microsof…

061bd6a

…t/terminal into cinnamon/html-copy-spec

zadjii-msft reviewed Apr 2, 2020

View reviewed changes

carlos-zamora mentioned this pull request Apr 3, 2020

Update default settings.json experience #5217

Merged

3 tasks

jsoref reviewed Apr 5, 2020

View reviewed changes

cinnamon-msft and others added 3 commits April 6, 2020 10:44

Update doc/specs/#4191 - Formatted Copy/spec.md

43a0522

Co-Authored-By: Josh Soref <jsoref@users.noreply.github.com>

Update doc/specs/#4191 - Formatted Copy/spec.md

fce8895

Co-Authored-By: Josh Soref <jsoref@users.noreply.github.com>

Update doc/specs/#4191 - Formatted Copy/spec.md

7f62273

Co-Authored-By: Josh Soref <jsoref@users.noreply.github.com>

This was referenced Apr 6, 2020

copyFormatting Keybinding Arg for Copy #5262

Closed

Add 'copyFormatting' global setting #5263

Merged

cinnamon-msft added 2 commits April 7, 2020 10:41

Merge branch 'master' into cinnamon/html-copy-spec

ff82a35

minor fixes

fb5ddc2

cinnamon-msft requested a review from DHowett-MSFT April 7, 2020 17:49

DHowett-MSFT suggested changes Apr 7, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 7, 2020

added conclusion

16f23a2

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 7, 2020

cinnamon-msft requested a review from DHowett-MSFT April 7, 2020 20:58

DHowett-MSFT approved these changes Apr 7, 2020

View reviewed changes

DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label Apr 7, 2020

ghost requested review from miniksa and leonMSFT April 7, 2020 21:38

leonMSFT suggested changes Apr 9, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 9, 2020

Update doc/specs/#4191 - Formatted Copy/spec.md

a9e0147

Co-Authored-By: Leon Liang <57155886+leonMSFT@users.noreply.github.com>

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

cinnamon-msft added 2 commits April 9, 2020 10:03

removed alt functionality on mouse

ac55620

Merge branch 'cinnamon/html-copy-spec' of https://github.com/microsof…

9c39283

…t/terminal into cinnamon/html-copy-spec

zadjii-msft reviewed Apr 9, 2020

View reviewed changes

leonMSFT approved these changes Apr 9, 2020

View reviewed changes

carlos-zamora mentioned this pull request Apr 9, 2020

Add 'copyFormatting' global setting #5299

Merged

addressed nits

4953f54

zadjii-msft approved these changes Apr 9, 2020

View reviewed changes

DHowett-MSFT changed the title ~~Spec for Formatted Copying~~ doc: add a spec for the change to formatted copying Apr 9, 2020

DHowett-MSFT merged commit 08df4fc into master Apr 9, 2020

DHowett-MSFT deleted the cinnamon/html-copy-spec branch April 9, 2020 23:30

carlos-zamora mentioned this pull request May 19, 2020

Add copyFormatting keybinding arg and array support #6004

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: add a spec for the change to formatted copying #5212

doc: add a spec for the change to formatted copying #5212

cinnamon-msft commented Apr 1, 2020

carlos-zamora Apr 1, 2020

carlos-zamora Apr 1, 2020

DHowett-MSFT commented Apr 1, 2020

carlos-zamora commented Apr 1, 2020

carlos-zamora commented Apr 1, 2020

cinnamon-msft commented Apr 1, 2020

zadjii-msft left a comment

carlos-zamora commented Apr 2, 2020

DHowett-MSFT Apr 7, 2020

DHowett-MSFT Apr 7, 2020

cinnamon-msft Apr 7, 2020

zadjii-msft Apr 9, 2020

leonMSFT left a comment

leonMSFT Apr 9, 2020

cinnamon-msft Apr 9, 2020

zadjii-msft left a comment

zadjii-msft Apr 9, 2020

zadjii-msft Apr 9, 2020

zadjii-msft Apr 9, 2020

leonMSFT left a comment

cinnamon-msft commented Apr 9, 2020

carlos-zamora commented Apr 9, 2020

zadjii-msft left a comment


		One possible issue is that discovering how to copy the formatting might be difficult to find. We could mitigate this by adding it into the settings.json file and commenting it out.

		## Future considerations

		a. Just like the the `trimWhitespace` argument you can add to the `copy` key binding, we could add one for text formatting.

		`{"command": {"action": "copy", "keepFormatting": true}, "keys": "ctrl+a"}`


		`"copyFormats": ["html","rtf","plain"]`

		b. We could also just combine html and rtf into a single boolean. Users would either get plain text only (`false`) or all formatting (`true`) onto their clipboard. If this is set to `true`, the default right click behavior is reversed: right clicking copies the formatting and holding `alt` copies only the plain text.


		## Conclusions

		The team has decided to have plain text as the default copy behavior and to enable formatted copying with a global setting that accepts a boolean value (settings option 1 - global setting, option b). In the future, we can modify this setting to also accept an array, so the user can specify which formats they would like to copy. Additionally, a key binding can be added to allow for greater flexibility.

doc: add a spec for the change to formatted copying #5212

doc: add a spec for the change to formatted copying #5212

Conversation

cinnamon-msft commented Apr 1, 2020

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Apr 1, 2020

carlos-zamora commented Apr 1, 2020

carlos-zamora commented Apr 1, 2020

cinnamon-msft commented Apr 1, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

carlos-zamora commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonMSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonMSFT left a comment

Choose a reason for hiding this comment

cinnamon-msft commented Apr 9, 2020

carlos-zamora commented Apr 9, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment