Use DECCRA/DECFRA for ScrollConsoleScreenBuffer #17747

lhecker · 2024-08-20T14:10:48Z

This adds logic to get the DA1 report from the hosting terminal on
startup. We then use the information to figure out if it supports
rectangular area operations. If so, we can use DECCRA/DECFRA to
implement ScrollConsoleScreenBuffer.

This additionally changes ScrollConsoleScreenBuffer to always
forbid control characters as the fill character, even in conhost
(via VtIo::SanitizeUCS2). My hope is that this makes the API
more consistent and robust as it avoids another source for
invisible control characters in the text buffer.

Part of #17643

Validation Steps Performed

New tests ✅

DHowett

I've given it, and the tests, a close read. I've also tested it with vim (set termguicolors | color kolor) in Terminal (supports rectangular areas) and WezTerm (does not support rectangular areas!).

It's better than what we have, and I'm excited to get it out in Canary.

DHowett · 2024-08-20T22:00:35Z

src/host/getset.cpp

+                    Microsoft::Console::VirtualTerminal::VtIo::FormatAttributes(buf, TextAttribute{ fillAttribute });
+                }
+
+                if (copySourceViewport.IsValid() && copyTargetViewport.IsValid())


If we bail out here, is there still something we could have done by manual-copy? e.g. if it's not representable with DECCRA, is there still a potential fallback?

No, this just means that there was nothing to copy because either the source or target rects are completely out of bounds and so there's nothing to copy.

DHowett · 2024-08-20T22:04:17Z

src/host/getset.cpp

-            const auto h = std::max(0, sourceViewport.Height());
-            const auto a = static_cast<size_t>(w * h);
-            if (a == 0)
+            writer.BackupCursor();


DECCRA and DECFRA are documented as not changing the active cursor position. We may be able to skip backing up the cursor in the DECxRA cases

Oh, backing up the cursor also backs up the active attributes. We use those in DECFRA. Got it.

DHowett · 2024-08-20T22:04:47Z

src/host/getset.cpp

            {
-                return S_OK;
+                // This calculates just the positive offsets caused by out-of-bounds source and target coordinates.
+                // The source gets offset because it's outside the bounds of the buffer (= negative),


this might benefit from some ASCII art. I still don't understand it

If the source starts outside the buffer, we want to offset it by +source. if the target starts outside the clip region, we want to offset something by something.

I'm surprised we can apply the same offset to both the source and target!

I believe there's no good way to draw this. It's a logic that I came up with after noodling over the problem for a while.
Edit: Much better comment now.

DHowett · 2024-08-20T22:07:05Z

src/host/getset.cpp

+
+                const auto copyTargetViewport = Viewport::FromDimensions(target + offset, sourceViewport.Dimensions()).Clamp(clipViewport);
+                const auto copySourceViewport = Viewport::FromDimensions(sourceViewport.Origin() + offset, copyTargetViewport.Dimensions()).Clamp(bufferSize);
+                const auto fills = Viewport::Subtract(fillViewport, copyTargetViewport);


"SOME RECTS" strikes again

src/host/ut_host/VtIoTests.cpp

…ct-deccra

j4james · 2024-08-21T22:57:45Z

src/terminal/parser/InputStateMachineEngine.cpp

+            // The first parameter denotes the conformance level, so we skip it.
+            parameters.subspan(1).for_each([&](auto p) {
+                attributes.set(static_cast<DeviceAttribute>(p));
+                return true;
+            });


In practice this probably won't matter, but strictly speaking we should be checking that the first parameter is 60+, otherwise we can't assume that we're getting a list of features here. When the first parameter is less than 60, the parameters that follow take on a different meaning.

Should I check for >=60 or >=61? Documentation and comments that I found here imply that all terminals that support DA1 are at least 61... Edit: I used >=61 for now.

Using >=61 is fine, but I don't think it would matter either way. In STD070 they document the range 61 to 69 as representing levels 1 to 9. But they also go on say that values 1 to 59 are device specific codes, and that the new encoding scheme applies to values 60 or greater. So it's a bit ambiguous, but it's highly unlikely we'll ever encounter a terminal with the value 60 either way.

And while we're getting into technicalities, I should mention that certain features are supposed to be implicit at higher levels. In particular level 4+ devices are supposed to support the rectangular editing operations, but won't advertise feature 28 explicitly. Unfortunately modern terminals tend to lie about their conformance level, so we can't trust that value, and we're better off assuming no support unless feature 28 is explicitly listed.

zadjii-msft

12/14 I read closely, and getset.cpp looks... good enough. I'm out of time to give it a close enough review but i'm guessing we can ship this

zadjii-msft · 2024-08-22T21:50:33Z

src/terminal/parser/InputStateMachineEngine.cpp

+        // This assumes that InputStateMachineEngine is tightly coupled with VtInputThread and the rest of the ConPTY system.
+        // On startup, ConPTY will send a DA1 request to get more information about the hosting terminal.
+        // We catch it here and store the information for later retrieval.
+        if (_deviceAttributes.load(std::memory_order_relaxed) == 0)


if we get this while we're _lookingForDSR, but we don't get a DSR, should we disable looking for the cursor at that point?

Oh! I forgot about that!
Thanks!

…ct-deccra

zadjii-msft

Okay I wouldn't have been able to wrap my head around this without diagrams, thanks for that!

zadjii-msft · 2024-08-23T19:55:22Z

src/host/ut_host/VtIoTests.cpp

@@ -554,12 +557,224 @@ class ::Microsoft::Console::VirtualTerminal::VtIoTests
        actual = readOutput();
        VERIFY_ARE_EQUAL(expected, actual);

+        // Copying from a partially out-of-bounds source to a partially out-of-bounds target,


i friggin love me some diagrams

I tried to write a diagram but failed to make it readable & obvious since it requires 3 overlapping rectangles. Instead, I tried to improve the verbal description.

I'll merge for now since we're short on time.

Oh, I thought this was getset.cpp haha. Thank you in this case!

zadjii-msft · 2024-08-23T19:58:53Z

src/host/getset.cpp

+                    // DECCRA: Copy Rectangular Area
+                    fmt::format_to(
+                        std::back_inserter(buf),
+                        FMT_COMPILE(L"\x1b[{};{};{};{};;{};{}$v"),


CSI 9/11 Pts; Pls; Pbs; Prs; Pps; Ptd; Pld; Ppd $ v area to be copied destination

The omitted one here is:
Pps is the number of the page where the rectangular area is located. Default: Pps = 1.

and that makes sense

Use DECCRA/DECFRA for ScrollConsoleScreenBuffer

2f48b91

lhecker added the Product-Conpty For console issues specifically related to conpty label Aug 20, 2024

lhecker linked an issue Aug 20, 2024 that may be closed by this pull request

Known unresolved issues after removing VtEngine / adding VT passthrough #17643

Open

lhecker removed a link to an issue Aug 20, 2024

Known unresolved issues after removing VtEngine / adding VT passthrough #17643

Open

Stop complaining, it works

25a0376

DHowett approved these changes Aug 20, 2024

View reviewed changes

lhecker added 2 commits August 21, 2024 16:06

Revert test properties

bd0104e

Much better comment

2797fe0

DHowett approved these changes Aug 21, 2024

View reviewed changes

lhecker added 2 commits August 22, 2024 00:30

Merge remote-tracking branch 'origin/main' into dev/lhecker/scroll-re…

c7c52ef

…ct-deccra

Fixed tests, Slighly improved comment

0a619dc

DHowett approved these changes Aug 21, 2024

View reviewed changes

j4james reviewed Aug 21, 2024

View reviewed changes

lhecker added 2 commits August 22, 2024 01:18

Check device conformance level

f287e85

Delete Copilot hallucination

0c52c0d

zadjii-msft added this to the Terminal v1.22 milestone Aug 22, 2024

zadjii-msft reviewed Aug 22, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into dev/lhecker/scroll-re…

8f9030c

…ct-deccra

DHowett approved these changes Aug 23, 2024

View reviewed changes

zadjii-msft approved these changes Aug 23, 2024

View reviewed changes

lhecker merged commit 040f261 into main Aug 23, 2024
20 checks passed

lhecker deleted the dev/lhecker/scroll-rect-deccra branch August 23, 2024 20:02

j4james mentioned this pull request Aug 26, 2024

Some ScrollConsoleScreenBuffer directions have stopped working correctly #17801

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use DECCRA/DECFRA for ScrollConsoleScreenBuffer #17747

Use DECCRA/DECFRA for ScrollConsoleScreenBuffer #17747

lhecker commented Aug 20, 2024

DHowett left a comment

DHowett Aug 20, 2024

lhecker Aug 20, 2024

DHowett Aug 20, 2024

DHowett Aug 20, 2024

DHowett Aug 20, 2024

DHowett Aug 20, 2024

DHowett Aug 20, 2024

lhecker Aug 21, 2024 •

edited

Loading

DHowett Aug 20, 2024

j4james Aug 21, 2024

lhecker Aug 21, 2024 •

edited

Loading

j4james Aug 22, 2024

zadjii-msft left a comment

zadjii-msft Aug 22, 2024

lhecker Aug 23, 2024

zadjii-msft left a comment

zadjii-msft Aug 23, 2024

lhecker Aug 23, 2024

lhecker Aug 23, 2024

zadjii-msft Aug 23, 2024

Use DECCRA/DECFRA for ScrollConsoleScreenBuffer #17747

Use DECCRA/DECFRA for ScrollConsoleScreenBuffer #17747

Conversation

lhecker commented Aug 20, 2024

Validation Steps Performed

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

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 Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Aug 21, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

lhecker Aug 21, 2024 •

edited

Loading

lhecker Aug 21, 2024 •

edited

Loading