Add support for Underlined independent of LVB_UNDERSCORE #2915
Comments
zadjii-msft
added
Product-Conhost
ghost
added
the
Needs-Triage
DHowett-MSFT
removed
the
Needs-Triage
## Summary of the Pull Request Adds support for Italics, Blinking, Invisible, CrossedOut text, THROUGH CONPTY. This does **NOT** add support for those styles to conhost or the terminal. We will store these "Extended Text Attributes" in a `TextAttribute`. When we go to render a line, we'll see if the state has changed from our previous state, and if so, we'll appropriately toggle that state with VT. Boldness has been moved from a `bool` to a single bit in these flags. Technically, now that these are stored in the buffer, we only need to make changes to the renderers to be able to support them. That's not being done as a part of this PR however. ## References See also #2915 and #2916, which are some follow-up tasks from this fix. I thought them too risky for 20H1. ## PR Checklist * [x] Closes #2554 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated <hr> * store text with extended attributes too * Plumb attributes through all the renderers * parse extended attrs, though we're not renderering them right * Render these states correctly * Add a very extensive test * Cleanup for PR * a block of PR feedback * add 512 test cases * Fix the build * Fix @carlos-zamora's suggestions * @miniksa's PR feedback
## Summary of the Pull Request Adds support for Italics, Blinking, Invisible, CrossedOut text, THROUGH CONPTY. This does **NOT** add support for those styles to conhost or the terminal. We will store these "Extended Text Attributes" in a `TextAttribute`. When we go to render a line, we'll see if the state has changed from our previous state, and if so, we'll appropriately toggle that state with VT. Boldness has been moved from a `bool` to a single bit in these flags. Technically, now that these are stored in the buffer, we only need to make changes to the renderers to be able to support them. That's not being done as a part of this PR however. ## References See also #2915 and #2916, which are some follow-up tasks from this fix. I thought them too risky for 20H1. ## PR Checklist * [x] Closes #2554 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated <hr> * store text with extended attributes too * Plumb attributes through all the renderers * parse extended attrs, though we're not renderering them right * Render these states correctly * Add a very extensive test * Cleanup for PR * a block of PR feedback * add 512 test cases * Fix the build * Fix @carlos-zamora's suggestions * @miniksa's PR feedback (cherry picked from commit dec5c11)
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
|
The diff to do this directly is pretty minimal (and it looks nicer now that @j4james has done #7107).
Nope, for the exact reason you mentioned.
There was an open discussion about how underline types stack in #2916. James asserts, about double/single underline being a trinary state,
I'm inclined to agree with this. If we don't have to worry about making underlined/doubly underlined trinary, we can close 2915 with |
|
Before
After
|
|
@DHowett I was just putting together a PR for this, but it looks like you've done it already. Should I leave this with you? As for the doubly underlined attribute, I was going to suggest we simply render that as a single underline for now, since that's what Edit: I've just done some retesting, and there aren't as many terminals treating single and double underlines the same as I thought. It's not always obvious that a double underline is being used (at least on a hidpi screen) until you zoom in. I still think it might be a reasonable temporary solution, though. |
|
I wasn’t going to do the Doubly Underlined bits, so I imagine you’re farther along than I am :) I’ll leave it to you Agree on equating doubly with singly during render for now Thanks! |
|
OK so I started with rendering the doubly underlined attribute as a simple alias for the single underline, but it occurred to me that it might not be that difficult to add a proper renderer for it with the framework we now have in place. And you can see below some examples of what I've got so far. I'm still tweaking the look, and there are some issues with the conpty pass-through that I'm not completely happy with, so in the meantime I thought it would be best if I just submit a PR with the basic underline functionality to start with. Then we can continue the doubly-underline discussion in #2916 and add a separate PR for that later.
|
ghost
added
Resolution-Fix-Committed
This PR updates the rendering of the _underlined_ graphic rendition attribute, using the style specified in the active font, instead of just reusing the grid line at the bottom of the character cell. * Support for drawing the correct underline effect in the grid line renderer was added in #7107. There was already an `ExtendedAttributes` flag defined for the underlined state, but I needed to update the `SetUnderlined` and `IsUnderlined` methods in the `TextAttribute` class to use that flag now in place of the legacy `LVB_UNDERSCORE` attribute. This enables underlines set via a VT sequence to be tracked separately from `LVB_UNDERSCORE` grid lines set via the console API. I then needed to update the `Renderer::s_GetGridlines` method to activate the `GridLines::Underline` style when the `Underlined` attribute was set. The `GridLines::Bottom` style is still triggered by the `LVB_UNDERSCORE` attribute to produce the bottom grid line effect. Validation ---------- Because this is a change from the existing behaviour, certain unit tests that were expecting the `LVB_UNDERSCORE` to be toggled by `SGR 4` and `SGR 24` have now had to be updated to check the `Underlined` flag instead. There were also some UI Automation tests that were checking for `SGR 4` mapping to `LVB_UNDERSCORE` attribute, which I've now substituted with a test of the `SGR 53` overline attribute mapping to `LVB_GRID_HORIZONTAL`. These tests only work with legacy attributes, so they can't access the extended underline state, and I thought a replacement test that covered similar ground would be better than dropping the tests altogether. As far as the visual rendering is concerned, I've manually confirmed that the VT underline sequences now draw the underline in the correct position and style, while grid lines output via the console API are still displayed in their original form. Closes #2915
|
🎉This issue was addressed in #7148, which has now been successfully released as Handy links: |
Discussed with @DHowett-MSFT, as a follow up of #2554.
We've been using LVB_UNDERSCORE for underlines in the console since Windows 10 was released. However, LVB_UNDERSCORE is more of a box-drawing feature, it draws the underline fully under the text.
We should more correctly have a separate Underline state, that draws the underline at the baseline of the text, not just under it.
In VT, underline is in a trinary state: [No Underline, Underlined, Doubly Underlined]. We should make sure to store this in the buffer appropriately.
Technically, we believe characters could have both Underlined and LVB_UNDERSCORE.
Open Questions:
The text was updated successfully, but these errors were encountered: