Add support for passing through extended text attributes, like… #2917

zadjii-msft · 2019-09-26T17:10:17Z

Summary of the Pull Request

Adds support for Italics, Blinking, Invisible, CrossedOut text, THROUGH CONPTY. This does NOT add support for those styles to conhost or the terminal.

We will store these "Extended Text Attributes" in a TextAttribute. When we go to render a line, we'll see if the state has changed from our previous state, and if so, we'll appropriately toggle that state with VT. Boldness has been moved from a bool to a single bit in these flags.

Technically, now that these are stored in the buffer, we only need to make changes to the renderers to be able to support them. That's not being done as a part of this PR however.

References

See also #2915 and #2916, which are some follow-up tasks from this fix. I thought them too risky for 20H1.

PR Checklist

Closes ConPTY swallows SGR 3 and 23, so terminals like Alacritty are prevented from supporting italics #2554
I work here
Tests added/passed
[n/a] Requires documentation to be updated

Validation Steps Performed

Replaced the conhost in my system32 and tried running the following script, via WSL interop:

import sys
import time # time.sleep is in seconds
def write(s):
    sys.stdout.write(s)

def csi(seq):
    sys.stdout.write('\x1b[{}'.format(seq))

def sgr(code=0):
    csi('{}m'.format(code))

def cupxy(x=0, y=0):
    cup(y+1, x+1)

def clear_all():
    cupxy(0,0)
    csi('2J')

# $ python ~/vttests/extended-attrs.py
if __name__ == '__main__':
    clear_all()
    print('This is the VT Test template.')

    sgr(0)
    write('[Normal Text]')
    sgr(3)
    write('[Italicized]')
    sgr(0)
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(4)
    write('[Underlined]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(4)
    write('[Underlined]')
    sgr(3)
    write('[U & I]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(4)
    write('[Underlined]')
    sgr(3)
    write('[U & I]')
    sgr(23)
    write('[Underlined]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(4)
    write('[Underlined]')
    sgr(3)
    write('[U & I]')
    sgr(24)
    write('[Italicized]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(5)
    write('[Blinking]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(8)
    write('[Invisible]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(9)
    write('[Crossed-out]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

    ############################################################################


    sgr(0)
    write('[Normal Text]')
    sgr(1)
    write('[Bold]')
    sgr(3)
    write('[B & Italicized]')
    sgr(0)
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(1)
    write('[Bold]')
    sgr(4)
    write('[B & Underlined]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(1)
    write('[Bold]')
    sgr(4)
    write('[B & Underlined]')
    sgr(3)
    write('[B & U & I]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(1)
    write('[Bold]')
    sgr(4)
    write('[B & Underlined]')
    sgr(3)
    write('[U & I]')
    sgr(23)
    write('[B & Underlined]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(1)
    write('[Bold]')
    sgr(4)
    write('[B & Underlined]')
    sgr(3)
    write('[B & U & I]')
    sgr(24)
    write('[B & Italicized]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(1)
    write('[Bold]')
    sgr(5)
    write('[B & Blinking]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(1)
    write('[Bold]')
    sgr(8)
    write('[B & Invisible]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

    sgr(0)
    write('[Normal Text]')
    sgr(1)
    write('[Bold]')
    sgr(9)
    write('[B & Crossed-out]')
    sgr(0)
    write('[Normal Again]')
    write('\n')

ran tests
Using the (added) static assert, ensured that the size of the TextAttribute did not increase.

src/renderer/vt/VtSequences.cpp

src/renderer/vt/Xterm256Engine.cpp

DHowett-MSFT · 2019-09-26T17:41:39Z

src/renderer/vt/Xterm256Engine.cpp

+
+    auto hr = updateFlagAndState(ExtendedAttributes::Italics,
+                                 _usingItalics,
+                                 std::bind(&Xterm256Engine::_BeginItalics, this),


binds are sooooooooo expensive. :|

i wonder if there's a trick you can do with auto-typed args in lambdas

If we have a particular qualm with std::bind, we should probably file a code health work item with the proposed replacement pattern for them. There's a ton of existing ones.

I just did a cursory find-all for std::bind, and it didn't look like there were any other perf-intensive (loops) places where we did a bind, usually only on startup. @DHowett-MSFT think we should still file a follow-up? I was able to remove this one at least.

src/terminal/adapter/DispatchTypes.hpp

DHowett-MSFT · 2019-09-26T17:46:57Z

src/terminal/adapter/adaptDispatchGraphics.cpp


                i += (cOptionsConsumed - 1); // cOptionsConsumed includes the opt we're currently on.
            }
            else
            {
                _SetGraphicsOptionHelper(opt, &attr);
-                fSuccess = !!_conApi->PrivateSetLegacyAttributes(attr, _fChangedForeground, _fChangedBackground, _fChangedMetaAttrs);
+                fSuccess = !!_conApi->PrivateSetLegacyAttributes(attr,


this won't stomp ext attrs, will it?

src/host/ut_host/ScreenBufferTests.cpp

src/host/getset.cpp

src/host/outputStream.hpp

src/host/ut_host/ScreenBufferTests.cpp

src/renderer/vt/WinTelnetEngine.cpp

src/renderer/vt/Xterm256Engine.cpp

miniksa · 2019-09-26T18:15:20Z

src/renderer/vt/Xterm256Engine.cpp

+
+    auto hr = updateFlagAndState(ExtendedAttributes::Italics,
+                                 _usingItalics,
+                                 std::bind(&Xterm256Engine::_BeginItalics, this),


If we have a particular qualm with std::bind, we should probably file a code health work item with the proposed replacement pattern for them. There's a ton of existing ones.

src/terminal/adapter/adaptDispatchGraphics.cpp

DHowett-MSFT · 2019-09-30T22:50:55Z

/azp run

azure-pipelines · 2019-09-30T22:51:04Z

Azure Pipelines successfully started running 1 pipeline(s).

DHowett-MSFT · 2019-10-01T17:47:05Z

You may need to merge in master to get the CI pipeline repaired.

…italics_ # Conflicts: # src/host/ut_host/ScreenBufferTests.cpp

DHowett-MSFT

hot dang

carlos-zamora

Just a few minor things.

src/buffer/out/TextAttribute.hpp

tools/bcz.cmd

src/renderer/vt/Xterm256Engine.hpp

src/renderer/vt/Xterm256Engine.cpp

miniksa

A few more comments

src/buffer/out/TextAttribute.hpp

miniksa · 2019-10-03T20:51:39Z

src/renderer/vt/Xterm256Engine.cpp

+                                 &Xterm256Engine::_EndItalics);
+    RETURN_IF_FAILED(hr);
+
+    hr = updateFlagAndState(ExtendedAttributes::Blinking,


Nnnn... what happens in the scenario where:

We're attempting to set Italics, Blink, and Invisible.

Italics succeeds

Blink fails

Invisible would have succeeded, but we early returned...

Are we OK with that happening or should we have a provision to try the others and aggregate the result code later?

I don't think there's really a way for us to make this atomic where they all work or none of them work... or is there?

EDIT: I'm worried that the potentially torn state will result in a weird debugging situation in the future...

You're right that's it's possible that we'd end in a bad state if one of these fails. Looking at the implementation of these, this is the only spot that can actually fail:

try { _buffer.append(str); return S_OK; } CATCH_RETURN();

(where str is the VT sequence we're writing). IMO, if that fails, then we're probably going to end up in a much worse torn state than just this one sequence being lost.

The idea to do them all atomically would probably match well with #3016.

Let's stash the idea inside the notes for #3016 then and just let this roll as is.

src/renderer/vt/Xterm256Engine.hpp

src/terminal/adapter/adaptDispatchGraphics.cpp

I addressed Carlos's minor feedback, and got 2 from the rest of the team, so Dustin said this one's good to ship

@carlos-zamora

## Summary of the Pull Request Adds support for Italics, Blinking, Invisible, CrossedOut text, THROUGH CONPTY. This does **NOT** add support for those styles to conhost or the terminal. We will store these "Extended Text Attributes" in a `TextAttribute`. When we go to render a line, we'll see if the state has changed from our previous state, and if so, we'll appropriately toggle that state with VT. Boldness has been moved from a `bool` to a single bit in these flags. Technically, now that these are stored in the buffer, we only need to make changes to the renderers to be able to support them. That's not being done as a part of this PR however. ## References See also #2915 and #2916, which are some follow-up tasks from this fix. I thought them too risky for 20H1. ## PR Checklist * [x] Closes #2554 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated <hr> * store text with extended attributes too * Plumb attributes through all the renderers * parse extended attrs, though we're not renderering them right * Render these states correctly * Add a very extensive test * Cleanup for PR * a block of PR feedback * add 512 test cases * Fix the build * Fix @carlos-zamora's suggestions * @miniksa's PR feedback (cherry picked from commit dec5c11)

- Fix snapping to the cursor in "Terminal Scrolling" mode (GH-2705) fixes GH-1222 PSReadline calls SetConsoleCursorPosition on each character. We try to snap. However, we'd only ever do this with the visible viewport, the viewport defined by `SCREEN_INFORMATION::_viewport`. When there's a virtual viewport in Terminal Scrolling mode, we actually need to snap the virtual viewport, so that this behavior looks more regular. (cherry picked from commit 6f4b98a) - Passthrough BEL in conpty (GH-2990) fixes GH-2952. (cherry picked from commit 6831120) - make filling chars (and, thus, erase line/char) unset wrap (GH-2831) EraseInLine calls `FillConsoleOutputCharacterW()`. In filling the row with chars, we were setting the wrap flag. We need to specifically not do this on ANY _FILL_ operation. Now a fill operation UNSETS the wrap flag if we fill to the end of the line. Originally, we had a boolean `setWrap` that would mean... - **true**: if writing to the end of the row, SET the wrap value to true - **false**: if writing to the end of the row, DON'T CHANGE the wrap value ,- current wrap value | ,- fill last cell? | | ,- new wrap value | | | ,- comments |-|-|-| |1|0|0| THIS CASE WAS UNHANDLED To handle that special case (1-0-0), we need to UNSET the wrap. So now, we have ~setWrap~ `wrap` mean the following: - **true**: if writing to the end of the row, SET the wrap value to TRUE - **false**: if writing to the end of the row, SET the wrap value to FALSE - **nullopt**: leave the wrap value as it is Closes GH-1126 (cherry picked from commit 4dd9f9c) - When reverse indexing, preserve RGB/256 attributes (GH-2987) (cherry picked from commit 847d6b5) - do not allow CUU and CUD to move "across" margins. (GH-2996) If we're moving the cursor up, its vertical movement should be stopped at the top margin. Similarly, this applies to moving down and the bottom margin. Fixes GH-2929. (cherry picked from commit 0ce08af) - Prevent the v1 propsheet from zeroing colors, causing black text on black background. (GH-2651) fixes GH-2319 (cherry picked from commit b97db63) - Render the cursor state to VT (GH-2829) (cherry picked from commit a9f3849) - Don't put NUL in the buffer in VT mode (GH-3015) (cherry picked from commit a82d6b8) - Return to ground when we flush the last char (GH-2823) The InputStateMachineEngine was incorrectly not returning to the ground state after flushing the last sequence. That means that something like alt+backspace would leave us in the Escape state, not the ground state. This makes sure we return to ground. Additionally removes the "Parser.UnitTests-common.vcxproj" file, which was originally used for a theoretical time when we only open-sourced the parser. It's unnecessary now, and we can get rid of it. Also includes a small patch to bcz.cmd, to make sure bx works with projects with a space in their name. (cherry picked from commit 53c81a0) - Add support for passing through extended text attributes, like… (GH-2917) ... Related work items: #23678919, #23678920, #23731910, #23731911, #23731912, #23731913, #23731914, #23731915, #23731916, #23731917, #23731918

ghost · 2019-10-23T19:55:02Z

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

Handy links:

This PR adds support for the ANSI _crossed-out_ graphic rendition attribute, which is enabled by the `SGR 9` escape sequence. * Support for the escape sequences and storage of the attribute was originally added in #2917. * Support for drawing the strikethrough effect in the grid line renderer was added in #7107. Since the majority of the code required for this attribute had already been implemented, it was just a matter of activating the `GridLines::Strikethrough` style in the `Renderer::s_GetGridlines` method when the `CrossedOut` attribute was set. VALIDATION There were already some unit tests in place in `VtRendererTest` and the `ScreenBufferTests`, but I've also now extended the SGR tests in `AdapterTest` to cover this attribute. I've manually confirmed the first test case from #6205 now works as expected in both conhost and Terminal. Closes #6205

zadjii-msft added 6 commits September 25, 2019 10:19

store text with extended attributes too

6a39606

Plumb attributes through all the renderers

bfee5fe

parse extended attrs, though we're not renderering them right

87dfc88

Render these states correctly

dedb191

Add a very extensive test

93aebf6

Cleanup for PR

2c9f539

zadjii-msft added Product-Conpty For console issues specifically related to conpty Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. labels Sep 26, 2019

zadjii-msft requested review from DHowett-MSFT, miniksa and carlos-zamora September 26, 2019 17:10

DHowett-MSFT approved these changes Sep 26, 2019

View reviewed changes

miniksa reviewed Sep 26, 2019

View reviewed changes

zadjii-msft mentioned this pull request Oct 1, 2019

ConPTY: Optimize SGR sequences by combining them into a single sequence #3016

Closed

a block of PR feedback

02d7d0e

zadjii-msft added 2 commits October 1, 2019 14:38

add 512 test cases

6aca2ec

Merge remote-tracking branch 'origin/master' into dev/migrie/b/2554-_…

e0df256

…italics_ # Conflicts: # src/host/ut_host/ScreenBufferTests.cpp

DHowett-MSFT approved these changes Oct 1, 2019

View reviewed changes

Fix the build

efe6b3c

carlos-zamora previously requested changes Oct 2, 2019

View reviewed changes

src/buffer/out/TextAttribute.hpp Outdated Show resolved Hide resolved

tools/bcz.cmd Show resolved Hide resolved

src/renderer/vt/Xterm256Engine.hpp Outdated Show resolved Hide resolved

src/renderer/vt/Xterm256Engine.cpp Outdated Show resolved Hide resolved

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 2, 2019

Fix @carlos-zamora's suggestions

9cf6996

zadjii-msft requested a review from carlos-zamora October 3, 2019 19:56

miniksa reviewed Oct 3, 2019

View reviewed changes

@miniksa's PR feedback

4d41c4e

miniksa approved these changes Oct 4, 2019

View reviewed changes

zadjii-msft changed the title ~~Add support for passing through extended text attributes, like italics~~ Add support for passing through extended text attributes, like… Oct 4, 2019

zadjii-msft merged commit dec5c11 into master Oct 4, 2019

DHowett-MSFT deleted the dev/migrie/b/2554-_italics_ branch October 4, 2019 20:57

ghost mentioned this pull request Oct 23, 2019

ConPTY swallows SGR 3 and 23, so terminals like Alacritty are prevented from supporting italics #2554

Closed

DHowett-MSFT mentioned this pull request Dec 4, 2019

Add support for XTPUSHSGR / XTPOPSGR #1978

Merged

j4james mentioned this pull request Aug 1, 2020

Add support for the "crossed-out" graphic rendition attribute #7143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for passing through extended text attributes, like… #2917

Add support for passing through extended text attributes, like… #2917

zadjii-msft commented Sep 26, 2019 •

edited

Loading

DHowett-MSFT Sep 26, 2019

DHowett-MSFT Sep 26, 2019

miniksa Sep 26, 2019

zadjii-msft Oct 1, 2019

DHowett-MSFT Sep 26, 2019

miniksa Sep 26, 2019

DHowett-MSFT commented Sep 30, 2019

azure-pipelines bot commented Sep 30, 2019

DHowett-MSFT commented Oct 1, 2019

DHowett-MSFT left a comment

carlos-zamora left a comment

miniksa left a comment

miniksa Oct 3, 2019

zadjii-msft Oct 4, 2019

miniksa Oct 4, 2019

ghost commented Oct 23, 2019

Add support for passing through extended text attributes, like… #2917

Add support for passing through extended text attributes, like… #2917

Conversation

zadjii-msft commented Sep 26, 2019 • edited Loading

Summary of the Pull Request

References

PR Checklist

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

DHowett-MSFT commented Sep 30, 2019

azure-pipelines bot commented Sep 30, 2019

DHowett-MSFT commented Oct 1, 2019

DHowett-MSFT left a comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

miniksa 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

ghost commented Oct 23, 2019

zadjii-msft commented Sep 26, 2019 •

edited

Loading