Add support for XTPUSHSGR / XTPOPSGR #1978

jazzdelightsme · 2019-07-15T16:00:02Z

Summary of the Pull Request

Implement the XTPUSHSGR and XTPOPSGR control sequences (see #1796).

This change adds a new pair of methods to ITermDispatch:
PushGraphicsRendition and PopGraphicsRendition, and then plumbs the
change through AdaptDispatch, TerminalDispatch, ITerminalApi and
TerminalApi.

The stack logic is encapsulated in the SgrStack class, to allow it to
be reused between the two APIs (AdaptDispatch and TerminalDispatch).

Like xterm, only ten levels of nesting are supported.

The stack is implemented as a "ring stack": if you push when the stack
is full, the bottom of the stack will be dropped to make room.

Partial pushes (see the description of XTPUSHSGR in Issue #1796) are
implemented per xterm spec.

Validation Steps Performed

Tests added, plus manual verification of the feature.

Closes #1796

jazzdelightsme · 2019-07-15T16:05:25Z

The maximum number of saved states is 10 (like xterm). Perhaps I could additionally place a (much higher) limit on the number of allowed pushes (say 10,000). With that bound, you know that you need only do 10,001 pops to be sure that you've recovered from an unbalanced state. And then we also wouldn't have to worry about the push counter wrapping around.

zadjii-msft

So I think the right way of implementing this at the moment would be to implement it not in the Terminal (with TerminalApi and TerminalDispatch) but instead in conhost, with adaptDispatch. If you're able to make the changes there, then conhost should be able to "render" the changed state correctly to the terminal, without the terminal even needing to process these messages itself (though, leaving the implementation there isn't necessarily a bad idea, as the terminal will want to be able to handle these messages itself one day).
I believe that if you return false in AdaptDispatch, conpty will just let the sequence fallthrough to the connected terminal.
So maybe finish implementing it in the Terminal first, while conhost is letting the sequences fall through, then implement again in conhost (which will probably be basically the same implementation), and make sure it keeps working.

src/terminal/adapter/conGetSet.hpp

This is not a polished PR that is ready to merge; it demonstrates a direction for which I need to get buy-off from the team before pursuing further. I'm currently working on implementing the XTPUSHSGR/XTPOPSGR control sequences (WIP PR [here](microsoft#1978), which requires saving a [stack of] text attributes, and not just the legacy attributes, but full RGB colors, etc. My first instinct was to implement the "business logic" (the stack) in the `AdaptDispatch` layer, but that will require getting the [full] text attributes from the underlying console API. This is not a *terribly* "invasive" change, but it is exposing new stuff at a layer boundary. Put another way, this means pound-including the "../buffer/out/TextAttribute.hpp" header in a few places outside of "buffer/out", and is that okay, or does that header need to be kept private and isolated? So there are a few ways this could go: 1. You folks might say "ugh, no!", and: 1. I could push (haha) the business logic of pushing and popping text attributes down another layer (so `AdaptDispatch` just forwards on the [PushGraphicsRendition](https://github.com/microsoft/terminal/blob/0af275b9cb68da14f38f05b2cdcbb35da99cb17c/src/terminal/adapter/adaptDispatchGraphics.cpp#L452) call to the lower layer, OR 2. You suggest a different, cleaner way of exposing the text attributes. OR 2. Maybe you think this general direction is fine, but maybe you have some particular requests that I do certain things differently (I totally understand being picky about stuff that cuts across API layers). What do you think?

This is not a polished PR that is ready to merge; it demonstrates a direction for which I need to get buy-off from the team before pursuing further. I'm currently working on implementing the XTPUSHSGR/XTPOPSGR control sequences (WIP PR [here](microsoft#1978), which requires saving a [stack of] text attributes, and not just the legacy attributes, but full RGB colors, etc. My first instinct was to implement the "business logic" (the stack) in the `AdaptDispatch` layer, but that will require getting the [full] text attributes from the underlying console API. This is not a *terribly* "invasive" change, but it is exposing new stuff at a layer boundary. Put another way, this means pound-including the "../buffer/out/TextAttribute.hpp" header in a few places outside of "buffer/out", and is that okay, or does that header need to be kept private and isolated? So there are a few ways this could go: 1. You folks might say "ugh, no!", and: 1. I could push (haha) the business logic of pushing and popping text attributes down another layer (so `AdaptDispatch` just forwards on the [PushGraphicsRendition](https://github.com/microsoft/terminal/blob/0af275b9cb68da14f38f05b2cdcbb35da99cb17c/src/terminal/adapter/adaptDispatchGraphics.cpp#L452) call to the lower layer, OR 2. You suggest a different, cleaner way of exposing the text attributes. OR 2. Maybe you think this general direction is fine, but maybe you have some particular requests that I do certain things differently (I totally understand being picky about stuff that cuts across API layers). What do you think? Related: PR microsoft#1978 Related: Issue microsoft#1796

The RawUI's LengthInBufferCells needs to treat VT control sequences as zero-width for the purpose of determining string length. Previously, it only handled SGR (Select Graphics Rendition) sequences (which do things like set fg color, bg color, etc.). I'm currently adding support for some new control sequences in the microsoft/terminal project: XTPUSHSGR and XTPOPSGR. Initial WIP PR [here](microsoft/terminal#1978). Summarized descriptions of XTPUSHSGR and XTPOPSGR from XTerm's [ctlseqs](https://invisible-island.net/xterm/ctlseqs/ctlseqs.html) documentation: ``` CSI # { CSI Ps ; Ps # { Push video attributes onto stack (XTPUSHSGR), xterm. CSI # } Pop video attributes from stack (XTPOPSGR), xterm. Popping restores the video-attributes which were saved using XTPUSHSGR to their previous state. CSI # p CSI Ps ; Ps # p Push video attributes onto stack (XTPUSHSGR), xterm. This is an alias for CSI # {. CSI # q Pop video attributes from stack (XTPOPSGR), xterm. This is an alias for CSI # }. ``` The scenario enabled by these sequences is composability (see [Issue 1796](microsoft/terminal#1796)). I'd like to support these sequences in PowerShell, similarly to SGR sequences, to enable better SGR content composability.

src/buffer/out/TextAttribute.cpp

jazzdelightsme · 2019-08-26T15:54:39Z

@zadjii-msft I think this PR is ready to go.

src/buffer/out/sgrStack.hpp

src/terminal/adapter/conGetSet.hpp

DHowett-MSFT

All minor things, but I have some questions 😄

src/cascadia/TerminalCore/TerminalApi.cpp

src/host/getset.cpp

src/terminal/adapter/ut_adapter/adapterTest.cpp

src/terminal/parser/OutputStateMachineEngine.cpp

src/buffer/out/sgrStack.hpp

DHowett-MSFT · 2019-12-04T18:58:00Z

src/buffer/out/sgrStack.hpp

+                }
+            }
+
+            if (_GraphicsOptionToFlag(DispatchTypes::GraphicsOptions::SaveForegroundColor) & validParts)


This list of attributes may need to be extended to support strikethrough/italic/the other extended text attributes introduced in #2917.

DHowett · 2021-02-09T18:52:01Z

Did we end up converging on option 3?

My lingering doubt has been quelled by one simple fact: this is now an xterm compatibility concern. Once I thought of it that way, it became obvious that we need to do something.

This change adds a new pair of methods to ITermDispatch: PushGraphicsRendition and PopGraphicsRendition, and then plumbs the change through AdaptDispatch, TerminalDispatch, ITerminalApi and TerminalApi. The stack logic is encapsulated in the SgrStack class, to allow it to be reused between the two APIs (AdaptDispatch and TerminalDispatch). Like xterm, only ten levels of nesting are supported. The stack is implemented as a "ring stack": if you push when the stack is full, the bottom of the stack will be dropped to make room.

src/buffer/out/TextAttribute.cpp

src/terminal/parser/OutputStateMachineEngine.hpp

jazzdelightsme · 2021-02-14T05:20:07Z

@DHowett as things stand in this PR, what I actually implemented is option 1. Which, in terms of this being an xterm compat issue, seems reasonable--it matches xterm right now, and if later people come to a different consensus, then we could change it. But I'd also be happy to dike out the partial saves if it meant getting this feature in.

DHowett

My concerns are mostly around comments, exceptions and line endings. This is beautiful work, and we've done it a disservice by not driving it to ground.

James can double-check me on how I feel about VTParameter's value_or, if he wants. 😄

.github/actions/spelling/expect/expect.txt

src/terminal/adapter/adaptDispatch.hpp

src/types/lib/types.vcxproj

src/types/sgrStack.cpp

DHowett · 2021-02-16T23:50:43Z

src/types/sgrStack.cpp

+            if (_nextPushIndex == 0)
+            {
+                _nextPushIndex = gsl::narrow<int>(_storedSgrAttributes.size() - 1);
+            }
+            else
+            {
+                _nextPushIndex--;
+            }


nit: technically, you can do a wraparound mod math trick here:

_nextPushIndex = (size + _nextPushIndex - 1) % size

That's if concision was the goal, of course. It's not necessarily the goal!

Hmmm... I waffled on this. I lean toward the more verbose, but I'd change it if y'all had a strong opinion.

src/types/sgrStack.cpp

DHowett · 2021-02-17T00:06:15Z

Here's what I'm thinking. Option 1 is "compatible", and we can see how much use it gets in the wild. I suspect that we can move to option 3 and treat partials as fulls if we have to. Let's celebrate incremental improvement, shall we? (Does that mollify your main concerns, or at least enough to accept it for the time being, @j4james?)

miniksa

I'm fine with this pending Dustin's comments.

j4james · 2021-02-17T01:42:38Z

Here's what I'm thinking. Option 1 is "compatible", and we can see how much use it gets in the wild. I suspect that we can move to option 3 and treat partials as fulls if we have to. Let's celebrate incremental improvement, shall we?

I think you might have the options the wrong way around? Option 3 is the partial implementation, and option 1 is essentially the full thing. I could understand starting with 3 and later moving to 1, but going in the other direction doesn't really make sense.

(Does that mollify your main concerns, or at least enough to accept it for the time being, @j4james?)

My dislike of this feature has only grown more intense over time, so I don't think it's worth trying to mollify me. Also, with Mintty having implemented it now too, it sort of feels like a lost battle. If you guys all like it, I'm not going to argue against it.

DHowett · 2021-02-18T02:22:57Z

My dislike of this feature has only grown more intense over time, so I don't think it's worth trying to mollify me. Also, with Mintty having implemented it now too, it sort of feels like a lost battle. If you guys all like it, I'm not going to argue against it.

I appreciate that and respect your candor. 🙂

You're right, of course, about Option 1. I had it in my head that we could do the "more compatible" thing (1) and then if it didn't hold up to scrutiny or seem to be as well-used in the wild just gank the features that were controversial (getting us to 3).

DHowett

@jazzdelightsme Thanks for putting up with our literally almost two year long review cycle. I'm merging this today despite some community misgivings, but I do not believe they deeply detract from what's otherwise a solid feature. I get why we would want this, and I get why xterm wanted it and mintty probably wanted it, so I think we shouldn't stand against progress. We can always revisit our decisions -- nothing in this repository is etched in stone.

jazzdelightsme · 2021-02-18T07:01:12Z

Thank you!

ghost · 2021-03-01T18:02:12Z

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

Handy links:

DHowett · 2021-03-17T19:17:28Z

This is available in conhost as of Windows Insider Preview build 21337. Thanks again! 😄

zadjii-msft reviewed Jul 16, 2019

View reviewed changes

src/terminal/adapter/conGetSet.hpp Outdated Show resolved Hide resolved

jazzdelightsme mentioned this pull request Jul 17, 2019

Bug Report: What is reordering VT escape sequences in my string? #2011

Closed

jazzdelightsme mentioned this pull request Jul 19, 2019

WIP: Expose current TextAttributes from the underlying console API. #2032

Closed

5 tasks

jazzdelightsme mentioned this pull request Jul 23, 2019

Formatting: handle XTPUSHSGR/XTPOPSGR sequences PowerShell/PowerShell#10208

Merged

14 tasks

jazzdelightsme force-pushed the user/danthom/wip/xtpushsgr branch from 0af275b to f07a68b Compare August 12, 2019 14:09

jazzdelightsme changed the title ~~WIP: Add support for XTPUSHSGR / XTPOPSGR~~ Add support for XTPUSHSGR / XTPOPSGR Aug 12, 2019

jazzdelightsme force-pushed the user/danthom/wip/xtpushsgr branch from f07a68b to 17c4bd6 Compare August 12, 2019 14:21

jazzdelightsme commented Aug 12, 2019

View reviewed changes

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

jazzdelightsme commented Aug 26, 2019

View reviewed changes

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

jazzdelightsme force-pushed the user/danthom/wip/xtpushsgr branch from f61d3eb to 76e9df9 Compare August 30, 2019 01:59

jazzdelightsme force-pushed the user/danthom/wip/xtpushsgr branch 2 times, most recently from c1e5a12 to 8f18939 Compare September 13, 2019 01:19

jazzdelightsme mentioned this pull request Sep 13, 2019

Feature Request: implement XTPUSHSGR / XTPOPSGR #1796

Closed

jazzdelightsme force-pushed the user/danthom/wip/xtpushsgr branch 8 times, most recently from 29c3640 to fa93100 Compare October 16, 2019 17:45

jazzdelightsme mentioned this pull request Nov 18, 2019

Support XTPUSHSGR / XTPOPSGR xtermjs/xterm.js#2570

Open

DHowett-MSFT reviewed Dec 4, 2019

View reviewed changes

src/terminal/adapter/conGetSet.hpp Outdated Show resolved Hide resolved

DHowett-MSFT suggested changes Dec 4, 2019

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 4, 2019

jazzdelightsme force-pushed the user/danthom/wip/xtpushsgr branch from d5de9a4 to afcf1d4 Compare July 17, 2020 21:41

cinnamon-msft modified the milestones: Terminal v1.x, Terminal v2.0 Sep 29, 2020

microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:28

ghost added 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 Oct 22, 2020

This was referenced Jan 13, 2021

SGR support migueldeicaza/SwiftTerm#163

Closed

SGR support migueldeicaza/SwiftTerm#164

Open

jazzdelightsme force-pushed the user/danthom/wip/xtpushsgr branch from afcf1d4 to ffcb365 Compare February 14, 2021 00:50

j4james reviewed Feb 14, 2021

View reviewed changes

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

Fix static analysis complaints about possible exceptions.

34547cf

j4james reviewed Feb 14, 2021

View reviewed changes

src/terminal/parser/OutputStateMachineEngine.hpp Outdated Show resolved Hide resolved

PR feedback: don't need Set...From; sort Csi codes

48c9720

DHowett requested changes Feb 16, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 16, 2021

miniksa approved these changes Feb 17, 2021

View reviewed changes

PR feedback

e4c9ca6

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 17, 2021

DHowett approved these changes Feb 18, 2021

View reviewed changes

DHowett merged commit 72cbe59 into microsoft:main Feb 18, 2021

jazzdelightsme deleted the user/danthom/wip/xtpushsgr branch February 18, 2021 07:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for XTPUSHSGR / XTPOPSGR #1978

Add support for XTPUSHSGR / XTPOPSGR #1978

jazzdelightsme commented Jul 15, 2019 •

edited by DHowett

Loading

jazzdelightsme commented Jul 15, 2019

zadjii-msft left a comment

jazzdelightsme commented Aug 26, 2019

DHowett-MSFT left a comment

DHowett-MSFT Dec 4, 2019

jazzdelightsme Dec 18, 2019

DHowett commented Feb 9, 2021

jazzdelightsme commented Feb 14, 2021

DHowett left a comment

DHowett Feb 16, 2021

jazzdelightsme Feb 17, 2021

DHowett commented Feb 17, 2021

miniksa left a comment

j4james commented Feb 17, 2021

DHowett commented Feb 18, 2021

DHowett left a comment

jazzdelightsme commented Feb 18, 2021

ghost commented Mar 1, 2021

DHowett commented Mar 17, 2021

Add support for XTPUSHSGR / XTPOPSGR #1978

Add support for XTPUSHSGR / XTPOPSGR #1978

Conversation

jazzdelightsme commented Jul 15, 2019 • edited by DHowett Loading

Summary of the Pull Request

Validation Steps Performed

jazzdelightsme commented Jul 15, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

jazzdelightsme commented Aug 26, 2019

DHowett-MSFT left a comment

Choose a reason for hiding this comment

DHowett-MSFT Dec 4, 2019

Choose a reason for hiding this comment

jazzdelightsme Dec 18, 2019

Choose a reason for hiding this comment

DHowett commented Feb 9, 2021

jazzdelightsme commented Feb 14, 2021

DHowett left a comment

Choose a reason for hiding this comment

DHowett Feb 16, 2021

Choose a reason for hiding this comment

jazzdelightsme Feb 17, 2021

Choose a reason for hiding this comment

DHowett commented Feb 17, 2021

miniksa left a comment

Choose a reason for hiding this comment

j4james commented Feb 17, 2021

DHowett commented Feb 18, 2021

DHowett left a comment

Choose a reason for hiding this comment

jazzdelightsme commented Feb 18, 2021

ghost commented Mar 1, 2021

DHowett commented Mar 17, 2021

jazzdelightsme commented Jul 15, 2019 •

edited by DHowett

Loading