Refactor `TextBuffer::GenHTML/RTF` to read the buffer directly #16377

tusharsnx · 2023-11-26T15:50:00Z

TextBuffer::GenHTML and TextBuffer::GenRTF now read directly from the TextBuffer.

Since we're reading from the buffer, we can now read all the attributes saved in the buffer. Formatted copy now copies most (if not all) font/color attributes in the requested format (RTF/HTML).
Use TextBuffer::CopyRequest to pass all copy-related options into text generation functions as one unit.
Helper function TextBuffer::CopyRequest::FromConfig() generates a copy request based on Selection mode and user configuration.
Both formatted text generation functions now use std::string and fmt::format_to to generate the required strings. Previously, we were using std::ostringstream which is not recommended due to its potential overhead.
Reading attributes from ROW's attribute RLE simplified the logic as we don't have to track attribute change between the text.
On the caller side, we do not have to rebuild the plain text string from the vector of strings anymore. TextBuffer::GetPlainText() returns the entire text as one std::string.
Removed TextBuffer::TextAndColors.
Removed TextBuffer::GetText(). TextBuffer::GetPlainText() took its place.

This PR also fixes two bugs in the formatted copy:

We were applying line breaks after each selected row, even though the row could have been a Wrapped row. This caused the wrapped rows to break when they shouldn't.
We mishandled Unicode text (\uN) within the RTF copy. Every next character that uses a surrogate pair or high codepoint was missing in the copied text when pasted to MSWord. The command \uc4 should have been \uc1, which is used to tell how many fallback characters are used for each Unicode codepoint (\u). We always use one ? character as the fallback.

Closes #16191

References and Relevant Issues

Use full TextAttribute when copying data into Clipboard #16270

Validation Steps Performed

Casual copy-pasting from Terminal or OpenConsole to word editors works as before.
Verified HTML copy by copying the generated HTML string and running it through an HTML viewer. Sample
Verified RTF copy by copy-pasting the generated RTF string into MSWord.
SingleLine mode works (Shift+ copy)
BlockSelection mode works (Alt selection)

PR Checklist

Tests added/passed

we were adding newlines after every row! Causing wrapped rows to break unexpectedly

…different colors

tusharsnx · 2023-11-26T16:07:04Z

After lhecker's recommendation and implementing the changes, it seemed to be that it was probably better to create this PR instead of pushing of #16270 mainly because it deals with Copy operations at large.

This already includes all the changes from #16270. So if you guys are okay I suggest we discard the old PR and take this one forward.

tusharsnx · 2023-12-07T09:39:15Z

@lhecker

I'm writing a new tool

Performance measurement is not my strongest suit right now. I use VS profiler that comes built-in, but don't really know how to measure performance in an ideal way (e.g., comparing different implementations of the same function. If I had to do this I would probably be clicking Ctrl+Shift+A, Ctrl+C to copy all text and see how much time that took overall which is so wrong 😂). I'll be happy to try the tool you're building. Lemme know whenever that happens! 😄

And thanks for all the performance measurements you did. They're very insightful.

Edit:
Just came to know about instrumentation, and how it can be used in this situation. VS also recently shipped its instrumentation based profiler for C++: https://learn.microsoft.com/en-us/visualstudio/profiling/instrumentation-overview?view=vs-2022

lhecker · 2023-12-11T17:29:44Z

Sorry for the long wait - the tool still had a lot of rough edges and I used it as a test bed to learn some new programming paradigms. Here it is: #16453

tusharsnx · 2023-12-12T14:25:55Z

thanks for the tool @lhecker. it's a really good way to test performance regression precisely.

On my system, I can see these results:

openconsole (release) on main

openconsole (release) this PR

DHowett

OKAY I made it through the whole review! A couple questions, a couple notes, and a couple nits.

DHowett · 2023-12-12T19:09:30Z

src/interactivity/win32/clipboard.hpp

-        void CopyTextToSystemClipboard(const TextBuffer::TextAndColor& rows, _In_ const bool copyFormatting);
-        void CopyToSystemClipboard(std::string stringToPlaceOnClip, LPCWSTR lpszFormat);
+        void CopyTextToSystemClipboard(const std::wstring_view text, const std::optional<std::string> htmlData, const std::optional<std::string> rtfData) const;
+        void CopyToSystemClipboard(const std::string_view stringToPlaceOnClip, LPCWSTR lpszFormat) const;


@lhecker you're gonna conflict this, and because of that conflict I won't be able to easily backport to 1.19; thoughts? Want to maintain two copies of that PR?

I'd be happy to split my PR into two to adapt for this one. 🙂

DHowett · 2023-12-12T19:10:49Z

src/host/selectionState.cpp

+{
+    if (!_fSelectionVisible)
+    {
+        return {};


can we distinguish this from there being a single selection at 0,0? do we need to?

For consistency, we should return something like { { 0, 0 }, { -1, -1 } } to represent an empty selection. Fixed.

This also helped me find a bug in the possible range for CopyRequest's end coordinate. Since it's an inclusive coordinate we should be clamping it within { buffer.width - 1, buffer.height - 1 } ( and not { buffer.width, buffer.height }). This is fixed too.

Thanks.

DHowett · 2023-12-12T19:11:49Z

src/cascadia/TerminalCore/Terminal.hpp

+        std::optional<std::string> html;
+        std::optional<std::string> rtf;


i wonder. it should be a fair assumption that if the string is empty, it didn't exist. do we need the optional?

(Not asking you to change this, just curious)

That depends on user expectations. If I select the empty space after the prompt, I'd expect it to copy an empty string (which got trimmed) and put that into the clipboard. Also, the UI behaves like something is copied. (User has an active selection -> triggers copy action -> selection goes away -> ctrl+v -> should paste empty string). Now, if we couldn't select the empty spaces then this would be a different story 😅

That case is probably fine even if "empty string" means "not present"! Because the framing for the HTML and RTF documents always includes data (the DOCTYPE/Clipboard header or the RTF preamble), selecting and copying the empty space after the prompt wouldn't result in an empty string.

the HTML and RTF documents always includes data (the DOCTYPE/Clipboard header or the RTF preamble)

Sorry, I missed that part completely.

That means we can use an empty string as an empty RTF/HTML data. 😄

Except, we do return an empty string from the three text generation functions, but only when we get a CopyRequest with req.beg > req.end i.e. empty selection range (possibly unreal). Since it's an empty selection, it should be okay to return an empty string I guess.

Should I replace it with assert calls to convey that the req is ill-formed? WDYT?

Another option is to remove the special case handling and let them fill the std::string with just HTML/RTF header. (Have to test whether this works fine or not)

I am A-OK with us leaving the optionals, btw! It does make it a lot clearer.

Sure, let's go ahead with std::optional approach 👍

src/cascadia/TerminalCore/TerminalSelection.cpp

DHowett · 2023-12-12T19:15:25Z

src/cascadia/TerminalCore/TerminalSelection.cpp

+    }
+
+    const auto bgColor = _renderSettings.GetAttributeColors({}).second;
+    const auto isIntenseBold = _renderSettings.GetRenderMode(::Microsoft::Console::Render::RenderSettings::Mode::IntenseIsBold);


hey, we didn't handle this properly before did we? nice!

src/buffer/out/textBuffer.cpp

DHowett · 2023-12-12T19:42:10Z

src/buffer/out/textBuffer.cpp


            // note: MS Word doesn't support padding (in this way at least)
-            htmlBuilder << "padding:";
-            htmlBuilder << 4; // todo: customizable padding


why on earth did we format this in as a number lol

src/buffer/out/textBuffer.cpp

DHowett · 2023-12-12T19:46:01Z

src/buffer/out/textBuffer.cpp

-                };
-
-                if (rows.text.at(row).at(col) == '\r' || rows.text.at(row).at(col) == '\n')
+                const auto nextX = gsl::narrow_cast<uint16_t>(x + length);


i forget, are attribute lengths associated with code units or with columns? Will this cause problems for e.g. single-width surrogate pair characters like U+1F574 MAN IN BUSINESS SUIT LEVITATING?

Attributes operate in columns. The ROW::_charOffsets array then maps columns back to code units.

src/buffer/out/textBuffer.cpp

DHowett · 2023-12-12T19:51:50Z

@tusharsnx you're taking on some of the scary parts of our codebase, and I've gotta commend you for that! Thank you!

DHowett · 2023-12-13T16:12:29Z

(Can't leave a diff comment on mobile!)

Ah, I was thinking we should keep the case labels for the SinglyUnderlined cases but place them directly above/in the fallthrough position for default. Otherwise, static analyzers might think that we didn't cover all the cases :)

lhecker

I can't really see any flaws whatsoever. If there are any: Let's fix them once we ever find any! Fantastic work!

I've commented a couple nits, but they're not that important.

src/types/UiaTextRangeBase.cpp

src/interactivity/win32/Clipboard.cpp

src/host/selectionState.cpp

DHowett · 2023-12-15T23:19:57Z

I'll leave off my review in case you want to think about Leonard's comments! I'll be around tomorrow or so to check up. 😄

tusharsnx · 2024-01-25T08:05:09Z

Resolved merge conflicts generated by #16480

DHowett · 2024-01-29T21:53:27Z

I didn't realize this one didn't merge! And it's blocked on ME??

tusharsnx added 26 commits November 6, 2023 23:43

make full TextAttribute accessible to GenHTML and GenRTF

159db72

remove extra counter

bf1c1ce

fix spelling

db8e4c7

avoid decrementing attrRleSize when not copying attributes

55765bc

rename copyTextColor

4e2f981

revert StringPaste change

88b9926

use RLE's backing runs vector for efficient looping

5cfd18c

fix audit mode errors

436d456

fix a bug that I almost thought was an intended behavior

901c12a

we were adding newlines after every row! Causing wrapped rows to break unexpectedly

Merge remote-tracking branch 'upstream/main' into copypasta

aa53564

copy font properties in HTML copy

0a2f187

copy text attribute properties in RTF copy

315f35f

remove redundant comment text

06a5536

add spellings

e2e121e

fix mishandling of unicode characters in RTF copy

5fad9e6

use nested span for applying overline, strikethrough, underline with …

b60609d

…different colors

format

728d287

remove extraneous std::move

490f24f

initial commit

a532217

remove refs and move values instead

55e0cf0

replace std::ostringstream with std::string in GenHTML

6cdb80c

remove an unintended copy op

cf5967f

replace std::ostringstream with std::string in GenRTF

4f07166

format

da86324

Merge remote-tracking branch 'upstream/main' into copypasta2

ce7937b

fix comments

0f2b994

microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. labels Nov 26, 2023

fix spell checking

7e6901e

tusharsnx marked this pull request as ready for review December 7, 2023 09:24

DHowett requested changes Dec 12, 2023

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 12, 2023

microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 13, 2023

tusharsnx added 3 commits December 13, 2023 14:19

address feedback

6e902c9

fix CopyRequest end coordinate range

1555bf0

use correct anchors for empty selection

78321d8

add singly underlined case inside switch case

316de6c

DHowett requested review from lhecker and DHowett December 13, 2023 19:05

lhecker approved these changes Dec 15, 2023

View reviewed changes

src/types/UiaTextRangeBase.cpp Show resolved Hide resolved

src/interactivity/win32/Clipboard.cpp Outdated Show resolved Hide resolved

src/host/selectionState.cpp Outdated Show resolved Hide resolved

tusharsnx added 2 commits December 16, 2023 12:43

conhost selection anchors are always ordered (start <= end)

74948bb

use string empty-ness as a way to detect no data for that format

0367e7f

lhecker approved these changes Dec 16, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into copypasta2

ea2c125

DHowett approved these changes Jan 29, 2024

View reviewed changes

DHowett changed the title ~~Refactor TextBuffer::GenHTML and TextBuffer::GenRTF to read directly from the buffer~~ Refactor TextBuffer::GenHTML/RTF to read the buffer directly Jan 29, 2024

DHowett enabled auto-merge (squash) January 29, 2024 21:57

DHowett merged commit a3ac337 into microsoft:main Jan 29, 2024
14 of 15 checks passed

tusharsnx deleted the copypasta2 branch March 18, 2024 15:17

DHowett mentioned this pull request Aug 13, 2024

Introduce the concept of "selection spans" instead of "rects" #17638

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `TextBuffer::GenHTML/RTF` to read the buffer directly #16377

Refactor `TextBuffer::GenHTML/RTF` to read the buffer directly #16377

tusharsnx commented Nov 26, 2023 •

edited

Loading

tusharsnx commented Nov 26, 2023

tusharsnx commented Dec 7, 2023 •

edited

Loading

lhecker commented Dec 11, 2023

tusharsnx commented Dec 12, 2023

DHowett left a comment

DHowett Dec 12, 2023

lhecker Dec 12, 2023

DHowett Dec 12, 2023

tusharsnx Dec 13, 2023 •

edited

Loading

DHowett Dec 12, 2023

tusharsnx Dec 13, 2023

DHowett Dec 13, 2023

tusharsnx Dec 13, 2023 •

edited

Loading

tusharsnx Dec 13, 2023 •

edited

Loading

tusharsnx Dec 13, 2023

DHowett Dec 13, 2023

tusharsnx Dec 13, 2023

DHowett Dec 12, 2023

DHowett Dec 12, 2023

DHowett Dec 12, 2023

lhecker Dec 12, 2023 •

edited

Loading

DHowett commented Dec 12, 2023

DHowett commented Dec 13, 2023

lhecker left a comment

DHowett commented Dec 15, 2023

tusharsnx commented Jan 25, 2024

DHowett commented Jan 29, 2024

		std::optional<std::string> html;
		std::optional<std::string> rtf;

Refactor TextBuffer::GenHTML/RTF to read the buffer directly #16377

Refactor TextBuffer::GenHTML/RTF to read the buffer directly #16377

Conversation

tusharsnx commented Nov 26, 2023 • edited Loading

tusharsnx commented Nov 26, 2023

tusharsnx commented Dec 7, 2023 • edited Loading

lhecker commented Dec 11, 2023

tusharsnx commented Dec 12, 2023

DHowett 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

tusharsnx Dec 13, 2023 • edited Loading

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

tusharsnx Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

tusharsnx Dec 13, 2023 • edited Loading

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

lhecker Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

DHowett commented Dec 12, 2023

DHowett commented Dec 13, 2023

lhecker left a comment

Choose a reason for hiding this comment

DHowett commented Dec 15, 2023

tusharsnx commented Jan 25, 2024

DHowett commented Jan 29, 2024

Refactor `TextBuffer::GenHTML/RTF` to read the buffer directly #16377

Refactor `TextBuffer::GenHTML/RTF` to read the buffer directly #16377

tusharsnx commented Nov 26, 2023 •

edited

Loading

tusharsnx commented Dec 7, 2023 •

edited

Loading

tusharsnx Dec 13, 2023 •

edited

Loading

tusharsnx Dec 13, 2023 •

edited

Loading

tusharsnx Dec 13, 2023 •

edited

Loading

lhecker Dec 12, 2023 •

edited

Loading