Add support for selective erase operations #14046

j4james · 2022-09-20T17:57:43Z

This PR adds support for the selective erase escape sequences: DECSED,
DECSEL, and DECSCA. They provide a way of marking certain areas of
the screen as "protected", so you can erase the content everywhere else
without affecting those protected areas.

This adds another bit in the CharacterAttributes enum to track the
protected status of each cell, and an operation triggered by the
DECSCA sequence which can toggle that bit in the active attributes.

There there are two new erase operations triggered by the DECSED and
DECSEL sequences, which work similar to the existing ED (erase in
display) and EL (erase in line) operations, but which only apply to
unprotected cells.

I've also updated the DECRQSS settings request, so you can query the
active protected attribute status.

Validation Steps Performed

I've manually confirmed that we pass the selective erase tests in Vttest
now, and I've also manually tested some more complicated edge cases and
confirmed that we match the behavior of the hardware VT240 emulator in
MAME.

For unit testing I've extended the existing erase tests to cover
selective erase as an additional option, I've added a test covering the
DECSCA sequence, and I've extended the DECRQSS adapter test to
confirm the attribute reporting is working.

Closes #14029

j4james · 2022-09-20T19:28:23Z

src/terminal/adapter/adaptDispatch.cpp

+            auto& rowBuffer = textBuffer.GetRowByOffset(row);
+            const auto& attrs = rowBuffer.GetAttrRow();
+            auto& chars = rowBuffer.GetCharRow();
+            for (auto col = eraseRect.left; col < eraseRect.right; col++)
+            {
+                // Only unprotected cells are affected.
+                if (!attrs.GetAttrByColumn(col).IsProtected())
+                {
+                    // The text is cleared but the attributes are left as is.
+                    chars.ClearGlyph(col);
+                    textBuffer.TriggerRedraw(Viewport::FromCoord({ col, row }));
+                }
+            }


I realise this bit will likely have to be rewritten somewhat to work with the new ROW implementation in PR #13626, so I'm happy to wait for that to merge first.

ROW is going to take much longer than this PR to merge, so I'll ask Leonard to do that part 😄 Thanks though!

DHowett

Beautiful and straightforward code. Happy to merge this before ROW.

Noted: selective erase won't pass over ConPTY today--nor will protection--but due to the way ConPTY renders output it will technically work fine. Interesting!

DHowett · 2022-09-20T19:42:41Z

src/buffer/out/TextAttribute.cpp

 void TextAttribute::SetDefaultRenditionAttributes() noexcept
 {
-    _attrs = CharacterAttributes::Normal;
+    _attrs &= CharacterAttributes::Protected;


Should we introduce a metaconstant for the rendition attributes? or should we just defer that until we have more non-rendition attributes... :)

I don't know. If we did, it would be everything but Protected, would seems like a pain to maintain. We could go the other way around and create a metaconstant for the "logical" attributes, but that would just be Protected which feels a bit pointless. I'd say defer it until have more non-rendition attributes, but realistically that'll probably be never.

DHowett · 2022-09-20T19:47:18Z

src/terminal/adapter/adaptDispatch.cpp

+            auto& rowBuffer = textBuffer.GetRowByOffset(row);
+            const auto& attrs = rowBuffer.GetAttrRow();
+            auto& chars = rowBuffer.GetCharRow();
+            for (auto col = eraseRect.left; col < eraseRect.right; col++)
+            {
+                // Only unprotected cells are affected.
+                if (!attrs.GetAttrByColumn(col).IsProtected())
+                {
+                    // The text is cleared but the attributes are left as is.
+                    chars.ClearGlyph(col);
+                    textBuffer.TriggerRedraw(Viewport::FromCoord({ col, row }));
+                }
+            }


ROW is going to take much longer than this PR to merge, so I'll ask Leonard to do that part 😄 Thanks though!

DHowett · 2022-09-20T19:49:15Z

src/terminal/adapter/adaptDispatchGraphics.cpp

+        switch (opt)
+        {
+        case Default:
+            attr.SetProtected(false);
+            break;
+        case Protected:
+            attr.SetProtected(true);
+            break;
+        case Unprotected:
+            attr.SetProtected(false);
+            break;
+        }


Hmm. I like how clear this is, but it makes me wonder if it shouldn't be attr.SetProtected(opt == Protected) for some reason. I have no reason to request it specifically other than aesthetics, so f.f. to ignore :)

When opt is something other than those three options it shouldn't have any effect, so that won't work.

Ah, of course. Thanks!

DHowett · 2022-09-20T19:52:29Z

Also noted: part of making it transit ConPTY (if we were so inclined) would be to make triply sure the (ever-problematic) optimization to avoid sending empty spaces didn't optimize out a protection attribute :)

j4james · 2022-09-20T20:07:56Z

Also noted: part of making it transit ConPTY (if we were so inclined) would be to make triply sure the (ever-problematic) optimization to avoid sending empty spaces didn't optimize out a protection attribute :)

The protected attributes don't need to pass over conpty, because all of the logic is happening on the conhost side (edit: I see now you already noted that in the comment above).

Although now that I think about it, it's possible the existence of a protected attribute might prevent spaces being optimized where they potentially could have been, but that shouldn't break anything. I have tested this in Windows Terminal, and haven't seen any problems.

Edit: Looking at the HasIdenticalVisualRepresentationForBlankSpace implementation here:

terminal/src/buffer/out/TextAttribute.hpp

Lines 143 to 152 in 1ce22a8

    
           return !IsAnyGridLineEnabled() && // grid lines have a visual representation 
        
                  // crossed out, doubly and singly underlined have a visual representation 
        
                  WI_AreAllFlagsClear(_attrs, CharacterAttributes::CrossedOut | CharacterAttributes::DoublyUnderlined | CharacterAttributes::Underlined) && 
        
                  // hyperlinks have a visual representation 
        
                  !IsHyperlink() && 
        
                  // all other attributes do not have a visual representation 
        
                  _attrs == other._attrs && 
        
                  ((checkForeground && _foreground == other._foreground) || 
        
                   (!checkForeground && _background == other._background)) && 
        
                  IsHyperlink() == other.IsHyperlink();

Technically that _attrs == other._attrs test could potentially mask out the protected attribute, but I'm not sure I want to introduce any additional complexity here to optimise what is likely an uncommon edge case.

ghost · 2022-09-20T21:55:37Z

Hello @DHowett!

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.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 4 hours 2 minutes. No worries though, I will be back when the time is right! 😉

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.

lhecker

I'm ok with this PR. I'm just delaying my approval in case you wish to change anything. Otherwise I'll approve it anyways.

lhecker · 2022-09-21T23:24:39Z

src/terminal/adapter/adaptDispatch.cpp

+// - <none>
+void AdaptDispatch::_SelectiveEraseRect(TextBuffer& textBuffer, const til::rect& eraseRect)
+{
+    if (eraseRect.left < eraseRect.right && eraseRect.top < eraseRect.bottom)


You could just write if (eraseRect) here. The operator bool does exactly this, plus it ensures that all coordinates are >= 0, which seems to me like something we might want in this code.

Oh that's neat. I'll definitely do that.

lhecker · 2022-09-21T23:28:10Z

src/terminal/adapter/adaptDispatchGraphics.cpp

+        switch (opt)
+        {
+        case Default:
+            attr.SetProtected(false);
+            break;
+        case Protected:
+            attr.SetProtected(true);
+            break;
+        case Unprotected:
+            attr.SetProtected(false);
+            break;
+        }


But what about this?

switch (opt) { case Default: case Unprotected: // <-- deduplicated attr.SetProtected(false); break; case Protected: attr.SetProtected(true); break; default: // <-- explicit (maybe?) break; }

I did consider that, but the reason I left the Default separate from the Unprotected was because semantically they're actually different. Default serves the same purpose as SGR 0 does for rendition attributes - it resets all of the logical attributes. For now that just means resetting the protected attribute, because that's all there is, but if we ever added others, those two cases would definitely be different. I thought it better to get the structure right up front rather than trying to over optimise it.

christianparpart · 2022-09-24T20:07:49Z

src/terminal/adapter/adaptDispatchGraphics.cpp

+{
+    auto& textBuffer = _api.GetTextBuffer();
+    auto attr = textBuffer.GetCurrentAttributes();
+    for (size_t i = 0; i < options.size(); i++)


Out of personal curiosity, when looking at the spec at https://vt100.net/docs/vt510-rm/DECSCA.html it looks like DECSCA accepts one single numeric parameter but you iterate over every one (from zero upwards?). Every subsequent parameter will override its predecessor. I can't find that in the linked spec. Why?

@christianparpart It's documented in DEC STD-070 on page 5-166 (I can get there directly in my copy of the PDF by selecting page 421). The sequence was designed to be extendable with additional attribute types similar to SGR. So just like you can have \e[1;22m turning bold on and off again, you can have \e[1;2"q turning the protected attribute on and off again.

And just like \e[0m turns off all rendition attributes, \e[0"q was actually intended to turn off all logical attributes. It's just that in this case that's effectively the same as \e[2q, because they never got around to defining any other logical attributes (at least as far as I'm aware).

Edit: I should also mention that everyone gets this wrong, so I wouldn't stress too much about it.

Thanks for that clarification and reference.

carlos-zamora · 2022-10-07T16:46:20Z

I'm assuming this is ready to merge.

@msftbot merge this in 10 minutes

EDIT: huh, looks like the bot isn't working? Merging!

ghost · 2023-01-24T18:19:46Z

🎉Windows Terminal Preview v1.17.1023 has been released which incorporates this pull request.:tada:

Handy links:

j4james added 4 commits September 20, 2022 17:00

Add support for the DECSCA sequence.

f19599f

Report DECSCA state with DECRQSS.

4587f4d

Add support for DECSED and DECSEL sequences.

5b9f548

Extend units tests to cover selective erasure.

6d462be

ghost added Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase labels Sep 20, 2022

Add missing overrides.

78ec87d

j4james marked this pull request as ready for review September 20, 2022 19:26

j4james commented Sep 20, 2022

View reviewed changes

DHowett approved these changes Sep 20, 2022

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 20, 2022

lhecker reviewed Sep 21, 2022

View reviewed changes

Make the most of the til::rect bool operator.

22b1296

ghost removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 22, 2022

lhecker approved these changes Sep 22, 2022

View reviewed changes

christianparpart reviewed Sep 24, 2022

View reviewed changes

j4james mentioned this pull request Oct 1, 2022

Add support for the rectangular area operations #14112

Closed

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 7, 2022

carlos-zamora merged commit 657dd5f into microsoft:main Oct 7, 2022

j4james deleted the feature-selective-erase branch October 11, 2022 08:54

alexrp mentioned this pull request Oct 15, 2022

Support DECSCA and DECSED/DECSEL vezel-dev/cathode#99

Closed

zadjii-msft mentioned this pull request Nov 11, 2022

Theme-controlled color scheme switch #4066

Closed

ghost mentioned this pull request Jan 24, 2023

Add support for selective erase operations #14029

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for selective erase operations #14046

Add support for selective erase operations #14046

j4james commented Sep 20, 2022 •

edited by DHowett

Loading

j4james Sep 20, 2022

DHowett Sep 20, 2022

DHowett left a comment

DHowett Sep 20, 2022

j4james Sep 20, 2022

DHowett Sep 20, 2022

DHowett Sep 20, 2022

j4james Sep 20, 2022

DHowett Sep 20, 2022

DHowett commented Sep 20, 2022

j4james commented Sep 20, 2022 •

edited

Loading

ghost commented Sep 20, 2022

lhecker left a comment

lhecker Sep 21, 2022 •

edited

Loading

j4james Sep 22, 2022

lhecker Sep 21, 2022

j4james Sep 22, 2022

christianparpart Sep 24, 2022

j4james Sep 24, 2022 •

edited

Loading

christianparpart Sep 24, 2022

carlos-zamora commented Oct 7, 2022 •

edited

Loading

ghost commented Jan 24, 2023

Add support for selective erase operations #14046

Add support for selective erase operations #14046

Conversation

j4james commented Sep 20, 2022 • edited by DHowett Loading

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

DHowett commented Sep 20, 2022

j4james commented Sep 20, 2022 • edited Loading

ghost commented Sep 20, 2022

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.

lhecker left a comment

Choose a reason for hiding this comment

lhecker Sep 21, 2022 • 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

j4james Sep 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora commented Oct 7, 2022 • edited Loading

ghost commented Jan 24, 2023

j4james commented Sep 20, 2022 •

edited by DHowett

Loading

j4james commented Sep 20, 2022 •

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.

lhecker Sep 21, 2022 •

edited

Loading

j4james Sep 24, 2022 •

edited

Loading

carlos-zamora commented Oct 7, 2022 •

edited

Loading