Add support for HTML copy #1224

carlos-zamora · 2019-06-12T18:01:26Z

Summary of the Pull Request

HTML data is now copied to the clipboard. I tried reusing the old clipboard method so I had to move some stuff around. The design is pretty straightforward. Instead of sending just text data up on a CopyToClipboard event, send the HTML data up too in the same winrt object.

Here's some specifics:

Clipboard::GenHTML() Changes:
- moved from Clipboard to TextBuffer
- added iFontHeightPoints and fontFaceName so that I don't depend on the ServiceLocator
TermControl: have CopyToClipboardEventArgs have space for both data types
TerminalApp: just extract the data from the event and put it into the clipboard

References

#1146 (HTML portion)

PR Checklist

Closes half of Feature Request: HTML Copy #1146
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.

Detailed Description of the Pull Request / Additional comments

I do have a few things I'm not sure of:

As of now, HTML copy is always enabled. I can easily add make it optional but idk what the design should look like (i.e.: new keybinding copies only HTML?)
I don't think we can/should add tests to this because (1) it resuses old code and (2) we definitely don't want to actually copy stuff to the clipboard during a test. The only thing to test would be what GenHTML does but that was already here.
Not sure if the iFontHeightPointsis right (or even described properly for that matter) in GenHTML().
- for Clipboard we do int const iFontHeightPoints = fontData.GetUnscaledSize().Y * 72 / ServiceLocator::LocateGlobals().dpi;
- for TermControl I did fontData.GetUnscaledSize().Y
- I don't really know how to verify if this is correct. But I also don't know if we actually even need the DPI here. Thoughts?

Validation Steps Performed

Copy selections and paste to MSWord (regular text vs formatted text)

fcharlie · 2019-06-13T01:07:22Z

src/buffer/out/textBuffer.cpp

+
+    try
+    {
+        std::string const szHtmlClipFormat =


Maybe constexpr std::wstring_view is better

fcharlie · 2019-06-13T01:07:35Z

src/buffer/out/textBuffer.cpp

+        // measure clip header
+        size_t const cbHeader = 157; // when formats are expanded, there will be 157 bytes in the header.
+
+        std::string const szHtmlHeader =


Maybe constexpr std::wstring_view is better

mauve · 2019-06-13T06:36:05Z

src/buffer/out/textBuffer.cpp

+        size_t const cbHtmlHeader = szHtmlHeader.size();
+
+        std::string const szHtmlFragStart = "<!--StartFragment -->";
+        std::string const szHtmlFragEnd = "<!--EndFragment -->";


This constant is only used once (in line 1239), its definition should be moved to that location and the constant should be changed to constexpr std::string_view:

// copy HTML end fragment constexpr std::string_view szHtmlFragEnd = "..."; szClipboard.append(szHtmlFragEnd);

Given that the constant is only used once and the point of use even has a comment stating what is being done (append end fragment) you can even consider removing the constant altogether, as this code is not less readable than the above:

// copy HTML end fragment szClipboard.append("...");

mauve · 2019-06-13T06:36:22Z

src/buffer/out/textBuffer.cpp

+            "<!DOCTYPE><HTML><HEAD><TITLE>Windows Console Host</TITLE></HEAD><BODY>";
+        size_t const cbHtmlHeader = szHtmlHeader.size();
+
+        std::string const szHtmlFragStart = "<!--StartFragment -->";


move to point of first use

mauve · 2019-06-13T06:37:16Z

src/buffer/out/textBuffer.cpp

+        std::string szSpanFontSize;
+        szSpanFontSize.resize(cbSpanFontSize + 1); // reserve space for null after string for sprintf
+        sprintf_s(szSpanFontSize.data(), cbSpanFontSize + 1, szSpanFontSizePattern.data(), iFontHeightPoints);
+        szSpanFontSize.resize(cbSpanFontSize); //chop off null at end


nitpick: This might be the only way to do formatting, but the resize, format, resize again seems cumbersome.

I know everybody hates C++ streams but it seems that they would work relatively well for this function at least.

mauve · 2019-06-13T06:37:36Z

src/buffer/out/textBuffer.cpp

+// - string containing the generated HTML
+std::string TextBuffer::GenHTML(const TextAndColor& rows, const int iFontHeightPoints, const PCWCHAR fontFaceName)
+{
+    std::string szClipboard; // we will build the data going back in this string buffer


move to point of first use

src/buffer/out/textBuffer.cpp

mauve · 2019-06-13T06:41:01Z

src/buffer/out/textBuffer.cpp

+
+        std::string const szSpanStartPattern = R"X(<SPAN STYLE="color:#%02x%02x%02x;background-color:#%02x%02x%02x">)X";
+
+        size_t const cbSpanStart = 53; // when format is expanded, there will be 53 bytes per color pattern.


Are we still using Hungarian Notation? According to NL.5: Avoid encoding type information in names explicitly not recommended.

zadjii-msft

Overall I'm fine with this, save for the _actualFont thing. I don't think it's your responsibility to update GenHTML to be more stylistically modern, since you're just lifting that function. We could always file a follow-up issue to modernize that function.

src/cascadia/TerminalControl/TermControl.h

src/cascadia/TerminalControl/TermControl.cpp

src/interactivity/win32/Clipboard.cpp

oising · 2019-06-13T20:52:17Z

Oh, HTML copy :)

CSI Pm i  Media Copy (MC).
            Ps = 0  -> Print screen (default).
            Ps = 4  -> Turn off printer controller mode.
            Ps = 5  -> Turn on printer controller mode.
            Ps = 1 0  -> HTML screen dump, xterm.
            Ps = 1 1  -> SVG screen dump, xterm.

Refactor RetrieveSelectedTextFromBuffer Modify CopyToClipboardEventArgs to include HTML data

NOTE: refactoring text buffer code is a separate task. New issue to be created.

Fixes #1846.

src/cascadia/TerminalControl/TermControl.cpp

src/cascadia/TerminalApp/App.cpp

DHowett-MSFT · 2019-08-19T18:29:12Z

src/interactivity/win32/Clipboard.cpp

@@ -515,7 +275,9 @@ void Clipboard::CopyTextToSystemClipboard(const TextBuffer::TextAndColor& rows,

    if (fAlsoCopyHtml)
    {
-        std::string HTMLToPlaceOnClip = GenHTML(rows);
+        const auto& fontData = ServiceLocator::LocateGlobals().getConsoleInformation().GetActiveOutputBuffer().GetCurrentFont();
+        int const iFontHeightPoints = fontData.GetUnscaledSize().Y * 72 / ServiceLocator::LocateGlobals().dpi;


wonder if there's an existing constant for this 72.

I hate to say it but I couldn't find anything on where the value came from or what constant we could use for it. :(

DHowett-MSFT · 2019-08-19T18:30:21Z

nit: name. something like add support for HTML copy or whatever

ghost · 2019-08-19T22:46:06Z

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 · 2019-08-27T16:09:29Z

🎉Windows Terminal Preview v0.4.2382.0 has been released which incorporates this pull request.:tada:

Handy links:

## Summary of the Pull Request RTF data is now copied to the clipboard. Tested by copy pasting text from terminal to WordPad.  ## PR Checklist * [x] Closes #2487 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Requires documentation to be updated * [x] 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: #2487  ## Detailed Description of the Pull Request / Additional comments Mostly similar to PR #1224. Added a new static method `GenRTF` in `TextBuffer` that is responsible for generating the RTF representation of a given text. The generated RTF is added to the `DataPackage` that is ultimately passed to the clipboard.  ## Validation Steps Performed Validated by copy pasting text from the terminal to WordPad. Validated with different colors to make sure that is working. (MS Word seems to prefer HTML data from the clipboard instead of RTF.) <hr> * Copy RTF data to the clipboard * Added comment explaining various parts of the header * Fixed static code analysis issues and added noexcept to GenRTF() * Removed noexcept

fcharlie reviewed Jun 13, 2019

View reviewed changes

mauve reviewed Jun 13, 2019

View reviewed changes

zadjii-msft requested changes Jun 13, 2019

View reviewed changes

src/cascadia/TerminalControl/TermControl.h Outdated Show resolved Hide resolved

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved

src/interactivity/win32/Clipboard.cpp 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 Jun 13, 2019

zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. labels Jun 13, 2019

carlos-zamora self-assigned this Jun 18, 2019

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 18, 2019

carlos-zamora added 3 commits July 5, 2019 13:13

Move Clipboard::GenHTML to TextBuffer (add params)

9116bdf

Refactor RetrieveSelectedTextFromBuffer Modify CopyToClipboardEventArgs to include HTML data

minor code format fix

44e3da8

PR Changes

514d007

NOTE: refactoring text buffer code is a separate task. New issue to be created.

carlos-zamora force-pushed the dev/cazamor/html-copy branch from 40a8d8e to 514d007 Compare July 5, 2019 21:16

carlos-zamora mentioned this pull request Jul 5, 2019

Modernize TextBuffer's GenHTML() function #1846

Closed

carlos-zamora mentioned this pull request Jul 26, 2019

Refactor TextBuffer::GenHTML #2038

Merged

5 tasks

Refactor TextBuffer::GenHTML (#2038)

fa89e53

Fixes #1846.

zadjii-msft approved these changes Aug 14, 2019

View reviewed changes

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved

carlos-zamora added 2 commits August 14, 2019 13:18

Merge branch 'master' into dev/cazamor/html-copy

a95d1cb

nit change

752be0f

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Aug 16, 2019

carlos-zamora added 2 commits August 16, 2019 15:02

x86 build fix

cdda59c

Merge branch 'master' into dev/cazamor/html-copy

7f7fcfc

DHowett-MSFT approved these changes Aug 19, 2019

View reviewed changes

carlos-zamora changed the title ~~HTML Copy~~ Add support for HTML copy Aug 19, 2019

nit changes

e93cb5c

carlos-zamora added AutoMerge Marked for automatic merge by the bot when requirements are met and removed Needs-Second It's a PR that needs another sign-off labels Aug 19, 2019

ghost merged commit 71eaf62 into master Aug 19, 2019

ghost deleted the dev/cazamor/html-copy branch August 19, 2019 22:59

carlos-zamora mentioned this pull request Aug 20, 2019

Feature Request: Rich Text Format (RTF) Copy #2487

Closed

zadjii-msft mentioned this pull request Aug 23, 2019

Clean up boundary between terminal app and terminal page #2208

Merged

4 tasks

carlos-zamora mentioned this pull request Sep 12, 2019

copied text does not include sufficient/correct HTML entity escaping #2738

Closed

anirudhrb mentioned this pull request Nov 12, 2019

Copy RTF data to the clipboard #3535

Merged

5 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for HTML copy #1224

Add support for HTML copy #1224

carlos-zamora commented Jun 12, 2019 •

edited

Loading

fcharlie Jun 13, 2019

fcharlie Jun 13, 2019

mauve Jun 13, 2019

mauve Jun 13, 2019

mauve Jun 13, 2019

mauve Jun 13, 2019

mauve Jun 13, 2019

zadjii-msft left a comment

oising commented Jun 13, 2019

DHowett-MSFT Aug 19, 2019

carlos-zamora Aug 19, 2019

DHowett-MSFT commented Aug 19, 2019

ghost commented Aug 19, 2019

ghost commented Aug 27, 2019


		std::string const szSpanStartPattern = R"X(<SPAN STYLE="color:#%02x%02x%02x;background-color:#%02x%02x%02x">)X";

		size_t const cbSpanStart = 53; // when format is expanded, there will be 53 bytes per color pattern.

Add support for HTML copy #1224

Add support for HTML copy #1224

Conversation

carlos-zamora commented Jun 12, 2019 • edited Loading

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

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

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

oising commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Aug 19, 2019

ghost commented Aug 19, 2019

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 commented Aug 27, 2019

carlos-zamora commented Jun 12, 2019 •

edited

Loading

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.