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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

jazzdelightsme
Copy link
Member

Summary of the Pull Request

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, 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 call to the lower layer, OR
    2. You suggest a different, cleaner way of exposing the text
      attributes.

OR

  1. 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?

References

Related: PR #1978
Related: Issue #1796

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

none

@jazzdelightsme
WIP: Expose current TextAttributes from the underlying console API.
14e3bc4 
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
zadjii-msft
zadjii-msft requested changes Jul 25, 2019
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Largely, this is exactly what I would have done. I am worried though that the implementation as it is leaves the opportunity for a caller to accidentally modify the buffer's state.

src/terminal/adapter/conGetSet.hpp Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 25, 2019
@zadjii-msft zadjii-msft 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 Jul 25, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 26, 2019
@jazzdelightsme jazzdelightsme mentioned this pull request Aug 12, 2019
Merged
@jazzdelightsme
Copy link
Member Author

This has been fleshed out and incorporated as part of my main PR (#1978).

@jazzdelightsme jazzdelightsme deleted the user/danthom/wip/exposeCurrentTextAttribute branch August 12, 2019 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft zadjii-msft requested changes

Assignees
No one assigned
Labels
Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jazzdelightsme @zadjii-msft