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 Kitty/VTE extended underlines (4:x) [SGR] #7228

Open
DHowett opened this issue Aug 8, 2020 · 5 comments
Open

Add support for Kitty/VTE extended underlines (4:x) [SGR] #7228

DHowett opened this issue Aug 8, 2020 · 5 comments
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase
Milestone

Comments

@DHowett
Copy link
Member

DHowett commented Aug 8, 2020

depends on #4321

Re underline:

The Kitty terminal emulator came up with the awesome idea of supporting curly and colored underlines, with the obvious intent of supporting user-friendly spell checking in terminal-based text editors. The choice of the escape sequence was coordinated between Kitty and VTE. The feature was then implemented so far at least in Kitty, VTE, Mintty, Hterm, probably a few more as well, and feature requests are filed to even more, including iTerm2, Konsole, xterm.js. Some have even added dotted and dashed underlines, too.

It would be lovely if you also considered these extensions.

(A bit of technical info: With truecolor support I assume you already have like 25 bits for the foreground and background color each (in order to be able to store the 256 legacy palette values as well as "default", in addition to 24 bit RGB). At least this is how we do in VTE. And we didn't want to waste another 25 bits for this rarely used feature. So we approximate truecolor underline colors to 4+5+4 bits of R, G, B, respectively. This way all the color information of a charcell fits in an int64.)
from egmontkob in #2916

For what it's worth I've extended this a little, and my programs support the following subparameters:

  1. single
  2. double
  3. (short wavelength) curly
  4. (closely spaced) dotted (8 dots per cell)
  5. (short) dashed (4 eighth-width dashes per cell)
  6. long dashed (2 quarter-width dashes per cell)
  7. extra long dashed (1 half-width dash per cell)
  8. medium spaced dotted (4 dots per cell)
  9. widely spaced dotted (2 dots per cell)
  10. long wavelength curly

from @jdebp in #2916

@DHowett DHowett added Product-Conhost For issues in the Console codebase Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. labels Aug 8, 2020
@DHowett DHowett added this to the Terminal Backlog milestone Aug 8, 2020
@msftbot msftbot bot added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 8, 2020
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 10, 2020
@zadjii-msft zadjii-msft modified the milestones: Terminal Backlog, Backlog Jan 4, 2022
@Tyriar
Copy link
Member

Tyriar commented Aug 1, 2022

I added this to xterm.js last week, currently the sequences get ignored by conpty, we need them to pass through. xtermjs/xterm.js#3921

@zadjii-msft
Copy link
Member

zadjii-msft commented Aug 1, 2022

I suspect even the premptive flushing of #13462 wouldn't fix this for conpty consumers either. We'd need to actually specifically note this category of sequence and then return false (or just, handle ourselves). Though, handling ourselves might be fairly engineering expensive (parsing, storing, re-rendering to conpty, rendering in dx/atlas), so maybe it does make sense to include the no-op passthrough for a smaller conpty package update.

@j4james
Copy link
Collaborator

j4james commented Aug 4, 2022

FYI, passthrough for sequences like this is not workable, even with flushing. Testing with a simple printf may briefly give the impression that it's working, but any reasonably complicated application is bound to fail eventually.

To understand why, have a look at the debug tab while scrolling through a file in vim. Notice how it appears to redraw the entire screen every time you scroll down the page? That's not vim doing the redrawing - that's conpty. Vim is just using something like a linefeed combined with scroll margins.

So what happens when vim decides to output some wavy underlines? They may appear to work the first time the screen is rendered, but as soon as you scroll, it'll be conpty that does the redrawing. And since conpty knows nothing of those attributes, it's just going to obliterate them.

Bottom line: Unless conpty stores and forwards the extended attributes, they're not going to work.

@Tyriar
Copy link
Member

Tyriar commented Aug 4, 2022

Yep, these need to be stored in conpty's model, as opposed to OSC 133 and the like which just need pass-through and don't need to be emitted again on redraw

@j4james
Copy link
Collaborator

j4james commented Aug 4, 2022

Technically OSC 133 can break in a similar manner - it's just less likely in typical usage. The only way we can guarantee perfect fidelity with passthrough is if we're doing it for everything, with no conpty buffering at all (i.e. the passthrough mode of #1173). So our choices are either buffer everything and then rerender to conpty, or buffer nothing and pass everything through directly. Mixing the two is always going to be flaky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-VT Virtual Terminal sequence support 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
Development

No branches or pull requests

4 participants