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

XTPUSHSGR doesn't handle default and zero parameters correctly #11883

Closed
j4james opened this issue Dec 4, 2021 · 7 comments · Fixed by #11888
Closed

XTPUSHSGR doesn't handle default and zero parameters correctly #11883

j4james opened this issue Dec 4, 2021 · 7 comments · Fixed by #11888
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-2 A description (P2) 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.
Milestone
Terminal v2.0

Comments

@j4james
Copy link
Collaborator

j4james commented Dec 4, 2021

Windows Terminal version

1.12.2931.0

Windows build number

10.0.19041.1348

Other Software

No response

Steps to reproduce

  1. Open a bash shell.
  2. Execute the following statements:
printf "\033[0;4;7m\033[#p\033[m\033[#qREVERSE UNDERLINE\033[m\n"
printf "\033[0;4;7m\033[;4#p\033[m\033[#qUNDERLINE ONLY\033[m\n"
printf "\033[0;4;7m\033[0#p\033[m\033[#qNEITHER REVERSE NOR UNDERLINE\033[m\n"

Expected Behavior

The first line should be both reversed and underlined.
The second line should be underlined only.
The last line should be neither reversed nor underlined.

This is a screenshot from XTerm:
image

Actual Behavior

All three lines are displayed as reversed and underlined.

image

The problem is that we're interpreting zero parameters and default parameters to mean that all attributes should be saved. However, the documentation states that we should only be saving all attributes when no parameters have been specified at all.
@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 Dec 4, 2021
@elsaco
Copy link

elsaco commented Dec 6, 2021

@j4james is there anything xterm specific in those test strings? On my Fedora workstation the xterm shows as above however gnome-terminal doesn't:

xterm test:

xterm

gnome-terminal test:
fish

@j4james
Copy link
Collaborator Author

j4james commented Dec 6, 2021 •
edited
Loading

VTE doesn't support XTPUSHSGR.

For reference, see https://gitlab.gnome.org/GNOME/vte/-/issues/23

@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase labels Dec 6, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Dec 6, 2021
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone Dec 6, 2021
@j4james j4james mentioned this issue Dec 7, 2021
Merged
7 tasks
@ghost ghost added the In-PR This issue has a related PR label Dec 7, 2021
@ghost ghost closed this as completed in #11888 Dec 7, 2021
ghost pushed a commit that referenced this issue Dec 7, 2021
@j4james
Use the til::enumset type for the SgrSaveRestoreStackOptions enum (#1…
91ad17d 
…1888)

## Summary of the Pull Request

This replaces the `std::bitset` for the `SgrSaveRestoreStackOptions` enum with a `til::enumset` type, thus avoiding the need to cast the `enum` to a `size_t` every time a value is set or tested.

This also fixes an issue with the handling of omitted and zero parameters in the `XTPUSHSGR` sequence, which are meant to be ignored, and not interpreted as "all".

## PR Checklist
* [x] Closes #11879
* [x] Closes #11883
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema 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

In addition to dropping all the `static_cast` operations, the use of the `til::enumset` also allowed us to get rid of the `try`/`catch` handling that was previously required in a couple of places, since the `til::enumset` operations don't throw. 

And to fix the zero parameter handling, we just needed to add an additional lower bound when validating that options are in range - if an option is 0 (`All`), it will now just be ignored.

## Validation Steps Performed

The updated code still passes the existing unit tests, and I've manually confirmed that it fixes the test case for omitted and zero parameters from issue #11883.
@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 Dec 7, 2021
@DHowett
Copy link
Member

DHowett commented Dec 8, 2021

/cc @jazzdelightsme for errata about XTPUSH/POP in Terminal 😄

@DHowett
Copy link
Member

DHowett commented Dec 8, 2021 •
edited
Loading

Unfortunately, this is broken in conhost in Windows 11 as well -- and is not likely to be fixed until the next release milestone (Windows 14?)

@j4james
Copy link
Collaborator Author

j4james commented Dec 8, 2021

I don't think that's a big deal though. These are really edge cases. I just happened to notice them when I was testing my enumset changes.

@jazzdelightsme
Copy link
Member

Belated thank you for noticing and fixing this!

@ghost
Copy link

ghost commented Feb 3, 2022

🎉This issue was addressed in #11888, which has now been successfully released as Windows Terminal Preview v1.13.10336.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-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-2 A description (P2) 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
Milestone
Terminal v2.0
Development

Successfully merging a pull request may close this issue.

Use the til::enumset type for the SgrSaveRestoreStackOptions enum j4james/terminal
5 participants
@DHowett @jazzdelightsme @elsaco @j4james @zadjii-msft