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

Add support for SGR 58/59 (underline coloring) #15599

Closed
wants to merge 3 commits into from

Conversation

tusharsnx
Copy link
Contributor

@tusharsnx tusharsnx commented Jun 25, 2023

Summary of the Pull Request

Adds support for underline coloring with SGR 58/59 sequences. ConPTY can now parse, store and send those sequences across console applications and client terminal app.

Tested it on Wezterm which already has support for drawing colored underlines. For testing, I replaced the prebundled OpenConsole.exe and conpty.dll from Wezterm with the modified versions.

printf "\n\e[4m\e[58;2;0;255;215mRGBCyanUnderline\e[24m \e[58;5;50m\e[4mINDEX256CyanUnderline\e[24m \e[4m\e[58;5;50m\e[38;2;255;0;95mUnderlineColorWithColoredText\e[24m \e[4m\e[38;5;50m\e[58;2;255;0;95mAnotherExample\n"

Above code produces output like so:

Left: Wezterm, Right: WindowsTerminal Dev
Screenshot 2023-06-25 183224

Changes aren't visible on WindowsTerminal as of now. Atleast one of the graphic render, E.g. AtlasEngine, needs to support coloring of underline and drawing them on screen. Will be working on this in next few days.

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

PR Checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated
    • If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated (if necessary)

@tusharsnx

This comment was marked as off-topic.

@tusharsnx

This comment was marked as resolved.

@tusharsnx
Copy link
Contributor Author

tusharsnx commented Jun 25, 2023

Almost! 🤩

image

No blinking issue in OpenConsole.exe atleast.

const auto r = GetRValue(color);
const auto g = GetGValue(color);
const auto b = GetBValue(color);
return _WriteFormatted(FMT_COMPILE("\x1b[58;2;{};{};{}m"), r, g, b);

Choose a reason for hiding this comment

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

Can these use colon-separated subparameters? So as to minimise the risk that applications ignorant of underline colors treat the RGB numbers as separate attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean, may be we can do only : instead, but that would require removing the invalidation of : as a CSI parameter delimiter (conditionally). Thanks for the heads up.

Choose a reason for hiding this comment

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

I see, it's blocked by #4321.

@tusharsnx
Copy link
Contributor Author

tusharsnx commented Jun 26, 2023

in case an emulator doesn't recognize a sequence (let's say doesn't recognize the new extension of 58), subsequent numbers are interpreted as other attributes. E.g. if 256-color mode is chosen then the next numeric parameter is 5 which turns on blinking.

I guess, this comment explains the blinking issue, and why we need to prefer using : as parameter separator for underline colouring.

@tusharsnx
Copy link
Contributor Author

Interestingly, running the example command on WT dev makes few words to blink (??), don't know what might cause this.

Ok, so there is no blinking issue in WT dev. I had changed the project location to another drive and forgot to re-deploy it from that location, so it was using the outdated one.

image

DHowett pushed a commit that referenced this pull request Jul 18, 2023
Adds support for colon `:` separated sub parameters in parser.
Technically, after this PR, nothing should change except, now sub
parameters are parsed, stored safely and we don't invalidate the whole
sequence when a `:` is received within a parameter substring.

In this PR:
- If sub parameters are detected with a parameter, but the usage is
unrecognised, we simply *skip* the parameter in `adaptDispatch`.
- A separate store for sub parameters is used to avoid too many changes
to the codebase.
- We currently allow up to `6` sub parameters for each parameter, extra
sub parameters are *ignored*.
- Introduced `VTSubParameters` for easy access to underlying sub
parameters.

> **Info**: We don't use sub parameters for any feature yet, this is
just the core implementation to support newer usecases.

## Validation Steps Performed
- [x] Use of sub parameters must not have any effect on the output.
- [x] Skip parameters with unexpected set of sub parameters.
- [x] Skip sequences with unexpected set of sub parameters.

References #4321
References #7228
References #15599
References xtermjs/xterm.js#2751
Closes #4321
@tusharsnx
Copy link
Contributor Author

closing this in favour of: #15795

@tusharsnx tusharsnx closed this Aug 8, 2023
@tusharsnx tusharsnx deleted the feat-underline-coloring branch September 9, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants