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

Support for strikethrough text in Windows Terminal #6205

Closed
udif opened this issue May 26, 2020 · 7 comments · Fixed by #7143
Closed

Support for strikethrough text in Windows Terminal #6205

udif opened this issue May 26, 2020 · 7 comments · Fixed by #7143
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues 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.

Comments

@udif
Copy link

udif commented May 26, 2020

Environment

Windows build number: Microsoft Windows [Version 10.0.18363.836]
Windows Terminal version (if applicable): Windows Terminal Version: 1.0.1401.0

Steps to reproduce

Please run these under bash (can be from WSL):

echo -e '\e[9mstrikethrough\e[0m'
echo -e 's\u0336t\u0336r\u0336i\u0336k\u0336e\u0336t\u0336h\u0336r\u0336o\u0336u\u0336g\u0336h\u0336'

Expected behavior

In Ubuntu 18.04 under Gnome Terminal (I tested with 3.28) You would get strikethrough in both cases:

vncviewer_KMfTbM1sOW

Actual behavior

In Ubuntu 18.04 WSL1 (or plain windows ssh to a real Ubuntu machive/VM) under Windows terminal you would get neither:

WindowsTerminal_NGGQtnqxgd

I'm trying to get Windows Terminal to print strikethrough, but can't get it to work in either nethod (ANSI control sequence or Unicode combining marks).

Note: I really care about the ANSI sequence issue. I've also tried the unicode path, but since this issue should probably be tracking only one thing, I intended to open it only for the ANSI control sequence issue. I can open a separate issue for the Unicode combining mark if necessary.

@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 May 26, 2020
@zadjii-msft
Copy link
Member

Huh, I would have assumed we'd have an issue for this by now. We've got issues for bold, italics, overline, but not for strikethrough. We've already got support in the buffer for this fortunately, now it's just a task of rendering them correctly.

Thanks!

@zadjii-msft zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues 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. labels May 26, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 26, 2020
@jdebp
Copy link

jdebp commented May 26, 2020

Italics is #5461. Bold is #109. Underlined is #2916. As I have already noted, there are quite a few attributes missing and Attributes.ps1 demonstrates which ones are. You don't have issues for several other missing attributes. (Note that the modern trend is to not support blink, but there are several other missing attributes.)

@egmontkob
Copy link

Note that the modern trend is to not support blink

Not sure how much it's an explicit intent not to support it (because it's annoying and should die), or just developer laziness / lack of time / being somewhere down there on the todo list. Properly implementing blinking is truly tricky – although I had quite some fun finding a nice solution for VTE.

@j4james
Copy link
Collaborator

j4james commented May 26, 2020

Note that the modern trend is to not support blink

In my (admittedly limited) experience, blinking was actually one of the most widely supported attributes, I'm assuming because it's one of the original DEC attributes. It's possible some terminals might have it disabled by default though.

Of the ANSI/ECMA attributes that we don't yet handle, the most widely supported (again in my experience) were faint, concealed, italicized, and crossed-out (in that order). A number of terminals implement doubly underlined as an alias for the regular underline, but I haven't seen many with a real double underline. And overlined doesn't seem to have that much support either, but that should be an easy one for us.

There is also a bunch of ANSI/ECMA stuff with very limited or non-existent support, like framed, encircled, the alternative fonts, and the ideogram attributes. Although the latter might actually be useful for us to pass the CJK gridlines through conpty - not sure how appropriate that is.

So I think the big ones we're missing issues for are blinking, faint, and concealed. But I'm not sure it's worth actually raising issues for each of them unless a user does it for us. That way we know it's something that users actually want.

@egmontkob
Copy link

So I think the big ones we're missing issues for are blinking, faint, and concealed.

Faint is somewhat problematic, similarly to bold/bright. The legacy behavior is that bold/bright brightens colors 0..7 into their counterpart colors 8..15, and have no effect on colors 8..15. Faint, however, has no palette colors to map to, so it's often computed from colors 0..7 using some hardwired formula. Then it's unclear if faint on colors 8..15 should revert to colors 0..7, or apply that same formula. It's also unclear if bold/bright and faint are supposed to be mutually exclusive or not.

And with the modern trend of fully separating bold typeface from the colors, faint shouldn't tamper with the colors either. This means that either faint cannot be supported in this mode, or maybe it should pick a thinner font.

See #109 (comment) and follow the second link for further thoughts.

While at it, I personally can't recall ever encountering an app that used faint. But maybe it's just me. In my opinion, it's absolutely fair not to support faint.


A new style on the rise is curly (maybe dotted and dashed, too) and colored underlines, mainly for spell checking purposes. Supported at least by Kitty, VTE, hterm, mintty, iTerm2; probably a few more. See e.g. https://gitlab.com/gnachman/iterm2/-/issues/6382. Much more useful than blinking, faint or concealed – IMHO.

@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 May 27, 2020
@jdebp
Copy link

jdebp commented May 28, 2020

Not supporting blink may well be unconscious in some cases. But the reasons to consciously not support it are much the same as the arguments for not having a blinking cursor. The Kitty underline subparameters are already in #2916 which I listed. Overline is #6000. I'll add that to the attributes test script when I get a moment.

DHowett pushed a commit that referenced this issue Jul 1, 2020
This is essentially a rewrite of the
`TerminalDispatch::SetGraphicsRendition` method, bringing it into closer
alignment with the `AdaptDispatch` implementation, simplifying the
`ITerminalApi` interface, and making the code easier to extend. It adds
support for a number of attributes which weren't previously implemented.

REFERENCES

* This is a mirror of the `AdaptDispatch` refactoring in PR #5758.
* The closer alignment with `AdaptDispatch` is a small step towards
  solving issue #3849.
* The newly supported attributes should help a little with issues #5461
  (italics) and #6205 (strike-through).

DETAILS

I've literally copied and pasted the `SetGraphicsRendition`
implementation from `AdaptDispatch` into `TerminalDispatch`, with only
few minor changes:

* The `SetTextAttribute` and `GetTextAttribute` calls are slightly
  different in the `TerminalDispatch` version, since they don't return a
  pointless `success` value, and in the case of the getter, the
  `TextAttribute` is returned directly instead of by reference.
  Ultimately I'd like to move the `AdaptDispatch` code towards that way
  of doing things too, but I'd like to deal with that later as part of a
  wider refactoring of the `ConGetSet` interface.
* The `SetIndexedForeground256` and `SetIndexedBackground256` calls
  required the color indices to be remapped in the `AdaptDispatch`
  implementation, because the conhost color table is in a different
  order to the XTerm standard. `TerminalDispatch` doesn't have that
  problem, so doesn't require the mapping.
* The index color constants used in the 16-color `SetIndexedForeground`
  and `SetIndexedBackground` calls are also slightly different for the
  same reason.

VALIDATION

I cherry-picked this code on top of the #6506 and #6698 PRs, since
that's only way to really get the different color formats passed-through
to the terminal. I then ran a bunch of manual tests with various color
coverage scripts that I have, and confirmed that all the different color
formats were being rendered as expected.

Closes #6725
ghost pushed a commit that referenced this issue Jul 30, 2020
This is a refactoring of the grid line renderers, adjusting the line
widths to scale with the font size, and optimising the implementation to
cut down on the number of draw calls. It also extends the supported grid
line types to include true underlines and strike-through lines in the
style of the active font.

The main gist of the optimisation was to render the horizontal lines
with a single draw call, instead of a loop with lots of little strokes
joined together. In the case of the vertical lines, which still needed
to be handled in a loop, I've tried to move the majority of static
calculations outside the loop, so there is bit of optimisation there
too.

At the same time this code was updated to support a variable stroke
width for the lines, instead of having them hardcoded to 1 pixel. The
width is now calculated as a fraction of the font size (0.025 "em"),
which is still going to be 1 pixel wide in most typical usage, but will
scale up appropriately if you zoom in far enough.

And in preparation for supporting the SGR strike-through attribute, and
true underlines, I've extended the grid line renders with options for
handling those line types as well. The offset and thickness of the lines
is obtained from the font metrics (rounded to a pixel width, with a
minimum of one pixel), so they match the style of the font.

VALIDATION

For now we're still only rendering grid lines, and only the top and
bottom lines in the case of the DirectX renderer in Windows Terminal. So
to test, I hacked in some code to force the renderer to use all the
different options, confirming that they were working in both the GDI and
DirectX renderers.

I've tested the output with a number of different fonts, comparing it
with the same text rendered in WordPad. For the most part they match
exactly, but there can be slight differences when we adjust the font
size for grid alignment. And in the case of the GDI renderer, where
we're working with pixel heights rather than points, it's difficult to
match the sizes exactly.

This is a first step towards supporting the strike-through attribute
(#6205) and true underlines (#2915).

Closes #6911
@ghost ghost added the In-PR This issue has a related PR label Aug 1, 2020
@ghost ghost closed this as completed in #7143 Aug 1, 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 Aug 1, 2020
ghost pushed a commit that referenced this issue Aug 1, 2020
This PR adds support for the ANSI _crossed-out_ graphic rendition
attribute, which is enabled by the `SGR 9` escape sequence.

* Support for the escape sequences and storage of the attribute was
  originally added in #2917.
* Support for drawing the strikethrough effect in the grid line renderer
  was added in #7107.

Since the majority of the code required for this attribute had already
been implemented, it was just a matter of activating the
`GridLines::Strikethrough` style in the `Renderer::s_GetGridlines`
method when the `CrossedOut` attribute was set.

VALIDATION

There were already some unit tests in place in `VtRendererTest` and the
`ScreenBufferTests`, but I've also now extended the SGR tests in
`AdapterTest` to cover this attribute.

I've manually confirmed the first test case from #6205 now works as
expected in both conhost and Terminal.

Closes #6205
@ghost
Copy link

ghost commented Aug 26, 2020

🎉This issue was addressed in #7143, which has now been successfully released as Windows Terminal Preview v1.3.2382.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
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues 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
Development

Successfully merging a pull request may close this issue.

5 participants