Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Request: implement XTPUSHSGR / XTPOPSGR #1796

Closed
jazzdelightsme opened this issue Jul 3, 2019 · 9 comments · Fixed by #1978
Closed

Feature Request: implement XTPUSHSGR / XTPOPSGR #1796

jazzdelightsme opened this issue Jul 3, 2019 · 9 comments · Fixed by #1978
Labels
Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@jazzdelightsme
Copy link
Member

jazzdelightsme commented Jul 3, 2019

Summary of the new feature/enhancement

I would like to add at least partial support for the control sequences "XTPUSHSGR" and "XTPOPSGR" (as well as their aliases). Summarized from xterm's ctlseqs doc:

XTPUSHSGR

CSI # {
          Push video attributes onto stack (XTPUSHSGR), xterm.  [...]
          If no parameters are given, all of the video attributes are
          saved.  The stack is limited to 10 levels.
CSI # p
          Push video attributes onto stack (XTPUSHSGR), xterm.  This is
          an alias for CSI # { .

XTPOPSGR

CSI # }   Pop video attributes from stack (XTPOPSGR), xterm.  Popping
          restores the video-attributes which were saved using XTPUSHSGR
          to their previous state.
CSI # q   Pop video attributes from stack (XTPOPSGR), xterm.  This is an
          alias for CSI # } .

The problem this helps with is composability of content.

Suppose you have code that emits text that contains SGR control sequences to colorize it. You will find that your text is not easily composable--that is, you may have a format string like "The name is: %s, the position is: %s", and then you want to sprintf some inserts into that. Then that whole string you may want to insert into another, and so forth.

The problem is that to have SGR sequences in that one has to have sort of a global awareness of content and associated coloring for that to work out.

With SGR push/pop, though, you can do things like:

<fgBlack>   : CSI 3 0 m
<fgGreen>   : CSI 3 2 m
<fgYellow>  : CSI 3 3 m
<fgMagenta> : CSI 3 5 m
<fgWhite>   : CSI 3 7 m
<bgBlack>   : CSI 4 0 m
<bgYellow>  : CSI 4 3 m
<bgWhite>   : CSI 4 7 m
<pushSgr>   : CSI # {
<popSgr>    : CSI # }

name = "Dan <pushSgr><fgBlack><bgWhite>Thompson<popSgr>"
position = "<pushSgr><fgMagenta><bgYellow>Developer at Large<popSgr>"
line1 = "<pushSgr><bgBlack><fgWhite>The name is: %s, the position is: %s<popSgr>"

And then you can printf(line1, name, position), and have it all work out.

Without a push/pop mechanism, composing strings like that cannot be done that way at all, unless you do your own custom rendering pass to handle the pushes and pops.

Xterm supports saving/restoring of individual attributes, but this most essential "composability" scenario only requires saving and restoring them all together, and should be simplest to implement.

Proposed technical implementation details (optional)

The "business logic" of these commands is not too complicated... it's just a stack.

Pull Request

PR is here: #1978

@jazzdelightsme jazzdelightsme added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jul 3, 2019
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jul 3, 2019
@zadjii-msft
Copy link
Member

Woah, I had no idea those existed. Did those get implemented recently?

@zadjii-msft zadjii-msft added this to the 20H1 milestone Jul 3, 2019
@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Jul 3, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 3, 2019
@jazzdelightsme
Copy link
Member Author

One open question: what to do if parameters are supplied to the XTPUSHSGR sequence? I summarized the command in the Issue; here is the full spec:

CSI # {
CSI Ps ; Ps # {
          Push video attributes onto stack (XTPUSHSGR), xterm.  The
          optional parameters correspond to the SGR encoding for video
          attributes, except for colors (which do not have a unique SGR
          code):
            Ps = 1  -> Bold.
            Ps = 2  -> Faint.
            Ps = 3  -> Italicized.
            Ps = 4  -> Underlined.
            Ps = 5  -> Blink.
            Ps = 7  -> Inverse.
            Ps = 8  -> Invisible.
            Ps = 9  -> Crossed-out characters.
            Ps = 1 0  -> Foreground color.
            Ps = 1 1  -> Background color.
            Ps = 2 1  -> Doubly-underlined.

          If no parameters are given, all of the video attributes are
          saved.  The stack is limited to 10 levels.

I don't think it is necessary to start out implementing support for the parameters, but if parameters are seen, what to do? Throw some sort of error? Or just ignore the entire sequence? I think the latter would be best. But note that to properly ignore it one would actually need to do an "empty" push, so that a subsequent XTPOPSGR sequence will have no effect (and not unbalance the stack).

@jazzdelightsme
Copy link
Member Author

@zadjii-msft: yes; first introduced last August in patch #334, with aliases added more recently.

@miniksa
Copy link
Member

miniksa commented Jul 3, 2019

This sounds great.

I agree with the interim compromise of doing the empty push and ignoring the excess parameters we can't handle at that moment.

I think it makes sense to get it going in phase 1 then add parameters in phase 2 to make implementation easier. I would just not like to see a long long time go by between the two phases.

I think it's better to not support this at all than leave it in a half-baked state for a long time.

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 8, 2019
@DHowett-MSFT DHowett-MSFT modified the milestones: 20H1, Console Backlog Jul 8, 2019
@ghost ghost added the In-PR This issue has a related PR label Jul 15, 2019
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Jul 19, 2019
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
jazzdelightsme added a commit to jazzdelightsme/PowerShell that referenced this issue Jul 23, 2019
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.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Aug 12, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Aug 12, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Aug 12, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Aug 30, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Sep 10, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Sep 13, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
@ghost ghost removed the Help Wanted We encourage anyone to jump in on these. label Sep 13, 2019
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Oct 16, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Oct 16, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Oct 16, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Oct 16, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Oct 16, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Oct 16, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Oct 16, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Oct 16, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
@j4james
Copy link
Collaborator

j4james commented Dec 10, 2019

FYI, there's an ongoing discussion regarding these sequences in the xtermjs issue tracker (xtermjs/xterm.js#2570), and whether they're worth implementing as currently specified.

jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Dec 11, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Dec 16, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
jazzdelightsme added a commit to jazzdelightsme/terminal that referenced this issue Dec 18, 2019
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.

Pushes beyond ten will remain balanced (an equal number of pops will
take you back down to zero), up to 100 pushes. Beyond 100 pushes, pushes
will become unbalanced (the internal counter will no longer be
incremented). This bound gives the terminal a deterministic way to
recover from garbage--do 101 pops and you know you've cleared the stack
back down to zero.

For partial pushes (see the description of XTPUSHSGR in Issue microsoft#1796),
only attributes that are supported by terminal are saved; others are
ignored (this change does not including adding general support for
double underlines, for example). A partial push of unsupported
parameters results in an "empty" push--the subsequent pop will not
change the current text attributes.
@DHowett
Copy link
Member

DHowett commented Feb 18, 2021

Hey @jazzdelightsme, how should/does this interact with RIS hard reset?

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Feb 18, 2021
DHowett pushed a commit that referenced this issue Feb 18, 2021
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
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Feb 18, 2021
@jazzdelightsme
Copy link
Member Author

@DHowett That's the beaty of the ring buffer implementation: nothing special needs to be done to "reset to initial state"; you just decide "okay, we're going to call where we are right now 'the beginning'". (More about this in the comment here.)

It's true that if you had done some pushes, and then you executed a RIS, and then you did some pops, it would indeed restore the saved attributes. But I think the point of RIS (and actually I'm not an expert on that; I had to look it up) is that you want to "start over", in which case why would you immediately execute "negative" pops? So I don't think it's necessary to do anything special (like clear out the stack) in response to RIS. Just execute the RIS, and act as if you have 10 fresh, open sgrStack slots ahead of you (because you always do).

@j4james
Copy link
Collaborator

j4james commented Feb 18, 2021

@jazzdelightsme RIS is essentially the equivalent of a computer reboot. So for something like this I would generally expect RIS to have cleared the stack. However, I think it's probably more important to match whatever Xterm and Mintty are doing - so if they don't clear the stack then it's probably OK that we don't either. If they do clear the stack, we definitely should be matching that behaviour (same goes for a soft reset).

@ghost
Copy link

ghost commented Mar 1, 2021

🎉This issue was addressed in #1978, which has now been successfully released as Windows Terminal Preview v1.7.572.0.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants