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

Add support for DECSCNM in Windows Terminal #6622

Closed
j4james opened this issue Jun 21, 2020 · 1 comment · Fixed by #6809
Closed

Add support for DECSCNM in Windows Terminal #6622

j4james opened this issue Jun 21, 2020 · 1 comment · Fixed by #6809
Labels
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-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.2

Comments

@j4james
Copy link
Collaborator

j4james commented Jun 21, 2020

Description of the new feature/enhancement

When the DECSCNM mode was implemented in PR #3817, it was only actually added to conhost, but relied on the fact that the conpty renderer was passing through the translated colors (rather than the underlying buffer values), so it gave the impression that it worked in Windows Terminal as well. Once PR #6506 is merged, though, this will no longer be the case, so we need to add explicit support for DECSCNM in WT if we want to retain that functionality.

Proposed technical implementation details (optional)

In the conhost AdaptDispatch, we need to return false from the SetScreenMode method (when in conpty mode), to force the DECSCNM sequence to be passed through to the conpty pipe. Then on the WT side, we need to duplicate a lot of the AdaptDispatch implementation in TerminalDispatch, and update the renderdata GetForegroundColor and GetBackgroundColor calculations to take the reversed screen mode into account.
@j4james j4james added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jun 21, 2020
@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 Jun 21, 2020
@DHowett DHowett 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-Terminal The new Windows Terminal. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 21, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 21, 2020
@DHowett DHowett added this to the Terminal v1.2 milestone Jun 21, 2020
@j4james j4james mentioned this issue Jul 7, 2020
Merged
6 tasks
@ghost ghost added the In-PR This issue has a related PR label Jul 7, 2020
@ghost ghost closed this as completed in #6809 Jul 9, 2020
@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 Jul 9, 2020
ghost pushed a commit that referenced this issue Jul 9, 2020
@j4james
Add support for DECSCNM in Windows Terminal (#6809)
695ebff 
## Summary of the Pull Request

This PR adds full support for the `DECSCNM` reverse screen mode in the Windows Terminal to align with the implementation in conhost.

## References

* The conhost implementation of `DECSCNM` was in PR #3817.
* WT originally inherited that functionality via the colors being passed through, but that behaviour was lost in PR #6506.

## PR Checklist
* [x] Closes #6622
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [x] 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: #6622

## Detailed Description of the Pull Request / Additional comments

The `AdaptDispatch::SetScreenMode` now checks if it's in conpty mode and simply returns false to force a pass-through of the mode change. And the `TerminalDispatch` now has its own `SetScreenMode` implementation that tracks any changes to the reversed state, and triggers a redraw in the renderer.

To make the renderer work, we just needed to update the `GetForegroundColor` and `GetBackgroundColor` methods of the terminal's `IRenderData` implementation to check the reversed state, and switch the colors being calculated, the same way the `LookupForegroundColor` and `LookupBackgroundColor` methods work in the conhost `Settings` class.

## Validation Steps Performed

I've manually tested the `DECSCNM` functionality for Windows Terminal in Vttest, and also with some of my own test scripts.
@ghost
Copy link

ghost commented Jul 22, 2020

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

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
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-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.2
Development

Successfully merging a pull request may close this issue.

Add support for DECSCNM in Windows Terminal j4james/terminal
2 participants
@DHowett @j4james